From b0433aa546e52c982fe1765c9e653739f5a55a11 Mon Sep 17 00:00:00 2001 From: Hong Minhee Date: Tue, 12 May 2026 15:33:21 +0900 Subject: [PATCH 1/4] Handle double-knock transport failures Wrap transport failures from doubleKnock() in FetchError so callers get a consistent document loading error instead of raw TypeError values from the runtime fetch implementation. Retry idempotent signed GET and HEAD requests once after a short delay, which covers transient DNS and connection hiccups without replaying non-idempotent inbox deliveries. Fixes https://github.com/fedify-dev/fedify/issues/762 Assisted-by: Codex:gpt-5.5 --- CHANGES.md | 9 +++ packages/fedify/src/sig/http.test.ts | 104 ++++++++++++++++++++++++++- packages/fedify/src/sig/http.ts | 80 +++++++++++++++++---- 3 files changed, 178 insertions(+), 15 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index c15bc1935..e467fc8fd 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -8,6 +8,15 @@ 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]] + +[#762]: https://github.com/fedify-dev/fedify/issues/762 + Version 2.0.16 -------------- diff --git a/packages/fedify/src/sig/http.test.ts b/packages/fedify/src/sig/http.test.ts index fc4ea3259..0d5034f90 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,108 @@ 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() 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..bceea3c59 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,69 @@ 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 <= maxAttempts; 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) { + if (isAbortError(error, signal)) throw error; + if (attempt >= maxAttempts) throw createFetchError(request.url, error); + await sleep(DOUBLE_KNOCK_TRANSPORT_RETRY_DELAY_MS, signal); + } + } + throw new FetchError(request.url); +} + +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, signal?: AbortSignal): boolean { + return error instanceof Error && error.name === "AbortError" || + signal?.aborted === true && error === signal.reason; +} + +async function sleep(ms: number, signal?: AbortSignal): Promise { + if (signal == null) { + await new Promise((resolve) => setTimeout(resolve, ms)); + return; + } + const retrySignal = signal; + if (retrySignal.aborted) throw getAbortReason(retrySignal); + await new Promise((resolve, reject) => { + const timeout = setTimeout(() => { + retrySignal.removeEventListener("abort", handleAbort); + resolve(); + }, ms); + function handleAbort() { + clearTimeout(timeout); + reject(getAbortReason(retrySignal)); + } + retrySignal.addEventListener("abort", handleAbort, { once: true }); + }); +} + +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 +1445,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 +1502,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 && From e50f619ccde249da434f138ba045e43dbe2251fd Mon Sep 17 00:00:00 2001 From: Hong Minhee Date: Tue, 12 May 2026 17:28:30 +0900 Subject: [PATCH 2/4] Preserve double-knock request aborts Keep aborts attached to the original Request or signed Request out of the transport retry path. This preserves custom abort reasons and avoids turning cancellation into FetchError during retry handling. Add regression coverage for pre-aborted requests and aborts that occur while waiting to retry an idempotent transport failure. Assisted-by: Codex:gpt-5.5 --- packages/fedify/src/sig/http.test.ts | 56 ++++++++++++++++++++++++++++ packages/fedify/src/sig/http.ts | 48 ++++++++++++++++++------ 2 files changed, 92 insertions(+), 12 deletions(-) diff --git a/packages/fedify/src/sig/http.test.ts b/packages/fedify/src/sig/http.test.ts index 0d5034f90..bcd574a4b 100644 --- a/packages/fedify/src/sig/http.test.ts +++ b/packages/fedify/src/sig/http.test.ts @@ -1827,6 +1827,62 @@ test("doubleKnock() does not retry non-idempotent transport errors", async () => } }); +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() 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 bceea3c59..b43f3f742 100644 --- a/packages/fedify/src/sig/http.ts +++ b/packages/fedify/src/sig/http.ts @@ -1354,9 +1354,16 @@ async function fetchDoubleKnockRequest( signal, }); } catch (error) { - if (isAbortError(error, signal)) throw error; + if (isAbortError(error, signal, request.signal, signedRequest.signal)) { + throw error; + } if (attempt >= maxAttempts) throw createFetchError(request.url, error); - await sleep(DOUBLE_KNOCK_TRANSPORT_RETRY_DELAY_MS, signal); + await sleep( + DOUBLE_KNOCK_TRANSPORT_RETRY_DELAY_MS, + signal, + request.signal, + signedRequest.signal, + ); } } throw new FetchError(request.url); @@ -1369,28 +1376,45 @@ function createFetchError(url: string, cause: unknown): FetchError { return error; } -function isAbortError(error: unknown, signal?: AbortSignal): boolean { +function isAbortError( + error: unknown, + ...signals: (AbortSignal | undefined)[] +): boolean { return error instanceof Error && error.name === "AbortError" || - signal?.aborted === true && error === signal.reason; + signals.some((signal) => + signal?.aborted === true && error === signal.reason + ); } -async function sleep(ms: number, signal?: AbortSignal): Promise { - if (signal == null) { +async function sleep( + ms: number, + ...signals: (AbortSignal | undefined)[] +): Promise { + const abortSignals = signals.filter((signal) => signal != null); + const abortedSignal = abortSignals.find((signal) => signal.aborted); + if (abortedSignal != null) throw getAbortReason(abortedSignal); + if (abortSignals.length < 1) { await new Promise((resolve) => setTimeout(resolve, ms)); return; } - const retrySignal = signal; - if (retrySignal.aborted) throw getAbortReason(retrySignal); await new Promise((resolve, reject) => { + const removeAbortListeners = () => { + for (const signal of abortSignals) { + signal.removeEventListener("abort", handleAbort); + } + }; const timeout = setTimeout(() => { - retrySignal.removeEventListener("abort", handleAbort); + removeAbortListeners(); resolve(); }, ms); - function handleAbort() { + function handleAbort(event: Event) { clearTimeout(timeout); - reject(getAbortReason(retrySignal)); + removeAbortListeners(); + reject(getAbortReason(event.currentTarget as AbortSignal)); + } + for (const signal of abortSignals) { + signal.addEventListener("abort", handleAbort, { once: true }); } - retrySignal.addEventListener("abort", handleAbort, { once: true }); }); } From dae7217f486e2edef9f450e5bc1fee6d9f7b7325 Mon Sep 17 00:00:00 2001 From: Hong Minhee Date: Tue, 12 May 2026 18:22:53 +0900 Subject: [PATCH 3/4] Add a PR link to the changelog --- CHANGES.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGES.md b/CHANGES.md index e467fc8fd..aac260030 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -13,9 +13,10 @@ To be released. - 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]] + `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 From 5aa596605817dfa9d2dd30e4705955f62c0f8b4e Mon Sep 17 00:00:00 2001 From: Hong Minhee Date: Tue, 12 May 2026 18:37:37 +0900 Subject: [PATCH 4/4] Honor aborts in retry handling Prefer cancellation over transport failure when a signal aborts while fetching or waiting to retry. This keeps doubleKnock() from wrapping a late cancellation as FetchError. Remove the now-unneeded fallback throw after the retry loop by making the loop explicit about running until it returns or throws. https://github.com/fedify-dev/fedify/pull/763#discussion_r3225259523 https://github.com/fedify-dev/fedify/pull/763#discussion_r3225266552 https://github.com/fedify-dev/fedify/pull/763#discussion_r3225266572 Assisted-by: Codex:gpt-5.5 --- packages/fedify/src/sig/http.test.ts | 35 ++++++++++++++++++++++++++++ packages/fedify/src/sig/http.ts | 29 +++++++++++++---------- 2 files changed, 52 insertions(+), 12 deletions(-) diff --git a/packages/fedify/src/sig/http.test.ts b/packages/fedify/src/sig/http.test.ts index bcd574a4b..b020f34fe 100644 --- a/packages/fedify/src/sig/http.test.ts +++ b/packages/fedify/src/sig/http.test.ts @@ -1883,6 +1883,41 @@ test("doubleKnock() preserves Request signal aborts during retry delay", async ( } }); +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 b43f3f742..04a0beb0b 100644 --- a/packages/fedify/src/sig/http.ts +++ b/packages/fedify/src/sig/http.ts @@ -1344,7 +1344,7 @@ async function fetchDoubleKnockRequest( const maxAttempts = request.method === "GET" || request.method === "HEAD" ? 2 : 1; - for (let attempt = 1; attempt <= maxAttempts; attempt++) { + for (let attempt = 1;; attempt++) { try { return await fetch(signedRequest, { // Since Bun has a bug that ignores the `Request.redirect` option, @@ -1354,7 +1354,13 @@ async function fetchDoubleKnockRequest( signal, }); } catch (error) { - if (isAbortError(error, signal, request.signal, signedRequest.signal)) { + 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); @@ -1366,7 +1372,6 @@ async function fetchDoubleKnockRequest( ); } } - throw new FetchError(request.url); } function createFetchError(url: string, cause: unknown): FetchError { @@ -1376,14 +1381,8 @@ function createFetchError(url: string, cause: unknown): FetchError { return error; } -function isAbortError( - error: unknown, - ...signals: (AbortSignal | undefined)[] -): boolean { - return error instanceof Error && error.name === "AbortError" || - signals.some((signal) => - signal?.aborted === true && error === signal.reason - ); +function isAbortError(error: unknown): boolean { + return error instanceof Error && error.name === "AbortError"; } async function sleep( @@ -1391,7 +1390,7 @@ async function sleep( ...signals: (AbortSignal | undefined)[] ): Promise { const abortSignals = signals.filter((signal) => signal != null); - const abortedSignal = abortSignals.find((signal) => signal.aborted); + const abortedSignal = getAbortedSignal(...abortSignals); if (abortedSignal != null) throw getAbortReason(abortedSignal); if (abortSignals.length < 1) { await new Promise((resolve) => setTimeout(resolve, ms)); @@ -1418,6 +1417,12 @@ async function sleep( }); } +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");