fix(vm): report ENOSPC to the guest instead of pausing the VM, and let the stuck-terminate backstop fire (#626)

* fix(vm): report ENOSPC to the guest on data volumes instead of pausing the VM

QEMU's default werror=enospc pauses the whole VM when a data volume's
backend runs out of space. The boot drive already avoids this via
werror=report,rerror=report; data volumes had no on-error policy at
all in either the cold-boot -device or the hotplug device_add.

Set werror=report,rerror=report on the virtio-blk-pci device itself,
since a data volume's blockdev-add NBD node has no BlockBackend to
carry it the way -drive does for the boot drive.

* fix(vm): stamp ShuttingDownAt even when the state transition write fails

transitionWithPrecheck returned before stampShuttingDownAt whenever
TransitionState failed, even though the in-memory status had already
moved to the target. On a full disk this leaves ShuttingDownAt zero,
and StuckTerminateReaper.Sweep skips any VM whose ShuttingDownAt is
zero — so the backstop meant to bound a wedged terminate is disabled
by exactly the condition it exists to catch.

Stamp on the persistence-failure branch too, using the same helper
and locking as the success path, while still returning the original
error to the caller. The raced branch (TransitionState failed and the
in-memory status never reached target) is left unstamped, since the
target state was never actually entered there.

spinifex/vm/devices.go

@@ -105,6 +105,11 @@ func HotplugEBSBus(port int) string {
 // AttachVolume's QMP device_add (rendered as a map) and buildDrives' -device
 // (rendered as a joined command-line string) so the two block-graph shapes
 // cannot diverge.
+//
+// werror/rerror mirror the boot drive's on-error policy (see Drive.Werror):
+// a data volume has no -drive-backed BlockBackend to carry it, so the qdev
+// device itself sets werror=report,rerror=report to report backend ENOSPC to
+// the guest instead of letting QEMU's default werror=enospc pause the VM.
 func VolumeBlkDeviceArgs(volumeID, nodeName, iothreadID, bus string) []string {
 	return []string{
 		fmt.Sprintf("id=%s", VolumeDeviceID(volumeID)),
@@ -112,6 +117,8 @@ func VolumeBlkDeviceArgs(volumeID, nodeName, iothreadID, bus string) []string {
 		fmt.Sprintf("iothread=%s", iothreadID),
 		fmt.Sprintf("serial=%s", VolumeSerial(volumeID)),
 		fmt.Sprintf("bus=%s", bus),
+		"werror=report",
+		"rerror=report",
 	}
 }
 
@@ -123,7 +130,8 @@ func VolumeBlkDevice(volumeID, nodeName, iothreadID, bus string) Device {
 }
 
 // VolumeBlkDeviceQMPArgs renders the same virtio-blk-pci arguments as a
-// device_add argument map for AttachVolume's QMP hot-attach path.
+// device_add argument map for AttachVolume's QMP hot-attach path. See
+// VolumeBlkDeviceArgs for why werror/rerror are set here.
 func VolumeBlkDeviceQMPArgs(volumeID, nodeName, iothreadID, bus string) map[string]any {
 	return map[string]any{
 		"driver":   "virtio-blk-pci",
@@ -132,6 +140,8 @@ func VolumeBlkDeviceQMPArgs(volumeID, nodeName, iothreadID, bus string) map[stri
 		"iothread": iothreadID,
 		"serial":   VolumeSerial(volumeID),
 		"bus":      bus,
+		"werror":   "report",
+		"rerror":   "report",
 	}
 }
 

spinifex/vm/devices_test.go

@@ -84,3 +84,20 @@ func TestRngDevice_MMIO_WithOptions(t *testing.T) {
 	d := RngDevice("microvm,x-option-roms=off,pic=off")
 	assert.Equal(t, "virtio-rng-device", d.Value)
 }
+
+// TestVolumeBlkDevice_OnErrorReport pins the cold-boot data-volume device
+// string to werror=report,rerror=report, mirroring the boot drive's existing
+// on-error policy so a full backend reports ENOSPC to the guest instead of
+// pausing the VM.
+func TestVolumeBlkDevice_OnErrorReport(t *testing.T) {
+	d := VolumeBlkDevice("vol-data-a", "nbd-vol-data-a", "ioth-vol-data-a", "hotplug-ebs3")
+	assert.Equal(t, "virtio-blk-pci,id=vdisk-vol-data-a,drive=nbd-vol-data-a,iothread=ioth-vol-data-a,serial=voldataa,bus=hotplug-ebs3,werror=report,rerror=report", d.Value)
+}
+
+// TestVolumeBlkDeviceQMPArgs_OnErrorReport is the hotplug counterpart: the
+// QMP device_add argument map AttachVolume sends must carry the same policy.
+func TestVolumeBlkDeviceQMPArgs_OnErrorReport(t *testing.T) {
+	args := VolumeBlkDeviceQMPArgs("vol-data-a", "nbd-vol-data-a", "ioth-vol-data-a", "hotplug-ebs3")
+	assert.Equal(t, "report", args["werror"])
+	assert.Equal(t, "report", args["rerror"])
+}

spinifex/vm/lifecycle_test.go

@@ -171,7 +171,7 @@ func TestBuildDrives(t *testing.T) {
 				{Value: "driver=nbd,node-name=nbd-vol-data-a,server.type=unix,server.path=/tmp/data-a.sock,export="},
 			},
 			wantDevices: []Device{
-				{Value: "virtio-blk-pci,id=vdisk-vol-data-a,drive=nbd-vol-data-a,iothread=ioth-vol-data-a,serial=voldataa,bus=hotplug-ebs3"},
+				{Value: "virtio-blk-pci,id=vdisk-vol-data-a,drive=nbd-vol-data-a,iothread=ioth-vol-data-a,serial=voldataa,bus=hotplug-ebs3,werror=report,rerror=report"},
 			},
 			wantHotplugPorts: []int{3},
 		},
@@ -186,7 +186,7 @@ func TestBuildDrives(t *testing.T) {
 				{Value: "driver=nbd,node-name=nbd-vol-data-b,server.type=unix,server.path=/tmp/data-b.sock,export="},
 			},
 			wantDevices: []Device{
-				{Value: "virtio-blk-pci,id=vdisk-vol-data-b,drive=nbd-vol-data-b,iothread=ioth-vol-data-b,serial=voldatab,bus=hotplug-ebs1"},
+				{Value: "virtio-blk-pci,id=vdisk-vol-data-b,drive=nbd-vol-data-b,iothread=ioth-vol-data-b,serial=voldatab,bus=hotplug-ebs1,werror=report,rerror=report"},
 			},
 			wantHotplugPorts: []int{1},
 		},
@@ -206,8 +206,8 @@ func TestBuildDrives(t *testing.T) {
 				{Value: "driver=nbd,node-name=nbd-vol-data-d,server.type=unix,server.path=/tmp/data-d.sock,export="},
 			},
 			wantDevices: []Device{
-				{Value: "virtio-blk-pci,id=vdisk-vol-data-c,drive=nbd-vol-data-c,iothread=ioth-vol-data-c,serial=voldatac,bus=hotplug-ebs1"},
-				{Value: "virtio-blk-pci,id=vdisk-vol-data-d,drive=nbd-vol-data-d,iothread=ioth-vol-data-d,serial=voldatad,bus=hotplug-ebs2"},
+				{Value: "virtio-blk-pci,id=vdisk-vol-data-c,drive=nbd-vol-data-c,iothread=ioth-vol-data-c,serial=voldatac,bus=hotplug-ebs1,werror=report,rerror=report"},
+				{Value: "virtio-blk-pci,id=vdisk-vol-data-d,drive=nbd-vol-data-d,iothread=ioth-vol-data-d,serial=voldatad,bus=hotplug-ebs2,werror=report,rerror=report"},
 			},
 			wantHotplugPorts: []int{1, 2},
 		},
@@ -247,7 +247,7 @@ func TestBuildDrives(t *testing.T) {
 			},
 			wantDevices: []Device{
 				{Value: "virtio-blk-pci,drive=os,iothread=ioth-os,num-queues=4,bootindex=1"},
-				{Value: "virtio-blk-pci,id=vdisk-vol-data-a,drive=nbd-vol-data-a,iothread=ioth-vol-data-a,serial=voldataa,bus=hotplug-ebs3"},
+				{Value: "virtio-blk-pci,id=vdisk-vol-data-a,drive=nbd-vol-data-a,iothread=ioth-vol-data-a,serial=voldataa,bus=hotplug-ebs3,werror=report,rerror=report"},
 			},
 			wantHotplugPorts: []int{0, 0, 3},
 		},

