autoresume: lifecycle component in sdk#1146
autoresume: lifecycle component in sdk#1146matthewlouisbrockman wants to merge 44 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 504de2c The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
Package ArtifactsBuilt from 92a36d1. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.13.1-lifecycle-in-sdk.0.tgzCLI ( npm install ./e2b-cli-2.7.4-lifecycle-in-sdk.0.tgzPython SDK ( pip install ./e2b-2.14.0+lifecycle.in.sdk-py3-none-any.whl |
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
adding the changeset should not have caused tests to fail, tests seem a bit flaky for some reason (not related to this PR i don't think) |
matthewlouisbrockman
requested review from
ValentaTomas,
jakubno and
mishushakov
as code owners
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1e0ba8559
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
mishushakov
left a comment
mishushakov
left a comment
There was a problem hiding this comment.
all looks good, just:
- let's do the param validation on the server side
- I think mocks are not necessary here
- instead of mock tests let's do full end to end test where we create a Sandbox with an auto-pause/auto-resume policy, waits until the timeout and then .connect to it again (or a http request to the sandbox url?)
|
got the issues - the end to end tests now pass on staging / fail on prod (expected; client proxy not live on prod yet so should fail) |
| """ | ||
|
|
||
|
|
||
| def get_auto_resume_policy(lifecycle: Optional[SandboxLifecycle]) -> Optional[str]: |
There was a problem hiding this comment.
I'd just do this inline, no need for another function
| assert False, "Sandbox not found" | ||
|
|
||
|
|
||
| def test_lifecycle_auto_resume_policy_mapping(): |
mishushakov
left a comment
mishushakov
left a comment
There was a problem hiding this comment.
approved but:
- unnecessary function get_auto_resume_policy which we don't have in JS.
- i am confused you'd call auto_resume policy "any" - it's too vague, most people won't be able to infer the meaning of the setting (I know it's only on backend, but still).
|
| secure: opts?.secure ?? true, | ||
| allow_internet_access: opts?.allowInternetAccess ?? true, | ||
| network: opts?.network, | ||
| ...(autoPause !== undefined ? { autoPause } : {}), |
There was a problem hiding this comment.
Conditional autoPause !== undefined is always true
Low Severity
The autoPause variable is computed as lifecycle.onTimeout === 'pause', which always produces a boolean — never undefined. The conditional autoPause !== undefined on the spread is therefore always true, making the ternary dead code. This was likely written by analogy with the autoResumePolicy check below (where undefined IS a real possibility), but it's misleading here since a reader might incorrectly assume there's a path where autoPause is intentionally excluded from the body.
Additional Locations (1)
|
|
plan's it goes to |
| autoResume?: { | ||
| policy: 'any' | 'off' | ||
| } | ||
| } = { |
There was a problem hiding this comment.
Redundant type intersection for already-defined schema field
Low Severity
The type intersection & { autoResume?: { policy: 'any' | 'off' } } on the body variable is redundant because components['schemas']['NewSandbox'] already includes autoResume?: components["schemas"]["SandboxAutoResumeConfig"] which resolves to exactly { policy: "any" | "off" }. This adds unnecessary type complexity and misleadingly implies the generated schema doesn't support autoResume, when it already does.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| return None | ||
|
|
||
| auto_resume = lifecycle.get("auto_resume") | ||
| return "any" if auto_resume else "off" |
There was a problem hiding this comment.
Reviewer-acknowledged function still present after agreed removal
Low Severity
The get_auto_resume_policy helper function was explicitly flagged for removal/inlining in the PR review, and the author acknowledged they would remove it, but it remains in the final diff along with its duplicated tests. The JS SDK inlines the equivalent logic directly in createSandbox, creating an unnecessary asymmetry between the two SDKs. The function is only a few lines and the logic is simple enough to inline at the two call sites in sandbox_async/sandbox_api.py and sandbox_sync/sandbox_api.py.
Additional Locations (2)
2 participants
Implements the
lifecycleprop onSandbox.create, taking over and deprecating thebeta_pausefunctionality.Currently supports:
on_timeout:kill(default) |pause. Controls what should happen to the sandbox when it hits end of life. Pause allows for resumingauto_resume: False (default) | True. Whether the sandbox should autoresume on trafficIntended for additional functionality as we update the backend to support additional props. Blocked from deploying until the API and client-proxy are deployed but for pre-approval.
(Meant to be extended later as add more capabilities to the API)
Note
Medium Risk
Changes sandbox creation request payloads in both JS and Python SDKs to support new lifecycle/auto-resume behavior, which could affect sandbox timeout/termination semantics. Backwards compatibility is partially preserved via deprecated
autoPause/betaPauseshims, but behavior depends on backend support.Overview
Adds a new
lifecycleoption toSandbox.create/AsyncSandbox.createthat controls what happens on timeout (killvspause) and whether paused sandboxes auto-resume on traffic, mapping this to the API’sautoPauseandautoResume.policyfields.Promotes pausing to a stable API (
Sandbox.pause/SandboxApi.pausein JS;Sandbox.pause/AsyncSandbox.pausein Python) and deprecatesbetaPause(and JSautoPause) with compatibility wrappers; JSSandbox.connect()instance method now takesSandboxConnectOpts.Updates and expands test coverage for pausing/resuming via
connectand for lifecycle payload behavior (including auto-resume wakeup), plus a minor Python connect-return fix and small test cleanup.Written by Cursor Bugbot for commit 504de2c. This will update automatically on new commits. Configure here.