fix(core): forward abortSignal to retryWithBackoff in BaseLlmClient - #29089
fix(core): forward abortSignal to retryWithBackoff in BaseLlmClient#29089chelsealong wants to merge 1 commit into
Conversation
BaseLlmClient.generateContent/generateJson pass abortSignal into each individual API call, but never as the `signal` option of retryWithBackoff. This means a caller-side timeout (e.g. Config's 5s session-summary timeout) only cancels the in-flight request; the retry loop keeps waiting out its full backoff schedule and issuing further attempts regardless, which is why users on custom/proxy endpoints see several retries with backoff and stack-trace spam before the call finally gives up (google-gemini#29065). geminiChat.ts and web-fetch.ts already pass `signal` to retryWithBackoff; this aligns BaseLlmClient with that pattern.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the reliability and responsiveness of LLM client operations by ensuring that cancellation signals are properly propagated through the retry logic. Previously, while individual HTTP requests could be aborted, the retry loop itself would continue to execute for its full duration. By passing the abortSignal to the retry handler, the system now fails fast when a timeout or cancellation occurs, preventing unnecessary background activity and reducing log noise. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
📊 PR Size: size/S
|
There was a problem hiding this comment.
Code Review
This pull request updates BaseLlmClient to forward the caller-provided abortSignal to the retryWithBackoff utility, ensuring that the retry loop honors the abort signal. It also adds a corresponding unit test in baseLlmClient.test.ts to verify this behavior. There are no review comments, and I have no additional feedback to provide.
Relates to #29065
What
BaseLlmClient.generateContent/generateJson(the client used bySessionSummaryService, chat compression, the classifier, promptcompletion, etc.) accept an
abortSignaland thread it into eachindividual API call, but never pass it as the
signaloption ofretryWithBackoff. As a result, a caller-side timeout only aborts thein-flight HTTP request — the retry loop itself is not bounded by it and
keeps waiting out its full exponential-backoff schedule and issuing
further attempts regardless of the timeout having already fired.
geminiChat.tsandweb-fetch.tsalready passsignaltoretryWithBackofffor this exact reason;BaseLlmClientwas theoutlier.
Relationship to #29065
#29065 reports that session summary generation hardcodes
gemini-3.1-flash-lite(via thesummarizer-defaultmodel config indefaultModelConfigs.ts) instead of using the user's configured/activemodel, which causes every summary attempt to fail outright on
custom/proxy endpoints that don't serve that model. This PR does not
fix that. The hardcoded-model issue is the actual root cause reported
in #29065 and needs a separate fix (making
summarizer-default, andlikely the other
UTILITY_*configs with the same pattern, derive fromthe user's configured model).
What this PR does fix is a related but secondary symptom visible in the
same issue's logs: because
BaseLlmClientnever forwardedabortSignalinto
retryWithBackoff,SessionSummaryService's own 5s timeoutcouldn't stop the retry loop, which otherwise runs ~65s
(5s/10s/20s/30s backoff across 5 attempts) printing "Attempt N failed...
Retrying with backoff" stack traces the whole time. With this fix, a
timed-out or unreachable summary model now fails fast and quietly
(
SessionSummaryServicealready handlesAbortErrorby logging atdebug level and returning
null) instead of spamming the terminal forover a minute past its own timeout budget.
In short: after this change, users on custom endpoints will still see
every session summary attempt fail (since it still targets a hardcoded
model their endpoint may not serve), but they'll see it fail in ~5s
with no retry spam instead of ~65s with a wall of stack traces. This
PR should not auto-close #29065 — leaving it open until the
model-hardcoding is addressed.
Change
One line: pass
signal: abortSignalinto theretryWithBackoffcallinside
BaseLlmClient._generateWithRetry.Test plan
Added a regression test in
packages/core/src/core/baseLlmClient.test.tsasserting that
generateContentforwards itsabortSignaltoretryWithBackoffas thesignaloption.Confirmed the test fails without the fix (reverted the one-line change
locally and re-ran):
With the fix applied:
Also ran:
npx eslint packages/core/src/core/baseLlmClient.ts packages/core/src/core/baseLlmClient.test.ts— clean.npx tsc -p packages/core/tsconfig.json --noEmit— clean.AI assistance disclosure
This change (investigation, fix, and test) was authored with the
assistance of an AI coding agent (Claude Code) and reviewed by me
before submission.