Skip to content

fix(auth): drop a stale API explorer credential when a tab reloads after missing a sign-out - #1956

Merged
dawsontoth merged 8 commits into
stagefrom
claude/1946-explorer-epoch-survives-clear
Oct 14, 2026
Merged

dawsontoth merged 8 commits into
stagefrom
claude/1946-explorer-epoch-survives-clear

Conversation

@dawsontoth

@dawsontoth dawsontoth commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

A squirrel sweeps the pantry shelves into a basket but keeps the notched tally stick on the wall, while a waking hedgehog tosses away its old key

⊙ Problem

A sign-out bumps the API explorer's sign-out generation, then wipes localStorage, which deleted the bump. A tab that missed the sign-out (reloading or frozen) checks its stored explorer credential against that generation when it loads. After the wipe it read 0 again, so a credential stamped 0 stayed usable (#1946).

🤔 Should the sign-out's wipe keep anything at all?

Yes: the load-time check is the only one a tab that missed the sign-out gets, and every sign-out reset it. What is kept is a single count, with no user or instance in it.

💡 Solution

  • A sign-out now leaves the explorer generation above every credential stamped before it. A tab that missed the sign-out drops its credential when it reloads.
  • The kept record is one global count, written once per sign-out. No instance id carries over to the next account.
  • This holds when a teardown step throws or the store is full. A full store still tells the other tabs once the wipe makes room.

⚖️ Alternatives

  • Wipe before signOutAllLocally — the bump then starts from nothing, so every sign-out writes the same count and a credential stamped after one sign-out survives the next.
  • Keep the per-instance counts — the kept record would then carry the previous account's instance ids.

🤔 Keep the key through the wipe, or write it back after localStorage.clear()?

Keeping it means the normal path never writes the key after the bump, so another tab's bump in between survives. Writing back a snapshot would overwrite that bump. A full store needs the retry either way.

🔧 Changes

🤔 Should a store that refuses every write still reach the other tabs?

Not here: both signals are storage writes, so other tabs then learn from their next request, as before. A BroadcastChannel fallback would reach running tabs only. Both open deferrals are this limit.

✅ Verification

  • Fails on stage: in signOutHandlers.test.ts, a credential stored before the 1st or 2nd sign-out is dropped when a tab that missed it reloads, through both clearAuthStateLocally and logoutOnSuccess. On stage (3e9e160) it is kept, with authGeneration: 0. One write per sign-out (4 on stage) and the announcement after a full store's wipe (never sent on stage) fail there too.
  • Real browser, two tabs (Vite dev server, on a8596fdd9): one tab holding a credential stamped 0 sat on a static page while the other ran clearAuthStateLocally, then reloaded the app. On stage the credential survived; on the branch it came back as {"ins-a":{"method":"bearer"}}.
  • Gates (Node 24.21.0, real exit codes, on 0282f21): vitest run --maxWorkers=4 0 on the second run (553 files, 5,509 passed); the first run's one error, a Radix timer in the unrelated AddOrganizationRoleModal.test.tsx, passes alone. tsc -b 0, oxlint 0, dprint check 0. The husky hook's full run passed on every commit.
The other new tests, and mutation checks

I reverted each guard locally and confirmed a test goes red (17 mutations, all red):

mutation fails
the wipe keeps nothing (the bug), or is localStorage.clear() again the reload cases and the wipe test, both entry points
no retry after the wipe; the retry runs first; a failed removal skips it the full-store and failed-removal cases
the retry never sees a refused generation the full-store case
the collapse keeps the per-instance counts, or does not lift above them the collapse and throwing-teardown tests
per-entity bumps are not skipped, or stay skipped after a teardown throws the one-write test; the bump-after-throw test
an owed collapse is not written at the next entity bump the owed-collapse test
a throwing subscriber aborts the bumps; subscribers are called back after a refused write the throwing-subscriber test; the no-callback test
a refused announcement is not retried, is retried again later, is re-sent after it landed, or is left in storage the announcement tests
the global bump runs only after a clean teardown the throwing-teardown test
one failed removal stops the rest, or is swallowed; keys are listed with Object.keys the clearLocalStorage tests

The collapse finds the highest count with a loop rather than spreading every slot into Math.max, which throws RangeError past about 150,000 slots on Node 24's main thread. No test pins that: a vitest worker accepted 450,000 arguments, and a record that large reaches jsdom's 5 MB storage quota.

🤖 Generated with Claude Code

Related PRs: #1948 overlaps (merged; this moves the explorer bump into its sign-out finally and changes its teardown test), #1954 overlaps (open; adds signOutEntitiesLocally to authStore.ts, no shared lines)

Complexity: complicated

Deferred findings

Review-Coverage: authored=claude; ran=gemini,codex,cursor-composer; adjudicated=domain; blocked=cursor-muse(no-receipt),cursor-grok(no-receipt); declined=cursor-kimi,muse; rounds=10; full=4 @ 0282f21

