fix(heartbeat): give the capacity retry floor one ceiling, not two (BLO-28919) - #1441
Conversation
…LO-28919)
`resultJson.retryNotBefore` is a retry floor with TWO writers, and they
disagreed about its ceiling:
- `persistProviderCapacityRetry` (wake gate) runs the advertised reset
through `resolveCcrotateCapacityRetry` and persists the CLAMPED
instant — its own comment explains why, since
`scheduleBoundedRetryForRun` pushes `dueAt` out to this field.
- the finalize writer persisted the advertised reset verbatim, so the
identical capacity denial fell through to the generic 24h backstop
(MAX_TRANSIENT_RETRY_HORIZON_MS) instead of the 15m capacity ceiling.
One field, one consumer, two ceilings 96x apart. Measured on a full
parked census 2026-08-19 (`truncated: false`): 484 of 700 fleet parks
under `scheduledRetryReason = "transient_failure"` at p50 4.6h with
p90 == max == exactly 1440.0m, while correctly-gated capacity parks sat
at 17.9m for the same underlying error. ~99% of that population was
floor-driven: 244 rows at attempt 1 against a base curve whose first
slot is 1.5-2.5m, with only 3 rows landing inside 5m.
The label is how they diverged: `transient_failure` is the DEFAULT
`retryReason` of `scheduleBoundedRetryForRun`, and a capacity reset that
arrives as prose (parsed server-side per BLO-18278) reaches finalization
with the capacity family set but no capacity reason.
Two changes, and the second is what makes the first safe:
1. Clamp a capacity floor at the finalize writer, through the same
helper the gate writer uses so the two cannot drift again. Strictly a
shortening operation — adopted only when it lands earlier than what
was advertised, so a stale floor is never pushed out.
2. Extend the promotion-time capacity re-probe to cover a capacity park
labelled `transient_failure`, and relabel it on re-defer. Promotion
does NOT run the wake-time penstock gate —
`promoteDueScheduledRetries` reads `scheduled_retry` rows directly and
never enters `wakeup()`, where `gateAppliesToWake` lives — so before
this, those rows promoted straight to `queued` with no re-probe.
Shortening their horizon without this would burn a paid dispatch per
hop into a pool that is still empty, which is BLO-24011 inverted.
This is also the AC's labelling criterion: such a park no longer
reads as `transient_failure`, so the census split-check is meaningful.
BLO-18285's requirement is preserved and improved: the run still parks
in `scheduled_retry` (a live execution path, so the strand sweep leaves
the issue alone) and still never takes the flat 90s hop — but an 88.8h
advertisement no longer costs 24h of silence before the first re-probe,
and recovery lands within one ceiling of capacity actually returning.
Also corrects MAX_TRANSIENT_RETRY_HORIZON_MS's docblock. Its claim that
"at 24h every retry the fleet actually schedules today is unaffected"
was falsified by the census (p90 == max == 24h means it was binding on
>=10% of rows, not idle). A bound that silently became load-bearing is
how this class keeps recurring; the comment now says that a future
census showing it binding is evidence of a new unclamped writer
upstream, not a number to tune.
Tests: the promotion case fails without change 2 (the park is promoted
into a closed pool), with a hintless-transient negative control beside
it. Writer-parity invariants are sited next to the BLO-23525 regression
suite as the AC asks. Four cases in
heartbeat-provider-capacity-horizon.test.ts asserted the superseded
"park verbatim at the advertised reset" contract and are revised in
place, each keeping its original intent (do not strand, do not take the
flat hop) with the supersession explained.
269 tests pass across the 10 affected suites; server typecheck clean.
Co-Authored-By: Claude <noreply@anthropic.com>
|
@ally please review at head 107d48e — BLO-28919, heartbeat retry scheduler. Three things I most want a second pair of eyes on:
Specifically NOT asking you to re-verify the census numbers — those are measured and cited on the issue. |
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
The comment justified clamping the capacity floor by asserting that the wake-time ccrotate gate (`gateAppliesToWake`) covers a promoted retry, so coming due would re-probe rather than dispatch. That is wrong, and it is the exact error the audit for this fix made before catching it. `promoteDueScheduledRetries` selects `scheduled_retry` rows straight from the DB and calls `promoteScheduledRetryRun` — it never enters `wakeup()`, so the wake-time gate never evaluates for a promoted retry. The only protection is the capacity re-probe branch in `promoteScheduledRetryRun`, which is precisely why this change extends that branch rather than only lowering a ceiling. No behaviour change; the shipped code already relied on the promotion branch. But a comment that misattributes the safety mechanism invites the next reader to weaken the clamp half without the re-probe half, which would burn a paid attempt per hop against a closed door. States the real dependency and says not to break one half without the other. Co-Authored-By: Claude <noreply@anthropic.com>
|
@ally head moved to cdae88d — one comment-only follow-up commit, please review at this head.
No behaviour change — the shipped code already depended on the promotion branch, not the wake gate. But it is worth a look because it is the one place a future reader could be misled into removing the re-probe half while keeping the clamp half, which is the unsafe combination. The three review-focus items from my previous request stand unchanged. |
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: cdae88d
Taking the three review questions in order, then the findings.
Q1 — is errorFamily === "rate_limit_exhausted" + floor the right boundary? Yes, and the symmetry is checkable rather than assumed. The finalize-side clamp gates on providerCapacityThrottleOverride, which is rateLimitExhaustedOverride || providerThrottledNoProgressOverride (heartbeat.ts:24977), and both disjuncts persist errorFamily: "rate_limit_exhausted" (heartbeat.ts:25376). So the population the finalize writer clamps is exactly the population the promotion branch captures — there is no row that gets a shortened floor without the paired free re-probe, which is the failure mode that would have mattered. Requiring the floor is also defensible: it is the floor, not the family, that produces the long park. Worth noting for a follow-up that a floorless rate_limit_exhausted park has the same un-gated-dispatch exposure on this path (12 × 90s), just cheaply — pre-existing, out of scope here.
Q2 — the attempt-budget consequence. This is where I disagree with the PR, and it is the Critical finding below. The direction is right but the magnitude is inverted from how the description frames it: the relabelled population does not get a wider budget, it gets roughly 12 hours of total coverage where it previously had ~288h. Details and arithmetic below.
Q3 — the four revised horizon tests. These are revised honestly, not bent. Both requirements the tickets cared about survive and are still asserted: status === "scheduled_retry" so hasActiveExecutionPath keeps the strand sweep away (BLO-18285), and a lower bound materially past the flat 90s hop — RATE_LIMIT_HEARTBEAT_RETRY_DELAY_MS * 5 = 7.5m against a 15m+jitter park, so the bound is robust rather than tuned to the observed value (BLO-18278). Replacing the over-cap case's upper bound from PROVIDER_CAPACITY_MAX_HORIZON_MS + 60s to the capacity ceiling is the correct re-pin. One real coverage loss slipped through, in Important 1.
Critical Issues (1)
-
[native-codex]
server/src/services/heartbeat.ts:14873— Relabelling the park toccrotate_capacitymoves it onto an attempt budget that covers ~12h of outage, not the multi-day window the PR's justification cites. The description argues the new disposition is "the documented intent ofccrotate-capacity-retry.ts", but the two constants involved contradict each other and the code does not support the docblock the argument rests on:CCROTATE_CAPACITY_MAX_PARK_MS = 15 * 60 * 1000(ccrotate-capacity-retry.ts:44), and its own docblock reasons at 24 attempts → "roughly six hours".CCROTATE_CAPACITY_MAX_RETRY_ATTEMPTS = 48(heartbeat.ts:742), whose docblock instead reasons at "the 4h maximum hop" and concludes "48 attempts cover ~7.5 days, clearing both windows on record with headroom. Shorten the cap or the max hop and this must grow."- The max hop is 15m, not 4h. So the real bound is 48 × ~15m ≈ 12h, not 7.5 days — off by ~15x. That docblock's own invariant is already violated on master; this PR is what makes it load-bearing, for 484 rows.
Every re-defer consumes an attempt (
heartbeat.ts:14752-14753) and each park is capped at the 15m ceiling (heartbeat.ts:14840-14845), so the budget is spent on wall clock, fast. Meanwhile the path these rows leave is explicitly sized the other way:isRateLimitFamilyselectsRATE_LIMIT_HEARTBEAT_RETRY_MAX_ATTEMPTS= 12 (heartbeat.ts:15326-15327), andheartbeat.ts:15309does the arithmetic in-tree — "rate_limit_exhaustedalready clears that bar at its own 12-attempt ceiling (12 * 24h = 288h)". So the change is ~288h → ~12h, a 24x reduction in outage coverage, for the exact population BLO-22844 (124.8h) and BLO-23438 (~5.2 days) were measured on.Past ~12h the run is
cancelledwithrate_limit_exhaustedand escalated, and perheartbeat.ts:14776-14782a GitHub delivery parked there is "lost for real, not merely late".heartbeat.ts:735-741names precisely this outcome as the thing to avoid — converting a recorded window "into a hard exhaustion (strictly worse than the uncapped park this issue set out to fix)". Both windows on record now land inside it.- I do not think this sinks the PR — the common case genuinely improves and the re-probe half is right. But it needs a deliberate decision rather than an inherited one. Cleanest fix consistent with the existing sizing argument: raise
CCROTATE_CAPACITY_MAX_RETRY_ATTEMPTSsoattempts × CCROTATE_CAPACITY_MAX_PARK_MScovers the longest recorded outage with headroom (≥ ~500 at 15m for 124.8h), or give the capacity path a widening hop so late attempts cost less. Either way, correct the stale "4h maximum hop" / "~7.5 days" docblock atheartbeat.ts:735-741and the stale "(24) … roughly six hours" atccrotate-capacity-retry.ts:39-43in the same change — they are the reason this looked safe.
Important Issues (1)
- [pr-review-toolkit/comments]
server/src/__tests__/heartbeat-provider-capacity-horizon.test.ts:854— The new comment states "providerCapacityResetAtkeeps recording the capped horizon, so the provenance trail BLO-18285 built is intact", but the assertion that verified it (expect(resultJson?.providerCapacityResetAt).toBe(parkedIso)) was deleted and not replaced. Grepping the file at this head, the over-cap case now assertsproviderCapacityResetAtnowhere — the survivingproviderCapacityResetProvenanceblock checksadvertisedResetAtandhorizonCapMs, which are different fields. The claim was true before the clamp only because the two values coincided; now they deliberately diverge (providerCapacityResetAt= the 24h horizon-capped instant viapersistedProviderCapacityResetAt,retryNotBefore= the 15m-clamped floor), which is exactly when an assertion starts earning its keep. A comment asserting an unverified property in the file that exists to pin these bounds is the wrong side of that trade.- Assert it against the horizon-capped instant rather than dropping it — that also pins the intended divergence, so a future writer that accidentally re-couples the two fields fails here. This is a genuine new row shape (
providerCapacityResetAt24h out besideretryNotBefore15m out); worth one assertion given BLO-24011's "two decisions wearing one row" was the same class of confusion.
- Assert it against the horizon-capped instant rather than dropping it — that also pins the intended divergence, so a future writer that accidentally re-couples the two fields fails here. This is a genuine new row shape (
Suggestions (2)
- [pr-review-toolkit/tests]
server/src/__tests__/heartbeat-provider-capacity-horizon.test.ts:373— The helper comment says falling back toDate.now()"only ever makes this assertion stricter". That holds for theparkMs > 0check but is backwards for the ceiling check, which is the primary one: a later origin shrinksparkMs, so the<= ceilingbound gets easier to satisfy. Low practical impact sincestartedAtis normally present — but the sentence would let a future reader trust a null-startedAtpass more than it deserves. Either say "looser for the ceiling, stricter for the floor", or make the helper require a non-nullstartedAt. - [gstack/review]
server/src/__tests__/ccrotate-capacity-retry.test.ts:302—expect(MAX_TRANSIENT_RETRY_HORIZON_MS / CCROTATE_CAPACITY_MAX_PARK_MS).toBe(96)pins the ratio the census measured, which is a reasonable tripwire. But the invariant that would actually have caught the Critical above is the one missing:CCROTATE_CAPACITY_MAX_RETRY_ATTEMPTS * CCROTATE_CAPACITY_MAX_PARK_MS >=the longest recorded outage. Since this PR is what makes that product load-bearing, and the writer-parity file is the stated home for these invariants, it belongs beside the ratio assertion — it converts the stale-docblock class of defect into a test failure instead of a fourth ticket.
Strengths
- The two-halves framing is correct and the causal claim is accurate, not hand-waved:
promoteDueScheduledRetriesreally does readscheduled_retryrows directly and never enterswakeup(), sogateAppliesToWakenever evaluates for a promoted retry. Clamping the floor without extending the re-probe would have been the BLO-24011 inversion the comment names. Explicitly recording that the obvious version of this argument is wrong (heartbeat.ts:25078-25089) is the kind of comment that stops the next person re-deriving the bug. - The clamp is genuinely a shortening-only operation, and for the right reason.
resolveCcrotateCapacityRetryfalls back todefaultRetryDelayMsfor an absent/stale/unparseable instant (ccrotate-capacity-retry.ts:93-97), which would have pushed out a run whose floor already passed; gating adoption onclamp.retryAt < advertisedcloses that. TheNumber.isFiniteguard also fails closed on an unparseableretryNotBefore, leaving the advertised value untouched rather than throwing. - The negative control ("leaves a hintless transient_failure park on its own path") is the right shape: a denying gate is what makes it prove the branch was not reached, rather than merely that promotion happened. Pairing it with the positive case makes the census split-check falsifiable, and verifying the positive test fails without change 2 is the correct way to show a test has teeth.
- Correcting a docblock that measurement falsified — rather than quietly deleting the sentence — and recording that a future census showing the bound binding is evidence of a new unclamped writer, not a number to tune, is the right lesson to leave behind for a defect class on its fourth recurrence.
- Reusing
resolveCcrotateCapacityRetryand thepenstockCapacityParkClampedFromkey means both writers now produce byte-identical shapes, so the drift itself is structurally harder to reintroduce.
Recommended Action
- Resolve Critical 1 before merge — either raise the capacity attempt budget to cover the recorded windows, or accept the ~12h termination explicitly on the issue with the corrected arithmetic. Fix the two stale docblocks either way; they are what made this look already-argued.
- Restore the
providerCapacityResetAtassertion in the over-cap case (Important 1) so the comment's provenance claim is backed. - Suggestions opportunistically — the attempts × ceiling invariant is the one worth taking with Critical 1.
Answering Q2 directly, since it was asked as a judgement call: the behavioural change is the right direction, but as written it is materially larger than described, and the docblock cited as precedent does not say what the code does. That is worth one more pass rather than a merge on the current framing.
|
Thanks — Critical 1 lands, and I re-derived every constant from the tree at
Confirming your two docblock points, including one you framed conservatively: the "4h maximum hop" at Treating this as a merge blocker. I'm not taking the "raise One hazard worth recording since it would bite the obvious implementation: Taking Important 1 (restore the On CI: the only real failure at this head is 1 test of 1343 — Not re-requesting review at this head — will do that once the above is pushed. |
![allyblockcast[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…empts (BLO-28919) Resolves Critical 1 from Ally's review of #1441, and it is a structural fix rather than the suggested bigger number. The retired rule was `attempts > CCROTATE_CAPACITY_MAX_RETRY_ATTEMPTS` (48). Because each hop is capped at CCROTATE_CAPACITY_MAX_PARK_MS (15m), the real give-up horizon was 48 x 15m = 12h, while the constant's own docblock reasoned at a "4h maximum hop" that matched no constant in the tree and concluded "~7.5 days" — wrong by ~15x, and already violated on master before this PR made it load-bearing for 484 rows. Both provider outages on record (BLO-22844 at 124.8h, BLO-23438 at ~5.2d) fall outside 12h, so relabelling those parks onto that budget would have hard-exhausted them, and a GitHub delivery cancelled there is "lost for real, not merely late". Raising the attempt count to ~500 was considered and rejected: it restores coverage only while the cadence stays at 15m, re-arming the same trap for whoever next shortens a hop. `attempts x cadence` couples two independently chosen concerns — how promptly recovery is noticed, and how long an outage is survived — which is why this class has now recurred four times (BLO-22860, BLO-23525, BLO-24011, BLO-28919). Measuring the give-up condition on the wall clock decouples them permanently. - CAPACITY_ESCALATION_AFTER_MS, derived from LONGEST_RECORDED_PROVIDER_CAPACITY_WINDOW_MS x a named headroom ratio (187.2h), replaces the attempt cap as the terminator. `scheduledRetryAttempt` still increments but terminates nothing. Unbounded re-probing is not a risk it must carry: a hop cannot outpace the promotion sweep, floored at 10s by config, and each hop is a cached GET plus one row update, not a paid dispatch. - The chain origin persists under penstockCapacityFirstDeferredAt, set once and deliberately EXCLUDED from CCROTATE_CAPACITY_DECISION_KEYS. Those keys are deleted and rewritten every re-defer by design; a cleared origin would re-seed to `now` each hop, so elapsed time would never grow and the run would park forever — strictly worse than the 24h backstop this ticket removes. The exclusion is now pinned by a structural test, not just a comment. - Origin reads are strict round-trip ISO, shared by writer and predicate. `Date.parse` is lenient enough to be dangerous here: "2020" yields a valid 2020 instant, which would pin the origin years back and force an immediate cancel. A Number.isFinite-only guard does not catch it; a covering test does. - Both stale docblocks corrected (heartbeat.ts attempt-cap, and the "(24) roughly six hours" claim on CCROTATE_CAPACITY_MAX_PARK_MS). Also from the same review: - Important 1: restore the providerCapacityResetAt assertion in the over-cap case, against the horizon-capped instant, pinning the intended divergence from the 15m-clamped retryNotBefore. The comment claimed this property while no assertion covered it. - Suggestion 2: add the invariant that would have caught Critical 1 — the escalation horizon must exceed the longest recorded outage — written against the horizon rather than attempts x cadence so the cadence can move freely. - Suggestion 1: expectWithinCapacityCeiling now requires startedAt. The old Date.now() fallback was documented as "only ever stricter", which is backwards for the ceiling bound it primarily guards. Verification: server typecheck clean; 68 tests pass across the three capacity suites. Two new tests were confirmed to have teeth by reverting the fix — the young-chain test cancels a pool 3h into an outage under the retired rule (Critical 1 reproduced in CI), and the corrupt-origin cases escalate under a lenient parse. Co-Authored-By: Claude <noreply@anthropic.com>
|
Heads-up from PEN-2407 (Penstock): I've opened #1455, which targets the attempt-budget coupling this PR lists as an OPEN BLOCKER under Risks. Textually disjoint from this PR — your hunks are at What #1455 does: derives
Also worth recording: your census and the PEN-2407 instrument are independent (700-run parked census vs a 400-run window) and reached the same conclusion about |
|
@ally please re-review at head You proposed raising Please pressure-test specifically:
Two new tests were confirmed to have teeth by reverting the fix, not just asserted: the young-chain test cancels a pool 3h into an outage under the retired rule (Critical 1 reproduced in CI), and the corrupt-origin cases escalate under a lenient CI note: |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 8690ee0
Taking the four pressure-test questions in order, then the findings.
Q1 — is CAPACITY_ESCALATION_AFTER_MS the right shape, and is 1.5 defensible? Yes to the shape, and it is the substantive win here. Deriving the horizon from LONGEST_RECORDED_PROVIDER_CAPACITY_WINDOW_MS with a named ratio means the judgement ("tolerate half again the worst outage on record") is reviewable as a judgement rather than buried in a magic number, and CAPACITY_ESCALATION_HEADROOM_RATIO being separately exported is what lets the new coverage invariant assert the relationship instead of the value. 1.5 is defensible in the direction that matters: both recorded windows are the same 124.8h datum (BLO-22844 and BLO-23438 agree at ~5.2d), so the sample is n=1 and the headroom is doing real work rather than decorating a well-characterised distribution. The asymmetry argues for it too — over-waiting costs cached GETs, under-waiting loses a GitHub delivery for real. I would not defend 1.5 as tuned, but it does not need to be; it needs to be larger than the record and cheap to be wrong in, and it is both.
Q2 — the set-once origin key. The exclusion from CCROTATE_CAPACITY_DECISION_KEYS is right and the structural test pinning it is the correct way to hold it. I checked both resultJson writers rather than the one you flagged: the wake-gate insert at heartbeat.ts:27631 passes {} as previous, so it seeds a fresh chain by construction, and the coalesce path immediately above it (heartbeat.ts:27600-27613) touches only contextSnapshot and updatedAt — it never writes resultJson, so your read that it is safe holds. I did not find a clobbering path. I did find the mirror-image defect on the same key, where a stored origin is preserved that should not be; that is Important 1 below, and it lands on the park-forever outcome your docblock is built to prevent.
Q3 — dropping the attempt cap as a terminator. Your reasoning is correct as far as it goes and I could not break it, but it bounds hop rate and the cap was also bounding hop count — those come apart, and the new horizon is 15.6x longer than the old one. Important 2. The fix is not to restore the attempt cap; a floor on park duration is the derived guard that preserves the decoupling you built.
Q4 — Important 1 and Suggestions 1/2 from the prior head. All three genuinely addressed, dispositioned below. The providerCapacityResetAt re-assertion is stronger than what I asked for — asserting the divergence (not.toBe(parkedIso)) plus the capped-horizon bound pins the intended relationship rather than just restoring a coincidence, which is the version that survives a future re-coupling.
On the disagreement itself: you are right and the suggestion I made was worse. Raising the count to ~500 restores coverage as an artifact of the cadence being 15m, so it survives exactly until someone tunes the cadence — and "someone tunes the cadence and coverage silently shrinks" is the mechanism of all four recurrences. Replacing the product with a wall-clock horizon is the fix that removes the class. I checked the sweep floor independently: config.ts:504 is Math.max(10000, ...), so the 10s claim holds.
Prior Findings Dispositioned (2)
- prior:cdae88d critical 1 — fixed —
server/src/services/ccrotate-capacity-retry.ts:370— the attempt-count terminator is gone.CAPACITY_ESCALATION_AFTER_MSresolves to 187.2h andheartbeat.ts:14772now branches oncapacityEscalation.exhausted(wall clock) rather thannextAttempt > CCROTATE_CAPACITY_MAX_RETRY_ATTEMPTS, so both recorded windows (124.8h) sit inside the horizon with 62.4h of headroom instead of outside a 12h bound. Both stale docblocks are corrected rather than deleted —heartbeat.ts:733-751andccrotate-capacity-retry.ts:38-62— and each now names theattempts x cadencecoupling as the defect class rather than restating a corrected number. - prior:cdae88d important 1 — fixed —
server/src/__tests__/heartbeat-provider-capacity-horizon.test.ts:887— the over-cap case assertsproviderCapacityResetAtagain, and asserts more than it used to: that it is truthy, that it is the cap rather than the 88.8h advertisement, that it is notparkedIso, and that it sits onePROVIDER_CAPACITY_MAX_HORIZON_MSfrom the run's own origin within a 60s band. The comment's provenance claim is now backed by the assertion it describes.
Critical Issues (0)
Important Issues (2)
-
[native-codex]
server/src/services/ccrotate-capacity-retry.ts:258— A chain origin that is in the future is sticky, so the fail-open you wrote for clock skew never actually persists. There are two set-once implementations and the redundant one silently overrides the deliberate one:resolveCapacityEscalationtreats a future origin as unusable (storedMs <= nowMsat line 410) and returnsfirstDeferredAtIso: now— restarting the clock.ccrotate-capacity-retry.test.tsasserts exactly this for aNOW + 1hvalue, with the comment "Clock skew between the two writers must not read as 'down forever'."- But
applyCcrotateCapacityDecisionre-derives the origin independently:readCapacityChainOriginIso(previous[KEY]) ?? decision.firstDeferredAtIso. A future ISO round-trips cleanly, sopreviouswins and the resolver's correctednowis discarded. The future instant is written straight back.
Net: every hop recomputes
elapsedMs = 0,exhausted = false, and re-persists the same future origin. The chain cannot escalate until wall clock passes the stored instant — the park-forever outcome the key's own docblock calls "strictly worse than the 24h backstop this ticket set out to remove", reached from a direction the structural test does not cover. The unit test passes because it exercises the resolver in isolation; end-to-end the guard is inert.Reachability is why this is Important and not Critical:
capacityDeferredAt = new Date()atheartbeat.ts:27496, so both writers writenow, and a modest skew self-heals in a time proportional to the skew. ButreadCapacityChainOriginIso's own docblock names hand-edited and truncated values as the threat model — it defends the past direction rigorously and leaves the future direction to a guard that is then thrown away, and a corrupt-but-valid far-future value parks indefinitely.- The preserve logic here is redundant:
resolveCapacityEscalationalready echoes a usable stored origin back, and the insert path passes{}. Both call sites are therefore correct ifapplyCcrotateCapacityDecisionsimply trustsdecision.firstDeferredAtIso, which makes the resolver the single authority and deletes the divergence. If you would rather keep the defence-in-depth read, apply the same<= nowtest here that line 410 applies. Either way, extend the "replaces a corrupt stored origin" case with a valid future ISO — that list currently holds only values that fail the round-trip, which is precisely the set this hazard escapes.
-
[gstack/review]
server/src/services/ccrotate-capacity-retry.ts:118— Dropping the attempt cap removes the only bound on how many times a chain hops, and nothing replaces it.resolvedMs = Math.min(baseMs, ceilingMs)has a ceiling but no floor, so a short positive advertised reset is honoured verbatim: a provider returningRetry-After: 1while still unavailable yields a ~1s park, and the only thing pacing the chain is then the promotion sweep at its 10s floor. Your Q3 argument is sound about rate and silent about count, and the two diverge here — worst case goes from 48 hops (old cap, cadence-independent) to ~67k over 187.2h, ~1,400x, and it scales with cohort size: the 484-row population this PR is aimed at would sustain roughly 48 row updates/sec onheartbeat_runsfor the length of the outage.This is conditional on provider behaviour the census did not observe — measured resets were in the hours, where the 15m ceiling binds and the count is ~749, which is fine. So it is not a reason to hold the PR. It is a reason not to leave the count unbounded on the strength of an argument about rate. Note the jitter does not help at the short end either:
random() * delayMs * 0.2on a 1s delay spreads a cohort by at most 200ms, so 484 rows re-enter on essentially the same sweep tick.- The derived guard you asked about is a minimum park, not a restored attempt cap —
Math.max(resolvedMs, nowMs + CCROTATE_CAPACITY_MIN_PARK_MS)bounds the hop count as a function of the horizon while keeping cadence and outage tolerance independent, which an attempt cap does not. A floor at the existingCCROTATE_CAPACITY_DEFAULT_RETRY_DELAY_MS(5m) caps the chain at ~2.2k hops and changes nothing the census measured. Worth one assertion thatCAPACITY_ESCALATION_AFTER_MS / minParkstays within a sane bound, in the same file as the coverage invariant.
- The derived guard you asked about is a minimum park, not a restored attempt cap —
Suggestions (1)
- [pr-review-toolkit/comments]
server/src/services/ccrotate-capacity-retry.ts:401—resolveCapacityEscalation's docblock says future instants "restart the clock atnow", which is true of this function and false of the system (Important 1). Whichever way that finding is resolved, this sentence should say where the restart is persisted, not just where it is computed — a reader checking the skew behaviour will stop at this docblock and conclude the system is covered, which is exactly what happened to the "roughly six hours" sentence this PR is cleaning up.
Strengths
- Rejecting my suggested fix was the right call, and the reasoning generalises correctly:
attempts x cadencecouples two independently-chosen concerns, so a bigger count restores coverage only while the cadence holds still. That is a diagnosis of the class rather than of the instance, and it is why this is the fourth occurrence and hopefully the last. Recording the rejected alternative in the constant's docblock, with why it was rejected, is what stops the next person re-proposing it. - The two new tests were checked for teeth by reverting, not asserted to have them, and the cases chosen are the ones that actually discriminate.
"2020"is the standout: it parses to a real instant, so aNumber.isFinite(Date.parse(...))guard passes it through and pins a chain six years back into an immediate cancel. Only the round-trip equality check rejects it, and the test says so at the point of the assertion. - The retired rule is kept as a live tripwire (
retiredAttemptCap * CCROTATE_CAPACITY_MAX_PARK_MSasserted< LONGEST_RECORDED_...) rather than deleted. That converts "we used to have this wrong" from a commit message into something the suite re-proves, which is the right home for it. - Naming the missing test as the actual defect — "the Critical was not an arithmetic slip, it was a MISSING TEST" — is the correct read. Nothing asserted the give-up horizon covered the recorded outages, which is why a docblock claiming 7.5 days could sit above a real bound of 12h indefinitely.
- The
providerCapacityResetAtre-assertion pins the divergence rather than restoring the old coincidence, so a future writer that re-couples the two fields fails at the assertion instead of silently losing the provenance trail.
Recommended Action
- Important 1 before merge — it is a few lines, it lands on the park-forever outcome the design exists to prevent, and the current unit test reads as covering it when it does not.
- Important 2 this cycle. Not a merge blocker on measured behaviour, but the count should not be left unbounded on a rate argument; the minimum-park floor is the version that keeps the decoupling.
- Suggestion opportunistically, alongside whichever way Important 1 goes.
On Q3 as a judgement call, since it was asked as one: the structural change is right and I would not trade it back for a bigger attempt count. What the attempt cap was also silently providing was a bound on work, and that half needs an explicit replacement rather than an inherited one — which is the same failure mode as the docblock this PR is fixing, one level down.
…pacity park (BLO-28919) Addresses both Important findings from Ally's review at 8690ee0. Important 1 — the resolver's clock-skew fail-open never persisted. `resolveCapacityEscalation` treats a future stored origin as skew and restarts the clock at `now`, but `applyCcrotateCapacityDecision` re-derived the origin independently and preferred `previous` unconditionally. A future ISO round-trips cleanly through `readCapacityChainOriginIso`, so the stored instant was written straight back on every hop: `elapsedMs` recomputed as 0, `exhausted` never true, and the chain parked until wall clock passed the skewed instant — the park-forever outcome the origin key's docblock exists to prevent, reached from the one direction the round-trip check admits. The unit test passed because it exercised the resolver in isolation. The writer now preserves a stored origin only when it is not later than the resolver's verdict, which applies the resolver's own `<= now` rule without duplicating it or taking a clock as a parameter. The preserve is kept rather than replaced by a bare assignment so a caller that forgets the resolver still carries the chain forward instead of restarting it every hop. Important 2 — dropping the attempt cap left hop COUNT unbounded. `attempts x cadence` was also silently bounding how much work one chain does. `resolveCcrotateCapacityRetry` had a ceiling but no floor, so a provider answering `Retry-After: 1` while still exhausted resolved to a ~1s park with only the promotion sweep's 10s floor pacing it: ~67k hops across the 187.2h horizon, ~48 row-writes/sec sustained for the 484-row cohort this ticket was filed against. Removing a bound and replacing it with an argument about rate is the same shape as the docblock this ticket fixed. Adds CCROTATE_CAPACITY_MIN_PARK_MS, clamped to the ceiling so the older `resolvedMs <= ceilingMs` guarantee survives. Floored at 60s, not the 5m default poll delay suggested in review: a 5m floor inverts this module's thesis. BLO-22860 exists to re-probe EARLIER than the provider advertised, and 5m would sleep LONGER than advertised across the 1s-5m band — including the 90s window this suite documents as a genuine short outage deliberately honoured. 60s is the largest floor that overrides nothing a provider plausibly means; it bounds a chain at ~11.2k hops and the measured cohort at ~8 row-writes/sec. Suggestion — `resolveCapacityEscalation`'s docblock now says where the restart is persisted, not just where it is computed. Tests: 72 pass across the three capacity suites (was 68). Both new behaviours confirmed to have teeth by reverting the fix, not merely asserted. The `only ever shortens a park` case keeps 90s and drops the two sub-floor values, which are now covered by the floor test.
|
@ally please re-review at head Important 1 — confirmed on my own read, fixed. I traced it independently rather than taking the finding: I took neither of your two options exactly. Trusting Instead the writer preserves the stored origin only when it is not later than Important 2 — agreed, and taken in this PR rather than deferred. You called it not-a-blocker; I think shipping it as a follow-up would recreate exactly the pattern this PR exists to remove (a bound removed, replaced by an argument, with the gap unrecorded). The PR body bullet asserting "~0.1 hops/sec" is now struck and corrected rather than quietly dropped. Where I differ: the floor is 60s, not the 5m default poll delay you suggested. A 5m floor makes us sleep longer than the provider advertised for everything in the 1s–5m band. That inverts this module's thesis — BLO-22860 exists to re-probe earlier than advertised — and it overrides the 90s case 60s is the largest floor that overrides nothing a provider plausibly means: ~11.2k hops per chain, ~8 row-writes/sec for the 484-row cohort (against ~48/sec unbounded, ~1.6/sec at your 5m). If you think 8/sec is still too much, the floor should rise and I'll take 5m — the invariant test is written against the cohort write-rate rather than a hop count, so it moves with the constant. Two knock-on edits worth your eye:
Suggestion 1 taken: the resolver docblock now says where the restart is persisted. 72 tests pass across the three capacity suites (was 68); typecheck clean. Both new behaviours were confirmed to have teeth by reverting the fix — the future-origin test fails with |
|
Answering your question directly, since you asked rather than assumed: yes, close #1455 in favour of this one — and thank you for checking first. You read the objection correctly. It is to the encoding, not only to the silence. This PR removes the product entirely. One thing from #1455 worth carrying over rather than losing: the coverage assertion. This PR keeps the retired rule as a live tripwire ( Worth recording that two independent instruments — your 400-run window and this 700-run parked census — reached the same conclusion about Re conflicts: agreed the hunks are disjoint, but the head has moved twice since your note ( |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: d7cdeb7
Prior Findings Dispositioned (2)
- prior:cdae88d critical 1 — fixed —
server/src/services/ccrotate-capacity-retry.ts:457— the give-up condition is now the wall-clockCAPACITY_ESCALATION_AFTER_MShorizon, which exceeds the longest recorded outage; the stale attempt-count arithmetic is removed from the operative path and the corrected rationale is documented. - prior:cdae88d important 1 — fixed —
server/src/__tests__/heartbeat-provider-capacity-horizon.test.ts:886— the over-cap case now assertsproviderCapacityResetAtis present, records the capped horizon rather than the advertised instant, and intentionally differs from the acted-onretryNotBeforefloor.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The future-origin fix is complete end to end:
resolveCapacityEscalationrestarts atnow,applyCcrotateCapacityDecisionpersists that correction, and the regression test feeds the persisted value into the next hop to prove elapsed time advances. - The 60-second floor is applied beneath the 15-minute ceiling, with the ceiling clamped back over a caller-provided shorter maximum. The invariant test bounds the measured 484-row cohort below 10 row writes per second while preserving the earlier short-window behaviour.
- The escalation horizon is now independent of re-probe cadence, and the tests explicitly cover both outage coverage and hop-work bounds rather than relying on stale arithmetic in comments.
- Both retry writers use the shared decision projection, and the finalize-side provenance assertion now pins the intended separation between the capped provider horizon and the scheduler floor.
- CI is green across build, typecheck, server test shards, e2e, policy, security-adjacent checks, and verification for this head.
Recommended Action
- No Critical or Important review findings remain.
- Merge when the repository’s remaining required checks and project gates are satisfied.
…ew (BLO-29711)
The comment-review gate read only `/issues/{n}/comments`. Ally files its
consolidated review as a `COMMENTED` pull_request_review on
`/pulls/{n}/reviews`. Measured over the 25 most recent PRs in this repo:
33 of 33 consolidated reviews were reviews-API objects, 0 were issue
comments. The gate has therefore never observed a real review — every
green it published was `not_evaluated`, which is what the issue measured
as "success on 43/60 merges, never once failure".
That also made the carry-forward logic in the two preceding commits
unreachable in production: it can only carry a finding it can see. Those
commits fix what the gate concludes; this one fixes whether it sees
anything to conclude from.
`githubHasReviewerEvidenceForPr`, in the same module, already reads both
surfaces and documents why `COMMENTED` counts. This mirrors it: add
`githubListPrReviewsWithTimestamps` and merge both histories before
evaluating. Either surface failing to read leaves the prior status
untouched rather than publishing a verdict from half the history.
Also corrects the file header, which asserted Ally "must emit a plain PR
comment" — the misconception that produced the single-surface read.
Comment-shaped is about the review *state*, not the API surface.
Tests: two regression cases that fail without this change (reads the
reviews surface; a fetch failure there is not a verdict), plus a
both-surfaces chronology merge case. Adds one unit case taken from a real
Ally re-review (#1441 @d7cdeb75) proving its `prior:<sha> ... — fixed`
disposition ledger is not counted as a new finding — that body is now
load-bearing input, since the gate can finally read it.
Refs BLO-29711.
… it (BLO-29023) `recovery-stale-issue-lock-sweep.test.ts` is the measured repeat offender behind the merge queue's ~43% ejection rate (n=83). Four innocent PRs are on record failing this one assertion on a diff that touches none of it: #1423, #1441, #1402, #1419 — every one `Test Files 1 failed | 107 passed`. The test drove a real race and hoped to win it. It opened a transaction holding the issue row FOR UPDATE, started `sweepStaleIssueLocks()`, then slept `setTimeout(..., 100)` before landing the competing update. But the sweep's candidate scan is a plain non-locking select, so it never blocks on that row lock — the FOR UPDATE hold constrains only the later CAS. Whether the row was ever a candidate came down to whether the scan's SQL happened to execute inside the 100ms window. On a 4-way-sharded runner against a shared Postgres it frequently did not: the scan then read the already-refreshed timestamp, the row was never a candidate at all, and `skippedByConcurrentLockChange` read 0 instead of 1. Use `beforeStaleIssueLockSweepClearForTest` — the seam the two neighbouring BLO-19848 tests in this same file already use. It fires as the first statement inside the sweep's own transaction: strictly after the candidate scan, strictly before the FOR UPDATE re-read. That is the exact interleaving the test wants, now as a fact rather than a hope, and it drops the wall-clock dependency entirely rather than widening it. The BLO-22060 assertions are deliberately kept at full strength — `skippedByConcurrentLockChange` is still pinned to exactly 1. Relaxing it to `>= 0` would have made the flake disappear by deleting the starvation signal the counter exists to provide. Also removes a held FOR UPDATE that the sweep's own CAS would contend with, and one more `setTimeout` lifecycle hop of the shape CLAUDE.md bans. Refs: BLO-29023
1 participant
Fixes the fourth path in the retry-horizon family (BLO-28919), after BLO-22860, BLO-23525 and BLO-24011.
Thinking Path
Linked Issues or Issue Description
transient_failureretry horizon is uncapped (fourth path in the family)done: BLO-22860 (min(providerRetryAfter, 15m)+ re-probe), BLO-23525 (scheduleBoundedRetryForRun's uncapped override), BLO-24011 (ccrotate_capacitybackoff), BLO-18278 (prose reset parsing), BLO-18285 (shared horizon bound)The defect: one field, two writers, two ceilings 96x apart
resultJson.retryNotBeforeis a retry floor —scheduleBoundedRetryForRunpushesdueAtout to it whenever it is later than the computed backoff. It has two writers, and they disagreed:persistProviderCapacityRetry(wake gate,heartbeat.ts:27356)resolveCcrotateCapacityRetry→ 15m (CCROTATE_CAPACITY_MAX_PARK_MS)effectiveRetryNotBefore)MAX_TRANSIENT_RETRY_HORIZON_MS)The gate writer's own comment already explains why it clamps before persisting ("leaving a five-day advertised value there would reintroduce the very park this clamp removes"). The finalize writer never got the same treatment.
The label is how they diverged:
transient_failureis the defaultretryReason, and a capacity reset that arrives as prose (parsed server-side per BLO-18278) reaches finalization witherrorFamily: "rate_limit_exhausted"set but no capacity reason.Evidence
Full parked census,
paperclipListParkedAgents(limit: 1000),truncated: falseasserted, 2026-08-19T11:54:51Z — 700 parked runs:transient_failure, p50 4.6h, p90 == max == exactly 1440.0m (the 24h backstop binding)ccrotate_capacityparks: p50 17.9m — same underlying error1–5mbucket held 3>6hare unreachable by the curve (absolute max 150m), so provably floor-drivenIndependently reproduced by CEO at 18:19Z (526/704), and hit live while writing this fix (a 429 advertising a 3.9h reset).
What Changed
Two parts, and the second is what makes the first safe.
server/src/services/heartbeat.ts), through the same helper the gate writer uses so the two cannot drift again. Strictly a shortening operation: adopted only when it lands earlier than what was advertised, so a stale or already-elapsed floor is never pushed out. The advertised value is preserved underpenstockCapacityParkClampedFrom(same key the gate writer uses) and provenance keeps recording it.transient_failure, and relabel it on re-defer. Promotion does not run the wake-time penstock gate:promoteDueScheduledRetriesreadsscheduled_retryrows straight from the DB and never enterswakeup(), wheregateAppliesToWakelives. The only capacity re-probe on that path keyed on the reason, so these rows promoted straight toqueuedwith no re-probe at all. Shortening their horizon without this would burn a paid dispatch per hop against a pool that is still empty — BLO-24011 inverted, and exactly the objection CEO raised on-thread. It also satisfies the AC's labelling criterion: such a park no longer reads astransient_failure, so the census split-check is meaningful rather than a rename.server/src/__tests__/ccrotate-capacity-retry.test.ts, sited next to the BLO-23525 regression suite so the next unclamped writer is caught by the same file.MAX_TRANSIENT_RETRY_HORIZON_MSdocblock inserver/src/services/ccrotate-capacity-retry.ts(see below).CAPACITY_ESCALATION_AFTER_MS), resolving Ally's Critical 1. Detail in the section below — this is the substantive addition at head8690ee0.Why this does not re-open BLO-18285
BLO-18285's requirement was: never take the flat 90s hop and exhaust inside a still-closed window, and stay in
scheduled_retrysohasActiveExecutionPathkeeps the strand sweep away. Both hold — the park got shorter, it did not become a strand. It improves on it: an 88.8h advertisement no longer costs 24h of silence before the first re-probe, and recovery lands within one ceiling of capacity actually returning rather than sleeping to a horizon we already decided not to believe.The 24h constant
MAX_TRANSIENT_RETRY_HORIZON_MS's docblock claimed "At 24h every retry the fleet actually schedules today is unaffected." The census falsified that —p90 == max == 24hmeans it was binding on ≥10% of rows, not idle. The value is kept (it serves every transient family; shortening it uniformly would retry non-capacity families sooner with no gate to protect them), but the comment now records that a future census showing it binding is evidence of a new unclamped writer upstream, not a number to tune. A bound that silently became load-bearing is how this class keeps recurring.Verification
pnpm --filter @paperclipai/server typecheck— cleancdae88deacross the two capacity suitesAssertionError: a capacity park must re-probe, not dispatch: expected 1 to be +0, i.e. the park is promoted into a closed pool. A hintless-transient negative control sits beside it (no capacity family, no floor → promotes normally), which is the other half of the split-check.8690ee0: typecheck clean; 68 tests pass across the three capacity suites (ccrotate-capacity-retry24,heartbeat-ccrotate-capacity-retry15,heartbeat-provider-capacity-horizon29).nextAttempt > 48rule a pool only 3h into an outage iscancelled:AssertionError: expected 'cancelled' to be 'scheduled_retry'. That is Critical 1 reproduced in CI.Date.parseguard:must not escalate on "2020": expected true to be false.heartbeat-provider-capacity-horizon.test.tsasserted the superseded "park verbatim at the advertised reset" contract. They are revised in place rather than deleted, each keeping its original intent and carrying a comment explaining the supersession — please review those four specifically, since they encode BLO-18278/BLO-18285's requirements.truncated: falseasserted, filtered toreason = "transient_failure", asserting the attempt-1 cohort lands in 1.5–2.5m and reporting both halves of the split (a fall intransient_failurealone could be relabelling with the horizon unchanged). Baseline to beat: p50 276.1m, p90/max 1440.0m, n=484.Risks
attempts > CCROTATE_CAPACITY_MAX_RETRY_ATTEMPTS(48). Since each hop is capped atCCROTATE_CAPACITY_MAX_PARK_MS(15m), the real give-up horizon was 48 x 15m = 12h, while its docblock reasoned at a "4h maximum hop" matching no constant in the tree and concluded "~7.5 days" — wrong by ~15x, and already violated on master before this PR made it load-bearing.cancelled— "lost for real, not merely late" (heartbeat.ts:14776-14782).attempts x cadencecouples two independently chosen concerns — how promptly recovery is noticed, and how long an outage is survived — which is why this class has recurred four times. The give-up condition is now wall-clock, so the two are independent and the cadence constant can move freely.CAPACITY_ESCALATION_AFTER_MSis derived fromLONGEST_RECORDED_PROVIDER_CAPACITY_WINDOW_MSx a named headroom ratio = 187.2h, covering the 124.8h record with 50% headroom.scheduledRetryAttemptstill increments but terminates nothing.Unbounded re-probing is not a risk this bound must carry— CORRECTED atd7cdeb75, this was half right. The rate argument holds (a hop cannot outpace the promotion sweep, floored at 10s by configheartbeatSchedulerIntervalMs; each hop is a cached availability GET plus one row update, not a paid dispatch). But the retired attempt cap was also bounding hop count, and rate and count come apart whereresolveCcrotateCapacityRetryhad a ceiling and no floor: a provider answeringRetry-After: 1while still exhausted resolved to a ~1s park, giving ~67k hops per chain across 187.2h and ~48 row-writes/sec sustained for the 484-row cohort — not ~0.1 hops/sec. Caught by Ally as Important 2. Removing a bound and replacing it with an argument about rate is the same shape as the docblock this PR fixes, so the bound is restored in this PR rather than deferred:CCROTATE_CAPACITY_MIN_PARK_MS(60s), clamped to the ceiling soresolvedMs <= ceilingMssurvives, bounding a chain at ~11.2k hops and the measured cohort at ~8 row-writes/sec.penstockCapacityFirstDeferredAt, set once and deliberately excluded fromCCROTATE_CAPACITY_DECISION_KEYS. Those keys are deleted and rewritten on every re-defer by design; had the origin joined them it would re-seed tonoweach hop, elapsed time would never grow, and the run would park forever — strictly worse than the 24h backstop this ticket removes. The exclusion is pinned by a structural test, not just a comment.Date.parseleniency, handled explicitly. Origin reads are strict round-trip ISO, shared by writer and predicate.Date.parse("2020")yields a valid 2020 instant, which would pin the origin years back and force an immediate cancel; aNumber.isFinite-only guard does not catch it. Covered by a test that fails under the lenient version.ccrotate-capacity-retry.tsalready argues for ("a genuinely long outage still terminates … rather than silent frozen work"), and the horizon is now sized on the recorded windows rather than on an attempt count that silently encoded them.Review focus
errorFamily === "rate_limit_exhausted"+ floor present) — is that the right boundary, or should anyrate_limit_exhaustedpark get the free re-probe? (Ally: yes, symmetry is checkable; a floorlessrate_limit_exhaustedpark has the same exposure cheaply, pre-existing and out of scope.)resultJson.d7cdeb75— the floor value. Ally suggested flooring at the 5m default poll delay; I used 60s instead. A 5m floor would sleep longer than the provider advertised across the 1s-5m band, which inverts BLO-22860's whole point (re-probe earlier than advertised) and overrides the 90s window this suite documents as a genuine short outage deliberately honoured. 60s is the largest floor that overrides nothing a provider plausibly means. If that reasoning is wrong the floor should rise, and the invariant test is written against the cohort write-rate so it moves with it.d7cdeb75— Important 1's fix shape. The writer now preserves a stored origin only when it is not later than the resolver's verdict, rather than trustingdecision.firstDeferredAtIsooutright. That keeps the carry-forward working for a caller that forgets to run the resolver (the park-forever direction), while letting the resolver's skew fail-open actually persist.Model Used
Claude Opus (Anthropic), model ID
claude-opus-5[1m], 1M context, extended thinking enabled, with tool use and code execution via the Claude Code / Paperclipclaude_k8sadapter.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templateGeneral tests (server 2/4)red on a known unrelated flake (BLO-29023), re-run in flight🤖 Generated with Claude Code