diff --git a/apps/desktop/src/store/gateway-switch.ts b/apps/desktop/src/store/gateway-switch.ts index cbcd2d03e8..19928dc64f 100644 --- a/apps/desktop/src/store/gateway-switch.ts +++ b/apps/desktop/src/store/gateway-switch.ts @@ -20,6 +20,7 @@ import { setSessions, setSessionsLoading } from '@/store/session' +import { resetSessionPinMirror } from '@/store/session-pin-sync' import { clearAllSessionStates } from '@/store/session-states' // True while a soft gateway-mode apply is mid-flight (wipe → re-dial). Lets the @@ -43,6 +44,10 @@ export function wipeSessionListsForGatewaySwitch(): void { // The next backend is a different runtime — don't carry the old one's // "batched sidebar endpoint missing" capability verdict across the switch. resetSidebarBatchCapability() + // Pins are mirrored per-backend. The next gateway has its own state.db and + // has never seen them, so drop the "already pushed" bookkeeping and let the + // next reconcile re-assert the whole set against the new backend. + resetSessionPinMirror() setSessions([]) setSessionProfilesTruncated({}) setCronSessions([]) diff --git a/apps/desktop/src/store/session-pin-sync.test.ts b/apps/desktop/src/store/session-pin-sync.test.ts index 7a9debac9b..494650ad7e 100644 --- a/apps/desktop/src/store/session-pin-sync.test.ts +++ b/apps/desktop/src/store/session-pin-sync.test.ts @@ -13,7 +13,7 @@ vi.mock('@/hermes', () => ({ import { $pinnedSessionIds } from '@/store/layout' import { $sessions } from '@/store/session' -import { watchSessionPins } from './session-pin-sync' +import { resetSessionPinMirror, watchSessionPins } from './session-pin-sync' const row = (id: string, extra: Partial = {}): SessionInfo => ({ id, message_count: 1, source: 'cli', started_at: 0, title: id, ...extra }) as SessionInfo @@ -30,6 +30,10 @@ beforeAll(() => { beforeEach(() => { $sessions.set([]) $pinnedSessionIds.set([]) + // The mirror/pending/unconfirmed maps are module-global, so one test's + // bookkeeping would otherwise suppress the next test's PATCH (or fence out + // its page). Same reset the gateway switch uses. + resetSessionPinMirror() patch.mockClear() }) @@ -198,14 +202,89 @@ describe('watchSessionPins remote pull', () => { expect($pinnedSessionIds.get()).toContain('race') - // Once the write is acked, later server truth is honoured again. settle({ ok: true }) await flush() await flush() - $sessions.set([row('race', { pinned: false }), row('other')]) + expect($pinnedSessionIds.get()).toContain('race') + }) + + it('still ignores a pre-write page that lands AFTER the ack (#76919)', async () => { + // The ack is not proof: a list request issued before the PATCH is slower + // than the PATCH itself, so it can arrive afterwards still carrying the + // old value. Reverting on it un-pins the session AND pushes the wrong + // value back to the server, making the mistake durable. + $sessions.set([row('acked')]) + $pinnedSessionIds.set(['acked']) + await flush() + await flush() + expect(patch).toHaveBeenCalledWith('acked', true, undefined) + patch.mockClear() + + // Post-ack, but this page predates the write. + $sessions.set([row('acked', { pinned: false })]) await flush() - expect($pinnedSessionIds.get()).not.toContain('race') + expect($pinnedSessionIds.get()).toContain('acked') + expect(patch).not.toHaveBeenCalled() + }) + + it('releases the guard once a page confirms the written value', async () => { + $sessions.set([row('confirmed')]) + $pinnedSessionIds.set(['confirmed']) + await flush() + await flush() + + // The server catches up and echoes our value back. + $sessions.set([row('confirmed', { pinned: true })]) + await flush() + patch.mockClear() + + // With the write confirmed, a genuine remote unpin is authoritative again. + $sessions.set([row('confirmed', { pinned: false })]) + await flush() + + expect($pinnedSessionIds.get()).not.toContain('confirmed') + }) + + it('stops fencing once the guard cooldown expires', async () => { + vi.useFakeTimers() + + try { + $sessions.set([row('stale')]) + $pinnedSessionIds.set(['stale']) + await flush() + await flush() + + // No page ever confirms the write. The guard must not fence forever — + // after the cooldown the server's answer wins again. + vi.advanceTimersByTime(11_000) + + $sessions.set([row('stale', { pinned: false })]) + await flush() + + expect($pinnedSessionIds.get()).not.toContain('stale') + } finally { + vi.useRealTimers() + } + }) + + it('keeps the pin and retries when the write itself fails', async () => { + patch.mockImplementationOnce(() => Promise.reject(new Error('offline'))) + + $sessions.set([row('failed')]) + $pinnedSessionIds.set(['failed']) + await flush() + await flush() + patch.mockClear() + + // The PATCH never landed, so the server legitimately still says unpinned — + // but that's OUR undelivered intent, not a remote decision. The pin stays + // and the next reconcile retries it rather than silently dropping it. + $sessions.set([row('failed', { pinned: false })]) + await flush() + + expect($pinnedSessionIds.get()).toContain('failed') + expect(patch).toHaveBeenCalledWith('failed', true, undefined) }) }) diff --git a/apps/desktop/src/store/session-pin-sync.ts b/apps/desktop/src/store/session-pin-sync.ts index a65b31d530..dddbe741d0 100644 --- a/apps/desktop/src/store/session-pin-sync.ts +++ b/apps/desktop/src/store/session-pin-sync.ts @@ -17,7 +17,8 @@ * authoritative: adopt pins this app hasn't seen, and drop local pins the * server says are gone. Only rows actually present in the payload are * consulted, so a backend predating the flag (`pinned === undefined`) leaves - * the local set untouched. + * the local set untouched — and a page that predates one of our own writes is + * fenced out until a later page confirms the value we wrote. */ import { setSessionPinnedRemote } from '@/hermes' @@ -28,11 +29,17 @@ import { $sessions, sessionMatchesStoredId, sessionPinId } from '@/store/session const mirrored = new Set() // pin ids awaiting their row so we can resolve the owning profile before PATCH. const pending = new Set() -// Writes we've issued but not yet had acked, id -> value written. A list page -// already in flight when we PATCH still carries the old value, so it must not -// be read as the server disagreeing with us. Cleared when the write settles — -// the request's own lifetime is the guard, so nothing can leave one open. -const unconfirmed = new Map() +// Writes we've issued, id -> the value we wrote and when. A list page already +// in flight when we PATCH still carries the OLD value, and it can land after +// our ack — so the ack is not proof the page we're reading is newer than the +// write. Hold the guard until a page actually CONFIRMS the written value, +// with a cooldown so a row that never comes back can't fence itself forever. +const unconfirmed = new Map() + +// How long an unconfirmed write outranks a page that contradicts it. Long +// enough to cover a list request issued just before the PATCH (those are the +// slow ones), short enough that a genuine server-side change still wins. +const WRITE_GUARD_MS = 10_000 function profileFor(pinId: string): null | string | undefined { return $sessions.get().find(row => sessionMatchesStoredId(row, pinId))?.profile @@ -40,13 +47,18 @@ function profileFor(pinId: string): null | string | undefined { /** PATCH the flag, guarding reads against pages that predate the write. */ function writePin(id: string, pinned: boolean, profile?: null | string): Promise { - unconfirmed.set(id, pinned) + unconfirmed.set(id, { at: Date.now(), value: pinned }) return setSessionPinnedRemote(id, pinned, profile).then( () => { - unconfirmed.delete(id) + // Deliberately NOT cleared here: a list request issued before this PATCH + // can still land after the ack carrying the pre-write value. The guard + // is released by pullRemotePins when a page confirms the written value, + // or by the cooldown if none ever does. }, (err: unknown) => { + // A failed write leaves the server on the old value, so the guard would + // be fencing out the truth. Drop it and let the page win. unconfirmed.delete(id) throw err } @@ -76,11 +88,22 @@ function pullRemotePins(): void { const pinId = sessionPinId(row) const heldLocally = local.has(pinId) || local.has(row.id) - // A write of ours the page hasn't caught up to yet is newer than the page. - const awaited = unconfirmed.has(pinId) ? unconfirmed.get(pinId) : unconfirmed.get(row.id) + // A write of ours this page may predate. Confirmed (page agrees) → release + // the guard, the server has caught up. Contradicted but still inside the + // cooldown → the page was almost certainly issued before our PATCH, so our + // write is newer: skip the row. Contradicted past the cooldown → no page + // ever confirmed us, so stop fencing and let the server win. + const guardKey = unconfirmed.has(pinId) ? pinId : unconfirmed.has(row.id) ? row.id : null + const guard = guardKey ? unconfirmed.get(guardKey) : undefined - if (awaited !== undefined && awaited !== row.pinned) { - continue + if (guard && guardKey) { + if (guard.value === row.pinned) { + unconfirmed.delete(guardKey) + } else if (Date.now() - guard.at < WRITE_GUARD_MS) { + continue + } else { + unconfirmed.delete(guardKey) + } } // Local intent still waiting on its PATCH (row unresolved when the push @@ -160,3 +183,20 @@ export function watchSessionPins(): void { $pinnedSessionIds.listen(reconcile) $sessions.listen(reconcile) } + +/** + * Forget what we've mirrored, because the backend we mirrored it TO is gone. + * + * `mirrored` / `pending` / `unconfirmed` all mean "relative to the gateway we + * are talking to". After a soft switch the next backend has its own state.db + * and has never seen these pins, but `mirrored` would report them as already + * pushed and suppress the PATCHes — so the user's pins silently fail to reach + * the new gateway (and its auto-archive sweep is free to hide them). Dropping + * the bookkeeping makes the next reconcile re-assert the whole set, which is + * the same path that migrates pre-existing pins at boot. + */ +export function resetSessionPinMirror(): void { + mirrored.clear() + pending.clear() + unconfirmed.clear() +}