From 49acb93e2ec44bc6bec859a5ba2fb2e19961a637 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 24 Aug 2026 23:57:43 -0700 Subject: [PATCH] fix(daemon): let PTY inventory recover from a dead terminal host (#16363) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Worktree removal inventories PTYs through DaemonPtyAdapter.listProcesses. That called ensureConnected bare, so once the terminal-host pipe was dead the removal failed with `connect ENOENT \\?\pipe\orca-terminal-host-...` and stayed broken until the whole app was restarted. spawn already wrapped its work in withDaemonRetry and recovered from exactly this. Inventory did not — so the one path that must not get stuck was the only one that could not heal itself. Both the connect and the listSessions request go inside the retry: a host that dies between them throws the same daemon-gone error, so retrying only the connect would still fail. The reconciliation after the request is deliberately outside it; retrying that would be wrong. Reproduced first, with a real daemon killed mid-test: listProcesses threw DaemonConnectionLostError while a control asserting spawn recovery from the identical kill passed. Both are now regression tests, so the asymmetry cannot come back silently. Co-authored-by: innocarpe --- ...emon-pty-adapter-inventory-respawn.test.ts | 76 +++++++++++++++++++ .../daemon/daemon-pty-session-inventory.ts | 25 ++++-- 2 files changed, 95 insertions(+), 6 deletions(-) create mode 100644 src/main/daemon/daemon-pty-adapter-inventory-respawn.test.ts diff --git a/src/main/daemon/daemon-pty-adapter-inventory-respawn.test.ts b/src/main/daemon/daemon-pty-adapter-inventory-respawn.test.ts new file mode 100644 index 00000000000..f53e2c4c410 --- /dev/null +++ b/src/main/daemon/daemon-pty-adapter-inventory-respawn.test.ts @@ -0,0 +1,76 @@ +/* Inventory after the terminal host dies: worktree removal must not hard-fail. */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { DaemonPtyAdapter } from './daemon-pty-adapter' +import { DaemonServer } from './daemon-server' +import { createMockSubprocess, startDaemonAdapterHarness } from './daemon-pty-adapter-test-harness' + +describe('inventory after the terminal host dies (#10087)', () => { + let harness: Awaited> + let respawnServer: DaemonServer | undefined + + beforeEach(async () => { + harness = await startDaemonAdapterHarness(() => createMockSubprocess()) + }) + afterEach(async () => { + harness.adapter.dispose() + await respawnServer?.shutdown() + await harness.server.shutdown().catch(() => {}) + respawnServer = undefined + }) + + /** An adapter that can bring the host back, as production does. */ + const healingAdapter = (): { adapter: DaemonPtyAdapter; respawn: ReturnType } => { + const respawn = vi.fn(async () => { + respawnServer = new DaemonServer({ + socketPath: harness.socketPath, + tokenPath: harness.tokenPath, + spawnSubprocess: () => createMockSubprocess() + }) + await respawnServer.start() + }) + return { + adapter: new DaemonPtyAdapter({ + socketPath: harness.socketPath, + tokenPath: harness.tokenPath, + respawn + }), + respawn + } + } + + it('lists processes after the host dies instead of failing the removal', async () => { + // The reported failure: worktree remove inventories PTYs through + // listProcesses, and a dead named pipe surfaced as + // `connect ENOENT \\?\pipe\orca-terminal-host-...`, blocking removal until + // the whole app was restarted. spawn already recovers from this; inventory + // did not, so the destructive path was the one that could not heal. + const { adapter, respawn } = healingAdapter() + try { + await adapter.spawn({ cols: 80, rows: 24 }) + await harness.server.shutdown() + + const processes = await adapter.listProcesses() + + expect(Array.isArray(processes)).toBe(true) + expect(respawn).toHaveBeenCalled() + } finally { + adapter.dispose() + } + }) + + it('still recovers on spawn, the path that already worked', async () => { + // Control: proves the harness really kills the host, so the test above is + // not passing because nothing broke. + const { adapter, respawn } = healingAdapter() + try { + await adapter.spawn({ cols: 80, rows: 24 }) + await harness.server.shutdown() + + await adapter.spawn({ cols: 80, rows: 24 }) + + expect(respawn).toHaveBeenCalled() + } finally { + adapter.dispose() + } + }) +}) diff --git a/src/main/daemon/daemon-pty-session-inventory.ts b/src/main/daemon/daemon-pty-session-inventory.ts index 7a02a7e1e3d..b61e35f78e5 100644 --- a/src/main/daemon/daemon-pty-session-inventory.ts +++ b/src/main/daemon/daemon-pty-session-inventory.ts @@ -21,14 +21,27 @@ export abstract class DaemonPtySessionInventory extends DaemonPtyProcessInspecti // be reconciled away below. const preRequestActiveIds = new Set(this.activeSessionIds) try { + // Why retry: this inventory is what destructive teardown consults, and a + // dead host pipe surfaced as `connect ENOENT \\?\\pipe\\orca-terminal-host-...` + // that failed worktree removal until the app was restarted (#10087). Spawn + // already recovered from exactly this; inventory did not, so the one path + // that must not get stuck was the only one that could not heal. + // + // Why the request is inside too: a host that dies between connect and + // listSessions throws the same daemon-gone error, so retrying only the + // connect would still fail. The reconciliation below stays outside -- + // retrying that would double-apply it. + // // Why: connect + listSessions share the caller's one absolute deadline so a // wedged handshake cannot burn the whole teardown budget before the list issues. - await this.ensureConnected(opts?.deadlineMs) - const result = await this.client.request( - 'listSessions', - undefined, - remainingDaemonRequestTimeoutMs(opts?.deadlineMs) - ) + const result = await this.withDaemonRetry(async () => { + await this.ensureConnected(opts?.deadlineMs) + return this.client.request( + 'listSessions', + undefined, + remainingDaemonRequestTimeoutMs(opts?.deadlineMs) + ) + }) const admission = new PtyProcessListAdmission() const processes: PtyProcessInfo[] = [] const aliveSessionIds = new Set()