fix(core): prevent unhandled AbortError crash during stream loop detection#21123
Conversation
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 resolves a critical bug in the CLI that caused unhandled Highlights
Changelog
Activity
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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses a crash caused by an unhandled AbortError during stream processing when a loop is detected. The fix defers aborting the stream's controller until after the stream iteration loop is exited, preventing the error. A new unit test is added to verify this behavior and prevent regressions. The changes are confined to packages/core/src/core/client.ts and its corresponding test file.
There was a problem hiding this comment.
Code Review
This pull request addresses a critical unhandled AbortError crash that occurs during stream processing when a loop is detected. The fix correctly defers the call to controller.abort() until after the stream's for await loop has been safely exited using a break statement, preventing the unhandled promise rejection. The changes are sound, and the new unit test effectively validates that the fix works as expected without introducing regressions.
|
Thanks for the fix! It looks good, but there is a merge conflict in |
|
@ruomengz, I've resolved the conflicts and restructured the code to align with the updated architecture. |
There was a problem hiding this comment.
Code Review
This pull request addresses a crash that occurred when an AbortError was thrown from within an active asynchronous stream iteration. The fix involves deferring the abort() call until after the stream's for await...of loop has terminated. This is achieved by setting a flag within the loop and then acting on it after the loop breaks. This pattern is also applied to the loop recovery logic, which could also trigger an abort. A new unit test has been added to verify that the stream now completes gracefully under these conditions.
|
@ruomengz , Could you please run CI? |
|
@ruomengz , A particular test fails due to a timeout, what should I do? |
…ction (google-gemini#21123) Co-authored-by: Gaurav <39389231+gsquared94@users.noreply.github.com> Co-authored-by: ruomeng <ruomeng@google.com>
…ction (google-gemini#21123) Co-authored-by: Gaurav <39389231+gsquared94@users.noreply.github.com> Co-authored-by: ruomeng <ruomeng@google.com>
…ction (google-gemini#21123) Co-authored-by: Gaurav <39389231+gsquared94@users.noreply.github.com> Co-authored-by: ruomeng <ruomeng@google.com>
…ction (google-gemini#21123) Co-authored-by: Gaurav <39389231+gsquared94@users.noreply.github.com> Co-authored-by: ruomeng <ruomeng@google.com>
…ction (google-gemini#21123) Co-authored-by: Gaurav <39389231+gsquared94@users.noreply.github.com> Co-authored-by: ruomeng <ruomeng@google.com>
Summary
This PR resolves a critical bug where the CLI crashes with an unhandled
AbortErrorduring stream processing.Details
The crash occurred in
processTurnbecausecontroller.abort()was being invoked directly within thefor await (const event of resultStream)iteration upon detecting a stream loop. This caused the underlying active stream to immediately throw anAbortError. Lacking atry...catchblock, this resulted in an unhandled promise rejection that abruptly terminated the Node.js process.To resolve this structurally, a
loopDetectedAbortboolean flag was introduced. The logic now sets this flag and usesbreakto safely exit the asynchronous iteration first. Thecontroller.abort()method is then called after the loop has gracefully terminated, preventing the exception from leaking into the active stream processing state.Related Issues
Fixed #21122
How to Validate
Expected Result: Tests should pass successfully without triggering any unhandled AbortError rejections.
Pre-Merge Checklist