From 89b0fcab51e6a8c06208ae7eb64f43c8a1dfe259 Mon Sep 17 00:00:00 2001
From: Jean-Philippe Sirois <jp@sirois.xyz>
Date: Thu, 16 Jul 2026 15:54:54 -0400
Subject: [PATCH] fix(remote): stop CI teardown reporting itself as a broken
 connection

At the end of a CI run, disposing the RPC stub makes capnweb fire
onRpcBroken with "RPC session was shut down by disposing the main stub".
Because dispose() never set the broken flag, that event flowed through
triggerBroken into runInCI's onBroken and logged "API connection broken
during CI run" right next to the verdict, reading as if the run was cut
off. The run had actually completed normally (Site #3580).

Set broken = true at the top of dispose() so an intentional teardown
makes triggerBroken a no-op. connectWithReconnect is unaffected: its
onBroken calls dispose() after the fact, and triggerBroken already
guards on broken.

Covered by an integration test that drives ApiClient.connect over an
in-process WebSocket relay: an intentional dispose stays silent, while a
real transport drop still reports the broken connection.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---
 src/remote/api-client.test.ts | 84 +++++++++++++++++++++++++++++++++++
 src/remote/api-client.ts      |  5 +++
 2 files changed, 89 insertions(+)
 create mode 100644 src/remote/api-client.test.ts

diff --git a/src/remote/api-client.test.ts b/src/remote/api-client.test.ts
new file mode 100644
index 0000000..b9c51a2
--- /dev/null
+++ b/src/remote/api-client.test.ts
@@ -0,0 +1,84 @@
+import { afterEach, describe, expect, it, vi } from "vitest";
+import { newWebSocketRpcSession, RpcTarget } from "capnweb";
+import { WebSocketServer, type WebSocket as ServerSocket } from "ws";
+import type { Remote } from "./remote.ts";
+import { ApiClient } from "./api-client.ts";
+
+// Relay stand-in: answers `authenticate` with a ServerApi-shaped stub so
+// ApiClient.connect runs its real handshake and dispose path over a genuine
+// WebSocket. That transport is the only place capnweb fires onRpcBroken with
+// "RPC session was shut down by disposing the main stub" — an in-memory session
+// doesn't reproduce it, so the bug would slip through a lighter fake.
+class FakeServerApi extends RpcTarget {
+  async ping() {
+    return true;
+  }
+}
+
+class FakeRelay extends RpcTarget {
+  async authenticate() {
+    return new FakeServerApi();
+  }
+}
+
+// In CI mode connect() never calls back into `remote` (that wiring is the
+// persistent-server path), so an empty stand-in is enough.
+const CI_MODE = { kind: "ci", branch: "main", sha: "" } as const;
+const NO_REMOTE = {} as unknown as Remote;
+
+async function startRelay() {
+  const wss = new WebSocketServer({ port: 0 });
+  await new Promise<void>((resolve) => wss.once("listening", () => resolve()));
+  const sockets: ServerSocket[] = [];
+  const closes: Promise<void>[] = [];
+  wss.on("connection", (socket) => {
+    sockets.push(socket);
+    closes.push(new Promise((resolve) => socket.once("close", () => resolve())));
+    newWebSocketRpcSession(socket as unknown as WebSocket, new FakeRelay());
+  });
+  const { port } = wss.address() as { port: number };
+  return {
+    endpoint: `http://127.0.0.1:${port}`,
+    dropConnection: () => sockets.forEach((s) => s.terminate()),
+    // The client's onRpcBroken can only fire once its transport is gone.
+    // Awaiting the server-observed close and then flushing the timer queue
+    // guarantees that window has fully passed, so "callback not called" means it
+    // genuinely never fired rather than that we asserted too early.
+    afterTransportClosed: async () => {
+      await Promise.all(closes);
+      await new Promise((resolve) => setTimeout(resolve, 50));
+    },
+    close: () => new Promise<void>((resolve) => wss.close(() => resolve())),
+  };
+}
+
+describe("ApiClient.connect (integration)", () => {
+  let relay: Awaited<ReturnType<typeof startRelay>> | undefined;
+
+  afterEach(async () => {
+    await relay?.close();
+    relay = undefined;
+  });
+
+  it("stays silent when the caller disposes the connection", async () => {
+    relay = await startRelay();
+    const onBroken = vi.fn();
+    const { dispose } = await ApiClient.connect(relay.endpoint, "token", CI_MODE, NO_REMOTE, onBroken);
+
+    dispose();
+    await relay.afterTransportClosed();
+
+    expect(onBroken).not.toHaveBeenCalled();
+  });
+
+  it("reports a broken connection when the transport drops mid-run", async () => {
+    relay = await startRelay();
+    const onBroken = vi.fn();
+    await ApiClient.connect(relay.endpoint, "token", CI_MODE, NO_REMOTE, onBroken);
+
+    relay.dropConnection();
+    await relay.afterTransportClosed();
+
+    expect(onBroken).toHaveBeenCalledWith(expect.any(Error));
+  });
+});
diff --git a/src/remote/api-client.ts b/src/remote/api-client.ts
index a580acb..31112e5 100644
--- a/src/remote/api-client.ts
+++ b/src/remote/api-client.ts
@@ -108,6 +108,11 @@ export class ApiClient extends RpcTarget implements ClientApi {
     const dispose = () => {
       if (disposed) return;
       disposed = true;
+      // Mark the connection broken before disposing the stub. Disposing makes
+      // capnweb fire onRpcBroken with "RPC session was shut down by disposing
+      // the main stub"; without this guard triggerBroken would report our own
+      // intentional teardown as a broken connection.
+      broken = true;
       stopPing();
       try { api[Symbol.dispose](); } catch { /* already gone */ }
       try { (unauthenticated as unknown as Disposable)[Symbol.dispose]?.(); } catch { /* already gone */ }