diff --git a/migrations/0145_impact_map_query_cache_repo_fetched_at_idx.sql b/migrations/0145_impact_map_query_cache_repo_fetched_at_idx.sql new file mode 100644 index 000000000..05dd67d7b --- /dev/null +++ b/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); diff --git a/src/db/schema.ts b/src/db/schema.ts index 7f5e4f96f..6c7cf0ab4 100644 --- a/src/db/schema.ts +++ b/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), }), ); diff --git a/src/review/impact-map.ts b/src/review/impact-map.ts index 36ca4057d..7781c1fea 100644 --- a/src/review/impact-map.ts +++ b/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 { 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,6 +115,7 @@ async function putCachedImpactMapQuery( result: RagRetrievalResult, ): Promise { 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) @@ -115,7 +123,7 @@ async function putCachedImpactMapQuery( 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 diff --git a/test/unit/impact-map.test.ts b/test/unit/impact-map.test.ts index f0565d455..17055ae02 100644 --- a/test/unit/impact-map.test.ts +++ b/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());