Handle double-knock transport failures

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
 --------------

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();

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<Response> {
+  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<void> {
+  const abortSignals = signals.filter((signal) => signal != null);
+  const abortedSignal = getAbortedSignal(...abortSignals);
+  if (abortedSignal != null) throw getAbortReason(abortedSignal);
+  if (abortSignals.length < 1) {
+    await new Promise<void>((resolve) => setTimeout(resolve, ms));
+    return;
+  }
+  await new Promise<void>((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 &&