From 393c055cab6dfa880cdbfb3fade6762514bc8f15 Mon Sep 17 00:00:00 2001 From: Matthew Riddell Date: Fri, 9 Oct 2026 19:25:39 -0500 Subject: [PATCH] fix(server): keep PR-status lookups for ten minutes, not one (fork patch, board #15036) The settlement and thread-PR sweeps run every minute and re-read every open PR from GitHub once their one-minute cache entries expire: 500 to 1,100 GraphQL calls an hour on the fleet box (listPullRequestsByHead from ThreadPullRequestServiceV2.synchronize and ThreadSettlementServiceV2.sweep, getPullRequestSummary from PullRequestReadCache). - GitManager: PR_LOOKUP_CACHE_TTL 60 s -> 10 min; the no-open-PR TTL 5 min -> 10 min. Refresh paths (turn end, push, create PR, user refresh) still bypass the cache; failure backoff is unchanged. - PullRequestReadCache: entries and scope invalidations last ENTRY_TTL (10 min) instead of 60 s, so an invalidation still outlives any entry read before it. PR badges and external merges can lag up to ten minutes. Matt, q3440 (Telegram 2026-10-10T00:17:56Z): "q3440: 1". --- apps/server/src/git/GitManager.test.ts | 104 +++++++++--------- apps/server/src/git/GitManager.ts | 22 ++-- .../pullRequest/PullRequestReadCache.test.ts | 12 +- .../src/pullRequest/PullRequestReadCache.ts | 14 ++- 4 files changed, 82 insertions(+), 70 deletions(-) diff --git a/apps/server/src/git/GitManager.test.ts b/apps/server/src/git/GitManager.test.ts index 9f5f368d2cf2..52b99e5582cc 100644 --- a/apps/server/src/git/GitManager.test.ts +++ b/apps/server/src/git/GitManager.test.ts @@ -1717,59 +1717,55 @@ it.layer(layerGitManagerTest)("GitManager", (it) => { }), ); - it.effect("branch PR lookup rechecks open PRs every minute and settled answers less often", () => - Effect.gen(function* () { - const repoDir = yield* makeTempDir("t3code-git-manager-"); - yield* initRepo(repoDir); - const remoteDir = yield* createBareRemote(); - yield* runGit(repoDir, ["remote", "add", "origin", remoteDir]); - for (const branch of ["feature/open-pr", "feature/merged-pr", "feature/no-pr"]) { - yield* runGit(repoDir, ["checkout", "-b", branch, "main"]); - yield* runGit(repoDir, ["push", "-u", "origin", branch]); - } - const pullRequest = (number: number, headRefName: string, state: string) => ({ - number, - title: headRefName, - url: `https://github.com/pingdotgg/codething-mvp/pull/${number}`, - baseRefName: "main", - headRefName, - state, - updatedAt: "2026-04-07T15:00:00Z", - }); - const { manager, ghCalls } = yield* makeManager({ - ghScenario: { - prListByHeadSelector: { - "feature/open-pr": encodeCliJson([pullRequest(301, "feature/open-pr", "OPEN")]), - "feature/merged-pr": encodeCliJson([pullRequest(302, "feature/merged-pr", "MERGED")]), + it.effect( + "branch PR lookup keeps open and settled answers for ten minutes (fork patch #15036)", + () => + Effect.gen(function* () { + const repoDir = yield* makeTempDir("t3code-git-manager-"); + yield* initRepo(repoDir); + const remoteDir = yield* createBareRemote(); + yield* runGit(repoDir, ["remote", "add", "origin", remoteDir]); + for (const branch of ["feature/open-pr", "feature/merged-pr", "feature/no-pr"]) { + yield* runGit(repoDir, ["checkout", "-b", branch, "main"]); + yield* runGit(repoDir, ["push", "-u", "origin", branch]); + } + const pullRequest = (number: number, headRefName: string, state: string) => ({ + number, + title: headRefName, + url: `https://github.com/pingdotgg/codething-mvp/pull/${number}`, + baseRefName: "main", + headRefName, + state, + updatedAt: "2026-04-07T15:00:00Z", + }); + const { manager, ghCalls } = yield* makeManager({ + ghScenario: { + prListByHeadSelector: { + "feature/open-pr": encodeCliJson([pullRequest(301, "feature/open-pr", "OPEN")]), + "feature/merged-pr": encodeCliJson([pullRequest(302, "feature/merged-pr", "MERGED")]), + }, }, - }, - }); - const lookupAll = Effect.forEach( - ["feature/open-pr", "feature/merged-pr", "feature/no-pr"], - (branch) => manager.branchPullRequest({ cwd: repoDir, branch }), - ); - const prListCalls = () => ghCalls.filter((call) => call.startsWith("pr list ")); - - yield* lookupAll; - expect(prListCalls()).toHaveLength(3); - - yield* TestClock.adjust("61 seconds"); - yield* lookupAll; - expect(prListCalls()).toHaveLength(4); - expect(prListCalls().at(-1)).toContain("--head feature/open-pr"); - - // Just inside the 5-minute window only the open PR is asked again. - yield* TestClock.adjust("238 seconds"); - yield* lookupAll; - expect(prListCalls()).toHaveLength(5); - expect(prListCalls().at(-1)).toContain("--head feature/open-pr"); - - // Just past it the settled answers expire too. - yield* TestClock.adjust("2 seconds"); - yield* lookupAll; - expect(prListCalls()).toHaveLength(7); - expect(prListCalls().slice(-2).join("\n")).not.toContain("--head feature/open-pr"); - }), + }); + const lookupAll = Effect.forEach( + ["feature/open-pr", "feature/merged-pr", "feature/no-pr"], + (branch) => manager.branchPullRequest({ cwd: repoDir, branch }), + ); + const prListCalls = () => ghCalls.filter((call) => call.startsWith("pr list ")); + + yield* lookupAll; + expect(prListCalls()).toHaveLength(3); + + // Just inside ten minutes nothing is asked again, not even the open PR. + yield* TestClock.adjust("599 seconds"); + yield* lookupAll; + expect(prListCalls()).toHaveLength(3); + + // Just past it all three answers expire. + yield* TestClock.adjust("2 seconds"); + yield* lookupAll; + expect(prListCalls()).toHaveLength(6); + expect(prListCalls().slice(-3).join("\n")).toContain("--head feature/open-pr"); + }), ); it.effect("branch PR lookup announces a pull request when it reads it merged", () => @@ -1811,8 +1807,8 @@ it.layer(layerGitManagerTest)("GitManager", (it) => { yield* lookup("feature/merges-later"); yield* lookup("feature/already-merged"); - // Open answers are re-read after a minute. - yield* TestClock.adjust("61 seconds"); + // Open answers are re-read after ten minutes (fork patch #15036). + yield* TestClock.adjust("601 seconds"); expect((yield* lookup("feature/merges-later"))?.state).toBe("merged"); const announced = yield* Stream.runCollect(Stream.take(changes, 2)); diff --git a/apps/server/src/git/GitManager.ts b/apps/server/src/git/GitManager.ts index 593910ba9a4a..0ef73f725acf 100644 --- a/apps/server/src/git/GitManager.ts +++ b/apps/server/src/git/GitManager.ts @@ -166,19 +166,19 @@ const SHORT_SHA_LENGTH = 7; const TOAST_DESCRIPTION_MAX = 72; const STATUS_RESULT_CACHE_TTL = Duration.seconds(1); const STATUS_RESULT_CACHE_CAPACITY = 2_048; -// Matches the automatic settlement sweep cadence so every background sweep -// reads fresh branch state: an external merge settles within about a minute -// instead of waiting out a longer cache. Unpublished branches never reach the -// host (a local probe answers first), and failed lookups still back off -// exponentially via prLookupFailureTtl, so throttling pressure still drops -// under 429s instead of amplifying it. -const PR_LOOKUP_CACHE_TTL = Duration.seconds(60); +// Fork patch (MattRiddell/t3code, board #15036): an open PR's lookup is kept +// for ten minutes, not one. Upstream matched the one-minute settlement sweep +// so an external merge settled within about a minute, but that re-asked +// GitHub for every open-PR branch every minute (500 to 1,100 GraphQL calls an +// hour on the fleet box). A PR badge or an external merge can now lag up to +// ten minutes. Git actions, turn ends and user refreshes still bypass this +// cache, and failed lookups still back off via prLookupFailureTtl. +const PR_LOOKUP_CACHE_TTL = Duration.minutes(10); // Answers without an open PR ("no PR yet", merged, closed) only change when // someone opens a PR, and the paths that do that in-app (turn end, push, -// create PR, user refresh) bypass this cache. Re-asking every minute for each -// idle branch was the bulk of background GitHub quota use, so these wait out -// a longer TTL and a PR opened outside the app shows up within minutes. -const PR_LOOKUP_NO_OPEN_PR_CACHE_TTL = Duration.minutes(5); +// create PR, user refresh) bypass this cache. They wait at least as long as +// an open PR's answer (fork patch, board #15036; upstream: five minutes). +const PR_LOOKUP_NO_OPEN_PR_CACHE_TTL = Duration.minutes(10); const PR_LOOKUP_FAILURE_BASE_TTL = Duration.seconds(20); const PR_LOOKUP_FAILURE_MAX_TTL = Duration.minutes(15); const PR_LOOKUP_CACHE_CAPACITY = 2_048; diff --git a/apps/server/src/pullRequest/PullRequestReadCache.test.ts b/apps/server/src/pullRequest/PullRequestReadCache.test.ts index f1b543d1eddf..61240583904a 100644 --- a/apps/server/src/pullRequest/PullRequestReadCache.test.ts +++ b/apps/server/src/pullRequest/PullRequestReadCache.test.ts @@ -105,6 +105,10 @@ it.layer(NodeServices.layer)("PR filesystem cache", (it) => { ), ); + it("keeps an entry for ten minutes (fork patch #15036)", () => { + assert.strictEqual(Duration.toMillis(PullRequestReadCache.ENTRY_TTL), 600_000); + }); + it.effect("reuses files after restart and respects the original expiry", () => Effect.gen(function* () { const fs = yield* FileSystem.FileSystem; @@ -114,7 +118,9 @@ it.layer(NodeServices.layer)("PR filesystem cache", (it) => { 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"); + yield* TestClock.adjust( + Duration.subtract(PullRequestReadCache.ENTRY_TTL, Duration.seconds(1)), + ); const restarted = yield* cacheLayer(directory); assert.strictEqual(yield* restarted.get(key, lookup), "1"); yield* TestClock.adjust("1 second"); @@ -228,7 +234,9 @@ it.layer(NodeServices.layer)("PR filesystem cache", (it) => { for (let index = 0; index < 100; index++) yield* cache.invalidate(`pr-${index}`); assert.strictEqual((yield* fs.readDirectory(directory)).length, 2); const before = (yield* fs.stat(`${directory}/revisions`)).size; - yield* TestClock.adjust("59 seconds"); + yield* TestClock.adjust( + Duration.subtract(PullRequestReadCache.ENTRY_TTL, Duration.seconds(1)), + ); assert.strictEqual(yield* cache.get("summary", Effect.succeed("fresh"), ["pr"]), "fresh"); yield* TestClock.adjust("1 second"); yield* cache.invalidate("other-pr"); diff --git a/apps/server/src/pullRequest/PullRequestReadCache.ts b/apps/server/src/pullRequest/PullRequestReadCache.ts index a749a14031fd..474cbc4955ba 100644 --- a/apps/server/src/pullRequest/PullRequestReadCache.ts +++ b/apps/server/src/pullRequest/PullRequestReadCache.ts @@ -30,11 +30,19 @@ const STORE_ID = "pr-v2"; // An entry file is the store id followed by the SHA-256 hex digest of its key. const ENTRY_FILE_NAME = new RegExp(`^${STORE_ID}[0-9a-f]{64}$`); /** - * Entries expire a minute after they are written, and a write sets the file's + * Entries expire ENTRY_TTL after they are written, and a write sets the file's * mtime. A file untouched for a day is long expired; the day only leaves slack * for clock changes. Pruning a live entry would cost one refetch. */ export const ENTRY_FILE_MAX_AGE = Duration.days(1); +/** + * Fork patch (MattRiddell/t3code, board #15036): a pull request read is kept + * for ten minutes, not one, so the settlement and sync sweeps stop re-reading + * every PR from GitHub each minute. A scope invalidation lasts as long as an + * entry, so an entry read before the invalidation can never outlive it. + */ +export const ENTRY_TTL = Duration.minutes(10); +const ENTRY_TTL_MILLIS = Duration.toMillis(ENTRY_TTL); type ReadError = PullRequestOperationError | PullRequestUnavailableError; const revisionCodec = Schema.fromJsonString( Schema.Record( @@ -107,7 +115,7 @@ export const make = Effect.gen(function* () { request.lookup.pipe( Effect.map((payload) => ({ payload, - expiresAt: clock.currentTimeMillisUnsafe() + 60_000, + expiresAt: clock.currentTimeMillisUnsafe() + ENTRY_TTL_MILLIS, revision: request.revision, })), ), @@ -162,7 +170,7 @@ export const make = Effect.gen(function* () { const next = Object.fromEntries( Object.entries(current).filter(([, value]) => value.expiresAt > now), ); - next[scope] = { revision: yield* crypto.randomUUIDv4, expiresAt: now + 60_000 }; + next[scope] = { revision: yield* crypto.randomUUIDv4, expiresAt: now + ENTRY_TTL_MILLIS }; const encoded = yield* Schema.encodeEffect(revisionCodec)(next); yield* backing.set("revisions", encoded); yield* Cache.set(revisions, undefined, next);