Skip to content

PYTHON-6136 Make pool checkout accounting transactional - #3089

Merged
blink1073 merged 13 commits into
mongodb:mainfrom
blink1073:PYTHON-6136
Oct 2, 2026
Merged

blink1073 merged 13 commits into
mongodb:mainfrom
blink1073:PYTHON-6136

Conversation

@blink1073

@blink1073 blink1073 commented Oct 2, 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.
Every failed checkout now restores its counters through one
accounted-guarded replay, replacing the per-mutation flags. Behavior,
lock regions, and telemetry ordering are unchanged.

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

Pending-slot rollback fails to wake maxConnecting waiters, allowing the pool to remain blocked.

Review effort: Balanced
Findings: 1 High severity

Open (1)
What changed in this PR

Makes pool checkout accounting transactional so interrupted checkouts restore all counters.

Changes:

  • Tracks applied accounting with rollback flags.
  • Adds deterministic interruption regression tests.
  • Strengthens gevent race coverage and counter assertions.
File Description
pymongo/​asynchronous/​pool.py Implements transactional checkout rollback.
pymongo/​synchronous/​pool.py Generated synchronous equivalent.
test/​asynchronous/​test_pooling.py Adds async accounting regression tests.
test/​test_pooling.py Generated synchronous tests.
test/​asynchronous/​test_client.py Expands gevent race coverage.
test/​test_client.py Generated synchronous race test.

💡 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 2, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.35484% with 7 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
pymongo/synchronous/pool.py 93.54% 2 Missing and 2 partials ⚠️
pymongo/asynchronous/pool.py 95.16% 1 Missing and 2 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

🟡 Changes recommended

An interrupted notification can restore counters without waking blocked waiters, preserving a deadlock path.

Review effort: Balanced
Findings: 1 High severity · 1 Low severity

Open (2)
Resolved since last review (1)

Comment thread pymongo/asynchronous/pool.py
Comment thread pymongo/synchronous/pool.py Outdated
…ling

Track notification completion separately from counter restoration so a
kill landing inside notify() retries the wake-up instead of stranding
waiters. Reword comments containing 'waiter', which the synchro
replacement table mangles ('aiter' is a substring).

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 interrupted max-connecting notification can still leave an existing checkout blocked indefinitely.

Review effort: Balanced
Findings: 2 High severity

Open (2)
Resolved since last review (2)

Comment thread pymongo/asynchronous/pool.py Outdated
Comment thread pymongo/synchronous/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

Make maxConnecting notification cleanup retryable after interruption in both pool implementations.

Review effort: Lite
Findings: None

Resolved since last review (2)

A kill landing inside the connect cleanup's notify() (a gevent yield
point) could strand a checkout waiting at the maxConnecting gate. Retry
the notify with the accounted idiom so the wake-up survives.

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

Critical rollback-safety issues and missing operation_count regression assertions remain.

Review effort: Lite
Findings: 2 High severity · 1 Low severity

Open (3)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Assert operation_count drains in stress test

test/​asynchronous/​test_client.py:2873

The stress test now verifies that requests, _pending, and active_sockets drain, but it never checks operation_count, which is the counter this PR also makes transactional. A checkout can therefore leak the load metric while this regression test still passes; add an operation_count == 0 assertion to the post-settle checks.

Medium severity Assert operation_count drains in stress test

test/​test_client.py:2824

The stress test now verifies that requests, _pending, and active_sockets drain, but it never checks operation_count, which is the counter this PR also makes transactional. A checkout can therefore leak the load metric while this regression test still passes; add an operation_count == 0 assertion to the post-settle checks.

Comment thread pymongo/asynchronous/pool.py
Comment thread pymongo/synchronous/pool.py
Comment thread pymongo/synchronous/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

Interruption-sensitive accounting changes across sync and async pools warrant final human validation.

Review effort: Lite
Findings: None

Resolved since last review (3)

@blink1073
blink1073 marked this pull request as ready for review October 2, 2026 11:39
@blink1073
blink1073 requested a review from a team as a code owner October 2, 2026 11:40
@blink1073
blink1073 requested a review from aclark4life October 2, 2026 11:40
self._max_connecting_cond.notify()
notified |= _UNDO_PENDING
finally:
async with self.size_cond:

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.

Looks like this locks whether it needs to or not, how about something like this instead:

      finally:
          # Both conditions below read only local variables, so the lock
          # is not needed to evaluate them; skip the acquire when there
          # is no leftover work.
          if not accounted or applied & ~notified:
              async with self.size_cond:
                  if not accounted:
                      self._restore_applied(applied)
                  missing = applied & ~notified
                  if missing & _UNDO_REQUESTS:
                      self.size_cond.notify()
                  if missing & _UNDO_PENDING:
                      self._max_connecting_cond.notify()

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I verified that the current structure is correct and added a comment.

@aclark4life aclark4life 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.

LGTM

@blink1073
blink1073 merged commit 07a7b8e into mongodb:main Oct 2, 2026
91 of 93 checks passed
@blink1073
blink1073 deleted the PYTHON-6136 branch October 2, 2026 21:07
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.

3 participants