valkey: don't let a stale reconnect timer clobber an in-flight socket - #32803
valkey: don't let a stale reconnect timer clobber an in-flight socket#32803robobun wants to merge 2 commits into
Conversation
WalkthroughAdds a reconnect-status guard and socket-closed assertion in the Valkey client, plus a concurrent integration test that exercises explicit ChangesValkey reconnect race fix
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 3:11 AM PT - Jul 11th, 2026
❌ @robobun, your commit e7f52e6 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 32803That installs a local version of the PR into your bun-32803 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
#18895 is a different bug, so I'm not adding the auto-close line. That issue is about connection attempts that fail (connection refused or an unresolvable host): the This PR is about the opposite situation: a connection attempt that succeeds while a stale reconnect timer starts a second one, which orphans the in-flight socket. It does not make #18895's promises settle, so it should not close it. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the guard is strictly narrowing (reconnect only from Disconnected) and the debug_assert is release-inert.
Extended reasoning...
Overview
This PR touches src/runtime/valkey_jsc/js_valkey.rs (two hunks: a 3-line early-return guard in reconnect() and a debug_assert! in connect()) and adds a regression test to test/js/valkey/valkey-gc.test.ts. The fix prevents a stale auto-reconnect timer from calling connect() while another connection is already in Connecting/Connected, which previously overwrote client.socket and orphaned a live fd whose late callbacks then drove a client with a weak/collected JS wrapper.
Security risks
None. This is internal connection-lifecycle bookkeeping for Bun.RedisClient; no auth, crypto, parsing, or user-input handling is touched. The change strictly reduces the set of states from which a new socket can be opened.
Level of scrutiny
Low-to-moderate. The source change is tiny and conservative:
- The new guard only adds an early return — it cannot cause
reconnect()to do more than before, only less. The one other caller ofreconnect()(do_connect()) already gates onStatus::Disconnectedbefore calling it, so the new check is a no-op there and only constrains the timer path. debug_assert!(socket.is_closed())is release-inert; I verifiedis_closed()returnstruefor theDetachedvariant (src/uws_sys/socket.rs:272-276), which is what every transition intoDisconnectedand the initial state assign, so the assert holds on existing call paths.
Other factors
- No CODEOWNERS entry for valkey.
- The PR description is unusually thorough (full repro trace, cause analysis, before/after test output on both debug and release builds).
- The regression test has two independent oracles (accepted-connection count, clean exit) so it does not depend on debug assertions, and I traced the with-fix path to confirm the count is deterministically 3 regardless of whether the stale timer fires before or after the explicit
connect(). - The test is somewhat timing-orchestrated (
sleepSync(60), 50ms reconnect-delay assumption), but the primary oracle (no 4th accept) holds under any interleaving once the guard is in place, so flakiness risk is low. - Bug hunter found nothing; no prior human review comments to address.
2dd220a to
71214e2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/valkey/valkey-gc.test.ts`:
- Line 230: The timing-sensitive subprocess fixture in valkey-gc.test.ts is
being run under test.concurrent, which can skew reconnect-timer deadlines and
leave child processes unmanaged. Change the affected test group to run
sequentially using the existing test wrapper in this file, and keep the
child-process/reconnect logic under a non-concurrent test declaration. Use the
surrounding test case setup in valkey-gc.test.ts to locate the concurrent block
and make only this fixture sequential.
- Around line 212-229: The comment block in the valkey GC test is too long and
includes bug-history details that should not live in source comments. Trim the
preamble in the test fixture to only the durable invariant and keep the rest of
the failure narrative in the PR description; update the comment near the
scripted RESP3 server/reconnect scenario so it stays within the 3-line limit and
still identifies the reconnect/orphaned socket edge case.
- Around line 316-324: The reconnect flow in the valkey GC test is not being
asserted, because c.connect() is fire-and-forget and its rejection is ignored.
Update the test around the explicit reconnect on c so it awaits the reconnect
promise and fails the test if it rejects, then only close c after the awaited
reconnect completes. Use the c.connect() call and the surrounding HELLO-reply
release logic to locate the spot, and make the test assert that reconnect really
succeeds before checking the accepted socket count.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: abe01aa8-63d0-4631-a6f5-cfb5b7320113
📒 Files selected for processing (2)
src/runtime/valkey_jsc/js_valkey.rstest/js/valkey/valkey-gc.test.ts
|
CI on the rebased head (e7f52e6, build 71876, final): 284 jobs passed. The new regression test in The two red lanes are both darwin-14-aarch64 shards failing on runner environment issues unrelated to this diff:
Their first attempt also expired waiting on an agent before the retries above ran. One yellow: Review feedback addressed, all threads resolved. The diff is green; the red lanes need a runner fix or a skip, not a code change. I'm not pushing retrigger commits. |
|
Independently reproduced the mechanism in this PR on stock 1.4.0 with a scripted RESP3 server: after While reproducing it I hit an adjacent bug this diff does not cover: Verified both are independent: with the |
An auto-reconnect timer armed by a connection loss stays armed when an explicit connect() starts a new connection. When it fired, reconnect() called connect() again, which overwrote `client.socket` without closing the in-flight socket. The orphaned socket's late open/data callbacks then drove a client that had already moved on: they reset `is_authenticated`, sent another HELLO, and handled its reply while the client was Disconnected and its JS wrapper only weakly referenced, failing `debug_assert!(self.this_value.get().is_strong())` in on_valkey_connect and, in release builds, running against a wrapper the GC is free to collect. reconnect() now only proceeds from Status::Disconnected, and connect() debug-asserts that the previous socket is closed or detached.
Run it sequentially, trim the preamble to the invariant, and await the explicit connect() so a rejection fails the test instead of being swallowed.
a2c7855 to
e7f52e6
Compare
|
Status note: this still reproduces. On the current release (1.4.0) a client whose first dial was refused, then given an explicit The same sequence came up again in review of #37993 (noted there as pre-existing and out of that PR's scope), so this is worth landing. The branch now conflicts with main: #37971 removed repro script used (in-process net server, no redis needed)import net from "node:net";
import { RedisClient } from "bun";
const port: number = await new Promise(resolve => {
const l = net.createServer();
l.listen(0, "127.0.0.1", () => {
const p = (l.address() as net.AddressInfo).port;
l.close(() => resolve(p));
});
});
const client = new RedisClient(`redis://127.0.0.1:${port}`, { maxRetries: 5 });
// first dial via a command so no connect() promise is cached; refused -> 50ms retry armed
const cmd = client.get("x").catch(() => {});
await new Promise(r => setTimeout(r, 15));
let accepted = 0;
const sockets: net.Socket[] = [];
const server = net.createServer(sock => {
accepted++;
sockets.push(sock);
sock.on("data", () => setTimeout(() => sock.write("+OK\r\n"), 200));
});
await new Promise<void>(r => server.listen(port, "127.0.0.1", () => r()));
await Promise.all([client.connect(), new Promise(r => setTimeout(r, 400))]);
console.log({ accepted }); // { accepted: 2 } on 1.4.0, expected 1
client.close();
for (const s of sockets) s.destroy();
server.close();
await cmd; |
1 participant
What does this PR do?
Fixes a
Bun.RedisClientconnection-lifecycle bug where a stale auto-reconnect timer starts a second, competing connection and orphans the one already in flight. On debug/assertion builds the orphan's late traffic trips:On release builds the same sequence silently runs socket callbacks against a client whose JS wrapper is only weakly referenced (it is also an fd leak: the orphaned socket is never closed).
Reproduction (scripted in-process RESP3 server; full version is the test):
connect().do_connect()starts a new socket; the timer stays armed.is_reconnectingis still set when the timer fires.on_reconnect_timer()->reconnect()->connect()creates another socket and assigns it toclient.socket, orphaning the in-flight one.on_openresetsis_authenticatedand writes a HELLO, but never setsstatus = Connectingor re-upgrades the wrapper ref, so by the time the reply arrives the client is Disconnected with a weak (or collected) wrapper, andon_valkey_connectasserts.Cause
reconnect()only checkedis_reconnecting. A reconnect timer armed by a connection loss is not cancelled by a later explicitconnect(), andconnect()assignsclient.socketwithout closing the previous one, so the timer firing while a connection is in flight (Status::Connecting, or evenConnected) clobbers a live socket.is_reconnectingis only cleared by a successful HELLO, so a withheld or slow HELLO response is enough to keep the stale timer armed and active.Fix
reconnect()returns unlessstatus == Disconnected.Disconnectedis the only state with no live socket attached (every transition into it detaches or closes the socket first), soconnect()can no longer overwrite a live one.connect()nowdebug_assert!s that the previous socket is closed or detached, so any future path that violates the one-live-socket invariant fails loudly in debug and fuzz builds instead of orphaning a socket.Not included here: making the
SocketHandlercallbacks ignore events from a socket other thanclient.socket(defense in depth for the same class). With the invariant restored at the source that condition is unreachable, and filteringon_close/on_connect_errorinteracts with the per-socket keep-alive refcount, so it deserves its own change if we want it.Related but distinct open PRs in this area: #32768 and #32779 fix synchronously-failing
connect()paths; this one is about a successful competing connect.Verification
test/js/valkey/valkey-gc.test.tsgets a spawned fixture with a scripted in-process RESP3 server. It has two oracles, so it does not depend on debug assertions:[review] gate passed · iteration 4 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 4
evidence per changed file