Uh oh!
There was an error while loading. Please reload this page.
fix(auth): stop sweepStore aborting the sign-in purge on one throwing removeItem (#5763) - #5779
Merged
Merged
Conversation
… removeItem sweepStore's try wrapped the WHOLE loop, not each removal. A removeItem that threw on key n aborted the walk, so keys n+1..end were never swept, and the failure was swallowed — purgePreviousUserClientState() returned normally and SessionUserScope.adopt believed the sign-in purge had completed. This is the #5664 allowlist sweep; a partial sweep is a partial allowlist, and which keys survived depended on Object.keys iteration order rather than on anything bounded. Object.keys(store) — the reason a guard exists here at all — stays guarded on its own; only the per-key guard is new. Unlike ActiveOrganizationStorage.clear() (#5731), a failed removal here is not verified by read-back and not quarantined: clear() owns every future read of its one key through get(), which is what makes a quarantine meaningful; sweepStore walks keys it does not own reads for, so there is no get() to guard and nothing to quarantine here — adding read-back verification would be a general storage-error-handling refactor of the module, out of this card's scope. What is mirrored is the reporting channel: a key whose removeItem throws is named in a console.warn, the same channel clear() uses, so a partial sweep is discoverable instead of silent. Adds a partial-failure test: a store whose removeItem throws on one key, asserting every other non-device-scoped key on both sides of it is still swept and the device-scoped allowlist is still respected, plus a control that the warning fires only on an actual failure. Fixes#5763 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EuPCi56cnGyykygi3z9w4m
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
os-zhuang
marked this pull request as ready for review
August 23, 2026 07:55
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes#5763
The shape
packages/auth/src/ActiveOrganizationStorage.ts,sweepStore():The
trywrapped the WHOLE loop, not each removal. AremoveItemthat threw on keynaborted the walk, so keysn+1..endwere never swept, and the failure wasswallowed —
purgePreviousUserClientState()returned normally andSessionUserScope.adoptbelieved the sign-in purge had completed. This is the #5664 allowlist sweep (part 3 of
that fix) precisely so the next un-namespaced key — one nobody has written yet — cannot
re-open the cross-user pollution class; a partial sweep is a partial allowlist, and
which keys survived depended on
Object.keysiteration order rather than on anythingbounded.
What changed
Object.keys(store)— the reason a guard exists here at all — stays guarded on itsown; only the per-key guard is new, so one uncooperative key now costs exactly that
key, not the rest of the purge (the invariant triage named).
The "should a failed sweep report" question — measured, and where it diverges from #5731
Triage directed following whatever #5731's landed answer (PR #5764, merged before this
branch was cut) was for
clear(), and saying so explicitly if it doesn't transfer. Readon
origin/mainat 286dd8d:clear()'s verdict is a READ-BACK (removePersistedreturns whether the key isstill readable after the attempt), not "did
removeItemthrow" — because a wrapped orproxied
localStoragewhoseremoveItemis a silent no-op never throws and leavesidentical residue.
clear()quarantines a key whose removal it cannot verify, in_unremovedKeys,so
get()skips the persisted branch for it and answers from_memoryValueinstead —the quarantine is what turns the verdict into an outcome.
clear()reports viaconsole.warn, because none of its five callers (includingpurgePreviousUserClientStateitself) can act on a storage failure.Reporting transfers; read-back-verdict and quarantine do not, and here is why.
clear()can quarantine because it owns every future read of its ONE key, throughActiveOrganizationStorage.get()— the quarantine is meaningless without aget()toconsult it.
sweepStorewalks an open-ended set of keys it does not own reads for(another package's recents cache, a metadata seed written by
MetadataProvider) — thereis no
get()here to guard, so there is nothing to quarantine, and building ageneralized read-back-plus-quarantine mechanism for keys this function doesn't otherwise
touch would be the "general storage-error-handling refactor of the module" the card
scopes this fix away from. What is mirrored is the channel: a key whose
removeItemthrows is named in a
console.warn, same asclear(), so the failure the card's titleis about — a silent partial purge — is now discoverable. The caller
(
SessionUserScope.adopt, on the sign-in path, inside anAuthProvidereffect) stillcannot act on it and must not throw either, same reasoning #5731 laid out for the
sign-out callers.
One consequence worth naming: because the verdict here is "did
removeItemthrow" andnot a read-back, a wrapped/proxied
localStoragewhoseremoveItemis a silent no-op(the "too narrow" case #5731's own docblock names) would sweep silently and unreported
through this function — narrower coverage than
clear()'s. That gap is accepted asin-scope-for-#5731-only, not closed here; closing it would mean the same generalization
just ruled out.
Tests
packages/auth/src/__tests__/sessionUserChangePurge-5664.test.tsx, two new cases (14total in the file, up from 12; 23 total across it and
activeOrgStorageFallback-5703.test.tsx):sweeps every other non-device-scoped key when one removeItem throws (#5763)— alocalStorage.removeItemspy throws for one poisoned key, with keys seeded on BOTHsides of it in insertion order. Asserts every other key — including the one seeded
after the poison — is swept in both stores, the device-scoped allowlist survives,
and the failure is named once in a
console.warncontaining the poisoned key.reports nothing when every removal sticks— control on the case above: noconsole.warncall on a healthy sweep, so the warning is a measurement of an actualfailure and not a constant.
Reverse verification (fix committed first, so the restore below is a real
checkpoint, not the working tree as the only copy):
objectui-favorites— seeded after the poisoned key — survives on pre-fix codeexactly as the card describes: the whole-loop
tryaborts the walk at the throw. Fixrestored with
git checkout claude/issue-5763-sweepstore-per-key-try -- packages/auth/src/ActiveOrganizationStorage.ts;tree byte-identical to the committed fix (
git statusclean), full suite re-run greenafterward.
A real-jsdom
Storagegotcha found while writing the case, noted for the nextperson:
vi.spyOn(localStorage, 'removeItem').mockImplementation(...)was observed tosurvive this file's shared
afterEach(() => vi.restoreAllMocks())— the very next testmeasured the mock still installed on entry (
(localStorage.removeItem as any).mock !== undefinedwastrue). Fixed by capturing the spy and calling.mockRestore()explicitly in a
finallyinside the test that installs it, rather than relying on theshared teardown for this particular spy target.
Verification — union run at
8e1409881(final commit, clean tree)vitest run packages/auth/src/__tests__/sessionUserChangePurge-5664.test.tsx packages/auth/src/__tests__/activeOrgStorageFallback-5703.test.tsxvitest run packages/auth/pnpm --filter @object-ui/auth type-checktsc --noEmitandtsc -p tsconfig.test.json)pnpm --filter @object-ui/auth lint✖ 29 problems (0 errors, 29 warnings), same count as #5764's baseline, none in the changed filescheck-control-bytesOK (scanned 4814 tracked text file(s); skipped 85 binary)check-changeset-presence2 source file(s) of 1 released package(s) changed … declares 1 changeset(s)check-changeset-no-major·check-changeset-fixed@object-ui/authis vitest-aliased topackages/auth/src, so every run above exercisesthe edited source directly — no
dist/in the resolution path.Serial-note discharge (triage's note)
Triage's serial note said this shares
ActiveOrganizationStorage.tswith #5731'sthen-in-flight PR and to land after it or rebase. #5731's PR #5764 merged to
mainat0610ca74b(2026-08-23, before this branch was cut fromorigin/mainat286dd8daa),so this branch already contains that fix — confirmed by reading the file on this base:
removePersisted's read-back verdict andActiveOrganizationStorage._unremovedKeysarealready present. Nothing to serialise behind.
Out of scope
AuthProvider.tsx'spurgeSignedOutClientCaches()— the sign-out counterpart, whichpurges
sessionStoragemetadata-seed-cache keys — has the identical shape: atrywrapping the whole loop instead of each
removeItem. Same defect class, different file,different caller, and this card's own scope note rules out widening into a general
refactor of the module. Filed separately, unassigned: #5777.
Generated by Claude Code