From bfbebc7b80abbc482a6a3ee445ab5cadd3820d47 Mon Sep 17 00:00:00 2001 From: Kaede Akino Date: Wed, 26 Aug 2026 09:36:53 +0800 Subject: [PATCH] Answer tool calls that a killed process left hanging MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Force quitting during a tool batch produced a conversation that could not be continued at all, only deleted: `failed to stream request: error: No tool output found for function call call_…`. The assistant turn is persisted as soon as it arrives, its results only when the run next saves a snapshot, so a process that dies in between restores a turn whose calls nothing answers — and every provider rejects that outright rather than degrading. It is the exact mirror of the orphaned result this codebase already repaired, and the same class of fatal. Recorded results come back first. `toolFinished` is durable and lands as each tool finishes, so in the common case the output is sitting in the database while the transcript claims to know nothing about it; `loadConversations` now materializes any result whose transcript row was never written. In a batch of three killed during the third, the first two come back with the output they actually produced. `AgentUnansweredToolCallRepair` answers whatever storage cannot, at the model boundary and at the snapshot seed — the same two places the orphan repair runs, and for the same reason: repairing at the seed writes the fix forward instead of re-deriving it on every future turn. What it says matters as much as that it says something. It does not claim the tool did not run. The process died after the command was dispatched, so whether the remote side executed it is genuinely unknown, and a model told "this never ran" will cheerfully run it again — which is a data-loss bug the first time the call was `rm`. It says the outcome is unknown and to check before repeating. A card caught mid-flight also came back spinning. Every unfinished run is marked interrupted at launch, so `running` is not a state a restored card can legitimately be in, and nothing was ever going to stop that spinner. Worth noting against the previous commit: batching the snapshot save to once per turn rather than once per tool widened this window, and the recovery above is what closes it — results are still recorded one at a time, so nothing a tool produced is lost, only deferred. Co-Authored-By: Claude Opus 5 (1M context) --- ARCHITECTURE.md | 10 ++ .../Agent/AgentContextPipeline.swift | 75 ++++++++- Termind/Application/Agent/AgentRuntime.swift | 11 +- .../FeatureModels/AssistantLibraryModel.swift | 7 +- .../Core/Persistence/AgentPersistence.swift | 61 ++++++- .../AgentInterruptedRunRecoveryTests.swift | 158 ++++++++++++++++++ 6 files changed, 312 insertions(+), 10 deletions(-) create mode 100644 TermindTests/AgentInterruptedRunRecoveryTests.swift diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index c66b582d..69b00522 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -116,3 +116,13 @@ Agent conversations, runs, visible messages, tool calls/results, and reviews are stored in SwiftData after truncation and redaction. Launch marks unfinished runs `interrupted`; history is restored but remote actions are never resumed or replayed. + +A process that dies mid-batch — force quit, crash, the OS reclaiming the app — +leaves tool calls that nothing answered, which the next request would be +rejected for. Restore closes that in two steps. Results are recorded as each +tool finishes but their transcript rows wait for the run's next snapshot save, +so `loadConversations` first materializes any recorded result whose transcript +row was never written. Whatever is still unanswered is answered at the model +boundary by `AgentUnansweredToolCallRepair`, which says the outcome is unknown +rather than that the tool did not run: the process died after the command was +dispatched, and a model told otherwise would repeat it. diff --git a/Termind/Application/Agent/AgentContextPipeline.swift b/Termind/Application/Agent/AgentContextPipeline.swift index 51696301..85692980 100644 --- a/Termind/Application/Agent/AgentContextPipeline.swift +++ b/Termind/Application/Agent/AgentContextPipeline.swift @@ -37,16 +37,18 @@ nonisolated struct AgentContextPipeline: Sendable { /// What a chat run uses. /// - /// Order matters. Ordering runs before the orphan repair so a reordered - /// block is what gets checked; trimming runs after both so it measures the - /// results that will actually be sent; injection runs last because it is - /// the only step that adds a message, and every earlier step reasons about + /// Order matters. The unanswered repair runs first so the results it + /// invents are sorted into place by the step after it; ordering runs before + /// the orphan repair so a reordered block is what gets checked; trimming + /// runs after all three so it measures the results that will actually be + /// sent; injection runs last because everything before it reasons about /// tool results sitting directly behind their assistant turn. static func chat( sessionContext: AgentTranscriptMessage?, before promptID: AgentTranscriptMessage.ID? ) -> AgentContextPipeline { AgentContextPipeline([ + AgentUnansweredToolCallRepair(), AgentToolResultOrdering(), AgentOrphanedToolResultRepair(), AgentToolResultTrimming(), @@ -149,6 +151,71 @@ nonisolated struct AgentOrphanedToolResultRepair: AgentContextTransforming { } } +// MARK: - Unanswered calls + +/// Answers tool calls that nothing ever answered. +/// +/// The mirror of `AgentOrphanedToolResultRepair`, and the same class of fatal: +/// where a result with no call is rejected, so is a call with no result. OpenAI +/// says `No tool output found for function call call_…`, and the other two +/// reject the equivalent — the conversation cannot be continued at all after +/// that, only deleted. +/// +/// The gap this closes is a process that dies mid-batch. A live run always +/// answers every call, cancellation included — see `AgentToolScheduler` — but a +/// force quit, a crash, or the OS reclaiming the app takes the run's remaining +/// writes with it, and the history restored at launch has an assistant turn +/// with nothing behind it. +/// +/// The wording matters as much as the presence. It must not say the tool did +/// not run: the process died *after* the command was dispatched, so whether the +/// remote side executed it is genuinely unknown, and a model told "this never +/// ran" will happily run it again. Telling it to check first is the only honest +/// instruction, and the only safe one when the call was `rm`. +nonisolated struct AgentUnansweredToolCallRepair: AgentContextTransforming { + func transform(_ context: AgentModelContext) -> AgentModelContext { + var result = context + result.messages = Self.apply(result.messages) + return result + } + + static func apply(_ messages: [AgentTranscriptMessage]) -> [AgentTranscriptMessage] { + guard messages.contains(where: { $0.role == .assistant && !$0.toolCalls.isEmpty }) + else { return messages } + + var repaired: [AgentTranscriptMessage] = [] + repaired.reserveCapacity(messages.count) + var index = messages.startIndex + while index < messages.endIndex { + let message = messages[index] + repaired.append(message) + index += 1 + guard message.role == .assistant, !message.toolCalls.isEmpty else { continue } + + var answered: Set = [] + while index < messages.endIndex, messages[index].role == .tool { + if let id = messages[index].toolCallID { answered.insert(id) } + repaired.append(messages[index]) + index += 1 + } + for call in message.toolCalls where !answered.contains(call.id) { + repaired.append(AgentTranscriptMessage( + role: .tool, text: interrupted, toolCallID: call.id, + toolName: call.name, isError: true + )) + } + } + return repaired + } + + private static let interrupted = String( + localized: """ + Termind stopped before this tool call's result was recorded. Whether the action \ + completed is unknown; check the current state before doing it again. + """ + ) +} + // MARK: - Trimming /// Bounds what replaying old tool results costs. diff --git a/Termind/Application/Agent/AgentRuntime.swift b/Termind/Application/Agent/AgentRuntime.swift index 6e3a22f1..ad7de27f 100644 --- a/Termind/Application/Agent/AgentRuntime.swift +++ b/Termind/Application/Agent/AgentRuntime.swift @@ -120,10 +120,13 @@ actor AgentRuntime { state: .running, permissionMode: request.permissionMode, // Applied to incoming history as well as at the model boundary, - // which is what makes it a repair and not just a guard: this - // snapshot is persisted, so filtering here stops a stored orphan - // from being written forward into the next run. - messages: AgentOrphanedToolResultRepair.apply(request.priorMessages) + // which is what makes these repairs and not just guards: this + // snapshot is persisted, so a call left unanswered by a process + // that died — or a result left behind by one — is fixed once here + // rather than re-derived on every future turn of the conversation. + messages: AgentOrphanedToolResultRepair.apply( + AgentUnansweredToolCallRepair.apply(request.priorMessages) + ) ) activeRunID = snapshot.id let runID = snapshot.id diff --git a/Termind/Application/FeatureModels/AssistantLibraryModel.swift b/Termind/Application/FeatureModels/AssistantLibraryModel.swift index a07d8390..067e5005 100644 --- a/Termind/Application/FeatureModels/AssistantLibraryModel.swift +++ b/Termind/Application/FeatureModels/AssistantLibraryModel.swift @@ -66,7 +66,12 @@ final class AssistantConversationStore { let state: AgentToolCard.State if card.result?.isError == true { state = .failed } else if card.result != nil || card.state == "finished" { state = .completed } - else if card.state == "started" { state = .running } + // Not `.running`, which is what this used to restore to. Every + // unfinished run is marked interrupted at launch, so a card + // that was mid-flight when the process died is not running + // now and never will be — it came back as a spinner that + // nothing would ever stop. + else if card.state == "started" { state = .failed } else { state = .proposed } return AgentToolCard( invocation: card.invocation, descriptor: card.descriptor, diff --git a/Termind/Core/Persistence/AgentPersistence.swift b/Termind/Core/Persistence/AgentPersistence.swift index 62fe6814..d014bf85 100644 --- a/Termind/Core/Persistence/AgentPersistence.swift +++ b/Termind/Core/Persistence/AgentPersistence.swift @@ -283,7 +283,13 @@ final class SwiftDataAgentRunRepository: AgentRunPersisting, Sendable { .sorted { $0.createdAt < $1.createdAt } .compactMap { storedToolCard($0, results: results, reviews: reviews) } return StoredAgentConversation( - id: conversation.id, messages: transcript, toolCards: cards, + id: conversation.id, + messages: Self.recovering( + transcript, + results: results.filter { runIDs.contains($0.runID) }, + invocations: invocations.filter { runIDs.contains($0.runID) } + ), + toolCards: cards, lastRunState: state, title: conversation.title ) } @@ -338,6 +344,59 @@ final class SwiftDataAgentRunRepository: AgentRunPersisting, Sendable { try context.save() } + /// Puts back tool results that were recorded but never made it into the + /// transcript. + /// + /// The two are written on different schedules: a result row lands as its + /// tool finishes, while its transcript row waits for the run's next + /// snapshot save. A process killed in between — force quit, crash, the OS + /// reclaiming the app — leaves the call answered in storage and unanswered + /// in history, and an assistant turn with an unanswered call is rejected + /// outright on the next request. + /// + /// `AgentUnansweredToolCallRepair` would invent a placeholder for the same + /// gap, and does for anything still missing after this. Doing it here first + /// is what gives the model the output the tool actually produced instead of + /// being told the outcome is unknown when it is sitting in the database. + static func recovering( + _ transcript: [AgentTranscriptMessage], + results: [PersistentAgentToolResult], + invocations: [PersistentAgentToolInvocation] + ) -> [AgentTranscriptMessage] { + guard transcript.contains(where: { $0.role == .assistant && !$0.toolCalls.isEmpty }) + else { return transcript } + + let names = Dictionary( + invocations.map { ($0.callID, $0.toolName) }, uniquingKeysWith: { first, _ in first } + ) + var recovered: [AgentTranscriptMessage] = [] + recovered.reserveCapacity(transcript.count) + var index = transcript.startIndex + while index < transcript.endIndex { + let message = transcript[index] + recovered.append(message) + index += 1 + guard message.role == .assistant, !message.toolCalls.isEmpty else { continue } + + var answered: Set = [] + while index < transcript.endIndex, transcript[index].role == .tool { + if let id = transcript[index].toolCallID { answered.insert(id) } + recovered.append(transcript[index]) + index += 1 + } + for call in message.toolCalls where !answered.contains(call.id) { + // The last recorded one: a call id is unique to its invocation, + // so more than one row can only mean a rewritten result. + guard let stored = results.last(where: { $0.callID == call.id }) else { continue } + recovered.append(AgentTranscriptMessage( + role: .tool, text: stored.content, toolCallID: call.id, + toolName: names[call.id] ?? call.name, isError: stored.isError + )) + } + } + return recovered + } + private func upsertInvocation( _ invocation: AgentToolInvocation, state: String ) throws { diff --git a/TermindTests/AgentInterruptedRunRecoveryTests.swift b/TermindTests/AgentInterruptedRunRecoveryTests.swift new file mode 100644 index 00000000..02a0a344 --- /dev/null +++ b/TermindTests/AgentInterruptedRunRecoveryTests.swift @@ -0,0 +1,158 @@ +import Foundation +import SwiftData +import Testing + +@testable import Termind + +/// What a force quit during a tool batch leaves behind, and what it takes to +/// make that conversation usable again. +/// +/// The failure it guards is total, not partial: an assistant turn carrying a +/// tool call that nothing answers is rejected outright by every provider — +/// OpenAI says `No tool output found for function call call_…` — so the +/// conversation cannot be continued at all, only deleted. +@Suite +struct AgentInterruptedRunRecoveryTests { + private func assistant(calling ids: [String]) -> AgentTranscriptMessage { + AgentTranscriptMessage( + role: .assistant, text: "Checking both.", + toolCalls: ids.map { AgentToolCall(id: $0, name: "ssh_exec", arguments: .object([:])) } + ) + } + + private func answer(_ id: String, _ text: String) -> AgentTranscriptMessage { + AgentTranscriptMessage(role: .tool, text: text, toolCallID: id, toolName: "ssh_exec") + } + + @Test + func everyToolCallEndsUpAnsweredAtTheModelBoundary() { + let messages = [ + AgentTranscriptMessage(role: .user, text: "check both hosts"), + assistant(calling: ["call-a", "call-b"]), + answer("call-a", "up 3 days"), + ] + + let sent = AgentContextPipeline.chat(sessionContext: nil, before: nil)( + AgentModelContext(systemPrompt: "s", messages: messages, tools: []) + ).messages + + let answered = Set(sent.compactMap(\.toolCallID)) + #expect(answered == ["call-a", "call-b"]) + // Placed against the call it answers, not appended to the end. + #expect(sent.map(\.role) == [.user, .assistant, .tool, .tool]) + // The recovered output is untouched; only the missing one is invented. + #expect(sent[2].text == "up 3 days") + #expect(sent[3].isError) + } + + /// It must not claim the action did not happen. The process died after the + /// command was sent, so whether the remote side ran it is genuinely + /// unknown, and a model told "this never ran" will cheerfully run it again. + @Test + func theSynthesizedAnswerDoesNotClaimTheToolNeverRan() { + let repaired = AgentUnansweredToolCallRepair.apply([assistant(calling: ["call-a"])]) + + let text = try! #require(repaired.last?.text) + #expect(text.contains("unknown")) + #expect(!text.lowercased().contains("never ran")) + } + + @Test + func repairingIsIdempotentAndLeavesCompleteTurnsAlone() { + let messages = [assistant(calling: ["call-a"]), answer("call-a", "ok")] + + let once = AgentUnansweredToolCallRepair.apply(messages) + #expect(once == messages) + #expect(AgentUnansweredToolCallRepair.apply(once) == once) + } + + /// Results are recorded as each tool finishes, but their transcript rows are + /// only written when the run next saves its snapshot. A process killed in + /// between leaves the call answered in storage and unanswered in history — + /// so restore should hand back the real output, not a placeholder for it. + @MainActor + @Test + func restoreRecoversResultsRecordedBeforeTheProcessDied() async throws { + let configuration = ModelConfiguration( + "AgentInterruptedRunRecoveryTests", schema: HostPersistence.schema, + isStoredInMemoryOnly: true + ) + let container = try ModelContainer( + for: HostPersistence.schema, configurations: [configuration] + ) + let repository = SwiftDataAgentRunRepository(context: container.mainContext) + + // The snapshot as it stood when the assistant turn was persisted: two + // calls, no results yet. + let snapshot = AgentRunSnapshot( + conversationID: UUID(), state: .running, permissionMode: .askForApproval, + messages: [ + AgentTranscriptMessage(role: .user, text: "check both hosts"), + assistant(calling: ["call-a", "call-b"]), + ] + ) + try await repository.save(snapshot) + + // One tool finished and was recorded; the other was still running when + // the app was killed, so no snapshot save ever followed. + let finished = AgentToolInvocation( + runID: snapshot.id, + call: AgentToolCall(id: "call-a", name: "ssh_exec", arguments: .object([:])) + ) + try await repository.record(.toolStarted(finished), runID: snapshot.id) + try await repository.record( + .toolFinished(finished, AgentToolResult(callID: "call-a", content: "up 3 days")), + runID: snapshot.id + ) + let running = AgentToolInvocation( + runID: snapshot.id, + call: AgentToolCall(id: "call-b", name: "ssh_exec", arguments: .object([:])) + ) + try await repository.record(.toolStarted(running), runID: snapshot.id) + + // Relaunch. + let reopened = SwiftDataAgentRunRepository(context: container.mainContext) + let restored = try #require(try await reopened.loadConversations().first) + + let recovered = try #require(restored.messages.first { $0.toolCallID == "call-a" }) + #expect(recovered.text == "up 3 days", "the recorded output should come back") + #expect(!recovered.isError) + #expect(restored.lastRunState == .interrupted) + + // The one that never finished is still unanswered here — storage has + // nothing to recover — and is repaired at the model boundary. + let sent = AgentContextPipeline.chat(sessionContext: nil, before: nil)( + AgentModelContext(systemPrompt: "s", messages: restored.messages, tools: []) + ).messages + #expect(Set(sent.compactMap(\.toolCallID)) == ["call-a", "call-b"]) + } + + /// A card left mid-flight by a force quit must not come back spinning. + /// Every run is marked interrupted at launch, so `running` is never a state + /// a restored card can legitimately be in. + @MainActor + @Test + func restoredCardsDoNotComeBackRunning() { + let store = AssistantConversationStore() + let invocation = AgentToolInvocation( + runID: UUID(), + call: AgentToolCall(id: "call-b", name: "ssh_exec", arguments: .object([:])) + ) + store.restore([StoredAgentConversation( + id: UUID(), + messages: [], + toolCards: [StoredAgentToolCard( + invocation: invocation, + descriptor: AgentToolDescriptor( + name: "ssh_exec", summary: "exec", inputSchema: .object([:]), + target: .host, safety: .requiresAuthorization + ), + state: "started", result: nil + )], + lastRunState: .interrupted + )]) + + let card = try! #require(store.conversations.first?.toolCards.first) + #expect(card.state != .running) + } +}