From 65531c359a49327ff3712bc466133621b5a26ef3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Fri, 7 Aug 2026 12:12:39 +0200 Subject: [PATCH] fix(sharing): stop a switch mistaking its own reconcile for a foreign one The scope mirror is watched so a switch this browser did not begin still holds every shell until /me reconciles it: another tab on an older bundle, the referential action behind an account deletion, an operator. It recognised its own by comparing the mirrored scope against the scope it was switching to, and that is not a true test. fetchMe mirrors whatever record the server currently reports and it runs DURING a switch, both when postSwitchWithReconcile reconciles a lost compare-and-set and when a second tab reloads beside the initiator. At that moment the server still reports the record being left, so the write did not match and was read as somebody else's switch. What followed was silent. A foreign write publishes a new transition id, and every terminal call opens by checking the id it was handed, so the commit that followed was dropped without a trace and the expected scope was left naming a record no /me would ever confirm. Nothing could release the hold, and both tabs sat behind the hydration gate until the stored transition aged out thirty seconds later. That is the intermittent browser failure that has been carried through three rounds as a canary and never explained: a peer reload racing a switch, a gate that never lifts, and a run forty-six seconds long made of a fifteen-second wait and a thirty-second one. On the selection that provokes it, eight runs failed three times before and twenty failed none after, and the runs that used to take fifty-seven seconds now take twenty-five. A write is foreign only when nothing of this browser's own is in flight. Ignoring it while held costs nothing, because a held browser is already in the state that branch would install. The rule is a named function so it can be checked in a suite that has no DOM, and the property the guard exists for is pinned beside it: a response that disagrees with a committed scope is stale and still must not release. --- .../record-session-transition.test.ts | 92 +++++++++++++++++++ .../query-keys/record-session-transition.ts | 47 ++++++++-- 2 files changed, 131 insertions(+), 8 deletions(-) diff --git a/src/lib/query-keys/__tests__/record-session-transition.test.ts b/src/lib/query-keys/__tests__/record-session-transition.test.ts index 5d26beb41..59389a61e 100644 --- a/src/lib/query-keys/__tests__/record-session-transition.test.ts +++ b/src/lib/query-keys/__tests__/record-session-transition.test.ts @@ -6,6 +6,7 @@ import { commitRecordSessionTransition, getRecordSessionTransition, resolveUnknownRecordSessionTransition, + isForeignScopeWrite, settleRefusedRecordSessionTransition, settleRecordSessionTransition, } from "@/lib/query-keys/record-session-transition"; @@ -93,4 +94,95 @@ describe("record session transition", () => { expectedScope: undefined, }); }); + + // ── The scope mirror is not a foreign switch ───────────────────────────── + // + // The listener that watches the scope mirror catches a switch this browser + // did not begin. It recognised its OWN by comparing the mirrored scope + // against `expectedScope`, which is not a true test: `fetchMe()` mirrors + // whatever record the server currently reports and it runs during a switch — + // `postSwitchWithReconcile` calls it to reconcile a lost compare-and-set, and + // a peer tab reloading beside the initiator calls it too. At that moment the + // server still reports the record being left, so the write did not match and + // was read as foreign. + // + // The consequence is silent and total. A foreign write publishes a NEW id, + // and every terminal call opens with `if (transition.id !== id) return`, so + // the commit that follows is dropped without a trace and `expectedScope` is + // left naming a record no `/me` will confirm. Both tabs then sit behind the + // hydration gate until the stored transition ages out thirty seconds later. + // + // The rule is extracted rather than driven through a StorageEvent because + // this suite runs in `node`: with no `window` the listener never attaches, + // so a DOM-shaped test here would pass without exercising anything. + + it("treats a scope write as foreign only when nothing of ours is in flight", () => { + expect( + isForeignScopeWrite({ + id: null, + phase: "ready", + expectedScope: undefined, + }), + ).toBe(true); + }); + + it("does not treat its own in-flight switch as a foreign write", () => { + // Both non-ready phases, each with an `expectedScope` that does NOT match + // what a reconcile mirrors — the exact combination the old comparison got + // wrong. + expect( + isForeignScopeWrite({ + id: "t1", + phase: "blocking", + expectedScope: "shared-record", + }), + ).toBe(false); + expect( + isForeignScopeWrite({ + id: "t1", + phase: "resolving", + expectedScope: "shared-record", + }), + ).toBe(false); + }); + + it("does not treat an outcome-unknown hold as a foreign write either", () => { + // `resolveUnknownRecordSessionTransition` leaves `expectedScope` + // undefined, which never matched a mirrored scope, so under the old + // comparison every scope write during that hold reassigned the id. + expect( + isForeignScopeWrite({ + id: "t1", + phase: "resolving", + expectedScope: undefined, + }), + ).toBe(false); + }); + + it("keeps a commit landing on the transition it was issued for", () => { + // The end-to-end shape without the DOM: the id must not move between + // begin and commit, or the commit is dropped and no `/me` releases. + const transitionId = beginRecordSessionTransition("shared-record"); + expect(getRecordSessionTransition().id).toBe(transitionId); + + commitRecordSessionTransition(transitionId, "shared-record"); + expect(getRecordSessionTransition()).toMatchObject({ + phase: "resolving", + expectedScope: "shared-record", + }); + + settleRecordSessionTransition("shared-record"); + expect(getRecordSessionTransition().phase).toBe("ready"); + }); + + it("still refuses to release on a scope the transition did not expect", () => { + // The property the guard exists for, pinned so the change above cannot be + // read as permission to release on anything: a `/me` that disagrees with a + // COMMITTED scope is stale and must not release. + const transitionId = beginRecordSessionTransition("shared-record"); + commitRecordSessionTransition(transitionId, "shared-record"); + + settleRecordSessionTransition(null); + expect(getRecordSessionTransition().phase).toBe("resolving"); + }); }); diff --git a/src/lib/query-keys/record-session-transition.ts b/src/lib/query-keys/record-session-transition.ts index 403b5ec79..9d9fd9406 100644 --- a/src/lib/query-keys/record-session-transition.ts +++ b/src/lib/query-keys/record-session-transition.ts @@ -156,14 +156,11 @@ function ensureTransport(): void { event.newValue === null || event.newValue.length === 0 ? null : event.newValue; - // A current protocol transition already names this scope. The scope - // mirror is its second storage write, not a separate legacy switch. - if ( - transition.phase !== "ready" && - transition.expectedScope === expectedScope - ) { - return; - } + // A transition of this browser's own is already in flight and owns the + // outcome — including the scope its own reconcile mirrors. See + // `isForeignScopeWrite` for what treating that as a foreign switch + // cost. + if (!isForeignScopeWrite(transition)) return; publish({ id: makeId(), phase: "resolving", @@ -185,6 +182,40 @@ function ensureTransport(): void { } } +/** + * Whether a write to the scope mirror is a switch this browser did not begin. + * + * The mirror is watched so that a switch from outside this protocol — another + * tab on an older bundle, the referential action behind an account deletion, + * an operator — still holds every shell until `/me` reconciles it. It used to + * recognise its own by comparing the mirrored scope against `expectedScope`, + * and that is too narrow to be true. + * + * `fetchMe()` mirrors whatever record the server currently reports, and it runs + * DURING a switch: `postSwitchWithReconcile` calls it to reconcile a lost + * compare-and-set, and a peer tab reloading beside the initiator calls it too. + * At that moment the server still reports the record being switched away from, + * so the mirrored scope is not the expected one and the write read as foreign. + * + * What followed was silent. The branch published a NEW transition id, and every + * terminal call is keyed on the id its caller was handed — + * `commitRecordSessionTransition` and `resolveUnknownRecordSessionTransition` + * both open with `if (transition.id !== id) return`. So the commit was dropped + * without a trace, `expectedScope` was left naming a record no `/me` would ever + * confirm, and `settleRecordSessionTransition` refused every answer the server + * gave. Both tabs sat behind the hydration gate until the stored transition + * aged past `TRANSITION_MAX_AGE_MS` — thirty seconds, which is where the + * intermittent forty-six-second failures in the browser suite came from. + * + * So a write is foreign only when nothing of this browser's own is in flight. + * Ignoring it while held costs nothing: `resolving` already means "held, + * outcome unknown, `/me` decides", which is precisely the state this branch + * would otherwise install. + */ +export function isForeignScopeWrite(current: RecordSessionTransition): boolean { + return current.phase === "ready"; +} + function seed(): void { if (seeded) return; if (typeof window === "undefined") {