Fix Process.Kill(entireProcessTree: true) when intermediate child has KillOnParentExit - #128598
#128598Fix Process.Kill(entireProcessTree: true) when intermediate child has KillOnParentExit#128598adamsitnik merged 13 commits intomaindotnet/runtime:mainfrom copilot/fix-process-tree-kill-issuedotnet/runtime:copilot/fix-process-tree-kill-issueCopy head branch name to clipboard
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-diagnostics-process |
…te process Adds a regression test to KillOnParentExitTests.cs that mimics issue #107992: when an intermediate child process has KillOnParentExit (job object with JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE), calling Kill(entireProcessTree: true) on the root should kill grandchildren not in the job object. The test creates a 3-level process hierarchy: - Process A (root) -> Process B (KillOnParentExit=true) -> Process C (long-running) The test is marked with [ActiveIssue] since the underlying bug is not yet fixed. Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/44753eb7-cbae-41b4-b37c-592baab16686 Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
The test should pass on every OS supported by the test class and should be executed by CI. Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot please address my feedback
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Converted to |
|
@copilot is it possible to somehow collect a memory dump of a test process that times out on macOS using this repo CI infrastructure? Don't update the code, just answer. |
|
@adamsitnik Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub. You can ask me to try again later by mentioning me in a new comment. If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: Sorry for the inconvenience! |
Yes — but not automatically for plain test timeouts today. In this repo’s Helix test runner, dump collection is pre-configured ( Relevant infra pieces already exist:
So: feasible with a targeted Helix repro configuration, but not something current default timeout handling guarantees out of the box. |
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot based on the information I've received offline from @jozkee (see the details box below), I believe that we need to change the way to register SIGCHILD here:
and use SA_NOCLDSTOP:
-installed = InstallSignalHandler(SIGCHLD, SA_RESTART);
+installed = InstallSignalHandler(SIGCHLD, SA_RESTART | SA_NOCLDSTOP);so we receive only SIGCHILD when a child process has exited or has been killed, not stopped and/or resumed
Details
I re-triggered the hang and sampled the stuck runner process ( sample <pid> ). The blocked/spinning thread is not in proc_pidinfo — in fact proc_pidinfo appears zero times in the whole sample. The PR thread's "proc_pidinfo blocks on a suspended process" theory does not match what's actually happening.
What's actually hung: the .NET SigHandler thread is busy-spinning (root process stuck in state T ), tight-looping over the child-reaping syscalls:
Thread ...: .NET SigHandler
SignalHandlerLoop (libSystem.Native)
├─ SystemNative_WaitPidExitedNoHang → __wait4_nocancel
└─ SystemNative_WaitIdAnyExitedNoHangNoWait → __waitid_nocancel
(interleaved with GetErrNo / SetLastPInvokeError — i.e. a hot retry loop)
Meanwhile the runner's main thread is idle ( LowLevelMonitor_Wait → __psynch_cvwait ) — just waiting for the test task that never finishes.
Interpretation: The test SIGSTOPs process A (61506), which is a direct child of the test/runner process. That makes the kernel deliver SIGCHLD (job-control, child stopped) to the runtime. The runtime's SIGCHLD reaping loop peeks with waitid(WEXITED | WNOHANG | WNOWAIT) and reaps with wait4 — neither of which consumes a stopped (not exited) child notification. So the notification is never cleared and the SigHandler thread spins forever calling waitid / wait4 , burning CPU, and the Kill(entireProcessTree:true) call never completes → test hangs.
So the true macOS root cause is: SIGSTOP'ing a child of the .NET process wedges the runtime's SIGCHLD child-reaping loop into a busy spin, not a proc_pidinfo block. This still points to the same fix direction (don't leave the tree SIGSTOP'd on macOS / avoid the two-phase stop on non-Linux), but for a different underlying reason than the PR describes.
Without SA_NOCLDSTOP, every SIGSTOP sent to a child process in the StopTree algorithm triggers a SIGCHLD notification to the parent. On macOS this was causing Kill_EntireProcessTree_True_EntireTreeTerminated to hang indefinitely. Adding SA_NOCLDSTOP to both SIGCHLD registrations ensures the handler is only invoked when a child exits or is killed, not when it is stopped or resumed. This eliminates spurious SIGCHLD handling during the stop-then-kill tree traversal. Both InstallSignalHandler(SIGCHLD, ...) call sites are updated because InstallSignalHandler is idempotent — whichever is called first installs the handler with its flags, and the second call is a no-op. Changing only one site would leave the fix dependent on initialization order. Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Done. Added Both sites are updated because |
| installed = InstallSignalHandler(SIGCONT, SA_RESTART); | ||
| assert(installed); | ||
| installed = InstallSignalHandler(SIGCHLD, SA_RESTART); | ||
| installed = InstallSignalHandler(SIGCHLD, SA_RESTART | SA_NOCLDSTOP); |
There was a problem hiding this comment.
to other code reviewers: after making the code send SIGSTOP to all processes in its tree, some of the tests started to time out on macOS. copilot was giving me really strange explanations of it so I've asked @jozkee for help with debugging offline (I don't have a mac machine that can build dotnet/runtime), I got following update:
Details
I re-triggered the hang and sampled the stuck runner process ( sample <pid> ). The blocked/spinning thread is not in proc_pidinfo — in fact proc_pidinfo appears zero times in the whole sample. The PR thread's "proc_pidinfo blocks on a suspended process" theory does not match what's actually happening.
What's actually hung: the .NET SigHandler thread is busy-spinning (root process stuck in state T ), tight-looping over the child-reaping syscalls:
Thread ...: .NET SigHandler
SignalHandlerLoop (libSystem.Native)
├─ SystemNative_WaitPidExitedNoHang → __wait4_nocancel
└─ SystemNative_WaitIdAnyExitedNoHangNoWait → __waitid_nocancel
(interleaved with GetErrNo / SetLastPInvokeError — i.e. a hot retry loop)
Meanwhile the runner's main thread is idle ( LowLevelMonitor_Wait → __psynch_cvwait ) — just waiting for the test task that never finishes.
Interpretation: The test SIGSTOPs process A (61506), which is a direct child of the test/runner process. That makes the kernel deliver SIGCHLD (job-control, child stopped) to the runtime. The runtime's SIGCHLD reaping loop peeks with waitid(WEXITED | WNOHANG | WNOWAIT) and reaps with wait4 — neither of which consumes a stopped (not exited) child notification. So the notification is never cleared and the SigHandler thread spins forever calling waitid / wait4 , burning CPU, and the Kill(entireProcessTree:true) call never completes → test hangs.
So the true macOS root cause is: SIGSTOP'ing a child of the .NET process wedges the runtime's SIGCHLD child-reaping loop into a busy spin, not a proc_pidinfo block. This still points to the same fix direction (don't leave the tree SIGSTOP'd on macOS / avoid the two-phase stop on non-Linux), but for a different underlying reason than the PR describes.
Which lead me to conclusion that if we use SA_NOCLDSTOP here we can receive SIGCHILD only when a child process has exited or has been killed, not stopped and/or resumed.
There was a problem hiding this comment.
From the above, my understanding is that we're adding SA_NOCLDSTOP to avoid getting signalled for stopped children. This avoids getting stuck in a loop because macOS returns stopped, non-exited children from this check (even though we don't include WSTOPPED in the waitid):
runtime/src/native/libs/System.Native/pal_process.c
Lines 1185 to 1194 in 402ed14
Fix lgtm.
|
/ba-g full framework build failure is unrelated |
The newly added test discovered a bug on Linux where
Process.Kill(entireProcessTree: true)fails to kill grandchild processes when an intermediate child hasPR_SET_PDEATHSIGset (exposed viaProcessStartInfo.KillOnParentExit). Killing the root immediately terminates the intermediate child (via parent death signal) before the tree traversal can discover its descendants.The test passes on Windows without any code changes — on Windows every process inherits Job objects from the parent process (unless they explicitly escape the Job), so the grandchild is already part of the same Job and gets terminated along with its parent. The bug described in issue #107992 could not be reproduced because Job objects are inherited by grandchild processes.
Summary
Fixes a Linux-specific bug in
Process.Kill(entireProcessTree: true)where grandchild processes were left running when an intermediate child hadPR_SET_PDEATHSIG(KillOnParentExit) set. The fix introduces a two-phase stop-then-kill algorithm: recursively SIGSTOP the entire tree first (collecting all processes into a flat list), then SIGKILL all collected processes. This ensures the full tree is discovered while all processes are alive and stopped, before any kill signals trigger cascading parent-death signal deaths.Previously, the algorithm killed each process immediately after enumerating its direct children. This caused processes with
PR_SET_PDEATHSIG(KillOnParentExit) to be killed by the kernel before their own children could be discovered, leaving grandchild processes orphaned and running.Changes
Fix: Unix
KillTreeinProcess.Unix.csRefactored the
KillTreemethod to use a two-phase stop-then-kill approach:StopTree: Recursively SIGSTOPs all processes depth-first and collects them into a flat list. This captures the entire tree while all processes are alive and stopped. Returnstrueif the process was successfully stopped and added to the list,falseif it exited early or SIGSTOP failed (in which case the caller disposes it). The kill/dispose loop runs in afinallyblock so it always executes even ifStopTreethrows partway through.Additional robustness improvements applied during code review:
StopTreereturnsboolso the caller can immediately dispose childProcessinstances that were not added tostoppedProcesses(exited early or SIGSTOP failed), preventing resource leaks.finallyblock, ensuring already-stopped processes are always killed and disposed even ifStopTreethrows partway through tree enumeration.GetLastErrorInfo().RawErrnobefore any other allocation and passed explicitly toWin32Exception, preventing it from being clobbered.Test: Regression test in
KillOnParentExitTests.csAdds a
[ConditionalTheory]parameterized with[InlineData(true)]and[InlineData(false)]that creates a 3-level process hierarchy:Kill(entireProcessTree: true)on Process AKillOnParentExitis enabled on the intermediate child[PlatformSpecific]or[ActiveIssue]guards)Testing
KillOnParentExitTestspass on Linux for bothenabled: trueandenabled: falsecasesProcessTestspass on Linux (226 tests, 0 failures)