Make deletion of missing VMs idempotent

.github/workflows/ci.yml

@@ -18,12 +18,12 @@ jobs:
     runs-on: ubuntu-latest
     timeout-minutes: 15
     steps:
-      - uses: actions/checkout@v6
-      - uses: actions/setup-go@v6
+      - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6.1.0
+      - uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6.5.0
         with:
           go-version-file: go.mod
           cache: true
-      - uses: golangci/golangci-lint-action@v9
+      - uses: golangci/golangci-lint-action@ba0d7d2ec06a0ea1cb5fa41b2e4a3ab91d21278a # v9.3.0
         with:
           version: v2.13.0
           only-new-issues: true
@@ -33,13 +33,13 @@ jobs:
     runs-on: ubuntu-latest
     timeout-minutes: 60
     steps:
-      - uses: actions/checkout@v6
-      - uses: actions/setup-go@v6
+      - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6.1.0
+      - uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6.5.0
         with:
           go-version-file: go.mod
           cache: true
       - name: Check out Vetu
-        uses: actions/checkout@v6
+        uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6.1.0
         with:
           repository: openai/vetu
           path: _vetu
@@ -67,8 +67,8 @@ jobs:
     runs-on: macos-26
     timeout-minutes: 45
     steps:
-      - uses: actions/checkout@v6
-      - uses: actions/setup-go@v6
+      - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6.1.0
+      - uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6.5.0
         with:
           go-version-file: go.mod
           cache: true

internal/worker/vmmanager/base/cmd.go

@@ -30,8 +30,8 @@ func Cmd(
 	err := cmd.Run()
 	if err != nil {
 		if errors.Is(err, exec.ErrNotFound) {
-			return "", "", fmt.Errorf("%s command not found in PATH, make sure %s is installed",
-				commandName, strings.ToTitle(commandName))
+			return "", "", fmt.Errorf("%s command not found in PATH, make sure %s is installed: %w",
+				commandName, strings.ToTitle(commandName), err)
 		}
 
 		if exitErr, ok := err.(*exec.ExitError); ok {
@@ -46,9 +46,9 @@ func Cmd(
 				)
 			}
 
-			// Command failed, redefine the error to be the command-specific output
-			err = fmt.Errorf("%s command failed: %q", commandName,
-				firstNonEmptyLine(stderr.String(), stdout.String()))
+			// Preserve the exit status while adding the command-specific output.
+			err = fmt.Errorf("%s command failed: %q: %w", commandName,
+				firstNonEmptyLine(stderr.String(), stdout.String()), exitErr)
 		}
 	}
 

internal/worker/vmmanager/base/cmd_test.go

@@ -0,0 +1,61 @@
+package base_test
+
+import (
+	"context"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strconv"
+	"testing"
+	"time"
+
+	"github.com/cirruslabs/orchard/internal/worker/vmmanager/base"
+	"github.com/stretchr/testify/require"
+	"go.uber.org/zap"
+)
+
+func TestCmdPreservesExitStatus(t *testing.T) {
+	for _, commandName := range []string{"tart", "vetu"} {
+		t.Run(commandName, func(t *testing.T) {
+			commandPath := filepath.Join(t.TempDir(), commandName)
+			script := `#!/bin/sh
+printf 'command output\n'
+printf '\ncommand failed\nmore detail\n' >&2
+exit "$1"
+`
+			require.NoError(t, os.WriteFile(commandPath, []byte(script), 0o600))
+			require.NoError(t, os.Chmod(commandPath, 0o700)) //nolint:gosec // The fake command must be executable.
+
+			for _, exitCode := range []int{1, 2, 64} {
+				t.Run("exit-"+strconv.Itoa(exitCode), func(t *testing.T) {
+					ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second)
+					defer cancel()
+
+					stdout, stderr, err := base.Cmd(ctx, zap.NewNop().Sugar(), commandPath, strconv.Itoa(exitCode))
+
+					require.Equal(t, "command output\n", stdout)
+					require.Equal(t, "\ncommand failed\nmore detail\n", stderr)
+					require.ErrorContains(t, err, "command failed")
+					var exitErr *exec.ExitError
+					require.ErrorAs(t, err, &exitErr)
+					require.Equal(t, exitCode, exitErr.ExitCode())
+				})
+			}
+		})
+	}
+}
+
+func TestCmdPreservesSignalStatus(t *testing.T) {
+	commandPath := filepath.Join(t.TempDir(), "signalled-command")
+	require.NoError(t, os.WriteFile(commandPath, []byte("#!/bin/sh\nkill -TERM $$\n"), 0o600))
+	require.NoError(t, os.Chmod(commandPath, 0o700)) //nolint:gosec // The fake command must be executable.
+	ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second)
+	defer cancel()
+
+	_, _, err := base.Cmd(ctx, zap.NewNop().Sugar(), commandPath)
+
+	require.NoError(t, ctx.Err(), "the command must terminate before the test deadline")
+	var exitErr *exec.ExitError
+	require.ErrorAs(t, err, &exitErr)
+	require.Equal(t, -1, exitErr.ExitCode())
+}

internal/worker/vmmanager/tart/delete_test.go

