From fedd9dcf350e60651ba4d52447094410ddf65431 Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 8 Oct 2026 21:16:04 +0200 Subject: [PATCH] Hold the removal order a module may rely on, and say what a killed stop leaves (hq ADR 0263) --- internal/apply/process.go | 8 ++ internal/apply/removal_order_test.go | 118 +++++++++++++++++++++++++++ internal/store/store.go | 11 +++ 3 files changed, 137 insertions(+) create mode 100644 internal/apply/removal_order_test.go diff --git a/internal/apply/process.go b/internal/apply/process.go index e924de3..917e79b 100644 --- a/internal/apply/process.go +++ b/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) } diff --git a/internal/apply/removal_order_test.go b/internal/apply/removal_order_test.go new file mode 100644 index 0000000..711603c --- /dev/null +++ b/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) + } +} diff --git a/internal/store/store.go b/internal/store/store.go index 90f0bb3..01db4d6 100644 --- a/internal/store/store.go +++ b/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-- {