Skip to content

Commit a2111db

Browse files
DavidBabinecDavidBabinec
authored
fix(agent): recover interrupted browser tool turns (CoreBunch#210)
1 parent 4c8a426 commit a2111db

26 files changed

Lines changed: 1252 additions & 138 deletions

‎docs/features/agent.md‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -721,7 +721,7 @@ The server admits only one active writer per conversation. A concurrent tab rece
721721

722722
**User attachments are not tool screenshots.** A pasted image is a persisted `kind:'image'` block on a user message. Conversation-detail responses, authenticated image responses, and chat streams use `Cache-Control: private, no-store`; the database remains the intentional durable copy. Images returned by `site_render_snapshot` or another browser tool instead travel transiently on the plural `AiToolOutput.images` channel, are subject to the heavy-evidence rules above, and remain session-only even though the panel exposes every returned image through the same gallery and draggable preview. The two paths share provider-native image mapping but have different storage and replay lifecycles.
723723

724-
**Tool outcomes are first-class.** A `role:'tool'` row records its result as a `{ kind: 'toolResult', ok, error? }` block — `ok` is an explicit boolean, never inferred from the emptiness of a text block. The persister writes it (`appendToolResult`), `buildMessageHistory` reads `ok`/`error` straight off the block to reconstruct the replay `AiToolOutput`, and the client folds it back into the matching tool-call badge (`rehydrateMessages`). The heavy successful `data` an `AiToolOutput` may carry is intentionally **not** persisted: the model already consumed it in the round that produced the result, so replay only needs `{ ok, error }` — re-feeding large tool payloads every turn would bloat the context for no benefit.
724+
**Tool outcomes are first-class.** A `role:'tool'` row records its result as a `{ kind: 'toolResult', ok, error? }` block — `ok` is an explicit boolean, never inferred from the emptiness of a text block. The persister writes it (`appendToolResult`), `buildMessageHistory` reads `ok`/`error` straight off the block to reconstruct the replay `AiToolOutput`, and the client folds it back into the matching tool-call badge (`rehydrateMessages`). A loaded conversation never owns a live bridge from its previous process: any persisted call without a matching valid result is finalized as `INTERRUPTED_TOOL_RESULT_ERROR`, never restored as pending. The heavy successful `data` an `AiToolOutput` may carry is intentionally **not** persisted: the model already consumed it in the round that produced the result, so replay only needs `{ ok, error }` — re-feeding large tool payloads every turn would bloat the context for no benefit.
725725

726726
---
727727

@@ -767,16 +767,13 @@ unblocks deletion of the credential that had been protected by the default FK.
767767

768768
## Abort + crash recovery
769769

770-
- **Abort.** "Stop" calls `agentSlice.abortAgent()` → `AbortController.abort()` → the fetch stream closes. When the abort signal fires on the server:
771-
- `req.signal` is passed straight to every `fetch()` call in the driver loop (`fetch(endpoint, { signal })`). The in-flight HTTP request to the provider is cancelled immediately — no further tokens are generated or billed. On `AbortError` the loop returns cleanly with no `error` event.
772-
- Any `callBrowser` promise still waiting for a browser tool-result rejects via the `onAbort` listener registered per pending call (in `server/ai/runtime/transport.ts`). The listener fires, clears the timeout, and removes the pending entry.
773-
- The stream's `destroy()` hook fires, rejects any remaining pending entries, and removes the bridge from the registry.
774-
- **Interrupted tool calls.** If a stream aborts mid-turn — between the assistant's `tool_use` row write and the matching `tool_result` row write (e.g. `ERR_INCOMPLETE_CHUNKED_ENCODING`, server restart) — the persisted history has an unanswered `tool_use` block. `buildMessageHistory` in `server/ai/conversations/history.ts` heals the gap: every tool-call id that has no persisted `tool` result row gets a synthetic error result (`INTERRUPTED_TOOL_RESULT_ERROR`) injected before the next user turn. The model reads the error and can retry; the conversation is never permanently un-sendable. Adjacent synthetic results plus the following real user prompt are merged into one user turn by `pushUserContent` in `server/ai/drivers/anthropic.ts`, satisfying Anthropic's strict user/assistant alternation requirement.
775-
- **Browser tool timeout.** If the browser never POSTs a tool-result, `callBrowser` rejects after 90 seconds (`BROWSER_TOOL_TIMEOUT_MS` in `server/ai/runtime/transport.ts`). The driver sees a rejection, emits an error, and the stream closes. This prevents a closed or unresponsive tab from hanging the tool loop indefinitely.
770+
- **Abort owns the whole response.** "Stop" calls `agentSlice.abortAgent()` and aborts the chat fetch. The server also owns a response-lifecycle controller: `ReadableStream.cancel()` or a failed `controller.enqueue()` aborts the same turn even when the original request signal does not observe a disappearing response consumer. `AbortSignal.any()` threads that combined signal into the provider request and browser bridge, and the handler's `finally` destroys the bridge and releases the per-conversation writer lock.
771+
- **Pending calls become terminal.** `runChat` persists `INTERRUPTED_TOOL_RESULT_ERROR` for every declared tool call still unresolved on a graceful abort or terminal driver event. A hard process stop can still land between those two writes, so both recovery projections enforce the same invariant: `buildMessageHistory` injects a synthetic error for provider replay, while `rehydrateMessages` renders an unmatched or malformed call as a failed historical badge. `pending` therefore means only work owned by the current live stream; a reload never shows an old spinner. Adjacent synthetic results plus the following real user prompt are merged into one user turn by `pushUserContent` in `server/ai/drivers/anthropic.ts`, satisfying Anthropic's strict user/assistant alternation requirement.
772+
- **Browser bridge failures are terminal once.** A browser executor resolving `{ ok: false }` remains an ordinary model-correctable tool outcome. A rejected `callBrowser` is transport failure instead: the loop emits exactly one failed `toolResult`, then one terminal `error`, and does not spend another provider round retrying against the same dead bridge. A missing result still has a 90-second upper bound (`BROWSER_TOOL_TIMEOUT_MS`), but it now ends the turn rather than starting a chain of 90-second retries.
776773
- **Crash on server.** If `runChat` throws, the stream emits `{ type: 'error', message }`. The browser surfaces the message verbatim in the Agent Panel (admin-only surface, so info-disclosure is not a concern).
777774
- **Tool failure.** Browser executors wrap every call in try/catch. Failures return `{ ok: false, error }`. The model reads the error message in the next turn and retries with corrected input.
778-
- **Bridge-result POST after abort.** If the browser POSTs a tool-result after the stream has closed, the server returns 404 and drops the result silently.
779-
- **Page reload mid-stream.** The stream dies. The conversation row and its persisted messages survive. The user can reload the past thread via `loadAgentConversation` and re-send.
775+
- **Tool-result delivery failure.** A 404 means the browser completed work for a bridge the active runtime no longer owns (commonly a server restart). While the chat signal is active, `postToolResult` propagates that failure, the client aborts the stale response, finalizes its pending badge, and asks the user to send again. Only a POST already being torn down by an aborted signal is ignored quietly. A clean NDJSON EOF without `done` or `error` is handled the same way instead of being mistaken for success.
776+
- **Page reload mid-stream.** The response cancel hook aborts the provider and releases the writer lock. Conversation rows survive; loading the thread shows any unmatched call once as interrupted, with no reconstructed session-only screenshot and no live timeout/spinner.
780777

781778
---
782779

@@ -828,7 +825,7 @@ unblocks deletion of the credential that had been protected by the default FK.
828825
- `server/ai/handlers/chat.ts` — `POST /admin/api/ai/chat/:scope` endpoint
829826
- `server/ai/handlers/conversations.ts` — conversation CRUD plus the ownership-guarded lazy image endpoint
830827
- `server/ai/handlers/toolResult.ts` — `POST /admin/api/ai/tool-result` endpoint
831-
- `server/ai/conversations/history.ts` — `buildMessageHistory()` + `INTERRUPTED_TOOL_RESULT_ERROR` (heals interrupted tool calls)
828+
- `src/core/ai/toolOutput.ts` — canonical `AiToolOutput` envelope + shared `INTERRUPTED_TOOL_RESULT_ERROR`
832829
- `server/ai/conversations/store.ts` — `appendMessage`, `listMessagesForConversation`, `readConversationForUser`
833830
- `server/ai/runtime/runner.ts` — `runChat()` driver loop
834831
- `server/ai/contextTokens.ts` — `normalizeContextTokens()` — provider-normalised "context used" for the meter
@@ -838,6 +835,9 @@ unblocks deletion of the credential that had been protected by the default FK.
838835
- `server/ai/runtime/persister.ts` — `ConversationsPersister` interface + `createConversationsPersister()`
839836
- `server/ai/runtime/types.ts` — canonical `AiStreamEvent`, `AiMessage`, `AiTool`, `ToolContext` types
840837
- `server/ai/runtime/transport.ts` — `createBridge()` / `resolveBridgeToolResult()`
838+
- `src/admin/ai/toolResultApi.ts` — browser tool-result delivery; active failures terminate the stale chat turn
839+
- `src/admin/pages/site/agent/agentApi.ts` — conversation bootstrap + terminal historical tool-call rehydration
840+
- `src/admin/pages/site/agent/toolCallLifecycle.ts` — live-stream pending-call finalization
841841
- `server/ai/audit/store.ts` — `getUsageTotals`, `getUsageByUser`, `getUsageByScope`, `getUsageByModel`, `getUsageByDay` (usage rollup queries)
842842
- `server/ai/handlers/audit.ts` — `GET /admin/api/ai/audit` handler
843843
- `server/time.ts` — `resolveTimeZone` + `localDayKeyFactory` (shared timezone day-bucketing utilities)
@@ -848,7 +848,7 @@ unblocks deletion of the credential that had been protected by the default FK.
848848
- `src/admin/pages/site/agent/agentSlice.ts` — scope-agnostic slice factory (`createAgentSlice`)
849849
- `src/admin/pages/site/agent/agentProviderUpdate.ts` — timed provider/model update and ambiguous-commit reconciliation
850850
- `src/admin/pages/site/agent/agentSliceConfig.site.ts` — site-editor scope config
851-
- `src/admin/pages/site/agent/agentApi.ts` — tool-result POST, conversation bootstrap, message rehydration
851+
- `src/admin/pages/site/agent/agentApi.ts` — conversation bootstrap and message rehydration
852852
- `src/admin/pages/site/agent/streamEvents.ts` — `ServerStreamEventSchema` + `processStreamEvent`
853853
- `src/admin/pages/site/panels/AgentPanel/AgentImageGallery.tsx` — shared compact gallery for persisted and session-only images
854854
- `src/admin/pages/site/panels/AgentPanel/AgentImagePreview.tsx` — draggable modeless image preview

‎server/ai/conversations/history.ts‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -23,16 +23,10 @@
2323
* preceding tool call in the replayed history.
2424
*/
2525

26+
import { INTERRUPTED_TOOL_RESULT_ERROR } from '@core/ai'
2627
import type { AiContentBlock, AiMessage, AiToolOutput } from '../runtime/types'
2728
import type { MessageRecord } from './types'
2829

29-
/**
30-
* Error surfaced to the model for a tool call whose result was never persisted
31-
* because the turn was interrupted before it completed.
32-
*/
33-
export const INTERRUPTED_TOOL_RESULT_ERROR =
34-
'Tool call did not complete — the previous turn was interrupted before a result was produced.'
35-
3630
export const NON_VISION_USER_IMAGE_OMITTED =
3731
'[Attached image omitted because the selected model does not support image input.]'
3832

‎server/ai/drivers/http/execTool.ts‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,13 @@ import type {
2323
import type { ToolContextBase } from '../types'
2424

2525
/**
26-
* Execute one tool call and return the canonical `AiToolOutput`. Never throws —
27-
* validation, handler, and bridge failures all collapse to `{ ok: false, error }`
28-
* so the loop can feed the failure back to the model and let it recover.
26+
* Execute one tool call and return the canonical `AiToolOutput`.
27+
*
28+
* Input, permission, and server-handler failures are ordinary tool outcomes:
29+
* the loop feeds `{ ok: false, error }` back to the model so it can recover.
30+
* A rejected browser bridge is different — no result can reach the active
31+
* turn, so that infrastructure failure propagates to the loop and terminates
32+
* the turn instead of inviting the model to retry against the same dead bridge.
2933
*/
3034
export async function executeAiTool(
3135
aiTool: AiTool,
@@ -64,12 +68,9 @@ export async function executeAiTool(
6468
}
6569

6670
// Browser execution: forward to the bridge and wait for the POST-back.
67-
try {
68-
return await bridge.callBrowser(aiTool.name, validated)
69-
} catch (err) {
70-
const message = err instanceof Error ? err.message : `Tool ${aiTool.name} failed.`
71-
return { ok: false, error: message }
72-
}
71+
// A resolved `{ ok: false }` remains a recoverable domain failure. Rejection
72+
// means the transport itself is unavailable and deliberately propagates.
73+
return await bridge.callBrowser(aiTool.name, validated)
7374
}
7475

7576
/**

‎server/ai/drivers/http/toolLoop.ts‎

Lines changed: 39 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,15 @@ export async function* runToolLoop<TMessage>(
132132
let cacheCreationTokens = 0
133133
let costUsd: number | undefined
134134

135+
const aggregateUsageEvent = (): Extract<AiStreamEvent, { type: 'usage' }> => ({
136+
type: 'usage',
137+
promptTokens,
138+
completionTokens,
139+
costUsd,
140+
cacheReadTokens: cacheReadTokens || undefined,
141+
cacheCreationTokens: cacheCreationTokens || undefined,
142+
})
143+
135144
for (;;) {
136145
if (req.signal.aborted) return
137146

@@ -219,9 +228,35 @@ export async function* runToolLoop<TMessage>(
219228
for (const call of turn.toolCalls) {
220229
const tool = toolsByName.get(call.name)
221230
const input = prepareToolInput(call, req)
222-
const output: AiToolOutput = tool
223-
? await executeAiTool(tool, input, req.bridge, req.signal, req.toolContextBase)
224-
: { ok: false, error: `Unknown tool: ${call.name}` }
231+
let output: AiToolOutput
232+
try {
233+
output = tool
234+
? await executeAiTool(tool, input, req.bridge, req.signal, req.toolContextBase)
235+
: { ok: false, error: `Unknown tool: ${call.name}` }
236+
} catch (err) {
237+
// Browser tools communicate domain failures by resolving an
238+
// `AiToolOutput`. Rejection means the bridge transport disappeared
239+
// (timeout, server reload, closed stream). Retrying within this turn
240+
// would hit the same dead bridge and can burn repeated provider rounds.
241+
if (req.signal.aborted) return
242+
const detail = err instanceof Error ? err.message : String(err)
243+
yield {
244+
type: 'toolResult',
245+
toolCallId: call.id,
246+
toolName: call.name,
247+
ok: false,
248+
error: detail,
249+
}
250+
// The provider already completed and billed this round before the
251+
// bridge failed. Persist its accumulated usage before the terminal
252+
// error so conversation totals and the failed-turn audit stay honest.
253+
yield aggregateUsageEvent()
254+
yield {
255+
type: 'error',
256+
message: `Browser tool transport failed: ${detail}`,
257+
}
258+
return
259+
}
225260
yield {
226261
type: 'toolResult',
227262
toolCallId: call.id,
@@ -240,14 +275,7 @@ export async function* runToolLoop<TMessage>(
240275
}
241276
}
242277

243-
yield {
244-
type: 'usage',
245-
promptTokens,
246-
completionTokens,
247-
costUsd,
248-
cacheReadTokens: cacheReadTokens || undefined,
249-
cacheCreationTokens: cacheCreationTokens || undefined,
250-
}
278+
yield aggregateUsageEvent()
251279
}
252280

253281
/**

‎server/ai/handlers/chat.ts‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -320,6 +320,13 @@ async function handleAiChat(
320320
})()
321321
const { messages, systemPrompt, tokensAtStart } = prepared
322322

323+
// `req.signal` covers request-side aborts, but a streaming response consumer
324+
// can disappear independently (tab reload, dev-server hot restart, proxy
325+
// disconnect). Own a second lifecycle signal and abort it from the response
326+
// stream's `cancel()` hook or when enqueue proves the consumer is gone.
327+
const streamAbort = new AbortController()
328+
const turnSignal = AbortSignal.any([req.signal, streamAbort.signal])
329+
323330
const stream = new ReadableStream<Uint8Array>({
324331
async start(controller) {
325332
let streamClosed = false
@@ -348,6 +355,7 @@ async function handleAiChat(
348355
controller.enqueue(encodeStreamEvent(wireEvent))
349356
} catch {
350357
streamClosed = true
358+
streamAbort.abort()
351359
}
352360
}
353361

@@ -366,7 +374,7 @@ async function handleAiChat(
366374
}
367375
const { bridgeId, bridge, destroy } = createBridge(
368376
emit,
369-
req.signal,
377+
turnSignal,
370378
undefined,
371379
(next) => { toolContextBase.snapshot = next },
372380
)
@@ -382,7 +390,7 @@ async function handleAiChat(
382390
modelId: conversation.modelId,
383391
modelCapabilities,
384392
credentials: resolvedCredential,
385-
signal: req.signal,
393+
signal: turnSignal,
386394
bridge,
387395
toolContextBase,
388396
}
@@ -435,6 +443,12 @@ async function handleAiChat(
435443
}
436444
}
437445
},
446+
cancel() {
447+
// Abort provider fetches and pending browser waiters immediately; the
448+
// handler's finally block then destroys the bridge and releases the
449+
// per-conversation writer lock.
450+
streamAbort.abort()
451+
},
438452
})
439453

440454
return new Response(stream, {

‎server/ai/handlers/toolResult.ts‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -52,9 +52,14 @@ async function handleAiToolResult(req: Request, db: DbClient): Promise<Response>
5252

5353
const matched = resolveBridgeToolResult(bridgeId, requestId, result, snapshot)
5454
if (!matched) {
55-
// Bridge gone or unknown requestId — likely the stream was aborted
56-
// before the browser's POST arrived. Not a fatal client error.
57-
return jsonResponse({ ok: false }, { status: 404 })
55+
// The browser completed work for a turn this runtime no longer owns — most
56+
// commonly after a server restart. The active chat client must abort its
57+
// old response instead of silently waiting 90 seconds and letting the
58+
// model retry the same tool repeatedly.
59+
return jsonResponse(
60+
{ error: 'The AI tool bridge is no longer active. The server may have restarted; send the message again.' },
61+
{ status: 404 },
62+
)
5863
}
5964
return jsonResponse({ ok: true })
6065
}

‎server/ai/mcp/server.test.ts‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,32 @@ describe('mcp server', () => {
150150
])
151151
})
152152

153+
it('returns an MCP tool error when the live editor bridge disconnects mid-call', async () => {
154+
const userId = 'u1-disconnected-workspace'
155+
const controller = new AbortController()
156+
const reader = createEditorBridgeStream(userId, 'site', controller.signal).getReader()
157+
await readUntil(reader, (event) => event.type === 'bridgeReady')
158+
const client = await connectClient(
159+
db,
160+
['ai.chat', 'ai.tools.write', 'site.structure.edit'],
161+
userId,
162+
)
163+
164+
const call = client.callTool({
165+
name: 'site_insert_html',
166+
arguments: { parentId: 'root', html: '<p>site</p>' },
167+
})
168+
await readUntil(reader, (event) => event.type === 'toolRequest')
169+
controller.abort()
170+
171+
const result = await call
172+
expect(result.isError).toBe(true)
173+
expect(JSON.stringify(result.content)).toContain('before tool result arrived')
174+
175+
await client.close()
176+
await reader.read().catch(() => {})
177+
})
178+
153179
it('enforces an own-only connector grant before relaying through a full-power owner browser', async () => {
154180
await db`
155181
insert into users (id, email, email_normalized, display_name, password_hash, role_id)

0 commit comments

Comments
 (0)