From d91e4556497aa9d612ab181cdb4375089a500462 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Fri, 28 Aug 2026 18:02:10 +0200 Subject: [PATCH] fix(ai-sessions): scope a cross-tab Stop to its turn and hand back a cleared queue Co-Authored-By: Claude Opus 5 --- .../copilot/chat/AIChatManager.svelte.ts | 21 ++++++++------ .../copilot/chat/AIChatManager.test.ts | 22 +++++++------- .../sessions/sessionRunOwner.svelte.ts | 20 ++++++++++--- .../sessions/sessionRunOwner.test.ts | 29 ++++++++++++++++++- .../sessions/sessionRuntime.svelte.ts | 16 +++++++--- .../components/sessions/sessionSync.svelte.ts | 19 +++++++----- 6 files changed, 91 insertions(+), 36 deletions(-) diff --git a/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts b/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts index e1ae03aa17..c1e4fdfd24 100644 --- a/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts +++ b/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts @@ -1098,11 +1098,9 @@ export class AIChatManager { count === 1 ? 'A background job just finished.' : `${count} background jobs just finished.` this.instructions = note const accepted = await this.sendRequest({ synthetic: true }) - // Only sendRequestImpl clears these, and a refusal returns before it — - // leaving them set, which this method's own guard above reads as "the - // user is mid-compose" and never resumes again. Cleared only while they - // are still the note put there: a send that won the lock meanwhile owns - // the field, and blanking it would take the user's message mid-turn. + // A refusal returns before sendRequestImpl's clear, and the guard above + // reads a leftover note as "the user is mid-compose" and never resumes. + // Conditional because a send that won the lock meanwhile owns this field. if (accepted === false && this.instructions === note) this.instructions = '' } catch (e) { console.error('Auto-resume after background job failed', e) @@ -4172,10 +4170,15 @@ export class AIChatManager { this.displayMessages = [] this.messages = [] this.contextUsage = undefined - // Same reason saveAndClear drops it: a queue belongs to the conversation it - // was written in, and this is the other door onto a fresh chat — one left - // here would auto-send into a conversation it was never meant for. - this.#clearQueue() + // A queue belongs to the conversation it was written in, and this is the + // other door onto a fresh chat, so it cannot stay queued. Handed back + // rather than dropped: unlike saveAndClear, nobody in this tab asked for + // this — the clear arrived from elsewhere, and one of the two ways here is + // a turn that rolled back, where the conversation the follow-up was written + // for is still the one on screen. Gated on a mounted composer because + // restoreToInput's fallback is the very queue being emptied. + if (this.aiChatInput) this.dequeueMessage() + else this.#clearQueue() this.clearBackgroundJobs() if (this.modifiedItems) this.modifiedItems = new SvelteSet() this.#syncMessageFiles() diff --git a/frontend/src/lib/components/copilot/chat/AIChatManager.test.ts b/frontend/src/lib/components/copilot/chat/AIChatManager.test.ts index c2c62538b5..579c14f2d1 100644 --- a/frontend/src/lib/components/copilot/chat/AIChatManager.test.ts +++ b/frontend/src/lib/components/copilot/chat/AIChatManager.test.ts @@ -401,12 +401,7 @@ describe('AIChatManager.sendOrQueue', () => { expect(manager.queuedMessage).toBe('') }) - // The auto-resume writes its prompt into `this.instructions` before sending, - // and only sendRequestImpl clears them. A refusal that returns before that - // `instructions` belongs to whichever turn is running, not to the send being - // refused. A synthetic auto-resume racing a user's send must not unwind it - // from under them — its own caller clears it, and only while it still holds - // the note it put there. + // The refused send must not unwind a field the winning turn now owns. it('does not blank instructions when a synthetic send is refused', async () => { const manager = new AIChatManager() manager.isSessionChat = true @@ -980,16 +975,21 @@ describe('AIChatManager queued messages', () => { expect(manager.queuedMessage).toBe('first line\nsecond line') }) - // A queue belongs to the conversation it was written in. adoptEmptyChat is the - // cross-tab door onto a fresh chat — another tab ran "/clear" — and a queue - // left behind would auto-send into a conversation it was never meant for. - it('drops a queued message when another tab clears the chat', () => { - const manager = createManager() + // A queue belongs to the conversation it was written in, so it cannot survive + // the cross-tab door onto a fresh chat — it would auto-send into a + // conversation it was never meant for. Handed back rather than dropped: + // nobody in this tab asked for the clear, and one of the two ways here is a + // rolled-back turn, where the conversation it was written for is still on + // screen. + it('hands a queued message back to the composer when another tab clears the chat', () => { + const input = createInputMock() + const manager = createManager(input) manager.queueMessage('follow up to the old conversation') manager.adoptEmptyChat('a-brand-new-chat-id') expect(manager.queuedMessage).toBe('') + expect(input.prependText).toHaveBeenCalledWith('follow up to the old conversation', [], []) }) it('dequeues the message and restores it into the input', () => { diff --git a/frontend/src/lib/components/sessions/sessionRunOwner.svelte.ts b/frontend/src/lib/components/sessions/sessionRunOwner.svelte.ts index 0d65e00778..b138fffadb 100644 --- a/frontend/src/lib/components/sessions/sessionRunOwner.svelte.ts +++ b/frontend/src/lib/components/sessions/sessionRunOwner.svelte.ts @@ -1,5 +1,6 @@ import { BROWSER } from 'esm-env' import { SvelteMap } from 'svelte/reactivity' +import { randomUUID } from '$lib/utils/uuid' // Who holds a session's run, for every tab open on it. // @@ -31,10 +32,14 @@ const DRIVER_SILENCE_MS = 10_000 * the model. */ export type RunPosition = | { state: 'idle' } - | { state: 'driving' } + | { state: 'driving'; runId: string } | { state: 'watching' lastHeardAt: number + /** Which of the driver's turns this tab is watching. A control message + * names the run it was pressed for, so one delayed across a turn + * boundary is dropped instead of landing on the turn that followed. */ + runId: string /** The driver's plan-mode posture, carried so a watching tab can show the * mode the run is actually under. It lives here rather than on the * manager so it cannot outlive the run it describes: the position moves @@ -74,14 +79,21 @@ export function runHeldElsewhere(sessionId: string | undefined): boolean { } /** The driver said how its run is going, which is also its sign of life. */ -export function noteDriverAlive(sessionId: string, planMode: boolean): void { +export function noteDriverAlive(sessionId: string, planMode: boolean, runId: string): void { // A tab mid-turn is the authority on its own session; a status message // reaching it can only be an echo of the run it is itself driving. if (isDriving(sessionId)) return - positions.set(sessionId, { state: 'watching', lastHeardAt: Date.now(), planMode }) + positions.set(sessionId, { state: 'watching', lastHeardAt: Date.now(), planMode, runId }) ensureReaper() } +/** The run this tab is in, either as its driver or as a watcher. Control + * messages carry it so they can only act on the turn they were meant for. */ +export function currentRunId(sessionId: string): string | undefined { + const p = runPosition(sessionId) + return p.state === 'driving' || p.state === 'watching' ? p.runId : undefined +} + /** The driver says its turn is over. The re-read that follows is what frees this * tab, so the position moves to `catchingUp` — but only where a runtime exists * to perform it. The channel runs this in every tab that loads it, and one with @@ -190,7 +202,7 @@ async function bestEffort(sessionId: string, body: () => Promise): Promise } async function drive(sessionId: string, body: () => Promise): Promise { - positions.set(sessionId, { state: 'driving' }) + positions.set(sessionId, { state: 'driving', runId: randomUUID() }) try { return await body() } finally { diff --git a/frontend/src/lib/components/sessions/sessionRunOwner.test.ts b/frontend/src/lib/components/sessions/sessionRunOwner.test.ts index 6251882ff7..dac03bfdf6 100644 --- a/frontend/src/lib/components/sessions/sessionRunOwner.test.ts +++ b/frontend/src/lib/components/sessions/sessionRunOwner.test.ts @@ -1,13 +1,20 @@ import { afterEach, describe, expect, it, vi } from 'vitest' import { clearRunPosition, + currentRunId, noteDriverAlive, noteRemoteTurnEnded, onDriverLost, withSessionRunLock } from './sessionRunOwner.svelte' -const SESSIONS = ['session-watching', 'session-catching-up', 'session-idle', 'session-unheard'] +const SESSIONS = [ + 'session-watching', + 'session-catching-up', + 'session-idle', + 'session-unheard', + 'session-runs' +] // `positions` is module state and a watching entry keeps the reaper interval // armed, so leave nothing behind for the next suite. @@ -45,6 +52,26 @@ describe('withSessionRunLock with no lock to take', () => { expect(body).toHaveBeenCalledTimes(1) }) + // A control message names the run it was pressed for, so a Stop delivered + // after its turn ended is dropped rather than landing on the turn that + // followed — which the queue flush and job auto-resume start immediately. + // Reusing an id across turns would defeat that, so pin that they differ. + it('gives each turn its own run id', async () => { + const seen: (string | undefined)[] = [] + await withSessionRunLock('session-runs', async () => { + seen.push(currentRunId('session-runs')) + }) + await withSessionRunLock('session-runs', async () => { + seen.push(currentRunId('session-runs')) + }) + + expect(seen[0]).toBeTruthy() + expect(seen[1]).toBeTruthy() + expect(seen[0]).not.toBe(seen[1]) + // And nothing is left claiming a run once the turn is over. + expect(currentRunId('session-runs')).toBeUndefined() + }) + // A tab that opened between the driver's last status message and its turn-end // never saw the run start, so it is idle — holding the conversation from // before the turn, and idle is the position that permits driving on it. diff --git a/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts b/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts index d582dee995..81c2a2ada1 100644 --- a/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts +++ b/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts @@ -103,6 +103,7 @@ import { } from './sessionSync.svelte' import { clearRunPosition, + currentRunId, isCatchingUp, isDriving, isWatching, @@ -956,12 +957,12 @@ async function initRuntime(runtime: SessionRuntime, session: Session) { // Stop is available wherever the run is visible, so from a watching tab it // has to travel to the one holding the turn. manager.remoteCancel = () => { - if (!isWatching(session.id)) return false - requestCancel(session.id) + const runId = currentRunId(session.id) + if (!isWatching(session.id) || !runId) return false + requestCancel(session.id, runId) return true } - await manager.historyManager.init() manager.historyManager.setSessionId(session.id) // Restore linked files persisted for this session (live handles re-grant on send; @@ -1001,8 +1002,11 @@ function postRunStatus(sessionId: string): void { const runtime = runtimes.get(sessionId) if (!runtime) return const m = runtime.manager + const runId = currentRunId(sessionId) + if (!runId) return broadcastRunStatus({ sessionId, + runId, loading: m.loading, compacting: m.compacting, blockedOnUser: pendingUserAction(m.displayMessages) !== undefined, @@ -1138,7 +1142,11 @@ function cancelCatchUpRetry(sessionId: string): void { } /** A Stop pressed in a watching tab reaches the run here. */ -function applyCancelRequest(sessionId: string): void { +function applyCancelRequest(sessionId: string, runId: string): void { + // Named for the turn it was pressed during. A Stop can be delivered after that + // turn ended, and the queue flush and job auto-resume both start the next one + // immediately, so an unqualified Stop would kill a turn nobody asked it to. + if (currentRunId(sessionId) !== runId) return if (!isDriving(sessionId)) return // No reason: this IS the user's Stop, just pressed elsewhere, and the // queued-message and rollback paths key off that. diff --git a/frontend/src/lib/components/sessions/sessionSync.svelte.ts b/frontend/src/lib/components/sessions/sessionSync.svelte.ts index fbd18ceac4..49fd184897 100644 --- a/frontend/src/lib/components/sessions/sessionSync.svelte.ts +++ b/frontend/src/lib/components/sessions/sessionSync.svelte.ts @@ -54,6 +54,9 @@ type TurnEndMsg = { kind: 'turn-end'; sessionId: string; chatId: string } type RunStatusMsg = { kind: 'run-status' sessionId: string + /** Identifies the turn. A watcher echoes it back on a control message so one + * delayed past the turn's end cannot act on the turn that followed. */ + runId: string loading: boolean compacting: boolean /** Parked on a question that only the driving tab can render. The watcher @@ -66,8 +69,10 @@ type RunStatusMsg = { * tab's memory. */ planModeActive: boolean } -/** A Stop pressed in a tab that is only watching the run. */ -type CancelRequestMsg = { kind: 'cancel-request'; sessionId: string } +/** A Stop pressed in a tab that is only watching the run. Names the run it was + * pressed for: turn-end and the next turn's start can both land inside the + * delivery gap, and the queue flush and job auto-resume start one immediately. */ +type CancelRequestMsg = { kind: 'cancel-request'; sessionId: string; runId: string } type SyncMsg = | SessionPutMsg | SessionDeleteMsg @@ -82,7 +87,7 @@ type Handlers = { onSessionArtifact: (sessionId: string, artifactId: string) => void onTurnEnd: (sessionId: string, chatId: string) => void onRunStatus: (msg: RunStatusMsg) => void - onCancelRequest: (sessionId: string) => void + onCancelRequest: (sessionId: string, runId: string) => void } const subscribers: Partial[] = [] @@ -154,11 +159,11 @@ function receive(msg: SyncMsg): void { emit('onTurnEnd', msg.sessionId, msg.chatId) break case 'run-status': - noteDriverAlive(msg.sessionId, msg.planModeActive) + noteDriverAlive(msg.sessionId, msg.planModeActive, msg.runId) emit('onRunStatus', msg) break case 'cancel-request': - emit('onCancelRequest', msg.sessionId) + emit('onCancelRequest', msg.sessionId, msg.runId) break } } @@ -197,8 +202,8 @@ export function broadcastTurnEnd(sessionId: string, chatId: string): void { post({ kind: 'turn-end', sessionId, chatId }) } -export function requestCancel(sessionId: string): void { - post({ kind: 'cancel-request', sessionId }) +export function requestCancel(sessionId: string, runId: string): void { + post({ kind: 'cancel-request', sessionId, runId }) } export type RunStatus = Omit