Move session locking into persistence

plans/versioned-localstorage-session-tangents.md

@@ -77,31 +77,21 @@ No action. This whole-root localStorage read-modify-write race predates the vers
 
 Compatibility reference: [MDN `Navigator.locks`](https://developer.mozilla.org/docs/Web/API/Navigator/locks).
 
-## 3. Enforce the session lock invariant
+## 3. Keep lock ownership inside persistence
 
 ### Problem
 
-`writeSession()` performs the conditional session read-modify-write but does not acquire the persisted-storage lock itself. Current session persistence callsites run it inside `runWithCredentialLock()`, which aliases the root storage lock, but this is a convention rather than an enforced API invariant.
+`writeSession()` previously relied on every caller to enter the persisted-storage lock before dispatching a session action. The lock ownership was transitive and invisible at the write API, so a future direct caller could silently bypass serialization.
 
-The lock cannot simply be added inside `writeSession()` while callers retain the outer lock because Web Locks are not reentrant. A nested request for the same exclusive lock would deadlock.
+### Resolution
 
-A future caller could accidentally do this:
+`writeSession()` now owns the persisted-storage lock on web. Its lock callback contains exactly the shared-state commit:
 
-```ts
+1. read the authoritative root from localStorage;
-await persisted.writeSession({
+2. conditionally merge the session mutation;
-  nextSession,
+3. write the updated root; and
-  credentialMutations,
+4. broadcast the committed update.
-})
-```
 
-without first entering the root lock.
+Session callsites no longer acquire the lock. Network requests, reducer work, expiry rescue, and local bundle reconciliation remain outside it. Native retains its existing serialized AsyncStorage queue inside `writeSession()`.
 
-### Possible direction
+This makes every call to `writeSession()` safe by construction, avoids non-reentrant nested Web Locks, and keeps the lock scoped to the operation that actually needs cross-tab exclusion.
-
-Potential enforcement options include:
-
-1. Add a development assertion tracking whether the current realm is inside `runWithPersistedStorageLock()`.
-2. Expose the unlocked commit only through a capability passed to the lock callback.
-3. Move lock ownership into a higher-level session transaction API that performs the authoritative read, reconciliation, and write together.
-
-Any enforcement should preserve the existing requirement that network refreshes happen outside the lock.

plans/versioned-localstorage-sessions.md

@@ -134,7 +134,7 @@ Tab A sends refresh token A to the PDS
 Tab A receives successor generation B
 ```
 
-Before committing the response, Tab A acquires the credential lock and synchronously rereads localStorage. The source of truth may have changed while its network request was in flight.
+Before committing the response, `writeSession()` acquires the persisted-storage lock and synchronously rereads localStorage. The source of truth may have changed while the network request was in flight.
 
 ### Case 1: nothing else changed
 
@@ -276,16 +276,14 @@ Tab A plans:  write version 9
 Tab B plans:  write version 9
 ```
 
-Network refreshes do not run while holding a Web Lock. A refresh captures the generation it uses, performs the network request, and acquires the lock only to reconcile and commit its result:
+Network refreshes do not run while holding a Web Lock. A refresh captures the generation it uses and performs the network request first. The resulting session mutation is then passed to `writeSession()`, which owns the lock around reconciliation and persistence:
 
-```ts
+```text
-const baseRefreshJti = getRefreshJti(session.refreshJwt)
+Capture refresh generation A
-const refreshed = await refreshSession()
+Perform network refresh A -> B
-
+Call writeSession with base A and result B
-await navigator.locks.request('bsky-persisted-storage', async () => {
+writeSession acquires the persisted-storage lock
-  const latest = readAccountFromLocalStorage(did)
+writeSession rereads, conditionally merges, and persists
-  // Commit only if latest is active and still has baseRefreshJti.
-})
 ```
 
 This avoids holding a cross-tab lock over network I/O. Multiple refresh requests may be in flight concurrently; server-side convergence and the generation-specific conditional commit make their results safe.

src/state/persisted/index.ts

@@ -16,10 +16,6 @@ import {type PersistedApi} from './types'
 import {normalizeData} from './util'
 
 export type {SessionCredentialMutation} from './session-merge'
-export {
-  runWithPersistedStorageLock as runWithCredentialLock,
-  runWithPersistedStorageLock,
-} from './storage-lock'
 export type {PersistedAccount, Schema} from '#/state/persisted/schema'
 export {defaults} from '#/state/persisted/schema'
 
@@ -78,6 +74,7 @@ export function write<K extends keyof Schema>(
 }
 write satisfies PersistedApi['write']
 
+/** Queue a conditional session merge with other root storage writes. */
 export function writeSession({
   nextSession,
   credentialMutations,

src/state/persisted/index.web.ts

@@ -17,10 +17,6 @@ import {type PersistedApi} from './types'
 import {normalizeData} from './util'
 
 export type {SessionCredentialMutation} from './session-merge'
-export {
-  runWithPersistedStorageLock as runWithCredentialLock,
-  runWithPersistedStorageLock,
-} from './storage-lock'
 export type {PersistedAccount, Schema} from '#/state/persisted/schema'
 export {defaults} from '#/state/persisted/schema'
 
@@ -112,8 +108,8 @@ export function write<K extends keyof Schema>(
 }
 write satisfies PersistedApi['write']
 
-// eslint-disable-next-line @typescript-eslint/require-await
+/** Commit a conditional session merge while holding the root storage lock. */
-export async function writeSession({
+export function writeSession({
   nextSession,
   credentialMutations,
   currentAccountDid,
@@ -122,6 +118,8 @@ export async function writeSession({
   credentialMutations: SessionCredentialMutation[]
   currentAccountDid?: string
 }): Promise<Schema['session']> {
+  return runWithPersistedStorageLock({
+    operation: () => {
       const stored = readFromStorage() ?? _state
       const session = applySessionUpdate({
         storedSession: stored.session,
@@ -135,6 +133,8 @@ export async function writeSession({
 
       broadcastUpdate({key: 'session'})
       return session
+    },
+  })
 }
 writeSession satisfies PersistedApi['writeSession']
 

src/state/persisted/storage-lock.ts

@@ -1,5 +1,3 @@
-import {type PersistedApi} from './types'
-
 export function runWithPersistedStorageLock<T>({
   operation,
 }: {
@@ -15,4 +13,3 @@ export function runWithPersistedStorageLock<T>({
     )
   }
 }
-runWithPersistedStorageLock satisfies PersistedApi['runWithPersistedStorageLock']

src/state/persisted/storage-lock.web.ts

@@ -1,5 +1,3 @@
-import {type PersistedApi} from './types'
-
 const PERSISTED_STORAGE_LOCK = 'bsky-persisted-storage'
 
 export function runWithPersistedStorageLock<T>({
@@ -22,8 +20,6 @@ export function runWithPersistedStorageLock<T>({
 
   return lockManager.request(PERSISTED_STORAGE_LOCK, operation)
 }
-runWithPersistedStorageLock satisfies PersistedApi['runWithPersistedStorageLock']
-
 function getLockManager(): LockManager | undefined {
   if (typeof navigator === 'undefined' || !('locks' in navigator)) {
     return undefined

src/state/persisted/types.ts

@@ -21,9 +21,6 @@ export type PersistedApi = {
     /** Omit to preserve the current account read from persisted storage. */
     currentAccountDid?: string
   }): Promise<Schema['session']>
-  runWithPersistedStorageLock<T>(args: {
-    operation: () => T | Promise<T>
-  }): Promise<T>
   onUpdate<K extends keyof Schema>(
     key: K,
     cb: (v: Schema[K]) => void,

src/state/session/__tests__/provider-abort-test.tsx

@@ -17,8 +17,6 @@ jest.mock('#/state/persisted', () => {
     readLatest: () => defaults.session,
     writeSession: ({nextSession}: {nextSession: typeof defaults.session}) =>
       Promise.resolve(nextSession),
-    runWithCredentialLock: ({operation}: {operation: () => unknown}) =>
-      Promise.resolve(operation()),
     onUpdate: () => () => {},
   }
 })

src/state/session/__tests__/provider-clients-test.tsx

@@ -21,8 +21,6 @@ jest.mock('#/state/persisted', () => {
     readLatest: () => defaults.session,
     writeSession: ({nextSession}: {nextSession: typeof defaults.session}) =>
       Promise.resolve(nextSession),
-    runWithCredentialLock: ({operation}: {operation: () => unknown}) =>
-      Promise.resolve(operation()),
     onUpdate: () => () => {},
   }
 })

src/state/session/__tests__/provider-refresh-session-test.tsx

@@ -20,8 +20,6 @@ jest.mock('#/state/persisted', () => {
     readLatest: () => defaults.session,
     writeSession: ({nextSession}: {nextSession: typeof defaults.session}) =>
       Promise.resolve(nextSession),
-    runWithCredentialLock: ({operation}: {operation: () => unknown}) =>
-      Promise.resolve(operation()),
     onUpdate: () => () => {},
   }
 })

src/state/session/__tests__/provider-session-events-test.tsx

@@ -49,8 +49,6 @@ jest.mock('#/state/persisted', () => ({
     mockPersisted.latest = committed
     return Promise.resolve(committed)
   },
-  runWithCredentialLock: ({operation}: {operation: () => unknown}) =>
-    Promise.resolve(operation()),
   onUpdate: (_key: 'session', callback: (value: Schema['session']) => void) => {
     mockPersistedListeners.push(callback)
     return () => {}

src/state/session/index.tsx

@@ -161,7 +161,7 @@ export function Provider({children}: React.PropsWithChildren<{}>) {
   const onSessionChangeRef = useRef<OnSessionChange | null>(null)
 
   const onSessionChange = useCallback(
-    (
+    async (
       bundle: SessionBundle,
       accountDid: string,
       sessionEvent: AtpSessionEvent,
@@ -172,8 +172,6 @@ export function Provider({children}: React.PropsWithChildren<{}>) {
           ? bundle.session.session.refreshJwt
           : sessionData?.refreshJwt
 
-      return persistedSession.runWithCredentialLock({
-        operation: async () => {
       /*
        * Only the live bundle may reset the expiry-rescue bookkeeping: a stale
        * bundle's late update would otherwise clear the failed-generation set
@@ -203,8 +201,7 @@ export function Provider({children}: React.PropsWithChildren<{}>) {
           ? sessionDataToSessionAccount(
               sessionData,
               sessionData.service,
-                  store.getState().accounts.find(a => a.did === accountDid)
+              store.getState().accounts.find(a => a.did === accountDid)?.pdsUrl,
-                    ?.pdsUrl,
             )
           : undefined
 
@@ -216,8 +213,8 @@ export function Provider({children}: React.PropsWithChildren<{}>) {
        */
       if (sessionEvent === 'expired') {
         const current = store.getState()
-            const currentBundle = current.currentBundleState
+        const currentBundle = current.currentBundleState.bundle as unknown as
-              .bundle as unknown as SessionBundle | PublicSessionBundle
+          SessionBundle | PublicSessionBundle
         const dyingRefreshJwt = sessionData?.refreshJwt
         // Stale bundle events are handled by the reducer's identity guard.
         if (
@@ -329,8 +326,6 @@ export function Provider({children}: React.PropsWithChildren<{}>) {
         }
       }
     },
-      })
-    },
     [store],
   )
   /*
@@ -358,9 +353,7 @@ export function Provider({children}: React.PropsWithChildren<{}>) {
         disposeBundle(bundle)
         return
       }
-      await persistedSession.runWithCredentialLock({
+      await store.dispatch(
-        operation: () =>
-          store.dispatch(
         {
           type: 'switched-to-account',
           newBundle: bundle,
@@ -373,8 +366,7 @@ export function Provider({children}: React.PropsWithChildren<{}>) {
             resultRefreshJwt: account.refreshJwt,
           },
         ],
-          ),
+      )
-      })
       ax.metric('account:create:success', metrics, {
         session: utils.accountToSessionMetadata(account),
       })
@@ -401,9 +393,7 @@ export function Provider({children}: React.PropsWithChildren<{}>) {
         disposeBundle(bundle)
         return
       }
-      await persistedSession.runWithCredentialLock({
+      await store.dispatch(
-        operation: () =>
-          store.dispatch(
         {
           type: 'switched-to-account',
           newBundle: bundle,
@@ -416,8 +406,7 @@ export function Provider({children}: React.PropsWithChildren<{}>) {
             resultRefreshJwt: account.refreshJwt,
           },
         ],
-          ),
+      )
-      })
       ax.metric(
         'account:loggedIn',
         {logContext, withPassword: true},
@@ -441,13 +430,10 @@ export function Provider({children}: React.PropsWithChildren<{}>) {
       const prevState = store.getState()
       const accountDid = prevState.currentBundleState.did
       if (accountDid) {
-        void persistedSession
+        void store
-          .runWithCredentialLock({
+          .dispatch({type: 'logged-out-current-account', accountDid}, [
-            operation: () =>
-              store.dispatch({type: 'logged-out-current-account', accountDid}, [
             {type: 'logout', accountDid},
-              ]),
+          ])
-          })
           .catch(() => {})
       }
       ax.metric(
@@ -489,17 +475,14 @@ export function Provider({children}: React.PropsWithChildren<{}>) {
             .accounts.map(account => account.did),
         ]),
       ]
-      void persistedSession
+      void store
-        .runWithCredentialLock({
+        .dispatch(
-          operation: () =>
-            store.dispatch(
           {type: 'logged-out-every-account'},
           accountDids.map(accountDid => ({
             type: 'logout' as const,
             accountDid,
           })),
-            ),
+        )
-        })
         .catch(() => {})
       ax.metric(
         'account:loggedOut',
@@ -556,9 +539,7 @@ export function Provider({children}: React.PropsWithChildren<{}>) {
         disposeBundle(bundle)
         return
       }
-      const committedSession = await persistedSession.runWithCredentialLock({
+      const committedSession = await store.dispatch(
-        operation: () =>
-          store.dispatch(
         {
           type: 'switched-to-account',
           newBundle: bundle,
@@ -572,8 +553,7 @@ export function Provider({children}: React.PropsWithChildren<{}>) {
             resultRefreshJwt: account.refreshJwt,
           },
         ],
-          ),
+      )
-      })
       const committedAccount = committedSession?.accounts.find(
         candidate => candidate.did === account.did,
       )
@@ -625,9 +605,7 @@ export function Provider({children}: React.PropsWithChildren<{}>) {
     /* getSession targets the PDS; only the persisted account fields are patched. */
     const data = await bundle.pdsClient.call(com.atproto.server.getSession, {})
     if (signal.aborted) return
-    await persistedSession.runWithCredentialLock({
+    await store.dispatch({
-      operation: () =>
-        store.dispatch({
       type: 'partial-refresh-session',
       /*
        * Read the did off the response rather than the session: the bundle may
@@ -639,7 +617,6 @@ export function Provider({children}: React.PropsWithChildren<{}>) {
         emailConfirmed: data.emailConfirmed,
         emailAuthFactor: data.emailAuthFactor,
       },
-        }),
     })
   }, [store, cancelPendingTask])
 
@@ -711,17 +688,14 @@ export function Provider({children}: React.PropsWithChildren<{}>) {
         account: redactAccount(account),
       })
       cancelPendingTask()
-      void persistedSession
+      void store
-        .runWithCredentialLock({
+        .dispatch(
-          operation: () =>
-            store.dispatch(
           {
             type: 'removed-account',
             accountDid: account.did,
           },
           [{type: 'remove', accountDid: account.did}],
-            ),
+        )
-        })
         .catch(() => {})
       addSessionDebugLog({
         type: 'method:end',