fix(review): cache impact-map's per-file vector query results (#4544)

JSONbored · web-flow · commit dfef43806c88 · 2026-07-09T18:28:21.000-07:00
* fix(review): cache impact-map's per-file vector query results computeImpactMap issues one retrieveContextWithMetrics call per changed- symbol file (up to MAX_IMPACT_MAP_INPUT_FILES=20), each a real embedding call plus a live vector-index query with no result cache -- only a 60-second cold-index existence check is memoized. Impact-map is a dynamic feature (bypasses the durable ai_review cache), so only the 30-minute non-cacheable retry cooldown throttled repeat computation; once that lapsed, the full up-to-20x embed+query loop re-fired for an unchanged PR. Adds impact_map_query_cache, keyed by (project, repo, fingerprint) where the fingerprint hashes every input that affects the result (queryText, excludePaths, topK, minScore, reranker) -- excludePaths in particular varies per changed file, since each excludes itself. Unlike the grounding file-content cache, this needs a TTL: the underlying vector index can change as new commits get embedded, so an identical query issued later could legitimately have a different correct answer. Matches the same 30-minute cooldown that already throttles how often this computation is re-attempted. Closes #4500. * fix(review): close impact-map query-cache coverage gaps Restructure the cache-hit branch to a proper if/else (a braceless single-statement if with no else was mis-instrumented by v8's coverage collector) and add a fail-safe test for a throwing cache read.
diff --git a/migrations/0131_impact_map_query_cache.sql b/migrations/0131_impact_map_query_cache.sql
@@ -0,0 +1,17 @@
+-- Impact-map query cache (#4500): computeImpactMap issues one retrieveContextWithMetrics call per
+-- changed-symbol file (up to MAX_IMPACT_MAP_INPUT_FILES=20), and each call does a REAL embedding-model
+-- inference call plus a live vector-index query with no result cache -- only a 60-second cold-index
+-- existence check is memoized (rag.ts's chunkCountCache), not query results. Unlike grounding_file_content_cache
+-- (migration 0130), this DOES need a TTL: the underlying vector index can change as new commits get embedded,
+-- so an identical query issued later could legitimately have a different correct answer. query_fingerprint
+-- hashes every input that affects the result (queryText, excludePaths, topK, minScore, reranker) since all of
+-- them vary meaningfully -- excludePaths in particular varies per changed file (each excludes itself).
+CREATE TABLE IF NOT EXISTS impact_map_query_cache (
+  project TEXT NOT NULL,
+  repo TEXT NOT NULL,
+  query_fingerprint TEXT NOT NULL,
+  context TEXT NOT NULL,
+  metrics_json TEXT NOT NULL,
+  fetched_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP,
+  PRIMARY KEY (project, repo, query_fingerprint)
+);
diff --git a/src/db/schema.ts b/src/db/schema.ts
@@ -1476,3 +1476,28 @@ export const groundingFileContentCache = sqliteTable(
     primary: primaryKey({ columns: [table.repoFullName, table.path, table.headSha] }),
   }),
 );
+
+// Impact-map query cache (#4500): computeImpactMap issues one retrieveContextWithMetrics call per
+// changed-symbol file with no result cache -- only a 60-second cold-index existence check is memoized. Unlike
+// linkedIssueSatisfactionCache, this DOES need a TTL (checked at the read site, not a schema constraint): the
+// underlying vector index can change as new commits get embedded, so an identical query issued later could
+// legitimately have a different correct answer.
+export const impactMapQueryCache = sqliteTable(
+  "impact_map_query_cache",
+  {
+    project: text("project").notNull(),
+    repo: text("repo").notNull(),
+    // Hashes every input that affects the result (queryText, excludePaths, topK, minScore, reranker) -- all of
+    // them vary meaningfully; excludePaths in particular varies per changed file (each excludes itself).
+    queryFingerprint: text("query_fingerprint").notNull(),
+    context: text("context").notNull(),
+    metricsJson: text("metrics_json").notNull(),
+    /* v8 ignore next -- this default only fires for a Drizzle query-builder insert omitting fetchedAt;
+     * putCachedImpactMapQuery always writes via raw SQL with an explicit fetched_at value, so this callback
+     * is never actually invoked by the real code path (defensive schema-level default only). */
+    fetchedAt: text("fetched_at").notNull().$defaultFn(() => nowIso()),
+  },
+  (table) => ({
+    primary: primaryKey({ columns: [table.project, table.repo, table.queryFingerprint] }),
+  }),
+);
diff --git a/src/review/impact-map.ts b/src/review/impact-map.ts
@@ -9,8 +9,10 @@
 // FAIL-SAFE (mirrors rag.ts's own guarantee): a missing/cold RAG index, no changed symbols, or any retrieval
 // error degrades to an EMPTY impact map — this computation can never break or block a review.
 
