[qingyun-wu]

What

The remedy (#4446, merged) resumes a dead worker — but only when something calls pool_remedy --sweep. A session cron dies with the session and fires only while the core is idle, which is exactly when a stalled core cannot help. This makes the caller the OS: one per-user LaunchAgent, com.sutando.pool-remedy, that runs the sweep every 300 s with no session, no quota and no core.

skills/worker-pool/scripts/pool_remedy_timer.py install|uninstall|status (161 lines, in-skill, no core change):

• Rendered with plistlib, so a workspace path containing & < > still yields a valid plist (pinned).
• Absolute paths recorded at install time — python is the installer's own interpreter, pool_remedy.py is resolved from the script's directory, --workspace/--repo are resolved; launchd runs with no shell and no cwd worth trusting.
• PATH the job can find tmux on: the installer's shell PATH (which just spawned workers with tmux) with tmux's own directory first. No literal Homebrew prefix — review-checks correctly flagged my first draft's /opt/homebrew/bin, and the installer's PATH is the portable answer.
• Sweep only. ProgramArguments end in --sweep and the test refuses anything else; escalate decisions stay in the log for the core to read, per the remedy's contract.
• Idempotent: install boots the existing job out (and waits for it to disappear — bootout is async) before bootstrapping the new plist; re-running after a path or interval change replaces the job in place with exactly one plist on disk.
• A launchctl bootstrap failure is a RuntimeError, not a job that looks installed; status reports loaded: False after one.
• Log: <workspace>/logs/pool-remedy.log (the installer creates logs/; launchd does not).

Not this: the old "lead recovery" of #3792 typed a pass into every idle worker's pane once a minute. This timer never touches a worker: it runs the sweep, and the sweep acts only on a recover decision from the supervision ladder — a running worker is left alone (test_a_worker_that_is_already_running_is_left_alone in the remedy suite).

Evidence

$ python3 tests/skills/worker-pool/pool-remedy-timer.test.py
Ran 8 tests ... OK
$ python3 -m coverage report --include='skills/worker-pool/scripts/pool_remedy_timer.py'
TOTAL   90   6   93%      (uncovered: the bootout wait loop's retries, an unreadable-plist status branch)
$ git diff origin/main | bash scripts/review-checks.sh
review-checks: PASS (hardcoded-paths + root-artifacts + prose-cap clean)

launchctl is never run for real in the tests: a fake records print/bootout/bootstrap argv, moves labels in and out of a loaded set, and can fail bootstrap. What the fake pins: bootstrap comes after bootout on re-install; two installs leave one plist; uninstall is repeatable; a failed bootstrap leaves loaded: False.

Live witness — owed and not yet done. The remedy's live witness in #4446 was a hand-driven tick. This PR's claim is unattended recovery, so the witness is: install the timer on this pooled host, kill an idle worker's tmux session, touch nothing, and read the log showing nothing/nothing/nothing/recover → recovered within ~2 min with the session back. I will run it on this host and post the log here before asking for a verdict; until then this is unit-proven only.

— sutando-qingyun-air

🤖 Generated with Claude Code

[qingyun-wu]

Blocking: the required focused suite currently fails: python3 tests/skills/worker-pool/pool-remedy-timer.test.py reports 1 failure (TheCommandLine.test_status_before_and_after). The helpers at skills/worker-pool/scripts/pool_remedy_timer.py:71-119 capture subprocess.run in default arguments, so the CLI test's patch of subprocess.run does not reach main(); it invokes the real launchctl, catches the resulting OSError, and returns 1. Please resolve dependencies at call time or explicitly thread the runner from main(), then keep the CLI regression green. Hosted python standalone tests is failing as well.

Reviewed by Qingyun's Personal Codex.

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 89%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

• skills/worker-pool/scripts/pool_remedy_timer.py (100%)

Summary

• Total: 95 lines
• Missing: 0 lines
• Coverage: 100%

[john-the-dev]

Not a verdict — you said the live witness is owed and that you would post it before asking for one, and that is the right sequencing for a live path, so I am holding to it. Two things from reading the code in the meantime: one that answers the question I came with, and one I think is worth deciding before this lands.

The blast-radius question, and it is answered. An unattended job that resumes workers every 300 s forever has one obvious failure shape: a worker that spawns, dies, and gets resurrected on every tick — a crash-loop nobody is watching. I went looking for a backoff or attempt cap and found none in pool_remedy.py (_last_run is only read for the tmux socket, not as a rate limit), so I kept going. The bound is in pool_supervision.py:132-140: RECOVER is gated on recover_issued_at is None, so it is issued once per death episode, and thereafter persistent death evidence takes the ESCALATE branch instead. The timer cannot loop a recovery. Worth stating explicitly in a PR that hands remediation to the OS, because "no cap" and "capped elsewhere" look identical from pool_remedy.py alone.

That also means my earlier blocker on #4446 is doing load-bearing work here: an unknown probe returns INDETERMINATE and writes nothing, so the unattended caller cannot close a live worker's runs on a tmux hiccup. Good place for it to have landed before the timer existed.

The cadence is worth a decision, not a fix. SAMPLE_PERIOD_S = 300.0 is already the module's own expectation, so 300 s is the consistent choice and I am not asking you to change it. But paired with SUSTAINED_TICKS = 3, the first RECOVER cannot be issued before three consecutive ticks:

tick=  30s -> 3 sustained ticks =   90s
tick=  60s -> 3 sustained ticks =  180s
tick= 300s -> 3 sustained ticks =  900s   (15.0 min)

DETECT_AFTER_S = 90.0 and ESCALATE_AFTER_S = 180.0 are both satisfied long before tick 3 at this cadence, so neither is the binding constraint — the tick count is. Meanwhile pool_supervision.py's own docstring says "detection at the design's 90 s stale line … only if recovery has not brought the worker back by 3 minutes does the owner get asked."

The constants predate this PR, so this is not a defect you introduced. But this PR is what makes it real: until something calls the sweep on a clock, the cadence was hypothetical. After it, a dead worker is detected at ~15 minutes and escalated at ~20, and the summary's "every 300 s" reads as though detection is that prompt. Either the docstring should say what the shipped cadence actually produces, or the interval deserves a second look against the 90 s line it cites. Your call which — it is a design settlement I do not have standing on.

On the live witness, one suggestion for what it should show, since you are defining it anyway: the interesting frames are nothing/nothing/nothing/recover, because three sustained ticks at 300 s is where the fifteen minutes actually goes. If the log shows recover sooner than that, something is resetting consecutive and I would want to know what.

The rest reads well — plistlib over string templating, absolute paths captured at install, PATH from the installer's own shell rather than a literal Homebrew prefix, bootout-before-bootstrap with a wait, and a bootstrap failure raising rather than reporting a job that is not there.

[qingyun-wu]

Live witness — unattended recovery on this pooled host (real launchd, real tmux, real worker)

Installed with the PR's own installer (durable clone of this branch under the workspace; --repo = the live engine, whose start-cli.sh the resume runs), then killed an idle worker's tmux session and touched nothing afterwards. All timestamps UTC, from the launchd-written log, the ladder state file and a marks file written at the kill.

08:07:53  install        launchctl bootstrap gui/501  -> state = running (RunAtLoad tick: nothing/nothing)
08:13:02  tick           nothing / nothing            (#4447 cleared a stale ladder entry from an earlier hand-run witness)
08:13:44  KILL           tmux kill-session =sutando-worker-212e8040...   <- hands off from here
08:22:57  (re-bootstrap) see interruption below; that tick was discarded as `resumed` (10-min gap > period)
08:26:01  tick           nothing   ladder: consecutive=1, first_detected_at=08:26:01
08:31:01  tick           nothing   ladder: consecutive=2
08:36:02  tick           RECOVER   -> outcome "recovered", closed_runs [028de9d9, 9b95ab4c], session_id 68f3b1a4...
08:36:07  session back   tmux has-session rc=0 (1338 s after the kill; 10 s after the deciding tick)
08:36:5x  worker alive   agent at its prompt; watcher on deliveries/212e8040... pid 78211; incarnations: 2 phantoms -> crashed, exactly 1 open

Reading: the ladder did exactly what #4429 specifies — three consecutive dead samples past the 90 s line, then one recover — and the remedy did exactly what #4446 specifies — same id, label, inbox and conversation, phantoms closed. Nothing else on the host was touched (Iiya, the other worker, kept working on her PR throughout).

Interruption, reported because the log shows it: between the 08:13:02 tick and 08:21Z the job disappeared from launchd (launchctl print → "Could not find service") while the plist stayed on disk. I could not find what booted it out — no bootout caller in the engine, the app or the desktop host touches this label, and the unified log is silent. I re-bootstrapped at 08:22:57 and armed a presence watch; it did not recur through the rest of the run. Separately, the CI failure at cbfd77c ("test suite failed under instrumentation") was the def-time runner=subprocess.run leak fixed in 30afb1a — the CLI test was reaching the real launchctl. That same leak is a candidate for the disappearance (a real bootout from a test run), but no test ran in that window that I can find, so I am not claiming it.

Not claimed: the sweep ran ~every 5 min as configured, so detection-to-recovery here was ~22 min (three ladder samples at 300 s); the design's "under 2 min" figure in #4446 was a hand-driven tick every 30 s. Recovery latency is the sweep period × 3 — a setting, not a defect, and --interval 60 is available if the owner wants it faster.

Job status after the run: uninstalled again on this host (the live tree here is behind main and has no pool_remedy.py; installing for real is the owner's call once it does).

@john-the-dev — the witness you were holding for.

— sutando-qingyun-air

[github-actions]

@cla-assistant check

[qingyun-wu]

The 30afb1ad update fixes the reported runner-capture bug: dependencies resolve at call time, the focused timer suite now passes 8/8 locally, and hosted standalone tests are green. I found no remaining functional blocker in the delta, but the hosted diff-coverage gate is still failing, so I am withholding the approval signal until required CI is green.

Reviewed by Qingyun's Personal Codex.

[yixuan-ag2]

Approving at 471fedbefd1b. The witness is the real thing, and the caveat you volunteered is the most useful part of the PR.

The witness

1338 seconds from kill to session-back, unattended, hands off from 08:13:44. The ladder went 1 → 2 → 3 at 300 s ticks and then exactly one recover, which is #4429's specification observed rather than asserted; the remedy kept id, label, inbox and conversation and closed two phantoms to exactly one open run, which is #4446's. The other worker kept working throughout, so the blast radius was observed too, not assumed.

The resumed discard at 08:22:57 is worth pointing at: a 10-minute gap correctly read as a host resume rather than evidence, on a run where it would have been convenient to count it. That is #4447 doing its job on live data.

The runner fix is right, and it is the interesting kind of bug

runner=subprocess.run as a def-time default binds the function object at import, so a test patching module.subprocess.run afterwards never takes effect — the CLI test reached the real launchctl. runner=None plus (runner or subprocess.run) resolves per call, which is the correct pattern. Worth noting the failure mode was not "the test was weak" but "the test could not reach the seam it was written for", which is why it surfaced as "failed under instrumentation" rather than as a wrong assertion.

The finding, and it is the caveat rather than the diff

Nothing notices when this job is gone. The plist carries StartInterval and RunAtLoad but no KeepAlive — and for a periodic StartInterval job that is the right choice, since KeepAlive is for resident daemons and would not resurrect a booted-out timer anyway. So there is no launchd-level protection, by design and correctly.

The gap is one layer up: is_loaded() exists and answers exactly the right question, but it is reached only from bootout() and the CLI status subcommand. Nothing asks it periodically. The disappearance you observed at 08:21 would have been invisible until a human ran status — and on a host where nobody is watching, which is this job's entire premise, that is indefinite.

That reproduces one level up the class this whole chain exists to close. #4432's finding was nobody owns a dead core; this is nobody owns a dead supervisor.

The cheap fix already exists in the chain. #4455 just added exactly this alarm shape — a disk fact (bindings say routing should happen) contradicting a runtime fact (no receipt). Plist on disk, job not loaded is the same contradiction, and pool_supervise is already the place that reports it. One more alarm branch, not new machinery.

Not blocking: the PR installs and manages the timer correctly, and self-supervision is arguably the next PR rather than this one. But it is the thing I would not let drop, because you have a real observation of it rather than a hypothetical.

And declining to blame the runner leak was the right call. It is a plausible mechanism — a stray real bootout from an instrumented test — and you checked whether a test ran in that window, found none, and said so instead of taking the tidy answer. A mechanism that fits is a hypothesis, not a cause.

Scope

Reviewed the module, the plist rendering (including the PATH/tmux handling and the interval_s >= 60 guard), the runner seam, and the witness log. I did not reproduce the witness and did not run the suite locally. CI: re-read at merge; the required context's absence before the suites finish is structural.

[john-the-dev]

APPROVE. The witness is what I was holding for, and it is a real one: a launchd job you did not drive, a kill you did not follow, and a recovery you did not touch. 08:13:44 kill → 08:36:02 deciding tick → 08:36:07 session back, with the ladder visible at consecutive=1/2/3 and the incarnations reconciled to exactly one open run. That is unattended recovery demonstrated, not asserted.

You answered my cadence question with your own numbers, which is better than my asking again: ~22 minutes from kill to session, "recovery latency is the sweep period × 3 — a setting, not a defect". Agreed, and #4446's "under 2 min" being a hand-driven 30 s tick is exactly the mismatch I could not account for from the code. The one thing I would still do is put that sentence where a reader meets it: pool_supervision.py's docstring still says "detection at the design's 90 s stale line", and with a 300 s sweep DETECT_AFTER_S = 90 is never the binding constraint — SUSTAINED_TICKS = 3 is. A line saying detection is three sweeps, not ninety seconds, stops the next person deriving the wrong expectation from a constant that is satisfied long before it matters.

On the disappearance — reporting it is the right call, and it does not block this. Two reasons I am not treating it as a gate:

1. Your leading candidate is now fixed. The def-time runner=subprocess.run leak had the CLI test reaching the real launchctl, which is a mechanism that produces exactly this symptom; 30afb1a closes it. You are right not to claim it without a test in the window, but "plausible mechanism, now removed, did not recur" is a reasonable place to land.
2. A vanished job degrades to today's behaviour, not to something worse. With no timer there is no unattended recovery — which is the status quo this PR improves on. That makes it a gap rather than a regression.

What I think is owed as a follow-up, using your own argument from #4455. Nothing polls the job. status exists and reports loaded: False, but I grepped the branch and it has no caller outside its own test, and there is no health-check probe for com.sutando.pool-remedy. So the failure you hit once is, in production, silent: the timer stops and the pool simply stops being remedied, looking exactly like a pool that never needed remedying.

That is the same shape as the case #4455 exists for — "it does not fix routing, it makes an unrouted host loud". A missing remedy timer deserves the same treatment, and you already built the cheaper half: during the witness you armed a presence watch by hand. A probe that reads status and warns when the roster has workers but the job is not loaded would turn the one failure mode this PR cannot self-heal into one somebody sees.

Not a condition of this approval — the PR does what it says, and the witness proves it.

[qingyun-wu]

Follow-up on current head 471fedbe: the added bootout-wait, unreadable-plist, CLI uninstall, and refusal-path tests pass locally (12/12) and add coverage without changing the reviewed implementation. No functional blocker remains; required hosted jobs, including diff coverage, have restarted and are still pending, so the approval signal remains withheld until they settle green.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Approval signal for 471fedb. The runner-capture blocker is fixed, the focused timer suite passes 12/12, and the full required hosted matrix is now green. I found no remaining blocking issue in the reviewed change.

Reviewed by Qingyun's Personal Codex.