fix(terminal): confirm the agent from ConPTY console presence to avoid false exits#9258
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughWindows foreground-process detection now uses cached agent identity and ConPTY console membership to confirm active agents before running the full resolution scan. ConPTY results are validated and filtered to exclude the helper process. A new confirmation helper defines the fast-path conditions, while daemon refresh logic preserves cached identities during degraded scans and retires them only after shell-only confirmation. Tests cover membership filtering, fast confirmation, fallback scans, and identity retention. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Windows validation: the fix is correct but lands on a path that doesn't run on Windows-with-daemonValidated on real Windows 11. Summary: the technique is sound, but as written this change does not engage on the default Windows configuration, so it does not fix the false "agent done" it targets. The same is true of the already-merged sibling change in The routing gap (root cause of "we can't see it engage live")On Windows the terminal daemon hosts PTYs. Direct proof (instrumented both paths, verified compiled into the running build, then drove a real agent):
Every foreground resolution went through the daemon; the modified code never ran. The false-exit reproduces on the daemon pathWith a recognized agent genuinely working ("Running 1 shell command · 2m 16s", status "Working…"), under the load that times out the whole-table CIM scan (CIM 31/31 timed out, avg ~3175ms):
Mechanism: The good news: the technique is validatedThe ConPTY console-membership read is reliable under exactly the load that times out the CIM scan. Standalone rig against the real
So the approach is right — it just has to live in the daemon. Where the fix belongsApply the same idea to the daemon's Net: keep this change (correct, no-regression on the fallback path), but the Windows-with-daemon fix needs to move into |
8b87868 to
bf93202
Compare
Expanded to the primary Windows path (daemon)Validation on real Windows showed the initial change never engaged: it lived in the local PTY provider, but Windows PTYs are hosted by the terminal daemon, whose own foreground-identity refresh is what runs by default. Instrumented hit counts confirmed the local-provider path was bypassed entirely. Added the same protection to the daemon foreground path (
New daemon degraded-scan tests cover keep-on-degraded, keep-on-incomplete-snapshot-with-console-child, and retire-on-shell-only-console. Full daemon + providers suites green; typecheck/lint clean. Head is now |
…d false exits On Windows the foreground scan is a whole-process-table PowerShell fork that, under load, exceeds its timeout or returns an incomplete snapshot — the completion coordinator then reads the shell as the foreground and fires a false "agent done" while the agent is still working. While a recognized agent is still active, confirm it with a cheap ConPTY console-membership read instead: a child process still attached to the console means the agent is working, so keep it without the whole-table scan. Fall through to the authoritative scan only when the console is shell-only (the agent likely exited). No-op off Windows, and never worse than the existing degraded-scan fallback.
…foreground path Windows PTYs are hosted by the terminal daemon, whose foreground-identity refresh (not the local provider) is what runs by default — it retired the cached agent on a timed-out or incomplete CIM scan, so foreground reads fell back to the shell and fired a false "agent done" while the agent was working. Mirror the local-provider fix on the daemon path: a degraded scan (available:false) no longer retires the identity; an authoritative scan that resolves no agent is confirmed against a ConPTY console-membership read before retiring (an incomplete snapshot with a child still attached keeps the agent); and the sync foreground read serves the cached agent across a shell fallback on Windows (an unreliable exit signal under load) until the background refresh authoritatively retires it. The membership read stays off the sync path.
…PTY membership The console-membership helper attaches to the console to read it, so GetConsoleProcessList counts the helper's own forked process. A bare shell therefore read as [helper, shell] and looked like it still had a child, so a genuine shell-only console (an exited agent) was never detected — the foreground refresh held the exited agent's identity indefinitely. Drop the helper's own pid before judging membership; a remaining set of only the shell (or the AttachConsole-failure fallback) is not child proof. Fixes real-exit detection on both the daemon and local foreground paths.
bf93202 to
59c5cee
Compare
Windows re-validation: the relocation to the daemon path works — false-exit-under-load is genuinely fixedValidated on real Windows 11. Instrumented the daemon ( #1 — Fast path fires; no false "agent done" under load: PASSWith the agent genuinely working, under CIM load (CIM 23/23 timed out, avg ~3.1s):
Daemon decision log confirms the mechanism:
The primary bug — the false "agent done" when the CIM scan times out under load — is fixed on the path that actually runs on Windows (the terminal daemon). #2 — Real exit detection: a regression was found in this revision, root-caused, and has since been fixedOn the revision first validated, after a real exit the daemon kept reporting the exited agent's name indefinitely (40s+, 0 retires). Root cause: the ConPTY console-membership helper attaches to the console to read it, so it counts its own probe process — a bare shell read as size 2 ( Fix direction (now landed): drop the helper's own PID before judging membership and treat "only the shell remaining" as no-child, in the shared helper so both the daemon and local-provider paths are covered — plus a real-function unit test for the Net: keep this change — the false-exit-under-load is genuinely fixed on the daemon path, and the exit-detection regression it briefly introduced has been root-caused and corrected in the same shared helper. |
3 participants
Follow-up to #9244. Fixes false "agent done" notifications on Windows by removing the whole-process-table scan from the hot path of foreground detection — including the daemon path, which is what actually runs on Windows.
Problem
Deciding an agent finished depends partly on "is the agent still the terminal's foreground process?" On Windows that answer comes from a whole-process-table
Get-CimInstance Win32_ProcessPowerShell scan with a 3s timeout. Under load (many agents doing heavy work) the scan times out or returns an incomplete snapshot, so the completion coordinator reads the shell as the foreground, concludes the agent exited, and fires a false completion while it's still working — multiplied across panes into the notification flood.#9244 addressed the timeout case in the local PTY provider, but Windows PTYs are hosted by the terminal daemon, so that change is inert on the default path. This PR fixes the primary (daemon) path.
Fix
Confirm the agent with a cheap, terminal-scoped ConPTY console-membership read (
readWindowsConptyProcessIds, already vendored) instead of the whole-table scan:available: false) no longer retires the identity; an authoritative no-agent scan is confirmed against the console read before retiring (an incomplete snapshot with a child still attached keeps the agent). Scoped to a shell fallback so the deliberate wrapper (npm-after-exit) retirement is unaffected; the membership read stays off the sync foreground read.GetConsoleProcessListcounts the helper's own forked process — a bare shell read as[helper, shell]and never looked shell-only, holding an exited agent's identity forever. The helper's own pid is now dropped before judging membership, so a real exit is surfaced promptly. This fixes both the daemon and local paths.No-op off Windows; never worse than the pre-existing degraded-scan fallback.
Testing
[helper, shell] → {shell},[helper, shell, agent] → {shell, agent}).src/main/daemon+src/main/providerssuites (1319 tests) green;typecheck:node, oxlint, oxfmt clean.Windows validation (real hardware, under CIM-timeout load)
Supersedes the Windows behavior of #9244 (which remains a correct, no-regression improvement to the daemon-unavailable fallback path).
Reliability and performance contract
agent-session.foreground-liveness): A recognized Windows agent remains foreground while evidence is degraded or inconclusive, and retires only after a no-agent scan plus verified shell-only ConPTY membership. Generic wrappers keep their existing retirement behavior.terminal-input.windows-modified-enter-routing; a dedicated completion-notification gate is an accepted manifest gap for this PR.null), verified shell-only membership ({shell}), and attached-child membership ({shell, child...}), so future failures can be classified without treating probe failure as exit proof.Additional review verification: the 22-file reliability gate passed with 1,443 tests; full node/CLI/web typecheck, oxlint, reliability-manifest validation, and max-lines ratchet passed.