diff --git a/build/systemd/spinifex-awsgw.service b/build/systemd/spinifex-awsgw.service index cb1e5ef21..5efc21605 100644 --- a/build/systemd/spinifex-awsgw.service +++ b/build/systemd/spinifex-awsgw.service @@ -19,6 +19,10 @@ RestartSec=5 TimeoutStopSec=10 OOMScoreAdjust=-500 +# Per-service ceiling bounds a leak in this service alone rather than +# tripping the slice-wide cap, which would pick an arbitrary victim. +MemoryHigh=10% +MemoryMax=20% Environment=SPINIFEX_CONFIG_PATH=/etc/spinifex/spinifex.toml Environment=SPINIFEX_BASE_DIR=/var/lib/spinifex/awsgw/ Environment=SPINIFEX_AWSGW_TLS_CERT=/etc/spinifex/server.pem diff --git a/build/systemd/spinifex-daemon.service b/build/systemd/spinifex-daemon.service index 54767f701..ea16f9b75 100644 --- a/build/systemd/spinifex-daemon.service +++ b/build/systemd/spinifex-daemon.service @@ -19,6 +19,10 @@ KillMode=process TimeoutStopSec=15 # RG-4: infra tier — reaped only after every guest OOMScoreAdjust=-500 +# Guest QEMU still runs inside this cgroup, so the ceiling tracks the slice +# budget (just under 80%/90%), not just daemon overhead. +MemoryHigh=75% +MemoryMax=85% Environment=SPINIFEX_CONFIG_PATH=/etc/spinifex/spinifex.toml Environment=SPINIFEX_BASE_DIR=/var/lib/spinifex/spinifex/ Environment=SPINIFEX_WAL_DIR=/var/lib/spinifex/spinifex/ diff --git a/build/systemd/spinifex-nats.service b/build/systemd/spinifex-nats.service index 8c9762563..583ed3d11 100644 --- a/build/systemd/spinifex-nats.service +++ b/build/systemd/spinifex-nats.service @@ -17,6 +17,10 @@ RestartSec=5 # stops before nats does, so size this for nats's own graceful-stop needs. TimeoutStopSec=20 OOMScoreAdjust=-500 +# Per-service ceiling bounds a leak in this service alone rather than +# tripping the slice-wide cap, which would pick an arbitrary victim. +MemoryHigh=10% +MemoryMax=20% Environment=SPINIFEX_CONFIG_PATH=/etc/spinifex/nats/nats.conf # Filesystem access diff --git a/build/systemd/spinifex-northstar.service b/build/systemd/spinifex-northstar.service index fa199b98b..f65bb2507 100644 --- a/build/systemd/spinifex-northstar.service +++ b/build/systemd/spinifex-northstar.service @@ -17,6 +17,10 @@ RestartSec=5 # NATS unsubscribe/close and process exit that follow it. TimeoutStopSec=10 OOMScoreAdjust=-500 +# Per-service ceiling bounds a leak in this service alone rather than +# tripping the slice-wide cap, which would pick an arbitrary victim. +MemoryHigh=10% +MemoryMax=20% 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 cd6d211ce..307db3194 100644 --- a/build/systemd/spinifex-predastore.service +++ b/build/systemd/spinifex-predastore.service @@ -22,6 +22,11 @@ RestartSec=5 # backstop if that ordering is ever bypassed (direct stop, future edit). TimeoutStopSec=30 OOMScoreAdjust=-500 +# Legitimately the most memory-hungry infra service (S3 buffering/caching); +# High/Max stay well above idle so normal load isn't throttled, while still +# bounding a leak short of the slice-wide cap. +MemoryHigh=40% +MemoryMax=55% Environment=SPINIFEX_PREDASTORE_CONFIG_PATH=/etc/spinifex/predastore/predastore.toml Environment=SPINIFEX_PREDASTORE_BASE_PATH=/var/lib/spinifex/predastore/ Environment=SPINIFEX_PREDASTORE_TLS_CERT=/etc/spinifex/server.pem diff --git a/build/systemd/spinifex-qmp-collector.service b/build/systemd/spinifex-qmp-collector.service index 707eb2be4..0cdb59842 100644 --- a/build/systemd/spinifex-qmp-collector.service +++ b/build/systemd/spinifex-qmp-collector.service @@ -19,6 +19,10 @@ 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 +# Per-service ceiling bounds a leak in this service alone rather than +# tripping the slice-wide cap, which would pick an arbitrary victim. +MemoryHigh=10% +MemoryMax=20% 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 ee6d306e1..600f5641a 100644 --- a/build/systemd/spinifex-ui.service +++ b/build/systemd/spinifex-ui.service @@ -16,6 +16,10 @@ RestartSec=5 # long streaming proxy responses, e.g. console passthrough); add margin above it. TimeoutStopSec=40 OOMScoreAdjust=-500 +# Per-service ceiling bounds a leak in this service alone rather than +# tripping the slice-wide cap, which would pick an arbitrary victim. +MemoryHigh=10% +MemoryMax=20% Environment=SPINIFEX_UI_TLS_CERT=/etc/spinifex/server.pem Environment=SPINIFEX_UI_TLS_KEY=/etc/spinifex/server.key Environment=SPINIFEX_UI_BASE_DIR=/var/lib/spinifex/spinifex-ui/ diff --git a/build/systemd/spinifex-viperblock.service b/build/systemd/spinifex-viperblock.service index 3c83933f4..71f83c62f 100644 --- a/build/systemd/spinifex-viperblock.service +++ b/build/systemd/spinifex-viperblock.service @@ -15,6 +15,10 @@ ExecStart=/usr/local/bin/spx service viperblock start Restart=on-failure RestartSec=5 OOMScoreAdjust=-500 +# Per-service ceiling bounds a leak in this service alone rather than +# tripping the slice-wide cap, which would pick an arbitrary victim. +MemoryHigh=15% +MemoryMax=25% # KillMode=process: nbdkit still serving a guest survives a restart here, the # way guest QEMU survives a daemon restart. Tearing one out mid-write corrupts # that guest's filesystem; idle nbdkit is reaped explicitly on shutdown. diff --git a/build/systemd/spinifex-vpcd.service b/build/systemd/spinifex-vpcd.service index f4d37325e..3ef8606ed 100644 --- a/build/systemd/spinifex-vpcd.service +++ b/build/systemd/spinifex-vpcd.service @@ -18,6 +18,9 @@ RestartSec=5 # no sub-timeout bounds that call today, so this covers a normal pass. TimeoutStopSec=20 OOMScoreAdjust=-500 +# Per-service ceiling: bounds a leak here instead of the slice-wide cap. +MemoryHigh=10% +MemoryMax=20% Environment=SPINIFEX_CONFIG_PATH=/etc/spinifex/spinifex.toml Environment=SPINIFEX_BASE_DIR=/var/lib/spinifex/vpcd/ diff --git a/go.mod b/go.mod index 82890692e..74f7fdf73 100644 --- a/go.mod +++ b/go.mod @@ -22,7 +22,7 @@ require ( github.com/miekg/dns v1.1.72 github.com/mulgadc/northstar v1.14.0 github.com/mulgadc/predastore v1.14.1-0.20260730221229-608939ef2bb4 - github.com/mulgadc/viperblock v1.14.1-0.20260801223811-8a4787ade83a + github.com/mulgadc/viperblock v1.14.1-0.20260802084514-ed52f6a0e4c0 github.com/nats-io/nats-server/v2 v2.14.3 github.com/nats-io/nats.go v1.52.0 github.com/opencontainers/runtime-spec v1.3.0 diff --git a/go.sum b/go.sum index d6ef7700e..837017fdb 100644 --- a/go.sum +++ b/go.sum @@ -1321,8 +1321,8 @@ github.com/mulgadc/northstar v1.14.0 h1:8JzDcS4k3SN5PF3jBdB8x9FOWC24eITCClmOdwpo github.com/mulgadc/northstar v1.14.0/go.mod h1:6EArbWIQUrCFQ+XZ5ZJrGS/ep/u/rF6l+dKZh5vVIRs= github.com/mulgadc/predastore v1.14.1-0.20260730221229-608939ef2bb4 h1:Q4/NEryYTMiALkFwbPPOrKpNgLir+d7T2hbyRpAFnWc= github.com/mulgadc/predastore v1.14.1-0.20260730221229-608939ef2bb4/go.mod h1:na0aNXABZF03/HfFWCrAPHrId/2FIEIUQfB82vlu1xg= -github.com/mulgadc/viperblock v1.14.1-0.20260801223811-8a4787ade83a h1:7HZ8K0hpA0tuExkWgcNmUuK1ZH7ZvjNpVsFvKLvOMrE= -github.com/mulgadc/viperblock v1.14.1-0.20260801223811-8a4787ade83a/go.mod h1:5ENQb0OrjTplIRiLShRW/oNWOzbFvSCFgo+PkNH4858= +github.com/mulgadc/viperblock v1.14.1-0.20260802084514-ed52f6a0e4c0 h1:etjtOa6Sq21CtX+8RU4vYb5JBz+dExPLLym6xpXD4Mw= +github.com/mulgadc/viperblock v1.14.1-0.20260802084514-ed52f6a0e4c0/go.mod h1:5ENQb0OrjTplIRiLShRW/oNWOzbFvSCFgo+PkNH4858= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq1c1nUAm88MOHcQC9l5mIlSMApZMrHA= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ= github.com/mwitkow/go-conntrack v0.0.0-20161129095857-cc309e4a2223/go.mod h1:qRWi+5nqEBWmkhHvq77mSJWrCKwh8bxhgT7d/eI7P4U= diff --git a/spinifex/services/viperblockd/viperblockd.go b/spinifex/services/viperblockd/viperblockd.go index 8a4b31895..d5c1a06f8 100644 --- a/spinifex/services/viperblockd/viperblockd.go +++ b/spinifex/services/viperblockd/viperblockd.go @@ -253,12 +253,51 @@ func isAuxVolume(volumeName string) bool { return strings.HasSuffix(volumeName, "-efi") } +// validVolumeName reports whether volume is safe to use as a single path +// component under BaseDir: non-empty, no separator, and not "." or "..". +func validVolumeName(volume string) bool { + return volume != "" && volume != "." && volume != ".." && filepath.Base(volume) == volume +} + +// localVolumeDir validates volume and baseDir, then returns baseDir/volume. +// Every handler that turns a wire-supplied volume name into a filesystem path +// (mount, unmount, delete, config) must go through this: the name arrives +// unmarshalled straight from a NATS message, so an empty or ".."-laden value +// must never reach a filesystem call. The character checks in validVolumeName +// already rule out an escape, but the path is still resolved and checked +// against baseDir as a second, independent guard. +func localVolumeDir(baseDir, volume string) (string, error) { + if baseDir == "" { + return "", fmt.Errorf("empty base directory") + } + if !validVolumeName(volume) { + return "", fmt.Errorf("invalid volume name %q", volume) + } + + absBase, err := filepath.Abs(baseDir) + if err != nil { + return "", fmt.Errorf("resolve base directory: %w", err) + } + dir := filepath.Join(absBase, volume) + + rel, err := filepath.Rel(absBase, dir) + if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return "", fmt.Errorf("volume name %q escapes base directory", volume) + } + return dir, nil +} + // volumeNeedsSeal reports whether an unmounted volume has local viperblock // state under baseDir/ to flush. A node that never held the local // WAL has nothing to seal. Callers handle auxiliary volumes separately (see // isAuxVolume). func volumeNeedsSeal(volumeName, baseDir string) bool { - _, err := os.Stat(filepath.Join(baseDir, volumeName)) + dir, err := localVolumeDir(baseDir, volumeName) + if err != nil { + slog.Warn("volumeNeedsSeal: rejecting invalid volume name", "volume", volumeName, "err", err) + return false + } + _, err = os.Stat(dir) return err == nil } @@ -469,6 +508,17 @@ func launchService(cfg *Config) (err error) { return } + // Reject before touching anything: an empty or path-traversing name + // must never reach the RemoveAll below, which is otherwise wide + // enough to wipe BaseDir itself or escape it entirely. + localPath, err := localVolumeDir(cfg.BaseDir, ebsRequest.Volume) + if err != nil { + slog.ErrorContext(ctx, "ebs.delete: refusing invalid volume name", "volume", ebsRequest.Volume, "err", err) + utils.MarkSpanError(span, err) + respondJSON(msg, types.EBSDeleteResponse{Volume: ebsRequest.Volume, Error: fmt.Sprintf("invalid volume name: %v", err)}) + return + } + response := types.EBSDeleteResponse{Volume: ebsRequest.Volume, Success: true} // Find and clean up the mounted volume if it exists @@ -515,6 +565,20 @@ func launchService(cfg *Config) (err error) { slog.InfoContext(ctx, "ebs.delete: volume not mounted (expected for available volumes)", "volume", ebsRequest.Volume) } + // Delete is permanent: remove the on-disk WAL/checkpoint cache + // regardless of mount-tracking state. -efi volumes never go through + // the unmount seal (isAuxVolume skips it, they carry no durable + // data), and a main volume's seal can have been skipped after a + // failed flush (e.g. disk full), so this is the only guaranteed + // cleanup point for both. localPath was validated above. + if err := os.RemoveAll(localPath); err != nil { + slog.ErrorContext(ctx, "ebs.delete: failed to remove local volume directory", + "volume", ebsRequest.Volume, "path", localPath, "err", err) + } else { + slog.InfoContext(ctx, "ebs.delete: removed local volume directory", + "volume", ebsRequest.Volume, "path", localPath) + } + respondJSON(msg, response) }); err != nil { return fmt.Errorf("failed to subscribe to ebs.delete: %w", err) @@ -718,6 +782,16 @@ func launchService(cfg *Config) (err error) { return } + // Reject before the not-live branch below can hand this name to + // openLoadedVolumeVB, which derives a BaseDir/ path. + if !validVolumeName(req.Volume) { + err := fmt.Errorf("invalid volume name %q", req.Volume) + slog.ErrorContext(ctx, "ebs.config: refusing invalid volume name", "volume", req.Volume) + utils.MarkSpanError(span, err) + respondJSON(msg, types.EBSConfigUpdateResponse{Volume: req.Volume, Error: err.Error()}) + return + } + cfg.mu.Lock() var live *viperblock.VB for _, volume := range cfg.MountedVolumes { @@ -793,6 +867,18 @@ func launchService(cfg *Config) (err error) { slog.InfoContext(ctx, "ebs.mount", "request", ebsRequest) + // Reject before any local path is derived from the name: this is the + // first point an unvalidated wire name would otherwise reach the + // filesystem (clearStaleSealReceipt) and, further down, viperblock's + // own BaseDir/ layout. + if !validVolumeName(ebsRequest.Name) { + err := fmt.Errorf("invalid volume name %q", ebsRequest.Name) + slog.ErrorContext(ctx, "ebs.mount: refusing invalid volume name", "volume", ebsRequest.Name) + utils.MarkSpanError(span, err) + respondJSON(msg, types.EBSMountResponse{Error: err.Error()}) + return + } + // Clear any receipt left by a previous mount before anything else can // return early, so a stale receipt can never survive into this mount. clearStaleSealReceipt(cfg.BaseDir, ebsRequest.Name) diff --git a/spinifex/services/viperblockd/viperblockd_handlers_test.go b/spinifex/services/viperblockd/viperblockd_handlers_test.go index 15d4fc153..ad72827d0 100644 --- a/spinifex/services/viperblockd/viperblockd_handlers_test.go +++ b/spinifex/services/viperblockd/viperblockd_handlers_test.go @@ -125,6 +125,166 @@ func TestIntegration_EBSDeleteUnmountedVolume(t *testing.T) { assert.True(t, resp.Success) } +// TestIntegration_EBSDeleteRemovesLocalVolumeDirectory covers the residual +// leak: terminateCleanup always unmounts before DeleteVolumes sends +// ebs.delete, so the common case is an already-unmounted volume. Before the +// fix, ebs.delete only cleaned up a still-mounted volume's nbdkit +// process/socket and never touched cfg.BaseDir/, so the +// WAL/checkpoint directory a prior mount (or a failed unmount seal) left +// behind survived every DeleteVolume call — including for -efi companion +// volumes, which never go through the unmount seal at all (isAuxVolume skips +// it), so they leaked unconditionally rather than only on a failed seal. +func TestIntegration_EBSDeleteRemovesLocalVolumeDirectory(t *testing.T) { + t.Parallel() + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + + ns, natsURL := setupEmbeddedNATS(t) + defer ns.Shutdown() + + cfg := setupTestConfig(t, natsURL) + // No mounted volumes: matches the terminate path where Unmount already + // ran and left local state behind (a failed seal for the main volume, or + // unconditionally for the -efi companion) before ebs.delete fires. + createMockVolumeState(t, cfg.BaseDir, "vol-residual-test") + createMockVolumeState(t, cfg.BaseDir, "vol-residual-test-efi") + + go func() { launchService(cfg) }() + time.Sleep(500 * time.Millisecond) + + nc, err := nats.Connect(natsURL) + require.NoError(t, err) + defer nc.Close() + + for _, volume := range []string{"vol-residual-test", "vol-residual-test-efi"} { + reqData, _ := json.Marshal(types.EBSDeleteRequest{Volume: volume}) + msg, err := nc.Request("ebs.delete", reqData, 3*time.Second) + require.NoError(t, err) + + var resp types.EBSDeleteResponse + require.NoError(t, json.Unmarshal(msg.Data, &resp)) + assert.True(t, resp.Success) + + assert.False(t, fileExistsCheck(filepath.Join(cfg.BaseDir, volume)), + "ebs.delete must remove the local volume directory for %s", volume) + } +} + +// TestIntegration_EBSDeleteEmptyVolumeNameDoesNotWipeBaseDir is the +// regression test for the data-destruction hazard: before validation, +// filepath.Join(cfg.BaseDir, "") collapsed to cfg.BaseDir itself, so an empty +// Volume made ebs.delete os.RemoveAll the node's entire local WAL/checkpoint +// cache. This must be rejected, and BaseDir must survive. +func TestIntegration_EBSDeleteEmptyVolumeNameDoesNotWipeBaseDir(t *testing.T) { + t.Parallel() + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + + ns, natsURL := setupEmbeddedNATS(t) + defer ns.Shutdown() + + cfg := setupTestConfig(t, natsURL) + createMockVolumeState(t, cfg.BaseDir, "vol-survivor") + + go func() { launchService(cfg) }() + time.Sleep(500 * time.Millisecond) + + nc, err := nats.Connect(natsURL) + require.NoError(t, err) + defer nc.Close() + + reqData, _ := json.Marshal(types.EBSDeleteRequest{Volume: ""}) + msg, err := nc.Request("ebs.delete", reqData, 3*time.Second) + require.NoError(t, err) + + var resp types.EBSDeleteResponse + require.NoError(t, json.Unmarshal(msg.Data, &resp)) + assert.False(t, resp.Success, "an empty volume name must not report success") + assert.NotEmpty(t, resp.Error) + + assert.True(t, fileExistsCheck(cfg.BaseDir), "BaseDir itself must survive an empty volume name") + assert.True(t, fileExistsCheck(filepath.Join(cfg.BaseDir, "vol-survivor")), + "an unrelated volume directory must survive an empty volume name") +} + +// TestIntegration_EBSDeleteRejectsPathTraversal covers names that would +// otherwise let ebs.delete escape BaseDir via filepath.Join's ".." cleaning. +// Both cases must be refused before any RemoveAll runs, and Success must not +// come back true for a request that was refused. +func TestIntegration_EBSDeleteRejectsPathTraversal(t *testing.T) { + t.Parallel() + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + + ns, natsURL := setupEmbeddedNATS(t) + defer ns.Shutdown() + + cfg := setupTestConfig(t, natsURL) + createMockVolumeState(t, cfg.BaseDir, "vol-survivor") + + go func() { launchService(cfg) }() + time.Sleep(500 * time.Millisecond) + + nc, err := nats.Connect(natsURL) + require.NoError(t, err) + defer nc.Close() + + for _, name := range []string{"../..", "a/b"} { + reqData, _ := json.Marshal(types.EBSDeleteRequest{Volume: name}) + msg, err := nc.Request("ebs.delete", reqData, 3*time.Second) + require.NoError(t, err) + + var resp types.EBSDeleteResponse + require.NoError(t, json.Unmarshal(msg.Data, &resp)) + assert.False(t, resp.Success, "volume name %q must not report success", name) + assert.NotEmpty(t, resp.Error, "volume name %q must return an error", name) + } + + assert.True(t, fileExistsCheck(filepath.Join(cfg.BaseDir, "vol-survivor")), + "an unrelated volume directory must survive a path-traversal attempt") +} + +// TestIntegration_EBSDeleteValidNameLeavesSiblingsUntouched is the essential +// non-regression check for the new validation: a legitimate delete must still +// remove exactly its own directory, and nothing else under BaseDir. +func TestIntegration_EBSDeleteValidNameLeavesSiblingsUntouched(t *testing.T) { + t.Parallel() + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + + ns, natsURL := setupEmbeddedNATS(t) + defer ns.Shutdown() + + cfg := setupTestConfig(t, natsURL) + createMockVolumeState(t, cfg.BaseDir, "vol-target") + createMockVolumeState(t, cfg.BaseDir, "vol-sibling") + + go func() { launchService(cfg) }() + time.Sleep(500 * time.Millisecond) + + nc, err := nats.Connect(natsURL) + require.NoError(t, err) + defer nc.Close() + + reqData, _ := json.Marshal(types.EBSDeleteRequest{Volume: "vol-target"}) + msg, err := nc.Request("ebs.delete", reqData, 3*time.Second) + require.NoError(t, err) + + var resp types.EBSDeleteResponse + require.NoError(t, json.Unmarshal(msg.Data, &resp)) + assert.True(t, resp.Success) + assert.Empty(t, resp.Error) + + assert.False(t, fileExistsCheck(filepath.Join(cfg.BaseDir, "vol-target")), + "the targeted volume directory must be removed") + assert.True(t, fileExistsCheck(filepath.Join(cfg.BaseDir, "vol-sibling")), + "a sibling volume directory must survive deleting a different volume") +} + func TestIntegration_EBSDeleteInvalidJSON(t *testing.T) { t.Parallel() if testing.Short() { diff --git a/spinifex/services/viperblockd/viperblockd_test.go b/spinifex/services/viperblockd/viperblockd_test.go index a64ee70c6..9fd10c18d 100644 --- a/spinifex/services/viperblockd/viperblockd_test.go +++ b/spinifex/services/viperblockd/viperblockd_test.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "os" + "path/filepath" "sync" "testing" "time" @@ -512,3 +513,62 @@ func TestRetryLoadState(t *testing.T) { assert.Equal(t, 225*time.Millisecond, lastDelay) }) } + +// TestValidVolumeName covers the character-level gate every handler that +// turns a wire volume name into a filesystem path must pass first. +func TestValidVolumeName(t *testing.T) { + valid := []string{"vol-0123456789abcdef0", "vol-0123456789abcdef0-efi", "a"} + for _, name := range valid { + assert.True(t, validVolumeName(name), "expected %q to be valid", name) + } + + invalid := []string{"", ".", "..", "../..", "a/b", "/etc/passwd", "vol/../.."} + for _, name := range invalid { + assert.False(t, validVolumeName(name), "expected %q to be rejected", name) + } +} + +// TestLocalVolumeDir is the regression test for the ebs.delete data-destruction +// hazard: an unvalidated volume name reaching os.RemoveAll could wipe BaseDir +// itself (empty name) or escape it entirely ("../.."). Every case here must be +// rejected before a caller ever gets a path back to remove. +func TestLocalVolumeDir(t *testing.T) { + baseDir := t.TempDir() + + t.Run("empty_volume_rejected", func(t *testing.T) { + _, err := localVolumeDir(baseDir, "") + require.Error(t, err, "an empty volume must never resolve to BaseDir itself") + }) + + t.Run("dot_rejected", func(t *testing.T) { + _, err := localVolumeDir(baseDir, ".") + require.Error(t, err) + }) + + t.Run("parent_traversal_rejected", func(t *testing.T) { + for _, name := range []string{"..", "../..", "../../etc"} { + _, err := localVolumeDir(baseDir, name) + require.Error(t, err, "volume %q must not escape BaseDir", name) + } + }) + + t.Run("embedded_separator_rejected", func(t *testing.T) { + for _, name := range []string{"a/b", "a/../../b", "/abs/path"} { + _, err := localVolumeDir(baseDir, name) + require.Error(t, err, "volume %q must not contain a path separator", name) + } + }) + + t.Run("empty_base_dir_rejected", func(t *testing.T) { + _, err := localVolumeDir("", "vol-abc123") + require.Error(t, err) + }) + + t.Run("valid_name_resolves_under_base_dir", func(t *testing.T) { + dir, err := localVolumeDir(baseDir, "vol-abc123") + require.NoError(t, err) + absBase, err := filepath.Abs(baseDir) + require.NoError(t, err) + assert.Equal(t, filepath.Join(absBase, "vol-abc123"), dir) + }) +} diff --git a/spinifex/vm/shutdown.go b/spinifex/vm/shutdown.go index 66b02d177..e93745811 100644 --- a/spinifex/vm/shutdown.go +++ b/spinifex/vm/shutdown.go @@ -240,8 +240,21 @@ func (m *Manager) finalizeTerminated(instance *VM) error { // instance that was never inserted into the local map. m.Inspect(instance, func(v *VM) { v.LastNode = m.deps.NodeID }) - if err := m.transitionWithPrecheck(instance, StateTerminated); err != nil { - return fmt.Errorf("transition to terminated: %w", err) + transitionErr := m.transitionWithPrecheck(instance, StateTerminated) + if transitionErr != nil && errors.Is(transitionErr, ErrInvalidTransition) { + // A genuine invalid/raced transition: in-memory status never reached + // terminated, so there is nothing durable to record yet. + return fmt.Errorf("transition to terminated: %w", transitionErr) + } + if transitionErr != nil { + // In-memory status reached terminated even though local persistence + // failed (e.g. ENOSPC writing the local state file). Keep going: + // WriteTerminatedInstance below is a JetStream KV write independent + // of local disk space, so the durable record can still land and + // TerminatedTeardownReaper picks it up on its next sweep without + // requiring an operator restart. + slog.Warn("Local state persistence failed on terminate, continuing to durable KV write", + "instanceId", instance.ID, "err", transitionErr) } // Stamp the termination time so the GC backstop can preserve a @@ -254,6 +267,9 @@ func (m *Manager) finalizeTerminated(instance *VM) error { if err := m.deps.StateStore.WriteTerminatedInstance(instance.ID, instance); err != nil { slog.Error("Failed to write terminated instance to KV, keeping in local state for retry", "instanceId", instance.ID, "err", err) + if transitionErr != nil { + return transitionErr + } return err } } @@ -276,7 +292,7 @@ func (m *Manager) finalizeTerminated(instance *VM) error { } slog.Info("Released instance ownership to KV", "instanceId", instance.ID, "state", string(StateTerminated), "lastNode", m.deps.NodeID) - return nil + return transitionErr } // reconcileVanishedQEMU finalizes a shutting-down instance whose QEMU process diff --git a/spinifex/vm/shutdown_test.go b/spinifex/vm/shutdown_test.go index f1c9c1163..c7141215f 100644 --- a/spinifex/vm/shutdown_test.go +++ b/spinifex/vm/shutdown_test.go @@ -1018,6 +1018,55 @@ func TestTerminate_WriteTerminatedFailure_PropagatesAndKeepsLocal(t *testing.T) assert.True(t, stillInMap, "instance must remain in local map for retry on the next daemon restart") } +// TestTerminate_LocalPersistenceFailure_StillDurablyRecordsTerminated covers +// the ENOSPC scenario: TransitionState fails to persist locally (e.g. the +// local state file write hits a full disk) but the in-memory status still +// reaches StateTerminated. Before the fix, finalizeTerminated bailed out on +// that error before ever calling WriteTerminatedInstance, so the durable +// terminated record TerminatedTeardownReaper scans was never written and +// nothing retried teardown without an operator restart. The durable KV write +// (a JetStream call independent of local disk space) must still happen. +func TestTerminate_LocalPersistenceFailure_StillDurablyRecordsTerminated(t *testing.T) { + t.Setenv("XDG_RUNTIME_DIR", t.TempDir()) + persistErr := errors.New("no space left on device") + store := newFakeStateStore() + cleaner := &recordingInstanceCleaner{} + down := &atomic.Int64{} + m := NewManager() + m.SetDeps(Deps{ + NodeID: "test-node", + StateStore: store, + VolumeMounter: &fakeVolumeMounter{}, + InstanceCleaner: cleaner, + TransitionState: func(v *VM, target InstanceState) error { + // Mirrors daemon.TransitionState: memory mutation always + // succeeds; only the local file persistence fails, matching a + // full disk on the terminated transition specifically. + m.Inspect(v, func(vv *VM) { vv.Status = target }) + if target == StateTerminated { + return persistErr + } + return nil + }, + ShutdownSignal: func() bool { return false }, + Hooks: ManagerHooks{ + OnInstanceDown: func(id string) { down.Add(1) }, + }, + }) + + v := &VM{ID: "i-local-persist-fail", Status: StateRunning, InstanceType: "t3.micro", Instance: &ec2.Instance{}} + m.Insert(v) + + err := m.Terminate(v.ID) + + require.ErrorIs(t, err, persistErr, "the local persistence failure must still surface to the caller") + require.NotNil(t, store.terminated[v.ID], + "WriteTerminatedInstance must still run so TerminatedTeardownReaper can find and finish this instance") + assert.Equal(t, int64(1), down.Load(), "OnInstanceDown must still fire once the durable record lands") + _, stillInMap := m.Get(v.ID) + assert.False(t, stillInMap, "instance must leave the local running map once terminated is durable") +} + // TestTerminate_DeleteIfReclaimed_NoHook covers the slot-reclaim branch // in finalizeTerminated (shutdown.go:197-201): a concurrent handler // inserts a fresh VM under the same id between WriteTerminatedInstance