mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 00:02:31 +00:00
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.
This commit is contained in:
@@ -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 {
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user