From e5d73a670bb0ae406c6c4f0bdc4e756c02d590f8 Mon Sep 17 00:00:00 2001 From: Sylvain Duchesne Date: Tue, 15 Sep 2026 12:12:47 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20une=20panne=20n'est=20plus=20mise=20en?= =?UTF-8?q?=20cache=20comme=20une=20absence=20de=20capacit=C3=A9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../src/emulated-verifier/public-store.ts | 110 ++++++++++++----- packages/polyfill/test/public-store.test.ts | 111 ++++++++++++++++-- 2 files changed, 181 insertions(+), 40 deletions(-) diff --git a/packages/polyfill/src/emulated-verifier/public-store.ts b/packages/polyfill/src/emulated-verifier/public-store.ts index b54f346..8a743dc 100644 --- a/packages/polyfill/src/emulated-verifier/public-store.ts +++ b/packages/polyfill/src/emulated-verifier/public-store.ts @@ -102,10 +102,28 @@ import type { Nuri, ReadCap } from "../model/types"; * The round-trip is what is worth saving, not the filing. So the memo holds the value and * the caller files it for whoever is connected, every time. * - * A scope never changes here (a document is created in a store and stays there), so a - * cached `null` cannot go stale for a document that existed when it was taken. It CAN for - * one created afterwards in the same page — {@link resetPublicStoreFetches} is the way - * out, and a session or wallet reset calls it. + * ── What may be memoised is an ANSWER — never a failure ─────────────────── + * Two values are filed here, and each is safe to keep for its OWN reason: + * - a **cap** — the document served it, and nothing withdraws it within a session; + * - **`null`** — the document's Header branch was READ and carries no exposed cap: an + * ESTABLISHED absence. Safe because a scope never changes here (a document is created + * in a store and stays there), so that answer cannot go stale for a document that + * existed when it was taken. + * A fetch that THREW is neither of those. The broker was not asked, or could not answer, + * and "no answer" is not "no cap" — so it is dropped from the map the moment it rejects + * and the next caller asks again. {@link downloadReadCap} is what keeps the two apart. + * + * That separation repairs a defect that stood until 2026-09-15: the download caught its + * own errors and returned the same `null` as a real absence, so ONE transient outage + * recorded "no capability" for that document for the rest of the session. Deposits being + * consumed, a deposit on that document was then retired before its arm could read + * anything — and every re-deposit sent to repair it met the same memoised `null` and was + * destroyed too. The justification above was written when `null` meant absence and + * ONLY absence; it was left standing when the `catch` gave the value a second meaning it + * never covered. It is now stated per value, so it cannot silently widen again. + * + * A cached absence CAN still go stale for a document created afterwards in the same page — + * {@link resetPublicStoreFetches} is the way out, and a session or wallet reset calls it. */ const attempted = new Map>(); @@ -163,8 +181,10 @@ export async function exposeReadCap(doc: Nuri, cap: ReadCap): Promise { * Nothing is asked when the cap is already held: a document you can read needs no * fetching, and skipping it keeps the ordinary path free of physical reads. * - * Never throws — a document that is not in a public store simply answers nothing, which - * is not an error but the normal case. + * Never throws. Two different negatives reach the caller as the same `false`, and the + * caller is right not to tell them apart — it obtained no cap either way. They are kept + * apart where it matters, in the memo: an absence is remembered, a failure is not (see + * the note on {@link attempted}). */ // @provenance fetchReadCap kind=declared-not-wired level=1 ref=engine/net/src/types.rs:PublicRepoLinkV0 — the type DECLARES the cap is downloaded from the outer overlay if the brokers allow; it is constructed nowhere, both `PinRepo` constructors hard-code `expose_outer: false`, and `ExtTopicSyncReq` is `unimplemented!()` export async function fetchReadCap(docLike: Nuri): Promise { @@ -179,8 +199,29 @@ export async function fetchReadCap(docLike: Nuri): Promise { if (pending === undefined) { pending = downloadReadCap(doc); attempted.set(doc, pending); + // A failure is not an answer, so it must not be filed as one: drop the memo the + // moment the download rejects, and the next caller reaches the broker again. + // Attached HERE — once per download, before anybody awaits — so the eviction and the + // report each happen exactly once however many callers share this one in-flight + // fetch, and happen at all even if every one of them has already given up. The + // identity check is what keeps it from evicting a LATER attempt: by the time a + // rejection lands, the map may already hold a fresh promise for this document. + const started = pending; + void started.catch((error: unknown) => { + if (attempted.get(doc) === started) attempted.delete(doc); + console.error(accessLogPrefix() + " fetchReadCap failed:", error); + }); + } + let cap: ReadCap | null; + try { + cap = await pending; + } catch { + // Reported and forgotten by the handler attached where the download started, just + // above — not swallowed here. The answer to the caller is the same as for an absence + // (no cap was obtained), but the memo now holds nothing for this document, so the + // next call asks the network instead of replaying this failure. + return false; } - const cap = await pending; if (cap === null) return false; // Filed for whoever is connected NOW, on every call — the memo spares the round-trip, // not the filing. See the note on {@link attempted}. @@ -189,33 +230,38 @@ export async function fetchReadCap(docLike: Nuri): Promise { return true; } -/** The fetch itself, through the machinery's door. Returns the cap, or `null` when the - * document is not in a public store — which is the normal case, not an error. */ +/** + * The fetch itself, through the machinery's door. **Two outcomes, deliberately not one** — + * telling them apart is this function's whole shape: + * + * - it **resolves** — the Header branch was READ. With the cap when the document exposes + * one; with `null` when it does not, which is the normal case and not an error. Either + * way the network answered, so the value is an answer and {@link attempted} may keep it. + * - it **rejects** — the question could not be put at all: the repo would not open, the + * broker did not answer, the read threw. That is "no answer", not "no cap", and + * {@link fetchReadCap} keeps it out of the memo for exactly that reason. + * + * So nothing is caught here. It used to be, and the two collapsed into the same `null`. + */ async function downloadReadCap(doc: Nuri): Promise { const s = await session(); - try { - // The repo has to be in the session before an anchored read resolves it — the - // cold-start heal, through the PHYSICAL door: this is the emulated broker serving - // an outer overlay, and it does not ask who is asking (see `open-repo.ts`). - await ensurePhysicalRepoOpen(doc); - const res = await physicalQuery( - s.sessionId, - `SELECT ?c WHERE { <${HEADER_BRANCH_SUBJECT}> <${P.exposedReadCap}> ?c }`, - undefined, - doc, - "fetchReadCap", - ); - for (const row of readBindings(res)) { - const cap = bindingValue(row, "c"); - // `targetOf` guards the one confusion that would matter: a cap exposed on - // document A must not file a cap for document B. A document only ever speaks - // for itself. - if (cap && hasReadCap(cap) && targetOf(cap) === doc) return cap; - } - } catch (error) { - // Not in a public store, not synced, or no such document — all of them mean the - // same thing to the caller: no cap was obtained. - console.error(accessLogPrefix() + " fetchReadCap failed:", error); + // The repo has to be in the session before an anchored read resolves it — the + // cold-start heal, through the PHYSICAL door: this is the emulated broker serving + // an outer overlay, and it does not ask who is asking (see `open-repo.ts`). + await ensurePhysicalRepoOpen(doc); + const res = await physicalQuery( + s.sessionId, + `SELECT ?c WHERE { <${HEADER_BRANCH_SUBJECT}> <${P.exposedReadCap}> ?c }`, + undefined, + doc, + "fetchReadCap", + ); + for (const row of readBindings(res)) { + const cap = bindingValue(row, "c"); + // `targetOf` guards the one confusion that would matter: a cap exposed on + // document A must not file a cap for document B. A document only ever speaks + // for itself. + if (cap && hasReadCap(cap) && targetOf(cap) === doc) return cap; } return null; } diff --git a/packages/polyfill/test/public-store.test.ts b/packages/polyfill/test/public-store.test.ts index 4f553d4..9e6f6f8 100644 --- a/packages/polyfill/test/public-store.test.ts +++ b/packages/polyfill/test/public-store.test.ts @@ -33,19 +33,27 @@ function inject() { if (m) quads.push({ g: anchor, s: m[1]!, p: m[2]!, o: m[3]! }); return undefined; }); - const sparql_query = mock(async (...a: unknown[]) => ({ - results: { - bindings: quads - .filter((q) => q.g === (a[3] as string) && q.p === `${SHIM}:exposedReadCap`) - .map((q) => ({ c: { value: q.o } })), - }, - })); + // Documents the emulated broker cannot be asked about right now — an outage, a repo + // this session has not synced, a rejected read. Scoped to ONE anchor on purpose: the + // connection work `setCurrentUser` fires reads on its own account, and an outage that + // took those down would be testing something else. + const unreachable = new Set(); + const sparql_query = mock(async (...a: unknown[]) => { + if (unreachable.has(a[3] as string)) throw new Error("emulated broker: cannot serve that overlay"); + return { + results: { + bindings: quads + .filter((q) => q.g === (a[3] as string) && q.p === `${SHIM}:exposedReadCap`) + .map((q) => ({ c: { value: q.o } })), + }, + }; + }); configure({ ng: { doc_create: mock(async () => "did:ng:o:x"), sparql_update, sparql_query } as any, useShape: (() => {}) as any }); configureStoreRegistry({ getSession: async () => SESSION }); resetCaps(); resetPublicStoreFetches(); setCurrentUser(null); - return { sparql_query, quads }; + return { sparql_query, quads, unreachable }; } afterEach(() => { @@ -186,3 +194,90 @@ test("resetting the caps forgets the memo — a stale yes would hand back what i expect(await fetchReadCap(PUB)).toBe(true); expect(sparql_query.mock.calls.length).toBeGreaterThan(before); // asked again }); + +// ── A failure is not an answer ─────────────────────────────────────────────── +// REGRESSION (2026-09-15). `downloadReadCap` caught its own errors and returned the same +// `null` as a genuine absence, which the memo then held for the whole session: ONE +// transient outage recorded "no capability" for that document until the session ended. +// Deposits being consumed, the deposit was retired before its arm read anything — and +// every re-deposit sent to repair it met the same memoised `null` and was destroyed too. + +/** How many times the network was asked ABOUT `nuri` — anchored reads only, so the + * connection work happening in the background on other documents does not count. */ +function asksAbout(sparql_query: { mock: { calls: unknown[][] } }, nuri: string): number { + return sparql_query.mock.calls.filter((c) => c[3] === nuri).length; +} + +test("a fetch that FAILED is asked again, and the cap arrives once the broker recovers", async () => { + const { sparql_query, unreachable } = inject(); + setCurrentUser("alice"); + await aliceExposesHerNote(); + armEmulation(); + setCurrentUser("bob"); + + unreachable.add(PUB); // the broker cannot answer for this document + expect(await fetchReadCap(PUB)).toBe(false); + expect(asksAbout(sparql_query, PUB)).toBe(1); + expect(getCaps().capFor(PUB)).toBeUndefined(); + + unreachable.delete(PUB); // …and recovers + expect(await fetchReadCap(PUB)).toBe(true); + expect(asksAbout(sparql_query, PUB)).toBe(2); // asked AGAIN — the outage was not an answer + expect(getCaps().capFor(PUB)).toBe(mintCap(PUB)); +}); + +// The two negatives in one test, so neither half can be "fixed" by sacrificing the other: +// an absence was READ off the Header branch and is kept; a failure was never read at all. +test("an absence is remembered, a failure is not — the two negatives are not the same", async () => { + const { sparql_query, unreachable } = inject(); + armEmulation(); + setCurrentUser("bob"); + + const absent = "did:ng:o:exposes-nothing" as Nuri; + expect(await fetchReadCap(absent)).toBe(false); + expect(await fetchReadCap(absent)).toBe(false); + expect(asksAbout(sparql_query, absent)).toBe(1); // read once, and the answer stands + + const unreached = "did:ng:o:cannot-ask" as Nuri; + unreachable.add(unreached); + expect(await fetchReadCap(unreached)).toBe(false); + expect(await fetchReadCap(unreached)).toBe(false); + expect(asksAbout(sparql_query, unreached)).toBe(2); // never answered, so asked every time +}); + +// The memo's OTHER job. Losing it would turn one cold read by N holders into N round-trips. +test("concurrent askers share ONE failing round-trip, and exactly one re-ask follows", async () => { + const { sparql_query, unreachable } = inject(); + armEmulation(); + setCurrentUser("bob"); + + unreachable.add(PUB); + const answers = await Promise.all([fetchReadCap(PUB), fetchReadCap(PUB), fetchReadCap(PUB)]); + expect(answers).toEqual([false, false, false]); + expect(asksAbout(sparql_query, PUB)).toBe(1); // one in-flight fetch served all three + + expect(await fetchReadCap(PUB)).toBe(false); + expect(asksAbout(sparql_query, PUB)).toBe(2); // and the failure left nothing behind +}); + +// `fetchReadCap` files for whoever is connected NOW, on EVERY call — the memo spares the +// round-trip, not the filing. A re-attempt after a failure must obey that too, or the +// recovered cap would land in the ring of whoever happened to hit the outage. +test("a cap obtained after a failure is filed for whoever is connected THEN", async () => { + const { unreachable } = inject(); + setCurrentUser("alice"); + await aliceExposesHerNote(); + armEmulation(); + + setCurrentUser("bob"); + unreachable.add(PUB); + expect(await fetchReadCap(PUB)).toBe(false); + + unreachable.delete(PUB); + setCurrentUser("carol"); + expect(await fetchReadCap(PUB)).toBe(true); + expect(getCaps().capFor(PUB)).toBe(mintCap(PUB)); // carol asked, carol holds it + + setCurrentUser("bob"); + expect(getCaps().capFor(PUB)).toBeUndefined(); // bob only ever met the outage +});