mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 16:02:32 +00:00
Neil
a1ec0479e2
* fix(windows): answer console membership from the job object, not a forked helper
node-pty answers "which processes are attached to this pane's console?" by
FORKING a helper, because GetConsoleProcessList must run from a process
attached to that console. Orca asked on a foreground poll, per pane, so each
read spawned a conpty_console_list_agent -- hundreds of hidden processes
exhausting RAM within minutes, respawning as fast as they were killed (#10857).
QueryInformationJobObject has no console-attachment constraint: any process
holding the job handle can ask. Orca already creates that job per PTY, and
listPtyJobProcessIds has exposed it since the W1/W2 work with zero callers.
One syscall, no children.
Semantics the three call sites rely on are preserved: a root-only set still
proves the shell is alone (so a stale agent can be retired), and size > 1 still
proves something is running under it. The single difference is that a
descendant detached from the console stays in the job -- which widens the set,
the conservative direction for every caller.
Also fixes the third call site, which returned { available: false } whenever
membership was unavailable AND a recognized agent existed -- i.e. exactly while
an agent was running. Membership only ever narrowed the candidate list, so an
unavailable answer now leaves it unfiltered instead of failing the whole
resolution.
The no-fork test is asserted through a module-level vi.mock of
node:child_process. A vi.spyOn of a require()'d child_process does not
intercept the module's own import binding: the first version of that test
passed with a fork() deliberately reintroduced.
* 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.
* fix: restore the coverage and ratchets the module split dropped
Round 2 of review. Two blockers, both from moving the forking code to a new
file without moving what guarded it.
- The child_process import ratchet was RED: windows-console-attached-processes.ts
imports node:child_process and was unlisted, and the old entry was stale. I
never ran that suite -- lint and the providers/daemon tests both pass without
it, which is exactly the gap the ratchet exists to close. Entry repointed;
count unchanged at 159.
- The forking module had ZERO tests. Its 11 assertions -- bounded timeout,
single kill, spawn error, malformed message, helper-pid removal -- were in the
file that now answers a different question, so the module that actually caused
#10857 was shipping untested. Moved with the code.
Also: nothing pinned the round-1 fix itself. No test drove console attachment to
null and asserted the fail-closed result, so re-deleting that branch would have
gone green. Now covered, and verified to fail when the branch is removed.
Cleanups the split left behind: `consoleMembershipUnavailable`/`consoleProcessIds`
renamed to `pane*` where they now hold job membership, the duplicated
`WindowsConptyMembershipDeps` type name, comments still describing the console
on the job path, and eight reliability-gate paths pointing at the moved tests.
* fix(windows): let a superset job answer expire instead of vetoing retirement
Round 3. The job read had reintroduced #9258's bug by a new mechanism.
`size > 1` returned unconditionally, so any pane holding a console-detached
descendant never retired its cached agent. A WSL pane always holds some: the
measurement in this PR's own test recorded job [40980,104068,4888,69908] against
console [69908,40980], i.e. console said "shell alone, retire" while the job said
"three others alive, keep". #9258's third commit describes the identical failure
from the other direction -- a bare shell reading as [helper, shell] "looked like
it still had a child ... the foreground refresh held the exited agent's identity
indefinitely" -- and that is what came back.
It bites because the read branch that serves the cached name across a Windows
shell fallback is deliberately untimed: #9258 made it so on the stated assumption
that "the background refresh authoritatively retires it". Removing the retire
authority left the identity with no bound at all. Second-order: a non-null cache
makes idleNoEvidenceShell false, which pins the refresh at the 1s TTL, so an idle
WSL pane also scanned the process table every second forever.
A TTL on the read would have been the wrong fix -- untimed is deliberate, because
on Windows the fallback name is structurally uninformative. Instead the job answer
is treated as what it is: a SUPERSET of the console, which cannot tell a working
agent from a leftover. Proof of absence retires immediately (size 1, unchanged);
an inconclusive answer ages out at 30s; unverifiable (null) still holds forever
per ssh-execution-boundary.md. Only successful scans that found no agent advance
the clock -- a degraded scan returns before this -- so the fix cannot expire an
agent it simply failed to see.
Also from review:
- Restore the root requirement the forked probe had. Without it a set of one
non-root pid -- shell gone, descendant alive -- read as "shell alone, retire",
inverting the truth.
- Rename to windows-pty-job-membership.ts / readWindowsPtyJobProcessIds. The old
name still said ConPTY console while reading the job, and conflating those two
sets is precisely the bug aee07c24aa reverted. Same for
windows-console-foreground.ts, which guards a job read now.
- Gate the two files that had no coverage: the job read and the retire path.
* fix(windows): bound the provider's job short-circuit too
The previous commit fixed the daemon retire path and left the identical bug in
the local provider, which I found while asking the reviewer to check for it.
local-pty-provider.ts returned the cached agent early on `size > 1` and that
early return skips the scan at the bottom of getForegroundProcess -- the ONLY
code that can delete ptyLastRecognizedForeground. So on a WSL pane, whose job
always holds console-detached plumbing, the short-circuit was permanent and the
identity could never be cleared. Same failure, second location, and the daemon
fix did nothing for it because this path never calls retireStaleForegroundIdentity.
The cache was a bare Map<id, name> with no timestamp, so bounding it needs one.
Added ptyLastRecognizedForegroundAt, stamped only when the recognized name
actually changes, and paired with every existing delete including pane teardown
so the new map cannot outlive the old one.
The 30s threshold now lives in windows-cached-agent-revalidation.ts rather than
being duplicated: that module already answers "can we revalidate this cached
agent without a scan", and the max age is the other half of that question.
Also renamed two tests that still said "ConPTY console presence" while driving a
job read. Re-conflating those two sets by name is how this PR got its first two
review rounds wrong.
* fix(windows): stamp the provider cache on every confirmation, not on change
My own previous commit was wrong, and wrong in the direction #9258 exists to
prevent. Review caught it; the test in this commit reproduces it first.
I stamped ptyLastRecognizedForegroundAt only when the recognized name CHANGED.
That makes the value the time of first recognition, so the age measures how long
the agent has been running rather than how long since we last confirmed it. For
a live agent recognized as the same name every cycle the stamp never moved, the
age crossed 30s and stayed there, and the short-circuit died permanently.
Two consequences, the second serious:
- every getForegroundProcess call on a >30s-old agent pane ran the whole-table
scan, defeating the exact optimization the branch exists for;
- with the short-circuit off, one available-but-agentless snapshot was enough to
delete a LIVE agent's identity, because paneMembershipUnavailable is false in
this state so the degraded-scan substitution does not engage. That is the false
"agent done" this code's own comment warns about.
The daemon path was already right -- it re-stamps refreshedAt on every positive
recognition -- so the same constant meant two different things in the two files.
Now both mean "time since we last saw the agent", which turns the bound from
"disable the short-circuit after 30s" into "force one revalidating scan every
30s": ~16-31ms per pane per 30s via the native process table.
Test asserts the scan count stops incrementing after the revalidation, and fails
against the stamp-on-change form.
Also correct the shared docstring, which had dropped the invariant the whole
design rests on, and stop calling this a WSL bug: the trigger is a persistent
console-detached job member plus a fallback that reads as a shell. wsl.exe is
not in SHELL_NAMES, so a plain WSL pane does not even reach this code -- WSL is
just where it was measured.
* refactor(windows): shrink the job-membership path
Elegance pass. No behaviour change -- all three mutation checks still bind
(restoring the size>1 veto, stamping only on name change, dropping the root
requirement each turn their tests red).
- windows-pty-job-membership.ts 54 -> 31 lines. A deps object carrying one
optional function became a defaulted parameter, the accumulate loop became a
filter, and the docblock lost two thirds of its bulk.
It also lost a claim that was simply false: it said a widened set "is the
conservative direction for every caller: it keeps a live agent rather than
retiring it early". For the retire caller, never retiring IS the failure --
that is the bug this stack just fixed, still being described as a feature
three commits later.
- One local `identityOlderThan(ms)` in the tracker replaces two hand-rolled
`Date.now() - refreshedAt` comparisons, one of which I had added.
- The provider's two parallel maps collapse into one Map<id, {name, at}>.
Parallel maps meant every delete site had to remember its sibling, in three
places; the reviewer flagged the leak risk and I fixed it by pairing them,
which leaves the hazard for the next person. One map removes the class.
Comments trimmed to the load-bearing sentence throughout, per AGENTS.md.
* fix(windows): preserve foreground cache age evidence
* fix(windows): anchor cached agent identity to the pid that proved it
The job short-circuit and retirement veto only knew 'something besides the
shell is alive', so a detached leftover pinned a dead agent's name for the
30s age bound, and 30s of incomplete-but-successful scans could retire a
live one. The scan already knows which row proved the name: carry that pid
through the resolution, and judge the cache against the job with it --
membership of a known pid in a complete, inescapable job list is proof of
life (restamp, never expire), and its absence is proof of exit (retire now,
leftovers notwithstanding). Unanchored identities keep the age-bound
superset behavior.
* fix(windows): anchor the reported process, and let a scan refute a recycled pid
Review findings on the pid anchor:
1. The anchor followed the LEAF that proved a collapsed name: 'omp' reported,
pi's pid stored. Pi exiting or restarting under a live OMP then read as the
wrapper's exit -- retiring the identity before a scan that (degraded) may
miss OMP, a false 'agent done'. resolveOuterWrapperForegroundIdentity now
carries the pid of the process the name belongs to.
2. A bare numeric pid can be recycled inside the pane's job, making membership
falsely confirm a dead identity indefinitely. Command lines are immutable,
so a scan row holding the anchor pid without recognizing as an agent proves
a different process: the resolution reports it (anchorPidForeign) and both
consumers retire immediately. A query-denied row (command falls back to the
image name) stays inconclusive -- never grounds to drop a live agent.
* fix(windows): find a recycled anchor pid in the full table, not the ppid walk
A squatter that inherited the pane job from a leftover whose creator then
exited is orphaned out of the shell-rooted descendant projection, so the
foreign-anchor refutation never saw its row. Pluck the anchor pid's row from
the same whole-table snapshot instead; a job member holding the pid is in the
table even when no ppid chain reaches it.
* fix(windows): survive an agent restart, and refute a squatter by name
Two review findings on the exit verdicts:
1. 'exited' deleted the cache before the scan, so an agent restarting under a
new pid plus a degraded scan at that instant reported the shell -- a false
'agent done'. Only the shell standing alone is decisive now; an anchor
leaving a job that still has members downgrades to unanchored, age-bounded
evidence and lets the scan decide. The daemon tracker keeps immediate
retirement: its verdict path only runs after an available scan already
found no agent.
2. The foreign-anchor refutation treated any recognized row as 'ours'. A pid
recycled by a DIFFERENT agent now compares against the cached name the
anchor is supposed to prove.
77 lines
2.7 KiB
TypeScript
77 lines
2.7 KiB
TypeScript
import { describe, expect, it } from 'vitest'
|
|
import {
|
|
resolveOuterWrapperForegroundIdentity,
|
|
resolveOuterWrapperForegroundProcess
|
|
} from './foreground-wrapper-agent'
|
|
|
|
describe('resolveOuterWrapperForegroundProcess', () => {
|
|
const omp = { agent: 'omp' as const, processName: 'omp' }
|
|
const pi = { agent: 'pi' as const, processName: 'pi' }
|
|
|
|
it('collapses a wrapped pi read onto the shallower omp wrapper', () => {
|
|
// Winner is the deeper pi (depth 2); omp is its wrapper at depth 1.
|
|
expect(
|
|
resolveOuterWrapperForegroundProcess(pi, { pid: 102, ppid: 101, command: 'pi' }, [
|
|
{ pid: 102, ppid: 101, command: 'pi' },
|
|
{ pid: 101, ppid: 100, command: 'omp' }
|
|
])
|
|
).toBe('omp')
|
|
})
|
|
|
|
it('keeps bare pi when no same-group wrapper is present', () => {
|
|
const barePi = { pid: 101, ppid: 100, command: 'pi' }
|
|
expect(resolveOuterWrapperForegroundProcess(pi, barePi, [barePi])).toBe('pi')
|
|
})
|
|
|
|
it('leaves a cross-group agent (codex) untouched even under a deeper same-name child', () => {
|
|
const codex = { agent: 'codex' as const, processName: 'codex' }
|
|
expect(
|
|
resolveOuterWrapperForegroundProcess(
|
|
codex,
|
|
{ pid: 102, ppid: 101, command: 'node /usr/bin/codex' },
|
|
[
|
|
{ pid: 102, ppid: 101, command: 'node /usr/bin/codex' },
|
|
{ pid: 101, ppid: 100, command: 'bash -l' }
|
|
]
|
|
)
|
|
).toBe('codex')
|
|
})
|
|
|
|
it('does not promote a deeper pi over an already-outer omp winner', () => {
|
|
expect(
|
|
resolveOuterWrapperForegroundProcess(omp, { pid: 101, ppid: 100, command: 'omp' }, [
|
|
{ pid: 101, ppid: 100, command: 'omp' },
|
|
{ pid: 102, ppid: 101, command: 'pi' }
|
|
])
|
|
).toBe('omp')
|
|
})
|
|
|
|
it('does not treat an unrelated shallower omp sibling as the pi wrapper', () => {
|
|
expect(
|
|
resolveOuterWrapperForegroundProcess(pi, { pid: 103, ppid: 102, command: 'pi' }, [
|
|
{ pid: 101, ppid: 100, command: 'omp' },
|
|
{ pid: 102, ppid: 100, command: 'node server.js' },
|
|
{ pid: 103, ppid: 102, command: 'pi' }
|
|
])
|
|
).toBe('pi')
|
|
})
|
|
|
|
it('carries the wrapper pid with a collapsed name, so liveness tracks the wrapper', () => {
|
|
// Anchoring 'omp' to pi's pid would read a pi restart as omp's exit.
|
|
expect(
|
|
resolveOuterWrapperForegroundIdentity(pi, { pid: 102, ppid: 101, command: 'pi' }, [
|
|
{ pid: 102, ppid: 101, command: 'pi' },
|
|
{ pid: 101, ppid: 100, command: 'omp' }
|
|
])
|
|
).toEqual({ processName: 'omp', processId: 101 })
|
|
})
|
|
|
|
it('keeps the winner pid when nothing collapses', () => {
|
|
const barePi = { pid: 101, ppid: 100, command: 'pi' }
|
|
expect(resolveOuterWrapperForegroundIdentity(pi, barePi, [barePi])).toEqual({
|
|
processName: 'pi',
|
|
processId: 101
|
|
})
|
|
})
|
|
})
|