From bd98e568f6a0778d54eba98ba5a7655af72d5651 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 27 May 2026 21:32:06 -0400 Subject: [PATCH] fix(#308): bound websearch fetch with timeout and retry (#387) cmdWebsearch called fetch() with no timeout and no retry, so a hung connection blocked indefinitely and transient 429/5xx/network failures were not recovered. Add AbortSignal.timeout (configurable via GSD_WEBSEARCH_TIMEOUT_MS, default 10s) and a bounded retry loop (max 2 retries, exponential backoff + jitter) for 429/5xx/network errors, honoring Retry-After on 429 (capped at 60s). Non-429 4xx fail immediately (no wasted retries). Transient-exhausted failures report an `attempts` count. Worst-case time is bounded by timeout*(1+retries)+backoff. Co-authored-by: Claude Opus 4.7 (1M context) --- .changeset/308-websearch-timeout-retry.md | 5 + get-shit-done/bin/lib/commands.cjs | 128 +++++++++++++---- tests/commands.test.cjs | 159 +++++++++++++++++++++- 3 files changed, 260 insertions(+), 32 deletions(-) create mode 100644 .changeset/308-websearch-timeout-retry.md diff --git a/.changeset/308-websearch-timeout-retry.md b/.changeset/308-websearch-timeout-retry.md new file mode 100644 index 000000000..8f17e9fa0 --- /dev/null +++ b/.changeset/308-websearch-timeout-retry.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 308 +--- +Add a per-attempt timeout (configurable via GSD_WEBSEARCH_TIMEOUT_MS) and bounded retry with Retry-After handling to the Brave websearch fetch (#308). diff --git a/get-shit-done/bin/lib/commands.cjs b/get-shit-done/bin/lib/commands.cjs index 855276de1..a706636b1 100644 --- a/get-shit-done/bin/lib/commands.cjs +++ b/get-shit-done/bin/lib/commands.cjs @@ -517,6 +517,30 @@ function cmdSummaryExtract(cwd, summaryPath, fields, raw) { output(fullResult, raw); } +function _wsSleep(ms) { + return new Promise(resolve => setTimeout(resolve, ms)); +} + +function _wsParseRetryAfter(header) { + if (!header) return null; + const trimmed = header.trim(); + if (/^\d+$/.test(trimmed)) { + return Math.min(Math.max(parseInt(trimmed, 10) * 1000, 0), 60000); + } + const asDate = Date.parse(trimmed); + if (!isNaN(asDate)) { + return Math.min(Math.max(asDate - Date.now(), 0), 60000); + } + return null; +} + +function _wsRetryDelayMs(attempt) { + const base = 250; + const cap = 2000; + const exp = Math.min(base * Math.pow(2, attempt), cap); + return exp + Math.floor(Math.random() * 100); +} + async function cmdWebsearch(query, options, raw) { const apiKey = process.env.BRAVE_API_KEY; @@ -543,39 +567,82 @@ async function cmdWebsearch(query, options, raw) { params.set('freshness', options.freshness); } - try { - const response = await fetch( - `https://api.search.brave.com/res/v1/web/search?${params}`, - { - headers: { - 'Accept': 'application/json', - 'X-Subscription-Token': apiKey - } + const rawTimeout = parseInt(process.env.GSD_WEBSEARCH_TIMEOUT_MS, 10); + const timeoutMs = (Number.isInteger(rawTimeout) && rawTimeout > 0) ? rawTimeout : 10000; + + const MAX_RETRIES = 2; + let attempt = 0; + + while (true) { + try { + const ac = new AbortController(); + const timer = setTimeout(() => ac.abort(new Error('timeout')), timeoutMs); + let response; + try { + response = await fetch( + `https://api.search.brave.com/res/v1/web/search?${params}`, + { + headers: { + 'Accept': 'application/json', + 'X-Subscription-Token': apiKey + }, + signal: ac.signal + } + ); + } finally { + clearTimeout(timer); } - ); - if (!response.ok) { - output({ available: false, error: `API error: ${response.status}` }, raw, ''); - return; + if (response.ok) { + const data = await response.json(); + const results = (data.web?.results || []).map(r => ({ + title: r.title, + url: r.url, + description: r.description, + age: r.age || null + })); + output({ + available: true, + query, + count: results.length, + results + }, raw, results.map(r => `${r.title}\n${r.url}\n${r.description}`).join('\n\n')); + return; + } + + const status = response.status; + const isRetryable = status === 429 || status >= 500; + + if (!isRetryable) { + // Non-retryable 4xx — fail immediately, no attempts field + output({ available: false, error: `API error: ${status}` }, raw, ''); + return; + } + + // Retryable HTTP error + attempt++; + if (attempt > MAX_RETRIES) { + output({ available: false, error: `API error: ${status}`, attempts: attempt }, raw, ''); + return; + } + + let delay; + if (status === 429) { + const retryAfter = _wsParseRetryAfter(response.headers.get('retry-after')); + delay = retryAfter !== null ? retryAfter : _wsRetryDelayMs(attempt - 1); + } else { + delay = _wsRetryDelayMs(attempt - 1); + } + await _wsSleep(delay); + + } catch (err) { + attempt++; + if (attempt > MAX_RETRIES) { + output({ available: false, error: err.message, attempts: attempt }, raw, ''); + return; + } + await _wsSleep(_wsRetryDelayMs(attempt - 1)); } - - const data = await response.json(); - - const results = (data.web?.results || []).map(r => ({ - title: r.title, - url: r.url, - description: r.description, - age: r.age || null - })); - - output({ - available: true, - query, - count: results.length, - results - }, raw, results.map(r => `${r.title}\n${r.url}\n${r.description}`).join('\n\n')); - } catch (err) { - output({ available: false, error: err.message }, raw, ''); } } @@ -1060,4 +1127,5 @@ module.exports = { cmdScaffold, cmdStats, cmdCheckCommit, + _wsParseRetryAfter, }; diff --git a/tests/commands.test.cjs b/tests/commands.test.cjs index cd93be5d4..fefe80b31 100644 --- a/tests/commands.test.cjs +++ b/tests/commands.test.cjs @@ -1558,14 +1558,15 @@ describe('websearch command', () => { global.fetch = async () => ({ ok: false, - status: 429, + status: 401, + headers: { get: () => null }, }); await cmdWebsearch('test query', {}, false); const output = JSON.parse(captured); assert.strictEqual(output.available, false); - assert.ok(output.error.includes('429'), 'error should include status code'); + assert.ok(output.error.includes('401'), 'error should include status code'); }); test('handles network failure', async () => { @@ -1581,6 +1582,113 @@ describe('websearch command', () => { assert.strictEqual(output.available, false); assert.strictEqual(output.error, 'Network timeout'); }); + + // ── New retry/timeout tests (A–E) ────────────────────────────────────────── + + test('A. timeout is bounded: AbortSignal fires, resolves with available=false and attempts field', async (t) => { + process.env.BRAVE_API_KEY = 'test-key'; + process.env.GSD_WEBSEARCH_TIMEOUT_MS = '20'; + t.after(() => { delete process.env.GSD_WEBSEARCH_TIMEOUT_MS; }); + + global.fetch = async (_url, init) => new Promise((_resolve, reject) => { + init.signal.addEventListener('abort', () => reject(new DOMException('aborted', 'AbortError'))); + }); + + await cmdWebsearch('q', {}, false); + + const output = JSON.parse(captured); + assert.strictEqual(output.available, false, 'should be available=false after timeout exhaustion'); + assert.ok(typeof output.attempts === 'number', 'should include attempts field'); + }); + + test('B. retry on 503 then success: succeeds on 2nd attempt, fetch called exactly twice', async () => { + process.env.BRAVE_API_KEY = 'test-key'; + let callCount = 0; + + global.fetch = async () => { + callCount++; + if (callCount === 1) { + return { ok: false, status: 503, headers: { get: () => null } }; + } + return { + ok: true, + json: async () => ({ + web: { results: [{ title: 'T', url: 'https://example.com', description: 'D' }] }, + }), + }; + }; + + await cmdWebsearch('test query', {}, false); + + const output = JSON.parse(captured); + assert.strictEqual(output.available, true, 'should succeed after retry'); + assert.strictEqual(output.results.length, 1, 'should have one result'); + assert.strictEqual(callCount, 2, 'fetch should be called exactly twice'); + }); + + test('C. 429 honors Retry-After then succeeds on 2nd call, fetch called exactly twice', async () => { + process.env.BRAVE_API_KEY = 'test-key'; + let callCount = 0; + + global.fetch = async () => { + callCount++; + if (callCount === 1) { + return { + ok: false, + status: 429, + headers: { get: (h) => h.toLowerCase() === 'retry-after' ? '0' : null }, + }; + } + return { + ok: true, + json: async () => ({ + web: { results: [{ title: 'T', url: 'https://example.com', description: 'D' }] }, + }), + }; + }; + + await cmdWebsearch('test query', {}, false); + + const output = JSON.parse(captured); + assert.strictEqual(output.available, true, 'should succeed after 429 retry'); + assert.strictEqual(callCount, 2, 'fetch should be called exactly twice'); + }); + + test('D. no retry on 401: fails immediately, fetch called exactly once', async () => { + process.env.BRAVE_API_KEY = 'test-key'; + let callCount = 0; + + global.fetch = async () => { + callCount++; + return { ok: false, status: 401, headers: { get: () => null } }; + }; + + await cmdWebsearch('test query', {}, false); + + const output = JSON.parse(captured); + assert.strictEqual(output.available, false, 'should be available=false'); + assert.strictEqual(output.error, 'API error: 401', 'error should be API error: 401'); + assert.strictEqual(output.attempts, undefined, 'should NOT have attempts field on immediate fail'); + assert.strictEqual(callCount, 1, 'fetch should be called exactly once'); + }); + + test('E. network error retried then exhausted: attempts=3, fetch called 3 times', async () => { + process.env.BRAVE_API_KEY = 'test-key'; + let callCount = 0; + + global.fetch = async () => { + callCount++; + throw new Error('boom'); + }; + + await cmdWebsearch('test query', {}, false); + + const output = JSON.parse(captured); + assert.strictEqual(output.available, false, 'should be available=false'); + assert.ok(output.error.includes('boom'), 'error should include boom'); + assert.strictEqual(output.attempts, 3, 'attempts should be 3'); + assert.strictEqual(callCount, 3, 'fetch should be called 3 times'); + }); }); describe('stats command', () => { @@ -1952,3 +2060,50 @@ describe('check-commit command', () => { assert.ok(result.error.includes('unstage'), 'error should suggest unstage command'); }); }); + +describe('_wsParseRetryAfter (#308)', () => { + const { _wsParseRetryAfter } = require('../get-shit-done/bin/lib/commands.cjs'); + + test('integer seconds: "120" → 60000 (capped at 60s)', () => { + assert.strictEqual(_wsParseRetryAfter('120'), 60000); + }); + + test('leading zero: "01" → 1000', () => { + assert.strictEqual(_wsParseRetryAfter('01'), 1000); + }); + + test('whitespace: " 5 " → 5000', () => { + assert.strictEqual(_wsParseRetryAfter(' 5 '), 5000); + }); + + test('"0" → 0', () => { + assert.strictEqual(_wsParseRetryAfter('0'), 0); + }); + + test('value > 60s cap: "120000" → 60000', () => { + assert.strictEqual(_wsParseRetryAfter('120000'), 60000); + }); + + test('future HTTP-date → value in (0, 60000]', () => { + const futureDate = new Date(Date.now() + 5000).toUTCString(); + const v = _wsParseRetryAfter(futureDate); + assert.ok(typeof v === 'number' && v > 0 && v <= 60000, `expected (0,60000], got ${v}`); + }); + + test('past HTTP-date → 0', () => { + const pastDate = new Date(Date.now() - 5000).toUTCString(); + assert.strictEqual(_wsParseRetryAfter(pastDate), 0); + }); + + test('"garbage" → null', () => { + assert.strictEqual(_wsParseRetryAfter('garbage'), null); + }); + + test('"" → null', () => { + assert.strictEqual(_wsParseRetryAfter(''), null); + }); + + test('null → null', () => { + assert.strictEqual(_wsParseRetryAfter(null), null); + }); +});