valkey: cancel the retry timer on close() and connect()

[alii]

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.

---

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

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your current included review allowance is based on your included PR review attempts over the past 7 days.

Next review available in: 32 minutes

Limit details: You’ve used the included review currently available. Your 75 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.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: b3595727-a6c8-4354-ba2d-b101c9648718

📥 Commits

Reviewing files that changed from the base of the PR and between ec7a24b and a386d4c.

📒 Files selected for processing (4)

• src/runtime/valkey_jsc/js_valkey.rs
• src/runtime/valkey_jsc/valkey.rs
• test/js/valkey/reliability/connection-failures.test.ts
• test/js/valkey/valkey-gc.test.ts

---

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

[claude]

I reviewed this PR and didn't find any bugs. Because it changes the Valkey client's connection state machine and intrusive-refcount balance across timer/close paths, a human look would still be worthwhile.

What was reviewed:

• Traced cancel_reconnect()'s ref_() through on_close() → on_valkey_close()'s ScopedRef::adopt — balanced on the manual-close path (the only caller sets is_manually_closed first).
• Checked the new reconnect() guards against every caller of connect() for the new socket.is_closed() debug_assert.
• Verified js_disconnect's new fall-through for Disconnected && is_reconnecting matches the new disconnect() match arm, and that failed && is_reconnecting can't co-occur (so close() after exhausted retries still early-returns).
• New tests follow harness conventions (poll-until, subprocess pipe drain, local stub server).

Extended reasoning...

Overview

This PR fixes three timer-lifecycle bugs in the Valkey/Redis client (src/runtime/valkey_jsc/js_valkey.rs, valkey.rs): close() during a retry delay left the retry timer armed; connect() during a retry delay left it armed and could open a second socket on the same client; and the connect-timeout timer of a terminally-closed attempt could fire into the next one. The fix disarms reconnect_timer in reconnect() and in a new cancel_reconnect() path reached from disconnect(), disarms timer in on_valkey_close(), and guards reconnect() against dialling when a socket already exists. Six new tests are added to test/js/valkey/reliability/connection-failures.test.ts.

Security risks

None identified. This is client-side connection lifecycle; no untrusted input parsing, auth, or crypto changes.

Level of scrutiny

