From a3c39ab84190904f462eb5cc16fa5e58ba2bee72 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Mon, 7 Sep 2026 09:40:02 -0300 Subject: [PATCH 1/7] fix(github): report a 403 whose throttle wording matched no classification 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. --- CHANGELOG.md | 4 + .../thrillhousebot/github/GitHubApiError.java | 83 ++++++++- .../github/GitHubWriteRetry.java | 37 ++++ .../github/GitHubApiErrorTest.java | 137 ++++++++++++++ .../github/GitHubWriteRetryTest.java | 175 ++++++++++++++++++ 5 files changed, 431 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 98a78176..65c43ed6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ All notable changes to ThrillhouseBot. ## [Unreleased] +### Fixed + +- **A 403 whose wording the throttle classification does not know is written down instead of failing like a permission refusal** (#784): both readings of a refused write, whether it is a throttle worth repeating and whether it is the content-creation block the 30-second floor is sized against, are whitelists of specific phrases, and a body GitHub words differently matched neither. A missed block dropped the floor and spent the budget on the linear 5s/10s/15s, and a missed throttle failed fast with no repeat, so the generated content was lost on the first refusal; both were silent. The headers GitHub documents (`retry-after`, `x-ratelimit-remaining: 0`) are read before any wording and decide the class on their own, and the wording is the fallback for the 403 that carries neither, 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, 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, and a throttle that names a block in unknown words is warned about once per call when its wait falls back to the linear backoff, so the next wording GitHub adopts is added on evidence rather than guessed at + ## [0.6.7] — 2026-09-07 Two production reviews drove this one: a pull request that was approved after most of the model's answer was thrown away, and one that was pushed to while under review and lost every finding to the push. The rest is hardening found by auditing the merged pull requests and by dogfooding the repository configuration. No configuration changes; upgrading is a redeploy. The one behaviour a deployment may notice is that `ignored-files` globs now match the way the documentation always said they did, so a pattern that was silently doing nothing starts excluding files. diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java index 7ac63d82..ea12b713 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java @@ -152,20 +152,57 @@ public final class GitHubApiError { Pattern.compile("(?Every alternative is a phrase GitHub is on record as sending (#784); none is a guess at a + * variant. {@code secondary rate limit} is the sentence documented above and measured in #722. + * {@code secondary-rate-limits} is the anchor of the {@code documentation_url} the same responses + * carry — {@code #secondary-rate-limits} under {@code resources-in-the-rest-api} and {@code + * #about-secondary-rate-limits} under {@code rate-limits-for-the-rest-api}, both recorded on the + * content-creation block in GitHub's community discussions #50326 and #32120 — and is the one + * part of the body sent as a token rather than as prose, so a message reworded around it is still + * read. {@code abuse detection} is the pre-2021 name for the same limit ("You have triggered an + * abuse detection mechanism…"). {@code rate limit exceeded} is the primary-limit sentence ("API + * rate limit exceeded for…"), and {@code request quota exhausted} the other primary-limit + * sentence GitHub sends beside it ("Request quota exhausted for request GET /search/issues", + * octokit/plugin-throttling#124); both usually arrive with {@code x-ratelimit-remaining: 0}, + * which is read first, and the words cover the response that does not. * *

