From b4c858434dfe607ce3df900777868a8e81cc154d Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Mon, 13 Jul 2026 02:49:07 +0800 Subject: [PATCH 1/5] feat(observability): add retrieval cache hit-rate counter to /api/health deep probe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes runbook item 3.2 (observability/alerts/rollback). The answer-SLO snapshot from #513 already exposes the hybrid_rpc_errors and degraded/source-only rates; the one remaining silent-degradation counter — cache hit-rate — is now instrumented in-process at the retrieval hot path and surfaced on the deep probe. - New src/lib/observability/cache-metrics.ts: cumulative process-start hit/miss counters (Prometheus-style; a scraper derives a windowed rate from deltas). - getCachedSearch records exactly one hit/miss per cache-enabled lookup (skipCache / TTL-or-size-0 records neither). - Health route exposes a `cache` block for any authorized deep probe (in-process, so it works in demo mode too); like `slo`, it never flips 200/503 liveness. - Refresh docs/observability-slos.md: the /api/health extension is now shipped (both `slo` and `cache`); only alert-channel wiring + host metrics remain. Tests: cache-metrics unit test + a health-route deep-probe case; full vitest suite green (1682 passed, 1 skipped). Co-Authored-By: Claude Opus 4.8 --- docs/observability-slos.md | 52 ++++++++++++++++++++++---- src/app/api/health/route.ts | 51 ++++++++++++++----------- src/lib/observability/cache-metrics.ts | 41 ++++++++++++++++++++ src/lib/rag-cache.ts | 14 +++++++ tests/cache-metrics.test.ts | 33 ++++++++++++++++ tests/health-route.test.ts | 34 ++++++++++++++--- 6 files changed, 191 insertions(+), 34 deletions(-) create mode 100644 src/lib/observability/cache-metrics.ts create mode 100644 tests/cache-metrics.test.ts diff --git a/docs/observability-slos.md b/docs/observability-slos.md index 2dc48c74a..820270e7d 100644 --- a/docs/observability-slos.md +++ b/docs/observability-slos.md @@ -168,13 +168,49 @@ Operational notes: `workflow_dispatch` run and confirm it goes green before trusting the nightly cadence (repo gate for this workflow). -## 4. Gaps / next steps (not in this change) +## 4. Degradation counters on `/api/health` (shipped) + +The §2 reliability SQL is now also a scrape. An **authorized deep probe** — +`GET /api/health?deep=1` with the `x-health-deep-token: $HEALTH_DEEP_PROBE_SECRET` +header (same operator gate as the Supabase probe) — returns two counter blocks: + +- **`slo`** — `answerSloSnapshot` (`src/lib/observability/answer-slo.ts`) counts + `rag_queries` over the trailing `windowMinutes` (60) and reports + `hybridRpcErrorQueries` / `hybridRpcErrorRate` (the §2 silent-RPC-death guard) + and `degradedQueries` / `degradedRate` (the source-only/fallback guard). These + are windowed rates straight from the persisted telemetry. + +- **`cache`** — `cacheMetricsSnapshot` (`src/lib/observability/cache-metrics.ts`) + reports `{ lookups, hits, misses, hitRate }` for the retrieval search cache, + incremented in-process at the `getCachedSearch` hot path (`src/lib/rag-cache.ts`). + These are **cumulative since process start** (Prometheus-style): a scraper + derives a windowed hit-rate from the delta between two polls. Measuring at the + lookup site — not from `rag_queries` — also captures coalesced and fully + cache-served requests that never write a telemetry row. `hitRate` is a + convenience for eyeballing a single probe. + +```jsonc +// GET /api/health?deep=1 (authorized, live) +"slo": { "windowMinutes": 60, "totalQueries": 412, + "hybridRpcErrorQueries": 0, "hybridRpcErrorRate": 0.0, + "degradedQueries": 26, "degradedRate": 0.063 }, +"cache": { "lookups": 1873, "hits": 1402, "misses": 471, "hitRate": 0.748 } +``` -- Host-level metrics (CPU, memory, restart count) and log drains once the +**Neither block flips liveness.** The probe stays `200` even when a rate is bad; +a Supabase-side RPC fault or a cold cache must not trip the container +`HEALTHCHECK` into a restart it cannot fix. Degradation is made _visible_, not +_fatal_ — exactly the failure mode this doc guards against. The `slo` query +throwing degrades to an absent `slo` block (not a false-healthy zero); the cache +counter is always present for an authorized probe (in-process, works in demo +mode). The §2 warn/page thresholds map directly onto these fields. + +## 5. Gaps / next steps + +- **Wire the warn/page thresholds into an actual alerting channel.** The §4 + counters are now scrapeable; the remaining work is routing them past their §2 + thresholds (and the 3 h-sustained hybrid escalation) into a real channel — a + host-native alerter polling `/api/health?deep=1`, or a scheduled workflow + evaluating the §2 SQL. +- **Host-level metrics** (CPU, memory, restart count) and log drains once the container host exists (`docs/deployment-architecture.md` §2). -- A lightweight `/api/health` extension exposing cache hit-rate and - `hybrid_rpc_errors`-in-last-hour counters for host-native alerting, so the - SQL above can become a scrape instead of a manual query. (Touches app code — - deliberately excluded from this change.) -- Wire the warn/page thresholds into an actual alerting channel (Supabase log - drain → host alerts, or a scheduled workflow evaluating the SQL above). diff --git a/src/app/api/health/route.ts b/src/app/api/health/route.ts index b53de6313..2c997f70e 100644 --- a/src/app/api/health/route.ts +++ b/src/app/api/health/route.ts @@ -1,6 +1,7 @@ import { NextResponse } from "next/server"; import { env, isDemoMode } from "@/lib/env"; import type { AnswerSloSnapshot } from "@/lib/observability/answer-slo"; +import { cacheMetricsSnapshot, type CacheMetricsSnapshot } from "@/lib/observability/cache-metrics"; import { allowDeepHealthProbe } from "@/lib/deep-probe-auth"; export const runtime = "nodejs"; @@ -16,33 +17,40 @@ export async function GET(request: Request) { }; let slo: AnswerSloSnapshot | null = null; + let cache: CacheMetricsSnapshot | null = null; if (deep) { if (!allowDeepHealthProbe(request)) { checks.supabase = "unauthorized"; - } else if (supabaseConfigured && !isDemoMode()) { - try { - const [{ createAdminClient }, { probeSupabaseHealth }, { answerSloSnapshot }] = await Promise.all([ - import("@/lib/supabase/admin"), - import("@/lib/supabase/health"), - import("@/lib/observability/answer-slo"), - ]); - const admin = createAdminClient(); - const health = await probeSupabaseHealth(admin); - checks.supabase = health.ok ? "ok" : "error"; - if (health.ok) { - // Reliability telemetry only — a failure here must NOT flip liveness to - // 503, so it stays out of `checks` (which gates `ready`). - try { - slo = await answerSloSnapshot(admin); - } catch { - slo = null; + } else { + // In-process retrieval cache hit-rate — the hot-path half of the + // silent-degradation counters. It reads a cumulative counter (no DB), so + // it is available to any authorized deep probe, including in demo mode. + cache = cacheMetricsSnapshot(); + if (supabaseConfigured && !isDemoMode()) { + try { + const [{ createAdminClient }, { probeSupabaseHealth }, { answerSloSnapshot }] = await Promise.all([ + import("@/lib/supabase/admin"), + import("@/lib/supabase/health"), + import("@/lib/observability/answer-slo"), + ]); + const admin = createAdminClient(); + const health = await probeSupabaseHealth(admin); + checks.supabase = health.ok ? "ok" : "error"; + if (health.ok) { + // Reliability telemetry only — a failure here must NOT flip liveness to + // 503, so it stays out of `checks` (which gates `ready`). + try { + slo = await answerSloSnapshot(admin); + } catch { + slo = null; + } } + } catch { + checks.supabase = "error"; } - } catch { - checks.supabase = "error"; + } else { + checks.supabase = "skipped"; } - } else { - checks.supabase = "skipped"; } } @@ -58,6 +66,7 @@ export async function GET(request: Request) { uptimeSeconds: Math.round(process.uptime()), checks, ...(slo ? { slo } : {}), + ...(cache ? { cache } : {}), }, { status: ready ? 200 : 503, headers: { "Cache-Control": "no-store" } }, ); diff --git a/src/lib/observability/cache-metrics.ts b/src/lib/observability/cache-metrics.ts new file mode 100644 index 000000000..a3cb4e542 --- /dev/null +++ b/src/lib/observability/cache-metrics.ts @@ -0,0 +1,41 @@ +// In-process cache-lookup counters for the retrieval hot path. +// +// The cache hit-rate half of the /api/health silent-degradation counters (see +// docs/observability-slos.md). Its sibling `answer-slo.ts` aggregates +// `rag_queries` over a trailing window; cache effectiveness instead has to be +// measured where the lookups actually happen (rag-cache.ts) so it also covers +// requests that never write a rag_queries row — coalesced or fully cache-served. +// +// Counters are cumulative since process start (Prometheus-style): a host-native +// scraper derives a windowed hit-rate from the delta between two polls, which is +// exactly what a counter is for. `hitRate` is a convenience for eyeballing a +// single probe. Only cache-enabled lookups are recorded — a request that skips +// the cache (skipCache / TTL or size 0) is neither a hit nor a miss. + +let lookups = 0; +let hits = 0; + +/** Record one retrieval-cache lookup with its outcome. Cheap; safe on the hot path. */ +export function recordCacheLookup(hit: boolean): void { + lookups += 1; + if (hit) hits += 1; +} + +export type CacheMetricsSnapshot = { + lookups: number; + hits: number; + misses: number; + // 0..1; 0 when there have been no lookups yet (avoid divide-by-zero noise). + hitRate: number; +}; + +export function cacheMetricsSnapshot(): CacheMetricsSnapshot { + const misses = lookups - hits; + return { lookups, hits, misses, hitRate: lookups > 0 ? hits / lookups : 0 }; +} + +/** Test-only: reset the counters between cases. */ +export function resetCacheMetrics(): void { + lookups = 0; + hits = 0; +} diff --git a/src/lib/rag-cache.ts b/src/lib/rag-cache.ts index b9fce6b59..5e2868144 100644 --- a/src/lib/rag-cache.ts +++ b/src/lib/rag-cache.ts @@ -4,6 +4,7 @@ import { buildClinicalTextSearchQuery } from "@/lib/clinical-search"; import { readExpiringCacheEntry, writeBoundedExpiringCacheEntry } from "@/lib/bounded-ttl-cache"; import { ragDeepMemoryVersion } from "@/lib/deep-memory"; import { env } from "@/lib/env"; +import { recordCacheLookup } from "@/lib/observability/cache-metrics"; import { queryCacheKeyForStorage } from "@/lib/query-privacy"; import { ragCacheKeyMatchesOwner } from "@/lib/rag-cache-utils"; import { compactContextText } from "@/lib/rag-source-block"; @@ -184,8 +185,21 @@ export async function getCachedSearch( queryClass?: RagQueryClass, queryVariants: string[] = [], ): Promise<{ results: SearchResult[]; telemetry: SearchTelemetry } | null> { + // Caching disabled for this call — neither a hit nor a miss, so it stays out + // of the hit-rate counter entirely. if (args.skipCache || env.RAG_SEARCH_CACHE_TTL_MS <= 0 || env.RAG_SEARCH_CACHE_SIZE <= 0) return null; + const cached = await lookupCachedSearch(args, queryClass, queryVariants); + // Cache hit-rate telemetry for the deep /api/health probe (docs/observability-slos.md §4). + recordCacheLookup(cached !== null); + return cached; +} + +async function lookupCachedSearch( + args: SearchChunksArgs, + queryClass?: RagQueryClass, + queryVariants: string[] = [], +): Promise<{ results: SearchResult[]; telemetry: SearchTelemetry } | null> { const key = scopedSearchCacheKey(args, queryClass, queryVariants); const cached = searchCache.get(key); if (!cached) return null; diff --git a/tests/cache-metrics.test.ts b/tests/cache-metrics.test.ts new file mode 100644 index 000000000..0891eb14e --- /dev/null +++ b/tests/cache-metrics.test.ts @@ -0,0 +1,33 @@ +import { afterEach, describe, expect, it } from "vitest"; + +import { cacheMetricsSnapshot, recordCacheLookup, resetCacheMetrics } from "@/lib/observability/cache-metrics"; + +afterEach(() => { + resetCacheMetrics(); +}); + +describe("cache metrics counter", () => { + it("reports a zero hit-rate (not NaN) before any lookups", () => { + expect(cacheMetricsSnapshot()).toEqual({ lookups: 0, hits: 0, misses: 0, hitRate: 0 }); + }); + + it("accumulates hits and misses and derives the hit-rate", () => { + recordCacheLookup(true); + recordCacheLookup(true); + recordCacheLookup(true); + recordCacheLookup(false); + + expect(cacheMetricsSnapshot()).toEqual({ lookups: 4, hits: 3, misses: 1, hitRate: 0.75 }); + }); + + it("is cumulative across calls until reset", () => { + recordCacheLookup(false); + expect(cacheMetricsSnapshot()).toMatchObject({ lookups: 1, hits: 0, misses: 1, hitRate: 0 }); + + recordCacheLookup(true); + expect(cacheMetricsSnapshot()).toMatchObject({ lookups: 2, hits: 1, misses: 1, hitRate: 0.5 }); + + resetCacheMetrics(); + expect(cacheMetricsSnapshot()).toEqual({ lookups: 0, hits: 0, misses: 0, hitRate: 0 }); + }); +}); diff --git a/tests/health-route.test.ts b/tests/health-route.test.ts index cad01debb..fbe8da0f3 100644 --- a/tests/health-route.test.ts +++ b/tests/health-route.test.ts @@ -1,19 +1,22 @@ import { afterEach, describe, expect, it, vi } from "vitest"; -function mockEnv(options: { configured: boolean; demoMode?: boolean }) { +const DEEP_TOKEN = "deep-probe-secret"; + +function mockEnv(options: { configured: boolean; demoMode?: boolean; deepSecret?: boolean }) { vi.resetModules(); vi.doMock("@/lib/env", () => ({ env: { NEXT_PUBLIC_SUPABASE_URL: options.configured ? "https://sjrfecxgysukkwxsowpy.supabase.co" : undefined, SUPABASE_SERVICE_ROLE_KEY: options.configured ? "service-role-key" : undefined, OPENAI_API_KEY: options.configured ? "openai-key" : undefined, + HEALTH_DEEP_PROBE_SECRET: options.deepSecret ? DEEP_TOKEN : undefined, }, isDemoMode: () => Boolean(options.demoMode), })); } -function healthRequest(query = "") { - return new Request(`http://localhost/api/health${query}`); +function healthRequest(query = "", headers?: HeadersInit) { + return new Request(`http://localhost/api/health${query}`, headers ? { headers } : undefined); } async function payload(response: Response) { @@ -52,8 +55,8 @@ describe("GET /api/health", () => { expect(body.checks).toMatchObject({ supabaseConfig: "missing", openaiConfig: "missing" }); }); - it("gates the deep probe without a token and omits the slo snapshot", async () => { - mockEnv({ configured: true }); + it("gates the deep probe without a token and omits the slo/cache snapshots", async () => { + mockEnv({ configured: true, deepSecret: true }); const { GET } = await import("../src/app/api/health/route"); const response = await GET(healthRequest("?deep=1")); @@ -62,6 +65,27 @@ describe("GET /api/health", () => { expect(response.status).toBe(503); expect(body.checks).toMatchObject({ supabase: "unauthorized" }); expect(body.slo).toBeUndefined(); + expect(body.cache).toBeUndefined(); + }); + + it("exposes the in-process cache hit-rate counter on an authorized deep probe", async () => { + // Demo mode skips the Supabase-backed slo query, so no admin mock is needed; + // the cache counter is in-process and must still be reported. + mockEnv({ configured: true, demoMode: true, deepSecret: true }); + const { GET } = await import("../src/app/api/health/route"); + + const response = await GET(healthRequest("?deep=1", { "x-health-deep-token": DEEP_TOKEN })); + const body = await payload(response); + + expect(response.status).toBe(200); + const cache = body.cache as Record; + // Counters are process-cumulative, so assert shape/invariants, not exact counts. + expect(typeof cache.lookups).toBe("number"); + expect(typeof cache.hits).toBe("number"); + expect(cache.misses).toBe(cache.lookups - cache.hits); + expect(cache.hitRate).toBeGreaterThanOrEqual(0); + expect(cache.hitRate).toBeLessThanOrEqual(1); + expect(body.slo).toBeUndefined(); }); it("does not leak secret values in the payload", async () => { From 2ee80270d282671b1764b5d2139746ae48363202 Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Mon, 13 Jul 2026 03:00:09 +0800 Subject: [PATCH 2/5] fix(observability): count shared-cache hits in the cache hit-rate counter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Codex P2 on #536. The counter was incremented inside getCachedSearch, which only sees the process-local cache — a local miss followed by a shared (rag_response_cache) hit was recorded as a miss, so a cold process or a multi-instance deployment with a warm shared cache would show a deflated hit-rate and could trip a false degradation alert. Move the recording to the two-layer cache orchestration in searchChunksWithTelemetry, recording exactly once with full knowledge of both layers via a pure classifySearchCacheOutcome(): a request served by either the local or the shared cache is a hit; a miss is counted only when a layer was consulted and missed; disabled/skipped lookups (shared returns null) record neither. getCachedSearch reverts to its original form. Tests: classifySearchCacheOutcome unit tests covering the local-miss/shared-hit regression, plus the disabled and both-miss cases. Co-Authored-By: Claude Opus 4.8 --- docs/observability-slos.md | 16 ++++++++------ src/lib/observability/cache-metrics.ts | 24 +++++++++++++++++++-- src/lib/rag-cache.ts | 14 ------------- src/lib/rag.ts | 11 +++++++++- tests/cache-metrics.test.ts | 29 +++++++++++++++++++++++++- 5 files changed, 70 insertions(+), 24 deletions(-) diff --git a/docs/observability-slos.md b/docs/observability-slos.md index 820270e7d..bd6dbfbea 100644 --- a/docs/observability-slos.md +++ b/docs/observability-slos.md @@ -182,12 +182,16 @@ header (same operator gate as the Supabase probe) — returns two counter blocks - **`cache`** — `cacheMetricsSnapshot` (`src/lib/observability/cache-metrics.ts`) reports `{ lookups, hits, misses, hitRate }` for the retrieval search cache, - incremented in-process at the `getCachedSearch` hot path (`src/lib/rag-cache.ts`). - These are **cumulative since process start** (Prometheus-style): a scraper - derives a windowed hit-rate from the delta between two polls. Measuring at the - lookup site — not from `rag_queries` — also captures coalesced and fully - cache-served requests that never write a telemetry row. `hitRate` is a - convenience for eyeballing a single probe. + incremented in-process at the two-layer cache orchestration in + `searchChunksWithTelemetry` (`src/lib/rag.ts`). A request served by **either** + the process-local or the shared (`rag_response_cache`) layer counts as a hit, + so a cold process falling through to a warm shared cache is not miscounted as a + miss; disabled/skipped lookups are recorded as neither. These are **cumulative + since process start** (Prometheus-style): a scraper derives a windowed hit-rate + from the delta between two polls. Measuring at the lookup site — not from + `rag_queries` — also captures coalesced and fully cache-served requests that + never write a telemetry row. `hitRate` is a convenience for eyeballing a single + probe. ```jsonc // GET /api/health?deep=1 (authorized, live) diff --git a/src/lib/observability/cache-metrics.ts b/src/lib/observability/cache-metrics.ts index a3cb4e542..df49316cd 100644 --- a/src/lib/observability/cache-metrics.ts +++ b/src/lib/observability/cache-metrics.ts @@ -3,8 +3,9 @@ // The cache hit-rate half of the /api/health silent-degradation counters (see // docs/observability-slos.md). Its sibling `answer-slo.ts` aggregates // `rag_queries` over a trailing window; cache effectiveness instead has to be -// measured where the lookups actually happen (rag-cache.ts) so it also covers -// requests that never write a rag_queries row — coalesced or fully cache-served. +// measured where the lookups actually happen (the search-cache orchestration in +// searchChunksWithTelemetry) so it also covers requests that never write a +// rag_queries row — coalesced or fully cache-served. // // Counters are cumulative since process start (Prometheus-style): a host-native // scraper derives a windowed hit-rate from the delta between two polls, which is @@ -39,3 +40,22 @@ export function resetCacheMetrics(): void { lookups = 0; hits = 0; } + +/** + * Classify the outcome of the two-layer search-cache lookup so the counter + * reflects real cache effectiveness. Retrieval consults the process-local cache + * first and the shared (`rag_response_cache`) cache second; a request served by + * *either* layer is a hit. A miss is only counted when a layer was actually + * consulted and returned nothing — when caching is disabled/skipped the shared + * lookup returns `null` and the request is `"skip"` (recorded as neither), so a + * cold process with a warm shared cache does not read as false degradation. + * Pure, so the orchestration stays trivial and this stays unit-tested. + */ +export function classifySearchCacheOutcome( + localHit: boolean, + sharedResult: { kind: "hit" | "miss" } | null | undefined, +): "hit" | "miss" | "skip" { + if (localHit || sharedResult?.kind === "hit") return "hit"; + if (sharedResult?.kind === "miss") return "miss"; + return "skip"; +} diff --git a/src/lib/rag-cache.ts b/src/lib/rag-cache.ts index 5e2868144..b9fce6b59 100644 --- a/src/lib/rag-cache.ts +++ b/src/lib/rag-cache.ts @@ -4,7 +4,6 @@ import { buildClinicalTextSearchQuery } from "@/lib/clinical-search"; import { readExpiringCacheEntry, writeBoundedExpiringCacheEntry } from "@/lib/bounded-ttl-cache"; import { ragDeepMemoryVersion } from "@/lib/deep-memory"; import { env } from "@/lib/env"; -import { recordCacheLookup } from "@/lib/observability/cache-metrics"; import { queryCacheKeyForStorage } from "@/lib/query-privacy"; import { ragCacheKeyMatchesOwner } from "@/lib/rag-cache-utils"; import { compactContextText } from "@/lib/rag-source-block"; @@ -185,21 +184,8 @@ export async function getCachedSearch( queryClass?: RagQueryClass, queryVariants: string[] = [], ): Promise<{ results: SearchResult[]; telemetry: SearchTelemetry } | null> { - // Caching disabled for this call — neither a hit nor a miss, so it stays out - // of the hit-rate counter entirely. if (args.skipCache || env.RAG_SEARCH_CACHE_TTL_MS <= 0 || env.RAG_SEARCH_CACHE_SIZE <= 0) return null; - const cached = await lookupCachedSearch(args, queryClass, queryVariants); - // Cache hit-rate telemetry for the deep /api/health probe (docs/observability-slos.md §4). - recordCacheLookup(cached !== null); - return cached; -} - -async function lookupCachedSearch( - args: SearchChunksArgs, - queryClass?: RagQueryClass, - queryVariants: string[] = [], -): Promise<{ results: SearchResult[]; telemetry: SearchTelemetry } | null> { const key = scopedSearchCacheKey(args, queryClass, queryVariants); const cached = searchCache.get(key); if (!cached) return null; diff --git a/src/lib/rag.ts b/src/lib/rag.ts index fe0fc6229..0d69af704 100644 --- a/src/lib/rag.ts +++ b/src/lib/rag.ts @@ -85,6 +85,7 @@ export { packedContextCacheKey, retrievalPlanCacheQuery, } from "@/lib/rag-cache"; +import { classifySearchCacheOutcome, recordCacheLookup } from "@/lib/observability/cache-metrics"; import { buildRagSourceBlock, compactContextText, neutralizeIdentityField } from "@/lib/rag-source-block"; export { buildRagSourceBlock, truncateForModel } from "@/lib/rag-source-block"; import { @@ -2985,8 +2986,16 @@ export async function searchChunksWithTelemetry(args: SearchChunksArgs) { const queryVariants = buildRetrievalQueryVariants(retrievalQuery, queryAnalysis, ragAliases); telemetry.retrieval_query_variant_count = queryVariants.length; const cached = await getCachedSearch(args, queryClassification.queryClass, queryVariants); + // Only consult the shared cache when the process-local cache missed (preserves + // the original short-circuit), then record the hit-rate counter ONCE with full + // knowledge of both layers: a request served by either cache is a hit, so a + // cold process that falls through to a warm shared cache is not miscounted as a + // miss (deep /api/health cache hit-rate — docs/observability-slos.md §4). + const sharedCached = cached ? null : await getSharedCachedSearch(args, queryClassification.queryClass, queryVariants); + const cacheOutcome = classifySearchCacheOutcome(Boolean(cached), sharedCached); + if (cacheOutcome !== "skip") recordCacheLookup(cacheOutcome === "hit"); + if (cached) return cached; - const sharedCached = await getSharedCachedSearch(args, queryClassification.queryClass, queryVariants); if (sharedCached?.kind === "hit") { await setCachedSearch(args, sharedCached.results, sharedCached.telemetry, queryVariants); return { results: sharedCached.results, telemetry: sharedCached.telemetry }; diff --git a/tests/cache-metrics.test.ts b/tests/cache-metrics.test.ts index 0891eb14e..a6848b15a 100644 --- a/tests/cache-metrics.test.ts +++ b/tests/cache-metrics.test.ts @@ -1,6 +1,11 @@ import { afterEach, describe, expect, it } from "vitest"; -import { cacheMetricsSnapshot, recordCacheLookup, resetCacheMetrics } from "@/lib/observability/cache-metrics"; +import { + cacheMetricsSnapshot, + classifySearchCacheOutcome, + recordCacheLookup, + resetCacheMetrics, +} from "@/lib/observability/cache-metrics"; afterEach(() => { resetCacheMetrics(); @@ -31,3 +36,25 @@ describe("cache metrics counter", () => { expect(cacheMetricsSnapshot()).toEqual({ lookups: 0, hits: 0, misses: 0, hitRate: 0 }); }); }); + +describe("classifySearchCacheOutcome", () => { + it("counts a process-local cache hit as a hit", () => { + expect(classifySearchCacheOutcome(true, null)).toBe("hit"); + expect(classifySearchCacheOutcome(true, { kind: "miss" })).toBe("hit"); + }); + + it("counts a shared-cache hit after a local miss as a hit (not a miss)", () => { + // Regression: a cold process serving fully from the shared cache must not + // deflate the hit-rate into false-degradation territory. + expect(classifySearchCacheOutcome(false, { kind: "hit" })).toBe("hit"); + }); + + it("counts a miss only when both layers were consulted and missed", () => { + expect(classifySearchCacheOutcome(false, { kind: "miss" })).toBe("miss"); + }); + + it("skips (records neither) when caching is disabled and the shared lookup returns null", () => { + expect(classifySearchCacheOutcome(false, null)).toBe("skip"); + expect(classifySearchCacheOutcome(false, undefined)).toBe("skip"); + }); +}); From 81cd518079f0500afd0a008f28c7d202ed3b826e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 12 Jul 2026 19:25:49 +0000 Subject: [PATCH 3/5] fix: format health/ready route.ts with Prettier --- src/app/api/health/ready/route.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/app/api/health/ready/route.ts b/src/app/api/health/ready/route.ts index 561653008..9e7c6a463 100644 --- a/src/app/api/health/ready/route.ts +++ b/src/app/api/health/ready/route.ts @@ -6,5 +6,10 @@ export const dynamic = "force-dynamic"; // Railway cannot attach the authenticated deep-probe header. This endpoint is // intentionally limited to readiness state and exposes no diagnostic details. export async function GET(request: Request) { - return healthResponse(request, { forceDeep: true, allowUnauthenticatedDeep: true, includeSlo: false, includeCache: false }); + return healthResponse(request, { + forceDeep: true, + allowUnauthenticatedDeep: true, + includeSlo: false, + includeCache: false, + }); } From 8d5b6ebbcc8bb48e47155d6a0997e985737b23e8 Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Mon, 13 Jul 2026 03:27:28 +0800 Subject: [PATCH 4/5] harden(observability): gate /api/health cache counter on token auth; format ready route Builds on the bot's main-integration (cache metrics in the shared healthResponse helper). Adds defense-in-depth so the operator-gated cache counter cannot leak: it is exposed only to a genuinely token-authorized deep probe (not merely `includeCache`), so a future route passing `allowUnauthenticatedDeep` without `includeCache:false` still cannot expose it. Adds a /api/health/ready guard test asserting no slo/cache leak even to a token-bearing caller, plus a doc note. Also reflows the ready route to satisfy prettier (the bot's one-liner exceeded print width and would have failed format:check). Co-Authored-By: Claude Opus 4.8 --- docs/observability-slos.md | 7 +++++-- src/app/api/health/ready/route.ts | 7 ++++++- src/lib/health-response.ts | 13 +++++++++---- tests/health-route.test.ts | 13 +++++++++++++ 4 files changed, 33 insertions(+), 7 deletions(-) diff --git a/docs/observability-slos.md b/docs/observability-slos.md index bd6dbfbea..6e156cc04 100644 --- a/docs/observability-slos.md +++ b/docs/observability-slos.md @@ -206,8 +206,11 @@ a Supabase-side RPC fault or a cold cache must not trip the container `HEALTHCHECK` into a restart it cannot fix. Degradation is made _visible_, not _fatal_ — exactly the failure mode this doc guards against. The `slo` query throwing degrades to an absent `slo` block (not a false-healthy zero); the cache -counter is always present for an authorized probe (in-process, works in demo -mode). The §2 warn/page thresholds map directly onto these fields. +counter is always present for a token-authorized probe (in-process, works in demo +mode). Both blocks are wired through `src/lib/health-response.ts` and are +**withheld from the unauthenticated `/api/health/ready` endpoint** (Railway's +readiness target, which exposes no diagnostic details). The §2 warn/page +thresholds map directly onto these fields. ## 5. Gaps / next steps diff --git a/src/app/api/health/ready/route.ts b/src/app/api/health/ready/route.ts index 561653008..9e7c6a463 100644 --- a/src/app/api/health/ready/route.ts +++ b/src/app/api/health/ready/route.ts @@ -6,5 +6,10 @@ export const dynamic = "force-dynamic"; // Railway cannot attach the authenticated deep-probe header. This endpoint is // intentionally limited to readiness state and exposes no diagnostic details. export async function GET(request: Request) { - return healthResponse(request, { forceDeep: true, allowUnauthenticatedDeep: true, includeSlo: false, includeCache: false }); + return healthResponse(request, { + forceDeep: true, + allowUnauthenticatedDeep: true, + includeSlo: false, + includeCache: false, + }); } diff --git a/src/lib/health-response.ts b/src/lib/health-response.ts index abab23116..628f1773a 100644 --- a/src/lib/health-response.ts +++ b/src/lib/health-response.ts @@ -22,12 +22,17 @@ export async function healthResponse(request: Request, options: HealthResponseOp let cache: CacheMetricsSnapshot | null = null; if (deep) { - if (!options.allowUnauthenticatedDeep && !allowDeepHealthProbe(request)) { + const tokenAuthorized = allowDeepHealthProbe(request); + if (!options.allowUnauthenticatedDeep && !tokenAuthorized) { checks.supabase = "unauthorized"; } else { - // In-process retrieval cache hit-rate — available to any authorized deep - // probe, including in demo mode (reads a cumulative counter, no DB needed). - if (options.includeCache !== false) { + // Cache hit-rate is operator-gated internal telemetry (the in-process + // hot-path half of the silent-degradation counters). Expose it only to a + // genuinely token-authorized deep probe — never the unauthenticated + // readiness endpoint, which exposes no diagnostic details — and only when + // not explicitly suppressed. Reads a cumulative counter (no DB), so it is + // available even in demo mode. Like `slo`, it never flips liveness. + if (tokenAuthorized && options.includeCache !== false) { cache = cacheMetricsSnapshot(); } diff --git a/tests/health-route.test.ts b/tests/health-route.test.ts index a748b77bb..45a1de270 100644 --- a/tests/health-route.test.ts +++ b/tests/health-route.test.ts @@ -111,4 +111,17 @@ describe("GET /api/health/ready", () => { expect(response.status).toBe(200); expect(body.checks).toMatchObject({ supabaseConfig: "ok", openaiConfig: "ok", supabase: "skipped" }); }); + + it("exposes no diagnostic details (slo/cache) even to a token-bearing caller", async () => { + mockEnv({ configured: true, demoMode: true, deepSecret: true }); + const { GET } = await import("../src/app/api/health/ready/route"); + + const response = await GET( + new Request("http://localhost/api/health/ready", { headers: { "x-health-deep-token": DEEP_TOKEN } }), + ); + const body = await payload(response); + + expect(body.slo).toBeUndefined(); + expect(body.cache).toBeUndefined(); + }); }); From d556cd8402b4911c706b038da3ab3e37823e606c Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Mon, 13 Jul 2026 03:36:46 +0800 Subject: [PATCH 5/5] fix(observability): skip cache counter when search cache is size/TTL-disabled; clarify liveness doc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses two PR #536 review findings: - Codex P2 (rag.ts): getSharedCachedSearch only short-circuits on skipCache/TTL, not on RAG_SEARCH_CACHE_SIZE, so a size-0 deployment (local cache off, default TTL) recorded disabled lookups as misses — a false 0% hit-rate. Gate the counter on a shared isSearchCacheEnabled() predicate (skipCache / TTL / size), threaded through classifySearchCacheOutcome(cacheEnabled, …). Added regression tests for the disabled-but-shared-ran case. - CodeRabbit (docs): the "neither block flips liveness" wording implied a Supabase probe failure stays 200. Corrected to distinguish counter *values* (never flip liveness) from probeSupabaseHealth() failures (set checks.supabase = error → 503) and answerSloSnapshot() failures (omit slo only). Co-Authored-By: Claude Opus 4.8 --- docs/observability-slos.md | 15 ++++++++------- src/lib/observability/cache-metrics.ts | 12 ++++++++---- src/lib/rag-cache.ts | 10 +++++++++- src/lib/rag.ts | 3 ++- tests/cache-metrics.test.ts | 23 ++++++++++++++++------- 5 files changed, 43 insertions(+), 20 deletions(-) diff --git a/docs/observability-slos.md b/docs/observability-slos.md index 6e156cc04..925dac59e 100644 --- a/docs/observability-slos.md +++ b/docs/observability-slos.md @@ -201,13 +201,14 @@ header (same operator gate as the Supabase probe) — returns two counter blocks "cache": { "lookups": 1873, "hits": 1402, "misses": 471, "hitRate": 0.748 } ``` -**Neither block flips liveness.** The probe stays `200` even when a rate is bad; -a Supabase-side RPC fault or a cold cache must not trip the container -`HEALTHCHECK` into a restart it cannot fix. Degradation is made _visible_, not -_fatal_ — exactly the failure mode this doc guards against. The `slo` query -throwing degrades to an absent `slo` block (not a false-healthy zero); the cache -counter is always present for a token-authorized probe (in-process, works in demo -mode). Both blocks are wired through `src/lib/health-response.ts` and are +**The counter _values_ never flip liveness.** A bad SLO rate or a cold cache +stays visible without changing readiness — degradation is made _visible_, not +_fatal_, which is the failure mode this doc guards against. Liveness is driven +only by the reachability `checks`: a failing `probeSupabaseHealth()` still sets +`checks.supabase = "error"` and returns `503`, whereas an `answerSloSnapshot()` +query failure merely omits the `slo` block (never a false-healthy zero). The +cache counter is always present for a token-authorized probe (in-process, works +in demo mode). Both blocks are wired through `src/lib/health-response.ts` and are **withheld from the unauthenticated `/api/health/ready` endpoint** (Railway's readiness target, which exposes no diagnostic details). The §2 warn/page thresholds map directly onto these fields. diff --git a/src/lib/observability/cache-metrics.ts b/src/lib/observability/cache-metrics.ts index df49316cd..bc8a28371 100644 --- a/src/lib/observability/cache-metrics.ts +++ b/src/lib/observability/cache-metrics.ts @@ -45,16 +45,20 @@ export function resetCacheMetrics(): void { * Classify the outcome of the two-layer search-cache lookup so the counter * reflects real cache effectiveness. Retrieval consults the process-local cache * first and the shared (`rag_response_cache`) cache second; a request served by - * *either* layer is a hit. A miss is only counted when a layer was actually - * consulted and returned nothing — when caching is disabled/skipped the shared - * lookup returns `null` and the request is `"skip"` (recorded as neither), so a - * cold process with a warm shared cache does not read as false degradation. + * *either* layer is a hit, so a cold process falling through to a warm shared + * cache is not miscounted as a miss. When the search cache is disabled for this + * request (`cacheEnabled` false — skipCache, or TTL/size 0) the lookup is + * `"skip"` and recorded as neither; this is gated up front because the shared + * lookup only short-circuits on skipCache/TTL, so a size-0 deployment would + * otherwise record disabled lookups as misses and read as false degradation. * Pure, so the orchestration stays trivial and this stays unit-tested. */ export function classifySearchCacheOutcome( + cacheEnabled: boolean, localHit: boolean, sharedResult: { kind: "hit" | "miss" } | null | undefined, ): "hit" | "miss" | "skip" { + if (!cacheEnabled) return "skip"; if (localHit || sharedResult?.kind === "hit") return "hit"; if (sharedResult?.kind === "miss") return "miss"; return "skip"; diff --git a/src/lib/rag-cache.ts b/src/lib/rag-cache.ts index b9fce6b59..6b70349d2 100644 --- a/src/lib/rag-cache.ts +++ b/src/lib/rag-cache.ts @@ -179,12 +179,20 @@ function normalizeCacheStorageTelemetry(telemetry: SearchTelemetry): SearchTelem }; } +// Single source of truth for whether the process-local search cache is active +// for a request. Shared with the observability counter so a size-0 (or TTL-0 / +// skipCache) deployment records the lookup as neither hit nor miss rather than a +// false miss (the shared-cache lookup does not itself short-circuit on size). +export function isSearchCacheEnabled(args: Pick): boolean { + return !args.skipCache && env.RAG_SEARCH_CACHE_TTL_MS > 0 && env.RAG_SEARCH_CACHE_SIZE > 0; +} + export async function getCachedSearch( args: SearchChunksArgs, queryClass?: RagQueryClass, queryVariants: string[] = [], ): Promise<{ results: SearchResult[]; telemetry: SearchTelemetry } | null> { - if (args.skipCache || env.RAG_SEARCH_CACHE_TTL_MS <= 0 || env.RAG_SEARCH_CACHE_SIZE <= 0) return null; + if (!isSearchCacheEnabled(args)) return null; const key = scopedSearchCacheKey(args, queryClass, queryVariants); const cached = searchCache.get(key); diff --git a/src/lib/rag.ts b/src/lib/rag.ts index b37a9ed9d..bc0cf5cb1 100644 --- a/src/lib/rag.ts +++ b/src/lib/rag.ts @@ -73,6 +73,7 @@ import { getCachedSearch, getSharedCachedAnswer, getSharedCachedSearch, + isSearchCacheEnabled, packAdjacentSourceContext, packedContextCacheKey, scopedAnswerCacheKey, @@ -3022,7 +3023,7 @@ export async function searchChunksWithTelemetry(args: SearchChunksArgs) { // cold process that falls through to a warm shared cache is not miscounted as a // miss (deep /api/health cache hit-rate — docs/observability-slos.md §4). const sharedCached = cached ? null : await getSharedCachedSearch(args, queryClassification.queryClass, queryVariants); - const cacheOutcome = classifySearchCacheOutcome(Boolean(cached), sharedCached); + const cacheOutcome = classifySearchCacheOutcome(isSearchCacheEnabled(args), Boolean(cached), sharedCached); if (cacheOutcome !== "skip") recordCacheLookup(cacheOutcome === "hit"); if (cached) return cached; diff --git a/tests/cache-metrics.test.ts b/tests/cache-metrics.test.ts index a6848b15a..b782285da 100644 --- a/tests/cache-metrics.test.ts +++ b/tests/cache-metrics.test.ts @@ -39,22 +39,31 @@ describe("cache metrics counter", () => { describe("classifySearchCacheOutcome", () => { it("counts a process-local cache hit as a hit", () => { - expect(classifySearchCacheOutcome(true, null)).toBe("hit"); - expect(classifySearchCacheOutcome(true, { kind: "miss" })).toBe("hit"); + expect(classifySearchCacheOutcome(true, true, null)).toBe("hit"); + expect(classifySearchCacheOutcome(true, true, { kind: "miss" })).toBe("hit"); }); it("counts a shared-cache hit after a local miss as a hit (not a miss)", () => { // Regression: a cold process serving fully from the shared cache must not // deflate the hit-rate into false-degradation territory. - expect(classifySearchCacheOutcome(false, { kind: "hit" })).toBe("hit"); + expect(classifySearchCacheOutcome(true, false, { kind: "hit" })).toBe("hit"); }); it("counts a miss only when both layers were consulted and missed", () => { - expect(classifySearchCacheOutcome(false, { kind: "miss" })).toBe("miss"); + expect(classifySearchCacheOutcome(true, false, { kind: "miss" })).toBe("miss"); }); - it("skips (records neither) when caching is disabled and the shared lookup returns null", () => { - expect(classifySearchCacheOutcome(false, null)).toBe("skip"); - expect(classifySearchCacheOutcome(false, undefined)).toBe("skip"); + it("skips the disabled search cache even when the shared lookup still ran", () => { + // Regression: getSharedCachedSearch does not short-circuit on size, so a + // size-0 (or TTL-0 / skipCache) deployment would otherwise record disabled + // lookups as false misses and report a 0% hit-rate. + expect(classifySearchCacheOutcome(false, false, { kind: "miss" })).toBe("skip"); + expect(classifySearchCacheOutcome(false, false, { kind: "hit" })).toBe("skip"); + expect(classifySearchCacheOutcome(false, false, null)).toBe("skip"); + }); + + it("skips when enabled but no cache layer was consulted", () => { + expect(classifySearchCacheOutcome(true, false, null)).toBe("skip"); + expect(classifySearchCacheOutcome(true, false, undefined)).toBe("skip"); }); });