🐛 Settle exec on the command's exit, not its pipes' close - #344
Open
taras wants to merge 2 commits into
Open
Conversation
The Process API's default exec measured its deadline against @effectionx/process join(), which resolves only after exit AND stdio EOF. A pipe fd held open past the deadline — an inherited fd in a straggling grandchild, or stream delivery lagging under a loaded host — reported a command that finished within its budget as timed out and discarded the output it had already produced. Full runtime-suite conditions supply exactly that pressure, which is how packages/core execute.test.ts D3b could time out at 30s in the complete Bun run while passing focused (#343). exec now settles on the child's exit: the unchanged budget bounds spawn→exit, the command's process group is reaped the moment it exits so EOF follows promptly, and the output its pipes delivered is returned. A command still running at the deadline times out exactly as before. Windows keeps the close-settled path, where process groups don't exist. The regression test spawns a straggler that inherits the stdout pipe and would hold EOF open thirty times past the budget: close-settled execution fails it with the exact D3b signature; exit-settled execution returns the printed output and exit code on every runtime.
| try { | ||
| process.kill(-child.pid, "SIGTERM"); | ||
| } catch { | ||
| // the group is already gone |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
Suggested change
| // the group is already gone |
| ]); | ||
|
|
||
| if (settled.kind === "timeout") { | ||
| // ensure() reaps the group on the way out of this scope. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
Suggested change
| // ensure() reaps the group on the way out of this scope. |
| }; | ||
| // Process groups and `kill(-pid)` do not exist on Windows, so the | ||
| // exit-settled path below cannot reap stragglers there; Windows keeps | ||
| // the close-settled `@effectionx/process` path. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
Suggested change
| // the close-settled `@effectionx/process` path. |
PR #344: 🐛 Settle exec on the command's exit, not its pipes' close2 files, +178 / -16 Scope✅ PR scope looks good. Structural✅ No structural bloat detected. Slop
Static Analysis✅ Oxlint found no issues. CorrectnessNo extraneous code patterns detected. |
| try { | ||
| process.kill(-child.pid, "SIGTERM"); | ||
| } catch { | ||
| // the group is already gone |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
Suggested change
| // the group is already gone |
| ]); | ||
|
|
||
| if (settled.kind === "timeout") { | ||
| // ensure() reaps the group on the way out of this scope. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
Suggested change
| // ensure() reaps the group on the way out of this scope. |
| }; | ||
| // Process groups and `kill(-pid)` do not exist on Windows, so the | ||
| // exit-settled path below cannot reap stragglers there; Windows keeps | ||
| // the close-settled `@effectionx/process` path. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
Suggested change
| // the close-settled `@effectionx/process` path. |
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.
Why
Fixes #343. The Bun 1.3.14 runtime gate could time out
packages/core/tests/execute.test.tsD3b only inside the complete 168-file suite:exec(bash) timed out after 30000ms, nopartialstdout, while the same test passes focused in 59 ms. The issue forbids raising the timeout or waiving the gate without locating the lifetime problem.What changes
The Process API's default
execmeasured its 30s deadline against@effectionx/processjoin(), which resolves only after the child's exit and stdio EOF. Anything that holds a pipe open past the deadline — a pipe fd inherited by a straggling grandchild, or stream delivery lagging under a loaded host — turns a command that finished within its budget into a timeout and discards the output it already produced. The full suite supplies exactly that pressure (concurrent daemons, workers, CLI churn); a focused run never does.Before:
exec({command: ["bash", "-c", "(sleep 30 &); echo partial; exit 1"], timeout: 1000})throwsexec(bash) timed out after 1000mson every runtime — the exact D3b failure signature — even though bash printedpartialand exited 1 within milliseconds.After:
{stdout: "partial\n", exitCode: 1}in ~15 ms. The budget is unchanged and still bounds the command: a command still running at its deadline times out exactly as before.How it works
Exit is the ground truth for "the command finished". On exit the command's process group is reaped, so pipe EOF follows promptly and stragglers cannot defer the result; output is accumulated by pumps that also forward through the
@effectionx/processStdioAPI, preserving middleware capture and passthrough. Windows keeps the previous close-settled path — process groups andkill(-pid)do not exist there.Diagnosis trail
closepast the deadline → the exact D3b failure on Bun and Node. That reproduction is the regression test.How to verify it
packages/runtime/tests/exec-timeout.test.ts"delivers a completed command's output while a straggler holds the pipe" proves exit-settled semantics and fails with the exact D3b signature under the diagnosed mutation (reverting to close-settledjoin()), not because any timeout increased.deno task lint/ scopeddeno check packages scripts/deno task test(361 files, 0 failed) /deno task check:jsr/tsc --project tsconfig.node.json/test:nodeon Node 22 (2369 pass, 0 fail) /npx --yes --package=bun@1.3.14 bun run test:buntwice (2369 pass, 0 fail, 169 files).Scope
Included
execin the default Process API handler (packages/runtime/apis.ts), same budget, same error text.packages/runtime/tests/, first tests dir for this package — discovery picks it up on all three runtimes without config edits).Intentionally unchanged
daemon()and every other@effectionx/processuse — teardown semantics there are scope-owned, not deadline-owned.execFactory, the D3b test, and the Bun gate.Risks and limitations
daemon.@effectionx/processexposes no exit-settled join, which is why the handler spawns directly vianode:child_process(mirroring the package's own detached-group discipline). Worth filing upstream as a primitive gap.Scope confirmation