Skip to content

fix: test_successful_wait_for_connection test: make HashableMock thread-safe to fix flaky Windows CI - #766

Draft
mykaul wants to merge 1 commit into
scylladb:masterfrom
mykaul:fix/hashable-mock-thread-safety
Draft

fix: test_successful_wait_for_connection test: make HashableMock thread-safe to fix flaky Windows CI#766
mykaul wants to merge 1 commit into
scylladb:masterfrom
mykaul:fix/hashable-mock-thread-safety

Conversation

@mykaul

Copy link
Copy Markdown

Summary

Fixes flaky test_successful_wait_for_connection failure on Windows CI:

TypeError: __hash__ method should return an integer

at cassandra/pool.py:582 (connection in self._trash).

Root cause

HashableMock subclasses NonCallableMagicMock and overrides __hash__ to return id(self). However, MagicMixin.__init__ replaces __hash__on the type with a MagicMock object before any test runs — making the original override dead code that never executes.

The MagicMock standing in for __hash__ happens to return a hash-compatible value in single-threaded use, but MagicMock.__call__ is not thread-safe. In test_successful_wait_for_connection, two threads call pool.return_connection() on the same HashableMock concurrently, which triggers hash() via connection in self._trash (a set membership check). Under Windows thread scheduling, the concurrent MagicMock.__call__ can return a non-integer, causing the TypeError.

Fix

Restore a plain function as the class-level __hash__ in HashableMock.__init__, after MagicMixin has finished its work. This ensures hash() always resolves to a real, thread-safe function returning id(self).

Verification

  • All 10 pool tests pass
  • test_successful_wait_for_connection passes 100/100 runs in a loop

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

@mykaul
mykaul marked this pull request as draft March 26, 2026 10:12
@mykaulmykaul changed the title fix: make HashableMock thread-safe to fix flaky Windows CIfix: test_successful_wait_for_connection test: make HashableMock thread-safe to fix flaky Windows CIMar 26, 2026
…ixin
NonCallableMagicMock.__init__ (via MagicMixin) replaces __hash__ on the
type with a MagicMock object. That MagicMock is not thread-safe, so
concurrent hash() calls — e.g. `connection in self._trash` in pool.py
return_connection — can raise `TypeError: __hash__ method should return
an integer` under concurrent access on Windows.
The previous __hash__ override was dead code: MagicMixin.__init__ always
replaced it with a MagicMock before any test could call it.
Fix by restoring a plain function as the class-level __hash__ after
super().__init__ runs, so hash() always resolves to a real function
instead of a thread-unsafe MagicMock callable.
Fixes flaky test_successful_wait_for_connection on Windows CI.
CopilotAI review requested due to automatic review settings July 29, 2026 20:21
@mykaul
mykaulforce-pushed the fix/hashable-mock-thread-safety branch from 5677394 to f67df54CompareJuly 29, 2026 20:21
@coderabbitai

Copy link
Copy Markdown

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4175062a-c8db-43a6-85a7-a7c4f13f06d5

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@mykaul

Copy link
Copy Markdown
Author

Rebased onto latest `master` and re-verified the thread-safety fix.

Root-cause mechanics confirmed:NonCallableMock.__new__ gives every mock instance its own private, dynamically-created subclass (this is intentional upstream behavior in cpython's unittest/mock.py, specifically so per-instance magic-method patching doesn't leak across mocks). MagicMixin.__init__ installs a MagicProxy for __hash__ on that per-instance subclass, shadowing the old HashableMock.__hash__ override (making it dead code) with a non-thread-safe callable. The fix's type(self).__hash__ = HashableMock._id_hash runs after super().__init__(), on that same per-instance subclass, replacing the proxy with a plain function before any other thread can observe the instance. Because the subclass is unique per mock instance, this can't race with another HashableMock's own __init__, and once __init__ returns, __hash__ is a plain, side-effect-free function for the rest of the instance's life — this is a real fix, not just a narrowed window. Expanded the docstring in tests/unit/util.py to spell this out.

