feat(workspace): support async, durable workspace up execution - #798
Open
skevetter wants to merge 18 commits into
Open
feat(workspace): support async, durable workspace up execution#798skevetter wants to merge 18 commits into
skevetter wants to merge 18 commits into
Conversation
workspace up currently runs synchronously end-to-end, streaming a single JSON blob to the caller only once the container is ready. That blocks the CLI/desktop for the whole provisioning window and gives up all in-progress state on a crash or disconnect. Adds a submit/poll durable execution model alongside the existing synchronous flow: - pkg/task: a JSON-file task store (atomic writes, cross-process flock) recording status/PID/result for background-launched work. - workspace up --detach re-execs itself as a background process and returns a task ID immediately. - workspace task list/get/logs/cancel/rm: standard verb commands (kubectl/docker/gh-style) to manage submitted tasks. - pkg/devcontainer/status: a structured Phase/Event/Reporter model threaded through Runner.Up and the devcontainer build/run/setup pipeline, replacing ad hoc log lines with typed progress events. - A StatusUpdate RPC on the agent tunnel streams those events back to the CLI host live instead of waiting for the final result. - Desktop app updated to submit+poll via the new task model instead of parsing a single terminal JSON blob. See docs/rfcs/async-workspace-up.md for the full design.
✅ Deploy Preview for images-devsy-sh canceled.
|
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds detached workspace tasks, persistent worker-lock reconciliation, structured status propagation through container and tunnel paths, desktop IPC task streaming, task-control commands, and tests for lifecycle, concurrency, process termination, and envelope handling. ChangesWorkspace task lifecycle
Structured status propagation
Desktop detached-up flow
Supporting concurrency changes
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant DesktopIPC
participant DevsyCLI
participant TaskStore
participant TunnelServer
participant Renderer
DesktopIPC->>DevsyCLI: workspace up --detach
DevsyCLI->>TaskStore: Create task
DevsyCLI-->>DesktopIPC: task envelope
DesktopIPC->>DevsyCLI: workspace task logs --follow
DevsyCLI->>TunnelServer: Status(StatusUpdate)
DevsyCLI-->>DesktopIPC: status/result/error envelopes
DesktopIPC->>Renderer: workspace-status event
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Deploy Preview for devsydev canceled.
|
Fixes the unused status import left over from the intermediate up.go split, plus cyclop, funcorder, errcheck, forbidigo, goconst, golines, modernize, and revive findings across the new task/status/detach code.
fmt.Printf is disallowed by the forbidigo rule; fmt.Fprintf(os.Stdout, ...) is the same output without needing a suppression comment.
workspace_up now submits `up --detach` and polls `workspace task logs
<id> --follow` instead of running `up` to completion. The e2e mock CLI
still implemented the old synchronous `up` only: `--detach` was
silently swallowed by parseArgs's generic flag skip, so mock-devsy
wrote plain progress text instead of a {kind:"task", id} envelope.
cli.run's JSON.parse then threw, the up handler took its catch path,
and the wizard's success check never ran -- so the "Open Workspace"
button never appeared, timing out the three affected e2e specs.
Adds a `task` verb to the mock (list/get/logs/cancel/rm) backed by a
`tasks` map in the persisted mock state, and makes `up --detach` write
the {kind:"task", id} envelope and defer the actual workspace/progress
simulation to `task logs --follow`, mirroring pkg/task and
pkg/devcontainer/config/envelope.go.
skevetter
marked this pull request as ready for review
July 29, 2026 13:48
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
desktop/src/main/ipc.ts (1)
751-841: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winOld tunnel process isn't awaited before its map entry is overwritten — can leak an orphaned child.
existing.kill("SIGTERM")(Line 756) is fire-and-forget; execution continues immediately into the detached submit and eventuallytunnelProcesses.set(wsId, child)(Line 841) overwrites the map key before the old process is confirmed dead. If it hasn't exited yet, its handle is lost — no futurequiesceWorkspacecall can find/kill it.quiesceWorkspace(Lines 187-199) already implements the correct await-exit pattern for exactly this; this block should share it instead of duplicating a weaker copy that also omits the cli/pty cancellation. Consider extracting the tunnel-kill logic (and the active-task-cancel logic duplicated at Lines 759-765 vs. 179-186) into one helper both call.🔒 Proposed fix — await exit before proceeding
const existing = tunnelProcesses.get(wsId) if (existing) { - existing.kill("SIGTERM") tunnelProcesses.delete(wsId) + const existingExit = new Promise<void>((resolve) => { + if (existing.exitCode !== null || existing.signalCode !== null) { + resolve() + return + } + existing.once("close", () => resolve()) + }) + existing.kill("SIGTERM") + await existingExit }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@desktop/src/main/ipc.ts` around lines 751 - 841, Update the workspace startup cleanup around tunnelProcesses and activeUpTasks to reuse the existing quiesceWorkspace cleanup logic, or extract a shared helper used by both paths. Ensure the existing tunnel child is terminated and awaited before replacing its map entry, and preserve cancellation of any active CLI task, including the existing cli/pty cleanup behavior implemented by quiesceWorkspace.pkg/devcontainer/run.go (1)
180-188: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDrop the outer
PhaseReadyfailure reporting for inner dispatch failures.
dispatchByConfigKindroutes to paths that already report the actual failing phase (PhaseBuildingImage,PhaseStartingContainer,PhaseInjectingAgent,PhaseRunningLifecycleHook). This outerstatus.Fail(reporter, status.PhaseReady, err)re-emits those failures withStep: "ready", which mislabels build/startup/agent/lifecycle failures and duplicates the failure event.🐛 Possible fix: only report the phase name once, closest to the source, and drop this generic outer Fail
result, err := r.dispatchByConfigKind(ctx, substitutedConfig, params) if result != nil { result.RecoveryContainer = r.recovering } if err != nil { - status.Fail(reporter, status.PhaseReady, err) + // Assumes the inner pipeline already reported the granular failing + // phase via status.Fail; avoid re-labeling it as "ready" here. return result, err } status.Leave(reporter, status.PhaseReady, "")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/devcontainer/run.go` around lines 180 - 188, Remove the outer status.Fail call for errors returned by dispatchByConfigKind in the run flow, while preserving the error return and result recovery assignment. Let the dispatched paths report their specific phases—PhaseBuildingImage, PhaseStartingContainer, PhaseInjectingAgent, or PhaseRunningLifecycleHook—without emitting a duplicate PhaseReady failure.pkg/client/clientimplementation/workspace_client.go (1)
1148-1163: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winForward the local-up status reporter into
runTunnelServer.
pkg/client/clientimplementation/workspace_client.go:1158-1161appends a finalWithStatusReporter(status.NewLogReporter()), which overwrites any per-call status reporter becausetunnelserver.New()applies options in order and each option setss.statusReporter. This ignores the caller’s progress/reporter setup, including theUpCommandReporterwired incmd/workspace/up/agent.go; status events from the agent over RPC are logged and then discarded. Threadcmd.statusReporterorUpOptions.ReporterintoBuildAgentClientOptions.TunnelOptionsinstead of forcing a discard-only logger here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/client/clientimplementation/workspace_client.go` around lines 1148 - 1163, The runTunnelServer function currently overwrites caller-provided reporters with status.NewLogReporter. Remove that forced reporter and ensure the local-up status reporter, such as cmd.statusReporter or UpOptions.Reporter, is propagated through BuildAgentClientOptions.TunnelOptions before reaching RunUpServer.
🧹 Nitpick comments (1)
pkg/client/clientimplementation/workspace_client.go (1)
288-315: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueTask-store file I/O runs while holding
s.mfor the wholeStatus()call.
taskStatusOverride()/latestUpTask()perform directory listing + per-file reads whiles.m.Lock()is held for the entire method, extending lock contention beyond what's needed to safely reads.workspace.ID.♻️ Suggested: snapshot the workspace ID, then do file I/O outside the lock
func (s *workspaceClient) Status( ctx context.Context, opt client.StatusOptions, ) (client.Status, error) { s.m.Lock() - defer s.m.Unlock() - var ( result client.Status err error ) switch { case s.isMachineProvider() && len(s.config.Exec.Status) > 0: result, err = s.machineStatus(ctx, opt) case opt.ContainerStatus: result, err = s.getContainerStatus(ctx) default: result, err = s.workspaceFolderStatus() } + workspaceID := s.workspace.ID + s.m.Unlock() if err != nil || result != client.StatusNotFound { return result, err } - - if override, ok := s.taskStatusOverride(); ok { + if override, ok := s.taskStatusOverride(workspaceID); ok { return override, nil } return result, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/client/clientimplementation/workspace_client.go` around lines 288 - 315, Update workspaceClient.Status to avoid holding s.m during task-store I/O: lock only long enough to snapshot the workspace ID needed by taskStatusOverride/latestUpTask, then unlock before performing status resolution and override checks. Ensure taskStatusOverride uses the snapshot rather than reading s.workspace.ID while the mutex is held, preserving existing status precedence and return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/workspace/task.go`:
- Around line 154-194: Validate that the parsed interval is strictly positive
immediately after time.ParseDuration in the command handler, before passing it
through followTask or creating a ticker. Return a descriptive error for zero or
negative durations, while preserving the existing parse error and normal
positive-interval behavior.
In `@cmd/workspace/up/status.go`:
- Around line 30-39: Update plainStatusReporter.Report to handle non-started
phase events in addition to failures and starts, emitting a completion/ready log
for Leave events including the final PhaseReady event. Mirror the completion
behavior used by logReporter while preserving the existing failure and
phase-start messages.
In `@cmd/workspace/up/up.go`:
- Around line 226-237: Update the task initialization error paths in the
workspace-up flow so both openTask and SetWorkspaceID failures are passed
through reportErr(err, emitJSON, out) before returning. Preserve the existing
failTask behavior for SetWorkspaceID while ensuring JSON mode emits an error
envelope for either failure.
In `@pkg/client/clientimplementation/daemonclient/up.go`:
- Around line 450-492: Update statusSniffingWriter.Close to run the buffered
trailing bytes through config.ParseStatusLine before forwarding them to next,
matching the complete-line handling in Write. Report and suppress a trailing
status envelope without a newline; forward non-status content unchanged, then
clear the buffer and preserve the existing error behavior.
In `@pkg/command/process_supported.go`:
- Around line 45-47: In pkg/command/process_supported.go lines 45-47, update the
process escalation flow around syscall.Kill to retain and validate a stable
process identity before sending SIGKILL, avoiding blind signaling of a reused
numeric PID. In pkg/command/process_test.go lines 33-42, replace the stale
real-PID scenario with an injected or stubbed signal operation that returns
syscall.ESRCH, and verify the existing handling for that result.
In `@pkg/compose/helper_test.go`:
- Around line 194-196: Update the Podman reachability probe in the test setup to
use exec.CommandContext with a short timeout, canceling the context after the
probe completes. Preserve the existing skip behavior when the timed command
fails or times out.
In `@pkg/devcontainer/status/status.go`:
- Around line 32-35: Update jsonStatusReporter.Report to serialize access to Out
with synchronization before encoding and writing each status event. Ensure
concurrent tunnel Status RPC reports produce complete, non-interleaved NDJSON
lines while preserving the existing reporting behavior.
In `@pkg/task/store.go`:
- Around line 67-89: Update Store.Delete to acquire and hold the same per-id
flock used by update and write for the entire status-check and file-removal
sequence, releasing it on every return path. Reuse the existing locking helper
and preserve the force/non-terminal validation and deletion behavior while
ensuring no concurrent atomic rename can recreate the task after removal.
In `@pkg/task/task.go`:
- Around line 87-93: Guard Task.Succeed, Task.Fail, and taskReporter.Report with
the same State.Terminal() check used by Cancel before mutating state. Preserve
the existing terminal state and result/error when the task is already canceled
or otherwise terminal; only apply success, failure, or phase/step updates for
non-terminal tasks.
---
Outside diff comments:
In `@desktop/src/main/ipc.ts`:
- Around line 751-841: Update the workspace startup cleanup around
tunnelProcesses and activeUpTasks to reuse the existing quiesceWorkspace cleanup
logic, or extract a shared helper used by both paths. Ensure the existing tunnel
child is terminated and awaited before replacing its map entry, and preserve
cancellation of any active CLI task, including the existing cli/pty cleanup
behavior implemented by quiesceWorkspace.
In `@pkg/client/clientimplementation/workspace_client.go`:
- Around line 1148-1163: The runTunnelServer function currently overwrites
caller-provided reporters with status.NewLogReporter. Remove that forced
reporter and ensure the local-up status reporter, such as cmd.statusReporter or
UpOptions.Reporter, is propagated through BuildAgentClientOptions.TunnelOptions
before reaching RunUpServer.
In `@pkg/devcontainer/run.go`:
- Around line 180-188: Remove the outer status.Fail call for errors returned by
dispatchByConfigKind in the run flow, while preserving the error return and
result recovery assignment. Let the dispatched paths report their specific
phases—PhaseBuildingImage, PhaseStartingContainer, PhaseInjectingAgent, or
PhaseRunningLifecycleHook—without emitting a duplicate PhaseReady failure.
---
Nitpick comments:
In `@pkg/client/clientimplementation/workspace_client.go`:
- Around line 288-315: Update workspaceClient.Status to avoid holding s.m during
task-store I/O: lock only long enough to snapshot the workspace ID needed by
taskStatusOverride/latestUpTask, then unlock before performing status resolution
and override checks. Ensure taskStatusOverride uses the snapshot rather than
reading s.workspace.ID while the mutex is held, preserving existing status
precedence and return behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a92c0000-ca8f-4986-be56-496cfd1301e6
⛔ Files ignored due to path filters (2)
pkg/agent/tunnel/tunnel.pb.gois excluded by!**/*.pb.gopkg/agent/tunnel/tunnel_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (44)
cmd/internal/agentworkspace/up.gocmd/internal/container_tunnel.gocmd/workspace/task.gocmd/workspace/task_test.gocmd/workspace/up/agent.gocmd/workspace/up/detach.gocmd/workspace/up/detach_test.gocmd/workspace/up/status.gocmd/workspace/up/up.gocmd/workspace/up/up_flags.gocmd/workspace/workspace.godesktop/e2e/fixtures/mock-devsy.cjsdesktop/src/main/ipc.tsdesktop/src/renderer/src/lib/ipc/events.tsdesktop/src/renderer/src/lib/types/index.tsdesktop/src/shared/cli-error.tspkg/agent/tunnel/tunnel.protopkg/agent/tunnelserver/options.gopkg/agent/tunnelserver/status_sender.gopkg/agent/tunnelserver/tunnelserver.gopkg/client/client.gopkg/client/clientimplementation/daemonclient/stop.gopkg/client/clientimplementation/daemonclient/up.gopkg/client/clientimplementation/daemonclient/up_test.gopkg/client/clientimplementation/workspace_client.gopkg/client/clientimplementation/workspace_client_status_test.gopkg/command/process_supported.gopkg/command/process_test.gopkg/compose/helper_test.gopkg/config/pathmanager.gopkg/devcontainer/build.gopkg/devcontainer/config/envelope.gopkg/devcontainer/feature/extend.gopkg/devcontainer/feature/lockfile.gopkg/devcontainer/run.gopkg/devcontainer/setup.gopkg/devcontainer/single.gopkg/devcontainer/status/log.gopkg/devcontainer/status/status.gopkg/devcontainer/status/status_test.gopkg/flags/names/names.gopkg/task/store.gopkg/task/task.gopkg/task/task_test.go
Comment on lines
+45
to
+47
| err = syscall.Kill(parsedPid, syscall.SIGKILL) | ||
| if err != nil && !errors.Is(err, syscall.ESRCH) { | ||
| return err |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Numeric PID reuse makes both escalation and the ESRCH test unsafe. A PID may belong to a different process after the original target exits, so it cannot safely identify the process for a delayed SIGKILL or a real-process ESRCH test.
pkg/command/process_supported.go#L45-L47: retain and validate a stable process identity before escalation, rather than blindly signaling the numeric PID after the delay.pkg/command/process_test.go#L33-L42: replace the stale real-PID scenario with an injected/stubbed signal operation that returnssyscall.ESRCH.
📍 Affects 2 files
pkg/command/process_supported.go#L45-L47(this comment)pkg/command/process_test.go#L33-L42
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/command/process_supported.go` around lines 45 - 47, In
pkg/command/process_supported.go lines 45-47, update the process escalation flow
around syscall.Kill to retain and validate a stable process identity before
sending SIGKILL, avoiding blind signaling of a reused numeric PID. In
pkg/command/process_test.go lines 33-42, replace the stale real-PID scenario
with an injected or stubbed signal operation that returns syscall.ESRCH, and
verify the existing handling for that result.
skevetter
marked this pull request as draft
July 29, 2026 14:31
A canceled task whose worker finished afterwards had its cancellation reason erased, because Succeed/Fail/Report mutated state without the Terminal() guard Cancel already used. Guard all three so the recorded outcome reflects why the task actually stopped. Also from review of the async workspace-up changes: - Delete holds the per-id flock across the status check and removal, so a concurrent update's atomic rename can't recreate the task. - Reject a non-positive --interval instead of panicking in NewTicker. - Serialize jsonStatusReporter writes; events arrive from both the local pipeline and the tunnel Status RPC goroutine, and interleaving would corrupt the NDJSON framing callers parse line-by-line. - Report PhaseReady completion in plain output, and route task-init failures through reportErr so JSON mode emits an error envelope. - Sniff trailing bytes left without a newline so a final status envelope is reported rather than leaking into the caller's stream as raw JSON. - Order the tunnel's default log reporter ahead of caller-supplied options so it no longer clobbers them, and forward up's reporter into the platform path. - Bound the podman probe with a context so an unreachable machine can't hang the test on the socket. - Await the prior up child's exit before overwriting its map entry, which otherwise orphaned it beyond the reach of any later kill.
skevetter
marked this pull request as ready for review
July 29, 2026 19:28
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
desktop/src/main/ipc.ts (1)
177-183: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not discard task ownership when cancellation fails.
The mapping is removed before cancellation and errors are swallowed. If cancellation fails,
workspace_stop/workspace_deleteproceeds while the detacheduptask still runs, and no later action can retry cancelling it. Retain the entry until cancellation succeeds (or the task is confirmed terminal), and propagate unexpected cancellation failures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@desktop/src/main/ipc.ts` around lines 177 - 183, The cancelActiveUp function currently deletes the active task mapping and suppresses cancellation errors before cancellation succeeds. Move activeUpTasks.delete(workspaceId) until after a successful cancellation or confirmed terminal task state, and let unexpected cli.run failures propagate so workspace_stop/workspace_delete cannot proceed while the task remains active.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@desktop/src/main/ipc.ts`:
- Around line 761-764: Update the workspace_up flow around cancelActiveUp and
the detached submission/follower registration logic to serialize cancel, submit,
and registration per wsId, preventing concurrent requests from overwriting one
another. In result, error, and exit cleanup callbacks, remove tunnelProcesses
entries only when their stored task or process identity matches the completing
session; preserve newer workspace_up sessions and their cancellability.
---
Outside diff comments:
In `@desktop/src/main/ipc.ts`:
- Around line 177-183: The cancelActiveUp function currently deletes the active
task mapping and suppresses cancellation errors before cancellation succeeds.
Move activeUpTasks.delete(workspaceId) until after a successful cancellation or
confirmed terminal task state, and let unexpected cli.run failures propagate so
workspace_stop/workspace_delete cannot proceed while the task remains active.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 818eaff1-c8a6-478d-8d2a-cbb6c1e08c52
📒 Files selected for processing (11)
cmd/workspace/task.gocmd/workspace/up/agent.gocmd/workspace/up/status.gocmd/workspace/up/up.godesktop/src/main/ipc.tspkg/client/clientimplementation/daemonclient/up.gopkg/client/clientimplementation/workspace_client.gopkg/compose/helper_test.gopkg/task/store.gopkg/task/task.gopkg/task/task_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- cmd/workspace/up/status.go
- pkg/compose/helper_test.go
- cmd/workspace/up/agent.go
- pkg/task/task.go
- pkg/client/clientimplementation/workspace_client.go
- pkg/task/store.go
- cmd/workspace/task.go
- pkg/client/clientimplementation/daemonclient/up.go
- cmd/workspace/up/up.go
skevetter
marked this pull request as draft
July 29, 2026 19:36
Two ways a detached `up` could keep running with nothing left to stop it: cancelActiveUp dropped the workspace's task mapping before cancelling and swallowed the failure, so a failed cancel left the task running while stop/delete proceeded to tear the workspace down under it. Hold the entry until the cancel lands and let the failure propagate; the up handler reports it and skips submitting a replacement, and stop/delete callers already surface the rejection as a toast. Concurrent workspace_up calls for one workspace also both passed the cancel step before either registered its task, orphaning the first. Serialize the cancel/submit/register sequence per workspace, and scope the cleanup callbacks to the task they belong to so a finishing session can't clear a newer one's entry.
The Phase/Event/Reporter model isn't devcontainer-specific; provider install/init flows want the same structured progress events. Move it to pkg/status before another pipeline imports it under a name that would misdescribe it. Import-path churn only; no behavior change.
skevetter
marked this pull request as ready for review
July 29, 2026 23:17
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/client/clientimplementation/workspace_client.go (1)
336-380: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftReconcile abandoned tasks before reporting provisioning.
latestUpTaskmaps every persisted non-terminal task toStatusProvisioning. If the detached worker is killed after task creation but before it records a terminal result, later status checks can report provisioning indefinitely for a missing workspace. Add worker liveness/heartbeat expiry or startup reconciliation before applying this override.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/client/clientimplementation/workspace_client.go` around lines 336 - 380, Update latestUpTask and taskStatusOverride so persisted non-terminal up tasks are reconciled before returning StatusProvisioning. Detect abandoned tasks using the task worker’s existing liveness/heartbeat expiry or startup reconciliation mechanism, mark them terminal/failed as appropriate, and only report provisioning for an actively running task; preserve the existing best-effort ok=false behavior when task state cannot be read.desktop/src/main/ipc.ts (1)
239-242: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
quiesceWorkspacebypasses the serialization chain, so stop/delete can still race an in-flightupsubmission.
workspace_upnow runscancel → submit → registerinsideserializePerWorkspace, butquiesceWorkspace(used byworkspace_stop/workspace_delete) callscancelActiveUpdirectly. If a stop lands while anupis betweencli.run([... "--detach"])andactiveUpTasks.set(wsId, taskId), the cancel observes no mapping, then theupregisters its task id — the detached task survives the stop/delete and keeps running against a workspace that is being torn down.Enqueue the quiesce on the same per-workspace chain.
🔒️ Proposed fix
async function quiesceWorkspace(workspaceId: string): Promise<void> { - await cancelActiveUp(workspaceId) - await Promise.all([cli.cancelFor(workspaceId), pty.cancelFor(workspaceId)]) + await serializePerWorkspace(workspaceId, async () => { + await cancelActiveUp(workspaceId) + await Promise.all([ + cli.cancelFor(workspaceId), + pty.cancelFor(workspaceId), + ]) + }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@desktop/src/main/ipc.ts` around lines 239 - 242, Update quiesceWorkspace to execute cancelActiveUp and the cli/pty cancellation operations through serializePerWorkspace for the given workspaceId, ensuring it joins the same chain used by workspace_up. Preserve the existing cancellation ordering and Promise.all behavior while preventing stop/delete from racing an in-flight submission.pkg/agent/tunnelserver/options.go (1)
80-83: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPreserve the no-op reporter when the option receives nil.
Line 82 can overwrite the
status.Nop()default with nil. A laterstatus.Enter/Failcall then invokesReporton a nil reporter and panics. Ignore nil or normalize it tostatus.Nop().Proposed fix
func WithStatusReporter(reporter status.Reporter) Option { return func(s *tunnelServer) *tunnelServer { - s.statusReporter = reporter + if reporter != nil { + s.statusReporter = reporter + } return s } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/agent/tunnelserver/options.go` around lines 80 - 83, Update WithStatusReporter so passing a nil reporter does not replace the tunnelServer’s existing status.Nop() reporter; ignore nil or normalize it to status.Nop(), while continuing to assign non-nil reporters.
🧹 Nitpick comments (1)
desktop/src/main/__tests__/ipc-up-tasks.test.ts (1)
93-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the streamed-envelope path too.
The three tests cover cancel/serialize/retain-on-failure well, but nothing exercises the
parseCliEnvelopebranch inworkspace_up:statusenvelopes emittingworkspace-status, andresult/errorenvelopes releasing the task and completing the sink before the exit callback. Driving therunStreamingonLinecallback with a few NDJSON lines would lock in that contract cheaply, and would also catch agetMainWindow()returningnullregression (the current double always returnsnull, so the send path is never hit).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@desktop/src/main/__tests__/ipc-up-tasks.test.ts` around lines 93 - 154, Extend the workspace_up test coverage to drive runStreaming’s onLine callback with NDJSON envelopes parsed by parseCliEnvelope: verify status envelopes emit workspace-status, while result and error envelopes release the task and complete the sink before the exit callback. Update the getMainWindow test double to return a window so the send path is exercised, while retaining coverage for the null-window case if supported.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@desktop/src/main/ipc.ts`:
- Around line 801-817: Validate submitted.id immediately after cli.run in the
existing try block before assigning taskId or updating activeUpTasks; if it is
missing or invalid, throw a clear error so the existing catch routes the failure
through sink.done with the standard error metadata and returns cmdId.
---
Outside diff comments:
In `@desktop/src/main/ipc.ts`:
- Around line 239-242: Update quiesceWorkspace to execute cancelActiveUp and the
cli/pty cancellation operations through serializePerWorkspace for the given
workspaceId, ensuring it joins the same chain used by workspace_up. Preserve the
existing cancellation ordering and Promise.all behavior while preventing
stop/delete from racing an in-flight submission.
In `@pkg/agent/tunnelserver/options.go`:
- Around line 80-83: Update WithStatusReporter so passing a nil reporter does
not replace the tunnelServer’s existing status.Nop() reporter; ignore nil or
normalize it to status.Nop(), while continuing to assign non-nil reporters.
In `@pkg/client/clientimplementation/workspace_client.go`:
- Around line 336-380: Update latestUpTask and taskStatusOverride so persisted
non-terminal up tasks are reconciled before returning StatusProvisioning. Detect
abandoned tasks using the task worker’s existing liveness/heartbeat expiry or
startup reconciliation mechanism, mark them terminal/failed as appropriate, and
only report provisioning for an actively running task; preserve the existing
best-effort ok=false behavior when task state cannot be read.
---
Nitpick comments:
In `@desktop/src/main/__tests__/ipc-up-tasks.test.ts`:
- Around line 93-154: Extend the workspace_up test coverage to drive
runStreaming’s onLine callback with NDJSON envelopes parsed by parseCliEnvelope:
verify status envelopes emit workspace-status, while result and error envelopes
release the task and complete the sink before the exit callback. Update the
getMainWindow test double to return a window so the send path is exercised,
while retaining coverage for the null-window case if supported.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d0634d59-0d8e-4640-86c6-543dacef7c11
📒 Files selected for processing (24)
cmd/internal/container_tunnel.gocmd/workspace/task.gocmd/workspace/up/status.gocmd/workspace/up/up.godesktop/src/main/__tests__/ipc-up-tasks.test.tsdesktop/src/main/ipc.tspkg/agent/tunnelserver/options.gopkg/agent/tunnelserver/status_sender.gopkg/agent/tunnelserver/tunnelserver.gopkg/client/client.gopkg/client/clientimplementation/daemonclient/stop.gopkg/client/clientimplementation/daemonclient/up.gopkg/client/clientimplementation/daemonclient/up_test.gopkg/client/clientimplementation/workspace_client.gopkg/devcontainer/build.gopkg/devcontainer/config/envelope.gopkg/devcontainer/run.gopkg/devcontainer/setup.gopkg/devcontainer/single.gopkg/status/log.gopkg/status/status.gopkg/status/status_test.gopkg/task/task.gopkg/task/task_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/agent/tunnelserver/tunnelserver.go
- pkg/task/task.go
- pkg/client/clientimplementation/daemonclient/up.go
- cmd/internal/container_tunnel.go
- pkg/devcontainer/setup.go
- pkg/devcontainer/config/envelope.go
- cmd/workspace/up/up.go
skevetter
marked this pull request as draft
July 29, 2026 23:25
Four findings from review of the async workspace-up changes: A detached worker killed before recording a result left its task non-terminal forever, so client.Status reported Provisioning indefinitely for a workspace with no container. Reconcile against the worker's recorded PID: a non-terminal task whose process is gone is marked failed. Tasks with no PID yet are still starting, and Fail preserves an existing terminal state so a cancellation reason isn't overwritten. quiesceWorkspace cancelled directly instead of joining the per-workspace chain workspace_up uses, so a stop landing between an up's submit and its activeUpTasks.set saw no mapping to cancel — the task then registered and outlived the teardown. Enqueue the quiesce on the same chain. WithStatusReporter installed a nil reporter over the Nop default, which would panic on the next status event. Ignore nil. workspace_up trusted the submit response's id; a missing one was stored and later used to cancel a task that doesn't exist, orphaning the real one. Also covers the streamed-envelope path in the workspace_up tests, which had no coverage: status envelopes emitting workspace-status, stderr lines not being parsed as envelopes, and result/error envelopes releasing the task.
Signed-off-by: Samuel K <skevetter@pm.me>
skevetter
marked this pull request as ready for review
July 30, 2026 00:49
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
desktop/src/main/ipc.ts (1)
877-889: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep the task registered when the log follower exits unexpectedly.
Line 878 releases
taskIdeven when noresultorerrorenvelope arrived. A failed follower can therefore orphan a still-running detached task from later cancellation. Release only on a terminal envelope (or after independently confirming the task is terminal).Proposed fix
(code, cliError) => { - releaseTask() if (tunnelProcesses.get(wsId) === child) { tunnelProcesses.delete(wsId) } if (signalledDone) returnAdd a regression test where the follower exits nonzero without an envelope, then verify the next
workspace_upcancels the original task.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@desktop/src/main/ipc.ts` around lines 877 - 889, Update the follower exit callback around releaseTask so an unexpected nonzero exit without a result or error envelope does not unregister the still-running task; release the task only after receiving a terminal envelope or independently confirming termination. Preserve cleanup of tunnelProcesses and sink reporting, and add a regression test verifying that a subsequent workspace_up cancels the original task.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/task/store.go`:
- Around line 113-122: Update Abandoned to avoid treating PID liveness alone as
worker identity: persist a worker-instance identity alongside State.PID and
verify that identity for the current process before considering the task alive.
If the identity is missing, mismatched, or the process is no longer running,
return true for a non-terminal task; preserve the existing safe behavior when
liveness or identity cannot be determined.
In `@pkg/task/task.go`:
- Around line 17-20: Remove the duplicate ErrAbandoned declaration from the
package-level var block in task.go, preserving the existing declaration in
store.go and retaining ErrCanceled unchanged.
---
Outside diff comments:
In `@desktop/src/main/ipc.ts`:
- Around line 877-889: Update the follower exit callback around releaseTask so
an unexpected nonzero exit without a result or error envelope does not
unregister the still-running task; release the task only after receiving a
terminal envelope or independently confirming termination. Preserve cleanup of
tunnelProcesses and sink reporting, and add a regression test verifying that a
subsequent workspace_up cancels the original task.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ed8f5388-9dac-41fd-a1ba-6367f893f6bc
⛔ Files ignored due to path filters (1)
pkg/agent/tunnel/tunnel.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (25)
cmd/workspace/task.gocmd/workspace/up/detach.gocmd/workspace/up/status.gocmd/workspace/up/up.godesktop/e2e/fixtures/mock-devsy.cjsdesktop/e2e/workspaces.e2e.tsdesktop/src/main/__tests__/ipc-up-tasks.test.tsdesktop/src/main/ipc.tsdesktop/src/renderer/src/lib/types/index.tspkg/agent/tunnel/tunnel.protopkg/agent/tunnelserver/options.gopkg/client/client.gopkg/client/clientimplementation/workspace_client.gopkg/client/clientimplementation/workspace_client_status_test.gopkg/config/pathmanager.gopkg/devcontainer/config/envelope.gopkg/devcontainer/feature/extend.gopkg/devcontainer/feature/lockfile.gopkg/devcontainer/run.gopkg/devcontainer/setup.gopkg/status/log.gopkg/status/status.gopkg/task/store.gopkg/task/task.gopkg/task/task_test.go
💤 Files with no reviewable changes (4)
- pkg/agent/tunnel/tunnel.proto
- cmd/workspace/up/up.go
- pkg/config/pathmanager.go
- pkg/devcontainer/setup.go
🚧 Files skipped from review as they are similar to previous changes (9)
- pkg/status/log.go
- desktop/src/renderer/src/lib/types/index.ts
- pkg/devcontainer/config/envelope.go
- cmd/workspace/up/detach.go
- pkg/client/client.go
- pkg/devcontainer/feature/lockfile.go
- pkg/devcontainer/run.go
- pkg/devcontainer/feature/extend.go
- pkg/status/status.go
Comment on lines
+17
to
+20
| var ( | ||
| ErrCanceled = errors.New("canceled") | ||
| ErrAbandoned = errors.New("worker exited without recording a result") | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the duplicate ErrAbandoned declaration.
pkg/task/store.go:19 already declares the package-level ErrAbandoned identifier. Defining it again here causes a Go compile error; retain a single declaration in the task package.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/task/task.go` around lines 17 - 20, Remove the duplicate ErrAbandoned
declaration from the package-level var block in task.go, preserving the existing
declaration in store.go and retaining ErrCanceled unchanged.
skevetter
marked this pull request as draft
July 30, 2026 01:01
PID-based liveness had two problems: a recycled PID makes a dead worker look alive, so an abandoned task could still be reported as running; and command.IsRunning panics on Windows, so Abandoned would crash there. Have the worker hold an exclusive flock on <id>.json.worker.lock for its lifetime instead. The kernel releases it however the process ends, so liveness becomes "is the lock acquirable" — no PID to be reused, and it works on every platform. Separate from the read-modify-write lock in withLock, which is held only per update. HoldWorkerLock also rejects a second worker for the same task. Also from review: the log follower exiting no longer releases the task. The follower dying says nothing about the detached worker, and releasing there left a live task with no id to cancel it by. Restores the podman reachability probe in pkg/compose, dropped by an earlier comment cleanup; without it the test asserts against whichever compose backend the helper falls back to instead of skipping.
Abandoned released the worker lock before Reconcile wrote ErrAbandoned. A replacement worker could claim it in that window and then be marked failed while actually running — withLock guards state writes, not this lock, so it could not prevent that. Reconcile now claims the lock itself and holds it through the write, and re-reads state under it so a result the worker recorded in the meantime stands instead of being overwritten. The regression test needs a seam: the window is between two internal calls, so a test can only act inside it via a hook. Without one the test passes against the buggy version, which is worse than no test. Also release worker locks via t.Cleanup so an fd stays closed when an assertion fails, which can otherwise block TempDir removal.
write() renamed a temp file without syncing it or the directory. rename(2) stops a reader seeing a torn file, but after a crash the entry could point at unflushed data — and a task that loses its terminal update reads as permanently in-flight, which is exactly the state reconciliation exists to avoid. Sync the temp file before the rename and the directory after, matching pkg/secrets/atomicwrite.go. pkg/provider's helper deliberately skips the directory sync because its callers re-resolve on the next run; task state has no such fallback. Costs ~8ms per update, so ~160ms across a workspace up's ~20 status events — immaterial against a multi-minute provision, and the writes were already serialized under the per-task lock.
skevetter
marked this pull request as ready for review
July 30, 2026 04:53
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/task/export_test_helpers.go (1)
1-17: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRename file so it's excluded from production builds.
export_test_helpers.godoes not end in_test.go, so Go compiles it into every regular build —ReleaseWorkerLockForTest()andSetAfterClaimForTest()(which force-drop a held worker lock / inject a callback intoReconcile's locked critical section) ship as exported production API, not gated behindgo test. These directly bypass the worker-lock exclusivity this PR relies on to prevent duplicate detached workers.🔒 Proposed fix — use the real `_test.go` suffix
-pkg/task/export_test_helpers.go +pkg/task/export_test.go🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/task/export_test_helpers.go` around lines 1 - 17, Rename export_test_helpers.go to a filename ending in _test.go so ReleaseWorkerLockForTest and SetAfterClaimForTest are compiled only during tests and excluded from production builds.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/compose/helper_test.go`:
- Line 191: Update the reachability probe in the affected test to use the
existing testPodmanCmd symbol instead of the hardcoded "podman" executable,
while preserving the current compose version check and skip/failure behavior.
---
Outside diff comments:
In `@pkg/task/export_test_helpers.go`:
- Around line 1-17: Rename export_test_helpers.go to a filename ending in
_test.go so ReleaseWorkerLockForTest and SetAfterClaimForTest are compiled only
during tests and excluded from production builds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 890d35d8-39f8-47e4-adba-cb41be6f9c97
📒 Files selected for processing (12)
cmd/workspace/up/detach.gocmd/workspace/up/up.godesktop/src/main/__tests__/ipc-up-tasks.test.tsdesktop/src/main/ipc.tspkg/client/clientimplementation/workspace_client_status_test.gopkg/compose/helper_test.gopkg/config/pathmanager.gopkg/devcontainer/config/envelope.gopkg/task/export_test_helpers.gopkg/task/store.gopkg/task/task.gopkg/task/task_test.go
💤 Files with no reviewable changes (1)
- pkg/devcontainer/config/envelope.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/workspace/up/up.go
- pkg/config/pathmanager.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
workspace up currently runs synchronously end-to-end, streaming a single JSON blob to the caller only once the container is ready. That blocks the CLI/desktop for the whole provisioning window and gives up all in-progress state on a crash or disconnect.
Adds a submit/poll durable execution model alongside the existing synchronous flow:
pkg/task: a JSON-file task store (atomic writes, cross-process flock) recording status/PID/result for background-launched work.workspace up --detachre-execs itself as a background process and returns a task ID immediately.workspace task list/get/logs/cancel/rm: standard verb commands (kubectl/docker/gh-style) to manage submitted tasks.pkg/devcontainer/status: a structured Phase/Event/Reporter model threaded throughRunner.Upand the devcontainer build/run/setup pipeline, replacing ad hoc log lines with typed progress events.StatusUpdateRPC on the agent tunnel streams those events back to the CLI host live instead of waiting for the final result.See
docs/rfcs/async-workspace-up.mdfor the full design.Includes an unrelated one-line test fix in
pkg/compose/helper_test.go(skip when the local podman machine is unreachable, a machine-local CI-vs-local flake noticed during this work).Summary by CodeRabbit
workspace up --detachwith task-based progress you can query, follow logs, cancel, and remove.workspace upruns in the background.kill.