Repository navigation
Rate-limited PR reviews wait up to ~7h after the limit resets — the retry relies on GitHub's best-effort cron, which fires far less often than every 15 min #1994
Description
Summary
When pr-review defers a PR because an advisory reviewer is rate-limited, it writes:
<!-- pr-review-agent rate-limited v1 sha=<HEAD> status=rate-limited reset=<ISO> -->
from scripts/lib/advisory-review-gate.sh (~L340). The only thing that re-dispatches the review after reset is pr-review-sweep.yml / scripts/sweep-stuck-reviews.sh (~L300).
The sweep's comments call the cron the "guaranteed backstop (≤15 min)". It isn't: GitHub's schedule trigger is best-effort and gets dropped under load. Recent scheduled sweeps ran at 2026-09-30 21:22Z, then 2026-10-01 00:46Z, 06:36Z and 13:56Z, roughly every 3–7 hours against a 2,17,32,47 * * * * cron. The run history back to July shows about hourly at best.
The workflow_run fast path doesn't cover this case either: it is scoped to the PR whose CI just completed, so a PR waiting on a timer never gets one.
Observed on #1952
The 13:56Z sweep deferred #1952 because the reset (14:25:38Z) hadn't elapsed. No sweep has run since, so #1952 has not been re-reviewed. The reset was minutes away; the latency is governed by GitHub's cron, which is unbounded.
Goal
A time-bounded retry must fire close to its reset without depending on GitHub's scheduler. Target: re-dispatch within ~15 minutes after reset for resets up to ~60 minutes out. Longer resets may fall back to the sweep.
Acceptance criteria
- When a PR is deferred on an un-elapsed rate-limit marker whose
resetis within a bounded horizon (default ~60 min, configurable), a deterministic delayed retry is armed for that PR. It waits untilreset(+ small buffer) and then re-dispatches the review through the same path the sweep uses.- Suggested approach: the sweep, or the marker writer, dispatches a scoped run (
pr=<n>,not_before=<iso>) that sleeps untilnot_before, re-checks that the marker still applies (same head SHA, still not approved/reviewed at head), then dispatches. - Any approach is fine if it doesn't rely on
schedule. State the choice and the runner-minute cost.
- Suggested approach: the sweep, or the marker writer, dispatches a scoped run (
- Concurrency: today
pr-review-sweep.ymlgroups ongithub.event.workflow_run.head_branch || github.event_namewithcancel-in-progress: true, so twoworkflow_dispatchruns would cancel each other. Delayed per-PR retries must not cancel each other or the full sweep (e.g. a per-PR group withcancel-in-progress: false, or a separate job/workflow). - Idempotent and bounded:
- one armed retry per (PR, head SHA, reset);
- a newer push supersedes it, and the retry no-ops on a SHA mismatch;
- it respects the existing exclusions (escalated /
needs-human-review/ held PRs, P1 (#860 follow-up): retry crons must skip escalated/needs-human PRs before re-dispatching #946) andMAX_DISPATCH; - it never force-reviews.
- Correct the "≤15 min guaranteed backstop" comments in
pr-review-sweep.ymlto describe the real guarantee. - Tests:
- a deferral with a near reset arms exactly one retry;
- a far reset arms none and leaves it to the sweep;
- the retry no-ops on a SHA change or an already-reviewed head;
- two retries for different PRs don't cancel each other.
- Keep it scoped to the rate-limit retry. Don't rework the sweep's other duties.
Related
- Reliable scheduled Actions: off-peak cron offsets, scheduling standard, and schedule-reliability reporting #722: general schedule reliability (off-peak offsets, reporting). It's still open and complementary: this issue removes the dependency for the one time-critical retry rather than hoping the cron fires.
- [Phase 4] Narrow pr-review-sweep's cron scope to genuinely un-eventable cases and surface a sweep-hit-rate metric in daily health #1408: narrowed the sweep's cron scope to un-eventable cases.
- pr-review self-review deadlock:
ci-pendingskip + "already reviewed" marker blocks approval once CI goes green #898: theworkflow_runfast path.
Activity
github-actions commentedon Oct 1, 2026
Thanks for the detailed report and for the concrete evidence from the sweep run history and #1952, which makes the problem easy to follow. The proposed goal and acceptance criteria are clear. One question before this is picked up: for the bounded horizon, is ~60 minutes the right default for your workloads, or have you seen rate-limit resets in practice that are usually shorter or longer than that?
don-petry commentedon Oct 1, 2026
Plan — deterministic delayed retry for rate-limited PR reviews (#1994)
Problem recap
When
pr-reviewdefers a PR because an advisory reviewer is rate-limited, it writes<!-- pr-review-agent rate-limited v1 sha=<HEAD> status=rate-limited reset=<ISO> -->. The only thing thatre-dispatches after
resetis the scheduledpr-review-sweep.ymlcron, which GitHub drops under load(observed 3–7 h instead of every 15 min — #1952). We need a retry that fires close to
resetwithoutdepending on
schedule.Approach (AC1)
Arm a deterministic delayed retry from the sweep's defer branch (the sweep already holds the dispatch PAT;
the marker-writer runs under
GITHUB_TOKENand cannot dispatch). When the sweep defers a PR on an un-elapsedrate-limit marker whose
resetis within a bounded horizon (DELAYED_RETRY_HORIZON_SEC, default 3600 s),it dispatches a new
pr-review-delayed-retry.ymlworkflow withpr_url,head_sha,not_before=<reset ISO>.That workflow runs
scripts/delayed-rate-limit-retry.sh, which:not_before+ small buffer (DELAYED_RETRY_BUFFER_SEC, default 120 s), capped by a safetyceiling so a malformed
not_beforecan never sleep unbounded;head_sha— if a newer push superseded it, logs ano-op and exits 0;
sweep-stuck-reviews.shscoped to that onePR (
SWEEP_PRS_FILE) withARM_DELAYED_RETRY=false(no re-arm loop). The sweep re-validates the marker,standing verdict, CI and exclusions, then re-dispatches
pr-review-trigger.ymlwithout force.Resets beyond the horizon fall back to the existing cron sweep (unchanged behaviour).
Runner-minute cost: one mostly-sleeping runner per armed (PR, head), bounded by the horizon + buffer
(default ≤ ~62 min; hard-capped by the workflow
timeout-minutes). Re-arming is idempotent (see AC3), so armsdo not stack.
Concurrency (AC2)
pr-review-delayed-retry.ymluses a per-PR+SHA concurrency group (…-<pr_url>-<head_sha>). Different PRs(and different head SHAs) get distinct groups, so retries never cancel each other, and the group never matches
the sweep's group, so a retry never cancels the full sweep.
Idempotency / bounded (AC3)
head_sha→ the armed retry no-ops on the SHA-mismatch guard; a fresh review isdriven by the push event.
standing-verdict exclusions; arms are bounded by
MAX_DISPATCH. Never force-reviews.AC4
Correct the "guaranteed ≤15 min backstop" wording in
pr-review-sweep.yml(the cron is best-effort under load;the delayed retry is the near-reset path).
Tests (AC5) — written first (TDD)
tests/test_sweep_stuck_reviews.bats: near-reset defer arms exactly one retry; far-reset defer armsnone (preserves existing
FUTURE_RESET=2999tests);ARM_DELAYED_RETRY=falsenever arms.tests/test_delayed_rate_limit_retry.bats: retry no-ops on SHA change; no-ops onalready-reviewed/standing-verdict head (via real-sweep delegation with mocked
gh); two PRs' retries usedistinct concurrency groups (parsed from the workflow) so they don't cancel each other.
Change checklist
scripts/delayed-rate-limit-retry.sh(new).github/workflows/pr-review-delayed-retry.yml(new; Class 1,workflow_dispatchonly)scripts/sweep-stuck-reviews.sh—arm_delayed_retry()+ call in defer branch;ARM_DELAYED_RETRY/DELAYED_RETRY_HORIZON_SEC/DELAYED_RETRY_WORKFLOWenv.github/workflows/pr-review-sweep.yml— AC4 comment fixesdocs/agentic-interaction-model.md— §4 row for the new workflow (Class 1, notimer_role: the in-run sleepis a deterministic armed delay, not a scheduled trigger)
interaction-contracts/pr-review.yml— add the workflow +workflow_dispatch:pr-review-delayed-retryemitAGENTS.md— repo-specific-workflow exception for the new workflow.github/workflows/lint.yml— register the new bats fileVerification
bash scripts/run-bats.sh tests/test_delayed_rate_limit_retry.bats tests/test_sweep_stuck_reviews.bats,full bats suite,
bash .dev-lead/scripts/dev-lead-lint.sh,validate-interaction-model.sh, actionlint.Scope stays on the rate-limit retry (AC6) — the sweep's other duties are untouched.
don-petry commentedon Oct 1, 2026
On the horizon question: ~60 minutes (3600 s) is the right default.
The reset that stranded #1952 was ~30 minutes out (deferred 13:56Z, reset 14:25:38Z), so it falls well inside it. Permanent quota exhaustion like Codex's (#1993) has no meaningful reset, and the advisory gate already drops such bots, so it shouldn't drive the horizon. Keep it configurable as planned.
The plan above looks good to proceed with.
Generated by Claude Code
don-petry commentedon Oct 1, 2026
Dev-Lead: Implementation Complete — PR #1999
The implementation for issue #1994 is durable: commits are pushed and a pull request is open. This record is backed by verifiable artifacts you can confirm in one click.
dd20521d36764ec762d9b578e7b4f52388c23527main: 10 file(s), +566/-15 linesReview happens on the PR. Acceptance criteria and test results are evidenced by the PR's diff and CI checks — not asserted here. If a later stage fails, this claim is retracted in place.
feat: implement issue #1994 — Rate-limited PR reviews wait up to ~7h …