High. This touches the intrusive-refcount balance (cancel_reconnect takes a ref_() that on_valkey_close's ScopedRef::adopt releases), timer arm/disarm invariants, and the socket/status state machine — exactly the class of code the review guidelines flag as most-blocked. I traced the ref through: disconnect() sets is_manually_closed = true before calling cancel_reconnect(), so on_close() always takes the manual-close branch and reaches on_valkey_close() (never on_valkey_reconnect()), which adopts the ref. The one edge — fail() returning Err via ? before on_valkey_close() runs — is a pre-existing pattern shared with SocketHandler::on_close and ValkeyDeferredClose::run, and the PR description explicitly scopes ref guards to #39543.

Other factors

The tests are thorough and follow the repo's conventions (bounded polling instead of sleeps, local stub servers, subprocess exit assertions with concurrent pipe drain). The PR description states four of the six new tests fail on main. The change is well-reasoned and I found no defects, but the state-machine surface area and refcount coupling put it outside what I'd auto-approve without a human pass.

[alii]

@robobun adopt

[robobun]

Adopted. #39543 has landed and this PR targets main. The head (a386d4c, main merged in after the bun_css build fix #39586 landed) carries the same diff that was green in build 100681. It builds here and the valkey suites pass on an ASAN build (valkey-gc, connection-failures, tls-verify, resp-nesting-depth, #29925: 73 pass, 0 fail). Build 100810 is running for it. The Rust side needs no change. Ready to land once that build is green.

One open PR still overlaps: #39569 (adopts #39542) gives ValkeyClient::on_close() a CloseReason argument. Whichever of the two lands second needs to update the on_close() call in cancel_reconnect.

[claude]

I reviewed this PR and didn't find any bugs. The ref accounting in cancel_reconnect and the new reconnect() gate check out against the callers' scoped refs, and the seven new tests plus the ASAN/LSan case cover each timer path. Because this reworks re-entrant close/timer paths in refcount-sensitive native code (and interacts with #39543's ref-adoption move as robobun noted), a human look at the ref balancing would still be worthwhile.

What was reviewed:

• cancel_reconnect ref accounting: caller (js_disconnect) holds a ref_scope(), so disarm's deref and on_close() are covered; is_manually_closed is set before the call so on_close() takes the terminal branch.
• The reconnect() status/socket gate vs. the WithoutSocket deferred-close hold: Connecting status blocks the retry timer, and do_connect during the hold either returns the cached promise or falls through without dialling.
• The debug_assert replacing the early-return in ValkeyDeferredClose::WithoutSocket: verified no dial entry can slip through during the hold given the new gate.

Extended reasoning...

Overview

This PR fixes three timer-lifecycle bugs in the Valkey/Redis client's reconnect state machine (src/runtime/valkey_jsc/{js_valkey,valkey}.rs): (1) close() during a retry delay was a no-op that left the retry timer armed; (2) connect() during a retry delay left the retry timer armed, opening a second socket on top of the explicit dial; (3) the connect-timeout timer of a terminally-closed attempt could fire into the next attempt. The fix adds cancel_reconnect(), disarms reconnect_timer at the top of reconnect(), gates reconnect() on Disconnected + no live socket, and disarms timer in on_valkey_close. Two conditional early-returns are tightened to debug_assert!s. Seven new tests in connection-failures.test.ts and one ASAN/LSan worker-termination case in valkey-gc.test.ts.

Security risks

None. This is client-side connection-lifecycle logic; no untrusted-input parsing, auth, or crypto changes.

Level of scrutiny

High. This is native Rust touching intrusive refcounting (RefCountedTimer, ScopedRef) on re-entrant close/connect paths — exactly the class the repo's review rules single out ("reference counts provably balanced on every terminal path", "anything that can run user JS can synchronously free your state"). valkey-gc.test.ts documents multiple prior UAFs in this exact subsystem. The PR also converts a defensive early-return into a debug_assert!, which is a tightening that must be provably unreachable.

I traced the ref accounting: cancel_reconnect is only reached via js_disconnect → disconnect(), and js_disconnect holds a ref_scope() guard across the whole call, so reconnect_timer.disarm(self)'s deref and on_close()'s callee chain (which reaches on_valkey_close → user onclose → possible re-entry) are covered. reconnect()'s new disarm is covered by do_connect's or on_reconnect_timer's guard. The new gate in reconnect() (status != Disconnected || socket not closed → return) plus the Connecting hold in close_without_socket_next_tick together justify the WithoutSocket debug_assert. This all looks correct, but the interaction surface (two timers × four status states × re-entrant JS callbacks × the socket keep-alive ref from #39543) is large enough that a maintainer familiar with this state machine should confirm.

Other factors

• CI is green on all lanes (build 100570) and robobun verified the new tests fail on base / pass on branch under ASAN.
• The PR description is unusually thorough about which behaviours changed and which tests pin what.
• robobun flagged two open PRs (#39569, #39543) that will need small rebase adjustments in cancel_reconnect — whichever lands second needs coordination.
• No prior human review comments to address.

[robobun]

Pushed 4cd780d. The branch is rebased, so reset a local checkout before you add to it.

Build 100654 (bac3230) had one real failure: the new valkey-gc case, terminated 0 instead of terminated 1 on x64-asan. It reproduces on a local ASAN build. After close() cancels the retry, nothing keeps the worker alive. The worker exits on its own, and terminate() then reports that exit. This is the fix at work, not a leak: LSan was clean. The case now arms a long timer after close(), so terminate() ends a running worker like the other cases. It passes 5 of 5 runs here. With the cancel_reconnect of the first commit (the one that took a ref) it still fails, and LSan reports the Box<JSValkeyClient>, so it still pins the accounting. The other red lanes in that build passed on retry and are not valkey tests.

The base branch (#39543) moved twice since bac3230 and the PR was conflicting. 33a88b9 turned the gc stub into one string without the endOn option this case used, so the case now passes a flag to terminateWorker that makes the stub end the connection at the INCR. The two commits are otherwise unchanged and are rebased onto c5852ce.

Checked against a build of the base: the first, second, fourth and seventh connection-failures tests fail, the third, fifth and sixth pass. The fifth (retry budget) passes without the fix too, because the stub drops each connection well inside the 50ms retry delay, so the stale retry timer is re-armed before it fires. I changed that sentence in the description. With the fix, connection-failures (35 tests) and valkey-gc (17 tests, with the two new pending-dial cases from c5852ce) pass on the ASAN build.

I also reworded the comment above armedAt in the connect timer test. The deadline computed before connect() is at or before the real one, not after it. The test logic was right, only the comment was inverted.

[claude]

I reviewed this PR and didn't find any bugs. The timer/refcount interactions in the reconnect state machine are subtle enough that a human look would still be worthwhile.

Checked: ref accounting through cancel_reconnect — the js_disconnect scoped guard covers the disarm's deref, and no socket ref is adopted; reconnect() disarming before its own ref_scope is safe because both callers (on_reconnect_timer, do_connect) already hold a guard; the ValkeyDeferredClose::WithoutSocket early-return → debug_assert is sound because the new status != Disconnected gate in reconnect() closes the last dial entry while Connecting is held; droppingServer() re-declares its own connections getter so the spread doesn't freeze it.

Extended reasoning...

Overview

This PR fixes three timer-lifecycle bugs in the Valkey/Redis client's reconnect state machine (src/runtime/valkey_jsc/js_valkey.rs, valkey.rs): close() during a retry delay now cancels the retry instead of no-opping; connect() during a retry delay disarms the pending retry so it can't open a second socket on top of the explicit dial; and the connect/idle timer is disarmed in on_valkey_close so a stale deadline can't fire into the next attempt. It adds a new cancel_reconnect() path, gates reconnect() on Disconnected + no live socket, and converts a defensive early-return in ValkeyDeferredClose to a debug_assert. Seven new behavioral tests plus one ASAN/LSan worker-terminate case cover each fix.

Security risks

None. No untrusted-input parsing, auth, or crypto is touched; the change is internal timer/state bookkeeping.

Level of scrutiny

High. This is intrusive-refcounted native Rust with re-entrant JS callbacks — exactly the "reference counts provably balanced on every terminal path" category REVIEW.md flags as most-blocked. The change adds a new close path (cancel_reconnect) that runs on_close() without a socket event, replaces a defensive runtime check with a debug assertion, and reorders a disarm() (which can deref) before the local ref_scope guard in reconnect(). Each of these is correct on inspection, but the correctness depends on invariants held by every caller and on the Connecting status excluding all dial entry points — the kind of cross-function reasoning a maintainer familiar with this state machine should confirm.

Other factors

The PR is exceptionally well-documented and well-tested: robobun confirmed the new tests fail on base and pass here with ASAN, and CI is green on every lane. The LSan test specifically pins the ref accounting of the new cancel_reconnect path (and caught an over-ref in an earlier commit). It stacks on #39543 and interacts with #39569, both noted for rebase. No prior human review or outstanding comments. The bug-hunting system found nothing. Deferring solely because the refcount/timer state machine is complex enough that a maintainer sign-off is appropriate, not because of any specific concern.

[robobun]

Rebased again and pushed. #39543 was rebased onto main (891c1c7, which brings in #39530), so this branch still carried the old copies of its commits and showed as conflicting. The three commits here replayed onto the new head of #39543 (3b2d0dd) without conflicts, and the diff against the base is unchanged apart from line numbers. Build 100664 for the previous head (4cd780d) passed on all lanes. On the rebased tree, connection-failures and valkey-gc pass on a local ASAN build. The branch history was rewritten, so reset a local checkout before you add to it.

[claude]

I reviewed this PR and didn't find any bugs. Because it stacks on #39543 (not yet landed), reworks refcount/timer accounting in a code path with a history of UAF fixes, and replaces a defensive runtime guard with a debug_assert! in ValkeyDeferredClose::WithoutSocket, a human look is still worthwhile before merging.

What was reviewed:

• Traced ref accounting for cancel_reconnect(): js_disconnect's ref_scope() covers the disarm+on_close(), and disarm() on an already-fired/disarmed timer is a no-op, so no over-release.
• Verified the new reconnect() gate plus the Connecting hold makes the WithoutSocket assertion sound — every entry to connect() (do_connect, send, reconnect) is blocked while the deferred close is pending.
• Checked on_valkey_close disarming timer is idempotent when reached via cancel_reconnect (already disarmed by on_valkey_reconnect).
• Confirmed do_connect returns the cached promise before reaching reconnect(), so a pending first-connect's retry schedule is left alone as described.

Extended reasoning...

Overview

This PR fixes three timer-lifetime bugs in the Valkey/Redis client: close() during a retry delay left the reconnect timer armed (dialing a closed client), connect() during a retry delay left the timer armed (opening a second socket on top of the in-flight dial), and the connect-timeout timer of a terminally-closed attempt could fire into the next attempt. The fix adds cancel_reconnect() (disarm + run on_close() by hand since there is no socket to dispatch a close event), makes reconnect() disarm the retry timer at entry and refuse to dial when a socket already exists or status isn't Disconnected, and makes on_valkey_close disarm the connect-timeout timer. It also converts a defensive early-return in ValkeyDeferredClose::WithoutSocket into a debug_assert!, on the basis that the new reconnect() gate plus the Connecting hold provably keeps every dial entry out. Seven new tests cover each scenario, plus one ASAN/LSan case pinning the ref accounting.

Security risks

None identified. This is connection lifecycle/state-machine work with no parsing of untrusted data, no auth changes, and no new externally-reachable surface.

Level of scrutiny

High. This code has a documented history of heap-use-after-free bugs (several of the existing tests in valkey-gc.test.ts are UAF regression fixtures for exactly this refcount/timer machinery). The PR reasons carefully about which ref covers which call — cancel_reconnect deliberately takes no ref of its own and relies on the caller's ref_scope(), and the LSan test was added specifically because the first commit's version leaked. Converting a runtime guard to a debug_assert! in release builds means the invariant must hold, and while I traced every entry point to connect() and believe it does, a maintainer familiar with this state machine should confirm.

Other factors

• The PR is stacked on #39543 and robobun explicitly noted it is "ready for review once #39543 lands" — merge order matters here.
• There is a known overlap with #39569 (CloseReason argument to on_close()): whichever lands second must update the on_close() call in cancel_reconnect.
• CI was green on all lanes for the previous head (build 100664) per robobun, and the new tests were verified to fail on a build of the base for the four cases claimed to.
• The test coverage is thorough and follows the repo's conventions (bounded polling, subprocess exit assertions, ASAN-gated leak case with a stated failure mode on the unfixed build).

Given the memory-safety sensitivity, the stacked dependency, and the pending merge-order coordination with #39569, I'm deferring rather than approving.

[claude]

I reviewed this PR and didn't find any bugs. Because it touches ref-counted native lifecycle paths (timer disarm ordering, cancel_reconnect running on_close() without a socket ref, and defensive guards converted to debug_assert!), and it's stacked on #39543 with a known merge-order interaction with #39569, a human look would still be worthwhile before landing.

Traced that reconnect()'s new reconnect_timer.disarm(self) runs before its own ref_scope() but both callers (do_connect, on_reconnect_timer) already hold one; cancel_reconnect is only reached via js_disconnect's scoped ref (disconnect() has no other callers); the new socket.is_closed() asserts hold because close_without_socket_next_tick parks the client at Connecting and the new reconnect() gate blocks every dial entry until the deferred task runs. The ASAN/LSan case in valkey-gc.test.ts pins the no-extra-ref accounting in cancel_reconnect.

Extended reasoning...

Overview

This PR fixes three timer-lifetime bugs in RedisClient: (1) close() during a retry delay was a no-op and the armed retry dialled a closed client; (2) connect() during a retry delay left the retry timer armed, which then opened a second socket on the same state machine; (3) the connect-timeout timer of an attempt ended by a terminal close stayed armed and fired into the next attempt. The fix disarms reconnect_timer at the top of reconnect() and gates it on Disconnected && socket.is_closed(), adds cancel_reconnect() (disarm + on_close() by hand) reached from disconnect() when Disconnected && is_reconnecting, and disarms timer in on_valkey_close(). Two defensive guards become debug_assert!s. Seven new tests in connection-failures.test.ts and one ASAN/LSan case in valkey-gc.test.ts cover each path; robobun confirmed 4 of the 7 fail on the base and all lanes are green on the head.

Security risks

None. No untrusted-input parsing or auth/crypto changes; this is client-side connection-lifecycle bookkeeping.

Level of scrutiny

High. JSValkeyClient is intrusively refcounted with timers and socket callbacks each holding a strong ref, and every path here can re-enter user JS (onclose, promise rejections). REVIEW.md's "reference counts provably balanced on every terminal path" and "anything that can run user JS can synchronously free your state" both apply directly. The PR also converts a runtime guard in ValkeyDeferredClose::WithoutSocket and adds one in connect() as debug_assert! — correct by the traced invariants, but a release-build regression if those invariants are ever weakened.

What I checked

• Ref safety of the early disarm in reconnect(): it runs before _guard = self.ref_scope(), but the only callers are do_connect (holds ref_scope()) and on_reconnect_timer (holds ref_scope() plus the timer's fire ref, and the timer is FIRED so disarm is a no-op on both branches). No over-release.
• cancel_reconnect accounting: disconnect() has exactly one caller, js_disconnect, which holds a ref_scope(). disarm releases the timer's ref; there was never a socket ref (connect() forgets it only once a socket exists); on_close() → fail() → close(Failure) returns early on the closed socket → on_valkey_close() disarms timer and runs update_poll_ref. The valkey-gc worker case fails with LSan reporting the Box<JSValkeyClient> when an extra ref is taken, and passes here.
• New debug_assert!(socket.is_closed()) invariants: close_without_socket_next_tick sets Connecting; do_connect on Connecting returns the cached promise; reconnect() now gates on status == Disconnected && socket.is_closed(); send() only dials from NeverConnected. No path reaches connect() or the deferred WithoutSocket task with a live socket.
• close() after retries exhausted: fail() clears is_reconnecting, so js_disconnect takes the early return and no second onclose fires — the third new test pins this.

Other factors

The PR is stacked on #39543 (socket keep-alive ref moves to the close event's entry) and robobun flagged that #39569 gives on_close() a CloseReason argument, so whichever lands second must update the call in cancel_reconnect. There's also a user-visible behavior change worth a maintainer's sign-off: connect() during a retry delay now resets retry_attempts to zero (documented in the description and pinned by the fifth test), and close() between retries now fires onclose where it previously did nothing. Given the ref-counting subtlety, the guard→assert conversions, and the stacking/merge-order coordination, this is not a change I'd approve without a human maintainer's review even though I found nothing wrong with it.

[robobun]

^{Updated 3:33 PM PT - Aug 18th, 2026}

@alii, your commit a386d4c is building: #100810

[claude]

I reviewed this PR and didn't find any bugs. Because it reworks refcount-bearing timer lifecycle in the Valkey client (cancel_reconnect, the new reconnect() gate, and a debug_assert! replacing a live-socket early return in ValkeyDeferredClose::WithoutSocket), a human look at the ref accounting would still be worthwhile.

What was reviewed:

• cancel_reconnect() ref accounting — sole caller is js_disconnect via ValkeyClient::disconnect(), which holds a ref_scope() across the call; disarm releases the timer's ref and on_close() adopts none.
• The reconnect() status/socket gate and its interaction with do_connect()'s Disconnected branch — the cached connection promise short-circuits before reconnect() when a first-dial retry is still pending, matching the description.
• The new timer.disarm in on_valkey_close() — idempotent with on_valkey_reconnect()'s existing disarm; RefCountedTimer::disarm guards on ref_held.
• Tests: the seven new connection-failures cases and the ASAN worker-terminate case await observable conditions (bounded until polls, subprocess exit) rather than fixed sleeps, and the connect-timer test asserts beforeStaleDeadline so a slow machine fails rather than passing vacuously.

Extended reasoning...

Overview

The PR fixes three timer-ownership defects in RedisClient: close() during a retry delay was a no-op (retry timer stayed armed and dialled again), connect() during a retry delay dialled on top of the pending retry (two sockets driving one state machine), and a stale connect-timeout timer could fire into a subsequent attempt. The Rust changes are in js_valkey.rs (js_disconnect now falls through when Disconnected && is_reconnecting; new cancel_reconnect(); reconnect() disarms the retry timer up front and gates on Disconnected + no live socket; on_valkey_close() disarms timer; connect() gains a debug assertion; ValkeyDeferredClose::WithoutSocket swaps its live-socket early return for a debug_assert!) and valkey.rs (disconnect() now matches on status and calls cancel_reconnect() for the between-retries case). ~330 lines of new tests across two files.

Security risks

None identified. This is client-side connection lifecycle management for the Redis/Valkey client — no auth, crypto, or input parsing changes. The stub servers in tests are local.

Level of scrutiny

High. This is native code touching intrusive refcounting (RefCountedTimer, ScopedRef::adopt) across a state machine with multiple re-entry points (timer fire, socket callbacks, JS close()/connect()). REVIEW.md flags reference-count balance on every terminal path as the most-blocked category, and the PR history itself records that an earlier cancel_reconnect iteration took a ref of its own and leaked the Box<JSValkeyClient> — the new valkey-gc case pins exactly that accounting. The swap of a defensive early return for a debug_assert! in ValkeyDeferredClose::WithoutSocket means release builds now rely on the reconnect() gate holding; the reasoning in the PR description is sound (Connecting status plus the socket check block every dial entry), but it's the kind of invariant a reviewer familiar with the state machine should confirm.

Other factors

The test coverage is unusually thorough: seven new scenario tests plus an LSan-gated worker-terminate case, with the description enumerating which fail on main and which pin already-held behaviour. robobun iterated on a CI flake in the gc case (worker exiting on its own after the fix released the last keep-alive) and reports the suite green on ASAN. There is a noted overlap with #39569 (adds a CloseReason arg to on_close()) — whichever lands second needs to update the on_close() call in cancel_reconnect. No prior human review on the thread; alii adopted the PR but has not left review comments.