From 8cb45318d76db4899e0790f255e56c2c1d37d7f1 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sat, 25 Jul 2026 22:28:07 -0700 Subject: [PATCH] fix(windows): fail closed on unknown PTY identity (#10674) * fix(windows): fail closed on unknown PTY identity * Gate Windows tree-kill on PTY identity verification Only taskkill when the identity probe returns `own`; skip tree-kill for `unknown`/`foreign`/`absent` to avoid terminating unrelated processes. Clarifies the fail-closed behavior across agent and plain shell teardown paths, matching the POSIX descendant-snapshot discipline. --- src/main/daemon/terminal-session-teardown.ts | 7 ++++--- src/main/providers/local-pty-provider.ts | 9 +++++---- src/main/pty-descendant-termination.test.ts | 8 ++++---- src/main/pty-descendant-termination.ts | 10 +++++----- src/main/windows-pty-root-identity.test.ts | 14 +++++++++++--- src/main/windows-pty-root-identity.ts | 17 +++++++++-------- 6 files changed, 38 insertions(+), 27 deletions(-) diff --git a/src/main/daemon/terminal-session-teardown.ts b/src/main/daemon/terminal-session-teardown.ts index d9f9d996a..1ba583937 100644 --- a/src/main/daemon/terminal-session-teardown.ts +++ b/src/main/daemon/terminal-session-teardown.ts @@ -49,9 +49,10 @@ export class TerminalSessionTeardown { * reap orphaned children (node-pty `useConptyDll` skips the console-process reap), so a * live `pnpm i`/`node` survives shell exit, keeps the ConPTY console non-empty, and holds * the worktree cwd — failing destructive worktree removal with "Failed to physically stop - * every PTY". taskkill /T /F the tree first so physical exit becomes verifiable. Mirrors - * the agent path (#10004/#10100). POSIX shells already reach their child pgroup on - * forceKill, so they stay on the plain force-kill path. + * every PTY". Tree-kill only when the OS identity probe returns `own`; `unknown`/`foreign`/ + * `absent` skip taskkill and rely on root close alone. Mirrors the agent path + * (#10004/#10100). POSIX shells already reach their child pgroup on forceKill, so they + * stay on the plain force-kill path. */ private async forceKillPlainShellSession(sessionId: string, session: Session): Promise { if (process.platform === 'win32') { diff --git a/src/main/providers/local-pty-provider.ts b/src/main/providers/local-pty-provider.ts index 3b864408c..e31110525 100644 --- a/src/main/providers/local-pty-provider.ts +++ b/src/main/providers/local-pty-provider.ts @@ -1123,16 +1123,17 @@ export class LocalPtyProvider implements IPtyProvider { this.requestTrackedPtyShutdown(id, proc, operation.immediate) } if (ptyAgentSessionIds.has(id)) { - // Why: POSIX needs a pre-kill descendant snapshot; Windows uses taskkill /T so - // agent/MCP orphans cannot hold the worktree cwd after shell stop (#10004). + // Why: POSIX needs a pre-kill descendant snapshot; Windows tree-kills only when the + // identity probe returns `own` so agent/MCP orphans cannot hold the worktree cwd + // (#10004). `unknown`/`foreign`/`absent` skip taskkill and rely on root close alone. await killWithDescendantSweep(proc.pid, signalRoot, { ownsRoot: () => ptyProcesses.get(id) === proc }) } else if (process.platform === 'win32' && operation.immediate) { // Why: a plain shell's ConPTY teardown doesn't reap orphaned children (useConptyDll // skips the console reap), so a live `pnpm i`/`node` keeps the ConPTY console alive and - // holds the worktree cwd, failing destructive removal. taskkill /T /F clears the tree so - // physical stop is verifiable. POSIX shells reach their child pgroup on forceKill (#10004). + // holds the worktree cwd. Tree kill runs only when the OS identity probe returns `own`; + // otherwise root close alone, and detached children may block physical stop (#10004). await killWithDescendantSweep(proc.pid, signalRoot, { ownsRoot: () => ptyProcesses.get(id) === proc }) diff --git a/src/main/pty-descendant-termination.test.ts b/src/main/pty-descendant-termination.test.ts index e17e18628..c6941e1cb 100644 --- a/src/main/pty-descendant-termination.test.ts +++ b/src/main/pty-descendant-termination.test.ts @@ -467,7 +467,7 @@ describe('killWithDescendantSweep', () => { expect(killRoot).toHaveBeenCalledOnce() }) - it('on Windows still taskkills when identity is unknown, keeping orphan cleanup', async () => { + it('on Windows skips taskkill when identity is unknown', async () => { const killWindowsTree = vi.fn(async () => {}) const killRoot = vi.fn() await killWithDescendantSweep(4242, killRoot, { @@ -475,11 +475,11 @@ describe('killWithDescendantSweep', () => { killWindowsTree, verifyTreeKillTarget: async () => 'unknown' }) - expect(killWindowsTree).toHaveBeenCalledWith(4242) + expect(killWindowsTree).not.toHaveBeenCalled() expect(killRoot).toHaveBeenCalledOnce() }) - it('on Windows still taskkills when the identity probe throws', async () => { + it('on Windows skips taskkill when the identity probe throws', async () => { const killWindowsTree = vi.fn(async () => {}) const killRoot = vi.fn() await killWithDescendantSweep(4242, killRoot, { @@ -489,7 +489,7 @@ describe('killWithDescendantSweep', () => { throw new Error('probe exploded') } }) - expect(killWindowsTree).toHaveBeenCalledWith(4242) + expect(killWindowsTree).not.toHaveBeenCalled() expect(killRoot).toHaveBeenCalledOnce() }) diff --git a/src/main/pty-descendant-termination.ts b/src/main/pty-descendant-termination.ts index db1f7638a..34ad425b8 100644 --- a/src/main/pty-descendant-termination.ts +++ b/src/main/pty-descendant-termination.ts @@ -204,7 +204,7 @@ type SnapshotDeps = { * signalled: once the root dies, surviving descendants reparent to pid 1 and * can no longer be found by a ppid walk. Resolves null (never rejects) on * Windows, ps failure, or timeout — callers then degrade to shell-only kill - * on POSIX, or Windows `taskkill /T` via killWithDescendantSweep. + * on POSIX, or identity-gated Windows `taskkill /T` via killWithDescendantSweep. */ export async function captureDescendantSnapshot( rootPid: number, @@ -237,9 +237,9 @@ type KillSweepDeps = SnapshotDeps & /** * Standard agent-session kill sequencing. * - POSIX: snapshot the descendant tree, signal members, then killRoot. - * - Windows: taskkill /T /F walks the ConPTY tree (shell → agent → MCP) so - * worktree teardown is not blocked by orphans holding the cwd handle, but only - * after the OS confirms the root PID is still ours and not a recycled stranger. + * - Windows: taskkill /T /F walks the ConPTY tree only when the identity probe + * returns `own` (and ownsRoot still holds). `unknown`/`foreign`/`absent` skip + * tree kill; killRoot always runs. Detached children may survive probe failure. * Callers must not signal the root before this runs on POSIX — a dead root's * descendants reparent to pid 1 and become unfindable. Snapshot failure * degrades to killRoot alone on POSIX. @@ -260,7 +260,7 @@ export async function killWithDescendantSweep( const verify = deps.verifyTreeKillTarget ?? verifyWindowsTreeKillTarget const target = await verify(rootPid).catch((): WindowsTreeKillTarget => 'unknown') // Re-check ownership: the identity query awaits, so exit can land meanwhile. - if (target !== 'absent' && target !== 'foreign' && (deps.ownsRoot?.() ?? true)) { + if (target === 'own' && (deps.ownsRoot?.() ?? true)) { const killTree = deps.killWindowsTree ?? terminateWindowsProcessTree // Why: taskkill may race an already-exited tree; never block killRoot on that. await killTree(rootPid).catch(() => {}) diff --git a/src/main/windows-pty-root-identity.test.ts b/src/main/windows-pty-root-identity.test.ts index d6852b91d..c4be64e27 100644 --- a/src/main/windows-pty-root-identity.test.ts +++ b/src/main/windows-pty-root-identity.test.ts @@ -36,6 +36,14 @@ describe('classifyWindowsTreeKillTarget', () => { expect(classifyWindowsTreeKillTarget(4242, rows, ORCA_PID)).toBe('own') }) + it('documents that a recycled PID under another Orca pane still classifies as own', () => { + // Dead PTY root 4242 recycled as a tool under a different pane's agent tree. + // Ancestry still reaches us, so taskkill is allowed — wrong process, own tree. + // Closing this needs spawn-time CreationDate / Job Object (#10680). + const rows = [link(ORCA_PID, 900), link(7000, ORCA_PID), link(7100, 7000), link(4242, 7100)] + expect(classifyWindowsTreeKillTarget(4242, rows, ORCA_PID)).toBe('own') + }) + it('rejects a recycled pid whose ancestry never reaches this process', () => { const rows = [...SYSTEM_CHAIN, link(ORCA_PID, 900), link(4242, 900)] expect(classifyWindowsTreeKillTarget(4242, rows, ORCA_PID)).toBe('foreign') @@ -100,21 +108,21 @@ describe('verifyWindowsTreeKillTarget', () => { ).resolves.toBe('foreign') }) - it('falls open to unknown when both Windows process probes are unavailable', async () => { + it('returns unknown when both Windows process probes are unavailable', async () => { const readRows = vi.fn().mockResolvedValue(null) await expect( verifyWindowsTreeKillTarget(4242, { readRows, ownerPid: ORCA_PID, platform: 'win32' }) ).resolves.toBe('unknown') }) - it('falls open to unknown when the process query rejects', async () => { + it('returns unknown when the process query rejects', async () => { const readRows = vi.fn().mockRejectedValue(new Error('powershell missing')) await expect( verifyWindowsTreeKillTarget(4242, { readRows, ownerPid: ORCA_PID, platform: 'win32' }) ).resolves.toBe('unknown') }) - it('falls open to unknown when the query throws synchronously', async () => { + it('returns unknown when the query throws synchronously', async () => { const readRows = vi.fn(() => { throw new Error('spawn EPERM') }) diff --git a/src/main/windows-pty-root-identity.ts b/src/main/windows-pty-root-identity.ts index e8cc16c45..b2f6ba43e 100644 --- a/src/main/windows-pty-root-identity.ts +++ b/src/main/windows-pty-root-identity.ts @@ -5,10 +5,10 @@ import { queryWindowsProcessRowsFresh } from './providers/windows-foreground-pro * subtree membership, not root identity: a recycled PID that lands on any other * Orca descendant also reads `own`. It bounds the blast radius of a bad * `taskkill /T /F` to our own tree; it does not prove we spawned this PTY. - * - `own`: ancestry reaches us, so the tree is ours to kill. + * - `own`: ancestry reaches us, so the tree is eligible for guarded teardown. * - `absent`: the PID is gone; `taskkill` would no-op anyway. * - `foreign`: the PID resolves to a process we did not start (PID recycle). - * - `unknown`: no usable evidence; callers keep their prior behavior. + * - `unknown`: no usable evidence; callers must not force-kill the tree. */ export type WindowsTreeKillTarget = 'own' | 'absent' | 'foreign' | 'unknown' @@ -28,11 +28,12 @@ export type WindowsProcessLinkReader = () => Promise