valkey: release the socket keep-alive ref at the close-event entry - #39543
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 39 minutes Limit details: You’ve used the included review currently available. Your 79 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. 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 within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Updated 2:32 PM PT - Aug 18th, 2026
✅ @alii, your commit 06ff535bcdcc4de857a9652b0415c515b633599d passed in 🧪 To try this PR locally: bunx bun-pr 39543That installs a local version of the PR into your bun-39543 --bun |
|
@robobun adopt |
|
Adopted. Confirmed on a debug ASAN build: the five worker.terminate() cases report the Box leak with src/ at main, and the file passes with this branch. Head 06ff535 is rebased on current main (#39570) and re-verified both ways. Review threads are resolved and the PR body has the verification details. Ready for a maintainer to check the adopt sites (contract in the comment below) and merge once CI finishes. |
There was a problem hiding this comment.
Both earlier test nits are addressed in 33a88b9 (the filler await now rejects on 'error', and the dial-pending case is test.concurrent.skipIf). This run found no further issues. The change relocates an unsafe ScopedRef::adopt across three close-event entries and drops the stand-in ref in ValkeyDeferredClose; the ref-count balance reasoning is sound as written but is exactly the class REVIEW.md flags for human sign-off, so a maintainer look at the adopt/forget pairing would still be worthwhile.
Checked: the three new adopt sites each sit after a fresh ref_scope() guard so the count stays >0 through the adopt's drop; connect() only forget()s after a socket exists, so DeferredClose::WithoutSocket correctly no longer takes a stand-in ref; reject_all_pending_commands now drains both queues fully and keeps the first error; the semi-socket branch of close() still runs on_close() when an exception is already pending and combines both results with .and().
Extended reasoning...
Overview
Moves the socket keep-alive ref release from on_valkey_close/on_valkey_reconnect (reached only via fallible ? chains inside ValkeyClient::on_close()) to RAII guards at the entry of each socket close event: SocketHandler::on_close, SocketHandler::on_connect_error, and the semi-socket branch of ValkeyClient::close(). Also removes the stand-in ref_() from ValkeyDeferredClose::WithoutSocket and makes reject_all_pending_commands drain both queues past the first failed rejection. Adds five ASAN/LSan-gated worker-termination leak tests.
Security risks
None. Internal lifetime management only; no user-controlled input paths change.
Level of scrutiny
High. This is intrusive refcounting in native code with three new unsafe { ScopedRef::adopt } sites — the exact category REVIEW.md calls "the most-blocked". An imbalance here is a UAF or a leak. The reasoning in the PR description is careful and matches the code (each adopt is paired with the socket_ref.forget() in connect(), and each entry point holds its own ref_scope() so the adopt cannot be the last ref), but a human reviewer familiar with the uSockets close-event contract should confirm that every socket connect() returns gets exactly one of these three entries.
Other factors
- Both nits from the previous run were fixed in the head commit (33a88b9).
- The PR description states all five new tests report the leak on main and pass with the fix; robobun is confirming both sides.
thrown.and(closed)in the semi-socket branch was checked: ifsocket.close()left an exception,on_close()still runs (releasing the adopted ref via the guard) and both errors are combined — the earlier version returned early and would have leaked the ref there too.- The
DeferredFailure::runpath also callsreject_all_pending_commands, so the queue-drain fix covers the finalized-client deferred path as well.
|
For the reviewer who signs off on the adopt/forget pairing, this is the contract I checked.
Coverage per shape, as of 06ff535: the four worker.terminate() cases with a server hit the established socket. The pending dial case hits the |
There was a problem hiding this comment.
Both earlier nits are addressed in 33a88b9/c5852ce8 and this pass found nothing new. Deferring to a human for sign-off on the adopt/forget pairing itself — this is intrusive-refcount balancing across uSockets close-event delivery, and robobun's contract comment already frames exactly what needs verifying.
What was reviewed:
- Traced the forgotten ref in
connect()against each of the three adopt sites plusDeferredClose::WithoutSocket; confirmedon_valkey_close/on_valkey_reconnecthave no callers outsideValkeyClient::on_close(), so removing their adopts does not strand a ref. - Checked
ValkeyClient::close()'s new semi-socket branch runson_close()even with a pending exception — matches whatSocketHandler::on_closealready does for an established socket, andon_valkey_close's ownhas_exception()guard bails before entering JS. reject_all_pending_commandsnow reads out every entry soEntry.serialized_datadrops; first error is kept withoutand().
Extended reasoning...
Overview
Moves the socket keep-alive ref release from on_valkey_close/on_valkey_reconnect (reached only via ValkeyClient::on_close(), which can early-return on ? when promise rejection fails during VM termination) to a ScopedRef::adopt guard at the entry of each socket close event: SocketHandler::on_close, SocketHandler::on_connect_error, and the SEMI_SOCKET branch of ValkeyClient::close(). DeferredClose::WithoutSocket drops its stand-in ref_() since connect() never forgot a ref on that path. reject_all_pending_commands now drains both queues fully after the first rejection fails, keeping the first error. Seven new ASAN/LSan tests in valkey-gc.test.ts cover each on_close() branch under worker termination plus the two SEMI_SOCKET entries on the main thread.
Security risks
None. This is lifetime/refcount bookkeeping in the Valkey client; no auth, crypto, input parsing, or trust-boundary changes.
Level of scrutiny
High. This is exactly the "reference counts provably balanced on every terminal path" category from REVIEW.md — intrusive refcounting across an FFI boundary where an imbalance is either a leak or a UAF. The correctness argument depends on uSockets delivering exactly one close event per handle connect() returns, which the PR body and robobun's follow-up comment document with source-line citations into packages/bun-usockets/src/{socket,context}.c. That contract is the thing a human should confirm.
Other factors
- My two prior nits (filler error → reject;
test.concurrent.skipIf) are fixed and the threads resolved. - The PR body documents that each new test fails on main with the exact LSan report and passes with the fix, satisfying the "fails for the right reason" bar.
- I grepped for other callers of
on_valkey_close/on_valkey_reconnect— there are none outsideValkeyClient::on_close(), so removing their adopts cannot over-release on some other path. - The semi-socket branch now calls
on_close()even whenglobal.has_exception()— this is a deliberate change (stated in the PR body) that makes it consistent withSocketHandler::on_close;on_valkey_closealready guards onhas_exception()before entering JS, and the secondfail()is a no-op viaflags.failed. - Not auto-approving because refcount rebalancing in native code with an
unsafe { ScopedRef::adopt }at three new sites is the kind of change REVIEW.md flags as needing a maintainer to sign off on the invariant, and robobun's comment is explicitly addressed "for the reviewer who signs off".
c5852ce to
3b2d0dd
Compare
…able items (#39570) Replaces #39545. The problem The Valkey client keeps two queues: the offline queue of serialized commands and the in-flight queue of promises. Both were LinearFifo rings. LinearFifo never drops its items. Every consumer of that ring except these two holds bytes, raw pointers or Copy structs, so that was fine there. Here each item owns a JS promise handle and a boxed byte buffer. That leaks on main today. reject_all_pending_commands moves both queues into locals and rejects each item with `?`. When a reject throws, for example during a worker teardown, the function returns early and both locals are dropped with items still inside. Those promises and boxes are never freed. The ASAN tests in #39543 observe exactly this leak. There was a second bug. Two places read the queue with `readable_slice(0)`, which only returns the first contiguous half of a wrapped ring. The auto-pipeline count and the memory estimate both under-counted once the ring had wrapped. What changed The two queue aliases are now std::collections::VecDeque. Every call site is a mechanical rename: init to new, readable_length to len or is_empty, readable_slice(0)[0] to front, write_item to push_back, read_item to pop_front, the two whole-queue scans to iter. Control flow is unchanged. VecDeque drops what is left inside it, so any early return now frees the remaining items. #39543 still fixes the drain loop itself so every promise gets rejected; this PR only makes the early return leak-free. LinearFifo now requires `T: Copy` on all of its impl blocks. The ring never runs item destructors, and the bound states that where cargo check and rust-analyzer see it, before monomorphization. Two consumers needed a derive: FillItem in the lockfile tree builder and RefDataValue in the test runner. Both hold only integers, raw pointers and Copy structs. Every other consumer was already Copy. The per-method `T: Copy` clauses that read, write, unget and peek_item carried are gone with the impl-level bound, and the memmove helper is now slice::copy_within. The header comment states the contract. Visible changes Two, both fixes. Before, the flush wrote the pre-wrap segment of the ring, stayed registered, and wrote the rest only when the event loop woke again. Nothing about the pending flush shortens the poll, so that wake was whatever else happened to fire: a reply, a timer, other I/O. Measured with nothing else live, the tail of a burst left about 80 ms after its head. Now every pipelineable command goes out in one write. estimateShallowMemoryUsageOf counts every queued command's bytes. Before, it counted only the pre-wrap segment. Tests Three new tests in test/js/valkey/reliability/connection-failures.test.ts. One drains and refills the queue so the old ring wrapped, then checks the memory estimate covers all queued bytes. It fails on main (2244 bytes reported for 5000 queued). The other queues 40 commands against a stub that never finishes the handshake, closes, and checks all 40 reject. That one passes on main too and is there to pin the behaviour. The third runs the client in a child process with nothing else live and has a stub count the GETs in the first read after the ring wrapped: main writes 2 of 5 there and the other 3 on a later wake, this branch writes all 5. Not in this PR The drain loop in reject_all_pending_commands still stops at the first throwing reject. #39543 fixes that. The DeferredFailure path when the VM is already stopping still does not settle its promises; that is a gap for the state machine rewrite. <!-- robobun:evidence:begin --> --- **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/valkey/reliability/connection-failures.test.ts <!-- robobun:evidence:end --> Two follow-ups outside this PR. #37618 puts an OwnedRef into the MySQL request queue's LinearFifo; with the Copy bound that no longer compiles, and the queue should become a VecDeque the same way, which also removes its manual Drop drain. The Postgres request queue holds raw pointers today and is the next candidate for the same change. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
…e concurrently The server helper took helloOn and endOn options that no case used. It is one string now. The filler's connect await rejects on error instead of hanging until the test timeout. The dial case uses test.concurrent like the other four cases in the block.
|
@robobun rebase |
3b2d0dd to
06ff535
Compare
|
Your force-push at 21:14 UTC (head 06ff535, on main at 5ebcaff) landed while I was resolving the same conflict, so I dropped my copy and verified yours instead. The src diff is unchanged apart from the pop_front and mem::take renames from #39570. With src/ at main the five worker.terminate() cases still fail (the box is still reported leaked, the byte leaks are gone now that the queues drop their items), and the file passes on the branch (17). I updated the problem and "Not in this PR" paragraphs in the body for #39570. Nothing further to push from my side. |
There was a problem hiding this comment.
I reviewed this PR and the bug hunting pass found no issues; the two earlier test nits (filler error → reject, test.concurrent.skipIf) are addressed. Because this relocates ScopedRef::adopt sites for the socket keep-alive ref across three close-event entries plus the semi-socket branch of ValkeyClient::close() — an unsafe refcount-balance contract robobun already flagged for maintainer sign-off — a human look at the adopt/forget pairing is still warranted.
What was reviewed:
- Traced the three new adopt sites and the removed adopts in
on_valkey_close/on_valkey_reconnectagainstconnect()'ssocket_ref.forget()— one forget per socket, one adopt per close event, andDeferredClose::WithoutSocketno longer takes a stand-in ref becauseconnect()never forgot one on that path. - Checked
reject_all_pending_commandsnow drains both queues fully before returning the first error, so entries left behind on a termination-pending VM drop rather than leak. - Verified the semi-socket branch of
close()still runson_close()when an exception is already pending and combines both results with.and(); the adopt there is guarded by every caller holding its own scoped ref.
Extended reasoning...
Overview
This PR moves the release point of the socket keep-alive ref on JSValkeyClient from the callees on_valkey_close/on_valkey_reconnect (reached only through ValkeyClient::on_close(), which can early-return on ? when the VM's termination is pending) to the entry of each socket close event: SocketHandler::on_close, SocketHandler::on_connect_error, and the half-open-socket branch of ValkeyClient::close(). It also makes reject_all_pending_commands continue draining both queues after a rejection fails, and drops the stand-in ref in DeferredClose::WithoutSocket. Seven ASAN-only LSan tests are added covering each on_close branch, the offline-queue leak, the on_connect_error entry, and the semi-socket close() path.
Security risks
None. This is internal refcounting on the Valkey client's box; no user-facing surface, parsing, or auth path is touched.
Level of scrutiny
High. Per REVIEW.md this is squarely in the "Reference counts provably balanced on every terminal path" category — unsafe { ScopedRef::adopt(...) } at three new sites, with the old adopts removed. An imbalance here is either a leak (the bug being fixed) or a use-after-free (over-release). The PR body and robobun's follow-up comment lay out the exact per-socket-shape contract against uSockets' dispatch guarantees, and explicitly ask a maintainer to verify it. That is the right call: the argument is careful and looks sound to me, but it depends on uSockets delivering exactly one close event per handle across established/connecting/semi-socket shapes, which is the kind of cross-crate invariant a maintainer should confirm.
Other factors
- Both earlier inline nits (filler error wired to reject;
test.concurrent.skipIf) are resolved in the current diff. - The test suite is thorough (one case per
on_close()branch, plus offline-queue and semi-socket paths), verified fail-on-main/pass-on-branch under LSan per the PR body, and existing reliability tests were re-run. - The
ValkeyClient::close()rewrite now runson_close()even whensocket.close()left an exception pending — a behavior change from the old early-return that looks intentional (so the ref is always released) but is another reason for a human eye. - alii has adopted the PR and requested a rebase; robobun's comment frames this as "ready for a maintainer to check the adopt sites and merge once CI finishes", which matches deferring here.
Stacks on #39543 (the socket keep-alive ref moves to the close event's entry); this PR is based on that branch and its diff shows only the timer changes. The problem The RedisClient has two timers. `reconnect_timer` schedules the next retry. `timer` bounds one attempt (connect timeout) or one idle period. Only the retry path and the finalizer disarmed them. Three things went wrong because of that. 1. `close()` during a retry delay did nothing. The client was `Disconnected`, so `close()` returned early. The retry timer stayed armed. It dialled a closed client, fired `onconnect` on it, and kept the process alive for the whole retry schedule. 2. `connect()` during a retry delay dialled at once but left the retry timer armed. The timer fired into the in-flight dial and opened a second socket on the same client. Both sockets then drove one state machine. 3. The connect timer of an attempt that ended in a terminal close stayed armed. A dial that fails outright arms no timer of its own, so the stale one fired "Connection timeout" into the next attempt. What changed `close()` during a retry delay cancels the retry. It disarms `reconnect_timer`, marks the close as manual, and runs the close path once: the cached `connect()` promise rejects, `onclose` runs once, and the event loop ref is released. Nothing dials again. This path takes no ref of its own: the timer's ref goes back with the disarm, and there was never a socket ref to release, which is the accounting #39543 established for a dial that never got a socket. `reconnect()` disarms `reconnect_timer` before it dials. It only dials from `Disconnected` with no socket. A `connect()` that takes over a pending retry is now the one dial. A retry timer that fires while a dial is in flight does nothing. `on_valkey_close` disarms `timer`, as `on_valkey_reconnect` already did. No attempt's timer outlives it. `connect()` asserts in debug builds that the client has no live socket, and the deferred close of a dial that failed outright asserts the same instead of returning early on a live socket, since no dial can start during its hold. Visible changes `close()` between retries is honoured. `onclose` receives `ERR_REDIS_CONNECTION_CLOSED` with the message "Connection closed". A `connect()` promise still pending from before the retries rejects with the same error. Queued commands reject with it too. The process can exit. `connect()` between retries: if a `connect()` promise is still pending (the client has not connected yet and its first dial is being retried), it is returned and the retry schedule is left alone. Otherwise the pending retry is cancelled, the client dials at once, and retry counting starts over from zero, so a client one retry from giving up gets a full `maxRetries` budget again. Before, the dial happened too, but on top of the retry, which then opened a second socket. Tests New tests in `test/js/valkey/reliability/connection-failures.test.ts`: - `close()` during the retry delay fires `onclose` once, rejects the queued command, and the stub sees no second connection. - A spawned process that calls `close()` during the retry delay exits with one `onclose` and no second `onconnect`. - `close()` after the retries are exhausted does not report a second close. - `connect()` during the retry delay opens one connection while HELLO is held past the retry deadline, then resolves. - `connect()` during the retry delay starts the retry budget over: with `maxRetries: 2` and a stub that drops every later connection, the client is dropped three more times before it gives up. - `close()` during that in-flight dial reports the close once. - The connect timer of an attempt ended by `close()` does not fire into a next attempt whose dial fails outright in the same loop iteration. The stale deadline is measured from before the first `connect()`, and the test asserts the callback ran before it, so a slow machine fails the test rather than passing it vacuously. New case in the ASAN block of `test/js/valkey/valkey-gc.test.ts`: a worker calls `close()` during the retry delay and is then terminated; LSan reports nothing, which pins the ref accounting above. The first, second, fourth and seventh fail on main. The third, fifth and sixth pin behaviour that already held (the fifth passes without the fix too: the stub drops each connection within the 50ms retry delay, so the stale retry timer is re-armed before it can fire). The valkey-gc case fails with the `cancel_reconnect` of the first commit, which took a ref of its own: LSan reports the `Box<JSValkeyClient>`. This supersedes #33306 and #32803. Their tests are kept in adapted form here. Not in this PR The failure reason reported to `connect()` and `onclose` (#39542). The `Connection` state enum. <!-- robobun:evidence:begin --> --- **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/valkey/reliability/connection-failures.test.ts test/js/valkey/valkey-gc.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
The teardown test from #39543 pinned connect() rejecting with ERR_REDIS_CONNECTION_CLOSED while the queued command got the timeout. With this branch both get the one timeout error, which is what build 101138 printed.
2 participants
Supersedes #39193 and keeps its three tests.
The problem
connect() takes one keep-alive ref on the RedisClient for its socket. The ref was released inside on_valkey_close and on_valkey_reconnect. Both are reached only through ValkeyClient::on_close(). Every branch of on_close() rejects promises before it gets there, with a ? after each call. Rejecting a promise fails while the VM's termination is pending. That is the state a terminated worker tears its sockets down in. So on_close() returned early, no releaser ran, and the client's box leaked. LSan reports it as a direct leak of 880 bytes from Box::new. Since #39513 the deferred close for a dial with no socket took a stand-in ref that relied on the same path.
The first failed rejection also stopped the drain of the command queues. The remaining entries were never rejected. Until #39570 their promise handles and serialized bytes leaked as well, because the queues did not drop their items.
What changed
The socket ref is now adopted by a guard at the entry of each close event, right after the existing scoped ref: SocketHandler::on_close, SocketHandler::on_connect_error, and the half-open socket branch of ValkeyClient::close(). That branch also runs on_close() now when an exception is already pending, and returns both results. The deferred close for a dial with no socket no longer takes a stand-in ref. connect() forgets its socket ref only once it has a socket, and that task exists because it never got one, so there was nothing to give back. The two adopts inside on_valkey_close and on_valkey_reconnect are gone.
reject_all_pending_commands keeps reading both queues after a rejection fails and returns the first error. on_close() itself is unchanged.
Why this shape
The ref belongs to the socket. It is released where the socket's close event ends, not where a callee happens to be reached. Any new ? in on_close(), fail() or reject_all_pending_commands can no longer bring the leak back. The MySQL client already does this in its on_close. #34874 and #36837 become unnecessary.
Tests
test/js/valkey/valkey-gc.test.ts, ASAN only. The three cases from #39193 (retry scheduled, autoReconnect off, retries exhausted) terminate a worker with commands in flight. Two more cover the offline queue: commands queued behind a non-pipelined command, and commands queued behind a dial that never completes, which is the on_connect_error entry. All five report the leaked box on main and pass with this change. Two more cover the half-open socket branch of ValkeyClient::close(), on the main thread: the client dials an IP literal whose accept queue is full, so uSockets hands connect() a real socket that never opens and delivers no close event for it. One case calls close() while the dial is pending. The other lets connectionTimeout fire during it. Each asserts the connect() rejection, the queued command's rejection, one onclose call, connected false, and a clean exit under LSan. Both report the leaked box with that branch's adopt removed and pass with it. A main-thread process.exit() variant was tried and dropped: that teardown has no termination pending, the rejections succeed, and nothing leaked before this change either.
Not in this PR
The queues themselves: #39570 (merged) made them VecDeque, so an early return no longer leaks the items left in them. This PR is rebased on it. The drain loop here still rejects every entry instead of stopping at the first failure.
Verification
Debug ASAN build on linux-x64, branch as rebased onto main at 5ebcaff (#39570): commits 99cb497, 9aaa1ec, 06ff535. The src diff is the same as before the rebase apart from the
pop_front()andmem::takerenames that #39570 made inreject_all_pending_commands.bun bd test test/js/valkey/valkey-gc.test.tswith src/ at main: the five worker.terminate() cases fail, 12 pass. Each failure reports the 880 byteBox<JSValkeyClient>as leaked. The two close() cases pass on main as well: that branch released the ref on main too, so they pin the rewritten branch rather than show the leak.test/js/valkey/reliability/{connection-failures,recovery,error-handling}.test.ts28 pass (the rest need the docker setup and skip here, CI runs them), andtest/js/web/workers/worker-terminate-lifetime.test.ts24 pass. Its one failure (the dns.lookup c-ares case) fails the same way with src/ at main and does not involve valkey. It is reported separately.9aaa1ec and 06ff535 only touch the test file.
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/valkey/valkey-gc.test.ts