fix: stop leaking http.server daemons from ticks (reaping only orphans)

igor commented

2026-07-26 02:11:29 +00:00

Collaborator

What this PR does

Supersedes #422 -- carries all of its commits plus one review fix. Opened as a separate branch rather than pushing onto the bot's PR branch, which collides with the rework loop.

fix: stop leaking python3 -m http.server daemons from ticks (original #422 work)
fix: reap only ORPHANED servers, not ones somebody is using (review fix)

The review fix

http_reap_select_victims read the ppid field and discarded it (while read -r pid _ etimes cmd), leaving age and cmdline as the only criteria. Any match older than HTTP_REAP_STALE_SECS was SIGKILLed regardless of parentage:

55555 40000 400 python3 -m http.server 8000   (started from a shell) -> SELECTED
55555     1 400 python3 -m http.server 8000   (orphaned leak)        -> SELECTED

An operator running python3 -m http.server to eyeball a build would lose it inside five minutes, to a sweep that now runs on every tick, with the only trace a line in the journal. That is a worse day than the leak being fixed.

Now gated on ppid == 1, using the field already being read. That is the leak's actual signature -- every orphan in igor#418 was reparented to init when its tick exited -- and it additionally spares a listener that a currently running tick legitimately owns, which the age check alone would have killed mid-verification.

Test gap that hid it

Every row in bin/test-http-reap.sh used ppid 1, so the suite passed whether the guard existed or not. Added three rows differing ONLY in parentage, plus a live-parent sibling in the multi-row case, so this cannot regress silently.

Test plan

bash bin/test-http-reap.sh passes -- 20 assertions, including the 3 new ppid rows
make test passes (24 suites)
shellcheck lib/http-reap.sh bin/test-http-reap.sh clean
Ran the predicate against the real host ps table: selects nothing, correct -- the four orphans from igor#418 were manually reaped on 2026-07-24
Confirmed the igor#392 leading-dash argv0 row still reaches basename (it is ppid 1, so the new guard does not short-circuit that coverage)

Not changed

browser_reap_sweep moving from maintenance-only to every-tick stays as the original PR had it -- right direction, ~30x cadence increase on igor#388's predicate. Flagging rather than touching it.

Closes #418

## What this PR does Supersedes #422 -- carries all of its commits plus one review fix. Opened as a separate branch rather than pushing onto the bot's PR branch, which collides with the rework loop. - [x] fix: stop leaking `python3 -m http.server` daemons from ticks (original #422 work) - [x] fix: reap only ORPHANED servers, not ones somebody is using (review fix) ### The review fix `http_reap_select_victims` read the ppid field and discarded it (`while read -r pid _ etimes cmd`), leaving age and cmdline as the only criteria. Any match older than `HTTP_REAP_STALE_SECS` was SIGKILLed regardless of parentage: ``` 55555 40000 400 python3 -m http.server 8000 (started from a shell) -> SELECTED 55555 1 400 python3 -m http.server 8000 (orphaned leak) -> SELECTED ``` An operator running `python3 -m http.server` to eyeball a build would lose it inside five minutes, to a sweep that now runs on **every** tick, with the only trace a line in the journal. That is a worse day than the leak being fixed. Now gated on `ppid == 1`, using the field already being read. That is the leak's actual signature -- every orphan in igor#418 was reparented to init when its tick exited -- and it additionally spares a listener that a **currently running** tick legitimately owns, which the age check alone would have killed mid-verification. ### Test gap that hid it Every row in `bin/test-http-reap.sh` used `ppid 1`, so the suite passed whether the guard existed or not. Added three rows differing ONLY in parentage, plus a live-parent sibling in the multi-row case, so this cannot regress silently. ## Test plan - [x] `bash bin/test-http-reap.sh` passes -- 20 assertions, including the 3 new ppid rows - [x] `make test` passes (24 suites) - [x] `shellcheck lib/http-reap.sh bin/test-http-reap.sh` clean - [x] Ran the predicate against the real host `ps` table: selects nothing, correct -- the four orphans from igor#418 were manually reaped on 2026-07-24 - [x] Confirmed the igor#392 leading-dash argv0 row still reaches `basename` (it is ppid 1, so the new guard does not short-circuit that coverage) ## Not changed `browser_reap_sweep` moving from maintenance-only to every-tick stays as the original PR had it -- right direction, ~30x cadence increase on igor#388's predicate. Flagging rather than touching it. Closes #418

igor added 4 commits

2026-07-26 02:11:29 +00:00

fix: stop leaking python3 -m http.server daemons from ticks

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

Details

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

Details

3c30e01129

fix: run the reap sweeps every tick, not only on maintenance ticks

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

Details

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

Details

0e34f4d0ed

Review feedback on #422.

1. The `1003b` assertion was vacuous: `http_reap_select_victims` drops
   the row at the numeric-pid guard, so it never reached the signature
   check it claimed to exercise. The compiler row now uses pid `1013` so
   it flows through the predicate, and the non-numeric-pid guard gets its
   own deliberate case (same otherwise-reapable cmdline as T1, so the pid
   is the only reason it's spared).

2. The cadence now matches the PR description rather than the other way
   round. Both sweeps sat inside `do_maintenance_tick`, which only runs
   on a tick that falls all the way down the cascade -- so a busy stretch
   of ticks left orphans alive for exactly as long as #418 reported. The
   pair moved to the top of the cascade, above the health gate and the
   deploy barrier: unconditional, every tick, no model call. Moving
   `browser_reap_sweep` along with it closes the same latent gap for
   igor#388 and keeps the two in one block under one comment.

Also tightened the cmdline branch (reviewer's non-blocking note): the
substring match is now gated on a `python*`/`php*` basename, so a
long-lived process that merely mentions a signature in its args
(`tail -F http-server.log`, `grep -r http.server`) is no longer a
SIGKILL candidate. Covered by two new spared cases. Noted in the lib why
node-hosted servers (`npx serve`) stay directive-only: `node` is
protected wholesale, and mistaking the harness's own node for a leak is
worse than missing one.

make test and shellcheck on the touched files pass; the two remaining
shellcheck warnings are pre-existing and in untouched code.

Merge branch 'master' into agent/418-ticks-leak-python3-m-http-server-daemons-stale

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

Details

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

Details

e951ec034f

fix: reap only ORPHANED static-file servers, not ones in use

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

Details

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

Details

3d54a9eec4

Review follow-up on #422. The predicate read the ppid field and threw
it away (`while read -r pid _ etimes cmd`), so age plus cmdline were
the only criteria and any match older than HTTP_REAP_STALE_SECS was
SIGKILLed regardless of parentage:

  55555 40000 400 python3 -m http.server 8000  (from a shell) SELECTED
  55555     1 400 python3 -m http.server 8000  (orphaned leak) SELECTED

An operator running `python3 -m http.server` to eyeball a build would
lose it within five minutes, to a sweep that now runs on EVERY tick,
with the only trace a line in the journal. That is a worse outcome than
the leak this reaps.

Gate on ppid 1 instead. That is the leak's actual signature -- every
orphan in igor#418 was reparented to init when its tick exited -- and
it additionally spares a listener a currently-running tick owns, which
the age check alone would kill mid-verification.

Every row in the existing suite used ppid 1, so it passed either way
and could not see this. Added three rows that differ ONLY in parentage,
plus a live-parent sibling in the multi-row case so the regression
can't come back silently.

igor referenced this pull request

2026-07-26 02:11:40 +00:00

fix: stop leaking python3 -m http.server daemons from ticks #422

igor commented

2026-07-26 02:14:41 +00:00

Author

Collaborator

🤖 Review — `REQUEST_CHANGES` (automated)

CI for 3d54a9ee: success

Adds a python3 -m http.server-style reaper (lib/http-reap.sh + unit tests + directive text) and hoists both it and the existing browser reaper out of do_maintenance_tick to run on every tick. The http predicate itself is careful work and the ppid fix over #422 is the right call — but the PR also carries an unrelated cadence change to browser_reap_sweep that it explicitly declines to make safe, and one false-positive class in the new predicate.

Blocking

bin/tick.sh:1222-1231 / 3047-3059 — out-of-scope behavior change to browser_reap_sweep, at ~30x cadence, without the guard this PR was opened to add. Moving the browser sweep from maintenance-only to unconditional-every-tick is a change to igor#388's subsystem, not igor#418's; the description acknowledges it ("Flagging rather than touching it") but flagging doesn't put it in scope. Worse, the diff to lib/browser-reap.sh is comments only, so as far as I can see from this diff its predicate is still age + cmdline with no ppid == 1 gate. That is precisely the bug this PR fixes for http servers — "the age check alone would have killed a listener a currently running tick legitimately owns" — now amplified 30x for browsers. Fix: either leave browser_reap_sweep inside do_maintenance_tick and hoist only http_reap_sweep (this PR's actual issue), or apply the same orphan guard to browser_reap_select_victims in a separate PR. Either way the browser cadence change doesn't belong bundled here. I could not verify the browser predicate from the diff, which is itself a reason not to ship the cadence change in this PR.
lib/http-reap.sh:96 — ppid == 1 is not the same as "orphaned" on a systemd host. Every systemd-managed service is ppid 1. A supervised php -S ... -t dist unit, an http-server under a unit file, or any python service whose argv contains http-server is stale-by-definition (uptime ≫ 300s), ppid 1, and matches the signature — so this sweep SIGKILLs it every tick, and systemd restarts it, producing a flap loop whose only trace is a journal line. That's the exact failure mode the ppid guard was added to prevent, just moved to a different victim. At minimum this needs an additional discriminator (e.g. skip processes with a systemd cgroup / /proc/$pid/cgroup not under the agent's slice) or an explicit, documented statement that no supervised static server may exist on this host. A test row covering "ppid 1 but supervised" would pin whatever you choose.

Needs an answer before merge

bin/test-http-reap.sh is new but the Makefile isn't touched. The test plan claims make test passes with 24 suites. If the runner enumerates suites explicitly rather than globbing bin/test-*.sh, this file is never executed by CI and the green status says nothing about it. Confirm the glob (or add the wiring).

Non-blocking notes

http_reap_sweep and _http_reap_kill_tree have no coverage at all — understandable (they call real kill), but the row-lookup path (grep -E "^[[:space:]]*${pid}[[:space:]]" + read -r _ _ etimes cmd) is untested string handling in the logging path. Low risk since the regex is anchored, just noting the gap.
The ppid guard also narrows the fix: a leak spawned via setsid/nohup/a subshell that exits is ppid 1 while the owning tick is still alive, so it can still be killed mid-verification; conversely a leak whose parent shell survives is now never reaped. Both are defensible tradeoffs, but the header comment claims the guard "spares a listener a currently-RUNNING tick legitimately owns," which is only true when the tick is the direct parent. Worth softening the comment.
Checklist honesty: all six checked items map to real changes in the diff (ppid gate at lib/http-reap.sh:96, three new ppid rows T7A/T7B/T7C plus the 2005 live-parent sibling in MULTI, the -bash row at T6). Assertion count reconciles at exactly 20. No fabrication found.
Doc changes in AGENTS.md and bin/lib/site-work-directive.md are in scope and read well.

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

### 🤖 Review — `REQUEST_CHANGES` _(automated)_ CI for `3d54a9ee`: **success** Adds a `python3 -m http.server`-style reaper (`lib/http-reap.sh` + unit tests + directive text) and hoists both it and the existing browser reaper out of `do_maintenance_tick` to run on every tick. The http predicate itself is careful work and the ppid fix over #422 is the right call — but the PR also carries an unrelated cadence change to `browser_reap_sweep` that it explicitly declines to make safe, and one false-positive class in the new predicate. ## Blocking - **`bin/tick.sh:1222-1231` / `3047-3059` — out-of-scope behavior change to `browser_reap_sweep`, at ~30x cadence, without the guard this PR was opened to add.** Moving the browser sweep from maintenance-only to unconditional-every-tick is a change to igor#388's subsystem, not igor#418's; the description acknowledges it ("Flagging rather than touching it") but flagging doesn't put it in scope. Worse, the diff to `lib/browser-reap.sh` is *comments only*, so as far as I can see from this diff its predicate is still age + cmdline with no `ppid == 1` gate. That is precisely the bug this PR fixes for http servers — "the age check alone would have killed a listener a currently running tick legitimately owns" — now amplified 30x for browsers. Fix: either leave `browser_reap_sweep` inside `do_maintenance_tick` and hoist only `http_reap_sweep` (this PR's actual issue), or apply the same orphan guard to `browser_reap_select_victims` in a separate PR. Either way the browser cadence change doesn't belong bundled here. I could not verify the browser predicate from the diff, which is itself a reason not to ship the cadence change in this PR. - **`lib/http-reap.sh:96` — `ppid == 1` is not the same as "orphaned" on a systemd host.** Every systemd-managed service is ppid 1. A supervised `php -S ... -t dist` unit, an `http-server` under a unit file, or any python service whose argv contains `http-server` is stale-by-definition (uptime ≫ 300s), ppid 1, and matches the signature — so this sweep SIGKILLs it *every tick*, and systemd restarts it, producing a flap loop whose only trace is a journal line. That's the exact failure mode the ppid guard was added to prevent, just moved to a different victim. At minimum this needs an additional discriminator (e.g. skip processes with a systemd cgroup / `/proc/$pid/cgroup` not under the agent's slice) or an explicit, documented statement that no supervised static server may exist on this host. A test row covering "ppid 1 but supervised" would pin whatever you choose. ## Needs an answer before merge - **`bin/test-http-reap.sh` is new but the Makefile isn't touched.** The test plan claims `make test` passes with 24 suites. If the runner enumerates suites explicitly rather than globbing `bin/test-*.sh`, this file is never executed by CI and the green status says nothing about it. Confirm the glob (or add the wiring). ## Non-blocking notes - `http_reap_sweep` and `_http_reap_kill_tree` have no coverage at all — understandable (they call real `kill`), but the row-lookup path (`grep -E "^[[:space:]]*${pid}[[:space:]]"` + `read -r _ _ etimes cmd`) is untested string handling in the logging path. Low risk since the regex is anchored, just noting the gap. - The ppid guard also *narrows* the fix: a leak spawned via `setsid`/`nohup`/a subshell that exits is ppid 1 while the owning tick is still alive, so it can still be killed mid-verification; conversely a leak whose parent shell survives is now never reaped. Both are defensible tradeoffs, but the header comment claims the guard "spares a listener a currently-RUNNING tick legitimately owns," which is only true when the tick is the direct parent. Worth softening the comment. - Checklist honesty: all six checked items map to real changes in the diff (ppid gate at `lib/http-reap.sh:96`, three new ppid rows T7A/T7B/T7C plus the `2005` live-parent sibling in `MULTI`, the `-bash` row at T6). Assertion count reconciles at exactly 20. No fabrication found. - Doc changes in `AGENTS.md` and `bin/lib/site-work-directive.md` are in scope and read well. --- <sub>Independent review by the harness on `claude-opus-5` (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.</sub>

igor added spent time

2026-07-26 02:14:41 +00:00

1 minute 43 seconds

igor self-assigned this

2026-07-26 02:14:42 +00:00

igor removed their assignment

2026-07-26 02:16:46 +00:00

igor commented

2026-07-26 02:27:32 +00:00

Author

Collaborator

🔧 Rework — round 1 (automated)

Addressed the review on claude-opus-5 at effort high — 2 new commit(s).

### 🔧 Rework — round 1 _(automated)_ Addressed the review on `claude-opus-5` at **effort high** — 2 new commit(s).

igor added spent time

2026-07-26 02:27:57 +00:00

10 minutes 52 seconds

igor added 2 commits

2026-07-26 02:27:57 +00:00

fix: leave the browser reap on its maintenance cadence 5ce238f654

Review: the every-tick hoist swept up `browser_reap_sweep` too, which is
igor#388's subsystem, not igor#418's. Worse, browser_reap_select_victims
has no orphan guard -- age plus cmdline only -- so hoisting it took the
exact bug this PR fixes for http servers and multiplied its blast radius
by the cadence increase. A screenshot capture that a running tick owns
would be SIGKILLed mid-capture, ~30x more often than before.

So: browser_reap_sweep goes back inside do_maintenance_tick verbatim,
lib/browser-reap.sh reverts to master (the diff there was comments only),
and only http_reap_sweep stays hoisted -- which is what igor#418 needs,
since a leaked listener squatting a port answers a LATER tick's build
verification with a stale build from another repo, and maintenance-tick
cadence leaves that window open for hours.

Giving the browser predicate the same orphan guard is a real follow-up,
just not this PR.

fix: don't reap a static server systemd is supervising

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

Details

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

Details

7f3f257ab9

Review, and it's right: on a systemd host `ppid == 1` is not the same as
"orphaned" -- every supervised service is ppid 1. A `php -S -t dist` or
`http-server` unit is stale by definition (uptime is the point), matches
the signature, and would have been SIGKILLed every tick, restarted by
systemd every tick, with a journal line the only trace. That's the same
failure the ppid guard was added to prevent, pointed at a different
victim.

Second discriminator: the candidate's cgroup. _http_reap_cgroup_path
reads the `0::` line of /proc/<pid>/cgroup and
_http_reap_supervising_unit walks it innermost-outward for the `.service`
that owns the process. A foreign service means somebody else supervises
it -- skip. The harness's own unit (agent.service), a session/tmux
`.scope`, or an unreadable cgroup stay reapable, so the igor#418 leak is
still caught wherever it landed. The walk stops at the first `.scope`
deliberately: a scope tracks a login session or one-off spawn, so the
`user@N.service` above it supervises the SESSION, not the process, and a
leak in there is still a leak. Verified against this host's real layout
(agent.service under app.slice; nginx.service, stonks-discord.service,
and a 26-process tmux scope all present).

Five test rows, identical but for their cgroup: supervised-elsewhere
spared, own-unit reaped, session-scope reaped, unreadable-cgroup reaped,
and nested-under-a-foreign-service spared (systemd nests its own, e.g.
systemd-udevd.service/udev, so the walk has to look past the leaf). The
stub is installed file-wide so no row's verdict depends on what the host
happens to be running under those pids.

Also per review: softened the header's "anything past the threshold is a
leak, full stop" and the claim that ppid 1 spares a running tick's
listener. It only does when the tick is the DIRECT parent; a
setsid/nohup double-fork is ppid 1 with its tick alive, and a leak whose
parent shell survives is never reaped. Both are now stated as the
deliberate tradeoff they are -- this is a backstop, so under-reaping is
the cheap failure.

Reviewer's question about the suite wiring: bin/check-sync.sh globs
`for t in bin/test-*.sh`, so no Makefile change is needed -- `make test`
picks the file up and its line reads `+ bin/test-http-reap.sh passed`
(25 suites, exit 0).

igor commented

2026-07-26 02:32:04 +00:00

Author

Collaborator

🤖 Review — `COMMENT` (automated)

CI for 7f3f257a: success

Adds a per-tick sweep (lib/http-reap.sh) that SIGKILLs orphaned static-file servers, plus directive text telling the agent not to spawn them, wired into bin/tick.sh with a mock-table unit suite. The code itself reads carefully and the ppid/cgroup guards are well reasoned — but it's a destructive host-level kill running on every tick, and there are two things I can't verify from the diff, so I'm leaving this for a human rather than approving.

What I checked and found clean

Checklist is honest. Both checked items map to real diff content: the reaper + tick wiring exists, and the ppid == 1 gate is genuinely present (lib/http-reap.sh, [ "$ppid" = "1" ] || continue) with three dedicated ppid rows (T7A/T7B/T7C) and a live-parent sibling (2005) in the multi-row case.
Predicate logic traces correctly against every test row I hand-simulated, including the cgroup walk: /system.slice/static-site.service/worker → static-site.service (spared), session-5.scope → short-circuits to empty (reaped), .../app.slice/agent.service == HTTP_REAP_OWN_UNIT (reaped).
No stdin clobbering inside the while read loop — the /proc read happens in a command substitution with its own redirection.
set -e safety: every A && B && continue / [ -z "$x" ] && return 0 is mid-body, and the while loop's terminal status is 0 on all paths I could construct, so victims=$(http_reap_select_victims ...) shouldn't abort a tick.
No security issue. grep -E "^[[:space:]]*${pid}..." is fed a pid already validated numeric; no untrusted interpolation, no secrets, no CI-config change.

Things a human should weigh

Is bin/test-http-reap.sh actually run by make test? The diff adds the file but no Makefile / test-runner registration. If the runner globs bin/test-*.sh this is fine; if suites are listed explicitly, the entire new suite never runs in CI and the green check says nothing about it. This is the one thing that would change my verdict either way — please confirm.
Assertion count in the test plan is wrong. The description claims "20 assertions"; I count 25 killed/spared/inline checks (T1, T2, T2B, T3, T3B, T3C, T3D, T3E, T4, T4B, T5A, T5B, the stderr check, T6, TB1, TB2, T7A–C, T8A–E, multi-row). The tests are real and more thorough than claimed — just note the number is off.
Blast radius on the host. A nohup/setsid/disowned python3 -m http.server an operator left running is ppid 1 with no .service cgroup, so it gets SIGKILLed 5 minutes later with only a journal line. The code documents this as deliberate ("under-reaping is the cheap failure" — but this is the over-reaping direction). Whether that's acceptable depends on whether this host is single-purpose; the human owns that call, not me.
PID-reuse TOCTOU (http_reap_sweep). Victims come from a ps snapshot; _http_reap_kill_tree kills $pid later without re-confirming the cmdline still matches. The window is milliseconds, but the consequence is an unrelated SIGKILL. Cheap hardening: re-read /proc/$pid/cmdline and bail if the signature no longer matches.
SIGKILL with no SIGTERM first — intentional for a leak, but worth a beat since the same path now reaps anything matching the signature every tick.
Diff is ~427 added lines, marginally over the ~400 budget. Cohesive and not padded (the overage is the test file), so I wouldn't split it — noting for the record.
browser_reap_sweep cadence was left as-is and explicitly flagged in the description and in the tick.sh comment. Good call; correctly out of scope.
Housekeeping: #422 should be closed manually since this supersedes it.

Follow-ups worth a ticket

HTTP_REAP_OWN_UNIT is documented as a test seam but is env-readable in production; a stray export would silently narrow (or widen) the cgroup guard. Consider making the test override explicit instead (e.g. only honored when a test flag is set).
No test covers the real _http_reap_cgroup_path / _http_reap_supervising_unit pair against a synthetic /proc-style fixture — the whole cgroup layer is stubbed out in the suite, so a parsing regression in 0:: handling would pass CI.

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

### 🤖 Review — `COMMENT` _(automated)_ CI for `7f3f257a`: **success** Adds a per-tick sweep (`lib/http-reap.sh`) that SIGKILLs orphaned static-file servers, plus directive text telling the agent not to spawn them, wired into `bin/tick.sh` with a mock-table unit suite. The code itself reads carefully and the ppid/cgroup guards are well reasoned — but it's a destructive host-level kill running on every tick, and there are two things I can't verify from the diff, so I'm leaving this for a human rather than approving. ## What I checked and found clean - **Checklist is honest.** Both checked items map to real diff content: the reaper + tick wiring exists, and the `ppid == 1` gate is genuinely present (`lib/http-reap.sh`, `[ "$ppid" = "1" ] || continue`) with three dedicated ppid rows (T7A/T7B/T7C) and a live-parent sibling (`2005`) in the multi-row case. - **Predicate logic traces correctly** against every test row I hand-simulated, including the cgroup walk: `/system.slice/static-site.service/worker` → `static-site.service` (spared), `session-5.scope` → short-circuits to empty (reaped), `.../app.slice/agent.service` == `HTTP_REAP_OWN_UNIT` (reaped). - **No stdin clobbering** inside the `while read` loop — the `/proc` read happens in a command substitution with its own redirection. - **`set -e` safety**: every `A && B && continue` / `[ -z "$x" ] && return 0` is mid-body, and the `while` loop's terminal status is 0 on all paths I could construct, so `victims=$(http_reap_select_victims ...)` shouldn't abort a tick. - **No security issue.** `grep -E "^[[:space:]]*${pid}..."` is fed a pid already validated numeric; no untrusted interpolation, no secrets, no CI-config change. ## Things a human should weigh - **Is `bin/test-http-reap.sh` actually run by `make test`?** The diff adds the file but no Makefile / test-runner registration. If the runner globs `bin/test-*.sh` this is fine; if suites are listed explicitly, the entire new suite never runs in CI and the green check says nothing about it. This is the one thing that would change my verdict either way — please confirm. - **Assertion count in the test plan is wrong.** The description claims "20 assertions"; I count 25 `killed`/`spared`/inline checks (T1, T2, T2B, T3, T3B, T3C, T3D, T3E, T4, T4B, T5A, T5B, the stderr check, T6, TB1, TB2, T7A–C, T8A–E, multi-row). The tests are real and more thorough than claimed — just note the number is off. - **Blast radius on the host.** A `nohup`/`setsid`/disowned `python3 -m http.server` an operator left running is ppid 1 with no `.service` cgroup, so it gets SIGKILLed 5 minutes later with only a journal line. The code documents this as deliberate ("under-reaping is the cheap failure" — but this is the *over*-reaping direction). Whether that's acceptable depends on whether this host is single-purpose; the human owns that call, not me. - **PID-reuse TOCTOU (`http_reap_sweep`).** Victims come from a `ps` snapshot; `_http_reap_kill_tree` kills `$pid` later without re-confirming the cmdline still matches. The window is milliseconds, but the consequence is an unrelated SIGKILL. Cheap hardening: re-read `/proc/$pid/cmdline` and bail if the signature no longer matches. - **SIGKILL with no SIGTERM first** — intentional for a leak, but worth a beat since the same path now reaps anything matching the signature every tick. - **Diff is ~427 added lines**, marginally over the ~400 budget. Cohesive and not padded (the overage is the test file), so I wouldn't split it — noting for the record. - **`browser_reap_sweep` cadence** was left as-is and explicitly flagged in the description and in the `tick.sh` comment. Good call; correctly out of scope. - Housekeeping: #422 should be closed manually since this supersedes it. ## Follow-ups worth a ticket - `HTTP_REAP_OWN_UNIT` is documented as a test seam but is env-readable in production; a stray export would silently narrow (or widen) the cgroup guard. Consider making the test override explicit instead (e.g. only honored when a test flag is set). - No test covers the real `_http_reap_cgroup_path` / `_http_reap_supervising_unit` pair against a synthetic `/proc`-style fixture — the whole cgroup layer is stubbed out in the suite, so a parsing regression in `0::` handling would pass CI. --- <sub>Independent review by the harness on `claude-opus-5` (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.</sub>

igor added spent time

2026-07-26 02:32:04 +00:00

2 minutes 7 seconds

igor requested review from joshtronic

2026-07-26 02:32:05 +00:00

joshtronic added 1 commit

2026-07-26 16:35:58 +00:00

Merge branch 'master' into fix/418-reap-only-orphans

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

Details

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

Details