Document async session change review

plans/current-review.md

@@ -0,0 +1,169 @@
+# 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**
+
+`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:
+
+- `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**
+
+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.
+
+**Recommendation:** Accept and document this as a tradeoff unless a reliable distinction can be made. Add a distinctive log when a refresh mutation is dropped and its result `jti` differs from the stored one, since that identifies credentials being discarded.
+
+**Tests:**
+
+- Unit-test `applySessionUpdate` for a missing intermediate chain-link.
+- Provider-test the committed-generation mismatch path that rebuilds onto the older generation, so the downgrade remains explicit and visible.
+
+### B3 - Cross-tab listener's `void resumeSession(...)` has new unhandled-rejection paths
+
+**Severity: Low**
+
+At `index.tsx:740`, the cross-tab listener calls:
+
+```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.
+
+**Minimal fix:** Attach `.catch(...)` and log the failure.
+
+### B4 - Login-shaped methods reject after committing reducer state
+
+**Severity: Low**
+
+`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.
+
+**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.
+
+## 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`.
+
+## Recommended order
+
+1. Fix B1 by rechecking abort and bundle identity after the awaited resume commit.
+2. Fix B3 by catching and logging the cross-tab listener's `resumeSession` rejection.
+3. Decide and document the B4 persistence-failure policy for login-shaped methods.
+4. Add tests for:
+   - resume versus logout while persistence is pending;
+   - `refreshSession` with rejecting `writeSession`;
+   - a dropped refresh mutation followed by committed-generation regression.
+5. Accept and document B2 unless a safe distinction between cross-tab advancement and a missing local chain-link emerges; add a distinctive dropped-generation log.
+
+## 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 one genuine state-corruption hole is B1: `resumeSession` performs unguarded asynchronous continuations after persistence. B2 is a knowable conditional-merge tradeoff; B3 and B4 are lower-severity error-policy issues.