Merge pull request 'A run-once step names what it reads and runs again when it changed (ADR 0099)' (#18) from multiple-fixes into main

internal/apply/runonce_test.go

@@ -3,6 +3,7 @@ package apply
 import (
 	"context"
 	"errors"
+	"path/filepath"
 	"strings"
 	"testing"
 
@@ -216,18 +217,102 @@ func TestARunOnceStepReRunsWhenItsDeclarationChanged(t *testing.T) {
 	}
 }
 
-func TestARunOnceContainerCannotAlsoDeclareRestartOn(t *testing.T) {
+func TestARunOnceStepRunsAgainWhenWhatItReadsChanged(t *testing.T) {
-	// restart-on brings a running container back when a file it read changed; a run-once step does
+	// A step that fetches a fact from a provider names the binding file it reads. What a
-	// not stay running. The two lifecycles contradict, so the parser refuses the pair rather than
+	// container reads is part of its digest, so when the provider moved and the mesh rewrote the
-	// silently resolving to one.
+	// file, the step's marker no longer matches and it runs again (novox/hq ADR 0099) — the same
-	_, err := declaration.Parse([]byte(`{"declaration":1,"resources":[
+	// rule that recreates a running container, read as "again" for a step.
-	  {"id":"f","type":"file","path":"/tmp/x","content":"y"},
+	dir := t.TempDir()
-	  {"id":"seed","type":"container","name":"seed","image":"` + pinned + `","run-once":true,"restart-on":["f"]}
+	env := filepath.Join(dir, "acme.env")
-	]}`))
+	d := parseTrusted(t, `{"declaration":1,"resources":[
-	if err == nil {
+	  {"id":"env","type":"file","path":"`+env+`","content":"ACME_ROOTS=https://10.0.0.2/roots.pem\n"},
-		t.Fatal("a run-once container that also declared restart-on was accepted")
+	  {"id":"trust","type":"container","name":"trust","image":"`+pinned+`","run-once":true,"restart-on":["env"]}
+	]}`)
+	// The record from when the provider was elsewhere: the step's digest against the old file.
+	was := map[string]string{"env": declaredDigest(&declaration.File{Content: "ACME_ROOTS=https://10.0.0.1/roots.pem\n"})}
+	known := store.State{}
+	known.Record(store.Applied{ID: "trust", Type: "container", Origin: store.OriginCarried, Target: "trust",
+		Wrote: containerSpec(d.Resources[1].(*declaration.Container), was)})
+
+	var ran bool
+	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		switch args[0] {
+		case "info":
+			return "27.0\n", nil
+		case "run":
+			ran = true
+			return "", nil
 		}
-	if !strings.Contains(err.Error(), "run-once") {
+		return "", nil
-		t.Errorf("refused for the wrong reason: %v", err)
+	}
+	report, _, err := Apply(context.Background(), archHost(t), d, known, store.OriginCarried, run, nil, nil)
+	if err != nil {
+		t.Fatalf("apply failed: %v", err)
+	}
+	if !ran {
+		t.Fatal("a run-once step whose named file changed was not run again")
+	}
+	if report.Outcomes[1].Action != "created" {
+		t.Errorf("the re-run step was not reported as having run: %+v", report.Outcomes[1])
+	}
+
+	// And with the file unchanged, the step stays done: "again" is when something changed.
+	ran = false
+	now := map[string]string{"env": declaredDigest(d.Resources[0].(*declaration.File))}
+	settled := store.State{}
+	settled.Record(store.Applied{ID: "env", Type: "file", Origin: store.OriginCarried, Target: env, Wrote: now["env"]})
+	settled.Record(store.Applied{ID: "trust", Type: "container", Origin: store.OriginCarried, Target: "trust",
+		Wrote: containerSpec(d.Resources[1].(*declaration.Container), now)})
+	if _, _, err := Apply(context.Background(), archHost(t), d, settled, store.OriginCarried, run, nil, nil); err != nil {
+		t.Fatalf("re-apply failed: %v", err)
+	}
+	if ran {
+		t.Error("a run-once step whose named file did not change was run again")
+	}
+}
+
+func TestAContainerNamingARunOnceStepIsRecreatedWhenItRan(t *testing.T) {
+	// The service that consumes what a step made names the step: when the step ran this pass —
+	// fetched a new root — the running container holds the old one, and its spec did not move,
+	// so restart-on is what brings it back with the new one (novox/hq ADR 0099).
+	d := parseTrusted(t, `{"declaration":1,"resources":[
+	  {"id":"trust","type":"container","name":"trust","image":"`+pinned+`","run-once":true},
+	  {"id":"server","type":"container","name":"server","image":"`+pinned+`","restart-on":["trust"]}
+	]}`)
+	declares := map[string]string{"trust": declaredDigest(d.Resources[0].(*declaration.Container))}
+	spec := containerSpec(d.Resources[1].(*declaration.Container), declares)
+
+	var removed, created bool
+	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		switch args[0] {
+		case "info":
+			return "27.0\n", nil
+		case "inspect":
+			// The server is up, made from exactly this spec — nothing but the step's run says
+			// it must be replaced.
+			return "true\t" + spec, nil
+		case "rm":
+			if len(args) > 0 && args[len(args)-1] == "server" {
+				removed = true
+			}
+			return "", nil
+		case "run":
+			if args[len(args)-1] != "trust" && strings.Contains(strings.Join(args, " "), "--name server") {
+				created = true
+			}
+			return "deadbeef\n", nil
+		}
+		return "", nil
+	}
+	report, _, err := Apply(context.Background(), archHost(t), d, store.State{}, store.OriginCarried, run, nil, nil)
+	if err != nil {
+		t.Fatalf("apply failed: %v", err)
+	}
+	if !removed || !created {
+		t.Fatalf("the container naming the step was not recreated after the step ran (removed=%v created=%v)", removed, created)
+	}
+	server := report.Outcomes[len(report.Outcomes)-1]
+	if server.Action != "updated" || !strings.Contains(server.Detail, "trust") {
+		t.Errorf("the recreation did not name the step as its reason: %+v", server)
 	}
 }

internal/declaration/declaration.go

@@ -678,7 +678,8 @@ type Container struct {
 	// env, volumes — does not include a mounted file's *content*, so a settings change that
 	// re-renders that file is invisible to the ordinary spec diff. This closes that: the host
 	// recreates the container when one of these resources changed this pass, even if the spec
-	// matches.
+	// matches. On a run-once step it means *run again*: a step that fetches a fact from a provider
+	// names the binding it reads, and is run again when the provider moved (novox/hq ADR 0099).
 	RestartOn []string `json:"restart-on,omitempty"`
 
 	// RunOnce marks a container the host runs to completion rather than leaves running: a step,
@@ -713,13 +714,9 @@ func (c *Container) validate(where string, _ bool) []string {
 	if c.Name == "" {
 		problems = append(problems, where+": a container needs a name")
 	}
-	// restart-on brings a *running* container back when a file it read changed; a run-once step
+	// A run-once step may name what it reads under restart-on. For a step the word means *run
-	// does not stay running to be brought back. Declaring both asks for two contradictory
+	// again*: what a container reads is part of its digest, so a step whose named resource changed
-	// lifecycles at once, so it is refused rather than silently resolved to one of them.
+	// is a different step and runs again (novox/hq ADR 0099). Not refused.
-	if c.RunOnce && len(c.RestartOn) > 0 {
-		problems = append(problems, where+": a run-once container cannot also declare restart-on; "+
-			"it runs to completion rather than staying running to be restarted")
-	}
 	// A container runs once and gates, on a cadence, or stays up — never two of these
 	// (novox/hq ADR 0053). run-once and schedule are the two "runs to completion" lifecycles and
 	// contradict each other, and a scheduled step does not stay running to be brought back by