fix(windows): keep console attachment for the candidate filter

Readiness review caught that this PR changed two different questions as if they
were one, and the repo's own plan doc had already said so:

  "The job is the wrong set here -- it would re-admit precisely the detached
   process the filter exists to drop."  (windows-wsl-root-cause-plan.html, Use B)

The two uses:

- Use A, `size > 1` at local-pty-provider and the daemon tracker -- "is anything
  in this pane besides the shell?". The job answers this, in-process and with no
  fork. Unchanged from the previous commit.
- Use B, the candidate filter -- "which of these are ATTACHED TO THIS CONSOLE?".
  Its whole job is dropping a descendant that detached, and the job object keeps
  those, so answering it from the job makes the filter a no-op in its motivating
  case: a detached `Start-Process droid` would be granted byte authority, and a
  detached sibling would make an attached agent look ambiguous.

Use B goes back to GetConsoleProcessList, in its own module named for what it
answers, with its fail-closed null restored. That path is not the #10857 storm:
it runs only when a recognized agent candidate already exists, not on every
foreground poll. Bounding it to one pooled supervised helper is the remaining
half, and per the plan doc either half alone takes #10857 from unbounded to one.

My earlier claim that widening membership is "the conservative direction for
every caller" was wrong -- true for Use A, backwards for Use B. The hardware run
did not catch it because I measured a WSL pane, where the superset is harmless,
and never a detached GUI child, which is the divergence.
This commit is contained in:
Neil
2026-08-25 02:26:20 -07:00
parent 50e66451e8
commit aee07c24aa
7 changed files with 146 additions and 22 deletions
@@ -3,6 +3,7 @@ import { win32 as pathWin32 } from 'node:path'
import { getAgentForegroundContextPaths } from '../../providers/agent-foreground-context-paths'
import { resolveAgentForegroundProcessWithAvailability } from '../../providers/agent-foreground-process'
import { readWindowsConptyProcessIds } from '../../providers/windows-conpty-process-membership'
import { readWindowsConsoleAttachedProcessIds } from '../../providers/windows-console-attached-processes'
import {
isAgentForegroundWrapperProcess,
recognizeAgentProcess,
@@ -247,7 +248,8 @@ export function createPtyForegroundProcessTracker(args: {
...(process.platform === 'win32'
? {
forceProcessScan: true,
readWindowsConptyProcessIds: () => readWindowsConptyProcessIds(proc)
readWindowsConsoleAttachedProcessIds: () =>
readWindowsConsoleAttachedProcessIds(proc.pid)
}
: {})
}
@@ -52,14 +52,14 @@ describe('Pi Windows foreground recognition', () => {
getAllProcessesMock.mockImplementation((cb: (snapshot: unknown) => void) => {
cb(withSelf(rows))
})
const readWindowsConptyProcessIds = vi.fn(() => new Set([100, 101]))
const readWindowsConsoleAttachedProcessIds = vi.fn(async () => new Set([100, 101]))
await expect(
resolveAgentForegroundProcessWithAvailability(100, 'node.exe', {
fresh: true,
readWindowsConptyProcessIds
readWindowsConsoleAttachedProcessIds
})
).resolves.toEqual({ available: true, processName: 'pi' })
expect(readWindowsConptyProcessIds).toHaveBeenCalledTimes(1)
expect(readWindowsConsoleAttachedProcessIds).toHaveBeenCalledTimes(1)
})
})
@@ -508,7 +508,7 @@ describe('resolveAgentForegroundProcess', () => {
await expect(
resolveAgentForegroundProcessWithAvailability(100, 'powershell.exe', {
fresh: true,
readWindowsConptyProcessIds: () => new Set([100, 101])
readWindowsConsoleAttachedProcessIds: async () => new Set([100, 101])
})
).resolves.toEqual({ available: true, processName: 'droid' })
})
@@ -655,15 +655,15 @@ describe('resolveAgentForegroundProcess', () => {
commandLine: 'droid'
}
])
const readWindowsConptyProcessIds = vi.fn(() => new Set([100, 101, 999]))
const readWindowsConsoleAttachedProcessIds = vi.fn(async () => new Set([100, 101, 999]))
await expect(
resolveAgentForegroundProcessWithAvailability(100, 'powershell.exe', {
fresh: true,
readWindowsConptyProcessIds
readWindowsConsoleAttachedProcessIds
})
).resolves.toEqual({ available: true, processName: 'droid' })
expect(readWindowsConptyProcessIds).toHaveBeenCalledTimes(1)
expect(readWindowsConsoleAttachedProcessIds).toHaveBeenCalledTimes(1)
})
it('excludes a detached Windows Droid descendant from byte authority', async () => {
@@ -686,7 +686,7 @@ describe('resolveAgentForegroundProcess', () => {
await expect(
resolveAgentForegroundProcessWithAvailability(100, 'powershell.exe', {
fresh: true,
readWindowsConptyProcessIds: () => new Set([100, 999])
readWindowsConsoleAttachedProcessIds: async () => new Set([100, 999])
})
).resolves.toEqual({ available: true, processName: 'powershell.exe' })
})
@@ -701,14 +701,14 @@ describe('resolveAgentForegroundProcess', () => {
commandLine: 'powershell.exe'
}
])
const readWindowsConptyProcessIds = vi.fn(() => new Set([100, 999]))
const readWindowsConsoleAttachedProcessIds = vi.fn(async () => new Set([100, 999]))
await expect(
resolveAgentForegroundProcessWithAvailability(100, 'powershell.exe', {
fresh: true,
readWindowsConptyProcessIds
readWindowsConsoleAttachedProcessIds
})
).resolves.toEqual({ available: true, processName: 'powershell.exe' })
expect(readWindowsConptyProcessIds).not.toHaveBeenCalled()
expect(readWindowsConsoleAttachedProcessIds).not.toHaveBeenCalled()
})
})
+3 -1
View File
@@ -65,6 +65,7 @@ import { getAgentForegroundContextPaths } from './agent-foreground-context-paths
import { recognizeAgentProcessFromCommandLine } from '../../shared/agent-process-recognition'
import { killWithDescendantSweep } from '../pty-descendant-termination'
import { readWindowsConptyProcessIds } from './windows-conpty-process-membership'
import { readWindowsConsoleAttachedProcessIds } from './windows-console-attached-processes'
import { terminatePtyJob } from '../windows/windows-pty-job'
import { canConfirmAgentFromConsolePresence } from './windows-console-foreground'
import { forceKillPosixPtyProcessGroups } from '../pty/posix-pty-process-groups'
@@ -1480,7 +1481,8 @@ export class LocalPtyProvider implements IPtyProvider {
...(process.platform === 'win32'
? {
forceProcessScan: true,
readWindowsConptyProcessIds: () => readWindowsConptyProcessIds(proc)
readWindowsConsoleAttachedProcessIds: () =>
readWindowsConsoleAttachedProcessIds(proc.pid)
}
: {})
}
@@ -23,7 +23,7 @@ export type AgentForegroundResolutionOptions = {
/** Force confirmation scans even when node-pty reports a recognized name. */
forceProcessScan?: boolean
/** Lazily proves which global descendants still belong to this ConPTY. */
readWindowsConptyProcessIds?: () => ReadonlySet<number> | null
readWindowsConsoleAttachedProcessIds?: () => Promise<ReadonlySet<number> | null>
}
export type WindowsAgentForegroundResolution = {
@@ -71,15 +71,17 @@ export async function resolveWindowsAgentForegroundProcessWithAvailability(
options.contextPaths
)
let filteredCandidates = candidates
if (hasRecognizedCandidate && options.readWindowsConptyProcessIds) {
const conptyProcessIds = options.readWindowsConptyProcessIds()
// Why not bail on null: membership only ever NARROWED the candidates. When
// it cannot answer, the unfiltered list is still a usable answer, and
// failing the whole resolution instead reported "unavailable" exactly while
// an agent was recognized -- disabling the callers that gate on it.
if (conptyProcessIds) {
filteredCandidates = candidates.filter((candidate) => conptyProcessIds.has(candidate.pid))
if (hasRecognizedCandidate && options.readWindowsConsoleAttachedProcessIds) {
// Why console attachment and not the job: this filter exists to DROP a
// descendant that detached from the console, and the job still contains
// those by design. Answering it from the job would re-admit precisely what
// the filter is for -- granting byte authority to a detached `Start-Process
// droid`, or making an attached agent look ambiguous.
const consoleProcessIds = await options.readWindowsConsoleAttachedProcessIds()
if (!consoleProcessIds) {
return { available: false, processName: null }
}
filteredCandidates = candidates.filter((candidate) => consoleProcessIds.has(candidate.pid))
}
return {
available: true,
@@ -67,3 +67,24 @@ describe('readWindowsConptyProcessIds', () => {
expect(membership).toEqual(new Set([100, 200]))
})
})
describe('why the filter does NOT use this', () => {
it('documents that job membership keeps console-detached descendants', () => {
// The candidate filter in windows-agent-foreground-process.ts exists to DROP
// a descendant that left the console (`Start-Process droid`, a GUI child).
// The job still contains those by design, so answering that filter from the
// job would re-admit exactly what it is for -- granting byte authority to a
// pane no agent owns, or making an attached agent look ambiguous.
// docs/windows-wsl-root-cause-plan.html calls this out as "Use B".
//
// Measured on Windows 11 against a real WSL pane: job [40980,104068,4888,69908]
// vs console [69908,40980] -- the job is a superset. Harmless for the
// `size > 1` callers, wrong for the filter.
const detachedChild = 104068
const membership = readWindowsConptyProcessIds(pty(40980), {
listJobProcessIds: () => [40980, detachedChild]
})
expect(membership?.has(detachedChild)).toBe(true)
})
})
@@ -0,0 +1,97 @@
import { fork, type ChildProcess } from 'node:child_process'
const CONPTY_PROCESS_LIST_TIMEOUT_MS = 3_000
type ProcessListMessage = { consoleProcessList?: unknown }
type WindowsConptyMembershipDeps = {
forkProcess?: typeof fork
resolveAgentPath?: () => string
timeoutMs?: number
}
function resolveNodePtyConsoleListAgent(): string {
return require.resolve('node-pty/lib/conpty_console_list_agent.js')
}
/**
* Processes ATTACHED TO THIS PANE'S CONSOLE, or null when unavailable.
*
* Distinct from job membership on purpose. `GetConsoleProcessList` must be
* called from a process attached to that console, and a process can hold only
* one console at a time -- which is why node-pty answers it from a separate
* process, and why this still forks.
*
* Only the candidate FILTER may use this. That filter exists to drop a
* descendant which detached from the console (`Start-Process`, a GUI child), and
* the job object deliberately still contains those, so the job cannot answer it
* -- see docs/windows-wsl-root-cause-plan.html, "Use B".
*
* This is not the fork storm in #10857: it runs only when a recognized agent
* candidate already exists, not on every foreground poll. Bounding it to one
* pooled, supervised helper is the remaining half of that fix.
*/
export function readWindowsConsoleAttachedProcessIds(
rootPid: number,
deps: WindowsConptyMembershipDeps = {}
): Promise<ReadonlySet<number> | null> {
if (!Number.isSafeInteger(rootPid) || rootPid <= 0) {
return Promise.resolve(null)
}
let child: ChildProcess
try {
child = (deps.forkProcess ?? fork)(
(deps.resolveAgentPath ?? resolveNodePtyConsoleListAgent)(),
[String(rootPid)],
{ silent: true }
)
} catch {
return Promise.resolve(null)
}
return new Promise((resolve) => {
let settled = false
const finish = (value: ReadonlySet<number> | null): void => {
if (settled) {
return
}
settled = true
clearTimeout(timeout)
child.removeListener('message', onMessage)
// Why: kill failures can emit asynchronously after timeout settlement;
// teardown listeners stay until exit so they cannot crash the daemon.
resolve(value)
}
const onFailure = (): void => finish(null)
const onExit = (): void => {
child.removeListener('error', onFailure)
finish(null)
}
const onMessage = (message: ProcessListMessage): void => {
const value = message?.consoleProcessList
const helperPid = child.pid
if (
!Array.isArray(value) ||
helperPid === undefined ||
!value.includes(rootPid) ||
!value.includes(helperPid) ||
value.some((pid) => !Number.isSafeInteger(pid) || pid <= 0)
) {
finish(null)
return
}
// Why: GetConsoleProcessList includes this helper; removing it makes a
// root-only set authoritative shell-only evidence instead of a false child.
const consoleProcessIds = new Set(value)
consoleProcessIds.delete(helperPid)
finish(consoleProcessIds)
}
const timeout = setTimeout(() => {
child.kill()
finish(null)
}, deps.timeoutMs ?? CONPTY_PROCESS_LIST_TIMEOUT_MS)
child.once('message', onMessage)
child.once('error', onFailure)
child.once('exit', onExit)
})
}