redis: gate the socket keep-alive adopt and defer destruction to finalize

[robobun]

Follow-up to #34760. The RefCountedTimer wrapper closed the timer over-release path, but the fuzzer still produces the same heap-use-after-free on b18d5df35e75 (10/10 on a release-asan build, 2/2 under the exact-fault plan).

Problem

access:  state js_valkey.rs:394 <- disarm :428 <- stop_timers :1536
         <- JSValkeyClient::finalize :1524 <- Heap::finalize
freed:   JSValkeyClient::deinit :1726
         <- rc_deref <- drop ScopedRef<JSValkeyClient>
         <- JSValkeyClient::on_connection_timeout :1199
         <- __bun_fire_timer <- All::drain_timers

on_valkey_close / on_valkey_reconnect adopt connect()'s socket keep-alive ref unconditionally. That adopt is the only release on the teardown path that is not tracked against its acquire. The close/fail paths can re-enter: on_data parse fail (or on_connection_timeout -> client_fail) -> fail_with_js_value -> close() dispatches on_close synchronously, and the enter_event_loop_scope drain inside on_valkey_close runs the connect() rejection while the outer socket-handler frames are still on the stack, so user code (or a second dispatch in the same batch) can reach on_valkey_close again with the ref already released. The second adopt spends the JS wrapper's +1; the ScopedRef drops at the end of on_connection_timeout then bring the count to 0 and deinit frees the box while the wrapper is live, and GC finalize -> stop_timers reads the freed allocation.

Fix

take_socket_ref() mirrors RefCountedTimer: connect() records each socket_ref.forget() in a Cell<u32>, and the adopt is a checked_sub so a re-entrant caller with no outstanding ref returns None instead of over-releasing. A counter (not a bool) because a stale reconnect can leave more than one us_socket_t in flight for the same client.

Verification

$ bun bd test test/js/valkey/valkey-gc.test.ts
 9 pass
 0 fail

The new test drives on_data -> parse fail -> close() deterministically with a server that replies !garbage to HELLO. The fuzzer interleaving under on_connection_timeout is timing-dependent (0/35 runs on the CI container vs 10/10 on the reporting hardware), so fuzzer re-verification against this branch is the intended proof for that face.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 624b7911-eb41-4d41-8bdc-a6b50286eee5

📥 Commits

Reviewing files that changed from the base of the PR and between ec40209 and b601586.

📒 Files selected for processing (1)

• test/js/valkey/valkey-gc.test.ts

---

Walkthrough

Changes

The Valkey client now counts and conditionally consumes socket keep-alive references during connection, reconnect, and close handling, with deinitialization validation. A concurrent regression test covers malformed RESP replies, client closure, garbage collection, and clean subprocess completion.

Valkey socket lifecycle

Layer / File(s)	Summary
Socket reference lifecycle src/runtime/valkey_jsc/js_valkey.rs	JSValkeyClient initializes and tracks socket references, consumes them during reconnect and close callbacks, increments them in connection paths, and asserts zero references during deinitialization.
Malformed reply regression coverage test/js/valkey/valkey-gc.test.ts	A concurrent subprocess test sends malformed RESP data, repeatedly connects and closes Redis clients, runs garbage collection, and verifies clean completion.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is clear, specific, and matches the socket keep-alive reference-counting fix.
Description check	✅ Passed	The description covers the change and verification, though it uses Problem/Fix/Verification instead of the template headings.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

^{Updated 4:05 AM PT - Jul 30th, 2026}

❌ @robobun, your commit efea2cc has some failures in Build #85531 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 34874

That installs a local version of the PR into your bun-34874 executable, so you can run:

bun-34874 --bun

[github-actions]

Found 2 issues this PR may fix:

1. Segmentation fault at address 0x8 (After ~2 hours) #21002 - Segfault during GC heap sweeping after ~2 hours of Redis usage, consistent with use-after-free of a finalized JSValkeyClient object
2. redis.connect behave very wrong when failed to connect (see description) #18895 - Silent exit on connection timeout with RedisClient, consistent with premature destruction when on_connection_timeout drops the final ref

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #21002
Fixes #18895

🤖 Generated with Claude Code

[robobun]

