From 653c7113dc3c64effd90e129d10b822b9df36016 Mon Sep 17 00:00:00 2001 From: Eric Bailey Date: Mon, 31 Aug 2026 17:53:29 -0500 Subject: [PATCH] Handle cross-tab resume failures --- plans/current-review.md | 13 +++---- .../provider-session-events-test.tsx | 34 ++++++++++++++++++- src/state/session/index.tsx | 8 +++-- 3 files changed, 46 insertions(+), 9 deletions(-) diff --git a/plans/current-review.md b/plans/current-review.md index 15aa3a6ef2..2ae5bfbbfe 100644 --- a/plans/current-review.md +++ b/plans/current-review.md @@ -75,7 +75,9 @@ This is not a lock race; it is a false positive in the deliberate conditional me **Severity: Low** -At `index.tsx:740`, the cross-tab listener calls: +**Status: Resolved.** The detached cross-tab resume now catches and logs failures with `safeMessage`; a focused provider test covers a rejected session factory. + +At `index.tsx:740`, the cross-tab listener originally called: ```ts void resumeSession(syncedAccount) @@ -83,7 +85,7 @@ void resumeSession(syncedAccount) The pattern existed on `main`, where the resume factory was its main rejection source. This branch adds persistence rejection because `resumeSession` now awaits `store.dispatch`, which awaits `writeSession`. A storage failure can therefore become an unhandled promise rejection. -**Minimal fix:** Attach `.catch(...)` and log the failure. +**Resolution:** Attach `.catch(...)` and log the failure. ### B4 - Login-shaped methods reject after committing reducer state @@ -154,12 +156,11 @@ Add a `refreshSession` test with a rejecting `writeSession` to pin the intended ## Remaining recommended order -1. Fix B3 by catching and logging the cross-tab listener's `resumeSession` rejection. -2. Decide and document the B4 persistence-failure policy for login-shaped methods. -3. Add a `refreshSession` test with rejecting `writeSession`. +1. Decide and document the B4 persistence-failure policy for login-shaped methods. +2. Add a `refreshSession` test with rejecting `writeSession`. ## Verdict The core async design is sound. Serialization through `PasswordSession.#sessionPromise`, the per-bundle error channel, arm/kill lifecycle, reducer identity guards, and internally owned persistence lock compose correctly in the reviewed interleavings. -The genuine state-corruption hole from B1 is now closed. B2 is accepted, documented, instrumented, and covered as a conditional-merge tradeoff. B3 and B4 remain lower-severity error-policy issues. +The genuine state-corruption hole from B1 is now closed. B2 is accepted, documented, instrumented, and covered as a conditional-merge tradeoff. B3 is closed; B4 remains as the final lower-severity error-policy issue. diff --git a/src/state/session/__tests__/provider-session-events-test.tsx b/src/state/session/__tests__/provider-session-events-test.tsx index 5a57868b48..42ab863110 100644 --- a/src/state/session/__tests__/provider-session-events-test.tsx +++ b/src/state/session/__tests__/provider-session-events-test.tsx @@ -64,9 +64,16 @@ jest.mock('#/state/util', () => ({useCloseAllActiveElements: () => () => {}})) jest.mock('#/components/dialogs/Context', () => ({ useGlobalDialogsControlContext: () => ({signinDialogControl: {open() {}}}), })) +const mockAnalyticsLoggerError = jest.fn() jest.mock('#/analytics', () => ({ AnalyticsContext: ({children}: {children: React.ReactNode}) => children, - useAnalyticsBase: () => ({metric() {}, logger: {debug() {}, error() {}}}), + useAnalyticsBase: () => ({ + metric() {}, + logger: { + debug() {}, + error: (...args: unknown[]) => mockAnalyticsLoggerError(...args), + }, + }), utils: {accountToSessionMetadata: () => ({}), useMeta: () => undefined}, })) jest.mock('#/state/shell/onboarding', () => ({ @@ -145,6 +152,7 @@ import { import {type SessionApiContext} from '#/state/session/types' const DID = 'did:plc:example123' +const OTHER_DID = 'did:plc:other456' const SERVICE = 'https://bsky.social/' function makeAccount(overrides: Partial = {}): SessionAccount { @@ -299,6 +307,7 @@ beforeEach(() => { mockRebuild.mockClear() mockDisposeBundle.mockReset() mockEmitSessionDropped.mockReset() + mockAnalyticsLoggerError.mockReset() }) /* @@ -550,6 +559,29 @@ function emitSynced(session: Schema['session']) { * against the store having advanced underneath it. */ describe('cross-tab sync', () => { + it('logs a failed resume for an account selected by another tab', async () => { + const account = makeAccount() + const bundle = makeBundle(account) + await renderLoggedIn(account, bundle) + const otherAccount = makeAccount({ + did: OTHER_DID, + handle: 'bob.test', + }) + const error = new Error('resume failed') + mockResume.mockRejectedValueOnce(error) + + await act(async () => { + emitSynced({accounts: [otherAccount], currentAccount: otherAccount}) + await Promise.resolve() + await Promise.resolve() + }) + + expect(mockAnalyticsLoggerError).toHaveBeenCalledWith( + 'Failed to resume session from persisted update', + {safeMessage: error}, + ) + }) + it('short-circuits an update carrying the tokens the live session already has', async () => { const account = makeAccount() const bundle = makeBundle(account) diff --git a/src/state/session/index.tsx b/src/state/session/index.tsx index 9f37a81272..a8cfe6bff1 100644 --- a/src/state/session/index.tsx +++ b/src/state/session/index.tsx @@ -747,7 +747,11 @@ export function Provider({children}: React.PropsWithChildren<{}>) { if (syncedAccount && syncedAccount.refreshJwt) { if (syncedAccount.did !== state.currentBundleState.did) { // The leader refreshes before broadcasting, so followers receive fresh tokens. - void resumeSession(syncedAccount) + void resumeSession(syncedAccount).catch(error => { + ax.logger.error('Failed to resume session from persisted update', { + safeMessage: error, + }) + }) } else { /* * PasswordSession cannot be patched in place. Rebuild from the tokens @@ -806,7 +810,7 @@ export function Provider({children}: React.PropsWithChildren<{}>) { } } }) - }, [store, state, resumeSession, onSessionChange, cancelPendingTask]) + }, [store, state, resumeSession, onSessionChange, cancelPendingTask, ax]) const stateContext = useMemo( () => ({