Repository navigation
Reap owned subprocesses on every exit path - #105
Merged
Merged
Conversation
BoundedProcess.await takes the call's CancellationToken. On timeout, cancellation, interrupt or any exception while waiting it kills the child and its descendants, captured once before the first signal, and forcibly kills whatever is still alive after BoundedProcess.GRACE, without letting an interrupt cut the grace short. Each output stream and LocalProcessExecutionProvider's stdin feeder get the same grace after the child exits, so a backgrounded descendant holding a pipe no longer hangs the call; such a stream is reported truncated. CommandGrant.invoke takes the token from CommandGrantTool, and a cancelled grant returns "Cancelled while invoking <name>". LocalProcessExecutionProvider drops its own kill callback and SIGKILL branch in favour of the shared escalation. A JvmSandbox launch that fails after its RPC channel opens closes the channel before the socket, kills the child's tree, and installs the prelude before the constructor registers a shutdown hook.
ProcessTree.killAndReap now gives the processes it SIGKILLs the same bounded grace to end, so await no longer returns while a killed child whose output is already closed is still alive. BoundedSink.awaitEnd throws on an interrupt pending on entry, so an interrupt the reaping kept for the caller propagates even when both drains have already ended.
BoundedProcess.await learnt of the child's exit through Process.onExit(), which completes on the common pool. With that pool busy, a child that had already exited was reported as timed out with exit code -1, or its result waited until the timeout. The exit is now signalled by a virtual thread blocked in Process.waitFor(). BoundedSink holds the stream's monitor for the whole drain. The JDK's process reaper takes that monitor when the child exits to close the pipe, even while a descendant still holds it, so a drain caught between two reads ended early, unmarked, at random. The background-child test now forces the drain to be reading before the child exits (the child writes more than a pipe holds), which made it deterministic; it failed whenever it ran alone.
join() already waits through an interrupt and sets it again, so the hand-written latch and retry loop go. The timeout runs on the delay scheduler, not a common-pool worker. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…thread waitFor is signalled by the JDK's process reaper, so it needs neither the common pool nor a virtual-thread carrier: the exit-signal thread, its FutureTask and the isAlive confirmation go. A cancel wakes the wait by reaping the tree on a thread of its own, which ends the child. Co-Authored-By: Claude Opus 5.5 <[email protected]>
repl's tests already depend on session's test-jar, whose LogCapture does the same job and keeps expected records out of the build output, so the repl copy goes. Co-Authored-By: Claude Opus 5.5 <[email protected]>
The Breaking paragraph and row no longer repeat each other, the Fixed row drops the narration, and drain's javadoc names ProcessPipeInputStream.processExited so the reliance can be checked. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
ProcessTree signals the descendants captured before the first signal plus those visible at each later signal. No test failed when the live re-read was dropped (snapshot only). The new test starts the grandchild after a first kill of a SIGTERM-deaf child, then lets the timeout reap the tree. Co-Authored-By: Claude Opus 5.5 <[email protected]>
The JDK closes an exited child's pipe under the stream's monitor even while a backgrounded descendant still writes to it; BoundedSink.drain holds that monitor from the first read to the end. No test failed when the synchronized block was dropped (10 of 10 runs of the backgrounded-child test passed), because the race lies between two reads. The new test holds the drain up in the sink between reads and shows a contender for the stream's monitor still blocks. Co-Authored-By: Claude Opus 5.5 <[email protected]>
aDescendantHoldingStdinUnreadDoesNotHoldUpTheCall passed with the feeder join made unbounded again: its child exited at once, so the feeder usually started writing after the exit and failed on the stdin the JDK had already closed. The child now reads one byte first, so the feeder is blocked on the rest when the child exits, and only the bounded join lets the call return. Co-Authored-By: Claude Opus 5.5 <[email protected]>
aFailedLaunchClosesItsChannelAndLeavesNothingBehind compared the set of helios-rpc-* and helios-sandbox-cwd-* entries in java.io.tmpdir before and after the launch, so any other sandbox created or removed on the host during the launch failed it (reproduced by creating and removing a helios-rpc- directory in a loop while the test ran). The test now reads the launch's socket directory from the accepted socket's address and its working directory from the new subprocess's cwd, and asserts both are gone. Co-Authored-By: Claude Opus 5.5 <[email protected]>
LocalProcessExecutionProvider.close() reaped with its own copy of the old escalation, which re-read the descendants after the parent died and so left a re-parented grandchild that ignores SIGTERM running. BoundedProcess.kill() now kills and reaps the tree (TERM, GRACE, KILL on the snapshot), the cancel callback runs it, and InflightProcesses tracks BoundedProcess and calls it. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Four test classes each read a child's pid file, and two copied the same destroyRecorded. PidFile.await waits for the pid and returns the process; PidFile.destroy ends a recorded process that still runs. Co-Authored-By: Claude Opus 5.5 <[email protected]>
join(Duration) does not wait for a non-positive duration; it only checks whether the thread has ended, so the clamp to zero changed nothing. Co-Authored-By: Claude Opus 5.5 <[email protected]>
The Fixed row had a broken wrap from an inline insertion, the kill() row now reads as the 3.0 behaviour, and the provider's lifecycle javadoc says close() kills and reaps rather than destroys forcibly at once. Co-Authored-By: Claude Opus 5.5 <[email protected]>
completeOnTimeout fires on the common pool's one delay-scheduler thread, which also runs other futures' dependents synchronously, so a blocking callback anywhere in the host could hold a kill's grace open indefinitely. endWithin waits on its own clock again. A cancel sends SIGTERM on the token's thread before starting the escalation, so it reaches the child while every virtual-thread carrier is busy. Co-Authored-By: Claude Opus 5.5 <[email protected]>
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.
Spec:
audit-process-lifecycle(audit F15, P1).What changes
BoundedProcess.await(CancellationToken)replacesawait(). It waits withProcess.waitFor(timeout). On timeout, cancellation, interrupt or any exception while waiting it kills and reaps the tree, then rethrows with the interrupt status set again. Cancelling the token sends SIGTERM on the token's thread and then runskill()on a virtual thread, so a SIGTERM-deaf child is killed forcibly and does not keep the call waiting until the timeout. A child the call killed reports exit code-1.BoundedProcess.kill()kills and reaps. Descendants are captured once, before the first signal. Every signal also reaches descendants still visible at that moment. Whatever is alive afterBoundedProcess.GRACE(2 s) is destroyed forcibly, whether or not the parent has exited.kill()returns once the tree has ended, or after a secondGRACE. Each grace is timed on the waiting thread's own clock, not on the JVM's sharedCompletableFuturetimer, which a blocking callback elsewhere in the host can hold up. The signalling lives in the package-privateProcessTree.GRACEafter the child exits. A drain still open then is abandoned and its stream is reported truncated.LocalProcessExecutionProvider's stdin feeder join uses the same constant.BoundedSink.drainholds the stream's monitor for the whole drain, so the JDK'sprocessExitedcannot close a pipe that a backgrounded descendant still writes to.CommandGrant.invoke(args, token)receivesctx.cancellation()fromCommandGrantTool. A cancelled call returnsCancelled while invoking <name>.LocalProcessExecutionProviderno longer has its own kill callback or SIGKILL branch.InflightProcessesnow tracksBoundedProcessand reaps throughkill(), soclose()and the shutdown hook use the same escalation. Before this, close re-read descendants after the parent died and left a re-parented grandchild that ignores SIGTERM running.JvmSandbox.createon failure closes the RPC channel before the socket and kills the child withSandboxLauncher.destroyTree.installPreludenow runs before the constructor, so a failed install never registers a shutdown hook. A package-private overload hands the acceptedSandboxRpcto tests; Uday chose to keep it.awaitandkill(), and one Fixed row.Differences from the spec's wording
awaitstops the child promptly with the bounded TERM-then-KILL escalation. It does not send SIGTERM and then wait out the timeout.BoundedProcessTest.interruptWhileAwaitingPropagatesAndLeavesTheProcessToTheCallerasserted the old behaviour. Acceptance test 2 replaces it. Every other existing test keeps its assertions.close()is fixed in this PR (kill()now means kill-and-reap).echo startedalone left that to chance.Size
+68 net main-code lines against
df416089(javadoc, comments, imports and blanks excluded), within the ~90-line budget. The pre-merge review removed the exit-signal thread and latch andInflightProcesses' duplicate escalation.Verification
mvn -B clean verify javadoc:jarat the head: BUILD SUCCESS, 7,882 tests run, 0 failures, 0 errors, 62 skipped (key-gated live tests). PMD, CPD, Spotless, JaCoCo and ArchUnit pass.BoundedProcessTest,BoundedSinkTest,CommandGrantTestandLocalProcessExecutionProviderRobustnessTest, with 0 failures. Delay injection at seven race windows found no failure. The final commit (own-clock grace, synchronous SIGTERM on cancel) passed five further full runs of those classes.Not pinned by a test, and why:
destroyTreeversusdestroyForcibly: the launch has no grandchildren at that point.ProcessTree's interrupt handling: it changes timing only.TimeUnitargument. TheTimedWaitInTestPMD rule forbids that, and suppressions are not allowed.🤖 Generated with Claude Code