-
Notifications
You must be signed in to change notification settings - Fork 1k
fix(sdk): let callers omit lifecycle.onTimeout #1703
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| --- | ||
| 'e2b': patch | ||
| '@e2b/python-sdk': patch | ||
| --- | ||
|
|
||
| Make `lifecycle.onTimeout` / `lifecycle["on_timeout"]` optional, so the "no timeout action configured" state the SDKs already put on the wire can actually be expressed by a typed caller, and treat a nullish `keepMemory` / `keep_memory` as unconfigured rather than as an explicit choice. | ||
|
|
||
| ```ts | ||
| import { Sandbox } from 'e2b' | ||
|
|
||
| // Opt out of auto-resume without expressing a preference about the timeout | ||
| // action — previously a type error, since onTimeout was required. | ||
| await Sandbox.create({ lifecycle: { autoResume: false } }) | ||
|
|
||
| // A keepMemory that spreads in as undefined is no longer sent as `true`, and no | ||
| // longer trips the pause-only guard on a kill action. | ||
| const keepMemory: boolean | undefined = undefined | ||
| await Sandbox.create({ lifecycle: { onTimeout: { action: 'pause', keepMemory } } }) | ||
| ``` | ||
|
|
||
| ```python | ||
| from e2b import Sandbox | ||
|
|
||
| # Opt out of auto-resume without expressing a preference about the timeout | ||
| # action — previously a type error, since on_timeout was required. | ||
| Sandbox.create(lifecycle={"auto_resume": False}) | ||
|
|
||
| # A keep_memory that is None is no longer sent as True, and no longer trips the | ||
| # pause-only guard on a kill action. | ||
| Sandbox.create(lifecycle={"on_timeout": {"action": "pause", "keep_memory": None}}) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This sample does not typecheck, and the changeset is the one artifact here that ships verbatim to users (into both The JS twin above it compiles clean, so this is the asymmetry described in the other comment rather than a typo. The Since the recommendation is to leave the type strict, the sample is what should change. This version typechecks, and I confirmed with a request-capture probe that it produces exactly the wire behavior the changeset describes — # A keep_memory the caller does not have an opinion about is left out of the
# request entirely, so the API's default applies.
Sandbox.create(
lifecycle={
"on_timeout": {
"action": "pause",
**({"keep_memory": keep_memory} if keep_memory is not None else {}),
}
}
)One more prose nit further down: line 33 writes |
||
| ``` | ||
|
|
||
| `autoResume: True` still requires an explicit `onTimeout` of `'pause'`; the error now names that knob instead of implying the SDK knows which action the API would have picked. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -456,14 +456,15 @@ export type SandboxLifecycle = { | |
| * Omitted from the create request when unset, leaving the API's default | ||
| * (currently `kill`) in effect. | ||
| */ | ||
| onTimeout: SandboxOnTimeout | ||
| onTimeout?: SandboxOnTimeout | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line — the PR's headline change — has no automated guard whatsoever in the JS SDK. I reverted it to It is not that the contract is unexpressed — your tests express it perfectly, that is precisely what dropping the
Adding The honest cost: flipping Not blocking for this PR — but as long as it holds, "the tests cover the optional |
||
|
|
||
| /** | ||
| * Auto-resume enabled flag. | ||
| * | ||
| * Leave unset to let the API pick the behavior. Set `false` to opt out | ||
| * explicitly and keep auto-resume off even if the API's default changes. | ||
| * Can be `true` only when `onTimeout` is `pause`. Not supported when | ||
| * Can be `true` only alongside an explicit `onTimeout` of `pause`, because | ||
| * auto-resume only has meaning for a sandbox that pauses. Not supported when | ||
| * `keepMemory` is `false` (a filesystem-only snapshot must be resumed | ||
| * explicitly via `connect()`). | ||
| */ | ||
|
|
@@ -1575,18 +1576,21 @@ export class SandboxApi { | |
| // `{ action, keepMemory }`. The discriminated union type forbids `keepMemory` | ||
| // on `action: 'kill'`; re-check at runtime for untyped callers. | ||
| const requestedOnTimeout = opts?.lifecycle?.onTimeout | ||
| // A missing (or explicitly nullish, for untyped callers) onTimeout is not a | ||
| // choice of `kill` — it leaves the timeout action to the API. Locally it | ||
| // still resolves to kill semantics for the validation below. | ||
| // A missing (or explicitly nullish) onTimeout is not a choice of `kill` — it | ||
| // leaves the timeout action to the API. The guards below therefore never | ||
| // assume what the API would pick; they only constrain what the caller said. | ||
| const onTimeoutConfigured = requestedOnTimeout != null | ||
| const onTimeout = requestedOnTimeout ?? 'kill' | ||
| const action = typeof onTimeout === 'string' ? onTimeout : onTimeout.action | ||
| const hasKeepMemory = | ||
| // A nullish keepMemory is not a choice of snapshot kind, the same way a | ||
| // nullish onTimeout is not a choice of action: the field is left out of the | ||
| // request and the API's default applies. | ||
| const requestedKeepMemory = | ||
| typeof onTimeout !== 'string' && 'keepMemory' in onTimeout | ||
| const keepMemory = | ||
| typeof onTimeout !== 'string' && 'keepMemory' in onTimeout | ||
| ? (onTimeout.keepMemory ?? true) | ||
| : true | ||
| ? onTimeout.keepMemory | ||
| : undefined | ||
| const hasKeepMemory = requestedKeepMemory != null | ||
| const keepMemory = requestedKeepMemory ?? true | ||
| // A missing autoResume (or an explicit null from an untyped caller) is left | ||
| // out of the request entirely, so the API keeps ownership of the default | ||
| // instead of receiving the SDK's local one as an explicit opt-out. | ||
|
|
@@ -1599,8 +1603,13 @@ export class SandboxApi { | |
| } | ||
|
|
||
| if (autoResume && action !== 'pause') { | ||
| // Without a configured action there is no `kill` to name — the SDK no | ||
| // longer decides what an unset onTimeout means — so point at the knob. | ||
| const hint = onTimeoutConfigured | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same half-covered conditional as the python side (see the comment on It is invisible in the summary because this file's branch total is byte-identical on base and head (1338/1827 overall, 44 → 42 arms in this block, 4 zero-count arms at both ends): the Making the hint unconditional keeps all 12 tests in test('an explicit kill action is not told to leave onTimeout unset', async () => {
await expect(
Sandbox.create('base', {
apiKey: TEST_API_KEY,
lifecycle: { onTimeout: 'kill', autoResume: true },
})
).rejects.toThrowError(
/^autoResume can only be true when onTimeout action is 'pause'\.$/
)
})The anchored regex is what does the work — it fails if the hint is appended when an action was given. |
||
| ? '' | ||
| : " Set lifecycle.onTimeout to 'pause': leaving it unset defers the action to the API." | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gating the hint is right — an unconditional hint that tells the user to do the thing they already did was a real defect on #1687 — but this is gated the less useful way round. The branch that gets the hint is the one where A single message covers both without a branch, and states the rule in the option names the caller typed: Non-blocking, and if you keep the branch, note the Python side spells the same knob |
||
| throw new InvalidArgumentError( | ||
| "autoResume can only be true when onTimeout action is 'pause'." | ||
| `autoResume can only be true when onTimeout action is 'pause'.${hint}` | ||
| ) | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -534,7 +534,7 @@ | |
| `"kill"`); an omitted `auto_resume` leaves the choice to the API. | ||
| """ | ||
|
|
||
| on_timeout: SandboxOnTimeout | ||
| on_timeout: NotRequired[SandboxOnTimeout] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The JS half of this change fully delivers what the PR sets out to do; the Python half delivers only one of the two states, and I think the right fix is in the docs rather than here.
I checked that widening works mechanically — changing this line and So the code here looks right as it stands. What needs to change is the advertising: the changeset and the PR description both present One small thing that follows from this: nothing typed can reach the |
||
| """ | ||
| What should happen to the sandbox when timeout is reached. `"kill"` terminates | ||
| the sandbox; `"pause"` pauses it for later resume. Accepts either the bare | ||
|
|
@@ -548,8 +548,9 @@ | |
| """ | ||
| Whether activity should cause the sandbox to resume when paused. Leave unset | ||
| to let the API pick the behavior. Set `False` to opt out explicitly and keep | ||
| auto-resume off even if the API's default changes. Can be `True` only when | ||
| `on_timeout` is `pause`. Not supported when `keep_memory` is `False` | ||
| auto-resume off even if the API's default changes. Can be `True` only | ||
| alongside an explicit `on_timeout` of `pause`, because auto-resume only has | ||
| meaning for a sandbox that pauses. Not supported when `keep_memory` is `False` | ||
| (a filesystem-only snapshot must be resumed explicitly via `connect()`). | ||
| """ | ||
|
|
||
|
|
@@ -811,20 +812,23 @@ | |
| """ | ||
| # on_timeout accepts a bare action or {"action", "keep_memory"}; normalize. | ||
| # Only the object form carries keep_memory; anything else (a bare action | ||
| # string, or an unexpected value from an untyped caller) passes through as | ||
| # the action, so a non-"pause" value resolves to kill instead of crashing. | ||
| on_timeout_raw = lifecycle.get("on_timeout") if lifecycle else None | ||
| # A missing on_timeout — or an explicit None from an untyped caller — is not | ||
| # a choice of kill. It only resolves to kill semantics locally, for the | ||
| # validation below and for keep_memory. | ||
| # A missing on_timeout — or an explicit None — is not a choice of kill. The | ||
| # guards below therefore never assume what the API would pick; they only | ||
| # constrain what the caller said. | ||
| on_timeout_configured = on_timeout_raw is not None | ||
| if isinstance(on_timeout_raw, dict): | ||
| on_timeout = on_timeout_raw.get("action", "kill") | ||
| keep_memory_provided = "keep_memory" in on_timeout_raw | ||
| # A None keep_memory is not a choice of snapshot kind, the same way a | ||
| # None on_timeout is not a choice of action: the field is left out of | ||
| # the request and the API's default applies. | ||
| keep_memory = on_timeout_raw.get("keep_memory") | ||
| keep_memory_provided = keep_memory is not None | ||
| else: | ||
| # Only fall back when unconfigured, not on other falsy-but-present | ||
| # values an untyped caller might pass. | ||
|
Check warning on line 831 in packages/python-sdk/e2b/sandbox/sandbox_api.py
|
||
| on_timeout = on_timeout_raw if on_timeout_configured else "kill" | ||
| keep_memory = None | ||
| keep_memory_provided = False | ||
|
|
@@ -845,9 +849,15 @@ | |
| auto_resume = lifecycle.get("auto_resume") if lifecycle else None | ||
|
|
||
| if auto_resume and on_timeout != "pause": | ||
| raise InvalidArgumentException( | ||
| "auto_resume can only be True when on_timeout action is 'pause'." | ||
| ) | ||
| message = "auto_resume can only be True when on_timeout action is 'pause'." | ||
| if not on_timeout_configured: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This conditional is half-tested, and it is the only uncovered thing the PR adds. On base, The reason is that no test in either repo pairs That matters because the failure mode is the exact confusion this PR set out to remove. The behaviour on head is correct: But nothing holds it there, so a later edit could tell a caller who did set Verified fix — passes on head, fails against the surviving mutant: @pytest.mark.parametrize(
"on_timeout",
[
pytest.param("kill", id="bare-kill"),
pytest.param({"action": "kill"}, id="object-kill"),
],
)
def test_explicit_kill_auto_resume_error_does_not_suggest_leaving_on_timeout_unset(
test_api_key, on_timeout
):
with pytest.raises(InvalidArgumentException) as excinfo:
Sandbox.create(
api_key=test_api_key,
lifecycle={"on_timeout": on_timeout, "auto_resume": True},
)
assert "leaving it unset" not in str(excinfo.value)This also closes the arc, taking the function back to fully covered. |
||
| # Without a configured action there is no "kill" to name — the SDK no | ||
| # longer decides what an unset on_timeout means — so point at the knob. | ||
| message += ( | ||
| " Set lifecycle['on_timeout'] to 'pause': leaving it unset defers" | ||
| " the action to the API." | ||
| ) | ||
| raise InvalidArgumentException(message) | ||
|
|
||
| if not keep_memory and auto_resume: | ||
| raise InvalidArgumentException( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When users copy this new example into a type-checked Python project, Pyright rejects it because
SandboxOnTimeoutPause.keep_memoryremainsNotRequired[bool], soNoneis not accepted even though the runtime now treats it as unset. Either widen the public type to acceptNone, matching the advertised JS/Python behavior, or avoid documentingNoneas a supported typed call.AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.