fix: make CALL_TIMEOUT_MS env-overridable and raise HTTP worker default_timeout to 600 s#867
Open
kek-Sec wants to merge 2 commits into
Open
fix: make CALL_TIMEOUT_MS env-overridable and raise HTTP worker default_timeout to 600 s#867kek-Sec wants to merge 2 commits into
kek-Sec wants to merge 2 commits into
Conversation
…aults to 600 s Hardcoded 15 s proxy timeout causes memory_consolidate and memory_reflect to abort on large stores (1 100+ semantic memories, 630+ insights) before the server finishes the LLM-backed operation. - Replace CALL_TIMEOUT_MS = 15_000 with DEFAULT_CALL_TIMEOUT_MS = 600_000 and a callTimeoutMs() helper (mirrors existing probeTimeoutMs() pattern) that reads AGENTMEMORY_CALL_TIMEOUT_MS from the environment so users can tune the ceiling without changing code. - Raise iii-config.yaml default_timeout from 180 000 ms to 600 000 ms for the HTTP worker, keeping both ceilings in sync. - Export callTimeoutMs() and add test/mcp-call-timeout.test.ts covering the default value, env-var parsing, malformed-value fallback, and the abort behaviour under a hanging server response. Fixes rohitg00#866 Signed-off-by: George Petrakis <g.petrakis@natechbanking.com>
|
@kek-Sec is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
📝 WalkthroughWalkthroughRaise the MCP proxy default call timeout to 600000 ms, add exported callTimeoutMs() that reads and validates AGENTMEMORY_CALL_TIMEOUT_MS (floors floats, clamps to Node.js timer ceiling), use it with AbortSignal.timeout() for proxied fetch calls, update iii-http worker default_timeout, and add unit/integration tests for parsing and runtime behavior. ChangesMCP Proxy Call Timeout Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@iii-config.yaml`:
- Line 6: Update the Docker worker timeout to match the new default_timeout of
600000 by changing the timeout value in iii-config.docker.yaml from 180000 to
600000; locate the Docker-specific timeout setting (the default_timeout or
docker worker timeout key) in iii-config.docker.yaml and set it to 600000 so
Docker users use the same 600s ceiling as the main default_timeout.
In `@src/mcp/rest-proxy.ts`:
- Around line 13-18: callTimeoutMs() currently validates positive numeric input
but doesn't cap huge values; clamp the computed timeout to Node's safe maximum
timer value (2147483647 ms) before returning so values passed to
AbortSignal.timeout(...) (used later) won't be misinterpreted or immediately
fire. Update callTimeoutMs() to compute n = Math.floor(Number(raw)) as now, then
return Math.min(n, 2147483647) (falling back to DEFAULT_CALL_TIMEOUT_MS when
invalid) so timeouts are both positive and within the safe upper bound.
🪄 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: 0fc4e594-d8ae-4cd5-b764-9c6b1ba71eb7
📒 Files selected for processing (3)
iii-config.yamlsrc/mcp/rest-proxy.tstest/mcp-call-timeout.test.ts
…e max - iii-config.docker.yaml: raise default_timeout 180 000 → 600 000 to match iii-config.yaml (Docker users get the same ceiling as the default config) - rest-proxy.ts: clamp AGENTMEMORY_CALL_TIMEOUT_MS to 2 147 483 647 ms (Node.js signed 32-bit timer ceiling); values above this wrap to negative and fire AbortSignal immediately - test/mcp-call-timeout.test.ts: add two tests covering the clamp boundary Signed-off-by: George Petrakis <g.petrakis@natechbanking.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/mcp-call-timeout.test.ts (1)
110-121: ⚡ Quick winConsider asserting the specific error type for abort timeout.
The test currently uses a generic
.rejects.toThrow()which passes for any error. SinceAbortSignal.timeout()throws a specificDOMExceptionwith name"AbortError", consider asserting the error type to improve test precision and catch regressions more reliably.✨ Proposed improvement
await expect( handle.call("/agentmemory/smart-search", { method: "POST", body: JSON.stringify({ query: "test" }), }), - ).rejects.toThrow(); + ).rejects.toThrow(/abort/i);Or for even more precision:
- ).rejects.toThrow(); + ).rejects.toMatchObject({ + name: "AbortError", + });🤖 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/mcp-call-timeout.test.ts` around lines 110 - 121, The test currently uses a generic .rejects.toThrow() which accepts any error; update the assertion to check for the specific abort error thrown by AbortSignal.timeout — assert that the rejection is a DOMException/AbortError when calling handle.call("/agentmemory/smart-search", ...) in the test named "aborts a proxy call that hangs beyond the configured timeout" (or assert the error.name === "AbortError" / use toThrow with the AbortError constructor/message) so the test only passes for the intended timeout abort.
🤖 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 `@test/mcp-call-timeout.test.ts`:
- Around line 110-121: The test currently uses a generic .rejects.toThrow()
which accepts any error; update the assertion to check for the specific abort
error thrown by AbortSignal.timeout — assert that the rejection is a
DOMException/AbortError when calling handle.call("/agentmemory/smart-search",
...) in the test named "aborts a proxy call that hangs beyond the configured
timeout" (or assert the error.name === "AbortError" / use toThrow with the
AbortError constructor/message) so the test only passes for the intended timeout
abort.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a27a95a-6a6b-40dd-bed3-f2017b8dca3b
📒 Files selected for processing (3)
iii-config.docker.yamlsrc/mcp/rest-proxy.tstest/mcp-call-timeout.test.ts
✅ Files skipped from review due to trivial changes (1)
- iii-config.docker.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- src/mcp/rest-proxy.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
CALL_TIMEOUT_MSinsrc/mcp/rest-proxy.tsis now env-overridable viaAGENTMEMORY_CALL_TIMEOUT_MS, with a default of 600 000 ms (was 15 000 ms).Mirrors the existing
AGENTMEMORY_PROBE_TIMEOUT_MSpattern already in thesame file.
iii-config.yamldefault_timeoutraised from 180 000 ms to 600 000 ms.callTimeoutMs()is exported;test/mcp-call-timeout.test.tscovers thedefault value, env-var parsing, malformed-value fallback, and the abort
behaviour under a hanging server response (9 new tests, all pass).
Why
On stores with 1 100+ semantic memories and 630+ insights,
memory_consolidateandmemory_reflectreliably time out because theproxy's HTTP client aborts the request after 15 s — before the server
finishes the LLM-backed operation.
Making the timeout env-overridable is a non-breaking improvement: users who
don't set the env var get the new 600 s default; anyone who needs a
different ceiling can set
AGENTMEMORY_CALL_TIMEOUT_MSin their MCP serverenv config.
Related to #655 (which addresses the server-side sequential KV bottleneck).
This addresses the client-side transport ceiling — both fixes are needed for
large stores to complete reliably.
How to verify
memory_consolidate— it should complete instead of returning atimeout error.
AGENTMEMORY_CALL_TIMEOUT_MS=5000and confirm a small store stillrespects the override.
npm test— the new suite attest/mcp-call-timeout.test.tspasses (9tests); no regressions in the existing suite.
Fixes #866
Summary by CodeRabbit
New Features
Tests