diff --git a/config/scripts/daemon-replacement-live-agent-pty-preservation-repro.mjs b/config/scripts/daemon-replacement-live-agent-pty-preservation-repro.mjs index 495865be268..aa3ad99b5de 100644 --- a/config/scripts/daemon-replacement-live-agent-pty-preservation-repro.mjs +++ b/config/scripts/daemon-replacement-live-agent-pty-preservation-repro.mjs @@ -453,6 +453,12 @@ function teardown(staged) { staged.daemon.child.unref() } for (const marker of staged.markers ?? []) { + // Why re-verify by tag: phase 1 waits for these pids to die, and teardown runs a minute + // later. Signalling a remembered pid after that would be signalling whatever the OS has + // since recycled it onto — which is the mistake this whole script exists to study. + if (!isMarkerAlive(marker)) { + continue + } for (const pid of [marker.pid, marker.sessionPid]) { try { process.kill(pid, 'SIGKILL') diff --git a/src/main/daemon/daemon-live-pty-evidence.test.ts b/src/main/daemon/daemon-live-pty-evidence.test.ts index 5c3c313b142..47dcdf70433 100644 --- a/src/main/daemon/daemon-live-pty-evidence.test.ts +++ b/src/main/daemon/daemon-live-pty-evidence.test.ts @@ -23,6 +23,9 @@ function row(pid: number, ppid: number, overrides: Partial = {} const daemonRow = row(DAEMON_PID, 1, { command: 'daemon-entry.js' }) +// macOS wraps every terminal in login(1); only the wrapper is the session leader. +const LOGIN_WRAPPER = '/usr/bin/login -flpq nwparker /bin/bash …' + function windowsCandidate(pid: number): WindowsProcessCandidate { return { pid, ppid: DAEMON_PID, name: 'bash.exe', command: 'bash', executablePath: '', depth: 1 } } @@ -49,7 +52,7 @@ describe('inspectDaemonPtyOwnership on POSIX', () => { readPosixProcessTable: posixTable([ daemonRow, row(101, DAEMON_PID, { command: '/usr/bin/login -flpq nwparker' }), - row(202, 101, { command: 'claude' }) + row(202, 101, { stat: 'S+', command: 'claude' }) ]) }) ).resolves.toBe('owns-live-ptys') @@ -87,7 +90,7 @@ describe('inspectDaemonPtyOwnership on POSIX', () => { readPosixProcessTable: posixTable([ daemonRow, row(101, DAEMON_PID, { stat: 'Z', command: '' }), - row(202, 101, { command: 'codex' }) + row(202, 101, { stat: 'Ss', command: 'codex' }) ]) }) ).resolves.toBe('owns-live-ptys') @@ -408,13 +411,32 @@ describe('inspectDaemonPtyOwnership sampling', () => { ).resolves.toBe('owns-live-ptys') }) - it('answers no-live-ptys on the first conclusive read without re-sampling', async () => { + it('confirms emptiness with a second read before letting it authorize a kill', async () => { const readPosixProcessTable = vi.fn(async () => [daemonRow]) await expect( inspectDaemonPtyOwnership(DAEMON_PID, { platform: 'darwin', readPosixProcessTable }) ).resolves.toBe('no-live-ptys') - expect(readPosixProcessTable).toHaveBeenCalledTimes(1) + expect(readPosixProcessTable).toHaveBeenCalledTimes(2) + }) + + it('sees a terminal whose shell had not yet appeared on the first read', async () => { + // A terminal contributes exactly one session leader — on macOS the login wrapper — and it + // is childless for the moment between forkpty creating it and the shell appearing. One + // snapshot cannot tell that from a wrapper whose shell has gone, and guessing wrong here + // kills a live terminal. + const readPosixProcessTable = vi + .fn<() => Promise>() + .mockResolvedValueOnce([daemonRow, row(101, DAEMON_PID, { command: LOGIN_WRAPPER })]) + .mockResolvedValueOnce([ + daemonRow, + row(101, DAEMON_PID, { command: LOGIN_WRAPPER }), + row(202, 101, { stat: 'S+', command: '/opt/homebrew/bin/bash --rcfile …' }) + ]) + + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { platform: 'darwin', readPosixProcessTable }) + ).resolves.toBe('owns-live-ptys') }) it('gives up as unknown rather than guessing when every read stays blind', async () => { diff --git a/src/main/daemon/daemon-live-pty-evidence.ts b/src/main/daemon/daemon-live-pty-evidence.ts index 7ca522606ba..e242d789fce 100644 --- a/src/main/daemon/daemon-live-pty-evidence.ts +++ b/src/main/daemon/daemon-live-pty-evidence.ts @@ -89,7 +89,7 @@ function programBasename(command: string): string { return base.endsWith('.exe') ? base.slice(0, -'.exe'.length) : base } -function isDaemonSelfSpawnedPty(row: ProcessTableRow): boolean { +function isDaemonSelfSpawnedPty(row: Pick): boolean { const command = row.command.trim().toLowerCase() return DAEMON_SELF_SPAWNED_PTY_COMMANDS.some(({ program, args }) => { const suffix = ` ${args}` @@ -176,9 +176,7 @@ async function probeOnce( if (descendants === null) { return 'unknown' } - const hosted = descendants.filter( - (row) => !isDaemonSelfSpawnedPty({ command: row.command } as ProcessTableRow) - ) + const hosted = descendants.filter((row) => !isDaemonSelfSpawnedPty(row)) return hosted.length > 0 ? 'owns-live-ptys' : 'no-live-ptys' } const deadlineMs = deps.posixDeadlineMs ?? POSIX_OWNERSHIP_PROBE_DEADLINE_MS @@ -220,6 +218,7 @@ export async function inspectDaemonPtyOwnership( deps: DaemonPtyOwnershipDeps = {} ): Promise { const platform = deps.platform ?? process.platform + let sawEmpty = false for (let attempt = 0; attempt < PTY_OWNERSHIP_PROBE_ATTEMPTS; attempt++) { let sample: DaemonPtyOwnership try { @@ -227,9 +226,20 @@ export async function inspectDaemonPtyOwnership( } catch { sample = 'unknown' } + // Why a second look before accepting emptiness on POSIX: a terminal contributes exactly + // one session leader — on macOS the login wrapper — and it is invisible for the moment + // between forkpty creating it and the shell appearing beneath it. One snapshot cannot tell + // that from a wrapper whose shell has gone. Emptiness authorizes a kill, so it is the + // answer worth paying a second read for; 'owns-live-ptys' needs no confirmation. + if (sample === 'no-live-ptys' && platform !== 'win32' && !sawEmpty) { + sawEmpty = true + continue + } if (sample !== 'unknown') { return sample } } - return 'unknown' + // Only reached when every sample was blind, or when the confirming read disagreed with an + // emptiness it could not corroborate. + return sawEmpty ? 'no-live-ptys' : 'unknown' } diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index e0c5fad3442..7221d783bb3 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -7444,9 +7444,15 @@ export function registerPtyHandlers( ipcMain.removeAllListeners('pty:signal') ipcMain.on('pty:signal', (_event, args: { id: string; signal: string }) => { - tryGetProviderForPty(args.id) - ?.sendSignal(args.id, args.signal) - .catch(() => {}) + try { + // Why try/catch as well as .catch: routing refuses a session whose host is unreachable, + // and it may do so synchronously. Nothing sits above an ipcMain.on listener to catch it. + tryGetProviderForPty(args.id) + ?.sendSignal(args.id, args.signal) + .catch(() => {}) + } catch { + /* unreachable host — the signal is undeliverable and there is no caller to tell */ + } }) ipcMain.removeAllListeners('pty:clearBuffer')