diff --git a/migrations/0131_impact_map_query_cache.sql b/migrations/0131_impact_map_query_cache.sql new file mode 100644 index 0000000000..ad676d6c5d --- /dev/null +++ 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 index a2f688c90a..ded7a8a9bf 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -1471,3 +1471,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 index 0bd994b355..faac53f02b 100644 --- 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 { + 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 { + 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 { + 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 index d437f5de10..9445df9e0a 100644 --- 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 } { + const rows = new Map(); + 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); + }); });