diff --git a/plans/current-review.md b/plans/current-review.md new file mode 100644 index 0000000000..9b0354cad2 --- /dev/null +++ b/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.