Wire request timeout and 429/5xx retry onto Pinecone I/O hot path - #225
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughPinecone outbound operations now use centralized per-attempt timeouts and transient retries. Configured timeouts propagate through search and reranking, index-session calls are wrapped consistently, and timeout classification uses shared retry utilities. Tests cover retries, non-retryable errors, timeout behavior, propagation, and degradation. ChangesPinecone timeout and retry policy
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PineconeClient
participant PineconeIndexSession
participant runWithPolicy
participant Pinecone
PineconeClient->>PineconeIndexSession: getRequestTimeoutMs()
PineconeClient->>runWithPolicy: execute search or rerank with timeout
runWithPolicy->>Pinecone: Pinecone request
Pinecone-->>runWithPolicy: result or transient error
runWithPolicy-->>PineconeClient: result, retry, or timeout
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #225 +/- ##
=======================================
Coverage ? 85.86%
=======================================
Files ? 47
Lines ? 2512
Branches ? 852
=======================================
Hits ? 2157
Misses ? 354
Partials ? 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/core/pinecone/indexes.ts (2)
277-317: 🩺 Stability & Availability | 🔵 TrivialStartup reachability check inherits default retries, extending failure-detection time.
checkIndexes()is meant to fail fast before the server starts, but eachrunIocall here uses the default 2 retries with backoff, so an unreachable endpoint could take up to ~3x the per-attempt timeout (default 15s) plus backoff before--check-indexesreports failure. Consider passing a lowerretries(e.g., 0) for this specific health-check path if fast failure at startup is preferred over resilience.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/pinecone/indexes.ts` around lines 277 - 317, Update the two runIo calls in checkIndexes—describeIndexStats-dense and describeIndexStats-sparse—to pass retries: 0 for this startup reachability check, while leaving runIo defaults unchanged elsewhere.
64-67: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
runIosilently drops theAbortSignalfromrunWithPolicy.
runWithPolicy'sfnparameter is typed(signal: AbortSignal) => Promise<T>, butrunIocalls it as() => fn(), so the signal fromwithTimeout'sAbortControllernever reaches the wrapped SDK calls. Since allrunIocall sites are read-only (stats/fetch/query), this isn't a correctness issue, just means in-flight requests keep running in the background after a timeout is declared (wasted request, no actual cancellation). Consider forwarding the signal for consistency with the policy's cancellation contract, even if unused today.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/pinecone/indexes.ts` around lines 64 - 67, Update the private runIo method to accept the AbortSignal supplied by runWithPolicy and forward it to the wrapped callback, preserving the existing timeout and policy options while maintaining the current call-site behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/pinecone-client.ts`:
- Around line 108-116: Import isAppTimeoutError from the retry module and update
the Promise.allSettled rejection handling in query: when both dense and sparse
searches reject, rethrow whichever rejection is an application timeout before
falling back to the existing generic hybrid-search error.
---
Nitpick comments:
In `@src/core/pinecone/indexes.ts`:
- Around line 277-317: Update the two runIo calls in
checkIndexes—describeIndexStats-dense and describeIndexStats-sparse—to pass
retries: 0 for this startup reachability check, while leaving runIo defaults
unchanged elsewhere.
- Around line 64-67: Update the private runIo method to accept the AbortSignal
supplied by runWithPolicy and forward it to the wrapped callback, preserving the
existing timeout and policy options while maintaining the current call-site
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ddb4027-c7c9-4e88-9aa2-5c5929cf066f
📒 Files selected for processing (11)
src/core/pinecone-client.test.tssrc/core/pinecone-client.tssrc/core/pinecone/indexes.test.tssrc/core/pinecone/indexes.tssrc/core/pinecone/rerank.test.tssrc/core/pinecone/rerank.tssrc/core/pinecone/search.test.tssrc/core/pinecone/search.tssrc/core/server/retry.test.tssrc/core/server/retry.tssrc/core/server/tool-error.ts
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/pinecone/indexes.ts (1)
64-67: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
AbortSignalfromrunWithPolicyis never used to cancel the underlying Pinecone call.
runIo,searchIndex, andrerankResultsall accept asignal/_signalparameter fromrunWithPolicybut never pass it to the actual SDK call. On timeout, the promise wrapper rejects but the in-flight HTTP request to Pinecone keeps running; combined with retries, transient outages can pile up multiple abandoned in-flight requests per logical call.
src/core/pinecone/indexes.ts#L64-L67: forward the signal intofnif the SDK call site supports an abort option.src/core/pinecone/search.ts#L44-L87: forward_signalintoindex.search/target.searchRecordsif supported.src/core/pinecone/rerank.ts#L27-L53: forward_signalintopc.inference.rerankif supported.Please check whether
@pinecone-database/pineconev8 SDK calls accept anAbortSignal/similar cancellation option before wiring this through.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/pinecone/indexes.ts` around lines 64 - 67, Verify the `@pinecone-database/pinecone` v8 SDK cancellation API, then update runIo in src/core/pinecone/indexes.ts (lines 64-67) to pass runWithPolicy’s AbortSignal into fn and propagate it through the SDK request options where supported. In src/core/pinecone/search.ts (lines 44-87), forward _signal to index.search and target.searchRecords; in src/core/pinecone/rerank.ts (lines 27-53), forward _signal to pc.inference.rerank. Preserve behavior without inventing unsupported SDK options, and ensure the signal reaches every supported underlying Pinecone call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/core/pinecone/indexes.ts`:
- Around line 64-67: Verify the `@pinecone-database/pinecone` v8 SDK cancellation
API, then update runIo in src/core/pinecone/indexes.ts (lines 64-67) to pass
runWithPolicy’s AbortSignal into fn and propagate it through the SDK request
options where supported. In src/core/pinecone/search.ts (lines 44-87), forward
_signal to index.search and target.searchRecords; in src/core/pinecone/rerank.ts
(lines 27-53), forward _signal to pc.inference.rerank. Preserve behavior without
inventing unsupported SDK options, and ensure the signal reaches every supported
underlying Pinecone call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8025bcb1-d2d4-406a-910e-8978a7f43cfa
📒 Files selected for processing (11)
src/core/pinecone-client.test.tssrc/core/pinecone-client.tssrc/core/pinecone/indexes.test.tssrc/core/pinecone/indexes.tssrc/core/pinecone/rerank.test.tssrc/core/pinecone/rerank.tssrc/core/pinecone/search.test.tssrc/core/pinecone/search.tssrc/core/server/retry.test.tssrc/core/server/retry.tssrc/core/server/tool-error.ts
…jonathanMLDev/pinecone-read-only-mcp-typescript into fix/wire-timeout-retry-hot-path # Conflicts: # src/core/pinecone/rerank.test.ts
- added some tests
3 participants
Summary
runWithPolicy(withRetry+withTimeout) withtransientShouldRetryso app-level timeouts fail fast while 429/502/503/504 and network errors get bounded backoffdescribeIndexStats, namespace metadata sampling, andfetchRecordFieldsrequestTimeoutMsfromPineconeClientthroughPineconeIndexSession.getRequestTimeoutMs()into all outbound call sitesisAppTimeoutError(shared bysearch.tsandtool-error.ts)Test plan
npm run cigreen (415 tests, 85%+ coverage)retry.test.ts,search.test.ts,rerank.test.ts,indexes.test.ts,pinecone-client.test.tsFollow-up (not in scope)
Hybrid per-leg search timeouts are logged but still surface as a generic error when both legs fail;
TIMEOUTtool-error classification for partial hybrid degradation is unchanged.Related Issue
Summary by CodeRabbit