Skip to content

cluster: preserve Unix control endpoint for local host - #944

Merged
dkropachev merged 1 commit into
masterfrom
dk/fix-942-maintenance-mode-close
Sep 10, 2026
Merged

cluster: preserve Unix control endpoint for local host#944
dkropachev merged 1 commit into
masterfrom
dk/fix-942-maintenance-mode-close

Conversation

@dkropachev

@dkropachev dkropachev commented Jul 28, 2026

Copy link
Copy Markdown

Fixes #942.

Root cause

Since a0cde2e stopped creating provisional Host objects for contact points, the control connection constructs the local Host from the system.local row. That row advertises the regular native TCP address even when the control connection reached the node through its Unix maintenance socket.

Topology refresh therefore discarded the reachable UnixSocketEndPoint and left the session with the unavailable TCP endpoint. The Scylla test harness could establish the control connection but could not finish its maintenance-mode readiness check.

Change

For a newly discovered local host reached through UnixSocketEndPoint, preserve connection.original_endpoint as the route while retaining the advertised TCP address and port in the Host broadcast metadata.

The implementation also preserves the existing endpoint of an already-known Host instead of mutating it between Unix and network endpoints, because Host hashes depend on the endpoint. Factory-created endpoints continue to drive duplicate detection, peer rows, regular network connections, address translation, SNI/cloud endpoints, Client Routes, and custom endpoint factories. Mixed Unix/network Host ordering is made deterministic while all non-Unix ordering behavior remains unchanged.

When a retained Unix host is later observed as a peer, schema agreement resolves it by host ID. The control connection records that same stable host ID so host lookup, error attribution, and DOWN/REMOVE reconnect callbacks remain correct even if the active connection route and Host endpoint differ. Callback matching keeps an exact recorded-ID match after metadata removal, then follows canonical connection-host lookup if that ID has gone stale.

Shard-aware pools keep the Unix route: additional connections ignore advertised shard-aware TCP ports and source-port targeting, and use the existing optimistic non-shard-aware path through the Unix listener.

Scope

This PR intentionally does not redesign general host-state transition handling. Existing concurrency and ordering problems involving ADD, UP, DOWN, REMOVE, or concurrent pool creation remain outside this PR.

It also does not change startup-close classification, general replacement recovery, shard-attempt coordination, or pool-transition sequencing. Stable host-ID lookup also affects non-Unix control routes that differ from the advertised Host endpoint, such as contact-point/broadcast-address mismatches. Failures are attributed to that Host, with direct reconnect retained when no DOWN callback is queued. Endpoint-equal TCP behavior remains unchanged, including the suppressed-DOWN problem tracked by #847. Direct heterogeneous EndPoint ordering is tracked separately by #1009; this PR only makes mixed Host ordering deterministic.

Validation

  • The exact Scylla maintenance-mode test timed out after 45 seconds on origin/master; the branch passed three repeated runs, and the final functional re-run passed in 19.02 seconds with ScyllaDB Python Driver 3.29.11.
  • TZ=UTC uv run pytest -q tests/unit — 1072 passed, 24 skipped.
  • Focused control, metadata, shard-aware, and policy tests — 211 passed.
  • uv run --with build python -m build --no-isolation — source distribution and wheel built successfully.

Copilot AI review requested due to automatic review settings July 28, 2026 02:57
@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: QUIET

Plan: Advanced

Run ID: fcb2d7db-96be-4a52-a454-7d6d49b9c13f

📥 Commits

Reviewing files that changed from the base of the PR and between 83bdd46 and 6fcb367.

📒 Files selected for processing (4)
  • cassandra/cluster.py
  • cassandra/connection.py
  • cassandra/pool.py
  • tests/unit/test_control_connection.py

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The driver adds Unix socket handling to control-connection refresh, schema queries, host identity resolution, and shard-aware connection creation. It preserves known Unix and network routes across topology changes. Tests cover maintenance endpoints, route mismatches, host-ID resolution, metadata refresh, and Unix socket shard behavior.

Sequence Diagram(s)

sequenceDiagram
  participant ControlConnection
  participant UnixSocket
  participant Metadata
  ControlConnection->>UnixSocket: connect through maintenance socket
  UnixSocket-->>ControlConnection: return local host identity
  ControlConnection->>Metadata: refresh topology by host ID
  Metadata-->>ControlConnection: preserve Unix route and update schema state
Loading

Suggested reviewers: sylwiaszunejko, mykaul

