feat(github): pace content-creating GitHub calls so a burst is never produced - #597
Open
devops-thiago wants to merge 2 commits into
Open
feat(github): pace content-creating GitHub calls so a burst is never produced#597devops-thiago wants to merge 2 commits into
devops-thiago wants to merge 2 commits into
Conversation
…produced Backoff (#568) is reactive: it only acts once GitHub has already refused a post. In the run behind that issue, 56 commands across 8 PRs created content faster than GitHub would accept it and drew 29 rejections — each a wasted round trip, a spent retry attempt and a dispatcher slot held while a doomed call backed off. GitHubWritePacer spaces content-creating calls out instead. It sits on the same seam as the backoff, so it covers exactly the calls GitHub counts as content creation, and it is process-wide: a review posting one call per inline finding, or several PRs reviewed at once, both produce a burst without anyone doing anything unusual. Each caller atomically claims the next free instant and waits for it, with no lock held across the HTTP call. The spacing (default 1s, GitHub's published guidance) and the ceiling on a single wait (default 60s) are knobs. Past the ceiling the call goes out unpaced and the bounded backoff handles a refusal, so a long queue never parks a finished command. An interrupted pacing wait sends the call anyway: the limiter must never be the reason a paid-for post is lost. Fixes #579
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 doesAdds a process-wide GitHubWritePacer that spaces content-creating GitHub calls (comments, reviews, review comments) via an atomic slot cursor claimed in GitHubWriteRetry.call, so bursts are spaced out before GitHub refuses them instead of being discovered by 403s. Both the min interval (default 1s, 0 disables) and the max wait (default 60s) are configurable knobs, every retry attempt is paced, and an interrupted pacing wait still lets the call proceed. Control-Flow Diagram🔀 Show diagramflowchart TD
A["Write path: createComment, createReview, etc."] --> B["GitHubWriteRetry.call(operation)"]
B --> C["pacer.acquire(operation)"]
C --> D{"slot already past?"}
D -->|"yes"| E["call goes out immediately"]
D -->|"no"| F{"wait exceeds max-wait?"}
F -->|"no"| G["sleep until claimed slot"]
F -->|"yes"| H["sleep max-wait, then go unpaced"]
E --> I["operationCall.get() posts to GitHub"]
G --> I
H --> I
I --> J{"throttled 403/429?"}
J -->|"no"| K["return result"]
J -->|"yes"| L["bounded backoff sleep"]
L --> B
Changes Overview
Changed Files
Risk Assessment
Things to double-check2 lower-confidence findings
|
| Check | Type | Status | Detail |
|---|---|---|---|
| test | check-run | ⏳ Pending | - |
| frontend | check-run | ⏳ Pending | - |
| changes | check-run | ⏳ Pending | - |
| trivy | check-run | ⏳ Pending | - |
| format | check-run | ⏳ Pending | - |
| actionlint | 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}
There was a problem hiding this comment.
ThrillhouseBot noted 2 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- MEDIUM: Verify all five content-creating methods dispatch through GitHubWriteRetry.call (
src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubWriteRetry.java:107)
The PR's central claim is thatGitHubWriteRetry#callis exactly the seam GitHub counts as content creation: createComment, updateComment, createReview, createPullRequestComment, replyToReviewComment. The changed linepacer.acquire(operation);only paces paths that reach this method, and the provided material demonstrates only one of the five paths reaching it:GitHubWritePacingTestdrivesGitHubCommentClient.createCommentend-to-end, andGitHubWriteRetryTest.everyAttemptWaitsForItsPacingSlotIncludingTheRepeatsinvokescall(...)directly. The triggering scenarios in #579 — a review posting many inline findings, or several PRs reviewed concurrently — depend oncreateReview,replyToReviewComment,updateCommentandcreatePullRequestCommentrouting throughGitHubWriteRetry.call, but none of those client default methods is visible in the diff. Verify that all four dispatch throughGitHubWriteRetry.call; if any bypasses it, that content-creating path remains unpaced and can still produce the burst this PR is meant to prevent. - MEDIUM: Documented '0 disables pacing' value never exercised through the config converter (
src/main/resources/application.properties:45)
Three places in this PR tell operators thatGITHUB_WRITE_MIN_INTERVAL=0disables pacing: application.properties line 45 (# GITHUB_WRITE_MIN_INTERVAL: spacing between two content-creating requests (0 disables pacing).), theThrillhouseConfig.writeMinInterval()Javadoc, and theGitHubWritePacer.MIN_INTERVAL_KEYJavadoc. The only test of zero-interval behavior,GitHubWritePacerTest.aZeroIntervalTurnsPacingOffEntirely, constructsGitHubWritePacerwithDuration.ZEROdirectly, bypassingconfigured()and the SmallRyeDurationConverter; the knob testtheEnvelopeIsAKnobAndFallsBackToTheGuidanceWhenItIsUnsetonly exercises the suffix form250ms. Whether a bare"0"converts toDuration.ZERO— or is rejected, which would makeGITHUB_WRITE_MIN_INTERVAL=0fail Quarkus startup validation of the@ConfigMappingnamespace and contradict the documented behavior — is not established by the provided material. Verify with the actual converter (e.g. boot a QuarkusTest withGITHUB_WRITE_MIN_INTERVAL=0, or callGitHubWritePacer.configured("thrillhousebot.github.write-min-interval", DEFAULT_MIN_INTERVAL)with the property set to0); if conversion fails, the docs should say0sso the documented disable path actually works.
thrillhousebot
Bot
added
enhancement
…isables it Two review findings asked for verification rather than reporting a defect, and both check out: all five content-creating methods do reach GitHubWriteRetry.call, and a bare "0" does convert to Duration.ZERO through the SmallRye converter. Neither needed a code change; both were thin on evidence, which is worth closing. The end-to-end pacing proof drove only createComment, while the bursts #579 names — a review posting many inline findings, several PRs reviewed at once — run through the review client. Fire all five at once instead, so a future write path that skips the seam fails the test rather than silently reintroducing the burst. The documented "0 disables pacing" promise runs through the config converter, while the only zero-interval test handed Duration.ZERO straight to the constructor. Set the property, convert it, and pace a burst with the result, since a converter that rejected the value would fail startup and stay invisible until an operator reached for the knob.
|
🤖 ThrillhouseBot — changes since the last review
|
There was a problem hiding this comment.
ThrillhouseBot noted 1 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- LOW: New config keys not documented in README config table or .env.example (
src/main/resources/application.properties:50)
This PR adds two new environment variables/config keys: GITHUB_WRITE_MIN_INTERVAL -> thrillhousebot.github.write-min-interval and GITHUB_WRITE_MAX_WAIT -> thrillhousebot.github.write-max-wait. They are documented only in application.properties comments and in the ThrillhouseConfig Javadoc. The repository guidance requires every new config key to also appear in the README config table and in .env.example; neither file is changed in this diff. Operators using the standard config reference will not discover the new knobs. Add the two keys to README's config table and to .env.example.
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
#568 made a throttled post survivable; it does not stop the burst being produced. In the run behind
it, 56 commands across 8 PRs created content faster than GitHub would accept it and drew 29
rejections — every one a wasted round trip, a spent retry attempt, and a dispatcher slot held
while a doomed call backed off.
GitHubWritePacerspaces content-creating calls out instead, so the bot stays inside GitHub'ssecondary-rate-limit envelope rather than discovering it by rejection.
Where it sits. On the same seam as the backoff —
GitHubWriteRetry#call— which is exactly theset GitHub counts as content creation:
createComment,updateComment,createReview,createPullRequestComment,replyToReviewComment. It is therefore process-wide and shared acrossreviews and commands rather than per review, which is what the issue asks for: the bot produces a
burst without anyone doing anything unusual.
ReviewPublisher#tryPostInlineCommentissues onecontent-creating call per inline finding, and several PRs are reviewed concurrently.
How a slot is claimed. Each caller atomically claims the next free instant and advances a shared
cursor by one interval, then waits until the instant it claimed. Claiming is a single atomic update
with no lock held across the HTTP call, so a slow request never blocks the queue behind it, and
callers go out in the order they arrived instead of in a thundering herd when the interval elapses.
Both numbers are knobs rather than constants, as the issue asks:
thrillhousebot.github.write-min-intervalGITHUB_WRITE_MIN_INTERVAL1s0disables pacing entirelythrillhousebot.github.write-max-waitGITHUB_WRITE_MAX_WAIT60sThe default is GitHub's published guidance: no more than one content-creating request per second.
Why there is a ceiling. A wait holds the per-PR serialization slot in the dispatcher — the same
reason #568's backoff is bounded twice over. Past the ceiling the call goes out unpaced and the
bounded backoff handles a refusal, which is exactly where the bot is today, rather than parking a
finished command for minutes.
Why pacing can never cost a payload. A pacing wait that is interrupted proceeds with the call
rather than failing it. The content on its way out has already been paid for, so the worst this
limiter is ever allowed to do is let a burst through.
How this composes with #568 and #578. The pacer is the preventative form: it keeps the burst
from being produced. The backoff remains the fallback for throttling pacing cannot prevent — another
instance of the App, or a repo busy for reasons the bot did not cause — and #578's dropped-post
notice is the last resort when even that runs out. Every attempt is paced, repeats included, so a
repeat queues rather than jumping the limiter.
Related Issues
Fixes #579
How Has This Been Tested?
GitHubWritePacerTestdrives the clock by hand and records the waiting instead of serving it, sowhat is pinned is the arithmetic of the shared cursor: the lone write that is not delayed at all, a
burst handed out one interval apart, a queue that drained charging nothing, the ceiling clamping a
long queue,
0disabling pacing, an interrupted wait sending the call anyway, and the knob fallingback to the guidance when unset.
GitHubWriteRetryTestgains a case showing a throttle repeat ispaced too.
GitHubWritePacingTestis a@QuarkusTestdriving a real REST client against a loopback GitHub —the only thing that can show the limiter is on the path a comment actually takes, and that it holds
when the burst is genuinely concurrent. It asserts on when the requests arrive at GitHub, not on
anything the bot recorded about itself.
Red/green proof
GitHubWritePacingTestcompiles unchanged against the base commit, so it was run there:[0, 0, 0]ms apartis the defect verbatim: four concurrent posts all reach GitHub inside the samemillisecond, which is the burst it answers with 403. With the fix they arrive a second apart.
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: 2779, Failures: 0, Errors: 0, Skipped: 0git diff -U0 fda4bc7...HEAD→ 0 uncovered lines, 0 uncovered branches across the 38 trackable changed main-code lines (GitHubWritePacer32,GitHubWriteRetry6)Checklist
Screenshots / Logs
The limiter is quiet by design — it logs at debug, because a slot wait is normal operation rather
than an incident:
The line that used to appear instead is the 403 the wait prevents.
Additional Notes
ThrillhouseConfig.GitHubConfiggains the two knobs. That is not optional:@ConfigMapping(prefix = "thrillhousebot")validates its whole namespace, so an undeclaredthrillhousebot.github.*keyfails startup outright —
— and the declaration is the namespace's schema. The limiter itself reads the key directly, because
it lives on the REST clients'
default-method write path where there is no CDI.The follow-up for #578 (announcing a dropped post on the PR) touches a disjoint set of files —
GitHubCommentClient,GitHubReviewClientand a new class — so the two merge cleanly in eitherorder.
Review follow-up
Both of the review's lower-confidence findings were checked against the code rather than argued
with. Both turned out to be right about the evidence being thin and wrong about there being a
defect, so nothing under
src/mainchanged and two tests were added instead."Verify all five content-creating methods dispatch through
GitHubWriteRetry.call." They do —all five, and nothing else does:
Five hits, five methods, no other call sites — and
callpaces unconditionally, so reaching it isbeing paced. The finding is right that only one of the five was demonstrated end to end, and that is
worth closing because it is this PR's central claim.
GitHubWritePacingTest.everyContentCreatingCallIsPacedAndNotJustTheConversationCommentnow fires all five at once against the loopback GitHub and asserts every arrival is spaced. On the
base commit it fails exactly as the finding predicts an unpaced path would:
"Documented '0 disables pacing' value never exercised through the config converter." It survives
it. Measured against the real SmallRye converter rather than reasoned about:
So a bare
0converts toDuration.ZERO, the documentation is accurate, and the startup-validationfailure the finding feared does not occur — no wording changed. The gap it identifies is real
though: that promise runs through the converter, while the only zero-interval test handed
Duration.ZEROstraight to the constructor.GitHubWritePacerTest.theDocumentedBareZeroSurvivesTheConfigConverterAndReallyDisablesPacingnow sets the property, converts it, and paces a burst with the result, because the failure mode
would otherwise stay invisible until an operator reached for the knob during an incident.