From 7d5ebabecc4a8b90a3b5b3612dfd60acf4cffaef Mon Sep 17 00:00:00 2001 From: Josh Siviour Date: Sun, 2 Aug 2026 08:20:39 +1000 Subject: [PATCH] fix: explicit stop timeouts, graceful-shutdown exit code, and QEMU orphan classification (#640) * fix(systemd): give every unit an explicit TimeoutStopSec 9 of 12 spinifex systemd units had no TimeoutStopSec and inherited systemd's 90s default, which is either far more or far less time than their own SIGTERM handling needs. Size each one to what its shutdown path actually does: predastore matches viperblock's 30s WAL-drain budget since viperblock flushes through it (After= already sequences viperblock's stop first; this is the backstop if that's ever bypassed), spinifex-ui covers its 30s internal shutdown context plus margin for long-lived streaming proxy connections, nats covers its own JetStream-shutdown/raft-stepdown/client-drain signal handler, and services with no SIGTERM handler at all (awsgw) get a short bound since they exit immediately regardless. Extends the existing unit-file invariant test with a check that every .service file declares TimeoutStopSec, so a newly added unit that forgets one fails CI instead of silently falling back to 90s. * fix(vm): reconcile QEMU processes with no instance record on restore classifyRestoredInstances only walks Manager.Snapshot(), so a qemu-system process with no instance record at all -- a crash mid-launch, a lost KV write, a stale process surviving a wiped state file -- was never handed to killOrphanedQEMU and stayed permanently invisible to recovery. Add a host-process scan that diffs live qemu-system PIDs (matched via /proc//comm) against the PIDs claimed by known instances' own -pidfile files. It only reports what it finds, never kills: this daemon cannot distinguish "record not loaded yet" or "owned by another daemon/tenant on this host" from a genuine orphan, and killing a running customer VM to fix an invisibility gap is the wrong trade. Runs at the end of Restore, once every known instance has been classified and relaunched, so a live process still unaccounted for at that point genuinely has no record anywhere. * fix(vm): distinguish recordless QEMU orphans by pidfile ownership A recordless qemu-system process with a stale spinifex pidfile is positively this daemon's own orphan; one with no pidfile at all could belong to another tenant on a shared host. Log the two cases distinctly instead of one blanket warning, so an operator can tell them apart. Also sandbox XDG_RUNTIME_DIR in the shared daemon test harness so Restore()-driven tests never read this box's real runtime directory. * fix(spinifexui): treat ErrServerClosed as a clean shutdown, not a failure Serve returns http.ErrServerClosed after Shutdown is called deliberately (the SIGTERM handler above does exactly that), but Start propagated it as an error, making systemd log a graceful stop as exit code 1 and Result: exit-code -- indistinguishable from a real crash. --- .../systemd/regenerate-ssh-host-keys.service | 3 + build/systemd/spinifex-awsgw.service | 3 + build/systemd/spinifex-nats-watchdog.service | 3 + build/systemd/spinifex-nats.service | 4 + build/systemd/spinifex-northstar.service | 3 + build/systemd/spinifex-predastore.service | 4 + build/systemd/spinifex-qmp-collector.service | 3 + build/systemd/spinifex-ui.service | 3 + build/systemd/spinifex-vpcd.service | 3 + spinifex/daemon/state_test.go | 3 + spinifex/services/spinifexui/spinifexui.go | 9 +- .../services/spinifexui/spinifexui_test.go | 92 ++++++++++ spinifex/systemd/units_invariants_test.go | 24 +++ spinifex/vm/orphan_scan.go | 134 +++++++++++++++ spinifex/vm/orphan_scan_test.go | 157 ++++++++++++++++++ spinifex/vm/restore.go | 4 + 16 files changed, 451 insertions(+), 1 deletion(-) create mode 100644 spinifex/vm/orphan_scan.go create mode 100644 spinifex/vm/orphan_scan_test.go diff --git a/build/systemd/regenerate-ssh-host-keys.service b/build/systemd/regenerate-ssh-host-keys.service index 4830e9f08..f771a5d2e 100644 --- a/build/systemd/regenerate-ssh-host-keys.service +++ b/build/systemd/regenerate-ssh-host-keys.service @@ -7,6 +7,9 @@ Before=ssh.service Type=oneshot ExecStart=/usr/bin/ssh-keygen -A RemainAfterExit=yes +# No ExecStop: the keygen process has already exited by the time this unit +# is stopped, so there is nothing to signal. +TimeoutStopSec=5 [Install] WantedBy=multi-user.target diff --git a/build/systemd/spinifex-awsgw.service b/build/systemd/spinifex-awsgw.service index 1d6be5a78..cb1e5ef21 100644 --- a/build/systemd/spinifex-awsgw.service +++ b/build/systemd/spinifex-awsgw.service @@ -14,6 +14,9 @@ ExecStartPre=/var/lib/spinifex/wait-for-nats.sh ExecStart=/usr/local/bin/spx service awsgw start Restart=on-failure RestartSec=5 +# No SIGTERM handler in this process: it exits immediately on signal, so this +# only bounds an unexpectedly wedged process before SIGKILL. +TimeoutStopSec=10 OOMScoreAdjust=-500 Environment=SPINIFEX_CONFIG_PATH=/etc/spinifex/spinifex.toml diff --git a/build/systemd/spinifex-nats-watchdog.service b/build/systemd/spinifex-nats-watchdog.service index b29cd667c..e97c83a5c 100644 --- a/build/systemd/spinifex-nats-watchdog.service +++ b/build/systemd/spinifex-nats-watchdog.service @@ -9,6 +9,9 @@ Type=oneshot # remedy is `systemctl try-restart spinifex-nats` when JetStream is disabled while # the process stays alive (which Restart=on-failure cannot catch). ExecStart=/var/lib/spinifex/nats-js-watchdog.sh +# Sized for the probe loop's own worst case (3 samples x 5s curl + 2 gaps x 2s +# ~= 19s): a stop mid-cycle lets the script finish rather than cutting it off. +TimeoutStopSec=20 # Hardening (kept compatible with talking to PID1 over /run/systemd). NoNewPrivileges=yes diff --git a/build/systemd/spinifex-nats.service b/build/systemd/spinifex-nats.service index 62fc04ea3..8c9762563 100644 --- a/build/systemd/spinifex-nats.service +++ b/build/systemd/spinifex-nats.service @@ -12,6 +12,10 @@ Group=spinifex ExecStart=/usr/local/bin/spx service nats start Restart=on-failure RestartSec=5 +# nats-server installs its own SIGTERM handler (JetStream store shutdown, raft +# stepdown, client drain) with no internal deadline; every dependent service +# stops before nats does, so size this for nats's own graceful-stop needs. +TimeoutStopSec=20 OOMScoreAdjust=-500 Environment=SPINIFEX_CONFIG_PATH=/etc/spinifex/nats/nats.conf diff --git a/build/systemd/spinifex-northstar.service b/build/systemd/spinifex-northstar.service index ff43360a0..fa199b98b 100644 --- a/build/systemd/spinifex-northstar.service +++ b/build/systemd/spinifex-northstar.service @@ -13,6 +13,9 @@ Group=spinifex ExecStart=/usr/local/bin/spx service northstar start Restart=on-failure RestartSec=5 +# server.Shutdown() is called with an internal 5s context; margin covers the +# NATS unsubscribe/close and process exit that follow it. +TimeoutStopSec=10 OOMScoreAdjust=-500 Environment=SPINIFEX_CONFIG_PATH=/etc/spinifex/spinifex.toml Environment=SPINIFEX_BASE_DIR=/var/lib/spinifex/northstar/ diff --git a/build/systemd/spinifex-predastore.service b/build/systemd/spinifex-predastore.service index 097c42008..cd6d211ce 100644 --- a/build/systemd/spinifex-predastore.service +++ b/build/systemd/spinifex-predastore.service @@ -17,6 +17,10 @@ ExecStartPre=/var/lib/spinifex/wait-for-nats.sh ExecStart=/var/lib/spinifex/predastore-start.sh Restart=on-failure RestartSec=5 +# Matches viperblock's drain budget: viperblock's WAL flush writes into +# predastore, and After= already stops viperblock first, but this is the +# backstop if that ordering is ever bypassed (direct stop, future edit). +TimeoutStopSec=30 OOMScoreAdjust=-500 Environment=SPINIFEX_PREDASTORE_CONFIG_PATH=/etc/spinifex/predastore/predastore.toml Environment=SPINIFEX_PREDASTORE_BASE_PATH=/var/lib/spinifex/predastore/ diff --git a/build/systemd/spinifex-qmp-collector.service b/build/systemd/spinifex-qmp-collector.service index bae2aec62..707eb2be4 100644 --- a/build/systemd/spinifex-qmp-collector.service +++ b/build/systemd/spinifex-qmp-collector.service @@ -16,6 +16,9 @@ ExecStartPre=/var/lib/spinifex/wait-for-nats.sh ExecStart=/usr/local/bin/spx service qmp-collector start Restart=on-failure RestartSec=5 +# Poll loop selects on ctx.Done() directly and closes NATS locally; shutdown +# is near-instant, so this only bounds an unexpectedly wedged process. +TimeoutStopSec=10 Environment=SPINIFEX_CONFIG_PATH=/etc/spinifex/spinifex.toml Environment=SPINIFEX_BASE_DIR=/var/lib/spinifex/spinifex/ # Same runtime dir as the daemon: telemetry sockets + metadata live there diff --git a/build/systemd/spinifex-ui.service b/build/systemd/spinifex-ui.service index 46b7d108c..ee6d306e1 100644 --- a/build/systemd/spinifex-ui.service +++ b/build/systemd/spinifex-ui.service @@ -12,6 +12,9 @@ Group=spinifex ExecStart=/usr/local/bin/spx service spinifex-ui start Restart=on-failure RestartSec=5 +# server.Shutdown() uses an internal 30s context (WriteTimeout=120s covers +# long streaming proxy responses, e.g. console passthrough); add margin above it. +TimeoutStopSec=40 OOMScoreAdjust=-500 Environment=SPINIFEX_UI_TLS_CERT=/etc/spinifex/server.pem Environment=SPINIFEX_UI_TLS_KEY=/etc/spinifex/server.key diff --git a/build/systemd/spinifex-vpcd.service b/build/systemd/spinifex-vpcd.service index 38bbc7f01..f4d37325e 100644 --- a/build/systemd/spinifex-vpcd.service +++ b/build/systemd/spinifex-vpcd.service @@ -14,6 +14,9 @@ ExecStartPre=/var/lib/spinifex/wait-for-nats.sh ExecStart=/usr/local/bin/spx service vpcd start Restart=on-failure RestartSec=5 +# Waits for an in-flight OVN drift-reconcile pass to return before exiting; +# no sub-timeout bounds that call today, so this covers a normal pass. +TimeoutStopSec=20 OOMScoreAdjust=-500 Environment=SPINIFEX_CONFIG_PATH=/etc/spinifex/spinifex.toml Environment=SPINIFEX_BASE_DIR=/var/lib/spinifex/vpcd/ diff --git a/spinifex/daemon/state_test.go b/spinifex/daemon/state_test.go index 5a81eab8c..a8370b334 100644 --- a/spinifex/daemon/state_test.go +++ b/spinifex/daemon/state_test.go @@ -26,6 +26,9 @@ func createDaemonWithJetStream(t *testing.T) *Daemon { _, nc, _ := testutil.StartTestJetStream(t) tmpDir := t.TempDir() + // Restore's orphan reconciliation reads pidfiles from the runtime dir; + // sandbox it so tests never touch this box's real /run/spinifex. + t.Setenv("XDG_RUNTIME_DIR", tmpDir) clusterCfg := &config.ClusterConfig{ Node: "node-1", diff --git a/spinifex/services/spinifexui/spinifexui.go b/spinifex/services/spinifexui/spinifexui.go index 01ba181ac..f87588c28 100644 --- a/spinifex/services/spinifexui/spinifexui.go +++ b/spinifex/services/spinifexui/spinifexui.go @@ -4,6 +4,7 @@ import ( "context" "crypto/tls" "embed" + "errors" "fmt" "io/fs" "log/slog" @@ -279,7 +280,13 @@ func (svc *Service) launchService() error { } slog.Info("Starting spinifex-ui service with HTTPS (auto-redirect HTTP)", "addr", addr) - return server.Serve(splitLn) + // ErrServerClosed is Serve's documented return value after Shutdown was + // called deliberately (see the SIGTERM handler above) -- success, not a + // failure to report or exit non-zero for. + if err := server.Serve(splitLn); err != nil && !errors.Is(err, http.ErrServerClosed) { + return err + } + return nil } // Content-Security-Policy header. All API requests are proxied through the diff --git a/spinifex/services/spinifexui/spinifexui_test.go b/spinifex/services/spinifexui/spinifexui_test.go index 55822bb98..e5d2445c6 100644 --- a/spinifex/services/spinifexui/spinifexui_test.go +++ b/spinifex/services/spinifexui/spinifexui_test.go @@ -2,9 +2,15 @@ package spinifexui import ( "compress/gzip" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" "crypto/tls" + "crypto/x509" + "crypto/x509/pkix" "encoding/pem" "io" + "math/big" "net" "net/http" "net/http/httptest" @@ -12,6 +18,7 @@ import ( "path/filepath" "strconv" "testing" + "time" "github.com/go-chi/chi/v5/middleware" "github.com/stretchr/testify/assert" @@ -448,3 +455,88 @@ func TestShutdown_WithServer(t *testing.T) { err = svc.Shutdown() assert.NoError(t, err) } + +// writeTestTLSFiles generates a self-signed cert/key pair on disk for a +// launchService test, which needs real file paths (tls.LoadX509KeyPair), +// unlike loadTLSCert's in-memory tls.Certificate used by the split-listener tests. +func writeTestTLSFiles(t *testing.T) (certPath, keyPath string) { + t.Helper() + + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + + template := &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: "spinifexui-test"}, + NotBefore: time.Now(), + NotAfter: time.Now().Add(time.Hour), + IPAddresses: []net.IP{net.ParseIP("127.0.0.1")}, + } + certDER, err := x509.CreateCertificate(rand.Reader, template, template, &key.PublicKey, key) + require.NoError(t, err) + keyDER, err := x509.MarshalECPrivateKey(key) + require.NoError(t, err) + + certPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: certDER}) + + dir := t.TempDir() + certPath = filepath.Join(dir, "server.pem") + keyPath = filepath.Join(dir, "server.key") + require.NoError(t, os.WriteFile(certPath, certPEM, 0o600)) + require.NoError(t, os.WriteFile(keyPath, + pem.EncodeToMemory(&pem.Block{Type: "EC PRIVATE KEY", Bytes: keyDER}), 0o600)) + // newProxyTransport reads /ca.pem; the self-signed leaf + // itself is a valid enough PEM cert to satisfy that read in a test. + require.NoError(t, os.WriteFile(filepath.Join(dir, "ca.pem"), certPEM, 0o600)) + return certPath, keyPath +} + +// freePort binds an ephemeral port, closes it immediately, and returns the +// number so Start can bind the same port without hardcoding 3000 (which may +// already be in use by a real spinifex-ui on a shared box). +func freePort(t *testing.T) int { + t.Helper() + ln, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + port := ln.Addr().(*net.TCPAddr).Port + require.NoError(t, ln.Close()) + return port +} + +// TestStart_GracefulShutdownIsNotAnError guards against Start propagating +// http.ErrServerClosed -- Serve's documented return value after a deliberate +// Shutdown -- as a failure. Before the fix this made a clean stop look like +// a crash (exit code 1) to systemd. +func TestStart_GracefulShutdownIsNotAnError(t *testing.T) { + t.Setenv("XDG_RUNTIME_DIR", t.TempDir()) + certPath, keyPath := writeTestTLSFiles(t) + + svc, err := New(&Config{ + Port: freePort(t), + Host: "127.0.0.1", + TLSCert: certPath, + TLSKey: keyPath, + }) + require.NoError(t, err) + + startErrCh := make(chan error, 1) + go func() { + _, startErr := svc.Start() + startErrCh <- startErr + }() + + require.Eventually(t, func() bool { + svc.mu.Lock() + defer svc.mu.Unlock() + return svc.server != nil + }, 2*time.Second, 10*time.Millisecond, "server never registered before shutdown") + + require.NoError(t, svc.Shutdown()) + + select { + case startErr := <-startErrCh: + assert.NoError(t, startErr, "Start must treat a deliberate Shutdown's ErrServerClosed as success") + case <-time.After(2 * time.Second): + t.Fatal("Start did not return after Shutdown") + } +} diff --git a/spinifex/systemd/units_invariants_test.go b/spinifex/systemd/units_invariants_test.go index e373fe039..c409b0236 100644 --- a/spinifex/systemd/units_invariants_test.go +++ b/spinifex/systemd/units_invariants_test.go @@ -262,6 +262,30 @@ func TestRG11_LeanUnits(t *testing.T) { } } +// TestAllServiceUnitsDeclareTimeoutStopSec asserts every .service unit pins an +// explicit TimeoutStopSec, so a newly added unit that forgets one falls back to +// systemd's blanket 90s default instead of a value chosen for that service's +// own SIGTERM handling. +func TestAllServiceUnitsDeclareTimeoutStopSec(t *testing.T) { + dir := unitsDir(t) + for _, name := range unitFiles(t, dir) { + if !strings.HasSuffix(name, ".service") { + continue + } + u := readUnit(t, dir, name) + found := false + for l := range strings.SplitSeq(u, "\n") { + if strings.HasPrefix(strings.TrimSpace(l), "TimeoutStopSec=") { + found = true + break + } + } + if !found { + t.Errorf("%s must declare an explicit TimeoutStopSec=", name) + } + } +} + // TestApplicationUnitsExportTelemetry asserts every unit that runs an spx // application service sources the telemetry drop-in. That file carries // OTEL_EXPORTER_OTLP_ENDPOINT, MULGA_ENV and MULGA_SOURCE, so a unit missing it diff --git a/spinifex/vm/orphan_scan.go b/spinifex/vm/orphan_scan.go new file mode 100644 index 000000000..604576298 --- /dev/null +++ b/spinifex/vm/orphan_scan.go @@ -0,0 +1,134 @@ +package vm + +import ( + "log/slog" + "os" + "path/filepath" + "strconv" + "strings" + + "github.com/mulgadc/spinifex/spinifex/utils" +) + +// qemuProcessPrefix matches both qemu-system-x86_64 and qemu-system-aarch64; +// /proc//comm truncates at 15 bytes but this prefix is only 11. +const qemuProcessPrefix = "qemu-system" + +// qemuProcRoot is the /proc mount scanned for live qemu-system processes. +// Overridden in tests to avoid depending on the real host process table. +var qemuProcRoot = "/proc" + +// scanLiveQEMUPIDs walks procRoot for processes whose comm starts with +// qemuProcessPrefix. procRoot is a parameter so tests point it at a +// fabricated directory instead of spawning a real qemu-system binary. +func scanLiveQEMUPIDs(procRoot string) ([]int, error) { + entries, err := os.ReadDir(procRoot) + if err != nil { + return nil, err + } + + var pids []int + for _, entry := range entries { + pid, err := strconv.Atoi(entry.Name()) + if err != nil || pid <= 0 { + continue + } + comm, err := os.ReadFile(filepath.Join(procRoot, entry.Name(), "comm")) + if err != nil { + continue + } + if strings.HasPrefix(strings.TrimSpace(string(comm)), qemuProcessPrefix) { + pids = append(pids, pid) + } + } + return pids, nil +} + +// pidFileOwners reads every ".pid" file in runtimeDir and +// returns the PID -> instance ID it names, regardless of whether that +// instance still exists in Snapshot(). QEMU itself writes these via +// -pidfile, so an entry here is a claim this daemon made at some point -- +// not necessarily one it still stands behind. +func pidFileOwners(runtimeDir string) map[int]string { + owners := make(map[int]string) + entries, err := os.ReadDir(runtimeDir) + if err != nil { + return owners + } + for _, entry := range entries { + name := entry.Name() + if !strings.HasSuffix(name, ".pid") { + continue + } + id := strings.TrimSuffix(name, ".pid") + if pid, err := utils.ReadPidFileFrom(runtimeDir, id); err == nil { + owners[pid] = id + } + } + return owners +} + +// qemuOrphan is a live qemu-system PID with no matching instance in +// Snapshot(). hasPidFile distinguishes two very different situations: a +// pidfile this daemon itself wrote (instanceID is that pidfile's claim, +// positively this daemon's process) versus no pidfile at all (could be +// another tenant's process on a shared host). +type qemuOrphan struct { + pid int + instanceID string + hasPidFile bool +} + +// classifyQEMUOrphans returns every live qemu-system PID not vouched for by +// a pidfile naming a known instance. This is the gap classifyRestoredInstances +// cannot see, since it only walks Snapshot(). +func classifyQEMUOrphans(procRoot, runtimeDir string, known map[string]bool) ([]qemuOrphan, error) { + live, err := scanLiveQEMUPIDs(procRoot) + if err != nil { + return nil, err + } + if len(live) == 0 { + return nil, nil + } + + owners := pidFileOwners(runtimeDir) + var orphans []qemuOrphan + for _, pid := range live { + id, hasPidFile := owners[pid] + if hasPidFile && known[id] { + continue // legitimate, tracked instance + } + orphans = append(orphans, qemuOrphan{pid: pid, instanceID: id, hasPidFile: hasPidFile}) + } + return orphans, nil +} + +// reportRecordlessQEMUOrphans logs, but never kills, a qemu-system process +// with no instance record. A pidfile in runtimeDir naming an unknown +// instance positively identifies the process as this daemon's own stale +// orphan (logged distinctly, safe for an operator to reap by hand); no +// pidfile at all could belong to another tenant/daemon on a shared host, so +// it is logged as unowned. Neither case signals the process automatically: +// a false positive here would destroy a running customer VM. Call only +// after Restore has finished loading and relaunching every known instance. +func (m *Manager) reportRecordlessQEMUOrphans() { + known := make(map[string]bool) + for _, instance := range m.Snapshot() { + known[instance.ID] = true + } + + orphans, err := classifyQEMUOrphans(qemuProcRoot, utils.RuntimeDir(), known) + if err != nil { + slog.Warn("recordless QEMU orphan scan failed", "error", err) + return + } + for _, o := range orphans { + if o.hasPidFile { + slog.Warn("qemu-system process has a stale spinifex pidfile with no matching instance record; not killing, safe to reap manually", + "pid", o.pid, "instanceId", o.instanceID) + continue + } + slog.Warn("qemu-system process has no spinifex pidfile at all; not killing, ownership unconfirmed", + "pid", o.pid) + } +} diff --git a/spinifex/vm/orphan_scan_test.go b/spinifex/vm/orphan_scan_test.go new file mode 100644 index 000000000..4d22b56dc --- /dev/null +++ b/spinifex/vm/orphan_scan_test.go @@ -0,0 +1,157 @@ +package vm + +import ( + "os" + "path/filepath" + "strconv" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// fakeProcEntry creates procRoot//comm containing name, mimicking the +// /proc layout scanLiveQEMUPIDs reads, without spawning a real process. +func fakeProcEntry(t *testing.T, procRoot string, pid int, name string) { + t.Helper() + dir := filepath.Join(procRoot, strconv.Itoa(pid)) + require.NoError(t, os.MkdirAll(dir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "comm"), []byte(name+"\n"), 0o644)) +} + +func TestScanLiveQEMUPIDs(t *testing.T) { + procRoot := t.TempDir() + fakeProcEntry(t, procRoot, 111, "qemu-system-x86_64") + fakeProcEntry(t, procRoot, 222, "qemu-system-aarch64") + fakeProcEntry(t, procRoot, 333, "bash") + // Non-numeric /proc entries (self, curproc, etc.) must not crash the scan. + require.NoError(t, os.MkdirAll(filepath.Join(procRoot, "self"), 0o755)) + + pids, err := scanLiveQEMUPIDs(procRoot) + + require.NoError(t, err) + assert.ElementsMatch(t, []int{111, 222}, pids, "only qemu-system* processes are returned") +} + +func TestScanLiveQEMUPIDs_MissingProcRoot(t *testing.T) { + _, err := scanLiveQEMUPIDs(filepath.Join(t.TempDir(), "does-not-exist")) + assert.Error(t, err, "a missing proc root must surface an error, not a silent empty result") +} + +func TestPidFileOwners(t *testing.T) { + runtimeDir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(runtimeDir, "i-known.pid"), []byte("111"), 0o600)) + require.NoError(t, os.WriteFile(filepath.Join(runtimeDir, "i-gone.pid"), []byte("999"), 0o600)) + + owners := pidFileOwners(runtimeDir) + + assert.Equal(t, "i-known", owners[111], "a pidfile's PID maps to the instance ID it names") + assert.Equal(t, "i-gone", owners[999], + "pidFileOwners is not filtered by known -- it returns every claim on disk, known or not") +} + +func TestClassifyQEMUOrphans(t *testing.T) { + t.Run("known instance's QEMU is left alone", func(t *testing.T) { + procRoot, runtimeDir := t.TempDir(), t.TempDir() + fakeProcEntry(t, procRoot, 111, "qemu-system-x86_64") + require.NoError(t, os.WriteFile(filepath.Join(runtimeDir, "i-known.pid"), []byte("111"), 0o600)) + + orphans, err := classifyQEMUOrphans(procRoot, runtimeDir, map[string]bool{"i-known": true}) + + require.NoError(t, err) + assert.Empty(t, orphans, "a QEMU process claimed by a known instance's pidfile is not an orphan") + }) + + t.Run("recordless QEMU process with no pidfile at all", func(t *testing.T) { + procRoot, runtimeDir := t.TempDir(), t.TempDir() + fakeProcEntry(t, procRoot, 222, "qemu-system-x86_64") + // No pidfile at all ties PID 222 to any instance. + + orphans, err := classifyQEMUOrphans(procRoot, runtimeDir, map[string]bool{}) + + require.NoError(t, err) + require.Len(t, orphans, 1) + assert.Equal(t, 222, orphans[0].pid) + assert.False(t, orphans[0].hasPidFile, "no pidfile claims this PID -- ownership is unconfirmed") + }) + + t.Run("stale pidfile naming an unknown instance is classified as this daemon's own", func(t *testing.T) { + procRoot, runtimeDir := t.TempDir(), t.TempDir() + fakeProcEntry(t, procRoot, 333, "qemu-system-aarch64") + // Pidfile exists but names an instance this manager has no record of. + require.NoError(t, os.WriteFile(filepath.Join(runtimeDir, "i-gone.pid"), []byte("333"), 0o600)) + + orphans, err := classifyQEMUOrphans(procRoot, runtimeDir, map[string]bool{}) + + require.NoError(t, err) + require.Len(t, orphans, 1) + assert.Equal(t, 333, orphans[0].pid) + assert.True(t, orphans[0].hasPidFile, "a pidfile this daemon wrote positively identifies the process") + assert.Equal(t, "i-gone", orphans[0].instanceID) + }) + + t.Run("mixed: known left alone, recordless-with-pidfile and recordless-without kept separate", func(t *testing.T) { + procRoot, runtimeDir := t.TempDir(), t.TempDir() + fakeProcEntry(t, procRoot, 111, "qemu-system-x86_64") // known + fakeProcEntry(t, procRoot, 222, "qemu-system-x86_64") // stale pidfile + fakeProcEntry(t, procRoot, 333, "qemu-system-x86_64") // no pidfile + require.NoError(t, os.WriteFile(filepath.Join(runtimeDir, "i-known.pid"), []byte("111"), 0o600)) + require.NoError(t, os.WriteFile(filepath.Join(runtimeDir, "i-gone.pid"), []byte("222"), 0o600)) + + orphans, err := classifyQEMUOrphans(procRoot, runtimeDir, map[string]bool{"i-known": true}) + + require.NoError(t, err) + require.Len(t, orphans, 2) + byPID := map[int]qemuOrphan{} + for _, o := range orphans { + byPID[o.pid] = o + } + assert.True(t, byPID[222].hasPidFile) + assert.Equal(t, "i-gone", byPID[222].instanceID) + assert.False(t, byPID[333].hasPidFile) + }) + + t.Run("no live qemu-system processes returns no orphans", func(t *testing.T) { + procRoot, runtimeDir := t.TempDir(), t.TempDir() + fakeProcEntry(t, procRoot, 444, "bash") + + orphans, err := classifyQEMUOrphans(procRoot, runtimeDir, map[string]bool{}) + + require.NoError(t, err) + assert.Empty(t, orphans) + }) +} + +// TestReportRecordlessQEMUOrphans covers the Manager-level entry point used +// by Restore: a known instance's QEMU must not be reported, and the two +// orphan classes (own stale pidfile vs no pidfile at all) must be logged +// distinctly so an operator can tell them apart. Neither is signalled -- +// there is no kill path in this function to assert against. +func TestReportRecordlessQEMUOrphans(t *testing.T) { + procRoot := t.TempDir() + runtimeDir := t.TempDir() + t.Setenv("XDG_RUNTIME_DIR", runtimeDir) + + origProcRoot := qemuProcRoot + qemuProcRoot = procRoot + t.Cleanup(func() { qemuProcRoot = origProcRoot }) + + fakeProcEntry(t, procRoot, 111, "qemu-system-x86_64") // claimed by i-known + fakeProcEntry(t, procRoot, 222, "qemu-system-x86_64") // stale pidfile, unknown instance + fakeProcEntry(t, procRoot, 333, "qemu-system-x86_64") // no pidfile at all + require.NoError(t, os.WriteFile(filepath.Join(runtimeDir, "i-known.pid"), []byte("111"), 0o600)) + require.NoError(t, os.WriteFile(filepath.Join(runtimeDir, "i-gone.pid"), []byte("222"), 0o600)) + + m := NewManager() + m.Replace(map[string]*VM{"i-known": {ID: "i-known", Status: StateRunning}}) + + buf := captureSlogRestore(t) + m.reportRecordlessQEMUOrphans() + output := buf.String() + + assert.NotContains(t, output, "pid=111", "the known instance's QEMU PID must not be reported as an orphan") + assert.Contains(t, output, "pid=222", "the stale-pidfile orphan must be reported") + assert.Contains(t, output, "i-gone", "the stale pidfile's instance ID must be logged for the operator") + assert.Contains(t, output, "pid=333", "the no-pidfile orphan must be reported") + assert.Contains(t, output, "not killing", "neither orphan class is signalled") +} diff --git a/spinifex/vm/restore.go b/spinifex/vm/restore.go index 3dcecfaf9..67546344a 100644 --- a/spinifex/vm/restore.go +++ b/spinifex/vm/restore.go @@ -49,6 +49,10 @@ func (m *Manager) Restore() { if err := m.writeRunningState(); err != nil { slog.Error("Failed to persist state after restore", "error", err) } + + // Every known instance is classified and relaunched by this point, so a + // live qemu-system process still unaccounted for genuinely has no record. + m.reportRecordlessQEMUOrphans() } // loadRunningState replaces the manager's running set from the persisted snapshot.