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 —
-			// only the mounted file changed.
-			return "true\t" + spec, nil
+			// Already there and running, created against the file as it was. After the host
+			// recreates it, the runtime holds the one it just made — as a real one would.
+			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) {

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