[PR #4268] [CLOSED] fix(dev): terminate the owned command on Windows cancellation (#4258) #6148

Closed
opened 2026-10-31 01:12:14 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4268
Author: @DeryFerd
Created: 10/15/2026
Status: ❌ Closed

Base: main ← Head: fix/4258-windows-runner-terminates-command


📝 Commits (1)

  • 9a2e595 fix(dev): terminate the owned command on Windows cancellation

📊 Changes

2 files changed (+118 additions, -1 deletions)

View changed files

📝 scripts/run-exclusive-command.mjs (+9 -1)
📝 tests/process-global/runnerCancellation.test.ts (+109 -0)

📄 Description

Summary

The exclusive-command supervisor recorded IPC cancellation and parent disconnection on Windows but returned without stopping the command it directly owns. It then waited for the command's close event while retaining the SQLite transaction, so a command that does not exit on its own kept the exclusive lock after the parent requested shutdown. The supervisor now terminates the owned ChildProcess through its handle on win32, matching the existing POSIX process-group behaviour while keeping the transaction until the command's exit is observed.

Fixes #4258.

Problem and expected behavior

The smallest trigger is an owned command that ignores cancellation and keeps running. The supervisor receives a forwarded SIGTERM (or loses its IPC parent) and records the shutdown, but on Windows handleSignal returned immediately and never signalled the command. run() awaits runCommand(), which only settles on the command's close event, so the process stayed alive holding the exclusive lock. The POSIX path already killed its process group and exited 143.

The violated invariant is that a requested cancellation or a lost parent must stop the directly owned command, observe its exit, and then release the lock. The expected behavior is that both cancellation routes terminate the command and the supervisor exits with the signal's code once the command is gone.

Change and scope

scripts/run-exclusive-command.mjs no longer returns early on win32. It terminates the owned command through the ChildProcess handle it already holds (child.kill(signal)) and keeps the transaction open until runCommand() observes the close event, so the lock lifetime is unchanged. The POSIX branch keeps signalling its own process group with the existing bounded SIGKILL escalation.

The change deliberately uses the already-owned child handle rather than an unverified PID or a process-tree kill, as the report requests. It does not alter command resolution, argument handling, lock acquisition, or the POSIX path.

Contract and boundary impact

  • Semantic owner and earliest changed stage: exclusive-command supervision in scripts/run-exclusive-command.mjs; the earliest changed stage is the Windows shutdown handler.
  • Contribution scope fit and linked maintainer decision: ordinary defect fix within existing development-runner ownership; a separate maintainer scope decision is not applicable.
  • CLI and MCP/tool-catalog contract: none; this is an internal development command runner.
  • Provider, bridge, target-format, or platform compatibility: Windows-only shutdown behaviour; the POSIX branch is unchanged and continues to skip on Windows.
  • Evidence, artifact, provenance, or reconstruction contract: none.
  • Target selection, authority, upstream behavior, and cleanup details: the command is terminated through its owned handle, never a bare PID; the SQLite transaction still releases only after the observed exit.
  • Generated metadata (docs/public/product-catalog.json), package, or installation impact: none.

Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible.

  • Read existing files or captured data.
  • Launch or attach to a process.
  • Inject instrumentation or run scripts/code.
  • Change process memory, registers, files, or configuration.
  • Contact network services.
  • Recover protected code or work around protections.
  • Documentation or usability only; no target interaction.
  • Not sure yet; investigation or maintainer guidance needed.

Evidence and regression coverage

  • Tests added or updated: tests/process-global/runnerCancellation.test.ts gains two Windows-only cases. One forwards SIGTERM over the supervisor's IPC channel and asserts the owned command's PID is no longer signallable and the supervisor exits 143. The other drops the IPC channel and asserts the same. Both read the command PID from a file the gated command writes, and resolve on whichever of exit/close fires first, because Node emits exit but not close after the parent disconnects.
  • Base reproduction or other evidence: reproduced on a real Windows 11 x64 host (Node 24.21.0). After an IPC SIGTERM and after disconnect(), the owned command stayed alive; a direct child.kill("SIGTERM") on the owned handle terminated it. This confirms the branch defect on native Windows rather than only by forcing the branch.
  • User-visible CLI/MCP output (if applicable): the supervisor exits 143 after cancellation instead of hanging, so the exclusive lock is released.
  • Remaining proof gaps: native descendant behaviour on Windows and a real Windows SQLite contender were not exercised; the report leaves both as NOT_ENOUGH_INFO. The POSIX cancellation cases remain skipped on Windows and were not run here.

For evidence-bearing changes:

  • Observed, derived, and inferred claims remain distinguishable.
  • Artifact identity, source provenance, and failed attempts remain preserved.
  • Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

  • npx vitest run tests/process-global/runnerCancellation.test.ts --project process-global — 6 passed, 3 skipped (the POSIX-only cases skip on Windows).
  • Red-green check — reverting scripts/run-exclusive-command.mjs makes both new Windows cases fail with supervisor did not finish; restoring it makes them pass.
  • npm run verify:module-boundaries — 1949 source files checked.
  • npx oxlint scripts/run-exclusive-command.mjs tests/process-global/runnerCancellation.test.ts — 0 errors.
  • npx oxfmt --check on both changed files — format clean.
  • Real Windows x64 shutdown boundary — IPC SIGTERM and parent disconnect each left the command alive before the fix and terminated it after.

Compatibility, safety, and release

  • Breaking changes or migration steps: none.
  • Real Hopper/Ghidra, browser, or OS coverage: real Windows x64 process-termination boundary exercised; no provider or engine behaviour changed.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: the command is terminated through the ChildProcess handle the supervisor already owns, not an unverified PID or a process-tree kill, and the SQLite transaction still releases only after the observed exit.

Author readiness

Human owner: @DeryFerd

  • I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
  • I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
  • I declare this PR ready for initial maintainer review.

Before merge

  • This PR addresses a concrete problem or an agreed enhancement.
  • Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
  • The PR has one focused outcome and the title follows type(scope): outcome.
  • Related issue is linked, or the reason for not linking one is stated above.
  • Tests cover changed observable behavior and meaningful failure paths.
  • Owning docs, contracts, and generated metadata are updated where needed.
  • User-visible CLI/MCP changes include representative output.
  • I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/4268 **Author:** [@DeryFerd](https://github.com/DeryFerd) **Created:** 10/15/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/4258-windows-runner-terminates-command` --- ### 📝 Commits (1) - [`9a2e595`](https://github.com/morluto/rea/commit/9a2e5953ef5083b523b46e83576d3f320aa2967c) fix(dev): terminate the owned command on Windows cancellation ### 📊 Changes **2 files changed** (+118 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `scripts/run-exclusive-command.mjs` (+9 -1) 📝 `tests/process-global/runnerCancellation.test.ts` (+109 -0) </details> ### 📄 Description ## Summary The exclusive-command supervisor recorded IPC cancellation and parent disconnection on Windows but returned without stopping the command it directly owns. It then waited for the command's `close` event while retaining the SQLite transaction, so a command that does not exit on its own kept the exclusive lock after the parent requested shutdown. The supervisor now terminates the owned `ChildProcess` through its handle on `win32`, matching the existing POSIX process-group behaviour while keeping the transaction until the command's exit is observed. Fixes #4258. ## Problem and expected behavior The smallest trigger is an owned command that ignores cancellation and keeps running. The supervisor receives a forwarded `SIGTERM` (or loses its IPC parent) and records the shutdown, but on Windows `handleSignal` returned immediately and never signalled the command. `run()` awaits `runCommand()`, which only settles on the command's `close` event, so the process stayed alive holding the exclusive lock. The POSIX path already killed its process group and exited 143. The violated invariant is that a requested cancellation or a lost parent must stop the directly owned command, observe its exit, and then release the lock. The expected behavior is that both cancellation routes terminate the command and the supervisor exits with the signal's code once the command is gone. ## Change and scope `scripts/run-exclusive-command.mjs` no longer returns early on `win32`. It terminates the owned command through the `ChildProcess` handle it already holds (`child.kill(signal)`) and keeps the transaction open until `runCommand()` observes the `close` event, so the lock lifetime is unchanged. The POSIX branch keeps signalling its own process group with the existing bounded `SIGKILL` escalation. The change deliberately uses the already-owned child handle rather than an unverified PID or a process-tree kill, as the report requests. It does not alter command resolution, argument handling, lock acquisition, or the POSIX path. ## Contract and boundary impact - Semantic owner and earliest changed stage: exclusive-command supervision in `scripts/run-exclusive-command.mjs`; the earliest changed stage is the Windows shutdown handler. - Contribution scope fit and linked maintainer decision: ordinary defect fix within existing development-runner ownership; a separate maintainer scope decision is not applicable. - CLI and MCP/tool-catalog contract: none; this is an internal development command runner. - Provider, bridge, target-format, or platform compatibility: Windows-only shutdown behaviour; the POSIX branch is unchanged and continues to skip on Windows. - Evidence, artifact, provenance, or reconstruction contract: none. - Target selection, authority, upstream behavior, and cleanup details: the command is terminated through its owned handle, never a bare PID; the SQLite transaction still releases only after the observed exit. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact: none. Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible. - [x] Read existing files or captured data. - [x] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [ ] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. ## Evidence and regression coverage - Tests added or updated: `tests/process-global/runnerCancellation.test.ts` gains two Windows-only cases. One forwards `SIGTERM` over the supervisor's IPC channel and asserts the owned command's PID is no longer signallable and the supervisor exits 143. The other drops the IPC channel and asserts the same. Both read the command PID from a file the gated command writes, and resolve on whichever of `exit`/`close` fires first, because Node emits `exit` but not `close` after the parent disconnects. - Base reproduction or other evidence: reproduced on a real Windows 11 x64 host (Node 24.21.0). After an IPC `SIGTERM` and after `disconnect()`, the owned command stayed alive; a direct `child.kill("SIGTERM")` on the owned handle terminated it. This confirms the branch defect on native Windows rather than only by forcing the branch. - User-visible CLI/MCP output (if applicable): the supervisor exits 143 after cancellation instead of hanging, so the exclusive lock is released. - Remaining proof gaps: native descendant behaviour on Windows and a real Windows SQLite contender were not exercised; the report leaves both as `NOT_ENOUGH_INFO`. The POSIX cancellation cases remain skipped on Windows and were not run here. For evidence-bearing changes: - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed - `npx vitest run tests/process-global/runnerCancellation.test.ts --project process-global` — 6 passed, 3 skipped (the POSIX-only cases skip on Windows). - Red-green check — reverting `scripts/run-exclusive-command.mjs` makes both new Windows cases fail with `supervisor did not finish`; restoring it makes them pass. - `npm run verify:module-boundaries` — 1949 source files checked. - `npx oxlint scripts/run-exclusive-command.mjs tests/process-global/runnerCancellation.test.ts` — 0 errors. - `npx oxfmt --check` on both changed files — format clean. - Real Windows x64 shutdown boundary — IPC `SIGTERM` and parent disconnect each left the command alive before the fix and terminated it after. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: real Windows x64 process-termination boundary exercised; no provider or engine behaviour changed. - Package or release metadata impact: none. - Security, privacy, process, or containment review: the command is terminated through the `ChildProcess` handle the supervisor already owns, not an unverified PID or a process-tree kill, and the SQLite transaction still releases only after the observed exit. ## Author readiness Human owner: @DeryFerd - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. ## Before merge - [ ] This PR addresses a concrete problem or an agreed enhancement. - [ ] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [ ] The PR has one focused outcome and the title follows `type(scope): outcome`. - [ ] Related issue is linked, or the reason for not linking one is stated above. - [ ] Tests cover changed observable behavior and meaningful failure paths. - [ ] Owning docs, contracts, and generated metadata are updated where needed. - [ ] User-visible CLI/MCP changes include representative output. - [ ] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
bsakaguchi 2026-10-31 01:12:14 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
morluto/rea#6148
No description provided.