fix(vscode-ide-companion): resolve stop() with an MCP stream open - #29088
fix(vscode-ide-companion): resolve stop() with an MCP stream open#29088chiruu12 wants to merge 1 commit into
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 addresses a critical issue where the IDE server would fail to shut down properly because long-lived MCP streaming connections prevented the server from closing. By explicitly managing keep-alive intervals and forcing the closure of active transports and sockets before server shutdown, the changes ensure that 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
|
|
📊 PR Size: size/M
|
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where stopping the IDE server would block indefinitely when an MCP stream is open. It introduces tracking of keep-alive intervals via a keepAliveIntervals set, ensures active transports are closed, and calls closeAllConnections() on the server during shutdown to allow connections to drain properly. A comprehensive integration test has also been added to verify this behavior. There are no review comments, and I have no additional feedback to provide.
Fixes #28785.
IdeServer.stop()awaitsthis.server.close(). That callback fires only once every established connection has drained, and the MCP transport holds a long-lived streaming response onGET /mcp, so nothing drains.stop()never resolves and extension deactivate blocks behind it. VS Code then reports the extension as unresponsive on shutdown.What changed
stop()now clears the keep-alive intervals, closes the open transports, and callscloseAllConnections()alongsideclose(). Closing the transports is what lets the stream drain;closeAllConnections()releases anything still holding a socket.Keep-alive intervals are tracked in a
Setthat is written immediately aftersetIntervalreturns, not inonsessioninitialized. A transport whose handshake never completes is in neithertransportsnor any session map, so an interval registered at initialization time would leave that timer running against a server that is already gone.The
missedPings >= 3branch has always loggedClosing connection and cleaning up interval, but only cleared the interval. It now closes the transport too, which is what the message says and what drops the session.What I did not change
The
missedPings = 0reset on success. The issue I filed lists it, and on reflection consecutive-miss counting is a reasonable reading of "missed pings" rather than a bug. Changing the threshold semantics is a policy call, it is what took the previous attempt at this out of scope, and the leak it was worried about is closed bystop()clearing every interval regardless.On the test, and what it does not prove
The raw mechanism does reproduce on this Node. A plain server holding one streaming response, then
close():The added test opens a real session, holds a real
GET /mcpstream, and assertsstop()resolves. Being straight about it: that assertion alone passes with or without this change in the vitest harness, so on its own it is a regression guard, not a reproduction.What makes it fail-first is the second assertion, that
stop()logsSession closed: <id>. That only happens ifstop()actually closes the transport, andoncloseis what clears the keep-alive interval and drops the session. Revertingstop()to its previous body and keeping the test fails on exactly that line. An idle socket does not reproduce anything either, sinceclose()destroys those, which is why the test does the full handshake.Gates
npm run lintandnpm run typecheckclean.vitest run src/inpackages/vscode-ide-companion: 41 passed, 1 skipped.Two files, no API changes.