Priority: ➖ Normal

Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 6fcb3

Unix maintenance connections now retain local socket routing while advertised TCP metadata remains available. Unit, build, and maintenance-mode integration validation passed, with no identified merge-blocking risk.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 4.75% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 463 functions across 14 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the primary change: preserving the Unix control endpoint for the local host.
Description check ✅ Passed The description explains the root cause, implementation, scope, linked issue, tests, functional validation, and build results. It does not reproduce the checklist, but the required information is most…
Linked Issues check ✅ Passed The changes address issue #942 by preserving the Unix maintenance socket route, retaining advertised metadata, supporting schema and host-ID lookup, and validating maintenance-mode startup with focuse…
Out of Scope Changes check ✅ Passed The reported code and tests support Unix endpoint preservation, stable host identity, schema agreement, reconnect handling, endpoint ordering, and shard-aware pool behavior. No unrelated code changes …
  • Fix all pre-merge checks with AI

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

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.

Pull request overview

Restores pre-3.29.11 handling of clean server-side closes during connection startup.

Changes:

  • Returns cleanly closed startup connections to their owner.
  • Adds regression coverage for this behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
cassandra/connection.py Removes immediate ConnectionShutdown for clean startup closes.
tests/unit/test_connection.py Tests returning a cleanly closed connection.

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

Comment thread cassandra/connection.py
Copilot AI review requested due to automatic review settings July 28, 2026 03:27
@dkropachev
dkropachev force-pushed the dk/fix-942-maintenance-mode-close branch from dc80701 to fc09951 Compare July 28, 2026 03:27

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.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

cassandra/connection.py:990

  • Returning a closed connection is treated as a successful reconnect by existing owners. _ReconnectionHandler.run() invokes on_reconnection() and its callback for any returned value (cassandra/pool.py:281-305), and _HostReconnectionHandler then marks the host up (cassandra/pool.py:353-361); initial pool construction likewise installs the returned connection without checking is_closed (cassandra/pool.py:429-431). A clean startup close can therefore falsely mark a maintenance-mode host up and build a pool around a closed socket instead of preserving reconnection cadence. The owning paths need explicit closed-result handling (with coverage), or the factory needs a distinct failure/result contract.
        else:
            return conn

Copilot AI review requested due to automatic review settings July 28, 2026 11:08
@dkropachev
dkropachev force-pushed the dk/fix-942-maintenance-mode-close branch from fc09951 to abfcaf8 Compare July 28, 2026 11:08

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.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (4)

cassandra/connection.py:975

  • The opening sentence (“returns a connection once startup has completed”) conflicts with the documented behavior immediately below (direct callers may receive a closed connection when startup did not complete). Consider rewording the first sentence to reflect that the method may return a closed connection when the server cleanly closes during the handshake.
        A factory function which returns a connection once startup has
        completed, or raises an exception otherwise.

        Direct callers may receive a closed connection when the server accepts
        the socket and then cleanly closes it during the startup handshake.
        Callers that own pool startup or reconnection probes pass ``host_conn``
        or ``_raise_on_startup_close`` and receive :exc:`ConnectionShutdown`
        for that clean startup close instead.

cassandra/connection.py:998

  • The exception is constructed using the input endpoint rather than the connection’s actual endpoint (conn.endpoint). Using conn.endpoint (both for message formatting and for the exception’s endpoint attribute) is typically more accurate if the connection normalizes/rewrites endpoints (e.g., via an endpoint factory or translation) and also avoids the slightly confusing % (endpoint,) tuple formatting.
        elif conn.is_closed and (host_conn is not None or raise_on_startup_close):
            raise ConnectionShutdown(
                "Connection to %s was closed during the startup handshake" % (endpoint,),
                endpoint)

tests/unit/test_connection.py:480

  • server.first_frame[4] can raise IndexError if the server receives fewer than 5 bytes (e.g., if the client closes early or a timeout occurs). Adding an explicit assert len(server.first_frame) >= 5 (or parsing/validating the 9-byte header length) would make failures clearer and avoid masking the underlying cause.
            assert conn.is_closed
            assert server.received_frame.wait(2)
            assert server.error is None
            assert server.first_frame[4] == 0x05  # OPTIONS

