mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-21 00:02:30 +00:00
fix(ai-sessions): unwind a refused send completely and stop claiming a delete barrier
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
e6f1137257
commit
2fa8b5e8ec
@@ -1949,6 +1949,14 @@ export class AIChatManager {
|
||||
// a Retry that is refused charges them against the conversation's budget
|
||||
// for the lifetime of the manager.
|
||||
this.#releaseOutgoingReservation(options.resendReservationKey)
|
||||
// A synthetic send carries none of the user's text, but the auto-resume set
|
||||
// `this.instructions` before calling it and only sendRequestImpl clears
|
||||
// them. Its own arming check bails while they are non-empty, so leaving
|
||||
// them here disarms every later auto-resume for this session.
|
||||
if (options.synthetic) {
|
||||
this.instructions = ''
|
||||
return
|
||||
}
|
||||
if (options.queued) return
|
||||
const restored = this.aiChatInput?.restoreInstructions(
|
||||
options.instructions ?? '',
|
||||
@@ -1957,9 +1965,15 @@ export class AIChatManager {
|
||||
options.files ?? []
|
||||
)
|
||||
// No composer mounted (a programmatic send): park it on the queue so it is
|
||||
// still the user's to send rather than silently gone.
|
||||
// still the user's to send rather than silently gone. The queue holds plain
|
||||
// text, so the pastes are expanded into it — parked as bare tokens they
|
||||
// would point at blobs nothing holds any more.
|
||||
if (restored !== true && options.instructions) {
|
||||
this.restoreToInput(options.instructions, options.images, options.files)
|
||||
this.restoreToInput(
|
||||
expanded(chatDraft(options.instructions, options.pastes ?? [])),
|
||||
options.images,
|
||||
options.files
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -420,6 +420,24 @@ 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
|
||||
// leaves them set, and the arming check bails while they are non-empty — so
|
||||
// one refused resume disarms every later one for the session.
|
||||
it('leaves nothing behind when a synthetic auto-resume is refused', async () => {
|
||||
const manager = new AIChatManager()
|
||||
manager.isSessionChat = true
|
||||
manager.sessionId = 'session-synthetic-refusal'
|
||||
onDriverLost(() => {})
|
||||
noteDriverAlive('session-synthetic-refusal', false)
|
||||
manager.instructions = 'A background job just finished.'
|
||||
|
||||
await manager.sendRequest({ synthetic: true })
|
||||
|
||||
expect(mocks.runChatLoop).not.toHaveBeenCalled()
|
||||
expect(manager.instructions).toBe('')
|
||||
})
|
||||
|
||||
// These senders have no draft of their own, so a refusal that keeps nothing
|
||||
// loses what the user typed.
|
||||
it('hands a programmatic prompt back instead of queueing it for another tab', () => {
|
||||
|
||||
@@ -588,11 +588,12 @@ export default class HistoryManager {
|
||||
this.pruneImageIds(this.currentChatId)
|
||||
}
|
||||
|
||||
/** Returns once the row is actually gone, for the one caller that has to know:
|
||||
* a rolled-back turn announces its end to the other tabs, and they re-read
|
||||
* this chat from the store. Dropped in flight, that read still finds the
|
||||
* transcript the rollback exists to remove. Everywhere else the removal is
|
||||
* visible from `savedChats` at once and the promise can be ignored. */
|
||||
/** Returns once the delete has had its turn on the write queue, which orders
|
||||
* it before a rolled-back turn announces its end and the other tabs re-read
|
||||
* this chat. Ordering, not a guarantee: like every write here it is dropped
|
||||
* when the store is unavailable, so a watcher whose own read still succeeds
|
||||
* can find the transcript the rollback meant to remove. Everywhere else the
|
||||
* removal shows in `savedChats` at once and the promise can be ignored. */
|
||||
deletePastChat(id: string): Promise<void> {
|
||||
this.savedChats = Object.fromEntries(
|
||||
Object.entries(this.savedChats).filter(([key]) => key !== id)
|
||||
|
||||
@@ -83,24 +83,12 @@ export function noteDriverAlive(sessionId: string, planMode: boolean): void {
|
||||
ensureReaper()
|
||||
}
|
||||
|
||||
/** The driver says its turn is over. The re-read that follows is what actually
|
||||
* frees this tab, so the position moves to `catchingUp` rather than to idle —
|
||||
* but only where something can perform that re-read.
|
||||
*
|
||||
* The channel calls this from module scope in every tab that has it, including
|
||||
* ones that never load sessionRuntime (a page carrying the session sidebar with
|
||||
* no session open). Only sessionRuntime can call {@link noteCaughtUp}, so
|
||||
* entering `catchingUp` there would be entering a state with nothing able to
|
||||
* leave it — and `mirroringRemoteRun` reads true for as long as it lasts, which
|
||||
* locks the composer against a run that has already ended and drops the edits
|
||||
* mask and background-job writes that expect the re-read to reseed them. With
|
||||
* no runtime there is no mirrored transcript to be out of step with, so idle is
|
||||
* the truthful position rather than merely the convenient one. */
|
||||
/** 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
|
||||
* no runtime would sit in a state nothing can leave. */
|
||||
export function noteRemoteTurnEnded(sessionId: string): void {
|
||||
if (runPosition(sessionId).state !== 'watching') return
|
||||
// Deleted rather than set to idle: a missing entry already reads as idle, and
|
||||
// the tab this branch exists for has no runtime, so nothing in it would ever
|
||||
// call clearRunPosition to take the entry back out again.
|
||||
if (!canCompleteCatchUp()) {
|
||||
positions.delete(sessionId)
|
||||
return
|
||||
@@ -232,9 +220,8 @@ export function onDriverLost(fn: (sessionId: string) => void): void {
|
||||
driverLost = fn
|
||||
}
|
||||
|
||||
/** Whether a module that can finish a catch-up is loaded in this tab. It is the
|
||||
* same registration either way: sessionRuntime owns both the re-read and the
|
||||
* handler, so having one means having the other. */
|
||||
/** sessionRuntime owns both the re-read and this handler, so registering one
|
||||
* means having the other. */
|
||||
function canCompleteCatchUp(): boolean {
|
||||
return driverLost !== undefined
|
||||
}
|
||||
|
||||
@@ -46,13 +46,7 @@ describe('withSessionRunLock with no lock to take', () => {
|
||||
})
|
||||
})
|
||||
|
||||
// The channel runs these transitions in every tab that loads it, including ones
|
||||
// that never load sessionRuntime — a page carrying the session sidebar with no
|
||||
// session open. Only sessionRuntime performs the re-read that leaves
|
||||
// `catchingUp`, so parking there would strand the session: `mirroringRemoteRun`
|
||||
// stays true, which locks the composer against a run that already ended and
|
||||
// drops the edits-mask and background-job writes that expect the re-read to
|
||||
// reseed them. A fresh module instance is the only honest way to test it — the
|
||||
// A fresh module instance is the only honest way to test this: the runtime's
|
||||
// registration is process-wide and one-way, exactly as it is in a real tab.
|
||||
describe('a turn ending in a tab with no session runtime', () => {
|
||||
it('settles to idle rather than waiting for a re-read nobody can do', async () => {
|
||||
|
||||
Reference in New Issue
Block a user