fix(sweep): reap the whole MCP-launcher orphan tree, not just its root

[iamwhatever]

Problem / Motivation

The orphan sweep reclaims the marked MCP launcher at the top of an orphaned tree, then relies on surviving children reparenting to init so they become candidates themselves on a later pass. That fallback breaks on an unmarked intermediate: it is a candidate but not sweepable, so it lives forever and hides its own marked children behind a ppid that is not init, where _our_orphan_pids never enumerates them. A setsid-ing launcher escapes the killpg fast path the same way — its payload lands in a new process group.

Observed shape, produced by any launcher wrapper that resolves a package and then execs the resolved binary:

<wrapper> mcp start-server <pkg>      <- marked, swept
  -> <wrapper> mcp start-server ...   <- marked, swept
    -> node .../bin/<pkg>-server      <- UNMARKED, leaked
      -> npm exec <pkg>@latest        <- marked but unreachable

One host accumulated 112 such processes holding 15.2 GB RSS over 23 days, with the sweep running the whole time.

Why it matters

That backlog was enough to pin kirocrew-agents.slice at its memory.high ceiling. The kernel then throttled the whole subtree and every freshly-spawned adapter missed its 30s initialize deadline, so the leak surfaced as repeated Request initialize timed out after 30s on unrelated sessions — a symptom with no visible connection to its cause. Any host whose MCP launcher nests more than two levels deep accumulates the same debt silently.

Worth stating what is not broken: the probe's own teardown is correct. mcp_discovery killpg's the whole probe group on timeout, verified live — orphans killed by hand respawned and were reaped within two minutes. The leak comes from paths that skip that finally entirely: the gateway's os._exit(0) force exits and any SIGKILL, including a cgroup OOM-kill. Orphan ages cluster in groups of 2–6, matching PROBE_MAX_CONCURRENCY = 5 — each death stranded one in-flight probe fan-out.

What changed (motivation → approach → change)

Symptom: a marked orphan that _our_orphan_pids can never enumerate. Root cause: the sweep treats a launcher tree as a single PID, so reclaiming the root is where it stops; the documented "reclaimed on a subsequent sweep" fallback silently assumes every surviving descendant reparents to init, which an unmarked intermediate prevents.

kill_orphan_mcps now enumerates the subtree before signalling the root (once it dies the /proc parent links this walk needs are gone) and kills leftover members leaf-first — the same treatment _kill_orphan_work_tree already gives the work class, so this is an existing pattern extended to a second class rather than a new mechanism.

The root passing the sweep gate does not license killing arbitrary descendants, so identity is established twice over:

• The walk prunes a gateway/CLI entrypoint together with its whole subtree. Excluding only the entrypoint's own PID is not enough: the walk is flat, so a peer gateway's live workers would still be enumerated, and each carries KIROCREW_SPAWNED with no gateway marker in its own argv — so each would pass the per-member gate and be SIGKILLed, crashing that pod's active sessions. An unreadable argv prunes too: a process whose identity cannot be established is not a case for descending.
• Each surviving member is then re-verified on its own — KIROCREW_SPAWNED in its exec-time environ, not a _GATEWAY_MARKERS entrypoint, never self / group leader / pid <= 1 — and an unreadable cmdline is skipped rather than killed.

Members count against the same _ORPHAN_SWEEP_MAX_KILLS budget as roots, so a large backlog drains over several cycles instead of one unbounded burst, and the kill is SEL-audited (orphan_mcp_sweep / mcp_subtree).

The descendant walk is a local /proc read (_direct_child_pids) rather than an import of the ACP layer's equivalent: check_agent_sdk_boundary.py offers no opt-out marker and its baseline only shrinks, so a sweep here must not grow an ACP-layer edge. _kill_orphan_work_tree keeps its own baselined import untouched.

Linux-only in effect, since _env_has_kirocrew_marker is fail-closed elsewhere — matching the existing work-class floor. Windows is an explicit no-op: the tree kill after a session ends already went through taskkill /T.

Tests

26 in test/test_orphan_mcp_subtree.py:

