test: track TcpProxy forwarders through socket cleanup - #963
Conversation
📝 WalkthroughWalkthrough
Sequence Diagram(s)sequenceDiagram
participant TcpProxy
participant Shutdown
participant Forwarder
participant SocketPair
TcpProxy->>Shutdown: acquire lifecycle lock
Shutdown->>SocketPair: shut down sockets
Forwarder->>Forwarder: wait for lifecycle lock
Forwarder->>SocketPair: close sockets
Forwarder->>TcpProxy: remove connection
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
Pull request overview
Keeps TcpProxy connections tracked until socket cleanup completes, fixing inaccurate connection reporting and shutdown synchronization.
Changes:
- Reorders forwarder cleanup and connection deregistration.
- Adds deterministic regression coverage for blocked socket cleanup.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
tests/integration/standard/test_client_routes.py |
Deregisters connections after closing sockets. |
tests/unit/test_tcp_proxy.py |
Tests tracking throughout socket cleanup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/test_tcp_proxy.py (1)
187-187: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid materializing
_connectionsfor one entry.Ruff reports RUF015 on this line. Use
next(iter(...))to avoid creating a list.Proposed change
- connection, thread = list(self.proxy._connections.items())[0] + connection, thread = next(iter(self.proxy._connections.items()))🤖 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_tcp_proxy.py` at line 187, Update the connection extraction in the test around self.proxy._connections to use next(iter(...)) and retrieve the single key-value pair without materializing the dictionary items into a list.Source: Linters/SAST tools
🤖 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.
Inline comments:
In `@tests/unit/test_tcp_proxy.py`:
- Around line 192-195: Update blocking_close_pair to wait indefinitely on
allow_cleanup before calling real_close_pair, removing the timeout so cleanup
cannot proceed until the test assertions and finally block release the barrier.
---
Nitpick comments:
In `@tests/unit/test_tcp_proxy.py`:
- Line 187: Update the connection extraction in the test around
self.proxy._connections to use next(iter(...)) and retrieve the single key-value
pair without materializing the dictionary items into a list.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c69fef7b-02cf-4866-b549-6152e33c0890
📒 Files selected for processing (2)
tests/integration/standard/test_client_routes.pytests/unit/test_tcp_proxy.py
4d6df3a to
35caeb5
Compare
nikagra
left a comment
nikagra
left a comment
There was a problem hiding this comment.
Verified and LGTM. The one-line reorder is correct and minimal, and the new test genuinely fails on the pre-fix ordering rather than passing on both.
The invariant this establishes
Entry absent from _connections => both sockets are already closed. The old pop-then-close window left a live forwarder holding open fds untracked, so a concurrent stop()/drop_connections() could snapshot straight past it while active_connections under-reported.
Why the new order is safe
Worth recording, since it's the non-obvious half. Close-before-pop means a concurrent stop() can now snapshot an entry whose sockets are already closed and call _shutdown_pair() on them. That's a harmless no-op rather than an fd-reuse hazard: CPython sets sock_fd = INVALID_SOCKET before the OS close(2), so shutdown() on a closed socket raises OSError EBADF (which _shutdown_pair catches) and never hands a recycled fd to the kernel. Confirmed locally - errno 9, fileno() == -1. So the _shutdown_and_join_connections docstring guarantee, "only the forwarder thread itself closes its sockets", still holds.
Two other properties the swap preserves:
_close_pair()is still called outside_lock, soactive_connectionsstays reachable during cleanup. No deadlock - and this is what makes the new test observable at all._close_pair()swallows per-socket exceptions, so thepop()is still unconditionally reached; a failingclose()can't leak the entry.
Verification
| Check | Result |
|---|---|
| New test logic on PR head | 40/40 pass |
| Same logic on pre-fix ordering | fails: active_connections == 0, expected 1 |
| Existing stress test against new ordering | 2178 connections, 0 escaping exceptions, active_connections == 0, 0 live forwarders |
Method: extracted TcpProxy and exec'd it standalone against a loopback echo server, so the pre/post comparison isolates the two-line swap and nothing else.
Also confirming patch.object(..., new=staticmethod(blocking_close_pair)) is the right construction here - mock reads target.__dict__[name], so it restores the staticmethod descriptor rather than degrading it to a bound method. Written as a plain function it would silently break self._close_pair(csock, tsock) for the sibling tests, so the wrapper is doing real work.
Non-blocking notes
Take or leave any of these:
- The class docstring (lines 105-115) still describes only #948, though the module docstring was updated to "#948 and #962". The new test's one-line docstring also departs from the file's convention of spelling out exactly what regresses - both sibling tests do.
- Line 187 reads
_connectionswithout_lock;test_concurrent_stop_and_drop_leaves_no_live_forwarderstakes the lock for the same read. Safe as written (single connection, idle accept loop) and consistent with the #948 test at line 144. - The test asserts the mechanism - the entry stays tracked. The harm named in #962 is the consequence: a concurrent
stop()/drop_connections()misses the forwarder. A thread callingdrop_connections()while_close_pair()is blocked would cover that directly. - Pre-existing, out of scope:
_handle_new_connection'st.start()failure path pops before closing - the mirror image of what this fixes. It's inside_lock, so no observer can see the intermediate state; correct as-is, just asymmetric now.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
292b7cd to
6fdc73b
Compare
6fdc73b to
32505c6
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (2)
tests/unit/test_tcp_proxy.py-17-17 (1)
17-17: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd
Fixes:#962`` to the PR description.The supplied PR description has no required issue-closing annotation.
As per coding guidelines, add appropriate
Fixes:annotations to the pull request description.🤖 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_tcp_proxy.py` at line 17, Update the pull request description to include the issue-closing annotation “Fixes: `#962`” for the shutdown/join synchronization change. Do not modify the implementation or tests.Source: Coding guidelines
tests/unit/test_tcp_proxy.py-238-242 (1)
238-242: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSuppress the intentional broad exception.
Line 241 triggers Ruff BLE001. Add an explicit
# noqa: BLE001if this helper must collect everydrop_connections()failure.As per coding guidelines, ensure all commits compile, pass static checks, and pass tests.
🤖 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_tcp_proxy.py` around lines 238 - 242, Update the drop_connections helper to explicitly suppress Ruff BLE001 on the intentional broad exception handler, while continuing to collect every failure in dropper_errors. Keep the existing self.proxy.drop_connections() call and error-collection behavior unchanged.Sources: Coding guidelines, Linters/SAST tools
🤖 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.
Other comments:
In `@tests/unit/test_tcp_proxy.py`:
- Line 17: Update the pull request description to include the issue-closing
annotation “Fixes: `#962`” for the shutdown/join synchronization change. Do not
modify the implementation or tests.
- Around line 238-242: Update the drop_connections helper to explicitly suppress
Ruff BLE001 on the intentional broad exception handler, while continuing to
collect every failure in dropper_errors. Keep the existing
self.proxy.drop_connections() call and error-collection behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: 570fd2b2-0dbf-4e0b-b8cf-10329c15b8f6
📒 Files selected for processing (2)
tests/tcp_proxy.pytests/unit/test_tcp_proxy.py
|
Regarding the CodeRabbit BLE001 note: the broad catch is intentional because this worker records any escaping |
Summary
Fixes: #962.
TcpProxyconnection registered until its forwarder finishes closing both socketsactive_connectionsreporting throughout socket cleanup_close_pair()and verifies the still-running forwarder remains trackedScope and compatibility
This changes only the
TcpProxyintegration-test helper and its unit coverage. Driver runtime behavior, wire protocols, and Scylla/Cassandra compatibility are unaffected.Validation
uv run pytest -q tests/unit/test_tcp_proxy.py(4 passed)git diff --check