diff --git a/.env.example b/.env.example index 86b30123..f20378c8 100644 --- a/.env.example +++ b/.env.example @@ -113,7 +113,9 @@ GITHUB_WEBHOOK_SECRET=your_webhook_secret # Max inline comments posted per review: #THRILLHOUSEBOT_REVIEW_MAX_REVIEW_COMMENTS=50 # AI call retry policy (attempts, exponential backoff base) and per-attempt client-side wait -# (keep the wait >= AI_TIMEOUT so timed-out attempts don't leave orphaned provider streams): +# (keep the wait >= AI_TIMEOUT so timed-out attempts don't leave orphaned provider streams). +# At most two attempts of one call may end at the wait; the call fails there rather than spending +# the rest of its retries on a request that already missed the deadline: #THRILLHOUSEBOT_REVIEW_MAX_AI_RETRIES=5 #THRILLHOUSEBOT_REVIEW_AI_RETRY_BASE_DELAY_MS=2000 #THRILLHOUSEBOT_REVIEW_AI_TIMEOUT_SECONDS=300 diff --git a/CHANGELOG.md b/CHANGELOG.md index daddae93..b048b54a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ All notable changes to ThrillhouseBot. ### Fixed +- **Repeated timeouts on one AI call stop spending the whole retry budget** (#862): a streaming attempt waits `THRILLHOUSEBOT_REVIEW_AI_TIMEOUT_SECONDS`, 900 seconds in production, and a timeout was then retried like any other transient failure up to `THRILLHOUSEBOT_REVIEW_MAX_AI_RETRIES`, so one call could spend 75 minutes of wall clock while its review held the pull request's dispatcher slot. Production saw 20 timed-out attempts in 24 hours, all on one 503-file pull request, and the failed reviews of that day cost more than the completed ones. At most two attempts of one call may now end at the deadline: the second one fails the call instead of spending the attempts that are left, and the decision is logged at WARN with the session id, the attempt and the wait. Every other transient failure keeps the whole budget, a timeout followed by a successful attempt still succeeds, and the reasoning step-down's repeat (#839) shares the bound rather than getting a second pair of waits. The repeat keeps the full deadline, since it is there for the attempt whose first token never arrived because the provider queued the request, and the bound already brings the ceiling down from 75 minutes to 30. The setting still means one attempt's wait. The final summary call shares the loop and behaves the same way, and a review whose batches time out still discloses the files it did not read - **A review interrupted by a restart no longer stays `in_progress` for good** (#863): a session row is written `in_progress` when the review starts and updated when it ends, so a review killed between those two writes — a deploy restart, a crash, a `docker kill` — never got the terminal one and stayed `in_progress` for the life of the database. Production had 10 such rows, the oldest from 2026-06-09 and the newest from the 2026-09-09 restart, each counting as a running review and hiding the genuinely in-flight ones among them. Startup now moves every `in_progress` row to `failed` with "Review interrupted before it finished (bot restart or crash)" as the reason, which is what tells it apart on the dashboard from a review that failed on its own. Nothing carries a review across a restart, so a row still in progress at boot belongs to a review that is over: the sweep needs no age threshold and reconciles the rows stranded before it existed, with no manual SQL. Only the status and the reason are written, so the tokens and the cost the review had already paid for stay on the row. Sweeping every row at boot is safe because the bot is a single process; running more than one replica would need the rows to carry an owner first ## [0.6.8] — 2026-09-15 diff --git a/README.md b/README.md index 1fe3b79c..c5fe85f8 100644 --- a/README.md +++ b/README.md @@ -331,9 +331,9 @@ will change per provider: | `REVIEW_MAX_TOKENS_PER_REVIEW` | Ceiling on the tokens one review may consume across every AI call it makes — actual input+output as the provider reports them, counting retries and the final summary call, where `REVIEW_MAX_AI_CALLS` only counts planned calls. Once reached no further review call is made: remaining batches are disclosed by name as not reviewed (the verdict holds and the summary names the ceiling as the reason) and the summary degrades to a counts-only rendering that keeps the findings already paid for. `0` disables the ceiling. Review path only — the on-demand commands keep their own call cap | `0` | | `REVIEW_MAX_DIFF_LINES` | Line cap on single-call diff renders (replies, base comparison, budgeting-disabled review). Token-budgeted reviews and the batched commands — `/improve`, `/describe`, `/changelog`, `/generate-tests`, `/add-docs` — ignore it (the planner owns coverage by tokens); `0` disables the cap | `5000` | | `THRILLHOUSEBOT_REVIEW_MAX_REVIEW_COMMENTS` | Maximum inline comments posted per review; findings over the cap are surfaced in the summary instead of dropped | `50` | -| `THRILLHOUSEBOT_REVIEW_MAX_AI_RETRIES` | Attempts per failed AI call before the review errors out | `5` | +| `THRILLHOUSEBOT_REVIEW_MAX_AI_RETRIES` | Attempts per failed AI call before the review errors out. At most two of them may end at `THRILLHOUSEBOT_REVIEW_AI_TIMEOUT_SECONDS`; the rest of the budget is for the other transient failures | `5` | | `THRILLHOUSEBOT_REVIEW_AI_RETRY_BASE_DELAY_MS` | Base delay of the exponential retry backoff, in milliseconds | `2000` | -| `THRILLHOUSEBOT_REVIEW_AI_TIMEOUT_SECONDS` | Client-side wait per AI streaming attempt; keep it >= `AI_TIMEOUT` so timed-out attempts don't leave orphaned provider streams | `300` | +| `THRILLHOUSEBOT_REVIEW_AI_TIMEOUT_SECONDS` | Client-side wait per AI streaming attempt; keep it >= `AI_TIMEOUT` so timed-out attempts don't leave orphaned provider streams. One call may spend it twice at most: a second attempt that also times out fails the call instead of waiting out the remaining retries | `300` | | `THRILLHOUSEBOT_REVIEW_INSTRUCTIONS_FILE` | Repo-relative path of the per-repo instructions file read on each review | `.github/thrillhousebot.md` | | `THRILLHOUSEBOT_REVIEW_IGNORED_FILES` | Comma-separated gitignore-style globs excluded from review — lockfiles, generated code, build output. A pattern with no `/` matches at any depth (`*.lock`, `vendor`); one carrying a `/` is anchored to the repository root (`docs/generated/**`); a trailing `/` means that directory's tree; `*` does not cross `/`, so use `**` to span directories. The value is comma-separated, so write a `{a,b}` alternation as separate patterns. Replaces (not extends) the default list, so re-include the defaults you still want | lockfiles, generated and minified code, sourcemaps, and build or vendor trees; the full list is the shipped value of `thrillhousebot.review.ignored-files` in `application.properties` | | `THRILLHOUSEBOT_REVIEW_REPO_CONFIG_ENABLED` | Let each repository extend the ignore list with globs of its own, scope review rules to a path, and name its coverage-report artifact, from `.github/thrillhousebot.yml` (see [Repository configuration](#repository-configuration)). Both are additive; set `false` to make the deployment list and the global instructions the only ones that count | `true` | diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/AiReviewService.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/AiReviewService.java index 1edbbef9..3ab29ee5 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/AiReviewService.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/AiReviewService.java @@ -57,6 +57,20 @@ public class AiReviewService { private static final int TOO_MANY_REQUESTS = 429; + /** + * How many attempts of one logical call may end at the streaming deadline (#862): the first and + * one repeat. Every other transient failure keeps the whole {@code max-ai-retries} budget. A + * timed-out attempt is the most expensive failure the loop can have — it spends {@code + * ai-timeout-seconds} in full, 15 minutes in production, and the review holds its pull request's + * place in the dispatcher throughout — and it is the failure least likely to go away on a repeat, + * because a prompt the model did not finish in 15 minutes is the same prompt on the next attempt. + * Production saw one 503-file pull request spend 20 such attempts in a day. The repeat is kept + * because a first token that never arrived can also be the provider queueing the request, which + * the next attempt need not repeat; at the shipped five retries the ceiling was 75 minutes of + * waiting per call, and it is 30 now. + */ + private static final int MAX_TIMED_OUT_ATTEMPTS = 2; + /** Wording of Ollama cloud's refusal when no concurrent request slot freed in time (#838). */ private static final String CONCURRENT_SLOT_REFUSAL = "concurrent request slot"; @@ -174,14 +188,20 @@ private TokenStream reviewStream(PromptInputs inputs) { *

