fix(js-sdk): enforce inflight concurrency cap on streaming bodies - #1667
fix(js-sdk): enforce inflight concurrency cap on streaming bodies#1667ujjwalredd wants to merge 3 commits into
Conversation
|
We require contributors to sign our Contributor License Agreement, and we don't have @ujjwalredd on file. You can sign our CLA at https://e2b.dev/docs/cla . Once you've signed, post a comment here that says '@cla-bot check' |
🦋 Changeset detectedLatest commit: 6442b84 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 |
![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: 96e53df180
ℹ️ 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".
| return new Proxy(res, { | ||
| get(target, prop, receiver) { | ||
| if (prop === 'body') return bodyProxy | ||
| return Reflect.get(target, prop, receiver) |
There was a problem hiding this comment.
When the limited fetch returns any Response with a body, this proxy passes the proxy object as the receiver for native Response accessors. In Node's Response implementation those accessors/methods (e.g. ok, status, headers, and clone()) read private state from this, so response.ok/response.status throws before SDK handlers such as handleApiError can process the response whenever the inflight cap is enabled. The trap should read/bind native members against target while still special-casing body.
Useful? React with 👍 / 👎.
| return reader | ||
| } | ||
| } | ||
| return Reflect.get(target, prop, receiver) |
There was a problem hiding this comment.
This trap only instruments getReader; all other standard body consumers fall through unwrapped. In the SDK stream error paths that do res.response.body.cancel() (and user code using for await, pipeTo, or body.cancel()), the underlying stream can finish/cancel without calling safeRelease(), leaving the semaphore slot occupied and eventually deadlocking requests once max such streams are handled. Wrap these body methods/iterators or release from their completion/cancel paths.
Useful? React with 👍 / 👎.
| reader.read = async () => { | ||
| try { | ||
| const result = await originalRead() |
There was a problem hiding this comment.
When callers request a BYOB reader with response.body.getReader({ mode: 'byob' }), read() requires the destination view, but this replacement drops all arguments and calls the original with none. BYOB streaming of capped responses therefore throws even though the native response works; forward ...args from the wrapper to originalRead.
Useful? React with 👍 / 👎.
|
@codex address that feedback |
|
To use Codex here, create a Codex account and connect to github. |
1 participant
Fixes #1666
This defers the
release()call for the inflight semaphore until theResponse.bodyis fully consumed, aborted, or errors out by proxying the Response object and its ReadableStream.