tests/unit/test_connection.py:502

  • This test asserts on the private/internal attribute _pending_connections, which can make the test brittle to refactors of internal connection tracking. If possible, assert the externally observable behavior you care about (e.g., that the factory raises and that the returned/created connection is closed) without coupling to _pending_connections’ internal representation.
            host_conn = Mock()
            host_conn.is_shutdown = False
            host_conn._pending_connections = []

            with pytest.raises(ConnectionShutdown) as exc_info:
                MaintenanceModeConnection.factory(
                    DefaultEndPoint('127.0.0.1', server.port),
                    timeout=2,
                    host_conn=host_conn)

            assert "closed during the startup handshake" in str(exc_info.value)
            assert server.received_frame.wait(2)
            assert server.error is None
            assert server.first_frame[4] == 0x05  # OPTIONS
            assert len(host_conn._pending_connections) == 1
            assert host_conn._pending_connections[0].is_closed

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.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (4)

cassandra/connection.py:975

  • The first sentence says the factory “returns a connection once startup has completed,” but the docstring later states direct callers may receive a closed connection during the startup handshake. Please revise the opening description to reflect that the method can return a closed (non-serviceable) connection in the clean-startup-close case, so the contract is not self-contradictory.
        A factory function which returns a connection once startup has
        completed, or raises an exception otherwise.

        Direct callers may receive a closed connection when the server accepts
        the socket and then cleanly closes it during the startup handshake.
        Callers that own pool startup or reconnection probes pass ``host_conn``
        or ``_raise_on_startup_close`` and receive :exc:`ConnectionShutdown`
        for that clean startup close instead.

cassandra/connection.py:966

  • _raise_on_startup_close is currently a “hidden” control flag extracted from **kwargs, which makes the factory behavior easier to call incorrectly (typos silently change behavior) and harder to discover for implementers of custom Connection subclasses. Consider making it an explicit (preferably keyword-only) parameter of factory() (defaulting to False) and documenting it alongside host_conn.
    def factory(cls, endpoint, timeout, host_conn = None, *args, **kwargs):

cassandra/connection.py:977

  • _raise_on_startup_close is currently a “hidden” control flag extracted from **kwargs, which makes the factory behavior easier to call incorrectly (typos silently change behavior) and harder to discover for implementers of custom Connection subclasses. Consider making it an explicit (preferably keyword-only) parameter of factory() (defaulting to False) and documenting it alongside host_conn.
        raise_on_startup_close = kwargs.pop('_raise_on_startup_close', False)

tests/unit/test_connection.py:429

  • close() uses a timed join(2) on a daemon thread and does not verify the thread actually stopped. This can leak background activity across tests and introduce intermittency under slow CI. Prefer a deterministic shutdown (e.g., signal + unblock accept/recv, then join() without a timeout, or assert not self.thread.is_alive() after joining) so failures are visible and cleanup is reliable.
            def close(self):
                self._sock.close()
                self.thread.join(2)

Copilot AI review requested due to automatic review settings July 28, 2026 11:52
@dkropachev
dkropachev force-pushed the dk/fix-942-maintenance-mode-close branch from abfcaf8 to a3607f0 Compare July 28, 2026 11:52

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.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

cassandra/connection.py:998

  • This clean-close exception is still raised before reaching this branch for AsyncoreConnection: its close() assigns a generated ConnectionShutdown to last_error while startup is pending (cassandra/io/asyncorereactor.py:392-397), and factory() raises any last_error at line 987. Asyncore is the documented default when libev is unavailable (cassandra/cluster.py:953-956), so those users do not get the restored clean-close behavior described here. The synthetic connection in the new test leaves last_error unset and therefore misses this reactor-specific path; distinguish a clean-close-generated shutdown from a real startup error and cover Asyncore as well.
        elif conn.is_closed and (host_conn is not None or raise_on_startup_close):
            raise ConnectionShutdown(
                "Connection to %s was closed during the startup handshake" % (endpoint,),
                endpoint)

@dkropachev
dkropachev removed the request for review from mykaul July 28, 2026 13:02
@dkropachev
dkropachev force-pushed the dk/fix-942-maintenance-mode-close branch from a3607f0 to 19f27f0 Compare July 28, 2026 13:24
Copilot AI review requested due to automatic review settings July 28, 2026 13:24
@dkropachev
dkropachev force-pushed the dk/fix-942-maintenance-mode-close branch from 19f27f0 to a8630ad Compare July 28, 2026 13:39
Comment thread tests/integration/standard/test_maintenance_mode_connection.py Fixed

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.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (3)