The diff is ready for review. The over-release is timing-dependent (fuzzer reproduces 10/10 on the reporting hardware; 0/20 on the CI container on both the parent and this branch), so the mechanical fail-before check cannot observe it. The new test exercises the auto-reconnect close path and would surface the UAF under ASAN if the timing fires, but the intended proof is re-running the fuzzer's public repro and exact-fault plan against this branch.

The fix is two-layered: take_socket_ref() gates the only remaining unconditional ScopedRef::adopt on the on_connection_timeout call tree, and the destructor guard stops any refcount-0 from freeing the box while the JS wrapper is still attached (assertion builds panic; plain release donates the ref back so finalize() frees normally). Both are no-ops on every existing test path (no debug_assert fires across the nine valkey-gc.test.ts cases on debug+ASAN or release+ASAN).

[claude]

Thanks — b909587 addresses the earlier feedback (wrappers now fall out of scope so finalize() is reachable, and the header is honest that the interleaving is timing-dependent). I didn't find a bug in the Rust change, but this is intrusive-refcount memory-safety code — the third fix in this exact path in as many PRs — and the destructor guard's release-build recovery (restore the stolen +1 rather than free) is a design choice worth a maintainer's eye, so leaving this for a human.

What was reviewed:

• take_socket_ref() mirrors RefCountedTimer::take_fire_ref; both socket_ref.forget() sites in connect() increment the counter, and deinit asserts it back to 0.
• The ref_() inside destructor at count 0: in debug builds debug_assert!(false) fires first (and DebugData::deinit has already zeroed magic, so ref_()'s assert_valid would panic anyway); in release the Cell<u32> bump 0→1 is fine and finalize() later re-enters destructor with finalized=true.
• Per the gate evidence the new test passes on the parent's debug+ASAN build, which the revised comment now states plainly — it's defensive coverage plus the destructor assertion, not a CI repro.

Extended reasoning...

Overview

Follow-up to #34760 in src/runtime/valkey_jsc/js_valkey.rs: (1) a socket_refs: Cell<u32> counter gates the previously-unconditional ScopedRef::adopt in on_valkey_close/on_valkey_reconnect via take_socket_ref(), (2) RefCounted::destructor now checks !finalized && this_value.is_not_empty() and, if so, restores a ref instead of freeing (debug builds panic). One new test in test/js/valkey/valkey-gc.test.ts exercises the auto-reconnect close → connection-timeout interleaving.

Prior review

I flagged that the first version of the test held every wrapper in a clients array read after both Bun.gc(true) calls, so finalize() could never run and the finalize claim in the comments was wrong. b909587 dropped the array, moved the .connected probe into the loop, and rewrote the header to say the race is timing-dependent and the test relies on ASAN plus the destructor assertion. That resolves the structural issue and the misleading comment.

What I checked this pass

• Counter balance: incremented at both socket_ref.forget() sites (the tls_ctx_failed early-close branch and the success tail of connect()), decremented only in take_socket_ref(), asserted 0 in deinit. on_valkey_close/on_valkey_reconnect are the only former unconditional-adopt sites; clone_without_connecting and both create* initialize the counter to 0.
• Destructor guard: traced RefCount::deref_with_context (src/ptr/ref_count.rs:315-342). In debug builds it calls debug.deinit() (zeroing magic) before destructor, so any ref_() from inside would trip assert_valid — but the guard's own debug_assert!(false) fires first, which is the intent. In release builds the #[cfg(debug_assertions)] gates strip both, so ref_() just bumps the Cell<u32> 0→1; finalize() later adopts that +1, sets finalized, and the second entry into destructor proceeds to deinit. assert_no_refs() in deinit sees 0. No leak on the recovery path.

Security risks

None user-facing; this is lifetime bookkeeping with no external-input parsing.

Level of scrutiny

High. REVIEW.md calls native memory safety the most-blocked category, and specifically "never add a ref just to silence ASAN — find the actual imbalance." Here the actual imbalance is being fixed (the socket-ref gate), and the destructor guard is a layered safety net that debug-panics so any other over-release stays visible under ASAN — but silently recovering in release builds is a policy call a maintainer should sign off on.

Other factors

The PR's own gate evidence shows "ASAN without fix: all passed" for the new test, and the one release-build failure on the parent is an unrelated pre-existing test. So the new test is defensive coverage (exercises the path, would catch a future over-release via the debug assertion) rather than a repro that fails on the parent — the revised header comment now says exactly that, and the PR names the fuzzer environment as the intended re-verification. Given the refcount churn across #34714 → #34760 → this PR, deferring to a human.

[claude]

Thanks for addressing the earlier test-structure comment — the clients array is gone and the header now states the timing-dependence honestly. No further issues found, but this is intrusive-refcount / GC-finalize code and the destructor fallback (donate-the-ref-back in release builds) is a design call a maintainer should sign off on.

What was reviewed

• take_socket_ref() counter mirrors RefCountedTimer::ref_held; both socket_ref.forget() sites in connect() increment, both adopt sites consume — balanced on every path traced.
• destructor guard: debug_assert!(false) keeps the over-release loud under ASAN; release-build path restores the ref rather than freeing, so any un-caught imbalance becomes defer-to-finalize instead of UAF.
• New test now lets wrappers fall out of scope so finalize() is reachable; gate output confirms it does not fail on the parent commit (author acknowledges — fuzzer is the intended re-verification).

Extended reasoning...

Overview

Follow-up to #34760's RefCountedTimer refactor. Two changes to src/runtime/valkey_jsc/js_valkey.rs: (1) a socket_refs: Cell<u32> counter gates the previously-unconditional ScopedRef::adopt in on_valkey_close/on_valkey_reconnect via a new take_socket_ref() helper, so a close/reconnect dispatch that arrives without a live socket ref cannot release one it does not own; (2) the RefCounted::destructor impl now checks !flags.finalized && this_value.is_not_empty() and, if the count reaches 0 before the JS wrapper is finalized, restores one ref and returns instead of running deinit (with debug_assert!(false) so assertion builds still panic). A new timing-dependent test in valkey-gc.test.ts exercises the auto-reconnect close path.

Security risks

None user-facing. This is a heap-use-after-free fix in the Redis client's lifetime management; the risk profile is memory safety, not injection/auth/data exposure.

Level of scrutiny

High. This is exactly the category REVIEW.md's memory-safety section is written for — intrusive refcounting, ScopedRef::adopt, GC finalize interaction, and re-entrant teardown paths. The take_socket_ref() gate is a straightforward mirror of the RefCountedTimer::ref_held construction from #34760 and I traced both increment sites (connect()'s tls_ctx_failed branch and its success path) against both consume sites (on_valkey_close, on_valkey_reconnect). The destructor guard is more of a policy question: REVIEW.md warns against "a downstream null-check … papering over the defect" and "never add a ref just to silence ASAN — find the actual imbalance"; here the actual imbalance is being fixed by take_socket_ref(), and the guard is an explicit second layer with a loud debug_assert. Whether that release-build fallback is acceptable (vs. a hard invariant) is a maintainer call.

