🐛 Arm process kill-teardown before spawning the child - #237
Closed
taras wants to merge 1 commit into
Closed
Conversation
A halt delivered while exec was still acquiring its process could land on any suspension point between the OS spawn and the ensure that registers the kill-teardown, discarding the rest of the acquisition sequence. The teardown never armed, so the child — and its whole process tree — kept running with no owner. Register a guard teardown before the process exists and spawn in the same synchronous continuation, so at no point does the process exist without an armed teardown. The guard reuses the configured shutdown semantics but joins on process exit alone, since the stdio pumps may never have been wired; it stands down once the primary teardown at the end of the acquisition has begun. Exit, close, and error observation moves from spawned watchers to Node listeners attached in the spawn's continuation so the guard can always join on process exit, wherever a halt lands. The regression test exploits the scheduler's round-robin between same-generation routines to sweep a halt deterministically across every suspension point of the acquisition sequence. Fixes #236
|
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
marked this pull request as draft
August 10, 2026 06:23
This was referenced Aug 12, 2026
taras
added a commit
that referenced
this pull request
Aug 12, 2026
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
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.
Fixes #236
Problem
A scope halted while
execis acquiring its child process can leave the child — and its whole process tree — running with no owner. The OS process was created first and theensurethat terminates it registered last, with several suspension points in between. A halt landing on any of them discards the remaining instructions, so the kill-teardown never arms and the orphan runs to completion. The window cannot be guarded from the caller's side: it is inside the singleyield* exec(...), so the caller never receives aProcessto clean up with.Fix
Both
createPosixProcessandcreateWin32Processnow:The shutdown-mode logic (
graceful/forced/ policy function) is factored into a singleshutdownProcess(join)used by both teardowns, so the orphan-window path honors the configured shutdown semantics too.Regression test
Effection's reducer interleaves same-generation routines one instruction per turn. The new test spawns a halter one scope generation deeper than the exec task — the same generation as the acquisition's internal routines — and sweeps the number of turns before
task.halt(), deterministically landing the halt on every suspension point of the acquisition sequence, then asserts viapsthat no child survives. Against the previous code this reliably orphans the child (halts landing after the spawn but before the teardown registration); with the fix, all sweep positions stay clean. No timers, no load dependence.Stacked on #230 (
agent/process-exited).