H1 engine-stop-at-failure: a module's apply stops at its first failed resource

A failed resource of a module gates the rest of that module, which is reported
skipped with the failed resource named; the module's result names each one left.
A resource of no module, and other modules, go on as before. One table test
replaces two in apply_test.go; two fixtures adapted to the rule.

Builds: design 42 zfs-owns-zfs §1 (ADR 0679 §16 (a)), A-22

cmd/mesh-host/probe_test.go

@@ -88,11 +88,12 @@ func TestThePackagesGoInWhenTheApplyThatRestoresThePackageManagerWroteIt(t *test
 	conf := filepath.Join(dir, "pacman.conf")
 	pacmanAnsweringOnce(t, conf)
 	opts := options{state: filepath.Join(dir, "state.json")}
-	// The package first: within one pass it is asked about before the file is written, which is the order that
+	// The file first, as the pacman module declares it: a module's apply stops at its first failed resource
-	// leaves it to a second pass.
+	// (design 42 zfs-owns-zfs §1, answer A-26), so the file that makes the package manager answer comes before the
+	// package that needs it.
 	raw := []byte(`{"declaration":1,"resources":[` +
-		`{"id":"pacman.base","type":"package","package":"base"},` +
+		`{"id":"pacman.conf","type":"file","path":"` + conf + `","content":"[options]\n"},` +
-		`{"id":"pacman.conf","type":"file","path":"` + conf + `","content":"[options]\n"}]}`)
+		`{"id":"pacman.base","type":"package","package":"base"}]}`)
 
 	report := applyAndKeep(context.Background(), opts, raw, nil, nil, nil)
 

internal/apply/apply.go

