A one-shot service that finished is not stopped, and a container is what it reads

Two faults that both reported success while being wrong, found while proving
the firewall module actually delivers.

A unit whose job is to apply something and exit — load a rule set, set a
sysctl — is inactive the instant it succeeds. Reading that as stopped made it
permanently unsatisfiable: the host started it, it worked, the host read back
stopped and reported the machine as not doing what it was told, on every apply,
for ever, with the rules correctly in place the whole time. That is what the
firewall has been doing on every machine it was assigned to, and why the
four-machine bed was red.

And a container took its identity from its own fields, not from the files it
reads. A file written in an earlier apply — or before the container declared it
as a dependency — left a process holding a credential the mesh had already
replaced, with everything reporting success (novox/hq 04-ISSUES/045). What a
container reads is now part of what it is, so the comparison is a standing one
rather than a tripwire that fires during one apply and never again.

internal/apply/apply.go

@@ -150,6 +150,20 @@ func Apply(
 	// restarting for it every time would make a steady machine restart its services for ever.
 	changed := map[string]bool{}
 
+	// **What each resource currently says, so a container can be identified by its inputs.**
+	//
+	// `changed` above is edge-triggered and only within one apply, which is right for "restart it
+	// because this just moved" and wrong for "is this container running the file that is there
+	// now". A file written in an earlier apply, or written before the container declared it as a
+	// dependency, leaves a container holding values nothing will ever re-read: it is up, the
+	// machine reports success, and what is inside is using a credential the mesh has replaced
+	// (novox/hq 04-ISSUES/045). Folding these into the container's spec makes the comparison a
+	// standing one instead.
+	declares := map[string]string{}
+	for _, resource := range d.Resources {
+		declares[resource.Identity()] = declaredDigest(resource)
+	}
+
 	// Everything is attempted, and every failure is reported.
 	//
 	// **It used to stop at the first one**, and that made one broken resource hold the whole
@@ -169,7 +183,7 @@ func Apply(
 	var failures []*Error
 	for _, resource := range d.Resources {
 		was, _ := known.Find(resource.Identity())
-		outcome, err := applyOne(ctx, sys, resource, run, changed, was, unseal)
+		outcome, err := applyOne(ctx, sys, resource, run, changed, declares, was, unseal)
 		if err != nil {
 			failed := &Error{Resource: resource.Identity(), Err: err, Done: report}
 			failures = append(failures, failed)
@@ -239,7 +253,8 @@ func Apply(
 type Unseal func(sealed string) ([]byte, error)
 
 func applyOne(ctx context.Context, sys system.System, r declaration.Resource, run Runner,
-	changed map[string]bool, previous store.Applied, unseal Unseal) (Outcome, error) {
+	changed map[string]bool, declares map[string]string, previous store.Applied,
+	unseal Unseal) (Outcome, error) {
 	switch res := r.(type) {
 	case *declaration.Directory:
 		return applyDirectory(res)
@@ -250,7 +265,7 @@ func applyOne(ctx context.Context, sys system.System, r declaration.Resource, ru
 	case *declaration.Package:
 		return applyPackage(ctx, sys, res, run)
 	case *declaration.Container:
-		return applyContainer(ctx, res, run, changed, previous)
+		return applyContainer(ctx, res, run, changed, declares, previous)
 	case *declaration.User:
 		return applyUser(ctx, sys, res, run)
 	case *declaration.Archive:
@@ -853,7 +868,7 @@ const (
 
 // containerSpec is the identity of a declared container: everything that, if changed, means
 // the running container is no longer what was asked for.
-func containerSpec(r *declaration.Container) string {
+func containerSpec(r *declaration.Container, declares map[string]string) string {
 	keys := make([]string, 0, len(r.Env))
 	for k := range r.Env {
 		keys = append(keys, k)
@@ -880,6 +895,19 @@ func containerSpec(r *declaration.Container) string {
 	if r.Schedule != "" {
 		b.WriteString("schedule " + r.Schedule + "\n")
 	}
+	// **What this container reads is part of what it is.**
+	//
+	// A container takes its environment and its mounted files once, at start, and never looks
+	// again. Comparing only the fields above meant a container whose configuration had since been
+	// rewritten compared equal and was left alone — running values the machine no longer holds,
+	// while every check reported success (novox/hq 04-ISSUES/045). Naming what it depends on here
+	// makes that comparison standing rather than a tripwire that fires during one apply and never
+	// again. Sorted, so the digest does not move for a reordering nobody made.
+	depends := append([]string{}, r.RestartOn...)
+	sort.Strings(depends)
+	for _, id := range depends {
+		b.WriteString("reads " + id + "=" + declares[id] + "\n")
+	}
 	return fmt.Sprintf("%x", sha256.Sum256([]byte(b.String())))
 }
 
@@ -944,9 +972,10 @@ func applyNetwork(ctx context.Context, r *declaration.Network, run Runner) (Outc
 	return out, nil
 }
 
-func applyContainer(ctx context.Context, r *declaration.Container, run Runner, changed map[string]bool, previous store.Applied) (Outcome, error) {
+func applyContainer(ctx context.Context, r *declaration.Container, run Runner,
+	changed map[string]bool, declares map[string]string, previous store.Applied) (Outcome, error) {
 	out := begin(r)
-	want := containerSpec(r)
+	want := containerSpec(r, declares)
 
 	cri, err := containerRuntime(ctx, run)
 	if err != nil {
@@ -1346,3 +1375,25 @@ func holds(resource declaration.Resource) []int {
 	}
 	return out
 }
+
+// declaredDigest is what a resource currently says it should be.
+//
+// **Content, not identity.** It exists so a container can be told apart by what it reads: a file
+// whose text changed must produce a different digest, or the container mounting it compares equal
+// to one started against the old text. Only the shapes something can read are digested; for
+// everything else the identity is enough, because nothing mounts a package.
+func declaredDigest(r declaration.Resource) string {
+	var material string
+	switch res := r.(type) {
+	case *declaration.File:
+		// The content as declared, before any sealing is opened — two machines are given different
+		// ciphertext for the same secret, and digesting that would make an unchanged file look
+		// changed on every apply and restart the container reading it for ever.
+		material = res.Content
+	case *declaration.Directory:
+		material = res.Path + "\n" + res.Mode
+	default:
+		return ""
+	}
+	return fmt.Sprintf("%x", sha256.Sum256([]byte(material)))
+}

internal/apply/apply_test.go

@@ -666,7 +666,7 @@ func TestAContainerWhoseDeclarationChangedIsReplaced(t *testing.T) {
 	d := parseTrusted(t, `{"declaration":1,"resources":[
 	  {"id":"store","type":"container","name":"store","image":"`+pinned+`","env":{"PGDATA":"/data"}}
 	]}`)
-	want := containerSpec(d.Resources[0].(*declaration.Container))
+	want := containerSpec(d.Resources[0].(*declaration.Container), nil)
 
 	var removed, created bool
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
@@ -704,7 +704,7 @@ func TestAContainerThatMatchesIsLeftAlone(t *testing.T) {
 	d := parseTrusted(t, `{"declaration":1,"resources":[
 	  {"id":"store","type":"container","name":"store","image":"`+pinned+`","env":{"PGDATA":"/data"}}
 	]}`)
-	spec := containerSpec(d.Resources[0].(*declaration.Container))
+	spec := containerSpec(d.Resources[0].(*declaration.Container), nil)
 
 	var touched bool
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
@@ -743,7 +743,13 @@ func TestAContainerIsRecreatedWhenARestartOnResourceChanged(t *testing.T) {
 	  {"id":"config","type":"file","path":"`+conf+`","content":"{\"token\":\"new\"}\n"},
 	  {"id":"app","type":"container","name":"app","image":"`+pinned+`","restart-on":["config"]}
 	]}`)
-	spec := containerSpec(d.Resources[1].(*declaration.Container))
+	// The spec the container was created with, back when the file said something else. Built the
+	// way the host builds it, so this is the real comparison rather than a hand-written string:
+	// what a container reads is part of what it is, so the old content yields a different spec.
+	was := map[string]string{"config": declaredDigest(&declaration.File{Content: "{\"token\":\"old\"}\n"})}
+	now := map[string]string{"config": declaredDigest(d.Resources[0].(*declaration.File))}
+	stale := containerSpec(d.Resources[1].(*declaration.Container), was)
+	fresh := containerSpec(d.Resources[1].(*declaration.Container), now)
 
 	var removed, created bool
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
@@ -751,9 +757,12 @@ func TestAContainerIsRecreatedWhenARestartOnResourceChanged(t *testing.T) {
 		case "info":
 			return "27.0\n", nil
 		case "inspect":
-			// The container already exists, running, with exactly the spec it is declared with —
+			// Already there and running, created against the file as it was. After the host
-			// only the mounted file changed.
+			// recreates it, the runtime holds the one it just made — as a real one would.
-			return "true\t" + spec, nil
+			if created {
+				return "true\t" + fresh, nil
+			}
+			return "true\t" + stale, nil
 		case "rm":
 			removed = true
 			return "", nil
@@ -1492,3 +1501,60 @@ func TestAnEmptyDirectoryIsStillRemoved(t *testing.T) {
 		t.Fatal("an empty directory the mesh made was left behind, so nothing is ever cleaned up")
 	}
 }
+
+// A container holding values from before is replaced, even when nothing changed this pass.
+//
+// **This is the fault the restart-on tripwire could not catch** (novox/hq 04-ISSUES/045). That
+// mechanism fires while a resource is being changed, so it covers the apply where the file moved
+// and nothing afterwards. A file written in an earlier apply — or written before the container
+// declared it as a dependency — leaves a process holding a credential the mesh has already
+// replaced, and every check passes: the container is up, the spec matched, the machine reported
+// success. Making what a container reads part of what it is turns that from a tripwire into a
+// standing comparison.
+func TestAContainerStaleFromAnEarlierApplyIsReplaced(t *testing.T) {
+	dir := t.TempDir()
+	conf := filepath.Join(dir, "db.env")
+	// Already on disk with the current content, so this apply changes nothing at all.
+	if err := os.WriteFile(conf, []byte("PASSWORD=new\n"), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	d := parseTrusted(t, `{"declaration":1,"resources":[
+	  {"id":"env","type":"file","path":"`+conf+`","content":"PASSWORD=new\n","mode":"0600"},
+	  {"id":"app","type":"container","name":"app","image":"`+pinned+`","restart-on":["env"]}
+	]}`)
+
+	// The container was created when the file said something else.
+	was := map[string]string{"env": declaredDigest(&declaration.File{Content: "PASSWORD=old\n"})}
+	now := map[string]string{"env": declaredDigest(d.Resources[0].(*declaration.File))}
+	stale := containerSpec(d.Resources[1].(*declaration.Container), was)
+	fresh := containerSpec(d.Resources[1].(*declaration.Container), now)
+
+	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":
+			if created {
+				return "true\t" + fresh, nil
+			}
+			return "true\t" + stale, nil
+		case "rm":
+			removed = true
+			return "", nil
+		case "run":
+			created = true
+			return "deadbeef\n", nil
+		}
+		return "", nil
+	}
+
+	_, _, 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("a container running values the machine no longer holds was left alone "+
+			"(removed=%v created=%v)", removed, created)
+	}
+}

internal/apply/runonce_test.go

@@ -68,7 +68,7 @@ func TestARunOnceStepIsRunToCompletionNotLeftRunning(t *testing.T) {
 	if !ok {
 		t.Fatal("a completed run-once step was not recorded")
 	}
-	if applied.Wrote != containerSpec(d.Resources[0].(*declaration.Container)) {
+	if applied.Wrote != containerSpec(d.Resources[0].(*declaration.Container), nil) {
 		t.Errorf("the run-once record is not the declaration's digest: %q", applied.Wrote)
 	}
 }
@@ -154,7 +154,7 @@ func TestARunOnceStepAlreadyCompletedIsNotReRun(t *testing.T) {
 	d := parseTrusted(t, `{"declaration":1,"resources":[
 	  {"id":"seed","type":"container","name":"seed","image":"`+pinned+`","run-once":true}
 	]}`)
-	want := containerSpec(d.Resources[0].(*declaration.Container))
+	want := containerSpec(d.Resources[0].(*declaration.Container), nil)
 
 	var ran bool
 	run := func(ctx context.Context, name string, args ...string) (string, error) {
@@ -222,7 +222,7 @@ func TestARunOnceContainerCannotAlsoDeclareRestartOn(t *testing.T) {
 	// silently resolving to one.
 	_, err := declaration.Parse([]byte(`{"declaration":1,"resources":[
 	  {"id":"f","type":"file","path":"/tmp/x","content":"y"},
-	  {"id":"seed","type":"container","name":"seed","image":"`+pinned+`","run-once":true,"restart-on":["f"]}
+	  {"id":"seed","type":"container","name":"seed","image":"` + pinned + `","run-once":true,"restart-on":["f"]}
 	]}`))
 	if err == nil {
 		t.Fatal("a run-once container that also declared restart-on was accepted")

internal/apply/schedule.go

@@ -107,7 +107,10 @@ func (s *Scheduler) Sync(d *declaration.Declaration) {
 		}
 		seen[c.Identity()] = true
 
-		spec := containerSpec(c)
+		// Nothing to read: a scheduled container may not declare restart-on — it runs to completion
+		// on its cadence rather than staying running to be restarted — so its identity cannot
+		// depend on another resource's content and there is nothing to pass.
+		spec := containerSpec(c, nil)
 		if existing := s.jobs[c.Identity()]; existing != nil && existing.spec == spec {
 			// Unchanged: keep where it is in its cadence, refresh the declaration pointer only.
 			existing.container = c

internal/system/arch.go

@@ -99,9 +99,10 @@ func staleIndex(out string) bool {
 // would have had to drop.
 func (arch) ServiceState(ctx context.Context, run Runner, unit string) (string, error) {
 	out, _ := run(ctx, "systemctl", "show", unit,
-		"--property=LoadState", "--property=ActiveState")
+		"--property=LoadState", "--property=ActiveState", "--property=Type",
+		"--property=RemainAfterExit", "--property=ExecMainStatus")
 
-	var load, active string
+	var load, active, kind, remains, exited string
 	for _, line := range strings.Split(out, "\n") {
 		key, value, found := strings.Cut(strings.TrimSpace(line), "=")
 		if !found {
@@ -112,6 +113,12 @@ func (arch) ServiceState(ctx context.Context, run Runner, unit string) (string,
 			load = value
 		case "ActiveState":
 			active = value
+		case "Type":
+			kind = value
+		case "RemainAfterExit":
+			remains = value
+		case "ExecMainStatus":
+			exited = value
 		}
 	}
 
@@ -129,6 +136,26 @@ func (arch) ServiceState(ctx context.Context, run Runner, unit string) (string,
 		return "", fmt.Errorf("%s is installed but its unit file cannot be loaded (%s)", unit, load)
 	}
 
+	// **A one-shot that finished is not stopped.** A unit whose whole job is to apply something
+	// and exit — load a rule set, set a sysctl — is reported inactive the moment it succeeds, and
+	// unless it is told to linger there is no state in which it is ever "active". Reading that as
+	// "stopped" makes such a unit permanently unsatisfiable: the host starts it, it does its work,
+	// it exits, the host reads back "stopped" and reports failure — for ever, on every apply,
+	// while the thing it configured is in place and working.
+	//
+	// That is not hypothetical. It is what the firewall did on every machine it was ever assigned
+	// to: rules loaded, service reported failed, the mesh reported a machine not doing what it was
+	// told, and the only visible symptom was a red line about a unit nobody could see anything
+	// wrong with.
+	//
+	// So for that shape, what "running" means is "it ran, and it worked".
+	if kind == "oneshot" && remains != "yes" && active == "inactive" {
+		if exited == "0" || exited == "" {
+			return "running", nil
+		}
+		return "stopped", nil
+	}
+
 	switch active {
 	case "active", "activating", "reloading":
 		return "running", nil

internal/system/arch_test.go

@@ -0,0 +1,51 @@
+package system
+
+import (
+	"context"
+	"testing"
+)
+
+// A one-shot unit that did its work and exited is satisfied, not stopped.
+//
+// **This made the firewall permanently unsatisfiable.** Its unit loads a rule set and exits, so it
+// is inactive the instant it succeeds — the host started it, it worked, the host read back
+// "stopped" and reported the machine as not doing what it was told. On every apply, for ever, with
+// the rules correctly in place the whole time.
+func TestAOneShotThatFinishedIsRunning(t *testing.T) {
+	run := func(_ context.Context, _ string, _ ...string) (string, error) {
+		return "LoadState=loaded\nActiveState=inactive\nType=oneshot\nRemainAfterExit=no\nExecMainStatus=0\n", nil
+	}
+	state, err := arch{}.ServiceState(context.Background(), run, "nftables.service")
+	if err != nil {
+		t.Fatalf("a one-shot that succeeded was an error: %v", err)
+	}
+	if state != "running" {
+		t.Fatalf("a one-shot that did its work reads as %q, so it can never be satisfied", state)
+	}
+}
+
+// And one that failed is still stopped, or the host would report success for work that did not
+// happen — which is the opposite mistake and the worse one.
+func TestAOneShotThatFailedIsStopped(t *testing.T) {
+	run := func(_ context.Context, _ string, _ ...string) (string, error) {
+		return "LoadState=loaded\nActiveState=inactive\nType=oneshot\nRemainAfterExit=no\nExecMainStatus=1\n", nil
+	}
+	state, err := arch{}.ServiceState(context.Background(), run, "nftables.service")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if state != "stopped" {
+		t.Fatalf("a one-shot that failed reads as %q, so a broken firewall reports success", state)
+	}
+}
+
+// A unit that lingers deliberately is unaffected: it says so, and its active state is the answer.
+func TestAOneShotThatRemainsIsJudgedByItsActiveState(t *testing.T) {
+	run := func(_ context.Context, _ string, _ ...string) (string, error) {
+		return "LoadState=loaded\nActiveState=active\nType=oneshot\nRemainAfterExit=yes\nExecMainStatus=0\n", nil
+	}
+	state, _ := arch{}.ServiceState(context.Background(), run, "thing.service")
+	if state != "running" {
+		t.Fatalf("a lingering one-shot reads as %q", state)
+	}
+}