fix(teardown): wait for the session's process tree before worktree teardown (#183) - #184
Merged
Conversation
…ardown (#183) Harvest pane pids plus their descendant tree (with pid-reuse identities) before the first kill strike, then reap identity-confirmed stragglers within teardown_grace_s once the window dies — a setsid/double-fork survivor can no longer race the worktree merge+removal. Adds the ProcessHost.descendants seam (/proc walk on Linux, psutil elsewhere, never raises) and mirrors the reap in opencode_http's _kill_process.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughProcess-host implementations now discover descendant trees, while the GenericTmuxAdapter and OpenCode adapter harvest identities before teardown and reap confirmed survivors within bounded grace periods. Tests and documentation cover PID reuse, uncertain identities, detached processes, and worktree teardown ordering. ChangesProcess-tree teardown
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Session as Session teardown
participant Host as ProcessHost
participant Window as tmux window
participant Tree as Descendant processes
Session->>Host: Harvest pane PIDs and identities
Session->>Window: Kill window
Window-->>Session: Window exits or grace expires
Session->>Host: Check harvested survivors
Host->>Tree: Terminate identity-confirmed survivors
Host->>Tree: Force-kill remaining confirmed survivors
Session-->>Session: Proceed to worktree teardown
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bmad_loop/adapters/generic.py`:
- Around line 789-808: The straggler-reap flow must never signal descendants
whose identity is None. In src/bmad_loop/adapters/generic.py lines 789-808,
filter unconfirmed entries before terminate, polling, and forced escalation,
using only confirmed identities for all signaling decisions. In
tests/test_generic_tmux.py lines 314-331, update coverage to assert that an
unconfirmed PID receives no signal.
In `@src/bmad_loop/adapters/opencode_http.py`:
- Line 994: Update the process-host handling around get_process_host() to catch
ProcessHostError, perform the existing legacy Popen root strike before
re-raising the error, and ensure teardown still propagates the failure. Add the
equivalent regression test for this invalid process-host override scenario,
matching the coverage already present for the tmux path.
In `@src/bmad_loop/process_host.py`:
- Around line 243-283: Make descendant discovery return a PID-to-identity
snapshot rather than bare PIDs: update _linux_descendants and
_psutil_descendants to capture each descendant’s identity while enumerating
ancestry, preserving the never-raise behavior. In src/bmad_loop/process_host.py
lines 243-283, define and propagate the mapping; in
src/bmad_loop/adapters/generic.py lines 724-728, consume the captured identities
without reacquiring them. In tests/test_process_host.py lines 191-229, add
coverage for enumerating generation A followed by PID reuse as generation B.
In `@tests/test_generic_tmux.py`:
- Around line 314-331: The test
test_kill_never_force_kills_identity_none_straggler currently expects
termination of an unconfirmed PID. Update its assertions so PID 200 receives
neither terminate nor force-kill, while preserving the existing lifecycle
outcome checks and recording it as unconfirmed if that behavior is exposed.
In `@tests/test_opencode_http.py`:
- Around line 532-548: Update test_kill_process_reaps_detached_descendant to
avoid invoking the external setsid utility: replace the shell-based
detached-child setup with a Python subprocess launched using
start_new_session=True, while preserving PID-file creation, child liveness, and
the existing descendant-reaping assertions.
In `@tests/test_process_host.py`:
- Around line 191-229: Add a test case for _psutil_descendants that simulates
PID reuse between enumeration and identity validation: return a process from the
initial child enumeration representing generation A, then make the subsequent
identity lookup represent generation B for the same PID. Assert the resulting
snapshot excludes that PID and does not authenticate the reused generation.
In `@tests/test_stories_e2e.py`:
- Around line 213-245: Gate the E2E test containing DETACHED_WRITER_FAKE_CLI to
run only on Linux, skipping it on macOS and other non-Linux platforms. Update
the existing platform-skip logic in the relevant test rather than changing the
fake CLI.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9223180f-de11-4a82-9dfe-96df8748e64e
📒 Files selected for processing (8)
docs/FEATURES.mdsrc/bmad_loop/adapters/generic.pysrc/bmad_loop/adapters/opencode_http.pysrc/bmad_loop/process_host.pytests/test_generic_tmux.pytests/test_opencode_http.pytests/test_process_host.pytests/test_stories_e2e.py
…ities (PR #184 review) - ProcessHost.descendants now returns a pid -> identity snapshot captured during enumeration (same /proc stat read on Linux, same psutil Process object elsewhere); a member whose identity can't be captured is omitted, closing the enumerate-then-stamp pid-reuse window. - The straggler reaps (tmux + opencode mirror) no longer signal identity-None members at all: no SIGTERM, no poll burn, no force-kill — observation-only, reported via the unreaped breadcrumb. - opencode _kill_process now mirrors the tmux strike-before-reraise doctrine for a bogus BMAD_LOOP_PROCESS_HOST override (+ regression test). - Tests: pid-rollover omission coverage, portable start_new_session=True detached child (no setsid(1) — macOS lacks it), stories E2E gated to Linux (the fake CLIs are bash + GNU coreutils).
Collaborator
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bmad_loop/adapters/generic.py (1)
758-763: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winIdentity-guard refreshed pane PIDs before force-killing.
A pane can exit after
window_pane_pids(), allowing its PID to be reused before Line 762. Use the harvested identity andalive_and_ours()instead of force-killing the bare PID.🔒 Proposed fix
for pid in repane: + identity = tree.get(pid) + if identity is None or not host.alive_and_ours(pid, identity): + continue try: host.force_kill(pid)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bmad_loop/adapters/generic.py` around lines 758 - 763, Update the escalation loop around window_pane_pids and force_kill to retain each harvested process identity, revalidate it with alive_and_ours() immediately before termination, and only force-kill when the identity still belongs to the expected task. Preserve the existing lifecycle note and already-gone exception handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bmad_loop/process_host.py`:
- Around line 294-305: Update the psutil child handling in the descendant
collection function to read create_time(), then verify the same child Process
with is_running() before recording its PID and generation; omit the child when
validation returns false or raises, while preserving the existing never-raise
behavior. Update the rollover test fake so create_time() returns generation B
and is_running() returns False, covering reuse without relying on an exception.
---
Outside diff comments:
In `@src/bmad_loop/adapters/generic.py`:
- Around line 758-763: Update the escalation loop around window_pane_pids and
force_kill to retain each harvested process identity, revalidate it with
alive_and_ours() immediately before termination, and only force-kill when the
identity still belongs to the expected task. Preserve the existing lifecycle
note and already-gone exception handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c36043f2-67c2-4e38-ba05-5bbcd91a0489
📒 Files selected for processing (7)
src/bmad_loop/adapters/generic.pysrc/bmad_loop/adapters/opencode_http.pysrc/bmad_loop/process_host.pytests/test_generic_tmux.pytests/test_opencode_http.pytests/test_process_host.pytests/test_stories_e2e.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/test_stories_e2e.py
- src/bmad_loop/adapters/opencode_http.py
- tests/test_generic_tmux.py
…g (PR #184 review) `_psutil_descendants` stamped each enumerated child with `create_time()` and the docstring claimed that was reuse-safe because "psutil binds identity at construction". That is false on macOS. Per `psutil.Process._get_ident` (7.2.2), only the WINDOWS branch pre-populates the epoch cache (`self._create_time = create_time(fast_only=True)`). The `LINUX or NETBSD or OSX` branch binds a *monotonic* starttime and leaves `self._create_time` unset, and `create_time()` itself never consults `_raise_if_pid_reused()`. So on macOS our stamp was the first call — a raw call-time kernel read. A pid reaped and reused between `children()` and the stamp handed back the NEWCOMER's identity, which teardown would then authenticate successfully and SIGTERM/SIGKILL: precisely the invariant this branch exists to enforce, defeated by a comment asserting the opposite. Read the identity, revalidate via `is_running()` (construction-bound ident vs. a fresh `Process`), then record. Order matters — validating first reopens the same TOCTOU gap. An unconfirmable member is omitted, never stamped. Linux never reaches this path (psutil-free `/proc` walk); Windows was already safe via the `_get_ident` cache. Both consumers ride the seam (`generic.py`, `opencode_http.py`), so neither adapter changes. The rollover test's exception-only fake passed against the buggy code; it now also models the shape that actually bites — `create_time()` returning generation B while `is_running()` reports False — and fails without the fix.
Collaborator
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
Collaborator
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
This was referenced Jul 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #183. Follow-up to #139/#182: that PR made a lost teardown race degrade instead of crash; this one removes the race itself.
Mechanism
A process spawned inside a session (backgrounded pytest, a
setsid/double-forked runner) escapes the pane's process group, sotmux kill-window's SIGHUP never reaches it — and the #157 verified kill only harvested pane pids when the window outlivedteardown_grace_s. A session that ended cleanly (window dead on the first probe) never had any pids read at all, leaving stragglers free to write into the worktree while the engine merged and deleted it.Fix
GenericAdapter.kill— whenteardown_grace_s > 0, harvest the pane pids plus their transitive descendants (each stamped with a pid-reuse identity) before the first kill strike, while the tree is provably alive. After the window dies,_reap_straggler_treeterminates → polls → force-kills identity-confirmed survivors within the same grace deadline (one budget, two phases; terminate first so a mid-write straggler can flush). The wedged-window escalation also force-kills harvested descendants, with the same None-identity refusal as the clean path.grace = 0remains the legacy single strike, byte-for-byte.ProcessHost.descendants(pid)— new seam capability: transitive descendants as adict[int, float | None](pid → pid-reuse identity captured DURING enumeration, so there is no post-hoc stamp to race a reuse); a member whose identity can't be confirmed is OMITTED rather than returned unstamped. Never raises,{}degrade. Linux uses a one-pass/procppid map + BFS, taking ppid and starttime from the SAMEstatread (the core stays psutil-free); macOS/win32 go through the existing lazy_psutil(), revalidating each child viais_running()before stamping it —create_time()is only construction-bound on the WINDOWS branch of psutil's_get_ident, so on macOS it is a call-time read that would otherwise return a recycled pid's identity.opencode_http._kill_process— mirrored pre-signal harvest + reap bounded bykill_wait_s(kept as its own copy per the adapter's teardown convention; these sessions have no tmux window, so the process-tree wait is the only teardown signal there).BMAD_LOOP_PROCESS_HOSToverride still strikes the window once before the loudProcessHostErrorre-raise, so a config error can never leak a live window.Accepted limitations (documented at the harvest site): a double-fork orphan that reparented before the harvest, or a process detached in the harvest→kill TOCTOU window, is unfindable by construction; macOS without the
non-linuxextra degrades to today's behavior.Verification
test_e2e_detached_writer_reaped_before_worktree_teardown) automates the issue's acceptance sketch: a fake CLIsetsid-detaches a writer and ends cleanly; the writer is reaped within the grace, the worktree removes cleanly, and noworktree-teardown-degradedfires. (The existing session-end journaled 2h19 after session_timeout_min fires when the session is wedged inside a tool call #157 fake child shares the pane pgid and dies via SIGHUP — it cannot reproduce this bug.)test_kill_returns_on_first_dead_probe_without_escalating'spane_pid_reads == 0assertion is deliberately inverted to== 1— the old assertion encoded the bug.descendantsseam: real grandchild/procenumeration + monkeypatched-psutil never-raise contract, plus a psutil pid-rollover unit (create_time()returning the recycled generation whileis_running()reports False — that pid must be omitted, verified to fail without the fix); opencode mirror: real Popensetsiddescendant reaped, None-identity refusal unit.trunk checkclean.Summary by CodeRabbit