From 869d0f4b84e131ffed2b83e4fbbd38b462d4027d Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Wed, 9 Sep 2026 16:23:14 -0700 Subject: [PATCH] fix(server): preserve recent PR reads across server restarts --- .../pullRequest/PullRequestReadCache.test.ts | 78 +++++++++++ .../src/pullRequest/PullRequestReadCache.ts | 125 ++++++++++++++++++ .../pullRequest/PullRequestService.test.ts | 9 ++ .../src/pullRequest/PullRequestService.ts | 89 +++++++++---- apps/server/src/server.ts | 2 + 5 files changed, 275 insertions(+), 28 deletions(-) create mode 100644 apps/server/src/pullRequest/PullRequestReadCache.test.ts create mode 100644 apps/server/src/pullRequest/PullRequestReadCache.ts diff --git a/apps/server/src/pullRequest/PullRequestReadCache.test.ts b/apps/server/src/pullRequest/PullRequestReadCache.test.ts new file mode 100644 index 000000000000..f94ff3cf586d --- /dev/null +++ b/apps/server/src/pullRequest/PullRequestReadCache.test.ts @@ -0,0 +1,78 @@ +import { assert, it } from "@effect/vitest"; +import * as NodeServices from "@effect/platform-node/NodeServices"; +import { PullRequestOperationError } from "@t3tools/contracts"; +import * as Deferred from "effect/Deferred"; +import * as Effect from "effect/Effect"; +import * as Fiber from "effect/Fiber"; +import * as FileSystem from "effect/FileSystem"; +import * as Layer from "effect/Layer"; +import * as TestClock from "effect/testing/TestClock"; +import * as KeyValueStore from "effect/unstable/persistence/KeyValueStore"; +import * as Persistence from "effect/unstable/persistence/Persistence"; +import * as PullRequestReadCache from "./PullRequestReadCache.ts"; + +const cacheLayer = (directory: string) => + PullRequestReadCache.make.pipe( + Effect.provide( + Persistence.layerKvs.pipe(Layer.provideMerge(KeyValueStore.layerFileSystem(directory))), + ), + ); + +it.layer(NodeServices.layer)("PR filesystem cache", (it) => { + it.effect("reuses files after restart and respects the original expiry", () => + Effect.gen(function* () { + const fs = yield* FileSystem.FileSystem; + const directory = yield* fs.makeTempDirectoryScoped({ prefix: "t3-pr-cache-" }); + let reads = 0; + const lookup = Effect.sync(() => String(++reads)); + const first = yield* cacheLayer(directory); + const key = "long/repository/key".repeat(100); + assert.strictEqual(yield* first.get(key, lookup), "1"); + yield* TestClock.adjust("59 seconds"); + const restarted = yield* cacheLayer(directory); + assert.strictEqual(yield* restarted.get(key, lookup), "1"); + yield* TestClock.adjust("1 second"); + assert.strictEqual(yield* restarted.get(key, lookup), "2"); + assert.strictEqual(reads, 2); + assert.strictEqual((yield* fs.readDirectory(directory)).length, 1); + }), + ); + + it.effect("clears in-flight reads before a new service can reuse them", () => + Effect.gen(function* () { + const fs = yield* FileSystem.FileSystem; + const directory = yield* fs.makeTempDirectoryScoped({ prefix: "t3-pr-cache-" }); + const started = yield* Deferred.make(); + const release = yield* Deferred.make(); + const cache = yield* cacheLayer(directory); + const read = yield* cache + .get( + "summary", + Deferred.succeed(started, undefined).pipe( + Effect.andThen(Deferred.await(release)), + Effect.as("old"), + ), + ) + .pipe(Effect.forkChild); + yield* Deferred.await(started); + const invalidate = yield* cache.invalidate.pipe(Effect.forkChild({ startImmediately: true })); + yield* Deferred.succeed(release, undefined); + yield* Fiber.join(read); + yield* Fiber.join(invalidate); + const restarted = yield* cacheLayer(directory); + assert.strictEqual(yield* restarted.get("summary", Effect.succeed("new")), "new"); + }), + ); + + it.effect("does not persist failed GitHub reads", () => + Effect.gen(function* () { + const fs = yield* FileSystem.FileSystem; + const directory = yield* fs.makeTempDirectoryScoped({ prefix: "t3-pr-cache-" }); + const cache = yield* cacheLayer(directory); + const error = new PullRequestOperationError({ operation: "summary", detail: "unavailable" }); + yield* cache.get("summary", Effect.fail(error)).pipe(Effect.flip); + const restarted = yield* cacheLayer(directory); + assert.strictEqual(yield* restarted.get("summary", Effect.succeed("recovered")), "recovered"); + }), + ); +}); diff --git a/apps/server/src/pullRequest/PullRequestReadCache.ts b/apps/server/src/pullRequest/PullRequestReadCache.ts new file mode 100644 index 000000000000..62d1cffc3c83 --- /dev/null +++ b/apps/server/src/pullRequest/PullRequestReadCache.ts @@ -0,0 +1,125 @@ +import * as Cache from "effect/Cache"; +import * as Clock from "effect/Clock"; +import * as Equal from "effect/Equal"; +import * as Hash from "effect/Hash"; +import { PullRequestOperationError, PullRequestUnavailableError } from "@t3tools/contracts"; +import * as Crypto from "effect/Crypto"; +import * as Encoding from "effect/Encoding"; +import * as Option from "effect/Option"; +import * as Context from "effect/Context"; +import * as Duration from "effect/Duration"; +import * as Effect from "effect/Effect"; +import * as Exit from "effect/Exit"; +import * as Layer from "effect/Layer"; +import * as Path from "effect/Path"; +import * as Schema from "effect/Schema"; +import * as Semaphore from "effect/Semaphore"; +import * as KeyValueStore from "effect/unstable/persistence/KeyValueStore"; +import * as Persistable from "effect/unstable/persistence/Persistable"; +import * as PersistedCache from "effect/unstable/persistence/PersistedCache"; +import * as Persistence from "effect/unstable/persistence/Persistence"; +import { ServerConfig } from "../config.ts"; + +const CONCURRENT_READS = 512; +type ReadError = PullRequestOperationError | PullRequestUnavailableError; + +class Read extends Persistable.Class<{ + payload: { key: string; lookup: Effect.Effect }; +}>()("PullRequestRead", { + primaryKey: ({ key }) => key, + success: Schema.Struct({ payload: Schema.String, expiresAt: Schema.Finite }), + error: Schema.Union([PullRequestOperationError, PullRequestUnavailableError]), +}) { + [Equal.symbol](that: unknown): boolean { + return that instanceof Read && that.key === this.key; + } + [Hash.symbol](): number { + return Hash.string(this.key); + } +} + +export class PullRequestReadCache extends Context.Service< + PullRequestReadCache, + { + readonly get: ( + key: string, + lookup: Effect.Effect, + ) => Effect.Effect; + readonly invalidate: Effect.Effect; + } +>()("t3/pullRequest/PullRequestReadCache") {} + +export const make = Effect.gen(function* () { + const backing = yield* KeyValueStore.KeyValueStore; + const crypto = yield* Crypto.Crypto; + const clock = yield* Clock.Clock; + let enabled = true; + const lock = yield* Semaphore.make(CONCURRENT_READS); + const timeToLive: Persistable.TimeToLiveFn = (exit) => + Exit.isSuccess(exit) + ? Duration.millis(Math.max(0, exit.value.expiresAt - clock.currentTimeMillisUnsafe())) + : Duration.zero; + const cache = yield* PersistedCache.make( + (request: Read) => + request.lookup.pipe( + Effect.map((payload) => ({ payload, expiresAt: clock.currentTimeMillisUnsafe() + 60_000 })), + ), + { + storeId: "pr-v2", + timeToLive, + inMemoryTTL: timeToLive, + inMemoryCapacity: CONCURRENT_READS, + }, + ); + return PullRequestReadCache.of({ + get: Effect.fn("PullRequestReadCache.get")(function* (key, lookup) { + if (!enabled) return yield* lookup; + const digest = yield* crypto + .digest("SHA-256", new TextEncoder().encode(key)) + .pipe(Effect.option); + if (Option.isNone(digest)) return yield* lookup; + const read = yield* Effect.cached(lookup); + return yield* cache + .get(new Read({ key: Encoding.encodeHex(digest.value), lookup: read })) + .pipe( + Effect.map((result) => result.payload), + Effect.catchTags({ + PersistenceError: () => read, + SchemaError: () => read, + }), + Effect.uninterruptible, + lock.withPermits(1), + ); + }), + // Let existing reads finish before clearing, so they cannot repopulate stale entries. + invalidate: Cache.invalidateAll(cache.inMemory).pipe( + Effect.andThen(backing.clear), + Effect.catch(() => { + enabled = false; + return Effect.logWarning("PR cache disabled after clearing failed"); + }), + lock.withPermits(CONCURRENT_READS), + ), + }); +}); + +export const layer = Layer.unwrap( + Effect.gen(function* () { + const config = yield* ServerConfig; + const path = yield* Path.Path; + return Layer.effect(PullRequestReadCache, make).pipe( + Layer.provide(Persistence.layerKvs), + Layer.provide( + KeyValueStore.layerFileSystem( + path.join(config.providerStatusCacheDir, "pull-requests"), + ).pipe( + Layer.catch(() => + Layer.effectDiscard( + Effect.logWarning("PR cache directory unavailable; using memory cache"), + ).pipe(Layer.provideMerge(KeyValueStore.layerMemory)), + ), + ), + ), + ); + }), +); diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index db2f6cf2642f..3117c0072a78 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -1,3 +1,6 @@ +import * as NodeServices from "@effect/platform-node/NodeServices"; +import * as KeyValueStore from "effect/unstable/persistence/KeyValueStore"; +import * as Persistence from "effect/unstable/persistence/Persistence"; import { assert, it } from "@effect/vitest"; import * as Deferred from "effect/Deferred"; import * as Effect from "effect/Effect"; @@ -24,6 +27,7 @@ import { } from "./PullRequestProvider.ts"; import { PullRequestProviderRegistry, fromProviders } from "./PullRequestProviderRegistry.ts"; import * as PullRequestService from "./PullRequestService.ts"; +import * as PullRequestReadCache from "./PullRequestReadCache.ts"; function project(input: { readonly id: string; @@ -198,6 +202,11 @@ function makeService(input: { }), }), SourceControlRateLimit.layer, + Layer.effect(PullRequestReadCache.PullRequestReadCache, PullRequestReadCache.make).pipe( + Layer.provide(Persistence.layerKvs), + Layer.provide(KeyValueStore.layerMemory), + Layer.provide(NodeServices.layer), + ), ), ), ); diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index 4c81d0f6d264..37716da44005 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -11,6 +11,7 @@ import * as Effect from "effect/Effect"; import * as Exit from "effect/Exit"; import * as Layer from "effect/Layer"; import * as PubSub from "effect/PubSub"; +import * as Option from "effect/Option"; import * as Schema from "effect/Schema"; import type * as Scope from "effect/Scope"; import * as Stream from "effect/Stream"; @@ -50,8 +51,8 @@ import { type PullRequestLabelCandidateList, type PullRequestLabelChangeInput, type PullRequestSubmitReviewInput, - type PullRequestStack, - type PullRequestSummary, + PullRequestStack, + PullRequestSummary, type PullRequestThreadReplyInput, type PullRequestThreadResolutionInput, type PullRequestThreadCommentsInput, @@ -71,6 +72,7 @@ import { type PullRequestProviderApi, PullRequestProviderError, } from "./PullRequestProvider.ts"; +import * as PullRequestReadCache from "./PullRequestReadCache.ts"; import { PullRequestProviderRegistry } from "./PullRequestProviderRegistry.ts"; export interface PullRequestMergeEvent extends PullRequestRef { @@ -114,7 +116,6 @@ const REPOSITORY_SEARCH_CHUNK = 100; * `invalidate` rather than a flag on the read, so an ordinary read can never opt out. */ const LIST_CACHE_TTL = Duration.seconds(30); -const SUMMARY_CACHE_TTL = Duration.seconds(60); const DETAIL_CACHE_TTL = Duration.seconds(15); const DIFF_CACHE_TTL = Duration.seconds(60); /** A commit is content-addressed, so its own diff cannot change under its key. */ @@ -535,6 +536,7 @@ export const make = Effect.gen(function* () { const projections = yield* ProjectionSnapshotQuery.ProjectionSnapshotQuery; const sourceControlProviders = yield* SourceControlProviderRegistry.SourceControlProviderRegistry; const rateLimits = yield* SourceControlRateLimit.SourceControlRateLimit; + const readCache = yield* PullRequestReadCache.PullRequestReadCache; const refineUnknownProjectKinds = ( projects: ReadonlyArray, @@ -2333,18 +2335,49 @@ export const make = Effect.gen(function* () { }; }; - const summaryCache = yield* Cache.makeWith( - (key: string) => { - return summaryUncached(refOfCacheKey(key)); - }, - { - capacity: DETAIL_CACHE_CAPACITY, - timeToLive: (exit) => (Exit.isSuccess(exit) ? SUMMARY_CACHE_TTL : Duration.zero), - }, - ); + const persistedRead = Effect.fn("PullRequestService.persistedRead")(function* ( + input: PullRequestRef, + operation: string, + codec: Schema.Codec, + read: Effect.Effect, + ) { + const project = yield* requireProject(input); + const key = [ + operation, + project.api.kind, + project.host.toLowerCase(), + project.repository.toLowerCase(), + project.project.id, + project.project.workspaceRoot, + String(input.number), + ] + .map(encodeURIComponent) + .join(":"); + const lookup = yield* Effect.cached(read); + const encodedRead = lookup.pipe( + Effect.flatMap((value) => + Schema.encodeEffect(codec)(value).pipe( + Effect.mapError( + (cause) => + new PullRequestOperationError({ + operation: "cache", + detail: "Could not encode PR cache data.", + cause, + }), + ), + ), + ), + ); + const payload = yield* readCache.get(key, encodedRead); + const decoded = yield* Schema.decodeUnknownEffect(codec)(payload).pipe(Effect.option); + return Option.isSome(decoded) ? decoded.value : yield* lookup; + }); + const summaryCodec = Schema.fromJsonString(PullRequestSummary); + const stackCodec = Schema.fromJsonString(Schema.NullOr(PullRequestStack)); + const summary: PullRequestService["Service"]["summary"] = (input, options) => { const key = refCacheKey(input); - const cached = Cache.get(summaryCache, key); + const cached = persistedRead(input, "summary", summaryCodec, summaryUncached(input)); const held = lastGoodSummary.peek(key); return held !== undefined && (options?.recoverTransientFailure !== false || held.state === "merged") @@ -2356,18 +2389,13 @@ export const make = Effect.gen(function* () { ); }; - const stackCache = yield* Cache.makeWith( - (key: string) => { - const [referenceKey, includeDetails] = JSON.parse(key) as [string, boolean]; - return stackUncached(refOfCacheKey(referenceKey), { includeDetails }); - }, - { - capacity: DETAIL_CACHE_CAPACITY, - timeToLive: (exit) => (Exit.isSuccess(exit) ? SUMMARY_CACHE_TTL : Duration.zero), - }, - ); const stack: PullRequestService["Service"]["stack"] = (input, options) => - Cache.get(stackCache, JSON.stringify([refCacheKey(input), options?.includeDetails !== false])); + persistedRead( + input, + `stack:${options?.includeDetails !== false}`, + stackCodec, + stackUncached(input, options), + ); // Keys serialize positionally and parse back in the lookup, so the cache is the only holder // of in-flight state: concurrent identical reads coalesce on the key into one host request. @@ -2642,7 +2670,7 @@ export const make = Effect.gen(function* () { const invalidate: PullRequestService["Service"]["invalidate"] = (input) => { const reference = input.reference; if (reference !== undefined) { - return Effect.sync(() => bumpRefEpoch(reference)); + return readCache.invalidate.pipe(Effect.andThen(Effect.sync(() => bumpRefEpoch(reference)))); } return Effect.sync(() => { listingsEpoch = ++epochCounter; @@ -2652,7 +2680,9 @@ export const make = Effect.gen(function* () { const refreshAfterTurn: PullRequestService["Service"]["refreshAfterTurn"] = Effect.suspend(() => { turnRefreshEpoch = listingsEpoch = ++epochCounter; - return SubscriptionRef.set(pullRequestRefreshes, turnRefreshEpoch); + return readCache.invalidate.pipe( + Effect.andThen(SubscriptionRef.set(pullRequestRefreshes, turnRefreshEpoch)), + ); }); // A mutation's own client re-reads right after it, and every other client's next read must @@ -2663,7 +2693,9 @@ export const make = Effect.gen(function* () { method: (input: I) => Effect.Effect, ): ((input: I) => Effect.Effect) => (input) => - method(input).pipe( + readCache.invalidate.pipe( + Effect.andThen(method(input)), + Effect.ensuring(readCache.invalidate), Effect.tap(() => Effect.sync(() => { bumpRefEpoch(input); @@ -2674,7 +2706,8 @@ export const make = Effect.gen(function* () { const runActionAndInvalidate: PullRequestService["Service"]["runAction"] = Effect.fn( "PullRequestService.runActionAndInvalidate", )(function* (input) { - const repository = yield* runAction(input); + yield* readCache.invalidate; + const repository = yield* runAction(input).pipe(Effect.ensuring(readCache.invalidate)); bumpRefEpoch({ ...input, repository }); listingsEpoch = ++epochCounter; if (input.action === "merge") { diff --git a/apps/server/src/server.ts b/apps/server/src/server.ts index c531cab63c8f..3831763a3eac 100644 --- a/apps/server/src/server.ts +++ b/apps/server/src/server.ts @@ -93,6 +93,7 @@ import * as VcsStatusBroadcaster from "./vcs/VcsStatusBroadcaster.ts"; import * as GitWorkflowService from "./git/GitWorkflowService.ts"; import * as ReviewService from "./review/ReviewService.ts"; import * as SourceControlProviderRegistry from "./sourceControl/SourceControlProviderRegistry.ts"; +import * as PullRequestReadCache from "./pullRequest/PullRequestReadCache.ts"; import * as SourceControlRateLimit from "./sourceControl/SourceControlRateLimit.ts"; import * as SourceControlRepositoryService from "./sourceControl/SourceControlRepositoryService.ts"; import * as ProjectSetupScriptRunner from "./project/ProjectSetupScriptRunner.ts"; @@ -318,6 +319,7 @@ const SourceControlProviderRegistryLayerLive = SourceControlProviderRegistry.lay const PullRequestServiceLive = PullRequestService.layer.pipe( Layer.provide(PullRequestProviderRegistry.layer), + Layer.provide(PullRequestReadCache.layer), Layer.provide(SourceControlProviderRegistryLayerLive), Layer.provide(SourceControlRateLimit.layer), );