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/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 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"); + } + } +}