Harden retry/timeout classification on structured HTTP status and bra… (#235)

* Harden retry/timeout classification on structured HTTP status and branded app timeouts

* addressed ai reviews

* addressed all reviews

* make tests more strength

---------

Co-authored-by: zhao <jornathanm910923@gmail.com>

Author: jonathanMLDev

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;
     });

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 } },

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);

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,19 +79,28 @@ 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;
     await expect(searchIndex(index, 'hi', 5)).rejects.toThrow(/401/);
     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;
   });

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<T>(success: T): ReturnType<typeof vi.fn> {
+  let n = 0;
+  return vi.fn().mockImplementation(async () => {
+    n++;
+    if (n < 2) {
+      throw Object.assign(new Error('Rate limited'), { status: 429 });
+    }
+    return success;
+  });
+}

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;
   });