fix logout bug: expiry rescue, resume bail, stale-bundle event drops
Three production logout root causes (Logout issues Linear project): - a stale tab's ExpiredToken no longer clears newer tokens; the expiry path compares against the latest persisted generation and rescues from it (loop-guarded) instead of logging every tab out - follower-tab resumeSession disposes its bundle and bails when a logout or account switch raced it; aborted resumes now dispose too - the reducer drops ALL events from non-current bundles, so a refresh landing mid-logout can't resurrect cleared tokens; session-dropped toast gated on the bundle still being current Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -31,6 +31,18 @@ export function get<K extends keyof Schema>(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<K extends keyof Schema>(key: K): Schema[K] {
|
||||
return _state[key]
|
||||
}
|
||||
readLatest satisfies PersistedApi['readLatest']
|
||||
|
||||
export async function write<K extends keyof Schema>(
|
||||
key: K,
|
||||
value: Schema[K],
|
||||
|
||||
@@ -39,6 +39,29 @@ export function get<K extends keyof Schema>(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<K extends keyof Schema>(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<K extends keyof Schema>(
|
||||
key: K,
|
||||
|
||||
@@ -3,6 +3,15 @@ import {type Schema} from './schema'
|
||||
export type PersistedApi = {
|
||||
init(): Promise<void>
|
||||
get<K extends keyof Schema>(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<K extends keyof Schema>(key: K): Schema[K]
|
||||
write<K extends keyof Schema>(key: K, value: Schema[K]): Promise<void>
|
||||
onUpdate<K extends keyof Schema>(
|
||||
key: K,
|
||||
|
||||
@@ -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([])
|
||||
|
||||
|
||||
@@ -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,
|
||||
|
||||
+10
-11
@@ -44,22 +44,21 @@ export type SessionApiContext = {
|
||||
) => Promise<void>
|
||||
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<void>
|
||||
/**
|
||||
* 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<SessionAccount | undefined>
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user