fix(auth): send the sign-out logout to the instance, and stop its failure reaching Error Tracking - #1680
Open
dawsontoth wants to merge 1 commit into
Open
fix(auth): send the sign-out logout to the instance, and stop its failure reaching Error Tracking#1680dawsontoth wants to merge 1 commit into
dawsontoth wants to merge 1 commit into
Conversation
…lure reaching Error Tracking `signOutFromPotentiallyAuthenticatedInstances` posts a best-effort `logout` to every entity the tab was signed into, but it cleared `potentiallyAuthenticated` (the only source of `getOperationsUrl`) and the in-memory Fabric token *before* building the client, so any entity without a Fabric Connect flag got a client with no `baseURL` and the POST went to Studio's own origin — RUM shows 41 such POSTs answered 404 across 17 sessions in 30 days. Build the client before the local clears so the logout reaches the instance, and keep a client that cannot be built from aborting the sweep: the local clears run regardless. With the client captured first, the sweep can clear everything local through `signOutLocally` — including the stored basic-auth entry and Fabric Connect flag, which the old loop left for the POST's success path, so a failed logout kept the instance password in localStorage. Because those POSTs now leave for real hosts, the sweep no longer awaits them one at a time: every entity is cleared locally up front, the logouts run in parallel with a 10s timeout and without the gateway-retry or token-recovery interceptors, and only then does the caller's central-manager logout follow — one unreachable instance can no longer hold Sign out for a 60s timeout plus 35s of retry backoff per entity. The failure was then reported with `console.error`, which the RUM SDK collects as an error (`source: "console"`), putting a failure the catch exists to swallow into Error Tracking — 34 events across 13 sessions in 30 days. Log it at debug, as the Reo loader does (#1658), and log only the message: the full Axios error carries the request credentials. Closes #1672
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the sign-out process in authStore.ts to execute instance logouts concurrently and clear local state up front, while applying a 10-second timeout and disabling retries to ensure a fast, best-effort logout. It also introduces a new test suite to thoroughly verify these behaviors. The review feedback recommends converting non-Error objects to strings using String(err) in reportLogoutFailure to prevent potential credential leakage from raw objects like Axios configurations.
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
dawsontoth
marked this pull request as ready for review
September 4, 2026 17:10
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.
Signing out of Studio posts a best-effort
logoutto every instance and cluster the tab was signed into, and swallows a failure there because the local credentials are already cleared and the sign-out carries on. Two things were wrong with that loop. First, it cleared the entity'spotentiallyAuthenticatedentry — the only source of its operations URL — and its in-memory Fabric token before building the logout client, so any entity without a Fabric Connect flag (cookie-session or basic-auth sign-in) got a client with nobaseURL, and the POST went to Studio's own origin instead of the instance: RUM shows 41 such POSTs tohttps://fabric.harper.fast/answered 404 across 17 sessions in the last 30 days, which is the 404 half of #1672. The client is now built before the clears, the same way the manual disconnect inClusterHomebuilds it while still signed in; a client that cannot be built is skipped rather than aborting the sweep. With the client captured first, the sweep can clear everything local throughsignOutLocally— including the stored basic-auth entry and Fabric Connect flag, which the old loop left for the POST's success path, so a failed logout used to keep the instance password in localStorage. Because those POSTs now leave for real hosts, the sweep also stops awaiting them one at a time: every entity is cleared locally up front, the logouts run in parallel with a 10s timeout and without the gateway-retry or token-recovery interceptors, and only then does the caller's central-manager logout follow. Second, the swallowed failure was reported withconsole.error, which the RUM SDK collects as an error (source: "console"), so the one line whose job was to keep the failure out of the way is what putFailed to log out from <id>, carrying oninto Error Tracking (34 events, 13 sessions, 30 days). It now logs atconsole.debug, the channel the Reo loader already uses for exactly this reason (#1658), and logs only the message, since the full Axios error carries the request's credentials. Closes #1672.For the human reviewer
Studio:BasicAuth:<id>andStudio:FabricConnect:<id>toonInstanceLogoutSubmit's success path, so a failed logout kept the instance password on disk untillogoutOnSuccesswiped localStorage — which never runs if the central-manager logout also fails. Chosen: callsignOutLocallyper entity, the primitive the rest of the store already uses, now that the client is captured beforehand with its basic auth, Bearer, or proxy URL baked in. The trade is that local credentials are gone even when the remote session survives an unreachable instance; that was already true for the connection state, and the alternative leaves user A's password for user B on a shared browser.getInstanceClientcreates a new instance per call — agetInstanceClient({ bestEffort: true })option that never installs them would make that structural, at the cost of widening a shared helper's signature for one caller. The central-manager logout still waits for the sweep, because proxy-routed Fabric Connect logouts authenticate with the CM session cookie. Residual the review raised: N parallel 10s clocks against one CM origin can starve the last requests under a browser's per-host connection cap on HTTP/1.1; not addressed, since it needs many simultaneously signed-in proxied instances and a slow proxy at once.console.errorfor non-404s, at the cost of ~18 events/30d staying in Error Tracking.datadogRum.addAction('instance_logout_failed')beside theconsole.debugso the fleet-wide rate stays measurable; not done, since it would make the auth store import the Datadog integration and the daily RUM review already watches the@type:resourceside. Relatedly, a logout POST that fails against a direct instance URL is also a RUM resource error, andshouldKeepEventonly drops network failures for the proxy URL shape — that is today'sbeforeSendpolicy for every direct-instance request, and this PR neither widens nor narrows it. Two related choices the review left to you:console.debugis hidden unless a console shows Verbose, soconsole.warn(also not collected by RUM as an error) would make a silently failing logout visible to a support engineer reading a customer's console; and no test drives the realuseLogoutto prove the sweep is awaited before the central-manager/Logout/— that ordering is load-bearing for proxy-mode entities, whose logout rides the CM session cookie, and a future fire-and-forget refactor would pass this suite.Two further findings from the review are pre-existing and outside this method's change, so they are tracked in #1679 instead of fixed here: an in-flight
establishFabricConnectAuthre-writing auth state after the sweep has run, and an entity re-added mid-sweep being skipped. (#1679's items 1 and 2 — the serial shape and the credential residue — are addressed here.)Verification
signOutFromPotentiallyAuthenticatedInstances.test.ts, eight tests, using the realgetInstanceClientwith only the network calls mocked. (1) Two signed-in instances: eachonInstanceLogoutSubmitcall receives a client whosedefaults.baseURLis that entity's operations URL and whose timeout is 10s. (2) The captured client, given an adapter that rejects with a 503 the way axios's own adapters do, rejects within a 2s test budget — with the gateway-retry interceptor still installed it sleeps 5s first and the test times out. (3) A Fabric Connect direct-mode entity, established throughestablishFabricConnectAuth: the client keeps the direct operations URL and itsAuthorization: Bearerheader, and the token is gone afterwards. (4) A Fabric Connect proxy-mode entity routes to the central-manager/HDBInstance/<id>/operationURL, and its flag is cleared afterwards. (5) With two logouts pending, every entity is already signed out locally and both logouts have been issued; the sweep stays unsettled after the first resolves and settles only after the second — droppingawait Promise.all, or awaiting only the first, fails it. (6) The first logout rejects with a 500-shaped error: the sweep still reaches the second entity, both end up signed out locally, the captured client carried the entity's basic auth and that stored entry is gone afterwards,console.erroris never called, and the message reachesconsole.debug. (7) A corruptStudio:BasicAuth:<id>entry makes the client build throw synchronously: that entity is skipped and reported at debug, the next entity still gets its logout. (8) The cloud slot gets no network logout. WithauthStore.tsreverted toorigin/stage, tests 1–3 and 5–7 fail ([ 'ins-a', undefined ]for the target;expected "wrapper" to not be called at all, but actually been called 1 timesforconsole.error); tests 4 and 6 also fail against the intermediate that kept the hand-rolled clear list. All eight pass on the branch.npx vitest run— 337 files, 2879 passed, 11 skipped;npx tsc -bexit 0;oxlintanddprint checkclean on the changed files.https://fabric.harper.fast/dropping to zero in RUM, and (b) theFailed to log out from…family reading zero under@error.source:console, both across several daily windows after the next prod deploy carries this and with sign-outs having happened in the window. That is the daily RUM review's check, not something a local run can show.Complexity: medium
Review-Coverage: authored=claude; ran=gemini,codex; adjudicated=domain; declined=cursor-grok,cursor-composer; rounds=8 @ 67bb3f3
Human-Review-Need: 4 (decisions: clearing-local-state-before-the-post-lands, per-request-timeout-not-a-sweep-budget, post-hoc-interceptor-clearing-vs-client-option, console-debug-vs-console-warn, sweep-awaited-before-cm-logout) @ 67bb3f3