Uh oh!
There was an error while loading. Please reload this page.
fix(auth): stop ActiveOrganizationStorage.clear() swallowing a failed removal - #5764
Merged
Merged
Conversation
…ed removal (#5731) `clear()` nulled `_memoryValue` and then removed the persisted key inside a `try`/`catch` that discarded any failure. Since #5703 `get()` prefers a non-null `localStorage` read, so a removal that did not stick left the key readable, the read order preferred it, and sign-out silently did not stick — the cleared org went back on the wire as `X-Tenant-ID`. The removal is now judged by a read-back rather than by catching the throw, which also covers a wrapped `localStorage` whose `removeItem` is a silent no-op, and does not misreport SSR or a fully-throwing store as a failure. A key whose removal cannot be verified is quarantined in memory for the rest of the page-load: `get()` skips its persisted branch and answers from `_memoryValue`. The quarantine is released as soon as a removal on that key sticks. Not thrown and not returned: all five callers arrive after the transition they follow up on has already happened and none can act on a storage failure, so the invariant is restored inside `clear()` and the failure is reported to the console. 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
|
This was referenced Aug 23, 2026
os-zhuang
marked this pull request as ready for review
August 23, 2026 05:43
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#5731
The file the card names is not the file the code is in
Both the card and its triage line say
packages/auth/src/createAuthenticatedFetch.ts. That was true when they were written. #5664 / PR #5744 movedActiveOrganizationStorageinto its own module at 03:56Z the same day;createAuthenticatedFetch.tsnow only re-exports it, and that re-export is untouched here. The asymmetry was re-derived on the post-#5664 code rather than taken from the card's quote, and it survives the move intact.The asymmetry, on
origin/mainat 9850c6eget()prefers a non-null persisted read and only falls through to_memoryValue(the #5703 read order). Soclear()'s two halves are not equally strong: nulling memory always sticks, while a removal that does not stick leaves the key readable — and the read order prefers it. The memory null is shadowed by the surviving persisted value. Sign-out is one of five callers, so the failure mode is "sign-out does not stick", silently, with the cleared org back on the wire asX-Tenant-ID.Distinguishing the accidental swallow from the deliberate one
set()/writePersistedswallows a failingsetItemone function away. That is #5703's memory-fallback design and it is not touched here. The test that tells them apart is not "which one catches" but does the swallow leave a path that still upholds the method's postcondition:set()get()reads back what was set_memoryValueholds it, andget()'s fallback is built to consult itclear()get()answersnull_memoryValueis null but the surviving persisted value outranks itA case in the new file pins that the deliberate one still behaves as #5703 designed it.
What changed
1. The verdict is a read-back, not a caught throw.
removePersistednow returns whether the key is still readable.get()can only answer with whatgetItemhands back, so "did this removal stick" is "is the key still readable". Deciding it from the throw would be wrong in both directions:localStoragewhoseremoveItemis a silent no-op never throws and leaves identical residue. That was the only candidate the filer could name, and it is now covered; a throw-based guard would have missed it entirely.2. The invariant is restored inside
clear(). A key whose removal cannot be verified is quarantined in memory for the rest of the page-load;get()skips the persisted branch for it and answers from_memoryValue— whichclear()just nulled, and which a laterset()refills with the value that write was meant to persist. Correct in both directions with no release step of its own. The quarantine is released as soon as a removal on that key sticks, so it describes the last attempt rather than a permanent verdict on the browser. What it gives up is cross-tab freshness for one key in a browser that has just proved it cannot delete from storage: an unstampedX-Tenant-IDis a documented state of the edge contract (#5279), a re-stamped signed-out org is not.3. The failure is reported, not thrown and not returned.
clear()'s callers were enumerated first, and the shape follows from what they can do:purgeSignedOutClientCaches(sign-out)switchOrganizationdeleteOrganization/leaveOrganizationpurgePreviousUserClientStateviaSessionUserScope.adoptNot one of them can act on it, so a
booleanreturn only moves the problem: five call sites with nothing to write in the failure branch, and a return value every caller ignores reads as handled when it is not.console.warnis the channel this package already uses for intentional diagnostics ([AuthProvider] Failed to load organizations:) and eslint'sno-consoleallows it.Rejected third shape: re-writing the key with an empty value. It relocates the fix into every consumer's truthiness test (
createAuthenticatedFetchdoesif (activeOrgId)) — the lenient consumer AGENTS.md #0.1 forbids — and leaves a signed-out browser holding a live key.Tests
packages/auth/src/__tests__/activeOrgClearRemovalFailure-5731.test.tsx, 10 cases. Per triage, no browser state is reproduced: alocalStoragedouble is the instrument, and the headline assertions are outcomes (get()is null; the header is absent from the wire), not "did it warn".Both failing-removal shapes are driven —
removeItemthrows andremoveItemis a silent no-op — and each case asserts its own premise (the value really reached the persisted layer; the removal really did not stick), so a green case cannot be explained by a double that quietly deleted the key.Controls, without which the pins would pass on an implementation that always returns null: a working store where the removal sticks and the key is gone; a
set()after a failedclear()that must be readable again; and the release-on-recovery case.Ablation
Committed first, mutated, confirmed on disk by single-line anchored counts in both directions, restored under
trap ... EXIT INT TERM, restored tree re-run green.get()quarantine guard_unremovedKeys.has(key)1 → 0 → 1removePersistedstops verifying (return true)readPersisted(key) === null1 → 0 → 1; barereturn true;0 → 1 → 0git statuscleanThe two legs separate the mechanism from its input, which is why both are pinned. Under A the three outcome pins die while the mechanism pins survive —
clear()still populates the quarantine correctly, and the bookkeeping alone does nothing; theget()guard is what turns it into the outcome. Under B the mechanism pins die too, because verification is upstream of everything.Three cases survive both legs, and that is the point of them: "stays quiet when the removal sticks", "stays quiet when there is no storage at all", and the
set()-swallow case. They assert behaviour on stores where nothing fails and on the swallow this card deliberately leaves alone. Had any of them died, the change would have been over-reaching — reporting on a healthy store, crying wolf on SSR, or breaking #5703's memory fallback. Their survival is the evidence that the change is scoped to measured failures.Verification — union run at
0610ca74b(final commit, clean tree)vitest run packages/auth/vitest runapp-shellMetadataProvider.{firstBootOrgScope,orgScopedCache,crossPrincipalSeed}pnpm --filter @object-ui/auth type-checktsc --noEmitandtsc -p tsconfig.test.json)pnpm --filter @object-ui/auth lint✖ 29 problems (0 errors, 29 warnings), all pre-existing and none in the changed filescheck-control-bytesOK (scanned 4805 tracked text file(s))check-changeset-presence2 source file(s) of 1 released package(s) changed ... declares 1 changeset(s)check-changeset-no-major·check-changeset-fixedcheck-lint-coverage·check-type-check-coverage46/46 packages linted;41/41 packages compile their testscheck-package-self-import·check-phantom-dependenciesBoth load-bearing pins in the blast radius are green and unmodified:
activeOrgStorageFallback-5703.test.tsx(9 cases) andsessionUserChangePurge-5664.test.tsx(12 cases).Narrowed lint, declared.
pnpm lintisturbo run lint(per-packageeslint .); the run above is the whole@object-ui/authpackage — a superset of the diff — measured at 46 files from--format json, with the population decided by eslint's own config rather than by a guess. The invariance that makes the narrowing a measurement:eslint.config.jsextendstseslint.configs.recommended, not the type-checked preset, and carries noprojectService/parserOptions/project:(each grepped with the exit code captured before any pipe:grep_exit=1, 0 hits, against control terms in the same file that hit —languageOptions1,rules10,files:6). With no type-aware linting, a diff confined topackages/authcannot move the verdict on any untouched file. The rest of the farm is CI's run.@object-ui/authis vitest-aliased topackages/auth/src, so every run above — the consumer tests and both ablation legs included — exercises the edited source directly. Nodist/is in the resolution path, and no stale-artifact false green is possible.Out of scope
createAuthenticatedFetch.tsis untouched — the re-export still resolves, and no signature moved. One neighbouring finding is filed separately and stays out of this PR: #5763 (open, unassigned) —sweepStore'strywraps the whole loop, so one throwingremoveItemsilently cancels the rest of the session-user purge. Same class, different function and different callers, and it needs a partial-failure test of its own.Generated by Claude Code