From 4d4b7c81b45e1aa04179134feb506e5801942a36 Mon Sep 17 00:00:00 2001 From: Whoisaldo Date: Mon, 28 Sep 2026 13:56:49 -0400 Subject: [PATCH] fix(server): first pull request open no longer fails as interrupted A read that arrives while effect/Cache is still stopping an abandoned lookup joins it and gets back a bare interrupt. Pull request reads now ask once more in that case, which starts a fresh lookup. A caller that was itself interrupted does not retry. Closes #14113. --- .../src/pullRequest/PullRequestService.ts | 19 +++--- apps/server/src/utils/getCached.test.ts | 61 +++++++++++++++++++ apps/server/src/utils/getCached.ts | 20 ++++++ 3 files changed, 91 insertions(+), 9 deletions(-) create mode 100644 apps/server/src/utils/getCached.test.ts create mode 100644 apps/server/src/utils/getCached.ts diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index 866a45e49b09..22114599bc92 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -77,6 +77,7 @@ import * as ProjectionSnapshotQuery from "../orchestration/Services/ProjectionSn import * as PullRequestFilesViewed from "../persistence/PullRequestFilesViewed.ts"; import * as SourceControlProviderRegistry from "../sourceControl/SourceControlProviderRegistry.ts"; import * as SourceControlRateLimit from "../sourceControl/SourceControlRateLimit.ts"; +import { getCached } from "../utils/getCached.ts"; import { type ProviderChangeRequest, type ProviderListCursor, @@ -1014,13 +1015,13 @@ export const make = Effect.gen(function* () { // roots are the same lookup, and putting them on separate flights would spawn two of // this host's CLIs on a cold page load, which is the coalescing this exists for. const key = JSON.stringify([host, api.kind, [...new Set(roots)].sort()]); - if (options?.allowPaused === true) return Cache.get(viewerFlights, key); + if (options?.allowPaused === true) return getCached(viewerFlights, key); // The pause is checked here rather than inside the lookup, so that it holds back the // callers nobody is waiting on without splitting the flight they share with a press. // A failed lookup is held nowhere, so letting a background read through would spawn // this host's CLI on every refresh for as long as the pause lasted, and re-extend it. return rateLimits.check({ provider: api.kind, host }).pipe( - Effect.flatMap(() => Cache.get(viewerFlights, key)), + Effect.flatMap(() => getCached(viewerFlights, key)), Effect.catch((error) => Effect.succeed({ host, @@ -2854,7 +2855,7 @@ export const make = Effect.gen(function* () { ? null : Object.entries(input.cursors).toSorted(([left], [right]) => left.localeCompare(right)), ]); - return Cache.get(listCache, key); + return getCached(listCache, key); }; const detailCache = yield* Cache.makeWith( @@ -2922,7 +2923,7 @@ export const make = Effect.gen(function* () { // `serveHeld` returns immediately. Skip the write when that read is older // than a later strict summary — display reuse would otherwise keep the // regression and never ask the host again. - const read = Cache.get(detailCache, key).pipe( + const read = getCached(detailCache, key).pipe( Effect.tap((value) => { const summary = summaryFromDetail(value, lastGoodSummary.peek(key)); return shouldReplaceHeldSummary(key, summary) @@ -2959,7 +2960,7 @@ export const make = Effect.gen(function* () { return Cache.getSuccess(detailCache, key).pipe( Effect.flatMap( Option.match({ - onNone: () => Cache.get(previewCache, key), + onNone: () => getCached(previewCache, key), onSome: (detail) => Effect.succeed(previewFields(detail)), }), ), @@ -2967,7 +2968,7 @@ export const make = Effect.gen(function* () { }; const activity: PullRequestService["Service"]["activity"] = (input) => { const key = refCacheKey(input); - return Cache.get(activityCache, key); + return getCached(activityCache, key); }; const diffCache = yield* Cache.makeWith( @@ -2997,7 +2998,7 @@ export const make = Effect.gen(function* () { ? (lastGoodSummary.peek(refCacheKey(input))?.updatedAt ?? null) : null, ]); - const read = Cache.get(diffCache, key).pipe( + const read = getCached(diffCache, key).pipe( Effect.tap((value) => canCacheDiff(value) ? Effect.void @@ -3029,7 +3030,7 @@ export const make = Effect.gen(function* () { const filesViewed: PullRequestService["Service"]["filesViewed"] = (input) => canonicalRef(input).pipe( Effect.flatMap((ref) => - Cache.get(filesViewedCache, JSON.stringify([refCacheKey(ref), filesViewedEpoch(ref)])), + getCached(filesViewedCache, JSON.stringify([refCacheKey(ref), filesViewedEpoch(ref)])), ), ); @@ -3077,7 +3078,7 @@ export const make = Effect.gen(function* () { } if (missing.size === 0) return { stats: held }; const key = statsBatchKey(missing.values()); - const { result, at } = yield* Cache.get(listStatsCache, key); + const { result, at } = yield* getCached(listStatsCache, key); for (const [key, ref] of missing) { const stat = result.stats.find( (stat) => diff --git a/apps/server/src/utils/getCached.test.ts b/apps/server/src/utils/getCached.test.ts new file mode 100644 index 000000000000..78ef6a3f68de --- /dev/null +++ b/apps/server/src/utils/getCached.test.ts @@ -0,0 +1,61 @@ +import { assert, describe, it } from "@effect/vitest"; +import * as Cache from "effect/Cache"; +import * as Deferred from "effect/Deferred"; +import * as Effect from "effect/Effect"; +import * as Exit from "effect/Exit"; +import * as Fiber from "effect/Fiber"; + +import { getCached } from "./getCached.ts"; + +describe("getCached", () => { + it.effect("a read that joins an abandoned lookup gets a fresh result", () => + Effect.gen(function* () { + const started = yield* Deferred.make(); + const stopped = yield* Deferred.make(); + let lookups = 0; + const cache = yield* Cache.make({ + capacity: 10, + lookup: () => + ++lookups === 1 + ? // Like `gh`, the first lookup takes a moment to stop once interrupted. + Effect.yieldNow.pipe( + Effect.andThen(Deferred.succeed(started, undefined)), + Effect.andThen(Effect.never), + Effect.onInterrupt(() => Deferred.await(stopped)), + ) + : Effect.succeed("fresh"), + }); + const first = yield* getCached(cache, "detail").pipe(Effect.forkChild); + yield* Deferred.await(started); + yield* Fiber.interrupt(first).pipe(Effect.forkChild({ startImmediately: true })); + const second = yield* getCached(cache, "detail").pipe( + Effect.forkChild({ startImmediately: true }), + ); + yield* Deferred.succeed(stopped, undefined); + + assert.deepStrictEqual(yield* Fiber.await(second), Exit.succeed("fresh")); + assert.strictEqual(lookups, 2); + }), + ); + + it.effect("an interrupted read does not start another lookup", () => + Effect.gen(function* () { + const started = yield* Deferred.make(); + let lookups = 0; + const cache = yield* Cache.make({ + capacity: 10, + lookup: () => { + lookups++; + return Deferred.succeed(started, undefined).pipe(Effect.andThen(Effect.never)); + }, + }); + const read = yield* getCached(cache, "detail").pipe(Effect.forkChild); + yield* Deferred.await(started); + + yield* Fiber.interrupt(read); + + assert.isTrue(Exit.hasInterrupts(yield* Fiber.await(read))); + assert.strictEqual(lookups, 1); + }), + ); +}); diff --git a/apps/server/src/utils/getCached.ts b/apps/server/src/utils/getCached.ts new file mode 100644 index 000000000000..b9b4066f4c6c --- /dev/null +++ b/apps/server/src/utils/getCached.ts @@ -0,0 +1,20 @@ +import * as Cache from "effect/Cache"; +import * as Cause from "effect/Cause"; +import * as Effect from "effect/Effect"; + +/** + * `Cache.get` for a caller that must not inherit another caller's interrupt. + * + * When a lookup's last caller is interrupted, `Cache` interrupts the lookup but + * keeps its entry until the lookup has stopped. A caller that arrives in that + * window joins the dying lookup and fails with a bare interrupt. The entry is + * gone by the time that failure arrives, so one more `get` starts a fresh + * lookup. A caller that was itself interrupted never reaches the retry. + */ +export const getCached = ( + cache: Cache.Cache, + key: Key, +): Effect.Effect => + Cache.get(cache, key).pipe( + Effect.catchCauseIf(Cause.hasInterruptsOnly, () => Cache.get(cache, key)), + );