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

[robobun]

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). valkey: close the socket on every fail() and mark the client disconnected before onclose runs #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}

[robobun]

Status: superseded, closed. The base branch picked up the same change in 218faf0 (tls_ctx_failed now goes through close_without_socket_next_tick(), with status = Connecting keeping the wrapper and the event loop alive until the task runs, which covers the case the Strong in this PR was for), so the fix lives in #37993.

The test cases from here that 218faf0 does not already have pass unchanged on top of it: ali/valkey-fail-recovery...farm/eb4582f3/valkey-tls-ctx-extra-tests (a command starting the attempt, an onclose that throws, and clients referenced only by their pending connect() across a Bun.gc(true); that last one is the only test in the file that fails, by timing out, if the status = Connecting / update_poll_ref() lines of 218faf0 are removed).

Original status

Reproduced on a debug build of the base branch (ali/valkey-fail-recovery at de72c16): with tls: { ca: "not a certificate" }, onclose has already run by the time connect() returns, an onclose that dials again recurses until Maximum call stack size exceeded (abort on debug builds), and an onclose that throws aborts on a double reject of the connect() promise. The four new tests in test/js/valkey/reliability/connection-failures.test.ts fail against the base branch and pass with this diff; the fifth (GC) test guards the Strong the deferred close holds.

[github-actions]

This PR may be a duplicate of:

1. redis: harden connection lifecycle (Handshake enum, promise-settlement fixes, -1251 LOC) #34829 - Rewrites the same tls_ctx_failed branch in JSValkeyClient::connect() to stop the JS error from on_valkey_close() leaking out of connect(), the same double-settle abort this PR fixes.

🤖 Generated with Claude Code

[robobun]

Not a duplicate of #34829. That PR keeps the tls_ctx_failed branch synchronous: it still hands the socket ref to on_valkey_close() and calls it from inside connect(), and only stops the error from a throwing onclose from escaping as a connect() error. So it covers the double-reject side effect noted above, but not the bug this PR is about: onclose running inside the connect() (or command) call that started the attempt, so an onclose that dials again recurses on the same stack. This PR moves the report to the deferred no-socket close from #37993 (which #34829 predates and does not use); the throwing case stops being reachable as a consequence.

[claude]

I reviewed this PR and didn't find any bugs. Because it changes GC rooting (adds a jsc::Strong held across an event-loop task) and rebalances the socket keep-alive ref on the tls_ctx_failed path — both in the memory-safety category the repo treats as most-blocked — a human look would still be worthwhile, especially since it's stacked on #37993.

Checked: the socket ref that on_valkey_close() used to consume is now released by socket_ref's Drop on the early return, and the deferred task's own ref_() is what on_valkey_close/on_valkey_reconnect adopts — balanced on both run and release_unrun.
Checked: DeferredClose losing Copy is fine — match self.what/match task.what have no by-value bindings, so the Strong stays in the boxed task and drops with it after on_close() has read this_value.
Checked: reset_connection_timeout() after the now-Ok(()) return is a no-op (failed → get_timeout_interval()==0 → arm early-returns), and fail() inside the deferred on_close() early-returns on failed so no second rejection.

Extended reasoning...

Overview

The PR moves the TLS-context-build-failure branch in JSValkeyClient::connect() from calling on_valkey_close() inline to enqueuing the existing DeferredClose::WithoutSocket task, so onclose and the connect() promise settle from the event loop like every other failure mode. To keep the JS wrapper alive across the enqueue→run gap (after update_poll_ref() downgrades it on the way out of do_connect/send), the task variant now carries an Option<jsc::Strong> on the wrapper. DeferredClose accordingly loses Clone, Copy. Five new tests cover: async report from both connect() and a command, redialing onclose not recursing, throwing onclose surfacing as uncaughtException, and survival across Bun.gc(true).

Security risks

None. The path is a client-side TLS config validation failure; no untrusted input parsing, auth, or protocol handling is touched.

Level of scrutiny

High. This is native code touching intrusive refcounts and GC rooting — REVIEW.md's most-blocked category. The change is small and reuses #37993's task, but adding a Strong whose lifetime spans an event-loop task boundary (and must release correctly in both run and release_unrun) is exactly the kind of thing a maintainer should sign off on. The repo guidance is explicit that new Strong refs need justification; the PR justifies it well (0/100 vs 100/100 probe), but confirming that holding the Strong in the task rather than teaching update_poll_ref() about the pending close is the preferred shape is a maintainer call.

Other factors

• Refcount balance verified by inspection: the old path forgot socket_ref and let on_valkey_close adopt it; the new path lets socket_ref drop naturally, and the task's own this.ref_() supplies the ref on_valkey_close/on_valkey_reconnect adopts. enqueue_deferred_close's ref is adopted in both run and release_unrun.
• The Strong drops with the boxed task: match self.what { WithoutSocket { .. } => ... } has no by-value binding, so _wrapper remains owned by self/task and is released after the arm body (i.e., after on_close() has read this_value), and in release_unrun after the poll ref is disabled.
• get_timeout_interval() returns 0 when failed, so the callers' reset_connection_timeout() after the now-Ok(()) return arms nothing — matches the PR's claim.
• Stacked on #37993's branch; whoever reviews that will likely want to see this alongside it.
• Tests follow harness conventions (bunExe/bunEnv, await using, combined {stdout,stderr,exitCode} assertion, no network) and each is stated to fail on the base branch.

[robobun]

On the one open point in the review above, the task holding the Strong versus teaching update_poll_ref() about the queued close: a flag would have to be set at enqueue and cleared in both run() (before on_close(), so that the update_poll_ref() inside on_valkey_close() can still downgrade the wrapper) and release_unrun(), which is the kind of bookkeeping #37993 spent a round on for is_reconnecting. The Strong is released by Drop on both exits with nothing to keep in step, and it follows the shape DeferredFailure already uses on this client: the task that is going to touch JS state holds that state itself. Happy to switch if the maintainer reviewing #37993 prefers the flag.

[robobun]

^{Updated 8:05 PM PT - Aug 14th, 2026}

@robobun, your commit 6d26f04 is building: #97044

[robobun]

^{Updated 9:05 PM PT - Aug 14th, 2026}

❌ @robobun, your commit 6d26f04 has some failures in Build #97044 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 38794

That installs a local version of the PR into your bun-38794 executable, so you can run:

bun-38794 --bun