From fb492da7e78c6f2a18aa73b45e8c39e22d731126 Mon Sep 17 00:00:00 2001 From: jochens Date: Fri, 13 Nov 2026 00:17:11 +0100 Subject: [PATCH] H1 engine-stop-at-failure: a module's apply stops at its first failed resource MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 9 +- internal/apply/apply.go | 10 ++ internal/apply/apply_test.go | 144 +++++++++++++---------- internal/apply/module_results.go | 7 ++ internal/apply/refused_directory_test.go | 6 +- 5 files changed, 105 insertions(+), 71 deletions(-) diff --git a/cmd/mesh-host/probe_test.go b/cmd/mesh-host/probe_test.go index 357aa1e0..be454719 100644 --- a/cmd/mesh-host/probe_test.go +++ b/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 - // leaves it to a second pass. + // The file first, as the pacman module declares it: a module's apply stops at its first failed resource + // (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) diff --git a/internal/apply/apply.go b/internal/apply/apply.go index a6c00f9a..25e555c7 100644 --- a/internal/apply/apply.go +++ b/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 diff --git a/internal/apply/apply_test.go b/internal/apply/apply_test.go index b5b62294..506a81a4 100644 --- a/internal/apply/apply_test.go +++ b/internal/apply/apply_test.go @@ -182,77 +182,93 @@ func TestARenameToTheSamePathDoesNotDeleteTheNewFile(t *testing.T) { } } -func TestAFailedStepFailsTheApplyAndTheRestIsStillAttempted(t *testing.T) { - // The apply fails, names the resource, and carries what did happen — because the machine is - // in whatever state the apply reached and the only honest thing to hand back is that list. - // - // **And everything is attempted.** It used to stop at the first failure, which made one - // 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. +// A failed resource fails the apply and names itself, and what else is attempted depends on whose it is +// (novox/hq 04-ISSUES/011; design 42 zfs-owns-zfs §1, ADR 0679 §16 (a)): the resources of no module are all +// 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. +func TestAFailedResourceFailsTheApplyAndWhatFollowsIsAttemptedByWhoseItIs(t *testing.T) { dir := t.TempDir() - blocker := filepath.Join(dir, "blocker") - if err := os.WriteFile(blocker, []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 { + for _, name := range []string{"blocker", "one", "two"} { + 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":"first","type":"directory","path":"`+filepath.Join(dir, "one")+`"}, - {"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") + file := func(id, name string) string { + return `{"id":"` + id + `","type":"file","path":"` + filepath.Join(dir, name) + `","content":"a\n"}` } - var applyErr *Error - if !errors.As(err, &applyErr) { - t.Fatalf("got %T", err) + unwritable := func(id, name string) string { // a file below a file: it cannot be made + return `{"id":"` + id + `","type":"file","path":"` + filepath.Join(dir, name, "x.conf") + `","content":"a\n"}` } - if applyErr.Others != 1 { - t.Errorf("the failure says %d others also failed, and one did", applyErr.Others) + doomed := func(id, name string) string { + return `{"id":"` + id + `","type":"directory","path":"` + filepath.Join(dir, name) + `"}` } - if !strings.Contains(err.Error(), "one other resource also failed") { - t.Errorf("the message does not say others failed: %v", err) + exists := func(name string) bool { + _, 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) + } + }) } } diff --git a/internal/apply/module_results.go b/internal/apply/module_results.go index affe5fbb..5156ecc7 100644 --- a/internal/apply/module_results.go +++ b/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, "; ")} } diff --git a/internal/apply/refused_directory_test.go b/internal/apply/refused_directory_test.go index 997cf288..f6ae9d0e 100644 --- a/internal/apply/refused_directory_test.go +++ b/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, - // and the directory the other. + // A refused file has no outcome: it is a failure. Declared first, it is the failure the apply reports, and the + // 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 != "" {