diff --git a/plans/versioned-localstorage-sessions.md b/plans/versioned-localstorage-sessions.md index 6b65c1d285..5e609f62c9 100644 --- a/plans/versioned-localstorage-sessions.md +++ b/plans/versioned-localstorage-sessions.md @@ -270,7 +270,7 @@ Network refreshes do not run while holding a Web Lock. A refresh captures the ge const baseRefreshJti = getRefreshJti(session.refreshJwt) const refreshed = await refreshSession() -await navigator.locks.request(`bsky-session:${did}`, async () => { +await navigator.locks.request('bsky-persisted-storage', async () => { const latest = readAccountFromLocalStorage(did) // Commit only if latest is active and still has baseRefreshJti. }) @@ -278,19 +278,11 @@ await navigator.locks.request(`bsky-session:${did}`, async () => { 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. -All credential-changing commits use the same per-account lock: - -- successful refresh reconciliation; -- expiration; -- logout; -- account removal; and -- login replacing an existing account. - -Because all persisted values share one localStorage blob, every write also takes a root persisted-storage lock. The root lock prevents an unrelated preference write from racing the session read-modify-write; the per-account lock expresses credential ownership and gives account operations a consistent order. +All persisted values share one localStorage blob, so every write uses the same persisted-storage lock. This includes successful refresh reconciliation, expiration, logout, account removal, login replacement, and unrelated preference writes. One root lock is sufficient: it serializes the complete read-modify-write operation, so nesting per-account locks would add no coordination. Feature-detect the Web Locks API. If `navigator.locks.request` is unavailable, run the operation without a lock rather than failing startup or session operations. Generation-specific conditional commits still reject stale work in this fallback mode, but localStorage read-modify-write is not fully serialized across tabs. -If Tab A holds the locks, Tab B waits. Once Tab A writes and releases them, Tab B acquires them and rereads Tab A's new localStorage state before deciding what to commit. +If Tab A holds the lock, Tab B waits. Once Tab A writes and releases it, Tab B acquires it and rereads Tab A's new localStorage state before deciding what to commit. The complete refresh flow is: @@ -301,7 +293,7 @@ Capture base refresh jti Perform network refresh without a lock | v -Acquire root + per-account Web Locks +Acquire persisted-storage Web Lock | v Read authoritative localStorage state diff --git a/src/state/persisted/__tests__/session-lock.web-test.ts b/src/state/persisted/__tests__/storage-lock.web-test.ts similarity index 58% rename from src/state/persisted/__tests__/session-lock.web-test.ts rename to src/state/persisted/__tests__/storage-lock.web-test.ts index 6d9aafdc1d..bcbfa36bb0 100644 --- a/src/state/persisted/__tests__/session-lock.web-test.ts +++ b/src/state/persisted/__tests__/storage-lock.web-test.ts @@ -1,6 +1,6 @@ import {afterEach, describe, expect, it, jest} from '@jest/globals' -import {runWithSessionCredentialLock} from '../session-lock.web' +import {runWithPersistedStorageLock} from '../storage-lock.web' const originalNavigatorDescriptor = Object.getOwnPropertyDescriptor( globalThis, @@ -22,7 +22,7 @@ afterEach(() => { } }) -describe('session credential locks on unsupported browsers', () => { +describe('persisted storage lock on unsupported browsers', () => { it.each([ ['navigator is unavailable', undefined], ['navigator.locks is unavailable', {}], @@ -31,12 +31,23 @@ describe('session credential locks on unsupported browsers', () => { setNavigator(navigatorValue) const operation = jest.fn(() => 'result') - await expect( - runWithSessionCredentialLock({ - accountDids: ['did:plc:example'], - operation, - }), - ).resolves.toBe('result') + await expect(runWithPersistedStorageLock({operation})).resolves.toBe( + 'result', + ) expect(operation).toHaveBeenCalledTimes(1) }) + + it('uses one root lock when Web Locks are available', async () => { + const request = jest.fn( + (_name: string, operation: () => string | Promise) => + Promise.resolve(operation()), + ) + setNavigator({locks: {request}}) + + await expect( + runWithPersistedStorageLock({operation: () => 'result'}), + ).resolves.toBe('result') + expect(request).toHaveBeenCalledTimes(1) + expect(request.mock.calls[0]?.[0]).toBe('bsky-persisted-storage') + }) }) diff --git a/src/state/persisted/index.web.ts b/src/state/persisted/index.web.ts index 058a66c9d1..f0de527a16 100644 --- a/src/state/persisted/index.web.ts +++ b/src/state/persisted/index.web.ts @@ -8,11 +8,11 @@ import { tryParse, tryStringify, } from '#/state/persisted/schema' -import {runWithSessionCredentialLock} from './session-lock' import { applySessionUpdate, type SessionCredentialMutation, } from './session-merge' +import {runWithPersistedStorageLock} from './storage-lock' import {type PersistedApi} from './types' import {normalizeData} from './util' @@ -76,8 +76,7 @@ export function write( "Session state must be written through '#/state/persisted/session'", ) } - return runWithSessionCredentialLock({ - accountDids: [], + return runWithPersistedStorageLock({ operation: () => { const next = readFromStorage() if (next) { @@ -145,8 +144,7 @@ export function onUpdate( onUpdate satisfies PersistedApi['onUpdate'] export function clearStorage(): Promise { - return runWithSessionCredentialLock({ - accountDids: [], + return runWithPersistedStorageLock({ operation: () => { try { localStorage.removeItem(BSKY_STORAGE) diff --git a/src/state/persisted/session-lock.web.ts b/src/state/persisted/session-lock.web.ts deleted file mode 100644 index 153a95321b..0000000000 --- a/src/state/persisted/session-lock.web.ts +++ /dev/null @@ -1,45 +0,0 @@ -const PERSISTED_STORAGE_LOCK = 'bsky-persisted-storage' -const SESSION_LOCK_PREFIX = 'bsky-session:' - -export function runWithSessionCredentialLock({ - accountDids, - operation, -}: { - accountDids: string[] - operation: () => T | Promise -}): Promise { - const lockManager = getLockManager() - if (!lockManager) { - try { - return Promise.resolve(operation()) - } catch (error) { - return Promise.reject( - error instanceof Error - ? error - : new Error('Session credential operation failed', {cause: error}), - ) - } - } - - const accountLockNames = [...new Set(accountDids)] - .sort() - .map(did => `${SESSION_LOCK_PREFIX}${did}`) - /* All values share one localStorage blob, so every write also takes its lock. */ - const lockNames = [PERSISTED_STORAGE_LOCK, ...accountLockNames] - - const run = (index: number): Promise => { - const lockName = lockNames[index] - if (!lockName) return Promise.resolve(operation()) - return lockManager.request(lockName, () => run(index + 1)) - } - - return run(0) -} - -function getLockManager(): LockManager | undefined { - if (typeof navigator === 'undefined' || !('locks' in navigator)) { - return undefined - } - const lockManager = navigator.locks - return typeof lockManager?.request === 'function' ? lockManager : undefined -} diff --git a/src/state/persisted/session.ts b/src/state/persisted/session.ts index e72d82b8ef..1a957aa22c 100644 --- a/src/state/persisted/session.ts +++ b/src/state/persisted/session.ts @@ -1,9 +1,9 @@ import * as persisted from './index' import {type Schema} from './schema' -import {runWithSessionCredentialLock} from './session-lock' import {type SessionCredentialMutation} from './session-merge' export type {SessionCredentialMutation} from './session-merge' +export {runWithPersistedStorageLock as runWithCredentialLock} from './storage-lock' export function read(): Schema['session'] { return persisted.get('session') @@ -28,16 +28,6 @@ export function write({ }) } -export function runWithCredentialLock({ - accountDids, - operation, -}: { - accountDids: string[] - operation: () => T | Promise -}): Promise { - return runWithSessionCredentialLock({accountDids, operation}) -} - export function onUpdate( callback: (session: Schema['session']) => void, ): () => void { diff --git a/src/state/persisted/session-lock.ts b/src/state/persisted/storage-lock.ts similarity index 54% rename from src/state/persisted/session-lock.ts rename to src/state/persisted/storage-lock.ts index bd8ab5fc8d..1fb4ef3e65 100644 --- a/src/state/persisted/session-lock.ts +++ b/src/state/persisted/storage-lock.ts @@ -1,18 +1,15 @@ -export function runWithSessionCredentialLock({ - accountDids, +export function runWithPersistedStorageLock({ operation, }: { - accountDids: string[] operation: () => T | Promise }): Promise { - void accountDids try { return Promise.resolve(operation()) } catch (error) { return Promise.reject( error instanceof Error ? error - : new Error('Session credential operation failed', {cause: error}), + : new Error('Persisted storage operation failed', {cause: error}), ) } } diff --git a/src/state/persisted/storage-lock.web.ts b/src/state/persisted/storage-lock.web.ts new file mode 100644 index 0000000000..bfbb7e7526 --- /dev/null +++ b/src/state/persisted/storage-lock.web.ts @@ -0,0 +1,30 @@ +const PERSISTED_STORAGE_LOCK = 'bsky-persisted-storage' + +export function runWithPersistedStorageLock({ + operation, +}: { + operation: () => T | Promise +}): Promise { + const lockManager = getLockManager() + if (!lockManager) { + try { + return Promise.resolve(operation()) + } catch (error) { + return Promise.reject( + error instanceof Error + ? error + : new Error('Persisted storage operation failed', {cause: error}), + ) + } + } + + return lockManager.request(PERSISTED_STORAGE_LOCK, operation) +} + +function getLockManager(): LockManager | undefined { + if (typeof navigator === 'undefined' || !('locks' in navigator)) { + return undefined + } + const lockManager = navigator.locks + return typeof lockManager?.request === 'function' ? lockManager : undefined +} diff --git a/src/state/session/index.tsx b/src/state/session/index.tsx index 6c0fe2c20d..051137dbae 100644 --- a/src/state/session/index.tsx +++ b/src/state/session/index.tsx @@ -168,7 +168,6 @@ export function Provider({children}: React.PropsWithChildren<{}>) { : sessionData?.refreshJwt return persistedSession.runWithCredentialLock({ - accountDids: [accountDid], operation: async () => { /* * Only the live bundle may reset the expiry-rescue bookkeeping: a stale @@ -355,7 +354,6 @@ export function Provider({children}: React.PropsWithChildren<{}>) { return } await persistedSession.runWithCredentialLock({ - accountDids: [account.did], operation: () => store.dispatch( { @@ -399,7 +397,6 @@ export function Provider({children}: React.PropsWithChildren<{}>) { return } await persistedSession.runWithCredentialLock({ - accountDids: [account.did], operation: () => store.dispatch( { @@ -441,7 +438,6 @@ export function Provider({children}: React.PropsWithChildren<{}>) { if (accountDid) { void persistedSession .runWithCredentialLock({ - accountDids: [accountDid], operation: () => store.dispatch({type: 'logged-out-current-account', accountDid}, [ {type: 'logout', accountDid}, @@ -488,7 +484,6 @@ export function Provider({children}: React.PropsWithChildren<{}>) { ] void persistedSession .runWithCredentialLock({ - accountDids, operation: () => store.dispatch( {type: 'logged-out-every-account'}, @@ -555,7 +550,6 @@ export function Provider({children}: React.PropsWithChildren<{}>) { return } const committedSession = await persistedSession.runWithCredentialLock({ - accountDids: [account.did], operation: () => store.dispatch( { @@ -625,7 +619,6 @@ export function Provider({children}: React.PropsWithChildren<{}>) { const data = await bundle.pdsClient.call(com.atproto.server.getSession, {}) if (signal.aborted) return await persistedSession.runWithCredentialLock({ - accountDids: [data.did], operation: () => store.dispatch({ type: 'partial-refresh-session', @@ -713,7 +706,6 @@ export function Provider({children}: React.PropsWithChildren<{}>) { cancelPendingTask() void persistedSession .runWithCredentialLock({ - accountDids: [account.did], operation: () => store.dispatch( {