Skip to content

Fix Process.Kill(entireProcessTree: true) hang on macOS (#131944) #133930

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
jozkee wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Fix Process.Kill(entireProcessTree: true) hang on macOS (#131944) #133930

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1164,5 +1164,57 @@ public void ChildProcess_WithParentSignalHandler_CanReceiveSignals()
Assert.Equal(RemotelyInvokable.SuccessExitCode, childHandle.Process.ExitCode);
});
}

// Repro attempt for https://github.com/dotnet/runtime/issues/131944:
// Process.Kill(entireProcessTree: true) can hang indefinitely on macOS.
// The two-phase KillTree (SIGSTOP the whole tree, then SIGKILL) opens a window in which a
// direct child is SIGSTOP'd. On macOS, waitid(P_ALL, WEXITED|WNOHANG|WNOWAIT) also reports
// SIGSTOP'd children, so the SIGCHLD handler's CheckChildren loop spins forever (waitpid
// WNOHANG never reaps a stopped child, WNOWAIT keeps it waitable) while holding
// s_childProcessWaitStates. Any concurrent Process construction (e.g. the tree enumeration in
// another Kill) then blocks on that lock, so the stopped child is never SIGKILL'd -> deadlock.
[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
[PlatformSpecific(TestPlatforms.OSX)]
public void Kill_EntireProcessTree_Concurrent_DoesNotHang()

@adamsitnik adamsitnik Sep 15, 2026

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.

I was able to confirm that this test reproduces the problem: adamsitnik/macosrepro#2

{
const int TreeCount = 8;
const int Iterations = 30;
Comment on lines +1180 to +1181

@adamsitnik adamsitnik Sep 15, 2026

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.

Hmm this test spawns a LOT of processes. I think it's reasonable to keep it, but we should most likely consider moving it to Outerloop.


for (int iteration = 0; iteration < Iterations; iteration++)
{
var roots = new Process[TreeCount];
for (int i = 0; i < TreeCount; i++)
{
// Each direct child spawns a grandchild and then blocks forever, producing a
// small process tree rooted at a direct child of this test host.
Process root = CreateProcess(() =>
{
using Process grandChild = Process.Start("/bin/sleep", "1000");
Thread.Sleep(Timeout.Infinite);
return RemoteExecutor.SuccessExitCode;
});
root.Start();
roots[i] = root;
}

// Give the grandchildren time to start so the trees are fully formed.
Thread.Sleep(500);

var tasks = new Task[TreeCount];
for (int i = 0; i < TreeCount; i++)
{
Process root = roots[i];
tasks[i] = Task.Run(() => root.Kill(entireProcessTree: true));
}

bool completed = Task.WaitAll(tasks, TimeSpan.FromSeconds(60));
Assert.True(completed, $"Kill(entireProcessTree: true) hung on iteration {iteration}.");

foreach (Process root in roots)
{
Assert.True(root.WaitForExit(WaitInMS));
Comment on lines +1213 to +1215
}
}
}
}
}
67 changes: 52 additions & 15 deletions src/native/libs/System.Native/pal_process.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1179,25 +1179,62 @@ void SystemNative_SysLog(SysLogPriority priority, const char* message, const cha

int32_t SystemNative_WaitIdAnyExitedNoHangNoWait(void)
{
siginfo_t siginfo;
memset(&siginfo, 0, sizeof(siginfo));
int32_t result;
while (CheckInterrupted(result = waitid(P_ALL, 0, &siginfo, WEXITED | WNOHANG | WNOWAIT)));
if (result == 0)
while (true)
{
// 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
siginfo_t siginfo;
memset(&siginfo, 0, sizeof(siginfo));
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;
}
else if (errno == ECHILD)
{
// The calling process has no existing unwaited-for child processes.
result = 0;
if (siginfo.si_pid == 0)
{
// No waitable child.
return 0;
}

// We requested WEXITED only, but some platforms (notably macOS) also report

@adamsitnik adamsitnik Sep 15, 2026

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.

Hmm this is exactly why we have used SA_NOCLDSTOP in #128598 so it "should work" but apparently was not enough. It's reasonable for me to make it more defensive here. Linux and other reasonable Unixes should not be affected in any negative way (cc @tmds for double checking)

// children that have stopped (SIGSTOP) or continued (SIGCONT). Because WNOWAIT
// was specified, such a notification is not consumed and would be returned again
// on every call, causing the SIGCHLD handler (CheckChildren) to spin indefinitely
// while a process tree is temporarily stopped by Process.Kill(entireProcessTree: true).
// Only report children that have actually exited.
if (siginfo.si_code == CLD_EXITED ||
siginfo.si_code == CLD_KILLED ||
siginfo.si_code == CLD_DUMPED)
{
return siginfo.si_pid;
}

// Consume the stopped/continued notification so it isn't observed again, then keep
// looking for a child that has exited. This is a no-op on platforms that correctly
// honor WEXITED (e.g. Linux), where this branch is never reached.
siginfo_t drain;
memset(&drain, 0, sizeof(drain));
while (CheckInterrupted(result = waitid(P_PID, (id_t)siginfo.si_pid, &drain, WSTOPPED | WCONTINUED | WNOHANG)));
Comment on lines +1217 to +1219

@adamsitnik adamsitnik Sep 15, 2026

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.

This is valid feedback. I wish SA_NOCLDSTOP would just prevent us from getting here in the first place.

if (result != 0)
{
// Unable to consume the notification (e.g. the child changed state concurrently).
// Avoid spinning: report no exited child for now. A real exit will be observed on
// a subsequent SIGCHLD.
return 0;
}
continue;
}
else if (errno == ECHILD)
{
// The calling process has no existing unwaited-for child processes.
return 0;
}

// Unexpected error.
return result;
}
return result;
}

int32_t SystemNative_WaitPidExitedNoHang(int32_t pid, int32_t* exitCode, int32_t* terminatingSignal)
Expand Down
Loading