From e9ab5d2392b8aa3184136e8e1f763537d936deb5 Mon Sep 17 00:00:00 2001 From: Chintan Date: Sat, 5 Sep 2026 20:13:06 -0400 Subject: [PATCH] A malformed status crashed the lookup, and eight other quiet failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three of these are in `src/server/lookup.ts`, and all three arrived with the same change: the pinned `node:https` transport replaced `fetch`, and undici had been doing more than it looked. `new Response(body, { status })` takes a status of 200–599 and refuses a body on the four statuses defined not to carry one. A server is bound by neither. A 205 with an HTML body, or a 600, makes the constructor throw — and it throws inside the `https.request` response callback, where there is no promise for it to land in. That is an uncaught exception, which takes the dev server, or the serverless invocation, down with it. Construction moved into `toResponse`: an out-of-range status becomes 502, a bodyless one has its stream torn down, and the callback wraps the call so a throw destroys the socket and rejects instead. A response dropped without being read — a redirect hop, an error page, a PDF — used to be collected along with its socket, because `fetch` has a GC backstop and a body timeout. The raw transport has neither, and the abort timer is cleared in `finally`, so a server that writes a 302 body and never ends it holds a socket and an `IncomingMessage` open after `lookupProduct` has returned. In dev that is one socket per request; in a function it is the invocation held open until the platform's hard timeout, which is the exact thing the size cap and the timer exist to prevent. Every early exit cancels the body first now. And a timeout that lands mid-body no longer looks like one. `fetch` rejects with an `AbortError`; `node:https` destroys the `IncomingMessage`, so a body still being read fails with an `aborted` ECONNRESET, and the user was told "could not reach that page" about a page that had answered and then stalled. The catch asks `controller.signal.aborted` rather than the error's name. Nothing caught any of this because the 504 test stubs the transport with a rejecting `AbortError` and `pinnedFetch` is never exercised by a test at all — there are now four over `toResponse`, one driving the mid-body timeout on fake timers, and one asserting the body is cancelled on all three drop paths. In `walk.ts`, the `inside` branch of `contactNormal` summed a unit vector toward every edge, and for a rectangle — every wall segment, most of the furniture — the four cancel to exactly zero. The branch always returned null while its comment said it pointed at open air. It takes the nearest edge alone now, which is what the comment always claimed. It was also re-running the broad phase `isClear` had just run and projecting every edge twice; `slide` collects the touching blockers once and hands them over. A move squarely into a surface loses its whole length to the normal and leaves a 1e-13mm residue, which is a new position every frame for a walker standing still. `slide` returns `from` itself. The test that claimed to cover this passed for a different reason — its blocked point landed exactly on the hypotenuse, where no edge has a distance and no normal exists, so the axis fallback answered — and it now starts from a point where the projection is what runs. A second test covers a running step long enough to land its centre inside the polygon, which is where the nearest-edge branch earns its place. `scene.bench.ts` never measured a collision. At the sparse pitch the walker started inside a placement, so `slide` took the free-move escape; at the other two the grid fills a corner of a 40×32m hall and the walker stood in open floor. The 0.011–0.028ms §10.4 quoted as the per-frame cost was the cost of a step that touches nothing, and the contact-normal pass it was quoted about was never in it. Two rows now, clear and blocked, seeded against the first row of the grid, with `assertStride` failing the bench if either walker is standing inside something or stepping where its label does not say. 0.029–0.032ms clear, 0.045–0.049ms blocked; §10.4 carries both and says why the old figure was wrong. `playwright.media.config.ts` had `reuseExistingServer: true`, so a preview left running from an earlier build would be photographed and committed to `docs/media` as the current application — and every assert in that run checks state, not pixels, so all of them would pass. `playwright.config.ts` already sets it false, with a comment about the time that happened. And `capture.spec.ts` said the walker seeds facing east, which is why the clip has to look 90° first; it seeds facing north. 779 unit tests, 124 e2e, `tsc` and `eslint` clean. --- PLAN.md | 21 ++++--- media/capture.spec.ts | 7 ++- playwright.media.config.ts | 6 +- src/core/scene.bench.ts | 56 ++++++++++++++++-- src/core/walk.test.ts | 28 ++++++++- src/core/walk.ts | 85 ++++++++++++++++++--------- src/server/lookup.test.ts | 115 ++++++++++++++++++++++++++++++++++++- src/server/lookup.ts | 87 +++++++++++++++++++++++----- 8 files changed, 345 insertions(+), 60 deletions(-) diff --git a/PLAN.md b/PLAN.md index de9f60c..4f632dd 100644 --- a/PLAN.md +++ b/PLAN.md @@ -911,7 +911,7 @@ puts 500 placements on a floor at three densities and times the passes they feed not in CI, for the reason no timing should be: a benchmark that gates a merge fails on whatever else the machine was doing. -Milliseconds per call, 500 placements, Node 24 on a Windows laptop, 2026-09-04. *Sparse* +Milliseconds per call, 500 placements, Node 24 on a Windows laptop, 2026-09-05. *Sparse* is a 1.1m pitch — a furnished floor with clearance around everything. *Touching* is 520mm, where every item overlaps its neighbours and the validation panel has something to say about all of them. *Piled* is 120mm, which is not a plan anyone drew and is here @@ -924,15 +924,22 @@ to show where the cost comes from. | `blockersOf` | 0.004 | 0.004 | 0.005 | | `findCollisions` | 0.37 | 5.1 | 188 | | `validateFloor` | 0.37 | 5.9 | 216 | -| **`stepWalker`** | **0.011** | **0.027** | **0.028** | +| **`stepWalker`, clear** | **0.029** | **0.030** | **0.032** | +| **`stepWalker`, blocked** | **0.045** | **0.046** | **0.049** | Three things fall out of that. -`stepWalker` is the only row that runs inside a frame; everything above it runs once per -edit, against a scene the cache in `ui/space/scene-cache.ts` keeps until the document -changes. At 0.011–0.028ms against 500 blockers it uses well under a percent of the -16.7ms budget, which means the CPU half of the target is not where the risk is. Whether -500 placements hold 60fps is a question about draw calls, and this says nothing about it. +`stepWalker` is the only pair of rows that runs inside a frame; everything above them +runs once per edit, against a scene the cache in `ui/space/scene-cache.ts` keeps until +the document changes. It is timed twice because a frame that touches nothing and a frame +that is blocked do different work: the first is a bounding-box rejection per blocker, +the second sweeps the blockers it is against for their normals and projects the move +along them. At 0.03ms clear and 0.05ms blocked against 500 blockers, either uses well +under a percent of the 16.7ms budget, which means the CPU half of the target is not where +the risk is. Whether 500 placements hold 60fps is a question about draw calls, and this +says nothing about it. (An earlier figure of 0.011–0.028ms was for a step that touched +nothing — and at one density for a walker that had been stood inside a placement — which +is why the blocked frame is now timed on its own and the bench checks its own geometry.) Placement count is not the variable. **Colliding pairs** are: 17 pairs cost 0.37ms and 17,315 pairs cost 216ms, on the same 500 placements. The broad phase is doing its job — diff --git a/media/capture.spec.ts b/media/capture.spec.ts index 329491f..3d86850 100644 --- a/media/capture.spec.ts +++ b/media/capture.spec.ts @@ -66,9 +66,10 @@ async function nameRoom(page: Page, at: { x: number; y: number }, name: string) * T-junctions where the partition meets the shell. * * The interior door sits on y = 3000 on purpose. The walker seeds at the centre of - * the largest room facing east, so that line is the one along which a scripted - * clip can hold one key and end up in the next room — and the layout keeps it - * clear of everything but the rug, which is 10mm tall and meant to be walked over. + * the largest room facing north, and the clip's first beat turns it 90° to face + * east — so that line is the one along which holding one key ends up in the next + * room, and the layout keeps it clear of everything but the rug, which is 10mm tall + * and meant to be walked over. */ async function apartment(page: Page) { const stage = page.getByTestId('plan-stage'); diff --git a/playwright.media.config.ts b/playwright.media.config.ts index be304e9..b22f0cd 100644 --- a/playwright.media.config.ts +++ b/playwright.media.config.ts @@ -49,7 +49,11 @@ export default defineConfig({ webServer: { command: `pnpm run build && pnpm run preview --port ${PORT} --strictPort`, url: `http://localhost:${PORT}`, - reuseExistingServer: true, + // Always start our own server, as the e2e config does. Reusing whatever is on the + // port would photograph a preview left over from an earlier build and commit it + // as the current application — and every assert here checks state, not pixels, + // so nothing would notice. + reuseExistingServer: false, timeout: 180_000, }, }); diff --git a/src/core/scene.bench.ts b/src/core/scene.bench.ts index 09ba51e..8b48bb2 100644 --- a/src/core/scene.bench.ts +++ b/src/core/scene.bench.ts @@ -1,11 +1,20 @@ import { bench, describe } from 'vitest'; import { createCatalogItem } from './catalog'; -import { findCollisions } from './geometry/collision'; +import { findCollisions, type Volume } from './geometry/collision'; import { createDocument, type Floor, type SpaceDocument } from './document'; import { blockersOf, buildScene, buildStack } from './scene'; import { commitRoomRect } from './tools'; import { validateFloor } from './validation'; -import { NO_INPUT, createWalker, stepWalker } from './walk'; +import { + NO_INPUT, + WALK_SPEED_MMS, + bodySpan, + createWalker, + forwardVector, + isClear, + stepWalker, + type Walker, +} from './walk'; /** * What PLAN.md §10.4's target can honestly be checked against without a GPU. @@ -18,7 +27,10 @@ import { NO_INPUT, createWalker, stepWalker } from './walk'; * What *is* measurable is everything the renderer is handed, and one thing that runs * inside the frame. `buildScene` and `validateFloor` run once per edit; * `stepWalker` runs sixty times a second against the cached blocker list, so it is - * the only figure here that comes out of the 16.7ms budget. + * the only figure here that comes out of the 16.7ms budget. It is timed twice, because + * a frame that touches nothing and a frame that is blocked do different work: the + * first is a bounding-box rejection per blocker, the second sweeps the blockers again + * for the surfaces it is against and projects the move along them. * * Three densities, because placement count turns out not to be the variable that * matters — the number of *overlapping pairs* is, and those are two very different @@ -77,14 +89,42 @@ function floorOf(pitchMm: number): { doc: SpaceDocument; floor: Floor } { return { doc, floor }; } +/** + * Check that a walker is standing clear and that one frame's stride ahead is what the + * bench says it is. A bench that times the wrong thing is worse than none — the first + * version of this file stood its walker inside a placement at one pitch and in open + * floor at the others, and the blocked frame it claimed to measure never happened. + */ +function assertStride(walker: Walker, blockers: readonly Volume[], blocked: boolean) { + const span = bodySpan(walker); + const { x, y } = walker.position; + if (!isClear(walker.position, span, blockers)) { + throw new Error(`the walker at ${x},${y} is standing inside something`); + } + const f = forwardVector(walker.heading); + const stride = WALK_SPEED_MMS / 60; + const ahead = { x: x + f.x * stride, y: y + f.y * stride }; + if (isClear(ahead, span, blockers) === blocked) { + throw new Error(`the step from ${x},${y} should be ${blocked ? 'blocked' : 'clear'}`); + } +} + for (const [label, pitchMm] of Object.entries(PITCHES)) { describe(`${COUNT} placements, ${label}`, () => { const { doc, floor } = floorOf(pitchMm); const scene = buildScene(doc, floor); const blockers = blockersOf(scene); - const walker = createWalker({ x: 20000, y: 16000 }, 45); const world = { blockers, mode: 'walk' as const }; + // Open floor in the far corner, beyond the grid at every pitch. + const clear = createWalker({ x: 32000, y: 28000 }, 45); + // Ten millimetres clear of the first row's face and a stride short of touching + // it, walking south-east — into the row, with an east component for the slide to + // keep, which is the whole of the blocked path. + const blocked = createWalker({ x: 900, y: 390 }, 135); + assertStride(clear, blockers, false); + assertStride(blocked, blockers, true); + bench('buildScene', () => { buildScene(doc, floor); }); @@ -106,8 +146,12 @@ for (const [label, pitchMm] of Object.entries(PITCHES)) { }); // The frame. Everything above is per edit. - bench('stepWalker', () => { - stepWalker(walker, { ...NO_INPUT, forward: 1 }, 1 / 60, world); + bench('stepWalker, clear', () => { + stepWalker(clear, { ...NO_INPUT, forward: 1 }, 1 / 60, world); + }); + + bench('stepWalker, blocked', () => { + stepWalker(blocked, { ...NO_INPUT, forward: 1 }, 1 / 60, world); }); }); } diff --git a/src/core/walk.test.ts b/src/core/walk.test.ts index c15f011..eac3ca6 100644 --- a/src/core/walk.test.ts +++ b/src/core/walk.test.ts @@ -374,12 +374,34 @@ describe('sliding', () => { it('still stops dead when the move is straight into the surface', () => { // Nothing tangential is left to keep: the whole move is the component that has - // to go. A wall you walk squarely at is a wall you stop at. - const from = { x: 1000, y: 1600 }; - const into = { x: 300, y: -300 }; + // to go. A wall you walk squarely at is a wall you stop at — *exactly* where you + // were, not a few 1e-13mm off it. + // + // The move lands short of the line rather than on it. Standing on the line, no + // edge is at any distance at all and there is no normal to project against, so + // the axis retries would give the same answer for a different reason and the + // projection would go untested. And the start is chosen so that the projection + // does leave residue — from (1000, 1600) it happens to cancel to zero exactly, + // and this assertion would pass without the exact-stop rule it is here for. + const from = { x: 1000, y: 1523 }; + const into = { x: 198, y: -198 }; expect(slide(from, into, { bottom: 0, top: 1800 }, [diagonal])).toEqual(from); }); + it('finds the way out when a long step lands its centre inside the surface', () => { + // A running step on a slow frame is longer than the body radius, so the blocked + // position can be *inside* the polygon, where every edge counts as touched and + // their normals average to something that points along or into it. The nearest + // edge alone says where open air is — and the slide goes along the surface, not + // back the way it came. + const from = { x: 1000, y: 1600 }; + const after = slide(from, { x: 500, y: -200 }, { bottom: 0, top: 1800 }, [diagonal]); + + expect(after.x).toBeGreaterThan(from.x); + expect(after.y).toBeGreaterThan(from.y); + expect(isClear(after, { bottom: 0, top: 1800 }, [diagonal])).toBe(true); + }); + it('leaves a walker who is already inside something free to get out', () => { // Unchanged, and asserted here because the contact normal is computed from the // blocked candidate: a walker standing inside a wall has candidates that are all diff --git a/src/core/walk.ts b/src/core/walk.ts index b1a067d..663679c 100644 --- a/src/core/walk.ts +++ b/src/core/walk.ts @@ -45,7 +45,6 @@ import { circleIntersects, spansOverlap, type Span, type Volume } from './geomet import { containsPoint } from './geometry/polygon'; import { closestPointOnSegment, - distanceToSegment, dot, length, normalize, @@ -249,7 +248,29 @@ export function groundHeight( } /** - * Which way the surfaces touching `at` face, averaged, or null if nothing touches it. + * Every blocker a body of `span` centred at `at` is touching. Empty means clear. + * + * `isClear` stops at the first hit; this collects them, for the one caller that goes + * on to ask which way they face and should not have to sweep the list a second time + * to find out. + */ +function contacts( + at: Vec2, + span: Span, + blockers: readonly Volume[], + radiusMm: number, +): Volume[] { + const touching: Volume[] = []; + for (const blocker of blockers) { + if (!spansOverlap(span, blocker.span)) continue; + if (circleIntersects(blocker.outline, at, radiusMm)) touching.push(blocker); + } + return touching; +} + +/** + * Which way the surfaces `touching` a body centred at `at` face, averaged, or null if + * none of them can say. * * Averaged rather than nearest-wins because a walker in an inside corner is against * two surfaces at once, and one of them alone would send them straight into the other. @@ -257,35 +278,41 @@ export function groundHeight( * — and failing is the right answer there, because it is what hands the move to the * axis retries that can still slide along one of the two walls. * - * A body already *inside* a blocker takes the direction from itself to the nearest - * edge instead, so the normal still points at open air rather than deeper in. + * A body whose centre is *inside* a blocker — a running step on a slow frame is longer + * than the body radius, and can land one there — takes the direction to that + * blocker's nearest edge instead. From inside every edge counts as touched, and for a + * rectangle, which is every wall and most of the furniture, their normals cancel to + * exactly nothing. The nearest edge alone is the way out. */ -function contactNormal( - at: Vec2, - span: Span, - blockers: readonly Volume[], - radiusMm: number, -): Vec2 | null { +function contactNormal(at: Vec2, touching: readonly Volume[], radiusMm: number): Vec2 | null { let sum: Vec2 = { x: 0, y: 0 }; - for (const blocker of blockers) { - if (!spansOverlap(span, blocker.span)) continue; - if (!circleIntersects(blocker.outline, at, radiusMm)) continue; - - const inside = containsPoint(blocker.outline, at); + for (const blocker of touching) { const pts = blocker.outline.pts; + + if (containsPoint(blocker.outline, at)) { + let out: Vec2 | null = null; + let nearest = Infinity; + for (let i = 0; i < pts.length; i++) { + const away = sub(closestPointOnSegment(at, pts[i]!, pts[(i + 1) % pts.length]!), at); + const d = length(away); + if (d < nearest) { + nearest = d; + out = away; + } + } + if (out && nearest > 1e-6) sum = { x: sum.x + out.x / nearest, y: sum.y + out.y / nearest }; + continue; + } + for (let i = 0; i < pts.length; i++) { - const a = pts[i]!; - const b = pts[(i + 1) % pts.length]!; // Only the edges actually being touched. A long wall's far edge is part of the // same polygon and points the opposite way; including it would cancel the // normal out to nothing. - if (!inside && distanceToSegment(at, a, b) > radiusMm) continue; - - const closest = closestPointOnSegment(at, a, b); - const away = inside ? sub(closest, at) : sub(at, closest); - if (length(away) < 1e-6) continue; - sum = { x: sum.x + normalize(away).x, y: sum.y + normalize(away).y }; + const away = sub(at, closestPointOnSegment(at, pts[i]!, pts[(i + 1) % pts.length]!)); + const d = length(away); + if (d > radiusMm || d < 1e-6) continue; + sum = { x: sum.x + away.x / d, y: sum.y + away.y / d }; } } @@ -314,13 +341,14 @@ export function slide( } const full = { x: from.x + delta.x, y: from.y + delta.y }; - if (isClear(full, span, blockers, radiusMm)) return full; + const touching = contacts(full, span, blockers, radiusMm); + if (touching.length === 0) return full; const candidates: Vec2[] = []; // The normal is taken at the blocked position rather than at `from`, because `from` // is clear by the check above and so touches nothing to take a normal from. - const normal = contactNormal(full, span, blockers, radiusMm); + const normal = contactNormal(full, touching, radiusMm); if (normal) { const into = dot(delta, normal); // Only a move that goes *into* the surface has a component to lose. A move that @@ -328,7 +356,12 @@ export function slide( // away the escape. if (into < 0) { const along = sub(delta, scale(normal, into)); - candidates.push({ x: from.x + along.x, y: from.y + along.y }); + // A move squarely into the surface has nothing left once its normal component + // goes. What floating point leaves of it — a few 1e-13mm — would still be a new + // position every frame, so it is `from` itself, exactly. + candidates.push( + length(along) < 1e-6 ? from : { x: from.x + along.x, y: from.y + along.y }, + ); } } diff --git a/src/server/lookup.test.ts b/src/server/lookup.test.ts index 58f7e96..40a86dd 100644 --- a/src/server/lookup.test.ts +++ b/src/server/lookup.test.ts @@ -1,11 +1,15 @@ -import { describe, expect, it } from 'vitest'; +import { Readable } from 'node:stream'; +import type { IncomingMessage } from 'node:http'; +import { describe, expect, it, vi } from 'vitest'; import { BlockedUrlError, MAX_RESPONSE_BYTES, + REQUEST_TIMEOUT_MS, USER_AGENT, isPrivateAddress, lookupProduct, parseTargetUrl, + toResponse, type LookupDeps, } from './lookup'; @@ -396,6 +400,74 @@ describe('what comes back', () => { expect(result).toMatchObject({ ok: false, status: 504 }); }); + it('says a page took too long when the timeout lands mid-body', async () => { + // The pinned transport does not reject with an `AbortError`. Node tears the socket + // down, and a body still being read fails with an `aborted` ECONNRESET instead — + // the same shape as a server that hung up, which it is not. The page answered and + // then stalled, and the caller should hear "too long", not "could not reach". + vi.useFakeTimers(); + try { + const pending = lookupProduct( + 'https://shop.example.com/p', + deps({ + fetch: (_url, init) => { + const body = new ReadableStream({ + start(controller) { + init.signal?.addEventListener('abort', () => { + controller.error(Object.assign(new Error('aborted'), { code: 'ECONNRESET' })); + }); + }, + }); + return Promise.resolve( + new Response(body, { headers: { 'content-type': 'text/html' } }), + ); + }, + }), + ); + await vi.advanceTimersByTimeAsync(REQUEST_TIMEOUT_MS); + expect(await pending).toMatchObject({ ok: false, status: 504 }); + } finally { + vi.useRealTimers(); + } + }); + + it('drops the body of a response it is not going to read', async () => { + // A redirect, an error page and a PDF are each answered without reading the body, + // and under the pinned transport a body nobody reads is a socket nobody closes. + // Cancelling is what releases it, so cancelling is what is asserted. + const unread = (status: number, headers: Record) => { + let cancelled = false; + const body = new ReadableStream({ + pull(controller) { + controller.enqueue(new TextEncoder().encode('x'.repeat(1000))); + }, + cancel() { + cancelled = true; + }, + }); + return { response: new Response(body, { status, headers }), cancelled: () => cancelled }; + }; + + const redirect = unread(302, { location: 'https://shop.example.com/p/sofa' }); + const redirected = await lookupProduct( + 'https://shop.example.com/p', + deps({ + fetch: (url) => + Promise.resolve(url === 'https://shop.example.com/p' ? redirect.response : html()), + }), + ); + expect(redirected.ok).toBe(true); + expect(redirect.cancelled()).toBe(true); + + const missing = unread(404, { 'content-type': 'text/html' }); + await lookupProduct('https://shop.example.com/p', deps({ fetch: () => Promise.resolve(missing.response) })); + expect(missing.cancelled()).toBe(true); + + const pdf = unread(200, { 'content-type': 'application/pdf' }); + await lookupProduct('https://shop.example.com/p', deps({ fetch: () => Promise.resolve(pdf.response) })); + expect(pdf.cancelled()).toBe(true); + }); + it('says it could not reach a page rather than throwing', async () => { const result = await lookupProduct( 'https://shop.example.com/p', @@ -404,3 +476,44 @@ describe('what comes back', () => { expect(result).toMatchObject({ ok: false, status: 502 }); }); }); + +describe('what node:https received, as a Response', () => { + /** + * `Response` accepts a status of 200–599 and refuses a body on 101, 204, 205 and + * 304. A server is bound by neither, and the constructor throwing inside a socket + * callback is not an error the caller sees — it is the process going down. + */ + function incoming(statusCode: number, body = '', headers = {}): IncomingMessage { + return Object.assign(Readable.from([Buffer.from(body)]), { + statusCode, + headers: { 'content-type': 'text/html', ...headers }, + }) as unknown as IncomingMessage; + } + + it('passes an ordinary page through, body and all', async () => { + const response = toResponse(incoming(200, 'Thing'), Readable); + expect(response.status).toBe(200); + expect(response.headers.get('content-type')).toBe('text/html'); + expect(await response.text()).toBe('Thing'); + }); + + it('does not throw on a bodyless status that arrived with a body', () => { + // A 205 with an HTML body is malformed, and it is also something a server sends. + for (const status of [204, 205, 304]) { + const response = toResponse(incoming(status), Readable); + expect(response.status, String(status)).toBe(status); + expect(response.body, String(status)).toBeNull(); + } + }); + + it('reports a status outside what a Response can carry as a bad gateway', () => { + for (const status of [600, 999, 199]) { + expect(toResponse(incoming(status), Readable).status, String(status)).toBe(502); + } + }); + + it('keeps every value of a repeated header', () => { + const response = toResponse(incoming(200, '', { 'set-cookie': ['a=1', 'b=2'] }), Readable); + expect(response.headers.getSetCookie()).toEqual(['a=1', 'b=2']); + }); +}); diff --git a/src/server/lookup.ts b/src/server/lookup.ts index 3a2317c..8cbf6c7 100644 --- a/src/server/lookup.ts +++ b/src/server/lookup.ts @@ -32,6 +32,7 @@ * with a `lookup` that returns the addresses already validated. */ +import type { IncomingMessage } from 'node:http'; import type * as NodeHttps from 'node:https'; import type * as NodeStream from 'node:stream'; @@ -205,6 +206,44 @@ function addressFamily(address: string): number { return address.includes(':') ? 6 : 4; } +/** The statuses defined to carry no body. `Response` throws if handed one alongside. */ +const BODYLESS_STATUSES = new Set([101, 204, 205, 304]); + +/** + * A `Response` over what `node:https` received, within what the constructor accepts. + * + * It accepts a status of 200–599 and nothing else, and no body at all on the four + * statuses defined not to have one. A server is under no such constraint — a 205 with + * a body and a 600 are both things a real one sends — and either would throw. Out of + * range becomes 502, which is what the caller reports for a page that did not answer + * properly anyway. A bodyless status has its stream torn down here, because nothing + * will ever read it, and a stream nobody reads is a socket nobody closes. + * + * Exported for its test only: the transport around it needs a TLS server to reach. + */ +export function toResponse( + incoming: IncomingMessage, + Readable: typeof NodeStream.Readable, +): Response { + const status = incoming.statusCode ?? 502; + const usable = status >= 200 && status <= 599 ? status : 502; + + const received = new Headers(); + for (const [name, value] of Object.entries(incoming.headers)) { + if (Array.isArray(value)) for (const one of value) received.append(name, one); + else if (value !== undefined) received.set(name, value); + } + + if (BODYLESS_STATUSES.has(usable)) { + incoming.destroy(); + return new Response(null, { status: usable, headers: received }); + } + return new Response(Readable.toWeb(incoming) as unknown as ReadableStream, { + status: usable, + headers: received, + }); +} + /** * `fetch`, with the socket pinned to addresses that have already been checked. * @@ -272,19 +311,14 @@ async function pinnedFetch( }, }, (incoming) => { - const status = incoming.statusCode ?? 502; - const received = new Headers(); - for (const [name, value] of Object.entries(incoming.headers)) { - if (Array.isArray(value)) for (const one of value) received.append(name, one); - else if (value !== undefined) received.set(name, value); + try { + settle(toResponse(incoming, Readable)); + } catch (err) { + // A throw in here has no promise to land in. Left alone it is an uncaught + // exception, which takes the dev server — or the function — down with it. + incoming.destroy(); + fail(err); } - // 204 and 304 are defined to carry no body, and `Response` refuses to be given - // one — a redirect chain through either would throw here rather than be read. - const empty = status === 204 || status === 304; - const body = empty - ? null - : (Readable.toWeb(incoming) as unknown as ReadableStream); - settle(new Response(body, { status, headers: received })); }, ); outgoing.on('error', fail); @@ -350,6 +384,23 @@ export type LookupDeps = { resolve?: Resolver | null; }; +/** + * Drop a response without reading it. + * + * Under `fetch` an unread body is released when the response is collected. Under the + * pinned transport nothing collects it: a redirect or an error page that was never + * read keeps its socket open until the server closes it, which a hostile server never + * does — and in a serverless function that is the invocation held open until the + * platform's hard timeout, the exact thing the size cap and the timer exist to prevent. + */ +async function discard(response: Response): Promise { + try { + await response.body?.cancel(); + } catch { + // Already closed, errored or locked: nothing is holding a socket. + } +} + /** Read at most `MAX_RESPONSE_BYTES`, then stop — a cap that is not merely advisory. */ async function readCapped(response: Response): Promise { const body = response.body; @@ -419,6 +470,9 @@ export async function lookupProduct( if (response.status < 300 || response.status >= 400) break; + // A redirect's body is never read. See `discard`. + await discard(response); + const location = response.headers.get('location'); if (!location) break; url = new URL(location, url); @@ -439,6 +493,7 @@ export async function lookupProduct( return { ok: false, status: 502, message: 'That page redirected too many times.' }; } if (!response.ok) { + await discard(response); return { ok: false, status: 502, @@ -448,6 +503,7 @@ export async function lookupProduct( const type = response.headers.get('content-type') ?? ''; if (!/text\/html|application\/xhtml/i.test(type)) { + await discard(response); return { ok: false, status: 415, message: 'That URL is not a web page.' }; } @@ -455,7 +511,12 @@ export async function lookupProduct( return { ok: true, url: url.toString(), draft: parseProduct(html, url.toString()) }; } catch (err) { if (err instanceof BlockedUrlError) return { ok: false, status: 400, message: err.message }; - if (err instanceof Error && err.name === 'AbortError') { + // The signal is asked rather than the error, because the two transports report a + // timeout differently. `fetch` rejects with an `AbortError`; `node:https` tears + // the socket down, and a body still being read fails with an `aborted` ECONNRESET + // instead — which is not a page that could not be reached, but one that answered + // and then took too long to finish. + if (controller.signal.aborted || (err instanceof Error && err.name === 'AbortError')) { return { ok: false, status: 504, message: 'That page took too long to answer.' }; } return { ok: false, status: 502, message: 'Could not reach that page.' };