Repository navigation
fix(http): retain corked responses in the socket write queue - #185
Conversation
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. |
![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: 193d527de5
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const result = socket[kWriteOperation]( | ||
| { response, kind, chunk, encoding, length, bodyLength, strict, responseCorked }, | ||
| callback, |
There was a problem hiding this comment.
When a HEAD, 204, or 304 response is corked, this queues the callback even though the body was discarded above. Node schedules that callback on the next tick independently of the cork, so code such as await new Promise(resolve => res.write("ignored", resolve)); res.uncork() now deadlocks because the callback cannot run until the unreachable uncork(). Complete bodyless-write callbacks immediately and queue the header operation with a no-op callback.
AGENTS.md reference: AGENTS.md:L187-L191
Useful? React with 👍 / 👎.
| Writable.prototype[kWriteOperation] = function (operation, callback) { | ||
| return writeOrError(this, this._writableState, operation, kWriteOperation, callback ?? nop) === true; |
There was a problem hiding this comment.
For a corked writeEarlyHints() or writeProcessing() call, a non-function callback such as 123 reaches this path. The direct _writeRaw path and Node ignore non-function optional callbacks, but callback ?? nop stores the value as Writable's completion callback, so afterWrite later attempts to invoke it and raises an uncaught TypeError. Normalize this argument to nop unless it is callable before passing it into writeOrError.
AGENTS.md reference: AGENTS.md:L187-L191
Useful? React with 👍 / 👎.
What changed
ServerResponse.write()bypassed the socket's Writable queue, so an explicit cork could leak bytes and interleaved response/raw socket writes could arrive out of order. Response operations now use that existing queue while the socket is corked or still has queued/in-flight work. The direct native path resumes only once the queue fully drains; response framing remains in the native response writer.This also matches Node's separate response/socket cork counts, force-flushes on
end(), preserves queued header values, and ties backpressure, drain, and callback delivery to the queue. There is no second body buffer or larger native batching buffer. Uncorked writes on an empty queue retain the native fast path.Dispatcher-cork removal adapts oven-sh/bun#35664 by robobun. Related open proposals #44676 and #44877 cover client corking and response accounting; neither implements this explicit ServerResponse retention contract.
Verification
Linux release builds use
bun run build:release --lto=off. The 32 oracle modes pass on Node 24.22.0 and the final candidate. The final macOS CI executable passes the complete 74-case cork file; its Git tree matches this PR head exactly. Coverage includes retention before uncork, nested and cross-surface cork counts,end()while corked, high-water-mark accounting, next-tick/timer release, destruction callbacks, response reassignment on a keep-alive connection, raw/response interleaving (BCDE), and immediate writes after uncork while the queue remains busy. It also covers native backpressure, informational/frozen headers, strict Content-Length edge cases, standalone responses on upgrade sockets, and large informational headers with a paused reader.The supplied pipeline repro delivers zero response headers/body bytes in all seven pre-uncork samples on both Node and the candidate. The unpatched baseline fails 13 of the initial oracle cases; additional edge cases fail the preceding candidate before their respective fixes.
test/js/node/httptest/js/node/http2test/js/node/streamtest/js/web/fetchThe full HTTP suite uses the repository's Node 26.3.0 pin. The new version-specific oracle cases run against Node only on Node 24; the Bun cases always run. No test timeout, memory limit, or assertion was relaxed. JavaScript lint, formatting, and touched native-file formatting pass.
Five alternating baseline/candidate benchmark pairs per size, on the same machine with pinned server/client CPUs, found no uncorked fast-path regression. Each run used a one-second warmup and a three-second measurement with four
wrkthreads and 32 connections:The Linux measurements precede the final standalone and informational-callback corrections. Those corrections affect queued operations; the uncorked, empty-queue path short-circuits before them. All benchmark responses had the expected length; no request/socket errors were reported. The 64 KiB result is within measurement noise; the small-response result improved in this sample, without claiming a general speedup.
Independent Codex and Claude reviews are scoped-clean at P2. Final native CI passes on Linux x64 and macOS arm64. The final artifact source tree matches this PR head.