The repeat's calls are bound as reasoning-disabled on the thread that starts them (see * {@link #streamOnce}); {@link ReasoningStepDownStreamingModel} reads that binding and sends * {@code reasoning_effort=none} on those calls alone. + * + *

The timed-out attempts are counted here rather than inside the loop, so the bound of {@link + * #MAX_TIMED_OUT_ATTEMPTS} is on the logical call and both passes share it (#862). A call that + * already waited out one deadline before its length stop has one attempt left to wait out + * another, whether or not reasoning is still on for it. */ private ReviewResponse runWithRetries( ReviewSession session, Supplier streamFactory, boolean broadcastTokens, ModelLane lane) { + var timeouts = new TimeoutBudget(); try { - return attemptWithRetries(session, streamFactory, broadcastTokens, lane, false); + return attemptWithRetries(session, streamFactory, broadcastTokens, lane, false, timeouts); } catch (AiResponseTruncatedException e) { var effort = reasoningEffortToStepDownFrom(e, lane); if (effort.isEmpty()) { @@ -196,7 +216,7 @@ private ReviewResponse runWithRetries( broadcaster.broadcast( SessionEventBroadcaster.SessionEvent.retry( session, 1, config.review().maxAiRetries(), REASONING_STEP_DOWN_REASON)); - return attemptWithRetries(session, streamFactory, broadcastTokens, lane, true); + return attemptWithRetries(session, streamFactory, broadcastTokens, lane, true, timeouts); } } @@ -235,7 +255,8 @@ private ReviewResponse attemptWithRetries( Supplier streamFactory, boolean broadcastTokens, ModelLane lane, - boolean reasoningDisabled) { + boolean reasoningDisabled, + TimeoutBudget timeouts) { var maxAttempts = config.review().maxAiRetries(); RuntimeException lastFailure = null; @@ -284,6 +305,7 @@ private ReviewResponse attemptWithRetries( broadcaster.broadcast( SessionEventBroadcaster.SessionEvent.streamFailed( session, attempt, maxAttempts, sanitize(e))); + failIfTimeoutBudgetSpent(session, attempt, maxAttempts, e, timeouts); } } @@ -291,6 +313,76 @@ private ReviewResponse attemptWithRetries( "AI review failed after " + maxAttempts + " attempts", maxAttempts, lastFailure); } + /** + * Ends the call when this failure is the streaming deadline and the call has no timed-out attempt + * left (#862), after the attempt has been logged and broadcast like any other transient failure. + * Returns for every other failure, and for a timeout the call may still repeat. + * + *

Only the client-side deadline counts. A call that found no free model call slot (#838) was + * never sent, and a timeout the provider itself reports arrives as a stream error; both are + * ordinary transient failures and keep the ordinary budget. + * + * @throws AiReviewTimeoutException naming the attempts made and the wall clock they spent, when + * the bound is reached + */ + private static void failIfTimeoutBudgetSpent( + ReviewSession session, + int attempt, + int maxAttempts, + RuntimeException failure, + TimeoutBudget timeouts) { + if (!(failure instanceof AiReviewTimeoutException timeout) + || !timeouts.spend(timeout.waited())) { + return; + } + Log.warnf( + "AI review attempt %d/%d for session %d timed out after %s; that is the last of the %d" + + " timed-out attempts one call may have (%s 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", + attempt, + maxAttempts, + session.id, + timeout.waited(), + MAX_TIMED_OUT_ATTEMPTS, + timeouts.waited()); + throw new AiReviewTimeoutException( + "AI review failed after " + + attempt + + " attempts, " + + timeouts.timedOut() + + " of which timed out", + attempt, + timeouts.waited(), + timeout); + } + + /** + * The timed-out attempts of one logical call and the wall clock they spent (#862). Not thread + * safe by design: one logical call runs its attempts one after another on a single thread, and + * parallel batches each run their own call with a budget of their own. + */ + private static final class TimeoutBudget { + + private int timedOut; + private Duration waited = Duration.ZERO; + + /** Records a timed-out attempt and reports whether the call has none left. */ + boolean spend(Duration wait) { + timedOut++; + waited = waited.plus(wait); + return timedOut >= MAX_TIMED_OUT_ATTEMPTS; + } + + int timedOut() { + return timedOut; + } + + Duration waited() { + return waited; + } + } + /** * The prompt sections sent to the model for one review, pre-escaped for templating. The * instructions section arrives pre-rendered with its header and source attribution. @@ -368,6 +460,7 @@ private ReviewResponse streamInSlot( // reasoning disabled (#839). Both run before the stream returns, on this same thread. ReviewSessionContext.bind(session.id, attempt, reasoningDisabled); var callId = ReviewSessionContext.currentCallId(); + var deadline = streamTimeout(); TokenStream stream = null; try { stream = streamFactory.get(); @@ -382,12 +475,14 @@ private ReviewResponse streamInSlot( .onError(error -> handleStreamError(error, result, flushStream, cancelled)) .start(); - return result.get(streamTimeout().toMillis(), TimeUnit.MILLISECONDS); + return result.get(deadline.toMillis(), TimeUnit.MILLISECONDS); } catch (TimeoutException e) { cancelled.set(true); cancelStream(stream, session.id, attempt); flushStream.run(); - throw new AiReviewException("AI review timed out after " + streamTimeout(), 1, e); + // Typed apart from the other transient failures: the retry loop bounds how many attempts of + // one call may spend the deadline, and it needs the wait this one spent to say so (#862). + throw new AiReviewTimeoutException("AI review timed out after " + deadline, 1, deadline, e); } catch (ExecutionException e) { throw asAiReviewException(e); } catch (InterruptedException e) { diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/AiReviewTimeoutException.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/AiReviewTimeoutException.java new file mode 100644 index 00000000..9ff1a89d --- /dev/null +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/AiReviewTimeoutException.java @@ -0,0 +1,54 @@ +/* + * Copyright 2026 Thiago Gonzaga + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package dev.thiagogonzaga.thrillhousebot.review.ai; + +import java.time.Duration; + +/** + * A streaming attempt that reached its client-side deadline ({@code + * THRILLHOUSEBOT_REVIEW_AI_TIMEOUT_SECONDS}) without a complete response, and the failure of a call + * that ended because too many of its attempts did (#862). + * + *

Named apart from a plain {@link AiReviewException} so the retry loop can tell a deadline from + * the other transient failures it retries on the same terms. A deadline says something about the + * request and not only about the provider: a prompt that did not finish inside it is unlikely to + * finish inside another one, and every repeat costs the whole deadline while the review holds its + * pull request's place in the dispatcher. Production spent 75 minutes of wall clock that way on a + * single 503-file pull request, so {@link AiReviewService} bounds how many attempts of one logical + * call may end here. + * + *

Still an {@link AiReviewException}: to everything downstream this is a call that was attempted + * and did not produce a response, so the degradations that keep a review's paid work when its + * summary call fails (#851) and the batch lane's disclosure of the files it could not read apply + * unchanged. + */ +public class AiReviewTimeoutException extends AiReviewException { + + private final Duration waited; + + public AiReviewTimeoutException(String message, int attempts, Duration waited, Throwable cause) { + super(message, attempts, cause); + this.waited = waited; + } + + /** + * How long the attempts this failure describes spent at the deadline: one attempt's wait when a + * single attempt timed out, the sum over the call's timed-out attempts when the bound ended it. + */ + public Duration waited() { + return waited; + } +} diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipelineTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipelineTest.java index 0eecaa9d..915c7a01 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipelineTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipelineTest.java @@ -44,12 +44,14 @@ import dev.thiagogonzaga.thrillhousebot.review.ai.AiResponseTruncatedException; import dev.thiagogonzaga.thrillhousebot.review.ai.AiReviewException; import dev.thiagogonzaga.thrillhousebot.review.ai.AiReviewService; +import dev.thiagogonzaga.thrillhousebot.review.ai.AiReviewTimeoutException; import dev.thiagogonzaga.thrillhousebot.review.ai.FindingVerificationService; import dev.thiagogonzaga.thrillhousebot.review.ai.PrReviewPrompts; import dev.thiagogonzaga.thrillhousebot.review.ai.ReviewResponse; import dev.thiagogonzaga.thrillhousebot.review.ai.ReviewTokenLedger; import dev.thiagogonzaga.thrillhousebot.review.ai.TokenCounter; import dev.thiagogonzaga.thrillhousebot.review.ai.TokenSpendCeilingExceededException; +import java.time.Duration; import java.util.ArrayList; import java.util.List; import java.util.Map; @@ -511,6 +513,39 @@ void multiCallDoesNotRetryABatchRejectedForExceedingTheContextWindow() { assertTrue(captor.getValue().changedFiles().contains("a.java (not reviewed")); } + @Test + void multiCallDisclosesABatchWhoseCallGaveUpOnItsTimedOutAttempts() { + // #862: a call the AI service ended on its timed-out attempts is an ordinary spent call here. + // 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 files are disclosed when that attempt fails too. + var session = ReviewSession.create("owner/repo", 1, "Big PR", "sha"); + var ctx = reviewContext(); + var template = new AiReviewService.PromptInputs("d", "ctx", "base", "stack", "tests", "", ""); + when(aiReviewService.reviewBatch(eq(session), any(), eq(1), anyInt())) + .thenThrow( + new AiReviewTimeoutException( + "AI review failed after 2 attempts, 2 of which timed out", + 2, + Duration.ofMinutes(30), + null)); + when(aiReviewService.reviewBatch(eq(session), any(), eq(2), anyInt())) + .thenReturn(new ReviewResponse(List.of(finding("b.java", "B")), List.of(), null)); + var summary = new ReviewResponse.Summary(1, 0, 0, 1, 0, "ok", "does things", List.of()); + var captor = ArgumentCaptor.forClass(AiReviewService.SummaryInputs.class); + when(aiReviewService.summarize(eq(session), captor.capture())) + .thenReturn(new ReviewResponse(List.of(), List.of(), summary)); + + var plan = multiBatchPlan(); + var result = pipeline.run(session, template, ctx, plan, new DiffLineResolver(Map.of())); + + verify(aiReviewService, times(2)).reviewBatch(eq(session), any(), eq(1), anyInt()); + assertEquals(1, result.findings().size()); + assertEquals("B", result.findings().get(0).title()); + assertEquals(List.of("a.java"), plan.runtimeUncoveredFiles()); + assertTrue(captor.getValue().changedFiles().contains("a.java (not reviewed")); + } + /** One complete finding element for a stubbed partial body, anchored in batch 1's file. */ private static String bodyFinding(String title) { return "{\"risk\":\"medium\",\"confidence\":\"high\",\"file\":\"a.java\",\"line\":1," diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/AiReviewServiceTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/AiReviewServiceTest.java index 5a57d8cb..627c23dd 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/AiReviewServiceTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/AiReviewServiceTest.java @@ -40,6 +40,10 @@ import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; import java.util.function.BooleanSupplier; +import java.util.logging.Handler; +import java.util.logging.Level; +import java.util.logging.LogRecord; +import java.util.logging.Logger; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -472,6 +476,128 @@ void shouldTimeoutWhenStreamNeverCompletes() { assertTrue(ex.getCause().getMessage().contains("timed out")); } + @Test + void repeatedTimeoutsEndTheCallInsteadOfSpendingTheRetryBudget() { + // #862: every attempt spends the whole deadline while the review holds its pull request's + // dispatcher slot, so one repeat is all a call gets before the deadline is taken as the + // answer. The five configured retries used to be spent on it, 75 minutes in production. + ReviewSession session = reviewSession(); + when(reviewConfig.maxAiRetries()).thenReturn(5); + when(reviewConfig.aiTimeoutSeconds()).thenReturn(1); + stubReviewStreams(new HangingTokenStream()); + + AiReviewException ex = + assertThrows(AiReviewException.class, () -> service.review(session, PROMPT_INPUTS)); + + assertEquals(2, ex.attempts()); + assertTrue(ex.getCause().getMessage().contains("timed out")); + verifyReviewStreamCalls(2); + } + + /** + * A pull request that reliably times out has to be visible without reading every line, so the + * decision to end the call names the session, the attempt and the wait (#862). The records arrive + * already formatted, so the assertions read the rendered line. + */ + @Test + void theCallEndedByRepeatedTimeoutsIsLoggedWithSessionAttemptAndWait() { + ReviewSession session = reviewSession(); + when(reviewConfig.maxAiRetries()).thenReturn(5); + when(reviewConfig.aiTimeoutSeconds()).thenReturn(1); + stubReviewStreams(new HangingTokenStream()); + var logger = Logger.getLogger(AiReviewService.class.getName()); + var logged = new CopyOnWriteArrayList(); + var capture = + new Handler() { + @Override + public void publish(LogRecord entry) { + logged.add(entry); + } + + @Override + public void flush() { + // Nothing is buffered. + } + + @Override + public void close() { + // Nothing to release. + } + }; + logger.addHandler(capture); + + try { + assertThrows(AiReviewException.class, () -> service.review(session, PROMPT_INPUTS)); + } finally { + logger.removeHandler(capture); + } + + var decision = + logged.stream() + .filter(entry -> Level.WARNING.equals(entry.getLevel())) + .map(LogRecord::getMessage) + .filter(line -> line.contains("fails here")) + .findFirst(); + assertTrue(decision.isPresent(), "expected a WARN naming the decision to end the call"); + assertTrue(decision.get().contains("attempt 2/5"), decision.get()); + assertTrue(decision.get().contains("session 42"), decision.get()); + assertTrue(decision.get().contains("PT2S"), decision.get()); + } + + @Test + void aTimeoutFollowedByASuccessfulAttemptStillSucceeds() { + // The repeat is why the bound is two and not one: a first token that never arrived can be the + // provider queueing the request, and the next attempt need not repeat it. + ReviewSession session = reviewSession(); + when(reviewConfig.aiTimeoutSeconds()).thenReturn(1); + stubReviewStreams(new HangingTokenStream(), new FakeTokenStream("{\"findings\":[]}")); + when(parser.parse(anyString())).thenReturn(new ReviewResponse(List.of(), List.of(), null)); + + assertNotNull(service.review(session, PROMPT_INPUTS)); + + verifyReviewStreamCalls(2); + } + + @Test + void aTimeoutDoesNotShortenTheBudgetOfOtherTransientFailures() { + // Only the deadline is bounded (#862). A provider error says nothing about the request, so a + // call that timed out once keeps all five attempts for the failures that are worth repeating. + ReviewSession session = reviewSession(); + when(reviewConfig.maxAiRetries()).thenReturn(5); + when(reviewConfig.aiTimeoutSeconds()).thenReturn(1); + stubReviewStreams( + new ErrorTokenStream(new RuntimeException("provider down")), + new HangingTokenStream(), + new ErrorTokenStream(new RuntimeException("provider down")), + new ErrorTokenStream(new RuntimeException("provider down")), + new FakeTokenStream("{\"findings\":[]}")); + when(parser.parse(anyString())).thenReturn(new ReviewResponse(List.of(), List.of(), null)); + + assertNotNull(service.review(session, PROMPT_INPUTS)); + + verifyReviewStreamCalls(5); + } + + @Test + void repeatedTimeoutsEndTheSummaryCallOnTheSameTerms() { + // The summary call shares the retry loop, so it is bounded by the same count; its failure + // still degrades to the counts-only summary that keeps the paid findings (#851). + ReviewSession session = reviewSession(); + when(reviewConfig.maxAiRetries()).thenReturn(5); + when(reviewConfig.aiTimeoutSeconds()).thenReturn(1); + when(prSummarizer.summarizeStream( + anyString(), anyString(), anyString(), anyString(), anyString())) + .thenReturn(new HangingTokenStream()); + var inputs = new AiReviewService.SummaryInputs("ctx", "[]", "files", "", ""); + + AiReviewException ex = + assertThrows(AiReviewException.class, () -> service.summarize(session, inputs)); + + assertEquals(2, ex.attempts()); + verify(prSummarizer, times(2)) + .summarizeStream(anyString(), anyString(), anyString(), anyString(), anyString()); + } + @Test void shouldParseFromCompleteResponseWhenNoPartialTokens() { ReviewSession session = reviewSession(); @@ -1776,6 +1902,18 @@ private void stubReviewStreams(TokenStream first, TokenStream... then) { .thenReturn(first, then); } + private void verifyReviewStreamCalls(int expected) { + verify(prReviewer, times(expected)) + .reviewStream( + anyString(), + anyString(), + anyString(), + anyString(), + anyString(), + anyString(), + anyString()); + } + /** * Spins until {@code condition} holds. A bounded poll on observable state rather than a timed * sleep, so what the test then asserts never depends on how long a thread took to be scheduled.