cassandra/pool.py:614

  • This call still runs while _replace() holds self._lock (line 599), but shutdown() needs that same lock before it can copy and close _pending_connections (lines 628-653). Consequently, registering the replacement as pending cannot let shutdown cancel a stalled startup; shutdown remains blocked until the connection attempt completes or times out. Move the blocking factory call outside the locked region, then reacquire the lock to install it only if the pool is still active.
                    connection = self._session.cluster.connection_factory(
                        self.host.endpoint,
                        host_conn=self,
                        on_orphaned_stream_released=self.on_orphaned_stream_released)

tests/unit/test_cluster.py:282

  • This test name says a startup close is rejected, but the supplied connection has is_closed=False and the assertions verify the successful return path. Rename it to describe the open-connection case so failures and coverage are not misleading.
    def test_reconnection_factory_rejects_startup_close(self):

tests/unit/test_host_connection_pool.py:254

  • No startup close occurs here: replacement_conn.is_closed is false, and the mocked cluster factory simply returns it. The test only verifies that _replace() forwards the pool as host_conn; rename it accordingly rather than claiming closed-startup rejection coverage.
    def test_replace_tracks_pending_connection_and_rejects_startup_close(self):

Comment thread cassandra/cluster.py Outdated
Copilot AI review requested due to automatic review settings July 28, 2026 13:42
@dkropachev
dkropachev marked this pull request as ready for review July 31, 2026 00:58
@dkropachev
dkropachev marked this pull request as draft July 31, 2026 00:58
@coderabbitai
coderabbitai Bot requested review from Lorak-mmk and mykaul July 31, 2026 00:59

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.

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated no new comments.

@coderabbitai coderabbitai Bot 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.

🧹 Nitpick comments (7)
cassandra/connection.py (2)

1958-1963: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Chain the converted validation error.

Without from, the traceback loses the originating ErrorMessage.

♻️ Proposed change
         except ProtocolRequestValidationException as validation_error:
-            raise validation_error.to_exception()
+            raise validation_error.to_exception() from validation_error
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cassandra/connection.py` around lines 1958 - 1963, Update the exception
handling in the wait_for_response flow to chain the converted exception from
ProtocolRequestValidationException using the original validation_error as its
cause. Keep RequestValidationException propagation unchanged.

Source: Linters/SAST tools


1125-1145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Extract the pending-connection fallback into helpers.

The "call _register_pending_connection else poke _pending_connections under _lock else poke it unlocked" ladder is duplicated for register and unregister inside factory. Two module-level helpers (_register_pending, _unregister_pending) would keep factory's control flow readable while preserving the third-party-pool compatibility behavior.

Also applies to: 1214-1234

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cassandra/connection.py` around lines 1125 - 1145, Extract the
pending-connection compatibility ladder from factory into module-level
_register_pending and _unregister_pending helpers, covering both registration
and removal paths. Preserve the existing _register_pending_connection
preference, _lock-guarded fallback, and unlocked fallback for third-party pools,
then have factory call these helpers while retaining its shutdown and
connection-close behavior.
cassandra/pool.py (2)

778-782: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Redundant has_current_pool_fence guard in the lock list.

The not has_current_pool_fence case already returned above, so this condition is always true and only obscures the lock set.

♻️ Proposed change
-        locks = [
-            lock for lock in (cluster_lock, host_lock, session_lock)
-            if lock is not None and (
-                lock is not session_lock or has_current_pool_fence)]
+        locks = [
+            lock for lock in (cluster_lock, host_lock, session_lock)
+            if lock is not None]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cassandra/pool.py` around lines 778 - 782, Remove the redundant
has_current_pool_fence condition from the locks comprehension in the pool lock
acquisition flow, retaining only the non-None filter and existing session_lock
exclusion. Preserve the earlier return behavior for cases without a current pool
fence.

261-305: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

_transition_owner is write-only and lacks a class-level default.

Cluster._run_host_transition sets and clears host._transition_owner, but nothing reads it; and unlike the sibling transition fields it has no class attribute. Either drop it or add the default alongside the others.

♻️ Proposed change
     _transition_lock = None
     _transition_queue = None
     _transition_running = False
+    _transition_owner = None
     _transition_notification_queue = None
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cassandra/pool.py` around lines 261 - 305, Remove the unused
_transition_owner assignment and initialization from
Cluster._run_host_transition and Host.__init__, or alternatively define a
class-level default alongside the other transition fields if the state is
required; keep the transition queue behavior unchanged.
tests/unit/test_connection.py (1)

330-342: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Bind responses explicitly in the closure.

The closure is only invoked within the same iteration, so behavior is correct today, but binding the loop variable as a default silences the linter and keeps it correct if the test is later refactored.

