🐛 Settle process join() only after stdout/stderr pumps complete by taras · Pull Request #245 · thefrontside/effectionx

taras · 2026-08-12T02:30:54Z

Closed: superseded by #248, which carries this fix and its regression tests rebuilt on the useNativeProcess architecture from #247.

Motivation

Addresses #244. On POSIX, Process.join()/expect() resolved as soon as the child-process close event fired, while the stdout/stderr pump tasks could still be forwarding the final chunks through Stdio middleware. A caller could observe the exit status before all output had passed through its middleware and the public signals. The Windows adapter already waited for the pumps before resolving.

Approach

posix: after the close event, the close watcher waits for both pump done-resolvers before resolving processResult — the same drain-before-result ordering win32 already had, so a blocked Stdio handler provides real backpressure against join().
both adapters: a throwing Stdio handler resolves processResult with Err, so join()/expect() throw instead of hanging; pumps always resolve their done-resolvers and close their signals in a synchronous finally.
an internal CloseEvent seam (src/exec/internal.ts, not exported from mod.ts) lets the regression tests order assertions deterministically around the close event without scheduler sleeps. Mutation-verified: with the pump wait removed, both completeness tests fail 5/5.

Process.join() could resolve on the child-process close event while the stdout/stderr pump tasks were still forwarding the final chunks through Stdio middleware, so callers could observe an exit status before all output was delivered. - posix: wait for both pump done-resolvers after the close event before resolving processResult, matching the win32 adapter's existing drain-before-result ordering - both adapters: a failing Stdio handler now resolves processResult with Err so join()/expect() throw instead of hanging, and the pumps always resolve their done-resolvers and close their signals on the way out - add an internal CloseEvent context so tests can order assertions deterministically around the close event without scheduler sleeps Fixes #244

coderabbitai · 2026-08-12T02:31:12Z

📝 Walkthrough

Walkthrough

Process execution now invokes registered close callbacks, waits for stdout and stderr pumps before settling, and propagates output-processing errors. Tests cover output completeness and error handling for join() and expect().

Changes

Process completion handling

Layer / File(s)	Summary
Close-event contract and shutdown flow `process/src/exec/internal.ts`, `process/src/exec/posix.ts`, `process/src/exec/win32.ts`	Adds the exported `CloseEvent` context. POSIX and Windows adapters invoke the callback and wait for both output pumps before resolving completion.
Output pump completion and errors `process/src/exec/posix.ts`, `process/src/exec/win32.ts`	Output-processing failures resolve `processResult` with `Err`. Signals and pump-completion resolvers close in `finally` blocks.
Output completeness and error tests `process/test/exec.test.ts`	Tests verify complete stdout and stderr forwarding and error propagation through `join()` and `expect()`.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related issues

process: join() settles before stdout/stderr pumps finish Stdio middleware #244 — Addresses delayed process settlement and output pump error handling.
process: no way to observe exit — join() settles on close (exit + stdio EOF), which can outlive the command #228 — Covers related close-event and stdio completion timing.

Possibly related PRs

thefrontside/effectionx#159 — Introduced related process execution and middleware paths.
thefrontside/effectionx#195 — Added related POSIX, Windows, and execution test behavior.
thefrontside/effectionx#230 — Modified related process close and stdio settlement handling.

Suggested reviewers: cowboyd

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error)

Check name	Status	Explanation	Resolution
Policy Compliance	❌ Error	The PR changes three `@effectionx/process` source files, but process/package.json remains 0.8.2 with no version diff; version-bump.md requires a semantic bump.	Bump `@effectionx/process` to an appropriate semantic version, such as 0.8.3 for this bug fix.

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely states the primary change: process join() now waits for stdout and stderr pumps to complete.
Description check	✅ Passed	The description explains the problem, implementation, internal test seam, and tests, although it uses Problem and Changes headings instead of the template headings.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/process-join-pump-drain

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

pkg-pr-new · 2026-08-12T02:32:39Z

