From b7a0d1e18a27b0b715ea54f1eabc72daaa3284fc Mon Sep 17 00:00:00 2001 From: Daniel Rouleau Date: Wed, 28 Oct 2026 15:30:01 -0700 Subject: [PATCH 1/2] Invalidate community leave across windows Co-authored-by: Codex Signed-off-by: Daniel Rouleau --- docs/communities.md | 7 + docs/local-artifact-storage.md | 2 +- src/features/artifact-storage/service.test.ts | 18 +- src/features/artifact-storage/service.ts | 22 +- src/features/communities/service.test.ts | 36 ++- src/features/communities/service.ts | 243 +++++++++++------- tests/browser/community-artifacts.spec.mjs | 145 +++++++++++ tests/fixtures/community-artifacts.html | 1 + tests/fixtures/community-artifacts.ts | 54 ++++ 9 files changed, 425 insertions(+), 103 deletions(-) create mode 100644 tests/browser/community-artifacts.spec.mjs create mode 100644 tests/fixtures/community-artifacts.html create mode 100644 tests/fixtures/community-artifacts.ts diff --git a/docs/communities.md b/docs/communities.md index 6bdec43da8..00b15eaf62 100644 --- a/docs/communities.md +++ b/docs/communities.md @@ -115,6 +115,13 @@ empty state there. Try drafting in A, switching to B, then returning to A. Selected channels, drafts and reading offsets are partitioned by the canonical community origin and viewer; channel IDs alone are not sufficient keys. +Windows on the same app origin share the saved memberships and selection. +Each snapshot and command reads a changed device record before it uses membership. +Storage events also notify readers and dispose sessions for removed memberships. +A new local join has a generation value. Artifact handles capture this value, so +leave and rejoin cannot restore an old handle. Records without a generation remain +readable. Explicit rejoin creates a new handle and session. + ## Import from the previous desktop app After native startup, Buzz offers the previous desktop app's saved communities diff --git a/docs/local-artifact-storage.md b/docs/local-artifact-storage.md index 13bec1824c..54194fe220 100644 --- a/docs/local-artifact-storage.md +++ b/docs/local-artifact-storage.md @@ -132,5 +132,5 @@ and indexed pages use all four stores. Metadata operations use only metadata. Large row batches can still delay other collections' reads or writes. A changed row rewrites its indexes. Index upgrades scan the collection. Delta completion does not scan records. Ancestry comparison and separate source-membership writes are absent. The database upgrade removes the obsolete membership store, builds the host -channel index, and preserves records and device preferences. Leaving a community purges only its saved partition. If database +channel index, and preserves records and device preferences. Leaving a community purges only its saved partition. The community service propagates membership and selection changes to other windows. Handles capture the local membership generation and close when it changes. Opens read the current device record, so a window cannot reopen a left partition before its storage event arrives. Explicit rejoin permits new handles; old handles stay closed. If database opening fails, close that handle and open another after the cause clears. diff --git a/src/features/artifact-storage/service.test.ts b/src/features/artifact-storage/service.test.ts index d728b3c561..559b9789f9 100644 --- a/src/features/artifact-storage/service.test.ts +++ b/src/features/artifact-storage/service.test.ts @@ -4,7 +4,11 @@ import { ArtifactStorageService } from "./service"; it("binds collection ownership to the calling plugin and fences scope changes", async () => { const root = new Context(); - let scope = { viewer: "viewer", selected: "https://relay.test" }; + let scope = { + viewer: "viewer", + selected: "https://relay.test", + memberships: [{ id: "https://relay.test" }], + }; const subscribers = new Set<() => void>(); root.provide("communityReader", { snapshot: () => scope, @@ -66,10 +70,20 @@ it("binds collection ownership to the calling plugin and fences scope changes", expect(refreshError).toHaveBeenCalledOnce(); await handle.get("task"); expect(get).toHaveBeenCalledWith("task"); - scope = { viewer: "other", selected: "https://relay.test" }; + let finishRead!: () => void; + get.mockImplementationOnce( + () => + new Promise((resolve) => { + finishRead = () => resolve(undefined); + }), + ); + const pending = handle.get("task"); + scope = { ...scope, viewer: "other" }; for (const listener of subscribers) listener(); expect(listener.mock.calls[0]?.[0].signal.aborted).toBe(true); expect(close).toHaveBeenCalled(); expect(() => handle.get("task")).toThrow("scope changed"); + finishRead(); + await expect(pending).rejects.toThrow("scope changed"); await root.fiber.dispose(); }); diff --git a/src/features/artifact-storage/service.ts b/src/features/artifact-storage/service.ts index 060e6ad06e..b554991baf 100644 --- a/src/features/artifact-storage/service.ts +++ b/src/features/artifact-storage/service.ts @@ -47,13 +47,19 @@ export class ArtifactStorageService extends Service implements ArtifactStorage { open(definition: ArtifactCollectionDefinition): ArtifactCollection { const owner = this.ctx.pluginOwner?.id; const capture = () => { - const { viewer, selected } = this.ctx.communityReader.snapshot(); + const { viewer, selected, memberships } = + this.ctx.communityReader.snapshot(); + const membership = memberships.find((entry) => entry.id === selected); return { viewer, - origin: selected ? communityDestination(selected).url : undefined, + generation: membership?.generation, + origin: + selected && membership + ? communityDestination(selected).url + : undefined, }; }; - const { viewer, origin } = capture(); + const { viewer, origin, generation } = capture(); if (!owner || !viewer || !origin) throw new Error("Artifact storage needs a plugin, identity and relay"); const scope = JSON.stringify([viewer, origin]); @@ -78,13 +84,19 @@ export class ArtifactStorageService extends Service implements ArtifactStorage { if ( controller.signal.aborted || now.viewer !== viewer || - now.origin !== origin + now.origin !== origin || + now.generation !== generation ) throw new Error("Artifact storage scope changed"); }; unsubscribe = this.ctx.communityReader.subscribe(() => { const now = capture(); - if (now.viewer !== viewer || now.origin !== origin) close(); + if ( + now.viewer !== viewer || + now.origin !== origin || + now.generation !== generation + ) + close(); }); const checked = async (operation: Promise) => { const result = await operation; diff --git a/src/features/communities/service.test.ts b/src/features/communities/service.test.ts index b17b81a144..244fa3bac1 100644 --- a/src/features/communities/service.test.ts +++ b/src/features/communities/service.test.ts @@ -1,7 +1,7 @@ import { Context } from "@deepseek-ai/cordis"; import { assert, afterEach, expect, it, vi } from "vitest"; import { createCommunities } from "./service"; -import { communityDestination } from "./destination"; +import { communityDestination, relayAddress } from "./destination"; import { localCommunityNameKey } from "./names"; import * as destinations from "./destination"; import { flush } from "../relay/testing"; @@ -420,7 +420,7 @@ it("retains unresolved memberships but honors an explicit Personal selection or localStorage.getItem(`buzz-client.v1:${viewer}`) ?? "null", ); expect(persisted.memberships).toEqual([ - { id: "primary", name: "New community" }, + expect.objectContaining({ id: "primary", name: "New community" }), membership, ]); expect(persisted.selected).toBe("primary"); @@ -1028,3 +1028,35 @@ it("hydrates native public identity without contacting the development broker", expect(fetch).not.toHaveBeenCalled(); expect(client.relay.snapshot().status).not.toBe("ready"); }); + +it("keeps a leave from another window when a local command saves before its storage event", async () => { + const client = setup({ + profile: { name: "Local", picture: "" }, + memberships: [{ id: "primary", name: "Primary" }], + selected: "primary", + }); + await flush(); + const record = `buzz-client.v1:${viewer}`; + localStorage.setItem( + record, + JSON.stringify({ + profile: { name: "Local", picture: "" }, + memberships: [], + selected: null, + sync: { + known: [], + outbox: [{ url: relayAddress("primary"), removed: true }], + }, + }), + ); + client.saveProfile({ name: "Updated", picture: "" }); + expect(JSON.parse(localStorage.getItem(record) ?? "null")).toMatchObject({ + profile: { name: "Updated" }, + memberships: [], + selected: null, + sync: { outbox: [{ url: relayAddress("primary"), removed: true }] }, + }); + expect(client.open("primary")).toBeUndefined(); + expect(client.relay.snapshot().status).toBe("disconnected"); + expect(() => client.select("primary")).toThrow("Join this community first"); +}); diff --git a/src/features/communities/service.ts b/src/features/communities/service.ts index 23058cc012..92a94df9c1 100644 --- a/src/features/communities/service.ts +++ b/src/features/communities/service.ts @@ -30,6 +30,8 @@ export const PROFILE_ABOUT_MAX_LENGTH = 500; export type PersonalProfile = { name: string; picture: string; about?: string }; export type Membership = { id: string; + /** One local join lifetime. Older records have no generation. */ + generation?: string; /** Last-known default from joining; never inferred to be a personal override. */ name: string; icon?: string; @@ -124,6 +126,76 @@ export function createCommunities( scopes.push(scope); return scope; } + let savedRecord: string | null | undefined; + function readSaved(stored: string | null): Saved { + let saved = empty(); + const raw = JSON.parse(stored ?? "null"); + unresolvedMemberships.length = 0; + unresolvedSelection = null; + if (raw) + saved = { + profile: { + name: typeof raw.profile?.name === "string" ? raw.profile.name : "", + picture: + typeof raw.profile?.picture === "string" ? raw.profile.picture : "", + about: + typeof raw.profile?.about === "string" ? raw.profile.about : "", + }, + memberships: Array.isArray(raw.memberships) + ? raw.memberships + .flatMap((m: unknown): Membership[] => { + if ( + !m || + typeof m !== "object" || + !("id" in m) || + typeof m.id !== "string" || + !("name" in m) || + typeof m.name !== "string" + ) + return []; + const membership = { + id: m.id, + name: m.name, + ...("generation" in m && typeof m.generation === "string" + ? { generation: m.generation } + : {}), + ...("icon" in m && + typeof m.icon === "string" && + m.icon.startsWith("https://") + ? { icon: m.icon } + : {}), + }; + try { + return [{ ...membership, id: communityDestination(m.id).id }]; + } catch { + if ( + isCommunityAlias(m.id) && + !unresolvedMemberships.some((entry) => entry.id === m.id) + ) + unresolvedMemberships.push(membership); + return []; + } + }) + .filter( + (m: Membership, index: number, all: Membership[]) => + all.findIndex((entry) => entry.id === m.id) === index, + ) + : [], + selected: null, + sync: parseSync(raw.sync), + }; + if (typeof raw?.selected === "string") { + try { + saved.selected = communityDestination(raw.selected).id; + } catch { + if (unresolvedMemberships.some((m) => m.id === raw.selected)) + unresolvedSelection = raw.selected; + } + } + if (!saved.memberships.some((m) => m.id === saved.selected)) + saved.selected = null; + return saved; + } const current = () => state.selected ? (sessions.get(state.selected) ?? disconnected) @@ -141,22 +213,23 @@ export function createCommunities( const selection = persist && Object.hasOwn(patch, "selected") ? null : unresolvedSelection; try { - if (persist && next.viewer) - localStorage.setItem( - `buzz-client.v1:${next.viewer}`, - JSON.stringify({ - profile: next.profile, - memberships: [...next.memberships, ...unresolvedMemberships].map( - ({ id, name, icon }) => ({ - id, - name, - ...(icon ? { icon } : {}), - }), - ), - selected: next.selected ?? selection, - sync: next.sync, - }), - ); + if (persist && next.viewer) { + const record = JSON.stringify({ + profile: next.profile, + memberships: [...next.memberships, ...unresolvedMemberships].map( + ({ id, name, icon, generation }) => ({ + id, + name, + ...(generation ? { generation } : {}), + ...(icon ? { icon } : {}), + }), + ), + selected: next.selected ?? selection, + sync: next.sync, + }); + localStorage.setItem(`buzz-client.v1:${next.viewer}`, record); + savedRecord = record; + } } catch (error) { // The storage error rides along as the cause so a caller can name it: a // store that never saves should not read as the same retry every time. @@ -252,87 +325,16 @@ export function createCommunities( let saved = empty(); let seeded = false; try { - const stored = localStorage.getItem(`buzz-client.v1:${viewer}`); - const raw = JSON.parse(stored ?? "null"); - if (raw) - saved = { - profile: { - name: - typeof raw.profile?.name === "string" ? raw.profile.name : "", - picture: - typeof raw.profile?.picture === "string" - ? raw.profile.picture - : "", - about: - typeof raw.profile?.about === "string" - ? raw.profile.about - : "", - }, - memberships: Array.isArray(raw.memberships) - ? raw.memberships - .flatMap((m: unknown): Membership[] => { - if ( - !m || - typeof m !== "object" || - !("id" in m) || - typeof m.id !== "string" || - !("name" in m) || - typeof m.name !== "string" - ) - return []; - const membership = { - id: m.id, - name: m.name, - ...("icon" in m && - typeof m.icon === "string" && - m.icon.startsWith("https://") - ? { icon: m.icon } - : {}), - }; - try { - return [ - { ...membership, id: communityDestination(m.id).id }, - ]; - } catch { - if ( - isCommunityAlias(m.id) && - !unresolvedMemberships.some( - (entry) => entry.id === m.id, - ) - ) - unresolvedMemberships.push(membership); - return []; - } - }) - .filter( - (m: Membership, index: number, all: Membership[]) => - all.findIndex((entry) => entry.id === m.id) === index, - ) - : [], - selected: null, - sync: parseSync(raw.sync), - }; - else if (openRelay && stored === null) { - // Development opt-in for a viewer with no saved record on this origin. - // Any stored record, including Personal space or one this reader - // cannot understand, wins over the seed. + savedRecord = localStorage.getItem(`buzz-client.v1:${viewer}`); + saved = readSaved(savedRecord); + if (openRelay && savedRecord === null) { const { id, name } = communityDestination(openRelay); saved = { ...saved, memberships: [{ id, name }], selected: id }; seeded = true; } - if (typeof raw?.selected === "string") { - try { - saved.selected = communityDestination(raw.selected).id; - } catch { - if (unresolvedMemberships.some((m) => m.id === raw.selected)) - unresolvedSelection = raw.selected; - } - } } catch { /* Invalid local preferences do not prevent opening the client. */ } - if (!saved.memberships.some((m) => m.id === saved.selected)) - saved.selected = null; presenceActivity.setViewer(viewer); if (saved.selected) acquire(saved.selected, viewer); saved.memberships = saved.memberships.map((membership) => ({ @@ -412,11 +414,49 @@ export function createCommunities( }), ); } + // A command can run before another window's storage event arrives. + function syncSaved() { + const viewer = state.viewer; + if (disposed || state.status !== "ready" || !viewer) return; + let saved: Saved; + try { + const stored = localStorage.getItem(`buzz-client.v1:${viewer}`); + if (stored === savedRecord) return; + saved = readSaved(stored); + savedRecord = stored; + } catch { + return; + } + for (const { id, generation } of state.memberships) { + if ( + saved.memberships.some( + (entry) => entry.id === id && entry.generation === generation, + ) + ) + continue; + presentations.delete(id); + membershipVersions.set(id, (membershipVersions.get(id) ?? 0) + 1); + void release(id); + } + saved.memberships = saved.memberships.map((entry) => ({ + ...state.memberships.find( + (current) => + current.id === entry.id && current.generation === entry.generation, + ), + ...entry, + localName: readLocalCommunityName(entry.id, viewer), + })); + if (saved.selected) acquire(saved.selected); + update(saved, false); + refreshPresentation(); + } const syncLocalNames = (event: StorageEvent) => { const viewer = state.viewer; if (!viewer) return; try { if (event.storageArea !== localStorage) return; + if (event.key === null || event.key === `buzz-client.v1:${viewer}`) + syncSaved(); let changed = false; const memberships = state.memberships.map((membership) => { if ( @@ -448,7 +488,10 @@ export function createCommunities( return { presence: presenceActivity, relay, - snapshot: () => state, + snapshot() { + syncSaved(); + return state; + }, subscribe(fn: () => void) { listeners.add(fn); return () => { @@ -462,6 +505,7 @@ export function createCommunities( }; }, setLocalName(id: string, viewer: string, value: string) { + syncSaved(); id = communityDestination(id).id; const membership = state.memberships.find((entry) => entry.id === id); if ( @@ -485,6 +529,7 @@ export function createCommunities( ); }, select(id: string | null) { + syncSaved(); if (id) id = communityDestination(id).id; if (id && !state.memberships.some((m) => m.id === id)) throw new Error("Join this community first"); @@ -494,18 +539,25 @@ export function createCommunities( /** A joined community's session, connected without selecting it, for * background work such as agents; it lives until the community is left. */ open(id: string): RelayData | undefined { + syncSaved(); id = communityDestination(id).id; return state.memberships.some((m) => m.id === id) ? acquire(id) : undefined; }, saveProfile(profile: PersonalProfile) { + syncSaved(); update({ profile }); }, joined(membership: Membership, profile: PersonalProfile) { + syncSaved(); + const id = communityDestination(membership.id).id; membership = { ...membership, - id: communityDestination(membership.id).id, + generation: + state.memberships.find((entry) => entry.id === id)?.generation ?? + crypto.randomUUID(), + id, ...(state.viewer ? { localName: readLocalCommunityName(membership.id, state.viewer) } : {}), @@ -552,6 +604,7 @@ export function createCommunities( id: string, { purge = true }: { purge?: boolean } = {}, ): Promise { + syncSaved(); id = communityDestination(id).id; if (!state.memberships.some((m) => m.id === id)) return []; const { viewer } = state; @@ -576,7 +629,10 @@ export function createCommunities( failures.push(...(await purgeCommunityDeviceState(origin, viewer))); return failures; }, - pendingSync: () => state.sync.outbox, + pendingSync() { + syncSaved(); + return state.sync.outbox; + }, /** Writes the sync owner's settled state and, in the same device record, * the memberships a server list adds: destinations saved on another * device appear here under their host name, or the viewer's local name @@ -584,6 +640,7 @@ export function createCommunities( * Persistence follows `joined`, and that save is the only step that * throws, before anything has changed. */ async applySync(next: SyncState, add: string[] = []) { + syncSaved(); const memberships = [...state.memberships]; for (const url of add) { const { id, name } = communityDestination(url); diff --git a/tests/browser/community-artifacts.spec.mjs b/tests/browser/community-artifacts.spec.mjs new file mode 100644 index 0000000000..87c9937a04 --- /dev/null +++ b/tests/browser/community-artifacts.spec.mjs @@ -0,0 +1,145 @@ +import { test, expect } from "./source-fixture.mjs"; + +// Real storage events and IndexedDB ordering need separate browser windows. +for (const { delayedEvent, purge } of [ + { delayedEvent: false, purge: true }, + { delayedEvent: true, purge: true }, + { delayedEvent: false, purge: false }, +]) { + test(`leave fences artifact storage across windows with ${delayedEvent ? "delayed" : "normal"} storage events and purge=${purge}`, async ({ + page, + context, + }) => { + await page.goto("/tests/fixtures/community-artifacts.html"); + await page.waitForFunction( + () => window.communityArtifacts?.snapshot().status === "ready", + ); + await page.evaluate(() => window.communityArtifacts.join()); + const other = await context.newPage(); + await other.addInitScript(() => { + window.addEventListener("storage", (event) => { + if (window.pauseStorage) event.stopImmediatePropagation(); + }); + }); + await other.goto(page.url()); + await other.waitForFunction( + () => + window.communityArtifacts?.snapshot().selected === + "https://fixture.example", + ); + await other.evaluate((delay) => { + window.handle = window.communityArtifacts.open(); + window.pauseStorage = delay; + return window.handle.importBatch({ + records: [{ id: "task", local: { title: "saved" } }], + }); + }, delayedEvent); + await page.evaluate( + (purge) => window.communityArtifacts.leave(purge), + purge, + ); + if (!delayedEvent) + await other.waitForFunction(() => window.communityArtifacts.aborted()); + const result = await other.evaluate(async () => { + let openError; + try { + window.communityArtifacts.open(); + } catch (error) { + openError = error.message; + } + let writeError; + try { + await window.handle.importBatch({ + records: [{ id: "task", local: { title: "stale" } }], + }); + } catch (error) { + writeError = error.message; + } + return { + snapshot: window.communityArtifacts.snapshot(), + aborted: window.communityArtifacts.aborted(), + openError, + writeError, + saved: await window.communityArtifacts.saved(), + }; + }); + expect(result.snapshot.memberships).toEqual([]); + expect(result.snapshot.selected).toBeNull(); + expect(result.aborted).toBe(true); + expect(result.openError).toBe( + "Artifact storage needs a plugin, identity and relay", + ); + expect(result.writeError).toBe("Artifact storage scope changed"); + if (purge) expect(result.saved).toBeUndefined(); + else expect(result.saved.record.local).toEqual({ title: "saved" }); + await other.evaluate(async () => { + window.communityArtifacts.join(); + const fresh = window.communityArtifacts.open(); + await fresh.importBatch({ + records: [{ id: "rejoined", local: { title: "rejoined" } }], + }); + }); + await page.waitForFunction( + () => + window.communityArtifacts.snapshot().selected === + "https://fixture.example", + ); + expect( + await page.evaluate( + async () => + (await window.communityArtifacts.open().get("rejoined"))?.record + .local, + ), + ).toEqual({ title: "rejoined" }); + await expect( + other.evaluate(() => window.handle.get("task")), + ).rejects.toThrow("scope changed"); + }); +} + +test("rejects an old handle after leave and rejoin before event delivery", async ({ + page, + context, +}) => { + await page.goto("/tests/fixtures/community-artifacts.html"); + await page.waitForFunction( + () => window.communityArtifacts?.snapshot().status === "ready", + ); + await page.evaluate(() => window.communityArtifacts.join()); + const other = await context.newPage(); + await other.addInitScript(() => { + window.addEventListener("storage", (event) => { + if (window.pauseStorage) event.stopImmediatePropagation(); + }); + }); + await other.goto(page.url()); + await other.waitForFunction( + () => window.communityArtifacts?.snapshot().selected, + ); + await other.evaluate(async () => { + window.handle = window.communityArtifacts.open(); + await window.handle.get("task"); + window.pauseStorage = true; + }); + await page.evaluate(async () => { + await window.communityArtifacts.leave(true); + window.communityArtifacts.join(); + await window.communityArtifacts + .open() + .importBatch({ records: [{ id: "task", local: { title: "fresh" } }] }); + }); + await expect( + other.evaluate(() => + window.handle.importBatch({ records: [{ id: "stale", local: {} }] }), + ), + ).rejects.toThrow("scope changed"); + expect(await other.evaluate(() => window.communityArtifacts.aborted())).toBe( + true, + ); + expect( + await other.evaluate( + async () => + (await window.communityArtifacts.open().get("task"))?.record.local, + ), + ).toEqual({ title: "fresh" }); +}); diff --git a/tests/fixtures/community-artifacts.html b/tests/fixtures/community-artifacts.html new file mode 100644 index 0000000000..6ee9896f4c --- /dev/null +++ b/tests/fixtures/community-artifacts.html @@ -0,0 +1 @@ +Community artifact checks

