fix(ai): bound how many attempts of one call may end at the streaming deadline

[devops-thiago]

What type of PR is this?

• 🐛 Bug fix
• ✨ Feature
• 📝 Documentation
• 🔧 Refactor
• 🚀 Performance
• ✅ Test
• 🔒 Security
• 📦 Dependency update
• 🏗️ CI/CD

Description

A streaming attempt waits THRILLHOUSEBOT_REVIEW_AI_TIMEOUT_SECONDS (900 in production, 300 by default) and AiReviewService then retried that timeout like any other transient failure, up to THRILLHOUSEBOT_REVIEW_MAX_AI_RETRIES (5). One logical call could spend 75 minutes of wall clock, all of it holding its pull request's dispatcher slot, and the pull request is pushed to often enough that the review is superseded and the replacement starts the same wait.

Every claim in the issue holds against the code:

• streamInSlot waited result.get(streamTimeout().toMillis(), …) and turned the TimeoutException into a plain AiReviewException, which attemptWithRetries caught in its general catch (RuntimeException e) branch and retried like a connection reset. The deterministic failures each have their own escape (AiResponseTruncatedException, AiContextWindowExceededException), the deadline had none.
• maxAttempts is config.review().maxAiRetries(), shipped as 5 in application.properties; ai-timeout-seconds ships as 300 there and production overrides it to 900.
• summarize goes through the same runWithRetries, so the summary lane had the same behaviour.
• ReviewDispatcher.runSerialized serializes work per PrKey, so the wait does hold that pull request's slot.

One correction to the issue's framing: THRILLHOUSEBOT_REVIEW_AI_TIMEOUT_SECONDS is 300 in the shipped defaults; the 900 in the report is the production override, and the issue's arithmetic is about that deployment.

The fix names the deadline apart from the rest. AiReviewTimeoutException carries the wait the attempt spent; runWithRetries opens a TimeoutBudget for the logical call and both of its passes share it; attemptWithRetries logs and broadcasts the attempt exactly as before, then failIfTimeoutBudgetSpent ends the call when it was the second attempt to end at the deadline. Every other transient failure keeps the whole budget, and a timeout followed by a successful attempt still succeeds. The WARN line names the session, the attempt and the wait:

AI review attempt 2/5 for session 4685 timed out after PT15M; that is the last of the 2 timed-out attempts one call may have (PT30M of waiting in all), so the call fails here instead of spending its remaining attempts on a request that already did not finish inside the deadline

Should the repeat wait a shorter deadline? It should not, and it does not here. The repeat exists for the one shape of timeout that is not about the request: a first token that never arrived because the provider queued the call. That attempt needs the full deadline to come back, and a halved one would turn a review that was going to succeed into a failure the bot still paid for — on exactly the large pull requests that legitimately take ten-plus minutes to review. The issue also asks that the setting keep its meaning for a single attempt, which a shorter second deadline would break. The wall-clock complaint is answered by the count alone: the ceiling per call goes from 75 minutes to 30, and nothing in the change makes a call that would have succeeded fail.

