Repository navigation
fix: wait for macOS process group reaping before runtime cleanup - #84
Conversation
Walkthrough
The fix keeps polling Darwin
Assessment
Verdict: APPROVE — correct, fail-closed Darwin EPERM→ESRCH handling; one optional test-coverage suggestion, no blocking findings. |
| assert.match(outcome.error ?? '', new RegExp(`process group check failed \\(${code}\\)`)) | ||
| }) | ||
| } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion — packages/core/test/unit/mcp/mcp-runtime-runner.test.ts:104
The rewritten poll loop has one branch the new suite never reaches: a group probe that keeps succeeding (a live member survives) right up to the deadline, which exits through the probeError ?? '…still exists…' fallback at packages/core/src/mcp/mcp-runtime-runner.ts:268. Every new test throws on signal === 0, so the success path and its operator-facing diagnostic are unverified — a future edit could drop that classification without CI noticing. Optional and non-blocking; the case below is verified to pass on this head.
| }) | |
| it('keeps polling a surviving group and reports it as still existing at the deadline', async (t) => { | |
| t.mock.method(process, 'kill', () => true) // the group keeps answering: a member survives | |
| const outcome = await cancelManagedTree({ pid: 1234 }, Promise.resolve(), { | |
| platform: 'darwin', | |
| confirmMs: 50, | |
| }) | |
| assert.deepStrictEqual(outcome, { | |
| confirmed: false, | |
| error: 'managed npm process group still exists after the confirmation deadline', | |
| }) | |
| }) | |
| }) |
🤖 Ask ns-control-tower to fix this
@ns-control-tower please fix: add the surviving-group probe case so the still exists after the confirmation deadline fallback in cancelManagedTree has test coverage.
brycebaril
left a comment
brycebaril
left a comment
There was a problem hiding this comment.
Thanks, this is the macOS fix from #83, and its code and tests match what I reviewed there. On macOS, probing a process group whose members are all zombies returns EPERM until they're reaped. The fix keeps polling within the same deadline, and only ESRCH confirms. A persistent EPERM keeps its diagnostic. The tests cover transient and persistent EPERM, and other platforms' errors.
It fixes the macOS failure that also hit main on 10-26 and #78, so it can merge now.
On macOS, a killed npm process group can still contain unreaped zombies. A signal-0 group probe returns
EPERMduring that interval, so runtime setup prematurely reports unconfirmed termination and retains staging.Keep polling Darwin
EPERMwithin the existing confirmation deadline. OnlyESRCHconfirms that the managed group is gone; persistent probe errors retain staging and preserve their diagnostic. Other Unix probe errors and the Windows termination path keep their behavior. The tests cover transient and persistentEPERM, other-platform errors, and the emitted signal targets.Separated from #83 so this runtime fix can merge independently of the matching N|Solid Console MCP server release.
Validation: 738 tests pass locally, plus lint, build and marketplace installation. Native Claude
/review highandcodex reviewcompleted on the standalone change; findings were independently checked. A minor optional test-coverage suggestion is recorded. CI runs on Linux, macOS and Windows.