Skip to content

fix(e2b): stop a non-object connect error payload from becoming an opaque, retried e2b_sandbox_error - #2336

Open
federicodeponte wants to merge 3 commits into
mainfrom
fix/e2b-sandbox-error-residue
Open

fix(e2b): stop a non-object connect error payload from becoming an opaque, retried e2b_sandbox_error#2336
federicodeponte wants to merge 3 commits into
mainfrom
fix/e2b-sandbox-error-residue

Conversation

@federicodeponte

Copy link
Copy Markdown
Member

Summary

An external user was failing 48% of runs, dominated by e2b_sandbox_error. It is not an E2B platform problem. It is a defect in the e2b SDK's vendored error decoder that our catch-all classifier then hid, and our retry scheduler then multiplied by three.

Root cause

The e2b SDK vendors e2b_connect, whose error decoder assumes the decoded payload is a JSON object:

def make_error(error):
    code_value = error.get("code")            # AttributeError when not a Mapping
    ...
    return ConnectException(status, error.get("message", ""))

error_for_response only catches JSONDecodeError/KeyError, and the server-stream trailer path calls make_error(data["error"]) with no guard at all. A worker command runs as a connect server stream, so that is the path these runs took.

An envd/edge error whose body is valid JSON but not an object (a bare JSON string, null, a number, an array) therefore raises AttributeError: 'str' object has no attribute 'get' from inside the SDK. Reproduced deterministically against the real vendored SDK for all four shapes.

The damage is that the AttributeError replaces the real upstream error:

  1. The operator and the user never learn what actually failed.
  2. Its text matches none of _TRANSIENT_E2B_TRANSPORT_MARKERS, so _is_transient_e2b_transport_error returns False and the driver's bounded transport retry never engages.
  3. It falls through to a generic retryable e2b_sandbox_error, so the run scheduler re-dispatches twice more into the same masked failure.

That third step is why the user's numbers look catastrophic. This is exactly why the earlier transport-retry work (1317 to 21 weekly failures) left this residue untouched: it could not see it.

Evidence (prod)

  • 36 runs across 3 users carry this exact AttributeError, 2026-06-27 to 2026-07-20.
  • For the worst-hit user: 26 of 38 failures in 30 days, in exact 1-manual + 2-retry triplets. 9 real incidents produced 26 failed rows.
  • Retries of this class: 0 of 27 succeeded. Retries of the general e2b_sandbox_error population: 645 of 1580 succeeded (41%). So the generic retry is valuable and is kept; only the programming-error class becomes non-retryable.
  • No e2b_quota_exhausted and no rate-limit/concurrency errors in 60 days. This is not an E2B plan or quota problem.

Changes

1. runner_sandbox/e2b_connect_hardening.py (new). Wraps e2b_connect.client.make_error so a non-Mapping payload is normalized into the {code, message} shape the SDK expects, preserving the upstream text. make_error is the single chokepoint, verified against the vendored 2.34.0 tree: nothing imports make_error or error_for_response by name, so both call sites resolve the module attribute. Memoized under a lock, structurally guarded, and a no-op if the module is absent or its signature changed. Installed from both sandbox entry points.

Wrapping rather than pinning e2b>=2.35 is deliberate: 2.35 drops the vendored e2b_connect for third-party connectrpc and swaps the HTTP transport, which would invalidate retry behaviour tuned against 2.34.

2. New terminal code sandbox_driver_internal_error, non-retryable. A Python defect reaching the sandbox catch-all is not an E2B failure and is not fixed by re-running. Filing our own bugs under a generic retryable code is what let this sit undiagnosed behind a catch-all. Registered across the retry, metrics, alerting and public-view taxonomies so it is never auto-retried, never lands in "unknown", always pages the operator, and shows the user a headline that does not blame their worker.

3. Classify by where an exception was raised, not just its message. 33 prod runs have an error detail that is a bare integer ("2251", "203", "19"). str(KeyError(2251)) is "2251"; httpcore's HTTP/2 handler indexes its per-stream event map by stream id, so a torn-down stream raises exactly that. Those are transport races that are correctly retried today, and change 2 would have stolen them. _raised_in_transport_stack inspects the innermost traceback frame so the same KeyError is transport out of httpcore and a defect out of our own code.