Community artifact checks

diff --git a/tests/fixtures/community-artifacts.ts b/tests/fixtures/community-artifacts.ts new file mode 100644 index 0000000000..aa37c85046 --- /dev/null +++ b/tests/fixtures/community-artifacts.ts @@ -0,0 +1,54 @@ +import { Context } from "@deepseek-ai/cordis"; +import { createCommunities } from "../../src/features/communities/service"; +import { ArtifactStorageService } from "../../src/features/artifact-storage/service"; +import { createArtifactStore } from "../../src/features/artifact-storage/store"; +import type { ArtifactCollectionDefinition } from "../../src/features/artifact-storage/types"; + +const viewer = "a".repeat(64); +const membership = { id: "https://fixture.example", name: "Fixture" }; +const profile = { name: "Fixture", picture: "" }; +const root = new Context(); +const communities = createCommunities( + root, + false, + undefined, + "", + undefined, + Promise.resolve(viewer), +); +root.provide("communityReader", communities); +const storage = new ArtifactStorageService(root); +const plugin = root.extend({ + pluginOwner: { id: "test.plugin", revision: "one" }, +}); +const definition: ArtifactCollectionDefinition = { + name: "tasks", + type: "test.task", + version: 1, + project: () => [], +}; +let signal: AbortSignal | undefined; +storage.onOpen((entry) => { + signal = entry.signal; +}); +const api = { + snapshot: communities.snapshot, + join: () => communities.joined(membership, profile), + leave: (purge: boolean) => communities.leave(membership.id, { purge }), + open: () => plugin.artifactStorage.open(definition), + aborted: () => signal?.aborted, + async saved() { + const store = createArtifactStore(viewer, membership.id); + try { + return await store.collection("test.plugin", definition).get("task"); + } finally { + store.close(); + } + }, +}; +declare global { + interface Window { + communityArtifacts: typeof api; + } +} +window.communityArtifacts = api; From e3e9917514e86713e4207608bd7908c32255092f Mon Sep 17 00:00:00 2001 From: Daniel Rouleau Date: Wed, 28 Oct 2026 15:48:41 -0700 Subject: [PATCH 2/2] Serialize purge and fence queued artifact work Co-authored-by: Codex Signed-off-by: Daniel Rouleau --- docs/communities.md | 8 +- docs/local-artifact-storage.md | 2 +- .../builderlab/known-communities/sync.test.ts | 66 ++++++ src/features/artifact-storage/service.test.ts | 6 +- src/features/artifact-storage/service.ts | 10 +- src/features/artifact-storage/store.ts | 105 ++++++---- .../communities/legacy-communities.test.ts | 5 +- src/features/communities/service.test.ts | 6 +- src/features/communities/service.ts | 84 ++++---- tests/browser/community-artifacts.spec.mjs | 193 +++++++++++++++--- tests/fixtures/community-artifacts.ts | 4 + 11 files changed, 382 insertions(+), 107 deletions(-) diff --git a/docs/communities.md b/docs/communities.md index 00b15eaf62..c1b3443a97 100644 --- a/docs/communities.md +++ b/docs/communities.md @@ -120,7 +120,13 @@ Each snapshot and command reads a changed device record before it uses membershi Storage events also notify readers and dispose sessions for removed memberships. A new local join has a generation value. Artifact handles capture this value, so leave and rejoin cannot restore an old handle. Records without a generation remain -readable. Explicit rejoin creates a new handle and session. +readable. Explicit rejoin creates a new handle and session. Account sync also assigns a new +generation when it adds a membership. Snapshot reads notify subscribers after the +read returns, so a sync owner cannot start a second drain from inside its snapshot. + +Leave takes the artifact partition lock before it removes membership. It holds the +lock through device purge. New artifact collections wait for that purge before +they initialize, so a concurrent rejoin keeps its fresh records. ## Import from the previous desktop app diff --git a/docs/local-artifact-storage.md b/docs/local-artifact-storage.md index 54194fe220..dd635d75f8 100644 --- a/docs/local-artifact-storage.md +++ b/docs/local-artifact-storage.md @@ -132,5 +132,5 @@ and indexed pages use all four stores. Metadata operations use only metadata. Large row batches can still delay other collections' reads or writes. A changed row rewrites its indexes. Index upgrades scan the collection. Delta completion does not scan records. Ancestry comparison and separate source-membership writes are absent. The database upgrade removes the obsolete membership store, builds the host -channel index, and preserves records and device preferences. Leaving a community purges only its saved partition. The community service propagates membership and selection changes to other windows. Handles capture the local membership generation and close when it changes. Opens read the current device record, so a window cannot reopen a left partition before its storage event arrives. Explicit rejoin permits new handles; old handles stay closed. If database +channel index, and preserves records and device preferences. Leaving a community purges only its saved partition. The community service propagates membership and selection changes to other windows. Handles capture the local membership generation and close when it changes. Opens read the current device record, so a window cannot reopen a left partition before its storage event arrives. Explicit rejoin permits new handles; old handles stay closed. Leave holds an exclusive Web Lock for the partition before membership removal, through purge. Collection initialization takes the same lock in shared mode. New collections wait for an in-flight purge. Queued initialization and writes also validate the captured membership. If database opening fails, close that handle and open another after the cause clears. diff --git a/src/bundled/builderlab/known-communities/sync.test.ts b/src/bundled/builderlab/known-communities/sync.test.ts index e611606922..f1c3728c02 100644 --- a/src/bundled/builderlab/known-communities/sync.test.ts +++ b/src/bundled/builderlab/known-communities/sync.test.ts @@ -1,5 +1,7 @@ // @vitest-environment jsdom import { afterEach, beforeEach, expect, it, vi } from "vitest"; +import { Context } from "@deepseek-ai/cordis"; +import { createCommunities } from "../../../features/communities/service"; import { emptySync, enqueue, @@ -804,3 +806,67 @@ it("disposal stops listening", async () => { await vi.advanceTimersByTimeAsync(120_000); expect(h.client.add).not.toHaveBeenCalled(); }); + +it("keeps one upload in flight when a wake reads a changed device record", async () => { + const root = new Context(); + const record = `buzz-client.v1:${viewer}`; + localStorage.removeItem(record); + const communities = createCommunities( + root, + false, + undefined, + "", + undefined, + Promise.resolve(viewer), + ); + await until(() => expect(communities.snapshot().status).toBe("ready")); + const client = fakeClient(); + const upload = deferred(); + client.add.mockReturnValue(upload.promise); + const session = createOAuthSession(async () => ({ + value: "fixture", + account: { + subject: "user", + email: "fixture@example.test", + canUseRemoteAgents: false, + }, + })); + const stop = startKnownCommunitiesSync({ + client, + session, + knownCommunities: { + snapshot: communities.snapshot, + subscribe: communities.subscribe, + pending: communities.pendingSync, + apply: communities.applySync, + }, + }); + try { + await session.signIn(); + await until(() => expect(communities.snapshot().sync.account).toBe("user")); + localStorage.setItem( + record, + JSON.stringify({ + ...communities.snapshot(), + memberships: [{ id: "https://primary.example", name: "Primary" }], + sync: { + known: [], + outbox: [{ url: primary, removed: false }], + account: "user", + }, + }), + ); + window.dispatchEvent(new Event("online")); + await until(() => expect(client.add).toHaveBeenCalled()); + expect(client.add).toHaveBeenCalledTimes(1); + upload.resolve(accepted); + await until(() => expect(communities.pendingSync()).toEqual([])); + expect(client.add).toHaveBeenCalledTimes(1); + } finally { + upload.resolve(accepted); + stop(); + session.dispose(); + await root.fiber.dispose(); + localStorage.removeItem(record); + } +}); diff --git a/src/features/artifact-storage/service.test.ts b/src/features/artifact-storage/service.test.ts index 559b9789f9..9424f3e6c3 100644 --- a/src/features/artifact-storage/service.test.ts +++ b/src/features/artifact-storage/service.test.ts @@ -57,7 +57,11 @@ it("binds collection ownership to the calling plugin and fences scope changes", } listener.mockClear(); const handle = plugin.artifactStorage.open(definition); - expect(factory).toHaveBeenCalledWith("test.plugin", definition); + expect(factory).toHaveBeenCalledWith( + "test.plugin", + definition, + expect.any(Function), + ); expect(listener.mock.calls[0]?.[0]).toMatchObject({ owner: "test.plugin", viewer: "viewer", diff --git a/src/features/artifact-storage/service.ts b/src/features/artifact-storage/service.ts index b554991baf..9ce18bb9c5 100644 --- a/src/features/artifact-storage/service.ts +++ b/src/features/artifact-storage/service.ts @@ -68,7 +68,7 @@ export class ArtifactStorageService extends Service implements ArtifactStorage { store = createArtifactStore(viewer, origin); this.stores.set(scope, store); } - const collection = store.collection(owner, definition); + const collection = store.collection(owner, definition, valid); const controller = new AbortController(); let unsubscribe: (() => void) | undefined; let removeEffect: (() => void) | undefined; @@ -79,16 +79,18 @@ export class ArtifactStorageService extends Service implements ArtifactStorage { collection.close(); removeEffect?.(); }; - const valid = () => { + function valid() { const now = capture(); if ( controller.signal.aborted || now.viewer !== viewer || now.origin !== origin || now.generation !== generation - ) + ) { + close(); throw new Error("Artifact storage scope changed"); - }; + } + } unsubscribe = this.ctx.communityReader.subscribe(() => { const now = capture(); if ( diff --git a/src/features/artifact-storage/store.ts b/src/features/artifact-storage/store.ts index 05428edcca..90de3b406f 100644 --- a/src/features/artifact-storage/store.ts +++ b/src/features/artifact-storage/store.ts @@ -29,6 +29,21 @@ function sameRefresh( return left?.source === right.source && left.generation === right.generation; } +/** Leave holds this lock before it removes membership, through the whole purge. */ +export function withArtifactPartitionLock( + viewer: string, + relay: string, + mode: LockMode, + work: () => Promise, + signal?: AbortSignal, +): Promise { + return navigator.locks.request( + `${DB}:${relayPartition(relay, viewer)}`, + { mode, ...(signal ? { signal } : {}) }, + work, + ); +} + /** Same IndexedDB transaction boundary as the relay journals. No head-cache eviction. */ export function createArtifactStore(viewer: string, relay: string) { if (!viewer || !relay) @@ -174,6 +189,7 @@ export function createArtifactStore(viewer: string, relay: string) { function collection( owner: string, definition: ArtifactCollectionDefinition, + validate?: () => void, ): HostArtifactCollection { if (closed) throw new Error("Artifact storage closed"); if ( @@ -234,46 +250,56 @@ export function createArtifactStore(viewer: string, relay: string) { }, ); } - const ready = transact( - "readwrite", - STORES, - (tx, then, done) => { - const metadata = tx.objectStore("metadata"); - then(metadata.get(key("schema")), (schema) => { - if (schema?.type && schema.type !== definition.type) - throw new Error("Artifact collection type changed"); - if (schema?.version === definition.version) { - done(); - return; - } - if (schema?.version > definition.version) - throw new Error("Artifact collection requires a newer plugin"); - metadata.delete(key("source")); - metadata.delete(key("refresh")); - metadata.delete(key("checkpoint")); - const request = tx.objectStore("records").openCursor(range(prefix)); - then(request, (cursor) => { - if (!cursor) { - metadata.put({ - scope, - collection, - key: "schema", - version: definition.version, - type: definition.type, + const ready = withArtifactPartitionLock( + viewer, + relay, + "shared", + () => + transact( + "readwrite", + STORES, + (tx, then, done) => { + validate?.(); + const metadata = tx.objectStore("metadata"); + then(metadata.get(key("schema")), (schema) => { + if (schema?.type && schema.type !== definition.type) + throw new Error("Artifact collection type changed"); + if (schema?.version === definition.version) { + done(); + return; + } + if (schema?.version > definition.version) + throw new Error("Artifact collection requires a newer plugin"); + metadata.delete(key("source")); + metadata.delete(key("refresh")); + metadata.delete(key("checkpoint")); + const request = tx + .objectStore("records") + .openCursor(range(prefix)); + then(request, (cursor) => { + if (!cursor) { + metadata.put({ + scope, + collection, + key: "schema", + version: definition.version, + type: definition.type, + }); + done(); + return; + } + then( + tx.objectStore("preferences").get(key(cursor.value.id)), + (pref: StoredPreference | undefined) => + reindex(tx, then, cursor.value, pref?.value, () => + cursor.continue(), + ), + ); }); - done(); - return; - } - then( - tx.objectStore("preferences").get(key(cursor.value.id)), - (pref: StoredPreference | undefined) => - reindex(tx, then, cursor.value, pref?.value, () => - cursor.continue(), - ), - ); - }); - }); - }, + }); + }, + lifetime.signal, + ), lifetime.signal, ); // Keep an unopened handle's initialization rejection observable by its first operation. @@ -291,6 +317,7 @@ export function createArtifactStore(viewer: string, relay: string) { stores, (tx, then, done) => { then(tx.objectStore("metadata").get(key("schema")), (schema) => { + validate?.(); if (schema?.version !== definition.version) throw new Error("Artifact collection version changed"); work(tx, then, done); diff --git a/src/features/communities/legacy-communities.test.ts b/src/features/communities/legacy-communities.test.ts index 183a270d9f..1a52c40660 100644 --- a/src/features/communities/legacy-communities.test.ts +++ b/src/features/communities/legacy-communities.test.ts @@ -37,7 +37,10 @@ it("imports once and only after consent", async () => { answer(true); await flush(); expect(communities.snapshot().memberships).toEqual([ - { id: "https://old.example", name: "old.example" }, + expect.objectContaining({ + id: "https://old.example", + name: "old.example", + }), ]); expect(communities.snapshot().sync.outbox).toEqual([ { url: "wss://old.example", removed: false }, diff --git a/src/features/communities/service.test.ts b/src/features/communities/service.test.ts index 244fa3bac1..a3cc01e79e 100644 --- a/src/features/communities/service.test.ts +++ b/src/features/communities/service.test.ts @@ -920,7 +920,11 @@ it("a synced-in community shows the viewer's local name and reads its presentati "wss://new.example", ]); expect(client.snapshot().memberships).toEqual([ - { id: "https://new.example", name: "new.example", localName: "Mine" }, + expect.objectContaining({ + id: "https://new.example", + name: "new.example", + localName: "Mine", + }), ]); await flush(); expect(requests).toContain( diff --git a/src/features/communities/service.ts b/src/features/communities/service.ts index 92a94df9c1..2a50d5edfa 100644 --- a/src/features/communities/service.ts +++ b/src/features/communities/service.ts @@ -12,6 +12,7 @@ import { relayAddress, } from "./destination"; import { purgeCommunityDeviceState, type PurgeFailure } from "./device-state"; +import { withArtifactPartitionLock } from "../artifact-storage/store"; import { fetchCommunityPresentation } from "./community-icon"; import { emptySync, @@ -203,6 +204,11 @@ export function createCommunities( const emitRelay = () => { for (const fn of relayListeners) fn(); }; + const notify = () => { + if (disposed) return; + for (const fn of listeners) fn(); + emitRelay(); + }; const update = ( patch: Partial, persist = true, @@ -242,8 +248,7 @@ export function createCommunities( } unresolvedSelection = selection; state = next; - for (const fn of listeners) fn(); - emitRelay(); + notify(); }; const acquire = (id: string, viewer = state.viewer) => { if (!connect) return disconnected; @@ -427,17 +432,12 @@ export function createCommunities( } catch { return; } - for (const { id, generation } of state.memberships) { - if ( - saved.memberships.some( + const removed = state.memberships.filter( + ({ id, generation }) => + !saved.memberships.some( (entry) => entry.id === id && entry.generation === generation, - ) - ) - continue; - presentations.delete(id); - membershipVersions.set(id, (membershipVersions.get(id) ?? 0) + 1); - void release(id); - } + ), + ); saved.memberships = saved.memberships.map((entry) => ({ ...state.memberships.find( (current) => @@ -446,8 +446,15 @@ export function createCommunities( ...entry, localName: readLocalCommunityName(entry.id, viewer), })); + state = { ...state, ...saved }; + for (const { id } of removed) { + presentations.delete(id); + membershipVersions.set(id, (membershipVersions.get(id) ?? 0) + 1); + void release(id); + } if (saved.selected) acquire(saved.selected); - update(saved, false); + // Snapshot readers can hold their own operation guard only after this call returns. + queueMicrotask(notify); refreshPresentation(); } const syncLocalNames = (event: StorageEvent) => { @@ -604,30 +611,36 @@ export function createCommunities( id: string, { purge = true }: { purge?: boolean } = {}, ): Promise { - syncSaved(); id = communityDestination(id).id; - if (!state.memberships.some((m) => m.id === id)) return []; - const { viewer } = state; const origin = communityDestination(id).url; - update( - { - memberships: state.memberships.filter((m) => m.id !== id), - ...(state.selected === id ? { selected: null } : {}), - sync: enqueue(state.sync, relayAddress(id), true), - }, - true, - !!nativeConnect && !live, - ); - // Fence metadata responses for the old membership, including remove/rejoin. - presentations.delete(id); - membershipVersions.set(id, (membershipVersions.get(id) ?? 0) + 1); - refreshPresentation(); - const failures = await release(id); - // The session is gone (or at least detached), so nothing below can - // refill the purged stores. - if (purge && viewer) - failures.push(...(await purgeCommunityDeviceState(origin, viewer))); - return failures; + const forget = async () => { + syncSaved(); + if (!state.memberships.some((m) => m.id === id)) return []; + const { viewer } = state; + update( + { + memberships: state.memberships.filter((m) => m.id !== id), + ...(state.selected === id ? { selected: null } : {}), + sync: enqueue(state.sync, relayAddress(id), true), + }, + true, + !!nativeConnect && !live, + ); + // Fence metadata responses for the old membership, including remove/rejoin. + presentations.delete(id); + membershipVersions.set(id, (membershipVersions.get(id) ?? 0) + 1); + refreshPresentation(); + const failures = await release(id); + // The session is gone (or at least detached), so nothing below can + // refill the purged stores. + if (purge && viewer) + failures.push(...(await purgeCommunityDeviceState(origin, viewer))); + return failures; + }; + const viewer = state.viewer; + return purge && viewer && typeof indexedDB !== "undefined" + ? withArtifactPartitionLock(viewer, origin, "exclusive", forget) + : forget(); }, pendingSync() { syncSaved(); @@ -648,6 +661,7 @@ export function createCommunities( memberships.push({ id, name, + generation: crypto.randomUUID(), ...(state.viewer ? { localName: readLocalCommunityName(id, state.viewer) } : {}), diff --git a/tests/browser/community-artifacts.spec.mjs b/tests/browser/community-artifacts.spec.mjs index 87c9937a04..8054881e7a 100644 --- a/tests/browser/community-artifacts.spec.mjs +++ b/tests/browser/community-artifacts.spec.mjs @@ -97,7 +97,67 @@ for (const { delayedEvent, purge } of [ }); } -test("rejects an old handle after leave and rejoin before event delivery", async ({ +for (const syncJoin of [false, true]) { + test(`rejects an old handle after leave and ${syncJoin ? "synced" : "explicit"} rejoin before event delivery`, async ({ + page, + context, + }) => { + if (syncJoin) + await page.addInitScript(() => { + localStorage.setItem( + `buzz-client.v1:${"a".repeat(64)}`, + JSON.stringify({ + memberships: [{ id: "https://fixture.example", name: "Fixture" }], + selected: "https://fixture.example", + }), + ); + }); + await page.goto("/tests/fixtures/community-artifacts.html"); + await page.waitForFunction( + () => window.communityArtifacts?.snapshot().status === "ready", + ); + if (!syncJoin) await page.evaluate(() => window.communityArtifacts.join()); + const other = await context.newPage(); + await other.addInitScript(() => { + window.addEventListener("storage", (event) => { + if (window.pauseStorage) event.stopImmediatePropagation(); + }); + }); + await other.goto(page.url()); + await other.waitForFunction( + () => window.communityArtifacts?.snapshot().selected, + ); + await other.evaluate(async () => { + window.handle = window.communityArtifacts.open(); + await window.handle.get("task"); + window.pauseStorage = true; + }); + await page.evaluate(async (syncJoin) => { + await window.communityArtifacts.leave(true); + if (syncJoin) await window.communityArtifacts.syncJoin(); + else window.communityArtifacts.join(); + await window.communityArtifacts + .open() + .importBatch({ records: [{ id: "task", local: { title: "fresh" } }] }); + }, syncJoin); + await expect( + other.evaluate(() => + window.handle.importBatch({ records: [{ id: "stale", local: {} }] }), + ), + ).rejects.toThrow("scope changed"); + expect( + await other.evaluate(() => window.communityArtifacts.aborted()), + ).toBe(true); + expect( + await other.evaluate( + async () => + (await window.communityArtifacts.open().get("task"))?.record.local, + ), + ).toEqual({ title: "fresh" }); + }); +} + +test("rejoin waits for a pending purge before it imports fresh artifacts", async ({ page, context, }) => { @@ -107,35 +167,57 @@ test("rejects an old handle after leave and rejoin before event delivery", async ); await page.evaluate(() => window.communityArtifacts.join()); const other = await context.newPage(); - await other.addInitScript(() => { - window.addEventListener("storage", (event) => { - if (window.pauseStorage) event.stopImmediatePropagation(); - }); - }); await other.goto(page.url()); await other.waitForFunction( () => window.communityArtifacts?.snapshot().selected, ); - await other.evaluate(async () => { - window.handle = window.communityArtifacts.open(); - await window.handle.get("task"); - window.pauseStorage = true; - }); await page.evaluate(async () => { - await window.communityArtifacts.leave(true); - window.communityArtifacts.join(); - await window.communityArtifacts - .open() - .importBatch({ records: [{ id: "task", local: { title: "fresh" } }] }); + const open = IDBFactory.prototype.open; + const held = new Promise((resolve) => { + IDBFactory.prototype.open = function (...args) { + const request = open.apply(this, args); + if (args[0] === "buzz-channel-heads-v1") { + Object.defineProperty(request, "onsuccess", { + set(listener) { + request.addEventListener( + "success", + (event) => { + IDBFactory.prototype.open = open; + window.releasePurge = () => listener.call(request, event); + resolve(); + }, + { once: true }, + ); + }, + }); + } + return request; + }; + }); + window.pendingLeave = window.communityArtifacts.leave(true); + await held; }); - await expect( - other.evaluate(() => - window.handle.importBatch({ records: [{ id: "stale", local: {} }] }), - ), - ).rejects.toThrow("scope changed"); - expect(await other.evaluate(() => window.communityArtifacts.aborted())).toBe( - true, - ); + try { + await other.evaluate(() => { + window.communityArtifacts.join(); + window.freshImport = window.communityArtifacts + .open() + .importBatch({ records: [{ id: "task", local: { title: "fresh" } }] }); + }); + const pending = await other.evaluate( + async () => (await navigator.locks.query()).pending, + ); + expect( + pending.some( + (lock) => + lock.name.startsWith("buzz-artifacts-v1:") && lock.mode === "shared", + ), + ).toBe(true); + } finally { + await page.evaluate(() => window.releasePurge()); + } + await page.evaluate(() => window.pendingLeave); + await other.evaluate(() => window.freshImport); expect( await other.evaluate( async () => @@ -143,3 +225,66 @@ test("rejects an old handle after leave and rejoin before event delivery", async ), ).toEqual({ title: "fresh" }); }); + +test("cancels an import whose collection initialization waits behind leave", async ({ + page, + context, +}) => { + await page.goto("/tests/fixtures/community-artifacts.html"); + await page.waitForFunction( + () => window.communityArtifacts?.snapshot().status === "ready", + ); + await page.evaluate(() => window.communityArtifacts.join()); + const other = await context.newPage(); + await other.addInitScript(() => + window.addEventListener("storage", (event) => + event.stopImmediatePropagation(), + ), + ); + await other.goto(page.url()); + await other.waitForFunction( + () => window.communityArtifacts?.snapshot().selected, + ); + await page.evaluate(async () => { + const held = new Promise((resolve) => { + window.partitionGate = navigator.locks.request( + `buzz-artifacts-v1:https://fixture.example:${"a".repeat(64)}`, + async () => { + resolve(); + await new Promise((release) => { + window.releaseGate = release; + }); + }, + ); + }); + await held; + window.pendingLeave = window.communityArtifacts.leave(true); + }); + try { + await other.evaluate(() => { + window.pendingImport = window.communityArtifacts + .open() + .importBatch({ records: [{ id: "task", local: { title: "stale" } }] }) + .then( + () => null, + (error) => error.message, + ); + }); + const pending = await page.evaluate( + async () => (await navigator.locks.query()).pending, + ); + expect(pending.map((lock) => lock.mode)).toEqual(["exclusive", "shared"]); + } finally { + await page.evaluate(() => window.releaseGate()); + } + await page.evaluate(() => window.pendingLeave); + expect(await other.evaluate(() => window.pendingImport)).toMatch( + /scope changed|closed/, + ); + expect(await other.evaluate(() => window.communityArtifacts.aborted())).toBe( + true, + ); + expect( + await other.evaluate(() => window.communityArtifacts.saved()), + ).toBeUndefined(); +}); diff --git a/tests/fixtures/community-artifacts.ts b/tests/fixtures/community-artifacts.ts index aa37c85046..6f9c4b1628 100644 --- a/tests/fixtures/community-artifacts.ts +++ b/tests/fixtures/community-artifacts.ts @@ -34,6 +34,10 @@ storage.onOpen((entry) => { const api = { snapshot: communities.snapshot, join: () => communities.joined(membership, profile), + async syncJoin() { + await communities.applySync({ known: [], outbox: [] }, [membership.id]); + communities.select(membership.id); + }, leave: (purge: boolean) => communities.leave(membership.id, { purge }), open: () => plugin.artifactStorage.open(definition), aborted: () => signal?.aborted,