diff --git a/.gitignore b/.gitignore index ba6de0002..a1aa9ebf3 100644 --- a/.gitignore +++ b/.gitignore @@ -16,6 +16,8 @@ artifact /.e2e-store /.e2e-runs server/assets_tmp +# Same, for the plugin assets `build.rs` stages before embedding them. +server/integrations_assets_tmp server/js-build.log .netlify scratchpad diff --git a/browser/CHANGELOG.md b/browser/CHANGELOG.md index 8e55af3ef..6a2ed99b3 100644 --- a/browser/CHANGELOG.md +++ b/browser/CHANGELOG.md @@ -4,6 +4,20 @@ This changelog covers all five packages, as they are (for now) updated as a whol ## UNRELEASED +- Turning workspace sync off says what is actually in the way. All three of its + preconditions used to answer with "Open this drive with local storage + available before disconnecting", so someone signed out, or on a server this + client holds no live connection to, was told to do something they had already + done. Each one now names itself, and a socket that exists but is not open says + so rather than leaving the attempt to fail on "WebSocket is not open". The + local database is also waited for instead of refused: it attaches a few + hundred milliseconds after a page load and again after every sign-in, and a + click inside that window was rejected outright. +- A local-only workspace is recognised whatever spelling its subject is written + in. The set of disconnected workspaces is read through the canonical form but + was written raw, so a trailing slash or a legacy `did:ad:` identifier could + store a key nothing found again, and a resource inside such a workspace could + still try to reach the server. - One Loro wasm panic no longer turns into an error every few seconds for the rest of a tab's life. Presence gives up the drive's ephemeral store on the first failed call, whichever call it is, including a peer's bytes and the diff --git a/browser/lib/src/make-drive-local.test.ts b/browser/lib/src/make-drive-local.test.ts new file mode 100644 index 000000000..5df1141fd --- /dev/null +++ b/browser/lib/src/make-drive-local.test.ts @@ -0,0 +1,127 @@ +import { describe, it } from 'vitest'; +import type { ClientDbWorker } from './client-db.js'; +import { Resource } from './resource.js'; +import { testStore } from './test-store.js'; + +const DRIVE = 'https://example.com/drive/abc'; + +/** Only the two members `makeDriveLocal`'s preconditions read. `setClientDb` + * touches nothing else on a store with no pending writes. */ +const dbStub = (initialized: boolean) => + ({ + isReady: false, + waitForInit: async () => initialized, + }) as unknown as ClientDbWorker; + +/** + * Turning workspace sync off has three preconditions, and they used to share + * one message: "Open this drive with local storage available before + * disconnecting". Signed out, or on a server this client holds no socket for, + * that told the user to do something they had already done. + */ +describe('makeDriveLocal preconditions', () => { + it('asks a signed-out user to sign in', async ({ expect }) => { + const { store } = await testStore(); + store.setAgent(undefined); + + await expect(store.makeDriveLocal(DRIVE)).rejects.toThrow(/Sign in/); + }); + + it('still asks for local storage when there is no database', async ({ + expect, + }) => { + // No `expectClientDb`, so this app has opted out and waiting is pointless. + const { store } = await testStore(); + + await expect(store.makeDriveLocal(DRIVE)).rejects.toThrow(/local storage/); + }); + + it('asks for local storage when the database never initializes', async ({ + expect, + }) => { + const { store } = await testStore(); + store.setClientDb(dbStub(false)); + + await expect(store.makeDriveLocal(DRIVE)).rejects.toThrow(/local storage/); + }); + + it('asks for a server once the database is there', async ({ expect }) => { + const { store } = await testStore(); + store.setClientDb(dbStub(true)); + + // Past the storage guard, and a test store holds no websocket. + await expect(store.makeDriveLocal(DRIVE)).rejects.toThrow( + /Connect to a server/, + ); + }); + + it('waits for a database that attaches after the click', async ({ + expect, + }) => { + // The attach lands a few hundred ms after boot and after every agent + // change. A click inside that window used to be refused outright. + const { store } = await testStore(); + store.expectClientDb(); + + const pending = store.makeDriveLocal(DRIVE); + store.setClientDb(dbStub(true)); + + // Reaching the socket check proves it waited instead of refusing. + await expect(pending).rejects.toThrow(/Connect to a server/); + }); +}); + +/** The set is read through `normalizeSubject`, so it has to be written + * through it too. */ +describe('local-only drive registration', () => { + it('matches a drive registered with a trailing slash', async ({ expect }) => { + const { store } = await testStore(); + store.registerLocalOnlyDrive(`${DRIVE}/`); + + expect(store.isLocalOnlyDrive(DRIVE)).toBe(true); + }); + + it('unregisters either spelling', async ({ expect }) => { + const { store } = await testStore(); + store.registerLocalOnlyDrive(DRIVE); + store.unregisterLocalOnlyDrive(`${DRIVE}/`); + + expect(store.isLocalOnlyDrive(DRIVE)).toBe(false); + }); + + it('matches a drive registered with the legacy scheme', async ({ + expect, + }) => { + // `vaultAutoBackup` registers and reads back `did:ad:drive:…` verbatim, + // which is the case a raw `has` on the set got wrong. + const { store } = await testStore(); + store.registerLocalOnlyDrive('did:ad:drive:test'); + + expect(store.isLocalOnlyDrive('did:ad:drive:test')).toBe(true); + expect(store.isLocalOnlyDrive('atomic:drive:test')).toBe(true); + + store.unregisterLocalOnlyDrive('did:ad:drive:test'); + + expect(store.isLocalOnlyDrive('did:ad:drive:test')).toBe(false); + }); + + it('matches a resource whose drive propval uses the legacy scheme', async ({ + expect, + }) => { + // A demo workspace registers `did:ad:` and the server writes the same + // spelling into each resource's `drive`. The set holds the canonical + // `atomic:` form, so this branch has to canonicalize before it looks. + const { store } = await testStore(); + store.registerLocalOnlyDrive('did:ad:legacy-drive'); + + const child = new Resource('did:ad:legacy-child'); + child.setStore(store); + child.applyHydratedValues([ + ['https://atomicdata.dev/properties/drive', 'did:ad:legacy-drive'], + ]); + child.loading = false; + store.addResource(child); + + expect(store.isLocalOnlySubject('did:ad:legacy-child')).toBe(true); + }); +}); diff --git a/browser/lib/src/store.refused-drive.test.ts b/browser/lib/src/store.refused-drive.test.ts index 15eefdfe1..3c9078faa 100644 --- a/browser/lib/src/store.refused-drive.test.ts +++ b/browser/lib/src/store.refused-drive.test.ts @@ -12,7 +12,8 @@ function storeWithLocalCopy() { vi.spyOn(store, 'getAgent').mockReturnValue({} as Agent); vi.spyOn(store, 'getClientDb').mockReturnValue({ isReady: true, - } as ClientDbWorker); + waitForInit: async () => true, + } as unknown as ClientDbWorker); return store; } @@ -63,8 +64,9 @@ describe('a drive the server refuses as not enrolled', () => { const store = storeWithLocalCopy(); vi.spyOn(store, 'getDefaultWebSocket').mockReturnValue(undefined); + // Verifying needs the server's inventory, so without a socket it stops. await expect(store.makeDriveLocal(DRIVE)).rejects.toThrow( - 'Open this drive with local storage available before disconnecting.', + 'Connect to a server before disconnecting this workspace.', ); expect(store.isLocalOnlyDrive(DRIVE)).toBe(false); }); diff --git a/browser/lib/src/store.ts b/browser/lib/src/store.ts index 509b6951b..554b2747d 100644 --- a/browser/lib/src/store.ts +++ b/browser/lib/src/store.ts @@ -794,7 +794,14 @@ export class Store { const rawLocalOnly = localStorage.getItem('atomic.localOnlyDrives'); if (rawLocalOnly) { - this.localOnlyDrives = new Set(JSON.parse(rawLocalOnly)); + // Normalized on the way back in: an earlier build stored whatever + // spelling the caller passed, and every reader looks with the + // canonical form. + this.localOnlyDrives = new Set( + (JSON.parse(rawLocalOnly) as string[]).map(subject => + this.normalizeSubject(subject), + ), + ); } } catch { // ignore corrupt value @@ -979,16 +986,22 @@ export class Store { private localOnlyDrives = new Set(); /** Mark a drive as local-only. Must be called BEFORE the drive's first - * `save()` — registration is what routes saves away from the outbox. */ + * `save()` — registration is what routes saves away from the outbox. + * + * Normalized on the way in, because every reader normalizes before it + * looks (`isLocalOnlyDrive`, `isLocalOnlySubject`): a caller's trailing + * slash or `did:ad:` spelling would otherwise store a key nothing finds. */ public registerLocalOnlyDrive(drive: string): void { + const normalized = this.normalizeSubject(drive); + if (typeof localStorage !== 'undefined') { localStorage.setItem( 'atomic.localOnlyDrives', - JSON.stringify([...new Set([...this.localOnlyDrives, drive])]), + JSON.stringify([...new Set([...this.localOnlyDrives, normalized])]), ); } - this.localOnlyDrives.add(drive); + this.localOnlyDrives.add(normalized); } /** @@ -1020,6 +1033,12 @@ export class Store { /** Switch this client to browser-only sync after verifying its local copy. * Does not delete data from the server or alter other devices' configuration. * + * The three preconditions each get their own message. They used to share + * "Open this drive with local storage available before disconnecting", which + * is only true for one of them: someone signed out, or on a server this + * client holds no socket for, was told to do something they had already done + * and given nothing to act on. + * * A drive the server refuses ({@link isDriveRefusedByServer}) skips the * verification: the server will not serve its copy, so this device's copy * is the only one there is, and waiting on its inventory or on the refused @@ -1027,16 +1046,25 @@ export class Store { * local-only drive is never pushed; turning sync on again resyncs the whole * drive rather than replaying them. */ public async makeDriveLocal(drive: string): Promise { - const db = this.getClientDb(); + const normalized = this.normalizeSubject(drive); const agent = this.getAgent(); - if (this.isDriveRefusedByServer(drive)) { - if (!db?.isReady || !agent) - throw new Error( - 'Open this drive with local storage available before disconnecting.', - ); + if (!agent) throw new Error('Sign in before disconnecting this workspace.'); - const normalized = this.normalizeSubject(drive); + // The local database attaches a few hundred ms after boot and again after + // every agent change, so a click inside that window found no database at + // all. Wait for the attach rather than refusing. `waitForInit` and not + // `isReady`, because the latter also demands the bootstrap seed and + // nothing below reads a bootstrap resource. + await this.waitForClientDb(); + const db = this.getClientDb(); + + if (!db || !(await db.waitForInit())) + throw new Error( + 'Open this drive with local storage available before disconnecting.', + ); + + if (this.isDriveRefusedByServer(drive)) { this.registerLocalOnlyDrive(drive); this.getDefaultWebSocket()?.unsubscribeFromDrive(drive); @@ -1062,10 +1090,21 @@ export class Store { } const serverUrl = this.serverUrl; - const ws = this.getDefaultWebSocket(); - if (!db?.isReady || !agent || !ws) - throw new Error( - 'Open this drive with local storage available before disconnecting.', + // Sockets are registered under whatever string opened them, which is not + // always `serverUrl`, so fall back to the drive's origin exactly as + // `promoteLocalDrive` does. An open one, at that: the inventory below is a + // live request, and a socket that merely exists left `driveInventory` to fail + // with "WebSocket is not open", which is not something a user can act on. + const open = (candidate: WSClient | undefined) => + candidate?.readyState === WebSocket.OPEN ? candidate : undefined; + const ws = + open(this.getDefaultWebSocket()) ?? + open(this.getWebSocketForSubject(normalized)); + + if (!ws) + throw new AtomicError( + 'Connect to a server before disconnecting this workspace.', + ErrorType.Server, ); const current = () => { @@ -1084,6 +1123,11 @@ export class Store { }; current(); + // The caller's spelling goes to the socket and to the verification, not the + // normalized one: `driveInventory` echoes the subjects it was asked about, + // `verifyLocalDriveCopy` matches the drive against them, and the drive-wide + // SUB this later drops was sent under `getDrive()`'s own spelling, which is + // not normalized either. `registerLocalOnlyDrive` normalizes for itself. const inventory = await ws.driveInventory(drive, ''); await verifyLocalDriveCopy(db, drive, inventory); // A second inventory catches changes made while attachment verification ran. @@ -1097,7 +1141,8 @@ export class Store { /** Forget a local-only drive (e.g. after deleting a demo workspace), * keeping the persisted registration set bounded. */ public unregisterLocalOnlyDrive(drive: string): void { - if (!this.localOnlyDrives.delete(drive)) return; + // Normalized to match what `registerLocalOnlyDrive` stored. + if (!this.localOnlyDrives.delete(this.normalizeSubject(drive))) return; if (typeof localStorage !== 'undefined') { localStorage.setItem( @@ -1108,7 +1153,9 @@ export class Store { } public isLocalOnlyDrive(drive: string): boolean { - return this.localOnlyDrives.has(drive); + // Normalized, like the write side: the set holds canonical keys, and a + // caller may hold the `did:ad:` spelling or a trailing slash. + return this.localOnlyDrives.has(this.normalizeSubject(drive)); } /** @@ -1196,11 +1243,20 @@ export class Store { ); const drive = resource?.get('https://atomicdata.dev/properties/drive'); - if (typeof drive === 'string' && this.localOnlyDrives.has(drive)) { + // Normalized, because the set holds normalized keys and a propval is + // whatever the server wrote: a `did:ad:` spelling of an `atomic:` drive + // would otherwise miss here and let a local-only resource try a POST. + if ( + typeof drive === 'string' && + this.localOnlyDrives.has(this.normalizeSubject(drive)) + ) { return true; } - return this.localOnlyDrives.has(this.driveOf(normalized)); + // `driveOf` returns a raw `parent` propval, so normalize that too. + return this.localOnlyDrives.has( + this.normalizeSubject(this.driveOf(normalized)), + ); } /** Returns the ClientDbWorker if one has been set (may still be initializing). */