Open in StackBlitz

npm i https://pkg.pr.new/@effectionx/process@245

commit: 962dfd2

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@process/test/exec.test.ts`:
- Around line 388-390: Update the stderr assertion in the joined execution test
around stderrAtSettle to compare the complete normalized fixture output,
matching the stdout test, instead of using toContain("boom\n"). Preserve the
existing status check and verify the full stderr value at settlement.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 5d316826-a798-417b-a4dc-da4a93569f6b

📥 Commits

Reviewing files that changed from the base of the PR and between 87bc487 and 962dfd2.

📒 Files selected for processing (4)

process/src/exec/internal.ts
process/src/exec/posix.ts
process/src/exec/win32.ts
process/test/exec.test.ts

coderabbitai · 2026-08-12T02:33:25Z

+      const status = yield* joined;
+      expect(status.code).toEqual(0);
+      expect(stderrAtSettle).toContain("boom\n");


🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert the complete stderr value.

toContain("boom\n") passes if a later stderr chunk is missing. Compare stderrAtSettle with the complete normalized fixture output, as the stdout test does. This verifies the stated stderr-completeness contract.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@process/test/exec.test.ts` around lines 388 - 390, Update the stderr assertion in the joined execution test around stderrAtSettle to compare the complete normalized fixture output, matching the stdout test, instead of using toContain("boom\n"). Preserve the existing status check and verify the full stderr value at settlement.

Replace the fromReadable pump architecture in both adapters with a single createNativeProcess core that wires the child process entirely through native Node listeners attached in the same synchronous continuation as the spawn. posix and win32 reduce to SpawnStrategy objects: how to spawn, and how to shut down. - the orphan window from #236 closes: a guard teardown registers before the child exists and the spawn plus all listener wiring follow with no suspension points in between - close-settled means raw-output-complete by construction (#244): Node emits "close" only after both stdio streams have closed, and chunk delivery into the raw signals is synchronous with stream emission - Stdio middleware runs as consumer tasks over the raw signals; a sequencer settles join()/expect() only after the close event and both consumers draining, and a failing handler resolves the result with Err instead of hanging - the graceful-shutdown drain contract is preserved: the primary teardown registers after the consumers, so they are still alive to forward output produced during termination - the eval scope remains solely to service around(); documented at the point of use Includes the halt-sweep regression test originated in #237 and the output-completeness and middleware-failure tests originated in #245, each verified to fail when its guarantee is mutated out. Fixes #236 Fixes #244

taras · 2026-08-12T03:44:44Z

Superseded by #248, which carries this fix and its regression tests rebuilt on the useNativeProcess architecture from #247.

coderabbitai Bot reviewed Aug 12, 2026

View reviewed changes

taras mentioned this pull request Aug 12, 2026

⚗️ Rebuild process adapters on a native-listener core #246

Closed

This was referenced Aug 12, 2026

✨ useNativeProcess: a native-listener child-process resource #247

Open

🐛 Settle join() only after output clears Stdio middleware #248

Open

taras closed this Aug 12, 2026

Navigation Menu

Uh oh!

🐛 Settle process join() only after stdout/stderr pumps complete - #245

🐛 Settle process join() only after stdout/stderr pumps complete#245
taras wants to merge 1 commit into
mainfrom
fix/process-join-pump-drain

taras commented Aug 12, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Aug 12, 2026 •

edited

Loading

Walkthrough

Changes

Pre-merge checks failed

Uh oh!

pkg-pr-new Bot commented Aug 12, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot Aug 12, 2026

Uh oh!

taras commented Aug 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

taras commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Approach

Uh oh!

coderabbitai Bot commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Pre-merge checks failed

❌ Failed checks (1 error)

Uh oh!

pkg-pr-new Bot commented Aug 12, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

taras commented Aug 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

taras commented Aug 12, 2026 •

edited

Loading

coderabbitai Bot commented Aug 12, 2026 •

edited

Loading