diff --git a/src/main/daemon/pty-subprocess/foreground-process-tracker.ts b/src/main/daemon/pty-subprocess/foreground-process-tracker.ts index 02a5dcb8f2f..bbdad502a6b 100644 --- a/src/main/daemon/pty-subprocess/foreground-process-tracker.ts +++ b/src/main/daemon/pty-subprocess/foreground-process-tracker.ts @@ -123,22 +123,21 @@ export function createPtyForegroundProcessTracker(args: { } foregroundRefreshInFlight = true lastForegroundRefreshStartedAt = now + const identityOlderThan = (ms: number): boolean => + cachedAgentForeground !== null && Date.now() - cachedAgentForeground.refreshedAt > ms const retireStaleForegroundIdentity = ({ onlyWhenAged = false } = {}): void => { const currentFallbackProcess = getFallbackProcess() - const identityAgeMs = - cachedAgentForeground === null ? 0 : Date.now() - cachedAgentForeground.refreshedAt if ( fallbackIsShell && !getActiveStartupAgent() && currentFallbackProcess !== null && isShellProcess(currentFallbackProcess) && - (!onlyWhenAged || identityAgeMs > WINDOWS_DETACHED_DESCENDANT_IDENTITY_MAX_AGE_MS) + (!onlyWhenAged || identityOlderThan(WINDOWS_DETACHED_DESCENDANT_IDENTITY_MAX_AGE_MS)) ) { cachedAgentForeground = null startupAgentForeground = null } else if ( - cachedAgentForeground !== null && - Date.now() - cachedAgentForeground.refreshedAt > FOREGROUND_AGENT_CACHE_TTL_MS && + identityOlderThan(FOREGROUND_AGENT_CACHE_TTL_MS) && currentFallbackProcess !== null && isAgentForegroundWrapperProcess(currentFallbackProcess) ) { @@ -154,19 +153,14 @@ export function createPtyForegroundProcessTracker(args: { } if (!processName || !recognizeAgentProcess(processName)) { if (process.platform === 'win32' && fallbackIsShell && cachedAgentForeground !== null) { - // Job, not console: "is anything besides the shell alive?" needs no - // console attachment, so it needs no forked helper (#10857). + // Job, not console: needs no console attachment, so no fork (#10857). const paneProcessIds = readWindowsPtyJobProcessIds(proc) // Unverifiable is never exit proof (ssh-execution-boundary.md): hold. if (paneProcessIds === null) { return } - // A superset answer is not proof of life -- a WSL pane keeps - // console-detached plumbing in its job, so `size > 1` stays true - // forever and used to veto retirement outright. That pinned a dead - // agent's name (#9258's bug, new mechanism) and, because a non-null - // cache makes idleNoEvidenceShell false, pinned the refresh at 1s - // and defeated the 15s idle backoff. Let age settle it instead. + // `size > 1` is a superset, not proof of life. Vetoing on it pinned a + // dead agent forever wherever a detached descendant lives; age settles it. retireStaleForegroundIdentity({ onlyWhenAged: paneProcessIds.size > 1 }) return } diff --git a/src/main/providers/local-pty-provider.ts b/src/main/providers/local-pty-provider.ts index b160515b43b..0a08433c43f 100644 --- a/src/main/providers/local-pty-provider.ts +++ b/src/main/providers/local-pty-provider.ts @@ -124,10 +124,8 @@ const pendingLocalPtySpawns = new Map>() const ptyShellName = new Map() const ptyAgentForegroundContextPaths = new Map() // Why: remember the last recognized agent foreground so a degraded scan doesn't report the shell and look like an exit. -const ptyLastRecognizedForeground = new Map() -// When each entry above was last confirmed by a scan, so job evidence -- which is -// only a superset -- cannot keep it alive indefinitely. -const ptyLastRecognizedForegroundAt = new Map() +// `at` is when a scan last confirmed it, so job evidence -- only a superset -- cannot hold it forever. +const ptyLastRecognizedForeground = new Map() const ptyTerminalHandle = new Map() const ptyWorktreeId = new Map() const ptyInitialCwd = new Map() @@ -287,7 +285,6 @@ function clearPtyState(id: string): void { ptyShellName.delete(id) ptyAgentForegroundContextPaths.delete(id) ptyLastRecognizedForeground.delete(id) - ptyLastRecognizedForegroundAt.delete(id) ptyTerminalHandle.delete(id) ptyWorktreeId.delete(id) ptyInitialCwd.delete(id) @@ -1408,14 +1405,13 @@ export class LocalPtyProvider implements IPtyProvider { const proc = ptyProcesses.get(id) if (!proc) { ptyLastRecognizedForeground.delete(id) - ptyLastRecognizedForegroundAt.delete(id) return null } const fallbackProcess = resolveForegroundFallbackProcess( proc.process || null, ptyShellName.get(id) ) - const cachedAgent = ptyLastRecognizedForeground.get(id) ?? null + const cachedAgent = ptyLastRecognizedForeground.get(id)?.name ?? null let paneMembershipUnavailable = false // Why: job membership preserves a live cached agent without the whole-table // scan (incomplete under Windows load). Job, not console: this asks "is @@ -1430,16 +1426,15 @@ export class LocalPtyProvider implements IPtyProvider { if (ptyProcesses.get(id) !== proc) { return null } - // Why the age bound: `size > 1` is a superset answer, not proof of life. - // A WSL pane keeps console-detached plumbing in its job, so this - // short-circuit was permanent there and the scan below -- the only path - // that can clear the cache -- never ran again. Fall through once stale. - const cachedAgentAgeMs = Date.now() - (ptyLastRecognizedForegroundAt.get(id) ?? 0) + // Why the age bound: `size > 1` is a superset, not proof of life, so this + // short-circuit was permanent wherever a detached descendant lives -- and it + // skips the scan below, the only path that can clear the cache. + const confirmedAt = ptyLastRecognizedForeground.get(id)?.at ?? 0 if ( paneProcessIds !== null && paneProcessIds.size > 1 && cachedAgent !== null && - cachedAgentAgeMs <= WINDOWS_DETACHED_DESCENDANT_IDENTITY_MAX_AGE_MS + Date.now() - confirmedAt <= WINDOWS_DETACHED_DESCENDANT_IDENTITY_MAX_AGE_MS ) { return cachedAgent } @@ -1461,7 +1456,7 @@ export class LocalPtyProvider implements IPtyProvider { return null } // Why: a degraded scan reporting shell-as-foreground fires a false "agent done"; keep last recognized agent instead. - const lastRecognizedAgent = ptyLastRecognizedForeground.get(id) ?? null + const lastRecognizedAgent = ptyLastRecognizedForeground.get(id)?.name ?? null const resolvedAgent = resolution.processName ? recognizeAgentProcessFromCommandLine(resolution.processName) : null @@ -1473,14 +1468,12 @@ export class LocalPtyProvider implements IPtyProvider { lastRecognizedAgent ) if (stable.lastRecognizedAgent) { - // Every confirmation restarts the clock: the bound means "time since we - // last saw the agent", not "how long it has run". Stamping only on a - // change would age out a live agent that keeps being recognized. - ptyLastRecognizedForegroundAt.set(id, Date.now()) - ptyLastRecognizedForeground.set(id, stable.lastRecognizedAgent) + // Every confirmation restarts the clock: the bound is "time since we last + // saw the agent", not "how long it ran". Stamping only on a name change + // would expire a live agent that keeps being recognized. + ptyLastRecognizedForeground.set(id, { name: stable.lastRecognizedAgent, at: Date.now() }) } else { ptyLastRecognizedForeground.delete(id) - ptyLastRecognizedForegroundAt.delete(id) } return stable.processName } catch { @@ -1488,7 +1481,7 @@ export class LocalPtyProvider implements IPtyProvider { return null } // Why: an inspection error is a degraded read; fall back to last recognized agent (null reads as an exit). - return ptyLastRecognizedForeground.get(id) ?? null + return ptyLastRecognizedForeground.get(id)?.name ?? null } } diff --git a/src/main/providers/windows-pty-job-membership.test.ts b/src/main/providers/windows-pty-job-membership.test.ts index be60fd3f6f3..8c8ab42122e 100644 --- a/src/main/providers/windows-pty-job-membership.test.ts +++ b/src/main/providers/windows-pty-job-membership.test.ts @@ -27,7 +27,7 @@ describe('readWindowsPtyJobProcessIds', () => { forkMock.mockClear() for (let read = 0; read < 50; read += 1) { - readWindowsPtyJobProcessIds(pty(), { listJobProcessIds }) + readWindowsPtyJobProcessIds(pty(), listJobProcessIds) } expect(forkMock).not.toHaveBeenCalled() @@ -35,16 +35,14 @@ describe('readWindowsPtyJobProcessIds', () => { }) it('reports the shell alone, which is what lets a stale agent be retired', () => { - const membership = readWindowsPtyJobProcessIds(pty(), { listJobProcessIds: () => [100] }) + const membership = readWindowsPtyJobProcessIds(pty(), () => [100]) expect(membership).toEqual(new Set([100])) expect(membership?.size).toBe(1) }) it('reports descendants, which is what keeps a live agent cached', () => { - const membership = readWindowsPtyJobProcessIds(pty(), { - listJobProcessIds: () => [100, 200, 300] - }) + const membership = readWindowsPtyJobProcessIds(pty(), () => [100, 200, 300]) expect(membership?.size).toBe(3) }) @@ -56,13 +54,11 @@ describe('readWindowsPtyJobProcessIds', () => { // null is never evidence that processes died // (docs/reference/ssh-execution-boundary.md). An empty list means the tree // is gone, which this function has never been the one to report. - expect(readWindowsPtyJobProcessIds(pty(), { listJobProcessIds: () => pids })).toBeNull() + expect(readWindowsPtyJobProcessIds(pty(), () => pids)).toBeNull() }) it('drops nonsense pids rather than trusting the whole answer', () => { - const membership = readWindowsPtyJobProcessIds(pty(), { - listJobProcessIds: () => [100, 0, -1, 1.5, 200] - }) + const membership = readWindowsPtyJobProcessIds(pty(), () => [100, 0, -1, 1.5, 200]) expect(membership).toEqual(new Set([100, 200])) }) @@ -73,9 +69,7 @@ describe('why the filter does NOT use this', () => { // Shell exited, a descendant is still up. Size 1 -- but reading that as // "the shell is alone, retire the agent" inverts the truth. The forked // probe this replaced required the root in the raw list; so does this. - const membership = readWindowsPtyJobProcessIds(pty(100), { - listJobProcessIds: () => [200] - }) + const membership = readWindowsPtyJobProcessIds(pty(100), () => [200]) expect(membership).toBeNull() }) @@ -88,7 +82,7 @@ describe('why the filter does NOT use this', () => { const listJobProcessIds = vi.fn(() => [100]) const proc = pty(100) - readWindowsPtyJobProcessIds(proc, { listJobProcessIds }) + readWindowsPtyJobProcessIds(proc, listJobProcessIds) expect(listJobProcessIds).toHaveBeenCalledWith(proc) expect(listJobProcessIds).not.toHaveBeenCalledWith(100) @@ -106,9 +100,7 @@ describe('why the filter does NOT use this', () => { // vs console [69908,40980] -- the job is a superset. Harmless for the // `size > 1` callers, wrong for the filter. const detachedChild = 104068 - const membership = readWindowsPtyJobProcessIds(pty(40980), { - listJobProcessIds: () => [40980, detachedChild] - }) + const membership = readWindowsPtyJobProcessIds(pty(40980), () => [40980, detachedChild]) expect(membership?.has(detachedChild)).toBe(true) }) diff --git a/src/main/providers/windows-pty-job-membership.ts b/src/main/providers/windows-pty-job-membership.ts index c0b20680d66..c6c53b4afdb 100644 --- a/src/main/providers/windows-pty-job-membership.ts +++ b/src/main/providers/windows-pty-job-membership.ts @@ -1,54 +1,31 @@ import type { IPty } from 'node-pty' import { listPtyJobProcessIds } from '../windows/windows-pty-job' -type WindowsPtyJobMembershipDeps = { - listJobProcessIds?: (proc: IPty) => readonly number[] | null -} - /** * Processes still running under a pane, or null when there is no answer. * - * Read from the pane's Win32 job object, not from `GetConsoleProcessList`. - * That API must be called from a process attached to the console, which is why - * node-pty answered it by forking a helper — and why asking on a foreground - * poll, per pane, accumulated hundreds of hidden `conpty_console_list_agent` - * processes until the machine ran out of memory (#10857). Each read spawned a - * process; killing them changed nothing because the next poll spawned more. + * Read from the pane's Win32 job object. `GetConsoleProcessList` would need + * console attachment, which is why node-pty answers it by forking a helper, and + * why asking on a foreground poll exhausted memory (#10857). + * `QueryInformationJobObject` has no such constraint: one syscall, no children. * - * `QueryInformationJobObject` has no such constraint: any process holding the - * job handle can ask, so this is one syscall and zero children. Orca already - * creates the job per PTY. + * The job is a SUPERSET of the console -- it keeps console-detached descendants + * -- so `size > 1` is not proof of life, only absence of proof of absence. + * `size === 1` (the shell alone) is decisive; callers must bound the rest. * - * Semantics callers rely on, unchanged by the switch: - * - a root-only set proves the shell is alone, so a stale agent can be retired; - * - `size > 1` proves something is still running under the shell. - * - * The one difference is that a descendant which detached from the console stays - * in the job. That widens the set, which is the conservative direction for - * every caller: it keeps a live agent rather than retiring it early. - * - * Null still means unverifiable per docs/reference/ssh-execution-boundary.md -- - * an unpatched node-pty, a non-ConPTY terminal, or a tree no longer tracked. - * It is never evidence that processes died. + * Null means unverifiable per docs/reference/ssh-execution-boundary.md, never + * that processes died. */ export function readWindowsPtyJobProcessIds( proc: IPty, - deps: WindowsPtyJobMembershipDeps = {} + listJobProcessIds: (proc: IPty) => readonly number[] | null = listPtyJobProcessIds ): ReadonlySet | null { - const pids = (deps.listJobProcessIds ?? listPtyJobProcessIds)(proc) + const pids = listJobProcessIds(proc) if (!pids) { return null } - const membership = new Set() - for (const pid of pids) { - if (Number.isSafeInteger(pid) && pid > 0) { - membership.add(pid) - } - } - // Why require the shell: callers read a size-1 set as "the shell is alone, so - // retire the agent". A set of one pid that is NOT the shell means the shell - // already exited with a descendant still up -- the opposite state, and one the - // forked probe this replaced refused outright (it required the root in the raw - // list). Reporting unverifiable keeps that refusal. + const membership = new Set(pids.filter((pid) => Number.isSafeInteger(pid) && pid > 0)) + // Without the shell, a size-1 set would read as "shell alone, retire" when it + // means the opposite. The forked probe this replaced refused the same way. return membership.has(proc.pid) ? membership : null }