fix: add AbortController timeout to all raw-fetch providers#379
fix: add AbortController timeout to all raw-fetch providers#379SATYAM-PRATIBHAN wants to merge 2 commits into
Conversation
|
@SATYAM-PRATIBHAN is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
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 (10)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughAdds a shared fetchWithTimeout helper (env AGENTMEMORY_LLM_TIMEOUT_MS, default 60s), updates LLM and embedding providers to use it for outbound requests, and adds unit/regression tests plus README documentation. ChangesFetch timeout mechanism and provider adoption
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![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 (1)
test/fetch-timeout.test.ts (1)
18-18: 💤 Low valueConsider a more descriptive DOMException message.
Both DOMException instances use
"AbortError"for both the message and name parameters. While functional, it's more idiomatic to use a descriptive message like"The operation was aborted"for the first parameter, reserving"AbortError"for the name only.♻️ Suggested improvement
if (init.signal.aborted) { - reject(new DOMException("AbortError", "AbortError")); + reject(new DOMException("The operation was aborted", "AbortError")); return; } init.signal.addEventListener("abort", () => { - reject(new DOMException("AbortError", "AbortError")); + reject(new DOMException("The operation was aborted", "AbortError")); });Also applies to: 22-22
🤖 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 `@test/fetch-timeout.test.ts` at line 18, Replace the two uses of reject(new DOMException("AbortError", "AbortError")) in the test so the first argument is a descriptive message (e.g., "The operation was aborted") and the second argument remains the name "AbortError"; update both occurrences of the reject(new DOMException(...)) calls in test/fetch-timeout.test.ts accordingly.
🤖 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/providers/_fetch.ts`:
- Around line 8-14: The wrapper currently allows invalid timeout values
(NaN/negative) and unconditionally replaces any caller-provided init.signal with
the timeout controller (ctl), losing upstream abort propagation; fix by
validating and normalizing ms: compute let ms = timeoutMs ??
parseInt(getEnvVar("AGENTMEMORY_LLM_TIMEOUT_MS") ?? "60000", 10) and if ms is
not a finite positive number set ms = 60000, and preserve/compose existing
init.signal by creating the timeout AbortController (ctl) but if init.signal
exists then if init.signal.aborted call ctl.abort() immediately and otherwise
add an event listener init.signal.addEventListener('abort', () => ctl.abort())
so upstream aborts propagate to ctl; pass ctl.signal to fetch and ensure you
remove the event listener in the finally/cleanup (and still clearTimeout(t));
reference symbols: timeoutMs, getEnvVar, ms, AbortController, ctl, init.signal,
t, fetch.
---
Nitpick comments:
In `@test/fetch-timeout.test.ts`:
- Line 18: Replace the two uses of reject(new DOMException("AbortError",
"AbortError")) in the test so the first argument is a descriptive message (e.g.,
"The operation was aborted") and the second argument remains the name
"AbortError"; update both occurrences of the reject(new DOMException(...)) calls
in test/fetch-timeout.test.ts accordingly.
🪄 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: 8324a3bb-ea06-4412-9351-65f20037ca51
📒 Files selected for processing (10)
README.mdsrc/providers/_fetch.tssrc/providers/embedding/cohere.tssrc/providers/embedding/gemini.tssrc/providers/embedding/openai.tssrc/providers/embedding/openrouter.tssrc/providers/embedding/voyage.tssrc/providers/minimax.tssrc/providers/openrouter.tstest/fetch-timeout.test.ts
…ohitg00#373) Signed-off-by: Satyam Pratibhan <142714564+SATYAM-PRATIBHAN@users.noreply.github.com.>
…hTimeout Signed-off-by: Satyam Pratibhan <142714564+SATYAM-PRATIBHAN@users.noreply.github.com.>
SATYAM-PRATIBHAN
force-pushed
the
fix/provider-fetch-timeout
branch
from
32c421f to
8ee2926
Compare
1 participant
Closes #373
What
Adds a shared
fetchWithTimeouthelper (src/providers/_fetch.ts) and wires it into every raw-fetch provider so outbound HTTP calls can't hang indefinitely.Why
All raw-fetch providers were calling
fetch()with no timeout. A hung upstream (rate-limit hold, TLS stall, slow-loris) would block compress/summarize/embed forever — manifesting as fly machines stuck "warming up" and observation pipelines silently halting.Changes
src/providers/_fetch.ts— new shared helper; readsAGENTMEMORY_LLM_TIMEOUT_MS(default 60s)fetch()withfetchWithTimeout()in all 7 raw-fetch providers:providers/minimax.tsproviders/openrouter.ts(covers Gemini LLM path too)providers/embedding/{gemini,openai,cohere,voyage,openrouter}.tsREADME.md— documentsAGENTMEMORY_LLM_TIMEOUT_MSin the env-vars blocktest/fetch-timeout.test.ts— 11 regression tests; mocks a fetch that only resolves onAbortSignaland asserts each provider rejects within the timeoutOut of scope
Retry/backoff and per-provider timeout overrides (
AGENTMEMORY_ANTHROPIC_TIMEOUT_MSetc.) — as noted in the issue.Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests