[codex] fix: make macOS VM creation transactional

cmd/vm/clone.go

@@ -1,6 +1,7 @@
 package vm
 
 import (
+	"errors"
 	"fmt"
 	"path/filepath"
 	"time"
@@ -18,14 +19,17 @@ func (h *Handler) Clone(cmd *cobra.Command, args []string) error {
 	if err != nil {
 		return err
 	}
-	name, _ := cmd.Flags().GetString("name")
-	if name == "" {
-		name = src + "-clone-" + time.Now().Format("150405")
-	}
+	name := requestedVMName(cmd, src+"-clone-"+time.Now().Format("150405"))
+	return withVMLock(cliutil.CommandContext(cmd), home.VMDir(cmd, name), func() error {
+		return h.clone(cmd, srcRec, name)
+	})
+}
+
+func (h *Handler) clone(cmd *cobra.Command, srcRec *record, name string) (retErr error) {
 	netMode, _ := cmd.Flags().GetString("net")
 	vnc, _ := cmd.Flags().GetInt("vnc")
 	vncPass, _ := cmd.Flags().GetString("vnc-password")
-	if err = requireCNIVNCPassword(netMode == netCNI, vnc, vncPass); err != nil {
+	if err := requireCNIVNCPassword(netMode == netCNI, vnc, vncPass); err != nil {
 		return err
 	}
 	// SRC's disk names are reserved: extra --data-disk specs must not collide and the combined count still honors the AHCI cap
@@ -42,6 +46,12 @@ func (h *Handler) Clone(cmd *cobra.Command, args []string) error {
 	if err != nil {
 		return err
 	}
+	var r *record
+	defer func() {
+		if retErr != nil {
+			retErr = errors.Join(retErr, cleanupFailedVM(cmd, dir, r))
+		}
+	}()
 	ctx := cliutil.CommandContext(cmd)
 	copied, err := copyDataDisks(dir, srcRec.DataDisks)
 	if err != nil {
@@ -51,7 +61,7 @@ func (h *Handler) Clone(cmd *cobra.Command, args []string) error {
 	if err != nil {
 		return err
 	}
-	r := &record{
+	r = &record{
 		Name: name, Image: srcRec.Image, ImageDigest: digest, Disk: overlay,
 		OVMFCode: srcRec.OVMFCode, OVMFVars: ovmfVars, CPUs: srcRec.CPUs, Memory: srcRec.Memory,
 		DataDisks: append(copied, newDisks...),

cmd/vm/handler_test.go

@@ -1,12 +1,140 @@
 package vm
 
 import (
+	"io"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"runtime"
 	"slices"
+	"strings"
 	"testing"
 
 	"github.com/spf13/cobra"
 )
 
+func TestStartAlreadyRunningIsIdempotent(t *testing.T) {
+	stateDir := t.TempDir()
+	vmDir := filepath.Join(stateDir, "vms", "macos-demo")
+	if err := os.MkdirAll(vmDir, 0o755); err != nil {
+		t.Fatal(err)
+	}
+
+	// A real process whose argv0+args satisfy isRunning without qemu/KVM.
+	fakeQEMU := filepath.Join(t.TempDir(), qemuBinary)
+	if err := os.Symlink("/bin/sh", fakeQEMU); err != nil {
+		t.Fatal(err)
+	}
+	disk := filepath.Join(vmDir, "disk.qcow2")
+	process := exec.Command(fakeQEMU, "-c", "while :; do sleep 1; done", disk)
+	if err := process.Start(); err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() {
+		_ = process.Process.Kill()
+		_ = process.Wait()
+	})
+
+	rec := &record{Name: "macos-demo", Disk: disk, PID: process.Process.Pid, VNCDisp: 1}
+	if err := saveRec(vmDir, rec); err != nil {
+		t.Fatal(err)
+	}
+
+	cmd := &cobra.Command{}
+	cmd.SetContext(t.Context())
+	cmd.Flags().String("state-dir", stateDir, "")
+	cmd.Flags().Int("vnc", -1, "")
+	cmd.Flags().String("vnc-password", "", "")
+	if err := cmd.Flags().Set("vnc", "2"); err != nil {
+		t.Fatal(err)
+	}
+	if err := cmd.Flags().Set("vnc-password", "newpass"); err != nil {
+		t.Fatal(err)
+	}
+
+	output, err := captureStdout(t, func() error {
+		return NewHandler().Start(cmd, []string{"macos-demo"})
+	})
+	if err != nil {
+		t.Fatalf("duplicate start must adopt the live qemu: %v", err)
+	}
+	if !strings.Contains(output, "supplied VNC settings ignored because live QEMU cannot be retargeted") {
+		t.Fatalf("duplicate start output = %q, want ignored VNC settings warning", output)
+	}
+	got, err := loadRec(vmDir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got.PID != rec.PID || got.VNCDisp != 1 {
+		t.Fatalf("live record changed: pid=%d vnc=%d, want pid=%d vnc=1", got.PID, got.VNCDisp, rec.PID)
+	}
+}
+
+func captureStdout(t *testing.T, fn func() error) (string, error) {
+	t.Helper()
+	reader, writer, err := os.Pipe()
+	if err != nil {
+		t.Fatal(err)
+	}
+	original := os.Stdout
+	os.Stdout = writer
+	callErr := fn()
+	os.Stdout = original
+	if err := writer.Close(); err != nil {
+		t.Fatal(err)
+	}
+	output, err := io.ReadAll(reader)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := reader.Close(); err != nil {
+		t.Fatal(err)
+	}
+	return string(output), callErr
+}
+
+func TestStartAdoptsQEMUWhenRecordPIDWasNotCommitted(t *testing.T) {
+	if runtime.GOOS != "linux" {
+		t.Skip("process cmdline adoption requires /proc")
+	}
+	stateDir := t.TempDir()
+	vmDir := filepath.Join(stateDir, "vms", "macos-demo")
+	if err := os.MkdirAll(vmDir, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	fakeQEMU := filepath.Join(t.TempDir(), qemuBinary)
+	if err := os.Symlink("/bin/sh", fakeQEMU); err != nil {
+		t.Fatal(err)
+	}
+	disk := filepath.Join(vmDir, "disk.qcow2")
+	process := exec.Command(fakeQEMU, "-c", "while :; do sleep 1; done", disk)
+	if err := process.Start(); err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() {
+		_ = process.Process.Kill()
+		_ = process.Wait()
+	})
+	if err := saveRec(vmDir, &record{Name: "macos-demo", Disk: disk, VNCDisp: -1}); err != nil {
+		t.Fatal(err)
+	}
+	cmd := &cobra.Command{}
+	cmd.SetContext(t.Context())
+	cmd.Flags().String("state-dir", stateDir, "")
+	cmd.Flags().Int("vnc", -1, "")
+	cmd.Flags().String("vnc-password", "", "")
+	if err := NewHandler().Start(cmd, []string{"macos-demo"}); err != nil {
+		t.Fatalf("Start must adopt the already-running QEMU: %v", err)
+	}
+	got, err := loadRec(vmDir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got.PID != process.Process.Pid {
+		t.Fatalf("adopted PID = %d, want %d", got.PID, process.Process.Pid)
+	}
+}
+
 func TestCloneOpenCoreBase(t *testing.T) {
 	tests := []struct {
 		name string