Merge pull request 'A failed run-once process runs again on every apply until it passes (hq issue 467)' (#81) from fix/467-a-failed-step-runs-again into main

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).
@@ -670,6 +674,24 @@ 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. 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 outcome.refused {
+					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/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)

internal/apply/step_failed_test.go

@@ -0,0 +1,254 @@
+package apply
+
+import (
+	"context"
+	"errors"
+	"net/http"
+	"net/http/httptest"
+	"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
+	storeGone         bool // whether the bundle's store answers
+}
+
+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"})
+	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) {
+	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)
+	}
+}
+
+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)
+		}
+	}
+}