refactor: remove docker requirement of tuic#1345
Conversation
📝 WalkthroughWalkthroughAdds an in-process TUIC test server and test utilities, moves TUIC tests off Docker to local TCP tests, bumps Changes
Sequence DiagramsequenceDiagram
participant Test as Test Code
participant TSP as TuicServerProcess
participant Task as Tokio Task
participant Server as tuic-server
participant Channel as Oneshot Channel
Test->>TSP: start()
activate TSP
TSP->>TSP: bind UDP to find free port
TSP->>TSP: construct tuic_server::Config (creds, TLS, ALPN)
TSP->>Task: spawn async server task with config
activate Task
Task->>Server: Server::init(ctx) on selected port
activate Server
Server->>Channel: send readiness (oneshot)
deactivate Server
deactivate Task
TSP->>Channel: await readiness (30s timeout)
Channel-->>TSP: readiness received
TSP-->>Test: return TuicServerProcess(handle, port)
deactivate TSP
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clash-lib/src/proxy/tuic/mod.rs`:
- Around line 533-559: The test currently sets session.destination to
"127.0.0.1:9999" (where nothing listens) so failures can come from an
unreachable upstream; instead spawn a real local TcpListener and use its bound
address as the session.destination so the upstream is reachable, then proceed to
call handler.connect_stream(&session, resolver).await, accept the incoming
connection on the listener if any, and assert that the TUIC stream is
rejected/closed (i.e., that stream.write_all or stream.read_exact returns Err)
before any successful proxy I/O; update the code around Session construction and
the connect_stream/assertion to create the listener (e.g.,
tokio::net::TcpListener::bind("127.0.0.1:0").await), set destination from
listener.local_addr(), optionally spawn a task to accept the upstream
connection, and ensure the test cleans up the listener/task after the assertion.
In `@clash-lib/src/proxy/tuic/test_utils.rs`:
- Around line 14-15: The current alloc_port() implementation picks a free port
by creating and immediately dropping a temporary UdpSocket, introducing a TOCTOU
race when tuic-server later binds; change alloc_port() to bind a socket to 0
(e.g., UdpSocket::bind("127.0.0.1:0") or TcpListener::bind("127.0.0.1:0")), read
the actual assigned port from socket.local_addr().port(), and return that port
(keeping the socket alive until the server is spawned if necessary), or
alternatively modify the tuic-server startup path to accept port 0 and read the
bound port from the listener it creates; if keeping the original approach, add a
bounded retry loop around server bind (checking for EADDRINUSE) to retry a few
times before failing to avoid flakiness when alloc_port() selection races with
other processes.
- Around line 73-93: The test helper currently treats Server::init errors as
readiness by always sending on ready_tx and then ignoring the channel result;
change the ready signalling to propagate init failures: make ready_tx/ready_rx
send a Result<(), anyhow::Error> (or equivalent) instead of unit, and on Err(e)
from tuic_server::server::Server::init send Err(e) (or forward the error) rather
than sending Ok(()) so the caller can observe the failure; then when awaiting
the timeout on ready_rx, propagate the received Err to the caller (using ? or
map_err) instead of calling .ok(), so start()/the test returns Err when
initialization failed and does not report readiness for a dead server.
🪄 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 Plus
Run ID: 098d516d-f27a-4929-b891-74be75e6d082
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
clash-lib/Cargo.tomlclash-lib/src/proxy/tuic/mod.rsclash-lib/src/proxy/tuic/test_utils.rsclash-lib/src/proxy/utils/test_utils/docker_utils/consts.rs
💤 Files with no reviewable changes (1)
- clash-lib/src/proxy/utils/test_utils/docker_utils/consts.rs
| let port = alloc_port(); | ||
| let server_addr: SocketAddr = format!("127.0.0.1:{port}").parse()?; |
There was a problem hiding this comment.
Avoid the free-port race in alloc_port().
Picking a port with a temporary UdpSocket, dropping it, and then asking tuic-server to bind that same port is a TOCTOU race. Under parallel test execution another process can claim the port between Lines 124 and 73, which turns this helper flaky.
Use a bind-to-:0 flow that returns the actual bound port, or add a retry loop around startup when bind fails with address-in-use.
Also applies to: 120-124
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clash-lib/src/proxy/tuic/test_utils.rs` around lines 14 - 15, The current
alloc_port() implementation picks a free port by creating and immediately
dropping a temporary UdpSocket, introducing a TOCTOU race when tuic-server later
binds; change alloc_port() to bind a socket to 0 (e.g.,
UdpSocket::bind("127.0.0.1:0") or TcpListener::bind("127.0.0.1:0")), read the
actual assigned port from socket.local_addr().port(), and return that port
(keeping the socket alive until the server is spawned if necessary), or
alternatively modify the tuic-server startup path to accept port 0 and read the
bound port from the listener it creates; if keeping the original approach, add a
bounded retry loop around server bind (checking for EADDRINUSE) to retry a few
times before failing to avoid flakiness when alloc_port() selection races with
other processes.
There was a problem hiding this comment.
Let me modify tuic-server to fix this
There was a problem hiding this comment.
@Itsusinn, sounds good! The cleanest approach on the tuic-server side would be:
- Allow binding to port
0in the listener setup so the OS assigns a free port atomically. - Expose the actual bound port back to the caller — for example, by sending it over the readiness
oneshotchannel (changing its type from()tou16) or storing it in a sharedArc<AtomicU16>that the server writes before signalling readiness.
Then on the test_utils.rs side, the flow becomes:
// Instead of alloc_port(), just pass 0:
let server_addr: SocketAddr = "127.0.0.1:0".parse()?;
// After the ready signal, receive the actual bound port:
let port = ready_rx.await?; // if oneshot carries u16This completely eliminates the TOCTOU window since the port is never released between selection and bind. Let me know if you'd like a more detailed sketch of the tuic-server-side changes once you start working on it!
🧠 Learnings used
Learnt from: CR
Repo: Watfaq/clash-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-19T07:48:02.063Z
Learning: Applies to **/*.rs : Use conditional compilation with feature flags for different protocols (shadowsocks, tuic, ssh, onion, shadowquic, tokio-console, bench)
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📊 Proxy Throughput ResultsFull test logDownload the |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/coverage.yml:
- Around line 36-39: The coverage workflow removed explicit NASM provisioning
but README.md/CLAUDE.md still declare NASM required for Windows builds; restore
an explicit NASM install step in .github/workflows/coverage.yml (near the
"Install cargo-llvm-cov" step) that runs only for the windows-latest job, or
alternatively update the docs to state that GitHub runners now include NASM;
locate the relevant job in coverage.yml around the "Install cargo-llvm-cov" step
and either insert a Windows-only NASM install action (e.g., choco/winget
installation) before build/coverage steps or change README.md/CLAUDE.md to
reflect the runner provisioning so the declared requirements match the workflow.
🪄 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 Plus
Run ID: 7d4d0947-98f7-4d56-8cbd-a1bc2d990bed
📒 Files selected for processing (1)
.github/workflows/coverage.yml
There was a problem hiding this comment.
🧹 Nitpick comments (2)
clash-lib/src/proxy/tuic/test_utils.rs (1)
60-74: Consider combining duplicate iteration patterns.Both
online_counterandtraffic_statsiterate overcfg.userswith identical structure. A single loop would be cleaner.♻️ Suggested refactor
- let mut online_counter = HashMap::new(); - for (user, _) in cfg.users.iter() { - online_counter - .insert(user.to_owned(), std::sync::atomic::AtomicUsize::new(0)); - } - let mut traffic_stats = HashMap::new(); - for (user, _) in cfg.users.iter() { - traffic_stats.insert( - user.to_owned(), - ( - std::sync::atomic::AtomicUsize::new(0), - std::sync::atomic::AtomicUsize::new(0), - ), - ); - } + let (online_counter, traffic_stats) = cfg + .users + .keys() + .map(|user| { + let counter = (user.to_owned(), std::sync::atomic::AtomicUsize::new(0)); + let stats = ( + user.to_owned(), + ( + std::sync::atomic::AtomicUsize::new(0), + std::sync::atomic::AtomicUsize::new(0), + ), + ); + (counter, stats) + }) + .unzip();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clash-lib/src/proxy/tuic/test_utils.rs` around lines 60 - 74, The code duplicates iteration over cfg.users to populate online_counter and traffic_stats; refactor by using a single loop over cfg.users and within that loop insert both online_counter.insert(user.to_owned(), AtomicUsize::new(0)) and traffic_stats.insert(user.to_owned(), (AtomicUsize::new(0), AtomicUsize::new(0))); reference the existing symbols online_counter, traffic_stats, and cfg.users and keep the same to_owned()/AtomicUsize::new(0) semantics to preserve behavior.clash-lib/src/proxy/tuic/mod.rs (1)
564-575: Arbitrary sleep makes test potentially slow and flaky.The 1-second sleep at line 567 is a fixed delay hoping the server processes auth rejection. This could be too short under load or unnecessarily slow in normal conditions.
Consider polling with a short timeout in a loop instead:
♻️ Suggested approach
if let Ok(mut stream) = result { let mut buf = [0u8; 5]; - // Give the server time to process auth and close - tokio::time::sleep(Duration::from_secs(1)).await; - let write_result = stream.write_all(b"hello").await; - let read_result = stream.read_exact(&mut buf).await; - assert!( - write_result.is_err() || read_result.is_err(), - "expected IO error after auth failure, but both read and write \ - succeeded" - ); + // Poll for failure with timeout instead of fixed sleep + let io_result = tokio::time::timeout(Duration::from_secs(5), async { + loop { + if stream.write_all(b"hello").await.is_err() { + return true; // write failed as expected + } + if stream.read_exact(&mut buf).await.is_err() { + return true; // read failed as expected + } + tokio::time::sleep(Duration::from_millis(50)).await; + } + }) + .await; + assert!( + io_result.is_ok(), + "expected IO error after auth failure within timeout" + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clash-lib/src/proxy/tuic/mod.rs` around lines 564 - 575, The fixed 1s sleep before testing stream IO makes the test slow and flaky; replace the tokio::time::sleep(Duration::from_secs(1)).await in the block that handles Ok(mut stream) (the variable result/stream used with stream.write_all and stream.read_exact) with a short polling loop that repeatedly attempts a non-blocking/read-write probe (call the same stream.write_all and stream.read_exact) until either you observe the expected IO error or a small overall timeout elapses (use tokio::time::timeout or loop with tokio::time::sleep(Duration::from_millis(...)) between probes); fail the test if the timeout elapses without an IO error and keep the existing assert semantics once an error is observed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@clash-lib/src/proxy/tuic/mod.rs`:
- Around line 564-575: The fixed 1s sleep before testing stream IO makes the
test slow and flaky; replace the
tokio::time::sleep(Duration::from_secs(1)).await in the block that handles
Ok(mut stream) (the variable result/stream used with stream.write_all and
stream.read_exact) with a short polling loop that repeatedly attempts a
non-blocking/read-write probe (call the same stream.write_all and
stream.read_exact) until either you observe the expected IO error or a small
overall timeout elapses (use tokio::time::timeout or loop with
tokio::time::sleep(Duration::from_millis(...)) between probes); fail the test if
the timeout elapses without an IO error and keep the existing assert semantics
once an error is observed.
In `@clash-lib/src/proxy/tuic/test_utils.rs`:
- Around line 60-74: The code duplicates iteration over cfg.users to populate
online_counter and traffic_stats; refactor by using a single loop over cfg.users
and within that loop insert both online_counter.insert(user.to_owned(),
AtomicUsize::new(0)) and traffic_stats.insert(user.to_owned(),
(AtomicUsize::new(0), AtomicUsize::new(0))); reference the existing symbols
online_counter, traffic_stats, and cfg.users and keep the same
to_owned()/AtomicUsize::new(0) semantics to preserve behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 14c9eb89-3b23-424a-aaa9-350e3f6cd16f
📒 Files selected for processing (2)
clash-lib/src/proxy/tuic/mod.rsclash-lib/src/proxy/tuic/test_utils.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
clash-lib/Cargo.toml (1)
176-176: Consider adding explicitrevvalues to TUIC git dependencies for self-contained manifests.Line 176 and Line 225 specify
tag = "v1.7.2"without explicitrevvalues. While Cargo.lock already pins the exact commit (18b74bcf11fe33caf9dcfc9e2d6685c5230a2e0a), addingrevdirectly in Cargo.toml makes the dependency specification more transparent and reduces reliance on Cargo.lock for reproducibility. This is especially valuable for supply-chain clarity in projects with frequent dependency updates.Also applies to: 225-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clash-lib/Cargo.toml` at line 176, The TUIC git dependency entries (e.g., the tuic-core dependency declaration) currently specify tag = "v1.7.2" without an explicit rev; update each TUIC git dependency in Cargo.toml (the tuic-core entry at the shown locations and the other TUIC git entry around the later occurrence) to include rev = "18b74bcf11fe33caf9dcfc9e2d6685c5230a2e0a" (the commit from Cargo.lock) alongside tag = "v1.7.2" so the manifest is self-contained and reproducible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@clash-lib/Cargo.toml`:
- Line 176: The TUIC git dependency entries (e.g., the tuic-core dependency
declaration) currently specify tag = "v1.7.2" without an explicit rev; update
each TUIC git dependency in Cargo.toml (the tuic-core entry at the shown
locations and the other TUIC git entry around the later occurrence) to include
rev = "18b74bcf11fe33caf9dcfc9e2d6685c5230a2e0a" (the commit from Cargo.lock)
alongside tag = "v1.7.2" so the manifest is self-contained and reproducible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b936b8f5-8796-4ac1-b55a-af719f3aaeb1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
clash-lib/Cargo.toml
| let server = TuicServerProcess::start().await?; | ||
| let port = server.port(); | ||
|
|
||
| // Start a local echo server as the target |
There was a problem hiding this comment.
any chance to have this reuse the existing test suites and echo servers so we don't need reinvent the wheels here
|
is it overall better to run the server like this than using a docker container? i think docker is doing it pretty well to avoid crafting the server logic manually for each outbounds, don't see much benefits here replacing it |
It doesn't run on all platform, such as windows. I think using dep could test more cases, not just on linux which is the only platform supplies docker |
|
ok that's a fair point. we should have macOS support after this #1039 maybe we should enable them all and fix Windows support too if not working already |
2 participants
Summary by CodeRabbit
Tests
Chores