diff --git a/.changeset/bounded-openai-timeouts.md b/.changeset/bounded-openai-timeouts.md new file mode 100644 index 0000000..2f5a323 --- /dev/null +++ b/.changeset/bounded-openai-timeouts.md @@ -0,0 +1,11 @@ +--- +"@planningo/duul": patch +--- + +Fix silent multi-minute hangs in the OpenAI/Codex reviewer path + +- Configure the OpenAI client with `timeout: 120s, maxRetries: 0` so the SDK's defaults (600s timeout + 2 silent internal retries) can no longer stretch one hung request into 30 minutes of unlogged silence. +- Classify abort/connection failures (no HTTP status) as retryable — the SDK's `APIUserAbortError`/`APIConnectionError` keep `name: 'Error'`, so the old `name === 'AbortError'` check never retried them. +- Race the stateless (ChatGPT backend) stream against the 120s abort and abort the SDK stream controller in `finally`, as a backstop for mid-SSE stalls. +- Bound the Codex OAuth token refresh fetch with a 30s timeout — it runs outside the review AbortController and could hang the whole review silently. +- Log before each API call and tool execution, and log the previously-silent tool-loop continue paths (cache hit / repeat limit / budget block), so a stall now names its await. diff --git a/src/__tests__/openai-stream-timeout.test.ts b/src/__tests__/openai-stream-timeout.test.ts new file mode 100644 index 0000000..ddc1c74 --- /dev/null +++ b/src/__tests__/openai-stream-timeout.test.ts @@ -0,0 +1,40 @@ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { OpenAIProvider } from '../services/providers/openai.js'; + +// Regression test: a stateless (ChatGPT backend) stream whose SSE connection +// hangs must be cut off by the review timeout instead of awaiting forever. +// The fake stream ignores the request signal — like a hung connection — so +// only the explicit Promise.race in apiCallWithRetry can unblock the call. +function hangingStream() { + let rejectHang: (err: Error) => void = () => {}; + const hang = new Promise((_, reject) => { + rejectHang = reject; + }); + return { + aborted: false, + abort() { + this.aborted = true; + rejectHang(Object.assign(new Error('Request was aborted.'), { name: 'AbortError' })); + }, + async *[Symbol.asyncIterator]() { + await hang; + }, + }; +} + +test('stateless stream that never ends is aborted at the review deadline', async () => { + const provider = new OpenAIProvider({ chatgpt: { accessToken: 'tok', accountId: 'acct' } }); + const stream = hangingStream(); + (provider as unknown as { client: unknown }).client = { responses: { stream: () => stream } }; + + const start = Date.now(); + await assert.rejects( + (provider as unknown as { + apiCallWithRetry(params: Record, deadline: number): Promise; + }).apiCallWithRetry({}, Date.now() + 300), + /aborted|deadline/i, + ); + assert.ok(Date.now() - start < 5_000, 'call must fail near the deadline, not hang'); + assert.equal(stream.aborted, true, 'underlying stream must be aborted'); +}); diff --git a/src/services/providers/codex-auth.ts b/src/services/providers/codex-auth.ts index 82fb4d7..0d55857 100644 --- a/src/services/providers/codex-auth.ts +++ b/src/services/providers/codex-auth.ts @@ -114,6 +114,9 @@ export async function refreshCodexToken(auth: CodexAuth): Promise { const res = await fetch(OAUTH_TOKEN_URL, { method: 'POST', + // This runs outside the review loop's AbortController; without its own + // timeout a stalled OAuth endpoint hangs the whole review silently. + signal: AbortSignal.timeout(30_000), headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ client_id: OAUTH_CLIENT_ID, diff --git a/src/services/providers/openai.ts b/src/services/providers/openai.ts index 46d3c57..552dc9c 100644 --- a/src/services/providers/openai.ts +++ b/src/services/providers/openai.ts @@ -257,6 +257,10 @@ export class OpenAIProvider implements ReviewerProvider { private buildClient(apiKey: string): OpenAI { return new OpenAI({ apiKey, + // duul owns retries and timeouts. SDK defaults (600s timeout + 2 silent + // internal retries) can stretch one hung request into 30min of silence. + timeout: 120_000, + maxRetries: 0, ...(this.baseURL ? { baseURL: this.baseURL } : {}), ...(this.defaultHeaders ? { defaultHeaders: this.defaultHeaders } : {}), }); @@ -409,21 +413,25 @@ export class OpenAIProvider implements ReviewerProvider { callCounts.set(cacheKey, count); if (count > MAX_REPEAT_CALLS) { + console.error(`[duul] ${call.name}(${argSummary}) -> repeat limit (${count} calls)`); toolResults.push({ type: 'function_call_output' as const, call_id: call.call_id, output: 'You have already read this content multiple times. Use the context you already have to complete your review.' }); continue; } if (toolCache.has(cacheKey)) { + console.error(`[duul] ${call.name}(${argSummary}) -> cache hit`); toolResults.push({ type: 'function_call_output' as const, call_id: call.call_id, output: toolCache.get(cacheKey)! }); continue; } const currentLevel = getStrategyLevel(); if (!isToolAllowed(call.name, currentLevel)) { + console.error(`[duul] ${call.name}(${argSummary}) -> blocked (budget level ${currentLevel})`); toolResults.push({ type: 'function_call_output' as const, call_id: call.call_id, output: budgetMessage(call.name, currentLevel) }); continue; } + console.error(`[duul] → ${call.name}(${argSummary})`); const result = await executeFilesystemTool(effectiveRoot, call.name, args, workspaceScope, byteBudget); toolCache.set(cacheKey, result); allUsedTools.push(`${call.name}(${argSummary})`); @@ -500,7 +508,9 @@ export class OpenAIProvider implements ReviewerProvider { const remaining = remainingReviewMs(deadline); if (remaining === 0) throw new ReviewTimeoutError(); const controller = new AbortController(); - const timeout = setTimeout(() => controller.abort(), Math.min(120_000, remaining)); + const timeoutMs = Math.min(120_000, remaining); + const timeout = setTimeout(() => controller.abort(), timeoutMs); + console.error(`[duul] → openai ${this.stateless ? 'stream' : 'create'} (attempt ${attempt + 1}/${MAX_RETRIES}, timeout ${timeoutMs}ms)`); try { let response: OpenAI.Responses.Response; if (this.stateless) { @@ -510,7 +520,22 @@ export class OpenAIProvider implements ReviewerProvider { params as Parameters[0], { signal: controller.signal }, ); - response = await this.aggregateStream(stream); + // The SDK does not reliably propagate the abort signal into a hung + // SSE iteration, so the 120s timeout must race the stream explicitly. + const aggregated = this.aggregateStream(stream); + aggregated.catch(() => {}); // late rejection after a lost race is expected + try { + response = await Promise.race([ + aggregated, + new Promise((_, reject) => + controller.signal.addEventListener('abort', () => + reject(Object.assign(new Error(`OpenAI stream aborted after ${timeoutMs}ms`), { name: 'AbortError' })), + { once: true }), + ), + ]); + } finally { + if (!stream.aborted) stream.abort(); + } } else { response = (await this.client.responses.create( { ...params, stream: false } as Parameters[0], @@ -538,7 +563,10 @@ export class OpenAIProvider implements ReviewerProvider { } } - const isRetryable = error instanceof Error && (status !== undefined ? (status === 429 || status >= 500) : error.name === 'AbortError'); + // No HTTP status = abort/connection failure. The SDK's APIUserAbortError + // and APIConnectionError keep name 'Error', so match on the missing + // status rather than error.name. + const isRetryable = error instanceof Error && (status === undefined || status === 429 || status >= 500); if (isRetryable && attempt < MAX_RETRIES - 1) { const delay = 1000 * Math.pow(2, attempt); console.error(`[duul] Retry ${attempt + 1}/${MAX_RETRIES} after ${delay}ms`);