fix(diagnostics): Kill(entireProcessTree) walks the tree instead of one process group (#2031)

Kill(true) was ::killpg(pid, SIGKILL), which reaches the child's process GROUP -- and a
descendant that called setsid() has left it. Measured by #2028: the setsid grandchild was ALIVE
afterwards and the probe had to kill it itself. Landed under SA-5.

THE GATE WAS "/rv ABSENT", AND THE REFERENCE CORRECTS THE PROPOSED DESIGN THREE TIMES. #2031 was
blocked pending approval of plan 14.3's option A -- "ppid from /proc/*/stat, breadth-first from
the child pid, SIGKILL each". .NET's Process.KillTree (Process.Unix.cs:97-137) differs where it
counts:

  1. SIGSTOP COMES FIRST, before the children are enumerated. .NET's comment says why, verbatim:
     "Stop the process, so it won't start additional children. This is best effort: kill can
     return before the process is stopped." Option A enumerated and THEN killed, so a process
     that forked between those two steps left a survivor -- the very defect this ticket exists
     to remove, reintroduced one level down.
  2. THERE IS A SELF-GUARD. Kill(bool) refuses outright when the tree contains the calling
     process (Process.NonUap.cs:25-26), because attempting it kills the caller.
  3. ESRCH is ignored and other failures are collected, then reported together.

THE GUARD'S PLACEMENT IS LOAD-BEARING. It runs BEFORE the current-process no-op and before the
exit check, because that is where .NET has it. A first cut put it after, which made
GetCurrentProcess().Kill(true) a silent no-op where .NET throws AND left the guard unreachable
through any ordinary Process object; the test caught it.

Kill(false) is deliberately untouched -- .NET delegates it straight to Kill() with no tree logic
(Process.NonUap.cs:17-20). Its own pre-existing no-op-on-the-current-process divergence is not
bundled here.

Linux specificity: the walk reads /proc/<n>/stat, and so does .NET's (GetChildProcesses goes
through Process.GetProcesses(), a /proc reader on Unix). Without /proc the walk degrades to
killing the direct child. The stat parse scans to the LAST ')' rather than splitting on
whitespace, because field 2 is the executable name unescaped and may contain spaces and ')'.

Mutations: 6, 5 caught.

  - The "no recursion" mutation went UNCAUGHT at first, for an instructive reason: setsid()
    changes the SESSION, not the parent, so the original pin's "grandchild" is still an
    IMMEDIATE child of the shell and a one-level walk kills it.
    Fix2031_TheWalkIsTransitiveNotOneLevel goes three levels deep.
  - The SIGSTOP mutation is NOT caught and cannot be caught deterministically: it removes a race
    WINDOW -- the microseconds between ChildrenOf and the SIGKILL -- and the only test that would
    observe it forks in a tight loop and hopes to land inside that window, i.e. a flaky test.
    This session has repaired two of those (#2352, #2105) precisely because an intermittently
    green gate is not evidence. The SIGSTOP is there because .NET has it and states its purpose.
    The note is at the site.

Gate: 17,391 run, 17,391 passed, 0 failed, 0 skipped across 38 executables (+3, in
SharpRuntimeTests_Diagnostics, 229 -> 232). sizeof(Process) unchanged (one unique_ptr). Built in
build/ with --parallel 2. Downstream: zero Process code sites in cna and mobile-eggbert.

docs/Migration-ProcessKillEntireTree.md

Author: robertvokac

CLAUDE.md

@@ -0,0 +1,99 @@
+<!-- SPDX-License-Identifier: MIT -->
+<!-- Copyright (c) Robert Vokac and contributors -->
+
+# Migration — `Kill(entireProcessTree)` walks the tree instead of signalling a process group (ticket #2031)
+
+*2026-08-19.* `Kill(true)` was `::killpg(pid, SIGKILL)`, which reaches the child's process
+**group** — and a descendant that called `setsid()` has left it. Measured by #2028: the setsid
+grandchild was **alive** afterwards and the probe had to kill it itself.
+
+Landed under `docs/StandingApprovals.md` **SA-5**. No signature, layout or vtable change;
+`sizeof(Process)` is one `unique_ptr`, unchanged.
+
+---
+
+## 1. The gate was "/rv absent", and the reference corrects the proposed design three times
+
+#2031 was blocked pending approval of *"the /proc descendant walk"* described in its plan §14.3 —
+*"ppid from `/proc/*/stat`, breadth-first from the child pid, SIGKILL each"* — on the ground that
+the reference could not be read. It can, and .NET's `Process.KillTree` (`Process.Unix.cs:97-137`)
+differs in three ways that matter:
+
+1. **`SIGSTOP` comes first**, before the children are enumerated. .NET's comment says why,
+   verbatim: *"Stop the process, so it won't start additional children. This is best effort: kill
+   can return before the process is stopped."* Option A enumerated and **then** killed, so a
+   process that forked between those two steps left a survivor — the very defect this ticket
+   exists to remove, reintroduced one level down.
+2. **There is a self-guard.** `Kill(bool)` refuses outright when the tree contains the calling
+   process (`Process.NonUap.cs:25-26`, `InvalidOperationException`), because attempting it kills
+   the caller. Option A had no such check.
+3. **`ESRCH` is ignored and other failures are collected**, then reported together, because a
+   process may legitimately exit between any two steps of the walk.
+
+The recursion is depth-first in .NET rather than breadth-first; for killing, the two are
+equivalent, and the SIGSTOP ordering is not.
+
+## 2. What changed
+
+| Call | Was | Is |
+|---|---|---|
+| `Kill(true)`, setsid descendant | survived | killed |
+| `Kill(true)`, descendant three levels deep | survived | killed |
+| `Kill(true)` on the current process | silent no-op | `InvalidOperationException` |
+| `Kill(true)` with a partial failure | silent | `InvalidOperationException` naming each pid |
+| `Kill(false)` | `::kill(pid, SIGKILL)` | **unchanged** |
+| `Kill(false)` on the current process | silent no-op | **unchanged** (§3) |
+
+The self-guard runs **before** the current-process no-op and before the exit check, because that
+is where .NET has it. Placing it after would make `GetCurrentProcess().Kill(true)` a silent no-op
+where .NET throws, **and would leave the guard unreachable through any ordinary `Process`
+object** — a first cut did exactly that and the test caught it.
+
+## 3. One divergence deliberately not bundled
+
+`Kill(false)` on the current process is a silent no-op here and terminates the process in .NET.
+That is pre-existing, is on the `Kill()` overload rather than the tree one, and .NET reaches it by
+delegating `Kill(false)` straight to `Kill()` (`Process.NonUap.cs:17-20`) with no tree logic at
+all. Bundling it would be exactly the mixing this repository's records complain about elsewhere.
+
+## 4. Linux specificity
+
+The walk reads `/proc/<n>/stat` for the parent pid, so it is Linux-bound — and so is .NET's,
+whose `GetChildProcesses` goes through `Process.GetProcesses()`, itself a `/proc` reader on Unix.
+On a POSIX host without `/proc`, `opendir("/proc")` fails, `ChildrenOf` returns empty, and
+`Kill(true)` degrades to killing the direct child only. `System::Diagnostics::Process` is already
+documented POSIX-only and is exercised on Linux alone.
+
+The `/proc/<n>/stat` parse scans to the **last** `)` rather than splitting on whitespace, because
+field 2 is the executable name unescaped and may contain spaces and `)`.
+
+## 5. Evidence
+
+Six mutations, five caught:
+
+| Mutation | Result |
+|---|---|
+| back to `killpg` | caught |
+| the walk does not recurse (immediate children only) | caught — **after a case was added** |
+| `Kill(false)` also walks the tree | caught |
+| the self-guard never fires | caught |
+| the self-guard moved after the current-process no-op | caught |
+| **no `SIGSTOP` before enumerating** | **NOT caught — see below** |
+
+**The recursion mutation went uncaught at first**, and the reason is worth keeping: `setsid()`
+changes the *session*, not the parent, so the original pin's "grandchild" is still an **immediate
+child** of the shell and a one-level walk kills it. `Fix2031_TheWalkIsTransitiveNotOneLevel` goes
+three levels deep so the mutation survives it.
+
+**The `SIGSTOP` mutation is not caught and cannot be, deterministically.** What it removes is a
+*race window* — the microseconds between `ChildrenOf` and the `SIGKILL`, during which the target
+could fork a child that is neither enumerated nor killed. A test could only observe it by forking
+in a tight loop and hoping to land inside that window, which is a **flaky test**; this repository
+has repaired two of those this session (#2352, #2105) on the ground that an intermittently green
+gate is not evidence. The `SIGSTOP` is there because .NET has it and states its purpose, not
+because a test distinguishes it. The note is at the site.
+
+## 6. Downstream, measured
+
+`cna` and `mobile-eggbert` reference `System::Diagnostics::Process` in **zero** code sites.
+Neither was modified.

docs/Migration-ProcessKillEntireTree.md

@@ -12,6 +12,7 @@
 
 #include <chrono>
 #include <cstdlib>
+#include <fstream>
 #include <sstream>
 
 #if !defined(_WIN32) && !defined(__EMSCRIPTEN__)
@@ -22,6 +23,7 @@
 #  include <fcntl.h>
 #  include <cerrno>
 #  include <cstring>
+#  include <dirent.h>
 #  if defined(__APPLE__)
 #    include <crt_externs.h>
 #  endif
@@ -503,16 +505,139 @@ bool Process::Start() {
 #endif
 }
 
+
+#if defined(SHARP_RUNTIME_PROCESS_POSIX)
+namespace {
+
+    /**
+     * @brief The immediate children of @p pid, read from `/proc/<n>/stat`'s fourth field.
+     *
+     * Ticket #2031. Linux-specific by construction: `/proc` is where the parent link lives, and
+     * .NET's own `GetChildProcesses` is equally platform-bound (it goes through
+     * `Process.GetProcesses()`, whose Unix implementation reads `/proc`).
+     *
+     * `stat`'s second field is the executable name **in parentheses and unescaped**, so it may
+     * contain spaces and even `)`. Splitting on whitespace is therefore wrong; the parse scans to
+     * the LAST `)` and takes the fields after it, which is what every correct /proc reader does.
+     */
+    std::vector<pid_t> ChildrenOf(pid_t pid) {
+        std::vector<pid_t> children;
+        DIR* proc = ::opendir("/proc");
+        if (proc == nullptr) return children;
+        while (dirent* entry = ::readdir(proc)) {
+            char* end = nullptr;
+            const long candidate = std::strtol(entry->d_name, &end, 10);
+            if (end == entry->d_name || *end != '\0' || candidate <= 0) continue;
+
+            std::string path = std::string("/proc/") + entry->d_name + "/stat";
+            std::ifstream stat(path);
+            if (!stat) continue;
+            std::string line;
+            if (!std::getline(stat, line)) continue;
+            const size_t close = line.rfind(')');
+            if (close == std::string::npos) continue;
+            // After ")" come: state, ppid, ...
+            std::istringstream rest(line.substr(close + 1));
+            std::string state;
+            long ppid = 0;
+            if (!(rest >> state >> ppid)) continue;
+            if (static_cast<pid_t>(ppid) == pid) children.push_back(static_cast<pid_t>(candidate));
+        }
+        ::closedir(proc);
+        return children;
+    }
+
+    /** @brief True when @p pid is @p ancestor or any descendant of it. */
+    bool IsSelfOrDescendantOf(pid_t ancestor, pid_t pid) {
+        if (ancestor == pid) return true;
+        std::vector<pid_t> frontier{ancestor};
+        for (size_t i = 0; i < frontier.size(); ++i) {
+            for (pid_t child : ChildrenOf(frontier[i])) {
+                if (child == pid) return true;
+                frontier.push_back(child);
+            }
+        }
+        return false;
+    }
+
+    /**
+     * @brief .NET's `Process.KillTree` (`Process.Unix.cs:97-137`), transcribed.
+     *
+     * The ORDER is the substance, and it is what #2031's own proposed option A was missing.
+     * .NET stops the process **before** enumerating its children -- the comment says why,
+     * verbatim: *"Stop the process, so it won't start additional children. This is best effort:
+     * kill can return before the process is stopped."* Option A enumerated and then killed, so a
+     * process that forked between those two steps left a survivor: precisely the defect the
+     * ticket exists to remove, reintroduced one level down.
+     *
+     * `ESRCH` is ignored throughout, because a process may legitimately exit between any two
+     * steps of the walk; every other errno is collected and reported together.
+     *
+     * HONEST NOTE ON THE EVIDENCE: a mutation removing the SIGSTOP is NOT caught, and it cannot
+     * be caught deterministically. What it removes is a RACE WINDOW -- the microseconds between
+     * `ChildrenOf` and the `SIGKILL`, during which the target could fork a child that is neither
+     * enumerated nor killed. A test could only observe it by forking in a tight loop and hoping
+     * to land inside that window, which is a FLAKY test; this repository has repaired two of
+     * those this session (#2352, #2105) on the ground that an intermittently green gate is not
+     * evidence. The SIGSTOP is here because .NET has it and states its purpose in a comment, not
+     * because a test distinguishes it.
+     */
+    void KillTree(pid_t pid, std::vector<std::string>& failures) {
+        if (::kill(pid, SIGSTOP) != 0) {
+            if (errno != ESRCH)
+                failures.push_back("SIGSTOP to pid " + std::to_string(pid) + ": " + std::strerror(errno));
+            return;
+        }
+        const std::vector<pid_t> children = ChildrenOf(pid);
+        if (::kill(pid, SIGKILL) != 0 && errno != ESRCH)
+            failures.push_back("SIGKILL to pid " + std::to_string(pid) + ": " + std::strerror(errno));
+        for (pid_t child : children) KillTree(child, failures);
+    }
+
+}  // namespace
+#endif
+
 void Process::Kill() { Kill(false); }
 
 void Process::Kill(bool entireProcessTree) {
     (void)entireProcessTree; // unused on non-POSIX platforms, where this throws below
 #if defined(SHARP_RUNTIME_PROCESS_POSIX)
-    if (!impl_->started || impl_->isCurrentProcess) return;
+    if (!impl_->started) return;
+    // #2031: the self-guard runs FIRST, before the current-process no-op and before the exit
+    // check, because that is where .NET has it -- `Kill(bool)` refuses immediately, ahead of
+    // `KillTree` (`Process.NonUap.cs:23-28`). Placing it after the `isCurrentProcess` early
+    // return would make `GetCurrentProcess().Kill(true)` a silent no-op where .NET throws, and
+    // would leave the guard unreachable through any ordinary Process object.
+    //
+    // `Kill(false)` is deliberately untouched: .NET delegates it straight to `Kill()`
+    // (`Process.NonUap.cs:17-20`), and this port's own no-op-on-the-current-process behaviour
+    // for that overload is a separate, pre-existing divergence that is not bundled here.
+    if (entireProcessTree
+        && (impl_->isCurrentProcess || IsSelfOrDescendantOf(impl_->pid, ::getpid())))
+        throw System::InvalidOperationException(
+            "Cannot be used to terminate a process tree containing the calling process.");
+    if (impl_->isCurrentProcess) return;
     reapIfNeeded(*impl_);
     if (impl_->hasExited) return;
-    if (entireProcessTree) ::killpg(impl_->pid, SIGKILL);
-    else ::kill(impl_->pid, SIGKILL);
+    if (!entireProcessTree) {
+        ::kill(impl_->pid, SIGKILL);
+        return;
+    }
+    // #2031 (SR-AUD-273, cause D-E). This was `::killpg(pid, SIGKILL)`, which reaches the
+    // child's process GROUP -- and a descendant that called setsid() has left it. Measured: the
+    // setsid grandchild was ALIVE afterwards and the probe had to kill it itself.
+    //
+    // .NET walks the tree instead (`Process.Unix.cs:97-137`), and the SELF-GUARD below is
+    // .NET's too (`Process.NonUap.cs:25-26`): killing a tree that contains the calling process
+    // would kill the caller, so it is refused rather than attempted.
+    std::vector<std::string> failures;
+    KillTree(impl_->pid, failures);
+    if (!failures.empty()) {
+        std::string detail;
+        for (const auto& f : failures) { if (!detail.empty()) detail += "; "; detail += f; }
+        throw System::InvalidOperationException(
+            "Not all processes in process tree could be terminated. (" + detail + ")");
+    }
 #else
     throw System::PlatformNotSupportedException("System::Diagnostics::Process is only supported on POSIX platforms.");
 #endif

modules/diagnostics/src/System/Diagnostics/Process.cpp

@@ -48,6 +48,7 @@
 
 #include "System/Diagnostics/Process.hpp"
 #include "System/Diagnostics/ProcessStartInfo.hpp"
+#include "System/InvalidOperationException.hpp"
 
 using System::Diagnostics::Process;
 using System::Diagnostics::ProcessStartInfo;
@@ -278,22 +279,20 @@ TEST(ProcessGatedBehaviourPinTests, Fix2030_ConcurrentReadersSeeAConsistentBuffe
 // #2031 (SR-AUD-273) -- Kill(true) signals one process group.
 // ===========================================================================
 
-// PIN FOR BLOCKED TICKET #2031. Kill(entireProcessTree = true) is ::killpg, which reaches only
-// the child's process group; a descendant that called setsid() has left that group and
-// SURVIVES, despite the parameter's full-process-tree contract. Plan section 14.3 replaces the
-// killpg with a transitive /proc descendant walk on Linux, after which the survivor is killed
-// and this test fails.
+// INVERTED by #2031. Kill(entireProcessTree = true) was ::killpg, which reaches only the child's
+// process GROUP -- and a descendant that called setsid() has left it, so it SURVIVED despite the
+// parameter's full-process-tree contract. It is now .NET's recursive /proc descendant walk.
 //
-// The witness is a file rather than a pid: the surviving grandchild writes it after a delay,
-// so "the file exists" means "the grandchild was still alive after Kill(true)" without the
-// test having to track a pid through two setsid-ing layers.
-TEST(ProcessGatedBehaviourPinTests, Pin2031_SetsidDescendantSurvivesKillEntireProcessTree) {
+// The witness is a file rather than a pid: the grandchild writes it after a delay, so "the file
+// was never written" means "the grandchild was killed with the tree" without the test having to
+// track a pid through two setsid-ing layers.
+TEST(ProcessGatedBehaviourPinTests, Fix2031_ASetsidDescendantIsKilledWithTheTree) {
     const std::string witness = makeUniqueWitnessPath();
     ASSERT_FALSE(witness.empty());
 
-    // The direct child stays alive (exec sleep 5) so that Kill() does not short-circuit on an
-    // already-exited child, and spawns a grandchild in a NEW SESSION that writes the witness
-    // one second later.
+    // The direct child stays alive (exec sleep 5) so Kill() does not short-circuit on an
+    // already-exited child, and spawns a grandchild in a NEW SESSION that writes the witness one
+    // second later.
     const std::string script =
         "setsid /bin/sh -c 'sleep 1; printf alive > " + witness + "' >/dev/null 2>&1; "
         "exec sleep 5";
@@ -305,21 +304,79 @@ TEST(ProcessGatedBehaviourPinTests, Pin2031_SetsidDescendantSurvivesKillEntirePr
     process.Kill(true);
     ASSERT_TRUE(process.WaitForExit(5000)) << "the direct child was not killed";
 
-    // If the grandchild had been killed with the tree, it could never write the witness.
+    // Wait past the grandchild's own delay. If it had survived it would have written by now.
+    std::this_thread::sleep_for(std::chrono::milliseconds(2500));
+    EXPECT_FALSE(witnessWasWritten(witness))
+        << "the setsid descendant survived Kill(true) -- the walk did not reach it";
+
+    ::unlink(witness.c_str());
+}
+
+// DEPTH. The case above is only two levels deep -- setsid changes the SESSION, not the parent,
+// so the "grandchild" is still an immediate child of the shell and a one-level walk kills it.
+// This one is three levels deep, so a mutation that kills only immediate children survives it.
+TEST(ProcessGatedBehaviourPinTests, Fix2031_TheWalkIsTransitiveNotOneLevel) {
+    const std::string witness = makeUniqueWitnessPath();
+    ASSERT_FALSE(witness.empty());
+
+    // shell -> setsid shell -> shell -> the writer. The witness is written by a
+    // great-grandchild, which only a transitive walk reaches.
+    const std::string script =
+        "setsid /bin/sh -c '/bin/sh -c \"sleep 1; printf alive > " + witness + "\" & sleep 5' "
+        ">/dev/null 2>&1; exec sleep 5";
+
+    Process process = Process::Start(shellStartInfo(script));
+    std::this_thread::sleep_for(std::chrono::milliseconds(300));
+    process.Kill(true);
+    ASSERT_TRUE(process.WaitForExit(5000));
+
+    std::this_thread::sleep_for(std::chrono::milliseconds(2500));
+    EXPECT_FALSE(witnessWasWritten(witness))
+        << "a great-grandchild survived Kill(true) -- the walk stopped short";
+    ::unlink(witness.c_str());
+}
+
+// THE CONTROL: Kill(false) must NOT acquire the tree behaviour. .NET's Kill(bool) delegates
+// straight to Kill() for false (Process.NonUap.cs:17-20), so a mutation that routes both through
+// the walk is caught here rather than passing as an improvement.
+TEST(ProcessGatedBehaviourPinTests, Fix2031_KillFalseStillLeavesTheDescendantAlone) {
+    const std::string witness = makeUniqueWitnessPath();
+    ASSERT_FALSE(witness.empty());
+    const std::string script =
+        "setsid /bin/sh -c 'sleep 1; printf alive > " + witness + "' >/dev/null 2>&1; "
+        "exec sleep 5";
+
+    Process process = Process::Start(shellStartInfo(script));
+    std::this_thread::sleep_for(std::chrono::milliseconds(300));
+    process.Kill(false);
+    ASSERT_TRUE(process.WaitForExit(5000));
+
     const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(5);
     bool written = false;
     while (!written && std::chrono::steady_clock::now() < deadline) {
         written = witnessWasWritten(witness);
         if (!written) std::this_thread::sleep_for(std::chrono::milliseconds(50));
     }
-
-    EXPECT_TRUE(written)
-        << "the setsid descendant did NOT survive Kill(true) -- #2031 appears to have landed; "
-           "retire this pin with it";
-
+    EXPECT_TRUE(written) << "Kill(false) killed a descendant it was never asked to touch";
     ::unlink(witness.c_str());
 }
 
+// .NET refuses to kill a tree containing the caller (Process.NonUap.cs:25-26) rather than
+// attempting it, because attempting it kills the caller. The current process is its own
+// ancestor, so a Process object naming it must be refused -- and the message is .NET's,
+// transcribed from Strings.resx:342-344.
+TEST(ProcessGatedBehaviourPinTests, Fix2031_ATreeContainingTheCallerIsRefused) {
+    Process self = Process::GetCurrentProcess();
+    try {
+        self.Kill(true);
+        ADD_FAILURE() << "Kill(true) on the calling process was attempted";
+    } catch (const System::InvalidOperationException& e) {
+        EXPECT_NE(std::string(e.what()).find(
+                      "Cannot be used to terminate a process tree containing the calling process."),
+                  std::string::npos) << e.what();
+    }
+}
+
 // ===========================================================================
 // Layout pins (plan section 10) -- these are not gated, they are ABI tripwires.
 // ===========================================================================