Repository navigation
fix: stop Windows agent trees without PowerShell - #1027
small-angel wants to merge 3 commits into
Conversation
Stopping an agent on Windows listed its descendants through PowerShell and Get-CimInstance, which took seconds, so an ACP shutdown often returned before its agent had exited. One Toolhelp snapshot now finds the descendants in milliseconds. The walk only read command lines to spare `orx supervise`, which orx no longer launches, so both platforms drop that case. Assisted-by: claude-opus-5.5
[Medium impact] The changes reach agent shutdown and restart workflows on Windows and Unix. Fix the Windows test's output race and satisfy the required error-reporting rule before merging. FindingsFix with agent prompt### Issue 1
src/local/chat/mod.rs:1891
`ping` shares the PowerShell output pipe because it starts with `-NoNewWindow` and no output redirect. Its opening blank line or banner can arrive before `$p.Id`. The test then parses that line as `u32` and panics before checking shutdown. Redirect the command's output separately so this pipe carries only the PID.
### Issue 2
src/local/chat/mod.rs:1783-1785
The new Windows walk silently drops failures from `OpenProcess` and `GetProcessTimes`, treats every end to enumeration as normal completion, and ignores the result of `TerminateProcess`. This violates the repository requirement to log developer-fixable failures with context and not treat failed inspection as absence. Distinguish expected exits from other failures and log the failing operation, PID, and Windows error. This requirement must be satisfied before merging.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryReplaces the Windows PowerShell tree walk with a Toolhelp snapshot and direct process termination. Unix walks now read only process IDs and parent IDs.
Reviews (1) · Last reviewed commit: "fix: stop Windows agent trees without Po..." · Reviewed by Greptile |
| .args([ | ||
| "-NoProfile", | ||
| "-Command", | ||
| "$p = Start-Process ping -ArgumentList '-n','60','127.0.0.1' -PassThru -NoNewWindow; $p.Id; Wait-Process -Id $p.Id", |
There was a problem hiding this comment.
Windows test reads command output
ping shares the PowerShell output pipe because it starts with -NoNewWindow and no output redirect. Its opening blank line or banner can arrive before $p.Id. The test then parses that line as u32 and panics before checking shutdown. Redirect the command's output separately so this pipe carries only the PID.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/local/chat/mod.rs
Line: 1891
Comment:
**Windows test reads command output**
`ping` shares the PowerShell output pipe because it starts with `-NoNewWindow` and no output redirect. Its opening blank line or banner can arrive before `$p.Id`. The test then parses that line as `u32` and panics before checking shutdown. Redirect the command's output separately so this pipe carries only the PID.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
fixed in 93b8b6a: ping's output now goes to NUL, so the pipe carries only the PID. passed 5/5 on a Windows 11 VM.
| if handle.is_null() { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
Windows cleanup hides failures
The new Windows walk silently drops failures from OpenProcess and GetProcessTimes, treats every end to enumeration as normal completion, and ignores the result of TerminateProcess. This violates the repository requirement to log developer-fixable failures with context and not treat failed inspection as absence. Distinguish expected exits from other failures and log the failing operation, PID, and Windows error. This requirement must be satisfied before merging.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/local/chat/mod.rs
Line: 1783-1785
Comment:
**Windows cleanup hides failures**
The new Windows walk silently drops failures from `OpenProcess` and `GetProcessTimes`, treats every end to enumeration as normal completion, and ignores the result of `TerminateProcess`. This violates the repository requirement to log developer-fixable failures with context and not treat failed inspection as absence. Distinguish expected exits from other failures and log the failing operation, PID, and Windows error. This requirement must be satisfied before merging.
**Context Used:** AGENTS.md ([source](https://github.com/alphaxiv/openresearch/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
fixed in 93b8b6a: open, start-time, enumeration and terminate failures now log the PID and the Windows error. a process that already exited stays quiet (ERROR_INVALID_PARAMETER on open, or a finished wait after a failed terminate).
Assisted-by: claude-opus-5.5
Assisted-by: claude-opus-5.5
Stopping an agent on Windows walked its descendants through PowerShell and
Get-CimInstance, which took seconds on CI runners. The ACP host waits only 5s for an agent to close, sostalled_session_setup_times_out_and_stops_the_processkept failing on Windows (four main pushes on Nov 13, plus #1015). Desktop quit could also exit before the agent was gone. One Toolhelp snapshot plusTerminateProcessnow does it in milliseconds. A process only counts as a child if it started after its parent, since Windows keeps a dead parent's PID on its orphans and can reuse it.The walk only read command lines to spare
orx supervise, which orx no longer launches (arc/experiments.md), so Unix drops that case too and readsps -o pid=,ppid=.On a Windows 11 VM with 8 busy loops pinning the CPU, main failed the ACP test 1 run in 10, while this branch passed 10/10 in about half the time (1.8–2.2s vs 3.8–4.6s per run). Both runs had the CPU pinned by 8 busy loops. Throwaway scenario tests killed every process in a deep tree, a 40-child tree and a parent respawning every 30ms, in 7–78ms. The PowerShell walk took 1–4.7s and let 5 of the respawned children escape. Neither version reaches a command whose launcher already exited (
cmd /c start), as before.Assisted-by: claude-opus-5.5