Repository navigation
Conversation
…close response An async handler that awaits before returning a Response dropped both connection-lifetime signals: a request's Connection: close was not echoed and did not close the socket, and a Response carrying connection: close put the header on the wire but kept the socket open. RFC 9112 9.6 makes both a MUST; the sync request-header path was the only one that already closed. Mechanism: the async response ends inside cork(), so internalEnd takes its corked branch and uncorks without a close check, and cork()'s own close check is behind a findCorkSlot early return that the same uncork triggers. Synchronous handlers end inside onData, whose tail has its own close check. internalEnd now routes both end paths through uncorkAndCloseIfNeeded, which uncorks and then closes when flagged unless the parser is running on THIS socket (a new per-socket isParsingHttp, replacing the per-server flag so a response that resolves inside another socket's request handler still closes). The Connection: close response header is now written whenever the server closes unless the application already wrote a Connection header (HTTP_WROTE_CONNECTION_HEADER, set by Bun.serve's header writer and by node:http's writeHead, which always writes its own). uws_res_end_without_body gets the same close-after-end.
WalkthroughHTTP response parsing state now lives on ChangesHTTP close flow
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
On the suggested issues:
|
|
Updated 2:20 PM PT - Jul 25th, 2026
⏳ @robobun, your commit 7d10e2b is still building in
|
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
On the duplicate-PR flags: both are real overlaps.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
… serverClosed from 'close without client destroy' in the test
The clientError early-return path leaves the socket alive with
HTTP_NODE_PARSING_STOPPED set; without clearing isParsingHttp there, a
later close-flagged response end on that socket would defer its close
to an onData tail that never runs.
The test's on('end') detection of serverClosed is not portable: on
Windows the second write can draw an RST that pre-empts the FIN, so the
socket emits 'close' without 'end'. Track whether the client destroyed
instead; the socket only closes without that when the server closed it.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/http/serve.test.ts`:
- Around line 3759-3776: Replace the polling loop around the /a request in the
fetch handler test with a readiness Promise. Resolve it immediately when armed
is assigned, then await that promise before requesting /b, preserving the
existing request and response behavior without using Bun.sleep.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 020a9266-f49c-40ab-b2b3-81e277c42d47
📒 Files selected for processing (7)
packages/bun-uws/src/HttpContext.hpackages/bun-uws/src/HttpContextData.hpackages/bun-uws/src/HttpResponse.hpackages/bun-uws/src/HttpResponseData.hsrc/jsc/bindings/NodeHTTP.cppsrc/uws_sys/libuwsockets.cpptest/js/bun/http/serve.test.ts
💤 Files with no reviewable changes (1)
- packages/bun-uws/src/HttpContextData.h
…aths; early-return after close in the content-length branch - uws_res_end_sendfile now routes through uncorkAndCloseIfNeeded like the other end paths (it had the identical markDone+resetTimeout shape and its close_connection parameter was already ignored). - Clear isParsingHttp on onData's fall-through (returnedData nullptr without close/upgrade: the HTTP_NODE_PARSING_STOPPED and us_socket_is_shut_down lambda early-returns), guarded by !us_socket_is_closed. - Early-return from internalEnd's content-length branch when uncorkAndCloseIfNeeded closes, matching the chunked branch. - Cross-socket test awaits a readiness promise instead of polling. - Trimmed the flagged multi-line comments to match the surrounding one-line-per-header pattern.
There was a problem hiding this comment.
The findings from my earlier passes were addressed in e2dd58e and no new issues surfaced on this revision, but this reworks connection-close/teardown ordering across every uWS end path (internalEnd, sendfile, end-without-body, upgrade) and moves isParsingHttp from per-context to per-socket state — worth a human look before landing.
What was reviewed:
- Re-traced every
onDataexit path for the per-socketisParsingHttpclear after e2dd58e: the clientError branch, thereturnedData != nullptrblock, and the new fall-through guard now cover all live-socket returns. - Confirmed the content-length branch of
internalEndnow early-returns afteruncorkAndCloseIfNeededcloses, matching the chunked branch; the remaininghasResponded()read on the pre-existing uncorked path was left as-is per the author's reply. - Checked
HTTP_WROTE_CONNECTION_HEADERis set on both the FetchHeaders write path and node:http'swriteHead, and is included inresetResponseState's per-response clear (it is not inHTTP_CONNECTION_SCOPED). - The new test's
exchangehelper wires'close'(not just'end') to resolution and destroys on a second response, so the broken case cannot hang.
Extended reasoning...
Overview
Fixes RFC 9112 §9.6 compliance for async Bun.serve handlers: a request or response carrying Connection: close now actually closes the socket after the response, instead of keeping it alive until idleTimeout. Mechanically: (1) isParsingHttp moves from the per-server HttpContextData::flags to the per-socket HttpResponseData, so a close-flagged response resolving inside another socket's onData (microtask drain) can still close; (2) both branches of internalEnd, plus uws_res_end_sendfile and uws_res_end_without_body, route through a new uncorkAndCloseIfNeeded() that uncorks and then runs the same close check the uncorked path already had; (3) the automatic Connection: close header is now gated on a new HTTP_WROTE_CONNECTION_HEADER bit (set by writeFetchHeadersToUWSResponse and unconditionally by node:http's writeHead) rather than HTTP_CONNECTION_CLOSE, so it is echoed to the client even when the close came from the request. Five raw-net tests cover sync/async × request-header/response-header plus the cross-socket resolution case.
Security risks
None new. This is response-end control flow, not input parsing; the isParsingHttp gate exists specifically to preserve onData's tail body-validation on the sync path (request-smuggling.test.ts was verified). The change surface is close-earlier, not accept-more.
Level of scrutiny
High. packages/bun-uws/src/HttpResponse.h / HttpContext.h are the hot path for every Bun.serve and node:http response, and the change interacts with C++ object lifetime: us_socket_close synchronously runs ~HttpResponseData, so any code that keeps touching httpResponseData after a close is [basic.life] UB even though usockets defers the free. My earlier passes on this PR found four such edge cases (the clientError early-return, two more nullptr-without-close lambda exits, the missing sendfile sibling, and the missing early-return after close in the content-length branch); all were addressed across 8ecd52f and e2dd58e, but the density of subtle exit paths here is exactly why a maintainer familiar with the uWS close/cork state machine should sign off.
Other factors
- All prior inline findings from this reviewer are resolved; the author declined only the
uws_res_try_endreorder with a correct justification (movingclearOnWritableAndAbortedbeforetryEndwould droponWritableon backpressure). - One
comment-copinline (NodeHTTP.cpp:944, 3-line comment) is still open — cosmetic. - The PR body references sibling PRs #33005 and #28390 that this subsumes/rebases; someone with context on those should confirm the intended scope split.
- Tests cover the fix well but the PR body's own verification note says node:http failure sets are "identical to a main debug build", not zero — CI results on e2dd58e should be checked.
|
Status for reviewers (7d10e2b, identical to e2dd58e): the change is complete and every review thread is resolved. CI: neither build for the final diff reached the test lanes.
The test lanes that did run on this diff are in 80255 (8ecd52f, two commits back): the five new The delta since 80255 (e2dd58e) is review-driven: Not pushing further re-rolls while the build fleet is unstable; the diff is ready. |
|
Noting one more case this fixes that is still broken on const upstream = Bun.serve({
port: 0,
fetch() {
let left = 3;
return new Response(
new ReadableStream({
async pull(c) {
if (left-- === 0) return c.close();
await 1;
c.enqueue(new TextEncoder().encode("hi"));
},
}),
);
},
});
const proxy = Bun.serve({
port: 0,
idleTimeout: 0,
async fetch() {
const r = await fetch(upstream.url);
return new Response(r.body, { headers: { Connection: "close" } });
},
});
// net.connect to proxy, GET / HTTP/1.1 (no request Connection header):
// main -> header and 0-chunk arrive, socket stays open forever
// this PR -> socket closes after the 0-chunkThe path is A request-side |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-25, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
2 participants
What
An async handler (any
awaitbefore returning theResponse) dropped both connection-lifetime signals:Connection: closewas not echoed and did not close the socket; a second request on the same connection was answeredResponsecarryingconnection: closeput the header on the wire but kept the socket openRFC 9112 section 9.6 makes both a MUST. The sync request-header path already closed via
onData's tail; every other path kept the socket alive untilidleTimeout.Why
The async response ends inside
HttpResponse::cork().internalEndis therefore corked, takes itselse if (!keepCorked) this->uncork()branch, and returns without a close check.cork()'s own close check at the tail would have caught it, but it sits behind thefindCorkSlot(this) == INVALID_CORK_SLOTearly return thatinternalEnd's uncork just triggered. So nothing ever reachesshutdown()/close().The
Connection: closeresponse header was also gated on(state & HTTP_CONNECTION_CLOSE) == 0, which skips it whenever the close came from the request (the bit is already set), so the SHOULD-echo was only written when neither side had asked for the close.Fix
internalEndnow routes both end paths through a newuncorkAndCloseIfNeeded(), which uncorks and then closes when flagged unless the parser is running on THIS socket (so a sync end insideonDatastill defers toonData's tail andrequest-smuggling.test.ts's body-validation cases keep 400ing). The flag is per socket rather than per server so a close-flagged response that resolves inside another socket's request handler (its microtask drain) still closes;upgrade()now reads the same per-socket flag, captured before theHttpResponseDatadestructor.The response header check now tests
HTTP_WROTE_CONNECTION_HEADER(set bywriteFetchHeadersToUWSResponseand by node:http'swriteHead, which always writes its own) instead ofHTTP_CONNECTION_CLOSE, so the header is echoed whenever the server is closing and the application did not write a Connection header itself.uws_res_end_without_bodygets the same close-after-end, withkeepCorked=trueso node:http's destroy path (which corks, calls it, then force-closes) keeps discarding the buffered bytes rather than flushing them.This is the minimal async-face of #33005, rebased onto
main'suint32_t stateword and theresetResponseState/shouldCloseConnectionhelpers that landed since; #33005 additionally replaces thelength() == 5request-side detection with a token-list scan and discards pipelined bytes behind a close-flagged request.Verification
New
Connection: close on an async handlerdescribe block intest/js/bun/http/serve.test.ts(5 tests, rawnetsocket):Connection: close: one response, oneConnection: closeheader, server FIN, second request refusedResponsewithconnection: close: sameonData: still closesOn
main4 of the 5 fail (sync+response-header already works); with this change all 5 pass.No regressions in
bun-serve-headers.test.ts,request-smuggling.test.ts(81 pass),serve.test.ts,websocket-server.test.ts, ortest/js/node/http/*(failure sets identical to amaindebug build).no test proof · iteration 4 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/http/serve.test.ts