fix(coding-agent): reap orphaned process trees during Windows worker recovery - #1
Conversation
…recovery On Windows, reapOrphanProcesses only polled journaled orphans and never signaled them. Orphan trees spawned outside the worker's kill-on-close Job Object (autonomous children) survived the worker, so recovery failed after three attempts with "One or more orphaned process trees could not be terminated". Signal each identity-current orphan tree with signalProcessTree (taskkill /F /T) before polling for disappearance, matching the POSIX path's semantics. Add a win32 regression test that plants a journaled, non-Job-assigned orphan, SIGKILLs the worker, and requires recovery to terminate it. fixes PrimeIntellect-ai#917
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26bc2093a8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const child = spawn( | ||
| process.execPath, | ||
| [ |
There was a problem hiding this comment.
Because this test is under test/suite/, manually spawning the CLI bypasses the repository-required test/suite/harness.ts and faux provider setup. Move this process-level test outside the suite tree or extend the harness to support this daemon-recovery scenario so it uses the standard isolated lifecycle and provider configuration.
AGENTS.md reference: AGENTS.md:L32-L32
Useful? React with 👍 / 👎.
| if (isOrphanProcessIdentityCurrent(orphan)) { | ||
| signalProcessTree(orphan.pid, "SIGKILL"); |
There was a problem hiding this comment.
If the journaled process exits after this second identity check but before signalProcessTree starts taskkill.exe, Windows can recycle the PID and the PID-only /F /T /PID invocation will terminate an unrelated process tree. The extra check narrows the race but does not provide the claimed identity safety; hold a process handle across start-ID validation and termination so the PID cannot be reused in between.
Useful? React with 👍 / 👎.
| for (const orphan of remaining) { | ||
| if (isOrphanProcessIdentityCurrent(orphan)) { | ||
| signalProcessTree(orphan.pid, "SIGKILL"); |
There was a problem hiding this comment.
When a crashed worker has multiple live journal entries, this sequential loop invokes the Windows signalProcessTree implementation, which uses spawnSync with a separate five-second timeout for every taskkill.exe call. Because no await occurs until all signals finish, the deadline above does not bound this phase and the supervisor can stop serving every daemon client for roughly five seconds times the orphan count; use asynchronous/concurrent termination or enforce one shared deadline.
Useful? React with 👍 / 👎.
|
Post-review notes from an independent closeout pass (all bounded, none blocking):
|
MiniMax-M3 was explicitly filtered out from the minimax/minimax-cn providers due to an outdated supported-ids allowlist. Added to minimaxDirectSupportedIds and the context window override logic updated so M3 keeps its native 1M context window instead of inheriting M2.7's 204800 limit. Regenerated model catalog. Also fixed pre-existing TS errors in context-overflow and total-tokens tests by updating deprecated gemini-2.0-flash model reference to gemini-2.5-flash.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This reverts commit 3324bd5.
1 participant
Stacked on @WDDGDA's PrimeIntellect-ai#744. This fixes one confirmed gap left in the Windows recovery path, filed upstream as PrimeIntellect-ai#917.
On Windows,
reapOrphanProcessespolls journaled orphan processes but never signals them. Orphans spawned outside the worker's kill-on-close Job Object (autonomous-mode children) survive a worker crash, so recovery burns three attempts waiting for processes nobody is killing, then fails withOne or more orphaned process trees could not be terminated.The win32 path now signals each identity-current orphan with the existing
signalProcessTree(taskkill /F /T /PID, hidden) before polling, the same signal-then-verify shape as the POSIX path. The identity check sits right next to the signal so a recycled PID never gets taskkilled by mistake.Test
test/suite/regressions/917-reap-orphans-windows.test.ts, win32-only, per the repo's regression convention. It boots a real supervisor and resident worker on a named pipe (offline, no API keys), plants a live process journaled as the worker's orphan but outside the Job Object, SIGKILLs the worker, and requires recovery to actually terminate the orphan. Process-start-id checks keep PID reuse from faking a pass.Red before the fix: three
Could not reap orphaned worker resourceslog lines, thenWorker failed after three recovery attempts, orphan still alive. Green after, about 6-8s. One scoping note: the fixture is a single process, so this covers the journal-to-signal path that actually broke; taskkill's tree walk is older utility code and isn't separately exercised here.What I ran
Windows 11 x64, Node 24.18.0, npm 11.12.1, Git Bash at the standard location. Package-root vitest runs per repo AGENTS.md,
PI_NO_LOCAL_LLM=1, provider key env unset, no real keys anywhere.test/daemon-supervisor-process.test.ts: 8 passed, 6 skipped.windows-job-object,bash-close-hang-windows,git-context-windows,clipboard-image): 12/12.npm run check: clean (biome, tsgo, both installer checks, browser smoke).coding-agentsuite on this machine: 87 failed / 3918 passed (87 unique failed tests; 95 raw failure lines counting retries). @FlamesONE measured 137 failing onmainon their Windows 11 box, so this branch plus the fix cuts that by about a third. The 95 raw lines break down as roughly 56 kernel/python provisioning, 28 behavioral (a lot of those are POSIX literals baked into test data, likefile:/tmp/...), 7 environment (hardlink EPERM on the Program Filesnode.exe,/tmpsocket paths), and a few flakes. None intersect this change. The one recovery-adjacent failure,4603-worker-recovery, dies in test setup trying to hardlinkC:\Program Files\nodejs\node.exe, before any supervisor exists.I also ran
install.ps1end to end locally: packed the branch release, served it over a local HTTP endpoint with the base-URL and channel sentinels filled in, installed through Windows PowerShell 5.1. Checksum verified, per-user npm-global install, kernel venv provisioned, andprime-agent --versionprints0.7.0from a fresh shell. Local builds were required for that validation; the AGENTS.md no-build default is about dev loops, which this was not.Checked but not changed
bootstrap.tsdoesrm(venv, { recursive: true })over a mapped executable). That fix belongs to that PR; no overlap here.0ef7b8e) is already in this branch head. Clipboard tests pass.git.exeinstaller idea: not reproducible on this machine (Git Bash is in the standard place), so I didn't patch it blind. Still looks worth doing for nonstandard layouts.status/agentsprobes were running, and worker shutdown loggedFailed to sync '<stdout>': Incorrect function.Flagging in case either rings a bell.There's a CHANGELOG bullet in the diff per repo convention.