Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 96 additions & 0 deletions apps/server/src/sourceControl/GitHubCli.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
27 changes: 25 additions & 2 deletions apps/server/src/sourceControl/GitHubCli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, string>();

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",
Expand Down Expand Up @@ -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,
}),
),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium sourceControl/GitHubCli.ts:512

probeCwds retains every host\0credentialFingerprint and cwd indefinitely, so long-lived servers accumulate entries even though quota is bounded to 32 cached probes. Remove the mapping when Cache.get completes, including failed probes, so the auxiliary map does not grow without bound.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/sourceControl/GitHubCli.ts around line 512:

`probeCwds` retains every `host\0credentialFingerprint` and `cwd` indefinitely, so long-lived servers accumulate entries even though `quota` is bounded to 32 cached probes. Remove the mapping when `Cache.get` completes, including failed probes, so the auxiliary map does not grow without bound.

);
yield* budget.query(host, "query {}", allowReserve ? { allowReserve: true } : undefined);
return yield* executeRaw(input);
}).pipe(
Expand Down
Loading