fix: do not await the discarded stream's cancel in /api/fetch (#2319 round 26)
Third and last site of the same class. `ReadableStream.cancel()` adopts the underlying source's cancel promise, which is permitted never to settle, so awaiting it kept the handler pending forever — and on an unbounded request there is no timer to release it either. The cancel is started and the handler returns, so the JSON response and the `finally` complete regardless. The `void` carries the justification for which of AGENTS.md's permitted cases it is: the callee owns its failures through its own `catch`, and the caller cannot await. Test: a `/stream-hostile` upstream that sends event-stream headers and one frame and then never closes, asserting the route still answers 200 with no body. 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
b4c02865e4
commit
cf856c740c
2 changed files with 32 additions and 1 deletions
|
|
@ -353,6 +353,14 @@ describe("server.ts supplemental coverage", () => {
|
|||
});
|
||||
const srv = createServer((req, res) => {
|
||||
upstreamHits.push(req.url ?? "");
|
||||
if (req.url === "/stream-hostile") {
|
||||
// Headers, one frame, and a connection whose teardown the route must
|
||||
// not wait on: `ReadableStream.cancel()` adopts the source's cancel
|
||||
// promise, which may never settle.
|
||||
res.writeHead(200, { "Content-Type": "text/event-stream" });
|
||||
res.write("data: hi\n\n");
|
||||
return;
|
||||
}
|
||||
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
|
||||
|
|
@ -568,6 +576,22 @@ describe("server.ts supplemental coverage", () => {
|
|||
expect(await settled).toBe("aborted by us");
|
||||
});
|
||||
|
||||
it("answers a discarded stream without waiting on its cancellation", async () => {
|
||||
// ⚠️ The route starts the cancel and returns. Awaiting it would keep the
|
||||
// handler pending on a source that never settles its cancel promise —
|
||||
// and on an unbounded request there is no timer to release it either.
|
||||
const res = await fetch(`${h.baseUrl}/api/fetch`, {
|
||||
method: "POST",
|
||||
headers: { "content-type": "application/json" },
|
||||
body: JSON.stringify({ url: `${targetUrl}/stream-hostile` }),
|
||||
});
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
const payload = (await res.json()) as { status: number; body?: string };
|
||||
expect(payload.status).toBe(200);
|
||||
expect(payload.body).toBeUndefined();
|
||||
});
|
||||
|
||||
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
|
||||
|
|
|
|||
|
|
@ -1323,7 +1323,14 @@ export function createRemoteApp(
|
|||
// 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(() => {});
|
||||
//
|
||||
// ⚠️ Started, not awaited. `ReadableStream.cancel()` adopts the
|
||||
// underlying source's cancel promise, which is permitted never to
|
||||
// settle — awaiting it would keep this handler pending forever, and on
|
||||
// an unbounded request there is no timer to release it either
|
||||
// (Copilot). The `void` is the documented case where the callee owns
|
||||
// its failures, via the `catch` below, and the caller cannot await.
|
||||
void res.body?.cancel().catch(() => {});
|
||||
}
|
||||
|
||||
return c.json({
|
||||
|
|
|
|||
Loading…
Reference in a new issue