fix(session): kill leaked provider process trees

[rubencu]

Problem / Motivation

The leaked-provider fallback could leave a kiro-cli runtime alive after its provider object was discarded. On sandboxed launches, the PID exposed by the provider can be a launcher whose descendants include the real runtime and MCP processes. _sync_kill_provider signalled only that launcher PID, so the launcher exited while descendants were reparented to init and escaped every provider teardown path.

The existing orphan detector then reported a managed runtime carrying the spawn marker but present in neither PID file, because the discarded provider had already lost the ownership handle needed to reclaim that tree.

Why it matters

Each escaped agent runtime keeps its process tree and memory alive until manual cleanup or host restart. Repeated provider discards can therefore accumulate invisible background processes and progressively consume host resources.

What changed (motivation → approach → change)

The fallback already has a positively identified provider PID and exists specifically to reap a leaked provider. Instead of signalling only that PID, _sync_kill_provider now routes both POSIX TERM/KILL escalation and the Windows hard-kill path through platform_compat.kill_process_tree.

On POSIX, the provider launcher is created as an isolated process-group leader, so group signalling reaches the launcher and descendants even after the launcher exits during TERM grace. On Windows, the existing compatibility helper uses recursive process-tree termination. Invalid/sentinel PIDs and existing error handling remain unchanged.

The audit message now says that a process tree was killed, matching the operation performed.

Tests

• Added a real POSIX regression that starts an isolated launcher with a SIGTERM-ignoring runtime child and verifies provider cleanup leaves neither process alive.
• Updated _sync_kill_provider unit coverage to require kill_process_tree for POSIX escalation, already-dead processes, Windows cleanup, and invalid PID rejection.
• Updated session-pool discard coverage to assert sentinel PIDs never reach the tree-kill helper.
• test/test_pid_lifecycle.py + test/test_session_pool.py: 247 passed.
• Mutation proof: PROVEN; reverting the production hunk makes the new regression assertion fail.
• Black, isort, flake8, and mypy: passed.
• Diff-scoped backend suite: 83,686 passed; the remaining failures were unrelated platform-composition and overlong local socket-path environment failures outside the changed files.

Manual verification

N/A — the regression test exercises a real isolated launcher/process group and a child that survives TERM, which covers the process-lifecycle behavior directly.

Related Issues

no linked issue: the detector introduced during #2930 exposed this additional teardown path after that investigation closed.

Pattern harvest

Rule candidate: review-prompt
Pattern: a lifecycle fallback that owns a launcher PID must reap the isolated process tree, not only the launcher process.

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) — N/A; no user-facing contract changed
• No secrets, credentials, or internal references in the diff

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• PR #7871 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7871: KEEP. Independent change in a different function; can merge in either order. Files: src/kiro_crew/session_pid.py. The two independent directions used different labels; the matrix conservatively retains OVERLAPPING for coordination.
• This PR is OVERLAPPING with PR #8039. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #8276: KEEP. Independent fixes; only a one-line import-block adjacency in the shared test file. Files: src/kiro_crew/session_pid.py. The two independent directions used different labels; the matrix conservatively retains OVERLAPPING for coordination.
• This PR is OVERLAPPING with PR #8304. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #8276: KEEP. Complementary halves of the same leak class on two different code paths; landing either does not make the other unnecessary. Files: src/kiro_crew/session_pid.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

[bolichen97]

Rebased onto main 41dcadf2 by a maintainer as part of the 2026-09-08 open-PR audit. The branch was 531 commits behind its merge base.

Clean rebase, no conflicts. The one extra change: black (pinned, line-length 100) reformats the new assert not platform_compat.pid_exists(child_pid), "..." in test_posix_kills_sandbox_launcher_descendants to the call-split form, so that hunk was reformatted. test/test_session_pool.py is still not black-clean, but it was already listed in .github/black-baseline.txt before this PR, so it was left untouched.

Gates run locally on the changed files only: black, isort, flake8 all clean; pytest test/test_pid_lifecycle.py::TestSyncKillProvider test/test_session_pool.py::TestDiscardReaping -> 15 passed.

Please review the resolution. A maintainer push makes the maintainer the last pusher, so under the repo's last-push rule a second approver is needed. Reply if anything looks wrong.

[github-actions]

Design Review (Fable 5, fork) — 🟡 CONCERNS

Design-level review of cb43261dca4287a074bebe45a894366bc3b4f756 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

I have everything I need. The core premise is verified (ACP providers spawn with start_new_session=IS_POSIX at acp/client.py:5112 and acp/runtime.py:1366, so group-kill is well-aimed), and kill_process_tree has a safe self-pgid degrade guard. The one design residual is that the TERM→KILL escalation loop remains keyed on the launcher PID's liveness rather than the group's.

