fix(github): floor a content-creation block's wait however the delay was derived

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

@@ -130,10 +130,19 @@ public final class GitHubApiError {
    *
    * <p>Neither number is GitHub speaking about this block. The reset instant belongs to the
    * <em>primary</em> window, which a secondary limit leaves alone — the run behind #722 is exactly
-   * that, a content-creation block carrying {@code x-ratelimit-remaining=4771}. So a delay that was
-   * derived rather than given is floored here, which is what makes {@link
-   * GitHubWriteRetry#TOTAL_BUDGET} a floor for this failure instead of only a ceiling. An explicit
-   * {@code Retry-After} still wins outright: there GitHub is naming its own deadline.
+   * that, a content-creation block carrying {@code x-ratelimit-remaining=4771}. So the delay is
+   * floored here, which is what makes {@link GitHubWriteRetry#TOTAL_BUDGET} a floor for this
+   * failure instead of only a ceiling.
+   *
+   * <p>The floor applies however the delay was arrived at, an explicit {@code Retry-After} included
+   * (#730). An earlier revision floored only a <em>derived</em> delay, on the reading that a {@code
+   * Retry-After} is GitHub naming its own deadline — but that header names a deadline for <em>this
+   * request</em>, not the width of the block. A {@code Retry-After: 5} on the measured body left
+   * four attempts spread over 15 seconds against a 72-second block: the whole budget spent inside
+   * the window, and less waiting than the linear fallback this floor replaced. So the invariant
+   * held on one branch of the derivation only. Longer is still GitHub's to ask for: a {@code
+   * Retry-After} past the floor wins outright, clamped by {@link
+   * GitHubWriteRetry#MAX_DELAY_PER_ATTEMPT} exactly as before.
    */
   static final Duration CONTENT_CREATION_BLOCK_MIN_DELAY = Duration.ofSeconds(30);
 
@@ -224,32 +233,40 @@ public boolean isSevere() {
    * so a silent throttle still slows down. Never negative — for everything but the block below, a
    * reset already in the past means the window has reopened and the call can go straight back out.
    *
-   * <p>One exception, added in #722: when GitHub is blocking content creation and named no deadline
-   * of its own, a derived delay is floored at {@link #CONTENT_CREATION_BLOCK_MIN_DELAY}. Both
-   * derivations undershoot that block badly — the linear fallback by design, a stale reset instant
-   * by returning nothing at all.
+   * <p>One exception, added in #722 and widened in #730: when GitHub is blocking content creation,
+   * a wait shorter than {@link #CONTENT_CREATION_BLOCK_MIN_DELAY} is lifted to it. Every way of
+   * arriving at a wait undershoots that block badly — the linear fallback by design, a stale reset
+   * instant by returning nothing at all, and a short {@code Retry-After} by pacing one call rather
+   * than describing the block.
    *
    * <p>The floor applies to <em>every</em> rate-limit header on such a block, including {@code
    * x-ratelimit-remaining: 0}. Those headers describe the <em>primary</em> window, which a
    * content-creation block leaves untouched, so a primary window that is also spent says nothing
    * about when creation reopens: a block carrying {@code remaining=0} and a reset ten seconds out
    * would otherwise wait ten seconds a time and spend the whole budget inside the 72-second window
    * this is sized against. An earlier revision carved that case out and reintroduced exactly the
-   * failure the floor exists to prevent.
+   * failure the floor exists to prevent; a second one carved out {@code Retry-After} and reopened
+   * it again, since three waits of five seconds is a smaller budget still.
    */
   public Duration retryDelay(int attempt, Instant now) {
-    var fromHeader = retryAfterSeconds();
-    if (fromHeader.isPresent()) {
-      return atLeastZero(Duration.ofSeconds(fromHeader.get()));
-    }
-    var reset = parseLong(rateLimitReset);
-    var derived =
-        reset.isPresent()
-            ? atLeastZero(Duration.between(now, Instant.ofEpochSecond(reset.get())))
-            : FALLBACK_DELAY.multipliedBy(attempt);
-    return blocksContentCreation() && derived.compareTo(CONTENT_CREATION_BLOCK_MIN_DELAY) < 0
+    var delay =
+        retryAfterSeconds()
+            .map(seconds -> atLeastZero(Duration.ofSeconds(seconds)))
+            .orElseGet(() -> derivedDelay(attempt, now));
+    return blocksContentCreation() && delay.compareTo(CONTENT_CREATION_BLOCK_MIN_DELAY) < 0
         ? CONTENT_CREATION_BLOCK_MIN_DELAY
-        : derived;
+        : delay;
+  }
+
+  /**
+   * The wait GitHub implied rather than named: the reset instant of the exhausted rate-limit window
+   * when one was sent, and otherwise a linear backoff off {@link #FALLBACK_DELAY} so a silent
+   * throttle still slows down.
+   */
+  private Duration derivedDelay(int attempt, Instant now) {
+    return parseLong(rateLimitReset)
+        .map(reset -> atLeastZero(Duration.between(now, Instant.ofEpochSecond(reset))))
+        .orElseGet(() -> FALLBACK_DELAY.multipliedBy(attempt));
   }
 
   /**

src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java

@@ -334,11 +334,31 @@ void isRecognisedFromTheGerundWordingToo() {
       }
 
       @Test
-      void stillYieldsToADeadlineGitHubNamed() {
-        // An explicit Retry-After is GitHub speaking about this block, so the floor must not
-        // override it — not even upwards.
+      void isFlooredEvenWhenGitHubNamedAShorterDeadline() {
+        // #730. A Retry-After names a deadline for THIS request; it does not describe how wide the
+        // block is. Taken literally, three seconds a time spends all four attempts in nine seconds
+        // against a block measured at 72 — a smaller budget than the linear fallback this floor
+        // replaced, which is the #722 failure with a header on it.
         var error =
             GitHubApiError.from(outbound(403, CONTENT_CREATION_BLOCK_BODY, "Retry-After", "3"));
+        assertEquals(Duration.ofSeconds(30), error.retryDelay(1, NOW));
+      }
+
+      @Test
+      void stillYieldsToALongerDeadlineGitHubNamed() {
+        // The floor lifts a wait that undershoots; it must not shorten one. A Retry-After past the
+        // floor is GitHub asking for longer, and it stays — the retry's own ceiling clamps it.
+        var error =
+            GitHubApiError.from(outbound(403, CONTENT_CREATION_BLOCK_BODY, "Retry-After", "60"));
+        assertEquals(Duration.ofSeconds(60), error.retryDelay(1, NOW));
+      }
+
+      @Test
+      void doesNotFloorARetryAfterOnAThrottleThatIsNotThisBlock() {
+        // The floor is sized against this block alone. A milder throttle that names a short
+        // deadline
+        // keeps it, or every secondary limit would hold a PR's dispatcher slot for 30 seconds.
+        var error = GitHubApiError.from(outbound(403, SECONDARY_LIMIT_BODY, "Retry-After", "3"));
         assertEquals(Duration.ofSeconds(3), error.retryDelay(1, NOW));
       }
     }

src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetryTest.java

@@ -633,6 +633,70 @@ class TheMeasuredSecondaryLimitWindow {
     /** The window measured in #722, which the budget has to outlast to be worth having. */
     private static final Duration OBSERVED_BLOCK = Duration.ofSeconds(72);
 
+    /** The body measured in #722, verbatim: the generic wording AND the clause naming the block. */
+    private static final String BLOCK_BODY =
+        "{\"message\":\"You have exceeded a secondary rate limit and have been temporarily blocked"
+            + " from content creation. Please retry your request again later.\"}";
+
+    /**
+     * The block, driven end to end on a clock the recorded waits advance: GitHub keeps refusing
+     * until as much simulated time has passed as the measured window lasted, so what is pinned is
+     * the wall clock the budget actually spans rather than how many attempts it took to get there.
+     *
+     * @param headers what GitHub sends alongside the block body
+     */
+    private void spansTheBlock(String... headers) {
+      var calls = new AtomicInteger();
+      var elapsed = new AtomicLong();
+      var start = Instant.ofEpochSecond(1_800_000_000L);
+      var backoff =
+          new GitHubWriteRetry(
+              wait -> elapsed.addAndGet(wait.toSeconds()), () -> start.plusSeconds(elapsed.get()));
+
+      var result =
+          backoff.call(
+              "an inline comment on o/r #7",
+              () -> {
+                calls.incrementAndGet();
+                if (elapsed.get() < OBSERVED_BLOCK.toSeconds()) {
+                  throw failure(403, BLOCK_BODY, headers);
+                }
+                return "posted";
+              });
+
+      assertEquals("posted", result, "the write GitHub was blocking has to land in the end");
+      assertTrue(
+          elapsed.get() >= OBSERVED_BLOCK.toSeconds(),
+          "the budget has to span the block, not expire inside it — waited only "
+              + elapsed.get()
+              + "s");
+    }
+
+    /**
+     * #730, the branch that was still broken: GitHub names a deadline of its own, and it is far
+     * shorter than the block. Before the floor covered this branch, three waits of five seconds
+     * gave up 15 seconds into a 72-second block — half of what the linear fallback #722 replaced
+     * would have spread.
+     */
+    @Test
+    void aBlockIsOutlastedEvenWhenGitHubNamesAShortDeadline() {
+      spansTheBlock("Retry-After", "5");
+    }
+
+    /**
+     * #722's own shape, kept as the control on the other branch: no {@code Retry-After}, primary
+     * quota nowhere near exhausted, and a reset instant that belongs to that untouched primary
+     * window and so is already in the past. This one passed before #730 and has to keep passing.
+     */
+    @Test
+    void aBlockIsOutlastedWhenGitHubNamesNoDeadlineAtAll() {
+      spansTheBlock(
+          "x-ratelimit-remaining",
+          "4771",
+          "x-ratelimit-reset",
+          String.valueOf(Instant.ofEpochSecond(1_800_000_000L).getEpochSecond() - 90));
+    }
+
     @Test
     void aBlockAsLongAsTheMeasuredOneIsOutlasted() {
       var calls = new AtomicInteger();