• The leaked shape — a marked leaf behind an unmarked parent is reaped; the blocking intermediate dies too.
• Gateway-subtree prune — a peer gateway's live worker is never enumerated, the gateway itself is not, and a sibling outside that subtree still is.
• Per-member identity — a descendant without KIROCREW_SPAWNED is spared; a _GATEWAY_MARKERS entrypoint is spared.
• Ordering — leaf-first kills, and enumeration strictly precedes the root signal.
• Budget — subtree members share the global _ORPHAN_SWEEP_MAX_KILLS cap with roots.
• Fail-closed paths — vanished descendant, unreadable argv, kill_pid failure not counted, SEL audit emitted only when something dies.
• Walk semantics — preorder, cycle termination, Windows no-op.

Mutation-verified in five directions: removing the reap fails 5, removing the gateway-subtree prune fails 2, flipping the prune's fail-closed branch fails 1, removing the cycle guard fails 1, reversing the leaf-first order fails 1.

_direct_child_pids, _pid_cmdline and _kill_orphan_mcp_descendants are added to BENIGN_SPAWNS alongside their three sibling sweep functions (_our_orphan_pids, find_orphan_mcp_candidates, kill_orphan_mcps) — same justification: the argv is a fixed ps/pgrep probe with a PID as its only variable, never agent-influenced.

Manual verification

Verified against the live leak on the reporting host: 112 orphans reclaimed by exact PID (~14.8 GB freed, load average 66 → 33), and the probe teardown confirmed still working — two trees respawned within 30s and were gone within two minutes, which is what ruled the timeout path out as the cause.

Local gates: all 34 backend gates green. Full backend suite 84,540 passed / 103 failed, none attributable — AF_UNIX path too long from the worktree's long path, the 4-worker xdist budget, and the known pre-existing test_host_isolation_floor set. Neither test_pid_lifecycle nor the new file appears among them.

Related Issues

no linked issue: found while diagnosing a live host, not filed first.

Pattern harvest

Rule candidate: review-prompt

Pattern: a reaper that excludes a process by matching its OWN argv, while iterating a flat descendant list, still reaps that process's children — the exclusion has to prune the subtree during traversal, not filter at the kill step.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable) — behaviour is documented in the functions' own docstrings; no user-facing doc surface changed
• No secrets, credentials, or internal references in the diff

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of ecf1358bc0fd6bdecd39241498e6e74d90169400 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Root-cause fix with a correct, deliberately ordered kill sequence; the fail-closed identity discipline is proportionate to the SIGKILL-a-live-process harm it prevents.

Watch

• The budget-spares-root rule doesn't extend to per-member spares: a live descendant skipped for unprovable identity (scrubbed KIROCREW_SPAWNED environ, or pruned on unreadable cmdline during the walk) reparents to init unsweepable once the root is killed the same pass — the exact unreachable-intermediate shape this PR closes, recreated for that branch. Deferring the root (as the budget path already does) whenever a provably-live member was spared would keep the handle.

Suggestions

• The module now carries two leaf-first tree reapers with divergent rigor: _kill_orphan_work_tree enumerates via acp.client._get_child_pids and kills with no token or parent-edge guard, while the new MCP path has both. Backport the single-stat parent-edge+token guard to the work class (follow-up PR) or unify enumeration on _build_child_map.

[DESIGN-REVIEWED] ecf1358

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of ecf1358bc0fd6bdecd39241498e6e74d90169400 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] ecf1358

False positive or not applicable? A repository writer can comment:
/ai-review override gpt ecf1358bc0fd6bdecd39241498e6e74d90169400: <one-sentence reason>

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of ecf1358bc0fd6bdecd39241498e6e74d90169400 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All verification complete. The shipped walk reuses the existing _build_child_map (session_pid.py:2583), the token format matches platform_compat.get_process_start_id (both return stat field 22 after the last )), and I confirmed _direct_child_pids appears nowhere in the repo or diff while BENIGN_SPAWNS (test_spawn_audit.py:1203-1205) is untouched — the description's account of that surface is stale. Here is the review.

First-Principles-Verdict: CONCERNS

Every item traces to the counted 112-process/15.2 GB leak, but the description narrates an implementation that did not ship, and the same recycle hole stays open in the work-class sibling.

What this change ships

Intent: stop orphaned MCP launcher trees from leaking forever behind an unmarked intermediate. FIX.

