Neither number is GitHub speaking about this block. The reset instant belongs to the * primary 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. + * + *
The floor applies however the delay was arrived at, an explicit {@code Retry-After} included + * (#730). An earlier revision floored only a derived delay, on the reading that a {@code + * Retry-After} is GitHub naming its own deadline — but that header names a deadline for this + * request, 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,10 +233,11 @@ 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. * - *
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. + *
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. * *
The floor applies to every rate-limit header on such a block, including {@code * x-ratelimit-remaining: 0}. Those headers describe the primary window, which a @@ -235,21 +245,28 @@ public boolean isSevere() { * 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)); } /** diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java index dff0085d..8c1cae4c 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java +++ b/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)); } } diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetryTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetryTest.java index c7de29db..790236cd 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetryTest.java +++ b/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();