fix(e2b): prevent double-execute on transport-retry; bound cancel-loop kill; close create race - #2327
Merged
Merged
Conversation
…p kill; close create race The E2B transport-retry loop re-ran run.py from scratch in a new sandbox after a transient transport drop (Server disconnected / ConnectionTerminated / HPACK / reset / 503). The only safety net was a best-effort sandbox.kill() over the SAME dropped transport, swallowed on failure. If that kill silently failed, the original sandbox kept running (real side effects: emails, CRM writes, sends) while the retry ran a second time -> duplicate customer side effects. Fix: - Never re-dispatch after a post-command-start drop until the ORIGINAL sandbox is CONFIRMED terminated. _run_in_sandbox now tracks whether the worker command started and, on transport unwind, runs a bounded kill-and-verify (_terminate_and_confirm_dead) using the control-plane kill (a separate transport from the dropped envd stream) plus an is_running() liveness probe. It stamps the in-flight exception with started + confirmed-dead. run() retries only when the command never started (create/upload phase: no side effects yet) OR the original is confirmed dead; otherwise it returns the new terminal operator-action code `sandbox_liveness_unconfirmed` (retryable=False) instead of blind-retrying. Bounded budget replaces the unbounded 60s kill on a dead transport. - Bound the shutdown/worker-deletion cancel loop: each cancel_sandbox kill is time-bounded and the loop stops issuing kills once the overall budget is spent (stragglers handled by thread-join + startup recovery), so one hung kill can no longer blow the whole shutdown budget. - Close the post-cancel create race: _run_in_sandbox re-checks cancel-requested immediately before spawning, so a run cancelled while it sat in pre-sandbox setup cannot spawn an orphan the cancel sweep already believes it terminated. - Register sandbox_liveness_unconfirmed across the taxonomies (permanent/ non-retryable, crash category, OPS alert, operator headline). Residual (documented, not implemented): action-boundary idempotency keys keyed on run_id threaded into side-effecting connector calls. Those calls run INSIDE the sandbox via the worker's own code, so this is a large cross-cutting change; the confirmed-dead gate above removes the double-execution that would be needed to trigger a duplicate, so this is a defense-in-depth follow-up. Tests: no-double-execute when kill unconfirmed (ends sandbox_liveness_unconfirmed, 1 sandbox); retry proceeds when confirmed dead (2 sandboxes, success); post-cancel create race spawns nothing; cancel-loop kill is time-bounded. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ncel gate, retry veto, unified cancel deadline Adversarial Codex review found 3 P1 + 1 P2 in the first cut: - P1: a transport drop on the post-completion result.json read retried once the sandbox was confirmed dead, but the worker had already RUN TO COMPLETION so its side effects were done -> retry duplicated them. Fix: track worker_command_completed (set when sandbox.commands.run returns) and never retry a completed run regardless of liveness. - P1: create-race window between the pre-spawn cancel check and _register_sandbox (cancel during the up-to-120s Sandbox.create was missed by the cancel sweep and by the pre-spawn check). Fix: add a second cancel gate immediately before the worker command, after registration; together with registration this closes the window. - P1: a worker manifest retry.on:[sandbox_liveness_unconfirmed] (or retryable=True) could override the permanent-code check and re-dispatch a liveness-unconfirmed run. Fix: _NEVER_RETRY_ERROR_CODES vetoes retry before the manifest-exact check. - P2: the cancel loop gave the kill pass and the join pass each a full timeout_seconds (total ~2x). Fix: one shared overall_deadline for both passes. Tests: completed-worker + result-read drop -> no retry; cancel during setup after registration -> no command run; liveness-unconfirmed never retried even with manifest opt-in. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nd 2) Codex round 2 confirmed the round-1 fixes but flagged a residual P1: the confirmed-dead -> retry path (Phase B) can still DUPLICATE a side effect the worker committed before the transport dropped. Killing the original stops concurrent execution but cannot undo a committed email/CRM write/send, and the platform does not yet enforce action-level idempotency at the connector boundary. Resolution (safety over availability): refuse to auto-retry ANY run whose worker command started, whether or not the sandbox is confirmed dead. Retry stays enabled ONLY for create/upload-phase drops (command never started -> no worker code ran -> no side effects possible), which still covers the common sandbox-create / HPACK / header-block transport drops. Post-command-start drops end terminally as sandbox_liveness_unconfirmed (operator action). The bounded kill-and-verify still runs to stop the original promptly. This is stricter than the original task's "confirmed-dead -> retry" test gate; it trades some auto-recovery on mid-run transport blips for a hard guarantee that a started worker is never re-executed. Re-enabling safe post-start retry requires durable run_id+operation idempotency keys threaded into connector calls (the documented residual / follow-up). Tests updated: post-command-start drop (even confirmed dead) -> no retry, terminal; create-phase drop -> retry proceeds to success. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codex round 3 confirmed the in-driver guard holds but found the double-execute moved UP a layer, two P1s: - P1: an opaque, NON-transient exception raised after the worker command started escaped the transport-retry handling and fell through to the driver's outer handler, which classified it as a RETRYABLE code (e2b_sandbox_error). The run_service retry scheduler then re-dispatched the whole run in a fresh sandbox, re-running the worker. Fix: the outer handler now forces the non-retryable sandbox_liveness_unconfirmed terminal whenever the command started (stamped on the exception). - P1: graceful-shutdown requeue (_requeue_interrupted_run_in_place) re-queued every joined active run regardless of execution stage, so a run whose worker command started got re-run on the next drain. Fix: the driver signals run_service (mark_run_worker_command_started) when the command starts; the shutdown requeue now skips runs with worker_command_started=True (left cancelled for operator action). Pre-command runs still requeue safely. The invariant is now enforced at every layer: once the worker command starts, no path (driver transport-retry, driver outer handler, run_service retry scheduler, graceful-shutdown requeue) re-runs it. Startup recovery was already safe (only requeues dispatch-orphans with no sandbox logs). Tests: opaque post-start failure -> non-retryable terminal, one execution; shutdown does not requeue a started-worker run but does requeue a pre-command run. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…mp (Codex round 4) Codex round 4: a failure that unwinds AFTER the worker command completed and keep_warm was set would return the sandbox to the warm pool (pooled=True), skipping the exception-stamping block. run()'s outer handler then fell back to a retryable e2b_sandbox_error, which the run_service scheduler re-dispatches -> duplicate. Fix: stamp _e2b_worker_command_started/_completed on EVERY post-command-start unwind BEFORE the warm-pool branch, and never return a sandbox to the warm pool while unwinding a failure (only pool cleanly-completed runs). Test: warm-pool enabled + a post-completion failure -> non-retryable sandbox_liveness_unconfirmed terminal and the sandbox is NOT pooled. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… bounded cancel-loop kill These two tests live only in the CI-blocking root `tests/` suite (not `apps/api/tests`), so the earlier apps/api-focused runs missed them. Both are test-only fixes to match the (correct) product behavior; no product code changed. - test_e2b_artifact_collection.py::test_e2b_cancelled_sandbox_exception_reads_active_repository: the new pre-spawn create-race guard short-circuited this test (repo reported cancel_requested from the start), so it stopped exercising the post-spawn terminate + active-repo read it was written for. Moved the cancellation to AFTER the sandbox spawns (the FakeCancelledSandbox command hook flips the run's cancel flag when the worker command runs), and added an assertion that a sandbox actually spawned. Still asserts cancelled/user_cancel via the active-repository read. - test_s35_shutdown.py::test_request_active_run_shutdown_marks_cancel_and_kills_sandbox: updated the mocked cancel_sandbox to accept the new bounded `request_timeout` kwarg the cancel loop now passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…no global monotonic patch) test_cancel_loop_is_time_bounded_on_hung_kills patched the global run_service.time.monotonic with a fake clock, which is fragile under full-suite runs (a prior test's leftover state could break the fake clock, making the loop skip all kills -> calls==[]). It passed in isolation but failed in the full apps/api/tests serial run. Rewrote it to use REAL wall-clock: the first fake cancel sleeps longer than the small budget, so the loop provably stops issuing kills after the budget is spent. No product change; the bound itself is unchanged (in prod timeout_seconds is 30-75s so the first kill always fires). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
federicodeponte
marked this pull request as ready for review
July 24, 2026 21:15
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.
Problem
The E2B transport-retry loop (
e2b_driver.run) re-ranrun.pyfrom scratch in a NEW sandbox after a transient transport drop (Server disconnected / ConnectionTerminated / HPACK / reset / 503). The only safety net was a best-effortsandbox.kill()over the SAME dropped transport, swallowed on failure. If that kill silently failed, the original sandbox kept running (real customer side effects: emails, CRM writes, sends) while the retry ran a second time -> duplicate side effects.Fix
_run_in_sandboxtracks whether the worker command started and, on a transport unwind, runs a bounded kill-and-verify (_terminate_and_confirm_dead): control-planekill()(a DELETE against api.e2b.dev, a DIFFERENT transport than the dropped envd command stream) is authoritative proof of termination; if it keeps raising, fall back to pollingis_running()within a bounded budget (not the unbounded 60s control timeout on a dead transport). It stamps the in-flight exception withcommand_started+confirmed_dead.run()retries ONLY when the command never started (create/upload phase: no side effects yet) OR the original is confirmed dead. Otherwise it returns the new terminal operator-action codesandbox_liveness_unconfirmed(retryable=False) instead of blind-retrying.cancel_sandboxkill and stops issuing kills once the overall budget is spent (stragglers handled by thread-join + startup recovery), so one hung kill can no longer blow the whole shutdown budget._run_in_sandboxre-checksrun_cancel_requestedimmediately before spawning, so a run cancelled while it sat in pre-sandbox setup cannot spawn an orphan the cancel sweep already believes it terminated.sandbox_liveness_unconfirmedregistered as permanent/non-retryable (run_service), crash category (run_metrics), OPS alert (alerting), operator headline (public_view).Residual (documented, not implemented)
Action-boundary idempotency keys keyed on
run_idthreaded into side-effecting connector calls. Those calls run INSIDE the sandbox via the worker's own code, so this is a large cross-cutting change. The confirmed-dead gate removes the double-execution that would be needed to trigger a duplicate; the idempotency key is a defense-in-depth follow-up.Tests
New
test_e2b_double_execute_prevention.py+test_cancel_loop_kill_bounded.py:sandbox_liveness_unconfirmedcancel_sandboxforwards boundedrequest_timeoutFull e2b_driver + runner_sandbox suites green; the 12 pre-existing failures in the broad selection are identical on the base SHA (
1d9e15e) and unrelated (git-clone/MCP/posthog environment).Do NOT merge/deploy yet - parent gates merge + canary.
🤖 Generated with Claude Code