From 2c6a9f8165fe3d58ea096184146b261aa8605c3b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Tue, 2 Jun 2026 16:21:45 +0200 Subject: [PATCH] fix(insights): decouple the forced warm sections so a slow comprehensive step cannot starve the rest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The on-demand force-warm ran the comprehensive insight first and only ran the per-status (7) and generic-metric (~30) assessment passes when that step returned generated or cached. A slow or stalled comprehensive generation therefore aborted the entire warm — observed in production as a 102 s run ending with assessments_warmed: 0 — leaving every card cold and the user back on the lazy 30-60 s on-first-click path. Give the comprehensive step its own bounded budget and treat a timeout, error, or skip as non-fatal: the per-status and generic-metric passes now always run afterwards. Each per-card generator resolves its own provider chain and bounds its own provider call, so running them after a failed comprehensive costs a cheap chain-resolve when no provider is configured and is capped per card otherwise. The result reports each section's outcome independently, and a timed-out comprehensive is reported distinctly from a failed one so a partial warm is observable. Co-Authored-By: Claude Opus 4.8 (1M context) (cherry picked from commit f52f8e9c3b32590f3b085d082418ebdd3a923f1d) --- .../__tests__/insight-pregenerate.test.ts | 94 ++++++++++++++++++- src/lib/jobs/insight-pregenerate.ts | 84 +++++++++++++---- 2 files changed, 157 insertions(+), 21 deletions(-) diff --git a/src/lib/jobs/__tests__/insight-pregenerate.test.ts b/src/lib/jobs/__tests__/insight-pregenerate.test.ts index 572f28e50..83c478848 100644 --- a/src/lib/jobs/__tests__/insight-pregenerate.test.ts +++ b/src/lib/jobs/__tests__/insight-pregenerate.test.ts @@ -415,15 +415,22 @@ describe("forceWarmUser — on-demand single-user warm (v1.8.7.1)", () => { }); }); - it("does NOT warm the per-status or generic caches when the comprehensive pass skipped (no provider)", async () => { + it("still runs the per-status + generic warm even when the comprehensive pass skipped (no provider)", async () => { + // v1.9.0 — the three sections are decoupled. A skipped comprehensive + // (no provider) must NOT short-circuit the per-status + generic passes: + // each generator resolves its own provider chain and no-ops cheaply when + // none is configured, so running them is safe and is what warms the cards + // when the comprehensive briefing is the only thing missing a provider. const { prisma } = makePrisma([]); const generate = vi .fn() .mockResolvedValue({ status: "skipped", reason: "no-provider" }); + // Each generator independently reports no-provider — nothing warmed, but + // it WAS invoked (it is the generator, not the caller, that decides). const statusGenerators = Array.from({ length: 7 }, () => vi.fn().mockResolvedValue({ hasProvider: false, cached: true }), ); - const warmGenericMetrics = vi.fn().mockResolvedValue(5); + const warmGenericMetrics = vi.fn().mockResolvedValue(0); const result = await forceWarmUser(prisma as never, "u1", "de", { generate, @@ -432,9 +439,10 @@ describe("forceWarmUser — on-demand single-user warm (v1.8.7.1)", () => { }); for (const g of statusGenerators) { - expect(g).not.toHaveBeenCalled(); + expect(g).toHaveBeenCalledTimes(1); + expect(g).toHaveBeenCalledWith("u1", { locale: "de", force: true }); } - expect(warmGenericMetrics).not.toHaveBeenCalled(); + expect(warmGenericMetrics).toHaveBeenCalledWith("u1", ["de"]); expect(result).toMatchObject({ comprehensive: "skipped", assessmentsWarmed: 0, @@ -442,6 +450,84 @@ describe("forceWarmUser — on-demand single-user warm (v1.8.7.1)", () => { }); }); + it("warms the per-status + generic caches even when the comprehensive pass fails", async () => { + // v1.9.0 — a failed comprehensive (provider chain unhealthy this run) + // is non-fatal: the cheaper per-status (7) + generic (~30) cards still + // warm so the user's first click is a cache read, not a 30–60 s lazy + // generation. This is the prod regression the stability wave closes. + const { prisma } = makePrisma([]); + const generate = vi + .fn() + .mockResolvedValue({ status: "failed", reason: "all-providers-failed" }); + const statusGenerators = Array.from({ length: 7 }, () => + vi.fn().mockResolvedValue({ hasProvider: true, cached: false }), + ); + const warmGenericMetrics = vi.fn().mockResolvedValue(4); + + const result = await forceWarmUser(prisma as never, "u1", "de", { + generate, + statusGenerators, + warmGenericMetrics, + }); + + for (const g of statusGenerators) { + expect(g).toHaveBeenCalledTimes(1); + } + expect(warmGenericMetrics).toHaveBeenCalledWith("u1", ["de"]); + expect(result).toMatchObject({ + comprehensive: "failed", + assessmentsWarmed: 7, + metricAssessmentsWarmed: 4, + }); + }); + + it("reports a timed-out comprehensive distinctly and still warms the rest", async () => { + // A comprehensive generation that exceeds its bounded budget is + // abandoned and reported as `timeout`, but the per-status + generic + // passes run regardless. The prod incident was a 102 s comprehensive + // that aborted the whole warm with `assessments_warmed: 0`. + const { prisma } = makePrisma([]); + const generate = vi.fn().mockImplementation( + () => + new Promise((resolve) => { + // Never settles within the bounded budget; the fake timers below + // advance past it so the warm continues. + setTimeout( + () => resolve({ status: "generated", providerType: "x" }), + 120_000, + ); + }), + ); + const statusGenerators = Array.from({ length: 7 }, () => + vi.fn().mockResolvedValue({ hasProvider: true, cached: false }), + ); + const warmGenericMetrics = vi.fn().mockResolvedValue(3); + + vi.useFakeTimers(); + try { + const promise = forceWarmUser(prisma as never, "u1", "de", { + generate, + statusGenerators, + warmGenericMetrics, + }); + // Advance past the comprehensive budget so withTimeout fires. + await vi.advanceTimersByTimeAsync(60_000); + const result = await promise; + + for (const g of statusGenerators) { + expect(g).toHaveBeenCalledTimes(1); + } + expect(warmGenericMetrics).toHaveBeenCalledWith("u1", ["de"]); + expect(result).toMatchObject({ + comprehensive: "timeout", + assessmentsWarmed: 7, + metricAssessmentsWarmed: 3, + }); + } finally { + vi.useRealTimers(); + } + }); + it("short-circuits to a no-op when the briefing surface is disabled globally", async () => { getAssistantFlags.mockResolvedValue({ enabled: false, briefing: false }); const { prisma } = makePrisma([]); diff --git a/src/lib/jobs/insight-pregenerate.ts b/src/lib/jobs/insight-pregenerate.ts index 780dc3037..c189c4171 100644 --- a/src/lib/jobs/insight-pregenerate.ts +++ b/src/lib/jobs/insight-pregenerate.ts @@ -68,6 +68,7 @@ import { getMetricStatusMeta, type MetricStatusMetricId, } from "@/lib/insights/metric-status-registry"; +import { withTimeout } from "@/lib/insights/with-timeout"; import { INSIGHT_PREGENERATE_QUEUE, enqueueForceWarm, @@ -109,10 +110,28 @@ const PREGENERATE_BUDGET_WINDOW_MS = 20 * 60 * 60 * 1000; */ const WARM_PASS_CONCURRENCY = 3; +/** + * Bounded budget for the comprehensive step of a forced full warm. The + * comprehensive insight is the single heaviest generation (full feature + * extraction + a 1500-token completion) and a slow or stalled provider + * could pin the whole warm for a minute-plus, after which the entire job + * aborted with the per-status + generic-metric passes never run. Capping + * it lets the forced path abandon a slow comprehensive and still warm the + * cheaper per-status (7) + generic-metric (~30) assessments, so the user's + * cards are warm even when the briefing generation stalls. Set a hair + * below the queue's own retry horizon so a single tick stays bounded. + */ +const FORCE_WARM_COMPREHENSIVE_TIMEOUT_MS = 45_000; + /** Per-user result of one forced full warm. */ export interface ForceWarmResult { - /** Outcome of the comprehensive-insight generation. */ - comprehensive: GenerateOutcome["status"]; + /** + * Outcome of the comprehensive-insight generation. `"timeout"` is + * distinct from `"failed"`: the comprehensive step exceeded its own + * bounded budget (see `FORCE_WARM_COMPREHENSIVE_TIMEOUT_MS`) and was + * abandoned so the per-status + generic-metric passes could still run. + */ + comprehensive: GenerateOutcome["status"] | "timeout"; /** Count of specialised per-status assessments written warm. */ assessmentsWarmed: number; /** Count of generic per-HealthKit-metric assessments written warm. */ @@ -493,11 +512,20 @@ export async function runInsightPregenerate( * the queue `singletonKey`. * * Unlike the nightly cron (de+en), the forced path warms only the caller's - * active `locale` so a single-locale user pays half the provider cost. A - * `failed`/`skipped` comprehensive outcome short-circuits the per-status - * warm exactly as the cron does — re-running seven (then ~30) generators - * against an unhealthy or provider-less chain would only multiply the - * failure. + * active `locale` so a single-locale user pays half the provider cost. + * + * The three sections are decoupled: the comprehensive step runs under its + * own bounded budget (`FORCE_WARM_COMPREHENSIVE_TIMEOUT_MS`) and its + * failure / timeout / skip is non-fatal — the per-status (7) and + * generic-metric (~30) warm passes ALWAYS run afterwards. The earlier + * design short-circuited both passes on a non-`generated`/`cached` + * comprehensive, so a single slow briefing generation left every card cold + * and the user back on the lazy 30–60 s on-first-click path. The per-card + * generators each resolve their own provider chain and bound their own + * provider call, so a missing provider costs a cheap chain-resolve and a + * degraded one is capped per card — running them after a failed + * comprehensive can never multiply a stall. The result reports each + * section's outcome independently so a partial warm is observable. */ export async function forceWarmUser( prisma: PrismaClient, @@ -536,18 +564,40 @@ export async function forceWarmUser( const locales: ReadonlyArray<"de" | "en"> = [locale]; - const outcome = await generate(userId, { locale, force: true }); - result.comprehensive = outcome.status; - - if (outcome.status === "generated" || outcome.status === "cached") { - result.assessmentsWarmed = await warmPerStatusCaches( - userId, - locales, - statusGenerators, - ); - result.metricAssessmentsWarmed = await warmGenericMetrics(userId, locales); + // The comprehensive step gets its own bounded budget and its failure is + // non-fatal. A slow or stalled briefing generation must not short-circuit + // the per-status + generic-metric passes — those are the ~37 cards the + // user actually clicks first, and warming them is independent of the + // comprehensive insight (each generator resolves its own provider chain + // and writes its own cache row). A timeout / error / skipped comprehensive + // now falls through to the warm passes instead of aborting the whole job. + const comprehensive = await withTimeout( + () => generate(userId, { locale, force: true }), + FORCE_WARM_COMPREHENSIVE_TIMEOUT_MS, + null, + ); + if (comprehensive.timedOut) { + result.comprehensive = "timeout"; + } else if (comprehensive.errored || comprehensive.value === null) { + result.comprehensive = "failed"; + } else { + result.comprehensive = comprehensive.value.status; } + // Always run the per-status + generic-metric warm passes, regardless of + // the comprehensive outcome. Each generator independently no-ops to its + // no-key fallback when no provider is configured (no LLM call) and serves + // its fallback without persisting on a per-card timeout, so running them + // after a failed comprehensive is safe and bounded by their own per-card + // budgets. The only outcome that has nothing to warm is a missing provider, + // and the generators detect that themselves at near-zero cost. + result.assessmentsWarmed = await warmPerStatusCaches( + userId, + locales, + statusGenerators, + ); + result.metricAssessmentsWarmed = await warmGenericMetrics(userId, locales); + annotate({ action: { name: "insights.pregenerate.force" }, meta: {