Host service opens two pty-daemon connections when terminals open at the same time #9452

Activity

1. github-actions commented on Nov 7, 2026

   

   github-actionsBot

   on Nov 7, 2026 – with GitHub Actions

   Contributor

   More actions

   The reported race is real. In getDaemonClient(), the in-flight promise is stored only after an await, so two calls made at the same time can each open their own pty-daemon connection.

   Summary

   When two callers ask for the daemon client at the same time and no connection exists yet, each one opens its own DaemonClient. Only the last one to finish is stored, so the other client is never disposed. It also keeps its own disconnect handler, which means onDaemonDisconnect listeners run once per client. Because disposeDaemonClient() only knows about the stored client, the other connection stays open after shutdown or test teardown. The expected behaviour is that every concurrent caller shares one in-flight connection.

   Affected code

   • packages/host-service/src/terminal/daemon-client-singleton.ts:65-102: getDaemonClient()
   • packages/host-service/src/terminal/daemon-client-singleton.ts:105-115: disposeDaemonClient() only sees cached and connecting
   • Callers that can run at the same time: packages/host-service/src/terminal/terminal.ts:959, :3125 (terminal create/attach), :3570, and packages/host-service/src/terminal/reaper/reaper.ts
   • Disconnect listener: packages/host-service/src/terminal/terminal.ts:734

   Root cause

   if (cached?.isConnected) return cached;
   if (connecting) return connecting;          // :67
   const sockPath = await ptyDaemonSocketPath(); // :68  <- yields here
   ...
   connecting = client.connect()...             // :87  <- assigned too late

   connecting is assigned only after await ptyDaemonSocketPath(). In production that call waits on waitForDaemonReady(...), which takes real time. Even with the SUPERSET_PTY_DAEMON_SOCKET test override, the await still gives up control for a microtask. So two callers that start in the same tick both pass the if (connecting) check and both build a client.

   What follows:

   • Both clients register client.onDisconnect (:70). The cached === client guard (:75) protects only cached; the listener loop (:76-85) runs for both clients. If the orphaned connection drops, the terminal.ts:734 handler closes every terminal WS socket and starts sweepAgentBindingsAfterDaemonLoss, even though the live client is still healthy.
   • cached is overwritten by whichever connect() finishes last, and the other client leaks.
   • A smaller, related problem: .finally(() => { connecting = null; }) (:98-100) clears the field without checking that it still holds the same promise. If disposeDaemonClient() runs during a connect and a new connect starts, the old finally can clear the new in-flight promise.

   Reproduction

   As a node:test / bun:test test, using the existing test override so the supervisor is not needed:

   1. Start a stub pty-daemon socket server (or reuse the setup from terminal.adoption.node-test.ts) and set process.env.SUPERSET_PTY_DAEMON_SOCKET to its path.
   2. const [a, b] = await Promise.all([getDaemonClient(), getDaemonClient()]);
   3. Assert a === b. This fails on main today. You can also count accepted connections on the stub server: it sees 2.
   4. Optionally register onDaemonDisconnect(spy), close the server-side sockets, and assert that spy was called once. Today it is called twice.
   5. await disposeDaemonClient(), then assert that the stub server has no open connections. Today one is left open.

   Manual check: start the host service and open two terminals at the same moment before any daemon connection exists. The daemon sees two client connections.

   Fix direction

   Assign the in-flight promise synchronously, before the first await, so the socket-path lookup is inside the shared promise:

   export function getDaemonClient(): Promise<DaemonClient> {
   	if (cached?.isConnected) return Promise.resolve(cached);
   	if (connecting) return connecting;
   	const attempt = connectDaemonClient().finally(() => {
   		if (connecting === attempt) connecting = null;
   	});
   	connecting = attempt;
   	return attempt;
   }

   Here connectDaemonClient() holds the current body: ptyDaemonSocketPath(), new DaemonClient, onDisconnect, connect(), and dispose-on-failure. The connecting === attempt check also fixes the smaller problem above.

   With this change, a failed ptyDaemonSocketPath() still resets connecting through finally. The next caller then retries, as it does today. One regression test, the Promise.all identity and connection-count check above, should be enough.