From 836b3b779bc1b91c85485cf8e01659dd0f8e94ea Mon Sep 17 00:00:00 2001 From: hugocasa Date: Thu, 27 Aug 2026 13:37:09 +0200 Subject: [PATCH] fix(ai-sessions): guard runtime-init sends and keep unreadable stores from looking empty Co-Authored-By: Claude Opus 5 --- .../copilot/chat/HistoryManager.svelte.ts | 8 +++-- .../copilot/chat/artifacts/artifactsDB.ts | 20 ++++++++++-- .../chat/artifacts/artifactsState.svelte.ts | 14 ++++++--- .../sessions/sessionRunOwner.svelte.ts | 30 +++++++++++++++--- .../sessions/sessionRunOwner.test.ts | 31 ++++++++++++++++++- .../sessions/sessionRuntime.svelte.ts | 23 ++++++++++---- 6 files changed, 105 insertions(+), 21 deletions(-) diff --git a/frontend/src/lib/components/copilot/chat/HistoryManager.svelte.ts b/frontend/src/lib/components/copilot/chat/HistoryManager.svelte.ts index dabaf96157..a8f32f7e93 100644 --- a/frontend/src/lib/components/copilot/chat/HistoryManager.svelte.ts +++ b/frontend/src/lib/components/copilot/chat/HistoryManager.svelte.ts @@ -612,9 +612,13 @@ export default class HistoryManager { * 'unavailable' is a fact about this browser. Callers act on the first and * must not act on the second — treating a closed database as an empty chat * would throw away a transcript that is merely unreadable right now. */ - async reloadChat(id: string): Promise<'loaded' | 'missing' | 'unavailable'> { + async reloadChat(id: string): Promise<'loaded' | 'missing' | 'unavailable' | 'no-store'> { const db = await this.dbh.whenReady() - if (!db) return 'unavailable' + // No database at all (disabled, private mode, a failed open) rather than a + // read that happened to fail. Worth distinguishing: a caller waiting for a + // readable store can wait forever on this one, and there is nothing stored + // for it to be out of step with either. + if (!db) return 'no-store' try { const chat = await db.get('chats', id) if (!chat) return 'missing' diff --git a/frontend/src/lib/components/copilot/chat/artifacts/artifactsDB.ts b/frontend/src/lib/components/copilot/chat/artifacts/artifactsDB.ts index 94afac404e..5646640fe3 100644 --- a/frontend/src/lib/components/copilot/chat/artifacts/artifactsDB.ts +++ b/frontend/src/lib/components/copilot/chat/artifacts/artifactsDB.ts @@ -124,13 +124,27 @@ export async function putArtifact(artifact: PersistedArtifact): Promise { } export async function getArtifact(id: string): Promise { + const read = await readArtifact(id) + return read.state === 'loaded' ? read.artifact : undefined +} + +/** A read whose failure is distinguishable from an absence, for the one caller + * that acts on "the row is gone". Collapsing the two into `undefined` — which + * is all {@link getArtifact} can say — turns a moment of unreadable storage + * into a deletion. */ +export async function readArtifact( + id: string +): Promise< + { state: 'loaded'; artifact: PersistedArtifact } | { state: 'missing' } | { state: 'unavailable' } +> { const db = await getDB() - if (!db) return undefined + if (!db) return { state: 'unavailable' } try { - return await db.get('items', id) + const artifact = await db.get('items', id) + return artifact ? { state: 'loaded', artifact } : { state: 'missing' } } catch (err) { console.error('Could not read artifact', err) - return undefined + return { state: 'unavailable' } } } diff --git a/frontend/src/lib/components/copilot/chat/artifacts/artifactsState.svelte.ts b/frontend/src/lib/components/copilot/chat/artifacts/artifactsState.svelte.ts index bce91c5adf..ef1f673227 100644 --- a/frontend/src/lib/components/copilot/chat/artifacts/artifactsState.svelte.ts +++ b/frontend/src/lib/components/copilot/chat/artifacts/artifactsState.svelte.ts @@ -5,6 +5,7 @@ import { deleteArtifact, getArtifact, getArtifactVersion, + readArtifact, isPlanArtifact, listArtifactVersions, listArtifactsForSession, @@ -136,20 +137,25 @@ export class SessionArtifactsStore { * record does and for the same reason: delivery order and IndexedDB commit order are * independent, so a shipped copy could be older than what the store already holds. * - * A read that comes back empty is how a removal arrives. It can only drop an id this list + * A row that is genuinely absent is how a removal arrives. It can only drop an id this list * already has, so an artifact held here after a failed persist — which is exactly what the * same-id resync guard in setSession protects — is never one of them: the other tab has * nothing to say about a row it has never seen. + * + * Storage that cannot be read says nothing about the row and is left alone. Treating it as + * an absence would take a plan off screen mid-approval because a read blipped; the next + * write to that artifact broadcasts again and converges. */ async applyRemoteArtifact(artifactId: string): Promise { const loaded = this.#sessionId if (!loaded) return - const stored = await getArtifact(artifactId) + const read = await readArtifact(artifactId) // Read once the await settles: a session switched underneath it must not have the // previous one's row placed into its list, nor one of its own rows dropped. if (this.#sessionId !== loaded) return - if (stored) { - this.#place(stored) + if (read.state === 'unavailable') return + if (read.state === 'loaded') { + this.#place(read.artifact) return } const next = this.artifacts.filter((a) => a.id !== artifactId) diff --git a/frontend/src/lib/components/sessions/sessionRunOwner.svelte.ts b/frontend/src/lib/components/sessions/sessionRunOwner.svelte.ts index f9cd2163df..72d9924fe6 100644 --- a/frontend/src/lib/components/sessions/sessionRunOwner.svelte.ts +++ b/frontend/src/lib/components/sessions/sessionRunOwner.svelte.ts @@ -79,6 +79,7 @@ export function noteDriverAlive(sessionId: string, planMode: boolean): void { // A tab mid-turn is the authority on its own session; a frame reaching it can // only be an echo of the run it is itself driving. if (isDriving(sessionId)) return + seenDriver.add(sessionId) positions.set(sessionId, { state: 'watching', lastHeardAt: Date.now(), planMode }) ensureReaper() } @@ -102,6 +103,7 @@ export function noteCaughtUp(sessionId: string): void { export function clearRunPosition(sessionId: string): void { positions.delete(sessionId) probeAnswers.delete(sessionId) + seenDriver.delete(sessionId) } /** True from the driver's turn-end until this tab has re-read what it left @@ -169,6 +171,11 @@ export function setDriverProbe(fn: (sessionId: string) => void): void { const probeAnswers = new Set() +/** Sessions this tab has ever seen driven elsewhere. Only these are worth + * probing before taking a lockless run; it is never cleared, because "there was + * another tab once" stays the reason to ask. */ +const seenDriver = new Set() + /** A driver answered a probe. Recorded rather than folded into the position: * the answer proves a run is alive, but carries none of the frame state a * `watching` position is made of. */ @@ -181,16 +188,29 @@ export function noteDriverAnswered(sessionId: string): void { * while its turn runs on perfectly well, and the reaper will have retired it * long before that. So ask, and treat only an unanswered probe as absence. * Getting this wrong runs a second turn against the same chat id, which is the - * duplicate tool calls and lost transcript this whole module exists to stop. */ + * duplicate tool calls and lost transcript this whole module exists to stop. + * + * This narrows the window rather than closing it, and it cannot close it: a tab + * the browser has frozen runs no script at all, so it answers a probe exactly + * the way a closed one does and nothing over this channel can tell them apart. + * Mutual exclusion needs Web Locks; where there is none, the honest guarantee + * is best-effort — still strictly more than the nothing a session had before + * any of this, and the reason the lock is what secure origins rely on. */ async function bestEffort(sessionId: string, body: () => Promise): Promise { - if (isWatching(sessionId)) return 'busy' - if (sendDriverProbe) { + if (isMirroring(sessionId)) return 'busy' + // Only worth asking where a driver has actually been seen. A session this tab + // has had to itself has nobody to answer, and paying the grace on every send + // would tax the single-tab case — the common one — for a race it cannot be in. + if (sendDriverProbe && seenDriver.has(sessionId)) { probeAnswers.delete(sessionId) sendDriverProbe(sessionId) await new Promise((resolve) => setTimeout(resolve, PROBE_GRACE_MS)) if (probeAnswers.delete(sessionId)) return 'busy' - // A frame may also have arrived while the probe was outstanding. - if (isWatching(sessionId)) return 'busy' + // A frame or a turn-end may also have landed while the probe was out. + // `isMirroring`, not `isWatching`: a tab that has moved on to `catchingUp` + // is owed a re-read, and driving from here would drop it and send the + // pre-run history the re-read exists to replace. + if (isMirroring(sessionId)) return 'busy' } return await drive(sessionId, body) } diff --git a/frontend/src/lib/components/sessions/sessionRunOwner.test.ts b/frontend/src/lib/components/sessions/sessionRunOwner.test.ts index 591e882131..7a1441d111 100644 --- a/frontend/src/lib/components/sessions/sessionRunOwner.test.ts +++ b/frontend/src/lib/components/sessions/sessionRunOwner.test.ts @@ -1,17 +1,29 @@ import { afterEach, describe, expect, it, vi } from 'vitest' import { clearRunPosition, + noteCaughtUp, + noteDriverAlive, noteDriverAnswered, + noteRemoteTurnEnded, setDriverProbe, withSessionRunLock } from './sessionRunOwner.svelte' -const SESSIONS = ['session-throttled-driver', 'session-driver-gone'] +const SESSIONS = ['session-throttled-driver', 'session-driver-gone', 'session-never-shared'] // `positions` is module state and a watching entry keeps the reaper interval // armed, so leave nothing behind for the next suite. afterEach(() => SESSIONS.forEach(clearRunPosition)) +/** The sequence a watcher goes through when its driver stops sending frames: + * seen driving, then reaped back to idle. The tab is now free to send, and + * whether it may is the question the probe answers. */ +function driverWasHereThenWentQuiet(sessionId: string) { + noteDriverAlive(sessionId, false) + noteRemoteTurnEnded(sessionId) + noteCaughtUp(sessionId) +} + // Under the test env there is no Web Locks API, which is the same footing a // self-hosted instance served over plain HTTP runs on — so these exercise the // path that has nothing but the channel to arbitrate with. @@ -23,6 +35,7 @@ afterEach(() => SESSIONS.forEach(clearRunPosition)) // and whichever save lands last discards the other's transcript. describe('withSessionRunLock with no lock to take', () => { it('refuses the run when a driver answers the probe', async () => { + driverWasHereThenWentQuiet('session-throttled-driver') setDriverProbe((sessionId) => noteDriverAnswered(sessionId)) const body = vi.fn(async () => 'ran') @@ -33,6 +46,7 @@ describe('withSessionRunLock with no lock to take', () => { }) it('takes the run when nothing answers', async () => { + driverWasHereThenWentQuiet('session-driver-gone') setDriverProbe(() => {}) const body = vi.fn(async () => 'ran') @@ -41,4 +55,19 @@ describe('withSessionRunLock with no lock to take', () => { expect(outcome).toBe('ran') expect(body).toHaveBeenCalledTimes(1) }) + + // The single-tab case, which is the common one: nobody has ever driven this + // session from anywhere else, so there is no one to answer and waiting out + // the grace on every send would be a tax paid for a race that cannot happen. + it('does not wait on a probe for a session it has never shared', async () => { + const probe = vi.fn() + setDriverProbe(probe) + const started = Date.now() + + const outcome = await withSessionRunLock('session-never-shared', async () => 'ran') + + expect(outcome).toBe('ran') + expect(probe).not.toHaveBeenCalled() + expect(Date.now() - started).toBeLessThan(300) + }) }) diff --git a/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts b/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts index f42c93b923..ae54ff9465 100644 --- a/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts +++ b/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts @@ -924,13 +924,13 @@ function createRuntime(session: Session): SessionRuntime { async function initRuntime(runtime: SessionRuntime, session: Session) { const { manager } = runtime - await manager.historyManager.init() - manager.historyManager.setSessionId(session.id) - // Restore linked files persisted for this session (live handles re-grant on send; - // snapshots restore directly). Non-transient sessions persist immediately. - await manager.attachedFiles.restore(session.id, !session.transient) - await ensureChatIdsSeeded(manager.historyManager) + // Wired before the first await, not after. `getOrCreateRuntime` does not wait + // for this function, so the manager is mounted and its composer live while the + // restores below are still in flight — and a send in that window would find no + // guard and start a turn without ever asking who owns the run. None of these + // depend on what the awaits produce. + // // Keep the session record's chatId following the manager's active chat: a // "/clear" rotation or a history switch would otherwise leave it pointing at // the previous chat, and the compare-page handoff (`from_session`) would @@ -981,6 +981,13 @@ async function initRuntime(runtime: SessionRuntime, session: Session) { return true } + await manager.historyManager.init() + manager.historyManager.setSessionId(session.id) + // Restore linked files persisted for this session (live handles re-grant on send; + // snapshots restore directly). Non-transient sessions persist immediately. + await manager.attachedFiles.restore(session.id, !session.transient) + await ensureChatIdsSeeded(manager.historyManager) + if (session.chatId) { manager.historyManager.setCurrentChatId(session.chatId) await manager.historyManager.tagChatWithSession(session.chatId, session.id) @@ -1181,6 +1188,10 @@ async function applyTurnEnd(sessionId: string, chatId: string, attempt = 0): Pro scheduleCatchUpRetry(sessionId, chatId, attempt) return } + // There is no store to catch up from, and no retry that would change that. + // Nothing was saved for this tab to be out of step with either, so holding + // the composer shut would cost the session for no gain. + if (found === 'no-store') caughtUp = true if (found === 'loaded') { // `refresh`: the same conversation caught up from the store, so whatever // this manager still holds for it — a queued message, the background-job