feat(github): bound a review's waiting on the rate limit with a per-review budget - #827
Conversation
…ation GitHubApiError decides two things from the words of a 403 body: whether the failure is a throttle worth repeating at all, and whether it is the content-creation block the 30-second floor is sized against. Both are fixed alternations of specific phrases, so a body GitHub words differently matched neither. A missed block dropped the floor and spent the whole budget on the linear 5s/10s/15s inside a window three times as wide, and a missed throttle was worse: the call was read as a permission refusal and failed fast with no repeat, so the generated content was lost on the first refusal. Neither miss left anything in the log. The headers GitHub documents for its limits are read before any wording and decide the class on their own: Retry-After for a secondary limit and x-ratelimit-remaining: 0 for a primary one. A 403 carrying either is a throttle whatever the body says, and a Retry-After is honoured as the wait directly. What the headers cannot decide is the 403 with neither, which is how the measured block presented (remaining=4771): a permission refusal presents identically, so a remaining count above zero rules the primary limit out and nothing in, and the wording stays the deciding signal there, which is what GitHub's own documentation says of a secondary limit. The phrase list gains only wording GitHub is on record as sending: the secondary-rate-limits anchor of the documentation_url those responses carry, which is the one part of the body sent as a token rather than prose, and the "Request quota exhausted" primary-limit sentence. A 403 that still matches nothing while its body mentions a rate limit, blocking or abuse is not retried, but the retry now names it and the body at WARN; a throttle that names a block in unknown words is warned about once per call when its wait falls back to the linear backoff. The next wording GitHub adopts is added on evidence rather than guessed at.
…eview budget GitHubWriteRetry bounds one call at 90 seconds of waiting, and that bound is per call. A review posts each finding by up to three routes, the line-anchored comment, the same comment without its suggestion block and the thread on the file, so a finding GitHub refuses throughout can wait 3 x 90 seconds, and a review of fifty findings refused throughout can hold its pull request's dispatcher slot for roughly 225 minutes. The dispatcher serializes per pull request, so nothing else starves, but a review in that state is wedged for hours with nothing in the log saying it is waiting rather than hung. A review now publishes under a write-retry budget, configured by GITHUB_WRITE_RETRY_BUDGET with a default of five minutes and turned off by zero. Every wait the retry is about to serve during the review's publication is charged to it first. The wait that crosses the ceiling is still served, so the overrun is bounded by one clamped wait, and is warned about once, naming the pull request and the budget. After it a throttled write goes out once and is not repeated: the retry gives it up exactly as it does once the attempts are spent, so the write takes the path a write GitHub outlasted already takes, through the file-level fallback to the review body, and the review body's list of findings GitHub accepted no thread for says the budget is the reason and asks for a re-run. A first attempt is never withheld, since GitHub may have reopened and a write that lands is a finding saved. The ledger is thread state, for the reason GitHubLostWrites keeps its deliveries on the thread: the retry that charges it sits behind the REST client interface with no handle to be passed one through, and a review's routes run one after another on the thread publishing it. The on-demand commands and thread replies post outside a review and keep the per-call bound alone.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
🤖 ThrillhouseBot PR SummaryWhat this PR doesThis PR adds a per-review write-retry budget (GITHUB_WRITE_RETRY_BUDGET, default 5m) so that a review throttled by GitHub on every finding stops retrying after the budget is crossed instead of holding its PR dispatcher slot for hours; the crossing wait is served once and warned about, later throttled writes go out once unretried, and the review body's unanchored-findings section names the budget and asks for a re-run. Alongside it (#784), 403s whose body reads like a throttle or block in wording the classification whitelists do not know are now written down at WARN with the response diagnostics instead of failing silently like permission refusals, without widening either whitelist. Description vs. ImplementationNo mismatch found between the PR description and the change. Control-Flow Diagram🔀 Show diagramflowchart TD
A["postReview(request) called"] --> B["writeBudget.within(review)"]
B --> C{"budget <= 0 or ledger already open?"}
C -- "yes" --> D["publishReview(post) unchanged"]
C -- "no" --> E["open ThreadLocal ledger for review"]
E --> F["publishReview: post findings via routes"]
F --> G["route write fails, GitHubWriteRetry.retryDelay() runs"]
G --> H{"isThrottled()?"}
H -- "no (refusal)" --> I["warn on unrecognised throttle wording; give up write"]
H -- "yes" --> J{"attempt >= MAX_ATTEMPTS?"}
J -- "yes" --> K["give-up WARN with diagnostics; write fails"]
J -- "no" --> L["compute clamped wait"]
L --> M{"GitHubWriteBudget.admits(wait)"}
M -- "ledger exhausted" --> N["budget-spent WARN; no wait; write falls to next route or review body"]
M -- "wait crosses ceiling" --> O["serve the wait; mark exhausted; warn once naming review"]
M -- "within budget" --> P["sleep; retry write"]
O --> P
F --> Q["build review body: unanchored section reads exhausted()"]
Q --> R["review created"]
E --> S["finally: close ledger"]
Changes Overview
Changed Files
Risk Assessment
Things to double-check1 lower-confidence finding
|
| Check | Type | Status | Detail |
|---|---|---|---|
| trivy | check-run | ⏳ Pending | - |
| format | check-run | ⏳ Pending | - |
| frontend | check-run | ⏳ Pending | - |
| test | check-run | ⏳ Pending | - |
| dependency-review | check-run | ⏳ Pending | - |
Automated review by ThrillhouseBot. Reply with /review to re-run.
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
There was a problem hiding this comment.
ThrillhouseBot noted 1 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- LOW: Warn claims linear backoff lost the 30s floor even when Retry-After dictates the wait (
src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetry.java:302)
The #784 block-miss warning states: "GitHub throttled {} with wording that names a block but matched no known content-creation wording — retried on the linear backoff without the {}s floor; ...". The claim is emitted whenever error.hasUnrecognisedBlockWording() is true, but that predicate does not consider whether the response carried a Retry-After header. GitHubApiErrorTest (added in this diff) pins that the documented headers are read before any wording — a 403 with Retry-After: 45 yields retryDelay of 45s — so for a reworded block body that also carries Retry-After, the actual wait served is the header value (capped by MAX_DELAY_PER_ATTEMPT), not the linear 5s/10s/15s the log describes. Input not in the diff: a 403 whose body says something like "temporarily blocked from creating comments" (unknown block wording) accompanied by a Retry-After header. Consequence is limited to an inaccurate operator-facing log line, and the scenario may be rare, so risk is low; verify GitHubApiError.retryDelay() precedence for header-vs-linear before treating the message as ground truth. A fix would condition the message on the absence of a Retry-After response header.
thrillhousebot
Bot
added
enhancement
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…near backoff The warning for a throttle that names a block in unknown wording said the write was retried on the linear backoff. That is only what a body with no headers gets: a Retry-After or a reset instant is honoured before the wording is read, so the wait can be either of those. What is true in every case is that the wait goes out without the 30-second floor the block is sized against, so the line says that and no more.
…ation The budget was an instance sized from the raw config, held on the publisher through a second constructor so a test could hand in a smaller one. That gave the publisher a nine-parameter constructor and left the budget class shaped like a record for a single field, both of which SonarCloud flagged. The publisher already holds the typed configuration, so it reads the ceiling from there when it opens a review's ledger, and the ledger itself is what it always was: thread state, opened by whoever names the ceiling. The class keeps no configuration and no instances; a test sizes the ceiling through the mocked configuration like every other knob the publisher reads.
|
@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetry.java:302 — Warn claims linear backoff lost the 30s floor even when Retry-After dictates the wait Right: |
|
The next review will close every previous finding this comment names by its |
🤖 ThrillhouseBot — changes since the last review
|
…s served The block-miss line from #784 says the write is retried without the floor, and it was written before the review's write-retry budget had its say. Once the budget is spent the very next check refuses the wait, so the log said the write was retried and then that it was not. The report now runs only after the wait has been admitted. The budget line carries the response diagnostics, so nothing is lost for the write it stops. The test that pins the ledger being closed after a failing review is also reshaped so the assertThrows lambda holds one call.
🤖 ThrillhouseBot — changes since the last review
|
There was a problem hiding this comment.
ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:
- Check frontend is pending
- Check format is pending
- Check trivy is pending
- Check test is pending
- Check dependency-review is pending
ThrillhouseBot closed 1 previous finding(s) this round:
src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetry.java:250— Block-miss warning says 'retried' when the review budget refusal stops the retry
|
/review |
…udget # Conflicts: # CHANGELOG.md # src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetryTest.java
Merging main brought #784's copy of warnIfWordingWasMissed alongside this branch's, so the class carried the method twice and called it twice on the throttled path: once before the review's write-retry budget was consulted and once after. The early call is the one #784 shipped and the later one is what this branch moved behind the budget check, so a write the budget stopped was also reported as retried, and a throttle naming an unknown block was written down twice. Only the post-budget call survives, with the javadoc that explains why it sits there.
🤖 ThrillhouseBot — changes since the last review
|
There was a problem hiding this comment.
ThrillhouseBot noted 2 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- LOW: Budget ledger is thread-local, so a review's writes on different threads bypass the ceiling (
src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteBudget.java:96)
The ledger is stored in a ThreadLocal (OPEN). The PR description states 'a review's routes run one after another on the thread publishing it', but the diff does not show thatpostReviewand all its routes are guaranteed to run on the same thread. If any route (e.g., the file-level fallback, or a nested call) executes on a different thread,admitssees no ledger and admits every wait, so the per-review budget is not enforced. The description's claim is a stated assumption, not a demonstrated invariant in the provided code. Verify thatpublishReviewand all its downstream calls (inline comments, file-level fallbacks, review body) execute on the same thread aspostReview; if not, the budget is bypassed for those writes. - LOW: ThreadLocal ledger is not cleared if the review's work spawns a new thread that outlives the review (
src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteBudget.java:96)
The ledger is thread-local. IfpublishReview(or any route) spawns a new thread that performs a write after the review'swithinscope has closed, that thread will see no ledger and admit every wait, bypassing the budget. The PR description claims routes run on the publishing thread, but the diff does not show that no new threads are spawned. Verify that no route or downstream call creates a new thread; if it does, the budget is not enforced for those writes.
⚠️ Large PR — partial review. 13 file(s) were only partially reviewed because the model's response was cut at its length cap (max-output-tokens) — findings up to the cut were kept (src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewWriteBudgetTest.java, src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteBudgetTest.java, src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetryTest.java, src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteBudget.java, src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetry.java, src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java, src/main/java/dev/thiagogonzaga/thrillhousebot/config/ThrillhouseConfig.java, src/main/resources/application.properties, src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisherTest.java, .env.example, +3 more); the findings and verdict below cover only the reviewed portion.
|
/review |
There was a problem hiding this comment.
No new issues in this revision, but 2 previous finding(s) remain unresolved — fix them, or reply on their review thread with why they are deferred. A finding listed only under "Things to double-check" has no thread: clear it by commenting @thrillhousebot resolved path/to/File.java:42 — <the finding's title> on this PR.
…udget # Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:
- Check frontend is pending
- Check test is pending
- Check format is pending
- Check trivy is pending
- Check dependency-review is pending
Additionally, No new issues in this revision, but 2 previous finding(s) remain unresolved — fix them, or reply on their review thread with why they are deferred. A finding listed only under "Things to double-check" has no thread: clear it by commenting @thrillhousebot resolved path/to/File.java:42 — <the finding's title> on this PR.
…udget # Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:
- Check format is pending
- Check test is pending
- Check frontend is pending
- Check trivy is pending
- Check dependency-review is pending
Additionally, No new issues in this revision, but 2 previous finding(s) remain unresolved — fix them, or reply on their review thread with why they are deferred. A finding listed only under "Things to double-check" has no thread: clear it by commenting @thrillhousebot resolved path/to/File.java:42 — <the finding's title> on this PR.
|
/review |
🤖 ThrillhouseBot — changes since the last review
|
There was a problem hiding this comment.
ThrillhouseBot noted 1 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- LOW: Review body attributes every unanchored finding to the spent budget even when its cause differs (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java:506)
unanchoredFindingsBody produces a blanket causal sentence whenever the review's ledger reports exhaustion: it appends " — this review spent its ... write-retry budget waiting on GitHub's rate limit, so later writes were not retried; re-run/reviewto post them" over the whole section. The method's own javadoc (added in this diff) states the section carries two different causes — the finding "GitHub accepted no review thread for" can be "a diff and a post GitHub simply refused" — so a finding whose line and file routes were refused before the review crossed the budget, or refused with a non-throttle failure the retry never repeats regardless of any budget, lands in the same list and inherits the budget attribution. For that finding the appended text is false: its loss was not caused by the budget, and the "re-run/reviewto post them" guidance fails, because a re-run refuses the same anchoring again. The maintainer is sent re-running the bot for a defect a re-run cannot fix (and the javadoc's narrower claim — "every finding below it that was refused after the crossing shares the reason" — is true only for the subset, not for the section the sentence is attached to). The green ReviewWriteBudgetTest.aReviewStopsRetryingOnceItsBudgetIsSpentAndSaysSoBesideTheFindingsItCouldNotPost does not cover this: both of its findings are throttled throughout, so the section's cause is accurate in that fixture; a review with mixed causes would still render the same text. Restrict the sentence to findings refused after the crossing (track post-crossing unanchored results separately), or attribute it per finding.
…d to (#834) ## What type of PR is this? - [x] 🐛 Bug fix - [x] ✅ Test ## Description `GitHubWritePacer.DEFAULT_MAX_WAIT` is 90 s and `GitHubWritePacerTest` pins it to `GitHubWriteRetry.TOTAL_BUDGET` (#723: waiting for a pacing slot must never be worse than being refused and repeated). But the pacer reads `thrillhousebot.github.write-max-wait` first, and that property shipped as `60s`; the constant only applies to a build with no `application.properties`. So the pin guarded a value production never used, and a caller gave up on its slot 30 s before the refusal-and-repeat path would have. - `application.properties`, `.env.example` and the README default now say `90s`, with the reason next to the property. - The constant's javadoc states where the effective value comes from. - A second pin reads the shipped property through SmallRye with Quarkus's `DurationConverter` (the `DevSchemaManagementTest` pattern) and asserts it equals the constant, so the two cannot drift again. The second half of #830 (5/10/15 s backoff on a generic secondary limit with neither header, where GitHub's guidance is 60 s) is a recorded decision on the issue and needs no code; #784's new WARN will show if the case is ever observed. ## Related Issues Fixes #830 ## How Has This Been Tested? - [x] Unit tests Red on unfixed code: `GitHubWritePacerTest.theShippedPropertyDefaultCarriesTheSameCeiling:189 expected: <PT1M30S> but was: <PT1M>`. Green after the property change. `./mvnw -B clean compile spotbugs:check spotless:check` (`BugInstance size is 0`) and the full `./mvnw -B clean test`: 3632 tests, 0 failures. Patch coverage: no uncovered lines (the `src/main` change is javadoc only). ## Checklist - [x] My code follows the project's coding standards - [x] I have performed a self-review of my own code - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [x] I have updated the documentation accordingly - [x] My changes generate no new warnings or errors ## Screenshots / Logs N/A ## Additional Notes Touches `application.properties` and `README.md` near the lines #827 adds for `GITHUB_WRITE_RETRY_BUDGET`; if #827 lands first this rebases cleanly (different lines), and the `[Unreleased]` changelog section will need the usual keep-both resolution.
…udget # Conflicts: # .env.example # README.md # src/main/resources/application.properties
|
@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java:506 — Review body attributes every unanchored finding to the spent budget even when its cause differs Taken in 2c3e79b. The section does mix causes, and the sentence no longer claims the budget for all of it: it now says the review also spent its write-retry budget, that a finding refused after that was not retried, and that re-running @thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteBudget.java:96 — Budget ledger is thread-local, so a review's writes on different threads bypass the ceiling Checked, and there is no such write. @thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteBudget.java:96 — ThreadLocal ledger is not cleared if the review's work spawns a new thread that outlives the review No route spawns a thread (see the previous item), and the ledger cannot be left behind: |
|
The next review will close every previous finding this comment names by its |
|
🤖 ThrillhouseBot — changes since the last review
|
There was a problem hiding this comment.
ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:
- Check trivy is pending
- Check frontend is pending
- Check test is pending
- Check dependency-review is pending
- Check format is pending
ThrillhouseBot closed 1 previous finding(s) this round:
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java:506— Review body attributes every unanchored finding to the spent budget even when its cause differs
|
/review |
1 participant
What type of PR is this?
Description
GitHubWriteRetrybounds one call at 90 seconds of waiting, and that bound is per call. A review posts each finding by up to three routes — the line-anchored comment, the same comment without its suggestion block, and the thread on the file (#721) — so a finding GitHub refuses throughout can wait 3 × 90 s, and a review of fifty findings refused throughout can hold its pull request's dispatcher slot for roughly 225 minutes. The dispatcher serializes per pull request, so nothing else starves (verified: virtual-thread-per-task executor, per-PR slot), but a review in that state is wedged for hours with nothing in the log saying it is waiting rather than hung. The issue's numbers check out against the code:MAX_ATTEMPTS = 4,MAX_DELAY_PER_ATTEMPT = 30s,TOTAL_BUDGET = 90s, three routes per finding inReviewPublisher.postFindingCommentRoutes,max-review-comments = 50.The budget. A review now publishes under a write-retry budget,
GITHUB_WRITE_RETRY_BUDGET(thrillhousebot.github.write-retry-budget), default5m,0turns it off. It sits beside the pacing keys inThrillhouseConfig.GitHubConfig, is read byReviewPublisherthrough the typed configuration when it opens a review's ledger, and is documented in the README configuration table,application.propertiesand.env.examplein the existing style. Five minutes is room for a few of the 72-second content-creation blocks measured in #722 while staying far short of the hours a review refused throughout used to wait; a limit that outlasts it is the deployment writing too fast, which is the pacer's job, not a review holding its slot.How it is charged.
ReviewPublisher.postReviewopens a ledger for the review's publication (GitHubWriteBudget.within), andGitHubWriteRetry.retryDelayasks the ledger before serving any wait (GitHubWriteBudget.admits). What is charged is the waiting the retry serves, not wall-clock time: the model calls and the HTTP round trips are not what #734 measured, and a pacing wait is bounded on its own. The wait that crosses the ceiling is still served — the overrun is bounded by one clamped wait, and refusing it would throw away waiting already paid for — and it is warned about once, naming the pull request, the budget and what was being posted. The ledger is thread state, for the reasonGitHubLostWriteskeeps its deliveries on the thread: the retry sits behind the REST client interface with no handle to be passed one through, and a review's routes run one after another on the thread publishing it. Nested publication rejoins the open ledger rather than restarting it, the scope closes on every exit path, and outside a review (on-demand commands, thread replies) nothing changes.What happens to the rest. After the crossing a throttled write goes out once and is not repeated. The retry gives it up exactly as it does once the attempts are spent (with its own WARN line carrying the response's diagnostics), so the write takes the path a write GitHub outlasted already takes:
tryPostInlineCommentreturns the rejection reason,postFindingCommentRouteslogs it and falls topostFileLevelComment, and when that is refused too the finding lands inInlineCommentResult.unanchoredand the review body's "GitHub accepted no review thread for" section. That section now says the budget is the reason and asks for a re-run — stated once for the section, since the budget is the review's — andGitHubLostWritesstill records the loss for the dropped-post notice, since the refusal is a throttle. Nothing is dropped silently. A first attempt is never withheld: GitHub may have reopened, and a write that lands is a finding saved.Branched from
fix/784-throttle-classification(#820) rather thanmain, because both changeGitHubWriteRetry.retryDelayand its test; the diff here shows #820's commits until it merges.Related Issues
Fixes #734. Builds on #820 (#784).
How Has This Been Tested?
Red, in two steps. First the budget does not exist:
Then, with
GitHubWriteBudgetand the publisher's scope in place but the retry not consulting it (the red run predates the follow-up commit that moved the ceiling onto the typed configuration; the assertions are unchanged):A third red came from review: the #784 block-miss line ("retried, but without the floor") was written before the budget gate, so a write the budget stopped was logged as both retried and not retried:
ReviewWriteBudgetTestdrivesGitHubReviewClient's realdefaultmethods (the retry, the pacer and the lost-writes accounting all execute) for the reasonRescuedFindingLostWriteTestdoes: a mocked client stubs the seam under test away. With a 1-second budget andRetry-After: 1, two findings cost five HTTP attempts where sixteen were possible, and the review body names both findings and the budget.Green:
Checklist
Screenshots / Logs
The crossing, once per review, and the per-write stop after it:
And the review body's section:
Additional Notes
Refused first attempts after the crossing still go through the pacer, so a review with many findings left costs about one second per remaining route in pacing rather than nothing; that is bounded, and it is the price of never withholding a first attempt. The summary comment, thread resolution and labels are posted by the orchestrator outside
postReviewand are single writes under the per-call bound, which is what the issue measured as fine.