From 1400e9158c10d1e8a1cb9db9f8c9ffaa220f4505 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 19 Oct 2026 21:27:11 -0600 Subject: [PATCH 01/16] fix(workspace): prevent invalid IDs from hanging creation --- .github/workflows/pr-ci.yml | 4 +- cmd/workspace/task.go | 2 +- cmd/workspace/task_tail_test.go | 37 +++ cmd/workspace/up/detach.go | 36 +-- cmd/workspace/up/early_task_test.go | 175 +++++++++++ cmd/workspace/up/up.go | 71 ++++- cmd/workspace/up/up_client.go | 3 + desktop/e2e/workspace-id.e2e.ts | 82 +++++ .../main/__tests__/ipc-workspace-jobs.test.ts | 285 +++++++++++++++++- desktop/src/main/ipc.ts | 168 ++++++----- desktop/src/main/workspace-jobs.ts | 12 +- .../workspace/WorkspaceWizard.svelte | 61 ++-- .../workspace/WorkspaceWizard.test.ts | 63 +++- .../src/lib/utils/workspace-id.test.ts | 40 +++ .../renderer/src/lib/utils/workspace-id.ts | 35 +++ e2e/tests/ssh/detached_validation.go | 227 ++++++++++++++ pkg/command/process.go | 9 + .../process_exit_child_supported_test.go | 14 + .../process_exit_child_windows_test.go | 7 + pkg/command/process_exit_test.go | 62 ++++ pkg/command/process_supported.go | 15 + pkg/command/process_windows.go | 8 + pkg/task/startup_child_supported_test.go | 14 + pkg/task/startup_child_windows_test.go | 7 + pkg/task/startup_test.go | 118 ++++++++ pkg/task/store.go | 44 ++- pkg/workspace/workspace.go | 5 +- pkg/workspace/workspace_test.go | 8 +- .../create-a-workspace.mdx | 14 + 29 files changed, 1450 insertions(+), 176 deletions(-) create mode 100644 cmd/workspace/up/early_task_test.go create mode 100644 desktop/e2e/workspace-id.e2e.ts create mode 100644 desktop/src/renderer/src/lib/utils/workspace-id.test.ts create mode 100644 desktop/src/renderer/src/lib/utils/workspace-id.ts create mode 100644 e2e/tests/ssh/detached_validation.go create mode 100644 pkg/command/process_exit_child_supported_test.go create mode 100644 pkg/command/process_exit_child_windows_test.go create mode 100644 pkg/command/process_exit_test.go create mode 100644 pkg/task/startup_child_supported_test.go create mode 100644 pkg/task/startup_child_windows_test.go create mode 100644 pkg/task/startup_test.go diff --git a/.github/workflows/pr-ci.yml b/.github/workflows/pr-ci.yml index acd0d26358..f87c67393d 100644 --- a/.github/workflows/pr-ci.yml +++ b/.github/workflows/pr-ci.yml @@ -145,7 +145,7 @@ jobs: needs: [changes] if: needs.changes.outputs.go == 'true' runs-on: windows-latest - timeout-minutes: 15 + timeout-minutes: 30 permissions: contents: read steps: @@ -154,6 +154,8 @@ jobs: with: go-version-file: go.mod cache: false + - name: test native Windows detached task lifecycle + run: go test -race -count=1 -timeout 10m ./pkg/command ./pkg/task ./pkg/workspace ./cmd/workspace/up ./cmd/workspace - name: test native Windows transport run: >- go test -race -count=1 -v -timeout 5m ./pkg/docker diff --git a/cmd/workspace/task.go b/cmd/workspace/task.go index 39105ba3d5..684def6376 100644 --- a/cmd/workspace/task.go +++ b/cmd/workspace/task.go @@ -195,7 +195,7 @@ func followTask(ctx context.Context, store *task.Store, opts followTaskOptions) for { tailer.poll(os.Stderr) - state, err := store.Get(opts.id) + state, err := store.ReconcileTask(ctx, opts.id) if err != nil { return err } diff --git a/cmd/workspace/task_tail_test.go b/cmd/workspace/task_tail_test.go index 429b5c436f..150db2cac8 100644 --- a/cmd/workspace/task_tail_test.go +++ b/cmd/workspace/task_tail_test.go @@ -290,3 +290,40 @@ func TestFollowTaskDrainsStatusBeforeResult(t *testing.T) { require.Equal(t, "ide", completion.OperationID) require.Contains(t, lines[2], `"kind":"result"`) } + +func TestFollowTaskReconcilesExitedWorker(t *testing.T) { + config.SetPathManager(fakeRuntimeDirPathManager{dir: t.TempDir()}) + t.Cleanup(config.ResetPathManager) + store, err := task.NewStoreAt(t.TempDir()) + require.NoError(t, err) + tk, err := store.Create(task.CreateOptions{Command: "up"}) + require.NoError(t, err) + require.NoError(t, tk.HoldWorkerLock()) + require.NoError(t, tk.ReleaseWorkerLock()) + streamsPath, err := config.DefaultPathManager(). + ProcessStreamsFile(task.WorkerProcessName(tk.ID())) + require.NoError(t, err) + require.NoError(t, os.WriteFile(streamsPath, []byte("startup failed\n"), 0o600)) + r, w, err := os.Pipe() + require.NoError(t, err) + original := os.Stderr + os.Stderr = w + t.Cleanup(func() { os.Stderr = original; _ = r.Close(); _ = w.Close() }) + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + followErr := followTask( + ctx, + store, + followTaskOptions{id: tk.ID(), interval: time.Millisecond, emitJSON: true}, + ) + os.Stderr = original + require.NoError(t, w.Close()) + logs, err := io.ReadAll(r) + require.NoError(t, err) + require.Error(t, followErr) + require.NotErrorIs(t, followErr, context.DeadlineExceeded) + require.Equal(t, 1, strings.Count(string(logs), "startup failed")) + state, err := store.Get(tk.ID()) + require.NoError(t, err) + require.Equal(t, task.StatusFailed, state.Status) +} diff --git a/cmd/workspace/up/detach.go b/cmd/workspace/up/detach.go index 84b7eddffd..430bab878b 100644 --- a/cmd/workspace/up/detach.go +++ b/cmd/workspace/up/detach.go @@ -45,13 +45,10 @@ func (cmd *UpCmd) runDetached(args []string, cfg *config.Config) error { } if err := t.BeginLaunch(); err != nil { - _ = t.Fail(err) - return fmt.Errorf("prepare detached up: %w", err) + return errors.Join(fmt.Errorf("prepare detached up: %w", err), t.Fail(err)) } if err := launchDetached(t, cmd.detachedInvocationEnvironment()); err != nil { - _ = t.FinishLaunch() - _ = t.Fail(err) - return fmt.Errorf("launch detached up: %w", err) + return errors.Join(fmt.Errorf("launch detached up: %w", err), t.FinishLaunch(), t.Fail(err)) } return cmd.renderDetachedTask(t) } @@ -74,6 +71,9 @@ func (cmd *UpCmd) prepareDetached(cfg *config.Config) error { if err := mergeDevsyUpOptions(&cmd.CLIOptions); err != nil { return err } + if err := workspace2.ValidateID(cmd.ID); err != nil { + return err + } return cmd.preflightLocalValues(cfg) } @@ -149,7 +149,6 @@ func (cmd *UpCmd) openTask() (*task.Task, error) { } t := store.Open(cmd.taskID) if err := t.HoldWorkerLock(); err != nil { - failTask(t, err) return nil, err } deadline := time.NewTimer(5 * time.Second) @@ -159,9 +158,7 @@ func (cmd *UpCmd) openTask() (*task.Task, error) { for { state, err := store.Get(cmd.taskID) if err != nil { - failTask(t, err) - _ = t.ReleaseWorkerLock() - return nil, err + return nil, errors.Join(err, t.ReleaseWorkerLock()) } if state.Status.Terminal() || state.CancelRequested { if err := t.ReleaseWorkerLock(); err != nil { @@ -172,35 +169,24 @@ func (cmd *UpCmd) openTask() (*task.Task, error) { 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 nil, finalizeWorkerTask(t, 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 + return nil, finalizeWorkerTask(t, err) case <-ticker.C: } } } -func failTask(t *task.Task, err error) { - if t == nil { - return - } - _ = t.Fail(err) -} - -func succeedTask(t *task.Task, result *config2.Result) { +func succeedTask(t *task.Task, result *config2.Result) error { if t == nil { - return + return nil } - _ = t.Succeed(result) + return t.Succeed(result) } func (cmd *UpCmd) detachedInvocationEnvironment() []string { diff --git a/cmd/workspace/up/early_task_test.go b/cmd/workspace/up/early_task_test.go new file mode 100644 index 0000000000..bc4b8accf7 --- /dev/null +++ b/cmd/workspace/up/early_task_test.go @@ -0,0 +1,175 @@ +package up + +import ( + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/devsy-org/devsy/cmd/flags" + "github.com/devsy-org/devsy/pkg/clierr" + "github.com/devsy-org/devsy/pkg/config" + "github.com/devsy-org/devsy/pkg/task" + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func isolatedTaskStore(t *testing.T) *task.Store { + t.Helper() + t.Setenv("DEVSY_HOME", t.TempDir()) + config.ResetPathManager() + t.Cleanup(config.ResetPathManager) + store, err := task.NewStore() + require.NoError(t, err) + return store +} + +func TestDetachedRejectsInvalidIDBeforeCreatingTask(t *testing.T) { + for _, id := range []string{"Backend", "backend_name", "backend.api", "has spaces", strings.Repeat("a", 49)} { + t.Run(id, func(t *testing.T) { + store := isolatedTaskStore(t) + cmd := &UpCmd{GlobalFlags: &flags.GlobalFlags{}} + cmd.ID = id + cmd.ResultFormat = "json" + err := cmd.runDetached(nil, &config.Config{}) + assert.ErrorContains(t, err, "workspace name") + tasks, err := store.List() + require.NoError(t, err) + require.Empty(t, tasks) + }) + } +} + +func TestWorkerRecordsValidationFailureBeforeRun(t *testing.T) { + store := isolatedTaskStore(t) + tk, err := store.Create(task.CreateOptions{Command: "up"}) + require.NoError(t, err) + require.NoError(t, tk.SetPID(os.Getpid())) + cmd := &UpCmd{GlobalFlags: &flags.GlobalFlags{}, taskID: tk.ID()} + cmd.WorkspaceMountConsistency = "invalid" + cobraCmd := &cobra.Command{} + cobraCmd.SetContext(context.Background()) + err = cmd.execute(cobraCmd, nil) + require.ErrorContains(t, err, "workspace-mount-consistency") + state, getErr := store.Get(tk.ID()) + require.NoError(t, getErr) + require.Equal(t, task.StatusFailed, state.Status) + require.Contains(t, state.Error, "workspace-mount-consistency") + assertWorkerLockReleased(t, store, tk.ID()) +} + +func TestDuplicateWorkerCannotFailOwnerTask(t *testing.T) { + store := isolatedTaskStore(t) + tk, err := store.Create(task.CreateOptions{Command: "up"}) + require.NoError(t, err) + require.NoError(t, tk.SetPID(os.Getpid())) + require.NoError(t, tk.HoldWorkerLock()) + defer func() { require.NoError(t, tk.ReleaseWorkerLock()) }() + cmd := &UpCmd{taskID: tk.ID()} + _, err = cmd.openTask() + require.ErrorContains(t, err, "already has a running worker") + state, err := store.Get(tk.ID()) + require.NoError(t, err) + require.Equal(t, task.StatusPending, state.Status) +} + +func TestDetachedValidatesMergedID(t *testing.T) { + isolatedTaskStore(t) + t.Setenv(config.EnvFlagsUp, `{"id":"Backend"}`) + cmd := &UpCmd{} + cmd.ID = "backend" + require.ErrorContains(t, cmd.prepareDetached(nil), "lowercase") + require.Equal(t, "Backend", cmd.ID) +} + +func TestWorkerRecordsConfigAndClientErrors(t *testing.T) { + for _, stage := range []string{"config", "client", "provider"} { + t.Run(stage, func(t *testing.T) { + store := isolatedTaskStore(t) + tk, err := store.Create(task.CreateOptions{Command: "up"}) + require.NoError(t, err) + require.NoError(t, tk.SetPID(os.Getpid())) + cmd := &UpCmd{GlobalFlags: &flags.GlobalFlags{}, taskID: tk.ID()} + cmd.ResultFormat = "json" + expected := "prepare workspace client" + switch stage { + case "config": + path, pathErr := config.GetConfigPath() + require.NoError(t, pathErr) + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o700)) + require.NoError(t, os.WriteFile(path, []byte("contexts: ["), 0o600)) + expected = "load devsy config" + case "client": + cmd.ID = "Backend" + case "provider": + cmd.Provider = "missing-provider" + } + cobraCmd := &cobra.Command{} + cobraCmd.SetContext(context.Background()) + err = cmd.execute(cobraCmd, []string{t.TempDir()}) + require.ErrorContains(t, err, expected) + state, getErr := store.Get(tk.ID()) + require.NoError(t, getErr) + require.Equal(t, task.StatusFailed, state.Status) + require.Contains(t, state.Error, expected) + assertWorkerLockReleased(t, store, tk.ID()) + }) + } +} + +func TestFinalizeWorkerPreservesClassifiedRedactedError(t *testing.T) { + store := isolatedTaskStore(t) + testValue := strings.Repeat("test-value-", 8) + t.Setenv("DEVSY_SECRETS_PASSPHRASE", testValue) + tk, err := store.Create(task.CreateOptions{Command: "up"}) + require.NoError(t, err) + require.NoError(t, tk.HoldWorkerLock()) + original := &clierr.CLIError{ + Code: clierr.CodeCanceled, + Message: testValue, + Hint: "retry " + testValue, + Context: map[string]string{"source": testValue}, + } + require.ErrorIs(t, finalizeWorkerTask(tk, original), original) + second := errors.New("second failure") + require.ErrorIs(t, finalizeWorkerTask(tk, second), second) + state, err := store.Get(tk.ID()) + require.NoError(t, err) + require.Equal(t, string(clierr.CodeCanceled), state.ErrorCode) + require.NotContains(t, state.Error, testValue) + require.NotContains(t, state.ErrorHint, testValue) + require.NotContains(t, state.ErrorContext["source"], testValue) + assertWorkerLockReleased(t, store, tk.ID()) +} + +func TestDetachedAllowsOmittedAndMaximumLengthID(t *testing.T) { + for _, id := range []string{"", strings.Repeat("a", 48)} { + t.Run(id, func(t *testing.T) { + isolatedTaskStore(t) + cfg, err := config.LoadConfig("", "") + require.NoError(t, err) + cmd := &UpCmd{GlobalFlags: &flags.GlobalFlags{}} + cmd.ID = id + require.NoError(t, cmd.prepareDetached(cfg)) + require.Equal(t, id, cmd.ID) + }) + } +} + +func TestFinalizeWorkerSurfacesPersistenceFailure(t *testing.T) { + store := isolatedTaskStore(t) + tk, err := store.Create(task.CreateOptions{Command: "up"}) + require.NoError(t, err) + require.NoError(t, tk.HoldWorkerLock()) + dir, err := config.DefaultPathManager().TaskDir() + require.NoError(t, err) + require.NoError(t, os.Remove(filepath.Join(dir, tk.ID()+".json"))) + original := errors.New("original worker failure") + err = finalizeWorkerTask(tk, original) + require.ErrorIs(t, err, original) + require.ErrorContains(t, err, "record detached task failure") + assertWorkerLockReleased(t, store, tk.ID()) +} diff --git a/cmd/workspace/up/up.go b/cmd/workspace/up/up.go index 5192bba095..907d2de5cf 100644 --- a/cmd/workspace/up/up.go +++ b/cmd/workspace/up/up.go @@ -2,6 +2,7 @@ package up import ( "context" + "errors" "fmt" "io" "os" @@ -63,7 +64,8 @@ type UpCmd struct { // See cmd.runDetached. Detach bool // Set only on the detached child re-exec. - taskID string + taskID string + workerTask *task.Task // Out receives result/error JSON envelopes; nil falls back to os.Stdout. Out io.Writer // Set at the start of Run; nil until then. @@ -261,6 +263,25 @@ func (cmd *UpCmd) Run( devsyConfig *config.Config, client client2.BaseWorkspaceClient, args []string, +) (retErr error) { + if cmd.workerTask == nil && cmd.taskID != "" { + t, err := cmd.openTask() + if err != nil { + return err + } + cmd.workerTask = t + defer func() { + retErr = finalizeWorkerTask(t, retErr) + cmd.workerTask = nil + }() + } + return cmd.runWorkspace(ctx, devsyConfig, client) +} + +func (cmd *UpCmd) runWorkspace( + ctx context.Context, + devsyConfig *config.Config, + client client2.BaseWorkspaceClient, ) error { cmd.prepareWorkspace(client) @@ -276,7 +297,6 @@ func (cmd *UpCmd) Run( wctx, err := cmd.executeDevsyUp(ctx, devsyConfig, client) if err != nil { - failTask(t, err) return reportErr(err, emitJSON, out) } if wctx == nil || cmd.Prebuild { @@ -291,11 +311,9 @@ func (cmd *UpCmd) Run( out: out, }) if err != nil { - failTask(t, err) return err } - succeedTask(t, wctx.result) - return nil + return succeedTask(t, wctx.result) } func (cmd *UpCmd) configureRun(workspaceID string) (bool, io.Writer, error) { @@ -324,28 +342,22 @@ func (cmd *UpCmd) finishWithoutWorkspace( if err := status.Run(ctx, cmd.reporter(), status.Operation{ Phase: status.PhaseReady, }, func(context.Context) error { return nil }); err != nil { - failTask(t, err) return reportErr(err, emitJSON, out) } - succeedTask(t, nil) - return nil // Platform mode or prebuild-only run. + return succeedTask(t, nil) } -// setUpTask opens the run's task (if any), tees the status reporter into it, +// setUpTask tees the status reporter into the claimed task (if any), // and records the workspace ID on it. func (cmd *UpCmd) setUpTask( client client2.BaseWorkspaceClient, emitJSON bool, out io.Writer, ) (*task.Task, error) { - t, err := cmd.openTask() - if err != nil { - return nil, reportErr(err, emitJSON, out) - } + t := cmd.workerTask if t != nil { cmd.statusReporter = status.Tee(cmd.statusReporter, t.Reporter()) if err := t.SetWorkspaceID(client.Workspace()); err != nil { - failTask(t, err) return nil, reportErr(err, emitJSON, out) } } @@ -535,7 +547,21 @@ func emitUpResult(wctx *workspaceContext, ideURL string, out io.Writer) { }) } -func (cmd *UpCmd) execute(cobraCmd *cobra.Command, args []string) error { +func (cmd *UpCmd) execute(cobraCmd *cobra.Command, args []string) (retErr error) { + t, err := cmd.openTask() + if err != nil { + return err + } + cmd.workerTask = t + defer func() { + retErr = finalizeWorkerTask(t, retErr) + cmd.workerTask = nil + }() + + return cmd.executeUp(cobraCmd, args) +} + +func (cmd *UpCmd) executeUp(cobraCmd *cobra.Command, args []string) error { if err := cmd.validate(); err != nil { return err } @@ -752,3 +778,18 @@ func watchForForceShutdown(ctx context.Context, done <-chan struct{}) { signal.Stop(signals) } } + +func finalizeWorkerTask(t *task.Task, err error) error { + if t == nil { + return err + } + if err != nil { + if failErr := t.Fail(err); failErr != nil { + err = errors.Join(err, fmt.Errorf("record detached task failure: %w", failErr)) + } + } + if releaseErr := t.ReleaseWorkerLock(); releaseErr != nil { + err = errors.Join(err, fmt.Errorf("release detached worker lock: %w", releaseErr)) + } + return err +} diff --git a/cmd/workspace/up/up_client.go b/cmd/workspace/up/up_client.go index b338cce009..96c5589ab5 100644 --- a/cmd/workspace/up/up_client.go +++ b/cmd/workspace/up/up_client.go @@ -147,6 +147,9 @@ func (cmd *UpCmd) prepareClientEnvironment( if err := mergeDevsyUpOptions(&cmd.CLIOptions); err != nil { return err } + if err := workspace2.ValidateID(cmd.ID); err != nil { + return err + } if cmd.Platform.Enabled { log.Debug("running in platform mode") config.MergeContextOptions(devsyConfig.Current(), os.Environ()) diff --git a/desktop/e2e/workspace-id.e2e.ts b/desktop/e2e/workspace-id.e2e.ts new file mode 100644 index 0000000000..0b7b9bec9d --- /dev/null +++ b/desktop/e2e/workspace-id.e2e.ts @@ -0,0 +1,82 @@ +import type { ElectronApplication } from "@playwright/test" +import { expect, test } from "@playwright/test" +import { launchApp, resetMockState } from "./electron-app.js" + +for (const [kind, source] of [ + ["git", "https://example.com/Backend.git"], + ["local", "/var/home/dev/Projects/Backend"], +]) { + test(`validates explicit IDs and preserves the ${kind} source`, async () => { + resetMockState() + let app: ElectronApplication | undefined + try { + const launched = await launchApp() + app = launched.app + const page = launched.page + await page.click('[data-sidebar="sidebar"] a[href="#/workspaces"]') + await page + .getByRole("button", { name: "Create Workspace", exact: true }) + .click() + const dialog = page.getByRole("dialog") + await dialog + .getByRole("button", { name: "Continue", exact: true }) + .click() + if (kind === "local") + await dialog.getByText("Local Directory", { exact: true }).click() + await dialog + .getByLabel( + kind === "local" ? "Local Directory *" : "Repository URL *", + { exact: true }, + ) + .fill(source) + await dialog + .getByRole("button", { name: "Continue", exact: true }) + .click() + await dialog + .getByRole("button", { name: "Continue", exact: true }) + .click() + const name = dialog.getByLabel("Workspace Name", { exact: true }) + await expect(name).toHaveValue("backend") + await expect(dialog.getByText(source, { exact: true })).toBeVisible() + await name.fill("Backend") + await expect(name).toHaveValue("Backend") + await expect(name).toHaveAttribute("aria-invalid", "true") + await expect(name).toHaveAttribute( + "aria-describedby", + "workspace-name-invalid", + ) + await expect(dialog.locator("#workspace-name-invalid")).toContainText( + 'Try "backend"', + ) + const launch = dialog.getByRole("button", { name: "Launch", exact: true }) + await expect(launch).toBeDisabled() + await name.fill("backend") + await expect(launch).toBeEnabled() + await launch.click() + await expect + .poll(async () => + page.evaluate(async () => { + const snapshot = (await window.electronAPI.invoke( + "workspace_snapshot", + )) as { + workspaces: Array<{ + id: string + source?: { gitRepository?: string } + }> + jobs: Record + } + const workspace = snapshot.workspaces.find( + (entry) => entry.id === "backend", + ) + return { + state: snapshot.jobs.backend?.state, + source: workspace?.source?.gitRepository, + } + }), + ) + .toEqual({ state: "succeeded", source }) + } finally { + await app?.close() + } + }) +} diff --git a/desktop/src/main/__tests__/ipc-workspace-jobs.test.ts b/desktop/src/main/__tests__/ipc-workspace-jobs.test.ts index d4931d6f04..f65475f192 100644 --- a/desktop/src/main/__tests__/ipc-workspace-jobs.test.ts +++ b/desktop/src/main/__tests__/ipc-workspace-jobs.test.ts @@ -33,7 +33,7 @@ function setup() { let line!: (line: string, stream: "stdout") => void const cli = { setUnlockHandler: vi.fn(), - run: vi.fn(async () => ({ id: "task-1" })), + run: vi.fn(async (_args: string[]) => ({ id: "task-1" })), runRaw: vi.fn( async () => '{"kind":"status","schemaVersion":1,"phase":"building_image","state":"started"}', @@ -54,9 +54,13 @@ function setup() { url: pathToFileURL(join(__dirname, "../../renderer/index.html")).href, isDestroyed: () => false, } - const webContents = { send, mainFrame, isDestroyed: () => false } + const webContents = { send, mainFrame, isDestroyed: vi.fn(() => false) } const win = { webContents, isDestroyed: () => false } trustedEvent = { sender: webContents, senderFrame: mainFrame } + const createLogFile = vi.fn(() => "/tmp/log") + const closeLog = vi.fn(async () => {}) + const appendLog = vi.fn(() => true) + const getMainWindow = vi.fn(() => win) const actions = registerIpcHandlers({ cli, state: { @@ -65,19 +69,24 @@ function setup() { workspaceList: () => [], }, logStore: { - createLogFile: () => "/tmp/log", - appendLog: () => true, - closeLog: async () => {}, + createLogFile, + appendLog, + closeLog, onDrain: async () => {}, }, pty: { cancelFor: vi.fn(async () => {}) }, - getMainWindow: () => win, + getMainWindow, providerJobs: new ProviderJobs(), workspaceJobs: jobs, } as unknown as Parameters[0]) return { jobs, cli, + closeLog, + appendLog, + createLogFile, + getMainWindow, + webContents, send, actions, exit: (code: number) => exit(code), @@ -181,6 +190,270 @@ describe("workspace lifecycle IPC", () => { await flush() expect(ctx.jobs.get("ws")?.state).toBe("succeeded") }) + it("finishes progress after an unexpected launch exception and permits retry", async () => { + const ctx = setup() + vi.spyOn(ctx.actions.tunnelProcesses, "set").mockImplementationOnce(() => { + throw new Error("unexpected follower registration error") + }) + invoke("workspace_up", { source: "ws", commandId: "first" }) + await flush() + expect(ctx.jobs.get("ws")).toMatchObject({ state: "failed" }) + const terminal = ctx.send.mock.calls.filter( + ([channel, value]) => channel === "command-progress" && value.done, + ) + expect(terminal).toHaveLength(1) + expect(terminal[0][1]).toMatchObject({ commandId: "first", success: false }) + expect(ctx.closeLog).toHaveBeenCalledOnce() + invoke("workspace_up", { source: "ws", commandId: "retry" }) + await flush() + expect(ctx.jobs.get("ws")).toMatchObject({ + commandId: "retry", + state: "running", + }) + expect(ctx.cli.run).toHaveBeenCalledWith([ + "workspace", + "task", + "cancel", + "task-1", + ]) + }) + + it("finishes progress if creating the log fails before submission", async () => { + const ctx = setup() + ctx.createLogFile.mockImplementationOnce(() => { + throw new Error("log directory unavailable") + }) + invoke("workspace_up", { source: "ws", commandId: "up" }) + await flush() + expect(ctx.cli.run).not.toHaveBeenCalled() + expect(ctx.jobs.get("ws")).toMatchObject({ + state: "failed", + error: "log directory unavailable", + }) + const terminal = ctx.send.mock.calls.filter( + ([channel, value]) => channel === "command-progress" && value.done, + ) + expect(terminal).toHaveLength(1) + expect(terminal[0][1]).toMatchObject({ + success: false, + cliError: { message: "log directory unavailable" }, + }) + }) + + it("preserves preflight error and hint in one terminal event", async () => { + const ctx = setup() + ctx.cli.run.mockRejectedValueOnce( + Object.assign(new Error("invalid ID"), { + cliError: { + code: "invalid_id", + message: "invalid ID", + hint: "Use lowercase", + }, + }), + ) + invoke("workspace_up", { source: "ws", commandId: "up" }) + await flush() + const terminal = ctx.send.mock.calls.filter( + ([channel, value]) => channel === "command-progress" && value.done, + ) + expect(terminal).toHaveLength(1) + expect(terminal[0][1]).toMatchObject({ + success: false, + cliError: { + code: "invalid_id", + message: "invalid ID", + hint: "Use lowercase", + }, + }) + expect(ctx.jobs.get("ws")).toMatchObject({ + state: "failed", + error: "invalid ID", + }) + expect(ctx.closeLog).toHaveBeenCalledOnce() + }) + + it("reports a persisted failed task after its follower disconnects", async () => { + const ctx = setup() + invoke("workspace_up", { source: "ws", commandId: "up" }) + await flush() + ctx.cli.runRaw.mockRejectedValue(new Error("task failed")) + ctx.cli.run.mockResolvedValue([ + { + id: "task-1", + status: "failed", + error: "invalid ID", + errorCode: "invalid_id", + }, + ] as never) + ctx.exit(1) + await flush() + expect(ctx.jobs.get("ws")).toMatchObject({ + state: "failed", + error: "invalid ID", + }) + const terminal = ctx.send.mock.calls.filter( + ([channel, value]) => channel === "command-progress" && value.done, + ) + expect(terminal).toHaveLength(1) + expect(terminal[0][1]).toMatchObject({ + success: false, + cliError: { message: "invalid ID" }, + }) + expect(ctx.closeLog).toHaveBeenCalledOnce() + ctx.cli.run.mockResolvedValue({ id: "task-2" }) + invoke("workspace_up", { source: "ws", commandId: "retry" }) + await flush() + expect(ctx.jobs.get("ws")).toMatchObject({ + commandId: "retry", + state: "running", + }) + expect(ctx.cli.run.mock.calls.at(-1)?.[0]).not.toContain("cancel") + }) + + it("ignores a stale persisted result after a new generation owns the workspace", async () => { + const ctx = setup() + invoke("workspace_up", { source: "ws", commandId: "old" }) + await flush() + let resolve!: (value: string) => void + ctx.cli.runRaw.mockImplementationOnce( + () => + new Promise((done) => { + resolve = done + }), + ) + ctx.exit(1) + await flush() + ctx.jobs.clear("ws") + ctx.jobs.start("ws", "creating", "new") + resolve('{"kind":"result","outcome":"success"}') + await flush() + expect(ctx.jobs.get("ws")).toMatchObject({ + commandId: "new", + state: "running", + }) + expect( + ctx.send.mock.calls.filter( + ([channel, value]) => channel === "command-progress" && value.done, + ), + ).toHaveLength(0) + }) + + it.each(["success", "failure"])( + "keeps %s visible and unconfirmed when log close fails", + async (outcome) => { + const ctx = setup() + ctx.closeLog.mockRejectedValueOnce(new Error("disk unavailable")) + invoke("workspace_up", { source: "ws", commandId: "up" }) + await flush() + ctx.line( + outcome === "success" + ? '{"kind":"result","outcome":"success"}' + : '{"kind":"error","outcome":"error","code":"invalid_id","message":"invalid ID","hint":"Use lowercase"}', + ) + await flush() + expect(ctx.jobs.get("ws")).toMatchObject({ + state: outcome === "success" ? "reconciling" : "failed", + refreshError: "Workspace log could not be saved: disk unavailable", + ...(outcome === "failure" ? { error: "invalid ID" } : {}), + }) + const terminal = ctx.send.mock.calls.filter( + ([channel, value]) => channel === "command-progress" && value.done, + ) + expect(terminal).toHaveLength(1) + expect(terminal[0][1].success).toBe(outcome === "success") + if (outcome === "failure") + expect(terminal[0][1].cliError).toMatchObject({ + message: "invalid ID", + hint: "Use lowercase", + }) + await ctx.jobs.retryRefresh("ws") + expect(ctx.jobs.get("ws")?.state).toBe( + outcome === "success" ? "succeeded" : "failed", + ) + }, + ) + + it.each(["success", "failure"])( + "preserves terminal %s when renderer delivery throws", + async (outcome) => { + const ctx = setup() + vi.stubEnv("DEVSY_TEST_TOKEN", "private-value") + const unhandled: unknown[] = [] + const onUnhandled = (error: unknown) => unhandled.push(error) + process.on("unhandledRejection", onUnhandled) + try { + invoke("workspace_up", { source: "ws", commandId: "up" }) + await flush() + ctx.send.mockImplementation((channel, progress) => { + if (channel === "command-progress" && progress.done) + throw new Error("renderer delivery failed") + }) + ctx.line( + outcome === "success" + ? '{"kind":"result","outcome":"success"}' + : '{"kind":"error","outcome":"error","code":"invalid_id","message":"invalid ID token=private-value","hint":"Use lowercase"}', + ) + await flush() + await vi.advanceTimersByTimeAsync(0) + expect(unhandled).toEqual([]) + expect(ctx.jobs.get("ws")).toMatchObject({ + state: outcome === "success" ? "succeeded" : "failed", + ...(outcome === "failure" ? { error: "invalid ID token=***" } : {}), + }) + expect(ctx.closeLog).toHaveBeenCalledOnce() + if (outcome === "failure") { + const terminal = ctx.send.mock.calls.find( + ([channel, progress]) => + channel === "command-progress" && progress.done, + ) + expect(terminal?.[1].cliError).toMatchObject({ + message: "invalid ID token=***", + hint: "Use lowercase", + }) + } + invoke("workspace_up", { source: "ws", commandId: "retry" }) + await flush() + expect(ctx.jobs.get("ws")).toMatchObject({ + commandId: "retry", + state: "running", + }) + expect(ctx.cli.run).not.toHaveBeenCalledWith([ + "workspace", + "task", + "cancel", + "task-1", + ]) + } finally { + process.removeListener("unhandledRejection", onUnhandled) + vi.unstubAllEnvs() + } + }, + ) + + it.each(["destroyed contents", "window lookup throws"])( + "finishes the main job when %s prevents progress delivery", + async (boundary) => { + const ctx = setup() + invoke("workspace_up", { source: "ws", commandId: "up" }) + await flush() + if (boundary === "destroyed contents") + ctx.webContents.isDestroyed.mockReturnValue(true) + else + ctx.getMainWindow.mockImplementation(() => { + throw new Error("window unavailable") + }) + ctx.line('{"kind":"result","outcome":"success"}') + await flush() + expect(ctx.jobs.get("ws")?.state).toBe("succeeded") + expect( + ctx.send.mock.calls.filter( + ([channel]) => channel === "command-progress", + ), + ).toHaveLength(0) + expect(ctx.closeLog).toHaveBeenCalledOnce() + }, + ) + it("uses the same registry for tray stop", async () => { const ctx = setup() const completion = ctx.actions.workspaceActions.stop("ws") diff --git a/desktop/src/main/ipc.ts b/desktop/src/main/ipc.ts index ed6c5d3cd4..bcd5fb8891 100644 --- a/desktop/src/main/ipc.ts +++ b/desktop/src/main/ipc.ts @@ -192,7 +192,7 @@ interface ProgressSink { cliError?: CLIError success?: boolean }, - ): Promise + ): Promise } function redactSensitiveText(value: string): string { @@ -293,24 +293,28 @@ function createLogSink( if (!done && buf.length === 0) return const lines = buf buf = [] - const win = getWin() - if (!win || win.isDestroyed?.()) return - win.webContents.send("command-progress", { - commandId, - lines: lines.map(redactSensitiveText), - done, - ...(extra - ? { - ...extra, - message: extra.message - ? redactSensitiveText(extra.message) - : undefined, - cliError: extra.cliError - ? redactCLIError(extra.cliError) - : undefined, - } - : {}), - }) + try { + const win = getWin() + if (!win || win.isDestroyed?.() || win.webContents.isDestroyed?.()) return + win.webContents.send("command-progress", { + commandId, + lines: lines.map(redactSensitiveText), + done, + ...(extra + ? { + ...extra, + message: extra.message + ? redactSensitiveText(extra.message) + : undefined, + cliError: extra.cliError + ? redactCLIError(extra.cliError) + : undefined, + } + : {}), + }) + } catch { + mainLog.warn("[workspace-operation] progress delivery unavailable") + } } return { @@ -328,14 +332,21 @@ function createLogSink( finished = true const safeLine = redactSensitiveText(finalLine) buf.push(safeLine) + let flushError: string | undefined try { - appendLog?.(safeLine) - await flush?.() + try { + appendLog?.(safeLine) + } finally { + await flush?.() + } } catch (error) { - // Log persistence must not change a command's actual outcome. + flushError = redactSensitiveText( + `Workspace log could not be saved: ${error instanceof Error ? error.message : String(error)}`, + ) mainLog.warn("[workspace-operation] log flush failed", error) } post(true, { message: finalLine, ...extra }) + return flushError }, } } @@ -2006,6 +2017,7 @@ export function registerIpcHandlers(deps: IpcDependencies): { `[workspace-operation] ${cmdId} ${stage} ${Math.round(performance.now() - started)}ms`, ) timing("accepted") + let failureSink = createLogSink(deps.getMainWindow, cmdId) const completion = (async () => { const logPath = logStore.createLogFile(state.workspaceContext(wsId), wsId) const sink = createLogSink( @@ -2015,6 +2027,16 @@ export function registerIpcHandlers(deps: IpcDependencies): { () => logStore.closeLog(logPath), ) + failureSink = sink + const finishUp = async ( + line: string, + extra: Parameters[1], + error?: string, + ) => { + const flushError = await sink.done(line, extra) + await workspaceJobs.finish(wsId, generation, error, flushError) + } + await serializePerWorkspace(wsId, async () => { // Tear down any prior run for this workspace before starting a new // one. @@ -2035,21 +2057,15 @@ export function registerIpcHandlers(deps: IpcDependencies): { } taskId = submitted.id } catch (error) { - const err = error as Error & { cliError?: CLIError } - void sink.done(formatLogLine(err.message, "ERROR"), { - level: "error", - success: false, - cliError: err.cliError ?? { - code: "up_failed", - message: err.message, + const safe = redactCLIError(cliErrorOrFallback(error, "up_failed")) + await finishUp( + formatLogLine(safe.message, "ERROR"), + { + level: "error", + success: false, + cliError: safe, }, - }) - await workspaceJobs.finish( - wsId, - generation, - redactCLIError( - err.cliError ?? { code: "up_failed", message: err.message }, - ).message, + safe.message, ) return cmdId } @@ -2105,32 +2121,28 @@ export function registerIpcHandlers(deps: IpcDependencies): { signalledDone = true releaseTask() timing("completed") - void workspaceJobs.finish(wsId, generation) - void sink.done(formatted, { success: true }) + void finishUp(formatted, { success: true }) return } if (envelope?.kind === "error") { signalledDone = true releaseTask() - void workspaceJobs.finish( - wsId, - generation, - redactCLIError({ - code: envelope.code ?? "up_failed", - message: envelope.message, - }).message, - ) - void sink.done(formatted, { - level: "error", - success: false, - cliError: { - code: envelope.code ?? "up_failed", - message: envelope.message, - hint: envelope.hint, - context: envelope.context, - }, + const safe = redactCLIError({ + code: envelope.code ?? "up_failed", + message: envelope.message, + hint: envelope.hint, + context: envelope.context, }) + void finishUp( + formatted, + { + level: "error", + success: false, + cliError: safe, + }, + safe.message, + ) return } @@ -2183,11 +2195,19 @@ export function registerIpcHandlers(deps: IpcDependencies): { return cmdId }) })().catch(async (error) => { - await workspaceJobs.finish( - wsId, - generation, - redactCLIError(cliErrorOrFallback(error, "up_failed")).message, - ) + const safe = redactCLIError(cliErrorOrFallback(error, "up_failed")) + let flushError: string | undefined + if (workspaceJobs.owns(wsId, cmdId)) { + flushError = await failureSink.done( + formatLogLine(safe.message, "ERROR"), + { + success: false, + level: "error", + cliError: safe, + }, + ) + } + await workspaceJobs.finish(wsId, generation, safe.message, flushError) }) return { commandId: cmdId, completion } } @@ -2226,12 +2246,10 @@ export function registerIpcHandlers(deps: IpcDependencies): { const envelope = parseCliEnvelope(line) if (envelope?.kind === "result") { release() - await sink - .done(formatLogLine("Completed"), { success: true }) - .catch((error) => - mainLog.warn("[workspace-operation] log flush failed", error), - ) - await workspaceJobs.finish(wsId, generation) + const flushError = await sink.done(formatLogLine("Completed"), { + success: true, + }) + await workspaceJobs.finish(wsId, generation, undefined, flushError) return } if (envelope?.kind === "status") @@ -2263,11 +2281,19 @@ export function registerIpcHandlers(deps: IpcDependencies): { code: task.errorCode ?? "up_failed", message: task.error || "Workspace task failed", }) - await sink.done(formatLogLine(safe.message, "ERROR"), { - success: false, - cliError: safe, - }) - await workspaceJobs.finish(wsId, generation, safe.message) + const flushError = await sink.done( + formatLogLine(safe.message, "ERROR"), + { + success: false, + cliError: safe, + }, + ) + await workspaceJobs.finish( + wsId, + generation, + safe.message, + flushError, + ) return } } catch { diff --git a/desktop/src/main/workspace-jobs.ts b/desktop/src/main/workspace-jobs.ts index 3260cc6335..fc53f45e6b 100644 --- a/desktop/src/main/workspace-jobs.ts +++ b/desktop/src/main/workspace-jobs.ts @@ -79,20 +79,26 @@ export class WorkspaceJobs { this.generations.set(id, ++this.lastGeneration) if (this.jobs.delete(id)) this.emit() } - async finish(id: string, generation: number, error?: string): Promise { + async finish( + id: string, + generation: number, + error?: string, + confirmationError?: string, + ): Promise { const job = this.jobs.get(id) if (!job || this.generation(id) !== generation || job.state !== "running") return this.generations.set(id, ++this.lastGeneration) // invalidate pre-completion polls this.jobs.set(id, { ...job, - state: "reconciling", + state: error && confirmationError ? "failed" : "reconciling", + refreshError: confirmationError, phase: job.activity === "deleting" ? "Refreshing list" : "Refreshing status", error, }) this.emit() - await this.retryRefresh(id) + if (!confirmationError) await this.retryRefresh(id) } async retryRefresh(id: string): Promise { const job = this.jobs.get(id) diff --git a/desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.svelte b/desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.svelte index 5c1bac7e6a..57ac8f0704 100644 --- a/desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.svelte +++ b/desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.svelte @@ -35,11 +35,7 @@ import { reduceWorkspaceLaunchTimeline, } from "$lib/utils/workspace-launch-timeline.js" import type { LaunchConfirmationState } from "$lib/utils/workspace-launch-presentation.js" -import { - uniqueNamesGenerator, - adjectives, - animals, -} from "unique-names-generator" + import { workspaceUp, workspaceRefresh, @@ -72,6 +68,13 @@ import { } from "$lib/utils/log-parser.js" import type { UnlistenFn } from "$lib/ipc/types.js" +import { + deriveWorkspaceId, + isValidWorkspaceId, + uniqueWorkspaceId, + workspaceIdRule, +} from "$lib/utils/workspace-id.js" + type Step = "provider" | "source" | "ide" | "review" | "launch" type StepState = "pending" | "active" | "complete" | "error" @@ -380,15 +383,10 @@ let filteredIdes = $derived( ) let resolvedId = $derived( - workspaceName.trim() || deriveWorkspaceNameFromSource(assembled.source), + workspaceName || deriveWorkspaceNameFromSource(assembled.source), ) -let resolvedIdInvalid = $derived( - resolvedId === "" || - resolvedId === "." || - resolvedId.includes(":") || - !/^[A-Za-z0-9._-]+$/.test(resolvedId), -) +let resolvedIdInvalid = $derived(!isValidWorkspaceId(resolvedId)) let nameConflict = $derived( resolvedId !== "" && @@ -758,6 +756,7 @@ async function handleLaunch(recovery = false) { } }) + if (!isValidWorkspaceId(workspaceId)) throw new Error(workspaceIdRule) const cmdId = await workspaceUp({ source: assembled.source, workspaceId, @@ -846,32 +845,15 @@ function goToStep(step: Step) { currentStep = step } -function randomName(): string { - return uniqueNamesGenerator({ - dictionaries: [adjectives, animals], - separator: "-", - length: 2, - style: "lowerCase", - }) -} - function deriveWorkspaceNameFromSource(source: string): string { - const leaf = source.trim().split(/[\\/]/).pop() ?? "" - return leaf - .replace(/\.git$/i, "") - .replace(/@.*$/, "") - .replace(/[^A-Za-z0-9._-]+/g, "-") - .replace(/^-+|-+$/g, "") + return deriveWorkspaceId(source) } function uniquifyName(base: string): string { - const existing = new Set($workspaces.map((ws) => ws.id.toLowerCase())) - if (base && !existing.has(base.toLowerCase())) return base - for (let i = 0; i < 50; i++) { - const candidate = randomName() - if (!existing.has(candidate.toLowerCase())) return candidate - } - return `${randomName()}-${Date.now().toString(36)}` + return uniqueWorkspaceId( + base, + $workspaces.map((ws) => ws.id), + ) } function continueFromProvider() { @@ -884,8 +866,6 @@ function continueFromSource() { if (!workspaceName.trim()) { const derived = deriveWorkspaceNameFromSource(assembled.source) if (derived) workspaceName = uniquifyName(derived) - } else { - workspaceName = uniquifyName(workspaceName.trim()) } currentStep = "ide" } @@ -912,12 +892,7 @@ function continueFromReview() { function selectTemplate(t: { name: string; source: string }) { repoUrl = t.source if (!workspaceName) { - workspaceName = uniquifyName( - t.name - .toLowerCase() - .replace(/[^a-z0-9]+/g, "-") - .replace(/^-+|-+$/g, ""), - ) + workspaceName = uniquifyName(deriveWorkspaceId(t.name)) } } @@ -1326,7 +1301,7 @@ function selectTemplate(t: { name: string; source: string }) { - Please provide an explicit workspace name. The derived id is empty or invalid. + {workspaceIdRule} Try "{deriveWorkspaceId(resolvedId)}". {/if} diff --git a/desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.test.ts b/desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.test.ts index 8b61475962..9528ebddd1 100644 --- a/desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.test.ts +++ b/desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.test.ts @@ -382,6 +382,41 @@ describe("WorkspaceWizard", () => { unmount() }) + it.each([ + ["Local Directory", "/var/home/dev/Projects/Backend", "Local Directory *"], + ["Git Repository", "https://example.com/Backend.git", "Repository URL *"], + ])( + "derives a valid ID from %s without changing the source", + async (tab, source, label) => { + providers.set([makeProvider("docker")]) + const { getByText, getByLabelText, unmount } = render(WorkspaceWizard, { + props: { open: true }, + }) + await flushAsync() + await fireEvent.click(getByText("docker")) + await fireEvent.click(getActiveContinue(getByText)) + await flushAsync() + if (tab === "Local Directory") + await fireEvent.click(getByText("Local Directory")) + await fireEvent.input(getByLabelText(label), { + target: { value: source }, + }) + await fireEvent.click(getActiveContinue(getByText)) + await flushAsync() + await fireEvent.click(getActiveContinue(getByText)) + await flushAsync() + expect( + (document.querySelector("#workspace-name") as HTMLInputElement).value, + ).toBe("backend") + await fireEvent.click(getLaunchButton()) + await flushAsync() + expect(workspaceUp).toHaveBeenCalledWith( + expect.objectContaining({ workspaceId: "backend", source }), + ) + unmount() + }, + ) + it("shows the image picker when the Image tab is selected", async () => { providers.set([makeProvider("docker")]) const { getByText, unmount } = render(WorkspaceWizard, { @@ -432,6 +467,32 @@ describe("WorkspaceWizard", () => { unmount() }) + it.each(["Backend", "back_end", "back.end", "back end", "a".repeat(49)])( + "blocks invalid explicit ID %s with an accessible rule", + async (id) => { + providers.set([makeProvider("docker")]) + const { getByText, unmount } = render(WorkspaceWizard, { + props: { open: true }, + }) + await flushAsync() + await advanceToReview(getByText) + const input = document.querySelector( + "#workspace-name", + ) as HTMLInputElement + await fireEvent.input(input, { target: { value: id } }) + await flushAsync() + expect(input.value).toBe(id) + expect(input.getAttribute("aria-invalid")).toBe("true") + expect(input.getAttribute("aria-describedby")).toBe( + "workspace-name-invalid", + ) + expect(getByText(/1-48 lowercase letters/)).toBeTruthy() + expect(getLaunchButton().disabled).toBe(true) + expect(workspaceUp).not.toHaveBeenCalled() + unmount() + }, + ) + it("blocks Launch when the user edits the name into a conflict", async () => { providers.set([makeProvider("docker")]) workspaces.set([{ id: "python" }]) @@ -1035,7 +1096,7 @@ describe("WorkspaceWizard", () => { const nameInput = document.querySelector( 'input[placeholder*="derived from source"]', ) as HTMLInputElement - expect(nameInput.value).toBe("ubuntu-22.04") + expect(nameInput.value).toBe("ubuntu-22-04") const launchBtn = Array.from(document.querySelectorAll("button")).find( (b) => b.textContent?.trim() === "Launch", diff --git a/desktop/src/renderer/src/lib/utils/workspace-id.test.ts b/desktop/src/renderer/src/lib/utils/workspace-id.test.ts new file mode 100644 index 0000000000..b4328459bb --- /dev/null +++ b/desktop/src/renderer/src/lib/utils/workspace-id.test.ts @@ -0,0 +1,40 @@ +import { describe, expect, it } from "vitest" +import { + deriveWorkspaceId, + isValidWorkspaceId, + uniqueWorkspaceId, +} from "./workspace-id.js" + +describe("workspace IDs", () => { + it.each(["backend", "backend-name-1", "a".repeat(48)])("accepts %s", (id) => { + expect(isValidWorkspaceId(id)).toBe(true) + }) + it.each(["", "Backend", "back_end", "back.end", "back end", "a".repeat(49)])( + "rejects %s", + (id) => { + expect(isValidWorkspaceId(id)).toBe(false) + }, + ) + it.each([ + "/var/home/dev/Projects/Backend", + "https://example.com/Backend.git", + "https://example.com/Backend.git@feature/foo", + "git@host:org/Backend.git", + "github.com/org/Backend@main@subpath:sub/path", + "github.com/org/Backend@main:sub/path", + ])("derives backend from %s without changing the source", (source) => { + const original = source + expect(deriveWorkspaceId(source)).toBe("backend") + expect(source).toBe(original) + }) + it("normalizes, truncates and falls back to valid IDs", () => { + for (const source of ["a".repeat(80), "___", "App.Name With Spaces"]) + expect(isValidWorkspaceId(deriveWorkspaceId(source))).toBe(true) + }) + it("preserves case insensitive collisions within the length limit", () => { + const base = "a".repeat(48) + const id = uniqueWorkspaceId(base, [base.toUpperCase()]) + expect(id).toBe(`${"a".repeat(46)}-1`) + expect(isValidWorkspaceId(id)).toBe(true) + }) +}) diff --git a/desktop/src/renderer/src/lib/utils/workspace-id.ts b/desktop/src/renderer/src/lib/utils/workspace-id.ts new file mode 100644 index 0000000000..fc818b3f45 --- /dev/null +++ b/desktop/src/renderer/src/lib/utils/workspace-id.ts @@ -0,0 +1,35 @@ +export const workspaceIdRule = + "Use 1-48 lowercase letters (a-z), numbers, or dashes." + +export function isValidWorkspaceId(id: string): boolean { + return /^[a-z0-9-]{1,48}$/.test(id) +} + +export function deriveWorkspaceId(source: string): string { + const repository = source + .trim() + .replace(/^[^/@]+@[^:]+:/, "") + .replace(/^[a-z][a-z0-9+.-]*:\/\/[^/]*\//i, "") + .split("@")[0] + const leaf = repository.split(/[\\/]/).pop() ?? "" + return ( + leaf + .replace(/@.*$/, "") + .replace(/\.git$/i, "") + .toLowerCase() + .replace(/[^a-z0-9-]+/g, "-") + .replace(/^-+|-+$/g, "") + .slice(0, 48) + .replace(/-+$/g, "") || "workspace" + ) +} + +export function uniqueWorkspaceId(base: string, ids: string[]): string { + const existing = new Set(ids.map((id) => id.toLowerCase())) + if (!existing.has(base.toLowerCase())) return base + for (let i = 1; ; i++) { + const suffix = `-${i}` + const candidate = `${base.slice(0, 48 - suffix.length)}${suffix}` + if (!existing.has(candidate.toLowerCase())) return candidate + } +} diff --git a/e2e/tests/ssh/detached_validation.go b/e2e/tests/ssh/detached_validation.go new file mode 100644 index 0000000000..aa9dc8a85d --- /dev/null +++ b/e2e/tests/ssh/detached_validation.go @@ -0,0 +1,227 @@ +package ssh + +import ( + "context" + "encoding/json" + "os" + "os/exec" + "path/filepath" + "strings" + "time" + + "github.com/devsy-org/devsy/e2e/framework" + "github.com/devsy-org/devsy/pkg/task" + "github.com/devsy-org/devsy/pkg/workspace" + "github.com/onsi/ginkgo/v2" + "github.com/onsi/gomega" +) + +var _ = ginkgo.Describe("detached workspace input validation", ginkgo.Label("task-cancel"), func() { + ginkgo.It( + "rejects invalid explicit IDs without submitting tasks", + ginkgo.SpecTimeout(time.Minute), + func(ctx context.Context) { + wd, err := os.Getwd() + framework.ExpectNoError(err) + f := framework.NewDefaultFramework(filepath.Join(wd, "bin")) + home, err := os.MkdirTemp("", "devsy-invalid-id-") + framework.ExpectNoError(err) + ginkgo.DeferCleanup(os.RemoveAll, home) + cli := detachedValidationCLI{framework: f, home: home} + for _, id := range []string{"Backend", "backend_name", "backend.api", "has spaces", strings.Repeat("a", 49)} { + out, err := cli.command(ctx, + cmdWorkspace, + "up", + "--detach", + "--result-format=json", + "--id", + id, + ) + gomega.Expect(err).To(gomega.HaveOccurred()) + gomega.Expect(out).To(gomega.ContainSubstring("workspace name")) + gomega.Expect(out).NotTo(gomega.ContainSubstring(`"kind":"task"`)) + } + out, err := cli.command(ctx, cmdWorkspace, "task", "list") + framework.ExpectNoError(err) + var states []detachedUpState + framework.ExpectNoError(json.Unmarshal([]byte(out), &states)) + gomega.Expect(states).To(gomega.BeEmpty()) + }, + ) + + ginkgo.It( + "records early provider failure and preserves a mixed-case local source", + ginkgo.SpecTimeout(time.Minute), + func(ctx context.Context) { + wd, err := os.Getwd() + framework.ExpectNoError(err) + f := framework.NewDefaultFramework(filepath.Join(wd, "bin")) + home, err := os.MkdirTemp("", "devsy-early-worker-") + framework.ExpectNoError(err) + ginkgo.DeferCleanup(os.RemoveAll, home) + cli := detachedValidationCLI{framework: f, home: home} + source := filepath.Join(home, "Backend.API") + framework.ExpectNoError(os.MkdirAll(source, 0o700)) + sentinel := filepath.Join(source, "KeepCase.txt") + framework.ExpectNoError(os.WriteFile(sentinel, []byte("original source"), 0o600)) + out, err := cli.command(ctx, + cmdWorkspace, + "up", + "--detach", + "--result-format=json", + "--provider=missing-provider-1471", + source, + ) + framework.ExpectNoError(err) + var envelope struct { + Kind string `json:"kind"` + ID string `json:"id"` + } + framework.ExpectNoError(json.Unmarshal([]byte(strings.TrimSpace(out)), &envelope)) + gomega.Expect(envelope.Kind).To(gomega.Equal("task")) + var states []struct { + ID string `json:"id"` + Status string `json:"status"` + WorkspaceID string `json:"workspaceId"` + Error string `json:"error"` + } + gomega.Eventually(func() string { + out, err := cli.command(ctx, cmdWorkspace, "task", "list") + if err != nil { + return err.Error() + } + if err := json.Unmarshal([]byte(out), &states); err != nil { + return err.Error() + } + for _, state := range states { + if state.ID == envelope.ID { + return state.Status + } + } + return "missing" + }).WithContext(ctx).WithTimeout(20 * time.Second).WithPolling(100 * time.Millisecond).Should(gomega.Equal("failed")) + gomega.Expect(states).To(gomega.HaveLen(1)) + gomega.Expect(states[0].WorkspaceID).To(gomega.Equal(workspace.ToID(source))) + gomega.Expect(states[0].Error).To(gomega.ContainSubstring("prepare workspace client")) + gomega.Expect(states[0].Error).NotTo(gomega.ContainSubstring("missing-provider-1471")) + cli.expectFailureEnvelope(ctx, envelope.ID, "prepare workspace client") + sourceRoot, err := os.OpenRoot(source) + framework.ExpectNoError(err) + ginkgo.DeferCleanup(sourceRoot.Close) + contents, err := sourceRoot.ReadFile("KeepCase.txt") + framework.ExpectNoError(err) + gomega.Expect(string(contents)).To(gomega.Equal("original source")) + entries, err := os.ReadDir(home) + framework.ExpectNoError(err) + var entryNames []string + for _, entry := range entries { + entryNames = append(entryNames, entry.Name()) + } + gomega.Expect(entryNames).To(gomega.ContainElement("Backend.API")) + gomega.Expect(entryNames).NotTo(gomega.ContainElement("backend.api")) + }, + ) +}) + +type detachedValidationCLI struct { + framework *framework.Framework + home string +} + +func (cli detachedValidationCLI) newCommand(ctx context.Context, args ...string) *exec.Cmd { + // #nosec G204 -- repository-built CLI fixture with spec-controlled arguments and temporary paths. + cmd := exec.CommandContext( + ctx, + filepath.Join(cli.framework.DevsyBinDir, cli.framework.DevsyBinName), + args...) + cmd.Env = detachedValidationEnvironment(cli.home) + cmd.WaitDelay = time.Second + return cmd +} + +func (cli detachedValidationCLI) command(ctx context.Context, args ...string) (string, error) { + out, err := cli.newCommand(ctx, args...).CombinedOutput() + return string(out), err +} + +var _ = ginkgo.Describe( + "detached workspace early worker errors", + ginkgo.Label("task-cancel"), + func() { + for _, stage := range []string{"config", "client"} { + ginkgo.It( + "persists early "+stage+" failure for JSON followers", + ginkgo.SpecTimeout(time.Minute), + func(ctx context.Context) { + wd, err := os.Getwd() + framework.ExpectNoError(err) + f := framework.NewDefaultFramework(filepath.Join(wd, "bin")) + home, err := os.MkdirTemp("", "devsy-worker-error-") + framework.ExpectNoError(err) + ginkgo.DeferCleanup(os.RemoveAll, home) + cli := detachedValidationCLI{framework: f, home: home} + store, err := task.NewStoreAt(filepath.Join(home, "state", "tasks")) + framework.ExpectNoError(err) + tk, err := store.Create(task.CreateOptions{Command: "up"}) + framework.ExpectNoError(err) + framework.ExpectNoError(tk.BeginLaunch()) + expected := "prepare workspace client" + args := []string{ + cmdWorkspace, + "up", + "--task-id", + tk.ID(), + "--result-format=json", + "--id=Backend", + } + if stage == "config" { + framework.ExpectNoError( + os.WriteFile( + filepath.Join(home, "config.yaml"), + []byte("contexts: ["), + 0o600, + ), + ) + expected = "load devsy config" + } + worker := cli.newCommand(ctx, args...) + framework.ExpectNoError(worker.Start()) + framework.ExpectNoError(tk.SetPID(worker.Process.Pid)) + gomega.Expect(worker.Wait()).To(gomega.HaveOccurred()) + state, err := store.Get(tk.ID()) + framework.ExpectNoError(err) + gomega.Expect(state.Status).To(gomega.Equal(task.StatusFailed)) + gomega.Expect(state.Error).To(gomega.ContainSubstring(expected)) + cli.expectFailureEnvelope(ctx, tk.ID(), expected) + }, + ) + } + }, +) + +func (cli detachedValidationCLI) expectFailureEnvelope(ctx context.Context, id, diagnostic string) { + followCtx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() + out, err := cli.command(followCtx, + cmdWorkspace, + "task", + "logs", + id, + "--follow", + "--result-format=json", + ) + gomega.Expect(err).To(gomega.HaveOccurred()) + gomega.Expect(followCtx.Err()).NotTo(gomega.HaveOccurred()) + var envelope struct { + Kind string `json:"kind"` + Message string `json:"message"` + } + framework.ExpectNoError(json.Unmarshal([]byte(strings.TrimSpace(out)), &envelope)) + gomega.Expect(envelope.Kind).To(gomega.Equal("error")) + gomega.Expect(envelope.Message).To(gomega.ContainSubstring(diagnostic)) + gomega.Expect(out).NotTo(gomega.ContainSubstring("missing-provider-1471")) +} + +func detachedValidationEnvironment(home string) []string { + return append(os.Environ(), "DEVSY_HOME="+home, "DEVSY_TEST_TOKEN=missing-provider-1471") +} diff --git a/pkg/command/process.go b/pkg/command/process.go index c58c6f3172..2eb4b0d024 100644 --- a/pkg/command/process.go +++ b/pkg/command/process.go @@ -64,3 +64,12 @@ func KillTreeAfterWorkerExit(pid, treeName, identity string) error { func ProcessTreeIdentity(pid int) (string, error) { return processTreeIdentity(pid) } + +// ProcessExited reports positive evidence that the saved worker has exited. +// Identityless records cannot distinguish a worker from a reused PID. +func ProcessExited(ref ProcessRef) (bool, error) { + if ref.PID <= 0 || ref.Identity == "" || !strongProcessIdentitySupported { + return false, nil + } + return processRefExited(ref) +} diff --git a/pkg/command/process_exit_child_supported_test.go b/pkg/command/process_exit_child_supported_test.go new file mode 100644 index 0000000000..6873ceff87 --- /dev/null +++ b/pkg/command/process_exit_child_supported_test.go @@ -0,0 +1,14 @@ +//go:build linux || darwin || unix + +package command + +import ( + "os/exec" + "syscall" +) + +func processExitTestChild() *exec.Cmd { + child := exec.Command("sleep", "30") + child.SysProcAttr = &syscall.SysProcAttr{Setsid: true} + return child +} diff --git a/pkg/command/process_exit_child_windows_test.go b/pkg/command/process_exit_child_windows_test.go new file mode 100644 index 0000000000..d664de08aa --- /dev/null +++ b/pkg/command/process_exit_child_windows_test.go @@ -0,0 +1,7 @@ +package command + +import "os/exec" + +func processExitTestChild() *exec.Cmd { + return exec.Command("ping", "-n", "30", "127.0.0.1") +} diff --git a/pkg/command/process_exit_test.go b/pkg/command/process_exit_test.go new file mode 100644 index 0000000000..a741e57c5d --- /dev/null +++ b/pkg/command/process_exit_test.go @@ -0,0 +1,62 @@ +package command + +import ( + "errors" + "path/filepath" + "testing" + + "github.com/devsy-org/devsy/pkg/config" + "github.com/stretchr/testify/require" +) + +func TestProcessExitedRequiresSavedWorkerIdentity(t *testing.T) { + child := processExitTestChild() + require.NoError(t, child.Start()) + t.Cleanup(func() { _ = child.Process.Kill() }) + identity, err := ProcessTreeIdentity(child.Process.Pid) + require.NoError(t, err) + ref := ProcessRef{PID: child.Process.Pid, Identity: identity} + exited, err := ProcessExited(ref) + require.NoError(t, err) + require.False(t, exited) + stale := ref + stale.Identity += "-old" + exited, err = ProcessExited(stale) + require.NoError(t, err) + require.True(t, exited) + // A stale identity describes an exited worker, never authority to signal the current PID. + exited, err = ProcessExited(ref) + require.NoError(t, err) + require.False(t, exited) + require.NoError(t, child.Process.Kill()) + _ = child.Wait() + exited, err = ProcessExited(ref) + require.NoError(t, err) + require.True(t, exited) + ref.Identity = "" + exited, err = ProcessExited(ref) + require.NoError(t, err) + require.False(t, exited) +} + +func TestSupervisedPublicationFailureCleansUnclaimedWorker(t *testing.T) { + t.Setenv(config.EnvHome, t.TempDir()) + child := processExitTestChild() + var ref ProcessRef + publicationErr := errors.New("publication rejected") + err := startSupervisedDetached(child, SupervisedStartOptions{ + Name: "test-publication-failure", + OnStarted: func(started ProcessRef) error { + ref = started + exited, err := ProcessExited(ref) + require.NoError(t, err) + require.False(t, exited, "a published child waiting to resume is healthy") + return publicationErr + }, + }, filepath.Join(t.TempDir(), "streams")) + require.ErrorIs(t, err, publicationErr) + require.Positive(t, ref.PID) + exited, err := ProcessExited(ref) + require.NoError(t, err) + require.True(t, exited) +} diff --git a/pkg/command/process_supported.go b/pkg/command/process_supported.go index 8ddeced62b..9df7e3d5c1 100644 --- a/pkg/command/process_supported.go +++ b/pkg/command/process_supported.go @@ -423,3 +423,18 @@ func prepareBackgroundTree(cmd *exec.Cmd) { } cmd.SysProcAttr.Setsid = true } + +func processRefExited(ref ProcessRef) (bool, error) { + current, err := processTreeIdentity(ref.PID) + if err != nil { + return false, err + } + if current == "" || current != ref.Identity { + return true, nil + } + live, err := processIsLive(ref.PID) + if err != nil { + return false, err + } + return !live, nil +} diff --git a/pkg/command/process_windows.go b/pkg/command/process_windows.go index 0f5ac8c1af..ef2e2ad968 100644 --- a/pkg/command/process_windows.go +++ b/pkg/command/process_windows.go @@ -406,3 +406,11 @@ func truncateOutput(output string) string { } return output } + +func processRefExited(ref ProcessRef) (bool, error) { + running, err := savedWorkerRunning(ref) + if err != nil { + return false, err + } + return !running, nil +} diff --git a/pkg/task/startup_child_supported_test.go b/pkg/task/startup_child_supported_test.go new file mode 100644 index 0000000000..a922ba186d --- /dev/null +++ b/pkg/task/startup_child_supported_test.go @@ -0,0 +1,14 @@ +//go:build linux || darwin || unix + +package task + +import ( + "os/exec" + "syscall" +) + +func startupTestChild() *exec.Cmd { + child := exec.Command("sleep", "30") + child.SysProcAttr = &syscall.SysProcAttr{Setsid: true} + return child +} diff --git a/pkg/task/startup_child_windows_test.go b/pkg/task/startup_child_windows_test.go new file mode 100644 index 0000000000..c9637a08ec --- /dev/null +++ b/pkg/task/startup_child_windows_test.go @@ -0,0 +1,7 @@ +package task + +import "os/exec" + +func startupTestChild() *exec.Cmd { + return exec.Command("ping", "-n", "30", "127.0.0.1") +} diff --git a/pkg/task/startup_test.go b/pkg/task/startup_test.go new file mode 100644 index 0000000000..7bf7f8ccdd --- /dev/null +++ b/pkg/task/startup_test.go @@ -0,0 +1,118 @@ +package task + +import ( + "context" + "errors" + "os" + "path/filepath" + "sync" + "testing" + + "github.com/devsy-org/devsy/pkg/command" + "github.com/stretchr/testify/require" +) + +func TestReconcileWorkerBeforeClaim(t *testing.T) { + for _, claimed := range []bool{false, true} { + t.Run(map[bool]string{false: "unclaimed", true: "claimed"}[claimed], func(t *testing.T) { + store := newTestStore(t) + store.processController = fakeProcessController{} + tk := createTaskWith(t, store, CreateOptions{Command: "up"}) + child := startupTestChild() + require.NoError(t, child.Start()) + t.Cleanup(func() { _ = child.Process.Kill() }) + require.NoError(t, tk.SetPID(child.Process.Pid)) + if claimed { + require.NoError(t, tk.HoldWorkerLock()) + } + state, err := store.ReconcileTask(context.Background(), tk.ID()) + require.NoError(t, err) + require.False(t, state.Status.Terminal(), "healthy startup must not be abandoned") + require.NoError(t, child.Process.Kill()) + _ = child.Wait() + if claimed { + require.NoError(t, tk.ReleaseWorkerLock()) + } + var wg sync.WaitGroup + results := make(chan error, 4) + for range 4 { + wg.Go( + func() { _, err := store.ReconcileTask(context.Background(), tk.ID()); results <- err }, + ) + } + wg.Wait() + close(results) + for err := range results { + require.NoError(t, err) + } + state, err = store.ReconcileTask(context.Background(), tk.ID()) + require.NoError(t, err) + require.Equal(t, StatusFailed, state.Status) + require.True(t, state.ProcessCleanupComplete) + }) + } +} + +func TestReconcileUnclaimedWorkerCleanupRetry(t *testing.T) { + store := newTestStore(t) + tk := createTaskWith(t, store, CreateOptions{Command: "up"}) + child := startupTestChild() + require.NoError(t, child.Start()) + require.NoError(t, tk.SetPID(child.Process.Pid)) + require.NoError(t, child.Process.Kill()) + _ = child.Wait() + calls := 0 + store.processController = fakeProcessController{cleanup: func(command.ProcessRef) error { + calls++ + if calls == 1 { + return errors.New("cleanup unavailable") + } + return nil + }} + _, err := store.ReconcileTask(context.Background(), tk.ID()) + require.Error(t, err) + state, err := store.Get(tk.ID()) + require.NoError(t, err) + require.False(t, state.Status.Terminal()) + state, err = store.ReconcileTask(context.Background(), tk.ID()) + require.NoError(t, err) + require.Equal(t, StatusFailed, state.Status) + require.True(t, state.ProcessCleanupComplete) +} + +func TestExitConfirmationNeverCreatesWorkerClaim(t *testing.T) { + path := filepath.Join(t.TempDir(), "worker.lock") + calls := 0 + lock, ok := claimConfirmedWorkerExit(path+".reconcile", func() bool { + calls++ + return calls == 1 + }) + require.False(t, ok) + require.Nil(t, lock) + _, err := os.Stat(path) + require.ErrorIs(t, err, os.ErrNotExist) + lock, ok = claimConfirmedWorkerExit(path+".reconcile", func() bool { return false }) + require.False(t, ok) + require.Nil(t, lock) +} + +func TestUnclaimedWorkerExitPreservesCancellation(t *testing.T) { + store := newTestStore(t) + store.processController = fakeProcessController{} + tk := createTaskWith(t, store, CreateOptions{Command: "up"}) + child := startupTestChild() + require.NoError(t, child.Start()) + t.Cleanup(func() { _ = child.Process.Kill() }) + require.NoError(t, tk.SetPID(child.Process.Pid)) + _, err := tk.requestCancel() + require.NoError(t, err) + require.NoError(t, child.Process.Kill()) + _ = child.Wait() + state, err := store.ReconcileTask(context.Background(), tk.ID()) + require.NoError(t, err) + require.True(t, state.Canceled()) + require.True(t, state.ProcessCleanupComplete) + state, err = store.ReconcileTask(context.Background(), tk.ID()) + require.NoError(t, err) + require.True(t, state.Canceled()) +} diff --git a/pkg/task/store.go b/pkg/task/store.go index 0c44a41d09..cd5327c9f8 100644 --- a/pkg/task/store.go +++ b/pkg/task/store.go @@ -3,6 +3,7 @@ package task import ( "context" "encoding/json" + "errors" "fmt" "os" "path/filepath" @@ -515,26 +516,61 @@ func (s *Store) tryLock( return lock, true, nil } -// claimDeadWorkerLock acquires the task's worker lock, which only succeeds when -// no worker holds it. +// claimDeadWorkerLock claims reconciliation ownership after a worker releases +// its lock or its saved identity proves it exited before claiming one. func (s *Store) claimDeadWorkerLock(id string) (*flock.Flock, bool) { path, err := s.workerLockPath(id) if err != nil { return nil, false } - // No lock file means the worker never got far enough to claim one. - if _, statErr := os.Stat(path); statErr != nil { + _, statErr := os.Stat(path) + if errors.Is(statErr, os.ErrNotExist) { + // Reconciliation must never manufacture evidence that a worker claimed its lock. + return claimConfirmedWorkerExit( + path+".reconcile", + func() bool { return s.workerExitConfirmed(id) }, + ) + } + if statErr != nil { + return nil, false + } + lock := flock.New(path) + locked, err := lock.TryLock() + if err != nil || !locked { return nil, false } + return lock, true +} +func claimConfirmedWorkerExit(path string, confirmed func() bool) (*flock.Flock, bool) { + if !confirmed() { + return nil, false + } lock := flock.New(path) locked, err := lock.TryLock() if err != nil || !locked { return nil, false } + if !confirmed() { + _ = lock.Unlock() + return nil, false + } return lock, true } +func (s *Store) workerExitConfirmed(id string) bool { + state, err := s.Get(id) + if err != nil { + return false + } + ref, ok := state.ProcessReference() + if !ok { + return false + } + exited, err := command.ProcessExited(ref) + return err == nil && exited +} + // workerLockPath is distinct from the read-modify-write lock in withLock: this // one is held for the worker's entire lifetime, so sharing it would block // every state update. diff --git a/pkg/workspace/workspace.go b/pkg/workspace/workspace.go index 74af72b513..a2f9349474 100644 --- a/pkg/workspace/workspace.go +++ b/pkg/workspace/workspace.go @@ -71,7 +71,7 @@ func Resolve( devsyConfig *config.Config, params ResolveParams, ) (client.BaseWorkspaceClient, error) { - if err := validateDesiredID(params.DesiredID); err != nil { + if err := ValidateID(params.DesiredID); err != nil { return nil, err } @@ -115,7 +115,8 @@ func Resolve( return workspaceClient, nil } -func validateDesiredID(desiredID string) error { +// ValidateID checks an explicit workspace ID; empty leaves ID derivation to the source. +func ValidateID(desiredID string) error { if desiredID == "" { return nil } diff --git a/pkg/workspace/workspace_test.go b/pkg/workspace/workspace_test.go index 5c4e45b2af..2124f28b1e 100644 --- a/pkg/workspace/workspace_test.go +++ b/pkg/workspace/workspace_test.go @@ -21,7 +21,7 @@ const ( testDevContainerPath = ".devcontainer/devcontainer.json" ) -func TestValidateDesiredID(t *testing.T) { +func TestValidateID(t *testing.T) { cases := []struct { name string desiredID string @@ -45,7 +45,7 @@ func TestValidateDesiredID(t *testing.T) { } for _, c := range cases { t.Run(c.name, func(t *testing.T) { - err := validateDesiredID(c.desiredID) + err := ValidateID(c.desiredID) if c.wantErr { require.Error(t, err) } else { @@ -55,10 +55,10 @@ func TestValidateDesiredID(t *testing.T) { } } -func TestValidateDesiredID_InvalidCharsCheckedBeforeLength(t *testing.T) { +func TestValidateID_InvalidCharsCheckedBeforeLength(t *testing.T) { // An over-length id that also contains invalid characters should report the // character rule first, matching the historical ordering. - err := validateDesiredID(strings.Repeat("A", maxWorkspaceIDLength+1)) + err := ValidateID(strings.Repeat("A", maxWorkspaceIDLength+1)) require.Error(t, err) assert.Contains(t, err.Error(), "lowercase letters") } diff --git a/sites/docs-devsy-sh/content/docs/developing-in-workspaces/create-a-workspace.mdx b/sites/docs-devsy-sh/content/docs/developing-in-workspaces/create-a-workspace.mdx index c2cabcea26..a93de3ea05 100644 --- a/sites/docs-devsy-sh/content/docs/developing-in-workspaces/create-a-workspace.mdx +++ b/sites/docs-devsy-sh/content/docs/developing-in-workspaces/create-a-workspace.mdx @@ -25,6 +25,20 @@ Open **Workspaces** and click **Create**. The wizard has five steps: The desktop app runs `devsy workspace up` for you. +### Workspace IDs + +The workspace name is its ID. Use lowercase ASCII letters (`a-z`), digits (`0-9`), and hyphens (`-`), with a maximum of 48 characters. + +Desktop suggests a valid ID from your source. For example, a local folder named `Backend` suggests `backend`; the source path keeps its original case. If you enter a name yourself, it must follow the same rules: + +```sh +devsy workspace up ./Projects/Backend --id backend +``` + + +Uppercase workspace IDs are rejected before detached creation starts. Startup failures are recorded as failed tasks and shown in Desktop instead of leaving creation waiting indefinitely. Mixed-case source folder and repository names remain supported. + + ### Devsy CLI [Install the CLI](../getting-started/install.mdx#install-devsy-cli) and add a provider, for example Docker: From 13d7f38b105df28cd630c7f135912c79c7ac5dfc Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 19 Oct 2026 22:30:47 -0600 Subject: [PATCH 02/16] fix(workspace): keep detached recovery retryable on Windows --- .github/workflows/pr-ci.yml | 9 ++- cmd/workspace/task.go | 25 +++++++- cmd/workspace/task_tail_test.go | 99 +++++++++++++++++++++++++++++ pkg/command/job_windows.go | 17 ++--- pkg/command/process_windows_test.go | 72 +++++++++++++++++++++ pkg/task/task_test.go | 6 ++ 6 files changed, 216 insertions(+), 12 deletions(-) diff --git a/.github/workflows/pr-ci.yml b/.github/workflows/pr-ci.yml index f87c67393d..812c4c7946 100644 --- a/.github/workflows/pr-ci.yml +++ b/.github/workflows/pr-ci.yml @@ -154,8 +154,13 @@ jobs: with: go-version-file: go.mod cache: false - - name: test native Windows detached task lifecycle - run: go test -race -count=1 -timeout 10m ./pkg/command ./pkg/task ./pkg/workspace ./cmd/workspace/up ./cmd/workspace + - name: test native Windows detached task storage and processes + run: go test -race -count=1 -timeout 10m ./pkg/command ./pkg/task ./pkg/workspace + - name: test native Windows detached workspace lifecycle + run: >- + go test -race -count=1 -timeout 10m + -run '^Test(Detached.*|DetachWorkspaceLabel.*|WorkerRecords.*|DuplicateWorkerCannotFailOwnerTask|FinalizeWorker.*|OpenTaskExitsWhenAlreadyCanceled|FollowTask.*|LogTailer.*|ResultEnvelopeFrom.*|EmitTaskTransition|ReportTaskStateJSON.*|TaskErrorMessage|TaskStatusEvent.*)$' + ./cmd/workspace/up ./cmd/workspace - name: test native Windows transport run: >- go test -race -count=1 -v -timeout 5m ./pkg/docker diff --git a/cmd/workspace/task.go b/cmd/workspace/task.go index 684def6376..b54b3a65aa 100644 --- a/cmd/workspace/task.go +++ b/cmd/workspace/task.go @@ -181,7 +181,12 @@ type followTaskOptions struct { emitJSON bool } -func followTask(ctx context.Context, store *task.Store, opts followTaskOptions) error { +type taskFollowerStore interface { + ReconcileTask(context.Context, string) (*task.State, error) + Get(string) (*task.State, error) +} + +func followTask(ctx context.Context, store taskFollowerStore, opts followTaskOptions) error { var last *task.State ticker := time.NewTicker(opts.interval) defer ticker.Stop() @@ -195,7 +200,7 @@ func followTask(ctx context.Context, store *task.Store, opts followTaskOptions) for { tailer.poll(os.Stderr) - state, err := store.ReconcileTask(ctx, opts.id) + state, err := readTaskForFollower(ctx, store, opts.id) if err != nil { return err } @@ -216,6 +221,22 @@ func followTask(ctx context.Context, store *task.Store, opts followTaskOptions) } } +func readTaskForFollower( + ctx context.Context, + store taskFollowerStore, + id string, +) (*task.State, error) { + state, err := store.ReconcileTask(ctx, id) + if err == nil { + return state, nil + } + if ctx.Err() != nil { + return nil, ctx.Err() + } + // Cleanup can be retried; only persisted terminal state ends the follower. + return store.Get(id) +} + func emitTaskTransition(last, current *task.State, emitJSON bool) { if last != nil && last.Phase == current.Phase && last.Step == current.Step && last.Status == current.Status && last.OperationID == current.OperationID && diff --git a/cmd/workspace/task_tail_test.go b/cmd/workspace/task_tail_test.go index 150db2cac8..f0f014c889 100644 --- a/cmd/workspace/task_tail_test.go +++ b/cmd/workspace/task_tail_test.go @@ -3,6 +3,7 @@ package workspace import ( "bytes" "context" + "errors" "io" "os" "path/filepath" @@ -10,6 +11,7 @@ import ( "testing" "time" + "github.com/devsy-org/devsy/pkg/clierr" "github.com/devsy-org/devsy/pkg/config" devcconfig "github.com/devsy-org/devsy/pkg/devcontainer/config" "github.com/devsy-org/devsy/pkg/status" @@ -24,6 +26,7 @@ func newTestTailer(t *testing.T, taskID string) (*logTailer, string) { t.Cleanup(config.ResetPathManager) tailer := newLogTailer(taskID) + t.Cleanup(func() { tailer.flush(io.Discard) }) if tailer.path == "" { t.Fatal("newLogTailer produced an empty path") } @@ -327,3 +330,99 @@ func TestFollowTaskReconcilesExitedWorker(t *testing.T) { require.NoError(t, err) require.Equal(t, task.StatusFailed, state.Status) } + +type failingReconciliationStore struct { + *task.Store + reconcile func(context.Context, string) (*task.State, error) +} + +func (s failingReconciliationStore) ReconcileTask( + ctx context.Context, + id string, +) (*task.State, error) { + return s.reconcile(ctx, id) +} + +func TestFollowTaskRetriesCleanupBeforeTerminalFailure(t *testing.T) { + config.SetPathManager(fakeRuntimeDirPathManager{dir: t.TempDir()}) + t.Cleanup(config.ResetPathManager) + store, err := task.NewStoreAt(t.TempDir()) + require.NoError(t, err) + tk, err := store.Create(task.CreateOptions{Command: "up"}) + require.NoError(t, err) + require.NoError(t, tk.HoldWorkerLock()) + require.NoError(t, tk.ReleaseWorkerLock()) + path, err := config.DefaultPathManager().ProcessStreamsFile(task.WorkerProcessName(tk.ID())) + require.NoError(t, err) + require.NoError(t, os.WriteFile(path, []byte("captured startup failure\n"), 0o600)) + transient := errors.New("transient cleanup failure") + calls := 0 + wrapper := failingReconciliationStore{ + Store: store, + reconcile: func(ctx context.Context, id string) (*task.State, error) { + calls++ + if calls == 1 { + return nil, transient + } + state, err := store.Get(id) + require.NoError(t, err) + require.False(t, state.Status.Terminal(), "cleanup failure must leave task retryable") + return store.ReconcileTask(ctx, id) + }, + } + reader, writer, err := os.Pipe() + require.NoError(t, err) + original := os.Stderr + os.Stderr = writer + t.Cleanup(func() { os.Stderr = original; _ = reader.Close(); _ = writer.Close() }) + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + followErr := followTask( + ctx, + wrapper, + followTaskOptions{id: tk.ID(), interval: time.Millisecond, emitJSON: true}, + ) + os.Stderr = original + require.NoError(t, writer.Close()) + logs, err := io.ReadAll(reader) + require.NoError(t, err) + require.Equal(t, 1, strings.Count(string(logs), "captured startup failure")) + require.Error(t, followErr) + require.NotErrorIs(t, followErr, transient) + require.NotErrorIs(t, followErr, context.DeadlineExceeded) + var classified *clierr.CLIError + require.ErrorAs(t, followErr, &classified) + require.Equal(t, 2, calls) + state, err := store.Get(tk.ID()) + require.NoError(t, err) + require.Equal(t, task.StatusFailed, state.Status) + require.Equal(t, state.Error, classified.Message) +} + +func TestFollowTaskRetryHonorsCancellation(t *testing.T) { + config.SetPathManager(fakeRuntimeDirPathManager{dir: t.TempDir()}) + t.Cleanup(config.ResetPathManager) + store, err := task.NewStoreAt(t.TempDir()) + require.NoError(t, err) + tk, err := store.Create(task.CreateOptions{}) + require.NoError(t, err) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + calls := 0 + wrapper := failingReconciliationStore{ + Store: store, + reconcile: func(context.Context, string) (*task.State, error) { + calls++ + if calls == 2 { + cancel() + } + return nil, errors.New("cleanup unavailable") + }, + } + err = followTask(ctx, wrapper, followTaskOptions{id: tk.ID(), interval: time.Millisecond}) + require.ErrorIs(t, err, context.Canceled) + require.Equal(t, 2, calls) + state, err := store.Get(tk.ID()) + require.NoError(t, err) + require.False(t, state.Status.Terminal()) +} diff --git a/pkg/command/job_windows.go b/pkg/command/job_windows.go index 57bdc4156f..6b5f88bbfb 100644 --- a/pkg/command/job_windows.go +++ b/pkg/command/job_windows.go @@ -87,7 +87,7 @@ func canonicalDirectoryPath(path string) (string, error) { 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 canonicalMissingDirectoryPath(absPath, err) } return "", err } @@ -105,15 +105,16 @@ func canonicalDirectoryPath(path string) (string, error) { return windows.UTF16ToString(buffer[:length]), nil } -func normalizedWindowsPath(path string) string { - path = filepath.Clean(path) - if strings.HasPrefix(path, `\\?\`) { - return path +func canonicalMissingDirectoryPath(absPath string, lookupErr error) (string, error) { + parent := filepath.Dir(absPath) + if parent == absPath { + return "", lookupErr } - if strings.HasPrefix(path, `\\`) { - return `\\?\UNC\` + strings.TrimPrefix(path, `\\`) + canonicalParent, err := canonicalDirectoryPath(parent) + if err != nil { + return "", err } - return `\\?\` + path + return filepath.Join(canonicalParent, filepath.Base(absPath)), nil } func prepareBackgroundTree(cmd *exec.Cmd) { diff --git a/pkg/command/process_windows_test.go b/pkg/command/process_windows_test.go index 29ee3f60eb..807011a9c0 100644 --- a/pkg/command/process_windows_test.go +++ b/pkg/command/process_windows_test.go @@ -735,3 +735,75 @@ func TestKillTerminatesOrphanedGrandchildViaJobObject(t *testing.T) { t.Fatalf("KillTreeAfterWorkerExit after natural tree cleanup: %v", err) } } + +func TestJobNameForMissingNestedHomeUsesCanonicalAncestor(t *testing.T) { + parent := filepath.Join(t.TempDir(), "parent") + home := filepath.Join(parent, "home") + if err := os.MkdirAll(home, 0o700); err != nil { + t.Fatal(err) + } + t.Setenv(config.EnvHome, home) + before, err := jobNameFor("jupyter") + if err != nil { + t.Fatal(err) + } + if err := os.Remove(home); err != nil { + t.Fatal(err) + } + if err := os.Remove(parent); err != nil { + t.Fatal(err) + } + after, err := jobNameFor("jupyter") + if err != nil { + t.Fatal(err) + } + if before != after { + t.Fatalf("deleted nested home changed job identity: %q != %q", before, after) + } +} + +func TestJobNameForDeletedHomePreservesShortParentAlias(t *testing.T) { + parent := filepath.Join(t.TempDir(), "long parent directory for short alias") + if err := os.Mkdir(parent, 0o700); err != nil { + t.Fatal(err) + } + parentPtr, err := windows.UTF16PtrFromString(parent) + if err != nil { + t.Fatal(err) + } + buffer := make([]uint16, 32768) + length, err := windows.GetShortPathName(parentPtr, &buffer[0], uint32(len(buffer))) + if err != nil { + t.Fatal(err) + } + if length >= uint32(len(buffer)) { + t.Fatal("short parent path exceeds buffer") + } + shortParent := windows.UTF16ToString(buffer[:length]) + if strings.EqualFold(parent, shortParent) { + t.Skip("filesystem does not provide an 8.3 alias") + } + home := filepath.Join(shortParent, "home") + if err := os.Mkdir(home, 0o700); err != nil { + t.Fatal(err) + } + t.Setenv(config.EnvHome, home) + before, err := jobNameFor("jupyter") + if err != nil { + t.Fatal(err) + } + if err := os.Remove(home); err != nil { + t.Fatal(err) + } + after, err := jobNameFor("jupyter") + if err != nil { + t.Fatal(err) + } + if before != after { + t.Fatalf( + "deleted home with short parent alias changed job identity: %q != %q", + before, + after, + ) + } +} diff --git a/pkg/task/task_test.go b/pkg/task/task_test.go index d940baca5b..8e18785675 100644 --- a/pkg/task/task_test.go +++ b/pkg/task/task_test.go @@ -1285,6 +1285,12 @@ func TestCancelUnpublishedPIDStaysRetryable(t *testing.T) { t.Fatalf("BeginLaunch: %v", err) } + t.Cleanup(func() { + if err := releaseLauncherLock(tk); err != nil { + t.Errorf("release launcher: %v", err) + } + }) + setKillProcessForTest(store, tk, func(pid, treeName string) error { t.Error("kill invoked before the worker published a pid") return nil From 17cdd82345baabe646da9544b21b0856b5d9a368 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 20 Oct 2026 01:27:31 -0600 Subject: [PATCH 03/16] fix(desktop): isolate workspace status delivery failures --- .../main/__tests__/ipc-workspace-jobs.test.ts | 78 ++++++++++++++++++ desktop/src/main/__tests__/watcher.test.ts | 82 +++++++++++++++++++ desktop/src/main/ipc.ts | 43 +++++----- desktop/src/main/renderer-events.ts | 17 ++++ desktop/src/main/watcher.ts | 6 +- 5 files changed, 198 insertions(+), 28 deletions(-) create mode 100644 desktop/src/main/renderer-events.ts diff --git a/desktop/src/main/__tests__/ipc-workspace-jobs.test.ts b/desktop/src/main/__tests__/ipc-workspace-jobs.test.ts index f65475f192..af77c97afa 100644 --- a/desktop/src/main/__tests__/ipc-workspace-jobs.test.ts +++ b/desktop/src/main/__tests__/ipc-workspace-jobs.test.ts @@ -454,6 +454,84 @@ describe("workspace lifecycle IPC", () => { }, ) + it.each(["success", "failure"])( + "keeps %s and task ownership after status delivery throws", + async (outcome) => { + const ctx = setup() + invoke("workspace_up", { source: "ws", commandId: "up" }) + await flush() + ctx.send.mockImplementation((channel) => { + if (channel === "workspace-status") + throw new Error("status renderer unavailable") + }) + expect(() => + ctx.line( + '{"kind":"status","schemaVersion":1,"phase":"building_image","state":"started"}', + ), + ).not.toThrow() + expect(ctx.jobs.get("ws")).toMatchObject({ state: "running" }) + ctx.line( + outcome === "success" + ? '{"kind":"result","outcome":"success"}' + : '{"kind":"error","outcome":"error","code":"invalid_id","message":"invalid ID","hint":"Use lowercase"}', + ) + await flush() + expect(ctx.jobs.get("ws")).toMatchObject({ + state: outcome === "failure" ? "failed" : "succeeded", + ...(outcome === "failure" ? { error: "invalid ID" } : {}), + }) + expect(ctx.closeLog).toHaveBeenCalledOnce() + const terminal = ctx.send.mock.calls.filter( + ([channel, progress]) => + channel === "command-progress" && progress.done, + ) + expect(terminal).toHaveLength(1) + if (outcome === "failure") + expect(terminal[0][1].cliError).toMatchObject({ + code: "invalid_id", + message: "invalid ID", + hint: "Use lowercase", + }) + invoke("workspace_up", { source: "ws", commandId: "retry" }) + await flush() + expect(ctx.jobs.get("ws")).toMatchObject({ + commandId: "retry", + state: "running", + }) + expect(ctx.cli.run).not.toHaveBeenCalledWith([ + "workspace", + "task", + "cancel", + "task-1", + ]) + }, + ) + + it.each([0, 1])( + "retains action outcome %s when forwarded status delivery throws", + async (exitCode) => { + const ctx = setup() + invoke("workspace_rebuild") + await flush() + ctx.send.mockImplementation((channel) => { + if (channel === "workspace-status") + throw new Error("status renderer unavailable") + }) + expect(() => + ctx.line( + '{"kind":"status","schemaVersion":1,"phase":"building_image","state":"started"}', + ), + ).not.toThrow() + expect(ctx.jobs.get("ws")?.state).toBe("running") + ctx.exit(exitCode) + await flush() + expect(ctx.jobs.get("ws")).toMatchObject({ + state: exitCode === 0 ? "succeeded" : "failed", + ...(exitCode === 1 ? { error: "Command exited with code 1" } : {}), + }) + }, + ) + it("uses the same registry for tray stop", async () => { const ctx = setup() const completion = ctx.actions.workspaceActions.stop("ws") diff --git a/desktop/src/main/__tests__/watcher.test.ts b/desktop/src/main/__tests__/watcher.test.ts index 2ac2a6f273..469d438e31 100644 --- a/desktop/src/main/__tests__/watcher.test.ts +++ b/desktop/src/main/__tests__/watcher.test.ts @@ -330,3 +330,85 @@ describe("workspace status ordering", () => { await expect(watcher.refreshWorkspaceStatus("ws")).resolves.toBeUndefined() }) }) + +describe("Watcher job broadcasts", () => { + it.each( + ["success", "failure"].flatMap((outcome) => + ["send", "lookup", "window", "contents"].map((boundary) => [ + outcome, + boundary, + ]), + ), + )( + "keeps %s reconciliation independent of renderer %s failure", + async (outcome, boundary) => { + const state = new DaemonState() + const workspaceJobs = new WorkspaceJobs() + const send = vi.fn() + const win = { + isDestroyed: vi.fn(() => false), + webContents: { isDestroyed: vi.fn(() => false), send }, + } + const getMainWindow = vi.fn(() => win) + const cli = { + run: vi.fn(async () => [{ id: "ws", context: "default" }]), + runRaw: vi.fn(async () => JSON.stringify({ state: "running" })), + } + const watcher = new Watcher({ + cli: cli as never, + state, + getMainWindow: getMainWindow as never, + providerJobs: { snapshot: () => ({}) } as never, + workspaceJobs, + }) + workspaceJobs.onChange(() => watcher.broadcastWorkspaces()) + const refresh = vi.fn(async () => { + await watcher.refreshWorkspaces() + await watcher.refreshWorkspaceStatus("ws") + watcher.broadcastWorkspaces() + }) + workspaceJobs.setRefresh(refresh) + const generation = workspaceJobs.start("ws", "creating", "first") + send.mockClear() + if (boundary === "send") + send.mockImplementation(() => { + throw new Error("renderer delivery failed") + }) + if (boundary === "lookup") + getMainWindow.mockImplementation(() => { + throw new Error("window lookup failed") + }) + if (boundary === "window") win.isDestroyed.mockReturnValue(true) + if (boundary === "contents") + win.webContents.isDestroyed.mockReturnValue(true) + expect(() => + workspaceJobs.phase("ws", "first", "Launching command"), + ).not.toThrow() + expect(() => + workspaceJobs.progress("ws", "first", { + phase: "building_image", + state: "started", + }), + ).not.toThrow() + const originalError = outcome === "failure" ? "invalid ID" : undefined + await expect( + workspaceJobs.finish("ws", generation, originalError), + ).resolves.toBeUndefined() + expect(refresh).toHaveBeenCalledOnce() + expect(cli.run).toHaveBeenCalledOnce() + expect(cli.runRaw).toHaveBeenCalledOnce() + expect(workspaceJobs.get("ws")).toMatchObject({ + state: outcome === "failure" ? "failed" : "succeeded", + error: originalError, + refreshError: undefined, + }) + if (boundary === "window" || boundary === "contents") + expect(send).not.toHaveBeenCalled() + expect(() => workspaceJobs.start("ws", "creating", "retry")).not.toThrow() + expect(workspaceJobs.get("ws")).toMatchObject({ + commandId: "retry", + state: "running", + }) + }, + ) +}) diff --git a/desktop/src/main/ipc.ts b/desktop/src/main/ipc.ts index bcd5fb8891..9400fa3604 100644 --- a/desktop/src/main/ipc.ts +++ b/desktop/src/main/ipc.ts @@ -25,6 +25,7 @@ import type { MachineDiagnosticsManager } from "./machine-diagnostics-manager.js import type { MachineDiagnosticsStore } from "./machine-diagnostics-store.js" import type { ProviderActivity, ProviderJobs } from "./provider-jobs.js" import type { PtyManager } from "./pty.js" +import { sendRendererEvent } from "./renderer-events.js" import { isTrustedSecretIpcSender } from "./secret-ipc-policy.js" import { SecretSession } from "./secret-session.js" import type { SettingsService } from "./settings-service.js" @@ -293,28 +294,22 @@ function createLogSink( if (!done && buf.length === 0) return const lines = buf buf = [] - try { - const win = getWin() - if (!win || win.isDestroyed?.() || win.webContents.isDestroyed?.()) return - win.webContents.send("command-progress", { - commandId, - lines: lines.map(redactSensitiveText), - done, - ...(extra - ? { - ...extra, - message: extra.message - ? redactSensitiveText(extra.message) - : undefined, - cliError: extra.cliError - ? redactCLIError(extra.cliError) - : undefined, - } - : {}), - }) - } catch { - mainLog.warn("[workspace-operation] progress delivery unavailable") - } + sendRendererEvent(getWin, "command-progress", { + commandId, + lines: lines.map(redactSensitiveText), + done, + ...(extra + ? { + ...extra, + message: extra.message + ? redactSensitiveText(extra.message) + : undefined, + cliError: extra.cliError + ? redactCLIError(extra.cliError) + : undefined, + } + : {}), + }) } return { @@ -964,7 +959,7 @@ export function registerIpcHandlers(deps: IpcDependencies): { if (!workspaceJobs.owns(workspaceId, commandId)) return true const status = redactOperationStatus(normalizeOperationStatus(envelope)) workspaceJobs.progress(workspaceId, commandId, status) - deps.getMainWindow()?.webContents.send("workspace-status", { + sendRendererEvent(deps.getMainWindow, "workspace-status", { commandId, workspaceId, ...status, @@ -2107,7 +2102,7 @@ export function registerIpcHandlers(deps: IpcDependencies): { normalizeOperationStatus(envelope), ) workspaceJobs.progress(wsId, cmdId, status) - deps.getMainWindow()?.webContents.send("workspace-status", { + sendRendererEvent(deps.getMainWindow, "workspace-status", { commandId: cmdId, workspaceId: wsId, ...status, diff --git a/desktop/src/main/renderer-events.ts b/desktop/src/main/renderer-events.ts new file mode 100644 index 0000000000..4fb81c7135 --- /dev/null +++ b/desktop/src/main/renderer-events.ts @@ -0,0 +1,17 @@ +import type { BrowserWindow } from "electron" +import { mainLog } from "./logging.js" + +export function sendRendererEvent( + getWindow: () => BrowserWindow | null, + channel: string, + payload: unknown, +): void { + try { + const window = getWindow() + if (!window || window.isDestroyed?.() || window.webContents.isDestroyed?.()) + return + window.webContents.send(channel, payload) + } catch { + mainLog.warn(`[renderer] ${channel} delivery unavailable`) + } +} diff --git a/desktop/src/main/watcher.ts b/desktop/src/main/watcher.ts index 6a8e9b0c3d..8dde979e41 100644 --- a/desktop/src/main/watcher.ts +++ b/desktop/src/main/watcher.ts @@ -7,6 +7,7 @@ import type { CliRunner } from "./cli.js" import type { DaemonClient } from "./daemon-client.js" import { mainLog } from "./logging.js" import type { ProviderJobs } from "./provider-jobs.js" +import { sendRendererEvent } from "./renderer-events.js" import type { DaemonState, Machine, Workspace } from "./state.js" import type { WorkspaceJobs } from "./workspace-jobs.js" import { normalizeWorkspaceStatus } from "./workspace-status.js" @@ -381,9 +382,6 @@ export class Watcher { } private send(channel: string, payload: unknown): void { - const win = this.deps.getMainWindow() - if (win && !win.isDestroyed()) { - win.webContents.send(channel, payload) - } + sendRendererEvent(this.deps.getMainWindow, channel, payload) } } From bbd0e12b58a53bb869ff0abe2966be34d25e8db9 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 20 Oct 2026 04:15:26 -0600 Subject: [PATCH 04/16] fix(workspace): log retryable task reconciliation errors --- cmd/workspace/task.go | 11 +++- cmd/workspace/task_tail_test.go | 112 ++++++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+), 1 deletion(-) diff --git a/cmd/workspace/task.go b/cmd/workspace/task.go index b54b3a65aa..0fed237d99 100644 --- a/cmd/workspace/task.go +++ b/cmd/workspace/task.go @@ -12,6 +12,7 @@ import ( config2 "github.com/devsy-org/devsy/pkg/devcontainer/config" cliflags "github.com/devsy-org/devsy/pkg/flags" "github.com/devsy-org/devsy/pkg/flags/names" + "github.com/devsy-org/devsy/pkg/log" "github.com/devsy-org/devsy/pkg/output" "github.com/devsy-org/devsy/pkg/status" "github.com/devsy-org/devsy/pkg/table" @@ -188,6 +189,7 @@ type taskFollowerStore interface { func followTask(ctx context.Context, store taskFollowerStore, opts followTaskOptions) error { var last *task.State + var lastReconcileError error ticker := time.NewTicker(opts.interval) defer ticker.Stop() @@ -200,7 +202,7 @@ func followTask(ctx context.Context, store taskFollowerStore, opts followTaskOpt for { tailer.poll(os.Stderr) - state, err := readTaskForFollower(ctx, store, opts.id) + state, err := readTaskForFollower(ctx, store, opts.id, &lastReconcileError) if err != nil { return err } @@ -225,14 +227,21 @@ func readTaskForFollower( ctx context.Context, store taskFollowerStore, id string, + lastReconcileError *error, ) (*task.State, error) { state, err := store.ReconcileTask(ctx, id) if err == nil { + *lastReconcileError = nil return state, nil } if ctx.Err() != nil { return nil, ctx.Err() } + message := err.Error() + if *lastReconcileError == nil || message != (*lastReconcileError).Error() { + log.Debugf("Reconcile task %s: %s", id, message) + *lastReconcileError = err + } // Cleanup can be retried; only persisted terminal state ends the follower. return store.Get(id) } diff --git a/cmd/workspace/task_tail_test.go b/cmd/workspace/task_tail_test.go index f0f014c889..f87c355652 100644 --- a/cmd/workspace/task_tail_test.go +++ b/cmd/workspace/task_tail_test.go @@ -3,6 +3,7 @@ package workspace import ( "bytes" "context" + "encoding/json" "errors" "io" "os" @@ -14,6 +15,8 @@ import ( "github.com/devsy-org/devsy/pkg/clierr" "github.com/devsy-org/devsy/pkg/config" devcconfig "github.com/devsy-org/devsy/pkg/devcontainer/config" + "github.com/devsy-org/devsy/pkg/log" + "github.com/devsy-org/devsy/pkg/secrets" "github.com/devsy-org/devsy/pkg/status" "github.com/devsy-org/devsy/pkg/task" "github.com/stretchr/testify/require" @@ -426,3 +429,112 @@ func TestFollowTaskRetryHonorsCancellation(t *testing.T) { require.NoError(t, err) require.False(t, state.Status.Terminal()) } + +func TestFollowTaskReconciliationDiagnostics(t *testing.T) { + for _, level := range []string{"debug", "info"} { + t.Run(level, func(t *testing.T) { + config.SetPathManager(fakeRuntimeDirPathManager{dir: t.TempDir()}) + t.Cleanup(config.ResetPathManager) + secret := strings.Repeat("test-value-", 8) + reader, writer, err := os.Pipe() + require.NoError(t, err) + original := os.Stdout + os.Stdout = writer + t.Cleanup(func() { os.Stdout = original; _ = reader.Close(); _ = writer.Close() }) + log.InitTest(t) + log.Init( + log.Config{ + Level: level, + Format: "json", + Redactor: secrets.NewRedactor([]string{"TOKEN=" + secret}), + }, + ) + var diagnostics bytes.Buffer + remove := log.AddSink(&diagnostics) + defer remove() + // Each follower must report its own first failure, suppress consecutive + // duplicates, and report a recurrence after successful reconciliation. + for range 2 { + runFollowerReconciliationSequence(t, secret) + } + _ = log.Sync() + os.Stdout = original + require.NoError(t, writer.Close()) + output, err := io.ReadAll(reader) + require.NoError(t, err) + lines := strings.Split(strings.TrimSpace(string(output)), "\n") + require.Len(t, lines, 2, "only terminal result envelopes belong on stdout") + for _, line := range lines { + var envelope map[string]any + require.NoError(t, json.Unmarshal([]byte(line), &envelope)) + require.Equal(t, "result", envelope["kind"]) + require.NotContains(t, line, "cleanup") + } + requireReconciliationDiagnostics(t, level, diagnostics.String(), secret) + }) + } +} + +func requireReconciliationDiagnostics(t *testing.T, level, diagnostics, secret string) { + t.Helper() + require.NotContains(t, diagnostics, secret) + if level == "info" { + require.Empty(t, diagnostics) + return + } + records := strings.Split(strings.TrimSpace(diagnostics), "\n") + require.Len(t, records, 6) + for i, line := range records { + var record map[string]any + require.NoError(t, json.Unmarshal([]byte(line), &record)) + require.Equal(t, "debug", record["level"]) + require.NotContains(t, record, "kind") + if i%3 == 1 { + require.Contains(t, record["msg"], "lock unavailable") + } else { + require.Contains(t, record["msg"], "cleanup ***") + } + } +} + +func runFollowerReconciliationSequence(t *testing.T, secret string) { + t.Helper() + store, err := task.NewStoreAt(t.TempDir()) + require.NoError(t, err) + tk, err := store.Create(task.CreateOptions{}) + require.NoError(t, err) + sequence := []string{ + "cleanup " + secret, + "cleanup " + secret, + "lock unavailable", + "lock unavailable", + "", + "cleanup " + secret, + "cleanup " + secret, + } + calls := 0 + wrapper := failingReconciliationStore{ + Store: store, + reconcile: func(_ context.Context, id string) (*task.State, error) { + if calls == len(sequence) { + require.NoError(t, tk.Succeed(nil)) + return store.Get(id) + } + message := sequence[calls] + calls++ + if message == "" { + return store.Get(id) + } + return nil, errors.New(message) + }, + } + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + followErr := followTask( + ctx, + wrapper, + followTaskOptions{id: tk.ID(), interval: time.Millisecond, emitJSON: true}, + ) + cancel() + require.NoError(t, followErr) + require.Equal(t, len(sequence), calls) +} From 20d76ece4bc8415c2f57cf93e504163b3a71612b Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 20 Oct 2026 05:48:43 -0600 Subject: [PATCH 05/16] fix(desktop): preserve source names and log confirmation --- .../main/__tests__/ipc-workspace-jobs.test.ts | 44 ++++++++++++++++++- desktop/src/main/ipc.ts | 32 ++++++++------ .../src/lib/utils/workspace-id.test.ts | 18 ++++++++ .../renderer/src/lib/utils/workspace-id.ts | 17 ++++--- 4 files changed, 90 insertions(+), 21 deletions(-) diff --git a/desktop/src/main/__tests__/ipc-workspace-jobs.test.ts b/desktop/src/main/__tests__/ipc-workspace-jobs.test.ts index af77c97afa..eabff8221b 100644 --- a/desktop/src/main/__tests__/ipc-workspace-jobs.test.ts +++ b/desktop/src/main/__tests__/ipc-workspace-jobs.test.ts @@ -29,7 +29,7 @@ vi.mock("../analytics.js", () => ({ const { registerIpcHandlers } = await import("../ipc.js") function setup() { const jobs = new WorkspaceJobs() - let exit!: (code: number) => void + let exit!: (code: number, error?: unknown) => void let line!: (line: string, stream: "stdout") => void const cli = { setUnlockHandler: vi.fn(), @@ -89,7 +89,7 @@ function setup() { webContents, send, actions, - exit: (code: number) => exit(code), + exit: (code: number, error?: unknown) => exit(code, error), line: (text: string) => line(text, "stdout"), } } @@ -532,6 +532,46 @@ describe("workspace lifecycle IPC", () => { }, ) + it.each([0, 1])( + "keeps action outcome %s unconfirmed when log close fails", + async (exitCode) => { + const ctx = setup() + vi.stubEnv("TEST_API_TOKEN", "private-token") + ctx.closeLog.mockRejectedValueOnce(new Error("disk unavailable")) + invoke("workspace_rebuild") + await flush() + ctx.exit(exitCode, { + code: "action_failed", + message: "Cannot rebuild private-token", + }) + await flush() + expect(ctx.jobs.get("ws")).toMatchObject({ + state: exitCode === 0 ? "reconciling" : "failed", + refreshError: "Workspace log could not be saved: disk unavailable", + error: exitCode === 0 ? undefined : "Cannot rebuild ***", + }) + const terminal = () => + ctx.send.mock.calls.filter( + ([channel, value]) => channel === "command-progress" && value.done, + ) + expect(terminal()).toHaveLength(1) + expect(terminal()[0][1].success).toBe(exitCode === 0) + if (exitCode !== 0) + expect(terminal()[0][1].cliError).toMatchObject({ + code: "action_failed", + message: "Cannot rebuild ***", + }) + await ctx.jobs.retryRefresh("ws") + expect(ctx.jobs.get("ws")).toMatchObject({ + state: exitCode === 0 ? "succeeded" : "failed", + refreshError: undefined, + error: exitCode === 0 ? undefined : "Cannot rebuild ***", + }) + expect(terminal()).toHaveLength(1) + expect(ctx.closeLog).toHaveBeenCalledOnce() + }, + ) + it("uses the same registry for tray stop", async () => { const ctx = setup() const completion = ctx.actions.workspaceActions.stop("ws") diff --git a/desktop/src/main/ipc.ts b/desktop/src/main/ipc.ts index 9400fa3604..0f41d0f617 100644 --- a/desktop/src/main/ipc.ts +++ b/desktop/src/main/ipc.ts @@ -2391,29 +2391,35 @@ export function registerIpcHandlers(deps: IpcDependencies): { .catch(reject) }) timing("completed") - await sink + const flushError = await sink .done(formatLogLine("Completed"), { success: true }) - .catch((error) => - mainLog.warn("[workspace-operation] log flush failed", error), - ) - await workspaceJobs.finish(args.workspaceId, generation) + .catch((error) => { + mainLog.warn("[workspace-operation] log flush failed", error) + return undefined + }) + await workspaceJobs.finish( + args.workspaceId, + generation, + undefined, + flushError, + ) timing("reconciled") } catch (error) { const cliError = redactCLIError( cliErrorOrFallback(error, "workspace_operation_failed"), ) - if (sink) - await sink - .done(formatLogLine(cliError.message, "ERROR"), { - success: false, - level: "error", - cliError, - }) - .catch(() => {}) + const flushError = await sink + ?.done(formatLogLine(cliError.message, "ERROR"), { + success: false, + level: "error", + cliError, + }) + .catch(() => undefined) await workspaceJobs.finish( args.workspaceId, generation, cliError.message, + flushError, ) throw error } diff --git a/desktop/src/renderer/src/lib/utils/workspace-id.test.ts b/desktop/src/renderer/src/lib/utils/workspace-id.test.ts index b4328459bb..dd86219ee8 100644 --- a/desktop/src/renderer/src/lib/utils/workspace-id.test.ts +++ b/desktop/src/renderer/src/lib/utils/workspace-id.test.ts @@ -17,7 +17,18 @@ describe("workspace IDs", () => { ) it.each([ "/var/home/dev/Projects/Backend", + "/home/dev/@work/Backend", + "/home/first@corp/Backend", + "C:\\Users\\first@corp\\Backend", + "./first@corp/Backend", + "../first@corp/Backend", + "~/first@corp/Backend", "https://example.com/Backend.git", + "ssh://git@host/org/Backend.git@main", + "https://user@example.com/org/Backend.git@sha256:abc123", + "git@host:org/Backend.git@feature/foo@subpath:sub/path", + "github.com/org/Backend@pull/123/head", + "file:///projects/Backend.git@main", "https://example.com/Backend.git@feature/foo", "git@host:org/Backend.git", "github.com/org/Backend@main@subpath:sub/path", @@ -27,6 +38,13 @@ describe("workspace IDs", () => { expect(deriveWorkspaceId(source)).toBe("backend") expect(source).toBe(original) }) + it.each([ + "/home/dev/Backend@work", + "C:\\Users\\dev\\Backend@work", + "Backend@work", + ])("preserves the local basename in %s", (source) => { + expect(deriveWorkspaceId(source)).toBe("backend-work") + }) it("normalizes, truncates and falls back to valid IDs", () => { for (const source of ["a".repeat(80), "___", "App.Name With Spaces"]) expect(isValidWorkspaceId(deriveWorkspaceId(source))).toBe(true) diff --git a/desktop/src/renderer/src/lib/utils/workspace-id.ts b/desktop/src/renderer/src/lib/utils/workspace-id.ts index fc818b3f45..8cf0314788 100644 --- a/desktop/src/renderer/src/lib/utils/workspace-id.ts +++ b/desktop/src/renderer/src/lib/utils/workspace-id.ts @@ -6,15 +6,20 @@ export function isValidWorkspaceId(id: string): boolean { } export function deriveWorkspaceId(source: string): string { - const repository = source - .trim() - .replace(/^[^/@]+@[^:]+:/, "") - .replace(/^[a-z][a-z0-9+.-]*:\/\/[^/]*\//i, "") - .split("@")[0] + const trimmed = source.trim() + const isRepository = + /^[a-z][a-z0-9+.-]*:\/\//i.test(trimmed) || + /^[^/@\\]+@[^:/\\]+:/.test(trimmed) || + /^[^/@\\]+\.[^/@\\]+\//.test(trimmed) + const repository = isRepository + ? trimmed + .replace(/^[^/@]+@[^:]+:/, "") + .replace(/^[a-z][a-z0-9+.-]*:\/\/[^/]*\//i, "") + .split("@")[0] + : trimmed const leaf = repository.split(/[\\/]/).pop() ?? "" return ( leaf - .replace(/@.*$/, "") .replace(/\.git$/i, "") .toLowerCase() .replace(/[^a-z0-9-]+/g, "-") From 089c401586de79a5b418461a6b545f471c414db3 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 20 Oct 2026 22:31:21 -0600 Subject: [PATCH 06/16] fix(workspace): recover interrupted detached launches --- cmd/workspace/up/detach.go | 118 +++++++-- cmd/workspace/up/launch_handoff_test.go | 60 +++++ pkg/command/background.go | 33 ++- pkg/command/launch_unix.go | 90 +++++++ pkg/command/launch_unix_test.go | 72 ++++++ pkg/command/launch_windows.go | 272 ++++++++++++++++++++ pkg/command/launch_windows_test.go | 65 +++++ pkg/task/launch.go | 305 ++++++++++++++++++++++ pkg/task/launch_test.go | 324 ++++++++++++++++++++++++ pkg/task/store.go | 67 +++-- pkg/task/task.go | 62 ++--- 11 files changed, 1386 insertions(+), 82 deletions(-) create mode 100644 cmd/workspace/up/launch_handoff_test.go create mode 100644 pkg/command/launch_unix.go create mode 100644 pkg/command/launch_unix_test.go create mode 100644 pkg/command/launch_windows.go create mode 100644 pkg/command/launch_windows_test.go create mode 100644 pkg/task/launch.go create mode 100644 pkg/task/launch_test.go diff --git a/cmd/workspace/up/detach.go b/cmd/workspace/up/detach.go index 430bab878b..9a35753ced 100644 --- a/cmd/workspace/up/detach.go +++ b/cmd/workspace/up/detach.go @@ -1,6 +1,7 @@ package up import ( + "context" "errors" "fmt" "os" @@ -36,7 +37,7 @@ func (cmd *UpCmd) runDetached(args []string, cfg *config.Config) error { if err != nil { return err } - t, err := store.Create(task.CreateOptions{ + t, err := store.CreateLaunch(task.CreateOptions{ Command: "up", WorkspaceID: workspaceID, }) @@ -44,11 +45,8 @@ func (cmd *UpCmd) runDetached(args []string, cfg *config.Config) error { return err } - if err := t.BeginLaunch(); err != nil { - return errors.Join(fmt.Errorf("prepare detached up: %w", err), t.Fail(err)) - } if err := launchDetached(t, cmd.detachedInvocationEnvironment()); err != nil { - return errors.Join(fmt.Errorf("launch detached up: %w", err), t.FinishLaunch(), t.Fail(err)) + return errors.Join(fmt.Errorf("launch detached up: %w", err), t.FailLaunch(err)) } return cmd.renderDetachedTask(t) } @@ -85,9 +83,17 @@ func launchDetached(t *task.Task, invocationEnv []string) error { args := append(detachedArgs(os.Args[1:]), names.Flag(names.TaskID), t.ID()) return command.StartSupervisedBackground(command.SupervisedStartOptions{ - Name: task.WorkerProcessName(t.ID()), + Name: task.WorkerProcessName(t.ID()), + Lease: t.LaunchLease(), + AtomicOwnership: true, + OnPrepared: t.PrepareProcessOwnership, OnStarted: func(ref command.ProcessRef) error { - return t.SetProcess(ref) + if err := t.PublishProcess(ref); err != nil { + return err + } + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + return t.CommitLaunch(ctx) }, }, func() (*exec.Cmd, error) { return &exec.Cmd{ @@ -151,37 +157,105 @@ func (cmd *UpCmd) openTask() (*task.Task, error) { if err := t.HoldWorkerLock(); err != nil { return nil, err } + if err := waitForTaskLaunch(t, store); err != nil { + return nil, finalizeWorkerTask(t, err) + } + return t, nil +} + +func waitForTaskLaunch(t *task.Task, store *task.Store) error { deadline := time.NewTimer(5 * time.Second) defer deadline.Stop() ticker := time.NewTicker(20 * time.Millisecond) defer ticker.Stop() + leaseRetained := false for { - state, err := store.Get(cmd.taskID) + state, err := readWorkerLaunchState(t, store) if err != nil { - return nil, errors.Join(err, t.ReleaseWorkerLock()) + return 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 + ready, err := advanceTaskLaunch(t, store, state, &leaseRetained) + if err != nil { + return err } - if !state.LaunchPending { - if _, ok := state.ProcessReference(); !ok { - err := errors.New("detached worker launch did not publish process metadata") - return nil, finalizeWorkerTask(t, err) - } - return t, nil + if ready { + return nil } select { case <-deadline.C: - err := errors.New("timed out waiting for detached worker launch metadata") - return nil, finalizeWorkerTask(t, err) + return errors.New("timed out waiting for detached worker launch metadata") case <-ticker.C: } } } +func readWorkerLaunchState(t *task.Task, store *task.Store) (*task.State, error) { + state, err := store.Get(t.ID()) + if err != nil { + return nil, err + } + if state.Status.Terminal() || state.CancelRequested { + return nil, task.ErrCanceled + } + return state, nil +} + +func advanceTaskLaunch( + t *task.Task, + store *task.Store, + state *task.State, + leaseRetained *bool, +) (bool, error) { + if !state.LaunchPending { + if _, ok := state.ProcessReference(); !ok { + return false, errors.New("detached worker launch did not publish process metadata") + } + return true, nil + } + if state.LaunchProtocol != 1 { + return false, nil + } + if err := retainWorkerLaunchLease(leaseRetained); err != nil { + return false, err + } + if err := checkWorkerLauncher(t, store); err != nil { + return false, err + } + if state.PID != 0 && !state.WorkerReady { + return false, t.WorkerLaunchReady(os.Getpid()) + } + return false, nil +} + +func retainWorkerLaunchLease(retained *bool) error { + if *retained { + return nil + } + if err := command.RetainLaunchLease(); err != nil { + return err + } + *retained = true + return nil +} + +func checkWorkerLauncher(t *task.Task, store *task.Store) error { + alive, err := t.LauncherAlive() + if err != nil { + return err + } + if alive { + return nil + } + latest, err := store.Get(t.ID()) + if err != nil { + return err + } + if latest.LaunchPending { + return errors.New("detached worker launcher exited before commit") + } + return nil +} + func succeedTask(t *task.Task, result *config2.Result) error { if t == nil { return nil diff --git a/cmd/workspace/up/launch_handoff_test.go b/cmd/workspace/up/launch_handoff_test.go new file mode 100644 index 0000000000..bb1b3e455d --- /dev/null +++ b/cmd/workspace/up/launch_handoff_test.go @@ -0,0 +1,60 @@ +package up + +import ( + "context" + "os" + "os/exec" + "testing" + "time" + + "github.com/devsy-org/devsy/pkg/command" + "github.com/devsy-org/devsy/pkg/task" + "github.com/stretchr/testify/require" +) + +func TestDetachedWorkerCommitsBeforeWorkspaceWork(t *testing.T) { + store := isolatedTaskStore(t) + tk, err := store.CreateLaunch(task.CreateOptions{Command: "up"}) + require.NoError(t, err) + err = command.StartSupervisedBackground(command.SupervisedStartOptions{ + Name: task.WorkerProcessName(tk.ID()), Lease: tk.LaunchLease(), AtomicOwnership: true, + OnPrepared: tk.PrepareProcessOwnership, + OnStarted: func(ref command.ProcessRef) error { + if err := tk.PublishProcess(ref); err != nil { + return err + } + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + return tk.CommitLaunch(ctx) + }, + }, func() (*exec.Cmd, error) { + child := exec.Command( + os.Args[0], + "-test.run=^TestDetachedWorkerHandoffHelper$", + ) // #nosec G204 G702 -- re-exec this test binary + child.Env = append(os.Environ(), "DEVSY_TEST_HANDOFF_ID="+tk.ID()) + return child, nil + }) + require.NoError(t, err) + t.Cleanup(func() { _ = tk.Cancel() }) + require.Eventually(t, func() bool { + state, err := store.Get(tk.ID()) + return err == nil && state.Status == task.StatusSucceeded + }, 10*time.Second, 20*time.Millisecond) + state, err := store.Get(tk.ID()) + require.NoError(t, err) + require.True(t, state.WorkerReady) + require.False(t, state.LaunchPending) +} + +func TestDetachedWorkerHandoffHelper(t *testing.T) { + id := os.Getenv("DEVSY_TEST_HANDOFF_ID") + if id == "" { + return + } + cmd := &UpCmd{taskID: id} + tk, err := cmd.openTask() + require.NoError(t, err) + require.NoError(t, tk.Succeed(nil)) + require.NoError(t, tk.ReleaseWorkerLock()) +} diff --git a/pkg/command/background.go b/pkg/command/background.go index bd40a55b59..a1d7ed9b9c 100644 --- a/pkg/command/background.go +++ b/pkg/command/background.go @@ -14,8 +14,11 @@ import ( type CreateCommand func() (*exec.Cmd, error) type SupervisedStartOptions struct { - Name string - OnStarted func(ProcessRef) error + Name string + OnStarted func(ProcessRef) error + Lease *LaunchLease + AtomicOwnership bool + OnPrepared func(string) error } // StartBackgroundOnce starts a background process, ensuring only one instance @@ -160,18 +163,22 @@ func startSupervisedDetached(cmd *exec.Cmd, opts SupervisedStartOptions, streams if err != nil { return err } - prepareBackgroundTree(cmd) - if err := cmd.Start(); err != nil { + if opts.Lease != nil { + opts.Lease.attach(cmd) + } + release, err := startOwnedBackground(cmd, opts.Name, opts.AtomicOwnership, opts.OnPrepared) + if err != nil { closeFile(streamsF) return fmt.Errorf("start process: %w", err) } + defer release() closeFile(streamsF) pid := cmd.Process.Pid cleanup := func() { _ = abortSupervisedLaunch(pid, opts.Name) _ = cmd.Wait() } - if err := ownProcessTree(pid, opts.Name); err != nil { + if err := opts.ownTree(pid); err != nil { cleanup() return fmt.Errorf("assign process tree ownership for pid %d: %w", pid, err) } @@ -185,7 +192,7 @@ func startSupervisedDetached(cmd *exec.Cmd, opts SupervisedStartOptions, streams cleanup() return fmt.Errorf("publish process reference for pid %d: %w", pid, err) } - if err := resumeBackgroundTree(pid); err != nil { + if err := opts.resumeTree(pid); err != nil { _ = terminateProcessRef(ref) cleanup() return fmt.Errorf("resume process %d: %w", pid, err) @@ -194,6 +201,20 @@ func startSupervisedDetached(cmd *exec.Cmd, opts SupervisedStartOptions, streams return nil } +func (o SupervisedStartOptions) ownTree(pid int) error { + if o.AtomicOwnership { + return nil + } + return ownProcessTree(pid, o.Name) +} + +func (o SupervisedStartOptions) resumeTree(pid int) error { + if o.AtomicOwnership { + return nil + } + return resumeBackgroundTree(pid) +} + func isProcessRunning(pidFile string) (bool, error) { pid, err := os.ReadFile(pidFile) // #nosec G304: not user input if err != nil { diff --git a/pkg/command/launch_unix.go b/pkg/command/launch_unix.go new file mode 100644 index 0000000000..54918ce14b --- /dev/null +++ b/pkg/command/launch_unix.go @@ -0,0 +1,90 @@ +//go:build linux || darwin || unix + +package command + +import ( + "errors" + "fmt" + "os" + "os/exec" + "strconv" + + "golang.org/x/sys/unix" +) + +const launchLeaseEnv = "DEVSY_INTERNAL_LAUNCH_LEASE_FD" + +// LaunchLease survives launcher death until the worker exits, including the +// interval before its process reference has been published. +type LaunchLease struct{ file *os.File } + +func NewLaunchLease(path string) (*LaunchLease, error) { + f, err := os.OpenFile( + path, + os.O_CREATE|os.O_RDWR, + 0o600, + ) // #nosec G304 -- validated task-store lease path + if err != nil { + return nil, err + } + if err := unix.Flock(int(f.Fd()), unix.LOCK_EX|unix.LOCK_NB); err != nil { + _ = f.Close() + return nil, err + } + return &LaunchLease{file: f}, nil +} + +func (l *LaunchLease) Close() error { return l.file.Close() } + +func (l *LaunchLease) attach(cmd *exec.Cmd) { + fd := 3 + len(cmd.ExtraFiles) + cmd.ExtraFiles = append(cmd.ExtraFiles, l.file) + if cmd.Env == nil { + cmd.Env = os.Environ() + } + cmd.Env = append(cmd.Env, launchLeaseEnv+"="+strconv.Itoa(fd)) +} + +// RetainLaunchLease keeps the inherited claim until process exit, without +// passing it on to commands the worker executes. +func RetainLaunchLease() error { + value := os.Getenv(launchLeaseEnv) + if value == "" { + return errors.New("detached worker launch lease is missing") + } + fd, err := strconv.Atoi(value) + if err != nil || fd < 3 { + return errors.New("invalid detached worker launch lease") + } + var stat unix.Stat_t + if err := unix.Fstat(fd, &stat); err != nil { + return err + } + unix.CloseOnExec(fd) + return os.Unsetenv(launchLeaseEnv) +} + +// CleanupUnpublishedLaunch cannot signal an unidentified process. An available +// lifetime lease proves neither the launcher nor its bootstrap child remains. +func CleanupUnpublishedLaunch(path, _ string, _ bool) error { + lease, err := NewLaunchLease(path) + if err != nil { + return fmt.Errorf("detached launch still owns its lifetime lease: %w", err) + } + return lease.Close() +} + +func startOwnedBackground( + cmd *exec.Cmd, + _ string, + _ bool, + prepared func(string) error, +) (func(), error) { + prepareBackgroundTree(cmd) + if prepared != nil { + if err := prepared(""); err != nil { + return nil, err + } + } + return func() {}, cmd.Start() +} diff --git a/pkg/command/launch_unix_test.go b/pkg/command/launch_unix_test.go new file mode 100644 index 0000000000..12c905b23b --- /dev/null +++ b/pkg/command/launch_unix_test.go @@ -0,0 +1,72 @@ +//go:build linux || darwin || unix + +package command + +import ( + "bufio" + "fmt" + "io" + "os" + "os/exec" + "path/filepath" + "strconv" + "testing" + "time" + + "github.com/stretchr/testify/require" + "golang.org/x/sys/unix" +) + +func TestLaunchLeaseSurvivesLauncherCloseUntilChildExit(t *testing.T) { + path := filepath.Join(t.TempDir(), "lease") + lease, err := NewLaunchLease(path) + require.NoError(t, err) + t.Cleanup(func() { _ = lease.Close() }) + child := exec.Command( + os.Args[0], + "-test.run=^TestLaunchLeaseHelper$", + ) // #nosec G204 G702 -- re-exec this test binary + child.Env = append(os.Environ(), "DEVSY_TEST_LAUNCH_LEASE=1") + lease.attach(child) + input, err := child.StdinPipe() + require.NoError(t, err) + output, err := child.StdoutPipe() + require.NoError(t, err) + child.Stderr = os.Stderr + require.NoError(t, child.Start()) + t.Cleanup(func() { _ = input.Close(); _ = child.Process.Kill() }) + ready := make(chan bool, 1) + go func() { scanner := bufio.NewScanner(output); ready <- scanner.Scan() && scanner.Text() == "ready" }() + select { + case ok := <-ready: + require.True(t, ok) + case <-time.After(5 * time.Second): + t.Fatal("child did not retain the launch lease") + } + require.NoError(t, lease.Close()) + require.Error( + t, + CleanupUnpublishedLaunch(path, "", true), + "the unidentified child must retain positive lifetime evidence", + ) + require.NoError(t, input.Close()) + require.NoError(t, child.Wait()) + require.NoError(t, CleanupUnpublishedLaunch(path, "", true)) +} + +func TestLaunchLeaseHelper(t *testing.T) { + if os.Getenv("DEVSY_TEST_LAUNCH_LEASE") == "" { + return + } + fd, err := strconv.Atoi(os.Getenv(launchLeaseEnv)) + require.NoError(t, err) + require.NoError(t, RetainLaunchLease()) + flags, err := unix.FcntlInt(uintptr(fd), unix.F_GETFD, 0) + require.NoError(t, err) + require.NotZero(t, flags&unix.FD_CLOEXEC) + require.Empty(t, os.Getenv(launchLeaseEnv)) + _, err = fmt.Fprintln(os.Stdout, "ready") + require.NoError(t, err) + _, err = io.Copy(io.Discard, os.Stdin) + require.NoError(t, err) +} diff --git a/pkg/command/launch_windows.go b/pkg/command/launch_windows.go new file mode 100644 index 0000000000..5b55ae0172 --- /dev/null +++ b/pkg/command/launch_windows.go @@ -0,0 +1,272 @@ +//go:build windows + +package command + +import ( + "errors" + "fmt" + "os" + "os/exec" + "slices" + "strings" + "unsafe" + + "golang.org/x/sys/windows" +) + +// Windows uses a named Job Object as the launch lifetime claim. +type LaunchLease struct{} + +func NewLaunchLease(_ string) (*LaunchLease, error) { return &LaunchLease{}, nil } +func (*LaunchLease) Close() error { return nil } +func (*LaunchLease) attach(_ *exec.Cmd) {} +func RetainLaunchLease() error { return nil } + +func CleanupUnpublishedLaunch(_, name string, prepared bool) error { + if !prepared { + return nil + } + _, err := terminateNamedJob(name) + return err +} + +func startOwnedBackground( + cmd *exec.Cmd, + name string, + atomicOwnership bool, + prepared func(string) error, +) (func(), error) { + if !atomicOwnership { + prepareBackgroundTree(cmd) + return func() {}, cmd.Start() + } + return startInWorkerJob(cmd, name, prepared) +} + +// Assigning the job in CreateProcess closes the crash window between creating +// a suspended process and assigning it to its recoverable ownership object. +func startInWorkerJob( + cmd *exec.Cmd, + workerName string, + prepared func(string) error, +) (release func(), err error) { + if err := validateAtomicWorker(cmd); err != nil { + return nil, err + } + job, jobID, err := createLaunchJob(workerName) + if err != nil { + return nil, err + } + closeJob := func() { _ = windows.CloseHandle(job) } + defer func() { + if err != nil { + _, cleanupErr := terminateJobHandle(job, workerName) + err = errors.Join(err, cleanupErr) + closeJob() + } + }() + si, closeStartup, err := workerStartupInfo(cmd, job) + if err != nil { + return nil, err + } + defer closeStartup() + if err := notifyLaunchPrepared(prepared, jobID); err != nil { + return nil, err + } + pi, err := createOwnedWorker(cmd, si) + if err != nil { + return nil, err + } + defer func() { _ = windows.CloseHandle(pi.Thread); _ = windows.CloseHandle(pi.Process) }() + if err := retainWorkerJob(cmd, job, pi); err != nil { + return nil, err + } + return closeJob, nil +} + +func validateAtomicWorker(cmd *exec.Cmd) error { + if cmd.Err != nil { + return cmd.Err + } + if cmd.SysProcAttr != nil || cmd.Process != nil || cmd.Cancel != nil || cmd.WaitDelay != 0 { + return errors.New( + "atomic worker launch requires an unstarted command without process attributes or cancellation", + ) + } + return nil +} + +func createLaunchJob(workerName string) (windows.Handle, string, error) { + jobName, err := jobNameFor(workerName) + if err != nil { + return 0, "", err + } + name, err := windows.UTF16PtrFromString(jobName) + if err != nil { + return 0, "", err + } + job, err := createWorkerJobObject(name) + if err != nil { + return 0, "", err + } + 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 { + _ = windows.CloseHandle(job) + return 0, "", err + } + return job, jobName, nil +} + +func workerStartupInfo(cmd *exec.Cmd, job windows.Handle) (*windows.StartupInfoEx, func(), error) { + attributes, err := windows.NewProcThreadAttributeList(2) + if err != nil { + return nil, nil, err + } + const procThreadAttributeJobList = 0x0002000d + if err := attributes.Update( + procThreadAttributeJobList, + unsafe.Pointer(&job), + unsafe.Sizeof(job), + ); err != nil { + attributes.Delete() + return nil, nil, fmt.Errorf("configure creation-time worker job: %w", err) + } + handles, closeHandles, err := workerStandardHandles(cmd) + if err != nil { + attributes.Delete() + return nil, nil, err + } + cleanup := func() { closeHandles(); attributes.Delete() } + if err := attributes.Update(windows.PROC_THREAD_ATTRIBUTE_HANDLE_LIST, + unsafe.Pointer(&handles[0]), uintptr(len(handles))*unsafe.Sizeof(handles[0])); err != nil { + cleanup() + return nil, nil, err + } + si := &windows.StartupInfoEx{ + StartupInfo: windows.StartupInfo{ + Cb: uint32(unsafe.Sizeof(windows.StartupInfoEx{})), Flags: windows.STARTF_USESTDHANDLES, + StdInput: handles[0], StdOutput: handles[1], StdErr: handles[2], + }, + ProcThreadAttributeList: attributes.List(), + } + return si, cleanup, nil +} + +func notifyLaunchPrepared(prepared func(string) error, jobID string) error { + if prepared == nil { + return nil + } + return prepared(jobID) +} + +func createOwnedWorker( + cmd *exec.Cmd, + si *windows.StartupInfoEx, +) (*windows.ProcessInformation, error) { + application, err := windows.UTF16PtrFromString(cmd.Path) + if err != nil { + return nil, err + } + args := cmd.Args + if len(args) == 0 { + args = []string{cmd.Path} + } + commandLine, err := windows.UTF16PtrFromString(windows.ComposeCommandLine(args)) + if err != nil { + return nil, err + } + var directory *uint16 + if cmd.Dir != "" { + directory, err = windows.UTF16PtrFromString(cmd.Dir) + if err != nil { + return nil, err + } + } + block, err := workerEnvironment(cmd.Environ()) + if err != nil { + return nil, err + } + pi := new(windows.ProcessInformation) + err = windows.CreateProcess(application, commandLine, nil, nil, true, + windows.CREATE_UNICODE_ENVIRONMENT|windows.EXTENDED_STARTUPINFO_PRESENT, + &block[0], directory, &si.StartupInfo, pi) + if err != nil { + return nil, err + } + return pi, nil +} + +func retainWorkerJob(cmd *exec.Cmd, job windows.Handle, pi *windows.ProcessInformation) error { + // Until the worker holds this handle, launcher death closes the last handle + // and the kernel kills the child. Afterwards recovery can open the named job. + var childJob windows.Handle + if err := windows.DuplicateHandle(windows.CurrentProcess(), job, pi.Process, &childJob, + 0, false, windows.DUPLICATE_SAME_ACCESS); err != nil { + return err + } + var err error + cmd.Process, err = os.FindProcess(int(pi.ProcessId)) + return err +} + +func workerStandardHandles(cmd *exec.Cmd) ([]windows.Handle, func(), error) { + handles := make([]windows.Handle, 0, 3) + closeHandles := func() { + for _, h := range handles { + _ = windows.CloseHandle(h) + } + } + null, err := os.OpenFile(os.DevNull, os.O_RDWR, 0) + if err != nil { + return nil, closeHandles, err + } + defer func() { _ = null.Close() }() + for _, stream := range []any{cmd.Stdin, cmd.Stdout, cmd.Stderr} { + file := null + if stream != nil { + var ok bool + file, ok = stream.(*os.File) + if !ok { + closeHandles() + return nil, func() {}, errors.New("atomic worker streams must be files") + } + } + var inherited windows.Handle + if err := windows.DuplicateHandle( + windows.CurrentProcess(), + windows.Handle(file.Fd()), + windows.CurrentProcess(), + &inherited, + 0, + true, + windows.DUPLICATE_SAME_ACCESS, + ); err != nil { + closeHandles() + return nil, func() {}, err + } + handles = append(handles, inherited) + } + return handles, closeHandles, nil +} + +func workerEnvironment(env []string) ([]uint16, error) { + env = slices.Clone(env) + slices.SortFunc( + env, + func(a, b string) int { return strings.Compare(strings.ToUpper(a), strings.ToUpper(b)) }, + ) + var block []uint16 + for _, entry := range env { + encoded, err := windows.UTF16FromString(entry) + if err != nil { + return nil, err + } + block = append(block, encoded...) + } + if len(block) == 0 { + block = append(block, 0) + } + return append(block, 0), nil +} diff --git a/pkg/command/launch_windows_test.go b/pkg/command/launch_windows_test.go new file mode 100644 index 0000000000..a993070413 --- /dev/null +++ b/pkg/command/launch_windows_test.go @@ -0,0 +1,65 @@ +//go:build windows + +package command + +import ( + "os" + "os/exec" + "strconv" + "testing" + + "github.com/devsy-org/devsy/pkg/config" + "github.com/stretchr/testify/require" + "golang.org/x/sys/windows" +) + +func TestAtomicLaunchRejectsExistingJobWithoutClaimingIt(t *testing.T) { + t.Setenv(config.EnvHome, t.TempDir()) + const name = "devsy-launch-collision-test" + job, _, err := createLaunchJob(name) + require.NoError(t, err) + t.Cleanup(func() { _ = windows.CloseHandle(job) }) + worker := startHelper(t) + process, err := windows.OpenProcess( + windows.PROCESS_SET_QUOTA|windows.PROCESS_TERMINATE, + false, + uint32(worker.Process.Pid), + ) + require.NoError(t, err) + t.Cleanup(func() { _ = windows.CloseHandle(process) }) + require.NoError(t, windows.AssignProcessToJobObject(job, process)) + cmd := exec.Command( + os.Args[0], + "-test.run=^TestAtomicLaunchRejectsExistingJobWithoutClaimingIt$", + ) // #nosec G204 G702 -- re-exec this test binary + prepared := false + _, err = startInWorkerJob(cmd, name, func(string) error { prepared = true; return nil }) + require.ErrorContains(t, err, "already exists") + require.False(t, prepared) + require.Nil(t, cmd.Process) + require.NoError(t, CleanupUnpublishedLaunch("", name, prepared)) + running, err := IsRunning(strconv.Itoa(worker.Process.Pid)) + require.NoError(t, err) + require.True(t, running, "a failed ownership claim must not terminate the existing job") +} + +func TestLaunchJobPublicationKeepsCreatedIdentity(t *testing.T) { + t.Setenv(config.EnvHome, t.TempDir()) + job, jobID, err := createLaunchJob("devsy-launch-publication-test") + require.NoError(t, err) + t.Cleanup(func() { _ = windows.CloseHandle(job) }) + t.Setenv(config.EnvHome, t.TempDir()) + var published string + require.NoError(t, notifyLaunchPrepared(func(id string) error { + published = id + return nil + }, jobID)) + owned, found, err := openNamedJob(published, published) + require.NoError(t, err) + require.True( + t, + found, + "publication must identify the created job after the home directory changes", + ) + t.Cleanup(func() { _ = windows.CloseHandle(owned) }) +} diff --git a/pkg/task/launch.go b/pkg/task/launch.go new file mode 100644 index 0000000000..cb7d6892e4 --- /dev/null +++ b/pkg/task/launch.go @@ -0,0 +1,305 @@ +package task + +import ( + "context" + "errors" + "fmt" + "time" + + "github.com/devsy-org/devsy/pkg/clierr" + "github.com/devsy-org/devsy/pkg/command" +) + +func (s *Store) launchLeasePath(id string) (string, error) { + path, err := s.path(id) + if err != nil { + return "", err + } + return path + ".launch.lease", nil +} + +func (t *Task) LaunchLease() *command.LaunchLease { return t.launchLease } + +func (t *Task) PrepareProcessOwnership(treeID string) error { + accepted := false + err := t.store.update(t.id, func(s *State) { + if !s.acceptsLaunch() || s.LaunchOwned { + return + } + s.LaunchOwned = true + s.LaunchTreeID = treeID + accepted = true + }) + if err != nil { + return err + } + if !accepted { + return ErrCanceled + } + return nil +} + +func (t *Task) FailLaunch(cause error) error { + failErr := t.recordFailure(cause, true) + releaseErr := releaseLauncherLock(t) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + _, cleanupErr := t.store.reconcileLaunch(ctx, t.id, true) + return errors.Join(failErr, releaseErr, cleanupErr) +} + +// PublishProcess retains launcher ownership until the worker acknowledges the +// matching process reference and commit authorizes workspace work. +func (t *Task) PublishProcess(ref command.ProcessRef) error { + if ref.PID <= 0 || ref.Identity == "" { + return errors.New("worker launch requires a strong process reference") + } + accepted := false + err := t.store.update(t.id, func(s *State) { + if !s.acceptsLaunch() || s.Process != nil { + return + } + s.Process = &ref + s.PID = ref.PID + s.ProcessTreeIdentity = ref.Identity + accepted = true + }) + if err != nil { + return err + } + if !accepted { + return ErrCanceled + } + return nil +} + +func (t *Task) WorkerLaunchReady(pid int) error { + identity, err := command.ProcessTreeIdentity(pid) + if err != nil { + return err + } + accepted := false + err = t.store.update(t.id, func(s *State) { + if !s.acceptsLaunch() || s.PID != pid || identity == "" || + s.ProcessTreeIdentity != identity { + return + } + s.WorkerReady = true + accepted = true + }) + if err != nil { + return err + } + if !accepted { + return ErrCanceled + } + return nil +} + +func (t *Task) CommitLaunch(ctx context.Context) error { + ticker := time.NewTicker(20 * time.Millisecond) + defer ticker.Stop() + for { + if err := ctx.Err(); err != nil { + return err + } + committed, err := t.commitLaunchStep() + if err != nil { + return err + } + if committed { + return releaseLauncherLock(t) + } + select { + case <-ctx.Done(): + return ctx.Err() + case <-ticker.C: + } + } +} + +func (t *Task) LauncherAlive() (bool, error) { + lock, available, err := t.store.tryLauncherLock(t.id) + if err != nil { + return false, err + } + if available { + return false, lock.Unlock() + } + return true, nil +} + +func (s *Store) reconcilePendingLaunch(ctx context.Context, id string) (*State, error) { + return s.reconcileLaunch(ctx, id, false) +} + +func (s *Store) reconcileLaunch(ctx context.Context, id string, aborting bool) (*State, error) { + lock, available, err := s.tryLauncherLock(id) + if err != nil { + return nil, err + } + if !available { + return s.Get(id) + } + defer func() { _ = lock.Unlock() }() + state, err := s.Get(id) + if err != nil || (!state.LaunchPending && !aborting) { + return state, err + } + if state.ProcessCleanupComplete { + return state, nil + } + if err := s.cleanupPendingLaunch(ctx, state); err != nil { + return state, err + } + return s.finishAbandonedLaunch(id, aborting) +} + +func (s *Store) cleanupPendingLaunch(ctx context.Context, state *State) error { + if err := ctx.Err(); err != nil { + return err + } + if state.LaunchProtocol != 1 { + return errors.New("cannot verify cleanup of a legacy abandoned launch") + } + if ref, ok := state.ProcessReference(); ok { + if err := s.processController.Terminate(ref); err != nil { + return err + } + if err := s.processController.CleanupAfterExit(ref); err != nil { + return err + } + } + path, err := s.launchLeasePath(state.ID) + if err != nil { + return err + } + if err := command.CleanupUnpublishedLaunch( + path, + state.LaunchTreeID, + state.LaunchOwned, + ); err != nil { + return fmt.Errorf("clean up abandoned launch %s: %w", state.ID, err) + } + return nil +} + +func (s *Store) finishAbandonedLaunch(id string, aborting bool) (*State, error) { + err := s.update(id, func(current *State) { + if !current.LaunchPending && !aborting { + return + } + current.LaunchPending = false + current.ProcessCleanupComplete = true + if current.Status.Terminal() { + return + } + if current.CancelRequested { + markCanceled(current) + return + } + failure := clierr.Classify(ErrAbandoned) + current.Status = StatusFailed + current.Error = failure.Message + current.ErrorCode = string(failure.Code) + current.ErrorHint = failure.Hint + }) + if err != nil { + return nil, err + } + return s.Get(id) +} + +func (s *State) acceptsLaunch() bool { + return s.LaunchProtocol == 1 && s.LaunchPending && !s.Status.Terminal() && !s.CancelRequested +} + +func (s *State) hasPendingLaunch() bool { return s != nil && s.LaunchPending } + +func (s *State) markLaunchCleanupPending() { + if s.LaunchProtocol == 1 && !s.ProcessCleanupComplete { + s.LaunchPending = true + } +} + +func (t *Task) commitLaunchStep() (bool, error) { + committed, rejected := false, false + err := t.store.update(t.id, func(s *State) { + if !s.acceptsLaunch() || s.Process == nil { + rejected = true + return + } + if !s.WorkerReady { + return + } + s.LaunchPending = false + committed = true + }) + if err != nil { + return false, err + } + if rejected { + return false, ErrCanceled + } + return committed, nil +} + +func (t *Task) claimLaunchOwnership() 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) + } + leasePath, err := t.store.launchLeasePath(t.id) + if err != nil { + _ = launcherLock.Unlock() + return err + } + lease, err := command.NewLaunchLease(leasePath) + if err != nil { + _ = launcherLock.Unlock() + return err + } + t.launcherLock = launcherLock + t.launchLease = lease + return nil +} + +func (s *State) checkDeletion(force bool) error { + if s.LaunchPending || s.NeedsExitedWorkerCleanup() || s.NeedsCanceledWorkerCleanup() { + return fmt.Errorf("task %s still needs process tree cleanup", s.ID) + } + if !force && !s.Status.Terminal() { + return fmt.Errorf( + "task %s is still %s; cancel it first or delete with force", + s.ID, + s.Status, + ) + } + return nil +} + +func (s *Store) stateAfterLaunchCleanup(id string) (*State, error) { + state, err := s.Get(id) + if err != nil || !state.LaunchPending { + return state, err + } + state, err = s.reconcilePendingLaunch(context.Background(), id) + if err != nil { + return nil, err + } + if state.LaunchPending { + return nil, fmt.Errorf("task %s launch is still owned", id) + } + return state, nil +} + +func (s *Store) pendingLaunchState(state *State) *State { + current, err := s.reconcilePendingLaunch(context.Background(), state.ID) + if err != nil { + return state + } + return current +} diff --git a/pkg/task/launch_test.go b/pkg/task/launch_test.go new file mode 100644 index 0000000000..026f4d253f --- /dev/null +++ b/pkg/task/launch_test.go @@ -0,0 +1,324 @@ +package task + +import ( + "bufio" + "context" + "errors" + "fmt" + "io" + "os" + "os/exec" + "path/filepath" + "testing" + "time" + + "github.com/devsy-org/devsy/pkg/command" + "github.com/devsy-org/devsy/pkg/config" + "github.com/stretchr/testify/require" +) + +func TestReconcileLaunchAbandonedBeforeSpawn(t *testing.T) { + store := newTestStore(t) + tk, err := store.Create(CreateOptions{}) + require.NoError(t, err) + require.NoError(t, tk.BeginLaunch()) + require.NoError(t, releaseLauncherLock(tk)) + state, err := store.ReconcileTask(context.Background(), tk.ID()) + require.NoError(t, err) + require.Equal(t, StatusFailed, state.Status) + require.False(t, state.LaunchPending) + require.True(t, state.ProcessCleanupComplete) +} + +func TestLaunchCommitRequiresReadinessAndPreservesCancellation(t *testing.T) { + store := newTestStore(t) + tk, err := store.CreateLaunch(CreateOptions{}) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, releaseLauncherLock(tk)) }) + identity, err := command.ProcessTreeIdentity(os.Getpid()) + require.NoError(t, err) + ref := command.ProcessRef{PID: os.Getpid(), Identity: identity} + require.NoError(t, tk.PublishProcess(ref)) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + require.ErrorIs(t, tk.CommitLaunch(ctx), context.Canceled) + state, err := store.Get(tk.ID()) + require.NoError(t, err) + require.True(t, state.LaunchPending) + require.ErrorIs(t, tk.WorkerLaunchReady(os.Getpid()+1), ErrCanceled) + require.NoError(t, tk.WorkerLaunchReady(os.Getpid())) + _, err = tk.requestCancel() + require.NoError(t, err) + require.ErrorIs(t, tk.CommitLaunch(context.Background()), ErrCanceled) + state, err = store.Get(tk.ID()) + require.NoError(t, err) + require.True(t, state.LaunchPending) + require.True(t, state.CancelRequested) + require.NoError(t, releaseLauncherLock(tk)) + store.processController = fakeProcessController{} + state, err = store.ReconcileTask(context.Background(), tk.ID()) + require.NoError(t, err) + require.True(t, state.Canceled()) + require.True(t, state.ProcessCleanupComplete) +} + +func TestLegacyPendingLaunchCannotClaimCleanup(t *testing.T) { + store := newTestStore(t) + tk, err := store.Create(CreateOptions{}) + require.NoError(t, err) + require.NoError(t, store.update(tk.ID(), func(s *State) { s.LaunchPending = true })) + state, err := store.ReconcileTask(context.Background(), tk.ID()) + require.ErrorContains(t, err, "legacy abandoned launch") + require.True(t, state.LaunchPending) + require.False(t, state.ProcessCleanupComplete) + require.Error(t, tk.Cancel()) + state, err = store.Get(tk.ID()) + require.NoError(t, err) + require.True(t, state.LaunchPending) + require.False(t, state.ProcessCleanupComplete) +} + +func TestFailedLaunchAfterCommitStillCleansTheWorkerTree(t *testing.T) { + store := newTestStore(t) + tk, err := store.CreateLaunch(CreateOptions{}) + require.NoError(t, err) + identity, err := command.ProcessTreeIdentity(os.Getpid()) + require.NoError(t, err) + require.NoError(t, tk.PublishProcess(command.ProcessRef{PID: os.Getpid(), Identity: identity})) + require.NoError(t, tk.WorkerLaunchReady(os.Getpid())) + require.NoError(t, tk.CommitLaunch(context.Background())) + terminationCalls := 0 + store.processController = fakeProcessController{ + terminate: func(command.ProcessRef) error { terminationCalls++; return nil }, + } + failure := errors.New("launch handoff could not release ownership") + require.NoError(t, tk.FailLaunch(failure)) + state, err := store.Get(tk.ID()) + require.NoError(t, err) + require.Equal(t, 1, terminationCalls) + require.True(t, state.ProcessCleanupComplete) + require.False(t, state.LaunchPending) + require.Equal(t, StatusFailed, state.Status) + require.Contains(t, state.Error, failure.Error()) + require.NoError(t, tk.FailLaunch(failure)) + require.Equal( + t, + 1, + terminationCalls, + "verified cleanup must not signal the saved reference again", + ) +} + +func TestCommittedLaunchCleanupFailureCanBeRetried(t *testing.T) { + store := newTestStore(t) + tk, err := store.CreateLaunch(CreateOptions{}) + require.NoError(t, err) + identity, err := command.ProcessTreeIdentity(os.Getpid()) + require.NoError(t, err) + require.NoError(t, tk.PublishProcess(command.ProcessRef{PID: os.Getpid(), Identity: identity})) + require.NoError(t, tk.WorkerLaunchReady(os.Getpid())) + require.NoError(t, tk.CommitLaunch(context.Background())) + cleanupFailure := errors.New("process cleanup unavailable") + store.processController = fakeProcessController{ + terminate: func(command.ProcessRef) error { return cleanupFailure }, + } + require.ErrorIs(t, tk.FailLaunch(errors.New("handoff failed")), cleanupFailure) + state, err := store.Get(tk.ID()) + require.NoError(t, err) + require.True(t, state.LaunchPending) + require.False(t, state.ProcessCleanupComplete) + require.Error(t, store.Delete(tk.ID(), true)) + store.processController = fakeProcessController{} + state, err = store.ReconcileTask(context.Background(), tk.ID()) + require.NoError(t, err) + require.False(t, state.LaunchPending) + require.True(t, state.ProcessCleanupComplete) + require.Contains(t, state.Error, "handoff failed") +} + +func TestAbandonedLaunchCleanupFailureRemainsRetryable(t *testing.T) { + store := newTestStore(t) + tk, err := store.CreateLaunch(CreateOptions{}) + require.NoError(t, err) + require.NoError(t, tk.PublishProcess(command.ProcessRef{PID: 123, Identity: "fixture"})) + require.NoError(t, releaseLauncherLock(tk)) + cleanupFailure := errors.New("cannot verify cleanup") + store.processController = fakeProcessController{ + terminate: func(command.ProcessRef) error { return cleanupFailure }, + } + state, err := store.ReconcileTask(context.Background(), tk.ID()) + require.ErrorIs(t, err, cleanupFailure) + require.True(t, state.LaunchPending) + require.False(t, state.ProcessCleanupComplete) + require.False(t, state.Status.Terminal()) + require.Error(t, store.Delete(tk.ID(), true)) + store.processController = fakeProcessController{} + state, err = store.ReconcileTask(context.Background(), tk.ID()) + require.NoError(t, err) + require.False(t, state.LaunchPending) + require.True(t, state.ProcessCleanupComplete) + require.Equal(t, StatusFailed, state.Status) +} + +func TestLauncherCrashCannotAuthorizeWorkspaceWork(t *testing.T) { + for _, stage := range []string{"before-spawn", "before-publication", "after-publication", "after-ready"} { + t.Run(stage, func(t *testing.T) { + dir := t.TempDir() + launcher := exec.Command( + os.Args[0], + "-test.run=^TestLaunchCrashHelper$", + ) // #nosec G204 G702 -- re-exec this test binary + launcher.Env = append( + os.Environ(), + "DEVSY_LAUNCH_TEST=launcher", + "DEVSY_LAUNCH_STAGE="+stage, + config.EnvHome+"="+dir, + "DEVSY_LAUNCH_STORE="+filepath.Join(dir, "tasks"), + ) + stdout, err := launcher.StdoutPipe() + require.NoError(t, err) + stdin, err := launcher.StdinPipe() + require.NoError(t, err) + launcher.Stderr = os.Stderr + require.NoError(t, launcher.Start()) + t.Cleanup(func() { _ = stdin.Close(); _ = launcher.Process.Kill() }) + line := make(chan string, 1) + go func() { + scanner := bufio.NewScanner(stdout) + if scanner.Scan() { + line <- scanner.Text() + } else { + line <- "" + } + }() + var id string + select { + case id = <-line: + require.Len(t, id, 12) + case <-time.After(15 * time.Second): + t.Fatal("launcher did not reach crash barrier") + } + store, err := NewStoreAt(filepath.Join(dir, "tasks")) + require.NoError(t, err) + state, err := store.ReconcileTask(context.Background(), id) + require.NoError(t, err) + require.True(t, state.LaunchPending, "a live launcher still owns the handoff") + require.NoError(t, launcher.Process.Kill()) + require.Error(t, launcher.Wait()) + require.Eventually(t, func() bool { + state, err = store.ReconcileTask(context.Background(), id) + return err == nil && !state.LaunchPending && state.ProcessCleanupComplete + }, 15*time.Second, 20*time.Millisecond, "abandoned launch must reconcile with proven cleanup") + require.Equal(t, StatusFailed, state.Status) + _, err = os.Stat(filepath.Join(dir, "workspace-work-authorized")) + require.ErrorIs(t, err, os.ErrNotExist) + }) + } +} + +func TestLaunchCrashHelper(t *testing.T) { + role := os.Getenv("DEVSY_LAUNCH_TEST") + if role == "" { + return + } + config.ResetPathManager() + store, err := NewStoreAt(os.Getenv("DEVSY_LAUNCH_STORE")) + require.NoError(t, err) + if role == "worker" { + runLaunchCrashWorker(t, store) + return + } + tk, err := store.CreateLaunch(CreateOptions{}) + require.NoError(t, err) + barrier := func() { + _, err := fmt.Fprintln(os.Stdout, tk.ID()) + require.NoError(t, err) + _, _ = io.Copy(io.Discard, os.Stdin) + } + stage := os.Getenv("DEVSY_LAUNCH_STAGE") + if stage == "before-spawn" { + barrier() + return + } + err = command.StartSupervisedBackground(command.SupervisedStartOptions{ + Name: WorkerProcessName( + tk.ID(), + ), + Lease: tk.LaunchLease(), + AtomicOwnership: true, + OnPrepared: tk.PrepareProcessOwnership, + OnStarted: func(ref command.ProcessRef) error { + if stage != "before-publication" { + require.NoError(t, tk.PublishProcess(ref)) + } + if stage == "after-ready" { + require.Eventually( + t, + func() bool { state, err := store.Get(tk.ID()); return err == nil && state.WorkerReady }, + 5*time.Second, + 20*time.Millisecond, + ) + } + barrier() + return fmt.Errorf("test barrier released without commit") + }, + }, func() (*exec.Cmd, error) { + worker := exec.Command( + os.Args[0], + "-test.run=^TestLaunchCrashHelper$", + ) // #nosec G204 G702 -- re-exec this test binary + worker.Env = append(os.Environ(), "DEVSY_LAUNCH_TEST=worker", "DEVSY_LAUNCH_ID="+tk.ID()) + return worker, nil + }) + require.Error(t, err) +} + +func runLaunchCrashWorker(t *testing.T, store *Store) { + tk := store.Open(os.Getenv("DEVSY_LAUNCH_ID")) + require.NoError(t, command.RetainLaunchLease()) + require.NoError(t, tk.HoldWorkerLock()) + defer func() { require.NoError(t, tk.ReleaseWorkerLock()) }() + ticker := time.NewTicker(10 * time.Millisecond) + defer ticker.Stop() + deadline := time.NewTimer(10 * time.Second) + defer deadline.Stop() + for { + state, err := store.Get(tk.ID()) + require.NoError(t, err) + if !advanceCrashWorker(t, tk, state) { + return + } + select { + case <-ticker.C: + case <-deadline.C: + t.Fatal("worker bootstrap did not resolve") + } + } +} + +func advanceCrashWorker(t *testing.T, tk *Task, state *State) bool { + if state.Status.Terminal() || state.CancelRequested { + return false + } + if !state.LaunchPending { + require.NoError( + t, + // #nosec G703 -- the parent test supplies its private TempDir as DEVSY_HOME. + os.WriteFile( + filepath.Join(os.Getenv(config.EnvHome), "workspace-work-authorized"), + nil, + 0o600, + ), + ) + return false + } + alive, err := tk.LauncherAlive() + require.NoError(t, err) + if !alive { + return false + } + if state.PID != 0 { + require.NoError(t, tk.WorkerLaunchReady(os.Getpid())) + } + return true +} diff --git a/pkg/task/store.go b/pkg/task/store.go index cd5327c9f8..1556e504e9 100644 --- a/pkg/task/store.go +++ b/pkg/task/store.go @@ -46,20 +46,12 @@ func NewStoreAt(dir string) (*Store, error) { } func (s *Store) Create(opts CreateOptions) (*Task, error) { - id := random.String(12) - now := time.Now() - state := &State{ - ID: id, - Command: opts.Command, - WorkspaceID: opts.WorkspaceID, - Status: StatusPending, - StartedAt: now, - UpdatedAt: now, - } - if err := s.write(state); err != nil { - return nil, err - } - return &Task{store: s, id: id}, nil + return s.create(opts, false) +} + +// CreateLaunch acquires launch ownership before the task becomes visible. +func (s *Store) CreateLaunch(opts CreateOptions) (*Task, error) { + return s.create(opts, true) } // Open returns a handle without reading the task's state. @@ -89,16 +81,8 @@ func (s *Store) Delete(id string, force bool) error { if err != nil { return err } - if state.NeedsExitedWorkerCleanup() || state.NeedsCanceledWorkerCleanup() { - return fmt.Errorf("task %s still needs process tree cleanup", id) - } - if !force { - if !state.Status.Terminal() { - return fmt.Errorf( - "task %s is still %s; cancel it first or delete with force", - id, state.Status, - ) - } + if err := state.checkDeletion(force); err != nil { + return err } if err := os.Remove(path); err != nil { return fmt.Errorf("delete task %s: %w", id, err) @@ -146,6 +130,9 @@ func (s *Store) ReconcileTask(ctx context.Context, id string) (*State, error) { if err != nil { return nil, err } + if current.LaunchPending { + return s.reconcilePendingLaunch(ctx, id) + } if !current.NeedsReconciliation() { return current, nil } @@ -164,6 +151,9 @@ func (s *Store) ReconcileTask(ctx context.Context, id string) (*State, error) { // 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 state.hasPendingLaunch() { + return s.pendingLaunchState(state) + } if !state.NeedsReconciliation() { return state } @@ -193,7 +183,7 @@ func (s *Store) ReconcileState(state *State) *State { // 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) + state, err := s.stateAfterLaunchCleanup(id) if err != nil { return err } @@ -681,3 +671,30 @@ func syncDir(dir string) { defer func() { _ = d.Close() }() _ = d.Sync() } + +func (s *Store) create(opts CreateOptions, launch bool) (*Task, error) { + id := random.String(12) + t := &Task{store: s, id: id} + if launch { + if err := t.claimLaunchOwnership(); err != nil { + return nil, err + } + } + now := time.Now() + state := &State{ + ID: id, + Command: opts.Command, + WorkspaceID: opts.WorkspaceID, + Status: StatusPending, + StartedAt: now, + UpdatedAt: now, + } + if launch { + state.LaunchPending = true + state.LaunchProtocol = 1 + } + if err := s.write(state); err != nil { + return nil, errors.Join(err, releaseLauncherLock(t)) + } + return t, nil +} diff --git a/pkg/task/task.go b/pkg/task/task.go index c797a25cb3..4bc6da26e2 100644 --- a/pkg/task/task.go +++ b/pkg/task/task.go @@ -66,6 +66,10 @@ type State struct { ProcessCleanupComplete bool `json:"processCleanupComplete,omitempty"` CancelRequested bool `json:"cancelRequested,omitempty"` LaunchPending bool `json:"launchPending,omitempty"` + LaunchProtocol int `json:"launchProtocol,omitempty"` + WorkerReady bool `json:"workerReady,omitempty"` + LaunchTreeID string `json:"launchTreeId,omitempty"` + LaunchOwned bool `json:"launchOwned,omitempty"` StartedAt time.Time `json:"startedAt"` UpdatedAt time.Time `json:"updatedAt"` } @@ -85,6 +89,7 @@ type Task struct { // Held for the worker process's lifetime; see HoldWorkerLock. workerLock *flock.Flock launcherLock *flock.Flock + launchLease *command.LaunchLease } func (t *Task) ID() string { return t.id } @@ -141,30 +146,25 @@ func (s *State) NeedsReconciliation() bool { } 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) + if err := t.claimLaunchOwnership(); err != nil { + return err } var canceled bool - err = t.store.update(t.id, func(s *State) { - if s.Status.Terminal() || s.CancelRequested { + err := t.store.update(t.id, func(s *State) { + if s.Status.Terminal() || s.CancelRequested || s.LaunchProtocol != 0 || s.PID != 0 { canceled = true return } s.LaunchPending = true + s.LaunchProtocol = 1 + s.WorkerReady = false }) if err != nil { - _ = launcherLock.Unlock() - return err + return errors.Join(err, releaseLauncherLock(t)) } if canceled { - _ = launcherLock.Unlock() - return ErrCanceled + return errors.Join(ErrCanceled, releaseLauncherLock(t)) } - t.launcherLock = launcherLock return nil } @@ -237,15 +237,20 @@ func (t *Task) ReleaseWorkerLock() error { } func releaseLauncherLock(t *Task) error { + var leaseErr error + if t.launchLease != nil { + leaseErr = t.launchLease.Close() + t.launchLease = nil + } if t.launcherLock == nil { - return nil + return leaseErr } 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 + return leaseErr } // SetWorkspaceID corrects the task's workspace label to the resolved ID, @@ -470,23 +475,15 @@ func waitForLaunchStep(t *Task) (*State, bool, error) { } func clearAbandonedLaunch(t *Task) (bool, error) { - launcherLock, available, err := t.store.tryLauncherLock(t.id) + state, err := t.store.Get(t.id) if err != nil { - return false, fmt.Errorf("cancel task %s: inspect launcher lock: %w", t.id, err) + return false, 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) + if state.LaunchProtocol == 1 { + current, err := t.store.reconcilePendingLaunch(context.Background(), t.id) + return current != nil && !current.LaunchPending, err } - return true, nil + return false, errors.New("cannot verify cleanup of a legacy abandoned launch") } func waitForCancellation(ctx context.Context) error { @@ -511,6 +508,10 @@ func markCanceled(s *State) { // Fail preserves an existing terminal state, so the error a canceled worker // reports on its way out doesn't mask ErrCanceled as the reason it stopped. func (t *Task) Fail(err error) error { + return t.recordFailure(err, false) +} + +func (t *Task) recordFailure(err error, failedLaunch bool) error { message := "" var classified *clierr.CLIError if err != nil { @@ -527,6 +528,9 @@ func (t *Task) Fail(err error) error { } redactor := secrets.NewEnvironmentRedactor(os.Environ()) return t.store.update(t.id, func(s *State) { + if failedLaunch { + s.markLaunchCleanupPending() + } if s.Status.Terminal() || s.CancelRequested { return } From 3de19a7df95b23ff01e7bebfb7a1d6405330a5e3 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 20 Oct 2026 22:52:51 -0600 Subject: [PATCH 07/16] fix(task): preserve launch cleanup across quiescing and state reads --- pkg/task/read_unix.go | 9 +++++++++ pkg/task/read_windows.go | 22 ++++++++++++++++++++++ pkg/task/read_windows_test.go | 31 +++++++++++++++++++++++++++++++ pkg/task/store.go | 8 +++++++- pkg/workspace/quiesce.go | 2 +- pkg/workspace/quiesce_test.go | 15 +++++++++++++++ 6 files changed, 85 insertions(+), 2 deletions(-) create mode 100644 pkg/task/read_unix.go create mode 100644 pkg/task/read_windows.go create mode 100644 pkg/task/read_windows_test.go diff --git a/pkg/task/read_unix.go b/pkg/task/read_unix.go new file mode 100644 index 0000000000..4f887790dd --- /dev/null +++ b/pkg/task/read_unix.go @@ -0,0 +1,9 @@ +//go:build !windows + +package task + +import "os" + +func openTaskFile(path string) (*os.File, error) { + return os.Open(path) // #nosec G304 G703 -- path validated by Store.path +} diff --git a/pkg/task/read_windows.go b/pkg/task/read_windows.go new file mode 100644 index 0000000000..4eefeecdb2 --- /dev/null +++ b/pkg/task/read_windows.go @@ -0,0 +1,22 @@ +package task + +import ( + "os" + + "golang.org/x/sys/windows" +) + +func openTaskFile(path string) (*os.File, error) { + name, err := windows.UTF16PtrFromString(path) + if err != nil { + return nil, err + } + // Readers must allow the atomic state replacement while their handle is open. + handle, err := windows.CreateFile(name, windows.GENERIC_READ, + windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE, + nil, windows.OPEN_EXISTING, windows.FILE_ATTRIBUTE_NORMAL, 0) + if err != nil { + return nil, err + } + return os.NewFile(uintptr(handle), path), nil +} diff --git a/pkg/task/read_windows_test.go b/pkg/task/read_windows_test.go new file mode 100644 index 0000000000..816c105a7e --- /dev/null +++ b/pkg/task/read_windows_test.go @@ -0,0 +1,31 @@ +package task + +import ( + "io" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestTaskReaderAllowsAtomicStateReplacement(t *testing.T) { + store := newTestStore(t) + tk, err := store.Create(CreateOptions{}) + require.NoError(t, err) + path, err := store.path(tk.ID()) + require.NoError(t, err) + reader, err := openTaskFile(path) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, reader.Close()) }) + before, err := io.ReadAll(reader) + require.NoError(t, err) + + require.NoError(t, tk.Fail(ErrAbandoned)) + state, err := store.Get(tk.ID()) + require.NoError(t, err) + require.Equal(t, StatusFailed, state.Status) + _, err = reader.Seek(0, io.SeekStart) + require.NoError(t, err) + after, err := io.ReadAll(reader) + require.NoError(t, err) + require.Equal(t, before, after) +} diff --git a/pkg/task/store.go b/pkg/task/store.go index 1556e504e9..04c6dcb179 100644 --- a/pkg/task/store.go +++ b/pkg/task/store.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "io" "os" "path/filepath" "sort" @@ -96,7 +97,12 @@ func (s *Store) Get(id string) (*State, error) { if err != nil { return nil, err } - data, err := os.ReadFile(path) // #nosec G304 -- path validated by s.path + file, err := openTaskFile(path) + if err != nil { + return nil, fmt.Errorf("read task %s: %w", id, err) + } + defer func() { _ = file.Close() }() + data, err := io.ReadAll(file) if err != nil { return nil, fmt.Errorf("read task %s: %w", id, err) } diff --git a/pkg/workspace/quiesce.go b/pkg/workspace/quiesce.go index 7b4ad9a0cf..9393f451d3 100644 --- a/pkg/workspace/quiesce.go +++ b/pkg/workspace/quiesce.go @@ -41,7 +41,7 @@ func quiesceUpTasks( var errs []error handled := 0 for _, state := range states { - if state.Status.Terminal() && !state.NeedsExitedWorkerCleanup() && + if state.Status.Terminal() && !state.LaunchPending && !state.NeedsExitedWorkerCleanup() && !state.NeedsCanceledWorkerCleanup() { continue } diff --git a/pkg/workspace/quiesce_test.go b/pkg/workspace/quiesce_test.go index 2b07761214..aa9b6c5e1a 100644 --- a/pkg/workspace/quiesce_test.go +++ b/pkg/workspace/quiesce_test.go @@ -3,6 +3,8 @@ package workspace import ( "context" "errors" + "os" + "path/filepath" "runtime" "testing" @@ -160,3 +162,16 @@ func TestQuiesceUpTasksBoundsEachCancellation(t *testing.T) { ) require.NoError(t, err) } + +func TestQuiesceUpTasksRejectsUnverifiedTerminalLaunch(t *testing.T) { + dir := t.TempDir() + store, err := task.NewStoreAt(dir) + require.NoError(t, err) + require.NoError(t, os.WriteFile(filepath.Join(dir, "interrupted.json"), []byte( + `{"id":"interrupted","command":"up","workspaceId":"ws-1","status":"failed","launchPending":true}`, + ), 0o600)) + + err = QuiesceUpTasks(store, "ws-1") + require.ErrorContains(t, err, "cannot verify cleanup of a legacy abandoned launch") + assert.True(t, taskState(t, store, "interrupted").LaunchPending) +} From ef40901c0768f1e11101e08a65bb68e16e070178 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 20 Oct 2026 23:05:02 -0600 Subject: [PATCH 08/16] fix(task): replace Windows state while readers remain open --- pkg/task/store.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/task/store.go b/pkg/task/store.go index 04c6dcb179..99181711c5 100644 --- a/pkg/task/store.go +++ b/pkg/task/store.go @@ -15,6 +15,7 @@ import ( "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/random" "github.com/gofrs/flock" + "tailscale.com/atomicfile" ) // lockTimeout bounds how long update waits to acquire a task's file lock. @@ -661,7 +662,7 @@ func (s *Store) write(state *State) error { if err := tmp.Close(); err != nil { return fmt.Errorf("close temp task file: %w", err) } - if err := os.Rename(tmpPath, target); err != nil { + if err := atomicfile.Rename(tmpPath, target); err != nil { return fmt.Errorf("commit task state: %w", err) } syncDir(s.dir) From b257211471ad90c40952d6e75c5529b67564d935 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 21 Oct 2026 15:33:45 -0600 Subject: [PATCH 09/16] fix(task): synchronize state reads and recognize exited workers Serialize task reads across Windows atomic replacement and observe unreaped worker exit without weakening identity checks. Update the detached fixture to use the production handoff, preserve trailing-source basename suggestions, and prevent test capture pipe deadlocks. --- cmd/provider/list_test.go | 33 ++++++--- cmd/secrets/list_test.go | 32 ++++++--- cmd/workspace/helpers_test.go | 33 ++++++--- .../src/lib/utils/workspace-id.test.ts | 5 ++ .../renderer/src/lib/utils/workspace-id.ts | 2 +- e2e/tests/ssh/detached_validation.go | 54 +++++++++++++-- pkg/command/process_exit_test.go | 18 +++++ pkg/command/process_supported.go | 11 +-- pkg/task/read_windows_test.go | 12 ++++ pkg/task/store.go | 68 +++++++++++++------ pkg/task/store_test.go | 49 +++++++++++++ 11 files changed, 258 insertions(+), 59 deletions(-) diff --git a/cmd/provider/list_test.go b/cmd/provider/list_test.go index 8f83fe380c..7a5b972edf 100644 --- a/cmd/provider/list_test.go +++ b/cmd/provider/list_test.go @@ -5,6 +5,7 @@ import ( "errors" "io" "os" + "strings" "testing" "github.com/devsy-org/devsy/pkg/config" @@ -90,24 +91,36 @@ type providerListOutput struct { err error } -func captureProviderListOutput(t *testing.T, render func() error) providerListOutput { +func captureProviderListOutput(t *testing.T, render func() error) (output providerListOutput) { t.Helper() - read, write, err := os.Pipe() + capture, err := os.CreateTemp(t.TempDir(), "stdout") require.NoError(t, err) previous := os.Stdout - os.Stdout = write + os.Stdout = capture defer func() { os.Stdout = previous - _ = write.Close() - _ = read.Close() + output.err = errors.Join(output.err, capture.Close()) }() renderErr := render() - os.Stdout = previous - closeErr := write.Close() - data, readErr := io.ReadAll(read) - readCloseErr := read.Close() + _, seekErr := capture.Seek(0, io.SeekStart) + data, readErr := io.ReadAll(capture) return providerListOutput{ text: string(data), - err: errors.Join(renderErr, closeErr, readErr, readCloseErr), + err: errors.Join(renderErr, seekErr, readErr), } } + +func TestCaptureProviderListOutputLargeOutput(t *testing.T) { + want := strings.Repeat("0123456789abcdef", 1<<16) + renderErr := errors.New("render failed") + previous := os.Stdout + output := captureProviderListOutput(t, func() error { + n, err := io.WriteString(os.Stdout, want) + require.NoError(t, err) + require.Equal(t, len(want), n) + return renderErr + }) + require.Equal(t, want, output.text) + require.ErrorIs(t, output.err, renderErr) + require.Same(t, previous, os.Stdout) +} diff --git a/cmd/secrets/list_test.go b/cmd/secrets/list_test.go index a91fd8ce7f..bfb9d3f1c4 100644 --- a/cmd/secrets/list_test.go +++ b/cmd/secrets/list_test.go @@ -4,11 +4,13 @@ import ( "encoding/json" "io" "os" + "strings" "testing" "time" "github.com/devsy-org/devsy/pkg/config" devsysecrets "github.com/devsy-org/devsy/pkg/secrets" + "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" ) @@ -141,23 +143,37 @@ func (s *ListTestSuite) TestListEntriesReportsAvailabilityInJSONAndPlainOutput() func captureListOutput(t *testing.T, render func()) string { t.Helper() - read, write, err := os.Pipe() + capture, err := os.CreateTemp(t.TempDir(), "stdout") if err != nil { t.Fatal(err) } previous := os.Stdout - os.Stdout = write - defer func() { os.Stdout = previous }() + os.Stdout = capture + defer func() { + os.Stdout = previous + if err := capture.Close(); err != nil { + t.Error(err) + } + }() render() - if err := write.Close(); err != nil { + if _, err := capture.Seek(0, io.SeekStart); err != nil { t.Fatal(err) } - output, err := io.ReadAll(read) + output, err := io.ReadAll(capture) if err != nil { t.Fatal(err) } - if err := read.Close(); err != nil { - t.Fatal(err) - } return string(output) } + +func TestCaptureListOutputLargeOutput(t *testing.T) { + want := strings.Repeat("0123456789abcdef", 1<<16) + previous := os.Stdout + output := captureListOutput(t, func() { + n, err := io.WriteString(os.Stdout, want) + require.NoError(t, err) + require.Equal(t, len(want), n) + }) + require.Equal(t, want, output) + require.Same(t, previous, os.Stdout) +} diff --git a/cmd/workspace/helpers_test.go b/cmd/workspace/helpers_test.go index cb4f5a982a..bbbe674119 100644 --- a/cmd/workspace/helpers_test.go +++ b/cmd/workspace/helpers_test.go @@ -4,6 +4,7 @@ import ( "context" "io" "os" + "strings" "testing" "github.com/devsy-org/devsy/pkg/client" @@ -57,20 +58,36 @@ func (f *fakeWorkspaceClient) WorkspaceConfig() *provider.Workspace { return f.c func (f *fakeWorkspaceClient) Lock(context.Context) error { return nil } func (f *fakeWorkspaceClient) Unlock() {} -// captureStdout runs fn while redirecting os.Stdout to a pipe and returns -// everything written. +// captureStdout runs fn while redirecting os.Stdout to a temporary file and +// returns everything written. func captureStdout(t *testing.T, fn func()) string { t.Helper() - orig := os.Stdout - r, w, err := os.Pipe() + capture, err := os.CreateTemp(t.TempDir(), "stdout") require.NoError(t, err) - os.Stdout = w - defer func() { os.Stdout = orig }() + orig := os.Stdout + os.Stdout = capture + defer func() { + os.Stdout = orig + require.NoError(t, capture.Close()) + }() fn() - require.NoError(t, w.Close()) - out, err := io.ReadAll(r) + _, err = capture.Seek(0, io.SeekStart) + require.NoError(t, err) + out, err := io.ReadAll(capture) require.NoError(t, err) return string(out) } + +func TestCaptureStdoutLargeOutput(t *testing.T) { + want := strings.Repeat("0123456789abcdef", 1<<16) + orig := os.Stdout + got := captureStdout(t, func() { + n, err := io.WriteString(os.Stdout, want) + require.NoError(t, err) + require.Equal(t, len(want), n) + }) + require.Equal(t, want, got) + require.Same(t, orig, os.Stdout) +} diff --git a/desktop/src/renderer/src/lib/utils/workspace-id.test.ts b/desktop/src/renderer/src/lib/utils/workspace-id.test.ts index dd86219ee8..ec190d8b88 100644 --- a/desktop/src/renderer/src/lib/utils/workspace-id.test.ts +++ b/desktop/src/renderer/src/lib/utils/workspace-id.test.ts @@ -17,6 +17,11 @@ describe("workspace IDs", () => { ) it.each([ "/var/home/dev/Projects/Backend", + "/home/dev/Backend/", + "/home/dev/@work/Backend//", + "/home/first@corp/Backend/", + "C:\\Users\\dev\\Backend\\", + "C:\\Users\\first@corp\\Backend\\", "/home/dev/@work/Backend", "/home/first@corp/Backend", "C:\\Users\\first@corp\\Backend", diff --git a/desktop/src/renderer/src/lib/utils/workspace-id.ts b/desktop/src/renderer/src/lib/utils/workspace-id.ts index 8cf0314788..63386c1063 100644 --- a/desktop/src/renderer/src/lib/utils/workspace-id.ts +++ b/desktop/src/renderer/src/lib/utils/workspace-id.ts @@ -17,7 +17,7 @@ export function deriveWorkspaceId(source: string): string { .replace(/^[a-z][a-z0-9+.-]*:\/\/[^/]*\//i, "") .split("@")[0] : trimmed - const leaf = repository.split(/[\\/]/).pop() ?? "" + const leaf = repository.split(/[\\/]/).filter(Boolean).pop() ?? "" return ( leaf .replace(/\.git$/i, "") diff --git a/e2e/tests/ssh/detached_validation.go b/e2e/tests/ssh/detached_validation.go index aa9dc8a85d..e88b77f9a3 100644 --- a/e2e/tests/ssh/detached_validation.go +++ b/e2e/tests/ssh/detached_validation.go @@ -3,6 +3,7 @@ package ssh import ( "context" "encoding/json" + "errors" "os" "os/exec" "path/filepath" @@ -10,6 +11,8 @@ import ( "time" "github.com/devsy-org/devsy/e2e/framework" + "github.com/devsy-org/devsy/pkg/command" + "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/task" "github.com/devsy-org/devsy/pkg/workspace" "github.com/onsi/ginkgo/v2" @@ -159,12 +162,14 @@ var _ = ginkgo.Describe( home, err := os.MkdirTemp("", "devsy-worker-error-") framework.ExpectNoError(err) ginkgo.DeferCleanup(os.RemoveAll, home) + ginkgo.GinkgoT().Setenv(config.EnvHome, home) + config.ResetPathManager() + ginkgo.DeferCleanup(config.ResetPathManager) cli := detachedValidationCLI{framework: f, home: home} store, err := task.NewStoreAt(filepath.Join(home, "state", "tasks")) framework.ExpectNoError(err) - tk, err := store.Create(task.CreateOptions{Command: "up"}) + tk, err := store.CreateLaunch(task.CreateOptions{Command: "up"}) framework.ExpectNoError(err) - framework.ExpectNoError(tk.BeginLaunch()) expected := "prepare workspace client" args := []string{ cmdWorkspace, @@ -184,10 +189,12 @@ var _ = ginkgo.Describe( ) expected = "load devsy config" } - worker := cli.newCommand(ctx, args...) - framework.ExpectNoError(worker.Start()) - framework.ExpectNoError(tk.SetPID(worker.Process.Pid)) - gomega.Expect(worker.Wait()).To(gomega.HaveOccurred()) + ref, err := cli.launchWorker(ctx, tk, args...) + framework.ExpectNoError(err) + ginkgo.DeferCleanup(tk.Cancel) + gomega.Eventually(func() (bool, error) { + return command.ProcessExited(ref) + }).WithContext(ctx).WithTimeout(10 * time.Second).Should(gomega.BeTrue()) state, err := store.Get(tk.ID()) framework.ExpectNoError(err) gomega.Expect(state.Status).To(gomega.Equal(task.StatusFailed)) @@ -199,6 +206,41 @@ var _ = ginkgo.Describe( }, ) +func (cli detachedValidationCLI) launchWorker( + ctx context.Context, + tk *task.Task, + args ...string, +) (command.ProcessRef, error) { + var ref command.ProcessRef + err := command.StartSupervisedBackground(command.SupervisedStartOptions{ + Name: task.WorkerProcessName(tk.ID()), + Lease: tk.LaunchLease(), + AtomicOwnership: true, + OnPrepared: tk.PrepareProcessOwnership, + OnStarted: func(started command.ProcessRef) error { + ref = started + if err := tk.PublishProcess(started); err != nil { + return err + } + commitCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + return tk.CommitLaunch(commitCtx) + }, + }, func() (*exec.Cmd, error) { + // Atomic Windows launch requires a plain command with file streams. + // #nosec G204 -- repository-built CLI fixture with spec-controlled arguments. + worker := exec.Command( + filepath.Join(cli.framework.DevsyBinDir, cli.framework.DevsyBinName), + args...) + worker.Env = detachedValidationEnvironment(cli.home) + return worker, nil + }) + if err != nil { + return ref, errors.Join(err, tk.FailLaunch(err)) + } + return ref, nil +} + func (cli detachedValidationCLI) expectFailureEnvelope(ctx context.Context, id, diagnostic string) { followCtx, cancel := context.WithTimeout(ctx, 10*time.Second) defer cancel() diff --git a/pkg/command/process_exit_test.go b/pkg/command/process_exit_test.go index a741e57c5d..cecbfeef36 100644 --- a/pkg/command/process_exit_test.go +++ b/pkg/command/process_exit_test.go @@ -4,11 +4,29 @@ import ( "errors" "path/filepath" "testing" + "time" "github.com/devsy-org/devsy/pkg/config" "github.com/stretchr/testify/require" ) +func TestProcessExitedRecognizesUnreapedChild(t *testing.T) { + child := processExitTestChild() + require.NoError(t, child.Start()) + t.Cleanup(func() { _ = child.Process.Kill(); _ = child.Wait() }) + identity, err := ProcessTreeIdentity(child.Process.Pid) + require.NoError(t, err) + ref := ProcessRef{PID: child.Process.Pid, Identity: identity} + exited, err := ProcessExited(ref) + require.NoError(t, err) + require.False(t, exited) + require.NoError(t, child.Process.Kill()) + require.Eventually(t, func() bool { + exited, err := ProcessExited(ref) + return err == nil && exited + }, time.Second, time.Millisecond, "exit must be observable before the parent reaps the child") +} + func TestProcessExitedRequiresSavedWorkerIdentity(t *testing.T) { child := processExitTestChild() require.NoError(t, child.Start()) diff --git a/pkg/command/process_supported.go b/pkg/command/process_supported.go index 9df7e3d5c1..e7c1a246d6 100644 --- a/pkg/command/process_supported.go +++ b/pkg/command/process_supported.go @@ -425,16 +425,19 @@ func prepareBackgroundTree(cmd *exec.Cmd) { } func processRefExited(ref ProcessRef) (bool, error) { - current, err := processTreeIdentity(ref.PID) + live, err := processIsLive(ref.PID) if err != nil { return false, err } - if current == "" || current != ref.Identity { + if !live { return true, nil } - live, err := processIsLive(ref.PID) + current, err := processTreeIdentity(ref.PID) if err != nil { return false, err } - return !live, nil + if current == "" || current != ref.Identity { + return true, nil + } + return false, nil } diff --git a/pkg/task/read_windows_test.go b/pkg/task/read_windows_test.go index 816c105a7e..aedbfe1c10 100644 --- a/pkg/task/read_windows_test.go +++ b/pkg/task/read_windows_test.go @@ -5,8 +5,20 @@ import ( "testing" "github.com/stretchr/testify/require" + "golang.org/x/sys/windows" ) +func TestGetWaitsForWriterHoldingExclusiveFileHandle(t *testing.T) { + assertTaskReadWaitsForWriter(t, func(path string) func() { + name, err := windows.UTF16PtrFromString(path) + require.NoError(t, err) + handle, err := windows.CreateFile(name, windows.GENERIC_READ, 0, + nil, windows.OPEN_EXISTING, windows.FILE_ATTRIBUTE_NORMAL, 0) + require.NoError(t, err) + return func() { require.NoError(t, windows.CloseHandle(handle)) } + }) +} + func TestTaskReaderAllowsAtomicStateReplacement(t *testing.T) { store := newTestStore(t) tk, err := store.Create(CreateOptions{}) diff --git a/pkg/task/store.go b/pkg/task/store.go index 99181711c5..455de130a3 100644 --- a/pkg/task/store.go +++ b/pkg/task/store.go @@ -27,6 +27,7 @@ type Store struct { processController command.ProcessController afterClaimHook func() afterCancelLockClaimedHook func() + afterStateLockBlockedHook func() } func NewStore() (*Store, error) { @@ -79,7 +80,7 @@ func (s *Store) Delete(id string, force bool) error { // 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) + state, err := s.readState(id) if err != nil { return err } @@ -94,24 +95,13 @@ func (s *Store) Delete(id string, force bool) error { } func (s *Store) Get(id string) (*State, error) { - path, err := s.path(id) - if err != nil { - return nil, err - } - file, err := openTaskFile(path) - if err != nil { - return nil, fmt.Errorf("read task %s: %w", id, err) - } - defer func() { _ = file.Close() }() - data, err := io.ReadAll(file) - if err != nil { - return nil, fmt.Errorf("read task %s: %w", id, err) - } - state := &State{} - if err := json.Unmarshal(data, state); err != nil { - return nil, fmt.Errorf("parse task %s: %w", id, err) - } - return state, nil + var state *State + err := s.withLock(id, func() error { + var err error + state, err = s.readState(id) + return err + }) + return state, err } // Abandoned reports whether a non-terminal task's worker is gone. @@ -326,6 +316,29 @@ func (s *Store) WaitForWorkerObservation( } } +// The caller must hold the state lock: Windows replacement can briefly make +// the path unavailable, and lock-owning callbacks cannot acquire it again. +func (s *Store) readState(id string) (*State, error) { + path, err := s.path(id) + if err != nil { + return nil, err + } + file, err := openTaskFile(path) + if err != nil { + return nil, fmt.Errorf("read task %s: %w", id, err) + } + defer func() { _ = file.Close() }() + data, err := io.ReadAll(file) + if err != nil { + return nil, fmt.Errorf("read task %s: %w", id, err) + } + state := &State{} + if err := json.Unmarshal(data, state); err != nil { + return nil, fmt.Errorf("parse task %s: %w", id, err) + } + return state, nil +} + func (s *Store) markProcessCleanupComplete(id string) error { return s.update(id, func(state *State) { state.ProcessCleanupComplete = true @@ -589,7 +602,7 @@ func (s *Store) launcherLockPath(id string) (string, error) { func (s *Store) update(id string, mutate func(*State)) error { return s.withLock(id, func() error { - state, err := s.Get(id) + state, err := s.readState(id) if err != nil { return err } @@ -609,9 +622,20 @@ func (s *Store) withLock(id string, fn func() error) error { } lock := flock.New(path + ".lock") + locked, err := lock.TryLock() + if err != nil { + return fmt.Errorf("lock task %s: %w", id, err) + } + if locked { + defer func() { _ = lock.Unlock() }() + return fn() + } + if s.afterStateLockBlockedHook != nil { + s.afterStateLockBlockedHook() + } ctx, cancel := context.WithTimeout(context.Background(), lockTimeout) defer cancel() - locked, err := lock.TryLockContext(ctx, 20*time.Millisecond) + locked, err = lock.TryLockContext(ctx, 20*time.Millisecond) if err != nil { return fmt.Errorf("lock task %s: %w", id, err) } @@ -700,7 +724,7 @@ func (s *Store) create(opts CreateOptions, launch bool) (*Task, error) { state.LaunchPending = true state.LaunchProtocol = 1 } - if err := s.write(state); err != nil { + if err := s.withLock(id, func() error { return s.write(state) }); err != nil { return nil, errors.Join(err, releaseLauncherLock(t)) } return t, nil diff --git a/pkg/task/store_test.go b/pkg/task/store_test.go index 8e3de69fde..7ee010a1de 100644 --- a/pkg/task/store_test.go +++ b/pkg/task/store_test.go @@ -3,6 +3,8 @@ package task import ( "context" "errors" + "os" + "sync" "testing" "time" @@ -14,6 +16,53 @@ const workspaceOne = "ws-1" const testProcessIdentity = "sess-1" +func TestGetWaitsForWriterWhenRecordTemporarilyMissing(t *testing.T) { + assertTaskReadWaitsForWriter(t, func(path string) func() { + previous := path + ".previous" + require.NoError(t, os.Rename(path, previous)) + return func() { require.NoError(t, os.Rename(previous, path)) } + }) +} + +func assertTaskReadWaitsForWriter(t *testing.T, interruptRead func(string) func()) { + t.Helper() + writer := newTestStore(t) + reader, err := NewStoreAt(writer.dir) + require.NoError(t, err) + tk, err := writer.Create(CreateOptions{}) + require.NoError(t, err) + state, err := writer.Get(tk.ID()) + require.NoError(t, err) + path, err := writer.path(tk.ID()) + require.NoError(t, err) + blocked := make(chan struct{}) + reader.afterStateLockBlockedHook = func() { close(blocked) } + type readResult struct { + state *State + err error + } + read := make(chan readResult, 1) + require.NoError(t, writer.withLock(tk.ID(), func() error { + restore := sync.OnceFunc(interruptRead(path)) + defer restore() + go func() { + state, err := reader.Get(tk.ID()) + read <- readResult{state, err} + }() + select { + case <-blocked: + case result := <-read: + t.Fatalf("reader bypassed active writer: state=%+v error=%v", result.state, result.err) + } + restore() + state.Phase = "replacement-committed" + return writer.write(state) + })) + result := <-read + require.NoError(t, result.err) + require.Equal(t, "replacement-committed", result.state.Phase) +} + func createTaskWith(t *testing.T, store *Store, opts CreateOptions) *Task { t.Helper() tk, err := store.Create(opts) From b7caed6750ea0e8b87d6554fc96c8b68257f6ee3 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 21 Oct 2026 18:27:20 -0600 Subject: [PATCH 10/16] fix(desktop): preserve log-save warnings across status refreshes Keep log persistence errors separate from status refresh failures, retain an explicit unconfirmed state, and allow a new operation generation to retry. Add a real-process safety test showing why an exited legacy reference cannot authorize cleanup of an unpublished later attempt. --- .../main/__tests__/ipc-workspace-jobs.test.ts | 12 ++- desktop/src/main/__tests__/watcher.test.ts | 43 ++++++++ desktop/src/main/workspace-jobs.ts | 16 +-- .../workspace/WorkspaceLaunchProgress.svelte | 9 +- .../workspace/WorkspaceOperation.svelte | 3 + .../workspace/WorkspaceOperation.test.ts | 28 +++++ .../workspace/WorkspaceWizard.svelte | 12 ++- .../workspace/WorkspaceWizard.test.ts | 27 +++++ .../src/lib/stores/workspaces.test.ts | 44 ++++++++ .../src/renderer/src/lib/stores/workspaces.ts | 1 + desktop/src/shared/workspace-operation.ts | 14 ++- pkg/task/legacy_launch_test.go | 102 ++++++++++++++++++ 12 files changed, 294 insertions(+), 17 deletions(-) create mode 100644 pkg/task/legacy_launch_test.go diff --git a/desktop/src/main/__tests__/ipc-workspace-jobs.test.ts b/desktop/src/main/__tests__/ipc-workspace-jobs.test.ts index eabff8221b..785d6c1b38 100644 --- a/desktop/src/main/__tests__/ipc-workspace-jobs.test.ts +++ b/desktop/src/main/__tests__/ipc-workspace-jobs.test.ts @@ -353,7 +353,7 @@ describe("workspace lifecycle IPC", () => { await flush() expect(ctx.jobs.get("ws")).toMatchObject({ state: outcome === "success" ? "reconciling" : "failed", - refreshError: "Workspace log could not be saved: disk unavailable", + logError: "Workspace log could not be saved: disk unavailable", ...(outcome === "failure" ? { error: "invalid ID" } : {}), }) const terminal = ctx.send.mock.calls.filter( @@ -368,7 +368,10 @@ describe("workspace lifecycle IPC", () => { }) await ctx.jobs.retryRefresh("ws") expect(ctx.jobs.get("ws")?.state).toBe( - outcome === "success" ? "succeeded" : "failed", + outcome === "success" ? "reconciling" : "failed", + ) + expect(ctx.jobs.get("ws")?.logError).toBe( + "Workspace log could not be saved: disk unavailable", ) }, ) @@ -547,7 +550,7 @@ describe("workspace lifecycle IPC", () => { await flush() expect(ctx.jobs.get("ws")).toMatchObject({ state: exitCode === 0 ? "reconciling" : "failed", - refreshError: "Workspace log could not be saved: disk unavailable", + logError: "Workspace log could not be saved: disk unavailable", error: exitCode === 0 ? undefined : "Cannot rebuild ***", }) const terminal = () => @@ -563,7 +566,8 @@ describe("workspace lifecycle IPC", () => { }) await ctx.jobs.retryRefresh("ws") expect(ctx.jobs.get("ws")).toMatchObject({ - state: exitCode === 0 ? "succeeded" : "failed", + state: exitCode === 0 ? "reconciling" : "failed", + logError: "Workspace log could not be saved: disk unavailable", refreshError: undefined, error: exitCode === 0 ? undefined : "Cannot rebuild ***", }) diff --git a/desktop/src/main/__tests__/watcher.test.ts b/desktop/src/main/__tests__/watcher.test.ts index 469d438e31..afd073f26f 100644 --- a/desktop/src/main/__tests__/watcher.test.ts +++ b/desktop/src/main/__tests__/watcher.test.ts @@ -10,6 +10,7 @@ function makeWatcher( ) { const state = { updateProviders: vi.fn().mockReturnValue(false), + updateWorkspaces: vi.fn().mockReturnValue(false), providerReadRevision: vi.fn().mockReturnValue(0), providerList: vi.fn().mockReturnValue([]), workspaceList: vi.fn().mockReturnValue(workspaces), @@ -412,3 +413,45 @@ describe("Watcher job broadcasts", () => { }, ) }) + +describe("Watcher log confirmation", () => { + it.each([undefined, "command denied"])( + "preserves log failures through polling and manual refresh (%s)", + async (error) => { + const { watcher, workspaceJobs } = makeWatcher(async () => ({})) + const refresh = vi + .fn() + .mockRejectedValueOnce(new Error("offline")) + .mockResolvedValue(undefined) + workspaceJobs.setRefresh(refresh) + const generation = workspaceJobs.start("ws", "creating", "first") + await workspaceJobs.finish( + "ws", + generation, + error, + "Workspace log could not be saved: disk unavailable", + ) + await (watcher as unknown as { pollOnce(): Promise }).pollOnce() + expect(refresh).not.toHaveBeenCalled() + await workspaceJobs.retryRefresh("ws") + expect(workspaceJobs.get("ws")?.refreshError).toBe("offline") + await (watcher as unknown as { pollOnce(): Promise }).pollOnce() + for (let i = 0; i < 20; i++) await Promise.resolve() + if (error) await workspaceJobs.retryRefresh("ws") + expect(refresh).toHaveBeenCalledTimes(2) + await workspaceJobs.retryRefresh("ws") + expect(workspaceJobs.get("ws")).toMatchObject({ + state: error ? "failed" : "reconciling", + logError: "Workspace log could not be saved: disk unavailable", + refreshError: undefined, + error, + }) + const next = workspaceJobs.start("ws", "creating", "retry") + expect(next).not.toBe(generation) + expect(workspaceJobs.get("ws")?.state).toBe("running") + expect(workspaceJobs.get("ws")?.logError).toBeUndefined() + await workspaceJobs.finish("ws", generation) + expect(workspaceJobs.get("ws")?.commandId).toBe("retry") + }, + ) +}) diff --git a/desktop/src/main/workspace-jobs.ts b/desktop/src/main/workspace-jobs.ts index fc53f45e6b..62686ee7b3 100644 --- a/desktop/src/main/workspace-jobs.ts +++ b/desktop/src/main/workspace-jobs.ts @@ -83,7 +83,7 @@ export class WorkspaceJobs { id: string, generation: number, error?: string, - confirmationError?: string, + logError?: string, ): Promise { const job = this.jobs.get(id) if (!job || this.generation(id) !== generation || job.state !== "running") @@ -91,21 +91,21 @@ export class WorkspaceJobs { this.generations.set(id, ++this.lastGeneration) // invalidate pre-completion polls this.jobs.set(id, { ...job, - state: error && confirmationError ? "failed" : "reconciling", - refreshError: confirmationError, + state: error && logError ? "failed" : "reconciling", + logError, phase: job.activity === "deleting" ? "Refreshing list" : "Refreshing status", error, }) this.emit() - if (!confirmationError) await this.retryRefresh(id) + if (!logError) await this.retryRefresh(id) } async retryRefresh(id: string): Promise { const job = this.jobs.get(id) if ( !job || (job.state !== "reconciling" && - !(job.state === "failed" && job.refreshError)) || + !(job.state === "failed" && (job.refreshError || job.logError))) || this.refreshing.has(job.commandId) ) return @@ -115,7 +115,11 @@ export class WorkspaceJobs { if (this.jobs.get(id) !== job) return this.jobs.set(id, { ...job, - state: job.error ? "failed" : "succeeded", + state: job.error + ? "failed" + : job.logError + ? "reconciling" + : "succeeded", phase: job.error ? "See workspace logs for details" : "", refreshError: undefined, }) diff --git a/desktop/src/renderer/src/lib/components/workspace/WorkspaceLaunchProgress.svelte b/desktop/src/renderer/src/lib/components/workspace/WorkspaceLaunchProgress.svelte index 35ff7d0b89..fcb32d5250 100644 --- a/desktop/src/renderer/src/lib/components/workspace/WorkspaceLaunchProgress.svelte +++ b/desktop/src/renderer/src/lib/components/workspace/WorkspaceLaunchProgress.svelte @@ -17,6 +17,7 @@ interface Props { running: boolean error?: string confirmationState: LaunchConfirmationState + logError?: string refreshError?: string refreshRetrying?: boolean onRetryStatus?: () => void @@ -34,6 +35,7 @@ let { running, error, confirmationState, + logError, refreshError, refreshRetrying = false, onRetryStatus, @@ -117,10 +119,12 @@ function elapsed(value: number): string { - {#if confirmationState === "stale"} + {#if logError} +

{logError}

+ {/if} + {#if confirmationState === "stale" && refreshError}

Status may be out of date

- {#if refreshError} - {/if}
{/if} diff --git a/desktop/src/renderer/src/lib/components/workspace/WorkspaceOperation.svelte b/desktop/src/renderer/src/lib/components/workspace/WorkspaceOperation.svelte index 5d030fa324..b77a2d1c38 100644 --- a/desktop/src/renderer/src/lib/components/workspace/WorkspaceOperation.svelte +++ b/desktop/src/renderer/src/lib/components/workspace/WorkspaceOperation.svelte @@ -92,4 +92,7 @@ const badgeVariant = $derived( {/if} {/if} + {#if density === "expanded" && view.error && job?.logError} +

{job.logError}

+ {/if} diff --git a/desktop/src/renderer/src/lib/components/workspace/WorkspaceOperation.test.ts b/desktop/src/renderer/src/lib/components/workspace/WorkspaceOperation.test.ts index ffb509a9b9..9f2b3bcbe0 100644 --- a/desktop/src/renderer/src/lib/components/workspace/WorkspaceOperation.test.ts +++ b/desktop/src/renderer/src/lib/components/workspace/WorkspaceOperation.test.ts @@ -23,6 +23,34 @@ beforeEach(() => { vi.clearAllMocks() }) describe("WorkspaceOperation", () => { + it.each(["reconciling", "failed"] as const)( + "shows persistent log warning for %s without a spinner", + (state) => { + workspaceJobs.set({ + ws: { + commandId: "cmd", + activity: "creating", + state, + phase: "", + logError: "Workspace log could not be saved: disk unavailable", + ...(state === "failed" ? { error: "CLI denied" } : {}), + }, + }) + const ui = render(WorkspaceOperation, { + id: "ws", + status: "Running", + density: "expanded", + }) + expect(ui.getByText(/Workspace log could not be saved/)).toBeTruthy() + expect(ui.getByRole("status").getAttribute("aria-busy")).toBe("false") + expect(ui.queryByText(/Status may be out of date/)).toBeNull() + expect( + ui.queryByRole("button", { name: "Retry status for ws" }), + ).toBeNull() + if (state === "failed") expect(ui.getByText(/CLI denied/)).toBeTruthy() + }, + ) + it("renders compact lifecycle status as a single-line badge", () => { const ui = render(WorkspaceOperation, { id: "ws", diff --git a/desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.svelte b/desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.svelte index 57ac8f0704..7947abb51d 100644 --- a/desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.svelte +++ b/desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.svelte @@ -315,6 +315,9 @@ let launchJob = $derived( let jobMatchesAttempt = $derived( !!commandId && launchJob?.commandId === commandId, ) +let launchLogError = $derived( + jobMatchesAttempt ? launchJob?.logError : undefined, +) let launchConfirmationState: LaunchConfirmationState = $derived( launchSuccess ? "confirmed" @@ -322,7 +325,7 @@ let launchConfirmationState: LaunchConfirmationState = $derived( ? "failed" : launchRefreshRetrying ? "confirming" - : launchStatusRefreshError + : launchStatusRefreshError || launchLogError ? "stale" : launchCommandSucceeded ? "confirming" @@ -340,6 +343,7 @@ $effect(() => { finishSuccessfulCommand(lastAttemptedId) if (job.refreshError) finishUnconfirmedLaunch(job.refreshError) else launchStatusRefreshError = null + if (job.logError) stopLaunchTimer() return } if (job.state === "succeeded") { @@ -1450,6 +1454,7 @@ function selectTemplate(t: { name: string; source: string }) { running={launchRunning || launchConfirmationState === "confirming"} error={launchError || undefined} confirmationState={launchConfirmationState} + logError={launchLogError} refreshError={launchStatusRefreshError ?? undefined} refreshRetrying={launchRefreshRetrying} onRetryStatus={retryLaunchStatus} @@ -1485,9 +1490,9 @@ function selectTemplate(t: { name: string; source: string }) { {/if} - {#if launchError || launchSuccess || launchStatusRefreshError} + {#if launchError || launchSuccess || launchStatusRefreshError || launchLogError}
- {#if !launchError && (launchSuccess || launchStatusRefreshError)} + {#if !launchError && (launchSuccess || launchStatusRefreshError || launchLogError)} + {#if launchLogError}{/if} {:else if launchError} {#if launchBuildFailed && !launchIsRecovery} diff --git a/desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.test.ts b/desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.test.ts index 9528ebddd1..bfef3c592f 100644 --- a/desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.test.ts +++ b/desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.test.ts @@ -748,6 +748,33 @@ describe("WorkspaceWizard", () => { unmount() }) + it("keeps a log failure unconfirmed without a status spinner or completion", async () => { + providers.set([makeProvider("docker")]) + const oncomplete = vi.fn() + const ui = render(WorkspaceWizard, { props: { open: true, oncomplete } }) + await flushAsync() + await advanceToReview(ui.getByText) + await fireEvent.click(getLaunchButton()) + await flushAsync() + ;(workspaceJobs as { set: (v: unknown) => void }).set({ + python: { + commandId: "cmd-1", + activity: "creating", + state: "reconciling", + phase: "", + logError: "Workspace log could not be saved: disk unavailable", + }, + }) + await flushAsync() + expect(ui.getByText(/Workspace log could not be saved/)).toBeTruthy() + expect(ui.queryByText("Workspace ready")).toBeNull() + expect(ui.queryByText("Confirming workspace status")).toBeNull() + expect(ui.queryByRole("button", { name: "Retry status" })).toBeNull() + expect(ui.getByRole("button", { name: /^Retry$/ })).toBeTruthy() + expect(oncomplete).not.toHaveBeenCalled() + ui.unmount() + }) + it("keeps a refresh-only failure stale until Retry status confirms the workspace", async () => { providers.set([makeProvider("docker")]) const oncomplete = vi.fn() diff --git a/desktop/src/renderer/src/lib/stores/workspaces.test.ts b/desktop/src/renderer/src/lib/stores/workspaces.test.ts index c0525123d7..2b388319dc 100644 --- a/desktop/src/renderer/src/lib/stores/workspaces.test.ts +++ b/desktop/src/renderer/src/lib/stores/workspaces.test.ts @@ -336,6 +336,50 @@ describe("workspace snapshot store", () => { expect(toasts.success).not.toHaveBeenCalled() expect(toasts.error).not.toHaveBeenCalled() }) + it("does not toast ready while log persistence remains unconfirmed", async () => { + mockInvoke.mockResolvedValue({ + revision: 1, + workspaces: [{ id: "ws", status: "Running" }], + jobs: { ws: job }, + }) + await initWorkspaces() + for (const revision of [2, 3]) + event({ + revision, + workspaces: [{ id: "ws", status: "Running" }], + jobs: { + ws: { + ...job, + state: "reconciling", + logError: "Workspace log could not be saved: disk unavailable", + }, + }, + }) + expect(toasts.success).not.toHaveBeenCalled() + expect(toasts.error).not.toHaveBeenCalled() + }) + it("retains a deleted workspace row while its log warning is unresolved", async () => { + mockInvoke.mockResolvedValue({ + revision: 1, + workspaces: [{ id: "ws", status: "Running" }], + jobs: { ws: job }, + }) + await initWorkspaces() + event({ + revision: 2, + workspaces: [], + jobs: { + ws: { + ...job, + activity: "deleting", + state: "reconciling", + logError: "Workspace log could not be saved: disk unavailable", + }, + }, + }) + expect(get(workspaces).map((workspace) => workspace.id)).toEqual(["ws"]) + expect(toasts.success).not.toHaveBeenCalled() + }) it("leaves existing observations intact when snapshot loading fails", async () => { workspaces.set([{ id: "ws", status: "Running" }]) mockInvoke.mockRejectedValue(new Error("offline")) diff --git a/desktop/src/renderer/src/lib/stores/workspaces.ts b/desktop/src/renderer/src/lib/stores/workspaces.ts index 6fb44bb291..73d7209e70 100644 --- a/desktop/src/renderer/src/lib/stores/workspaces.ts +++ b/desktop/src/renderer/src/lib/stores/workspaces.ts @@ -50,6 +50,7 @@ function apply( ([id, job]) => !updated.some((workspace) => workspace.id === id) && (job.state === "running" || + !!job.logError || (job.activity === "creating" && job.state !== "succeeded")), ) workspaces.set([...updated, ...pending.map(([id]) => ({ id }))]) diff --git a/desktop/src/shared/workspace-operation.ts b/desktop/src/shared/workspace-operation.ts index d4f05a9abf..151265c440 100644 --- a/desktop/src/shared/workspace-operation.ts +++ b/desktop/src/shared/workspace-operation.ts @@ -14,12 +14,14 @@ export interface WorkspaceJob { phase: string status?: OperationStatus error?: string + logError?: string refreshError?: string } export function workspaceJobBusy(job?: WorkspaceJob): boolean { return ( - job?.state === "running" || (job?.state === "reconciling" && !job.error) + job?.state === "running" || + (job?.state === "reconciling" && !job.error && !job.logError) ) } @@ -153,6 +155,16 @@ export function presentWorkspaceStatus(input: { } const headline = HEADLINE[job.activity] if (job.state === "reconciling") { + if (job.logError) { + return { + headline, + tone: "warning", + busy: false, + announce: `${headline}. ${job.logError}`, + recovery: { message: job.logError, canRetry: !!job.refreshError }, + detailsAvailable: true, + } + } if (job.refreshError) { const message = stalledMessage(job.activity) return { diff --git a/pkg/task/legacy_launch_test.go b/pkg/task/legacy_launch_test.go new file mode 100644 index 0000000000..780d2be6dc --- /dev/null +++ b/pkg/task/legacy_launch_test.go @@ -0,0 +1,102 @@ +package task + +import ( + "context" + "os/exec" + "testing" + "time" + + "github.com/devsy-org/devsy/pkg/command" + "github.com/stretchr/testify/require" +) + +func TestLegacyPendingLaunchDoesNotTrustPreviousWorkerExit(t *testing.T) { + for _, scenario := range []struct { + name string + cancelRequested bool + terminal bool + }{ + {name: "pending"}, + {name: "cancel requested", cancelRequested: true}, + {name: "already failed", terminal: true}, + } { + t.Run(scenario.name, func(t *testing.T) { + store, tk, unpublished := ambiguousLegacyLaunch(t) + require.NoError(t, store.update(tk.ID(), func(s *State) { + s.CancelRequested = scenario.cancelRequested + if scenario.terminal { + s.Status = StatusFailed + s.Error = "original worker failure" + s.ErrorCode = "original_code" + } + })) + before, err := store.Get(tk.ID()) + require.NoError(t, err) + terminateCalls, cleanupCalls := 0, 0 + store.processController = fakeProcessController{ + terminate: func(command.ProcessRef) error { terminateCalls++; return nil }, + cleanup: func(command.ProcessRef) error { cleanupCalls++; return nil }, + } + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + state, err := store.ReconcileTask(ctx, tk.ID()) + require.ErrorContains(t, err, "legacy abandoned launch") + require.Equal(t, before.CancelRequested, state.CancelRequested) + requireLegacyLaunchUnverified(t, before, state) + require.ErrorContains(t, tk.CancelContext(ctx), "legacy abandoned launch") + state, err = store.Get(tk.ID()) + require.NoError(t, err) + require.Equal(t, !scenario.terminal || before.CancelRequested, state.CancelRequested) + requireLegacyLaunchUnverified(t, before, state) + require.Zero(t, terminateCalls) + require.Zero(t, cleanupCalls) + exited, err := command.ProcessExited(unpublished) + require.NoError(t, err) + require.False(t, exited, "the unpublished attempt must remain alive") + }) + } +} + +func ambiguousLegacyLaunch(t *testing.T) (*Store, *Task, command.ProcessRef) { + t.Helper() + store := newTestStore(t) + tk, err := store.Create(CreateOptions{}) + require.NoError(t, err) + previous := legacyLaunchChild(t) + require.NoError(t, tk.SetPID(previous.Process.Pid)) + state, err := store.Get(tk.ID()) + require.NoError(t, err) + previousRef, ok := state.ProcessReference() + require.True(t, ok) + require.NotEmpty(t, previousRef.Identity) + require.NoError(t, previous.Process.Kill()) + require.Error(t, previous.Wait()) + exited, err := command.ProcessExited(previousRef) + require.NoError(t, err) + require.True(t, exited) + unpublished := legacyLaunchChild(t) + identity, err := command.ProcessTreeIdentity(unpublished.Process.Pid) + require.NoError(t, err) + require.NotEmpty(t, identity) + // Legacy BeginLaunch could retain an earlier attempt's reference; the current + // protocol rejects reuse, so this fixture restores that ambiguous stored state. + require.NoError(t, store.update(tk.ID(), func(s *State) { s.LaunchPending = true })) + return store, tk, command.ProcessRef{PID: unpublished.Process.Pid, Identity: identity} +} + +func legacyLaunchChild(t *testing.T) *exec.Cmd { + t.Helper() + child := startupTestChild() + require.NoError(t, child.Start()) + t.Cleanup(func() { _ = child.Process.Kill(); _ = child.Wait() }) + return child +} + +func requireLegacyLaunchUnverified(t *testing.T, before, after *State) { + t.Helper() + require.True(t, after.LaunchPending) + require.False(t, after.ProcessCleanupComplete) + require.Equal(t, before.Status, after.Status) + require.Equal(t, before.Error, after.Error) + require.Equal(t, before.ErrorCode, after.ErrorCode) +} From 50d2e61d5f9b2ec0991ac705c032f8bc76155ef5 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 21 Oct 2026 19:13:34 -0600 Subject: [PATCH 11/16] chore(ci): fix actionlint subprocess deadlock on macOS --- prek.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prek.toml b/prek.toml index 5683a7aeb7..ace6fa7185 100644 --- a/prek.toml +++ b/prek.toml @@ -64,7 +64,7 @@ hooks = [ # Tracked upstream: https://github.com/rhysd/actionlint/issues/693 [[repos]] repo = "https://github.com/rhysd/actionlint" -rev = "v1.7.12" +rev = "fd33e9f582a01a02885c93ae3775e1190cf63653" hooks = [ { id = "actionlint", From c9f3ffb733f37a6fecf443bc512006911b9b736a Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 21 Oct 2026 20:58:20 -0600 Subject: [PATCH 12/16] fix(task): reject incomplete task inventories before cleanup --- pkg/task/store.go | 24 ++++++++++--- pkg/task/store_test.go | 64 +++++++++++++++++++++++++++++++++++ pkg/workspace/quiesce_test.go | 16 +++++++++ 3 files changed, 100 insertions(+), 4 deletions(-) diff --git a/pkg/task/store.go b/pkg/task/store.go index 455de130a3..e48151a84d 100644 --- a/pkg/task/store.go +++ b/pkg/task/store.go @@ -28,6 +28,7 @@ type Store struct { afterClaimHook func() afterCancelLockClaimedHook func() afterStateLockBlockedHook func() + stateLockTimeout time.Duration } func NewStore() (*Store, error) { @@ -242,11 +243,22 @@ func (s *Store) List() ([]*State, error) { continue } id := entry.Name()[:len(entry.Name())-len(".json")] - state, err := s.Get(id) + var state *State + err := s.withLock(id, func() error { + var readErr error + state, readErr = s.readState(id) + // Only an absent state read under the lock confirms concurrent deletion. + if errors.Is(readErr, os.ErrNotExist) { + return nil + } + return readErr + }) if err != nil { - continue + return nil, fmt.Errorf("list task %s: %w", id, err) + } + if state != nil { + states = append(states, state) } - states = append(states, state) } sort.Slice(states, func(i, j int) bool { @@ -633,7 +645,11 @@ func (s *Store) withLock(id string, fn func() error) error { if s.afterStateLockBlockedHook != nil { s.afterStateLockBlockedHook() } - ctx, cancel := context.WithTimeout(context.Background(), lockTimeout) + timeout := s.stateLockTimeout + if timeout == 0 { + timeout = lockTimeout + } + ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() locked, err = lock.TryLockContext(ctx, 20*time.Millisecond) if err != nil { diff --git a/pkg/task/store_test.go b/pkg/task/store_test.go index 7ee010a1de..f314ed7f29 100644 --- a/pkg/task/store_test.go +++ b/pkg/task/store_test.go @@ -503,3 +503,67 @@ func TestStateNeedsReconciliation(t *testing.T) { }) } } + +func TestTaskQueriesRejectUncertainRecords(t *testing.T) { + for _, failure := range []string{"state-lock", "malformed-state"} { + t.Run(failure, func(t *testing.T) { + store := newTestStore(t) + tk := createTaskWith(t, store, CreateOptions{WorkspaceID: workspaceOne, Command: "up"}) + queries := map[string]func() ([]*State, error){ + "List": store.List, + "ForWorkspace": func() ([]*State, error) { return store.ForWorkspace(workspaceOne, "up") }, + "ActiveForWorkspace": func() ([]*State, error) { return store.ActiveForWorkspace(workspaceOne, "up") }, + } + check := func() error { + for name, query := range queries { + t.Run(name, func(t *testing.T) { + states, err := query() + require.Error(t, err) + require.Nil(t, states) + }) + } + return nil + } + if failure == "state-lock" { + store.stateLockTimeout = -time.Nanosecond + require.NoError(t, store.withLock(tk.ID(), check)) + } else { + path, err := store.path(tk.ID()) + require.NoError(t, err) + require.NoError(t, os.WriteFile(path, []byte("{"), 0o600)) + require.NoError(t, check()) + } + }) + } +} + +func TestListOmitsRecordDeletedWhileWaitingForStateLock(t *testing.T) { + writer := newTestStore(t) + reader, err := NewStoreAt(writer.dir) + require.NoError(t, err) + tk := createTaskWith(t, writer, CreateOptions{}) + path, err := writer.path(tk.ID()) + require.NoError(t, err) + blocked := make(chan struct{}) + reader.afterStateLockBlockedHook = func() { close(blocked) } + type result struct { + states []*State + err error + } + read := make(chan result, 1) + require.NoError(t, writer.withLock(tk.ID(), func() error { + go func() { + states, err := reader.List() + read <- result{states, err} + }() + select { + case <-blocked: + case r := <-read: + t.Fatalf("listing bypassed writer: %+v", r) + } + return os.Remove(path) + })) + r := <-read + require.NoError(t, r.err) + require.Empty(t, r.states) +} diff --git a/pkg/workspace/quiesce_test.go b/pkg/workspace/quiesce_test.go index aa9b6c5e1a..30853e120f 100644 --- a/pkg/workspace/quiesce_test.go +++ b/pkg/workspace/quiesce_test.go @@ -175,3 +175,19 @@ func TestQuiesceUpTasksRejectsUnverifiedTerminalLaunch(t *testing.T) { require.ErrorContains(t, err, "cannot verify cleanup of a legacy abandoned launch") assert.True(t, taskState(t, store, "interrupted").LaunchPending) } + +func TestQuiesceUpTasksRejectsMalformedTaskRecord(t *testing.T) { + dir := t.TempDir() + store, err := task.NewStoreAt(dir) + require.NoError(t, err) + tk := createUpTask(t, store, "ws-1") + path := filepath.Join(dir, tk.ID()+".json") + require.NoError(t, os.WriteFile(path, []byte("{"), 0o600)) + called := false + err = quiesceUpTasks(context.Background(), store, "ws-1", func(context.Context, string) error { + called = true + return nil + }) + require.ErrorContains(t, err, "list up tasks for workspace ws-1") + require.False(t, called) +} From f334a7c1812a94dd621f3c13b9d0630465b0b380 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 21 Oct 2026 23:27:41 -0600 Subject: [PATCH 13/16] test(ssh): capture bounded proxy command diagnostics --- e2e/tests/ssh/proxy_command.go | 71 +++++++++++++++++++++++++++-- e2e/tests/ssh/proxy_command_test.go | 27 +++++++++++ hack/ci/suites.yaml | 8 +++- 3 files changed, 101 insertions(+), 5 deletions(-) create mode 100644 e2e/tests/ssh/proxy_command_test.go diff --git a/e2e/tests/ssh/proxy_command.go b/e2e/tests/ssh/proxy_command.go index c9b96550a0..edcf43ebae 100644 --- a/e2e/tests/ssh/proxy_command.go +++ b/e2e/tests/ssh/proxy_command.go @@ -3,13 +3,16 @@ package ssh import ( "bytes" "context" + "fmt" "os" "os/exec" "path/filepath" + "strconv" "strings" "time" "github.com/devsy-org/devsy/e2e/framework" + "github.com/devsy-org/devsy/pkg/secrets" "github.com/onsi/ginkgo/v2" "github.com/onsi/gomega" ) @@ -74,6 +77,18 @@ var _ = ginkgo.Describe( sshPath, err := exec.LookPath("ssh.exe") framework.ExpectNoError(err) host := filepath.Base(tempDir) + ".devsy" + traceDir := ginkgo.GinkgoT().TempDir() + traceRoot, err := os.OpenRoot(traceDir) + framework.ExpectNoError(err) + ginkgo.DeferCleanup(traceRoot.Close) + tracePath := filepath.Join(traceDir, proxyCommandTraceFile) + traceFile, err := traceRoot.OpenFile( + proxyCommandTraceFile, + os.O_CREATE|os.O_EXCL|os.O_WRONLY, + 0o600, + ) + framework.ExpectNoError(err) + framework.ExpectNoError(traceFile.Close()) sshCtx, cancelSSH := context.WithTimeout(ctx, 30*time.Second) defer cancelSSH() // #nosec G204 -- controlled OpenSSH invocation for the E2E test @@ -81,6 +96,7 @@ var _ = ginkgo.Describe( sshCtx, sshPath, "-F", sshConfigPath, + "-vvv", "-E", tracePath, "-o", "BatchMode=yes", host, "printf", @@ -89,12 +105,31 @@ var _ = ginkgo.Describe( var stdout, stderr bytes.Buffer cmd.Stdout = &stdout cmd.Stderr = &stderr + started := time.Now() err = cmd.Run() - framework.ExpectNoError( - err, - "OpenSSH should launch ProxyCommand; stdout=%q stderr=%q", - stdout.String(), stderr.String(), + elapsed := time.Since(started) + sshErr, cause, parentErr := sshCtx.Err(), context.Cause(sshCtx), ctx.Err() + exitCode := -1 + if cmd.ProcessState != nil { + exitCode = cmd.ProcessState.ExitCode() + } + diagnostic := fmt.Sprintf( + "started=%s elapsed=%s sshContext=%v cause=%v parentContext=%v exitCode=%d; %s", + started.UTC(). + Format(time.RFC3339Nano), + elapsed, + sshErr, + cause, + parentErr, + exitCode, + proxyCommandOutputDiagnostic(traceRoot, stdout.String(), stderr.String()), + ) + ginkgo.AddReportEntry( + "OpenSSH ProxyCommand diagnostic", + diagnostic, + ginkgo.ReportEntryVisibilityFailureOrVerbose, ) + framework.ExpectNoError(err, "OpenSSH should launch ProxyCommand; %s", diagnostic) gomega.Expect(strings.TrimSpace(stdout.String())).To( gomega.Equal("proxy-command-ok"), ) @@ -102,3 +137,31 @@ var _ = ginkgo.Describe( ) }, ) + +func proxyCommandOutputDiagnostic(traceRoot *os.Root, stdout, stderr string) string { + trace, err := traceRoot.ReadFile(proxyCommandTraceFile) + redactor := secrets.NewEnvironmentRedactor(os.Environ()) + return fmt.Sprintf( + "stdout=%s stderr=%s traceReadError=%s trace=%s", + proxyCommandExcerpt(redactor, stdout), + proxyCommandExcerpt(redactor, stderr), + proxyCommandExcerpt( + redactor, + fmt.Sprint(err), + ), + proxyCommandExcerpt(redactor, string(trace)), + ) +} + +const ( + proxyCommandTraceFile = "ssh trace.log" + proxyCommandExcerptLimit = 8192 +) + +func proxyCommandExcerpt(redactor *secrets.Redactor, text string) string { + quoted := strconv.QuoteToASCII(redactor.Redact(text)) + if len(quoted) <= proxyCommandExcerptLimit { + return quoted + } + return "[truncated]" + quoted[len(quoted)-proxyCommandExcerptLimit:] +} diff --git a/e2e/tests/ssh/proxy_command_test.go b/e2e/tests/ssh/proxy_command_test.go new file mode 100644 index 0000000000..3fd2702729 --- /dev/null +++ b/e2e/tests/ssh/proxy_command_test.go @@ -0,0 +1,27 @@ +package ssh + +import ( + "strings" + "testing" + + "github.com/devsy-org/devsy/pkg/secrets" + "github.com/stretchr/testify/require" +) + +func TestProxyCommandExcerptProtectsFailureReport(t *testing.T) { + secret := strings.Repeat("test-value-", 8) + redactor := secrets.NewEnvironmentRedactor([]string{"DEVSY_TOKEN=" + secret}) + input := strings.Repeat( + "padding", + 3000, + ) + "\x1b[31m\n" + secret + " https://user:password@example.com/end" + got := proxyCommandExcerpt(redactor, input) + require.LessOrEqual(t, len(got), proxyCommandExcerptLimit+len("[truncated]")) + require.NotContains(t, got, secret) + require.NotContains(t, got, "password") + require.NotContains(t, got, "\x1b") + require.NotContains(t, got, "\n") + require.Contains(t, got, `\x1b`) + require.Contains(t, got, "example.com/end") + require.Contains(t, got, "***") +} diff --git a/hack/ci/suites.yaml b/hack/ci/suites.yaml index 72b0987699..83b3856940 100644 --- a/hack/ci/suites.yaml +++ b/hack/ci/suites.yaml @@ -38,7 +38,13 @@ unit: - name: e2e-runtime-helpers description: E2E runtime helper packages - packages: [./e2e/framework/runenv/..., ./e2e/framework/dockerwsl, ./e2e/framework/ginkgoreport, ./e2e/cmd/devsy-ci] + packages: + - ./e2e/framework/runenv/... + - ./e2e/framework/dockerwsl + - ./e2e/framework/ginkgoreport + - ./e2e/cmd/devsy-ci + - ./e2e/tests/ssh + require_tests: [TestProxyCommandExcerptProtectsFailureReport] args: [-race, -count=1, -timeout=5m] - name: e2e-framework From 3370ae64f83b1863e178832372ff03cf267a211d Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 22 Oct 2026 00:49:51 -0600 Subject: [PATCH 14/16] fix(desktop): preserve context edits during option loading --- .../components/context/ContextSheet.svelte | 28 ++- .../components/context/ContextSheet.test.ts | 225 ++++++++++++++++++ 2 files changed, 243 insertions(+), 10 deletions(-) create mode 100644 desktop/src/renderer/src/lib/components/context/ContextSheet.test.ts diff --git a/desktop/src/renderer/src/lib/components/context/ContextSheet.svelte b/desktop/src/renderer/src/lib/components/context/ContextSheet.svelte index ecd572263d..1af138a981 100644 --- a/desktop/src/renderer/src/lib/components/context/ContextSheet.svelte +++ b/desktop/src/renderer/src/lib/components/context/ContextSheet.svelte @@ -1,4 +1,5 @@