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)

2026-10-29 21:04:17 +01:00
parent d2645a27e9
commit a7acee3eed
4 changed files with 129 additions and 31 deletions
@@ -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 {
@@ -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
@@ -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)
+			}
+		})
 	}
 }