authorgravatar for admin@snappeh.comTristan Knight <admin@snappeh.com> 2026-10-12 01:38:37+01:00
committergravatar for noreply@github.comGitHub <noreply@github.com> 2026-10-12 00:38:37+00:00
log499a53c307ec6a0fbff5c83efde355729bd7d343
tree0150f254b16e8bf0fbd8d2358245a2e8495ada17
parent8569ce962a072cfd3943519f81645d6e51d8a03f
signature Signed by PGP key B5690EEEBB952194

perf(server): expired PR diffs no longer stay in memory until restart (#17786)

2 files changed, 83 insertions(+), 2 deletions(-)

apps/server/src/pullRequest/PullRequestService.test.ts+37
...@@ -1,5 +1,7 @@...@@ -1,5 +1,7 @@
1import { SourceControlProviderKind } from "@t3tools/contracts";1import { SourceControlProviderKind } from "@t3tools/contracts";
2import * as NodeServices from "@effect/platform-node/NodeServices";2import * as NodeServices from "@effect/platform-node/NodeServices";
3import * as NodeV8 from "node:v8";
4import * as NodeVM from "node:vm";
3import * as ChildProcessSpawner from "effect/process/ChildProcessSpawner";5import * as ChildProcessSpawner from "effect/process/ChildProcessSpawner";
4import * as KeyValueStore from "effect/persistence/KeyValueStore";6import * as KeyValueStore from "effect/persistence/KeyValueStore";
5import { assert, it } from "@effect/vitest";7import { assert, it } from "@effect/vitest";
...@@ -6188,6 +6190,41 @@ it.effect("caches a small replacement after releasing a large diff", () =>...@@ -6188,6 +6190,41 @@ it.effect("caches a small replacement after releasing a large diff", () =>
6188 }),6190 }),
6189);6191);
61906192
6193it.effect("releases an expired diff nobody reads again", () =>
6194 Effect.gen(function* () {
6195 NodeV8.setFlagsFromString("--expose-gc");
6196 const collect = NodeVM.runInNewContext("gc") as () => void;
6197 const settle = Effect.promise(async () => {
6198 // A weak reference keeps its target for the rest of the job that created it.
6199 await new Promise((resolve) => setImmediate(resolve));
6200 collect();
6201 });
6202 const service = yield* makeService({
6203 projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })],
6204 providers: [
6205 fakeProvider("github", {
6206 getDiff: () => Effect.sync(() => ({ patch: "@@", truncated: false, nextCursor: null })),
6207 }),
6208 ],
6209 });
6210 const reference = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 };
6211 const read = (input: Parameters<typeof service.diff>[0]) =>
6212 Effect.map(service.diff(input), (result) => new WeakRef(result));
6213 const page = yield* read(reference);
6214 const commit = yield* read({ ...reference, commit: "a".repeat(40) });
6215
6216 yield* TestClock.adjust("9 minutes");
6217 yield* settle;
6218 assert.isDefined(page.deref());
6219 assert.isDefined(commit.deref());
6220
6221 yield* TestClock.adjust("2 minutes");
6222 yield* settle;
6223 assert.isUndefined(page.deref());
6224 assert.isUndefined(commit.deref());
6225 }),
6226);
6227
6191const environmentViewedProvider = (6228const environmentViewedProvider = (
6192 revisions: Map<string, string>,6229 revisions: Map<string, string>,
6193 asked: Array<ReadonlyArray<string>>,6230 asked: Array<ReadonlyArray<string>>,
apps/server/src/pullRequest/PullRequestService.ts+46-2
...@@ -176,6 +176,8 @@ const LIST_STATS_CACHE_TTL = Duration.seconds(60);...@@ -176,6 +176,8 @@ const LIST_STATS_CACHE_TTL = Duration.seconds(60);
176const FILES_VIEWED_CACHE_TTL = Duration.seconds(15);176const FILES_VIEWED_CACHE_TTL = Duration.seconds(15);
177/** A diff can stay interactive while its next cached value is fetched off the critical path. */177/** A diff can stay interactive while its next cached value is fetched off the critical path. */
178const DIFF_STALE_WINDOW = Duration.minutes(10);178const DIFF_STALE_WINDOW = Duration.minutes(10);
179/** How long an expired diff may stay held before it is dropped unread. */
180const DIFF_SWEEP_INTERVAL = Duration.minutes(1);
179/** How long one host's signed-in login is believed without asking its CLI again. */181/** How long one host's signed-in login is believed without asking its CLI again. */
180const VIEWER_CACHE_TTL = Duration.minutes(10);182const VIEWER_CACHE_TTL = Duration.minutes(10);
181const SEARCH_VISIBILITY_TTL = Duration.minutes(10);183const SEARCH_VISIBILITY_TTL = Duration.minutes(10);
...@@ -1977,6 +1979,7 @@ export const make = Effect.gen(function* () {...@@ -1977,6 +1979,7 @@ export const make = Effect.gen(function* () {
1977 );1979 );
19781980
1979 const context = yield* Effect.context<never>();1981 const context = yield* Effect.context<never>();
1982 const scope = yield* Effect.scope;
1980 /** Runs a refresh as its own fiber, for the reads that answer from a held value first. */1983 /** Runs a refresh as its own fiber, for the reads that answer from a held value first. */
1981 const runFork = Effect.runForkWith(context);1984 const runFork = Effect.runForkWith(context);
19821985
...@@ -2697,8 +2700,9 @@ export const make = Effect.gen(function* () {...@@ -2697,8 +2700,9 @@ export const make = Effect.gen(function* () {
2697 if (oldest !== undefined) held.delete(oldest);2700 if (oldest !== undefined) held.delete(oldest);
2698 }2701 }
2699 held.set(key, { at, value });2702 held.set(key, { at, value });
2703 armDiffSweep();
2700 });2704 });
2701 return <E>(key: string, read: Effect.Effect<PullRequestDiffResult, E>) => {2705 const serve = <E>(key: string, read: Effect.Effect<PullRequestDiffResult, E>) => {
2702 const recorded = read.pipe(Effect.tap((value) => record(key, value)));2706 const recorded = read.pipe(Effect.tap((value) => record(key, value)));
2703 return Effect.flatMap(Clock.currentTimeMillis, (now) => {2707 return Effect.flatMap(Clock.currentTimeMillis, (now) => {
2704 const snapshot = held.get(key);2708 const snapshot = held.get(key);
...@@ -2709,6 +2713,14 @@ export const make = Effect.gen(function* () {...@@ -2709,6 +2713,14 @@ export const make = Effect.gen(function* () {
2709 return revalidate(recorded).pipe(Effect.as(snapshot.value));2713 return revalidate(recorded).pipe(Effect.as(snapshot.value));
2710 });2714 });
2711 };2715 };
2716 /** Drops what `serve` would already refuse, and says how many values are still held. */
2717 const dropExpired = Effect.map(Clock.currentTimeMillis, (now) => {
2718 for (const [key, snapshot] of held) {
2719 if (now - snapshot.at > staleMs) held.delete(key);
2720 }
2721 return held.size;
2722 });
2723 return { serve, dropExpired };
2712 })();2724 })();
27132725
2714 const makeLastGoodRead = <A>(capacity: number) => {2726 const makeLastGoodRead = <A>(capacity: number) => {
...@@ -3272,6 +3284,36 @@ export const make = Effect.gen(function* () {...@@ -3272,6 +3284,36 @@ export const make = Effect.gen(function* () {
3272 },3284 },
3273 },3285 },
3274 );3286 );
3287 /**
3288 * Both diff holders drop an expired value only when its own key is read again, and a diff's key
3289 * moves with every push, so a closed page's patches would stay held until the server restarted.
3290 * One pending sweep at a time, armed in the same step that leaves a value held and re-armed
3291 * while anything still is, so an idle server keeps no timer.
3292 */
3293 let diffSweepPending = false;
3294 // Forked from the service's own context, in one step with the flag: a caller's interruption
3295 // cannot strand the flag, and its request is not kept alive by the wait.
3296 const armDiffSweep = () => {
3297 if (diffSweepPending) return;
3298 diffSweepPending = true;
3299 runFork(Effect.forkIn(sweepDiffs, scope));
3300 };
3301 const sweepDiffs: Effect.Effect<void> = Effect.sleep(DIFF_SWEEP_INTERVAL).pipe(
3302 Effect.andThen(
3303 Effect.suspend(() => {
3304 // Cleared before the sweep, so a read that settles during it arms the next one itself.
3305 diffSweepPending = false;
3306 return staleDiff.dropExpired;
3307 }),
3308 ),
3309 // The cache drops its expired entries as its keys are walked.
3310 Effect.flatMap((stale) =>
3311 Effect.map(Cache.keys(diffCache), (keys) => stale + Array.from(keys).length),
3312 ),
3313 Effect.map((held) => {
3314 if (held > 0) armDiffSweep();
3315 }),
3316 );
3275 const diff: PullRequestService["Service"]["diff"] = (input) => {3317 const diff: PullRequestService["Service"]["diff"] = (input) => {
3276 const key = JSON.stringify([3318 const key = JSON.stringify([
3277 refCacheKey(input),3319 refCacheKey(input),
...@@ -3294,8 +3336,10 @@ export const make = Effect.gen(function* () {...@@ -3294,8 +3336,10 @@ export const make = Effect.gen(function* () {
3294 Effect.uninterruptible,3336 Effect.uninterruptible,
3295 ),3337 ),
3296 ),3338 ),
3339 // However the read ends, it may have left a value behind in the cache.
3340 Effect.ensuring(Effect.sync(armDiffSweep)),
3297 );3341 );
3298 return staleDiff(key, read);3342 return staleDiff.serve(key, read);
3299 };3343 };
33003344
3301 const filesViewedCache = yield* Cache.makeWith(3345 const filesViewedCache = yield* Cache.makeWith(