test: make client.tcl CLIENT KILL deterministic against connection-registration race

[liunyl]

Problem

tests/unit/eloq/client.tcl intermittently fails in CI's 3-node cluster log-replay phase with [exception]: Executing test client: ERR No such client. on r client kill $addr (observed on PR #527, run 29137305901, job RelWithDebInfo-ELOQDSS_ELOQSTORE-arm64). In that same job the file ran 9 times in cluster mode — 8 passed, only the log-replay run failed — so this is a race, not a cluster incompatibility, and it is independent of that PR's changes.

Root cause

The test opens a second connection, reads its addr via CLIENT INFO, then kills it from the r connection.

• CLIENT KILL / CLIENT LIST / CLIENT INFO and SELECT are all DirectCommands — they execute on the connected node with no auto_redirect / key routing. So both connections live on node 0, and CLIENT KILL scans node 0's own connection map. "Target landed on another node" is ruled out; $addr is also stable (nothing touches the killed connection between CLIENT INFO and the kill).
• The actual race: a freshly accepted brpc connection can already answer its own commands (via the io_uring recv ring) before Acceptor::OnNewConnectionsUntilEAGAIN inserts its socket into _socket_map — the map that CLIENT LIST / CLIENT KILL enumerate (brpc documents this "funny race condition" in acceptor.cpp). When CLIENT KILL wins that race it matches zero sockets → killed_ == 0 → ERR No such client.
• The window is sub-millisecond normally but widens under the CPU starvation of the log-replay phase (three nodes sharing core_number=2), which is why it flaked only there.

Fix

EloqKV's CLIENT KILL only supports the old-style addr:port form (killing by id would need a server change), so the fix waits until the target addr is actually visible in CLIENT LIST — the same _socket_map that CLIENT KILL scans — before killing it. Once the addr appears, the kill is deterministic. A small kill_client_by_addr helper replaces the three racy r client kill $addr sites. This keeps cluster coverage instead of restricting the cases to single-node, and follows the same "wait on the real condition, don't sleep/race" approach as the earlier pubsub flake fix (e5ca361).

Verification

Ran the real harness against a scratch single-node server: all four client.tcl tests pass, including the two later CLIENT KILL-by-addr sites (CLIENT ID, CLIENT INFO, CLIENT LIST, CLIENT KILL). No product code changes.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Tests
  • Improved client connection tests by waiting for client registration before terminating connections.
  • Reduced intermittent test failures and made client removal assertions more reliable.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c84db519-8d33-4ee9-b1db-629ed4c291ae

📥 Commits

Reviewing files that changed from the base of the PR and between 965d005 and d4fc936.

📒 Files selected for processing (1)

• tests/unit/eloq/client.tcl

---

Walkthrough

Client kill tests now wait for target addresses to appear in CLIENT LIST before issuing CLIENT KILL, preventing registration-order races and preserving removal assertions.

Changes

Client kill test synchronization

Layer / File(s)	Summary
Registration-aware client killing tests/unit/eloq/client.tcl	Adds kill_client_by_addr and updates client kill tests to wait for address visibility before killing clients and asserting their removal.

Estimated code review effort: 2 (Simple) | ~10 minutes

Poem

A bunny checked the client list twice,
Then sent each lagging hare advice:
“Appear first, then hop away!”
The tests now pass without delay,
While carrots cheer the tidy race.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and accurately summarizes the test-only fix for a CLIENT KILL race in client.tcl.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/client-tcl-kill-race

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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