node:http: queue pipelined responses on fallback connections instead of throwing ERR_HTTP_SOCKET_ASSIGNED - #36991
Open
robobun wants to merge 8 commits into
Open
node:http: queue pipelined responses on fallback connections instead of throwing ERR_HTTP_SOCKET_ASSIGNED#36991robobun wants to merge 8 commits into
robobun wants to merge 8 commits into
Claude / Claude Code Review
completed
Aug 5, 2026 in 23m 51s
Code review found 1 important issue
Found 4 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 1 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/js/internal/http1_server_fallback.ts:335-339 |
Queued pipelined responses on fallback connections are never destroyed when the socket closes |
| 🟡 Nit | src/js/internal/http1_server_fallback.ts:335-336 |
Fallback pipelining lacks the outgoingData read-pause flood guard |
| 🟡 Nit | src/js/node/_http_server.ts:4041-4043 |
Internal pipelining helpers exported on public node:_http_server surface |
Annotations
Check failure on line 339 in src/js/internal/http1_server_fallback.ts
claude / Claude Code Review
Queued pipelined responses on fallback connections are never destroyed when the socket closes
The fallback socket `'close'` handler never destroys `socket[kPipelinedResponses]`, so if the duplex closes while responses are queued they (and their `req`) are never destroyed and never emit `'close'` — Node's `socketOnClose` aborts them, and the native path does the same in `NodeHTTPServerSocket.#onClose`. Before this PR the fallback path never populated the queue, so this is a newly-introduced leaked error path; mirroring the `#onClose` loop in the fallback `'close'` listener fixes it.
Check warning on line 336 in src/js/internal/http1_server_fallback.ts
claude / Claude Code Review
Fallback pipelining lacks the outgoingData read-pause flood guard
The fallback path queues pipelined responses but doesn't carry over the `kOutgoingData` read-pause flood guard the native dispatcher applies right after `queuePipelinedResponse` (`_http_server.ts:962-963`, mirroring Node's `parserOnIncoming` and exercised by `test-http-pipeline-flood`). `bufferPipelinedWrite`/`End` still increment `socket[kOutgoingData]` on the plain Duplex, but nothing consults it here, and `pausePipelineReads`/`releasePipelineOutgoingData`'s resume both go through `socket[kHan
Check warning on line 4043 in src/js/node/_http_server.ts
claude / Claude Code Review
Internal pipelining helpers exported on public node:_http_server surface
nit: `queuePipelinedResponse` and `advanceResponsePipeline` are Bun-internal helpers, but exporting them from `node:_http_server` puts them on a user-visible Node compat surface — `Object.keys(require('node:_http_server'))` now includes two names Node doesn't have. The fallback file already does `require('internal/http')` on the line right below the new require; consider exporting these two from an `internal/*` module instead so the `node:` surface stays clean.
Loading