mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-21 00:02:30 +00:00
fix(ai-sessions): do not re-announce a delete this tab is mirroring
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
2936544e37
commit
d441f3c5a3
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -187,21 +187,13 @@ export async function withSessionRunLock<T>(
|
||||
}
|
||||
}
|
||||
|
||||
/** 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<T>(sessionId: string, body: () => Promise<T>): Promise<T | 'busy'> {
|
||||
return await drive(sessionId, body)
|
||||
}
|
||||
|
||||
@@ -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<string, { from: number; prompt?: string }>()
|
||||
|
||||
function currentRunPrompt(sessionId: string, messages: DisplayMessage[]): string | undefined {
|
||||
|
||||
@@ -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<SessionSchema>, id: string): Promise<void> {
|
||||
async function deleteSessionRow(
|
||||
db: IDBPDatabase<SessionSchema>,
|
||||
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<void> {
|
||||
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<void> {
|
||||
}
|
||||
}
|
||||
|
||||
export async function deleteSessionRecord(id: string): Promise<void> {
|
||||
export async function deleteSessionRecord(id: string, announce = true): Promise<void> {
|
||||
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({
|
||||
|
||||
@@ -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<typeof import('./sessionSync.svelte')>()),
|
||||
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<typeof import('$lib/gen')>()
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user