diff --git a/internal/agent/agent.go b/internal/agent/agent.go index 09568b6..f114d28 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -409,6 +409,9 @@ func (a *Agent) heldOut(except string) []delivery.Composed { if h, ok := a.running[m]; ok && h.Phase != delivery.PhaseRemoved { continue } + if w := a.wants[m]; w.Present && w.Want.Removed { + continue // a module the assigner removed holds nothing back + } if w := a.wants[m]; w.Present && !w.Want.Removed && w.Want.FP == fp { waiting[m] = c continue diff --git a/internal/agent/agent_test.go b/internal/agent/agent_test.go index f5f0596..e01214a 100644 --- a/internal/agent/agent_test.go +++ b/internal/agent/agent_test.go @@ -676,3 +676,45 @@ func TestTheNodesOwnApplyHoldsOutAModuleThatMovedToItsOwnComposition(t *testing. t.Fatalf("the others of the node's own apply were %v: systemd-resolved's resources would be removed", names) } } + +// A module the assigner removed holds nothing back, its compositions still in the bucket or not: its resources are its +// removal's to take (issue 1945). +func TestARemovedModulesCompositionHoldsNothingBack(t *testing.T) { + b := newBed(t) + fp := fpOf("b") + rk, rraw, _ := composition(t, "gone", fp, false, file("gone", "x", "1")) + b.comps.before(rk, rraw) + gk, err := delivery.WantKey(node, "gone") + if err != nil { + t.Fatal(err) + } + graw, err := delivery.EncodeWant(delivery.Want{Removed: true}) + if err != nil { + t.Fatal(err) + } + b.wants.before(gk, graw) + nk, nraw, ndig := composition(t, NodeModule, "", false) + nwk, _ := delivery.WantNodeKey(node) + nwraw, _ := delivery.EncodeNodeWant(delivery.NodeWant{Composed: ndig}) + b.comps.before(nk, nraw) + b.wants.before(nwk, nwraw) + stop := b.run() + defer stop() + eventually(t, "the node's own not applied", func() bool { + for _, m := range b.appliedList() { + if m == NodeModule { + return true + } + } + return false + }) + b.mu.Lock() + defer b.mu.Unlock() + for _, others := range b.others { + for _, c := range others { + if c.Module == "gone" { + t.Fatal("a removed module's composition was held out") + } + } + } +} diff --git a/internal/apply/apply.go b/internal/apply/apply.go index 03c57b6..731d432 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -430,9 +430,32 @@ func applyScoped( // And what a record going hands to a resource that still asks for its group (novox/hq ADR 0252 ยง3). handed := Handed{} + // **A resource another module's composition now holds is that module's, and is left** (issue 1945): a module that + // leaves the node's own composition for one of its own takes its resources with it, by their identity, and the node's + // own apply leaves each as it is, record and all, until that module's apply takes it. Removed, a node lost its + // resolver guard, its sudoers file and a running container between two applies. + heldByOthers := map[string]string{} + if scope != nil { + for _, r := range scope.Others { + heldByOthers[r.Identity()] = r.DeclaredModule() + } + } + removeOrphan := func(orphan store.Applied) error { var action, detail string var err error + if module, held := heldByOthers[orphan.ID]; held && !declared[orphan.ID] { + if module == "" { + module = "another module" + } + detail := "no longer declared here; the composition of " + module + " holds it, so it was left as it is" + report.Outcomes = append(report.Outcomes, Outcome{ + ID: orphan.ID, Type: orphan.Type, Target: orphan.Target, + Action: "kept", Detail: detail, + }) + log(fmt.Sprintf(" kept %s (%s): %s", orphan.ID, orphan.Target, detail)) + return nil + } if by, held := heldFiles[fileKey(orphan)]; held && by != orphan.ID { if scope != nil && !declared[by] { // Another module declares the path: it is that module's, so this record is let go and the file is diff --git a/internal/apply/module_scope_test.go b/internal/apply/module_scope_test.go index 2ad9d20..67b494a 100644 --- a/internal/apply/module_scope_test.go +++ b/internal/apply/module_scope_test.go @@ -111,3 +111,38 @@ func TestARecordWithoutAnOwnerBelongsToTheModuleItsIdNames(t *testing.T) { t.Fatalf("a's file exists %v, b's file exists %v", exists(fa), exists(fb)) } } + +// A resource that leaves the node's own composition for its module's own is that module's now: the node's own apply +// 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). +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") + } +} + +func state0() store.State { return store.State{} }