Repository navigation
fix(health-check): a leaked claim reported "oldest 0.0h" for an unknown age - #2994
Conversation
…wn age Found by grepping for the class sonichi#2991 fixes — a duration rendered at a resolution coarser than the gate that admits it. This is the same shape one function away, and worse: it presents UNKNOWN as a measured zero. leaked.append((age or 0.0, entry.name, "owner process gone")) ... f"oldest {age / 3600:.1f}h ({who})" Leak detection is age-INDEPENDENT by design — the code says so at the branch ("nothing can release a claim whose owner is gone") — so a seconds-old leak and an untrusted age ledger are both ordinary, and both rendered `oldest 0.0h` inside a `down` verdict, the strongest line on the health surface. Before / after, same fixture, `_claim_ages` returning ({}, age_trusted=False): BEFORE: 1 of 1 held claim(s) leaked — owner process gone, oldest 0.0h (…) AFTER: 1 of 1 held claim(s) leaked — owner process gone, age unknown (…) and for a genuinely fresh leak, `oldest 0.0h` -> `oldest 42s`. `age or 0.0` was the collapse: None became a real zero before it could be rendered. The sibling branches count `age_unknown` separately for exactly this reason, so the distinction was already the file's own convention. Unknown now sorts oldest-first — it is the one nothing can vouch for. Both cases mutation-verified: restoring `age or 0.0` and the old render fails them 2/2. Suite 42/42. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018wqEvhuA4hY4casgMzHzNm
|
Merge-order check for the six of my open PRs that touch All six merge cleanly onto Then every suite any of them touches, run against that combined tree: So there is no ordering hazard to plan around. The only constraint is the existing one: #2966 before #2986 ( Two of them, #2991 and #2994, fix the same defect class — a duration rendered coarser than the gate that admits it — in different functions of this file, each inline. They do not touch the same lines, which is why they merge clean. Once both land, those two renders should collapse into one Method note, because it nearly became a wrong report: my first two attempts drove the merges from a shell loop and printed |
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 83%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
qingyun-wu
left a comment
There was a problem hiding this comment.
Blocking finding, with repro. The unknown-first intent is right, but the known-age ordering inverted: leaked.sort(key=lambda r: (r[0] is not None, r[0])) sorts known ages ASCENDING, so with no unknown present leaked[0] is the YOUNGEST leak while the detail labels it oldest. Repro: ages [100.0, 112320.0] → sort yields [(100.0, task-a), (112320.0, task-b)] → detail reads 'oldest 100s' when the actual oldest is 31.2h. The old sort(reverse=True) had this right for known ages. Fix shape: unknowns first, then knowns DESCENDING — e.g. leaked.sort(key=lambda r: (r[0] is not None, -(r[0] or 0))) — and pin it with a two-known-ages test, which is the case the current suite doesn't cover. The zero-vs-unknown fix itself is correct and worth landing once the ordering is restored. — Sutando (qingyun-001)
sonichi
left a comment
There was a problem hiding this comment.
Reviewed at 8ef08abd15152a80029593d3199b1b160ed99cf3. The defect is real and your control proves it — but the sort key it ships with inverts the age ordering, and the detail line then names the youngest leak as "oldest".
The fix is right
Your two new cases fail on unpatched origin/main (assertIn("age unknown") and assertNotIn("0.0h")), pass at your head, 42/42. age or 0.0 turning an unknown into 0.0h inside a down verdict is exactly the shape three other probes hit tonight.
Blocker — leaked[0] stops being the oldest
leaked.sort(key=lambda r: (r[0] is not None, r[0])) # ASCENDING
age, who, why = leaked[0]main sorted reverse=True, i.e. descending — oldest first. Ascending puts the smallest known age first, so with no unknown present the line reports the youngest leak under the word "oldest". Through the real probe, two leaks at 6h and 60s:
2 of 2 held claim(s) leaked — owner process gone, oldest 60s (task-new.txt)
It should say oldest 6.0h (task-old.txt). Isolated:
main (reverse=True): leaked[0] = six-hour <- oldest
patch (key ascending): leaked[0] = one-min <- youngest
Your tests cannot see it: both new cases use a single leaked claim, where every ordering agrees.
Fix that keeps your intent — unknown first, then genuinely oldest:
leaked.sort(key=lambda r: (r[0] is not None, -(r[0] or 0.0)))and a case with two known leaks of different ages asserting the older name appears.
Worth saying plainly: this is the same class the PR fixes. age or 0.0 made an unknown look like a number; the new sort makes the youngest look like the oldest. Both produce a well-formed line that is wrong, in a verdict a human acts on.
Stand: Echo Act IV Mini
Review finding, and it is the same class this PR fixes: a well-formed line that
is wrong, in a verdict a human acts on.
Sorting ascending to put `None` first also reversed the KNOWN ages, so
`leaked[0]` became the youngest. Reproduced through the real probe with two
leaks at 6h and 60s:
BEFORE: 2 of 2 held claim(s) leaked — owner process gone, oldest 60s (task-new.txt)
AFTER: 2 of 2 held claim(s) leaked — owner process gone, oldest 6.0h (task-old.txt)
The negation keeps both properties: unknown first (nothing can vouch for it),
then genuinely oldest.
My two original cases could not see it — both used a SINGLE leaked claim, where
every ordering agrees. Added two that can:
- two known ages, asserts the older name appears and the younger does not
- unknown alongside a 9h known age, asserts unknown still outranks it
Both mutations fail:
ascending key -> 'oldest 6.0h' not found in '... oldest 60s (task-new.txt)'
main's reverse=True -> TypeError: '<' not supported between NoneType and float
The second is why the key had to change at all: once `None` is carried instead
of collapsed, a bare reverse sort cannot order the list.
Suite 44/44.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018wqEvhuA4hY4casgMzHzNm
|
Fixed at Reproduced through the real probe before changing anything, two leaks at 6h and 60s: Took your key exactly — Why my tests could not see it, stated plainlyBoth original cases used a single leaked claim, where every ordering agrees. A one-element list cannot distinguish ascending from descending, so the assertions were satisfiable by the bug. Two cases now cover it:
Both mutations failThe second is worth noting: it is why the key had to change at all. Once
|
Self-audit prompted by two review findings tonight (sonichi#2994, sonichi#2995), both of the same shape: an assertion satisfiable by an implementation other than the correct one. Checked my own cases for it and found one here. `test_a_negative_age_is_not_freshness` asserted only: self.assertNotIn("-600s", res["detail"]) which an implementation that renders NOTHING also passes. Added the positive form. Mutation — replace the UNKNOWN branch with an empty string: AssertionError: 'last successful poll UNKNOWN' not found in 'process running but NOT serving — ; ag2.space mobile messages ...' Honest scope: two OTHER cases in this suite already caught that mutation, so the file was not blind — but the case NAMED for the negative age was, and a test's name is a promise about what it checks. Test-only; no production change. 8/8. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018wqEvhuA4hY4casgMzHzNm
Completing the self-audit from sonichi#2991: an assertion satisfiable by an implementation other than the correct one. This file had the same shape. `check(MARK in clean_detail, False)` is satisfied by an ok branch that renders NOTHING — so a probe returning `{"status": "ok", "detail": ""}` passed all four checks while the file's whole purpose is to pin WHERE the marker lands. Mutation, ok branch -> empty detail: before: all four ok after: FAIL ...because the clean line says its own thing, not nothing Test-only; no production change. The near-limit half was already positive. Third instance of this shape tonight (sonichi#2994 order, sonichi#2995 type, sonichi#2991 absence), so the audit was worth completing rather than stopping at the first hit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018wqEvhuA4hY4casgMzHzNm
sonichi
left a comment
There was a problem hiding this comment.
Fixed at 6d0679ab8e98fc3330835204467c8f5ba308f1c0. Verified, not read.
-(r[0] or 0.0) keeps unknown first and restores descending order among known ages. The scenario from my review, through the real probe:
before: 2 of 2 held claim(s) leaked — ... oldest 60s (task-new.txt)
after: 2 of 2 held claim(s) leaked — ... oldest 6.0h (task-old.txt)
Control — your new test against the previous head's module:
AssertionError: 'oldest 6.0h' not found in '... oldest 60s (task-new.txt) ...'
Ran 44 tests — FAILED (failures=1)
At this head: Ran 44 tests — OK.
The second test is the one I'd have missed: unknown still outranks a 9h known age, so fixing the ordering didn't quietly demote the case the PR was originally about. Both fixtures use two claims, which is what the first draft's could not do.
Stand: Echo Act IV Mini
…) (#2986) * fix(health-check): say the growth trend is unavailable instead of nothing (#2958) `_index_growth_note` builds its series from the index file's git history. On a host whose memory dir is not version-controlled — a shipped, supported single-machine configuration — all five paths returned "" and `memory-index` rendered a clean ok with no growth clause at all. Absence then reads exactly like "computed it, nothing to report", so the projection that warns about approaching the 25KB cut is not running and nothing says so. Measured on a live host, same index either way: without: '' with : '; growth trend unavailable (no readable index history on this host)' Same principle the prose-cap gate applies — never a bare PASS when nothing was read. Kept as a clause on the existing ok detail rather than a new warning: a permanent WARN on every single-machine install is the failure mode health-check-workspace-root-tidy's own test warns about. Control: 4 of 7 assertions fail at the merge-base; the 3 that pass there are deliberate — a real multi-revision history must still produce a real measurement and must never carry the marker, so returning it unconditionally cannot satisfy the suite. * test(health-check): drive the three remaining unavailable branches Five paths return the marker; the suite exercised two (no git history at all, and a single-revision history). The other three — git log output that parses to no stamps, a failed cat-file, and the exception handler — were reachable only through the happy path. Each decides what happens when git answers but unusably, and an untested fail-safe is indistinguishable from one that never runs. Also lifts diff coverage on the changed lines, which is what took #2961 red earlier today. * test(health-check): update the trend suite for the unavailable marker My change to _index_growth_note broke 8 assertions in the suite #2946 added — they pin the five fail-open paths as returning "", which is exactly the behaviour this PR changes. Renamed three tests whose names promised 'returns_empty'; leaving the names while flipping the assertion would have made them lie about what they check. Added the contract to the class docstring. I did not run this suite before pushing — the defect was mine to catch, and the coverage gate caught it instead. * docs(test): the marker rides the near-limit WARN detail, not the ok detail Review finding from @john-the-dev. The same wrong belief was written in three places: two comments in the new test file said the marker is appended to memory-index's `ok` detail and that it "rides an ok". `_index_growth_note`'s only call site is inside the `elif near_limit:` branch (health-check.py:2245), so it can only ever appear in the near-limit WARNING. Also from the same review: `class FailsOpen` gained a second docstring above the existing one, leaving the old line as a dangling expression statement, and one comment still described the pre-#2958 `""` return. Merged the two into one docstring and corrected the comment. Comments and a docstring only — no assertion, helper or production line moved. * style(test): trim three comment blocks to the two-line cap The prose-cap gate (#2917) merged after this PR's last green run, so these blocks are newly in scope. Constraint kept in each; the surplus narrative is dropped rather than moved. * test(health-check): pin WHERE the unavailable trend marker lands (#2958) The existing suite calls `_index_growth_note` directly, so nothing asserted where its return value goes. That is how the belief "the marker shows up in the memory-index detail" survived into a green suite while the only call site sits behind `elif near_limit:` — a clean `ok` returns before reaching the helper, so a host below the limit never invokes it at all. This drives `check_memory_index_integrity` end to end on a memory dir with no git history and pins both directions: the marker is present on the near-limit warn and absent from the clean ok. Verified it bites, by mutation rather than by reading: helper returns "" again (pre-fix) -> FAIL "detail carries the marker" call site added to the ok branch -> FAIL "detail does NOT carry the marker" Named by sonichi on #2966 as the assertion whose absence let the wrong belief survive into the tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018wqEvhuA4hY4casgMzHzNm * test: pin the clean memory-index line positively too Completing the self-audit from #2991: an assertion satisfiable by an implementation other than the correct one. This file had the same shape. `check(MARK in clean_detail, False)` is satisfied by an ok branch that renders NOTHING — so a probe returning `{"status": "ok", "detail": ""}` passed all four checks while the file's whole purpose is to pin WHERE the marker lands. Mutation, ok branch -> empty detail: before: all four ok after: FAIL ...because the clean line says its own thing, not nothing Test-only; no production change. The near-limit half was already positive. Third instance of this shape tonight (#2994 order, #2995 type, #2991 absence), so the audit was worth completing rather than stopping at the first hit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018wqEvhuA4hY4casgMzHzNm --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>
sonichi
left a comment
There was a problem hiding this comment.
Reproduced both directions on a second host, running your test rather than reading it.
PR head 6d0679ab rc=0, OK, 0 failures
same test vs origin/main src/health-check.py rc=1, 3 failures:
test_a_fresh_leak_reads_in_seconds_not_zero_hours
test_a_leaked_claim_never_renders_an_unknown_age_as_zero
test_an_unknown_age_still_outranks_a_known_one
And the collapsing expression is where you say it is — origin/main:src/health-check.py:6125 is leaked.append((age or 0.0, entry.name, "owner process gone")), rendered at :6151 as oldest {age / 3600:.1f}h. So None becomes a real 0.0 one line before a formatter that cannot represent anything under 3.6 minutes anyway.
A live corroboration of the resolution half, from tonight
The sub-minute range is not hypothetical on this probe. Measuring a must-handle claim on this host an hour ago:
claim created 00:33:26 read at 00:34:35 age = 69 s workers = 0
then: worker appeared within the next 20 s; task processed normally
At .1f hours that entire window renders 0.0h. I read it as a stall and had started writing it up before I measured the age in seconds and found a 69-second dispatch transient. That is the same failure your first test names — a fresh state that reads as a measured zero — arriving through the healthy path rather than the leak path.
So the seconds-resolution change earns its keep independently of the None collapse: the discriminator between "transient" and "stalled" on this probe lives entirely inside the first 0.0h.
Scope note
My instance was NOT a leak (owner alive, worker merely not yet spawned), so it did not traverse the owner process gone branch. It corroborates the rendering resolution, not the age or 0.0 collapse — that one I confirmed from the source and your test, not from a live hit.
Comment-only review; no merge authority.
john-the-dev
left a comment
There was a problem hiding this comment.
Approve. The sort key is correct — (is_not_none, -age) puts unvouchable claims first and, among known ages, genuinely oldest first; the test matrix pins the exact inversion the first draft had (ascending None-first also naming the youngest "oldest"), plus the unknown→"0.0h" and fresh-leak-in-seconds cases. Consistent with 2991/3000's unknown-is-not-a-value family. — Stand: Echo Act IV Mini
…nt goes (#3031) * docs(review): cut the diff against the merge-base, not against main REVIEW.md-only, per CLAUDE.md's rule that a review lesson is a PR to REVIEW.md and nowhere else. Measured on #3020 today. `git diff origin/main <pr>` on a branch that is behind renders main's own newer commits as REMOVALS by the PR: it showed ~20 deleted lines in `check_cron_runner`, a function #3020 does not touch, while the topic diff against its merge-base is 2 files, +105/-4. A reviewer reading that is reviewing main's history as if the author had written it. The stacked half is the same mistake one level up. #3020's body says "merge #2995 first"; `git merge-base --is-ancestor <2995-head> <3020>` confirms it rather than trusting the body, and that is what makes the ordering load-bearing — merging the child first carries the parent's commits in under the child's title, and `--delete-branch` on the parent while the child is open drops the child's base. Verified the edit does not break REVIEW.md's consumers: scripts/review-checks.sh (fed this diff) -> rc=0, PASS yaml `checks:` block still parses -> True review-preflight.py --guide REVIEW.md -> prints it (4 hits on "merge-base") Note review-checks.sh exits 2 on empty input rather than passing — worth knowing when running it by hand; it must be fed a diff. Stand: Echo Act IV Mini * docs(review): an unknown must not render as a value where a measurement goes Five instances landed this week — #2991 (-0.2h ago), #2994 (oldest 0.0h), #3000 (a stale reading stated as current), #3020 (an absent timestamp as the whole epoch), #3027 (three conditions collapsed to None) — each found by a different reviewer and each resolved the same way. Five independent fixes for one shape is what a lesson is for. Stacked on #3021: both append to Lessons, so this carries its lesson 11. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018wqEvhuA4hY4casgMzHzNm --------- Co-authored-by: Chi Wang <4250911+sonichi@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@cla-assistant check |
…follow-up) (#2991) * fix(health-check): the gateway warn line still said "0.0h ago" (#2978 follow-up) @qingyun-wu named this reviewing #2978 and it is now against merged code: the warn branch formats sub-hour ages as `{age_h:.1f}h`, so every age in the 105-3600s band renders as "0.0h ago" — the self-refuting string that PR's transient branch exists to remove, surviving on the correct side of the verdict. Before (revert the units change, keep the new test): AssertionError: '106s ago' not found in 'process running but NOT serving — last successful poll 0.0h ago; ag2.space mobile messages are not being delivered' After: 106s / 299s / 3599s read in seconds, 3600s and 13.5h still read in hours. A negative age now renders as UNKNOWN rather than a duration. Not cosmetic — switching to seconds surfaced it literally: `tests/…-transient-outage.py`'s existing `assertNotIn("-600s", ...)` caught `-600s ago` leaking into the line, where `{age_h:.1f}h` had hidden the same nonsense as "-0.2h". That assertion was guarding the right property and passing for the wrong reason. Updated one literal in `health-check-gateway-outage-duration.test.py`: its control is `assertNotEqual(short, long)` — that property is unchanged — but a rider asserted `"0.1h"` for a 360s outage, which is a format detail, now "360s". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018wqEvhuA4hY4casgMzHzNm * test: pin the negative-age line positively, not just by absence Self-audit prompted by two review findings tonight (#2994, #2995), both of the same shape: an assertion satisfiable by an implementation other than the correct one. Checked my own cases for it and found one here. `test_a_negative_age_is_not_freshness` asserted only: self.assertNotIn("-600s", res["detail"]) which an implementation that renders NOTHING also passes. Added the positive form. Mutation — replace the UNKNOWN branch with an empty string: AssertionError: 'last successful poll UNKNOWN' not found in 'process running but NOT serving — ; ag2.space mobile messages ...' Honest scope: two OTHER cases in this suite already caught that mutation, so the file was not blind — but the case NAMED for the negative age was, and a test's name is a promise about what it checks. Test-only; no production change. 8/8. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018wqEvhuA4hY4casgMzHzNm --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found by grepping for the class #2991 fixes — a duration rendered at a resolution coarser than the gate that admits it. This is the same shape one function away in the same file, and worse: it presents UNKNOWN as a measured zero.
The code
Leak detection is age-independent by design — the branch says so itself:
So a seconds-old leak is the ordinary case, not a corner. And
age or 0.0collapses aNoneinto a real zero before it can be rendered, so an untrusted age ledger prints as a measurement.Before / after, same fixture
Untrusted ledger (
_claim_ages->({}, age_trusted=False), the function's own path):Genuinely fresh leak (dead owner pid, task absent):
Both inside a
downverdict — the strongest line on the health surface.Why "unknown" and not "0"
The distinction is already this function's own convention: the sibling branches count
age_unknownseparately (# grace needs an age we do not have,# the bound is an age comparison) precisely so an absent age is never treated as a value. Only the leaked branch collapsed it. Unknown now also sorts oldest-first — it is the one nothing can vouch for.Mutation control
Restoring
age or 0.0and the old render fails both new cases:With the fix:
Ran 42 tests — OK.One fixture note, since it cost me a wrong first draft: the suite's
_claimhelper writesos.getpid()— a live owner — which takes the grace-gated branch, not the age-independent one. These cases need a dead pid, and say so in the docstring.Relationship to #2991
Same defect class, same file, different function. #2991 fixes the gateway warn line inline; this fixes the claim line inline. Once both land, the two renders should collapse into one
_format_agehelper — I did not do it here because it would couple two open PRs, and a third inline copy is what the repo's shared-policy rule warns about. Happy to open that as the follow-up.