spinifex/vm/shutdown.go

@@ -551,6 +551,10 @@ func (m *Manager) transitionWithPrecheck(instance *VM, target InstanceState) err
 			return fmt.Errorf("%w: %s -> %s for instance %s (raced)",
 				ErrInvalidTransition, current, target, instance.ID)
 		}
+		// The in-memory status reached target even though persistence
+		// failed, so the stamp must still land: it is what lets the
+		// stuck-terminate backstop see and eventually bound this instance.
+		m.Inspect(instance, func(v *VM) { stampShuttingDownAt(v, target) })
 		return err
 	}
 	m.Inspect(instance, func(v *VM) { stampShuttingDownAt(v, target) })

spinifex/vm/shutdown_test.go

@@ -765,6 +765,59 @@ func TestTransitionWithPrecheck_PersistenceFailure_PassesThroughError(t *testing
 		"persistence-failure branch implies status reached target before error returned")
 }
 
+// TestTransitionWithPrecheck_PersistenceFailure_StampsShuttingDownAt covers
+// the specific target that matters for the stuck-terminate backstop: when
+// TransitionState fails but the in-memory status still reached
+// StateShuttingDown, ShuttingDownAt must be stamped anyway so
+// StuckTerminateReaper.Sweep can eventually see and bound this instance.
+// Before the fix this never ran because the function returned before
+// reaching the stamp call.
+func TestTransitionWithPrecheck_PersistenceFailure_StampsShuttingDownAt(t *testing.T) {
+	persistErr := errors.New("no space left on device")
+	var m *Manager
+	m = NewManagerWithDeps(Deps{
+		TransitionState: func(v *VM, target InstanceState) error {
+			// Memory mutation succeeded; only persistence (e.g. a full disk
+			// writing local state) failed.
+			m.Inspect(v, func(vv *VM) { vv.Status = target })
+			return persistErr
+		},
+	})
+
+	instance := &VM{ID: "i-persist-fail-shutdown", Status: StateRunning}
+	m.Insert(instance)
+
+	err := m.transitionWithPrecheck(instance, StateShuttingDown)
+
+	require.ErrorIs(t, err, persistErr, "the persistence error must still reach the caller")
+	assert.False(t, instance.ShuttingDownAt.IsZero(),
+		"ShuttingDownAt must be stamped even though the state write failed, so the reaper can see this instance")
+}
+
+// TestTransitionWithPrecheck_Raced_DoesNotStampShuttingDownAt is the
+// contrast: when TransitionState fails AND the in-memory status did not
+// reach StateShuttingDown (a concurrent transition beat this call), the
+// target state was never actually entered, so stamping would be a lie.
+func TestTransitionWithPrecheck_Raced_DoesNotStampShuttingDownAt(t *testing.T) {
+	persistErr := errors.New("kv put failed")
+	m := NewManagerWithDeps(Deps{
+		TransitionState: func(_ *VM, _ InstanceState) error {
+			// Return error without mutating status, matching a concurrent
+			// transition that beat this call.
+			return persistErr
+		},
+	})
+
+	instance := &VM{ID: "i-raced-shutdown", Status: StateRunning}
+	m.Insert(instance)
+
+	err := m.transitionWithPrecheck(instance, StateShuttingDown)
+
+	require.ErrorIs(t, err, ErrInvalidTransition)
+	assert.True(t, instance.ShuttingDownAt.IsZero(),
+		"the raced branch never actually entered StateShuttingDown, so it must not stamp")
+}
+
 // TestTransitionWithPrecheck_InvalidInitialTransition_WrapsErrInvalidTransition
 // covers the static precheck rejecting an illegal transition before
 // TransitionState is ever called. The error must wrap

