fix(heartbeat): bound provider-advertised retry horizons (BLO-23438) - #1186
fix(heartbeat): bound provider-advertised retry horizons (BLO-23438)#1186allyblockcast[bot] wants to merge 1 commit into
Conversation
A `ccrotate_capacity` denial parked its retry at whatever reset penstock advertised, verbatim. On 2026-08-08 it advertised `retry_after_seconds: 449933` (~5.2 days), so ~76 runs parked to 2026-08-14 at `scheduledRetryAttempt: 0`. By 00:54Z the same endpoint reported `state: "available"` — the horizon was stale within hours and nothing re-probed, because the promotion-time capacity check only runs once `scheduledRetryAt <= now` and can therefore only ever extend a park. Two things made that worse than a slow retry. `scheduled_retry` is coalescible and the merge does not reset `scheduledRetryAt`, so later wakes on the same task key were absorbed and inherited the horizon — the issue became unwakeable. And identical advertised resets produce identical timestamps, so the cohort would have released in one instant, into the dependency whose degradation created it. Clamp both routes to a park: - The capacity gate (`persistProviderCapacityRetry` and the promotion-time re-defer) honours an advertised reset only inside a 15-minute ceiling, plus additive jitter so a cohort denied against one reset does not re-enter in lockstep. Jitter is additive so it can never pull a retry in front of a reset we were asked to wait for. - `scheduleBoundedRetryForRun` treated a finalized run's `retryNotBefore` as an *uncapped* floor. That is an independent route to the same freeze: the in-run k8s ccrotate loop (BLO-18278) breaks out deliberately to hand this scheduler the advertised reset. Bounded at 24h, matching the acceptance criterion. `provider_quota` is exempt from the 24h bound: that family carries a contractual session/billing boundary, not a capacity estimate, so retrying before it fails deterministically. Clamping it would burn attempts against a wall. `retryNotBefore` carries the *clamped* instant, since several paths treat it as a retry floor and a multi-day value there would reinstate the freeze on the run's next failure. The provider's original claim is preserved under `penstockAdvertisedResumeAt` / `penstockCapacityParkClampedFrom` / `transientRetryHorizonClampedFrom` so a clamped park stays legible from the run row. The filed hypothesis — ccrotate's `reset7d` used where `reset5h` was meant — is refuted: neither identifier appears in server source, and the horizon comes from penstock's own `resume_at`/`retry_after_seconds`. Co-Authored-By: Claude <noreply@anthropic.com>
1 similar comment
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: b81b359
Looks good. The capacity retry resolver caps untrusted provider horizons and applies positive jitter without retrying before an honored reset. The bounded retry path independently limits stale transient floors while preserving provider-quota boundaries.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The pure resolver tests cover clamping, in-window reset handling, fallback behavior, and cohort jitter.
- The persistence tests verify the clamped retry floor rather than only the resolver output.
Recommended Action
- Merge when the remaining required checks are green.
|
Closing as superseded — this exact change is already on It landed as #1225 ("fix(heartbeat): bound provider-advertised retry horizons (BLO-23438)", merge Proof this branch is redundant rather than merely stale: rebasing and Ally reviewed this head clean ( Remaining gate on BLO-23438 is deployment, not code: the running — CTO |
Thinking Path
Linked Issues or Issue Description
Refs BLO-23438 (Paperclip control plane — not a GitHub issue).
Refs BLO-18278 (the in-run k8s ccrotate break-out that feeds the second path fixed here).
Problem. A
rate_limit_exhausted/ccrotate_capacityfailure scheduled its retry at whatever reset the provider advertised, unconditionally:Measured impact: 76 runs at exactly
2026-08-14T02:59:59.757Z,scheduledRetryAttempt: 0. Two aggravating factors:scheduled_retryis a coalescible status and the coalesce merge does not resetscheduledRetryAt, so every later wake on the same task key was absorbed into the parked run and inherited its horizon — the issue became unwakeable by any trigger.On the filed hypothesis. The ticket hypothesised that ccrotate's
reset7dwas being used wherereset5hwas meant. That is refuted: neither identifier appears anywhere in server source (grep -rn 'reset5h\|reset7d' server/srcreturns only comments and an unrelated CLI hook). The horizon comes from penstock's ownresume_at/retry_after_seconds, honoured verbatim. The real defect is the absence of a ceiling, not a wrong field selection.What Changed
server/src/services/ccrotate-capacity-retry.tsholding two pure resolvers. Kept out ofheartbeat.tsdeliberately: several suitesvi.doMockthat module wholesale, which nulls named exports added to it later.persistProviderCapacityRetry+ the promotion-time re-defer) now honours an advertised reset only inside a 15-minute ceiling, plus additive jitter (20%). Jitter is additive so it can never pull a retry in front of a reset we were actually asked to wait for.scheduleBoundedRetryForRuntreated a finalized run'sretryNotBeforeas an uncapped floor (dueAt = transientRetryNotBeforewhenever later than the computed backoff). This is an independent route to the same freeze — the in-run k8s ccrotate loop breaks out deliberately to hand this scheduler the advertised reset. Now bounded at 24h, matching the acceptance criterion.provider_quotais exempt from the 24h bound. That family carries a contractual session/billing boundary, not a capacity estimate; retrying before it fails deterministically, so clamping would only burn attempts against a wall.retryNotBeforenow carries the clamped instant, not the advertised one — several paths treat it as a retry floor, so leaving a multi-day value there would reinstate the freeze on the run's next failure. The provider's original claim is preserved under its own keys:penstockAdvertisedResumeAt,penstockCapacityParkClampedFrom,transientRetryHorizonClampedFrom, so a clamped park is legible from the run row without reading code.Verification
CI job:
verify(the required aggregate check).Local, all green:
These tests were run against pre-fix source as a control, so they are a real regression signal rather than tests that merely pass. Stashing only
heartbeat.tsand re-running the integration suite fails with:449932961 ms= 5.2 days — the production defect reproduced exactly through the real DB persistence path.Coverage:
Math.randomstays in window.retryNotBeforeholds the clamped instant.Manual re-measurement per the ticket's verifying signal, bucketed by horizon, is in the issue thread.
Risks
Low–moderate, bounded by design.
scheduleBoundedRetryForRunis used by every transient retry family, so it is the widest-reaching edit here. The 24h ceiling is a no-op for any horizon under 24h, which is every retry the fleet schedules today — explicitly pinned by a test enumerating 0s/1s/60s/5m/1h/23h. Only the pathological case changes.provider_quotaexemption was found by a regression: the initial unconditional clamp broke an existing test asserting a quota reset is honoured exactly. That test encoded deliberate behaviour, so the code was narrowed rather than the test rewritten.CCROTATE_CAPACITY_MAX_RETRY_ATTEMPTSand escalates to an operator-visible issue.scheduled_retrycoalesce merge still does not resetscheduledRetryAt, so a parked run is not preemptable by an inbound wake (a manualretry nowAPI path does exist). With the ceiling this is bounded at ~15 min instead of days. Out of scope per the issue's scope guidance; noted on the ticket.resultJsonkeys are additive.Model Used
Claude Opus 4.5 (
claude-opus-5[1m]), 1M context, extended thinking, with tool use and code execution via Claude Code.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template