Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Fix Process.Kill(entireProcessTree: true) when intermediate child has KillOnParentExit - #128598

#128598
Merged
adamsitnik merged 13 commits into
maindotnet/runtime:mainfrom
copilot/fix-process-tree-kill-issuedotnet/runtime:copilot/fix-process-tree-kill-issueCopy head branch name to clipboard
Jul 17, 2026
Merged

Fix Process.Kill(entireProcessTree: true) when intermediate child has KillOnParentExit#128598
adamsitnik merged 13 commits into
maindotnet/runtime:mainfrom
copilot/fix-process-tree-kill-issuedotnet/runtime:copilot/fix-process-tree-kill-issueCopy head branch name to clipboard

Conversation

Copilot AI commented May 26, 2026

Copy link
Copy Markdown
Contributor

The newly added test discovered a bug on Linux where Process.Kill(entireProcessTree: true) fails to kill grandchild processes when an intermediate child has PR_SET_PDEATHSIG set (exposed via ProcessStartInfo.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 had PR_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 KillTree in Process.Unix.cs

Refactored the KillTree method to use a two-phase stop-then-kill approach:

  1. 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. Returns true if the process was successfully stopped and added to the list, false if it exited early or SIGSTOP failed (in which case the caller disposes it). The kill/dispose loop runs in a finally block so it always executes even if StopTree throws partway through.
  2. Kill phase: SIGKILLs all collected (already stopped) processes.

Additional robustness improvements applied during code review:

  • StopTree returns bool so the caller can immediately dispose child Process instances that were not added to stoppedProcesses (exited early or SIGSTOP failed), preventing resource leaks.
  • The kill/dispose loop runs in a finally block, ensuring already-stopped processes are always killed and disposed even if StopTree throws partway through tree enumeration.
  • errno is captured into GetLastErrorInfo().RawErrno before any other allocation and passed explicitly to Win32Exception, preventing it from being clobbered.

Test: Regression test in KillOnParentExitTests.cs

Adds a [ConditionalTheory] parameterized with [InlineData(true)] and [InlineData(false)] that creates a 3-level process hierarchy:

Process A (root)
  └── Process B (KillOnParentExit=enabled, controlled by parameter)
        └── Process C (not in job object, should be killed)
  • Calls Kill(entireProcessTree: true) on Process A
  • Asserts Process C is also terminated
  • Verifies behavior regardless of whether KillOnParentExit is enabled on the intermediate child
  • Runs on all platforms supported by the test class (no [PlatformSpecific] or [ActiveIssue] guards)

Testing

  • All KillOnParentExitTests pass on Linux for both enabled: true and enabled: false cases
  • All ProcessTests pass on Linux (226 tests, 0 failures)

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

…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>
Copilot AI requested review from Copilot and removed request for Copilot May 26, 2026 17:35
Copilot AI changed the title [WIP] Fix Process.Kill not terminating entire process tree Add regression test for Kill(entireProcessTree: true) with job object intermediate process May 26, 2026
Copilot AI requested a review from adamsitnik May 26, 2026 17:37

@adamsitnik adamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please apply my suggestions

Comment thread src/libraries/System.Diagnostics.Process/tests/KillOnParentExitTests.cs Outdated
Comment thread src/libraries/System.Diagnostics.Process/tests/KillOnParentExitTests.cs Outdated
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>

Copilot AI commented May 26, 2026

Copy link
Copy Markdown
Contributor Author

@copilot please apply my suggestions

Done — removed both [PlatformSpecific(TestPlatforms.Windows)] and [ActiveIssue(...)] in commit b84f84c.

Copilot AI requested review from Copilot and removed request for Copilot May 26, 2026 18:06
Copilot AI requested a review from adamsitnik May 26, 2026 18:07

@adamsitnik adamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please address my feedback

Comment thread src/libraries/System.Diagnostics.Process/tests/KillOnParentExitTests.cs Outdated
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>

Copilot AI commented May 26, 2026

Copy link
Copy Markdown
Contributor Author

@copilot please address my feedback

Converted to [ConditionalTheory] with [InlineData(true)] and [InlineData(false)] — the enabled parameter controls KillOnParentExit on the intermediate child process, verifying that Kill(entireProcessTree: true) kills the entire tree in both cases. Done in commit e5c4476.

Copilot AI requested review from Copilot and removed request for Copilot May 26, 2026 18:17
Copilot AI review requested due to automatic review settings June 18, 2026 12:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@adamsitnik

Copy link
Copy Markdown
Member

@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.

Copilot AI commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

@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: 9cf95ebb-ce1c-46e1-b309-2ae22b467c3b

Sorry for the inconvenience!

Copilot AI commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

@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.

Yes — but not automatically for plain test timeouts today.

In this repo’s Helix test runner, dump collection is pre-configured (DOTNET_DbgEnableMiniDump=1, DOTNET_DbgMiniDumpName=$HELIX_DUMP_FOLDER/...), which primarily helps for crashes/aborts. For timeout hangs on macOS, you need a repro leg that explicitly triggers dump capture before Helix kills the work item (e.g., invoke createdump from the test wrapper/timeout path and upload from $HELIX_DUMP_FOLDER).

Relevant infra pieces already exist:

  • eng/testing/RunnerTemplate.sh sets dump env vars.
  • src/libraries/sendtohelixhelp.proj ad-hoc signs createdump on macOS so it can run in Helix.

So: feasible with a targeted Helix repro configuration, but not something current default timeout handling guarantees out of the box.

Copilot AI review requested due to automatic review settings July 16, 2026 03:13
@jozkee
jozkee temporarily deployed to copilot-pat-pool July 16, 2026 03:13 — with GitHub Actions Inactive
@jozkee
jozkee temporarily deployed to copilot-pat-pool July 16, 2026 03:13 — with GitHub Actions Inactive

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@adamsitnik adamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:

installed = InstallSignalHandler(SIGCHLD, SA_RESTART);

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>
Copilot AI requested review from Copilot and removed request for Copilot July 16, 2026 15:32

Copilot AI commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

@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:

https://github.com/dotnet/runtime...

Done. Added SA_NOCLDSTOP to both InstallSignalHandler(SIGCHLD, ...) call sites in commit 9cd4cbc.

Both sites are updated because InstallSignalHandler is idempotent — whichever is called first (SystemNative_SetTerminalInvalidationHandler from Console initialization, or SystemNative_RegisterForSigChld from Process initialization) installs the handler with its flags, and the second call is a no-op. Changing only the one at L510 would leave the fix dependent on initialization order.

Copilot AI requested a review from adamsitnik July 16, 2026 15:34

@adamsitnik adamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, the PR is ready for review now.

@tmds @jkotas PTAL, it fixes a bug that I've introduced by introducing KillOnParentExit on Linux

installed = InstallSignalHandler(SIGCONT, SA_RESTART);
assert(installed);
installed = InstallSignalHandler(SIGCHLD, SA_RESTART);
installed = InstallSignalHandler(SIGCHLD, SA_RESTART | SA_NOCLDSTOP);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):

while (CheckInterrupted(result = waitid(P_ALL, 0, &siginfo, WEXITED | WNOHANG | WNOWAIT)));
if (result == 0)
{
// When there are no waitable children and WNOHANG is specified,
// waitid may return zero with si_pid unchanged.
assert(siginfo.si_pid == 0 || // no waitable child
siginfo.si_signo == SIGCHLD); // waitable child
result = siginfo.si_pid;
}

Fix lgtm.

@adamsitnik

Copy link
Copy Markdown
Member

/ba-g full framework build failure is unrelated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

6 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.