Hold the removal order a module may rely on, and say what a killed stop leaves (hq ADR 0263)

internal/apply/process.go

@@ -383,6 +383,14 @@ func removeProcess(ctx context.Context, a store.Applied, run Runner) (string, st
 		}
 		found = true
 		// The timer first, so a scheduled run cannot start the service between the two.
+		//
+		// `--now` waits for the stop. The unit the host writes sets no stop timeout and no kill mode, so the
+		// service manager's defaults hold: SIGTERM, then after its default stop timeout (90 s unless the
+		// machine changed DefaultTimeoutStopSec) SIGKILL to the whole group. What a process does on SIGTERM —
+		// nfs-server removing its export file when its configuration is gone (novox/hq ADR 0263) — happens
+		// only if it finishes within that time; a killed process cleans nothing up, and an unassigned module
+		// never starts again to do it later. So a process that cleans up on stop does that first and keeps it
+		// short, rather than after work that can block.
 		if _, err := run(ctx, "systemctl", "disable", "--now", unit); err != nil {
 			return "", "", fmt.Errorf("stopping %s: %w", unit, err)
 		}

internal/apply/removal_order_test.go

@@ -0,0 +1,118 @@
+package apply
+
+import (
+	"context"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/novox/mesh-host/internal/store"
+)
+
+// The contract at store.State.Orphans, held: a module's resources are removed newest record first, so
+// a process recorded before the configuration it reads is stopped after that configuration is gone.
+// mesh-catalog's nfs-server relies on it (novox/hq ADR 0263): its `exports` process, declared before
+// its config directory and file, finds the configuration gone when it is stopped on unassignment and
+// only then removes its export file; on a restart or a push the configuration is still there and it
+// leaves the exports alone.
+func TestAnUnassignedModulesProcessIsStoppedAfterTheConfigurationItReads(t *testing.T) {
+	units, bundles := t.TempDir(), t.TempDir()
+	wasUnits, wasBundles := unitDir, daemonRoot
+	unitDir, daemonRoot = units, bundles
+	t.Cleanup(func() { unitDir, daemonRoot = wasUnits, wasBundles })
+
+	etc := filepath.Join(t.TempDir(), "nfs-server")
+	config := filepath.Join(etc, "shares.conf")
+	body, digest := anArchive(t, map[string]string{"nfs-server": "#!/bin/sh\n"})
+	process := `{"id":"nfs-server.exports","type":"process","name":"nfs-server-exports","source":"` + serving(t, body) +
+		`","digest":"` + digest + `","run":["./nfs-server","exports"],"restart-on":["nfs-server.config"]}`
+	// In the manifest's order: the process first, then the directory and the file it reads.
+	module := process + `,
+	  {"id":"nfs-server.config-dir","type":"directory","path":"` + etc + `","mode":"0755"},
+	  {"id":"nfs-server.config","type":"file","path":"` + config + `","content":"shares=library=/srv/library\n","mode":"0644"}`
+	other := `{"id":"other.f","type":"file","path":"` + filepath.Join(t.TempDir(), "other") + `","content":"o\n"}`
+
+	var configAtStop []bool
+	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		if name == "systemctl" && strings.Join(args, " ") == "disable --now nfs-server-exports.service" {
+			_, err := os.Stat(config)
+			configAtStop = append(configAtStop, err == nil)
+		}
+		return "", nil
+	}
+
+	_, state, err := Apply(context.Background(), archHost(t), declare(t, module+","+other), store.State{}, store.OriginDeclared, run, nil, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	// The record keeps the declared order: a process is not moved after what it restarts on (only a
+	// service is, by afterWhatTheyRead), and a record replaced on a later apply keeps its place.
+	var order []string
+	for _, r := range state.Resources {
+		order = append(order, r.ID)
+	}
+	if strings.Join(order, " ") != "nfs-server.exports nfs-server.config-dir nfs-server.config other.f" {
+		t.Fatalf("recorded in the order %v, not the declared one", order)
+	}
+	if _, state, err = Apply(context.Background(), archHost(t), declare(t, module+","+other), state, store.OriginDeclared, run, nil, nil); err != nil {
+		t.Fatal(err)
+	}
+
+	report, _, err := Apply(context.Background(), archHost(t), declare(t, other), state, store.OriginDeclared, run, nil, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(configAtStop) != 1 || configAtStop[0] {
+		t.Errorf("the process was stopped while its configuration was still there (seen at each stop: %v)", configAtStop)
+	}
+	var removed []string
+	for _, o := range report.Outcomes {
+		if o.Action == "removed" {
+			removed = append(removed, o.ID)
+		}
+	}
+	if strings.Join(removed, " ") != "nfs-server.config nfs-server.config-dir nfs-server.exports" {
+		t.Errorf("removed in the order %v; the newest record goes first", removed)
+	}
+}
+
+// What the contract does not promise: a resource first recorded after the module's other resources —
+// one added in a later version of the module, or one whose first apply failed — is appended, so it
+// is removed first. A module that relies on the order declares the process from its first version and
+// keeps it applying; the test shows the flip so nobody reads the contract as stronger than it is.
+func TestAProcessRecordedAfterItsConfigurationIsStoppedFirst(t *testing.T) {
+	units, bundles := t.TempDir(), t.TempDir()
+	wasUnits, wasBundles := unitDir, daemonRoot
+	unitDir, daemonRoot = units, bundles
+	t.Cleanup(func() { unitDir, daemonRoot = wasUnits, wasBundles })
+
+	config := filepath.Join(t.TempDir(), "shares.conf")
+	body, digest := anArchive(t, map[string]string{"nfs-server": "#!/bin/sh\n"})
+	process := `{"id":"m.exports","type":"process","name":"m-exports","source":"` + serving(t, body) +
+		`","digest":"` + digest + `","run":["./nfs-server","exports"],"restart-on":["m.config"]}`
+	file := `{"id":"m.config","type":"file","path":"` + config + `","content":"x\n","mode":"0644"}`
+	other := `{"id":"other.f","type":"file","path":"` + filepath.Join(t.TempDir(), "other") + `","content":"o\n"}`
+
+	var configAtStop []bool
+	run := func(ctx context.Context, name string, args ...string) (string, error) {
+		if name == "systemctl" && strings.Join(args, " ") == "disable --now m-exports.service" {
+			_, err := os.Stat(config)
+			configAtStop = append(configAtStop, err == nil)
+		}
+		return "", nil
+	}
+	_, state, err := Apply(context.Background(), archHost(t), declare(t, file+","+other), store.State{}, store.OriginDeclared, run, nil, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, state, err = Apply(context.Background(), archHost(t), declare(t, process+","+file+","+other), state, store.OriginDeclared, run, nil, nil); err != nil {
+		t.Fatal(err)
+	}
+	if _, _, err = Apply(context.Background(), archHost(t), declare(t, other), state, store.OriginDeclared, run, nil, nil); err != nil {
+		t.Fatal(err)
+	}
+	if len(configAtStop) != 1 || !configAtStop[0] {
+		t.Errorf("a process recorded after its configuration was not stopped first (seen at each stop: %v)", configAtStop)
+	}
+}

internal/store/store.go

@@ -583,6 +583,17 @@ func (s *State) Forget(id string) {
 // Reverse order because undoing in the order things were made undoes a directory before the
 // file inside it. Reversing is the only ordering the host can derive without deciding
 // anything, which is the line novox/hq ADR 0005 draws.
+//
+// **A module may rely on this order, and on no more than it says.** The order is the order of the
+// records: a resource is recorded when it is first applied (Record keeps an existing record's place),
+// and resources are applied in their declared order, except that a service waits for what it restarts
+// or reloads on (afterWhatTheyRead) — a process does not. So of one module's resources, what was
+// declared later is removed first. mesh-catalog's nfs-server relies on it (novox/hq ADR 0263): its
+// process, declared before its configuration, is stopped after that configuration is removed, finds it
+// gone, and only then removes the export file it wrote; on a restart or a push the configuration is
+// there and the exports stay. What the order does not promise: a resource first recorded later — added
+// in a later version of the module, or whose first apply failed — is appended, and so goes first.
+// Both cases are held by internal/apply/removal_order_test.go.
 func (s State) Orphans(declared map[string]bool, origin string) []Applied {
 	var out []Applied
 	for i := len(s.Resources) - 1; i >= 0; i-- {