From b5aa490f2654da63740ba0aad2869fcf95913d90 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Sun, 16 Aug 2026 18:07:45 +0000 Subject: [PATCH 1/2] fix(review): group the review body's own fallback into one delivery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The review body has the same shape a finding has — one piece of content with more than one route to the pull request — but only the finding's routes were grouped. `createReviewWithFallback` tries `createReview` and then, on a definite refusal, preserves the same body as an issue comment (#704); both ends do their own dropped-post accounting, so a content-creation block (a 403, hence a refusal) made route 1 remember a loss that route 2's `carrying` read straight back. The notice was printed above the very comment that was delivering the rescued body, and one lost body was counted twice when neither route landed — #729's symptom and its double count, still live on this pair. Wrapping the method's routes in `GitHubReviewClient.asOneComment` settles both halves: route 1's throttle now only marks the delivery refused, so route 2 carries an empty notice, and a body no route could deliver is remembered once. That fix also makes a latent trap reachable: `asOneDelivery` no-opped whenever any scope was open, including one opened for a different pull request, which left the inner group's routes with no accounting at all and remembered each of them separately. It was unreachable while `asOneComment` had a single caller; this change adds the second. A group now takes over the thread when the targets differ and hands the outer one back as it closes, so a genuinely lost write is still reported exactly once and the outer group is unaffected. Fixes #748 --- .../github/GitHubLostWrites.java | 18 +- .../github/GitHubReviewClient.java | 15 +- .../review/ReviewPublisher.java | 27 ++ .../github/GitHubLostWritesTest.java | 74 +++++ .../RescuedReviewBodyLostWriteTest.java | 276 ++++++++++++++++++ 5 files changed, 401 insertions(+), 9 deletions(-) create mode 100644 src/test/java/dev/thiagogonzaga/thrillhousebot/review/RescuedReviewBodyLostWriteTest.java diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubLostWrites.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubLostWrites.java index b60f90d9..c82c031b 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubLostWrites.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubLostWrites.java @@ -211,11 +211,17 @@ public T recording(Target target, Supplier send) { * finding that really is lost still announces itself — and announces itself once rather than once * per attempt. * - *

Nesting reuses the outer group rather than opening a second one, so a caller cannot lose - * another caller's routes by grouping its own. + *

Nesting for the same pull request reuses the outer group rather than opening a + * second one, so a caller cannot lose another caller's routes by grouping its own. Nesting for a + * different pull request cannot reuse it: a group speaks only for the pull request it was opened + * on ({@link #deliveryFor}), so handing the inner routes the outer group would leave them with no + * accounting at all and remember each of them separately — the per-route over-count #729 removed, + * reintroduced for the nested caller and silently, with no log and no exception (#748). The inner + * group therefore takes over the thread and hands the outer one back when it closes. */ public T asOneDelivery(Target target, Supplier routes) { - if (delivery.get() != null) { + var outer = delivery.get(); + if (outer != null && outer.target.equals(target)) { return routes.get(); } var scope = new Delivery(target); @@ -223,7 +229,11 @@ public T asOneDelivery(Target target, Supplier routes) { try { return routes.get(); } finally { - delivery.remove(); + if (outer == null) { + delivery.remove(); + } else { + delivery.set(outer); + } if (scope.refused && !scope.landed) { remember(target); } diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubReviewClient.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubReviewClient.java index 11718682..99fddb21 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubReviewClient.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubReviewClient.java @@ -252,18 +252,23 @@ default PullRequestCommentResponse createPullRequestComment( } /** - * Runs {@code routes} — several {@link #createPullRequestComment} calls that are alternative ways - * of getting the same content onto the pull request — as one delivery, so a throttle - * that costs one route its turn is only announced to the pull request if no route delivered the - * content at all (#729). + * Runs {@code routes} — several content-creating calls that are alternative ways of getting the + * same content onto the pull request — as one delivery, so a throttle that costs one + * route its turn is only announced to the pull request if no route delivered the content at all + * (#729). * *

Without this the accounting counts refused HTTP calls rather than lost content: #721's * file-level fallback lands the finding and the review body published moments later still leads * with "an earlier reply on this pull request was never posted … run the command again", twice * over for a finding whose suggestion block earned the line-anchored route a second attempt. * + *

The routes need not all be {@link #createPullRequestComment}: #704's fallback carries a + * refused review body over to {@link GitHubCommentClient#createComment}, and those two are one + * delivery for the same reason (#748). What has to match is the pull request they aim at — a + * group speaks for one pull request only. + * *

Lives here rather than at the call site because the notice registry is this package's, and - * the call it groups is {@link #createPullRequestComment} on this interface. + * {@link #createPullRequestComment} on this interface is the call it was built for. */ static T asOneComment(String owner, String repo, int pullNumber, Supplier routes) { return GitHubLostWrites.SHARED.asOneDelivery( diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java index 144659cf..f6ad93cb 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java @@ -926,6 +926,15 @@ void dismissPendingBotReviews( * ambiguous failure (timeout, connection reset, 5xx) on any attempt may have landed that * attempt's review, so it throws {@link ReviewPostException} instead of risking a duplicate — as * does a refusal whose comment fallback fails too. + * + *

Every route here is a route to the same review body, so they are one delivery + * ({@link GitHubReviewClient#asOneComment}) for exactly the reason a finding's routes are (#729, + * #741): the dropped-post notice counts content the pull request never received, and a body the + * comment fallback rescued was received. Ungrouped, a content-creation block — a 403, so a + * definite refusal — made {@code createReview} remember a loss that the fallback's own {@code + * carrying} then read back, printing "an earlier reply on this pull request was never posted" + * above the very comment delivering the body, and counting one lost body twice when neither route + * landed (#748). */ void createReviewWithFallback( String auth, @@ -933,6 +942,24 @@ void createReviewWithFallback( String repo, int prNumber, GitHubReviewClient.CreateReviewRequest req) { + GitHubReviewClient.asOneComment( + owner, + repo, + prNumber, + () -> { + createReviewRoutes(auth, owner, repo, prNumber, req); + // The routes report through their return or their exception, not through a value. + return null; + }); + } + + /** The routes themselves, in preference order. See {@link #createReviewWithFallback}. */ + private void createReviewRoutes( + String auth, + String owner, + String repo, + int prNumber, + GitHubReviewClient.CreateReviewRequest req) { RuntimeException rejection; boolean anyAmbiguous; try { diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubLostWritesTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubLostWritesTest.java index 4b18e0c1..104b66aa 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubLostWritesTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubLostWritesTest.java @@ -459,6 +459,80 @@ void aDeliveryNestedInsideAnotherIsOneDelivery() { assertEquals("", carried.getLast(), carried.getLast()); } + /** + * #748. A group inside a group for a different pull request is a group of its own: the + * outer delivery speaks only for its own pull request, so reusing it would leave the inner routes + * with no accounting at all and announce content the inner group delivered as lost. + */ + @Test + void aDeliveryNestedInsideOneForAnotherPullRequestIsGroupedOnItsOwn() { + var carried = new ArrayList(); + + lost.asOneDelivery( + PR, + () -> + lost.asOneDelivery( + OTHER_PR, + () -> { + assertThrows( + WebApplicationException.class, + () -> lost.recording(OTHER_PR, () -> throwIt(throttled()))); + return lost.recording(OTHER_PR, () -> "posted"); + })); + post(OTHER_PR, carried, null); + + assertEquals( + "", + carried.getLast(), + () -> + "the inner delivery was not grouped, so content its second route delivered was" + + " announced as lost: " + + carried.getLast()); + } + + /** + * The other direction of the same nesting, and the outer delivery on the far side of it: the + * inner group really is lost and is announced exactly once, while the outer group is handed back + * intact — a route it lost before the inner group ran is still only held, not remembered, and a + * later route of its own settles it. + */ + @Test + void aNestedDeliveryIsAnnouncedOnceAndHandsTheOuterOneBack() { + var carried = new ArrayList(); + + lost.asOneDelivery( + PR, + () -> { + assertThrows( + WebApplicationException.class, () -> lost.recording(PR, () -> throwIt(throttled()))); + lost.asOneDelivery( + OTHER_PR, + () -> { + for (var route = 0; route < 2; route++) { + assertThrows( + WebApplicationException.class, + () -> lost.recording(OTHER_PR, () -> throwIt(throttled()))); + } + return "no route landed"; + }); + return lost.recording(PR, () -> "posted"); + }); + post(OTHER_PR, carried, null); + post(PR, carried, null); + + assertTrue(carried.get(0).contains("An earlier reply"), carried.get(0)); + assertFalse( + carried.get(0).contains("earlier replies"), + () -> "one lost nested delivery was counted once per refused route: " + carried.get(0)); + assertEquals( + "", + carried.get(1), + () -> + "the outer delivery lost its scope to the nested one, so a route it landed itself no" + + " longer settled it: " + + carried.get(1)); + } + private static String throwIt(WebApplicationException failure) { throw failure; } diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/RescuedReviewBodyLostWriteTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/RescuedReviewBodyLostWriteTest.java new file mode 100644 index 00000000..c3d4624d --- /dev/null +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/RescuedReviewBodyLostWriteTest.java @@ -0,0 +1,276 @@ +/* + * 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.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; + +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.ReviewThreadService; +import jakarta.ws.rs.WebApplicationException; +import jakarta.ws.rs.core.Response; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.atomic.AtomicInteger; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * #748 — what the pull request is told about a review body that its own comment fallback rescued. + * + *

{@link RescuedFindingLostWriteTest} pins the same question for a finding, whose routes #741 + * grouped into one delivery. The review body has the identical shape — {@code createReview}, then + * the issue comment carrying the same body (#704) — and was left ungrouped, so #729's symptom and + * its double count were still live on this pair. + * + *

Both clients' own {@code default} methods run here, only the {@code *Once} HTTP attempts are + * faked: a mock of the client stubs the {@code default} methods away, which is precisely what hid + * the accounting from every publisher test until #729. + */ +class RescuedReviewBodyLostWriteTest { + + /** + * A plain secondary-rate-limit body rather than a content-creation block, so the retry derives + * its delay from {@code Retry-After} with no floor applied (#738) and the test does not sleep. + */ + private static final String SECONDARY_LIMIT_BODY = + "{\"message\":\"You have exceeded a secondary rate limit.\"}"; + + private static final String REVIEW_BODY = "ThrillhouseBot requested changes — see inline."; + + private static final String NOTICE = "An earlier reply on this pull request was never posted"; + + /** + * The registry the clients write to is process-wide, so each test takes a pull request of its own + * rather than reading another's bookkeeping. + */ + private static final AtomicInteger PR_NUMBERS = new AtomicInteger(800); + + private int prNumber; + private ThrottledReviewClient reviewClient; + private FakeCommentClient commentClient; + private ReviewPublisher publisher; + + @BeforeEach + void setUp() { + prNumber = PR_NUMBERS.incrementAndGet(); + reviewClient = new ThrottledReviewClient(); + commentClient = new FakeCommentClient(); + publisher = + new ReviewPublisher( + reviewClient, + commentClient, + mock(ReviewThreadService.class), + mock(SuggestionFormatter.class), + mock(FollowUpAnalyzer.class), + mock(PrLabeler.class), + mock(ThrillhouseConfig.class), + BotIdentity.of("thrillhousebot[bot]")); + } + + private static WebApplicationException throttled() { + return new WebApplicationException( + Response.status(403).header("Retry-After", "0").entity(SECONDARY_LIMIT_BODY).build()); + } + + private void publishReviewBody() { + publisher.createReviewWithFallback( + "Bearer token", + "owner", + "repo", + prNumber, + new GitHubReviewClient.CreateReviewRequest("sha", REVIEW_BODY, "COMMENT", List.of())); + } + + /** + * The production sequence: GitHub is refusing content creation, {@code createReview} burns its + * whole retry budget, and #704's issue-comment fallback lands the same body on the far side of + * the window. The review body is on the pull request, so the comment that delivered it must not + * open by telling the maintainer it was never posted. + */ + @Test + void aReviewBodyTheCommentFallbackRescuedIsNotAnnouncedAsLost() { + publishReviewBody(); + + var landed = commentClient.bodies.getLast(); + assertTrue( + landed.contains(REVIEW_BODY), + () -> "the fallback did not carry the review body: " + landed); + assertFalse( + landed.contains(NOTICE), + () -> + "the review body was delivered by its comment fallback, and the very comment that" + + " delivered it opens by telling the maintainer it was never posted:\n" + + landed); + } + + /** + * #729's other half on this route pair: when neither route lands, one lost review body is one + * loss, not one per refused route. + */ + @Test + void aReviewBodyNoRouteCouldDeliverIsAnnouncedExactlyOnce() { + commentClient.throttle = true; + + assertThrows(ReviewPostException.class, this::publishReviewBody); + + // Whatever the bot lands on the pull request next is what carries the notice. + commentClient.throttle = false; + commentClient.createComment( + "Bearer token", + "application/vnd.github+json", + "owner", + "repo", + prNumber, + new GitHubCommentClient.CreateCommentRequest("the next thing the bot posts")); + + var carried = commentClient.bodies.getLast(); + assertTrue( + carried.contains(NOTICE), + () -> "the genuinely lost review body must still be announced:\n" + carried); + assertFalse( + carried.contains("earlier replies on this pull request were never posted"), + () -> "one lost review body was counted once per refused route:\n" + carried); + } + + /** Every {@code createReview} attempt is throttled away; nothing else is exercised. */ + private static final class ThrottledReviewClient implements GitHubReviewClient { + @Override + public ReviewResponse createReviewOnce( + String auth, + String accept, + String owner, + String repo, + int pullNumber, + CreateReviewRequest request) { + throw throttled(); + } + + @Override + public PullRequestCommentResponse createPullRequestCommentOnce( + String auth, + String accept, + String owner, + String repo, + int pullNumber, + CreatePullRequestCommentRequest request) { + throw new UnsupportedOperationException("not part of this seam"); + } + + @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"); + } + } + + /** The fallback surface, real {@code createComment} default and all, so {@code carrying} runs. */ + private static final class FakeCommentClient implements GitHubCommentClient { + private final List bodies = new ArrayList<>(); + private boolean throttle; + + @Override + public CommentResponse createCommentOnce( + String auth, + String accept, + String owner, + String repo, + int issueNumber, + CreateCommentRequest request) { + if (throttle) { + throw throttled(); + } + bodies.add(request.body()); + return new CommentResponse(1L, "https://example.invalid/1"); + } + + @Override + public List listCommentsPageOnce( + String auth, + String accept, + String owner, + String repo, + int issueNumber, + int perPage, + int page) { + return List.of(); + } + + @Override + public IssueDetails getIssueOnce( + String auth, String accept, String owner, String repo, int issueNumber) { + throw new UnsupportedOperationException("not part of this seam"); + } + + @Override + public CommentResponse updateCommentOnce( + String auth, + String accept, + String owner, + String repo, + long commentId, + CreateCommentRequest request) { + throw new UnsupportedOperationException("not part of this seam"); + } + } +} From ca10dac5b614d3a9a8e6bef3f2cec21cd0118b8d Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga <2332561+devops-thiago@users.noreply.github.com> Date: Sun, 16 Aug 2026 15:52:13 -0300 Subject: [PATCH 2/2] test(review): stop RescuedFindingLostWriteTest sleeping through the content-creation floor (#752) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What type of PR is this? - [x] ✅ Test > **Stacked PR.** Based on `fix/748-review-body-delivery` (#751) so its diff stays scoped to this issue. **Re-target to `main` once #751 merges.** ## Description `RescuedFindingLostWriteTest`'s fixture threw GitHub's content-creation block with `Retry-After: 0` and documented that as "naming a deadline of 'now' so the test does not sleep". That was true when it was written. #738 then made `CONTENT_CREATION_BLOCK_MIN_DELAY` (30 s) apply **however the delay was derived**, an explicit `Retry-After` included — and shipped in the same release. Every attempt of every exhausted route has waited the floor ever since: 3 sleeps per route, 6 exhausted routes across the class. Neither change is wrong. The fixture simply encodes an assumption that stopped being true. What these tests actually need from the throttle is that `GitHubApiError.isThrottled` says yes and that the route burns `GitHubWriteRetry.MAX_ATTEMPTS`. A plain secondary rate limit does both, and nothing in the class asserts on the block wording — the assertions are all about accounting. So `BLOCK_BODY` becomes `THROTTLE_BODY`, a plain secondary-limit message, with a comment on why it is deliberately not a content-creation block so the next reader does not "restore" it. All three tests and every assertion in them are unchanged. ## Related Issues Fixes #749 Proof: audit report AUDIT7-A, finding A3. ## How Has This Been Tested? - [x] Unit tests This is a build-cost fix with no behavioural change, so there is no red/green proof to quote — the three tests pass before and after, on the same assertions, and that is the point. The evidence is the measured wall clock, before and after, on the same machine and JVM. ### Before (`surefire` XML, this branch's parent) ``` Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 461.1 s -- in RescuedFindingLostWriteTest CLASS 461.054 aFindingTheFileLevelFallbackRescuedIsNotAnnouncedAsLost 94.013 aRescuedFindingWithASuggestionIsNotAnnouncedTwiceEither 184.004 aFindingNoRouteCouldDeliverIsStillAnnouncedAsLost 182.998 ``` ### After ``` Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 25.97 s -- in RescuedFindingLostWriteTest CLASS 25.974 aFindingTheFileLevelFallbackRescuedIsNotAnnouncedAsLost 6.954 aRescuedFindingWithASuggestionIsNotAnnouncedTwiceEither 9.984 aFindingNoRouteCouldDeliverIsStillAnnouncedAsLost 9.002 ``` **461.05 s → 25.97 s for the class (−435 s, 17.7x), 94.01 s → 6.95 s for the single test.** The 94 s figure was 3 x 30 s of floor plus overhead, to the second; what remains is the retry pacing the seam genuinely exercises. A whole `clean test` on this branch drops from **12:22 to 04:40**. ### Gates - `spotless:apply` → `clean compile spotbugs:check spotless:check`: **BugInstance size is 0**, BUILD SUCCESS - `clean test`: **Tests run: 3308, Failures: 0, Errors: 0, Skipped: 0** — BUILD SUCCESS - Coverage: no main code changed by this PR, so there is nothing new to cover. ## Checklist - [x] My code follows the project's coding standards - [x] I have performed a self-review of my own code - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [ ] I have updated the documentation accordingly - [x] My changes generate no new warnings or errors --- .../review/RescuedFindingLostWriteTest.java | 29 +++++++++++++------ 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/RescuedFindingLostWriteTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/RescuedFindingLostWriteTest.java index a182ff92..652fb1e2 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/RescuedFindingLostWriteTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/RescuedFindingLostWriteTest.java @@ -58,10 +58,21 @@ class RescuedFindingLostWriteTest { private static final String FILE_LEVEL_THREAD = "the finding, filed on its file"; private static final String REVIEW_BODY = "the review body"; - /** The response measured in #722, which is what sends a route round the backoff to exhaustion. */ - 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.\"}"; + /** + * A secondary rate limit, which is what sends a route round the backoff to exhaustion. + * + *

Not the content-creation wording measured in #722, deliberately (#749). The seam under test + * is the accounting, not the backoff: what these tests need from the throttle is that {@link + * dev.thiagogonzaga.thrillhousebot.github.GitHubApiError#isThrottled} says yes and that the route + * exhausts {@code GitHubWriteRetry.MAX_ATTEMPTS}, both of which a plain secondary limit does. A + * content-creation block additionally lifts every wait to {@code + * CONTENT_CREATION_BLOCK_MIN_DELAY} — 30 seconds — however the delay was derived, {@code + * Retry-After} included since #738. The {@code Retry-After: 0} below used to mean "no sleep" and + * stopped meaning it in the same release, at a cost of 461 s for this class and 94 s for a single + * test. Nothing here asserts on the block wording, so the cheaper body pins the same behaviour. + */ + 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 a pull request of its own @@ -101,7 +112,7 @@ void setUp() { } /** - * The production sequence: GitHub is in a content-creation block, the line-anchored comment burns + * The production sequence: GitHub is refusing comment creation, the line-anchored comment burns * its whole retry budget, the file-level thread lands on the far side of the window, and the * review body goes out moments later. The finding has a working thread, so the body must not open * with "an earlier reply on this pull request was never posted … run the command again". @@ -140,10 +151,10 @@ void aRescuedFindingWithASuggestionIsNotAnnouncedTwiceEither() { } /** - * The other direction, which the fix must not cost: when the block outlasts every route the + * The other direction, which the fix must not cost: when the throttle outlasts every route the * finding really is gone, and the maintainer does have to run the command again. Said once, for * one finding, rather than once per refused route — the over-count is not confined to the rescued - * case, and a review that lost three findings to a wide block should say three, not nine. + * case, and a review that lost three findings to a wide window should say three, not nine. */ @Test void aFindingNoRouteCouldDeliverIsStillAnnouncedAsLost() { @@ -275,10 +286,10 @@ public ReviewResponse createReviewOnce( return new ReviewResponse(1L, request.body(), request.event(), request.commitId(), null); } - /** GitHub's content-creation block, naming a deadline of "now" so the test does not sleep. */ + /** GitHub throttling the post, naming a deadline of "now" so the test does not sleep. */ private static WebApplicationException blocked() { return new WebApplicationException( - Response.status(403).header("Retry-After", "0").entity(BLOCK_BODY).build()); + Response.status(403).header("Retry-After", "0").entity(THROTTLE_BODY).build()); } @Override