🐛 Settle join() only after output clears Stdio middleware - #248
Open
taras wants to merge 1 commit into
Open
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
commit: |
taras
force-pushed
the
fix/join-settles-after-stdio-drain
branch
from
August 13, 2026 01:55
6110727 to
427ee01
Compare
On POSIX, join()/expect() resolved directly from the useNativeProcess result, which settles on the child-process close event while the Stdio middleware tasks may still be forwarding the final chunks. Callers could observe an exit status before all output had passed through their middleware and the public signals. - posix gains the same drain-then-resolve task win32 already had: processResult settles only after the close result plus both middleware tasks completing, so a blocked handler keeps join() pending - both platforms: a failing Stdio handler resolves processResult with Err, so join()/expect() throw the handler's error instead of crashing the scope or hanging the drain, and the middleware tasks always close their signals and resolve their done-resolvers on the way out - an internal CloseEvent context (not exported from mod.ts) lets the regression tests order assertions deterministically around the close event without scheduler sleeps Fixes #244
taras
force-pushed
the
fix/join-settles-after-stdio-drain
branch
from
August 13, 2026 02:14
427ee01 to
f2964df
Compare
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
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.
Motivation
Fixes #244. On POSIX,
join()andexpect()read theuseNativeProcessresult directly, which settles on the child-processcloseevent — while theStdiomiddleware tasks may still be forwarding the final chunks. A caller could observe the exit status before all output had passed through its middleware and the public signals: finalize a decoder early, persist truncated output, or dismantle a middleware scope that still owns pending chunks. The win32 adapter already drained before resolving. Separately, a throwingStdiohandler crashed the host scope on posix and could deadlock the drain wait on win32.Approach
Stacked on #247; supersedes #245, whose fix and regression tests are carried here. No changes to
native.ts— the resource's close-settled result is correct; this is a middleware-layer change in the adapters:processResultsettles only after the resource result plus both middleware tasks completing, so a blocked handler keepsjoin()pending. The two adapters are now symmetric.join()stays close-settled per process: no way to observe exit — join() settles on close (exit + stdio EOF), which can outlive the command #228's decision — the drain adds to the close wait, never replaces it — andExec.join()/Exec.expect()inherit the guarantee.Stdiohandler resolvesprocessResultwithErr, sojoin()/expect()throw the handler's error instead of hanging, and the middleware tasks always close their signals and resolve their done-resolvers in a synchronousfinally, so teardown can never wait on a dead task.CloseEventseam (src/exec/internal.ts, not exported frommod.ts) confirms the close event was received —join()therefore settles strictly after close yet must still show complete output, with no scheduler sleeps. Mutation-verified: deleting the posix drain fails both completeness tests deterministically. If a publicexited()lands later (process: no way to observe exit — join() settles on close (exit + stdio EOF), which can outlive the command #228's original ask), the seam is deleted and the tests gate on that instead.40/40 process tests × 3 runs on top of #247; typecheck/lint/format clean. The platform test matrix runs once this retargets to main after #247 merges.