feat(github): bound a review's waiting on the rate limit with a per-review budget

.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=90s
+# 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

CHANGELOG.md

@@ -24,6 +24,7 @@ All notable changes to ThrillhouseBot.
 
 - **A pull request that touches the native image's inputs builds the native image** (#792): `native-build` ran only on `main`, so the only image a pull request ever built was the JVM one, and a change that links on the JVM and fails under GraalVM (reflection, resource loading, serialization, a swapped YAML engine) reached `main` before anything built it. The `changes` job now also reports whether the pull request's own diff touches `pom.xml`, `src/main/java`, `src/main/resources`, `src/main/docker` or the CI workflow, and `native-build` runs on a pull request when it does. Pushes to `main` build as before. A pull request build keeps no binary, and a skipped run reports a `skipped` conclusion, so the job can be made a required check without blocking a docs or test-only change. The build proves the image links; there are no `@QuarkusIntegrationTest` classes, so a failure that only shows when the code path runs, such as a missing reflection registration, is still not covered (#671)
 - **The artifact download client is built once, not once per download** (#478): `ArtifactZipFetcher` built an `HttpClient` for every coverage-artifact download and closed it in the same call, a selector thread and a fresh TLS handshake each time for a client that carries no per-call state. One client now lives for the life of the bean and is shut down with the application. The per-request timeout stays on the request, no credential is attached to the shared client and the field says so, and the JVM's default proxy selector is read once when the bean is built rather than per download, which changes nothing for a proxy configured through system properties at startup
+- **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.

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 (`<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. Ships equal to the write backoff's total budget | `90s` |
+| `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` |

src/main/java/dev/thiagogonzaga/thrillhousebot/config/ThrillhouseConfig.java

@@ -94,6 +94,18 @@ interface GitHubConfig {
     @WithName("write-max-wait")
     @WithDefault("90s")
     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. Read by
+     * {@code ReviewPublisher} when it opens a review's ledger (see {@code GitHubWriteBudget}).
+     */
+    @WithName("write-retry-budget")
+    @WithDefault("5m")
+    Duration writeRetryBudget();
   }
 
   interface WebhookConfig {

src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteBudget.java

@@ -0,0 +1,138 @@
+/*
+ * 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).
+ *
+ * <p>{@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.
+ *
+ * <p>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.
+ *
+ * <p>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, 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.
+ *
+ * <p>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);
+
+  /** The review open on this thread, if one is. Absent while nothing is. */
+  private static final ThreadLocal<Ledger> 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 GitHubWriteBudget() {}
+
+  /**
+   * 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 static void within(String review, Duration budget, 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<Duration> exhausted() {
+    var ledger = OPEN.get();
+    return ledger != null && ledger.exhausted ? Optional.of(ledger.budget) : Optional.empty();
+  }
+}

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.
  *
+ * <p>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.
+ *
  * <h2>Why the budget is what it is</h2>
  *
  * #722: the budget was three attempts, and its own documentation claimed the resulting 60s was
@@ -223,16 +229,17 @@ private Optional<String> 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.
    *
-   * <p>That line sits behind a level check because {@link GitHubApiError#diagnostics()} builds its
+   * <p>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<Duration> retryDelay(
       String operation, WebApplicationException failure, int attempt) {
@@ -241,9 +248,6 @@ private Optional<Duration> 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(
@@ -255,7 +259,21 @@ private Optional<Duration> 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();
+    }
+    if (attempt == 1) {
+      warnIfWordingWasMissed(operation, error.get());
+    }
+    return Optional.of(delay);
   }
 
   /**
@@ -266,7 +284,10 @@ private Optional<Duration> 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()) {