Repository navigation
fix(sdk): fail wait() after disconnect() with a SandboxError, not a TimeoutError - #1972
TanishGudise wants to merge 2 commits into
Conversation
…imeoutError disconnect() aborts the command/PTY output stream. In JS the abort reached wait() as a TimeoutError blaming requestTimeoutMs; async Python raised CancelledError into a task nobody cancelled, and sync Python a bare Exception. A stream error after disconnect() is now a clean end, as for WatchHandle.stop() (e2b-dev#1923), and wait() reports the disconnect with a SandboxError / SandboxException. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 08f820f The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ed1d3b841
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if not self._disconnected or _current_task_cancelling(): | ||
| raise |
There was a problem hiding this comment.
Distinguish prior cancellations from this wait
On Python 3.11+, a task's cancelling() count remains nonzero after it has caught a prior CancelledError unless it explicitly calls uncancel(). If that task later calls wait() after disconnect(), the CancelledError from the handle's internally canceled _wait takes this branch and escapes as though the caller canceled the current wait, instead of producing the promised SandboxException like JS and sync Python; compare against a count captured on entry to wait() (or otherwise distinguish a new cancellation) rather than treating any historical count as pending.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed: a task that had handled an earlier cancel kept cancelling() > 0, so wait() after disconnect() raised CancelledError instead of SandboxException. Fixed in 08f820f: wait() records cancelling() on entry and only treats an increase as the caller being cancelled (as asyncio.timeout() does), with a regression test for both disconnect paths.
A task that handled an earlier cancellation without uncancel() keeps a nonzero Task.cancelling(), so wait() after disconnect() raised CancelledError instead of SandboxException. Compare against the count captured on entry, as asyncio.timeout() does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Summary
CommandHandle.disconnect()aborts the command's output stream. In the JS SDK, await()that is pending at that point, or called afterwards, reports the abort as a request timeout:This is the
CommandHandle/ PTY twin of theWatchHandle.stop()bug fixed in #1923 (#1895), and the fix copies that pattern: the handle already records the disconnect, and a stream error that arrives after it is now a clean end.wait()then rejects with aSandboxErrorsaying the handle was disconnected before the command finished. Before, the error could also depend on timing: if an output event was already in flight whendisconnect()was called, thedisconnectedcheck inhandleEventsbroke out of the loop andwait()rejected withSandboxError('Process exited without a result')instead. Both cases now get the same message.The Python SDK had the same gap with different symptoms, so per the repo's parity rule it gets the matching change:
wait()raised a bareException("Command ended without an end event"), whichexcept SandboxExceptiondoes not catch.wait()raisedasyncio.CancelledErrorinto a task nobody cancelled.disconnect()cancels the handle's internal event task, andwait()re-raised that cancellation.except Exceptiondoes not catch it.Both now raise
SandboxExceptionwith the same message as JS.Scope:
onStdout/on_stdoutand friends) are still reported, as in fix(js-sdk): treat WatchHandle.stop() as a clean watch end #1923.stdout/stderr. Async Python no longer passes them to the callbacks after the disconnect (JS already didn't); sync Python is unchanged here.disconnect()that fails (for example from another thread during a read, which raisesValueError: generator already executing, as onmain) leaves the handle as it was.wait()on a command that finished before the disconnect still returns its result.Exceptionwhen a stream ends without an end event (fix(python-sdk): raise SandboxException instead of bare Exception when command stream ends without exit event #1727 addresses that separately).wait()still cancels it. On Python 3.11+ that also holds when the cancel anddisconnect()land in the same event-loop tick (checked withTask.cancelling(), counting only cancellations requested during thewait()call, asasyncio.timeout()does). Python 3.10 has noTask.cancelling(), so in that same-tick case the waiter gets theSandboxExceptioninstead, and CI (3.10) skips the test for it. If you'd rather close that gap on every version,wait()could wait withasyncio.wait([self._wait]), which never passes the caller's cancellation into the event task, so a cancelled caller always sees its ownCancelledErrorand thecancelling()checks go away. The cost is that cancellingwait()would no longer stop the handle's event task, which changes behavior, so I left that to you.Usage
Tests
packages/js-sdk/tests/sandbox/commands/commandHandle.test.ts: a pendingwait()(stdout and PTY handles) and a laterwait()reject with theSandboxError; a stream error on a handle that was not disconnected still maps toTimeoutError; a callback error raised afterdisconnect()is still reported; decoder bytes buffered at the disconnect end up instdoutbut not in the callback. The stream fake fails the pending read with theCanceledConnectErrorthat connect-es raises for an aborted request.packages/python-sdk/tests/test_command_handle.py, sync and async, mostly on the repo's HTTP/2 envd frame server (envd_frame_server) with the real connectrpc client: disconnect while idle, async disconnect while a callback is running, sync disconnect from inside a callback, two guards that cancelling the waiter still cancels it (one is 3.11+ only), a cancellation the calling task handled beforewait()is not taken as a new one (3.11+, both disconnect paths), a syncdisconnect()that fails leaves the handle connected, and async decoder bytes buffered at the disconnect end up instdoutbut not in the callback.main(3 JS, 7 Python) and pass with the fix; the guard tests pass on both. 20 repeated runs were clean.spec/runtime-refand run in Docker with-isnotfc: onmain, 20/20 command and 20/20 PTYwait()s afterdisconnect()fail withTimeoutErrorin JS,CancelledErrorin async Python and a bareExceptionin sync Python. With the fix, all of them raise the newSandboxError/SandboxException. The command and PTY test folders inE2B_DEBUGmode against that envd pass. JS (tests/sandbox/commands,tests/sandbox/pty): 45 passed / 2 skipped, againstmain's 39 plus the 6 new tests. Python (tests/{sync,async}/sandbox_*/{commands,pty}): 52 passed / 5 skipped on both.main. Without anE2B_API_KEY(I don't have one), the live tests fail, and so do three JS Secret/Volume tests that also fail onmain. Python tests also ran on 3.10 (CI's version) and 3.14.pnpm run lint,pnpm run format(no diff) andpnpm run typecheckpass.Includes an
e2b/@e2b/python-sdkpatch changeset.🤖 Generated with Claude Code