Conversation
…p, not from the caller us_socket_resume() closes the socket when the poll registration it needs fails (#33974). The close dispatched on_close from inside resume(), which runs from the middle of an owner's work: uWS destructs the HttpResponseData and Bun's abort handler frees the RequestContext that work still uses. The loop now owns that close. us_socket_resume() lists the socket with the close code and wakes the loop, and us_internal_loop_post() closes it at the same point it frees closed sockets, where no owner frame is on the stack. The list drops every address the socket free invalidates, and follows a socket us_socket_adopt() moved.
|
Status Reproduced on the debug + ASAN build of main with the existing Run it: Review. The automatic review raised 5 findings. Four are addressed. In 5d8eca9: the close now runs at every tick depth, and the fixture now proves that the injected failure fired (that also exposed a wrong order in the The fail-before runs (12 of 12 on the base build) used the fixtures as of 5d8eca9. The two commits after it do not change what the fixtures do, and were run on the build with the change only. The automated self-review did not complete. It was started three times and was cut off each time before it reported, so there is no result from it. These points were checked by hand, by reading the code:
Not covered by a test: a socket that is adopted, or a loop that is torn down, between the failed resume and the close. The other owners of a resumed socket (the fetch client, the WebSocket client, the SQL clients) get the same deferred close, but only |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. WalkthroughThe change defers closure of sockets whose poll registration fails during ChangesResume failure handling
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to Failed socket resumes are now closed during loop processing rather than on the caller’s stack, preventing the reported request-state crashes. The targeted fault paths pass, so this change is mergeable with normal checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 12:10 AM PT - Sep 22nd, 2026
✅ @robobun, your commit c5c81912a19c7b50b422db4107cfbadac8a0382d passed in 🧪 To try this PR locally: bunx bun-pr 43738That installs a local version of the PR into your bun-43738 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the C/Rust mirror of us_internal_loop_data_t field-by-field (int tick_depth → int num_unresumable_sockets → pointer, same order and #[repr(C)], so the layout stays in sync), and the drain in us_internal_close_unresumable_sockets (pops one entry per iteration and re-reads the count, so a close handler that lists or closes another socket during the walk is not affected).
Extended reasoning...
The change moves the failed-resume close in usockets from the caller's stack to the loop's post step via a new realloc-grown list on the loop data, with a Rust struct mirror and Linux-only ASAN fault-injection tests. It touches no auth, crypto, or input-parsing surface. Three inline findings were posted (nested-tick drain gap, a test proof gap, and a pre-existing kqueue sibling), so the inline comments already signal that a human look is warranted; this note only records what else was examined.
…t prove the fault fired us_internal_loop_post() closed the listed sockets only at the outermost tick. A tick that a callback started can be the one that waits for that close (expect().rejects from inside a dispatch), and it then never ended. The close now runs at every depth; only the free waits for the outermost tick, as before. A frame below an inner tick has yielded to the loop, whose dispatch closes sockets at that depth already. The fixture's pings now use a connection of their own. fetch() reuses a unix connection, which registers no poll, so the ping after the handler could not see a rule the resume had not consumed. With that fixed, the response-ends mode showed that its ping ran before the response ended: it now waits until the server releases the request. A fourth case covers the inner tick.
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
5d8eca9 addresses both points from the earlier pass (the drain now runs at every tick depth, and the fixture's pings open their own connection). Beyond the inline nit, I also checked the drain loop against a close handler that lists another socket: it copies the entry out and re-reads data->unresumable_sockets each iteration, so a nested us_realloc cannot leave a stale pointer; the array has one owner (us_internal_loop_data_free) and starts zeroed from the loop's calloc. Given this is a new deferred-close lifetime path in uSockets, a human look is still worthwhile.
Extended reasoning...
The change adds a deferred-close list to uSockets loop data (loop_data.h, loop.c, socket.c), a matching Rust struct mirror, and Linux-only fault-injection tests. It touches no auth, crypto, or input-parsing surface. Not approving because it introduces a new socket-lifetime mechanism in native code that interacts with adopt/free ordering, and a verified finding from this run is not being posted.
…timeout The previous commit gave the four cases a 30 s timeout, with a comment about the time a sanitizer report takes to symbolize. That was not why it was added: run together, the four debug + ASAN processes took 3.8 s to 4.8 s each on a loaded machine, against the 5 s default. test/CLAUDE.md says not to set a timeout on tests. Run one at a time they take 1.9 s to 3.5 s on the same machine, which is where the other cases of this file are, so they use the default again.
… one shutdown per fixture A fixture that crashes, which is the failure these cases catch, left its unix socket file in the shared tmpdir: the path was built by hand and removed on the success path only. The test now owns a tempDir and hands the socket path to the fixture, so the cleanup does not depend on how the fixture ends. Both fixtures stopped the server by hand and through `using`. The graceful stop() is the one that matters (it resolves once every connection is gone), so it stays and `using` goes.
There was a problem hiding this comment.
I reviewed the latest push and found no bugs; the fixture cleanup from the previous round is addressed (one shutdown per fixture, socket under the test's tempDir). Because this changes when usockets closes a socket relative to the caller's stack, a human look at the loop.c lifetime handling would still be worthwhile.
What was reviewed:
- The deferred-close list against the only socket free site (loop.c
us_internal_free_closed_sockets) — entries are pruned or re-pointed throughus_internal_socket_follow_adoptedbefore that free, and the drain skips already-closed sockets, so a re-list, a self-close, or an adopt between resume and loop_post does not leave a dangling entry. - The every-depth drain in
us_internal_loop_postversus thetick_depth <= 1free — the close only moves the socket ontoclosed_head; the free still waits for the outermost tick. - The Rust mirror in
src/uws_sys/InternalLoopData.rs— field order matches the C struct and the existingoffset_of!(PosixLoop, num_polls) == size_of::<InternalLoopData>()assert covers the size. - The nested fixture's un-awaited
expect().rejects— it goes throughvm.wait_for_promise, so it is the inner tick the test claims to exercise.
Extended reasoning...
The change touches the vendored usockets C loop (loop.c, socket.c, loop_data.h, internal.h), moving the failure path of us_socket_resume from a synchronous close on the caller's stack to a realloc-grown list drained in us_internal_loop_post, plus the matching Rust layout mirror and four new Linux-only ASAN fault-injection tests. It touches no auth, crypto, or injection surface; the sensitive part is socket lifetime and memory ownership in C. All four of my earlier inline comments were followed by commits that plausibly addressed them (every-depth drain, fresh-connection pings, default timeout, tempDir + single shutdown), and the bug hunt ran dry with no findings. I could not run the tests here (no debug/ASAN build present, and the fault hook is only compiled into ASAN builds), and the change alters ordering of close-handler dispatch in a shared C loop, so I am deferring rather than approving.
This branch has not been deployed
1 participant
Problem
us_socket_resume()closes the socket when the kernel refuses its poll registration (epoll_ctl, ENOMEM or ENOSPC). It dispatched that close on the caller's stack: uWS destructs the response state and the abort handler frees Bun'sRequestContext, so the caller works on freed state.Bun.servewith the failure injected:use-after-poisoninRequestContext::live_resp(fromon_start_buffering), aJSC::WeakImplSEGV(fromPendingValue::set_promise), andpanic: assertion failed: this.resp.get().is_some()inon_abort(fromdetach_response).socket.c, theus_poll_changebranch) and it is needed: nothing else delivers that socket's tail, end or close.Fix
us_socket_resume()lists the socket with its close code and wakes the loop.us_internal_loop_post()closes it, so the close never runs insideresume(). The list drops every address the socket free invalidates, and follows a socketus_socket_adopt()moved.serve-syscall-fault.test.ts, each failing on main in 3 of 3 runs. Also theBun.serve, node:http,node:netand fetch suites.Background
us_socket_pause()drops a socket's read interest for backpressure; the unread bytes wait in the kernel. A peer's hangup cannot be masked, soloop.ctakes the paused fd out of the epoll set instead of acting over that tail (usockets: defer eof for a paused socket that already sent FIN; stop backpressure pauses from holding the loop #33974). The resume is then a freshEPOLL_CTL_ADD.Bun.servepauses the request socket for a handler that reads its body slowly (Bun.serve: apply TCP backpressure to a request body the handler reads slowly #36006). That resume runs fromreq.arrayBuffer(), from a stream read, and from the end of the response.loop->data.closed_headholds the sockets whose close already ran.loop_postfrees them a tick later, because a dispatch on the stack may still read one. The new list is the step before it.Notes
The reproduction.
bun:internal-for-testing'ssocketFaultInjectionhas apoll_startrule for this path, which is unreachable otherwise; it is compiled in for ASAN builds. A unix socket, because the close of its peer raises the hangup at once (a TCP peer has to take both directions down). The client sends more thanREQUEST_BODY_HIGH_WATER_MARKand closes, the server pauses, the dispatcher parks the fd, then the handler's body read or response end resumes it. The fixture makes one/pingrequest before it arms the fault, which is what makes the loop poll and park the fd. epoll only: kqueue and libuv never park the fd, so their resume is a plain filter or poll change with nothing to fail. With the fixtures as of 5d8eca9, 12 of 12 runs fail on the base build (four fixtures, three runs each) and none fails with this change. The two commits after it change how the cases run and where the socket file lives, not what the fixtures do, and were run on the build with the change only.The proof that the fault fired (review finding). The fixture makes a second
/pingafter the handler: a one-shot rule that the resume did not consume fails the first poll registration of that connection. The first version usedfetch()for it, which reuses a unix connection and so registers nothing. With the rule armed, a secondfetch()returned 200 and anet.connect()of its own failed withECONNREFUSED. Both pings are now a connection of their own. That showed a second problem: in theresponse-endsmode the last ping ran before the response ended, consumed the rule itself, and the resume never failed (after: error ECONNREFUSED). On main that mode had crashed only because the reused connection left the rule for the resume. The fixture now waits untilserver.pendingRequestsis 0, which is after the resume in every mode.The close runs at every tick depth (review finding). The first version closed the listed sockets under the same
tick_depth <= 1condition as the free, on the reasoning that the owner that resumed the socket can be on the outer stack. That reasoning does not hold: a frame below an inner tick has yielded to the loop, the dispatch of that inner tick closes sockets at that depth already, and so can any JavaScript that runs there. What a caller cannot take is a close insideresume()itself. With the condition, a tick that a callback started could wait for that close without end.serve-resume-fault-nested-fixture.tscovers it: the resume fails inside the dispatch of another request, which then waits on the loop with anexpect().rejectsthat is not awaited. With the condition in place that test timed out at 5000 ms.The test timeout (review finding). The second push gave the four cases a 30 s timeout, with a comment about the time a sanitizer report takes to symbolize. That comment did not give the real reason. The real reason was the passing path: run together, the four debug + ASAN processes took 3.8 s to 4.8 s each on a loaded machine (load average about 130), against the 5 s default.
test/CLAUDE.mdsays not to set a timeout on tests. The cases now run one at a time on the default, like the other cases of that file: 1.9 s to 3.5 s each on the same machine, 12 of 12 passes over three runs.The socket file and the shutdown (review finding). The fixtures built their unix socket path by hand in the shared tmpdir and removed it on the success path only, so a fixture that crashed left it behind: the runs on the unfixed build for this PR had left 37 of them. The test now owns a
tempDirand hands the path to the fixture, so the cleanup does not depend on how the fixture ends. Both fixtures also stopped the server twice, by hand and throughusing. The gracefulstop()is the one that matters (it resolves once every connection is gone, the failed one included), so it stays andusinggoes.Follow-up, not in this PR (review finding, also on main). On kqueue,
us_poll_change()discards the result ofkqueue_change(), so a refusedEV_ADDleaves a resumed socket open and deaf on macOS where epoll now closes it. Returning that result would makeus_socket_resume()list the socket there too, with no other change. It is left out because it changes which sockets close on macOS and it could not be run here: no fault rule reaches that path (poll_startis inus_poll_start_rc(), which the kqueueus_poll_change()never calls), andkqueue_change()returns 1 for every error event that is not theENOENTof a delete, so each errno anEV_ADDon a live socket can report needs a look first.The automated self-review did not complete. It was started three times and was cut off each time before it reported. The review findings above come from the automatic reviewer on this PR.
The crashes in full, from the fixtures:
req.arrayBuffer()on a paused body:AddressSanitizer: use-after-poison,READ of size 8,RequestContext::live_resp(RequestContext.rs:4283) fromon_start_buffering(:4363). The poisoned read is the freed pool slot of the context. The nested fixture dies the same way.req.text()on a materialized body:SEGVinJSC::WeakImpl::state(), fromreadable_stream::Strong's drop insidePendingValue::set_promise(Body.rs:435).panic: assertion failed: this.resp.get().is_some()inon_abort(RequestContext.rs:1460), reached fromdetach_responsethroughend_without_body. A release build has no assertion there and releases the context twice instead.Why not report the closure out of
resume(). The first shape returned the verdict throughHttpResponse::resume()anduws_res_resume()intoAnyResponse::resume() -> bool, with a bail-out per call site. That needs a correct bail-out in about ten places (detach_responsealone is reached from twelve), each of which must also keep the context alive across the call to read its own state afterwards, and it leaves the next caller exposed.detach_responseadditionally had to disarm the abort handler before the resume, since an abort must not fire for a request that is being ended. Thereq.text()crash is inBody.rs, three frames above the resume, so the bail-outs do not stop at the callers either. Deferring the close removes all of that.The sites this covers, all on main, all of which resume and then keep using the response or the context:
src/runtime/server/RequestContext.rs:on_start_buffering,on_buffered_body_chunk(three resumes),on_request_body_stream_drained,detach_response.src/runtime/webcore/streams.rs:HTTPServerWritable::mark_response_ended.packages/bun-uws/src/HttpResponse.h:HttpResponse::resume()callsresetTimeout(), which readsidleTimeoutout of the destructed ext block.src/jsc/bindings/node/JSNodeHTTPServerSocket.cpp,JSNodeHTTPServerSocketPrototype.cpp: the node:http flood-prevention resumes.packages/bun-usockets/src/crypto/openssl.c: the resume in the TLS adopt, whose comment already says that no dispatch may happen there, because the caller has not repointed the ext slot yet.Not changed, and one edge.
us_socket_pause()ignores the return value of its ownus_poll_change(). A pause the kernel refuses only means reads keep flowing, which costs the backpressure and nothing else. The edge:us_loop_run_bun_tick()returns at once when no poll refs the loop, so a listed socket that its owner unref'd, in a process with nothing else alive, is not closed before that process exits. Before, itscloseevent ran insideresume().Not covered by a test. A socket that is adopted, or a loop that is torn down, between the failed resume and the close (the pruning in
us_internal_free_closed_sockets()is for those). The other owners of a resumed socket (the fetch client, the WebSocket client, the SQL clients) get the same deferred close, but onlyBun.serveand the existingnode:netcase insocket-syscall-fault.test.tsrun under the injected failure.Suites, debug + ASAN.
serve.test.ts,bun-server,bun-serve-file,request-smuggling,serve-close-delimited-framing,serve-pending-promise-abort-leak,node-http,node-http-backpressure,node-http-server-socket-end-drain,socket.test.ts,node-net,fetch-backpressure,fetch.stream,serve-syscall-fault,socket-syscall-fault,node-http-syscall-fault,fetch-syscall-fault,ws-syscall-fault,websocket-syscall-fault,tls-syscall-fault,socket-fault-injection, and upstreamtest-http-pause,test-http-pause-no-dump,test-http-pause-resume-one-end,test-net-write-slow. After the review changes:serve-syscall-faultandsocket-syscall-faultagain, and thenode:netparked-resume case 10 of 10 on its own.Environment, not from this change.
serve.test.ts'sroot range portand/bun:infoloopback cases fail the same way on the released build here (the tests run as root).node-net.test.ts's reused-handle leak case spawns 16,000 connects and exceeds its 60 s budget under ASAN.node-http-syscall-fault.test.ts's TLS proxy case needs 6.0 s against the 5 s default and passes with--timeout. After the review changes the host was under load (load average about 130): in 2 of 5 runs of the two fault files together, cases with the 5 s default timed out. In the run whose output was kept they wereBun.listenand the parked resume ofsocket-syscall-fault.test.ts, together, and theBun.listencase never resumes a socket. Three more runs of that file passed, with those two cases at 2.2 s to 4.9 s, and the parked resume takes 2.4 s to 2.9 s on its own.Related. Found during the work on #38128, which guards only the two resumes it adds; with this change those guards never trigger. #41384 holds a tick level across
close_allso a nested tick frees nothing, and #37099 changes whatus_socket_pause()arms. Both touch these functions and conflict textually at most.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/http/serve-syscall-fault.test.ts