Repository navigation
fix(adapter): read the rest of a body its answer left unread, or close the connection - #439
Conversation
…e the connection The native server handed the runtime `Readable.toWeb(req)`. A body the application stopped reading left the request paused, or destroyed with its connection detached, and in both cases Node.js stopped reading the connection. That happened to a Server Action over its size limit, to a middleware's side of a split body nobody read, and to a POST to a static page. The connection then answered nothing more until its keep-alive timeout. A client that sent its next request on it waited that long: ten minutes with `--keepAliveTimeout 600000`. A slow upload stalled until `requestTimeout`. The body is now read from the request directly. Cancelling it stops the reading and leaves the request alone. Once the answer has gone out, the rest is read and dropped within workerd's grace (64 KiB, 1 s). Past either, the connection is ended and dropped 500 ms later. A read of the body after its answer fails, as it does in workerd.
|
Arkor — Preview deployments for arkorlab-upwind Use the checkbox below to choose whether this pull request gets preview deployments.
Checking this builds the latest commit and future pushes. Unchecking stops queued and future builds; a running build can finish and an existing preview stays available. |
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. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The account paying for this security review has reached its Codex usage limits. The payer can check the Codex usage dashboard. For personal accounts, using credits requires enabling “Use credits for security reviews” in Code review settings. If you do not manage the paying account, contact this repository's admins. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 36 minutes. View limit details
 |
| Layer / File(s) | Summary |
|---|---|
Request body stream and cleanup packages/adapter/src/native/request-body.ts |
nativeBody streams request chunks, handles stream and request lifecycle events, and drains unread data within byte and time limits before closing the connection. |
Adapter integration and lifecycle tests packages/adapter/src/native/http.ts, packages/adapter/test/native-request-body.test.ts |
nativeRequest uses nativeBody for non-GET/HEAD methods. Tests cover body reads, cancellation, socket reuse, and connection closure. |
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix
Sequence Diagram(s)
sequenceDiagram
participant Client
participant Adapter as Native HTTP adapter
participant Body as nativeBody
participant Request as IncomingMessage
participant App as Application
participant Response as ServerResponse
participant Socket
Client->>Adapter: Send HTTP request
Adapter->>Body: Create body stream for non-GET/HEAD
Request->>Body: Deliver request chunks
App->>Body: Read or cancel body stream
Response-->>Body: Emit finish before request completes
Body->>App: Error an open stream
Body->>Request: Drain remaining request data
Body->>Socket: End socket if drain grace expires
Merge Risk: 
2d305
-
A cancelled upload can keep consuming data while the application prepares its response. This is a bounded merge risk that merits a pause-on-cancel fix.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title names the main change: the adapter now reads the unread remainder of a request body after the response, or closes the connection when the drain limits are exceeded. The raw summary and objec… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title names the main change: the adapter now reads the unread remainder of a request body after the response, or closes the connection when the drain limits are exceeded. The raw summary and objectives both describe this behavior in packages/adapter/src/native/request-body.ts and http.ts, so the title is fully related to the changeset. It is specific and conveys the primary fix.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 4 functions across 3 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
 [Medium impact] The PR appears safe to merge; both previous findings are fixed, and no new actionable issues were found. SummaryThe PR replaces
Reviews (2) · Last reviewed commit: "fix(adapter): fail a late read of any un..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/adapter/src/native/request-body.ts">
<violation number="1" location="packages/adapter/src/native/request-body.ts:82">
P2: Cancelling after the body has started flowing does not stop Node from reading it. Pause the request before removing the listener so cancellation preserves the bounded drain behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| }; | ||
| const shut = (): void => { | ||
| open = false; | ||
| req.off('data', onData); |
There was a problem hiding this comment.
P2: Cancelling after the body has started flowing does not stop Node from reading it. Pause the request before removing the listener so cancellation preserves the bounded drain behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/adapter/src/native/request-body.ts, line 82:
<comment>Cancelling after the body has started flowing does not stop Node from reading it. Pause the request before removing the listener so cancellation preserves the bounded drain behavior.</comment>
<file context>
@@ -0,0 +1,117 @@
+ };
+ const shut = (): void => {
+ open = false;
+ req.off('data', onData);
+ };
+ req.pause();
</file context>
| req.off('data', onData); | |
| req.pause(); | |
| req.off('data', onData); |
…led one - The body stream queued up to 64 KiB ahead of the application. A small body could therefore be read whole into its queue and closed, and a read after the answer still succeeded. The stream now queues nothing ahead of the application's reads. - At the answer's end, a body left open fails its readers whatever was received. A body received whole is only dropped, because the connection already reads its next request. - `req.read(0)` marks the body as begun. Otherwise Node.js reads a body nothing has read to its end before the next request, with no bound. - Cancelling now pauses the request. Without a 'data' listener, a flowing request kept reading its connection, past the grace. - The test of a stalled body bounds its close at 3 s.
|
The account paying for this security review has reached its Codex usage limits. The payer can check the Codex usage dashboard. For personal accounts, using credits requires enabling “Use credits for security reviews” in Code review settings. If you do not manage the paying account, contact this repository's admins. |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
Readable.toWeb(req)as the request body. When the application stopped reading a body, the request was left paused, or destroyed with its connection detached. Either way, Node.js stopped reading the connection.HttpServerSettings.How it showed up
In the 16.4.3 deploy-tests nightly (run 41491299214),
app-action-size-limit-invalidand its node-middleware variant timed out onnode, once on Node.js 24 and once on 26./formafter the 3 MB "exceeds the limit" action never completed.upwind start --keepAliveTimeout 600000(feat(start): take --keepAliveTimeout, as next start does #434), so the browser's next request on that connection waited for the 10-minute timeout.Verification
The new
native-request-body.test.tshas 6 cases. 4 of them fail on main: the connection is not closed after the answer, a slow remainder is not bounded, a cancelled body leaves the connection stuck, and a late read succeeds.The upstream suite's sequence (the 3 MB plaintext action, then
/form) was driven with Playwright against a local build. All runs used a keep-alive timeout of 10 minutes:upwind start, mainupwind start, this PRnext startThe tests ran on Node.js 24.22.1 and 26.13.0.
Not in this PR
The private bridge between the runtime and Node.js handlers leaves the same kind of connection behind.
That is #440.