fix(flows): kill shell children on interrupt and node timeout

[SebTardif]

What Problem This Solves

Fixes an issue where users running acpx flow / FlowRunner.run with a shell() node would leave the spawned command running after Ctrl+C or after an outer node timeout. runShellAction only sent SIGTERM/SIGKILL on its own timeoutMs. FlowRunner.run persisted the interrupted bundle and rejected; the CLI then process.exited and orphaned the child. The same orphan happens when the node timer fires (timeoutMs: 200) while the action sets execution.timeoutMs: 0, so the inner kill timer never starts.

Why This Change Was Made

runShellAction now accepts an AbortSignal. Abort and the existing inner timeout share the same SIGTERM then SIGKILL grace (1s). FlowRunner.run aborts that signal on SIGINT/SIGTERM/SIGHUP. executeActionNode also aborts it from runWithHeartbeat's onTimeout, matching how ACP nodes cancel a session on node timeout.

This is not stdin EPIPE handling (open #529). It is not output bounding. It does not change successful shell completion.

The missing abort dates to #179 (697ee1f, 2026-03-26, "feat: add experimental acpx flows runtime and examples"), 156 days on main. #188 persisted interrupted runs and still did not reap the spawn.

User Impact

A flow shell() child is reaped when the operator interrupts the run or when the node timeout fires, including when the action sets timeoutMs: 0. The host still reports TimeoutError / InterruptedError. Operators no longer leak long-running sleep / compiler / yes processes after the CLI has exited.

Evidence

terminal output from live node against compiled public defineFlow / shell() / FlowRunner (dist/flows.js).

Same scripts: one-node flow whose exec returns command: process.execPath, args that write process.pid then setInterval, and timeoutMs: 0. Outer node timeout is 250ms. Interrupt script starts the same flow in a child host, then sends SIGINT.

Before, on unpatched src/flows/executors/shell.ts and src/flows/runtime.ts compiled to dist, the host rejects but the child stays alive:

$ ACPX_ROOT=/tmp/oc-pr-acpx-F004 node /tmp/acpx-F004-proof.mjs
{
  "hostAlive": true,
  "hostPid": 56463,
  "status": "rejected",
  "errorName": "TimeoutError",
  "errorMessage": "Timed out after 250ms",
  "childPid": 56471,
  "childAliveAfterTimeout": true
}

$ SIGINT to FlowRunner host after the shell child wrote its pid
{
  "hostPid": 56490,
  "hostExit": 130,
  "childPid": 56499,
  "childAliveBeforeSigint": true,
  "childAliveAfterHostExit": true
}

After, on the patched compiled public API, the same flows reject and the child is gone:

$ ACPX_ROOT=/tmp/oc-pr-acpx-F004 node /tmp/acpx-F004-proof.mjs
{
  "hostAlive": true,
  "hostPid": 56583,
  "status": "rejected",
  "errorName": "TimeoutError",
  "errorMessage": "Timed out after 250ms",
  "childPid": 56590,
  "childAliveAfterTimeout": false
}

$ SIGINT to FlowRunner host after the shell child wrote its pid
{
  "hostPid": 56619,
  "hostExit": 130,
  "childPid": 56627,
  "childAliveBeforeSigint": true,
  "childAliveAfterHostExit": false
}

Real behavior proof

• Behavior or issue addressed: Flow shell() children stayed running after Ctrl+C and after an outer node timeout when execution.timeoutMs was 0, so process.exit orphaned the spawn.

• Real environment tested: macOS, Node v26.7.0, acpx compiled from this branch at /tmp/oc-pr-acpx-F004 (dist/flows.js).

• Exact steps or command run after this patch:

  ACPX_ROOT=/tmp/oc-pr-acpx-F004 node /tmp/acpx-F004-proof.mjs
  ACPX_ROOT=/tmp/oc-pr-acpx-F004 ACPX_F004_PID=/tmp/acpx-f004-int.pid node /tmp/acpx-F004-interrupt-host.mjs

  The timeout script imports defineFlow, shell, and FlowRunner from compiled dist/flows.js, then runs a one-node flow with node timeoutMs: 250 and action timeoutMs: 0. The interrupt host runs the same long child; the parent sends SIGINT after the pid file appears.

• Evidence after fix: terminal output from the patched compiled public API (shown above). Timeout path prints childAliveAfterTimeout: false. Interrupt path prints childAliveAfterHostExit: false and host exit 130.

• Observed result after fix: The same pid-writing setInterval child that stayed alive after TimeoutError and after SIGINT on unpatched dist is now gone once the host rejects. The host still surfaces TimeoutError / InterruptedError.

• What was not tested: Windows cmd.exe spawn, and a child that ignores SIGTERM for the full 1s grace before SIGKILL when the CLI process.exits immediately.

Related: #179 (origin), #188 (interrupt persist only), #529 (stdin EPIPE, different file), #501, #498. Same leftover-child class as merged process-list timeouts in #501.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

[clawsweeper]

Codex review: needs changes before merge. Reviewed August 29, 2026, 9:27 PM ET / August 30, 2026, 01:27 UTC.

ClawSweeper review

What this changes

Adds abort propagation from flow interrupts and node timeouts to shell actions, with direct-child termination tests.

Merge readiness

⛔ Blocked by patch quality or review findings - 8 items remain

Keep open: direct-child cancellation is proven, but the two previously reported P1 teardown gaps remain, so SIGTERM-resistant commands and descendants of shell wrappers can still survive an interrupted CLI.

Priority: P1
Reviewed head: e3e1ada0c300756b32ff3c900474575695d42853

Review scores

Measure	Result	What it means
Overall readiness	🦪 silver shellfish (2/6)	The direct public-API proof is useful, but two P1 cleanup defects leave the advertised process-reaping guarantee incomplete.
Proof confidence	🐚 platinum hermit (4/6)	Sufficient (terminal): The PR body supplies before/after terminal traces from compiled dist/flows.js using public FlowRunner and shell entrypoints: a direct pid-writing child is absent after both an outer timeout and SIGINT on the patched branch. That proves the direct-child recovery, while the findings identify unproven resistant-child and descendant cases.
Patch quality	🦪 silver shellfish (2/6)	3 actionable review findings remain.

Verification

Check	Result	Evidence
Real behavior	Verified	Sufficient (terminal): The PR body supplies before/after terminal traces from compiled dist/flows.js using public FlowRunner and shell entrypoints: a direct pid-writing child is absent after both an outer timeout and SIGINT on the patched branch. That proves the direct-child recovery, while the findings identify unproven resistant-child and descendant cases.
Evidence reviewed	8 items	Forced-kill timer is not retained: The introduced termination helper sends SIGTERM, then schedules SIGKILL with unref(); a SIGTERM-resistant child therefore has no retained cleanup boundary. CLI exits immediately after interruption: Once FlowRunner rejects with InterruptedError, the CLI calls process.exit(130), which can end the host before the unref'ed SIGKILL callback runs. Shell wrappers are supported but not reaped as trees: Shell actions explicitly accept shell: true or a shell path, while the introduced helper signals only the ChildProcess wrapper; a descendant can outlive a killed wrapper.
Findings	3 actionable findings	[P1] Keep forced-kill cleanup alive through CLI shutdown [P1] Terminate shell process trees rather than only wrappers [P3] Document the user-visible flow cleanup fix
Security	None	None.

How this fits together

Flows execute user-authored graphs containing runtime-owned shell actions and persist run state as they progress. Interrupts and node timers must stop the command being run before the CLI reports the failed run and exits.

flowchart TD
  A[Flow definition] --> B[Flow runner]
  B --> C[Shell action executor]
  D[Interrupt or node timeout] --> E[Cancellation signal]
  E --> C
  C --> F[Spawned command tree]
  F --> G[Run result and stored bundle]

Loading

Before merge

• Keep forced-kill cleanup alive through CLI shutdown (P1) - The new SIGKILL timer is unref()ed. After withInterrupt rejects, src/cli-core.ts immediately calls process.exit(130), so a child that ignores or handles SIGTERM is orphaned before the escalation runs. Retain and await a teardown completion boundary, then cover CLI interruption with a SIGTERM-resistant child.
• Terminate shell process trees rather than only wrappers (P1) - ShellActionExecution supports shell: true, but this helper only signals the wrapper ChildProcess. A shell or command that forks can exit on SIGTERM while its descendant continues; the later SIGKILL then targets the dead wrapper. Use process-group/tree termination and cover a wrapper-plus-descendant case.
• Document the user-visible flow cleanup fix (P3) - Late review note: this PR changes the observable behavior of interrupted flow runs, yet it adds no Unreleased Fixes entry. Add the required concise changelog entry.
• Resolve merge risk (P1) - Merging would leave interrupted CLI flows able to orphan SIGTERM-resistant commands or descendants of shell-mode wrappers, contrary to the stated cleanup guarantee.
• Complete next step (P2) - The P1 defects have a bounded repair path in the shell executor and targeted flow/CLI regressions.
• Improve patch quality - Retain and await forced-kill escalation so CLI interruption cannot exit before teardown.
• Improve patch quality - Terminate process groups or trees and add a shell-wrapper descendant regression.
• Improve patch quality - Add the required Unreleased changelog entry.

Findings

• [P1] Keep forced-kill cleanup alive through CLI shutdown — src/flows/executors/shell.ts:9-11
• [P1] Terminate shell process trees rather than only wrappers — src/flows/executors/shell.ts:7-11
• [P3] Document the user-visible flow cleanup fix — src/flows/runtime.ts:222-223

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Production versus test delta	production +306/-136, tests +157/-1	The cancellation refactor is nearly twice as large in production code as in test additions, so the missing shutdown and descendant cases need focused coverage.

Root-cause cluster

Relationship: canonical
Canonical: #530
Summary: This PR is the canonical open attempt to fix flow shell-action interruption cleanup; the nearby flow PRs address separate stdin and output-buffer defects.

Members:

• same_root_cause: feat: add experimental acpx flows runtime and examples #179 - Supplied history identifies the merged flow-runtime introduction as the origin of the missing shell-action cleanup.
• adjacent_distinct: fix(flows): ignore EPIPE on shell action stdin #529 - It fixes shell-action stdin EPIPE handling rather than interruption cleanup.
• adjacent_distinct: fix(flows): cap captured shell stdout and stderr #531 - It bounds captured shell output rather than terminating interrupted commands.

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge-risk options

Maintainer options:

1. Complete command-tree teardown (recommended)
   Retain and await SIGKILL escalation through CLI shutdown, terminate shell process groups or trees on supported platforms, and add focused resistant-child and descendant regressions.

Technical review

Best possible solution:

Use a tracked, awaited platform-aware process-group/tree teardown for shell actions and prove it through CLI interruption before merging.

Do we have a high-confidence way to reproduce the issue?

Yes — the supplied compiled-public-API before/after terminal traces reproduce the direct-child timeout and interrupt leak against the PR base, and the source confirms the relevant cancellation path.

Is this the best way to solve the issue?

No — abort propagation fixes the direct-child case, but wrapper-only signalling and an unref'ed escalation timer do not guarantee cleanup when the CLI exits.

Full review comments:

• [P1] Keep forced-kill cleanup alive through CLI shutdown — src/flows/executors/shell.ts:9-11
  The new SIGKILL timer is unref()ed. After withInterrupt rejects, src/cli-core.ts immediately calls process.exit(130), so a child that ignores or handles SIGTERM is orphaned before the escalation runs. Retain and await a teardown completion boundary, then cover CLI interruption with a SIGTERM-resistant child.
  Confidence: 0.99
• [P1] Terminate shell process trees rather than only wrappers — src/flows/executors/shell.ts:7-11
ShellActionExecution supports shell: true, but this helper only signals the wrapper ChildProcess. A shell or command that forks can exit on SIGTERM while its descendant continues; the later SIGKILL then targets the dead wrapper. Use process-group/tree termination and cover a wrapper-plus-descendant case.
  Confidence: 0.97
• [P3] Document the user-visible flow cleanup fix — src/flows/runtime.ts:222-223
  Late review note: this PR changes the observable behavior of interrupted flow runs, yet it adds no Unreleased Fixes entry. Add the required concise changelog entry.
  Confidence: 0.98
  Late finding: first raised on code an earlier review cycle already covered.

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against ef6b81e71756.

Labels

Label justifications:

• P1: Interrupted flow commands can remain running after the CLI exits, affecting active workflow execution.
• merge-risk: 🚨 availability: The introduced cleanup path can leave long-running descendant processes alive after cancellation.
• rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🐚 platinum hermit and patch quality is 🦪 silver shellfish.
• status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The PR body supplies before/after terminal traces from compiled dist/flows.js using public FlowRunner and shell entrypoints: a direct pid-writing child is absent after both an outer timeout and SIGINT on the patched branch. That proves the direct-child recovery, while the findings identify unproven resistant-child and descendant cases.
• proof: sufficient: Contributor real behavior proof is sufficient. The PR body supplies before/after terminal traces from compiled dist/flows.js using public FlowRunner and shell entrypoints: a direct pid-writing child is absent after both an outer timeout and SIGINT on the patched branch. That proves the direct-child recovery, while the findings identify unproven resistant-child and descendant cases.

Evidence

Acceptance criteria:

• [P1] pnpm run test.
• [P1] pnpm run check.

What I checked:

• Forced-kill timer is not retained: The introduced termination helper sends SIGTERM, then schedules SIGKILL with unref(); a SIGTERM-resistant child therefore has no retained cleanup boundary. (src/flows/executors/shell.ts:11, e3e1ada0c300)
• CLI exits immediately after interruption: Once FlowRunner rejects with InterruptedError, the CLI calls process.exit(130), which can end the host before the unref'ed SIGKILL callback runs. (src/cli-core.ts:511, e3e1ada0c300)
• Shell wrappers are supported but not reaped as trees: Shell actions explicitly accept shell: true or a shell path, while the introduced helper signals only the ChildProcess wrapper; a descendant can outlive a killed wrapper. (src/flows/types.ts:81, e3e1ada0c300)
• Existing process-tree precedent: The ACP terminal manager uses platform-aware POSIX process-group signalling and Windows tree termination, then waits for tracked descendants; this is the relevant established cleanup shape. (src/acp/terminal-manager.ts:476, e3e1ada0c300)
• Added tests cover only direct children: The new tests spawn process.execPath directly and wait for it to die; they do not exercise a SIGTERM-resistant child through CLI exit or a shell wrapper that forks a descendant. (test/flows-shell.test.ts:106, e3e1ada0c300)
• Prior blockers remain at this head: The previous completed review identified both the unref'ed SIGKILL and wrapper-only termination. The follow-up commit changes runtime state handling and tests, not the shell executor. (src/flows/executors/shell.ts:7, e3e1ada0c300)

Likely related people:

• osolmaz: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
• SebTardif: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (1 earlier review cycle)

• reviewed 2026-08-29T18:45:14.480Z sha 6721547 :: needs changes before merge. :: [P1] Keep the forced-kill timer alive through CLI shutdown | [P1] Terminate the shell process tree instead of only its wrapper

[steipete]

Maintainer triage: CLOSE recommended for the current implementation. Independent built-CLI proof confirms that interruption still leaks both resistant direct children and shell descendants.

The proof launched a real flow through this PR's dist/cli.js, waited for a Node child to write its PID, sent SIGINT to the CLI, and checked the recorded processes after 1600 ms:

resistant-direct-child:
  CLI exit=130
  childAliveAfterGrace=true
shell-wrapper-descendant:
  CLI exit=130
  wrapperAliveAfterGrace=false
  childAliveAfterGrace=true

Environment: macOS, Node 26.8.1, isolated synthetic HOME/cwd/TMPDIR, no provider or network. The fixture's final cleanup killed and verified absence of every known test process.

The root cause is the remaining ownership gap: the interrupt path aborts but does not await command teardown before src/cli-core.ts calls process.exit(130). The unref'ed escalation timer cannot then run. Separately, child.kill() targets the wrapper PID, so descendants survive even when the wrapper exits.

A complete replacement needs one runtime-owned, awaited command-tree cleanup boundary shared by timeout and interruption. It must wait only for owned process teardown, not for arbitrary user exec/parse callbacks that might never settle. The current green tests do not establish those guarantees. No contributor changes are requested, and the PR is left open for the orchestrator's disposition.

[steipete]

Thanks @SebTardif for working on this. The interruption/timeout cleanup problem is real, but I’m closing this implementation because the built-CLI proof still leaves both a SIGTERM-resistant direct child and a shell-wrapper descendant alive 1.6 seconds after the CLI exits with code 130.

Cancellation needs a shared, awaited process-tree cleanup boundary: finish teardown and escalation for owned processes before CLI exit, without waiting indefinitely for user callbacks. Killing only the wrapper PID and scheduling an unref’ed escalation timer does not provide that guarantee. A corrected design with interruption and timeout proof for resistant children and descendants is very welcome.