2 Commits

Include renames

Author	SHA1	Message	Date
Neil	a1ec0479e2	fix(windows): revalidate PTY liveness from the job object, not a forked helper (#16419) ... * 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.	2026-08-25 20:32:54 -07:00
Brennan Benson	f655ae3929	fix(pi): resolve OMP tab identity from the outer wrapper, not the wrapped pi (#6364) (#9600) ... * fix(pi): resolve OMP tab identity from the outer wrapper, not the wrapped pi (#6364) OMP runs as a `shell → omp → pi` process tree and Orca recognizes both `omp` and `pi` as distinct agents. The foreground-process readers scored the deepest `+` foreground descendant, so they returned the wrapped `pi` engine instead of the `omp` the user actually launched — and because the sampled `+` frame moves between omp and pi across command boundaries, the read oscillated and the tab icon/label flickered OMP↔Pi. It surfaced on Remote Host / SSH (only in a worktree, only while working) where a mirrored/restored pane has no client-side launchAgent to anchor identity, so the (host-read) foreground was the only pi/omp signal — and it was wrong. Fix the detection at its source so foregroundAgent is trustworthy everywhere (local tab icon, exit detection, and the remote mirror): - Add `resolveOuterWrapperForegroundProcess` (src/shared/foreground-wrapper-agent.ts): when the recognized foreground winner shares a `titleIdentityGroup` with a shallower recognized ancestor, return the shallowest — the outer wrapper. Stable regardless of which of omp/pi holds the foreground at the sample; bare Pi and cross-group reads (Codex, etc.) are unchanged. - Wire it into both readers: local `agent-foreground-process.ts` and the SSH relay `pty-shell-utils.ts`. Also harden the relay's fallback short-circuit so a pi-compatible process reported by node-pty rescans for the omp wrapper instead of trusting `pi` raw (never downgrading below the fallback). Also unify the host-side publish path onto the shared owner resolver: the mobile session snapshot builders now resolve pane ownership via `resolvePaneAgentOwner` ({launchAgent, hookAgent}) instead of a bespoke `launchAgent ?? foregroundAgent`, so the host stops being a divergent fourth identity path and the hook's host-stamped identity survives when launchAgent is dropped on a mirrored pane. Tests: unit coverage for the wrapper resolver, `shell→omp→pi` ps trees for both readers (outer omp, oscillating-frame, bare pi, node-pty pi fallback), and a host-snapshot regression that keeps an OMP hook labeled OMP when the wrapped pi child owns the foreground. * fix(pi): harden wrapper foreground ownership * fix(pi): preserve OMP ownership in daemon foreground reads * fix(pi): retain OMP owner when process scans fail	2026-07-20 16:16:34 -07:00