valkey: report a TLS context that fails to build from the event loop
Closed
RO
3mo ago+135+135+012345678901234567890123456789-14-14-01234567890123456789
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 whenconnect()cannot build theSSL_CTX. That branch (tls_ctx_failedinJSValkeyClient::connect, src/runtime/valkey_jsc/js_valkey.rs) calledon_valkey_close()inline, so theconnect()promise was rejected and the user'sonclosewas called from inside theconnect()(or command) call that started the attempt. Every other way an attempt can fail reports from the event loop.- An
onclosethat dials again therefore re-entered the same branch on the same stack:connect()->onclose->connect()-> ... until JSC threwMaximum call stack size exceeded, which on a debug build ends inASSERTION FAILED: Unexpected exception observed/releaseAssertNoException(abort). The sameoncloseagainst a refused port just loops through the event loop. - An
onclosethat simply threw was also mishandled on this path: the exception came back out ofconnect()as acrate::Error, whichdo_connecttakes to mean "dial failed" and answers by rejecting the promise thaton_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 = falseand the inlinefail(), and replaces the inlineon_valkey_close()(plus the socket ref it consumed) withclose_without_socket_next_tick(), the taskreconnect()already uses when a dial fails before a socket exists. The task marks the client disconnected and runson_close(), which takes the manual-close pathfail()set up and reacheson_valkey_close()on a fresh stack: a redialingonclosenow loops through the event loop like it does against a refused port, and anonclosethat 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 becausefailedis set, the callers'reset_connection_timeout()arms nothing for an attempt that is already over. Only the JS-visible report (promise settlement andonclose) moves.- The deferred close now holds a
Strongon the JS wrapper while queued. A socket that is still going to report its close keeps the wrapper strong throughupdate_poll_ref()(statusConnecting); this queued close is invisible toupdate_poll_ref(), and afterfail()nothing else it counts as activity is left, so theupdate_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 andconnect()never settled (0 of 100 settled in aBun.gc(true)loop; 100 of 100 with theStrong).reconnect()'s existing use of the task was not affected (is_reconnectingkeeps the wrapper strong there; 30 of 30 settled without theStrong), andduplicate()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; anonclosethat redials three times never runs inside aconnect()call; anonclosethat 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 andoncloseacross aBun.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 theStrong. - 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
JSValkeyClientis the native object behindBun.RedisClient; it is intrusively refcounted, and separately holdsthis_value, a reference to its JS wrapper thatupdate_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). Theconnect()promise lives in a slot on the wrapper andoncloseis 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 cachedconnect()promise and callsonclose). #37993 addedValkeyDeferredClose::WithoutSocket, an event-loop task that runson_close()for an attempt that failed without ever having a socket, precisely so thatonclosedoes not run inside the call that failed.dispatch::foldis 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}