Repository navigation
fix(cluster): log a removed self-hosted cluster's sign-ins out of its servers, then forget them - #1966
Merged
dawsontoth merged 8 commits intoOct 15, 2026
Conversation
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to sign out of self-hosted cluster entities when they are removed or terminated, ensuring their active sessions are closed. It adds a new signOutEntities method to AuthStore to perform concurrent, best-effort logouts, along with comprehensive unit tests and updated design documentation. Feedback on these changes highlights a potential race condition in terminateCluster.ts where concurrent deletion and retrieval can cause both reads to fail with a 404, preventing logouts from being sent. Additionally, it is recommended to unconditionally clear stored credentials and to improve type safety in Promise.all by returning Promise.resolve() instead of falsy values.
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
dawsontoth
force-pushed
the
claude/1954-self-hosted-logout
branch
from
October 14, 2026 22:28
86fa72d to
beb7dbd
Compare
This was referenced Oct 14, 2026
dawsontoth
marked this pull request as ready for review
October 15, 2026 03:03
… servers, then forget them A self-hosted cluster's servers may outlive its removal from Fabric, so #1954 kept its remembered sign-ins rather than forget them with no logout. Removing one from Studio now ends those sessions instead. authStore.signOutEntities(ids) starts one best-effort logout per remembered id, built the way app sign-out builds them (the remembered credential, a 10s timeout, no response interceptors), forgets every id locally whatever the servers answer, and never rejects. terminateCluster calls it for a self-hosted cluster's own sign-in and each instance's once the DELETE succeeds, without making the removal wait. A Fabric cluster's terminate is unchanged, and a removed self-hosted cluster is still never cleared on render. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
onInstanceLogoutSubmit clears the entity's Basic Auth credential and Fabric Connect state once the logout answers. App sign-out and cluster removal forget the entity before they send, so by the time a late answer lands, anything recorded for it is a newer sign-in, and the clear wiped it: a new Basic Auth credential (from this tab or another), a new Fabric Connect flag, or the token of a Fabric Connect still verifying. sendBestEffortLogout now snapshots what the answer would clear when it sends: the stored Basic Auth credential and Fabric Connect flag, read raw from localStorage so another tab's writes count, and the connection generation, which moves when a Fabric Connect token is claimed. It hands onInstanceLogoutSubmit a signedInSince check, which skips the clear once the snapshot has changed and clears again otherwise. Explicit sign-outs pass no check and clear as before. signOutEntities also reads the sign-in markers from storage, not only this tab's copy, so removing a self-hosted cluster logs out a sign-in another tab made after this one loaded, and drops that sign-in's marker from storage too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…arkers Each tab read Studio:PotentiallyAuthenticated once at startup and wrote that whole copy back on every sign-in and sign-out. That erased every marker another tab had added since. An app sign-out from the stale tab then sent those sign-ins no logout, and a sign-out of an entity only another tab held left its marker behind. - flagKeyAsSignedIn and flagKeyAsSignedOut now change one entity in the record as stored now, and leave the rest alone. A sign-in updates this tab's copy only once its write lands, so a refused write is retried at the next sign-in. - A passing check of a sign-in this tab already marked still writes nothing, and a failing one removes the marker only when it still holds the URL it checked. Neither undoes another tab's change. - When storage refuses to take a signed-out entity out of the record, this tab's later writes leave it out until one lands, instead of trusting or discarding the whole record. - App sign-out, the local full sign-out and signOutEntities act on this tab's markers and every stored one, and log out each URL an entity is marked at, since another tab may have signed in again at a newer one. - signOutEntities' forgetStoredMarkers is gone: flagKeyAsSignedOut now drops another tab's marker for the entity it signs out, so managed cluster termination, which signs out locally, reaches it too. - A stored record that is not an object is treated as unreadable. - DESIGN.md records what remains: storage has no atomic read-modify-write, so two tabs writing at once can still lose one change, and a marker removed elsewhere stays removed. Closes #1968 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The markers lived in one shared Studio:PotentiallyAuthenticated record, so every write read and rewrote the whole of it. Two tabs writing at once could still lose a change, a refused write could take every marker with it, and a full sign-out rewrote the record once per entity. - Each marker now has its own key, Studio:PotentiallyAuthenticated:<id>, holding the URL signed into. A sign-in or sign-out touches only its entity's key, and sweeps enumerate the keys by prefix. - On load, the legacy record is copied into per-entity keys once per storage, and a marker already stored per entity wins. A record that is not one (an array, or not JSON) is removed at every load, since the previous release cannot start on it. - For one release, every marker change is mirrored into the legacy record, and sign-out sweeps read it. A tab still on the previous release, or a rollback, then still sees this release's sign-ins and sign-outs, and an app sign-out here reaches a sign-in only such a tab recorded. The copy is never repeated: that tab writes its whole load-time record back, which would bring back what this release signed out. - A sign-in storage refuses to record no longer throws out of setUserForIdAndKey or a check. Its next sign-in or check records it. A marker storage would not remove is retried at every marker write. - A check's answer is dropped when this tab signed in again at another URL while the check was out. - Unit tests seed and read markers through signInMarkersTestHelpers, and the e2e anon specs through e2e/tests/signInMarkers.ts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ng silently - The sign-out wipe keeps the migration flag, so a previous-release tab writing its record back after a full sign-out isn't migrated again. - Until a migration lands, as when a full quota refuses it, the legacy record also seeds this tab's markers, so its sign-ins still check out. - Migration decides from one read, and also drops entries that aren't markers from the legacy record. A sign-in leaves a legacy record it can't read alone instead of replacing other tabs' markers. - A sign-in always records itself, so signing in again restores a marker another tab removed; only a check's passing answer skips it. - A check's answer is dropped when this tab signed in again while it was out, at the same URL or another, using a per-entity sign-in revision. - A marker storage refused to remove is retried only while it still holds the value it had, so another tab's sign-in there survives, and a marker that couldn't be read at sign-out is still removed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- A failed check is a probe, not a sign-out: when it can't read the marker it leaves it, and a mirror write it can't make no longer takes the whole legacy record. - For the app sign-in, which every /User/current answer re-signs, those answers no longer write the marker, so they can't put back one another tab's failed check dropped. - A failing app check that was out during a new app sign-in is dropped, as an entity's already was. - This tab's own sign-in cancels a removal of its marker still owed from a refused write. - Migration checks its flag before the best-effort cleanup, so a refused cleanup on a migrated storage can't seed signed-out entities. The sign-out wipe marks the storage migrated, as nothing of the legacy record survives it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A page-load app check still out when the user signed in again applied its answer for the old session over the new sign-in: getCurrentUser synced it into the store, and loadUser applied it again. - The store's app check now fetches through fetchCurrentUser, which doesn't sync, so loadUser applies the answer itself, and the sign-in revision guard drops a stale passing answer as it drops a failing one. - getCurrentUser syncs its answer only if no sign-in landed while it was in flight, using the store's new getSignInRevision. - DESIGN.md links #1970 for removing the legacy mirror. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- getCurrentUser syncs through the new syncAppUser, which writes no marker this tab already holds and leaves the sign-in revision alone. A sync landing during the store's own app check therefore no longer makes that check drop its answer and skip its sign-out re-ask. - setUserForIdAndKey treats every sign-in, the app's included, as one that always records itself. - A removal storage still owes is cancelled up front only when the kept marker already holds the URL being signed in, and otherwise once the new marker is written. - A migration flag that can't be read is reported as migrated, so a previous-release write-back can't seed that load. - getSignInRevision sits under its own doc comment. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dawsontoth
force-pushed
the
claude/1954-self-hosted-logout
branch
from
October 15, 2026 13:33
e61ed02 to
0b14c61
Compare
dawsontoth
changed the base branch from
stage
to
claude/1924-full-scan-dialog
October 15, 2026 13:34
This was referenced Oct 15, 2026
Merged
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
#1954 forgets a terminated cluster's sign-ins but keeps a removed self-hosted cluster's, whose servers may still run. So removing one left its sign-ins and any stored Basic Auth credential until the next app sign-out. Separately, a logout that answered late cleared a sign-in made after it was sent.
🤔 Should removing a self-hosted cluster log its sessions out?
Yes, as decided on #1954: log out of its servers, best effort, and forget the sign-ins. Keeping them leaves live sessions and stored credentials for a cluster Studio no longer shows.
💡 Solution
⚖️ Alternatives
🤔 Should a failed logout still forget the sign-in?
I chose yes, as decided. The cost: a server unreachable at removal keeps its session, and no later app sign-out retries it (#1967). Keeping the marker until a logout succeeds would trade that for leaving the sign-in remembered.
🔧 Changes
authStore.ts:signOutEntities(ids)builds one logout client per remembered id, reading the markers from storage too, passes each URL tobuildLogoutClient, forgets the ids, drops another tab's marker for them, then sends. It never rejects.sendBestEffortLogout, shared with app sign-out, takes a snapshot of the stored Basic Auth credential, Fabric Connect flag and connection generation when it sends.onInstanceLogoutSubmit.tsskips its clear when that snapshot has changed. Explicit sign-outs pass no check and clear as before.terminateCluster.ts: a self-hosted cluster goes throughsignOutEntities; a Fabric one goes throughsignOutEntitiesLocallyas before.terminatedClusterSignIns.ts:clusterSignInIdssplit out, soterminatedClusterSignInsstill returns none for a self-hosted cluster on render.cluster/DESIGN.mdrecords the removal rule and the accepted cost.✅ Verification
End-to-end route: (d). A real logout needs a running self-hosted server; the flows run in unit tests.
lateLogoutAnswer.test.tsruns the realonInstanceLogoutSubmitbehind both paths. A new Basic Auth credential (also one another tab stores), Fabric Connect flag or still-verifying token survives a late answer. With nothing replaced, the answer clears again. The eight replaced-sign-in cases fail on the previous head.signOutEntities.test.tscovers the logout's credential and timeout, a failed logout, a session-cookie sign-in, and another tab's sign-in.terminateCluster.test.ts: a self-hosted removal logs out and forgets the cluster and every instance, even when a logout fails, and without waiting. Keep checks need the whole sign-in. These fail onstage.vitest --maxWorkers=4(560 files, 5,705 passed),tsc -b,oxlintanddprint checkexit 0 on Node 24.21.0.Mutation checks and a flaky unrelated file
curryRetryGatewayErrors(retryGatewayErrors.ts) replays reads only.cardLinksDuringPendingNavigation.test.tsxorSecretsManager.delivery.test.tsx. Neither file runs this code, and both pass alone. The pre-commit run on the final commit was clean.Follow-up to #1954 (#1931).
🤖 Generated with Claude Code
Related PRs: #1954 overlaps (merged; this builds on its terminate and DESIGN.md lines), #1956 independent
Complexity: medium
Deferred findings
360de88dc2418237minor security —src/features/auth/store/authStore.ts— retry markers forgotten before the self-hosted logout is answered or dispatched — filed Self-hosted cluster removal: retry a logout that failed, or one skipped because the cluster couldn't be read #1967be82d5c881a51a56minor security —src/features/auth/store/authStore.ts— marker writes serialize this tab's load-time map, erasing markers other tabs added; now also reached from signOutEntities — filed Sign-in markers: a tab's write erases sign-ins other tabs added, so app sign-out can skip their logout #196885883ef1b2f4b7a1nit reliability —src/features/clusters/mutations/terminateCluster.ts— both cluster reads fail so self-hosted removal posts no logout — filed Self-hosted cluster removal: retry a logout that failed, or one skipped because the cluster couldn't be read #1967afad652932df433cnit reliability —src/features/clusters/mutations/terminateCluster.ts— managed termination skips forgetStoredMarkers, so a marker another tab added outlives the local forget — filed Sign-in markers: a tab's write erases sign-ins other tabs added, so app sign-out can skip their logout #1968Review-Coverage: authored=claude; ran=codex,gemini,cursor-grok,cursor-muse; adjudicated=domain; declined=cursor-composer,cursor-kimi,muse; rounds=11; full=7 @ e61ed02
Review-Attention: study ~20m (sensitive: authStore.ts, onInstanceLogoutSubmit.ts; decisions: forget-before-logout-answer, self-hosted-removal-logs-out, removal-not-awaiting-logouts, late-answer-guard-shape, managed-vs-self-hosted-split, do-less-alternative, forget-before-answer, snapshot-guard-shape, logout-only-on-studio-removal) @ e61ed02