fix(doctor): sweeps.alive reports unknown, not FAIL, for a root the service does not serve (model_performance-jyg) - #77
Conversation
627f648 to
1dbce0f
Compare
Manager verification — FIX. Rebased onto current main (which moved 7 commits). Merging.Head Fail-before, against current mainThe target this lane was filed for
That is the first time it has passed here. The root cause is a scope join, and the fix follows the right precedent
Fixed by reading the served root back out of the unit's own Gates
Squash + |
Manager verification — the masking defect that hid two real bugs this batch. Merging.Rebased onto current The mechanism, and why it was never a test bug
Fixed by reading the served root back out of the unit's own The conservative case is the part I checked hardest, because this is where a fix like this usually goes wrong — silencing the real signal to make a test pass: # when `served_root` is None (unit unreadable, no --root at all, or the caller
# passed a path we cannot resolve) -> goes on to evaluate the heartbeat and can still FAILSo "cannot tell" resolves toward still evaluating, and the FAIL path is untouched against the served root. A genuinely dead sweep on a real installation still fails loudly. Gates
The cli tier is the headline. The one remaining Squash + |
…he service does not serve `_check_sweeps_alive` joined a SERVICE-scoped fact (`describe_service()` -- one singleton unit per user) to a ROOT-scoped one (a heartbeat file under whichever workspace root `doctor` was pointed at). That join is only sound when the two refer to the same root. Against any other root the heartbeat is absent BY CONSTRUCTION, and the check reported that absence as "no heartbeat ever recorded ... the loop may never have started" -- a hard FAIL, exit 1, on a machine whose sweep loops were provably running fine. Every isolated test root is such a root, so tests/cli/test_cli_surface.py::test_doctor_quick_succeeds_against_the_real_ installed_bd failed on any developer machine whose service serves a real workspace -- and, because `assert returncode == 0` came first, it MASKED every later assertion in that test. Measured twice: model_performance-wp6 (announcement-predicate collision, invisible locally, CI-only, blocked PR #70 for days) and model_performance-kxk. The assumption is right; it is simply not evaluable against a root the supervisor was never given. `_serve_argv_tail` already bakes `--root` into the unit as an explicit argument, so the served root is readable: `ServiceInfo.served_root` reads it back (systemd ExecStart, launchd ProgramArguments), and both sweep checks report `unknown` on a mismatch -- following the precedent `sweeps.reclaiming` set in model_performance-oy4 (46d7da4) for exactly this shape. The FAIL path is untouched where it matters: against the SERVED root, a missing, stale, or dead-pid heartbeat is still a loud failure, and a sweep that failed on every project still fails `sweeps.reclaiming`. When the served root cannot be determined at all, the checks evaluate the heartbeat anyway and can still FAIL -- "cannot tell" must never become a way to make a real dead-loop failure disappear. Separately, the masking itself is fixed: the cli test now checks its three properties independently and reports them together (`_util.assert_doctor_run_is_clean`), so the next environmental exit-1 from any cause cannot hide the next real defect. `assert_no_silent_failure` and its announcement predicate are called unchanged. doctor MEASURED at 38/38 (unchanged).
…/38; five tiers green)
1dbce0f to
8efd055
Compare
2 participants
Closes
model_performance-jyg.The defect
cli._check_sweeps_alivejoins two facts of different scope:--userunitservice.describe_service()heartbeat.heartbeat_path(root)That join is only sound when both refer to the same root. It never checked.
doctorruns against whichever root it was pointed at (AMPLIFIER_WORK_TRACKER_ROOT,--root); the heartbeat is written by the supervisor under the root it was given. Against any other root the file is absent by construction, so its absence proves nothing about the loops — and the check reported it as[FAIL] sweeps.alive, exit 1, on machines whose sweep loops were provably healthy.Measured on the dev host:
while
tests/conftest.py:299points everyrun_clicall at a freshtmp_path_factoryroot.The cost was masking, not noise.
test_doctor_quick_succeeds_against_the_real_installed_bdassertsreturncode == 0first, so this environmental exit-1 killed the test before its two later assertions ran — hiding real defects twice:model_performance-wp6(announcement-predicate collision; invisible locally, CI-only, blocked #70 for days) andmodel_performance-kxk.Is
sweeps.aliveright?Yes — the assumption is correct, and an isolated root is not a legitimate place to evaluate it. The honest report is neither PASS (claiming proof we lack) nor FAIL (red-lining a healthy service): it is
unknown, the third optionsweeps.reclaimingalready established inmodel_performance-oy4(46d7da4).Detectable because
_serve_argv_tailbakes--rootinto the unit as an explicit argument (deliberately, per the unit template's own comment) — so the served root is readable, and the mismatch is decidable rather than guessed.The "have the TEST provision a heartbeat" option was rejected: it makes the test manufacture the evidence it then asserts on, and leaves every other
doctorcaller unable to tell a wrong-root from a dead loop.Changes
service.py:ServiceInfo.served_root; pure readersparse_systemd_served_root/parse_launchd_served_root(None= "cannot tell", never a guess);_read_served_rootnever raises.cli.py:_served_root_mismatchgate — both sweep checks reportunknownon a mismatch. Dependency ordering unchanged.tests/_util.py+tests/cli/test_cli_surface.py: the test's three properties are now checked independently and reported together.assert_no_silent_failureand its announcement predicate are called unchanged — nothing narrowed, test neither deleted nor skipped.The FAIL path still fails
sweeps.reclaiming)unknownRow five is the point: "cannot tell" must never become a way to make a real dead-loop failure disappear. Pinned as a test so it cannot be quietly inverted.
Evidence
doctor— MEASURED, never computed: 38/38, unchanged (noAGENTS.mdedit needed)sweeps.aliveno heartbeat ever recordedunknown -- the installed service serves --root /home/.../.amplifier-work-tracker, not /tmp/...reap sweep completed 64s ago (threshold 900s); notify sweep completed 261s agoThe third row is load-bearing: against the served root the assumption is still genuinely evaluated, not skipped.
Target test, same box
assert result.returncode == 0—1 failed in 49.67sevidence/tier-cli.log:89)Masking demonstration (injected
wp6-shaped defect, same box, reverted before commit)BEFORE (pristine
46d7da4): the injected defect appears 0 times in the whole log;silent-failure guardappears 0 times — the guard never ran. The reader sees only thesweeps.aliveFAIL.AFTER:
Also pinned permanently by
test_an_exit_1_does_not_hide_a_silent_failure_announcement(no bd, no subprocess).All four documented tiers + the modules suite, by name
make test-unitmake test-integrationmake test-climake test-ledgermake ledger-mutatemake test-moduleruff check/ruff format --check/pyrightclean (0 errors, 0 warnings). The knowntest_supervisor_web.pyport-binding flake did not occur.31 new tests across
test_sweeps_root_scope.py,test_served_root.py,test_doctor_surface_failures.py. Raw logs committed underdocs/lanes/jyg-doctor-sweep-heartbeat/evidence/; full write-up in that lane'sDONE-NOTE.md.Spend
$0.00 against a $0.00 authority (
0 runs x 0 arms x $0 / 1.00 = $0.00). Pure code/test change; no infrastructure created.