diff --git a/README.md b/README.md index f853ab8..615c1f0 100644 --- a/README.md +++ b/README.md @@ -144,6 +144,15 @@ It must support OpenAI-style function tools. Tool calling is required: an endpoint that rejects tools fails clearly rather than producing a diff-only review that looks complete. +Each logical non-streaming model call has one fixed ten-minute deadline shared +across all attempts. A timeout or abort is terminal and is never duplicated; +fast connection failures, 429s and 5xx responses retry only while their request +and backoff fit inside the remaining budget. Consumer workflow jobs must leave +enough time for the complete multi-call review, and proxies must not impose a +shorter upstream deadline. The larger bound is intentional for high-reasoning +models; the shared deadline and workflow timeout remain the outer backstops for +a stalled provider. + Structured Outputs are used when supported. Endpoints vary on `response_format`, `max_tokens` and `temperature`, so commitreview adapts those optional parameters when an endpoint explicitly rejects them. Tool calling is diff --git a/src/config.js b/src/config.js index 5798914..921fccd 100644 --- a/src/config.js +++ b/src/config.js @@ -64,7 +64,7 @@ const LIMITS = { minSeverity: 'low', temperature: 0.1, maxOutputTokens: 16000, - requestTimeoutMs: 180000, + requestTimeoutMs: 600000, }; export function readConfig() { diff --git a/src/llm.js b/src/llm.js index 1818a22..2a7fc0c 100644 --- a/src/llm.js +++ b/src/llm.js @@ -18,8 +18,14 @@ import * as core from './core.js'; export class LLM { - constructor(config) { + constructor(config, runtime = {}) { this.config = config; + this.runtime = { + fetch: runtime.fetch ?? ((input, init) => globalThis.fetch(input, init)), + now: runtime.now ?? (() => Date.now()), + sleep: runtime.sleep ?? core.sleep, + timeoutSignal: runtime.timeoutSignal ?? ((ms) => AbortSignal.timeout(ms)), + }; // Identifies this client in findings and in the summary footer. this.label = config.label || config.model; this.quirks = { @@ -70,15 +76,18 @@ export class LLM { */ async send(messages, options = {}) { const url = `${this.config.baseUrl}/chat/completions`; + const deadline = this.runtime.now() + this.config.requestTimeoutMs; let attempt = 0; let networkRetries = 0; for (;;) { + const remainingMs = deadline - this.runtime.now(); + if (remainingMs <= 0) throw requestDeadlineError(this.config.requestTimeoutMs); const builtAt = this.quirksVersion; const body = this.buildBody(messages, options); let res; try { - res = await fetch(url, { + res = await this.runtime.fetch(url, { method: 'POST', headers: { authorization: `Bearer ${this.config.apiKey}`, @@ -86,16 +95,21 @@ export class LLM { 'user-agent': 'commitreview', }, body: JSON.stringify(body), - signal: AbortSignal.timeout(this.config.requestTimeoutMs), + signal: this.runtime.timeoutSignal(remainingMs), }); } catch (err) { - if (networkRetries++ >= 3) throw new Error(`Model request failed: ${err.message}`, { cause: err }); - await core.sleep(core.backoff(networkRetries)); + if (isRequestAbort(err) || this.runtime.now() >= deadline) { + throw requestDeadlineError(this.config.requestTimeoutMs, err); + } + if (networkRetries++ >= 3) throw new Error(`Model request failed: ${errorMessage(err)}`, { cause: err }); + await this.waitForRetry(core.backoff(networkRetries), deadline, err); continue; } + if (this.runtime.now() >= deadline) throw requestDeadlineError(this.config.requestTimeoutMs); if (res.ok) { const data = /** @type {any} */ (await res.json()); + if (this.runtime.now() >= deadline) throw requestDeadlineError(this.config.requestTimeoutMs); this.usage.requests++; this.usage.prompt += data?.usage?.prompt_tokens || 0; this.usage.completion += data?.usage?.completion_tokens || 0; @@ -112,8 +126,9 @@ export class LLM { if (res.status === 429 || res.status >= 500) { if (networkRetries++ >= 3) throw new Error(`Model request failed: ${res.status} ${truncate(text)}`); const after = Number(res.headers.get('retry-after')); - await core.sleep( + await this.waitForRetry( Number.isFinite(after) && after > 0 ? Math.min(after, 60) * 1000 : core.backoff(networkRetries), + deadline, ); continue; } @@ -147,6 +162,13 @@ export class LLM { } } + async waitForRetry(delayMs, deadline, cause) { + const remainingMs = deadline - this.runtime.now(); + if (remainingMs <= delayMs) throw requestDeadlineError(this.config.requestTimeoutMs, cause); + await this.runtime.sleep(delayMs); + if (this.runtime.now() >= deadline) throw requestDeadlineError(this.config.requestTimeoutMs, cause); + } + /** Drop or rename whatever the endpoint just complained about. @returns {boolean} changed */ adapt(errorText) { const changed = this.#adapt(errorText || ''); @@ -226,6 +248,18 @@ export class LLM { } } +function isRequestAbort(error) { + return error instanceof Error && (error.name === 'AbortError' || error.name === 'TimeoutError'); +} + +function errorMessage(error) { + return error instanceof Error ? error.message : String(error); +} + +function requestDeadlineError(timeoutMs, cause) { + return new Error(`Model request exceeded its ${timeoutMs}ms logical deadline.`, { cause }); +} + /** * Pull a JSON value out of arbitrary model text. Returns null when there is none. * diff --git a/test/config.test.js b/test/config.test.js index edd67d7..33e7af3 100644 --- a/test/config.test.js +++ b/test/config.test.js @@ -88,6 +88,7 @@ test('configuration has six public inputs and no URL fallback', () => { assert.equal(config.baseUrl, 'https://models.example/v1'); assert.equal(config.githubApiUrl, 'https://github.example/api/v3'); assert.equal(config.instructions, 'Use integer pence.'); + assert.equal(config.requestTimeoutMs, 600000); assert.ok(config.ignore.includes('private/**')); const withoutBase = { ...values }; diff --git a/test/llm.test.js b/test/llm.test.js index e37320b..11171fe 100644 --- a/test/llm.test.js +++ b/test/llm.test.js @@ -79,6 +79,103 @@ test('request body carries the tunables the endpoint is expected to support', () assert.deepEqual(body.response_format, { type: 'json_object' }); }); +test('a timed-out logical request is not duplicated', async () => { + let now = 0; + let requests = 0; + const timeout = new Error('request timed out'); + timeout.name = 'TimeoutError'; + const llm = new LLM( + { ...base, requestTimeoutMs: 100 }, + { + fetch: async () => { + requests++; + now = 100; + throw timeout; + }, + now: () => now, + sleep: async () => assert.fail('a timed-out request must not sleep or retry'), + timeoutSignal: () => new AbortController().signal, + }, + ); + + await assert.rejects(() => llm.send([{ role: 'user', content: 'hi' }]), /100ms logical deadline/); + assert.equal(requests, 1); +}); + +test('a transient network failure retries within the shared deadline', async () => { + let now = 0; + let requests = 0; + const timeoutBudgets = []; + const llm = new LLM( + { ...base, requestTimeoutMs: 10_000 }, + { + fetch: async () => { + requests++; + if (requests === 1) throw new Error('connection reset'); + return Response.json({ choices: [{ message: { content: 'ok' } }] }); + }, + now: () => now, + sleep: async (ms) => { + now += ms; + }, + timeoutSignal: (remainingMs) => { + timeoutBudgets.push(remainingMs); + return new AbortController().signal; + }, + }, + ); + + assert.deepEqual(await llm.send([{ role: 'user', content: 'hi' }]), { + message: { content: 'ok' }, + }); + assert.equal(requests, 2); + assert.ok(now > 0 && now < 10_000); + assert.equal(timeoutBudgets.length, 2); + assert.equal(timeoutBudgets[0], 10_000); + assert.ok(timeoutBudgets[1] < timeoutBudgets[0]); +}); + +test('response decoding must finish inside the shared deadline', async () => { + let now = 0; + const llm = new LLM( + { ...base, requestTimeoutMs: 100 }, + { + fetch: async () => ({ + ok: true, + json: async () => { + now = 100; + return { choices: [{ message: { content: 'too late' } }] }; + }, + }), + now: () => now, + timeoutSignal: () => new AbortController().signal, + }, + ); + + await assert.rejects(() => llm.send([{ role: 'user', content: 'hi' }]), /100ms logical deadline/); +}); + +test('a retry delay that exceeds the shared deadline fails closed', async () => { + let now = 0; + let requests = 0; + const llm = new LLM( + { ...base, requestTimeoutMs: 100 }, + { + fetch: async () => { + requests++; + now = 99; + return new Response('busy', { status: 503 }); + }, + now: () => now, + sleep: async () => assert.fail('the retry cannot fit inside the deadline'), + timeoutSignal: () => new AbortController().signal, + }, + ); + + await assert.rejects(() => llm.send([{ role: 'user', content: 'hi' }]), /100ms logical deadline/); + assert.equal(requests, 1); +}); + test('sends a json schema as response_format when a schema is passed', () => { const llm = new LLM(base); const schema = { type: 'object', properties: { a: { type: 'string' } }, required: ['a'] };