From ecc965346a866af528f9ad3292edeed225a98fc3 Mon Sep 17 00:00:00 2001 From: Anton Wiklund Date: Sun, 8 Nov 2026 05:41:36 +0100 Subject: [PATCH] fix(application): look up unclaimed documents once per address The instance check and the per-deck check skip each deck's unclaimed documents in any spelling of their address. They did so by comparing every document of the instance with every unclaimed document of every deck, parsing both addresses each time, so the work grew with the square of the decks a catalog names and with their addresses' length. A foreign instance listing many decks whose documents are unclaimed could hold the tab's main thread for long on every open. The unclaimed documents are now brought to the form a request sends once per check, keyed by pieces so a long address hashes in full, and each document of the instance is looked up once. Every spelling of an unclaimed document is still skipped. Refs: GHSA-p55m-36p9-7298 --- docs/validation.md | 7 ++- .../application/src/instanceChecks.test.ts | 54 +++++++++++++++++++ packages/application/src/instanceChecks.ts | 43 +++++++++++---- 3 files changed, 91 insertions(+), 13 deletions(-) diff --git a/docs/validation.md b/docs/validation.md index 14f46684..38055010 100644 --- a/docs/validation.md +++ b/docs/validation.md @@ -135,8 +135,11 @@ flowchart LR instance's own folder (the same `data-model.md` section). It's no violation: nothing is blocked or set aside, and the deck is still read and studied from. Those documents aren't - checked, under any spelling of their address (`isSameUrl`), since a - repair writes to the document. Solid Memo's data + checked, under any spelling of their address, since a + repair writes to the document. Each address is compared as a + request sends it (`canonicalUrlOf`), the unclaimed ones brought to + that form once per check, so a catalog of many decks costs one + lookup per document, not a comparison of every pair. Solid Memo's data check and the Studio's Health screen show them (`UnclaimedDecksNotice`) with "These are this deck's documents", which claims them once the user confirms diff --git a/packages/application/src/instanceChecks.test.ts b/packages/application/src/instanceChecks.test.ts index 09a01323..5dd586cd 100644 --- a/packages/application/src/instanceChecks.test.ts +++ b/packages/application/src/instanceChecks.test.ts @@ -11,6 +11,23 @@ import type { LibraryDeckContent } from "@solid-memo/domain/library"; import { instance, deck, libraryContent, makeDeps } from "./testing/fakePorts"; import { current, card2, setup } from "./testing/digestWorld"; +/** How many addresses the code under test brings to the form a request sends (canonicalUrlOf) or compares (isSameUrl). */ +const urlWork = vi.hoisted(() => ({ count: 0 })); +vi.mock("@solid-memo/domain/instanceLayout", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + canonicalUrlOf: (text: string) => { + urlWork.count += 1; + return actual.canonicalUrlOf(text); + }, + isSameUrl: (a: string, b: string) => { + urlWork.count += 2; + return actual.isSameUrl(a, b); + }, + }; +}); + describe("instance checks", () => { it("planRepair plans from a report, and applyRepairs hands the repairs to the repository", async () => { const deps = makeDeps(); @@ -195,6 +212,43 @@ describe("instance checks", () => { expect(checked).toContain(`${instance.url}catalog.ttl`); expect(checked.filter((url) => url.includes("/notes/"))).toEqual([]); }); + it("checks an address that is no URL, unclaimed or not", async () => { + const { deps, useCases } = setup(); + const named = { ...deck, cardsDocumentUrl: "no url" }; + deps.deckRepository.unclaimedDocuments = vi.fn(async () => ["also no url"]); + await useCases.checkDeck(instance.url, named, { plain: (value) => value, check: () => [] }); + expect( + vi.mocked(deps.shapeValidator.validateDocumentSince).mock.calls.map(([url]) => url), + ).toEqual(expect.arrayContaining(["no url", deck.reviewsDocumentUrl])); + }); + it("skips the unclaimed documents of many decks in work that grows with their number, not its square", async () => { + const unclaimedWork = async (count: number) => { + const { deps, useCases } = setup(); + const decks = Array.from({ length: count }, (_, index) => ({ + ...deck, + id: `deck-${index}`, + url: `${instance.url}catalog.ttl#deck-${index}`, + cardsDocumentUrl: `https://elsewhere.example/c${index}.ttl`, + reviewsDocumentUrl: `https://elsewhere.example/r${index}.ttl`, + })); + deps.deckRepository.listDecks = vi.fn(async () => decks); + deps.deckRepository.unclaimedDocuments = vi.fn(async (one) => [ + one.cardsDocumentUrl, + one.reviewsDocumentUrl, + ]); + urlWork.count = 0; + const report = await useCases.checkInstance(instance.url); + const work = urlWork.count; + expect(report.unclaimed).toHaveLength(count); + expect( + vi.mocked(deps.shapeValidator.validateDocumentSince).mock.calls.map(([url]) => url), + ).not.toContain(decks[0]!.cardsDocumentUrl); + return work; + }; + const few = await unclaimedWork(100); + // Ten times the decks, about ten times the work; comparing every pair would be about a hundred. + expect(await unclaimedWork(1000)).toBeLessThan(few * 20); + }); describe("documents outside the instance's folder and any storage of the user's that holds it", () => { const ownElsewhere = "https://alice.example/notes/old-cards.ttl"; const others = "https://bob.example/memo/"; diff --git a/packages/application/src/instanceChecks.ts b/packages/application/src/instanceChecks.ts index e7b24c4d..245d62ad 100644 --- a/packages/application/src/instanceChecks.ts +++ b/packages/application/src/instanceChecks.ts @@ -5,11 +5,11 @@ import { planRepair, type Repair, type RepairPlan } from "@solid-memo/domain/repair"; import type { Deck } from "@solid-memo/domain/deck"; import { + canonicalUrlOf, catalogUrlOf, historyUrlOf, instanceDocumentUrls, isDocumentIn, - isSameUrl, } from "@solid-memo/domain/instanceLayout"; import { summarize, @@ -33,6 +33,7 @@ import type { import type { AnswerStore } from "./answerStore"; import { AppError } from "@solid-memo/domain/appError"; import { documentUrlOf } from "@solid-memo/domain/subjectUrl"; +import { TextMap } from "@solid-memo/domain/pathKeys"; import { mapAtOnce } from "./atOnce"; import { noting } from "./digestStore"; import type { DigestStore } from "./digestStore"; @@ -88,14 +89,32 @@ export interface InstanceCheckUseCases { } /** - * Whether the document at `url` is one of these decks' unclaimed ones, - * in whatever spelling (isSameUrl): a deck may name one document twice, - * and only the first spelling is listed as unclaimed. + * A test of whether the document at an address is one of these decks' + * unclaimed ones, in whatever spelling: a deck may name one document + * twice, and only the first spelling is listed as unclaimed. Each + * address is compared as a request would send it (canonicalUrlOf), the + * unclaimed ones brought to that form once for the test, so that a + * catalog naming many decks costs one lookup per address, not a + * comparison with every unclaimed document. */ -function isUnclaimed(url: string, unclaimed: readonly UnclaimedDeck[]): boolean { - return unclaimed.some((deck) => - deck.documents.some((document) => isSameUrl(documentUrlOf(document), documentUrlOf(url))), - ); +function unclaimedTest(unclaimed: readonly UnclaimedDeck[]): (url: string) => boolean { + // Keyed by pieces (TextMap): an address as a request sends it may be longer than V8 hashes all of. + const skipped = new TextMap(); + for (const deck of unclaimed) { + for (const document of deck.documents) { + const canonical = canonicalDocumentOf(document); + if (canonical !== null) skipped.set(canonical, true); + } + } + return (url) => { + const canonical = canonicalDocumentOf(url); + return canonical !== null && skipped.has(canonical); + }; +} + +/** The address of the document `url` is in, as a request sends it (canonicalUrlOf); null for one that is no URL. */ +function canonicalDocumentOf(url: string): string | null { + return canonicalUrlOf(documentUrlOf(url)); } /** @@ -195,7 +214,7 @@ export function createInstanceCheckUseCases({ /** * The instance's documents, then its drafts': each with where it is, * which picks its shapes. An unclaimed document is not checked, in any - * spelling of its address (isUnclaimed): it may be anyone's, and a + * spelling of its address (unclaimedTest): it may be anyone's, and a * repair of what the check finds writes to it. */ function documentsToCheck( @@ -204,9 +223,10 @@ export function createInstanceCheckUseCases({ drafts: readonly string[], unclaimed: readonly UnclaimedDeck[], ): { url: string; context: Exclude }[] { + const isUnclaimed = unclaimedTest(unclaimed); return [ ...instanceDocumentUrls(instanceUrl, decks) - .filter((url) => !isUnclaimed(url, unclaimed)) + .filter((url) => !isUnclaimed(url)) .map((url) => ({ url, context: "pod" as const })), ...drafts.map((url) => ({ url, context: "draft" as const })), ]; @@ -288,8 +308,9 @@ export function createInstanceCheckUseCases({ const [report, cards, release] = await Promise.all([ Promise.all([digestStore.of(instanceUrl), unclaimedDecksOf(instanceUrl, [deck])]).then( async ([digest, unclaimed]) => { + const isUnclaimed = unclaimedTest(unclaimed); const urls = [deck.cardsDocumentUrl, deck.reviewsDocumentUrl].filter( - (url) => !isUnclaimed(url, unclaimed), + (url) => !isUnclaimed(url), ); return withUnclaimed( await checkDocuments(