feat: reap stale headless-browser process trees at the top of the maintenance tick #389

Merged

joshtronic merged 1 commit from agent/388-maintenance-reap-stale-headless-browser-chrome into master

2026-07-15 14:40:07 +00:00

igor commented

2026-07-15 03:12:07 +00:00

Collaborator

What this PR does

feat: reap stale headless-browser process trees at the top of the maintenance tick
Add lib/browser-reap.sh: pure predicate browser_reap_select_victims (age + signature match, protects claude/node) plus the effectful browser_reap_sweep that snapshots ps, selects victims, kills each victim's whole process tree, and logs one line per victim
Wire browser_reap_sweep into do_maintenance_tick (bin/tick.sh), called first and unconditionally, ahead of the weekly per-repo eligibility loop
Add bin/test-browser-reap.sh: unit tests for the predicate against a mock ps table (stale/fresh headless_shell, stale non-browser incl. a node build, stale chrome with --headless, the harness's own claude/node processes, threshold boundary, multi-row selection) -- exercises selection only, never a real ps/kill

Threshold is hardcoded at BROWSER_REAP_STALE_SECS=3600 (60 min) per the issue's "strong opinions" convention -- no env knob. claude/node basenames are always protected, independent of any cmdline signature match, so the reaper can never touch the harness's own driver process.

Test plan

bash bin/check-sync.sh (== make test) passes, including the new bin/test-browser-reap.sh
shellcheck lib/browser-reap.sh bin/test-browser-reap.sh bin/tick.sh clean (one pre-existing SC2015 note in tick.sh at an unrelated line, not introduced by this change)
Mutation check: temporarily flipped the -ge threshold comparison to -gt, confirmed the boundary test caught it, then reverted and re-ran green
Manual: on the live host, confirm a real leaked headless_shell/chrome process older than 60 minutes gets SIGKILLed (with its child tree) on the next maintenance tick, and that the victim line shows up in the log/logwatch

Closes #388

## What this PR does - [x] feat: reap stale headless-browser process trees at the top of the maintenance tick - [x] Add `lib/browser-reap.sh`: pure predicate `browser_reap_select_victims` (age + signature match, protects `claude`/`node`) plus the effectful `browser_reap_sweep` that snapshots `ps`, selects victims, kills each victim's whole process tree, and logs one line per victim - [x] Wire `browser_reap_sweep` into `do_maintenance_tick` (`bin/tick.sh`), called first and unconditionally, ahead of the weekly per-repo eligibility loop - [x] Add `bin/test-browser-reap.sh`: unit tests for the predicate against a mock ps table (stale/fresh headless_shell, stale non-browser incl. a node build, stale chrome with `--headless`, the harness's own claude/node processes, threshold boundary, multi-row selection) -- exercises selection only, never a real `ps`/`kill` Threshold is hardcoded at `BROWSER_REAP_STALE_SECS=3600` (60 min) per the issue's "strong opinions" convention -- no env knob. `claude`/`node` basenames are always protected, independent of any cmdline signature match, so the reaper can never touch the harness's own driver process. ## Test plan - [x] `bash bin/check-sync.sh` (== `make test`) passes, including the new `bin/test-browser-reap.sh` - [x] `shellcheck lib/browser-reap.sh bin/test-browser-reap.sh bin/tick.sh` clean (one pre-existing SC2015 note in `tick.sh` at an unrelated line, not introduced by this change) - [x] Mutation check: temporarily flipped the `-ge` threshold comparison to `-gt`, confirmed the boundary test caught it, then reverted and re-ran green - [ ] Manual: on the live host, confirm a real leaked `headless_shell`/`chrome` process older than 60 minutes gets SIGKILLed (with its child tree) on the next maintenance tick, and that the victim line shows up in the log/logwatch Closes #388

igor added 1 commit

2026-07-15 03:12:07 +00:00

feat: reap stale headless-browser process trees at the top of the maintenance tick

Lint / check-sync (push) Successful in 4s

Details

Lint / check-sync (pull_request) Successful in 4s

Details

4d3b046e71

igor commented

2026-07-15 03:15:31 +00:00

Author

Collaborator

🤖 Review — `COMMENT` (automated)

CI for 4d3b046e: success

Adds a host-level stale-headless-browser reaper (lib/browser-reap.sh) wired to run first/unconditionally in do_maintenance_tick, with a well-tested pure selection predicate. The change is in scope (~200 lines, one issue), the checklist matches the diff, and CI is green. My hesitation is that this is an unconditional SIGKILL of process trees on a live production host, and the effectful half of the code is untested — worth a human's eyes before merge.

Checklist honesty: verified — every checked box maps to real diff content. browser_reap_sweep is genuinely the first statement in do_maintenance_tick; the unchecked box is the manual live-host step (honestly left unchecked). Good.

Strengths

browser_reap_select_victims is a clean pure predicate with solid unit coverage: staleness boundary (3599 vs 3600), signature vs. basename matching, protected claude/node, multi-row selection. The mutation-test note (-ge→-gt) is a nice touch.
Kill tree walks downward by PPID, so it structurally cannot reach the reaper's own ancestors (claude launches the browser, not vice-versa) — the "never kill the harness" invariant holds for the normal topology.

Things a human should weigh (not clear blockers)

Effectful path is entirely untested. Tests exercise only the predicate. browser_reap_sweep (the ps snapshot, the grep -E "^[[:space:]]*${pid}[[:space:]]" row re-lookup for logging, the read -r _ _ etimes cmd) and _browser_reap_kill_tree (recursion + kill -9) have zero coverage. These are the parts that actually destroy processes on the host. Understandable that you don't want to kill in CI, but the grep-back and tree-walk are non-trivial and unverified.
Protection doesn't extend to descendants. _browser_reap_is_protected gates selection, but _browser_reap_kill_tree SIGKILLs the entire descendant tree unconditionally. If a selected browser ever has a node/claude child (reverse of the normal topology), it would be killed despite being "protected." Low probability, but the protection guarantee is narrower than the comment implies.
PID-reuse race. Victims are chosen from a snapshot; by the time _browser_reap_kill_tree runs, a reaped-then-recycled PID could belong to an innocent process. Inherent to any reaper and the 1h-stale window makes it unlikely, but it's an unconditional kill -9 so worth acknowledging.

Follow-up suggestion: consider a dependency-injected ps/kill (or a dry-run mode emitting the kill commands) so the sweep + tree-walk can be unit-tested against the same mock table the predicate already uses. That would close the untested-effectful-path gap without touching real processes.

No security issues, no scope creep, CI green — routing to a human rather than blocking, because the concrete concerns are judgment calls on a destructive operation rather than definite defects.

_{Independent review by the harness on claude-opus-4-8 (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.}

### 🤖 Review — `COMMENT` _(automated)_ CI for `4d3b046e`: **success** Adds a host-level stale-headless-browser reaper (`lib/browser-reap.sh`) wired to run first/unconditionally in `do_maintenance_tick`, with a well-tested pure selection predicate. The change is in scope (~200 lines, one issue), the checklist matches the diff, and CI is green. My hesitation is that this is an **unconditional SIGKILL of process trees on a live production host**, and the effectful half of the code is untested — worth a human's eyes before merge. **Checklist honesty:** verified — every checked box maps to real diff content. `browser_reap_sweep` is genuinely the first statement in `do_maintenance_tick`; the unchecked box is the manual live-host step (honestly left unchecked). Good. **Strengths** - `browser_reap_select_victims` is a clean pure predicate with solid unit coverage: staleness boundary (3599 vs 3600), signature vs. basename matching, protected `claude`/`node`, multi-row selection. The mutation-test note (`-ge`→`-gt`) is a nice touch. - Kill tree walks *downward* by PPID, so it structurally cannot reach the reaper's own ancestors (claude launches the browser, not vice-versa) — the "never kill the harness" invariant holds for the normal topology. **Things a human should weigh (not clear blockers)** - **Effectful path is entirely untested.** Tests exercise only the predicate. `browser_reap_sweep` (the `ps` snapshot, the `grep -E "^[[:space:]]*${pid}[[:space:]]"` row re-lookup for logging, the `read -r _ _ etimes cmd`) and `_browser_reap_kill_tree` (recursion + `kill -9`) have zero coverage. These are the parts that actually destroy processes on the host. Understandable that you don't want to `kill` in CI, but the grep-back and tree-walk are non-trivial and unverified. - **Protection doesn't extend to descendants.** `_browser_reap_is_protected` gates *selection*, but `_browser_reap_kill_tree` SIGKILLs the entire descendant tree unconditionally. If a selected browser ever has a `node`/`claude` child (reverse of the normal topology), it would be killed despite being "protected." Low probability, but the protection guarantee is narrower than the comment implies. - **PID-reuse race.** Victims are chosen from a snapshot; by the time `_browser_reap_kill_tree` runs, a reaped-then-recycled PID could belong to an innocent process. Inherent to any reaper and the 1h-stale window makes it unlikely, but it's an unconditional `kill -9` so worth acknowledging. **Follow-up suggestion:** consider a dependency-injected `ps`/`kill` (or a dry-run mode emitting the kill commands) so the sweep + tree-walk can be unit-tested against the same mock table the predicate already uses. That would close the untested-effectful-path gap without touching real processes. No security issues, no scope creep, CI green — routing to a human rather than blocking, because the concrete concerns are judgment calls on a destructive operation rather than definite defects. --- <sub>Independent review by the harness on `claude-opus-4-8` (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.</sub>

igor added spent time

2026-07-15 03:15:31 +00:00

1 minute 9 seconds

igor requested review from joshtronic

2026-07-15 03:15:31 +00:00