fix(github): classify a throttle from the body, not from the line the log gets - #753
Merged
devops-thiago merged 1 commit intoAug 16, 2026
Merged
Conversation
… log gets `isThrottled()` and `blocksContentCreation()` matched against the same string `diagnostics()` prints, so every narrowing the log line asks for narrowed the retry decision with it. The 512-character cap did it first (#732): wording past the cap was not seen, and the failure is total rather than partial — `GitHubWriteRetry.retryDelay` returns empty on `!isThrottled()`, so the write is rethrown on the first attempt and not repeated at all, which is the pre-#495 behaviour this area exists to prevent. #740 then bounded the redaction input to 1024 characters and closed the one path by which deeper wording still arrived: v0.6.3 redacted the whole collapsed body first, so a long credential-shaped prefix compressed to `***` and carried the message forward into the classified string (#747). Measured threshold: 900 characters of prefix still classified, 1010 no longer. Both readings are now taken from one collapse pass and kept apart. The log line keeps the `bounded → redacted → capped` order and its ellipsis; the classification reads the collapsed body bounded at 8 KB and nothing else. It is deliberately the unredacted text, because the mask ran before the classifier could read it and a mask that swallowed the word `blocked` turned a content-creation block into a permission refusal. Nothing in that window is ever logged or returned. Also guards `Instant.ofEpochSecond`. `Long.parseLong` accepts values it rejects, and the resulting `DateTimeException` escaped `GitHubWriteRetry.call` past every `catch (WebApplicationException)` in the write path — so `GitHubLostWrites` did not record the write as lost either, and a header from an intermediary took the write and the record of its loss together. A value that cannot be an instant now means what a non-numeric header already means here: unspecified, and the linear fallback takes over.
Contributor
🤖 ThrillhouseBot PR SummaryWhat this PR doesSeparates the response body reading into a log-facing line (unchanged: bounded, redacted, capped at 512) and a wider classification window (collapsed, unredacted, bounded at 8 KB) so throttle/content-creation wording can no longer be hidden by the log cap or the redaction-input bound; also treats an x-ratelimit-reset outside Instant's range as unspecified so GitHubWriteRetry keeps its WebApplicationException contract and linear fallback. Description vs. ImplementationNo mismatch found between the PR description and the change. Control-Flow Diagram🔀 Show diagramflowchart TD
A["GitHubApiError.from(Response)"] --> B["readBody()"]
B --> C["clean(raw): collapse whitespace once"]
C --> D["logged = bounded 1024, redacted, capped 512 + ellipsis"]
C --> E["classified = collapsed cut to 8192, unredacted"]
D --> F["diagnostics() logs redacted line only"]
E --> G["isThrottled() / blocksContentCreation() regex match"]
G --> H{"throttle wording or headers?"}
H -- "no" --> I["WebApplicationException rethrown; no retry"]
H -- "yes" --> J["derivedDelay: parse x-ratelimit-reset"]
J --> K{"reset names a valid Instant?"}
K -- "yes" --> L["wait until reset (floored at zero)"]
K -- "no / absent" --> M["linear fallback: 5s per attempt"]
Changes Overview
Changed Files
Risk Assessment
Everything's coming up Thrillhouse! 🎉 No issues found in this PR. Automated review by ThrillhouseBot. Reply with |
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
thrillhousebot
Bot
added
bug
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
One change closing two issues, because they are the same coupling seen twice.
isThrottled()andblocksContentCreation()matched againstthis.body— the stringdiagnostics()prints. So every narrowing the log line asks for narrowed the retry decision with it, and the consequence is not a shorter wait:GitHubWriteRetry.retryDelayreturnsOptional.empty()on!isThrottled(), theWebApplicationExceptionis rethrown on the first attempt, and the write is not repeated at all — the pre-#495 behaviour this area exists to prevent. The 30-second floor #738 just widened is never consulted.#732 is the 512-character cap doing it: a body with a long
documentation_urlor echoed headers ahead of the message classifies as a refusal. Equally true at v0.6.3.#747 is the bound #740 put on the redaction input. v0.6.3 had exactly one path by which wording deeper than the cap still survived — redaction compressing a long credential-shaped prefix to
***and carrying the message forward — and cutting to 1024 before redacting closed it. Threshold sweep from the audit, body ="Bearer " + "a".repeat(n) + " " + <content-creation block>:PT30SPT30SPT5SPT5SSame remedy for both, so they land together.
The fix. One collapse pass now feeds two readings that are kept apart in a small
Bodyrecord. The logged line is unchanged — samebounded → redacted → cappedorder, same ellipsis, same 512 characters. Classification reads the collapsed body bounded at 8 KB and nothing else. The bound is still wanted (the entity is read with no size limit of its own, and #731 is about not letting the configured host set the cost of explaining a failed write), but both patterns are flat literal alternations with no backtracking, so widening the window costs a linear scan; the quadratic shape #731 found is in the credential redaction, which still sees only its own 1024.Deliberately the unredacted text. Masking runs before the classifier could read it, and a mask that swallowed the word
blockedturned a content-creation block into a permission refusal — the classification tail of the JWT over-match in #746. Nothing in that window is ever logged or returned; the two patterns answer yes or no and the string is dropped.Also, the
Instant.ofEpochSecondguard #732 asks for in its second half.Long.parseLongaccepts valuesInstant.ofEpochSecondrejects, and the resultingDateTimeExceptionwas thrown from insidederivedDelay, out throughGitHubWriteRetry.retryDelayandcall, past everycatch (WebApplicationException)in the write path.GitHubLostWrites.recordingcatches that type specifically, so a write that died this way was not remembered as lost either — the write and the record of its loss went together, over one header from an intermediary. A value that cannot be an instant now means what a non-numeric header already means here: unspecified, and the linear fallback takes over.Neither classification bug is reachable against api.github.com, whose error bodies are ~300 characters with the message first; both need a large body from the configured API host. The header case needs an intermediary sending a ~10^17 reset.
Related Issues
Fixes #747
Fixes #732
How Has This Been Tested?
Eight new behavioural assertions, all red on the parent branch (
1e0d7fc) in exactly the claimed way and green after. Verbatim, from./mvnw -o test -Dtest=GitHubApiErrorTest,GitHubWriteRetryTestwith only the test files applied:The escape path in full, from the same run — this is the whole of the second finding:
Two of the six new
GitHubApiErrortests are labelled controls rather than proof, and both are green before the fix:doesNotWidenWhatReachesTheLog(the log line keeps its own 512-character cap and its ellipsis whatever the classifier may see) anddoesNotMakeAPermissionRefusalLookLikeAThrottle(a 4 KB body with none of the wording is still a refusal, so the wider window did not make the classifier credulous).isReadFromABoundedWindowRatherThanFromAnUnboundedBodyis half proof and half honest edge: wording at 4 000 characters is now read, wording at 64 000 is still not.Gates on this tree:
./mvnw -B spotless:apply→ clean./mvnw -B clean compile spotbugs:check spotless:check→ BugInstance size is 0, Error size is 0, spotless clean./mvnw -B clean test→ Tests run: 3320, Failures: 0, Errors: 0, Skipped: 0git diff -U0 fc54d93...HEADover changed main code → 139 changed lines, zero uncovered lines and zero uncovered branchesChecklist
Additional Notes
GitHubWriteRetry's main code is untouched — the guard belongs where the exception is raised, and the retry loop's contract (aWebApplicationExceptionin, the same one back out) is what the new test pins from the outside.The 8 KB window is a judgement call, not a measurement: it is sixteen times the log cap and several times any body GitHub sends, chosen so the classifier stops depending on where in a body the message sits while the pass over it stays a linear scan.