mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 16:02:32 +00:00
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:
@@ -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()
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
}
|
||||
Reference in New Issue
Block a user