diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index 8e9262a081048c30e58d7fe380c26ae7bad06cb6..9d563d3965c33b05c18680291ea5f409aef6057f 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -5382,7 +5382,7 @@ it.effect("keeps the diff cached across a file being ticked off", () => yield* service.diff(reference); yield* service.filesViewed(reference); - // The press forgets only the reader's own ticks: a diff of any size survives it. + // The press forgets only the reader's own ticks; cached diffs survive it. assert.strictEqual(diffReads, 1); assert.strictEqual(viewedReads, 2); @@ -5397,6 +5397,72 @@ it.effect("keeps the diff cached across a file being ticked off", () => }), ); +it.effect("returns large diff slices intact without retaining them in either cache", () => + Effect.gen(function* () { + let reads = 0; + const patch = "\u{1f4bb}".repeat(140_000); + const service = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], + providers: [ + fakeProvider("github", { + getDiff: () => + Effect.sync(() => { + reads += 1; + return { patch, truncated: false, nextCursor: "2" }; + }), + }), + ], + }); + const reference = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 }; + for (const input of [ + reference, + { ...reference, cursor: "2" }, + { ...reference, commit: "a".repeat(40) }, + ]) { + const before = reads; + assert.deepStrictEqual(yield* service.diff(input), { + patch, + truncated: false, + nextCursor: "2", + }); + assert.deepStrictEqual(yield* service.diff(input), { + patch, + truncated: false, + nextCursor: "2", + }); + assert.strictEqual(reads, before + 2); + } + }), +); + +it.effect("caches a small replacement after releasing a large diff", () => + Effect.gen(function* () { + let reads = 0; + const largePatch = "x".repeat(300_000); + const service = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], + providers: [ + fakeProvider("github", { + getDiff: () => + Effect.sync(() => { + reads += 1; + return { + patch: reads === 1 ? largePatch : "@@ small replacement", + truncated: false, + nextCursor: null, + }; + }), + }), + ], + }); + const reference = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 }; + assert.strictEqual((yield* service.diff(reference)).patch, largePatch); + assert.strictEqual((yield* service.diff(reference)).patch, "@@ small replacement"); + assert.strictEqual((yield* service.diff(reference)).patch, "@@ small replacement"); + assert.strictEqual(reads, 2); + }), +); + const environmentViewedProvider = ( revisions: Map, asked: Array>, diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index 2cc1180bb826633d2d097ea8b3a204bc33cfbf2d..a653ef94c713f5c76cc48543df19556911da79e5 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -151,6 +151,10 @@ const LIST_CACHE_CAPACITY = 64; const LIST_STATS_CACHE_CAPACITY = 32; const DETAIL_CACHE_CAPACITY = 128; const DIFF_CACHE_CAPACITY = 128; +// Each diff cache can retain at most 64 MiB of patch text, counting UTF-16 storage. +const MAX_CACHED_DIFF_PATCH_BYTES = 512 * 1024; +const canCacheDiff = (value: PullRequestDiffResult) => + value.patch.length * 2 <= MAX_CACHED_DIFF_PATCH_BYTES; const FILES_VIEWED_CACHE_CAPACITY = 128; const VIEWER_CACHE_CAPACITY = 32; @@ -2507,6 +2511,7 @@ export const make = Effect.gen(function* () { const record = (key: string, value: PullRequestDiffResult) => Effect.map(Clock.currentTimeMillis, (at) => { held.delete(key); + if (!canCacheDiff(value)) return; if (held.size >= DIFF_CACHE_CAPACITY) { const oldest = held.keys().next().value; if (oldest !== undefined) held.delete(oldest); @@ -2977,7 +2982,21 @@ export const make = Effect.gen(function* () { ? (lastGoodSummary.peek(refCacheKey(input))?.updatedAt ?? null) : null, ]); - return staleDiff(key, Cache.get(diffCache, key)); + const read = Cache.get(diffCache, key).pipe( + Effect.tap((value) => + canCacheDiff(value) + ? Effect.void + : Cache.getSuccess(diffCache, key).pipe( + Effect.flatMap((current) => + Option.isSome(current) && current.value === value + ? Cache.invalidate(diffCache, key) + : Effect.void, + ), + Effect.uninterruptible, + ), + ), + ); + return staleDiff(key, read); }; const filesViewedCache = yield* Cache.makeWith(