spinifex/vm/stuck_terminate_reaper_test.go

@@ -101,6 +101,58 @@ func TestStuckTerminateReaper(t *testing.T) {
 		assert.True(t, ok, "the instance must stay in the running map")
 	})
 
+	t.Run("force-completes a VM whose ShuttingDownAt was only stamped by the persistence-failure path", func(t *testing.T) {
+		// This is the case the backstop existed to cover: on a full disk the
+		// state write fails, but transitionWithPrecheck must still stamp
+		// ShuttingDownAt so this VM is not skipped forever.
+		t.Setenv("XDG_RUNTIME_DIR", t.TempDir())
+		cleaner := &recordingInstanceCleaner{}
+		store := newFakeStateStore()
+		persistErr := errors.New("no space left on device")
+		var callCount int
+		var m *Manager
+		m = NewManagerWithDeps(Deps{
+			NodeID:          "test-node",
+			StateStore:      store,
+			InstanceCleaner: cleaner,
+			// Only the first call (the shutting-down transition below) fails,
+			// modelling the disk being full at that moment; the reaper's own
+			// later transition to terminated succeeds normally.
+			TransitionState: func(v *VM, target InstanceState) error {
+				m.Inspect(v, func(vv *VM) { vv.Status = target })
+				callCount++
+				if callCount == 1 {
+					return persistErr
+				}
+				return nil
+			},
+		})
+
+		const id = "i-wedged-persist-fail"
+		instance := &VM{ID: id, Status: StateRunning}
+		m.Insert(instance)
+
+		err := m.transitionWithPrecheck(instance, StateShuttingDown)
+		require.ErrorIs(t, err, persistErr, "the persistence error must still reach the caller")
+		require.False(t, instance.ShuttingDownAt.IsZero(),
+			"the failure path must stamp ShuttingDownAt for the reaper to ever see this instance")
+
+		// Back-date the stamp instead of sleeping so the reaper sees a
+		// timed-out VM without a real wait.
+		m.Inspect(instance, func(v *VM) {
+			v.ShuttingDownAt = time.Now().Add(-(stuckTerminateTimeout + time.Minute))
+		})
+
+		reaper := m.NewStuckTerminateReaper()
+		reaped, err := reaper.Sweep(context.Background())
+		require.NoError(t, err)
+		assert.Equal(t, 1, reaped,
+			"a VM stamped only via the persistence-failure path must still be force-completed once past the timeout")
+
+		_, ok := m.Get(id)
+		assert.False(t, ok, "the finalized instance must leave the local running map")
+	})
+
 	t.Run("a shutting-down instance with no timestamp is never force-completed", func(t *testing.T) {
 		cleaner := &recordingInstanceCleaner{}
 		reaper, _ := newStuckTerminateReaper(t, cleaner)

spinifex/vm/vm_test.go

@@ -266,7 +266,7 @@ func TestExecute_Blockdev_UnixServer(t *testing.T) {
 	}
 	require.Greater(t, deviceIdx, -1)
 	assert.Greater(t, deviceIdx, blockdevIdx, "-blockdev must precede the -device referencing it")
-	assert.Equal(t, "virtio-blk-pci,id=vdisk-vol-data-a,drive=nbd-vol-data-a,iothread=ioth-vol-data-a,serial=voldataa,bus=hotplug-ebs3", args[deviceIdx+1])
+	assert.Equal(t, "virtio-blk-pci,id=vdisk-vol-data-a,drive=nbd-vol-data-a,iothread=ioth-vol-data-a,serial=voldataa,bus=hotplug-ebs3,werror=report,rerror=report", args[deviceIdx+1])
 }
 
 // TestExecute_Blockdev_InetServer asserts the TCP NBD form renders