diff --git a/CHANGES.md b/CHANGES.md index c15bc1935..aac260030 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -8,6 +8,16 @@ Version 2.0.17 To be released. +### @fedify/fedify + + - Fixed `doubleKnock()` so transient transport failures such as DNS hiccups + no longer leak raw `TypeError`s. Idempotent authenticated document + fetches are retried once, and remaining transport failures are reported as + `FetchError` with the original error as the cause. [[#762], [#763]] + +[#762]: https://github.com/fedify-dev/fedify/issues/762 +[#763]: https://github.com/fedify-dev/fedify/pull/763 + Version 2.0.16 -------------- diff --git a/packages/fedify/src/sig/http.test.ts b/packages/fedify/src/sig/http.test.ts index fc4ea3259..b020f34fe 100644 --- a/packages/fedify/src/sig/http.test.ts +++ b/packages/fedify/src/sig/http.test.ts @@ -1,6 +1,6 @@ import { mockDocumentLoader, test } from "@fedify/fixture"; import type { CryptographicKey, Multikey } from "@fedify/vocab"; -import { exportSpki } from "@fedify/vocab-runtime"; +import { exportSpki, FetchError } from "@fedify/vocab-runtime"; import { assert, assertEquals, @@ -1725,6 +1725,199 @@ test("doubleKnock() detects redirect loops", async () => { fetchMock.hardReset(); }); +test("doubleKnock() retries idempotent request transport errors", async () => { + fetchMock.spyGlobal(); + + try { + let requestCount = 0; + fetchMock.get("https://example.com/flaky-document", () => { + requestCount++; + if (requestCount === 1) { + throw new TypeError("temporary DNS failure"); + } + return new Response("Success", { status: 200 }); + }); + + const request = new Request("https://example.com/flaky-document"); + const response = await doubleKnock( + request, + { + keyId: rsaPublicKey2.id!, + privateKey: rsaPrivateKey2, + }, + ); + + assertEquals(response.status, 200); + assertEquals(await response.text(), "Success"); + assertEquals(requestCount, 2); + } finally { + fetchMock.hardReset(); + } +}); + +test("doubleKnock() wraps repeated transport errors", async () => { + fetchMock.spyGlobal(); + + try { + let requestCount = 0; + const failure = new TypeError("DNS lookup failed"); + fetchMock.get("https://example.com/unreachable-document", () => { + requestCount++; + throw failure; + }); + + const request = new Request("https://example.com/unreachable-document"); + const error = await assertRejects( + () => + doubleKnock( + request, + { + keyId: rsaPublicKey2.id!, + privateKey: rsaPrivateKey2, + }, + ), + FetchError, + "DNS lookup failed", + ); + + assertEquals(error.url.href, "https://example.com/unreachable-document"); + assertEquals(error.cause, failure); + assertEquals(requestCount, 2); + } finally { + fetchMock.hardReset(); + } +}); + +test("doubleKnock() does not retry non-idempotent transport errors", async () => { + fetchMock.spyGlobal(); + + try { + let requestCount = 0; + const failure = new TypeError("connection reset"); + fetchMock.post("https://example.com/flaky-inbox", () => { + requestCount++; + throw failure; + }); + + const request = new Request("https://example.com/flaky-inbox", { + method: "POST", + body: "Test activity content", + headers: { + "Content-Type": "application/activity+json", + }, + }); + const error = await assertRejects( + () => + doubleKnock( + request, + { + keyId: rsaPublicKey2.id!, + privateKey: rsaPrivateKey2, + }, + ), + FetchError, + "connection reset", + ); + + assertEquals(error.url.href, "https://example.com/flaky-inbox"); + assertEquals(error.cause, failure); + assertEquals(requestCount, 1); + } finally { + fetchMock.hardReset(); + } +}); + +test("doubleKnock() preserves Request signal abort reasons", async () => { + const controller = new AbortController(); + const abortReason = "request aborted"; + controller.abort(abortReason); + + const request = new Request("https://example.com/request-abort", { + signal: controller.signal, + }); + const error = await assertRejects( + () => + doubleKnock( + request, + { + keyId: rsaPublicKey2.id!, + privateKey: rsaPrivateKey2, + }, + ), + ); + + assertEquals(error, abortReason); +}); + +test("doubleKnock() preserves Request signal aborts during retry delay", async () => { + fetchMock.spyGlobal(); + + try { + let requestCount = 0; + const controller = new AbortController(); + const abortReason = "retry aborted"; + fetchMock.get("https://example.com/aborted-retry", () => { + requestCount++; + setTimeout(() => controller.abort(abortReason)); + throw new TypeError("temporary DNS failure"); + }); + + const request = new Request("https://example.com/aborted-retry", { + signal: controller.signal, + }); + const error = await assertRejects( + () => + doubleKnock( + request, + { + keyId: rsaPublicKey2.id!, + privateKey: rsaPrivateKey2, + }, + ), + ); + + assertEquals(error, abortReason); + assertEquals(requestCount, 1); + } finally { + fetchMock.hardReset(); + } +}); + +test("doubleKnock() prefers Request aborts over transport errors", async () => { + fetchMock.spyGlobal(); + + try { + let requestCount = 0; + const controller = new AbortController(); + const abortReason = "transport aborted"; + fetchMock.get("https://example.com/abort-with-transport-error", () => { + requestCount++; + controller.abort(abortReason); + throw new TypeError("temporary DNS failure"); + }); + + const request = new Request( + "https://example.com/abort-with-transport-error", + { signal: controller.signal }, + ); + const error = await assertRejects( + () => + doubleKnock( + request, + { + keyId: rsaPublicKey2.id!, + privateKey: rsaPrivateKey2, + }, + ), + ); + + assertEquals(error, abortReason); + assertEquals(requestCount, 1); + } finally { + fetchMock.hardReset(); + } +}); + test("doubleKnock() async specDeterminer test", async () => { // Install mock fetch handler fetchMock.spyGlobal(); diff --git a/packages/fedify/src/sig/http.ts b/packages/fedify/src/sig/http.ts index 6c01b066e..04a0beb0b 100644 --- a/packages/fedify/src/sig/http.ts +++ b/packages/fedify/src/sig/http.ts @@ -24,6 +24,7 @@ import metadata from "../../deno.json" with { type: "json" }; import { fetchKey, type KeyCache, validateCryptoKey } from "./key.ts"; const DEFAULT_MAX_REDIRECTION = 20; +const DOUBLE_KNOCK_TRANSPORT_RETRY_DELAY_MS = 100; /** * The standard to use for signing and verifying HTTP signatures. @@ -1335,6 +1336,98 @@ function createRedirectRequest( }); } +async function fetchDoubleKnockRequest( + request: Request, + signedRequest: Request, + signal?: AbortSignal, +): Promise { + const maxAttempts = request.method === "GET" || request.method === "HEAD" + ? 2 + : 1; + for (let attempt = 1;; attempt++) { + try { + return await fetch(signedRequest, { + // Since Bun has a bug that ignores the `Request.redirect` option, + // to work around it we specify `redirect: "manual"` here too: + // https://github.com/oven-sh/bun/issues/10754 + redirect: "manual", + signal, + }); + } catch (error) { + const abortedSignal = getAbortedSignal( + signal, + request.signal, + signedRequest.signal, + ); + if (abortedSignal != null) throw getAbortReason(abortedSignal); + if (isAbortError(error)) { + throw error; + } + if (attempt >= maxAttempts) throw createFetchError(request.url, error); + await sleep( + DOUBLE_KNOCK_TRANSPORT_RETRY_DELAY_MS, + signal, + request.signal, + signedRequest.signal, + ); + } + } +} + +function createFetchError(url: string, cause: unknown): FetchError { + const message = cause instanceof Error ? cause.message : String(cause); + const error = new FetchError(url, message); + error.cause = cause; + return error; +} + +function isAbortError(error: unknown): boolean { + return error instanceof Error && error.name === "AbortError"; +} + +async function sleep( + ms: number, + ...signals: (AbortSignal | undefined)[] +): Promise { + const abortSignals = signals.filter((signal) => signal != null); + const abortedSignal = getAbortedSignal(...abortSignals); + if (abortedSignal != null) throw getAbortReason(abortedSignal); + if (abortSignals.length < 1) { + await new Promise((resolve) => setTimeout(resolve, ms)); + return; + } + await new Promise((resolve, reject) => { + const removeAbortListeners = () => { + for (const signal of abortSignals) { + signal.removeEventListener("abort", handleAbort); + } + }; + const timeout = setTimeout(() => { + removeAbortListeners(); + resolve(); + }, ms); + function handleAbort(event: Event) { + clearTimeout(timeout); + removeAbortListeners(); + reject(getAbortReason(event.currentTarget as AbortSignal)); + } + for (const signal of abortSignals) { + signal.addEventListener("abort", handleAbort, { once: true }); + } + }); +} + +function getAbortedSignal( + ...signals: (AbortSignal | undefined)[] +): AbortSignal | undefined { + return signals.find((signal) => signal?.aborted); +} + +function getAbortReason(signal: AbortSignal): unknown { + return signal.reason ?? + new DOMException("The operation was aborted.", "AbortError"); +} + /** * Performs a double-knock request to the given URL. For the details of * double-knocking, see @@ -1381,13 +1474,7 @@ async function doubleKnockInternal( { spec: firstTrySpec, tracerProvider, body }, ); log?.(signedRequest); - let response = await fetch(signedRequest, { - // Since Bun has a bug that ignores the `Request.redirect` option, - // to work around it we specify `redirect: "manual"` here too: - // https://github.com/oven-sh/bun/issues/10754 - redirect: "manual", - signal, - }); + let response = await fetchDoubleKnockRequest(request, signedRequest, signal); // Follow redirects manually to get the final URL: if ( response.status >= 300 && response.status < 400 && @@ -1444,13 +1531,7 @@ async function doubleKnockInternal( { spec, tracerProvider, body }, ); log?.(signedRequest); - response = await fetch(signedRequest, { - // Since Bun has a bug that ignores the `Request.redirect` option, - // to work around it we specify `redirect: "manual"` here too: - // https://github.com/oven-sh/bun/issues/10754 - redirect: "manual", - signal, - }); + response = await fetchDoubleKnockRequest(request, signedRequest, signal); // Follow redirects manually to get the final URL: if ( response.status >= 300 && response.status < 400 &&