From d441f3c5a352d94124feafbff09e9abc6456e642 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Mon, 31 Aug 2026 21:03:05 +0200 Subject: [PATCH] fix(ai-sessions): do not re-announce a delete this tab is mirroring Co-Authored-By: Claude Opus 5 --- .../copilot/chat/AIChatDisplay.svelte | 14 ++++------- .../copilot/chat/AIChatManager.test.ts | 5 +--- .../sessions/sessionRunOwner.svelte.ts | 20 +++++---------- .../sessions/sessionRuntime.svelte.ts | 11 +++----- .../sessions/sessionState.svelte.ts | 19 +++++++++----- .../sessions/sessionStateIndexedDb.test.ts | 25 +++++++++++++++++++ 6 files changed, 54 insertions(+), 40 deletions(-) diff --git a/frontend/src/lib/components/copilot/chat/AIChatDisplay.svelte b/frontend/src/lib/components/copilot/chat/AIChatDisplay.svelte index 9194eebb61..450dc8c0f5 100644 --- a/frontend/src/lib/components/copilot/chat/AIChatDisplay.svelte +++ b/frontend/src/lib/components/copilot/chat/AIChatDisplay.svelte @@ -550,15 +550,11 @@ return pending?.action === 'question' ? pending.toolCallId : undefined }) - // The composer is locked for as long as another tab's run is in flight: this - // tab still shows the transcript from before that run, and a turn sent from - // it would reach the model as a conversation the driver has already moved - // past. It unlocks on its own once the re-read that follows the turn lands. - // - // No exception for a run parked on a question. Only the driving tab holds the - // question and the resolver waiting on it; a card that looks parked here is a - // restored one whose resolver left with the old page, and answering it would - // deliver to nobody. + // Locked while another tab's run is in flight: this tab still shows the + // pre-run transcript, so a turn sent here would reach the model as a + // conversation the driver has moved past. No exception for a run parked on a + // question — only the driving tab holds the resolver, so answering the card + // shown here would deliver to nobody. const composerLocked = $derived(aiChatManager.runHeldElsewhere) // The prompt the other tab's run is working on, drawn beside the indicator so diff --git a/frontend/src/lib/components/copilot/chat/AIChatManager.test.ts b/frontend/src/lib/components/copilot/chat/AIChatManager.test.ts index 48bbbd4bad..e92f7b3573 100644 --- a/frontend/src/lib/components/copilot/chat/AIChatManager.test.ts +++ b/frontend/src/lib/components/copilot/chat/AIChatManager.test.ts @@ -245,10 +245,7 @@ describe('AIChatManager cross-tab run guard', () => { expect(manager.queuedMessage).toBe('follow up') }) - // Whether a rival tab owns the session is settled by the lock inside the guard, - // not by the heartbeat a send reads on the way in — two tabs sending at once - // both pass that check. So the rewind has to sit behind the guard: refused in - // front of it, the loser would keep a transcript truncated for a turn it never ran. + // Pins `sendRequest`'s `rewind` contract from the caller's side. it('leaves both histories intact when the guard refuses a retry', async () => { const manager = new AIChatManager() manager.isSessionChat = true diff --git a/frontend/src/lib/components/sessions/sessionRunOwner.svelte.ts b/frontend/src/lib/components/sessions/sessionRunOwner.svelte.ts index 120613a810..186436de6e 100644 --- a/frontend/src/lib/components/sessions/sessionRunOwner.svelte.ts +++ b/frontend/src/lib/components/sessions/sessionRunOwner.svelte.ts @@ -187,21 +187,13 @@ export async function withSessionRunLock( } } -/** What exclusion amounts to with no lock to take: refuse while another tab's - * run is visibly on screen, and otherwise go. +/** What exclusion amounts to with no lock to take: refuse while another tab's run + * is visibly on screen, and otherwise go. * - * This is deliberately not mutual exclusion, and cannot be made into it. A - * driver whose timers have been throttled in a hidden tab goes silent long - * before its turn ends, so it is reaped as dead and a send from here can - * start a second turn against the same chat id. Nothing over the channel fixes - * that: a probe distinguishes a throttled tab from a closed one, but not a - * frozen tab from a closed one, and the browser freezes hidden tabs on much the - * same schedule as it throttles them. - * - * Accepted rather than solved, because the only origins that land here are - * served over plain HTTP and are not localhost — a shape used for local testing - * rather than for running Windmill. Every HTTPS deployment, and localhost, is a - * secure context and takes the real lock above. */ + * Not mutual exclusion, and cannot be made into it — a throttled hidden driver + * goes silent before its turn ends and is reaped as dead, and no probe tells a + * frozen tab from a closed one. Accepted because only plain-HTTP non-localhost + * origins land here; every real deployment takes the lock above. */ async function bestEffort(sessionId: string, body: () => Promise): Promise { return await drive(sessionId, body) } diff --git a/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts b/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts index b712be01e2..44d5d88833 100644 --- a/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts +++ b/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts @@ -1030,13 +1030,10 @@ function runPromptEcho(messages: DisplayMessage[], from: number): string | undef /** The prompt each running turn is working on, pinned for the life of the run. * - * Not read fresh each tick, and searched only past where the transcript stood - * when the run began. The first status goes out the moment the guard is - * entered, which is before the send has appended the user message, so the - * transcript still ends with the PREVIOUS turn's prompt then — echoing whatever - * is last would show a watching tab that one and then swap it for the real one. - * By position rather than by text, so sending the same prompt twice running - * still echoes the second one. */ + * Searched only past where the transcript stood at guard entry: the first status + * goes out before the send appends the user message, so reading whatever is last + * would echo the PREVIOUS turn's prompt and then swap it. By position, not text, + * so the same prompt sent twice running still echoes the second one. */ const runPrompts = new Map() function currentRunPrompt(sessionId: string, messages: DisplayMessage[]): string | undefined { diff --git a/frontend/src/lib/components/sessions/sessionState.svelte.ts b/frontend/src/lib/components/sessions/sessionState.svelte.ts index 5575548869..58483b3869 100644 --- a/frontend/src/lib/components/sessions/sessionState.svelte.ts +++ b/frontend/src/lib/components/sessions/sessionState.svelte.ts @@ -447,10 +447,16 @@ export function __resetDeletedSessionIdsForTesting(): void { // The one way to remove a session's record. Tombstones BEFORE awaiting the delete so a // putSession racing this transaction cannot commit its write behind it — a direct // db.delete elsewhere would silently reopen that window. -async function deleteSessionRow(db: IDBPDatabase, id: string): Promise { +async function deleteSessionRow( + db: IDBPDatabase, + id: string, + // Off when this tab is only mirroring another's delete: the other tabs already + // know, and answering them would have them answer back without end. + announce = true +): Promise { deletedSessionIds.add(id) await db.delete('sessions', id) - broadcastSessionDelete(id) + if (announce) broadcastSessionDelete(id) } // The one way to write a session's record, and the other half of the invariant above: @@ -505,12 +511,12 @@ export async function putSession(s: Session): Promise { } } -export async function deleteSessionRecord(id: string): Promise { +export async function deleteSessionRecord(id: string, announce = true): Promise { if (!BROWSER) return const db = await sessionsDb.whenReady() if (!db) return try { - await deleteSessionRow(db, id) + await deleteSessionRow(db, id, announce) } catch (e) { console.error('Failed to delete session record', e) } @@ -629,8 +635,9 @@ function applyRemoteSessionDelete(id: string): void { if (sessionState.currentSessionId === id) sessionState.currentSessionId = undefined // The tombstone stops writes that have not started; one already past that check // can still land after the other tab's delete committed, leaving the row behind - // to reappear on reload. Removing it again here collects that straggler. - void deleteSessionRecord(id) + // to reappear on reload. Removing it again here collects that straggler, silently + // — this delete is a mirror, and announcing it would echo back. + void deleteSessionRecord(id, false) } registerSyncHandlers({ diff --git a/frontend/src/lib/components/sessions/sessionStateIndexedDb.test.ts b/frontend/src/lib/components/sessions/sessionStateIndexedDb.test.ts index 6a88fd53fe..06c98e5346 100644 --- a/frontend/src/lib/components/sessions/sessionStateIndexedDb.test.ts +++ b/frontend/src/lib/components/sessions/sessionStateIndexedDb.test.ts @@ -23,6 +23,14 @@ vi.mock('../copilot/chat/artifacts/artifactsDB', async (orig) => ({ deleteArtifactsForSession: deleteArtifactsForSessionMock })) +// Capture what the row funnels announce to the other tabs, so a delete that is +// itself the echo of another tab's delete can be shown not to answer back. +const { deleteBroadcasts } = vi.hoisted(() => ({ deleteBroadcasts: [] as string[] })) +vi.mock('./sessionSync.svelte', async (orig) => ({ + ...(await orig()), + broadcastSessionDelete: (id: string) => void deleteBroadcasts.push(id) +})) + // sessionState imports WorkspaceService; these tests don't touch the network. vi.mock('$lib/gen', async (orig) => { const actual = await orig() @@ -270,6 +278,23 @@ describe('sessionState IndexedDB persistence', () => { await vi.waitFor(() => expect(sessionState.sessions.map((s) => s.id)).toEqual(['keep'])) }) + // A mirrored delete that announced itself would be mirrored straight back, and + // the two tabs would trade the same message and transaction without end. + it('removes a mirrored delete without announcing it', async () => { + const user = freshUser() + await login(user) + await putSession(session({ id: 'mirrored', createdAt: 1 })) + + deleteBroadcasts.length = 0 + await deleteSessionRecord('mirrored', false) + expect(deleteBroadcasts).toEqual([]) + + // Still genuinely removed — silence is not a no-op, it is what collects the + // row a write racing the other tab's delete left behind. + await rehydrate(user) + await vi.waitFor(() => expect(sessionState.sessions).toEqual([])) + }) + it('isolates sessions between users', async () => { const a = freshUser() const b = freshUser()