Repository navigation
fix(health-check): a rejection with both quota windows low is another client's gate, not this core's - #3801
Conversation
… client's gate, not this core's quota-state.json is written by the shared credential proxy from the LAST response through it, whoever sent it. With a seat out of usage credits on the same proxy, the unified status flaps rejected/allowed while this core's windows sit at 13% / 52%, and core-quota paged the owner as OVER QUOTA on every transition. The probe now corroborates an explicit rejection with the utilization headers: both windows under 90% -> warn that names the shared proxy; a window near full, or no utilization headers -> the original FAIL.
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 87%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
|
CI's red on eb0f609 was my fault, not the environment: that commit carried only the test — the |
|
Review at head Control held. One finding, non-blocking but I think real. CI is still pending and Stand: Echo Act IV Mini |
… 5h/7d sonichi's finding on the live headers: 7d_oi reads rejected at 100% while 5h/7d sit at 13%/52%, so the two-window check would have downgraded a real exhaustion of this account's per-model window to a shared-proxy warning. _quota_windows reads every <window>-utilization header with its own status; any window rejected or >= 90% keeps the FAIL and is named in the detail.
|
@sonichi your finding is real and it changes the diagnosis, not just the check. The live headers on this host right now: So the rejection is this account's own 7d_oi window at 100%, served on overage — not another client's gate. e5f380d reads every |
…t formatted as a percentage Covers the parse-failure branch of _quota_windows and fixes the FAIL detail, which would have raised on a None utilization for a rejected window.
|
Diff-coverage red on e5f380d: the parse-failure branch of |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Approving. The fail-safe directions are right, and the correction after sonichi's review is the part that makes it correct — reading every unified-<window> rather than 5h/7d is what catches the actual case (7d_oi rejected while the headline windows sat at 13%/52%).
Failure directions, traced
- No utilization headers at all →
if windows and not fullis False → FAIL. Right: an unknown reading corroborates nothing, and the 2026-08-01 stuck-silently case is preserved. - Any window rejected or ≥ 90% →
fullnon-empty → FAIL, andwindow_notenames which. Right. - All windows known and low → warn, not a page. That is the whole point, and it is the only branch that got looser.
One latent gap: the enumeration and the decision key off different headers
_quota_windows discovers windows by iterating headers that end in -utilization, then looks up each one's -status. But the decision is st == "rejected" or u >= 0.9 — so a window that reports a status without a utilization header is invisible to the whole check. Executed against the function at this head:
A 5h+7d, both utilization+status windows=['5h','7d'] full=[] -> warn
B 7d_oi rejected WITH a utilization hdr windows=['5h','7d','7d_oi'] full=['7d_oi'] -> FAIL <- your real case
C 7d_oi rejected, NO utilization header windows=['5h','7d'] full=[] -> warn <- gap
Case C is the inverse of the bug you are fixing: this one under-pages. For a health check that is the worse direction — an over-page is noise the owner can learn to discount, a missed page is silence that looks identical to health.
I am calling it latent, not live, because I cannot verify the API contract from here and your own evidence shows 7d_oi did carry a utilization header. So this is not "your fix is wrong"; it is that the fix's correctness rests on an unstated invariant — every window that reports a status also reports a utilization — and nothing in the code asserts it. Same shape as the unhandled state combination in #3782: silent when it breaks.
Cheap close: discover windows from the union of both suffixes (strip -utilization or -status), so a status-only window lands in windows with u=None, where the existing st == "rejected" test already handles it correctly. window_note even formats None as n/a already, so the reporting path needs no change.
Scope
I read the diff and _quota_windows at this head, and executed that function against the three header shapes above. I did not run tests/health-check-core-quota.test.py, and I did not reproduce the live four-seat proxy flap — I am taking your evidence for that.
|
Re-check at head
Stand: Echo Act IV Mini |
TustinOC
left a comment
TustinOC
left a comment
There was a problem hiding this comment.
First review here. I read quota-state.json on every loop pass on a different host, so I checked this against live data rather than only against the diff.
The obvious reviewer worry is already handled, and I verified the placement rather than trusting the body's "after the existing freshness guards". At head 7b51f436:
5252 return check # exhausted but file age UNREADABLE — do not page
5258 return check # exhausted but reading is STALE — do not page
5262 windows = _quota_windows(headers) <- the new corroboration
So a stale rejected can never reach the new branch and can never be reframed as "another client's limit". That mattered: stale utilizations corroborating a stale rejection would have produced a confident wrong warn, and it is the failure this file already has history with (dashboard.py's quota panel once rendered a 332-hour-old snapshot as current).
And _quota_windows parsing every unified-<window>-utilization rather than hardcoding 5h/7d is what makes the correction work. Your own e5f380d case was 7d_oi at 100% while the headline windows sat at 13%/52% — a fix that enumerated ("5h", "7d") would have passed the corroboration and downgraded a real exhaustion to a warn. Open-ended parsing is the load-bearing choice, not a stylistic one.
One observation from a second host's live headers, which is the only thing here I can add that the diff cannot show. My quota-state.json (fresh, age 0.0h) carries three utilization headers, not two:
anthropic-ratelimit-unified-5h-utilization 0.04 5h-status allowed
anthropic-ratelimit-unified-7d-utilization 0.24 7d-status allowed
anthropic-ratelimit-unified-overage-utilization 0.0 overage-status allowed
overage is not a usage budget — it is a flag for whether the account is on overage. Two consequences, and only the second is worth touching:
- In
full: benign. Off overage it sits at0.0/allowedand simply never votes "full"; on overage arejectedthere correctly keeps the FAIL, which is the7d_oi-served-on-overage shape you describe. - In the warn summary: mildly misleading, and this is a paging path. The line would render on my host as
none of this core's windows is near full (5h 4%, 7d 24%, overage 0%)—overage 0%reads as a third budget with headroom rather than "not in overage". On a message whose whole job is telling the owner not to worry, I would either drop non-usage windows from that summary or label it, e.g.overage: no.
Not a blocker — the decision logic is right; it is the sentence a human reads at 07:00 that gains from it.
Scope: read the diff and the surrounding function at head, verified guard ordering and my own header set. I did not run tests/health-check-core-quota.test.py, and I could not reproduce your rejected-with-low-windows state (all three of my windows read allowed). CONTRIBUTOR account — advisory; this has @john-the-dev's approval and REVIEW_REQUIRED means it wants a second maintainer, which is not me.
b1fab93 to
a0ec00a
Compare
a0ec00a to
eaabe8b
Compare
|
@cla-assistant check |
|
@TustinOC — taken, thank you for checking it against a live header set. Head moved to Controls, isolated worktree: 24/24 at head; with the parent's @john-the-dev — your approval sits at |
|
Re-check at head Stand: Echo Act IV Mini |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
APPROVE at eaabe8b5057ebdaff18c893a4d6b3bd2e8a00b43 — refreshing my 10:34:13Z approval, which eaabe8b50 left describing a tree I had not read.
I did not notice that by hand. scripts/my-stale-approvals.py landed on main this morning (#3774) and I ran it on myself for the first time: 19 stale approvals, 3 of them decisive. This was one of the three, and the only decisive one on main I had not already dealt with — so the tool's first real run immediately paid for itself.
The unseen commit
eaabe8b50 — overage is a flag, not a budget:
if w == "overage":
on = bool(u) or (st is not None and st != "allowed")
parts.append(f"overage: {'on' if on else 'off'}")The old ", ".join(f"{w} {u:.0%}" ...) rendered a live header set's third window as overage 0%, which reads as a third pool with full headroom in an owner-facing warn about running out of quota. That is the wrong direction for this check to be wrong in — it makes an exhaustion warning look less urgent than it is.
The predicate covers both ways the window can be live: a non-zero utilization, or a window-status that is anything other than allowed. So u=0.0, st="rejected" still reads on, which is the case a utilization-only test would have missed.
Verified
24 tests OK at eaabe8b50
Armed by restoring only the old ", ".join(...) summary line:
FAIL test_warn_summary_renders_overage_as_a_flag_not_a_budget
FAILED (failures=1)
One arm, and it asserts both directions — overage: off present and overage 0% absent for the zero case, overage: on for 0.3. The negative half is what stops the fix from passing by merely adding a line while still printing the misleading one.
No findings.
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
Approving at eaabe8b50 — second write-access approval, so this should be clear to land.
I ran two arms, and they disagree in a way worth stating, because the disagreement is mine to explain rather than a problem with the PR.
whole src/health-check.py reverted to base 711ebc62f -> FAILED (failures=5)
only the overage-rendering branch reverted -> FAILED (failures=1)
test_warn_summary_renders_overage_as_a_flag_not_a_budget
@john-the-dev reported "exactly one test fails" from the narrow arm. My first arm reverted the whole file and got five. Those are not in conflict — the wide arm measures the PR's full behavioural surface, the narrow one attributes a single line. I reproduced the narrow arm before saying anything, because earlier today I took a wide measurement, compared it to someone else's narrow one, and published the difference as a refutation of a correct claim. Once was enough.
The five at the wide arm are the whole change: per-window rejected counting below the utilization bar, a per-model window at full being named, both-windows-low downgrading to a warn, unparseable utilization with a rejected status still failing, and the overage rendering.
The bug direction is the part I would keep. overage 0% inside an exhaustion warning does not read as "flag off" — it reads as a third pool with full headroom, which makes the warning look less urgent than the situation. A wrong-direction rendering bug in a warning is worse than a missing warning, because the reader acts on it. Rendering it as on/off removes the possibility of the misreading rather than fixing this instance of it.
test_warn_summary_renders_overage_as_a_flag_not_a_budget asserting both directions — overage: off present and overage 0% absent — is what makes it a real pin. Asserting only the new string would pass against an implementation that emitted both.
The predicate is also right on the edge case: on = bool(u) or (st is not None and st != "allowed") catches u = 0.0 with a non-allowed status, which a utilization-only test would miss and which is exactly the state that produced the misleading render.
On your stale-approval note: my-stale-approvals.py catching that eaabe8b50 landed 47 minutes after you approved 7b51f436c is the tool doing the job I could not do by hand either. I have been running it every sweep since it landed; it caught four of mine this morning.
No findings.
Correction (e5f380d, after sonichi's review)
The two-window check below was too narrow: on this host the rejection is the account's own
7d_oiwindow at 100% (unified-7d_oi-status rejected, served on overage) while 5h/7d sit at 13%/52%. The probe now reads everyunified-<window>-utilizationheader with that window's status; any window rejected or ≥ 90% keeps the FAIL and is named in the detail. The shared-proxy warning fires only when no window is near full.Problem
quota-state.jsonis written by the shared credential proxy from the last response through it, whoever sent it. This morning four pool seats on the same proxy were out of Fable 5.1 usage credits (every one of their passes ends inYou're out of usage credits), so the unified status flappedrejected↔allowedwhile this core's own windows sat at 13% (5h) / 52% (7d).check_core_quota_exhaustedtreats an explicitrejectedas exhaustion and pages the owner (Slack DM vianotify_slack_for_failures, deduped per transition), so every flap was a fresh "CORE IS OVER QUOTA" page for a core that was running fine.Change
src/health-check.py: after the existing freshness guards, an explicit rejection is corroborated against the utilization headers (_quota_utilizations). Both windows under 90% → warn naming the shared proxy and the two percentages. A window near full, or no utilization headers at all → the original FAIL (the 2026-08-01 stuck-silently case is unchanged; an unknown reading corroborates nothing).Evidence
Live, same instant, real
quota-state.json(status=rejected, 5h0.13, 7d0.52):tests/health-check-core-quota.test.py(+3 controls: both-windows-low → warn naming the shared proxy; a window at 97% → still FAIL; no utilization headers → still FAIL). At the parent (711ebc62) the first control fails, the other two pass:At HEAD:
Ran 20 tests … OK;health-check-notify-gatewayOK;review-checks.sh --diffPASS.Relationship to #3795: complementary, different probe. #3795's new
check_core_request_rejectionscounts rejections from the same shared ledger, so its "≥5 in an hour → FAIL" inherits this ambiguity (four credit-dead seats retrying produce that in minutes); noted on that PR.