diff --git a/apps/server/src/sourceControl/GitHubCli.test.ts b/apps/server/src/sourceControl/GitHubCli.test.ts index 5893c21ff772..787d644854ef 100644 --- a/apps/server/src/sourceControl/GitHubCli.test.ts +++ b/apps/server/src/sourceControl/GitHubCli.test.ts @@ -109,6 +109,102 @@ describe("GitHubCli.layer", () => { }).pipe(Effect.provide(layer.pipe(Layer.provide(GitHubGraphQlBudget.layer)))), ); + it.effect("probes quota using the caller's cwd, not the process cwd", () => + Effect.gen(function* () { + const probeCwds: string[] = []; + const gh = yield* GitHubCli.make.pipe( + Effect.provideService(VcsProcess.VcsProcess, { + run: (input) => + Effect.sync(() => { + if (input.args[1] === "rate_limit") { + probeCwds.push(input.cwd); + return quotaOutput(); + } + return processOutput("[]"); + }), + }), + ); + yield* gh.execute({ cwd: "/repo/project-a", args: ["pr", "list"] }); + assert.deepStrictEqual(probeCwds, ["/repo/project-a"]); + }).pipe(Effect.provide(Layer.merge(GitHubGraphQlBudget.layer, SourceControlRateLimit.layer))), + ); + + it.effect("shares one host-scoped probe across projects on the same host", () => + Effect.gen(function* () { + const probeCwds: string[] = []; + const gh = yield* GitHubCli.make.pipe( + Effect.provideService(VcsProcess.VcsProcess, { + run: (input) => + Effect.sync(() => { + if (input.args[1] === "rate_limit") { + probeCwds.push(input.cwd); + return quotaOutput(); + } + return processOutput("[]"); + }), + }), + ); + yield* gh.execute({ cwd: "/repo/project-a", args: ["pr", "list"] }); + yield* gh.execute({ cwd: "/repo/project-b", args: ["pr", "list"] }); + // The budget the probe feeds is host+credential scoped, so a second project must reuse + // the cached probe instead of spawning its own `gh api rate_limit`. + assert.deepStrictEqual(probeCwds, ["/repo/project-a"]); + }).pipe(Effect.provide(Layer.merge(GitHubGraphQlBudget.layer, SourceControlRateLimit.layer))), + ); + + it.effect("does not block the read when the quota probe itself fails", () => + Effect.gen(function* () { + const gh = yield* GitHubCli.make.pipe( + Effect.provideService(VcsProcess.VcsProcess, { + run: (input) => + input.args[1] === "rate_limit" + ? Effect.fail( + new VcsProcessExitError({ + operation: "GitHubCli.execute", + command: "gh", + cwd: input.cwd, + exitCode: 1, + failureKind: "command-failed", + detail: "Process exited with a non-zero status.", + }), + ) + : Effect.succeed(processOutput("[]")), + }), + ); + const result = yield* gh.execute({ cwd: "/repo", args: ["pr", "list"] }); + assert.strictEqual(result.stdout, "[]"); + }).pipe(Effect.provide(Layer.merge(GitHubGraphQlBudget.layer, SourceControlRateLimit.layer))), + ); + + it.effect("surfaces a rate-limited probe instead of spending the read on the same 403", () => + Effect.gen(function* () { + const commands: string[] = []; + const gh = yield* GitHubCli.make.pipe( + Effect.provideService(VcsProcess.VcsProcess, { + run: (input) => + input.args[1] === "rate_limit" + ? Effect.fail( + new VcsProcessExitError({ + operation: "GitHubCli.execute", + command: "gh", + cwd: input.cwd, + exitCode: 1, + failureKind: "rate-limited", + detail: "Process exited with a non-zero status.", + }), + ) + : Effect.sync(() => { + commands.push(input.args.slice(0, 2).join(" ")); + return processOutput("[]"); + }), + }), + ); + const error = yield* gh.execute({ cwd: "/repo", args: ["pr", "list"] }).pipe(Effect.flip); + assert.strictEqual(error._tag, "GitHubCliRateLimitError"); + assert.deepStrictEqual(commands, []); + }).pipe(Effect.provide(Layer.merge(GitHubGraphQlBudget.layer, SourceControlRateLimit.layer))), + ); + it.effect("keeps quota snapshots separate for verified credentials on the same host", () => Effect.gen(function* () { let reads = 0; diff --git a/apps/server/src/sourceControl/GitHubCli.ts b/apps/server/src/sourceControl/GitHubCli.ts index c525740efeae..93b040cbdc29 100644 --- a/apps/server/src/sourceControl/GitHubCli.ts +++ b/apps/server/src/sourceControl/GitHubCli.ts @@ -438,11 +438,17 @@ export const make = Effect.gen(function* () { }, ); + // The probe's result is host+credential scoped (see `githubGraphQlBudget`), so the cache key + // stays host+credential scoped too and one probe serves every project on that host. The `cwd` + // is only needed when a refresh actually spawns `gh`, so the newest caller's one is recorded + // here rather than folded into the key, which would cost one probe per project per sweep. + const probeCwds = new Map(); + const quota = yield* Cache.makeWith( (key: string) => { const host = key.split("\0")[0]!; return executeRaw({ - cwd: globalThis.process.cwd(), + cwd: probeCwds.get(key) ?? globalThis.process.cwd(), args: [ "api", "rate_limit", @@ -487,7 +493,24 @@ export const make = Effect.gen(function* () { const guarded = Effect.gen(function* () { const lease = yield* limits.check(key, allowReserve ? { allowPaused: true } : undefined); return yield* Effect.gen(function* () { - yield* Cache.get(quota, `${host}\0${credential?.credentialFingerprint ?? ""}`); + // A quota probe is a courtesy check, not a precondition: its own transport or + // command failure must not block the read it is guarding. `budget.query` right + // after it is what actually enforces a known-exhausted budget. A rate-limited + // probe is the exception, since that failure is the very signal the probe exists + // to report: re-raise it so the `tapError` below records it against the lease + // instead of spending the guarded read to rediscover the same 403. + const quotaKey = `${host}\0${credential?.credentialFingerprint ?? ""}`; + probeCwds.set(quotaKey, input.cwd); + yield* Cache.get(quota, quotaKey).pipe( + Effect.catchIf( + (error) => error._tag !== "GitHubCliRateLimitError", + (error) => + Effect.logWarning("GitHub API quota probe failed; proceeding without it", { + host, + error, + }), + ), + ); yield* budget.query(host, "query {}", allowReserve ? { allowReserve: true } : undefined); return yield* executeRaw(input); }).pipe(