♻️ Proposed change
-                def send_response(message, request_id, callback):
-                    callback(responses.pop(0))
+                def send_response(
+                        message, request_id, callback,
+                        responses=responses):
+                    callback(responses.pop(0))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/test_connection.py` around lines 330 - 342, Update the
send_response closure in the test loop to bind the current responses value
explicitly through a default parameter, rather than relying on the loop-scoped
variable lookup. Preserve the existing callback behavior and response ordering
for each subTest.

Source: Linters/SAST tools

cassandra/cluster.py (1)

5563-5596: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Prefer an outer loop over tail recursion for the drain restart.

_drain_keyspace_completions re-enters itself from finally when a newly-ready head appears; a plain outer loop removes the (unlikely but unbounded) stack growth and reads more clearly.

♻️ Sketch
-    def _drain_keyspace_completions(self):
-        with self._keyspace_completion_lock:
-            if self._keyspace_completion_runner_active:
-                return
-            self._keyspace_completion_runner_active = True
-
-        rerun = False
-        try:
+    def _drain_keyspace_completions(self):
+        with self._keyspace_completion_lock:
+            if self._keyspace_completion_runner_active:
+                return
+            self._keyspace_completion_runner_active = True
+
+        while True:
+            try:
                 while True:
                     ...
-        finally:
-            with self._keyspace_completion_lock:
-                self._keyspace_completion_runner_active = False
-                rerun = ...
-            if rerun:
-                self._drain_keyspace_completions()
+            finally:
+                with self._keyspace_completion_lock:
+                    self._keyspace_completion_runner_active = False
+                    rerun = ...
+                    if rerun:
+                        self._keyspace_completion_runner_active = True
+            if not rerun:
+                return
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cassandra/cluster.py` around lines 5563 - 5596, Replace the tail-recursive
restart in _drain_keyspace_completions with an outer loop that repeatedly drains
newly-ready keyspace completions. Keep the lock-protected runner-active state
and callback error handling intact, ensuring the method continues processing
when the queue head becomes dispatch-complete and ready without growing the call
stack.
tests/integration/standard/test_maintenance_mode_connection.py (1)

26-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Duplicated maintenance-mode fake CQL server across the two suites. Both files implement the same listener (bind ephemeral port, read a 9-byte frame, close without replying) to reproduce a maintenance-mode startup close; the shared root cause is one test double written twice.

  • tests/integration/standard/test_maintenance_mode_connection.py#L26-L81: keep this as the single implementation (ideally moved to a shared test helper module so it can be imported).
  • tests/unit/test_connection.py#L539-L668: drop the nested MaintenanceModeCqlServer/MaintenanceModeConnection real-socket server and reduce this test to the pending-connection ownership assertions using an in-process fake, relying on the integration test for the real-socket path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/integration/standard/test_maintenance_mode_connection.py` around lines