Design-Verdict: CONCERNS

Right layer and right fix, but the TERM→KILL escalation is still keyed on launcher-PID liveness, so a narrow window of the original leak survives.

Watch

The escalation loop skips SIGKILL exactly when it's needed: after kill_process_tree(pid, SIGTERM) the launcher exits; if it is reaped during the grace step (os.waitpid(pid, WNOHANG) — or the gateway's asyncio child watcher reaps it first, raising ChildProcessError → return), the SIGKILL round hits os.getpgid(pid) → ProcessLookupError → return # already dead, leaving a TERM-ignoring runtime alive — the PR's own regression scenario. The description's claim that "group signalling reaches the launcher and descendants even after the launcher exits during TERM grace" holds only while the launcher lingers as an unreaped zombie; nothing pins that. The same window makes test_posix_kills_sandbox_launcher_descendants a timing-dependent flake on loaded CI.
Clears when: the POSIX path resolves the pgid once before signalling and keys TERM→grace→KILL on the group (e.g. killpg(pgid, 0) liveness), not on the launcher PID.

Suggestions

• Treat ProcessLookupError from the SIGKILL round as "launcher gone, group may live" rather than terminal: retry the KILL against the cached pgid before returning.

[DESIGN-REVIEWED] cb43261

[github-actions]

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of cb43261dca4287a074bebe45a894366bc3b4f756 via the fork AI-review pipeline — 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 facts are verified. The fix is mechanism-level, uses the existing repo-wide kill_process_tree helper, and its regression test fails on base by construction. But I confirmed one genuine unfixed sibling: _kill_orphan_pid (src/kiro_crew/subagent.py:1830, called from subagent_manager/monitoring.py:100) single-PID-SIGKILLs the same kind of runtime PID (runtime.pid, spawned as a group leader at acp/runtime.py:1366) — the exact pattern the PR's own "pattern harvest" names as the rule.

First-Principles-Verdict: CONCERNS

The harvested rule already has a counted unfixed sibling: _kill_orphan_pid still single-PID-kills the same group-leader runtime PID this PR stops mis-killing.

What this change ships

Intent: stop a discarded provider's sandbox launcher from orphaning its kiro-cli/MCP descendants at teardown — a FIX.

Inventory (4 items)

1. Leaked-provider cleanup on POSIX now signals the launcher's whole process group, TERM then KILL — justified
2. Leaked-provider cleanup on Windows now taskkills the whole tree (/T /F), not one PID — justified
3. Cleanup audit log now says a PID "tree" was killed — justified
4. New POSIX regression: a TERM-ignoring runtime child must die with its launcher — justified

Watch

