Add end-to-end HTTP/3 support and prepare v0.1.3-rc.10#40
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughSummary by CodeRabbit
Walkthrough该 PR 为 rginx 引入 HTTP/3（QUIC）端到端支持：增加配置模型、编译/验证、上游/下游 HTTP/3 客户端与服务器、TLS/QUIC 集成、监听器绑定与继承、Admin 快照/状态扩展，以及大量 E2E/单元测试与 CI/脚本调整（含 Clippy gate 脚本）。 Changes
Sequence DiagramsequenceDiagram
autonumber
participant Client as Client (HTTP/3)
participant QUIC as QUIC Endpoint / Listener
participant Handler as rginx Handler
participant Proxy as ProxyClient (HTTP / HTTP3)
participant Upstream as Upstream Server
Client->>QUIC: QUIC + TLS1.3 握手，H3 请求
QUIC->>Handler: 转换为 Request<HttpBody>
Handler->>Handler: 计算 alt_svc_header_value(listener, version)
alt route is Return
Handler->>Client: Response + Alt‑Svc header（若启用）
else route is Proxy
Handler->>Proxy: request(upstream, peer, Request<HttpBody>)
alt upstream protocol == http3
Proxy->>Proxy: build_http3_client_config()
Proxy->>Upstream: QUIC connect (SNI/mTLS) + H3 request
Upstream->>Proxy: H3 response headers/body/trailers
Proxy->>Handler: Response<HttpBody>（BufferedResponseBody）
else upstream protocol == http1/http2
Proxy->>Upstream: HTTP/1.x 或 HTTP/2 请求
Upstream->>Proxy: response
Proxy->>Handler: Response<HttpBody>
end
Handler->>Handler: build_downstream_response(Response<HttpBody>, alt_svc)
Handler->>Client: 下游响应（含可选 Alt‑Svc 注入）
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Pull request overview
This pull request completes end-to-end HTTP/3 enablement across rginx (config/compile/validate, runtime bootstrap & restart inheritance, downstream accept loop, upstream proxy client, admin/check/status snapshots), wires HTTP/3 into release/CI gates, and prepares the v0.1.3-rc.10 release.
Changes:
- Add downstream HTTP/3 server support (QUIC endpoint binding + accept/serve loop) and HTTP/3-aware listener bindings/snapshots.
- Add upstream HTTP/3 proxy support (new
UpstreamProtocol::Http3+ QUIC client plumbing) and expand reload/restart + drain test coverage. - Update release/CI tooling (clippy gate script, soak/slow/tls gates, benchmark matrix) and bump workspace/versioned docs to
0.1.3-rc.10.
Reviewed changes
Copilot reviewed 88 out of 89 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/test-slow.sh | Run integration tests sequentially and enforce presence of required HTTP/3 gate targets. |
| scripts/test-fast.sh | Convert fast tests into a labeled matrix loop + keep rginx bin unit tests. |
| scripts/run-tls-gate.sh | Add HTTP/3 downstream/upstream/gRPC tests and normalize ROOT_DIR handling. |
| scripts/run-soak.sh | Add HTTP/3 scenarios to the soak matrix. |
| scripts/run-clippy-gate.sh | New clippy gate script used by CI/release workflows. |
| scripts/run-benchmark-matrix.py | Add HTTP/3 and gRPC-over-HTTP/3 benchmark scenario flags. |
| scripts/prepare-release.sh | Use the new clippy gate script in the release pipeline. |
| scripts/nginx_compare/scenarios.py | Add rginx-only HTTP/3 return scenario and mark nginx as unsupported for HTTP/3. |
| scripts/nginx_compare/render.py | Update report text to include HTTP/3 benchmarking context. |
| scripts/nginx_compare/configs.py | Add an HTTP/3-enabled rginx return config generator. |
| README.md | Document HTTP/3 support surface, gates, and update version to v0.1.3-rc.10. |
| crates/rginx-runtime/src/restart.rs | Extend restart socket inheritance to include UDP sockets for HTTP/3 listeners. |
| crates/rginx-runtime/src/ocsp.rs | Add/expand OCSP unit tests (HTTP responder, cache behaviors). |
| crates/rginx-runtime/src/health.rs | Update test fixtures for new listener HTTP/3 field. |
| crates/rginx-runtime/src/bootstrap/shutdown.rs | Add shutdown unit tests around draining/timeout behavior. |
| crates/rginx-runtime/src/bootstrap/reload.rs | Pass full next config into listener-binding preparation. |
| crates/rginx-runtime/src/bootstrap/mod.rs | Update initial listener group bootstrap to take full config and inherited sockets. |
| crates/rginx-runtime/src/bootstrap/listeners.rs | Add UDP+HTTP/3 endpoint handling in listener worker groups (bind/activate/reload checks). |
| crates/rginx-runtime/src/admin.rs | Bump admin snapshot schema version to 12. |
| crates/rginx-runtime/Cargo.toml | Add quinn dependency for HTTP/3 runtime integration. |
| crates/rginx-http/src/transition.rs | Extend reload/restart boundary reporting for HTTP/3 listener fields. |
| crates/rginx-http/src/tls/mod.rs | Re-export HTTP/3 server TLS config builder. |
| crates/rginx-http/src/tls/certificates.rs | Switch PEM parsing to rustls pki-types iterators and improve parse error mapping. |
| crates/rginx-http/src/tls/acceptor.rs | Add HTTP/3-only server TLS config builder (TLS1.3 + ALPN h3 constraints). |
| crates/rginx-http/src/state/tls_runtime/listeners.rs | Include HTTP/3 TLS/listener metadata in TLS status snapshots. |
| crates/rginx-http/src/state/tls_runtime/bindings.rs | Update test fixtures for new listener HTTP/3 field. |
| crates/rginx-http/src/state/tests.rs | Add snapshot tests for HTTP/3 bindings and snapshot-delta module filtering. |
| crates/rginx-http/src/state/snapshots.rs | Add binding-level snapshot types and HTTP/3 fields to runtime/TLS listener snapshots. |
| crates/rginx-http/src/state/lifecycle.rs | Populate new binding snapshot fields from listener transport bindings. |
| crates/rginx-http/src/server/tests.rs | Update test PEM parsing to rustls pki-types iterators. |
| crates/rginx-http/src/server/mod.rs | Introduce HTTP/3 server module and exports. |
| crates/rginx-http/src/server/http3.rs | New downstream HTTP/3 server implementation (endpoint binding, accept loop, request/response bridging). |
| crates/rginx-http/src/server/graceful.rs | Normalize request body type to HttpBody by boxing incoming bodies for handler. |
| crates/rginx-http/src/proxy/tests/mod.rs | Update listener fixtures for new HTTP/3 field. |
| crates/rginx-http/src/proxy/tests/client_profiles.rs | Extend upstream protocol tests to include HTTP/3 and client cache partitioning. |
| crates/rginx-http/src/proxy/request_body.rs | Switch downstream request body handling to unified HttpBody. |
| crates/rginx-http/src/proxy/mod.rs | Adjust proxy exports (keep ProxyClients public). |
| crates/rginx-http/src/proxy/health/registry.rs | Update listener fixtures for new HTTP/3 field. |
| crates/rginx-http/src/proxy/health.rs | Route upstream requests through protocol-aware ProxyClient::request. |
| crates/rginx-http/src/proxy/forward/response.rs | Switch upstream response body type to unified HttpBody. |
| crates/rginx-http/src/proxy/forward/mod.rs | Block upgrade requests to HTTP/3 upstreams; route requests through protocol-aware client. |
| crates/rginx-http/src/proxy/common.rs | Map UpstreamProtocol::Http3 to HTTP/3 request version. |
| crates/rginx-http/src/proxy/clients/tls.rs | Add QUIC/TLS client config builder for upstream HTTP/3 (TLS1.3 + ALPN h3). |
| crates/rginx-http/src/proxy/clients/tests.rs | Update listener fixtures for new HTTP/3 field. |
| crates/rginx-http/src/proxy/clients/mod.rs | Introduce protocol-aware ProxyClient enum (HTTP vs HTTP/3) and selection/build logic. |
| crates/rginx-http/src/proxy/clients/http3.rs | New upstream HTTP/3 client using Quinn + h3. |
| crates/rginx-http/src/pki/certificate.rs | Switch certificate chain PEM parsing to rustls pki-types iterators. |
| crates/rginx-http/src/lib.rs | Re-export new runtime listener binding snapshot type. |
| crates/rginx-http/src/handler/tests.rs | Add Alt-Svc injection test + update calls for new finalize signature. |
| crates/rginx-http/src/handler/mod.rs | Introduce boxed_body helper and migrate handler to unified HttpBody. |
| crates/rginx-http/src/handler/dispatch.rs | Inject Alt-Svc headers for eligible HTTPS requests; migrate to unified HttpBody. |
| crates/rginx-http/Cargo.toml | Add HTTP/3 deps (h3, h3-quinn, quinn) and remove unused deps. |
| crates/rginx-core/src/lib.rs | Export new listener binding-related types. |
| crates/rginx-core/src/config/upstream.rs | Add UpstreamProtocol::Http3. |
| crates/rginx-core/src/config/tests.rs | Add tests for listener binding inventory and HTTP/3 binding details. |
| crates/rginx-core/src/config.rs | Add listener HTTP/3 config + transport binding inventory helpers/types. |
| crates/rginx-config/src/validate/upstream.rs | Enforce HTTPS-only peers for HTTP/3 upstream protocol. |
| crates/rginx-config/src/validate/tests.rs | Add validation tests for HTTP/3 listener TLS requirement + HTTP/3 upstream HTTPS requirement. |
| crates/rginx-config/src/validate/server.rs | Add HTTP/3 listener validation and TCP/UDP bind de-duplication. |
| crates/rginx-config/src/model.rs | Add HTTP/3 config schema and UpstreamProtocolConfig::Http3. |
| crates/rginx-config/src/compile/upstream.rs | Compile HTTP/3 upstream protocol with HTTPS-only guard. |
| crates/rginx-config/src/compile/tests.rs | Add compile coverage for HTTP/3 upstreams and HTTP/3 listener defaults. |
| crates/rginx-config/src/compile/server.rs | Compile listener HTTP/3 metadata into core snapshot (default listen + Alt-Svc defaults). |
| crates/rginx-app/tests/upstream_server_name.rs | Update test PEM parsing via rustls pki-types. |
| crates/rginx-app/tests/upstream_mtls.rs | Update test PEM parsing via rustls pki-types. |
| crates/rginx-app/tests/upstream_http3.rs | New integration tests for proxying to HTTP/3 upstreams (incl. mTLS + SNI override). |
| crates/rginx-app/tests/upstream_http2.rs | Update test PEM parsing via rustls pki-types. |
| crates/rginx-app/tests/reload/restart_flow.rs | Add restart regression coverage for HTTP/3 listener availability. |
| crates/rginx-app/tests/reload/reload_flow.rs | Add drain regression coverage for removing an HTTP/3 listener on reload. |
| crates/rginx-app/tests/ocsp.rs | Add integration tests for OCSP cache retention and reload recovery paths. |
| crates/rginx-app/tests/grpc_proxy.rs | Update test PEM parsing via rustls pki-types. |
| crates/rginx-app/tests/downstream_mtls.rs | Update test PEM parsing via rustls pki-types. |
| crates/rginx-app/tests/check.rs | Update check output assertions for new reload boundary fields + default config changes; add HTTP/3 listener binding coverage. |
| crates/rginx-app/tests/admin/snapshot.rs | Update schema version assertions and add HTTP/3 binding snapshot coverage. |
| crates/rginx-app/tests/admin/commands.rs | Add status command coverage for HTTP/3 listener bindings. |
| crates/rginx-app/src/main.rs | Extend check output to include listener binding inventory and HTTP/3/TLS listener details. |
| crates/rginx-app/src/admin_cli/status.rs | Extend status CLI output to include listener binding details and HTTP/3 state. |
| crates/rginx-app/Cargo.toml | Add HTTP/3 deps for integration tests; remove unused http dep. |
| configs/rginx.ron | Make default install config self-contained (no upstream), return a welcome page. |
| configs/conf.d/default.ron | Make default vhost config self-contained (Return handler) and clarify proxy example location. |
| clippy.toml | Add explicit MSRV for clippy baseline. |
| Cargo.toml | Bump workspace version to 0.1.3-rc.10 and add workspace deps for HTTP/3 (h3/h3-quinn/quinn). |
| Cargo.lock | Lock new HTTP/3 and Quinn dependency graph + bump crate versions. |
| ARCHITECTURE_HTTP3_PHASE0_FREEZE.md | Add Phase 0 HTTP/3 semantics freeze document (dated 2026-04-12). |
| .github/workflows/release.yml | Switch clippy step to run the clippy gate script. |
| .github/workflows/ci.yml | Add clippy as an explicit dependency and use the clippy gate script. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
.github/workflows/release.yml (1)
123-124: 建议将 Clippy Gate 前置到慢测之前以更快失败。当前命令本身没问题；但可把该步骤提前到 fast tests 后，减少无效慢测开销并与
prepare-release.sh顺序保持一致。♻️ 建议调整步骤顺序（示意）
- name: Run Fast Test Layer run: ./scripts/test-fast.sh + - name: Run Clippy + run: ./scripts/run-clippy-gate.sh + - name: Run Slow Test Layer run: ./scripts/test-slow.sh - name: Run TLS Release Gate run: ./scripts/run-tls-gate.sh - - - name: Run Clippy - run: ./scripts/run-clippy-gate.sh🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 123 - 124, 将名为 "Run Clippy" 的步骤（当前调用 ./scripts/run-clippy-gate.sh）从其当前位置提前：把它放在 fast tests 之后、slow tests 之前，以便更快失败并减少无谓的慢测开销，同时使步骤顺序与 prepare-release.sh 保持一致；修改 release.yml 中该 job/step 的顺序即可，确保依赖关系（如果有）仍然满足。scripts/nginx_compare/configs.py (1)
132-176: 建议抽取公共返回配置模板，减少重复维护
rginx_http3_return_config与rginx_tls_return_config的主体几乎一致，后续改路由或 runtime 参数时容易出现不一致。建议把公共骨架抽到一个内部 helper，仅注入server扩展段。可选重构示例
+def _rginx_return_config_with_server_extra(port: int, server_extra: str) -> str: + return textwrap.dedent( + f"""\ + Config( + runtime: RuntimeConfig( + shutdown_timeout_secs: 5, + worker_threads: Some({BENCHMARK_WORKERS}), + accept_workers: Some({BENCHMARK_WORKERS}), + ), + server: ServerConfig( + listen: "127.0.0.1:{port}", + keep_alive: Some(true), +{server_extra} + ), + upstreams: [], + locations: [ + LocationConfig( + matcher: Exact("/-/ready"), + handler: Return(status: 200, location: "", body: Some("ready\\n")), + ), + LocationConfig( + matcher: Exact("/"), + handler: Return(status: 200, location: "", body: Some("ok\\n")), + ), + ], + servers: [], + ) + """ + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/nginx_compare/configs.py` around lines 132 - 176, Both rginx_http3_return_config and rginx_tls_return_config share nearly identical config bodies; extract the common template into a helper (e.g., _rginx_base_return_config) that builds the shared runtime, upstreams, locations, and servers, and accepts a parameter for the server-specific block (the TLS/http3 server stanza). Update rginx_http3_return_config and rginx_tls_return_config to call this helper, passing their respective server extension (tls/http3) strings (or a small builder function) so future changes to runtime/routes are maintained in one place.crates/rginx-runtime/src/ocsp.rs (1)
429-445: 测试 responder 线程建议显式回收，降低偶发卡住风险当前
spawn_http_responder启动后不返回JoinHandle；若请求在连接前失败，线程可能在accept()持续阻塞。建议返回句柄并在测试末尾 join（或引入超时退出）以提升测试稳定性。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-runtime/src/ocsp.rs` around lines 429 - 445, spawn_http_responder currently spawns a thread with thread::spawn and never returns its JoinHandle, leaving the responder possibly blocked in accept() and causing flaky tests; modify spawn_http_responder to return the thread::JoinHandle (or a wrapper that can be joined) and propagate that handle to the caller (tests) so they can join() the thread (or use a timeout/wakeup) at test teardown, and update callers to join the handle to ensure the responder thread is explicitly joined/cleaned up; key symbols: spawn_http_responder, thread::spawn, JoinHandle.crates/rginx-app/tests/admin/snapshot.rs (1)
126-130: 建议避免按下标断言 binding 顺序。当前断言依赖
bindings固定顺序，后续只要内部排序策略调整，测试就会出现非功能性失败。建议改为按transport查找目标 binding 再断言字段。可参考的改法
- assert_eq!(listener.bindings[0].transport, "tcp"); - assert_eq!(listener.bindings[1].transport, "udp"); - assert_eq!(listener.bindings[1].protocols, vec!["http3".to_string()]); - assert_eq!(listener.bindings[1].advertise_alt_svc, Some(true)); - assert_eq!(listener.bindings[1].alt_svc_max_age_secs, Some(7200)); + let udp_binding = listener + .bindings + .iter() + .find(|binding| binding.transport == "udp") + .expect("snapshot should include udp binding"); + assert_eq!(udp_binding.protocols, vec!["http3".to_string()]); + assert_eq!(udp_binding.advertise_alt_svc, Some(true)); + assert_eq!(udp_binding.alt_svc_max_age_secs, Some(7200));- assert_eq!( - snapshot["status"]["listeners"][0]["bindings"][1]["transport"], - serde_json::Value::from("udp") - ); - assert_eq!( - snapshot["status"]["listeners"][0]["bindings"][1]["protocols"][0], - serde_json::Value::from("http3") - ); + let udp_json = snapshot["status"]["listeners"][0]["bindings"] + .as_array() + .and_then(|bindings| { + bindings.iter().find(|binding| binding["transport"] == serde_json::Value::from("udp")) + }) + .expect("snapshot JSON should include udp binding"); + assert_eq!(udp_json["protocols"][0], serde_json::Value::from("http3"));Also applies to: 146-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-app/tests/admin/snapshot.rs` around lines 126 - 130, Current assertions rely on fixed ordering in listener.bindings (using bindings[0], bindings[1]) which is brittle; update tests to locate the target binding by its transport field instead of by index. For example, search listener.bindings for the binding where binding.transport == "tcp" or "udp" (using iterator/find or equivalent) and then assert on that binding's protocols, advertise_alt_svc, and alt_svc_max_age_secs; apply the same change for the other assertions around the listener.bindings usage (including the block referenced at lines ~146-155).crates/rginx-app/tests/grpc_http3.rs (1)
105-109: 临时目录清理在测试失败时可能泄漏。当前的清理逻辑在测试结束时通过
fs::remove_dir_all执行。如果测试中的assert!失败，清理代码不会执行，导致临时目录残留。考虑使用
Drop守卫或tempfilecrate 来确保清理总是发生。不过这是测试代码，影响有限，可以作为后续优化。🔧 可选：使用 tempfile crate 自动清理
// 使用 tempfile::TempDir 代替手动创建的目录 let shared_dir = tempfile::tempdir().expect("temp dir should be created"); // TempDir 会在 Drop 时自动清理🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-app/tests/grpc_http3.rs` around lines 105 - 109, The test currently calls fs::remove_dir_all on upstream_temp_dir and shared_dir after server.shutdown_and_wait and upstream_task.await, which will not run if the test panics and thus leaks temp directories; replace the manual dirs with tempfile::TempDir (or wrap the paths in a custom Drop guard) so cleanup happens on Drop even if assertions fail, and update usages that reference upstream_temp_dir and shared_dir to use TempDir::path() (or your guard's path accessor); keep server.shutdown_and_wait and upstream_task.await where appropriate but remove the final fs::remove_dir_all calls since the TempDir/Drop guard will guarantee removal.crates/rginx-app/tests/http3.rs (1)
249-340: 建议：HTTP/3 客户端清理可优化
http3_request函数在 Line 336 中止 driver task 后立即关闭 endpoint，但未等待 driver task 完成。在测试场景下这是可接受的，但如果要提升健壮性，可以考虑短暂等待。另外，当
body参数为None时，Line 300-305 的逻辑仍然正确，因为直接跳过并调用finish()。♻️ 可选的清理优化
driver_task.abort(); + let _ = driver_task.await; endpoint.close(quinn::VarInt::from_u32(0), b"done");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-app/tests/http3.rs` around lines 249 - 340, The driver task launched in http3_request is aborted and the endpoint is immediately closed, which can leave background work in an inconsistent state; update http3_request to attempt a short, bounded wait for driver_task to finish before calling endpoint.close (e.g., await driver_task with a timeout or poll for completion and then abort if still running), referencing the driver_task variable and the endpoint.close call so the cleanup sequence is: request_stream[*] complete, try to await driver_task with a small timeout, if it hasn’t finished then abort it, then call endpoint.close to ensure orderly shutdown.crates/rginx-http/src/proxy/clients/http3.rs (2)
130-144: 注意：完整响应体缓冲可能导致内存压力当前实现将整个上游响应体缓冲到内存中（Line 130-138）。对于大响应，这可能导致内存压力。
对于初始 HTTP/3 支持这是可接受的，但建议在后续迭代中考虑流式传递响应体，以支持大文件代理场景。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/proxy/clients/http3.rs` around lines 130 - 144, The code currently fully buffers the upstream HTTP/3 response body into buffered_body by looping on request_stream.recv_data() and then calling request_stream.recv_trailers(), which can cause OOM for large responses; change the logic in the request handling for request_stream to stream chunks directly instead of accumulating them: read each chunk from request_stream.recv_data() and forward/write it to the downstream responder/response body sink as you receive it (flushing as needed), only buffering small temporary slices if required, and then handle trailers from request_stream.recv_trailers() after the stream ends; use the existing request_stream, recv_data, recv_trailers, and buffered_trailers symbols to locate and replace the buffering logic.
27-53: 考虑连接复用以提升性能当前实现为每个请求创建新的
quinn::Endpoint（Line 32）。这在初始实现中是可接受的，但对于高吞吐量场景，复用 endpoint 和连接可以显著减少握手开销。考虑在后续迭代中引入连接池机制，类似于 Hyper 客户端的
pool_idle_timeout和pool_max_idle_per_host配置。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/proxy/clients/http3.rs` around lines 27 - 53, The code currently creates a new quinn::Endpoint for every request (see resolve_peer_socket_addr -> endpoint variable and endpoint.connect usage), which causes repeated handshakes; refactor to reuse a shared Endpoint and/or maintain a connection pool keyed by upstream/peer (or SocketAddr/hostname) on the client struct instead of instantiating quinn::Endpoint per call, initialize that shared Endpoint once (applying client_config clone) and update connect logic to borrow from the pool or reuse existing quinn::Connection before falling back to endpoint.connect with respect to the existing connect_timeout handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/rginx-app/src/admin_cli/status.rs`:
- Around line 21-34: The bind_addrs logic currently only checks
status.listeners.is_empty() so if listeners exist but all listener.bindings are
empty it produces an empty string; update the computation around bind_addrs in
admin_cli::status.rs to collect the formatted bindings from status.listeners
(the same iterator using listener.bindings and format!("{}://{}",
binding.transport, binding.listen_addr)), then if the collected Vec is empty
return "-".to_string() instead of joining to an empty string—ensure you
reference the bind_addrs variable, status.listeners, listener.bindings, and the
formatting step when implementing the empty-check and fallback.
In `@crates/rginx-app/tests/upstream_http3.rs`:
- Around line 323-330: The verify_client_cert implementation currently only
flips client_cert_seen and always returns Ok, which permits any presented cert
and yields false positives; modify verify_client_cert to actually validate the
end-entity and intermediates against the expected trust anchors and identity
instead of unconditional success: perform proper chain/CA verification (using
the same RootCertStore/verification routine used elsewhere in the codebase or
rustls/webpki helper calls), check the end_entity certificate subject/SAN
matches the expected identity if applicable, set client_cert_seen on success,
and return Err(rustls::Error::General(...)) on verification failure so the test
fails for wrong CA/identity. Ensure you update verify_client_cert and related
test scaffolding to use the real verifier rather than only toggling
client_cert_seen.
- Around line 289-295: 测试中的 resolve 方法将 observed_sni 总是回填为 server_name 或
default_server_name，掩盖了客户端未发送 SNI 的情况；修改 resolve（位于 impl 的 resolve(&self,
client_hello: ClientHello<'_>)）使得只在 client_hello.server_name() 为 Some 时更新
self.observed_sni，保持 None 时不写入默认值（即不要用 self.default_server_name 填充
observed_sni），同时仍返回 Some(self.certified_key.clone())，以便测试能区分实际发送的 SNI 与默认值。
In `@crates/rginx-http/src/proxy/tests/client_profiles.rs`:
- Around line 182-203: The test is inflating cached client count by changing the
peer authority for the http3 Upstream instead of only changing the protocol;
modify the http3 Upstream (the variable named http3) so its UpstreamPeer uses
the same url/authority/host as the other samples (keep the same
host/port/authority values used by http1/http2 peers) and only set
UpstreamProtocol::Http3 for upstream_settings; then rebuild the snapshot via
snapshot_with_upstreams and run ProxyClients::from_config so the assertion on
clients.cached_client_count() reflects protocol-only differences.
In `@crates/rginx-runtime/src/bootstrap/listeners.rs`:
- Around line 428-430: bind_std_udp_socket currently binds the UDP socket but
does not set it to non-blocking mode; update the function so after creating the
StdUdpSocket via StdUdpSocket::bind(listen_addr) it calls set_nonblocking(true)
and returns any resulting Io error wrapped with Error::Io (mirror the pattern
used in bind_std_listener). Ensure you operate on the StdUdpSocket instance
(from bind_std_udp_socket) before returning so the socket is non-blocking for
Quinn's AsyncUdpSocket requirements.
In `@crates/rginx-runtime/src/restart.rs`:
- Around line 138-140: The InheritedSocketKind::Udp branch creates the UDP
socket with StdUdpSocket::from_raw_fd but doesn't set it non-blocking; add a
call to set_nonblocking(true) on the created StdUdpSocket before inserting it
into udp so the socket meets quinn::Endpoint non-blocking requirements (refer to
InheritedSocketKind::Udp, StdUdpSocket::from_raw_fd, set_nonblocking, and
udp.insert).
In `@scripts/test-slow.sh`:
- Around line 33-35: The find command in scripts/test-slow.sh uses the GNU-only
-printf option which breaks on BSD/macOS; replace that step with a
POSIX-compatible approach so the pipeline still yields filenames without
extensions — i.e., have find emit the file paths (no -printf) and then invoke
basename for each result (via find -exec basename ... or piping to
xargs/basename) and then run the existing sed to strip ".rs" and sort; update
the pipeline around the find invocation to use this POSIX method.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 123-124: 将名为 "Run Clippy" 的步骤（当前调用
./scripts/run-clippy-gate.sh）从其当前位置提前：把它放在 fast tests 之后、slow tests
之前，以便更快失败并减少无谓的慢测开销，同时使步骤顺序与 prepare-release.sh 保持一致；修改 release.yml 中该 job/step
的顺序即可，确保依赖关系（如果有）仍然满足。
In `@crates/rginx-app/tests/admin/snapshot.rs`:
- Around line 126-130: Current assertions rely on fixed ordering in
listener.bindings (using bindings[0], bindings[1]) which is brittle; update
tests to locate the target binding by its transport field instead of by index.
For example, search listener.bindings for the binding where binding.transport ==
"tcp" or "udp" (using iterator/find or equivalent) and then assert on that
binding's protocols, advertise_alt_svc, and alt_svc_max_age_secs; apply the same
change for the other assertions around the listener.bindings usage (including
the block referenced at lines ~146-155).
In `@crates/rginx-app/tests/grpc_http3.rs`:
- Around line 105-109: The test currently calls fs::remove_dir_all on
upstream_temp_dir and shared_dir after server.shutdown_and_wait and
upstream_task.await, which will not run if the test panics and thus leaks temp
directories; replace the manual dirs with tempfile::TempDir (or wrap the paths
in a custom Drop guard) so cleanup happens on Drop even if assertions fail, and
update usages that reference upstream_temp_dir and shared_dir to use
TempDir::path() (or your guard's path accessor); keep server.shutdown_and_wait
and upstream_task.await where appropriate but remove the final
fs::remove_dir_all calls since the TempDir/Drop guard will guarantee removal.
In `@crates/rginx-app/tests/http3.rs`:
- Around line 249-340: The driver task launched in http3_request is aborted and
the endpoint is immediately closed, which can leave background work in an
inconsistent state; update http3_request to attempt a short, bounded wait for
driver_task to finish before calling endpoint.close (e.g., await driver_task
with a timeout or poll for completion and then abort if still running),
referencing the driver_task variable and the endpoint.close call so the cleanup
sequence is: request_stream[*] complete, try to await driver_task with a small
timeout, if it hasn’t finished then abort it, then call endpoint.close to ensure
orderly shutdown.
In `@crates/rginx-http/src/proxy/clients/http3.rs`:
- Around line 130-144: The code currently fully buffers the upstream HTTP/3
response body into buffered_body by looping on request_stream.recv_data() and
then calling request_stream.recv_trailers(), which can cause OOM for large
responses; change the logic in the request handling for request_stream to stream
chunks directly instead of accumulating them: read each chunk from
request_stream.recv_data() and forward/write it to the downstream
responder/response body sink as you receive it (flushing as needed), only
buffering small temporary slices if required, and then handle trailers from
request_stream.recv_trailers() after the stream ends; use the existing
request_stream, recv_data, recv_trailers, and buffered_trailers symbols to
locate and replace the buffering logic.
- Around line 27-53: The code currently creates a new quinn::Endpoint for every
request (see resolve_peer_socket_addr -> endpoint variable and endpoint.connect
usage), which causes repeated handshakes; refactor to reuse a shared Endpoint
and/or maintain a connection pool keyed by upstream/peer (or
SocketAddr/hostname) on the client struct instead of instantiating
quinn::Endpoint per call, initialize that shared Endpoint once (applying
client_config clone) and update connect logic to borrow from the pool or reuse
existing quinn::Connection before falling back to endpoint.connect with respect
to the existing connect_timeout handling.
In `@crates/rginx-runtime/src/ocsp.rs`:
- Around line 429-445: spawn_http_responder currently spawns a thread with
thread::spawn and never returns its JoinHandle, leaving the responder possibly
blocked in accept() and causing flaky tests; modify spawn_http_responder to
return the thread::JoinHandle (or a wrapper that can be joined) and propagate
that handle to the caller (tests) so they can join() the thread (or use a
timeout/wakeup) at test teardown, and update callers to join the handle to
ensure the responder thread is explicitly joined/cleaned up; key symbols:
spawn_http_responder, thread::spawn, JoinHandle.
In `@scripts/nginx_compare/configs.py`:
- Around line 132-176: Both rginx_http3_return_config and
rginx_tls_return_config share nearly identical config bodies; extract the common
template into a helper (e.g., _rginx_base_return_config) that builds the shared
runtime, upstreams, locations, and servers, and accepts a parameter for the
server-specific block (the TLS/http3 server stanza). Update
rginx_http3_return_config and rginx_tls_return_config to call this helper,
passing their respective server extension (tls/http3) strings (or a small
builder function) so future changes to runtime/routes are maintained in one
place.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: afdcb378-f83d-4af1-a312-bba78584fdd3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (88)
.github/workflows/ci.yml.github/workflows/release.ymlARCHITECTURE_HTTP3_PHASE0_FREEZE.mdARCHITECTURE_HTTP3_PLAN.mdCargo.tomlREADME.mdclippy.tomlconfigs/conf.d/default.ronconfigs/rginx.roncrates/rginx-app/Cargo.tomlcrates/rginx-app/src/admin_cli/status.rscrates/rginx-app/src/main.rscrates/rginx-app/tests/admin/commands.rscrates/rginx-app/tests/admin/snapshot.rscrates/rginx-app/tests/check.rscrates/rginx-app/tests/downstream_mtls.rscrates/rginx-app/tests/grpc_http3.rscrates/rginx-app/tests/grpc_proxy.rscrates/rginx-app/tests/http3.rscrates/rginx-app/tests/ocsp.rscrates/rginx-app/tests/reload/reload_flow.rscrates/rginx-app/tests/reload/restart_flow.rscrates/rginx-app/tests/upstream_http2.rscrates/rginx-app/tests/upstream_http3.rscrates/rginx-app/tests/upstream_mtls.rscrates/rginx-app/tests/upstream_server_name.rscrates/rginx-config/src/compile/server.rscrates/rginx-config/src/compile/tests.rscrates/rginx-config/src/compile/upstream.rscrates/rginx-config/src/model.rscrates/rginx-config/src/validate/server.rscrates/rginx-config/src/validate/tests.rscrates/rginx-config/src/validate/upstream.rscrates/rginx-core/src/config.rscrates/rginx-core/src/config/tests.rscrates/rginx-core/src/config/upstream.rscrates/rginx-core/src/lib.rscrates/rginx-http/Cargo.tomlcrates/rginx-http/src/handler/dispatch.rscrates/rginx-http/src/handler/mod.rscrates/rginx-http/src/handler/tests.rscrates/rginx-http/src/lib.rscrates/rginx-http/src/pki/certificate.rscrates/rginx-http/src/proxy/clients/http3.rscrates/rginx-http/src/proxy/clients/mod.rscrates/rginx-http/src/proxy/clients/tests.rscrates/rginx-http/src/proxy/clients/tls.rscrates/rginx-http/src/proxy/common.rscrates/rginx-http/src/proxy/forward/mod.rscrates/rginx-http/src/proxy/forward/response.rscrates/rginx-http/src/proxy/health.rscrates/rginx-http/src/proxy/health/registry.rscrates/rginx-http/src/proxy/mod.rscrates/rginx-http/src/proxy/request_body.rscrates/rginx-http/src/proxy/tests/client_profiles.rscrates/rginx-http/src/proxy/tests/mod.rscrates/rginx-http/src/server/graceful.rscrates/rginx-http/src/server/http3.rscrates/rginx-http/src/server/mod.rscrates/rginx-http/src/server/tests.rscrates/rginx-http/src/state/lifecycle.rscrates/rginx-http/src/state/snapshots.rscrates/rginx-http/src/state/tests.rscrates/rginx-http/src/state/tls_runtime/bindings.rscrates/rginx-http/src/state/tls_runtime/listeners.rscrates/rginx-http/src/tls/acceptor.rscrates/rginx-http/src/tls/certificates.rscrates/rginx-http/src/tls/mod.rscrates/rginx-http/src/transition.rscrates/rginx-runtime/Cargo.tomlcrates/rginx-runtime/src/admin.rscrates/rginx-runtime/src/bootstrap/listeners.rscrates/rginx-runtime/src/bootstrap/mod.rscrates/rginx-runtime/src/bootstrap/reload.rscrates/rginx-runtime/src/bootstrap/shutdown.rscrates/rginx-runtime/src/health.rscrates/rginx-runtime/src/ocsp.rscrates/rginx-runtime/src/restart.rsscripts/nginx_compare/configs.pyscripts/nginx_compare/render.pyscripts/nginx_compare/scenarios.pyscripts/prepare-release.shscripts/run-benchmark-matrix.pyscripts/run-clippy-gate.shscripts/run-soak.shscripts/run-tls-gate.shscripts/test-fast.shscripts/test-slow.sh
Owner
Author
|
Addressed the actionable review items in follow-up commit
I left the larger architectural suggestions (upstream HTTP/3 connection pooling and fully streaming upstream H3 response bodies) for a later iteration, since the current buffered-response implementation is an intentional stability tradeoff for this PR. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/rginx-app/src/admin_cli/status.rs (1)
203-261: 布尔字段渲染格式存在细微不一致。
status_listener的tls/http3字段（第 144、148 行）使用"enabled"/"disabled"字符串，而status_tls_listener的tls/http3_enabled字段（第 210-211 行）直接使用.to_string()输出"true"/"false"。虽然字段名称不同（带
_enabled后缀），但对于解析状态输出的消费者可能产生混淆。建议统一布尔字段的输出格式，或在文档中明确说明差异。♻️ 可选：统一使用 "enabled"/"disabled" 格式
("listener_id", listener.listener_id.clone()), ("listen", listener.listen_addr.to_string()), - ("tls", listener.tls_enabled.to_string()), - ("http3_enabled", listener.http3_enabled.to_string()), + ("tls", if listener.tls_enabled { "enabled" } else { "disabled" }.to_string()), + ("http3_enabled", if listener.http3_enabled { "enabled" } else { "disabled" }.to_string()),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-app/src/admin_cli/status.rs` around lines 203 - 261, The boolean rendering for TLS/HTTP3 is inconsistent between the two print blocks: in status_listener the tls/http3 fields emit "enabled"/"disabled" but in the status_tls_listener loop the fields ("tls" and "http3_enabled") call .to_string() producing "true"/"false"; update the status_tls_listener branch (inside the for listener in &status.tls.listeners loop passed to print_record) to use the same "enabled"/"disabled" formatting as status_listener (extract or reuse the existing helper logic—or introduce a small helper like render_enabled_disabled(bool) and call it for listener.tls_enabled and listener.http3_enabled) so all boolean fields are rendered consistently. Ensure you update both "tls" and "http3_enabled" entries accordingly and keep other fields (render_optional_string_list, client_auth_* etc.) unchanged.crates/rginx-runtime/src/bootstrap/listeners.rs (1)
319-335: 建议增加防御性断言以明确 HTTP/3 配置与 UDP socket 的对应关系。当前逻辑：如果
std_udp_socket为Some，使用它创建 endpoint；否则回退到bind_http3_endpoint。根据上游调用点 (
build_initial_listener_groups和prepare_added_listener_bindings)，当listener.http3配置存在时，std_udp_socket必定为Some。但当前代码结构允许std_udp_socket为None而listener.http3为Some的情况，此时bind_http3_endpoint可能会创建一个不被std_udp_socket追踪的 socket，导致 restart 时无法继承该 UDP socket。♻️ 可选：增加防御性断言
let http3_endpoint = match &std_udp_socket { Some(socket) => Some(rginx_http::server::bind_http3_endpoint_with_socket( &listener, default_vhost, vhosts, socket.try_clone()?, )?), - None => rginx_http::server::bind_http3_endpoint(&listener, default_vhost, vhosts)?, + None => { + debug_assert!( + listener.http3.is_none(), + "http3 config present but std_udp_socket is None; restart inheritance may be broken" + ); + rginx_http::server::bind_http3_endpoint(&listener, default_vhost, vhosts)? + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-runtime/src/bootstrap/listeners.rs` around lines 319 - 335, The code may create an HTTP/3 endpoint without a tracked UDP socket which breaks restart socket inheritance; add a defensive check before calling bind_http3_endpoint or constructing PreparedListenerWorkerGroup to assert that if listener.http3 is Some then std_udp_socket.is_some(), and otherwise return a clear error (or propagate an Err) instead of silently calling bind_http3_endpoint; update the branch around bind_http3_endpoint_with_socket / bind_http3_endpoint (and the PreparedListenerWorkerGroup construction) to enforce this invariant so callers like build_initial_listener_groups and prepare_added_listener_bindings cannot end up with an untracked UDP socket.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/rginx-app/src/admin_cli/status.rs`:
- Around line 203-261: The boolean rendering for TLS/HTTP3 is inconsistent
between the two print blocks: in status_listener the tls/http3 fields emit
"enabled"/"disabled" but in the status_tls_listener loop the fields ("tls" and
"http3_enabled") call .to_string() producing "true"/"false"; update the
status_tls_listener branch (inside the for listener in &status.tls.listeners
loop passed to print_record) to use the same "enabled"/"disabled" formatting as
status_listener (extract or reuse the existing helper logic—or introduce a small
helper like render_enabled_disabled(bool) and call it for listener.tls_enabled
and listener.http3_enabled) so all boolean fields are rendered consistently.
Ensure you update both "tls" and "http3_enabled" entries accordingly and keep
other fields (render_optional_string_list, client_auth_* etc.) unchanged.
In `@crates/rginx-runtime/src/bootstrap/listeners.rs`:
- Around line 319-335: The code may create an HTTP/3 endpoint without a tracked
UDP socket which breaks restart socket inheritance; add a defensive check before
calling bind_http3_endpoint or constructing PreparedListenerWorkerGroup to
assert that if listener.http3 is Some then std_udp_socket.is_some(), and
otherwise return a clear error (or propagate an Err) instead of silently calling
bind_http3_endpoint; update the branch around bind_http3_endpoint_with_socket /
bind_http3_endpoint (and the PreparedListenerWorkerGroup construction) to
enforce this invariant so callers like build_initial_listener_groups and
prepare_added_listener_bindings cannot end up with an untracked UDP socket.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: bcef60e8-cf9f-439e-85ce-f350025dc677
📒 Files selected for processing (17)
.github/workflows/ci.yml.github/workflows/release.ymlcrates/rginx-app/src/admin_cli/status.rscrates/rginx-app/tests/admin/snapshot.rscrates/rginx-app/tests/grpc_http3.rscrates/rginx-app/tests/http3.rscrates/rginx-app/tests/reload/reload_flow.rscrates/rginx-app/tests/reload/restart_flow.rscrates/rginx-app/tests/upstream_http3.rscrates/rginx-http/src/proxy/tests/client_profiles.rscrates/rginx-http/src/server/http3.rscrates/rginx-runtime/src/bootstrap/listeners.rscrates/rginx-runtime/src/ocsp.rscrates/rginx-runtime/src/restart.rsdeny.tomlscripts/nginx_compare/configs.pyscripts/test-slow.sh
✅ Files skipped from review due to trivial changes (4)
- deny.toml
- crates/rginx-app/tests/grpc_http3.rs
- crates/rginx-app/tests/http3.rs
- crates/rginx-http/src/server/http3.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- .github/workflows/release.yml
- crates/rginx-http/src/proxy/tests/client_profiles.rs
- scripts/test-slow.sh
- crates/rginx-runtime/src/ocsp.rs
- crates/rginx-app/tests/reload/restart_flow.rs
- scripts/nginx_compare/configs.py
- crates/rginx-app/tests/upstream_http3.rs
- crates/rginx-app/tests/reload/reload_flow.rs
This was referenced Apr 13, 2026
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
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
0.1.3-rc.10Validation
./scripts/test-fast.sh./scripts/run-clippy-gate.sh./scripts/test-slow.sh./scripts/run-tls-gate.sh./scripts/run-soak.sh --iterations 1./scripts/prepare-release.sh --tag v0.1.3-rc.10 --allow-dirty --skip-fetch