Review-Attention: deep ~30m (critical: clearLocalStorage.ts; sensitive: clearAuthStateLocally.ts, authStore.ts; decisions: wipe-owned-by-authstore, owed-collapse-overinvalidates, per-key-removal, do-less-alternative) @ 0282f21

…ut's storage wipe

A full local sign-out bumps the API explorer's sign-out generation
(`Studio:ExplorerAuthEpoch`) and then wipes localStorage, which deleted
that bump. The generation read 0 again, so a tab that missed the live
`storage` event (reloading, frozen, in the back-forward cache) kept, on
its next load, any explorer credential stamped 0: every credential made
since the previous sign-out, which had reset it too.

- `clearLocalStorage` now removes every key except the ones it is told
  to keep, attempting each removal and then throwing the first failure.
  `clearAuthStateLocally` keeps the generation key. Nothing writes it
  after the bump, so no rewrite can fail or overwrite another tab's.
- The global bump collapses the record to one `'*'` slot above every
  entity's generation, so the kept record carries no instance or
  cluster ids into the next account's session.
- The global bump runs in the same `finally` as the sign-out
  announcement, so a teardown step that throws no longer skips it. When
  a full store refuses it, the store notes that and the sign-out writes
  it again once its wipe has made room.
- An explorer subscriber that throws no longer stops the sign-out's
  remaining bumps: each is caught and reported.

Closes #1946

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gemini-code-assist[bot]

This comment was marked as resolved.

…rage members

`Object.keys(localStorage)` leaves out a stored key whose name is also a
`Storage` member (`length`, `key`, `getItem`, ...), so a wipe built on it
would keep such a key. `localStorage.key(index)` lists every key, which
is why `clearLocalStorage` uses it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 79.3% 14525 / 18316
🔵 Statements 79.24% 15415 / 19453
🔵 Functions 74.59% 3867 / 5184
🔵 Branches 73.89% 11298 / 15289
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
src/features/auth/handlers/clearAuthStateLocally.ts 100% 100% 100% 100%
src/features/auth/store/authStore.ts 94.1% 83.71% 96.51% 94.04% 122, 264-265, 274-277, 325, 346, 367-378, 386, 391-394, 496-499, 863, 868, 945, 969
src/lib/storage/clearLocalStorage.ts 100% 100% 100% 100%
Generated in workflow #2370 for commit 0282f21 by the Vitest Coverage Report Action

dawsontoth and others added 4 commits October 14, 2026 13:24
…y a refused announcement

- A full sign-out bumped every signed-in entity's explorer generation
  and then the global slot, so it wrote the record N+1 times and called
  each entity's subscriber twice. The per-entity bumps now wait while
  `signOutAllLocally` runs; its one collapsed write, still in the
  `finally`, lifts every entity above its old generation on its own.
- A store too full to take the `Studio:AppSignOut` announcement left
  every other tab signed in until its next request came back 403. The
  refused announcement is now written by the same post-wipe step that
  rewrites a refused generation (`retrySignOutWritesAfterWipe`, renamed
  from `retryUnwrittenExplorerSignOut`), then removed, since the other
  tabs read the user from the event and the wipe keeps no trace of it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…after a refused write

- The post-wipe retry forgot the pending announcement before writing it,
  so a retry storage also refused dropped it. It is now cleared only once
  the write lands.
- A refused explorer-generation write still called every same-tab
  subscriber, each of which re-read storage to find nothing had changed.
  Subscribers now hear only about a write that landed.
- `explorerBumpsDeferred` is renamed `fullSignOutInProgress`: the
  per-entity bumps it covers are skipped, not replayed. DESIGN.md says
  so, and that the guarantee needs storage to accept one write.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…one step

The retry only worked if `clearAuthStateLocally` ran it after the wipe,
and nothing held the two steps in that order. `wipeLocalStorageForSignOut`
now does both: it keeps the explorer generation, and its `finally` runs
the retry even when a removal failed. The generation key and the retry
go back to being private.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d its announcement only once

- When storage refused both the full sign-out's collapse and its retry,
  the next entity sign-out bumped that entity alone, so every other
  entity kept the generation the revoked credentials were stamped with.
  It now writes the owed collapse instead.
- A refused announcement was kept pending after the post-wipe retry,
  with no later caller. It is now dropped there on purpose: sent any
  later, it could end a session the user has signed back into in another
  tab. DESIGN.md says both.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dawsontoth and others added 2 commits October 14, 2026 14:10
…t owes it

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dawsontoth
dawsontoth added this pull request to the merge queue Oct 14, 2026
Merged via the queue into stage with commit a0d981e Oct 14, 2026
5 checks passed
@dawsontoth
dawsontoth deleted the claude/1946-explorer-epoch-survives-clear branch October 14, 2026 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants