Repository navigation
feat(sandbox): implement E2B pause and resume - #3810
Conversation
hemildesai
added
feature
|
/ok to test f3df6f7 |
|
/claude review |
|
🌿 Preview your docs: https://nvidia-preview-pr-3810-a04b523e2e62.docs.buildwithfern.com/nemo/gym |
|
SHIP — clean, well-scoped addition of E2B pause/resume with strong test coverage. Reviewed What's solid:
One non-blocking note (posted inline at Docs and tests are consistent with the code. No async-safety, public-API, or config-convention concerns. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
yaoyu-33
added
complexity:low
|
Team review: SHIP WITH CARE. Thanks for this! The main item is @ananthsub's resume-lifetime thread on
Generated by Claude Code |
f3df6f7 to
0b1b2ab
Compare
|
/review model=claude |
|
/ok to test b71689e |
|
/ok to test 83ae6be |
|
/review model=claude |
|
/review model=claude |
svcnvidia-nemo-ci
left a comment
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
SHIP: LGTM, no concerns found. My last review (5746662132, at b2082a4) had no findings. None of the three changes since then raise anything eligible for this follow-up.
This is a follow-up review. I compared the head with the tree I reviewed last time (/context/previous-review/, b2082a4). Only three changes landed since then, all in 79e7b90. They match ananthsub's three suggestions from review 5759053088.
Changes since b2082a4
- Pause 409 message.
pause()now says the sandbox "refused pause with 409 Conflict (already paused or busy); check status()".- Only the text changed. The exception type (
RuntimeError), theis Falsebranch and the handle state are all the same as at b2082a4 L610-L615. - No caller I found reads the message. The
AsyncSandbox.pausefacade calls the provider directly, andSupportsSandboxPauseResumedoesn't specify an error message. - Both tests now use
match="refused pause with 409", at L506 and L1376. Both strings match the new text.
- Only the text changed. The exception type (
- Docs sentence. e2b.mdx#L204-L207 now also mentions E2B cloud's 409 for a sandbox in a transitional state.
- The "which status reports as paused" clause still refers to the AgentENV snapshot/fork case.
- The rewrap kept every other sentence, including the resume lifetime and
ttl_snote, the no-retry note, and thettl_sbackstop paragraph at L215-L217.
- Two new resume tests, at L1390-L1410. I traced both against the source; I didn't run them.
test_resume_prefers_the_configured_request_timeout:create()keepsrequest_timeout=90.0on the handle. Inresume(),setdefaultthen keeps the configured 12, so the call istimeout=3600, request_timeout=12. Ifsetdefaultbecame an assignment, the call would send 90.0 and the test would fail. The existing test at L1387 can't catch that, because its create and configured timeouts are both 12.test_resume_sends_whole_second_lifetime:create()accepts 0.2 and 90.4 because it only requires a finite value above 0 (L447-L449).resume()sends theintvalues 91 and 1. Withoutceil, the 90.4 case would send the float 90.4 and fail both assertions.- Both tests patch
FakeSandbox.connectthroughmonkeypatch, which undoes the patch after each test, so nothing leaks into other tests.
Other reviewers' threads that GitHub still marks current
- ananthsub's request for these two resume tests (on test_e2b_provider.py L1387) is done; the tests are described above.
- pthombre's
ttl_sbackstop docs request is still met at e2b.mdx L215-L217. - yaoyu-33's real-SDK test request is still met by
test_real_sdk_pause_resume_and_status_use_only_the_control_plane(L472-L521).
ananthsub's "Outside this PR" items (e2b 2.55 compatibility and the missing lifetime bound for paused sandboxes) cover code that already existed before b2082a4. They aren't in scope for this follow-up.
Coverage note
- I split this follow-up into three topics, each handled by a delegated child: the pause 409 message and its callers, the e2b.mdx changes, and the new and updated tests.
- All three children finished, but only the tests child's report reached me. I checked the other two topics myself:
- I compared the e2b.mdx Pause and Resume section line by line against b2082a4.
- I read
pause()in both trees, theAsyncSandboxpause/resume facades and the pause/resume protocol.
- I couldn't run tests, search the whole repo for other uses of the message text, or read the E2B SDK or AgentENV source.
Review generated by claude. This was a follow-up review. Request a full review with /review reset=true.
Review generated by claude. This was a findings-only review. Request a full review with /review reset=true.
ananthsub
left a comment
ananthsub
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround. All three suggestions are in, and the E2B tests pass on 79e7b90 with the locked e2b 2.46.0. The e2b 2.55 compatibility and paused-sandbox cleanup items from the summary comment can be tracked separately.
Signed-off-by: Hemil Desai <hemildesai@users.noreply.github.com>
Resume now passes create.timeout_s to connect, so a resumed sandbox keeps the provider lifetime instead of the SDK's 300 s default, and keeps the request timeout the SDK retained from create. Pause confirms the state when the SDK returns False, since the SDK maps every 409 to False. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Hemil Desai <hemildesai@users.noreply.github.com>
Drives the real e2b SDK through a mock transport: status maps the SDK's SandboxState enums, a 409 on an already-paused sandbox is a no-op, resume sends the create lifetime, and no request reaches envd. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Hemil Desai <hemildesai@users.noreply.github.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Hemil Desai <hemildesai@users.noreply.github.com>
AgentENV answers pause with 409 during a snapshot or fork, and its control-plane status reports those states as paused although the VM returns to running. The get_info fallback therefore reported a pause that did not happen. Raise on every 409 instead. Document that a paused sandbox no longer counts down its timeout, so ttl_s stops acting as a cleanup backstop. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Hemil Desai <hemildesai@users.noreply.github.com>
The pause error now says the request was refused (already paused or busy) and points at status(), instead of claiming the sandbox was not paused. The docs list E2B cloud's transitional-state 409s too. Add tests that a configured request timeout wins on resume and that the resume lifetime is sent as whole seconds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Hemil Desai <hemildesai@users.noreply.github.com>
79e7b90 to
a04b523
Compare
|
/ok to test 79e7b90 |
@hemildesai, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
/ok to test a04b523 |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/41756396552 |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/41757954180 |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/41759760746 |
…ilience Resolve the E2B docs page against #3810: keep the new-sandbox grace paragraph before the control-plane status note and the new Pause and Resume section. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Hemil Desai <hemildesai@users.noreply.github.com>
What does this PR do?
Implement E2B support for the public
AsyncSandbox.pause()/resume()and synchronousSandboxmethods introduced in #2636. Previously these methods raisedNotImplementedErrorfor E2B.Pause calls the SDK's public
pause()method. Resume connects to the same sandbox ID and replaces the SDK handle only after success. Failed lifecycle calls are not retried by the provider, and the previous handle remains available for status checks or cleanup. Status uses control-plane metadata to reportPAUSEDwithout waking the guest.This is independent of automatic pause/resume configuration in #3757 and changes no benchmark configs. Both PRs need the same control-plane status behavior. No separate issue is needed for this focused implementation of the existing public capability.
Validation
python -m pytest tests/unit_tests/test_e2b_provider.py tests/unit_tests/test_e2b_build.py tests/unit_tests/test_sandbox.py tests/unit_tests/test_sandbox_connect.py tests/unit_tests/test_sandbox_access.py --no-cov -q -o addopts='': 274 passed, 2 skipped with the locked E2B 2.46.0 on the branch rebased onto main; skips are existing opt-in live sandbox tests.pre-commit run --all-files: passed.cd fern && npm run check -- --warnings: zero errors; redirect checking skipped because Fern authentication is unavailable.git diff --check: passed.CI classification: full (shared provider changes). The complete core/server suites were not run locally.
Rollouts
Live public-API smoke passed against AgentENV with
osworld-slim, E2B 2.46.0, and certificate-verified HTTPS:PAUSED; repeated status checks do not resume it; repeated pause succeeds.Sandbox.pause()/resume()preserves a file and permits subsequent exec.A model-driven agent/verifier rollout has not been run; the live smoke directly exercises the new public provider capability.
Compatibility and benchmark impact
No public API signature, creation policy, or benchmark configuration changes. Resume restarts the sandbox lifetime at
create.timeout_s(the provider default); a per-sandboxSandboxSpec.ttl_sis not reapplied. Pause raises on a 409 Conflict, which E2B cloud sends for an already-paused sandbox and AgentENV sends during a snapshot or fork. Existing E2B descriptor connect behavior is unchanged. Status now reflects control-plane lifecycle state instead of guest-health reachability. Timeouts and transport errors can leave remote state unknown; check status before retrying. No dataset or scoring changes; benchmark-result impact is unmeasured.Checklist