Skip to content

PYTHON-6040 Preserve version-to-name mapping in Client Metadata - #29

Closed
blink1073 wants to merge 9 commits into
mainfrom
PYTHON-6040
Closed

blink1073 wants to merge 9 commits into
mainfrom
PYTHON-6040

Conversation

@blink1073

@blink1073 blink1073 commented Sep 15, 2026

Copy link
Copy Markdown
Owner

PYTHON-6040

Implements DRIVERS-3251: Ensure driver.name and driver.version require positional alignment in the handshake specification.

Changes in this PR

  • Reject | in DriverInfo fields at construction.
  • Append empty version entries for the |c and |async name suffixes.
  • Always append the delimiter to name and version in _update_metadata.
  • Deduplicate appended drivers by whole DriverInfo object.
  • Keep name/version index-aligned in _truncate_metadata at the 512-byte limit.
  • Add the new handshake prose tests.
  • Update the metadata, dedup, and handshake tests.

Test Plan

  • New prose test passes.

Checklist

Checklist for Author

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

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?

@blink1073
blink1073 requested a balanced review from Copilot September 15, 2026 00:56
Comment thread pymongo/pool_options.py Outdated
Comment thread pymongo/pool_options.py Outdated
Comment thread test/asynchronous/test_client_metadata.py

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Successive appends can exceed the handshake limit, and concurrent updates can permanently lose metadata.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Preserves one-to-one correspondence between client metadata driver names and versions.

Changes:

  • Rejects reserved delimiters and aligns appended metadata fields.
  • Updates deduplication and truncation behavior.
  • Expands synchronous and asynchronous handshake tests.
File summaries
File Description
pymongo/driver_info.py Validates reserved delimiters.
pymongo/pool_options.py Aligns, deduplicates, and truncates metadata.
test/test_client.py Tests synchronous metadata alignment.
test/test_client_metadata.py Tests synchronous append behavior.
test/asynchronous/test_client.py Tests asynchronous metadata alignment.
test/asynchronous/test_client_metadata.py Tests asynchronous append behavior.
test/mockupdb/test_handshake.py Updates handshake expectations.
Review details
  • Files reviewed: 7/7 changed files
  • Comments generated: 3
  • Review effort level: Balanced

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

Comment thread pymongo/pool_options.py Outdated
Comment thread pymongo/pool_options.py Outdated
Comment thread pymongo/driver_info.py
Trim the _truncate_metadata comments and number/label the handshake
prose tests (backpressure no. 9, delimiter no. 10, index no. 11).
Reapply the 512-byte handshake limit after append_metadata, guard the
check/update/record sequence with a lock for thread-safe clients, and
document the reserved '|' delimiter on DriverInfo.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The raw metadata lock can remain permanently locked after a fork, causing append_metadata() to deadlock.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

pymongo/pool_options.py:363

  • Storing every historical DriverInfo in a list makes duplicate detection linear per call and a sequence of unique appends quadratic overall, even after metadata has hit its 512-byte cap. DriverInfo is hashable, so this should be a set with add() to keep membership checks constant-time.
    test/asynchronous/test_client_metadata.py:270
  • This case does not test equal versions: it appends only one driver with no version, duplicating the preceding trailing-delimiter case. A regression that incorrectly deduplicates distinct drivers by version would still pass; use two distinct names with the same non-empty version.
  • Files reviewed: 7/7 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread pymongo/pool_options.py Outdated
Use _create_lock() so the metadata lock is registered with
pymongo.lock and reset after a fork, avoiding a deadlock in the child
process.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Metadata updates can raise KeyError after truncation, and the new truncation order can unnecessarily discard driver identity.

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

pymongo/pool_options.py:255

  • The loop now truncates driver.name before touching driver.version, reversing the existing metadata truncation priority. With a short wrapper name and an oversized wrapper version, the name collapses to PyMongo and rebuilding then discards that wrapper's version entirely, even though truncating only the version content would fit and preserve the driver identity. Trim version-segment content first while retaining its delimiters, and only truncate names (dropping paired version segments as needed) if the document is still oversized.

This issue also appears on line 418 of the same file.
test/asynchronous/test_client_metadata.py:270

  • This case duplicates the preceding trailing-empty-version scenario, so it does not verify that equal non-empty versions remain as separate aligned entries. Use two distinct names sharing one version to exercise the stated regression.

pymongo/pool_options.py:419

  • After truncation removes the optional platform field, a later append_metadata(DriverInfo(..., platform="...")) raises KeyError here. This occurs, for example, after enough name/version-only appends exceed 512 bytes: _truncate_metadata pops platform, but subsequent updates still index it directly. Recreate the field when it is absent before truncating again.
            if driver.platform:
                metadata["platform"] = "{}|{}".format(metadata["platform"], driver.platform)
  • Files reviewed: 7/7 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Trim wrapper version content before dropping name/version segments so
driver identity is preserved, and recreate the platform field when a
platform append follows truncation that removed it.
Comment thread pymongo/pool_options.py Outdated
Comment thread test/asynchronous/test_client_metadata.py Outdated
Revert the 'Equal versions do not collapse' prose test case to the
specification and shorten the truncation comment.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Moderate finding (3 votes): __appended_drivers retains truncated unique drivers indefinitely, allowing unbounded memory growth and slower membership checks.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

pymongo/pool_options.py:413

  • __appended_drivers retains every distinct DriverInfo forever, including entries that _truncate_metadata has already removed to stay within the 512-byte metadata limit. A caller can repeatedly append unique (and arbitrarily large) values, so memory grows without bound and the linear membership check becomes progressively slower even though the serialized metadata is bounded. Please use a bounded retention strategy for drivers that are no longer representable, or otherwise avoid recording truncated entries.
  • Files reviewed: 7/7 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Only record drivers that remain representable in the truncated metadata,
so __appended_drivers cannot grow without bound and the dedup membership
check stays fast. Add a regression test.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Truncated-away platform-only drivers can still accumulate in the deduplication set without appearing in published metadata.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 7/7 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread pymongo/pool_options.py Outdated
Use the name delimiter count before/after the update to decide whether an
appended pair survived truncation, instead of a name/version branch that
always recorded platform-only (empty name/version) drivers.
@blink1073
blink1073 requested a lite review from Copilot September 15, 2026 11:26

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

No unresolved review issues were identified.

Review details
  • Files reviewed: 7/7 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@blink1073 blink1073 closed this Sep 15, 2026
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