From 215f354bef95c8c97efc42002a0d992606caba91 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Thu, 27 Aug 2026 14:22:56 +0200 Subject: [PATCH] fix(ai-sessions): drop the driver probe and state the lockless limit plainly Co-Authored-By: Claude Opus 5 --- .../copilot/chat/HistoryManager.svelte.ts | 8 +-- .../sessions/sessionRunOwner.svelte.ts | 72 +++++-------------- .../sessions/sessionRunOwner.test.ts | 68 ++++++------------ .../sessions/sessionRuntime.svelte.ts | 4 -- .../components/sessions/sessionSync.svelte.ts | 30 +------- 5 files changed, 40 insertions(+), 142 deletions(-) diff --git a/frontend/src/lib/components/copilot/chat/HistoryManager.svelte.ts b/frontend/src/lib/components/copilot/chat/HistoryManager.svelte.ts index a8f32f7e93..dabaf96157 100644 --- a/frontend/src/lib/components/copilot/chat/HistoryManager.svelte.ts +++ b/frontend/src/lib/components/copilot/chat/HistoryManager.svelte.ts @@ -612,13 +612,9 @@ 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' | 'no-store'> { + async reloadChat(id: string): Promise<'loaded' | 'missing' | 'unavailable'> { const db = await this.dbh.whenReady() - // 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' + if (!db) return 'unavailable' try { const chat = await db.get('chats', id) if (!chat) return 'missing' diff --git a/frontend/src/lib/components/sessions/sessionRunOwner.svelte.ts b/frontend/src/lib/components/sessions/sessionRunOwner.svelte.ts index 72d9924fe6..bc7f74116f 100644 --- a/frontend/src/lib/components/sessions/sessionRunOwner.svelte.ts +++ b/frontend/src/lib/components/sessions/sessionRunOwner.svelte.ts @@ -79,7 +79,6 @@ 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,8 +101,6 @@ export function noteCaughtUp(sessionId: string): void { * down tab can't leave a position behind for a session nothing is watching. */ 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 @@ -157,61 +154,26 @@ export async function withSessionRunLock( } } -/** How long a probe waits for the driver to answer before its silence counts. - * Long enough to survive a busy main thread, short enough that the pre-flight - * is not felt; only origins without Web Locks ever pay it. */ -const PROBE_GRACE_MS = 750 - -let sendDriverProbe: ((sessionId: string) => void) | undefined - -/** Registered by sessionSync, which owns the channel this is asked over. */ -export function setDriverProbe(fn: (sessionId: string) => void): void { - sendDriverProbe = fn -} - -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. */ -export function noteDriverAnswered(sessionId: string): void { - probeAnswers.add(sessionId) -} - -/** Exclusion without a lock. Silence is not enough to conclude a driver is - * gone — a hidden tab's heartbeat is throttled to as little as once a minute - * 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. +/** What exclusion amounts to with no lock to take: refuse while another tab's + * run is visibly on screen, and otherwise go. * - * 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. */ + * This is deliberately not mutual exclusion, and cannot be made into it. A + * driver whose timers have been throttled in a hidden tab stops sending frames + * 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. `isMirroring` and not + * `isWatching`: a tab that has moved on to `catchingUp` is still owed its + * re-read, and driving from here would send the pre-run history that re-read + * exists to replace. */ async function bestEffort(sessionId: string, body: () => Promise): Promise { 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 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 7a1441d111..19bf5b9100 100644 --- a/frontend/src/lib/components/sessions/sessionRunOwner.test.ts +++ b/frontend/src/lib/components/sessions/sessionRunOwner.test.ts @@ -1,73 +1,45 @@ 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', 'session-never-shared'] +const SESSIONS = ['session-watching', 'session-catching-up', 'session-idle'] // `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. -// -// A driving tab that is hidden has its timers throttled to as little as once a -// minute, so its heartbeat stops long before its turn does and the reaper -// retires it. Concluding from that silence that the run is over starts a second -// turn against the same chat id: duplicate tool calls against the workspace, -// and whichever save lands last discards the other's transcript. +// The test env has no Web Locks API, which is the footing an origin served over +// plain HTTP runs on, so these exercise the fallback rather than the lock. 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)) + it('refuses while another tab is driving', async () => { + noteDriverAlive('session-watching', false) const body = vi.fn(async () => 'ran') - const outcome = await withSessionRunLock('session-throttled-driver', body) - - expect(outcome).toBe('busy') + expect(await withSessionRunLock('session-watching', body)).toBe('busy') expect(body).not.toHaveBeenCalled() }) - it('takes the run when nothing answers', async () => { - driverWasHereThenWentQuiet('session-driver-gone') - setDriverProbe(() => {}) + // The window after the driver's turn ends and before this tab has re-read what + // it left behind. The run is over, so a check for "someone is driving" says + // go — but the transcript on screen is still paired with the history from + // before that turn, and sending would put that pair to the model. + it('refuses while still catching up on a finished turn', async () => { + noteDriverAlive('session-catching-up', false) + noteRemoteTurnEnded('session-catching-up') const body = vi.fn(async () => 'ran') - const outcome = await withSessionRunLock('session-driver-gone', body) + expect(await withSessionRunLock('session-catching-up', body)).toBe('busy') + expect(body).not.toHaveBeenCalled() + }) - expect(outcome).toBe('ran') + it('takes a session no other tab holds', async () => { + const body = vi.fn(async () => 'ran') + + expect(await withSessionRunLock('session-idle', body)).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 ae54ff9465..27045d4fbe 100644 --- a/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts +++ b/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts @@ -1188,10 +1188,6 @@ 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 diff --git a/frontend/src/lib/components/sessions/sessionSync.svelte.ts b/frontend/src/lib/components/sessions/sessionSync.svelte.ts index 75b3f93917..057a97b0f6 100644 --- a/frontend/src/lib/components/sessions/sessionSync.svelte.ts +++ b/frontend/src/lib/components/sessions/sessionSync.svelte.ts @@ -1,13 +1,7 @@ import { BROWSER } from 'esm-env' import { onUserChange, scopedKey } from '$lib/userScopedStorage' import type { DisplayMessage } from '$lib/components/copilot/chat/shared' -import { - isDriving, - noteDriverAlive, - noteDriverAnswered, - noteRemoteTurnEnded, - setDriverProbe -} from './sessionRunOwner.svelte' +import { noteDriverAlive, noteRemoteTurnEnded } from './sessionRunOwner.svelte' // The channel between tabs open on the same AI session. Everything a session is // made of — the record list, the chat transcript, the run itself — lives in the @@ -87,13 +81,6 @@ type QuestionAnswerMsg = { choices: string[] } -/** "Is anyone still running this?", asked by a tab about to drive where no lock - * can answer for it. Delivery is a task rather than a timer, so a driver whose - * timers have been throttled to a crawl in a hidden tab still receives it and - * still answers — which is exactly the case silence alone gets wrong. */ -type DriverProbeMsg = { kind: 'driver-probe'; sessionId: string } -type DriverAliveMsg = { kind: 'driver-alive'; sessionId: string } - type SyncMsg = | SessionPutMsg | SessionDeleteMsg @@ -104,8 +91,6 @@ type SyncMsg = | CancelRequestMsg | ToolConfirmationMsg | QuestionAnswerMsg - | DriverProbeMsg - | DriverAliveMsg type Handlers = { onSessionPut: (id: string) => void @@ -203,15 +188,6 @@ function receive(msg: SyncMsg): void { case 'question-answer': emit('onQuestionAnswer', msg.sessionId, msg.toolId, msg.choices) break - case 'driver-probe': - // Answered from inside the handler, never off a timer: a throttled tab - // still runs tasks, and routing the answer through a timeout would make - // it look dead for exactly the reason the probe exists to rule out. - if (isDriving(msg.sessionId)) post({ kind: 'driver-alive', sessionId: msg.sessionId }) - break - case 'driver-alive': - noteDriverAnswered(msg.sessionId) - break } } @@ -245,10 +221,6 @@ export function broadcastSessionArtifact(sessionId: string, artifactId: string): // Live mirroring // --------------------------------------------------------------------------- -// sessionRunOwner decides when a probe is worth sending; posting one is this -// module's job, and registering it here is what keeps the two out of a cycle. -setDriverProbe((sessionId) => post({ kind: 'driver-probe', sessionId })) - export function broadcastTurnEnd(sessionId: string, chatId: string): void { post({ kind: 'turn-end', sessionId, chatId }) }