diff --git a/src/gateway/user-profiles-http.test.ts b/src/gateway/user-profiles-http.test.ts index 3d7ab54332b2..1f20e4f96cb3 100644 --- a/src/gateway/user-profiles-http.test.ts +++ b/src/gateway/user-profiles-http.test.ts @@ -1,6 +1,8 @@ import { createHash } from "node:crypto"; +import { EventEmitter } from "node:events"; import type { IncomingMessage, ServerResponse } from "node:http"; import { beforeEach, describe, expect, it, vi } from "vitest"; +import { createDeferred } from "../../test/helpers/promise.js"; import { handleUserProfileAvatarHttpRequest } from "./user-profiles-http.js"; const authorizeScopedUserProfileAvatarHttpRequestOrReply = vi.hoisted(() => vi.fn()); @@ -35,14 +37,56 @@ function response() { const writeHead = vi.fn(); return { end, - response: { end, setHeader, writeHead } as unknown as ServerResponse, + response: Object.assign(new EventEmitter(), { + destroyed: false, + end, + setHeader, + socket: null, + writeHead, + }) as unknown as ServerResponse, setHeader, writeHead, }; } function request(path: string, headers: Record = {}) { - return { method: "GET", url: path, headers } as unknown as IncomingMessage; + return { + method: "GET", + url: path, + headers, + socket: Object.assign(new EventEmitter(), { destroyed: false }), + } as unknown as IncomingMessage; +} + +function disconnectableRequest(path: string) { + const socket = Object.assign(new EventEmitter(), { destroyed: false }); + return { + request: { method: "GET", url: path, headers: {}, socket } as unknown as IncomingMessage, + socket, + }; +} + +function waitForDeferredResponse( + deferred: { promise: Promise }, + signal: AbortSignal | null | undefined, +): Promise { + return new Promise((resolve, reject) => { + const onAbort = () => { + const reason = signal?.reason; + reject(reason instanceof Error ? reason : new Error(String(reason))); + }; + signal?.addEventListener("abort", onAbort, { once: true }); + void deferred.promise.then( + (value) => { + signal?.removeEventListener("abort", onAbort); + resolve(value); + }, + (error: unknown) => { + signal?.removeEventListener("abort", onAbort); + reject(error instanceof Error ? error : new Error(String(error))); + }, + ); + }); } describe("profile avatar HTTP endpoint", () => { @@ -397,6 +441,128 @@ describe("profile avatar HTTP endpoint", () => { expect(res.end).toHaveBeenCalledWith(new Uint8Array([2, 2, 2])); }); + it("keeps a shared Gravatar fetch alive after its first waiter's deadline", async () => { + const firstProfileId = "profile-shared-deadline-first"; + const secondProfileId = "profile-shared-deadline-second"; + const primaryHash = emailHash("shared-deadline-primary@example.com"); + const sharedHash = emailHash("shared-deadline@example.com"); + const sharedResponse = createDeferred(); + const totalDeadlines: AbortController[] = []; + const timeoutSpy = vi.spyOn(AbortSignal, "timeout").mockImplementation((delay) => { + const controller = new AbortController(); + if (delay === 6_000) { + totalDeadlines.push(controller); + } + return controller.signal; + }); + getProfileAvatar.mockReturnValue(undefined); + getUserProfileListItem.mockImplementation((profileId: string) => ({ + id: profileId, + emails: + profileId === firstProfileId + ? ["shared-deadline-primary@example.com", "shared-deadline@example.com"] + : ["shared-deadline@example.com"], + hasAvatar: false, + })); + const fetchImpl = vi.fn((input: URL | RequestInfo, init?: RequestInit) => { + if (fetchUrl(input).includes(primaryHash)) { + return Promise.resolve(new Response(null, { status: 404 })); + } + expect(fetchUrl(input)).toContain(sharedHash); + return waitForDeferredResponse(sharedResponse, init?.signal); + }); + const first = response(); + const second = response(); + + try { + const firstRequest = handleUserProfileAvatarHttpRequest( + request("/ignored-by-handler"), + first.response, + `/api/users/${firstProfileId}/avatar`, + { auth: {} as never, fetchImpl }, + ); + await vi.waitFor(() => expect(fetchImpl).toHaveBeenCalledTimes(2)); + const secondRequest = handleUserProfileAvatarHttpRequest( + request("/ignored-by-handler"), + second.response, + `/api/users/${secondProfileId}/avatar`, + { auth: {} as never, fetchImpl }, + ); + await vi.waitFor(() => expect(totalDeadlines).toHaveLength(2)); + + totalDeadlines[0]?.abort(); + await firstRequest; + sharedResponse.resolve( + new Response(new Uint8Array([7, 8, 9]), { + status: 200, + headers: { "content-type": "image/png" }, + }), + ); + await secondRequest; + + expect(first.response.statusCode).toBe(502); + expect(second.writeHead).toHaveBeenCalledWith(200, expect.any(Object)); + expect(second.end).toHaveBeenCalledWith(new Uint8Array([7, 8, 9])); + expect(fetchImpl).toHaveBeenCalledTimes(2); + } finally { + timeoutSpy.mockRestore(); + } + }); + + it("retires a disconnected waiter without aborting its shared Gravatar fetch", async () => { + const profileId = "profile-shared-disconnect"; + const sharedResponse = createDeferred(); + getProfileAvatar.mockReturnValue(undefined); + getUserProfileListItem.mockReturnValue({ + id: profileId, + emails: ["shared-disconnect@example.com"], + hasAvatar: false, + }); + const fetchImpl = vi.fn((_input: URL | RequestInfo, init?: RequestInit) => + waitForDeferredResponse(sharedResponse, init?.signal), + ); + const firstReq = disconnectableRequest("/ignored-by-handler"); + const firstRes = response(); + const secondRes = response(); + let firstSettled = false; + + const firstRequest = handleUserProfileAvatarHttpRequest( + firstReq.request, + firstRes.response, + `/api/users/${profileId}/avatar`, + { auth: {} as never, fetchImpl }, + ).then((handled) => { + firstSettled = true; + return handled; + }); + await vi.waitFor(() => expect(fetchImpl).toHaveBeenCalledOnce()); + const secondRequest = handleUserProfileAvatarHttpRequest( + request("/ignored-by-handler"), + secondRes.response, + `/api/users/${profileId}/avatar`, + { auth: {} as never, fetchImpl }, + ); + + firstReq.socket.emit("close"); + await vi.waitFor(() => expect(firstSettled).toBe(true)); + expect(firstRes.writeHead).not.toHaveBeenCalled(); + expect(firstRes.end).not.toHaveBeenCalled(); + expect(firstReq.socket.listenerCount("close")).toBe(0); + + sharedResponse.resolve( + new Response(new Uint8Array([3, 2, 1]), { + status: 200, + headers: { "content-type": "image/png" }, + }), + ); + await expect(firstRequest).resolves.toBe(true); + await secondRequest; + + expect(secondRes.writeHead).toHaveBeenCalledWith(200, expect.any(Object)); + expect(secondRes.end).toHaveBeenCalledWith(new Uint8Array([3, 2, 1])); + expect(fetchImpl).toHaveBeenCalledOnce(); + }); + it("caps the Gravatar fan-out so a profile with many linked emails is bounded", async () => { const profileId = "profile-many-emails"; const emails = Array.from({ length: 12 }, (_, index) => `many-${index}@example.com`); diff --git a/src/gateway/user-profiles-http.ts b/src/gateway/user-profiles-http.ts index e74c8ed70975..eaef8b228e6a 100644 --- a/src/gateway/user-profiles-http.ts +++ b/src/gateway/user-profiles-http.ts @@ -13,7 +13,7 @@ import { import type { AuthRateLimiter } from "./auth-rate-limit.js"; import type { ResolvedGatewayAuth } from "./auth.js"; import { parseControlUiUserAvatarPath } from "./control-ui-contract.js"; -import { sendJson, sendMethodNotAllowed } from "./http-common.js"; +import { sendJson, sendMethodNotAllowed, watchClientDisconnect } from "./http-common.js"; import { matchesHttpIfNoneMatch } from "./http-conditional.js"; import { authorizeScopedUserProfileAvatarHttpRequestOrReply, @@ -201,13 +201,11 @@ async function cancelGravatarBody(body: ReadableStream | null): Prom async function fetchGravatar( hash: string, fetchImpl: typeof globalThis.fetch, - deadline?: AbortSignal, ): Promise { try { - const perCall = AbortSignal.timeout(GRAVATAR_FETCH_TIMEOUT_MS); const response = await fetchImpl(`${GRAVATAR_BASE_URL}/${hash}?s=256&d=404`, { headers: { Accept: "image/webp,image/png,image/jpeg,image/gif" }, - signal: deadline ? AbortSignal.any([deadline, perCall]) : perCall, + signal: AbortSignal.timeout(GRAVATAR_FETCH_TIMEOUT_MS), }); if (response.status === 404) { await cancelGravatarBody(response.body); @@ -239,7 +237,7 @@ async function fetchGravatar( async function resolveGravatar( hash: string, - options: { fetchImpl: typeof globalThis.fetch; nowMs: () => number; deadline?: AbortSignal }, + options: { fetchImpl: typeof globalThis.fetch; nowMs: () => number }, ): Promise { const cached = getCachedGravatar(hash, options.nowMs()); if (cached) { @@ -249,7 +247,7 @@ async function resolveGravatar( gravatarRequests, hash, async () => { - const result = await fetchGravatar(hash, options.fetchImpl, options.deadline); + const result = await fetchGravatar(hash, options.fetchImpl); if (result.kind !== "error") { cacheGravatar(hash, result, options.nowMs()); } @@ -259,6 +257,35 @@ async function resolveGravatar( ); } +async function waitForGravatar( + result: Promise, + signal: AbortSignal, +): Promise { + if (signal.aborted) { + return undefined; + } + return await new Promise((resolve, reject) => { + const onAbort = () => { + signal.removeEventListener("abort", onAbort); + resolve(undefined); + }; + signal.addEventListener("abort", onAbort, { once: true }); + void result.then( + (value) => { + signal.removeEventListener("abort", onAbort); + resolve(value); + }, + (error: unknown) => { + signal.removeEventListener("abort", onAbort); + reject(error instanceof Error ? error : new Error(String(error))); + }, + ); + if (signal.aborted) { + onAbort(); + } + }); +} + function sendAvatar( req: IncomingMessage, res: ServerResponse, @@ -382,29 +409,53 @@ export async function handleUserProfileAvatarHttpRequest( // Resolve linked emails sequentially and stop at the first hit: the primary // email keeps precedence, and a secondary email's hash is disclosed to - // Gravatar only once the earlier one is a definite miss. A single shared - // deadline bounds the total wait, so an unreachable Gravatar cannot stall the - // held connection by one timeout per linked email. + // Gravatar only once the earlier one is a definite miss. Shared hash fetches + // own their intrinsic timeout; this waiter independently owns the total HTTP + // deadline and disconnect lifecycle. + const clientAbort = new AbortController(); + const stopWatchingDisconnect = watchClientDisconnect(req, res, clientAbort); const deadline = AbortSignal.timeout(GRAVATAR_TOTAL_TIMEOUT_MS); - let transientFailure = false; - for (const hash of hashes) { - const result = await resolveGravatar(hash, { - fetchImpl: opts.fetchImpl ?? globalThis.fetch, - nowMs: opts.nowMs ?? Date.now, - deadline, - }); - if (result.kind === "hit") { - sendAvatar(req, res, result, "private, max-age=0, must-revalidate"); + const waiterSignal = AbortSignal.any([clientAbort.signal, deadline]); + try { + let transientFailure = false; + for (const hash of hashes) { + if (clientAbort.signal.aborted) { + return true; + } + if (deadline.aborted) { + transientFailure = true; + break; + } + const result = await waitForGravatar( + resolveGravatar(hash, { + fetchImpl: opts.fetchImpl ?? globalThis.fetch, + nowMs: opts.nowMs ?? Date.now, + }), + waiterSignal, + ); + if (!result || waiterSignal.aborted) { + if (clientAbort.signal.aborted) { + return true; + } + transientFailure = true; + break; + } + if (result.kind === "hit") { + sendAvatar(req, res, result, "private, max-age=0, must-revalidate"); + return true; + } + transientFailure ||= result.kind === "error"; + } + if (clientAbort.signal.aborted) { return true; } - transientFailure ||= result.kind === "error"; - if (deadline.aborted) { - break; - } + transientFailure ||= deadline.aborted; + sendJson(res, transientFailure ? 502 : 404, { + ok: false, + error: { type: transientFailure ? "avatar_upstream_unavailable" : "not_found" }, + }); + return true; + } finally { + stopWatchingDisconnect(); } - sendJson(res, transientFailure ? 502 : 404, { - ok: false, - error: { type: transientFailure ? "avatar_upstream_unavailable" : "not_found" }, - }); - return true; }