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

@@ -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

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
+}

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)
+	}
+}