test(txn): fix txn_kv_wiring port-collision TOCTOU flake#276
Conversation
start_txn_server bound a throwaway :0 probe to discover a port, dropped it, then let run_sharded rebind that port — a classic TOCTOU. Between the drop and the rebind a parallel test (same or another test binary) could win the freed ephemeral port; the caller then blind-slept 200ms and handed back a dead/wrong port, intermittently failing the suite when not pinned to --test-threads=1. Two defenses: - `reserve_unique_port`: a process-global `LazyLock<Mutex<HashSet<u16>>>` rejects any port the OS recycled from a just-finished sibling test, so no two tests in THIS binary are ever handed the same port. This is the half no readiness check can disambiguate — a sibling's live server would answer PING on the very port we lost. - `await_server_ready`: an active connect+PING poll (up to 3s) replaces the fixed 200ms sleep (itself a slow-CI-startup flake source) and lets start_txn_server retry the whole start on a fresh port when a bind is lost to another process. Thread-spawn body extracted to `spawn_txn_server_thread`; no production code touched. Verified flake-free across 13 fully-parallel runs of all 12 tests. author: Tin Dang <tindang.ht97@gmail.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughThe transaction KV wiring server harness now reserves process-unique ports, verifies readiness with Redis ChangesTransaction server startup reliability
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant TestHarness
participant MoonServer
participant RedisClient
TestHarness->>MoonServer: Start on reserved port
TestHarness->>RedisClient: Connect and send PING
RedisClient-->>TestHarness: Return readiness result
TestHarness->>MoonServer: Cancel and retry on failure
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![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
🤖 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/txn_kv_wiring.rs`:
- Around line 297-307: Update reserve_unique_port to use parking_lot::Mutex for
the HANDED_OUT static instead of std::sync::Mutex, and remove the .unwrap() from
its lock call because parking_lot::Mutex::lock() returns the guard directly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23e0e14b-c2ee-4149-9214-bbc70470829e
📒 Files selected for processing (2)
CHANGELOG.mdtests/txn_kv_wiring.rs
| async fn reserve_unique_port() -> u16 { | ||
| static HANDED_OUT: std::sync::LazyLock<std::sync::Mutex<std::collections::HashSet<u16>>> = | ||
| std::sync::LazyLock::new(|| std::sync::Mutex::new(std::collections::HashSet::new())); | ||
| loop { | ||
| let probe = TcpListener::bind("127.0.0.1:0").await.unwrap(); | ||
| let port = probe.local_addr().unwrap().port(); | ||
| drop(probe); | ||
| if HANDED_OUT.lock().unwrap().insert(port) { | ||
| return port; | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use parking_lot::Mutex and eliminate the poisoned-lock unwrap pattern.
reserve_unique_port uses std::sync::Mutex and .lock().unwrap(), violating two coding guidelines: (1) "Use parking_lot::RwLock and parking_lot::Mutex instead of std::sync locks" and (2) "do not use poisoned-lock unwrap patterns." parking_lot is already a dependency in this file (e.g., lines 174, 192), so the fix is straightforward — parking_lot::Mutex::lock() returns a guard directly without Result.
As per coding guidelines: "Use parking_lot::RwLock and parking_lot::Mutex instead of std::sync locks; do not use poisoned-lock unwrap patterns."
🔒 Proposed fix
async fn reserve_unique_port() -> u16 {
- static HANDED_OUT: std::sync::LazyLock<std::sync::Mutex<std::collections::HashSet<u16>>> =
- std::sync::LazyLock::new(|| std::sync::Mutex::new(std::collections::HashSet::new()));
+ static HANDED_OUT: std::sync::LazyLock<parking_lot::Mutex<std::collections::HashSet<u16>>> =
+ std::sync::LazyLock::new(|| parking_lot::Mutex::new(std::collections::HashSet::new()));
loop {
let probe = TcpListener::bind("127.0.0.1:0").await.unwrap();
let port = probe.local_addr().unwrap().port();
drop(probe);
- if HANDED_OUT.lock().unwrap().insert(port) {
+ if HANDED_OUT.lock().insert(port) {
return port;
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async fn reserve_unique_port() -> u16 { | |
| static HANDED_OUT: std::sync::LazyLock<std::sync::Mutex<std::collections::HashSet<u16>>> = | |
| std::sync::LazyLock::new(|| std::sync::Mutex::new(std::collections::HashSet::new())); | |
| loop { | |
| let probe = TcpListener::bind("127.0.0.1:0").await.unwrap(); | |
| let port = probe.local_addr().unwrap().port(); | |
| drop(probe); | |
| if HANDED_OUT.lock().unwrap().insert(port) { | |
| return port; | |
| } | |
| } | |
| async fn reserve_unique_port() -> u16 { | |
| static HANDED_OUT: std::sync::LazyLock<parking_lot::Mutex<std::collections::HashSet<u16>>> = | |
| std::sync::LazyLock::new(|| parking_lot::Mutex::new(std::collections::HashSet::new())); | |
| loop { | |
| let probe = TcpListener::bind("127.0.0.1:0").await.unwrap(); | |
| let port = probe.local_addr().unwrap().port(); | |
| drop(probe); | |
| if HANDED_OUT.lock().insert(port) { | |
| return port; | |
| } | |
| } | |
| } |
🤖 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/txn_kv_wiring.rs` around lines 297 - 307, Update reserve_unique_port to
use parking_lot::Mutex for the HANDED_OUT static instead of std::sync::Mutex,
and remove the .unwrap() from its lock call because parking_lot::Mutex::lock()
returns the guard directly.
Source: Coding guidelines
2 participants
Summary
Fixes an intermittent failure in the
txn_kv_wiringintegration suite caused by a port-selection TOCTOU race.start_txn_serverbound a throwawayTcpListener::bind("127.0.0.1:0")probe to discover a free port, dropped it, then letrun_shardedrebind that port. Between the drop and the rebind, a parallel test — in this binary or another test binary — could win the just-freed ephemeral port. The caller then blind-slept 200ms and handed back a dead/wrong port, so tests flaked whenever the suite wasn't pinned to--test-threads=1(which the module doc-comment recommends but nothing enforces).Fix
Two independent defenses, no production code touched:
reserve_unique_port— a process-globalLazyLock<Mutex<HashSet<u16>>>rejects any port the OS recycled from a just-finished sibling test, so no two tests in this binary are ever handed the same port. This is the half a readiness check alone cannot solve: a sibling's live server would happily answerPINGon the very port we lost, masking the collision.await_server_ready— an active connect+PINGpoll (up to 3s) replaces the fixed 200ms sleep (itself a slow-CI-startup flake source) and letsstart_txn_serverretry the whole start on a fresh port when a bind is genuinely lost to another process.The background thread-spawn body was extracted verbatim into
spawn_txn_server_threadso the retry loop can re-invoke it per attempt.Verification
--no-default-features --features runtime-tokio,jemalloc.--test-threads=1) 13× — all 12 tests pass every run, 0 failures, with the readiness poll absorbing slow/contended starts (one run 3.0s vs typical 0.1s) instead of flaking.Summary by CodeRabbit
Bug Fixes
Documentation