fix(lib): say what actually blocks disconnecting a workspace

.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

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

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);
+  });
+});

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);
   });

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<string>();
 
   /** 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,23 +1033,38 @@ 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
    * writes would never end. Those writes are dropped from the outbox, since a
    * local-only drive is never pushed; turning sync on again resyncs the whole
    * drive rather than replaying them. */
   public async makeDriveLocal(drive: string): Promise<void> {
-    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). */