fix(core): prevent double server.close() and promise rejection in OAuth callback server#24120
Conversation
…th callback server Add guards to prevent ERR_SERVER_NOT_RUNNING errors when server.close() is called multiple times in the OAuth callback server. Also prevent double promise rejection by tracking settlement state. - Add serverClosed guard to ensure server.close() is only called once - Add promiseSettled guard to ensure resolve/reject is only called once - Update all code paths to use the new safe functions - Add error type conversion for non-Error exceptions Fixes google-gemini#24088
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 stability issues in the OAuth callback server where improper handling of server lifecycle events led to runtime errors and promise rejection conflicts. By implementing safe wrappers for server closure and promise settlement, the changes ensure that the server handles concurrent requests and error states gracefully without crashing. 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
|
There was a problem hiding this comment.
Code Review
This pull request improves the reliability of the OAuth callback server by introducing state management to prevent double-closing the server or double-settling the authorization promise. It adds safe wrapper functions for resolving, rejecting, and closing the server, along with comprehensive tests for concurrent requests and error handling. Dependency updates for @vitest/coverage-v8 and modifications to the VS Code companion's license notices are also included. I have no feedback to provide.
|
Hi there! Thank you for your interest in contributing to Gemini CLI. To ensure we maintain high code quality and focus on our prioritized roadmap, we have updated our contribution policy (see Discussion #17383). We only guarantee review and consideration of pull requests for issues that are explicitly labeled as 'help wanted'. All other community pull requests are subject to closure after 14 days if they do not align with our current focus areas. For this reason, we strongly recommend that contributors only submit pull requests against issues explicitly labeled as 'help-wanted'. This pull request is being closed as it has been open for 14 days without a 'help wanted' designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding and for being part of our community! |
Description
This PR fixes issue #24088 where multiple
server.close()calls without guard in the OAuth callback server can throwERR_SERVER_NOT_RUNNINGerrors.Problem
In
packages/core/src/utils/oauth-flow.ts, thestartCallbackServer()function callsserver.close()in multiple code paths without checking if the server has already been closed:Additionally, the
server.on('error')handler calls bothportReject(error)andreject(error), which can cause double promise rejection.Solution
Added guard variables and helper functions:
serverClosedboolean to track if server has been closedpromiseSettledboolean to track if promise has been resolved/rejectedcloseServer()helper that only closes if not already closedsafeResolve()helper that only resolves if not already settledsafeReject()helper that only rejects if not already settledAll code paths now use these safe helpers instead of calling
server.close(),resolve(), orreject()directly.Testing
should handle multiple concurrent requests without double-closing servershould handle rapid error and success without double rejectionshould not throw ERR_SERVER_NOT_RUNNING on state mismatchChecklist
npm run testin packages/core)server.close()calls without guard in OAuth callback server can throwERR_SERVER_NOT_RUNNING#24088