Review

Adversarial review by Gemini 3.1 Pro (verdict BLOCK) and DeepSeek v4 Pro (APPROVE-WITH-CHANGES). Two real defects found and fixed in the third commit:

  • Install race (critical). The once-guard set _install_attempted before computing the result, letting a second thread proceed against the unpatched SDK. Verified against the old implementation: 7 of 8 threads went unpatched. Now lock-guarded, with a regression test that fails on the racy version.
  • Traceback-scan false positive (high). Scanning every frame meant an exception merely passing through httpx/httpcore counted as transport. Our own on_stdout/on_stderr callbacks are invoked from inside the SDK stream loop, so a defect in one of them would have been retried forever. Now innermost-frame only, with a test building that exact nested frame shape.

Rejected with rationale (and pinned by a test): widening the internal-error type list to ValueError/RuntimeError/AssertionError. Given the 41% recovery rate above, wrongly removing a retry is the expensive direction. The list widens from observed failures, not speculation.

Tests

tests/test_e2b_connect_non_object_error_payload.py, 58 tests, including an end-to-end case against the real vendored SDK and the two review regressions.

Full CI lane, exactly as CI runs it, from the engine root:

python -m pytest tests/ -p no:warnings -p no:cacheprovider -m "not flaky_ci" -n auto --dist loadscope
995 passed, 1 skipped

ruff check . clean.

Follow-ups (not in this PR)

  • e2b_sandbox_error is one of three catch-alls. worker_reported_error (1687 failures/30d) and run_execution_exception (703) are the others, plus 2201 failed runs with a NULL error_code.
  • Cloud repo, separate: apps/api/db/supabase_repos.py defaults runner to "local" where the engine's sqlite repo defaults to "e2b". Retry rows never pass a runner, so all 202 retry/restart_retry runs in the last 14 days are labelled as running on a runner that no longer exists.

federicodeponte and others added 3 commits July 30, 2026 17:26
…aque, retried e2b_sandbox_error

The e2b SDK vendors e2b_connect, whose error decoder assumes the decoded error
payload is a JSON object:

    def make_error(error):
        code_value = error.get("code")            # AttributeError when not a Mapping
        ...
        return ConnectException(status, error.get("message", ""))

error_for_response only catches JSONDecodeError/KeyError, and the server-stream
trailer path calls make_error(data["error"]) with no guard at all. A worker
command runs as a connect server stream, so an envd/edge error whose body is
valid JSON but not an object (a bare JSON string, null, a number, an array)
raises AttributeError: 'str' object has no attribute 'get' from inside the SDK.

That AttributeError replaced the real upstream error, with two consequences:

  * the failure was undiagnosable from the run record, and
  * its text matches none of _TRANSIENT_E2B_TRANSPORT_MARKERS, so
    _is_transient_e2b_transport_error returned False, the driver's bounded
    transport retry never engaged, and the run fell through to the generic
    RETRYABLE e2b_sandbox_error. The run scheduler then re-dispatched twice more
    into the same masked failure, so one real problem produced three failed runs.

Prod: 36 runs across 3 users, 2026-06-27 to 2026-07-20, all in that shape. For
the worst-hit user it is 26 of 38 failures in 30 days, in 1-manual-plus-2-retry
triplets, which is why the transport-retry work (1317 -> 21 weekly failures) left
this residue untouched: it could not see it.

Two changes:

1. runner_sandbox/e2b_connect_hardening.py wraps e2b_connect.client.make_error
   so a non-Mapping payload is normalized into the {code, message} shape the SDK
   expects, preserving the upstream text. make_error is the single chokepoint for
   all three call sites. Idempotent, guarded, and a no-op if the module is absent
   or its shape changed, so an SDK bump cannot break sandbox execution. Installed
   from both sandbox entry points (e2b_driver and agent_driver).

   Wrapping rather than pinning e2b>=2.35 is deliberate: 2.35 drops the vendored
   e2b_connect for third-party connectrpc and swaps the HTTP transport, which
   would invalidate the retry behaviour tuned against 2.34.

