http3: implement phase 7 early-data, retry, and telemetry (review replay of #41)#43
Closed
vansour wants to merge 8 commits intoreview-pr-41-basefrom
Closed
http3: implement phase 7 early-data, retry, and telemetry (review replay of #41)#43vansour wants to merge 8 commits intoreview-pr-41-basefrom
vansour wants to merge 8 commits intoreview-pr-41-basefrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 49 minutes and 33 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
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:
📝 WalkthroughWalkthrough此 PR 大规模引入并启用 HTTP/3 功能：配置与验证扩展、QUIC/HTTP3 会话复用与流式响应、0‑RTT/early‑data 策略与计数、多个 UDP socket/accept 工作器、运行时快照/遥测扩展、测试与浸泡/发布门控脚本，以及文档和版本更新。 Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as HTTP/3 Server
participant State as SharedState
participant Handler as RouteHandler
participant Upstream
Client->>Server: QUIC Initial (含 0‑RTT)
Note over Server: attach EarlyDataFlag(true)
Server->>State: record retry/early-data 事件
Server->>Handler: dispatch(request with EarlyDataFlag)
alt early_data && !route.allow_early_data
Handler->>State: record_http3_early_data_rejected_request
Handler-->>Client: 425 Too Early / grpc Unavailable
else early_data && route.allow_early_data
Handler->>State: record_http3_early_data_accepted_request
Handler->>Upstream: forward request
Upstream-->>Handler: response
Handler-->>Client: 流式返回 response frames
end
Note over State: 快照版本递增并 notify waiters
sequenceDiagram
participant Bootstrap as RuntimeBootstrap
participant Socket as UDP Socket Layer
participant Endpoint as QuinnEndpoint
participant Worker as AcceptWorker
Bootstrap->>Bootstrap: 计算 accept_workers N
Bootstrap->>Socket: bind_std_udp_sockets(addr, N)
loop 为每个 socket
Socket->>Socket: 创建 StdUdpSocket (可能启用 SO_REUSEPORT)
Socket-->>Bootstrap: 返回 socket 列表
end
Bootstrap->>Endpoint: 为每个 socket 创建 quinn::Endpoint
Bootstrap-->>Worker: spawn accept worker per endpoint
Worker->>Endpoint: 接收 QUIC 包并处理 retry/0‑RTT/handshake
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR reproduces the original changeset from PR #41 (release prep for v0.1.3-rc.11) so automated review tooling can complete review against the pre-merge base commit. The changes primarily expand HTTP/3 release gating assets, QUIC/HTTP/3 runtime telemetry, and early-data (0-RTT) replay-safety controls across config, runtime, CLI, and tests.
Changes:
- Add HTTP/3 release-gate scripts (gate, soak, Phase 0 baseline, release wrapper) and wire them into release preflight + GitHub Release workflow.
- Extend HTTP/3 runtime model: per-worker UDP sockets (reuse-port), QUIC runtime telemetry, retry support, and 0-RTT early-data policy with route-level replay safety.
- Update control-plane snapshots/status/check outputs and integration/unit tests; bump snapshot schema version to 13 and workspace version to
0.1.3-rc.11.
Reviewed changes
Copilot reviewed 56 out of 57 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/run-http3-soak.sh | New focused HTTP/3 soak runner with optional netem/MTU shaping. |
| scripts/run-http3-release-gate.sh | New wrapper to run HTTP/3 regression gate + soak + optional nginx compare. |
| scripts/run-http3-phase0.sh | New Phase 0 baseline runner for HTTP/3 regression/soak subset. |
| scripts/run-http3-gate.sh | New dedicated HTTP/3 regression gate script. |
| scripts/prepare-release.sh | Run HTTP/3 release gate during release preflight. |
| RELEASE_NOTES_v0.1.3-rc.11.md | New release notes for v0.1.3-rc.11 focusing on HTTP/3 maturity. |
| README.md | Update displayed current version and HTTP/3 plan/docs pointers + new gate commands. |
| crates/rginx-runtime/src/restart.rs | Preserve multiple UDP sockets across restart for HTTP/3 listeners. |
| crates/rginx-runtime/src/bootstrap/shutdown.rs | Update test fixtures for new allow_early_data route field. |
| crates/rginx-runtime/src/bootstrap/listeners.rs | Bind one UDP socket per accept worker (reuse-port) and start multiple HTTP/3 accept workers. |
| crates/rginx-runtime/src/admin.rs | Bump admin snapshot schema version to 13. |
| crates/rginx-runtime/Cargo.toml | Add socket2 dependency for UDP socket options/binding. |
| crates/rginx-http/src/transition.rs | Update transition tests for expanded HTTP/3 listener config fields. |
| crates/rginx-http/src/tls/acceptor.rs | Allow HTTP/3 server TLS config to enable early-data via max_early_data_size. |
| crates/rginx-http/src/state/traffic.rs | Add HTTP/3 runtime telemetry into traffic snapshots. |
| crates/rginx-http/src/state/tls_runtime/listeners.rs | Expose more HTTP/3 listener transport settings in TLS status snapshots. |
| crates/rginx-http/src/state/tests.rs | Extend status/traffic snapshot tests for HTTP/3 telemetry and settings. |
| crates/rginx-http/src/state/snapshots.rs | Add snapshot fields for HTTP/3 runtime telemetry and early-data counters/settings. |
| crates/rginx-http/src/state/lifecycle.rs | Aggregate HTTP/3 runtime counters into top-level runtime status snapshot. |
| crates/rginx-http/src/state/helpers.rs | Add helper to build per-listener HTTP/3 runtime telemetry snapshots. |
| crates/rginx-http/src/state/counters.rs | Add HTTP/3 runtime/early-data counters (listener + global). |
| crates/rginx-http/src/state/connections.rs | Add guards + APIs to track active HTTP/3 connections/streams and record telemetry events. |
| crates/rginx-http/src/server/http3.rs | Implement HTTP/3 retry, runtime telemetry hooks, 0-RTT early-data propagation, and host-key based token/reset keys. |
| crates/rginx-http/src/server/connection.rs | Track early_data in connection metadata and export TLS handshake failure classifier. |
| crates/rginx-http/src/router.rs | Update tests to include new allow_early_data field on routes. |
| crates/rginx-http/src/proxy/clients/http3.rs | Rework upstream HTTP/3 client to reuse endpoints/sessions and stream response bodies. |
| crates/rginx-http/src/handler/tests.rs | Update handler tests for new route field and connection metadata. |
| crates/rginx-http/src/handler/response.rs | Add a 425 Too Early convenience response builder. |
| crates/rginx-http/src/handler/mod.rs | Attach early-data info into request extensions for routing/policy. |
| crates/rginx-http/src/handler/dispatch.rs | Enforce route-level early-data replay safety and record accept/reject counters. |
| crates/rginx-http/src/client_ip.rs | Extend connection metadata to include early-data flag. |
| crates/rginx-http/Cargo.toml | Add aws-lc-rs and quinn-proto for QUIC host-key derived token/reset/cid behavior. |
| crates/rginx-core/src/config/tests.rs | Update config tests for expanded HTTP/3 binding fields and new route field. |
| crates/rginx-core/src/config/route.rs | Add allow_early_data to compiled route model. |
| crates/rginx-core/src/config.rs | Extend ListenerHttp3 and transport bindings with QUIC/HTTP3 tuning + early-data fields. |
| crates/rginx-config/src/validate/tests.rs | Add validation coverage for HTTP/3 transport settings and early-data constraints. |
| crates/rginx-config/src/validate/server.rs | Validate HTTP/3 tuning fields, host_key_path, retry prerequisites, and early-data TLS requirements. |
| crates/rginx-config/src/model.rs | Extend config model for HTTP/3 tuning + route allow_early_data. |
| crates/rginx-config/src/compile/tests.rs | Update compile tests to include new route field defaults. |
| crates/rginx-config/src/compile/server.rs | Compile HTTP/3 tuning fields, host_key_path resolution, and defaults. |
| crates/rginx-config/src/compile/route.rs | Compile allow_early_data into the route model. |
| crates/rginx-app/tests/support/mod.rs | Improve HTTP response parsing helper to handle content-length and chunked responses robustly. |
| crates/rginx-app/tests/http3.rs | Add extensive integration tests for HTTP/3 telemetry, retry host-key, mTLS, and early-data replay safety. |
| crates/rginx-app/tests/check.rs | Expand check output expectations for HTTP/3 binding/early-data fields and accept workers. |
| crates/rginx-app/tests/admin/snapshot.rs | Update snapshot tests for schema 13 and new HTTP/3 fields/telemetry. |
| crates/rginx-app/tests/admin/commands.rs | Update admin CLI tests for new HTTP/3 runtime and early-data counters/fields. |
| crates/rginx-app/src/main.rs | Extend check summary/output with HTTP/3 binding settings and early-data-enabled listener count. |
| crates/rginx-app/src/admin_cli/traffic.rs | Print per-listener HTTP/3 runtime telemetry in traffic output. |
| crates/rginx-app/src/admin_cli/status.rs | Print aggregated + per-listener HTTP/3 runtime telemetry and binding settings in status output. |
| crates/rginx-app/src/admin_cli/counters.rs | Print global HTTP/3 early-data accept/reject counters. |
| Cargo.toml | Bump workspace version to 0.1.3-rc.11. |
| Cargo.lock | Lockfile updates for new deps (aws-lc-rs, quinn-proto, socket2 v0.5.10, etc.). |
| ARCHITECTURE_REMEDIATION_RELEASE_NOTE.md | Remove obsolete remediation release note doc. |
| ARCHITECTURE_REMEDIATION_PLAN.md | Remove obsolete remediation plan doc. |
| ARCHITECTURE_HTTP3_PLAN.md | Remove superseded HTTP/3 plan doc (replaced by nginx-alignment docs referenced elsewhere). |
| ARCHITECTURE_HTTP3_PHASE0_FREEZE.md | Remove superseded Phase 0 freeze doc (replaced by newer phase docs). |
| .github/workflows/release.yml | Run HTTP/3 release gate during release verification job. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+98
to
+101
| if [[ "${need_privileged}" -eq 1 ]]; then | ||
| have tc || die "tc is required when --netem-profile is used" | ||
| have ip || die "ip is required when --mtu is used" | ||
| [[ "${EUID}" -eq 0 ]] || die "root privileges are required for netem/mtu operations" |
There was a problem hiding this comment.
tc is required unconditionally whenever need_privileged=1, which includes the --mtu-only case. As written, ./scripts/run-http3-soak.sh --mtu ... will fail with “tc is required…” even though netem isn’t being used. Consider checking tc only when NETEM_PROFILE != none, and checking ip only when MTU is set (keeping the root check gated the same way).
Comment on lines
+431
to
+437
| fn bind_std_udp_sockets( | ||
| listen_addr: std::net::SocketAddr, | ||
| count: usize, | ||
| ) -> Result<Vec<Arc<StdUdpSocket>>> { | ||
| let count = count.max(1); | ||
| (0..count).map(|_| bind_std_udp_socket(listen_addr, count > 1).map(Arc::new)).collect() | ||
| } |
There was a problem hiding this comment.
bind_std_udp_sockets will attempt to bind count UDP sockets to the same listen_addr. Outside Linux, the SO_REUSEPORT setsockopt is not applied (it’s behind #[cfg(target_os = "linux")]), so using accept_workers > 1 with HTTP/3 is likely to fail with EADDRINUSE on other Unix platforms. Consider either enabling the equivalent reuse-port option for other supported OSes (via socket2 APIs / per-OS cfg) or forcing count=1 (and/or returning a config error) when reuse-port can’t be enabled.
Comment on lines
+325
to
+327
| fn is_end_stream(&self) -> bool { | ||
| self.rx.is_closed() | ||
| } |
There was a problem hiding this comment.
StreamingResponseBody::is_end_stream returns rx.is_closed(), which becomes true as soon as the sender is dropped even if the receiver still has queued frames to yield. That violates the hyper::body::Body contract and can cause truncated upstream HTTP/3 responses. Consider tracking completion explicitly (e.g., set a done flag when poll_frame returns None, or use rx.is_closed() && rx.is_empty() if available) so is_end_stream only returns true once all frames have been drained.
Comment on lines
44
to
51
| pub(crate) fn attach_connection_metadata<B>( | ||
| request: &mut Request<B>, | ||
| connection: &ConnectionPeerAddrs, | ||
| ) { | ||
| request.extensions_mut().insert(connection.early_data); | ||
| if let Some(identity) = connection.tls_client_identity.clone() { | ||
| request.extensions_mut().insert(identity); | ||
| } |
There was a problem hiding this comment.
Storing early_data as a bare bool in Request extensions is fragile because extensions are keyed only by type; any future feature that also inserts a bool would collide and could cause incorrect early-data routing decisions. Consider introducing a dedicated newtype (e.g., struct EarlyData(pub bool)) and using that for both insertion and lookup.
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (11)
crates/rginx-http/src/proxy/clients/http3.rs (1)
414-418: 避免靠错误字符串识别H3_NO_ERROR这里把“干净关闭”的判断绑定到了
Display文本，升级h3/quinn后很容易失效。更稳妥的做法是直接匹配结构化错误码/枚举，否则一次普通依赖升级就可能把正常关闭误判成流错误。🤖 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 414 - 418, The current is_clean_http3_response_shutdown function relies on Display text matching which is brittle; change it to inspect concrete error types/enums instead: update the function signature (is_clean_http3_response_shutdown) to accept &dyn std::error::Error (or the concrete error type used in call sites), attempt downcasting (error.downcast_ref::<h3::Error>() or downcast_ref::<quinn::ConnectionError>() / quinn::TransportError>() as appropriate) and match the structured variants (e.g. h3::Error::ApplicationClose or Error::Application { code } and compare against h3::ErrorCode::H3_NO_ERROR or the quinn close-code enum) to return true for clean shutdowns; keep a Display fallback only as a last resort and update all call sites to pass the error as an error trait object or concrete type.crates/rginx-http/src/handler/mod.rs (1)
44-50: 不要把 0-RTT 标记直接存成裸bool扩展。
http::Extensions是按具体类型做 key 的，直接插入bool只有一个槽位。后面只要别的中间件也往扩展里放bool，这里的early_data就会被覆盖或读错。更稳妥的做法是定义一个专用 newtype。♻️ 建议改法
+#[derive(Clone, Copy, Debug)] +pub(crate) struct EarlyData(pub bool); + pub(crate) fn attach_connection_metadata<B>( request: &mut Request<B>, connection: &ConnectionPeerAddrs, ) { - request.extensions_mut().insert(connection.early_data); + request.extensions_mut().insert(EarlyData(connection.early_data)); if let Some(identity) = connection.tls_client_identity.clone() { request.extensions_mut().insert(identity); } }对应读取侧再改成
get::<EarlyData>().map(|v| v.0).unwrap_or(false)。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/handler/mod.rs` around lines 44 - 50, 当前实现把 0-RTT 的标记直接以裸 bool 插入 Request 的 extensions（在 attach_connection_metadata 中使用 connection.early_data），这会与其它 bool 扩展冲突；请定义一个专用 newtype（例如 EarlyData(bool)）并在 attach_connection_metadata 用 request.extensions_mut().insert(EarlyData(connection.early_data)) 插入；同时更新读取方改为 request.extensions().get::<EarlyData>().map(|v| v.0).unwrap_or(false)；保持对 tls_client_identity 的处理不变（仍在 attach_connection_metadata 中插入其具体类型）。crates/rginx-http/src/state/connections.rs (1)
314-416: 把重复的 HTTP/3 计数+快照更新逻辑抽成一个 helper。这些方法除了具体递增的字段不同外，
mark_snapshot_changed_components、mark_traffic_targets_changed和notify_snapshot_waiters的流程几乎完全一致。现在这块已经有不少重复，后面只要改一次版本标记或通知顺序，就很容易漏掉某个分支。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/state/connections.rs` around lines 314 - 416, All these record_http3_* methods repeat the same snapshot/notification sequence; extract a helper like bump_http3_counter_and_snapshot(listener_id: &str, op: impl FnOnce(&ListenerTrafficCounters)) that (1) looks up listener_traffic_counters(listener_id), (2) runs the provided closure to increment the specific atomic counter, and (3) calls mark_snapshot_changed_components(true, false, true, false, false), mark_traffic_targets_changed(..., Some(listener_id), None, None) and notify_snapshot_waiters(); then refactor record_http3_retry_issued, record_http3_retry_failed, record_http3_request_* and record_http3_response_stream_error to call this helper with a small closure that increments the right field, and refactor record_http3_connection_close to either call the same helper inside each match arm or use the helper with a closure that matches on the quinn::ConnectionError and increments the appropriate counter.crates/rginx-app/src/admin_cli/traffic.rs (1)
130-189: 建议将traffic_listener_http3输出块提取为独立 helper。当前字段列表很长，后续新增/改名指标时更容易与其他输出路径出现维护漂移。提取函数可降低认知负担。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-app/src/admin_cli/traffic.rs` around lines 130 - 189, Extract the long traffic_listener_http3 print logic into a helper function (e.g., print_listener_http3 or print_http3_metrics) that takes the listener id and a reference to listener.http3_runtime and calls print_record with the same metric keys/values; replace the inline block in the original code with a single call to that helper. Ensure the helper lives near other CLI printing utilities, accepts (&str or String) listener_id and the http3 runtime struct (match its type), converts the fields to strings as currently done, and preserves all metric names exactly so other output paths remain consistent.crates/rginx-core/src/config/tests.rs (1)
148-177: 建议补充early_data_enabled的 binding 断言覆盖。当前在 Line 154 已设置
early_data_enabled: false，但此测试里没有看到对应映射结果的显式校验（若 binding 暴露该字段，建议断言它）。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-core/src/config/tests.rs` around lines 148 - 177, The test sets early_data_enabled: false on the UDP listener but doesn't assert the mapped binding exposes it; add an assertion that the binding produced by listener.transport_bindings() reflects this flag (e.g. check bindings[1] for the http3 early-data field — something like bindings[1].http3_early_data_enabled == Some(false) or the appropriate field name used by the binding struct) to ensure early_data_enabled is covered by the test.scripts/run-http3-release-gate.sh (1)
88-92: 建议对--netem-profile做枚举校验并提前失败。当前仅校验了
soak-iterations；netem-profile与帮助文档声明的允许值不一致时，建议在本脚本直接报错。建议修改
[[ "${SOAK_ITERATIONS}" =~ ^[0-9]+$ ]] || die "--soak-iterations must be a positive integer" [[ "${SOAK_ITERATIONS}" -ge 1 ]] || die "--soak-iterations must be >= 1" +case "${NETEM_PROFILE}" in + none|loss|reorder|jitter) ;; + *) die "--netem-profile must be one of: none|loss|reorder|jitter" ;; +esacAlso applies to: 123-124
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/run-http3-release-gate.sh` around lines 88 - 92, The --netem-profile parsing currently only assigns NETEM_PROFILE without validating allowed values; add an explicit enum check after parsing NETEM_PROFILE (e.g., compare NETEM_PROFILE against the documented allowed values) and call die with a clear error if it is not one of them. Update the parsing branch that sets NETEM_PROFILE (and the other occurrence around the same code) to perform this validation immediately after shift, using the existing die helper for failure messages and referencing NETEM_PROFILE in the error so users see the invalid value and the allowed set.crates/rginx-http/src/handler/dispatch.rs (1)
61-61: 建议用专用类型包装boolextension，而非直接使用裸bool。当前仅
early_data使用bool作为 extension 键，暂无实际冲突。但建议创建专用 newtype（如struct EarlyDataFlag(bool)）作为 extension 键，遵循类型安全最佳实践，防止未来其他代码误插入bool类型导致隐蔽的键冲突。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/handler/dispatch.rs` at line 61, Create a small newtype wrapper for the extension instead of using a raw bool: define a simple struct EarlyDataFlag(pub bool) (derive Copy, Clone, Debug) and update any places that insert the extension to use EarlyDataFlag(true/false). Then change the retrieval in dispatch (where early_data is set from request.extensions()) to use request.extensions().get::<EarlyDataFlag>().map(|f| f.0).unwrap_or(false) (replacing get::<bool>().copied().unwrap_or(false)) so the extension key is type-safe and avoids collisions.crates/rginx-app/src/main.rs (2)
48-74: 提取CheckSummary构建逻辑。
check和-t分支现在维护两份几乎相同的CheckSummary初始化。本次新增的http3_early_data_enabled_listeners已经复制了一次，后面继续扩字段时很容易只改到一个分支。Also applies to: 82-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-app/src/main.rs` around lines 48 - 74, Extract the duplicated CheckSummary construction into a single helper function (e.g., fn build_check_summary(config: &Config) -> CheckSummary) and call it from both the check and -t branches instead of duplicating the struct literal; move the http3_early_data_enabled_listeners iterator logic (the .iter().filter(|listener| listener.http3.as_ref().is_some_and(|http3| http3.early_data_enabled)).count()) and all other fields (listener_model, listener_count, listener_binding_count, listeners, tls_enabled, http3_enabled, total_vhost_count, total_route_count, upstream_count, worker_threads, accept_workers, tls) into that helper so future fields are changed in one place and both branches simply call build_check_summary(&config) when invoking print_check_success.
576-578: 这里直接比较枚举会更稳。
binding.kind.as_str() == "udp"把逻辑耦合到了字符串表示；同一个 PR 里的crates/rginx-http/src/state/lifecycle.rs已经直接比较ListenerTransportKind::Udp了，建议统一成枚举比较。可参考的修改
- reuse_port_enabled: (binding.kind.as_str() == "udp") - .then_some(config.runtime.accept_workers > 1), + reuse_port_enabled: (binding.kind + == rginx_core::ListenerTransportKind::Udp) + .then_some(config.runtime.accept_workers > 1),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-app/src/main.rs` around lines 576 - 578, The code compares binding.kind via its string form; replace the string comparison with an enum comparison (e.g., use matches!(binding.kind, ListenerTransportKind::Udp) or direct equality against ListenerTransportKind::Udp) when setting reuse_port_enabled so it’s consistent with other code (see ListenerTransportKind::Udp) and still combine with config.runtime.accept_workers > 1 for the .then_some(...) logic; update the expression that computes reuse_port_enabled (which references binding.kind and config.runtime.accept_workers) to use the enum-based check.crates/rginx-config/src/compile/tests.rs (1)
2248-2258: 补上early_data默认值断言。这个用例已经把本次新增的大部分 HTTP/3 默认值都钉住了，但
early_data还没覆盖。若默认值以后意外变成开启，这组测试不会报错。可参考的补充断言
assert_eq!(http3.max_concurrent_streams, 128); assert_eq!(http3.stream_buffer_size, 64 * 1024); assert_eq!(http3.active_connection_id_limit, 2); assert!(!http3.retry); assert_eq!(http3.host_key_path, None); assert!(!http3.gso); + assert!(!http3.early_data_enabled); }Also applies to: 2286-2292
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-config/src/compile/tests.rs` around lines 2248 - 2258, The test's expected Http3Config currently lists early_data but does not assert the runtime default; update the test in crates/rginx-config/src/compile/tests.rs (both occurrences around the shown diff and the one at 2286-2292) to explicitly assert that Http3Config::early_data is None (i.e., the default is disabled) by adding a check against None for the early_data field when validating the compiled config so the test will fail if the default flips to enabled.crates/rginx-http/src/state/lifecycle.rs (1)
28-79: 把 HTTP/3 汇总计数改成单次遍历。这里对同一组 listener/counter 做了多次扫描。
status_snapshot()属于控制面热路径，改成一次 fold/accumulate 会更省锁持有时间，也能减少后续再加字段时漏改某一处的风险。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/state/lifecycle.rs` around lines 28 - 79, Multiple separate iterations over config.listeners to collect HTTP/3 counters should be replaced with a single loop/fold to avoid repeated lookups and reduce lock/CPU overhead in status_snapshot(); implement by iterating once over config.listeners (or using an iterator.fold) and for each listener do listener_traffic.listeners.get(&listener.id).map(|entry| { increment local accumulators: http3_active_connections, http3_active_request_streams, http3_retry_issued_total, http3_retry_failed_total, http3_request_accept_errors_total, http3_request_resolve_errors_total, http3_request_body_stream_errors_total, http3_response_stream_errors_total using the same .load(...) calls }); at the end assign or return the accumulated totals; keep the existing variable names (listener_traffic, http3_active_connections, http3_active_request_streams, http3_retry_issued_total, http3_retry_failed_total, http3_request_accept_errors_total, http3_request_resolve_errors_total, http3_request_body_stream_errors_total, http3_response_stream_errors_total) so callers like status_snapshot() continue to work.
🤖 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-http/src/proxy/clients/http3.rs`:
- Around line 369-388: The code currently skips reading HTTP trailers based on
expect_trailers (Content-Type), which silently drops valid non-gRPC trailers;
change the logic so that after the response body finishes you always call
request_stream.recv_trailers().await once (remove the conditional check on
expect_trailers) and keep the existing match arms (sending Frame::trailers via
tx, treating Ok(None) as no trailers, and the same error handling including
is_clean_http3_response_shutdown, session.mark_closed and tx.send of the error).
Ensure you update the block around request_stream.recv_trailers(),
Frame::trailers, tx.send and session.mark_closed so trailers are attempted
unconditionally.
- Around line 325-327: The is_end_stream() implementation returns true too early
because tokio::mpsc::Receiver::is_closed() only means no more sends will occur,
not that the buffer is empty; change is_end_stream() to check both closure and
emptiness by using self.rx.is_closed() && self.rx.len() == 0 (since Receiver has
no is_empty()), or implement and consult existing server-side flags (e.g.
data_finished and trailers_finished) and return true only when both data and
trailers are finished; update the is_end_stream() method accordingly to
reference self.rx and/or the data_finished/trailers_finished flags so the
response body isn't ended prematurely.
In `@crates/rginx-http/src/server/http3.rs`:
- Around line 424-468: The current read-then-create flow in
load_or_create_host_key_material/create_host_key_material allows a race where
two processes both observe NotFound and generate different keys, causing one to
overwrite the other; fix by using O_EXCL/create_new semantics when creating the
on-disk key: in create_host_key_material (called by
load_or_create_host_key_material) first ensure the parent dir exists, then
attempt to create the target file atomically with
OpenOptions::new().write(true).create_new(true).open(path) and write the
generated bytes into that handle, set permissions (on unix) and flush; if open
fails with AlreadyExists, fall back to reading the file (std::fs::read) and
validate via validate_host_key_material and return that content instead;
propagate other errors as before. Reference: load_or_create_host_key_material,
create_host_key_material, validate_host_key_material.
In `@crates/rginx-http/src/state/connections.rs`:
- Around line 145-182: The guards currently silently no-op if
listener_traffic_counters(listener_id) returns None; change
retain_http3_connection and retain_http3_request_stream to fail fast like
retain_connection_slot by returning a Result (e.g.
Result<ActiveHttp3ConnectionGuard, SomeError> /
Result<ActiveHttp3RequestStreamGuard, SomeError>) and return Err when
listener_traffic_counters(listener_id) is None instead of constructing a guard
with None counters; keep the existing increment/notify logic when counters is
Some and construct the Ok(guard) only then, and update callers to handle the
Result.
In `@crates/rginx-http/src/tls/acceptor.rs`:
- Around line 156-158: The current TLS config unconditionally sets
config.max_early_data_size = u32::MAX when http3_only &&
http3_early_data_enabled, which allows up to 4GB of 0-RTT and wastes memory;
change this to clamp the early-data limit to the application's
max_request_body_bytes (or a conservative default) instead. In acceptor.rs,
where http3_only and http3_early_data_enabled are checked, replace the
unconditional u32::MAX assignment with logic that computes a safe u32 cap from
max_request_body_bytes (e.g., min(max_request_body_bytes, u32::MAX) cast to u32)
or a configured default, and assign that value to config.max_early_data_size so
TLS buffers cannot exceed the application request body limit.
In `@crates/rginx-runtime/src/bootstrap/listeners.rs`:
- Around line 83-89: The code currently reuses inherited UDP sockets for
listener.http3 without reconciling their count with
config.runtime.accept_workers; update the branch that handles Some(sockets) so
you normalize the number to accept_workers: wrap existing sockets with Arc::new
and if sockets.len() > accept_workers truncate to accept_workers, if
sockets.len() < accept_workers call
bind_std_udp_sockets(listener.http3.listen_addr, missing) to create the missing
sockets and append them, then collect into the std_udp_sockets Vec; ensure you
reference listener.http3, inherited.udp, bind_std_udp_sockets and
config.runtime.accept_workers when making the change and preserve existing Arc
wrapping.
---
Nitpick comments:
In `@crates/rginx-app/src/admin_cli/traffic.rs`:
- Around line 130-189: Extract the long traffic_listener_http3 print logic into
a helper function (e.g., print_listener_http3 or print_http3_metrics) that takes
the listener id and a reference to listener.http3_runtime and calls print_record
with the same metric keys/values; replace the inline block in the original code
with a single call to that helper. Ensure the helper lives near other CLI
printing utilities, accepts (&str or String) listener_id and the http3 runtime
struct (match its type), converts the fields to strings as currently done, and
preserves all metric names exactly so other output paths remain consistent.
In `@crates/rginx-app/src/main.rs`:
- Around line 48-74: Extract the duplicated CheckSummary construction into a
single helper function (e.g., fn build_check_summary(config: &Config) ->
CheckSummary) and call it from both the check and -t branches instead of
duplicating the struct literal; move the http3_early_data_enabled_listeners
iterator logic (the .iter().filter(|listener|
listener.http3.as_ref().is_some_and(|http3| http3.early_data_enabled)).count())
and all other fields (listener_model, listener_count, listener_binding_count,
listeners, tls_enabled, http3_enabled, total_vhost_count, total_route_count,
upstream_count, worker_threads, accept_workers, tls) into that helper so future
fields are changed in one place and both branches simply call
build_check_summary(&config) when invoking print_check_success.
- Around line 576-578: The code compares binding.kind via its string form;
replace the string comparison with an enum comparison (e.g., use
matches!(binding.kind, ListenerTransportKind::Udp) or direct equality against
ListenerTransportKind::Udp) when setting reuse_port_enabled so it’s consistent
with other code (see ListenerTransportKind::Udp) and still combine with
config.runtime.accept_workers > 1 for the .then_some(...) logic; update the
expression that computes reuse_port_enabled (which references binding.kind and
config.runtime.accept_workers) to use the enum-based check.
In `@crates/rginx-config/src/compile/tests.rs`:
- Around line 2248-2258: The test's expected Http3Config currently lists
early_data but does not assert the runtime default; update the test in
crates/rginx-config/src/compile/tests.rs (both occurrences around the shown diff
and the one at 2286-2292) to explicitly assert that Http3Config::early_data is
None (i.e., the default is disabled) by adding a check against None for the
early_data field when validating the compiled config so the test will fail if
the default flips to enabled.
In `@crates/rginx-core/src/config/tests.rs`:
- Around line 148-177: The test sets early_data_enabled: false on the UDP
listener but doesn't assert the mapped binding exposes it; add an assertion that
the binding produced by listener.transport_bindings() reflects this flag (e.g.
check bindings[1] for the http3 early-data field — something like
bindings[1].http3_early_data_enabled == Some(false) or the appropriate field
name used by the binding struct) to ensure early_data_enabled is covered by the
test.
In `@crates/rginx-http/src/handler/dispatch.rs`:
- Line 61: Create a small newtype wrapper for the extension instead of using a
raw bool: define a simple struct EarlyDataFlag(pub bool) (derive Copy, Clone,
Debug) and update any places that insert the extension to use
EarlyDataFlag(true/false). Then change the retrieval in dispatch (where
early_data is set from request.extensions()) to use
request.extensions().get::<EarlyDataFlag>().map(|f| f.0).unwrap_or(false)
(replacing get::<bool>().copied().unwrap_or(false)) so the extension key is
type-safe and avoids collisions.
In `@crates/rginx-http/src/handler/mod.rs`:
- Around line 44-50: 当前实现把 0-RTT 的标记直接以裸 bool 插入 Request 的 extensions（在
attach_connection_metadata 中使用 connection.early_data），这会与其它 bool 扩展冲突；请定义一个专用
newtype（例如 EarlyData(bool)）并在 attach_connection_metadata 用
request.extensions_mut().insert(EarlyData(connection.early_data)) 插入；同时更新读取方改为
request.extensions().get::<EarlyData>().map(|v| v.0).unwrap_or(false)；保持对
tls_client_identity 的处理不变（仍在 attach_connection_metadata 中插入其具体类型）。
In `@crates/rginx-http/src/proxy/clients/http3.rs`:
- Around line 414-418: The current is_clean_http3_response_shutdown function
relies on Display text matching which is brittle; change it to inspect concrete
error types/enums instead: update the function signature
(is_clean_http3_response_shutdown) to accept &dyn std::error::Error (or the
concrete error type used in call sites), attempt downcasting
(error.downcast_ref::<h3::Error>() or downcast_ref::<quinn::ConnectionError>() /
quinn::TransportError>() as appropriate) and match the structured variants (e.g.
h3::Error::ApplicationClose or Error::Application { code } and compare against
h3::ErrorCode::H3_NO_ERROR or the quinn close-code enum) to return true for
clean shutdowns; keep a Display fallback only as a last resort and update all
call sites to pass the error as an error trait object or concrete type.
In `@crates/rginx-http/src/state/connections.rs`:
- Around line 314-416: All these record_http3_* methods repeat the same
snapshot/notification sequence; extract a helper like
bump_http3_counter_and_snapshot(listener_id: &str, op: impl
FnOnce(&ListenerTrafficCounters)) that (1) looks up
listener_traffic_counters(listener_id), (2) runs the provided closure to
increment the specific atomic counter, and (3) calls
mark_snapshot_changed_components(true, false, true, false, false),
mark_traffic_targets_changed(..., Some(listener_id), None, None) and
notify_snapshot_waiters(); then refactor record_http3_retry_issued,
record_http3_retry_failed, record_http3_request_* and
record_http3_response_stream_error to call this helper with a small closure that
increments the right field, and refactor record_http3_connection_close to either
call the same helper inside each match arm or use the helper with a closure that
matches on the quinn::ConnectionError and increments the appropriate counter.
In `@crates/rginx-http/src/state/lifecycle.rs`:
- Around line 28-79: Multiple separate iterations over config.listeners to
collect HTTP/3 counters should be replaced with a single loop/fold to avoid
repeated lookups and reduce lock/CPU overhead in status_snapshot(); implement by
iterating once over config.listeners (or using an iterator.fold) and for each
listener do listener_traffic.listeners.get(&listener.id).map(|entry| { increment
local accumulators: http3_active_connections, http3_active_request_streams,
http3_retry_issued_total, http3_retry_failed_total,
http3_request_accept_errors_total, http3_request_resolve_errors_total,
http3_request_body_stream_errors_total, http3_response_stream_errors_total using
the same .load(...) calls }); at the end assign or return the accumulated
totals; keep the existing variable names (listener_traffic,
http3_active_connections, http3_active_request_streams,
http3_retry_issued_total, http3_retry_failed_total,
http3_request_accept_errors_total, http3_request_resolve_errors_total,
http3_request_body_stream_errors_total, http3_response_stream_errors_total) so
callers like status_snapshot() continue to work.
In `@scripts/run-http3-release-gate.sh`:
- Around line 88-92: The --netem-profile parsing currently only assigns
NETEM_PROFILE without validating allowed values; add an explicit enum check
after parsing NETEM_PROFILE (e.g., compare NETEM_PROFILE against the documented
allowed values) and call die with a clear error if it is not one of them. Update
the parsing branch that sets NETEM_PROFILE (and the other occurrence around the
same code) to perform this validation immediately after shift, using the
existing die helper for failure messages and referencing NETEM_PROFILE in the
error so users see the invalid value and the allowed set.
🪄 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: 129e9f02-a8bc-4f69-889c-9f4928d3b2fc
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (56)
.github/workflows/release.ymlARCHITECTURE_HTTP3_PHASE0_FREEZE.mdARCHITECTURE_HTTP3_PLAN.mdARCHITECTURE_REMEDIATION_PLAN.mdARCHITECTURE_REMEDIATION_RELEASE_NOTE.mdCargo.tomlREADME.mdRELEASE_NOTES_v0.1.3-rc.11.mdcrates/rginx-app/src/admin_cli/counters.rscrates/rginx-app/src/admin_cli/status.rscrates/rginx-app/src/admin_cli/traffic.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/http3.rscrates/rginx-app/tests/support/mod.rscrates/rginx-config/src/compile/route.rscrates/rginx-config/src/compile/server.rscrates/rginx-config/src/compile/tests.rscrates/rginx-config/src/model.rscrates/rginx-config/src/validate/server.rscrates/rginx-config/src/validate/tests.rscrates/rginx-core/src/config.rscrates/rginx-core/src/config/route.rscrates/rginx-core/src/config/tests.rscrates/rginx-http/Cargo.tomlcrates/rginx-http/src/client_ip.rscrates/rginx-http/src/handler/dispatch.rscrates/rginx-http/src/handler/mod.rscrates/rginx-http/src/handler/response.rscrates/rginx-http/src/handler/tests.rscrates/rginx-http/src/proxy/clients/http3.rscrates/rginx-http/src/router.rscrates/rginx-http/src/server/connection.rscrates/rginx-http/src/server/http3.rscrates/rginx-http/src/state/connections.rscrates/rginx-http/src/state/counters.rscrates/rginx-http/src/state/helpers.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/listeners.rscrates/rginx-http/src/state/traffic.rscrates/rginx-http/src/tls/acceptor.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/shutdown.rscrates/rginx-runtime/src/restart.rsscripts/prepare-release.shscripts/run-http3-gate.shscripts/run-http3-phase0.shscripts/run-http3-release-gate.shscripts/run-http3-soak.sh
💤 Files with no reviewable changes (4)
- ARCHITECTURE_HTTP3_PLAN.md
- ARCHITECTURE_HTTP3_PHASE0_FREEZE.md
- ARCHITECTURE_REMEDIATION_RELEASE_NOTE.md
- ARCHITECTURE_REMEDIATION_PLAN.md
👮 Files not reviewed due to content moderation or server errors (8)
- crates/rginx-app/src/admin_cli/status.rs
- crates/rginx-app/tests/support/mod.rs
- crates/rginx-http/src/state/tests.rs
- crates/rginx-core/src/config.rs
- crates/rginx-runtime/src/restart.rs
- scripts/run-http3-soak.sh
- scripts/run-http3-phase0.sh
- crates/rginx-config/src/validate/tests.rs
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/rginx-http/src/tls/acceptor.rs (1)
64-74:⚠️ Potential issue | 🔴 CriticalClippy 构建失败：参数过多
Pipeline 日志显示
build_tls_server_config函数有 9 个参数，超过了 Clippy 的限制（8 个）。建议将相关参数重构为配置结构体。🔧 建议的重构方案
+struct TlsServerConfigOptions<'a> { + default_tls: Option<&'a ServerTls>, + default_certificate: Option<&'a str>, + tls_termination_enabled: bool, + default_vhost: &'a VirtualHost, + vhosts: &'a [VirtualHost], + alpn_protocols: Vec<String>, + http3_only: bool, + max_request_body_bytes: Option<usize>, + http3_early_data_enabled: bool, +} -fn build_tls_server_config( - default_tls: Option<&ServerTls>, - default_certificate: Option<&str>, - tls_termination_enabled: bool, - default_vhost: &VirtualHost, - vhosts: &[VirtualHost], - alpn_protocols: Vec<String>, - http3_only: bool, - max_request_body_bytes: Option<usize>, - http3_early_data_enabled: bool, -) -> Result<Option<Arc<ServerConfig>>> { +fn build_tls_server_config(options: TlsServerConfigOptions<'_>) -> Result<Option<Arc<ServerConfig>>> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/tls/acceptor.rs` around lines 64 - 74, The function build_tls_server_config currently takes nine parameters and triggers Clippy's "too many arguments" lint; create a new struct (e.g., TlsServerConfig or BuildTlsConfig) that groups the related fields (default_tls: Option<&ServerTls>, default_certificate: Option<&str>, tls_termination_enabled: bool, default_vhost: &VirtualHost, vhosts: &[VirtualHost], alpn_protocols: Vec<String>, http3_only: bool, max_request_body_bytes: Option<usize>, http3_early_data_enabled: bool), replace the nine-arg signature with build_tls_server_config(cfg: &TlsServerConfig) -> Result<Option<Arc<ServerConfig>>>, and update all call sites to construct and pass the new struct; keep the inner logic of build_tls_server_config and the types ServerTls, VirtualHost, ServerConfig unchanged.
♻️ Duplicate comments (1)
crates/rginx-runtime/src/bootstrap/listeners.rs (1)
86-95:⚠️ Potential issue | 🟠 Major当 HTTP/3 accept_workers 从 1 扩容到 N 时，继承的 socket 与新绑定 socket 的 SO_REUSEPORT 设置不匹配会导致热重启失败
当上一代
accept_workers=1时，继承的 UDP socket 由bind_std_udp_sockets(..., 1)创建，会传递reuse_port=false。在热重启时若accept_workers增加到 N（N>1），代码会尝试以reuse_port=true绑定新 socket，但在 Linux 上无法将开启SO_REUSEPORT的 socket 绑定到已被占用但未启用SO_REUSEPORT的地址，导致EADDRINUSE失败。需在sockets.len() == 1 && desired_udp_socket_count > 1时明确拒绝，避免将配置变更变成隐式的 bind 失败。建议的修复
Some(sockets) => { let mut sockets = sockets.into_iter().map(Arc::new).collect::<Vec<_>>(); + if sockets.len() == 1 && desired_udp_socket_count > 1 { + return Err(Error::Server(format!( + "listener `{}` cannot increase HTTP/3 accept_workers from 1 to {} during restart; do a cold restart or keep the previous worker count", + listener.name, + desired_udp_socket_count, + ))); + } if sockets.len() > desired_udp_socket_count { sockets.truncate(desired_udp_socket_count); } else if sockets.len() < desired_udp_socket_count {🤖 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 86 - 95, When upgrading from a single inherited UDP socket to multiple listeners, the code in the Some(sockets) branch must detect the incompatible SO_REUSEPORT mismatch and fail early instead of attempting new binds; add a guard that if sockets.len() == 1 && desired_udp_socket_count > 1 then return an explicit error (with a clear message) rather than calling bind_std_udp_sockets_with_reuse_port(http3.listen_addr, ...). Ensure this check sits before truncation/extension logic so bind_std_udp_sockets_with_reuse_port is only invoked when all existing sockets already match the intended reuse_port semantics.
🧹 Nitpick comments (1)
crates/rginx-config/src/compile/tests.rs (1)
2295-2384: 建议补充early_data显式开启分支，避免只覆盖默认分支。当前这个用例在 Line 2350 仍使用
early_data: None，建议直接在该测试里覆盖Some(true)，并断言early_data_enabled == true。♻️ 建议修改
http3: Some(Http3Config { listen: Some("127.0.0.1:9443".to_string()), advertise_alt_svc: Some(false), alt_svc_max_age_secs: Some(7200), max_concurrent_streams: Some(256), stream_buffer_size_bytes: Some(131072), active_connection_id_limit: Some(5), retry: Some(true), host_key_path: Some("quic/host.key".to_string()), gso: Some(true), - early_data: None, + early_data: Some(true), }), @@ assert_eq!(http3.host_key_path, Some(base_dir.path().join("quic/host.key"))); assert!(http3.gso); + assert!(http3.early_data_enabled); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-config/src/compile/tests.rs` around lines 2295 - 2384, Test compile_http3_applies_transport_settings_and_resolves_host_key_path sets Http3Config.early_data to None which only verifies default behavior; change the test to set Http3Config { ..., early_data: Some(true), ... } and add an assertion that the compiled snapshot's http3.early_data_enabled (or the equivalent field in the compiled Http3 struct) is true to explicitly test enabling early data; update the test function compile_http3_applies_transport_settings_and_resolves_host_key_path and the Http3Config instance and add the assert on http3.early_data_enabled.
🤖 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-http/src/proxy/clients/http3.rs`:
- Around line 191-205: The code currently releases the sessions lock before
calling connect_session(&key, peer_url) allowing concurrent callers to each
start a connection; fix by introducing an in-progress marker so only one
connect_session runs per Http3SessionKey: when sessions lock is held, check for
an existing non-closed session (sessions.get(&key).is_closed()), and if missing
insert a placeholder (e.g., a Pending entry implemented with a oneshot/Notify or
a shared JoinHandle/Future) into self.sessions to indicate "connecting", then
drop the lock and run connect_session(&key, peer_url); upon success replace the
placeholder with the real Arc session (or send it via the oneshot), and ensure
awaiters that see the Pending entry wait for the result instead of starting
their own connect_session; keep using self.sessions, connect_session,
Http3SessionKey, and is_closed to locate the relevant logic.
- Around line 296-304: StreamingResponseBody currently spawns a background task
that awaits request_stream.recv_data() but does not keep its JoinHandle, so
dropping the body won't cancel that task; modify StreamingResponseBody to hold
an Option<JoinHandle<()>> (e.g. field name join_handle), set that handle when
creating the body (where tokio::spawn is called), and implement Drop for
StreamingResponseBody to take() the join_handle and call abort() to cancel the
background task immediately (also consider joining/ignoring the join result if
needed); keep existing rx/size_hint/done behavior but ensure the spawned task
uses the same tx/rx and is aborted on drop to free HTTP/3 streams promptly.
In `@crates/rginx-http/src/server/http3.rs`:
- Around line 392-410: When http3.active_connection_id_limit equals
HTTP3_ACTIVE_CONNECTION_ID_LIMIT_MIGRATION, do not silently skip configuring a
hashed CID generator when host_key_material is None; instead detect
host_key_material == None in that branch (where derive_hashed_connection_id_key
and endpoint_config.cid_generator / quinn_proto::HashedConnectionIdGenerator are
used) and return an explicit Err (or at minimum log a warning and return Err)
indicating that host_key_path/host_key_material is required for migration mode
so callers cannot accidentally run without a stable HashedConnectionIdGenerator.
In `@scripts/run-http3-release-gate.sh`:
- Line 93: The error message in the die call uses backtick-style command
substitution around `${NETEM_PROFILE}` which can lead to command injection;
update the die invocation in the case fallback (the line using die
"--netem-profile must be one of: ... (got `${NETEM_PROFILE}`)") to avoid command
substitution by printing the variable literally and safely—e.g., stop using
backticks and instead include the variable via proper quoting (such as
"'$NETEM_PROFILE'" or printf-style safe expansion) so the NETEM_PROFILE value is
shown without executing it.
In `@scripts/run-http3-soak.sh`:
- Around line 141-143: 当前逻辑在 need_privileged=1 时无条件调用 ip 去读取 orig_mtu，导致在没有 ip
命令的环境（比如仅启用 netem 场景）失败；请在使用 ip 命令前加上可用性检查，修改该判断为同时满足 orig_mtu
为空、need_privileged=1 且 ip 命令存在（例如通过 command -v ip 或 command -v ip >/dev/null
检查）后再运行 ip -o link show dev "${NETEM_DEV}" | awk ...；保留变量名
orig_mtu、need_privileged 和 NETEM_DEV 不变以便定位。
---
Outside diff comments:
In `@crates/rginx-http/src/tls/acceptor.rs`:
- Around line 64-74: The function build_tls_server_config currently takes nine
parameters and triggers Clippy's "too many arguments" lint; create a new struct
(e.g., TlsServerConfig or BuildTlsConfig) that groups the related fields
(default_tls: Option<&ServerTls>, default_certificate: Option<&str>,
tls_termination_enabled: bool, default_vhost: &VirtualHost, vhosts:
&[VirtualHost], alpn_protocols: Vec<String>, http3_only: bool,
max_request_body_bytes: Option<usize>, http3_early_data_enabled: bool), replace
the nine-arg signature with build_tls_server_config(cfg: &TlsServerConfig) ->
Result<Option<Arc<ServerConfig>>>, and update all call sites to construct and
pass the new struct; keep the inner logic of build_tls_server_config and the
types ServerTls, VirtualHost, ServerConfig unchanged.
---
Duplicate comments:
In `@crates/rginx-runtime/src/bootstrap/listeners.rs`:
- Around line 86-95: When upgrading from a single inherited UDP socket to
multiple listeners, the code in the Some(sockets) branch must detect the
incompatible SO_REUSEPORT mismatch and fail early instead of attempting new
binds; add a guard that if sockets.len() == 1 && desired_udp_socket_count > 1
then return an explicit error (with a clear message) rather than calling
bind_std_udp_sockets_with_reuse_port(http3.listen_addr, ...). Ensure this check
sits before truncation/extension logic so bind_std_udp_sockets_with_reuse_port
is only invoked when all existing sockets already match the intended reuse_port
semantics.
---
Nitpick comments:
In `@crates/rginx-config/src/compile/tests.rs`:
- Around line 2295-2384: Test
compile_http3_applies_transport_settings_and_resolves_host_key_path sets
Http3Config.early_data to None which only verifies default behavior; change the
test to set Http3Config { ..., early_data: Some(true), ... } and add an
assertion that the compiled snapshot's http3.early_data_enabled (or the
equivalent field in the compiled Http3 struct) is true to explicitly test
enabling early data; update the test function
compile_http3_applies_transport_settings_and_resolves_host_key_path and the
Http3Config instance and add the assert on http3.early_data_enabled.
🪄 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: 3715897a-c530-43e7-9618-75466a6b51c5
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
crates/rginx-app/Cargo.tomlcrates/rginx-app/src/main.rscrates/rginx-config/src/compile/tests.rscrates/rginx-core/src/config/tests.rscrates/rginx-http/src/handler/dispatch.rscrates/rginx-http/src/handler/mod.rscrates/rginx-http/src/proxy/clients/http3.rscrates/rginx-http/src/server/http3.rscrates/rginx-http/src/state/connections.rscrates/rginx-http/src/state/lifecycle.rscrates/rginx-http/src/state/tests.rscrates/rginx-http/src/tls/acceptor.rscrates/rginx-runtime/src/bootstrap/listeners.rsscripts/run-http3-release-gate.shscripts/run-http3-soak.sh
✅ Files skipped from review due to trivial changes (2)
- crates/rginx-app/Cargo.toml
- crates/rginx-http/src/state/connections.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/rginx-http/src/handler/mod.rs
- crates/rginx-http/src/handler/dispatch.rs
- crates/rginx-app/src/main.rs
- crates/rginx-http/src/state/lifecycle.rs
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/rginx-config/src/compile/tests.rs (1)
107-107: 建议抽取LocationConfig测试构造器，减少重复样板。这批
allow_early_data: None的补齐是正确的，但现在 fixture 重复度很高，后续模型再加字段会继续放大改动面。建议在本测试文件内抽一个默认构造 helper。♻️ 可选重构示例
+fn test_location(matcher: MatcherConfig, handler: HandlerConfig) -> LocationConfig { + LocationConfig { + matcher, + handler, + grpc_service: None, + grpc_method: None, + allow_cidrs: Vec::new(), + deny_cidrs: Vec::new(), + requests_per_sec: None, + burst: None, + allow_early_data: None, + } +}- locations: vec![LocationConfig { - matcher: MatcherConfig::Prefix("/api".to_string()), - handler: HandlerConfig::Proxy { - upstream: "secure-backend".to_string(), - preserve_host: None, - strip_prefix: None, - proxy_set_headers: std::collections::HashMap::new(), - }, - grpc_service: None, - grpc_method: None, - allow_cidrs: Vec::new(), - deny_cidrs: Vec::new(), - requests_per_sec: None, - burst: None, - allow_early_data: None, - }], + locations: vec![test_location( + MatcherConfig::Prefix("/api".to_string()), + HandlerConfig::Proxy { + upstream: "secure-backend".to_string(), + preserve_host: None, + strip_prefix: None, + proxy_set_headers: std::collections::HashMap::new(), + }, + )],Also applies to: 224-224, 319-319, 426-426, 522-522, 633-633, 737-737, 835-835, 940-940, 1047-1047, 1151-1151, 1252-1252, 1346-1346, 1436-1436, 1487-1487, 1539-1539, 1593-1593, 1612-1612, 1691-1691, 1764-1764, 1848-1848, 1902-1902, 1955-1955, 2027-2027, 2042-2042, 2105-2105, 2189-2189, 2275-2275, 2367-2367
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-config/src/compile/tests.rs` at line 107, Tests repeat constructing LocationConfig (many places setting allow_early_data: None) causing boilerplate; add a single test helper that returns a default LocationConfig (e.g., fn default_location_config() -> LocationConfig or impl LocationConfig { fn test_default() -> Self }) that sets common fields including allow_early_data: None, then replace repeated literal constructions with struct update syntax (LocationConfig { specific_field: X, ..default_location_config() }) or call the helper and modify fields as needed; update all occurrences (reference symbol: LocationConfig) in this test file to use the helper to reduce duplication.crates/rginx-runtime/src/bootstrap/listeners.rs (1)
615-628: 建议把热重启归一化分支也补成测试。当前新增测试只覆盖 fresh bind 的
3 sockets路径，但这次改动里最容易回归的是 reload 时的三条分支：继承 socket 截断、补齐，以及1 -> N的 fail fast。建议至少把这三条路径补齐，否则这部分问题只能在热重启时才暴露。🤖 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 615 - 628, The new test only covers the fresh-bind path for bind_std_udp_sockets; add unit tests that exercise the hot-restart (reload) branches: (1) inherited socket truncation when more inherited sockets exist than workers, (2) filling up when fewer inherited sockets exist than workers, and (3) the fail-fast transition when going from 1 existing socket to N workers. Locate bind_std_udp_sockets and bind_std_udp_socket and write three tests that simulate inherited sockets (create seed sockets via bind_std_udp_socket or otherwise simulate the inherited set), call bind_std_udp_sockets in reload mode, and assert the resulting socket list length and local_addr behavior for each case to ensure truncation, filling, and fail-fast behave as expected.
🤖 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-config/src/validate/server.rs`:
- Around line 421-439: The current validation only rejects tls.session_tickets
== Some(true) when http3.early_data is enabled, but leaves None through (which
defaults to enabling tickets at runtime), so update the check in the
http3.early_data block to require tls.session_tickets be explicitly Some(false)
(i.e. reject None and Some(true)); locate the block referencing http3.early_data
and tls.session_tickets and change the conditional to error unless
tls.session_tickets == Some(false), keeping the existing Error::Config message
(or adjust wording to mention that session_tickets must be explicitly false).
In `@scripts/run-http3-soak.sh`:
- Around line 128-133: 函数 clear_faults 在 need_privileged=1 时无条件执行 tc qdisc
del，这会在用户仅传 --mtu（NETEM_PROFILE=none）时错误地删除接口的 root qdisc；修改 clear_faults：在判断
need_privileged 之前或内部，先检查 NETEM_PROFILE 是否为 "none"，仅当 NETEM_PROFILE !=
"none"（即确实使用 netem）时才运行 tc qdisc del dev "${NETEM_DEV}" root；保留对 orig_mtu
的恢复逻辑和现有重定向/容错（>/dev/null 2>&1 || true），并确保对变量
NETEM_DEV/NETEM_PROFILE/need_privileged 的引用与现有实现一致。
---
Nitpick comments:
In `@crates/rginx-config/src/compile/tests.rs`:
- Line 107: Tests repeat constructing LocationConfig (many places setting
allow_early_data: None) causing boilerplate; add a single test helper that
returns a default LocationConfig (e.g., fn default_location_config() ->
LocationConfig or impl LocationConfig { fn test_default() -> Self }) that sets
common fields including allow_early_data: None, then replace repeated literal
constructions with struct update syntax (LocationConfig { specific_field: X,
..default_location_config() }) or call the helper and modify fields as needed;
update all occurrences (reference symbol: LocationConfig) in this test file to
use the helper to reduce duplication.
In `@crates/rginx-runtime/src/bootstrap/listeners.rs`:
- Around line 615-628: The new test only covers the fresh-bind path for
bind_std_udp_sockets; add unit tests that exercise the hot-restart (reload)
branches: (1) inherited socket truncation when more inherited sockets exist than
workers, (2) filling up when fewer inherited sockets exist than workers, and (3)
the fail-fast transition when going from 1 existing socket to N workers. Locate
bind_std_udp_sockets and bind_std_udp_socket and write three tests that simulate
inherited sockets (create seed sockets via bind_std_udp_socket or otherwise
simulate the inherited set), call bind_std_udp_sockets in reload mode, and
assert the resulting socket list length and local_addr behavior for each case to
ensure truncation, filling, and fail-fast behave as expected.
🪄 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: 834001f3-4f09-45f9-b798-5aa5f44444b9
📒 Files selected for processing (8)
crates/rginx-config/src/compile/tests.rscrates/rginx-config/src/validate/server.rscrates/rginx-http/src/proxy/clients/http3.rscrates/rginx-http/src/server/http3.rscrates/rginx-http/src/tls/acceptor.rscrates/rginx-runtime/src/bootstrap/listeners.rsscripts/run-http3-release-gate.shscripts/run-http3-soak.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/run-http3-release-gate.sh
- crates/rginx-http/src/server/http3.rs
- crates/rginx-http/src/tls/acceptor.rs
Comment on lines
+421
to
+439
| if matches!(http3.early_data, Some(true)) { | ||
| if matches!(tls.session_resumption, Some(false)) { | ||
| return Err(Error::Config(format!( | ||
| "{owner_label} http3 early_data requires tls session_resumption to remain enabled" | ||
| ))); | ||
| } | ||
|
|
||
| if tls.session_cache_size.is_some_and(|size| size == 0) { | ||
| return Err(Error::Config(format!( | ||
| "{owner_label} http3 early_data requires tls session_cache_size to remain greater than 0" | ||
| ))); | ||
| } | ||
|
|
||
| if matches!(tls.session_tickets, Some(true)) { | ||
| return Err(Error::Config(format!( | ||
| "{owner_label} http3 early_data requires tls session_tickets to remain disabled so stateful resumption is used" | ||
| ))); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 目的：确认 session_tickets/session_resumption/session_cache_size 在“未配置(None)”时的最终默认行为
# 预期：若 early_data 要求“tickets 必须禁用”，则应能证明 None 最终落到 disabled；否则需要在校验中要求 Some(false)。
echo "== 查找 TLS 配置字段定义 =="
rg -n -C3 'struct ServerTlsConfig|session_tickets|session_resumption|session_cache_size' crates/rginx-config/src/model.rs
echo
echo "== 查找 validate_http3 相关逻辑 =="
rg -n -C4 'early_data|session_tickets|session_resumption|session_cache_size' crates/rginx-config/src/validate/server.rs
echo
echo "== 查找编译阶段默认值/回填逻辑 =="
rg -n -C4 'session_tickets|session_resumption|session_cache_size|unwrap_or|unwrap_or_else' crates/rginx-config/src/compile/server.rs crates/rginx-app/src/admin_cli/*.rs 2>/dev/null || trueRepository: vansour/rginx
Length of output: 48060
🏁 Script executed:
# 查找运行时 TLS 配置应用和 rustls 初始化逻辑
echo "== 查找 TLS 配置在运行时的应用（rginx-http 或 transport 层） =="
find crates -name "*.rs" -type f | xargs rg -l 'session_tickets|ServerTlsIdentity' | head -10
echo
echo "== 查找 rustls 配置或 TLS 参数应用 =="
rg -n 'ServerConfig|session_tickets|session_resumption' crates/rginx-http/src --type rust 2>/dev/null | head -30
echo
echo "== 查找编译后 TLS 配置到 rustls ServerConfig 的映射 =="
rg -n -A5 'fn.*tls|rustls::ServerConfig|session' crates/rginx-http/src/tls.rs 2>/dev/null | head -50Repository: vansour/rginx
Length of output: 3172
🏁 Script executed:
# 查看 session.rs 的完整逻辑
cat -n crates/rginx-http/src/tls/session.rsRepository: vansour/rginx
Length of output: 2272
HTTP/3 early_data 约束对 session_tickets=None 的验证不完整。
第434行仅拒绝 Some(true)，但当 session_tickets 未设置（为 None）时，运行时在 crates/rginx-http/src/tls/session.rs 第33行的逻辑中不会应用显式禁用，导致 rustls 使用默认行为（启用 session tickets）。这与注释目标"禁用 tickets 以启用有状态恢复"相悖。
建议将验证改为要求 Some(false) 或在 early_data=true 时禁止 None 值，以确保配置合规。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/rginx-config/src/validate/server.rs` around lines 421 - 439, The
current validation only rejects tls.session_tickets == Some(true) when
http3.early_data is enabled, but leaves None through (which defaults to enabling
tickets at runtime), so update the check in the http3.early_data block to
require tls.session_tickets be explicitly Some(false) (i.e. reject None and
Some(true)); locate the block referencing http3.early_data and
tls.session_tickets and change the conditional to error unless
tls.session_tickets == Some(false), keeping the existing Error::Config message
(or adjust wording to mention that session_tickets must be explicitly false).
Owner
Author
There was a problem hiding this comment.
Verified locally against the current rustls/quinn integration: requiring tls.session_tickets = Some(false) for http3.early_data makes 0-RTT resumption unavailable and breaks routes_http3_early_data_by_replay_safety as well as the CI http3 gate. Leaving the validation as-is here is intentional until the runtime/session policy changes to support that configuration.
There was a problem hiding this comment.
@vansour, understood — thank you for the detailed verification. Since session_tickets = None at the config layer is compatible with the 0-RTT resumption path in the current rustls/quinn integration (and enforcing Some(false) actively breaks it), keeping the validation at Some(true) rejection only is the right call for now. I'll note this for future reviews.
✏️ Learnings added
Learnt from: vansour
Repo: vansour/rginx PR: 43
File: crates/rginx-config/src/validate/server.rs:421-439
Timestamp: 2026-04-13T10:53:27.443Z
Learning: In `crates/rginx-config/src/validate/server.rs` (`validate_http3`), the check for `tls.session_tickets` when `http3.early_data = true` intentionally only rejects `Some(true)` and allows `None`. Requiring `Some(false)` breaks 0-RTT resumption and the `routes_http3_early_data_by_replay_safety` test and the CI `http3` gate in the current rustls/quinn integration. This is a deliberate design decision and should not be flagged as a bug until the runtime/session policy is updated to support that configuration.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/rginx-config/src/compile/tests.rs (2)
2254-2260: HTTP/3 默认值断言建议避免魔法数字。这里建议改为断言
super中的默认常量，避免默认策略调整时测试同时散点修改。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-config/src/compile/tests.rs` around lines 2254 - 2260, The assertions use magic numbers for HTTP/3 defaults; update the tests to reference the canonical default constants from the parent module instead of literals—replace assertions on http3.max_concurrent_streams, http3.stream_buffer_size, http3.active_connection_id_limit, http3.retry, http3.host_key_path, http3.gso, and http3.early_data_enabled to compare against the corresponding constants in super (e.g. super::DEFAULT_MAX_CONCURRENT_STREAMS, super::DEFAULT_STREAM_BUFFER_SIZE, super::DEFAULT_ACTIVE_CONNECTION_ID_LIMIT, and the boolean/default constants defined in the super module) so the test follows the module's single source of truth for defaults.
299-316: 建议继续用test_location收敛剩余样板LocationConfig。Line 299 这类仍在手写默认字段的块可以继续复用 helper，减少重复和漏字段风险。
可选改法示例
- locations: vec![LocationConfig { - matcher: MatcherConfig::Prefix("/".to_string()), - handler: HandlerConfig::Proxy { - upstream: "backend".to_string(), - preserve_host: None, - strip_prefix: None, - proxy_set_headers: std::collections::HashMap::new(), - }, - grpc_service: None, - grpc_method: None, - allow_cidrs: Vec::new(), - deny_cidrs: Vec::new(), - requests_per_sec: None, - burst: None, - allow_early_data: None, - }], + locations: vec![test_location( + MatcherConfig::Prefix("/".to_string()), + HandlerConfig::Proxy { + upstream: "backend".to_string(), + preserve_host: None, + strip_prefix: None, + proxy_set_headers: std::collections::HashMap::new(), + }, + )],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-config/src/compile/tests.rs` around lines 299 - 316, Replace the manually-constructed LocationConfig block with the existing test_location helper to remove repetitive default fields: locate the vec![LocationConfig { ... }] instance (using MatcherConfig::Prefix("/") and HandlerConfig::Proxy { upstream: "backend", ... }) and call test_location with the prefix matcher and a proxy handler descriptor (provide upstream "backend" and any non-default proxy fields like proxy_set_headers), letting test_location supply defaults for grpc_service, grpc_method, allow_cidrs/deny_cidrs, requests_per_sec, burst, and allow_early_data; ensure the helper invocation matches the helper's signature so all previously handwritten fields are omitted.
🤖 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-config/src/compile/tests.rs`:
- Around line 2254-2260: The assertions use magic numbers for HTTP/3 defaults;
update the tests to reference the canonical default constants from the parent
module instead of literals—replace assertions on http3.max_concurrent_streams,
http3.stream_buffer_size, http3.active_connection_id_limit, http3.retry,
http3.host_key_path, http3.gso, and http3.early_data_enabled to compare against
the corresponding constants in super (e.g.
super::DEFAULT_MAX_CONCURRENT_STREAMS, super::DEFAULT_STREAM_BUFFER_SIZE,
super::DEFAULT_ACTIVE_CONNECTION_ID_LIMIT, and the boolean/default constants
defined in the super module) so the test follows the module's single source of
truth for defaults.
- Around line 299-316: Replace the manually-constructed LocationConfig block
with the existing test_location helper to remove repetitive default fields:
locate the vec![LocationConfig { ... }] instance (using
MatcherConfig::Prefix("/") and HandlerConfig::Proxy { upstream: "backend", ...
}) and call test_location with the prefix matcher and a proxy handler descriptor
(provide upstream "backend" and any non-default proxy fields like
proxy_set_headers), letting test_location supply defaults for grpc_service,
grpc_method, allow_cidrs/deny_cidrs, requests_per_sec, burst, and
allow_early_data; ensure the helper invocation matches the helper's signature so
all previously handwritten fields are omitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f4c3b73b-e648-4fb7-82a6-00cd3a807168
📒 Files selected for processing (1)
crates/rginx-config/src/compile/tests.rs
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
vansour
changed the title
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
Superseded by #44. The validated fixes were merged into main and released as v0.1.3-rc.11. |
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.
This is a temporary review-only PR that reproduces the original diff from PR #41 against the pre-merge base commit.
Reason:
Intent:
Original PR: #41