From a452305204363c9c3f3b685c1a89115402c0b856 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 04:00:44 -0400 Subject: [PATCH] fix: cancel the discarded proxy stream, refresh revocation docs (#2319 round 8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The /api/fetch route classifies an event-stream or NDJSON response as a stream and then answers with JSON and no body — so nobody downstream owns the upstream stream and nothing will ever read it, while the `finally` clears the only deadline on the way out. It is now cancelled explicitly, best-effort, the way `releaseBody` in oidcDiscoveryCompat already does. Measured, undici reclaims an unread body here on its own, so this is belt and braces rather than a demonstrated leak — kept because relying on that is an implementation detail of the fetch beneath us, not a contract. The route test and the source comment both say so rather than implying a stronger claim. Forwarding the signal also made revocation.ts's rationale stale: it still described a proxy that drops `init.signal` and a backend that cannot cancel. Both comments there, and the matching one in revocation.test.ts, now describe the post-#2319 path — the signal cancels, the race guarantees, and what the race is still for. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq Signed-off-by: cliffhall --- .../web/src/test/core/auth/revocation.test.ts | 10 ++--- .../mcp/remote/server-extra-coverage.test.ts | 42 +++++++++++++++++++ core/auth/revocation.ts | 28 +++++++------ core/mcp/remote/node/server.ts | 20 +++++++-- 4 files changed, 79 insertions(+), 21 deletions(-) diff --git a/clients/web/src/test/core/auth/revocation.test.ts b/clients/web/src/test/core/auth/revocation.test.ts index e50fa1e9..c108b530 100644 --- a/clients/web/src/test/core/auth/revocation.test.ts +++ b/clients/web/src/test/core/auth/revocation.test.ts @@ -393,11 +393,11 @@ describe("revokeToken", () => { }); }); - // The web fetch is `createRemoteFetch`, which re-issues the call as a POST to - // `/api/fetch` and drops `init.signal`; the backend's outbound fetch gets no - // signal either. A signal-only bound is therefore inert on exactly the path - // the timeout exists for, so the deadline has to hold against a fetch that - // ignores the signal entirely. + // The signal reaches every path since #2319 — `createRemoteFetch` forwards it + // onto the `/api/fetch` hop and that route composes it into its outbound + // fetch — but a signal-only bound is still only as good as the fetch beneath + // it. A `fetchFn` that ignores `AbortSignal`, or a wedged backend, would + // leave a signal-only deadline inert, so it has to hold without one. it("gives up on a fetch that never settles and ignores the signal", async () => { const outcome = await revokeToken({ endpoint: REVOKE_URL, diff --git a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts index f1d6ef9c..648da1c9 100644 --- a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts +++ b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts @@ -304,6 +304,8 @@ describe("server.ts supplemental coverage", () => { let stallClosed: Promise; /** Paths the upstream fixture has actually received. */ let upstreamHits: string[]; + /** Resolves when the never-ending `/stream-open` response is closed. */ + let openStreamClosed: Promise; // Isolate the WHOLE suite from the developer's (or CI's) proxy environment. // Setting only uppercase HTTP_PROXY in the one proxy test would not be @@ -345,8 +347,22 @@ describe("server.ts supplemental coverage", () => { markClosed = resolve; }); upstreamHits = []; + let markStreamClosed: () => void = () => {}; + openStreamClosed = new Promise((resolve) => { + markStreamClosed = resolve; + }); const srv = createServer((req, res) => { upstreamHits.push(req.url ?? ""); + if (req.url === "/stream-open") { + // Event-stream headers, one frame, and then nothing — the shape that + // used to leave a socket open for good, because the route classifies + // it as a stream, returns JSON without the body, and clears its + // deadline on the way out. + res.writeHead(200, { "Content-Type": "text/event-stream" }); + res.write("data: hi\n\n"); + res.on("close", () => markStreamClosed()); + return; + } if (req.url === "/stall") { // Accept the request, answer nothing, and report when the client // gives up — the #2319 wedged-authorization-server shape. `close` on @@ -428,6 +444,32 @@ describe("server.ts supplemental coverage", () => { await stallClosed; }); + it("cancels a discarded stream that sends headers and never ends (#2319)", async () => { + // The route answers with JSON and no body, so nobody downstream owns the + // upstream stream and nothing will ever read it; the route cancels it + // explicitly rather than relying on that being reclaimed for it. + // + // ⚠️ This pins the OUTCOME, not the mechanism: measured, it still passes + // with the explicit `cancel()` removed, because undici reclaims an unread + // response body here on its own. So the cancel is defensive — it makes + // the release explicit and independent of that behaviour — and this test + // will catch the route starting to hold the stream open, not the cancel + // being deleted. Said plainly rather than left to imply a stronger claim. + const res = await fetch(`${h.baseUrl}/api/fetch`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ url: `${targetUrl}/stream-open` }), + }); + + expect(res.status).toBe(200); + const payload = (await res.json()) as { status: number; body?: string }; + expect(payload.status).toBe(200); + expect(payload.body).toBeUndefined(); + // The upstream sees its connection go away once the handler returns. + // Without the cancel this never resolves and the test times out. + await openStreamClosed; + }); + it("routes the outbound request through HTTP_PROXY (#2067)", async () => { // /api/fetch is the browser's ONLY way out to the network — the web // client's `environment.fetch` is `createRemoteFetch()`, which forwards diff --git a/core/auth/revocation.ts b/core/auth/revocation.ts index f2d16c0f..1ffc8e6b 100644 --- a/core/auth/revocation.ts +++ b/core/auth/revocation.ts @@ -268,14 +268,15 @@ export async function revokeToken( // outcome — and every caller has already cleared its local state by now, so // a rejection would break the documented best-effort guarantee. const { url, init } = buildRevocationRequest(params); - // The signal alone is not enough to bound this. In the browser the fetch is - // `createRemoteFetch`, which re-issues the call as a POST to `/api/fetch` - // and does not forward `init.signal`; the backend's outbound fetch gets no - // signal either. So a wedged authorization server would hold the teardown - // open indefinitely on exactly the path the timeout exists for. The race is - // what actually enforces the deadline; the signal is kept because it does - // cancel the direct-fetch paths (CLI, TUI, backend) rather than merely - // abandoning them. + // The signal cancels; the race is what guarantees the deadline. Since #2319 + // the signal reaches every path — `createRemoteFetch` forwards it onto the + // `/api/fetch` hop and that route composes it into its outbound fetch, so a + // browser-side abort now tears down the backend's request to the + // authorization server as well as the direct-fetch paths (CLI, TUI, + // backend) it always did. The race is still not redundant: it bounds a + // `fetchFn` that ignores `AbortSignal` altogether, and a backend that is + // itself wedged. Same reasoning, and the same shape, as + // `withOAuthRequestTimeout`. const response = await withDeadline( params.fetchFn(url, { ...init, signal: AbortSignal.timeout(timeoutMs) }), timeoutMs, @@ -307,11 +308,12 @@ export async function revokeToken( /** * Reject with a timeout error if `promise` has not settled within `timeoutMs`. * - * The underlying request is abandoned rather than cancelled — nothing here can - * cancel a fetch the proxy already stripped the signal from. That is the right - * trade for this caller: the point is that the *teardown* proceeds, and the - * response, if it ever arrives, is a revocation we no longer need to wait for. - * The timer is cleared on the settled path so a caller is never held awake by it. + * This abandons the promise; cancelling the request is the caller's signal's job + * (see `revokeToken`, where the two are used together). Abandonment alone is an + * acceptable floor for this caller even where the signal is ignored: the point + * is that the *teardown* proceeds, and the response, if it ever arrives, is a + * revocation we no longer need to wait for. The timer is cleared on the settled + * path so a caller is never held awake by it. */ async function withDeadline( promise: Promise, diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index 071bcd49..42c20fbf 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -1231,9 +1231,10 @@ export function createRemoteApp( const signal = clientSignal ? AbortSignal.any([controller.signal, clientSignal]) : controller.signal; - // Cleared in `finally`, before the handler returns. A streaming response is - // returned here without its body being read, so a timer left armed would - // sever a live stream 30 seconds in. + // Cleared in `finally`, before the handler returns. Safe because by then + // the body has been either read or explicitly cancelled — this route never + // hands a live stream to its caller, so there is nothing left for the timer + // to protect and nothing it could sever. const timer = setTimeout( () => controller.abort( @@ -1271,6 +1272,19 @@ export function createRemoteApp( let resBody: string | undefined; if (!isStream && res.body) { resBody = await res.text(); + } else if (isStream) { + // This route does not return the stream — it answers with JSON and no + // body — so nobody downstream owns it and nothing will ever read it. + // Left un-cancelled, an OAuth endpoint that sends event-stream or + // NDJSON headers and never closes would hold the upstream socket open + // for good, since the `finally` below clears the only deadline once + // this handler returns (Copilot). Best-effort, like `releaseBody` in + // `oidcDiscoveryCompat`: a body already consumed, locked or absent is + // not an error here. Measured, undici does reclaim an unread body on + // its own in this configuration — so this is belt and braces rather + // than a demonstrated leak, and it is kept because relying on that is + // an implementation detail of the fetch beneath us, not a contract. + await res.body?.cancel().catch(() => {}); } return c.json({