fix(js-sdk): release inflight concurrency slot on body end, not headers - #1717
fix(js-sdk): release inflight concurrency slot on body end, not headers#1717devin-ai-integration[bot] wants to merge 4 commits into
Conversation
Fixes #1666 Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
🦋 Changeset detectedLatest commit: bac0b8e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 5f158d0. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.42.1-devin-1787165477-inflight-body-end.0.tgzCLI ( npm install ./e2b-cli-2.16.4-devin-1787165477-inflight-body-end.0.tgzPython SDK ( pip install ./e2b-2.42.0+devin.1787165477.inflight.body.end-py3-none-any.whl |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be39cf12e3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| controller.close() | ||
| return | ||
| } | ||
| controller.enqueue(result.value) |
There was a problem hiding this comment.
Observe source errors that occur between pulls
When the source yields a chunk and then errors before the downstream consumer requests another chunk, enqueue fills the passthrough queue so no reader.read() remains pending; consequently neither this catch nor cancel invokes done(). This occurs, for example, when a streaming response is paused after a chunk and its request signal is then aborted, and it leaves the semaphore slot occupied even though the HTTP/2 stream has terminated, eventually blocking queued requests. Observe rejection of reader.closed (while retaining the current EOF behavior) so asynchronous source errors also release the slot.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 22860fa. releaseOnBodyEnd now also observes reader.closed (void reader.closed.then(done, done)), so a source that terminates between pulls — e.g. an abort erroring the stream while a chunk sits in the queue with no read pending — still releases the slot. EOF behavior is unchanged (done is idempotent via releaseOnce). Added a regression test: limitConcurrency releases when the body errors between pulls.
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
|
@devin-ai-integration check comments |
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
There was a problem hiding this comment.
Blocking: this reintroduces an envd RPC deadlock, and open PR #1695 already fixes it
The premise checks out — I confirmed the base really does release on headers, and that holding to body end is the right shape for #1666. But the naive version of this change wedges the envd RPC stack, and #1695 (still open, same issue, same objective) already carries fixes for every point below. These two should be consolidated rather than landed side by side.
1. Open streams starve the unary calls that end them (blocking)
createEnvdRpcFetch hands one process-wide semaphore (module-level cache, shared across every sandbox) to the whole Connect transport. Long-lived server-streaming calls — commands.run({ background: true }) and commands.connect (rpc.start/rpc.connect), pty.start/pty.connect, files.watchDir — now hold a slot for the stream's entire lifetime. The unary calls that end those streams (commands.kill, sendStdin, sendSignal, closeStdin, list, pty.sendInput/sendSignal) go through the same this.rpc.*, so they queue behind the streams they exist to tear down.
Live against a real sandbox with E2B_ENVD_RPC_INFLIGHT_REQUESTS=1 and one sleep 60 background command open:
call (requestTimeoutMs: 5000) |
this PR | base d55ddb8 |
|---|---|---|
commands.list |
fails at 5002 ms | OK in 137 ms |
commands.sendStdin |
fails at 5002 ms | reached the server in 88 ms |
commands.kill |
fails at 5000 ms | OK in 131 ms |
(The base sendStdin "failure" is the legitimate SandboxError 13: stdin not enabled or closed — it still proves the request left the process.) Same repro, same numbers as on #1695. The only escape is the caller's own abort signal; nothing in the SDK breaks the cycle.
At the default cap of 2000 you need many concurrent streams to hit this, but the semaphore is process-wide across all sandboxes, anyone who lowers the documented knob hits it immediately, and item 4 below makes the pool shrink monotonically. #1695 solves it by classifying Connect streaming requests off the application/connect+… Content-Type and reserving slots that streams cannot occupy, with an overflow slot so a cap of 1 still lets teardown through.
2. The failure surfaces as advice to raise requestTimeoutMs, which makes it worse
Every starved call above reported:
TimeoutError [canceled] The operation was aborted due to timeout: This error is
likely due to exceeding 'requestTimeoutMs'. You can pass the request timeout
value as an option when making the request.
The request never left the process, so raising requestTimeoutMs only lengthens the hang. TASTE: "Timeout errors must tell the user which knob to turn." Before this change queue waits were transient and the message was merely imprecise; now a queue wait can last as long as a stream, so the message needs to name the concurrency env var. That requires threading the knob name into limitConcurrency, which is what #1695's LimitConcurrencyOptions.envVarName does.
3. content-length: 0 and HEAD work only by accident
See the inline comments — the if (!body) guard misses both, and they survive today only because the default queuing strategy's highWaterMark of 1 triggers one eager pull.
4. An abandoned stream permanently lowers the cap, and nothing says so
There is no FinalizationRegistry here, so a body that is neither drained nor cancelled never returns its slot. That is reachable from the public surface: files.read(path, { format: 'stream' }) and volume.read(..., { format: 'stream' }) hand the stream to the user. wrapStreamWithConnectionCleanup already documents the sibling case ("a consumer that holds the stream but stops reading ... is reclaimed server-side, not by this timer") — the inflight slot is precisely the part that is not reclaimed server-side. Verified offline: one unread non-empty body permanently starves a cap of 1.
TASTE treats docstrings as API and asks every public method to document its failure modes, so this needs saying in two places:
- the three cap docstrings still read "max number of API requests that can be in flight at once" (
getApiInflightLimit,getEnvdInflightLimit,getEnvdRpcInflightLimit). After this change they bound how many response bodies can be open at once — a materially different quantity that users tune against. - the changeset should tell users that streams they obtain must be drained or cancelled. As written it reads as a pure internal accounting fix.
One small correction to the changeset text: aligning the release point makes the cap accurate, but with the default cap far above the dispatcher's connections, it does not by itself prevent ERR_HTTP2_TOO_MANY_CONCURRENT_STREAMS.
What checked out
packages/js-sdklint,typecheck, andprettier --checkare clean; the 8 new/updatedtests/api/inflight.test.tscases pass.- Full unit project: 1 failed | 461 passed | 30 skipped. The failure is the pre-existing
tests/sandbox/network.test.ts > firewall transform injects headers(404: template 'httpbin' not found), unrelated to this diff. - The
getReader()-instead-of-piping choice is correct and well argued — a foreign stream really would fail the platform brand check, and it matches the existingwrapStreamWithConnectionCleanuppattern rather than inventing a new one. - Carrying
url/redirected/typeacross the wrapper is more careful than needed (nothing insrc/,openapi-fetch, or@connectrpc/connect-webreads them) and is the right instinct anyway; the accompanying test is real. - The
catcharoundnew Response(...)with a fallback to the original response is a good instinct, and releasing the slot on that path is right. - Confirming the premise: on the base, headers really do free the slot, so #1666 is a real defect and not a stale TODO.
Nothing in this diff reaches the exported surface (limitConcurrency is not re-exported from src/index.ts), so the parity, options-object, and naming rules don't apply. The TASTE rules that do bite are the timeout-message rule (item 2) and the docstring rules (item 4).
Sent by Cursor Automation: /check SDK complies with TASTE.md
| */ | ||
| function releaseOnBodyEnd(response: Response, release: () => void): Response { | ||
| const body = response.body | ||
| if (!body) { |
There was a problem hiding this comment.
This misses two of the three shapes whose bodies the HTTP client never touches.
openapi-fetch@0.14.1 returns without reading the body on exactly status === 204 || method === 'HEAD' || headers.get('Content-Length') === '0' (dist/index.mjs:173). Only 204 (and 205/304) give response.body === null; I verified on node 22 that a 200 or 500 with Content-Length: 0 has a non-null body, both synthetic and over the wire. And this is a live path, not a hypothetical — sandbox/filesystem/index.ts:556 exists for it: "When the file is empty, the response body is skipped and res.data is undefined." So files.read() on an empty file, and any non-2xx with Content-Length: 0 (the case that produced the isRunning() bug TASTE calls out), take the tracked path.
Those cases do release today, but only by accident. The default queuing strategy has highWaterMark: 1, so the ReadableStream constructor issues exactly one eager pull with no consumer attached; on an empty source that pull sees done: true and releases. Measured:
| source | consumer | pull calls |
|---|---|---|
| empty | none | 1 → releases |
| non-empty | none | 1, then the queue is full → never releases |
empty, highWaterMark: 0 |
none | 0 → never releases |
Nothing in the suite pins that, so switching to a byte strategy or highWaterMark: 0 later would silently deadlock every empty-body response. Worth checking explicitly, the way #1695's bodyToTrack() does:
if (!body || response.bodyUsed || body.locked) return releaseAndReturn()
if (method.toUpperCase() === 'HEAD') return releaseAndReturn()
if (response.headers.get('content-length') === '0') return releaseAndReturn()The HEAD case needs the request method, which means reading it off init?.method ?? input.method up in limitConcurrency.
There was a problem hiding this comment.
Fixed in bac0b8e. hasBodyToTrack(response, method) now short-circuits explicitly on the same conditions openapi-fetch does — !body || bodyUsed || body.locked, HEAD (method read off init?.method ?? input.method), and content-length: 0 — releasing the slot immediately instead of relying on the constructor's one eager pull. New tests pin content-length: 0 (asserting bodyUsed === false and the body unlocked afterwards), HEAD with a non-zero advertised Content-Length, and locked/used bodies.
| // `getReader` rather than piping the stream itself into the new Response: | ||
| // a foreign stream (cross-realm, ponyfill) would fail the platform's brand | ||
| // check, while a native wrapper reading through the reader always passes. | ||
| const reader = body.getReader() |
There was a problem hiding this comment.
getReader() throws TypeError if the body is already locked or consumed, and this call sits outside any try, after the slot has been taken. So the throw escapes limitConcurrency to the caller and the slot is never released.
Verified with a fetcher whose response body already has a reader: the first call throws, and the next call on a cap of 1 starves indefinitely (still queued at 500 ms). Nothing in src/ locks a body before the fetch returns, but createRuntimeFetch late-binds globalThis.fetch specifically so that mocks and instrumentation can replace it, and those are exactly what would hand back a spoken-for body. Folding bodyUsed/locked into the guard above covers it — if we could not read the body to its end anyway, there is nothing to wait for.
There was a problem hiding this comment.
Fixed in bac0b8e. limitConcurrency now checks response.bodyUsed || response.body.locked (together with the other no-byte shapes) before any getReader() call, and releases the slot immediately in that case — a mock/interceptor that already spoke for the body can no longer throw past the wrapper or leak the slot. Regression test: limitConcurrency releases immediately when the body is already locked or used.
| @@ -73,10 +70,95 @@ export function limitConcurrency( | |||
| const signal = | |||
| init?.signal ?? (isRequestLike(input) ? input.signal : undefined) | |||
| const release = await sem.acquire(signal) | |||
There was a problem hiding this comment.
Worth threading the env var name that configured max into limitConcurrency here, so a request aborted while still queued can say so.
As it stands the abort surfaces through abortReason(signal), which echoes the caller's request-timeout abort: TimeoutError ... likely due to exceeding 'requestTimeoutMs'. That was tolerable when slots turned over at header speed; now a queue wait can last as long as a stream, and the advice is actively wrong — the request never reached the network, so a larger requestTimeoutMs just lengthens the hang. TASTE asks timeout errors to name the knob to turn, and the knob here is E2B_ENVD_RPC_INFLIGHT_REQUESTS / E2B_ENVD_INFLIGHT_REQUESTS / E2B_API_INFLIGHT_REQUESTS, which this layer currently has no way to name.
There was a problem hiding this comment.
Done in bac0b8e. limitConcurrency takes options.envVarName (wired as E2B_API_INFLIGHT_REQUESTS / E2B_ENVD_INFLIGHT_REQUESTS / E2B_ENVD_RPC_INFLIGHT_REQUESTS from the respective fetch factories), and a request aborted while still queued now rejects with a TimeoutError naming that env var and its current value — telling the caller to raise the cap or close open streams instead of pointing at requestTimeoutMs. Test: limitConcurrency names the env var when a queued request aborts.
| start(controller) { | ||
| controllerRef = controller | ||
| // One chunk fills the passthrough queue so no read is pending | ||
| // when the source errors later. |
There was a problem hiding this comment.
new Response(null, { status: 204 }) is the one bodiless shape that already works structurally (body === null). The interesting cases are the ones where the body exists but the HTTP client never reads it — a 200 or 500 with content-length: 0, and a HEAD response — since those are what openapi-fetch short-circuits on. Both currently pass only because of the constructor's one eager pull, so a case like
new Response('', { status: 200, headers: { 'content-length': '0' } })driven through createClient({ fetch: limited }) (asserting bodyUsed === false on the first response, then that a second request is admitted) would pin the behaviour against a later change of queuing strategy. A companion negative test — an unread non-empty body does not release — would document the abandoned-stream contract from item 4 of the review at the same time.
There was a problem hiding this comment.
Added in bac0b8e: limitConcurrency releases immediately for content-length: 0 responses drives a 200 with content-length: 0 through the wrapper and asserts bodyUsed === false / body unlocked on the first response before a second request is admitted, so the release no longer depends on the queuing strategy's eager pull. A HEAD counterpart covers the other short-circuit shape. The abandoned non-empty-body contract is pinned by the existing queue test (headers arrive, slot stays held until the body is read) and the reserved-capacity test, where an unread streaming body keeps its slot until the source closes.
|
Re-tested commit bac0b8e end-to-end (local streaming server through the SDK's real Reserved unary capacity & overflow (the review's starvation scenario): Queued abort now points at the cap: No-byte release + regression (all passed)Unit tests: 15/15 in both node and workerd (cloudflare) configs. Earlier v1 run (pre-review-fixes) also included a differential run proving the pre-fix code leaks past the cap on unread streaming bodies while the fix does not. Not covered: live-sandbox repro of the original starvation (no Tested by Devin — session link |
|
@devin-ai-integration check comments |
…ately Streaming Connect RPCs can no longer wedge the unary calls (kill, sendStdin, list) that end them: the envd RPC semaphore reserves one unary slot, with overflow when the cap is too small to carve it out. Responses carrying no bytes (content-length: 0, HEAD, locked/used bodies) release their slot immediately instead of relying on the stream's eager pull, and queued-abort errors name the inflight env var. Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
|
Re: the blocking starvation finding — addressed in bac0b8e, porting the reserved-unary-capacity design from #1695 (which is now closed, so consolidation happens here):
Tests cover the reserved cap (stream queues while unary is admitted), the |
|
@devin-ai-integration what's our Python story here |
No parity change needed — there is no counterpart to mirror. This PR fixes the JS-only Python's transports run on pyqwest/reqwest, which manages HTTP/2 stream limits itself: reqwest/hyper negotiates If we ever want a client-side concurrency budget in Python (e.g. to bound total open streams per process), that would be a new feature — happy to open an issue for it, but nothing here regresses or diverges Python behavior. |
|
@devin-ai-integration can we implement this on top of undici negotiates SETTINGS_MAX_CONCURRENT_STREAMS per connection and opens additional connections when a connection's streams are saturated, rather than erroring |
Good news: undici 8 already does this, and we already prefer it (
Empirical probe (local h2 TLS server advertising
So the transport layer already "opens additional connections when saturated, rather than erroring" on the path modern Node takes. What it can't do is distinguish request classes: when every connection is saturated by long-lived Connect streams, a We could push the reservation into a custom undici (Probe scripts run against the exact versions pinned in |
|
@devin-ai-integration what would you do? can we drop undici 7 altogether in our stack? |
Yes, I think you can — with one caveat about the supported Node range. Why undici 7 is there: the SDK's engines are What I'd do:
Happy to open the follow-up PR/issue if you want — it's a small, mostly-deletion change plus a changeset and CI matrix update (the workflows still test on Node versions that would fall out of range). |
|
lets keep this pr as is and follow up with PR for 2. |


Summary
Fixes #1666.
limitConcurrencyreleased its semaphore slot as soon asfetcherresolved with the response headers, so streaming responses (logs, command output) stopped counting against the cap while their bodies were still occupying HTTP/2 streams — the cap could overshoot intoERR_HTTP2_TOO_MANY_CONCURRENT_STREAMS. This resolves the existingTODOininflight.ts.Now the slot is held until the body ends:
No-byte responses release immediately (
hasBodyToTrack): null-body statuses (204/205/304), a body a mock/interceptor already read or locked (bodyUsed/locked— also preventsgetReader()throwing past the wrapper and leaking the slot),HEADresponses, andcontent-length: 0. These are exactly the shapesopenapi-fetchreturns without touching the body, so a slot waiting on one would deadlock the cap.Reserved unary capacity (consolidating the fix from the now-closed #1695): long-lived Connect streams (
Content-Type: application/connect+…— background commands, PTYs,watchDir) would otherwise hold slots for their whole lifetime and starve the unary calls that end them (commands.kill,sendStdin,list,pty.*) on the same envd RPC semaphore.limitConcurrency(fetcher, max, { reserved })caps streaming requests atmax - reserved; whenmaxis too small to carve out the reservation (e.g.max=1), unary gets a matching overflow slot beyondmax, so a stream can never wedge its own teardown.createEnvdRpcFetchreserves 1 slot. Unary waiters are woken before streaming ones.Queued-abort messaging: a request aborted while still waiting for a slot now rejects with a
TimeoutErrornaming the env var that configured the cap (E2B_API_INFLIGHT_REQUESTS/E2B_ENVD_INFLIGHT_REQUESTS/E2B_ENVD_RPC_INFLIGHT_REQUESTS) instead of echoing the caller'srequestTimeoutMsabort — raising the request timeout only lengthens the hang.releaseOnBodyEnd:ReadableStreamreading throughbody.getReader()that releases (idempotently, via the semaphore's slot handle) when the underlying stream is drained, errors, or is cancelled. Reading through the reader — rather than handing the stream itself tonew Response(...)— keeps foreign/cross-realm streams working, matching the repo's class-agnostic conventions inis.ts;reader.closed, so a source that terminates between pulls (e.g. an abort erroring the stream while a chunk sits in the queue with no read pending) still releases the slot;new Response(passthrough, response)getsurl/redirected/typecarried over viaObject.defineProperties, since the constructor only copies status/statusText/headers;Responsecan't represent the original (ponyfill edge cases), it falls back to the original response and releases the slot right away instead of breaking the request.Tests (15 in
tests/api/inflight.test.ts, run in both node and workerd) cover: slot held after headers until the body is read, release on body cancel / error / error-between-pulls, immediate release for204,content-length: 0(assertingbodyUsed === false),HEAD, and locked/used bodies, reserved streaming capacity with unary admission, themax=1, reserved=1overflow, the env-var-naming queued abort, and field preservation on the wrapped response.No user-facing API change — behavior-only fix; changeset included (patch for
e2b). Python SDK has no equivalent inflight cap, so no parity change is needed.Link to Devin session: https://app.devin.ai/sessions/66bc0b6c5e204bb98db02aacffea68fe
Requested by: @mishushakov