Skip to content

PYTHON-6187 Publish CommandFailedEvent for cancelled commands - #3226

Open
aclark4life wants to merge 2 commits into
mongodb:mainfrom
aclark4life:PYTHON-6187
Open

aclark4life wants to merge 2 commits into
mongodb:mainfrom
aclark4life:PYTHON-6187

Conversation

@aclark4life

Copy link
Copy Markdown
Contributor

PYTHON-6187

Changes in this PR

  • _run_command in both the async and sync command runners now catches BaseException instead of Exception, so a cancellation (asyncio.CancelledError) and, in the sync driver, KeyboardInterrupt/SystemExit publish the CommandFailedEvent and "Command failed" log message before the exception propagates, as the Command Logging and Monitoring spec guarantees.
  • Exceptions that stringify to "" (CancelledError, KeyboardInterrupt, SystemExit) get an errmsg of operation interrupted by <ClassName> instead of an empty message.
  • _convert_exception now annotates its parameter as BaseException.
  • Changelog entry for 4.21.0.

Test Plan

  • New regression test in test/asynchronous/test_async_cancellation.py: cancel a blocked find (failCommand blockConnection) and assert a correlating CommandFailedEvent with errtype == "CancelledError". Verified it fails without the fix and passes with it.
  • test_monitoring.py, test_command_monitoring.py, test_command_logging.py, and test_async_cancellation.py pass for both async and sync suites (167 passed).
  • just typing passes.

Checklist

Checklist for Author

  • Did you update the changelog (if necessary)?
  • Is the intention of the code captured in relevant tests?
  • If there are new TODOs, has a related JIRA ticket been created?

Checklist for Reviewer

  • Does the title of the PR reference a JIRA Ticket?
  • Do you fully understand the implementation? (Would you be comfortable explaining how this code works to someone else?)
  • Have you checked for spelling & grammar errors?
  • Is all relevant documentation (README or docstring) updated?

@aclark4life
aclark4life marked this pull request as ready for review November 13, 2026 23:27
@aclark4life
aclark4life requested a review from a team as a code owner November 13, 2026 23:27
@aclark4life
aclark4life requested review from sleepyStick and a balanced review from Copilot November 13, 2026 23:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The cancellation test relies on a 100 ms sleep rather than confirming that find has started, so it can fail spuriously and needs bounded event synchronization with guaranteed task cleanup.

1 open issue
What changed in this PR

This PR fixes command monitoring for interrupted operations so cancellation publishes a corresponding failure event and log message before the exception propagates.

Changes:

  • Catch BaseException in command runners and provide fallback messages for empty exception text.
  • Add an async cancellation regression test checking correlated command events.
  • Document the fix in the 4.21.0 changelog.
File Description
test/​asynchronous/​test_async_cancellation.py Adds cancellation-event regression coverage.
pymongo/​synchronous/​command_runner.py Mirrors interruption handling in the generated sync runner.
pymongo/​message.py Broadens the exception-conversion annotation to BaseException.
pymongo/​asynchronous/​command_runner.py Publishes failure telemetry for cancelled commands.
doc/​changelog.rst Records the cancellation-monitoring fix.

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No active review issues block approval of the command-monitoring and cancellation changes.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants