diff --git a/.github/workflows/pr-ci.yml b/.github/workflows/pr-ci.yml index c690f16b15..6541c572e0 100644 --- a/.github/workflows/pr-ci.yml +++ b/.github/workflows/pr-ci.yml @@ -302,6 +302,12 @@ jobs: install-kind: false requires-secret: false + - label: task-cancel + runner: ubuntu-latest + free-disk-space: false + install-kind: false + requires-secret: false + - label: ssh-credentials-server-race runner: ubuntu-latest free-disk-space: false @@ -592,6 +598,17 @@ jobs: free-disk-space: false install-kind: true requires-secret: false + - label: ssh-tunnel-mode + runner: windows-latest + free-disk-space: false + install-kind: true + requires-secret: false + + - label: task-cancel + runner: windows-latest + free-disk-space: false + install-kind: true + requires-secret: false runs-on: ${{ matrix.runner }} diff --git a/Taskfile.yml b/Taskfile.yml index f4766c8dfa..f7dfeee7a2 100644 --- a/Taskfile.yml +++ b/Taskfile.yml @@ -116,10 +116,20 @@ tasks: host_os="$(go env GOOS)" host_arch="$(go env GOARCH)" host_binary="devsy-${host_os}-${host_arch}" - host_dir="dist/devsy-dev_${host_os}_${host_arch}_v1" if [ "$host_os" = "windows" ]; then host_binary="${host_binary}.exe" fi + host_dir="" + for candidate in dist/devsy-dev_${host_os}_${host_arch}_*; do + if [ -d "$candidate" ]; then + if [ -n "$host_dir" ]; then + echo "multiple host build directories match ${host_os}/${host_arch}" >&2 + exit 1 + fi + host_dir="$candidate" + fi + done + test -n "$host_dir" test -f "${host_dir}/${host_binary}" cp "${host_dir}/${host_binary}" "e2e/bin/${host_binary}" diff --git a/cmd/workspace/stop.go b/cmd/workspace/stop.go index 9a1a12654a..bba8ec4cea 100644 --- a/cmd/workspace/stop.go +++ b/cmd/workspace/stop.go @@ -22,6 +22,7 @@ import ( type StopCmd struct { *flags.GlobalFlags client2.StopOptions + quiescer workspace2.UpTaskQuiescer } // NewStopCmd creates a new destroy command. @@ -123,6 +124,12 @@ func (cmd *StopCmd) run( devsyConfig *config.Config, client client2.BaseWorkspaceClient, ) error { + // Quiesce before waiting for the workspace lock: a still-provisioning up + // worker can be the one holding it. + if err := cmd.quiesce(ctx, client.Workspace()); err != nil { + return err + } + // lock workspace if !cmd.Platform.Enabled { err := status.RunStep( @@ -137,6 +144,13 @@ func (cmd *StopCmd) run( defer client.Unlock() } + // Quiesce again under the lock: a task may have become visible while + // the lock wait blocked, and a surviving up worker can restart the + // workspace after the stop. + if err := cmd.quiesce(ctx, client.Workspace()); err != nil { + return err + } + // get instance status var instanceStatus client2.Status err := status.RunStep( @@ -255,3 +269,18 @@ func otherWorkspaceUsesMachine( } return false } + +func (cmd *StopCmd) quiesce(ctx context.Context, workspaceID string) error { + quiescer := cmd.quiescer + if quiescer == nil { + quiescer = workspace2.StoreUpTaskQuiescer{} + } + if err := quiescer.Quiesce(ctx, workspaceID); err != nil { + return fmt.Errorf( + "cancel detached up tasks for workspace %s: %w", + workspaceID, + err, + ) + } + return nil +} diff --git a/cmd/workspace/stop_test.go b/cmd/workspace/stop_test.go new file mode 100644 index 0000000000..5fd572793e --- /dev/null +++ b/cmd/workspace/stop_test.go @@ -0,0 +1,131 @@ +package workspace + +import ( + "context" + "errors" + "testing" + + "github.com/devsy-org/devsy/cmd/flags" + client2 "github.com/devsy-org/devsy/pkg/client" + "github.com/devsy-org/devsy/pkg/config" + "github.com/devsy-org/devsy/pkg/provider" + "github.com/devsy-org/devsy/pkg/task" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type stopRecordingClient struct { + *fakeWorkspaceClient + calls *[]string +} + +type quiescerFunc func(context.Context, string) error + +func (f quiescerFunc) Quiesce(ctx context.Context, workspaceID string) error { + return f(ctx, workspaceID) +} + +func (c *stopRecordingClient) Stop(ctx context.Context, opts client2.StopOptions) error { + *c.calls = append(*c.calls, "stop") + return c.fakeWorkspaceClient.Stop(ctx, opts) +} + +func newStopTestConfig() *config.Config { + return &config.Config{ + DefaultContext: testContext, + Contexts: map[string]*config.ContextConfig{testContext: {}}, + } +} + +func TestStopCancelsPersistedUpTaskBeforeProviderStop(t *testing.T) { + var calls []string + fake := &fakeWorkspaceClient{ + workspace: testWorkspaceName, + context: testContext, + provider: testProvider, + config: &provider.Workspace{ID: testWorkspaceName, Context: testContext}, + status: client2.StatusRunning, + } + client := &stopRecordingClient{fakeWorkspaceClient: fake, calls: &calls} + cmd := &StopCmd{GlobalFlags: &flags.GlobalFlags{ResultFormat: formatPlain}} + cmd.quiescer = quiescerFunc(func(_ context.Context, workspaceID string) error { + assert.Equal(t, testWorkspaceName, workspaceID) + calls = append(calls, "quiesce") + return nil + }) + + require.NoError(t, cmd.run(t.Context(), newStopTestConfig(), client)) + + require.NotEmpty(t, calls) + assert.Equal(t, "quiesce", calls[0]) + assert.Contains(t, calls, "stop") + stopIndex := -1 + for i, call := range calls { + if call == "stop" { + stopIndex = i + break + } + } + quiesceBeforeStop := 0 + for _, call := range calls[:stopIndex] { + if call == "quiesce" { + quiesceBeforeStop++ + } + } + assert.Equal(t, 2, quiesceBeforeStop, "stop must quiesce before and after the workspace lock") +} + +func TestStopDoesNotProceedWhenActiveTaskCannotBeKilled(t *testing.T) { + fake := &fakeWorkspaceClient{ + workspace: testWorkspaceName, + context: testContext, + provider: testProvider, + config: &provider.Workspace{ID: testWorkspaceName, Context: testContext}, + status: client2.StatusRunning, + } + var calls []string + client := &stopRecordingClient{fakeWorkspaceClient: fake, calls: &calls} + cmd := &StopCmd{GlobalFlags: &flags.GlobalFlags{ResultFormat: formatPlain}} + cmd.quiescer = quiescerFunc(func(context.Context, string) error { + return errors.New("boom") + }) + + err := cmd.run(t.Context(), newStopTestConfig(), client) + require.Error(t, err) + assert.NotContains(t, calls, "stop") +} + +func TestStopFindsTaskWithoutDesktopState(t *testing.T) { + home := t.TempDir() + t.Setenv("DEVSY_HOME", home) + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + config.ResetPathManager() + t.Cleanup(config.ResetPathManager) + + // A persisted up task with no live worker, as a Desktop restart leaves + // behind. + store, err := task.NewStore() + require.NoError(t, err) + tk, err := store.Create(task.CreateOptions{Command: "up", WorkspaceID: testWorkspaceName}) + require.NoError(t, err) + + fake := &fakeWorkspaceClient{ + workspace: testWorkspaceName, + context: testContext, + provider: testProvider, + config: &provider.Workspace{ID: testWorkspaceName, Context: testContext}, + status: client2.StatusRunning, + } + var calls []string + client := &stopRecordingClient{fakeWorkspaceClient: fake, calls: &calls} + cmd := &StopCmd{GlobalFlags: &flags.GlobalFlags{ResultFormat: formatPlain}} + + require.NoError(t, cmd.run(t.Context(), newStopTestConfig(), client)) + assert.Contains(t, calls, "stop") + + state, err := store.Get(tk.ID()) + require.NoError(t, err) + assert.Equal(t, task.StatusFailed, state.Status) + assert.Equal(t, task.ErrCanceled.Error(), state.Error) +} diff --git a/cmd/workspace/up/detach.go b/cmd/workspace/up/detach.go index 4f03ef5506..fb95442762 100644 --- a/cmd/workspace/up/detach.go +++ b/cmd/workspace/up/detach.go @@ -1,16 +1,20 @@ package up import ( + "errors" "fmt" "os" "os/exec" "strings" + "time" "github.com/devsy-org/devsy/pkg/command" config2 "github.com/devsy-org/devsy/pkg/devcontainer/config" + "github.com/devsy-org/devsy/pkg/file" "github.com/devsy-org/devsy/pkg/flags/names" "github.com/devsy-org/devsy/pkg/output" "github.com/devsy-org/devsy/pkg/task" + workspace2 "github.com/devsy-org/devsy/pkg/workspace" ) // runDetached submits this invocation as a background task and returns @@ -20,15 +24,24 @@ func (cmd *UpCmd) runDetached(args []string) error { if err != nil { return err } + workspaceID, err := cmd.detachWorkspaceLabel(args) + if err != nil { + return err + } t, err := store.Create(task.CreateOptions{ Command: "up", - WorkspaceID: cmd.detachWorkspaceLabel(args), + WorkspaceID: workspaceID, }) if err != nil { return err } - if err := launchDetached(t.ID()); err != nil { + if err := t.BeginLaunch(); err != nil { + _ = t.Fail(err) + return fmt.Errorf("prepare detached up: %w", err) + } + if err := launchDetached(t); err != nil { + _ = t.FinishLaunch() _ = t.Fail(err) return fmt.Errorf("launch detached up: %w", err) } @@ -46,14 +59,19 @@ func (cmd *UpCmd) runDetached(args []string) error { return err } -func launchDetached(taskID string) error { +func launchDetached(t *task.Task) error { execPath, err := os.Executable() if err != nil { return err } - args := append(detachedArgs(os.Args[1:]), names.Flag(names.TaskID), taskID) - return command.StartBackground(task.WorkerProcessName(taskID), func() (*exec.Cmd, error) { + args := append(detachedArgs(os.Args[1:]), names.Flag(names.TaskID), t.ID()) + return command.StartSupervisedBackground(command.SupervisedStartOptions{ + Name: task.WorkerProcessName(t.ID()), + OnStarted: func(ref command.ProcessRef) error { + return t.SetProcess(ref) + }, + }, func() (*exec.Cmd, error) { return &exec.Cmd{ Path: execPath, Args: append([]string{execPath}, args...), @@ -76,15 +94,22 @@ func detachedArgs(args []string) []string { return out } -// detachWorkspaceLabel is a best-effort label for task list. -func (cmd *UpCmd) detachWorkspaceLabel(args []string) string { +// detachWorkspaceLabel derives the ID stop and delete use before worker startup. +func (cmd *UpCmd) detachWorkspaceLabel(args []string) (string, error) { + if cmd.FromSnapshot != "" { + if _, err := cmd.resolveExplicitSource(); err != nil { + return "", err + } + } if cmd.ID != "" { - return cmd.ID + return cmd.ID, nil } + args = cmd.ensureArgs(args) if len(args) > 0 { - return args[0] + _, source := file.IsLocalDir(args[0]) + return workspace2.ToID(source), nil } - return "" + return "", nil } func wd() string { @@ -105,16 +130,45 @@ func (cmd *UpCmd) openTask() (*task.Task, error) { return nil, err } t := store.Open(cmd.taskID) - // obtain the worker lock first. if err := t.HoldWorkerLock(); err != nil { failTask(t, err) return nil, err } - if err := t.SetPID(os.Getpid()); err != nil { - failTask(t, err) - return nil, err + deadline := time.NewTimer(5 * time.Second) + defer deadline.Stop() + ticker := time.NewTicker(20 * time.Millisecond) + defer ticker.Stop() + for { + state, err := store.Get(cmd.taskID) + if err != nil { + failTask(t, err) + _ = t.ReleaseWorkerLock() + return nil, err + } + if state.Status.Terminal() || state.CancelRequested { + if err := t.ReleaseWorkerLock(); err != nil { + return nil, fmt.Errorf("release canceled worker lock: %w", err) + } + return nil, task.ErrCanceled + } + if !state.LaunchPending { + if _, ok := state.ProcessReference(); !ok { + err := errors.New("detached worker launch did not publish process metadata") + failTask(t, err) + _ = t.ReleaseWorkerLock() + return nil, err + } + return t, nil + } + select { + case <-deadline.C: + err := errors.New("timed out waiting for detached worker launch metadata") + failTask(t, err) + _ = t.ReleaseWorkerLock() + return nil, err + case <-ticker.C: + } } - return t, nil } func failTask(t *task.Task, err error) { diff --git a/cmd/workspace/up/detach_test.go b/cmd/workspace/up/detach_test.go index 965d7b5551..4635e1faef 100644 --- a/cmd/workspace/up/detach_test.go +++ b/cmd/workspace/up/detach_test.go @@ -1,8 +1,14 @@ package up import ( + "errors" + "os" "reflect" "testing" + + "github.com/devsy-org/devsy/pkg/config" + "github.com/devsy-org/devsy/pkg/task" + workspace2 "github.com/devsy-org/devsy/pkg/workspace" ) const ( @@ -33,3 +39,105 @@ func TestDetachedArgs_NoDetachFlagIsUnchanged(t *testing.T) { t.Errorf("detachedArgs() = %v, want %v", got, want) } } + +func TestDetachWorkspaceLabelNormalizesPath(t *testing.T) { + cmd := &UpCmd{} + raw := "/tmp/some workspace dir" + got, err := cmd.detachWorkspaceLabel([]string{raw}) + if err != nil { + t.Fatalf("detachWorkspaceLabel: %v", err) + } + want := workspace2.ToID(raw) + if got != want { + t.Errorf("detachWorkspaceLabel() = %q, want %q", got, want) + } + if got == raw { + t.Error("label must be the workspace ID, not the raw path") + } +} + +func TestDetachWorkspaceLabelResolvesCurrentDirectory(t *testing.T) { + cmd := &UpCmd{} + got, err := cmd.detachWorkspaceLabel([]string{"."}) + if err != nil { + t.Fatalf("detachWorkspaceLabel: %v", err) + } + cwd, err := os.Getwd() + if err != nil { + t.Fatalf("Getwd: %v", err) + } + want := workspace2.ToID(cwd) + if got != want { + t.Fatalf("detachWorkspaceLabel(.) = %q, want %q", got, want) + } +} + +func TestDetachWorkspaceLabelUsesSourceFlag(t *testing.T) { + cmd := &UpCmd{} + cmd.Source = "/tmp/source workspace" + got, err := cmd.detachWorkspaceLabel(nil) + if err != nil { + t.Fatalf("detachWorkspaceLabel: %v", err) + } + if want := workspace2.ToID(cmd.Source); got != want { + t.Fatalf("detachWorkspaceLabel() = %q, want %q", got, want) + } +} + +func TestDetachWorkspaceLabelUsesSnapshotWorkspaceID(t *testing.T) { + cmd := &UpCmd{FromSnapshot: testSnapshotRef} + got, err := cmd.detachWorkspaceLabel(nil) + if err != nil { + t.Fatalf("detachWorkspaceLabel: %v", err) + } + if got != "my-ws" { + t.Fatalf("detachWorkspaceLabel() = %q, want my-ws", got) + } +} + +func TestOpenTaskExitsWhenAlreadyCanceled(t *testing.T) { + // DEVSY_HOME redirects the task store on every platform; $HOME alone + // misses the Windows path manager, which reads %LOCALAPPDATA%. + t.Setenv("DEVSY_HOME", t.TempDir()) + t.Setenv("HOME", t.TempDir()) + config.ResetPathManager() + t.Cleanup(config.ResetPathManager) + + store, err := task.NewStore() + if err != nil { + t.Fatalf("NewStore: %v", err) + } + tk, err := store.Create(task.CreateOptions{Command: "up"}) + if err != nil { + t.Fatalf("Create: %v", err) + } + if err := tk.Cancel(); err != nil { + t.Fatalf("Cancel: %v", err) + } + + cmd := &UpCmd{taskID: tk.ID()} + got, err := cmd.openTask() + if got != nil || !errors.Is(err, task.ErrCanceled) { + t.Fatalf("openTask() = (%v, %v), want (nil, ErrCanceled)", got, err) + } + + state, err := store.Get(tk.ID()) + if err != nil { + t.Fatalf("Get: %v", err) + } + if state.Error != task.ErrCanceled.Error() { + t.Errorf("openTask overwrote the canceled state: %+v", state) + } + assertWorkerLockReleased(t, store, tk.ID()) +} + +func assertWorkerLockReleased(t *testing.T, store *task.Store, id string) { + t.Helper() + lockHolder := store.Open(id) + if err := lockHolder.HoldWorkerLock(); err != nil { + t.Fatalf("worker lock remained held after canceled startup: %v", err) + } + if err := lockHolder.ReleaseWorkerLock(); err != nil { + t.Fatalf("release worker lock: %v", err) + } +} diff --git a/cmd/workspace/up/tunnel.go b/cmd/workspace/up/tunnel.go index 69b7cb5c23..b95a39f7f7 100644 --- a/cmd/workspace/up/tunnel.go +++ b/cmd/workspace/up/tunnel.go @@ -10,6 +10,23 @@ import ( "github.com/devsy-org/devsy/pkg/tunnel" ) +// workspaceTunnelHealth must stay observational; dialing SSH can revive a +// stopped workspace. +func workspaceTunnelHealth( + client client2.BaseWorkspaceClient, +) func(ctx context.Context) error { + return func(ctx context.Context) error { + status, err := client.Status(ctx, client2.StatusOptions{ContainerStatus: true}) + if err != nil { + return fmt.Errorf("workspace status: %w", err) + } + if status != client2.StatusRunning { + return fmt.Errorf("workspace is %q", status) + } + return nil + } +} + // startTunnel creates a local TCP tunnel that forwards connections to the // container SSH server. The returned cleanup function must be called when // the tunnel is no longer needed (typically via defer). @@ -30,8 +47,9 @@ func (cmd *UpCmd) startTunnel( } localTunnel, err := tunnel.NewLocalTunnel(ctx, tunnel.LocalTunnelOptions{ - BasePort: 10800, - DialFunc: dialer.Dial, + BasePort: 10800, + DialFunc: dialer.Dial, + HealthCheckFunc: workspaceTunnelHealth(client), }) if err != nil { return 0, nil, fmt.Errorf("create local tunnel: %w", err) diff --git a/cmd/workspace/up/tunnel_test.go b/cmd/workspace/up/tunnel_test.go new file mode 100644 index 0000000000..2ec2b2ce65 --- /dev/null +++ b/cmd/workspace/up/tunnel_test.go @@ -0,0 +1,52 @@ +package up + +import ( + "context" + "errors" + "testing" + + client2 "github.com/devsy-org/devsy/pkg/client" +) + +// statusOnlyClient panics on any method but Status, so a health probe that +// reaches for a mutating path fails the test. +type statusOnlyClient struct { + client2.BaseWorkspaceClient + status client2.Status + err error +} + +func (c *statusOnlyClient) Status( + context.Context, + client2.StatusOptions, +) (client2.Status, error) { + return c.status, c.err +} + +func TestWorkspaceTunnelHealthHealthyOnlyWhenRunning(t *testing.T) { + for _, tc := range []struct { + name string + status client2.Status + err error + healthy bool + }{ + {"running", client2.StatusRunning, nil, true}, + {"stopped", client2.StatusStopped, nil, false}, + {"not found", client2.StatusNotFound, nil, false}, + {"provisioning", client2.StatusProvisioning, nil, false}, + {"failed", client2.StatusFailed, nil, false}, + {"busy", client2.StatusBusy, nil, false}, + {"status error", "", errors.New("daemon unreachable"), false}, + } { + t.Run(tc.name, func(t *testing.T) { + health := workspaceTunnelHealth(&statusOnlyClient{status: tc.status, err: tc.err}) + err := health(t.Context()) + if tc.healthy && err != nil { + t.Errorf("health = %v, want nil", err) + } + if !tc.healthy && err == nil { + t.Error("health = nil, want failure") + } + }) + } +} diff --git a/e2e/framework/command.go b/e2e/framework/command.go index 14fe296ad6..9e7cb4a274 100644 --- a/e2e/framework/command.go +++ b/e2e/framework/command.go @@ -32,6 +32,7 @@ const ( cmdList = "list" cmdGet = "get" cmdDelete = "delete" + cmdCancel = "cancel" cmdSSH = "ssh" cmdProvider = "provider" cmdWorkspace = "workspace" @@ -228,10 +229,13 @@ func (f *Framework) DevsySSHOnce( workspace string, command string, ) (string, error) { - out, _, err := f.ExecCommandCapture(ctx, []string{ + out, stderr, err := f.ExecCommandCapture(ctx, []string{ cmdWorkspace, cmdSSH, workspace, flagCommand, command, flagDebug, }) if err != nil { + if stderr != "" { + return "", fmt.Errorf("devsy ssh failed: %w (stderr: %s)", err, stderr) + } return "", fmt.Errorf("devsy ssh failed: %w", err) } return out, nil @@ -478,6 +482,17 @@ func (f *Framework) DevsyWorkspaceStop(ctx context.Context, extraArgs ...string) return f.ExecCommandStdout(ctx, baseArgs) } +// DevsyWorkspaceTaskCancel cancels a detached workspace task. +func (f *Framework) DevsyWorkspaceTaskCancel(ctx context.Context, taskID string) error { + return f.ExecCommandStdout(ctx, []string{cmdWorkspace, "task", cmdCancel, taskID}) +} + +// DevsyWorkspaceTaskList returns the raw `workspace task list` JSON states. +func (f *Framework) DevsyWorkspaceTaskList(ctx context.Context) (string, error) { + return f.ExecCommandOutput(ctx, + []string{cmdWorkspace, "task", cmdList, flagResultFormat, formatJSON}) +} + func (f *Framework) DevsyWorkspaceDelete( ctx context.Context, workspace string, diff --git a/e2e/framework/retry_test.go b/e2e/framework/retry_test.go index 79e319b47f..0ccc08961e 100644 --- a/e2e/framework/retry_test.go +++ b/e2e/framework/retry_test.go @@ -498,6 +498,7 @@ exit 1 _, err = f.DevsySSHOnce(ctx, "test-ws", "echo test") require.Error(t, err) + assert.Contains(t, err.Error(), "Connection refused") // Read invocation counter data, readErr := os.ReadFile(counterFile) //nolint:gosec // G304 diff --git a/e2e/tests/ssh/detached_task.go b/e2e/tests/ssh/detached_task.go new file mode 100644 index 0000000000..2456a6d92b --- /dev/null +++ b/e2e/tests/ssh/detached_task.go @@ -0,0 +1,135 @@ +package ssh + +import ( + "context" + "encoding/json" + "fmt" + "runtime" + "strings" + "time" + + "github.com/devsy-org/devsy/e2e/framework" + "github.com/devsy-org/devsy/pkg/flags/names" + "github.com/onsi/gomega" +) + +// detachedUpState mirrors the `workspace task list` JSON fields the detached +// up specs poll on. +type detachedUpState struct { + ID string `json:"id"` + Status string `json:"status"` + Phase string `json:"phase"` + ErrorCode string `json:"errorCode"` + Error string `json:"error"` + PID int `json:"pid"` +} + +// setupTunnelProvider registers the docker provider; Windows runs use the +// podman runtime installed by CI. +func setupTunnelProvider(ctx context.Context, initialDir string) *framework.Framework { + binDir := initialDir + "/bin" + if runtime.GOOS == osWindows { + f, err := framework.SetupDockerProvider(binDir, "podman") + framework.ExpectNoError(err) + return f + } + f := framework.NewDefaultFramework(binDir) + _ = f.DevsyProviderAdd(ctx, "docker") + framework.ExpectNoError(f.DevsyProviderUse(ctx, "docker")) + return f +} + +func startDetachedTunnelUp( + ctx context.Context, + f *framework.Framework, + workspace string, + extraArgs ...string, +) (string, error) { + args := []string{ + cmdWorkspace, "up", + names.Flag(names.Debug), + names.Flag(names.IDE), "none", + names.Flag(names.SSHTunnel), + names.Flag(names.Detach), + names.Flag(names.ResultFormat), "json", + } + args = append(args, extraArgs...) + args = append(args, workspace) + + out, err := f.ExecCommandOutput(ctx, args) + if err != nil { + return "", err + } + var env struct { + Kind string `json:"kind"` + ID string `json:"id"` + } + line, _, _ := strings.Cut(strings.TrimSpace(out), "\n") + if err := json.Unmarshal([]byte(line), &env); err != nil { + return "", fmt.Errorf("parse task envelope %q: %w", out, err) + } + if env.Kind != "task" || env.ID == "" { + return "", fmt.Errorf("unexpected task envelope %q", line) + } + return env.ID, nil +} + +func findDetachedUpTask( + ctx context.Context, + f *framework.Framework, + taskID string, +) (detachedUpState, error) { + out, err := f.DevsyWorkspaceTaskList(ctx) + if err != nil { + return detachedUpState{}, err + } + var states []detachedUpState + if err := json.Unmarshal([]byte(out), &states); err != nil { + return detachedUpState{}, fmt.Errorf("parse task list: %w", err) + } + for _, s := range states { + if s.ID == taskID { + return s, nil + } + } + return detachedUpState{}, fmt.Errorf("task %s not in task list", taskID) +} + +func waitDetachedTunnelReady( + ctx context.Context, + f *framework.Framework, + taskID string, +) detachedUpState { + var state detachedUpState + gomega.Eventually(func() (string, error) { + var err error + state, err = findDetachedUpTask(ctx, f, taskID) + if err != nil { + return "", err + } + if state.Status == "failed" { + return "", gomega.StopTrying("detached up task failed: " + state.Error) + } + return state.Phase, nil + }).WithContext(ctx).WithTimeout(tunnelActiveTimeout).WithPolling(2 * time.Second). + Should(gomega.Equal("ready")) + return state +} + +func waitDetachedTaskCanceled( + ctx context.Context, + f *framework.Framework, + taskID string, +) detachedUpState { + var state detachedUpState + gomega.Eventually(func() string { + var err error + state, err = findDetachedUpTask(ctx, f, taskID) + if err != nil { + return "" + } + return state.ErrorCode + }).WithContext(ctx).WithTimeout(30 * time.Second).WithPolling(time.Second). + Should(gomega.Equal("canceled")) + return state +} diff --git a/e2e/tests/ssh/ssh_tunnel_mode.go b/e2e/tests/ssh/ssh_tunnel_mode.go index 7692a7b9f7..3e3f4c7a69 100644 --- a/e2e/tests/ssh/ssh_tunnel_mode.go +++ b/e2e/tests/ssh/ssh_tunnel_mode.go @@ -1,110 +1,31 @@ package ssh import ( - "bytes" "context" - "fmt" "net" "os" - "os/exec" "path/filepath" - "runtime" "strings" - "sync" - "syscall" "time" "github.com/devsy-org/devsy/e2e/framework" - "github.com/devsy-org/devsy/pkg/flags/names" "github.com/onsi/ginkgo/v2" "github.com/onsi/gomega" ) -const tunnelActiveMarker = "waiting for shutdown signal" - const tunnelActiveTimeout = 4 * time.Minute -type safeBuffer struct { - mu sync.Mutex - buf bytes.Buffer -} - -func (b *safeBuffer) Write(p []byte) (int, error) { - b.mu.Lock() - defer b.mu.Unlock() - return b.buf.Write(p) -} - -func (b *safeBuffer) String() string { - b.mu.Lock() - defer b.mu.Unlock() - return b.buf.String() -} - -type tunnelUpProcess struct { - cmd *exec.Cmd - output *safeBuffer - exited chan struct{} - exitErr error -} - -func startTunnelUp( - f *framework.Framework, workspace string, extraArgs ...string, -) (*tunnelUpProcess, error) { - args := []string{ - cmdWorkspace, "up", - names.Flag(names.Debug), - names.Flag(names.IDE), "none", - names.Flag(names.SSHTunnel), - } - args = append(args, extraArgs...) - args = append(args, workspace) - - // #nosec G204 -- test binary with controlled arguments - cmd := exec.Command(filepath.Join(f.DevsyBinDir, f.DevsyBinName), args...) - out := &safeBuffer{} - cmd.Stdout = out - cmd.Stderr = out - if err := cmd.Start(); err != nil { - return nil, fmt.Errorf("start devsy up --ssh-tunnel: %w", err) - } - - p := &tunnelUpProcess{cmd: cmd, output: out, exited: make(chan struct{})} - go func() { - p.exitErr = cmd.Wait() - close(p.exited) - }() - return p, nil -} - -func (p *tunnelUpProcess) waitUntilActive(ctx context.Context) { - gomega.Eventually(func() (string, error) { - out := p.output.String() - select { - case <-p.exited: - return out, gomega.StopTrying( - "devsy up exited before the tunnel became active", - ).Wrap(p.exitErr) - default: - return out, nil - } - }).WithContext(ctx).WithTimeout(tunnelActiveTimeout).WithPolling(100 * time.Millisecond). - Should(gomega.ContainSubstring(tunnelActiveMarker)) -} - -func (p *tunnelUpProcess) stop() { - select { - case <-p.exited: - return - default: - } - _ = p.cmd.Process.Signal(syscall.SIGINT) - select { - case <-p.exited: - case <-time.After(15 * time.Second): - _ = p.cmd.Process.Kill() - <-p.exited - } +// readTunnelSSHConfig waits for the worker to write the SSH config: the +// ready phase can win the race against the config write on slower runners. +func readTunnelSSHConfig(ctx context.Context, path string) string { + var configBytes []byte + gomega.Eventually(func() error { + var err error + configBytes, err = os.ReadFile(filepath.Clean(path)) + return err + }).WithContext(ctx).WithTimeout(time.Minute).WithPolling(2 * time.Second). + Should(gomega.Succeed()) + return string(configBytes) } var _ = ginkgo.Describe( @@ -123,68 +44,58 @@ var _ = ginkgo.Describe( ginkgo.It("should start workspace with --ssh-tunnel and SSH into it", ginkgo.SpecTimeout(framework.TimeoutModerate()), func(ctx context.Context) { - if runtime.GOOS == osWindows { - ginkgo.Skip("skipping on windows") - } - tempDir, err := framework.CopyToTempDir("tests/ssh/testdata/local-test") framework.ExpectNoError(err) - f := framework.NewDefaultFramework(initialDir + "/bin") - _ = f.DevsyProviderAdd(ctx, "docker") - err = f.DevsyProviderUse(ctx, "docker") - framework.ExpectNoError(err) + f := setupTunnelProvider(ctx, initialDir) ginkgo.DeferCleanup(func(cleanupCtx context.Context) { _ = f.DevsyWorkspaceDelete(cleanupCtx, tempDir) framework.CleanupTempDir(initialDir, tempDir) }) - proc, err := startTunnelUp(f, tempDir) + taskID, err := startDetachedTunnelUp(ctx, f, tempDir) framework.ExpectNoError(err) - ginkgo.DeferCleanup(proc.stop) + ginkgo.DeferCleanup(func(cleanupCtx context.Context) { + _ = f.DevsyWorkspaceTaskCancel(cleanupCtx, taskID) + }) - proc.waitUntilActive(ctx) + waitDetachedTunnelReady(ctx, f, taskID) - devsySSHCtx, cancelSSH := context.WithDeadline(ctx, time.Now().Add(20*time.Second)) - defer cancelSSH() - err = f.DevsySSHEchoTestString(devsySSHCtx, tempDir) - framework.ExpectNoError(err) + gomega.Eventually(func() (string, error) { + probeCtx, cancel := context.WithTimeout(ctx, 20*time.Second) + defer cancel() + return f.DevsySSHOnce(probeCtx, tempDir, "echo mYtEsTsTrInG") + }).WithContext(ctx).WithTimeout(tunnelActiveTimeout).WithPolling(2 * time.Second). + Should(gomega.ContainSubstring("mYtEsTsTrInG")) }, ) ginkgo.It("should write SSH config with Hostname and Port instead of ProxyCommand", ginkgo.SpecTimeout(framework.TimeoutModerate()), func(ctx context.Context) { - if runtime.GOOS == osWindows { - ginkgo.Skip("skipping on windows") - } - tempDir, err := framework.CopyToTempDir("tests/ssh/testdata/local-test") framework.ExpectNoError(err) sshConfigDir := ginkgo.GinkgoT().TempDir() sshConfigPath := filepath.Join(sshConfigDir, "config") - f := framework.NewDefaultFramework(initialDir + "/bin") - _ = f.DevsyProviderAdd(ctx, "docker") - err = f.DevsyProviderUse(ctx, "docker") - framework.ExpectNoError(err) + f := setupTunnelProvider(ctx, initialDir) ginkgo.DeferCleanup(func(cleanupCtx context.Context) { _ = f.DevsyWorkspaceDelete(cleanupCtx, tempDir) framework.CleanupTempDir(initialDir, tempDir) }) - proc, err := startTunnelUp(f, tempDir, "--ssh-config", sshConfigPath) + taskID, err := startDetachedTunnelUp(ctx, f, tempDir, "--ssh-config", sshConfigPath) framework.ExpectNoError(err) - ginkgo.DeferCleanup(proc.stop) + ginkgo.DeferCleanup(func(cleanupCtx context.Context) { + _ = f.DevsyWorkspaceTaskCancel(cleanupCtx, taskID) + }) - proc.waitUntilActive(ctx) + waitDetachedTunnelReady(ctx, f, taskID) - configBytes, err := os.ReadFile(filepath.Clean(sshConfigPath)) - framework.ExpectNoError(err) - config := string(configBytes) + config := readTunnelSSHConfig(ctx, sshConfigPath) gomega.Expect(config).To( gomega.ContainSubstring("Hostname 127.0.0.1"), @@ -204,35 +115,28 @@ var _ = ginkgo.Describe( ginkgo.It("should establish a working local TCP tunnel listener", ginkgo.SpecTimeout(framework.TimeoutModerate()), func(ctx context.Context) { - if runtime.GOOS == osWindows { - ginkgo.Skip("skipping on windows") - } - tempDir, err := framework.CopyToTempDir("tests/ssh/testdata/local-test") framework.ExpectNoError(err) sshConfigDir := ginkgo.GinkgoT().TempDir() sshConfigPath := filepath.Join(sshConfigDir, "config") - f := framework.NewDefaultFramework(initialDir + "/bin") - _ = f.DevsyProviderAdd(ctx, "docker") - err = f.DevsyProviderUse(ctx, "docker") - framework.ExpectNoError(err) + f := setupTunnelProvider(ctx, initialDir) ginkgo.DeferCleanup(func(cleanupCtx context.Context) { _ = f.DevsyWorkspaceDelete(cleanupCtx, tempDir) framework.CleanupTempDir(initialDir, tempDir) }) - proc, err := startTunnelUp(f, tempDir, "--ssh-config", sshConfigPath) + taskID, err := startDetachedTunnelUp(ctx, f, tempDir, "--ssh-config", sshConfigPath) framework.ExpectNoError(err) - ginkgo.DeferCleanup(proc.stop) + ginkgo.DeferCleanup(func(cleanupCtx context.Context) { + _ = f.DevsyWorkspaceTaskCancel(cleanupCtx, taskID) + }) - proc.waitUntilActive(ctx) + waitDetachedTunnelReady(ctx, f, taskID) - configBytes, err := os.ReadFile(filepath.Clean(sshConfigPath)) - framework.ExpectNoError(err) - config := string(configBytes) + config := readTunnelSSHConfig(ctx, sshConfigPath) var port string for line := range strings.SplitSeq(config, "\n") { @@ -245,48 +149,58 @@ var _ = ginkgo.Describe( gomega.Expect(port).NotTo(gomega.BeEmpty(), "should find Port in SSH config") addr := net.JoinHostPort("127.0.0.1", port) - conn, err := net.DialTimeout("tcp", addr, 5*time.Second) - gomega.Expect(err).NotTo(gomega.HaveOccurred(), - "should be able to connect to local tunnel port", - ) - _ = conn.Close() + gomega.Eventually(func() error { + conn, err := net.DialTimeout("tcp", addr, 5*time.Second) + if err != nil { + return err + } + return conn.Close() + }).WithContext(ctx).WithTimeout(time.Minute).WithPolling(2*time.Second). + Should(gomega.Succeed(), "should be able to connect to local tunnel port") }, ) ginkgo.It("should handle multiple sequential SSH commands via tunnel", ginkgo.SpecTimeout(framework.TimeoutModerate()), func(ctx context.Context) { - if runtime.GOOS == osWindows { - ginkgo.Skip("skipping on windows") - } - tempDir, err := framework.CopyToTempDir("tests/ssh/testdata/local-test") framework.ExpectNoError(err) - f := framework.NewDefaultFramework(initialDir + "/bin") - _ = f.DevsyProviderAdd(ctx, "docker") - err = f.DevsyProviderUse(ctx, "docker") - framework.ExpectNoError(err) + f := setupTunnelProvider(ctx, initialDir) ginkgo.DeferCleanup(func(cleanupCtx context.Context) { _ = f.DevsyWorkspaceDelete(cleanupCtx, tempDir) framework.CleanupTempDir(initialDir, tempDir) }) - proc, err := startTunnelUp(f, tempDir) + taskID, err := startDetachedTunnelUp(ctx, f, tempDir) framework.ExpectNoError(err) - ginkgo.DeferCleanup(proc.stop) + ginkgo.DeferCleanup(func(cleanupCtx context.Context) { + _ = f.DevsyWorkspaceTaskCancel(cleanupCtx, taskID) + }) - proc.waitUntilActive(ctx) + waitDetachedTunnelReady(ctx, f, taskID) - for i := range 3 { - sshCtx, cancelSSH := context.WithDeadline(ctx, time.Now().Add(20*time.Second)) - out, err := f.DevsySSH( - sshCtx, - tempDir, - "echo iteration-"+strings.Repeat("x", i), - ) - cancelSSH() + runSSH := func(i int, budget time.Duration) (string, error) { + sshCtx, cancelSSH := context.WithDeadline(ctx, time.Now().Add(budget)) + defer cancelSSH() + return f.DevsySSH(sshCtx, tempDir, "echo iteration-"+strings.Repeat("x", i)) + } + + // The SSH listener can lag the task's ready phase. + var out string + gomega.Eventually(func() error { + var err error + out, err = runSSH(0, time.Minute) + return err + }).WithContext(ctx).WithTimeout(3 * time.Minute).WithPolling(10 * time.Second). + Should(gomega.Succeed()) + gomega.Expect(out).To( + gomega.ContainSubstring("iteration-"), + "sequential SSH command should succeed", + ) + for i := 1; i < 3; i++ { + out, err := runSSH(i, 20*time.Second) framework.ExpectNoError(err) gomega.Expect(out).To( gomega.ContainSubstring("iteration-"), @@ -299,20 +213,13 @@ var _ = ginkgo.Describe( ginkgo.It("should fall back to ProxyCommand when tunnel mode is not enabled", ginkgo.SpecTimeout(framework.TimeoutModerate()), func(ctx context.Context) { - if runtime.GOOS == osWindows { - ginkgo.Skip("skipping on windows") - } - tempDir, err := framework.CopyToTempDir("tests/ssh/testdata/local-test") framework.ExpectNoError(err) sshConfigDir := ginkgo.GinkgoT().TempDir() sshConfigPath := filepath.Join(sshConfigDir, "config") - f := framework.NewDefaultFramework(initialDir + "/bin") - _ = f.DevsyProviderAdd(ctx, "docker") - err = f.DevsyProviderUse(ctx, "docker") - framework.ExpectNoError(err) + f := setupTunnelProvider(ctx, initialDir) ginkgo.DeferCleanup(func(cleanupCtx context.Context) { _ = f.DevsyWorkspaceDelete(cleanupCtx, tempDir) diff --git a/e2e/tests/ssh/task_cancel.go b/e2e/tests/ssh/task_cancel.go new file mode 100644 index 0000000000..d916641296 --- /dev/null +++ b/e2e/tests/ssh/task_cancel.go @@ -0,0 +1,114 @@ +package ssh + +import ( + "context" + "os" + "strconv" + "time" + + "github.com/devsy-org/devsy/e2e/framework" + "github.com/devsy-org/devsy/pkg/client" + "github.com/devsy-org/devsy/pkg/command" + "github.com/onsi/ginkgo/v2" + "github.com/onsi/gomega" +) + +// stoppedHoldWindow spans the health loop's 30-second, three-failure threshold. +const stoppedHoldWindow = 100 * time.Second + +var _ = ginkgo.Describe( + "devsy workspace task cancel", + ginkgo.Label("task-cancel"), + ginkgo.Ordered, + func() { + var initialDir string + + ginkgo.BeforeEach(func() { + var err error + initialDir, err = os.Getwd() + framework.ExpectNoError(err) + }) + + ginkgo.It("should cancel a detached tunnel task and keep the workspace stopped", + ginkgo.SpecTimeout(framework.TimeoutLong()), + func(ctx context.Context) { + tempDir, err := framework.CopyToTempDir("tests/ssh/testdata/local-test") + framework.ExpectNoError(err) + + f := setupTunnelProvider(ctx, initialDir) + + ginkgo.DeferCleanup(func(cleanupCtx context.Context) { + _ = f.DevsyWorkspaceDelete(cleanupCtx, tempDir) + framework.CleanupTempDir(initialDir, tempDir) + }) + + taskID, err := startDetachedTunnelUp(ctx, f, tempDir) + framework.ExpectNoError(err) + ginkgo.DeferCleanup(func(cleanupCtx context.Context) { + _ = f.DevsyWorkspaceTaskCancel(cleanupCtx, taskID) + }) + + active := waitDetachedTunnelReady(ctx, f, taskID) + gomega.Expect(active.PID).NotTo(gomega.BeZero(), + "detached up worker should publish its pid") + + framework.ExpectNoError(f.DevsyWorkspaceTaskCancel(ctx, taskID)) + + canceled := waitDetachedTaskCanceled(ctx, f, taskID) + gomega.Expect(canceled.Error).To(gomega.Equal("canceled")) + + gomega.Eventually(func() bool { + running, err := command.IsRunning(strconv.Itoa(active.PID)) + return err == nil && !running + }).WithContext(ctx).WithTimeout(30*time.Second).WithPolling(time.Second). + Should(gomega.BeTrue(), + "detached worker process should be gone after cancel") + + framework.ExpectNoError(f.DevsyStop(ctx, tempDir)) + + expectWorkspaceStaysStopped(ctx, f, tempDir) + }, + ) + + ginkgo.It("should quiesce a persisted detached task when stop runs without a task ID", + ginkgo.SpecTimeout(framework.TimeoutLong()), + func(ctx context.Context) { + tempDir, err := framework.CopyToTempDir("tests/ssh/testdata/local-test") + framework.ExpectNoError(err) + + f := setupTunnelProvider(ctx, initialDir) + + ginkgo.DeferCleanup(func(cleanupCtx context.Context) { + _ = f.DevsyWorkspaceDelete(cleanupCtx, tempDir) + framework.CleanupTempDir(initialDir, tempDir) + }) + + taskID, err := startDetachedTunnelUp(ctx, f, tempDir) + framework.ExpectNoError(err) + ginkgo.DeferCleanup(func(cleanupCtx context.Context) { + _ = f.DevsyWorkspaceTaskCancel(cleanupCtx, taskID) + }) + + waitDetachedTunnelReady(ctx, f, taskID) + + framework.ExpectNoError(f.DevsyStop(ctx, tempDir)) + + canceled := waitDetachedTaskCanceled(ctx, f, taskID) + gomega.Expect(canceled.Error).To(gomega.Equal("canceled")) + + expectWorkspaceStaysStopped(ctx, f, tempDir) + }, + ) + }, +) + +func expectWorkspaceStaysStopped(ctx context.Context, f *framework.Framework, workspace string) { + gomega.Consistently(func() string { + status, err := f.DevsyStatus(ctx, workspace) + if err != nil { + return "status error: " + err.Error() + } + return status.State + }).WithContext(ctx).WithTimeout(stoppedHoldWindow).WithPolling(10 * time.Second). + Should(gomega.Equal(client.StatusStopped)) +} diff --git a/pkg/client/clientimplementation/workspace_client.go b/pkg/client/clientimplementation/workspace_client.go index 245fcad443..5a81aeefc6 100644 --- a/pkg/client/clientimplementation/workspace_client.go +++ b/pkg/client/clientimplementation/workspace_client.go @@ -371,7 +371,7 @@ func (s *workspaceClient) taskStatusOverride() (client.Status, bool) { switch { case !latest.Status.Terminal(): return client.StatusProvisioning, true - case latest.Status == task.StatusFailed: + case latest.Status == task.StatusFailed && !latest.Canceled(): return client.StatusFailed, true default: return "", false @@ -394,7 +394,7 @@ func (s *workspaceClient) latestUpTask() *task.State { if latest == nil { return nil } - return store.Reconcile(latest) + return store.ReconcileState(latest) } // newestUpTask picks the most recently started `up` task for workspaceID. diff --git a/pkg/client/clientimplementation/workspace_client_status_test.go b/pkg/client/clientimplementation/workspace_client_status_test.go index 1ccf478b17..1ef5c53c8a 100644 --- a/pkg/client/clientimplementation/workspace_client_status_test.go +++ b/pkg/client/clientimplementation/workspace_client_status_test.go @@ -71,6 +71,18 @@ func TestTaskStatusOverride_FailedTaskReportsFailed(t *testing.T) { assertOverride(t, s, true, client.StatusFailed) } +func TestTaskStatusOverride_CanceledTaskDefersToWorkspaceStatus(t *testing.T) { + useTempTaskDir(t) + store, err := task.NewStore() + require.NoError(t, err) + tsk, err := store.Create(task.CreateOptions{Command: "up", WorkspaceID: testWorkspaceID}) + require.NoError(t, err) + require.NoError(t, tsk.Cancel()) + + s := &workspaceClient{providerWorkspace: &provider.Workspace{ID: testWorkspaceID}} + assertOverride(t, s, false, "") +} + func TestTaskStatusOverride_SucceededTaskDefersToContainerStatus(t *testing.T) { useTempTaskDir(t) store, err := task.NewStore() @@ -131,7 +143,7 @@ func TestTaskStatusOverride_AbandonedTaskReportsFailed(t *testing.T) { // Claim then release the worker lock: the kernel frees a dead worker's // lock the same way, which is what marks the task abandoned. require.NoError(t, tk.HoldWorkerLock()) - require.NoError(t, tk.ReleaseWorkerLockForTest()) + require.NoError(t, tk.ReleaseWorkerLock()) s := &workspaceClient{providerWorkspace: &provider.Workspace{ID: testWorkspaceID}} assertOverride(t, s, true, client.StatusFailed) @@ -151,7 +163,7 @@ func TestTaskStatusOverride_LiveWorkerStillReportsProvisioning(t *testing.T) { require.NoError(t, tk.HoldWorkerLock()) // Released on cleanup so the fd closes even if an assertion fails; an open // file can block TempDir removal on some platforms. - t.Cleanup(func() { require.NoError(t, tk.ReleaseWorkerLockForTest()) }) + t.Cleanup(func() { require.NoError(t, tk.ReleaseWorkerLock()) }) s := &workspaceClient{providerWorkspace: &provider.Workspace{ID: testWorkspaceID}} assertOverride(t, s, true, client.StatusProvisioning) diff --git a/pkg/command/background.go b/pkg/command/background.go index 70f97098d1..bd40a55b59 100644 --- a/pkg/command/background.go +++ b/pkg/command/background.go @@ -13,6 +13,11 @@ import ( type CreateCommand func() (*exec.Cmd, error) +type SupervisedStartOptions struct { + Name string + OnStarted func(ProcessRef) error +} + // StartBackgroundOnce starts a background process, ensuring only one instance // with the given commandName runs at a time. If a process is already running // (determined by PID file), or the lock cannot be acquired, it returns nil. @@ -101,6 +106,26 @@ func StartBackground(commandName string, createCommand CreateCommand) error { return startDetached(cmd, "", streamsFile) } +// StartSupervisedBackground starts a detached worker with an owned process +// tree and publishes its reference before the launch is considered complete. +func StartSupervisedBackground(opts SupervisedStartOptions, createCommand CreateCommand) error { + if opts.Name == "" { + return errors.New("supervised process name is required") + } + if opts.OnStarted == nil { + return errors.New("supervised process publication callback is required") + } + streamsFile, err := config.DefaultPathManager().ProcessStreamsFile(opts.Name) + if err != nil { + return fmt.Errorf("process streams file: %w", err) + } + cmd, err := createCommand() + if err != nil { + return err + } + return startSupervisedDetached(cmd, opts, streamsFile) +} + func startCommand(cmd *exec.Cmd, pidFile, streamsFile string) error { return startDetached(cmd, pidFile, streamsFile) } @@ -130,6 +155,45 @@ func startDetached(cmd *exec.Cmd, pidFile, streamsFile string) error { return nil } +func startSupervisedDetached(cmd *exec.Cmd, opts SupervisedStartOptions, streamsFile string) error { + streamsF, err := openStreamsFile(cmd, streamsFile) + if err != nil { + return err + } + prepareBackgroundTree(cmd) + if err := cmd.Start(); err != nil { + closeFile(streamsF) + return fmt.Errorf("start process: %w", err) + } + closeFile(streamsF) + pid := cmd.Process.Pid + cleanup := func() { + _ = abortSupervisedLaunch(pid, opts.Name) + _ = cmd.Wait() + } + if err := ownProcessTree(pid, opts.Name); err != nil { + cleanup() + return fmt.Errorf("assign process tree ownership for pid %d: %w", pid, err) + } + ref, err := processRefForStartedProcess(pid, opts.Name) + if err != nil { + cleanup() + return fmt.Errorf("capture process reference for pid %d: %w", pid, err) + } + if err := opts.OnStarted(ref); err != nil { + _ = terminateProcessRef(ref) + cleanup() + return fmt.Errorf("publish process reference for pid %d: %w", pid, err) + } + if err := resumeBackgroundTree(pid); err != nil { + _ = terminateProcessRef(ref) + cleanup() + return fmt.Errorf("resume process %d: %w", pid, err) + } + _ = cmd.Process.Release() + return nil +} + func isProcessRunning(pidFile string) (bool, error) { pid, err := os.ReadFile(pidFile) // #nosec G304: not user input if err != nil { diff --git a/pkg/command/job_windows.go b/pkg/command/job_windows.go new file mode 100644 index 0000000000..57bdc4156f --- /dev/null +++ b/pkg/command/job_windows.go @@ -0,0 +1,423 @@ +//go:build windows + +package command + +import ( + "crypto/sha256" + "encoding/hex" + "errors" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "syscall" + "time" + "unsafe" + + "github.com/devsy-org/devsy/pkg/config" + "golang.org/x/sys/windows" +) + +const jobObjectTerminateAccess = 0x0008 + +const jobObjectQueryAccess = 0x0004 + +const jobTerminationTimeout = 5 * time.Second + +const detachedTaskWorkerPrefix = "devsy-up-" + +type jobAccountingInfo struct { + totalUserTime int64 + totalKernelTime int64 + thisPeriodTotalUserTime int64 + thisPeriodTotalKernelTime int64 + totalPageFaultCount uint32 + totalProcesses uint32 + activeProcesses uint32 + totalTerminatedProcesses uint32 +} + +var ( + procCreateJobObjectForWorker = windows.NewLazySystemDLL("kernel32.dll"). + NewProc("CreateJobObjectW") + procOpenJobObjectW = windows.NewLazySystemDLL("kernel32.dll"). + NewProc("OpenJobObjectW") + procIsProcessInJobForTermination = windows.NewLazySystemDLL("kernel32.dll"). + NewProc("IsProcessInJob") +) + +func jobNameFor(name string) (string, error) { + dataDir := os.Getenv(config.EnvHome) + if dataDir == "" { + localAppData := os.Getenv("LOCALAPPDATA") + if localAppData == "" { + return "", errors.New( + "resolve Devsy data directory for job object: LOCALAPPDATA is not set", + ) + } + dataDir = filepath.Join(localAppData, config.RepoName) + } + canonicalDir, err := canonicalDirectoryPath(dataDir) + if err != nil { + return "", fmt.Errorf("resolve Devsy data directory for job object: %w", err) + } + dataDirHash := sha256.Sum256([]byte(canonicalDir)) + return "devsy-" + hex.EncodeToString(dataDirHash[:8]) + "-" + name, nil +} + +func canonicalDirectoryPath(path string) (string, error) { + absPath, err := filepath.Abs(path) + if err != nil { + return "", err + } + pathPtr, err := windows.UTF16PtrFromString(absPath) + if err != nil { + return "", err + } + handle, err := windows.CreateFile( + pathPtr, + windows.FILE_READ_ATTRIBUTES, + windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE, + nil, + windows.OPEN_EXISTING, + windows.FILE_FLAG_BACKUP_SEMANTICS, + 0, + ) + if err != nil { + if errors.Is(err, windows.ERROR_FILE_NOT_FOUND) || + errors.Is(err, windows.ERROR_PATH_NOT_FOUND) { + return normalizedWindowsPath(absPath), nil + } + return "", err + } + defer func() { _ = windows.CloseHandle(handle) }() + + const maxPath = 32768 + buffer := make([]uint16, maxPath) + length, err := windows.GetFinalPathNameByHandle(handle, &buffer[0], uint32(len(buffer)), 0) + if err != nil { + return "", err + } + if length >= uint32(len(buffer)) { + return "", fmt.Errorf("canonical directory path exceeds %d UTF-16 code units", len(buffer)) + } + return windows.UTF16ToString(buffer[:length]), nil +} + +func normalizedWindowsPath(path string) string { + path = filepath.Clean(path) + if strings.HasPrefix(path, `\\?\`) { + return path + } + if strings.HasPrefix(path, `\\`) { + return `\\?\UNC\` + strings.TrimPrefix(path, `\\`) + } + return `\\?\` + path +} + +func prepareBackgroundTree(cmd *exec.Cmd) { + if cmd.SysProcAttr == nil { + cmd.SysProcAttr = &syscall.SysProcAttr{} + } + cmd.SysProcAttr.CreationFlags |= windows.CREATE_SUSPENDED +} + +// resumeBackgroundTree lets the suspended worker run after job assignment. +func resumeBackgroundTree(pid int) error { + snapshot, err := windows.CreateToolhelp32Snapshot(windows.TH32CS_SNAPTHREAD, 0) + if err != nil { + return fmt.Errorf("snapshot process threads: %w", err) + } + defer func() { _ = windows.CloseHandle(snapshot) }() + + entry := windows.ThreadEntry32{Size: uint32(unsafe.Sizeof(windows.ThreadEntry32{}))} + if err := windows.Thread32First(snapshot, &entry); err != nil { + return fmt.Errorf("find primary thread for process %d: %w", pid, err) + } + for { + if entry.OwnerProcessID == uint32(pid) { + thread, err := windows.OpenThread(windows.THREAD_SUSPEND_RESUME, false, entry.ThreadID) + if err != nil { + return fmt.Errorf("open primary thread %d: %w", entry.ThreadID, err) + } + defer func() { _ = windows.CloseHandle(thread) }() + + previous, err := windows.ResumeThread(thread) + if err != nil { + return fmt.Errorf("resume primary thread %d: %w", entry.ThreadID, err) + } + if previous != 1 { + return fmt.Errorf( + "primary thread %d suspend count was %d, want 1", + entry.ThreadID, + previous, + ) + } + return nil + } + if err := windows.Thread32Next(snapshot, &entry); err != nil { + return fmt.Errorf("find primary thread for process %d: %w", pid, err) + } + } +} + +// ownProcessTree assigns the suspended worker before it can spawn descendants, +// keeping the whole tree in the Job Object. +func ownProcessTree(pid int, workerName string) error { + if _, err := config.DefaultPathManager().DataDir(); err != nil { + return fmt.Errorf("resolve Devsy data directory for job object: %w", err) + } + jobName, err := jobNameFor(workerName) + if err != nil { + return err + } + name, err := windows.UTF16PtrFromString(jobName) + if err != nil { + return err + } + job, err := createWorkerJobObject(name) + if err != nil { + return err + } + defer func() { _ = windows.CloseHandle(job) }() + + // The worker keeps the name open for cancellation; its exit also closes + // the last handle, which terminates any descendants it leaves behind. + var limits windows.JOBOBJECT_EXTENDED_LIMIT_INFORMATION + limits.BasicLimitInformation.LimitFlags = windows.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE + if _, err := windows.SetInformationJobObject( + job, + windows.JobObjectExtendedLimitInformation, + uintptr(unsafe.Pointer(&limits)), + uint32(unsafe.Sizeof(limits)), + ); err != nil { + return fmt.Errorf("configure worker job %s: %w", workerName, err) + } + + handle, err := windows.OpenProcess( + windows.PROCESS_SET_QUOTA|windows.PROCESS_TERMINATE|windows.PROCESS_DUP_HANDLE, + false, + uint32(pid), + ) + if err != nil { + return fmt.Errorf("open process %d: %w", pid, err) + } + defer func() { _ = windows.CloseHandle(handle) }() + + if err := windows.AssignProcessToJobObject(job, handle); err != nil { + return fmt.Errorf("assign process %d to job: %w", pid, err) + } + var workerJob windows.Handle + if err := windows.DuplicateHandle( + windows.CurrentProcess(), + job, + handle, + &workerJob, + 0, + false, + windows.DUPLICATE_SAME_ACCESS, + ); err != nil { + return fmt.Errorf("keep worker job open in process %d: %w", pid, err) + } + return nil +} + +func createWorkerJobObject(name *uint16) (windows.Handle, error) { + rawHandle, _, callErr := procCreateJobObjectForWorker.Call( + 0, + uintptr(unsafe.Pointer(name)), + ) + job := windows.Handle(rawHandle) + if job == 0 { + return 0, fmt.Errorf("create job object: %w", callErr) + } + if errors.Is(callErr, windows.ERROR_ALREADY_EXISTS) { + _ = windows.CloseHandle(job) + return 0, fmt.Errorf("job object %q already exists", windows.UTF16PtrToString(name)) + } + return job, nil +} + +// terminateJob kills the tree owned by the worker's Job Object. The bool +// reports whether a job existed; without one the caller falls back to +// taskkill, e.g. for workers launched before job ownership existed. +func terminateJob(workerName string) (bool, error) { + job, found, err := openWorkerJob(workerName) + if err != nil || !found { + return found, err + } + defer func() { _ = windows.CloseHandle(job) }() + return terminateJobHandle(job, workerName) +} + +func terminateJobAfterWorkerExit(workerName string) (bool, error) { + job, found, err := openWorkerJob(workerName) + if err != nil { + return false, err + } + if !found && isDetachedTaskWorkerName(workerName) { + job, found, err = openLegacyWorkerJob(workerName) + if err != nil { + return false, err + } + } + if !found { + return false, nil + } + defer func() { _ = windows.CloseHandle(job) }() + return terminateJobHandle(job, workerName) +} + +func terminateNamedJob(jobID string) (bool, error) { + if jobID == "" { + return false, errors.New("worker job reference is empty") + } + job, found, err := openNamedJob(jobID, jobID) + if err != nil || !found { + return found, err + } + defer func() { _ = windows.CloseHandle(job) }() + return terminateJobHandle(job, jobID) +} + +func isDetachedTaskWorkerName(workerName string) bool { + if !strings.HasPrefix(workerName, detachedTaskWorkerPrefix) { + return false + } + taskID := strings.TrimPrefix(workerName, detachedTaskWorkerPrefix) + if len(taskID) != 12 { + return false + } + for _, char := range taskID { + if !(char >= 'a' && char <= 'z') && !(char >= 'A' && char <= 'Z') && + !(char >= '0' && char <= '9') { + return false + } + } + return true +} + +func terminateJobForPID(workerName string, pid int) (bool, error) { + job, found, err := openWorkerJob(workerName) + if err != nil { + return false, err + } + if found { + inJob, verifyErr := processBelongsToJob(pid, job) + if verifyErr != nil { + _ = windows.CloseHandle(job) + if errors.Is(verifyErr, windows.ERROR_INVALID_PARAMETER) { + return false, fmt.Errorf( + "worker process %d exited before job membership was verified", + pid, + ) + } + return false, verifyErr + } + if inJob { + defer func() { _ = windows.CloseHandle(job) }() + return terminateJobHandle(job, workerName) + } + _ = windows.CloseHandle(job) + } + job, found, err = openLegacyWorkerJob(workerName) + if err != nil || !found { + return found, err + } + defer func() { _ = windows.CloseHandle(job) }() + inJob, err := processBelongsToJob(pid, job) + if errors.Is(err, windows.ERROR_INVALID_PARAMETER) { + return false, fmt.Errorf("worker process %d exited before job membership was verified", pid) + } + if err != nil { + return false, err + } + if !inJob { + return false, fmt.Errorf("process %d does not belong to worker job %s", pid, workerName) + } + return terminateJobHandle(job, workerName) +} + +func processBelongsToJob(pid int, job windows.Handle) (bool, error) { + process, err := windows.OpenProcess( + windows.PROCESS_QUERY_LIMITED_INFORMATION, + false, + uint32(pid), + ) + if err != nil { + return false, fmt.Errorf("open worker process %d: %w", pid, err) + } + defer func() { _ = windows.CloseHandle(process) }() + var inJob int32 + result, _, callErr := procIsProcessInJobForTermination.Call( + uintptr(process), + uintptr(job), + uintptr(unsafe.Pointer(&inJob)), + ) + if result == 0 { + return false, fmt.Errorf("verify process %d in job: %w", pid, callErr) + } + return inJob != 0, nil +} + +func openWorkerJob(workerName string) (windows.Handle, bool, error) { + jobName, err := jobNameFor(workerName) + if err != nil { + return 0, false, err + } + return openNamedJob(jobName, workerName) +} + +func openLegacyWorkerJob(workerName string) (windows.Handle, bool, error) { + return openNamedJob(workerName, workerName) +} + +func openNamedJob(jobName, workerName string) (windows.Handle, bool, error) { + name, err := windows.UTF16PtrFromString(jobName) + if err != nil { + return 0, false, err + } + handle, _, callErr := procOpenJobObjectW.Call( + uintptr(jobObjectTerminateAccess|jobObjectQueryAccess), + 0, + uintptr(unsafe.Pointer(name)), + ) + if handle == 0 { + if errors.Is(callErr, windows.ERROR_FILE_NOT_FOUND) { + return 0, false, nil + } + return 0, false, fmt.Errorf("open job %s: %w", workerName, callErr) + } + return windows.Handle(handle), true, nil +} + +func terminateJobHandle(job windows.Handle, workerName string) (bool, error) { + if err := windows.TerminateJobObject(job, 1); err != nil { + return false, fmt.Errorf("terminate job %s: %w", workerName, err) + } + deadline := time.Now().Add(jobTerminationTimeout) + for { + var info jobAccountingInfo + if err := windows.QueryInformationJobObject( + job, + windows.JobObjectBasicAccountingInformation, + uintptr(unsafe.Pointer(&info)), + uint32(unsafe.Sizeof(info)), + nil, + ); err != nil { + return false, fmt.Errorf("query job %s processes: %w", workerName, err) + } + if info.activeProcesses == 0 { + return true, nil + } + if time.Now().After(deadline) { + return false, fmt.Errorf( + "job %s still has %d active processes after termination", + workerName, + info.activeProcesses, + ) + } + time.Sleep(50 * time.Millisecond) + } +} diff --git a/pkg/command/process.go b/pkg/command/process.go index 4eae843f56..c58c6f3172 100644 --- a/pkg/command/process.go +++ b/pkg/command/process.go @@ -1,9 +1,66 @@ package command +// ProcessTreeKind identifies the platform primitive that owns a worker tree. +type ProcessTreeKind string + +const ( + ProcessTreeLegacyPID ProcessTreeKind = "legacy-pid" + ProcessTreeUnixGroup ProcessTreeKind = "unix-group-v1" + ProcessTreeWindowsJob ProcessTreeKind = "windows-job-v1" +) + +// ProcessRef is the persisted reference used to supervise a detached worker. +// TreeID and Identity are opaque outside this package. +type ProcessRef struct { + PID int `json:"pid,omitempty"` + TreeKind ProcessTreeKind `json:"treeKind,omitempty"` + TreeID string `json:"treeId,omitempty"` + Identity string `json:"identity,omitempty"` +} + +// ProcessController provides the two operations needed by task cancellation. +type ProcessController interface { + Terminate(ProcessRef) error + CleanupAfterExit(ProcessRef) error +} + +type processController struct{} + +func DefaultProcessController() ProcessController { return processController{} } + +func (processController) Terminate(ref ProcessRef) error { + return terminateProcessRef(ref) +} + +func (processController) CleanupAfterExit(ref ProcessRef) error { + return cleanupExitedProcessRef(ref) +} + func IsRunning(pid string) (bool, error) { return isRunning(pid) } func Kill(pid string) error { - return kill(pid) + return killTree(pid, "") +} + +// KillTree terminates the process and everything it spawned: the job or +// process group treeName identifies when set, otherwise the process alone. +func KillTree(pid, treeName string) error { + return killTree(pid, treeName) +} + +// KillTreeWithIdentity terminates a worker tree using its saved platform identity. +func KillTreeWithIdentity(pid, treeName, identity string) error { + return killTreeWithIdentity(pid, treeName, identity) +} + +// KillTreeAfterWorkerExit terminates descendants using the worker's saved session identity. +func KillTreeAfterWorkerExit(pid, treeName, identity string) error { + return killTreeAfterWorkerExit(pid, treeName, identity) +} + +// ProcessTreeIdentity returns the kernel identity used to distinguish a reused process ID. +func ProcessTreeIdentity(pid int) (string, error) { + return processTreeIdentity(pid) } diff --git a/pkg/command/process_identity_darwin.go b/pkg/command/process_identity_darwin.go new file mode 100644 index 0000000000..17d0657dab --- /dev/null +++ b/pkg/command/process_identity_darwin.go @@ -0,0 +1,123 @@ +//go:build darwin + +package command + +import ( + "errors" + "fmt" + "strconv" + "strings" + "syscall" + + "golang.org/x/sys/unix" +) + +const strongProcessIdentitySupported = true + +func processTreeIdentity(pid int) (string, error) { + sessionID, err := darwinSessionID(pid) + if err != nil { + if processLookupConfirmsExit(pid, err) { + return "", nil + } + return "", err + } + process, err := unix.SysctlKinfoProc("kern.proc.pid", pid) + if err != nil { + if processLookupConfirmsExit(pid, err) { + return "", nil + } + return "", err + } + return fmt.Sprintf( + "%d:%d:%d", + sessionID, + process.Proc.P_starttime.Sec, + process.Proc.P_starttime.Usec, + ), nil +} + +func processLookupConfirmsExit(pid int, lookupErr error) bool { + if !errors.Is(lookupErr, syscall.ESRCH) && !errors.Is(lookupErr, syscall.EIO) { + return false + } + return errors.Is(syscall.Kill(pid, 0), syscall.ESRCH) +} + +func processGroupMatchesIdentity(pgid int, identity string) (bool, error) { + identityParts := strings.Split(identity, ":") + if len(identityParts) != 3 { + return false, fmt.Errorf("parse process tree identity %q", identity) + } + wantSession, err := strconv.Atoi(identityParts[0]) + if err != nil { + return false, fmt.Errorf("parse session identity %q: %w", identity, err) + } + for _, part := range identityParts[1:] { + if _, err := strconv.ParseInt(part, 10, 64); err != nil { + return false, fmt.Errorf("parse process start identity %q: %w", identity, err) + } + } + processes, err := unix.SysctlKinfoProcSlice("kern.proc.all") + if err != nil { + return false, err + } + return darwinProcessGroupHasSession(processes, pgid, wantSession, identity) +} + +func darwinProcessGroupHasSession( + processes []unix.KinfoProc, + pgid, wantSession int, + identity string, +) (bool, error) { + foundMember := false + for _, process := range processes { + matches, leaderMismatch, err := darwinProcessGroupMemberMatches( + process, + pgid, + wantSession, + identity, + ) + if err != nil { + return false, err + } + if leaderMismatch { + return false, nil + } + foundMember = foundMember || matches + } + return foundMember, nil +} + +func darwinProcessGroupMemberMatches( + process unix.KinfoProc, + pgid, wantSession int, + identity string, +) (matches, leaderMismatch bool, err error) { + processGroupID := int(process.Eproc.Pgid) + if processGroupID != pgid { + return false, false, nil + } + pid := int(process.Proc.P_pid) + sessionID, err := darwinSessionID(pid) + if errors.Is(err, syscall.ESRCH) { + return false, false, nil + } + if err != nil { + return false, false, err + } + if pid == pgid { + leaderIdentity, err := processTreeIdentity(pid) + if err != nil { + return false, false, err + } + if leaderIdentity != "" && leaderIdentity != identity { + return false, true, nil + } + } + return sessionID == wantSession, false, nil +} + +func darwinSessionID(pid int) (int, error) { + return unix.Getsid(pid) +} diff --git a/pkg/command/process_identity_linux.go b/pkg/command/process_identity_linux.go new file mode 100644 index 0000000000..2ec7e6b6ea --- /dev/null +++ b/pkg/command/process_identity_linux.go @@ -0,0 +1,265 @@ +//go:build linux + +package command + +import ( + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + "strconv" + "strings" + "syscall" +) + +const strongProcessIdentitySupported = true + +func processTreeIdentity(pid int) (string, error) { + fields, ok, err := linuxProcessStat(pid) + if err != nil { + return "", err + } + if !ok { + return "", nil + } + if len(fields) < 20 { + return "", fmt.Errorf("read session identity for process %d", pid) + } + return fields[3] + ":" + fields[19], nil +} + +func processGroupMatchesIdentity(pgid int, identity string) (bool, error) { + wanted, err := parseLinuxProcessTreeIdentity(identity) + if err != nil { + return false, err + } + leaderMismatch, err := linuxProcessGroupLeaderMismatch(pgid, wanted) + if err != nil { + return false, err + } + if leaderMismatch { + return false, nil + } + entries, err := os.ReadDir("/proc") + if err != nil { + return false, err + } + scan, err := linuxProcessGroupEntriesMatch( + entries, + pgid, + wanted, + ) + if err != nil { + return false, err + } + if scan.leaderMismatch { + return false, nil + } + return linuxProcessGroupMatchResult(pgid, scan.foundMember, scan.skippedStatErr) +} + +func linuxProcessGroupLeaderMismatch( + pgid int, + wanted linuxProcessTreeIdentity, +) (bool, error) { + fields, found, err := linuxProcessStat(pgid) + if err != nil || !found { + return false, err + } + _, mismatch, err := linuxProcessGroupFieldsMatch(pgid, pgid, wanted, fields) + return mismatch, err +} + +type linuxProcessGroupScan struct { + foundMember bool + leaderMismatch bool + skippedStatErr error +} + +func linuxProcessGroupEntriesMatch( + entries []os.DirEntry, + pgid int, + wanted linuxProcessTreeIdentity, +) (linuxProcessGroupScan, error) { + var scan linuxProcessGroupScan + for _, entry := range entries { + member, mismatchedLeader, err := linuxProcessGroupEntryMatches(entry, pgid, wanted) + if err != nil { + if linuxProcessGroupEntryErrorCanBeSkipped(entry, pgid, err) { + scan.skippedStatErr = err + continue + } + return scan, err + } + if mismatchedLeader { + scan.leaderMismatch = true + return scan, nil + } + scan.foundMember = scan.foundMember || member + } + return scan, nil +} + +func linuxProcessGroupEntryErrorCanBeSkipped( + entry os.DirEntry, + pgid int, + err error, +) bool { + pid, parseErr := strconv.Atoi(entry.Name()) + return parseErr == nil && linuxCanSkipProcessStatError(pid, pgid, err) +} + +func linuxCanSkipProcessStatError(pid, pgid int, err error) bool { + return pid != pgid && errors.Is(err, fs.ErrPermission) +} + +func linuxProcessGroupMatchResult(pgid int, foundMember bool, skippedStatErr error) (bool, error) { + if skippedStatErr != nil && !foundMember { + if errors.Is(syscall.Kill(-pgid, 0), syscall.ESRCH) { + return false, nil + } + return false, fmt.Errorf( + "verify process group %d after unreadable process stats: %w", + pgid, + skippedStatErr, + ) + } + return foundMember, nil +} + +type linuxProcessTreeIdentity struct { + sessionID int + startTime uint64 +} + +func parseLinuxProcessTreeIdentity(identity string) (linuxProcessTreeIdentity, error) { + identityParts := strings.Split(identity, ":") + if len(identityParts) != 2 { + return linuxProcessTreeIdentity{}, fmt.Errorf("parse process tree identity %q", identity) + } + wanted := linuxProcessTreeIdentity{} + var err error + wanted.sessionID, err = strconv.Atoi(identityParts[0]) + if err != nil { + return linuxProcessTreeIdentity{}, fmt.Errorf( + "parse session identity %q: %w", + identity, + err, + ) + } + wanted.startTime, err = strconv.ParseUint(identityParts[1], 10, 64) + if err != nil { + return linuxProcessTreeIdentity{}, fmt.Errorf( + "parse process start identity %q: %w", + identity, + err, + ) + } + return wanted, nil +} + +// A live group leader must still have the saved start time as well as the session ID. +func linuxProcessGroupEntryMatches( + entry fs.DirEntry, + pgid int, + wanted linuxProcessTreeIdentity, +) (member, leaderMismatch bool, err error) { + pid, err := strconv.Atoi(entry.Name()) + if err != nil { + return false, false, nil + } + fields, ok, err := linuxProcessStat(pid) + if err != nil { + return false, false, err + } + if !ok || len(fields) < 4 { + return false, false, nil + } + return linuxProcessGroupFieldsMatch(pid, pgid, wanted, fields) +} + +func linuxProcessGroupFieldsMatch( + pid, pgid int, + wanted linuxProcessTreeIdentity, + fields []string, +) (member, leaderMismatch bool, err error) { + processGroup, sessionID, ok := linuxProcessGroupIDs(fields) + if !ok || processGroup != pgid { + return false, false, nil + } + if pid == pgid { + leaderMismatch, err = linuxProcessGroupFieldsLeaderMismatch(pid, sessionID, fields, wanted) + } + if err != nil || leaderMismatch { + return false, leaderMismatch, err + } + if fields[0] == "Z" || fields[0] == "X" { + return false, false, nil + } + return sessionID == wanted.sessionID, false, nil +} + +func linuxProcessGroupIDs(fields []string) (processGroup, sessionID int, ok bool) { + if len(fields) < 4 { + return 0, 0, false + } + processGroup, err := strconv.Atoi(fields[2]) + if err != nil { + return 0, 0, false + } + sessionID, err = strconv.Atoi(fields[3]) + return processGroup, sessionID, err == nil +} + +func linuxProcessGroupFieldsLeaderMismatch( + pid, sessionID int, + fields []string, + wanted linuxProcessTreeIdentity, +) (bool, error) { + matches, err := linuxProcessLeaderMatches(pid, sessionID, fields, wanted) + return !matches, err +} + +func linuxProcessStat(pid int) ([]string, bool, error) { + stat, err := os.ReadFile(filepath.Join("/proc", strconv.Itoa(pid), "stat")) + if err != nil { + if linuxProcessStatErrorIsAbsent(err) { + return nil, false, nil + } + return nil, false, err + } + fields, ok := linuxProcessStatFields(stat) + if !ok { + return nil, true, fmt.Errorf("parse process stat for process %d", pid) + } + return fields, true, nil +} + +func linuxProcessStatErrorIsAbsent(err error) bool { + return errors.Is(err, syscall.ESRCH) || os.IsNotExist(err) +} + +func linuxProcessLeaderMatches( + pid, sessionID int, + fields []string, + wanted linuxProcessTreeIdentity, +) (bool, error) { + if len(fields) < 20 { + return false, fmt.Errorf("read process start time for process %d", pid) + } + leaderStart, err := strconv.ParseUint(fields[19], 10, 64) + if err != nil { + return false, fmt.Errorf("parse process start time for process %d: %w", pid, err) + } + return sessionID == wanted.sessionID && leaderStart == wanted.startTime, nil +} + +func linuxProcessStatFields(stat []byte) ([]string, bool) { + statText := string(stat) + closingParen := strings.LastIndexByte(statText, ')') + if closingParen < 0 || closingParen+2 >= len(statText) { + return nil, false + } + return strings.Fields(statText[closingParen+2:]), true +} diff --git a/pkg/command/process_identity_linux_test.go b/pkg/command/process_identity_linux_test.go new file mode 100644 index 0000000000..564f59ba46 --- /dev/null +++ b/pkg/command/process_identity_linux_test.go @@ -0,0 +1,151 @@ +//go:build linux + +package command + +import ( + "os" + "os/exec" + "syscall" + "testing" +) + +const processStatOpenOperation = "open" + +func TestLinuxProcessStatErrorIsAbsent(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + { + name: "process disappeared", + err: &os.PathError{Op: processStatOpenOperation, Err: syscall.ESRCH}, + want: true, + }, + { + name: "missing proc entry", + err: &os.PathError{Op: processStatOpenOperation, Err: os.ErrNotExist}, + want: true, + }, + { + name: "permission denied", + err: &os.PathError{Op: processStatOpenOperation, Err: os.ErrPermission}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + if got := linuxProcessStatErrorIsAbsent(test.err); got != test.want { + t.Fatalf( + "linuxProcessStatErrorIsAbsent(%v) = %t, want %t", + test.err, + got, + test.want, + ) + } + }) + } +} + +func TestLinuxCanSkipUnrelatedUnreadableProcess(t *testing.T) { + permissionErr := &os.PathError{Op: processStatOpenOperation, Err: os.ErrPermission} + tests := []struct { + name string + pid int + pgid int + want bool + }{ + { + name: "unrelated process with verified leader", + pid: 12, + pgid: 7, + want: true, + }, + {name: "worker leader is unreadable", pid: 7, pgid: 7}, + {name: "leader exited", pid: 12, pgid: 7, want: true}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got := linuxCanSkipProcessStatError( + test.pid, + test.pgid, + permissionErr, + ) + if got != test.want { + t.Fatalf("linuxCanSkipProcessStatError() = %t, want %t", got, test.want) + } + }) + } +} + +func TestLinuxProcessGroupMatchRequiresAMemberWhenStatsAreUnreadable(t *testing.T) { + permissionErr := &os.PathError{Op: processStatOpenOperation, Err: os.ErrPermission} + pgid := startLinuxProcessGroupHelper(t) + matched, err := linuxProcessGroupMatchResult(pgid, true, permissionErr) + if err != nil || !matched { + t.Fatalf( + "verified group with unreadable unrelated process = (%t, %v), want (true, nil)", + matched, + err, + ) + } + matched, err = linuxProcessGroupMatchResult(pgid, false, permissionErr) + if err == nil || matched { + t.Fatalf("unverified group with unreadable process = (%t, %v), want error", matched, err) + } +} + +func TestLinuxProcessGroupMatchTreatsMissingGroupAsGone(t *testing.T) { + permissionErr := &os.PathError{Op: processStatOpenOperation, Err: os.ErrPermission} + const largestPID = int(^uint32(0) >> 1) + + matched, err := linuxProcessGroupMatchResult(largestPID, false, permissionErr) + if err != nil || matched { + t.Fatalf("missing process group = (%t, %v), want (false, nil)", matched, err) + } +} + +func TestLinuxProcessGroupFieldsIgnoreExitedMembers(t *testing.T) { + wanted := linuxProcessTreeIdentity{sessionID: 7, startTime: 42} + for _, state := range []string{"Z", "X"} { + member, mismatch, err := linuxProcessGroupFieldsMatch( + 12, 10, wanted, []string{state, "1", "10", "7"}, + ) + if err != nil || member || mismatch { + t.Fatalf("state %s = (%t, %t, %v), want no live member", state, member, mismatch, err) + } + } +} + +func TestLinuxProcessGroupProbeHelper(t *testing.T) { + if os.Getenv("DEVSY_PROCESS_GROUP_HELPER") != "1" { + return + } + select {} +} + +func startLinuxProcessGroupHelper(t *testing.T) int { + t.Helper() + // #nosec G204,G702 -- run this test binary with a fixed test selector. + cmd := exec.Command(os.Args[0], "-test.run=^TestLinuxProcessGroupProbeHelper$") + cmd.Env = append(os.Environ(), "DEVSY_PROCESS_GROUP_HELPER=1") + cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true} + if err := cmd.Start(); err != nil { + t.Fatalf("start process group helper: %v", err) + } + t.Cleanup(func() { + _ = cmd.Process.Kill() + _ = cmd.Wait() + }) + return cmd.Process.Pid +} + +func TestProcessTreeIdentityForMissingProcess(t *testing.T) { + identity, err := processTreeIdentity(int(^uint(0) >> 1)) + if err != nil { + t.Fatalf("processTreeIdentity for missing PID: %v", err) + } + if identity != "" { + t.Fatalf("processTreeIdentity for missing PID = %q, want empty identity", identity) + } +} diff --git a/pkg/command/process_identity_unix.go b/pkg/command/process_identity_unix.go new file mode 100644 index 0000000000..5298036421 --- /dev/null +++ b/pkg/command/process_identity_unix.go @@ -0,0 +1,15 @@ +//go:build unix && !linux && !darwin + +package command + +import "errors" + +const strongProcessIdentitySupported = false + +func processTreeIdentity(_ int) (string, error) { + return "", nil +} + +func processGroupMatchesIdentity(_ int, _ string) (bool, error) { + return false, errors.New("process tree identity is unsupported on this platform") +} diff --git a/pkg/command/process_supported.go b/pkg/command/process_supported.go index 4ec57a98a2..f5350ed875 100644 --- a/pkg/command/process_supported.go +++ b/pkg/command/process_supported.go @@ -4,7 +4,9 @@ package command import ( "errors" + "fmt" "os" + "os/exec" "strconv" "syscall" "time" @@ -15,6 +17,9 @@ func isRunning(pid string) (bool, error) { if err != nil { return false, err } + if parsedPid <= 0 { + return false, fmt.Errorf("invalid PID %d: must be positive", parsedPid) + } process, err := os.FindProcess(parsedPid) if err != nil { @@ -23,28 +28,334 @@ func isRunning(pid string) (bool, error) { err = process.Signal(syscall.Signal(0)) if err != nil { - return false, nil + if errors.Is(err, syscall.EPERM) { + return true, nil + } + if isProcessGone(err) || errors.Is(err, os.ErrProcessDone) { + return false, nil + } + return false, fmt.Errorf("check process %d: %w", parsedPid, err) } return true, nil } -func kill(pid string) error { +func processRefForStartedProcess(pid int, _ string) (ProcessRef, error) { + identity, err := processTreeIdentity(pid) + if err != nil { + if !strongProcessIdentitySupported { + identity = "" + } else { + return ProcessRef{}, err + } + } + return ProcessRef{ + PID: pid, + TreeKind: ProcessTreeUnixGroup, + TreeID: strconv.Itoa(pid), + Identity: identity, + }, nil +} + +func terminateProcessRef(ref ProcessRef) error { + if ref.PID <= 0 { + return fmt.Errorf("invalid worker PID %d", ref.PID) + } + if ref.TreeKind == ProcessTreeLegacyPID { + if ref.Identity != "" { + return killTreeWithIdentity(strconv.Itoa(ref.PID), ref.TreeID, ref.Identity) + } + return terminateUnidentifiedProcessGroup(ref.PID, strconv.Itoa(ref.PID)) + } + if ref.TreeKind != ProcessTreeUnixGroup { + return fmt.Errorf("unsupported Unix process tree kind %q", ref.TreeKind) + } + groupID := ref.TreeID + if groupID == "" { + groupID = strconv.Itoa(ref.PID) + } + if ref.Identity != "" { + return killTreeWithIdentity(groupID, "detached-task", ref.Identity) + } + return terminateUnidentifiedProcessGroup(ref.PID, groupID) +} + +func terminateUnidentifiedProcessGroup(pid int, groupID string) error { + if !strongProcessIdentitySupported { + return fmt.Errorf("cannot terminate worker group %d without process identity", pid) + } + identity, err := processTreeIdentity(pid) + if err != nil { + return fmt.Errorf("read process identity for worker %d: %w", pid, err) + } + if identity == "" { + return terminateUnidentifiedExitedGroup(pid) + } + if err := killTreeWithIdentity(groupID, "detached-task", identity); err != nil { + return err + } + return waitForUnidentifiedGroupExit(pid, identity) +} + +func waitForUnidentifiedGroupExit(pid int, identity string) error { + deadline := time.Now().Add(5 * time.Second) + for { + matches, err := processGroupMatchesIdentity(pid, identity) + if err != nil { + return fmt.Errorf("verify worker group %d after termination: %w", pid, err) + } + if !matches { + return nil + } + if time.Now().After(deadline) { + return fmt.Errorf("worker group %d still has processes after termination", pid) + } + time.Sleep(100 * time.Millisecond) + } +} + +// A worker whose leader exited may still leave members behind. +func processGroupExists(pid int) bool { + err := syscall.Kill(-pid, 0) + if isProcessGone(err) { + return false + } + // EPERM means the group exists but belongs to another user. + return true +} + +func cleanupExitedProcessRef(ref ProcessRef) error { + if ref.TreeKind == ProcessTreeLegacyPID { + if ref.Identity == "" { + return terminateUnidentifiedExitedGroup(ref.PID) + } + return killTreeAfterWorkerExit(strconv.Itoa(ref.PID), ref.TreeID, ref.Identity) + } + if ref.TreeKind != ProcessTreeUnixGroup { + return nil + } + groupID := ref.TreeID + if groupID == "" { + groupID = strconv.Itoa(ref.PID) + } + if ref.Identity == "" { + return terminateUnidentifiedExitedGroup(ref.PID) + } + return killTreeAfterWorkerExit(groupID, "detached-task", ref.Identity) +} + +// Never signals the group: after the leader exits the PGID may be reused. +func terminateUnidentifiedExitedGroup(pid int) error { + if pid <= 0 { + return fmt.Errorf("invalid worker PID %d", pid) + } + if processGroupExists(pid) { + return fmt.Errorf( + "worker group %d still has processes and no saved identity identifies them", + pid, + ) + } + return nil +} + +func abortSupervisedLaunch(pid int, _ string) error { + for _, target := range []int{-pid, pid} { + if err := syscall.Kill(target, syscall.SIGKILL); err != nil && + !isProcessGone(err) { + return fmt.Errorf("terminate uncommitted worker %d: %w", pid, err) + } + } + return nil +} + +// Detached workers use a session identity to distinguish their group after the leader exits. +func killTree(pid, treeName string) error { parsedPid, err := strconv.Atoi(pid) if err != nil { return err } + if parsedPid <= 0 { + return fmt.Errorf("invalid PID %d: must be positive", parsedPid) + } + identity, err := processTreeIdentity(parsedPid) + if err != nil { + return fmt.Errorf("read process tree identity for worker %d: %w", parsedPid, err) + } + if identity == "" { + return killUnidentifiedTree(parsedPid, treeName) + } - if err := syscall.Kill(parsedPid, syscall.SIGTERM); err != nil { - if errors.Is(err, syscall.ESRCH) { - return nil // already exited + return killTreeWithIdentity(pid, treeName, identity) +} + +func killUnidentifiedTree(parsedPid int, treeName string) error { + if !strongProcessIdentitySupported && treeName == "" { + if err := syscall.Kill(parsedPid, syscall.SIGTERM); err != nil && !isProcessGone(err) { + return fmt.Errorf("send SIGTERM to process %d: %w", parsedPid, err) } + return nil + } + if treeName == "" { + return nil + } + return checkUnidentifiedProcessGroup(parsedPid, treeName) +} + +func killTreeWithIdentity(pid, treeName, identity string) error { + parsedPid, err := strconv.Atoi(pid) + if err != nil { return err } - time.Sleep(2 * time.Second) - err = syscall.Kill(parsedPid, syscall.SIGKILL) - if err != nil && !errors.Is(err, syscall.ESRCH) { + if parsedPid <= 0 { + return fmt.Errorf("invalid PID %d: must be positive", parsedPid) + } + if treeName == "" { + return killSingleProcessWithIdentity(pid, parsedPid, identity) + } + if identity == "" { + return errors.New("process tree identity is required to terminate a worker") + } + return killWorkerTreeWithIdentity(parsedPid, treeName, identity) +} + +func killSingleProcessWithIdentity(pid string, parsedPid int, identity string) error { + if identity == "" { + running, err := isRunning(pid) + if err != nil { + return err + } + if !running { + return nil + } + return errors.New("process identity is required to terminate a process") + } + return signalProcessTree(parsedPid, true, identity) +} + +func killWorkerTreeWithIdentity(parsedPid int, treeName, identity string) error { + groupMatches, err := processGroupMatchesIdentity(parsedPid, identity) + if err != nil { + return fmt.Errorf("verify process group %d for worker %s: %w", parsedPid, treeName, err) + } + if groupMatches { + return signalProcessTree(-parsedPid, false, identity) + } + currentIdentity, err := processTreeIdentity(parsedPid) + if err != nil { + return fmt.Errorf("read process tree identity for worker %d: %w", parsedPid, err) + } + // The saved group is empty, so a different identity at this PID is a reused process. + if currentIdentity != identity { + return nil + } + return signalProcessTree(parsedPid, false, identity) +} + +func killTreeAfterWorkerExit(pid, treeName, identity string) error { + parsedPid, err := strconv.Atoi(pid) + if err != nil { return err } + if parsedPid <= 0 { + return fmt.Errorf("invalid PID %d: must be positive", parsedPid) + } + if treeName == "" { + return errors.New("process tree name is required after worker exit") + } + if identity == "" { + return checkUnidentifiedProcessGroup(parsedPid, treeName) + } + matches, err := processGroupMatchesIdentity(parsedPid, identity) + if err != nil { + return fmt.Errorf( + "verify process group %d for worker tree %s: %w", + parsedPid, + treeName, + err, + ) + } + if !matches { + return nil + } + return signalProcessTree(-parsedPid, false, identity) +} + +func checkUnidentifiedProcessGroup(pid int, treeName string) error { + err := syscall.Kill(-pid, 0) + if errors.Is(err, syscall.ESRCH) { + return nil + } + if err != nil { + return fmt.Errorf("verify process group %d for worker tree %s: %w", pid, treeName, err) + } + return fmt.Errorf( + "verify process group %d for worker tree %s: saved process identity is unavailable", + pid, + treeName, + ) +} + +func isProcessGone(err error) bool { + return err != nil && errors.Is(err, syscall.ESRCH) +} + +func signalProcessTree(target int, graceful bool, identity string) error { + if err := syscall.Kill(target, syscall.SIGTERM); err != nil { + if isProcessGone(err) { + return nil // already exited + } + return fmt.Errorf("send SIGTERM to process target %d: %w", target, err) + } + if graceful { + time.Sleep(2 * time.Second) + } + matches, err := signalTargetMatchesIdentity(target, identity) + if err != nil { + return fmt.Errorf("verify process target %d before SIGKILL: %w", target, err) + } + if !matches { + return nil + } + err = syscall.Kill(target, syscall.SIGKILL) + if err != nil && !isProcessGone(err) { + return fmt.Errorf("send SIGKILL to process target %d: %w", target, err) + } return nil } + +func signalTargetMatchesIdentity(target int, identity string) (bool, error) { + if identity == "" { + return false, errors.New("process identity is required before SIGKILL") + } + if target < 0 { + return processGroupMatchesIdentity(-target, identity) + } + currentIdentity, err := processTreeIdentity(target) + if err != nil { + if errors.Is(err, syscall.ESRCH) { + return false, nil + } + if errors.Is(err, syscall.EIO) && errors.Is(syscall.Kill(target, 0), syscall.ESRCH) { + return false, nil + } + return false, err + } + return currentIdentity == identity, nil +} + +// ownProcessTree is a no-op on Unix: the process group created by +// prepareBackgroundTree already scopes the tree. +func ownProcessTree(pid int, name string) error { + return nil +} + +func resumeBackgroundTree(pid int) error { + return nil +} + +func prepareBackgroundTree(cmd *exec.Cmd) { + if cmd.SysProcAttr == nil { + cmd.SysProcAttr = &syscall.SysProcAttr{} + } + cmd.SysProcAttr.Setsid = true +} diff --git a/pkg/command/process_test.go b/pkg/command/process_test.go index da4b185b01..ba8f145313 100644 --- a/pkg/command/process_test.go +++ b/pkg/command/process_test.go @@ -3,8 +3,12 @@ package command import ( + "os" "os/exec" + "path/filepath" "strconv" + "strings" + "syscall" "testing" "time" ) @@ -44,8 +48,290 @@ func TestKillOnAlreadyExitedProcessIsNoop(t *testing.T) { } } +func TestKillTreeTerminatesGroupAfterWorkerExits(t *testing.T) { + dir := t.TempDir() + childPIDFile := filepath.Join(dir, "child.pid") + // #nosec G204 -- the fixed shell script and all paths are test-controlled. + cmd := exec.Command( + "sh", + "-c", + `sleep 60 >/dev/null 2>&1 & echo $! > "$1"; exit 0`, + "sh", + childPIDFile, + ) + prepareBackgroundTree(cmd) + if err := cmd.Start(); err != nil { + t.Fatalf("start worker: %v", err) + } + workerPID := strconv.Itoa(cmd.Process.Pid) + identity, err := ProcessTreeIdentity(cmd.Process.Pid) + if err != nil { + t.Fatalf("process tree identity: %v", err) + } + t.Cleanup(func() { _ = killTreeAfterWorkerExit(workerPID, "detached-task", identity) }) + + childPID := readTestPID(t, childPIDFile) + + if err := cmd.Wait(); err != nil { + t.Fatalf("wait for worker exit: %v", err) + } + waitForTestProcess(t, childPID, true) + + if err := killTreeAfterWorkerExit(workerPID, "detached-task", identity); err != nil { + t.Fatalf("killTree after worker exit: %v", err) + } + waitForTestProcess(t, childPID, false) +} + +func TestKillTreeAfterWorkerExitWithoutIdentityDoesNotSignalUnrelatedGroup(t *testing.T) { + worker := startUnixTestProcess(t, &syscall.SysProcAttr{Setpgid: true}) + pid := strconv.Itoa(worker.Process.Pid) + err := killTreeAfterWorkerExit(pid, "stale-task", "") + if err == nil { + t.Fatal("killTreeAfterWorkerExit without identity = nil for a live process group") + } + if !strings.Contains(err.Error(), "saved process identity is unavailable") { + t.Fatalf("killTreeAfterWorkerExit error = %v, want missing-identity context", err) + } + if running, err := isRunning(pid); err != nil || !running { + t.Fatalf("unidentified process running=%t, err=%v", running, err) + } +} + +func TestKillTreeAfterWorkerExitWithMismatchedIdentityDoesNotSignalUnrelatedGroup(t *testing.T) { + worker := startUnixTestProcess(t, &syscall.SysProcAttr{Setpgid: true}) + pid := strconv.Itoa(worker.Process.Pid) + + otherSession := startUnixTestProcess(t, &syscall.SysProcAttr{Setsid: true}) + staleIdentity := processTreeIdentityForTest(t, otherSession.Process.Pid) + workerIdentity := processTreeIdentityForTest(t, worker.Process.Pid) + if staleIdentity == workerIdentity { + t.Fatalf("separate sessions have identical process tree identity %q", workerIdentity) + } + + if err := killTreeAfterWorkerExit(pid, "stale-task", staleIdentity); err != nil { + t.Fatalf("killTreeAfterWorkerExit with stale identity: %v", err) + } + if running, err := isRunning(pid); err != nil || !running { + t.Fatalf("unrelated process running=%t, err=%v", running, err) + } +} + +func TestKillTreeWithMismatchedSavedIdentityDoesNotSignalReusedProcessGroup(t *testing.T) { + worker := startUnixTestProcess(t, &syscall.SysProcAttr{Setpgid: true}) + otherSession := startUnixTestProcess(t, &syscall.SysProcAttr{Setsid: true}) + staleIdentity := processTreeIdentityForTest(t, otherSession.Process.Pid) + workerIdentity := processTreeIdentityForTest(t, worker.Process.Pid) + if staleIdentity == workerIdentity { + t.Fatalf("separate sessions have identical process tree identity %q", workerIdentity) + } + + if err := killTreeWithIdentity( + strconv.Itoa(worker.Process.Pid), + "stale-task", + staleIdentity, + ); err != nil { + t.Fatalf("killTreeWithIdentity with stale identity: %v", err) + } + if running, err := isRunning(strconv.Itoa(worker.Process.Pid)); err != nil || !running { + t.Fatalf("reused process running=%t, err=%v", running, err) + } +} + +func TestKillTreeIdentityRejectsDifferentGroupLeaderStartTime(t *testing.T) { + worker := startUnixTestProcess(t, &syscall.SysProcAttr{Setpgid: true}) + workerIdentity := processTreeIdentityForTest(t, worker.Process.Pid) + identityParts := strings.Split(workerIdentity, ":") + if len(identityParts) < 2 { + t.Fatalf("unexpected process tree identity %q", workerIdentity) + } + startTime, err := strconv.ParseInt(identityParts[len(identityParts)-1], 10, 64) + if err != nil { + t.Fatalf("parse process start identity: %v", err) + } + identityParts[len(identityParts)-1] = strconv.FormatInt(startTime+1, 10) + staleIdentity := strings.Join(identityParts, ":") + + matches, err := processGroupMatchesIdentity(worker.Process.Pid, staleIdentity) + if err != nil { + t.Fatalf("processGroupMatchesIdentity: %v", err) + } + if matches { + t.Fatal("process group matched another process start identity in the same session") + } + if err := killTreeWithIdentity( + strconv.Itoa(worker.Process.Pid), + "stale-task", + staleIdentity, + ); err != nil { + t.Fatalf("killTreeWithIdentity with stale start identity: %v", err) + } + if running, err := isRunning(strconv.Itoa(worker.Process.Pid)); err != nil || !running { + t.Fatalf("worker running=%t, err=%v", running, err) + } +} + +func TestKillTreeWithMissingIdentityDoesNotSignalWorker(t *testing.T) { + worker := startUnixTestProcess(t, &syscall.SysProcAttr{Setpgid: true}) + pid := strconv.Itoa(worker.Process.Pid) + if err := killTreeWithIdentity(pid, "task", ""); err == nil { + t.Fatal("killTreeWithIdentity without saved identity = nil for a live worker") + } + if running, err := isRunning(pid); err != nil || !running { + t.Fatalf("worker running=%t, err=%v", running, err) + } +} + +func TestProcessTreeIdentityMatchesDetachedSession(t *testing.T) { + worker := exec.Command("sleep", "30") + worker.SysProcAttr = &syscall.SysProcAttr{Setsid: true} + if err := worker.Start(); err != nil { + t.Fatalf("start worker: %v", err) + } + defer func() { _ = worker.Process.Kill() }() + + identity, err := ProcessTreeIdentity(worker.Process.Pid) + if err != nil { + t.Fatalf("process tree identity: %v", err) + } + matches, err := processGroupMatchesIdentity(worker.Process.Pid, identity) + if err != nil { + t.Fatalf("match process group identity: %v", err) + } + if !matches { + t.Fatal("process group does not belong to its detached session") + } +} + +func startUnixTestProcess(t *testing.T, attr *syscall.SysProcAttr) *exec.Cmd { + t.Helper() + cmd := exec.Command("sleep", "30") + cmd.SysProcAttr = attr + if err := cmd.Start(); err != nil { + t.Fatalf("start test process: %v", err) + } + t.Cleanup(func() { + _ = cmd.Process.Kill() + _ = cmd.Wait() + }) + return cmd +} + +func processTreeIdentityForTest(t *testing.T, pid int) string { + t.Helper() + identity, err := ProcessTreeIdentity(pid) + if err != nil { + t.Fatalf("read process tree identity: %v", err) + } + return identity +} + +func readTestPID(t *testing.T, path string) string { + t.Helper() + deadline := time.Now().Add(5 * time.Second) + for time.Now().Before(deadline) { + // #nosec G304 -- this test reads only its own temporary PID file. + data, err := os.ReadFile(path) + if err == nil { + return strings.TrimSpace(string(data)) + } + time.Sleep(10 * time.Millisecond) + } + t.Fatalf("worker did not publish PID in %s", path) + return "" +} + +func waitForTestProcess(t *testing.T, pid string, wantRunning bool) { + t.Helper() + deadline := time.Now().Add(5 * time.Second) + for time.Now().Before(deadline) { + running, err := isRunning(pid) + if err != nil { + t.Fatalf("isRunning(%s): %v", pid, err) + } + if running == wantRunning { + return + } + time.Sleep(10 * time.Millisecond) + } + t.Fatalf("process %s running=%t, want %t", pid, !wantRunning, wantRunning) +} + func TestKillInvalidPIDReturnsError(t *testing.T) { - if err := Kill("not-a-pid"); err == nil { - t.Error("Kill(\"not-a-pid\") = nil, want error") + for _, pid := range []string{"not-a-pid", "0", "-5"} { + if _, err := IsRunning(pid); err == nil { + t.Errorf("IsRunning(%q) = nil error, want rejection", pid) + } + if err := Kill(pid); err == nil { + t.Errorf("Kill(%q) = nil, want error", pid) + } } } + +func TestTerminateUnidentifiedProcessGroup(t *testing.T) { + cmd := startSigtermIgnoringGroup(t) + pid := cmd.Process.Pid + ref := ProcessRef{PID: pid, TreeKind: ProcessTreeLegacyPID} + err := terminateProcessRef(ref) + if !strongProcessIdentitySupported { + if err == nil { + t.Fatal("identityless platform signaled a group it could not verify") + } + if running, _ := isRunning(strconv.Itoa(pid)); !running { + t.Fatal("identityless platform terminated an unidentified worker") + } + return + } + if err != nil { + t.Fatalf("terminateProcessRef: %v", err) + } + deadline := time.Now().Add(5 * time.Second) + for processGroupExists(pid) && time.Now().Before(deadline) { + time.Sleep(50 * time.Millisecond) + } + if processGroupExists(pid) { + t.Fatalf("worker group %d still has processes after termination", pid) + } +} + +func TestCleanupExitedIdentitylessGroupReportsSurvivingDescendants(t *testing.T) { + pid := startUnixTestProcess(t, &syscall.SysProcAttr{Setpgid: true}).Process.Pid + ref := ProcessRef{ + PID: pid, + TreeKind: ProcessTreeUnixGroup, + TreeID: strconv.Itoa(pid), + } + + err := cleanupExitedProcessRef(ref) + if err == nil { + t.Fatal("cleanupExitedProcessRef = nil for an identityless group with survivors") + } + if !strings.Contains(err.Error(), "no saved identity") { + t.Fatalf("cleanupExitedProcessRef error = %v, want missing-identity context", err) + } + if running, _ := isRunning(strconv.Itoa(pid)); !running { + t.Fatal("cleanupExitedProcessRef terminated an unidentified group") + } +} + +// startSigtermIgnoringGroup starts a group leader that outlives SIGTERM. The +// child is reaped as soon as it exits, the way a supervising process does, so +// an unreaped zombie does not keep the group looking populated. +func startSigtermIgnoringGroup(t *testing.T) *exec.Cmd { + t.Helper() + cmd := exec.Command("sh", "-c", `trap "" TERM; sleep 30`) + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + if err := cmd.Start(); err != nil { + t.Fatalf("start SIGTERM-ignoring group: %v", err) + } + reaped := make(chan struct{}) + go func() { + _ = cmd.Wait() + close(reaped) + }() + t.Cleanup(func() { + _ = cmd.Process.Kill() + <-reaped + }) + return cmd +} diff --git a/pkg/command/process_unsupported.go b/pkg/command/process_unsupported.go deleted file mode 100644 index eeb7a35d9f..0000000000 --- a/pkg/command/process_unsupported.go +++ /dev/null @@ -1,11 +0,0 @@ -//go:build windows - -package command - -func isRunning(pid string) (bool, error) { - panic("unsupported") -} - -func kill(pid string) error { - panic("unsupported") -} diff --git a/pkg/command/process_windows.go b/pkg/command/process_windows.go new file mode 100644 index 0000000000..0f5ac8c1af --- /dev/null +++ b/pkg/command/process_windows.go @@ -0,0 +1,408 @@ +//go:build windows + +package command + +import ( + "errors" + "fmt" + "os/exec" + "path/filepath" + "strconv" + "strings" + "time" + + "github.com/devsy-org/devsy/pkg/config" + "golang.org/x/sys/windows" +) + +const strongProcessIdentitySupported = true + +func isRunning(pid string) (bool, error) { + parsed, err := parsePID(pid) + if err != nil { + return false, err + } + + handle, err := windows.OpenProcess(windows.SYNCHRONIZE, false, uint32(parsed)) + if err != nil { + // OpenProcess reports a nonexistent PID as ERROR_INVALID_PARAMETER; + // anything else (e.g. access denied) is a real query failure. + if errors.Is(err, windows.ERROR_INVALID_PARAMETER) { + return false, nil + } + return false, fmt.Errorf("open process %d: %w", parsed, err) + } + defer func() { _ = windows.CloseHandle(handle) }() + + event, err := windows.WaitForSingleObject(handle, 0) + if err != nil { + return false, fmt.Errorf("wait for process %d: %w", parsed, err) + } + switch event { + case windows.WAIT_OBJECT_0: + return false, nil + case uint32(windows.WAIT_TIMEOUT): + return true, nil + default: + return false, fmt.Errorf( + "wait for process %d returned unexpected result %#x", + parsed, + event, + ) + } +} + +func processRefForStartedProcess(pid int, workerName string) (ProcessRef, error) { + jobID, err := jobNameFor(workerName) + if err != nil { + return ProcessRef{}, err + } + identity, err := processTreeIdentity(pid) + if err != nil { + return ProcessRef{}, err + } + return ProcessRef{ + PID: pid, + TreeKind: ProcessTreeWindowsJob, + TreeID: jobID, + Identity: identity, + }, nil +} + +func terminateProcessRef(ref ProcessRef) error { + if ref.PID <= 0 { + return fmt.Errorf("invalid worker PID %d", ref.PID) + } + if ref.TreeKind == ProcessTreeWindowsJob { + terminated, err := terminateNamedJob(ref.TreeID) + if err != nil { + return err + } + if terminated { + return nil + } + running, err := savedWorkerRunning(ref) + if err != nil { + return err + } + if !running { + return nil + } + return fmt.Errorf( + "worker job %s is unavailable while process %d is running", + ref.TreeID, + ref.PID, + ) + } + if ref.TreeKind == ProcessTreeLegacyPID { + pid := strconv.Itoa(ref.PID) + if ref.Identity != "" { + return killTreeWithIdentity(pid, ref.TreeID, ref.Identity) + } + return terminateLegacyRefWithoutIdentity(ref) + } + return fmt.Errorf("unsupported Windows process tree kind %q", ref.TreeKind) +} + +// A record written before process identities existed has nothing to compare a +// reused PID against, so require a Devsy executable before taskkill takes down +// a whole tree. +func terminateLegacyRefWithoutIdentity(ref ProcessRef) error { + // Keep the process object open until taskkill returns. Windows cannot reuse + // its PID while this handle exists, even if the worker exits meanwhile. + handle, err := windows.OpenProcess( + windows.PROCESS_QUERY_LIMITED_INFORMATION|windows.SYNCHRONIZE, + false, + uint32(ref.PID), + ) + if errors.Is(err, windows.ERROR_INVALID_PARAMETER) { + return nil + } + if err != nil { + return fmt.Errorf("open legacy worker %d: %w", ref.PID, err) + } + defer func() { _ = windows.CloseHandle(handle) }() + result, err := windows.WaitForSingleObject(handle, 0) + if err != nil { + return fmt.Errorf("check legacy worker %d: %w", ref.PID, err) + } + if result == windows.WAIT_OBJECT_0 { + return nil + } + if result != uint32(windows.WAIT_TIMEOUT) { + return fmt.Errorf("check legacy worker %d: unexpected wait result %#x", ref.PID, result) + } + image, err := processImageName(handle, ref.PID) + if err != nil { + return err + } + if !isDevsyImage(image) { + return fmt.Errorf( + "refusing to terminate process %d: %q is not a %s worker", + ref.PID, + filepath.Base(image), + config.RepoName, + ) + } + terminated, err := terminateJobForPID(ref.TreeID, ref.PID) + if err != nil { + return err + } + if terminated { + return nil + } + cmd := exec.Command("taskkill", "/PID", strconv.Itoa(ref.PID), "/T", "/F") + output, err := cmd.CombinedOutput() + if err != nil { + return fmt.Errorf( + "taskkill /PID %d /T /F: %w: %s", + ref.PID, err, truncateOutput(string(output)), + ) + } + return verifyTerminated(ref.PID) +} + +func isDevsyImage(image string) bool { + name := strings.ToLower(filepath.Base(image)) + name = strings.TrimSuffix(name, filepath.Ext(name)) + return name == config.RepoName || strings.HasPrefix(name, config.RepoName+"-") +} + +func processImageName(handle windows.Handle, pid int) (string, error) { + const maxPath = 32768 + buffer := make([]uint16, maxPath) + size := uint32(len(buffer)) + if err := windows.QueryFullProcessImageName(handle, 0, &buffer[0], &size); err != nil { + return "", fmt.Errorf("read image name for process %d: %w", pid, err) + } + return windows.UTF16ToString(buffer[:size]), nil +} + +func savedWorkerRunning(ref ProcessRef) (bool, error) { + current, err := processTreeIdentity(ref.PID) + if err != nil { + return false, err + } + if current == "" || (ref.Identity != "" && current != ref.Identity) { + return false, nil + } + return isRunning(strconv.Itoa(ref.PID)) +} + +func cleanupExitedProcessRef(ref ProcessRef) error { + if ref.PID <= 0 { + return fmt.Errorf("invalid worker PID %d", ref.PID) + } + if ref.TreeKind == ProcessTreeWindowsJob { + terminated, err := terminateNamedJob(ref.TreeID) + if err != nil || terminated { + return err + } + running, err := savedWorkerRunning(ref) + if err != nil { + return err + } + if running { + return fmt.Errorf( + "worker job %s is unavailable while process %d is running", + ref.TreeID, + ref.PID, + ) + } + return nil + } + if ref.TreeKind == ProcessTreeLegacyPID && ref.Identity != "" { + return killTreeAfterWorkerExit(strconv.Itoa(ref.PID), ref.TreeID, ref.Identity) + } + if ref.TreeKind == ProcessTreeLegacyPID { + return fmt.Errorf( + "cannot verify descendants of exited legacy worker %d without a saved process identity", + ref.PID, + ) + } + return nil +} + +func abortSupervisedLaunch(pid int, workerName string) error { + terminated, err := terminateJobForPID(workerName, pid) + if err != nil { + return err + } + if terminated { + return nil + } + return killTree(strconv.Itoa(pid), "") +} + +func killTree(pid, treeName string) error { + parsed, err := parsePID(pid) + if err != nil { + return err + } + + var jobErr error + if treeName != "" { + if terminated, err := terminateJobForPID(treeName, parsed); err == nil && terminated { + return nil + } else if err != nil { + jobErr = err + } + } + + running, err := isRunning(pid) + if err != nil { + return err + } + if !running { + if jobErr != nil { + return fmt.Errorf("terminate process tree %s: %w", treeName, jobErr) + } + return nil + } + + // /T takes down the worker's descendants; /F stands in for SIGKILL, + // which Windows lacks for arbitrary processes. + cmd := exec.Command("taskkill", "/PID", strconv.Itoa(parsed), "/T", "/F") + output, runErr := cmd.CombinedOutput() + if runErr != nil { + // The process may have exited on its own between the checks. + stillRunning, checkErr := isRunning(pid) + if checkErr == nil && !stillRunning { + return nil + } + return fmt.Errorf( + "taskkill /PID %d /T /F: %w: %s", + parsed, + runErr, + truncateOutput(string(output)), + ) + } + + if jobErr != nil { + return fmt.Errorf("terminate process tree %s: %w", treeName, jobErr) + } + return verifyTerminated(parsed) +} + +func killTreeWithIdentity(pid, treeName, identity string) error { + parsed, err := parsePID(pid) + if err != nil { + return err + } + if identity == "" { + return fmt.Errorf("process identity is required to terminate worker %d", parsed) + } + if treeName == "" { + return fmt.Errorf("worker job identity is required") + } + currentIdentity, err := processTreeIdentity(parsed) + if err != nil { + return fmt.Errorf("read process identity for worker %d: %w", parsed, err) + } + if currentIdentity == "" { + return fmt.Errorf("process %d exited before its identity was verified", parsed) + } + if currentIdentity != identity { + return fmt.Errorf("process %d identity does not match the saved worker", parsed) + } + terminated, err := terminateJobForPID(treeName, parsed) + if err != nil { + return err + } + if terminated { + return nil + } + running, err := isRunning(pid) + if err != nil { + return err + } + if !running { + return nil + } + return fmt.Errorf( + "worker job %s is unavailable while process %d is still running", + treeName, + parsed, + ) +} + +func killTreeAfterWorkerExit(pid, treeName, identity string) error { + if _, err := parsePID(pid); err != nil { + return err + } + if treeName == "" { + return fmt.Errorf("process tree name is required after worker exit") + } + terminated, err := terminateJobAfterWorkerExit(treeName) + if err != nil { + return err + } + if !terminated { + if identity == "" { + return fmt.Errorf( + "worker job %s is unavailable; cannot verify descendants without process identity", + treeName, + ) + } + // SetPID runs only after launch assigns the worker to a kill-on-close job. + } + return nil +} + +func processTreeIdentity(pid int) (string, error) { + process, err := windows.OpenProcess(windows.PROCESS_QUERY_INFORMATION, false, uint32(pid)) + if err != nil { + if errors.Is(err, windows.ERROR_INVALID_PARAMETER) { + return "", nil + } + return "", fmt.Errorf("open process %d for identity: %w", pid, err) + } + defer func() { _ = windows.CloseHandle(process) }() + + var created, exited, kernel, user windows.Filetime + if err := windows.GetProcessTimes(process, &created, &exited, &kernel, &user); err != nil { + return "", fmt.Errorf("read process %d creation time: %w", pid, err) + } + return strconv.FormatInt(created.Nanoseconds(), 10), nil +} + +// verifyTerminated allows a brief window for the exit to become visible +// after the termination call reported success. +func verifyTerminated(parsed int) error { + pid := strconv.Itoa(parsed) + deadline := time.Now().Add(5 * time.Second) + for { + stillRunning, err := isRunning(pid) + if err != nil { + return err + } + if !stillRunning { + return nil + } + if time.Now().After(deadline) { + return fmt.Errorf("process %d is still running after termination", parsed) + } + time.Sleep(100 * time.Millisecond) + } +} + +func parsePID(pid string) (int, error) { + parsed, err := strconv.Atoi(pid) + if err != nil { + return 0, fmt.Errorf("invalid PID %q: %w", pid, err) + } + if parsed <= 0 { + return 0, fmt.Errorf("invalid PID %d: must be positive", parsed) + } + return parsed, nil +} + +// truncateOutput bounds captured taskkill output, which is localized and +// never parsed. +func truncateOutput(output string) string { + const maxOutput = 256 + if len(output) > maxOutput { + return output[:maxOutput] + "..." + } + return output +} diff --git a/pkg/command/process_windows_test.go b/pkg/command/process_windows_test.go new file mode 100644 index 0000000000..29ee3f60eb --- /dev/null +++ b/pkg/command/process_windows_test.go @@ -0,0 +1,737 @@ +//go:build windows + +package command + +import ( + "io" + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "testing" + "time" + "unsafe" + + "github.com/devsy-org/devsy/pkg/config" + "golang.org/x/sys/windows" +) + +const ( + helperEnvMarker = "DEVSY_TEST_HELPER_PROCESS" + helperEnvPIDFile = "DEVSY_TEST_HELPER_PIDFILE" + helperEnvChildPIDFile = "DEVSY_TEST_HELPER_CHILD_PIDFILE" + helperEnvWaitFile = "DEVSY_TEST_HELPER_WAIT_FILE" + helperEnvExitAfterSpawn = "DEVSY_TEST_HELPER_EXIT_AFTER_SPAWN" + helperEnvExitCode = "DEVSY_TEST_HELPER_EXIT_CODE" + helperEnvCheckJob = "DEVSY_TEST_HELPER_CHECK_JOB" +) + +var procIsProcessInJob = windows.NewLazySystemDLL("kernel32.dll").NewProc("IsProcessInJob") + +// TestHelperProcess is not a test; every helper process re-executes the test +// binary into it. +func TestHelperProcess(t *testing.T) { + if os.Getenv(helperEnvMarker) != "1" { + return + } + if exitCode := os.Getenv(helperEnvExitCode); exitCode != "" { + code, err := strconv.Atoi(exitCode) + if err != nil { + os.Exit(2) + } + os.Exit(code) + } + if jobResult := os.Getenv(helperEnvCheckJob); jobResult != "" { + inJob, err := currentProcessInJob() + if err != nil { + os.Exit(3) + } + if err := os.WriteFile(jobResult, []byte(strconv.FormatBool(inJob)), 0o600); err != nil { + os.Exit(4) + } + } + if waitFile := os.Getenv(helperEnvWaitFile); waitFile != "" { + for { + if _, err := os.Stat(waitFile); err == nil { + break + } + time.Sleep(50 * time.Millisecond) + } + } + if pidFile := os.Getenv(helperEnvPIDFile); pidFile != "" { + child := exec.Command(os.Args[0], "-test.run", "TestHelperProcess") + child.Env = helperEnv(helperEnvMarker + "=1") + if grandchildPIDFile := os.Getenv(helperEnvChildPIDFile); grandchildPIDFile != "" { + child.Env = append(child.Env, + helperEnvPIDFile+"="+grandchildPIDFile, + helperEnvExitAfterSpawn+"=1", + ) + } + if err := child.Start(); err != nil { + os.Exit(1) + } + if err := os.WriteFile( + pidFile, + []byte(strconv.Itoa(child.Process.Pid)), + 0o600, + ); err != nil { + os.Exit(1) + } + if os.Getenv(helperEnvExitAfterSpawn) == "1" { + os.Exit(0) + } + } + for { + time.Sleep(time.Hour) + } +} + +// helperEnv strips helper markers from the inherited environment; Windows +// process creation needs the rest (e.g. SYSTEMROOT) intact. +func helperEnv(extra ...string) []string { + env := []string{} + for _, kv := range os.Environ() { + key, _, _ := strings.Cut(kv, "=") + if strings.HasPrefix(key, "DEVSY_TEST_HELPER_") { + continue + } + env = append(env, kv) + } + return append(env, extra...) +} + +func startHelper(t *testing.T, extraEnv ...string) *exec.Cmd { + t.Helper() + cmd := exec.Command(os.Args[0], "-test.run", "TestHelperProcess") + cmd.Env = helperEnv(append([]string{helperEnvMarker + "=1"}, extraEnv...)...) + if err := cmd.Start(); err != nil { + t.Fatalf("start helper: %v", err) + } + t.Cleanup(func() { + _ = cmd.Process.Kill() + _ = cmd.Wait() + }) + return cmd +} + +func assertRunning(t *testing.T, pid int) { + t.Helper() + running, err := isRunning(strconv.Itoa(pid)) + if err != nil { + t.Fatalf("isRunning(%d): %v", pid, err) + } + if !running { + t.Fatalf("isRunning(%d) = false, want true", pid) + } +} + +func TestIsDevsyImage(t *testing.T) { + for image, want := range map[string]bool{ + `C:\Program Files\Devsy\devsy.exe`: true, + `C:\Program Files\Devsy\devsy.EXE`: true, + `C:\Users\me\bin\devsy-1.19.0-windows-amd64.exe`: true, + `C:\Windows\System32\notepad.exe`: false, + `C:\Users\me\devsync.exe`: false, + "": false, + } { + if got := isDevsyImage(image); got != want { + t.Errorf("isDevsyImage(%q) = %t, want %t", image, got, want) + } + } +} + +func TestTerminateLegacyRefWithoutIdentityRefusesForeignProcess(t *testing.T) { + helper := startHelper(t) + ref := ProcessRef{PID: helper.Process.Pid, TreeKind: ProcessTreeLegacyPID} + + err := terminateLegacyRefWithoutIdentity(ref) + if err == nil { + t.Fatal("terminateLegacyRefWithoutIdentity = nil for a non-Devsy process") + } + if !strings.Contains(err.Error(), "refusing to terminate") { + t.Fatalf("terminateLegacyRefWithoutIdentity error = %v, want a refusal", err) + } + assertRunning(t, helper.Process.Pid) +} + +func TestTerminateLegacyRefWithoutIdentityEndsDevsyTree(t *testing.T) { + dir := t.TempDir() + t.Setenv(config.EnvHome, dir) + source, err := os.Open(os.Args[0]) // #nosec G304 -- current test binary + if err != nil { + t.Fatalf("open test binary: %v", err) + } + defer func() { _ = source.Close() }() + path := filepath.Join(dir, "devsy.exe") + target, err := os.Create(path) // #nosec G304 -- test-controlled path + if err != nil { + t.Fatalf("create Devsy test binary: %v", err) + } + if _, err := io.Copy(target, source); err != nil { + t.Fatalf("copy Devsy test binary: %v", err) + } + if err := target.Close(); err != nil { + t.Fatalf("close Devsy test binary: %v", err) + } + childPIDFile := filepath.Join(dir, "child.pid") + worker := exec.Command(path, "-test.run", "TestHelperProcess") + worker.Env = helperEnv(helperEnvMarker+"=1", helperEnvPIDFile+"="+childPIDFile) + if err := worker.Start(); err != nil { + t.Fatalf("start legacy Devsy worker: %v", err) + } + t.Cleanup(func() { + _ = worker.Process.Kill() + _ = worker.Wait() + }) + childPID := waitForPIDFile(t, childPIDFile) + ref := ProcessRef{ + PID: worker.Process.Pid, + TreeKind: ProcessTreeLegacyPID, + TreeID: "devsy-up-abcdefghijkl", + } + if err := terminateLegacyRefWithoutIdentity(ref); err != nil { + t.Fatalf("terminate legacy Devsy worker: %v", err) + } + assertNotRunningEventually(t, worker.Process.Pid) + assertNotRunningEventually(t, childPID) +} + +func TestCleanupExitedLegacyRefWithoutIdentityFailsClosed(t *testing.T) { + err := cleanupExitedProcessRef(ProcessRef{ + PID: 4242, TreeKind: ProcessTreeLegacyPID, + }) + if err == nil || !strings.Contains(err.Error(), "cannot verify descendants") { + t.Fatalf("cleanupExitedProcessRef error = %v, want unverifiable descendants", err) + } +} + +func TestProcessRefRejectsInvalidPID(t *testing.T) { + ref := ProcessRef{TreeKind: ProcessTreeLegacyPID} + if err := terminateProcessRef(ref); err == nil { + t.Fatal("terminateProcessRef accepted PID zero") + } + if err := cleanupExitedProcessRef(ref); err == nil { + t.Fatal("cleanupExitedProcessRef accepted PID zero") + } +} + +func assertNotRunningEventually(t *testing.T, pid int) { + t.Helper() + deadline := time.Now().Add(15 * time.Second) + for time.Now().Before(deadline) { + running, err := isRunning(strconv.Itoa(pid)) + if err == nil && !running { + return + } + time.Sleep(200 * time.Millisecond) + } + t.Fatalf("process %d still running after 15s", pid) +} + +func TestIsRunningDetectsLiveAndExitedProcess(t *testing.T) { + cmd := startHelper(t) + assertRunning(t, cmd.Process.Pid) + + if err := cmd.Process.Kill(); err != nil { + t.Fatalf("kill helper: %v", err) + } + _ = cmd.Wait() + assertNotRunningEventually(t, cmd.Process.Pid) +} + +func TestIsRunningRecognizesExitCode259AsExited(t *testing.T) { + cmd := exec.Command(os.Args[0], "-test.run", "TestHelperProcess") + cmd.Env = helperEnv(helperEnvMarker+"=1", helperEnvExitCode+"=259") + if err := cmd.Start(); err != nil { + t.Fatalf("start helper: %v", err) + } + processHandle, err := windows.OpenProcess(windows.SYNCHRONIZE, false, uint32(cmd.Process.Pid)) + if err != nil { + _ = cmd.Process.Kill() + _ = cmd.Wait() + t.Fatalf("open helper process: %v", err) + } + defer func() { _ = windows.CloseHandle(processHandle) }() + t.Cleanup(func() { + _ = cmd.Process.Kill() + _ = cmd.Wait() + }) + + result, err := windows.WaitForSingleObject(processHandle, 10_000) + if err != nil { + t.Fatalf("wait for helper: %v", err) + } + if result != windows.WAIT_OBJECT_0 { + t.Fatalf("wait result = %#x, want WAIT_OBJECT_0", result) + } + + running, err := isRunning(strconv.Itoa(cmd.Process.Pid)) + if err != nil { + t.Fatalf("isRunning(%d): %v", cmd.Process.Pid, err) + } + if running { + t.Fatal("isRunning returned true for exited process with exit code 259") + } + + if err := cmd.Wait(); err == nil || cmd.ProcessState.ExitCode() != 259 { + t.Fatalf("helper exit = %v (%v), want exit code 259", err, cmd.ProcessState) + } +} + +func TestStartSupervisedBackgroundAssignsWorkerToJobBeforeItRuns(t *testing.T) { + dir := t.TempDir() + config.ResetPathManager() + t.Cleanup(config.ResetPathManager) + t.Setenv(config.EnvHome, dir) + jobName := "devsy-test-suspended-worker" + jobResult := filepath.Join(dir, "in-job") + cmd := exec.Command(os.Args[0], "-test.run", "TestHelperProcess") + cmd.Env = helperEnv(helperEnvMarker+"=1", helperEnvCheckJob+"="+jobResult) + var ref ProcessRef + if err := startSupervisedDetached(cmd, SupervisedStartOptions{ + Name: jobName, + OnStarted: func(process ProcessRef) error { + ref = process + return nil + }, + }, filepath.Join(dir, "streams")); err != nil { + t.Fatalf("startSupervisedDetached: %v", err) + } + t.Cleanup(func() { _ = DefaultProcessController().Terminate(ref) }) + job, found, err := openWorkerJob(jobName) + if err != nil || !found { + t.Fatalf("open worker job after launcher exit: found=%t, err=%v", found, err) + } + _ = windows.CloseHandle(job) + + deadline := time.Now().Add(10 * time.Second) + for time.Now().Before(deadline) { + if _, err := os.Stat(jobResult); err == nil { + break + } + time.Sleep(10 * time.Millisecond) + } + data, err := os.ReadFile(jobResult) // #nosec G304 -- test-controlled path + if err != nil { + t.Fatalf("read job membership result: %v", err) + } + if strings.TrimSpace(string(data)) != "true" { + t.Fatalf("worker job membership = %q, want true", data) + } +} + +func TestKillTreeWithIdentityDoesNotFallbackToUnverifiedPID(t *testing.T) { + worker := startHelper(t) + jobName := "devsy-test-no-job-" + strconv.Itoa(worker.Process.Pid) + identity, err := ProcessTreeIdentity(worker.Process.Pid) + if err != nil { + t.Fatalf("ProcessTreeIdentity: %v", err) + } + if identity == "" { + t.Fatal("ProcessTreeIdentity returned empty identity for live worker") + } + if err := killTreeWithIdentity( + strconv.Itoa(worker.Process.Pid), + jobName, + identity, + ); err == nil { + t.Fatal("killTreeWithIdentity without a job object = nil for a live worker") + } + if running, err := isRunning(strconv.Itoa(worker.Process.Pid)); err != nil || !running { + t.Fatalf("worker running=%t, err=%v", running, err) + } +} + +func TestKillTreeAfterWorkerExitWithoutIdentityFailsClosedWhenJobIsMissing(t *testing.T) { + worker := startHelper(t) + if err := worker.Process.Kill(); err != nil { + t.Fatalf("kill helper: %v", err) + } + _ = worker.Wait() + + err := killTreeAfterWorkerExit(strconv.Itoa(worker.Process.Pid), "devsy-up-abcdefghijkl", "") + if err == nil || + !strings.Contains(err.Error(), "cannot verify descendants without process identity") { + t.Fatalf("killTreeAfterWorkerExit error = %v, want fail-closed identity error", err) + } +} + +func TestOwnProcessTreeRejectsExistingJobObject(t *testing.T) { + workerName := "devsy-test-existing-job-" + strconv.FormatInt(time.Now().UnixNano(), 10) + jobName, err := jobNameFor(workerName) + if err != nil { + t.Fatalf("resolve job name: %v", err) + } + name, err := windows.UTF16PtrFromString(jobName) + if err != nil { + t.Fatalf("convert job name: %v", err) + } + existingJob, err := windows.CreateJobObject(nil, name) + if err != nil { + t.Fatalf("create existing job object: %v", err) + } + defer func() { _ = windows.CloseHandle(existingJob) }() + + err = ownProcessTree(0, workerName) + if err == nil || !strings.Contains(err.Error(), "already exists") { + t.Fatalf("ownProcessTree with an existing job = %v, want already-exists error", err) + } +} + +func TestKillTreeWithIdentityFindsLegacyJobObject(t *testing.T) { + worker := startHelper(t) + workerName := "devsy-test-legacy-job-" + strconv.Itoa(worker.Process.Pid) + name, err := windows.UTF16PtrFromString(workerName) + if err != nil { + t.Fatalf("convert legacy job name: %v", err) + } + job, err := windows.CreateJobObject(nil, name) + if err != nil { + t.Fatalf("create legacy job: %v", err) + } + defer func() { _ = windows.CloseHandle(job) }() + + process, err := windows.OpenProcess( + windows.PROCESS_SET_QUOTA|windows.PROCESS_TERMINATE, + false, + uint32(worker.Process.Pid), + ) + if err != nil { + t.Fatalf("open helper process: %v", err) + } + defer func() { _ = windows.CloseHandle(process) }() + if err := windows.AssignProcessToJobObject(job, process); err != nil { + t.Fatalf("assign helper to legacy job: %v", err) + } + identity, err := ProcessTreeIdentity(worker.Process.Pid) + if err != nil { + t.Fatalf("ProcessTreeIdentity: %v", err) + } + if err := killTreeWithIdentity( + strconv.Itoa(worker.Process.Pid), + workerName, + identity, + ); err != nil { + t.Fatalf("killTreeWithIdentity: %v", err) + } + assertNotRunningEventually(t, worker.Process.Pid) +} + +func TestKillTreeFindsLegacyJobObject(t *testing.T) { + worker := startHelper(t) + workerName := "devsy-test-legacy-cancel-" + strconv.Itoa(worker.Process.Pid) + name, err := windows.UTF16PtrFromString(workerName) + if err != nil { + t.Fatalf("convert legacy job name: %v", err) + } + job, err := windows.CreateJobObject(nil, name) + if err != nil { + t.Fatalf("create legacy job: %v", err) + } + defer func() { _ = windows.CloseHandle(job) }() + process, err := windows.OpenProcess( + windows.PROCESS_SET_QUOTA|windows.PROCESS_TERMINATE, + false, + uint32(worker.Process.Pid), + ) + if err != nil { + t.Fatalf("open helper process: %v", err) + } + defer func() { _ = windows.CloseHandle(process) }() + if err := windows.AssignProcessToJobObject(job, process); err != nil { + t.Fatalf("assign helper to legacy job: %v", err) + } + + if err := KillTree(strconv.Itoa(worker.Process.Pid), workerName); err != nil { + t.Fatalf("KillTree: %v", err) + } + assertNotRunningEventually(t, worker.Process.Pid) +} + +func TestKillTreeAfterWorkerExitFindsLegacyDetachedJob(t *testing.T) { + worker := startHelper(t) + workerName := "devsy-up-abcdefghijkl" + name, err := windows.UTF16PtrFromString(workerName) + if err != nil { + t.Fatalf("convert legacy job name: %v", err) + } + job, err := windows.CreateJobObject(nil, name) + if err != nil { + t.Fatalf("create legacy job: %v", err) + } + defer func() { _ = windows.CloseHandle(job) }() + + process, err := windows.OpenProcess( + windows.PROCESS_SET_QUOTA|windows.PROCESS_TERMINATE, + false, + uint32(worker.Process.Pid), + ) + if err != nil { + t.Fatalf("open helper process: %v", err) + } + defer func() { _ = windows.CloseHandle(process) }() + if err := windows.AssignProcessToJobObject(job, process); err != nil { + t.Fatalf("assign helper to legacy job: %v", err) + } + terminated, err := terminateJobAfterWorkerExit(workerName) + if err != nil { + t.Fatalf("terminateJobAfterWorkerExit: %v", err) + } + if !terminated { + t.Fatal("terminateJobAfterWorkerExit found no legacy job") + } + assertNotRunningEventually(t, worker.Process.Pid) +} + +func TestJobNameForSeparatesDevsyHomes(t *testing.T) { + config.ResetPathManager() + t.Cleanup(config.ResetPathManager) + + t.Setenv(config.EnvHome, t.TempDir()) + first, err := jobNameFor("jupyter") + if err != nil { + t.Fatalf("jobNameFor first home: %v", err) + } + + t.Setenv(config.EnvHome, t.TempDir()) + second, err := jobNameFor("jupyter") + if err != nil { + t.Fatalf("jobNameFor second home: %v", err) + } + if first == second { + t.Fatalf("job names collide across Devsy homes: %q", first) + } +} + +func TestJobNameForCanonicalizesEquivalentHomePaths(t *testing.T) { + home := t.TempDir() + t.Setenv(config.EnvHome, home) + first, err := jobNameFor("jupyter") + if err != nil { + t.Fatalf("jobNameFor first path: %v", err) + } + + t.Setenv(config.EnvHome, strings.ToUpper(home)) + second, err := jobNameFor("jupyter") + if err != nil { + t.Fatalf("jobNameFor second path: %v", err) + } + if first != second { + t.Fatalf("equivalent home paths have different job names: %q != %q", first, second) + } +} + +func TestJobNameForDoesNotRequireHomeDirectoryAtCancellation(t *testing.T) { + home := filepath.Join(t.TempDir(), "home") + if err := os.Mkdir(home, 0o700); err != nil { + t.Fatalf("create home: %v", err) + } + t.Setenv(config.EnvHome, home) + first, err := jobNameFor("jupyter") + if err != nil { + t.Fatalf("jobNameFor first call: %v", err) + } + if err := os.Remove(home); err != nil { + t.Fatalf("remove home: %v", err) + } + second, err := jobNameFor("jupyter") + if err != nil { + t.Fatalf("jobNameFor second call: %v", err) + } + if first != second { + t.Fatalf("missing home produced different job names: %q != %q", first, second) + } +} + +func TestTerminateJobForPIDFindsLegacyJobWhenScopedJobIsUnrelated(t *testing.T) { + worker := startHelper(t) + workerName := "devsy-test-legacy-fallback-" + strconv.Itoa(worker.Process.Pid) + scopedName, err := jobNameFor(workerName) + if err != nil { + t.Fatalf("resolve scoped job name: %v", err) + } + scopedNamePtr, err := windows.UTF16PtrFromString(scopedName) + if err != nil { + t.Fatalf("convert scoped job name: %v", err) + } + scopedJob, err := windows.CreateJobObject(nil, scopedNamePtr) + if err != nil { + t.Fatalf("create scoped job: %v", err) + } + defer func() { _ = windows.CloseHandle(scopedJob) }() + + legacyNamePtr, err := windows.UTF16PtrFromString(workerName) + if err != nil { + t.Fatalf("convert legacy job name: %v", err) + } + legacyJob, err := windows.CreateJobObject(nil, legacyNamePtr) + if err != nil { + t.Fatalf("create legacy job: %v", err) + } + defer func() { _ = windows.CloseHandle(legacyJob) }() + process, err := windows.OpenProcess( + windows.PROCESS_SET_QUOTA|windows.PROCESS_TERMINATE, + false, + uint32(worker.Process.Pid), + ) + if err != nil { + t.Fatalf("open helper process: %v", err) + } + defer func() { _ = windows.CloseHandle(process) }() + if err := windows.AssignProcessToJobObject(legacyJob, process); err != nil { + t.Fatalf("assign helper to legacy job: %v", err) + } + + terminated, err := terminateJobForPID(workerName, worker.Process.Pid) + if err != nil { + t.Fatalf("terminateJobForPID: %v", err) + } + if !terminated { + t.Fatal("terminateJobForPID found no matching job") + } + assertNotRunningEventually(t, worker.Process.Pid) +} + +func currentProcessInJob() (bool, error) { + var inJob int32 + result, _, callErr := procIsProcessInJob.Call( + uintptr(windows.CurrentProcess()), + 0, + uintptr(unsafe.Pointer(&inJob)), + ) + if result == 0 { + return false, callErr + } + return inJob != 0, nil +} + +func waitForPIDFile(t *testing.T, pidFile string) int { + t.Helper() + deadline := time.Now().Add(10 * time.Second) + for time.Now().Before(deadline) { + data, err := os.ReadFile(pidFile) // #nosec G304 -- test-controlled path + if err == nil { + pid, err := strconv.Atoi(strings.TrimSpace(string(data))) + if err == nil && pid > 0 { + return pid + } + } + time.Sleep(100 * time.Millisecond) + } + t.Fatalf("no PID reported in %s", pidFile) + return 0 +} + +func TestKillTerminatesProcessTree(t *testing.T) { + pidFile := filepath.Join(t.TempDir(), "child.pid") + parent := startHelper(t, helperEnvPIDFile+"="+pidFile) + + childPID := waitForPIDFile(t, pidFile) + + assertRunning(t, parent.Process.Pid) + assertRunning(t, childPID) + + if err := Kill(strconv.Itoa(parent.Process.Pid)); err != nil { + t.Fatalf("Kill(parent): %v", err) + } + + assertNotRunningEventually(t, parent.Process.Pid) + assertNotRunningEventually(t, childPID) +} + +func TestKillOnAlreadyExitedProcessIsNoop(t *testing.T) { + cmd := startHelper(t) + if err := cmd.Process.Kill(); err != nil { + t.Fatalf("kill helper: %v", err) + } + _ = cmd.Wait() + assertNotRunningEventually(t, cmd.Process.Pid) + + if err := Kill(strconv.Itoa(cmd.Process.Pid)); err != nil { + t.Errorf("Kill on exited process = %v, want nil", err) + } +} + +func TestKillIsIdempotent(t *testing.T) { + cmd := startHelper(t) + pid := strconv.Itoa(cmd.Process.Pid) + + if err := Kill(pid); err != nil { + t.Fatalf("first Kill: %v", err) + } + if err := Kill(pid); err != nil { + t.Errorf("second Kill = %v, want nil", err) + } +} + +func TestInvalidPIDReturnsError(t *testing.T) { + for _, pid := range []string{"not-a-pid", "0", "-5"} { + if _, err := isRunning(pid); err == nil { + t.Errorf("isRunning(%q) = nil error, want rejection", pid) + } + if err := killTree(pid, ""); err == nil { + t.Errorf("kill(%q) = nil, want error", pid) + } + } +} + +func TestKillTerminatesOrphanedGrandchildViaJobObject(t *testing.T) { + dir := t.TempDir() + waitFile := filepath.Join(dir, "go") + childPIDFile := filepath.Join(dir, "child.pid") + grandchildPIDFile := filepath.Join(dir, "grandchild.pid") + + // The child spawns the grandchild and exits, orphaning the grandchild + // from the perspective of parent-based termination. + parentPIDFile := filepath.Join(dir, "parent.pid") + parent := exec.Command(os.Args[0], "-test.run", "TestHelperProcess") + parent.Env = helperEnv( + helperEnvMarker+"=1", + helperEnvWaitFile+"="+waitFile, + helperEnvPIDFile+"="+childPIDFile, + helperEnvChildPIDFile+"="+grandchildPIDFile, + helperEnvExitAfterSpawn+"=1", + ) + var ref ProcessRef + if err := startSupervisedDetached( + parent, + SupervisedStartOptions{ + Name: "devsy-test-worker", + OnStarted: func(process ProcessRef) error { + ref = process + return os.WriteFile(parentPIDFile, []byte(strconv.Itoa(process.PID)), 0o600) + }, + }, + filepath.Join(dir, "streams"), + ); err != nil { + t.Fatalf("startSupervisedDetached: %v", err) + } + parentPID := ref.PID + identity, err := ProcessTreeIdentity(parentPID) + if err != nil { + t.Fatalf("ProcessTreeIdentity: %v", err) + } + if identity == "" { + t.Fatalf("ProcessTreeIdentity(%d) returned an empty identity", parentPID) + } + t.Cleanup( + func() { _ = KillTreeAfterWorkerExit(strconv.Itoa(parentPID), "devsy-test-worker", identity) }, + ) + if err := os.WriteFile(waitFile, []byte("go"), 0o600); err != nil { + t.Fatalf("signal helper: %v", err) + } + + childPID := waitForPIDFile(t, childPIDFile) + grandchildPID := waitForPIDFile(t, grandchildPIDFile) + assertNotRunningEventually(t, childPID) + assertNotRunningEventually(t, parentPID) + assertNotRunningEventually(t, grandchildPID) + if err := killTreeAfterWorkerExit( + strconv.Itoa(parentPID), + "devsy-test-worker", + identity, + ); err != nil { + t.Fatalf("KillTreeAfterWorkerExit after natural tree cleanup: %v", err) + } +} diff --git a/pkg/config/pathmanager_unix.go b/pkg/config/pathmanager_unix.go new file mode 100644 index 0000000000..139dfd41f3 --- /dev/null +++ b/pkg/config/pathmanager_unix.go @@ -0,0 +1,74 @@ +//go:build unix && !linux && !darwin + +package config + +import ( + "fmt" + "os" + "path/filepath" +) + +type unixPathManager struct { + basePathManager +} + +func newPlatformPathManager() PathManager { + pm := &unixPathManager{} + pm.pm = pm + return pm +} + +func (u *unixPathManager) ConfigDir() (string, error) { + if dir, ok, err := homeOverrideDir(); ok { + return dir, err + } + home, err := os.UserHomeDir() + if err != nil { + return "", fmt.Errorf("config dir: %w", err) + } + return ensureDir(filepath.Join(home, "."+RepoName)) +} + +func (u *unixPathManager) DataDir() (string, error) { + if dir, ok, err := homeOverrideDir(); ok { + return dir, err + } + home, err := os.UserHomeDir() + if err != nil { + return "", fmt.Errorf("data dir: %w", err) + } + return ensureDir(filepath.Join(home, "."+RepoName)) +} + +func (u *unixPathManager) CacheDir() (string, error) { + if dir, ok, err := homeOverrideDir("cache"); ok { + return dir, err + } + home, err := os.UserHomeDir() + if err != nil { + return "", fmt.Errorf("cache dir: %w", err) + } + return ensureDir(filepath.Join(home, ".cache", RepoName)) +} + +func (u *unixPathManager) StateDir() (string, error) { + if dir, ok, err := homeOverrideDir("state"); ok { + return dir, err + } + home, err := os.UserHomeDir() + if err != nil { + return "", fmt.Errorf("state dir: %w", err) + } + return ensureDir(filepath.Join(home, "."+RepoName, "state")) +} + +func (u *unixPathManager) RuntimeDir() (string, error) { + if dir, ok, err := homeOverrideDir("run"); ok { + return dir, err + } + return ensureDir(filepath.Join(os.TempDir(), fmt.Sprintf("%s-%d", RepoName, os.Getuid()))) +} + +func (u *unixPathManager) SystemBinDir() (string, error) { + return "/usr/local/bin", nil +} diff --git a/pkg/devcontainer/config/host_requirements_storage_unix.go b/pkg/devcontainer/config/host_requirements_storage_unix.go index ee8a23f3ab..33500622cd 100644 --- a/pkg/devcontainer/config/host_requirements_storage_unix.go +++ b/pkg/devcontainer/config/host_requirements_storage_unix.go @@ -9,5 +9,12 @@ func availableStorageBytes(path string) (uint64, error) { if err := syscall.Statfs(path, &stat); err != nil { return 0, err } - return stat.Bavail * uint64(stat.Bsize), nil //nolint:gosec // Bsize type varies by platform + if stat.Bavail <= 0 || stat.Bsize <= 0 { + return 0, nil + } + return uint64( + stat.Bavail, + ) * uint64( + stat.Bsize, + ), nil //nolint:gosec // Statfs field types vary by platform } diff --git a/pkg/task/export_test.go b/pkg/task/export_test.go deleted file mode 100644 index 9ffbc395a5..0000000000 --- a/pkg/task/export_test.go +++ /dev/null @@ -1,7 +0,0 @@ -package task - -// SetAfterClaimForTest runs fn inside Reconcile, just after it claims the dead -// worker's lock. -func (s *Store) SetAfterClaimForTest(fn func()) { - s.afterClaimForTest = fn -} diff --git a/pkg/task/store.go b/pkg/task/store.go index 87b3b5185e..3047de90f3 100644 --- a/pkg/task/store.go +++ b/pkg/task/store.go @@ -9,6 +9,7 @@ import ( "sort" "time" + "github.com/devsy-org/devsy/pkg/command" "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/random" "github.com/gofrs/flock" @@ -19,9 +20,10 @@ const lockTimeout = 5 * time.Second // Store persists task state as one JSON file per task under dir. type Store struct { - dir string - // Test seam; see Store.SetAfterClaimForTest. - afterClaimForTest func() + dir string + processController command.ProcessController + afterClaimHook func() + afterCancelLockClaimedHook func() } func NewStore() (*Store, error) { @@ -36,7 +38,10 @@ func NewStoreAt(dir string) (*Store, error) { if err := os.MkdirAll(dir, 0o750); err != nil { return nil, fmt.Errorf("create task dir: %w", err) } - return &Store{dir: dir}, nil + return &Store{ + dir: dir, + processController: command.DefaultProcessController(), + }, nil } func (s *Store) Create(opts CreateOptions) (*Task, error) { @@ -61,20 +66,32 @@ func (s *Store) Open(id string) *Task { return &Task{store: s, id: id} } -// Delete errors if the task is still pending or running unless force is set. +// Delete cleans any unfinished terminal process tree before removing its +// record. It rejects a non-terminal task unless force is set. func (s *Store) Delete(id string, force bool) error { path, err := s.path(id) if err != nil { return err } + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if err := s.CleanupCanceledWorkerTree(ctx, id); err != nil { + return err + } + if err := s.CleanupExitedWorkerTree(id); err != nil { + return err + } // Locked across the check and the removal so a concurrent update can't // commit its atomic rename after the check and recreate the task. return s.withLock(id, func() error { + state, err := s.Get(id) + if err != nil { + return err + } + if state.NeedsExitedWorkerCleanup() || state.NeedsCanceledWorkerCleanup() { + return fmt.Errorf("task %s still needs process tree cleanup", id) + } if !force { - state, err := s.Get(id) - if err != nil { - return err - } if !state.Status.Terminal() { return fmt.Errorf( "task %s is still %s; cancel it first or delete with force", @@ -118,27 +135,120 @@ func (s *Store) Abandoned(state *State) bool { return true } -// Reconcile marks a task failed when its worker died without recording a -// result, so it stops being reported as still running. Returns the effective -// state, which is unchanged for live and already-terminal tasks. -func (s *Store) Reconcile(state *State) *State { - if state == nil || state.Status.Terminal() { - return state +// ReconcileTask cleans up a task whose worker exited before recording its +// result, then records it as canceled or abandoned. +func (s *Store) ReconcileTask(ctx context.Context, id string) (*State, error) { + if err := ctx.Err(); err != nil { + return nil, err + } + current, err := s.Get(id) + if err != nil { + return nil, err + } + if !needsReconciliation(current) { + return current, nil + } + lock, ok := s.claimDeadWorkerLock(id) + if !ok { + return current, nil } + defer func() { _ = lock.Unlock() }() + if s.afterClaimHook != nil { + s.afterClaimHook() + } + return s.reconcileClaimedTask(ctx, id) +} + +func needsReconciliation(state *State) bool { + return state != nil && !state.Status.Terminal() && + !state.LaunchPending +} - // Held across the whole transition: releasing before the write would let a - // new worker claim the lock and then be marked failed while running. +// ReconcileState marks a task after its worker exits without a result. A +// requested cancellation completes cleanup here; an abandoned task's tree is +// cleaned up by the next workspace stop or delete. +func (s *Store) ReconcileState(state *State) *State { + if !needsStateReconciliation(state) { + return state + } lock, ok := s.claimDeadWorkerLock(state.ID) if !ok { return state } defer func() { _ = lock.Unlock() }() + current, err := s.Get(state.ID) + if err != nil || !needsStateReconciliation(current) { + if err == nil { + return current + } + return state + } + if current.CancelRequested { + reconciled, err := s.reconcileClaimedCancellation(context.Background(), current) + if err == nil { + return reconciled + } + return current + } + return s.failAbandoned(current) +} + +// CleanupExitedWorkerTree tears down any descendants a dead worker left behind. +// Claiming the worker lock proves no worker is running, so a saved reference +// that still points at live processes belongs to a crashed worker's tree. +func (s *Store) CleanupExitedWorkerTree(id string) error { + state, err := s.Get(id) + if err != nil { + return err + } + if !state.NeedsExitedWorkerCleanup() { + return nil + } + lock, ok := s.claimDeadWorkerLock(id) + if !ok { + return fmt.Errorf("cannot claim dead worker lock for abandoned task %s", id) + } + defer func() { _ = lock.Unlock() }() + state, err = s.Get(id) + if err != nil { + return err + } + if !state.NeedsExitedWorkerCleanup() { + return nil + } + ref, _ := state.ProcessReference() + if err := s.processController.CleanupAfterExit(ref); err != nil { + return fmt.Errorf("clean up task %s process tree: %w", id, err) + } + return s.markProcessCleanupComplete(id) +} - if s.afterClaimForTest != nil { - s.afterClaimForTest() +// CleanupCanceledWorkerTree handles records that were marked canceled before +// their worker tree was terminated. A successful termination of an identityless +// tree is sufficient only while the worker lock proves it was still live. +func (s *Store) CleanupCanceledWorkerTree(ctx context.Context, id string) error { + if ctx == nil { + ctx = context.Background() + } + state, err := s.Get(id) + if err != nil { + return err + } + if !state.NeedsCanceledWorkerCleanup() { + return nil } + ref, _ := state.ProcessReference() + return s.waitCanceledWorkerCleanup(ctx, id, ref) +} - return s.failAbandoned(state) +func needsStateReconciliation(state *State) bool { + return state != nil && !state.Status.Terminal() && + !state.LaunchPending +} + +func hasProcessReference(state *State) bool { + _, ok := state.ProcessReference() + return ok } // List returns every known task, most recently started first. @@ -167,6 +277,205 @@ func (s *Store) List() ([]*State, error) { return states, nil } +// ForWorkspace returns matching tasks, newest first, without changing state. +func (s *Store) ForWorkspace(workspaceID, command string) ([]*State, error) { + states, err := s.List() + if err != nil { + return nil, err + } + matching := make([]*State, 0, len(states)) + for _, state := range states { + if state.WorkspaceID == workspaceID && (command == "" || state.Command == command) { + matching = append(matching, state) + } + } + return matching, nil +} + +// ActiveForWorkspace returns non-terminal tasks for a workspace, newest first. +func (s *Store) ActiveForWorkspace(workspaceID, command string) ([]*State, error) { + states, err := s.ForWorkspace(workspaceID, command) + if err != nil { + return nil, err + } + + active := make([]*State, 0, len(states)) + for _, state := range states { + if state.Status.Terminal() { + continue + } + active = append(active, state) + } + return active, nil +} + +type WorkerObservation struct { + State *State + WorkerGone bool +} + +// WaitForWorkerObservation waits until process metadata is published, the +// task becomes terminal, or its worker lock becomes available. +func (s *Store) WaitForWorkerObservation( + ctx context.Context, + id string, +) (WorkerObservation, error) { + ticker := time.NewTicker(20 * time.Millisecond) + defer ticker.Stop() + for { + observation, ready, err := s.workerObservation(id) + if err != nil { + return WorkerObservation{}, err + } + if ready { + return observation, nil + } + select { + case <-ctx.Done(): + return WorkerObservation{}, ctx.Err() + case <-ticker.C: + } + } +} + +func (s *Store) markProcessCleanupComplete(id string) error { + return s.update(id, func(state *State) { + state.ProcessCleanupComplete = true + }) +} + +func (s *Store) waitCanceledWorkerCleanup( + ctx context.Context, + id string, + ref command.ProcessRef, +) error { + terminated := false + ticker := time.NewTicker(20 * time.Millisecond) + defer ticker.Stop() + for { + if err := ctx.Err(); err != nil { + return err + } + complete, nowTerminated, err := s.cleanupCanceledWorkerTreeStep(id, ref, terminated) + if err != nil || complete { + return err + } + terminated = nowTerminated + select { + case <-ctx.Done(): + return ctx.Err() + case <-ticker.C: + } + } +} + +func (s *Store) cleanupCanceledWorkerTreeStep( + id string, + ref command.ProcessRef, + terminated bool, +) (complete, nowTerminated bool, err error) { + lock, available, err := s.tryWorkerLock(id) + if err != nil { + return false, terminated, fmt.Errorf("inspect canceled task %s worker lock: %w", id, err) + } + if available { + defer func() { _ = lock.Unlock() }() + return true, terminated, s.finishCanceledWorkerCleanup(id, ref, terminated) + } + if !terminated { + if err := s.processController.Terminate(ref); err != nil { + return false, false, fmt.Errorf("terminate canceled task %s process tree: %w", id, err) + } + terminated = true + } + return false, terminated, nil +} + +func (s *Store) finishCanceledWorkerCleanup( + id string, + ref command.ProcessRef, + terminated bool, +) error { + current, err := s.Get(id) + if err != nil || !current.NeedsCanceledWorkerCleanup() { + return err + } + if !terminated || ref.Identity != "" { + if err := s.processController.CleanupAfterExit(ref); err != nil { + return fmt.Errorf("clean up canceled task %s process tree: %w", id, err) + } + } + return s.markProcessCleanupComplete(id) +} + +func (s *Store) workerObservation(id string) (WorkerObservation, bool, error) { + state, err := s.Get(id) + if err != nil { + return WorkerObservation{}, false, err + } + if state.Status.Terminal() || hasProcessReference(state) { + return WorkerObservation{State: state}, true, nil + } + lock, available, err := s.tryWorkerLock(id) + if err != nil { + return WorkerObservation{}, false, err + } + if !available { + return WorkerObservation{}, false, nil + } + _ = lock.Unlock() + return WorkerObservation{State: state, WorkerGone: true}, true, nil +} + +func (s *Store) reconcileClaimedTask(ctx context.Context, id string) (*State, error) { + current, err := s.Get(id) + if err != nil || !needsReconciliation(current) { + return current, err + } + if current.CancelRequested { + return s.reconcileClaimedCancellation(ctx, current) + } + cleaned := false + if ref, ok := current.ProcessReference(); ok { + if err := ctx.Err(); err != nil { + return current, err + } + if err := s.processController.CleanupAfterExit(ref); err != nil { + return current, fmt.Errorf("clean up task %s process tree: %w", id, err) + } + cleaned = true + } + return s.finishClaimedReconciliation(current, cleaned) +} + +func (s *Store) reconcileClaimedCancellation(ctx context.Context, current *State) (*State, error) { + if err := ctx.Err(); err != nil { + return current, err + } + if ref, ok := current.ProcessReference(); ok { + if err := s.processController.CleanupAfterExit(ref); err != nil { + return current, fmt.Errorf( + "clean up canceled task %s process tree: %w", current.ID, err, + ) + } + } + if err := s.Open(current.ID).finalizeCanceled(); err != nil { + return current, err + } + return s.Get(current.ID) +} + +func (s *Store) finishClaimedReconciliation(current *State, cleaned bool) (*State, error) { + current = s.failAbandoned(current) + if cleaned && current.Status == StatusFailed && current.Error == ErrAbandoned.Error() { + if err := s.markProcessCleanupComplete(current.ID); err != nil { + return current, err + } + return s.Get(current.ID) + } + return current, nil +} + // failAbandoned records ErrAbandoned for a task whose worker is confirmed gone. // Must be called with the worker lock held. func (s *Store) failAbandoned(state *State) *State { @@ -187,15 +496,38 @@ func (s *Store) failAbandoned(state *State) *State { return reconciled } -// workerAlive guards against signaling a PID whose original worker already -// exited and got recycled to an unrelated process. -func (s *Store) workerAlive(id string) bool { - lock, ok := s.claimDeadWorkerLock(id) - if !ok { - return true +// tryWorkerLock claims the task's worker lock when no worker owns it. Unlike +// claimDeadWorkerLock, it creates the lock file if the worker has not started. +func (s *Store) tryWorkerLock(id string) (*flock.Flock, bool, error) { + path, err := s.workerLockPath(id) + if err != nil { + return nil, false, err } - _ = lock.Unlock() - return false + lock := flock.New(path) + locked, err := lock.TryLock() + if err != nil { + return nil, false, err + } + if !locked { + return nil, false, nil + } + return lock, true, nil +} + +func (s *Store) tryLauncherLock(id string) (*flock.Flock, bool, error) { + path, err := s.launcherLockPath(id) + if err != nil { + return nil, false, err + } + lock := flock.New(path) + locked, err := lock.TryLock() + if err != nil { + return nil, false, err + } + if !locked { + return nil, false, nil + } + return lock, true, nil } // claimDeadWorkerLock acquires the task's worker lock, which only succeeds when @@ -229,6 +561,14 @@ func (s *Store) workerLockPath(id string) (string, error) { return path + ".worker.lock", nil } +func (s *Store) launcherLockPath(id string) (string, error) { + path, err := s.path(id) + if err != nil { + return "", err + } + return path + ".launcher.lock", nil +} + func (s *Store) update(id string, mutate func(*State)) error { return s.withLock(id, func() error { state, err := s.Get(id) diff --git a/pkg/task/store_test.go b/pkg/task/store_test.go new file mode 100644 index 0000000000..208cdcbf46 --- /dev/null +++ b/pkg/task/store_test.go @@ -0,0 +1,437 @@ +package task + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/devsy-org/devsy/pkg/command" + "github.com/stretchr/testify/require" +) + +const workspaceOne = "ws-1" + +const testProcessIdentity = "sess-1" + +func createTaskWith(t *testing.T, store *Store, opts CreateOptions) *Task { + t.Helper() + tk, err := store.Create(opts) + if err != nil { + t.Fatalf("Create: %v", err) + } + return tk +} + +func queryActive(t *testing.T, store *Store, workspaceID, command string) []*State { + t.Helper() + active, err := store.ActiveForWorkspace(workspaceID, command) + if err != nil { + t.Fatalf("ActiveForWorkspace: %v", err) + } + return active +} + +func deadWorkerWithProcess( + t *testing.T, + store *Store, + opts CreateOptions, + ref command.ProcessRef, +) *Task { + t.Helper() + tk := createTaskWith(t, store, opts) + if err := tk.SetProcess(ref); err != nil { + t.Fatalf("SetProcess: %v", err) + } + if err := tk.HoldWorkerLock(); err != nil { + t.Fatalf("HoldWorkerLock: %v", err) + } + if err := tk.ReleaseWorkerLock(); err != nil { + t.Fatalf("ReleaseWorkerLock: %v", err) + } + return tk +} + +func TestReconcileStateMarksCrashedWorkerAbandoned(t *testing.T) { + store := newTestStore(t) + tk := deadWorkerWithProcess( + t, + store, + CreateOptions{Command: "up", WorkspaceID: workspaceOne}, + command.ProcessRef{ + PID: 4242, + TreeKind: command.ProcessTreeUnixGroup, + Identity: testProcessIdentity, + }, + ) + + state, err := store.Get(tk.ID()) + if err != nil { + t.Fatalf("Get: %v", err) + } + if reconciled := store.ReconcileState(state); !reconciled.Status.Terminal() { + t.Fatalf("ReconcileState status = %q, want terminal", reconciled.Status) + } + + persisted, err := store.Get(tk.ID()) + if err != nil { + t.Fatalf("Get: %v", err) + } + if !persisted.Status.Terminal() { + t.Fatalf("persisted status = %q, want terminal", persisted.Status) + } +} + +func TestReconcileTaskFinalizesCanceledWorkerAfterCancellationError(t *testing.T) { + store := newTestStore(t) + ref := command.ProcessRef{ + PID: 4242, TreeKind: command.ProcessTreeUnixGroup, Identity: testProcessIdentity, + } + tk := deadWorkerWithProcess(t, store, + CreateOptions{Command: "up", WorkspaceID: workspaceOne}, ref, + ) + _, err := tk.requestCancel() + require.NoError(t, err) + cleaned := 0 + store.processController = fakeProcessController{ + cleanup: func(got command.ProcessRef) error { + require.Equal(t, ref, got) + cleaned++ + if cleaned == 1 { + return errors.New("cleanup unavailable") + } + return nil + }, + } + _, err = store.ReconcileTask(context.Background(), tk.ID()) + require.ErrorContains(t, err, "cleanup unavailable") + state, err := store.Get(tk.ID()) + require.NoError(t, err) + require.False(t, state.Status.Terminal()) + require.True(t, state.CancelRequested) + + state, err = store.ReconcileTask(context.Background(), tk.ID()) + require.NoError(t, err) + require.True(t, state.Canceled()) + require.True(t, state.ProcessCleanupComplete) + require.Equal(t, 2, cleaned) +} + +func TestReconcileStateFinalizesCanceledWorkerAfterExit(t *testing.T) { + store := newTestStore(t) + ref := command.ProcessRef{ + PID: 4242, TreeKind: command.ProcessTreeUnixGroup, Identity: testProcessIdentity, + } + tk := deadWorkerWithProcess(t, store, + CreateOptions{Command: "up", WorkspaceID: workspaceOne}, ref, + ) + state, err := tk.requestCancel() + require.NoError(t, err) + store.processController = fakeProcessController{ + cleanup: func(got command.ProcessRef) error { + require.Equal(t, ref, got) + return nil + }, + } + reconciled := store.ReconcileState(state) + require.True(t, reconciled.Canceled()) + require.True(t, reconciled.ProcessCleanupComplete) +} + +func TestCleanupExitedWorkerTreeTerminatesCrashedWorkerTree(t *testing.T) { + store := newTestStore(t) + ref := command.ProcessRef{ + PID: 4242, + TreeKind: command.ProcessTreeUnixGroup, + Identity: testProcessIdentity, + } + tk := deadWorkerWithProcess( + t, + store, + CreateOptions{Command: "up", WorkspaceID: workspaceOne}, + ref, + ) + if err := tk.Fail(ErrAbandoned); err != nil { + t.Fatalf("Fail: %v", err) + } + + var cleaned []command.ProcessRef + store.processController = fakeProcessController{ + cleanup: func(got command.ProcessRef) error { + cleaned = append(cleaned, got) + return nil + }, + } + if err := store.CleanupExitedWorkerTree(tk.ID()); err != nil { + t.Fatalf("CleanupExitedWorkerTree: %v", err) + } + if len(cleaned) != 1 || cleaned[0] != ref { + t.Fatalf("cleaned = %+v, want the saved reference", cleaned) + } + if err := store.CleanupExitedWorkerTree(tk.ID()); err != nil { + t.Fatalf("repeat CleanupExitedWorkerTree: %v", err) + } + if len(cleaned) != 1 { + t.Fatalf("cleaned = %+v after repeat, want one cleanup", cleaned) + } + state, err := store.Get(tk.ID()) + require.NoError(t, err) + require.True(t, state.ProcessCleanupComplete, "successful cleanup was not persisted") +} + +func TestCleanupExitedWorkerTreeSkipsLiveWorker(t *testing.T) { + store := newTestStore(t) + tk := createTaskWith(t, store, CreateOptions{Command: "up", WorkspaceID: workspaceOne}) + if err := tk.SetProcess( + command.ProcessRef{ + PID: 4242, + TreeKind: command.ProcessTreeUnixGroup, + Identity: testProcessIdentity, + }, + ); err != nil { + t.Fatalf("SetProcess: %v", err) + } + if err := tk.HoldWorkerLock(); err != nil { + t.Fatalf("HoldWorkerLock: %v", err) + } + t.Cleanup(func() { _ = tk.ReleaseWorkerLock() }) + + store.processController = fakeProcessController{ + cleanup: func(command.ProcessRef) error { + t.Error("cleaned up a live worker's tree") + return nil + }, + } + if err := store.CleanupExitedWorkerTree(tk.ID()); err != nil { + t.Fatalf("CleanupExitedWorkerTree: %v", err) + } +} + +func TestCleanupExitedWorkerTreeDoesNotReportSuccessWhileLockHeld(t *testing.T) { + store := newTestStore(t) + tk := createTaskWith(t, store, CreateOptions{Command: "up", WorkspaceID: workspaceOne}) + if err := tk.SetProcess(command.ProcessRef{ + PID: 4242, TreeKind: command.ProcessTreeUnixGroup, Identity: testProcessIdentity, + }); err != nil { + t.Fatalf("SetProcess: %v", err) + } + if err := tk.HoldWorkerLock(); err != nil { + t.Fatalf("HoldWorkerLock: %v", err) + } + t.Cleanup(func() { _ = tk.ReleaseWorkerLock() }) + if err := tk.Fail(ErrAbandoned); err != nil { + t.Fatalf("Fail: %v", err) + } + if err := store.CleanupExitedWorkerTree(tk.ID()); err == nil { + t.Fatal("cleanup reported success while worker lock was held") + } +} + +func TestDeleteKeepsAbandonedTaskWhenTreeCleanupFails(t *testing.T) { + store := newTestStore(t) + tk := deadWorkerWithProcess(t, store, + CreateOptions{Command: "up", WorkspaceID: workspaceOne}, + command.ProcessRef{ + PID: 4242, + TreeKind: command.ProcessTreeUnixGroup, + Identity: testProcessIdentity, + }, + ) + require.NoError(t, tk.Fail(ErrAbandoned)) + store.processController = fakeProcessController{ + cleanup: func(command.ProcessRef) error { return errors.New("cleanup failed") }, + } + for _, force := range []bool{false, true} { + require.ErrorContains(t, store.Delete(tk.ID(), force), "cleanup failed") + _, err := store.Get(tk.ID()) + require.NoError(t, err) + } +} + +func TestCleanupCanceledWorkerTreeReapsLegacyLiveWorker(t *testing.T) { + store := newTestStore(t) + tk := createTaskWith(t, store, CreateOptions{Command: "up", WorkspaceID: workspaceOne}) + ref := command.ProcessRef{PID: 4242, TreeKind: command.ProcessTreeLegacyPID} + require.NoError(t, tk.SetProcess(ref)) + require.NoError(t, tk.HoldWorkerLock()) + t.Cleanup(func() { _ = tk.ReleaseWorkerLock() }) + require.NoError(t, tk.Fail(ErrCanceled)) + terminated := 0 + store.processController = fakeProcessController{ + terminate: func(got command.ProcessRef) error { + require.Equal(t, ref, got) + terminated++ + return tk.ReleaseWorkerLock() + }, + cleanup: func(command.ProcessRef) error { + t.Fatal("identityless live tree was cleaned without a verified termination") + return nil + }, + } + require.NoError(t, store.CleanupCanceledWorkerTree(context.Background(), tk.ID())) + require.NoError(t, store.CleanupCanceledWorkerTree(context.Background(), tk.ID())) + require.Equal(t, 1, terminated) + state, err := store.Get(tk.ID()) + require.NoError(t, err) + require.True(t, state.ProcessCleanupComplete) +} + +func TestCleanupCanceledWorkerTreeDoesNotClearUnverifiedLegacyRecord(t *testing.T) { + store := newTestStore(t) + tk := deadWorkerWithProcess(t, store, + CreateOptions{Command: "up", WorkspaceID: workspaceOne}, + command.ProcessRef{PID: 4242, TreeKind: command.ProcessTreeLegacyPID}, + ) + require.NoError(t, tk.Fail(ErrCanceled)) + store.processController = fakeProcessController{ + cleanup: func(command.ProcessRef) error { return errors.New("cannot verify descendants") }, + } + require.ErrorContains(t, + store.CleanupCanceledWorkerTree(context.Background(), tk.ID()), + "cannot verify descendants", + ) + state, err := store.Get(tk.ID()) + require.NoError(t, err) + require.False(t, state.ProcessCleanupComplete) +} + +func TestActiveForWorkspaceFilters(t *testing.T) { + store := newTestStore(t) + + up := createTaskWith(t, store, CreateOptions{Command: "up", WorkspaceID: workspaceOne}) + _ = createTaskWith(t, store, CreateOptions{Command: "build", WorkspaceID: workspaceOne}) + otherWorkspace := createTaskWith(t, store, CreateOptions{Command: "up", WorkspaceID: "ws-2"}) + terminal := createTaskWith(t, store, CreateOptions{Command: "up", WorkspaceID: workspaceOne}) + if err := terminal.Succeed(nil); err != nil { + t.Fatalf("Succeed: %v", err) + } + + active := queryActive(t, store, workspaceOne, "up") + if len(active) != 1 || active[0].ID != up.ID() { + t.Errorf("command-filtered match = %+v, want only the live up task", active) + } + if all := queryActive(t, store, workspaceOne, ""); len(all) != 2 { + t.Errorf("unfiltered match = %d, want 2", len(all)) + } + if none := queryActive(t, store, otherWorkspace.ID(), "up"); len(none) != 0 { + t.Errorf("task id used as workspace id matched %d tasks, want 0", len(none)) + } +} + +func TestActiveForWorkspaceOrdersNewestFirst(t *testing.T) { + store := newTestStore(t) + + older := createTaskWith(t, store, CreateOptions{Command: "up", WorkspaceID: workspaceOne}) + if err := store.update(older.ID(), func(s *State) { + s.StartedAt = s.StartedAt.Add(-time.Hour) + }); err != nil { + t.Fatalf("backdate: %v", err) + } + newer := createTaskWith(t, store, CreateOptions{Command: "up", WorkspaceID: workspaceOne}) + + active, err := store.ActiveForWorkspace(workspaceOne, "up") + if err != nil { + t.Fatalf("ActiveForWorkspace: %v", err) + } + if len(active) != 2 || active[0].ID != newer.ID() || active[1].ID != older.ID() { + t.Errorf("unexpected order/contents: %+v", active) + } +} + +func TestActiveForWorkspaceDoesNotReconcileAbandonedWorkers(t *testing.T) { + store := newTestStore(t) + tk := createTaskWith(t, store, CreateOptions{Command: "up", WorkspaceID: workspaceOne}) + + // A worker that claimed then lost its lock died without recording a + // result; the query must reconcile it instead of reporting it active. + if err := tk.HoldWorkerLock(); err != nil { + t.Fatalf("HoldWorkerLock: %v", err) + } + if err := tk.ReleaseWorkerLock(); err != nil { + t.Fatalf("ReleaseWorkerLock: %v", err) + } + + active, err := store.ActiveForWorkspace(workspaceOne, "up") + if err != nil { + t.Fatalf("ActiveForWorkspace: %v", err) + } + if len(active) != 1 || active[0].ID != tk.ID() { + t.Errorf("query omitted unreconciled task: %+v", active) + } + + state, err := store.Get(tk.ID()) + if err != nil { + t.Fatalf("Get: %v", err) + } + if state.Status.Terminal() { + t.Errorf("query changed task status to %q", state.Status) + } +} + +func TestActiveForWorkspaceDoesNotCleanAbandonedProcessTree(t *testing.T) { + store := newTestStore(t) + state := abandonTask(t, store) + if err := store.Open(state.ID).SetWorkspaceID(workspaceOne); err != nil { + t.Fatalf("SetWorkspaceID: %v", err) + } + if err := setPIDForTest(store.Open(state.ID)); err != nil { + t.Fatalf("SetPID: %v", err) + } + + killCalls := 0 + store.processController = fakeProcessController{ + terminate: func(command.ProcessRef) error { killCalls++; return nil }, + cleanup: func(command.ProcessRef) error { killCalls++; return nil }, + } + + active, err := store.ActiveForWorkspace(workspaceOne, "up") + if err != nil { + t.Fatalf("ActiveForWorkspace: %v", err) + } + if len(active) != 1 || active[0].ID != state.ID { + t.Fatalf("query omitted an unreconciled task: %+v", active) + } + if killCalls != 0 { + t.Errorf("query invoked process cleanup %d times", killCalls) + } +} + +func TestActiveForWorkspaceDoesNotMutateTaskWhenTreeCleanupWouldFail(t *testing.T) { + store := newTestStore(t) + state := abandonTask(t, store) + if err := store.Open(state.ID).SetWorkspaceID(workspaceOne); err != nil { + t.Fatalf("SetWorkspaceID: %v", err) + } + if err := setPIDForTest(store.Open(state.ID)); err != nil { + t.Fatalf("SetPID: %v", err) + } + store.processController = fakeProcessController{ + terminate: func(command.ProcessRef) error { return errors.New("unexpected terminate") }, + cleanup: func(command.ProcessRef) error { return errors.New("descendant cleanup failed") }, + } + + active, err := store.ActiveForWorkspace(workspaceOne, "up") + if err != nil { + t.Fatalf("ActiveForWorkspace: %v", err) + } + if len(active) != 1 || active[0].ID != state.ID { + t.Fatalf("task was excluded after failed tree cleanup: %+v", active) + } + if active[0].Status.Terminal() { + t.Fatalf("task marked terminal %q after failed tree cleanup", active[0].Status) + } +} + +func TestActiveForWorkspaceTreatsLiveWorkerAsActive(t *testing.T) { + store := newTestStore(t) + tk := createTaskWith(t, store, CreateOptions{Command: "up", WorkspaceID: workspaceOne}) + holdWorkerLock(t, tk) + + active, err := store.ActiveForWorkspace(workspaceOne, "up") + if err != nil { + t.Fatalf("ActiveForWorkspace: %v", err) + } + if len(active) != 1 || active[0].ID != tk.ID() { + t.Errorf("live worker task not reported active: %+v", active) + } +} diff --git a/pkg/task/task.go b/pkg/task/task.go index 7cf6c82463..b935927c23 100644 --- a/pkg/task/task.go +++ b/pkg/task/task.go @@ -3,11 +3,11 @@ package task import ( + "context" "encoding/json" "errors" "fmt" "os" - "strconv" "time" "github.com/devsy-org/devsy/pkg/clierr" @@ -46,23 +46,28 @@ func (s Status) Terminal() bool { // doing the work, distinct from whatever process is merely polling this // state. type State struct { - ID string `json:"id"` - Command string `json:"command,omitempty"` - WorkspaceID string `json:"workspaceId,omitempty"` - Status Status `json:"status"` - Phase string `json:"phase,omitempty"` - Step string `json:"step,omitempty"` - OperationID string `json:"operationId,omitempty"` - ParentOperationID string `json:"parentOperationId,omitempty"` - DurationMs int64 `json:"durationMs,omitempty"` - Error string `json:"error,omitempty"` - ErrorCode string `json:"errorCode,omitempty"` - ErrorHint string `json:"errorHint,omitempty"` - ErrorContext map[string]string `json:"errorContext,omitempty"` - Result *config.Result `json:"result,omitempty"` - PID int `json:"pid,omitempty"` - StartedAt time.Time `json:"startedAt"` - UpdatedAt time.Time `json:"updatedAt"` + ID string `json:"id"` + Command string `json:"command,omitempty"` + WorkspaceID string `json:"workspaceId,omitempty"` + Status Status `json:"status"` + Phase string `json:"phase,omitempty"` + Step string `json:"step,omitempty"` + OperationID string `json:"operationId,omitempty"` + ParentOperationID string `json:"parentOperationId,omitempty"` + DurationMs int64 `json:"durationMs,omitempty"` + Error string `json:"error,omitempty"` + ErrorCode string `json:"errorCode,omitempty"` + ErrorHint string `json:"errorHint,omitempty"` + ErrorContext map[string]string `json:"errorContext,omitempty"` + Result *config.Result `json:"result,omitempty"` + PID int `json:"pid,omitempty"` + ProcessTreeIdentity string `json:"processTreeIdentity,omitempty"` + Process *command.ProcessRef `json:"process,omitempty"` + ProcessCleanupComplete bool `json:"processCleanupComplete,omitempty"` + CancelRequested bool `json:"cancelRequested,omitempty"` + LaunchPending bool `json:"launchPending,omitempty"` + StartedAt time.Time `json:"startedAt"` + UpdatedAt time.Time `json:"updatedAt"` } // CreateOptions labels a task at creation time for later listing. @@ -78,20 +83,119 @@ type Task struct { store *Store id string // Held for the worker process's lifetime; see HoldWorkerLock. - workerLock *flock.Flock + workerLock *flock.Flock + launcherLock *flock.Flock } func (t *Task) ID() string { return t.id } +func (s *State) Canceled() bool { + return s != nil && s.Status == StatusFailed && s.ErrorCode == string(clierr.CodeCanceled) +} + +func (s *State) ProcessReference() (command.ProcessRef, bool) { + if s == nil { + return command.ProcessRef{}, false + } + if s.Process != nil { + if s.Process.PID > 0 { + return *s.Process, true + } + } + if s.PID <= 0 { + return command.ProcessRef{}, false + } + return command.ProcessRef{ + PID: s.PID, + TreeKind: command.ProcessTreeLegacyPID, + TreeID: WorkerProcessName(s.ID), + Identity: s.ProcessTreeIdentity, + }, true +} + +// NeedsExitedWorkerCleanup identifies abandoned tasks whose saved tree has +// not yet been checked. Other terminal tasks completed their own teardown. +func (s *State) NeedsExitedWorkerCleanup() bool { + if s == nil || s.Status != StatusFailed || s.Error != ErrAbandoned.Error() || + s.ProcessCleanupComplete { + return false + } + _, ok := s.ProcessReference() + return ok +} + +// NeedsCanceledWorkerCleanup includes records written by versions that marked +// cancellation complete before the worker tree was actually terminated. +func (s *State) NeedsCanceledWorkerCleanup() bool { + if !s.Canceled() || s.ProcessCleanupComplete { + return false + } + _, ok := s.ProcessReference() + return ok +} + +func (t *Task) BeginLaunch() error { + launcherLock, available, err := t.store.tryLauncherLock(t.id) + if err != nil { + return fmt.Errorf("lock task %s launcher: %w", t.id, err) + } + if !available { + return fmt.Errorf("task %s already has a launcher", t.id) + } + var canceled bool + err = t.store.update(t.id, func(s *State) { + if s.Status.Terminal() || s.CancelRequested { + canceled = true + return + } + s.LaunchPending = true + }) + if err != nil { + _ = launcherLock.Unlock() + return err + } + if canceled { + _ = launcherLock.Unlock() + return ErrCanceled + } + t.launcherLock = launcherLock + return nil +} + +func (t *Task) FinishLaunch() error { + err := t.store.update(t.id, func(s *State) { s.LaunchPending = false }) + return errors.Join(err, releaseLauncherLock(t)) +} + +func (t *Task) SetProcess(ref command.ProcessRef) error { + if ref.PID <= 0 { + return fmt.Errorf("invalid worker PID %d", ref.PID) + } + err := t.store.update(t.id, func(s *State) { + s.Process = &ref + s.PID = ref.PID + s.ProcessTreeIdentity = ref.Identity + s.LaunchPending = false + }) + return errors.Join(err, releaseLauncherLock(t)) +} + +// SetPID reads legacy task state and tests that publish a worker after launch. func (t *Task) SetPID(pid int) error { - return t.store.update(t.id, func(s *State) { - s.PID = pid + identity, err := command.ProcessTreeIdentity(pid) + if err != nil { + return fmt.Errorf("identify process tree for pid %d: %w", pid, err) + } + return t.SetProcess(command.ProcessRef{ + PID: pid, + TreeKind: command.ProcessTreeLegacyPID, + TreeID: WorkerProcessName(t.id), + Identity: identity, }) } -// HoldWorkerLock claims this task's worker lock for the rest of the process's -// life, marking it as actively being worked on. Callers must not release it: -// the kernel does so when the process exits, crashes, or is killed. +// HoldWorkerLock claims this task's worker lock while its worker is active. +// Release it only when startup aborts before work begins. // // Returns an error if another process already holds the lock, since that means // a worker for this task is already running. @@ -113,12 +217,8 @@ func (t *Task) HoldWorkerLock() error { return nil } -// ReleaseWorkerLockForTest drops the lock HoldWorkerLock acquired, letting a -// test simulate a dead worker. Production code must never call it. -// -// Not in export_test.go: other packages' tests need it, and a _test.go file is -// only compiled into its own package's test binary. -func (t *Task) ReleaseWorkerLockForTest() error { +// ReleaseWorkerLock releases the worker claim when startup aborts before work begins. +func (t *Task) ReleaseWorkerLock() error { if t.workerLock == nil { return nil } @@ -130,6 +230,18 @@ func (t *Task) ReleaseWorkerLockForTest() error { return nil } +func releaseLauncherLock(t *Task) error { + if t.launcherLock == nil { + return nil + } + lock := t.launcherLock + t.launcherLock = nil + if err := lock.Unlock(); err != nil { + return fmt.Errorf("unlock task %s launcher: %w", t.id, err) + } + return nil +} + // SetWorkspaceID corrects the task's workspace label to the resolved ID, // which may differ from whatever label it was created with (e.g. a raw // source string guessed before workspace resolution ran). client.Status @@ -148,7 +260,7 @@ func (t *Task) Reporter() status.Reporter { // concurrently with a Cancel can't overwrite the canceled state with success. func (t *Task) Succeed(result *config.Result) error { return t.store.update(t.id, func(s *State) { - if s.Status.Terminal() { + if s.Status.Terminal() || s.CancelRequested { return } s.Status = StatusSucceeded @@ -160,30 +272,234 @@ func (t *Task) Succeed(result *config.Result) error { }) } -// Cancel is safe to call even if the process already exited on its own. The -// terminal check and state transition happen atomically under the same -// lock, so a concurrent report from the task's own worker can't race with -// marking it canceled here. +// Cancel terminates the worker before recording cancellation. A termination +// failure leaves the task retryable. func (t *Task) Cancel() error { - var pid int - err := t.store.update(t.id, func(s *State) { - if s.Status.Terminal() { - return + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + return t.CancelContext(ctx) +} + +func (t *Task) CancelContext(ctx context.Context) error { + if ctx == nil { + ctx = context.Background() + } + if err := ctx.Err(); err != nil { + return err + } + state, err := t.requestCancel() + if err != nil { + return err + } + if !state.Status.Terminal() { + if err := quiesceCancellation(t, ctx); err != nil { + return err } - pid = s.PID - s.Status = StatusFailed - s.Error = ErrCanceled.Error() - s.ErrorCode = string(clierr.CodeCanceled) - s.ErrorHint = "Retry the operation when ready." - s.ErrorContext = nil - }) + } + if err := t.store.CleanupCanceledWorkerTree(ctx, t.id); err != nil { + return err + } + return t.store.CleanupExitedWorkerTree(t.id) +} + +func quiesceCancellation(t *Task, ctx context.Context) error { + var terminated *command.ProcessRef + for { + retry, newTerminated, err := quiesceCancellationStep(t, ctx, terminated) + if err != nil { + return err + } + if !retry { + return nil + } + terminated = newTerminated + if err := waitForCancellation(ctx); err != nil { + return fmt.Errorf("cancel task %s: %w", t.id, err) + } + } +} + +func quiesceCancellationStep( + t *Task, + ctx context.Context, + terminated *command.ProcessRef, +) (bool, *command.ProcessRef, error) { + state, err := t.store.Get(t.id) + if err != nil { + return false, nil, fmt.Errorf("cancel task %s: read state: %w", t.id, err) + } + if state.Status.Terminal() { + return false, nil, nil + } + finalized, err := tryFinalizeWithoutWorker(t, ctx) + if err != nil { + return false, nil, err + } + if finalized { + return false, nil, nil + } + + retry, ref, err := processCancellationObservation(t, state, terminated, ctx) + if err != nil { + return false, nil, err + } + if retry { + return true, terminated, nil + } + if ref != nil { + terminated = ref + } + return true, terminated, nil +} + +func processCancellationObservation( + t *Task, + state *State, + terminated *command.ProcessRef, + ctx context.Context, +) (bool, *command.ProcessRef, error) { + ref, hasRef := state.ProcessReference() + if hasRef { + if err := terminateWorker(t, ref, terminated, ctx); err != nil { + return false, nil, err + } + return false, &ref, nil + } + observation, err := t.store.WaitForWorkerObservation(ctx, t.id) + if err != nil { + return false, nil, fmt.Errorf("cancel task %s: wait for worker startup: %w", t.id, err) + } + return observation.WorkerGone, nil, nil +} + +func tryFinalizeWithoutWorker(t *Task, ctx context.Context) (bool, error) { + workerLock, available, err := t.store.tryWorkerLock(t.id) + if err != nil { + return false, fmt.Errorf("cancel task %s: inspect worker lock: %w", t.id, err) + } + if !available { + return false, nil + } + if t.store.afterCancelLockClaimedHook != nil { + t.store.afterCancelLockClaimedHook() + } + return true, cancelWithoutWorker(t, ctx, workerLock) +} + +func terminateWorker( + t *Task, + ref command.ProcessRef, + terminated *command.ProcessRef, + ctx context.Context, +) error { + if terminated != nil && *terminated == ref { + return nil + } + if err := t.store.processController.Terminate(ref); err != nil { + finalized, lockErr := tryFinalizeWithoutWorker(t, ctx) + if lockErr == nil && finalized { + return nil + } + return fmt.Errorf("cancel task %s: terminate worker tree: %w", t.id, err) + } + return nil +} + +func cancelWithoutWorker(t *Task, ctx context.Context, workerLock *flock.Flock) error { + defer func() { _ = workerLock.Unlock() }() + state, err := waitForLaunch(t, ctx) if err != nil { return err } - if pid == 0 || !t.store.workerAlive(t.id) { + if state.Status.Terminal() { + return nil + } + if ref, ok := state.ProcessReference(); ok { + if err := t.store.processController.CleanupAfterExit(ref); err != nil { + return fmt.Errorf("cancel task %s: clean up worker tree: %w", t.id, err) + } + } + return t.finalizeCanceled() +} + +func waitForLaunch(t *Task, ctx context.Context) (*State, error) { + ticker := time.NewTicker(20 * time.Millisecond) + defer ticker.Stop() + for { + state, done, err := waitForLaunchStep(t) + if err != nil { + return nil, err + } + if done { + return state, nil + } + select { + case <-ctx.Done(): + return nil, fmt.Errorf( + "cancel task %s: wait for launch publication: %w", + t.id, + ctx.Err(), + ) + case <-ticker.C: + } + } +} + +func waitForLaunchStep(t *Task) (*State, bool, error) { + state, err := t.store.Get(t.id) + if err != nil { + return nil, false, fmt.Errorf("cancel task %s: read state: %w", t.id, err) + } + if state.Status.Terminal() || !state.LaunchPending { + return state, true, nil + } + resolved, err := clearAbandonedLaunch(t) + if err != nil { + return nil, false, err + } + if resolved { + return nil, false, nil + } + return nil, false, nil +} + +func clearAbandonedLaunch(t *Task) (bool, error) { + launcherLock, available, err := t.store.tryLauncherLock(t.id) + if err != nil { + return false, fmt.Errorf("cancel task %s: inspect launcher lock: %w", t.id, err) + } + if !available { + return false, nil + } + defer func() { _ = launcherLock.Unlock() }() + err = t.store.update(t.id, func(current *State) { + if current.CancelRequested && current.LaunchPending { + current.LaunchPending = false + } + }) + if err != nil { + return false, fmt.Errorf("cancel task %s: clear abandoned launch: %w", t.id, err) + } + return true, nil +} + +func waitForCancellation(ctx context.Context) error { + timer := time.NewTimer(20 * time.Millisecond) + defer timer.Stop() + select { + case <-ctx.Done(): + return ctx.Err() + case <-timer.C: return nil } - return command.Kill(strconv.Itoa(pid)) +} + +func markCanceled(s *State) { + s.Status = StatusFailed + s.Error = ErrCanceled.Error() + s.ErrorCode = string(clierr.CodeCanceled) + s.ErrorHint = "Retry the operation when ready." + s.ErrorContext = nil } // Fail preserves an existing terminal state, so the error a canceled worker @@ -205,7 +521,7 @@ func (t *Task) Fail(err error) error { } redactor := secrets.NewEnvironmentRedactor(os.Environ()) return t.store.update(t.id, func(s *State) { - if s.Status.Terminal() { + if s.Status.Terminal() || s.CancelRequested { return } s.Status = StatusFailed @@ -219,6 +535,31 @@ func (t *Task) Fail(err error) error { }) } +func (t *Task) requestCancel() (*State, error) { + var snapshot *State + err := t.store.update(t.id, func(state *State) { + if !state.Status.Terminal() { + state.CancelRequested = true + } + stateSnapshot := *state + snapshot = &stateSnapshot + }) + if err != nil { + return nil, err + } + return snapshot, nil +} + +func (t *Task) finalizeCanceled() error { + return t.store.update(t.id, func(state *State) { + if !state.Status.Terminal() && state.CancelRequested { + markCanceled(state) + state.LaunchPending = false + state.ProcessCleanupComplete = true + } + }) +} + type taskReporter struct { task *Task } @@ -228,7 +569,7 @@ func (r taskReporter) Report(e status.Event) { _ = r.task.store.update(r.task.id, func(s *State) { // A terminal task is done being described; late events from a worker // still unwinding must not resurrect it or rewrite its outcome. - if s.Status.Terminal() { + if s.Status.Terminal() || s.CancelRequested { return } if e.State == status.StateFailed { diff --git a/pkg/task/task_pid_supported_test.go b/pkg/task/task_pid_supported_test.go index 7c72c7836b..4faa282a96 100644 --- a/pkg/task/task_pid_supported_test.go +++ b/pkg/task/task_pid_supported_test.go @@ -4,6 +4,7 @@ package task import ( "os/exec" + "syscall" "testing" "time" ) @@ -17,10 +18,21 @@ func TestCancelSignalsALiveWorkersPID(t *testing.T) { holdWorkerLock(t, tk) worker := exec.Command("sleep", "30") + worker.SysProcAttr = &syscall.SysProcAttr{Setsid: true} if err := worker.Start(); err != nil { t.Fatalf("start: %v", err) } - defer func() { _ = worker.Process.Kill() }() + workerExited := make(chan struct{}) + var workerWaitErr error + go func() { + workerWaitErr = worker.Wait() + _ = tk.ReleaseWorkerLock() + close(workerExited) + }() + t.Cleanup(func() { + _ = worker.Process.Kill() + <-workerExited + }) if err := tk.SetPID(worker.Process.Pid); err != nil { t.Fatalf("SetPID: %v", err) } @@ -29,12 +41,60 @@ func TestCancelSignalsALiveWorkersPID(t *testing.T) { t.Fatalf("Cancel: %v", err) } - waitErr := worker.Wait() - if waitErr == nil { + <-workerExited + if workerWaitErr == nil { t.Fatal("worker process exited cleanly, want it signaled by Cancel") } } +func TestCancelUsesPublishedProcessIdentity(t *testing.T) { + store := newTestStore(t) + tk, state := newTaskWithWorkerIdentity(t, store) + if err := tk.HoldWorkerLock(); err != nil { + t.Fatalf("HoldWorkerLock: %v", err) + } + + var gotIdentity string + setKillProcessWithIdentityForTest(store, tk, func(_, _, identity string) error { + gotIdentity = identity + return nil + }) + if err := tk.Cancel(); err != nil { + t.Fatalf("Cancel: %v", err) + } + if state.ProcessTreeIdentity == "" || gotIdentity != state.ProcessTreeIdentity { + t.Fatalf( + "kill identity = %q, want saved identity %q", + gotIdentity, + state.ProcessTreeIdentity, + ) + } +} + +func newTaskWithWorkerIdentity(t *testing.T, store *Store) (*Task, *State) { + t.Helper() + tk, err := store.Create(CreateOptions{}) + if err != nil { + t.Fatalf("Create: %v", err) + } + worker := exec.Command("sleep", "30") + if err := worker.Start(); err != nil { + t.Fatalf("start worker: %v", err) + } + t.Cleanup(func() { + _ = worker.Process.Kill() + _ = worker.Wait() + }) + if err := tk.SetPID(worker.Process.Pid); err != nil { + t.Fatalf("SetPID: %v", err) + } + state, err := store.Get(tk.ID()) + if err != nil { + t.Fatalf("Get: %v", err) + } + return tk, state +} + func TestCancelDoesNotSignalAProcessThatReusedThePID(t *testing.T) { store := newTestStore(t) tk, err := store.Create(CreateOptions{}) @@ -47,8 +107,8 @@ func TestCancelDoesNotSignalAProcessThatReusedThePID(t *testing.T) { if err := tk.HoldWorkerLock(); err != nil { t.Fatalf("HoldWorkerLock: %v", err) } - if err := tk.ReleaseWorkerLockForTest(); err != nil { - t.Fatalf("ReleaseWorkerLockForTest: %v", err) + if err := tk.ReleaseWorkerLock(); err != nil { + t.Fatalf("ReleaseWorkerLock: %v", err) } innocent := exec.Command("sleep", "30") diff --git a/pkg/task/task_pid_windows_test.go b/pkg/task/task_pid_windows_test.go new file mode 100644 index 0000000000..3af1ce7de5 --- /dev/null +++ b/pkg/task/task_pid_windows_test.go @@ -0,0 +1,111 @@ +//go:build windows + +package task + +import ( + "errors" + "os/exec" + "strconv" + "testing" + "time" + + "github.com/devsy-org/devsy/pkg/command" + "github.com/devsy-org/devsy/pkg/config" +) + +// sleepWorker returns a long-running process using only tools a Windows +// runner always has. +func sleepWorker() *exec.Cmd { + return exec.Command("ping", "-n", "30", "127.0.0.1") +} + +func TestCancelSignalsALiveWorkersPID(t *testing.T) { + t.Setenv(config.EnvHome, t.TempDir()) + store := newTestStore(t) + tk, err := store.Create(CreateOptions{}) + if err != nil { + t.Fatalf("Create: %v", err) + } + holdWorkerLock(t, tk) + controller := command.DefaultProcessController() + store.processController = fakeProcessController{ + terminate: func(ref command.ProcessRef) error { + return errors.Join(controller.Terminate(ref), tk.ReleaseWorkerLock()) + }, + cleanup: controller.CleanupAfterExit, + } + worker := sleepWorker() + workerName := WorkerProcessName(tk.ID()) + if err := command.StartSupervisedBackground(command.SupervisedStartOptions{ + Name: workerName, + OnStarted: func(ref command.ProcessRef) error { + return tk.SetProcess(ref) + }, + }, func() (*exec.Cmd, error) { + return worker, nil + }); err != nil { + t.Fatalf("start supervised worker: %v", err) + } + pid := worker.Process.Pid + t.Cleanup(func() { _ = command.KillTree(strconv.Itoa(pid), workerName) }) + + if err := tk.Cancel(); err != nil { + t.Fatalf("Cancel: %v", err) + } + + running, err := command.IsRunning(strconv.Itoa(pid)) + if err != nil { + t.Fatalf("IsRunning: %v", err) + } + if running { + t.Fatalf("worker process %d is still running after Cancel", pid) + } +} + +func TestCancelDoesNotSignalAProcessThatReusedThePID(t *testing.T) { + store := newTestStore(t) + tk, err := store.Create(CreateOptions{}) + if err != nil { + t.Fatalf("Create: %v", err) + } + // Claim then release the worker lock: the kernel frees a dead worker's + // lock the same way, leaving its old PID free for the OS to hand to an + // unrelated process. + if err := tk.HoldWorkerLock(); err != nil { + t.Fatalf("HoldWorkerLock: %v", err) + } + if err := tk.ReleaseWorkerLock(); err != nil { + t.Fatalf("ReleaseWorkerLock: %v", err) + } + + innocent := sleepWorker() + if err := innocent.Start(); err != nil { + t.Fatalf("start: %v", err) + } + defer func() { _ = innocent.Process.Kill() }() + if err := tk.SetPID(innocent.Process.Pid); err != nil { + t.Fatalf("SetPID: %v", err) + } + exited := waitAsync(innocent) + + if err := tk.Cancel(); err != nil { + t.Fatalf("Cancel: %v", err) + } + + assertStillRunning(t, exited) +} + +func waitAsync(cmd *exec.Cmd) <-chan error { + exited := make(chan error, 1) + go func() { exited <- cmd.Wait() }() + return exited +} + +func assertStillRunning(t *testing.T, exited <-chan error) { + t.Helper() + select { + case err := <-exited: + t.Fatalf("process exited unexpectedly: %v", err) + case <-time.After(500 * time.Millisecond): + } +} diff --git a/pkg/task/task_test.go b/pkg/task/task_test.go index c7bfc0aa63..1340efe92c 100644 --- a/pkg/task/task_test.go +++ b/pkg/task/task_test.go @@ -1,15 +1,21 @@ package task import ( + "context" "errors" "fmt" + "os" + "strconv" "strings" "sync" "testing" "time" + "github.com/devsy-org/devsy/pkg/clierr" + "github.com/devsy-org/devsy/pkg/command" "github.com/devsy-org/devsy/pkg/devcontainer/config" "github.com/devsy-org/devsy/pkg/status" + "github.com/gofrs/flock" ) func newTestStore(t *testing.T) *Store { @@ -329,7 +335,7 @@ func TestSetPIDPersists(t *testing.T) { t.Fatalf("Create: %v", err) } - if err := task.SetPID(4242); err != nil { + if err := task.SetPID(os.Getpid()); err != nil { t.Fatalf("SetPID: %v", err) } @@ -337,8 +343,108 @@ func TestSetPIDPersists(t *testing.T) { if err != nil { t.Fatalf("Get: %v", err) } - if state.PID != 4242 { - t.Errorf("PID = %d, want 4242", state.PID) + if state.PID != os.Getpid() { + t.Errorf("PID = %d, want %d", state.PID, os.Getpid()) + } + if state.Process == nil || state.Process.PID != os.Getpid() || + state.Process.TreeKind != command.ProcessTreeLegacyPID { + t.Errorf("process reference = %+v, want legacy PID reference", state.Process) + } +} + +func TestCancelIntentWaitsForProcessPublicationAndCleanup(t *testing.T) { + store := newTestStore(t) + tk, err := store.Create(CreateOptions{}) + if err != nil { + t.Fatalf("Create: %v", err) + } + if err := tk.BeginLaunch(); err != nil { + t.Fatalf("BeginLaunch: %v", err) + } + + cleaned := make(chan command.ProcessRef, 1) + store.processController = fakeProcessController{ + cleanup: func(ref command.ProcessRef) error { + cleaned <- ref + return nil + }, + } + cancelDone := make(chan error, 1) + go func() { cancelDone <- tk.Cancel() }() + waitForCancelRequest(t, store, tk.ID()) + + assertCancelIntent(t, store, tk.ID()) + if err := tk.Succeed(&config.Result{}); err != nil { + t.Fatalf("Succeed during cancellation: %v", err) + } + if err := tk.SetProcess(command.ProcessRef{ + PID: 4242, + TreeKind: command.ProcessTreeUnixGroup, + TreeID: "4242", + }); err != nil { + t.Fatalf("SetProcess: %v", err) + } + + assertProcessCleaned(t, cleaned, cancelDone) + assertTaskCanceledResult(t, store, tk.ID()) +} + +func assertCancelIntent(t *testing.T, store *Store, id string) { + state, err := store.Get(id) + if err != nil { + t.Fatalf("Get pending cancellation: %v", err) + } + if state.Status.Terminal() || !state.CancelRequested { + t.Fatalf("cancel intent state = %+v, want non-terminal requested state", state) + } +} + +func assertProcessCleaned(t *testing.T, cleaned chan command.ProcessRef, cancelDone chan error) { + select { + case ref := <-cleaned: + if ref.PID != 4242 || ref.TreeID != "4242" { + t.Fatalf("cleaned process = %+v", ref) + } + case <-time.After(time.Second): + t.Fatal("cancellation finalized without process-tree cleanup") + } + if err := <-cancelDone; err != nil { + t.Fatalf("Cancel: %v", err) + } +} + +func assertTaskCanceledResult(t *testing.T, store *Store, id string) { + state, err := store.Get(id) + if err != nil { + t.Fatalf("Get canceled task: %v", err) + } + if !state.Canceled() || state.Result != nil { + t.Fatalf("final cancellation state = %+v", state) + } +} + +func TestCancelFinalizesLaunchAbandonedByLauncher(t *testing.T) { + store := newTestStore(t) + tk, err := store.Create(CreateOptions{}) + if err != nil { + t.Fatalf("Create: %v", err) + } + if err := tk.BeginLaunch(); err != nil { + t.Fatalf("BeginLaunch: %v", err) + } + if err := releaseLauncherLock(tk); err != nil { + t.Fatalf("release abandoned launcher lock: %v", err) + } + + if err := tk.Cancel(); err != nil { + t.Fatalf("Cancel: %v", err) + } + state, err := store.Get(tk.ID()) + if err != nil { + t.Fatalf("Get: %v", err) + } + if !state.Canceled() || state.LaunchPending { + t.Fatalf("abandoned launch state = %+v", state) } } @@ -363,7 +469,7 @@ func TestCancelWithoutPIDMarksFailed(t *testing.T) { if state.Error != ErrCanceled.Error() { t.Errorf("error = %q, want %q", state.Error, ErrCanceled.Error()) } - if state.ErrorCode != "canceled" || state.ErrorHint == "" { + if state.ErrorCode != string(clierr.CodeCanceled) || state.ErrorHint == "" { t.Errorf( "cancellation metadata = (%q, %q), want stable code and hint", state.ErrorCode, @@ -372,6 +478,137 @@ func TestCancelWithoutPIDMarksFailed(t *testing.T) { } } +func TestCancelHoldsWorkerLockThroughCancellationCommit(t *testing.T) { + store := newTestStore(t) + tk, err := store.Create(CreateOptions{}) + if err != nil { + t.Fatalf("Create: %v", err) + } + + lockClaimed := make(chan struct{}) + continueCancel := make(chan struct{}) + store.afterCancelLockClaimedHook = func() { + close(lockClaimed) + <-continueCancel + } + cancelDone := make(chan error, 1) + go func() { cancelDone <- tk.Cancel() }() + <-lockClaimed + + path, err := store.workerLockPath(tk.ID()) + if err != nil { + t.Fatalf("workerLockPath: %v", err) + } + workerLock := flock.New(path) + assertWorkerLock(t, workerLock, false, "while Cancel owns worker lock") + + close(continueCancel) + if err := <-cancelDone; err != nil { + t.Fatalf("Cancel: %v", err) + } + assertWorkerLock(t, workerLock, true, "after Cancel") + defer func() { _ = workerLock.Unlock() }() + + requireCanceledState(t, store, tk.ID()) +} + +func assertWorkerLock(t *testing.T, lock *flock.Flock, wantLocked bool, when string) { + t.Helper() + locked, err := lock.TryLock() + if err != nil { + t.Fatalf("TryLock %s: %v", when, err) + } + if locked != wantLocked { + if locked { + _ = lock.Unlock() + } + t.Fatalf("worker lock held %t %s, want %t", locked, when, wantLocked) + } +} + +func waitForCancelRequest(t *testing.T, store *Store, id string) { + t.Helper() + deadline := time.NewTimer(time.Second) + ticker := time.NewTicker(time.Millisecond) + defer deadline.Stop() + defer ticker.Stop() + for { + state, err := store.Get(id) + if err != nil { + t.Fatalf("Get: %v", err) + } + if state.CancelRequested { + return + } + select { + case <-deadline.C: + t.Fatal("cancellation request was not persisted") + case <-ticker.C: + } + } +} + +func TestOverlappingCancelsBeforePIDBothSucceed(t *testing.T) { + store := newTestStore(t) + tk, err := store.Create(CreateOptions{}) + if err != nil { + t.Fatalf("Create: %v", err) + } + + lockClaimed := make(chan struct{}) + continueCancel := make(chan struct{}) + store.afterCancelLockClaimedHook = func() { + close(lockClaimed) + <-continueCancel + } + firstDone := make(chan error, 1) + go func() { firstDone <- tk.Cancel() }() + <-lockClaimed + firstState, err := store.Get(tk.ID()) + if err != nil { + t.Fatalf("Get first cancellation state: %v", err) + } + + secondDone := make(chan error, 1) + secondStarted := make(chan struct{}) + go func() { + close(secondStarted) + secondDone <- tk.Cancel() + }() + <-secondStarted + waitForStateUpdate(t, store, tk.ID(), firstState) + close(continueCancel) + + if err := <-firstDone; err != nil { + t.Fatalf("first Cancel: %v", err) + } + if err := <-secondDone; err != nil { + t.Fatalf("second Cancel: %v", err) + } + requireCanceledState(t, store, tk.ID()) +} + +func waitForStateUpdate(t *testing.T, store *Store, tkID string, firstState *State) { + deadline := time.NewTimer(time.Second) + ticker := time.NewTicker(time.Millisecond) + defer deadline.Stop() + defer ticker.Stop() + for { + state, err := store.Get(tkID) + if err != nil { + t.Fatalf("Get overlapping cancellation state: %v", err) + } + if state.UpdatedAt.After(firstState.UpdatedAt) { + break + } + select { + case <-deadline.C: + t.Fatal("second cancellation did not publish its request") + case <-ticker.C: + } + } +} + func TestCancelOnTerminalTaskIsNoop(t *testing.T) { store := newTestStore(t) task, err := store.Create(CreateOptions{}) @@ -564,8 +801,8 @@ func holdWorkerLock(t *testing.T, tk *Task) { t.Fatalf("HoldWorkerLock: %v", err) } t.Cleanup(func() { - if err := tk.ReleaseWorkerLockForTest(); err != nil { - t.Errorf("ReleaseWorkerLockForTest: %v", err) + if err := tk.ReleaseWorkerLock(); err != nil { + t.Errorf("ReleaseWorkerLock: %v", err) } }) } @@ -625,8 +862,8 @@ func TestAbandonedDetectsReleasedLock(t *testing.T) { if err := tk.HoldWorkerLock(); err != nil { t.Fatalf("HoldWorkerLock: %v", err) } - if err := tk.ReleaseWorkerLockForTest(); err != nil { - t.Fatalf("ReleaseWorkerLockForTest: %v", err) + if err := tk.ReleaseWorkerLock(); err != nil { + t.Fatalf("ReleaseWorkerLock: %v", err) } state, err := store.Get(tk.ID()) if err != nil { @@ -663,8 +900,8 @@ func abandonTask(t *testing.T, store *Store) *State { if err := tk.HoldWorkerLock(); err != nil { t.Fatalf("HoldWorkerLock: %v", err) } - if err := tk.ReleaseWorkerLockForTest(); err != nil { - t.Fatalf("ReleaseWorkerLockForTest: %v", err) + if err := tk.ReleaseWorkerLock(); err != nil { + t.Fatalf("ReleaseWorkerLock: %v", err) } state, err := store.Get(tk.ID()) if err != nil { @@ -673,11 +910,20 @@ func abandonTask(t *testing.T, store *Store) *State { return state } +func reconcileTask(t *testing.T, store *Store, id string) *State { + t.Helper() + state, err := store.ReconcileTask(context.Background(), id) + if err != nil { + t.Fatalf("ReconcileTask: %v", err) + } + return state +} + func TestReconcileFailsAbandonedTask(t *testing.T) { store := newTestStore(t) state := abandonTask(t, store) - reconciled := store.Reconcile(state) + reconciled := reconcileTask(t, store, state.ID) if reconciled.Status != StatusFailed { t.Errorf("status = %s, want %s", reconciled.Status, StatusFailed) @@ -691,7 +937,7 @@ func TestReconcilePersistsTheFailure(t *testing.T) { store := newTestStore(t) state := abandonTask(t, store) - store.Reconcile(state) + reconcileTask(t, store, state.ID) persisted, err := store.Get(state.ID) if err != nil { @@ -711,8 +957,8 @@ func TestReconcileDoesNotFailARestartedWorker(t *testing.T) { if err := tk.HoldWorkerLock(); err != nil { t.Fatalf("HoldWorkerLock: %v", err) } - if err := tk.ReleaseWorkerLockForTest(); err != nil { - t.Fatalf("ReleaseWorkerLockForTest: %v", err) + if err := tk.ReleaseWorkerLock(); err != nil { + t.Fatalf("ReleaseWorkerLock: %v", err) } stale, err := store.Get(tk.ID()) if err != nil { @@ -724,16 +970,16 @@ func TestReconcileDoesNotFailARestartedWorker(t *testing.T) { // Reconcile still holds it. restarted := store.Open(tk.ID()) claimed := false - store.SetAfterClaimForTest(func() { + store.afterClaimHook = func() { claimed = restarted.HoldWorkerLock() == nil - }) + } - reconciled := store.Reconcile(stale) + reconciled := reconcileTask(t, store, stale.ID) if claimed { t.Error("a replacement worker acquired the lock during Reconcile") - if err := restarted.ReleaseWorkerLockForTest(); err != nil { - t.Errorf("ReleaseWorkerLockForTest: %v", err) + if err := restarted.ReleaseWorkerLock(); err != nil { + t.Errorf("ReleaseWorkerLock: %v", err) } } // With the lock held throughout, the reconcile is the only writer and the @@ -760,11 +1006,11 @@ func TestReconcileKeepsAWorkerRecordedResult(t *testing.T) { if err := tk.Succeed(nil); err != nil { t.Fatalf("Succeed: %v", err) } - if err := tk.ReleaseWorkerLockForTest(); err != nil { - t.Fatalf("ReleaseWorkerLockForTest: %v", err) + if err := tk.ReleaseWorkerLock(); err != nil { + t.Fatalf("ReleaseWorkerLock: %v", err) } - reconciled := store.Reconcile(stale) + reconciled := reconcileTask(t, store, stale.ID) if reconciled.Status != StatusSucceeded { t.Errorf("status = %s, want %s", reconciled.Status, StatusSucceeded) @@ -786,7 +1032,7 @@ func TestReconcileLeavesLiveTaskAlone(t *testing.T) { t.Fatalf("Get: %v", err) } - if got := store.Reconcile(state).Status; got != StatusPending { + if got := reconcileTask(t, store, state.ID).Status; got != StatusPending { t.Errorf("status = %s, want %s (unchanged)", got, StatusPending) } } @@ -805,11 +1051,11 @@ func TestReconcilePreservesCancellationReason(t *testing.T) { t.Fatalf("Get: %v", err) } - reconciled := store.Reconcile(state) + reconciled := reconcileTask(t, store, state.ID) if reconciled.Error != ErrCanceled.Error() { t.Errorf("error = %q, want %q", reconciled.Error, ErrCanceled.Error()) } - if reconciled.ErrorCode != "canceled" || reconciled.ErrorHint == "" { + if reconciled.ErrorCode != string(clierr.CodeCanceled) || reconciled.ErrorHint == "" { t.Errorf( "cancellation metadata = (%q, %q), want stable code and hint", reconciled.ErrorCode, @@ -817,3 +1063,376 @@ func TestReconcilePreservesCancellationReason(t *testing.T) { ) } } + +func newLiveWorkerTask(t *testing.T, store *Store) *Task { + t.Helper() + tk, err := store.Create(CreateOptions{}) + if err != nil { + t.Fatalf("Create: %v", err) + } + holdWorkerLock(t, tk) + if err := setPIDForTest(tk); err != nil { + t.Fatalf("setPIDForTest: %v", err) + } + return tk +} + +func setPIDForTest(tk *Task) error { + return tk.store.update(tk.id, func(s *State) { + s.PID = 4242 + s.ProcessTreeIdentity = "test-process-tree-identity" + }) +} + +type fakeProcessController struct { + terminate func(command.ProcessRef) error + cleanup func(command.ProcessRef) error +} + +func (f fakeProcessController) Terminate(ref command.ProcessRef) error { + if f.terminate == nil { + return nil + } + return f.terminate(ref) +} + +func (f fakeProcessController) CleanupAfterExit(ref command.ProcessRef) error { + if f.cleanup == nil { + return nil + } + return f.cleanup(ref) +} + +func setKillProcessForTest(store *Store, worker *Task, fn func(pid, treeName string) error) { + invoke := func(ref command.ProcessRef) error { + return fn(strconv.Itoa(ref.PID), ref.TreeID) + } + store.processController = fakeProcessController{ + terminate: func(ref command.ProcessRef) error { + if err := invoke(ref); err != nil { + return err + } + if worker != nil { + _ = worker.ReleaseWorkerLock() + } + return nil + }, + cleanup: invoke, + } +} + +func setKillProcessWithIdentityForTest( + store *Store, + worker *Task, + fn func(pid, treeName, identity string) error, +) { + store.processController = fakeProcessController{ + terminate: func(ref command.ProcessRef) error { + if err := fn(strconv.Itoa(ref.PID), ref.TreeID, ref.Identity); err != nil { + return err + } + if worker != nil { + _ = worker.ReleaseWorkerLock() + } + return nil + }, + cleanup: func(ref command.ProcessRef) error { + return fn(strconv.Itoa(ref.PID), ref.TreeID, ref.Identity) + }, + } +} + +func requireCanceledState(t *testing.T, store *Store, id string) { + t.Helper() + state, err := store.Get(id) + if err != nil { + t.Fatalf("Get: %v", err) + } + if state.Status != StatusFailed || + state.Error != ErrCanceled.Error() || + state.ErrorCode != string(clierr.CodeCanceled) { + t.Errorf("unexpected final state: %+v", state) + } +} + +func TestCancelKillFailureRemainsRetryable(t *testing.T) { + store := newTestStore(t) + tk := newLiveWorkerTask(t, store) + + killCalls := 0 + setKillProcessForTest(store, tk, func(pid, treeName string) error { + killCalls++ + if killCalls == 1 { + return errors.New("boom") + } + return nil + }) + + if err := tk.Cancel(); err == nil { + t.Fatal("first Cancel = nil, want kill failure") + } + state, err := store.Get(tk.ID()) + if err != nil { + t.Fatalf("Get: %v", err) + } + if state.Status.Terminal() { + t.Fatalf( + "task recorded terminal %q while its worker survived the failed kill", + state.Status, + ) + } + + if err := tk.Cancel(); err != nil { + t.Fatalf("retry Cancel: %v", err) + } + if killCalls != 3 { + t.Errorf("process control invoked %d times, want terminate + retry + cleanup", killCalls) + } + requireCanceledState(t, store, tk.ID()) +} + +func TestCancelLegacyLiveWorkerUsesPIDBoundTreeTermination(t *testing.T) { + store := newTestStore(t) + tk, err := store.Create(CreateOptions{}) + if err != nil { + t.Fatalf("Create: %v", err) + } + holdWorkerLock(t, tk) + if err := store.update(tk.ID(), func(state *State) { state.PID = 4242 }); err != nil { + t.Fatalf("set legacy PID: %v", err) + } + var actual killCall + setKillProcessForTest(store, tk, func(pid, tree string) error { + actual = killCall{calls: actual.calls + 1, pid: pid, tree: tree} + return nil + }) + if err := tk.Cancel(); err != nil { + t.Fatalf("Cancel: %v", err) + } + if actual.calls != 2 || actual.pid != "4242" || actual.tree != WorkerProcessName(tk.ID()) { + t.Errorf("kill call = %+v, want terminate and cleanup", actual) + } + requireCanceledState(t, store, tk.ID()) +} + +func TestCancelSuccessfulKillCommitsCanceled(t *testing.T) { + store := newTestStore(t) + tk := newLiveWorkerTask(t, store) + + setKillProcessForTest(store, tk, func(pid, treeName string) error { + state, err := store.Get(tk.ID()) + if err != nil { + t.Errorf("Get during kill: %v", err) + } + if state.Status.Terminal() { + t.Errorf("task recorded %q before its worker was terminated", state.Status) + } + return nil + }) + + if err := tk.Cancel(); err != nil { + t.Fatalf("Cancel: %v", err) + } + requireCanceledState(t, store, tk.ID()) +} + +func TestCancelAwaitsPIDPublication(t *testing.T) { + store := newTestStore(t) + tk, err := store.Create(CreateOptions{}) + if err != nil { + t.Fatalf("Create: %v", err) + } + holdWorkerLock(t, tk) + + var killedPID, killedTree string + setKillProcessForTest(store, tk, func(pid, treeName string) error { + killedPID, killedTree = pid, treeName + return nil + }) + + publishPID := make(chan struct{}) + go func() { + <-publishPID + if err := setPIDForTest(tk); err != nil { + t.Errorf("setPIDForTest: %v", err) + } + }() + + cancelDone := make(chan error, 1) + go func() { cancelDone <- tk.Cancel() }() + waitForCancelRequest(t, store, tk.ID()) + close(publishPID) + if err := <-cancelDone; err != nil { + t.Fatalf("Cancel: %v", err) + } + if killedPID != "4242" { + t.Errorf("kill pid = %q, want 4242", killedPID) + } + if killedTree != WorkerProcessName(tk.ID()) { + t.Errorf("kill tree = %q, want %q", killedTree, WorkerProcessName(tk.ID())) + } + requireCanceledState(t, store, tk.ID()) +} + +func TestCancelUnpublishedPIDStaysRetryable(t *testing.T) { + store := newTestStore(t) + tk, err := store.Create(CreateOptions{}) + if err != nil { + t.Fatalf("Create: %v", err) + } + holdWorkerLock(t, tk) + if err := tk.BeginLaunch(); err != nil { + t.Fatalf("BeginLaunch: %v", err) + } + + setKillProcessForTest(store, tk, func(pid, treeName string) error { + t.Error("kill invoked before the worker published a pid") + return nil + }) + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Millisecond) + defer cancel() + if err := tk.CancelContext(ctx); err == nil { + t.Fatal("Cancel = nil, want unpublished-pid error") + } + state, err := store.Get(tk.ID()) + if err != nil { + t.Fatalf("Get: %v", err) + } + if state.Status.Terminal() { + t.Fatalf("task recorded terminal %q with no killed worker", state.Status) + } +} + +func TestCancelDeadWorkerStillCleansUpProcessTree(t *testing.T) { + store := newTestStore(t) + tk, err := store.Create(CreateOptions{}) + if err != nil { + t.Fatalf("Create: %v", err) + } + if err := tk.HoldWorkerLock(); err != nil { + t.Fatalf("HoldWorkerLock: %v", err) + } + if err := tk.ReleaseWorkerLock(); err != nil { + t.Fatalf("ReleaseWorkerLock: %v", err) + } + if err := setPIDForTest(tk); err != nil { + t.Fatalf("setPIDForTest: %v", err) + } + + var actualKill killCall + setKillProcessForTest(store, tk, func(pid, treeName string) error { + actualKill.calls++ + actualKill.pid, actualKill.tree = pid, treeName + return nil + }) + + if err := tk.Cancel(); err != nil { + t.Fatalf("Cancel: %v", err) + } + assertKilledTree(t, actualKill, "4242", WorkerProcessName(tk.ID())) + requireCanceledState(t, store, tk.ID()) +} + +type killCall struct { + calls int + pid string + tree string +} + +func assertKilledTree(t *testing.T, actual killCall, wantPID, wantTree string) { + t.Helper() + if actual.calls != 1 || actual.pid != wantPID || actual.tree != wantTree { + t.Errorf( + "kill calls=(%d, %q, %q), want (1, %q, %q)", + actual.calls, + actual.pid, + actual.tree, + wantPID, + wantTree, + ) + } +} + +func TestCancelDeadWorkerCleanupFailureRemainsRetryable(t *testing.T) { + store := newTestStore(t) + tk, err := store.Create(CreateOptions{}) + if err != nil { + t.Fatalf("Create: %v", err) + } + if err := tk.HoldWorkerLock(); err != nil { + t.Fatalf("HoldWorkerLock: %v", err) + } + if err := tk.ReleaseWorkerLock(); err != nil { + t.Fatalf("ReleaseWorkerLock: %v", err) + } + if err := setPIDForTest(tk); err != nil { + t.Fatalf("setPIDForTest: %v", err) + } + setKillProcessForTest(store, tk, func(string, string) error { + return errors.New("descendant cleanup failed") + }) + + if err := tk.Cancel(); err == nil { + t.Fatal("Cancel = nil, want process-tree cleanup error") + } + state, err := store.Get(tk.ID()) + if err != nil { + t.Fatalf("Get: %v", err) + } + if state.Status.Terminal() { + t.Fatalf("task recorded terminal %q despite failed tree cleanup", state.Status) + } +} + +func TestCancelUsesIdentityCleanupWhenWorkerExitsDuringKill(t *testing.T) { + store := newTestStore(t) + tk := newLiveWorkerTask(t, store) + killCalls := 0 + setKillProcessForTest(store, tk, func(string, string) error { + killCalls++ + if killCalls == 1 { + if err := tk.ReleaseWorkerLock(); err != nil { + t.Errorf("ReleaseWorkerLock: %v", err) + } + return errors.New("worker exited before tree lookup") + } + return nil + }) + + if err := tk.Cancel(); err != nil { + t.Fatalf("Cancel: %v", err) + } + if killCalls != 2 { + t.Errorf("kill calls = %d, want 2 (live attempt and identity cleanup)", killCalls) + } + requireCanceledState(t, store, tk.ID()) +} + +func TestCancelRacingLateSuccessStillWinsAfterSuccessfulTermination(t *testing.T) { + store := newTestStore(t) + tk, err := store.Create(CreateOptions{}) + if err != nil { + t.Fatalf("Create: %v", err) + } + holdWorkerLock(t, tk) + if err := setPIDForTest(tk); err != nil { + t.Fatalf("setPIDForTest: %v", err) + } + + setKillProcessForTest(store, tk, func(pid, treeName string) error { + // The worker reports its own success while the kill unwinds it. + return tk.Succeed(&config.Result{}) + }) + + if err := tk.Cancel(); err != nil { + t.Fatalf("Cancel: %v", err) + } + state, err := store.Get(tk.ID()) + if err != nil { + t.Fatalf("Get: %v", err) + } + if state.Status != StatusFailed || state.Error != ErrCanceled.Error() { + t.Errorf("late worker success overwrote the accepted cancellation: %+v", state) + } +} diff --git a/pkg/tunnel/local_listener.go b/pkg/tunnel/local_listener.go index 410d9ca291..9f9707c1e3 100644 --- a/pkg/tunnel/local_listener.go +++ b/pkg/tunnel/local_listener.go @@ -20,6 +20,7 @@ type LocalTunnel struct { cancel context.CancelFunc wg sync.WaitGroup dialFunc DialFunc + healthCheckFunc func(ctx context.Context) error healthCheckInterval time.Duration } @@ -31,8 +32,13 @@ type DialFunc func(ctx context.Context) (io.ReadWriteCloser, error) type LocalTunnelOptions struct { // BasePort is the starting port to search from (default: 10800) BasePort int - // DialFunc creates a connection to the remote SSH endpoint + // DialFunc is the data path: it connects to the remote SSH endpoint + // for accepted local connections. DialFunc DialFunc + // HealthCheckFunc is the control-plane liveness probe. It must stay + // observational: never start, restart, or otherwise mutate the resource + // it probes. When nil, the listener lifetime follows its context. + HealthCheckFunc func(ctx context.Context) error // HealthCheckInterval overrides the default health check interval (for testing). // If zero, defaults to healthCheckInterval (30s). HealthCheckInterval time.Duration @@ -76,8 +82,11 @@ func NewLocalTunnel(ctx context.Context, opts LocalTunnelOptions) (*LocalTunnel, _ = listener.Close() }() - t.wg.Add(1) - go t.healthCheck() + if opts.HealthCheckFunc != nil { + t.healthCheckFunc = opts.HealthCheckFunc + t.wg.Add(1) + go t.healthCheck() + } return t, nil } @@ -112,8 +121,7 @@ func (t *LocalTunnel) healthCheck() { case <-t.ctx.Done(): return case <-ticker.C: - conn, err := t.dialFunc(t.ctx) - if err != nil { + if err := t.healthCheckFunc(t.ctx); err != nil { failures++ log.Debugf( "tunnel health check failed (%d/%d): %v", @@ -130,7 +138,6 @@ func (t *LocalTunnel) healthCheck() { return } } else { - _ = conn.Close() failures = 0 } } diff --git a/pkg/tunnel/local_listener_test.go b/pkg/tunnel/local_listener_test.go index dddc3c9fcb..c022436fc2 100644 --- a/pkg/tunnel/local_listener_test.go +++ b/pkg/tunnel/local_listener_test.go @@ -7,6 +7,7 @@ import ( "io" "net" "sync" + "sync/atomic" "syscall" "testing" "time" @@ -77,9 +78,9 @@ func listenerDialClosed(err error) bool { return errors.Is(err, syscall.ECONNREFUSED) || errors.Is(err, syscall.ECONNRESET) } -func waitForListenerClosed(t *testing.T, addr string, timeout time.Duration) { +func waitForListenerClosed(t *testing.T, addr string) { t.Helper() - deadline := time.NewTimer(timeout) + deadline := time.NewTimer(2 * time.Second) defer deadline.Stop() ticker := time.NewTicker(10 * time.Millisecond) defer ticker.Stop() @@ -194,7 +195,7 @@ func TestLocalTunnel_CloseStopsAccepting(t *testing.T) { addr := tun.Addr() _ = tun.Close() - waitForListenerClosed(t, addr, 2*time.Second) + waitForListenerClosed(t, addr) } func TestLocalTunnel_ContextCancellation(t *testing.T) { @@ -215,7 +216,7 @@ func TestLocalTunnel_ContextCancellation(t *testing.T) { cancel() // The listener is closed asynchronously after cancellation. - waitForListenerClosed(t, addr, 2*time.Second) + waitForListenerClosed(t, addr) } func TestLocalTunnel_HealthCheckShutdown(t *testing.T) { @@ -226,6 +227,9 @@ func TestLocalTunnel_HealthCheckShutdown(t *testing.T) { DialFunc: func(ctx context.Context) (io.ReadWriteCloser, error) { return nil, fmt.Errorf("workspace gone") }, + HealthCheckFunc: func(context.Context) error { + return fmt.Errorf("workspace gone") + }, HealthCheckInterval: 50 * time.Millisecond, }) if err != nil { @@ -234,5 +238,92 @@ func TestLocalTunnel_HealthCheckShutdown(t *testing.T) { defer func() { _ = tun.Close() }() // The health check should shut down the tunnel after 3 failures. - waitForListenerClosed(t, tun.Addr(), 2*time.Second) + waitForListenerClosed(t, tun.Addr()) +} + +func TestLocalTunnel_HealthCheckUsesHealthFuncNotDialFunc(t *testing.T) { + ctx := t.Context() + + // DialFunc always succeeds: only a health loop driven by the failing + // HealthCheckFunc may shut the listener down. + var healthCalls atomic.Int32 + tun, err := NewLocalTunnel(ctx, LocalTunnelOptions{ + BasePort: 18600, + DialFunc: func(ctx context.Context) (io.ReadWriteCloser, error) { + local, remote := net.Pipe() + _ = local.Close() + return remote, nil + }, + HealthCheckFunc: func(context.Context) error { + healthCalls.Add(1) + return fmt.Errorf("workspace gone") + }, + HealthCheckInterval: 50 * time.Millisecond, + }) + if err != nil { + t.Fatalf("NewLocalTunnel: %v", err) + } + defer func() { _ = tun.Close() }() + + waitForListenerClosed(t, tun.Addr()) + if got := healthCalls.Load(); got < 3 { + t.Errorf("health probe invoked %d times, want at least 3", got) + } +} + +func TestLocalTunnel_AcceptedConnectionStillUsesDialFunc(t *testing.T) { + ctx := t.Context() + echoServer := newEchoServer(t) + + var dialCalls atomic.Int32 + tun, err := NewLocalTunnel(ctx, LocalTunnelOptions{ + BasePort: 18700, + DialFunc: func(ctx context.Context) (io.ReadWriteCloser, error) { + dialCalls.Add(1) + return net.Dial("tcp", echoServer.Addr().String()) + }, + HealthCheckFunc: func(context.Context) error { + return nil + }, + HealthCheckInterval: 50 * time.Millisecond, + }) + if err != nil { + t.Fatalf("NewLocalTunnel: %v", err) + } + defer func() { _ = tun.Close() }() + + msg := []byte("data plane") + if buf := sendAndReceive(t, tun.Addr(), msg); string(buf) != string(msg) { + t.Errorf("expected %q, got %q", msg, buf) + } + if got := dialCalls.Load(); got == 0 { + t.Error("accepted connection did not invoke DialFunc") + } +} + +func TestLocalTunnel_HealthyStatusKeepsTunnelAlive(t *testing.T) { + ctx := t.Context() + + tun, err := NewLocalTunnel(ctx, LocalTunnelOptions{ + BasePort: 18800, + DialFunc: func(ctx context.Context) (io.ReadWriteCloser, error) { + return nil, io.EOF + }, + HealthCheckFunc: func(context.Context) error { + return nil + }, + HealthCheckInterval: 20 * time.Millisecond, + }) + if err != nil { + t.Fatalf("NewLocalTunnel: %v", err) + } + defer func() { _ = tun.Close() }() + + // Well past the failure window, a healthy probe keeps the listener up. + time.Sleep(200 * time.Millisecond) + conn, err := net.DialTimeout("tcp", tun.Addr(), time.Second) + if err != nil { + t.Fatalf("listener closed despite healthy probes: %v", err) + } + _ = conn.Close() } diff --git a/pkg/workspace/delete.go b/pkg/workspace/delete.go index 3745424387..9563227f93 100644 --- a/pkg/workspace/delete.go +++ b/pkg/workspace/delete.go @@ -26,6 +26,7 @@ type DeleteOptions struct { Force bool ClientDelete client2.DeleteOptions Owner platform.OwnerFilter + quiescer UpTaskQuiescer } // Delete deletes a workspace, handling imported workspaces, single-machine @@ -56,6 +57,12 @@ func Delete(ctx context.Context, opts DeleteOptions) (string, error) { defer opener.KillBrowserTunnel(client.Context(), client.Workspace()) + // A surviving detached up worker can recreate resources during or after + // the delete, so quiescence failure blocks even a forced delete. + if err := opts.quiesce(ctx, client.Workspace()); err != nil { + return "", err + } + if !opts.Force && client.WorkspaceConfig().Imported { var id string err := progress.RunStep( @@ -77,6 +84,12 @@ func Delete(ctx context.Context, opts DeleteOptions) (string, error) { } defer unlock() + // Re-scan under the lock: a task may have become visible while lock + // acquisition waited. + if err := opts.quiesce(ctx, client.Workspace()); err != nil { + return "", err + } + stopIfRunning(ctx, client, status) var id string @@ -132,26 +145,23 @@ func stopIfRunning( } } -// checkBeforeDelete acquires the lock and verifies the workspace exists -// unless force-deletion is requested. It returns an unlock function that -// must be called by the caller (typically deferred) to release the lock, -// and the resolved workspace status so the caller can decide whether a -// stop is required before delete. +// checkBeforeDelete acquires the workspace lock and returns its status and an +// unlock function. Force skips the status check, not the lock. func checkBeforeDelete( ctx context.Context, client client2.BaseWorkspaceClient, opts DeleteOptions, ) (func(), client2.Status, error) { - force := opts.Force || opts.ClientDelete.Force - if force { - return func() {}, "", nil - } - unlock, err := lockIfNeeded(ctx, client, opts) if err != nil { return nil, "", err } + force := opts.Force || opts.ClientDelete.Force + if force { + return unlock, "", nil + } + var status client2.Status err = progress.RunStep( ctx, @@ -235,14 +245,22 @@ func handleDeleteLoadError( return "", loadErr } - return forceDeleteFolder(opts, workspaceID) + return forceDeleteFolder(ctx, opts, workspaceID) } // forceDeleteFolder removes the workspace folder when the workspace client // cannot be loaded and --force is set. -func forceDeleteFolder(opts DeleteOptions, workspaceID string) (string, error) { +func forceDeleteFolder( + ctx context.Context, + opts DeleteOptions, + workspaceID string, +) (string, error) { log.Errorf("error retrieving workspace, force-deleting folder") + if err := opts.quiesce(ctx, workspaceID); err != nil { + return "", err + } + err := clientimplementation.DeleteWorkspaceFolder( clientimplementation.DeleteWorkspaceFolderParams{ Context: opts.DevsyConfig.DefaultContext, @@ -465,3 +483,18 @@ func removeIfContentOrphan(contextName, contentsDir string, entry os.DirEntry) { } log.Debugf("removed orphan content dir with no matching workspace: workspace=%s", entry.Name()) } + +func (opts DeleteOptions) quiesce(ctx context.Context, workspaceID string) error { + quiescer := opts.quiescer + if quiescer == nil { + quiescer = StoreUpTaskQuiescer{} + } + if err := quiescer.Quiesce(ctx, workspaceID); err != nil { + return fmt.Errorf( + "cancel detached up tasks for workspace %s: %w", + workspaceID, + err, + ) + } + return nil +} diff --git a/pkg/workspace/delete_progress_test.go b/pkg/workspace/delete_progress_test.go index 6e512f34da..5812e232ac 100644 --- a/pkg/workspace/delete_progress_test.go +++ b/pkg/workspace/delete_progress_test.go @@ -12,10 +12,11 @@ import ( type progressDeleteClient struct { client.BaseWorkspaceClient - lock func(context.Context) error - state client.Status - stopErr error - unlocked bool + lock func(context.Context) error + state client.Status + statusCalled bool + stopErr error + unlocked bool } func (c *progressDeleteClient) Lock(ctx context.Context) error { return c.lock(ctx) } @@ -25,6 +26,7 @@ func (c *progressDeleteClient) Status( context.Context, client.StatusOptions, ) (client.Status, error) { + c.statusCalled = true return c.state, nil } func (c *progressDeleteClient) Stop(context.Context, client.StopOptions) error { return c.stopErr } @@ -83,12 +85,15 @@ func TestDeleteProgressPreservesBestEffortStop(t *testing.T) { require.Equal(t, events[0].OperationID, events[1].ParentOperationID) } -func TestDeleteForceSkipsLockAndStatus(t *testing.T) { - c := &progressDeleteClient{} +func TestDeleteForceLocksButSkipsStatus(t *testing.T) { + c := &progressDeleteClient{ + lock: func(context.Context) error { return nil }, + } unlock, _, err := checkBeforeDelete(context.Background(), c, DeleteOptions{Force: true}) require.NoError(t, err) unlock() - require.False(t, c.unlocked) + require.True(t, c.unlocked, "forced delete must still release the workspace lock") + require.False(t, c.statusCalled, "forced delete skips only the status check") } func TestDeleteLockFailureReportsFailure(t *testing.T) { diff --git a/pkg/workspace/delete_test.go b/pkg/workspace/delete_test.go index dd07b9e5e0..06f5b389e1 100644 --- a/pkg/workspace/delete_test.go +++ b/pkg/workspace/delete_test.go @@ -2,6 +2,7 @@ package workspace import ( "context" + "errors" "os" "path/filepath" "testing" @@ -9,9 +10,16 @@ import ( "github.com/devsy-org/devsy/pkg/client" "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/provider" + "github.com/devsy-org/devsy/pkg/task" "github.com/stretchr/testify/require" ) +type testQuiescer func(context.Context, string) error + +func (f testQuiescer) Quiesce(ctx context.Context, workspaceID string) error { + return f(ctx, workspaceID) +} + func TestSweepOrphanWorkspaceDirs(t *testing.T) { setupTestPathManager(t) @@ -107,9 +115,67 @@ func (c *preResolvedDeleteClient) Workspace() string { return c.workspaceID } func (c *preResolvedDeleteClient) WorkspaceConfig() *provider.Workspace { return c.config } -func (c *preResolvedDeleteClient) Context() string { return c.config.Context } -func (c *preResolvedDeleteClient) Provider() string { return DefaultDockerCommand } +func (c *preResolvedDeleteClient) Context() string { return c.config.Context } +func (c *preResolvedDeleteClient) Lock(context.Context) error { return nil } +func (c *preResolvedDeleteClient) Unlock() {} +func (c *preResolvedDeleteClient) Provider() string { return DefaultDockerCommand } func (c *preResolvedDeleteClient) Delete(context.Context, client.DeleteOptions) error { c.deleted = true return nil } + +func TestDeleteQuiescesPersistedUpTaskBeforeDeleting(t *testing.T) { + setupTestPathManager(t) + + store, err := task.NewStore() + require.NoError(t, err) + tk, err := store.Create(task.CreateOptions{Command: "up", WorkspaceID: "chosen"}) + require.NoError(t, err) + + fake := &preResolvedDeleteClient{ + workspaceID: "chosen", + config: &provider.Workspace{ID: "chosen", Context: testDefaultContext}, + } + devsyConfig := &config.Config{ + DefaultContext: testDefaultContext, + Contexts: map[string]*config.ContextConfig{testDefaultContext: {}}, + } + + id, err := Delete(t.Context(), DeleteOptions{ + DevsyConfig: devsyConfig, + Client: fake, + Force: true, + }) + require.NoError(t, err) + require.Equal(t, "chosen", id) + require.True(t, fake.deleted) + + state, err := store.Get(tk.ID()) + require.NoError(t, err) + require.Equal(t, task.StatusFailed, state.Status) + require.Equal(t, task.ErrCanceled.Error(), state.Error) +} + +func TestDeleteBlockedWhenActiveUpTaskCannotBeQuiesced(t *testing.T) { + setupTestPathManager(t) + + fake := &preResolvedDeleteClient{ + workspaceID: "chosen", + config: &provider.Workspace{ID: "chosen", Context: testDefaultContext}, + } + devsyConfig := &config.Config{ + DefaultContext: testDefaultContext, + Contexts: map[string]*config.ContextConfig{testDefaultContext: {}}, + } + + _, err := Delete(t.Context(), DeleteOptions{ + DevsyConfig: devsyConfig, + Client: fake, + Force: true, + quiescer: testQuiescer(func(context.Context, string) error { + return errors.New("boom") + }), + }) + require.Error(t, err) + require.False(t, fake.deleted, "delete must not proceed while a live up task survives") +} diff --git a/pkg/workspace/quiesce.go b/pkg/workspace/quiesce.go new file mode 100644 index 0000000000..7b4ad9a0cf --- /dev/null +++ b/pkg/workspace/quiesce.go @@ -0,0 +1,108 @@ +package workspace + +import ( + "context" + "errors" + "fmt" + "time" + + "github.com/devsy-org/devsy/pkg/log" + "github.com/devsy-org/devsy/pkg/task" +) + +// upTaskCommand is the command label detached up workers are created with. +const upTaskCommand = "up" + +const upTaskCancellationTimeout = 5 * time.Second + +// QuiesceUpTasks cancels active detached up tasks for the workspace. It tries +// every match and returns all failures because a survivor can restart it. +func QuiesceUpTasks(store *task.Store, workspaceID string) error { + return QuiesceUpTasksContext(context.Background(), store, workspaceID) +} + +func QuiesceUpTasksContext(ctx context.Context, store *task.Store, workspaceID string) error { + return quiesceUpTasks(ctx, store, workspaceID, func(ctx context.Context, id string) error { + return store.Open(id).CancelContext(ctx) + }) +} + +func quiesceUpTasks( + ctx context.Context, + store *task.Store, + workspaceID string, + cancelTask func(ctx context.Context, id string) error, +) error { + states, err := store.ForWorkspace(workspaceID, upTaskCommand) + if err != nil { + return fmt.Errorf("list up tasks for workspace %s: %w", workspaceID, err) + } + + var errs []error + handled := 0 + for _, state := range states { + if state.Status.Terminal() && !state.NeedsExitedWorkerCleanup() && + !state.NeedsCanceledWorkerCleanup() { + continue + } + taskCtx, cancel := context.WithTimeout(ctx, upTaskCancellationTimeout) + err := quiesceTask(taskCtx, store, state, cancelTask) + cancel() + if err != nil { + errs = append(errs, fmt.Errorf("quiesce up task %s: %w", state.ID, err)) + continue + } + handled++ + } + if handled > 0 { + log.Debugf( + "quiesced up tasks for workspace %s: tasks=%d failures=%d", + workspaceID, + handled, + len(errs), + ) + } + return errors.Join(errs...) +} + +// A reconciled task is terminal, so cancelling it is a no-op, yet its crashed +// worker may have left descendants that would restart the workspace. +func quiesceTask( + ctx context.Context, + store *task.Store, + state *task.State, + cancelTask func(ctx context.Context, id string) error, +) error { + if !state.Status.Terminal() { + if err := cancelTask(ctx, state.ID); err != nil { + return err + } + } + if err := ctx.Err(); err != nil { + return err + } + if err := store.CleanupCanceledWorkerTree(ctx, state.ID); err != nil { + return err + } + return store.CleanupExitedWorkerTree(state.ID) +} + +type UpTaskQuiescer interface { + Quiesce(context.Context, string) error +} + +type StoreUpTaskQuiescer struct{} + +func (StoreUpTaskQuiescer) Quiesce(ctx context.Context, workspaceID string) error { + store, err := task.NewStore() + if err != nil { + return fmt.Errorf("open task store: %w", err) + } + return QuiesceUpTasksContext(ctx, store, workspaceID) +} + +// QuiesceUpTasksForWorkspace quiesces the workspace's active up tasks +// against the default task store. +func QuiesceUpTasksForWorkspace(workspaceID string) error { + return StoreUpTaskQuiescer{}.Quiesce(context.Background(), workspaceID) +} diff --git a/pkg/workspace/quiesce_test.go b/pkg/workspace/quiesce_test.go new file mode 100644 index 0000000000..0ba7fec789 --- /dev/null +++ b/pkg/workspace/quiesce_test.go @@ -0,0 +1,156 @@ +package workspace + +import ( + "context" + "errors" + "runtime" + "testing" + + "github.com/devsy-org/devsy/pkg/command" + "github.com/devsy-org/devsy/pkg/task" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func newTaskStore(t *testing.T) *task.Store { + t.Helper() + store, err := task.NewStoreAt(t.TempDir()) + require.NoError(t, err) + return store +} + +func createUpTask(t *testing.T, store *task.Store, workspaceID string) *task.Task { + t.Helper() + tk, err := store.Create(task.CreateOptions{Command: "up", WorkspaceID: workspaceID}) + require.NoError(t, err) + return tk +} + +func taskState(t *testing.T, store *task.Store, id string) *task.State { + t.Helper() + state, err := store.Get(id) + require.NoError(t, err) + return state +} + +func TestQuiesceUpTasksCancelsOnlyActiveUpTasksForWorkspace(t *testing.T) { + store := newTaskStore(t) + targetA := createUpTask(t, store, "ws-1") + targetB := createUpTask(t, store, "ws-1") + terminal := createUpTask(t, store, "ws-1") + require.NoError(t, terminal.SetProcess(command.ProcessRef{ + PID: 4242, TreeKind: command.ProcessTreeUnixGroup, TreeID: "4242", + })) + require.NoError(t, terminal.Succeed(nil)) + otherWorkspace := createUpTask(t, store, "ws-2") + otherCommand, err := store.Create(task.CreateOptions{Command: "build", WorkspaceID: "ws-1"}) + require.NoError(t, err) + + require.NoError(t, QuiesceUpTasks(store, "ws-1")) + + for _, id := range []string{targetA.ID(), targetB.ID()} { + state := taskState(t, store, id) + assert.Equal(t, task.StatusFailed, state.Status, "task %s", id) + assert.Equal(t, task.ErrCanceled.Error(), state.Error, "task %s", id) + } + assert.Equal(t, task.StatusSucceeded, taskState(t, store, terminal.ID()).Status) + assert.False(t, taskState(t, store, terminal.ID()).ProcessCleanupComplete) + assert.Equal(t, task.StatusPending, taskState(t, store, otherWorkspace.ID()).Status) + assert.Equal(t, task.StatusPending, taskState(t, store, otherCommand.ID()).Status) +} + +func TestQuiesceUpTasksAttemptsEveryTaskAndAggregatesFailures(t *testing.T) { + store := newTaskStore(t) + failing := createUpTask(t, store, "ws-1") + succeeding := createUpTask(t, store, "ws-1") + + // Both workers stay active; cancellation of one task fails. + for _, tk := range []*task.Task{failing, succeeding} { + require.NoError(t, tk.HoldWorkerLock()) + tk := tk + t.Cleanup(func() { _ = tk.ReleaseWorkerLock() }) + } + err := quiesceUpTasks( + context.Background(), + store, + "ws-1", + func(_ context.Context, id string) error { + if id == failing.ID() { + return errors.New("boom") + } + return store.Open(id).Fail(task.ErrCanceled) + }, + ) + + require.Error(t, err) + assert.Contains(t, err.Error(), failing.ID()) + + // The failing task stays retryable while the other task was still + // attempted and canceled. + assert.False(t, taskState(t, store, failing.ID()).Status.Terminal()) + state := taskState(t, store, succeeding.ID()) + assert.Equal(t, task.StatusFailed, state.Status) + assert.Equal(t, task.ErrCanceled.Error(), state.Error) +} + +func TestQuiesceUpTasksCleansTerminalTaskTreeInsteadOfCancelling(t *testing.T) { + store := newTaskStore(t) + crashed := createUpTask(t, store, "ws-1") + require.NoError(t, crashed.SetProcess(command.ProcessRef{ + PID: 4242, + TreeKind: command.ProcessTreeUnixGroup, + TreeID: "4242", + })) + require.NoError(t, crashed.HoldWorkerLock()) + require.NoError(t, crashed.ReleaseWorkerLock()) + require.NoError(t, crashed.Fail(task.ErrAbandoned)) + + var cancelled []string + require.NoError(t, quiesceUpTasks( + context.Background(), + store, + "ws-1", + func(_ context.Context, id string) error { + cancelled = append(cancelled, id) + return nil + }, + )) + + assert.Empty(t, cancelled, "a terminal task must not be cancelled again") +} + +func TestQuiesceUpTasksCleansWorkerThatBecomesAbandonedDuringCancel(t *testing.T) { + store := newTaskStore(t) + tk := createUpTask(t, store, "ws-1") + ref := command.ProcessRef{PID: 99999999, TreeKind: command.ProcessTreeUnixGroup} + if runtime.GOOS == "windows" { + ref.TreeKind = command.ProcessTreeWindowsJob + ref.TreeID = "devsy-test-missing-job" + } + require.NoError(t, tk.SetProcess(ref)) + require.NoError(t, tk.HoldWorkerLock()) + require.NoError(t, tk.ReleaseWorkerLock()) + require.NoError(t, quiesceUpTasks( + context.Background(), store, "ws-1", + func(context.Context, string) error { return tk.Fail(task.ErrAbandoned) }, + )) + assert.True(t, taskState(t, store, tk.ID()).ProcessCleanupComplete) +} + +func TestQuiesceUpTasksBoundsEachCancellation(t *testing.T) { + store := newTaskStore(t) + createUpTask(t, store, "ws-1") + + err := quiesceUpTasks( + context.Background(), + store, + "ws-1", + func(ctx context.Context, _ string) error { + if _, ok := ctx.Deadline(); !ok { + return errors.New("task cancellation has no deadline") + } + return nil + }, + ) + require.NoError(t, err) +} diff --git a/sites/docs-devsy-sh/content/docs/developing-in-workspaces/stop-and-delete-a-workspace.mdx b/sites/docs-devsy-sh/content/docs/developing-in-workspaces/stop-and-delete-a-workspace.mdx index 904ad87298..339fbce524 100644 --- a/sites/docs-devsy-sh/content/docs/developing-in-workspaces/stop-and-delete-a-workspace.mdx +++ b/sites/docs-devsy-sh/content/docs/developing-in-workspaces/stop-and-delete-a-workspace.mdx @@ -32,6 +32,12 @@ Some providers allow automatic stop of a workspace, usually to save costs when a For example, the Google Cloud provider will shutdown the virtual machine where the workspace is running after it's unused for 10 minutes, by default. This means you need to start the workspace after it has been stopped when you want to reconnect. +### Background up tasks + +Stopping or deleting a workspace first cancels any active detached `devsy workspace up --detach` task for that workspace, so a background up cannot keep recreating or restarting a workspace you asked to stop. +For current detached workers on Linux, macOS, and Windows, `devsy workspace task cancel ` terminates the task's process tree and records the task as canceled. On other Unix systems, or for older task records without a saved process identity, cancellation can fail when Devsy cannot safely identify surviving processes. +While an SSH tunnel is attached, its health checks observe the workspace but never start a stopped one. + ## Delete a Workspace Deleting a workspace means to erase all state of an existing workspace and remove all traces of it within Devsy.