From c957f1ec5e9cec4f95811a7fc9fe6de0af406099 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 11:07:17 +0200 Subject: [PATCH 1/2] Run a failed run-once process again on every apply until it passes (novox/hq issue 467) A failure recorded nothing, so the record of the step's last success stayed and the next apply skipped it, opening ADR 0136's gate on what it had just refused. The record is kept, for removal, with what it was run as cleared. --- internal/apply/apply.go | 16 +++ internal/apply/plan.go | 12 ++ internal/apply/step_failed_test.go | 197 +++++++++++++++++++++++++++++ 3 files changed, 225 insertions(+) create mode 100644 internal/apply/step_failed_test.go diff --git a/internal/apply/apply.go b/internal/apply/apply.go index e5ff726..4521d5a 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -670,6 +670,22 @@ func ApplyMindingWindows( } if gates { failed.Gated = true + // **A step that failed has not completed, whatever it did before** (novox/hq issue 467). A + // run-once process is judged done by its record: what it was last run as, against what it is + // declared as now, and run again besides when something it names under restart-on changed + // in this same apply. A failure records nothing, so the record of its last success stayed, + // and the next apply, in which nothing it reads changed again, found it done, skipped it, + // and let ADR 0136's gate open on what it had just refused. Its record is kept — where its + // unit and bundle are is what removes them when it is undeclared — with what it was run as + // cleared, so every apply runs it again until it passes, and passing records its success + // as before. A run-once container needs none of this: what it reads is part of what it is + // run as, so a refused input keeps it from matching its last success. + if p, ok := resource.(*declaration.Process); ok && p.RunOnce { + if rec, ok := known.Find(resource.Identity()); ok && rec.Wrote != "" { + rec.Wrote = "" + known.Record(rec) + } + } // **A module's step gates that module, not the machine** (novox/hq ADR 0136). // // Stopping the whole apply is what this loop's own comment above calls holding a diff --git a/internal/apply/plan.go b/internal/apply/plan.go index bbd42de..c632b88 100644 --- a/internal/apply/plan.go +++ b/internal/apply/plan.go @@ -270,6 +270,12 @@ func planned(r declaration.Resource, d *declaration.Declaration, known store.Sta step.Verb, step.Why = "create", "no record of it on this node" return step } + // A run-once process whose last run failed has its record cleared of what it was run as (novox/hq issue + // 467), and the apply runs it again. + if was.Wrote == "" && isStep(r) { + step.Verb, step.Why = "update", "its last run did not complete, so it runs again" + return step + } if want := wouldWrite(r); want != "" && was.Wrote != "" && want != was.Wrote { step.Verb, step.Why = "update", "the declaration changed since this host applied it" return step @@ -370,3 +376,9 @@ func wouldWrite(r declaration.Resource) string { } return digestOf(f.Content) } + +// isStep says whether a resource is a run-once process, whose record a failed run clears. +func isStep(r declaration.Resource) bool { + p, ok := r.(*declaration.Process) + return ok && p.RunOnce +} diff --git a/internal/apply/step_failed_test.go b/internal/apply/step_failed_test.go new file mode 100644 index 0000000..19d7d66 --- /dev/null +++ b/internal/apply/step_failed_test.go @@ -0,0 +1,197 @@ +package apply + +import ( + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/novox/mesh-host/internal/declaration" + "github.com/novox/mesh-host/internal/store" +) + +// A step that failed runs again until it passes (novox/hq issue 467, ADR 0136). +// +// The shape the issue was found in (mesh-catalog #205, issue 452): a module writes a value into a +// file, a run-once step judges the file, and the module's next resource takes the same value. A bad +// value fails the step and ADR 0136's gate skips the rest of the module. Before this, the next apply +// found the file unchanged and the step's record still holding its last success, so the step was not +// run, the gate stayed open and the bad value was written with no error. + +type aStepBed struct { + t *testing.T + dir, body, digest string + starts int // how many times the step's unit was started + refuse bool // whether the step fails when started +} + +func newStepBed(t *testing.T) *aStepBed { + units, bundles := t.TempDir(), t.TempDir() + wasUnits, wasBundles := unitDir, daemonRoot + unitDir, daemonRoot = units, bundles + t.Cleanup(func() { unitDir, daemonRoot = wasUnits, wasBundles }) + body, digest := anArchive(t, map[string]string{"check": "#!/bin/sh\n"}) + return &aStepBed{t: t, dir: t.TempDir(), body: serving(t, body), digest: digest} +} + +func (b *aStepBed) run(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) + if name == "systemctl" && len(args) > 1 && args[0] == "start" && args[1] == "mod-check.service" { + b.starts++ + if b.refuse { + return "", errors.New("exit status 1") + } + } + return "", nil +} + +// apply declares value into mod.conf, the step mod.check reading it, and mod.after taking it. +func (b *aStepBed) apply(known store.State, value string) (Report, store.State, error) { + b.t.Helper() + return Apply(context.Background(), archHost(b.t), b.declared(value), known, store.OriginDeclared, b.run, nil, nil) +} + +func (b *aStepBed) declared(value string) *declaration.Declaration { + b.t.Helper() + return declare(b.t, `{"id":"mod.conf","type":"file","path":"`+b.dir+`/mod.conf","content":"value=`+value+`\n"},`+ + `{"id":"mod.check","type":"process","name":"mod-check","source":"`+b.body+`","digest":"`+b.digest+`",`+ + `"run":["./check"],"run-once":true,"restart-on":["mod.conf"]},`+ + `{"id":"mod.after","type":"file","path":"`+b.dir+`/after","content":"`+value+`\n"}`) +} + +func (b *aStepBed) after() string { + got, err := os.ReadFile(filepath.Join(b.dir, "after")) + if err != nil { + return "" + } + return strings.TrimSpace(string(got)) +} + +func skippedIn(r Report, id string) bool { + for _, o := range r.Outcomes { + if o.ID == id && o.Action == "skipped" { + return true + } + } + return false +} + +func TestAStepThatFailedRunsAgainAndKeepsItsModuleGated(t *testing.T) { + b := newStepBed(t) + _, known, err := b.apply(store.State{}, "good") + if err != nil || b.starts != 1 || b.after() != "good" { + t.Fatalf("apply 1: err %v, starts %d, after %q", err, b.starts, b.after()) + } + + // Apply 2: the value turns bad, the file changes, the step runs and refuses it. + b.refuse = true + report, known, err := b.apply(known, "bad") + if err == nil || b.starts != 2 || !skippedIn(report, "mod.after") || b.after() != "good" { + t.Fatalf("apply 2: err %v, starts %d, skipped %v, after %q", err, b.starts, skippedIn(report, "mod.after"), b.after()) + } + + // The plan says so before the apply does. + for _, step := range Plan(b.declared("bad"), known, store.OriginDeclared) { + if step.ID == "mod.check" && step.Verb != "update" { + t.Errorf("the plan says %s for a step whose last run failed: %s", step.Verb, step.Why) + } + } + + // Apply 3: nothing changed since. The step must run again and refuse again, and the value must + // still not reach the module's later resource. + report, known, err = b.apply(known, "bad") + if b.starts != 3 { + t.Errorf("apply 3: a step that failed was not run again (started %d times, want 3)", b.starts) + } + if err == nil { + t.Error("apply 3: the failed step was not reported as a failure") + } + if !skippedIn(report, "mod.after") { + t.Error("apply 3: the module's later resource was not skipped") + } + if b.after() != "good" { + t.Errorf("apply 3: the refused value reached the module's later resource: %q", b.after()) + } + + // Apply 4: the step passes (the cause outside was fixed): it runs, records its success, and the + // module's later resource takes the value. + b.refuse = false + _, known, err = b.apply(known, "bad") + if err != nil || b.starts != 4 || b.after() != "bad" { + t.Fatalf("apply 4: err %v, starts %d, after %q", err, b.starts, b.after()) + } + + // Apply 5: passed, and nothing changed: it is not run again. + if _, _, err = b.apply(known, "bad"); err != nil || b.starts != 4 { + t.Errorf("apply 5: err %v, a step that passed after failing ran again (started %d times, want 4)", err, b.starts) + } +} + +func TestAStepThatNeverFailedRunsNoMoreThanBefore(t *testing.T) { + // Every node runs many steps: one that passes must run once per change, and not on every apply. + b := newStepBed(t) + _, known, err := b.apply(store.State{}, "one") + for i := 0; i < 3 && err == nil; i++ { + _, known, err = b.apply(known, "one") + } + if err != nil || b.starts != 1 { + t.Fatalf("err %v, started %d times over four applies of one value, want 1", err, b.starts) + } + if _, known, err = b.apply(known, "two"); err != nil || b.starts != 2 { + t.Fatalf("err %v, a changed value started it %d times in all, want 2", err, b.starts) + } + if _, _, err = b.apply(known, "two"); err != nil || b.starts != 2 { + t.Errorf("err %v, started %d times, want 2", err, b.starts) + } +} + +func TestAContainerStepThatFailedRunsAgain(t *testing.T) { + // A run-once container gates its module the same way (novox/hq ADR 0052, ADR 0136). It already ran + // again before issue 467 was fixed, because what it is declared with is part of what it is run as: + // held here so it stays that way. + runs, refuse := 0, false + run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) + switch args[0] { + case "info": + return "27.0\n", nil + case "run": + if nameOf(args) == "prepare" { + runs++ + if refuse { + return "", errors.New("exit status 1") + } + } + return "deadbeef\n", nil + } + return "", nil + } + declared := func(value string) *declaration.Declaration { + return parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"mod.prepare","type":"container","name":"prepare","image":"`+pinned+`","run-once":true,"env":{"VALUE":"`+value+`"}} + ]}`) + } + d := declared("good") + _, known, err := Apply(context.Background(), archHost(t), d, store.State{}, store.OriginCarried, run, nil, nil) + if err != nil || runs != 1 { + t.Fatalf("apply 1: err %v, runs %d", err, runs) + } + // Declared with a value it refuses: run, refused. Then the same declaration again: run again. + refuse = true + d = declared("bad") + if _, known, err = Apply(context.Background(), archHost(t), d, known, store.OriginCarried, run, nil, nil); err == nil || runs != 2 { + t.Fatalf("apply 2: err %v, runs %d", err, runs) + } + if _, known, err = Apply(context.Background(), archHost(t), d, known, store.OriginCarried, run, nil, nil); err == nil || runs != 3 { + t.Errorf("apply 3: a failed step was not run again: err %v, runs %d (want 3)", err, runs) + } + refuse = false + if _, known, err = Apply(context.Background(), archHost(t), d, known, store.OriginCarried, run, nil, nil); err != nil || runs != 4 { + t.Fatalf("apply 4: err %v, runs %d", err, runs) + } + if _, _, err = Apply(context.Background(), archHost(t), d, known, store.OriginCarried, run, nil, nil); err != nil || runs != 4 { + t.Errorf("apply 5: a step that passed ran again: err %v, runs %d (want 4)", err, runs) + } +} From 0ec96a048eb224d16b7c3cf80a95d130c340e1a5 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 11:12:15 +0200 Subject: [PATCH 2/2] Clear a step's success only when it was started and failed, and say updated when it passes again (novox/hq issue 467) A bundle that could not be fetched refused nothing; clearing for it would rerun every step on a node once the artifact store came back from an outage. --- internal/apply/apply.go | 10 ++++- internal/apply/process.go | 4 +- internal/apply/step_failed_test.go | 59 +++++++++++++++++++++++++++++- 3 files changed, 69 insertions(+), 4 deletions(-) diff --git a/internal/apply/apply.go b/internal/apply/apply.go index 4521d5a..efe8ed0 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -60,6 +60,10 @@ type Outcome struct { kept, keptMode, keptOwner string // stateless is a service whose unit's lifecycle is the machine's (novox/hq ADR 0117). stateless bool + // refused is a run-once process that was started and did not complete (novox/hq issue 467): only + // that clears what its record says it was run as. A step that failed before it was started — its + // bundle not fetched while the store was away — refused nothing, and its last success stands. + refused bool // scope and user are, for a service, whose manager it was applied through (novox/hq ADR 0177). scope, user string // found is, for a service, its unit as the host first found it (novox/hq ADR 0118). @@ -678,9 +682,11 @@ func ApplyMindingWindows( // and let ADR 0136's gate open on what it had just refused. Its record is kept — where its // unit and bundle are is what removes them when it is undeclared — with what it was run as // cleared, so every apply runs it again until it passes, and passing records its success - // as before. A run-once container needs none of this: what it reads is part of what it is + // as before. Only a step that was started and failed: one whose bundle could not be fetched + // refused nothing, and clearing it would run every step on the machine again once the + // artifact store came back from an outage. A run-once container needs none of this: what it reads is part of what it is // run as, so a refused input keeps it from matching its last success. - if p, ok := resource.(*declaration.Process); ok && p.RunOnce { + if outcome.refused { if rec, ok := known.Find(resource.Identity()); ok && rec.Wrote != "" { rec.Wrote = "" known.Record(rec) diff --git a/internal/apply/process.go b/internal/apply/process.go index 917e79b..bc5a3bf 100644 --- a/internal/apply/process.go +++ b/internal/apply/process.go @@ -172,10 +172,12 @@ func applyProcess(ctx context.Context, r *declaration.Process, run Runner, return out, err } if _, err := run(ctx, "systemctl", "start", r.Name+".service"); err != nil { + out.refused = true // started and did not complete: run again until it does (novox/hq issue 467) return out, fmt.Errorf("the %s step did not complete (journalctl -u %s.service says why): %w", r.Name, r.Name, err) } + // Updated whenever it has a record, whose Wrote a failed run cleared (novox/hq issue 467). out.Action = "created" - if previous.Wrote != "" { + if previous.Wrote != "" || previous.ID != "" { out.Action = "updated" } out.Detail = fmt.Sprintf("%d file(s), step completed", written) diff --git a/internal/apply/step_failed_test.go b/internal/apply/step_failed_test.go index 19d7d66..821df39 100644 --- a/internal/apply/step_failed_test.go +++ b/internal/apply/step_failed_test.go @@ -3,6 +3,8 @@ package apply import ( "context" "errors" + "net/http" + "net/http/httptest" "os" "path/filepath" "strings" @@ -25,6 +27,7 @@ type aStepBed struct { dir, body, digest string starts int // how many times the step's unit was started refuse bool // whether the step fails when started + storeGone bool // whether the bundle's store answers } func newStepBed(t *testing.T) *aStepBed { @@ -33,7 +36,18 @@ func newStepBed(t *testing.T) *aStepBed { unitDir, daemonRoot = units, bundles t.Cleanup(func() { unitDir, daemonRoot = wasUnits, wasBundles }) body, digest := anArchive(t, map[string]string{"check": "#!/bin/sh\n"}) - return &aStepBed{t: t, dir: t.TempDir(), body: serving(t, body), digest: digest} + b := &aStepBed{t: t, dir: t.TempDir(), digest: digest} + // The bundle's store, which can be made not to answer. + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + if b.storeGone { + http.NotFound(w, nil) + return + } + _, _ = w.Write(body) + })) + t.Cleanup(server.Close) + b.body = server.URL + "/check.tar.gz" + return b } func (b *aStepBed) run(ctx context.Context, name string, args ...string) (string, error) { @@ -195,3 +209,46 @@ func TestAContainerStepThatFailedRunsAgain(t *testing.T) { t.Errorf("apply 5: a step that passed ran again: err %v, runs %d (want 4)", err, runs) } } + +func TestAStepWhoseBundleCouldNotBeFetchedIsNotRunAgainForIt(t *testing.T) { + // A fetch that fails is a failure of the step, and gates its module, but the step was never started: + // it did not refuse anything, and its last success stands. Cleared for it, every step on a machine + // would run again once the artifact store came back from an outage. + b := newStepBed(t) + _, known, err := b.apply(store.State{}, "good") + if err != nil || b.starts != 1 { + t.Fatalf("apply 1: err %v, starts %d", err, b.starts) + } + b.storeGone = true + report, known, err := b.apply(known, "good") + if err == nil || !skippedIn(report, "mod.after") || b.starts != 1 { + t.Fatalf("store gone: err %v, skipped %v, starts %d", err, skippedIn(report, "mod.after"), b.starts) + } + b.storeGone = false + report, _, err = b.apply(known, "good") + if err != nil || b.starts != 1 { + t.Errorf("store back: err %v, a step that passed and was never started since ran again (started %d times, want 1)", err, b.starts) + } + for _, o := range report.Outcomes { + if o.ID == "mod.check" && o.Action != "unchanged" { + t.Errorf("store back: the step reports %s: %s", o.Action, o.Detail) + } + } +} + +func TestAStepPassingAfterAFailureSaysItWasUpdated(t *testing.T) { + b := newStepBed(t) + _, known, _ := b.apply(store.State{}, "good") + b.refuse = true + _, known, _ = b.apply(known, "bad") + b.refuse = false + report, _, err := b.apply(known, "bad") + if err != nil { + t.Fatal(err) + } + for _, o := range report.Outcomes { + if o.ID == "mod.check" && o.Action != "updated" { + t.Errorf("a step with a record that passed after failing says %s, want updated", o.Action) + } + } +}