Repository navigation
feat(process-hygiene): reap orphaned test processes left in agent scratchpads; fixtures die with their test - #5724
Conversation
…atchpads Interrupted or timed-out test runs leave background children re-parented to init that run for days: spin loops waiting on a file, fixture servers, watchers. Together they kept a host's load several times its idle level. - skills/process-hygiene: reap_orphans.py reports by default and with --reap stops a process only when it is this user's, has PPID 1, is older than PROCESS_HYGIENE_MIN_AGE_S, and its cwd is inside a per-session scratchpad; never-touch list for tmux, claude, Sutando core/worker/relay processes, launchd jobs, Chrome/Chromium and apps. Each target is re-read (pid, start time, cwd, command) before every signal; a group is signalled only when all its members qualify. --spotlight drops .metadata_never_index into scratchpad roots. - crons.example.json: an hourly launchd shell entry at minute 23. - tests/_helpers/process_group.py: bind_to_test (a pipe watchdog kills a bound group when the test exits by any means, SIGKILL included), run_in_own_group, and OWNER_LIFELINE_PY for fixtures started indirectly. - Fixtures fixed at the source: the agy-browser fake Chrome exits with its owner; the shutdown-leaves-inflight-claim, config-hot-reload (both) and foreign-instance-id watcher fixtures die with their test. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 92%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
skills/process-hygiene/scripts/reap_orphans.pyLines 110-118 110 proc: Proc
111 cwd: str
112
113 def identity(self) -> Tuple[int, str, str, str]:
! 114 return (self.proc.pid, self.proc.lstart, self.cwd, self.proc.command)
115
116
117 @dataclass
118 class Target:Lines 308-317 308 try:
309 sys.path.insert(0, str(REPO))
310 from src.sutando_config import resolve_core_config_dirs # noqa: E402
311 dirs += [e.get("value", "") for e in resolve_core_config_dirs() if e.get("type") == "claude"]
! 312 except Exception: # a missing or invalid config adds nothing; the env and home dirs remain
! 313 pass
314 seen: List[str] = []
315 for d in dirs:
316 if d and os.path.isdir(d) and os.path.realpath(d) not in seen:
317 seen.append(os.path.realpath(d))Lines 367-379 367 if is_dir:
368 sessions_seen = True
369 for name in sorted(os.listdir(sessions)):
370 if not name.endswith(".json"):
! 371 continue
372 with open(os.path.join(sessions, name)) as fh:
373 data = json.load(fh)
374 if not isinstance(data, dict):
! 375 return UNKNOWN
376 pid, proc_start = data.get("pid"), data.get("procStart")
377 if not isinstance(pid, int) or not isinstance(proc_start, str):
378 if data.get("sessionId") == session:
379 return UNKNOWNLines 547-555 547 def still_running(target: Target, ctx: Context) -> bool:
548 """Cheap poll during the grace: a planned member with its original start time is alive."""
549 procs = ctx.snapshot()
550 if procs is None:
! 551 return True
552 return any(procs[pid].lstart == target.members[pid].proc.lstart
553 for pid in alive_members(target, procs) & set(target.members))
554 Lines 611-619 611 if not ok:
612 ctx.log(f"abort-kill-changed {describe(target)}")
613 return "aborted"
614 if not send(target, signal.SIGKILL, ctx, alive):
! 615 return "aborted"
616 ctx.log(f"sigkill {describe(target)} survivors={','.join(str(p) for p in sorted(alive or ()))}")
617 return "killed"
618 |
liususan091219
left a comment
There was a problem hiding this comment.
Request changes, at 2040974. One reason: the fixture fix covers 4 watcher tests, and about 13 sibling tests with the same shape still leak. The reaper is designed not to reach those leaks.
The reaper is sound, and I am not asking for changes there. A process is a candidate only if it passes every check: same uid, PPID == orphan parent, older than 7200 s, not on the never-touch list, and its realpath cwd is inside <tmp>/claude-<uid>/*/*/scratchpad. It never matches on a command pattern or on age alone. Before each signal it re-reads start time, command and cwd. It sends TERM, waits the grace period, re-checks, and only then sends KILL. Report mode is the default, and every action is logged. The core, its task watcher, workers and user tmux/claude sessions are all safe on two independent counts: each has a live parent, and none has its cwd in a scratchpad. The one exposure is the case the PR body already names, a process the user deliberately nohuped from a scratchpad that is more than 2 h old. The tests are behavioural: real orphans run under a fake --tmp-base, signals are guarded to the test's own pids, and parented, outside, tmux, claude and young processes are shown to survive. Locally: reap-orphans 40/40 OK and process-group-binding 9/9 OK. Revert-to-fail on reap_orphans.py, each mutation alone: dropping the scratchpad-cwd rule → 4 failures, the PPID rule → 3, the never-touch list → 2 failures + 19 errors, the pre-signal recheck → 5. All four were killed.
Fixture fix, revert-to-fail. I ran the origin/main copy of tests/watch-tasks-stream-config-hot-reload.test.py and sent it SIGTERM at 6 s. It left a PPID-1 watch-tasks-stream.sh group plus a real fswatch. The HEAD copy under the same probe left nothing. The source fix works.
Blocking (bandaid_generalize). The PR body says leaks whose cwd is the repo are "fixed at the source above". In fact only these were converted: config-hot-reload, config-hot-reload-symlinked-workspace, foreign-instance-id-corrected and shutdown-leaves-inflight-claim. These tests start src/watch-tasks-stream.sh with start_new_session=True and tear down only in finally/stop(), which a SIGTERM or SIGKILL of the test skips:
- tests/watch-tasks-stream-bare-directory-event.test.py:77
- tests/watch-tasks-stream-priority-sweep.test.py:73, :145
- tests/watch-tasks-stream-dead-worker-reap.test.py:118
- tests/watch-tasks-stream-sentinel-ownership.test.py
- tests/watch-tasks-stream-malformed-roster-row.test.py
- tests/watch-tasks-stream-handler-terminal-rc.test.py
- tests/watch-tasks-stream-inbox-and-workspace-env.test.py
- tests/watch-tasks-stream-delivery-inbox-sentinel-identity.test.py
- tests/watch-tasks-stream-fallback-receipt-scope.test.py
- tests/watch-tasks-stream-readiness-window-decision-instant.test.py, and the shared tests/fixtures/readiness_window_helpers.py
- tests/worker-mode-is-gated-on-env.test.py, tests/telegram-ingress-identity.test.py
Repro on this head: python3 tests/watch-tasks-stream-bare-directory-event.test.py & sleep 4; kill -TERM $!. This leaves bash src/watch-tasks-stream.sh <tmp>/baredir-test-…/ws/tasks --role standby … at PPID 1 with a tail -f child. Its cwd is the repo, so the reaper skips it by design. Nothing else will ever stop it.
Ask: route these through popen_bound (or bind_to_test after Popen) the same way as the four you fixed. Ideally add a cheap guard so a new test cannot start watch-tasks-stream.sh with start_new_session=True without binding it. Otherwise the class grows back.
Not blocking, for the maintainers to decide: crons.example.json ships --reap, so a fresh install with the OS cron runner reaps hourly by default. A report-only example, with --reap added by the owner, is the more conservative default. The PR already offers this.
CI 33/33 green, license/cla success, no unresolved threads, no hardcoded workspace paths on added lines (review-checks PASS). Not verified here: a --reap run against a real host's process table. I deliberately did not run it on this machine.
— Sutando (susans-MacBook-Pro)
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes needed at 2040974. All required checks pass, the mechanics are careful and the fixture fixes close the traced leaks. The problem is what counts as a target.
How I checked: I applied the PR's own rules (parse_etime, protected_reason, launchd_pids, scratchpad_roots, inside) to a ps/lsof listing on a real dev Mac, without running run()/stop() on live processes. That gives 47 targets. Among them:
- live vite dev servers on :5393, :5394, :5395, :5397, :5398, :5400, :5401, :5402, :5403, :5410, :5420 and :8103, several of them the owner's active previews;
- two postgres instances;
- two cloudflared tunnels;
python -m http.serverinstances and aflutter attach.
Real leaks are in the same list too (notifier spin loops, a test fswatch, a leaked task-notifier copy, stray head/grep), so the reaper is useful. It just can't tell a deliberate server from a leak.
Blockers
- Target selection kills deliberate servers (
reap_orphans.py:263-283,classify). Anything an agent starts with&ornohupgets PPID 1 straight away, while its session is still alive. So "mine, PPID 1, older than 2 h, working directory in a scratchpad" isn't proof of a leak; on a dev host it's the common case. Before--reapruns by default, add one of these:- skip processes with a listening TCP socket (
lsof -a -p PID -iTCP -sTCP:LISTEN). That spares every server above and none of the leaks; - an opt-out marker in the working-directory chain;
- ship report-only.
- skip processes with a listening TCP socket (
--reapis on by default for new installs (skills/schedule-crons/crons.example.json:22-27).src/init.sh:305copies the example when a host has nocrons.json. Remove the entry, or make it report-only;--spotlightalone is fine.
Should fix
3. A symlinked scratchpad widens the reach (reap_orphans.py:229-235). scratchpad_roots uses realpath, so a scratchpad that's a symlink makes its target (the repo or workspace) a root. --spotlight would also drop .metadata_never_index there and three directories above (:412). Skip symlinked hits, or require the resolved root to sit under the resolved <tmp>/claude-<uid>.
4. The protection list is narrower than SKILL.md says (SKILL.md:40-44, PROTECTED_SUBSTRINGS at :60-64). Nothing names watch-tasks-stream.sh, task-notifier.sh, the voice agent, the *-bridge.py scripts, core_heartbeat or MCP servers. They're safe only because their working directory isn't normally a scratchpad, so a Sutando checkout run from a scratchpad worktree would get its services reaped. Add these names, or fix the wording.
Nits
watch-tasks-stream-config-hot-reload.test.py:110-119and the symlinked-workspace and foreign-instance teardowns callunbindafter waiting only for the leader. Usekill_group(pgid)first.run_in_own_groupintests/_helpers/process_group.pyhas no caller beyond its own test.- There's no
--min-agefloor when--reapis set (e.g. 600 s). - The lifeline checks the owner with
kill(pid,0)only, so a reused owner pid keeps the fixture alive. - A script-level
flockwould stop a manual run overlapping the cron run. Cron-to-cron overlap is already prevented bycron-runner.py:481-488's per-job flock.
What's correct
- Identity (pid, start time, command, working directory) is re-read before both TERM and KILL, and any change aborts, so pid reuse is handled.
- A group is signalled only if every live member is a target. The reaper's own group and ancestors are excluded.
- It runs as the user only, with no sudo, and logs every action.
- When unsure it does nothing: unreadable
ps, an unreadable working directory, or a changed group. - Provenance-based selection means substrings like "pytest" or "node" only add exclusions, never targets.
- A crash mid-reap leaves no state a later pass reads back.
- Config goes through
manifest.json(CLI > env > manifest).
…n report-only A live agent session may start a server in its scratchpad on purpose, so an orphan is a candidate only when the Claude Code session owning that scratchpad has ended. Liveness comes from Claude Code's own session marker (<config>/sessions/<pid>.json: sessionId, pid, procStart) with the pid's start time checked under TZ=UTC, or a transcript written within the minimum age; "gone" needs the transcript and a sessions/ dir; anything unreadable is unknown and spared. The example cron entry now only reports; --reap is a documented opt-in. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…r fixture dies with its test Reaper: - A candidate's process group must hold no listening TCP socket and no UDP socket (lsof on macOS, /proc/<pid>/net on Linux); unreadable spares the group, so a deliberate dev server is kept even after its session ends. - A scratchpad root is used only when claude-<uid> is a real directory of this user, no path component below it is a symlink, and the resolved root sits under the resolved claude-<uid>; --spotlight writes only there. - The never-touch list names the watcher, the task notifiers, the voice agent, channel bridges, core_heartbeat, the Station and MCP servers; SKILL.md lists exactly what is protected and why. An Xcode Contents/Developer toolchain is not an app. - owner_state maps any failing filesystem probe to unknown (Python 3.12's Path.is_dir raises where older versions return False). - --reap refuses a minimum age under 600 s; a run holds an exclusive flock so a manual run cannot overlap the cron run. Fixtures: - Every test that starts the task watcher, a task notifier or another long-running src script in its own session now goes through popen_bound, so a SIGTERMed or SIGKILLed test leaves nothing behind. - tests/watcher-fixtures-are-bound.test.py fails when such a Popen is not bound. - The owner lifeline checks the owner's start time as well as its pid, via the system ps; run_in_own_group is removed (no caller); the hot-reload and foreign-instance teardowns kill the group before unbinding it. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
liususan091219
left a comment
There was a problem hiding this comment.
Request changes, at 1d2bb81. My round-1 blocker is fixed. One thing is still blocking, and it is mechanical: CI is red at this head because of a file this PR adds.
Blocking: this PR's own test trips scripts/lint-hermetic-bridge-tests.py. It fails "refuse bridge tests that read host config", and through tests/lint-hermetic-bridge-tests.test.py / tests/lint-hermetic-discovery-floor.test.py it also fails "python standalone tests (3/9)" and "(4/9)". I reproduced it locally: python3 scripts/lint-hermetic-bridge-tests.py → FAIL … tests/skills/process-hygiene/reap-orphans.test.py. It is a false positive. The never-touch subtests carry the string literals "python3 src/discord-bridge.py" / "python3 src/telegram-bridge.py" (reap-orphans.test.py:272), and the file calls exec_module, so the lint's (discord|slack|telegram)-bridge\.py regex reads it as a bridge import. The real tests already isolate config (--claude-config-dir / a CLAUDE_CONFIG_DIR patch). Fix either side, for example by building those command strings so they don't match the regex, or by teaching the lint to skip literals. Then CI should go green. "node + out-of-tree python tests" was still running when I checked.
Round-1 item (sibling watcher fixtures): resolved. Probes, each on a fresh process table:
watch-tasks-stream-bare-directory-event.test.py, SIGTERM at 4 s: nothing left. In round 1 the same probe left a PPID-1 watcher plustail -f.watch-tasks-stream-dead-worker-reap.test.py, SIGTERM at 2 s: theorigin/maincopy left a PPID-1watch-tasks-stream.shplustail -f; the HEAD copy left nothing.
priority-sweep finishes before any kill time I could hit, so that probe proves nothing either way. A grep for test files that start watch-tasks-stream.sh/task-notifier.sh without popen_bound/bind_to_test turns up only plain foreground Popens (a 0.35 s argv probe, a communicate()-bound run, a -c pass). None are session-detached.
New lint (tests/watcher-fixtures-are-bound.test.py). It is an AST scan, not a substring grep. It passes here. I dropped the origin/main copy of bare-directory-event into a scratch tree and it flagged [77]. Its blind spots, which no file in the tree uses today (not blocking):
preexec_fn=os.setsidstart_new_sessionpassed through**kwor a variable- a script path imported as a constant from another module
- a function that binds a different Popen
Noted so the guard's reach is on record.
Reaper, round-2 claims checked behaviourally:
listening()against real sockets: a TCP listener → True, a UDP socket → True, no socket → False, an outbound TCP client only → False.- lsof missing → None, so the group is spared.
- Caveat (not blocking): for a pid whose fds this user cannot read,
lsof -wexits 1 with no output (pid 1 here →False, not None). "Unreadable spares" therefore holds when the probe fails, not when it is denied. Candidates are always same-uid, so this only matters for a foreign-uid member of a candidate's group. scratchpad_roots: a symlinkedscratchpadunderclaude-<uid>, a symlinked project dir and a symlinkedclaude-<uid>are all refused; a real one is kept.--reap --min-age 5exits 2 before any scan or signal (the floor check precedesrun()).- Owner-state probe errors are caught as OSError, so they read as unknown and the process is spared.
- Lifeline now checks pid plus start time via
/bin/ps; teardownskill_groupbeforeunbind;run_in_own_groupis removed; there is a flock. - Equivalent-mutant spot-check ("resolved root not required under
claude-<uid>"): I agree it is equivalent outside a race. Every component below a non-symlinkclaude-<uid>is lstat-checked, so the realpath can only leave the tree if a component is swapped between that check andrealpath. That is exactly what the kept check defends against. - Suites here: reap-orphans OK, process-group-binding OK, watcher-fixtures-are-bound OK.
Cron default: the example entry is now report-only (--spotlight, no --reap), which resolves my non-blocking note.
Gates: license/cla success (1 context), no unresolved threads, review-checks PASS. No hardcoded workspace paths on added lines; the only regex hit is the src.sutando_config import, not a path. No APPROVED review yet. Qingyun's round-1 review is COMMENTED, so the approve bar would not be met even with CI green. Not verified: a --reap run against a real host process table, which I deliberately did not run.
— Sutando (susans-MacBook-Pro)
qingyun-wu
left a comment
There was a problem hiding this comment.
Round 2 at 1d2bb81. COMMENT: the code is approvable, and all my round-1 blockers are fixed. One CI blocker remains.
Dry run with the new rules (read-only: kill/killpg stubbed to raise; only classify and find_candidates, which use ps/lsof/launchctl list), on the same dev Mac as round 1:
- 0 targets now, down from 47. All 45 orphaned processes in a scratchpad are kept as "owning session alive", with session markers matching start time.
- What-if, every session treated as ended: 21 targets, and 22 groups kept for their server sockets.
- Kept: every vite (:5393, :5394–:5420, :8103), both postgres, cloudflared, every
http.server,serve.mjs,flutter attach. - Caught: the leaks (notifier spin loops, a test fswatch, stray head/grep, Playwright scripts, terraform providers).
- Kept: every vite (:5393, :5394–:5420, :8103), both postgres, cloudflared, every
- Default reaping is off:
crons.example.json:26now runs--spotlightonly, and a test pins it. - The new lint is real: it fails at merge-base
2c2561a1(19 files, 32 call sites) and passes at the head. - The two surviving mutants are equivalent outside a check-then-act race; the
startswithconfinement is defense in depth.
Blocker: CI is red (4 checks, one root cause). The hermetic-bridge lint (scripts/lint-hermetic-bridge-tests.py) flags tests/skills/process-hygiene/reap-orphans.test.py. It exec_modules the reaper (:35), and its never-touch table (:272) contains the literal strings "python3 src/discord-bridge.py" and "python3 src/telegram-bridge.py", which match (discord|slack|telegram)-bridge\.py. That failure cascades into python standalone shards 3/9 and 4/9 and into tsc + tests. Build the strings so they don't match the pattern (e.g. "discord" + "-bridge.py"), or use names the lint doesn't catch.
Nits
lsof -whides the unreadable case on macOS (reap_orphans.py:432). For a process lsof may not read, it exits 1 with no output and no stderr, solistening()returns False ("no sockets") rather than None. I checked it on pid 1. Targets are the user's own processes, so the risk is low, but drop-w, or treat a member with a different uid as unreadable.run_lockruns outside the try (:693,:704). An uncreatable lock or log directory (e.g.--log /dev/nullputs the lock at/dev/process-hygiene.lock) crashes with non-zero instead of exiting 0.- Spotlight marker (
:590): useos.open(..., O_CREAT|O_EXCL|O_NOFOLLOW), so a dangling.metadata_never_indexsymlink is never followed. - Lint scope is overstated: the PR body says the lint "fails on any unbound child". It only checks
Popen(..., start_new_session=True)in files that mentionwatch-tasks-stream.shortask-notifier.sh(tests/watcher-fixtures-are-bound.test.py:23,40). - Optional: a Playwright node script whose group includes
chrome-headless-shellis signalled by its own pid only, and the orphaned Chrome children are never-touch.
Operational note: on a host with long-lived core sessions, real leaks in live sessions' scratchpads are never reaped, by design. The fixture-binding fixes, not the reaper, are what reduce load there.
…; neutral bridge fixture names /clear gives a running Claude Code process a new sessionId (same pid and procStart) and nothing on disk links it back to the old session, so the old scratchpad read "gone" while its owner ran. A session is now unknown (spared) while any live marker of the same project belongs to a process started no later than shortly after that session's directory was created. The never-touch fixtures name a neutral chat-bridge.py instead of a real channel bridge, which the hermetic bridge-test lint rightly refused. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
liususan091219
left a comment
There was a problem hiding this comment.
Comment, at b6b04e5. Nothing in my change-request list fires any more. Both earlier blockers are resolved. This is not an approval only because the approval bar is not met: there is no APPROVED review from a write-access reviewer at this head. The CHANGES_REQUESTED reviews I filed at 2040974 and 1d2bb81 still show on the PR. My concerns there are addressed, and nothing here asks for further changes.
Scope 1d2bb81..b6b04e5: three files: reap_orphans.py, its test, and SKILL.md. scripts/lint-hermetic-bridge-tests.py is not touched anywhere in the PR, and it now exits 0 on this tree. The fixture strings were neutralised (chat-bridge.py, example-bridge.py). CI: 33 success, 1 skipped, 0 failing. license/cla success (1 context). No unresolved threads. review-checks PASS, and there are no hardcoded workspace paths on added lines.
The /clear rule is sound, and it does fail closed. It does not compare against the time of the /clear. It compares the process start of every live same-project marker against the birth time of the scratchpad's session directory. A process that ran /clear necessarily started before the scratchpad it created, so started <= born always holds and the 120 s slack only widens what is spared. So "scratchpad created more than 120 s before the swap" cannot be reaped through this path. Every ambiguity also spares: a marker that is not a dict, one with no pid or procStart, one with no cwd, and an unparseable start time all return UNKNOWN. I checked procStart against this host's real markers: it is UTC, which matches utc_start's TZ=UTC.
Revert-to-fail, run with -B so no stale bytecode is used:
- drop the rule → 2 failures
- flip the slack sign → 2 failures
- treat an unknown start as not sparing → 1 failure
- treat a missing cwd as not sparing → 1 failure
All four were caught. A slack of 0 also turns one test red, but only because that test computes its "later" start from the slack constant, so the slack's value is not independently pinned. That is harmless, because the protection does not depend on it. The full suite is 74/74 OK.
One edge case I could not verify (not blocking): a process started with --resume that keeps an older session id and then runs /clear started after that scratchpad was born. It would read GONE. It only matters for a detached, socket-less orphan more than 2 h old in that scratchpad. Worth one line in SKILL.md if --resume does keep the id.
Is it a no-op? Not on this host. In report mode (no --reap) it found 23 roots: 14 GONE (all created 2026-10-04..10-16, before the long-lived core in that project started), 6 UNKNOWN and 3 ALIVE, with 0 candidates because nothing is orphaned there right now. On a host where one session outlives everything in its project, that project's scratchpads are spared indefinitely. Given that the example cron is report-only and the source-side fixture binding is what stops the leaks, I think that is the right trade, and it is stated in the PR.
Missing for approval: one substantive APPROVED review from a write-access reviewer at this head. Not verified: a --reap run on a real process table, which I deliberately did not run.
— Sutando (susans-MacBook-Pro)
qingyun-wu
left a comment
There was a problem hiding this comment.
Approve at b6b04e5. All 33 checks pass: newest run per name is 32 success and 1 skipped, and CLA is green.
- Bridge lint. The lint script and its workflow are byte-identical at 1d2bb81 and b6b04e5. It exits 1 at the old head and 0 at the new one. The only change is the neutral fixture names in
reap-orphans.test.py:272. - The
/clear120 s rule fails closed. An unparseableprocStart, a marker with nocwdorprocStart, a non-dict marker, and any OSError or ValueError all classify as UNKNOWN, and UNKNOWN is never reaped (reap_orphans.py:374-401). The Linux fallback birth timemin(mtime, ctime)can only err late, so it spares more, not less. Both sides are UTC (TZ=UTC, :295). Nothing deletes a scratchpad.--reapis opt-in (:658), and the shipped cron is report-only. - Tests:
reap-orphans.test.pyis 74/74 OK, and the binding tests pass. I did not re-run the mutants.
Non-blocking edges, not verified:
- If
--resumekeeps the old session id, a process resumed after the scratchpad was created and then/cleared reads as GONE. Its pid-1 orphans would then be exposed under--reap. - If a marker's
cwddrifts from the launch slug, for example after entering a worktree, the same-project match misses it.
Both affect only --reap, not the default report mode.
liususan091219
left a comment
There was a problem hiding this comment.
Approving at b6b04e5. This clears my two earlier change requests (at 2040974 and 1d2bb81); both are resolved.
The bar is met: qingyun-wu (write access, not the author's party) approved this head with a substantive review, and CI is 32 success, 1 skipped, with license/cla green.
Evidence from my review at this same head:
- Round-1 blocker (leaking sibling fixtures): fixed. The tests I reproduced leaking now leave nothing when stopped mid-run, and the new unbound-child lint flags main's leaking copy and passes here.
- Round-2 blocker (bridge lint misreading the new test): fixed with neutral fixture names, the lint itself untouched.
- The reaper is conservative and fails closed: same user, orphaned, older than 2 h, not protected, cwd inside a scratchpad; server sockets are spared; ambiguous cases spare; TERM → re-check → KILL; report-only by default with a 600 s floor on --reap. Removing any of these rules turns the tests red (74/74 pass).
Named, not blocking: the --resume + /clear edge (relevant only under --reap) goes into the planned SKILL.md follow-up, and nobody has yet run --reap against a real host.
— Sutando (susans-MacBook-Pro)
… cron New installs copy skills/schedule-crons/crons.example.json, so an entry there would give every user an hourly job. The skill is for Sutando developers: run by hand now, with health-check and witness-script triggers in a follow-up. The data pin now asserts no example entry and the developer-only wording. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@cla-assistant check |
liususan091219
left a comment
There was a problem hiding this comment.
Re-approving at 6273c3b.
I read the whole diff since my approval at b6b04e5 (one commit, 4 files). It only makes the skill developer-only: the process-hygiene entry is removed from crons.example.json (now identical to main), SKILL.md and the manifest say "Developer-only, not scheduled" and give the by-hand commands, and the data pin now asserts there is no shipped entry. No code in reap_orphans.py changed, so everything I verified at b6b04e5 still holds, and qingyun-wu's approval there covers the same code.
Dropping the shipped schedule also settles my earlier concern about new installs reaching an hourly job.
CI was still running when I submitted; please let it finish green before merge.
— Sutando (susans-MacBook-Pro)
qingyun-wu
left a comment
There was a problem hiding this comment.
Approve at 6273c3b. 32 checks passed and 1 was skipped.
The interdiff from b6b04e5 only removes things, as described, and touches no code:
crons.example.jsonno longer has a process-hygiene entry.- SKILL.md and the manifest now say "Developer-only, not scheduled".
- The data pin now asserts that the example crons have no process-hygiene entry, and checks both doc strings.
reap_orphans.py is unchanged, so my review at b6b04e5 still covers it.
What changed, and why
Agent sessions run tests from their per-session scratchpads. When a run is interrupted or times out, background children it started are re-parented to init and keep running for days: bash spin loops waiting on a file that never appears, fake-Chrome fixture servers,
node --testruns, a testfswatch, a hungswift -interpret, idle interpreters. On one host they kept the load average near 7; stopping them by hand brought it to 2.5. Spotlight also indexed the temp churn.This PR adds the remedy and closes the sources it could trace:
skills/process-hygiene/—scripts/reap_orphans.py(stdlib only). It reports by default;--reapstops. A candidate must be all of: this user's, PPID 1, older thanPROCESS_HYGIENE_MIN_AGE_S(manifest, default 7200 s), with a cwd (realpath;lsof -a -p PID -d cwd -Fnon macOS,/proc/PID/cwdon Linux) inside<tmp>/claude-<uid>/<slug>/<session-id>/scratchpad, and that scratchpad's owning Claude Code session has ended (a live session may have started a server there on purpose; see "Owning-session liveness" below), and no process in its group holds a listening TCP or any UDP socket (a deliberate server is spared even after its session ends; unreadable spares). Never touched: the full list is a table in SKILL.md (tmux,claude, the Claude Code CLI, Sutando core/worker/tmux/relay/Station, the task watcher and notifiers, the voice agent,*-bridge.py,core_heartbeat, MCP servers, browsers, apps), every pid inlaunchctl list, itself and its ancestors. A symlinked scratchpad, or one resolving outsideclaude-<uid>, is not a root.--reaprefuses a minimum age under 600 s, and a run holds an exclusiveflockso two runs cannot overlap.Stopping is per target, never by pattern: re-read pid, start time (
ps -o lstart=, C locale), cwd and command right before the signal and abort on any change (PID reuse); SIGTERM the group only when every live member qualifies, else the pid; waitPROCESS_HYGIENE_GRACE_S(default 5); SIGKILL survivors only after a second identical re-check. One log line per action in<workspace>/logs/process-hygiene.log(resolved withworkspace_default.resolve_workspace). Exit 0 for every run that parsed its arguments; 2 on usage errors.--spotlightdrops an empty.metadata_never_indexinto each scratchpad root and eachclaude-<uid>directory (idempotent, never truncates an existing one).skills/schedule-crons/crons.example.json, so an entry there would give every user an hourly job; this PR adds none, and the manifest and SKILL.md say developer-only. It runs by hand for now; its triggers (a health-check load warning that runs it once when the cause matches, and the end of witness scripts) come in a follow-up.Owning-session liveness (how "ended" is decided)
For each Claude config dir (
CLAUDE_CONFIG_DIR, the home.claude, and theclaudeentries ofcore_config_dirs;--claude-config-diroverrides):<config>/sessions/<pid>.jsonhassessionId== the scratchpad's session id and itspidis running with start time == the marker'sprocStart(compared againstps -o lstart=underTZ=UTCand the C locale, so a recycled pid does not count), or the transcript<config>/projects/<slug>/<session-id>.jsonlchanged within the minimum age;sessions/dir exists, neither alive condition holds, and no live marker of the same project belongs to a process started no later than 120 s after the session's directory was created (the/clearcase, round 3);sessions/dir or marker, a marker missingpid/procStart). Unknown is treated as alive: never reaped.Measured on a developer Mac before choosing it:
lsofover every live Claude process showed none holds its<session>.jsonlopen (it is appended, not held), and a session'stasks/*.outputfiles are open only while a background command runs, so neither separates an idle live session from an ended one.pid= the live CLI process, andprocStartmatched that pid'sps -o lstart=exactly underTZ=UTC(Fri Oct 2 02:46:19 2026both; the marker'sstartedAtepoch is the same second). In local time the strings differ by the UTC offset, hence theTZ=UTC. The check reads alive.catsit in the scratchpad of a session that is still running, so they are now left alone./clearrule: 0 candidates on that host (21 roots: 6 alive, 5 gone, 10 unknown). The fixture server's session is now unknown: the core in the same project was already running when that session's directory was created, so it could be that session after a/clear. That is the fail-closed cost: with a long-running session in a project, the reaper leaves that project's scratchpads alone.tests/_helpers/process_group.py(the existing owner of fixture process groups) gains:bind_to_test(pgid)/popen_bound(...): a watchdog holds the read end of a pipe and SIGKILLs every still-bound group when the test process exits by any means, SIGKILL included; a group whose leader start time changed (recycled pgid) is skipped.OWNER_LIFELINE_PY+owner_env(): for fixtures the test starts only indirectly; they exit once the owner pid is gone or has another start time.Applied to: the fake Chrome in
tests/claude-gemini-agy-browser.test.py; the watcher (and stubfswatch) plus live-pid holders intests/watch-tasks-stream-shutdown-leaves-inflight-claim.test.py; the real-fswatchwatchers intests/watch-tasks-stream-config-hot-reload*.test.pyandtests/watch-tasks-stream-foreign-instance-id-corrected.test.py.Placement
Walked the checklist in
docs/architecture-boundaries.md: not an inline tool, not phone/voice session code, and nothing in core needs it — it is an optional, self-contained host-maintenance capability, so it is a skill (item 4; "prefer the more specific layer"). Its two knobs are declared in the skill'smanifest.jsonconfigblock and read CLI > env > manifest perskills/MANIFEST.md; no new undeclared env var. The fixture cleanup is test tooling, so it extends the existingtests/_helpers/process_group.pyrather than adding a second helper.Not fixed here, with reasons
while [ ! -e "$GO" ]; do sleep 0.02; donenotifier loops (the bulk of the load) come from_PAUSE_AFTER_RESULT_CHECKintests/codex-core-launcher.test.pyon the open feat(codex-core): opt-incodex queuedelivery for the task notifier #5629 branch; the test is not onmain, so it cannot be changed here. Suggested change for that branch:bind_to_test(proc.pid)in_locked(it already usesstart_new_session=True) and a deadline in the loop, e.g.while [ ! -e "$GO" ] && [ $SECONDS -lt 300 ]; do sleep 0.02; done.swift -interpret: not traceable to an in-repo test. Measured:xcrun swift file.swiftruns as a singleswift-frontend -interpretprocess (no grandchild), so the twoxcrun swifttests'subprocess.run(timeout=)already reaps it, and their deliberately hung harness ran and left nothing. The orphan most likely came from an ad-hoc run killed by a tool timeout; the reaper covers that when its cwd is a scratchpad.node --test,agyon a devtools port, straycat, idle interpreters: no in-repo fixture starts them; the reaper covers them.cwd=<repo>) is deliberately out of the reaper's reach: nothing about its provenance distinguishes it from a service the owner started there. Those are fixed at the source: every test onmainthat starts the watcher, a task notifier or another long-runningsrcscript in its own session now goes throughpopen_bound(round 2 below), and a lint test keeps it that way.Round 2 (review at 2040974) — item → fix
lsof -w -a -p PID -iTCP -sTCP:LISTEN -iUDP;/proc/<pid>/net/{tcp,tcp6,udp,udp6}on Linux); unreadable spares the group--reapwhile a plain orphan beside it is reaped; group/unknown/lsof//procunit tests--reapon by default--reapis a documented opt-incrons.example.json+ SKILL.mdmainthat starts a long-runningsrcscript withstart_new_session=True(task notifiers, codex launcher, start-cli, voice lock), go throughpopen_bound— 20 files, 38 call sites, includingtests/fixtures/readiness_window_helpers.pytests/watcher-fixtures-are-bound.test.py(AST scan; fires onorigin/mainwith the converted files listed; positive and negative controls); repro belowclaude-<uid>is a symlink or not this user's, or any component below it is a symlink; the resolved root must sit under the resolvedclaude-<uid>;--spotlightonly writes thereclaude-<uid>, another owner'sclaude-<uid>, Spotlight writes nothing outsidewatch-tasks-stream.sh,task-notifier.sh,voice-agent,-bridge.py,core_heartbeat,sutando-station, MCP server names; SKILL.md has a table of exactly what is protected and whyunbindbefore the group is gonekill_groupfirstrun_in_own_grouphas no caller--min-agefloor under--reap--reapwith a minimum age under 600 s exits 2 (report mode has no floor)owner_env()passes pid and start time; the lifeline exits when the pid's start time differs (reads/bin/ps, since a fixture can run under a test's stubbedps)flockonprocess-hygiene.lockbeside the log; a second run prints that and does nothingrunnot calledPath.is_dirraises on a mockedstat)owner_stateis inside onetry; any failure is unknown (spared)os.staton thesessions/probe, raisinglistdir, raisingopen, the mode-000 dirSusan's repro,
runt <test> & sleep 4; kill -TERM $!(env -i,nice -n 19), new processes listed 3 s later,origin/maincopy vs this head:(The base leaks were killed by pid afterwards.)
Round 3 — item → fix
scripts/lint-hermetic-bridge-tests.pyrefused the never-touch fixture stringpython3 src/discord-bridge.pysrc/chat-bridge.py,relay/example-bridge.py) that the same-bridge.pyrule protects; the lint is unchangedpython3 scripts/lint-hermetic-bridge-tests.py→ rc 1 at 1d2bb81 (names the file), rc 0 at this head;--diffrc 0/clearin a live sessionsessionIdchanged under a live pid → unknown; process started after the directory → gone; other project → gone; dead marker pid → gone; marker without cwd/procStart → unknown/clearmeasurement (a throwaway interactiveclaudein a scratch tmux server on its own/tmpsocket, default login, killed afterwards along with its project and temp directories):/clear:sessions/<pid>.json→sessionId 9a292797…,procStart Mon Oct 19 10:01:08 2026; the temp tree had…/9a292797…/scratchpad./clear: the same file and pid now saysessionId b444a61c…,procStartunchanged; a new…/b444a61c…/directory andb444a61c….jsonltranscript appeared; the old…/9a292797…/scratchpadstayed.lsofon the process shows nothing open under the old session directory (its cwd is the project dir); the marker has no parent/previous-session field. On clean exit the marker file is removed./clearwould make a live session's old scratchpad read gone. The rule now: a session is unknown while any live marker (pid running with itsprocStart) has acwdin the same project (Claude Code's project name: every non-alphanumeric →-; all 5 live markers on the host map onto their existing temp andprojects/directories this way) and its process started no later than 120 s after the session directory was created (the measured directory was created ~0 s afterprocStart, ~1.5 s beforestartedAt, soprocStartis the comparison). A marker with nocwdorprocStartcounts as the same project. SKILL.md documents this and its cost.What to review first
skills/process-hygiene/scripts/reap_orphans.py—classify,plan,recheck,stop.tests/_helpers/process_group.py— the watchdog,popen_bound,OWNER_LIFELINE_PY/owner_env;tests/watcher-fixtures-are-bound.test.py.Evidence
Report mode on a developer Mac (default rules, nothing signalled; this PR never ran
--reapon a real host), at HEAD:(Before the owning-session rule the same run listed 4; the other 3 belong to a session that is still live.)
On the same host the always-on services with PPID 1 (the core's tmux server,
startup.shchildren, dashboards, a livefswatchon the workspace, a relay bridge, Playwright's headless shell, an 18-day-old leaked watcher fixture whose cwd is the repo) were all left out.Before/after: SIGTERM the test mid-run, then list what is left (same probe for both;
origin/maincopy vs this branch):(The processes the BASE runs leaked were killed by pid afterwards.)
Tests run
All one file at a time, foreground,
nice -n 19, underenv -i(no inheritedSUTANDO_*), macOS; reaper suite and lints at b6b04e5, the converted fixture suites at 1d2bb81 (unchanged since):tests/skills/process-hygiene/reap-orphans.test.py(new)scripts/lint-hermetic-bridge-tests.py(whole tree, and--diff)tests/process-group-binding.test.py(new)tests/watcher-fixtures-are-bound.test.py(new lint)origin/maincheckout it fails and lists the unconverted filestests/claude-gemini-agy-browser.test.pyclaude-task-notifier-inflight21,claude-task-notifier-submit17,restart-voice-agent-lock,startup-voice-wedge-reap,voice-lockcodex-core-launcher(80/81),start-cli-model-pin(2 failures)origin/maincheckout under the sameenv -irunner (test_launches_codex_and_managed_task_notifier; model-pindefault/env-ignored), so they are host-environment failures, not from this diff; CI is the arbiterci-covers-every-python-test71,test-discovery-has-one-owner14,python39-guard-follows-skills,schedule-crons-example-digest-delivery,skills/pending-questions/pq,cron-runnergit diff origin/main...HEAD | bash scripts/review-checks.sh→ PASS;ruff checkon every changed Python file → clean;git diff --check→ clean;scripts/lint-skill.py skills/process-hygiene --strict→ 0/0. Coverage ofreap_orphans.pyunder the repo.coveragerc: 99% (7 of 553 statements missed).The reaper's real-process tests start actual orphans under a fake scratchpad root passed as
--tmp-base; their kill/killpg are wrapped to refuse any pid the test did not start, and teardown kills only pids it started, verified by start time. Never run with--reapagainst a real host.Mutants (each applied alone, the named suite re-run; KILLED = suite went red)
Round 3, at b6b04e5: every reaper mutant re-run against
reap-orphans.test.py— 46 run, 44 KILLED, 2 survived (the same two equivalents below), including the 5 new/clearmutants (rule removed; any project counts; start-time bound ignored; a dead marker pid counted; a malformed other-session marker skipped) — all KILLED.Round 2, at 1d2bb81: 49 run, 47 KILLED, 2 survived (both equivalent).
claude-<uid>accepted; another owner'sclaude-<uid>accepted; new never-touch names dropped; Xcode toolchain treated as an app; no--min-agefloor; no run lock; owner-state probe errors not caught (the 3.12 case); lifeline ignores start time; lifeline uses PATHps; a helper fixture reverts to an unboundPopen; lint never flags;killpgEPERM not handled; unreadablesessions/read as empty — all KILLED.procStartnot compared, marker ignored, recent transcript not alive, malformed marker skipped, example cron with--reap— all KILLED.claude-<uid>" (with symlinked hits already skipped, no path can resolve outside; the check is defense in depth); Spotlight marker opened"w"instead of"a"(an existing marker is skipped before any open).Feature/documentation consistency
skills/process-hygiene/SKILL.mdis the canonical doc for the new skill (rules, launchd reasoning, what is out of scope, Spotlight, developer-only use, config). Nodocs/page catalogs individual skills, sodocs/catalog.jsonis unchanged.Risks, worst case, rollback
--reapby hand.--reap: a process deliberately started inside an agent scratchpad, whose session has ended and that is over 2 h old, is stopped (SIGTERM, 5 s grace, logged). Anything in a live session's scratchpad, or whose session cannot be determined, is left alone.systemd --userrather than PID 1): nothing qualifies; the reaper is a no-op there..metadata_never_indexis honoured below a volume root depends on the macOS release (unverified); the SKILL.md names the System Settings → Spotlight → Privacy alternative.Round 4 (6273c3b)
process-hygieneentry incrons.example.json, developer-only wording in SKILL.md and the manifestNo repo convention marks a skill developer-only (no manifest field or installer exclusion exists), so it is stated in the manifest description and SKILL.md.
reap-orphans.test.py74 OK, the bridge lint andscripts/review-checks.shpass at this head.Running it by hand
🤖 Generated with Claude Code