Verification performed:

  • Confirmed the Windows CI job (Build wheels for windows on windows-2022, part of "Test wheels building") does run pytest tests/unit per pyproject.toml's [tool.cibuildwheel.windows]test-command — so it does exercise this exact test, and it currently passes on this branch.
  • Reproduced the original race locally: temporarily reverted just tests/unit/util.py to the pre-fix __hash__ override and ran test_successful_wait_for_connection in a tight in-process loop — hit the exact same TypeError: cannot use 'unittest.mock.HashableMock' as a set element (__hash__ method should return an integer) at pool.py:574 (1 failure / 3000 iterations).
  • Restored the fix and reran the identical 3000-iteration loop: 0 failures.
  • Additionally stress-tested 16 threads calling hash() on a single shared HashableMock concurrently (20k calls/thread): 0 errors.
  • Full tests/unit/ suite: 720 passed, 88 skipped (unrelated, pre-existing), 0 failed.
  • All PR CI checks (wheel builds incl. windows-2022, sdist, asyncio/libev/asyncore matrix) currently green.

No unresolved review threads found. Amended into the existing commit and force-pushed (kept as draft).

CopilotAI 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.

Pull request overview

Fixes flaky concurrent hashing of HashableMock in Windows pool tests.

Changes:

  • Restores a thread-safe, identity-based __hash__ after mock initialization.
  • Documents why per-instance type patching is safe.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

mykaul added a commit to mykaul/python-driver that referenced this pull request Jul 30, 2026
Also ports the HashableMock thread-safety fix from PR scylladb#766
(fix/hashable-mock-thread-safety) to unblock this PR's own CI: the
"Build wheels for macos-arm on macos-14" job was failing on
tests/unit/test_host_connection_pool.py::HostConnectionTests::test_successful_wait_for_connection
with `TypeError: __hash__ method should return an integer not
'MagicMock'`. This is a pre-existing, unrelated flaky-test bug (not
introduced by this Python-2-cleanup PR); PR scylladb#766 is the canonical fix
and should still land separately.
MagicMixin.__init__ replaces __hash__ on the mock's type with a
MagicMock object, which is not thread-safe under concurrent hash()
calls (e.g. `connection in self._trash` in pool.py). Fix by restoring
a plain, id-based __hash__ function on the class after
super().__init__() runs.
mykaul added a commit to mykaul/python-driver that referenced this pull request Jul 30, 2026
Also ports the HashableMock thread-safety fix from PR scylladb#766
(fix/hashable-mock-thread-safety) to unblock this PR's own CI: the
"Build wheels for macos-arm on macos-14" job was failing on
tests/unit/test_host_connection_pool.py::HostConnectionTests::test_successful_wait_for_connection
with `TypeError: __hash__ method should return an integer not
'MagicMock'`. This is a pre-existing, unrelated flaky-test bug (not
introduced by this Python-2-cleanup PR); PR scylladb#766 is the canonical fix
and should still land separately.
MagicMixin.__init__ replaces __hash__ on the mock's type with a
MagicMock object, which is not thread-safe under concurrent hash()
calls (e.g. `connection in self._trash` in pool.py). Fix by restoring
a plain, id-based __hash__ function on the class after
super().__init__() runs.
mykaul added a commit to mykaul/python-driver that referenced this pull request Jul 30, 2026
Also ports the HashableMock thread-safety fix from PR scylladb#766
(fix/hashable-mock-thread-safety) to unblock this PR's own CI: the
"Build wheels for macos-arm on macos-14" job was failing on
tests/unit/test_host_connection_pool.py::HostConnectionTests::test_successful_wait_for_connection
with `TypeError: __hash__ method should return an integer not
'MagicMock'`. This is a pre-existing, unrelated flaky-test bug (not
introduced by this Python-2-cleanup PR); PR scylladb#766 is the canonical fix
and should still land separately.
MagicMixin.__init__ replaces __hash__ on the mock's type with a
MagicMock object, which is not thread-safe under concurrent hash()
calls (e.g. `connection in self._trash` in pool.py). Fix by restoring
a plain, id-based __hash__ function on the class after
super().__init__() runs.
Sign up for freeto 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

@mykaul