test: fix TcpProxy close/race in NLB test helper (test_client_routes.py)

[mykaul]

Summary

Fixes#948.

Two tests in tests/integration/standard/test_client_routes.py that drive a
hand-rolled NLBEmulator/TcpProxy test helper in front of a real CCM
cluster have been failing intermittently in CI across 15+ otherwise-unrelated
PRs (see #948 for the full failure history and signatures).

Code review of the helper turned up a real, reproducible bug:
TcpProxy.stop()/drop_connections() closed a connection's two sockets
directly from the manager thread while that connection's own forwarder
thread (_forward_loop) could still be blocked in select()/recv() on
those exact file descriptors -- with no synchronization at all. Since file
descriptors are process-global, the freed fd number could be silently
recycled by the OS into a brand new connection before the stale forwarder
thread's blocked call unwound, causing it to read/write/close a socket that
no longer belonged to it. stop() also only ever joined the accept-loop
thread, never the per-connection forwarder threads it had just closed
sockets out from under.

The fix (test helper only, no driver code touched)

• TcpProxy._connections now maps (client_sock, target_sock) to the
  forwarder thread serving that pair (was a bare set).
• stop()/drop_connections() now shutdown(SHUT_RDWR) both sockets --
  safe to call concurrently with a thread blocked in select()/recv()
  on the same socket, unlike close() -- and then join() every
  forwarder thread before returning. Only the forwarder thread itself
  ever calls close() on its own sockets now, and only after it has
  fully stopped using them.
• _handle_new_connection starts the forwarder thread before
  publishing it into _connections, so a concurrent
  stop()/drop_connections() can never observe (and try to .join())
  a thread that hasn't started yet (which would raise RuntimeError).
• NLBEmulator.add_node()/remove_node() now serialize against each
  other via an RLock, so a new proxy/connection can never be created
  while another thread's remove_node() is still tearing one down.

Validation

1. Standalone stress harness (fast, no CCM needed)

To get fast, high-iteration signal without paying for a full CCM bootstrap
each time, I extracted TcpProxy/NLBEmulator into a throwaway module and
drove it directly: a tiny local echo backend, TcpProxy in front of it, 16
concurrent clients continuously sending/verifying unique payloads through
the proxy, and a "chaos" thread calling drop_connections() /
stop()+restart-on-the-same-port dozens of times per iteration to force
the exact race.

	pre-fix (40 iters)	post-fix (40 iters)	post-fix (150 iters)
Corruptions	0	0	0
Unhandled thread exceptions	162 (ValueError: file descriptor cannot be a negative integer (-1) raised inside _forward_loop)	0	0
Leaked forwarder threads	0/40	0/40	0/150

The pre-fix crashes are a direct, reproducible demonstration of the race:
the manager thread's close() invalidates the Python socket object's fd
out from under a forwarder thread still using it in its while loop
(closing a socket sets its internal fd to -1, and the next
select.select() call in that thread raises ValueError on the now
-1 fd -- an exception type the original code's
except (OSError, ConnectionResetError, BrokenPipeError) did not even
catch). Post-fix, 190 total stress iterations under the same aggressive
chaos parameters produced zero corruptions, zero exceptions, and zero
leaked threads.

2. Full end-to-end runs against a real CCM cluster

TestFullNodeReplacementThroughNlb::test_should_survive_full_node_replacement_through_nlb
-- the test flagged in #948 as never having been run repeatedly before --
run back-to-back against a real local 3-node (scaling to 6-node) Scylla
cluster:

• Pre-fix: 12/12 passed, 0 failures. This did not reproduce the
  reported HTTP 500 / ConnectionShutdown failures locally in this many
  runs, which is expected given the race is timing-dependent and the
  failures were observed on the order of ~1 in several dozen CI runs
  historically, not deterministically.
• Post-fix: 20/20 passed, 0 failures. Ran back-to-back immediately
  after the pre-fix baseline above, same test, same environment. Combined
  with the stress-harness numbers (162 crashes -> 0 crashes under direct
  concurrency stress), this is now validated both mechanistically (the
  underlying race is real and gone) and end-to-end (the actual test the
  fix targets runs clean 32/32 total across pre+post-fix baselines).

I hit some local environment instability partway through testing (a
worktree got garbage-collected mid-run by this box's housekeeping, losing
a few in-flight CCM iterations to unrelated FileNotFoundErrors rather
than real test failures -- those are excluded from the counts above, which
only include cleanly-completed runs).

Scope

This only touches the test helper (TcpProxy/NLBEmulator in
tests/integration/standard/test_client_routes.py); no driver code is
modified.

[coderabbitai]

📝 Walkthrough

Walkthrough

TcpProxy now tracks forwarder threads by socket pair, coordinates connection registration and startup, interrupts socket operations during teardown, joins forwarders, and retains timed-out threads. NLBEmulator synchronizes node proxy creation and removal with a reentrant lock and snapshots proxies during address iteration. New unit tests cover timed-out joins and concurrent connection shutdown.

Sequence Diagram(s)

sequenceDiagram
participant NLBEmulator
participant TcpProxy
participant ForwarderThread
participant ClientSocket
participant TargetSocket
NLBEmulator->>TcpProxy: stop removed proxy
TcpProxy->>ClientSocket: shutdown(SHUT_RDWR)
TcpProxy->>TargetSocket: shutdown(SHUT_RDWR)
ClientSocket-->>ForwarderThread: interrupt blocked operation
TargetSocket-->>ForwarderThread: interrupt blocked operation
TcpProxy->>ForwarderThread: join with timeout
ForwarderThread->>TcpProxy: remove exited connection entry

Loading

Suggested reviewers:dkropachev

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	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 is concise and accurately summarizes the TcpProxy/NLB helper race fix in the integration test helper.
Description check	✅ Passed	The description covers the summary, fix details, validation, scope, and issue reference; only the checklist template is not fully mirrored.
Linked Issues check	✅ Passed	The changes address the reported TcpProxy teardown race, thread joining, connection tracking, and NLBEmulator locking described in #948.
Out of Scope Changes check	✅ Passed	The PR stays within the test helper and validation tests, with no unrelated driver or public API changes.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Fix failing CI checks

---

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

[Copilot]

Pull request overview

Fixes concurrency races in the integration-test TCP proxy lifecycle.

Changes:

• Tracks and joins connection-forwarding threads.
• Uses socket shutdown before forwarder cleanup.
• Serializes NLB node additions and removals.

---

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

[coderabbitai]

Actionable comments posted: 1

🤖 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/integration/standard/test_client_routes.py`:
- Around line 186-196: Update the connection setup around _forward_loop so
thread registration and t.start() occur atomically under self._lock. Insert the
thread into self._connections and increment total_connections before starting
it, ensuring the forwarder’s finally cleanup cannot run before registration;
preserve the existing connection key and counters.

🪄 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: 12d23d56-52d7-4e43-8ae8-f81ae9e59498

📥 Commits

Reviewing files that changed from the base of the PR and between 9b5b037 and 2b6705d.

📒 Files selected for processing (1)

• tests/integration/standard/test_client_routes.py

[mykaul]

Fixed the register-before-start race flagged by review (copilot-pull-request-reviewer + coderabbitai, both on line 196)

Both bots correctly flagged the same residual race in the previous version of this fix: _handle_new_connection called t.start()before publishing the thread into self._connections (in a separate, later with self._lock: block), specifically to avoid stop()/drop_connections() trying to .join() a thread object that hadn't been started yet. But that reopened a different window: a very short-lived connection (e.g. immediate EOF) could run _forward_loop's finally -- which pops the not-yet-present key (a no-op) and closes the sockets -- before the main thread ever inserted the entry, leaving a stale/leaked dict entry that nothing would ever clean up. Worse, a concurrent stop() could snapshot _connections during that exact window, miss the thread entirely, and return as if fully shut down while the thread was still actively using its sockets.

The fix

• _handle_new_connection now registers the thread into _connectionsand calls t.start() as a single atomic sequence, both inside the same with self._lock: block, registration beforestart(). It also re-checks _running inside that same critical section and rejects the new connection outright (closing both sockets, no thread ever created) if shutdown has already begun.
• _shutdown_and_join_connections() (used by both stop() and drop_connections()) now takes a stopping flag; only stop() passes stopping=True, which flips _running = False in the same locked critical section that takes the _connections snapshot. drop_connections() leaves _running untouched, since it's meant to drop in-flight connections while the proxy keeps running (reconnect tests rely on this).
• Net effect: either a new connection's register+start happens-before stop()'s snapshot (so it's included and properly joined), or stop()'s _running=False happens-before the new connection's check (so it's rejected before a thread is ever created). There's no third case, so neither a stale dict entry nor an early stop() return past a still-running thread is possible anymore -- and since registration always precedes start() inside the same critical section, any thread that could ever be joined is guaranteed to have already been started, eliminating the original RuntimeError: cannot join thread before it is started risk too.
• Also hardened t.start() itself with a try/except: if thread creation fails (e.g. OS resource exhaustion), the just-added registration is undone and the sockets closed, rather than leaving a registered-but-never-started thread behind (which would reintroduce the exact join-before-start RuntimeError this whole approach exists to avoid).

New stress-test evidence (targeting exactly this scenario)

Extended the existing stress harness with a dedicated scenario: many clients making rapid connect-then-immediately-close connections (the fastest way to trigger _forward_loop finishing before registration), racing against a chaos thread hammering stop() (and drop_connections()), instrumented to detect (a) stale entries in _connections right after stop() returns, and (b) forwarder threads still alive after stop() returns.

	pre-fix (40 iters / 11867 connects)	post-fix (100 iters / 30532 connects)
Stale _connections entries after stop()	74	0
Exceptions / RuntimeErrors	0 / 0	0 / 0
Leaked threads at end of iteration	0/40	0/100

The original race this PR fixes (full stress harness: many concurrent clients + chaos stop()/drop_connections()/reconnects) remains at 0 corruptions/exceptions/leaked threads over 100 iterations post-fix, confirming this change doesn't reopen it.

Other validation

• tests/unit/ (full suite): 720 passed, 88 skipped, 0 failed.
• Local CCM sanity checks: TestPrivateLinkConnectivity::test_queries_succeed_through_proxy 5/5 passed; TestFullNodeReplacementThroughNlb::test_should_survive_full_node_replacement_through_nlb 1/1 passed (87.9s -- this one is expensive, so only a single confirmatory run was done given the stress harness above is the primary evidence for this specific race).

Both review threads on this topic marked resolved. Pushed to this branch (fix/948-tcpproxy-forwarder-race), amended into the existing commit as before.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

tests/integration/standard/test_client_routes.py:275

• The forwarder removes itself from _connections before closing its sockets. If it is descheduled between these operations, a concurrent stop() or drop_connections() snapshots no entry and returns while this thread still owns open file descriptors, so the new drain guarantee is not actually established. Close the pair before unregistering it so a missing entry always means socket cleanup has completed.

 self._connections.pop((client_sock, target_sock), None)

tests/integration/standard/test_client_routes.py:124

• The synchronization added here has no committed regression test; the existing CCM tests only encounter the race probabilistically, and the standalone stress harness described in the PR is throwaway. Please add a focused automated test that forces stop()/drop_connections() to overlap an active forwarder and asserts that all forwarders and sockets are drained before return.

This issue also appears on line 275 of the same file.

 def _shutdown_and_join_connections(self, stopping=False):

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

tests/integration/standard/test_client_routes.py (1)

392-399: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

stop() iterates/clears _node_proxies without the lock now guarding it.

add_node()/remove_node() mutate self._node_proxies under self._lock, but stop() iterates self._node_proxies.values() (Line 396) and calls .clear() (Line 398) unguarded. A concurrent add_node()/remove_node() (e.g. during the PR's own stress/node-replacement scenarios) mutating the dict mid-iteration will raise RuntimeError: dictionary changed size during iteration, and entries added mid-clear() would leak an unstopped proxy. drop_all_connections()/total_proxy_connections()/active_proxy_connections() have the same unguarded-iteration pattern for the same reason.

🔧 Proposed fix: snapshot and clear under the lock

 def stop(self):
self._running = False
if self._discovery_proxy:
self._discovery_proxy.stop()
- for proxy in self._node_proxies.values():- proxy.stop()- self._node_proxies.clear()+ with self._lock:+ proxies = list(self._node_proxies.values())+ self._node_proxies.clear()+ for proxy in proxies:+ proxy.stop()
log.info("NLB stopped")

🤖 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_client_routes.py` around lines 392 - 399,
Update NLB.stop(), drop_all_connections(), total_proxy_connections(), and
active_proxy_connections() to synchronize access to _node_proxies with
self._lock. Snapshot the proxy values and clear the dictionary while holding the
lock, then perform stop or connection aggregation on the stable snapshot outside
the lock so concurrent add_node() and remove_node() operations cannot mutate the
collection during iteration or leave newly added proxies unstopped.

🤖 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.
Outside diff comments:
In `@tests/integration/standard/test_client_routes.py`:
- Around line 392-399: Update NLB.stop(), drop_all_connections(),
total_proxy_connections(), and active_proxy_connections() to synchronize access
to _node_proxies with self._lock. Snapshot the proxy values and clear the
dictionary while holding the lock, then perform stop or connection aggregation
on the stable snapshot outside the lock so concurrent add_node() and
remove_node() operations cannot mutate the collection during iteration or leave
newly added proxies unstopped.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 722b2ca8-517f-40d1-8d9b-0126a57673d2

📥 Commits

Reviewing files that changed from the base of the PR and between 2b6705d and 1c41efc.

📒 Files selected for processing (1)

• tests/integration/standard/test_client_routes.py

[coderabbitai]

Actionable comments posted: 1

🤖 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/integration/standard/test_client_routes.py`:
- Around line 344-348: Update remove_node and the rr_handler request path to
serialize node removal with discovery routing: hold self._lock through
_live_addresses(), node selection, and original_handler(...), so routing cannot
iterate or select a proxy while remove_node mutates _node_proxies. Preserve the
existing proxy stop behavior after removal.

🪄 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: e102de68-5c99-483c-8b46-ff70cc793e2e

📥 Commits

Reviewing files that changed from the base of the PR and between 1c41efc and fc83f91.

📒 Files selected for processing (1)

• tests/integration/standard/test_client_routes.py

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

tests/integration/standard/test_client_routes.py:147

• Do not discard entries for threads that are still alive after the timeout. Doing so makes active_connections report them as gone and prevents a later drop_connections()/stop() from retrying the shutdown and join, leaving the live forwarder and its file descriptors unmanaged. Retain live entries; _forward_loop will remove them when it actually exits.

 with self._lock:
for key, _thread in connections:
self._connections.pop(key, None)

tests/integration/standard/test_client_routes.py:122

• Please add a checked-in regression test for this synchronization path. The existing integration tests exercise it only through timing-dependent failures, while the deterministic stress harness described in the PR is throwaway code; without an automated assertion that concurrent drop/stop leaves no live forwarders or unhandled exceptions, the original race can regress unnoticed.

This issue also appears on line 145 of the same file.

 def _shutdown_and_join_connections(self, stopping=False):

[mykaul]

Addressed two suppressed (low-confidence, non-posted) Copilot review findings on _shutdown_and_join_connections in tests/integration/standard/test_client_routes.py:

1. Discarded entries for still-alive threads after join timeout._shutdown_and_join_connections was unconditionally popping every connection from self._connections after the join loop, even ones whose thread.join(timeout=5) timed out with the thread still running. That made active_connections under-report live connections and made it impossible for a later stop()/drop_connections() to retry reaping an orphaned forwarder thread — permanently leaking it and its fds. Fixed to only pop entries whose thread is confirmed dead (not thread.is_alive()); still-alive entries stay tracked so a subsequent shutdown call can retry (or _forward_loop's own lock-protected, idempotent self-removal eventually reaps them).

2. Missing checked-in regression test. Added tests/unit/test_tcp_proxy.py — TcpProxy has no CCM/cluster dependency (just sockets), so it runs as a fast, deterministic unit test against a local dummy TCP echo backend:

   • test_timed_out_forwarder_thread_is_retained_until_it_exits: deterministically forces the "still alive after the join timeout" path (by neutering _shutdown_pair and shrinking one thread's effective join wait) and asserts the connection stays tracked until a retried drop_connections() actually reaps it. Confirmed this fails reliably (15/15 runs) against the pre-fix code and passes reliably (30/30 runs) with the fix.
   • test_concurrent_stop_and_drop_leaves_no_live_forwarders: a stress test hammering drop_connections() from multiple threads while other threads continuously open/close real connections through the proxy, asserting no unhandled exceptions and no forwarder threads left alive once stop() returns.

Full tests/unit/ suite: 722 passed, 88 skipped, 0 failed.

Both changes are amended into the existing commit (no new commit) and pushed to this branch.

[Copilot]

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)

tests/integration/standard/test_client_routes.py:228

• Unregistering the forwarder before closing its sockets leaves a teardown race: stop()/drop_connections() can take their snapshot after this pop(), miss the still-running thread, and return while its socket pair is still open. This also weakens the new remove_node() serialization guarantee. Close the owned sockets first, then remove the entry; once the entry is absent, teardown can safely assume that the thread no longer owns open descriptors.

 self._connections.pop((client_sock, target_sock), None)

[coderabbitai]

🧹 Nitpick comments (3)

tests/integration/standard/test_client_routes.py (2)

392-403: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Same dict-iteration hazard remains in stop()/total_proxy_connections()/active_proxy_connections()/drop_all_connections().

_live_addresses() now correctly snapshots self._node_proxies.values() under _lock to avoid RuntimeError: dictionary changed size during iteration racing with add_node()/remove_node(). NLBEmulator.stop() (Line 342), total_proxy_connections() (Line 371), active_proxy_connections() (Line 374), and drop_all_connections() (Lines 377-380) iterate the same dict without holding _lock, so they're exposed to the identical class of failure this PR set out to fix, if ever invoked concurrently with add_node()/remove_node().

♻️ Suggested consistency fix

 def total_proxy_connections(self):
- return sum(p.total_connections for p in self._node_proxies.values())+ with self._lock:+ proxies = list(self._node_proxies.values())+ return sum(p.total_connections for p in proxies)
def active_proxy_connections(self):
- return sum(p.active_connections for p in self._node_proxies.values())+ with self._lock:+ proxies = list(self._node_proxies.values())+ return sum(p.active_connections for p in proxies)
def drop_all_connections(self):
- for proxy in self._node_proxies.values():+ with self._lock:+ proxies = list(self._node_proxies.values())+ for proxy in proxies:
proxy.drop_connections()

Please confirm whether these are ever called from a thread other than the one driving add_node()/remove_node() (e.g. from assertion helpers running while the discovery thread or a test worker mutates nodes); if so this is reachable, not just theoretical.

🤖 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_client_routes.py` around lines 392 - 403,
Protect every iteration over self._node_proxies in NLBEmulator.stop(),
total_proxy_connections(), active_proxy_connections(), and
drop_all_connections() with self._lock, matching the snapshot protection already
used by _live_addresses(). Preserve each method’s existing behavior while
ensuring concurrent add_node()/remove_node() calls cannot mutate the dictionary
during iteration.

---

348-358: 🚀 Performance & Scalability | 🔵 Trivial

Lock held across blocking proxy.start()/proxy.stop().

add_node()/remove_node() now hold self._lock for the full duration of proxy.start()/proxy.stop() (the latter can block up to 5s joining forwarders). Since _live_addresses() (called synchronously from the discovery proxy's own accept-loop thread on every routed connection) takes the same lock, a slow remove_node()/add_node() call from the test thread can stall new discovery-port connections for the join duration. This looks like an accepted trade-off to close the race the lock is meant to prevent, but worth keeping in mind if node-churn-heavy tests (e.g. full node replacement) start seeing connection timeouts rather than crashes.

🤖 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_client_routes.py` around lines 348 - 358,
Refactor add_node() and remove_node() so self._lock is not held during blocking
proxy.start() or proxy.stop() calls. Reserve or detach node proxies under the
lock, then perform lifecycle operations outside it while preserving the
race-prevention guarantees needed by _live_addresses() and _add_node_proxy().

tests/unit/test_tcp_proxy.py (1)

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

Minor style nit: prefer next(iter(...)) over slicing a materialized list.

Ruff (RUF015) flags this; purely cosmetic here since the dict only ever has one entry at this point in the test.

♻️ Suggested tweak

- (csock, tsock), thread = list(self.proxy._connections.items())[0]+ (csock, tsock), 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 144, In the test’s connection
extraction, replace the materialized-list indexing around
self.proxy._connections with next(iter(...)) to retrieve the sole mapping entry
directly and satisfy Ruff RUF015; leave the surrounding thread assignment and
assertions unchanged.

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.
Nitpick comments:
In `@tests/integration/standard/test_client_routes.py`:
- Around line 392-403: Protect every iteration over self._node_proxies in
NLBEmulator.stop(), total_proxy_connections(), active_proxy_connections(), and
drop_all_connections() with self._lock, matching the snapshot protection already
used by _live_addresses(). Preserve each method’s existing behavior while
ensuring concurrent add_node()/remove_node() calls cannot mutate the dictionary
during iteration.
- Around line 348-358: Refactor add_node() and remove_node() so self._lock is
not held during blocking proxy.start() or proxy.stop() calls. Reserve or detach
node proxies under the lock, then perform lifecycle operations outside it while
preserving the race-prevention guarantees needed by _live_addresses() and
_add_node_proxy().
In `@tests/unit/test_tcp_proxy.py`:
- Line 144: In the test’s connection extraction, replace the materialized-list
indexing around self.proxy._connections with next(iter(...)) to retrieve the
sole mapping entry directly and satisfy Ruff RUF015; leave the surrounding
thread assignment and assertions unchanged.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: d1ac3d78-3e5e-4402-86ff-910ce2f496d1

📥 Commits

Reviewing files that changed from the base of the PR and between fc83f91 and e11836c.

📒 Files selected for processing (2)

• tests/integration/standard/test_client_routes.py
• tests/unit/test_tcp_proxy.py