@@ -0,0 +1,106 @@
+package tart //nolint:testpackage // VM fixtures require private state without starting clone or run goroutines.
+
+import (
+	"context"
+	"fmt"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"testing"
+
+	"github.com/cirruslabs/orchard/internal/worker/ondiskname"
+	"github.com/cirruslabs/orchard/internal/worker/vmmanager/base"
+	v1 "github.com/cirruslabs/orchard/pkg/resource/v1"
+	"github.com/stretchr/testify/require"
+	"go.uber.org/zap"
+)
+
+func TestDeleteIsIdempotent(t *testing.T) {
+	vm, vmPath := newVMForDelete(t, `#!/bin/sh
+[ "$#" -eq 2 ] && [ "$1" = delete ] || exit 64
+vm_dir="$TART_HOME/vms/$2"
+if [ ! -e "$vm_dir" ]; then
+  printf 'the specified VM "%s" does not exist\n' "$2" >&2
+  exit 2
+fi
+/bin/rm -r "$vm_dir"
+`)
+	require.NoError(t, os.MkdirAll(vmPath, 0o700))
+
+	require.NoError(t, vm.Delete())
+	require.NoDirExists(t, vmPath)
+	require.NoError(t, vm.Delete(), "deleting the same VM again must succeed")
+
+	select {
+	case <-vm.ctx.Done():
+	default:
+		t.Fatal("Delete did not cancel the VM context")
+	}
+}
+
+func TestDeletePreservesCommandFailures(t *testing.T) {
+	for _, exitCode := range []int{1, 64, 126} {
+		t.Run(fmt.Sprintf("exit-%d", exitCode), func(t *testing.T) {
+			vm, _ := newVMForDelete(t, fmt.Sprintf(
+				"#!/bin/sh\nprintf 'the specified VM does not exist: permission denied\\n' >&2\nexit %d\n",
+				exitCode,
+			))
+
+			err := vm.Delete()
+
+			require.ErrorIs(t, err, base.ErrVMFailed)
+			require.ErrorContains(t, err, "permission denied")
+			var exitErr *exec.ExitError
+			require.ErrorAs(t, err, &exitErr)
+			require.Equal(t, exitCode, exitErr.ExitCode())
+		})
+	}
+}
+
+func TestDeletePreservesProcessStartFailure(t *testing.T) {
+	vm, _ := newVMForDelete(t, "#!/nonexistent/tart-interpreter\n")
+
+	err := vm.Delete()
+
+	require.ErrorIs(t, err, base.ErrVMFailed)
+	require.ErrorIs(t, err, os.ErrNotExist)
+	var pathErr *os.PathError
+	require.ErrorAs(t, err, &pathErr)
+}
+
+func TestDeletePreservesMissingExecutable(t *testing.T) {
+	vm, _ := newVMForDelete(t, "#!/bin/sh\nexit 0\n")
+	t.Setenv("PATH", t.TempDir())
+
+	err := vm.Delete()
+
+	require.ErrorIs(t, err, base.ErrVMFailed)
+	require.ErrorIs(t, err, exec.ErrNotFound)
+	require.ErrorContains(t, err, "tart command not found in PATH")
+}
+
+func newVMForDelete(t *testing.T, script string) (*VM, string) {
+	t.Helper()
+
+	binDir := t.TempDir()
+	commandPath := filepath.Join(binDir, "tart")
+	require.NoError(t, os.WriteFile(commandPath, []byte(script), 0o600))
+	require.NoError(t, os.Chmod(commandPath, 0o700)) //nolint:gosec // Fake Tart must be executable.
+	t.Setenv("PATH", binDir)
+	tartHome := t.TempDir()
+	t.Setenv("TART_HOME", tartHome)
+
+	logger := zap.NewNop().Sugar()
+	ctx, cancel := context.WithCancel(t.Context())
+	t.Cleanup(cancel)
+	vm := &VM{ //nolint:exhaustruct_v5 // The fixture initializes only the state used by Delete.
+		onDiskName: ondiskname.New("delete-test", "11111111-2222-4333-8444-555555555555", 0),
+		logger:     logger,
+		ctx:        ctx,
+		cancel:     cancel,
+		VM:         base.NewVM(logger),
+	}
+	vm.ConditionsSet().Remove(v1.ConditionTypeCloning)
+
+	return vm, filepath.Join(tartHome, "vms", vm.id())
+}

internal/worker/vmmanager/tart/tart.go

@@ -2,7 +2,9 @@ package tart
 
 import (
 	"context"
+	"errors"
 	"fmt"
+	"os/exec"
 	"strconv"
 	"strings"
 	"sync"
@@ -19,6 +21,8 @@ import (
 	"go.uber.org/zap"
 )
 
+const tartDeleteExitCodeNotFound = 2
+
 type VM struct {
 	onDiskName ondiskname.OnDiskName
 	resource   v1.VM
@@ -480,7 +484,12 @@ func (vm *VM) Delete() error {
 
 	_, _, err := Tart(context.Background(), vm.logger, "delete", vm.id())
 	if err != nil {
-		return fmt.Errorf("%w: failed to delete VM: %v", base.ErrVMFailed, err)
+		var exitErr *exec.ExitError
+		if errors.As(err, &exitErr) && exitErr.ExitCode() == tartDeleteExitCodeNotFound {
+			return nil
+		}
+
+		return fmt.Errorf("%w: failed to delete VM: %w", base.ErrVMFailed, err)
 	}
 
 	return nil