Clean up hidden rate limit PTYs

Clean up hidden Claude/Codex rate-limit fallback PTYs and cap Codex fallback output.
This commit is contained in:
Jinwoo Hong 2026-05-26 20:30:04 -04:00 committed by GitHub
parent 679d06f69f
commit bc706ef1bf
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 144 additions and 34 deletions

View File

@ -52,24 +52,24 @@ describe('fetchViaPty', () => {
it('disposes node-pty listeners before killing the hidden PTY on timeout', async () => {
const onDataDisposable = makeDisposable()
const onExitDisposable = makeDisposable()
const killMock = vi.fn()
spawnMock.mockReturnValue({
onData: vi.fn(() => onDataDisposable),
onExit: vi.fn(() => onExitDisposable),
write: vi.fn(),
kill: vi.fn()
kill: killMock
})
const resultPromise = fetchViaPty()
await vi.advanceTimersByTimeAsync(25_000)
await resultPromise
const term = spawnMock.mock.results[0]?.value as { kill: ReturnType<typeof vi.fn> }
expect(onDataDisposable.dispose.mock.invocationCallOrder[0]).toBeLessThan(
term.kill.mock.invocationCallOrder[0]
killMock.mock.invocationCallOrder[0]
)
expect(onExitDisposable.dispose.mock.invocationCallOrder[0]).toBeLessThan(
term.kill.mock.invocationCallOrder[0]
killMock.mock.invocationCallOrder[0]
)
})

View File

@ -6,6 +6,7 @@ import { resolveClaudeCommand } from '../codex-cli/command'
import type { ClaudeRuntimeAuthPreparation } from '../claude-accounts/runtime-auth-service'
import { applyClaudeEnvPatch } from '../claude-accounts/environment'
import { withMacTailscaleDnsHint } from '../network/macos-tailscale-dns-diagnostic'
import { cleanupHiddenRateLimitPty } from './hidden-pty-cleanup'
const PTY_TIMEOUT_MS = 25_000
const MAX_OUTPUT_LENGTH = 100_000 // 100KB buffer limit
@ -182,18 +183,10 @@ export async function fetchViaPty(options?: {
env: spawnEnv
})
const termDisposables: { dispose: () => void }[] = []
const disposeTermListeners = (): void => {
for (const disposable of termDisposables.splice(0)) {
disposable.dispose()
}
}
const timeout = setTimeout(() => {
if (!resolved) {
resolved = true
// Why: node-pty's NAPI callbacks can outlive the Electron JS
// environment if we kill the hidden PTY without disposing them first,
// which matches Orca's documented SIGABRT failure mode on shutdown.
if (claude21UsageSettleTimer) {
clearTimeout(claude21UsageSettleTimer)
claude21UsageSettleTimer = null
@ -202,8 +195,7 @@ export async function fetchViaPty(options?: {
clearInterval(enterInterval)
enterInterval = null
}
disposeTermListeners()
term.kill()
cleanupHiddenRateLimitPty(term, termDisposables, { kill: true })
// Even on timeout, try to parse whatever we collected
const clean = stripTerminalControlSequences(output)
const { session, weekly } = parsePtyUsage(clean)
@ -262,8 +254,7 @@ export async function fetchViaPty(options?: {
if (enterInterval) {
clearInterval(enterInterval)
}
disposeTermListeners()
term.kill()
cleanupHiddenRateLimitPty(term, termDisposables, { kill: true })
const clean = stripTerminalControlSequences(output)
const { session, weekly } = parsePtyUsage(clean)
@ -356,7 +347,7 @@ export async function fetchViaPty(options?: {
}
const onExitDisposable = term.onExit(() => {
disposeTermListeners()
cleanupHiddenRateLimitPty(term, termDisposables, { kill: false })
if (claude21UsageSettleTimer) {
clearTimeout(claude21UsageSettleTimer)
claude21UsageSettleTimer = null

View File

@ -49,6 +49,7 @@ describe('fetchCodexRateLimits', () => {
it('disposes node-pty listeners before killing the PTY fallback on timeout', async () => {
const onDataDisposable = makeDisposable()
const onExitDisposable = makeDisposable()
const killMock = vi.fn()
childSpawnMock.mockImplementation(() => {
throw new Error('rpc unavailable')
@ -57,19 +58,18 @@ describe('fetchCodexRateLimits', () => {
onData: vi.fn(() => onDataDisposable),
onExit: vi.fn(() => onExitDisposable),
write: vi.fn(),
kill: vi.fn()
kill: killMock
})
const resultPromise = fetchCodexRateLimits()
await vi.advanceTimersByTimeAsync(15_000)
await resultPromise
const term = ptySpawnMock.mock.results[0]?.value as { kill: ReturnType<typeof vi.fn> }
expect(onDataDisposable.dispose.mock.invocationCallOrder[0]).toBeLessThan(
term.kill.mock.invocationCallOrder[0]
killMock.mock.invocationCallOrder[0]
)
expect(onExitDisposable.dispose.mock.invocationCallOrder[0]).toBeLessThan(
term.kill.mock.invocationCallOrder[0]
killMock.mock.invocationCallOrder[0]
)
})

View File

@ -6,6 +6,7 @@ import { spawn } from 'node:child_process'
import { resolveCodexCommand } from '../codex-cli/command'
import { withMacTailscaleDnsHint } from '../network/macos-tailscale-dns-diagnostic'
import { getCmdExePath, getSpawnArgsForWindows } from '../win32-utils'
import { cleanupHiddenRateLimitPty } from './hidden-pty-cleanup'
const RPC_TIMEOUT_MS = 10_000
const PTY_TIMEOUT_MS = 15_000
@ -346,20 +347,11 @@ async function fetchViaPty(options?: FetchCodexRateLimitsOptions): Promise<Provi
}
})
const termDisposables: { dispose: () => void }[] = []
const disposeTermListeners = (): void => {
for (const disposable of termDisposables.splice(0)) {
disposable.dispose()
}
}
const timeout = setTimeout(() => {
if (!resolved) {
resolved = true
// Why: killing a hidden PTY without disposing node-pty's NAPI listener
// handles leaves ThreadSafeFunction callbacks alive into Electron
// shutdown, which can abort the app while Node cleans up its env.
disposeTermListeners()
term.kill()
cleanupHiddenRateLimitPty(term, termDisposables, { kill: true })
resolve({
provider: 'codex',
session: null,
@ -373,6 +365,11 @@ async function fetchViaPty(options?: FetchCodexRateLimitsOptions): Promise<Provi
const onDataDisposable = term.onData((data) => {
output += data
// Why: this background fallback only needs recent status output for
// parsing and diagnostics; cap noisy TUI output like the Claude fallback.
if (output.length > MAX_DIAGNOSTIC_OUTPUT_LENGTH) {
output = output.slice(-MAX_DIAGNOSTIC_OUTPUT_LENGTH)
}
// Wait for prompt, then send /status
if (!sentStatus && />\s*$/.test(data)) {
@ -389,8 +386,7 @@ async function fetchViaPty(options?: FetchCodexRateLimitsOptions): Promise<Provi
}
resolved = true
clearTimeout(timeout)
disposeTermListeners()
term.kill()
cleanupHiddenRateLimitPty(term, termDisposables, { kill: true })
// eslint-disable-next-line no-control-regex
const clean = output.replace(/\x1b\[[0-9;]*[a-zA-Z]/g, '')
@ -415,7 +411,7 @@ async function fetchViaPty(options?: FetchCodexRateLimitsOptions): Promise<Provi
}
const onExitDisposable = term.onExit(() => {
disposeTermListeners()
cleanupHiddenRateLimitPty(term, termDisposables, { kill: false })
if (!resolved) {
resolved = true
clearTimeout(timeout)

View File

@ -0,0 +1,86 @@
import { afterEach, describe, expect, it, vi } from 'vitest'
import { cleanupHiddenRateLimitPty } from './hidden-pty-cleanup'
function setPlatform(platform: NodeJS.Platform): void {
Object.defineProperty(process, 'platform', {
configurable: true,
value: platform
})
}
describe('cleanupHiddenRateLimitPty', () => {
const originalPlatform = process.platform
afterEach(() => {
setPlatform(originalPlatform)
vi.clearAllMocks()
})
it('disposes listeners, kills the child, then destroys the PTY fd on POSIX', () => {
setPlatform('darwin')
const dataDisposable = { dispose: vi.fn() }
const exitDisposable = { dispose: vi.fn() }
const killMock = vi.fn()
const term = {
kill: killMock,
destroy: vi.fn()
}
cleanupHiddenRateLimitPty(term, [dataDisposable, exitDisposable], { kill: true })
expect(dataDisposable.dispose.mock.invocationCallOrder[0]).toBeLessThan(
killMock.mock.invocationCallOrder[0]
)
expect(exitDisposable.dispose.mock.invocationCallOrder[0]).toBeLessThan(
killMock.mock.invocationCallOrder[0]
)
expect(killMock.mock.invocationCallOrder[0]).toBeLessThan(
term.destroy.mock.invocationCallOrder[0]
)
})
it('releases the PTY fd without killing again after natural exit', () => {
setPlatform('darwin')
const killMock = vi.fn()
const term = {
kill: killMock,
destroy: vi.fn()
}
cleanupHiddenRateLimitPty(term, [], { kill: false })
expect(killMock).not.toHaveBeenCalled()
expect(term.destroy).toHaveBeenCalledTimes(1)
})
it('neutralizes POSIX destroy-time SIGHUP after the intentional kill', () => {
setPlatform('linux')
const killMock = vi.fn()
const term = {
kill: killMock,
destroy: vi.fn(() => {
term.kill('SIGHUP')
})
}
cleanupHiddenRateLimitPty(term, [], { kill: true })
expect(killMock).toHaveBeenCalledTimes(1)
expect(killMock).toHaveBeenCalledWith()
})
it('does not neutralize kill on Windows because destroy closes ConPTY through kill', () => {
setPlatform('win32')
const term = {
kill: vi.fn(),
destroy: vi.fn(() => {
term.kill()
})
}
cleanupHiddenRateLimitPty(term, [], { kill: true })
expect(term.kill).toHaveBeenCalledTimes(2)
expect(term.destroy).toHaveBeenCalledTimes(1)
})
})

View File

@ -0,0 +1,37 @@
type HiddenPty = {
kill: (signal?: string) => void
destroy?: () => void
}
type Disposable = {
dispose: () => void
}
export function cleanupHiddenRateLimitPty(
term: HiddenPty,
disposables: Disposable[],
options: { kill: boolean }
): void {
for (const disposable of disposables.splice(0)) {
disposable.dispose()
}
if (options.kill) {
try {
term.kill()
} catch {
/* already exited */
}
}
// Why: node-pty destroy releases the master PTY fd; on POSIX, neutralize
// the post-close SIGHUP hook after exit/kill to avoid pid reuse.
if (process.platform !== 'win32') {
term.kill = () => {}
}
try {
term.destroy?.()
} catch {
/* already torn down */
}
}