feat(heartbeat): escalate ccrotate capacity exhaustion as a coalesced issue (PEN-382) - #307
Conversation
… issue (PEN-382) When a ccrotate-capacity scheduled retry exhausts its 24-attempt budget, the run is cancelled with a warn event — but that is easy to miss. File an operator-visible recovery issue (priority high) via recoveryService so a pool that never recovers gets attention. Coalesced per (company, ccrotate target): a pool outage exhausts many agents' retries at once, so the escalation is keyed on the target — one open issue per pool, not one per cancelled run. Best-effort: escalation failure never breaks the promotion sweep. Tests: assert the exhaustion path files exactly one escalation (origin ccrotate_capacity_exhausted, keyed on target, high/todo) and that a second agent exhausting the same pool coalesces onto it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + gstack/codex.
Clean, focused follow-up to #299 (+141/-0, 4 files). The exhaustion path, scope, and best-effort wrapping all check out — one substantive finding about the coalescing guarantee.
Critical Issues (0)
None.
Important Issues (1)
- [gstack/review · code]
recovery/service.ts(escalateCcrotateCapacityExhausted, the dedupSELECT … LIMIT 1~:4416) — the coalescing this PR sells is not enforced at the DB level, unlike every sibling escalation. The dedup is a bare check-then-create (SELECTopen issue by(companyId, originKind, originId)→ if none,issuesSvc.create). All five existing recovery origins back that same pattern with a dedicated partial unique index (issues_active_liveness_recovery_incident_uq,…_stale_run_evaluation_uq,…_productivity_review_uq,…_stranded_issue_recovery_uq,issues_open_routine_execution_uq), andrecovery/service.ts:~2255documents why: "The active-recovery uniqueness index prevents concurrent duplicates." This PR adds theccrotate_capacity_exhaustedorigin kind but no matching migration/index (the 4 changed files are test/heartbeat/origins/service only — confirmed against the schema at this SHA).- Within one
promoteDueScheduledRetriescall the loop is sequential, so the test's two-call coalescing passes. But the sweep is not globally single-threaded — two overlapping sweeps (multi-replica / re-entrant timer) each win the conditionalUPDATEfor a different exhausted run targeting the same pool, then bothSELECT(neither issue exists yet) and bothINSERT→ duplicate "ccrotate pool exhausted — " issues. This is most likely under exactly the scenario the PR is built for: a real outage exhausting many agents' retries at once. - Bounded and low-frequency (a couple of extra issues during a genuinely rare exhaustion, not a storm) — hence Important, not Critical.
- Fix: add a migration mirroring
issues_active_stranded_issue_recovery_uqfororigin_kind = 'ccrotate_capacity_exhausted'(WHERE origin_id IS NOT NULL AND hidden_at IS NULL AND status NOT IN ('done','cancelled')). It composes cleanly with the existing best-efforttry/catchinheartbeat.ts:~6418— a concurrent unique-violation becomes the caught/logged/swallowed case, so no second issue and no sweep breakage.
- Within one
Suggestions (2)
- [tests]
__tests__/heartbeat-ccrotate-capacity-retry.test.ts(thecoalesces …test, ~:309) — coverage only exercises the sequential path (two separatepromoteDueScheduledRetriescalls), which is precisely the path the missing index doesn't affect. It gives false confidence that coalescing holds under the concurrent mass-exhaustion the feature targets. Consider asserting the dedup at the DB level (or noting the concurrency gap) once the unique index lands. - [code · comments]
recovery/service.ts(issue body builder insideescalateCcrotateCapacityExhausted, ~:4440) — the escalation issue has no auto-resolution: it's createdtodo/unassigned and the description tells the operator to "mark it done once the pool recovers." If nobody closes it, a later genuine outage coalesces onto the stale-but-open issue, attaching a new incident to an old one's history. Defensible for v1 ("represents the whole outage"), but worth a follow-up to auto-close on pool recovery, or a runbook line so operators reliably close it.
Strengths
- Exhaustion path is correctly targeted: escalation fires only when
exhaustedis non-null, i.e. the promoter that actually flipped this run tocancelledvia the guarded conditionalUPDATE(status = 'scheduled_retry' AND scheduledRetryAt <= now) — so it's keyed to one real cancel, not every sweep pass. - Best-effort
try/catcharound the escalation (heartbeat.ts:~6418) never breaks the promotion sweep; the run is already cancelled and thewarnlifecycle event recorded before escalation runs. Right ordering, right failure mode. - Dedup predicate (
isNull(hiddenAt)+notInArray(status, ['done','cancelled'])+ originKind + originId) matches the established house pattern exactly; all SQL is parameterized drizzle (no injection surface, no LLM trust boundary). - Coalescing keyed on
(company, ccrotate target)is the right grain for a pool outage; unassigned +todokeeps it out of the assigned-issue dispatch/stranded sweeps, so it won't be mistaken for agent work.
Recommended Action
- Add the partial unique index migration for
ccrotate_capacity_exhaustedbefore merge — it's the one piece bringing this in line with the other five escalations and making the coalescing guarantee real. - Suggestions are opportunistic.
… review #307) Addresses the Important finding on #307: the coalescing was check-then-create only, not DB-enforced like the five sibling recovery escalations, so concurrent sweeps (multi-replica / re-entrant timer) during a real mass-exhaustion could each miss the SELECT and both INSERT → duplicate 'ccrotate pool exhausted' issues. - migration 0106 + schema: partial unique index issues_active_ccrotate_capacity_exhaustion_uq on (company_id, origin_kind, origin_id) WHERE origin_kind='ccrotate_capacity_exhausted' AND origin_id is not null AND hidden_at is null AND status not in ('done','cancelled') — mirrors issues_active_liveness_recovery_incident_uq exactly. Journaled (like every real index migration, e.g. 0069) so it applies via migratePg too. - service.ts: the create now catches the 23505 unique-violation and coalesces onto the winner (the best-effort try/catch in heartbeat.ts already swallows it → no second issue, no sweep breakage). - test: assert the index rejects a concurrent open duplicate (23505 + constraint) and allows a done duplicate (partial index excludes terminal). Suggestion 2 (auto-close stale escalations on pool recovery) deferred to a follow-up per the review (Ally: 'defensible for v1'). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @ally — addressed the Important finding in be76eb9. Important #1 (DB-level coalescing): added migration Suggestion 1 (DB-level test): added a test asserting the index rejects a concurrent open duplicate ( Suggestion 2 (auto-close stale escalation): agreed it's defensible for v1; deferring to a follow-up to auto-close on pool recovery (or a runbook line). Tracking separately. 7/7 ccrotate suite green + server typecheck clean locally. |
What
Follow-up to #299. When a ccrotate-capacity scheduled retry exhausts its 24-attempt budget (
CCROTATE_CAPACITY_MAX_RETRY_ATTEMPTS), the run is cancelled with awarnlifecycle event — but that's easy to miss in run history. This files an operator-visible recovery issue (priorityhigh,todo) viarecoveryServiceso a pool that never recovers actually gets attention.Design
originKind=ccrotate_capacity_exhausted,originId=<target>) — one open issue per pool, not one per cancelled run. Re-uses the established(originKind, originId)open-issue dedup pattern (mirrors the liveness escalations).Why now
error_code='rate_limit_exhausted'fired ~314× in the last 7 days (pre-#299, the immediate-cancel rate). Post-#299 most of those become silent auto-retries that recover; only genuine multi-hour pool starvation now reaches the 24-attempt cancel — making true exhaustion a rare, low-noise escalation trigger.Files
recovery/origins.ts— newccrotateCapacityExhaustedorigin kindrecovery/service.ts—escalateCcrotateCapacityExhausted()(dedup + create)heartbeat.ts— call it on the exhaustion path__tests__/heartbeat-ccrotate-capacity-retry.test.ts— assert one escalation on exhaustion + coalescing across two agents on the same poolTests
pnpm exec vitest run src/__tests__/heartbeat-ccrotate-capacity-retry.test.ts→ 6 passed (4 existing + exhaustion-files-escalation + coalescing). Server typecheck clean.🤖 Generated with Claude Code