fix(github): floor a content-creation block's wait however the delay was derived - #738
Merged
Conversation
…was derived An explicit Retry-After was returned before the #722 floor was ever consulted, so the floor held on the derived branch only. With the measured block body and Retry-After: 5 the whole budget was four attempts spread over 15 seconds against a 72-second block — less waiting than the linear fallback the floor replaced, and the write was given up on inside the window. A Retry-After names a deadline for one request, not the width of the block, so the floor now applies to it too. A Retry-After longer than the floor still wins outright and is clamped by MAX_DELAY_PER_ATTEMPT exactly as before, and a throttle that is not this block keeps its short wait. Fixes #730
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
🤖 ThrillhouseBot PR SummaryWhat this PR doesThis PR fixes #730 by moving the content-creation block floor so it applies regardless of how the retry delay was derived: an explicit Retry-After shorter than the 30s floor is now lifted to the floor instead of bypassing it. It also factors the derived-delay logic into a private helper and adds unit plus end-to-end clock-advancing tests that pin the wall-clock span of the retry budget on both derivation branches. Description vs. ImplementationNo mismatch found between the PR description and the change. Control-Flow Diagram🔀 Show diagramflowchart TD
A["GitHub throttles content creation"] --> B{"Retry-After present?"}
B -- "yes" --> C["delay = atLeastZero(Retry-After)"]
B -- "no" --> D["delay = derivedDelay(attempt, now)"]
C --> E{"content-creation block and delay below floor?"}
D --> E
E -- "yes" --> F["delay = CONTENT_CREATION_BLOCK_MIN_DELAY (30s)"]
E -- "no" --> G["keep delay"]
F --> H["GitHubWriteRetry waits and retries"]
G --> H
H --> A
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 |
|---|---|---|---|
| trivy | check-run | ⏳ Pending | - |
| format | check-run | ⏳ Pending | - |
| frontend | check-run | ⏳ Pending | - |
| test | 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
9 tasks
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
|
devops-thiago
added a commit
that referenced
this pull request
Aug 16, 2026
…ery line terminator (#740) > **Stacked on #738 (`fix/730-retry-after-floor`).** Based against that branch so the diff reads clean; **re-target to `main` once #738 merges**. Only the last commit (`154d172`) belongs to this PR. Note that `ci.yml` triggers on pull requests into `main`/`develop`/`release/**` only, so the full CI run happens on re-target — the gates below were run locally on this exact tree. ## What type of PR is this? - [x] 🐛 Bug fix - [x] 🔒 Security ## Description Two defects in the same method, both in the path that exists to *explain* a failed write. ### 1. Redaction ran before the cap, and the JWT shape is quadratic — the primary fix `readBody` reads the entity with no size bound of its own, and `clean()` redacted the whole of it, capping at 512 characters only afterwards. So the cost of logging one 4xx was set by whatever the configured API host chose to send. `CREDENTIAL_SHAPED_VALUE`'s JWT alternative is `eyJ[\w-]{8,}\.[\w-]{8,}\.[\w-]{8,}`. `[\w-]` excludes `.`, so on a body of repeated `eyJ` each greedy run consumes to end-of-input, fails to find its separator and backtracks one character at a time — from one position in three. That is quadratic, and it was measured as such: | body size | time | |---|---| | 20 000 | 331 ms | | 40 000 | 1 213 ms | | 80 000 | 4 843 ms | | 160 000 | 19 404 ms | | 1.2 MB | ~1 078 623 ms (≈ 18 minutes) | Every doubling costs about 4×. That time is spent on the review's own carrier thread, inside `GitHubApiError.from`, before the retry decision the cleaned body feeds is even reached. The fix is the one the audit called for: **cut, then redact, then cap.** ```java var collapsed = WHITESPACE.matcher(raw).replaceAll(" ").strip(); var bounded = cutTo(collapsed, MAX_BODY_CHARS * 2); var redacted = redactCredentials(bounded); var capped = cutTo(redacted, MAX_BODY_CHARS); return capped.length() < redacted.length() || bounded.length() < collapsed.length() ? capped + "…" : capped; ``` Every regex pass is now bounded at a constant. The pre-cut is twice `MAX_BODY_CHARS` rather than exactly it because redaction only ever *shortens*, so the wider window leaves material to fill the 512-char cap with; past that the output was going to be truncated anyway, and what a wider window would pull into view is more of the token-shaped run being masked out. The ellipsis now marks **either** cut, so a body shortened before redaction is never mistaken for one GitHub sent whole. The surrogate-pair guard that protected the 512 cut is factored into `cutTo` and now protects both cuts. ### 2. `\s` is the ASCII six, so most line terminators survived the collapse `WHITESPACE` was `\\s+`, which java.util.regex reads as `[ \t\n\x0B\f\r]` unless the pattern asks for Unicode character classes. CR and LF being collapsed closes the classic forged-record vector, but NEL (U+0085), LINE SEPARATOR (U+2028), PARAGRAPH SEPARATOR (U+2029), NUL and the ANSI escape all reached the warn line intact — and a log viewer, a terminal, or a JSON/ECS shipper may treat any of them as a record boundary or a screen-control sequence. The class already documents a body as attacker-influenced text on its way to a log file and already pays for a collapse pass on that basis; the pass simply did not cover the class it claimed to. Now `[\\s\\p{IsCc}\\u2028\\u2029]+` — `\p{IsCc}` is the Unicode general category rather than POSIX `\p{Cntrl}`, so it reaches the C1 controls (U+0080–U+009F, NEL among them) as well as C0 and DEL. The strip moved after the collapse so a terminator at either end does not survive as a stray space. ### Not in this PR - **The `finding.file()` interpolation** (`ReviewPublisher.java:752` and `:814`) — the third item on the issue. `ReviewPublisher` is being changed by separate work in this round, so touching it here would conflict; it needs `MarkdownSafe.oneLine` at both warn sites and is left to that change. - **Bounding `readEntity` itself.** The audit lists it as optional. Every regex pass is bounded now, and the remaining cost of a huge body is one linear collapse pass plus the read that already happened. - **A5's "match on more than the log-shaped body".** That is a separate finding about *which* string the retry decision reads, not about the cost of producing it. ## Related Issues Fixes #731 ## How Has This Been Tested? - [x] Unit tests Four tests added to `GitHubApiErrorTest.BodyHandling`, all reusing the audit's probes: - `doesNotScanAWholeOversizedBodyLookingForCredentials` — 200 KB of `eyJ` (probe P2c), bounded at 2 000 ms. The bound is enormously slack against what this costs once the body is cut first; it is sized to fail only on the quadratic, never on a slow machine. - `collapsesTheLineTerminatorsAndControlsThatAreNotAsciiWhitespace` — probe P3b's body, asserted as one clean line. - `stripsALineTerminatorAtEitherEndRatherThanLeavingASpace` - `marksABodyCutBeforeRedactionAsTruncatedEvenWhenTheMaskFitsUnderTheCap` — a 4 000-char bearer value masks to `***`, and the result must still say it was cut. The existing `capsAnOverLongBodySoOneFailureCannotFloodTheLog` and `neverCutsAnOverLongBodyThroughASurrogatePair` are unchanged and still pass, which is what pins the cap and the surrogate guard through the restructuring. ### Verbatim red output on unfixed code ``` [ERROR] Tests run: 15, Failures: 4, Errors: 0, Skipped: 0, Time elapsed: 95.25 s <<< FAILURE! -- in dev.thiagogonzaga.thrillhousebot.github.GitHubApiErrorTest$BodyHandling [ERROR] GitHubApiErrorTest.doesNotScanAWholeOversizedBodyLookingForCredentials cleaning a 200 KB body took 92877ms ==> expected: <true> but was: <false> [ERROR] GitHubApiErrorTest.collapsesTheLineTerminatorsAndControlsThatAreNotAsciiWhitespace expected: <a WARN forged-by-NEL WARN forged-by-LS WARN forged-by-PS NUL [2J ansi> but was: <a?WARN forged-by-NEL ?WARN forged-by-LS ?WARN forged-by-PS NUL ansi> [ERROR] GitHubApiErrorTest.stripsALineTerminatorAtEitherEndRatherThanLeavingASpace expected: <boom> but was: <boom ?> [ERROR] GitHubApiErrorTest.marksABodyCutBeforeRedactionAsTruncatedEvenWhenTheMaskFitsUnderTheCap expected: <***?> but was: <***> ``` (The `?` are the terminal's rendering of the surviving U+0085 / U+2028 / U+2029 and of the ellipsis.) **92 877 ms for one 200 KB body** — this machine is slower than the one the audit measured 24 s on, which only sharpens the point. After the fix the whole 15-test `BodyHandling` nest runs in **3.4 s**, and all four go green: ``` [INFO] Tests run: 15, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 3.393 s -- in dev.thiagogonzaga.thrillhousebot.github.GitHubApiErrorTest$BodyHandling ``` ### Gates - `./mvnw -B spotless:apply` → clean - `./mvnw -B clean compile spotbugs:check spotless:check` → `BugInstance size is 0`, BUILD SUCCESS - `./mvnw -B clean test` → `Tests run: 3290, Failures: 0, Errors: 0, Skipped: 0` - jacoco ∩ `git diff -U0` on this commit's main code → 0 uncovered lines, 0 uncovered branches (both conditions of the ellipsis test and both arms of the surrogate guard are exercised) ## 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 See the verbatim red output above. ## Additional Notes Severity is latent, not live: GitHub's real error bodies are around 300 characters, and no path was found by which a PR author gets GitHub itself to echo hundreds of kilobytes into a 4xx body. It needs a large body from the configured API host — a GHES or reverse-proxy error page, a misconfigured base URL, a compromised endpoint. The redaction half is pre-existing from #704 (v0.6.2); #723 made the same `clean()` output load-bearing for a retry decision, and promoted a debug line to warn, which is what widened the exposure of the second half.
This was referenced Aug 16, 2026
Closed
Merged
devops-thiago
added a commit
that referenced
this pull request
Aug 16, 2026
… log gets (#753) > **Stacked on #750 (`fix/746-credential-shapes`).** Based against that branch so the diff reads clean; **re-target to `main` once #750 merges**. Only the last commit (`afb2d41`) belongs to this PR. `ci.yml` triggers on pull requests into `main`/`develop`/`release/**` only, so the full CI run happens on re-target — the gates below were run locally on this exact tree. ## What type of PR is this? - [x] 🐛 Bug fix ## Description One change closing two issues, because they are the same coupling seen twice. `isThrottled()` and `blocksContentCreation()` matched against `this.body` — the string `diagnostics()` prints. So every narrowing the log line asks for narrowed the retry decision with it, and the consequence is not a shorter wait: `GitHubWriteRetry.retryDelay` returns `Optional.empty()` on `!isThrottled()`, the `WebApplicationException` is 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_url` or 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>`: | prefix | v0.6.3 | v0.6.4 | |---|---|---| | 900 chars | throttled, `PT30S` | throttled, `PT30S` | | 1010 chars | throttled | **not throttled**, `PT5S` | | 4000 chars | throttled | **not throttled**, `PT5S` | Same remedy for both, so they land together. **The fix.** One collapse pass now feeds two readings that are kept apart in a small `Body` record. The logged line is unchanged — same `bounded → redacted → capped` order, 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 `blocked` turned 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.ofEpochSecond` guard #732 asks for in its second half.** `Long.parseLong` accepts values `Instant.ofEpochSecond` rejects, and the resulting `DateTimeException` was thrown from inside `derivedDelay`, out through `GitHubWriteRetry.retryDelay` and `call`, past every `catch (WebApplicationException)` in the write path. `GitHubLostWrites.recording` catches 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? - [x] Unit tests - [ ] Integration tests - [ ] Manual testing 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,GitHubWriteRetryTest` with only the test files applied: ``` [ERROR] Failures: [ERROR] GitHubApiErrorTest.isStillReadWhenItSitsPastTheLengthCapTheLogLineUses body={"documentation_url":"xxxxx…xxx… ==> expected: <true> but was: <false> [ERROR] GitHubApiErrorTest.isStillReadWhenMoreCredentialShapedMaterialPrecedesItThanTheRedactionBoundHolds body=***… ==> expected: <true> but was: <false> [ERROR] GitHubApiErrorTest.isNotSomethingTheCredentialMaskCanDeleteBeforeItIsRead body={"message":"prefix *** from content creation"} ==> expected: <true> but was: <false> [ERROR] GitHubApiErrorTest.isReadFromABoundedWindowRatherThanFromAnUnboundedBody expected: <true> but was: <false> [ERROR] GitHubWriteRetryTest.anOutOfRangeResetHeaderStillFailsAsTheExceptionEveryCallerCatches:332 Unexpected exception type thrown, expected: <jakarta.ws.rs.WebApplicationException> but was: <java.time.DateTimeException> [ERROR] Errors: [ERROR] GitHubApiErrorTest.aRateLimitResetTooLargeToBeAnInstantIsTreatedAsUnspecified » DateTime Instant exceeds minimum or maximum instant [ERROR] GitHubApiErrorTest.aRateLimitResetTooSmallToBeAnInstantIsTreatedAsUnspecified » DateTime Instant exceeds minimum or maximum instant [ERROR] GitHubWriteRetryTest.anOutOfRangeResetHeaderOnAThrottleFallsBackToTheLinearWait:356 » DateTime Instant exceeds minimum or maximum instant [ERROR] Tests run: 93, Failures: 5, Errors: 3, Skipped: 0 ``` The escape path in full, from the same run — this is the whole of the second finding: ``` java.time.DateTimeException: Instant exceeds minimum or maximum instant at java.base/java.time.Instant.ofEpochSecond(Instant.java:308) at …GitHubApiError.lambda$derivedDelay$0(GitHubApiError.java:325) at …GitHubApiError.derivedDelay(GitHubApiError.java:325) at …GitHubApiError.retryDelay(GitHubApiError.java:312) at …GitHubWriteRetry.retryDelay(GitHubWriteRetry.java:254) at …GitHubWriteRetry.call(GitHubWriteRetry.java:189) ``` Two of the six new `GitHubApiError` tests 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) and `doesNotMakeAPermissionRefusalLookLikeAThrottle` (a 4 KB body with none of the wording is still a refusal, so the wider window did not make the classifier credulous). `isReadFromABoundedWindowRatherThanFromAnUnboundedBody` is 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: 0** - jacoco ∩ `git diff -U0 fc54d93...HEAD` over changed main code → 139 changed lines, **zero uncovered lines and zero uncovered branches** ## 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 ## Additional Notes `GitHubWriteRetry`'s main code is untouched — the guard belongs where the exception is raised, and the retry loop's contract (a `WebApplicationException` in, 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.
devops-thiago
added a commit
that referenced
this pull request
Aug 16, 2026
…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
devops-thiago
added a commit
that referenced
this pull request
Aug 16, 2026
…751) ## What type of PR is this? - [x] 🐛 Bug fix ## 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.createReviewWithFallback` tries `reviewClient.createReview` (wrapped in `GitHubLostWrites.carrying`) and, on a definite refusal, preserves the same body as an issue comment via `commentClient.createComment` (#704, also wrapped in `carrying`). GitHub's content-creation block is a 403, so it counts as a refusal: route 1 `remember()`s a loss and route 2's `carrying` reads 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: 1. **`ReviewPublisher.createReviewWithFallback`** now runs its routes inside `GitHubReviewClient.asOneComment`. The routes themselves moved verbatim into a private `createReviewRoutes`; no route, ordering or refusal/ambiguity rule changed. With a scope open, a throttled `createReview` only 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. 2. **`GitHubLostWrites.asOneDelivery`** no 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 (`deliveryFor` compares 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 #729 removed, reintroduced silently for the nested caller. It was unreachable while `asOneComment` had 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 the `finally`. 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? - [x] Unit tests New `RescuedReviewBodyLostWriteTest` (2 tests) drives the **real `default` methods of both clients** — only the `*Once` HTTP 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 `GitHubLostWritesTest` for the nesting half. ### Red output on unfixed code (`fc54d93`) ``` [ERROR] RescuedReviewBodyLostWriteTest.aReviewBodyTheCommentFallbackRescuedIsNotAnnouncedAsLost org.opentest4j.AssertionFailedError: the review body was delivered by its comment fallback, and the very comment that delivered it opens by telling the maintainer it was never posted: > [!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. (warning sign) GitHub refused the review post, so ThrillhouseBot is posting the review as a regular comment instead. ThrillhouseBot requested changes — see inline. ==> expected: [false] but was: [true] [ERROR] RescuedReviewBodyLostWriteTest.aReviewBodyNoRouteCouldDeliverIsAnnouncedExactlyOnce org.opentest4j.AssertionFailedError: the genuinely lost review body must still be announced: > [!WARNING] > **2 earlier replies on this pull request were 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. the next thing the bot posts ==> expected: [true] but was: [false] [ERROR] GitHubLostWritesTest.aDeliveryNestedInsideOneForAnotherPullRequestIsGroupedOnItsOwn org.opentest4j.AssertionFailedError: the inner delivery was not grouped, so content its second route delivered was announced as lost: > [!WARNING] > **An earlier reply on this pull request was never posted.** ... ==> expected: [] but was: [> [!WARNING] > **An earlier reply on this pull request was never posted.** ...] [ERROR] GitHubLostWritesTest.aNestedDeliveryIsAnnouncedOnceAndHandsTheOuterOneBack org.opentest4j.AssertionFailedError: > [!WARNING] > **2 earlier replies on this pull request were never posted.** ... ==> expected: [true] but was: [false] [ERROR] Tests run: 26, Failures: 4, Errors: 0, Skipped: 0 ``` (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 SUCCESS - `clean test`: **Tests run: 3308, Failures: 0, Errors: 0, Skipped: 0** — BUILD SUCCESS - jacoco ∩ `git diff -U0 fc54d93...HEAD` on changed main code: **zero uncovered lines, zero uncovered branches** ## 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 ## Additional Notes The javadoc on `asOneComment` and `asOneDelivery` was corrected too: the former was written as if it only ever groups `createPullRequestComment` calls, and the latter justified the unconditional nesting no-op with an argument that only holds when the targets match.
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
GitHubApiError.retryDelayreturned an explicitRetry-Afterbefore the #722 content-creation floor was consulted, so the invariant #723 claims — thatTOTAL_BUDGETbecomes a floor for this failure class rather than only a ceiling — held on the derived branch only.With the body measured in #722 and
Retry-After: 5, the shipped code spends all four attempts in 15 seconds against a 72-second block and gives up on a write whose content has already been paid for. That is less waiting than the pre-#723 linear fallback (5 + 10 + 15 = 30s) and one sixth of the derived path the fix installed.A
Retry-Afternames a deadline for this request; it is GitHub pacing one call, not a statement about how wide the block is. So the floor now applies to the delay however it was arrived at:Deliberately preserved:
Retry-Afterlonger than the floor still wins outright, clamped byMAX_DELAY_PER_ATTEMPTexactly as before — nothing GitHub actually sends today (60 for secondary limits) changes;The floor only ever binds when GitHub is still refusing — if a short
Retry-Afterwere honest, attempt 2 succeeds and the floor costs nothing.Related Issues
Fixes #730
How Has This Been Tested?
Two clock-advancing tests in
GitHubWriteRetryTest.TheMeasuredSecondaryLimitWindowdrive the realGitHubWriteRetry.callend to end with a clock the recorded waits advance, GitHub refusing until 72 seconds of simulated time have passed. They pin the wall clock the budget spans, not an attempt count, on both derivation branches:aBlockIsOutlastedEvenWhenGitHubNamesAShortDeadlineRetry-After: 5aBlockIsOutlastedWhenGitHubNamesNoDeadlineAtAllremaining=4771, stale resetThe second is the control on the branch #723 already fixed: it is green before and after, and it is here because the floor has to hold on both branches, not because it proves the bug. The red proof is the first one, plus the unit-level assertion in
GitHubApiErrorTest.Verbatim red output on unfixed main code
GitHubApiErrorTest.AContentCreationBlockGitHubGaveNoDeadlineFor(Retry-After: 3on the measured block body):GitHubWriteRetryTest.TheMeasuredSecondaryLimitWindow— the write is abandoned rather than landing, and the log shows the whole budget spent inside the block:The same run's control on the other branch logged
retrying in 30sthree times and posted. With the fix, both branches log 30s three times and the write lands at 90s.Also updated:
stillYieldsToADeadlineGitHubNamedpinned the old behaviour (Retry-After: 3 → PT3Son a block body) and is replaced byisFlooredEvenWhenGitHubNamedAShorterDeadline;stillYieldsToALongerDeadlineGitHubNamed(60s wins) anddoesNotFloorARetryAfterOnAThrottleThatIsNotThisBlockpin the two behaviours that must not change.Gates
./mvnw -B spotless:apply→ clean./mvnw -B clean compile spotbugs:check spotless:check→BugInstance size is 0, BUILD SUCCESS./mvnw -B clean test→Tests run: 3286, Failures: 0, Errors: 0, Skipped: 0git diff -U0 aa3c556...HEADon changed main code → 0 uncovered lines, 0 uncovered branchesChecklist
Screenshots / Logs
See the verbatim red output above.
Additional Notes
Not attempted here: the audit's other reading, "cap total attempts by elapsed wall time rather than count". That is a larger change to
GitHubWriteRetry's loop and would alter the bound every caller reasons about; the floor keeps the existing two bounds (4 attempts, 30s each) and simply stops one branch from undercutting them.