From 4f1b90621b01e8b8b57aaa31b1fc465dede96a62 Mon Sep 17 00:00:00 2001 From: Eric Bailey Date: Wed, 2 Sep 2026 09:17:37 -0500 Subject: [PATCH] Add review docs, WIP --- plans/code-review-11592-fixes.md | 259 +++++++++++++++++++++++++++++++ plans/current-review.md | 3 + 2 files changed, 262 insertions(+) create mode 100644 plans/code-review-11592-fixes.md create mode 100644 plans/current-review.md diff --git a/plans/code-review-11592-fixes.md b/plans/code-review-11592-fixes.md new file mode 100644 index 0000000000..2f75f94d17 --- /dev/null +++ b/plans/code-review-11592-fixes.md @@ -0,0 +1,259 @@ +# Plan: Fix code-review findings for PR 11592 (session persistence rework) + +Source: adversarial code review of PR 11592 (versioned localStorage sessions, +see `versioned-localstorage-sessions.md`). 16 deduped candidates, 2 refuted, +10 confirmed/plausible findings reported. This plan groups them into six +workstreams ordered by severity and shared root cause. No implementation has +started. + +## Root-cause summary + +The PR makes persistence writes awaited and failable where they were previously +fire-and-forget (`void persisted.write(...)`). Three systemic consequences fell +out of that change and account for most findings: + +1. **Inconsistent failure handling at call sites.** Some paths swallow write + failures silently, some rethrow into callers that treat any rejection as + total failure, and none distinguish "server operation succeeded but + persistence failed" from "operation failed." +2. **In-memory state (`_state`) is now only adopted after a successful write**, + so a storage failure also corrupts the in-memory view - a regression vs + main, where `_state` was updated before the (swallowed) write. +3. **Persistence now sits inside the awaited token-refresh chain**, so its + locking behavior can block the request path. + +## Findings-to-workstream map + +| # | Finding (short) | File anchor | Workstream | +| - | --- | --- | - | +| 1 | writeToStorage rethrow breaks private mode | `src/state/persisted/index.web.ts:221` | A | +| 2 | Indefinite Web Lock can hang token refresh in all tabs | `src/state/persisted/storage-lock.web.ts:21` | B | +| 3 | Stale bundle's token rotation silently not persisted | `src/state/session/index.tsx:119` | C | +| 4 | Failed logout write can resurrect session on cold start | `src/state/session/index.tsx:449` | A | +| 5 | resumeSession no-op resolves as success | `src/state/session/index.tsx:532` | D | +| 6 | resumeSession rejects after switch already committed | `src/state/session/index.tsx:554` | A | +| 7 | Removed empty-page cursor HACKFIX | `src/lib/api/feed/likes.ts:52` | E | +| 8 | Background refresh reorders account list | `src/state/persisted/session-merge.ts:292` | D | +| 9 | try/finally rewrite can strand pending flags | `src/screens/Login/ChooseAccountForm.tsx:62` | F | +| 10 | refreshSession throws despite successful rotation | `src/state/session/index.tsx:688` | A | + +## Workstream A: unify persistence-failure semantics (findings 1, 4, 6, 10) + +This is the core of the fix. Decide on one policy and apply it everywhere, +rather than patching the four symptomatic sites independently. + +### Proposed policy + +- **In-memory state always commits first.** A storage write failure must never + leave `_state` (or the session reducer) behind the truth. Restore the main + behavior: update `_state` before attempting the write, on both platforms + (`src/state/persisted/index.web.ts:104`, `src/state/persisted/index.ts:95-96`). +- **Persistence failures are reported, not thrown, on the session paths.** + Callers of `login` / `createAccount` / `refreshSession` / `resumeSession` / + `logout` should see success when the server-side operation succeeded. Storage + failure becomes a logged warning (with a `safeMessage`) plus, where it + matters, a one-time user-facing signal - not a rejection racing up into + callers like `ChangeHandleDialog.tsx:176/331` and `Deactivated.tsx:77` that + cannot distinguish the two. +- **Expected environmental failures stay quiet.** Web private mode / + quota-exceeded is expected; restore the main-era swallow (with a debug log) + in `writeToStorage` (`index.web.ts:216-221`) so ~25 fire-and-forget + `persisted.write()` call sites (e.g. `src/state/preferences/languages.tsx:46`, + `src/state/shell/onboarding.tsx:40`) do not each produce an unhandled + rejection. + +### Tasks + +- [ ] Restore commit-before-write ordering for `_state` on web + (`index.web.ts:104`) and native (`index.ts:95-96`). This alone fixes the + "second language toggle loses the first" in-memory regression (finding 1) + and the logout-resurrection vector (finding 4), since a later successful + write of any key spreads `{..._state, [key]: value}` from a `_state` that + already reflects the logout. +- [ ] Stop rethrowing from `writeToStorage` on web for quota/private-mode + errors; keep rethrow (or structured error) only for unexpected failures. +- [ ] Sweep every `store.dispatch(...)` persistence promise in + `src/state/session/index.tsx` (lines ~119-139, 371-375, 417-421, 449, + 498, 532-554, 688-693, 732) and make handling uniform per the policy + above. Today: login/createAccount `.catch()` and continue, + logout/removeAccount `.catch(() => {})` silently, resumeSession awaits + uncaught, refreshSession rethrows via `takeSessionChangeError`. All four + shapes should collapse to one helper, e.g. + `persistOrWarn(promise, context)`. +- [ ] resumeSession (finding 6): the in-memory switch commits synchronously at + `index.tsx:114-115`; the awaited write at `index.tsx:554` must not turn a + committed switch into a caller-visible failure + (`useAccountSwitcher.ts:49-57` currently shows "Please sign in as + @handle" and routes to login while `hasSession` is already true). +- [ ] refreshSession (finding 10): once persistence failures no longer throw, + confirm `ChangeHandleDialog` and `Deactivated` close/succeed correctly. + If we decide some callers DO need to know about persistence failure, + expose it as a discriminated result, not a rejection. +- [ ] Audit for remaining unhandled-rejection paths: every `persisted.write()` + and session `dispatch` call site either awaits with handling or is + explicitly `void`-ed against a non-throwing promise. + +### Open decision + +Whether logout should surface persistence failure to the user (it is the one +case where a swallowed failure has a security flavor: tokens remain on disk). +Options: (a) toast "couldn't fully clear stored credentials"; (b) retry harder +then give up silently once `_state` ordering is fixed (the resurrection bug is +gone either way); (c) queue a tombstone re-write on next successful write. +Recommend (b) plus a `logger.error` - the resurrection vector, not the disk +residue, was the real bug, and native disk-full is rare. + +## Workstream B: bound the Web Lock so it cannot hang token refresh (finding 2) + +`navigator.locks.request(PERSISTED_STORAGE_LOCK, op)` at +`storage-lock.web.ts:21` is exclusive, indefinite, and now sits inside the +awaited chain `fetchHandler -> refresh() -> onUpdated -> dispatch -> +writeSession` (`password-session.js:115-116`, `index.web.ts:121`). A tab paused +in devtools (or hung in the critical section) blocks token refresh - and with +it every authenticated request - in all other tabs. + +### Tasks + +- [ ] Add an `AbortSignal` timeout to the lock request (a few seconds). On + timeout, proceed per Workstream A policy: in-memory state is already + committed; log and skip (or retry once in the background) the persisted + write. Never let the request path wait indefinitely on another tab. +- [ ] Alternatively/additionally: move the persisted write out of the awaited + refresh chain entirely - resolve the session promise after the reducer + dispatch, and let `writeSession` run fire-and-forget behind the lock. + Evaluate against the cross-tab reconciliation logic before choosing; + the write ordering guarantees in `session-merge.ts` may depend on + awaiting. +- [ ] Add a regression test simulating a held lock: second tab's refresh must + complete (in memory) within the timeout budget. + +## Workstream C: persist successful rotations from stale bundles (finding 3) + +The reducer's bundle-identity guard (`src/state/session/reducer.ts:84-90`) +returns state unchanged for a mutation from a superseded bundle, so +`needsPersist` stays false and `dispatch` never calls `writeSession` +(`index.tsx:119-139`). But the server has already rotated the refresh token; +bundle disposal is deferred to a post-commit effect (`index.tsx:887-899`), so +account A's `onUpdated` legitimately fires mid-switch. Storage keeps the +consumed refresh token; resuming A after the PDS grace period (~2h) logs the +user out. + +### Tasks + +- [ ] Decouple "update reducer state" from "persist credentials": a valid + `refresh` mutation for a known account should reach storage (via the + session-merge path, which already reconciles by account DID and + generation) even when the reducer rejects it for bundle identity. + Concretely: in `onSessionChange` / dispatch, if the reducer returns + unchanged due to bundle mismatch but the mutation carries newer + credentials for a stored account, still call `writeSession` with a + credentials-only merge. +- [ ] Guard against the inverse: a rotation from a stale bundle must not + clobber NEWER stored credentials for the same account - rely on the + existing generation comparison in `session-merge.ts` (the "Log rejected + refresh generations" commit suggests the machinery exists). +- [ ] Test: switch A -> B while A has an in-flight refresh; assert storage ends + with A's rotated token, reducer stays on B. + +## Workstream D: resumeSession divergence and account ordering (findings 5, 8) + +### Finding 5: silent no-op resume + +`if (!latestStoredAccount?.refreshJwt) return` at `index.tsx:532` resolves as +success without dispatching, so `useAccountSwitcher.ts:38-42` shows "Signed in +as @handle" and fires the `account:loggedIn` metric while the previous account +is still active. + +- [ ] Make the no-credentials path throw a typed error (e.g. + `SessionResumeError('no-stored-credentials')`) so the existing caller + catch shows "Please sign in as @handle" and routes to login, matching + main's behavior. Keep the early return only if there is a case where + silent success is genuinely correct (none identified in review). + +### Finding 8: background refresh reorders the account list + +`replaceAccount` (`session-merge.ts:292`) unshifts to index 0 and is called by +both `refresh` (line 205) and `login` (line 238) mutations. Cross-tab sync +adopts the order verbatim (`reducer.ts:238-241`), so `AccountList.tsx:54` +reorders under the user's finger during a background refresh in another tab. + +- [ ] Preserve list position for `refresh` mutations (replace in place); + move-to-front only for explicit user actions (login, switch). If + "current account first" is an invariant elsewhere, derive display order + in the UI instead of mutating storage order. +- [ ] Test: refresh mutation for account at index 2 leaves it at index 2; + login mutation still fronts it. + +## Workstream E: restore empty-page cursor protection in feed APIs (finding 7) + +The removed HACKFIX ("-sfn" comment) in `src/lib/api/feed/likes.ts:52` and +`src/lib/api/feed/custom.ts:99` handled servers returning a cursor with zero +items. The PR's `seenCursors` guard exists only in `MergeFeedAPI` +(`merge.ts:226-236`). `useAutoPagination`'s repeated-cursor guard never matches +an advancing cursor and its 50-attempt failsafe resets whenever `itemCount` +changes; afterward `hasNextPage` stays true and every `onEndReached` refetches +empty pages with a flashing spinner (`PostFeed.tsx:773, 964`). + +- [ ] Either restore the empty-page-drops-cursor behavior in `LikesFeedAPI` + and `CustomFeedAPI`, or lift the `seenCursors` guard into shared code all + feed APIs use. Prefer the shared guard - it also covers future APIs. +- [ ] Test: mock a feed returning `{items: [], cursor: }` pages; + assert pagination terminates and `hasNextPage` goes false. + +## Workstream F: restore finally-guaranteed cleanup (finding 9) + +The systematic `try/finally` -> `try/catch` + trailing-statement rewrite (~14 +sites) skips cleanup when the catch body itself throws. Confirmed sites: + +- `src/screens/Login/ChooseAccountForm.tsx:62` - `setPendingDid(null)` skipped + if `onSelectAccount(account)` (line 60) throws; the `if (pendingDid) return` + guard (line 33) then ignores every tap until remount. +- `App.tsx:130` - `setIsReady` skipped -> stuck splash screen. +- `ChangePasswordDialog`, `ExportCarDialog`, `ReportDialog`, + `useAccountSwitcher` - `setIsProcessing`/`setLoading` flags, with + `Toast.show` in the catch as the potential thrower. + +### Tasks + +- [ ] Enumerate all rewritten sites in the PR diff (search the diff for removed + `finally` blocks) and restore `finally` for every state-flag cleanup. + Keep `catch` for the error handling; this is `try/catch/finally`, not a + revert. +- [ ] If any site intentionally moved cleanup out of `finally` (e.g. cleanup + must NOT run on failure), add a comment stating why. + +## Sequencing + +1. **A first** - it defines the failure-semantics policy the other session + workstreams build on, and fixes the two worst user-facing regressions + (private-mode breakage, logout resurrection). +2. **B and C next** - both touch the dispatch/writeSession chain A refactors; + doing them after A avoids rework. B's "move write off the refresh chain" + option interacts with A's helper. +3. **D** after A (finding 5's fix shape depends on A's error-vs-result + decision for resumeSession). +4. **E and F** are independent of A-D and of each other; can be parallelized + or done in separate small PRs at any point. + +## Verification + +- [ ] `pnpm test`, `pnpm typecheck`, `pnpm lint` clean. +- [ ] New unit tests called out in B, C, D, E above. +- [ ] Manual web pass in Safari private mode: toggle preferences (no unhandled + rejections, both of two consecutive language additions stick), log in, + change handle (dialog closes), switch accounts, log out. +- [ ] Manual two-tab pass: background refresh in tab B while tab A has the + account switcher open (no reorder); pause tab B in devtools and confirm + tab A's requests still complete after token expiry. +- [ ] Native smoke: login, switch, logout, relaunch (no session resurrection). + +## Explicitly out of scope (cut or refuted in review) + +- uriSize.web helper claim - refuted (blob URLs, not data URIs). +- No-Web-Locks fallback - documented accepted tradeoff. +- Cleanup-level items cut under the finding cap: duplicated reconciliation + blocks, writeSession fast-path/full-root round-trip, storage-lock + duplication, `persistWithRetry` vs `retry()`, `messages.po` churn. Revisit + in a follow-up `/simplify` pass after the correctness fixes land. +- Native `readLatest` lagging the write queue during rapid account switches - + weakest plausible race, mitigated by the PDS grace period; not planned. diff --git a/plans/current-review.md b/plans/current-review.md new file mode 100644 index 0000000000..0d72abcfd8 --- /dev/null +++ b/plans/current-review.md @@ -0,0 +1,3 @@ +# Current review + +- [ ] Clarify what “rebuilt” means in `const rebuilt = createSessionBundleFromStoredAccount`.