| author |  Julius Marminge <julius0216@outlook.com> 2026-09-18 19:03:56-07:00 |
| committer |  GitHub <noreply@github.com> 2026-09-18 19:03:56-07:00 |
| log | cb3d95c17487f3d834a5537f7c3aa0106bc9b5ec |
| tree | f722116ce694195333190d77aa94ef5118eca462 |
| parent | 9cb586acd0494c4f47f60045ee42ef25edb5a07c |
| signature | Signed by PGP key B5690EEEBB952194 |
fix(server): release oversized pull request diff cache entries (#12523)
2 files changed, 87 insertions(+), 2 deletions(-)
apps
server
src
apps/server/src/pullRequest/PullRequestService.test.ts+67-1
| ... | @@ -5382,7 +5382,7 @@ it.effect("keeps the diff cached across a file being ticked off", () => | ... | @@ -5382,7 +5382,7 @@ it.effect("keeps the diff cached across a file being ticked off", () => |
| 5382 | yield* service.diff(reference); | 5382 | yield* service.diff(reference); |
| 5383 | yield* service.filesViewed(reference); | 5383 | yield* service.filesViewed(reference); |
| 5384 | 5384 | ||
| 5385 | // The press forgets only the reader's own ticks: a diff of any size survives it. | 5385 | // The press forgets only the reader's own ticks; cached diffs survive it. |
| 5386 | assert.strictEqual(diffReads, 1); | 5386 | assert.strictEqual(diffReads, 1); |
| 5387 | assert.strictEqual(viewedReads, 2); | 5387 | assert.strictEqual(viewedReads, 2); |
| 5388 | 5388 | ||
| ... | @@ -5397,6 +5397,72 @@ it.effect("keeps the diff cached across a file being ticked off", () => | ... | @@ -5397,6 +5397,72 @@ it.effect("keeps the diff cached across a file being ticked off", () => |
| 5397 | }), | 5397 | }), |
| 5398 | ); | 5398 | ); |
| 5399 | 5399 | ||
| 5400 | it.effect("returns large diff slices intact without retaining them in either cache", () => | ||
| 5401 | Effect.gen(function* () { | ||
| 5402 | let reads = 0; | ||
| 5403 | const patch = "\u{1f4bb}".repeat(140_000); | ||
| 5404 | const service = yield* makeService({ | ||
| 5405 | projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], | ||
| 5406 | providers: [ | ||
| 5407 | fakeProvider("github", { | ||
| 5408 | getDiff: () => | ||
| 5409 | Effect.sync(() => { | ||
| 5410 | reads += 1; | ||
| 5411 | return { patch, truncated: false, nextCursor: "2" }; | ||
| 5412 | }), | ||
| 5413 | }), | ||
| 5414 | ], | ||
| 5415 | }); | ||
| 5416 | const reference = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 }; | ||
| 5417 | for (const input of [ | ||
| 5418 | reference, | ||
| 5419 | { ...reference, cursor: "2" }, | ||
| 5420 | { ...reference, commit: "a".repeat(40) }, | ||
| 5421 | ]) { | ||
| 5422 | const before = reads; | ||
| 5423 | assert.deepStrictEqual(yield* service.diff(input), { | ||
| 5424 | patch, | ||
| 5425 | truncated: false, | ||
| 5426 | nextCursor: "2", | ||
| 5427 | }); | ||
| 5428 | assert.deepStrictEqual(yield* service.diff(input), { | ||
| 5429 | patch, | ||
| 5430 | truncated: false, | ||
| 5431 | nextCursor: "2", | ||
| 5432 | }); | ||
| 5433 | assert.strictEqual(reads, before + 2); | ||
| 5434 | } | ||
| 5435 | }), | ||
| 5436 | ); | ||
| 5437 | |||
| 5438 | it.effect("caches a small replacement after releasing a large diff", () => | ||
| 5439 | Effect.gen(function* () { | ||
| 5440 | let reads = 0; | ||
| 5441 | const largePatch = "x".repeat(300_000); | ||
| 5442 | const service = yield* makeService({ | ||
| 5443 | projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], | ||
| 5444 | providers: [ | ||
| 5445 | fakeProvider("github", { | ||
| 5446 | getDiff: () => | ||
| 5447 | Effect.sync(() => { | ||
| 5448 | reads += 1; | ||
| 5449 | return { | ||
| 5450 | patch: reads === 1 ? largePatch : "@@ small replacement", | ||
| 5451 | truncated: false, | ||
| 5452 | nextCursor: null, | ||
| 5453 | }; | ||
| 5454 | }), | ||
| 5455 | }), | ||
| 5456 | ], | ||
| 5457 | }); | ||
| 5458 | const reference = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 }; | ||
| 5459 | assert.strictEqual((yield* service.diff(reference)).patch, largePatch); | ||
| 5460 | assert.strictEqual((yield* service.diff(reference)).patch, "@@ small replacement"); | ||
| 5461 | assert.strictEqual((yield* service.diff(reference)).patch, "@@ small replacement"); | ||
| 5462 | assert.strictEqual(reads, 2); | ||
| 5463 | }), | ||
| 5464 | ); | ||
| 5465 | |||
| 5400 | const environmentViewedProvider = ( | 5466 | const environmentViewedProvider = ( |
| 5401 | revisions: Map<string, string>, | 5467 | revisions: Map<string, string>, |
| 5402 | asked: Array<ReadonlyArray<string>>, | 5468 | asked: Array<ReadonlyArray<string>>, |
apps/server/src/pullRequest/PullRequestService.ts+20-1
| ... | @@ -151,6 +151,10 @@ const LIST_CACHE_CAPACITY = 64; | ... | @@ -151,6 +151,10 @@ const LIST_CACHE_CAPACITY = 64; |
| 151 | const LIST_STATS_CACHE_CAPACITY = 32; | 151 | const LIST_STATS_CACHE_CAPACITY = 32; |
| 152 | const DETAIL_CACHE_CAPACITY = 128; | 152 | const DETAIL_CACHE_CAPACITY = 128; |
| 153 | const DIFF_CACHE_CAPACITY = 128; | 153 | const DIFF_CACHE_CAPACITY = 128; |
| 154 | // Each diff cache can retain at most 64 MiB of patch text, counting UTF-16 storage. | ||
| 155 | const MAX_CACHED_DIFF_PATCH_BYTES = 512 * 1024; | ||
| 156 | const canCacheDiff = (value: PullRequestDiffResult) => | ||
| 157 | value.patch.length * 2 <= MAX_CACHED_DIFF_PATCH_BYTES; | ||
| 154 | const FILES_VIEWED_CACHE_CAPACITY = 128; | 158 | const FILES_VIEWED_CACHE_CAPACITY = 128; |
| 155 | const VIEWER_CACHE_CAPACITY = 32; | 159 | const VIEWER_CACHE_CAPACITY = 32; |
| 156 | 160 | ||
| ... | @@ -2507,6 +2511,7 @@ export const make = Effect.gen(function* () { | ... | @@ -2507,6 +2511,7 @@ export const make = Effect.gen(function* () { |
| 2507 | const record = (key: string, value: PullRequestDiffResult) => | 2511 | const record = (key: string, value: PullRequestDiffResult) => |
| 2508 | Effect.map(Clock.currentTimeMillis, (at) => { | 2512 | Effect.map(Clock.currentTimeMillis, (at) => { |
| 2509 | held.delete(key); | 2513 | held.delete(key); |
| 2514 | if (!canCacheDiff(value)) return; | ||
| 2510 | if (held.size >= DIFF_CACHE_CAPACITY) { | 2515 | if (held.size >= DIFF_CACHE_CAPACITY) { |
| 2511 | const oldest = held.keys().next().value; | 2516 | const oldest = held.keys().next().value; |
| 2512 | if (oldest !== undefined) held.delete(oldest); | 2517 | if (oldest !== undefined) held.delete(oldest); |
| ... | @@ -2977,7 +2982,21 @@ export const make = Effect.gen(function* () { | ... | @@ -2977,7 +2982,21 @@ export const make = Effect.gen(function* () { |
| 2977 | ? (lastGoodSummary.peek(refCacheKey(input))?.updatedAt ?? null) | 2982 | ? (lastGoodSummary.peek(refCacheKey(input))?.updatedAt ?? null) |
| 2978 | : null, | 2983 | : null, |
| 2979 | ]); | 2984 | ]); |
| 2980 | return staleDiff(key, Cache.get(diffCache, key)); | 2985 | const read = Cache.get(diffCache, key).pipe( |
| 2986 | Effect.tap((value) => | ||
| 2987 | canCacheDiff(value) | ||
| 2988 | ? Effect.void | ||
| 2989 | : Cache.getSuccess(diffCache, key).pipe( | ||
| 2990 | Effect.flatMap((current) => | ||
| 2991 | Option.isSome(current) && current.value === value | ||
| 2992 | ? Cache.invalidate(diffCache, key) | ||
| 2993 | : Effect.void, | ||
| 2994 | ), | ||
| 2995 | Effect.uninterruptible, | ||
| 2996 | ), | ||
| 2997 | ), | ||
| 2998 | ); | ||
| 2999 | return staleDiff(key, read); | ||
| 2981 | }; | 3000 | }; |
| 2982 | 3001 | ||
| 2983 | const filesViewedCache = yield* Cache.makeWith( | 3002 | const filesViewedCache = yield* Cache.makeWith( |