Merge pull request #1258 from sij411/fix/1249

Continue backfill after context load failures

Author: dahlia

CHANGES.md

@@ -14,9 +14,15 @@ To be released.
     collections.  Backfill now follows the first and subsequent pages while
     respecting traversal limits, and retains posts already found if a page
     cannot be loaded.  [[#1248], [#1256] by Jiwon Kwon\]
+ -  Fixed conversation backfill stopping when the context collection loader
+    failed.  Failed context loads are now skipped so later configured strategies
+    can still find accessible posts.  Cancellation and interval configuration
+    errors continue to propagate.  [[#1249], [#1258] by Jiwon Kwon\]
 
 [#1248]: https://github.com/fedify-dev/fedify/issues/1248
+[#1249]: https://github.com/fedify-dev/fedify/issues/1249
 [#1256]: https://github.com/fedify-dev/fedify/pull/1256
+[#1258]: https://github.com/fedify-dev/fedify/pull/1258
 
 
 Version 2.3.11

changes.d/backfill/context-load-failures.md

@@ -0,0 +1,9 @@
+---
+links:
+  '#1249': https://github.com/fedify-dev/fedify/issues/1249
+  '#1258': https://github.com/fedify-dev/fedify/pull/1258
+---
+ -  Fixed conversation backfill stopping when the context collection loader
+    failed.  Failed context loads are now skipped so later configured strategies
+    can still find accessible posts.  Cancellation and interval configuration
+    errors continue to propagate.  [[#1249], [#1258] by Jiwon Kwon]

packages/backfill/README.md

@@ -153,7 +153,11 @@ An `interval` string requires the global `Temporal` API or a polyfill.
 
 If the seed has no context, or its context resolves to a non-collection,
 context strategies yield nothing.  Loader failures are skipped unless
-traversal is aborted.
+traversal is aborted.  If the context collection cannot be loaded, later
+configured strategies can still run.  Failed loads consume a request, and
+later strategies can use embedded data even when the request budget is
+exhausted.  Configuration errors, such as an invalid `interval`, still
+propagate.
 
 Dereferenced documents are cached in memory for one `backfill()` traversal.
 Applications that need persistent or shared caching can provide it through

packages/backfill/src/backfill.test.ts

@@ -2028,3 +2028,167 @@ test("embedded pages without IDs follow next without loader calls", async () =>
   );
   deepStrictEqual(items.map((item) => item.id?.href), [a.id?.href, b.id?.href]);
 });
+
+describe("context loader failures", () => {
+  const contextId = new URL("https://example.com/thread");
+  const parent = new Note({ id: new URL("https://example.com/parent") });
+
+  for (const synchronous of [false, true]) {
+    const failure = synchronous ? "synchronous throw" : "promise rejection";
+    const fail = () => {
+      const error = new Error("Context collection unavailable");
+      if (synchronous) throw error;
+      return Promise.reject(error);
+    };
+
+    test(`${failure} allows reply-tree to yield an embedded parent`, async () => {
+      const seed = new Note({ contexts: [contextId], replyTarget: parent });
+      let requests = 0;
+      const items = await collect(
+        {
+          documentLoader: () => {
+            requests++;
+            return fail();
+          },
+        },
+        seed,
+        { strategies: ["context-auto", "reply-tree"], maxRequests: 1 },
+      );
+      deepStrictEqual(items.map((item) => item.object), [parent]);
+      strictEqual(items[0].strategy, "reply-tree");
+      strictEqual(requests, 1);
+    });
+
+    test(`${failure} finishes the default strategy without items`, async () => {
+      const seed = new Note({ contexts: [contextId] });
+      deepStrictEqual(await collect({ documentLoader: fail }, seed), []);
+    });
+
+    test(`${failure} consumes the request budget`, async () => {
+      const seed = new Note({ contexts: [contextId], replyTarget: parent.id });
+      const requests: string[] = [];
+      const items = await collect(
+        {
+          documentLoader: (url) => {
+            requests.push(url.href);
+            if (url.href === contextId.href) return fail();
+            return Promise.resolve(parent);
+          },
+        },
+        seed,
+        { strategies: ["context-auto", "reply-tree"], maxRequests: 1 },
+      );
+      deepStrictEqual(items, []);
+      deepStrictEqual(requests, [contextId.href]);
+    });
+
+    test(`${failure} propagates the cancellation reason`, async () => {
+      const controller = new AbortController();
+      const reason = new Error("Stop backfill");
+      const seed = new Note({ contexts: [contextId], replyTarget: parent });
+      const yielded: unknown[] = [];
+      await rejects(async () => {
+        for await (
+          const item of backfill(
+            {
+              documentLoader: (_url, options) => {
+                strictEqual(options?.signal, controller.signal);
+                controller.abort(reason);
+                return fail();
+              },
+            },
+            seed,
+            {
+              strategies: ["context-auto", "reply-tree"],
+              signal: controller.signal,
+            },
+          )
+        ) yielded.push(item);
+      }, (error) => error === reason);
+      deepStrictEqual(yielded, []);
+    });
+  }
+
+  test("a failed context load is not cached for later reply-tree loads", async () => {
+    const seed = new Note({ contexts: [contextId], replyTarget: contextId });
+    let requests = 0;
+    const items = await collect(
+      {
+        documentLoader: (url) => {
+          strictEqual(url.href, contextId.href);
+          requests++;
+          if (requests === 1) {
+            return Promise.reject(
+              new Error("Temporary failure"),
+            );
+          }
+          return Promise.resolve(parent);
+        },
+      },
+      seed,
+      { strategies: ["context-auto", "reply-tree"], maxRequests: 2 },
+    );
+    deepStrictEqual(items.map((item) => item.object.id?.href), [
+      parent.id?.href,
+    ]);
+    strictEqual(requests, 2);
+  });
+
+  test("an exhausted budget still allows embedded reply-tree data", async () => {
+    const seed = new Note({ contexts: [contextId], replyTarget: parent });
+    const items = await collect(
+      {
+        documentLoader: () => {
+          throw new Error("No requests allowed");
+        },
+      },
+      seed,
+      { strategies: ["context-auto", "reply-tree"], maxRequests: 0 },
+    );
+    deepStrictEqual(items.map((item) => item.object), [parent]);
+  });
+
+  test("interval callback errors propagate before the loader is called", async () => {
+    const reason = new Error("Invalid interval configuration");
+    let requests = 0;
+    const seed = new Note({ contexts: [contextId], replyTarget: parent });
+    await rejects(
+      collect(
+        {
+          documentLoader: () => {
+            requests++;
+            return Promise.resolve(null);
+          },
+        },
+        seed,
+        {
+          strategies: ["context-auto", "reply-tree"],
+          interval: () => {
+            throw reason;
+          },
+        },
+      ),
+      (error) => error === reason,
+    );
+    strictEqual(requests, 0);
+  });
+
+  test("invalid interval strings still propagate", async () => {
+    let requests = 0;
+    const seed = new Note({ contexts: [contextId], replyTarget: parent });
+    await rejects(collect(
+      {
+        documentLoader: () => {
+          requests++;
+          return Promise.resolve(null);
+        },
+      },
+      seed,
+      {
+        strategies: ["context-auto", "reply-tree"],
+        interval: "not a duration",
+      },
+    ));
+    strictEqual(requests, 0);
+  });
+});

packages/backfill/src/backfill.ts

@@ -200,7 +200,9 @@ async function* getContextStrategyItems(
 }> {
   const contextId = note.contextIds[0];
   if (contextId == null) return;
-  const collection = await loadObject(context, contextId, options, budget);
+  const collection = await loadObject(context, contextId, options, budget, {
+    skipLoaderErrors: true,
+  });
   if (!isCollection(collection)) return;
   for await (
     const object of getCollectionItems(
@@ -536,7 +538,7 @@ async function* getCollectionItems(
         new URL(url),
         options,
         budget,
-        true,
+        { throwOnBudgetExceeded: true },
       );
       if (object == null) throw new Error(`Collection page not found: ${url}`);
       return {
@@ -621,7 +623,7 @@ async function loadCollectionItemDocument(
       iri,
       options,
       budget,
-      true,
+      { throwOnBudgetExceeded: true },
     );
   } catch (error) {
     if (error instanceof MaxRequestsExceeded) throw error;
@@ -652,7 +654,13 @@ async function loadObject(
   iri: URL,
   options: BackfillOptions,
   budget: RequestBudget,
-  throwOnBudgetExceeded = false,
+  {
+    throwOnBudgetExceeded = false,
+    skipLoaderErrors = false,
+  }: {
+    throwOnBudgetExceeded?: boolean;
+    skipLoaderErrors?: boolean;
+  } = {},
 ): Promise<APObject | null> {
   budget.signal?.throwIfAborted();
   const cacheKey = iri.href;
@@ -671,14 +679,19 @@ async function loadObject(
   budget.signal?.throwIfAborted();
 
   budget.requestCount++;
-  const document = context.documentLoader(iri, { signal: budget.signal });
-  budget.documents.set(cacheKey, document);
+  let document: Promise<APObject | null> | undefined;
   try {
+    document = context.documentLoader(iri, { signal: budget.signal });
+    budget.documents.set(cacheKey, document);
     return await document;
   } catch (error) {
     if (budget.documents.get(cacheKey) === document) {
       budget.documents.delete(cacheKey);
     }
+    if (skipLoaderErrors) {
+      budget.signal?.throwIfAborted();
+      return null;
+    }
     throw error;
   }
 }