usockets: follow the whole adopted-socket chain when re-deriving the live socket - #37661
Conversation
…live socket us_socket_adopt retires the old block of a relocated socket with adopted set and prev pointing at its replacement. The dispatcher re-derived the live socket after each callback by following exactly one such link, so a callback that relocated the socket more than once left the dispatcher on a block that was itself retired. Put the walk in one helper (internal.h) and use it at the four loop.c sites and the libuv.c poll_cb helper.
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Updated 4:16 PM PT - Aug 11th, 2026
❌ @robobun, your commit 6a8e47d has 1 failures in
🧪 To try this PR locally: bunx bun-pr 37661That installs a local version of the PR into your bun-37661 --bun |
|
Status: merged as aa9a59f. Bookkeeping-only change in a path no in-tree caller reaches today (see the reachability section in the description), so no failing-before test was possible; verified by building (debug/ASAN) and running the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical dedup of five identical one-hop if checks into a shared static inline helper that walks the whole chain.
What was reviewed:
- Confirmed all readers of
flags.adoptedare covered (grep shows only the new helper reads it; context.c/loop.c/socket.c sites are writers). - Verified
us_socket_adoptsetsadopted=1together withprev=new_sand pushes toclosed_head, and thatus_internal_free_closed_socketsis gated ontick_depth <= 1, so the walk reads live memory mid-dispatch. - Helper is placed after the
us_socket_tdefinition and isstatic inline, so it compiles in the C++ TUs that includeinternal.h; NULL tolerance matches the old checks.
Extended reasoning...
Overview
Replaces five copies of if (s && s->flags.adopted && s->prev) s = s->prev; (four in loop.c, one in libuv.c) with a single static inline helper in internal/internal.h that uses while instead of if, so a multi-hop adoption chain is followed to its live tail rather than stopping after one link.
Security risks
None. No user input reaches this code path differently than before; the helper reads two flag bits and a pointer on structures the dispatcher already holds. No new allocation, no new syscalls, no external data.
Level of scrutiny
This is core socket event-dispatch code, so I looked closely at the one semantic change (if → while). For every currently-reachable input the loop body executes at most once — the PR demonstrates that no in-tree adopter grows its ext, so us_poll_resize early-returns and flags.adopted is never set — making the change behaviorally identical to the old code today. The multi-hop case is a defensive correctness fix for a latent issue, and the termination/liveness argument (retired blocks freed only in the outermost loop_post; the live tail's adopted bit is clear because us_poll_resize copies before us_socket_adopt flags) checks out against context.c and loop.c.
Other factors
- No CODEOWNERS entry covers
packages/bun-usockets/. - The deduplication into a shared helper is exactly what the repo's review guidelines call for when the same block appears at multiple sites, and moving the guard into a shared helper is the preferred fix shape.
- No test is included; the PR's justification (no input distinguishes old vs new behavior, so any test would pass with or without the change) is consistent with the repo rule against tests that cannot fail.
- The libuv.c change is a one-line body swap behind an unchanged
staticsignature, so Windows call sites are unaffected.
2 participants
What does this PR do?
When
us_socket_adopthas to grow a socket's ext it goes throughus_poll_resize, which allocates a newus_socket_t, copies the old one over, and retires the old block:is_closed = 1,flags.adopted = 1,prev = <replacement>, pushed ontoloop->data.closed_head(context.c,us_socket_adopt). The dispatcher inloop.cis usually holding the old pointer while this happens (the adoption runs insideon_open,on_writableoron_data), so after each of those callbacks it re-derives the live socket from that bookkeeping. Until now every one of those sites followed exactly one link:That is only right if the socket was relocated once per callback. Every relocation retires its source block the same way, so a callback that adopts twice before returning (
old -> mid -> new, withmidalso flagged and pointing atnew) leaves one hop parked onmid, which is itself retired. Theis_closedchecks that follow then give up on the rest of the event for a socket that is still live: the readable half of a combined readable+writable event, the rest of the recv drain loop and the eof/error handling behind it (loop.c), the deferred-accept readable dispatch afteron_open, and on Windows the paused-socket probe andfin_deferredbookkeeping inpoll_cb(libuv.c). The kernel re-reports these level-triggered conditions on the next tick, so the result is a dropped event tail rather than a crash, but the bookkeeping is only correct because there happens to be a single link.The fix replaces the five copies of the one-hop
ifwith one helper ininternal/internal.h,us_internal_socket_follow_adopted, which walks to the end of the chain.loop.cuses it at the four dispatch sites, and the libuv.cus_internal_poll_cb_adopted_sockethelper thatpoll_cbalready funnels through now calls it too. Nothing else readsflags.adopted, so these are all the readers.Why walking the chain is sound:
closed_headand are only freed byus_internal_free_closed_sockets, which runs in the outermost tick'sus_internal_loop_post, never from inside a dispatch (see thetick_depthcheck in loop.c). For the same reason a retired block's memory cannot be handed out as a new socket within the tick.us_poll_resizecopies the source beforeus_socket_adoptflags it, so the copy, and therefore the live tail, always hasadoptedclear.adoptedis set in exactly one place and always together withprev, so every link the walk follows is a forwarding link.When
adoptedis clear, which is every socket today, thewhileevaluates its condition once, which is the same work theifdid. The NULL tolerance of the old checks is kept becauseon_writableandon_datamay return NULL.Reachability and testing
This PR has no test because the code it changes cannot currently execute. No in-tree adopter grows its ext:
upgradeTLS, the Postgres and MySQL TLS upgrades (adopt_tls) and the WebSocket client (adopt_group) pass equal pointer-sized old/new ext sizes, and the uWS server WebSocket upgrade shrinks (checked against this tree's headers:sizeof(WebSocketData) + sizeof(void *)is 168 vssizeof(HttpResponseData<SSL>)at 232), sous_poll_resizealways takes its early return andflags.adoptedis never set. The redirection itself dates from #25361; #37098 (open, same grow path) reaches the same conclusion and adds a fault-injection hook so that a single relocation can be exercised. A two-link chain additionally needs two adoptions inside one callback, which nothing JS-reachable does even with that hook. There is therefore no input on which the old and new code behave differently, and any test added here would pass with or without the change, so none is included. The change adds no state and touches no data structures.It also does not touch
context.corepoll_kqueue.c, so it is independent of #37098; if that lands first, its tests run the helper's one-link case for real.How did you verify your code works?
internal.h(AsyncSocket.h,libuwsockets.cpp,bindings.cpp). libuv.c is Windows-only and its change is a one-line body replacement behind the same signature; the Windows CI lanes compile it.bun bd test test/js/bun/net/socket.test.tsandtest/js/bun/websocket/websocket-server.test.tscover the in-tree adopters (upgradeTLS, server WebSocket upgrade). The only failures are the ones this sandbox also produces with the released binary:localhostresolving to::1makes a handful ofBun.connecttests fail with ECONNREFUSED, one test needs public DNS, and the 300k-message(benchmark)case exceeds its 30s budget under ASAN. Everything else passes.