inventory: forgetting a module says what goes with it, and refuses until told

`module forget` cascaded. The settings, the module's own secrets and the ports the mesh
chose all name the module by a foreign key that cascades, so removing the row took all
three and reported "forgotten" — an action succeeding into a state its own verify would
reject (novox/hq 04-ISSUES/017). A sealed secret is not recoverable afterwards, because
the mesh discarded the plaintext when it made it.

It now reads what it would destroy, names each thing one at a time, and refuses.
`--and-what-it-holds` is how somebody says they mean it, and the removal then reports
what went — this being the only record that any of it ever existed.

Reported as "operator settings do not persist, because re-registering a module
cascade-deletes them". Half of that is wrong, and the test now says so out loud: the
upsert is on the name, so `module add` at a new version leaves the settings, the secrets
and the ports exactly where they were. The command that destroyed them was `forget`, and
a wrong belief about which command destroys data is expensive in both directions — it
sends people looking for a fault that is not there, and leaves the real one unexamined.

Checked by internal/inventory/forget_test.go, which writes all three, re-registers the
module at a new version, reads them back, and only then tries to forget it.

Claude-Session: https://claude.ai/code/session_01LrgweAeERJYBg88c5cKDzF

cmd/mesh-control/modules.go

@@ -188,13 +188,37 @@ func moduleCommand(ctx context.Context, args []string) error {
 		return nil
 
 	case "forget":
-		if len(args) != 2 {
+		// **What goes with it is said before it goes** (novox/hq 04-ISSUES/017). The settings, the
-			return errors.New("module forget <name>")
+		// module's own secrets and the ports the mesh chose all cascade off the module row, so
-		}
+		// `forget` used to destroy them and report "forgotten" — an action succeeding into a state
-		if err := inv.ForgetModule(ctx, args[1]); err != nil {
+		// its own verify would reject, and a sealed secret is not recoverable afterwards.
+		set := flag.NewFlagSet("module forget", flag.ContinueOnError)
+		andHeld := set.Bool("and-what-it-holds", false,
+			"discard its settings, its own secrets and its ports along with it")
+		positionals, err := parseAround(set, args[1:])
+		if err != nil {
 			return err
 		}
-		fmt.Printf("%s forgotten\n", args[1])
+		if len(positionals) != 1 {
+			return errors.New("module forget <name> [--and-what-it-holds]")
+		}
+		if !*andHeld {
+			if err := inv.ForgetModule(ctx, positionals[0]); err != nil {
+				return err
+			}
+			fmt.Printf("%s forgotten\n", positionals[0])
+			return nil
+		}
+		held, err := inv.DiscardModule(ctx, positionals[0])
+		if err != nil {
+			return err
+		}
+		fmt.Printf("%s forgotten\n", positionals[0])
+		for _, line := range held.Lines() {
+			// Said after the fact as well as before it: this is the only record that these
+			// existed, and the next person to ask why the module came back empty reads it here.
+			fmt.Printf("  discarded%s\n", strings.TrimPrefix(line, " "))
+		}
 		return nil
 
 	case "issue":

internal/inventory/catalogue.go

@@ -204,10 +204,165 @@ func (i *Inventory) Provided(ctx context.Context, name string) (bool, error) {
 	return source != nil && *source == "the control plane", nil
 }
 
-// ForgetModule removes a module, unless a machine is running it, and never one the control plane
+// ErrStillHolds is why a module cannot be forgotten without saying so first.
-// provides.
+//
+// Its own error because it is not a fault either: the operator settings, the module's own secrets
+// and the ports the mesh chose for it are all keyed on the module by name and all cascade when the
+// row goes. Removing the module removes them, silently, and none of them can be recovered — a
+// sealed secret least of all, because the mesh discarded the plaintext when it made it.
+var ErrStillHolds = errors.New("the mesh still holds things for that module")
+
+// Holdings is everything keyed on a module that would go with it.
+//
+// **Named one at a time rather than counted.** "3 settings" tells somebody there is something to
+// lose and not whether they can afford to lose it; "the mesh-wide layer, and anchor's" tells them
+// what to write down before they type the command again.
+type Holdings struct {
+	// Mesh is true when a mesh-wide settings layer exists for the module.
+	Mesh bool
+	// Nodes are the machines with a settings layer of their own for it, sorted.
+	Nodes []string
+	// Secrets are the module's own secrets, as "<name> on <node>", sorted. These are the ones the
+	// mesh cannot make again: what is stored is sealed to a machine and the plaintext is gone.
+	Secrets []string
+	// Ports are the ports the mesh chose for it, as "<wanted> on <node>", sorted. Made once and
+	// kept (novox/hq ADR 0038) — removing the module gives that promise up.
+	Ports []string
+}
+
+// Any reports whether removing the module would discard anything.
+func (h Holdings) Any() bool {
+	return h.Mesh || len(h.Nodes) > 0 || len(h.Secrets) > 0 || len(h.Ports) > 0
+}
+
+// Lines is what would be lost, one thing per line, for a person about to decide.
+func (h Holdings) Lines() []string {
+	var out []string
+	if h.Mesh {
+		out = append(out, "  settings, for the whole mesh")
+	}
+	for _, n := range h.Nodes {
+		out = append(out, "  settings, on "+n)
+	}
+	for _, s := range h.Secrets {
+		out = append(out, "  its own secret "+s+" — sealed, so the mesh cannot make it again")
+	}
+	for _, p := range h.Ports {
+		out = append(out, "  the port "+p)
+	}
+	return out
+}
+
+// HeldFor is everything the mesh keeps that is keyed on one module.
+//
+// Read rather than counted at the moment of removal, because the answer is the whole of what a
+// person needs in order to say yes.
+func (i *Inventory) HeldFor(ctx context.Context, name string) (Holdings, error) {
+	var held Holdings
+	rows, err := i.store.Pool().Query(ctx,
+		`select coalesce(n.name, '') from settings s left join node n on n.id = s.node
+		  where s.module = $1 order by n.name nulls first`, name)
+	if err != nil {
+		return Holdings{}, err
+	}
+	for rows.Next() {
+		var node string
+		if err := rows.Scan(&node); err != nil {
+			rows.Close()
+			return Holdings{}, err
+		}
+		if node == "" {
+			held.Mesh = true
+			continue
+		}
+		held.Nodes = append(held.Nodes, node)
+	}
+	rows.Close()
+	if err := rows.Err(); err != nil {
+		return Holdings{}, err
+	}
+
+	for _, read := range []struct {
+		query string
+		into  *[]string
+	}{
+		{`select s.name || ' on ' || n.name from module_secret s join node n on n.id = s.node
+		   where s.module = $1 order by n.name, s.name`, &held.Secrets},
+		{`select p.wanted::text || ' on ' || n.name from port_assignment p
+		   join node n on n.id = p.node where p.module = $1 order by n.name, p.wanted`, &held.Ports},
+	} {
+		rows, err := i.store.Pool().Query(ctx, read.query, name)
+		if err != nil {
+			return Holdings{}, err
+		}
+		for rows.Next() {
+			var one string
+			if err := rows.Scan(&one); err != nil {
+				rows.Close()
+				return Holdings{}, err
+			}
+			*read.into = append(*read.into, one)
+		}
+		rows.Close()
+		if err := rows.Err(); err != nil {
+			return Holdings{}, err
+		}
+	}
+	return held, nil
+}
+
+// ForgetModule removes a module, unless a machine is running it, unless the mesh still holds
+// things for it, and never one the control plane provides.
+//
+// **Refused rather than cascaded.** The settings, own-secrets and port assignments all name the
+// module by a foreign key that cascades, so the row going takes them with it and says nothing.
+// That is an action succeeding into a state its own verify would reject (novox/hq 04-ISSUES/017):
+// the command reports "forgotten", the operator re-registers the module a moment later, and what
+// comes back is a module with none of its configuration and none of its secrets — with nothing
+// anywhere naming the moment they were lost.
 func (i *Inventory) ForgetModule(ctx context.Context, name string) error {
+	if err := i.mayForget(ctx, name); err != nil {
+		return err
+	}
+	held, err := i.HeldFor(ctx, name)
+	if err != nil {
+		return err
+	}
+	if held.Any() {
+		return fmt.Errorf("%w:\n%s\n\nAll of it goes when the module does. Run "+
+			"`module forget %s --and-what-it-holds` if that is what you mean",
+			ErrStillHolds, strings.Join(held.Lines(), "\n"), name)
+	}
+	return i.discard(ctx, name)
+}
+
+// DiscardModule removes a module and everything the mesh holds for it, having been told to.
+//
+// The same checks as ForgetModule except the one about what is held: a machine running it still
+// refuses, and a module the control plane provides still refuses, because neither of those is
+// something an operator can consent to on the module's behalf.
+func (i *Inventory) DiscardModule(ctx context.Context, name string) (Holdings, error) {
+	if err := i.mayForget(ctx, name); err != nil {
+		return Holdings{}, err
+	}
+	held, err := i.HeldFor(ctx, name)
+	if err != nil {
+		return Holdings{}, err
+	}
+	if err := i.discard(ctx, name); err != nil {
+		return Holdings{}, err
+	}
+	// Returned so the caller can say what went, rather than "forgotten". A person who has just
+	// destroyed a sealed secret should be able to read which one from the output.
+	return held, nil
+}
+
+// mayForget is the part of forgetting that is not about what is held.
+func (i *Inventory) mayForget(ctx context.Context, name string) error {
 	provided, err := i.Provided(ctx, name)
+	if errors.Is(err, pgx.ErrNoRows) {
+		return fmt.Errorf("%w: %s", ErrNoSuchModule, name)
+	}
 	if err != nil {
 		return err
 	}
@@ -235,10 +390,17 @@ func (i *Inventory) ForgetModule(ctx context.Context, name string) error {
 		on = append(on, node)
 	}
 	rows.Close()
+	if err := rows.Err(); err != nil {
+		return err
+	}
 	if len(on) > 0 {
 		return fmt.Errorf("%w: %s. Unassign it first", ErrStillAssigned, strings.Join(on, ", "))
 	}
+	return nil
+}
 
+// discard is the removal itself, once it has been decided.
+func (i *Inventory) discard(ctx context.Context, name string) error {
 	tag, err := i.store.Pool().Exec(ctx, `delete from module where name = $1`, name)
 	if err != nil {
 		return err

internal/inventory/forget_test.go

@@ -0,0 +1,226 @@
+package inventory
+
+import (
+	"errors"
+	"strings"
+	"testing"
+
+	"github.com/novox/mesh-control/internal/catalogue"
+)
+
+// What removing a module takes with it, and what re-registering one does not.
+//
+// Both were reported as one fault — "operator settings do not persist, because re-registering a
+// module cascade-deletes them". Only half of it was true, and it was the other half.
+
+// **Registering a module again does not touch what the mesh holds for it.** The upsert is on the
+// name, so a manifest changing — a new version, a new requirement, a new resource — leaves the
+// settings, the secrets and the ports exactly where they were.
+//
+// This is here because it was believed not to be. A wrong belief about which command destroys data
+// is expensive in both directions: it sends people looking for a fault that is not there, and it
+// leaves the command that really does destroy it unexamined.
+func TestRegisteringAModuleAgainKeepsWhatTheMeshHoldsForIt(t *testing.T) {
+	inv := fresh(t)
+	ctx := t.Context()
+	node, err := inv.AddNode(ctx, "anchor")
+	if err != nil {
+		t.Fatal(err)
+	}
+	key, _ := aSealingKey(t)
+	if err := inv.RecordSealingKey(ctx, node.ID, key); err != nil {
+		t.Fatal(err)
+	}
+	m := catalogue.Manifest{Module: "step-ca", Version: "1",
+		Provides: catalogue.Offers("acme-ca")}
+	if err := inv.RegisterModule(ctx, m, Source{}); err != nil {
+		t.Fatal(err)
+	}
+	if err := inv.SetSettings(ctx, "", "step-ca", map[string]any{"issuer": "the mesh"}); err != nil {
+		t.Fatal(err)
+	}
+	if err := inv.SetSettings(ctx, "anchor", "step-ca", map[string]any{"port": 9000}); err != nil {
+		t.Fatal(err)
+	}
+	if err := inv.AcceptSecretForModule(ctx, "anchor", "step-ca", "password", "sealed"); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := inv.PortFor(ctx, "anchor", "step-ca", 9000, false); err != nil {
+		t.Fatal(err)
+	}
+
+	// Re-registered at a new version, which is exactly what somebody bumping one does.
+	m.Version = "2"
+	if err := inv.RegisterModule(ctx, m, Source{}); err != nil {
+		t.Fatal(err)
+	}
+
+	layers, err := inv.SettingsFor(ctx, "anchor", "step-ca")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(layers) != 2 {
+		t.Fatalf("re-registering lost a settings layer: %+v", layers)
+	}
+	held, err := inv.HeldFor(ctx, "step-ca")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !held.Mesh || len(held.Nodes) != 1 || len(held.Secrets) != 1 || len(held.Ports) != 1 {
+		t.Fatalf("re-registering lost something the mesh held: %+v", held)
+	}
+
+	// And the new manifest is what resolution sees, which is the point of re-registering at all.
+	shelf, err := inv.Catalogue(ctx)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if shelf["step-ca"].Version != "2" {
+		t.Fatalf("the new manifest did not replace the old: %+v", shelf["step-ca"])
+	}
+	if len(shelf["step-ca"].Provides) != 1 || shelf["step-ca"].Provides[0].Name != "acme-ca" {
+		// A version bump was reported as un-providing a provision. It does not.
+		t.Fatalf("a version bump changed what the module provides: %+v", shelf["step-ca"].Provides)
+	}
+}
+
+// **Forgetting a module refuses while the mesh still holds things for it, and says what they are.**
+//
+// The settings, the module's own secrets and the ports the mesh chose all name the module by a
+// foreign key that cascades. So the removal took them, silently, and reported "forgotten" — an
+// action succeeding into a state its own verify would reject (novox/hq 04-ISSUES/017). A sealed
+// secret is not recoverable afterwards: the mesh discarded the plaintext when it made it.
+func TestForgettingAModuleRefusesRatherThanDiscardingWhatTheMeshHolds(t *testing.T) {
+	inv := fresh(t)
+	ctx := t.Context()
+	node, err := inv.AddNode(ctx, "anchor")
+	if err != nil {
+		t.Fatal(err)
+	}
+	key, _ := aSealingKey(t)
+	if err := inv.RecordSealingKey(ctx, node.ID, key); err != nil {
+		t.Fatal(err)
+	}
+	if err := inv.RegisterModule(ctx, manifest("step-ca", nil, nil), Source{}); err != nil {
+		t.Fatal(err)
+	}
+	if err := inv.SetSettings(ctx, "anchor", "step-ca", map[string]any{"port": 9000}); err != nil {
+		t.Fatal(err)
+	}
+	if err := inv.AcceptSecretForModule(ctx, "anchor", "step-ca", "password", "sealed"); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := inv.PortFor(ctx, "anchor", "step-ca", 9000, false); err != nil {
+		t.Fatal(err)
+	}
+
+	err = inv.ForgetModule(ctx, "step-ca")
+	if !errors.Is(err, ErrStillHolds) {
+		t.Fatalf("forgetting discarded what the mesh held, or refused for another reason: %v", err)
+	}
+	// It names them one at a time. "3 things" says there is something to lose and not whether it
+	// can be afforded; the sealed secret is the one that cannot be made again, and it is named.
+	for _, want := range []string{"settings, on anchor", "password on anchor",
+		"the mesh cannot make it again", "the port 9000 on anchor", "--and-what-it-holds"} {
+		if !strings.Contains(err.Error(), want) {
+			t.Errorf("the refusal does not say %q:\n%s", want, err)
+		}
+	}
+	// And nothing went. A refusal that half-happened would be worse than the cascade.
+	shelf, err := inv.Catalogue(ctx)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, still := shelf["step-ca"]; !still {
+		t.Fatal("the module was removed by a command that refused")
+	}
+	layers, err := inv.SettingsFor(ctx, "anchor", "step-ca")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(layers) != 1 {
+		t.Fatalf("a refusal took the settings anyway: %+v", layers)
+	}
+}
+
+// Having been told, it goes — and what went is reported, because this is the only record that any
+// of it existed.
+func TestDiscardingAModuleSaysWhatWentWithIt(t *testing.T) {
+	inv := fresh(t)
+	ctx := t.Context()
+	node, err := inv.AddNode(ctx, "anchor")
+	if err != nil {
+		t.Fatal(err)
+	}
+	key, _ := aSealingKey(t)
+	if err := inv.RecordSealingKey(ctx, node.ID, key); err != nil {
+		t.Fatal(err)
+	}
+	if err := inv.RegisterModule(ctx, manifest("step-ca", nil, nil), Source{}); err != nil {
+		t.Fatal(err)
+	}
+	if err := inv.SetSettings(ctx, "", "step-ca", map[string]any{"issuer": "the mesh"}); err != nil {
+		t.Fatal(err)
+	}
+	if err := inv.AcceptSecretForModule(ctx, "anchor", "step-ca", "password", "sealed"); err != nil {
+		t.Fatal(err)
+	}
+
+	held, err := inv.DiscardModule(ctx, "step-ca")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !held.Mesh || len(held.Secrets) != 1 {
+		t.Fatalf("what went was not reported: %+v", held)
+	}
+	shelf, err := inv.Catalogue(ctx)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, still := shelf["step-ca"]; still {
+		t.Fatal("the module is still there")
+	}
+}
+
+// A module the mesh holds nothing for is forgotten without ceremony. The refusal is about loss,
+// not about the command.
+func TestForgettingAModuleTheMeshHoldsNothingForJustWorks(t *testing.T) {
+	inv := fresh(t)
+	ctx := t.Context()
+	if _, err := inv.AddNode(ctx, "anchor"); err != nil {
+		t.Fatal(err)
+	}
+	if err := inv.RegisterModule(ctx, manifest("plain", nil, nil), Source{}); err != nil {
+		t.Fatal(err)
+	}
+	if err := inv.ForgetModule(ctx, "plain"); err != nil {
+		t.Fatalf("a module holding nothing was refused: %v", err)
+	}
+}
+
+// A module still on a machine refuses first, whatever it holds: that is not a loss an operator can
+// consent to on the machine's behalf, and unassign is what "I do not want this" means.
+func TestAModuleStillAssignedRefusesBeforeAnythingAboutWhatItHolds(t *testing.T) {
+	inv := fresh(t)
+	ctx := t.Context()
+	if _, err := inv.AddNode(ctx, "anchor"); err != nil {
+		t.Fatal(err)
+	}
+	if err := inv.RegisterModule(ctx, manifest("step-ca", nil, nil), Source{}); err != nil {
+		t.Fatal(err)
+	}
+	if err := inv.SetSettings(ctx, "anchor", "step-ca", map[string]any{"a": 1}); err != nil {
+		t.Fatal(err)
+	}
+	if err := inv.Assign(ctx, "anchor", "step-ca"); err != nil {
+		t.Fatal(err)
+	}
+	for _, forget := range []func() error{
+		func() error { return inv.ForgetModule(ctx, "step-ca") },
+		func() error { _, err := inv.DiscardModule(ctx, "step-ca"); return err },
+	} {
+		if err := forget(); !errors.Is(err, ErrStillAssigned) {
+			t.Fatalf("an assigned module was not refused for being assigned: %v", err)
+		}
+	}
+}