diff --git a/plans/current-review.md b/plans/current-review.md index 9b0354cad2..877329579e 100644 --- a/plans/current-review.md +++ b/plans/current-review.md @@ -17,7 +17,9 @@ What changed relative to `main`: **Severity: Medium** -`resumeSession` (`index.tsx:509-591`) checks `signal.aborted` and the account entry before dispatching `switched-to-account`. After `await store.dispatch(...)` at line 542, however, it performs follow-up dispatches without rechecking either: +**Status: Resolved.** `resumeSession` now rechecks both its abort signal and bundle identity after the awaited persistence commit. Focused tests cover logout and same-account cross-tab bundle replacement while that write is pending. + +`resumeSession` (`index.tsx:509-591`) originally checked `signal.aborted` and the account entry before dispatching `switched-to-account`. After `await store.dispatch(...)` at line 542, however, it performed follow-up dispatches without rechecking either: - `index.tsx:560-566`: `synced-accounts` with the potentially stale `committedSession`. - `index.tsx:568-579`: `replaced-current-bundle` with a newly built, armed bundle. diff --git a/src/state/session/__tests__/provider-session-events-test.tsx b/src/state/session/__tests__/provider-session-events-test.tsx index e78c296d31..5a57868b48 100644 --- a/src/state/session/__tests__/provider-session-events-test.tsx +++ b/src/state/session/__tests__/provider-session-events-test.tsx @@ -24,10 +24,12 @@ const mockPersisted: {session: Schema['session']; latest: Schema['session']} = { * exists to catch. */ const mockPersistedListeners: ((value: Schema['session']) => void)[] = [] +let mockWriteSessionGate: Promise | undefined +let mockWriteSessionStarted: (() => void) | undefined jest.mock('#/state/persisted', () => ({ get: () => mockPersisted.session, readLatest: () => mockPersisted.latest, - writeSession: ({ + writeSession: async ({ nextSession, credentialMutations, currentAccountDid, @@ -36,6 +38,10 @@ jest.mock('#/state/persisted', () => ({ credentialMutations: import('#/state/persisted').SessionCredentialMutation[] currentAccountDid?: string }) => { + const started = mockWriteSessionStarted + mockWriteSessionStarted = undefined + started?.() + await mockWriteSessionGate const { applySessionUpdate, }: typeof import('#/state/persisted/session-merge') = require('#/state/persisted/session-merge') @@ -47,7 +53,7 @@ jest.mock('#/state/persisted', () => ({ }) mockPersisted.session = committed mockPersisted.latest = committed - return Promise.resolve(committed) + return committed }, onUpdate: (_key: 'session', callback: (value: Schema['session']) => void) => { mockPersistedListeners.push(callback) @@ -285,6 +291,8 @@ beforeEach(() => { mockPersisted.session = {accounts: [], currentAccount: undefined} mockPersisted.latest = {accounts: [], currentAccount: undefined} mockPersistedListeners.length = 0 + mockWriteSessionGate = undefined + mockWriteSessionStarted = undefined mockRebuilds.length = 0 mockLogin.mockReset() mockResume.mockReset() @@ -636,4 +644,97 @@ describe('cross-tab sync', () => { expect(mockDisposeBundle).toHaveBeenCalledWith(resumedBundle) expect(mockRebuilds.length).toBe(0) }) + + it('does not reconcile a resume after logout while its write is pending', async () => { + const account = makeAccount() + const bundle = makeBundle(account) + const {api, currentAccount, hasSession} = await renderLoggedIn( + account, + bundle, + ) + const resumedBundle = makeBundle(account) + mockResume.mockResolvedValueOnce({bundle: resumedBundle, account}) + + let releaseWrite!: () => void + mockWriteSessionGate = new Promise(resolve => { + releaseWrite = resolve + }) + const writeStarted = new Promise(resolve => { + mockWriteSessionStarted = resolve + }) + const pending = api.resumeSession(account) + await act(async () => { + await writeStarted + }) + + /* + * A newer persisted generation makes the pending resume want to rebuild + * after its write, while logout supersedes the bundle in reducer state. + */ + const rotated = makeAccount({ + accessJwt: 'access-jwt-2', + refreshJwt: 'refresh-jwt-2', + }) + mockPersisted.session = mockPersisted.latest = { + accounts: [rotated], + currentAccount: rotated, + credentialStates: { + [DID]: { + credentialVersion: 2, + refreshJti: 'refresh-jwt-2', + status: 'active', + }, + }, + } + act(() => { + api.logoutCurrentAccount({} as never) + }) + + await act(async () => { + releaseWrite() + await pending + }) + + expect(hasSession()).toBe(false) + expect(currentAccount()).toBeUndefined() + expect(mockRebuilds.length).toBe(0) + }) + + it('does not reconcile a resume after a synced bundle supersedes it', async () => { + const account = makeAccount() + const bundle = makeBundle(account) + const {api, currentAccount} = await renderLoggedIn(account, bundle) + const resumedBundle = makeBundle(account) + mockResume.mockResolvedValueOnce({bundle: resumedBundle, account}) + + let releaseWrite!: () => void + mockWriteSessionGate = new Promise(resolve => { + releaseWrite = resolve + }) + const writeStarted = new Promise(resolve => { + mockWriteSessionStarted = resolve + }) + const pending = api.resumeSession(account) + await act(async () => { + await writeStarted + }) + + const rotated = makeAccount({ + accessJwt: 'access-jwt-2', + refreshJwt: 'refresh-jwt-2', + }) + act(() => { + emitSynced({accounts: [rotated], currentAccount: rotated}) + }) + expect(mockRebuilds.length).toBe(1) + + await act(async () => { + releaseWrite() + await pending + }) + + expect(currentAccount()?.refreshJwt).toBe('refresh-jwt-2') + /* The stale resume must not replace the bundle selected by the sync. */ + expect(mockRebuilds.length).toBe(1) + }) }) diff --git a/src/state/session/index.tsx b/src/state/session/index.tsx index 5419fe8f45..9f37a81272 100644 --- a/src/state/session/index.tsx +++ b/src/state/session/index.tsx @@ -554,6 +554,16 @@ export function Provider({children}: React.PropsWithChildren<{}>) { }, ], ) + /* + * Persistence may have waited behind another operation. Do not reconcile + * its result after logout or a newer session task has replaced this bundle. + */ + if ( + signal.aborted || + store.getState().currentBundleState.bundle !== bundle + ) { + return + } const committedAccount = committedSession?.accounts.find( candidate => candidate.did === account.did, )