authorgravatar for me@t3.ggTheo Browne <me@t3.gg> 2026-08-07 21:09:46-04:00
committergravatar for noreply@github.comGitHub <noreply@github.com> 2026-08-07 18:09:46-07:00
log4eaf5ef8bb47b870397d5c61cd216b1a6bdd1510
treed079e362ae70cc76514cb1efd39d603938c548d2
parent0de9540739369301d5056870deeee3f258a03b14
signature Signed by PGP key B5690EEEBB952194

fix(server): stop PR status lookups amplifying GitHub rate limits (#5673)

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

2 files changed, 157 insertions(+), 9 deletions(-)

apps/server/src/git/GitManager.test.ts+68
...@@ -5,6 +5,7 @@ import * as NodeChildProcess from "node:child_process";...@@ -5,6 +5,7 @@ import * as NodeChildProcess from "node:child_process";
55
6import * as NodeServices from "@effect/platform-node/NodeServices";6import * as NodeServices from "@effect/platform-node/NodeServices";
7import { it } from "@effect/vitest";7import { it } from "@effect/vitest";
8import * as Duration from "effect/Duration";
8import * as Effect from "effect/Effect";9import * as Effect from "effect/Effect";
9import * as FileSystem from "effect/FileSystem";10import * as FileSystem from "effect/FileSystem";
10import * as Layer from "effect/Layer";11import * as Layer from "effect/Layer";
...@@ -948,6 +949,73 @@ it.layer(GitManagerTestLayer)("GitManager", (it) => {...@@ -948,6 +949,73 @@ it.layer(GitManagerTestLayer)("GitManager", (it) => {
948 }),949 }),
949 );950 );
950951
952 it.effect("status skips the provider lookup for a branch that was never pushed", () =>
953 Effect.gen(function* () {
954 const repoDir = yield* makeTempDir("t3code-git-manager-");
955 yield* initRepo(repoDir);
956 const remoteDir = yield* createBareRemote();
957 yield* runGit(repoDir, ["remote", "add", "origin", remoteDir]);
958 yield* runGit(repoDir, ["push", "-u", "origin", "main"]);
959 yield* runGit(repoDir, ["checkout", "-b", "feature/never-pushed"]);
960
961 const { manager, ghCalls } = yield* makeManager();
962
963 const status = yield* manager.status({ cwd: repoDir });
964
965 expect(status.refName).toBe("feature/never-pushed");
966 expect(status.pr).toBeNull();
967 expect(ghCalls.filter((call) => call.startsWith("pr list "))).toHaveLength(0);
968 }),
969 );
970
971 it.effect("status still looks up PRs for a branch pushed without --set-upstream", () =>
972 Effect.gen(function* () {
973 const repoDir = yield* makeTempDir("t3code-git-manager-");
974 yield* initRepo(repoDir);
975 const remoteDir = yield* createBareRemote();
976 yield* runGit(repoDir, ["remote", "add", "origin", remoteDir]);
977 yield* runGit(repoDir, ["push", "-u", "origin", "main"]);
978 yield* runGit(repoDir, ["checkout", "-b", "feature/pushed-no-upstream"]);
979 // No `-u`, so the remote-tracking ref exists but branch.<name>.merge does
980 // not. Most terminal and agent pushes land this way, and they can still
981 // have a PR, so the skip must not trigger here.
982 yield* runGit(repoDir, ["push", "origin", "feature/pushed-no-upstream"]);
983
984 const { manager, ghCalls } = yield* makeManager({
985 ghScenario: {
986 prListSequence: [
987 // @effect-diagnostics-next-line preferSchemaOverJson:off
988 JSON.stringify([
989 {
990 number: 214,
991 title: "Pushed without upstream",
992 url: "https://github.com/pingdotgg/t3code/pull/214",
993 baseRefName: "main",
994 headRefName: "feature/pushed-no-upstream",
995 state: "OPEN",
996 updatedAt: "2026-04-01T15:00:00Z",
997 },
998 ]),
999 ],
1000 },
1001 });
1002
1003 const status = yield* manager.status({ cwd: repoDir });
1004
1005 expect(status.pr?.number).toBe(214);
1006 expect(ghCalls.filter((call) => call.startsWith("pr list ")).length).toBeGreaterThan(0);
1007 }),
1008 );
1009
1010 it("backs off repeated PR lookup failures past the healthy refresh cadence", () => {
1011 expect(Duration.toMillis(GitManager.prLookupFailureTtl(1))).toBe(20_000);
1012 expect(Duration.toMillis(GitManager.prLookupFailureTtl(2))).toBe(40_000);
1013 // The point of the backoff: by the third retry a failing branch must not be
1014 // asking more often than a healthy one, which refreshes every 2 minutes.
1015 expect(Duration.toMillis(GitManager.prLookupFailureTtl(4))).toBeGreaterThan(120_000);
1016 expect(Duration.toMillis(GitManager.prLookupFailureTtl(20))).toBe(900_000);
1017 });
1018
951 it.effect(1019 it.effect(
952 "status ignores unrelated fork PRs when the current branch tracks the same repository",1020 "status ignores unrelated fork PRs when the current branch tracks the same repository",
953 () =>1021 () =>
apps/server/src/git/GitManager.ts+89-9
...@@ -110,8 +110,25 @@ const TOAST_DESCRIPTION_MAX = 72;...@@ -110,8 +110,25 @@ const TOAST_DESCRIPTION_MAX = 72;
110const STATUS_RESULT_CACHE_TTL = Duration.seconds(1);110const STATUS_RESULT_CACHE_TTL = Duration.seconds(1);
111const STATUS_RESULT_CACHE_CAPACITY = 2_048;111const STATUS_RESULT_CACHE_CAPACITY = 2_048;
112const PR_LOOKUP_CACHE_TTL = Duration.minutes(2);112const PR_LOOKUP_CACHE_TTL = Duration.minutes(2);
113const PR_LOOKUP_FAILURE_TTL = Duration.seconds(20);113const PR_LOOKUP_FAILURE_BASE_TTL = Duration.seconds(20);
114const PR_LOOKUP_FAILURE_MAX_TTL = Duration.minutes(15);
114const PR_LOOKUP_CACHE_CAPACITY = 2_048;115const PR_LOOKUP_CACHE_CAPACITY = 2_048;
116
117/**
118 * How long a failed PR lookup is cached, given the number of consecutive
119 * failures for that branch.
120 *
121 * A hosting provider rejects a throttled request immediately, so caching every
122 * failure for a flat 20s made a rate-limited poller re-ask *faster* than a
123 * healthy one does (which waits PR_LOOKUP_CACHE_TTL), turning a transient 429
124 * into sustained pressure. Backing off per branch keeps the retry rate below
125 * the healthy rate once a branch has failed more than a couple of times.
126 */
127export function prLookupFailureTtl(consecutiveFailures: number): Duration.Duration {
128 const exponent = Math.max(0, consecutiveFailures - 1);
129 const backoffMs = Duration.toMillis(PR_LOOKUP_FAILURE_BASE_TTL) * Math.pow(2, exponent);
130 return Duration.min(Duration.millis(backoffMs), PR_LOOKUP_FAILURE_MAX_TTL);
131}
115type StripProgressContext<T> = T extends any ? Omit<T, "actionId" | "cwd" | "action"> : never;132type StripProgressContext<T> = T extends any ? Omit<T, "actionId" | "cwd" | "action"> : never;
116type GitActionProgressPayload = StripProgressContext<GitActionProgressEvent>;133type GitActionProgressPayload = StripProgressContext<GitActionProgressEvent>;
117type GitActionProgressEmitter = (event: GitActionProgressPayload) => Effect.Effect<void, never>;134type GitActionProgressEmitter = (event: GitActionProgressPayload) => Effect.Effect<void, never>;
...@@ -889,6 +906,23 @@ export const make = Effect.gen(function* () {...@@ -889,6 +906,23 @@ export const make = Effect.gen(function* () {
889 // back to a null upstreamRef.906 // back to a null upstreamRef.
890 const prLookupCacheKey = (cwd: string, details: { branch: string; upstreamRef: string | null }) =>907 const prLookupCacheKey = (cwd: string, details: { branch: string; upstreamRef: string | null }) =>
891 [cwd, details.branch, details.upstreamRef ?? "", String(prLookupEpoch(cwd))].join("\u0000");908 [cwd, details.branch, details.upstreamRef ?? "", String(prLookupEpoch(cwd))].join("\u0000");
909 // Consecutive failures per cache key, so a branch that keeps failing waits
910 // longer before the next attempt. Cleared as soon as a lookup succeeds.
911 const prLookupFailureStreakByKey = new Map<string, number>();
912 const nextPrLookupFailureTtl = (key: string) => {
913 if (
914 !prLookupFailureStreakByKey.has(key) &&
915 prLookupFailureStreakByKey.size >= PR_LOOKUP_CACHE_CAPACITY
916 ) {
917 const oldestKey = prLookupFailureStreakByKey.keys().next().value;
918 if (oldestKey !== undefined) {
919 prLookupFailureStreakByKey.delete(oldestKey);
920 }
921 }
922 const streak = (prLookupFailureStreakByKey.get(key) ?? 0) + 1;
923 prLookupFailureStreakByKey.set(key, streak);
924 return prLookupFailureTtl(streak);
925 };
892 const prLookupCache = yield* Cache.makeWith(926 const prLookupCache = yield* Cache.makeWith(
893 (key: string) => {927 (key: string) => {
894 const [cwd = "", branch = "", upstreamRef = ""] = key.split("\u0000");928 const [cwd = "", branch = "", upstreamRef = ""] = key.split("\u0000");
...@@ -896,17 +930,26 @@ export const make = Effect.gen(function* () {...@@ -896,17 +930,26 @@ export const make = Effect.gen(function* () {
896 branch,930 branch,
897 upstreamRef: upstreamRef.length > 0 ? upstreamRef : null,931 upstreamRef: upstreamRef.length > 0 ? upstreamRef : null,
898 };932 };
899 return resolveBranchHeadContext(cwd, details).pipe(933 return Effect.gen(function* () {
900 Effect.flatMap((headContext) =>934 const headContext = yield* resolveBranchHeadContext(cwd, details);
901 findLatestPrForHeadContext(cwd, headContext).pipe(935 // Only skip when the branch is untracked as well: anything carrying an
902 Effect.map((latest) => ({ latest, headContext })),936 // upstream keeps the old behaviour.
903 ),937 if (details.upstreamRef === null && (yield* isUnpublishedBranch(cwd, headContext))) {
904 ),938 return { latest: null, headContext };
905 );939 }
940 const latest = yield* findLatestPrForHeadContext(cwd, headContext);
941 return { latest, headContext };
942 });
906 },943 },
907 {944 {
908 capacity: PR_LOOKUP_CACHE_CAPACITY,945 capacity: PR_LOOKUP_CACHE_CAPACITY,
909 timeToLive: (exit) => (Exit.isSuccess(exit) ? PR_LOOKUP_CACHE_TTL : PR_LOOKUP_FAILURE_TTL),946 timeToLive: (exit, key) => {
947 if (Exit.isSuccess(exit)) {
948 prLookupFailureStreakByKey.delete(key);
949 return PR_LOOKUP_CACHE_TTL;
950 }
951 return nextPrLookupFailureTtl(key);
952 },
910 },953 },
911 );954 );
912 // A transient lookup failure (rate limit, network blip) must not clear an955 // A transient lookup failure (rate limit, network blip) must not clear an
...@@ -1169,6 +1212,43 @@ export const make = Effect.gen(function* () {...@@ -1169,6 +1212,43 @@ export const make = Effect.gen(function* () {
1169 } satisfies BranchHeadContext;1212 } satisfies BranchHeadContext;
1170 });1213 });
11711214
1215 /**
1216 * Whether git has no record of this branch on any remote, so a change request
1217 * cannot exist for it and asking the provider is a guaranteed-empty API call.
1218 *
1219 * `git push` writes the remote-tracking ref even without `-u` (how most
1220 * terminal and agent pushes land), which makes this a safer "did it ever
1221 * reach the host" test than looking for upstream config, and the glob spans
1222 * every remote so a fork branch still counts. A repository that tracks no
1223 * remotes at all cannot answer the question, because then every branch looks
1224 * unpublished; it, and any failed probe, keeps the lookup.
1225 */
1226 const isUnpublishedBranch = Effect.fn("isUnpublishedBranch")(function* (
1227 cwd: string,
1228 headContext: Pick<BranchHeadContext, "headBranch">,
1229 ) {
1230 if (headContext.headBranch.length === 0) {
1231 return false;
1232 }
1233 const matchesRef = (pattern: string) =>
1234 gitCore
1235 .execute({
1236 operation: "GitManager.isUnpublishedBranch",
1237 cwd,
1238 args: ["for-each-ref", "--count=1", "--format=%(refname)", pattern],
1239 timeoutMs: 5_000,
1240 })
1241 .pipe(Effect.map((result) => result.stdout.trim().length > 0));
1242
1243 return yield* Effect.all(
1244 [matchesRef("refs/remotes"), matchesRef(`refs/remotes/*/${headContext.headBranch}`)],
1245 { concurrency: "unbounded" },
1246 ).pipe(
1247 Effect.map(([tracksAnyRemote, tracksThisBranch]) => tracksAnyRemote && !tracksThisBranch),
1248 Effect.orElseSucceed(() => false),
1249 );
1250 });
1251
1172 const findOpenPr = Effect.fn("findOpenPr")(function* (1252 const findOpenPr = Effect.fn("findOpenPr")(function* (
1173 cwd: string,1253 cwd: string,
1174 headContext: Pick<1254 headContext: Pick<