Conversation
…amD/gemini-cli into fix/web-fetch-ctrl-c-abort
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 addresses an issue where user-initiated cancellations (such as Ctrl+C) during a web fetch request were incorrectly handled as transient network timeouts. By refining the error handling logic to identify user-triggered aborts, the system now cancels immediately instead of attempting multiple retries, significantly improving user experience and telemetry accuracy. 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 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. Footnotes
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive implementation plan for silencing passing test output to improve the cleanliness of the preflight process. It also includes a specific fix in the core fetch utility to correctly distinguish between internal timeouts and user-initiated cancellations. A review comment suggests improving error handling by re-throwing the original abort reason or the caught error instead of creating a generic Error object, which helps preserve the stack trace and debugging information.
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive implementation plan for silencing passing test output and includes a specific fix for handling aborted fetch requests. The fetchWithTimeout utility was updated to distinguish between internal timeouts and user-initiated cancellations, ensuring that aborted signals are not incorrectly treated as retryable timeouts. A review comment suggests improving the error handling in fetch.ts by rethrowing the original abort reason or the error itself to preserve the stack trace and context, rather than creating a new generic Error object.
| const abortError = new Error('The operation was aborted.'); | ||
| abortError.name = 'AbortError'; | ||
| throw abortError; |
There was a problem hiding this comment.
Instead of creating a new generic Error object, it is better to rethrow the original error or the reason from the AbortSignal. This preserves the original stack trace and any custom abort reason (e.g., a specific cancellation message) provided by the caller. Since the project targets Node.js >= 20, options.signal.reason is available and is the standard way to propagate the cause of an abortion.
| const abortError = new Error('The operation was aborted.'); | |
| abortError.name = 'AbortError'; | |
| throw abortError; | |
| throw options.signal.reason ?? error; |
References
- Asynchronous operations that can be cancelled by the user should accept and propagate an AbortSignal to ensure cancellability and prevent dangling processes or network requests.
- When catching exceptions, log the detailed error for debugging instead of providing only a generic error message.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Problem
When
web_fetchis actively loading a URL and the user presses Ctrl+C, the CLI does not cancel immediately. Instead:ETIMEDOUTinstead of user cancellationRoot Cause
In
fetchWithTimeout, when the abort controller fires:AbortControllerAbortErroris unconditionally converted toFetchErrorwith codeETIMEDOUTETIMEDOUTis inRETRYABLE_NETWORK_CODES,retryWithBackofftreats user cancellation as a transient network failureSolution
The fix distinguishes between user-initiated cancellation and internal timeouts:
In
fetchWithTimeout: Check if the external signal was abortedAbortError(user cancellation)FetchErrorwithETIMEDOUT(timeout)In
retryWithBackoff: Immediately re-throwAbortErrorwithout retryingResult: Ctrl+C cancels immediately without retry delays
Changes Made
packages/core/src/utils/fetch.tsfetchWithTimeoutto detect when the caller's signal was abortedAbortErrorinstead ofFetchErrorwithETIMEDOUTpackages/core/src/utils/fetch.test.tsAbortError(not timeout)fixes:
fixes #21546
Testing
Affected Scenarios
✅ Fixed: Pressing Ctrl+C during active web_fetch request
✅ Fixed: Agent abort signals are now properly handled
✅ Fixed: Telemetry now correctly reports user cancellation (not timeout)