|
|
|
@@ -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.
|
Eric Bailey