@@ -1162,6 +1162,16 @@ func applyScoped(
 			if p, ok := resource.(*declaration.Process); ok && p.RunOnce {
 				gates = true
 			}
+			// **A module's apply stops at its first failed resource** (design 42 zfs-owns-zfs §1, ADR 0679 §16 (a)): any
+			// other resource of a module that failed gates the rest of that module, which is reported skipped with this
+			// reason. What it applied before stays applied; other modules, and a resource of no module, go on as before.
+			if module, ours := d.ModuleOfID(resource.Identity()); ours && !gates {
+				failed.Gated = true
+				gated[module] = true
+				gatedWhy[module] = fmt.Sprintf("%s failed, so nothing more of this module was attempted", resource.Identity())
+				log(fmt.Sprintf("  gated %s.*: %s failed, so the rest of it was not attempted", module, resource.Identity()))
+				continue
+			}
 			if gates {
 				failed.Gated = true
 				// **A step that failed has not completed, whatever it did before** (novox/hq issue 467). A

internal/apply/apply_test.go

@@ -182,77 +182,93 @@ func TestARenameToTheSamePathDoesNotDeleteTheNewFile(t *testing.T) {
 	}
 }
 
-func TestAFailedStepFailsTheApplyAndTheRestIsStillAttempted(t *testing.T) {
+// A failed resource fails the apply and names itself, and what else is attempted depends on whose it is
-	// The apply fails, names the resource, and carries what did happen — because the machine is
+// (novox/hq 04-ISSUES/011; design 42 zfs-owns-zfs §1, ADR 0679 §16 (a)): the resources of no module are all
-	// in whatever state the apply reached and the only honest thing to hand back is that list.
+// attempted, so one broken resource holds no machine hostage and every failure is counted, not just the first;
-	//
+// a module's apply stops at its first failed resource, reports each one left, and no other module is touched.
-	// **And everything is attempted.** It used to stop at the first failure, which made one
+func TestAFailedResourceFailsTheApplyAndWhatFollowsIsAttemptedByWhoseItIs(t *testing.T) {
-	// broken resource hold the whole machine hostage: a module declaring a package that does not
-	// exist meant every module after it was never applied, for ever
-	// (novox/hq 04-ISSUES/011). The case for stopping was that a later resource may depend on an
-	// earlier one — and it still may, and it then fails its own check and is reported, which is
-	// more information than not attempting it.
 	dir := t.TempDir()
-	blocker := filepath.Join(dir, "blocker")
+	for _, name := range []string{"blocker", "one", "two"} {
-	if err := os.WriteFile(blocker, []byte("i am a file\n"), 0o644); err != nil {
+		if err := os.WriteFile(filepath.Join(dir, name), []byte("i am a file\n"), 0o644); err != nil {
-		t.Fatal(err)
-	}
-
-	d := parse(t, `{"declaration":1,"resources":[
-	  {"id":"fine","type":"file","path":"`+filepath.Join(dir, "fine.conf")+`","content":"a\n"},
-	  {"id":"doomed","type":"directory","path":"`+blocker+`"},
-	  {"id":"after","type":"file","path":"`+filepath.Join(dir, "after.conf")+`","content":"b\n"}
-	]}`)
-
-	_, _, err := Apply(context.Background(), archHost(t), d, store.State{}, store.OriginCarried, noServices, nil, nil)
-	if err == nil {
-		t.Fatal("an impossible resource did not fail the apply")
-	}
-
-	var applyErr *Error
-	if !errors.As(err, &applyErr) {
-		t.Fatalf("expected an apply error, got %T", err)
-	}
-	if applyErr.Resource != "doomed" {
-		t.Errorf("the failure names %q, not the resource that failed", applyErr.Resource)
-	}
-	// Everything that worked is in the report, before and after the failure.
-	if len(applyErr.Done.Outcomes) != 2 {
-		t.Errorf("the error does not carry what was applied: %+v", applyErr.Done.Outcomes)
-	}
-	if _, err := os.Stat(filepath.Join(dir, "after.conf")); err != nil {
-		t.Error("a resource after the failing one was never attempted, so one broken module " +
-			"still blocks every module after it")
-	}
-}
-
-func TestEveryFailureIsCountedNotJustTheFirst(t *testing.T) {
-	// "One thing failed" and "eleven things failed" are different machines, and the first line is
-	// what somebody reads.
-	dir := t.TempDir()
-	for _, name := range []string{"one", "two"} {
-		if err := os.WriteFile(filepath.Join(dir, name), []byte("a file\n"), 0o644); err != nil {
 			t.Fatal(err)
 		}
 	}
-	d := parse(t, `{"declaration":1,"resources":[
+	file := func(id, name string) string {
-	  {"id":"first","type":"directory","path":"`+filepath.Join(dir, "one")+`"},
+		return `{"id":"` + id + `","type":"file","path":"` + filepath.Join(dir, name) + `","content":"a\n"}`
-	  {"id":"second","type":"directory","path":"`+filepath.Join(dir, "two")+`"}
-	]}`)
-
-	_, _, err := Apply(context.Background(), archHost(t), d, store.State{}, store.OriginCarried, noServices, nil, nil)
-	if err == nil {
-		t.Fatal("two impossible resources did not fail the apply")
 	}
-	var applyErr *Error
+	unwritable := func(id, name string) string { // a file below a file: it cannot be made
-	if !errors.As(err, &applyErr) {
+		return `{"id":"` + id + `","type":"file","path":"` + filepath.Join(dir, name, "x.conf") + `","content":"a\n"}`
-		t.Fatalf("got %T", err)
 	}
-	if applyErr.Others != 1 {
+	doomed := func(id, name string) string {
-		t.Errorf("the failure says %d others also failed, and one did", applyErr.Others)
+		return `{"id":"` + id + `","type":"directory","path":"` + filepath.Join(dir, name) + `"}`
 	}
-	if !strings.Contains(err.Error(), "one other resource also failed") {
+	exists := func(name string) bool {
-		t.Errorf("the message does not say others failed: %v", err)
+		_, err := os.Stat(filepath.Join(dir, name))
+		return err == nil
+	}
+	for _, tc := range []struct {
+		name      string
+		resources []string
+		failed    string
+		others    int
+		applied   []string // files that must exist
+		left      []string // files that must not, reported skipped
+		detail    string   // what the module's result says, when it is one module's
+	}{
+		{"of no module, the rest is attempted", []string{file("fine", "fine.conf"), doomed("doomed", "blocker"), file("after", "after.conf")},
+			"doomed", 0, []string{"fine.conf", "after.conf"}, nil, ""},
+		{"of no module, every failure is counted", []string{doomed("first", "one"), doomed("second", "two")},
+			"first", 1, nil, nil, ""},
+		{"of a module, it stops there and names each one left",
+			[]string{file("m.first", "first.conf"), unwritable("m.second", "blocker"), file("m.third", "third.conf"), file("m.fourth", "fourth.conf"), file("o.other", "other.conf")},
+			"m.second", 0, []string{"first.conf", "other.conf"}, []string{"third.conf", "fourth.conf"},
+			"m.second failed"},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			d := parse(t, `{"declaration":1,"resources":[`+strings.Join(tc.resources, ",")+`]}`)
+			report, _, err := Apply(context.Background(), archHost(t), d, store.State{}, store.OriginCarried, noServices, nil, nil)
+			var applyErr *Error
+			if !errors.As(err, &applyErr) {
+				t.Fatalf("expected an apply error, got %v", err)
+			}
+			if applyErr.Resource != tc.failed || applyErr.Others != tc.others {
+				t.Errorf("the failure names %q with %d others, want %q with %d", applyErr.Resource, applyErr.Others, tc.failed, tc.others)
+			}
+			if tc.others == 1 && !strings.Contains(err.Error(), "one other resource also failed") {
+				t.Errorf("the message does not say others failed: %v", err)
+			}
+			for _, name := range tc.applied {
+				if !exists(name) {
+					t.Errorf("%s was not applied", name)
+				}
+			}
+			for _, name := range tc.left {
+				if exists(name) {
+					t.Errorf("%s was applied after its module failed", name)
+				}
+			}
+			if tc.detail == "" {
+				return
+			}
+			outcomes := applyErr.Done.Outcomes
+			skipped := 0
+			for _, o := range outcomes {
+				if o.Action == "skipped" {
+					skipped++
+				}
+			}
+			if skipped != len(tc.left) {
+				t.Errorf("%d resources reported skipped, want %d: %+v", skipped, len(tc.left), outcomes)
+			}
+			result := ModuleResults(d, report, err)["m"]
+			if !result.Failed || !strings.Contains(result.Detail, tc.detail) ||
+				!strings.Contains(result.Detail, "m.third") || !strings.Contains(result.Detail, "m.fourth") {
+				t.Errorf("the module's result does not name the failed resource and each one left: %+v", result)
+			}
+			if other := ModuleResults(d, report, err)["o"]; other.Failed {
+				t.Errorf("another module was failed by it: %+v", other)
+			}
+		})
 	}
 }
 

internal/apply/module_results.go

@@ -41,6 +41,7 @@ func ModuleResults(d *declaration.Declaration, report Report, err error) map[str
 		outcomes = append(append([]Outcome(nil), outcomes...), failed.Done.Outcomes...)
 	}
 	skipped := map[string]string{}
+	left := map[string][]string{}
 	for _, o := range outcomes {
 		module, ok := d.ModuleOfID(o.ID)
 		if !ok {
@@ -48,6 +49,7 @@ func ModuleResults(d *declaration.Declaration, report Report, err error) map[str
 		}
 		if o.Action == "skipped" {
 			skipped[module] = o.Detail
+			left[module] = append(left[module], o.ID)
 			continue
 		}
 		attempted[o.ID] = true
@@ -92,6 +94,11 @@ func ModuleResults(d *declaration.Declaration, report Report, err error) map[str
 		}
 	}
 	for module, why := range reasons {
+		// Each resource left unapplied is named (design 42 zfs-owns-zfs §1).
+		if ids := left[module]; len(ids) > 0 {
+			sort.Strings(ids)
+			why = append(why, "left unapplied: "+strings.Join(ids, ", "))
+		}
 		sort.Strings(why)
 		out[module] = ModuleResult{Failed: true, Detail: strings.Join(why, "; ")}
 	}

internal/apply/refused_directory_test.go

@@ -118,10 +118,10 @@ func TestAFileIsJudgedByTheSameGuardAsADirectory(t *testing.T) {
 	if _, statErr := os.Stat(filepath.Join(home, "data", "notes.conf")); statErr == nil {
 		t.Error("a file was written below a directory the engine refused")
 	}
-	// A refused file has no outcome: it is a failure. Declared first, it is the failure the apply reports,
+	// A refused file has no outcome: it is a failure. Declared first, it is the failure the apply reports, and the
-	// and the directory the other.
+	// directory after it is left unapplied, as the rest of its module is (design 42 zfs-owns-zfs §1).
 	var applyErr *Error
-	if !errors.As(err, &applyErr) || applyErr.Resource != "notes.conf" || applyErr.Others != 1 {
+	if !errors.As(err, &applyErr) || applyErr.Resource != "notes.conf" || applyErr.Others != 0 {
 		t.Errorf("the file below a refused directory is not among the failures: %v", err)
 	}
 	if o := outcomeOf(report, "notes.conf"); o.ID != "" {