Conversation
🦋 Changeset detectedLatest commit: 5920e38 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 |
PR SummaryMedium Risk Overview Integration tests (JS + Python sync/async) assert cold boot by comparing kernel
Reviewed by Cursor Bugbot for commit 5920e38. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
TASTE.md compliance review (sdk-harness TASTE.md). Checked the changed surface against parity (T-1, T-1d, T-2), API shape (T-3, T-3a, T-5, T-6, T-10, T-14, T-23), generated types (T-18, T-20), timeouts/signal (T-46), and docs (T-47, T-69–T-71).
3 violations across 8 inline comments:
- T-3a — new Python
keep_memoryparameter is positional-or-keyword instead of keyword-only (6 signatures: sync + async, instance overload, static overload, implementation). - T-14 — boolean
keepMemoryselects a snapshot kind; an enum/literal union is preferred (flagged as a design decision, givenpauseprecedent). - T-70 / T-47 — JS default documented in prose instead of
@default; new 400/409 failure modes undocumented (T-69 / T-62).
Compliant: JS/Python names mirror (T-1a/T-10), option lives on the existing named CreateSnapshotOpts (T-23), static form still forwards full connection opts and signal (T-6, T-46), omitted values stay unset on the wire, generated SandboxSnapshotRequest doesn't leak into the public surface (T-18).
Not line-specific: the internal _cls_create_snapshot helpers could take keep_memory keyword-only too for consistency, though they're not public.
| def create_snapshot( | ||
| self, | ||
| name: Optional[str] = None, | ||
| keep_memory: Optional[bool] = None, |
There was a problem hiding this comment.
T-3a — new Python optionals must be keyword-only, enforced by a bare * in the signature. keep_memory is a brand-new parameter, so it should get the * from day one; as written, create_snapshot("name", False) binds positionally and any future reordering becomes a breaking change. Placing the * after the already-shipped name keeps existing callers working.
| keep_memory: Optional[bool] = None, | |
| *, | |
| keep_memory: Optional[bool] = None, |
There was a problem hiding this comment.
Done in d0f0024. keep_memory is keyword-only now in all six create_snapshot signatures (sync and async, instance overload, static overload and implementation), with the bare * placed after name so existing positional callers of name keep working. I also made it keyword-only on the internal _cls_create_snapshot helpers, which were already called with keywords.
| def create_snapshot( | ||
| sandbox_id: str, | ||
| name: Optional[str] = None, | ||
| keep_memory: Optional[bool] = None, |
There was a problem hiding this comment.
T-3a — same as above: keep_memory is new, so make it keyword-only with a bare *.
| keep_memory: Optional[bool] = None, | |
| *, | |
| keep_memory: Optional[bool] = None, |
There was a problem hiding this comment.
Done in d0f0024, same change as in the first thread.
| def create_snapshot( | ||
| self, | ||
| name: Optional[str] = None, | ||
| keep_memory: Optional[bool] = None, |
There was a problem hiding this comment.
T-3a — same as above: keep_memory is new, so make it keyword-only with a bare *.
| keep_memory: Optional[bool] = None, | |
| *, | |
| keep_memory: Optional[bool] = None, |
There was a problem hiding this comment.
Done in d0f0024, same change as in the first thread.
| async def create_snapshot( | ||
| self, | ||
| name: Optional[str] = None, | ||
| keep_memory: Optional[bool] = None, |
There was a problem hiding this comment.
T-3a — same as above: keep_memory is new, so make it keyword-only with a bare *.
| keep_memory: Optional[bool] = None, | |
| *, | |
| keep_memory: Optional[bool] = None, |
There was a problem hiding this comment.
Done in d0f0024, same change as in the first thread.
| async def create_snapshot( | ||
| sandbox_id: str, | ||
| name: Optional[str] = None, | ||
| keep_memory: Optional[bool] = None, |
There was a problem hiding this comment.
T-3a — same as above: keep_memory is new, so make it keyword-only with a bare *.
| keep_memory: Optional[bool] = None, | |
| *, | |
| keep_memory: Optional[bool] = None, |
There was a problem hiding this comment.
Done in d0f0024, same change as in the first thread.
| async def create_snapshot( | ||
| self, | ||
| name: Optional[str] = None, | ||
| keep_memory: Optional[bool] = None, |
There was a problem hiding this comment.
T-3a — same as above: keep_memory is new, so make it keyword-only with a bare *.
| keep_memory: Optional[bool] = None, | |
| *, | |
| keep_memory: Optional[bool] = None, |
There was a problem hiding this comment.
Done in d0f0024, same change as in the first thread.
| * of the source sandbox. The source sandbox keeps running either way. | ||
| * Defaults to `true` (full memory snapshot). | ||
| */ | ||
| keepMemory?: boolean |
There was a problem hiding this comment.
T-14 — prefer an enum (string-literal union) over a boolean for an option that selects a behavior. TASTE.md uses exactly this case as its counter-example: pause(sandboxId, { mode: 'memory' }), not { keepMemory: true }. A snapshot kind is the textbook case for a future third variant (e.g. memory-only / incremental), and a boolean has no room for it without a breaking change.
I recognize pause already ships keepMemory, so this mirrors existing surface (T-10); but createSnapshot is a fresh option and copying the pattern spreads it to a second method. Worth deciding deliberately before release (the PR is draft anyway) — e.g. kind?: 'memory' | 'filesystem' / kind="filesystem" on createSnapshot, with pause migrating at the next major via T-66/T-67. If the team decides parity with pause wins, this can be resolved as-is.
There was a problem hiding this comment.
I agree this is a shape decision rather than a bug, and it is one for the SDK maintainers to settle before this leaves draft. The current form mirrors the keepMemory option that pause already ships, so a caller sees one vocabulary for both operations. If the team prefers a kind literal on createSnapshot and a later migration of pause, I will switch this PR to it. Leaving the thread open for that call.
| * cold-boot (start fresh from disk) instead of restoring memory, so they | ||
| * begin without the running processes, in-memory state and open connections | ||
| * of the source sandbox. The source sandbox keeps running either way. | ||
| * Defaults to `true` (full memory snapshot). |
There was a problem hiding this comment.
T-70 / T-47 — defaults are documented with the JSDoc @default tag (as the other options in this file do, e.g. @default true / @default 100), not in prose.
Also T-69 / T-62: the option introduces new failure modes (400 snapshot_filesystem_only_disabled when the feature isn't enabled for the team, 409 snapshot_filesystem_only_unsupported_node) that the docs should say when they happen and what to do. Consider adding that to the createSnapshot JSDoc / Python docstrings (@throws).
| * Defaults to `true` (full memory snapshot). | |
| * | |
| * @default true |
There was a problem hiding this comment.
Done in d0f0024. The JS option now documents its default with @default true instead of prose. Both createSnapshot JSDoc blocks (static and instance) and all six Python docstrings now say what happens when the API refuses the request: a SandboxError (SandboxException in Python) with status 400 when the feature is not enabled for the team, error code snapshot_filesystem_only_disabled, and 409 when the sandbox's node runs an orchestrator that predates the option, error code snapshot_filesystem_only_unsupported_node. In both cases a full memory snapshot still works, and for 409 a pause and resume moves the sandbox to a node that supports it. I kept it to the generic error class because that is what the SDK raises for these statuses today; no new error type is introduced here.
Package ArtifactsBuilt from 840108b. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.52.1-feature-fs-only-snapshot.0.tgzCLI ( npm install ./e2b-cli-2.21.1-feature-fs-only-snapshot.0.tgzCode Interpreter JS SDK ( npm install ./e2b-code-interpreter-2.8.1-feature-fs-only-snapshot.0.tgzDesktop JS SDK ( npm install ./e2b-desktop-2.4.1-feature-fs-only-snapshot.0.tgzPython SDK ( pip install ./e2b-2.52.0+feature.fs.only.snapshot-py3-none-any.whlCode Interpreter Python SDK ( pip install ./e2b_code_interpreter-2.10.1+feature.fs.only.snapshot-py3-none-any.whlDesktop Python SDK ( pip install ./e2b_desktop-2.6.0+feature.fs.only.snapshot-py3-none-any.whl |
d0f0024 to
db975dd
Compare
`createSnapshot({ keepMemory: false })` (JS) and
`create_snapshot(keep_memory=False)` (Python) send `memory: false` on the
snapshot request, mirroring the pause option: only the filesystem is
persisted, so the snapshot is smaller and faster to take, and sandboxes
created from it cold-boot instead of restoring memory. The source sandbox
keeps running either way. Omitted, nothing is sent and the API default (a
full memory snapshot) applies.
The runtime spec pin (spec/runtime-ref) moves to the e2b-dev/runtime
commit that carries the API change, and both clients are regenerated
from it with the repo's codegen. The pin bump also brings the spec
changes landed upstream since the previous pin (18 September): the
team_id description, cached node admission fields, the
secret_limit_reached error code and 409 body, the envd metrics change
from mem_total_mib/mem_used_mib to oom_kills, and is_symlink on the
filesystem proto. None of them is read by SDK code outside the
generated files.
keep_memory is keyword-only in Python. Both SDKs document the two
refusals: status 400 (snapshot_filesystem_only_disabled) while the
feature is off for the team, and 409
(snapshot_filesystem_only_unsupported_node) when the sandbox's node
runs an orchestrator that predates the option.
The SDK tests prove the snapshot kind rather than only the file copy:
the source keeps its kernel boot id across the snapshot and the sandbox
created from it reports a different one, which a memory snapshot
cannot produce. The boot id is read with a command, not files.read,
because envd serves procfs files as an empty 200. Where the feature is
off for the team the API answers 400 and the tests skip with that
reason.
Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
db975dd to
5920e38
Compare
Summary
Adds
keepMemory(JS) andkeep_memory(Python) tocreateSnapshot/create_snapshot. Whenfalse, the SDK sendsmemory: falseon the snapshot request and the API takes a filesystem-only snapshot: only the filesystem is persisted, so the snapshot is smaller and faster to take, and sandboxes created from it cold-boot instead of restoring memory. The source sandbox keeps running either way. When omitted, nothing is sent and the API default, a full memory snapshot, applies.The option mirrors the
keepMemory/keep_memoryalready accepted on pause, and the wire field mirrors the pause request'smemory. Spec copy updated from the API, both generated clients regenerated, docstrings and a changeset (minor for both SDKs) included.Server side
The API support merged in e2b-dev/belt#4073 on top of the orchestrator support in e2b-dev/belt#4049. The API refuses
memory: falsewith 400snapshot_filesystem_only_disabledwhile the feature is not enabled for the team, and with 409snapshot_filesystem_only_unsupported_nodewhen the sandbox's node runs an orchestrator that predates the field. A request is never downgraded to a memory snapshot by a server that understands the field.Why this is a draft
An API deployment from before e2b-dev/belt#4073 accepts the request body, ignores
memory, and takes a memory snapshot while answering 201. Releasing this before production runs the new API would let callers ask for a filesystem-only snapshot and silently get a full one. Keep this in draft until production has the API build, then release.The SDK tests for the new option call the live API with
keepMemory: false. Where the feature is off for the test team the API answers 400 and the tests skip with that reason; where it is on, they prove the snapshot kind: the source keeps its kernel boot id across the snapshot and the sandbox created from it reports a different one, which a memory snapshot cannot produce. Against an API that predates the field those assertions fail, by design.Verification
main; Python modules compile; the JS SDK typechecks.spec/runtime-refis bumped to the e2b-dev/runtime commit that carries the API change and both clients are regenerated withmake codegen, so the Generated files check passes. The bump also pulls in the upstream spec changes since the previous pin (team id description, cached node admission fields, thesecret_limit_reachederror code, envdoom_killsreplacing the memory counters,is_symlinkon the filesystem proto); none of them is read by SDK code outside the generated files.keep_memoryis keyword-only in Python. Both SDKs document the 400 and 409 refusals oncreateSnapshot/create_snapshot.files.readbecause envd serves procfs files as an empty 200, and skip on the 400snapshot_filesystem_only_disabledrefusal.🤖 Generated with Claude Code