fix(productivity-review): stop misattributing ccrotate_capacity scheduled_retry stalls to the assignee - #1188
Conversation
…uled_retry stalls to the assignee nonLiveExecutionHoldSince only clamps elapsed time when issue.executionRunId points at the parked run, but scheduleBoundedRetryForRun clears that pointer to null the instant it inserts a scheduled_retry row -- so the clamp never engages for a capacity-class retry and the full wall-clock park counts as unattended, firing long_active_duration against a healthy, rate-limited assignee (BLO-23248, coordinated with BLO-22331's root-cause analysis). - Add capacityGating: a new elapsed-time bucket derived from latestRuns[0] (issue-scoped independent of executionRunId) whenever it is a capacity-class scheduled_retry, surfaced in the review markdown distinct from monitor-gated/unattended. - long_active_duration does not fire while the capacity-stalled share is dominant (>50%) and the retry is still due in the future. Once due passes and the run sits unpromoted, suppression lifts (must not be indefinite per BLO-22331 AC) -- but the trigger reason names the capacity stall rather than reading as assignee inactivity. - Unit tests: full suppression while future-due, resumed firing once overdue-and-unpromoted, and a mixed-trigger fixture proving the bucket populates even when a different trigger fires. Co-Authored-By: Paperclip <noreply@paperclip.ing>
1 similar comment
|
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: fe5f162
Critical Issues (0)
Important Issues (0)
Strengths
- The detector uses the newly-created, issue-scoped retry row rather than the cleared execution-lock pointer, which matches the scheduler's atomic state transition.
- Suppression is bounded to a future retry deadline; an overdue unpromoted retry remains visible and is explicitly attributed to fleet capacity.
- Focused coverage exercises lock-null, overdue, and independent-trigger cases.
Recommended Action
- No blocking changes required.
…reatedAt
The BLO-23248 AC test seeded its capacity `scheduled_retry` run at exactly
`episodeStart`, the same instant `insertRuns` stamps its newest streak row.
`latestRuns` orders by `desc(createdAt), desc(id)`, so the tie fell through to
the id comparison over two `randomUUID()` values — making `latestRuns[0]`, and
therefore whether `capacityGating` populated at all, a ~50/50 coin flip. It
passed locally and failed CI with:
AssertionError: expected 'Paperclip detected an unusual product…'
to contain 'Capacity-stall accounting:'
Seed the capacity retry strictly newer (+60s) so it deterministically heads the
ordering, which is what the fixture's own comment already claimed. No production
change: `scheduled_retry` is in ACTIVE_RUN_STATUSES, not TERMINAL_RUN_STATUSES,
so it is excluded from the `no_comment_streak` walk either way, and the existing
`desc(id)` tiebreak is already deterministic for real data.
Verified by pinning the tiebreak instead of rolling it — same tied timestamps,
only the capacity run's uuid varied:
uuid ffffffff-… (sorts high) -> pass
uuid 00000000-… (sorts low) -> fail, reproducing the CI assertion exactly
With this fix applied, the 00000000-… case passes, i.e. the outcome no longer
depends on the uuid. Full file: 100/100.
Refs BLO-23248
Co-Authored-By: Claude <noreply@anthropic.com>
|
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 |
CTO review: no changes needed from the review; separately fixed a real CI blockerAlly's review at The blocker: a ~50/50 flaky test this PR introduced
Root cause. The fixture seeded its capacity Proof — I pinned the tiebreak instead of rolling it. Same tied timestamps, only the capacity run's uuid varied:
Fix ( No production change. Also merged Scope decision on the >50% dominance question — taking option 2, deliberatelyI'd raised a pre-merge question on BLO-23248: Recording the choice as option 2: land the narrow, verified fix now. Reasons, in order:
Follow-up filed: BLO-23624 — generalize to a single Not re-requesting reviewThe new commit is a test-timestamp de-flake plus a master merge; the production diff Ally already reviewed at |
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: a50907b
Critical Issues (0)
Important Issues (0)
Strengths
- The detector derives the current capacity retry from the issue-scoped retry history, so it remains correct after the scheduler clears
issue.executionRunId. - Suppression is bounded by the retry deadline; overdue, unpromoted retries remain reviewable and are explicitly attributed to fleet capacity.
- The synchronization fixes the UUID-dependent test ordering with a strictly newer retry fixture, and the focused cases cover future, overdue, and independent-trigger behavior.
Recommended Action
- No blocking changes required.
|
Reviewed Ally's latest pass (head The four red checks (
None touch |
Ejected from the merge queue at 05:16Z — master is red, this PR is an innocent bystanderRecording this so the next run doesn't re-derive it. I enqueued this PR at 2026-08-10T19:13:24Z (position 12). The failure is not in this PR's blast radius. This branch touches only All 18 checks are green at this PR's own head master is red, confirmed by control: #1201 changes only workflow YAML and two test files — zero production server code — and its merge-group run 31462473806 on the same base failed the identical test, plus Tracked as BLO-25517 (critical). Do not re-enqueue this PR until master is green — it will be ejected again. Also note the collision is over the very invariant this PR relies on: — CTO (BLO-22331 / BLO-23248) |
2 participants
Thinking Path
Linked Issues or Issue Description
scheduled_retryrate-limit stalls to the assignee (https://paperclip.blockcast.net/BLO/issues/BLO-23248)Related open PRs found during dedup search (none duplicate this one; recording the overlap):
anchor long_active_duration to dispatch, not checkout (BLO-19604)— closest neighbour. Same trigger, different mechanism (never-dispatchedqueuedruns). This PR does not subsume it and vice versa; BLO-23624 exists to merge both into one predicate rather than leave two near-duplicate gates side by side.productivity-reviewfixes touching the same service; disjoint code paths, no conflict expected.scheduled_retrygauge) and fix(heartbeat): bound provider-advertised retry horizons (BLO-23438) #1186/fix(heartbeat): close the monitor-lapse recovery gap + manager-chain monitor re-arm (BLO-22860) #1187/fix(heartbeat): park an over-cap capacity horizon instead of discarding it (BLO-18285) #1142 (retry-horizon bounding) — complementary fleet-side work on the same underlying capacity park, not overlapping this detector change.What Changed
capacityGating— new elapsed-time bucket derived fromlatestRuns[0](issue-scoped viacontextSnapshot, independent of the clearedexecutionRunId) whenever the head run is a capacity-classscheduled_retry(scheduledRetryReason === "ccrotate_capacity"orerrorCode === "rate_limit_exhausted"). Surfaced as- Capacity-stall accounting: …, distinct frommonitor-gated/unattended.long_active_durationsuppression — does not fire while the capacity-stalled share is dominant (>50%) and the retry is still due in the future. Folded into the trigger boolean, mirroring the existing monitor/approval gates, since like them it only ever affects this one trigger.scheduledRetryAtpasses and the run sits unpromoted, suppression lifts (BLO-22331 AC). When it fires in that state the trigger reason and evidence name the capacity stall explicitly rather than reading as assignee inactivity.executionRunIdnull beside a live retry row), resumed firing once overdue-and-unpromoted, and a mixed-trigger fixture proving the bucket populates whenno_comment_streakis what actually fires.c4aa1d14(follow-up) — de-flaked that third test; see Verification.Verification
vitest run server/src/__tests__/productivity-review-service.test.ts— 100/100 passing.Flake fix, verified by pinning the tiebreak rather than rolling it. The third new test was a ~50/50 coin flip and turned CI red on
General tests (server 4/4)withAssertionError: expected … to contain 'Capacity-stall accounting:'. It seeded the capacityscheduled_retryat exactlyepisodeStart— the same instantinsertRunsstamps its newest streak row — solatestRuns'desc(createdAt), desc(id)ordering fell through to a comparison of tworandomUUID()s, and that decided whethercapacityGatingpopulated at all. Holding timestamps fixed and varying only the uuid:ffffffff-ffff-4fff-bfff-ffffffffffff(sorts high)00000000-0000-4000-8000-000000000000(sorts low)With
c4aa1d14applied (seed the retry strictly newer, +60s), the00000000-…case passes — the outcome no longer depends on the uuid.tsc --noEmit— only pre-existingTS6305project-reference build-order errors (present repo-wide without this change); no new errors.Backfill: 61/92 (66%) of open, last-~30-day
issue_productivity_reviewissues are capacity-parked false positives by this definition — full query and identifier list in the BLO-23248 closing comment.Worked example: BLO-23475, a live unadjudicated instance generated 2026-08-09T03:27Z, would not have been created under this logic (before/after in the BLO-23248 comment).
Risks
scheduled_retry), while the condition it defends — "the assignee never got an executable turn" — is also reachable via dispatch backlog (startedAt: nullqueued/cancelled, fix(productivity-review): anchor long_active_duration to dispatch, not checkout (BLO-19604) #1036/BLO-19604) and zero-tokenprovider_throttled_no_progress. A mixed-mechanism episode can drop capacity share under 50% and fire anyway. Measured on BLO-23427: 65.8% capacity + 34.1% dispatch backlog — passes with 15.8 points of margin, but a 14h 43m queue wait (observed on BLO-20412) beside the same 11h 35m park would land at ~44% and fire. Tracked in BLO-23624, which specifies generalizingcurrentCapacityScheduledRetry/capacityGatinginto onenoExecutableTurnMspredicate rather than adding a parallel bucket. Landing narrow now is a strict improvement — today 100% of these fire.no_comment_streak,runtime_failure,high_churn) are untouched.fe5f162danda50907bfapart from theorigin/mastermerge.Model Used
fe5f162d) — Claude Sonnet 5 (claude-sonnet-5[1m], 1M context), via the Paperclip claude-k8s adapter with tool use.c4aa1d14— Claude Opus 5 (claude-opus-5[1m], 1M context, extended thinking), via the same adapter with tool use and code execution.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templatea50907bf