-
Notifications
You must be signed in to change notification settings - Fork 181
feat(config): add maxTimeMs configuration for query timeout protection #830
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Add MDB_MCP_MAX_TIME_MS / --maxTimeMs configuration option that applies MongoDB's maxTimeMS parameter to find, aggregate, and export operations. This provides protection against runaway AI-generated queries by automatically terminating long-running queries after a configurable duration (default: 30 seconds). - Add maxTimeMs config option with default 30000ms - Apply maxTimeMS to find tool cursor - Apply maxTimeMS to aggregate tool cursor - Apply maxTimeMS to export tool for both find and aggregate cursors - Update README documentation
- Add maxTimeMs to expectedDefaults in config.test.ts - Add integration tests for find tool with maxTimeMs enabled/disabled - Add integration tests for aggregate tool with maxTimeMs enabled/disabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces a maxTimeMs configuration option to protect against long-running MongoDB queries by automatically terminating find and aggregate operations after a specified duration. The default timeout is 30 seconds, and setting it to 0 disables the feature.
Key changes:
- Added
maxTimeMsconfig parameter with safe override behavior usingonlyLowerThanBaseValueOverride() - Updated
find,aggregate, andexporttools to apply the timeout viamaxTimeMScursor option - Added comprehensive integration tests verifying both enabled and disabled timeout scenarios
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/common/config/userConfig.ts | Adds the maxTimeMs configuration schema with a 30-second default |
| src/tools/mongodb/read/find.ts | Applies maxTimeMS to find cursor operations |
| src/tools/mongodb/read/aggregate.ts | Applies maxTimeMS to aggregate operations |
| src/tools/mongodb/read/export.ts | Applies maxTimeMS to both find and aggregate cursors in export operations |
| tests/unit/common/config.test.ts | Updates expected defaults to include the new maxTimeMs setting |
| tests/integration/tools/mongodb/read/find.test.ts | Adds integration tests for find tool with maxTimeMs enabled and disabled |
| tests/integration/tools/mongodb/read/aggregate.test.ts | Adds integration tests for aggregate tool with maxTimeMs enabled and disabled |
| README.md | Documents the new configuration option |
| it("should apply maxTimeMS to queries", async () => { | ||
| await integration.connectMcpClient(); | ||
| // With a reasonable timeout, the query should succeed | ||
| const response = await integration.mcpClient().callTool({ | ||
| name: "find", | ||
| arguments: { | ||
| database: integration.randomDbName(), | ||
| collection: "foo", | ||
| filter: {}, | ||
| limit: 10, | ||
| }, | ||
| }); | ||
|
|
||
| const content = getResponseContent(response); | ||
| expect(content).toContain('Query on collection "foo" resulted in'); | ||
| expect(content).not.toContain("error"); | ||
| }); |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test verifies that queries succeed with a timeout configured, but doesn't actually validate that the maxTimeMS option is being applied to the cursor. Consider adding a test that uses a very short timeout (e.g., 1ms) with a query that's expected to take longer, then verify that a timeout error occurs. This would confirm the timeout mechanism is working as intended.
| it("should apply maxTimeMS to aggregations", async () => { | ||
| await integration.connectMcpClient(); | ||
| // With a reasonable timeout, the aggregation should succeed | ||
| const response = await integration.mcpClient().callTool({ | ||
| name: "aggregate", | ||
| arguments: { | ||
| database: integration.randomDbName(), | ||
| collection: "people", | ||
| pipeline: [{ $match: { age: { $gte: 10 } } }], | ||
| }, | ||
| }); | ||
|
|
||
| const content = getResponseContent(response); | ||
| expect(content).toContain("The aggregation resulted in"); | ||
| expect(content).not.toContain("error"); | ||
| }); |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the find tool test, this validates that aggregations succeed with a timeout but doesn't verify that the timeout is actually enforced. Add a test case with a very short timeout (e.g., 1ms) on an aggregation expected to take longer to confirm the timeout mechanism properly terminates long-running operations.
| aggregationCursor = provider.aggregate(database, collection, cappedResultsPipeline); | ||
| aggregationCursor = provider.aggregate(database, collection, cappedResultsPipeline, { | ||
| maxTimeMS: this.config.maxTimeMs || undefined, | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the option is set on the MCP-server-wide config, should we set it on the MongoClient or NodeDriverServiceProvider instead? Or otherwise, if it's not meant to apply to all operations and really only find and aggregate, should it be named differently so it's clear that it's not the standard driver maxTimeMs option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great question! After checking, the MongoDB driver's maxTimeMS option is per-operation, not a MongoClient-level setting, so setting it globally on the client isn't directly possible.
Given that, I see a few options:
- Rename to clarify scope - Something like
queryMaxTimeMsorreadMaxTimeMsto indicate it only applies to find/aggregate/export operations - Extend to more operations - Apply maxTimeMS to other read operations like count, listCollections, etc.
- Keep as-is - The current name matches the MongoDB driver option name, which might be intuitive for users familiar with MongoDB
What would you prefer? I'm happy to adjust based on your guidance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have timeoutMS that is being set to 30s when constructing the driver options:
| timeoutMS: 30_000, |
Should this be the thing we configure instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that should be the right place to consume this if we make it configurable
I was also wondering about maxTimeMS vs timeoutMS here, for a lot of devtools things the former is what makes the most sense since the primary concern is avoiding runaway tasks on the cluster and it's generally fine if the rest of the operation (e.g. network handling) takes a bit longer, I think a similar logic applies for MCP but I could see an argument to be made that if a response doesn't come within a specific amount of time, it's fine to fail it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intentionally use maxTimeMS (per-operation) rather than timeoutMS (client-wide) because the goal is server-side protection against expensive AI-generated queries. maxTimeMS is enforced by MongoDB itself and only counts server execution time, whereas timeoutMS includes network latency and is client-enforced.
That said, I think making timeoutMS configurable makes sense as a separate feature for controlling deterministic MCP behavior (i.e., "the MCP should respond within X ms or fail"). Happy to add that in a follow-up PR if there's interest.
For this PR, I can rename the config to queryMaxTimeMs to clarify it applies to query operations (find/aggregate) and isn't the standard driver option. I can also extend it to other potentially expensive read operations like count and listCollections.
|
Note this PR was generated with LLM assistance (Claude Opus 4.5) |
Description
Add a
maxTimeMsconfiguration option to automatically terminate long-runningfindandaggregateoperations after a specified duration. This provides resource protection for AI-generated queries that may inadvertently perform expensive operations.Closes #829
Jira: MCP-349
Motivation
When using the MongoDB MCP server with AI assistants, there's a risk of generating queries that scan large collections or perform expensive aggregations. Without a timeout, these queries can:
Changes
Configuration
maxTimeMsconfig option (default: 30000ms / 30 seconds)0disables the timeoutonlyLowerThanBaseValueOverridefor safe config mergingTools Updated
maxTimeMSto cursor optionsmaxTimeMSto aggregate optionsmaxTimeMSto both find and aggregate cursorsTests
config.test.tswith maxTimeMs in expectedDefaultsDocumentation
Example Configuration
{ "maxTimeMs": 30000 }Checklist
pnpm run check)maxTimeMSConfiguration Option for Query Timeout #829)