From f4074cb8d3489cd592904aabdeca854b5666de28 Mon Sep 17 00:00:00 2001 From: Yibo Zhuang Date: Mon, 31 Aug 2026 18:41:33 -0700 Subject: [PATCH] tart: make deletion of missing VMs idempotent Treat tart delete exit code 2 as successful cleanup so deleting an already-removed VM does not prevent reconciliation from continuing. Preserve other command failures and retain the wrapped process error so callers can inspect its exit status. Cover repeated deletion, other exit codes, process startup failures, and wrapped exit-status preservation. Pin CI actions to their previously used commit SHAs so jobs can start under the repository action-pinning policy. Validation: vmmanager package tests and lint for changed lines pass; CI action pins match the last successful run. --- .github/workflows/ci.yml | 16 +-- internal/worker/vmmanager/base/cmd.go | 10 +- internal/worker/vmmanager/base/cmd_test.go | 61 ++++++++++ internal/worker/vmmanager/tart/delete_test.go | 106 ++++++++++++++++++ internal/worker/vmmanager/tart/tart.go | 11 +- 5 files changed, 190 insertions(+), 14 deletions(-) create mode 100644 internal/worker/vmmanager/base/cmd_test.go create mode 100644 internal/worker/vmmanager/tart/delete_test.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 78a356d..2b36de1 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/internal/worker/vmmanager/base/cmd.go b/internal/worker/vmmanager/base/cmd.go index 7355232..837eb3d 100644 --- a/internal/worker/vmmanager/base/cmd.go +++ b/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) } } diff --git a/internal/worker/vmmanager/base/cmd_test.go b/internal/worker/vmmanager/base/cmd_test.go new file mode 100644 index 0000000..bd387b6 --- /dev/null +++ b/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()) +} diff --git a/internal/worker/vmmanager/tart/delete_test.go b/internal/worker/vmmanager/tart/delete_test.go new file mode 100644 index 0000000..5c59e30 --- /dev/null +++ b/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()) +} diff --git a/internal/worker/vmmanager/tart/tart.go b/internal/worker/vmmanager/tart/tart.go index a50065d..b655c47 100644 --- a/internal/worker/vmmanager/tart/tart.go +++ b/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