diff --git a/plans/current-review.md b/plans/current-review.md index 2ae5bfbbfe..6732eed998 100644 --- a/plans/current-review.md +++ b/plans/current-review.md @@ -91,6 +91,8 @@ The pattern existed on `main`, where the resume factory was its main rejection s **Severity: Low** +**Status: Resolved.** Login, account creation, and partial session metadata refresh now use a success-with-warning policy after reducer commit. Persistence rejection is converted into a `logger.warn` with `safeMessage`, while the method resolves and continues its success path. Focused tests cover all three operations. + `login`, `createAccount`, and `partialRefreshSession` synchronously commit reducer state and then await persistence: - `index.tsx:353-370`: login. @@ -103,10 +105,7 @@ On `main`, persistence was fire-and-forget. Logout and removal currently make th This requires genuinely broken storage, such as quota exhaustion or a browser security error. -**Recommendation:** Choose and document one policy: - -1. swallow and log, matching logout/removal; or -2. deliberately reject, with callers treating "session established but durability failed" as success with a warning rather than a failed login. +**Resolution:** Swallow and warn after the reducer commit. This preserves the successful in-memory operation without misreporting a completed login or account creation as failed; the warning records that durability was lost. ## Intended but consequential behavior @@ -156,11 +155,10 @@ Add a `refreshSession` test with a rejecting `writeSession` to pin the intended ## Remaining recommended order -1. Decide and document the B4 persistence-failure policy for login-shaped methods. -2. Add a `refreshSession` test with rejecting `writeSession`. +1. 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 is closed; B4 remains as the final lower-severity error-policy issue. +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 are closed. The remaining work is a focused test pinning explicit refresh behavior when `writeSession` rejects. diff --git a/plans/versioned-localstorage-sessions.md b/plans/versioned-localstorage-sessions.md index 27c57cd3fd..1f1cccc0c4 100644 --- a/plans/versioned-localstorage-sessions.md +++ b/plans/versioned-localstorage-sessions.md @@ -238,6 +238,8 @@ Tab A action: broadcast update notification Broadcasting after a failed write would tell Tab B to reread localStorage while it still contains `(7, A)`, spreading the stale generation instead of the new one. +Login, account creation, and partial session metadata refresh use a success-with-warning policy once their reducer state has committed. If persistence then fails, the method logs a warning with the error and resolves successfully instead of reporting that the already-completed login or account creation failed. The in-memory session remains usable, but may not survive a reload. The underlying write still throws, and the persisted cache and other tabs are not updated. + ### Edge case: a failed write leaves a missing lineage link If the process survives a failed write, its live `PasswordSession` may advance while authoritative storage remains behind: diff --git a/src/state/session/__tests__/provider-abort-test.tsx b/src/state/session/__tests__/provider-abort-test.tsx index 3de2bb7c66..5f7a2a6516 100644 --- a/src/state/session/__tests__/provider-abort-test.tsx +++ b/src/state/session/__tests__/provider-abort-test.tsx @@ -1,6 +1,9 @@ import {beforeEach, describe, expect, it, jest} from '@jest/globals' import {act, render} from '@testing-library/react-native' +let mockWriteSessionError: Error | undefined +const mockAnalyticsLoggerWarn = jest.fn() + /* * The provider pulls the whole app shell in through `#/state/util` and the * account factories. These mocks cut the tree back to the session lifecycle @@ -16,7 +19,9 @@ jest.mock('#/state/persisted', () => { get: () => defaults.session, readLatest: () => defaults.session, writeSession: ({nextSession}: {nextSession: typeof defaults.session}) => - Promise.resolve(nextSession), + mockWriteSessionError + ? Promise.reject(mockWriteSessionError) + : Promise.resolve(nextSession), onUpdate: () => () => {}, } }) @@ -26,7 +31,14 @@ jest.mock('#/components/dialogs/Context', () => ({ })) jest.mock('#/analytics', () => ({ AnalyticsContext: ({children}: {children: React.ReactNode}) => children, - useAnalyticsBase: () => ({metric() {}, logger: {debug() {}, error() {}}}), + useAnalyticsBase: () => ({ + metric() {}, + logger: { + debug() {}, + error() {}, + warn: (...args: unknown[]) => mockAnalyticsLoggerWarn(...args), + }, + }), utils: {accountToSessionMetadata: () => ({}), useMeta: () => undefined}, })) jest.mock('#/state/shell/onboarding', () => ({ @@ -81,6 +93,14 @@ function renderProvider(): SessionApiContext { return api } +beforeEach(() => { + mockLogin.mockReset() + mockCreateAccount.mockReset() + mockDisposeBundle.mockReset() + mockWriteSessionError = undefined + mockAnalyticsLoggerWarn.mockReset() +}) + /* * Every factory returns an ARMED bundle, so a call whose result is thrown away * because a newer call superseded it must dispose that bundle. Leaving it armed @@ -88,18 +108,6 @@ function renderProvider(): SessionApiContext { * for an account the app is no longer tracking. */ describe('superseded session tasks dispose their bundle', () => { - /* - * Without this, a recorded call from an earlier test satisfies a later - * assertion. The tagged bundles below are the other half of that guard: two - * `{}` literals are structurally equal, so `toHaveBeenCalledWith` could not - * tell one test's bundle from the other's even within a cleared mock. - */ - beforeEach(() => { - mockLogin.mockReset() - mockCreateAccount.mockReset() - mockDisposeBundle.mockReset() - }) - it('disposes the bundle of an aborted login', async () => { const bundle = {tag: 'login-bundle'} as never let resolveLogin!: (value: unknown) => void @@ -145,3 +153,74 @@ describe('superseded session tasks dispose their bundle', () => { expect(mockDisposeBundle).toHaveBeenCalledWith(bundle) }) }) + +describe('persistence failures after an in-memory commit', () => { + const account = { + did: 'did:plc:example', + handle: 'alice.test', + service: 'https://bsky.social/', + accessJwt: 'access-jwt', + refreshJwt: 'refresh-jwt', + } + + it('treats login as successful and logs a warning', async () => { + const error = new Error('storage failed') + mockLogin.mockResolvedValueOnce({bundle: {}, account}) + mockWriteSessionError = error + const api = renderProvider() + + await act(async () => { + await api.login({} as never, 'LoginForm') + }) + + expect(mockAnalyticsLoggerWarn).toHaveBeenCalledWith( + 'Logged in but session persistence failed', + {safeMessage: error}, + ) + }) + + it('treats account creation as successful and logs a warning', async () => { + const error = new Error('storage failed') + mockCreateAccount.mockResolvedValueOnce({bundle: {}, account}) + mockWriteSessionError = error + const api = renderProvider() + + await act(async () => { + await api.createAccount({} as never, {} as never) + }) + + expect(mockAnalyticsLoggerWarn).toHaveBeenCalledWith( + 'Account created but session persistence failed', + {safeMessage: error}, + ) + }) + + it('keeps refreshed metadata and logs a warning', async () => { + const error = new Error('storage failed') + const bundle = { + pdsClient: { + call: () => + Promise.resolve({ + did: account.did, + emailConfirmed: true, + emailAuthFactor: true, + }), + }, + } + mockLogin.mockResolvedValueOnce({bundle, account}) + const api = renderProvider() + await act(async () => { + await api.login({} as never, 'LoginForm') + }) + mockWriteSessionError = error + + await act(async () => { + await api.partialRefreshSession() + }) + + expect(mockAnalyticsLoggerWarn).toHaveBeenCalledWith( + 'Session metadata updated but persistence failed', + {safeMessage: error}, + ) + }) +}) diff --git a/src/state/session/index.tsx b/src/state/session/index.tsx index a8cfe6bff1..c756f8240d 100644 --- a/src/state/session/index.tsx +++ b/src/state/session/index.tsx @@ -353,20 +353,26 @@ export function Provider({children}: React.PropsWithChildren<{}>) { disposeBundle(bundle) return } - await store.dispatch( - { - type: 'switched-to-account', - newBundle: bundle, - newAccount: account, - }, - [ + await store + .dispatch( { - type: 'login', - accountDid: account.did, - resultRefreshJwt: account.refreshJwt, + type: 'switched-to-account', + newBundle: bundle, + newAccount: account, }, - ], - ) + [ + { + type: 'login', + accountDid: account.did, + resultRefreshJwt: account.refreshJwt, + }, + ], + ) + .catch(error => { + ax.logger.warn('Account created but session persistence failed', { + safeMessage: error, + }) + }) ax.metric('account:create:success', metrics, { session: utils.accountToSessionMetadata(account), }) @@ -393,20 +399,26 @@ export function Provider({children}: React.PropsWithChildren<{}>) { disposeBundle(bundle) return } - await store.dispatch( - { - type: 'switched-to-account', - newBundle: bundle, - newAccount: account, - }, - [ + await store + .dispatch( { - type: 'login', - accountDid: account.did, - resultRefreshJwt: account.refreshJwt, + type: 'switched-to-account', + newBundle: bundle, + newAccount: account, }, - ], - ) + [ + { + type: 'login', + accountDid: account.did, + resultRefreshJwt: account.refreshJwt, + }, + ], + ) + .catch(error => { + ax.logger.warn('Logged in but session persistence failed', { + safeMessage: error, + }) + }) ax.metric( 'account:loggedIn', {logContext, withPassword: true}, @@ -615,20 +627,26 @@ export function Provider({children}: React.PropsWithChildren<{}>) { /* getSession targets the PDS; only the persisted account fields are patched. */ const data = await bundle.pdsClient.call(com.atproto.server.getSession, {}) if (signal.aborted) return - await store.dispatch({ - type: 'partial-refresh-session', - /* - * Read the did off the response rather than the session: the bundle may - * have been disposed while the request was in flight, and the live - * getters throw in that state. - */ - accountDid: data.did, - patch: { - emailConfirmed: data.emailConfirmed, - emailAuthFactor: data.emailAuthFactor, - }, - }) - }, [store, cancelPendingTask]) + await store + .dispatch({ + type: 'partial-refresh-session', + /* + * Read the did off the response rather than the session: the bundle may + * have been disposed while the request was in flight, and the live + * getters throw in that state. + */ + accountDid: data.did, + patch: { + emailConfirmed: data.emailConfirmed, + emailAuthFactor: data.emailAuthFactor, + }, + }) + .catch(error => { + ax.logger.warn('Session metadata updated but persistence failed', { + safeMessage: error, + }) + }) + }, [store, cancelPendingTask, ax]) /** * Rotate the session's tokens and hand back the resulting account snapshot.