fix(core): prevent OAuth callback timeout leak and release resources - #28678
fix(core): prevent OAuth callback timeout leak and release resources#28678Rajeev91691 wants to merge 1 commit into
Conversation
|
📊 PR Size: size/M
|
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 potential memory leaks and stale timeout callbacks in the OAuth flow by centralizing the server settlement and resource cleanup logic. By standardizing how the callback server and associated timers are terminated, the changes ensure a more robust and predictable lifecycle for OAuth operations. 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a centralized cleanup function in the OAuth callback server to ensure that timeouts are cleared, abort event listeners are removed, and the server is closed properly across all success and error paths. It also adds unit tests to verify timeout clearance. However, calling server.close() inside cleanup when the server is not actively listening can throw an ERR_SERVER_NOT_RUNNING error, potentially crashing the process or causing validation failures. It is recommended to guard the server close call with a check on server.listening as suggested.
| const cleanup = () => { | ||
| if (cleanedUp) return; | ||
| cleanedUp = true; | ||
| if (timeoutId) { | ||
| clearTimeout(timeoutId); | ||
| timeoutId = undefined; | ||
| } | ||
| abortController.signal.removeEventListener('abort', onAbort); | ||
| server.close(); | ||
| }; |
There was a problem hiding this comment.
Calling server.close() when the server is not actively listening throws an ERR_SERVER_NOT_RUNNING error in Node.js.
This creates two critical issues in the current implementation:
- Process Crash on Port Collision: If
server.listen()fails (e.g.,EADDRINUSE), the'error'event is emitted. The error handler callscleanup(), which invokesserver.close(). Since the server never successfully started listening, this throwsERR_SERVER_NOT_RUNNINGsynchronously inside the event handler, crashing the entire CLI process. - Validation Failures: If
OAUTH_CALLBACK_PORTis invalid,cleanup()is called beforeserver.listen(). This throwsERR_SERVER_NOT_RUNNINGsynchronously, rejecting the promise with the wrong error and preventingportRejectfrom being called, which can cause callers awaitingserver.portto hang.
To fix this, guard the server.close() call by checking server.listening first.
const cleanup = () => {
if (cleanedUp) return;
cleanedUp = true;
if (timeoutId) {
clearTimeout(timeoutId);
timeoutId = undefined;
}
abortController.signal.removeEventListener('abort', onAbort);
if (server.listening) {
server.close();
}
};
Resolves #28652. Centralizes OAuth callback server settlement and resource cleanup to prevent retaining stale timeout callbacks and memory leaks.