From 2c8afe92bb3b7d11f2c329545c479db4250d25e1 Mon Sep 17 00:00:00 2001 From: Brandon Wallace Date: Sat, 10 Oct 2026 13:27:28 -0500 Subject: [PATCH 1/3] fix(cli): stop clodex occasionally hanging after a command finishes on Node 24 On Node 24, process.exit() can deadlock and never return (nodejs/node#64274): it joins V8's background threads before tearing down the isolate, and a background compile waiting for a main-thread garbage collection is never woken. Every clodex command, and clodex-claude's error and spawn-fallback paths, ended through process.exit(), so a finished command could occasionally hang forever, ignoring Ctrl-C. Both bins now end through exitAfterDrain() (src/process-exit.ts): it sets process.exitCode, cancels background work registered with cancelOnExit(), and lets the event loop drain, which tears the isolate down first. If something still holds the loop after one second it falls back to process.exit() with the same code; CLODEX_TRACE=1 or --trace names what held it. Work nothing awaits no longer holds the process open: - the models.dev refresh is cancelled on exit, and waits 500 ms before starting so quick commands (help, version, listings) never issue a DNS lookup that could not be cancelled; - a Responses WebSocket in its closing handshake is terminated on exit instead of waiting for the server's answer (it added the server round trip to every endpoint-mode exit); - outbound connections still being opened are destroyed on exit, since aborting a fetch does not stop undici's connection attempt and a stalled TLS handshake held the exit for 10 s; - launchClaude removes its signal forwarders once Claude Code exits, so a Ctrl-C while clodex winds down is not swallowed. Exit codes are unchanged: the first requested code wins, as with process.exit(); a child killed by a signal still maps to 128+n; a cancelled prompt still exits 0 and an unexpected error 1. --- CLAUDE.md | 6 + src/claude-wrapper.ts | 23 +- src/cli.ts | 14 +- src/launch.ts | 19 +- src/oauth/responses-websocket.ts | 19 +- src/outbound-proxy.ts | 36 +++ src/process-exit.ts | 104 +++++++ src/registry/models-dev.ts | 38 ++- tests/helpers/exit-probe.ts | 127 ++++++++ tests/helpers/parent-notice-probe.ts | 14 + tests/parent-notice-launch.test.ts | 7 + tests/process-exit.test.ts | 390 +++++++++++++++++++++++++ tests/responses-websocket-exit.test.ts | 116 ++++++++ 13 files changed, 886 insertions(+), 27 deletions(-) create mode 100644 src/process-exit.ts create mode 100644 tests/helpers/exit-probe.ts create mode 100644 tests/process-exit.test.ts create mode 100644 tests/responses-websocket-exit.test.ts diff --git a/CLAUDE.md b/CLAUDE.md index 938d6138..4bb951cb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -189,6 +189,12 @@ These bite from outside the subsystem that owns them, so they live here rather t every request to that origin fails until restart (#233); Node 24 CI cannot see that. Do not gate the install on proxy env again and do not drop the explicit `allowH2: false` because "undici 7 already defaults to it" — the option is what survives an undici 8 bump. +- **Never call `process.exit()` in `src/`; end through `exitAfterDrain()` (`src/process-exit.ts`).** + On Node 24 `process.exit()` can hang forever (nodejs/node#64274), so both bins set the exit code + and let the event loop drain, falling back to `process.exit()` after one second. That only works + if nothing holds the loop: background work that nothing awaits (the models.dev refresh, a + WebSocket's closing handshake, outbound connections still being opened) registers + `cancelOnExit()` so it is cut off as `process.exit()` used to cut it off. Run with `CLODEX_TRACE=1` to see what held the loop when the fallback fires. - **Every AI SDK generation entry point must resolve its timeout and retry budget through `src/upstream-retry.ts`.** Anthropic- and OpenAI-format `streamText` consumers abort at idle and total deadlines; `generateText` consumers abort at total only. Cancellation remains cooperative diff --git a/src/claude-wrapper.ts b/src/claude-wrapper.ts index c6b3845e..035ce480 100644 --- a/src/claude-wrapper.ts +++ b/src/claude-wrapper.ts @@ -50,6 +50,7 @@ import { wrapperSubstitutionEligible, } from './wrapper-env.js'; import { finalizeWrapperTarget, prepareWrapperTarget } from './wrapper-target.js'; +import { exitAfterDrain } from './process-exit.js'; const isWindows = process.platform === 'win32'; const WRAPPER_SERVER_READY_TIMEOUT_MS = 500; @@ -155,7 +156,8 @@ async function main(): Promise { if (!checkOnly && !claudePath) { process.stderr.write('clodex-claude: could not find the claude binary (set CLODEX_CLAUDE_PATH)\n'); - process.exit(127); + exitWrapper(127); + return; } // Selection policy (see orderWrapperServerCandidates): proxy-mode servers @@ -171,10 +173,14 @@ async function main(): Promise { WRAPPER_SERVER_READY_TIMEOUT_MS, { retryFailure: result => result === 'timeout' }, ); - if (checkOnly) process.exit(state ? 0 : 1); + if (checkOnly) { + exitWrapper(state ? 0 : 1); + return; + } if (!state && wrapperRequiresServer(process.env)) { process.stderr.write('clodex-claude: no live clodex server is available\n'); - process.exit(1); + exitWrapper(1); + return; } if (state && process.env[OAUTH_ACCOUNT_ENV]?.trim()) { process.stderr.write( @@ -230,15 +236,20 @@ async function main(): Promise { child.on('error', err => { process.stderr.write(`clodex-claude: failed to launch ${claudePath}: ${err.message}\n`); - process.exit(127); + exitWrapper(127); }); child.on('exit', (code, signal) => { if (signal) { const signum = osConstants.signals[signal as keyof typeof osConstants.signals]; - process.exit(signum ? 128 + signum : 1); + exitWrapper(signum ? 128 + signum : 1); + return; } - process.exit(code ?? 0); + exitWrapper(code ?? 0); }); } +function exitWrapper(code: number): void { + exitAfterDrain(code, { label: 'clodex-claude' }); +} + void main(); diff --git a/src/cli.ts b/src/cli.ts index b0425c9e..1862f3a8 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -80,6 +80,7 @@ import { } from './http-proxy/index.js'; import { runPatchCommand, runLaunchPatchCheck } from './patcher.js'; import { installOutboundDispatcher } from './outbound-proxy.js'; +import { exitAfterDrain } from './process-exit.js'; import { runInstallVscodeLauncherCommand } from './vscode-launcher.js'; import { CHECKED_EDITORS_DESCRIPTION } from './editor-extension-version.js'; const STARTER_CLAUDE_FLAGS = new Set(['--dry-run', '--trace', '--fast', '--endpoint', '--proxy', '--save-mode', '--help', '-h', '--version', '-v']); @@ -1878,13 +1879,10 @@ function isCliEntryPoint(): boolean { } if (isCliEntryPoint()) { - main().then((exitCode) => { - process.exit(exitCode); - }).catch((err: unknown) => { - if (err === Symbol.for('clack:cancel')) { - process.exit(0); - } - console.error(pc.red('\nUnexpected error:'), err); - process.exit(1); + const exitOptions = { trace: Boolean(parseArgs(process.argv.slice(2)).trace) }; + main().then((exitCode) => exitAfterDrain(exitCode, exitOptions), (err: unknown) => { + const cancelled = err === Symbol.for('clack:cancel'); + if (!cancelled) console.error(pc.red('\nUnexpected error:'), err); + exitAfterDrain(cancelled ? 0 : 1, exitOptions); }); } diff --git a/src/launch.ts b/src/launch.ts index c8680189..58bf6f92 100644 --- a/src/launch.ts +++ b/src/launch.ts @@ -125,19 +125,30 @@ export function launchClaude( shell: isWindows, }); - const forward = (signal: NodeJS.Signals): void => { - child.kill(signal); + const forwardSigint = (): void => { + child.kill('SIGINT'); + }; + const forwardSigterm = (): void => { + child.kill('SIGTERM'); }; - process.once('SIGINT', () => forward('SIGINT')); - process.once('SIGTERM', () => forward('SIGTERM')); + process.once('SIGINT', forwardSigint); + process.once('SIGTERM', forwardSigterm); + // Once claude is gone there is nothing to forward to: a Ctrl-C while clodex winds down must + // terminate it rather than be swallowed by a stale forwarder. + const stopForwarding = (): void => { + process.off('SIGINT', forwardSigint); + process.off('SIGTERM', forwardSigterm); + }; child.on('exit', (code) => { + stopForwarding(); restore(); resolve(code ?? 0); }); child.on('error', (err) => { + stopForwarding(); restore(); resolve(1); }); diff --git a/src/oauth/responses-websocket.ts b/src/oauth/responses-websocket.ts index dd0b9bb2..6ec14286 100644 --- a/src/oauth/responses-websocket.ts +++ b/src/oauth/responses-websocket.ts @@ -14,6 +14,7 @@ import type { FetchFunction } from '@ai-sdk/provider-utils'; import type { RawData, WebSocket as WsWebSocket } from 'ws'; import { CODEX_RESPONSES_WEBSOCKETS_BETA } from '../constants.js'; import { outboundWsProxyAgent } from '../outbound-proxy.js'; +import { cancelOnExit } from '../process-exit.js'; import { emitParentNotice } from '../parent-notice.js'; import { anthropicErrorType, @@ -1573,9 +1574,21 @@ function deleteEntry(entry: ConnectionEntry, closeSocket = true): void { entry.inFlight = false; entry.current = undefined; unregisterEntry(entry); - if (closeSocket) { - try { entry.socket.close(); } catch { /* ignore */ } - } + if (closeSocket) closeGracefully(entry.socket); +} + +/** ws `readyState` while a closing handshake is in progress. */ +const WS_CLOSING = 2; + +/** + * Start a closing handshake that cannot hold up the process exiting. The handshake waits for the + * peer to answer behind ws's ref'd 30-second close timer; an exit requested meanwhile terminates + * the socket instead, as process.exit() used to. + */ +function closeGracefully(socket: WsWebSocket): void { + try { socket.close(); } catch { return; } + if (socket.readyState !== WS_CLOSING) return; + socket.once('close', cancelOnExit(() => socket.terminate())); } function failContext( diff --git a/src/outbound-proxy.ts b/src/outbound-proxy.ts index 900c62a1..4b9f568e 100644 --- a/src/outbound-proxy.ts +++ b/src/outbound-proxy.ts @@ -25,9 +25,13 @@ import type { Agent as HttpAgent } from 'node:http'; import { randomUUID } from 'node:crypto'; +import diagnosticsChannel from 'node:diagnostics_channel'; +import { Socket } from 'node:net'; import { networkInterfaces } from 'node:os'; +import { TLSSocket } from 'node:tls'; import { HttpsProxyAgent } from 'https-proxy-agent'; import { emitParentNotice } from './parent-notice.js'; +import { cancelOnExit } from './process-exit.js'; // Private to this process. Sent only to an outbound proxy (not to the origin) // and removed if a request returns to our listener, regardless of its value. @@ -138,6 +142,37 @@ export function proxyUrlTargetsListener( let dispatcherInstalled = false; +/** + * Destroy outbound connections still being opened when the process is asked to exit. + * + * Aborting a request does not abort the connection being opened for it: undici keeps dialling + * until the TCP connect and TLS handshake finish or its 10 s connect timeout fires, and + * `Agent.destroy()` cannot reach a socket it has not been handed yet. A peer that stalls the + * handshake would therefore hold a requested exit until the fallback fires. process.exit() used + * to cut these off; this does the same. Established connections are left to their owners, which + * abort or unref them. Two cases are not seen, and a stall there still ends through the exit + * fallback: TLS sockets on Node 22, which does not report them on this channel, and the TLS + * handshake inside an HTTP(S)_PROXY tunnel, whose socket is tracked only until the TCP connection + * to the proxy is made. + */ +function abandonPendingConnectionsOnExit(): void { + const pending = new Set(); + const track = (message: unknown): void => { + const socket = (message as { socket?: unknown }).socket; + if (!(socket instanceof Socket)) return; + pending.add(socket); + const settled = () => pending.delete(socket); + socket.once(socket instanceof TLSSocket ? 'secureConnect' : 'connect', settled); + socket.once('close', settled); + }; + diagnosticsChannel.subscribe('net.client.socket', track); + cancelOnExit(() => { + diagnosticsChannel.unsubscribe('net.client.socket', track); + for (const socket of pending) socket.destroy(); + pending.clear(); + }); +} + /** Reset the install-once latch (tests only). */ export function resetOutboundDispatcherForTests(): void { dispatcherInstalled = false; @@ -160,6 +195,7 @@ export async function installOutboundDispatcher(): Promise { ? new EnvHttpProxyAgent({ allowH2: false }) : new Agent({ allowH2: false }); setGlobalDispatcher(dispatcher); + abandonPendingConnectionsOnExit(); dispatcherInstalled = true; return true; } catch (err) { diff --git a/src/process-exit.ts b/src/process-exit.ts new file mode 100644 index 00000000..9f0ec4da --- /dev/null +++ b/src/process-exit.ts @@ -0,0 +1,104 @@ +// src/process-exit.ts — how both bins (clodex, clodex-claude) end the process. +// +// Not process.exit(). On Node 24 it can hang forever: it joins V8's background threads before +// tearing down the isolate, so a background compile that is waiting for the main thread to collect +// garbage is never woken (https://github.com/nodejs/node/issues/64274). Letting the event loop +// drain with process.exitCode set tears the isolate down first, which releases that thread. +// +// A drain only happens if nothing still holds the loop, so work that nothing awaits registers a +// canceller here (see cancelOnExit) and is cut off when the exit is requested, exactly as +// process.exit() used to cut it off. If something still holds the loop after the grace period, +// the process falls back to process.exit() — the old behaviour, deadlock risk included — so an +// unforeseen leak costs a short delay rather than a process that never ends. + +import { writeSync } from 'node:fs'; + +/** + * How long a requested exit may wait for the event loop to drain before falling back to + * process.exit(). Measured drains finish in under a millisecond and never took more than about + * 60 ms, so this only matters when something unexpected holds the loop. One second keeps that case + * short for someone at a terminal, while leaving room for the waits a drain can legitimately + * include: an uncancellable in-flight DNS lookup or connection attempt, a pipe reader catching up on + * buffered output (which process.exit() can cut short), and closing sockets finishing their + * shutdown. Waiting also gives V8's background compiles time to finish, which should make a + * fallback that does fire less likely to meet the deadlock than an immediate process.exit(). + */ +export const EXIT_DRAIN_GRACE_MS = 1_000; + +export interface ExitAfterDrainOptions { + /** Report what still held the loop when the fallback fires. CLODEX_TRACE=1 also turns it on. */ + trace?: boolean; + /** Prefix for the trace line, e.g. `clodex` or `clodex-claude`. */ + label?: string; + graceMs?: number; +} + +const exitCancellers = new Set<() => void>(); +let exitRequested = false; + +/** + * Register work that nothing awaits and that must not keep the process alive once an exit is + * requested. Returns an unregister function; call it when the work settles. Work registered after + * an exit was requested is cancelled immediately. + */ +export function cancelOnExit(cancel: () => void): () => void { + if (exitRequested) { + runCanceller(cancel); + return () => {}; + } + exitCancellers.add(cancel); + return () => { + exitCancellers.delete(cancel); + }; +} + +function runCanceller(cancel: () => void): void { + try { + cancel(); + } catch { + // A canceller that throws must not stop the exit or the other cancellers. + } +} + +/** Active resources by type, e.g. `TCPSocketWrap x2, Timeout`. */ +export function describeActiveResources(resources: readonly string[] = process.getActiveResourcesInfo()): string { + if (resources.length === 0) return 'nothing reported'; + const counts = new Map(); + for (const resource of resources) counts.set(resource, (counts.get(resource) ?? 0) + 1); + return [...counts].map(([name, count]) => (count > 1 ? `${name} x${count}` : name)).join(', '); +} + +/** + * End the process with `code` once the event loop drains, the way process.exit(code) would have, + * without process.exit()'s Node 24 deadlock. Returns immediately: the caller must stop doing work + * (return) rather than rely on this call not returning. The first request wins, as with + * process.exit(); later requests are ignored. + */ +export function exitAfterDrain(code: number, options: ExitAfterDrainOptions = {}): void { + if (exitRequested) return; + exitRequested = true; + process.exitCode = code; + + const cancellers = [...exitCancellers]; + exitCancellers.clear(); + for (const cancel of cancellers) runCanceller(cancel); + + const graceMs = options.graceMs ?? EXIT_DRAIN_GRACE_MS; + // Unref'd, so it never holds the loop open itself: it fires only if something else does. + const fallback = setTimeout(() => { + if (options.trace || process.env.CLODEX_TRACE === '1') { + const label = options.label ?? 'clodex'; + try { + writeSync( + 2, + `${label}: event loop still busy ${graceMs}ms after exit was requested; ` + + `exiting with process.exit(${code}). Still active: ${describeActiveResources()}\n`, + ); + } catch { + // stderr gone; the exit matters more than the diagnostic. + } + } + process.exit(code); + }, graceMs); + fallback.unref(); +} diff --git a/src/registry/models-dev.ts b/src/registry/models-dev.ts index bf6867e1..a4495240 100644 --- a/src/registry/models-dev.ts +++ b/src/registry/models-dev.ts @@ -11,10 +11,13 @@ import { import { dirname, join } from 'node:path'; import bundledCache from '../data/models-dev-cache.json'; import { getAppHome } from '../paths.js'; +import { cancelOnExit } from '../process-exit.js'; import { normalizeModelIdCandidates } from './pricing.js'; export const MODELS_DEV_API_URL = 'https://models.dev/api.json'; const FETCH_TIMEOUT_MS = 15_000; +/** Longer than a help, version, or listing command takes end to end (about 250 ms measured). */ +export const MODELS_DEV_REFRESH_DELAY_MS = 500; const FILE_MODE = 0o600; export interface ModelsDevModalities { @@ -174,9 +177,12 @@ export function loadModelsDevCache(): ModelsDevCacheFile { return rememberModelsDevCache('bundled', loadBundledModelsDevCache()); } -export async function fetchModelsDevCache(): Promise { +export async function fetchModelsDevCache(signal?: AbortSignal): Promise { const controller = new AbortController(); const timer = setTimeout(() => controller.abort(), FETCH_TIMEOUT_MS); + const onCallerAbort = () => controller.abort(); + if (signal?.aborted) controller.abort(); + else signal?.addEventListener('abort', onCallerAbort, { once: true }); try { const response = await fetch(MODELS_DEV_API_URL, { signal: controller.signal, @@ -192,6 +198,7 @@ export async function fetchModelsDevCache(): Promise return null; } finally { clearTimeout(timer); + signal?.removeEventListener('abort', onCallerAbort); } } @@ -199,12 +206,31 @@ export function resolveModelsDevSlug(providerId: string): string { return REGISTRY_TO_MODELS_DEV[providerId] ?? providerId; } -/** Fetch latest models.dev catalog in the background; falls back to bundled snapshot offline. */ +/** + * Fetch latest models.dev catalog in the background; falls back to bundled snapshot offline. + * + * Nothing awaits this, so it must never keep the process alive: a requested exit cancels it, + * whether it is still waiting to start or already downloading. It waits MODELS_DEV_REFRESH_DELAY_MS + * before starting so that quick commands never touch the network at all. Once issued, a DNS lookup + * cannot be cancelled and would hold the exit until it resolved, and a command that finishes this + * quickly could never have completed the multi-megabyte download anyway. + */ export function refreshModelsDevCacheAsync(onComplete?: (updated: boolean) => void): void { - void (async () => { - const updated = (await fetchModelsDevCache()) !== null; - onComplete?.(updated); - })(); + const controller = new AbortController(); + const start = setTimeout(() => { + void (async () => { + try { + const updated = (await fetchModelsDevCache(controller.signal)) !== null; + onComplete?.(updated); + } finally { + unregister(); + } + })(); + }, MODELS_DEV_REFRESH_DELAY_MS); + const unregister = cancelOnExit(() => { + clearTimeout(start); + controller.abort(); + }); } export function findModelsDevModel( diff --git a/tests/helpers/exit-probe.ts b/tests/helpers/exit-probe.ts new file mode 100644 index 00000000..25407c18 --- /dev/null +++ b/tests/helpers/exit-probe.ts @@ -0,0 +1,127 @@ +// tests/helpers/exit-probe.ts +// +// Runs OUTSIDE vitest, in a plain Node process: whether a process ends by draining its event loop +// or through process.exit() is a property of a real process, and vitest's worker is not one this +// test may end. Bundled by tests/process-exit.test.ts. +// +// Contract: argv[2] is a mode, argv[3] an optional URL for the models-dev modes. The probe wraps +// process.exit so that path announces itself, and reports how the process ended on stderr: +// EXIT-REQUESTED just before exitAfterDrain is called +// EXITED via=process.exit code=N afterRequestMs=T the fallback (or any caller) used process.exit +// EXITED via=drain code=N afterRequestMs=T the event loop drained +// Every mode bounds itself: a probe that neither drains nor falls back is killed by the test. +import { cancelOnExit, exitAfterDrain } from '../../src/process-exit.js'; +import { installOutboundDispatcher } from '../../src/outbound-proxy.js'; +import { MODELS_DEV_API_URL, refreshModelsDevCacheAsync } from '../../src/registry/models-dev.js'; + +const mode = process.argv[2] ?? ''; +const upstreamUrl = process.argv[3] ?? ''; +const say = (line: string) => process.stderr.write(`${line}\n`); + +let viaProcessExit = false; +let requestedAt: number | undefined; +/** Measured here, not by the test: a loaded test worker can read both lines in one tick. */ +const sinceRequest = () => (requestedAt === undefined ? 'n/a' : Math.round(performance.now() - requestedAt)); +const realExit = process.exit.bind(process); +process.exit = ((code?: number) => { + viaProcessExit = true; + say(`EXITED via=process.exit code=${code} afterRequestMs=${sinceRequest()}`); + return realExit(code); +}) as typeof process.exit; +process.on('exit', code => { + if (!viaProcessExit) say(`EXITED via=drain code=${code} afterRequestMs=${sinceRequest()}`); +}); + +function requestExit(code: number, options: Parameters[1] = {}): void { + say('EXIT-REQUESTED'); + requestedAt = performance.now(); + exitAfterDrain(code, options); +} + +/** Holds the event loop open until cleared. */ +function holdLoop(): NodeJS.Timeout { + return setInterval(() => {}, 60_000); +} + +/** Point the models.dev refresh at the test's server, recording that the fetch really started. */ +function redirectModelsDevFetch(onStart: () => void): void { + const realFetch = globalThis.fetch; + globalThis.fetch = ((input: Parameters[0], init?: RequestInit) => { + if (String(input) !== MODELS_DEV_API_URL) return realFetch(input, init); + onStart(); + return realFetch(upstreamUrl, init); + }) as typeof fetch; +} + +async function run(): Promise { + switch (mode) { + case 'drain': + requestExit(7, { graceMs: 30_000 }); + return; + case 'held': { + holdLoop(); + requestExit(4, { graceMs: 300 }); + return; + } + case 'held-traced': { + holdLoop(); + requestExit(4, { graceMs: 300, trace: true }); + return; + } + case 'cancelled': { + const timer = holdLoop(); + cancelOnExit(() => clearInterval(timer)); + requestExit(0, { graceMs: 30_000 }); + return; + } + case 'unregistered': { + // A canceller whose work already finished must not run at exit. + const release = cancelOnExit(() => say('CANCELLER-RAN')); + release(); + requestExit(0, { graceMs: 30_000 }); + return; + } + case 'cancel-after-exit': { + requestExit(0, { graceMs: 30_000 }); + const timer = holdLoop(); + cancelOnExit(() => clearInterval(timer)); + return; + } + case 'first-wins': + requestExit(3, { graceMs: 30_000 }); + exitAfterDrain(5, { graceMs: 30_000 }); + return; + case 'models-dev-quick': { + // A command like `models --list`: some real work, finished well before the refresh's start + // delay. Without the delay its download would already be under way here. + redirectModelsDevFetch(() => say('FETCH-STARTED')); + refreshModelsDevCacheAsync(); + await new Promise(resolve => setTimeout(resolve, 100)); + requestExit(0, { graceMs: 30_000 }); + return; + } + case 'models-dev-in-flight': { + // A command still running when the refresh starts, ending while the download is in flight. + // The CLI's own fetch dispatcher, as main() installs it, owns the connection. + await installOutboundDispatcher(); + const started = new Promise(resolve => redirectModelsDevFetch(() => { + say('FETCH-STARTED'); + resolve(); + })); + refreshModelsDevCacheAsync(); + const command = holdLoop(); + await started; + // Give the connection time to reach the test's server, which never answers (over https, it + // never completes the TLS handshake either). + await new Promise(resolve => setTimeout(resolve, 200)); + clearInterval(command); + requestExit(0, { graceMs: 10_000, trace: true }); + return; + } + default: + say(`unknown mode ${mode}`); + process.exitCode = 99; + } +} + +void run(); diff --git a/tests/helpers/parent-notice-probe.ts b/tests/helpers/parent-notice-probe.ts index 783cf98d..a9169aa0 100644 --- a/tests/helpers/parent-notice-probe.ts +++ b/tests/helpers/parent-notice-probe.ts @@ -76,9 +76,14 @@ function awaitLaunch(exit: Promise, ms: number): Promise { }); } +function signalListenerCounts(): { sigint: number; sigterm: number } { + return { sigint: process.listenerCount('SIGINT'), sigterm: process.listenerCount('SIGTERM') }; +} + async function main(): Promise { const extraArgs = withDebugFile ? ['--debug-file', debugFile] : []; if (withDebugFile) writeFileSync(debugFile, ''); + const signalListenersBefore = signalListenerCounts(); const exit = launchClaude({ ...process.env }, undefined, extraArgs); try { @@ -109,6 +114,15 @@ async function main(): Promise { // Belt and braces: whatever happened above, the child is now free to exit. try { writeFileSync(release, ''); } catch { /* scratch may be gone */ } + // Signal forwarders left behind would swallow a Ctrl-C while clodex winds down. + const signalListenersAfter = signalListenerCounts(); + if (!isEpipe) { + process.stderr.write( + `probe-signal-listeners-added sigint=${signalListenersAfter.sigint - signalListenersBefore.sigint}` + + ` sigterm=${signalListenersAfter.sigterm - signalListenersBefore.sigterm}\n`, + ); + } + // After the child is gone the sink must be released, so this one goes straight // to the terminal rather than into a queue nobody will ever flush. emitParentNotice(AFTER_EXIT_TEXT); diff --git a/tests/parent-notice-launch.test.ts b/tests/parent-notice-launch.test.ts index ff63170a..80526d79 100644 --- a/tests/parent-notice-launch.test.ts +++ b/tests/parent-notice-launch.test.ts @@ -167,6 +167,13 @@ describe('parent notices while Claude Code owns the terminal', () => { expect(plain.output).toContain('probe-done exit=0'); }); + it('removes its signal forwarders once the child has exited', () => { + // A forwarder left installed would swallow a Ctrl-C aimed at clodex while it winds down + // after Claude Code exits. The signalled run consumed its SIGINT forwarder; SIGTERM's stayed. + expect(plain.output).toContain('probe-signal-listeners-added sigint=0 sigterm=0'); + expect(signalled.output).toContain('probe-signal-listeners-added sigint=0 sigterm=0'); + }); + it('never lets an ordinary parent write reach the terminal at all', () => { // The mute's original purpose: only opted-in notices are deferred-and-shown; // everything else stays swallowed exactly as before. diff --git a/tests/process-exit.test.ts b/tests/process-exit.test.ts new file mode 100644 index 00000000..5ea96d94 --- /dev/null +++ b/tests/process-exit.test.ts @@ -0,0 +1,390 @@ +// tests/process-exit.test.ts +// +// How clodex and clodex-claude end the process. process.exit() can deadlock on Node 24 +// (https://github.com/nodejs/node/issues/64274), so both bins request an exit and let the event +// loop drain instead, falling back to process.exit() only if something still holds the loop. +// +// Whether a process drained or was cut off is a property of a real process, so every case runs +// tests/helpers/exit-probe.ts in a plain Node process, bundled with tsup as +// tests/parent-notice-launch.test.ts does, and asserts on how it reported its own end. +import { type ChildProcess, spawn, spawnSync } from 'node:child_process'; +import { chmodSync, existsSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { createServer, type Server, type Socket } from 'node:net'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { build } from 'tsup'; +import { describeActiveResources } from '../src/process-exit.js'; +import { BOUNDED_NODE_CHILD } from './helpers/bounded-child.js'; + +const PROBE_SOURCE = fileURLToPath(new URL('./helpers/exit-probe.ts', import.meta.url)); +const PROBE_BUILD_NAME = 'exit-probe.built'; +const PROBE = join(dirname(PROBE_SOURCE), `${PROBE_BUILD_NAME}.mjs`); +const PROJECT_ROOT = join(dirname(PROBE_SOURCE), '..', '..'); +/** The two shipped bins, built next to the probe so bare imports resolve to the repo's node_modules. */ +const BUILT_CLI = join(dirname(PROBE_SOURCE), 'cli.built.mjs'); +const BUILT_WRAPPER = join(dirname(PROBE_SOURCE), 'claude-wrapper.built.mjs'); +/** Vitest's per-test timeout cannot cancel a subprocess, so the harness owns one. */ +const PROBE_TIMEOUT_MS = 30_000; + +interface ProbeRun { + code: number | null; + stderr: string; +} + +/** Milliseconds from the exit request to the exit, as the probe itself measured them. */ +function afterRequestMs(run: ProbeRun): number { + const match = /EXITED via=\S+ code=\S+ afterRequestMs=(\d+)/.exec(run.stderr); + if (!match) throw new Error(`probe did not report its exit:\n${run.stderr}`); + return Number(match[1]); +} + +const live = new Set(); +let home: string; + +function runProbe(mode: string, extraArgs: string[] = []): Promise { + return new Promise((resolve, reject) => { + const child = spawn(process.execPath, [PROBE, mode, ...extraArgs], { + stdio: ['ignore', 'ignore', 'pipe'], + env: { ...process.env, CLODEX_HOME: home, CLODEX_TRACE: '' }, + }); + live.add(child); + let stderr = ''; + child.stderr!.setEncoding('utf8'); + child.stderr!.on('data', (chunk: string) => { + stderr += chunk; + }); + const timer = setTimeout(() => { + child.kill('SIGKILL'); + reject(new Error(`exit probe "${mode}" did not end within ${PROBE_TIMEOUT_MS}ms:\n${stderr}`)); + }, PROBE_TIMEOUT_MS); + child.on('error', err => { + clearTimeout(timer); + reject(err); + }); + child.on('close', code => { + clearTimeout(timer); + live.delete(child); + resolve({ code, stderr }); + }); + }); +} + +/** A server that accepts connections and reads requests but never answers them. */ +function startSilentServer(): Promise<{ server: Server; url: string; requests: () => number; open: () => number }> { + return new Promise(resolve => { + const sockets = new Set(); + let requests = 0; + const server = createServer(socket => { + sockets.add(socket); + socket.on('data', () => { requests += 1; }); + socket.on('error', () => {}); + socket.on('close', () => sockets.delete(socket)); + }); + server.listen(0, '127.0.0.1', () => { + const address = server.address(); + const port = typeof address === 'object' && address ? address.port : 0; + resolve({ + server, + url: `http://127.0.0.1:${port}/api.json`, + requests: () => requests, + open: () => sockets.size, + }); + }); + }); +} + +beforeAll(async () => { + home = mkdtempSync(join(tmpdir(), 'clodex-exit-probe-')); + await build({ + entry: { + [PROBE_BUILD_NAME]: PROBE_SOURCE, + 'cli.built': join(PROJECT_ROOT, 'src', 'cli.ts'), + 'claude-wrapper.built': join(PROJECT_ROOT, 'src', 'claude-wrapper.ts'), + }, + outDir: dirname(PROBE_SOURCE), + format: ['esm'], + target: 'node22', + splitting: false, + sourcemap: false, + clean: false, + silent: true, + config: false, + dts: false, + shims: false, + skipNodeModulesBundle: true, + outExtension: () => ({ js: '.mjs' }), + }); +}, 120_000); + +afterAll(() => { + for (const child of live) child.kill('SIGKILL'); + live.clear(); + rmSync(home, { recursive: true, force: true }); + for (const built of [PROBE, BUILT_CLI, BUILT_WRAPPER]) rmSync(built, { force: true }); +}); + +describe('exitAfterDrain', () => { + it('ends the process with the requested code by draining, not through process.exit', async () => { + const run = await runProbe('drain'); + expect(run.code).toBe(7); + expect(run.stderr).toContain('EXITED via=drain code=7 '); + expect(run.stderr).not.toContain('via=process.exit'); + }); + + it('falls back to process.exit with the same code when something holds the loop', async () => { + const run = await runProbe('held'); + expect(run.code).toBe(4); + expect(run.stderr).toContain('EXITED via=process.exit code=4'); + // The fallback waits out its 300 ms grace period first: it is a backstop, not a race. + expect(afterRequestMs(run)).toBeGreaterThanOrEqual(290); + }); + + it('stays silent about the fallback unless tracing', async () => { + const run = await runProbe('held'); + expect(run.stderr).not.toContain('event loop still busy'); + }); + + it('names what still held the loop when tracing', async () => { + const run = await runProbe('held-traced'); + expect(run.code).toBe(4); + expect(run.stderr).toMatch( + /clodex: event loop still busy 300ms after exit was requested; exiting with process\.exit\(4\)\. Still active: [^\n]*Timeout/, + ); + }); + + it('runs registered cancellers so the work they guard cannot hold the exit', async () => { + const run = await runProbe('cancelled'); + expect(run.code).toBe(0); + expect(run.stderr).toContain('EXITED via=drain code=0'); + }); + + it('does not run a canceller whose work already finished', async () => { + const run = await runProbe('unregistered'); + expect(run.stderr).not.toContain('CANCELLER-RAN'); + expect(run.stderr).toContain('EXITED via=drain code=0'); + }); + + it('cancels work registered after the exit was already requested', async () => { + const run = await runProbe('cancel-after-exit'); + expect(run.stderr).toContain('EXITED via=drain code=0'); + }); + + it('keeps the first requested code, as process.exit did', async () => { + const run = await runProbe('first-wins'); + expect(run.code).toBe(3); + expect(run.stderr).toContain('EXITED via=drain code=3'); + }); +}); + +describe('the background models.dev refresh', () => { + it('never starts when the command finishes first', async () => { + const run = await runProbe('models-dev-quick'); + expect(run.stderr).not.toContain('FETCH-STARTED'); + expect(run.stderr).toContain('EXITED via=drain code=0'); + }); + + it('cannot hold the process open once the download is in flight', async () => { + const upstream = await startSilentServer(); + try { + const run = await runProbe('models-dev-in-flight', [upstream.url]); + // The download really was in flight against a server that never answers... + expect(run.stderr).toContain('FETCH-STARTED'); + expect(upstream.requests()).toBeGreaterThan(0); + // ...and the process still ended by draining, not by the 10 s fallback. + expect(run.stderr).not.toContain('event loop still busy'); + expect(run.stderr).toContain('EXITED via=drain code=0'); + expect(run.code).toBe(0); + } finally { + await new Promise(resolve => upstream.server.close(() => resolve())); + } + }, 25_000); + + it('cannot hold the process open while its TLS handshake is stalled', async () => { + const upstream = await startSilentServer(); + try { + const run = await runProbe('models-dev-in-flight', [upstream.url.replace('http:', 'https:')]); + // The connection really was opening, against a server that never completes TLS... + expect(run.stderr).toContain('FETCH-STARTED'); + expect(upstream.requests()).toBeGreaterThan(0); + // ...and the process still ended by draining, not by the 10 s fallback. + expect(run.stderr).not.toContain('event loop still busy'); + expect(run.stderr).toContain('EXITED via=drain code=0'); + expect(run.code).toBe(0); + } finally { + await new Promise(resolve => upstream.server.close(() => resolve())); + } + }, 25_000); +}); + +describe('describeActiveResources', () => { + it('groups resources by type with counts', () => { + expect(describeActiveResources(['TCPSocketWrap', 'Timeout', 'TCPSocketWrap'])).toBe('TCPSocketWrap x2, Timeout'); + }); + + it('says so when nothing is reported', () => { + expect(describeActiveResources([])).toBe('nothing reported'); + }); +}); + +/** + * Preload that records how a bin ended: `EXIT-WITNESS process.exit(N)` if anything called + * process.exit, else `EXIT-WITNESS drained(N)`. With WITNESS_NO_EXECVE=1 it also removes + * process.execve, forcing clodex-claude onto the spawn path Windows and Node < 22.15 take. + */ +function writeExitWitness(dir: string): string { + const path = join(dir, 'exit-witness.mjs'); + writeFileSync(path, [ + "if (process.env.WITNESS_NO_EXECVE === '1') delete process.execve;", + 'let viaExit = false;', + 'const realExit = process.exit.bind(process);', + 'process.exit = (code) => {', + ' viaExit = true;', + ' process.stderr.write(`EXIT-WITNESS process.exit(${code})\\n`);', + ' return realExit(code);', + '};', + "process.on('exit', (code) => {", + ' if (!viaExit) process.stderr.write(`EXIT-WITNESS drained(${code})\\n`);', + '});', + '', + ].join('\n')); + return path; +} + +describe('both bins end by draining the event loop', () => { + function runBin(bin: string, args: string[], env: NodeJS.ProcessEnv = {}) { + const scratch = mkdtempSync(join(tmpdir(), 'clodex-exit-bin-')); + try { + const witness = writeExitWitness(scratch); + const run = spawnSync(process.execPath, ['--import', witness, bin, ...args], { + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'pipe'], + env: { + ...process.env, + CLODEX_HOME: join(scratch, 'home'), + HOME: scratch, + CLODEX_TRACE: '', + CLODEX_CLAUDE_PATH: '', + PATH: '/usr/bin:/bin', + ...env, + }, + ...BOUNDED_NODE_CHILD, + }); + return { status: run.status, stderr: run.stderr, error: run.error, scratch }; + } finally { + rmSync(scratch, { recursive: true, force: true }); + } + } + + it.each([ + { args: ['--version'], status: 0 }, + { args: ['--help'], status: 0 }, + { args: ['models', '--list'], status: 0 }, + { args: ['--no-such-flag'], status: 1 }, + ])('clodex $args', ({ args, status }) => { + const run = runBin(BUILT_CLI, args); + expect(run.error).toBeUndefined(); + expect(run.status).toBe(status); + expect(run.stderr).toContain(`EXIT-WITNESS drained(${status})`); + expect(run.stderr).not.toContain('EXIT-WITNESS process.exit'); + }); + + it('clodex when a command fails with an unexpected error', () => { + const scratch = mkdtempSync(join(tmpdir(), 'clodex-exit-error-')); + try { + // A clodex home that cannot be created makes the server's log setup throw out of main(). + const notADirectory = join(scratch, 'file'); + writeFileSync(notADirectory, ''); + const run = runBin(BUILT_CLI, ['server', '--proxy', '--no-discovery'], { + CLODEX_HOME: join(notADirectory, 'home'), + }); + expect(run.status).toBe(1); + expect(run.stderr).toContain('Unexpected error'); + expect(run.stderr).toContain('EXIT-WITNESS drained(1)'); + expect(run.stderr).not.toContain('EXIT-WITNESS process.exit'); + } finally { + rmSync(scratch, { recursive: true, force: true }); + } + }); + + it.skipIf(process.platform === 'win32')('clodex-claude --check with no server', () => { + const run = runBin(BUILT_WRAPPER, ['--check']); + expect(run.status).toBe(1); + // Nothing else: the check must stop here rather than carry on toward a launch. + expect(run.stderr.trim()).toBe('EXIT-WITNESS drained(1)'); + }); + + it.skipIf(process.platform === 'win32')('clodex-claude when claude cannot be found', () => { + const run = runBin(BUILT_WRAPPER, ['-p', 'hi']); + expect(run.status).toBe(127); + expect(run.stderr.trim().split('\n')).toEqual([ + 'clodex-claude: could not find the claude binary (set CLODEX_CLAUDE_PATH)', + 'EXIT-WITNESS drained(127)', + ]); + }); + + it.skipIf(process.platform === 'win32')('clodex-claude when a server is required and none is live', () => { + const scratch = mkdtempSync(join(tmpdir(), 'clodex-exit-claude-')); + try { + const launched = join(scratch, 'launched'); + const fakeClaude = join(scratch, 'claude'); + writeFileSync(fakeClaude, `#!/bin/sh\ntouch "${launched}"\n`); + chmodSync(fakeClaude, 0o755); + const run = runBin(BUILT_WRAPPER, [fakeClaude], { CLODEX_REQUIRE_SERVER: '1', WITNESS_NO_EXECVE: '1' }); + expect(run.status).toBe(1); + expect(run.stderr.trim().split('\n')).toEqual([ + 'clodex-claude: no live clodex server is available', + 'EXIT-WITNESS drained(1)', + ]); + // Requesting the exit must also stop the launch that would otherwise follow. + expect(existsSync(launched)).toBe(false); + } finally { + rmSync(scratch, { recursive: true, force: true }); + } + }); + + it.skipIf(process.platform === 'win32')('clodex-claude on the spawn path, passing on claude\'s exit code', () => { + const scratch = mkdtempSync(join(tmpdir(), 'clodex-exit-claude-')); + try { + const fakeClaude = join(scratch, 'claude'); + writeFileSync(fakeClaude, '#!/bin/sh\nexit 3\n'); + chmodSync(fakeClaude, 0o755); + const run = runBin(BUILT_WRAPPER, [fakeClaude], { WITNESS_NO_EXECVE: '1' }); + expect(run.status).toBe(3); + expect(run.stderr).toContain('EXIT-WITNESS drained(3)'); + expect(run.stderr).not.toContain('EXIT-WITNESS process.exit'); + } finally { + rmSync(scratch, { recursive: true, force: true }); + } + }); + + it.skipIf(process.platform === 'win32')('clodex-claude on the spawn path when claude dies from a signal', () => { + const scratch = mkdtempSync(join(tmpdir(), 'clodex-exit-claude-')); + try { + const fakeClaude = join(scratch, 'claude'); + writeFileSync(fakeClaude, '#!/bin/sh\nkill -TERM $$\n'); + chmodSync(fakeClaude, 0o755); + const run = runBin(BUILT_WRAPPER, [fakeClaude], { WITNESS_NO_EXECVE: '1' }); + expect(run.status).toBe(128 + 15); + expect(run.stderr).toContain(`EXIT-WITNESS drained(${128 + 15})`); + expect(run.stderr).not.toContain('EXIT-WITNESS process.exit'); + } finally { + rmSync(scratch, { recursive: true, force: true }); + } + }); + + it.skipIf(process.platform === 'win32')('clodex-claude when the spawn itself fails', () => { + const scratch = mkdtempSync(join(tmpdir(), 'clodex-exit-claude-')); + try { + const unusable = join(scratch, 'claude'); + writeFileSync(unusable, '#!/bin/sh\nexit 0\n', { mode: 0o644 }); + const run = runBin(BUILT_WRAPPER, [], { CLODEX_CLAUDE_PATH: unusable }); + expect(run.status).toBe(127); + expect(run.stderr).toContain('failed to launch'); + expect(run.stderr).toContain('EXIT-WITNESS drained(127)'); + } finally { + rmSync(scratch, { recursive: true, force: true }); + } + }); +}); diff --git a/tests/responses-websocket-exit.test.ts b/tests/responses-websocket-exit.test.ts new file mode 100644 index 00000000..cf941ce2 --- /dev/null +++ b/tests/responses-websocket-exit.test.ts @@ -0,0 +1,116 @@ +// tests/responses-websocket-exit.test.ts +// +// A Responses WebSocket that clodex closes waits for the server to answer the closing handshake, +// behind ws's ref'd 30-second close timer. That wait must not hold up the process exiting: under +// `clodex claude --endpoint` it delayed every exit by the server's round trip, and a server that +// never answered would hold it until the exit fallback fired. Measured against the real ws package +// and chatgpt.com in the endpoint launch census; pinned here with a fake socket that reports the +// CLOSING state the real one enters on close(). +import { EventEmitter } from 'node:events'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const WS_CLOSING = 2; +const WS_CLOSED = 3; + +const { fakeSockets, exitCancellers, closeLeavesState } = vi.hoisted(() => ({ + fakeSockets: [] as FakeWebSocket[], + exitCancellers: [] as Array<{ cancel: () => void; release: ReturnType }>, + closeLeavesState: { value: 2 }, +})); + +class FakeWebSocket extends EventEmitter { + readyState = 0; + send = vi.fn(); + close = vi.fn(() => { + this.readyState = closeLeavesState.value; + }); + terminate = vi.fn(); + constructor(public url: string, public options: unknown) { + super(); + fakeSockets.push(this); + } +} + +vi.mock('ws', () => ({ WebSocket: FakeWebSocket, default: FakeWebSocket })); +vi.mock('../src/process-exit.js', () => ({ + cancelOnExit: vi.fn((cancel: () => void) => { + const release = vi.fn(); + exitCancellers.push({ cancel, release }); + return release; + }), +})); + +import { + createResponsesWebSocketFetch, + resetResponsesWebSocketConnectionsForTests, +} from '../src/oauth/responses-websocket.js'; + +const WS_URL = 'wss://chatgpt.com/backend-api/codex/responses'; + +async function readAll(res: Response): Promise { + return new Response(res.body).text(); +} + +/** One isolated request run to `response.completed`, which closes its socket. */ +async function completeOneRequest(): Promise { + const wsFetch = createResponsesWebSocketFetch(WS_URL); + const res = await wsFetch('https://x', { method: 'POST', headers: {}, body: '{}' }); + const socket = fakeSockets[fakeSockets.length - 1]!; + socket.readyState = 1; + socket.emit('open'); + socket.emit('message', Buffer.from(JSON.stringify({ type: 'response.completed' }))); + await readAll(res); + return socket; +} + +describe('closing a Responses WebSocket', () => { + beforeEach(() => { + resetResponsesWebSocketConnectionsForTests(); + fakeSockets.length = 0; + exitCancellers.length = 0; + closeLeavesState.value = WS_CLOSING; + }); + + it('terminates a socket still in its closing handshake when the process is asked to exit', async () => { + const socket = await completeOneRequest(); + expect(socket.close).toHaveBeenCalled(); + expect(exitCancellers.length).toBeGreaterThan(0); + expect(socket.terminate).not.toHaveBeenCalled(); + + for (const { cancel } of exitCancellers) cancel(); + expect(socket.terminate).toHaveBeenCalled(); + }); + + it('withdraws the exit hook once the handshake completes', async () => { + const socket = await completeOneRequest(); + socket.readyState = WS_CLOSED; + socket.emit('close', 1000, Buffer.alloc(0)); + for (const { release } of exitCancellers) expect(release).toHaveBeenCalled(); + }); + + it('registers nothing for a socket that closed without a handshake to wait for', async () => { + closeLeavesState.value = WS_CLOSED; + const socket = await completeOneRequest(); + expect(socket.close).toHaveBeenCalled(); + expect(exitCancellers).toHaveLength(0); + }); + + it('also covers sockets dropped through the failure path', async () => { + const wsFetch = createResponsesWebSocketFetch(WS_URL); + const controller = new AbortController(); + const res = await wsFetch('https://x', { + method: 'POST', + headers: {}, + body: '{}', + signal: controller.signal, + }); + const socket = fakeSockets[fakeSockets.length - 1]!; + socket.readyState = 1; + socket.emit('open'); + controller.abort(); + await readAll(res).catch(() => ''); + expect(socket.close).toHaveBeenCalled(); + for (const { cancel } of exitCancellers) cancel(); + expect(socket.terminate).toHaveBeenCalled(); + }); +}); From 3a7e4e5e401a073b29fb12db9a5ead383dd38c2b Mon Sep 17 00:00:00 2001 From: Brandon Wallace Date: Sat, 10 Oct 2026 14:02:31 -0500 Subject: [PATCH 2/3] fix(cli): stop clodex waiting a second to exit after adding or refreshing a provider The previous change made clodex end by letting its event loop drain. After `providers add` or `providers refresh-models`, the background pricing download still held the process until the one-second fallback, which is the old exit path this work exists to avoid. - Outbound connections still opening at exit are now destroyed with an error. A bare destroy never settled undici's connection attempt, so the fetch that opened it hung until its own 15 s timeout; now it fails at once, whoever started it. - The pricing enrichment registers with cancelOnExit(), as the models.dev refresh does, so an established request to a stalled server is abandoned too, along with its registry update. - After an exit is requested, stdout and stderr errors are ignored, as process.exit() did: a reader that went away no longer turns clodex-claude's 127 into 1. - launchClaude's exit and error handlers share one finish function, so the signal-forwarder cleanup cannot be dropped from either. Tests strip ambient proxy variables from their child processes, and the closed-pipe launch test now kills the nested probe's whole process group on timeout. --- CLAUDE.md | 6 +- src/launch.ts | 14 +-- src/outbound-proxy.ts | 13 ++- src/process-exit.ts | 10 +- src/registry/pricing.ts | 19 +++- tests/helpers/exit-probe.ts | 43 +++++++- tests/parent-notice-launch.test.ts | 22 +++- tests/process-exit.test.ts | 169 ++++++++++++++++++++++++++++- 8 files changed, 265 insertions(+), 31 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 4bb951cb..5248b6e0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -192,9 +192,9 @@ These bite from outside the subsystem that owns them, so they live here rather t - **Never call `process.exit()` in `src/`; end through `exitAfterDrain()` (`src/process-exit.ts`).** On Node 24 `process.exit()` can hang forever (nodejs/node#64274), so both bins set the exit code and let the event loop drain, falling back to `process.exit()` after one second. That only works - if nothing holds the loop: background work that nothing awaits (the models.dev refresh, a - WebSocket's closing handshake, outbound connections still being opened) registers - `cancelOnExit()` so it is cut off as `process.exit()` used to cut it off. Run with `CLODEX_TRACE=1` to see what held the loop when the fallback fires. + if nothing holds the loop: background work that nothing awaits (the models.dev refresh, the + pricing enrichment, a WebSocket's closing handshake, outbound connections still being opened) + registers `cancelOnExit()` so it is cut off as `process.exit()` used to cut it off. Run with `CLODEX_TRACE=1` to see what held the loop when the fallback fires. - **Every AI SDK generation entry point must resolve its timeout and retry budget through `src/upstream-retry.ts`.** Anthropic- and OpenAI-format `streamText` consumers abort at idle and total deadlines; `generateText` consumers abort at total only. Cancellation remains cooperative diff --git a/src/launch.ts b/src/launch.ts index 58bf6f92..b874ffb4 100644 --- a/src/launch.ts +++ b/src/launch.ts @@ -141,16 +141,14 @@ export function launchClaude( process.off('SIGTERM', forwardSigterm); }; - child.on('exit', (code) => { + // Both ways the child can end go through here, so neither can skip the cleanup. + const finish = (code: number): void => { stopForwarding(); restore(); - resolve(code ?? 0); - }); + resolve(code); + }; - child.on('error', (err) => { - stopForwarding(); - restore(); - resolve(1); - }); + child.on('exit', (code) => finish(code ?? 0)); + child.on('error', () => finish(1)); }); } diff --git a/src/outbound-proxy.ts b/src/outbound-proxy.ts index 4b9f568e..5652107d 100644 --- a/src/outbound-proxy.ts +++ b/src/outbound-proxy.ts @@ -150,7 +150,13 @@ let dispatcherInstalled = false; * `Agent.destroy()` cannot reach a socket it has not been handed yet. A peer that stalls the * handshake would therefore hold a requested exit until the fallback fires. process.exit() used * to cut these off; this does the same. Established connections are left to their owners, which - * abort or unref them. Two cases are not seen, and a stall there still ends through the exit + * abort or unref them. + * + * Each socket is destroyed WITH an error. undici settles a connection attempt only on connect or + * error, so a bare destroy() (which emits only 'close') would strand the request it was opened for: + * an un-aborted fetch would then never settle, and its own ref'd timeout would hold the exit + * instead. A no-op listener keeps the error from becoming an uncaught exception on a socket whose + * owner never listened for one. Two cases are not seen, and a stall there still ends through the exit * fallback: TLS sockets on Node 22, which does not report them on this channel, and the TLS * handshake inside an HTTP(S)_PROXY tunnel, whose socket is tracked only until the TCP connection * to the proxy is made. @@ -168,7 +174,10 @@ function abandonPendingConnectionsOnExit(): void { diagnosticsChannel.subscribe('net.client.socket', track); cancelOnExit(() => { diagnosticsChannel.unsubscribe('net.client.socket', track); - for (const socket of pending) socket.destroy(); + for (const socket of pending) { + socket.on('error', () => {}); + socket.destroy(Object.assign(new Error('clodex is exiting'), { code: 'ECONNABORTED' })); + } pending.clear(); }); } diff --git a/src/process-exit.ts b/src/process-exit.ts index 9f0ec4da..92ee5ecd 100644 --- a/src/process-exit.ts +++ b/src/process-exit.ts @@ -15,8 +15,8 @@ import { writeSync } from 'node:fs'; /** * How long a requested exit may wait for the event loop to drain before falling back to - * process.exit(). Measured drains finish in under a millisecond and never took more than about - * 60 ms, so this only matters when something unexpected holds the loop. One second keeps that case + * process.exit(). Most measured drains ended before a 1 ms poll could sample them, and none took + * more than about 60 ms, so this only matters when something unexpected holds the loop. One second keeps that case * short for someone at a terminal, while leaving room for the waits a drain can legitimately * include: an uncancellable in-flight DNS lookup or connection attempt, a pipe reader catching up on * buffered output (which process.exit() can cut short), and closing sockets finishing their @@ -78,6 +78,12 @@ export function exitAfterDrain(code: number, options: ExitAfterDrainOptions = {} if (exitRequested) return; exitRequested = true; process.exitCode = code; + // process.exit() discarded a stdio failure that arrived after the exit: a reader that has gone + // away reports EPIPE asynchronously, and unhandled it would turn this exit code into 1. + // Errors from before the request are untouched. + const ignoreStdioError = () => {}; + process.stdout.on('error', ignoreStdioError); + process.stderr.on('error', ignoreStdioError); const cancellers = [...exitCancellers]; exitCancellers.clear(); diff --git a/src/registry/pricing.ts b/src/registry/pricing.ts index 666edc42..d4c254f7 100644 --- a/src/registry/pricing.ts +++ b/src/registry/pricing.ts @@ -20,6 +20,7 @@ import { import { dirname, join } from 'node:path'; import bundledPricing from '../data/pricing-cache.json'; import { getAppHome } from '../paths.js'; +import { cancelOnExit } from '../process-exit.js'; import { getTemplateById } from '../provider-templates.js'; import { isRetainedOpenCodeGoProvider, retainedOpenCodeGoTemplate } from './resolve-template.js'; import type { CachedModel, RegistryModelsCache, RegistryProvider } from './types.js'; @@ -144,9 +145,12 @@ export function loadPricingCache(): PricingCacheFile { return readPricingFile(getUserPricingCachePath()) ?? loadBundledPricingCache(); } -export async function fetchPricingCache(): Promise { +export async function fetchPricingCache(signal?: AbortSignal): Promise { const controller = new AbortController(); const timer = setTimeout(() => controller.abort(), FETCH_TIMEOUT_MS); + const onCallerAbort = () => controller.abort(); + if (signal?.aborted) controller.abort(); + else signal?.addEventListener('abort', onCallerAbort, { once: true }); try { const response = await fetch(PRICING_API_URL, { signal: controller.signal, @@ -161,6 +165,7 @@ export async function fetchPricingCache(): Promise { return null; } finally { clearTimeout(timer); + signal?.removeEventListener('abort', onCallerAbort); } } @@ -300,10 +305,16 @@ export function applyCachedPricing(): boolean { }); } -/** Fetch latest pricing in the background; updates registry when complete. */ +/** + * Fetch latest pricing in the background; updates registry when complete. Nothing awaits this, + * so a requested exit abandons it, fetch and registry update alike, as process.exit() used to. + */ export function enrichPricingAsync(onComplete?: (updated: boolean) => void): void { + const controller = new AbortController(); + const unregister = cancelOnExit(() => controller.abort()); void (async () => { - const fetched = await fetchPricingCache(); + const fetched = await fetchPricingCache(controller.signal); + if (controller.signal.aborted) return; const cache = fetched ?? loadPricingCache(); const changed = await withRegistryWriteLock(() => { const registry = loadRegistryStrict(); @@ -312,7 +323,7 @@ export function enrichPricingAsync(onComplete?: (updated: boolean) => void): voi return updated; }); onComplete?.(changed); - })().catch(() => onComplete?.(false)); + })().catch(() => onComplete?.(false)).finally(unregister); } export function pricingPlatformForProvider(templateId: string, providerId: string): string | undefined { diff --git a/tests/helpers/exit-probe.ts b/tests/helpers/exit-probe.ts index 25407c18..dcbaa9e0 100644 --- a/tests/helpers/exit-probe.ts +++ b/tests/helpers/exit-probe.ts @@ -13,6 +13,7 @@ import { cancelOnExit, exitAfterDrain } from '../../src/process-exit.js'; import { installOutboundDispatcher } from '../../src/outbound-proxy.js'; import { MODELS_DEV_API_URL, refreshModelsDevCacheAsync } from '../../src/registry/models-dev.js'; +import { enrichPricingAsync, fetchPricingCache, PRICING_API_URL } from '../../src/registry/pricing.js'; const mode = process.argv[2] ?? ''; const upstreamUrl = process.argv[3] ?? ''; @@ -43,11 +44,11 @@ function holdLoop(): NodeJS.Timeout { return setInterval(() => {}, 60_000); } -/** Point the models.dev refresh at the test's server, recording that the fetch really started. */ -function redirectModelsDevFetch(onStart: () => void): void { +/** Point one public metadata URL at the test's server, recording that the fetch really started. */ +function redirectFetch(from: string, onStart: () => void): void { const realFetch = globalThis.fetch; globalThis.fetch = ((input: Parameters[0], init?: RequestInit) => { - if (String(input) !== MODELS_DEV_API_URL) return realFetch(input, init); + if (String(input) !== from) return realFetch(input, init); onStart(); return realFetch(upstreamUrl, init); }) as typeof fetch; @@ -94,7 +95,7 @@ async function run(): Promise { case 'models-dev-quick': { // A command like `models --list`: some real work, finished well before the refresh's start // delay. Without the delay its download would already be under way here. - redirectModelsDevFetch(() => say('FETCH-STARTED')); + redirectFetch(MODELS_DEV_API_URL, () => say('FETCH-STARTED')); refreshModelsDevCacheAsync(); await new Promise(resolve => setTimeout(resolve, 100)); requestExit(0, { graceMs: 30_000 }); @@ -104,7 +105,7 @@ async function run(): Promise { // A command still running when the refresh starts, ending while the download is in flight. // The CLI's own fetch dispatcher, as main() installs it, owns the connection. await installOutboundDispatcher(); - const started = new Promise(resolve => redirectModelsDevFetch(() => { + const started = new Promise(resolve => redirectFetch(MODELS_DEV_API_URL, () => { say('FETCH-STARTED'); resolve(); })); @@ -118,6 +119,38 @@ async function run(): Promise { requestExit(0, { graceMs: 10_000, trace: true }); return; } + case 'pricing-in-flight': { + // Pricing enrichment whose request is established and unanswered when the exit is requested. + await installOutboundDispatcher(); + const started = new Promise(resolve => redirectFetch(PRICING_API_URL, () => { + say('FETCH-STARTED'); + resolve(); + })); + // Reports only if the enrichment carried on to its registry update after the exit. + enrichPricingAsync(updated => say(`ENRICHMENT-FINISHED updated=${updated}`)); + const command = holdLoop(); + await started; + await new Promise(resolve => setTimeout(resolve, 200)); + clearInterval(command); + requestExit(0, { graceMs: 10_000, trace: true }); + return; + } + case 'unowned-fetch-connecting': { + // A fetch nobody aborts, with its own ref'd timeout (fetchPricingCache called without a + // signal), still opening its connection when the exit is requested. + await installOutboundDispatcher(); + const started = new Promise(resolve => redirectFetch(PRICING_API_URL, () => { + say('FETCH-STARTED'); + resolve(); + })); + void fetchPricingCache().then(result => say(`FETCH-SETTLED ${result === null ? 'null' : 'data'}`)); + const command = holdLoop(); + await started; + await new Promise(resolve => setTimeout(resolve, 200)); + clearInterval(command); + requestExit(0, { graceMs: 10_000, trace: true }); + return; + } default: say(`unknown mode ${mode}`); process.exitCode = 99; diff --git a/tests/parent-notice-launch.test.ts b/tests/parent-notice-launch.test.ts index 80526d79..984da4f1 100644 --- a/tests/parent-notice-launch.test.ts +++ b/tests/parent-notice-launch.test.ts @@ -61,6 +61,21 @@ async function buildProbe(): Promise { } const live = new Set(); +/** Children spawned as their own process group, whose descendants must die with them. */ +const groupLeaders = new WeakSet(); + +/** SIGKILL a child, and its whole process group when it leads one. */ +function kill(child: ChildProcess): void { + if (groupLeaders.has(child) && child.pid !== undefined) { + try { + process.kill(-child.pid, 'SIGKILL'); + return; + } catch { + // Group already gone; fall through to the child itself. + } + } + child.kill('SIGKILL'); +} function track(child: ChildProcess): ChildProcess { live.add(child); @@ -72,7 +87,7 @@ function track(child: ChildProcess): ChildProcess { function awaitExit(child: ChildProcess): Promise { return new Promise((resolve, reject) => { const timer = setTimeout(() => { - child.kill('SIGKILL'); + kill(child); reject(new Error('probe timed out')); }, PROBE_TIMEOUT_MS); timer.unref(); @@ -128,10 +143,13 @@ describe('parent notices while Claude Code owns the terminal', () => { const quoted = [process.execPath, PROBE, dir, '--epipe'] .map(part => `"${part}"`).join(' '); const script = `{ ${quoted}; echo "$?" >&3; } 3>"${statusPath}" 2>&1 | head -n 1`; + // Its own process group: on timeout the nested Node probe must die too, not just the shell. const child = track(spawn('/bin/sh', ['-c', script], { stdio: ['ignore', 'ignore', 'ignore'], env: { ...process.env, CLODEX_HOME: join(dir, 'home') }, + detached: true, })); + groupLeaders.add(child); await awaitExit(child); return readFileSync(statusPath, 'utf8').trim(); } @@ -151,7 +169,7 @@ describe('parent notices while Claude Code owns the terminal', () => { afterAll(() => { // Kill anything still alive BEFORE removing the scratch tree: the fake claude // polls for a release file inside it, so deleting first would strand it. - for (const child of live) child.kill('SIGKILL'); + for (const child of live) kill(child); live.clear(); rmSync(root, { recursive: true, force: true }); rmSync(PROBE, { force: true }); diff --git a/tests/process-exit.test.ts b/tests/process-exit.test.ts index 5ea96d94..ec76c955 100644 --- a/tests/process-exit.test.ts +++ b/tests/process-exit.test.ts @@ -8,14 +8,18 @@ // tests/helpers/exit-probe.ts in a plain Node process, bundled with tsup as // tests/parent-notice-launch.test.ts does, and asserts on how it reported its own end. import { type ChildProcess, spawn, spawnSync } from 'node:child_process'; -import { chmodSync, existsSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { chmodSync, existsSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; import { createServer, type Server, type Socket } from 'node:net'; import { tmpdir } from 'node:os'; import { dirname, join } from 'node:path'; import { fileURLToPath } from 'node:url'; import { afterAll, beforeAll, describe, expect, it } from 'vitest'; import { build } from 'tsup'; +import { createServer as createHttpServer } from 'node:http'; +import { CHILD_NETWORK_ENV_VARS } from '../src/network-env.js'; import { describeActiveResources } from '../src/process-exit.js'; +import { MODELS_DEV_API_URL } from '../src/registry/models-dev.js'; +import { PRICING_API_URL } from '../src/registry/pricing.js'; import { BOUNDED_NODE_CHILD } from './helpers/bounded-child.js'; const PROBE_SOURCE = fileURLToPath(new URL('./helpers/exit-probe.ts', import.meta.url)); @@ -43,11 +47,21 @@ function afterRequestMs(run: ProbeRun): number { const live = new Set(); let home: string; +/** + * The child environment without the ambient proxy settings, so the CLI's dispatcher is the plain + * Agent these cases exercise and loopback fixtures are reached directly. + */ +function hermeticEnv(overrides: NodeJS.ProcessEnv): NodeJS.ProcessEnv { + const env = { ...process.env, ...overrides }; + for (const name of CHILD_NETWORK_ENV_VARS) delete env[name]; + return env; +} + function runProbe(mode: string, extraArgs: string[] = []): Promise { return new Promise((resolve, reject) => { const child = spawn(process.execPath, [PROBE, mode, ...extraArgs], { stdio: ['ignore', 'ignore', 'pipe'], - env: { ...process.env, CLODEX_HOME: home, CLODEX_TRACE: '' }, + env: hermeticEnv({ CLODEX_HOME: home, CLODEX_TRACE: '' }), }); live.add(child); let stderr = ''; @@ -218,6 +232,125 @@ describe('the background models.dev refresh', () => { }, 25_000); }); +describe('the background pricing enrichment', () => { + // Started by provider add and refresh. The interactive provider hub keeps running after it starts, + // so its request can be established and waiting on a stalled server when the user leaves. + it('cannot hold the process open once its request is in flight', async () => { + const upstream = await startSilentServer(); + try { + const run = await runProbe('pricing-in-flight', [upstream.url]); + expect(run.stderr).toContain('FETCH-STARTED'); + expect(upstream.requests()).toBeGreaterThan(0); + // Abandoned outright, as process.exit() would: no registry update while the process exits. + expect(run.stderr).not.toContain('ENRICHMENT-FINISHED'); + expect(run.stderr).not.toContain('event loop still busy'); + expect(run.stderr).toContain('EXITED via=drain code=0'); + } finally { + await new Promise(resolve => upstream.server.close(() => resolve())); + } + }, 25_000); +}); + +describe('outbound connections still opening at exit', () => { + it('fail the fetch that opened them, so its own timeout cannot hold the exit', async () => { + const upstream = await startSilentServer(); + try { + // https against a server that never completes TLS: the connection is still opening. + const run = await runProbe('unowned-fetch-connecting', [upstream.url.replace('http:', 'https:')]); + expect(run.stderr).toContain('FETCH-STARTED'); + expect(upstream.requests()).toBeGreaterThan(0); + // A connection destroyed without an error strands the fetch: it never settles and its 15 s + // timer holds the loop until the 10 s fallback. + expect(run.stderr).toContain('FETCH-SETTLED null'); + expect(run.stderr).not.toContain('event loop still busy'); + expect(run.stderr).toContain('EXITED via=drain code=0'); + } finally { + await new Promise(resolve => upstream.server.close(() => resolve())); + } + }, 25_000); +}); + +/** + * `clodex providers refresh-models` on a loopback custom-openai provider: the real successful + * refresh path, which ends by starting the background pricing enrichment that nothing awaits. + * A preload points the two public metadata URLs (pricing, models.dev) at the same loopback server. + */ +describe('clodex providers refresh-models', () => { + async function refreshAgainstLoopback() { + const scratch = mkdtempSync(join(tmpdir(), 'clodex-exit-refresh-')); + const hits: string[] = []; + const server = createHttpServer((req, res) => { + hits.push(req.url ?? ''); + if (req.url === '/v1/models') { + res.writeHead(200, { 'content-type': 'application/json' }); + res.end(JSON.stringify({ data: [{ id: 'loop-model', object: 'model' }] })); + } else if (req.url === '/pricing') { + res.writeHead(200, { 'content-type': 'application/json' }); + res.end(JSON.stringify({ models: [] })); + } else { + res.writeHead(404).end(); + } + }); + await new Promise(resolve => server.listen(0, '127.0.0.1', resolve)); + const address = server.address(); + const url = `http://127.0.0.1:${typeof address === 'object' && address ? address.port : 0}`; + try { + const clodexHome = join(scratch, 'home'); + mkdirSync(clodexHome); + writeFileSync(join(clodexHome, 'providers.json'), JSON.stringify({ + schemaVersion: 1, + providers: [{ + id: 'loop', templateId: 'custom-openai', name: 'Loop', enabled: true, + authRef: 'none:anonymous', authType: 'none', + api: { npm: '@ai-sdk/openai-compatible', url: `${url}/v1` }, + addedAt: '2026-10-10T00:00:00.000Z', + }], + })); + const redirect = join(scratch, 'redirect-metadata.mjs'); + writeFileSync(redirect, [ + 'const realFetch = globalThis.fetch;', + 'const routes = {', + ` ${JSON.stringify(PRICING_API_URL)}: '/pricing',`, + ` ${JSON.stringify(MODELS_DEV_API_URL)}: '/models-dev',`, + '};', + 'globalThis.fetch = (input, init) => {', + ' const path = routes[String(input)];', + ` return realFetch(path ? ${JSON.stringify(url)} + path : input, init);`, + '};', + '', + ].join('\n')); + const child = spawn(process.execPath, [ + '--import', writeExitWitness(scratch), '--import', redirect, + BUILT_CLI, 'providers', 'refresh-models', 'loop', + ], { + stdio: ['ignore', 'pipe', 'pipe'], + env: hermeticEnv({ CLODEX_HOME: clodexHome, HOME: scratch, CLODEX_TRACE: '1' }), + }); + live.add(child); + let output = ''; + child.stdout!.on('data', chunk => { output += String(chunk); }); + child.stderr!.on('data', chunk => { output += String(chunk); }); + const killer = setTimeout(() => child.kill('SIGKILL'), PROBE_TIMEOUT_MS); + const status = await new Promise(resolve => child.on('close', code => resolve(code))); + clearTimeout(killer); + live.delete(child); + return { status, output, hits }; + } finally { + server.closeAllConnections(); + await new Promise(resolve => server.close(() => resolve())); + rmSync(scratch, { recursive: true, force: true }); + } + } + + it('drains naturally right after starting the pricing enrichment', async () => { + const run = await refreshAgainstLoopback(); + expect(run.hits).toContain('/v1/models'); + expect(run.output).not.toContain('event loop still busy'); + expect(run.output).toContain('EXIT-WITNESS drained(0)'); + expect(run.status).toBe(0); + }, 25_000); +}); + describe('describeActiveResources', () => { it('groups resources by type with counts', () => { expect(describeActiveResources(['TCPSocketWrap', 'Timeout', 'TCPSocketWrap'])).toBe('TCPSocketWrap x2, Timeout'); @@ -260,15 +393,14 @@ describe('both bins end by draining the event loop', () => { const run = spawnSync(process.execPath, ['--import', witness, bin, ...args], { encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'], - env: { - ...process.env, + env: hermeticEnv({ CLODEX_HOME: join(scratch, 'home'), HOME: scratch, CLODEX_TRACE: '', CLODEX_CLAUDE_PATH: '', PATH: '/usr/bin:/bin', ...env, - }, + }), ...BOUNDED_NODE_CHILD, }); return { status: run.status, stderr: run.stderr, error: run.error, scratch }; @@ -324,6 +456,33 @@ describe('both bins end by draining the event loop', () => { ]); }); + it.skipIf(process.platform === 'win32')('clodex-claude keeps its 127 when nobody is reading its stderr', async () => { + // `clodex-claude … 2>&1 | true`: the reader is gone before the diagnostic is written. The + // write fails with EPIPE asynchronously, after the exit was requested; process.exit() used to + // end the process first, and an unhandled stream error would now turn 127 into 1. + const scratch = mkdtempSync(join(tmpdir(), 'clodex-exit-epipe-')); + try { + const child = spawn(process.execPath, [BUILT_WRAPPER, '-p', 'hi'], { + stdio: ['ignore', 'ignore', 'pipe'], + env: hermeticEnv({ + CLODEX_HOME: join(scratch, 'home'), + HOME: scratch, + CLODEX_CLAUDE_PATH: '', + PATH: '/usr/bin:/bin', + }), + }); + live.add(child); + child.stderr!.destroy(); + const killer = setTimeout(() => child.kill('SIGKILL'), PROBE_TIMEOUT_MS); + const status = await new Promise(resolve => child.on('close', code => resolve(code))); + clearTimeout(killer); + live.delete(child); + expect(status).toBe(127); + } finally { + rmSync(scratch, { recursive: true, force: true }); + } + }); + it.skipIf(process.platform === 'win32')('clodex-claude when a server is required and none is live', () => { const scratch = mkdtempSync(join(tmpdir(), 'clodex-exit-claude-')); try { From 71301e6eb41f304deb5bb89a68f026e6b4dcf191 Mon Sep 17 00:00:00 2001 From: Brandon Wallace Date: Sat, 10 Oct 2026 14:18:07 -0500 Subject: [PATCH 3/3] fix(cli): stop a busy provider list from delaying clodex's exit after a refresh When another clodex process held the provider registry, the background pricing update waited for it after its download finished. That wait could not be interrupted, so a refresh that ended then fell back to the one-second forced exit, and could still write pricing after the exit was requested. withRegistryWriteLock now accepts an optional AbortSignal: it checks the signal before each attempt and abandons its retry sleep when the signal aborts, rejecting with the signal's reason. Callers that pass no signal behave exactly as before. The pricing enrichment passes its exit signal, re-checks it inside the lock before touching the registry, and reports no completion after an exit. --- src/registry/lock.ts | 28 ++++++++++++++--- src/registry/pricing.ts | 19 +++++++---- tests/helpers/exit-probe.ts | 20 +++++++++++- tests/process-exit.test.ts | 63 +++++++++++++++++++++++++++++++++++-- tests/registry-lock.test.ts | 33 +++++++++++++++++++ 5 files changed, 150 insertions(+), 13 deletions(-) diff --git a/src/registry/lock.ts b/src/registry/lock.ts index 7a66be37..063770ad 100644 --- a/src/registry/lock.ts +++ b/src/registry/lock.ts @@ -41,6 +41,14 @@ interface RegistryLockOptions { isAlive?: (pid: number) => boolean; } +interface RegistryWriteLockOptions extends RegistryLockOptions { + /** + * Abandons the wait for the lock: once aborted, the returned promise rejects with the signal's + * reason instead of acquiring. It never interrupts an operation that already holds the lock. + */ + signal?: AbortSignal; +} + interface RegistryLockContext { leases: ReadonlyMap; } @@ -308,8 +316,19 @@ export function tryAcquireRegistryLock( return null; } -function sleep(ms: number): Promise { - return new Promise((resolve) => setTimeout(resolve, ms)); +function sleep(ms: number, signal?: AbortSignal): Promise { + if (!signal) return new Promise((resolve) => setTimeout(resolve, ms)); + return new Promise((resolve, reject) => { + const onAbort = () => { + clearTimeout(timer); + reject(signal.reason); + }; + const timer = setTimeout(() => { + signal.removeEventListener('abort', onAbort); + resolve(); + }, ms); + signal.addEventListener('abort', onAbort, { once: true }); + }); } function sleepSync(ms: number): void { @@ -340,7 +359,7 @@ function lockTimeoutError( export async function withRegistryWriteLock( operation: () => Promise | T, - options: RegistryLockOptions = {}, + options: RegistryWriteLockOptions = {}, ): Promise { const lockPath = options.lockPath ?? getRegistryLockPath(); const inheritedLeases = registryLockContext.getStore()?.leases; @@ -353,6 +372,7 @@ export async function withRegistryWriteLock( let lease: RegistryLockLease | null = null; while (!lease) { + options.signal?.throwIfAborted(); lease = tryAcquireRegistryLock(lockPath, { now, isAlive: options.isAlive, @@ -360,7 +380,7 @@ export async function withRegistryWriteLock( if (lease) break; if (now() >= deadline) throw lockTimeoutError(lockPath, waitMs, options.isAlive ?? isPidAlive); - await sleep(retryMs); + await sleep(retryMs, options.signal); } const leases = new Map(inheritedLeases); diff --git a/src/registry/pricing.ts b/src/registry/pricing.ts index d4c254f7..80508e19 100644 --- a/src/registry/pricing.ts +++ b/src/registry/pricing.ts @@ -307,23 +307,30 @@ export function applyCachedPricing(): boolean { /** * Fetch latest pricing in the background; updates registry when complete. Nothing awaits this, - * so a requested exit abandons it, fetch and registry update alike, as process.exit() used to. + * so a requested exit abandons it, as process.exit() used to: the fetch, the wait for the registry + * lock (another clodex process may hold it), and the registry update. */ export function enrichPricingAsync(onComplete?: (updated: boolean) => void): void { const controller = new AbortController(); + const { signal } = controller; const unregister = cancelOnExit(() => controller.abort()); void (async () => { - const fetched = await fetchPricingCache(controller.signal); - if (controller.signal.aborted) return; + const fetched = await fetchPricingCache(signal); + if (signal.aborted) return; const cache = fetched ?? loadPricingCache(); const changed = await withRegistryWriteLock(() => { + // The wait above already stops on abort; this keeps an update from starting under a lease + // granted after the exit was requested, should acquiring ever yield before the operation. + if (signal.aborted) return undefined; const registry = loadRegistryStrict(); const updated = applyPricingToRegistryProviders(registry, cache); if (updated) saveRegistry(registry); return updated; - }); - onComplete?.(changed); - })().catch(() => onComplete?.(false)).finally(unregister); + }, { signal }); + if (changed !== undefined) onComplete?.(changed); + })().catch(() => { + if (!signal.aborted) onComplete?.(false); + }).finally(unregister); } export function pricingPlatformForProvider(templateId: string, providerId: string): string | undefined { diff --git a/tests/helpers/exit-probe.ts b/tests/helpers/exit-probe.ts index dcbaa9e0..8965930f 100644 --- a/tests/helpers/exit-probe.ts +++ b/tests/helpers/exit-probe.ts @@ -50,7 +50,9 @@ function redirectFetch(from: string, onStart: () => void): void { globalThis.fetch = ((input: Parameters[0], init?: RequestInit) => { if (String(input) !== from) return realFetch(input, init); onStart(); - return realFetch(upstreamUrl, init); + const response = realFetch(upstreamUrl, init); + void response.then(() => say('FETCH-ANSWERED'), () => {}); + return response; }) as typeof fetch; } @@ -135,6 +137,22 @@ async function run(): Promise { requestExit(0, { graceMs: 10_000, trace: true }); return; } + case 'pricing-lock-contended': { + // The pricing fetch completes; the registry lock it then needs is held by the test process. + await installOutboundDispatcher(); + const answered = new Promise(resolve => redirectFetch(PRICING_API_URL, () => { + say('FETCH-STARTED'); + resolve(); + })); + enrichPricingAsync(updated => say(`ENRICHMENT-FINISHED updated=${updated}`)); + const command = holdLoop(); + await answered; + // Long enough for the answer to arrive and the enrichment to start retrying the lock. + await new Promise(resolve => setTimeout(resolve, 300)); + clearInterval(command); + requestExit(0, { graceMs: 5_000, trace: true }); + return; + } case 'unowned-fetch-connecting': { // A fetch nobody aborts, with its own ref'd timeout (fetchPricingCache called without a // signal), still opening its connection when the exit is requested. diff --git a/tests/process-exit.test.ts b/tests/process-exit.test.ts index ec76c955..b9983d7c 100644 --- a/tests/process-exit.test.ts +++ b/tests/process-exit.test.ts @@ -20,6 +20,7 @@ import { CHILD_NETWORK_ENV_VARS } from '../src/network-env.js'; import { describeActiveResources } from '../src/process-exit.js'; import { MODELS_DEV_API_URL } from '../src/registry/models-dev.js'; import { PRICING_API_URL } from '../src/registry/pricing.js'; +import { tryAcquireRegistryLock } from '../src/registry/lock.js'; import { BOUNDED_NODE_CHILD } from './helpers/bounded-child.js'; const PROBE_SOURCE = fileURLToPath(new URL('./helpers/exit-probe.ts', import.meta.url)); @@ -57,17 +58,22 @@ function hermeticEnv(overrides: NodeJS.ProcessEnv): NodeJS.ProcessEnv { return env; } -function runProbe(mode: string, extraArgs: string[] = []): Promise { +function runProbe( + mode: string, + extraArgs: string[] = [], + options: { clodexHome?: string; onStderr?: (soFar: string) => void } = {}, +): Promise { return new Promise((resolve, reject) => { const child = spawn(process.execPath, [PROBE, mode, ...extraArgs], { stdio: ['ignore', 'ignore', 'pipe'], - env: hermeticEnv({ CLODEX_HOME: home, CLODEX_TRACE: '' }), + env: hermeticEnv({ CLODEX_HOME: options.clodexHome ?? home, CLODEX_TRACE: '' }), }); live.add(child); let stderr = ''; child.stderr!.setEncoding('utf8'); child.stderr!.on('data', (chunk: string) => { stderr += chunk; + options.onStderr?.(stderr); }); const timer = setTimeout(() => { child.kill('SIGKILL'); @@ -251,6 +257,59 @@ describe('the background pricing enrichment', () => { }, 25_000); }); +/** + * The enrichment's fetch has finished and it is waiting for the provider-registry lock, which + * another clodex process holds (the test process takes the real lock lease here). + */ +describe('the background pricing enrichment waiting on a held registry lock', () => { + async function runWithLockHeld(release: 'never' | 'soon-after-exit') { + const scratch = mkdtempSync(join(tmpdir(), 'clodex-exit-lock-')); + const server = createHttpServer((_req, res) => { + res.writeHead(200, { 'content-type': 'application/json' }); + res.end(JSON.stringify({ models: [] })); + }); + await new Promise(resolve => server.listen(0, '127.0.0.1', resolve)); + const address = server.address(); + const url = `http://127.0.0.1:${typeof address === 'object' && address ? address.port : 0}/pricing`; + const lease = tryAcquireRegistryLock(join(scratch, 'providers.json.lock')); + expect(lease).not.toBeNull(); + let releaseTimer: NodeJS.Timeout | undefined; + try { + return await runProbe('pricing-lock-contended', [url], { + clodexHome: scratch, + onStderr: soFar => { + if (release === 'soon-after-exit' && !releaseTimer && soFar.includes('EXIT-REQUESTED')) { + releaseTimer = setTimeout(() => lease!.release(), 200); + } + }, + }); + } finally { + clearTimeout(releaseTimer); + lease!.release(); + server.closeAllConnections(); + await new Promise(resolve => server.close(() => resolve())); + rmSync(scratch, { recursive: true, force: true }); + } + } + + it('stops waiting for the lock when the exit is requested', async () => { + const run = await runWithLockHeld('never'); + expect(run.stderr).toContain('FETCH-ANSWERED'); + // The lock is still held when the probe ends: only an abandoned wait lets it drain. + expect(run.stderr).not.toContain('event loop still busy'); + expect(run.stderr).toContain('EXITED via=drain code=0'); + expect(run.stderr).not.toContain('ENRICHMENT-FINISHED'); + }, 25_000); + + it('does not update the registry once the lock comes free after the exit', async () => { + const run = await runWithLockHeld('soon-after-exit'); + expect(run.stderr).toContain('FETCH-ANSWERED'); + expect(run.stderr).not.toContain('ENRICHMENT-FINISHED'); + expect(run.stderr).not.toContain('event loop still busy'); + expect(run.stderr).toContain('EXITED via=drain code=0'); + }, 25_000); +}); + describe('outbound connections still opening at exit', () => { it('fail the fetch that opened them, so its own timeout cannot hold the exit', async () => { const upstream = await startSilentServer(); diff --git a/tests/registry-lock.test.ts b/tests/registry-lock.test.ts index 342017dd..09fdffa4 100644 --- a/tests/registry-lock.test.ts +++ b/tests/registry-lock.test.ts @@ -320,6 +320,39 @@ describe('provider registry lock', () => { lease?.release(); }); + it('abandons the wait for a held lock as soon as its signal aborts', async () => { + const lockPath = temporaryLockPath(); + const holder = tryAcquireRegistryLock(lockPath); + expect(holder).not.toBeNull(); + const controller = new AbortController(); + const operation = vi.fn(); + try { + // A retry interval far beyond the test timeout: only an abortable wait can end this. + const waiting = withRegistryWriteLock(operation, { + lockPath, waitMs: 120_000, retryMs: 60_000, signal: controller.signal, + }); + setTimeout(() => controller.abort(new Error('exiting')), 20); + await expect(waiting).rejects.toThrow('exiting'); + expect(operation).not.toHaveBeenCalled(); + } finally { + holder?.release(); + } + }); + + it('does not take a free lock once its signal has aborted', async () => { + const lockPath = temporaryLockPath(); + const controller = new AbortController(); + controller.abort(new Error('exiting')); + const operation = vi.fn(); + + await expect(withRegistryWriteLock(operation, { lockPath, signal: controller.signal })) + .rejects.toThrow('exiting'); + expect(operation).not.toHaveBeenCalled(); + const lease = tryAcquireRegistryLock(lockPath); + expect(lease).toMatchObject({ active: true }); + lease?.release(); + }); + it('serializes independent asynchronous callers', async () => { const lockPath = temporaryLockPath(); const events: string[] = [];