From 5fed9f17901c862bed4dc49138871a85378232dc Mon Sep 17 00:00:00 2001 From: brennanb2025 Date: Mon, 6 Jul 2026 22:14:06 -0700 Subject: [PATCH] fix(daemon): hard shutdown barrier so idle exit cannot race a pre-hello socket MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An accepted socket that had not completed hello was invisible to the client map: an idle-exit fired in that window could tear down the server while the late hello still registered a client and spawned a live PTY — which process.exit(0) would then silently kill once server.close() drained. The same untracked socket could also hold server.close() (and the idle-exit callback) open forever. shuttingDown is now set synchronously at the top of shutdown(): new connections and late hellos are destroyed, RPCs are refused, all accepted sockets (tracked or pre-hello) are destroyed, TerminalHost refuses createOrAttach after dispose, and a failed idle shutdown still exits instead of stranding a half-shut daemon. Co-authored-by: Orca --- src/main/daemon/daemon-server.test.ts | 43 ++++++++++++++++++++++++++ src/main/daemon/daemon-server.ts | 44 ++++++++++++++++++++++++++- src/main/daemon/terminal-host.test.ts | 14 +++++++++ src/main/daemon/terminal-host.ts | 7 +++++ 4 files changed, 107 insertions(+), 1 deletion(-) diff --git a/src/main/daemon/daemon-server.test.ts b/src/main/daemon/daemon-server.test.ts index caebc2417b8..70536e64986 100644 --- a/src/main/daemon/daemon-server.test.ts +++ b/src/main/daemon/daemon-server.test.ts @@ -640,6 +640,49 @@ describe('DaemonServer', () => { expect(onIdleExit).not.toHaveBeenCalled() }) + it('destroys a pre-hello socket at idle exit instead of letting it hold the daemon open', async () => { + // Why: sockets that never complete hello are invisible to the client + // map. Without the shutdown barrier they would keep server.close() — + // and therefore onIdleExit — pending forever (zombie daemon), or worse, + // complete hello against a half-shut server. + const { idleExited, onIdleExit } = await startServerWithIdleExit(200) + + const rawSocket = connect(socketPath) + const rawClosed = new Promise((resolve) => rawSocket.once('close', resolve)) + rawSocket.on('error', () => {}) + + await idleExited + await rawClosed + + expect(onIdleExit).toHaveBeenCalledTimes(1) + }) + + it('refuses hello and createOrAttach once shutdown has begun', async () => { + const spawned: unknown[] = [] + server = new DaemonServer({ + socketPath, + tokenPath, + spawnSubprocess: () => { + const subprocess = createMockSubprocess() + spawned.push(subprocess) + return subprocess + } + }) + await server.start() + await server.shutdown() + + const daemon = server as unknown as DaemonServerPrivate + await expect( + daemon.routeRequest('late-client', { + id: 'req-late-1', + type: 'createOrAttach', + payload: { sessionId: 'late-session', cols: 80, rows: 24 } + }) + ).rejects.toThrow(/shutting down/) + expect(spawned).toHaveLength(0) + expect(daemon.clients.size).toBe(0) + }) + it('never fires while a session is alive and exits after the last session ends', async () => { const { idleExited, onIdleExit, lastSubprocess } = await startServerWithIdleExit(75) const daemon = server as unknown as DaemonServerPrivate diff --git a/src/main/daemon/daemon-server.ts b/src/main/daemon/daemon-server.ts index 7801fd013ad..3951ed74043 100644 --- a/src/main/daemon/daemon-server.ts +++ b/src/main/daemon/daemon-server.ts @@ -103,6 +103,12 @@ export class DaemonServer { private lastInputAtBySessionId = new Map() private stopStreamBacklogProbe: () => void = () => {} private idleExit: DaemonIdleExit | null = null + // Why: sockets accepted but not yet past hello are invisible to `clients`, + // so shutdown could otherwise strand them: a late hello would resurrect a + // half-shut server, and an open pre-hello socket would hold server.close() + // (and thus idle exit) open forever. + private openSockets = new Set() + private shuttingDown = false // Why: main-process PTY IPC has the same recent-input bypass, but daemon // output reaches main only after this stream layer. Keeping the window here @@ -139,7 +145,16 @@ export class DaemonServer { isIdle: () => this.clients.size === 0 && this.host.sessionCount() === 0, onExpired: () => { console.warn('[daemon] No sessions and no clients for the idle grace period — exiting') - void this.shutdown().then(() => onIdleExit()) + // Why: idleness was just verified, so exiting after a failed + // shutdown cannot lose sessions — but staying alive would strand a + // half-shut daemon that can never re-arm its idle countdown. + void this.shutdown().then( + () => onIdleExit(), + (err) => { + console.error('[daemon] Idle shutdown failed — exiting anyway:', err) + onIdleExit() + } + ) } }) } @@ -173,6 +188,10 @@ export class DaemonServer { } async shutdown(): Promise { + // Why: hard barrier, set synchronously before any teardown — after this, + // no accepted socket may become a client and no RPC may create a session, + // so shutdown can never race a live PTY into a half-shut server. + this.shuttingDown = true this.stopStreamBacklogProbe() this.transientFactRelay.dispose() // Why: dispose before teardown mutates client/session counts, so the idle @@ -187,6 +206,13 @@ export class DaemonServer { } this.clients.clear() + // Why: pre-hello sockets are not in `clients`; leaving them open would + // hold server.close() — and therefore the idle-exit callback — forever. + for (const socket of this.openSockets) { + socket.destroy() + } + this.openSockets.clear() + return new Promise((resolve) => { if (this.server) { this.server.close(() => { @@ -203,6 +229,12 @@ export class DaemonServer { } private handleConnection(socket: Socket): void { + if (this.shuttingDown) { + socket.destroy() + return + } + this.openSockets.add(socket) + socket.once('close', () => this.openSockets.delete(socket)) // Why: clients can send multibyte prompt/input text split across socket // chunks; keep UTF-8 sequences intact before NDJSON parsing. const decoder = new StringDecoder('utf8') @@ -222,6 +254,13 @@ export class DaemonServer { msg: unknown, _parser: ReturnType ): void { + // Why: a hello queued before shutdown destroyed its socket can still be + // delivered in the same tick; accepting it would register a client on a + // server whose host and listener are already torn down. + if (this.shuttingDown) { + socket.destroy() + return + } const hello = msg as HelloMessage if (hello.type !== 'hello') { this.log.log('client-hello-rejected', { reason: 'expected-hello' }) @@ -359,6 +398,9 @@ export class DaemonServer { } private async routeRequest(clientId: string, request: DaemonRequest): Promise { + if (this.shuttingDown) { + throw new Error('Daemon server is shutting down') + } const client = this.clients.get(clientId) switch (request.type) { diff --git a/src/main/daemon/terminal-host.test.ts b/src/main/daemon/terminal-host.test.ts index 4f92d4c1e1f..0f575f34003 100644 --- a/src/main/daemon/terminal-host.test.ts +++ b/src/main/daemon/terminal-host.test.ts @@ -429,6 +429,20 @@ describe('TerminalHost', () => { expect(lastSubprocess.dispose).toHaveBeenCalled() }) + it('rejects createOrAttach after dispose without spawning a subprocess', async () => { + host.dispose() + + await expect( + host.createOrAttach({ + sessionId: 'post-dispose', + cols: 80, + rows: 24, + streamClient: { onData: vi.fn(), onExit: vi.fn() } + }) + ).rejects.toThrow(/disposed/) + expect(spawnFn).not.toHaveBeenCalled() + }) + it('releases held shell-ready marker prefixes before final checkpoint', async () => { host.dispose() const onFinalCheckpoint = vi.fn() diff --git a/src/main/daemon/terminal-host.ts b/src/main/daemon/terminal-host.ts index ded6d33331c..a64364410c8 100644 --- a/src/main/daemon/terminal-host.ts +++ b/src/main/daemon/terminal-host.ts @@ -81,6 +81,7 @@ export class TerminalHost { private onFinalCheckpoint: TerminalHostOptions['onFinalCheckpoint'] private maxTombstones: number private onSessionCountChange: TerminalHostOptions['onSessionCountChange'] + private disposed = false constructor(opts: TerminalHostOptions) { this.spawnSubprocess = opts.spawnSubprocess @@ -103,6 +104,11 @@ export class TerminalHost { * already deliver them through shell launch arguments. */ async createOrAttach(opts: CreateOrAttachOptions): Promise { + // Why: a request that slips in while the daemon is shutting down must not + // spawn a PTY the dying process would silently take down with it. + if (this.disposed) { + throw new Error('TerminalHost is disposed') + } const existing = this.sessions.get(opts.sessionId) // Why: a session that has been asked to terminate (kill() called but the @@ -366,6 +372,7 @@ export class TerminalHost { } dispose(): void { + this.disposed = true // Why: write final checkpoints before killing sessions so graceful shutdown // has zero data loss. The checkpoint callback writes synchronously to disk. if (this.onFinalCheckpoint) {