Other factors

• My earlier inline comment (wrappers pinned by a clients array so finalize() couldn't run) was addressed in b909587; the test now matches the sibling tests' structure and its header is honest about non-determinism.
• The mechanical gate's "fails on main" evidence shows the new test passing on the parent under both debug+ASAN and release; the one release-build failure is an unrelated pre-existing test (ERR_REDIS_INVALID_RESPONSE vs ERR_REDIS_CONNECTION_CLOSED in the RESP simple-string test). So there is no CI-observable regression coverage for this specific fix — the author is up-front that the fuzzer environment is the intended re-verification.
• All nine valkey-gc.test.ts cases pass on debug+ASAN with no debug_assert firing, so the counter is balanced on every existing path.

[robobun]

Confirming this closes the on_data re-entrant face the fleet just flagged on b22e0e6d0a (on_data -> parse fail -> close() -> synchronous on_close -> on_valkey_close adopts, then SocketHandler::on_data calls update_poll_ref on the freed box). I'd started the same take_socket_ref() construction on farm/f4751e5b/redis-socket-ref-held before spotting this PR; the counter here is better than my bool (reconnect can have more than one us_socket_t in flight) and the destructor safeguard catches any other over-release path too. That branch also adds a malformed-RESP-reply test that exercises the on_data -> close path if it's useful to cherry-pick.

[Jarred-Sumner]

This is overly defensive slop. Figure out the underlying issue and fix it.

[robobun]

Agreed, pulling the defensive layers. I still have not pinned the concrete over-release path:

• Traced every ref/deref with stacks through a full auto-reconnect round (on_data -> on_valkey_connect microtask re-entry -> on_close -> on_valkey_reconnect -> subscribe -> close) and every adopt has a matching forget.
• Statically audited every ScopedRef::adopt/deref call site in the module plus the usockets close/connect_error/connecting_error dispatch paths; each reaches on_valkey_close/reconnect exactly once per connect().
• 0/35 runs of the posted repro (and variants that force the reconnect branch) on release-asan here.

I have not been able to reconstruct the 2/2 fault-plan interleaving from the stacks alone. Could you share the fault-plan script (or the exact fault it injects)? With that I can reproduce locally, pin the single unbalanced call, and fix that instead of the gate.

[robobun]

Dropped the destructor guard in e29310e. The fix is now just take_socket_ref(): connect() records each socket_ref.forget() in a Cell<u32>, and on_valkey_close/on_valkey_reconnect adopt via checked_sub so a re-entrant dispatch with no outstanding ref releases nothing. Same construction RefCountedTimer applied to the timer refs.

The unconditional adopt is the only release on the teardown path not paired with its acquire. The re-entry is: on_data parse fail / on_connection_timeout -> fail_with_js_value -> close() dispatches on_close synchronously, and on_valkey_close's enter_event_loop_scope drain runs the connect() rejection while the outer socket-handler frames are still stacked; user code (or a second dispatch in the same poll batch) can reach on_valkey_close again. I traced every ref/deref with full stacks through that sequence and every single-round path is balanced; the over-release needs the second dispatch to land, which is the timing-dependent piece I cannot reproduce here (0/35 vs 10/10 on your hardware). The new test drives the on_data -> parse fail -> close() half deterministically.

The farm/f4751e5b/redis-socket-ref-held branch arrived at the same fix independently against newer main; this PR uses a counter instead of its bool so the stale-reconnect double-connect case does not leak.

[robobun]

Diff is ready at efea2cc: take_socket_ref() counter gating the socket adopt, no defensive layers. 9/9 valkey-gc.test.ts pass locally on debug+ASAN.

The internal QA build check is currently failing on infrastructure unrelated to this diff: the "without fix" lane checks out files from current origin/main (readline.js, node_quic_shim.c, quic modules) into a worktree that still has this branch's older readline.promises.ts, producing a duplicate-key codegen error in generated_resolved_source_tag.rs and a C build error in node_quic_shim.c against the older lsquic headers. My commits touch only src/runtime/valkey_jsc/js_valkey.rs and test/js/valkey/valkey-gc.test.ts; rebasing onto current main would clear the codegen collision if that is preferred over the older base.

Separately, the fail-before check on this fix is the timing-dependent fuzzer interleaving (0/35 on the CI container vs 10/10 on the reporting hardware) documented above; fuzzer re-verification is the intended proof.

[robobun]

Closing this in favor of #39543.

This PR adds a counter around the two ScopedRef::adopt calls in on_valkey_close and on_valkey_reconnect, so that a re-entrant close cannot release the socket ref twice. #39543 removes both of those adopts. The socket ref is adopted once, at the entry of the socket's close event. Each socket gets exactly one such event: uSockets delivers on_close or on_connect_error, and ValkeyClient::close() runs the event itself for a half-open socket that gets neither. A second pass through on_close() then releases nothing, so the counter has nothing left to guard. The author of #39543 notes that it makes this PR unnecessary. It also answers the review comment above: the release moves to where the ref belongs, and no guard is added.

Verified on a debug ASAN build: the test from this PR, applied on top of #39543 (c5852ce), passes together with the rest of valkey-gc.test.ts (17 pass). It also passes on main at 4c68990 without the source change from this PR, which matches the note in the description that the fuzzer interleaving did not reproduce here.

If the use-after-free shows up again after #39543 lands, please reopen with the new report.