mirror of
https://github.com/badlogic/pi-mono.git
synced 2026-11-05 23:37:25 +00:00
fix(mcp): close remaining connections when the OAuth callback server closes (#11855)
After a successful MCP OAuth sign-in, `/mcp login` could stay on "Connecting…" for minutes. `OAuthCallbackServer.close()` awaited `http.Server.close()`, which resolves only after every socket has closed. Browsers may keep an unused connection open to the callback port (a preconnect, or a spare keep-alive socket). Node's idle-connection cleanup doesn't close that socket, and `close()` also stops Node's request timeout checks. So the wait lasted until the browser dropped the socket. `signInMcpServer` awaits this cleanup before reconnecting, and cancellation doesn't apply to it, so Ctrl+C did nothing. `close()` now calls `server.closeAllConnections()` after starting the shutdown. By the time the server closes, the callback response has already been sent, so dropping the remaining connections is safe. **Testing** - Added a test to `packages/mcp/test/oauth.test.ts`: it opens a spare TCP connection to the callback port, completes a callback, and expects `close()` to resolve within 2 s. It fails without the change and passes with it. - `npm run check` passes. The MCP OAuth tests and the coding-agent MCP tests pass. - Script reproduction: with a spare connection open, `close()` did not resolve within 400 s before the change, and resolves in about 1 ms after it. Fixes #11311 Requested by @petrroll Co-authored-by: triagent <triagent@localhost>
This commit is contained in:
parent
80d4d3c869
commit
63ea042ce5
2 changed files with 26 additions and 1 deletions
|
|
@ -107,9 +107,13 @@ export class OAuthCallbackServer {
|
|||
pending.reject(new Error("OAuth callback server closed"));
|
||||
}
|
||||
this.pending.clear();
|
||||
await new Promise<void>((resolve, reject) => {
|
||||
const closed = new Promise<void>((resolve, reject) => {
|
||||
this.server.close((error) => (error ? reject(error) : resolve()));
|
||||
});
|
||||
// `close()` waits for every connection, and a browser may keep an unused one open (a preconnect or a
|
||||
// spare keep-alive socket) for minutes. Responses have been sent by now, so drop them.
|
||||
this.server.closeAllConnections();
|
||||
await closed;
|
||||
}
|
||||
|
||||
private reply(response: ServerResponse, status: number, page: OAuthCallbackPage): void {
|
||||
|
|
|
|||
|
|
@ -1,4 +1,6 @@
|
|||
import { createHash } from "node:crypto";
|
||||
import { once } from "node:events";
|
||||
import { createConnection } from "node:net";
|
||||
import { afterEach, describe, expect, it } from "vitest";
|
||||
import { McpClient, StreamableHttpTransport } from "../src/index.ts";
|
||||
import {
|
||||
|
|
@ -622,6 +624,25 @@ describe("OAuthCallbackServer pages", () => {
|
|||
}
|
||||
});
|
||||
|
||||
// #11311
|
||||
it("closes while the browser keeps an unused connection open", async () => {
|
||||
const callback = await OAuthCallbackServer.listen();
|
||||
const url = new URL(callback.redirectUrl);
|
||||
const spare = createConnection({ host: url.hostname, port: Number(url.port) });
|
||||
try {
|
||||
await once(spare, "connect");
|
||||
const pending = callback.waitForCallback("s1");
|
||||
const response = await fetch(`${callback.redirectUrl}?code=abc&state=s1`);
|
||||
expect(await response.text()).toBe("Authorization complete. You may close this window.");
|
||||
expect((await pending).code).toBe("abc");
|
||||
const closed = callback.close().then(() => "closed");
|
||||
const timeout = new Promise((resolve) => setTimeout(() => resolve("timed out"), 2000));
|
||||
expect(await Promise.race([closed, timeout])).toBe("closed");
|
||||
} finally {
|
||||
spare.destroy();
|
||||
}
|
||||
});
|
||||
|
||||
// #10302
|
||||
it("rejects a response on another path than the expected one", async () => {
|
||||
const callback = await OAuthCallbackServer.listen({ extraPaths: ["/callback/server-id"] });
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue