fix: cancel the discarded proxy stream, refresh revocation docs (#2319 round 8)
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
f3e4023e35
commit
a452305204
2 changed files with 79 additions and 21 deletions
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -304,6 +304,8 @@ describe("server.ts supplemental coverage", () => {
|
|||
let stallClosed: Promise<void>;
|
||||
/** Paths the upstream fixture has actually received. */
|
||||
let upstreamHits: string[];
|
||||
/** Resolves when the never-ending `/stream-open` response is closed. */
|
||||
let openStreamClosed: Promise<void>;
|
||||
|
||||
// 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<void>((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
|
||||
|
|
|
|||
Loading…
Reference in a new issue