diff --git a/src/queue/processors.ts b/src/queue/processors.ts index cfd1fcad3e..91788c55ca 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -73,7 +73,6 @@ import { getGateBlockOutcome, hasActiveReviewForHeadSha, isDbFrozenForRepo, - listReviewSuppressions, markGateOutcomeOverridden, markPullRequestLinkedIssueHardRuleViolated, startActiveReviewTracking, @@ -503,7 +502,7 @@ import { buildRepoCultureProfileContext, isRepoCultureProfileEnabled, } from "../review/repo-culture-profile-wire"; -import { applyReviewMemorySuppression, shouldApplyReviewMemory } from "../review/review-memory-wire"; +import { applyReviewMemorySuppression, getCachedReviewSuppressions, invalidateReviewSuppressionCache, shouldApplyReviewMemory } from "../review/review-memory-wire"; import { buildReviewEnrichment, isEnrichmentEnabled, @@ -11048,7 +11047,10 @@ async function maybePublishPrPublicSurface( let renderedGate = commentGate; if (reviewMemoryEnabledForReview && commentGate.warnings.length > 0) { try { - const suppressionSignals = await listReviewSuppressions(env, repoFullName); + // #4508: cached (short in-isolate TTL, invalidated on write) — the 3 independent + // maybePublishPrPublicSurface call sites (auto re-review, webhook-triggered review, manual panel + // retrigger) no longer each force a fresh D1 read for the same repo within a short window. + const suppressionSignals = await getCachedReviewSuppressions(env, repoFullName, Date.now()); const { findings: suppressedWarnings, suppressedCount, demotedCount } = applyReviewMemorySuppression( commentGate.warnings, suppressionSignals, @@ -11649,7 +11651,7 @@ async function maybeProcessResolveCommand(env: Env, deliveryId: string, payload: const reviewManifest = await loadRepoFocusManifest(env, req.repoFullName).catch(() => null); const reviewMemoryEnabled = shouldApplyReviewMemory(env, resolveReviewMemoryManifestToggle(reviewManifest)); let recordedSuppressionCount = 0; - if (reviewMemoryEnabled && selection.findings.length > 0) { const { fingerprint } = await import("../review/review-memory-match"); const { recordReviewSuppression } = await import("../db/repositories"); const suppressionWrites = selection.findings.map((finding) => ({ category: finding.code, pathGlob: "", patternHash: fingerprint({ category: finding.code, message: `${finding.title} ${finding.detail}` }) })); await Promise.all(suppressionWrites.map((write) => recordReviewSuppression(env, { repoFullName: req.repoFullName, category: write.category, pathGlob: write.pathGlob, patternHash: write.patternHash, createdBy: req.actor }))); recordedSuppressionCount = suppressionWrites.length; await recordAuditEvent(env, { eventType: "github_app.review_memory_recorded", actor: req.actor, targetKey, outcome: "completed", detail: `Recorded ${recordedSuppressionCount} review-memory suppression signal(s).`, metadata: { deliveryId, repoFullName: req.repoFullName, recordedSuppressionCount, scope: findingRef.scope, ...(findingRef.scope === "single" ? { findingCode: findingRef.findingCode } : {}) } }); await recordGithubProductUsage(env, "review_memory_recorded", { actor: req.actor, repoFullName: req.repoFullName, targetKey, outcome: "completed", metadata: { recordedSuppressionCount, scope: findingRef.scope, ...(findingRef.scope === "single" ? { findingCode: findingRef.findingCode } : {}) } }); } + if (reviewMemoryEnabled && selection.findings.length > 0) { const { fingerprint } = await import("../review/review-memory-match"); const { recordReviewSuppression } = await import("../db/repositories"); const suppressionWrites = selection.findings.map((finding) => ({ category: finding.code, pathGlob: "", patternHash: fingerprint({ category: finding.code, message: `${finding.title} ${finding.detail}` }) })); await Promise.all(suppressionWrites.map((write) => recordReviewSuppression(env, { repoFullName: req.repoFullName, category: write.category, pathGlob: write.pathGlob, patternHash: write.patternHash, createdBy: req.actor }))); recordedSuppressionCount = suppressionWrites.length; invalidateReviewSuppressionCache(req.repoFullName); /* #4508: this repo's cached suppression list is stale as of this write -- the very next render must see it, not wait out the TTL. */ await recordAuditEvent(env, { eventType: "github_app.review_memory_recorded", actor: req.actor, targetKey, outcome: "completed", detail: `Recorded ${recordedSuppressionCount} review-memory suppression signal(s).`, metadata: { deliveryId, repoFullName: req.repoFullName, recordedSuppressionCount, scope: findingRef.scope, ...(findingRef.scope === "single" ? { findingCode: findingRef.findingCode } : {}) } }); await recordGithubProductUsage(env, "review_memory_recorded", { actor: req.actor, repoFullName: req.repoFullName, targetKey, outcome: "completed", metadata: { recordedSuppressionCount, scope: findingRef.scope, ...(findingRef.scope === "single" ? { findingCode: findingRef.findingCode } : {}) } }); } const resolvedLabel = findingRef.scope === "whole_pr" ? "all current advisory findings" : `\`${findingRef.findingCode}\``; const confirmation = sanitizePublicComment([AGENT_COMMAND_COMMENT_MARKER, "", "> [!NOTE]", `> **Review finding resolved by @${req.actor}**`, `> Marked ${resolvedLabel} as resolved for this PR. The Gate check-run is unchanged.`, ...(recordedSuppressionCount > 0 ? ["", `Recorded ${recordedSuppressionCount} review-memory suppression signal(s) for future reviews.`] : []), "", "---", gittensoryFooter()].join("\n")); await createOrUpdateAgentCommandComment(env, req.installationId, req.repoFullName, req.pr.number, confirmation, mode); diff --git a/src/review/review-memory-wire.ts b/src/review/review-memory-wire.ts index ea64b67f5a..c83b9c3217 100644 --- a/src/review/review-memory-wire.ts +++ b/src/review/review-memory-wire.ts @@ -6,6 +6,7 @@ // review path at all (the caller guards on this flag before doing any D1 read or matching), so the review // stays byte-identical to today. +import { listReviewSuppressions } from "../db/repositories"; import { matchSuppressions, type ReviewMemoryFindingInput } from "./review-memory-match"; import type { AdvisoryFinding, ReviewSuppressionRecord } from "../types"; @@ -26,6 +27,42 @@ export function shouldApplyReviewMemory( ): boolean { return isReviewMemoryEnabled(env) && manifestReviewMemoryEnabled; } +// Short in-isolate TTL cache for listReviewSuppressions (#4508), mirroring rag.ts's chunkCountCache: repeated +// unified-comment renders for the same repo within a short window (the 3 independent maybePublishPrPublicSurface +// call sites -- auto re-review, webhook-triggered review, manual panel retrigger -- can each fire this +// independently) reuse the same suppression set instead of re-reading D1 each time. Unlike chunkCountCache's +// "only cache the positive" (cold→hot is one-way), a suppression set can grow at any time via `@gittensory +// resolve`, so this is explicitly invalidated on every write (invalidateReviewSuppressionCache below) rather than +// relying on TTL expiry alone -- a maintainer's fresh suppression must take effect on the very next render, not +// be masked by a stale cached set. +const REVIEW_SUPPRESSION_CACHE_TTL_MS = 60_000; +const reviewSuppressionCache = new Map(); + +/** Cached read of listReviewSuppressions, keyed by repoFullName. `nowMs` is threaded in by the caller (mirrors + * rag.ts's hasIndexedChunks) rather than read internally, so a caller under fake timers gets a deterministic + * cache decision. */ +export async function getCachedReviewSuppressions(env: Env, repoFullName: string, nowMs: number): Promise { + const hit = reviewSuppressionCache.get(repoFullName); + if (hit && nowMs - hit.at < REVIEW_SUPPRESSION_CACHE_TTL_MS) return hit.signals; + const signals = await listReviewSuppressions(env, repoFullName); + reviewSuppressionCache.set(repoFullName, { signals, at: nowMs }); + return signals; +} + +/** Evict repoFullName's cached suppression set immediately. Called after recordReviewSuppression so the very + * next render sees the fresh write, instead of waiting out the TTL. */ +export function invalidateReviewSuppressionCache(repoFullName: string): void { + reviewSuppressionCache.delete(repoFullName); +} + +/** Test-only: clears every cached entry, mirroring clearInstallationTokenCacheForTest/ + * clearGitHubResponseCacheForTest. Without this, a test suite running many cases against the SAME repoFullName + * under fake timers (a fixed `Date.now()` reset per test) would otherwise see one test's cached read leak into + * the next. */ +export function clearReviewSuppressionCacheForTest(): void { + reviewSuppressionCache.clear(); +} + const RESOLVE_FINDING_CODE = /^[a-z][a-z0-9_]{0,199}$/; export function normalizeResolveFindingRef(raw: string | null | undefined): { ok: true; scope: "whole_pr" } | { ok: true; scope: "single"; findingCode: string } | { ok: false; reason: "malformed_finding_id" } { const trimmed = (raw ?? "").trim(); if (trimmed.length === 0) return { ok: true, scope: "whole_pr" }; const normalized = trimmed.toLowerCase().replace(/^finding-/, ""); if (!RESOLVE_FINDING_CODE.test(normalized)) return { ok: false, reason: "malformed_finding_id" }; return { ok: true, scope: "single", findingCode: normalized }; } export function selectWarningsForResolve(warnings: ReadonlyArray, ref: { ok: true; scope: "whole_pr" } | { ok: true; scope: "single"; findingCode: string }): { findings: AdvisoryFinding[]; reason?: "finding_not_found" } { if (ref.scope === "whole_pr") return { findings: [...warnings] }; const matches = warnings.filter((finding) => finding.code === ref.findingCode); if (matches.length === 0) return { findings: [], reason: "finding_not_found" }; return { findings: matches }; } diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index cbb9a341e7..580d1df95a 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -1,6 +1,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { generateKeyPairSync } from "node:crypto"; import { clearInstallationTokenCacheForTest } from "../../src/github/app"; +import { clearReviewSuppressionCacheForTest } from "../../src/review/review-memory-wire"; import { PR_PANEL_COMMENT_MARKER } from "../../src/github/comments"; import * as backfillModule from "../../src/github/backfill"; import * as rateLimitModule from "../../src/github/rate-limit"; @@ -106,6 +107,7 @@ describe("queue processors", () => { // stay deterministic regardless of when CI runs. beforeEach(() => { clearInstallationTokenCacheForTest(); + clearReviewSuppressionCacheForTest(); vi.mocked(fetchPullRequestFreshness).mockReset(); vi.mocked(fetchPullRequestFreshness).mockImplementation(async (_env, args) => ({ status: "current", diff --git a/test/unit/review-memory-store.test.ts b/test/unit/review-memory-store.test.ts index 5e960fb109..24f1010a67 100644 --- a/test/unit/review-memory-store.test.ts +++ b/test/unit/review-memory-store.test.ts @@ -1,5 +1,7 @@ import { describe, expect, it, vi } from "vitest"; import { MAX_REVIEW_SUPPRESSIONS_PER_REPO, listReviewSuppressions, recordReviewSuppression } from "../../src/db/repositories"; +import * as repositoriesModule from "../../src/db/repositories"; +import { clearReviewSuppressionCacheForTest, getCachedReviewSuppressions, invalidateReviewSuppressionCache } from "../../src/review/review-memory-wire"; import { createTestEnv } from "../helpers/d1"; // Review memory (#2178, data-model slice of #1964): insert/list repository accessors over the @@ -205,3 +207,62 @@ describe("review-memory suppression store (#2178)", () => { expect(await rawCount(env, repoFullName)).toBe(MAX_REVIEW_SUPPRESSIONS_PER_REPO); }); }); + +// Short in-isolate TTL cache over listReviewSuppressions (#4508), mirroring rag.ts's chunkCountCache. +describe("getCachedReviewSuppressions / invalidateReviewSuppressionCache (#4508)", () => { + it("INVARIANT: a repeated read within the TTL for the same repo makes ZERO additional D1 reads", async () => { + clearReviewSuppressionCacheForTest(); + const env = createTestEnv(); + await recordReviewSuppression(env, { repoFullName: "owner/repo", category: "ai_review_split", patternHash: "hash-1" }); + const t0 = 1_000_000; + const first = await getCachedReviewSuppressions(env, "owner/repo", t0); + expect(first).toHaveLength(1); + + const spy = vi.spyOn(repositoriesModule, "listReviewSuppressions"); + const second = await getCachedReviewSuppressions(env, "owner/repo", t0 + 30_000); // well within the 60s TTL + // Read the assertion BEFORE mockRestore() — mockRestore() also resets recorded calls. + expect(spy).not.toHaveBeenCalled(); // reused the cached set — no fresh listReviewSuppressions call + spy.mockRestore(); + + expect(second).toEqual(first); + }); + + it("REGRESSION: a fresh suppression recorded between two renders IS reflected in the very next render, not masked by a stale cache entry", async () => { + clearReviewSuppressionCacheForTest(); + const env = createTestEnv(); + const t0 = 2_000_000; + const before = await getCachedReviewSuppressions(env, "owner/live-repo", t0); + expect(before).toHaveLength(0); // cold cache, nothing recorded yet — this populates the cache with an empty set + + // A maintainer runs `@gittensory resolve` between the two renders, well within the cache's TTL. + await recordReviewSuppression(env, { repoFullName: "owner/live-repo", category: "ai_review_split", patternHash: "hash-fresh" }); + invalidateReviewSuppressionCache("owner/live-repo"); + + const after = await getCachedReviewSuppressions(env, "owner/live-repo", t0 + 5_000); // still within the 60s TTL + expect(after).toHaveLength(1); // the fresh write is visible — NOT masked by the stale empty cached set + expect(after[0]).toMatchObject({ patternHash: "hash-fresh" }); + }); + + it("cache expires naturally past the TTL even without an explicit invalidation", async () => { + clearReviewSuppressionCacheForTest(); + const env = createTestEnv(); + const t0 = 3_000_000; + await getCachedReviewSuppressions(env, "owner/repo", t0); // populates the cache with an empty set + + await recordReviewSuppression(env, { repoFullName: "owner/repo", category: "ai_review_split", patternHash: "hash-late" }); + // No invalidateReviewSuppressionCache call here — relies on TTL expiry alone. + const afterTtl = await getCachedReviewSuppressions(env, "owner/repo", t0 + 60_001); + expect(afterTtl).toHaveLength(1); + }); + + it("caches independently per repoFullName", async () => { + clearReviewSuppressionCacheForTest(); + const env = createTestEnv(); + await recordReviewSuppression(env, { repoFullName: "owner/repo-a", category: "ai_review_split", patternHash: "hash-a" }); + const t0 = 4_000_000; + const a = await getCachedReviewSuppressions(env, "owner/repo-a", t0); + const b = await getCachedReviewSuppressions(env, "owner/repo-b", t0); + expect(a).toHaveLength(1); + expect(b).toHaveLength(0); + }); +});