From c60228e71937f13b59b92094245c74da76416f29 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 23 Sep 2026 23:12:52 +0200 Subject: [PATCH 1/3] Recreate a container when the content of a file it reads at creation changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The host decided whether a container was still the one declared by a digest of its declaration, and the declaration names an env-file's path and a mount's path — never what is in them. So when the store was given a new port, the host rewrote the forge's and the analytics service's environment files, correctly, and left both containers running with the old port in their environment: a container reads its env-file when it is CREATED, and `docker restart` hands it the same environment again. Both looked healthy until they answered 502. What a running container takes in at creation is now part of its spec, by content: every env-file, a file bind-mounted into it, and every file this host wrote at or under a directory bind-mounted into it — the secrets, bindings and configs under a module's state directories. The digest is the one the store already records for a file the host wrote (`wrote`), read from the state as it stands when the container is reached, so a file rewritten earlier in the same apply is already the new one; a file the host has no record of — an env-file a predecessor left, the superuser secret genesis writes before any declaration names it — is read from disk, which is what keeps adopting a running store in place a reconcile and not a recreate. Deliberately not part of it: what else is in a bind-mounted directory, which is the service's own data and changes while it runs; a named volume; a seed created once, which digests as the seed the host wrote and not as what has grown in it; and a step — a run-once or scheduled container reads its files when it runs and runs fresh each time. On an adopted node a held container is held before any of this is looked at. The host records what each container was created reading, per file, so the recreate can say which file changed — "recreated: changed" in the report and, now with its detail, in the log. A container made before this record existed is recreated once and says so. novox/hq 04-ISSUES/103 --- internal/apply/apply.go | 162 ++++++++++++++++++-- internal/apply/apply_test.go | 12 +- internal/apply/reads_test.go | 221 ++++++++++++++++++++++++++++ internal/apply/runonce_test.go | 10 +- internal/apply/schedule.go | 2 +- internal/declaration/declaration.go | 16 +- internal/store/store.go | 29 ++++ 7 files changed, 423 insertions(+), 29 deletions(-) create mode 100644 internal/apply/reads_test.go diff --git a/internal/apply/apply.go b/internal/apply/apply.go index ba07b42..1ea2fda 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -52,6 +52,9 @@ type Outcome struct { wrote string // into is what a file written into held before the mesh's keys (novox/hq ADR 0102). into *store.Into + // reads is, for a container, the digest of each file it was created reading, by path — so + // the next apply can say which one changed (novox/hq 04-ISSUES/103). + reads map[string]string } // Report is what an apply did, in the order it did it. @@ -266,9 +269,16 @@ func ApplyKeeping( // 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{} + // + // And what each file a container reads at creation holds — its env-files and what is mounted + // into it — by the digest this host recorded when it wrote the file, read from `known` as it + // stands when the container is reached, so a file rewritten earlier in this same apply is + // already the new one (novox/hq 04-ISSUES/103). That needs the file applied before the + // container, which is the declared order; a container declared ahead of its file sees the + // change one apply late, and never misses it. + in := inputs{declares: map[string]string{}, known: &known} for _, resource := range d.Resources { - declares[resource.Identity()] = declaredDigest(resource) + in.declares[resource.Identity()] = declaredDigest(resource) } // Everything is attempted, and every failure is reported. @@ -340,7 +350,7 @@ func ApplyKeeping( !known.Recorded(string(declaration.TypeFile), f.Path) { keepFound = keep } - outcome, err = applyOne(ctx, sys, resource, run, changed, declares, was, unseal, keepFound) + outcome, err = applyOne(ctx, sys, resource, run, changed, in, was, unseal, keepFound) } if err != nil { failed := &Error{Resource: resource.Identity(), Err: err, Done: report} @@ -386,6 +396,7 @@ func ApplyKeeping( Target: outcome.Target, AppliedAt: time.Now().UTC(), Wrote: outcome.wrote, Into: outcome.into, + Reads: outcome.reads, Holds: holds(resource), }) // Its module has been taken, and what was held for it is now the mesh's. @@ -396,7 +407,14 @@ func ApplyKeeping( report.Outcomes = append(report.Outcomes, outcome) if outcome.Action != "unchanged" { changed[resource.Identity()] = true - log(fmt.Sprintf(" %s %s (%s)", outcome.Action, outcome.ID, outcome.Target)) + // With the detail, when there is one: "updated app" says a container was replaced; + // which file made that happen is what somebody reading the log at the time needs + // (novox/hq 04-ISSUES/103). + line := fmt.Sprintf(" %s %s (%s)", outcome.Action, outcome.ID, outcome.Target) + if outcome.Detail != "" { + line += ": " + outcome.Detail + } + log(line) } } @@ -440,7 +458,7 @@ const guardPrefix = declaration.AdoptionPrefix + "guard" type Unseal func(sealed string) ([]byte, error) func applyOne(ctx context.Context, sys system.System, r declaration.Resource, run Runner, - changed map[string]bool, declares map[string]string, previous store.Applied, + changed map[string]bool, in inputs, previous store.Applied, unseal Unseal, keepFound Keep) (Outcome, error) { switch res := r.(type) { case *declaration.Directory: @@ -452,7 +470,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, declares, previous) + return applyContainer(ctx, res, run, changed, in, previous) case *declaration.User: return applyUser(ctx, sys, res, run) case *declaration.Archive: @@ -1129,9 +1147,99 @@ const ( idLabel = "mesh-host.id" ) +// inputs is what a container takes in when it is created beyond its own declaration: what each +// resource it names under restart-on currently declares, and what the files it reads hold. +type inputs struct { + // declares is each resource's declared digest, by id (declaredDigest). + declares map[string]string + // known is the node's state as it stands when the container is reached — so a file applied + // earlier in the same pass is already its new self. Nil where nothing was written: a test, + // or a scheduled fire, which reads no file at creation. + known *store.State +} + +// fileDigest is what a file the container reads holds, by digest. +// +// **What this host wrote when it has a record of writing it, and what is on disk when it has +// not.** The record is preferred because it is what the host means by the file: a seed created +// once digests as the seed, not as whatever the service has grown in it, and a file written into +// digests as the mesh's keys, not the machine's (novox/hq ADR 0102). A file the host never wrote +// — an env-file a predecessor left, the superuser secret genesis writes before any declaration +// names it — is read, so the digest is the same one the host records when it later writes the +// same bytes there, and adopting a running store in place stays a reconcile rather than a +// recreate (bootstrap phase three). Empty when there is nothing readable there: the runtime +// refuses an absent env-file itself, with a better message than this could give. +func (in inputs) fileDigest(path string) string { + if in.known != nil { + for _, f := range in.known.FilesUnder(path) { + if f.Target == path { + return f.Wrote + } + } + } + info, err := os.Stat(path) + if err != nil || !info.Mode().IsRegular() { + return "" + } + content, err := os.ReadFile(path) + if err != nil { + return "" + } + return digestOf(string(content)) +} + +// reads is every file a running container takes in when it is created, by path and digest +// (novox/hq 04-ISSUES/103). +// +// - every env-file: the runtime reads it once, at create, and `docker restart` hands the +// container the same environment it had. +// - a file bind-mounted into it, by its content — and, for a directory bind-mounted into it, +// every file THIS HOST wrote at or beneath the source: the secrets, bindings and configs it +// put under the module's state directories. What else is in a mounted directory is the +// service's own data, which changes while it runs and is nothing to recreate it for. +// +// A step is not here: a run-once or scheduled container reads its files when it runs, and +// runs fresh each time. Only a container that stays running holds what it read. +func (in inputs) reads(r *declaration.Container) map[string]string { + if r.RunOnce || r.Schedule != "" { + return nil + } + out := map[string]string{} + for _, path := range r.EnvFile { + out[path] = in.fileDigest(path) + } + for _, v := range r.Volumes { + src := mountSource(v) + if !strings.HasPrefix(src, "/") { + continue // a named volume: the runtime's, holding data + } + if in.known != nil { + for _, f := range in.known.FilesUnder(src) { + out[f.Target] = f.Wrote + } + } + if _, recorded := out[src]; !recorded { + if digest := in.fileDigest(src); digest != "" { + out[src] = digest + } + } + } + if len(out) == 0 { + return nil + } + return out +} + // 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, declares map[string]string) string { +func containerSpec(r *declaration.Container, in inputs) string { + return containerSpecReading(r, in.declares, in.reads(r)) +} + +// containerSpecReading is containerSpec with what the container reads already read — so an +// applier that also records those digests reads each file once, and the label and the record +// cannot disagree about a file that moved between two reads. +func containerSpecReading(r *declaration.Container, declares, reads map[string]string) string { keys := make([]string, 0, len(r.Env)) for k := range r.Env { keys = append(keys, k) @@ -1171,6 +1279,15 @@ func containerSpec(r *declaration.Container, declares map[string]string) string for _, id := range depends { b.WriteString("reads " + id + "=" + declares[id] + "\n") } + // And what the files it reads at creation hold — not only their paths, which `volume` and the + // env-file arguments already name. The spec named the env-file's path and not its content, + // so the host rewrote two environment files with the store's new port and left both + // containers running with the old one, healthy-looking, until they answered 502 (novox/hq + // 04-ISSUES/103). Added only when there is something read, so a container that reads nothing + // keeps the digest it had. + for _, path := range sortedKeys(reads) { + b.WriteString("file " + path + "=" + reads[path] + "\n") + } return fmt.Sprintf("%x", sha256.Sum256([]byte(b.String()))) } @@ -1236,9 +1353,12 @@ func applyNetwork(ctx context.Context, r *declaration.Network, run Runner) (Outc } func applyContainer(ctx context.Context, r *declaration.Container, run Runner, - changed map[string]bool, declares map[string]string, previous store.Applied) (Outcome, error) { + changed map[string]bool, in inputs, previous store.Applied) (Outcome, error) { out := begin(r) - want := containerSpec(r, declares) + // Read once, so the spec and the record agree on what was read even if a file moves under them. + reads := in.reads(r) + want := containerSpecReading(r, in.declares, reads) + out.reads = reads cri, err := containerRuntime(ctx, run) if err != nil { @@ -1279,6 +1399,16 @@ func applyContainer(ctx context.Context, r *declaration.Container, run Runner, // already has. reasons := restartedBy(r.RestartOn, changed) + // Which of the files it reads no longer hold what it was created reading. The spec label says + // only that SOMETHING moved; the record of what was read says what — and that is the line a + // person needs when a service went stale without a word (novox/hq 04-ISSUES/103). + var changedFiles []string + for _, path := range sortedKeys(previous.Reads) { + if now, still := reads[path]; still && now != previous.Reads[path] { + changedFiles = append(changedFiles, path) + } + } + switch { case existed && before.Spec == want && before.Running && len(reasons) == 0: out.Action = "unchanged" @@ -1340,9 +1470,19 @@ func applyContainer(ctx context.Context, r *declaration.Container, run Runner, out.Action = "created" if existed { out.Action = "updated" - if len(reasons) > 0 { + switch { + case len(changedFiles) > 0: + out.Detail = "recreated: " + strings.Join(changedFiles, ", ") + " changed" + if len(reasons) > 0 { + out.Detail += "; and to pick up " + strings.Join(reasons, ", ") + } + case len(reasons) > 0: out.Detail = "recreated to pick up " + strings.Join(reasons, ", ") - } else { + case previous.Reads == nil && len(reads) > 0: + // A container made before this host kept what it read: whether the files it holds + // are the ones on disk cannot be known, so it is recreated once and from now on can. + out.Detail = "recreated: what it reads was not on record, so what it holds could not be checked" + default: out.Detail = "replaced; a container's configuration is fixed when it is created" } } diff --git a/internal/apply/apply_test.go b/internal/apply/apply_test.go index ba5c5aa..fea616b 100644 --- a/internal/apply/apply_test.go +++ b/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), nil) + want := containerSpec(d.Resources[0].(*declaration.Container), inputs{}) 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), nil) + spec := containerSpec(d.Resources[0].(*declaration.Container), inputs{}) var touched bool run := func(ctx context.Context, name string, args ...string) (string, error) { @@ -748,8 +748,8 @@ func TestAContainerIsRecreatedWhenARestartOnResourceChanged(t *testing.T) { // 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) + stale := containerSpec(d.Resources[1].(*declaration.Container), inputs{declares: was}) + fresh := containerSpec(d.Resources[1].(*declaration.Container), inputs{declares: now}) var removed, created bool run := func(ctx context.Context, name string, args ...string) (string, error) { @@ -1538,8 +1538,8 @@ func TestAContainerStaleFromAnEarlierApplyIsReplaced(t *testing.T) { // 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) + stale := containerSpec(d.Resources[1].(*declaration.Container), inputs{declares: was}) + fresh := containerSpec(d.Resources[1].(*declaration.Container), inputs{declares: now}) var removed, created bool run := func(ctx context.Context, name string, args ...string) (string, error) { diff --git a/internal/apply/reads_test.go b/internal/apply/reads_test.go new file mode 100644 index 0000000..a55507b --- /dev/null +++ b/internal/apply/reads_test.go @@ -0,0 +1,221 @@ +package apply + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/novox/mesh-host/internal/declaration" + "github.com/novox/mesh-host/internal/store" +) + +// Defends novox/hq 04-ISSUES/103: a container is recreated when the CONTENT of a file it reads at +// creation changes, not only when its path does. A container takes its env-file and its mounted +// files in once, when it is created; `docker restart` hands it the same environment again, so +// only a recreate carries a rewritten file into the process. +// +// The runtime here is the `machine` fake: it keeps the spec label the host gave a container and +// hands it back on inspect, so the comparison under test is the one the host really makes, +// against what it really wrote — not against a spec a test imagined. + +func applyCarried(t *testing.T, d *declaration.Declaration, known store.State, m *machine, + log func(string)) (Report, store.State) { + t.Helper() + report, state, err := Apply(context.Background(), archHost(t), d, known, store.OriginCarried, m.run, log, nil) + if err != nil { + t.Fatalf("apply failed: %v", err) + } + return report, state +} + +func TestAContainerIsRecreatedWhenItsEnvFileChanged(t *testing.T) { + // The night of the issue: the store was given a new port, the host rewrote the forge's + // environment file with it — and left the forge running with the old one. + dir := t.TempDir() + env := filepath.Join(dir, "forge.env") + declare := func(port string) *declaration.Declaration { + return parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"forge.env","type":"file","path":"`+env+`","content":"DATABASE_PORT=`+port+`\n","mode":"0600"}, + {"id":"forge.server","type":"container","name":"forge","image":"`+pinned+`","env-file":["`+env+`"]} + ]}`) + } + m := &machine{containers: map[string]*fakeContainer{}} + var logged []string + log := func(line string) { logged = append(logged, line) } + + report, state := applyCarried(t, declare("5432"), store.State{}, m, log) + if o := outcomeOf(report, "forge.server"); o.Action != "created" { + t.Fatalf("the container was not created: %+v", report.Outcomes) + } + + // The store moved. The file is rewritten in this apply, before the container is reached, and + // the container must follow it in the same pass. + m.asked, logged = nil, nil + report, state = applyCarried(t, declare("5433"), state, m, log) + if !m.removed("forge") || !m.did("docker run") { + t.Fatalf("the container kept running with the old environment after its env-file changed: %v", m.asked) + } + o := outcomeOf(report, "forge.server") + if o.Action != "updated" || o.Detail != "recreated: "+env+" changed" { + t.Errorf("the recreate did not say which file changed: %+v", o) + } + var said bool + for _, line := range logged { + if strings.Contains(line, "updated forge.server") && strings.Contains(line, "recreated: "+env+" changed") { + said = true + } + } + if !said { + t.Errorf("the log did not say which file made the container recreate: %q", logged) + } + + // And with nothing moved, it is left alone: content is part of the identity, not a tripwire. + m.asked = nil + report, _ = applyCarried(t, declare("5433"), state, m, log) + if m.did("docker rm") || m.did("docker run") || report.Changed() { + t.Errorf("a container whose env-file did not change was recreated: %v %+v", m.asked, report.Outcomes) + } +} + +func TestAnEnvFileTheHostDidNotWriteIsStillReadForWhatItHolds(t *testing.T) { + // The host has no record of this file — a predecessor left it, or something else on the + // machine maintains it — and the container still reads it once. Its content is read from the + // disk, so a change is a recreate exactly as for a file the host wrote. + dir := t.TempDir() + env := filepath.Join(dir, "app.env") + if err := os.WriteFile(env, []byte("TOKEN=old\n"), 0o600); err != nil { + t.Fatal(err) + } + d := parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"app.server","type":"container","name":"app","image":"`+pinned+`","env-file":["`+env+`"]} + ]}`) + m := &machine{containers: map[string]*fakeContainer{}} + _, state := applyCarried(t, d, store.State{}, m, nil) + + if err := os.WriteFile(env, []byte("TOKEN=new\n"), 0o600); err != nil { + t.Fatal(err) + } + m.asked = nil + report, _ := applyCarried(t, d, state, m, nil) + if !m.removed("app") || !m.did("docker run") { + t.Fatalf("a container reading an env-file the host did not write was not recreated when it changed: %v", m.asked) + } + if o := outcomeOf(report, "app.server"); o.Detail != "recreated: "+env+" changed" { + t.Errorf("the recreate did not name the file: %+v", o) + } +} + +func TestAContainerIsRecreatedWhenAMountedSecretChanged(t *testing.T) { + // A rotated credential has the same shape as a moved port: the host writes the file the + // container mounts, and the process holds the value it was created with. + dir := t.TempDir() + secret := filepath.Join(dir, "db.secret") + declare := func(value string) *declaration.Declaration { + return parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"app.secret","type":"file","path":"`+secret+`","content":"`+value+`","mode":"0600"}, + {"id":"app.server","type":"container","name":"app","image":"`+pinned+`", + "volumes":["`+secret+`:/run/secrets/db:ro"]} + ]}`) + } + m := &machine{containers: map[string]*fakeContainer{}} + _, state := applyCarried(t, declare("hunter2"), store.State{}, m, nil) + + m.asked = nil + report, _ := applyCarried(t, declare("correct-horse-battery-staple"), state, m, nil) + if !m.removed("app") || !m.did("docker run") { + t.Fatalf("the container kept the secret it was created with after the mounted file changed: %v", m.asked) + } + if o := outcomeOf(report, "app.server"); o.Action != "updated" || o.Detail != "recreated: "+secret+" changed" { + t.Errorf("the recreate did not say which file changed: %+v", o) + } +} + +func TestWhatAServiceWritesInAMountedDirectoryIsNotPartOfWhatItIs(t *testing.T) { + // A bind-mounted directory is the service's data: it changes while the service runs, and + // recreating for it would restart a database for every row it wrote. What the HOST wrote + // under that directory — a config it rendered there — is another matter: the service read + // that once, at start. + dir := t.TempDir() + state := filepath.Join(dir, "state") + config := filepath.Join(state, "config.toml") + declare := func(level string) *declaration.Declaration { + return parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"app.state","type":"directory","path":"`+state+`"}, + {"id":"app.config","type":"file","path":"`+config+`","content":"level = \"`+level+`\"\n"}, + {"id":"app.server","type":"container","name":"app","image":"`+pinned+`", + "volumes":["`+state+`:/var/lib/app"]} + ]}`) + } + m := &machine{containers: map[string]*fakeContainer{}} + _, known := applyCarried(t, declare("info"), store.State{}, m, nil) + + // The service grows its data in the directory it was given. + if err := os.WriteFile(filepath.Join(state, "app.db"), []byte("rows"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Join(state, "cache"), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(state, "cache", "index"), []byte("entries"), 0o600); err != nil { + t.Fatal(err) + } + m.asked = nil + report, known := applyCarried(t, declare("info"), known, m, nil) + if m.did("docker rm") || m.did("docker run") || report.Changed() { + t.Errorf("a container was recreated for data its service wrote in a mounted directory: %v %+v", + m.asked, report.Outcomes) + } + + // The host's own config under the same directory changes: that, the service read at start. + m.asked = nil + report, _ = applyCarried(t, declare("debug"), known, m, nil) + if !m.removed("app") || !m.did("docker run") { + t.Fatalf("a config the host wrote under a mounted directory changed and the container was not recreated: %v", m.asked) + } + if o := outcomeOf(report, "app.server"); o.Detail != "recreated: "+config+" changed" { + t.Errorf("the recreate did not name the config: %+v", o) + } +} + +func TestAHeldContainerIsNotRecreatedByAChangedHeldFile(t *testing.T) { + // On an adopted node the predecessor's container and the file it reads are both held as + // found (novox/hq ADR 0100). The predecessor rewriting its own file is reported on the file + // — and is nothing to recreate the container for: it is not the host's to recreate. + dir := t.TempDir() + env := filepath.Join(dir, "hello.env") + if err := os.WriteFile(env, []byte("PORT=5432\n"), 0o600); err != nil { + t.Fatal(err) + } + m := &machine{containers: map[string]*fakeContainer{ + "hello-web": {id: "predecessor-id", running: true}, + }} + d := adopted(t, untaken("hello-web.env", "hello-web.server"), + `{"id":"hello-web.env","type":"file","path":"`+env+`","content":"PORT=5433\n","mode":"0600"}, + {"id":"hello-web.server","type":"container","name":"hello-web","image":"`+pinned+`","env-file":["`+env+`"]}`) + report, state := applyAdopted(t, d, store.State{}, m, dir) + if outcomeOf(report, "hello-web.env").Action != "held" || outcomeOf(report, "hello-web.server").Action != "held" { + t.Fatalf("the predecessor's file and container were not held: %+v", report.Outcomes) + } + + if err := os.WriteFile(env, []byte("PORT=5434\n"), 0o600); err != nil { + t.Fatal(err) + } + m.asked = nil + report, state = applyAdopted(t, d, state, m, dir) + for _, a := range m.asked { + if strings.HasPrefix(a, "docker run") || strings.HasPrefix(a, "docker rm") { + t.Fatalf("a held container was acted on because a held file changed: %s", a) + } + } + if o := outcomeOf(report, "hello-web.server"); o.Action != "held" { + t.Errorf("the container is no longer held: %+v", o) + } + if h, _ := state.HeldAt("hello-web.env"); h.Changed != "rewritten" { + t.Errorf("the predecessor's rewrite was not reported on the file: %+v", h) + } + if h, _ := state.HeldAt("hello-web.server"); h.Changed != "" { + t.Errorf("a file change was charged to the container: %+v", h) + } +} diff --git a/internal/apply/runonce_test.go b/internal/apply/runonce_test.go index e728e7b..16b3919 100644 --- a/internal/apply/runonce_test.go +++ b/internal/apply/runonce_test.go @@ -69,7 +69,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), nil) { + if applied.Wrote != containerSpec(d.Resources[0].(*declaration.Container), inputs{}) { t.Errorf("the run-once record is not the declaration's digest: %q", applied.Wrote) } } @@ -155,7 +155,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), nil) + want := containerSpec(d.Resources[0].(*declaration.Container), inputs{}) var ran bool run := func(ctx context.Context, name string, args ...string) (string, error) { @@ -232,7 +232,7 @@ func TestARunOnceStepRunsAgainWhenWhatItReadsChanged(t *testing.T) { 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)}) + Wrote: containerSpec(d.Resources[1].(*declaration.Container), inputs{declares: was})}) var ran bool run := func(ctx context.Context, name string, args ...string) (string, error) { @@ -262,7 +262,7 @@ func TestARunOnceStepRunsAgainWhenWhatItReadsChanged(t *testing.T) { 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)}) + Wrote: containerSpec(d.Resources[1].(*declaration.Container), inputs{declares: now})}) if _, _, err := Apply(context.Background(), archHost(t), d, settled, store.OriginCarried, run, nil, nil); err != nil { t.Fatalf("re-apply failed: %v", err) } @@ -280,7 +280,7 @@ func TestAContainerNamingARunOnceStepIsRecreatedWhenItRan(t *testing.T) { {"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) + spec := containerSpec(d.Resources[1].(*declaration.Container), inputs{declares: declares}) var removed, created bool run := func(ctx context.Context, name string, args ...string) (string, error) { diff --git a/internal/apply/schedule.go b/internal/apply/schedule.go index ccede49..6c80de1 100644 --- a/internal/apply/schedule.go +++ b/internal/apply/schedule.go @@ -122,7 +122,7 @@ func (s *Scheduler) Sync(d *declaration.Declaration, held map[string]bool) { // 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) + spec := containerSpec(c, inputs{}) 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 diff --git a/internal/declaration/declaration.go b/internal/declaration/declaration.go index c0bc7af..42aea06 100644 --- a/internal/declaration/declaration.go +++ b/internal/declaration/declaration.go @@ -782,12 +782,16 @@ type Container struct { // RestartOn names resources whose change means this container must be recreated — the same // field a service has, for the same reason (novox/hq 04-ISSUES/009). A container reads a // mounted file once at start; a changed file leaves the running process holding the old value, - // while every check passes because the file on disk is right. The container's spec — image, - // 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. 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). + // while every check passes because the file on disk is right. The host recreates the container + // when one of these resources changed this pass, even if the spec matches. + // + // What a running container reads at creation — its env-files, a file mounted into it, and the + // files the host wrote under a directory mounted into it — is part of its spec by content + // since novox/hq 04-ISSUES/103, and needs no naming here. RestartOn is for what the spec + // cannot see: a resource the container reflects without reading it directly, a step it + // consumes the result of. 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, diff --git a/internal/store/store.go b/internal/store/store.go index 70bec77..09602e1 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -18,6 +18,7 @@ import ( "os" "path/filepath" "sort" + "strings" "time" ) @@ -74,6 +75,14 @@ type Applied struct { // each of the mesh's keys held before it set them, which of them were absent, and whether the // file itself was — so undeclaring it gives the machine back exactly what it had. Into *Into `json:"into,omitempty"` + + // Reads is, for a container, the digest of each file it was created reading — its env-files + // and the files mounted into it — by path (novox/hq 04-ISSUES/103). + // + // A container takes those in once, when it is created, and the digest of the whole is in the + // container's spec label; this is the same information kept per file, so that when the spec + // no longer matches the host can say WHICH file changed rather than only that something did. + Reads map[string]string `json:"reads,omitempty"` } // Into is what a file written into held before the mesh's keys. @@ -200,6 +209,26 @@ func (s State) Recorded(kind, target string) bool { return false } +// FilesUnder returns every file this host has a record of writing at a path or beneath it, of any +// origin and under any id, in path order. +// +// By path rather than by id because a container names what it reads by path: the id a file was +// declared under may change — the bundle's, then a module's, for the same file — and the container +// reading it does not care which. +func (s State) FilesUnder(path string) []Applied { + var out []Applied + for _, r := range s.Resources { + if r.Type != "file" { + continue + } + if r.Target == path || strings.HasPrefix(r.Target, strings.TrimSuffix(path, "/")+"/") { + out = append(out, r) + } + } + sort.Slice(out, func(i, j int) bool { return out[i].Target < out[j].Target }) + return out +} + // HeldAt returns what is held under a resource id. func (s State) HeldAt(id string) (Held, bool) { for _, h := range s.Held { -- 2.54.0 From 982b84310e6042f48e7d5e94f1e90ed5f50120b5 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 23 Sep 2026 23:37:50 +0200 Subject: [PATCH 2/3] Look at what a container mounts directly, accept a pre-upgrade label, and write the genesis secret without a newline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the first cut found four things. A directory mounted into a container is no longer looked inside, not even for the files this host wrote there. The controller records every provider's received and contributions file as a plain file under a mounted directory, so folding those in would have recreated the route proxy — which re-reads its routes live, by design — on every route change, and killed every provisioner sidecar, which polls what it receives, mid-reconcile on every grant. Whether a service reads a file under its directory once or watches it is the service's; restart-on is how a module says "once", and it stays the opt-in. Env-files and files mounted directly remain by content. Genesis wrote the superuser secret as `value\n`; `secret accept` strips the line ending by design, so the postgres module declared `value` — and with a mounted file's content in the spec, phase three would have recreated the store it meant to adopt in place, with the temporary control plane connected to it. Genesis now writes the value alone. readCredentialFile tolerated both endings already. Pinned with the bytes the genesis code path writes, then the module's declaration of the same container: it must reconcile. A container carrying a label from before the host folded in what it reads is accepted rather than recreated, when that label matches the spec as it used to be computed: what it reads is recorded then, a change is caught from that record from the next apply on, and the label is renewed at the next genuine recreate. Recreating them all would have been a restart storm across the mesh in declaration order, the store first. The trade-off is stated in the code: a container already stale at upgrade time is not caught, and could not have been either way. The record of what a container read is looked up by its name when its declared id has none — the bundle's `store` becomes `postgres.server` for the same container — so a change on the day it is adopted still names the file. The by-target lookup takes the most recently applied record, since the bundle's record for the same target is never removed by the mesh's. novox/hq 04-ISSUES/103 --- internal/apply/apply.go | 85 +++++++++----- internal/apply/reads_test.go | 125 ++++++++++++++++++--- internal/bootstrap/adopt_in_place_test.go | 130 ++++++++++++++++++++++ internal/bootstrap/rootsecrets.go | 10 +- internal/declaration/declaration.go | 15 +-- internal/store/store.go | 27 ++--- 6 files changed, 329 insertions(+), 63 deletions(-) create mode 100644 internal/bootstrap/adopt_in_place_test.go diff --git a/internal/apply/apply.go b/internal/apply/apply.go index 1ea2fda..586fa07 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -1167,14 +1167,13 @@ type inputs struct { // — an env-file a predecessor left, the superuser secret genesis writes before any declaration // names it — is read, so the digest is the same one the host records when it later writes the // same bytes there, and adopting a running store in place stays a reconcile rather than a -// recreate (bootstrap phase three). Empty when there is nothing readable there: the runtime -// refuses an absent env-file itself, with a better message than this could give. +// recreate (bootstrap phase three; genesis writes the value with no line ending for exactly this +// reason). Empty when there is nothing readable there: the runtime refuses an absent env-file +// itself, with a better message than this could give. func (in inputs) fileDigest(path string) string { if in.known != nil { - for _, f := range in.known.FilesUnder(path) { - if f.Target == path { - return f.Wrote - } + if f, recorded := in.known.At(string(declaration.TypeFile), path); recorded { + return f.Wrote } } info, err := os.Stat(path) @@ -1193,12 +1192,16 @@ func (in inputs) fileDigest(path string) string { // // - every env-file: the runtime reads it once, at create, and `docker restart` hands the // container the same environment it had. -// - a file bind-mounted into it, by its content — and, for a directory bind-mounted into it, -// every file THIS HOST wrote at or beneath the source: the secrets, bindings and configs it -// put under the module's state directories. What else is in a mounted directory is the -// service's own data, which changes while it runs and is nothing to recreate it for. +// - a file bind-mounted into it, DIRECTLY, by its content: a secret, a credential file. // -// A step is not here: a run-once or scheduled container reads its files when it runs, and +// **A directory bind-mounted into it is not looked inside**, not even for the files this host +// wrote there. What a service reads out of a mounted directory, and when, is the service's +// business: the route proxy re-reads its routes file live and would be recreated on every route +// change; a provisioner sidecar polls what it receives every few seconds and would be killed +// mid-reconcile on every grant. A module whose container does read such a file once, at start, +// says so with restart-on — that is what the field is for, and it stays the opt-in. +// +// A step is not here either: a run-once or scheduled container reads its files when it runs, and // runs fresh each time. Only a container that stays running holds what it read. func (in inputs) reads(r *declaration.Container) map[string]string { if r.RunOnce || r.Schedule != "" { @@ -1213,15 +1216,9 @@ func (in inputs) reads(r *declaration.Container) map[string]string { if !strings.HasPrefix(src, "/") { continue // a named volume: the runtime's, holding data } - if in.known != nil { - for _, f := range in.known.FilesUnder(src) { - out[f.Target] = f.Wrote - } - } - if _, recorded := out[src]; !recorded { - if digest := in.fileDigest(src); digest != "" { - out[src] = digest - } + // fileDigest is empty for a directory, and for anything else that is not a regular file. + if digest := in.fileDigest(src); digest != "" { + out[src] = digest } } if len(out) == 0 { @@ -1399,18 +1396,42 @@ func applyContainer(ctx context.Context, r *declaration.Container, run Runner, // already has. reasons := restartedBy(r.RestartOn, changed) + // What this container was created reading, as last recorded. By its declared id first, and by + // its NAME when that id has no record: the bundle's `store` becomes the postgres module's + // `postgres.server`, the same container under a new id, and a file change on the day it is + // adopted is a real change with a real record — under the old id. + wasReading := previous.Reads + if previous.ID == "" && in.known != nil { + if byName, ok := in.known.At(string(declaration.TypeContainer), r.Name); ok { + wasReading = byName.Reads + } + } + // Which of the files it reads no longer hold what it was created reading. The spec label says // only that SOMETHING moved; the record of what was read says what — and that is the line a // person needs when a service went stale without a word (novox/hq 04-ISSUES/103). var changedFiles []string - for _, path := range sortedKeys(previous.Reads) { - if now, still := reads[path]; still && now != previous.Reads[path] { + for _, path := range sortedKeys(wasReading) { + if now, still := reads[path]; still && now != wasReading[path] { changedFiles = append(changedFiles, path) } } + // **A container labelled before the host folded in what it reads is accepted, not recreated.** + // + // Its label is the spec without the file lines. Recreating every such container on the first + // apply after the host upgraded would be a restart storm across the mesh in declaration order — + // the store first, under everything that uses it. So a label that matches the spec as it used + // to be computed is taken as current: what it reads is recorded now, and from the next apply + // on a changed file is caught by that record; the label itself is rewritten at the next + // genuine recreate. The trade-off, stated: a container that was ALREADY stale when the host + // upgraded — created against a file that has since changed — is not caught by this, and could + // not be by the alternative either, which recreates it without knowing whether it needed to. + legacy := len(reads) > 0 && before.Spec == containerSpecReading(r, in.declares, nil) && + (wasReading == nil || sameReads(wasReading, reads)) + switch { - case existed && before.Spec == want && before.Running && len(reasons) == 0: + case existed && (before.Spec == want || legacy) && before.Running && len(reasons) == 0: out.Action = "unchanged" return out, nil case existed: @@ -1478,10 +1499,6 @@ func applyContainer(ctx context.Context, r *declaration.Container, run Runner, } case len(reasons) > 0: out.Detail = "recreated to pick up " + strings.Join(reasons, ", ") - case previous.Reads == nil && len(reads) > 0: - // A container made before this host kept what it read: whether the files it holds - // are the ones on disk cannot be known, so it is recreated once and from now on can. - out.Detail = "recreated: what it reads was not on record, so what it holds could not be checked" default: out.Detail = "replaced; a container's configuration is fixed when it is created" } @@ -1650,6 +1667,20 @@ func restartedBy(restartOn []string, changed map[string]bool) []string { return which } +// sameReads is whether two records of what a container reads name the same files holding the +// same content — a file added, dropped or changed makes them differ. +func sameReads(a, b map[string]string) bool { + if len(a) != len(b) { + return false + } + for path, digest := range a { + if b[path] != digest { + return false + } + } + return true +} + func sortedKeys(m map[string]string) []string { keys := make([]string, 0, len(m)) for k := range m { diff --git a/internal/apply/reads_test.go b/internal/apply/reads_test.go index a55507b..be2d8fe 100644 --- a/internal/apply/reads_test.go +++ b/internal/apply/reads_test.go @@ -132,24 +132,27 @@ func TestAContainerIsRecreatedWhenAMountedSecretChanged(t *testing.T) { } } -func TestWhatAServiceWritesInAMountedDirectoryIsNotPartOfWhatItIs(t *testing.T) { - // A bind-mounted directory is the service's data: it changes while the service runs, and - // recreating for it would restart a database for every row it wrote. What the HOST wrote - // under that directory — a config it rendered there — is another matter: the service read - // that once, at start. +func TestAMountedDirectoryIsNotLookedInside(t *testing.T) { + // A bind-mounted directory is not part of what a container is — not the data the service + // grows in it, and not the files the host itself writes there either. Whether a service reads + // a file under its directory once at start or watches it live is the service's business: the + // route proxy re-reads its routes live, a provisioner sidecar polls what it receives every few + // seconds, and recreating either for a file the host rewrote would kill them for nothing. A + // module whose container does read such a file once says so with restart-on, which stays the + // opt-in. dir := t.TempDir() state := filepath.Join(dir, "state") config := filepath.Join(state, "config.toml") - declare := func(level string) *declaration.Declaration { + declare := func(level, restartOn string) *declaration.Declaration { return parseTrusted(t, `{"declaration":1,"resources":[ {"id":"app.state","type":"directory","path":"`+state+`"}, {"id":"app.config","type":"file","path":"`+config+`","content":"level = \"`+level+`\"\n"}, {"id":"app.server","type":"container","name":"app","image":"`+pinned+`", - "volumes":["`+state+`:/var/lib/app"]} + "volumes":["`+state+`:/var/lib/app"]`+restartOn+`} ]}`) } m := &machine{containers: map[string]*fakeContainer{}} - _, known := applyCarried(t, declare("info"), store.State{}, m, nil) + _, known := applyCarried(t, declare("info", ""), store.State{}, m, nil) // The service grows its data in the directory it was given. if err := os.WriteFile(filepath.Join(state, "app.db"), []byte("rows"), 0o600); err != nil { @@ -162,20 +165,112 @@ func TestWhatAServiceWritesInAMountedDirectoryIsNotPartOfWhatItIs(t *testing.T) t.Fatal(err) } m.asked = nil - report, known := applyCarried(t, declare("info"), known, m, nil) + report, known := applyCarried(t, declare("info", ""), known, m, nil) if m.did("docker rm") || m.did("docker run") || report.Changed() { t.Errorf("a container was recreated for data its service wrote in a mounted directory: %v %+v", m.asked, report.Outcomes) } - // The host's own config under the same directory changes: that, the service read at start. + // The host rewrites its own file under the same directory: still not a reason. The container + // did not name it. m.asked = nil - report, _ = applyCarried(t, declare("debug"), known, m, nil) - if !m.removed("app") || !m.did("docker run") { - t.Fatalf("a config the host wrote under a mounted directory changed and the container was not recreated: %v", m.asked) + report, known = applyCarried(t, declare("debug", ""), known, m, nil) + if m.did("docker rm") || m.did("docker run") { + t.Errorf("a container was recreated for a file under a mounted directory it did not name: %v", m.asked) } - if o := outcomeOf(report, "app.server"); o.Detail != "recreated: "+config+" changed" { - t.Errorf("the recreate did not name the config: %+v", o) + if o := outcomeOf(report, "app.config"); o.Action != "updated" { + t.Fatalf("the config was not rewritten: %+v", o) + } + + // Naming it is what makes it a reason, as before this change. + m.asked = nil + report, _ = applyCarried(t, declare("trace", `,"restart-on":["app.config"]`), known, m, nil) + if !m.removed("app") || !m.did("docker run") { + t.Fatalf("a container naming a rewritten file under its mount was not recreated: %v", m.asked) + } + if o := outcomeOf(report, "app.server"); !strings.Contains(o.Detail, "app.config") { + t.Errorf("the recreate did not name why: %+v", o) + } +} + +func TestAContainerLabelledBeforeTheHostReadItsFilesIsAcceptedNotRecreated(t *testing.T) { + // The first apply after the host upgrades finds every container carrying a label computed + // without the file lines. Recreating them all would be a restart storm across the mesh in + // declaration order, the store first. A label that matches the spec as it used to be computed + // is accepted: what the container reads is recorded now, and from then on a change is caught. + dir := t.TempDir() + env := filepath.Join(dir, "forge.env") + declare := func(port string) *declaration.Declaration { + return parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"forge.env","type":"file","path":"`+env+`","content":"DATABASE_PORT=`+port+`\n","mode":"0600"}, + {"id":"forge.server","type":"container","name":"forge","image":"`+pinned+`","env-file":["`+env+`"]} + ]}`) + } + // The machine as the previous host left it: the file written and recorded, the container up + // under the label that host computed — the spec with nothing about the file's content. + if err := os.WriteFile(env, []byte("DATABASE_PORT=5432\n"), 0o600); err != nil { + t.Fatal(err) + } + d := declare("5432") + legacy := containerSpecReading(d.Resources[1].(*declaration.Container), nil, nil) + known := store.State{} + known.Record(store.Applied{ID: "forge.env", Type: "file", Origin: store.OriginCarried, Target: env, + Wrote: digestOf("DATABASE_PORT=5432\n")}) + known.Record(store.Applied{ID: "forge.server", Type: "container", Origin: store.OriginCarried, Target: "forge"}) + m := &machine{containers: map[string]*fakeContainer{"forge": {id: "made-by-host", running: true, spec: legacy}}} + + report, known := applyCarried(t, d, known, m, nil) + if m.did("docker rm") || m.did("docker run") || report.Changed() { + t.Fatalf("a container labelled by the previous host was recreated on upgrade: %v %+v", m.asked, report.Outcomes) + } + if got, _ := known.Find("forge.server"); got.Reads[env] != digestOf("DATABASE_PORT=5432\n") { + t.Fatalf("what the accepted container reads was not recorded: %+v", got) + } + // Accepted stays accepted: the next pass with nothing moved is quiet too. + m.asked = nil + report, known = applyCarried(t, d, known, m, nil) + if m.did("docker rm") || m.did("docker run") || report.Changed() { + t.Fatalf("an accepted container was recreated on the pass after: %v", m.asked) + } + + // And a change to the file is caught from the record, and the label is renewed. + m.asked = nil + report, _ = applyCarried(t, declare("5433"), known, m, nil) + if !m.removed("forge") || !m.did("docker run") { + t.Fatalf("an accepted container was not recreated when its env-file changed: %v", m.asked) + } + if o := outcomeOf(report, "forge.server"); o.Detail != "recreated: "+env+" changed" { + t.Errorf("the recreate did not name the file: %+v", o) + } + if m.containers["forge"].spec == legacy { + t.Error("the recreated container still carries the legacy label") + } +} + +func TestAContainerAdoptedUnderANewIdStillSaysWhichFileChanged(t *testing.T) { + // The bundle's `store` becomes the postgres module's `postgres.server`: the same container by + // name, under a new id with no record of its own. A file change on that day is a real change, + // and the record of what it read is under the old id — by name, it is found. + dir := t.TempDir() + env := filepath.Join(dir, "store.env") + m := &machine{containers: map[string]*fakeContainer{}} + raised := parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"env","type":"file","path":"`+env+`","content":"PORT=5432\n","mode":"0600"}, + {"id":"store","type":"container","name":"mesh-store","image":"`+pinned+`","env-file":["`+env+`"]} + ]}`) + _, known := applyCarried(t, raised, store.State{}, m, nil) + + adopted := parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"postgres.env","type":"file","path":"`+env+`","content":"PORT=5433\n","mode":"0600"}, + {"id":"postgres.server","type":"container","name":"mesh-store","image":"`+pinned+`","env-file":["`+env+`"]} + ]}`) + m.asked = nil + report, _, err := Apply(context.Background(), archHost(t), adopted, known, store.OriginDeclared, m.run, nil, nil) + if err != nil { + t.Fatal(err) + } + if o := outcomeOf(report, "postgres.server"); o.Action != "updated" || o.Detail != "recreated: "+env+" changed" { + t.Errorf("a container adopted under a new id did not say which file changed: %+v", o) } } diff --git a/internal/bootstrap/adopt_in_place_test.go b/internal/bootstrap/adopt_in_place_test.go new file mode 100644 index 0000000..9814cf7 --- /dev/null +++ b/internal/bootstrap/adopt_in_place_test.go @@ -0,0 +1,130 @@ +package bootstrap + +import ( + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/novox/mesh-host/internal/apply" + "github.com/novox/mesh-host/internal/declaration" + "github.com/novox/mesh-host/internal/store" +) + +// Defends phase three's premise (phase3.go): the store genesis raised is adopted by the postgres +// module IN PLACE — same name, same image, same spec — so the applier reconciles it and never +// recreates the mesh's memory with the temporary control plane connected to it. +// +// The host folds a mounted file's content into the container's spec (novox/hq 04-ISSUES/103), so +// this now depends on a byte: the superuser file genesis writes and mounts must be the same bytes +// the module later declares. The module's value is what `secret accept` took — the operator's +// file with its line ending removed and nothing else (mesh-control, asSupplied). Reproduced before +// it was fixed: genesis wrote `value\n`, the module wrote `value`, and the store was recreated +// during install. + +// labelled is a runtime that keeps the spec label the host gives a container and hands it back. +type labelled struct { + spec map[string]string + created []string + removed []string +} + +func (l *labelled) run(_ context.Context, _ string, args ...string) (string, error) { + switch args[0] { + case "info": + return "27.0\n", nil + case "inspect": + spec, ok := l.spec[args[len(args)-1]] + if !ok { + return "", errors.New("no such container") + } + return "true\t" + spec + "\n", nil + case "rm": + l.removed = append(l.removed, args[len(args)-1]) + delete(l.spec, args[len(args)-1]) + case "run": + var name, spec string + for i, a := range args { + if a == "--name" { + name = args[i+1] + } + if a == "--label" && strings.HasPrefix(args[i+1], "mesh-host.spec=") { + spec = strings.TrimPrefix(args[i+1], "mesh-host.spec=") + } + } + l.spec[name] = spec + l.created = append(l.created, name) + return "made\n", nil + } + return "", nil +} + +func TestTheStoreGenesisRaisedIsAdoptedInPlaceNotRecreated(t *testing.T) { + dir := t.TempDir() + secret := filepath.Join(dir, "superuser.secret") + + // The bytes genesis really writes — the code path, not a fixture that agrees with it. + if _, made, err := keptOrMade(secret, false); err != nil || !made { + t.Fatalf("genesis did not make the superuser secret: made=%v err=%v", made, err) + } + onDisk, err := os.ReadFile(secret) + if err != nil { + t.Fatal(err) + } + + image := "docker.io/library/postgres@sha256:" + strings.Repeat("ab", 32) + mounts := `"volumes":["mesh-store-data:/var/lib/postgresql/data","` + secret + `:` + storeSuperuserMount + `:ro"]` + + // The foundation's store, as the produced bundle raises it (RewriteRoot): the file mounted, + // declared by nothing — genesis wrote it before there was a declaration to name it. + raise, err := declaration.ParseTrusted([]byte(`{"declaration":1,"resources":[ + {"id":"` + StoreID + `","type":"container","name":"mesh-store","image":"` + image + `", + "env":{"POSTGRES_PASSWORD_FILE":"` + storeSuperuserMount + `"},` + mounts + `} + ]}`)) + if err != nil { + t.Fatal(err) + } + runtime := &labelled{spec: map[string]string{}} + _, known, err := apply.Apply(context.Background(), arch(t), raise, store.State{}, store.OriginCarried, + runtime.run, nil, nil) + if err != nil { + t.Fatalf("raising the foundation's store: %v", err) + } + if len(runtime.created) != 1 { + t.Fatalf("the store was not raised once: %v", runtime.created) + } + + // The postgres module's declaration of the same store: the superuser file as `secret accept` + // took it in — its line ending removed and nothing else — then the same container. + accepted := strings.TrimRight(string(onDisk), "\r\n") + adopt, err := declaration.ParseTrusted([]byte(`{"declaration":1,"resources":[ + {"id":"postgres.superuser","type":"file","path":"` + secret + `","content":"` + accepted + `","mode":"0600"}, + {"id":"postgres.server","type":"container","name":"mesh-store","image":"` + image + `", + "env":{"POSTGRES_PASSWORD_FILE":"` + storeSuperuserMount + `"},` + mounts + `} + ]}`)) + if err != nil { + t.Fatal(err) + } + runtime.created, runtime.removed = nil, nil + report, _, err := apply.Apply(context.Background(), arch(t), adopt, known, store.OriginDeclared, + runtime.run, nil, nil) + if err != nil { + t.Fatalf("adopting the store: %v", err) + } + + if len(runtime.removed) > 0 || len(runtime.created) > 0 { + t.Fatalf("the module's declaration recreated the store genesis raised (removed %v, created %v): "+ + "the file genesis mounted and the file the module declares are not the same bytes", + runtime.removed, runtime.created) + } + for _, o := range report.Outcomes { + if o.ID == "postgres.server" && o.Action != "unchanged" { + t.Errorf("the store was not adopted in place: %+v", o) + } + if o.ID == "postgres.superuser" && o.Action != "unchanged" { + t.Errorf("the module rewrote the superuser file genesis wrote: %+v", o) + } + } +} diff --git a/internal/bootstrap/rootsecrets.go b/internal/bootstrap/rootsecrets.go index 06a81f2..4b5ec7c 100644 --- a/internal/bootstrap/rootsecrets.go +++ b/internal/bootstrap/rootsecrets.go @@ -102,8 +102,16 @@ func keptOrMade(path string, dryRun bool) (value string, made bool, err error) { } // Written whole and renamed into place, at 0600, owned by whoever runs the installer — root, // which is also who the host runs as when it later writes the sealed copy here. + // + // **The value alone, no line ending.** The module that adopts the store declares this same + // file, and what it declares is the value as `secret accept` took it — its line ending gone, + // by design. The host folds a mounted file's content into the container's spec (novox/hq + // 04-ISSUES/103), so a genesis that wrote `value\n` here would raise a store whose label + // digests one byte more than the module's file, and phase three would RECREATE the store it + // meant to adopt in place, with the temporary control plane connected to it. readCredentialFile + // tolerates either ending, so a file an earlier genesis wrote still reads. tmp := path + ".genesis" - if err := os.WriteFile(tmp, []byte(value+"\n"), 0o600); err != nil { + if err := os.WriteFile(tmp, []byte(value), 0o600); err != nil { return "", false, err } if err := os.Rename(tmp, path); err != nil { diff --git a/internal/declaration/declaration.go b/internal/declaration/declaration.go index 42aea06..d7e877d 100644 --- a/internal/declaration/declaration.go +++ b/internal/declaration/declaration.go @@ -785,13 +785,14 @@ type Container struct { // while every check passes because the file on disk is right. The host recreates the container // when one of these resources changed this pass, even if the spec matches. // - // What a running container reads at creation — its env-files, a file mounted into it, and the - // files the host wrote under a directory mounted into it — is part of its spec by content - // since novox/hq 04-ISSUES/103, and needs no naming here. RestartOn is for what the spec - // cannot see: a resource the container reflects without reading it directly, a step it - // consumes the result of. 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). + // What a running container reads at creation — its env-files, and a file mounted into it + // directly — is part of its spec by content since novox/hq 04-ISSUES/103, and needs no naming + // here. A directory mounted into it is NOT looked inside, not even for files the host wrote + // there: whether a service reads such a file once or watches it live is the service's, and + // RestartOn is how a module says "once, at start" — a config the host renders under the + // module's state directory, a step whose result it consumes. 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, diff --git a/internal/store/store.go b/internal/store/store.go index 09602e1..eb8264d 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -18,7 +18,6 @@ import ( "os" "path/filepath" "sort" - "strings" "time" ) @@ -209,24 +208,26 @@ func (s State) Recorded(kind, target string) bool { return false } -// FilesUnder returns every file this host has a record of writing at a path or beneath it, of any -// origin and under any id, in path order. +// At returns what this host has a record of putting at a target of this kind, under any id and of +// any origin — Recorded, with the record. // -// By path rather than by id because a container names what it reads by path: the id a file was -// declared under may change — the bundle's, then a module's, for the same file — and the container -// reading it does not care which. -func (s State) FilesUnder(path string) []Applied { - var out []Applied +// By target rather than by id because the id a thing was declared under may change while the thing +// does not: the bundle's `store` becomes a module's `postgres.server` for the same container, and +// the file that container reads is the same file under either id. Both ids may then hold a record +// for the one target — the bundle's is never removed by the mesh's declaration — and the most +// recently applied is the one that says what is there now. +func (s State) At(kind, target string) (Applied, bool) { + var latest Applied + found := false for _, r := range s.Resources { - if r.Type != "file" { + if r.Type != kind || r.Target != target { continue } - if r.Target == path || strings.HasPrefix(r.Target, strings.TrimSuffix(path, "/")+"/") { - out = append(out, r) + if !found || r.AppliedAt.After(latest.AppliedAt) { + latest, found = r, true } } - sort.Slice(out, func(i, j int) bool { return out[i].Target < out[j].Target }) - return out + return latest, found } // HeldAt returns what is held under a resource id. -- 2.54.0 From 4840e21405a4a86f04b3047a036f7fe301a45274 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 23 Sep 2026 23:42:41 +0200 Subject: [PATCH 3/3] Say in the plan which file a container would be recreated for MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The plan says what an apply would change from the declaration and the record, before the machine is touched. The apply now recreates a container when the content of a file it reads at creation changed, and the plan said "check" for every recorded container — true, but a preview that hides the one step somebody asked about. So a recorded container whose record of what it read differs from what this apply will hand it — a plain file declared here, by its declared content; otherwise what this host last wrote at that path — is planned as an update naming the file, the same comparison applyContainer makes. What the record cannot settle stays a check: a file neither declared nor recorded is read from the machine by the apply, not by the plan; and a container with no record of what it read was labelled before the host kept that record and is accepted as it is. novox/hq 04-ISSUES/103, 104 --- internal/apply/plan.go | 58 +++++++++++++++++++++++++++++++++++++ internal/apply/plan_test.go | 36 +++++++++++++++++++++++ 2 files changed, 94 insertions(+) diff --git a/internal/apply/plan.go b/internal/apply/plan.go index ced96cc..07205bd 100644 --- a/internal/apply/plan.go +++ b/internal/apply/plan.go @@ -191,10 +191,68 @@ func planned(r declaration.Resource, d *declaration.Declaration, known store.Sta step.Verb, step.Why = "update", "the declaration changed since this host applied it" return step } + if c, ok := r.(*declaration.Container); ok { + if changed := readsChanged(c, d, known, was.Reads); len(changed) > 0 { + step.Verb = "update" + step.Why = "recreated: " + strings.Join(changed, ", ") + " changed since it was created" + return step + } + } step.Verb, step.Why = "check", "recorded here; corrected if this machine drifted from it" return step } +// readsChanged is which of the files a container was created reading the apply will hand it +// changed — the same comparison applyContainer makes (novox/hq 04-ISSUES/103), settled from the +// declaration and the record alone. +// +// A file's digest is what this apply will record for it: a plain file declared here, by its +// declared content; otherwise what this host last wrote there, under any id. A file neither +// declares nor records — an env-file a predecessor left — is read by the apply from the machine, +// which a plan does not do, so it stays a check. A container with no record of what it read was +// labelled before the host kept that record and is accepted as it is, so it is a check too. +func readsChanged(c *declaration.Container, d *declaration.Declaration, known store.State, + wasReading map[string]string) []string { + if len(wasReading) == 0 { + return nil + } + willWrite := map[string]string{} + for _, r := range d.Resources { + if f, ok := r.(*declaration.File); ok { + if want := wouldWrite(f); want != "" { + willWrite[f.Path] = want + } + } + } + // Only what it still reads: a file it was created reading and no longer names is a changed + // declaration, not a changed file. + stillReads := map[string]bool{} + for _, path := range c.EnvFile { + stillReads[path] = true + } + for _, v := range c.Volumes { + if src := mountSource(v); strings.HasPrefix(src, "/") { + stillReads[src] = true + } + } + var changed []string + for _, path := range sortedKeys(wasReading) { + if !stillReads[path] { + continue + } + now, settled := willWrite[path] + if !settled { + if f, recorded := known.At(string(declaration.TypeFile), path); recorded { + now, settled = f.Wrote, true + } + } + if settled && now != wasReading[path] { + changed = append(changed, path) + } + } + return changed +} + // wouldWrite is the digest a plain file would be recorded under, or empty where only the apply // can know: a sealed file, one with secrets in it, one written into, one carrying bytes. func wouldWrite(r declaration.Resource) string { diff --git a/internal/apply/plan_test.go b/internal/apply/plan_test.go index 768230a..4f8f7ce 100644 --- a/internal/apply/plan_test.go +++ b/internal/apply/plan_test.go @@ -225,3 +225,39 @@ func TestAResourceRunInAHeldContainerIsPlannedAsItIsApplied(t *testing.T) { t.Errorf("planned %q", got) } } + +func TestAPlanSaysAContainerIsRecreatedWhenAFileItReadsChanged(t *testing.T) { + // The apply recreates a container when the content of a file it reads at creation changed + // (novox/hq 04-ISSUES/103); the plan says so from the record alone — what the container was + // created reading, against what this apply will write. And a container recorded before the + // host kept that record is accepted, so it is a check, not an update. + dir := t.TempDir() + env := filepath.Join(dir, "forge.env") + declare := func(port string) *declaration.Declaration { + return trusted(t, `{"declaration":1,"resources":[ + {"id":"forge.env","type":"file","path":"`+env+`","content":"DATABASE_PORT=`+port+`\n"}, + {"id":"forge.server","type":"container","name":"forge","image":"`+pinned+`","env-file":["`+env+`"]}]}`) + } + created := digestOf("DATABASE_PORT=5432\n") + known := store.State{} + known.Record(store.Applied{ID: "forge.env", Type: "file", Target: env, Wrote: created}) + known.Record(store.Applied{ID: "forge.server", Type: "container", Target: "forge", + Reads: map[string]string{env: created}}) + + if got := verbs(Plan(declare("5432"), known, store.OriginCarried)); got != "check forge.env, check forge.server" { + t.Errorf("nothing changed and the plan says %q", got) + } + steps := Plan(declare("5433"), known, store.OriginCarried) + if got := verbs(steps); got != "update forge.env, update forge.server" { + t.Fatalf("the env-file changes and the plan says %q", got) + } + if !strings.Contains(steps[1].Why, env+" changed") { + t.Errorf("the plan does not say which file: %+v", steps[1]) + } + + // No record of what it read: labelled by an earlier host, accepted as it is. + known.Record(store.Applied{ID: "forge.server", Type: "container", Target: "forge"}) + if got := verbs(Plan(declare("5433"), known, store.OriginCarried)); got != "update forge.env, check forge.server" { + t.Errorf("a container with no record of what it read is planned as %q", got) + } +} -- 2.54.0