fix(gateway): close chat-start + sandbox lifecycle races - #159
Merged
Conversation
Multi-agent bug hunt over session start and the sandbox edge cases; every CONFIRMED finding fixed, each with the concrete interleaving that triggered it: - create() TOCTOU: the one-session-per-conversation dedup scan and the insert straddled the multi-second credential-baking await, so two tabs / a reload double-provisioned one conversation (two shims in one attached box, the second rotating the first's preview token). create() now re-checks and RESERVES the slot synchronously before baking; a prepare() failure drops the reservation and unblocks awaiters. - close() during provisioning orphaned the sandbox: _teardown short-circuits on runtime=None, while the background _provision kept building a box + shim + connection on a session no longer in _sessions — invisible to the reaper, leaking against the per-user cap. _provision now reclaims its runtime when the session was closed mid-provision. - Concurrent first-run in a fresh workspace built TWO unified boxes: the loser lost the Convex link race and ran its whole chat in a throwaway box whose files were silently discarded. First-box creation now serializes on a per-workspace lock; the loser re-resolves and attaches to the winner's box. - Concurrent revive double-provisioned: two turns on a stale session both failed healthz before either flipped to "reviving", so both re-launched the sandbox and the second orphaned the first's shim. _ensure_alive now serializes under session.lock with a health re-check. - set_config_option / switch_harness ran unguarded: _claim_parked (new-chat steal) and reset_conversation (rewind) could tear the runtime out from under them mid-call. Both now hold turn_guard like prompt does. - Transient recover() failures were tombstoned as "gone": except DaytonaError swallowed 429/5xx/timeout/conn-reset and rewrote them as DaytonaNotFoundError — on revive that DELETED a still-alive owned box and replaced it empty. Now only non-transient failures mark the box gone; transient ones re-raise for the caller's backoff. - A stale/foreign workspace sandbox link was never unlinked: createInternal's lost-race guard then declined every fresh box, so the workspace churned a new box per session forever. _resolve_sandbox_plan now unlinks before recreating (mirrors the DaytonaNotFoundError heal). - acp_provision_timeout_seconds 180→360: the wall-clock was smaller than the per-op budgets it wraps (archived restore 180s, first-attach bootstrap 300s), so a legitimate cold start reliably timed out on its first attempt. New regression tests: concurrent-create dedup + failed-reservation cleanup (test_session_start_races.py), concurrent first-provision serialization (unify), transient-vs-definitive recover() split (errored-sandbox). fastapi: 374 passed.
…s own link Self-review catch on the previous commit: the stale/foreign-link branch of _resolve_sandbox_plan called sandboxes:removeByDaytonaId, which deletes EVERY Convex row for that Daytona id and unlinks the row owners' harnesses/ workspaces. That blast radius is only safe when the box is provably gone from Daytona (the DaytonaNotFoundError heal). In the foreign-link branch the box may be alive and another user's (duplicate rows resolving to a different owner), so user B starting a chat could have destroyed user A's records. New workspaces:unlinkSandboxInternal (deploy-key only) clears just the one workspace's sandboxId — which is all createInternal's lost-race guard reads, so the workspace still un-sticks and links a fresh unified box. Convex tests pin the narrow blast radius (row + harness links survive) and the un-stick; the Python test now asserts removeByDaytonaId is NOT called on this path. convex: 221 passed, tsc clean. fastapi: 374 passed.
…vation Follow-up self-audit on the reservation change: a DELETE arriving while create() bakes credentials pops the reserved session, but create() would still schedule _provision for it — building a sandbox only for the _provision-side reclaim to destroy it. Check the reservation is still live before scheduling; set ready_event so any dedup'd awaiter unblocks.
A 17-agent adversarial workflow reviewed the race-fix commit for bugs the fixes themselves introduced; every confirmed finding addressed: - _teardown now detaches session.runtime SYNCHRONOUSLY (before any await) and passes it explicitly to _park_runtime/_destroy_runtime. Closes the window where the closed-mid-provision reclaim in _provision could destroy a runtime that close() had just parked — one a new session may already have adopted. _register_workspace_sandbox and the scratch-register tail snapshot/guard the runtime for the same reason. - recover() disambiguation: the SDK raises the SAME bare status-less DaytonaError for an API blip AND for recover's deterministic outcome failure (the box landed back in error/build_failed). Blips must retry, but the deterministic case retried forever and bricked the workspace (the previous commit's regression). After a transient-looking failure we now re-fetch the state: back-in-error → the 'gone' heal; otherwise re-raise for retry. - The vanished-box heal now RE-ENTERS _provision_once instead of provisioning inline, so the replacement unified box is created under the per-workspace lock (two sessions healing the same vanished box raced the link again). The locked branch releases the lock before running a re-resolved ATTACH, so the heal's re-entry can never deadlock on a self-held lock. - _provision_once bails early for a session no longer in _sessions (closed after create() scheduled it, or between retry attempts) — a zombie provision could steal a healthy session's warm runtime only to have it destroyed. - workspaces:unlinkSandboxInternal is now userId-scoped: workspace_id is client-supplied, so user B could otherwise unlink user A's workspace. (createInternal already owner-guards both its links.) - create()'s prepare-failure path sets session.error before ready_event, so a deduped awaiter surfaces the actual user-facing message. fastapi: 375 passed. convex: 221 passed, tsc + biome clean.
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
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.
Systematic bug hunt over chat start and the sandbox edge cases: three parallel finder agents (concurrency, sandbox-vanished/error-state, workspace-unify) + a 17-agent adversarial verification workflow over the fixes themselves. Every CONFIRMED finding is fixed, each with the concrete interleaving that triggered it and a regression test where practical.
The bugs (4 commits)
Chat-start races (
session_manager.py):session.error, and unblocks awaiters._teardownshort-circuits onruntime=Nonewhile the background provision kept building a box invisible to the reaper (leaking against the per-user cap)._provisionnow reclaims it, and create()/_provision_onceskip dead reservations entirely.session.lockwith a health re-check._claim_parked) or rewind (reset_conversation) could tear the runtime out from under them mid-call. Both now holdturn_guard, likeprompt.Sandbox lifecycle:
daytona_service.py) — a 429/5xx/timeout during recovery made revive delete a still-alive owned box and replace it empty (data loss). Now disambiguated: after a transient-looking failure the state is re-fetched — landed-back-in-error → the "gone" heal (retrying can never fix it); a genuine blip → re-raise for backoff.createInternal's lost-race guard then declined every fresh box, churning a new box per session forever. Now unlinks via a newworkspaces:unlinkSandboxInternalscoped to the one workspace link and userId-gated (workspace_id is client-supplied; the broadremoveByDaytonaIdstays reserved for provably-gone boxes)._teardownnow detachessession.runtimesynchronously before any await, so the closed-mid-provision reclaim can never destroy a runtime that was just parked (and possibly adopted by a new session).Verification
f408920,b23db92,a8710c8).Known-deferred (LOW / not confirmed)
queue_promptracing the done-yield suspension (queued message lost in a sub-tick window)._take_over_attachedtearing down a mid-turn holder (partially by design — attach takeover is last-writer-wins)._ensure_running; 30s_running_cachestaleness window.