fix(gpu): treat an already-released GPU as success during teardown (#749)

Release reported "nothing was claimed" as an error, and the terminated-instance
teardown reaper persisted that as the gpu teardown mark. The mark could then
never reach done, so the record was never purged and the GC re-drove it every
two minutes for as long as it existed, failing identically each time because
the condition being retried was that the work had already been done.

Stop-then-terminate is ordinary: the stop releases the GPU and the later
terminate finds nothing left to release. Two instances on wattle had been in
this state for days, at 2251 and 640 retries, logging an ERROR naming the
node's only GPU and falsely claiming it might need a manual rebind.

Release now returns a matchable ErrNoGPUClaimed and the teardown cleaner reads
it as success, which is what the reaper already documents its cleaners to do.
The sentinel stays an error rather than becoming nil so the launch-rollback
path can still tell that its claim/release pair is out of step, and so a real
vfio rebind failure - which clears Available, the health signal the Bedrock
capacity gate reads - stays distinct from the benign case.

spinifex/daemon/vm_adapters.go

@@ -10,6 +10,7 @@ import (
 	"time"
 
 	"github.com/mulgadc/spinifex/spinifex/awserrors"
+	"github.com/mulgadc/spinifex/spinifex/gpu"
 	handlers_ec2_placementgroup "github.com/mulgadc/spinifex/spinifex/handlers/ec2/placementgroup"
 	"github.com/mulgadc/spinifex/spinifex/network/topology"
 	"github.com/mulgadc/spinifex/spinifex/tags"
@@ -880,11 +881,21 @@ func (a *instanceCleanerAdapter) RemoveFromSpotRequest(instance *vm.VM) error {
 // ReleaseGPU unbinds the instance's GPU from vfio-pci and rebinds to its
 // original host driver. No-op for instances without a GPU allocation or
 // when GPU passthrough is disabled.
+//
+// Holding no device is success, not failure: teardown re-drives this until it
+// reports done, and a stop that already released leaves nothing for the later
+// terminate to do. Returning an error there wedges the record forever.
 func (a *instanceCleanerAdapter) ReleaseGPU(instance *vm.VM) error {
 	if a.d.gpuManager == nil || len(instance.GPUAttachments) == 0 {
 		return nil
 	}
-	if err := a.d.gpuManager.Release(instance.ID); err != nil {
+	err := a.d.gpuManager.Release(instance.ID)
+	switch {
+	case errors.Is(err, gpu.ErrNoGPUClaimed):
+		slog.Info("GPU already released, nothing to do",
+			"gpus", instance.GPUAttachments, "instanceId", instance.ID)
+		return nil
+	case err != nil:
 		slog.Error("Failed to release GPU on stop, device may need manual rebind",
 			"gpus", instance.GPUAttachments, "instanceId", instance.ID, "err", err)
 		return err

spinifex/daemon/vm_adapters_test.go

@@ -368,15 +368,22 @@ func TestReleaseGPU_NoAddresses_NoOp(t *testing.T) {
 	a.ReleaseGPU(instance)
 }
 
-// ReleaseGPU logs a warning when the manager returns an error (instance not claimed).
-func TestReleaseGPU_ManagerError_LogsWarning(t *testing.T) {
+// TestReleaseGPU_AlreadyReleased_IsSuccess pins the fix for a teardown record
+// that could never complete. A stop releases the GPU, so the later terminate
+// finds no claim; reporting that as failure left the gpu teardown mark stuck
+// short of done, so the record was never purged and the GC re-drove it every
+// two minutes indefinitely, failing identically each time.
+func TestReleaseGPU_AlreadyReleased_IsSuccess(t *testing.T) {
 	mgr := gpu.NewManager(nil)
 	d := &Daemon{gpuManager: mgr}
 	a := newInstanceCleanerAdapter(d)
-	// GPU address set but no claim registered — Release returns an error.
+	// GPU address set but no claim registered — the shape a re-driven release
+	// sees once the work is already done.
 	instance := &vm.VM{ID: "i-unclaimed", GPUAttachments: []gpu.GPUAttachment{{PCIAddress: "0000:03:00.0"}}}
-	// Must not panic; the error is logged as a warning.
-	a.ReleaseGPU(instance)
+
+	require.NoError(t, a.ReleaseGPU(instance), "an already-released GPU must not fail teardown")
+	// Idempotent under repetition, which is what the reaper does to it.
+	require.NoError(t, a.ReleaseGPU(instance))
 }
 
 // --- RemoveFromSpotRequest ---

spinifex/gpu/manager.go

@@ -8,6 +8,11 @@ import (
 	"sync"
 )
 
+// ErrNoGPUClaimed reports that the instance held no device when Release ran.
+// Teardown re-drives releases and should read this as success, since the state
+// it wants already holds; a claim/release pair that hits it is out of step.
+var ErrNoGPUClaimed = errors.New("no GPU claimed by instance")
+
 type gpuEntry struct {
 	Device        GPUDevice
 	MIGInstance   *MIGInstance       // nil = whole-GPU passthrough; non-nil = MIG slice
@@ -213,6 +218,10 @@ func (m *Manager) Claim(instanceID, profileName string) (*GPUDevice, *MIGInstanc
 // (already vfio-pci before the daemon) are left bound; Claim-bound devices are
 // unbound and rebind to their original driver (failure marks them unavailable).
 // For dynamically carved MIG GPUs, the last release destroys all slices.
+//
+// Holding nothing yields ErrNoGPUClaimed, which is a distinct condition rather
+// than a fault: it says the desired state already holds. Callers that re-drive
+// releases decide for themselves whether that counts as success.
 func (m *Manager) Release(instanceID string) error {
 	m.mu.Lock()
 	defer m.mu.Unlock()
@@ -269,7 +278,7 @@ func (m *Manager) Release(instanceID string) error {
 	}
 
 	if !released {
-		return fmt.Errorf("no GPU claimed by instance %s", instanceID)
+		return fmt.Errorf("%w %s", ErrNoGPUClaimed, instanceID)
 	}
 
 	// For dynamically carved MIG GPUs: destroy all slices when the last instance

spinifex/gpu/manager_mig_test.go

@@ -153,6 +153,7 @@ func TestMIGRelease_UnknownInstance_ReturnsError(t *testing.T) {
 	m := NewManager(nil)
 	err := m.Release("i-never-claimed")
 	require.Error(t, err)
+	require.ErrorIs(t, err, ErrNoGPUClaimed)
 }
 
 // --- MarkMIGFailed ---

spinifex/gpu/manager_test.go

@@ -1,8 +1,10 @@
 package gpu
 
 import (
+	"errors"
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 )
 
@@ -151,7 +153,15 @@ func TestManagerRelease_UnknownInstance(t *testing.T) {
 
 	err := m.Release("i-does-not-exist")
 	if err == nil {
-		t.Error("want error releasing unknown instance, got nil")
+		t.Fatal("want error releasing unknown instance, got nil")
+	}
+	// Matchable rather than a bare message: teardown re-drives releases and has
+	// to tell "already released" from a rebind failure without reading text.
+	if !errors.Is(err, ErrNoGPUClaimed) {
+		t.Errorf("want ErrNoGPUClaimed, got %v", err)
+	}
+	if !strings.Contains(err.Error(), "i-does-not-exist") {
+		t.Errorf("want the instance ID in the message, got %q", err)
 	}
 }
 
@@ -181,6 +191,12 @@ func TestManagerRelease_FailureMarksUnavailable(t *testing.T) {
 	if err == nil {
 		t.Error("want error when vfio-pci unbind fails, got nil")
 	}
+	// Must stay distinct from the benign already-released case, which callers
+	// are entitled to ignore. This one clears Available, the health signal the
+	// Bedrock capacity gate reads.
+	if errors.Is(err, ErrNoGPUClaimed) {
+		t.Errorf("a rebind failure must not report as ErrNoGPUClaimed, got %v", err)
+	}
 
 	// Pool entry must be marked unavailable so the broken GPU isn't re-offered.
 	if m.Available() != 0 {