Repository navigation
fix(auth): sign out without posting a logout the proxy cannot deliver - #1976
Merged
Merged
Conversation
Studio's sign-out posts `logout` to every remembered sign-in before the central-manager `/Logout/`. For an entity reached through the Fabric Connect proxy that POST cannot end anything of the user's: the host manager calls the instance over its socket with no cookie or token, so Harper's `logout` has no session to end and answers 500 — 16 of 24 proxied logouts in the 30 days to 2026-10-15, each a round trip the `/Logout/` waited on. The sweep now skips an entity that `connectsThroughProxy`; local clears and direct logouts are unchanged. Co-Authored-By: Claude Mythos 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Code Review
This pull request prevents sending logout requests to instances connected through the Fabric Connect proxy during app sign-out or cluster removal, as these proxied instances do not hold user credentials and would otherwise return 500 errors. The AuthStore has been updated to skip creating a logout client for these connections, and corresponding unit tests and design documentation have been updated to reflect this change. There are no review comments, and I have no additional feedback to provide.
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
⊙ Problem
Signing out of Studio posts
logoutto every remembered instance and cluster sign-in before the central-manager/Logout/. For an entity reached through the Fabric Connect proxy, thatlogoutcan't end anything. The host manager calls the instance with no cookie or token, so Harper has no session of the user's to end and answers 500. In 30 days of RUM, 16 of 24 proxied logouts were 500s, each a round trip/Logout/waited on. Closes #1973.🤔 Is skipping the request the right fix, rather than making it work?
Yes as filed: a proxied connection holds no credential of the user's on the instance, so no
logoutcould end anything. On app sign-out,/Logout/ends the only session there is. The 8 answers that were not 500 (6 × 400, 2 × 200) are unexplained, but carried no session either. Reverting is one line.💡 Solution
logoutfor an entity that connects through the proxy, using the predicategetInstanceClientalready exports.signOutEntities(self-hosted cluster removal).logout.⚖️ Alternatives
logouton the proxy — still one round trip per entity that/Logout/waits on; useful hardening for older clients, not the fix.connectsThroughProxy— one source of truth for routing, but the predicate already mirrorsgetInstanceClient, and the existingsrc/config/getInstanceClient.test.tspins that they agree. They differ only for an id without anins-/clu-prefix, which no entity has.🤔 Should the skip also apply when a self-hosted cluster is removed?
Yes:
signOutEntitiessharesbuildLogoutClient, so removing a cluster now skips a proxied sign-in too. There is no/Logout/on that path. There was never anything on the instance for the proxiedlogoutto end either. A terminated cluster's proxy answers 400. Narrowing the skip to the app sweep is a one-line move if you disagree.🔧 Changes
src/features/auth/store/authStore.ts—buildLogoutClientreturns no client for a proxied entity, inside its existing try so a corrupt stored credential still only skips that entity;connectsThroughProxyis imported besidegetInstanceClient.src/features/auth/store/signOutFromPotentiallyAuthenticatedInstances.test.ts— the former proxy-routing test becomes a mixed sweep: a proxied entity and one with a leftover Fabric flag over a stored Basic Auth credential get no logout, a Basic Auth and a cookie sign-in still do, and every entity is cleared; the fixture also stands for another tab's direct sign-in.src/features/auth/store/signOutEntities.test.ts— the same proxy case on the cluster-removal path.DESIGN.md— records the credential rule, both callers, the second-tab case and the Disconnect issue (HarperFast/studio#1974).src/features/cluster/DESIGN.md— no longer says every remembered host gets a logout on app sign-out or cluster removal.✅ Verification
npx vitest run: 566 files, 5,855 tests passed (1 expected fail, 11 skipped).tsc -b,oxlintanddprint checkclean.signOutEntities'); every existing direct-Bearer, corrupt-storage, timeout and race test stays green.POST /Cluster/*/operationand/HDBInstance/*/operationwithin 3 s beforePOST /Logout/should drop to zero after the next production deploy.🤖 Generated by Claude Code (Claude Mythos 5.1); posted via @dawsontoth.
Related PRs: #1966 overlaps (added the shared buildLogoutClient this guard lives in), 4 others independent
Complexity: easy
Deferred findings
1c2b8342a1512922minor reliability —src/features/cluster/ClusterHome.tsx— Fabric Connect Disconnect and signOutOfInstance still await the doomed proxied logout before clearing local state — filed Disconnect and per-instance sign-out still post a logout through the Fabric Connect proxy, which can never succeed #1974Review-Coverage: authored=claude; ran=gemini,codex,cursor-grok,cursor-muse; adjudicated=domain; blocked=claude(fallback)(out-of-budget),cursor-composer(out-of-budget); declined=cursor-kimi,muse; rounds=3; full=1 @ 1d7404f
Review-Attention: study ~9m (sensitive: authStore.ts; raised: degraded review) @ 1d7404f