26 - 81, Deduplicate the maintenance-mode socket test double: retain the
implementation in tests/integration/standard/test_maintenance_mode_connection.py
lines 26-81, optionally relocating MaintenanceModeCqlServer to a shared test
helper for import. In tests/unit/test_connection.py lines 539-668, remove the
nested MaintenanceModeCqlServer and MaintenanceModeConnection real-socket
server, and reduce the test to pending-connection ownership assertions using an
in-process fake; the integration test remains responsible for real-socket
coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@cassandra/cluster.py`:
- Around line 5563-5596: Replace the tail-recursive restart in
_drain_keyspace_completions with an outer loop that repeatedly drains
newly-ready keyspace completions. Keep the lock-protected runner-active state
and callback error handling intact, ensuring the method continues processing
when the queue head becomes dispatch-complete and ready without growing the call
stack.

In `@cassandra/connection.py`:
- Around line 1958-1963: Update the exception handling in the wait_for_response
flow to chain the converted exception from ProtocolRequestValidationException
using the original validation_error as its cause. Keep
RequestValidationException propagation unchanged.
- Around line 1125-1145: Extract the pending-connection compatibility ladder
from factory into module-level _register_pending and _unregister_pending
helpers, covering both registration and removal paths. Preserve the existing
_register_pending_connection preference, _lock-guarded fallback, and unlocked
fallback for third-party pools, then have factory call these helpers while
retaining its shutdown and connection-close behavior.

In `@cassandra/pool.py`:
- Around line 778-782: Remove the redundant has_current_pool_fence condition
from the locks comprehension in the pool lock acquisition flow, retaining only
the non-None filter and existing session_lock exclusion. Preserve the earlier
return behavior for cases without a current pool fence.
- Around line 261-305: Remove the unused _transition_owner assignment and
initialization from Cluster._run_host_transition and Host.__init__, or
alternatively define a class-level default alongside the other transition fields
if the state is required; keep the transition queue behavior unchanged.

In `@tests/integration/standard/test_maintenance_mode_connection.py`:
- Around line 26-81: Deduplicate the maintenance-mode socket test double: retain
the implementation in
tests/integration/standard/test_maintenance_mode_connection.py lines 26-81,
optionally relocating MaintenanceModeCqlServer to a shared test helper for
import. In tests/unit/test_connection.py lines 539-668, remove the nested
MaintenanceModeCqlServer and MaintenanceModeConnection real-socket server, and
reduce the test to pending-connection ownership assertions using an in-process
fake; the integration test remains responsible for real-socket coverage.

In `@tests/unit/test_connection.py`:
- Around line 330-342: Update the send_response closure in the test loop to bind
the current responses value explicitly through a default parameter, rather than
relying on the loop-scoped variable lookup. Preserve the existing callback
behavior and response ordering for each subTest.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a2755b6a-1f5d-4630-8574-af5e37fb539c

📥 Commits

Reviewing files that changed from the base of the PR and between 0dcb688 and 08e8726.

📒 Files selected for processing (12)
  • cassandra/cluster.py
  • cassandra/connection.py
  • cassandra/io/twistedreactor.py
  • cassandra/pool.py
  • tests/integration/standard/test_maintenance_mode_connection.py
  • tests/unit/io/test_twistedreactor.py
  • tests/unit/test_cluster.py
  • tests/unit/test_connection.py
  • tests/unit/test_control_connection.py
  • tests/unit/test_host_connection_pool.py
  • tests/unit/test_response_future.py
  • tests/unit/test_shard_aware.py

@roydahan

roydahan commented Sep 9, 2026

Copy link
Copy Markdown

@dkropachev have you been able to run test.py with these changes?

@Lorak-mmk

Copy link
Copy Markdown

@dkropachev I see you requested my review, but it is still a draft. Should I review it?

@dkropachev

Copy link
Copy Markdown
Author

@dkropachev I see you requested my review, but it is still a draft. Should I review it?

Not yet

@dkropachev

Copy link
Copy Markdown
Author

@dkropachev have you been able to run test.py with these changes?

I just finished, will run these tests overnight

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

The implementation matches the stated scope and includes comprehensive regression coverage for the affected paths.

Review details
  • Files reviewed: 6/6 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

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

PR description needs to be updated it does not take added commit 83bdd4605 into consideration.

Comment thread cassandra/cluster.py
Comment thread cassandra/pool.py
Comment thread cassandra/cluster.py
Comment thread cassandra/metadata.py
Comment thread cassandra/cluster.py Outdated
Comment thread cassandra/cluster.py
Comment thread cassandra/cluster.py Outdated
Comment thread cassandra/cluster.py
Comment thread cassandra/pool.py
Comment thread tests/unit/test_control_connection.py
Comment thread cassandra/cluster.py
Comment thread cassandra/pool.py
@qodo-scylladb

qodo-scylladb Bot commented Sep 10, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

Tip of the day
💡 Did you know, you can turn these tips off under Display preferences

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

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

Re-reviewed at 06ff1b43. The host-id/endpoint divergence I raised is fixed -- I traced the same-address replacement through the new matching and it reconnects, and test_down_matches_replacement_at_stale_control_endpoint pins it. Accepting the declines on the retained Unix route, the get_host_by_host_id fallback, EndPoint ordering (#1009) and the sort assertion. Unit suite green on the head worktree: 971 passed, 125 skipped. Two nits below, neither blocking.

Comment thread cassandra/cluster.py
Comment thread cassandra/cluster.py
Keep a newly discovered local host on the Unix socket used by the control connection while retaining advertised network metadata and factory-based duplicate detection.

Resolve control hosts by stable identity across topology refreshes, schema checks, error attribution, and DOWN/REMOVE callbacks. Preserve direct reconnect fallback for Unix-backed and alternate routes when DOWN handling queues no callback.

Keep shard-aware pools on the Unix route without advertised TCP ports or source-port shard targeting, and make mixed Unix/network Host ordering deterministic.
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.

Version 3.29.11 breaks maintenance mode

6 participants