From 94fcbe1908dc7a1cc9cbf320847f0662441f15a2 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 2 Sep 2026 22:57:27 -0700 Subject: [PATCH] fix(relay): stop reviving a PTY into a directory that is gone from the host (#18351) `reviveEntry` re-resolves and re-bounds every field it takes from serialized state -- shell override, WSL distro, envToDelete, TERM, history isolation, the credential guard -- except `cwd`, which went straight to `node-pty`. A serialized cwd only proves the directory existed when the client wrote it down: a worktree removed while the relay was down makes it a dead path. node-pty does not report that as a spawn error on POSIX. The child `chdir`s after the fork and `_exit(1)`s, so the pane revives already dead with no output and no diagnosis. On Windows `CreateProcess` fails instead, and the throw escapes `reviveEntry` (there is no shell override to degrade), then escapes `revive`'s loop, which has a `finally` but no `catch` -- so one dead directory costs every later entry in the batch its state. Skip that one entry instead, which is the call `reviveEntry` already makes for a shell override that can no longer spawn: substituting a different directory is the defect the serialized value exists to prevent, so dropping one pane is the honest outcome. The check runs inside `reviveEntry`, after `beginPtyCreation`, so the worktree-removal fence still sees the serialized path -- a removal in flight leaves it partly present, and statting it must not be what decides. Skipped entirely for a WSL shell, whose cwd lives in a guest that never stats on this host, matching the `executesOnRelayFilesystem` boundary `requireRelaySpawnCwd` already honours. Fixture paths in the revive tests move to a real directory: `/repo` and `C:\repo` never existed, so under the new check those panes would be skipped before the shell-override and session-cap behaviour under test could run. --- src/relay/pty-handler-revive.test.ts | 73 +++++++++++++++++++++++++--- src/relay/pty-handler.ts | 16 ++++-- 2 files changed, 78 insertions(+), 11 deletions(-) diff --git a/src/relay/pty-handler-revive.test.ts b/src/relay/pty-handler-revive.test.ts index c43a9af576a..e184cd56817 100644 --- a/src/relay/pty-handler-revive.test.ts +++ b/src/relay/pty-handler-revive.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest' import { existsSync, rmSync } from 'node:fs' -import { homedir } from 'node:os' +import { homedir, tmpdir } from 'node:os' import { join } from 'node:path' import { hashWorktreeId } from '../main/terminal-history-id' @@ -47,6 +47,10 @@ import type { MockDispatcher } from './pty-handler-test-harness' const PTY_1 = testPtyId(1) +// Why a real directory: revive drops an entry whose serialized cwd is gone from this host, so a +// fixture path that never existed would be skipped before the behaviour under test runs. +const LIVE_CWD = tmpdir() + describe('PtyHandler', () => { let dispatcher: MockDispatcher let handler: PtyHandler @@ -157,7 +161,7 @@ describe('PtyHandler', () => { pid: process.pid, cols: 80, rows: 24, - cwd: '/repo', + cwd: LIVE_CWD, worktreeId: 'repo-id::/repo' })) ) @@ -181,7 +185,7 @@ describe('PtyHandler', () => { pid: process.pid, cols: 80, rows: 24, - cwd: '/repo', + cwd: LIVE_CWD, worktreeId: 'repo-id::/repo' } ]) @@ -488,6 +492,32 @@ describe('PtyHandler', () => { ).rejects.toThrow(`PTY "${PTY_1}" not found`) }) + // Why: `cwd` is the last serialized field revive took on trust. It proves the directory existed + // when the client wrote it down, not that it exists now -- node-pty answers a removed one by + // _exit(1)-ing the child on POSIX and by throwing on Windows, and the throw escapes the loop. + it('skips a pane whose serialized cwd is gone from this host, keeping the batch', async () => { + const removedCwd = join(tmpdir(), `orca-revive-removed-${process.pid}`) + rmSync(removedCwd, { force: true, recursive: true }) + const state = JSON.stringify([ + { id: 'pty-20', pid: process.pid, cols: 80, rows: 24, cwd: removedCwd }, + { id: 'pty-21', pid: process.pid, cols: 80, rows: 24, cwd: LIVE_CWD } + ]) + const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => true) + try { + await dispatcher.callRequest('pty.revive', { state }) + } finally { + killSpy.mockRestore() + } + + // The later entry still revived, and no other directory stood in for the first. + expect(mockPtySpawn).toHaveBeenCalledTimes(1) + expect((mockPtySpawn.mock.calls[0][2] as { cwd: string }).cwd).toBe(LIVE_CWD) + const live = (await dispatcher.callRequest('pty.serialize', { + ids: ['pty-20', 'pty-21'] + })) as string + expect(JSON.parse(live).map((entry: { id: string }) => entry.id)).toEqual(['pty-21']) + }) + describe('a Windows relay reviving a WSL pane', () => { const worktreeId = 'r::/remote/wsl-worktree' const historyFile = join( @@ -579,7 +609,7 @@ describe('PtyHandler', () => { pid: process.pid, cols: 80, rows: 24, - cwd: 'C:\\repo', + cwd: LIVE_CWD, shellOverride: 'wsl.exe', terminalWindowsWslDistro: 'U'.repeat(257) } @@ -605,10 +635,10 @@ describe('PtyHandler', () => { pid: process.pid, cols: 80, rows: 24, - cwd: 'C:\\repo', + cwd: LIVE_CWD, shellOverride: 'wsl.exe' }, - { id: 'pty-13', pid: process.pid, cols: 80, rows: 24, cwd: 'C:\\repo' } + { id: 'pty-13', pid: process.pid, cols: 80, rows: 24, cwd: LIVE_CWD } ]) mockPtySpawn.mockImplementationOnce(() => { throw new Error('spawn wsl.exe ENOENT') @@ -628,6 +658,33 @@ describe('PtyHandler', () => { expect(JSON.parse(live).map((entry: { id: string }) => entry.id)).toEqual(['pty-13']) }) + // Why: relayHostDirectoryExists stats the relay's own filesystem, and a wsl.exe pane's cwd + // lives in the guest -- the same host boundary requireRelaySpawnCwd already honours. + it('revives a WSL pane whose cwd is a guest path this host cannot stat', async () => { + const state = JSON.stringify([ + { + id: 'pty-14', + pid: process.pid, + cols: 80, + rows: 24, + cwd: '/home/dev/guest-only-worktree', + shellOverride: 'wsl.exe', + terminalWindowsWslDistro: 'Ubuntu' + } + ]) + const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => true) + try { + await dispatcher.callRequest('pty.revive', { state }) + } finally { + killSpy.mockRestore() + } + + expect(mockPtySpawn).toHaveBeenCalledTimes(1) + expect((mockPtySpawn.mock.calls[0][2] as { cwd: string }).cwd).toBe( + '/home/dev/guest-only-worktree' + ) + }) + it('degrades one entry with an unsupported override without failing the batch', async () => { const state = JSON.stringify([ { @@ -635,10 +692,10 @@ describe('PtyHandler', () => { pid: process.pid, cols: 80, rows: 24, - cwd: 'C:\\repo', + cwd: LIVE_CWD, shellOverride: 'nc.exe' }, - { id: 'pty-10', pid: process.pid, cols: 80, rows: 24, cwd: 'C:\\repo' } + { id: 'pty-10', pid: process.pid, cols: 80, rows: 24, cwd: LIVE_CWD } ]) const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => true) try { diff --git a/src/relay/pty-handler.ts b/src/relay/pty-handler.ts index aba05b63be5..1333057eaa7 100644 --- a/src/relay/pty-handler.ts +++ b/src/relay/pty-handler.ts @@ -30,6 +30,7 @@ import { import { splitWorktreeIdForFilesystem } from '../shared/worktree/id' import { formatUnresolvedRelaySpawnCwdMessage, + relayHostDirectoryExists, resolveRelaySpawnCwd, type RelaySpawnCwdResolution } from './pty-spawn-cwd' @@ -2770,6 +2771,18 @@ export class PtyHandler { const shellOverride = typeof entry.shellOverride === 'string' ? entry.shellOverride.trim() : '' const resolvedShellOverride = resolveRevivedShellOverride(shellOverride) const shell = resolvedShellOverride || resolveDefaultShell() + // Mirrors spawn: the entry's override is what gets re-launched, so a WSL + // pane needs the same guest-visible HISTFILE and the same WSLENV carrier. + const wslShell = isRelayWslShell(shell) + // Why cwd is re-checked: it is the one serialized field revive still took on trust, and it only + // proves the directory existed when the client wrote it down. A worktree removed since leaves + // node-pty to _exit(1) the child on POSIX (a pane revived already dead) and to throw on Windows, + // which escapes the loop and costs every later entry its state. Same call as the shell override + // below: drop this one pane rather than substitute a directory it was never pointed at. Skipped + // for a WSL shell, whose cwd lives in a guest that never stats on this host. + if (!wslShell && !relayHostDirectoryExists(entry.cwd)) { + return + } const terminalWindowsWslDistro = typeof entry.terminalWindowsWslDistro === 'string' && entry.terminalWindowsWslDistro.length <= MAX_REVIVED_WSL_DISTRO_LENGTH @@ -2788,9 +2801,6 @@ export class PtyHandler { ) { injectRelayFishHistoryEnv(spawnEnv, entry.worktreeId) } - // Mirrors spawn: the entry's override is what gets re-launched, so a WSL - // pane needs the same guest-visible HISTFILE and the same WSLENV carrier. - const wslShell = isRelayWslShell(shell) if (historyIsolationEnabled && entry.worktreeId) { const historyRoot = injectRelayHistoryEnv(spawnEnv, entry.worktreeId, shell, { wsl: wslShell