From 9ec03540d3b60d8576782076189c5cdf7cd91592 Mon Sep 17 00:00:00 2001 From: akshatcoder-hash Date: Sat, 31 Oct 2026 12:17:04 +0700 Subject: [PATCH] fix(host-service): bound gh fan-out and drop queued calls when the gate opens Every PR refresh fanned out over every project, branch, and PR detail at once, so a host with ~100 projects started hundreds of `gh` children in the same tick. Each child then raced its 10s execFile deadline, and `isGitHubUnreachableError` counted every one of those kills as GitHub being down. The gate opened a 60s hold and logged "GitHub unreachable"; a success from the same burst closed it again, so the warning flickered and the recovery sweep started the next burst. Fixes #8998. Spawns now go through a 4-slot FIFO around the injected `execGh`, under `ConditionalGh`, so a queued call does not spend its timeout while it waits. An exec kill moves to `isGitHubTimeoutError` and no longer opens the gate on its own; three kills with no answer between still do, and any classified answer (auth, rate limit, transport) restarts that count. Octokit is still skipped for a kill, since it would add work to the same overload. Real transport failures keep opening the hold through their codes and gh's stderr. The gate is checked before a call queues, so opening a hold now drains the queue; the limiter reports each failure to the gate before it hands the slot on, so the drain lands ahead of the next spawn. Explicit probes are tagged in that queue: a hold drops background waiters but keeps a probe in place, so a user refresh still gets its turn and can reopen the gate early, without bypassing the slots. Proved with `bun test packages/host-service/src/runtime/pull-requests` (122 pass): 10 concurrent sweeps peak at 4 in-flight spawns; six lookups queued behind four held slots leave the spawn count at 4 when one of them fails and opens the hold (10 without the drain, 5 if the slot moves before the gate sees the failure); three probes queued behind a sweep all run across two holds; a repeat kill after a rate-limit answer opens nothing; one SIGTERM kill leaves `getGitHubStatus()` null while a third opens the hold; an `ENOTFOUND` opens it immediately. --- .../pull-requests/pull-requests.test.ts | 244 +++++++++++++++++- .../runtime/pull-requests/pull-requests.ts | 55 +++- .../gh-concurrency/gh-concurrency.test.ts | 92 +++++++ .../utils/gh-concurrency/gh-concurrency.ts | 84 ++++++ .../utils/gh-concurrency/index.ts | 1 + .../github-availability.test.ts | 64 ++++- .../github-availability.ts | 43 ++- .../utils/github-availability/index.ts | 1 + 8 files changed, 554 insertions(+), 30 deletions(-) create mode 100644 packages/host-service/src/runtime/pull-requests/utils/gh-concurrency/gh-concurrency.test.ts create mode 100644 packages/host-service/src/runtime/pull-requests/utils/gh-concurrency/gh-concurrency.ts create mode 100644 packages/host-service/src/runtime/pull-requests/utils/gh-concurrency/index.ts diff --git a/packages/host-service/src/runtime/pull-requests/pull-requests.test.ts b/packages/host-service/src/runtime/pull-requests/pull-requests.test.ts index 4bc80d615f8..ac5ab39d9e9 100644 --- a/packages/host-service/src/runtime/pull-requests/pull-requests.test.ts +++ b/packages/host-service/src/runtime/pull-requests/pull-requests.test.ts @@ -11,6 +11,7 @@ import * as schema from "../../db/schema"; import { pullRequests, workspaces } from "../../db/schema"; import type { WorkspaceChangedMessage } from "../../events/types"; import { PullRequestRuntimeManager } from "./pull-requests"; +import { GH_CALL_CONCURRENCY } from "./utils/gh-concurrency"; import { GitHubAvailabilityGate } from "./utils/github-availability"; import type { WorkspaceRefsSnapshot } from "./utils/workspace-refs"; @@ -240,6 +241,13 @@ async function withSilencedWarnings(fn: () => Promise): Promise { } } +// Lets every resume a released slot triggers run before the next assertion. +const tick = () => { + const { promise, resolve } = Promise.withResolvers(); + setImmediate(resolve); + return promise; +}; + describe("PullRequestRuntimeManager direct checkout PR linking", () => { test("links a fork PR workspace to the selected PR and records fork upstream", async () => { const db = createRealDb(); @@ -1006,7 +1014,9 @@ describe("PullRequestRuntimeManager refresh", () => { fetchOpenPrs(repoOf(name)).catch(() => {}), ), ); - expect(ghAttempts).toBe(6); + // Six lookups pass the gate; the hold opened on the first failure + // drops the two still queued behind the slots. + expect(ghAttempts).toBe(4); expect( warns.filter((w) => w.includes("GitHub unreachable")), ).toHaveLength(1); @@ -1014,13 +1024,242 @@ describe("PullRequestRuntimeManager refresh", () => { // One outage, one streak: the hold is the base minute, not the cap. setSystemTime(new Date(t0 + 61_000)); await fetchOpenPrs(repoOf("g")).catch(() => {}); - expect(ghAttempts).toBe(7); + expect(ghAttempts).toBe(5); } finally { console.warn = originalWarn; setSystemTime(); } }); + test("a gh timeout does not open the unreachable hold, a transport failure does", async () => { + const db = createRealDb(); + seedProject(db); + let ghAttempts = 0; + let timeout = true; + const manager = createManager(db, { + execGh: async () => { + ghAttempts += 1; + throw timeout + ? Object.assign(new Error("Command failed"), { + killed: true, + signal: "SIGTERM", + code: null, + }) + : Object.assign(new Error("Command failed"), { code: "ENOTFOUND" }); + }, + github: (async () => { + throw new Error("octokit must not run"); + }) as never, + }); + const repoOf = (name: string) => ({ + provider: "github" as const, + owner: REPO.owner, + name, + url: `https://github.com/${REPO.owner}/${name}.git`, + remoteName: "origin", + defaultBranch: "main", + }); + const fetchOpenPrs = openPullRequestsSweeper(manager); + + // Our own 10s deadline killed the child; GitHub never said anything. + await withSilencedWarnings(() => + fetchOpenPrs(repoOf("repo-a")).catch(() => {}), + ); + expect(ghAttempts).toBe(1); + expect(manager.getGithubStatus()).toBeNull(); + + // The resolver fails on the next repo: that is GitHub being unreachable. + timeout = false; + await withSilencedWarnings(() => + fetchOpenPrs(repoOf("repo-b")).catch(() => {}), + ); + expect(ghAttempts).toBe(2); + expect(manager.getGithubStatus()?.reason).toBe("unreachable"); + }); + + test("a sweep over many repos keeps gh spawns at the concurrency limit", async () => { + const db = createRealDb(); + seedProject(db); + let calls = 0; + let inFlight = 0; + let peak = 0; + const releases: Array<() => void> = []; + const manager = createManager(db, { + execGh: async () => { + calls += 1; + inFlight += 1; + peak = Math.max(peak, inFlight); + const answer = Promise.withResolvers(); + releases.push(() => { + inFlight -= 1; + answer.resolve([]); + }); + return answer.promise; + }, + }); + const repoOf = (name: string) => ({ + provider: "github" as const, + owner: REPO.owner, + name, + url: `https://github.com/${REPO.owner}/${name}.git`, + remoteName: "origin", + defaultBranch: "main", + }); + const fetchOpenPrs = openPullRequestsSweeper(manager); + + await withSilencedWarnings(async () => { + const sweeps = Array.from({ length: 10 }, (_, index) => + fetchOpenPrs(repoOf(`repo-${index}`)), + ); + await tick(); + expect(peak).toBe(GH_CALL_CONCURRENCY); + + while (releases.length > 0) { + releases.shift()?.(); + await tick(); + } + await Promise.all(sweeps); + }); + + expect(calls).toBe(10); + expect(peak).toBe(GH_CALL_CONCURRENCY); + }); + + test("the hold drops lookups the sweep already queued", async () => { + const db = createRealDb(); + seedProject(db); + let ghAttempts = 0; + const releases: Array<() => void> = []; + const rejects: Array<(error: Error) => void> = []; + const manager = createManager(db, { + execGh: () => { + ghAttempts += 1; + const answer = Promise.withResolvers(); + if (ghAttempts <= GH_CALL_CONCURRENCY) { + releases.push(() => answer.resolve([])); + rejects.push((error) => answer.reject(error)); + } else { + answer.resolve([]); + } + return answer.promise; + }, + github: (async () => { + throw new Error("octokit must not run"); + }) as never, + }); + const repoOf = (name: string) => ({ + provider: "github" as const, + owner: REPO.owner, + name, + url: `https://github.com/${REPO.owner}/${name}.git`, + remoteName: "origin", + defaultBranch: "main", + }); + const fetchOpenPrs = openPullRequestsSweeper(manager); + + await withSilencedWarnings(async () => { + const sweeps = Array.from({ length: 10 }, (_, index) => + fetchOpenPrs(repoOf(`repo-${index}`)).catch(() => {}), + ); + await tick(); + expect(ghAttempts).toBe(GH_CALL_CONCURRENCY); + expect(releases).toHaveLength(GH_CALL_CONCURRENCY); + + // One running lookup fails; the hold it opens drops the six waiting. + rejects.shift()?.( + Object.assign(new Error("Command failed"), { code: "ENOTFOUND" }), + ); + await tick(); + expect(manager.getGithubStatus()?.reason).toBe("unreachable"); + expect(ghAttempts).toBe(GH_CALL_CONCURRENCY); + + for (const release of releases.splice(0)) release(); + await Promise.all(sweeps); + }); + + // The queued six never reached gh, then or after. + expect(ghAttempts).toBe(GH_CALL_CONCURRENCY); + }); + + test("queued probes survive two holds", async () => { + const db = createRealDb(); + seedProject(db); + const calls: Array<{ + resolve: (value: unknown) => void; + reject: (error: Error) => void; + }> = []; + const manager = createManager(db, { + execGh: () => { + const answer = Promise.withResolvers(); + calls.push(answer); + return answer.promise; + }, + github: (async () => { + throw new Error("octokit must not run"); + }) as never, + }); + const repoOf = (name: string) => ({ + provider: "github" as const, + owner: REPO.owner, + name, + url: `https://github.com/${REPO.owner}/${name}.git`, + remoteName: "origin", + defaultBranch: "main", + }); + const fetchOpenPrs = openPullRequestsSweeper(manager); + const accessible = manager as unknown as { + getCachedOpenPullRequests( + repo: ReturnType, + options?: { bypassCache?: boolean }, + ): Promise; + }; + const offline = () => + Object.assign(new Error("Command failed"), { code: "ENOTFOUND" }); + + await withSilencedWarnings(async () => { + const sweeps = ["bg-0", "bg-1", "bg-2", "bg-3"].map((name) => + fetchOpenPrs(repoOf(name)).catch(() => {}), + ); + await tick(); + expect(calls).toHaveLength(GH_CALL_CONCURRENCY); + + // Three user refreshes queue behind the sweep. + const probes = ["p-0", "p-1", "p-2"].map((name) => + accessible + .getCachedOpenPullRequests(repoOf(name), { bypassCache: true }) + .catch((error: Error) => error), + ); + await tick(); + expect(calls).toHaveLength(GH_CALL_CONCURRENCY); + + // First hold: the background queue is drained, the probes keep their + // place, and one of them takes the freed slot. + calls[0]?.reject(offline()); + await tick(); + expect(manager.getGithubStatus()?.reason).toBe("unreachable"); + expect(calls).toHaveLength(GH_CALL_CONCURRENCY + 1); + + // Its success reopens the gate and the next probe starts. + calls[1]?.resolve([]); + await tick(); + expect(manager.getGithubStatus()).toBeNull(); + expect(calls).toHaveLength(GH_CALL_CONCURRENCY + 2); + + // A second hold must not drop the probe still waiting. + calls[2]?.reject(offline()); + await tick(); + expect(manager.getGithubStatus()?.reason).toBe("unreachable"); + + calls[3]?.resolve([]); + await tick(); + expect(calls).toHaveLength(GH_CALL_CONCURRENCY + 3); + + for (const call of calls) call.resolve([]); + expect(await Promise.all(probes)).toEqual([[], [], []]); + await Promise.all(sweeps); + }); + }); + test("an explicit refresh probes through an active hold", async () => { const db = createRealDb(); seedProject(db); @@ -1365,6 +1604,7 @@ describe("PullRequestRuntimeManager refresh", () => { // The fetch hangs past its own post-failure window (120s after the // first failure) and only then rejects. const slow = sweep(); + await tick(); expect(attempts).toBe(1); setSystemTime(new Date(t0 + 130_000)); rejectInFlight?.(new Error("slow failure")); diff --git a/packages/host-service/src/runtime/pull-requests/pull-requests.ts b/packages/host-service/src/runtime/pull-requests/pull-requests.ts index a88a1de2ff1..621d8364e08 100644 --- a/packages/host-service/src/runtime/pull-requests/pull-requests.ts +++ b/packages/host-service/src/runtime/pull-requests/pull-requests.ts @@ -16,10 +16,12 @@ import type { GitWatcher } from "../../events/git-watcher"; import type { ExecGh } from "../../trpc/router/workspace-creation/utils/exec-gh"; import { type GitFactory, resolveDefaultBranchName } from "../git"; import { ConditionalGh } from "./utils/conditional-gh"; +import { GH_CALL_CONCURRENCY, GhCallLimiter } from "./utils/gh-concurrency"; import { GitHubAvailabilityGate, type GitHubAvailabilityStatus, GitHubUnavailableError, + isGitHubTimeoutError, isGitHubUnreachableError, } from "./utils/github-availability"; import { @@ -220,6 +222,8 @@ interface PullRequestDetails { export class PullRequestRuntimeManager { private readonly db: HostDb; private readonly execGh: ExecGh; + private readonly probeExecGh: ExecGh; + private readonly ghCalls: GhCallLimiter; private readonly conditionalGh: ConditionalGh; private readonly git: GitFactory; private readonly github: () => Promise; @@ -284,8 +288,14 @@ export class PullRequestRuntimeManager { constructor(options: PullRequestRuntimeManagerOptions) { this.db = options.db; - this.conditionalGh = new ConditionalGh(options.execGh); + this.ghCalls = new GhCallLimiter( + options.execGh, + GH_CALL_CONCURRENCY, + (error) => this.recordGhFailure(error), + ); + this.conditionalGh = new ConditionalGh(this.ghCalls.exec); this.execGh = this.conditionalGh.exec; + this.probeExecGh = this.ghCalls.probe; this.git = options.git; this.github = options.github; this.gitWatcher = options.gitWatcher; @@ -1277,7 +1287,7 @@ export class PullRequestRuntimeManager { private async fetchFromGitHub( what: string, context: Record, - viaGh: () => Promise, + viaGh: (execGh: ExecGh) => Promise, viaOctokit: () => Promise, options: { probe?: boolean } = {}, ): Promise { @@ -1286,12 +1296,18 @@ export class PullRequestRuntimeManager { // once the network is back. if (!options.probe) this.githubGate.assertReachable(); try { - const result = await viaGh(); + // A probe keeps its queue slot through a hold, so a user refresh can + // reopen the gate even when its sweep was drained ahead of it. + const result = await viaGh( + options.probe ? this.probeExecGh : this.execGh, + ); this.githubGate.recordSuccess(); return result; } catch (ghError) { - if (isGitHubUnreachableError(ghError)) { - this.noteGitHubFailure(ghError); + // A gated or drained call never reached gh; Octokit would bypass the + // gate. The gate itself saw every failure in the limiter. + if (ghError instanceof GitHubUnavailableError) throw ghError; + if (isGitHubTimeoutError(ghError) || isGitHubUnreachableError(ghError)) { throw ghError; } console.warn( @@ -1309,10 +1325,21 @@ export class PullRequestRuntimeManager { } } + /** + * Runs from the limiter, before the slot moves on, so the hold opens (and + * drains the queue) ahead of the next spawn. + */ + private recordGhFailure(error: unknown): void { + if (isGitHubTimeoutError(error) || isGitHubUnreachableError(error)) { + this.noteGitHubFailure(error); + } + } + /** Trips the gate on a host-wide failure and says so once per hold. */ private noteGitHubFailure(error: unknown): void { const hold = this.githubGate.recordFailure(error); if (!hold?.opened) return; + this.ghCalls.drain(new GitHubUnavailableError(hold.reason, hold.holdMs)); console.warn( `[host-service:pull-request-runtime] GitHub ${hold.reason}; holding GitHub lookups for ${Math.round(hold.holdMs / 1000)}s`, { error }, @@ -1418,9 +1445,9 @@ export class PullRequestRuntimeManager { this.fetchFromGitHub( "PR head lookup", { owner: repo.owner, name: repo.name, head }, - () => + (execGh) => fetchPullRequestByHeadFromGh( - this.execGh, + execGh, { owner: repo.owner, name: repo.name }, head, ), @@ -1457,15 +1484,15 @@ export class PullRequestRuntimeManager { const [reviewDecision, checks] = await this.fetchFromGitHub( "PR review/check lookup", context, - () => + (execGh) => Promise.all([ fetchPullRequestReviewDecisionFromGh( - this.execGh, + execGh, repo, node.number, node.state, ), - fetchPullRequestChecksFromGh(this.execGh, repo, node.headRefOid), + fetchPullRequestChecksFromGh(execGh, repo, node.headRefOid), ]), async () => { const octokit = await this.github(); @@ -1491,9 +1518,9 @@ export class PullRequestRuntimeManager { isInMergeQueue = await this.fetchFromGitHub( "PR merge-queue lookup", context, - () => + (execGh) => fetchPullRequestMergeQueueStateFromGh( - this.execGh, + execGh, repo, node.number, ), @@ -1533,8 +1560,8 @@ export class PullRequestRuntimeManager { this.fetchFromGitHub( "open-PR sweep", { owner: repo.owner, name: repo.name }, - () => - fetchOpenPullRequestsFromGh(this.execGh, { + (execGh) => + fetchOpenPullRequestsFromGh(execGh, { owner: repo.owner, name: repo.name, }), diff --git a/packages/host-service/src/runtime/pull-requests/utils/gh-concurrency/gh-concurrency.test.ts b/packages/host-service/src/runtime/pull-requests/utils/gh-concurrency/gh-concurrency.test.ts new file mode 100644 index 00000000000..e2db190990d --- /dev/null +++ b/packages/host-service/src/runtime/pull-requests/utils/gh-concurrency/gh-concurrency.test.ts @@ -0,0 +1,92 @@ +import { describe, expect, test } from "bun:test"; +import type { ExecGh } from "../../../../trpc/router/workspace-creation/utils/exec-gh"; +import { GhCallLimiter } from "./gh-concurrency"; + +// Lets every resume that a release triggers run before the next assertion. +const tick = () => { + const { promise, resolve } = Promise.withResolvers(); + setImmediate(resolve); + return promise; +}; + +describe("GhCallLimiter", () => { + test("runs every call while never exceeding the limit", async () => { + let inFlight = 0; + let peak = 0; + const releases: Array<() => void> = []; + const run: ExecGh = async (args) => { + inFlight++; + peak = Math.max(peak, inFlight); + const answer = Promise.withResolvers(); + releases.push(() => { + inFlight--; + answer.resolve(args[1]); + }); + return answer.promise; + }; + const limiter = new GhCallLimiter(run, 3); + + const calls = Array.from({ length: 10 }, (_, index) => + limiter.exec(["api", `call-${index}`]), + ); + await tick(); + expect(peak).toBe(3); + expect(releases).toHaveLength(3); + + while (releases.length > 0) { + releases.shift()?.(); + await tick(); + } + + expect(peak).toBe(3); + expect(await Promise.all(calls)).toEqual( + Array.from({ length: 10 }, (_, index) => `call-${index}`), + ); + }); + + test("passes a failure to its own caller and frees the slot", async () => { + const limiter = new GhCallLimiter(async (args) => { + if (args[0] === "fail") throw new Error("gh exited 1"); + return "ok"; + }, 1); + + const failure = await limiter + .exec(["fail"]) + .catch((error: Error) => error.message); + expect(failure).toBe("gh exited 1"); + expect(await limiter.exec(["next"])).toBe("ok"); + }); + + test("drains queued background calls and keeps a probe waiting", async () => { + let started = 0; + const releases: Array<() => void> = []; + const limiter = new GhCallLimiter(() => { + started++; + const answer = Promise.withResolvers(); + releases.push(() => answer.resolve("done")); + return answer.promise; + }, 1); + + const running = limiter.exec(["api", "first"]); + const queued = limiter.exec(["api", "second"]); + const probe = limiter.probe(["api", "third"]); + await tick(); + expect(started).toBe(1); + + const offline = new Error("GitHub is unreachable from this host"); + limiter.drain(offline); + + expect(await queued.catch((error: Error) => error.message)).toBe( + offline.message, + ); + expect(started).toBe(1); + + releases.shift()?.(); + expect(await running).toBe("done"); + await tick(); + expect(started).toBe(2); + + releases.shift()?.(); + expect(await probe).toBe("done"); + }); +}); diff --git a/packages/host-service/src/runtime/pull-requests/utils/gh-concurrency/gh-concurrency.ts b/packages/host-service/src/runtime/pull-requests/utils/gh-concurrency/gh-concurrency.ts new file mode 100644 index 00000000000..72cdd164a9b --- /dev/null +++ b/packages/host-service/src/runtime/pull-requests/utils/gh-concurrency/gh-concurrency.ts @@ -0,0 +1,84 @@ +import type { ExecGh } from "../../../../trpc/router/workspace-creation/utils/exec-gh"; + +/** + * How many `gh` children a PR refresh may run at once. A refresh fans out over + * projects, branches, and PR details; on a host with ~100 projects that spawned + * hundreds of processes in the same tick, and each one then raced the 10s + * deadline on a machine that could no longer start it. + */ +export const GH_CALL_CONCURRENCY = 4; + +/** Background sweeps lose their place when the gate closes; probes keep theirs. */ +type GhCallLane = "background" | "probe"; + +/** + * A FIFO of `gh` calls. Queued calls wait for a slot before they start, so the + * caller's 10s deadline covers the call itself, not the wait. + */ +export class GhCallLimiter { + private active = 0; + private readonly waiting: Array<{ + lane: GhCallLane; + resolve: () => void; + reject: (error: Error) => void; + }> = []; + + constructor( + private readonly run: ExecGh, + private readonly limit: number, + /** Runs on a rejection before the slot is handed on. */ + private readonly onFailure?: (error: unknown) => void, + ) {} + + /** + * Fails every background call still waiting for a slot. The availability + * gate calls this when it closes, so a sweep that queued before the hold + * cannot keep spawning children against a dead network. Probes keep their + * place: a user refresh is the one call that can reopen the gate. + */ + drain(error: Error): void { + const kept: typeof this.waiting = []; + for (const waiter of this.waiting) { + if (waiter.lane === "probe") kept.push(waiter); + else waiter.reject(error); + } + this.waiting.splice(0, this.waiting.length, ...kept); + } + + private acquire(lane: GhCallLane): Promise { + if (this.active < this.limit) { + this.active++; + return Promise.resolve(); + } + const { promise, resolve, reject } = Promise.withResolvers(); + this.waiting.push({ lane, resolve, reject }); + return promise; + } + + // The freed slot goes straight to the next waiter; only an empty queue + // drops the count, so no later caller can take a slot twice. + private release(): void { + const next = this.waiting.shift(); + if (next) next.resolve(); + else this.active--; + } + + private inLane(lane: GhCallLane): ExecGh { + return async (args, options) => { + await this.acquire(lane); + try { + return await this.run(args, options); + } catch (error) { + // The gate must see the failure before this slot moves on, or the + // next queued call starts against a network already known to be down. + this.onFailure?.(error); + throw error; + } finally { + this.release(); + } + }; + } + + exec: ExecGh = this.inLane("background"); + probe: ExecGh = this.inLane("probe"); +} diff --git a/packages/host-service/src/runtime/pull-requests/utils/gh-concurrency/index.ts b/packages/host-service/src/runtime/pull-requests/utils/gh-concurrency/index.ts new file mode 100644 index 00000000000..2d9872c0c4a --- /dev/null +++ b/packages/host-service/src/runtime/pull-requests/utils/gh-concurrency/index.ts @@ -0,0 +1 @@ +export { GH_CALL_CONCURRENCY, GhCallLimiter } from "./gh-concurrency"; diff --git a/packages/host-service/src/runtime/pull-requests/utils/github-availability/github-availability.test.ts b/packages/host-service/src/runtime/pull-requests/utils/github-availability/github-availability.test.ts index 59f7fcd1e00..36fe6d84ee3 100644 --- a/packages/host-service/src/runtime/pull-requests/utils/github-availability/github-availability.test.ts +++ b/packages/host-service/src/runtime/pull-requests/utils/github-availability/github-availability.test.ts @@ -4,6 +4,7 @@ import { describeGitHubFailure, GitHubAvailabilityGate, GitHubUnavailableError, + isGitHubTimeoutError, isGitHubUnreachableError, } from "./github-availability"; @@ -22,7 +23,7 @@ describe("isGitHubUnreachableError", () => { expect(isGitHubUnreachableError(wrapped)).toBe(true); }); - test("matches gh's stderr phrasing and an execFile timeout kill", () => { + test("matches gh's stderr phrasing", () => { expect( isGitHubUnreachableError( Object.assign(new Error("Command failed: gh api"), { @@ -31,14 +32,17 @@ describe("isGitHubUnreachableError", () => { }), ), ).toBe(true); - expect( - isGitHubUnreachableError( - Object.assign(new Error("Command failed"), { - killed: true, - signal: "SIGTERM", - }), - ), - ).toBe(true); + }); + + test("an execFile timeout kill is our own deadline, not an outage", () => { + const timeout = Object.assign(new Error("Command failed"), { + killed: true, + signal: "SIGTERM", + code: null, + }); + expect(isGitHubTimeoutError(timeout)).toBe(true); + expect(isGitHubUnreachableError(timeout)).toBe(false); + expect(classifyGitHubHold(timeout)).toBeNull(); }); test("matches what gh 2.98 prints for a dead resolver and a refused proxy", () => { @@ -198,6 +202,48 @@ describe("GitHubAvailabilityGate", () => { expect(block).toBe(30 * 60_000); }); + test("repeated exec kills open the hold, one alone does not", () => { + const now = 0; + const gate = new GitHubAvailabilityGate({ now: () => now }); + const killed = () => + Object.assign(new Error("Command failed"), { + killed: true, + signal: "SIGTERM", + code: null, + }); + + expect(gate.recordFailure(killed())).toBeNull(); + expect(gate.recordFailure(killed())).toBeNull(); + expect(gate.status()).toBeNull(); + expect(gate.recordFailure(killed())).toEqual({ + reason: "unreachable", + holdMs: 60_000, + opened: true, + }); + + // A success in between starts the count over. + gate.recordSuccess(); + expect(gate.recordFailure(killed())).toBeNull(); + }); + + test("a classified answer resets the run of kills", () => { + let now = 0; + const gate = new GitHubAvailabilityGate({ now: () => now }); + const killed = () => + Object.assign(new Error("Command failed"), { + killed: true, + signal: "SIGTERM", + code: null, + }); + + expect(gate.recordFailure(killed())).toBeNull(); + expect(gate.recordFailure(killed())).toBeNull(); + // GitHub answered, even with a rate limit: the run of kills is over. + expect(gate.recordFailure(rateLimited())?.reason).toBe("rate-limited"); + now += 5 * 60_000; + expect(gate.recordFailure(killed())).toBeNull(); + }); + test("ignores answers from GitHub and resets on success", () => { let now = 0; const gate = new GitHubAvailabilityGate({ now: () => now }); diff --git a/packages/host-service/src/runtime/pull-requests/utils/github-availability/github-availability.ts b/packages/host-service/src/runtime/pull-requests/utils/github-availability/github-availability.ts index 3f2aa110679..e0315e1615a 100644 --- a/packages/host-service/src/runtime/pull-requests/utils/github-availability/github-availability.ts +++ b/packages/host-service/src/runtime/pull-requests/utils/github-availability/github-availability.ts @@ -38,6 +38,10 @@ const RATE_LIMIT_DEFAULT_HOLD_MS = 5 * 60_000; const RATE_LIMIT_MAX_HOLD_MS = 65 * 60_000; // Land just past the reset so the first retry is not a second 403. const RATE_LIMIT_RESET_SLACK_MS = 5_000; +// One exec kill is usually this host's own overload (see +// `isGitHubTimeoutError`). Repeated kills with no answer in between are what a +// dropped network looks like, and without the hold the queue would never drain. +const TIMEOUTS_BEFORE_HOLD = 3; export type GitHubHoldReason = "unreachable" | "rate-limited" | "auth"; @@ -98,14 +102,25 @@ export function isGitHubUnreachableError(error: unknown): boolean { for (let depth = 0; depth < MAX_CAUSE_DEPTH && current; depth++) { const code = codeOf(current); if (code && UNREACHABLE_CODES.has(code)) return true; - // execFile's timeout: the child was killed before it produced a result. - // For `gh` that is a hung network call; a fast failure exits on its own. + if (UNREACHABLE_STDERR.test(textOf(current))) return true; + current = (current as { cause?: unknown }).cause; + } + return false; +} + +/** + * A kill by our own exec limits: the 10s deadline, or the stdout cap in + * `exec-gh.ts`. The child never answered, which says nothing about GitHub on + * its own: on a busy host the process was starved, not the network. + */ +export function isGitHubTimeoutError(error: unknown): boolean { + let current: unknown = error; + for (let depth = 0; depth < MAX_CAUSE_DEPTH && current; depth++) { const { killed, signal } = current as { killed?: unknown; signal?: unknown; }; if (killed === true && typeof signal === "string") return true; - if (UNREACHABLE_STDERR.test(textOf(current))) return true; current = (current as { cause?: unknown }).cause; } return false; @@ -190,6 +205,7 @@ export function classifyGitHubHold(error: unknown): GitHubHoldReason | null { export class GitHubAvailabilityGate { private streak = 0; + private timeouts = 0; private hold: GitHubAvailabilityStatus | null = null; private readonly now: () => number; @@ -227,8 +243,11 @@ export class GitHubAvailabilityGate { recordFailure( error: unknown, ): { reason: GitHubHoldReason; holdMs: number; opened: boolean } | null { - const reason = classifyGitHubHold(error); - if (!reason) return null; + const classified = classifyGitHubHold(error); + // Every answer, classified or not, breaks a run of kills. + const reachedTimeoutThreshold = this.countTimeout(error); + if (!classified && !reachedTimeoutThreshold) return null; + const reason = classified ?? "unreachable"; const now = this.now(); if (this.hold && this.hold.until > now) { return { @@ -260,8 +279,22 @@ export class GitHubAvailabilityGate { return { reason, holdMs, opened: true }; } + /** + * A kill by our own exec limits says nothing about GitHub on its own, so + * only a run of them opens the hold. + */ + private countTimeout(error: unknown): boolean { + if (!isGitHubTimeoutError(error)) { + this.timeouts = 0; + return false; + } + this.timeouts += 1; + return this.timeouts >= TIMEOUTS_BEFORE_HOLD; + } + recordSuccess(): void { this.streak = 0; + this.timeouts = 0; this.hold = null; } } diff --git a/packages/host-service/src/runtime/pull-requests/utils/github-availability/index.ts b/packages/host-service/src/runtime/pull-requests/utils/github-availability/index.ts index 9ba0e52f6ae..a262a51b122 100644 --- a/packages/host-service/src/runtime/pull-requests/utils/github-availability/index.ts +++ b/packages/host-service/src/runtime/pull-requests/utils/github-availability/index.ts @@ -4,5 +4,6 @@ export { type GitHubAvailabilityStatus, type GitHubHoldReason, GitHubUnavailableError, + isGitHubTimeoutError, isGitHubUnreachableError, } from "./github-availability";