feat(workspaces): Resume long snapshot builds across turn yields - #1599
feat(workspaces): Resume long snapshot builds across turn yields#1599sentry-junior[bot] wants to merge 27 commits into
Conversation
Co-Authored-By: David Cramer <david@sentry.io>
Persist runtime, dependency count, and previous snapshot ids with the Workspace snapshot record. Keep retired ids for later GC. Co-Authored-By: David Cramer <david@sentry.io>
Snapshot build and ready artifacts are their own lifecycle. Keep prior ready rows for later Vercel GC instead of packing history onto recipes. Co-Authored-By: David Cramer <david@sentry.io>
Start only creates the builder. Install/clone and detached setup run on later check-ins. SQL ready rows boot when Redis misses, and builders stop after ready or failed. Co-Authored-By: David Cramer <david@sentry.io>
Move snapshot SQL and the multi-slice builder under sandbox/snapshot. switchWorkspace waits in a control-plane loop and throws CooperativeTurnYieldError near the host deadline so conversation work requeues like an agent turn. Resume attaches the same SQL job. Co-Authored-By: David Cramer <david@sentry.io>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Throwing CooperativeTurnYieldError mid-tool parked a non-continuable assistant toolCall and failed the next wake on no_progress. Return timed_out/building from switchWorkspace instead, keep waiting errors through setup wrap, and always advance one slice before soft yield. Co-Authored-By: David Cramer <david@sentry.io>
![sentry-junior[bot]](https://avatars.githubusercontent.com/in/2960336?s=60&v=4){kind=small}
There was a problem hiding this comment.
Adversarial self-review — a few real holes before this is safe for cold Sentry:
-
continueBuildis still one long function slice. Start is create-only (good), but the next check-in still awaits install + full repo clone + detach setup under the Redis lock. That was half the original timeout. If the function dies mid-prep, SQL staysbuildingwithsandboxNameand nocommandId, and the next wake re-runs prep from scratch on a half-written builder. Detach setup alone is not enough — clone/install need their own checkpoints (or a detached prep script that writes status files), same as setup. -
Legacy SQL ready rows won't boot. Migration backfills
runtimeanddependency_countasNULL, andsnapshotFromRowrequires both. After deploy + Redis miss, previously ready workspace snapshots look missing and rebuild. Backfill real values, or treat null runtime/deps as valid legacy ready. -
Soft-yield continuation is model-mediated.
timed_out+status: buildingis the right tool boundary (mid-toolCooperativeTurnYieldErrorwas wrong). But nothing durable keeps waiting unless the agent callsswitchWorkspaceagain after yield. That fails the "continue the wait across soft deadline" bar and also blocks unattended prebuild. Minimum: after this tool result, the runtime should keep the turn/work requeued on the same job until ready/failed/budget exhausted, not rely on the model to re-enter.
Bump the pre-scheduler migration residual count for 0032 and cast the sandbox update mock call so typecheck accepts the workspace prep test.
Drop the generic status field and tool-description retry prose, move waiting-error ownership into snapshot/waiting-error, and stop re-exporting snapshot writes from the recipe store. Co-Authored-By: David Cramer <david@sentry.io>
Checkpoint prep with durable builder markers so mid-prep death resumes cleanly. Backfill legacy SQL ready runtime/deps and boot null rows. Host-continue switchWorkspace waits across soft yields without model mediation. Co-Authored-By: David Cramer <david@sentry.io>
Co-Authored-By: David Cramer <david@sentry.io>
Assert scheduler row adoption behavior without hardcoding how many core migrations remain after the pre-scheduler baseline. Co-Authored-By: David Cramer <david@sentry.io>
Ship junior_snapshots with build_phase in a single 0032 migration instead of a create-then-alter pair on this branch. Co-Authored-By: David Cramer <david@sentry.io>
Resolve boots the ready junior_snapshots row for the current recipe profile instead of the newest ready row of any profile, so reverting setup_script reuses a retained snapshot when it still exists. Co-Authored-By: David Cramer <david@sentry.io>
Co-Authored-By: David Cramer <david@sentry.io>
Drop the @vercel/sandbox mock so integration coverage matches policy (only Slack + LLMs may be faked). Wire sandbox secrets into junior-tests and skip the suite when credentials are absent. Co-Authored-By: David Cramer <david@sentry.io>
State the integration-test boundary precisely in policy and harness docs, and reject any vi.mock/vi.doMock in integration tests. Co-Authored-By: David Cramer <david@sentry.io>
Vercel 5xx on builder runCommand was marking the snapshot job failed. Keep the SQL phase and retry in the wait loop instead.
Builder Sandbox.get used resume:false, so later slices had no active session and runCommand failed with API 500s. Resume the named builder and simplify the live lifecycle test to wait through real prep.
Co-Authored-By: David Cramer <david@sentry.io>
There was a problem hiding this comment.
Fresh pass on the current tip (b07dd16). The multi-slice control plane, profile-keyed ready lookup, host-owned wait continue, and real-sandbox lifecycle coverage are in good shape. A few merge blockers remain:
-
Product timeout is still 1h after an explicit 24h ask. In
snapshot/workspace.ts,BUILD_TIMEOUT_MS/ buildertimeout/ error text are all one hour. Thread request was “the whole thing to have a 24 hour timeout” (builder lifetime + wall-clock budget from SQLstartedAt). Right now a slow cold Sentry bootstrap dies at 1h even though the VM model was sold as multi-hour. Set the product budget to 24h; keep check-ins from extending it. -
CI is still sized as if the live cases run for the product budget.
.github/workflows/ci.ymlistimeout-minutes: 135with a “two live one-hour cases” comment, and the lifecycle suite isLIVE_TEST_TIMEOUT_MS = 65m. Those cases only write a marker script; they should keep a realistic prep ceiling (previous ~40m/45m was about docker install cost), not trackBUILD_TIMEOUT_MS. Don’t make default CI look like a deliberate hour-long soak. -
Missing ready snapshot falls off the durable path. In
session.tscreateSandboxFromResolvedSnapshot,isMissingErrorrebuilds via baselineresolveSnapshot(... prepareWorkspace)— i.e. inline clone/setup under the turn signal — even whenworkspaceis set. After Redis/SQL say ready and Vercel deleted the snapshot (or GC later), cold switch regresses to the original timeout failure mode. Rebuild should re-enterresolveWorkspaceSnapshot/ the multi-slice builder (and invalidate the stale ready row), not the baseline inline path.
Also worth fixing while here: clearNonReadySnapshots on recipe change deletes building rows without stopBuilder, so abandoned persistent builders leak until provider timeout. Stop-or-best-effort-stop before delete.
PR body is also stale (still says host re-invoke is out of scope; still describes a 24h builder while code is 1h) — refresh when the above lands.
Product snapshot budget stays one hour. Restore live-test ceilings to prep cost, rebuild missing ready snapshots on the multi-slice path, and stop abandoned builders when a recipe changes. Co-Authored-By: David Cramer <david@sentry.io>
There was a problem hiding this comment.
Correction on the previous review: product budget is intentionally 1h, not 24h. Misread the ask.
Landed on tip for the remaining real items:
- CI live ceilings back to prep cost (
45mjob /40mcase), not product budget - missing ready snapshot rebuilds via durable multi-slice path (invalidate stale Redis/SQL pointers first)
- recipe change stops abandoned builders before deleting non-ready rows
- PR body refreshed for 1h + host continue
Move resolved-snapshot boot/rebuild into snapshot/boot so the missing ready path stays durable without blowing the 1000-line session module.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Match wait yield to worker shouldYield when present, and poll the host continue loop instead of immediately re-entering switchWorkspace.
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
finishBuild now matches continueBuild: transient Sandbox API errors retry, permanent errors (missing builder/command) mark the SQL row failed and stop retrying the dead sandbox until the 1h wall clock. Add a live lifecycle case that puts a real builder on the poll path, stops the VM, and proves the next acquire fails the build instead of stalling. Co-Authored-By: David Cramer <david@sentry.io>
AbortSignal cancellation during finish/continue/start must not markFailed or invent a failed SQL row. Detect abort like other cancel paths and leave the durable builder for the next check-in; permanent errors still fail.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 778e653. Configure here.
isAbortError now recognizes real signal.reason values (API Turn cancelled, executeAgentRun timed out after …). Build catch paths also treat signal.aborted as cancel so bare aborts do not markFailed a healthy job.
| ); | ||
| // TODO: garbage-collect retired Vercel snapshots once retention policy exists. | ||
| await db | ||
| .delete(juniorSnapshots) | ||
| .where( | ||
| and( | ||
| eq(juniorSnapshots.workspaceId, workspaceId), | ||
| ne(juniorSnapshots.status, "ready"), | ||
| ), | ||
| ); | ||
| return builders | ||
| .map((row) => row.sandboxName) | ||
| .filter((name): name is string => Boolean(name)); | ||
| } | ||
|
|
||
| /** | ||
| * Drop one ready SQL row whose Vercel snapshot id is gone. | ||
| * Prior ready rows for other ids stay for later GC. | ||
| */ | ||
| export async function invalidateMissingReadySnapshot(params: { | ||
| workspaceId: string; | ||
| profileHash: string; | ||
| snapshotId: string; |
There was a problem hiding this comment.
clearNonReadySnapshots can delete a row without returning its builder for cleanup
clearNonReadySnapshots selects sandboxName in one statement and deletes in a second. Under READ COMMITTED, a concurrent startBuild insert that commits between the two statements is deleted without its sandboxName being captured, leaving the Vercel builder running but untracked.
Evidence
clearNonReadySnapshotsissues aSELECTthen a separateDELETEfor non-ready rows.startBuildcallsSandbox.create()and then commits its own transaction viasetWorkspaceSnapshotBuild().- Under PostgreSQL READ COMMITTED, a committed insert between the two statements is visible to the
DELETEbut not to the precedingSELECT. updateWorkspacestops only the builders captured by theSELECT; the deleted-but-missed builder is not stopped and leaks until Vercel's 1-hour builder timeout expires.
Identified by Warden · code-review · D2Q-EA8
There was a problem hiding this comment.
Good catch — select-then-delete race was real.
Fixed in the tip:
clearNonReadySnapshotsnow uses a singleDELETE … RETURNINGso every deleted row’s builder name is captured for stop, including concurrentstartBuildinserts
|
|
||
| async function markFailed( | ||
| workspace: Workspace, | ||
| build: WorkspaceSnapshotBuild, | ||
| error: string, | ||
| ): Promise<void> { | ||
| await setWorkspaceSnapshotBuild(workspace.id, { | ||
| ...build, | ||
| status: "failed", | ||
| error, | ||
| }); | ||
| await stopBuilder(build.sandboxName); |
There was a problem hiding this comment.
markFailed leaks builder sandbox if DB write fails
If setWorkspaceSnapshotBuild throws, stopBuilder never runs, leaving a persistent sandbox running untracked until it times out.
Evidence
markFailedawaitssetWorkspaceSnapshotBuildthenstopBuilderwith no guard or finally block.setWorkspaceSnapshotBuildexecutes a SQL transaction that can throw on connection errors.- When the write fails,
stopBuilderis skipped and the builder VM continues running. - The leak is bounded by the 1-hour sandbox timeout, but consumes resources and incurs cost.
Identified by Warden · code-review · NBQ-FFK
There was a problem hiding this comment.
Good catch — builder stop was gated on the SQL write.
Fixed in the tip:
markFailedstops the builder in afinallyso a failedsetWorkspaceSnapshotBuildstill tears down the VM
| }): Promise<Snapshot> { | ||
| const value = profile.create(params.runtime, params.workspace); | ||
| if (value) { | ||
| await clearCachedSnapshot(value.hash); | ||
| await invalidateMissingReadySnapshot({ | ||
| workspaceId: params.workspace.id, | ||
| profileHash: value.hash, | ||
| snapshotId: params.snapshotId, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Rebuild path leaves stale SQL pointer when DB deletion fails
rebuildMissingWorkspaceSnapshot clears the Redis cache before deleting the stale SQL row. If the deletion throws, the bad snapshot row remains and the next resolve will boot from it again, looping on missing snapshot errors.
Evidence
rebuildMissingWorkspaceSnapshotcallsclearCachedSnapshotbeforeinvalidateMissingReadySnapshot.invalidateMissingReadySnapshotperforms a SQL transaction that can fail.- On failure the stale
readyrow stays in the database. snapshotFromSqllater loads that row and returns it as valid becausestatus === "ready".boot.tstries to boot the missing snapshot id, fails, and repeats the rebuild cycle.
Identified by Warden · code-review · X7Y-CKB
There was a problem hiding this comment.
Good catch — Redis-before-SQL left a stale ready pointer on delete failure.
Fixed in the tip:
rebuildMissingWorkspaceSnapshotinvalidates the SQL ready row first, then clears Redis- if the SQL delete throws, resolve still sees the same ready row and can retry invalidate rather than looping after a silent Redis clear
| await setCachedSnapshot({ | ||
| profileHash: ready.profileHash, | ||
| snapshotId: ready.id, | ||
| runtime: ready.runtime, | ||
| createdAtMs: ready.generatedAt.getTime(), | ||
| dependencyCount: ready.dependencyCount, | ||
| buildDurationMs: ready.buildDurationMs, | ||
| }); |
There was a problem hiding this comment.
Redis failure during snapshotFromSql aborts a valid snapshot boot
snapshotFromSql attempts to refresh the Redis hot cache with setCachedSnapshot and lets any Redis error propagate. Because SQL is the durable source of truth, a cache-write failure should not prevent booting from the ready row.
Evidence
snapshotFromSqlawaitssetCachedSnapshotwithout any try/catch guard.setCachedSnapshotwrites to Redis viastateAdapter.set; network issues can throw.- If it throws, the function aborts instead of returning the valid SQL snapshot.
- The caller propagates the error as a non-transient failure, causing the workspace switch to fail.
Identified by Warden · code-review · GS8-LEY
There was a problem hiding this comment.
Good catch — Redis warm was on the critical path.
Fixed in the tip:
snapshotFromSqltreatssetCachedSnapshotas best-effort- a Redis write failure still returns the durable SQL ready snapshot for boot
- clearNonReadySnapshots uses DELETE … RETURNING so concurrent builds cannot be deleted without capturing the builder name - rebuildMissingWorkspaceSnapshot invalidates SQL before Redis - snapshotFromSql treats Redis warm as best-effort - markFailed always stops the builder even if the SQL write fails
Cold Workspace setup can exceed one agent-turn budget. Full Sentry bootstrap (clone +
devenv sync) dies with the function even though the sandbox VM can run longer.This change stops awaiting full setup on
switchWorkspace.Behavior
build_phasecheckpoints each slice; wall-clock budget is fixed from SQLstartedAt(check-ins do not extend it)timed_out+waiting: workspace_snapshotat a toolResult boundarySchema
junior_snapshotstable owns snapshot lifecycle (building|failed|ready)junior_workspacesno longer store snapshot stateStructure
sandbox/snapshot/Not in this PR
Requested by David Cramer.
--
View Junior Session [Sentry]