fix(github): keep every open delivery reachable while another is nested inside it

[devops-thiago]

What type of PR is this?

• 🐛 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 A finding rescued by the file-level fallback still posts a false "reply was never posted" notice #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 The review body's own fallback still posts the false "reply was never posted" notice #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?

• 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

• My code follows the project's coding standards
• I have performed a self-review of my own code
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• I have updated the documentation accordingly
• 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.

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

[thrillhousebot]

🤖 ThrillhouseBot PR Summary

What this PR does

Rewrites delivery accounting in GitHubLostWrites so routes of an outer delivery remain resolvable while a group for a different pull request is open inside it: open deliveries are kept on a per-thread deque (innermost first), deliveryFor searches past other-PR groups to find the owning delivery, and same-target nesting rejoins the existing group wherever it sits on the stack instead of opening a second one. Tests cover cross-PR nesting, exceptions and errors out of rejoined groups, and controls for pre-existing behavior.

Description vs. Implementation

No mismatch found between the PR description and the change.

Control-Flow Diagram

🔀 Show diagram

flowchart TD
  A["asOneDelivery(target, routes)"] --> B{"deliveryFor(target) != null?"}
  B -- "yes: rejoin" --> C["run routes; push nothing"]
  B -- "no" --> D["push Delivery onto thread-local deque"]
  D --> E["run routes"]
  E --> F["finally: pop; drop thread-local when empty"]
  F --> G{"refused && !landed?"}
  G -- "yes" --> H["remember(target)"]
  C --> I["recording(target, send)"]
  I --> J{"deliveryFor(target) finds a group?"}
  J -- "yes, past any other-PR groups" --> K["mark that delivery refused/landed"]
  J -- "no" --> L["standalone post path"]

Loading

Changes Overview

• Files changed: 2
• Lines added: +252
• Lines removed: -26

Changed Files

File	Change	Summary
src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubLostWrites.java	Modified	Replaces the single-slot ThreadLocal with a ThreadLocal<Deque> so every open delivery stays reachable; deliveryFor scans past other-PR groups and same-target nesting rejoins instead of pushing a new scope.
src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubLostWritesTest.java	Modified	Adds six tests: a route landing inside another PR's group settles the outer delivery, A-B-A nesting stays one delivery, exceptions/errors out of rejoined groups leave them open, plus two controls for pre-existing behavior.

Risk Assessment

Risk	Count
🔴 Critical	0
🟠 High	0
🟡 Medium	0
🔵 Low	0

No new issues found in this PR, but the review cannot be approved until required CI is confirmed green.

⚠️ Required CI Checks Status

Some required checks are still pending or have failed:

Check	Type	Status	Detail
frontend	check-run	⏳ Pending	-
trivy	check-run	⏳ Pending	-
format	check-run	⏳ Pending	-
test	check-run	⏳ Pending	-
dependency-review	check-run	⏳ Pending	-

---

Automated review by ThrillhouseBot. Reply with /review to re-run.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud