redis: fix abort and hangs after a synchronously failing reconnect

Closed
RO
3mo ago
+217-36

What does this PR do?

Fixes a process abort (and the hangs behind it) in Bun.RedisClient reachable through the public API with no fault injection.

panic: internal error: entered unreachable code
  ValkeyClient::enqueue   src/runtime/valkey_jsc/valkey.rs   (`_ => unreachable!()`)
  ValkeyClient::send      (offline-queue arm)

Reproduction (against a minimal in-process RESP server that answers +OK; the unix socket only makes the reconnect's connect(2) fail synchronously with ENOENT, socket(2) returning EMFILE under fd pressure reaches the same branch for TCP):

const sock = "/tmp/vk.sock";
let srv = Bun.listen({ unix: sock, socket: { data(s) { s.write("+OK\r\n"); } } });

const c = new Bun.RedisClient(`redis+unix://${sock}`);
await c.connect();
await c.set("k", "v");   // authenticated
c.close();
srv.stop(true);          // unlinks the socket

c.connect().catch(() => {});  // reconnect's connect(2) fails synchronously
await c.send("INFO", []);     // aborts the whole process

Fixes #18895

Cause

Three holes in the connection state machine stack up:

  1. ValkeyClient::on_close() only cleared flags.is_authenticated on its auto-reconnect branch. After a manual close() (or a server-side close with autoReconnect: false), a disconnected client still reported connection_ready() == true.
  2. do_connect() clears the sticky failed flag so an explicit connect() can recover (#29925). If that reconnect's connect() then fails synchronously, reconnect()'s error branch only fired the JS onclose callback: it never marked the client failed, never settled the cached connection promise, and ignored autoReconnect / maxRetries.
  3. enqueue() assumed connection_ready() implies Connecting | Connected and put _ => unreachable!() in the status match. With (1) and (2), the client reaches Status::Disconnected with connection_ready() true and failed false, and the next command that cannot be auto-pipelined (send("INFO"), or any command with enableAutoPipelining: false) lands on the unreachable!().

Writing the fix exposed two more in the same class (a terminal failure must settle the connection promise and release the event loop reference):

  1. After fail() the stale is_reconnecting flag kept update_poll_ref() holding the event loop open forever, so a process whose client exhausted maxRetries never exited.
  2. When connectionTimeout fires between reconnect attempts, the socket is already detached, so fail()'s close() is a no-op and no close callback ever reaches on_valkey_close(). The connect() promise stays pending and the event loop stays referenced. This is #18895: on current main, new Bun.RedisClient("redis://localhost:<closed port>", { connectionTimeout: 30 }).connect() rejects and then hangs the process forever, and an unresolvable host leaves the connect promise unsettled.

Fix

  • on_close() clears is_authenticated and is_selecting_db_internal unconditionally. A closed socket is never an authenticated session.
  • reconnect() routes a synchronous connect() failure through ValkeyClient::on_close(), the same state machine the asynchronous on_connect_error path already uses. That settles the pending connection promise (via on_valkey_close), rejects queued commands, and honors autoReconnect / maxRetries with backoff. connect() releases its own keep-alive ref on the failure path, and on_close() releases one, so the branch takes one ref to balance. JSValkeyClient::fail_with_js_value loses its only caller and is deleted.
  • enqueue() gates the direct-write fast path on status == Connected and drops the _ => unreachable!() arm entirely. A wire client must not be able to abort the process from state reachable through its public API. (Note: (1) alone already makes the panic unreachable; this removes the panic as a mechanism and also closes the pre-existing Status::Connecting arm, which would have written command bytes before the HELLO handshake if reached with a stale is_authenticated.)
  • ValkeyClient::fail_with_js_value() clears is_reconnecting when the client enters the terminal failed state.
  • on_connection_timeout() runs on_valkey_close() itself when the socket is already detached, so the connection promise settles and the poll ref is re-evaluated even though no socket close callback will arrive.

With these, every scenario in #18895 now settles the connect() promise and lets the process exit (details in the comment below). The total time to rejection with default options is still governed by autoReconnect / maxRetries, and connectionTimeout is still re-armed on every reconnect attempt (a per-attempt bound, not a total one); that is pre-existing and unchanged here.

Overlap with #32768

#32768 (a different fuzz finding: connectionPromise double-settle and a GC leak) also edits reconnect()'s synchronous-failure branch, so the two will conflict textually. They fix different bugs: #32768 does not touch on_close() or enqueue(), and this PR does not touch the stale promise slot in do_connect()'s needs_to_open_socket branch (#32768's item 1). This PR's reconnect() change is the on_close() routing that #32768's description explicitly deferred ("Fixing that requires calling on_valkey_close() from a context that did not take the socket keep-alive ref"); the balancing ref_() here is that refcount change. I left a comment on #32768 with the details so whichever lands second can rebase.

Verification

test/js/valkey/valkey-reconnect-failure.test.ts (in-process mock RESP server, no real Redis needed; each case in its own subprocess because the bug aborts the whole process):

  • a non-pipelined command after the failed reconnect rejects with ERR_REDIS_CONNECTION_CLOSED instead of aborting (autoReconnect: false)
  • same with enableAutoPipelining: false
  • with autoReconnect on, the failed reconnect retries with backoff and the offline queue drains once it succeeds
  • the process exits after auto-reconnect exhausts maxRetries (previously hung forever)
  • connectionTimeout firing between reconnect attempts rejects the connect() promise and the process exits (previously the promise stayed pending and the process hung)

On bun-v1.4.0-canary (942c22237) all five fail: the first three abort with the panic above, the last two hang. With the fix all five pass under the ASAN debug build. The existing mock-server valkey suites (valkey-gc, valkey-incremental-scan, valkey-tls-verify, reliability/resp-nesting-depth) still pass, and both scenarios from reliability/recovery.test.ts (#29925) were re-verified against a local redis-server.

Reported by bun-sys-fuzz.