Merge pull request 'Never remove a module's resources while its own composition waits to be applied (issue 1944)' (#136) from fix/r6-node-own-keeps-moved into main

Reviewed-on: #136

internal/agent/agent.go

@@ -16,6 +16,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"sort"
+	"strings"
 	"sync"
 
 	"git.novox.be/novox/mesh-sdk/go/delivery"
@@ -189,6 +190,8 @@ func (a *Agent) Run(ctx context.Context) error {
 				compsCaught = true
 			} else if m := a.takeComposition(u); m != "" {
 				dirty[m] = true
+				// The node's own may wait for this one to read (unreadableModules): looked at again with it.
+				dirty[NodeModule] = true
 			}
 		}
 		// Nothing is applied until what was there at the start is all in: a module's composition may arrive after its
@@ -389,6 +392,90 @@ func (a *Agent) others(except string) []delivery.Composed {
 	return out
 }
 
+// unreadableModules are the keys of the module compositions in the node's bucket that cannot be read or are refused, of a
+// module the assigner has not removed: for each module only the composition its want names, else its newest, since a
+// stale one nobody applies (written before a rule the SDK now holds) must not hold the node's own back. Called holding
+// a.mu.
+func (a *Agent) unreadableModules() []string {
+	type pick struct {
+		key string
+		c   compNow
+	}
+	current := map[string]pick{}
+	for key, c := range a.comps {
+		_, m, fp, isNode, err := delivery.ParseComposedKey(key)
+		if err != nil || isNode {
+			continue
+		}
+		w := a.wants[m]
+		if w.Present && w.Want.Removed {
+			continue
+		}
+		if w.Present && w.Want.FP != "" {
+			if w.Want.FP == fp {
+				current[m] = pick{key, c}
+			}
+			continue
+		}
+		if prev, ok := current[m]; !ok || c.Rev > prev.c.Rev {
+			current[m] = pick{key, c}
+		}
+	}
+	var out []string
+	for _, p := range current {
+		if p.c.Err != nil {
+			out = append(out, p.key)
+		}
+	}
+	sort.Strings(out)
+	return out
+}
+
+// heldOut is what an apply or a removal holds out: others, and every module whose own composition the node's bucket holds
+// and the agent does not run yet. A module that left the node's own composition for one of its own keeps its resources
+// until that one is applied, so the node's own apply never removes them in between (issue 1945, live on 2026-10-25:
+// shanks lost systemd-resolved's guard while its composition waited for a credential). The composition its want names,
+// else its newest. Not part of Everything: what has not been applied is not what runs.
+func (a *Agent) heldOut(except string) []delivery.Composed {
+	out := a.others(except)
+	held := map[string]bool{}
+	for _, c := range out {
+		held[c.Module] = true
+	}
+	waiting := map[string]compNow{}
+	for key, c := range a.comps {
+		_, m, fp, isNode, err := delivery.ParseComposedKey(key)
+		if err != nil || isNode || c.Err != nil || m == except || held[m] {
+			continue
+		}
+		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
+		}
+		if w := a.wants[m]; w.Present && !w.Want.Removed && waiting[m].Rev != 0 && waiting[m].C.FP == w.Want.FP {
+			continue
+		}
+		if prev, ok := waiting[m]; !ok || c.Rev > prev.Rev {
+			waiting[m] = c
+		}
+	}
+	names := make([]string, 0, len(waiting))
+	for m := range waiting {
+		names = append(names, m)
+	}
+	sort.Strings(names)
+	for _, m := range names {
+		out = append(out, waiting[m].C)
+	}
+	return out
+}
+
 // Everything is what the node runs: the compositions the agent runs, and the kept declaration's resources for each module
 // the agent does not hold yet. What the node judges its health by, and what the filling, the take act and the judgement
 // of directories read, is built from it.
@@ -457,7 +544,17 @@ func (a *Agent) settleOne(ctx context.Context, module string) {
 		a.mu.Unlock()
 		return // held for a failed backup, and no answer since
 	}
-	others := a.others(module)
+	if module == NodeModule {
+		// Not while a module's composition cannot be read or is refused: what it carries may be what left the node's own,
+		// and the node's own apply would remove it, held out by nothing (issue 1945, the review's gap 2).
+		if bad := a.unreadableModules(); len(bad) > 0 {
+			a.sayOnce("node-waits:"+strings.Join(bad, ","), fmt.Sprintf("the node's own composition is not applied while "+
+				"%s cannot be read or is refused: what it carries would be removed", strings.Join(bad, ", ")))
+			a.mu.Unlock()
+			return
+		}
+	}
+	others := a.heldOut(module)
 	a.mu.Unlock()
 	a.apply(ctx, module, w, comp, others)
 }
@@ -559,7 +656,7 @@ func (a *Agent) remove(ctx context.Context, module string, w wantNow) {
 		a.mu.Unlock()
 		return
 	}
