fix session refresh persistence, 2fa login, bundle disposal, cross-tab labelers

Review fixes (PR #11182 round 1):
- PasswordSession fires onUpdated/onDeleted BEFORE committing its
  session data; thread the hook payload through to the provider instead
  of reading the live getter. Fixes rotated tokens never persisting
  (eventual forced logout) and expiry not logging out.
- getErrorName now gates on LexError, so LexAuthFactorError (a sibling
  of XrpcError) surfaces AuthFactorTokenRequired and email-2fa users
  get the code input.
- disposeBundle was a no-op; add a kill-switch closure around the
  session's injected fetch (covers the internal auto-refresh path) so a
  replaced session can't consume rotated refresh tokens. kill() also
  disarms the hooks so stale bundles can't dispatch into the reducer.
- cross-tab same-did rebuild now reapplies subscribed labelers to the
  fresh appview client (was built with an empty per-instance set).
- isAppLabeler reads Client.appLabelers instead of the hard-coded prod
  did, restoring test-env and regional-authority classification.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

src/lib/moderation.ts

@@ -1,5 +1,5 @@
 import {useMemo} from 'react'
-import {api} from '@bsky.app/sdk'
+import {Client} from '@atproto/lex-client'
 import {
   type InterpretedLabelValueDefinition,
   LABELS,
@@ -104,10 +104,11 @@ export function isAppLabeler(
     | app.bsky.labeler.defs.LabelerView
     | app.bsky.labeler.defs.LabelerViewDetailed,
 ): boolean {
+  const appLabelers = Client.appLabelers as readonly string[]
   if (typeof labeler === 'string') {
-    return labeler === api.moderation.did
+    return appLabelers.includes(labeler)
   }
-  return labeler.creator.did === api.moderation.did
+  return appLabelers.includes(labeler.creator.did)
 }
 
 export function isLabelerSubscribed(

src/lib/xrpc-error.ts

@@ -1,4 +1,4 @@
-import {XrpcError, XrpcResponseError} from '@atproto/lex-client'
+import {LexError, XrpcError, XrpcResponseError} from '@atproto/lex-client'
 
 /**
  * True for an XRPC error from a lex `Client` (`@atproto/lex-client` `XrpcError`,
@@ -18,9 +18,16 @@ export function getErrorStatus(e: unknown): number | undefined {
   return e instanceof XrpcResponseError ? e.status : undefined
 }
 
-/** The lexicon error code (`err.error`). */
+/**
+ * The lexicon error code (`err.error`). Gated on `LexError` (the base of the
+ * lex error hierarchy) rather than `XrpcError` so sibling `LexError` subclasses
+ * that are NOT `XrpcError` also surface their `.error` - notably
+ * `LexAuthFactorError` (`'AuthFactorTokenRequired'`), which `PasswordSession`
+ * throws for email-2FA logins. Every `XrpcError` is a `LexError`, so all
+ * existing call sites keep working.
+ */
 export function getErrorName(e: unknown): string | undefined {
-  return isXrpcError(e) ? (e as {error?: string}).error : undefined
+  return e instanceof LexError ? e.error : undefined
 }
 
 /**

src/state/session/__tests__/session-core-test.ts

@@ -38,8 +38,10 @@ jest.mock('jwt-decode', () => ({
 
 import {
   type AtpSessionEvent,
+  disposeBundle,
   extractPdsUrl,
   makeSessionHooks,
+  registerBundleKillSwitch,
   sessionAccountToSessionData,
   type SessionBundle,
   sessionDataToSessionAccount,
@@ -425,7 +427,12 @@ describe('makeSessionHooks arm-latch + event mapping', () => {
   function setup() {
     const onSessionChange =
       jest.fn<
-        (bundle: SessionBundle, did: string, event: AtpSessionEvent) => void
+        (
+          bundle: SessionBundle,
+          did: string,
+          event: AtpSessionEvent,
+          sessionData?: SessionData,
+        ) => void
       >()
     /* the hook only passes this through by identity; a stub bundle suffices */
     const bundle = {} as SessionBundle
@@ -443,7 +450,7 @@ describe('makeSessionHooks arm-latch + event mapping', () => {
     expect(onSessionChange).not.toHaveBeenCalled()
   })
 
-  it("maps onUpdated -> 'update' after arm(), passing the bundle through", () => {
+  it("maps onUpdated -> 'update' after arm(), passing the bundle + payload through", () => {
     const {onSessionChange, bundle, hooks} = setup()
     hooks.arm()
     void hooks.onUpdated?.call(fakeSession, fakeData)
@@ -451,6 +458,8 @@ describe('makeSessionHooks arm-latch + event mapping', () => {
     expect(onSessionChange.mock.calls[0][0]).toBe(bundle)
     expect(onSessionChange.mock.calls[0][1]).toBe(DID)
     expect(onSessionChange.mock.calls[0][2]).toBe('update')
+    /* the fresh SessionData the library delivers is threaded through verbatim */
+    expect(onSessionChange.mock.calls[0][3]).toBe(fakeData)
   })
 
   it("maps onDeleted -> 'expired' after arm()", () => {
@@ -470,6 +479,169 @@ describe('makeSessionHooks arm-latch + event mapping', () => {
     )
     expect(onSessionChange.mock.calls[0][2]).toBe('network-error')
   })
+
+  it("does NOT thread a payload on the 'expired' path", () => {
+    /*
+     * onDeleted maps to 'expired' with no sessionData. The provider guards on
+     * `event === 'update' && sessionData`, so a missing payload here is what
+     * forces refreshedAccount === undefined (reducer clears tokens + logs out).
+     */
+    const {onSessionChange, hooks} = setup()
+    hooks.arm()
+    void hooks.onDeleted?.call(fakeSession, fakeData)
+    expect(onSessionChange.mock.calls[0][2]).toBe('expired')
+    expect(onSessionChange.mock.calls[0][3]).toBe(undefined)
+  })
+})
+
+/*
+ * The exact derivation from the provider's onSessionChange (index.tsx). Pinned
+ * here because the payload threading (session-core) and this mapping together
+ * are the fix: read tokens from the delivered payload on 'update', and force
+ * undefined on the drop paths so the reducer logs the user out.
+ */
+function deriveRefreshedAccount(
+  event: AtpSessionEvent,
+  sessionData?: SessionData,
+): SessionAccount | undefined {
+  return event === 'update' && sessionData
+    ? sessionDataToSessionAccount(sessionData, sessionData.service)
+    : undefined
+}
+
+/*
+ * Pins the pre-commit ordering bug fix. `PasswordSession` fires onUpdated with
+ * the fresh session BEFORE committing it internally, so the live getter is
+ * still stale at hook time. Driven through the real library (not a hand-rolled
+ * fixture) so the ordering is authentic.
+ */
+describe('session-hook payload threading (pre-commit ordering)', () => {
+  it('delivers the NEW tokens via the payload even though the live getter is still pre-commit stale', async () => {
+    const fetchMock = makeMockFetch()
+    let session!: PasswordSession
+    let liveGetterAtHookTime: SessionAccount | undefined
+    let refreshedAccountAtHookTime: SessionAccount | undefined
+    const onSessionChange = jest.fn(
+      (
+        _bundle: SessionBundle,
+        _did: string,
+        event: AtpSessionEvent,
+        sessionData?: SessionData,
+      ) => {
+        /* what the OLD code did: snapshot the live (mutable) getter */
+        liveGetterAtHookTime = sessionDataToSessionAccount(
+          session.session,
+          session.session.service,
+        )
+        /* what the fix does: derive from the delivered payload */
+        refreshedAccountAtHookTime = deriveRefreshedAccount(event, sessionData)
+      },
+    )
+    const hooks = makeSessionHooks(
+      onSessionChange,
+      () => ({}) as SessionBundle,
+      () => DID,
+    )
+    session = new PasswordSession(sessionAccountToSessionData(makeAccount()), {
+      ...hooks,
+      fetch: asFetch(fetchMock),
+    })
+    hooks.arm()
+
+    await session.refresh()
+
+    /* pre-commit ordering: at hook time the live getter still held OLD tokens */
+    expect(liveGetterAtHookTime?.accessJwt).toBe('access-jwt')
+    /* the fix reads the fresh tokens from the payload the hook delivered */
+    expect(refreshedAccountAtHookTime?.accessJwt).toBe('access-jwt-2')
+    expect(refreshedAccountAtHookTime?.refreshJwt).toBe('refresh-jwt-2')
+    /* and the session does eventually commit those same tokens */
+    expect(session.session.accessJwt).toBe('access-jwt-2')
+  })
+
+  it("yields refreshedAccount === undefined on the 'expired' path (forces logout)", async () => {
+    const fetchMock = makeMockFetch({
+      'com.atproto.server.refreshSession': () =>
+        new Response(
+          JSON.stringify({error: 'ExpiredToken', message: 'Token expired'}),
+          {status: 400, headers: {'content-type': 'application/json'}},
+        ),
+    })
+    let refreshedAccountAtHookTime: SessionAccount | undefined = makeAccount()
+    let observedEvent: AtpSessionEvent | undefined
+    const onSessionChange = jest.fn(
+      (
+        _bundle: SessionBundle,
+        _did: string,
+        event: AtpSessionEvent,
+        sessionData?: SessionData,
+      ) => {
+        observedEvent = event
+        refreshedAccountAtHookTime = deriveRefreshedAccount(event, sessionData)
+      },
+    )
+    const hooks = makeSessionHooks(
+      onSessionChange,
+      () => ({}) as SessionBundle,
+      () => DID,
+    )
+    const session = new PasswordSession(
+      sessionAccountToSessionData(makeAccount()),
+      {...hooks, fetch: asFetch(fetchMock)},
+    )
+    hooks.arm()
+
+    await expect(session.refresh()).rejects.toBeDefined()
+
+    expect(observedEvent).toBe('expired')
+    expect(refreshedAccountAtHookTime).toBe(undefined)
+  })
+})
+
+/*
+ * Pins the disposal kill-switch (fix 3). `PasswordSession` exposes no local
+ * destroy, so disposeBundle neutralizes the session by tripping the flag inside
+ * the injected fetch - after disposal every request (direct or auto-refresh,
+ * which shares this same captured fetch) throws before touching the network.
+ */
+describe('disposeBundle kill-switch', () => {
+  it('the injected fetch throws after disposeBundle', () => {
+    const hooks = makeSessionHooks(
+      jest.fn(),
+      () => ({}) as SessionBundle,
+      () => DID,
+    )
+    /* the injected fetch is the kill-switch wrapper makeSessionHooks bakes in */
+    const injectedFetch = hooks.fetch!
+
+    /*
+     * A live session is required for disposeBundle to act (it early-returns on
+     * a null/destroyed session).
+     */
+    const session = new PasswordSession(
+      sessionAccountToSessionData(makeAccount()),
+      {...hooks},
+    )
+    const bundle = {session} as unknown as SessionBundle
+    registerBundleKillSwitch(bundle, hooks.kill)
+
+    /*
+     * Before disposal the wrapper does NOT throw synchronously - it delegates
+     * to the async networkAwareFetch and returns a promise. Swallow that
+     * promise's rejection (the real network is unavailable under jest); we only
+     * care that no synchronous throw happened here.
+     */
+    const pending = injectedFetch('https://bsky.social/xrpc/x')
+    expect(pending).toBeInstanceOf(Promise)
+    void pending.catch(() => {})
+
+    disposeBundle(bundle)
+
+    /* after disposal every call through the injected fetch throws */
+    expect(() => injectedFetch('https://bsky.social/xrpc/x')).toThrow(
+      'session disposed',
+    )
+  })
 })
 
 /*

src/state/session/index.tsx

@@ -9,7 +9,7 @@ import {
   useSyncExternalStore,
 } from 'react'
 import {type Client} from '@atproto/lex-client'
-import {PasswordSession} from '@atproto/lex-password-session'
+import {PasswordSession, type SessionData} from '@atproto/lex-password-session'
 
 import * as persisted from '#/state/persisted'
 import {useCloseAllActiveElements} from '#/state/util'
@@ -19,6 +19,7 @@ import {IS_WEB} from '#/env'
 import {com} from '#/lexicons'
 import {emitSessionDropped} from '../events'
 import {getPublicLexClient, getUnauthenticatedClient} from './clients'
+import {configureModerationForAccount} from './moderation'
 import {type Action, getInitialState, reducer, type State} from './reducer'
 import {
   type AtpSessionEvent,
@@ -29,6 +30,7 @@ import {
   disposeBundle,
   makeSessionHooks,
   type PublicSessionBundle,
+  registerBundleKillSwitch,
   sessionAccountToSessionData,
   type SessionBundle,
   sessionDataToSessionAccount,
@@ -131,14 +133,23 @@ export function Provider({children}: React.PropsWithChildren<{}>) {
       bundle: SessionBundle,
       accountDid: string,
       sessionEvent: AtpSessionEvent,
+      sessionData?: SessionData,
     ) => {
-      // Snapshot the (mutable) live session data right away.
+      /*
+       * Build the refreshed account from the payload the hook delivers, NOT the
+       * live session getter. `PasswordSession` fires onUpdated/onDeleted BEFORE
+       * it commits its internal `#sessionData` (see password-session.js), so at
+       * hook time `bundle.session.session` still holds the OLD tokens (and, on
+       * the expiry path, `destroyed` is still false). Reading the live getter
+       * here would (a) persist stale tokens on 'update' -> eventual forced
+       * logout once the real refresh token expires, and (b) keep the user
+       * signed in on 'expired'. On 'update' the payload carries the new session;
+       * on 'expired'/'create-failed' we force it undefined so the reducer clears
+       * tokens and logs out (it treats undefined as "session gone").
+       */
       const refreshedAccount =
-        bundle.session && !bundle.session.destroyed
-          ? sessionDataToSessionAccount(
-              bundle.session.session,
-              bundle.session.session.service,
-            )
+        sessionEvent === 'update' && sessionData
+          ? sessionDataToSessionAccount(sessionData, sessionData.service)
           : undefined
       if (sessionEvent === 'expired' || sessionEvent === 'create-failed') {
         emitSessionDropped()
@@ -420,7 +431,20 @@ export function Provider({children}: React.PropsWithChildren<{}>) {
             hooks,
           )
           newBundle = buildBundle(newSession)
+          registerBundleKillSwitch(newBundle, hooks.kill)
           hooks.arm()
+          /*
+           * Reapply this account's subscribed labelers to the freshly built
+           * appview client. buildBundle starts with an empty per-instance
+           * labeler set, and unlike login/resume/createAccount this rebuild path
+           * never ran configureModerationForAccount - so without this, subscribed
+           * labelers would silently drop from appview requests in follower tabs
+           * until the next full resume. readLabelers is a local storage read (no
+           * network), preserving the branch's no-network intent; setLabelers
+           * mutates the client's Set in place so labels reappear when it
+           * resolves.
+           */
+          void configureModerationForAccount(newBundle, syncedAccount)
           addSessionDebugLog({
             type: 'agent:patch',
             agent: newBundle,

src/state/session/session-core.ts

@@ -295,6 +295,31 @@ export type SessionBundle = {
   readonly service: URL
 }
 
+/**
+ * Kill-switches for live bundles, keyed by bundle identity.
+ *
+ * `PasswordSession` exposes no local (logout-free) destroy, so disposal is
+ * implemented via a closure flag inside the session's injected `fetch` (see
+ * {@link makeSessionHooks}). The `kill()` that trips that flag is produced next
+ * to the hooks - before the bundle exists - so we stash it here once the bundle
+ * is built and look it up in {@link disposeBundle}. A `WeakMap` keeps this off
+ * the {@link SessionBundle} type (the reducer's opaque view must not see it) and
+ * lets the entry be GC'd with the bundle.
+ */
+const bundleKillSwitches = new WeakMap<SessionBundle, () => void>()
+
+/**
+ * Associate a bundle with the `kill()` from its {@link makeSessionHooks}, so
+ * {@link disposeBundle} can neutralize the underlying session. Call once, right
+ * after the bundle is built, at every session-construction site.
+ */
+export function registerBundleKillSwitch(
+  bundle: SessionBundle,
+  kill: () => void,
+) {
+  bundleKillSwitches.set(bundle, kill)
+}
+
 /**
  * Assemble a {@link SessionBundle} from a live session: the account, appview,
  * and chat clients, all read-through views over the one session. The appview
@@ -331,11 +356,20 @@ export function buildBundle(session: PasswordSession): SessionBundle {
  * -> `'expired'`, transient failure -> `'network-error'`. The whole
  * {@link SessionBundle} is handed through so the provider can snapshot the live
  * session and use the bundle itself as the reducer's identity token.
+ *
+ * `sessionData` is the fresh payload the library hands the hook. It matters
+ * because `PasswordSession` fires `onUpdated`/`onDeleted` BEFORE committing
+ * `#sessionData` (see `refresh()`/`logout()` in password-session.js), so the
+ * live getter (`bundle.session.session`) still returns the OLD tokens at hook
+ * time. The provider must build the refreshed account from this argument, not
+ * from the live getter. Present on the `'update'` path (the new session) and
+ * absent on the error paths.
  */
 type OnSessionChange = (
   bundle: SessionBundle,
   did: string,
   event: AtpSessionEvent,
+  sessionData?: SessionData,
 ) => void
 
 /**
@@ -352,6 +386,20 @@ type OnSessionChange = (
  * are constructed (the session is created first, then the bundle is built over
  * it).
  *
+ * The hooks thread the fresh `SessionData` the library delivers straight
+ * through to `onSessionChange` (the `'update'` payload). The library fires the
+ * hook BEFORE committing that data internally, so the provider must read tokens
+ * from this argument rather than the (still-stale) live session getter.
+ *
+ * The `fetch` option is wrapped in a kill-switch: `kill()` (returned alongside
+ * `arm()`) sets a closure flag so every subsequent request through this
+ * session - direct fetches AND the internal auto-refresh, which
+ * `PasswordSession` routes through the same `options.fetch` captured at
+ * construction - throws instead of hitting the network. `kill()` also disarms
+ * the hooks so a disposed session can never dispatch into the reducer. This is
+ * the disposal mechanism {@link disposeBundle} relies on (`PasswordSession`
+ * exposes no local destroy).
+ *
  * Exported for testing (the arm-latch + event mapping is the core semantics).
  */
 export function makeSessionHooks(
@@ -360,12 +408,13 @@ export function makeSessionHooks(
   getDid: () => string,
 ) {
   let armed = false
-  const dispatch = (event: AtpSessionEvent) => {
+  let killed = false
+  const dispatch = (event: AtpSessionEvent, sessionData?: SessionData) => {
     if (!armed) {
       return
     }
     const did = getDid()
-    onSessionChange(getBundle(), did, event)
+    onSessionChange(getBundle(), did, event, sessionData)
     /*
      * Mirror the old BskyAppAgent.prepare wiring: log any non-create/update
      * session event. In practice we only emit 'update'/'expired'/'network-error'
@@ -376,9 +425,14 @@ export function makeSessionHooks(
     }
   }
   const hooks: PasswordSessionOptions = {
-    fetch: networkAwareFetch,
-    onUpdated() {
-      dispatch('update')
+    fetch: (input, init) => {
+      if (killed) {
+        throw new Error('session disposed')
+      }
+      return networkAwareFetch(input, init)
+    },
+    onUpdated(data) {
+      dispatch('update', data)
     },
     onDeleted() {
       dispatch('expired')
@@ -391,6 +445,10 @@ export function makeSessionHooks(
     arm() {
       armed = true
     },
+    kill() {
+      killed = true
+      armed = false
+    },
   })
 }
 
@@ -475,6 +533,7 @@ export async function createSessionBundleAndResume(
   }
 
   bundle = buildBundle(session)
+  registerBundleKillSwitch(bundle, hooks.kill)
   const account =
     sessionDataToSessionAccount(session.session, session.session.service) ??
     storedAccount
@@ -527,6 +586,7 @@ export async function createSessionBundleAndLogin(
   })
 
   bundle = buildBundle(session)
+  registerBundleKillSwitch(bundle, hooks.kill)
   const account = sessionDataToSessionAccountOrThrow(session)
   accountDid = account.did
 
@@ -594,6 +654,7 @@ export async function createSessionBundleAndCreateAccount(
   )
 
   bundle = buildBundle(session)
+  registerBundleKillSwitch(bundle, hooks.kill)
   const account = sessionDataToSessionAccountOrThrow(session)
   accountDid = account.did
 
@@ -749,26 +810,25 @@ function sessionDataToSessionAccountOrThrow(
 /**
  * Neutralize a bundle's session so it can never refresh again.
  *
- * Called when switching away from / disposing an account. We null out the
- * session locally (constructing a fresh destroyed-state marker is not exposed,
- * so we rely on the reducer dropping all references) - the important guarantee
- * is that this session's tokens are no longer reachable by any live client. We
- * do NOT call `logout()` here: disposal is a local switch, not a server-side
- * revocation (revocation is handled separately via the push-token unregister
- * temporary sessions). The bundle's clients stay usable enough not to crash
- * late readers.
+ * Called when switching away from / disposing an account. `PasswordSession`
+ * exposes no synchronous, hook-free way to mark itself destroyed without a
+ * network logout (and `logout()`/`delete()` would revoke on the server, which
+ * we do NOT want for a local switch - revocation is handled separately via the
+ * push-token unregister temporary sessions). So we trip the kill-switch
+ * installed in the session's injected `fetch` (see {@link makeSessionHooks} /
+ * {@link registerBundleKillSwitch}): every subsequent request through this
+ * session - direct fetch AND the internal auto-refresh, which shares the same
+ * captured `options.fetch` - throws before touching the network. A tripped
+ * refresh routes into the `onUpdateFailure` path (session preserved locally,
+ * refresh token NOT consumed server-side). `kill()` also disarms the hooks so
+ * the stale bundle can no longer dispatch into the reducer. The important
+ * guarantee - matching the old `dispose()` - is that this session's tokens are
+ * no longer reachable by any live network path.
  */
 export function disposeBundle(bundle: SessionBundle | PublicSessionBundle) {
   const session = bundle.session
   if (!session || session.destroyed) {
     return
   }
-  /*
-   * There is no synchronous, hook-free way to mark a PasswordSession destroyed
-   * without a network logout. PasswordSession.delete() would revoke on the
-   * server, which we do NOT want for a local switch. So we fire-and-forget a
-   * logout-free neutralization by dropping our reference; GC reclaims the
-   * session. Any late fetchHandler call still uses valid tokens until the
-   * bundle is dereferenced by the reducer, which is the pre-existing behavior.
-   */
+  bundleKillSwitches.get(bundle)?.()
 }