From a7acee3eed371e802455c9ffd37b173c32efb45d Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 29 Oct 2026 10:23:37 +0100 Subject: [PATCH] Refuse an apply that would remove a resource of a module a want still places here, so a composition that lost what runs can never remove the bus server or a held module again (issue 774) --- cmd/mesh-host/nodeagent.go | 15 +++-- internal/agent/agent.go | 14 +++++ internal/apply/apply.go | 46 ++++++++++++++++ internal/apply/module_scope_test.go | 85 ++++++++++++++++++++--------- 4 files changed, 129 insertions(+), 31 deletions(-) diff --git a/cmd/mesh-host/nodeagent.go b/cmd/mesh-host/nodeagent.go index 31b2e59..0d22773 100644 --- a/cmd/mesh-host/nodeagent.go +++ b/cmd/mesh-host/nodeagent.go @@ -526,7 +526,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, sched, say) + res = applyComposition(actx, opts, held, others, ag.Placed(scopeName(c)), sched, say) } done() // The mesh's own certificate it carries is kept for this machine's record (ADR 0555 ยง3). @@ -545,7 +545,7 @@ func nodeAgent(ctx context.Context, opts options, signer ed25519.PublicKey, trus }, Remove: func(ctx context.Context, module string, others []delivery.Composed) agent.Result { actx, done := applyContext(session.WithLink(ctx)) - res := removeComposition(actx, opts, module, others, sched, say) + res := removeComposition(actx, opts, module, others, ag.Placed(module), sched, say) done() afterApply() sayFacts(ctx, ag) @@ -684,7 +684,10 @@ func (c *agentNow) get() *agent.Agent { } // applyComposition applies one composition: a module's, or the node's own. -func applyComposition(ctx context.Context, opts options, c delivery.Composed, others []delivery.Composed, +// +// 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, sched *apply.Scheduler, say link.Announce) agent.Result { name := scopeName(c) if c.Module == "" { @@ -700,7 +703,7 @@ func applyComposition(ctx context.Context, opts options, c delivery.Composed, ot if err := refuseEmptyWrites(d); err != nil { return failedWith(err.Error()) } - scope := apply.Scope{Module: name, Others: resourcesOf(others)} + scope := apply.Scope{Module: name, Others: resourcesOf(others), Placed: placed} got := applyModuleHeld(ctx, opts, d, &scope, sched, say) if got.Refused != "" { return failedWith(got.Refused) @@ -719,9 +722,9 @@ func applyComposition(ctx context.Context, opts options, c delivery.Composed, ot } // removeComposition removes one module's own resources and nothing of another's. -func removeComposition(ctx context.Context, opts options, module string, others []delivery.Composed, +func removeComposition(ctx context.Context, opts options, module string, others []delivery.Composed, placed map[string]bool, sched *apply.Scheduler, say link.Announce) agent.Result { - scope := apply.Scope{Module: module, Others: resourcesOf(others)} + scope := apply.Scope{Module: module, Others: resourcesOf(others), Placed: placed} got := applyModuleHeld(ctx, opts, &declaration.Declaration{Version: declaration.Version}, &scope, sched, say) switch { case got.Refused != "": diff --git a/internal/agent/agent.go b/internal/agent/agent.go index acb91db..8525b4b 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -1042,6 +1042,20 @@ func (a *Agent) Everything() []delivery.Composed { return a.others("\x00") } +// Placed is the modules a want still places on the node (present and not removed), held ones included, but except: +// what no apply but the module's own may remove (apply.Scope.Placed, issue 774). Called without a.mu held. +func (a *Agent) Placed(except string) map[string]bool { + a.mu.Lock() + defer a.mu.Unlock() + out := map[string]bool{} + for m, w := range a.wants { + if m != NodeModule && m != except && 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/apply/apply.go b/internal/apply/apply.go index 1c1023c..24f93c5 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -312,6 +312,27 @@ type Scope struct { Module string // Others are the resources of every other module the node has a composition for. Others []declaration.Resource + // Placed are the modules a want still places on the node (present, not removed), held ones included: no apply but + // 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 +} + +// 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 { + return "" + } + m := a.Module + if m == "" || m == store.NodeModule { + m, _ = declaration.ModuleGuess(a.ID) + } + if m == "" || m == sc.Module || !sc.Placed[m] { + return "" + } + return m } // owns reports whether a record belongs to the scope's module. @@ -441,6 +462,31 @@ func applyScoped( } } + // **A module a want still places here is never removed by another's apply** (issue 774): its resources go only with + // its own apply or its want's removal. One that would is refused whole, before anything is changed, naming them: a + // composition that leaves them out lost what runs, and removing what runs is the worst a node can do with that. + if scope != nil { + var refused []string + for _, orphan := range known.Orphans(declared, origin) { + if !scope.owns(orphan) { + continue + } + if _, kept := heldByOthers[orphan.ID]; kept { + continue + } + if m := scope.placedOwner(orphan); m != "" { + refused = append(refused, orphan.ID+" of "+m) + } + } + if len(refused) > 0 { + sort.Strings(refused) + err := fmt.Errorf("the apply of %s would remove %s, of modules a want still places here; a module's resources "+ + "go only with its own apply or its want's removal, so nothing was applied", scope.Module, strings.Join(refused, ", ")) + log(" refused: " + err.Error()) + return report, known, &Error{Resource: "the resources of modules placed here", Err: err, Done: report} + } + } + removeOrphan := func(orphan store.Applied) error { var action, detail string var err error diff --git a/internal/apply/module_scope_test.go b/internal/apply/module_scope_test.go index 67b494a..14b95f4 100644 --- a/internal/apply/module_scope_test.go +++ b/internal/apply/module_scope_test.go @@ -3,6 +3,7 @@ package apply import ( "context" "path/filepath" + "strings" "testing" "git.novox.be/novox/mesh-sdk/go/declaration" @@ -116,32 +117,66 @@ func TestARecordWithoutAnOwnerBelongsToTheModuleItsIdNames(t *testing.T) { // leaves it as it is, whatever its type, until the module's own apply takes it (issue 1945, live on 2026-10-25 at 02:32: // shanks and g14 lost systemd-resolved's guard, its resolver files and sudo's operator file, ace qbittorrent's server, // because the node's own apply removed every record no longer in it, the others holding the very same resources or not). +// +// And a resource the node's own composition carried for a module a want still places here (a folded module of the old +// form, held) is never removed by the node's own apply when a composition leaves it out: the apply is refused whole and +// says why (issue 774, live on 2026-10-29 at 07:54 UTC on the bus node: the node's own composition, composed without what +// it ran once the kept declaration was read no more, removed the bus server, the proxy and three held modules). Only its +// want's removal lets it go. func TestAResourceThatMovedToItsModulesCompositionIsLeftByTheNodesOwnApply(t *testing.T) { - dir := t.TempDir() - fact := filepath.Join(dir, "suffix") - gone := filepath.Join(dir, "gone.conf") - own := declare(t, `{"id":"systemd-resolved.fact-suffix","type":"file","path":"`+fact+`","content":"mesh\n"}, - {"id":"old.gone","type":"file","path":"`+gone+`","content":"g\n"}`) - _, state, err := ApplyModule(context.Background(), archHost(t), own, Scope{Module: store.NodeModule}, - state0(), store.OriginDeclared, noServices, nil, nil, nil, nil) - if err != nil { - t.Fatal(err) - } - // systemd-resolved now has a composition of its own, not applied yet; old.gone is no module's any more. - moved := declare(t, `{"id":"systemd-resolved.fact-suffix","type":"file","path":"`+fact+`","content":"mesh\n","module":"systemd-resolved"}`) - report, state, err := ApplyModule(context.Background(), archHost(t), &declaration.Declaration{}, - Scope{Module: store.NodeModule, Others: moved.Resources}, state, store.OriginDeclared, noServices, nil, nil, nil, nil) - if err != nil { - t.Fatal(err) - } - if !exists(fact) { - t.Fatalf("the moved resource was removed: %+v", outcomeOf(report, "systemd-resolved.fact-suffix")) - } - if exists(gone) { - t.Fatal("a resource no composition holds any more was kept") - } - if _, still := state.Find("systemd-resolved.fact-suffix"); !still { - t.Fatal("the moved resource's record was dropped, so its module's apply would not know it") + for _, tc := range []struct { + name string + id string // the resource the node's own composition carried, and no longer does + moved bool // its module's own composition now declares it (Others) + 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 + }{ + {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"}, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + fact := filepath.Join(dir, "carried") + gone := filepath.Join(dir, "gone.conf") + own := declare(t, `{"id":"`+tc.id+`","type":"file","path":"`+fact+`","content":"mesh\n"}, + {"id":"old.gone","type":"file","path":"`+gone+`","content":"g\n"}`) + _, state, err := ApplyModule(context.Background(), archHost(t), own, Scope{Module: store.NodeModule}, + state0(), store.OriginDeclared, noServices, nil, nil, nil, nil) + if err != nil { + t.Fatal(err) + } + module, _ := declaration.ModuleGuess(tc.id) + scope := Scope{Module: store.NodeModule} + if tc.moved { + moved := declare(t, `{"id":"`+tc.id+`","type":"file","path":"`+fact+`","content":"mesh\n","module":"`+module+`"}`) + scope.Others = moved.Resources + } + if tc.placed { + scope.Placed = map[string]bool{module: 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) { + t.Fatalf("refused %v, want %v: %v", err != nil, tc.refused, err) + } + if tc.refused { + if !strings.Contains(err.Error(), tc.id+" of "+module) { + t.Fatalf("the refusal does not name what it kept: %v", err) + } + after = state + } + if exists(fact) != tc.stays { + t.Fatalf("the carried resource exists %v, want %v: %+v", exists(fact), tc.stays, outcomeOf(report, tc.id)) + } + if _, still := after.Find(tc.id); still != tc.stays { + t.Fatalf("the carried resource's record kept %v, want %v", still, tc.stays) + } + if exists(gone) == !tc.refused { + t.Fatalf("a resource no composition holds any more exists %v, with the apply refused %v", exists(gone), tc.refused) + } + }) } }