1. A swept MCP orphan's whole subtree now dies leaf-first, not just its root — justified
2. Subtree enumerated before the root signal; root spared when the kill budget exhausts — justified
3. Peer gateway/dev-pod subtrees pruned from the walk, their workers never enumerated — justified
4. Descendants killed only on positive identity (env marker, no gateway argv, start-token match) — justified
5. Root itself re-verified (token, argv, pgid) immediately before its own signal — undeclared, derived
6. Stale snapshot edges dropped via a live-PPid re-read per child — undeclared, derived
7. New SEL audit event orphan_mcp_sweep / mcp_subtree — justified
8. Descendant reap is an explicit no-op off Linux (fail-closed marker) — justified
9. test_pid_lifecycle.py pruned from the black baseline plus format-only hunks — rides along, sanctioned by AGENTS.md

Watch

• The description is contradicted by the diff on the spawn surface: "_direct_child_pids, _pid_cmdline and _kill_orphan_mcp_descendants are added to BENIGN_SPAWNS… a fixed ps/pgrep probe" — grep _direct_child_pids: 0 hits anywhere; BENIGN_SPAWNS is unchanged; _pid_cmdline deliberately spawns nothing. The shipped walk reuses _build_child_map. Also "26 tests" vs ~54 in the file. The diff is more conservative than described, but an auditor reading the PR record gets a false account — refresh the description.
• Counted unfixed sibling: _kill_orphan_work_tree (src/kiro_crew/session_pid.py:2501) kills [*reversed(descendants), pid] with no start-token or live-PPid guard — the same PID-recycle hole this PR builds a guard for, which its own tests call "the third instance of this defect class". 1 sibling; accepted-and-deferred, since porting the token guard there is genuinely larger than this change.
• Items 5 and 6 are derived (this change itself widens the eligibility→signal gap) but undeclared; the description's "What changed" should own them.

[FIRST-PRINCIPLES-REVIEWED] ecf1358

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed ecf1358bc0fd6bdecd39241498e6e74d90169400 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] ecf1358

Verdict parsed from the review's SHA-scoped output markers for commit ecf1358bc0fd6bdecd39241498e6e74d90169400.

False positive or not applicable? A repository writer can comment:
/ai-review override fable ecf1358bc0fd6bdecd39241498e6e74d90169400: <one-sentence reason>

[iamwhatever]

Recycled descendant PID can kill a live worker — span=99fb501fe250 — fixed in 23020ba.

Root killpg reaps descendant -> PID is reused by a new marked worker -> stale descendant entry SIGKILLs the active worker.

Legitimate and reachable. Fixed as suggested, and then one step further, because pinning the token at enumeration alone still left a window.

_orphan_descendants now returns (pid, start_token) pairs, and _kill_orphan_mcp_descendants re-reads the token and requires it to match before signalling. A token missing on either side is treated as unproven identity and the member is skipped rather than killed — never as a mismatch, per _pid_start_token's documented contract — and is re-reaped next sweep, with a debug line so a host where identity never resolves is diagnosable instead of a silent no-op.

The window your fix names is not the only one: child_map is a snapshot reused across candidate roots in a sweep, so an edge can already be stale when the walk reads it. Capturing the token there would have paired a dead process's parent edge with a recycled PID's fresh token, which then matches at kill time. So the walk also re-reads each child's live PPid and requires it to still equal the parent it was traversed from, with the PPid and the token taken from ONE /proc/<pid>/stat read so they cannot describe two different processes.

Pinned by TestPidRecycleGuard (match / mismatch / either-token-None / both-None) and TestStaleMapEdge. Mutation-verified: dropping the token comparison fails 1, dropping the unproven-identity guard fails 1, dropping the live parent-edge check fails 3, and reading the wrong stat field fails 1.

[iamwhatever]

/proc/<pid>/task/*/children is the mechanism _build_child_map deliberately abandoned — span=aa990b4ba818 — fixed in 23020ba.

the new subtree walk reads (task / "children").read_text() (/proc/<pid>/task/*/children), the exact mechanism _build_child_map in this same file deliberately abandoned because it requires CONFIG_PROC_CHILDREN and is documented reliable only for frozen/stopped tasks

