From 81672b183df7d57f814ac181baab4c04f9a7d5c1 Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 29 Oct 2026 21:25:51 +0100 Subject: [PATCH] Keep the node's own as it runs from an engine's first start, and refuse the node's own apply removing a module no want removed (issue 774, review round 1) The reviewer showed how 76e214a still lets the 07:54 outage happen at the re-landed engine's first start on novox. applied/ is empty there, so loadApplied loads nothing. The kept declaration retires, because every placed module has a have. Nothing then carries the node's own composition. The guard misses nats, which has no want there, so its nats.conf is guessed as nats's and removed. - At the start, when the store has no node's own composition, the node's own runs as the kept declaration has it, and the store keeps it now. The kept declaration retires only once the store holds `node`. - What this engine applied runs until loadHad reads the composition a differing have names. The bucket no longer keeps the one novox's have names, so the stored one stays. - In the node's own apply, a resource of another module goes only when that module's want says removed (Scope.RemovedByWant). A module the node's own composition carried has no want, so Placed never named it. Cost: the node's own apply cannot drop one of its own dotted resources without such a want. It is refused whole and said, never removed silently. Tests, with no new test functions: - A first-start case in the agent's Everything test (empty store, then a start with the declaration retired). It fails on 76e214a. - Two rows in the guard's table: no want refuses; a want saying removed removes. - A case for the store in the authority test. --- cmd/mesh-host/applied_store.go | 10 ++++++ cmd/mesh-host/authority_test.go | 32 ++++++++++++++++++ cmd/mesh-host/nodeagent.go | 24 +++++++++++--- internal/agent/agent.go | 51 ++++++++++++++++++++++++++--- internal/agent/agent_test.go | 50 ++++++++++++++++++++++++++++ internal/apply/apply.go | 17 ++++++++-- internal/apply/module_scope_test.go | 12 +++++++ 7 files changed, 184 insertions(+), 12 deletions(-) diff --git a/cmd/mesh-host/applied_store.go b/cmd/mesh-host/applied_store.go index 60df951..75243ed 100644 --- a/cmd/mesh-host/applied_store.go +++ b/cmd/mesh-host/applied_store.go @@ -30,6 +30,16 @@ func (s appliedStore) path(module string) (string, error) { return filepath.Join(s.dir, module+".json"), nil } +// holds says whether the store keeps the module's composition. +func (s appliedStore) holds(module string) bool { + path, err := s.path(module) + if err != nil { + return false + } + _, err = os.Stat(path) + return err == nil +} + func (s appliedStore) Load() (map[string][]byte, error) { entries, err := os.ReadDir(s.dir) if errors.Is(err, os.ErrNotExist) { diff --git a/cmd/mesh-host/authority_test.go b/cmd/mesh-host/authority_test.go index 84d8ea9..ac7bdcb 100644 --- a/cmd/mesh-host/authority_test.go +++ b/cmd/mesh-host/authority_test.go @@ -127,6 +127,38 @@ func TestTheAppliedAuthorityIsKeptForTheRecord(t *testing.T) { if err := certifyThisNode(context.Background(), state, "n1", trust, ask, func() { written++ }); err == nil || written != 1 { t.Errorf("a certificate of another authority was kept: %v", err) } + + // Issue 774: what the engine applied is kept beside the state too, one file per module, 0600 in a 0700 directory, + // written by a rename so a crash leaves the one before; a name that is no module's is refused, never a path. + t.Run("the compositions the engine applied are kept beside the state", func(t *testing.T) { + applied := appliedStoreFor(filepath.Join(t.TempDir(), "state", "state.json")) + if m, err := applied.Load(); err != nil || len(m) != 0 || applied.holds("node") { + t.Fatalf("an empty store loads %v, %v", m, err) + } + for _, tc := range []struct { + module string + takes bool + }{{"node", true}, {"nats", true}, {"mesh-delivery-view", true}, {"../state", false}, {"a/b", false}, {"..", false}, {"", false}} { + if err := applied.Keep(tc.module, []byte(`{"v":1}`)); (err == nil) != tc.takes { + t.Errorf("keeping %q: %v, want taken %v", tc.module, err, tc.takes) + } + } + if err := applied.Keep("node", []byte(`{"v":2}`)); err != nil { + t.Fatal(err) + } + entries, _ := os.ReadDir(applied.dir) + dirInfo, _ := os.Stat(applied.dir) + fileInfo, _ := os.Stat(filepath.Join(applied.dir, "node.json")) + if len(entries) != 3 || dirInfo.Mode().Perm() != 0o700 || fileInfo.Mode().Perm() != 0o600 { + t.Fatalf("the store holds %d files (want 3, no temporary one left), directory %v, file %v", len(entries), dirInfo.Mode(), fileInfo.Mode()) + } + if err := applied.Drop("nats"); err != nil || applied.Drop("nats") != nil || applied.holds("nats") { + t.Fatalf("a dropped module is still held, or dropping twice fails: %v", err) + } + if m, err := applied.Load(); err != nil || string(m["node"]) != `{"v":2}` || len(m) != 2 { + t.Fatalf("the store loads %v, %v", m, err) + } + }) } // testAuthority is a self-signed authority certificate for key, as the vault makes its own. diff --git a/cmd/mesh-host/nodeagent.go b/cmd/mesh-host/nodeagent.go index 07fc79c..00531d8 100644 --- a/cmd/mesh-host/nodeagent.go +++ b/cmd/mesh-host/nodeagent.go @@ -454,8 +454,15 @@ func nodeAgent(ctx context.Context, opts options, signer ed25519.PublicKey, trus var ag *agent.Agent // The kept release-5 declaration is no longer read once every module the seat's placement puts here has a have of its // own (ADR 0550 §2: its signature is the controller's, trusted no more after the lease write). - if retired, why := placedAllHave(linked, session, node); retired && !keptRetired.Swap(true) { - say("the kept declaration is no longer read: " + why) + // Nor before this engine keeps the node's own as it runs (applied/node.json): until then the kept declaration is the one + // record of what the node's own composition carries, its folded modules among them (issue 774, review of the + // re-land, round 1: novox's first start would compose its own with nats running nothing). + if retired, why := placedAllHave(linked, session, node); retired && !keptRetired.Load() { + if !appliedStoreFor(opts.state).holds(agent.NodeModule) { + say("the kept declaration is still read: " + why + ", but this engine keeps no node's own composition yet") + } else if !keptRetired.Swap(true) { + say("the kept declaration is no longer read: " + why) + } } kept := keptOnce(opts, signer) // The mesh's own certificate the declaration kept from before carries, kept for the record before this machine @@ -529,7 +536,7 @@ func nodeAgent(ctx context.Context, opts options, signer ed25519.PublicKey, trus if herr != nil { res = failedWith(herr.Error()) } else { - res = applyComposition(actx, opts, held, others, ag.Placed(scopeName(c)), sched, say) + res = applyComposition(actx, opts, held, others, ag.Placed(scopeName(c)), ag.RemovedByWant(), sched, say) } done() // The mesh's own certificate it carries is kept for this machine's record (ADR 0555 §3). @@ -690,7 +697,10 @@ func (c *agentNow) get() *agent.Agent { // // placed is the modules a want still places here besides this one: none of their resources is removed by this apply, // which is refused instead (issue 774). -func applyComposition(ctx context.Context, opts options, c delivery.Composed, others []delivery.Composed, placed map[string]bool, +// +// removed is the modules whose want says removed: in the node's own apply, a resource of any other module goes only when +// its module is among them (issue 774). +func applyComposition(ctx context.Context, opts options, c delivery.Composed, others []delivery.Composed, placed, removed map[string]bool, sched *apply.Scheduler, say link.Announce) agent.Result { name := scopeName(c) if c.Module == "" { @@ -707,6 +717,12 @@ func applyComposition(ctx context.Context, opts options, c delivery.Composed, ot return failedWith(err.Error()) } scope := apply.Scope{Module: name, Others: resourcesOf(others), Placed: placed} + if name == agent.NodeModule { + scope.RemovedByWant = removed + if scope.RemovedByWant == nil { + scope.RemovedByWant = map[string]bool{} + } + } got := applyModuleHeld(ctx, opts, d, &scope, sched, say) if got.Refused != "" { return failedWith(got.Refused) diff --git a/internal/agent/agent.go b/internal/agent/agent.go index 0b47ede..7b6494c 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -904,7 +904,7 @@ func (a *Agent) loadHad(ctx context.Context) { // loadApplied takes, once at the start, what this engine last applied of each module as what runs of it, ahead of the // bucket and the kept declaration (Env.Applied, issue 774): it is what the machine holds, whatever the bus still keeps. -// One whose have names another composition that runs (applied since by another engine) is left to the have; one whose +// One whose have names another composition runs as applied until loadHad reads that one from the bucket; one whose // have cannot be read, or that has none, is kept as applied, so a module is never composed as running nothing. func (a *Agent) loadApplied(ctx context.Context) { if a.env.Applied == nil { @@ -914,6 +914,30 @@ func (a *Agent) loadApplied(ctx context.Context) { if err != nil { a.env.Say(fmt.Sprintf("the compositions this engine applied cannot be read, so what runs is read from the haves alone: %v", err)) } + // At an engine's first start nothing is kept yet: the node's own runs as the kept declaration has it, which the store + // keeps now, so the node never composes its own as if its folded modules ran nothing once the declaration retires + // (issue 774, review of the re-land, round 1). Taken whatever digest the node's have names: the composition that names + // is in no bucket, and the declaration is the one record of what runs on disk. + seeded := false + if _, own := kept[NodeModule]; !own && err == nil && a.env.Kept != nil { + if rs, ok := a.env.Kept()[NodeModule]; ok && len(rs) > 0 { + c, _ := a.keptComposed(NodeModule, rs) + c.Module = "" + raw, eerr := delivery.EncodeComposed(c) + if eerr == nil { + eerr = a.env.Applied.Keep(NodeModule, raw) + } + if eerr != nil { + a.env.Say(fmt.Sprintf("the node's own as the kept declaration has it cannot be kept: %v", eerr)) + } else { + if kept == nil { + kept = map[string][]byte{} + } + kept[NodeModule], seeded = raw, true + a.env.Say("the node's own runs as the kept declaration has it, kept now as what this engine runs") + } + } + } names := make([]string, 0, len(kept)) for m := range kept { names = append(names, m) @@ -933,10 +957,13 @@ func (a *Agent) loadApplied(ctx context.Context) { continue } h, found, unread := a.readHave(ctx, m) - if found && h.Composed != "" && h.Composed != dig && haveRuns(h) { - a.env.Say(fmt.Sprintf("%s's have names the composition %s, not %s this engine applied last: what runs of it is read from its have", - m, short(h.Composed), short(dig))) - continue + if seeded && m == NodeModule { + found, unread = true, false // what the kept declaration has runs, whatever the have names + } else if found && h.Composed != "" && h.Composed != dig && haveRuns(h) { + // The composition the have names replaces this once loadHad finds it in the bucket; until then, and when + // the bucket keeps it no more, what this engine applied runs (issue 774, review of the re-land, round 1). + a.env.Say(fmt.Sprintf("%s's have names the composition %s, not %s this engine applied last: it runs as applied "+ + "until the one its have names is read", m, short(h.Composed), short(dig))) } if m == NodeModule { c.Module, c.FP = "", "" @@ -1151,6 +1178,20 @@ func (a *Agent) Placed(except string) map[string]bool { return out } +// RemovedByWant is the modules whose want says removed (apply.Scope.RemovedByWant, issue 774): in the node's own apply, +// only their resources may go. Called without a.mu held. +func (a *Agent) RemovedByWant() map[string]bool { + a.mu.Lock() + defer a.mu.Unlock() + out := map[string]bool{} + for m, w := range a.wants { + if m != NodeModule && w.Present && w.Want.Removed { + out[m] = true + } + } + return out +} + // HoldsNode says whether the agent runs the node's own composition, so the node's parts are its and not the kept // declaration's. func (a *Agent) HoldsNode() bool { diff --git a/internal/agent/agent_test.go b/internal/agent/agent_test.go index 4aa7d9f..9ec0741 100644 --- a/internal/agent/agent_test.go +++ b/internal/agent/agent_test.go @@ -835,6 +835,56 @@ func TestEverythingIsWhatTheAgentRunsAndWhatIsKeptForTheRest(t *testing.T) { b.wants.put(wk, rraw) eventually(t, "the removed module's composition was not forgotten", func() bool { return store.get("web") == nil }) }) + + // Issue 774, review of the re-land, round 1: at the re-landed engine's first start nothing is kept yet, and the node's + // have names a composition the node composed itself, in no bucket. The node's own runs as the kept declaration has it + // and is kept then, so the next start, the declaration retired, still runs nats. + t.Run("at the first start nothing is kept: the node's own runs as the kept declaration has it", func(t *testing.T) { + natsConf, _ := json.Marshal(map[string]any{"id": "nats.conf", "type": "file", "path": "/tmp/r6/nats/conf", "content": "listen 4222"}) + store := &memApplied{kept: map[string][]byte{}} + h := &haves{} + h.Write(context.Background(), Have{Module: NodeModule, Composed: "sha256:" + strings.Repeat("c", 64)}) + firstPass := func(kept map[string][]json.RawMessage) []delivery.Composed { + b := newBed(t) + b.env.SelfComposed, b.env.Trusted, b.kept, b.env.Applied, b.env.Haves = true, nil, kept, store, h + firstCh := make(chan []delivery.Composed, 1) + comps := b.comps + var ag *Agent + b.env.Composed = watchFunc(func(ctx context.Context, p string) (<-chan Update, error) { + firstCh <- ag.Everything() + return comps.Watch(ctx, p) + }) + ag = New(b.env) + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan struct{}) + go func() { defer close(done); _ = ag.Run(ctx) }() + defer func() { cancel(); <-done }() + select { + case got := <-firstCh: + return got + case <-time.After(5 * time.Second): + t.Fatal("the composer was not watched") + } + return nil + } + runsNats := func(cs []delivery.Composed) bool { + for _, c := range cs { + if c.Module == "" && len(c.Resources) == 1 && strings.Contains(string(c.Resources[0]), "4222") { + return true + } + } + return false + } + if got := firstPass(map[string][]json.RawMessage{NodeModule: {natsConf}}); !runsNats(got) { + t.Fatalf("the first start does not run nats in the node's own (%d compositions)", len(got)) + } + if store.get(NodeModule) == nil { + t.Fatal("the first start kept no node's own composition") + } + if got := firstPass(nil); !runsNats(got) { + t.Fatalf("the next start, the kept declaration retired, does not run nats in the node's own (%d compositions)", len(got)) + } + }) } // watchFunc is a Watcher from a function. diff --git a/internal/apply/apply.go b/internal/apply/apply.go index 24f93c5..8eb83d8 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -316,23 +316,34 @@ type Scope struct { // the module's own removes a resource of one of them (issue 774: on 2026-10-29 the node's own apply on the bus node, // composed without what its folded modules ran, removed the bus server and four held modules). Nil guards nothing. Placed map[string]bool + // RemovedByWant are the modules whose want says removed. In the node's own apply (Module NodeModule) with this set + // non-nil, a resource of any other module is removed only when that module is here: a module the node runs folded + // in its own composition has no want, so Placed does not name it (issue 774, review of the re-land, round 1: nats on + // the bus node has none, and its nats.conf was guessed as nats's and removed). Nil guards by Placed alone. + RemovedByWant map[string]bool } // placedOwner is the module a want still places on the node that the record belongs to, other than the scope's own: // the module recorded with it, else the one its id names (a resource the node's own composition carried for a folded // module is recorded as the node's, its id naming the module). Empty when there is none. func (sc *Scope) placedOwner(a store.Applied) string { - if sc == nil || len(sc.Placed) == 0 { + if sc == nil || (len(sc.Placed) == 0 && sc.RemovedByWant == nil) { return "" } m := a.Module if m == "" || m == store.NodeModule { m, _ = declaration.ModuleGuess(a.ID) } - if m == "" || m == sc.Module || !sc.Placed[m] { + if m == "" || m == sc.Module || m == store.NodeModule { return "" } - return m + if sc.Placed[m] { + return m + } + if sc.Module == store.NodeModule && sc.RemovedByWant != nil && !sc.RemovedByWant[m] { + return m // a module the node's own composition carried, which no want removed + } + return "" } // owns reports whether a record belongs to the scope's module. diff --git a/internal/apply/module_scope_test.go b/internal/apply/module_scope_test.go index 14b95f4..51e42a5 100644 --- a/internal/apply/module_scope_test.go +++ b/internal/apply/module_scope_test.go @@ -131,10 +131,16 @@ func TestAResourceThatMovedToItsModulesCompositionIsLeftByTheNodesOwnApply(t *te placed bool // a want still places its module here refused bool // the node's own apply is refused whole stays bool // the resource is still on the machine and recorded + // wants names the modules whose want says removed (Scope.RemovedByWant); nil leaves the guard to Placed alone. + wants []string }{ {name: "moved to its module's own composition", id: "systemd-resolved.fact-suffix", moved: true, placed: true, stays: true}, {name: "a held folded module, in no composition", id: "nats.server-conf", placed: true, refused: true, stays: true}, {name: "its module's want removed", id: "nats.server-conf"}, + // Issue 774, review of the re-land, round 1: a module folded in the node's own has no want, so Placed does not name + // it; in the node's own apply only a module a want removed loses its resources. + {name: "a folded module with no want", id: "nats.server-conf", wants: []string{}, refused: true, stays: true}, + {name: "a folded module a want removed", id: "nats.server-conf", wants: []string{"nats", "old"}}, } { t.Run(tc.name, func(t *testing.T) { dir := t.TempDir() @@ -156,6 +162,12 @@ func TestAResourceThatMovedToItsModulesCompositionIsLeftByTheNodesOwnApply(t *te if tc.placed { scope.Placed = map[string]bool{module: true} } + if tc.wants != nil { + scope.RemovedByWant = map[string]bool{} + for _, m := range tc.wants { + scope.RemovedByWant[m] = true + } + } report, after, err := ApplyModule(context.Background(), archHost(t), &declaration.Declaration{}, scope, state, store.OriginDeclared, noServices, nil, nil, nil, nil) if tc.refused != (err != nil) {