The blocked-creation wording is carried here as well as in {@link #CONTENT_CREATION_BLOCK}, * in the same two word orders, because broadening only the latter would be inert: a body that * named the block without one of the other phrases would not be read as a throttle at all, so the * call would fail fast and the floor below it would never be consulted (#722). A 403 that says * creation is blocked is a throttle by definition, never a permission refusal. + * + *

A 403 that matches none of this and still talks about rate limits is not a throttle here, + * and is not silently a permission refusal either: {@link #hasUnrecognisedThrottleWording()} + * reports it, so the list grows on evidence rather than one imagined variant at a time. */ private static final Pattern THROTTLE_WORDING = Pattern.compile( - "(?i)secondary rate limit|abuse detection|rate limit exceeded" - + "|blocked from (?:content creation|creating content)"); + "(?i)secondary rate limit|secondary-rate-limits|abuse detection|rate limit exceeded" + + "|request quota exhausted|blocked from (?:content creation|creating content)"); + + /** + * What a 403 says when it is about rate limiting at all, in whatever words (#784). Deliberately + * loose, because it decides nothing about the call: it only decides whether a body that {@link + * #THROTTLE_WORDING} did not match is worth a warning, so the next wording GitHub adopts is a + * one-line fix on evidence rather than another guess. A false positive here costs one log line. + */ + private static final Pattern THROTTLE_HINT = Pattern.compile("(?i)rate.?limit|blocked|abuse"); + + /** + * The same reading for the block: a throttle whose body says something is blocked but not in the + * words {@link #CONTENT_CREATION_BLOCK} knows (#784). The generic secondary-limit sentence blocks + * nothing and says so, so it does not fire this. + */ + private static final Pattern BLOCK_HINT = Pattern.compile("(?i)blocked"); /** Backoff used when GitHub throttles without saying for how long. */ static final Duration FALLBACK_DELAY = Duration.ofSeconds(5); @@ -292,6 +329,18 @@ public static Optional of(WebApplicationException e) { * says so only through a {@code Retry-After}, an exhausted {@code x-ratelimit-remaining}, or the * rate-limit wording in the body. A permission 403 carries none of the three and so fails fast. * + *

The order is the headers first and the words last, and it is the whole of what the headers + * can decide (#784). {@code Retry-After} is the header GitHub documents for a secondary limit, + * and {@code x-ratelimit-remaining: 0} the one it documents for a primary limit, so a 403 + * carrying either is a throttle whatever the body says and however GitHub has reworded it since. + * What the headers cannot do is decide the rest: #784 proposed reading a 403 with {@code + * x-ratelimit-remaining} well above zero as a secondary limit outright, and that is exactly how + * the measured block presented ({@code remaining=4771}, no {@code Retry-After}) — but it is also + * exactly how a permission refusal presents, since a refused request counts against a quota it + * did not exhaust. A remaining count above zero rules the primary limit out and nothing in. So + * for a 403 with neither header the body is the only evidence there is, which is what GitHub's + * own documentation says of a secondary limit, and the wording stays the deciding signal there. + * *

The wording is looked for in {@link Body#classified}, not in the line that goes to the log: * the log's cap and the redaction bound are about what an operator should be shown, and letting * them decide whether a completed generation is repeated turned this into a question about where @@ -309,6 +358,30 @@ public boolean isThrottled() { || THROTTLE_WORDING.matcher(body.classified()).find(); } + /** + * Whether this is a 403 that {@link #isThrottled()} read as a refusal while its body talks about + * rate limiting, blocking or abuse — a throttle worded in a way the classification does not know + * (#784). The call still fails fast, since the hint is far too loose to spend three repeats on; + * what changes is that {@link GitHubWriteRetry} writes the body down at warning level, so the + * miss is a one-line fix instead of a permission refusal nobody can tell from a lost generation. + */ + public boolean hasUnrecognisedThrottleWording() { + return status == 403 && !isThrottled() && THROTTLE_HINT.matcher(body.classified()).find(); + } + + /** + * Whether this is a throttle whose body says something is blocked in words {@link + * #CONTENT_CREATION_BLOCK} does not know (#784). The repeat is kept, but the floor that makes the + * budget outlast the measured block is lost with the wording, so the linear backoff spends the + * whole budget inside the window — the #722 failure in different words. Reported for the same + * reason as {@link #hasUnrecognisedThrottleWording()}. + */ + public boolean hasUnrecognisedBlockWording() { + return isThrottled() + && !blocksContentCreation() + && BLOCK_HINT.matcher(body.classified()).find(); + } + /** * Whether GitHub rejected the credential rather than the request. 401 is the only status an * installation token that has expired, been revoked or been replaced can draw, and GitHub decides diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetry.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetry.java index dabc5693..f820ec91 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetry.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetry.java @@ -238,8 +238,12 @@ private Optional retryDelay( String operation, WebApplicationException failure, int attempt) { var error = GitHubApiError.of(failure); if (error.isEmpty() || !error.get().isThrottled()) { + error.ifPresent(refusal -> warnIfWordingWasMissed(operation, refusal)); return Optional.empty(); } + if (attempt == 1) { + warnIfWordingWasMissed(operation, error.get()); + } if (attempt >= MAX_ATTEMPTS) { if (log.isWarnEnabled()) { log.warn( @@ -254,6 +258,39 @@ private Optional retryDelay( return Optional.of(min(error.get().retryDelay(attempt, clock.get()), MAX_DELAY_PER_ATTEMPT)); } + /** + * Writes down a 403 whose body reads like a throttle but matched no known wording (#784). The + * classification is a whitelist of the phrases GitHub is known to send, and a miss used to be + * silent in both directions: a refusal-shaped miss failed fast exactly like a permission 403, and + * a block-shaped miss kept the repeat but lost the floor the budget is sized by. Neither decision + * changes here — a hint this loose must not spend repeats — but the body is named, so the next + * wording GitHub adopts is added on evidence rather than guessed at under review. Once per call: + * the throttled path asks on the first attempt only, since the body does not change between + * attempts. Behind a level check for the reason the give-up line above is. + */ + private void warnIfWordingWasMissed(String operation, GitHubApiError error) { + if (!log.isWarnEnabled()) { + return; + } + if (error.hasUnrecognisedThrottleWording()) { + log.warn( + "GitHub refused {} with a 403 that reads like a rate limit but matched no known throttle" + + " wording — not retried, so the generated content is lost; if this is a throttle," + + " its wording needs adding to GitHubApiError. {}", + operation, + error.diagnostics()); + } else if (error.hasUnrecognisedBlockWording()) { + log.warn( + "GitHub throttled {} with wording that names a block but matched no known" + + " content-creation wording — retried on the linear backoff without the {}s floor;" + + " if this is the content-creation block, its wording needs adding to" + + " GitHubApiError. {}", + operation, + GitHubApiError.CONTENT_CREATION_BLOCK_MIN_DELAY.toSeconds(), + error.diagnostics()); + } + } + private static Duration min(Duration left, Duration right) { return left.compareTo(right) <= 0 ? left : right; } diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java index e6503954..2635adb4 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java @@ -136,6 +136,143 @@ void anUnparsableRetryAfterIsIgnoredRatherThanGuessedAt() { GitHubApiError.from(outbound(403, PERMISSION_BODY, "Retry-After", "Wed, 21 Oct 2026 GMT")) .isThrottled()); } + + @Test + void theSecondaryLimitDocumentationAnchorIsEnoughOnItsOwn() { + // #784. The anchor is the one part of the body GitHub sends as a token rather than as prose, + // and it is the same token whichever sentence sits beside it: #secondary-rate-limits under + // resources-in-the-rest-api and #about-secondary-rate-limits under + // rate-limits-for-the-rest-api were both recorded on the content-creation block. A message + // reworded around it is still a secondary limit. + var body = + "{\"message\":\"Please slow down.\",\"documentation_url\":\"https://docs.github.com/rest" + + "/overview/rate-limits-for-the-rest-api#about-secondary-rate-limits\"}"; + assertTrue(GitHubApiError.from(outbound(403, body)).isThrottled()); + } + + @Test + void theRequestQuotaWordingIsAThrottle() { + // #784. "Request quota exhausted for request GET /search/issues" is a primary-limit wording + // GitHub sends beside "API rate limit exceeded"; it usually arrives with remaining=0, but the + // words are GitHub's and are honoured when the header is not there. + var body = + "{\"message\":\"Request quota exhausted for request POST /repos/o/r/issues/7/comments\"}"; + assertTrue(GitHubApiError.from(outbound(403, body)).isThrottled()); + } + } + + /** + * #784. Both classifications are whitelists of GitHub's wording, and a 403 that matched neither + * used to be indistinguishable from a permission refusal: it failed fast with no repeat, or kept + * the repeat and lost the floor, and nothing wrote down that the body had looked like a throttle. + * These pin the two readings that make a miss visible without widening either whitelist. + */ + @Nested + class WordingThatMatchedNoClassification { + + private static final Instant NOW = Instant.ofEpochSecond(1_800_000_000L); + + private static final String REWORDED_THROTTLE_BODY = + "{\"message\":\"You are being rate limited on this endpoint. Please slow down.\"}"; + + private static final String REWORDED_BLOCK_BODY = + "{\"message\":\"You have exceeded a secondary rate limit and have been temporarily blocked" + + " from creating comments.\"}"; + + @Test + void aRefusalThatTalksAboutRateLimitingIsReportedRatherThanTakenForAPermissionRefusal() { + var error = GitHubApiError.from(outbound(403, REWORDED_THROTTLE_BODY)); + + // The whitelist is not widened by the hint: the call still fails fast. What changes is that + // the miss is reported, so the wording can be added on evidence rather than guessed at. + assertFalse(error.isThrottled()); + assertTrue(error.hasUnrecognisedThrottleWording()); + } + + @Test + void aRefusalThatSaysBlockedOrAbuseIsReportedToo() { + assertTrue( + GitHubApiError.from(outbound(403, "{\"message\":\"You have been blocked.\"}")) + .hasUnrecognisedThrottleWording()); + assertTrue( + GitHubApiError.from(outbound(403, "{\"message\":\"Abuse of this endpoint.\"}")) + .hasUnrecognisedThrottleWording()); + } + + @Test + void aPermissionRefusalIsNotReported() { + var error = GitHubApiError.from(outbound(403, PERMISSION_BODY)); + + assertFalse(error.hasUnrecognisedThrottleWording()); + assertFalse(error.hasUnrecognisedBlockWording()); + } + + @Test + void aRecognisedThrottleIsNotReported() { + assertFalse( + GitHubApiError.from(outbound(403, SECONDARY_LIMIT_BODY)) + .hasUnrecognisedThrottleWording()); + assertFalse( + GitHubApiError.from(outbound(403, REWORDED_THROTTLE_BODY, "Retry-After", "45")) + .hasUnrecognisedThrottleWording()); + } + + @Test + void isReadOffA403Only() { + // A 429 is a throttle whatever it says, and a 422 about rate limiting is a payload problem. + assertFalse( + GitHubApiError.from(outbound(429, REWORDED_THROTTLE_BODY)) + .hasUnrecognisedThrottleWording()); + assertFalse( + GitHubApiError.from(outbound(422, REWORDED_THROTTLE_BODY)) + .hasUnrecognisedThrottleWording()); + } + + @Test + void aRewordedThrottleCarryingARetryAfterIsHonouredAtThatValue() { + // The headers GitHub documents for a secondary limit are read before any wording is, so a + // reworded body that came with a deadline is still a throttle and waits exactly that long. + var error = GitHubApiError.from(outbound(403, REWORDED_THROTTLE_BODY, "Retry-After", "45")); + + assertTrue(error.isThrottled()); + assertEquals(Duration.ofSeconds(45), error.retryDelay(1, NOW)); + } + + @Test + void aThrottleNamingABlockInUnknownWordsIsReported() { + var error = GitHubApiError.from(outbound(403, REWORDED_BLOCK_BODY)); + + // Still a throttle, on the generic wording; but the block whose width the floor is sized + // against is not recognised, so the wait falls back to the linear backoff. That is the + // #722 failure with different words, and it is what the report exists to make visible. + assertTrue(error.isThrottled()); + assertEquals(Duration.ofSeconds(5), error.retryDelay(1, NOW)); + assertTrue(error.hasUnrecognisedBlockWording()); + assertFalse(error.hasUnrecognisedThrottleWording()); + } + + @Test + void theMeasuredBlockIsNotReported() { + assertFalse( + GitHubApiError.from(outbound(403, CONTENT_CREATION_BLOCK_BODY)) + .hasUnrecognisedBlockWording()); + } + + @Test + void theGenericSecondaryLimitIsNotReportedAsAMissedBlock() { + // It names no block, so there is nothing the block wording could have missed. + assertFalse( + GitHubApiError.from(outbound(403, SECONDARY_LIMIT_BODY)).hasUnrecognisedBlockWording()); + } + + @Test + void aRefusalIsNeverReportedAsAMissedBlock() { + // "Blocked" on a body that is not a throttle at all is the other report's job. + var error = GitHubApiError.from(outbound(403, "{\"message\":\"You have been blocked.\"}")); + + assertFalse(error.hasUnrecognisedBlockWording()); + assertTrue(error.hasUnrecognisedThrottleWording()); + } } /** diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetryTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetryTest.java index f26b7173..0cb7ff72 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetryTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetryTest.java @@ -26,6 +26,7 @@ import java.time.Duration; import java.time.Instant; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import java.util.Optional; import java.util.concurrent.CopyOnWriteArrayList; @@ -35,6 +36,8 @@ import java.util.logging.Level; import java.util.logging.LogRecord; import java.util.logging.Logger; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; @@ -807,4 +810,176 @@ void theBudgetIsDerivedFromTheTwoBoundsThatProduceIt() { assertEquals(Duration.ofSeconds(90), derivedFromTheBounds); } } + + /** + * #784. The throttle classification is a whitelist of GitHub's wording, and a 403 that matched + * none of it used to be silent: it failed fast exactly like a permission refusal, so a reworded + * block cost the generated content on the first refusal and left nothing in the log to say the + * body had looked like a throttle. The decision is unchanged here — the whitelist is not widened + * by a hint — but a miss is written down, with the body, so the next one is a one-line fix. + */ + @Nested + class WordingThatMatchedNoClassification { + + private static final String REWORDED_THROTTLE_BODY = + "{\"message\":\"You are being rate limited on this endpoint. Please slow down.\"}"; + + private static final String REWORDED_BLOCK_BODY = + "{\"message\":\"You have exceeded a secondary rate limit and have been temporarily blocked" + + " from creating comments.\"}"; + + private final List logged = new CopyOnWriteArrayList<>(); + private final Logger julLogger = Logger.getLogger(GitHubWriteRetry.class.getName()); + private final Handler capture = + new Handler() { + @Override + public void publish(LogRecord entry) { + logged.add(entry); + } + + @Override + public void flush() { + // Nothing is buffered. + } + + @Override + public void close() { + // Nothing to release. + } + }; + private Level originalLevel; + + @BeforeEach + void captureLogging() { + originalLevel = julLogger.getLevel(); + julLogger.setLevel(Level.ALL); + julLogger.addHandler(capture); + } + + @AfterEach + void restoreLogging() { + julLogger.removeHandler(capture); + julLogger.setLevel(originalLevel); + } + + private List warnings() { + return logged.stream() + .filter(entry -> entry.getLevel().intValue() >= Level.WARNING.intValue()) + .map(entry -> entry.getMessage() + " " + Arrays.toString(entry.getParameters())) + .toList(); + } + + @Test + void aRefusalWordedLikeAThrottleStillFailsFastButSaysSo() { + var calls = new AtomicInteger(); + var refusal = failure(403, REWORDED_THROTTLE_BODY); + + var thrown = + assertThrows( + WebApplicationException.class, + () -> + retry.call( + "a comment on o/r #7", + () -> { + calls.incrementAndGet(); + throw refusal; + })); + + // Not retried: the hint decides nothing about the call. What it decides is that the log + // names the body that matched no wording, instead of reading like a permission refusal. + assertSame(refusal, thrown); + assertEquals(1, calls.get()); + assertEquals(List.of(), slept); + var warnings = warnings(); + assertEquals(1, warnings.size(), warnings.toString()); + assertTrue( + warnings.getFirst().contains("matched no known throttle wording"), warnings.toString()); + assertTrue(warnings.getFirst().contains("a comment on o/r #7"), warnings.toString()); + assertTrue(warnings.getFirst().contains("status=403"), warnings.toString()); + assertTrue( + warnings.getFirst().contains("rate limited on this endpoint"), warnings.toString()); + } + + @Test + void aPermissionRefusalIsNotReportedAsAMiss() { + var refusal = failure(403, "{\"message\":\"Resource not accessible by integration\"}"); + + assertThrows( + WebApplicationException.class, + () -> + retry.call( + "a comment on o/r #7", + () -> { + throw refusal; + })); + + assertEquals(List.of(), warnings()); + } + + @Test + void aThrottleNamingABlockInUnknownWordsIsRepeatedUnflooredAndSaysSoOnce() { + var calls = new AtomicInteger(); + + assertThrows( + WebApplicationException.class, + () -> + retry.call( + "a comment on o/r #7", + () -> { + calls.incrementAndGet(); + throw failure(403, REWORDED_BLOCK_BODY); + })); + + // Retried on the generic wording, but at the linear backoff: the block's 30-second floor was + // lost with the wording, which is the #722 failure in different words. Said once per call, + // not once per attempt, since the body does not change between them. + assertEquals(4, calls.get()); + assertEquals( + List.of(Duration.ofSeconds(5), Duration.ofSeconds(10), Duration.ofSeconds(15)), slept); + var misses = + warnings().stream() + .filter(line -> line.contains("matched no known content-creation wording")) + .toList(); + assertEquals(1, misses.size(), warnings().toString()); + assertTrue(misses.getFirst().contains("blocked from creating comments"), misses.toString()); + } + + @Test + void aRecognisedThrottleIsNotReportedAsAMissedBlock() { + var calls = new AtomicInteger(); + + retry.call( + "a comment on o/r #7", + () -> { + if (calls.incrementAndGet() == 1) { + throw throttled(); + } + return "posted"; + }); + + assertTrue( + warnings().stream().noneMatch(line -> line.contains("matched no known")), + warnings().toString()); + } + + @Test + void aMissIsNotReportedWhenWarningsAreOff() { + // The report sits behind a level check, because diagnostics() builds its string eagerly. + julLogger.setLevel(Level.OFF); + var refusal = failure(403, REWORDED_THROTTLE_BODY); + + var thrown = + assertThrows( + WebApplicationException.class, + () -> + retry.call( + "a comment on o/r #7", + () -> { + throw refusal; + })); + + assertSame(refusal, thrown); + assertTrue(logged.isEmpty(), logged.toString()); + } + } } From d22ad6dad371de47f8deaab101e850d0f02098c7 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Mon, 7 Sep 2026 10:05:30 -0300 Subject: [PATCH 2/7] feat(github): bound a review's waiting on the rate limit with a per-review 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. --- .env.example | 4 + CHANGELOG.md | 4 + README.md | 1 + .../config/ThrillhouseConfig.java | 13 + .../github/GitHubWriteBudget.java | 167 +++++++++++++ .../github/GitHubWriteRetry.java | 36 ++- .../review/ReviewPublisher.java | 59 ++++- src/main/resources/application.properties | 6 + .../github/GitHubWriteBudgetTest.java | 211 ++++++++++++++++ .../github/GitHubWriteRetryTest.java | 122 +++++++++ .../review/ReviewWriteBudgetTest.java | 236 ++++++++++++++++++ 11 files changed, 849 insertions(+), 10 deletions(-) create mode 100644 src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteBudget.java create mode 100644 src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteBudgetTest.java create mode 100644 src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewWriteBudgetTest.java diff --git a/.env.example b/.env.example index 1a868f7b..7bc2f295 100644 --- a/.env.example +++ b/.env.example @@ -22,6 +22,10 @@ GITHUB_WEBHOOK_SECRET=your_webhook_secret # goes out unpaced and the bounded backoff handles a refusal, so a long queue never parks a # finished command. #GITHUB_WRITE_MAX_WAIT=60s +# Optional: ceiling on how long one review may spend waiting on GitHub's rate limit across all of +# its writes. Once spent, later throttled writes in that review go out once and are not repeated; +# the review body names the findings they carried. 0 disables the ceiling. +#GITHUB_WRITE_RETRY_BUDGET=5m # Optional: webhook deduplication window for GitHub redeliveries #WEBHOOK_DEDUP_TTL=24h # Optional: comma-separated allowlist of logins permitted to trigger manual /review without repo access diff --git a/CHANGELOG.md b/CHANGELOG.md index 65c43ed6..1fc78629 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ All notable changes to ThrillhouseBot. - **A 403 whose wording the throttle classification does not know is written down instead of failing like a permission refusal** (#784): both readings of a refused write, whether it is a throttle worth repeating and whether it is the content-creation block the 30-second floor is sized against, are whitelists of specific phrases, and a body GitHub words differently matched neither. A missed block dropped the floor and spent the budget on the linear 5s/10s/15s, and a missed throttle failed fast with no repeat, so the generated content was lost on the first refusal; both were silent. The headers GitHub documents (`retry-after`, `x-ratelimit-remaining: 0`) are read before any wording and decide the class on their own, and the wording is the fallback for the 403 that carries neither, 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, 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, and a throttle that names a block in unknown words is warned about once per call when its wait falls back to the linear backoff, so the next wording GitHub adopts is added on evidence rather than guessed at +### Changed + +- **A review stops waiting on GitHub's rate limit once it has spent a per-review budget** (#734): the write backoff bounds one call at 90 seconds, and a review makes one call per route per finding, so a review GitHub refused throughout could hold its pull request's dispatcher slot for roughly 225 minutes at the default comment cap with nothing in the log saying it was waiting rather than hung. Every wait the backoff serves during a review's publication is now charged to `GITHUB_WRITE_RETRY_BUDGET` (default `5m`, `0` turns it off). The wait that crosses the ceiling is still served and is warned about once, naming the pull request; after it, a throttled write in that review goes out once and is not repeated, so it 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. The on-demand commands and thread replies post outside a review and keep the per-call bound alone + ## [0.6.7] — 2026-09-07 Two production reviews drove this one: a pull request that was approved after most of the model's answer was thrown away, and one that was pushed to while under review and lost every finding to the push. The rest is hardening found by auditing the merged pull requests and by dogfooding the repository configuration. No configuration changes; upgrading is a redeploy. The one behaviour a deployment may notice is that `ignored-files` globs now match the way the documentation always said they did, so a pattern that was silently doing nothing starts excluding files. diff --git a/README.md b/README.md index 95bd3783..2dc89a32 100644 --- a/README.md +++ b/README.md @@ -287,6 +287,7 @@ will change per provider: | `GITHUB_BOT_LOGINS` | Comma-separated bot account login(s) the bot skips to avoid replying to itself; override when deployed under a different App slug (`[bot]`) | `thrillhousebot[bot],thrillhouse-bot[bot]` | | `GITHUB_WRITE_MIN_INTERVAL` | Duration spacing two content-creating GitHub calls (comments, review comments, thread replies, reviews), shared process-wide. GitHub secondary-rate-limits rapid content creation and answers `403`; pacing keeps the bot inside that envelope instead of discovering it by rejection — its published guidance is no more than one such request per second. `0` disables pacing | `1s` | | `GITHUB_WRITE_MAX_WAIT` | Duration ceiling on how long one caller waits for its content-creation slot. Past it the call goes out unpaced and the bounded backoff handles a refusal, so a long queue never parks a finished command | `60s` | +| `GITHUB_WRITE_RETRY_BUDGET` | Duration ceiling on how long one review may spend waiting on GitHub's rate limit across all of its writes. The backoff bounds one call at 90s and a review makes one call per route per finding, so without it a review refused throughout could hold its PR's dispatcher slot for hours. Once spent, later throttled writes in that review go out once and are not repeated; the review body names the findings they carried and asks for a re-run. `0` disables the ceiling | `5m` | | `WEBHOOK_DEDUP_TTL` | Webhook deduplication time-to-live for GitHub redeliveries | `24h` | | `THRILLHOUSEBOT_REVIEW_MANUAL_TRIGGER_ALLOWED_LOGINS` | Comma-separated allowlist of logins permitted to run the slash commands without repo access; does not extend to the `@thrillhousebot resolved` directive, which always requires write access | _(empty)_ | | `MANUAL_TRIGGER_AUTH_TIMEOUT` | Upper bound on the manual-trigger write-access check on the webhook ACK thread; fails closed (denies) if GitHub is slower | `5s` | diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/config/ThrillhouseConfig.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/config/ThrillhouseConfig.java index 2770fb28..8575ddc4 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/config/ThrillhouseConfig.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/config/ThrillhouseConfig.java @@ -94,6 +94,19 @@ interface GitHubConfig { @WithName("write-max-wait") @WithDefault("60s") Duration writeMaxWait(); + + /** + * Ceiling on how long one review may spend waiting on GitHub's rate limit across all of its + * writes (#734). The backoff bounds one call at 90 seconds and a review makes one call per + * route per finding, so without this a review refused throughout could hold its pull request's + * dispatcher slot for hours. Once spent, later throttled writes in the review go out once and + * are not repeated; the review body names the findings they carried. Zero turns it off. + * Declared here as the namespace's schema, like the pacing keys above — the budget sits on the + * REST clients' write path and reads the key directly (see {@code GitHubWriteBudget}). + */ + @WithName("write-retry-budget") + @WithDefault("5m") + Duration writeRetryBudget(); } interface WebhookConfig { diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteBudget.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteBudget.java new file mode 100644 index 00000000..0684fc7a --- /dev/null +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteBudget.java @@ -0,0 +1,167 @@ +/* + * Copyright 2026 Thiago Gonzaga + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package dev.thiagogonzaga.thrillhousebot.github; + +import java.time.Duration; +import java.util.Optional; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Ceiling on how long one review may spend waiting on GitHub's rate limit across all of its writes + * (#734). + * + *

{@link GitHubWriteRetry} bounds one call at {@link GitHubWriteRetry#TOTAL_BUDGET}, 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 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. + * + *

So a review opens a ledger for the length of its publication ({@link #within}), and every wait + * the retry is about to serve on that thread is charged to it first ({@link #admits}). 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 + * review. Every throttled write after it is given up on without a wait: the retry treats the + * refusal as it treats spent attempts, so the write falls to the routes and disclosures that + * already exist for a write GitHub outlasted, and the review body says the budget is why (see + * {@code ReviewPublisher}). A first attempt is never withheld — GitHub may have reopened, and a + * write that lands is a finding saved — only the repeats are. + * + *

What is charged is the waiting the retry serves, not wall-clock time: the model calls before + * publication and the HTTP round trips themselves are not what #734 measured, and a pacing wait in + * {@link GitHubWritePacer} is bounded on its own. The ledger is thread state rather than an + * instance's, for the reason {@link GitHubLostWrites} keeps its deliveries on the thread: a + * review's routes run one after another on the thread publishing it, and the retry that charges the + * ledger sits behind the REST client interface with no handle to be passed one through. An instance + * carries only the size of the budget it opens a review with, which is what lets a test open one + * small enough to cross while the retry it exercises is the shared one. + * + *

Off outside a review: the on-demand commands and the thread replies post one piece of content + * each, and the per-call bound is the right one for them. + */ +public final class GitHubWriteBudget { + + private static final Logger log = LoggerFactory.getLogger(GitHubWriteBudget.class); + + /** Ceiling on one review's waiting across all of its writes; zero or negative turns it off. */ + public static final String KEY = "thrillhousebot.github.write-retry-budget"; + + /** + * Five minutes: room for a few of the 72-second content-creation blocks measured in #722 — the + * first finding refused inside one waits out most of the block and the rest flow — but well short + * of the hours a review refused throughout used to wait. A secondary limit that outlasts this is + * telling the deployment it is writing too fast, and the answer there is the pacing in {@link + * GitHubWritePacer}, not a review holding its slot until GitHub relents. + */ + static final Duration DEFAULT_BUDGET = Duration.ofMinutes(5); + + /** The instance production uses, sized once from configuration when the first review loads it. */ + public static final GitHubWriteBudget SHARED = + new GitHubWriteBudget(GitHubWritePacer.configured(KEY, DEFAULT_BUDGET)); + + /** The review open on this thread, if one is. Absent while nothing is. */ + private static final ThreadLocal OPEN = new ThreadLocal<>(); + + /** One review's running total against its ceiling. */ + private static final class Ledger { + private final String review; + private final Duration budget; + private Duration spent = Duration.ZERO; + private boolean exhausted; + + private Ledger(String review, Duration budget) { + this.review = review; + this.budget = budget; + } + } + + private final Duration budget; + + /** + * A budget of a given size. Production has one, {@link #SHARED}; this is how a publisher test in + * another package opens a review small enough to cross. + */ + public GitHubWriteBudget(Duration budget) { + this.budget = budget; + } + + /** The ceiling this instance opens a review with. */ + Duration budget() { + return budget; + } + + /** + * Runs one review's publication under this budget. A review already open on this thread is + * rejoined rather than restarted — nested publication is still the same review, and a ledger that + * reset on entry would let the inner scope spend what the outer one already had. A budget that is + * zero or negative opens nothing, so every wait is admitted. The ledger is closed on every exit + * path, so a review that fails leaves nothing behind for the next one on the thread. + */ + public void within(String review, Runnable work) { + if (!budget.isPositive() || OPEN.get() != null) { + work.run(); + return; + } + OPEN.set(new Ledger(review, budget)); + try { + work.run(); + } finally { + OPEN.remove(); + } + } + + /** + * Whether the retry may serve {@code wait} for {@code operation}, charging it to the review open + * on this thread when one is. Always yes outside a review. The wait that crosses the ceiling is + * admitted and is the last one that is; from then on the answer is no, and the retry gives the + * write up. The crossing is the one line the review leaves at warning level about its waiting, so + * it names the review, the ceiling and what was being posted when it was reached. + */ + static boolean admits(String operation, Duration wait) { + var ledger = OPEN.get(); + if (ledger == null) { + return true; + } + if (ledger.exhausted) { + return false; + } + ledger.spent = ledger.spent.plus(wait); + if (ledger.spent.compareTo(ledger.budget) >= 0) { + ledger.exhausted = true; + log.warn( + "The review of {} has spent its {}s write-retry budget waiting on GitHub's rate limit" + + " ({}s, the wait for {} included) — later throttled writes in this review are not" + + " retried, and the review body names the findings they carried", + ledger.review, + ledger.budget.toSeconds(), + ledger.spent.toSeconds(), + operation); + } + return true; + } + + /** + * The ceiling the review open on this thread has crossed, or empty while it has not — or while no + * review is open at all. What the review body reads when it explains a finding no route + * delivered. + */ + public static Optional exhausted() { + var ledger = OPEN.get(); + return ledger != null && ledger.exhausted ? Optional.of(ledger.budget) : Optional.empty(); + } +} diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetry.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetry.java index f820ec91..6ec3f950 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetry.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetry.java @@ -64,6 +64,12 @@ * are spent the failure propagates unchanged, and the log says the generated content was lost so an * operator can see the command needs re-running. * + *

That bound is per call, and a review makes one call per route per finding, so a review GitHub + * refuses throughout could still hold its slot for the sum of every route's backoff — hours, at the + * default comment cap (#734). Inside a review every wait is charged to the review's {@link + * GitHubWriteBudget} as well, and once that is spent a throttled write is given up on without a + * wait, exactly as it is once the attempts are. + * *

Why the budget is what it is

* * #722: the budget was three attempts, and its own documentation claimed the resulting 60s was @@ -223,16 +229,17 @@ private Optional replacementCredential( } /** - * How long to wait before repeating this failure, or empty when it must not be repeated — either - * because GitHub is refusing rather than throttling, or because the attempts are spent. The - * spent-attempts case is logged, because it is the one where a completed generation is discarded - * and the operator needs the response's own words to see why. + * How long to wait before repeating this failure, or empty when it must not be repeated — because + * GitHub is refusing rather than throttling, because the attempts are spent, or because the + * review this write belongs to has spent its {@link GitHubWriteBudget} (#734). The spent cases + * are logged, because they are the ones where a completed generation is discarded and the + * operator needs the response's own words to see why. * - *

That line sits behind a level check because {@link GitHubApiError#diagnostics()} builds its + *

Those lines sit behind a level check because {@link GitHubApiError#diagnostics()} builds its * string eagerly — a parameter placeholder defers the {@code toString}, not the call that - * produces the argument. The message itself is unchanged: this is the warning that surfaced the - * issue-624 diagnosis in production, so what it prints when it prints must stay exactly as it - * was. + * produces the argument. The spent-attempts message itself is unchanged: this is the warning that + * surfaced the issue-624 diagnosis in production, so what it prints when it prints must stay + * exactly as it was. */ private Optional retryDelay( String operation, WebApplicationException failure, int attempt) { @@ -255,7 +262,18 @@ private Optional retryDelay( } return Optional.empty(); } - return Optional.of(min(error.get().retryDelay(attempt, clock.get()), MAX_DELAY_PER_ATTEMPT)); + var delay = min(error.get().retryDelay(attempt, clock.get()), MAX_DELAY_PER_ATTEMPT); + if (!GitHubWriteBudget.admits(operation, delay)) { + if (log.isWarnEnabled()) { + log.warn( + "GitHub throttled {} after the review's write-retry budget was spent — not retried, so" + + " the content is lost unless a later route lands it. {}", + operation, + error.get().diagnostics()); + } + return Optional.empty(); + } + return Optional.of(delay); } /** diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java index c11f1e33..1ef1066b 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java @@ -21,6 +21,7 @@ import dev.thiagogonzaga.thrillhousebot.github.GitHubApiError; import dev.thiagogonzaga.thrillhousebot.github.GitHubCommentClient; import dev.thiagogonzaga.thrillhousebot.github.GitHubReviewClient; +import dev.thiagogonzaga.thrillhousebot.github.GitHubWriteBudget; import dev.thiagogonzaga.thrillhousebot.github.ReviewThreadService; import dev.thiagogonzaga.thrillhousebot.review.ai.ReviewResponse; import io.quarkus.logging.Log; @@ -54,6 +55,7 @@ public class ReviewPublisher { private final PrLabeler labeler; private final ThrillhouseConfig config; private final BotIdentity botIdentity; + private final GitHubWriteBudget writeBudget; @Inject public ReviewPublisher( @@ -65,6 +67,32 @@ public ReviewPublisher( PrLabeler labeler, ThrillhouseConfig config, BotIdentity botIdentity) { + this( + reviewClient, + commentClient, + reviewThreadService, + suggestionFormatter, + followUpAnalyzer, + labeler, + config, + botIdentity, + GitHubWriteBudget.SHARED); + } + + /** + * The same, naming the write-retry budget a review publishes under (#734). Production uses the + * shared, configured one; a test hands in one small enough to cross in a single wait. + */ + ReviewPublisher( + GitHubReviewClient reviewClient, + GitHubCommentClient commentClient, + ReviewThreadService reviewThreadService, + SuggestionFormatter suggestionFormatter, + FollowUpAnalyzer followUpAnalyzer, + PrLabeler labeler, + ThrillhouseConfig config, + BotIdentity botIdentity, + GitHubWriteBudget writeBudget) { this.reviewClient = reviewClient; this.commentClient = commentClient; this.reviewThreadService = reviewThreadService; @@ -73,6 +101,7 @@ public ReviewPublisher( this.labeler = labeler; this.config = config; this.botIdentity = botIdentity; + this.writeBudget = writeBudget; } /** @@ -333,7 +362,20 @@ void postReview( auth, owner, repo, prNumber, commitSha, result, lineResolver, false, List.of())); } + /** + * Publishes the review's outcome under the review's write-retry budget (#734): the inline + * comments, their file-level fallbacks and the review body all run inside one ledger, so a review + * GitHub refuses throughout stops waiting once the budget is spent rather than holding its pull + * request's dispatcher slot for the sum of every route's backoff. A write refused after that + * point takes the path a write GitHub outlasted already takes — the next route, then the review + * body — and {@link #unanchoredFindingsBody} says the budget is why. + */ void postReview(PostReviewRequest post) { + writeBudget.within( + post.owner() + "/" + post.repo() + " #" + post.prNumber(), () -> publishReview(post)); + } + + private void publishReview(PostReviewRequest post) { var auth = post.auth(); var owner = post.owner(); var repo = post.repo(); @@ -476,12 +518,27 @@ private static List skippedFindingsBodyParts(InlineCommentResult inline) * diff and a post GitHub simply refused, and on the round-7 corpus it was overwhelmingly the * second. Two independent scorers read it as a line-attribution defect and went looking for an * off-by-N that was not there, so the text now states only what happened. + * + *

One cause is named, because it is the one the maintainer can act on (#734): when the review + * spent its write-retry budget, the writes after that point were given up on without a repeat, + * and a re-run posts what they carried. Stated once for the section rather than per finding — the + * budget is the review's, and every finding below it that was refused after the crossing shares + * the reason. */ private static String unanchoredFindingsBody(List findings) { var sb = new StringBuilder(); sb.append("ThrillhouseBot found ") .append(findings.size()) - .append(" issue(s) GitHub accepted no review thread for:\n\n"); + .append(" issue(s) GitHub accepted no review thread for"); + GitHubWriteBudget.exhausted() + .ifPresent( + budget -> + sb.append(" — this review spent its ") + .append(budget.toSeconds()) + .append( + "s write-retry budget waiting on GitHub's rate limit, so later writes were" + + " not retried; re-run `/review` to post them")); + sb.append(":\n\n"); appendFindingList(sb, findings); return sb.toString(); } diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index bf08cd7f..15a95ec1 100644 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -47,8 +47,14 @@ thrillhousebot.github.bot-logins=${GITHUB_BOT_LOGINS:thrillhousebot[bot],thrillh # GitHub's published guidance is no more than one such request per second. # GITHUB_WRITE_MAX_WAIT: ceiling on one caller's wait for its slot. Past it the call goes out # unpaced and the bounded backoff handles a refusal, so a long queue never parks a command. +# GITHUB_WRITE_RETRY_BUDGET: ceiling on how long one review may spend waiting on GitHub's rate +# limit across all of its writes. The backoff bounds one call at 90s and a review makes one +# call per route per finding, so without it a review refused throughout could hold its PR's +# dispatcher slot for hours. Once spent, later throttled writes go out once and are not +# repeated; the review body names the findings they carried. 0 disables the ceiling. thrillhousebot.github.write-min-interval=${GITHUB_WRITE_MIN_INTERVAL:1s} thrillhousebot.github.write-max-wait=${GITHUB_WRITE_MAX_WAIT:60s} +thrillhousebot.github.write-retry-budget=${GITHUB_WRITE_RETRY_BUDGET:5m} # Webhook redelivery dedup — drop repeat X-GitHub-Delivery ids within the TTL so GitHub # redeliveries/retries do not trigger duplicate reviews (in-memory, per replica). diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteBudgetTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteBudgetTest.java new file mode 100644 index 00000000..dac97079 --- /dev/null +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteBudgetTest.java @@ -0,0 +1,211 @@ +/* + * Copyright 2026 Thiago Gonzaga + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package dev.thiagogonzaga.thrillhousebot.github; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.time.Duration; +import java.util.Arrays; +import java.util.List; +import java.util.Optional; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.logging.Handler; +import java.util.logging.Level; +import java.util.logging.LogRecord; +import java.util.logging.Logger; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * Covers {@link GitHubWriteBudget} — the per-review ceiling #734 asked for on the waiting {@link + * GitHubWriteRetry} serves. The retry bounds one call at 90 seconds, and a review posts one call + * per route per finding, so a review throttled on every finding could hold its pull request's + * dispatcher slot for hours with nothing in the log saying it was waiting rather than hung. + * + *

The arithmetic is pinned with hand-picked waits rather than a clock: what the budget counts is + * the waiting the retry was about to serve, and that is what these hand it. + */ +class GitHubWriteBudgetTest { + + private static final String REVIEW = "o/r #7"; + private static final String OPERATION = "an inline comment on o/r #7"; + private static final Duration FOUR_SECONDS = Duration.ofSeconds(4); + + private final List logged = new CopyOnWriteArrayList<>(); + private final Logger julLogger = Logger.getLogger(GitHubWriteBudget.class.getName()); + private final Handler capture = + new Handler() { + @Override + public void publish(LogRecord entry) { + logged.add(entry); + } + + @Override + public void flush() { + // Nothing is buffered. + } + + @Override + public void close() { + // Nothing to release. + } + }; + private Level originalLevel; + + @BeforeEach + void captureLogging() { + originalLevel = julLogger.getLevel(); + julLogger.setLevel(Level.ALL); + julLogger.addHandler(capture); + } + + @AfterEach + void restoreLogging() { + julLogger.removeHandler(capture); + julLogger.setLevel(originalLevel); + } + + private List warnings() { + return logged.stream() + .filter(entry -> entry.getLevel().intValue() >= Level.WARNING.intValue()) + .map(entry -> entry.getMessage() + " " + Arrays.toString(entry.getParameters())) + .toList(); + } + + @Test + void outsideAnyReviewEveryWaitIsAdmitted() { + // The on-demand commands and the thread replies post outside a review, and keep the + // per-call bound alone: a ledger that nobody opened bounds nothing. + assertTrue(GitHubWriteBudget.admits(OPERATION, Duration.ofHours(1))); + assertEquals(Optional.empty(), GitHubWriteBudget.exhausted()); + assertEquals(List.of(), warnings()); + } + + @Test + void aReviewIsStoppedOnceItsWaitingCrossesTheBudget() { + var budget = new GitHubWriteBudget(Duration.ofSeconds(6)); + var admitted = new java.util.ArrayList(); + + budget.within( + REVIEW, + () -> { + admitted.add(GitHubWriteBudget.admits(OPERATION, FOUR_SECONDS)); + assertEquals(Optional.empty(), GitHubWriteBudget.exhausted(), "4s of 6s is not spent"); + // The wait that crosses the ceiling is still served — the overrun is bounded by one + // clamped wait — and it is the last one that is. + admitted.add(GitHubWriteBudget.admits(OPERATION, FOUR_SECONDS)); + assertEquals(Optional.of(Duration.ofSeconds(6)), GitHubWriteBudget.exhausted()); + admitted.add(GitHubWriteBudget.admits(OPERATION, Duration.ofSeconds(1))); + }); + + assertEquals(List.of(true, true, false), admitted); + // Closed with the review: the next review on this thread starts from nothing. + assertEquals(Optional.empty(), GitHubWriteBudget.exhausted()); + assertTrue(GitHubWriteBudget.admits(OPERATION, FOUR_SECONDS)); + } + + @Test + void crossingTheBudgetIsWarnedAboutOnceNamingTheReviewAndTheBudget() { + var budget = new GitHubWriteBudget(Duration.ofSeconds(1)); + + budget.within( + REVIEW, + () -> { + GitHubWriteBudget.admits(OPERATION, Duration.ofSeconds(1)); + GitHubWriteBudget.admits(OPERATION, Duration.ofSeconds(1)); + GitHubWriteBudget.admits(OPERATION, Duration.ofSeconds(1)); + }); + + var warnings = warnings(); + assertEquals(1, warnings.size(), warnings.toString()); + assertTrue(warnings.getFirst().contains(REVIEW), warnings.toString()); + assertTrue(warnings.getFirst().contains("write-retry budget"), warnings.toString()); + assertTrue(warnings.getFirst().contains("1s"), warnings.toString()); + } + + @Test + void aZeroBudgetTurnsTheCeilingOff() { + var budget = new GitHubWriteBudget(Duration.ZERO); + + budget.within( + REVIEW, + () -> { + assertTrue(GitHubWriteBudget.admits(OPERATION, Duration.ofHours(1))); + assertEquals(Optional.empty(), GitHubWriteBudget.exhausted()); + }); + + assertEquals(List.of(), warnings()); + } + + @Test + void aNestedScopeRejoinsTheOpenReviewRatherThanStartingALedgerOfItsOwn() { + var outer = new GitHubWriteBudget(Duration.ofSeconds(6)); + var inner = new GitHubWriteBudget(Duration.ofHours(1)); + var admitted = new java.util.ArrayList(); + + outer.within( + REVIEW, + () -> { + admitted.add(GitHubWriteBudget.admits(OPERATION, FOUR_SECONDS)); + inner.within( + REVIEW, + () -> { + // Charged to the review already open, under its budget, not to a fresh hour. + admitted.add(GitHubWriteBudget.admits(OPERATION, FOUR_SECONDS)); + admitted.add(GitHubWriteBudget.admits(OPERATION, FOUR_SECONDS)); + }); + // The inner scope closing does not close the review it joined. + admitted.add(GitHubWriteBudget.admits(OPERATION, FOUR_SECONDS)); + assertEquals(Optional.of(Duration.ofSeconds(6)), GitHubWriteBudget.exhausted()); + }); + + assertEquals(List.of(true, true, false, false), admitted); + assertEquals(Optional.empty(), GitHubWriteBudget.exhausted()); + } + + @Test + void theScopeIsClosedEvenWhenTheReviewThrows() { + var budget = new GitHubWriteBudget(Duration.ofSeconds(1)); + + assertThrows( + IllegalStateException.class, + () -> + budget.within( + REVIEW, + () -> { + GitHubWriteBudget.admits(OPERATION, Duration.ofSeconds(1)); + throw new IllegalStateException("the review failed"); + })); + + assertEquals(Optional.empty(), GitHubWriteBudget.exhausted()); + assertTrue(GitHubWriteBudget.admits(OPERATION, FOUR_SECONDS)); + } + + @Test + void theSharedBudgetIsTheConfiguredOneAndDefaultsToFiveMinutes() { + // Nothing in the test configuration sets the key, so the shared instance carries the default + // the README documents. Read into locals first so neither constant sits in the expected slot. + var configured = GitHubWriteBudget.SHARED.budget(); + var documented = Duration.ofMinutes(5); + + assertEquals(documented, configured); + assertFalse(configured.isZero(), "the ceiling is on by default"); + } +} diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetryTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetryTest.java index 0cb7ff72..7de81594 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetryTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetryTest.java @@ -982,4 +982,126 @@ void aMissIsNotReportedWhenWarningsAreOff() { assertTrue(logged.isEmpty(), logged.toString()); } } + + /** + * #734. One call waits at most {@code TOTAL_BUDGET}, but a review makes one call per route per + * finding, so a review throttled on every finding could wait for hours while holding its pull + * request's dispatcher slot. Inside a review the waiting is charged to the review's budget as + * well, and once that is spent a throttled write is not repeated at all. Outside a review — the + * on-demand commands, the thread replies — nothing changes, which every other test here pins. + */ + @Nested + class TheReviewsWriteRetryBudget { + + private final List logged = new CopyOnWriteArrayList<>(); + private final Logger julLogger = Logger.getLogger(GitHubWriteRetry.class.getName()); + private final Handler capture = + new Handler() { + @Override + public void publish(LogRecord entry) { + logged.add(entry); + } + + @Override + public void flush() { + // Nothing is buffered. + } + + @Override + public void close() { + // Nothing to release. + } + }; + private Level originalLevel; + + @BeforeEach + void captureLogging() { + originalLevel = julLogger.getLevel(); + julLogger.setLevel(Level.ALL); + julLogger.addHandler(capture); + } + + @AfterEach + void restoreLogging() { + julLogger.removeHandler(capture); + julLogger.setLevel(originalLevel); + } + + private List warnings() { + return logged.stream() + .filter(entry -> entry.getLevel().intValue() >= Level.WARNING.intValue()) + .map(entry -> entry.getMessage() + " " + Arrays.toString(entry.getParameters())) + .toList(); + } + + private WebApplicationException throttledCall(AtomicInteger calls) { + return assertThrows( + WebApplicationException.class, + () -> + retry.call( + "a comment on o/r #7", + () -> { + calls.incrementAndGet(); + throw throttled("Retry-After", "4"); + })); + } + + @Test + void aThrottledWriteIsNotRepeatedOnceTheReviewHasSpentItsBudget() { + var calls = new AtomicInteger(); + + new GitHubWriteBudget(Duration.ofSeconds(6)) + .within( + "o/r #7", + () -> { + throttledCall(calls); + // The first wait leaves 2s; the second crosses the ceiling and is still served; + // the third is refused, so the call ends after three attempts rather than four. + assertEquals(3, calls.get()); + assertEquals(List.of(Duration.ofSeconds(4), Duration.ofSeconds(4)), slept); + + // The next write of the same review is not repeated at all. + throttledCall(calls); + assertEquals(4, calls.get()); + }); + + assertEquals(List.of(Duration.ofSeconds(4), Duration.ofSeconds(4)), slept); + var stops = warnings().stream().filter(line -> line.contains("write-retry budget")).toList(); + assertEquals(2, stops.size(), warnings().toString()); + assertTrue(stops.getFirst().contains("a comment on o/r #7"), stops.toString()); + assertTrue(stops.getFirst().contains("status=403"), stops.toString()); + } + + @Test + void theStopIsSilentWhenWarningsAreOff() { + // Behind the same level check as the give-up line, because diagnostics() is eager. + julLogger.setLevel(Level.OFF); + var calls = new AtomicInteger(); + + new GitHubWriteBudget(Duration.ofSeconds(1)) + .within( + "o/r #7", + () -> { + throttledCall(calls); + assertEquals(2, calls.get()); + }); + + assertEquals(List.of(Duration.ofSeconds(4)), slept); + assertTrue(logged.isEmpty(), logged.toString()); + } + + @Test + void aReviewThatNeverCrossesItsBudgetKeepsTheFullPerCallBackoff() { + var calls = new AtomicInteger(); + + new GitHubWriteBudget(Duration.ofHours(1)).within("o/r #7", () -> throttledCall(calls)); + + assertEquals(4, calls.get()); + assertEquals( + List.of(Duration.ofSeconds(4), Duration.ofSeconds(4), Duration.ofSeconds(4)), slept); + assertTrue( + warnings().stream().noneMatch(line -> line.contains("write-retry budget")), + warnings().toString()); + } + } } diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewWriteBudgetTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewWriteBudgetTest.java new file mode 100644 index 00000000..82201fc2 --- /dev/null +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewWriteBudgetTest.java @@ -0,0 +1,236 @@ +/* + * Copyright 2026 Thiago Gonzaga + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package dev.thiagogonzaga.thrillhousebot.review; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import dev.thiagogonzaga.thrillhousebot.config.BotIdentity; +import dev.thiagogonzaga.thrillhousebot.config.ThrillhouseConfig; +import dev.thiagogonzaga.thrillhousebot.github.GitHubCommentClient; +import dev.thiagogonzaga.thrillhousebot.github.GitHubReviewClient; +import dev.thiagogonzaga.thrillhousebot.github.GitHubWriteBudget; +import dev.thiagogonzaga.thrillhousebot.github.ReviewThreadService; +import jakarta.ws.rs.WebApplicationException; +import jakarta.ws.rs.core.Response; +import java.time.Duration; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; +import org.junit.jupiter.api.Test; + +/** + * #734 — what happens to a review's findings once the review has spent its write-retry budget. + * + *

Drives {@link GitHubReviewClient}'s own {@code default} methods for the reason {@code + * RescuedFindingLostWriteTest} does: the budget is charged inside the retry those methods run, so a + * mocked client would stub the very seam under test away. Only the client is real; the budget is + * handed to the publisher small enough to cross in one wait, and GitHub names a one-second deadline + * so that wait is real but short. + */ +class ReviewWriteBudgetTest { + + private static final String THROTTLE_BODY = + "{\"message\":\"You have exceeded a secondary rate limit.\"}"; + + /** The registry the client writes to is process-wide, so each test takes its own PR. */ + private static final AtomicInteger PR_NUMBERS = new AtomicInteger(800); + + private final int prNumber = PR_NUMBERS.incrementAndGet(); + private final FakeReviewClient reviewClient = new FakeReviewClient(); + + private ReviewPublisher publisher(GitHubWriteBudget budget) { + var suggestionFormatter = mock(SuggestionFormatter.class); + when(suggestionFormatter.formatReviewComment(any(), anyBoolean(), anyInt())) + .thenReturn("the finding"); + var config = mock(ThrillhouseConfig.class); + var reviewConfig = mock(ThrillhouseConfig.ReviewConfig.class); + when(config.review()).thenReturn(reviewConfig); + when(reviewConfig.maxReviewComments()).thenReturn(10); + return new ReviewPublisher( + reviewClient, + mock(GitHubCommentClient.class), + mock(ReviewThreadService.class), + suggestionFormatter, + mock(FollowUpAnalyzer.class), + mock(PrLabeler.class), + config, + BotIdentity.of("thrillhousebot[bot]"), + budget); + } + + /** + * GitHub refuses every write for the whole review. Without a review-wide ceiling each finding + * spends the full per-call budget on each of its routes; with one, the first wait that crosses it + * is the last wait the review serves, and every later write goes out once and is given up on. + * What the review says about those findings is the same thing it says about any finding no route + * delivered — with the reason added, so the maintainer knows a re-run will post them. + */ + @Test + void aReviewStopsRetryingOnceItsBudgetIsSpentAndSaysSoBesideTheFindingsItCouldNotPost() { + reviewClient.retryAfterSeconds = 1; + var first = finding("First bug", 10); + var second = finding("Second bug", 11); + + publisher(new GitHubWriteBudget(Duration.ofSeconds(1))) + .postReview( + "Bearer tok", "owner", "repo", prNumber, "sha", result(first, second), resolver()); + + // First finding, line route: one refusal, the one-second wait that spends the budget, a + // second refusal that is not waited on. Its file route and both routes of the second finding + // go out once each and are given up on. Five attempts in all, where sixteen were possible. + assertEquals(5, reviewClient.attempts.get(), "attempts: " + reviewClient.attempts); + var body = reviewClient.reviewBodies.getLast(); + assertTrue(body.contains("2 issue(s) GitHub accepted no review thread for"), body); + assertTrue(body.contains("write-retry budget"), body); + assertTrue(body.contains("1s"), body); + assertTrue(body.contains("First bug"), body); + assertTrue(body.contains("Second bug"), body); + } + + /** Control: a review that never crosses the ceiling discloses its lost findings as before. */ + @Test + void aReviewThatNeverCrossesTheBudgetSaysNothingAboutIt() { + reviewClient.retryAfterSeconds = 0; + var finding = finding("Only bug", 10); + + publisher(new GitHubWriteBudget(Duration.ofHours(1))) + .postReview("Bearer tok", "owner", "repo", prNumber, "sha", result(finding), resolver()); + + var body = reviewClient.reviewBodies.getLast(); + assertTrue(body.contains("1 issue(s) GitHub accepted no review thread for:"), body); + assertFalse(body.contains("write-retry budget"), body); + assertTrue(body.contains("Only bug"), body); + } + + // -------------------------------------------------------------------------------- the fixture + + private static Finding finding(String title, int line) { + return new Finding(RiskLevel.HIGH, "src/Main.java", line, title, "desc", null, null); + } + + private static ReviewResult result(Finding... findings) { + return new ReviewResult( + List.of(findings), + 0, + findings.length, + 0, + 0, + RiskLevel.HIGH, + ReviewState.REQUEST_CHANGES, + true, + "", + List.of(), + List.of(), + 0); + } + + private static DiffLineResolver resolver() { + return new DiffLineResolver( + Map.of("src/Main.java", "@@ -10,2 +10,2 @@\n-old\n-old\n+new\n+new")); + } + + /** Real everywhere the retry and the accounting live; only the HTTP attempts are stubbed. */ + private static final class FakeReviewClient implements GitHubReviewClient { + + private final AtomicInteger attempts = new AtomicInteger(); + private final List reviewBodies = new ArrayList<>(); + private int retryAfterSeconds; + + @Override + public PullRequestCommentResponse createPullRequestCommentOnce( + String auth, + String accept, + String owner, + String repo, + int pullNumber, + CreatePullRequestCommentRequest request) { + attempts.incrementAndGet(); + throw new WebApplicationException( + Response.status(403) + .header("Retry-After", String.valueOf(retryAfterSeconds)) + .entity(THROTTLE_BODY) + .build()); + } + + @Override + public ReviewResponse createReviewOnce( + String auth, + String accept, + String owner, + String repo, + int pullNumber, + CreateReviewRequest request) { + reviewBodies.add(request.body()); + return new ReviewResponse(1L, request.body(), request.event(), request.commitId(), null); + } + + @Override + public List listReviewsPageOnce( + String auth, + String accept, + String owner, + String repo, + int pullNumber, + int perPage, + int page) { + return List.of(); + } + + @Override + public List listPullRequestCommentsPageOnce( + String auth, + String accept, + String owner, + String repo, + int pullNumber, + int perPage, + int page) { + return List.of(); + } + + @Override + public PullRequestComment getPullRequestCommentOnce( + String auth, String accept, String owner, String repo, long commentId) { + throw new UnsupportedOperationException("not part of this seam"); + } + + @Override + public PullRequestCommentResponse replyToReviewCommentOnce( + String auth, + String accept, + String owner, + String repo, + int pullNumber, + long commentId, + ReplyToReviewCommentRequest request) { + throw new UnsupportedOperationException("not part of this seam"); + } + + @Override + public void deletePendingReview( + String auth, String accept, String owner, String repo, int pullNumber, long reviewId) { + throw new UnsupportedOperationException("not part of this seam"); + } + } +} From 6bcf55b2102d26a52dce3f0b5d882838502b9b76 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Mon, 7 Sep 2026 10:20:07 -0300 Subject: [PATCH 3/7] fix(github): say what the unfloored wait is rather than naming the linear 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. --- CHANGELOG.md | 2 +- .../thrillhousebot/github/GitHubApiError.java | 7 ++++--- .../thrillhousebot/github/GitHubWriteRetry.java | 6 +++--- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1fc78629..d2d1df35 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ All notable changes to ThrillhouseBot. ### Fixed -- **A 403 whose wording the throttle classification does not know is written down instead of failing like a permission refusal** (#784): both readings of a refused write, whether it is a throttle worth repeating and whether it is the content-creation block the 30-second floor is sized against, are whitelists of specific phrases, and a body GitHub words differently matched neither. A missed block dropped the floor and spent the budget on the linear 5s/10s/15s, and a missed throttle failed fast with no repeat, so the generated content was lost on the first refusal; both were silent. The headers GitHub documents (`retry-after`, `x-ratelimit-remaining: 0`) are read before any wording and decide the class on their own, and the wording is the fallback for the 403 that carries neither, 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, 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, and a throttle that names a block in unknown words is warned about once per call when its wait falls back to the linear backoff, so the next wording GitHub adopts is added on evidence rather than guessed at +- **A 403 whose wording the throttle classification does not know is written down instead of failing like a permission refusal** (#784): both readings of a refused write, whether it is a throttle worth repeating and whether it is the content-creation block the 30-second floor is sized against, are whitelists of specific phrases, and a body GitHub words differently matched neither. A missed block dropped the floor and spent the budget on the linear 5s/10s/15s, and a missed throttle failed fast with no repeat, so the generated content was lost on the first refusal; both were silent. The headers GitHub documents (`retry-after`, `x-ratelimit-remaining: 0`) are read before any wording and decide the class on their own, and the wording is the fallback for the 403 that carries neither, 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, 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, and a throttle that names a block in unknown words is warned about once per call, since its wait goes out without the 30-second floor the block is sized against, so the next wording GitHub adopts is added on evidence rather than guessed at ### Changed diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java index ea12b713..b2b229d0 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java @@ -372,9 +372,10 @@ public boolean hasUnrecognisedThrottleWording() { /** * Whether this is a throttle whose body says something is blocked in words {@link * #CONTENT_CREATION_BLOCK} does not know (#784). The repeat is kept, but the floor that makes the - * budget outlast the measured block is lost with the wording, so the linear backoff spends the - * whole budget inside the window — the #722 failure in different words. Reported for the same - * reason as {@link #hasUnrecognisedThrottleWording()}. + * budget outlast the measured block is lost with the wording: the wait is whatever {@code + * Retry-After}, the reset instant or the linear backoff gave, unlifted, and every one of those + * undershoots the block — the #722 failure in different words. Reported for the same reason as + * {@link #hasUnrecognisedThrottleWording()}. */ public boolean hasUnrecognisedBlockWording() { return isThrottled() diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetry.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetry.java index 6ec3f950..2ab4eeab 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetry.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetry.java @@ -300,9 +300,9 @@ private void warnIfWordingWasMissed(String operation, GitHubApiError error) { } else if (error.hasUnrecognisedBlockWording()) { log.warn( "GitHub throttled {} with wording that names a block but matched no known" - + " content-creation wording — retried on the linear backoff without the {}s floor;" - + " if this is the content-creation block, its wording needs adding to" - + " GitHubApiError. {}", + + " content-creation wording — retried, but without the {}s floor that block is" + + " sized against; if this is the content-creation block, its wording needs adding" + + " to GitHubApiError. {}", operation, GitHubApiError.CONTENT_CREATION_BLOCK_MIN_DELAY.toSeconds(), error.diagnostics()); From 935218839a82d61de768a4850de73105b63d66e4 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Mon, 7 Sep 2026 10:27:32 -0300 Subject: [PATCH 4/7] refactor(review): read the write-retry budget from the typed configuration 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. --- .../config/ThrillhouseConfig.java | 5 +- .../github/GitHubWriteBudget.java | 47 ++++-------------- .../review/ReviewPublisher.java | 37 +++----------- .../github/GitHubWriteBudgetTest.java | 49 +++++++------------ .../github/GitHubWriteRetryTest.java | 44 ++++++++--------- .../review/ReviewOrchestratorTest.java | 2 + .../review/ReviewPublisherTest.java | 5 ++ .../review/ReviewWriteBudgetTest.java | 20 ++++---- 8 files changed, 75 insertions(+), 134 deletions(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/config/ThrillhouseConfig.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/config/ThrillhouseConfig.java index 8575ddc4..633ff870 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/config/ThrillhouseConfig.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/config/ThrillhouseConfig.java @@ -100,9 +100,8 @@ interface GitHubConfig { * writes (#734). The backoff bounds one call at 90 seconds and a review makes one call per * route per finding, so without this a review refused throughout could hold its pull request's * dispatcher slot for hours. Once spent, later throttled writes in the review go out once and - * are not repeated; the review body names the findings they carried. Zero turns it off. - * Declared here as the namespace's schema, like the pacing keys above — the budget sits on the - * REST clients' write path and reads the key directly (see {@code GitHubWriteBudget}). + * are not repeated; the review body names the findings they carried. Zero turns it off. Read by + * {@code ReviewPublisher} when it opens a review's ledger (see {@code GitHubWriteBudget}). */ @WithName("write-retry-budget") @WithDefault("5m") diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteBudget.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteBudget.java index 0684fc7a..af8756a8 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteBudget.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteBudget.java @@ -44,12 +44,12 @@ * *

What is charged is the waiting the retry serves, not wall-clock time: the model calls before * publication and the HTTP round trips themselves are not what #734 measured, and a pacing wait in - * {@link GitHubWritePacer} is bounded on its own. The ledger is thread state rather than an - * instance's, for the reason {@link GitHubLostWrites} keeps its deliveries on the thread: a - * review's routes run one after another on the thread publishing it, and the retry that charges the - * ledger sits behind the REST client interface with no handle to be passed one through. An instance - * carries only the size of the budget it opens a review with, which is what lets a test open one - * small enough to cross while the retry it exercises is the shared one. + * {@link GitHubWritePacer} is bounded on its own. The ledger is thread state, for the reason {@link + * GitHubLostWrites} keeps its deliveries on the thread: a review's routes run one after another on + * the thread publishing it, and the retry that charges the ledger sits behind the REST client + * interface with no handle to be passed one through. The size of the ceiling is the caller's to + * name — the publisher reads it from the typed configuration, {@code + * thrillhousebot.github.write-retry-budget} — so this class holds no configuration of its own. * *

Off outside a review: the on-demand commands and the thread replies post one piece of content * each, and the per-call bound is the right one for them. @@ -58,22 +58,6 @@ public final class GitHubWriteBudget { private static final Logger log = LoggerFactory.getLogger(GitHubWriteBudget.class); - /** Ceiling on one review's waiting across all of its writes; zero or negative turns it off. */ - public static final String KEY = "thrillhousebot.github.write-retry-budget"; - - /** - * Five minutes: room for a few of the 72-second content-creation blocks measured in #722 — the - * first finding refused inside one waits out most of the block and the rest flow — but well short - * of the hours a review refused throughout used to wait. A secondary limit that outlasts this is - * telling the deployment it is writing too fast, and the answer there is the pacing in {@link - * GitHubWritePacer}, not a review holding its slot until GitHub relents. - */ - static final Duration DEFAULT_BUDGET = Duration.ofMinutes(5); - - /** The instance production uses, sized once from configuration when the first review loads it. */ - public static final GitHubWriteBudget SHARED = - new GitHubWriteBudget(GitHubWritePacer.configured(KEY, DEFAULT_BUDGET)); - /** The review open on this thread, if one is. Absent while nothing is. */ private static final ThreadLocal OPEN = new ThreadLocal<>(); @@ -90,29 +74,16 @@ private Ledger(String review, Duration budget) { } } - private final Duration budget; - - /** - * A budget of a given size. Production has one, {@link #SHARED}; this is how a publisher test in - * another package opens a review small enough to cross. - */ - public GitHubWriteBudget(Duration budget) { - this.budget = budget; - } - - /** The ceiling this instance opens a review with. */ - Duration budget() { - return budget; - } + private GitHubWriteBudget() {} /** - * Runs one review's publication under this budget. A review already open on this thread is + * Runs one review's publication under {@code budget}. A review already open on this thread is * rejoined rather than restarted — nested publication is still the same review, and a ledger that * reset on entry would let the inner scope spend what the outer one already had. A budget that is * zero or negative opens nothing, so every wait is admitted. The ledger is closed on every exit * path, so a review that fails leaves nothing behind for the next one on the thread. */ - public void within(String review, Runnable work) { + public static void within(String review, Duration budget, Runnable work) { if (!budget.isPositive() || OPEN.get() != null) { work.run(); return; diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java index 1ef1066b..05548b2f 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java @@ -55,7 +55,6 @@ public class ReviewPublisher { private final PrLabeler labeler; private final ThrillhouseConfig config; private final BotIdentity botIdentity; - private final GitHubWriteBudget writeBudget; @Inject public ReviewPublisher( @@ -67,32 +66,6 @@ public ReviewPublisher( PrLabeler labeler, ThrillhouseConfig config, BotIdentity botIdentity) { - this( - reviewClient, - commentClient, - reviewThreadService, - suggestionFormatter, - followUpAnalyzer, - labeler, - config, - botIdentity, - GitHubWriteBudget.SHARED); - } - - /** - * The same, naming the write-retry budget a review publishes under (#734). Production uses the - * shared, configured one; a test hands in one small enough to cross in a single wait. - */ - ReviewPublisher( - GitHubReviewClient reviewClient, - GitHubCommentClient commentClient, - ReviewThreadService reviewThreadService, - SuggestionFormatter suggestionFormatter, - FollowUpAnalyzer followUpAnalyzer, - PrLabeler labeler, - ThrillhouseConfig config, - BotIdentity botIdentity, - GitHubWriteBudget writeBudget) { this.reviewClient = reviewClient; this.commentClient = commentClient; this.reviewThreadService = reviewThreadService; @@ -101,7 +74,6 @@ public ReviewPublisher( this.labeler = labeler; this.config = config; this.botIdentity = botIdentity; - this.writeBudget = writeBudget; } /** @@ -368,11 +340,14 @@ void postReview( * GitHub refuses throughout stops waiting once the budget is spent rather than holding its pull * request's dispatcher slot for the sum of every route's backoff. A write refused after that * point takes the path a write GitHub outlasted already takes — the next route, then the review - * body — and {@link #unanchoredFindingsBody} says the budget is why. + * body — and {@link #unanchoredFindingsBody} says the budget is why. The ceiling is {@code + * thrillhousebot.github.write-retry-budget}; zero turns it off. */ void postReview(PostReviewRequest post) { - writeBudget.within( - post.owner() + "/" + post.repo() + " #" + post.prNumber(), () -> publishReview(post)); + GitHubWriteBudget.within( + post.owner() + "/" + post.repo() + " #" + post.prNumber(), + config.github().writeRetryBudget(), + () -> publishReview(post)); } private void publishReview(PostReviewRequest post) { diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteBudgetTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteBudgetTest.java index dac97079..1e443147 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteBudgetTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteBudgetTest.java @@ -16,11 +16,11 @@ package dev.thiagogonzaga.thrillhousebot.github; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import java.time.Duration; +import java.util.ArrayList; import java.util.Arrays; import java.util.List; import java.util.Optional; @@ -47,6 +47,7 @@ class GitHubWriteBudgetTest { private static final String REVIEW = "o/r #7"; private static final String OPERATION = "an inline comment on o/r #7"; private static final Duration FOUR_SECONDS = Duration.ofSeconds(4); + private static final Duration SIX_SECONDS = Duration.ofSeconds(6); private final List logged = new CopyOnWriteArrayList<>(); private final Logger julLogger = Logger.getLogger(GitHubWriteBudget.class.getName()); @@ -100,18 +101,18 @@ void outsideAnyReviewEveryWaitIsAdmitted() { @Test void aReviewIsStoppedOnceItsWaitingCrossesTheBudget() { - var budget = new GitHubWriteBudget(Duration.ofSeconds(6)); - var admitted = new java.util.ArrayList(); + var admitted = new ArrayList(); - budget.within( + GitHubWriteBudget.within( REVIEW, + SIX_SECONDS, () -> { admitted.add(GitHubWriteBudget.admits(OPERATION, FOUR_SECONDS)); assertEquals(Optional.empty(), GitHubWriteBudget.exhausted(), "4s of 6s is not spent"); // The wait that crosses the ceiling is still served — the overrun is bounded by one // clamped wait — and it is the last one that is. admitted.add(GitHubWriteBudget.admits(OPERATION, FOUR_SECONDS)); - assertEquals(Optional.of(Duration.ofSeconds(6)), GitHubWriteBudget.exhausted()); + assertEquals(Optional.of(SIX_SECONDS), GitHubWriteBudget.exhausted()); admitted.add(GitHubWriteBudget.admits(OPERATION, Duration.ofSeconds(1))); }); @@ -123,10 +124,9 @@ void aReviewIsStoppedOnceItsWaitingCrossesTheBudget() { @Test void crossingTheBudgetIsWarnedAboutOnceNamingTheReviewAndTheBudget() { - var budget = new GitHubWriteBudget(Duration.ofSeconds(1)); - - budget.within( + GitHubWriteBudget.within( REVIEW, + Duration.ofSeconds(1), () -> { GitHubWriteBudget.admits(OPERATION, Duration.ofSeconds(1)); GitHubWriteBudget.admits(OPERATION, Duration.ofSeconds(1)); @@ -142,10 +142,9 @@ void crossingTheBudgetIsWarnedAboutOnceNamingTheReviewAndTheBudget() { @Test void aZeroBudgetTurnsTheCeilingOff() { - var budget = new GitHubWriteBudget(Duration.ZERO); - - budget.within( + GitHubWriteBudget.within( REVIEW, + Duration.ZERO, () -> { assertTrue(GitHubWriteBudget.admits(OPERATION, Duration.ofHours(1))); assertEquals(Optional.empty(), GitHubWriteBudget.exhausted()); @@ -156,16 +155,16 @@ void aZeroBudgetTurnsTheCeilingOff() { @Test void aNestedScopeRejoinsTheOpenReviewRatherThanStartingALedgerOfItsOwn() { - var outer = new GitHubWriteBudget(Duration.ofSeconds(6)); - var inner = new GitHubWriteBudget(Duration.ofHours(1)); - var admitted = new java.util.ArrayList(); + var admitted = new ArrayList(); - outer.within( + GitHubWriteBudget.within( REVIEW, + SIX_SECONDS, () -> { admitted.add(GitHubWriteBudget.admits(OPERATION, FOUR_SECONDS)); - inner.within( + GitHubWriteBudget.within( REVIEW, + Duration.ofHours(1), () -> { // Charged to the review already open, under its budget, not to a fresh hour. admitted.add(GitHubWriteBudget.admits(OPERATION, FOUR_SECONDS)); @@ -173,7 +172,7 @@ void aNestedScopeRejoinsTheOpenReviewRatherThanStartingALedgerOfItsOwn() { }); // The inner scope closing does not close the review it joined. admitted.add(GitHubWriteBudget.admits(OPERATION, FOUR_SECONDS)); - assertEquals(Optional.of(Duration.ofSeconds(6)), GitHubWriteBudget.exhausted()); + assertEquals(Optional.of(SIX_SECONDS), GitHubWriteBudget.exhausted()); }); assertEquals(List.of(true, true, false, false), admitted); @@ -182,13 +181,12 @@ void aNestedScopeRejoinsTheOpenReviewRatherThanStartingALedgerOfItsOwn() { @Test void theScopeIsClosedEvenWhenTheReviewThrows() { - var budget = new GitHubWriteBudget(Duration.ofSeconds(1)); - assertThrows( IllegalStateException.class, () -> - budget.within( + GitHubWriteBudget.within( REVIEW, + Duration.ofSeconds(1), () -> { GitHubWriteBudget.admits(OPERATION, Duration.ofSeconds(1)); throw new IllegalStateException("the review failed"); @@ -197,15 +195,4 @@ void theScopeIsClosedEvenWhenTheReviewThrows() { assertEquals(Optional.empty(), GitHubWriteBudget.exhausted()); assertTrue(GitHubWriteBudget.admits(OPERATION, FOUR_SECONDS)); } - - @Test - void theSharedBudgetIsTheConfiguredOneAndDefaultsToFiveMinutes() { - // Nothing in the test configuration sets the key, so the shared instance carries the default - // the README documents. Read into locals first so neither constant sits in the expected slot. - var configured = GitHubWriteBudget.SHARED.budget(); - var documented = Duration.ofMinutes(5); - - assertEquals(documented, configured); - assertFalse(configured.isZero(), "the ceiling is on by default"); - } } diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetryTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetryTest.java index 7de81594..f4b942cb 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetryTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetryTest.java @@ -1050,20 +1050,20 @@ private WebApplicationException throttledCall(AtomicInteger calls) { void aThrottledWriteIsNotRepeatedOnceTheReviewHasSpentItsBudget() { var calls = new AtomicInteger(); - new GitHubWriteBudget(Duration.ofSeconds(6)) - .within( - "o/r #7", - () -> { - throttledCall(calls); - // The first wait leaves 2s; the second crosses the ceiling and is still served; - // the third is refused, so the call ends after three attempts rather than four. - assertEquals(3, calls.get()); - assertEquals(List.of(Duration.ofSeconds(4), Duration.ofSeconds(4)), slept); - - // The next write of the same review is not repeated at all. - throttledCall(calls); - assertEquals(4, calls.get()); - }); + GitHubWriteBudget.within( + "o/r #7", + Duration.ofSeconds(6), + () -> { + throttledCall(calls); + // The first wait leaves 2s; the second crosses the ceiling and is still served; + // the third is refused, so the call ends after three attempts rather than four. + assertEquals(3, calls.get()); + assertEquals(List.of(Duration.ofSeconds(4), Duration.ofSeconds(4)), slept); + + // The next write of the same review is not repeated at all. + throttledCall(calls); + assertEquals(4, calls.get()); + }); assertEquals(List.of(Duration.ofSeconds(4), Duration.ofSeconds(4)), slept); var stops = warnings().stream().filter(line -> line.contains("write-retry budget")).toList(); @@ -1078,13 +1078,13 @@ void theStopIsSilentWhenWarningsAreOff() { julLogger.setLevel(Level.OFF); var calls = new AtomicInteger(); - new GitHubWriteBudget(Duration.ofSeconds(1)) - .within( - "o/r #7", - () -> { - throttledCall(calls); - assertEquals(2, calls.get()); - }); + GitHubWriteBudget.within( + "o/r #7", + Duration.ofSeconds(1), + () -> { + throttledCall(calls); + assertEquals(2, calls.get()); + }); assertEquals(List.of(Duration.ofSeconds(4)), slept); assertTrue(logged.isEmpty(), logged.toString()); @@ -1094,7 +1094,7 @@ void theStopIsSilentWhenWarningsAreOff() { void aReviewThatNeverCrossesItsBudgetKeepsTheFullPerCallBackoff() { var calls = new AtomicInteger(); - new GitHubWriteBudget(Duration.ofHours(1)).within("o/r #7", () -> throttledCall(calls)); + GitHubWriteBudget.within("o/r #7", Duration.ofHours(1), () -> throttledCall(calls)); assertEquals(4, calls.get()); assertEquals( diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java index ef2b4f31..d68c5d90 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java @@ -34,6 +34,7 @@ import dev.thiagogonzaga.thrillhousebot.review.ai.TokenCounter; import jakarta.ws.rs.WebApplicationException; import jakarta.ws.rs.core.Response; +import java.time.Duration; import java.util.List; import java.util.Map; import java.util.concurrent.ExecutorService; @@ -140,6 +141,7 @@ void setUp() { ThrillhouseConfig.GitHubConfig githubConfig = mock(ThrillhouseConfig.GitHubConfig.class); when(config.github()).thenReturn(githubConfig); when(githubConfig.botLogins()).thenReturn(List.of(BOT_LOGIN)); + when(githubConfig.writeRetryBudget()).thenReturn(Duration.ofMinutes(5)); diffFormatter = new ReviewDiffFormatter(List.of(), 5000); reviewPublisher = new ReviewPublisher( diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisherTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisherTest.java index aeea3a78..a78993fe 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisherTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisherTest.java @@ -35,6 +35,7 @@ import dev.thiagogonzaga.thrillhousebot.github.GitHubReviewClient; import dev.thiagogonzaga.thrillhousebot.github.ReviewThreadService; import dev.thiagogonzaga.thrillhousebot.review.ai.ReviewResponse; +import java.time.Duration; import java.util.List; import java.util.Map; import org.junit.jupiter.api.Test; @@ -56,6 +57,8 @@ class ReviewPublisherTest { private final ThrillhouseConfig.ReviewConfig reviewConfig = mock(ThrillhouseConfig.ReviewConfig.class); + private final ThrillhouseConfig.GitHubConfig githubConfig = + mock(ThrillhouseConfig.GitHubConfig.class); private final ThrillhouseConfig.FollowUpSummaryConfig followUpSummaryConfig = mock(ThrillhouseConfig.FollowUpSummaryConfig.class); @@ -73,6 +76,8 @@ class ReviewPublisherTest { private void followUpSummaryEnabled(boolean enabled) { when(config.review()).thenReturn(reviewConfig); + when(config.github()).thenReturn(githubConfig); + when(githubConfig.writeRetryBudget()).thenReturn(Duration.ofMinutes(5)); when(reviewConfig.followUpSummary()).thenReturn(followUpSummaryConfig); when(followUpSummaryConfig.enabled()).thenReturn(enabled); } diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewWriteBudgetTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewWriteBudgetTest.java index 82201fc2..c3223bdc 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewWriteBudgetTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewWriteBudgetTest.java @@ -28,7 +28,6 @@ import dev.thiagogonzaga.thrillhousebot.config.ThrillhouseConfig; import dev.thiagogonzaga.thrillhousebot.github.GitHubCommentClient; import dev.thiagogonzaga.thrillhousebot.github.GitHubReviewClient; -import dev.thiagogonzaga.thrillhousebot.github.GitHubWriteBudget; import dev.thiagogonzaga.thrillhousebot.github.ReviewThreadService; import jakarta.ws.rs.WebApplicationException; import jakarta.ws.rs.core.Response; @@ -44,9 +43,9 @@ * *

Drives {@link GitHubReviewClient}'s own {@code default} methods for the reason {@code * RescuedFindingLostWriteTest} does: the budget is charged inside the retry those methods run, so a - * mocked client would stub the very seam under test away. Only the client is real; the budget is - * handed to the publisher small enough to cross in one wait, and GitHub names a one-second deadline - * so that wait is real but short. + * mocked client would stub the very seam under test away. Only the client is real; the configured + * budget is small enough to cross in one wait, and GitHub names a one-second deadline so that wait + * is real but short. */ class ReviewWriteBudgetTest { @@ -59,7 +58,8 @@ class ReviewWriteBudgetTest { private final int prNumber = PR_NUMBERS.incrementAndGet(); private final FakeReviewClient reviewClient = new FakeReviewClient(); - private ReviewPublisher publisher(GitHubWriteBudget budget) { + /** A publisher whose review publishes under {@code budget} — the configured ceiling. */ + private ReviewPublisher publisher(Duration budget) { var suggestionFormatter = mock(SuggestionFormatter.class); when(suggestionFormatter.formatReviewComment(any(), anyBoolean(), anyInt())) .thenReturn("the finding"); @@ -67,6 +67,9 @@ private ReviewPublisher publisher(GitHubWriteBudget budget) { var reviewConfig = mock(ThrillhouseConfig.ReviewConfig.class); when(config.review()).thenReturn(reviewConfig); when(reviewConfig.maxReviewComments()).thenReturn(10); + var githubConfig = mock(ThrillhouseConfig.GitHubConfig.class); + when(config.github()).thenReturn(githubConfig); + when(githubConfig.writeRetryBudget()).thenReturn(budget); return new ReviewPublisher( reviewClient, mock(GitHubCommentClient.class), @@ -75,8 +78,7 @@ private ReviewPublisher publisher(GitHubWriteBudget budget) { mock(FollowUpAnalyzer.class), mock(PrLabeler.class), config, - BotIdentity.of("thrillhousebot[bot]"), - budget); + BotIdentity.of("thrillhousebot[bot]")); } /** @@ -92,7 +94,7 @@ void aReviewStopsRetryingOnceItsBudgetIsSpentAndSaysSoBesideTheFindingsItCouldNo var first = finding("First bug", 10); var second = finding("Second bug", 11); - publisher(new GitHubWriteBudget(Duration.ofSeconds(1))) + publisher(Duration.ofSeconds(1)) .postReview( "Bearer tok", "owner", "repo", prNumber, "sha", result(first, second), resolver()); @@ -114,7 +116,7 @@ void aReviewThatNeverCrossesTheBudgetSaysNothingAboutIt() { reviewClient.retryAfterSeconds = 0; var finding = finding("Only bug", 10); - publisher(new GitHubWriteBudget(Duration.ofHours(1))) + publisher(Duration.ofHours(1)) .postReview("Bearer tok", "owner", "repo", prNumber, "sha", result(finding), resolver()); var body = reviewClient.reviewBodies.getLast(); From d4f4f68e273d4934bc02c754ee13f9f476614405 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Mon, 7 Sep 2026 10:41:27 -0300 Subject: [PATCH 5/7] fix(github): report unrecognised block wording only for a wait that is 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. --- .../github/GitHubWriteRetry.java | 11 ++++-- .../github/GitHubWriteBudgetTest.java | 22 ++++++----- .../github/GitHubWriteRetryTest.java | 37 +++++++++++++++++++ 3 files changed, 56 insertions(+), 14 deletions(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetry.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetry.java index 2ab4eeab..3037f989 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetry.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetry.java @@ -248,9 +248,6 @@ private Optional retryDelay( error.ifPresent(refusal -> warnIfWordingWasMissed(operation, refusal)); return Optional.empty(); } - if (attempt == 1) { - warnIfWordingWasMissed(operation, error.get()); - } if (attempt >= MAX_ATTEMPTS) { if (log.isWarnEnabled()) { log.warn( @@ -273,6 +270,9 @@ private Optional retryDelay( } return Optional.empty(); } + if (attempt == 1) { + warnIfWordingWasMissed(operation, error.get()); + } return Optional.of(delay); } @@ -284,7 +284,10 @@ private Optional retryDelay( * changes here — a hint this loose must not spend repeats — but the body is named, so the next * wording GitHub adopts is added on evidence rather than guessed at under review. Once per call: * the throttled path asks on the first attempt only, since the body does not change between - * attempts. Behind a level check for the reason the give-up line above is. + * attempts, and only once that attempt's wait has been admitted — the block-shaped line says the + * write is retried, and a write the review's budget stops is not, so asking before the budget + * would have the log say both (#734). Behind a level check for the reason the give-up line above + * is. */ private void warnIfWordingWasMissed(String operation, GitHubApiError error) { if (!log.isWarnEnabled()) { diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteBudgetTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteBudgetTest.java index 1e443147..cd290633 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteBudgetTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteBudgetTest.java @@ -181,18 +181,20 @@ void aNestedScopeRejoinsTheOpenReviewRatherThanStartingALedgerOfItsOwn() { @Test void theScopeIsClosedEvenWhenTheReviewThrows() { - assertThrows( - IllegalStateException.class, - () -> - GitHubWriteBudget.within( - REVIEW, - Duration.ofSeconds(1), - () -> { - GitHubWriteBudget.admits(OPERATION, Duration.ofSeconds(1)); - throw new IllegalStateException("the review failed"); - })); + assertThrows(IllegalStateException.class, GitHubWriteBudgetTest::aReviewThatFails); assertEquals(Optional.empty(), GitHubWriteBudget.exhausted()); assertTrue(GitHubWriteBudget.admits(OPERATION, FOUR_SECONDS)); } + + /** A review that spends its whole budget and then fails, so the ledger has to be cleaned up. */ + private static void aReviewThatFails() { + GitHubWriteBudget.within( + REVIEW, + Duration.ofSeconds(1), + () -> { + GitHubWriteBudget.admits(OPERATION, Duration.ofSeconds(1)); + throw new IllegalStateException("the review failed"); + }); + } } diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetryTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetryTest.java index f4b942cb..13ba067b 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetryTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetryTest.java @@ -1072,6 +1072,43 @@ void aThrottledWriteIsNotRepeatedOnceTheReviewHasSpentItsBudget() { assertTrue(stops.getFirst().contains("status=403"), stops.toString()); } + @Test + void aStoppedWriteIsNotAlsoReportedAsRetriedOnUnrecognisedBlockWording() { + // The #784 block-miss line says the write is retried without the floor. Once the budget is + // spent it is not retried at all, and a log that said both would contradict itself, so the + // wording report is made only once the wait has actually been admitted. + var body = + "{\"message\":\"You have exceeded a secondary rate limit and have been temporarily" + + " blocked from creating comments.\"}"; + var calls = new AtomicInteger(); + + GitHubWriteBudget.within( + "o/r #7", + Duration.ofSeconds(1), + () -> { + throttledCall(calls); + assertThrows( + WebApplicationException.class, + () -> + retry.call( + "a comment on o/r #7", + () -> { + calls.incrementAndGet(); + throw failure(403, body, "Retry-After", "4"); + })); + }); + + assertEquals(3, calls.get()); + var lines = warnings(); + assertTrue( + lines.stream().noneMatch(line -> line.contains("matched no known content-creation")), + lines.toString()); + assertEquals( + 2, + lines.stream().filter(line -> line.contains("write-retry budget")).count(), + lines.toString()); + } + @Test void theStopIsSilentWhenWarningsAreOff() { // Behind the same level check as the give-up line, because diagnostics() is eager. From d352529d6c3ce33c36bc98d5e81f920f77662824 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Wed, 9 Sep 2026 11:44:51 -0300 Subject: [PATCH 6/7] fix(github): keep one report of a 403 whose wording was not recognised 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. --- .../github/GitHubWriteRetry.java | 36 ------------------- 1 file changed, 36 deletions(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetry.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetry.java index 4c2312ff..3037f989 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetry.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetry.java @@ -248,9 +248,6 @@ private Optional retryDelay( error.ifPresent(refusal -> warnIfWordingWasMissed(operation, refusal)); return Optional.empty(); } - if (attempt == 1) { - warnIfWordingWasMissed(operation, error.get()); - } if (attempt >= MAX_ATTEMPTS) { if (log.isWarnEnabled()) { log.warn( @@ -315,39 +312,6 @@ private void warnIfWordingWasMissed(String operation, GitHubApiError error) { } } - /** - * Writes down a 403 whose body reads like a throttle but matched no known wording (#784). The - * classification is a whitelist of the phrases GitHub is known to send, and a miss used to be - * silent in both directions: a refusal-shaped miss failed fast exactly like a permission 403, and - * a block-shaped miss kept the repeat but lost the floor the budget is sized by. Neither decision - * changes here — a hint this loose must not spend repeats — but the body is named, so the next - * wording GitHub adopts is added on evidence rather than guessed at under review. Once per call: - * the throttled path asks on the first attempt only, since the body does not change between - * attempts. Behind a level check for the reason the give-up line above is. - */ - private void warnIfWordingWasMissed(String operation, GitHubApiError error) { - if (!log.isWarnEnabled()) { - return; - } - if (error.hasUnrecognisedThrottleWording()) { - log.warn( - "GitHub refused {} with a 403 that reads like a rate limit but matched no known throttle" - + " wording — not retried, so the generated content is lost; if this is a throttle," - + " its wording needs adding to GitHubApiError. {}", - operation, - error.diagnostics()); - } else if (error.hasUnrecognisedBlockWording()) { - log.warn( - "GitHub throttled {} with wording that names a block but matched no known" - + " content-creation wording — retried, but without the {}s floor that block is" - + " sized against; if this is the content-creation block, its wording needs adding" - + " to GitHubApiError. {}", - operation, - GitHubApiError.CONTENT_CREATION_BLOCK_MIN_DELAY.toSeconds(), - error.diagnostics()); - } - } - private static Duration min(Duration left, Duration right) { return left.compareTo(right) <= 0 ? left : right; } From 2c3e79b7d256a62f6cfce18a096776e5bc2a8e31 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Tue, 15 Sep 2026 11:25:42 -0300 Subject: [PATCH 7/7] fix(review): stop attributing every unthreaded finding to a spent write-retry budget --- .../thrillhousebot/review/ReviewPublisher.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java index 05548b2f..23d944b6 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java @@ -508,11 +508,14 @@ private static String unanchoredFindingsBody(List findings) { GitHubWriteBudget.exhausted() .ifPresent( budget -> - sb.append(" — this review spent its ") + // The list mixes causes: a finding can be here because GitHub refused its line and + // file routes outright, before or regardless of any budget. The sentence says what + // the budget did without claiming it for every finding below it (#827 review). + sb.append(" — this review also spent its ") .append(budget.toSeconds()) .append( - "s write-retry budget waiting on GitHub's rate limit, so later writes were" - + " not retried; re-run `/review` to post them")); + "s write-retry budget waiting on GitHub's rate limit, and a finding refused" + + " after that was not retried, so re-running `/review` may post it")); sb.append(":\n\n"); appendFindingList(sb, findings); return sb.toString();