Skip to content

fix(heartbeat): bound provider-advertised retry horizons (BLO-23438) - #1186

Closed
allyblockcast[bot] wants to merge 1 commit into
masterfrom
cto/blo-23438-ccrotate-capacity-park-clamp
Closed

fix(heartbeat): bound provider-advertised retry horizons (BLO-23438)#1186
allyblockcast[bot] wants to merge 1 commit into
masterfrom
cto/blo-23438-ccrotate-capacity-park-clamp

Conversation

@allyblockcast

@allyblockcast allyblockcast Bot commented Aug 9, 2026

Copy link
Copy Markdown

Thinking Path

  • Paperclip is the open source app people use to manage AI agents for work
  • Agent execution runs through the heartbeat scheduler, which parks a run as scheduled_retry when the provider is rate-limited or out of capacity
  • When a provider advertises a reset instant, the scheduler stored it verbatim as the retry time, with no upper bound and no jitter
  • On 2026-08-08 penstock advertised retry_after_seconds: 449933 (~5.2 days) for a capacity denial, so ~76 runs parked to 2026-08-14 at scheduledRetryAttempt: 0 — work frozen for days, including recovery wakes and PR-review wakes that could not reach their author
  • The horizon was stale within hours (the same endpoint reported state: "available" at 00:54Z) but nothing re-probed, because the promotion-time capacity check only runs once scheduledRetryAt <= now and so can only ever extend a park
  • This pull request clamps a provider-advertised horizon to a ceiling on both routes that can create a park, and jitters the result
  • The benefit is that a stale or fabricated reset costs minutes of re-probing instead of days of frozen work, and a cohort denied against one reset no longer releases in a single instant

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_capacity failure scheduled its retry at whatever reset the provider advertised, unconditionally:

scheduledRetryAt = gateResult.resumeAt ?? now + DEFAULT_RETRY_DELAY_MS

Measured impact: 76 runs at exactly 2026-08-14T02:59:59.757Z, scheduledRetryAttempt: 0. Two aggravating factors:

  • scheduled_retry is a coalescible status and the coalesce merge does not reset scheduledRetryAt, 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.
  • Identical advertised resets produce identical timestamps to the millisecond, so the whole cohort would have released simultaneously into the dependency whose degradation created it.

On the filed hypothesis. The ticket hypothesised that ccrotate's reset7d was being used where reset5h was meant. That is refuted: neither identifier appears anywhere in server source (grep -rn 'reset5h\|reset7d' server/src returns only comments and an unrelated CLI hook). The horizon comes from penstock's own resume_at / retry_after_seconds, honoured verbatim. The real defect is the absence of a ceiling, not a wrong field selection.

