From cca072fe670eb2062a6dc4a984cf48cf18ef7024 Mon Sep 17 00:00:00 2001 From: zhao Date: Fri, 24 Jul 2026 00:07:41 +0800 Subject: [PATCH 1/4] Harden retry/timeout classification on structured HTTP status and branded app timeouts --- src/core/pinecone/rerank.test.ts | 25 +++ src/core/pinecone/search.test.ts | 20 ++- src/core/server/retry.test.ts | 53 ++++++- src/core/server/retry.ts | 143 ++++++++++++++++-- src/core/server/tool-error.test.ts | 11 +- .../server/tools/query-tool.context.test.ts | 26 +++- 6 files changed, 260 insertions(+), 18 deletions(-) diff --git a/src/core/pinecone/rerank.test.ts b/src/core/pinecone/rerank.test.ts index c5f94b0..79ba2a1 100644 --- a/src/core/pinecone/rerank.test.ts +++ b/src/core/pinecone/rerank.test.ts @@ -75,6 +75,31 @@ describe('rerankResults', () => { expect(rerank).toHaveBeenCalledTimes(2); }); + it('retries on structured 429 without 429 in message then succeeds', async () => { + let n = 0; + const rerank = vi.fn().mockImplementation(async () => { + n++; + if (n < 2) { + throw Object.assign(new Error('Rate limited'), { status: 429 }); + } + return { + data: [ + { + score: 0.99, + document: { _id: '1', chunk_text: 'hello', metadata: { k: 'v' } }, + }, + ], + }; + }); + 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..e00151a 100644 --- a/src/core/pinecone/search.test.ts +++ b/src/core/pinecone/search.test.ts @@ -1,4 +1,5 @@ import { describe, it, expect, vi, afterEach } from 'vitest'; +import { AppTimeoutError } from '../server/retry.js'; import { searchIndex, mergeResults, @@ -77,6 +78,21 @@ describe('searchIndex', () => { expect(search).toHaveBeenCalledTimes(2); }); + it('retries on structured 429 without 429 in message then succeeds', async () => { + let n = 0; + const search = vi.fn().mockImplementation(async () => { + n++; + if (n < 2) { + throw Object.assign(new Error('Rate limited'), { status: 429 }); + } + return { result: { hits: [{ _id: '1', _score: 1, fields: {} }] } }; + }); + 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 +100,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/server/retry.test.ts b/src/core/server/retry.test.ts index 562b634..a5577f5 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,40 @@ 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 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('does not retry AppTimeoutError even via defaultShouldRetry', () => { + expect(defaultShouldRetry(new AppTimeoutError(50, 'search'))).toBe(false); + }); }); describe('transientShouldRetry', () => { @@ -35,12 +71,25 @@ 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); + }); }); describe('runWithPolicy', () => { @@ -82,7 +131,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 +148,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..c4b1018 100644 --- a/src/core/server/retry.ts +++ b/src/core/server/retry.ts @@ -9,10 +9,125 @@ 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. + */ +export 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 (429 or 5xx). */ +export function isRetryableHttpStatus(status: number): boolean { + return 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). + * Checks the error and its `cause` chain before falling back to {@link APP_TIMEOUT_PATTERN}. + */ export function isAppTimeoutError(error: unknown): boolean { + if ( + forEachErrorInChain(error, (current) => + current instanceof AppTimeoutError ? true : undefined + ) === true + ) { + return true; + } + const msg = error instanceof Error ? error.message : String(error); return APP_TIMEOUT_PATTERN.test(msg); } @@ -45,18 +160,22 @@ 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, structured HTTP status, + * transient Pinecone error names, then network-ish message regex as last resort. + */ export function defaultShouldRetry(error: unknown): boolean { + if (isAppTimeoutError(error)) return false; + + const status = getHttpStatus(error); + if (status !== undefined && isRetryableHttpStatus(status)) return true; + + if (hasTransientPineconeErrorName(error)) return true; + 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; + if (/ETIMEDOUT|ECONNRESET|ECONNREFUSED|ENOTFOUND/i.test(error.message)) return true; + if (/\btimeout\b/i.test(error.message)) return true; + if (/\b(429|502|503|504)\b/.test(error.message)) return true; } return false; } @@ -113,7 +232,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); + }); }); From 1d44d98890ee7a1b500ab3fa64f7d2ee89f6302a Mon Sep 17 00:00:00 2001 From: zhao Date: Fri, 24 Jul 2026 00:34:02 +0800 Subject: [PATCH 2/4] addressed ai reviews --- src/core/server/retry.test.ts | 6 ++++++ src/core/server/retry.ts | 7 ++++--- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/src/core/server/retry.test.ts b/src/core/server/retry.test.ts index a5577f5..99d25fe 100644 --- a/src/core/server/retry.test.ts +++ b/src/core/server/retry.test.ts @@ -55,6 +55,12 @@ describe('defaultShouldRetry', () => { 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', () => { diff --git a/src/core/server/retry.ts b/src/core/server/retry.ts index c4b1018..cbb9f40 100644 --- a/src/core/server/retry.ts +++ b/src/core/server/retry.ts @@ -161,14 +161,15 @@ export interface PolicyOptions { } /** - * Default retry predicate. Precedence: reject app timeouts, structured HTTP status, - * transient Pinecone error names, then network-ish message regex as last resort. + * 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 (isAppTimeoutError(error)) return false; const status = getHttpStatus(error); - if (status !== undefined && isRetryableHttpStatus(status)) return true; + if (status !== undefined) return isRetryableHttpStatus(status); if (hasTransientPineconeErrorName(error)) return true; From 4fa6ca7b43b4a37a4ee5944634600921a7c7965f Mon Sep 17 00:00:00 2001 From: zhao Date: Fri, 24 Jul 2026 14:11:36 +0800 Subject: [PATCH 3/4] addressed all reviews --- src/core/server/retry.test.ts | 17 +++++++++++++++++ src/core/server/retry.ts | 27 ++++++++++++++++++++------- 2 files changed, 37 insertions(+), 7 deletions(-) diff --git a/src/core/server/retry.test.ts b/src/core/server/retry.test.ts index 99d25fe..5f62f0c 100644 --- a/src/core/server/retry.test.ts +++ b/src/core/server/retry.test.ts @@ -27,6 +27,11 @@ describe('defaultShouldRetry', () => { 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', @@ -96,6 +101,18 @@ describe('isAppTimeoutError', () => { 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', () => { diff --git a/src/core/server/retry.ts b/src/core/server/retry.ts index cbb9f40..02a5e27 100644 --- a/src/core/server/retry.ts +++ b/src/core/server/retry.ts @@ -102,9 +102,9 @@ export function getHttpStatus(error: unknown): number | undefined { }); } -/** True when `status` is a transient HTTP code (429 or 5xx). */ +/** True when `status` is a transient HTTP code (408, 429, or 5xx). */ export function isRetryableHttpStatus(status: number): boolean { - return status === 429 || (status >= 500 && status < 600); + return status === 408 || status === 429 || (status >= 500 && status < 600); } function hasTransientPineconeErrorName(error: unknown): boolean { @@ -117,7 +117,7 @@ function hasTransientPineconeErrorName(error: unknown): boolean { /** * True for app-level {@link withTimeout} deadlines ({@link AppTimeoutError} or legacy message prefix). - * Checks the error and its `cause` chain before falling back to {@link APP_TIMEOUT_PATTERN}. + * Branded timeouts and {@link APP_TIMEOUT_PATTERN} are matched on the error `cause` chain. */ export function isAppTimeoutError(error: unknown): boolean { if ( @@ -128,8 +128,18 @@ export function isAppTimeoutError(error: unknown): boolean { return true; } - const msg = error instanceof Error ? error.message : String(error); - return APP_TIMEOUT_PATTERN.test(msg); + if ( + forEachErrorInChain(error, (current) => + APP_TIMEOUT_PATTERN.test(current.message) ? true : undefined + ) === true + ) { + return true; + } + + if (!(error instanceof Error)) { + return APP_TIMEOUT_PATTERN.test(String(error)); + } + return false; } /** Retry policy. */ @@ -176,14 +186,17 @@ export function defaultShouldRetry(error: unknown): boolean { if (error instanceof Error) { if (/ETIMEDOUT|ECONNRESET|ECONNREFUSED|ENOTFOUND/i.test(error.message)) return true; if (/\btimeout\b/i.test(error.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(error.message)) return true; } return false; } -/** 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); } From 4659791a2433e3ee1e97244236c49ee459f2281d Mon Sep 17 00:00:00 2001 From: zhao Date: Fri, 24 Jul 2026 14:36:14 +0800 Subject: [PATCH 4/4] make tests more strength --- src/core/pinecone-client.test.ts | 10 ++++----- src/core/pinecone/indexes.test.ts | 19 +++++++++++++++++ src/core/pinecone/rerank.test.ts | 25 +++++++++-------------- src/core/pinecone/search.test.ts | 11 +++------- src/core/pinecone/test-helpers.ts | 13 ++++++++++++ src/core/server/retry.test.ts | 10 +++++++++ src/core/server/retry.ts | 34 +++++++++++++++---------------- 7 files changed, 76 insertions(+), 46 deletions(-) create mode 100644 src/core/pinecone/test-helpers.ts 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 79ba2a1..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' } }, @@ -76,21 +77,15 @@ describe('rerankResults', () => { }); it('retries on structured 429 without 429 in message then succeeds', async () => { - let n = 0; - const rerank = vi.fn().mockImplementation(async () => { - n++; - if (n < 2) { - throw Object.assign(new Error('Rate limited'), { status: 429 }); - } - return { - data: [ - { - score: 0.99, - document: { _id: '1', chunk_text: 'hello', metadata: { k: 'v' } }, - }, - ], - }; - }); + 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); diff --git a/src/core/pinecone/search.test.ts b/src/core/pinecone/search.test.ts index e00151a..63775dc 100644 --- a/src/core/pinecone/search.test.ts +++ b/src/core/pinecone/search.test.ts @@ -1,5 +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, @@ -79,14 +80,8 @@ describe('searchIndex', () => { }); it('retries on structured 429 without 429 in message then succeeds', async () => { - let n = 0; - const search = vi.fn().mockImplementation(async () => { - n++; - if (n < 2) { - throw Object.assign(new Error('Rate limited'), { status: 429 }); - } - return { result: { hits: [{ _id: '1', _score: 1, fields: {} }] } }; - }); + 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); 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 5f62f0c..c94ebc3 100644 --- a/src/core/server/retry.test.ts +++ b/src/core/server/retry.test.ts @@ -57,6 +57,16 @@ describe('defaultShouldRetry', () => { 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); }); diff --git a/src/core/server/retry.ts b/src/core/server/retry.ts index 02a5e27..15b6ee6 100644 --- a/src/core/server/retry.ts +++ b/src/core/server/retry.ts @@ -45,7 +45,7 @@ function getErrorCause(error: Error): unknown { * Walk `error` and its `cause` chain (depth-capped, cycle-safe). * Stops when `visit` returns a non-undefined value or the chain ends. */ -export function forEachErrorInChain( +function forEachErrorInChain( error: unknown, visit: (current: Error) => T | undefined ): T | undefined { @@ -122,15 +122,9 @@ function hasTransientPineconeErrorName(error: unknown): boolean { export function isAppTimeoutError(error: unknown): boolean { if ( forEachErrorInChain(error, (current) => - current instanceof AppTimeoutError ? true : undefined - ) === true - ) { - return true; - } - - if ( - forEachErrorInChain(error, (current) => - APP_TIMEOUT_PATTERN.test(current.message) ? true : undefined + current instanceof AppTimeoutError || APP_TIMEOUT_PATTERN.test(current.message) + ? true + : undefined ) === true ) { return true; @@ -142,6 +136,14 @@ export function isAppTimeoutError(error: unknown): boolean { 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. */ export interface RetryOptions { /** Total number of attempts after the first try. Default 2. */ @@ -183,13 +185,11 @@ export function defaultShouldRetry(error: unknown): boolean { if (hasTransientPineconeErrorName(error)) return true; - if (error instanceof Error) { - if (/ETIMEDOUT|ECONNRESET|ECONNREFUSED|ENOTFOUND/i.test(error.message)) return true; - if (/\btimeout\b/i.test(error.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(error.message)) return true; - } - return false; + return ( + forEachErrorInChain(error, (current) => + errorMessageLooksRetryable(current.message) ? true : undefined + ) === true + ); } /**