+import { sha256Hex } from "../utils/crypto";
+import { nowIso } from "../utils/json";
 import type { FileChangedSymbols } from "./impact-symbols";
-import { retrieveContextWithMetrics, type RagInfra } from "./rag";
+import { retrieveContextWithMetrics, type RagInfra, type RagRetrievalResult } from "./rag";
 
 export type ImpactMapEntry = {
   /** The file whose changed exported symbol(s) triggered this entry. */
@@ -53,6 +55,71 @@ function buildSymbolQueryText(file: FileChangedSymbols): string {
   return `Changed symbols: ${file.symbols.join(", ")}\nFile: ${file.path}`;
 }
 
+// #4500: a query-result cache, distinct from grounding_file_content_cache -- the underlying vector index can
+// change as new commits get embedded, so (unlike file content at an immutable head SHA) an identical query
+// issued later could legitimately have a different correct answer. Matches
+// AI_REVIEW_NON_CACHEABLE_RETRY_COOLDOWN_MS (processors.ts), the SAME cooldown that throttles how often this
+// whole computation is even re-attempted -- a cache TTL any shorter would never actually prevent a redundant
+// re-embed within that window, and any longer would risk masking a real index update for no added benefit.
+const IMPACT_MAP_QUERY_CACHE_MAX_AGE_MS = 30 * 60 * 1000;
+
+/** One query's cache key: every input that affects retrieveContextWithMetrics' result. topK/minScore/reranker
+ *  are constants for this module's own calls, but are still hashed (not assumed) so this function stays
+ *  correct if a future caller ever varies them. excludePaths is sorted before hashing so argument order never
+ *  causes a spurious cache miss. */
+async function impactMapQueryFingerprint(input: {
+  queryText: string;
+  excludePaths: string[];
+  topK: number;
+  minScore: number;
+  reranker: string;
+}): Promise<string> {
+  const payload = [input.queryText, [...input.excludePaths].sort().join(","), String(input.topK), String(input.minScore), input.reranker].join("|");
+  return sha256Hex(payload);
+}
+
+async function getCachedImpactMapQuery(
+  storage: RagInfra["storage"],
+  project: string,
+  repo: string,
+  fingerprint: string,
+): Promise<RagRetrievalResult | null> {
+  try {
+    const row = await storage
+      .prepare("SELECT context, metrics_json AS metricsJson, fetched_at AS fetchedAt FROM impact_map_query_cache WHERE project = ? AND repo = ? AND query_fingerprint = ?")
+      .bind(project, repo, fingerprint)
+      .first<{ context: string; metricsJson: string; fetchedAt: string }>();
+    if (!row) return null;
+    const ageMs = Date.now() - Date.parse(row.fetchedAt);
+    if (!Number.isFinite(ageMs) || ageMs >= IMPACT_MAP_QUERY_CACHE_MAX_AGE_MS) return null;
+    return { context: row.context, metrics: JSON.parse(row.metricsJson) as RagRetrievalResult["metrics"] };
+  } catch {
+    return null; // fail-safe: a storage error degrades to "no cache", never blocks the query
+  }
+}
+
+async function putCachedImpactMapQuery(
+  storage: RagInfra["storage"],
+  project: string,
+  repo: string,
+  fingerprint: string,
+  result: RagRetrievalResult,
+): Promise<void> {
+  try {
+    await storage
+      .prepare(
+        `INSERT INTO impact_map_query_cache (project, repo, query_fingerprint, context, metrics_json, fetched_at)
+         VALUES (?, ?, ?, ?, ?, ?)
+         ON CONFLICT(project, repo, query_fingerprint) DO UPDATE SET
+           context = excluded.context, metrics_json = excluded.metrics_json, fetched_at = excluded.fetched_at`,
+      )
+      .bind(project, repo, fingerprint, result.context, JSON.stringify(result.metrics), nowIso())
+      .run();
+  } catch {
+    // fail-safe: a write failure only means this ONE result isn't cached -- never blocks the review
+  }
+}
+
 /**
  * Compute the deterministic impact map for a PR's changed symbols. One entry per changed file that has at
  * least one extracted symbol (files with none contribute no entry — there's nothing symbol-driven to query
@@ -71,17 +138,29 @@ export async function computeImpactMap(
   const queryableFiles = symbols.filter((file) => file.symbols.length > 0).slice(0, MAX_IMPACT_MAP_INPUT_FILES);
   for (const file of queryableFiles) {
     const queryText = buildSymbolQueryText(file);
+    const excludePaths = [file.path];
     let affectedModules: string[];
     try {
-      const result = await retrieveContextWithMetrics(ragContext.infra, {
-        project: ragContext.project,
-        repo: ragContext.repo,
-        queryText,
-        topK: IMPACT_MAP_TOP_K,
-        minScore: IMPACT_MAP_MIN_SCORE,
-        excludePaths: [file.path],
-        reranker: "bm25",
-      });
+      // #4500: reuse a still-fresh prior result for the IDENTICAL query instead of re-embedding + re-querying
+      // the vector index -- a real cost this loop pays up to MAX_IMPACT_MAP_INPUT_FILES times per pass, with
+      // nothing else memoizing it (impact-map is a dynamic feature that bypasses the durable ai_review cache).
+      const fingerprint = await impactMapQueryFingerprint({ queryText, excludePaths, topK: IMPACT_MAP_TOP_K, minScore: IMPACT_MAP_MIN_SCORE, reranker: "bm25" });
+      const cached = await getCachedImpactMapQuery(ragContext.infra.storage, ragContext.project, ragContext.repo, fingerprint);
+      let result: RagRetrievalResult;
+      if (cached !== null) {
+        result = cached;
+      } else {
+        result = await retrieveContextWithMetrics(ragContext.infra, {
+          project: ragContext.project,
+          repo: ragContext.repo,
+          queryText,
+          topK: IMPACT_MAP_TOP_K,
+          minScore: IMPACT_MAP_MIN_SCORE,
+          excludePaths,
+          reranker: "bm25",
+        });
+        await putCachedImpactMapQuery(ragContext.infra.storage, ragContext.project, ragContext.repo, fingerprint, result);
+      }
       affectedModules = result.metrics.paths.slice(0, MAX_AFFECTED_MODULES_PER_ENTRY);
       // Defense in depth: retrieveContextWithMetrics is itself fail-safe (its own try/catch degrades a
       // throwing vector/inference adapter to an empty result internally — never throws out to us), but this
diff --git a/test/unit/impact-map.test.ts b/test/unit/impact-map.test.ts
@@ -30,6 +30,38 @@ function storageStubWithText(count: number): StorageAdapter {
   } as unknown as StorageAdapter;
 }
 
+/** A storage stub that actually backs impact_map_query_cache's INSERT/SELECT/ON CONFLICT semantics in an
+ *  in-memory Map (keyed by "project|repo|fingerprint"), while still answering repo_chunks' COUNT/chunk-text
+ *  queries like storageStubWithText above -- lets the invariant/regression tests below assert genuine cache
+ *  hit/miss/expiry behavior instead of the other stubs' fixed canned responses (which always read as a miss). */
+function cachingStorageStub(count: number, fetchedAtOverride?: string): { storage: StorageAdapter; rows: Map<string, { context: string; metricsJson: string; fetchedAt: string }> } {
+  const rows = new Map<string, { context: string; metricsJson: string; fetchedAt: string }>();
+  const storage: StorageAdapter = {
+    prepare: (sql: string) => ({
+      bind: (...args: unknown[]) => ({
+        first: async () => {
+          if (/FROM impact_map_query_cache/i.test(sql)) {
+            const [project, repo, fingerprint] = args as [string, string, string];
+            return rows.get(`${project}|${repo}|${fingerprint}`) ?? null;
+          }
+          return { n: count };
+        },
+        all: async () =>
+          /SELECT id, text/i.test(sql) ? { results: args.map((id) => ({ id: String(id), text: `body for ${String(id)}` })) } : { results: [] },
+        run: async () => {
+          if (/INSERT INTO impact_map_query_cache/i.test(sql)) {
+            const [project, repo, fingerprint, context, metricsJson, fetchedAt] = args as [string, string, string, string, string, string];
+            rows.set(`${project}|${repo}|${fingerprint}`, { context, metricsJson, fetchedAt: fetchedAtOverride ?? fetchedAt });
+          }
+          return undefined;
+        },
+      }),
+    }),
+    batch: async () => undefined,
+  } as unknown as StorageAdapter;
+  return { storage, rows };
+}
+
 function vectorStub(matches: Array<{ id: string; score: number; metadata: { path: string } }>): VectorAdapter {
   return {
     query: async () => ({ matches }),
@@ -215,4 +247,132 @@ describe("computeImpactMap", () => {
       { changedModule: "src/review/impact-map.ts", affectedModules: ["src/review/y.ts"], callers: ["computeImpactMap"] },
     ]);
   });
+
+  it("INVARIANT (#4500): a second computeImpactMap call with the IDENTICAL changed-symbol set makes ZERO additional embed/vector-query calls", async () => {
+    let embedCalls = 0;
+    let queryCalls = 0;
+    const countingInference: InferenceAdapter = {
+      run: async () => {
+        embedCalls += 1;
+        return { data: [Array(1024).fill(0.1)] };
+      },
+    };
+    const countingVector: VectorAdapter = {
+      query: async () => {
+        queryCalls += 1;
+        return { matches: [{ id: "src/review/caller.ts::0", score: 0.9, metadata: { path: "src/review/caller.ts" } }] };
+      },
+      upsert: async () => undefined,
+      deleteByIds: async () => undefined,
+    } as unknown as VectorAdapter;
+    const { storage } = cachingStorageStub(5);
+    const infra: RagInfra = { storage, vector: countingVector, inference: countingInference };
+    const symbols: FileChangedSymbols[] = [{ path: "src/review/impact-map.ts", symbols: ["computeImpactMap"] }];
+
+    const first = await computeImpactMap(symbols, { infra, project: "acme", repo: "widgets" });
+    const second = await computeImpactMap(symbols, { infra, project: "acme", repo: "widgets" });
+
+    expect(first).toEqual(second);
+    expect(embedCalls).toBe(1);
+    expect(queryCalls).toBe(1);
+  });
+
+  it("REGRESSION (#4500, impact-map-refetch incident): repeated cooldown-driven computeImpactMap calls on an unchanged head only embed/query once per file, not once per call", async () => {
+    let embedCalls = 0;
+    let queryCalls = 0;
+    const countingInference: InferenceAdapter = {
+      run: async () => {
+        embedCalls += 1;
+        return { data: [Array(1024).fill(0.1)] };
+      },
+    };
+    const countingVector: VectorAdapter = {
+      query: async () => {
+        queryCalls += 1;
+        return { matches: [{ id: "src/review/caller.ts::0", score: 0.9, metadata: { path: "src/review/caller.ts" } }] };
+      },
+      upsert: async () => undefined,
+      deleteByIds: async () => undefined,
+    } as unknown as VectorAdapter;
+    const { storage } = cachingStorageStub(5);
+    const infra: RagInfra = { storage, vector: countingVector, inference: countingInference };
+    const symbols: FileChangedSymbols[] = [{ path: "src/review/impact-map.ts", symbols: ["computeImpactMap"] }];
+
+    // Simulates 5 separate scheduled-sweep-tick passes for the SAME unchanged PR head past the 30-minute
+    // non-cacheable cooldown -- previously each one re-embedded and re-queried the vector index from scratch.
+    for (let i = 0; i < 5; i += 1) {
+      // eslint-disable-next-line no-await-in-loop -- sequential passes, mirroring separate review invocations
+      await computeImpactMap(symbols, { infra, project: "acme", repo: "widgets" });
+    }
+
+    expect(embedCalls).toBe(1);
+    expect(queryCalls).toBe(1);
+  });
+
+  it("a throwing cache READ degrades to a fresh embed+query (fail-safe, never blocks the impact-map computation)", async () => {
+    const throwingStorage: StorageAdapter = {
+      prepare: () => ({
+        bind: () => ({
+          first: async () => {
+            throw new Error("cache read boom");
+          },
+          all: async () => ({ results: [] }),
+          run: async () => undefined,
+        }),
+      }),
+      batch: async () => undefined,
+    } as unknown as StorageAdapter;
+    const infra: RagInfra = {
+      storage: throwingStorage,
+      vector: vectorStub([{ id: "src/review/caller.ts::0", score: 0.9, metadata: { path: "src/review/caller.ts" } }]),
+      inference: ai1024,
+    };
+    const symbols: FileChangedSymbols[] = [{ path: "src/review/impact-map.ts", symbols: ["computeImpactMap"] }];
+    // The chunk-text lookup (repo_chunks) ALSO throws via this stub, which retrieveContextWithMetrics itself
+    // already degrades fail-safe -- the cache-read throw specifically is what this test targets, so the
+    // resulting entry is empty (no chunk text survives), but the computation must complete, not throw.
+    await expect(computeImpactMap(symbols, { infra, project: "acme", repo: "widgets" })).resolves.toEqual([]);
+  });
+
+  it("a genuinely different query (different changed symbols) still triggers a fresh embed+query, never masked by another file's cached entry", async () => {
+    let queryCalls = 0;
+    const countingVector: VectorAdapter = {
+      query: async () => {
+        queryCalls += 1;
+        return { matches: [{ id: "src/review/caller.ts::0", score: 0.9, metadata: { path: "src/review/caller.ts" } }] };
+      },
+      upsert: async () => undefined,
+      deleteByIds: async () => undefined,
+    } as unknown as VectorAdapter;
+    const { storage } = cachingStorageStub(5);
+    const infra: RagInfra = { storage, vector: countingVector, inference: ai1024 };
+
+    await computeImpactMap([{ path: "src/review/impact-map.ts", symbols: ["computeImpactMap"] }], { infra, project: "acme", repo: "widgets" });
+    // A different changed file with different symbols -- a genuinely different query, even though it shares
+    // the same project/repo as the first call.
+    await computeImpactMap([{ path: "src/review/rag.ts", symbols: ["retrieveContextWithMetrics"] }], { infra, project: "acme", repo: "widgets" });
+
+    expect(queryCalls).toBe(2);
+  });
+
+  it("a cache entry older than the TTL is treated as a miss, re-embedding and re-querying instead of serving a possibly-stale answer", async () => {
+    let queryCalls = 0;
+    const countingVector: VectorAdapter = {
+      query: async () => {
+        queryCalls += 1;
+        return { matches: [{ id: "src/review/caller.ts::0", score: 0.9, metadata: { path: "src/review/caller.ts" } }] };
+      },
+      upsert: async () => undefined,
+      deleteByIds: async () => undefined,
+    } as unknown as VectorAdapter;
+    // Every row this stub returns is stamped an hour old -- past the 30-minute TTL.
+    const { storage } = cachingStorageStub(5, new Date(Date.now() - 60 * 60 * 1000).toISOString());
+    const infra: RagInfra = { storage, vector: countingVector, inference: ai1024 };
+    const symbols: FileChangedSymbols[] = [{ path: "src/review/impact-map.ts", symbols: ["computeImpactMap"] }];
+
+    await computeImpactMap(symbols, { infra, project: "acme", repo: "widgets" });
+    await computeImpactMap(symbols, { infra, project: "acme", repo: "widgets" });
+
+    expect(queryCalls).toBe(2);
+  });
 });