• Point patch with 1 counted unfixed sibling. Grepped kill_pid\( in src/kiro_crew: 3 remaining call sites; sandbox.py:2570 (direct probe child) and ssh_tunnel_manager.py:951 (explicit non-tree branch) are deliberate, but _kill_orphan_pid (subagent.py:1833, invoked at subagent_manager/monitoring.py:100) SIGKILLs a subagent runtime.pid — the same group-leader PID spawned at acp/runtime.py:1366 — so its descendants escape identically. The PR's own pattern harvest states the general rule this site still violates, and kill_process_tree's own-pgid guard makes the one-line rerouting safe.
  Clears when: _kill_orphan_pid routes through platform_compat.kill_process_tree, or the author shows the orphan-recovery PID cannot own live descendants.

[FIRST-PRINCIPLES-REVIEWED] cb43261

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed cb43261dca4287a074bebe45a894366bc3b4f756 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

The sole candidate relies on a reap race between the SIGTERM iteration's non-blocking os.waitpid(pid, os.WNOHANG) and the SIGKILL iteration's os.getpgid(pid) inside kill_process_tree. For the child to survive, the launcher P must both exit and be fully reaped in the gap between two consecutive statements (so getpgid raises ProcessLookupError and aborts before the group SIGKILL). Tracing the actual code: an orphaned process group persists while the child is a member, and killpg still reaches a zombie leader's group — the escalation only fails if P is fully reaped. The only reaper on this synchronous path is the immediately-following WNOHANG waitpid, which runs before the just-signaled launcher has been scheduled to exit; establishing any wider window requires an external async child-watcher the candidate admits it "could not confirm" is active — i.e. assuming code not opened. The authors' own test_posix_kills_sandbox_launcher_descendants depends on this same race never firing and is expected to pass, confirming the launcher does not exit within that gap in practice. Outcome (c) resolves to "might," below the bar. Dropped.

[OPUS-REVIEWED] cb43261

[github-actions]

GPT 5.6 Review (fork) — 🔴 changes requested (blocking)

Reviewed cb43261dca4287a074bebe45a894366bc3b4f756 via the fork AI-review pipeline; updated in place on each push.

2 of 2 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands.

BLOCKING -- src/kiro_crew/session_pid.py:637 -- escalation loses the process group when the launcher exits
platform_compat.kill_process_tree(pid, sig)
TERM -> launcher exits and is reaped -> SIGKILL re-resolves its dead PID -> descendants survive and repeated leaks can exhaust memory.
Anchor: residual/crash-data-loss-corruption
Fix: Preserve the POSIX process-group ID across escalation, or use one tree-wide SIGKILL.

BLOCKING -- test/test_pid_lifecycle.py:1066 -- file-writing child inherits the repository working directory
launcher = subprocess.Popen(
..., start_new_session=True)
Test run -> child starts without cwd -> file-writing process tree inherits the checkout as mutable working directory.
Anchor: no-test-side-effects
Fix: Pass cwd=tmp_path.

[BLOCK-MERGE] cb43261
[GPT-REVIEWED] cb43261

Adjudication (Opus 4.8) — is blocking on each finding proportionate?

Both findings are FENCED (annotate-only); the adjudicable block is empty.

F1 — session_pid.py:637, escalation loses the process group when the launcher is reaped between SIGTERM and SIGKILL.
The losing path is real: kill_process_tree re-resolves os.getpgid(pid) (platform_compat.py:3294) on each iteration, so once the launcher pid is reaped, the SIGKILL iteration raises ProcessLookupError caught at session_pid.py:635 and never reaches the surviving group. The reap window (killpg at 634 → waitpid(WNOHANG) at 642, no intervening sleep) is narrow, and an out-of-band asyncio child watcher could still win it. The periodic reaper _cleanup_orphaned_mcp_servers (session.py:66, ~5 min) does kill tracked MCP children reparented to init (session_pid.py:704-711) — but the finding's named primary target, the kiro-cli sandbox-root process, is a bare PID line that the reaper only prunes when already dead, never kills (session_pid.py:681-682). I cannot complete a recovery record for the primary leaked process. When torn on a fenced/unbounded finding → UPHOLD-FENCED.

F2 — test_pid_lifecycle.py:1066, file-writing child inherits the repo working directory.
The cwd is inherited (no cwd= passed), but the harm requires a relative-path write into the checkout, and the test's own source performs none: child_pid_file = tmp_path / "runtime.pid" (patch:123) is passed as str(child_pid_file) (patch:133) and written via open(sys.argv[1], 'w') — an absolute tmp_path target (patch:124-130); the grandchild does only signal.SIG_IGN + sleep with zero file I/O (patch:128). No write can land in the checkout. The condition that reaches the harm is not producible by the test as written — a complete FLAG record.

[ADJUDICATION] cb43261dca4287a074bebe45a894366bc3b4f756 total=0 uphold=0 downgrade=0
[GPT-ADJUDICATED] cb43261dca4287a074bebe45a894366bc3b4f756
[ADJUDICATION-FENCED] cb43261dca4287a074bebe45a894366bc3b4f756 fenced=2 flagged=1
UPHOLD-FENCED F1 src/kiro_crew/session_pid.py:637 -- SIGKILL re-resolves getpgid on the reaped launcher pid and returns before reaching the surviving group; the kiro-cli sandbox root has no killing recovery path (only pruned when already dead), so no recovery record can be completed.
FLAG F2 test/test_pid_lifecycle.py:1066 -- cwd is inherited but every file write targets an absolute tmp_path (patch:123,133) and the grandchild does no file I/O (patch:128), so the relative-path write needed to pollute the checkout is not producible by the test as written.
[GPT-ADJUDICATED-FENCED] cb43261dca4287a074bebe45a894366bc3b4f756

🏷️ Fenced finding(s) machine-flagged as likely edge case

The security fence keeps these findings blocking regardless of adjudication; the only clearance path is a human override recorded by a repository writer, who must independently verify a rationale before recording it — it is machine-authored, and a wrong override on a security-class finding ships exactly the class the fence exists to stop. (This lane's comment deliberately carries no override command.)

• F2 test/test_pid_lifecycle.py:1066 — cwd is inherited but every file write targets an absolute tmp_path (patch:123,133) and the grandchild does no file I/O (patch:128), so the relative-path write needed to pollute the checkout is not producible by the test as written.