fix(mongo): honour query timeout on count() and sum() aggregate paths#913
Conversation
count() set maxTimeMS on $options but then reassigned $options from getTransactionOptions() before the aggregate, discarding it; sum() never set it at all. A total=true document list issues count(), so its aggregation ran unbounded — a slow/large mongo read hung until the caller gave up (surfacing downstream as a ~90s 503) instead of failing fast with a Timeout. Set maxTimeMS on the options actually passed to aggregate() in both paths. Regression: testCountTimeout.
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe adapter now resets timeout state when clearing a timeout, applies Mongo query timeouts after transaction options are resolved, and adds an e2e test that verifies count() surfaces a timeout exception. ChangesTimeout handling in adapter and Mongo queries
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Test as testCountTimeout()
participant Adapter as Mongo Adapter
participant MongoDB
Test->>Adapter: setTimeout(1ms)
Test->>Adapter: count(query)
Adapter->>Adapter: getTransactionOptions()
Adapter->>MongoDB: aggregate with maxTimeMS
MongoDB-->>Adapter: timeout error
Adapter-->>Test: TimeoutException
Test->>Adapter: clearTimeout()
Test->>Adapter: delete collection
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Greptile SummaryThis PR fixes two aggregate paths in the MongoDB adapter where query timeouts were silently ignored:
Confidence Score: 5/5Safe to merge — the fix is targeted, the logic is straightforward, and both count() and sum() now correctly honour the configured timeout. The changes are narrowly scoped to moving maxTimeMS onto the right options object and adding the missing exception propagation. The core logic of each method is unchanged, the test exercises the exact failure mode described in the PR, and cleanup is correctly guarded with finally. No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "test: force real scan work in testCountT..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR fixes Mongo adapter timeout handling for aggregation-based operations so setTimeout() is honored consistently across find(), count(), and sum() paths, and adds an e2e regression test to ensure count() fails fast with a timeout.
Changes:
- Mongo adapter: apply
maxTimeMSto the actual options passed toaggregate()incount()andsum(). - Mongo adapter: remove the earlier (now-dead)
maxTimeMSassignment incount()before$optionsis reassigned. - E2E tests: add
testCountTimeout()to validatecount()throwsTimeoutExceptionwhen timeouts are supported.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/e2e/Adapter/Scopes/GeneralTests.php | Adds an e2e regression test asserting count() respects setTimeout() and throws TimeoutException. |
| src/Database/Adapter/Mongo.php | Ensures maxTimeMS is set on the aggregate() options for count() and sum() aggregation pipelines. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -2718,10 +2718,6 @@ public function count(Document $collection, array $queries = [], ?int $max = nul | |||
| $options['limit'] = $max; | |||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Database/Adapter/Mongo.php (2)
2811-2857: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
sum()never converts driver exceptions intoTimeoutException.Unlike
find()/getDocument()/count(),sum()has no try/catch around$this->client->aggregate(...). Even after addingmaxTimeMShere, a timeout raised by the driver propagates as a rawMongoExceptionrather than being converted viaprocessException()intoTimeoutException. Callers that specifically catchTimeoutExceptionaroundsum()(e.g. mirroringtestCountTimeout) won't see it.🐛 Proposed fix
- return $this->client->aggregate($name, $pipeline, $options)->cursor->firstBatch[0]->total ?? 0; + try { + return $this->client->aggregate($name, $pipeline, $options)->cursor->firstBatch[0]->total ?? 0; + } catch (MongoException $e) { + throw $this->processException($e); + }🤖 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/Database/Adapter/Mongo.php` around lines 2811 - 2857, The `sum()` method in `Mongo` currently calls `client->aggregate()` without the same exception handling used by `find()`, `getDocument()`, and `count()`, so timeout errors bypass `processException()` and never become `TimeoutException`. Wrap the aggregate call (and result access) in the same try/catch pattern as the other read methods, and on any driver exception pass it through `processException()` so callers of `sum()` get consistent timeout behavior.
2778-2795: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick wincount() should rethrow mapped Mongo exceptions instead of swallowing them.
processException()already turns Mongo codes 50/262 intoTimeoutException, but thiscatch (MongoException $e) { return 0; }bypasses that and converts a timeout into a false0result. Re-throwprocessException($e)here socount()can surface timeouts as expected.🤖 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/Database/Adapter/Mongo.php` around lines 2778 - 2795, The count() method in Mongo.php is swallowing MongoException and returning 0, which prevents mapped exceptions from surfacing. Update the catch block in count() to rethrow the result of processException($e) instead of returning a fallback value, so timeout-related Mongo codes are converted and propagated as TimeoutException as intended. Keep the fix localized to count() and reuse the existing processException() helper.
🧹 Nitpick comments (1)
src/Database/Adapter/Mongo.php (1)
2714-2722: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePre-existing dead
$options['limit']is discarded by the latergetTransactionOptions()reassignment.
$options['limit']set here (used only for the legacycountcommand, not the aggregation pipeline) is unconditionally overwritten by$options = $this->getTransactionOptions();at line 2743, making these lines effectively dead code. Not introduced by this PR, but adjacent to the exact "options get replaced" bug being fixed here — worth cleaning up to avoid future confusion.🤖 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/Database/Adapter/Mongo.php` around lines 2714 - 2722, The $options['limit'] assignment in Mongo::buildFilters/count logic is dead because $options is later replaced by getTransactionOptions(), so clean up the pre-existing unused limit handling. Remove the obsolete limit setup or move any legacy count-specific option building into the code path that actually sends the count command, and keep the buildFilters/getTransactionOptions flow consistent so the final $options used for the query is the only one that matters.
🤖 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.
Outside diff comments:
In `@src/Database/Adapter/Mongo.php`:
- Around line 2811-2857: The `sum()` method in `Mongo` currently calls
`client->aggregate()` without the same exception handling used by `find()`,
`getDocument()`, and `count()`, so timeout errors bypass `processException()`
and never become `TimeoutException`. Wrap the aggregate call (and result access)
in the same try/catch pattern as the other read methods, and on any driver
exception pass it through `processException()` so callers of `sum()` get
consistent timeout behavior.
- Around line 2778-2795: The count() method in Mongo.php is swallowing
MongoException and returning 0, which prevents mapped exceptions from surfacing.
Update the catch block in count() to rethrow the result of processException($e)
instead of returning a fallback value, so timeout-related Mongo codes are
converted and propagated as TimeoutException as intended. Keep the fix localized
to count() and reuse the existing processException() helper.
---
Nitpick comments:
In `@src/Database/Adapter/Mongo.php`:
- Around line 2714-2722: The $options['limit'] assignment in
Mongo::buildFilters/count logic is dead because $options is later replaced by
getTransactionOptions(), so clean up the pre-existing unused limit handling.
Remove the obsolete limit setup or move any legacy count-specific option
building into the code path that actually sends the count command, and keep the
buildFilters/getTransactionOptions flow consistent so the final $options used
for the query is the only one that matters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0b34a94a-2bed-4f96-80ce-2c90c7854161
📒 Files selected for processing (2)
src/Database/Adapter/Mongo.phptests/e2e/Adapter/Scopes/GeneralTests.php
…SQL adapters - Mongo count() swallowed every client exception and returned 0, so a maxTimeMS expiry never surfaced; rethrow timeouts via processException. - Mongo sum() threw the raw client exception; route it through processException so timeouts map to the Timeout exception. - Remove count()'s dead early $options['limit'] assignment (the limit is applied via the $limit pipeline stage). - Adapter::clearTimeout() now resets the timeout property. Postgres applies the property on every statement (SET statement_timeout), so a cleared timeout kept cancelling all subsequent heavy statements on the connection. - Harden testCountTimeout: cleanup in finally, fixture read once, and the assertion moved out of the catch so a swallowed timeout fails clearly.
|
All review comments addressed in
The CI failures had two distinct causes, both real bugs this PR now fixes:
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/Database/Adapter/Mongo.php (2)
110-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant timeout reset after parent call.
parent::clearTimeout($event)now sets$this->timeout = 0itself (per theAdapter.phpchange), so the explicit$this->timeout = 0;here is now dead/duplicate code.♻️ Proposed cleanup
public function clearTimeout(string $event): void { parent::clearTimeout($event); - - $this->timeout = 0; }🤖 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/Database/Adapter/Mongo.php` around lines 110 - 115, The clearTimeout method in Mongo now duplicates the timeout reset already handled by Adapter::clearTimeout($event). Remove the extra $this->timeout = 0 assignment from Mongo::clearTimeout so the method only delegates to the parent and avoids redundant state updates.
2773-2796: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNon-timeout aggregation errors are still silently swallowed as
0.Only
TimeoutExceptionis rethrown; any other processedMongoException(e.g. a genuine query/connection failure) still falls through toreturn 0, indistinguishable from "no matching documents." This predates this change but is now more visible since the catch block was touched directly.Consider rethrowing all processed exceptions except cases genuinely intended to yield
0(e.g.NamespaceNotFound), so callers aren't misled by transient/real errors.🤖 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/Database/Adapter/Mongo.php` around lines 2773 - 2796, The aggregation error handling in Mongo::aggregateCount (the try/catch around $this->client->aggregate) still swallows non-timeout MongoException cases by returning 0, which hides real failures as “no results.” Update the catch block to rethrow processed exceptions unless they are explicitly intended to map to 0, keeping the existing TimeoutException handling but allowing genuine query/connection errors (after processException) to propagate to callers.
🤖 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/Database/Adapter/Mongo.php`:
- Around line 110-115: The clearTimeout method in Mongo now duplicates the
timeout reset already handled by Adapter::clearTimeout($event). Remove the extra
$this->timeout = 0 assignment from Mongo::clearTimeout so the method only
delegates to the parent and avoids redundant state updates.
- Around line 2773-2796: The aggregation error handling in Mongo::aggregateCount
(the try/catch around $this->client->aggregate) still swallows non-timeout
MongoException cases by returning 0, which hides real failures as “no results.”
Update the catch block to rethrow processed exceptions unless they are
explicitly intended to map to 0, keeping the existing TimeoutException handling
but allowing genuine query/connection errors (after processException) to
propagate to callers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a69d4795-6214-49d5-84a2-52f38e4a2cb9
📒 Files selected for processing (3)
src/Database/Adapter.phpsrc/Database/Adapter/Mongo.phptests/e2e/Adapter/Scopes/GeneralTests.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/Adapter/Scopes/GeneralTests.php
COUNT with a notEqual filter finishes in microseconds on Postgres (no row serialization, length-first text comparison), so statement_timeout never fired. A substring scan walks every 14MB value on all engines. Verified locally on Postgres, MongoDB, MariaDB and SharedTables/Postgres.
Problem
The Mongo adapter drops
maxTimeMSon thecount()andsum()aggregation paths, so those operations run unbounded even when a query timeout is set viasetTimeout():count()sets$options['maxTimeMS']early, but then reassigns$options = $this->getTransactionOptions();before building the aggregate pipeline, discarding the timeout.sum()never setsmaxTimeMSat all.find()is unaffected (it setsmaxTimeMSon the options it actually passes), which is why this went unnoticed.Impact: a
total=truedocument list issuescount(), so a large/slow Mongo read hangs until the client gives up instead of failing fast with aTimeout. Downstream (Appwrite Cloud DocumentsDB) this surfaced as a request stuck ~90s → 503, tying up a worker the whole time.Fix
Set
maxTimeMSon the options actually passed toaggregate()in bothcount()andsum(), and remove the now-dead early assignment incount().Test
testCountTimeout(shared adapter scope): builds a large collection, sets a 1ms timeout, and assertscount()throwsTimeout. Fails without this change on Mongo (count completes unbounded); passes with it.count()is type-agnostic so it is safe across all timeout-supporting adapters (MongomaxTimeMS, MariaDBMAX_EXECUTION_TIME, Postgresstatement_timeout).Summary by CodeRabbit