diff --git a/plans/current-review.md b/plans/current-review.md deleted file mode 100644 index 6732eed998..0000000000 --- a/plans/current-review.md +++ /dev/null @@ -1,164 +0,0 @@ -# Review: async `OnSessionChange` on `session-sketch` - -Review performed by Fable against the current `session-sketch` working tree. All 117 focused session and persistence tests passed as a baseline. - -## Context - -What changed relative to `main`: - -- On `main`, `makeSessionHooks.dispatch` is synchronous and ignores `onSessionChange`'s result. `SessionStore.dispatch` persists fire-and-forget with `void persisted.write('session', ...)`. -- On this branch, `dispatch` is async and awaits `onSessionChange` (`session-core.ts:161-191`). That awaits `store.dispatch(...)`, which awaits `persisted.writeSession(...)` (`index.tsx:116-141`), and may then perform a second dispatch to rebuild against the committed generation (`index.tsx:296-327`). Failures land in a per-bundle `WeakMap` side channel (`session-core.ts:71,88-96,178-190`) consumed only by `refreshSession` (`index.tsx:655-660`). -- `PasswordSession` awaits its hooks inside the `#sessionPromise` chain. Its `refresh()` implementation awaits `onUpdated(newSession)` before assigning `#sessionData = newSession`. The app-level persistence pipeline therefore now sits inside the session promise that every `fetchHandler` call awaits. -- Commit `732cf8cdd` narrowed the lock: instead of holding a credential lock around the whole `onSessionChange` body, only `writeSession()`'s read-merge-write-broadcast holds the lock (`persisted/index.web.ts:111-135`, with a synchronous callback). Native uses its FIFO queue (`persisted/index.ts:120-131`). - -## Actual bugs - -### B1 - `resumeSession` post-persistence continuations lack identity and abort rechecks - -**Severity: Medium** - -**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. - -By comparison, `onSessionChange` guards equivalent branches with `store.getState().currentBundleState.bundle === bundle` (`index.tsx:304,319-320`), and the cross-tab listener uses a `shouldActivate` guard (`index.tsx:764-790`). `resumeSession` has neither. - -Concrete interleaving: - -1. `resumeSession(X)` passes its guards and dispatches `switched-to-account(X)`. The reducer commits `bundle_X` synchronously, write `W_X` is enqueued, and the function suspends awaiting `W_X`. -2. The user logs out. `logoutCurrentAccount` runs synchronously, calls `cancelPendingTask()` and aborts the resume signal, clears X's tokens, and switches `currentBundleState` to the public bundle. The UI is logged out. `W_logout` is queued behind `W_X`. -3. `W_X` resolves. Its merge ran before `W_logout`, so it did not see the tombstone. If another tab meanwhile rotated X, or the resume lost a generation race, `committedAccount.refreshJwt !== account.refreshJwt`, so the replacement branch runs. -4. `replaced-current-bundle` installs a live, armed X bundle while preserving the reducer's current DID, which is now `undefined`. It also restores X's credentials in the reducer account entry. - -The result is `hasSession === false` while `useAppviewClient()` and `usePdsClient()` expose X-authenticated clients: a logged-out UI can make authenticated requests. It heals only if the zombie bundle later emits another update and reconciliation encounters the tombstone. - -The sibling `synced-accounts` branch has a milder version: a stale committed snapshot can clobber an account that logged in during the await. Storage remains correct, but same-tab writes do not loop back through `onUpdate`, so the reducer may not immediately heal. - -The window is one storage write, but `resumeSession` is also driven by cross-tab notifications. This is new branch behavior because `main` had no asynchronous post-dispatch continuation here. - -**Minimal fix:** After the awaited dispatch, bail out if either: - -```ts -signal.aborted || -(store.getState().currentBundleState.bundle as unknown) !== (bundle as unknown) -``` - -If a replacement bundle has already been built before bailing, dispose it. This mirrors the discipline already used in `onSessionChange`. - -**Test:** Gate `writeSession` on a manually resolved promise, interleave `resumeSession` with `logoutCurrentAccount`, then assert that no stale `synced-accounts` or `replaced-current-bundle` continuation lands and any unused rebuilt bundle is disposed. - -### B2 - Persistence-failure divergence can downgrade credentials and force logout instead of self-healing - -**Severity: Medium-Low** - -**Status: Accepted, documented, and instrumented.** The missing-lineage-link failure mode and its recovery limits are now explicit in the main plan under “Edge case: a failed write leaves a missing lineage link.” Conditional persistence continues to prefer the authoritative stored generation, and logs when a rejected refresh result differs from that stored generation without recording token material. - -On `main`, a failed session write left storage stale, but the next dispatch rewrote the complete session unconditionally. Storage therefore healed on the next refresh. On this branch, the `jti`-chained merge (`session-merge.ts:171-200`) cannot distinguish another tab advancing the chain from this tab losing its own previous chain-link write. - -Concrete sequence: - -1. At `t0`, automatic refresh rotates generation 0 to generation 1. The reducer commits generation 1, but `writeSession` fails because of quota, private-mode behavior, or another storage error. The failure is recorded in the bundle `WeakMap`, but no automatic-refresh caller consumes it. Storage remains at generation 0 while the live session is generation 1. -2. At `t1`, the next automatic refresh rotates generation 1 to generation 2. Its mutation has generation 1 as the base. Authoritative storage still contains generation 0, so `applySessionUpdate` drops the mutation. -3. The committed account contains generation 0 credentials. Because those differ from the refreshed generation 2 credentials, `onSessionChange` rebuilds the live bundle from generation 0, actively discarding valid generation 2 credentials. -4. If generation 0 remains valid in the PDS refresh grace chain, a later refresh may self-heal at the cost of extra round trips. Outside that grace period, generation 0 expires and the app logs out, whereas `main` would have retained generation 2 in memory and healed storage during the next full write. - -This is not a lock race; it is a false positive in the deliberate conditional merge. The trigger is narrow: a storage-write failure while the app remains alive, followed by enough idle time for the old generation to leave the server grace window. - -**Decision:** Accept this as a documented tradeoff unless a reliable distinction can be made. A distinctive warning log now records when a refresh result is rejected and differs from the persisted generation; same-generation convergent aliases remain silent. The log includes only credential version and status. - -**Coverage:** `applySessionUpdate` now has a focused missing-lineage-link test, and the provider suite pins committed-generation mismatch reconciliation. - -### B3 - Cross-tab listener's `void resumeSession(...)` has new unhandled-rejection paths - -**Severity: Low** - -**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) -``` - -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. - -**Resolution:** Attach `.catch(...)` and log the failure. - -### B4 - Login-shaped methods reject after committing reducer state - -**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. -- `index.tsx:394-411`: account creation. -- `index.tsx:608-620`: partial session refresh. - -If `writeSession` rejects, the method rejects after the UI has already switched to the authenticated state. This can produce an error from a login form behind or over an already signed-in app. The `account:create:success` or `account:loggedIn` metric is also skipped even though the in-memory session exists. - -On `main`, persistence was fire-and-forget. Logout and removal currently make the opposite policy choice by explicitly swallowing persistence rejections with `.catch(() => {})`, so behavior is asymmetric. - -This requires genuinely broken storage, such as quota exhaustion or a browser security error. - -**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 - -### T1 - Requests wait for persistence during refresh - -`PasswordSession.fetchHandler` awaits `#sessionPromise` on entry. That promise now resolves only after: - -```text -network refresh - -> optional getSession backfill - -> onSessionChange - -> reducer - -> writeSession - -> possible committed-generation rebuild -``` - -Normally this adds only a small storage delay. In the worst case, wedged AsyncStorage or a large shared root blob stalls all in-flight and new requests, whereas `main` only blocked on refresh network work. - -The lock design itself remains sound: the web lock callback is synchronous, so no Web Lock is held across suspension and no nested-lock deadlock was found. This is latency coupling to storage health, not a locking defect. - -### T2 - `refreshSession` can reject after a successful server refresh - -Two cases: - -- **Persistence failure:** Tokens rotated server-side and entered reducer state, but `takeSessionChangeError` rethrows the persistence failure. Callers report failure for a live in-memory session. This is defensible because durability failed, but it is new and currently untested. -- **A cross-tab generation wins during refresh:** Reconciliation rebuilds the bundle, then the bundle-identity check throws `The session changed while it was being refreshed` even when the account is the same and healthy. The identity check is pre-existing, but committed-generation rebuilds make same-DID bundle replacement more common. - -Add a `refreshSession` test with a rejecting `writeSession` to pin the intended contract. - -## Verified non-issues - -- **WeakMap error-channel races:** Hook dispatches for one bundle are serialized by `PasswordSession.#sessionPromise`. Two concurrent explicit refreshes cannot realistically consume each other's errors. Per-bundle keys prevent cross-bundle contamination. -- **Hook failures bricking the session:** `dispatch` catches synchronous and asynchronous `onSessionChange` failures, preventing `#sessionPromise` from becoming permanently rejected. Existing tests cover this. -- **Concurrent refresh behavior:** Automatic refresh deduplication still works. Concurrent explicit refreshes can perform two serialized rotations, but that is pre-existing and harmless apart from extra work. -- **Expiry rescue:** Bundle/DID guards, the bounded failed-generation set, and reducer identity guards prevent stale bundles from logging out or mutating the active account. Rescue dispatches do not persist and therefore cannot self-deadlock. -- **Locking:** `writeSession`'s web lock callback contains no `await`; no reentrancy, frozen-tab lock retention, or lock-ordering defect was found. Native queue order is likewise FIFO. Logout-versus-refresh, rotation-versus-rotation, and tombstone-versus-late-update interleavings converge through the `jti` merge and reducer guards. -- **Disposal and switching:** Post-commit bundle disposal, reducer identity checks, and fetch kill switches prevent replaced bundles from consuming later refresh generations. Killed in-flight retries fail loudly rather than silently authenticating. -- **`baseRefreshJwt` capture:** Update events read the pre-commit live getter, while expired events use the payload. Both identify the correct base generation, including chained rotations. -- **Cross-tab listener closure:** The listener resubscribes as state changes, and `shouldActivate` rereads live store state. The worst stale result found was an unnecessary rebuild that activation then rejects. - -## Pre-existing issues, not regressions from this branch - -- A definitively dead `PasswordSession.refresh()` can reject with a raw `xrpcSafe` response object rather than an `Error`. -- `emitSessionDropped` runs before the logout dispatch. A throwing listener can skip reducer logout; the hook catches and logs it, but does not redrive logout. The ordering matches `main`. -- Cross-tab metadata clobbering and the no-Web-Locks lost-update fallback are already documented in `plans/versioned-localstorage-session-tangents.md`. -- `createTemporaryClientsAndResume` may rotate a token before logout without persistence hooks; this matches `main`. - -## Remaining recommended order - -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 and B4 are closed. The remaining work is a focused test pinning explicit refresh behavior when `writeSession` rejects. diff --git a/src/state/session/__tests__/provider-refresh-session-test.tsx b/src/state/session/__tests__/provider-refresh-session-test.tsx index 08300212f0..1a8860a6cd 100644 --- a/src/state/session/__tests__/provider-refresh-session-test.tsx +++ b/src/state/session/__tests__/provider-refresh-session-test.tsx @@ -4,6 +4,26 @@ import {act, render} from '@testing-library/react-native' import {type SessionAccount} from '../types' +let mockWriteSessionError: Error | undefined +const mockLoggerError = jest.fn() +jest.mock('#/logger', () => { + const logger = { + debug() {}, + info() {}, + log() {}, + warn() {}, + error: (...args: unknown[]) => mockLoggerError(...args), + } + return { + logger, + Logger: { + create: () => logger, + Context: new Proxy({}, {get: (_target, key) => String(key)}), + Level: {}, + }, + } +}) + /* * 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 @@ -19,7 +39,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: () => () => {}, } }) @@ -66,7 +88,11 @@ jest.mock('../create-account', () => ({ import {Provider, useSession, useSessionApi} from '#/state/session' import {type SessionApiContext} from '#/state/session/types' import {buildAppviewClient, buildChatClient, buildPdsClient} from '../clients' -import {type SessionBundle} from '../session-core' +import { + makeSessionHooks, + type OnSessionChange, + type SessionBundle, +} from '../session-core' import {sessionAccountToSessionData} from '../session-data' import { asFetch, @@ -85,17 +111,27 @@ import { function makeBundle( account: SessionAccount, fetchMock: MockFetch, + onSessionChange: OnSessionChange, ): SessionBundle { + let bundle!: SessionBundle + const hooks = makeSessionHooks({ + onSessionChange, + getBundle: () => bundle, + getDid: () => account.did, + }) const session = new PasswordSession(sessionAccountToSessionData(account), { + ...hooks, fetch: asFetch(fetchMock), }) - return { + bundle = { session, appviewClient: buildAppviewClient(session), pdsClient: buildPdsClient(session), chatClient: buildChatClient(session), service: new URL(account.service), } + hooks.arm() + return bundle } type Harness = { @@ -124,9 +160,12 @@ async function renderLoggedIn( account: SessionAccount, fetchMock: MockFetch, ): Promise { - const bundle = makeBundle(account, fetchMock) const harness = renderProvider() - mockLogin.mockResolvedValueOnce({bundle, account}) + mockLogin.mockImplementationOnce((...args: unknown[]) => { + const onSessionChange = args[1] as OnSessionChange + const bundle = makeBundle(account, fetchMock, onSessionChange) + return Promise.resolve({bundle, account}) + }) await act(async () => { await harness.api.login({} as never, 'LoginForm') }) @@ -135,6 +174,8 @@ async function renderLoggedIn( beforeEach(() => { mockLogin.mockReset() + mockLoggerError.mockReset() + mockWriteSessionError = undefined }) describe('refreshSession', () => { @@ -154,14 +195,14 @@ describe('refreshSession', () => { expect(refreshed?.handle).toBe(HANDLE) }) - it('exposes the fresh tokens before the store has caught up', async () => { + it("returns fresh tokens instead of relying on the caller's account snapshot", async () => { const fetchMock = makeMockFetch() const {api, currentAccount} = await renderLoggedIn(makeAccount(), fetchMock) /* * The point of the return value: `SignupQueued` branches on the fresh - * accessJwt synchronously, without waiting for `onUpdated` -> dispatch -> - * re-render. + * accessJwt rather than the account snapshot its callback captured before + * awaiting the refresh. */ let refreshed: SessionAccount | undefined const before = currentAccount()?.accessJwt @@ -172,6 +213,44 @@ describe('refreshSession', () => { expect(refreshed?.accessJwt).toBe('access-jwt-2') }) + it('reports persistence failure without poisoning PasswordSession', async () => { + const fetchMock = makeMockFetch() + const {api} = await renderLoggedIn(makeAccount(), fetchMock) + const persistenceError = new Error('storage failed') + mockWriteSessionError = persistenceError + + /* + * PasswordSession awaits onUpdated inside its shared session promise. The + * hook must catch this rejection or every later refresh and request would + * inherit the rejected promise. The provider retrieves the caught error + * through takeSessionChangeError so this explicit operation still fails. + */ + let refreshError: unknown + await act(async () => { + try { + await api.refreshSession() + } catch (error) { + refreshError = error + } + }) + expect(refreshError).toBe(persistenceError) + expect(mockLoggerError).toHaveBeenCalledWith(persistenceError, { + message: "session: onSessionChange threw for a 'update' event", + }) + + /* + * A second network refresh proves the hook rejection did not poison that + * shared promise. Clearing the simulated failure lets the path complete. + */ + mockWriteSessionError = undefined + let refreshed: SessionAccount | undefined + await act(async () => { + refreshed = await api.refreshSession() + }) + expect(refreshed?.refreshJwt).toBe('refresh-jwt-2') + expect(fetchMock).toHaveBeenCalledTimes(2) + }) + it('resolves with undefined when logged out', async () => { const {api} = renderProvider() diff --git a/src/state/session/index.tsx b/src/state/session/index.tsx index c756f8240d..6b801cbf86 100644 --- a/src/state/session/index.tsx +++ b/src/state/session/index.tsx @@ -680,6 +680,11 @@ export function Provider({children}: React.PropsWithChildren<{}>) { if (!bundle.session) return undefined // logged out: nothing to refresh const before = bundle.session.session const after = await bundle.session.refresh() + /* + * Session hooks catch persistence and reducer failures so they cannot leave + * PasswordSession's shared promise permanently rejected. Recover that error + * out-of-band so an explicit refresh still reports its durability failure. + */ const sessionChangeError = takeSessionChangeError({bundle}) if (sessionChangeError) { throw sessionChangeError instanceof Error diff --git a/src/state/session/session-core.ts b/src/state/session/session-core.ts index d5c5ee1024..e3c8716c56 100644 --- a/src/state/session/session-core.ts +++ b/src/state/session/session-core.ts @@ -166,12 +166,13 @@ export function makeSessionHooks({ return } /* - * A hook must never throw. PasswordSession awaits its hooks inside the - * assignment to its internal session promise, so a synchronous throw here - * leaves that promise permanently rejected: every later request fails, and - * because the session is never marked destroyed, disposeBundle cannot even - * see that the bundle is dead. The dispatch path reaches reducer side - * effects and event emitters, so treat it as capable of throwing. + * A hook must never reject. PasswordSession awaits its hooks inside the + * assignment to its internal session promise, so a thrown or asynchronously + * rejected error here leaves that promise permanently rejected: every later + * request fails, and because the session is never marked destroyed, + * disposeBundle cannot even see that the bundle is dead. The dispatch path + * reaches persistence, reducer side effects, and event emitters, so isolate + * all of their failures from PasswordSession. */ try { const bundle = getBundle()