RUBY-3909 Close monitoring connection when server marked Unknown from network error#3076
Open
comandeo-mongo wants to merge 5 commits into
Open
RUBY-3909 Close monitoring connection when server marked Unknown from network error#3076comandeo-mongo wants to merge 5 commits into
comandeo-mongo wants to merge 5 commits into
Conversation
… network error The Server Monitoring spec requires that when a server is marked Unknown from a network error while reading or writing, the driver cancel the in-progress hello check and close the current monitoring connection, so the next check runs over a freshly established connection. Previously Server#unknown! only stopped the streaming PushMonitor. The polling Monitor's connection (which also serves RTT in streaming mode) was reused on recovery, letting a server be re-validated over a stale socket instead of proving a fresh connection can be established. Add Monitor#cancel_check!, which stops the PushMonitor and closes the polling connection under a dedicated lock (following the spec's cancelCheck pseudocode). Server#unknown! calls it via a new :network_error option, distinguishing network errors (connection suspect, cancel the check) from non-network connection errors such as auth failures (only the PushMonitor is torn down, as before).
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Ruby MongoDB driver’s SDAM monitoring behavior to comply with the Server Monitoring spec requirement to cancel an in-progress hello check and close the monitoring connection when a server is marked Unknown due to an application network error, preventing recovery over a potentially stale monitoring socket.
Changes:
- Add
Mongo::Server::Monitor#cancel_check!to stop the PushMonitor and close/clear the polling monitor connection. - Restructure
Monitor#checkto operate on a snapshot of the monitoring connection and to safely clear/store it under a new mutex. - Extend
Server#unknown!with a:network_erroroption and plumb that option from connection/connection-pool error paths; add specs covering these cases.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/mongo/server/monitor.rb | Adds cancel_check!, introduces @connection_lock, and restructures check/connection management for cancelability. |
| lib/mongo/server.rb | Adds :network_error handling to unknown! and updates callers/docstrings accordingly. |
| lib/mongo/server/connection.rb | Marks servers unknown with network_error: true on socket errors so monitoring checks are cancelled. |
| lib/mongo/server/connection_pool.rb | Determines when connection-establishment errors are network errors and passes network_error: into unknown!. |
| spec/mongo/server/monitor_spec.rb | Adds unit coverage for #cancel_check! and reconnect behavior in polling mode. |
| spec/mongo/server_spec.rb | Adds unit coverage for Server#unknown! branching between network_error and stop_push_monitor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The public connection reader and rtt_measurement_only? still touched @connection without the lock, so a concurrent cancel_check! could leave callers observing a stale reference on runtimes without the GVL. Route both reads through the lock.
jamis
approved these changes
Jul 10, 2026
| # may nil @connection from another thread; working on a local copy keeps | ||
| # this check consistent, and the guarded writeback below never clobbers | ||
| # a connection the monitor thread did not itself establish. | ||
| connection = @connection_lock.synchronize { @connection } |
Contributor
There was a problem hiding this comment.
Should this use the new #connection method? It seems like it duplicates the logic there.
Suggested change
| connection = @connection_lock.synchronize { @connection } | |
| connection = self.connection |
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
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.
Description
The Server Monitoring spec ("hello or legacy hello Cancellation") requires:
The Ruby driver only implemented this partially.
Server#unknown!calledmonitor.stop_push_monitor!, which closes the streaming (PushMonitor) connection, but never touched the pollingMonitor's own connection. That connection is reused on recovery (Monitor#check), and in streaming mode it also serves RTT measurement, so it survived the failure. As a result a degraded node that resets application sockets and refuses new connections while its old monitor socket stays healthy (the spec's Azure idle-close example; a failed-disk node is another) could be re-validated over the pre-failure socket and marked available again, so operations kept being routed to a node that could not serve them. This is one of the suspected contributing factors behind RUBY-3890.In polling mode (
server_monitoring_mode: :poll, or:autoon FaaS) there is no PushMonitor, so nothing was closed at all.Change
Monitor#cancel_check!: stops the PushMonitor (interrupting its awaited hello read) and closes the polling connection, so the next check must establish a fresh one. The connection is copied under a dedicated@connection_lockand disconnected outside the lock, following the spec'scancelCheckpseudocode;checkwas restructured to snapshot the connection under the lock and write it back with an identity guard, so a concurrent cancel cannot leave a stale socket installed or raise.Server#unknown!distinguishes a new:network_erroroption (cancel the check and close the connection) from:stop_push_monitor(non-network connection error, e.g. an auth failure: only tear down the PushMonitor, as before). This matches the spec's cancel trigger (isNetworkError(error) or not error.completedHandshake) and the pymongo reference, which does not cancel the monitor check on authentication failures.Files
lib/mongo/server/monitor.rb—cancel_check!, connection-lock-guardedcheck,store_connection/clear_connection.lib/mongo/server.rb—unknown!gates on:network_errorvs:stop_push_monitor.lib/mongo/server/connection.rb,lib/mongo/server/connection_pool.rb— passnetwork_error:only for genuine network errors.Test plan
spec/mongo/server/monitor_spec.rb(#cancel_check!closes and clears the connection; a live polling check then establishes a fresh connection) andspec/mongo/server_spec.rb(#unknown!cancels the check on a network error, only stops the PushMonitor otherwise). These cover connection identity, which the unified format cannot express.cancel-server-checkunified SDAM test still passes.spec/mongo/server/monitor_spec.rb,spec/mongo/server_spec.rb,spec/mongo/server/connection_spec.rb,spec/mongo/server/push_monitor_spec.rbspec/spec_tests/sdam_unified_spec.rb,spec/integration/server_monitor_spec.rbJira: https://jira.mongodb.org/browse/RUBY-3909