diff --git a/CHANGES.md b/CHANGES.md index 32d10f6b2..90c5c79a8 100644 --- a/CHANGES.md +++ b/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 diff --git a/changes.d/backfill/context-load-failures.md b/changes.d/backfill/context-load-failures.md new file mode 100644 index 000000000..4256845ba --- /dev/null +++ b/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] diff --git a/packages/backfill/README.md b/packages/backfill/README.md index 49e515e0f..1fcf784ac 100644 --- a/packages/backfill/README.md +++ b/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 diff --git a/packages/backfill/src/backfill.test.ts b/packages/backfill/src/backfill.test.ts index d1aff50cd..1a028e603 100644 --- a/packages/backfill/src/backfill.test.ts +++ b/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); + }); +}); diff --git a/packages/backfill/src/backfill.ts b/packages/backfill/src/backfill.ts index b2c9363d5..c19488e50 100644 --- a/packages/backfill/src/backfill.ts +++ b/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 { 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 | 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; } }