Skip to content

PYTHON-6136 Fix pool checkout accounting rollback on BaseException - #3088

Closed
blink1073 wants to merge 6 commits into
mongodb:mainfrom
blink1073:PYTHON-6136
Closed

blink1073 wants to merge 6 commits into
mongodb:mainfrom
blink1073:PYTHON-6136

Conversation

@blink1073

@blink1073 blink1073 commented Oct 1, 2026 •

Copy link
Copy Markdown
Member

PYTHON-6136

Changes in this PR

An interrupted checkout could permanently stick the pool; the pool's accounting is now fully restored on failure.

Test Plan

Covered by the gevent killall race test and new deterministic regression tests. Repro: AMPLIFY_RACE=1 python -m pytest test/test_client.py::TestExhaustCursor::test_gevent_kill_churn_deadlock

Checklist

Checklist for Author

  • Did you update the changelog (if necessary)? (Not necessary — internal accounting fix.)
  • Is there test coverage?
  • Is any followup work tracked in a JIRA ticket? If so, add link(s). (None needed.)

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?)
  • Is all relevant documentation (README or docstring) updated?

A BaseException (KeyboardInterrupt, CancelledError, GreenletExit)
landing inside Pool.checkout could leak checkout accounting: the size
gate (self.requests) was only rolled back after a socket was acquired,
and the maxConnecting _pending counter was only rolled back on the
happy path. Roll back both counters, with notify(), when a
BaseException interrupts the gate or the connection attempt. Rework
the gevent killall race test to amplify the checkout unwind windows
and assert the counters fully drain after the pool settles.

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.

Copilot review overview

🟡 Changes recommended

An interruption during pending-gate cleanup can bypass the subsequent size-gate rollback.

Review effort: Balanced
Findings: 1 High severity

Open (1)
What changed in this PR

Fixes pool checkout accounting leaks when BaseException interrupts gate transitions.

Changes:

  • Rolls back requests and _pending after interrupted checkouts.
  • Strengthens the gevent race regression test and verifies counters drain.
File Description
pymongo/​asynchronous/​pool.py Adds source accounting rollback logic.
pymongo/​synchronous/​pool.py Generated synchronous counterpart.
test/​asynchronous/​test_client.py Expands the gevent race test.
test/​test_client.py Generated synchronous test counterpart.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread pymongo/asynchronous/pool.py Outdated
@codecov

codecov Bot commented Oct 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 38.46154% with 48 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
pymongo/asynchronous/pool.py 38.46% 21 Missing and 3 partials ⚠️
pymongo/synchronous/pool.py 38.46% 21 Missing and 3 partials ⚠️

📢 Thoughts on this report? Let us know!

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.

Copilot review overview

🔵 Needs a closer look

Fix the potentially unbound pool test variable and correct or add the test named in the documented test plan.

Review effort: Lite
Findings: None

Resolved since last review (1)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Initialize pool before try/finally to preserve watchdog failures

test/​asynchronous/​test_client.py:2853

pool is initialized only after the watchdog and liveness probe succeed. If either path raises (for example, the intended deadlock timeout), control still runs the finally block and then reaches the assertions at 2872-2874, where pool is unbound and the test reports UnboundLocalError instead of the actual failure. Initialize the pool before entering the try/finally and remove this late assignment.

Low severity Test plan references missing regression test

test/​asynchronous/​test_client.py:2749

The test plan names test/asynchronous/test_client.py::TestExhaustCursor::test_exhaust_cursor_enabled_killall, but this change only modifies test_gevent_kill_churn_deadlock, and no test with the documented name exists in the checkout. That command therefore collects no regression test; please update the plan or add/rename the intended test.

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.

Copilot review overview

🔵 Needs a closer look

Unresolved moderate issues remain in checkout counter rollback and asynchronous connection cleanup.

Review effort: Lite
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Medium severity Rollback operation_count on every failed checkout

pymongo/​asynchronous/​pool.py:987

operation_count is incremented before this rollback block and is only decremented by _checkin_apply, which is reached only after a connection has been returned. A cancellation or other BaseException before that point therefore leaves operation_count permanently elevated (including failures while waiting on size_cond); subsequent checkouts keep the server-selection load metric inflated. Roll back this counter exactly once on every failed checkout and assert it in the new regression tests as well.

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.

Copilot review overview

🟡 Changes recommended

Protect the asynchronous operation-count rollback with the shared lock to prevent lost updates.

Review effort: Lite
Findings: 1 Medium severity

Open (1)

Comment thread pymongo/asynchronous/pool.py Outdated

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.

Copilot review overview

🔵 Needs a closer look

Interrupted rollback can leak successfully created connections in both pool implementations.

Review effort: Lite
Findings: None

Resolved since last review (1)

@blink1073

Copy link
Copy Markdown
Member Author

Closing this PR to make one with a simpler approach.

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