Repository navigation
fix(runner): prevent Windows process probes from blocking session lifecycle - #2035
techotaku39 wants to merge 8 commits into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review mode: initial
Requirement — Pass
Keeping Windows Runner responsive during discovery cleanup and session archive is justified. Existing synchronous process probes multiply registry-scan costs and can delay lifecycle requests.
Evidence
- PR description identifies session-started timeouts and archive RPC deadlines.
- Base cli/src/runner/run.ts:1124 filters every registry record through runtimeMayBeAlive; cli/src/utils/process.ts:75 performs synchronous tasklist/CIM generation probes.
Approach — Pass
A fresh process snapshot removes per-record Windows probes. Yielding between tree probes allows queued control work, while generation-aware verification avoids treating reused PIDs as surviving session processes.
Evidence
- cli/src/runner/run.ts:1124 reads registry records before obtaining a fresh snapshot and preserves conservative handling of scan failures.
- cli/src/utils/process.ts:436 yields between probes; line 481 accepts a changed known generation as termination evidence while retaining unknown identities.
Code — Reviewed
Reviewed the entire seven-file merge-base diff and surrounding callers. No actionable defects were identified. Runtime behavior was assessed statically without executing PR code.
No reportable code issues found.
Testing
Not run (automation; PR code execution is prohibited).
- Fixed-head CI: integration and windows-codex-mcp succeeded; test remained in progress when checked.
- Added tests cover snapshot filtering, invalid listings, event-loop yielding, unknown generations, PID reuse, and diagnostic callback failures.
- The PR reports 131 focused Windows tests and packaged lifecycle checks; these were not independently rerun.
- git diff --check passed. No PR tests, builds, or scripts were executed.
HAPI Bot
heavygee
added
bug
There was a problem hiding this comment.
Review mode: follow-up after new commits
Requirement — Pass
Keeping Windows Runner responsive during process checks is a justified lifecycle fix.
Evidence
- Issue #1914 identifies synchronous Windows tasklist polling as an event-loop bottleneck.
- The base heartbeat calls isProcessAlive for each tracked PID; cli/src/utils/process.ts:16 shells out synchronously on Windows.
Approach — Pass
A bounded asynchronous heartbeat snapshot addresses repeated blocking probes. Binding guards and conservative handling of failed queries are appropriate.
Evidence
- cli/src/runner/orphanReap.ts:199 performs an asynchronous CIM query with a timeout.
- cli/src/runner/run.ts:1771 captures bindings and rejects observations after binding or webhook-revision changes.
- cli/src/codex/shared/registry.ts:35 supports batched generation checks while retaining unknown identities.
Code — Reviewed
Reviewed the entire 11-file merge-base diff and surrounding callers. One cleanup race remains when a terminated process's PID is reused.
- [Minor] Preserve replacement bindings after generation-aware termination —
cli/src/runner/run.ts:1418
Testing
Not run (automation; PR code execution is prohibited).
- Fixed-head CI: integration and windows-codex-mcp succeeded; test remained in progress.
- git diff --check passed. No PR code, tests, builds or scripts were executed.
- Added tests cover responsiveness, failed snapshots, yielding and PID reuse, but do not cover replacement Runner bindings during stop cleanup. PR-reported Windows acceptance was not independently rerun.
HAPI Bot
There was a problem hiding this comment.
Review mode: follow-up after new commits
Requirement — Pass
Keeping Windows Runner responsive during process checks is a justified session-lifecycle fix.
Evidence
- Issue #1914 identifies synchronous Windows process polling as an event-loop bottleneck.
- Base cli/src/utils/process.ts:16 invokes tasklist synchronously; the base heartbeat repeats liveness checks for tracked PIDs.
Approach — Pass
A bounded asynchronous heartbeat snapshot addresses repeated blocking probes. Conservative failure handling and binding revisions appropriately protect concurrent lifecycle updates.
Evidence
- cli/src/runner/orphanReap.ts:199 performs an asynchronous CIM query with a timeout.
- cli/src/runner/run.ts:1783 captures bindings and rejects observations after binding or webhook-revision changes.
- cli/src/codex/shared/registry.ts:35 supports batched generation checks while retaining unknown identities.
Code — Reviewed
Reviewed the entire 13-file merge-base diff and surrounding callers. The tracked-session cleanup race is addressed, but the persisted-PID fallback remains vulnerable.
- [Minor] Preserve replacement bindings in persisted-PID stop cleanup —
cli/src/utils/process.ts:481
Testing
Not run (automation; PR code execution is prohibited).
- Fixed-head CI: test, integration and windows-codex-mcp succeeded.
- git diff --check passed. No PR code, tests, builds or scripts were executed.
- Added tests cover snapshot failures, responsiveness, generation-aware shutdown and the binding helper, but do not exercise replacement bindings through the persisted-PID stop fallback.
- PR-reported Windows acceptance results were not independently rerun.
HAPI Bot
There was a problem hiding this comment.
Review mode: follow-up after new commits
Requirement — Pass
Keeping Windows Runner responsive during process checks is a justified session-lifecycle fix.
Evidence
- Issue #1914 documents synchronous Windows process polling blocking the event loop.
- Base cli/src/utils/process.ts:16 uses synchronous tasklist probes; the base heartbeat repeats process checks for tracked PIDs.
Approach — Pass
A bounded asynchronous heartbeat snapshot addresses repeated blocking probes. Conservative failure handling and binding guards appropriately protect concurrent lifecycle updates.
Evidence
- cli/src/runner/orphanReap.ts:199 implements an asynchronous Windows query with a timeout.
- cli/src/runner/run.ts:1790 captures bindings and webhook revisions before querying processes.
- cli/src/runner/run.ts:1496 guards persisted-PID cleanup, addressing the previous review finding.
Code — Reviewed
Reviewed the entire 13-file merge-base diff and relevant callers. The previous cleanup finding is resolved, but the new pre-termination yields expose a shared-root termination race.
- [Major] Recheck bindings after yielding and before signalling the wrapper —
cli/src/utils/process.ts:439
Testing
Not run (automation; PR code execution is prohibited).
- Fixed-head CI: integration and windows-codex-mcp succeeded; test remained in progress when inspected.
- git diff --check passed. No PR code, tests, builds, or scripts were executed.
- Added tests cover yielding and binding cleanup separately, but do not cover a sibling webhook invalidating the stop decision during pre-signal marker collection.
- PR-reported Windows acceptance results were not independently rerun.
HAPI Bot
There was a problem hiding this comment.
Review mode: follow-up after new commits
Requirement — Pass
Keeping Windows Runner responsive during process checks is a justified session-lifecycle fix.
Evidence
- Issue #1914 documents synchronous Windows process polling blocking the event loop.
- Base cli/src/utils/process.ts:16 performs synchronous tasklist probes; the base heartbeat repeats process checks for tracked PIDs.
Approach — Pass
A bounded asynchronous heartbeat snapshot addresses repeated blocking probes. Conservative failure handling and binding checks are appropriate, but termination guards need implementation corrections.
Evidence
- cli/src/runner/orphanReap.ts:199 implements an asynchronous Windows query with a timeout.
- cli/src/runner/run.ts:1790 captures bindings and webhook revisions before applying process observations.
- cli/src/utils/process.ts:453 rechecks mapped-stop bindings before signalling.
Code — Reviewed
Reviewed the entire 13-file merge-base diff and relevant callers. The previous mapped-stop race is addressed, but two Windows lifecycle defects remain.
- [Major] Allow expected wrapper exit while reaping surviving descendants —
cli/src/utils/process.ts:464 - [Major] Propagate the binding guard through orphan-sweep termination —
cli/src/utils/process.ts:453
Testing
Not run (automation; PR code execution is prohibited).
- Fixed-head CI: test, integration, and windows-codex-mcp succeeded.
- git diff --check passed.
- No PR code, tests, builds, or scripts were executed. PR-reported Windows acceptance results were not independently rerun.
- Added tests do not cover guard invalidation by expected child-exit cleanup or sibling registration during orphan-sweep termination.
HAPI Bot
There was a problem hiding this comment.
Review mode: follow-up after new commits
Requirement — Pass
Keeping Windows Runner responsive during process checks addresses a supported session-lifecycle problem.
Evidence
- Issue #1914 identifies synchronous Windows process polling as event-loop blocking and recommends asynchronous or batched probes.
- Base cli/src/utils/process.ts:16 uses synchronous tasklist probes; the base heartbeat repeats process checks for tracked PIDs.
Approach — Pass
A bounded asynchronous heartbeat snapshot addresses repeated blocking probes. Conservative failure handling and binding guards are appropriate.
Evidence
- cli/src/runner/orphanReap.ts:199 implements an asynchronous Windows query with a timeout.
- cli/src/runner/run.ts:1800 captures bindings and webhook revisions before applying observations.
- cli/src/runner/run.ts:1201 and :1539 propagate binding guards through both stop-session orphan sweeps.
Code — Reviewed
Reviewed the entire 13-file merge-base diff and relevant callers. Both previous findings are addressed; one additional Windows shutdown regression remains.
- [Major] Preserve synchronous termination for process-exit cleanup —
cli/src/utils/process.ts:443
Testing
Not run (automation; PR code execution is prohibited).
- Fixed-head CI: integration and windows-codex-mcp succeeded; test was still running at the last check.
- The added tests do not cover synchronous process-exit cleanup. The Windows CI job selects only codexMcpProxy.test.ts, excluding the new native heartbeat test.
- git diff --check passed. No PR code, tests, builds, or scripts were executed; PR-reported Windows acceptance results were not independently rerun.
HAPI Bot
There was a problem hiding this comment.
Review mode: follow-up after new commits
Requirement — Pass
Keeping Windows Runner responsive during process checks addresses a supported session-lifecycle problem.
Evidence
- Issue #1914 identifies synchronous Windows process polling as event-loop blocking and recommends asynchronous or batched probes.
- Base cli/src/utils/process.ts:16 uses synchronous tasklist probes, which the base heartbeat repeats for tracked PIDs.
Approach — Pass
A bounded asynchronous heartbeat snapshot addresses repeated blocking probes. Binding guards, conservative observations, and separate synchronous exit cleanup are appropriate.
Evidence
- cli/src/runner/orphanReap.ts:199 implements the asynchronous Windows snapshot with a timeout.
- cli/src/runner/run.ts:1800 captures bindings and webhook revisions before applying observations.
- cli/src/claude/sdk/query.ts:444 uses synchronous termination for process-exit cleanup.
Code — Reviewed
Reviewed all 16 changed files and relevant callers. The previous exit-cleanup finding is addressed; two stop-path regressions remain.
- [Major] Continue descendant cleanup when the wrapper exits before signalling —
cli/src/utils/process.ts:444 - [Minor] Avoid blocking unrelated termination on a failed registry snapshot —
cli/src/runner/run.ts:1162
Testing
Not run (automation; PR code execution is prohibited).
- Fixed-head CI: test, integration, and windows-codex-mcp succeeded.
- Windows CI selects codexMcpProxy.test.ts and does not run the new native heartbeat test.
- The committed tests do not cover wrapper exit before initial signalling or failed registry snapshots during non-Codex termination.
- git diff --check passed. No PR code, tests, builds, or scripts were executed; PR-reported acceptance results were not independently rerun.
HAPI Bot
There was a problem hiding this comment.
Review mode: follow-up after new commits
Requirement — Pass
Keeping Windows Runner responsive during process checks addresses a supported session-lifecycle problem.
Evidence
- Issue #1914 identifies synchronous tasklist polling as event-loop blocking and recommends asynchronous or batched probes.
- Base cli/src/utils/process.ts:16 performs synchronous Windows liveness checks, repeated by the base heartbeat.
Approach — Pass
A bounded asynchronous heartbeat snapshot addresses repeated blocking probes. Conservative failure handling, binding guards, batched registry checks, and synchronous exit cleanup fit the lifecycle requirements.
Evidence
- cli/src/runner/orphanReap.ts:199 implements the asynchronous snapshot with a 10-second timeout.
- cli/src/runner/run.ts:1790 captures bindings and webhook revisions before applying observations.
- cli/src/runner/stopRuntimes.ts:14 retains potentially live registry records after snapshot failure.
- cli/src/claude/sdk/query.ts:441 preserves synchronous process-exit signalling.
Code — Reviewed
Reviewed all 18 changed files and relevant callers. Both previous findings are addressed: captured descendants can be cleaned after pre-signal wrapper exit, and failed registry snapshots no longer prevent unrelated termination. No further actionable defects were established. Runtime behavior was assessed statically.
No reportable code issues found.
Testing
Not run (automation; PR code execution is prohibited).
- Fixed-head CI: integration and windows-codex-mcp succeeded; test remained in progress when checked.
- Windows CI selects codexMcpProxy.test.ts and does not execute the new native Windows heartbeat test.
- New regression tests cover pre-signal wrapper exit, replacement bindings, new siblings, and conservative registry snapshot failure handling.
- git diff --check passed. No PR code, tests, builds, or scripts were executed; PR-reported acceptance results were not independently rerun.
HAPI Bot
This branch has not been deployed
2 participants
Summary
On Windows, synchronous process checks can block Runner long enough for a fresh session's
/session-startedrequest to time out. With 16 tracked sessions, local control requests stalled for over 20 seconds.Startup and archive deadlines are unchanged.
Validation
Windows checks used Bun 1.4.0 and isolated task homes, databases, credentials and ports.
cli/:bunx vitest run --config vitest.heartbeat.local.config.ts— 174 passed, including 38 new tests, native CIM coverage and startup-webhook regression coverage. The task-local config replaces automatic environment setup.bun typecheck,bun run build,bun run --cwd cli build:exe:allinone— passed.Invoke-HapiTaskPlaywright.ps1 -Name windows-lifecycle-pr -Suite Root terminal-wrap-fidelity.spec.ts— 2 passed.bun run gen:fixturesproduced no normalized content changes;bun run test:webthen passed 3,349 tests.bun run test:sharedpassed 327 tests.Invoke-HapiTaskPlaywright.ps1 -Name heartbeat-combined-check -Suite Root --config playwright.heartbeat.local.config.ts— 6 passed, including real SDK-parent exit with no surviving child/tool process, the two stop races, an unrelated tracked process terminating despite a failed registry snapshot (final status remains conservative), heartbeat load, and archive/reopen/sibling replies/mobile rendering.Acceptance harnesses are workspace-local. Mobile checks use emulation; model responses use a local fixture. Test environments were stopped afterward.
Remaining limitation
Standalone Windows archive acceptance can still exceed the Hub's 30-second RPC deadline; this also reproduces on the unchanged parent. The combined build with #2046 passed archive/reopen. One creation attempt concurrent with full-suite validation timed out; its identical serial rerun passed. This does not eliminate every startup or shutdown deadline failure under load.
Related Issues
Refs #1910.
Refs #1914.
Related PRs: #1940 and #2046.
AI Disclosure
Implementation and validation were assisted by OpenAI Codex, using GPT-6.