Fix cleanup of vm

Author: sjmiller609

lib/exec/client.go

@@ -67,15 +67,17 @@ func getOrCreateConn(ctx context.Context, dialer hypervisor.VsockDialer) (*grpc.
 	return conn, nil
 }
 
-// CloseConn closes and removes a connection from the pool (call when VM is deleted)
+// CloseConn removes a connection from the pool (call when VM is deleted).
+// We only remove from pool, not explicitly close - the connection will fail
+// naturally when the VM dies, and grpc will clean up. Calling Close() on a
+// connection with an active reader can cause panics in grpc internals.
 func CloseConn(dialerKey string) {
 	connPool.Lock()
 	defer connPool.Unlock()
 
-	if conn, ok := connPool.conns[dialerKey]; ok {
-		conn.Close()
+	if _, ok := connPool.conns[dialerKey]; ok {
 		delete(connPool.conns, dialerKey)
-		slog.Debug("closed gRPC connection", "key", dialerKey)
+		slog.Debug("removed gRPC connection from pool", "key", dialerKey)
 	}
 }
 

lib/hypervisor/qemu/pool.go

@@ -46,12 +46,14 @@ func GetOrCreate(socketPath string) (*QEMU, error) {
 
 // Remove closes and removes a client from the pool.
 // Called automatically on errors to allow fresh reconnection.
+// Close is done asynchronously to avoid blocking if the connection is in a bad state.
 func Remove(socketPath string) {
 	clientPool.Lock()
 	defer clientPool.Unlock()
 
 	if client, ok := clientPool.clients[socketPath]; ok {
-		client.client.Close()
 		delete(clientPool.clients, socketPath)
+		// Close asynchronously to avoid blocking on stuck connections
+		go client.client.Close()
 	}
 }

lib/hypervisor/qemu/vsock.go

@@ -3,6 +3,7 @@ package qemu
 import (
 	"context"
 	"fmt"
+	"io"
 	"log/slog"
 	"net"
 	"time"
@@ -158,7 +159,16 @@ func newVsockConn(fd int, remoteCID, remotePort uint32) (*vsockConn, error) {
 }
 
 func (c *vsockConn) Read(b []byte) (int, error) {
-	return unix.Read(c.fd, b)
+	n, err := unix.Read(c.fd, b)
+	// Ensure we never return negative n (violates io.Reader contract)
+	// This can happen when the vsock fd becomes invalid (VM died)
+	if n < 0 {
+		if err == nil {
+			err = io.EOF
+		}
+		return 0, err
+	}
+	return n, err
 }
 
 func (c *vsockConn) Write(b []byte) (int, error) {

lib/instances/delete.go

@@ -7,6 +7,8 @@ import (
 	"syscall"
 	"time"
 
+	"github.com/onkernel/hypeman/lib/exec"
+	"github.com/onkernel/hypeman/lib/hypervisor"
 	"github.com/onkernel/hypeman/lib/logger"
 	"github.com/onkernel/hypeman/lib/network"
 )
@@ -39,7 +41,12 @@ func (m *manager) deleteInstance(
 		}
 	}
 
-	// 3. If hypervisor might be running, force kill it
+	// 3. Close exec gRPC connection before killing hypervisor to prevent panic
+	if dialer, err := hypervisor.NewVsockDialer(inst.HypervisorType, inst.VsockSocket, inst.VsockCID); err == nil {
+		exec.CloseConn(dialer.Key())
+	}
+
+	// 4. If hypervisor might be running, force kill it
 	// Also attempt kill for StateUnknown since we can't be sure if hypervisor is running
 	if inst.State.RequiresVMM() || inst.State == StateUnknown {
 		log.DebugContext(ctx, "stopping hypervisor", "instance_id", id, "state", inst.State)
@@ -50,7 +57,7 @@ func (m *manager) deleteInstance(
 		}
 	}
 
-	// 4. Release network allocation
+	// 5. Release network allocation
 	if inst.NetworkEnabled {
 		log.DebugContext(ctx, "releasing network", "instance_id", id, "network", "default")
 		if err := m.networkManager.ReleaseAllocation(ctx, networkAlloc); err != nil {
@@ -59,7 +66,7 @@ func (m *manager) deleteInstance(
 		}
 	}
 
-	// 5. Detach and auto-unbind devices from VFIO
+	// 6. Detach and auto-unbind devices from VFIO
 	if len(inst.Devices) > 0 && m.deviceManager != nil {
 		for _, deviceID := range inst.Devices {
 			log.DebugContext(ctx, "detaching device", "id", id, "device", deviceID)
@@ -76,7 +83,7 @@ func (m *manager) deleteInstance(
 		}
 	}
 
-	// 5b. Detach volumes
+	// 6b. Detach volumes
 	if len(inst.Volumes) > 0 {
 		log.DebugContext(ctx, "detaching volumes", "instance_id", id, "count", len(inst.Volumes))
 		for _, volAttach := range inst.Volumes {
@@ -87,7 +94,7 @@ func (m *manager) deleteInstance(
 		}
 	}
 
-	// 6. Delete all instance data
+	// 7. Delete all instance data
 	log.DebugContext(ctx, "deleting instance data", "instance_id", id)
 	if err := m.deleteInstanceData(id); err != nil {
 		log.ErrorContext(ctx, "failed to delete instance data", "instance_id", id, "error", err)

lib/network/derive.go

@@ -9,7 +9,6 @@ import (
 
 	"github.com/onkernel/hypeman/lib/hypervisor"
 	"github.com/onkernel/hypeman/lib/logger"
-	"github.com/onkernel/hypeman/lib/vmm"
 )
 
 // instanceMetadata is the minimal metadata we need to derive allocations
@@ -18,6 +17,8 @@ type instanceMetadata struct {
 	Name           string
 	NetworkEnabled bool
 	HypervisorType string
+	IP             string // Assigned IP address
+	MAC            string // Assigned MAC address
 }
 
 // deriveAllocation derives network allocation from CH or snapshot
@@ -49,58 +50,38 @@ func (m *manager) deriveAllocation(ctx context.Context, instanceID string) (*All
 	}
 	netmask := fmt.Sprintf("%d.%d.%d.%d", ipNet.Mask[0], ipNet.Mask[1], ipNet.Mask[2], ipNet.Mask[3])
 
-	// 4. Try to derive from running VM first
-	socketPath := m.paths.InstanceSocket(instanceID, hypervisor.SocketNameForType(hypervisor.Type(meta.HypervisorType)))
-	if fileExists(socketPath) {
-		client, err := vmm.NewVMM(socketPath)
-		if err == nil {
-			resp, err := client.GetVmInfoWithResponse(ctx)
-			if err == nil && resp.JSON200 != nil && resp.JSON200.Config.Net != nil && len(*resp.JSON200.Config.Net) > 0 {
-				nets := *resp.JSON200.Config.Net
-				net := nets[0]
-				if net.Ip != nil && net.Mac != nil && net.Tap != nil {
-					log.DebugContext(ctx, "derived allocation from running VM", "instance_id", instanceID)
-					return &Allocation{
-						InstanceID:   instanceID,
-						InstanceName: meta.Name,
-						Network:      "default",
-						IP:           *net.Ip,
-						MAC:          *net.Mac,
-						TAPDevice:    *net.Tap,
-						Gateway:      defaultNet.Gateway,
-						Netmask:      netmask,
-						State:        "running",
-					}, nil
-				}
+	// 4. Use stored metadata to derive allocation (works for all hypervisors)
+	if meta.IP != "" && meta.MAC != "" {
+		tap := generateTAPName(instanceID)
+
+		// Determine state based on socket existence and snapshot
+		socketPath := m.paths.InstanceSocket(instanceID, hypervisor.SocketNameForType(hypervisor.Type(meta.HypervisorType)))
+		state := "stopped"
+		if fileExists(socketPath) {
+			state = "running"
+		} else {
+			// Check for snapshot (standby state)
+			snapshotConfigJson := m.paths.InstanceSnapshotConfig(instanceID)
+			if fileExists(snapshotConfigJson) {
+				state = "standby"
 			}
 		}
-	}
 
-	// 5. Try to derive from snapshot
-	// Cloud Hypervisor creates config.json in the snapshot directory
-	snapshotConfigJson := m.paths.InstanceSnapshotConfig(instanceID)
-	if fileExists(snapshotConfigJson) {
-		vmConfig, err := m.parseVmJson(snapshotConfigJson)
-		if err == nil && vmConfig.Net != nil && len(*vmConfig.Net) > 0 {
-			nets := *vmConfig.Net
-			if nets[0].Ip != nil && nets[0].Mac != nil && nets[0].Tap != nil {
-				log.DebugContext(ctx, "derived allocation from snapshot", "instance_id", instanceID)
-				return &Allocation{
-					InstanceID:   instanceID,
-					InstanceName: meta.Name,
-					Network:      "default",
-					IP:           *nets[0].Ip,
-					MAC:          *nets[0].Mac,
-					TAPDevice:    *nets[0].Tap,
-					Gateway:      defaultNet.Gateway,
-					Netmask:      netmask,
-					State:        "standby",
-				}, nil
-			}
-		}
+		log.DebugContext(ctx, "derived allocation from metadata", "instance_id", instanceID, "state", state)
+		return &Allocation{
+			InstanceID:   instanceID,
+			InstanceName: meta.Name,
+			Network:      "default",
+			IP:           meta.IP,
+			MAC:          meta.MAC,
+			TAPDevice:    tap,
+			Gateway:      defaultNet.Gateway,
+			Netmask:      netmask,
+			State:        state,
+		}, nil
 	}
 
-	// 6. No allocation (stopped or network not yet configured)
+	// 5. No allocation (network not yet configured)
 	return nil, nil
 }
 
@@ -164,22 +145,6 @@ func (m *manager) loadInstanceMetadata(instanceID string) (*instanceMetadata, er
 	return &meta, nil
 }
 
-// parseVmJson parses Cloud Hypervisor's config.json from snapshot
-// Note: Despite the function name, this parses config.json (what CH actually creates)
-func (m *manager) parseVmJson(path string) (*vmm.VmConfig, error) {
-	data, err := os.ReadFile(path)
-	if err != nil {
-		return nil, fmt.Errorf("read config.json: %w", err)
-	}
-
-	var vmConfig vmm.VmConfig
-	if err := json.Unmarshal(data, &vmConfig); err != nil {
-		return nil, fmt.Errorf("unmarshal config.json: %w", err)
-	}
-
-	return &vmConfig, nil
-}
-
 // fileExists checks if a file exists
 func fileExists(path string) bool {
 	_, err := os.Stat(path)