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

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<jsc::Strong> },
 }
 
 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.

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<Error & { code: string }>();
+      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<string>[] = [];
+    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<void>();
+      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.