Leave a unit alone when another declared service still holds it (hq issue 190)

Removing one record of a unit gave back what that record found, even while another declared service holds the unit: when the private network's docker.service record goes and the docker module's stays, a record that found the runtime stopped would stop it, and every container with it, only for the docker module to start it again in the same apply. The record is forgotten instead, and the plan says so. A test pins the registry member moving from the network's record to the docker module's in one apply without leaving the list.
2026-10-05 22:21:21 +02:00
parent a566add815
commit c9001abdb4
3 changed files with 166 additions and 0 deletions
@@ -241,9 +241,30 @@ func ApplyMindingWindows(
 	// this apply — a removal included — could change it or its records (novox/hq ADR 0103).
 	before := lookBefore(ctx, sys, d, known, run)

+	// **A unit still declared with a state is not given back when another record of it goes**
+	// (novox/hq issue 190, ADR 0222). Two resources may name one unit — the private network declared
+	// the container runtime's service to reload it, beside the runtime's own module — and what the
+	// one going found is not the machine's to restore while the other still holds the unit: giving
+	// it back would stop or disable a unit the declaration says is running, every container on it
+	// with it, only for the declared one to start it again in the same apply. The record is
+	// forgotten; the unit's found state is the remaining record's to give back.
+	heldUnits := unitsHeld(d.Resources)
+
 	removeOrphan := func(orphan store.Applied) error {
 		var action, detail string
 		var err error
+		if declaration.Type(orphan.Type) == declaration.TypeService {
+			if by, held := heldUnits[unitKey(orphan.Scope, orphan.User, orphan.Target)]; held {
+				known.Forget(orphan.ID)
+				detail := "no longer declared; " + by + " still holds the unit, so it was left as it is"
+				report.Outcomes = append(report.Outcomes, Outcome{
+					ID: orphan.ID, Type: orphan.Type, Target: orphan.Target,
+					Action: "forgotten", Detail: detail,
+				})
+				log(fmt.Sprintf("  forgotten %s (%s): %s", orphan.ID, orphan.Target, detail))
+				return nil
+			}
+		}
 		if declaration.Type(orphan.Type) == declaration.TypeOpening {
 			action, detail, err = removeOpening(ctx, orphan, run, known.Firewall)
 		} else {
@@ -2834,6 +2855,17 @@ func userUnitFiles(account, unit string) []string {
 // unitKey names a unit by the manager it is in and its name (novox/hq ADR 0177): the machine's
 // manager, or one account's. A system unit is the same key whether its record says "system" or
 // nothing, as every record before the scope existed does.
+// unitsHeld is every unit a declared service gives a state, by unitKey, naming the service.
+func unitsHeld(resources []declaration.Resource) map[string]string {
+	held := map[string]string{}
+	for _, r := range resources {
+		if s, ok := r.(*declaration.Service); ok && s.State != "" {
+			held[unitKey(s.Scope, s.User, s.Unit)] = s.ID
+		}
+	}
+	return held
+}
+
 func unitKey(scope, user, unit string) string {
 	if scope == declaration.ScopeUser {
 		return "user:" + user + "/" + unit
@@ -0,0 +1,129 @@
+package apply
+
+import (
+	"context"
+	"fmt"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/novox/mesh-host/internal/declaration"
+	"github.com/novox/mesh-host/internal/store"
+)
+
+// novox/hq issue 190, ADR 0222: the private network stops writing the mesh's registry into the
+// container runtime's file, and the runtime's own module writes the same member. Both are moved in
+// one apply, and the machine never loses the member: the network's record goes first (its member
+// leaves the list), the runtime's module is applied after (the member is added back, now recorded
+// as the runtime's), and the daemon is reloaded once, never stopped, disabled or restarted — even
+// though the record going says the unit was found stopped and disabled.
+
+const theRegistry = "anchor.internal:5100"
+
+func runtimeDecl(t *testing.T, path string, network bool) *declaration.Declaration {
+	t.Helper()
+	var resources []string
+	if network {
+		resources = append(resources,
+			fmt.Sprintf(`{"id":"networking.registry-trust","type":"file","path":%q,"into":"json","content":%q}`,
+				path, `{"insecure-registries":["`+theRegistry+`"]}`),
+			`{"id":"networking.registry-trust-reload","type":"service","unit":"docker.service","state":"running",
+			  "reload-on":["networking.registry-trust"]}`)
+	}
+	resources = append(resources,
+		fmt.Sprintf(`{"id":"docker.daemon","type":"file","path":%q,"into":"json","content":%q}`,
+			path, `{"live-restore":true,"insecure-registries":["`+theRegistry+`"]}`),
+		`{"id":"docker.runtime","type":"service","unit":"docker.service","state":"running","boot":"enabled",
+		  "reload-on":["docker.daemon"]}`)
+	return parse(t, `{"declaration":1,"resources":[`+strings.Join(resources, ",")+`]}`)
+}
+
+func TestTheRegistryMovesToTheRuntimesModuleWithoutLeavingTheList(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "daemon.json")
+	if err := os.WriteFile(path, []byte(`{"insecure-registries":["192.0.2.7:5000"]}`), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	var commands []string
+	run := recordingServices(&commands)
+
+	// Both declare it: the network's record added the member, so the runtime's finds it there.
+	_, state, err := Apply(context.Background(), archHost(t), runtimeDecl(t, path, true), store.State{},
+		store.OriginDeclared, run, nil, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	// Say the network's record found the runtime stopped and disabled: giving that back would stop
+	// every container on the machine.
+	for i := range state.Resources {
+		if state.Resources[i].ID == "networking.registry-trust-reload" {
+			state.Resources[i].Found = &store.FoundUnit{Unit: "docker.service", State: "stopped", Boot: "disabled"}
+		}
+	}
+
+	commands = nil
+	report, state, err := Apply(context.Background(), archHost(t), runtimeDecl(t, path, false), state,
+		store.OriginDeclared, run, nil, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got := fmt.Sprint(readObject(t, path)["insecure-registries"]); got != "[192.0.2.7:5000 "+theRegistry+"]" {
+		t.Fatalf("the list after the move: %s", got)
+	}
+	rec, _ := state.Find("docker.daemon")
+	if added := rec.Into.Added["insecure-registries"]; len(added) != 1 || canonical(added[0]) != `"`+theRegistry+`"` {
+		t.Errorf("the member is not recorded as the runtime's module's: %s", added)
+	}
+	reloads := 0
+	for _, c := range commands {
+		if strings.Contains(c, "docker.service") && (strings.Contains(c, " stop ") || strings.Contains(c, " restart ") ||
+			strings.Contains(c, " disable ")) {
+			t.Errorf("the runtime was given back as the network's record found it: %q", c)
+		}
+		if strings.Contains(c, "reload docker.service") {
+			reloads++
+		}
+	}
+	if reloads != 1 {
+		t.Errorf("the runtime was reloaded %d times; commands were %v", reloads, commands)
+	}
+	for _, o := range report.Outcomes {
+		if o.ID == "networking.registry-trust-reload" && o.Action != "forgotten" {
+			t.Errorf("the network's record of the unit was %q: %s", o.Action, o.Detail)
+		}
+	}
+
+	// Steady from here on, and undeclaring the runtime's module takes only what it added.
+	report, state, err = Apply(context.Background(), archHost(t), runtimeDecl(t, path, false), state,
+		store.OriginDeclared, run, nil, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, o := range report.Outcomes {
+		if o.ID == "docker.daemon" && o.Action != "unchanged" {
+			t.Errorf("a second apply was %q", o.Action)
+		}
+	}
+	if _, _, err := Apply(context.Background(), archHost(t), somethingElse(t), state, store.OriginDeclared, run, nil, nil); err != nil {
+		t.Fatal(err)
+	}
+	if got := fmt.Sprint(readObject(t, path)["insecure-registries"]); got != "[192.0.2.7:5000]" {
+		t.Errorf("undeclaring the runtime's module left %s", got)
+	}
+}
+
+// And the preview says the same before it happens.
+func TestThePlanForgetsARecordOfAUnitStillHeld(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "daemon.json")
+	var commands []string
+	_, state, err := Apply(context.Background(), archHost(t), runtimeDecl(t, path, true), store.State{},
+		store.OriginDeclared, recordingServices(&commands), nil, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, step := range Plan(runtimeDecl(t, path, false), state, store.OriginDeclared) {
+		if step.ID == "networking.registry-trust-reload" && step.Verb != "forget" {
+			t.Errorf("the plan would %s the network's record of a unit docker.runtime holds: %s", step.Verb, step.Why)
+		}
+	}
+}
@@ -84,10 +84,15 @@ func Plan(d *declaration.Declaration, known store.State, origin string) []Step {

 	var protecting, orphans []Step
 	made := meshMadeUnits(known)
+	heldUnits := unitsHeld(d.Resources)
 	for _, orphan := range known.Orphans(declared, origin) {
 		step := Step{Verb: "remove", Type: orphan.Type, ID: orphan.ID, Target: orphan.Target,
 			Why: "recorded here and no longer declared"}
+		held := heldUnits[unitKey(orphan.Scope, orphan.User, orphan.Target)]
 		switch {
+		case orphan.Type == string(declaration.TypeService) && held != "":
+			// In removeOrphan's words (novox/hq issue 190).
+			step.Verb, step.Why = "forget", "no longer declared; "+held+" still holds the unit, so it is left as it is"
 		case orphan.Stateless:
 			step.Verb, step.Why = "forget", "no longer declared; its unit's state was never the mesh's and is left as it is"
 		case orphan.Type == string(declaration.TypeArchive) && store.IsFormer(orphan.ID):