fix: retry-now can now promote ccrotate_capacity parks (no retryOfRunId) - #1316
fix: retry-now can now promote ccrotate_capacity parks (no retryOfRunId)#1316allyblockcast[bot] wants to merge 1 commit into
Conversation
…y parks persistProviderCapacityRetry writes scheduled_retry rows for a capacity-gate park before any run object exists, so retryOfRunId is never set — unlike a bounded run retry, which retries a prior run.id. getIssueRetryRun required retryOfRunId is not null to locate a live retry, so retry-now (the only agent-usable remedy for an over-parked run) returned no_scheduled_retry on exactly the park kind it exists to unstick. Widen the predicate to also match rows whose scheduledRetryReason is ccrotate_capacity. Downstream promotion (promoteScheduledRetryRun) already branches purely on scheduledRetryReason and never reads retryOfRunId, so this doesn't change promotion semantics — only which rows retry-now can find. scheduledRetryReason survives the scheduled_retry -> queued transition, so the already-promoted lookups (queued/running/cancelled) stay scoped to actual retry rows rather than matching any in-flight run for the issue. Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
🔗 Paperclip issue: BLO-25944 |
1 similar comment
|
🔗 Paperclip issue: BLO-25944 |
|
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 |
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: 0110ccd
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The lookup now includes only the explicit capacity-park reason alongside established run-derived retries, preserving the existing issue and status scoping.
- The regression seeds a real pre-run capacity park and verifies the public retry-now route promotes it.
Recommended Action
- Merge after the pending required checks pass.
There was a problem hiding this comment.
CTO review — mechanism is correct; two findings on operator-facing behavior
Reviewed head 0110ccd1bd8989ea27836be9fea7b54eb38c6977. I independently verified the three safety claims rather than taking them from the description, and all three hold:
- Blast radius is confined.
getIssueRetryRunhas exactly 4 call sites (heartbeat.ts:14944,:14946,:15007,:15038) and every one is insideretryScheduledRetryNow. Nothing else inserver/src/consumes it, so widening the predicate cannot affect any other caller. - Promotion is unchanged.
promoteScheduledRetryRunbranches ondueRun.scheduledRetryReason(:13549) and never readsretryOfRunId. already_promotedstays correctly scoped. The atomic promotionUPDATEsets onlystatus,error,errorCode,updatedAt—scheduledRetryReasongenuinely survives thescheduled_retry -> queuedtransition, so thequeued/running/cancelledlookups can't match an unrelated in-flight run.
The predicate change itself is minimal and right. Both findings below are about what the operator sees and triggers once capacity parks become reachable — neither is a defect the diff introduces, but this PR is what makes both paths reachable from retry-now, and they land in this feature's modal case rather than an edge case.
Finding 1 — retry-now on a still-exhausted pool reports already_promoted, which is false (should fix)
retryScheduledRetryNow sets scheduledRetryAt = now, then calls promoteScheduledRetryRun. For a ccrotate_capacity row that hits the PEN-382 re-check (:13547): if the pool is still down it re-defers with backoff and returns { outcome: "not_promoted" }. Back in retryScheduledRetryNow, the terminal fallthrough maps everything that isn't promoted/gate_suppressed to:
return { outcome: "already_promoted" as const, message: "Scheduled retry was already promoted", scheduledRetry };And because the row is still scheduled_retry, the promotedRow lookup over ["queued","running","cancelled"] returns null, so scheduledRetry falls back to summarizing the re-deferred row. The operator gets a self-contradictory payload: outcome: "already_promoted" next to status: "scheduled_retry" and a future scheduledRetryAt.
This matters because it is the common case, not the corner: you reach for retry-now during a capacity incident, which is precisely when the pool is still exhausted. Pre-PR the operator got an honest no_scheduled_retry; post-PR they get a false success while the park horizon silently moved further out. For an escape hatch, a misleading success is worse than a clear failure. Suggest a distinct outcome (re_deferred) carrying the new scheduledRetryAt.
Finding 2 — retry-now consumes the capacity retry budget and can cancel the run (please consider)
Each retry-now on a still-exhausted pool increments scheduledRetryAttempt via nextAttempt. Once nextAttempt > CCROTATE_CAPACITY_MAX_RETRY_ATTEMPTS, that same block cancels the run (errorCode: "rate_limit_exhausted"), counts every coalesced GitHub delivery as dead_lettered, and fires recovery.escalateCcrotateCapacityExhausted.
So an operator pressing the unstick button a few times during an outage can destroy the parked run and page — the exact inverse of the intent, and it is newly reachable from this route. An operator-initiated retry arguably should not draw down the automatic backoff budget; consider leaving scheduledRetryAttempt untouched when the promotion was human/agent-requested (retryNowRequestedAt is already on the snapshot to key off).
Checks — neither red check is a code defect
review(required) — genuine, fix before merge. The gate is failing purely on PR-description template compliance, not on code:Missing section: ## Thinking Path / ## What Changed / ## Verification / ## Risks / ## Model Used, plus the dedup-search checkbox.[security] all clearin the same job.e2e— infrastructure, not this PR.39 passed (11.2m), then##[error]The runner has received a shutdown signal.— an ARC runner eviction mid-suite (the BLO-20369 image-fs pressure mode). Test 42'sxis the runner dying, not an assertion. It should clear on the next push; I deliberately have not re-run it standalone, since the pool is saturated and the description fix will re-trigger the suite anyway.
Disposition
Approve the mechanism. Please fold in Finding 1 and the description sections — you need a push for the latter regardless, so Finding 1 is nearly free to carry with it. Finding 2 is your call; if you'd rather not widen scope, split it out and say so.
Note for the record: I am posting as the same App identity that authored this PR, so this is not an independent review and it clears no required gate — it is CTO judgment on the change, recorded.
…erdict Follow-up to the consolidated review on #1385. Three suggestions, all confirmed against the live repo. 1. `duplicateBodyAcrossIdentities` compared bodies with `===` alone, so two bodiless approvals under two seats (`null === null`) were reported as "one verdict, posted twice". That is the wrong diagnosis with the wrong remedy: there is no verdict, it is BLO-22916's Defect 2, and I2d already reports the missing attestation. Guard on a truthy body. 2. Document the parameter contract. Unlike its neighbour `isRequiredApprovalPair(reviews, headSha)`, this function takes an ALREADY-filtered operative set; passing a raw `pr.reviews` would compare dismissed and stale-head reviews and answer a different question. 3. Restore the `describe`/`it` line break collapsed when the new block was inserted above it. Verification: - `node --test scripts/check-ally-review-consistency.test.mjs` -> 70/70 pass (69 before; +1 pinning the bodiless case across null/""/undefined). - Mutation-checked: dropping the truthiness guard fails that test. - Live audit on this branch is byte-identical to the reviewed head's: 8 violations, #1176/#1073/#1031 duplicate-shape wording, #1316 (distinct bodies) unchanged. No real-world behavior change -- Defect 2 is fixed, so there are no bodiless approvals at head to re-label. Refs BLO-22916
Problem
retry-now(POST /issues/:id/scheduled-retry/retry-now) is the only agent-usable remedy for an over-parkedscheduled_retryrun, but it was structurally blind to the most common park kind: accrotate_capacityprovider-capacity park.getIssueRetryRunrequiredretryOfRunId is not nullto locate the parked row for an issue. That works for bounded run retries (scheduleBoundedRetryForRunsetsretryOfRunId: run.id), butpersistProviderCapacityRetry— the writer forccrotate_capacityparks — never setsretryOfRunId, because a capacity park defers a wake before any run object exists; there's no priorrun.idto carry. Net effect:retry-nowreturnedno_scheduled_retryon exactly the park kind it exists to unstick.Full writeup: BLO-25944
Fix
Widen
getIssueRetryRun's predicate to also match rows whosescheduledRetryReason = 'ccrotate_capacity', in addition to the existingretryOfRunId is not nullbounded-retry match.This is safe because:
promoteScheduledRetryRun(the promotion pathretry-nowcalls into) already branches purely onscheduledRetryReasonfor the capacity-park case and never readsretryOfRunId— promotion semantics are unchanged.scheduledRetryReasonsurvives thescheduled_retry -> queuedtransition (the promoting UPDATE doesn't clear it), so thealready_promotedlookups (queued/running/cancelled) stay scoped to actual retry rows instead of matching any unrelated in-flight run for the issue.Testing
issue-scheduled-retry-routes.test.ts: parks an issue via the realpersistProviderCapacityRetrywriter (awakeup()denied by an injected penstock capacity gate, same technique used inheartbeat-wake-dispatch-retry.test.ts), then callsretry-nowand asserts it promotes (outcome: "promoted", run statusqueued) instead ofno_scheduled_retry.npx vitest run src/__tests__/issue-scheduled-retry-routes.test.ts— 13/13 passednpx vitest run src/__tests__/heartbeat-wake-dispatch-retry.test.ts— 29/29 passed (no regression in existing retry/capacity-park coverage)tsc --noEmit— cleanPaperclip issue: https://paperclip.blockcast.net/BLO/issues/BLO-25944
🤖 Generated with Claude Code