-	others := a.others(module)
+	others := a.heldOut(module)
 	a.mu.Unlock()
 	res := a.env.Remove(ctx, module, others)
 	a.mu.Lock()

internal/agent/agent_test.go

@@ -111,6 +111,7 @@ type bed struct {
 	backup   func(module string) Backup
 	kept     map[string][]json.RawMessage
 	others   [][]delivery.Composed
+	said     []string
 	env      Env
 }
 
@@ -148,7 +149,12 @@ func newBed(t *testing.T) *bed {
 		},
 		Kept:    func() map[string][]json.RawMessage { return b.kept },
 		Trusted: func() []ed25519.PublicKey { return []ed25519.PublicKey{controllerPub, operatorPub} },
-		Say:     func(s string) { t.Log(s) },
+		Say: func(s string) {
+			t.Log(s)
+			b.mu.Lock()
+			b.said = append(b.said, s)
+			b.mu.Unlock()
+		},
 	}
 	return b
 }
@@ -640,3 +646,139 @@ func TestAHaveWithNoCompositionDefersToTheKeptDeclaration(t *testing.T) {
 		t.Fatalf("applied %v", got)
 	}
 }
+
+// A module that leaves the node's own composition for a composition of its own keeps its resources until that one is
+// applied: the node's own apply holds out every module whose composition the node's bucket holds and the agent does not
+// run yet (issue 1945, live on 2026-10-25 at 02:32: shanks' node-engine applied a node's own composition that no longer
+// carried systemd-resolved, whose own composition waited for its credential, and removed its guard, so the machine lost
+// its resolver).
+func TestTheNodesOwnApplyHoldsOutAModuleThatMovedToItsOwnComposition(t *testing.T) {
+	b := newBed(t)
+	fp := fpOf("b")
+	// systemd-resolved's own composition is written; no want names it yet.
+	rk, rraw, _ := composition(t, "systemd-resolved", fp, false, file("systemd-resolved", "guard", "g"))
+	b.comps.before(rk, rraw)
+	nk, nraw, ndig := composition(t, NodeModule, "", false)
+	nwk, err := delivery.WantNodeKey(node)
+	if err != nil {
+		t.Fatal(err)
+	}
+	nwraw, err := delivery.EncodeNodeWant(delivery.NodeWant{Composed: ndig})
+	if err != nil {
+		t.Fatal(err)
+	}
+	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 { return len(b.appliedList()) == 1 })
+	b.mu.Lock()
+	defer b.mu.Unlock()
+	var names []string
+	for _, c := range b.others[0] {
+		names = append(names, c.Module)
+	}
+	if !strings.Contains(strings.Join(names, ","), "systemd-resolved") {
+		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")
+			}
+		}
+	}
+}
+
+// The node's own composition is not applied while a module's composition in the node's bucket cannot be read or is not
+// signed by a trusted key: what that one carries may be what left the node's own, and the node's own apply would remove
+// it (issue 1945, the review's gap 2). It is said, naming the composition, and applied once that one reads.
+func TestTheNodesOwnWaitsWhileAModulesCompositionCannotBeRead(t *testing.T) {
+	b := newBed(t)
+	bad, err := delivery.ComposedKey(node, "broken", fpOf("c"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	b.comps.before(bad, []byte(`{"v":1,"not":"a composition"}`))
+	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 wait not said", func() bool {
+		b.mu.Lock()
+		defer b.mu.Unlock()
+		for _, s := range b.said {
+			if strings.Contains(s, bad) {
+				return true
+			}
+		}
+		return false
+	})
+	if got := b.appliedList(); len(got) != 0 {
+		t.Fatalf("applied %v while a composition could not be read", got)
+	}
+	// Read again, the node's own is applied.
+	gk, graw, _ := composition(t, "broken", fpOf("c"), false, file("broken", "x", "1"))
+	b.comps.put(gk, graw)
+	eventually(t, "the node's own not applied", func() bool { return len(b.appliedList()) == 1 })
+}
+
+// A stale refused composition beside a newer readable one of the same module does not hold the node's own apply back:
+// only the composition the module's want names, else its newest, counts (the review of issue 1945: the bucket keeps
+// compositions written before the filled paths, which the SDK now refuses, and nobody applies them).
+func TestAStaleRefusedCompositionDoesNotHoldTheNodesOwnBack(t *testing.T) {
+	b := newBed(t)
+	stale, err := delivery.ComposedKey(node, "power", fpOf("a"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	b.comps.before(stale, []byte(`{"v":1,"not":"a composition"}`))
+	gk, graw, _ := composition(t, "power", fpOf("b"), false, file("power", "x", "1"))
+	b.comps.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 { return len(b.appliedList()) == 1 })
+}

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

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{} }