From 6d26f04f6abdd154105e10956291a40d24abbeaf Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 15 Aug 2026 01:22:43 +0000 Subject: [PATCH] valkey: report a TLS context that fails to build from the event loop connect()'s tls_ctx_failed branch ran on_valkey_close() inline, so the connect() promise was rejected and onclose was called from inside the connect() (or command) call that started the attempt. An onclose that dials again therefore re-entered the same branch on the same stack until the stack ran out, and an onclose that threw had its exception turned into a connect() error that rejected the already rejected promise. The branch now uses the deferred no-socket close that reconnect() already uses for a dial that fails outright; fail() still runs inline so commands issued meanwhile are rejected as before and no connection timer is armed. The deferred close holds a strong ref to the JS wrapper while it is queued: unlike a socket close (status Connecting), nothing in update_poll_ref() counts this pending close as activity, so a client that only the pending attempt still referenced was collected before the task ran and the attempt never settled. --- src/runtime/valkey_jsc/js_valkey.rs | 36 +++--- .../reliability/connection-failures.test.ts | 113 ++++++++++++++++++ 2 files changed, 135 insertions(+), 14 deletions(-) diff --git a/src/runtime/valkey_jsc/js_valkey.rs b/src/runtime/valkey_jsc/js_valkey.rs index b8d9ecaeefd0..5ec5f1cb5cd7 100644 --- a/src/runtime/valkey_jsc/js_valkey.rs +++ b/src/runtime/valkey_jsc/js_valkey.rs @@ -1148,7 +1148,16 @@ impl JSValkeyClient { /// because `onclose` may call connect(), and a dial that fails the same way /// from in there would otherwise re-enter `on_close()` on the same stack. fn close_without_socket_next_tick(&self) { - self.enqueue_deferred_close(DeferredClose::WithoutSocket); + // A pending socket close keeps the wrapper strong through + // `update_poll_ref()` (status Connecting); this pending close is + // invisible to it, and after `fail()` nothing else it counts as + // activity is left, so the task has to hold the wrapper itself. + let wrapper = self + .this_value + .get() + .try_get() + .map(|this| jsc::Strong::create(this, &self.global_object)); + self.enqueue_deferred_close(DeferredClose::WithoutSocket { _wrapper: wrapper }); } fn enqueue_deferred_close(&self, what: DeferredClose) { @@ -1471,8 +1480,6 @@ impl JSValkeyClient { let _guard = self.ref_scope(); // Socket keep-alive ref, released by on_valkey_close/on_valkey_reconnect. - // Taken before the TLS-context check so the `tls_ctx_failed` branch's - // `on_valkey_close()` has a ref to consume instead of over-releasing. // Forgotten on success (the socket adopts it). let socket_ref = self.ref_scope(); @@ -1512,11 +1519,10 @@ impl JSValkeyClient { b"Failed to create TLS context", protocol::RedisError::ConnectionClosed, )?; - // `on_valkey_close()` consumes the socket ref; hand it over so it - // isn't released twice. - socket_ref.forget(); - self.client_mut().on_valkey_close()?; - self.client_mut().status = valkey::Status::Disconnected; + // Settles connect() and runs `onclose` from the event loop, as for + // a dial that fails asynchronously; `fail()` already closed the + // client, so the deferred close takes the manual-close path. + self.close_without_socket_next_tick(); return Ok(()); } let ssl_ctx: Option<*mut uws::SslCtx> = match &self.client.get().tls { @@ -2017,13 +2023,14 @@ impl Options { } } -#[derive(Clone, Copy)] enum DeferredClose { /// Close the socket the finalized wrapper left behind. Socket, /// Run the close path for a dial that never produced a socket - /// (`close_without_socket_next_tick`). - WithoutSocket, + /// (`close_without_socket_next_tick`). `_wrapper` is the JS object that + /// path settles connect() and calls `onclose` on, held until the task has + /// run (or is released unrun); `None` only if it was already finalized. + WithoutSocket { _wrapper: Option }, } pub(crate) struct ValkeyDeferredClose { @@ -2043,7 +2050,7 @@ impl ValkeyDeferredClose { DeferredClose::Socket => { crate::dispatch::fold(this.client_mut().close(uws::CloseCode::FastShutdown)) } - DeferredClose::WithoutSocket => { + DeferredClose::WithoutSocket { .. } => { // `on_close()` ends in `on_valkey_close`/`on_valkey_reconnect`, // which release the ref the socket would have held. this.ref_(); @@ -2065,8 +2072,9 @@ impl bun_event_loop::Taskable for ValkeyDeferredClose { // Script-free bookkeeping; do it. DeferredClose::Socket => task.run(), // The VM is going away: `on_close()` would run `onclose`, so only - // give back what `reconnect()` and the enqueue took. - DeferredClose::WithoutSocket => { + // give back the poll ref and the ref the enqueue took; the wrapper + // is released with `task`. + DeferredClose::WithoutSocket { .. } => { // SAFETY: as in `run`. let _enqueue_ref = unsafe { ScopedRef::adopt(task.ctx) }; // SAFETY: live per the ref above. diff --git a/test/js/valkey/reliability/connection-failures.test.ts b/test/js/valkey/reliability/connection-failures.test.ts index 18ddde138842..49d16f4e997f 100644 --- a/test/js/valkey/reliability/connection-failures.test.ts +++ b/test/js/valkey/reliability/connection-failures.test.ts @@ -628,6 +628,119 @@ describe("Valkey: Recovering After fail()", () => { }, ); + // A tls config whose certificates do not parse fails the attempt before + // anything is dialed, so no server is involved in these. + const badTls = { tls: { ca: "not a certificate" }, autoReconnect: false }; + + test.each([ + ["connect()", (client: RedisClient) => client.connect()], + ["a command", (client: RedisClient) => client.ping()], + ])("a tls config that fails to load is reported from the event loop when %s starts the attempt", async (_, start) => { + const client = new RedisClient("rediss://127.0.0.1:1", badTls); + try { + let closes = 0; + const closed = Promise.withResolvers(); + client.onclose = err => { + closes++; + closed.resolve(err); + }; + const attempt = start(client); + // Not from inside the call that started the attempt: an onclose that + // dials again from there would recurse straight back into the failure. + expect(closes).toBe(0); + await expect(attempt).rejects.toMatchObject({ code: "ERR_REDIS_CONNECTION_CLOSED" }); + expect(await closed.promise).toMatchObject({ code: "ERR_REDIS_CONNECTION_CLOSED" }); + expect({ closes, connected: client.connected }).toEqual({ closes: 1, connected: false }); + await expect(client.ping()).rejects.toMatchObject({ code: "ERR_REDIS_CONNECTION_CLOSED" }); + } finally { + client.close(); + } + }); + + test("a tls config failure is still reported for a client that nothing but the pending attempt references", async () => { + // Once connect() has returned, the report that is on its way is all that + // still needs the client. A client collected before it arrives would never + // settle its attempt (this test would then time out). Several clients + // because the most recent one tends to survive a collection anyway. + let closes = 0; + const attempts: Promise[] = []; + for (let i = 0; i < 3; i++) { + const client = new RedisClient("rediss://127.0.0.1:1", badTls); + client.onclose = () => closes++; + attempts.push( + client.connect().then( + () => "connected", + (err: Error & { code: string }) => `rejected: ${err.code}`, + ), + ); + Bun.gc(true); + } + const outcomes = await Promise.all(attempts); + expect({ outcomes, closes }).toEqual({ + outcomes: Array(3).fill("rejected: ERR_REDIS_CONNECTION_CLOSED"), + closes: 3, + }); + }); + + test("an onclose that dials again after a tls config failure goes through the event loop instead of recursing", async () => { + const client = new RedisClient("rediss://127.0.0.1:1", badTls); + try { + const ATTEMPTS = 3; + const settled = Promise.withResolvers(); + let closes = 0; + let closesInsideConnect = 0; + let insideConnect = false; + const dial = () => { + insideConnect = true; + try { + return client.connect(); + } finally { + insideConnect = false; + } + }; + client.onclose = () => { + closes++; + if (insideConnect) closesInsideConnect++; + if (closes < ATTEMPTS) { + dial().catch(() => {}); + } else { + settled.resolve(); + } + }; + await expect(dial()).rejects.toMatchObject({ code: "ERR_REDIS_CONNECTION_CLOSED" }); + await settled.promise; + expect({ closes, closesInsideConnect }).toEqual({ closes: ATTEMPTS, closesInsideConnect: 0 }); + } finally { + client.close(); + } + }); + + test("an onclose that throws after a tls config failure is reported as an uncaught exception", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + process.on("uncaughtException", err => console.log("uncaught", err.message)); + const client = new Bun.RedisClient("rediss://127.0.0.1:1", ${JSON.stringify(badTls)}); + client.onclose = () => { throw new Error("from onclose"); }; + const attempt = client.connect(); + console.log("connect() returned"); + await attempt.catch(err => console.log("connect rejected", err.code)); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout, stderr, exitCode }).toEqual({ + stdout: "connect() returned\nuncaught from onclose\nconnect rejected ERR_REDIS_CONNECTION_CLOSED\n", + stderr: "", + exitCode: 0, + }); + }); + test("a connect() issued from onclose is not fed the replies left over from the failed connection", async () => { // With a database in the URL, HELLO and SELECT are written together, so a // server that rejects HELLO delivers both error replies in one read.