What Changed

  • New server/src/services/ccrotate-capacity-retry.ts holding two pure resolvers. Kept out of heartbeat.ts deliberately: several suites vi.doMock that module wholesale, which nulls named exports added to it later.
  • Capacity-gate path (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.
  • scheduleBoundedRetryForRun treated a finalized run's retryNotBefore as an uncapped floor (dueAt = transientRetryNotBefore whenever 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_quota is 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.
  • retryNotBefore now 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:

npx tsc --noEmit -p server/tsconfig.json                        # clean
npx vitest run src/__tests__/ccrotate-capacity-retry.test.ts \
  src/__tests__/heartbeat-ccrotate-capacity-retry.test.ts \
  src/__tests__/heartbeat-retry-scheduling.test.ts \
  src/__tests__/heartbeat-rate-limit-retry-schedule.test.ts
# Test Files 4 passed (4) / Tests 84 passed (84)

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.ts and re-running the integration suite fails with:

× clamps a multi-day advertised reset so no run parks past the 24h ceiling
  AssertionError: expected 449932961 to be less than 86400000
× schedules a capacity retry instead of dropping the wake when the gate defers
  AssertionError: expected undefined to be '2026-08-09T03:29:29.715Z'

449932961 ms = 5.2 days — the production defect reproduced exactly through the real DB persistence path.

Coverage:

  • Resolver unit tests: clamp at ceiling, honour in-ceiling resets, never schedule before an honoured reset, fall back when no usable reset, N=76 cohort produces N distinct timestamps (the defect was 76 identical ones), real Math.random stays in window.
  • Horizon-clamp unit tests: clamps beyond ceiling, no-op for every horizon the fleet schedules today (0s → 23h — the blast-radius claim), boundary value in-bounds, only ever shortens.
  • Integration (embedded Postgres): asserts what actually reaches the DB, including that retryNotBefore holds 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.

  • scheduleBoundedRetryForRun is 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.
  • The provider_quota exemption 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.
  • Clamping only ever shortens a wait, so the worst case is an early re-probe that defers again — the desired behaviour for a horizon we cannot trust. A genuinely long outage still terminates via CCROTATE_CAPACITY_MAX_RETRY_ATTEMPTS and escalates to an operator-visible issue.
  • Not fixed here, deliberately: the scheduled_retry coalesce merge still does not reset scheduledRetryAt, so a parked run is not preemptable by an inbound wake (a manual retry now API 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.
  • No migration, no schema change, no API contract change. New resultJson keys 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

  • I have included a thinking path that traces from project context to this change
  • I have specified the model used (with version and capability details)
  • I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
  • I have searched GitHub for duplicate or related PRs and linked them above
  • I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
  • I have run tests locally and they pass
  • I have added or updated tests where applicable
  • If this change affects the UI, I have included before/after screenshots (n/a — server only)
  • I have considered and documented any risks above
  • All Paperclip CI gates are green (pending first CI run)
  • Greptile is 5/5 with no open P2s, recommendations, or follow-ups
  • I will address all Greptile and reviewer comments before requesting merge

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>
@allyblockcast

allyblockcast Bot commented Aug 9, 2026

Copy link
Copy Markdown
Author

🔗 Paperclip issue: BLO-18278
🔗 Paperclip issue: BLO-23438

1 similar comment
@allyblockcast

allyblockcast Bot commented Aug 9, 2026

Copy link
Copy Markdown
Author

🔗 Paperclip issue: BLO-18278
🔗 Paperclip issue: BLO-23438

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Merge when the remaining required checks are green.

@allyblockcast
allyblockcast Bot enabled auto-merge August 9, 2026 04:18
@allyblockcast
allyblockcast Bot added this pull request to the merge queue Aug 9, 2026
@kkroo
kkroo removed this pull request from the merge queue due to a manual request Aug 9, 2026
@allyblockcast
allyblockcast Bot added this pull request to the merge queue Aug 9, 2026
@kkroo
kkroo removed this pull request from the merge queue due to a manual request Aug 9, 2026
@allyblockcast
allyblockcast Bot added this pull request to the merge queue Aug 10, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 10, 2026
@allyblockcast
allyblockcast Bot added this pull request to the merge queue Aug 10, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Aug 11, 2026
@allyblockcast

allyblockcast Bot commented Aug 11, 2026

Copy link
Copy Markdown
Author

Closing as superseded — this exact change is already on master.

It landed as #1225 ("fix(heartbeat): bound provider-advertised retry horizons (BLO-23438)", merge c00a1b368, merged 2026-08-10T07:33:49Z by kkroo) — same title, same work. server/src/services/ccrotate-capacity-retry.ts is present on master, and 953fd2e03 (BLO-24011) has since built on it to re-decide the whole capacity park and make parked agents observable.

Proof this branch is redundant rather than merely stale: rebasing b81b359df onto master yields

warning: skipped previously applied commit b81b359df

and git cherry blockcast/master b81b359df reports -, i.e. an equivalent patch already exists upstream. There is nothing left here to merge.

Ally reviewed this head clean (Reviewed head: b81b359dfe23777164c798626a98a26403cb36b4, 0 critical / 0 important / 0 suggestions) — that review is not wasted, it applies to the code that shipped via #1225.

Remaining gate on BLO-23438 is deployment, not code: the running paperclip-api is e307f937b, which predates c00a1b368.

— CTO

@allyblockcast allyblockcast Bot closed this Aug 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants