diff --git a/CLAUDE.md b/CLAUDE.md index 938d6138..5248b6e0 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, 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/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..b874ffb4 100644 --- a/src/launch.ts +++ b/src/launch.ts @@ -125,21 +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) => { + // 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) => { - restore(); - resolve(1); - }); + child.on('exit', (code) => finish(code ?? 0)); + child.on('error', () => finish(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..5652107d 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,46 @@ 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. + * + * 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. + */ +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.on('error', () => {}); + socket.destroy(Object.assign(new Error('clodex is exiting'), { code: 'ECONNABORTED' })); + } + pending.clear(); + }); +} + /** Reset the install-once latch (tests only). */ export function resetOutboundDispatcherForTests(): void { dispatcherInstalled = false; @@ -160,6 +204,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..92ee5ecd --- /dev/null +++ b/src/process-exit.ts @@ -0,0 +1,110 @@ +// 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(). 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 + * 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; + // 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(); + 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/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/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/src/registry/pricing.ts b/src/registry/pricing.ts index 666edc42..80508e19 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,19 +305,32 @@ 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, 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(); + 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)); + }, { 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 new file mode 100644 index 00000000..8965930f --- /dev/null +++ b/tests/helpers/exit-probe.ts @@ -0,0 +1,178 @@ +// 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'; +import { enrichPricingAsync, fetchPricingCache, PRICING_API_URL } from '../../src/registry/pricing.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 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) !== from) return realFetch(input, init); + onStart(); + const response = realFetch(upstreamUrl, init); + void response.then(() => say('FETCH-ANSWERED'), () => {}); + return response; + }) 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. + redirectFetch(MODELS_DEV_API_URL, () => 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 => redirectFetch(MODELS_DEV_API_URL, () => { + 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; + } + 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 '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. + 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; + } +} + +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..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 }); @@ -167,6 +185,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..b9983d7c --- /dev/null +++ b/tests/process-exit.test.ts @@ -0,0 +1,608 @@ +// 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, 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 { 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)); +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; + +/** + * 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[] = [], + 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: 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'); + 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('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); +}); + +/** + * 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(); + 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'); + }); + + 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: 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 }; + } 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 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 { + 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/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[] = []; 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(); + }); +});