Repository navigation
feat(sdk): idempotencyKey connection option for sandbox create - #1978
devin-ai-integration[bot] wants to merge 2 commits into
Conversation
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
🦋 Changeset detectedLatest commit: 03e08c2 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 |
Package ArtifactsBuilt from a1da2ca. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-dockerfile-utils-0.1.2-devin-1791891129-sandbox-create-idempotency.0.tgz ./e2b-2.56.1-devin-1791891129-sandbox-create-idempotency.0.tgzCLI ( npm install ./e2b-cli-2.22.1-devin-1791891129-sandbox-create-idempotency.0.tgzCode Interpreter JS SDK ( npm install ./e2b-code-interpreter-2.9.1-devin-1791891129-sandbox-create-idempotency.0.tgzDesktop JS SDK ( npm install ./e2b-desktop-2.5.1-devin-1791891129-sandbox-create-idempotency.0.tgzPython SDK ( pip install ./e2b_dockerfile_utils-0.1.1+devin.1791891129.sandbox.create.idempotency-py3-none-any.whl ./e2b-2.56.0+devin.1791891129.sandbox.create.idempotency-py3-none-any.whlCode Interpreter Python SDK ( pip install ./e2b_code_interpreter-2.11.0+devin.1791891129.sandbox.create.idempotency-py3-none-any.whlDesktop Python SDK ( pip install ./e2b_desktop-2.7.0+devin.1791891129.sandbox.create.idempotency-py3-none-any.whl |
41c28a7 to
0bd8f1d
Compare
devin-ai-integration
Bot
changed the title
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. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /** | ||
| * Idempotency key sent as the `Idempotency-Key` header when creating a | ||
| * sandbox. Retrying the create with the same key returns the original result | ||
| * instead of creating another sandbox; it also lets the SDK retry the create | ||
| * after a 502 or a dropped connection. Use a new key per sandbox. 1–255 | ||
| * lowercase letters or digits. | ||
| */ | ||
| idempotencyKey?: string |
There was a problem hiding this comment.
🟡 (optional) Callers who pass the new key to an E2B client get the first sandbox back from every later create, or a mismatch error, instead of a new sandbox. idempotencyKey sits on ConnectionOpts/ApiParams, and the client binds those as defaults for every call. The client already strips signal because it is per-request; the key is per-request too but is not stripped. Fix: keep the key off the bound client options in both SDKs. Omit it from E2BClientOpts (client.ts:14) and E2BClientParams (client.py:17), and drop it at bind time like signal, so it is only honoured per create call.
Why this was flagged
E2BClientOpts = Omit<ConnectionOpts, 'signal'> (packages/js-sdk/src/client.ts:14) and E2BClientParams(ApiParams) (packages/python-sdk/e2b/client.py:17) now both accept the key. client.ts:75-87 and client.py:65-70 bind it as a default for every call. ClientFactory.resolveOpts → mergeOpts (connectionConfig.ts:507-530) merges it into every Sandbox.create, and sandboxApi.ts:1844 sends it as Idempotency-Key. Python does the same through _resolve_api_params and get_api_client headers in sandbox_sync/sandbox_api.py and sandbox_async/sandbox_api.py. So new E2B({ idempotencyKey: 'k' }) followed by two client.Sandbox.create() calls sends the same key twice. The server then returns the first sandbox again, or a 409 idempotency_request_mismatch when the bodies differ. On the base branch the option does not exist, and each create makes a new sandbox. The doc comment says "Use a new key per sandbox", but nothing enforces that for bound options, unlike signal, which client.ts:73-76 deletes for exactly this reason.
Verification: The constructor copies opts into boundOpts and deletes only signal (client.ts:75-76). mergeOpts starts from { ...boundOpts } and only overrides with per-call values that are not undefined (connectionConfig.ts:515-526). So a bound key survives into every create. Nothing in either SDK rejects or strips the key at client level.
| export function isReplayable(request: Request): boolean { | ||
| if (request.headers.has(IDEMPOTENCY_KEY_HEADER)) return true |
There was a problem hiding this comment.
🟡 (optional) Callers who put an Idempotency-Key in their headers option can get duplicate resources. Every POST then counts as replayable, not only sandbox create. isReplayable returns true whenever the header is present, whatever the route. Routes the server does not deduplicate (snapshot, volume, secret, fork creation) are then replayed after a 502 or a dropped connection. Fix: treat a request as replayable on the key only for the route that is keyed (POST /v2/sandboxes), and keep the per-route list for everything else. Apply the same change to is_replayable in packages/python-sdk/e2b/retry.py:70.
Why this was flagged
The trigger is a caller passing Idempotency-Key through the generic headers/api_headers option, or a header set on a Sandbox instance or an E2B client. Those headers are merged into every control-plane request, for example by get_api_params at packages/python-sdk/e2b/connection_config.py:411-415 and by ConnectionConfig.headers in JS. isReplayable (packages/js-sdk/src/retry.ts:114) and is_replayable (packages/python-sdk/e2b/retry.py:70-71) return true for any request carrying the header. So POST /sandboxes/{id}/snapshots, POST /volumes, POST /secrets and fork are retried after a 502 or a post-write network error. On the base branch these POSTs were never replayed because they are off REPLAYABLE_OPERATIONS. Per the PR description, the server deduplicates only POST /v2/sandboxes behind api-idempotency-routes. A replay of the other POSTs can therefore create a duplicate snapshot, volume or secret, or hit a 409 key mismatch. Nothing in the diff checks that the header was set by the create path and not by the user.
Verification: The new first line of isReplayable, if (request.headers.has(IDEMPOTENCY_KEY_HEADER)) return true (retry.ts:114), runs before the method/route allow-list. So any POST carrying that header is treated as replayable. Those POSTs are then replayed after a 502 or a network error that occurs after the request was written, while the base did not replay them.
| if (response.status === 409) { | ||
| if (!hasIdempotencyKey) return undefined | ||
| try { | ||
| const body: unknown = await response.clone().json() | ||
| if ( | ||
| typeof body !== 'object' || | ||
| body === null || | ||
| !('error_code' in body) || | ||
| body.error_code !== 'idempotency_in_progress' | ||
| ) { | ||
| return undefined | ||
| } | ||
| } catch { | ||
| return undefined | ||
| } | ||
| } | ||
| if (response.status === 502 && !replayable) return undefined | ||
|
|
||
| const retryAfter = parseRetryAfter(response.headers.get('Retry-After')) |
There was a problem hiding this comment.
🟡 (optional) A keyed create replayed after a dropped connection usually fails with a 409 "idempotency_in_progress" error, while the original sandbox keeps running unseen. The in-progress 409 reuses the 502/503 backoff (BACKOFF_BASE_MS 100, DEFAULT_RETRIES 3), so with no Retry-After the SDK gives up after about 0.35–0.7 s in total. Fix: when no Retry-After is sent, poll an in-progress 409 until the request deadline with a longer floor delay, and do not count those polls against retries. Do this in retry.ts and in both Python transports (retry.py _retry_delay).
Why this was flagged
Trigger: a create made with idempotencyKey/idempotency_key loses its connection after the request is written, and belt answers the replay with 409 error_code: "idempotency_in_progress" and no Retry-After. retryDelayMs (packages/js-sdk/src/retry.ts:185-207) and _retry_delay (packages/python-sdk/e2b/retry.py:99-113) fall through to backoffMs/_backoff_delay: 100·2^attempt ms with 50% jitter, capped by retries (DEFAULT_RETRIES = 3). The network-error retry already used attempt 0, so the 409s get about 0.1–0.6 s of polling before attempt === retries returns the last 409 (retry.ts:267). So handleApiError/handle_api_exception raise a generic 409 SandboxError/SandboxException. The caller never learns the sandbox ID, and the sandbox created by the first request runs until its timeout. The 60 s request deadline (retry.ts:243) would allow far more polling, but the retry count stops it first. The base branch had no keyed retry.
Verification: nit. In packages/js-sdk/src/retry.ts, retryDelayMs lets a keyed 409 with error_code === 'idempotency_in_progress' through (lines ~183-200). With no Retry-After it falls through to return backoffMs(attempt, random). Total waiting is about 350-700 ms. Python is the same (retry.py:81-83, 113). Against the base, nothing gets worse.
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
25ac1f3 to
ffc5484
Compare
devin-ai-integration
Bot
changed the title
There was a problem hiding this comment.
Devin Review found 2 new potential issues.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| api_client = get_api_client( | ||
| config, | ||
| headers=( | ||
| {IDEMPOTENCY_KEY_HEADER: config.idempotency_key} | ||
| if config.idempotency_key is not None | ||
| else None | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🔴 Keyed creates fail after dropped connections
When a connection drops after a keyed create is sent, the create fails without retrying. isRetryableFetchError excludes post-write errors, and RetryableTransport propagates them. The server can create the sandbox while the caller receives an error.
Learn more
A keyed create passes its header to the sync and async API clients, but the retry layer never examines it. isRetryableFetchError only accepts connection-establishment failures; RetryableTransport and AsyncRetryableTransport do not catch post-write transport errors. Once the server receives a create, a dropped response can leave the client unaware of a successfully created sandbox.
Example: A client sends POST /v2/sandboxes with key job42; the server creates it and the response connection resets. The call raises rather than replaying job42 to retrieve the result.
Recommended fix: Restore header-and-route-aware post-write network retries in the JS fetch wrapper and both Python retry transports. Reuse the same request body and key, exclude aborts and unkeyed creates, and cover the error path in tests.
Was this helpful? React with 👍 or 👎 to provide feedback.
| api_client = get_api_client( | ||
| config, | ||
| headers=( | ||
| {IDEMPOTENCY_KEY_HEADER: config.idempotency_key} | ||
| if config.idempotency_key is not None | ||
| else None | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🟡 Concurrent keyed creates return transient conflicts
If a keyed create receives an unmarked idempotency_in_progress 409, it fails immediately. The Python retry gate and JS retry gate require Should-Retry. The second caller never receives the first create's result.
Learn more
A concurrent keyed create can receive HTTP 409 with error_code: idempotency_in_progress while the first create is still running. The key is sent to the server, but the Python retry gate and JS retry gate only consider Should-Retry: true. Without that response header, no attempt waits for completion, even when the response includes Retry-After.
Example: Two workers create with key job42. One request starts creation; the other receives an unmarked in-progress 409 and raises rather than waiting to obtain the same sandbox.
Recommended fix: For keyed POST /v2/sandboxes, recognize only the in-progress 409 error code in both SDKs and retry within the existing budget; leave mismatch and unknown-outcome errors terminal. Add sync, async, and JS coverage.
Was this helpful? React with 👍 or 👎 to provide feedback.
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
This branch has not been deployed
1 participant
Summary
Client side of e2b-dev/belt#3984 (idempotent
POST /v2/sandboxes). Stacked on #1998, which retries API responses only when they carryShould-Retry: true. belt sends that header on a 409idempotency_in_progress, so this PR adds no retry logic of its own.idempotencyKeyin JS (ConnectionOpts/ConnectionConfig) andidempotency_keyin Python (ApiParams/ConnectionConfig). The SDK doesn't generate keys. When the option is set,Sandbox.createsends it asIdempotency-KeyonPOST /v2/sandboxesand on no other request, and feat(sdk): retry control-plane requests only on Should-Retry: true #1998's retries reuse the key. When it's unset, nothing changes.ConnectionConfigchecks only the length (1–255 characters). A key outside that range throwsInvalidArgumentError/ raisesInvalidArgumentException. The server validates the[a-z0-9]charset.get_api_client(config, headers=...)instead ofwith_headers, becausewith_headerswould change the shared HTTP client.Should-Retry: true.minorchangeset fore2band@e2b/python-sdk.Link to Devin session: https://app.devin.ai/sessions/c2403155222c488eb4960d755b2ddcc5
Open in Devin Desktop: https://app.devin.ai/desktop/session/c2403155222c488eb4960d755b2ddcc5?variant=devin
Requested by: @mishushakov