From ffae4f2f0ab40042cc8a4550e31d40123a2f720e Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 3 Sep 2026 14:15:01 -0700 Subject: [PATCH] fix(gh): stop re-spawning a gh/glab that keeps dying at its deadline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #18234's reporter has `exec mise x "gh" -- "gh" "$@"` in ~/.local/bin/gh. When mise hands the inner bare `gh` a PATH that still resolves back to the wrapper, the wrapper re-execs itself in place: one process, state R, one core at 100%, forever. Reproduced in a container — the `exec` is why it is a single spinning process rather than a growing ladder, and the syscall histogram matches the reporter's (futex ~62%, then statx/execve/readlinkat). #18239 bounded a single invocation and #18258 reaped its process group, but nothing bounded the sequence: the next caller started another 15-30s burn, so the machine never got the core back. Two consecutive deadline kills is enough evidence that the binary, not the network, is the problem — block further spawns for 1m, escalating to 30m, and reopen the moment gh answers anything. Scoped by CLI and runtime so a wedged native gh cannot disable glab or a WSL distro. glab is wired the same way: the wrapper hazard is a PATH-resolution fault, not a GitHub one. Also stops retrying a deadline kill — each retry paid another full deadline of the same spin before failing the call anyway. --- .../gh-exec-file-deadline.test.ts | 86 +++++++++++++ src/main/git/command-runner/gh-exec-file.ts | 47 ++++++- .../git/command-runner/gh-retry-policy.ts | 23 ++++ .../glab-exec-file-deadline.test.ts | 99 +++++++++++++++ src/main/git/command-runner/glab-exec-file.ts | 31 ++++- .../hosted-cli-unresponsive-breaker.test.ts | 115 ++++++++++++++++++ .../git/hosted-cli-unresponsive-breaker.ts | 114 +++++++++++++++++ src/main/git/runner-command-exec.test.ts | 4 + 8 files changed, 515 insertions(+), 4 deletions(-) create mode 100644 src/main/git/command-runner/glab-exec-file-deadline.test.ts create mode 100644 src/main/git/hosted-cli-unresponsive-breaker.test.ts create mode 100644 src/main/git/hosted-cli-unresponsive-breaker.ts diff --git a/src/main/git/command-runner/gh-exec-file-deadline.test.ts b/src/main/git/command-runner/gh-exec-file-deadline.test.ts index fa07dec32be..6e2136161a0 100644 --- a/src/main/git/command-runner/gh-exec-file-deadline.test.ts +++ b/src/main/git/command-runner/gh-exec-file-deadline.test.ts @@ -13,6 +13,7 @@ vi.mock('node:child_process', async (importOriginal) => ({ })) import { ghExecFileAsync } from './gh-exec-file' +import { _resetCliUnresponsiveBreaker } from '../hosted-cli-unresponsive-breaker' function mockChild(pid = 4321): ChildProcess { const child = new EventEmitter() as EventEmitter & Record @@ -47,8 +48,25 @@ describe('gh exec deadline', () => { afterEach(() => { vi.useRealTimers() vi.restoreAllMocks() + _resetCliUnresponsiveBreaker() }) + // Why filtered: the POSIX termination barrier shells out to `ps` to verify the + // process group died, so raw call counts mix those in with the gh spawns. + const ghSpawnCount = (): number => spawnMock.mock.calls.filter((call) => call[0] === 'gh').length + + /** Drive one gh call all the way to its deadline kill. */ + async function runToDeadline(pid: number): Promise { + const child = mockChild(pid) + spawnMock.mockReturnValue(child) + const rejection = expect( + ghExecFileAsync(['api', '--include', 'user/starred/stablyai/orca'], { timeout: 15_000 }) + ).rejects.toThrow('timed out') + await vi.advanceTimersByTimeAsync(15_000) + await vi.advanceTimersByTimeAsync(15_000) + await rejection + } + it.runIf(process.platform !== 'win32')( 'signals the whole process group, not just the child, when gh never exits', async () => { @@ -96,6 +114,74 @@ describe('gh exec deadline', () => { expect(options.shell).toBe(false) }) + /** + * The half #18239 and #18258 left open. Each invocation was bounded and its + * process group reaped, but the reporter's wrapper re-execs itself in place at + * 100% CPU, so every caller started a fresh 15s burn ~12s after the last one + * and the machine never got the core back. Two deadline kills is enough + * evidence that the binary — not the network — is the problem. + */ + it('stops spawning gh after two consecutive deadline kills', async () => { + await runToDeadline(1) + await runToDeadline(2) + expect(ghSpawnCount()).toBe(2) + + await expect( + ghExecFileAsync(['api', '--include', 'user/starred/stablyai/orca'], { timeout: 15_000 }) + ).rejects.toThrow('pausing gh') + + expect(ghSpawnCount()).toBe(2) + }) + + it('does not retry a deadline kill — each retry would pay another full deadline of spin', async () => { + // `gh api` reads as idempotent, so the transient-retry path is live here. + await runToDeadline(1) + + expect(ghSpawnCount()).toBe(1) + }) + + it('keeps spawning while gh still answers, even when it answers with a failure', async () => { + const failing = mockChild(7) + spawnMock.mockImplementation(() => { + queueMicrotask(() => { + failing.stderr?.emit('data', Buffer.from('gh: HTTP 404')) + failing.emit('exit', 1, null) + failing.emit('close', 1, null) + }) + return failing + }) + + for (let i = 0; i < 3; i++) { + await expect( + ghExecFileAsync(['api', '--include', 'user/starred/stablyai/orca'], { timeout: 15_000 }) + ).rejects.toThrow('HTTP 404') + } + + expect(ghSpawnCount()).toBe(3) + }) + + it('reopens after one deadline kill is followed by a healthy answer', async () => { + await runToDeadline(1) + + const child = mockChild(2) + spawnMock.mockImplementation(() => { + queueMicrotask(() => settleChild(child, 'HTTP/2.0 204 No Content\r\n')) + return child + }) + await ghExecFileAsync(['api', '--include', 'user/starred/stablyai/orca'], { timeout: 15_000 }) + + await runToDeadline(3) + // The healthy answer cleared the count, so this lone kill must not block. + const after = mockChild(4) + spawnMock.mockImplementation(() => { + queueMicrotask(() => settleChild(after, 'HTTP/2.0 204 No Content\r\n')) + return after + }) + await expect( + ghExecFileAsync(['api', '--include', 'user/starred/stablyai/orca'], { timeout: 15_000 }) + ).resolves.toBeDefined() + }) + it('fails rather than returning a clipped answer when gh overruns maxBuffer', async () => { const child = mockChild() spawnMock.mockImplementation(() => { diff --git a/src/main/git/command-runner/gh-exec-file.ts b/src/main/git/command-runner/gh-exec-file.ts index e8308a8e4b4..ee7bd1fcbf8 100644 --- a/src/main/git/command-runner/gh-exec-file.ts +++ b/src/main/git/command-runner/gh-exec-file.ts @@ -25,11 +25,19 @@ import { argsLookIdempotent } from './gh-idempotency' import { applyGhHostToArgs, explicitGhHostname, explicitGhRepoHostname } from './gh-host-args' import { defaultGhExecTimeoutMs, + HostedCliTimeoutError, isTransientGhError, sleep, GH_RETRY_AFTER_MAX_MS, GH_RETRY_DELAYS_MS } from './gh-retry-policy' +import { + cliRuntimeScopeKey, + createCliUnresponsiveError, + getCliUnresponsiveBlockedUntilMs, + recordCliDeadlineKill, + recordCliResponded +} from '../hosted-cli-unresponsive-breaker' // `cwd?` omitted for non-repo-scoped gh calls (rate_limit, listAccessibleProjects) so one WSL-aware wrapper serves both. // `wslDistro?` routes global cwd-less gh through `wsl.exe -d ` on WSL-only Windows where gh.exe isn't on host PATH. @@ -52,6 +60,19 @@ function nonInteractiveGhEnv(env: NodeJS.ProcessEnv = process.env): NodeJS.Proce } } +function ghRuntimeScope(resolved: ResolvedCommand): string { + return cliRuntimeScopeKey('gh', resolved.wsl?.distro) +} + +// Why before every spawn and not once per call: the WSL and host fallbacks below +// re-resolve the command, and a wedged native gh says nothing about a WSL one. +function assertGhResponsive(resolved: ResolvedCommand): void { + const blockedUntilMs = getCliUnresponsiveBlockedUntilMs(ghRuntimeScope(resolved)) + if (blockedUntilMs !== null) { + throw createCliUnresponsiveError('gh', blockedUntilMs) + } +} + function ghRateLimitScope( args: readonly string[], options: GhExecOptions, @@ -109,11 +130,17 @@ export async function ghExecFileAsync( // Why: scope by runtime and host so unrelated github.com, GHES, and WSL quotas cannot block each other. const rateLimitBucket = classifyGhRateLimitBucket(args) const rateLimitProbe = isGhRateLimitProbe(args) + const timeoutMs = options.timeout ?? defaultGhExecTimeoutMs(options.env) assertGhRateLimitScopeAvailable(args, options, resolved, rateLimitBucket, rateLimitProbe) let lastError: unknown let attemptedHostFallback = false let attemptedDefaultWslFallback = false for (let attempt = 0; attempt <= GH_RETRY_DELAYS_MS.length; attempt++) { + // Why here and not once before the loop: a retry that waits out a transient + // error must not re-spawn a gh the breaker has since blocked. Why outside + // the try: this throw means nothing was spawned, so the catch below must not + // read it as gh having answered. + assertGhResponsive(resolved) try { // Why to-termination and not execFileCapture: `gh` on PATH is routinely a // shim (mise, asdf, volta, a hand-written wrapper), so the deadline below @@ -128,15 +155,31 @@ export async function ghExecFileAsync( encoding: (options.encoding ?? 'utf-8') as BufferEncoding, maxBuffer: options.maxBuffer, // Why: bound gh so one stuck child fails visibly instead of wedging the IPC lane. - timeout: options.timeout ?? defaultGhExecTimeoutMs(options.env), + timeout: timeoutMs, env: nonInteractiveGhEnv(options.env), - signal: options.signal + signal: options.signal, + // Why typed: the breaker below counts deadline kills only, and a + // message match would also catch a `gh` that printed "timed out". + createTimeoutError: () => new HostedCliTimeoutError('gh', resolved.binary, timeoutMs) }, resolved.termination ) + recordCliResponded(ghRuntimeScope(resolved)) return { stdout: stdout as string, stderr: stderr as string } } catch (err) { lastError = err + if (err instanceof HostedCliTimeoutError) { + // Why no retry: the two retries would each pay another full deadline of + // the same 100%-CPU spin before failing the call anyway (#18234). + recordCliDeadlineKill(ghRuntimeScope(resolved)) + throw err + } + // Why on the error path too: a non-zero exit is gh answering, which is + // proof the binary is not wedged even though the call failed. An abort is + // the caller giving up and proves nothing either way, so it is excluded. + if (!(err instanceof Error && err.name === 'AbortError')) { + recordCliResponded(ghRuntimeScope(resolved)) + } const { stderr } = extractExecError(err) if (isGhPrimaryRateLimitStderr(stderr)) { notifyGhPrimaryRateLimit(rateLimitBucket, ghRateLimitScope(args, options, resolved)) diff --git a/src/main/git/command-runner/gh-retry-policy.ts b/src/main/git/command-runner/gh-retry-policy.ts index b3b06a1a88e..730a7c5c665 100644 --- a/src/main/git/command-runner/gh-retry-policy.ts +++ b/src/main/git/command-runner/gh-retry-policy.ts @@ -35,6 +35,29 @@ export const GH_RETRY_DELAYS_MS = [250, 1000] as const export const GH_RETRY_AFTER_MAX_MS = 30_000 const DEFAULT_GH_EXEC_TIMEOUT_MS = 30_000 +/** + * Thrown when a hosted-provider CLI was killed at its deadline without answering. + * + * Why a type and not a message match: the unresponsive breaker (#18234) must + * count *only* deadline kills. A CLI that exits non-zero answered, and an + * aborted call is the caller giving up — neither is evidence of a wedged binary. + * + * Why the message names the spawned binary rather than the CLI: under WSL the + * process that actually overran the deadline is `wsl.exe`, and callers already + * surface that distinction. + */ +export class HostedCliTimeoutError extends Error { + readonly cli: string + readonly timeoutMs: number + + constructor(cli: string, spawnedBinary: string, timeoutMs: number) { + super(`${spawnedBinary} timed out.`) + this.name = 'HostedCliTimeoutError' + this.cli = cli + this.timeoutMs = timeoutMs + } +} + export async function sleep(ms: number, signal?: AbortSignal): Promise { if (signal?.aborted) { throw createAbortError() diff --git a/src/main/git/command-runner/glab-exec-file-deadline.test.ts b/src/main/git/command-runner/glab-exec-file-deadline.test.ts new file mode 100644 index 00000000000..e4830e8b507 --- /dev/null +++ b/src/main/git/command-runner/glab-exec-file-deadline.test.ts @@ -0,0 +1,99 @@ +import { EventEmitter } from 'node:events' +import type { ChildProcess } from 'node:child_process' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const { spawnMock, processKillMock } = vi.hoisted(() => ({ + spawnMock: vi.fn(), + processKillMock: vi.fn() +})) + +vi.mock('node:child_process', async (importOriginal) => ({ + ...(await importOriginal()), + spawn: spawnMock +})) + +import { glabExecFileAsync } from './glab-exec-file' +import { _resetCliUnresponsiveBreaker } from '../hosted-cli-unresponsive-breaker' + +function mockChild(pid: number): ChildProcess { + const child = new EventEmitter() as EventEmitter & Record + child.pid = pid + child.kill = vi.fn(() => true) + child.stdin = Object.assign(new EventEmitter(), { end: vi.fn() }) + child.stdout = new EventEmitter() + child.stderr = new EventEmitter() + return child as unknown as ChildProcess +} + +/** + * The wrapper hazard behind #18234 is a PATH-resolution fault, not a GitHub one: + * a `~/.local/bin/glab` that re-invokes its own name wedges exactly the way the + * reporter's `gh` did, so glab gets the same breaker. + */ +describe('glab exec deadline', () => { + beforeEach(() => { + vi.useFakeTimers() + spawnMock.mockReset() + processKillMock.mockReset() + vi.spyOn(process, 'kill').mockImplementation(processKillMock as unknown as typeof process.kill) + }) + + afterEach(() => { + vi.useRealTimers() + vi.restoreAllMocks() + _resetCliUnresponsiveBreaker() + }) + + // Why filtered: the POSIX termination barrier shells out to `ps` to verify the + // process group died, so raw call counts mix those in with the glab spawns. + const glabSpawnCount = (): number => + spawnMock.mock.calls.filter((call) => call[0] === 'glab').length + + async function runToDeadline(pid: number): Promise { + spawnMock.mockReturnValue(mockChild(pid)) + const rejection = expect( + glabExecFileAsync(['api', 'projects/stablyai%2Forca/issues'], { cwd: '/repo' }) + ).rejects.toThrow('timed out') + await vi.advanceTimersByTimeAsync(30_000) + await vi.advanceTimersByTimeAsync(30_000) + await rejection + } + + it('stops spawning glab after two consecutive deadline kills', async () => { + await runToDeadline(2001) + await runToDeadline(2002) + expect(glabSpawnCount()).toBe(2) + + await expect( + glabExecFileAsync(['api', 'projects/stablyai%2Forca/issues'], { cwd: '/repo' }) + ).rejects.toThrow('pausing glab') + + expect(glabSpawnCount()).toBe(2) + }) + + it('does not retry a deadline kill — each retry would pay another full deadline', async () => { + await runToDeadline(2003) + + expect(glabSpawnCount()).toBe(1) + }) + + it('keeps spawning while glab still answers, even with a failure', async () => { + const child = mockChild(2004) + spawnMock.mockImplementation(() => { + queueMicrotask(() => { + child.stderr?.emit('data', Buffer.from('glab: HTTP 404')) + child.emit('exit', 1, null) + child.emit('close', 1, null) + }) + return child + }) + + for (let i = 0; i < 3; i++) { + await expect( + glabExecFileAsync(['api', 'projects/stablyai%2Forca/issues'], { cwd: '/repo' }) + ).rejects.toThrow('HTTP 404') + } + + expect(glabSpawnCount()).toBe(3) + }) +}) diff --git a/src/main/git/command-runner/glab-exec-file.ts b/src/main/git/command-runner/glab-exec-file.ts index 3257dd9e818..177597a9412 100644 --- a/src/main/git/command-runner/glab-exec-file.ts +++ b/src/main/git/command-runner/glab-exec-file.ts @@ -6,11 +6,19 @@ import { execFileCaptureToTermination } from './exec-file-capture' import type { GitExecOptions } from './git-exec-options' import { argsLookIdempotent } from './gh-idempotency' import { + HostedCliTimeoutError, isTransientGhError, sleep, GH_RETRY_AFTER_MAX_MS, GH_RETRY_DELAYS_MS } from './gh-retry-policy' +import { + cliRuntimeScopeKey, + createCliUnresponsiveError, + getCliUnresponsiveBlockedUntilMs, + recordCliDeadlineKill, + recordCliResponded +} from '../hosted-cli-unresponsive-breaker' // Why: cloned from the gh runner rather than abstracted behind a generic runner, to avoid touching the working gh path. const DEFAULT_GLAB_EXEC_TIMEOUT_MS = 30_000 @@ -59,9 +67,17 @@ export async function glabExecFileAsync( ): Promise<{ stdout: string; stderr: string }> { ;({ args, options } = redirectPortedHostnameToEnv(args, options)) let resolved = resolveCommand('glab', args, options.cwd, options.wslDistro) + const timeoutMs = options.timeout ?? DEFAULT_GLAB_EXEC_TIMEOUT_MS let lastError: unknown let attemptedDefaultWslFallback = false for (let attempt = 0; attempt <= GH_RETRY_DELAYS_MS.length; attempt++) { + // Why outside the try: a glab wedged the way #18234's gh was must not be + // re-spawned every cycle, and this throw means nothing was spawned. + const scope = cliRuntimeScopeKey('glab', resolved.wsl?.distro) + const blockedUntilMs = getCliUnresponsiveBlockedUntilMs(scope) + if (blockedUntilMs !== null) { + throw createCliUnresponsiveError('glab', blockedUntilMs) + } try { // Why to-termination: same shim chain as gh — the deadline has to reap the // whole tree, not just the wrapper that spawned it (#18234). @@ -72,15 +88,26 @@ export async function glabExecFileAsync( cwd: resolved.cwd, encoding: (options.encoding ?? 'utf-8') as BufferEncoding, maxBuffer: options.maxBuffer, - timeout: options.timeout ?? DEFAULT_GLAB_EXEC_TIMEOUT_MS, + timeout: timeoutMs, env: options.env, - signal: options.signal + signal: options.signal, + createTimeoutError: () => new HostedCliTimeoutError('glab', resolved.binary, timeoutMs) }, resolved.termination ) + recordCliResponded(scope) return { stdout: stdout as string, stderr: stderr as string } } catch (err) { lastError = err + if (err instanceof HostedCliTimeoutError) { + // Why no retry: each retry would pay another full deadline against a + // binary that has already proven it never answers (#18234). + recordCliDeadlineKill(scope) + throw err + } + if (!(err instanceof Error && err.name === 'AbortError')) { + recordCliResponded(scope) + } const { stderr } = extractExecError(err) if ( process.platform === 'win32' && diff --git a/src/main/git/hosted-cli-unresponsive-breaker.test.ts b/src/main/git/hosted-cli-unresponsive-breaker.test.ts new file mode 100644 index 00000000000..d208243b75a --- /dev/null +++ b/src/main/git/hosted-cli-unresponsive-breaker.test.ts @@ -0,0 +1,115 @@ +import { afterEach, describe, expect, it } from 'vitest' +import { + cliRuntimeScopeKey, + createCliUnresponsiveError, + getCliUnresponsiveBlockedUntilMs, + recordCliDeadlineKill, + recordCliResponded, + _resetCliUnresponsiveBreaker +} from './hosted-cli-unresponsive-breaker' + +const NOW = 1_700_000_000_000 +const GH = cliRuntimeScopeKey('gh') + +afterEach(() => { + _resetCliUnresponsiveBreaker() +}) + +describe('hosted CLI unresponsive breaker', () => { + it('lets a single deadline kill through — one slow call is not a wedged binary', () => { + recordCliDeadlineKill(GH, NOW) + + expect(getCliUnresponsiveBlockedUntilMs(GH, NOW)).toBeNull() + }) + + it('blocks after two consecutive deadline kills and reopens when the backoff expires', () => { + recordCliDeadlineKill(GH, NOW) + recordCliDeadlineKill(GH, NOW) + + expect(getCliUnresponsiveBlockedUntilMs(GH, NOW)).toBe(NOW + 60_000) + expect(getCliUnresponsiveBlockedUntilMs(GH, NOW + 59_999)).toBe(NOW + 60_000) + expect(getCliUnresponsiveBlockedUntilMs(GH, NOW + 60_000)).toBeNull() + }) + + it('escalates the backoff when the re-probe after a block wedges again', () => { + recordCliDeadlineKill(GH, NOW) + recordCliDeadlineKill(GH, NOW) + recordCliDeadlineKill(GH, NOW + 60_000) + + // Why escalating: #18234's wrapper recursion is a configuration fault, so + // re-probing every minute forever pays a full deadline of CPU each time. + expect(getCliUnresponsiveBlockedUntilMs(GH, NOW + 60_000)).toBe(NOW + 60_000 + 300_000) + }) + + it('clamps the backoff at the longest rung', () => { + for (let i = 0; i < 20; i++) { + recordCliDeadlineKill(GH, NOW) + } + + expect(getCliUnresponsiveBlockedUntilMs(GH, NOW)).toBe(NOW + 1_800_000) + }) + + it('closes the breaker as soon as gh answers anything', () => { + recordCliDeadlineKill(GH, NOW) + recordCliDeadlineKill(GH, NOW) + expect(getCliUnresponsiveBlockedUntilMs(GH, NOW)).not.toBeNull() + + recordCliResponded(GH) + + expect(getCliUnresponsiveBlockedUntilMs(GH, NOW)).toBeNull() + }) + + it('resets the escalation too, so a later wedge starts at the shortest backoff', () => { + recordCliDeadlineKill(GH, NOW) + recordCliDeadlineKill(GH, NOW) + recordCliDeadlineKill(GH, NOW) + recordCliResponded(GH) + + recordCliDeadlineKill(GH, NOW) + recordCliDeadlineKill(GH, NOW) + + expect(getCliUnresponsiveBlockedUntilMs(GH, NOW)).toBe(NOW + 60_000) + }) + + it('scopes by runtime so a wedged native gh does not block a WSL distro', () => { + recordCliDeadlineKill(GH, NOW) + recordCliDeadlineKill(GH, NOW) + + expect(getCliUnresponsiveBlockedUntilMs(GH, NOW)).not.toBeNull() + expect(getCliUnresponsiveBlockedUntilMs(cliRuntimeScopeKey('gh', 'Ubuntu'), NOW)).toBeNull() + }) + + it('scopes by CLI so a wedged gh does not disable GitLab', () => { + recordCliDeadlineKill(GH, NOW) + recordCliDeadlineKill(GH, NOW) + + expect(getCliUnresponsiveBlockedUntilMs(cliRuntimeScopeKey('glab'), NOW)).toBeNull() + }) + + it('keys WSL distros case-insensitively, the way the runner resolves them', () => { + expect(cliRuntimeScopeKey('gh', 'Ubuntu')).toBe(cliRuntimeScopeKey('gh', 'ubuntu')) + expect(cliRuntimeScopeKey('gh', undefined)).toBe(GH) + }) + + it('names the deadline kills and the wrapper hypothesis in the blocked error', () => { + const error = createCliUnresponsiveError('gh', NOW + 60_000, NOW) + + expect(error.cliUnresponsiveBlocked).toBe(true) + expect(error.message).toContain('~60s') + expect(error.message).toContain('18234') + // Why stderr: callers classify gh failures from stderr, not from message. + expect(error.stderr).toBe(error.message) + }) + + it('evicts cold runtimes instead of growing without bound', () => { + for (let i = 0; i < 200; i++) { + recordCliDeadlineKill(cliRuntimeScopeKey('gh', `distro-${i}`), NOW) + recordCliDeadlineKill(cliRuntimeScopeKey('gh', `distro-${i}`), NOW) + } + + expect(getCliUnresponsiveBlockedUntilMs(cliRuntimeScopeKey('gh', 'distro-0'), NOW)).toBeNull() + expect( + getCliUnresponsiveBlockedUntilMs(cliRuntimeScopeKey('gh', 'distro-199'), NOW) + ).not.toBeNull() + }) +}) diff --git a/src/main/git/hosted-cli-unresponsive-breaker.ts b/src/main/git/hosted-cli-unresponsive-breaker.ts new file mode 100644 index 00000000000..39056700c0d --- /dev/null +++ b/src/main/git/hosted-cli-unresponsive-breaker.ts @@ -0,0 +1,114 @@ +/** + * Global circuit breaker for a hosted-provider CLI (`gh`, `glab`) that never answers. + * + * Why: #18234. The reporter's `~/.local/bin/gh` is `exec mise x gh -- gh "$@"`. + * When mise hands the inner bare `gh` a PATH that still resolves back to the + * wrapper, the wrapper re-execs itself in place — one process, state `R`, one + * core at 100%, forever. The deadline (#18239) and the process-group kill + * (#18258) bound a single invocation; nothing bounded the *sequence*, so every + * caller started a fresh 15-30s burn and the machine never got the core back. + * + * Two consecutive deadline kills mean the binary is wedged rather than the + * network being slow, so stop spawning it until the backoff expires. A single + * timeout stays a timeout: slow links and huge responses are real. + * + * Scoped by CLI and execution runtime (`gh:native`, `glab:wsl:`), not by + * provider host — a wedged binary fails every host it is asked about, and a + * wedged `gh` says nothing about `glab`. + * + * Lives under git/ with zero imports so the runner can consult it without an + * import cycle (mirrors gh-rate-limit-breaker.ts). + */ + +const DEADLINE_KILLS_BEFORE_BLOCK = 2 +// Why escalating: a wrapper that re-execs itself is a configuration fault, not +// a transient one. Re-probing every minute forever would keep paying a full +// deadline of CPU for an answer that has not changed. +const BACKOFF_MS = [60_000, 5 * 60_000, 15 * 60_000, 30 * 60_000] as const +// Why bounded: one entry per runtime, and WSL distro names come from user input. +const MAX_ENTRIES = 64 + +type UnresponsiveState = { + /** Deadline kills since the last time gh answered anything at all. */ + deadlineKills: number + blockedUntilMs: number +} + +const stateByScope = new Map() + +/** Scope key for a resolved CLI command. Native and each WSL distro wedge independently. */ +export function cliRuntimeScopeKey(cli: string, wslDistro?: string): string { + return `${cli}:${wslDistro ? `wsl:${wslDistro.toLowerCase()}` : 'native'}` +} + +function touch(scope: string, state: UnresponsiveState): void { + // Why delete-then-set: Map preserves insertion order, so this makes the + // eviction below drop the coldest runtime rather than an active one. + stateByScope.delete(scope) + stateByScope.set(scope, state) + while (stateByScope.size > MAX_ENTRIES) { + const oldest = stateByScope.keys().next().value + if (oldest === undefined) { + break + } + stateByScope.delete(oldest) + } +} + +/** + * Record that gh was killed at its deadline without ever answering. + * + * Only call this for a deadline kill: an abort is the caller giving up, and a + * non-zero exit means gh ran and had an opinion. + */ +export function recordCliDeadlineKill(scope: string, nowMs: number = Date.now()): void { + const existing = stateByScope.get(scope) + const deadlineKills = (existing?.deadlineKills ?? 0) + 1 + if (deadlineKills < DEADLINE_KILLS_BEFORE_BLOCK) { + touch(scope, { deadlineKills, blockedUntilMs: 0 }) + return + } + const backoffIndex = Math.min(deadlineKills - DEADLINE_KILLS_BEFORE_BLOCK, BACKOFF_MS.length - 1) + touch(scope, { deadlineKills, blockedUntilMs: nowMs + BACKOFF_MS[backoffIndex] }) +} + +/** + * Record that gh answered — success or failure. Any answer proves the binary is + * not wedged, so it closes the breaker and resets the escalation. + */ +export function recordCliResponded(scope: string): void { + stateByScope.delete(scope) +} + +export function getCliUnresponsiveBlockedUntilMs( + scope: string, + nowMs: number = Date.now() +): number | null { + const state = stateByScope.get(scope) + if (!state || state.blockedUntilMs <= nowMs) { + return null + } + touch(scope, state) + return state.blockedUntilMs +} + +export function createCliUnresponsiveError( + cli: string, + blockedUntilMs: number, + nowMs: number = Date.now() +): Error & { stderr: string; cliUnresponsiveBlocked: true } { + const retryInSeconds = Math.max(1, Math.ceil((blockedUntilMs - nowMs) / 1000)) + const message = + `${cli} did not respond to its last ${DEADLINE_KILLS_BEFORE_BLOCK} invocations and had to be ` + + `killed at the deadline; pausing ${cli} for ~${retryInSeconds}s instead of spawning it again. ` + + `If ${cli} on PATH is a wrapper script, check that it resolves the real binary (stablyai/orca#18234).` + return Object.assign(new Error(message), { + stderr: message, + cliUnresponsiveBlocked: true as const + }) +} + +/** @internal — test-only */ +export function _resetCliUnresponsiveBreaker(): void { + stateByScope.clear() +} diff --git a/src/main/git/runner-command-exec.test.ts b/src/main/git/runner-command-exec.test.ts index 89bcc4dca15..a37dcdf09e2 100644 --- a/src/main/git/runner-command-exec.test.ts +++ b/src/main/git/runner-command-exec.test.ts @@ -26,8 +26,12 @@ import { GitAdmissionScheduler, _resetGitAdmissionForTests } from './command-runner/git-subprocess-admission' +import { _resetCliUnresponsiveBreaker } from './hosted-cli-unresponsive-breaker' afterEach(() => _resetGitAdmissionForTests()) +// Why: the unresponsive breaker is process-global by design, so a case that +// drives gh to its deadline would otherwise block the next case's spawn. +afterEach(() => _resetCliUnresponsiveBreaker()) type MockChildProcess = EventEmitter & { stdout: EventEmitter