valkey: report a TLS context that fails to build from the event loop

Closed
RO
3mo ago
+135-14

Follow-up to #37993, opened against its branch because it reuses the deferred no-socket close added there. Turned up while testing that branch.

Problem

  • new Bun.RedisClient("rediss://...", { tls: { ca: "not a certificate" } }) constructs fine; the attempt fails when connect() cannot build the SSL_CTX. That branch (tls_ctx_failed in JSValkeyClient::connect, src/runtime/valkey_jsc/js_valkey.rs) called on_valkey_close() inline, so the connect() promise was rejected and the user's onclose was called from inside the connect() (or command) call that started the attempt. Every other way an attempt can fail reports from the event loop.
  • An onclose that dials again therefore re-entered the same branch on the same stack: connect() -> onclose -> connect() -> ... until JSC threw Maximum call stack size exceeded, which on a debug build ends in ASSERTION FAILED: Unexpected exception observed / releaseAssertNoException (abort). The same onclose against a refused port just loops through the event loop.
  • An onclose that simply threw was also mishandled on this path: the exception came back out of connect() as a crate::Error, which do_connect takes to mean "dial failed" and answers by rejecting the promise that on_valkey_close() had already rejected (ASSERTION FAILED: Promise is already resolved or rejected, JSC__JSPromise__reject, abort on debug builds).

Fix

  • The branch keeps enable_auto_reconnect = false and the inline fail(), and replaces the inline on_valkey_close() (plus the socket ref it consumed) with close_without_socket_next_tick(), the task reconnect() already uses when a dial fails before a socket exists. The task marks the client disconnected and runs on_close(), which takes the manual-close path fail() set up and reaches on_valkey_close() on a fresh stack: a redialing onclose now loops through the event loop like it does against a refused port, and an onclose that throws is reported through the task's fold like any other callback.
  • fail() stays inline on purpose: commands issued before the task runs are rejected as before, and because failed is set, the callers' reset_connection_timeout() arms nothing for an attempt that is already over. Only the JS-visible report (promise settlement and onclose) moves.
  • The deferred close now holds a Strong on the JS wrapper while queued. A socket that is still going to report its close keeps the wrapper strong through update_poll_ref() (status Connecting); this queued close is invisible to update_poll_ref(), and after fail() nothing else it counts as activity is left, so the update_poll_ref() the callers run on the way out downgraded the wrapper before the task ran. With just the deferral, a client that only its pending attempt still referenced was collected in between and connect() never settled (0 of 100 settled in a Bun.gc(true) loop; 100 of 100 with the Strong). reconnect()'s existing use of the task was not affected (is_reconnecting keeps the wrapper strong there; 30 of 30 settled without the Strong), and duplicate() of a connected client is a case where nothing in JS references the new wrapper until its promise settles.
  • Tests in test/js/valkey/reliability/connection-failures.test.ts ("Recovering After fail()" block, no server needed since nothing is dialed): the report arrives after the starting call returns, for both connect() and a command; an onclose that redials three times never runs inside a connect() call; an onclose that throws is reported as an uncaught exception and the process exits 0; and clients referenced by nothing but their pending attempt still get their rejection and onclose across a Bun.gc(true). The first four fail against the base branch (the first three on their assertions, the throwing one with the abort above); the GC one fails by timing out against the deferral without the Strong.
  • Also run on this branch: the rest of connection-failures.test.ts (the docker-gated block is skipped here) and test/js/valkey/valkey-gc.test.ts, whose first test is this exact failure under ASAN.

Background

  • JSValkeyClient is the native object behind Bun.RedisClient; it is intrusively refcounted, and separately holds this_value, a reference to its JS wrapper that update_poll_ref() flips between strong and weak depending on whether anything is still going to need the wrapper (an open or connecting socket, queued commands, a scheduled reconnect). The connect() promise lives in a slot on the wrapper and onclose is read from it, so a close reported after the wrapper is collected reports to nobody.
  • fail() marks the client failed, rejects everything queued, and closes the socket if there is one; on_close() is what runs when a connection ends and decides between retrying and reporting the close (on_valkey_close() rejects the cached connect() promise and calls onclose). #37993 added ValkeyDeferredClose::WithoutSocket, an event-loop task that runs on_close() for an attempt that failed without ever having a socket, precisely so that onclose does not run inside the call that failed.
  • dispatch::fold is how callbacks dispatched from the event loop report an exception their JS left pending: as an uncaught exception, rather than returning it to a caller that has no idea what to do with it.
Probes (debug build of the base branch)

Synchronous report, bad ca vs a real dial:

{"label":"bad ca","closesRightAfterConnect":1,"outcome":"rejected: ERR_REDIS_CONNECTION_CLOSED","closesAfterRejection":1}
{"label":"tls:true (real dial to port 1)","closesRightAfterConnect":0,"outcome":"rejected: ERR_REDIS_CONNECTION_CLOSED","closesAfterRejection":1}

Unbounded redial from onclose:

ASSERTION FAILED: Unexpected exception observed on thread ...
Error Exception: Maximum call stack size exceeded.
!exception()
.../JavaScriptCore/ExceptionScope.h(62) : void JSC::ExceptionScope::releaseAssertNoException()
panic(main thread): abort() called

onclose that throws:

ASSERTION FAILED: Promise is already resolved or rejected
arg0->status() == JSC::JSPromise::Status::Pending
../../src/jsc/bindings/bindings.cpp(3876) : void JSC__JSPromise__reject(...)
panic(main thread): abort() called

100 clients referenced by nothing but their pending connect(), Bun.gc(true) after each:

deferral only:        unreferenced {"settled":0,"closes":0}    referenced {"settled":100,"closes":100}
deferral + Strong:    unreferenced {"settled":100,"closes":100} referenced {"settled":100,"closes":100}
reconnect() path (#37993 as is), 30 unreferenced clients: {"settled":30,"closes":30}