From 6f168e3bff9c7d91663f0bbb27d355495101bc6e Mon Sep 17 00:00:00 2001 From: ArticOdin <9603319+Artic0din@users.noreply.github.com> Date: Thu, 17 Sep 2026 20:22:11 +1000 Subject: [PATCH 1/3] fix(server): stop GitHub quota probe cwd/failure from blocking PR reads GitHubCli.execute's rate-limit preflight probe ran `gh api rate_limit` with `globalThis.process.cwd()` instead of the caller's cwd. On packaged desktop that is $HOME (apps/desktop sets backendCwd to the user's home directory), so the probe could fail there even when the actual read (pr view/pr list/repo view) had the correct project cwd and would otherwise succeed. Worse, a failed probe was never cached (`timeToLive` is zero on failure) and blocked the guarded read outright, so every retry of a sweep like PullRequestSyncReactor's 1-minute pass re-failed the probe and the real gh command never ran. Pass the caller's cwd into the probe, and treat a probe failure as an unknown-budget courtesy check rather than a precondition: log a warning and proceed to the actual read, which still passes through budget.query for a known-exhausted budget. Co-Authored-By: Claude Sonnet 5 --- .../src/sourceControl/GitHubCli.test.ts | 44 +++++++++++++++++++ apps/server/src/sourceControl/GitHubCli.ts | 19 ++++++-- 2 files changed, 60 insertions(+), 3 deletions(-) diff --git a/apps/server/src/sourceControl/GitHubCli.test.ts b/apps/server/src/sourceControl/GitHubCli.test.ts index 5893c21ff772..eb4633e6bd40 100644 --- a/apps/server/src/sourceControl/GitHubCli.test.ts +++ b/apps/server/src/sourceControl/GitHubCli.test.ts @@ -109,6 +109,50 @@ 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("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("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..037d3e2ea4d4 100644 --- a/apps/server/src/sourceControl/GitHubCli.ts +++ b/apps/server/src/sourceControl/GitHubCli.ts @@ -440,9 +440,9 @@ export const make = Effect.gen(function* () { const quota = yield* Cache.makeWith( (key: string) => { - const host = key.split("\0")[0]!; + const [host, , cwd] = key.split("\0") as [string, string, string]; return executeRaw({ - cwd: globalThis.process.cwd(), + cwd, args: [ "api", "rate_limit", @@ -487,7 +487,20 @@ 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. + yield* Cache.get( + quota, + `${host}\0${credential?.credentialFingerprint ?? ""}\0${input.cwd}`, + ).pipe( + Effect.catch((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( From 739cb4abfd6bebee43b8f45600d43ceec14b4c95 Mon Sep 17 00:00:00 2001 From: ArticOdin <9603319+Artic0din@users.noreply.github.com> Date: Mon, 21 Sep 2026 22:06:34 +1000 Subject: [PATCH 2/3] fix(server): keep the GitHub quota probe cache host-scoped Folding the caller's cwd into the quota cache key made the probe per-project rather than per-host. The budget it feeds is keyed on host plus credential scope (githubGraphQlBudget), so the extra entries bought no correctness and cost one `gh api rate_limit` subprocess per project per sweep: PullRequestSyncReactor.syncGroup fans out per projectId, and the 30s TTL expires inside the 60s sweep. The 32-entry capacity compounded it with eviction churn once a user had more projects than that. Keep the key as host plus credential fingerprint and record the newest caller's cwd alongside it, so a refresh still spawns `gh` from a real project directory while one probe serves every project on that host. A stale cwd is bounded by the 30s TTL and is harmless now that a probe failure no longer blocks the read. Add the regression test the existing coverage could not catch: two projects on one host must share a single probe. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/sourceControl/GitHubCli.test.ts | 23 +++++++++++++++++++ apps/server/src/sourceControl/GitHubCli.ts | 17 +++++++++----- 2 files changed, 34 insertions(+), 6 deletions(-) diff --git a/apps/server/src/sourceControl/GitHubCli.test.ts b/apps/server/src/sourceControl/GitHubCli.test.ts index eb4633e6bd40..5b68b6fc8d1d 100644 --- a/apps/server/src/sourceControl/GitHubCli.test.ts +++ b/apps/server/src/sourceControl/GitHubCli.test.ts @@ -129,6 +129,29 @@ describe("GitHubCli.layer", () => { }).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( diff --git a/apps/server/src/sourceControl/GitHubCli.ts b/apps/server/src/sourceControl/GitHubCli.ts index 037d3e2ea4d4..cb63e156341d 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, , cwd] = key.split("\0") as [string, string, string]; + const host = key.split("\0")[0]!; return executeRaw({ - cwd, + cwd: probeCwds.get(key) ?? globalThis.process.cwd(), args: [ "api", "rate_limit", @@ -490,10 +496,9 @@ export const make = Effect.gen(function* () { // 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. - yield* Cache.get( - quota, - `${host}\0${credential?.credentialFingerprint ?? ""}\0${input.cwd}`, - ).pipe( + const quotaKey = `${host}\0${credential?.credentialFingerprint ?? ""}`; + probeCwds.set(quotaKey, input.cwd); + yield* Cache.get(quota, quotaKey).pipe( Effect.catch((error) => Effect.logWarning("GitHub API quota probe failed; proceeding without it", { host, From 78c3e10e927d6499ed480da5eca9b4b0dcc1a6a6 Mon Sep 17 00:00:00 2001 From: ArticOdin <9603319+Artic0din@users.noreply.github.com> Date: Mon, 21 Sep 2026 22:19:25 +1000 Subject: [PATCH 3/3] fix(server): let a rate-limited quota probe still pause the limiter Failing the probe open caught every error, including the one failure that is the probe's whole point. A `rate-limited` exit maps to GitHubCliRateLimitError, which previously reached the tapError below and recorded the limit against the lease; swallowing it meant the guarded read ran anyway, rediscovered the same 403, and only then paused the host. Recover from everything except that tag, so a dead probe stays a courtesy check while a rate-limited one still reports what it found. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/sourceControl/GitHubCli.test.ts | 29 +++++++++++++++++++ apps/server/src/sourceControl/GitHubCli.ts | 17 +++++++---- 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/apps/server/src/sourceControl/GitHubCli.test.ts b/apps/server/src/sourceControl/GitHubCli.test.ts index 5b68b6fc8d1d..787d644854ef 100644 --- a/apps/server/src/sourceControl/GitHubCli.test.ts +++ b/apps/server/src/sourceControl/GitHubCli.test.ts @@ -176,6 +176,35 @@ describe("GitHubCli.layer", () => { }).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 cb63e156341d..93b040cbdc29 100644 --- a/apps/server/src/sourceControl/GitHubCli.ts +++ b/apps/server/src/sourceControl/GitHubCli.ts @@ -495,15 +495,20 @@ export const make = Effect.gen(function* () { return yield* Effect.gen(function* () { // 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. + // 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.catch((error) => - Effect.logWarning("GitHub API quota probe failed; proceeding without it", { - host, - error, - }), + 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);