diff --git a/src/core/pinecone-client.test.ts b/src/core/pinecone-client.test.ts index 29c8440..2147b2f 100644 --- a/src/core/pinecone-client.test.ts +++ b/src/core/pinecone-client.test.ts @@ -4,6 +4,7 @@ import { resolveConfig } from './config.js'; import type { SearchableIndex, PineconeHit } from '../types.js'; import * as rerankModule from './pinecone/rerank.js'; import { DENSE_LEG_FAILED_REASON, SPARSE_LEG_FAILED_REASON } from '../constants.js'; +import { AppTimeoutError } from './server/retry.js'; /** Stubs for private methods (assigned at runtime; avoid intersecting private `PineconeClient` members). */ type PineconeClientMethodStubs = { @@ -261,10 +262,7 @@ describe('PineconeClient', () => { it('propagates app timeout when both dense and sparse searches fail', async () => { const testClient = stubPineconeClient(client); - stubDualLegSearchFailure( - testClient, - new Error('Timeout after 50ms while waiting for search') - ); + stubDualLegSearchFailure(testClient, new AppTimeoutError(50, 'search')); await expect( client.query({ @@ -273,7 +271,7 @@ describe('PineconeClient', () => { topK: 5, useReranking: false, }) - ).rejects.toThrow('Timeout after 50ms while waiting for search'); + ).rejects.toBeInstanceOf(AppTimeoutError); }); }); @@ -573,7 +571,7 @@ describe('PineconeClient', () => { }); const p = timeoutClient.keywordSearch({ query: 'q', namespace: 'n' }); - const assertion = expect(p).rejects.toThrow(/Timeout after 50ms while waiting for search/); + const assertion = expect(p).rejects.toBeInstanceOf(AppTimeoutError); await vi.advanceTimersByTimeAsync(50); await assertion; }); diff --git a/src/core/pinecone/indexes.test.ts b/src/core/pinecone/indexes.test.ts index 7b28919..e41ef3f 100644 --- a/src/core/pinecone/indexes.test.ts +++ b/src/core/pinecone/indexes.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect, vi, afterEach } from 'vitest'; import { PineconeIndexSession } from './indexes.js'; import type { SearchableIndex } from '../../types.js'; +import { makeStructured429Once } from './test-helpers.js'; /** Subclass so tests inject index handles without calling the real Pinecone SDK. */ class PineconeIndexSessionTestDouble extends PineconeIndexSession { @@ -144,6 +145,24 @@ describe('PineconeIndexSession', () => { expect(sparse.describeIndexStats).toHaveBeenCalledTimes(2); }); + it('retries describeIndexStats on structured 429 then succeeds', async () => { + const stats = { namespaces: { papers: { recordCount: 7 } } }; + const describeIndexStats = makeStructured429Once(stats); + const sparse = { describeIndexStats } as unknown as SearchableIndex; + const session = new PineconeIndexSessionTestDouble({ + dense: {} as SearchableIndex, + sparse, + }); + + const result = await session.listNamespacesFromKeywordIndex(); + + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.namespaces).toEqual([{ namespace: 'papers', recordCount: 7 }]); + } + expect(describeIndexStats).toHaveBeenCalledTimes(2); + }); + it('invokes describeIndexStats on the sparse index receiver through runIo', async () => { const sparse = makeThisSensitiveIndex({ namespaces: { papers: { recordCount: 42 } }, diff --git a/src/core/pinecone/rerank.test.ts b/src/core/pinecone/rerank.test.ts index c5f94b0..84f3cbe 100644 --- a/src/core/pinecone/rerank.test.ts +++ b/src/core/pinecone/rerank.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect, vi } from 'vitest'; import { rerankResults } from './rerank.js'; import type { MergedHit } from '../../types.js'; +import { makeStructured429Once } from './test-helpers.js'; const sampleMerged: MergedHit[] = [ { _id: '1', _score: 0.5, chunk_text: 'hello', metadata: { k: 'v' } }, @@ -75,6 +76,25 @@ describe('rerankResults', () => { expect(rerank).toHaveBeenCalledTimes(2); }); + it('retries on structured 429 without 429 in message then succeeds', async () => { + const success = { + data: [ + { + score: 0.99, + document: { _id: '1', chunk_text: 'hello', metadata: { k: 'v' } }, + }, + ], + }; + const rerank = makeStructured429Once(success); + const pc = makePc(rerank); + + const out = await rerankResults(pc, 'm', 'q', sampleMerged, 5); + + expect(out.degraded).toBe(false); + expect(out.results[0]?.reranked).toBe(true); + expect(rerank).toHaveBeenCalledTimes(2); + }); + it('degrades after exhausting retries on persistent 503', async () => { const rerank = vi.fn().mockRejectedValue(new Error('HTTP 503')); const pc = makePc(rerank); diff --git a/src/core/pinecone/search.test.ts b/src/core/pinecone/search.test.ts index 61f5e05..63775dc 100644 --- a/src/core/pinecone/search.test.ts +++ b/src/core/pinecone/search.test.ts @@ -1,4 +1,6 @@ import { describe, it, expect, vi, afterEach } from 'vitest'; +import { AppTimeoutError } from '../server/retry.js'; +import { makeStructured429Once } from './test-helpers.js'; import { searchIndex, mergeResults, @@ -77,6 +79,15 @@ describe('searchIndex', () => { expect(search).toHaveBeenCalledTimes(2); }); + it('retries on structured 429 without 429 in message then succeeds', async () => { + const success = { result: { hits: [{ _id: '1', _score: 1, fields: {} }] } }; + const search = makeStructured429Once(success); + const index = { search } as unknown as SearchableIndex; + const hits = await searchIndex(index, 'hi', 5); + expect(hits).toHaveLength(1); + expect(search).toHaveBeenCalledTimes(2); + }); + it('does not retry on 401', async () => { const search = vi.fn().mockRejectedValue(new Error('HTTP 401')); const index = { search } as unknown as SearchableIndex; @@ -84,12 +95,12 @@ describe('searchIndex', () => { expect(search).toHaveBeenCalledTimes(1); }); - it('times out at requestTimeoutMs and preserves the timeout error prefix', async () => { + it('times out at requestTimeoutMs and rejects with AppTimeoutError', async () => { vi.useFakeTimers(); const search = vi.fn(() => new Promise(() => {})); const index = { search } as unknown as SearchableIndex; const p = searchIndex(index, 'hi', 5, undefined, undefined, undefined, 50); - const assertion = expect(p).rejects.toThrow(/^Timeout after 50ms while waiting for search/); + const assertion = expect(p).rejects.toBeInstanceOf(AppTimeoutError); await vi.advanceTimersByTimeAsync(50); await assertion; }); diff --git a/src/core/pinecone/test-helpers.ts b/src/core/pinecone/test-helpers.ts new file mode 100644 index 0000000..c6530f1 --- /dev/null +++ b/src/core/pinecone/test-helpers.ts @@ -0,0 +1,13 @@ +import { vi } from 'vitest'; + +/** First call throws structured HTTP 429; subsequent calls resolve with `success`. */ +export function makeStructured429Once(success: T): ReturnType { + let n = 0; + return vi.fn().mockImplementation(async () => { + n++; + if (n < 2) { + throw Object.assign(new Error('Rate limited'), { status: 429 }); + } + return success; + }); +} diff --git a/src/core/server/retry.test.ts b/src/core/server/retry.test.ts index 562b634..c94ebc3 100644 --- a/src/core/server/retry.test.ts +++ b/src/core/server/retry.test.ts @@ -6,6 +6,8 @@ import { transientShouldRetry, isAppTimeoutError, runWithPolicy, + AppTimeoutError, + getHttpStatus, } from './retry.js'; afterEach(() => { @@ -19,6 +21,61 @@ describe('defaultShouldRetry', () => { it('does not retry on 400', () => { expect(defaultShouldRetry(new Error('HTTP 400'))).toBe(false); }); + + it('retries on structured status 429 without 429 in message', () => { + const err = Object.assign(new Error('Too many requests'), { status: 429 }); + expect(defaultShouldRetry(err)).toBe(true); + }); + + it('retries on structured status 408 without status in message', () => { + const err = Object.assign(new Error('Request timeout'), { status: 408 }); + expect(defaultShouldRetry(err)).toBe(true); + }); + + it('retries on PineconeUnmappedHttpError with Status: 429 in message', () => { + const err = Object.assign(new Error('Status: 429. Body: throttled'), { + name: 'PineconeUnmappedHttpError', + }); + expect(defaultShouldRetry(err)).toBe(true); + expect(getHttpStatus(err)).toBe(429); + }); + + it('retries on statusCode 503 without status code in message', () => { + const err = Object.assign(new Error('Service unavailable'), { statusCode: 503 }); + expect(defaultShouldRetry(err)).toBe(true); + }); + + it('retries on PineconeUnavailableError with reworded message', () => { + const err = Object.assign(new Error('Service is down for maintenance'), { + name: 'PineconeUnavailableError', + }); + expect(defaultShouldRetry(err)).toBe(true); + expect(getHttpStatus(err)).toBe(503); + }); + + it('retries on ECONNRESET message without structured status', () => { + expect(defaultShouldRetry(new Error('ECONNRESET'))).toBe(true); + }); + + it('retries when retryable network signal is only on cause chain', () => { + const wrapped = new Error('Request failed', { cause: new Error('ECONNRESET') }); + expect(defaultShouldRetry(wrapped)).toBe(true); + }); + + it('does not retry wrapper with generic cause and no retry signals', () => { + const wrapped = new Error('Request failed', { cause: new Error('upstream failed') }); + expect(defaultShouldRetry(wrapped)).toBe(false); + }); + + it('does not retry AppTimeoutError even via defaultShouldRetry', () => { + expect(defaultShouldRetry(new AppTimeoutError(50, 'search'))).toBe(false); + }); + + it('does not retry when structured status is non-retryable despite retryable message', () => { + const err = Object.assign(new Error('HTTP 503 upstream glitch'), { status: 401 }); + expect(getHttpStatus(err)).toBe(401); + expect(defaultShouldRetry(err)).toBe(false); + }); }); describe('transientShouldRetry', () => { @@ -35,12 +92,37 @@ describe('transientShouldRetry', () => { false ); }); + + it('does not retry AppTimeoutError', () => { + expect(transientShouldRetry(new AppTimeoutError(50, 'search'))).toBe(false); + }); }); describe('isAppTimeoutError', () => { it('matches withTimeout rejection messages', () => { expect(isAppTimeoutError(new Error('Timeout after 50ms while waiting for search'))).toBe(true); }); + + it('matches AppTimeoutError instances', () => { + expect(isAppTimeoutError(new AppTimeoutError(50, 'search'))).toBe(true); + }); + + it('matches AppTimeoutError in cause chain', () => { + const wrapped = new Error('wrapped', { cause: new AppTimeoutError(50, 'search') }); + expect(isAppTimeoutError(wrapped)).toBe(true); + }); + + it('matches legacy withTimeout message prefix in cause chain', () => { + const wrapped = new Error('wrapper', { + cause: new Error('Timeout after 50ms while waiting for search'), + }); + expect(isAppTimeoutError(wrapped)).toBe(true); + }); + + it('does not match generic wrapper without timeout in cause chain', () => { + const wrapped = new Error('wrapper', { cause: new Error('upstream failed') }); + expect(isAppTimeoutError(wrapped)).toBe(false); + }); }); describe('runWithPolicy', () => { @@ -82,7 +164,7 @@ describe('runWithPolicy', () => { }, { timeoutMs: 50, label: 'test', retries: 2, backoffMs: 1 } ); - const assertion = expect(p).rejects.toThrow(/Timeout after 50ms/); + const assertion = expect(p).rejects.toBeInstanceOf(AppTimeoutError); await vi.advanceTimersByTimeAsync(50); await assertion; expect(n).toBe(1); @@ -99,7 +181,7 @@ describe('withTimeout', () => { }), { timeoutMs: 100, label: 'test' } ); - const assertion = expect(p).rejects.toThrow(/Timeout after 100ms/); + const assertion = expect(p).rejects.toBeInstanceOf(AppTimeoutError); await vi.advanceTimersByTimeAsync(100); await assertion; }); diff --git a/src/core/server/retry.ts b/src/core/server/retry.ts index b8a9c90..15b6ee6 100644 --- a/src/core/server/retry.ts +++ b/src/core/server/retry.ts @@ -9,12 +9,139 @@ import { warn, redactErrorMessage } from '../../logger.js'; -/** Matches {@link withTimeout} rejection message prefix; used by tool-error and callers. */ +const ERROR_CHAIN_MAX_DEPTH = 5; + +const PINECONE_NAME_TO_HTTP_STATUS: Readonly> = { + PineconeInternalServerError: 500, + PineconeUnavailableError: 503, +}; + +const TRANSIENT_PINECONE_ERROR_NAMES = new Set(['PineconeConnectionError']); + +const PINECONE_STATUS_MESSAGE_PATTERN = /(?:Status:|with status)\s*(\d{3})/i; + +/** Matches {@link withTimeout} rejection message prefix; legacy fallback for plain Error mocks. */ export const APP_TIMEOUT_PATTERN = /^Timeout after \d+ms while waiting for /i; +/** Branded error thrown by {@link withTimeout} when the per-attempt deadline fires. */ +export class AppTimeoutError extends Error { + readonly appTimeout = true as const; + readonly timeoutMs: number; + readonly label: string; + + constructor(timeoutMs: number, label: string) { + super(`Timeout after ${timeoutMs}ms while waiting for ${label}`); + this.name = 'AppTimeoutError'; + this.timeoutMs = timeoutMs; + this.label = label; + } +} + +function getErrorCause(error: Error): unknown { + return (error as Error & { cause?: unknown }).cause; +} + +/** + * Walk `error` and its `cause` chain (depth-capped, cycle-safe). + * Stops when `visit` returns a non-undefined value or the chain ends. + */ +function forEachErrorInChain( + error: unknown, + visit: (current: Error) => T | undefined +): T | undefined { + const seen = new Set(); + let current: unknown = error; + let depth = 0; + + while (current instanceof Error && depth < ERROR_CHAIN_MAX_DEPTH) { + if (seen.has(current)) return undefined; + seen.add(current); + + const result = visit(current); + if (result !== undefined) return result; + + current = getErrorCause(current); + depth++; + } + + return undefined; +} + +function readDirectHttpStatus(error: Error): number | undefined { + const candidate = + (error as Error & { status?: number; statusCode?: number }).status ?? + (error as Error & { statusCode?: number }).statusCode; + if (typeof candidate === 'number' && candidate >= 100 && candidate <= 599) { + return candidate; + } + return undefined; +} + +function readPineconeNamedHttpStatus(error: Error): number | undefined { + const mapped = PINECONE_NAME_TO_HTTP_STATUS[error.name]; + if (mapped !== undefined) return mapped; + + if (error.name === 'PineconeUnmappedHttpError' || error.name === 'PineconeRequestError') { + const match = PINECONE_STATUS_MESSAGE_PATTERN.exec(error.message); + if (match) { + const status = Number(match[1]); + if (status >= 100 && status <= 599) return status; + } + } + + return undefined; +} + +/** + * Extract the first HTTP status from `error` and its `cause` chain. + * Precedence per link: direct `status`/`statusCode`, then Pinecone `error.name` mappings. + */ +export function getHttpStatus(error: unknown): number | undefined { + return forEachErrorInChain(error, (current) => { + return readDirectHttpStatus(current) ?? readPineconeNamedHttpStatus(current); + }); +} + +/** True when `status` is a transient HTTP code (408, 429, or 5xx). */ +export function isRetryableHttpStatus(status: number): boolean { + return status === 408 || status === 429 || (status >= 500 && status < 600); +} + +function hasTransientPineconeErrorName(error: unknown): boolean { + return ( + forEachErrorInChain(error, (current) => + TRANSIENT_PINECONE_ERROR_NAMES.has(current.name) ? true : undefined + ) === true + ); +} + +/** + * True for app-level {@link withTimeout} deadlines ({@link AppTimeoutError} or legacy message prefix). + * Branded timeouts and {@link APP_TIMEOUT_PATTERN} are matched on the error `cause` chain. + */ export function isAppTimeoutError(error: unknown): boolean { - const msg = error instanceof Error ? error.message : String(error); - return APP_TIMEOUT_PATTERN.test(msg); + if ( + forEachErrorInChain(error, (current) => + current instanceof AppTimeoutError || APP_TIMEOUT_PATTERN.test(current.message) + ? true + : undefined + ) === true + ) { + return true; + } + + if (!(error instanceof Error)) { + return APP_TIMEOUT_PATTERN.test(String(error)); + } + return false; +} + +function errorMessageLooksRetryable(message: string): boolean { + if (/ETIMEDOUT|ECONNRESET|ECONNREFUSED|ENOTFOUND/i.test(message)) return true; + if (/\btimeout\b/i.test(message)) return true; + // 500/501 omitted: structured/Pinecone paths cover 500; 501 is not transient. Plain "HTTP 500" in message alone is not matched here. + if (/\b(429|502|503|504)\b/.test(message)) return true; + return false; } /** Retry policy. */ @@ -45,25 +172,31 @@ export interface PolicyOptions { backoffMs?: number; } -/** Default predicate: retry on common transient HTTP statuses (429/5xx) and network-ish messages. */ +/** + * Default retry predicate. Precedence: reject app timeouts; if {@link getHttpStatus} + * returns a defined status, use {@link isRetryableHttpStatus} only (no message fallback); + * otherwise transient Pinecone error names, then network-ish message regex. + */ export function defaultShouldRetry(error: unknown): boolean { - if (error instanceof Error) { - const msg = error.message; - if (/\b(429|502|503|504)\b/.test(msg)) return true; - if (/timeout|ETIMEDOUT|ECONNRESET|ECONNREFUSED|ENOTFOUND/i.test(msg)) return true; - } - const status = - (error as { status?: number; statusCode?: number })?.status ?? - (error as { statusCode?: number })?.statusCode; - if (typeof status === 'number' && (status === 429 || (status >= 500 && status < 600))) { - return true; - } - return false; + if (isAppTimeoutError(error)) return false; + + const status = getHttpStatus(error); + if (status !== undefined) return isRetryableHttpStatus(status); + + if (hasTransientPineconeErrorName(error)) return true; + + return ( + forEachErrorInChain(error, (current) => + errorMessageLooksRetryable(current.message) ? true : undefined + ) === true + ); } -/** Retry 429/5xx + network errors; do NOT retry app-level {@link withTimeout} deadlines. */ +/** + * Retry predicate for {@link runWithPolicy} Pinecone transient I/O. + * Delegates to {@link defaultShouldRetry} (including app-timeout exclusion). + */ export function transientShouldRetry(error: unknown): boolean { - if (isAppTimeoutError(error)) return false; return defaultShouldRetry(error); } @@ -113,7 +246,7 @@ export async function withTimeout( timer = setTimeout(() => { // Reject before abort so `Promise.race` observes the timeout error first; // abort still notifies cooperative callers. - reject(new Error(`Timeout after ${timeoutMs}ms while waiting for ${label}`)); + reject(new AppTimeoutError(timeoutMs, label)); controller.abort(); }, timeoutMs); }); diff --git a/src/core/server/tool-error.test.ts b/src/core/server/tool-error.test.ts index 13c9a7a..760d5a8 100644 --- a/src/core/server/tool-error.test.ts +++ b/src/core/server/tool-error.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it } from 'vitest'; +import { AppTimeoutError } from './retry.js'; import { classifyToolCatchError, DEFAULT_TIMEOUT_SUGGESTION, @@ -48,7 +49,15 @@ describe('ToolError schema and builders', () => { expect(parsed.message).toContain('disposed'); }); - it('TIMEOUT: classifyToolCatchError matches withTimeout message prefix', () => { + it('TIMEOUT: classifyToolCatchError matches AppTimeoutError', () => { + const err = classifyToolCatchError(new AppTimeoutError(100, 'query'), 'fallback'); + expect(err.code).toBe('TIMEOUT'); + expect(err.recoverable).toBe(true); + expect(err.suggestion).toMatch(/retry|timeout/i); + toolErrorSchema.parse(err); + }); + + it('TIMEOUT: classifyToolCatchError matches legacy withTimeout message prefix', () => { const err = classifyToolCatchError( new Error('Timeout after 100ms while waiting for query'), 'fallback' diff --git a/src/core/server/tools/query-tool.context.test.ts b/src/core/server/tools/query-tool.context.test.ts index 9659372..b62a73f 100644 --- a/src/core/server/tools/query-tool.context.test.ts +++ b/src/core/server/tools/query-tool.context.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it, vi } from 'vitest'; +import { AppTimeoutError } from '../retry.js'; import { registerQueryTool } from './query-tool.js'; import { querySuccessResponseSchema } from '../response-schemas.js'; import { @@ -136,7 +137,7 @@ describe('query tool handler (ServerContext instance path)', () => { ); }); - it('returns TIMEOUT when client throws timeout error', async () => { + it('returns TIMEOUT when client throws legacy timeout message', async () => { const query = vi .fn() .mockRejectedValue(new Error('Timeout after 5000ms while waiting for query')); @@ -160,4 +161,27 @@ describe('query tool handler (ServerContext instance path)', () => { ); expect(err.suggestion).toMatch(/retry|timeout/i); }); + + it('returns TIMEOUT when client throws AppTimeoutError', async () => { + const query = vi.fn().mockRejectedValue(new AppTimeoutError(5000, 'query')); + const ctx = createTestServerContext({ + client: { query } as never, + }); + ctx.markSuggested('wg21', { + recommended_tool: 'fast', + suggested_fields: ['title'], + user_query: 'q', + }); + const server = createMockServer(); + registerQueryTool(server as never, ctx); + const err = assertToolErrorCode( + await server.getHandler('query')!({ + query_text: 'hello', + namespace: 'wg21', + preset: 'fast', + }), + 'TIMEOUT' + ); + expect(err.suggestion).toMatch(/retry|timeout/i); + }); });