Fix killing a pipeline leaves its root process running - #7073
Merged
Conversation
fubhy
added
the
audit
🦋 Changeset detectedLatest commit: 90265a7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Important
This PR adds a focused regression test but does not include the implementation fix its title claims. packages/platform-node-shared/src/NodeChildProcessSpawner.ts still forwards kill: handle.kill for the final pipeline stage only, so the root process is left running. The fix should mirror the Deno adapter.
Reviewed changes
- Added
it.live("kills every process in a pipeline", ...)inpackages/platform-node-shared/test/NodeChildProcessSpawner.test.ts. The test correctly demonstrates the bug: afterhandle.kill({ killSignal: "SIGKILL" }), the root heartbeat continues growing (confirmed locally:AssertionError: expected 20n to equal 10natNodeChildProcessSpawner.test.ts:39). The test must be paired with the implementation fix before merging.
⚠️ Implementation fix is missing
Technical details
# Missing pipeline kill aggregation
## Affected sites
- `packages/platform-node-shared/src/NodeChildProcessSpawner.ts:621-642` — returns the pipeline handle with `kill: handle.kill`, which forwards `kill` to only the final stage.
## Required outcome
- Killing a `PipedCommand` handle must terminate every stage started for that pipeline, matching the existing `unref` aggregation and the Deno adapter behavior.
## Suggested approach
- Build an aggregate `kill` in the `"PipedCommand"` branch:
```ts
const kill = (options?: ChildProcess.KillOptions | undefined) =>
Effect.forEach([...handles].reverse(), (handle) => Effect.ignore(handle.kill(options)), { discard: true })
```
- Pass `kill` to `makeHandle` instead of `handle.kill`, analogous to `packages/platform-deno/src/DenoChildProcessSpawner.ts:365-366`.ℹ️ Nitpicks
- The test name promises "kills every process", but only asserts the root heartbeat stops. Consider also asserting the child heartbeat stops, matching the Deno test at
packages/platform-deno/test/DenoChildProcessSpawner.test.ts:138-139.
@v0 or keep the SHA fresh with Dependabot ｜ Fix it ➔ ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run reviewed the delta since the prior Pullfrog review (d427a584), which adds the implementation fix and completes the regression test coverage.
- Added aggregate pipeline kill in
NodeChildProcessSpawner.ts. The"PipedCommand"branch now builds akillthat iterates over every pipeline handle in reverse order, matching the Deno adapter. - Wired the aggregate
killinto the returned handle. The pipeline handle no longer forwardskillto only the final stage. - Completed the regression test assertions. The test now verifies both the root and child heartbeats stop after
handle.kill, matching the test name "kills every process in a pipeline". - Added a changeset describing the runtime behavior fix.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
Contributor
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Calling
killon a shared Node handle for a piped command terminates only the tail process and can leave earlier pipeline stages running.Important
This PR includes the focused regression test and the implementation fix.
Killing a pipeline leaves its root process running
Module:
platform-node-shared/NodeChildProcessSpawnerAudit ID:
effect-4bd8d5ecaeff09f4Severity / confidence: medium / high
What happens
Calling
killon a shared Node handle for a piped command terminates only the tail process and can leave earlier pipeline stages running.Why it happens
The shared Node spawner stores every stage in
handlesand appliesunrefacross all of them, but builds the returned pipeline handle withkill: handle.kill, wherehandleis only the final stage. The independent Deno adapter applieskillto all handles in reverse order, confirming the platform contract is implemented at pipeline scope there.Expected behavior
A spawned
PipedCommandyields oneChildProcessHandle, whosekilloperation controls the child process represented by that handle. As with the pipeline's all-handleunref, killing that aggregate command must terminate every process started for it.Relevant implementation
These links and excerpts are pinned to audit base
17f0b91a243ccfe4a38d27debdc983adf434e738.packages/platform-node-shared/src/NodeChildProcessSpawner.ts:567-642View problematic code at
packages/platform-node-shared/src/NodeChildProcessSpawner.ts:567-616View exact lines on GitHub
Excerpt truncated. Open the complete packages/platform-node-shared/src/NodeChildProcessSpawner.ts:567-642 range.
Reproduction
pnpm test --run packages/platform-node-shared/test/NodeChildProcessSpawner.test.tsObserved failure: Focused contract assertion failed against 17f0b91, demonstrating: Killing a pipeline leaves its root process running.
Implementation
The Node spawner now kills every pipeline handle in reverse order, matching the Deno adapter. The regression verifies that both the root and tail processes stop.
Audit provenance
17f0b91a243ccfe4a38d27debdc983adf434e73817f0b91a243ccfe4a38d27debdc983adf434e738effect-4bd8d5ecaeff09f4Closes EFF-480