From 05cb5cf7e901f03e35d816ffde7a8f93159e9c51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 25 Oct 2026 00:54:00 +0200 Subject: [PATCH] fix(insights): stop a briefing's model calls once its bound gives up The nightly warm, the forced warm and the morning refresh bound a comprehensive generation and abort it when the bound fires, but the abort only reached the generator's final check before the cache write. The provider calls never saw it: the call in flight ran on detached, and a reply that landed after the bound could still start the JSON-shape, grounding or screen retry, each a full generation nobody was waiting for. runBriefingCompletion now takes the caller's signal, hands it to the provider call, and reserves and sends nothing once it has fired. The generator passes it at all five of its calls (the generation, its three retries and the phrasing re-roll). The on-demand route passes none on purpose: it finishes and writes its briefing after the browser stopped waiting. Refs #1279 --- .../baselines/version-stamp-comments.json | 4 +-- .../briefing-provider-budget.test.ts | 24 +++++++++++++++ .../comprehensive-warm-timeout.test.ts | 28 +++++++++++++++++ src/lib/insights/briefing-provider.ts | 11 +++++++ src/lib/insights/comprehensive-generate.ts | 30 +++++++++---------- 5 files changed, 80 insertions(+), 17 deletions(-) diff --git a/src/__tests__/baselines/version-stamp-comments.json b/src/__tests__/baselines/version-stamp-comments.json index 2892e75da..deaf95332 100644 --- a/src/__tests__/baselines/version-stamp-comments.json +++ b/src/__tests__/baselines/version-stamp-comments.json @@ -1,7 +1,7 @@ { "$comment": "Comment lines that open with a release version, per file under src/. Maintained by src/__tests__/version-stamp-ratchet.test.ts.", "count": 2897, - "total": 9259, + "total": 9256, "entries": { "__tests__/account-payload-consumer-guard.test.ts": 2, "__tests__/acting-account-boundary-guard.test.ts": 7, @@ -2158,7 +2158,7 @@ "lib/insights/chart-tokens.ts": 24, "lib/insights/coach-launch-context.tsx": 10, "lib/insights/comprehensive-aggregator.ts": 27, - "lib/insights/comprehensive-generate.ts": 48, + "lib/insights/comprehensive-generate.ts": 45, "lib/insights/correlation-channel-series.ts": 16, "lib/insights/correlation-discovery.ts": 30, "lib/insights/correlation-series-builders.ts": 1, diff --git a/src/lib/insights/__tests__/briefing-provider-budget.test.ts b/src/lib/insights/__tests__/briefing-provider-budget.test.ts index bd9ff8143..cf19b4441 100644 --- a/src/lib/insights/__tests__/briefing-provider-budget.test.ts +++ b/src/lib/insights/__tests__/briefing-provider-budget.test.ts @@ -147,6 +147,30 @@ describe("runBriefingCompletion — ledger accounting", () => { expect(ledgerTotal).toBe(1234 + 2700); }); + it("reserves and sends nothing once the caller's bound fired (#1279)", async () => { + mockProviderReply(100); + const controller = new AbortController(); + controller.abort(); + + await expect( + runBriefingCompletion(completionArgs({ signal: controller.signal })), + ).rejects.toThrow(); + + expect(runRawCompletionWithFallback).not.toHaveBeenCalled(); + expect(ledgerTotal).toBe(0); + }); + + it("hands the bound's signal to the provider call", async () => { + mockProviderReply(100); + const controller = new AbortController(); + + await runBriefingCompletion(completionArgs({ signal: controller.signal })); + + expect(runRawCompletionWithFallback.mock.calls[0][0].params.signal).toBe( + controller.signal, + ); + }); + it("charges the reservation when the provider reports no token count", async () => { mockProviderReply(null); diff --git a/src/lib/insights/__tests__/comprehensive-warm-timeout.test.ts b/src/lib/insights/__tests__/comprehensive-warm-timeout.test.ts index e90362f4e..5468a5de7 100644 --- a/src/lib/insights/__tests__/comprehensive-warm-timeout.test.ts +++ b/src/lib/insights/__tests__/comprehensive-warm-timeout.test.ts @@ -155,6 +155,34 @@ describe("comprehensive warm-path timeout + failure observability", () => { expect(params.timeoutMs).toBe(AI_BUDGETS.comprehensive.timeoutMs); }); + it("sends nothing more once the caller's bound has given up (#1279)", async () => { + // A slow model: its reply (not valid JSON, so the generator would ask + // again) lands only after the warm's bound fired and aborted the run. + const controller = new AbortController(); + runRawCompletionWithFallback.mockImplementation(async () => { + controller.abort(); + return { + result: { content: "not json", tokensUsed: 10, model: "m" }, + workingProvider: { providerType: "codex" }, + fallbackHops: [], + tokensSpent: 0, + }; + }); + + await generateComprehensiveInsight("u1", { + locale: "de", + signal: controller.signal, + }); + + // The call in flight carried the bound's signal, so the client could tear + // its request down, and no retry left after the bound. + expect(runRawCompletionWithFallback).toHaveBeenCalledTimes(1); + expect(runRawCompletionWithFallback.mock.calls[0][0].params.signal).toBe( + controller.signal, + ); + expect(userUpdate).not.toHaveBeenCalled(); + }); + it("does not silently persist an empty block when every provider fails", async () => { // The mocked class ignores the argument; `[]` satisfies the real type. runRawCompletionWithFallback.mockRejectedValue( diff --git a/src/lib/insights/briefing-provider.ts b/src/lib/insights/briefing-provider.ts index ea4e493ae..d4758b9a2 100644 --- a/src/lib/insights/briefing-provider.ts +++ b/src/lib/insights/briefing-provider.ts @@ -112,6 +112,15 @@ export interface RunBriefingCompletionArgs { * Absent for the scheduled generator. */ interactive?: boolean; + /** + * #1279 — the caller's outer bound (the nightly and forced warms, the + * morning refresh). It reaches every provider call, so a call the bound + * gave up on is torn down, and once it fired no further call is reserved + * or sent: a briefing that outlives its bound does not keep running + * detached. The on-demand route passes none on purpose: it finishes and + * writes its briefing after the browser stopped waiting (#786). + */ + signal?: AbortSignal; } /** @@ -131,6 +140,7 @@ export interface RunBriefingCompletionArgs { export async function runBriefingCompletion( args: RunBriefingCompletionArgs, ): Promise { + args.signal?.throwIfAborted(); const dateKey = args.dateKey ?? (await buildDateKey(args.userId)); // v1.41 — one resolution per call: the operator's switch and cap, the payer @@ -195,6 +205,7 @@ export async function runBriefingCompletion( maxTokens: args.maxTokens, seed: args.seed, timeoutMs: args.timeoutMs, + signal: args.signal, // Every briefing call parses its reply with `JSON.parse`, so opt the // chains with a native JSON mode into it. responseFormat: "json", diff --git a/src/lib/insights/comprehensive-generate.ts b/src/lib/insights/comprehensive-generate.ts index 8ae3feb59..a5fb5d5a9 100644 --- a/src/lib/insights/comprehensive-generate.ts +++ b/src/lib/insights/comprehensive-generate.ts @@ -244,6 +244,8 @@ async function rerollBriefingParagraph(args: { * slow-backend allowance the main generation does. */ effectiveTimeoutMs: number; + /** The caller's bound: a re-roll it gave up on sends nothing more. */ + signal?: AbortSignal; }): Promise<{ text: string; providerType: string } | null> { let cached: Record; try { @@ -277,6 +279,7 @@ async function rerollBriefingParagraph(args: { // generation is not clipped at the client's 60 s default mid-stream. // v1.25 — honours the per-user response-timeout setting (see caller). timeoutMs: args.effectiveTimeoutMs, + signal: args.signal, stage: "reroll", // v1.41 — the re-roll is a fresh pass at the paragraph, so it may think // like the generation it replaces; the operator's switch still wins. @@ -716,13 +719,10 @@ interface GenerateOptions { /** Skip the 24 h cache short-circuit and force a fresh generation. */ force?: boolean; /** - * v1.9.0 — abort the generation when the caller has given up on it. - * - * The on-demand `forceWarmUser` path bounds the comprehensive step with a - * timeout and then proceeds to warm the per-status caches itself. Without - * this signal a comprehensive that resolves AFTER the timeout would still - * write a cache row + timestamp the caller no longer expects. The signal - * lets the caller cut the generation off before the cache write. + * The caller's bound gave up on the generation (the warms bound it, then + * move on). Every provider call carries it, so the call in flight is torn + * down and no later one is sent (#1279), and a generation that still + * resolves writes no cache row the caller no longer expects. */ signal?: AbortSignal; } @@ -1141,6 +1141,7 @@ export async function generateComprehensiveInsight( features, comparison: groundingExtra, effectiveTimeoutMs, + signal, }); if (rerolled) { // The reroll rewrites content derived from the scope captured before @@ -1232,6 +1233,7 @@ export async function generateComprehensiveInsight( // generation is not clipped at the client's 60 s default mid-stream. // v1.25 — honours the per-user response-timeout setting. timeoutMs: effectiveTimeoutMs, + signal, stage: "generate", // v1.41 — the briefing is one of the few background jobs that may // reason (medium on the person's own provider, low on the operator's, @@ -1328,6 +1330,7 @@ export async function generateComprehensiveInsight( // v1.21.5 — wider upstream budget; see the first generation call. // v1.25 — honours the per-user response-timeout setting. timeoutMs: effectiveTimeoutMs, + signal, stage: "json-retry", }); result = retry.result; @@ -1421,6 +1424,7 @@ export async function generateComprehensiveInsight( // v1.21.5 — wider upstream budget; see the first generation call. // v1.25 — honours the per-user response-timeout setting. timeoutMs: effectiveTimeoutMs, + signal, stage: "grounding-retry", }); const retryInsights = parseComprehensiveResult(retry.result.content); @@ -1553,6 +1557,7 @@ export async function generateComprehensiveInsight( temperature: AI_BUDGETS.comprehensive.temperature, maxTokens: resolveInsightsMaxTokens(), timeoutMs: effectiveTimeoutMs, + signal, stage: "screen-retry", }); const retryInsights = parseComprehensiveResult(retry.result.content); @@ -1594,15 +1599,10 @@ export async function generateComprehensiveInsight( } } - // v1.9.0 — the caller abandoned this generation (its bounded timeout - // fired and it has moved on to warm the per-status caches itself). Bail - // out before the cache write so a late-resolving comprehensive cannot - // stamp a timestamp/text the caller no longer expects. The provider call - // already cost what it cost; what we must NOT do now is touch the cache. + // The caller abandoned this generation (its bound fired). Bail out before + // the cache write, annotated and marked like every other failure: an + // abandoned generation is a real "no fresh briefing tonight" outcome. if (signal?.aborted) { - // v1.28.30 — annotated + marked like every other failure: an abandoned - // generation is a real "no fresh briefing tonight" outcome, and the read - // path needs the marker to render an honest state instead of silence. return failBeforeProvider(userId, locale, "aborted"); }