Scope kept to the retry loop, as the issue asks. The length-stop repeat (#839) is untouched apart from sharing the count — the bound is on the logical call, so a call that already waited out one deadline before its length stop has one attempt left, not two more. The slot-refusal backoff and the concurrency gate (#838) are untouched: a call that found no free slot was never sent and stays an ordinary transient failure, and the timed-out stream is still cancelled and its slot still released. The parse-failure handling (#850/#851) is untouched, and because AiReviewTimeoutException is an AiReviewException, a summary call ended this way still degrades to the counts-only summary that keeps the paid findings. On the batch lane the ended call is an ordinary spent call: the sequential pass still gives the batch the one fresh attempt it gives any transient failure — the parallel pass sends every batch at once, and a deadline missed under that contention can be the contention — and the batch's files are still disclosed as not reviewed when that attempt fails too. There is a test for that.

README.md and .env.example say how many attempts may spend the wait.

Related Issues

Fixes #862

How Has This Been Tested?

• Unit tests
• Integration tests
• Manual testing

Red first, against the unfixed code, with the two new bounding tests in place (the two guard tests passed then, as they must):

[ERROR] Tests run: 4, Failures: 2, Errors: 0, Skipped: 0
[ERROR] Failures:
[ERROR]   AiReviewServiceTest.repeatedTimeoutsEndTheCallInsteadOfSpendingTheRetryBudget:485 expected: <2> but was: <5>
[ERROR]   AiReviewServiceTest.repeatedTimeoutsEndTheSummaryCallOnTheSameTerms:533 expected: <2> but was: <5>

Both <5> readings are the shipped behaviour: the call spent every configured attempt on the deadline.

New tests:

• repeatedTimeoutsEndTheCallInsteadOfSpendingTheRetryBudget — repeated timeouts stop after two attempts of five.
• aTimeoutFollowedByASuccessfulAttemptStillSucceeds — the repeat is still made and still counts.
• aTimeoutDoesNotShortenTheBudgetOfOtherTransientFailures — one timeout mixed with provider errors leaves all five attempts for the errors.
• repeatedTimeoutsEndTheSummaryCallOnTheSameTerms — the summary lane takes the same path.
• theCallEndedByRepeatedTimeoutsIsLoggedWithSessionAttemptAndWait — the WARN names the session, the attempt and the wait.
• FindingPipelineTest.multiCallDisclosesABatchWhoseCallGaveUpOnItsTimedOutAttempts — a batch whose call ended this way is retried once by the sequential pass and its files are disclosed as not reviewed.

Gates, from the worktree root on JDK 25:

$ ./mvnw -B clean compile spotbugs:check spotless:check
[INFO] BugInstance size is 0
[INFO] BUILD SUCCESS

$ ./mvnw -B clean test
[INFO] Tests run: 3895, Failures: 0, Errors: 0, Skipped: 0
[INFO] BUILD SUCCESS

Patch coverage of the src/main diff against origin/main, lines and branches, read from target/site/jacoco/jacoco.xml: GAPS: none.

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

Additional Notes

No configuration change is needed on upgrade and no setting changes meaning. A deployment that wants the old behaviour has none — the bound is not configurable, in the same way the length-stop repeat of #839 is not. MAX_TIMED_OUT_ATTEMPTS is one constant with the reasoning next to it if that ever needs to move.

[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

Bounds how many streaming attempts of one AI review call may end at the streaming deadline: a second timed-out attempt in one logical call now fails the call (WARN-logged with session, attempt and wait) instead of spending the remaining retry budget, while every other transient failure keeps the full budget. The timeout is typed as a new AiReviewTimeoutException carrying the attempt's wait, and the budget is shared by both passes of the reasoning step-down so the ceiling per call drops from 75 minutes to 30.

Description vs. Implementation

No mismatch found between the PR description and the change.

Control-Flow Diagram

🔀 Show diagram

flowchart TD
    A["runWithRetries: create TimeoutBudget"] --> B["attemptWithRetries retry loop"]
    B --> C["streamInSlot: result.get(deadline)"]
    C --> D{"TimeoutException?"}
    D -->|"yes"| E["throw AiReviewTimeoutException carrying the wait"]
    D -->|"no"| F["provider error, parse or success"]
    E --> G["catch: broadcast streamFailed"]
    G --> H{"timeout and budget spent?"}
    H -->|"no — first timeout"| I["backoff, next attempt"]
    H -->|"yes — second timeout"| J["WARN log: session, attempt, wait"]
    J --> K["throw call-ending AiReviewTimeoutException"]
    I --> B
    F -->|"success"| L["return ReviewResponse"]

Loading

Changes Overview

• Files changed: 7
• Lines added: +336
• Lines removed: -8

Changed Files

File	Change	Summary
.env.example	Modified	Retry-policy comment now documents the two-timed-out-attempts-per-call bound
CHANGELOG.md	Modified	Unreleased Fixed entry narrating the 75- to 30-minute ceiling change, the WARN decision line, and untouched lanes (#838/#839/#851)
README.md	Modified	Config table rows for MAX_AI_RETRIES and AI_TIMEOUT_SECONDS now state that at most two attempts of one call may end at the deadline
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/AiReviewService.java	Modified	Adds MAX_TIMED_OUT_ATTEMPTS=2, a shared TimeoutBudget, failIfTimeoutBudgetSpent ending the call on the second timed-out attempt with a WARN line, and types the stream deadline as AiReviewTimeoutException carrying the wait
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/AiReviewTimeoutException.java	Added	New AiReviewException subclass distinguishing a deadline miss (and a call ended by repeated ones) from ordinary transient failures, carrying the wall clock the attempts spent
src/test/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipelineTest.java	Modified	Adds a batch-lane test: a batch call ended by repeated timeouts still gets the sequential pass's one retry and its files are disclosed as not reviewed
src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/AiReviewServiceTest.java	Modified	Adds five AiReviewService tests (repeated timeouts end at attempt two, timeout-then-success, timeout mixed with errors keeping the budget, summary lane, WARN capture) plus a verifyReviewStreamCalls helper

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
test	check-run	⏳ Pending	-
format	check-run	⏳ Pending	-

---

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

[thrillhousebot]

Required CI is now green for c507432, so the approval the earlier review held back is posted. The code was not re-reviewed: that review found no issues, and only the CI gate held its approval.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[devops-thiago]

/review

[thrillhousebot]

Everything's coming up Thrillhouse! 🎉

No issues found in this PR.

[devops-thiago]

/pause

[thrillhousebot]

⏸️ ThrillhouseBot is now paused on this PR — automatic and manual reviews are silenced. Comment /resume to re-enable.

[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