redis: clear the cached connectionPromise when connect() fails synchronously

Closed
RO
3mo ago
+313-18

What does this PR do?

Fixes a promise double-settle in Bun.RedisClient found by fuzzing.

RedisClient.prototype.connect() caches the pending promise it returns in the connectionPromise cached-property slot so concurrent connect() calls coalesce. When the socket cannot be created inside connect() itself (a redis+unix:// path whose target does not exist, socket(2) failure, a connect(2) errno other than EINPROGRESS), do_connect rejected that promise but never cleared the slot.

on_valkey_connect (resolve) and on_valkey_close (reject) both read the slot and settle whatever is in it. With the already-rejected promise still cached, the next connection attempt to make progress settles it a second time:

ASSERTION FAILED: arg0->status() == JSC::JSPromise::Status::Pending
  void JSC__JSPromise__reject(JSC::JSPromise*, JSGlobalObject*, EncodedJSValue)
  src/jsc/bindings/bindings.cpp:3733

(and the JSC__JSPromise__resolve twin at bindings.cpp:3764 when the later attempt succeeds instead of closing). On debug/ASAN builds this aborts the process; release builds silently operate on a non-pending promise.

The stale slot also has a behavior bug visible on release builds: a later connect() returned the same rejected promise instead of starting a new attempt.

Reproduction (no fault injection, the unix path just doesn't exist yet):

const sock = "/tmp/not-there-yet.sock";
const c = new Bun.RedisClient(`redis+unix://${sock}`);
await c.connect().catch(() => {}); // fails synchronously, rejected promise stays cached
Bun.listen({ unix: sock, socket: { data(s) { s.write("+OK\r\n"); } } });
const p = c.get("k"); // reconnects via send(), which does not consult the slot
c.close();            // on_valkey_close rejects the stale promise again -> abort
await p.catch(() => {});

The original fuzz case reached the same branch with an injected EAGAIN on the first connect(2), which is the only way a numeric-IP target produces a synchronous (rather than deferred) connect failure.

The fix is to clear the cached slot before rejecting, the same thing on_valkey_connect and on_valkey_close already do before settling it.

The other two paths to the same stale slot

Code review found the rest of the class. A synchronous connect() failure can strand the cached connectionPromise from three entry points, and only the first was covered above:

  1. do_connect's needs_to_open_socket branch (the original fix): rejected the promise but left it cached.
  2. do_connect's Status::Disconnected arm calls reconnect(), whose synchronous failure was swallowed into the onclose callback. The freshly-cached promise stayed pending forever, so that .connect() hung and every later .connect() returned the same stale promise.
  3. on_reconnect_timer calls reconnect() too, and the cached promise can still be alive there: after an asynchronous socket failure, on_close()'s auto-reconnect branch schedules the timer without ever reaching on_valkey_close() (which is what normally clears and settles the slot). The timer path discarded the error, so the .connect() promise hung.

All three now go through one helper, reject_connection_promise, which takes whatever is in the cached slot, clears it, and rejects it, the same read-clear-settle shape on_valkey_connect and on_valkey_close already use. reconnect() returns the synchronous connect() error so its two callers can do that.

A GC leak on the timer path

Code review also caught that settling the promise on path 3 is not enough to make the client collectible. on_close()'s auto-reconnect branch sets is_reconnecting, so update_poll_ref upgrades this_value to a strong GC root. Once the timer's synchronous connect() failure ends the retry loop, nothing cleared the flag or re-evaluated the root, so every such client stayed reachable for the life of the process. The timer's Err branch now clears is_reconnecting and runs update_poll_ref() (the same teardown do_connect's reconnect() error branch does), which downgrades this_value and releases the root.

Intentionally left out of this PR: when a timer-driven reconnect fails synchronously, pending commands in the offline and in-flight queues also never settle, because reconnect() routes the error through the onclose callback rather than ValkeyClient::fail. Fixing that requires calling on_valkey_close() from a context that did not take the socket keep-alive ref its trailing deref assumes, which is a refcount change unrelated to the promise slot this PR is about.

How did you verify your code works?

test/js/valkey/valkey-connect-retry.test.ts covers the symptoms against an in-process RESP server (no Redis needed), each in its own subprocess because the double settle aborts the whole process under assert-enabled builds:

  • connect() after a synchronous failure retries instead of returning the stale rejected promise
  • a later successful connection (the on_valkey_connect resolve path)
  • close() during a later attempt (the on_valkey_close reject path, the frame above)
  • connect() whose synchronous failure goes through do_connect's reconnect() call still settles
  • connect() whose socket dies and whose reconnect timer then fails synchronously still settles, and the client is collectible afterwards (8 rounds, then Bun.gc(true) and heapStats().objectTypeCounts.RedisClient < 4)

On a build with none of the fixes all five fail: two die with SIGABRT on the assertion and the reconnect ones hang until the test timeout. With only the first three fixes, the last test fails with 9 RedisClient instances still alive. With the full change all five pass. A 50-iteration run of the fuzzer's original shape (injected connect EAGAIN + torn send via bun:internal-for-testing's socketFaultInjection) also no longer crashes.