Wait for exiting VMM tasks and retain failed vGPU cleanup

lib/instances/delete.go

@@ -150,7 +150,8 @@ func (m *manager) deleteInstanceWithOptions(
 		log.InfoContext(ctx, "destroying vGPU", "instance_id", id, "uuid", stored.GPUMdevUUID)
 	}
 	if err := m.releaseStoredVGPUPersisted(ctx, meta); err != nil {
-		log.WarnContext(ctx, "failed to destroy vGPU, continuing with cleanup; the next allocation repairs the VF before reuse", "instance_id", id, "uuid", stored.GPUMdevUUID, "error", err)
+		log.ErrorContext(ctx, "failed to destroy vGPU; retaining instance metadata for retry", "instance_id", id, "uuid", stored.GPUMdevUUID, "error", err)
+		return fmt.Errorf("release vGPU: %w", err)
 	}
 
 	// 6. Release network allocation

lib/instances/lifecycle_noop_test.go

@@ -3,6 +3,7 @@ package instances
 import (
 	"context"
 	"errors"
+	"fmt"
 	"os"
 	"path/filepath"
 	"sync"
@@ -177,25 +178,59 @@ func TestDeletePersistsVGPUReleaseBeforeTeardown(t *testing.T) {
 	assert.Equal(t, restartpolicy.BlockedReasonManualStop, persisted.RestartStatus.BlockedReason)
 }
 
-func TestDeleteContinuesTeardownAfterFailedVGPURelease(t *testing.T) {
-	m, id := newLifecycleNoopManagerWithInstance(t, StateStopped, time.Now().UTC())
-	deviceManager := &recordingDeviceManager{}
-	m.deviceManager = deviceManager
-	meta, err := m.loadMetadata(id)
-	require.NoError(t, err)
-	meta.GPUProfile = "NVIDIA L40S-2Q"
-	meta.GPUFramework = devices.VGPUFramework("future-framework")
-	meta.GPUDevicePath = "/sys/bus/pci/devices/0000:82:00.4"
-	meta.Devices = []string{"dev-1"}
-	require.NoError(t, m.saveMetadata(meta))
-
-	// The failed release must not block the rest of the teardown: devices
-	// are detached and the instance is fully deleted.
-	require.NoError(t, m.DeleteInstance(context.Background(), id))
-	assert.Equal(t, []string{"dev-1"}, deviceManager.detached)
-
-	_, err = m.loadMetadata(id)
-	require.Error(t, err, "instance data must be deleted despite the failed release")
+func TestDeleteRetainsFailedVGPUReleaseForRetry(t *testing.T) {
+	releaseErr := errors.New("reset failed")
+	for _, reconcile := range []bool{false, true} {
+		t.Run(fmt.Sprintf("reconcile=%t", reconcile), func(t *testing.T) {
+			m, id := newLifecycleNoopManagerWithInstance(t, StateStopped, time.Now().UTC())
+			deviceManager := &recordingDeviceManager{}
+			m.deviceManager = deviceManager
+			m.destroyVGPU = func(context.Context, devices.VGPUAssignment) error { return releaseErr }
+			meta, err := m.loadMetadata(id)
+			require.NoError(t, err)
+			meta.RestartPolicy = &restartpolicy.Policy{Policy: restartpolicy.PolicyAlways}
+			meta.GPUProfile = "NVIDIA L40S-2Q"
+			meta.GPUFramework = devices.VGPUFrameworkVendorVFIO
+			meta.GPUDevicePath = "/sys/bus/pci/devices/0000:82:00.4"
+			meta.Devices = []string{"dev-1"}
+			require.NoError(t, m.saveMetadata(meta))
+
+			require.ErrorIs(t, m.DeleteInstance(t.Context(), id), releaseErr)
+			assert.Empty(t, deviceManager.detached)
+			retained, err := m.loadMetadata(id)
+			require.NoError(t, err)
+			assert.Equal(t, meta.GPUDevicePath, retained.GPUDevicePath)
+			assert.Equal(t, meta.GPUFramework, retained.GPUFramework)
+			assert.Equal(t, restartpolicy.BlockedReasonManualStop, retained.RestartStatus.BlockedReason)
+
+			if reconcile {
+				// A new manager must be able to recover the claim from disk.
+				restarted := &manager{
+					paths:                m.paths,
+					destroyVGPU:          m.destroyVGPU,
+					reconcileVGPUDevices: func(context.Context, map[string]struct{}) error { return nil },
+				}
+				restarted.ReconcileVGPUs(t.Context())
+				retained, err = restarted.loadMetadata(id)
+				require.NoError(t, err)
+				assert.Equal(t, meta.GPUDevicePath, retained.GPUDevicePath)
+				restarted.destroyVGPU = func(context.Context, devices.VGPUAssignment) error { return nil }
+				restarted.ReconcileVGPUs(t.Context())
+				retained, err = restarted.loadMetadata(id)
+				require.NoError(t, err)
+				assert.Empty(t, retained.GPUDevicePath)
+				assert.Equal(t, restartpolicy.BlockedReasonManualStop, retained.RestartStatus.BlockedReason)
+			} else {
+				m.destroyVGPU = func(context.Context, devices.VGPUAssignment) error { return nil }
+			}
+			// In the reconcile case m.destroyVGPU still fails; the retried delete
+			// succeeds because the claim was already cleared on disk.
+			require.NoError(t, m.DeleteInstance(t.Context(), id))
+			assert.Equal(t, []string{"dev-1"}, deviceManager.detached)
+			_, err = m.loadMetadata(id)
+			require.ErrorIs(t, err, ErrNotFound)
+		})
+	}
 }
 
 // A stale release during start must be persisted immediately: if start fails

lib/instances/process_exit_linux_test.go

@@ -0,0 +1,86 @@
+//go:build linux
+
+package instances
+
+import (
+	"bufio"
+	"fmt"
+	"os"
+	"os/exec"
+	"runtime"
+	"strconv"
+	"syscall"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+)
+
+func init() {
+	if os.Getenv("HYPEMAN_TEST_EXIT_LEADER") != "1" {
+		return
+	}
+	// init runs on the startup thread. Exit only that thread, leaving a worker
+	// alive to model a zombie QEMU leader with a vhost task still closing VFIO.
+	runtime.LockOSThread()
+	go func() {
+		fmt.Fprintln(os.Stdout, "ready")
+		_, _ = os.Stdin.Read(make([]byte, 1))
+		os.Exit(0)
+	}()
+	syscall.Syscall(syscall.SYS_EXIT, 0, 0, 0)
+}
+
+func TestProcessExitWaitsForZombieLeadersTasks(t *testing.T) {
+	child := exec.Command(os.Args[0], "-test.run=^$")
+	child.Env = append(os.Environ(), "HYPEMAN_TEST_EXIT_LEADER=1")
+	stdin, err := child.StdinPipe()
+	require.NoError(t, err)
+	stdout, err := child.StdoutPipe()
+	require.NoError(t, err)
+	require.NoError(t, child.Start())
+	t.Cleanup(func() {
+		_ = stdin.Close()
+		_ = child.Process.Kill()
+		_ = child.Wait()
+	})
+	_, err = bufio.NewReader(stdout).ReadString('\n')
+	require.NoError(t, err)
+	pid := child.Process.Pid
+	require.Eventually(t, func() bool {
+		state, err := readLinuxProcessState(pid)
+		return err == nil && state == "Z"
+	}, 5*time.Second, 10*time.Millisecond)
+
+	require.True(t, ProcessExists(pid), "a zombie leader does not imply its tasks have exited")
+	require.False(t, WaitForProcessExit(pid, 50*time.Millisecond))
+	// A sibling process gets ECHILD from wait4, as the API does after restart.
+	observer := exec.Command(os.Args[0], "-test.run=^TestProcessExitObserverHelper$")
+	observer.Env = append(os.Environ(), "HYPEMAN_TEST_WAIT_PID="+strconv.Itoa(pid))
+	output, err := observer.CombinedOutput()
+	require.NoError(t, err, "%s", output)
+
+	require.NoError(t, stdin.Close())
+	require.True(t, WaitForProcessExit(pid, 5*time.Second))
+	require.False(t, ProcessExists(pid))
+}
+
+func TestProcessExitObserverHelper(t *testing.T) {
+	value := os.Getenv("HYPEMAN_TEST_WAIT_PID")
+	if value == "" {
+		return
+	}
+	pid, err := strconv.Atoi(value)
+	require.NoError(t, err)
+	var status syscall.WaitStatus
+	_, err = syscall.Wait4(pid, &status, syscall.WNOHANG, nil)
+	require.ErrorIs(t, err, syscall.ECHILD)
+	require.False(t, WaitForProcessExit(pid, 50*time.Millisecond))
+	identity := HypervisorProcessIdentity{}
+	identity.Set(pid)
+	resolved, err := resolveLiveHypervisorPID(identity, "")
+	require.NoError(t, err)
+	require.Equal(t, pid, resolved)
+	m := &manager{}
+	require.True(t, m.vgpuHypervisorMayBeAlive(t.Context(), &StoredMetadata{HypervisorProcessIdentity: identity}))
+}

lib/instances/process_identity.go

@@ -136,7 +136,7 @@ func resolveLiveHypervisorPID(id HypervisorProcessIdentity, socketPath string) (
 		if ProcessExists(*id.HypervisorPID) {
 			stored = *id.HypervisorPID
 		} else {
-			// ProcessExists treats zombies as dead, so a direct-child VMM that
+			// ProcessExists treats fully exited zombies as dead, so a direct-child VMM that
 			// exited on its own never reaches the Wait4 in WaitForProcessExit
 			// and would sit unreaped. Reap it here: WNOHANG leaves a live child
 			// untouched, and a recycled or non-child PID fails with ECHILD.
@@ -201,7 +201,8 @@ func classifyResolvedHypervisorOwner(socketPath string, stored, resolved int, er
 	return 0, fmt.Errorf("cannot confirm ownership of socket %s: %w", socketPath, err)
 }
 
-// ProcessExists reports whether pid belongs to a live, non-zombie process.
+// ProcessExists reports whether pid belongs to a live process. A zombie
+// leader still counts as live while other tasks in its thread group remain.
 func ProcessExists(pid int) bool {
 	if pid <= 0 {
 		return false
@@ -217,7 +218,16 @@ func ProcessExists(pid int) bool {
 	if err != nil {
 		return true
 	}
-	return state != "Z"
+	if state != "Z" {
+		return true
+	}
+	// The leader can be a zombie while a vhost task is still releasing VFIO.
+	// Treat the whole group as alive until only the zombie leader remains.
+	tasks, err := os.ReadDir(filepath.Join("/proc", strconv.Itoa(pid), "task"))
+	if err != nil {
+		return !os.IsNotExist(err)
+	}
+	return len(tasks) > 1
 }
 
 func readLinuxProcessState(pid int) (string, error) {

lib/instances/process_identity_linux_test.go

@@ -44,8 +44,8 @@ func TestResolveLiveHypervisorPIDWithoutStoredPID(t *testing.T) {
 
 // TestResolveLiveHypervisorPIDReapsZombieChild guards against leaking one
 // zombie per direct-child VMM that exits on its own: ProcessExists treats
-// zombies as dead, so the confirmed-gone paths in stop, delete, and standby
-// never reach the Wait4 in WaitForProcessExit.
+// fully exited zombies as dead, so the confirmed-gone paths in stop, delete,
+// and standby never reach the Wait4 in WaitForProcessExit.
 func TestResolveLiveHypervisorPIDReapsZombieChild(t *testing.T) {
 	child := exec.Command("true")
 	require.NoError(t, child.Start())