2. A Python defect reaching _sandbox_exception_result now gets its own terminal
   code, sandbox_driver_internal_error, and is NOT retryable. Filing our own bugs
   under a generic retryable e2b_sandbox_error is what let this sit undiagnosed
   behind a catch-all, and the needless retries tripled the failed-run count a
   user sees. Registered across the retry, metrics, alerting and public-view
   taxonomies so it is never auto-retried, never lands in "unknown", always pages
   the operator, and shows the user a plain-language headline that does not blame
   their worker. Recognized transport signatures and the timeout classifier still
   win, so nothing the existing transport work handles is stolen.

Regression test: tests/test_e2b_connect_non_object_error_payload.py, including an
end-to-end case against the real vendored SDK.

Co-Authored-By: Claude <noreply@anthropic.com>
…sage

Follow-up to the driver-internal split, driven by prod evidence: 33 runs in the
last 60 days failed with an error detail that is nothing but a bare integer
("2251", "203", "191", "19", ...). str(KeyError(2251)) is "2251". httpcore's
HTTP/2 handler indexes its per-stream event map by stream id
(self._events[stream_id]), so a stream torn down mid-read raises exactly that.
Those are genuine transport races inside a dependency and they are already
handled correctly today: retryable e2b_sandbox_error.

The new driver-internal split would have stolen them. KeyError is a
programming-error type, and a bare integer matches none of the transient
message markers, so all 33 would have flipped to non-retryable
sandbox_driver_internal_error: a regression that removed a retry that was
working.

Message matching cannot separate these, so classify by origin as well:
_raised_in_transport_stack walks the traceback and treats any exception raised
inside h2/hpack/httpcore/httpx as a transport failure whatever its Python type.
This also makes the existing string markers ("deque mutated during iteration",
h2 state-machine errors) robust to upstream rewording.

Net effect on the split: the same KeyError is transport when it comes out of
httpcore and a driver defect when it comes out of our own code, which is the
distinction that actually matters.

Co-Authored-By: Claude <noreply@anthropic.com>
…ound in review

Adversarial review (Gemini 3.1 Pro, DeepSeek v4 Pro) found two real defects in
the first two commits. Both are fixed with regression tests.

1. CRITICAL, install race. The once-guard set _install_attempted = True BEFORE
   computing the result, so a second thread could see "already attempted",
   return bool(None), and spawn a sandbox against the UNPATCHED SDK. Runs execute
   on concurrent executor threads, so under load this reintroduced exactly the
   bug this module prevents. Verified against the old implementation: 7 of 8
   threads proceeded unpatched. Now memoized under a threading.Lock.

2. HIGH, traceback-scan false positive. _raised_in_transport_stack scanned every
   frame, so any exception whose stack merely PASSED THROUGH httpx/httpcore
   counted as transport. That is not hypothetical: our own on_stdout/on_stderr
   callbacks are invoked by the SDK from inside the stream loop, so a defect in
   one of them has transport frames above it and would have been labelled a
   transient transport failure and retried forever. Now only the innermost frame
   counts, which is where the exception was actually raised. Added a test that
   builds exactly that nested frame shape.

   For the case where a traceback is absent (an exception re-raised without one),
   origin is unknowable, so fall back to the narrow observed signature: a
   KeyError whose entire detail is digits is the httpcore stream-id shape.

Also from review:
- Structural guard instead of an e2b version pin: refuse to wrap make_error if
  it no longer accepts a single positional argument. A version string would
  refuse a patch release that still carries the defect and would still miss a
  signature change within one version.
- Documented that make_error is the only function needing a wrapper, verified
  against the vendored 2.34.0 tree: nothing imports make_error or
  error_for_response by name, so both call sites see the module attribute.
- Simplified the bytes/bytearray decode.

Deliberately NOT taken: widening _DRIVER_INTERNAL_EXCEPTION_TYPES to ValueError,
RuntimeError and AssertionError. Prod says the general e2b_sandbox_error retry
recovers 645 of 1580 attempts (41%), so removing a retry is expensive, while
retries of the programming-error class recover 0 of 27. The list stays narrow and
evidence-driven; it should widen from observed failures, not speculation. A test
pins that decision so it is a choice rather than an oversight.

Co-Authored-By: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant