From 499a53c307ec6a0fbff5c83efde355729bd7d343 Mon Sep 17 00:00:00 2001 From: Tristan Knight Date: Mon, 12 Oct 2026 01:38:37 +0100 Subject: [PATCH] perf(server): expired PR diffs no longer stay in memory until restart (#17786) --- .../pullRequest/PullRequestService.test.ts | 37 ++++++++++++++ .../src/pullRequest/PullRequestService.ts | 48 ++++++++++++++++++- 2 files changed, 83 insertions(+), 2 deletions(-) diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index 26093ad19790999b9f611a4ff2f2f90837a72165..cc1469436a15eb47e01f24e3a92909bd87e9cc9f 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -1,5 +1,7 @@ import { SourceControlProviderKind } from "@t3tools/contracts"; import * as NodeServices from "@effect/platform-node/NodeServices"; +import * as NodeV8 from "node:v8"; +import * as NodeVM from "node:vm"; import * as ChildProcessSpawner from "effect/process/ChildProcessSpawner"; import * as KeyValueStore from "effect/persistence/KeyValueStore"; import { assert, it } from "@effect/vitest"; @@ -6188,6 +6190,41 @@ it.effect("caches a small replacement after releasing a large diff", () => }), ); +it.effect("releases an expired diff nobody reads again", () => + Effect.gen(function* () { + NodeV8.setFlagsFromString("--expose-gc"); + const collect = NodeVM.runInNewContext("gc") as () => void; + const settle = Effect.promise(async () => { + // A weak reference keeps its target for the rest of the job that created it. + await new Promise((resolve) => setImmediate(resolve)); + collect(); + }); + const service = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], + providers: [ + fakeProvider("github", { + getDiff: () => Effect.sync(() => ({ patch: "@@", truncated: false, nextCursor: null })), + }), + ], + }); + const reference = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 }; + const read = (input: Parameters[0]) => + Effect.map(service.diff(input), (result) => new WeakRef(result)); + const page = yield* read(reference); + const commit = yield* read({ ...reference, commit: "a".repeat(40) }); + + yield* TestClock.adjust("9 minutes"); + yield* settle; + assert.isDefined(page.deref()); + assert.isDefined(commit.deref()); + + yield* TestClock.adjust("2 minutes"); + yield* settle; + assert.isUndefined(page.deref()); + assert.isUndefined(commit.deref()); + }), +); + const environmentViewedProvider = ( revisions: Map, asked: Array>, diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index 454733320da536e66dd8a9ebd03c6ab8e2f1b56c..d57934f10a1678ab7a6404de85c0bc2080974aad 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -176,6 +176,8 @@ const LIST_STATS_CACHE_TTL = Duration.seconds(60); const FILES_VIEWED_CACHE_TTL = Duration.seconds(15); /** A diff can stay interactive while its next cached value is fetched off the critical path. */ const DIFF_STALE_WINDOW = Duration.minutes(10); +/** How long an expired diff may stay held before it is dropped unread. */ +const DIFF_SWEEP_INTERVAL = Duration.minutes(1); /** How long one host's signed-in login is believed without asking its CLI again. */ const VIEWER_CACHE_TTL = Duration.minutes(10); const SEARCH_VISIBILITY_TTL = Duration.minutes(10); @@ -1977,6 +1979,7 @@ export const make = Effect.gen(function* () { ); const context = yield* Effect.context(); + const scope = yield* Effect.scope; /** Runs a refresh as its own fiber, for the reads that answer from a held value first. */ const runFork = Effect.runForkWith(context); @@ -2697,8 +2700,9 @@ export const make = Effect.gen(function* () { if (oldest !== undefined) held.delete(oldest); } held.set(key, { at, value }); + armDiffSweep(); }); - return (key: string, read: Effect.Effect) => { + const serve = (key: string, read: Effect.Effect) => { const recorded = read.pipe(Effect.tap((value) => record(key, value))); return Effect.flatMap(Clock.currentTimeMillis, (now) => { const snapshot = held.get(key); @@ -2709,6 +2713,14 @@ export const make = Effect.gen(function* () { return revalidate(recorded).pipe(Effect.as(snapshot.value)); }); }; + /** Drops what `serve` would already refuse, and says how many values are still held. */ + const dropExpired = Effect.map(Clock.currentTimeMillis, (now) => { + for (const [key, snapshot] of held) { + if (now - snapshot.at > staleMs) held.delete(key); + } + return held.size; + }); + return { serve, dropExpired }; })(); const makeLastGoodRead = (capacity: number) => { @@ -3272,6 +3284,36 @@ export const make = Effect.gen(function* () { }, }, ); + /** + * Both diff holders drop an expired value only when its own key is read again, and a diff's key + * moves with every push, so a closed page's patches would stay held until the server restarted. + * One pending sweep at a time, armed in the same step that leaves a value held and re-armed + * while anything still is, so an idle server keeps no timer. + */ + let diffSweepPending = false; + // Forked from the service's own context, in one step with the flag: a caller's interruption + // cannot strand the flag, and its request is not kept alive by the wait. + const armDiffSweep = () => { + if (diffSweepPending) return; + diffSweepPending = true; + runFork(Effect.forkIn(sweepDiffs, scope)); + }; + const sweepDiffs: Effect.Effect = Effect.sleep(DIFF_SWEEP_INTERVAL).pipe( + Effect.andThen( + Effect.suspend(() => { + // Cleared before the sweep, so a read that settles during it arms the next one itself. + diffSweepPending = false; + return staleDiff.dropExpired; + }), + ), + // The cache drops its expired entries as its keys are walked. + Effect.flatMap((stale) => + Effect.map(Cache.keys(diffCache), (keys) => stale + Array.from(keys).length), + ), + Effect.map((held) => { + if (held > 0) armDiffSweep(); + }), + ); const diff: PullRequestService["Service"]["diff"] = (input) => { const key = JSON.stringify([ refCacheKey(input), @@ -3294,8 +3336,10 @@ export const make = Effect.gen(function* () { Effect.uninterruptible, ), ), + // However the read ends, it may have left a value behind in the cache. + Effect.ensuring(Effect.sync(armDiffSweep)), ); - return staleDiff(key, read); + return staleDiff.serve(key, read); }; const filesViewedCache = yield* Cache.makeWith( -- 2.54.0