Correct, and worse than advisory in this context: an incomplete child set means silently dropped descendants, which is the exact bug class this PR exists to close. On a kernel without CONFIG_PROC_CHILDREN the fix would have no-opped with no signal.

_direct_child_pids is deleted. Enumeration now traverses _build_child_map() — the authoritative /proc/<pid>/stat PPid scan — built at most once per sweep and only when a marked orphan is actually confirmed, so a sweep that finds nothing pays for no /proc pass. Verified: no task/*/children path remains in the diff.

Pinned by TestOrphanDescendantWalk (preorder over a map wide enough that push order is observable, cycle termination, token capture) and test_child_map_is_built_once_per_sweep / test_child_map_not_built_when_no_mcp_orphan_is_confirmed.

[iamwhatever]

_direct_child_pids duplicates _build_child_map, same file — fixed in 23020ba.

and the existing one is the recorded winner ... Silently dropped descendants is this PR's own bug class: on a kernel without CONFIG_PROC_CHILDREN the fix no-ops with no signal. Subtraction: delete _direct_child_pids; build the map once per sweep with _build_child_map() and traverse it in _orphan_descendants.

Taken in full, including all three subtractions. Every one of them removed code:

• _direct_child_pids deleted; _orphan_descendants traverses _build_child_map(), built once per sweep and only when a marked orphan is confirmed.
• _kill_orphan_mcp_descendants calls _pid_cmdline instead of repeating the read, so that docstring's "both callers" is now true.
• _pid_cmdline returns b"" off Linux with no ps branch, for the reason you gave: _env_has_kirocrew_marker is fail-closed there, so the branch fed a gate that always refuses.

Consequence: all three BENIGN_SPAWNS entries are gone and test/test_spawn_audit.py is back to zero diff — the reap adds no subprocess spawn at all now.

Two further subtractions of the same kind, from your "unreachable production branches" note: the helper's IS_WINDOWS early return is deleted (the sole caller already returns on Windows, and every member additionally needs the fail-closed marker check), and the walk's redundant top-level cycle branch went earlier for the same reason — mutation testing showed the per-child check already covered it.

The pid-safety guards (target <= 1, self, group leader, == root) are kept deliberately: the helper takes a caller-supplied list, and test_root_is_not_signalled_twice and test_never_signals_pid_one_or_self exercise them against lists the walker would not produce.

One thing your review did not reach, found while fixing this: the once-per-sweep map is a snapshot reused across candidate roots, so an edge can be stale when the walk reads it. Each child's live PPid is now re-verified against the parent it was traversed from, taken from the same stat read as its start token. The walk is also iterative now — RecursionError on a deep chain would fire before the root is signalled and abort the whole sweep every cycle.

[iamwhatever]

Tree scan makes the root PID check stale — span=99fb501fe250 — fixed in 553d577.

Orphan exits during scan -> PID is reused by an active MCP -> stale killpg/kill terminates the active process.
Fix: capture the root start token before scanning and revalidate token, cmdline, and PGID immediately before signaling.

Legitimate, reachable, and mine: the cmdline re-read sat adjacent to the signal while the branch only called getpgid in between, and enumerating the subtree put a full /proc pass plus a stat per member in that gap.

This span has now been raised three times, so this round is a restructure rather than a third point-fix. Each instance named a different PID this sweep can signal after its identity went stale — descendants, then the descendants' parent edges, now the root. Patching a third instance invites a fourth, so the rule is stated once and applied to every signal site:

No signal is delivered to any PID whose start identity was not captured BEFORE this function's slow work and re-confirmed IMMEDIATELY before the signal.

The marked-MCP branch has exactly three signal sites, and all three now satisfy it:

#	Site	Covered by
1	os.killpg(pgid) — root's group	token captured pre-scan, re-confirmed pre-signal, plus a pgid re-check
2	os.kill(pid) — root PID	same guard, same placement
3	platform_compat.kill_pid(target) — each descendant	token pinned at enumeration + live PPid re-verified against the traversed parent

I took token and PGID as you asked, and deliberately not the cmdline re-read: once the start token matches it is provably the same process, so its argv cannot have changed, and _pid_cmdline returns b"" off Linux by design (its consumers all sit behind the fail-closed marker check), so comparing it there would break the macOS root-kill path — which is reachable, since _is_orphan_mcp matches on cmdline alone. The pgid re-check earns its place for the opposite reason: the token proves the process but not that it is still in the group this killpg targets.

Pinned by TestRootRecycleGuard: stable identity signals, changed token does not, either token None does not, both None does not, a process that left its group does not, and the capture order is asserted as token → scan → token. Mutation-verified: dropping the root guard fails 3, dropping its unproven-identity branch fails 1, dropping the pgid re-check fails 1.

Five existing kill_orphan_mcps tests in test_pid_lifecycle.py gained an autouse fixture supplying a stable token — they drive synthetic PIDs with no /proc entry, so the new guard correctly refused to signal them. That file is now black-clean, so its baseline entry is pruned.

[iamwhatever]

Root identity is captured after its stale cmdline — span=99fb501fe250 — fixed in 85f3aa2.

Orphan exits after cmdline read -> PID is reused -> stale cmdline licenses signalling the active replacement.
Fix: capture the token before cmdline validation, then revalidate token, cmdline, and PGID immediately before signalling.

Taken in full. root_token = _pid_start_token(pid) is now the first thing the loop does with a PID — ahead of the cmdline read, the eligibility predicate and getpgid. Your framing is what made the earlier placements wrong and this one right: the cmdline, the eligibility verdict and the pgid are all evidence about whichever process held the PID when each was read, so a capture after any of them leaves a window where both token reads see the replacement and agree, while the argv licensing the kill belonged to the dead process. There is no earlier point left, so this closes the sequence rather than shrinking the window again.

Before the root signal the full evidence set is revalidated, not just the token: _is_sweepable_orphan_mcp is re-run on a freshly read argv (the token proves the process, not that its argv still qualifies) and the pgid is re-read (a process can leave the group the killpg targets).

The residual is now revalidate → kill, which is irreducible with signal-based killing; closing it needs pidfd_open + pidfd_send_signal, a different change to a different layer.

Pinned by TestRootRecycleGuard and TestRootEvidenceRevalidated. Mutation-verified: moving the capture back after the cmdline read fails 25 tests, and dropping the eligibility revalidation or the pgid re-check each fail 1.

[iamwhatever]

Root death before the subtree budget is exhausted strands survivors — span=99fb501fe250 — fixed in 85f3aa2.

(Second of the two blocking findings on that head. They share a span id, so this is a separate record carrying the separate rationale.)

Escaped tree exceeds the cap -> root dies first -> surviving unrecognized ancestor reparents to init and permanently hides its marked children, allowing resource exhaustion.
Fix: reap descendants before the root and leave the root alive whenever the subtree budget is exhausted.

This is the best finding on the PR: my ordering re-created the exact leak the PR exists to close. The root was signalled first, the descendants got whatever budget remained, and once the tree exceeded _ORPHAN_SWEEP_MAX_KILLS the survivors could include the UNMARKED intermediate — which reparents to init, is not sweepable, and hides its marked children behind a non-init ppid. The same 112-process shape, re-formed by the fix.

Both halves applied. Descendants are reaped first, so they get the whole remaining budget instead of cap - 1. And the root is left alive and unsignalled whenever the counter has reached _ORPHAN_SWEEP_MAX_KILLS after the subtree: the root is the handle on the tree, marked and sweepable, so while it lives the remainder stays re-enumerable next sweep with a fresh budget. Signalling it is what loses the handle.

That reframes the group signal as belt-and-braces for anything still sharing the root's group, which is the honest description — a setsid-ing launcher escapes it entirely, which is why the explicit walk exists at all. The reasoning is in the code as a comment stating the ordering is load-bearing, not stylistic, so a later refactor does not quietly invert it.

Pinned by TestDescendantsBeforeRoot and TestBudgetExhaustionSparesRoot (root spared over the cap, root reaped when budget remains). Mutation-verified: deferring the descendants past the root fails 8 tests, and dropping the budget-spares-root rule fails 1. The pre-existing budget test was updated — it asserted cap - 1 on the old ordering, which was the defect encoded as an expectation.