fix(review): bound impact-map query cache retention

migrations/0145_impact_map_query_cache_repo_fetched_at_idx.sql

@@ -0,0 +1,6 @@
+-- Impact-map query cache retention (#4500 follow-up): computeImpactMap now evicts expired rows on
+-- read-miss and proactively deletes stale rows before every insert, both scoped by (project, repo,
+-- fetched_at). Migration 0132 shipped without this index -- an already-applied migration's SQL is
+-- NOT retroactively re-run, so the index must land as its own migration rather than editing 0132.
+CREATE INDEX IF NOT EXISTS impact_map_query_cache_repo_fetched_at_idx
+  ON impact_map_query_cache (project, repo, fetched_at);

src/db/schema.ts

@@ -1492,5 +1492,6 @@ export const impactMapQueryCache = sqliteTable(
   },
   (table) => ({
     primary: primaryKey({ columns: [table.project, table.repo, table.queryFingerprint] }),
+    repoFetchedAtIdx: index("impact_map_query_cache_repo_fetched_at_idx").on(table.project, table.repo, table.fetchedAt),
   }),
 );

src/review/impact-map.ts

@@ -65,6 +65,10 @@ function buildSymbolQueryText(file: FileChangedSymbols): string {
 // 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;
 
+function impactMapQueryCacheCutoffIso(): string {
+  return new Date(Date.now() - IMPACT_MAP_QUERY_CACHE_MAX_AGE_MS).toISOString();
+}
+
 /** 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
@@ -88,13 +92,16 @@ async function getCachedImpactMapQuery(
 ): 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 = ?")
+      .prepare("SELECT 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 }>();
+      .first<{ 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"] };
+    if (!Number.isFinite(ageMs) || ageMs >= IMPACT_MAP_QUERY_CACHE_MAX_AGE_MS) {
+      await storage.prepare("DELETE FROM impact_map_query_cache WHERE project = ? AND repo = ? AND query_fingerprint = ?").bind(project, repo, fingerprint).run();
+      return null;
+    }
+    return { 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
   }
@@ -108,14 +115,15 @@ async function putCachedImpactMapQuery(
   result: RagRetrievalResult,
 ): Promise<void> {
   try {
+    await storage.prepare("DELETE FROM impact_map_query_cache WHERE project = ? AND repo = ? AND fetched_at < ?").bind(project, repo, impactMapQueryCacheCutoffIso()).run();
     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())
+      .bind(project, repo, fingerprint, "", JSON.stringify(result.metrics), nowIso())
       .run();
   } catch {
     // fail-safe: a write failure only means this ONE result isn't cached -- never blocks the review

test/unit/impact-map.test.ts

@@ -60,6 +60,15 @@ function cachingStorageStub(count: number, fetchedAtOverride?: string): { storag
         all: async () =>
           /SELECT id, text/i.test(sql) ? { results: args.map((id) => ({ id: String(id), text: `body for ${String(id)}` })) } : { results: [] },
         run: async () => {
+          if (/DELETE FROM impact_map_query_cache/i.test(sql)) {
+            const [project, repo, third] = args as [string, string, string];
+            for (const [key, row] of rows) {
+              const [rowProject, rowRepo, rowFingerprint] = key.split("|");
+              const matchesExactRow = /query_fingerprint = \?/i.test(sql) && rowProject === project && rowRepo === repo && rowFingerprint === third;
+              const matchesExpiredRepoRow = /fetched_at < \?/i.test(sql) && rowProject === project && rowRepo === repo && row.fetchedAt < third;
+              if (matchesExactRow || matchesExpiredRepoRow) rows.delete(key);
+            }
+          }
           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 });
@@ -387,6 +396,45 @@ describe("computeImpactMap", () => {
     expect(queryCalls).toBe(2);
   });
 
+  it("REGRESSION: expired impact-map cache rows are evicted instead of accumulating indefinitely", async () => {
+    const staleIso = new Date(Date.now() - 60 * 60 * 1000).toISOString();
+    const freshIso = new Date().toISOString();
+    const { storage, rows } = cachingStorageStub(5);
+    rows.set("acme|widgets|expired-one", { context: "old context", metricsJson: "{}", fetchedAt: staleIso });
+    rows.set("acme|widgets|expired-two", { context: "old context", metricsJson: "{}", fetchedAt: staleIso });
+    rows.set("other|widgets|expired-other-project", { context: "old context", metricsJson: "{}", fetchedAt: staleIso });
+    rows.set("acme|widgets|fresh-one", { context: "fresh context", metricsJson: "{}", fetchedAt: freshIso });
+    const infra: RagInfra = {
+      storage,
+      vector: vectorStub([{ id: "src/review/caller.ts::0", score: 0.9, metadata: { path: "src/review/caller.ts" } }]),
+      inference: ai1024,
+    };
+
+    await computeImpactMap(testEnv(), [{ path: "src/review/impact-map.ts", symbols: ["computeImpactMap"] }], { infra, project: "acme", repo: "widgets" });
+
+    const acmeWidgetRows = [...rows.keys()].filter((key) => key.startsWith("acme|widgets|"));
+    expect(acmeWidgetRows).toHaveLength(2);
+    expect(acmeWidgetRows).toContain("acme|widgets|fresh-one");
+    expect(acmeWidgetRows).not.toContain("acme|widgets|expired-one");
+    expect(acmeWidgetRows).not.toContain("acme|widgets|expired-two");
+    expect(rows.has("other|widgets|expired-other-project")).toBe(true);
+  });
+
+  it("REGRESSION: impact-map cache rows retain only metrics, not the retrieved context body", async () => {
+    const { storage, rows } = cachingStorageStub(5);
+    const infra: RagInfra = {
+      storage,
+      vector: vectorStub([{ id: "src/review/caller.ts::0", score: 0.9, metadata: { path: "src/review/caller.ts" } }]),
+      inference: ai1024,
+    };
+
+    await computeImpactMap(testEnv(), [{ path: "src/review/impact-map.ts", symbols: ["computeImpactMap"] }], { infra, project: "acme", repo: "widgets" });
+
+    expect([...rows.values()]).toHaveLength(1);
+    expect([...rows.values()][0]?.context).toBe("");
+    expect([...rows.values()][0]?.metricsJson).toContain("src/review/caller.ts");
+  });
+
   describe("cache hit/miss telemetry (#4448)", () => {
     afterEach(() => resetMetrics());