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