feat: e2e throughput tests for SS outbound + transport Poll::Pending bug fixes#1332
Merged
Conversation
- Fix Poll::Pending state-loss in simple_obfs/http.rs (write + read) - Fix Poll::Pending state-loss in vmess/vmess_impl/http.rs (write + read, 0-capacity ReadBuf, wrong httparse::Request parser, zero header slots) - Add e2e throughput test harness in docker_utils (alloc_port, ThroughputResult, find_clash_rs_binary, socks5_connect, clash_process_e2e_throughput) - Add 5 parallel SS e2e tests covering plain, obfs-http, obfs-tls, v2ray-plugin WS+TLS, shadow-tls v3; gated by --cfg throughput_test - Write JSON-lines results to THROUGHPUT_RESULTS_FILE for CI collection - Add bench/format_throughput.py to render results as a Markdown table - Add .github/workflows/proxy-throughput.yml pipeline: builds release binary, runs tests with --test-threads=8, posts results as PR comment and GitHub Step Summary, uploads artifacts for 90 days Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a CI workflow and formatter, Docker-gated throughput e2e scaffolding and a Shadowsocks throughput test module, and refactors HTTP obfuscation streams to use internal buffering with correct partial-read/partial-write handling. Changes
Sequence DiagramsequenceDiagram
participant GHA as GitHub Actions
participant Runner as Runner (build/test)
participant Docker as Docker (images/containers)
participant ClashRS as clash-rs subprocess
participant SOCKS5 as SOCKS5 inbound
participant Echo as Echo server
participant Outbound as SS/Plugin outbound
participant Metrics as Throughput collector
participant Formatter as bench/format_throughput.py
participant PR as Pull Request
GHA->>Runner: Trigger workflow (PR / workflow_dispatch)
Runner->>Runner: Install toolchain & deps, build clash-rs (release)
Runner->>Docker: Pull required images
Runner->>Docker: Start SS/plugin containers (allocated ports)
Runner->>ClashRS: Spawn clash-rs with generated YAML
ClashRS->>SOCKS5: Accept connection
Runner->>Echo: Start echo server
SOCKS5->>Outbound: Dispatch to outbound plugin/server
Outbound->>Echo: Forward traffic
Echo->>Outbound: Reflect payload
Outbound->>Metrics: Return data (measure timings)
Metrics->>Runner: Write JSONL results
Runner->>Formatter: Run formatter -> produce throughput-comment.md
Runner->>PR: Post or update PR comment with formatted results
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.github/workflows/proxy-throughput.yml (1)
126-129: Bot comment detection may be fragile.The detection relies on
c.user.type === 'Bot'and body containing "Proxy Throughput Results". Consider also checkingc.user.loginincludes "github-actions" for more robust detection.Suggested improvement
const existing = comments.find(c => - c.user.type === 'Bot' && + c.user?.type === 'Bot' && + c.user?.login?.includes('github-actions') && c.body.includes('Proxy Throughput Results') );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/proxy-throughput.yml around lines 126 - 129, The bot comment detection in the comments.find predicate is fragile because it only checks c.user.type === 'Bot' and body text; update the predicate used when assigning existing to also verify the comment author's login contains "github-actions" (e.g., check c.user.login && c.user.login.includes('github-actions')) in addition to the existing body.includes('Proxy Throughput Results') check so the search reliably identifies the GitHub Actions bot comment; modify the predicate inside the comments.find call (where existing is declared) to include this additional login check.bench/format_throughput.py (1)
8-11: Minor: Replace en-dash (–) with hyphen-minus (-) in docstring.The docstring uses Unicode en-dash characters which Ruff flags as ambiguous. While this doesn't affect functionality, using standard ASCII hyphens improves consistency.
Suggested fix
- label – human-readable test name - upload_mbps – upload throughput in Mbps - download_mbps – download throughput in Mbps - total_bytes – payload size in bytes + label - human-readable test name + upload_mbps - upload throughput in Mbps + download_mbps - download throughput in Mbps + total_bytes - payload size in bytes🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bench/format_throughput.py` around lines 8 - 11, The docstring lines defining "label", "upload_mbps", "download_mbps", and "total_bytes" use Unicode en-dashes (–); replace each en-dash with the ASCII hyphen-minus (-) so the descriptions read "label - human-readable test name", "upload_mbps - upload throughput in Mbps", "download_mbps - download throughput in Mbps", and "total_bytes - payload size in bytes" to remove the ambiguous character flagged by linters.clash-lib/src/proxy/utils/test_utils/docker_utils/mod.rs (1)
67-82: Consider preferring release build for throughput tests.The function prefers debug build "for faster iteration", but throughput tests are run in release mode by the CI workflow. For consistency, consider preferring release build first in throughput test contexts, or making the preference configurable.
However, this is a minor concern since the workflow explicitly builds
--releasebefore running tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clash-lib/src/proxy/utils/test_utils/docker_utils/mod.rs` around lines 67 - 82, The current find_clash_rs_binary function prefers the debug binary (variables debug/release) which can cause inconsistency with CI throughput tests; update find_clash_rs_binary in clash-lib/src/proxy/utils/test_utils/docker_utils/mod.rs to prefer the release build first (check release.exists() before debug.exists()) or make the preference configurable via an environment variable (e.g., CLASHRS_PREFER_RELEASE) so the function (using config_helper::root_dir(), and variables debug/release) will select release when requested.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/proxy-throughput.yml:
- Around line 126-129: The bot comment detection in the comments.find predicate
is fragile because it only checks c.user.type === 'Bot' and body text; update
the predicate used when assigning existing to also verify the comment author's
login contains "github-actions" (e.g., check c.user.login &&
c.user.login.includes('github-actions')) in addition to the existing
body.includes('Proxy Throughput Results') check so the search reliably
identifies the GitHub Actions bot comment; modify the predicate inside the
comments.find call (where existing is declared) to include this additional login
check.
In `@bench/format_throughput.py`:
- Around line 8-11: The docstring lines defining "label", "upload_mbps",
"download_mbps", and "total_bytes" use Unicode en-dashes (–); replace each
en-dash with the ASCII hyphen-minus (-) so the descriptions read "label -
human-readable test name", "upload_mbps - upload throughput in Mbps",
"download_mbps - download throughput in Mbps", and "total_bytes - payload size
in bytes" to remove the ambiguous character flagged by linters.
In `@clash-lib/src/proxy/utils/test_utils/docker_utils/mod.rs`:
- Around line 67-82: The current find_clash_rs_binary function prefers the debug
binary (variables debug/release) which can cause inconsistency with CI
throughput tests; update find_clash_rs_binary in
clash-lib/src/proxy/utils/test_utils/docker_utils/mod.rs to prefer the release
build first (check release.exists() before debug.exists()) or make the
preference configurable via an environment variable (e.g.,
CLASHRS_PREFER_RELEASE) so the function (using config_helper::root_dir(), and
variables debug/release) will select release when requested.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cbdd5214-ef28-4b5d-acce-11260eacd168
📒 Files selected for processing (6)
.github/workflows/proxy-throughput.ymlbench/format_throughput.pyclash-lib/src/proxy/shadowsocks/outbound/mod.rsclash-lib/src/proxy/transport/simple_obfs/http.rsclash-lib/src/proxy/utils/test_utils/docker_utils/mod.rsclash-lib/src/proxy/vmess/vmess_impl/http.rs
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Gate NEXT_PORT/alloc_port/ThroughputResult/find_clash_rs_binary under #[cfg(throughput_test)] to avoid dead_code errors in normal builds - Wrap the full upload+download in the destination retry loop so that EPIPE/broken-pipe during data transfer tries the next destination (previously only socks5_connect errors triggered a retry) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ut tests - Add .port(obfs_port) to get_obfs_runner() to avoid defaulting to the hardcoded PORT=10002, which caused conflicts when two obfs tests ran in parallel - Add .port(stls_port) to get_shadowtls_runner() for the same reason - Fix container cleanup leak in two-container tests (obfs-http, obfs-tls, shadowtls): if the second container fails to start, the first container was dropped without async cleanup; use an explicit match + c1.cleanup().await.ok() to guarantee cleanup on that path Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
clash-lib/src/proxy/shadowsocks/outbound/mod.rs (2)
578-578: UseSHADOW_TLS_PASSWORDin generated plugin YAML to prevent drift.Line 924 hardcodes
"password"while a shared constant already exists at Line 578.Proposed fix
- let plugin_yaml = r#" plugin: shadow-tls + let plugin_yaml = format!(r#" plugin: shadow-tls plugin-opts: host: www.feishu.cn - password: password - version: 3"#; - let config = ss_base_config(&server, stls_port, socks_port, plugin_yaml); + password: {} + version: 3"#, SHADOW_TLS_PASSWORD); + let config = + ss_base_config(&server, stls_port, socks_port, plugin_yaml.as_str());Also applies to: 921-925
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clash-lib/src/proxy/shadowsocks/outbound/mod.rs` at line 578, The generated plugin YAML currently hardcodes the string "password" (around the plugin YAML construction at lines ~921-925); replace that literal with the shared constant SHADOW_TLS_PASSWORD (declared at top of file) so the plugin YAML uses SHADOW_TLS_PASSWORD instead of a duplicated literal, updating the YAML builder/formatting code that emits the password field to reference the constant.
779-780: Don’t silently swallow container cleanup failures.Using
.ok()here hides cleanup errors; if cleanup fails, later tests may flake due to leaked containers/ports. At least log the error.Proposed fix pattern
- c1.cleanup().await.ok(); + if let Err(cleanup_err) = c1.cleanup().await { + tracing::warn!( + "failed to cleanup first container after secondary startup failure: {cleanup_err}" + ); + }Also applies to: 829-830, 914-915
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clash-lib/src/proxy/shadowsocks/outbound/mod.rs` around lines 779 - 780, The cleanup futures for the container (calls like c1.cleanup().await.ok()) are currently swallowing errors; change each to capture the Result and log failures before returning (e.g., replace c1.cleanup().await.ok() with something like if let Err(clean_err) = c1.cleanup().await { log::warn!("container cleanup failed: {:?}", clean_err); } ), and apply the same change to the other occurrences (the blocks that return Err(e) after calling cleanup). Reference the c1.cleanup().await calls and the surrounding error-return paths so each cleanup error is logged prior to returning Err(e).
🤖 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/shadowsocks/outbound/mod.rs`:
- Around line 699-703: The code uses to_str().unwrap() when building mmdb from
config_helper::test_config_base_dir().join("Country.mmdb"), which can panic on
non-UTF8 paths; change this to a non-panicking conversion (e.g., use
to_string_lossy().into_owned() or explicitly handle the Option from to_str() and
return a Result/test failure) so mmdb is created safely without unwrap; update
the mmdb binding construction accordingly to avoid panics in tests.
---
Nitpick comments:
In `@clash-lib/src/proxy/shadowsocks/outbound/mod.rs`:
- Line 578: The generated plugin YAML currently hardcodes the string "password"
(around the plugin YAML construction at lines ~921-925); replace that literal
with the shared constant SHADOW_TLS_PASSWORD (declared at top of file) so the
plugin YAML uses SHADOW_TLS_PASSWORD instead of a duplicated literal, updating
the YAML builder/formatting code that emits the password field to reference the
constant.
- Around line 779-780: The cleanup futures for the container (calls like
c1.cleanup().await.ok()) are currently swallowing errors; change each to capture
the Result and log failures before returning (e.g., replace
c1.cleanup().await.ok() with something like if let Err(clean_err) =
c1.cleanup().await { log::warn!("container cleanup failed: {:?}", clean_err); }
), and apply the same change to the other occurrences (the blocks that return
Err(e) after calling cleanup). Reference the c1.cleanup().await calls and the
surrounding error-return paths so each cleanup error is logged prior to
returning Err(e).
🪄 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: c0ad62c2-83cc-40fe-b064-23499b7b81ba
📒 Files selected for processing (1)
clash-lib/src/proxy/shadowsocks/outbound/mod.rs
Comment on lines
+699
to
+703
| let mmdb = config_helper::test_config_base_dir() | ||
| .join("Country.mmdb") | ||
| .to_str() | ||
| .unwrap() | ||
| .to_owned(); |
Contributor
There was a problem hiding this comment.
Avoid panic in test config path conversion.
Line 702 uses to_str().unwrap(), which can panic on non-UTF8 paths and fail the suite before test logic runs.
Proposed fix
- let mmdb = config_helper::test_config_base_dir()
- .join("Country.mmdb")
- .to_str()
- .unwrap()
- .to_owned();
+ let mmdb = config_helper::test_config_base_dir()
+ .join("Country.mmdb")
+ .to_string_lossy()
+ .into_owned();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clash-lib/src/proxy/shadowsocks/outbound/mod.rs` around lines 699 - 703, The
code uses to_str().unwrap() when building mmdb from
config_helper::test_config_base_dir().join("Country.mmdb"), which can panic on
non-UTF8 paths; change this to a non-panicking conversion (e.g., use
to_string_lossy().into_owned() or explicitly handle the Option from to_str() and
return a Result/test failure) so mmdb is created safely without unwrap; update
the mmdb binding construction accordingly to avoid panics in tests.
Contributor
🚀 TUN Throughput Benchmark Results📊 Test Environment
Current PR Performance
Comparison with Baseline (master)
Direct vs TUN Comparison (Local)
🤖 Benchmark run on GitHub Actions CI • |
Contributor
📊 Proxy Throughput Results
Tests ran 5 variant(s) in parallel; each direction transfers the full payload. Full test logDownload the |
…binding conflicts E2e tests connect to containers via their internal docker bridge IP, not via localhost port mappings. Binding host ports in the macOS ephemeral range (49152-65535) caused EADDRINUSE when Docker tried to reserve those ports and the OS already had connections using them. Add DockerTestRunnerBuilder::no_port() which clears all host port bindings. Use it in all four e2e container builders (get_ss_runner, get_ss_runner_with_plugin, get_shadowtls_runner, get_obfs_runner) so Docker never touches a host port for these containers. Also simplify the obfs/shadowtls builders: the .port(obfs_port) / .port(stls_port) introduced in the previous commit was a partial fix (unique ports instead of colliding on 10002) but still hit the ephemeral-range problem; no_port() is the correct solution. Verified: all 5 e2e throughput tests pass with --test-threads=8. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
clash-lib/src/proxy/shadowsocks/outbound/mod.rs (1)
619-627:⚠️ Potential issue | 🟡 MinorAvoid panics from
to_str().unwrap()in test path handling.This can panic on non-UTF8 paths (cert/key mounts and
mmdbpath). The same issue at Line 699–Line 703 was already reported previously and is still present.Proposed fix
@@ - let cert = test_config_dir.join("example.org.pem"); - let key = test_config_dir.join("example.org-key.pem"); + let cert = test_config_dir.join("example.org.pem"); + let key = test_config_dir.join("example.org-key.pem"); + let cert = cert.to_string_lossy().into_owned(); + let key = key.to_string_lossy().into_owned(); @@ - cert.to_str().unwrap(), + cert.as_str(), @@ - key.to_str().unwrap(), + key.as_str(), @@ - let mmdb = config_helper::test_config_base_dir() - .join("Country.mmdb") - .to_str() - .unwrap() - .to_owned(); + let mmdb = config_helper::test_config_base_dir() + .join("Country.mmdb") + .to_string_lossy() + .into_owned();#!/bin/bash # Verify remaining panicking path conversions in this module. rg -n 'to_str\(\)\.unwrap\(\)' clash-lib/src/proxy/shadowsocks/outbound/mod.rsAlso applies to: 699-703
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clash-lib/src/proxy/shadowsocks/outbound/mod.rs` around lines 619 - 627, The code currently calls to_str().unwrap() on Path/OsString variables (cert, key, and mmdb) which can panic for non-UTF8 paths; replace those calls with a non-panicking conversion such as cert.to_string_lossy().into_owned() and key.to_string_lossy().into_owned() (and mmdb.to_string_lossy().into_owned()) where the mounts are built (the tuples using cert and key in the mounts array and the other mmdb usage around the second occurrence at lines ~699–703) so the test path handling won’t panic on non-UTF8 file paths.
🤖 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/shadowsocks/outbound/mod.rs`:
- Around line 638-642: When topology is the "no_port" variant, do not fall back
to "host.docker.internal" for ss_ip; instead detect ss_ip.is_none() and return
an explicit error before constructing ss_server_env (the code that currently
does let ss_server_env = format!("SERVER={}:{}",
ss_ip.unwrap_or("host.docker.internal".to_owned()), ss_port))). Replace the
unwrap_or fallback with an early Err when topology indicates .no_port() and
ss_ip is None, and apply the same change to the analogous block around the
ss_server_env construction at lines 666-670 so both container-to-container
no_port code paths fail fast with a clear error.
---
Duplicate comments:
In `@clash-lib/src/proxy/shadowsocks/outbound/mod.rs`:
- Around line 619-627: The code currently calls to_str().unwrap() on
Path/OsString variables (cert, key, and mmdb) which can panic for non-UTF8
paths; replace those calls with a non-panicking conversion such as
cert.to_string_lossy().into_owned() and key.to_string_lossy().into_owned() (and
mmdb.to_string_lossy().into_owned()) where the mounts are built (the tuples
using cert and key in the mounts array and the other mmdb usage around the
second occurrence at lines ~699–703) so the test path handling won’t panic on
non-UTF8 file paths.
🪄 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: 6af2fe0f-a6f0-4035-99f0-aa7ff3b8af99
📒 Files selected for processing (2)
clash-lib/src/proxy/shadowsocks/outbound/mod.rsclash-lib/src/proxy/utils/test_utils/docker_utils/docker_runner.rs
Comment on lines
+638
to
+642
| let ss_server_env = format!( | ||
| "SERVER={}:{}", | ||
| ss_ip.unwrap_or("host.docker.internal".to_owned()), | ||
| ss_port | ||
| ); |
Contributor
There was a problem hiding this comment.
Fail fast if SS container IP is missing under no_port() topology.
With .no_port(), the fallback to host.docker.internal:{ss_port} is not a valid route for these container-to-container links. Returning an explicit error here makes failures deterministic.
Proposed fix
@@
- let ss_server_env = format!(
- "SERVER={}:{}",
- ss_ip.unwrap_or("host.docker.internal".to_owned()),
- ss_port
- );
+ let ss_ip = ss_ip
+ .ok_or_else(|| anyhow::anyhow!("missing shadowsocks container IP"))?;
+ let ss_server_env = format!("SERVER={ss_ip}:{ss_port}");
@@
- let ss_server_env = format!(
- "{}:{}",
- ss_ip.unwrap_or("host.docker.internal".to_owned()),
- ss_port
- );
+ let ss_ip = ss_ip
+ .ok_or_else(|| anyhow::anyhow!("missing shadowsocks container IP"))?;
+ let ss_server_env = format!("{ss_ip}:{ss_port}");Also applies to: 666-670
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clash-lib/src/proxy/shadowsocks/outbound/mod.rs` around lines 638 - 642, When
topology is the "no_port" variant, do not fall back to "host.docker.internal"
for ss_ip; instead detect ss_ip.is_none() and return an explicit error before
constructing ss_server_env (the code that currently does let ss_server_env =
format!("SERVER={}:{}", ss_ip.unwrap_or("host.docker.internal".to_owned()),
ss_port))). Replace the unwrap_or fallback with an early Err when topology
indicates .no_port() and ss_ip is None, and apply the same change to the
analogous block around the ss_server_env construction at lines 666-670 so both
container-to-container no_port code paths fail fast with a clear error.
Both functions are only called from clash_process_e2e_throughput which requires #[cfg(all(docker_test, throughput_test))]. Using only #[cfg(docker_test)] caused dead_code errors when building without throughput_test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
- TIMEOUT_DURATION 30 → 120s: 2-core CI runners are slow; clash-rs startup alone can take 10-20s, leaving no time for the transfer - wait_for_port socks_port: 30 → 60s to match extended budget - clash-rs child stdio: null → inherit so crash logs are visible - proxy-throughput.yml: remove continue-on-error: true - proxy-throughput.yml: add set -o pipefail so `| tee` does not swallow cargo test exit code (the real false-positive cause) - Post PR comment step: if: always() so it runs even when tests fail Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- V2RayOBFSOption: make 'port' in plugin-opts optional; real Clash configs omit it (only the main proxy port matters for the TCP connection). Default to 443 when tls=true, 80 otherwise — the value is only used for the WebSocket Upgrade Host header. - proxy-throughput.yml: pre-download Country.mmdb before running tests so all 5 parallel clash-rs instances find the file already present and skip the concurrent-download race that was corrupting the file or causing startup failures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…e binary The telemetry feature (included via --all-features) starts console_subscriber and OpenTelemetry threads that deadlock with the main thread over a mutex during crypto initialization. When multiple clash-rs instances start concurrently this manifests as a 60 s+ startup hang, causing every parallel test to time out. Without telemetry the binary starts in < 1 s. Default features already include shadowsocks, tuic, wireguard, ssh, and everything else needed for these tests. Also flip find_clash_rs_binary() to prefer the release binary over debug: release is faster (important for throughput numbers) and is less likely to have been built with --all-features. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
cargo test -p clash-lib sets the test binary's CWD to clash-lib/,
so writing to the relative path 'throughput-results.json' created
the file at clash-lib/throughput-results.json. The format/upload
steps looked in the workspace root, so the file was never found.
Fix by setting THROUGHPUT_RESULTS_FILE to ${{ github.workspace }}/...
(absolute path) in the Run throughput tests step, and updating the
Format and Upload steps to use the same absolute path.
Also add #[allow(dead_code)] to the socks5_connect and wait_for_port
helpers to silence the dead_code lint when both docker_test and
throughput_test cfg flags are active.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add 'Print environment info' step showing OS, CPUs, memory, disk, Docker version, and Rust version before running tests - Echo the absolute results file path at the start of the test step so it's visible in the CI log (cargo test -p clash-lib uses clash-lib/ as CWD; THROUGHPUT_RESULTS_FILE is now absolute to avoid writing to the wrong directory) - Cross.toml passthrough not needed: throughput tests run natively via cargo test, not via cross Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use ${{ github.workspace }}/throughput-results.json at the workflow
level so the absolute path is declared once and reused everywhere,
instead of re-constructing it from a basename variable at each step.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
4 tasks
6 tasks
This was referenced Apr 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bug fixes (spotted by reviewing #1330 pattern)
simple_obfs/http.rs: Fixed twoPoll::Pendingstate-loss bugs:poll_write: HTTP upgrade header was rebuilt on every call — lost if inner write returnedPendingpoll_read: Partial HTTP response header accumulated in a stack-local buffer — lost onPending, causing parse errors on fragmented responsesvmess/vmess_impl/http.rs: Fixed four bugs in the HTTP transport:poll_write: Same header rebuild issue as abovepoll_read: Localheader_buflost onPending;BytesMutwith len=0 → 0-capacityReadBuf(inner stream could never deliver bytes);httparse::Requestused instead ofhttparse::Response; zero header slots allocatedE2E throughput test harness
Full-stack tests: test client → SOCKS5 inbound → dispatcher → SS outbound → docker proxy container → local echo server → back. 32 MB payload each direction.
5 parallel tests (no
#[serial_test::serial]):e2e_throughput_ss_plaine2e_throughput_ss_obfs_httpe2e_throughput_ss_obfs_tlse2e_throughput_ss_v2ray_plugine2e_throughput_ss_shadowtlsPorts allocated via a global
AtomicU16counter (starting at 52000) — no port collisions possible.CI pipeline (
.github/workflows/proxy-throughput.yml)--cfg docker_test --cfg throughput_test(separate from regular docker tests)clash-lib/src/proxy/**or manual dispatchTHROUGHPUT_RESULTS_FILE, formatted as a Markdown table bybench/format_throughput.py$GITHUB_STEP_SUMMARYRun locally
Summary by CodeRabbit
Tests
Bug Fixes
Chores
New Features