Conversation
There was a problem hiding this comment.
Pull request overview
Updates the repository’s Client-Side Operations Timeout (CSOT) unified spec test suite to the latest upstream versions, along with runner/driver changes needed to satisfy new timing and behavioral requirements (e.g., minPoolSize readiness, withTransaction CSOT semantics, and tailable-awaitData timeout refresh rules).
Changes:
- Synced/expanded CSOT unified spec YAMLs (schema bumps, adjusted timeout values, new scenarios like wait queue timeout behavior, and index pre-creation to avoid IndexNotFound differences across server versions).
- Enhanced the unified test runner to support
awaitMinPoolSizeMS, improvewithTransactionretries by propagating callback errors, and extend event filtering. - Updated driver CSOT behavior for withTransaction, cursor/change stream timeout refresh, getMore
maxAwaitTimeMShandling, and timeout override validation.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/spec_tests/data/client_side_operations_timeout/waitQueueTimeout.yml | Adds CSOT spec coverage ensuring wait queue timeouts don’t clear the pool. |
| spec/spec_tests/data/client_side_operations_timeout/tailable-non-awaitData.yml | Updates timeouts/blocking intervals for stability and spec alignment. |
| spec/spec_tests/data/client_side_operations_timeout/tailable-awaitData.yml | Adds/extends validation and maxAwaitTimeMS-vs-timeoutMS scenarios; adds remaining-timeout behavior checks. |
| spec/spec_tests/data/client_side_operations_timeout/sessions-override-timeoutMS.yml | Adjusts timeouts and expected abortTransaction behavior/events in withTransaction flows. |
| spec/spec_tests/data/client_side_operations_timeout/sessions-override-operation-timeoutMS.yml | Same as above, but for per-operation timeout overrides. |
| spec/spec_tests/data/client_side_operations_timeout/sessions-inherit-timeoutMS.yml | Bumps schema, adds minPoolSize/awaitMinPoolSizeMS, adjusts blocking/timeouts and expected events. |
| spec/spec_tests/data/client_side_operations_timeout/retryability-timeoutMS.yml | Bumps schema and adds minPoolSize/awaitMinPoolSizeMS to reduce flakiness. |
| spec/spec_tests/data/client_side_operations_timeout/override-operation-timeoutMS.yml | Adds index pre-creation before dropIndex to avoid server-version dependent IndexNotFound. |
| spec/spec_tests/data/client_side_operations_timeout/override-database-timeoutMS.yml | Adds generated database-level override coverage across many operations. |
| spec/spec_tests/data/client_side_operations_timeout/override-collection-timeoutMS.yml | Adds index pre-creation before dropIndex and updates expectations. |
| spec/spec_tests/data/client_side_operations_timeout/non-tailable-cursors.yml | Bumps schema, adds pool readiness settings, and adjusts timings to match new spec baselines. |
| spec/spec_tests/data/client_side_operations_timeout/global-timeoutMS.yml | Bumps schema; adds minPoolSize/awaitMinPoolSizeMS in many subtests for stability. |
| spec/spec_tests/data/client_side_operations_timeout/error-transformations.yml | Bumps schema; adds pool readiness settings. |
| spec/spec_tests/data/client_side_operations_timeout/deprecated-options.yml | Adds index pre-creation before dropIndex scenarios and updates expected events. |
| spec/spec_tests/data/client_side_operations_timeout/convenient-transactions.yml | Bumps schema; updates withTransaction timing and adds transient-retry timeout behavior test. |
| spec/spec_tests/data/client_side_operations_timeout/command-execution.yml | Bumps schema; adds pool readiness settings to multiple tests. |
| spec/spec_tests/data/client_side_operations_timeout/close-cursors.yml | Adjusts timeouts/blocking and close timeout override expectations. |
| spec/spec_tests/data/client_side_operations_timeout/change-streams.yml | Adjusts timeouts/blocking and aligns comments/expectations with updated spec behavior. |
| spec/spec_tests/client_side_operations_timeout_spec.rb | Retries CSOT spec suite to reduce intermittent failures. |
| spec/runners/unified/test.rb | Implements awaitMinPoolSizeMS and adds error propagation support for withTransaction callback operations. |
| spec/runners/unified/support_operations.rb | Ensures withTransaction callback ops are re-created per retry and adds commandName filtering in event selection. |
| spec/runners/unified/crud_operations.rb | Adds support for maxAwaitTimeMS in aggregate unified ops options extraction. |
| spec/runners/unified/change_stream_operations.rb | Refreshes CSOT timeout per “next call” and changes iterateUntilDocumentOrError behavior to poll until document/error. |
| spec/mongo/session_transaction_spec.rb | Updates expectation to TimeoutError for CSOT-related withTransaction timeout behavior. |
| lib/mongo/session.rb | Implements withTransaction CSOT timeout inheritance, forbids timeout overrides inside callbacks, and adjusts deadline/abort behavior. |
| lib/mongo/server/connection_pool.rb | Ensures CSOT deadline expiration during checkout maps to CSOT TimeoutError semantics. |
| lib/mongo/operation/get_more/op_msg.rb | Caps getMore maxTimeMS by remaining timeout when maxAwaitTimeMS is larger. |
| lib/mongo/cursor.rb | Adds per-iteration CSOT refresh for tailable awaitData cursors and preserves cumulative deadline across getMores. |
| lib/mongo/cursor_host.rb | Adjusts maxAwaitTimeMS vs timeoutMS validation logic. |
| lib/mongo/csot_timeout_holder.rb | Uses a stronger “inside withTransaction” guard and raises InvalidTransactionOperation for overrides. |
| lib/mongo/collection/view/change_stream.rb | Delegates timeout refresh to underlying cursor for change streams. |
| lib/mongo/client.rb | Adds timeout_ms to CRUD option set. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| blockTimeMS: 15 | ||
| # Create the index first so dropIndex doesn't return IndexNotFound on | ||
| # servers prior to 8.3. | ||
| - name: createIndex |
There was a problem hiding this comment.
The operation name has a trailing space (createIndex ), which will not match the unified runner operation dispatch (it will be underscored to a different method name). Remove the trailing whitespace so the operation is createIndex.
| - name: createIndex | |
| - name: createIndex |
| blockTimeMS: 15 | ||
| # Create the index first so dropIndex doesn't return IndexNotFound on | ||
| # servers prior to 8.3. | ||
| - name: createIndex |
There was a problem hiding this comment.
The operation name has a trailing space (createIndex ), which will prevent the unified runner from recognizing it as createIndex. Remove the trailing whitespace.
| - name: createIndex | |
| - name: createIndex |
| blockTimeMS: 15 | ||
| # Create the index first so dropIndex doesn't return IndexNotFound on | ||
| # servers prior to 8.3. | ||
| - name: createIndex |
There was a problem hiding this comment.
The operation name has a trailing space (createIndex ), which will break operation dispatch in the unified runner. Remove the trailing whitespace.
| - name: createIndex | |
| - name: createIndex |
| blockTimeMS: 15 | ||
| # Create the index first so dropIndex doesn't return IndexNotFound on | ||
| # servers prior to 8.3. | ||
| - name: createIndex |
There was a problem hiding this comment.
The operation name has a trailing space (createIndex ), which will break operation dispatch in the unified runner. Remove the trailing whitespace.
| - name: createIndex | |
| - name: createIndex |
| blockTimeMS: 15 | ||
| # Create the index first so dropIndex doesn't return IndexNotFound on | ||
| # servers prior to 8.3. | ||
| - name: createIndex |
There was a problem hiding this comment.
The operation name has a trailing space (createIndex ), which will break operation dispatch in the unified runner. Remove the trailing whitespace.
| - name: createIndex | |
| - name: createIndex |
| blockTimeMS: 5 | ||
| # Create the index first so dropIndex doesn't return IndexNotFound on | ||
| # servers prior to 8.3. | ||
| - name: createIndex |
There was a problem hiding this comment.
The operation name has a trailing space (createIndex ), which will break operation dispatch in the unified runner. Remove the trailing whitespace.
| - name: createIndex | |
| - name: createIndex |
| client: *client | ||
| # Create the index first so dropIndex doesn't return IndexNotFound on | ||
| # servers prior to 8.3. | ||
| - name: createIndex |
There was a problem hiding this comment.
The operation name has a trailing space (createIndex ), which will break operation dispatch in the unified runner. Remove the trailing whitespace.
| - name: createIndex | |
| - name: createIndex |
| blockTimeMS: 5 | ||
| # Create the index first so dropIndex doesn't return IndexNotFound on | ||
| # servers prior to 8.3. | ||
| - name: createIndex |
There was a problem hiding this comment.
The operation name has a trailing space (createIndex ), which will break operation dispatch in the unified runner. Remove the trailing whitespace.
| - name: createIndex | |
| - name: createIndex |
| object.refresh_timeout! if object.respond_to?(:refresh_timeout!) | ||
| loop do | ||
| doc = object.try_next | ||
| return doc if doc |
There was a problem hiding this comment.
This loop can spin forever if try_next returns nil without raising (e.g., a drained non-tailable cursor). Consider returning nil when the cursor/stream is exhausted (e.g., fully_iterated?/closed?) or adding a bounded wait/backoff to avoid hanging unified tests.
| return doc if doc | |
| return doc if doc | |
| # Avoid spinning forever on exhausted/closed cursors or streams. | |
| if object.respond_to?(:closed?) && object.closed? | |
| return nil | |
| end | |
| if object.respond_to?(:fully_iterated?) && object.fully_iterated? | |
| return nil | |
| end |
| rescue Exception => e | ||
| if within_states?(STARTING_TRANSACTION_STATE, TRANSACTION_IN_PROGRESS_STATE) | ||
| log_warn("Aborting transaction due to #{e.class}: #{e}") | ||
| @with_transaction_deadline = nil | ||
| # CSOT: if the deadline is already expired, clear it so that | ||
| # abort_transaction uses a fresh timeout (not the expired deadline). | ||
| # If the deadline is not yet expired, keep it so abort uses remaining time. | ||
| @with_transaction_deadline = nil if @with_transaction_deadline && deadline_expired?(deadline) | ||
| abort_transaction |
There was a problem hiding this comment.
In the withTransaction rescue path, if the CSOT deadline has expired after the callback error, the method later re-raises the original exception instead of a TimeoutError. This can surface a non-timeout error despite the deadline being exceeded; consider converting to TimeoutError when the deadline is expired (and optionally retaining the original exception as context).
No description provided.