From 9a631c8f8b5fdb7f6cb7853f51ea11c0a1084164 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 13 Aug 2026 02:39:08 +0200 Subject: [PATCH] coach: close an aborted turn with a persisted cancelled marker Navigating away mid-reply aborts the SSE fetch; the provider call tears down, but the conversation was left with the user message dangling and no trace of the interruption. Persist an empty assistant row tagged providerType "cancelled" (the refusal/nudge sentinel channel) when the request signal is aborted, refund stays intact, and the abort is annotated as insights.coach.cancelled instead of a provider failure or a GlitchTip defect. Cancelled markers are filtered out of the provider transcript on later turns. Refs #781 --- docs/api/openapi.yaml | 4 +- .../__tests__/route-cancelled-turn.test.ts | 246 ++++++++++++++++++ src/app/api/insights/chat/route.ts | 54 +++- src/lib/openapi/routes/coach.ts | 2 +- 4 files changed, 300 insertions(+), 6 deletions(-) create mode 100644 src/app/api/insights/chat/__tests__/route-cancelled-turn.test.ts diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index 644022880..a4f296586 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -28647,7 +28647,9 @@ components: anyOf: - type: string - type: "null" - description: Provider that produced the reply (e.g. anthropic, openai, local, refusal); null for user turns. + description: Provider that produced the reply (e.g. anthropic, openai, local, refusal). The sentinel `cancelled` marks + an empty assistant row closing a turn the client aborted mid-generation (navigation away); clients should + render it as an interrupted turn with a retry affordance. Null for user turns. promptVersion: anyOf: - type: string diff --git a/src/app/api/insights/chat/__tests__/route-cancelled-turn.test.ts b/src/app/api/insights/chat/__tests__/route-cancelled-turn.test.ts new file mode 100644 index 000000000..593a9fb7a --- /dev/null +++ b/src/app/api/insights/chat/__tests__/route-cancelled-turn.test.ts @@ -0,0 +1,246 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +/** + * #781 — a client that navigates away mid-generation aborts the SSE fetch. + * The route must close the turn honestly: persist an EMPTY assistant marker + * with `providerType: "cancelled"` so the conversation shows the interrupted + * turn (and the client can offer a retry), instead of leaving the user + * message dangling unanswered. The reservation is refunded in full, and the + * abort is never misreported as a provider failure. + * + * Route-level on purpose: the abort branch lives in `route.ts`'s catch, a + * place no pure-module test can see (the #591 lesson). + */ + +const SNAPSHOT_JSON = '{"bp":{"aggregate":{"mean":128}}}'; + +vi.mock("@/lib/api-handler", () => ({ + apiHandler: unknown>(fn: T) => fn, + requireAuth: vi.fn(async () => ({ user: { id: "u1", locale: "en" } })), + HttpError: class HttpError extends Error { + status: number; + constructor(status: number, message: string) { + super(message); + this.status = status; + } + }, +})); +vi.mock("@/lib/modules/gate", () => ({ + requireModuleEnabled: vi.fn(async () => ({ enabled: true })), + isModuleEnabled: vi.fn(async () => true), +})); +vi.mock("@/lib/feature-flags", () => ({ + requireAssistantSurface: vi.fn(async () => undefined), +})); +vi.mock("@/lib/api-response", () => ({ + apiError: (error: string, status: number) => ({ data: null, error, status }), + apiSuccess: (data: unknown) => ({ data, error: null, status: 200 }), +})); +const { annotate } = vi.hoisted(() => ({ annotate: vi.fn() })); +vi.mock("@/lib/logging/context", () => ({ + annotate, + getEvent: vi.fn(() => null), +})); +vi.mock("@/lib/auth/audit", () => ({ auditLog: vi.fn() })); +vi.mock("@/lib/db", () => ({ + prisma: { + user: { findUnique: vi.fn(async () => ({ coachPrefsJson: null })) }, + coachConversation: { findFirst: vi.fn(async () => ({ id: "c1" })) }, + workout: { findFirst: vi.fn(async () => null) }, + }, +})); +vi.mock("@/lib/rate-limit", () => ({ + checkRateLimit: vi.fn(async () => ({ allowed: true })), +})); +vi.mock("@/lib/i18n/server-locale", () => ({ + resolveServerLocale: vi.fn(async () => "en"), +})); + +const { runStreamingRawCompletionWithFallback } = vi.hoisted(() => ({ + runStreamingRawCompletionWithFallback: vi.fn(), +})); +vi.mock("@/lib/ai/provider-runner", () => ({ + AllProvidersFailedError: class extends Error {}, + runStreamingRawCompletionWithFallback, +})); + +const { resolveProviderChain } = vi.hoisted(() => ({ + resolveProviderChain: vi.fn(), +})); +vi.mock("@/lib/ai/provider", () => ({ + resolveProviderChain, + resolveProvider: vi.fn(), +})); +vi.mock("@/lib/ai/consent-guard", () => ({ + assertConsentForChain: vi.fn(async () => undefined), +})); +vi.mock("@/lib/ai/prompts/insight-generator", () => ({ PROMPT_VERSION: "x" })); +vi.mock("@/lib/ai/ai-budgets", () => ({ + AI_BUDGETS: { coach: { maxTokens: 1500, temperature: 0.4 } }, +})); + +vi.mock("@/lib/ai/coach/persistence", () => ({ + appendMessage: vi.fn(async () => ({ id: "m1" })), + createConversation: vi.fn(async () => ({ id: "c1" })), + fetchConversationWithMessages: vi.fn(), + listConversations: vi.fn(), +})); +vi.mock("@/lib/ai/coach/coach-memory-shared", () => ({ + enqueueCoachMemoryRefresh: vi.fn(), +})); +vi.mock("@/lib/ai/coach/facts", () => ({ + storeDeterministicFacts: vi.fn(async () => undefined), +})); + +const { reserveBudget, reconcileSpend } = vi.hoisted(() => ({ + reserveBudget: vi.fn(async () => ({ allowed: true, reserved: 3000 })), + reconcileSpend: vi.fn(async () => undefined), +})); +vi.mock("@/lib/ai/coach/budget", () => ({ + buildDateKey: vi.fn(() => "2026-08-13"), + reserveBudget, + reconcileSpend, + resolveDailyCap: vi.fn(() => 2_000_000), +})); + +vi.mock("@/lib/ai/coach/refusal", () => ({ + detectRefusal: vi.fn(() => ({ refuse: false })), +})); +vi.mock("@/lib/ai/coach/system-prompt", () => ({ + getCoachSystemPrompt: vi.fn(() => "SYSTEM"), +})); +vi.mock("@/lib/ai/coach/about-me", () => ({ + getSelfContextTextForUser: vi.fn(async () => null), +})); +vi.mock("@/lib/ai/coach/snapshot", () => ({ + buildCoachSnapshot: vi.fn(async () => ({ + snapshotJson: SNAPSHOT_JSON, + sections: { bloodPressure: { aggregate: { mean: 128 } } }, + provenance: { windows: ["last30days"], metrics: ["bp"] }, + referenceGrounding: "REFERENCE RANGES", + })), +})); +vi.mock("@/lib/workouts/hr-series", () => ({ + buildWorkoutHrSeries: vi.fn(async () => null), +})); +vi.mock("@/lib/workouts/zones", () => ({ + computeZones: vi.fn(() => null), + hrMaxFromAge: vi.fn(() => 185), + parseWhoopZoneDurations: vi.fn(() => null), +})); +vi.mock("@/lib/workouts/sport-context", () => ({ + buildSportContext: vi.fn(async () => null), +})); + +vi.mock("@/lib/ai/coach/tools", () => ({ + COACH_TOOL_DEFS: [{ name: "get_metric_series" }], + MAX_ROUNDS: 3, + buildCoachDataInventory: vi.fn(async () => ({ + entries: [], + restMode: false, + cycleEnabled: false, + window: "last30days", + probeScope: { sources: ["bp"], window: "last30days" }, + })), + renderDataInventory: vi.fn(() => "DATA INVENTORY"), + renderFocusHint: vi.fn(() => ""), + buildToolModeAddendum: vi.fn(() => "TOOL ADDENDUM"), + runCoachToolLoop: vi.fn(), +})); + +vi.mock("@/lib/ai/coach/keyvalues", () => ({ + parseKeyValuesSentinel: vi.fn(), +})); +vi.mock("@/lib/ai/coach/suggest-reminder", () => ({ + parseSuggestReminder: vi.fn(), +})); +vi.mock("@/lib/ai/coach/suggest-gate", () => ({ gateSuggestion: vi.fn() })); +vi.mock("@/lib/validations/coach-prefs", () => ({ + parseCoachPrefs: vi.fn(() => ({ defaultWindow: undefined })), + DEFAULT_REMINDER_SUGGESTION_PREFS: {}, +})); + +const { appendMessage } = await import("@/lib/ai/coach/persistence"); + +const sse = vi.hoisted(() => ({ done: Promise.resolve() as Promise })); +vi.mock("@/lib/sse/create-stream", () => ({ + createSseStream: ( + producer: (c: { + signal: { aborted: boolean }; + enqueue: () => void; + }) => void | Promise, + ) => { + sse.done = Promise.resolve( + producer({ signal: { aborted: true }, enqueue: () => {} }), + ); + return new ReadableStream(); + }, +})); + +import { POST } from "../route"; + +const post = POST as unknown as (req: Request) => Promise; + +function abortedChatReq(body: Record): Request { + const controller = new AbortController(); + const req = new Request("http://localhost/api/insights/chat", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify(body), + signal: controller.signal, + }); + // The body is read synchronously by the handler before the provider call; + // aborting here models the client tearing the connection down while the + // provider is generating (the runner rejects below). + controller.abort(); + return req; +} + +function abortError(): Error { + const err = new Error("This operation was aborted"); + err.name = "AbortError"; + return err; +} + +describe("coach chat — client abort mid-generation (#781)", () => { + beforeEach(() => { + vi.clearAllMocks(); + reserveBudget.mockResolvedValue({ allowed: true, reserved: 3000 }); + // No-tools chain so the turn runs the streaming runner (which rejects + // with the abort the torn-down connection produces). + resolveProviderChain.mockResolvedValue([ + { providerType: "local", instance: { supportsTools: false } }, + ]); + runStreamingRawCompletionWithFallback.mockRejectedValue(abortError()); + }); + + it("persists an empty assistant marker with providerType 'cancelled'", async () => { + const res = await post(abortedChatReq({ message: "How is my BP?" })); + expect(res.status).toBe(200); + await sse.done; + + const calls = (appendMessage as ReturnType).mock.calls; + // The user turn is persisted first, then the cancelled marker. + const roles = calls.map((c) => (c[0] as { role: string }).role); + expect(roles).toEqual(["user", "assistant"]); + const marker = calls[1][0] as { + role: string; + content: string; + providerType: string | null; + }; + expect(marker.providerType).toBe("cancelled"); + expect(marker.content).toBe(""); + }); + + it("refunds the full reservation and annotates the abort as cancelled, not a provider failure", async () => { + await post(abortedChatReq({ message: "How is my BP?" })); + await sse.done; + + expect(reconcileSpend).toHaveBeenCalledWith("u1", 3000, 0, "2026-08-13"); + const actions = annotate.mock.calls + .map((c) => (c[0] as { action?: { name?: string } }).action?.name) + .filter(Boolean); + expect(actions).toContain("insights.coach.cancelled"); + expect(actions).not.toContain("insights.coach.providerFailed"); + }); +}); diff --git a/src/app/api/insights/chat/route.ts b/src/app/api/insights/chat/route.ts index 2f03c16ae..d10447086 100644 --- a/src/app/api/insights/chat/route.ts +++ b/src/app/api/insights/chat/route.ts @@ -325,10 +325,16 @@ async function handleChatRequest(request: NextRequest): Promise { } workingConversationId = existing.id; priorSummary = existing.summary ?? null; - priorTurns = existing.messages.map((m) => ({ - role: m.role, - content: m.content, - })); + // #781 — cancelled-turn markers (`providerType: "cancelled"`, empty body) + // are UI rows, not conversation content: keep them out of the provider + // transcript so an interrupted turn adds no empty assistant line to the + // prompt. They stay in the DTO the client renders. + priorTurns = existing.messages + .filter((m) => m.providerType !== "cancelled") + .map((m) => ({ + role: m.role, + content: m.content, + })); // Ledger cross-turn sources (D3-safe): user-authored numbers + the // persisted tool figures of prior turns. Assistant PROSE is never read. priorUserMessages = existing.messages @@ -761,6 +767,35 @@ async function handleChatRequest(request: NextRequest): Promise { await reconcileSpend(userId, reservation.reserved, 0, reqDateKey).catch( () => {}, ); + // #781 — the client walked away mid-generation. The request's abort + // signal is threaded into every provider call, so the teardown surfaces + // here as an abort-shaped failure with `request.signal` already flipped. + // Close the turn honestly instead of letting the user message dangle + // unanswered: persist an EMPTY assistant marker tagged + // `providerType: "cancelled"` (the same channel "refusal" and "nudge" + // already use for non-provider rows) so the thread shows the turn as + // interrupted and offers a retry on reload. The content is empty by + // construction — nothing guarded was produced, and anything the client + // ever SAW was persisted before the first token frame left, so no + // unguarded partial text is ever written. The reservation was refunded + // above; a retry pays exactly what a fresh turn pays. + if (request.signal.aborted) { + await appendMessage({ + conversationId: workingConversationId, + role: "assistant", + content: "", + providerType: "cancelled", + promptVersion: PROMPT_VERSION, + }).catch(() => { + // Marker persistence is best-effort — a failure leaves the + // dangling user turn, which is exactly the pre-#781 state. + }); + annotate({ + action: { name: "insights.coach.cancelled" }, + meta: { conversationId: workingConversationId }, + }); + return { ok: false, code: "coach.cancelled" }; + } if (err instanceof AllProvidersFailedError) { annotate({ action: { name: "insights.coach.providerFailed" }, @@ -1241,6 +1276,17 @@ async function handleChatRequest(request: NextRequest): Promise { outcome = await produceReply(); } catch (err) { clearInterval(heartbeat); + // #781 — a client disconnect can also surface as an abort-shaped error + // on a path `produceReply`'s own catch does not own (e.g. a teardown + // rejection out of persistence). That is not a defect and nobody is + // listening: skip the GlitchTip forward and the error frame. + if (request.signal.aborted) { + annotate({ + action: { name: "insights.coach.cancelled" }, + meta: { conversationId: workingConversationId, lateAbort: true }, + }); + return; + } // The stream is already open, so we cannot 500: a tagged provider error // surfaces a graceful `coach.provider.*` frame; anything else degrades to // a generic unavailable frame. A genuine defect is still annotated. diff --git a/src/lib/openapi/routes/coach.ts b/src/lib/openapi/routes/coach.ts index 814cdfbf0..9e0336e3e 100644 --- a/src/lib/openapi/routes/coach.ts +++ b/src/lib/openapi/routes/coach.ts @@ -540,7 +540,7 @@ const coachMessageSchema = z .string() .nullable() .describe( - "Provider that produced the reply (e.g. anthropic, openai, local, refusal); null for user turns.", + "Provider that produced the reply (e.g. anthropic, openai, local, refusal). The sentinel `cancelled` marks an empty assistant row closing a turn the client aborted mid-generation (navigation away); clients should render it as an interrupted turn with a retry affordance. Null for user turns.", ), promptVersion: z .string()