From 6afddce6ce37bd010d18c21b91643ab5d5cc1207 Mon Sep 17 00:00:00 2001 From: callumalpass Date: Fri, 13 Nov 2026 13:08:51 +1100 Subject: [PATCH] Comments: recheck the account after a startup failure; never sign from a stale one On SDK beta.188 a linked account showed "Your comments are not signed ... could not be checked" on every fresh load, and comments written before Check again were saved unsigned. The SDK host makes the /identity READ under its collection control's lifetime (there is no per-request cancel), so an aborted fetch means the connection that sent it was closed mid-READ; the client then reports `unauthenticated`/`people_fenced`. The same one READ slot is shared with the SDK's launch people readiness, which reports `unavailable`/`people_busy`. Writer retried only `unavailable`, once after 250 ms, then cached the failed result: the panel kept the notice and createComment signed from that cached failure, so the comment went out unsigned. Writer now rechecks transient failures (unavailable, unauthenticated, cancelled, rate_limited) after 250 ms, 1 s and 3 s, and never reuses a failed check, so the next panel reload or new comment checks again. Refusals and unknown failures are still the answer. Refs #91 --- apps/writer/src/backend/native.test.ts | 40 +++++++++++++++++----- apps/writer/src/backend/native.ts | 47 +++++++++++++++++--------- changelog.d/signing-check-retry.md | 3 ++ docs/architecture.md | 12 +++++-- 4 files changed, 74 insertions(+), 28 deletions(-) create mode 100644 changelog.d/signing-check-retry.md diff --git a/apps/writer/src/backend/native.test.ts b/apps/writer/src/backend/native.test.ts index 58d14ac..98b6303 100644 --- a/apps/writer/src/backend/native.test.ts +++ b/apps/writer/src/backend/native.test.ts @@ -45,6 +45,8 @@ async function fixture(beforeBackend?: (client: MdbaseClient) => void) { return {replica, client, description, metadata, current, caller, backend, manuscript}; } +const linked = {account: {id: "fixture-account", displayName: "Fixture", identities: []}, me: {status: "linked", person: {id: "p", path: "people/me.md", name: "Me", identities: [], typeNames: ["person"]}}, people: [], invalid: []} as never; + // Public watch lifecycle stand-ins only; no private SDK fault hooks. function controlledWatch(initialState: ChangesWatch["state"] = "active", ready: Promise = Promise.resolve()) { let state = initialState; @@ -380,17 +382,37 @@ describe("native Writer domain backend", () => { h.replica.confirmAll(); h.replica.stallConfirmations(false); expect((await reply).ok).toBe(true); }); - it("checks the account once more after a transient directory failure (issue 91)", async () => { + it("checks the account again after its first identity READ is cut off (issue 91)", async () => { const h = await fixture(); - const account = {id: "fixture-account", displayName: "Fixture", identities: []}; - const directory = vi.spyOn(MdbasePeopleClient.prototype, "directory") - .mockRejectedValueOnce(new MdbaseError({code: "unavailable", recovery: "retry", message: "people read session changed", reason: "people_fenced"})) - .mockResolvedValueOnce({account, me: {status: "linked", person: {id: "p", path: "people/me.md", name: "Me", identities: [], typeNames: ["person"]}}, people: [], invalid: []} as never); + // What the SDK reports when the host's collection control closes mid-READ + // (its /identity fetch is aborted), and when the one READ slot is busy. + const fenced = new MdbaseError({code: "unauthenticated", recovery: "reauthorize", message: "people scope changed", reason: "people_fenced"}); + const busy = new MdbaseError({code: "unavailable", recovery: "retry", message: "people metadata request failed", reason: "people_busy"}); + const directory = vi.spyOn(MdbasePeopleClient.prototype, "directory").mockRejectedValueOnce(fenced).mockRejectedValueOnce(busy).mockResolvedValueOnce(linked); expect(await h.backend.people()).toMatchObject({signing: {kind: "linked"}, me: {name: "Me"}}); - expect(directory).toHaveBeenCalledTimes(2); - directory.mockRejectedValue(new MdbaseError({code: "unavailable", recovery: "retry", message: "people read session changed", reason: "people_fenced"})); - expect(await h.backend.people({fresh: true})).toMatchObject({signing: {kind: "unavailable"}}); - expect(directory).toHaveBeenCalledTimes(4); + expect(directory).toHaveBeenCalledTimes(3); + }); + it("takes a refusal as the answer without retrying", async () => { + const h = await fixture(); + const directory = vi.spyOn(MdbasePeopleClient.prototype, "directory").mockRejectedValue(new MdbaseError({code: "forbidden", recovery: "reauthorize", message: "no identity", reason: "people_access"})); + expect((await h.backend.people()).signing?.kind).not.toBe("linked"); + expect(directory).toHaveBeenCalledOnce(); + }); + it("never signs a new comment from a failed account check (issue 91)", async () => { + const h = await fixture(); + vi.useFakeTimers({toFake: ["setTimeout"]}); + try { + const directory = vi.spyOn(MdbasePeopleClient.prototype, "directory").mockRejectedValue(new MdbaseError({code: "unavailable", recovery: "retry", message: "people metadata request failed", reason: "people_busy"})); + const checked = h.backend.people(); + await vi.advanceTimersByTimeAsync(5_000); + expect(await checked).toMatchObject({signing: {kind: "unavailable"}}); + expect(directory).toHaveBeenCalledTimes(4); + directory.mockResolvedValue(linked); + } finally {vi.useRealTimers();} + const created = await h.backend.createComment({document: "manuscripts/original.md", text: "Signed"}); + if (!created.ok) throw new Error(created.message); + expect(created.value.createdBy).toBe("[[people/me]]"); + expect(await h.backend.people()).toMatchObject({signing: {kind: "linked"}}); }); it("still refuses a held or conflicted comment body on a display read", async () => { const h = await fixture(); diff --git a/apps/writer/src/backend/native.ts b/apps/writer/src/backend/native.ts index 8697321..d1a3f30 100644 --- a/apps/writer/src/backend/native.ts +++ b/apps/writer/src/backend/native.ts @@ -24,7 +24,9 @@ interface NativeMetadata { readonly files: ReadonlyMap; readonly generation: number; } -const RECONNECT_MIN_MS = 500, RECONNECT_MAX_MS = 30_000, PEOPLE_RETRY_MS = 250; +const RECONNECT_MIN_MS = 500, RECONNECT_MAX_MS = 30_000; +/** Pauses before each further account check after a transient failure. */ +const PEOPLE_RETRY_MS = [250, 1_000, 3_000]; function unprotected(record: RecordView): boolean { return record.state.state === "confirmed" && readable(record); } @@ -60,6 +62,7 @@ export class NativeWriterBackend implements WriterBackend { private cached: NativeMetadata | undefined; private metadataJob: Promise | undefined; private peopleJob: Promise | undefined; + private peopleFailed = false; private writing = false; /** This session's write whose reply was lost, so a repeated click can't duplicate it. * Writes from earlier sessions are the SDK write ledger's to keep and settle. */ @@ -334,25 +337,37 @@ export class NativeWriterBackend implements WriterBackend { }); } people(options: {fresh?: boolean} = {}): Promise { - if (!this.peopleJob || options.fresh) this.peopleJob = this.loadPeople(); + // A failed check is never reused: the next caller (the panel's reload, or a new + // comment about to be signed) checks again rather than writing it unsigned. + if (!this.peopleJob || options.fresh || this.peopleFailed) { + this.peopleFailed = false; + const job = this.loadPeople(); this.peopleJob = job; + void job.then(people => {if (this.peopleJob === job && people.signing?.kind === "unavailable") this.peopleFailed = true;}); + } return this.peopleJob; } private async loadPeople(): Promise { - try { - this.check(); - const directory = await this.connection.people.directory({members: "omit", signal: this.signal}).catch(async (error: unknown) => { - // A read session replaced mid-check (a reconnect) or a briefly incomplete - // inventory is transient: look once more before saying the account is unchecked. - if (!isMdbaseError(error, "unavailable")) throw error; - await new Promise(resolve => setTimeout(resolve, PEOPLE_RETRY_MS)); this.check(); - return this.connection.people.directory({members: "omit", signal: this.signal}); - }); this.check(); - return peopleFromDirectory(directory); - } catch (error) { - // Names only, never a substitute account/author or guessed identity. - try {const names = (await this.metadata()).store.people; this.check(); return {names, signing: {kind: "unavailable", reason: "Writer could not check the account's person record."}};} - catch {return {...NO_PEOPLE, signing: isMdbaseError(error) && error.code === "forbidden" ? {kind: "not-approved"} : {kind: "unavailable", reason: "Writer could not check the account's person record."}};} + let failure: unknown; + for (const delay of [0, ...PEOPLE_RETRY_MS]) { + try { + if (delay) await new Promise(resolve => setTimeout(resolve, delay)); + this.check(); + const directory = await this.connection.people.directory({members: "omit", signal: this.signal}); this.check(); + return peopleFromDirectory(directory); + } catch (error) { + failure = error; + // These are transient at startup: the SDK host's identity READ ends with its + // collection control (aborted when that connection is replaced: `people_fenced`), + // its one READ slot can be busy with launch readiness (`people_busy`), or the read + // session can change. Look again, briefly, before saying the account is unchecked. + // A refusal or an unknown failure is the answer. + const transient = isMdbaseError(error) && ["unavailable", "unauthenticated", "cancelled", "rate_limited"].includes(error.code); + if (!transient || this.signal.aborted || this.disposed) break; + } } + // Names only, never a substitute account/author or guessed identity. + try {const names = (await this.metadata()).store.people; this.check(); return {names, signing: {kind: "unavailable", reason: "Writer could not check the account's person record."}};} + catch {return {...NO_PEOPLE, signing: isMdbaseError(failure) && failure.code === "forbidden" ? {kind: "not-approved"} : {kind: "unavailable", reason: "Writer could not check the account's person record."}};} } reviewIdentityAccess(): Promise> { return this.safely(async () => {if (!this.options.authorize) throw new Error("Writer cannot request identity access here."); await this.options.authorize(); this.check(); this.peopleJob = undefined;}); diff --git a/changelog.d/signing-check-retry.md b/changelog.d/signing-check-retry.md new file mode 100644 index 0000000..3d4cde0 --- /dev/null +++ b/changelog.d/signing-check-retry.md @@ -0,0 +1,3 @@ +## Fixed + +- A linked account is no longer shown as "Your comments are not signed … could not be checked" when Writer opens, and comments written before pressing **Check again** are no longer saved unsigned. Writer checks the account again after a brief startup failure, and it never reuses a failed check. diff --git a/docs/architecture.md b/docs/architecture.md index 222299f..b9090f3 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -44,9 +44,15 @@ reports each write before it confirms, and the panel reloads at once); only a hold or an unresolved conflict refuses them. Writes and their readbacks still require a confirmed record. Account checks are applied in the order they were - started, so a slow, older check never replaces a newer signing state, and a - transient `unavailable` directory failure is checked once more before the - notice says the account could not be checked. + started, so a slow, older check never replaces a newer signing state. A + transient people-directory failure (`unavailable`, `unauthenticated`/`people_fenced`, + `cancelled`, `rate_limited`) is checked again after 250 ms, 1 s and 3 s before + the notice says the account could not be checked, and a failed check is never + reused: the next panel reload or new comment checks again, so a linked account + never writes an unsigned comment from a stale failure. The SDK host's identity + READ runs under its collection control's lifetime and shares one READ slot with + launch readiness, so a replaced connection or a busy slot fails Writer's first + check at startup. - **Images** are collection files, referenced relative to the record (`![Caption](figures/plot.png){#fig-plot}`) or by wikilink (`![[plot.png]]`).