Add review docs, WIP
This commit is contained in:
Eric Bailey
@@ -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: <advancing>}` 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.
|
||||
@@ -0,0 +1,3 @@
|
||||
# Current review
|
||||
|
||||
- [ ] Clarify what “rebuilt” means in `const rebuilt = createSessionBundleFromStoredAccount`.
|
||||
Reference in New Issue
Block a user