diff --git a/src/state/persisted/index.ts b/src/state/persisted/index.ts index 8c043a3420..0659e7817b 100644 --- a/src/state/persisted/index.ts +++ b/src/state/persisted/index.ts @@ -31,6 +31,18 @@ export function get(key: K): Schema[K] { } get satisfies PersistedApi['get'] +/** + * Native is single-instance: there is no other tab that could have written + * newer data behind our back, so the in-memory `_state` is already the truth + * and a synchronous fresh read is impossible anyway (AsyncStorage is async). + * This mirrors {@link get}; the web implementation is the one that actually + * re-reads the store. + */ +export function readLatest(key: K): Schema[K] { + return _state[key] +} +readLatest satisfies PersistedApi['readLatest'] + export async function write( key: K, value: Schema[K], diff --git a/src/state/persisted/index.web.ts b/src/state/persisted/index.web.ts index 35e796810d..a4c5d1b247 100644 --- a/src/state/persisted/index.web.ts +++ b/src/state/persisted/index.web.ts @@ -39,6 +39,29 @@ export function get(key: K): Schema[K] { } get satisfies PersistedApi['get'] +/** + * Force a fresh synchronous re-read of localStorage and return the requested + * key from it, WITHOUT adopting it as `_state`. + * + * This exists for the cross-tab expiry-rescue path. A frozen tab may not have + * processed a queued broadcast yet, so {@link get} (and persisted's in-memory + * `_state`) can be stale even though another tab already wrote healthy tokens + * to storage. Reading through storage directly here is the only way to see the + * true cross-tab-latest tokens on web. + * + * Crucially we do NOT adopt into `_state`. {@link readFromStorage} memoizes by + * raw string and returns the same object reference for unchanged data, so + * adopting here would make the later queued broadcast/storage event for that + * same write see `next === _state` and suppress its listener notification - + * leaving non-current-account changes (removals, other tokens, metadata) stale + * indefinitely. Leaving `_state` alone lets that queued event still fire. + */ +export function readLatest(key: K): Schema[K] { + const next = readFromStorage() + return next?.[key] ?? _state[key] +} +readLatest satisfies PersistedApi['readLatest'] + // eslint-disable-next-line @typescript-eslint/require-await export async function write( key: K, diff --git a/src/state/persisted/types.ts b/src/state/persisted/types.ts index d1fdfc26cb..4b5a29d938 100644 --- a/src/state/persisted/types.ts +++ b/src/state/persisted/types.ts @@ -3,6 +3,15 @@ import {type Schema} from './schema' export type PersistedApi = { init(): Promise get(key: K): Schema[K] + /** + * Like {@link get}, but on web forces a fresh synchronous re-read of the + * backing store before returning (without adopting it as the in-memory + * state). This exists for the cross-tab expiry-rescue path: a frozen tab may + * not have processed a queued broadcast yet, so {@link get} can be stale + * while another tab has already written healthy tokens to storage. On native + * it is identical to {@link get} (single-instance, no other writer). + */ + readLatest(key: K): Schema[K] write(key: K, value: Schema[K]): Promise onUpdate( key: K, diff --git a/src/state/session/__tests__/session-test.ts b/src/state/session/__tests__/session-test.ts index 6959398cec..0ace358b39 100644 --- a/src/state/session/__tests__/session-test.ts +++ b/src/state/session/__tests__/session-test.ts @@ -1098,7 +1098,7 @@ describe('session', () => { expect(state.accounts[0].accessJwt).toBe('alice-access-jwt-3') }) - it('accepts updates from a stale agent', () => { + it('ignores updates from a stale agent bundle', () => { let state = getInitialState([]) const aliceBundle = makeBundle('https://alice.com') @@ -1133,6 +1133,13 @@ describe('session', () => { expect(state.accounts.length).toBe(2) expect(state.currentAgentState.did).toBe('bob-did') + /* + * An 'update' from the stale (background) Alice bundle is now dropped + * ENTIRELY - identical state object, no token write. A refresh completing + * after switching away must not resurrect fresh tokens into the + * switched-away account entry. + */ + const beforeStaleUpdate = state state = run(state, [ { type: 'received-agent-event', @@ -1151,59 +1158,16 @@ describe('session', () => { sessionEvent: 'update', }, ]) - expect(state.accounts.length).toBe(2) + expect(beforeStaleUpdate === state).toBe(true) expect(state.accounts[1].did).toBe('alice-did') - // Should update Alice's tokens because otherwise they'll be stale. - expect(state.accounts[1].handle).toBe('alice-updated.test') - expect(state.accounts[1].accessJwt).toBe('alice-access-jwt-2') - expect(state.accounts[1].refreshJwt).toBe('alice-refresh-jwt-2') - expect(printState(state)).toMatchInlineSnapshot(` - { - "accounts": [ - { - "accessJwt": "bob-access-jwt-1", - "active": true, - "did": "bob-did", - "email": undefined, - "emailAuthFactor": false, - "emailConfirmed": false, - "handle": "bob.test", - "isSelfHosted": true, - "pdsUrl": undefined, - "refreshJwt": "bob-refresh-jwt-1", - "service": "https://bob.com/", - "signupQueued": false, - "status": undefined, - }, - { - "accessJwt": "alice-access-jwt-2", - "active": true, - "did": "alice-did", - "email": "alice@foo.bar", - "emailAuthFactor": false, - "emailConfirmed": false, - "handle": "alice-updated.test", - "isSelfHosted": true, - "pdsUrl": undefined, - "refreshJwt": "alice-refresh-jwt-2", - "service": "https://alice.com/", - "signupQueued": false, - "status": undefined, - }, - ], - "currentAgentState": { - "agent": { - "service": "https://bob.com/", - }, - "did": "bob-did", - }, - "needsPersist": true, - } - `) + // Alice's stored tokens are untouched (the stale update did not land). + expect(state.accounts[1].handle).toBe('alice.test') + expect(state.accounts[1].accessJwt).toBe('alice-access-jwt-1') + expect(state.accounts[1].refreshJwt).toBe('alice-refresh-jwt-1') state = run(state, [ { - // Update Bob. + // Update Bob (the current bundle) - this still applies. type: 'received-agent-event', accountDid: 'bob-did', agent: bobBundle, @@ -1242,16 +1206,16 @@ describe('session', () => { "status": undefined, }, { - "accessJwt": "alice-access-jwt-2", + "accessJwt": "alice-access-jwt-1", "active": true, "did": "alice-did", - "email": "alice@foo.bar", + "email": undefined, "emailAuthFactor": false, "emailConfirmed": false, - "handle": "alice-updated.test", + "handle": "alice.test", "isSelfHosted": true, "pdsUrl": undefined, - "refreshJwt": "alice-refresh-jwt-2", + "refreshJwt": "alice-refresh-jwt-1", "service": "https://alice.com/", "signupQueued": false, "status": undefined, @@ -1267,7 +1231,7 @@ describe('session', () => { } `) - // Ignore other events for inactive agent. + // Ignore other events for the inactive agent too (network-error, expired). const lastState = state state = run(state, [ { @@ -1291,6 +1255,100 @@ describe('session', () => { expect(lastState === state).toBe(true) }) + it('drops an update from a stale bundle even when its account entry still exists (no resurrection)', () => { + let state = getInitialState([]) + + const aliceBundle = makeBundle('https://alice.com') + state = run(state, [ + { + type: 'switched-to-account', + newAgent: aliceBundle, + newAccount: makeAccount('https://alice.com', { + active: true, + did: 'alice-did', + handle: 'alice.test', + accessJwt: 'alice-access-jwt-1', + refreshJwt: 'alice-refresh-jwt-1', + }), + }, + ]) + expect(state.currentAgentState.did).toBe('alice-did') + + // Alice logs out: her account entry stays, but tokens are cleared and the + // current bundle becomes the public (logged-out) bundle. + state = run(state, [{type: 'logged-out-current-account'}]) + expect(state.currentAgentState.did).toBe(undefined) + expect(state.accounts[0].did).toBe('alice-did') + expect(state.accounts[0].accessJwt).toBe(undefined) + expect(state.accounts[0].refreshJwt).toBe(undefined) + + /* + * A refresh that was already in flight on the (now stale) Alice bundle + * completes and delivers fresh tokens. It must NOT resurrect them into the + * soft-logged-out account entry - the bundle no longer matches the current + * (public) bundle, so the event is dropped entirely. + */ + const afterLogout = state + state = run(state, [ + { + type: 'received-agent-event', + accountDid: 'alice-did', + agent: aliceBundle, + refreshedAccount: makeAccount('https://alice.com', { + active: true, + did: 'alice-did', + handle: 'alice.test', + accessJwt: 'alice-access-jwt-2', + refreshJwt: 'alice-refresh-jwt-2', + }), + sessionEvent: 'update', + }, + ]) + expect(afterLogout === state).toBe(true) + expect(state.accounts[0].accessJwt).toBe(undefined) + expect(state.accounts[0].refreshJwt).toBe(undefined) + expect(state.currentAgentState.did).toBe(undefined) + }) + + it('applies an update from the current bundle', () => { + let state = getInitialState([]) + + const aliceBundle = makeBundle('https://alice.com') + state = run(state, [ + { + type: 'switched-to-account', + newAgent: aliceBundle, + newAccount: makeAccount('https://alice.com', { + active: true, + did: 'alice-did', + handle: 'alice.test', + accessJwt: 'alice-access-jwt-1', + refreshJwt: 'alice-refresh-jwt-1', + }), + }, + ]) + + state = run(state, [ + { + type: 'received-agent-event', + accountDid: 'alice-did', + agent: aliceBundle, + refreshedAccount: makeAccount('https://alice.com', { + active: true, + did: 'alice-did', + handle: 'alice.test', + accessJwt: 'alice-access-jwt-2', + refreshJwt: 'alice-refresh-jwt-2', + }), + sessionEvent: 'update', + }, + ]) + // The current bundle's update lands and rotates the stored tokens. + expect(state.accounts[0].accessJwt).toBe('alice-access-jwt-2') + expect(state.accounts[0].refreshJwt).toBe('alice-refresh-jwt-2') + expect(state.currentAgentState.did).toBe('alice-did') + }) + it('ignores updates from a removed agent', () => { let state = getInitialState([]) diff --git a/src/state/session/reducer.ts b/src/state/session/reducer.ts index 46f71c5e6e..7ac99c7d9b 100644 --- a/src/state/session/reducer.ts +++ b/src/state/session/reducer.ts @@ -6,11 +6,10 @@ import {type SessionAccount} from './types' import {createTemporaryClientsAndResume} from './util' /* - * A hack so that the reducer can't read anything from the session bundle. From - * the reducer's point of view it is a completely opaque object; the only field - * it ever reads is `service` (a URL), used for logging/snapshots. The provider - * stores the full `SessionBundle` here, but the reducer's static type only sees - * `service` (structural: the bundle has more, the reducer sees less). + * A hack so the reducer can't read anything from the session bundle. The + * provider stores the full `SessionBundle` here, but the reducer's static type + * only sees `service` (a URL, used for logging/snapshots) - structurally the + * bundle has more, the reducer sees less. */ type OpaqueSessionBundle = { readonly service: URL @@ -42,11 +41,17 @@ export type Action = } | { /* - * Same-did cross-tab sync. `PasswordSession` cannot be patched in place, - * so the provider builds a fresh bundle from the synced tokens (no - * network - the leader tab already refreshed) and swaps it in, keeping - * the current did and replacing the matching account entry. Does not - * persist (synced from another tab, avoid write cycles). + * Swap the current bundle in place, keeping the current did and replacing + * the matching account entry, without persisting (avoid write cycles). + * `PasswordSession` cannot be patched in place, so the provider rebuilds a + * fresh bundle from a set of tokens and swaps it in. Two producers: + * + * - Same-did cross-tab sync: the leader tab refreshed and broadcast the + * new tokens; this tab rebuilds from them (no network). + * - Expiry rescue: the current bundle's refresh token expired, but a + * newer generation for the same did is known (from reducer state or a + * fresh persisted re-read), so the provider rebuilds from that newer + * generation instead of logging out (see onSessionChange in index.tsx). */ type: 'replaced-current-bundle' newAgent: OpaqueSessionBundle @@ -92,13 +97,24 @@ let reducer = (state: State, action: Action): State => { switch (action.type) { case 'received-agent-event': { const {agent, accountDid, refreshedAccount, sessionEvent} = action - if ( - refreshedAccount === undefined && - agent !== state.currentAgentState.agent - ) { - // If the session got cleared out (e.g. due to expiry or network error) but - // this account isn't the active one, don't clear it out at this time. - // This way, if the problem is transient, it'll work on next resume. + if (agent !== state.currentAgentState.agent) { + /* + * Any event from a bundle that is not the current one is dropped + * entirely, in BOTH directions: + * + * - A clear (expiry/network-error, refreshedAccount === undefined) from + * a stale background bundle must not log the current user out. If the + * problem is transient, it works on the next resume. + * - An update (refreshedAccount present) from a stale bundle must not + * resurrect tokens: a refresh that completes after this bundle was + * logged out / switched away from would otherwise write fresh tokens + * back into a soft-logged-out (or switched-away) account entry. + * + * Trade-off: a background bundle's in-flight refresh that lands inside + * the disposal window now has its (already server-side-rotated) tokens + * discarded. The stored generation stays valid within the PDS 2h grace + * window, so this is strictly better than the resurrection bug. + */ return state } if (sessionEvent === 'network-error') { @@ -261,12 +277,9 @@ let reducer = (state: State, action: Action): State => { const {accountDid, patch} = action /* - * Previously this also mutated `agent.session.emailConfirmed/ - * emailAuthFactor` in place. `PasswordSession` has no public session - * setter and mutating its returned object is fragile, so we now patch - * only the account entry. Consumers that read these fields - * (useAccountEmailState) read from `currentAccount` instead of - * `agent.session` (see phase-2 design doc section 5). + * Patch only the account entry: `PasswordSession` has no public session + * setter, and consumers that read these fields (useAccountEmailState) + * read from `currentAccount` rather than the session. */ return { ...state, diff --git a/src/state/session/types.ts b/src/state/session/types.ts index d83d3de4e6..455f29e381 100644 --- a/src/state/session/types.ts +++ b/src/state/session/types.ts @@ -44,22 +44,21 @@ export type SessionApiContext = { ) => Promise removeAccount: (account: SessionAccount) => void /** - * Calls `getSession` and updates select fields on the current account and - * `BskyAgent`. This is an alternative to `resumeSession`, which updates - * current account/agent using the `persistSessionHandler`, but is more load - * bearing. This patches in updates without causing any side effects via - * `persistSessionHandler`. + * Fetches `com.atproto.server.getSession` through the active account's PDS + * client and patches the reducer's stored account entry with the returned + * `emailConfirmed`/`emailAuthFactor` fields. Unlike `refreshSession`, this + * does not rotate tokens, touch the `PasswordSession`, or fire session-change + * hooks - it only refreshes those email-state fields on the current account. */ partialRefreshSession: () => Promise /** * Force a full session refresh (re-runs `com.atproto.server.refreshSession` - * plus `getSession`) and return the refreshed account snapshot, or - * `undefined` when logged out. + * plus `getSession`) and return the refreshed account snapshot, or `undefined` + * when logged out. * - * The refresh routes through the session's own `refresh()`, whose success - * hook propagates the updated account into state; the returned snapshot lets - * callers read post-refresh fields synchronously without waiting on the - * (async) reducer update. Rejections propagate to the caller. + * The session's success hook propagates the updated account into state; the + * returned snapshot lets callers read post-refresh fields synchronously + * without waiting on the (async) reducer update. Rejections propagate. */ refreshSession: () => Promise }