From cd99d6693ea9b44fdd57e6fae2c856570f2c6f32 Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 20:09:38 -0700 Subject: [PATCH] fix(runtime): give "same publisher" one answer across the epoch fences MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Separable from the retraction fix beneath it, and it changes handover-path behaviour: a superseded generation that republishes under a merged epoch is now rejected where it was previously accepted. Take it independently or not at all. `publisher-identity-fences.ts` held two answers to "is this the same publisher". `noteRetiredValue` treated a `:headless-merge:` epoch as a SUCCESSOR of its base and retired the base when the merged form became current, while `sameSessionTabsPublicationLineage` treated the two as ONE publisher. Those are contradictory, and the retired-value check's exact-string match was the shim that kept them from ever meeting: a merged frame was a different string, so it never looked retired no matter what had been retired. The cost was that the same predecessor was accepted or rejected depending on which shape it arrived in. A generation a successor had replaced was fenced when it republished bare and admitted when it republished merged — the fail-open half of the same disagreement whose fail-closed half was the removal defect, and the reason that defect reproduced 1 run in 6 rather than every time. This cannot be fixed in the fence alone. Making the fence lineage-aware while a merged epoch still retires its base has the generation retire itself: the rebuild arrives, retires its own base, and the fence then rejects it as a retired generation. So both sides move together — a lineage sibling advances the current epoch instead of superseding it, and inherits its generation's retirement instead of escaping it. Scoped to the publication-epoch functions. Runtime-id retirement keeps exact matching, and `local-structured-session-tabs-sync` keeps its own `hasRetiredValue` call, where a lineage sibling is already excused explicitly and a retired epoch is deliberately not treated as proof of a dead generation. --- ...on-tabs-publisher-identity-lineage.test.ts | 103 ++++++++++++++++++ .../publisher-identity-fences.ts | 28 ++++- 2 files changed, 125 insertions(+), 6 deletions(-) create mode 100644 src/renderer/src/runtime/web-session-tabs-publisher-identity-lineage.test.ts diff --git a/src/renderer/src/runtime/web-session-tabs-publisher-identity-lineage.test.ts b/src/renderer/src/runtime/web-session-tabs-publisher-identity-lineage.test.ts new file mode 100644 index 00000000000..75d2d41631d --- /dev/null +++ b/src/renderer/src/runtime/web-session-tabs-publisher-identity-lineage.test.ts @@ -0,0 +1,103 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { RuntimeMobileSessionTabsResult } from '../../../shared/runtime-types' +import { decideWebSessionTabsSnapshot } from './web-session-tabs-sync' +import { + recordReceivedWebSessionTabsSnapshot, + shouldApplyRecoveredWebSessionTabsSnapshot +} from './web-session-tabs-sync/tracking' +import { resetWebSessionTabsSyncTestState } from './web-session-tabs-sync-test-harness' + +vi.mock('../store', () => ({ useAppStore: { setState: vi.fn() } })) +vi.mock('@/hooks/agent-hook-completion-notifications', () => ({ + observeAgentHookCompletionForNotification: vi.fn() +})) + +/** + * "Same publisher" had two answers that disagreed. `noteRetiredValue` treated a `:headless-merge:` + * epoch as a successor of its base and retired the base when it became current, while + * `sameSessionTabsPublicationLineage` treated the two as one publisher. The retired-value check + * matched exactly, which is what kept those two from ever meeting: a suffixed frame was simply a + * different string, so it never looked retired. + * + * The cost was that the same predecessor was accepted or rejected depending on which shape it + * arrived in. These pin the single answer: a lineage sibling is the same publisher everywhere — it + * advances the current epoch instead of superseding it, and it inherits its generation's + * retirement instead of escaping it. + */ +const ENV = 'remote-runtime' +const WORKTREE = 'repo::/worktree' +const GEN_1 = 'renderer-generation-1' +const GEN_2 = 'renderer-generation-2' +const MERGED_GEN_1 = `${GEN_1}:headless-merge:abc` + +function frame(publicationEpoch: string, snapshotVersion: number): RuntimeMobileSessionTabsResult { + return { + worktree: WORKTREE, + publicationEpoch, + snapshotVersion, + activeGroupId: null, + activeTabId: null, + activeTabType: null, + tabs: [] + } as RuntimeMobileSessionTabsResult +} + +describe('a headless merge is the same publisher as its base epoch', () => { + beforeEach(() => { + resetWebSessionTabsSyncTestState() + }) + + /** + * The fail-open half. A superseded generation used to walk straight back in by republishing + * under a merged epoch, because the fence compared strings and the merged form was a different + * string. The bare form of the identical frame was rejected. + */ + for (const [label, epoch] of [ + ['bare', GEN_1], + ['headless-merge', MERGED_GEN_1] + ] as const) { + it(`fences a ${label} frame from a generation a successor replaced`, () => { + expect(decideWebSessionTabsSnapshot(frame(GEN_1, 5), ENV).apply).toBe(true) + expect(decideWebSessionTabsSnapshot(frame(GEN_2, 1), ENV).apply).toBe(true) + + expect(decideWebSessionTabsSnapshot(frame(epoch, 9), ENV).apply).toBe(false) + }) + } + + /** + * The fail-closed half, and the reason this cannot be fixed in the fence alone. Making the fence + * lineage-aware while the base epoch is still retired by its own merged form has the generation + * retire itself: the rebuild arrives, retires `gen-1`, and is then rejected as a retired + * generation. A publisher must be able to add runtime-owned surfaces without fencing itself out. + */ + it('admits a generation rebuilding under a merged epoch, and returning to a bare one', () => { + expect(decideWebSessionTabsSnapshot(frame(GEN_1, 1), ENV).apply).toBe(true) + expect(decideWebSessionTabsSnapshot(frame(MERGED_GEN_1, 2), ENV).apply).toBe(true) + expect(decideWebSessionTabsSnapshot(frame(GEN_1, 3), ENV).apply).toBe(true) + }) + + /** The same single answer has to hold at the recovery gate, which fences on identity too. */ + it('fences a merged predecessor at the recovery gate as well', () => { + const firstReceived = recordReceivedWebSessionTabsSnapshot(ENV, frame(GEN_1, 5)) + expect(decideWebSessionTabsSnapshot(frame(GEN_1, 5), ENV).apply).toBe(true) + + const successorReceived = recordReceivedWebSessionTabsSnapshot(ENV, frame(GEN_2, 1)) + expect(successorReceived).toBeGreaterThan(firstReceived) + expect(decideWebSessionTabsSnapshot(frame(GEN_2, 1), ENV).apply).toBe(true) + + // Late enough to win on delivery order; retired by lineage, so it must still lose. + const merged = frame(MERGED_GEN_1, 9) + const mergedReceived = recordReceivedWebSessionTabsSnapshot(ENV, merged) + expect(mergedReceived).toBeGreaterThan(successorReceived) + expect(shouldApplyRecoveredWebSessionTabsSnapshot(ENV, merged, mergedReceived)).toBe(false) + }) + + /** A retirement is per worktree: a sibling worktree's history must not fence this one. */ + it('keeps lineage retirement scoped to the worktree that retired it', () => { + expect(decideWebSessionTabsSnapshot(frame(GEN_1, 5), ENV).apply).toBe(true) + expect(decideWebSessionTabsSnapshot(frame(GEN_2, 1), ENV).apply).toBe(true) + + const sibling = { ...frame(MERGED_GEN_1, 1), worktree: 'repo::/other-worktree' } + expect(decideWebSessionTabsSnapshot(sibling, ENV).apply).toBe(true) + }) +}) diff --git a/src/renderer/src/runtime/web-session-tabs-sync/publisher-identity-fences.ts b/src/renderer/src/runtime/web-session-tabs-sync/publisher-identity-fences.ts index fbab01b591b..f6779d5266c 100644 --- a/src/renderer/src/runtime/web-session-tabs-sync/publisher-identity-fences.ts +++ b/src/renderer/src/runtime/web-session-tabs-sync/publisher-identity-fences.ts @@ -131,11 +131,22 @@ export function acceptSessionTabsRuntimeId( return true } +/** + * Retirement is a property of the publishing generation, not of the exact string it published + * under. Matching `retired` exactly let a `:headless-merge:` rebuild of a superseded generation + * walk past this fence while the bare form hit it, so the same predecessor was accepted or + * rejected depending on which shape it happened to arrive in. + */ export function isRetiredSessionTabsPublicationEpoch( key: string, publicationEpoch: string ): boolean { - return hasRetiredValue(sessionTabsPublicationEpochHistoryByWorktree.get(key), publicationEpoch) + const history = sessionTabsPublicationEpochHistoryByWorktree.get(key) + return ( + history?.retired.some((retired) => + sameSessionTabsPublicationLineage(retired, publicationEpoch) + ) ?? false + ) } /** @@ -158,11 +169,16 @@ export function noteSessionTabsPublicationEpoch( key: string, publicationEpoch: string ): SessionTabsPublicationEpochHistory { - const history = noteRetiredValue( - sessionTabsPublicationEpochHistoryByWorktree.get(key), - publicationEpoch, - SESSION_TABS_RETIRED_EPOCH_LIMIT - ) + const existing = sessionTabsPublicationEpochHistoryByWorktree.get(key) + // A headless merge is the same publisher adding runtime-owned surfaces, so it advances the + // current epoch rather than superseding it. Retiring the base here would have the generation + // retire itself, and a lineage-aware fence then rejects its own next frame. + if (existing?.current && sameSessionTabsPublicationLineage(existing.current, publicationEpoch)) { + existing.current = publicationEpoch + sessionTabsPublicationEpochHistoryByWorktree.set(key, existing) + return existing + } + const history = noteRetiredValue(existing, publicationEpoch, SESSION_TABS_RETIRED_EPOCH_LIMIT) sessionTabsPublicationEpochHistoryByWorktree.set(key, history) return history }