feat(ai): Add support for AbortSignal
#8890
Open
Conversation
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
🦋 Changeset detectedLatest commit: 3a63e57 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
dlarocque
force-pushed
the
dl/abort-signal
branch
from
496add1 to
f9950ce
Compare
Contributor
Changeset File Check ✅
|
Contributor
Size Report 1Affected Products
Test Logs |
Contributor
Size Analysis Report 1Affected Products
Test Logs |
dlarocque
force-pushed
the
dl/abort-signal
branch
from
b9a363a to
3e0cc6b
Compare
dlarocque
force-pushed
the
dl/abort-signal
branch
from
b3e7dd8 to
c616128
Compare
DellaBitta
reviewed
May 12, 2025
Contributor
DellaBitta
left a comment
DellaBitta
left a comment
There was a problem hiding this comment.
Looks good, but won't approve until council review.
| If provided, calling `abort()` on the corresponding `AbortController` will attempt to cancel the underlying HTTP request. An `AbortError` will be thrown if cancellation is successful. | ||
| Note that this will not cancel the request in the backend, so billing will still be applied despite cancellation. |
DellaBitta
approved these changes
May 12, 2025
dlarocque
requested review from
NhienLam,
Xiaoshouzi-gh,
aashishpatil-g,
lisajian,
maneesht,
mansisampat,
pashanka,
sam-gc and
tonyjhuang
as code owners
Contributor
Author
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces support for AbortSignal to allow cancellation of AI requests, which is a valuable feature. The implementation correctly updates GenerativeModel, ImagenModel, and ChatSession and includes a comprehensive set of tests for the new cancellation logic. The API and documentation changes are also well-executed.
However, I've identified a few critical issues. The new singleRequestOptions are not implemented for TemplateGenerativeModel and TemplateImagenModel, with TODO comments left in the code. I also found a potential memory leak in the request handling logic and some issues with test structure, such as duplication and improper nesting. I've provided specific comments with suggestions to address these points. Once these are resolved, the pull request will be in excellent shape.
Contributor
Author
|
Re-requesting review since changes were made. |
hsubox76
reviewed
Nov 18, 2025
Contributor
hsubox76
left a comment
hsubox76
left a comment
There was a problem hiding this comment.
Weren't we going to have the iterator throw too if there was an error on that iteration? Does this do that? Did it already do that?
Contributor
Author
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces support for AbortSignal across various methods, allowing for cancellation of in-flight requests. The implementation is robust, using AbortSignal.any() to gracefully handle both user-initiated aborts and internal timeouts. The changes are well-tested with new integration and unit tests. I've left a few comments on potential improvements, mainly around ensuring atomicity of history updates in ChatSession to prevent inconsistent states, and a couple of minor cleanup items in generated documentation files. Overall, this is a great addition to the SDK.
common/api-review/ai.api.md
Outdated
Comment on lines
1341
to
1342
| generateContent(templateId: string, templateVariables: object, // anything! | ||
| singleRequestOptions?: SingleRequestOptions): Promise<GenerateContentResult>; |
Contributor
There was a problem hiding this comment.
There appears to be a leftover comment // anything! in the generateContent method signature. This should be removed for clarity in the public API documentation.
Suggested change
| generateContent(templateId: string, templateVariables: object, // anything! | |
| singleRequestOptions?: SingleRequestOptions): Promise<GenerateContentResult>; | |
| generateContent(templateId: string, templateVariables: object, singleRequestOptions?: SingleRequestOptions): Promise<GenerateContentResult>; |
Comment on lines
88
to
89
| generateContent(templateId: string, templateVariables: object, // anything! | ||
| singleRequestOptions?: SingleRequestOptions): Promise<GenerateContentResult>; |
Contributor
There was a problem hiding this comment.
Similar to the API report file, there's a leftover comment // anything! in the generateContent method signature in this documentation file. It should be removed.
Suggested change
| generateContent(templateId: string, templateVariables: object, // anything! | |
| singleRequestOptions?: SingleRequestOptions): Promise<GenerateContentResult>; | |
| generateContent(templateId: string, templateVariables: object, singleRequestOptions?: SingleRequestOptions): Promise<GenerateContentResult>; |
hsubox76
approved these changes
Dec 1, 2025
Contributor
hsubox76
left a comment
hsubox76
left a comment
There was a problem hiding this comment.
LG, thanks for adding the comments and the tests.
|
|
||
| // Simulate a response that is technically valid enough to resolve aggregation, | ||
| // but malformed in a way that causes the history update logic to throw. | ||
| // Passing `null` as a candidate causes `{ ...response.candidates[0].content }` to throw. |
Contributor
There was a problem hiding this comment.
This is a good test and we should keep it, but surfaces that we should probably fix the logic to protect against response.candidates[0].content throwing. Can you add a todo for me? In chat-session.ts line 188.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Add an
AbortSignalto a newSingleRequestOptionsthat can be passed to all methods that make requests to the backend, allowing them to be aborted.Fixes #8859
API Proposal: go/vinf-abort-request-api (internal)