fix: add Windows platform support for process group killing in ManagedRunner

[whutzefengxie-ops]

Fixes #1284

🐛 Problem

ManagedRunner._killProcessGroup uses Unix-style process.kill(-pid) syntax, which does not work on Windows. This causes child processes (sleep, cmd.exe) to survive after tests complete, leading to zombie processes and system resource leaks.

🔍 Root Cause

• process.kill(-pid) is Unix syntax for killing a process group
• On Windows, negative PIDs are invalid and ignored/rejected
• Only the parent shell (cmd.exe) is killed, child processes survive
• These zombie processes accumulate and can cause system instability (Windows freeze)

🔧 Solution

Added platform-specific process killing logic in packages/api/src/infrastructure/managed-runner.ts:

Windows:

spawn('taskkill', ['/PID', String(this._pid), '/T', forceFlag], {
  shell: true,
  stdio: 'ignore',
  detached: true,
});

• /T flag terminates all child processes in the tree
• /F flag added for SIGKILL equivalent (force termination)

Unix/macOS:

• Existing process.kill(-pid, signal) logic preserved

✅ Testing

• Built successfully on Windows 11
• TypeScript type check passes
• Verified zombie processes are now killed correctly
• No changes to Unix/macOS behavior (existing logic preserved)

📊 Impact

• Severity: High fix (prevents system resource leaks)
• Scope: All Windows developers running tests
• Affected tests: F167 Phase P wakeWhen integration tests
  • test/callback-hold-ball-wakewhen.test.js (T8, T9)
  • test/managed-runner.test.js (T3, T4, R4)

📝 Related

• Issue: [Bug] ManagedRunner leaves zombie processes on Windows - process group kill fails #1284
• Related to F167 Phase P wakeWhen feature implementation

[zts212653]

Exact-HEAD maintainer review — REQUEST CHANGES

Reviewed HEAD: 53f37dfb192f2d414837e7f9e41a45990f2193c6

The linked issue clowder-ai#1284 is now accepted, and the F167 anchor is valid. The one-file direction is useful, but two blocking findings remain.

1. P1 — synchronous taskkill can freeze the entire API event loop without a bound. The new Windows branch calls execFileSync(taskkill, ...) from timeout and user-cancel paths with no timeout. Those paths run inside the API process. If taskkill stalls under endpoint security, process-table pressure, or OS failure, every request and timer in the server is blocked, including the later SIGKILL escalation that is supposed to recover the command. Please use a non-blocking, bounded termination helper and preserve observable failure/timeout handling.

2. P1 — no test proves the changed Windows contract. This PR changes only managed-runner.ts. The green Windows Smoke job runs cli-spawn-win, process-liveness-probe, pick-directory, and desktop config tests; it does not run managed-runner.test.js or verify descendant cleanup. Add a deterministic unit seam for taskkill arguments/failure/timeout and a Windows regression that starts a parent plus descendant, cancels or times out the runner, and proves the descendant is gone. Preserve the existing SIGTERM → grace → SIGKILL semantics.

Required gate: Lint is currently red because Biome rejects the ternary formatting in this file. Fix it and rerun all current checks.

Five-question summary: the bug benefits the home and public target; the actual change is a platform-specific tree-kill branch; it is worth merging only after the findings above; intake would be absorbed + manual-port because this F167 infrastructure file has continued to evolve at home; the cleaner slice is a bounded platform adapter plus direct regression coverage, not a synchronous OS call in the server hot path.

The external author whutzefengxie-ops retains fix custody. I did not modify or merge the branch.

[小太阳·砚砚/GPT-5.6 Sol🐾]

[zts212653]

Review continuity — blockers unchanged on new HEAD

GitHub advanced the branch to a8d7752ae416b1ec3213b2d395c00054ab24ac94 via a merge from current main. I re-read the resulting PR diff: it still changes only managed-runner.ts, still uses unbounded execFileSync(taskkill, ...), and still adds no managed-runner or Windows descendant-cleanup test.

Therefore the exact findings from review pullrequestreview-4852216497 continue to block this new HEAD by content continuity. The new CI run is useful but cannot replace the requested implementation and regression changes. Please push a substantive author delta that closes both P1s before requesting another review.

[小太阳·砚砚/GPT-5.6 Sol🐾]

[zts212653]

CI follow-up — exact HEAD a8d7752ae416b1ec3213b2d395c00054ab24ac94

I re-read the failing GitHub Actions job 30892495759 / 91937610032. The single Lint failure is deterministic and source-owned: Biome rejects the multiline ternary at packages/api/src/infrastructure/managed-runner.ts:310 and would print it on one line.

Please run:

pnpm biome check --write packages/api/src/infrastructure/managed-runner.ts

Then rerun pnpm check before pushing. This formatting fix is necessary, but it does not clear the two existing P1 findings: replace the unbounded synchronous execFileSync(taskkill, ...) path with bounded non-blocking termination, and add deterministic unit plus real Windows descendant-cleanup regression coverage. Once all three are in a new author HEAD, the tracker will trigger fresh formal review.

The external author retains branch custody; no maintainer fixup was applied.

[小太阳·砚砚/GPT-5.6 Sol🐾]

[zts212653]

Strategy B authorization — branch update only

CVO authorization provenance: owner-thread message 0001786246354374-000615-98211283.

Scope: maintainer may update this contributor branch onto current clowder-ai main and resolve only mechanical conflicts. Existing review findings remain author-owned; this update will not silently implement or clear them. Any semantic conflict or scope expansion stops and returns to the author.

[小太阳·砚砚/GPT-5.6 Sol🐾]

[zts212653]

Strategy B branch update completed.

• old HEAD: a8d7752
• new HEAD: 94a600d
• rebased onto main: 927bec8
• continuity: range-diff is patch-equivalent; the PR remains a one-file ManagedRunner change
• local check: git diff --check passed

This update does not clear the existing author-owned findings. Current CI already confirms Lint still fails; the synchronous taskkill blocking risk and deterministic Windows descendant-cleanup test gap also remain for TokenFelix to fix on a fresh HEAD.

[小太阳·砚砚/GPT-5.6 Sol🐾]

[zts212653]

Exact-HEAD maintainer review — REQUEST CHANGES

Reviewed HEAD: e6fd426

This HEAD is a merge of main into 94a600d. Its only new files are package.json, scripts/lib/node-runtime-guard.sh, and scripts/node-runtime-guard.test.mjs from main; the PR diff remains the same one-file ManagedRunner patch. Therefore neither blocking finding is fixed:

1. P1 — packages/api/src/infrastructure/managed-runner.ts still calls unbounded synchronous execFileSync(taskkill, ...) from API timeout/cancel paths. A stalled taskkill can block the event loop and prevent the later recovery path. Replace it with a bounded non-blocking termination helper with observable failure/timeout handling.

2. P1 — the PR still adds no managed-runner or Windows descendant-cleanup tests. Add a deterministic seam covering taskkill arguments, failure, and timeout, plus a Windows regression that starts a parent and descendant, cancels or times out the runner, and proves the descendant is gone while preserving SIGTERM → grace → SIGKILL semantics.

Current CI is still running and cannot substitute for these missing code and regression changes. Please push a substantive author HEAD after resolving both findings.

The external author whutzefengxie-ops retains implementation custody; I did not modify or merge the branch.

[小太阳·砚砚/GPT-5.6 Sol🐾]