fix(review): group the review body's own fallback into one delivery - #751
Merged
Conversation
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
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
🤖 ThrillhouseBot PR SummaryWhat this PR doesWraps ReviewPublisher.createReviewWithFallback's createReview/createComment routes in GitHubReviewClient.asOneComment so the review body counts as one delivery: a body rescued by the comment fallback is no longer announced as lost, and a body no route could deliver is remembered exactly once. Also changes GitHubLostWrites.asOneDelivery so a nested group for a different pull request takes over the delivery thread with its own accounting and hands the outer group back in the finally (same-target nesting still reuses the outer group), fixing the silent per-route over-count #748 describes. Description vs. ImplementationNo mismatch found between the PR description and the change. Control-Flow Diagram🔀 Show diagramflowchart TD
A["createReviewWithFallback"] --> B["asOneComment opens delivery scope for PR"]
B --> C["routes run inside scope"]
C --> D{"createReview throttled 403?"}
D -- "yes: refusal marks scope" --> E{"comment fallback lands?"}
D -- "no: review posted" --> F["scope landed; nothing remembered"]
E -- "yes" --> F
E -- "no" --> H["scope refused, never landed"]
H --> I["finally remembers loss once"]
B --> J["asOneDelivery entry: group open?"]
J -- "same target" --> K["routes join the open group"]
J -- "different target" --> L["inner group takes over thread"]
L --> M["inner finally hands outer scope back"]
Changes Overview
Changed Files
Risk Assessment
No new issues found in this PR, but the review cannot be approved until required CI is confirmed green.
|
| Check | Type | Status | Detail |
|---|---|---|---|
| test | check-run | ⏳ Pending | - |
| frontend | check-run | ⏳ Pending | - |
| trivy | check-run | ⏳ Pending | - |
| format | check-run | ⏳ Pending | - |
| dependency-review | check-run | ⏳ Pending | - |
Automated review by ThrillhouseBot. Reply with /review to re-run.
thrillhousebot
Bot
added
bug
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Merged
8 tasks
…ontent-creation floor (#752) ## 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
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
|
This was referenced Aug 16, 2026
Closed
devops-thiago
added a commit
that referenced
this pull request
Aug 17, 2026
…ed inside it (#759) ## What type of PR is this? - [x] 🐛 Bug fix - [ ] ✨ Feature - [ ] 📝 Documentation - [ ] 🔧 Refactor - [ ] 🚀 Performance - [ ] ✅ Test - [ ] 🔒 Security - [ ] 📦 Dependency update - [ ] 🏗️ CI/CD ## Description `GitHubLostWrites.asOneDelivery` held the open delivery in a single-slot `ThreadLocal` and *replaced* it for the duration of a nested group instead of stacking it. While a group for another pull request held the thread, the outer group was not reachable at all: - a route of the outer delivery that ran in that window called `deliveryFor(outerTarget)`, got `null` because the slot held the inner target, and was treated as a post of its own — so when it **landed** it set nothing, and the outer `Delivery.landed` stayed `false`; - the outer group then closed with `refused && !landed` true and announced as lost a piece of content the pull request had actually received. That is #729's headline symptom arriving through the opposite door: a settled delivery whose settlement is invisible. The same single-slot comparison split one pull request's delivery in two on an `A → B → A` nesting, because the innermost group could only see the group directly enclosing it and so opened a second group for `A`. ### The change `GitHubLostWrites` now holds the deliveries open on the thread in an `ArrayDeque<Delivery>`, innermost first, and `deliveryFor(target)` returns the **innermost entry whose target matches** rather than the top entry when its target matches. - Same-target nesting rejoins the group already open for that pull request wherever it sits on the stack. That is a strict superset of the reuse the immediately-enclosing check gave, so `A → A` behaves exactly as before and `A → B → A` is now one delivery for `A` rather than two. - A group for a *different* pull request still cannot rejoin — it goes on the stack **above** the outer one rather than in place of it — so #748's fix is intact and the nested caller keeps its own accounting. - The `finally` pops exactly one entry on every exit path, including `Exception` and `Error`, and drops the deque once it empties so a thread that only posts ungrouped carries none. Rejoining pushes nothing, so a throw out of rejoined routes leaves the delivery they rejoined as it was. Scoped to `GitHubLostWrites`: no caller of `asOneComment` and nothing in `ReviewPublisher` changes. The defect is latent at `73cc33d` (both `asOneComment` callers operate on the current review's pull request), which is the posture #748 was in before #751 added a second caller. ## Related Issues Fixes #756 ## How Has This Been Tested? - [x] Unit tests - [ ] Integration tests - [ ] Manual testing Four new behavioural tests in `GitHubLostWritesTest`, plus two controls. All four are genuinely red on the unfixed tree at `73cc33d` and green with the fix. ### Red — verbatim, on `73cc33d` with the tests applied and the fix not ``` $ ./mvnw -B -o test -Dtest=GitHubLostWritesTest [ERROR] Tests run: 30, Failures: 4, Errors: 0, Skipped: 0, Time elapsed: 0.328 s <<< FAILURE! -- in dev.thiagogonzaga.thrillhousebot.github.GitHubLostWritesTest [ERROR] dev.thiagogonzaga.thrillhousebot.github.GitHubLostWritesTest.anErrorOutOfARejoinedGroupLeavesTheDeliveryItRejoinedOpen -- Time elapsed: 0.013 s <<< FAILURE! org.opentest4j.AssertionFailedError: the error closed a delivery of its own rather than leaving the open one alone, so a route held by it was remembered even though a later route landed: > [!WARNING] > **An earlier reply on this pull request was never posted.** GitHub was rate-limiting the bot and the retries ran out, so work it had already finished was thrown away. If you were waiting on an answer, run the command again. ==> expected: <> but was: <> [!WARNING] > **An earlier reply on this pull request was never posted.** GitHub was rate-limiting the bot and the retries ran out, so work it had already finished was thrown away. If you were waiting on an answer, run the command again.> at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1222) at dev.thiagogonzaga.thrillhousebot.github.GitHubLostWritesTest.anErrorOutOfARejoinedGroupLeavesTheDeliveryItRejoinedOpen(GitHubLostWritesTest.java:663) [ERROR] dev.thiagogonzaga.thrillhousebot.github.GitHubLostWritesTest.aRouteThatLandsInsideAGroupForAnotherPullRequestStillSettlesItsOwnDelivery -- Time elapsed: 0.004 s <<< FAILURE! org.opentest4j.AssertionFailedError: a route of the outer delivery landed while a group for the other pull request was open, and the content it delivered was announced as lost: > [!WARNING] > **An earlier reply on this pull request was never posted.** GitHub was rate-limiting the bot and the retries ran out, so work it had already finished was thrown away. If you were waiting on an answer, run the command again. ==> expected: <> but was: <> [!WARNING] > **An earlier reply on this pull request was never posted.** GitHub was rate-limiting the bot and the retries ran out, so work it had already finished was thrown away. If you were waiting on an answer, run the command again.> at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1222) at dev.thiagogonzaga.thrillhousebot.github.GitHubLostWritesTest.aRouteThatLandsInsideAGroupForAnotherPullRequestStillSettlesItsOwnDelivery(GitHubLostWritesTest.java:554) [ERROR] dev.thiagogonzaga.thrillhousebot.github.GitHubLostWritesTest.anExceptionOutOfARejoinedGroupLeavesTheDeliveryItRejoinedOpen -- Time elapsed: 0.004 s <<< FAILURE! org.opentest4j.AssertionFailedError: the throw closed a delivery of its own rather than leaving the open one alone, so a route held by it was remembered even though a later route landed: > [!WARNING] > **An earlier reply on this pull request was never posted.** GitHub was rate-limiting the bot and the retries ran out, so work it had already finished was thrown away. If you were waiting on an answer, run the command again. ==> expected: <> but was: <> [!WARNING] > **An earlier reply on this pull request was never posted.** GitHub was rate-limiting the bot and the retries ran out, so work it had already finished was thrown away. If you were waiting on an answer, run the command again.> at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1222) at dev.thiagogonzaga.thrillhousebot.github.GitHubLostWritesTest.anExceptionOutOfARejoinedGroupLeavesTheDeliveryItRejoinedOpen(GitHubLostWritesTest.java:623) [ERROR] dev.thiagogonzaga.thrillhousebot.github.GitHubLostWritesTest.aDeliveryNestedInsideAnotherPullRequestsGroupRejoinsItsOwnDelivery -- Time elapsed: 0.004 s <<< FAILURE! org.opentest4j.AssertionFailedError: one pull request's delivery was split in two by the group nested between its halves, so the half that landed settled nothing: > [!WARNING] > **An earlier reply on this pull request was never posted.** GitHub was rate-limiting the bot and the retries ran out, so work it had already finished was thrown away. If you were waiting on an answer, run the command again. ==> expected: <> but was: <> [!WARNING] > **An earlier reply on this pull request was never posted.** GitHub was rate-limiting the bot and the retries ran out, so work it had already finished was thrown away. If you were waiting on an answer, run the command again.> at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1222) at dev.thiagogonzaga.thrillhousebot.github.GitHubLostWritesTest.aDeliveryNestedInsideAnotherPullRequestsGroupRejoinsItsOwnDelivery(GitHubLostWritesTest.java:582) [ERROR] Tests run: 30, Failures: 4, Errors: 0, Skipped: 0 ``` Each failure is the defect exactly as described: content the pull request received announced as lost. The four cover, in order, an `Error` out of a group that rejoins an already-open delivery, a route landing inside a group for another pull request, an `Exception` out of a rejoining group, and the `A → B → A` split. ### Green — with the fix ``` [INFO] Tests run: 3330, Failures: 0, Errors: 0, Skipped: 0 [INFO] BUILD SUCCESS ``` ### Controls (green before and after — not proof of the fix) - `anErrorOutOfAGroupOfItsOwnLeavesNoDeliveryBehindOnTheThread` — an `Error` unwinding through a group that opened its own entry leaves nothing on the thread, so the next post is accounted for on its own rather than swallowed. - `anExceptionOutOfANestedGroupHandsTheOuterOneBack` — a throw out of a nested group for another pull request still hands the outer group back. ### The other direction — a genuine loss must not be forgotten The failure mode this fix must not introduce is the opposite one: a write that really was lost being silently dropped. The existing suite pins that and stays green — `contentNoRouteDeliveredIsAnnouncedExactlyOnce`, `aDeliveryThatWasRefusedRatherThanThrottledAnnouncesNothing`, `aCallForAnotherPullRequestIsNotOneOfThisDeliverysRoutes`, `aDeliveryNestedInsideOneForAnotherPullRequestIsGroupedOnItsOwn`, `aNestedDeliveryIsAnnouncedOnceAndHandsTheOuterOneBack`, plus `RescuedReviewBodyLostWriteTest` and `RescuedFindingLostWriteTest` through the real client `default` methods. Each genuine-loss path still reports exactly once: no route delivered, the fallback itself throttled, a non-throttle refusal, and a loss on one pull request while another pull request's delivery is open. ## 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 - [x] I have updated the documentation accordingly - [x] My changes generate no new warnings or errors ## Screenshots / Logs Gates on `32599b3`: ``` ./mvnw -B spotless:apply BUILD SUCCESS ./mvnw -B clean compile spotbugs:check spotless:check BugInstance size is 0 / BUILD SUCCESS ./mvnw -B clean test Tests run: 3330, Failures: 0, Errors: 0, Skipped: 0 ``` Coverage of the changed main code (jacoco ∩ `git diff -U0 73cc33d...HEAD`): **0 uncovered lines, 0 uncovered branches** across every executable changed line in `GitHubLostWrites.java` — both sides of the empty-stack check, the pop-and-drop, the rejoin short-circuit, and the scan's match / no-match / exhausted outcomes. ## Additional Notes The behavioural change is confined to nesting shapes no caller reaches at `73cc33d`, so nothing about today's posting behaviour moves. The `ThreadLocal` holds a `Deque` that is absent rather than empty while nothing is open, so an ungrouped post still costs one `ThreadLocal.get` returning `null`, exactly as before.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Description
#741 grouped a finding's routes into one delivery, but the review body has the identical shape — one piece of content with more than one route to the pull request — and was left ungrouped.
ReviewPublisher.createReviewWithFallbacktriesreviewClient.createReview(wrapped inGitHubLostWrites.carrying) and, on a definite refusal, preserves the same body as an issue comment viacommentClient.createComment(#704, also wrapped incarrying). GitHub's content-creation block is a 403, so it counts as a refusal: route 1remember()s a loss and route 2'scarryingreads that snapshot back, prepending "an earlier reply on this pull request was never posted" to the very comment that is delivering the rescued body — and remembering one lost body twice when neither route lands. That is #729's headline symptom and its double count, live in production on this route pair.Two changes, which have to land together:
ReviewPublisher.createReviewWithFallbacknow runs its routes insideGitHubReviewClient.asOneComment. The routes themselves moved verbatim into a privatecreateReviewRoutes; no route, ordering or refusal/ambiguity rule changed. With a scope open, a throttledcreateReviewonly marks the delivery refused, so the comment fallback carries an empty notice; the fallback landing marks it delivered, so nothing is remembered; and when no route delivers, the body is remembered exactly once.GitHubLostWrites.asOneDeliveryno longer no-ops on any open scope. It did so unconditionally, including for a scope opened on a different pull request — and a group speaks only for the pull request it was opened on (deliveryForcompares the target), so the inner group got no accounting at all and each of its routes was remembered separately: exactly the per-route over-count A finding rescued by the file-level fallback still posts a false "reply was never posted" notice #729 removed, reintroduced silently for the nested caller. It was unreachable whileasOneCommenthad one caller; change 1 adds the second, so it is fixed here. A group whose target differs from the open one now takes over the thread and hands the outer one back in thefinally. Same-target nesting still reuses the outer group, unchanged.Behaviour that must not regress and does not: a review body no route could deliver is still announced, once; the review body still carries notices left by earlier lost writes; a non-throttle refusal still announces nothing.
Related Issues
Fixes #748
Proof and traces: audit report AUDIT7-A, findings A1 and A2.
How Has This Been Tested?
New
RescuedReviewBodyLostWriteTest(2 tests) drives the realdefaultmethods of both clients — only the*OnceHTTP attempts are faked — because a Mockito mock of the client stubs those defaults away, which is what hid this accounting from every publisher test in the first place. Its throttle is a plain secondary-rate-limit 403 rather than a content-creation block, so #738's floor does not apply and the class does not sleep.Two tests added to
GitHubLostWritesTestfor the nesting half.Red output on unfixed code (
fc54d93)(Angle brackets around the JUnit expected/actual values replaced with square brackets so they survive rendering; everything else is verbatim.)
The first failure is the false notice printed directly above the content it is denying; the second and fourth are the double count. All four pass on this branch.
Gates
spotless:apply→clean compile spotbugs:check spotless:check: BugInstance size is 0, BUILD SUCCESSclean test: Tests run: 3308, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESSgit diff -U0 fc54d93...HEADon changed main code: zero uncovered lines, zero uncovered branchesChecklist
Additional Notes
The javadoc on
asOneCommentandasOneDeliverywas corrected too: the former was written as if it only ever groupscreatePullRequestCommentcalls, and the latter justified the unconditional nesting no-op with an argument that only holds when the targets match.