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.