feat(github): tell the pull request when a throttled reply was dropped - #598
Open
devops-thiago wants to merge 3 commits into
Open
feat(github): tell the pull request when a throttled reply was dropped#598devops-thiago wants to merge 3 commits into
devops-thiago wants to merge 3 commits into
Conversation
The backoff from #568 is bounded, so a persistently throttled reply is still dropped once the attempts are spent. That was logged and nothing else: from the PR the command simply never answered, which is indistinguishable from the bot ignoring the user, and nothing told them the right move is to run it again. The obvious remedy — reply "this was throttled, please re-run" — is itself a createComment: the exact call being throttled, sent at the moment GitHub is refusing it. So the notice is not posted on its own. GitHubLostWrites holds it, and the next content the bot successfully lands on that pull request carries it up front. That costs no additional content-creating request, cannot be throttled separately from the post it travels with, and appears where the loss happened rather than in a log the user cannot read. Comments and reviews carry a notice; inline comments and thread replies are anchored to a diff line, so they leave one behind without carrying one. Only a positively identified throttle counts — a permission refusal is a defect to fix, not a command to re-run. A notice is cleared only once the post carrying it has actually landed, is forgotten after six hours rather than being glued onto a much later comment, and the registry is bounded so a flood of losing pull requests cannot grow it without end. Fixes #578
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
🤖 ThrillhouseBot PR SummaryWhat this PR doesRecords throttled writes that exhaust the retry budget in a per-PR registry, prepends a Markdown warning to the next successful comment or review body, and records losses from inline comments and thread replies without carrying the notice. Control-Flow Diagram🔀 Show diagramflowchart TD
A["Throttled post dropped after retry budget"]
B["GitHubLostWrites.recording catches WebApplicationException"]
C{"GitHubApiError.isThrottled?"}
D["remember target in pending registry"]
E["Next createComment or createReview on same PR"]
F["carrying reads pendingCount"]
G{"count greater than 0?"}
H["prepend notice above body"]
I["GitHubWriteRetry sends post"]
J{"post succeeds?"}
K["settle subtracts carried notices"]
L["Notice stays pending for next post"]
A --> B
B --> C
C -- "yes" --> D
D --> E
E --> F
F --> G
G -- "yes" --> H
H --> I
I --> J
J -- "yes" --> K
J -- "no" --> L
Changes Overview
Changed Files
Risk Assessment
Key Findings
|
| Check | Type | Status | Detail |
|---|---|---|---|
| test | check-run | ⏳ Pending | - |
| actionlint | check-run | ⏳ Pending | - |
| trivy | check-run | ⏳ Pending | - |
| changes | check-run | ⏳ Pending | - |
| frontend | check-run | ⏳ Pending | - |
| format | check-run | ⏳ Pending | - |
| dependency-review | check-run | ⏳ Pending | - |
Automated review by ThrillhouseBot. Reply with /review to re-run.
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
thrillhousebot
Bot
added
enhancement
Subtracting each carrier's snapshot from one shared count loses a notice when two posts on the same pull request overlap. Both read pending=1 and carry notice(1); a third post is thrown away between their reads, taking the count to 2; the first carrier to settle subtracts to 1 and the second finds 1 > 1 false and removes the entry. The loss recorded between them was announced by nobody and is now gone, so the user is never told their content was dropped — the exact silence this feature exists to remove. Track two monotonic watermarks instead, lost and announced, with pending their difference. A delivered post advances announced towards the lost value it actually carried and the watermark never moves backwards, so a second carrier holding the same snapshot leaves the entry alone. The overlap can now only repeat a notice, never drop one, which is the harmless direction. No lock: serializing carrying per target would hold it across an HTTP call that may itself back off for up to a minute.
🤖 ThrillhouseBot — changes since the last review
|
Moving to watermarks left a fully announced entry in the map at zero pending, where only the six-hour TTL could retire it. Until then it still counted towards the cap, so once that many pull requests had been told about a loss inside one window, the next pull request to lose a post was only logged — silence again, reached from the other end, and with nothing outstanding to protect the slots it was denied. Drop an entry as soon as its last outstanding notice is delivered, so a slot is held only while a pull request is genuinely still owed one. Deleting it needs an identity guard, because both watermarks restart when a later loss recreates the entry: a carrier still in flight from the previous run holds a snapshot whose lost count can equal the new run's and would retire a loss it never carried. Each run of losses now carries an id, and a delivery only counts when the id still matches. Removing that check alone makes aCarrierLeftOverFromASettledEntryCannotRetireALaterLoss fail, so it is load-bearing rather than defensive.
|
🤖 ThrillhouseBot — changes since the last review
|
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
The backoff from #568 is bounded, so a persistently throttled reply is still dropped once the
attempts are spent. Today that is logged and nothing else: from the pull request the command simply
never answered, which is indistinguishable from the silent-decline class #538 fixed. The user cannot
tell whether the bot ignored them, crashed, or lost the post — and nothing says the right move is to
run it again.
The catch the issue names. The obvious remedy — reply "this was throttled, please re-run" — is
itself a
createComment: the exact call being throttled, sent at the exact moment GitHub is refusingit. Giving it its own budget just spends more attempts on the same refusal.
So the notice is never posted on its own.
GitHubLostWritesholds it, and the next content thebot successfully lands on that pull request carries it up front:
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.
That costs no additional content-creating request — which is the whole point of #579 — cannot be
throttled separately from the post it travels with, and appears on the pull request the loss
happened on rather than in a log the user cannot read. Of the three options the issue weighs, this
is the "persist the pending notice and post it on the next successful interaction" one; it also
needs no change in
review/orwebhook/, so every posting path inherits it at the client boundaryinstead of each fail-soft handler having to learn about throttling.
Which calls do what.
createCommentandcreateReviewboth land in the conversation, so theycarry a notice. An inline comment and a thread reply are anchored to a diff line and are a poor place
to announce an unrelated loss, so they leave a notice behind without carrying one — losing an inline
finding is still a loss the PR should hear about.
updateCommentdoes neither: it identifies itstarget by comment id, so this layer cannot tell which pull request it belongs to.
What it deliberately does not claim.
and announcing those would put a warning on every single comment.
/describeor/improvebehind it. An honest "if you were waiting on an answer, run it again"beats a guess.
does not take the notice with it — and a loss that arrives while a notice is in flight is still
announced next time.
than today's log-only behaviour.
Bounded, and quiet when stale. A notice is forgotten after six hours, because "a reply was
dropped" glued onto a comment days later is noise rather than a signal, and the registry holds at
most 200 pull requests so a sustained outage cannot grow it without end.
Why prepending is safe.
ReviewContextLoader.isBotSummaryCommentmatches the heading on anyline, not at the start of the body — precisely because the truncation banner already precedes it —
and no code in the repository matches a comment body with
startsWith.How this composes with #568 and #579. #579's pacer keeps the burst from being produced, #568's
backoff absorbs the throttling pacing cannot prevent, and this is the last resort for when even that
runs out. The three are strictly ordered by cost, and this one is the only one the user ever sees.
Related Issues
Fixes #578
How Has This Been Tested?
GitHubLostWritesTestdrives the carrier directly on a hand-held clock: a quiet PR carrying nothing,a dropped reply announced on the next comment, the notice said once rather than on everything
afterwards, a notice kept when the post meant to deliver it is itself dropped, a loss that lands
while a notice is in flight, scoping to the PR that lost the post, a permission refusal staying
silent, the six-hour staleness cut-off, the registry cap (including a PR already holding a notice
still counting at capacity) and an expired notice making room for a new one.
GitHubDroppedCommentNoticeTestis a@QuarkusTestdriving a real REST client against a loopbackGitHub. It asserts on the body GitHub receives, because that is the only thing the user ever
sees; the log line the retry already writes is exactly what the issue says is not enough.
Red/green proof
GitHubDroppedCommentNoticeTestcompiles unchanged against the base commit, so it was run there:The assertion message is the whole defect in one line: the very next comment GitHub receives is
{"body":"the answer to /describe"}and nothing else. The log knows a reply was thrown away and thepull request never hears about it.
Gates
./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: 2787, Failures: 0, Errors: 0, Skipped: 0git diff -U0 fda4bc7...HEAD→ 0 uncovered lines, 0 uncovered branches across the 79trackable changed main-code lines (
GitHubLostWrites62,GitHubReviewClient13,GitHubCommentClient4)Checklist
Screenshots / Logs
The loss still says so in the log, and now says what will happen about it:
Additional Notes
Every file touched is inside
dev.thiagogonzaga.thrillhousebot.github; no caller signature changedand no fail-soft handler in
review/orwebhook/needed to move.The companion PR for #579 (pacing content-creating calls) touches a disjoint set of files —
GitHubWriteRetry, a new limiter,ThrillhouseConfigandapplication.properties— so the twomerge cleanly in either order. Once both land, the notice becomes rarer: the pacer stops most of the
bursts that produce the throttling in the first place.
Review follow-up
"Concurrent carrying posts can clear a notice that arrived in flight." Correct, reachable, and
fixed — this is the one direction the feature cannot afford to fail in.
The original
settlesubtracted each carrier's snapshot from one shared count. With two posts onthe same pull request in flight, both having read
pending = 1, and a third post thrown awaybetween their reads and their completions:
notice(1)carried = 1)2 > 1, so subtractcarried = 1)1 > 1is false, so the entry is removedThe second loss is retired having been announced by nobody, and the user is never told their content
was dropped — precisely the silence this PR exists to remove. I had noted the over-announcing side
of this overlap as benign and accepted it; the review is right that the clearing side is the opposite
direction and is not acceptable.
The fix replaces the outstanding count with two monotonic watermarks,
lostandannounced(
pending = lost - announced). A delivered post advancesannouncedtowards thelostvalue itactually carried, and the watermark never moves backwards, so a second carrier holding the same
snapshot finds
announced >= carried.lostand leaves the entry alone. Replaying the table above:carrier B advances
announcedto 1, carrier A is a no-op, andpendingstays 1 — the third loss isstill waiting and rides the next comment. The overlap can now only ever repeat a notice, never drop
one, and repeating is the harmless direction.
No locking was added: serializing
carryingper target would hold a lock across an HTTP call thatmay itself back off for up to a minute.
Red/green proof
GitHubLostWritesTest.aLossThatLandsWhileTwoPostsCarryTheSameNoticeIsStillAnnouncedAfterwardsnests one
carryinginside another so the inner post runs entirely between the outer post's readand its completion — the interleaving described above, deterministically. Against this PR's previous
head (
b0973e7), where the other 14 cases in the class still pass:the next comment carried: ""is the defect verbatim: the pull request says nothing at all.Second round: "Announced (zero-pending) entries are never removed and can exhaust the registry cap"
Correct, and a regression the watermark fix above introduced. Before it,
settleremoved the entrythe moment nothing was outstanding; after it, a delivered entry sat at
pending == 0withlost == announced > 0and only the TTL could retire it.What evicted an entry, before this round. Two paths, both TTL:
So the TTL does not cover it. It bounds growth — the map never exceeds the cap and a settled
entry is swept six hours after its last loss, since
announcepreservesloss.at(). What it doesnot do is free the slot in the meantime. Once
DEFAULT_MAX_TARGETSpull requests have lost a postinside one six-hour window, every slot can be a settled entry, and the next pull request to lose one
takes the
pending.size() >= maxTargets && !pending.containsKey(target)branch and is only logged —the silence this PR removes, arriving from the other end, and with nothing outstanding to protect
the slots it was denied.
The fix drops an entry as soon as its last outstanding notice is delivered, so a slot is held
only while a pull request is genuinely still owed one — which is what
DEFAULT_MAX_TARGETSclaimsto bound.
The guard the finding asked for is real, and it is load-bearing. Both watermarks restart when a
later loss recreates an entry, so a carrier still in flight from the previous run holds a snapshot
whose
lostcan equal the new run's and would retire a loss it never carried. Each run of lossesnow carries an
id, and a delivery only counts whenloss.id() == carried.id(). This is notdefensive coding: deleting that one condition and leaving everything else in place makes
aCarrierLeftOverFromASettledEntryCannotRetireALaterLossfail.Red/green proof
GitHubLostWritesTest.aRegistryFullOfAlreadyDeliveredNoticesStillHasRoomForANewLossfills bothslots of the two-target fixture, has both notices delivered, then loses a post on a third pull
request. Against the previous head (
c942633), with the other 16 cases in the class still passing: