Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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 2 minutes and 31 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 (2)
📝 WalkthroughWalkthrough此 PR 移除 nginx-migrate 功能、改为基于监听器的快照与逐监听器 admin 输出，统一主机/SNI 最佳匹配规则，重构响应最终化管道（包含压缩与 Vary 行为）、TLS 绑定逻辑与快照模式，更新打包/服务单元与基准脚本，并新增架构整改计划与发布说明文档。 Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 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
Prepares v0.1.3-rc.9 by completing the architecture remediation plan (phases 0–6), aligning host/SNI best-match semantics across routing and TLS diagnostics, hardening the downstream response finalization pipeline, and tightening release/packaging + benchmarking gates.
Changes:
- Unify HTTP host routing and TLS SNI selection via best-match semantics; update TLS binding diagnostics and control-plane listener reporting (admin schema v11).
- Harden response finalization (compression/Vary handling, HEAD behavior, gRPC observability pipeline ordering) and update associated test coverage.
- Update packaging/systemd integration, and improve the nginx comparison harness (rounds/medians, warmups, ulimit/no-file handling, docker harness tweaks).
Reviewed changes
Copilot reviewed 65 out of 68 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/run-tls-gate.sh | Removes migrate test from TLS gate run list. |
| scripts/run-nginx-compare-docker.sh | Adds higher nofile ulimit for docker benchmark runs. |
| scripts/nginx_compare/scenarios.py | Adds multi-round collection + median aggregation; grpc backend binds on ephemeral port. |
| scripts/nginx_compare/render.py | Reports medians across rounds; updates benchmark tool description. |
| scripts/nginx_compare/main.py | Adds --rounds; increases default requests. |
| scripts/nginx_compare/launch.py | Replaces ab with Python HTTP/1.1 keepalive runner; adds warmups + median helpers; improves upstream server. |
| scripts/nginx_compare/configs.py | Scales worker counts; tweaks proxy upstream pool config. |
| scripts/nginx_compare/common.py | Ensures higher RLIMIT_NOFILE; adds benchmark result dataclasses. |
| scripts/nginx_compare/checkout.py | Handles workspaces without git metadata for version stamping. |
| scripts/build-deb.sh | Installs systemd unit into /usr/lib/systemd/system. |
| README.md | Updates install paths + service notes; removes migrate-nginx references. |
| packaging/apt/control.in | Adds init-system-helpers dependency for deb helper scripts. |
| packaging/apt/postinst | Uses deb-systemd helpers; starts/restarts service on install/upgrade. |
| packaging/apt/prerm | Stops service on remove with deb-systemd helpers (narrowed conditions). |
| packaging/apt/postrm | Purges runtime dirs on purge; daemon-reload on remove; systemd helper purge handling. |
| docker/nginx-compare/Dockerfile | Switches rust toolchain install to stable for docker harness. |
| deploy/systemd/rginx.service | Hardens unit: StateDirectory, PIDFile path, capabilities, ExecStartPre -t, stricter write paths. |
| crates/rginx-runtime/src/admin.rs | Bumps admin snapshot schema version to 11. |
| crates/rginx-runtime/src/health.rs | Adjusts tests for new listener-centric snapshot model. |
| crates/rginx-http/src/router.rs | Changes vhost selection to best-match (exact > wildcard specificity). |
| crates/rginx-core/src/config.rs | Adds ServerNameMatch::priority + best_matching_server_name_pattern; updates vhost matching APIs. |
| crates/rginx-core/src/lib.rs | Re-exports best-match helper. |
| crates/rginx-core/src/config/tests.rs | Updates tests for removed ConfigSnapshot.server. |
| crates/rginx-config/src/compile/mod.rs | Removes ConfigSnapshot.server from compiled model. |
| crates/rginx-config/src/compile/tests.rs | Updates compile tests to use first listener server instead of global server. |
| crates/rginx-http/src/state/snapshots.rs | Replaces listen_addr with listeners: Vec<RuntimeListenerSnapshot> in runtime status snapshot. |
| crates/rginx-http/src/state/lifecycle.rs | Populates listener inventory for runtime status snapshots. |
| crates/rginx-http/src/state/tests.rs | Updates snapshot tests to assert listener inventory. |
| crates/rginx-http/src/tls/sni.rs | Uses shared best-match helper for certificate selection; limits wildcard helper to tests. |
| crates/rginx-http/src/state/tls_runtime/bindings.rs | Reworks TLS binding snapshot generation + default-certificate selection logic; adds targeted tests. |
| crates/rginx-http/src/handler/dispatch.rs | Makes response finalization pipeline explicit; uses listener-local access log format. |
| crates/rginx-http/src/handler/tests.rs | Adds tests for best-match host routing and response finalization behaviors. |
| crates/rginx-http/src/compression.rs | Fixes compression fallback metadata; merges Vary instead of overwriting; adds tests. |
| crates/rginx-http/src/proxy/forward/response.rs | Extracts upstream body timeout/deadline wrapper pipeline helper. |
| crates/rginx-http/src/proxy/tests/mod.rs | Updates tests for removed ConfigSnapshot.server. |
| crates/rginx-http/src/proxy/health/registry.rs | Updates tests for removed ConfigSnapshot.server. |
| crates/rginx-http/src/proxy/clients/tests.rs | Updates tests for removed ConfigSnapshot.server. |
| crates/rginx-http/src/transition.rs | Updates tests for removed ConfigSnapshot.server. |
| crates/rginx-http/src/lib.rs | Re-exports new RuntimeListenerSnapshot. |
| crates/rginx-app/src/main.rs | Removes migrate-nginx command; updates check output to include listener inventory. |
| crates/rginx-app/src/cli.rs | Removes migrate-nginx CLI; updates installed PID path under /run/rginx/. |
| crates/rginx-app/src/admin_cli/status.rs | Prints listener inventory for status output. |
| crates/rginx-app/src/admin_cli/mod.rs | Removes migrate-nginx from admin routing. |
| crates/rginx-app/tests/check.rs | Updates assertions for listener inventory output in check. |
| crates/rginx-app/tests/admin/snapshot.rs | Updates schema version + status assertions for listener inventory. |
| crates/rginx-app/tests/admin/commands.rs | Extends status command tests to validate listener inventory output. |
| crates/rginx-app/tests/vhost.rs | Adds end-to-end tests for exact-over-wildcard and wildcard-specificity routing. |
| crates/rginx-app/tests/multi_listener.rs | Adds integration test validating listener-specific access log formats (HTTP + HTTPS). |
| crates/rginx-app/tests/migrate.rs | Removes migrate-nginx integration tests. |
| crates/rginx-app/src/migrate_nginx/mod.rs | Removes migrate-nginx implementation module. |
| crates/rginx-app/src/migrate_nginx/tokenize.rs | Removes migrate-nginx tokenizer. |
| crates/rginx-app/src/migrate_nginx/parser.rs | Removes migrate-nginx parser. |
| crates/rginx-app/src/migrate_nginx/convert.rs | Removes migrate-nginx converter. |
| crates/rginx-app/src/migrate_nginx/render.rs | Removes migrate-nginx renderer. |
| crates/rginx-app/src/migrate_nginx/tests.rs | Removes migrate-nginx unit tests. |
| Cargo.toml | Bumps rustls-webpki dependency version. |
| Cargo.lock | Updates lockfile for dependency bumps. |
| ARCHITECTURE_REMEDIATION_PLAN.md | Adds remediation plan document. |
| ARCHITECTURE_REMEDIATION_RELEASE_NOTE.md | Adds release note summarizing remediation outcomes and gates. |
| docs/x509-parser-to-rasn-plan.md | Removes x509-parser migration plan doc. |
| docs/x509-parser-stage0-baseline.md | Removes x509-parser stage0 baseline doc. |
| docs/ocsp-stage0-baseline.md | Removes OCSP stage0 baseline doc. |
| docs/ocsp-rasn-refactor-plan.md | Removes OCSP rasn refactor plan doc. |
| docs/nginx-comparison.md | Removes nginx comparison doc. |
| docs/nginx-comparison-snapshots/2026-04-10-trixie-smoke.md | Removes nginx comparison snapshot doc. |
| .gitignore | Ignores docs/. |
💡 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/rginx-http/src/compression.rs (1)
66-73:⚠️ Potential issue | 🟠 Major不要把 body 收集失败伪装成原状态码的空响应。
Line 66 这里一旦
collect()失败，就会返回“空 body + 原状态码”。如果原响应是200 OK，调用方和缓存层会看到一次成功响应，而真实的流读取失败被吞掉了。这里至少应转成明确的错误响应（例如 500/502），而不是继续沿用原状态码。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/compression.rs` around lines 66 - 73, The current error branch in the compression path masks a body.collect() failure by returning an empty body with the original status; instead change the Err(error) branch in the code handling body.collect() so it logs the error and returns a clear error response (e.g., 500 or 502) rather than using the original status. Specifically, modify the handler around body.collect() so that on Err(error) you call Response::from_parts with parts_without_compression_metadata(parts) but with a Response head/status set to an explicit error status (not the original parts.status), keep the warning with %error and content_coding.label(), and return full_body(Bytes::new()) for the response body; ensure the failure is not swallowed by propagating an error-status response to callers and caches instead of masking as a successful original-status response.README.md (1)
5-5:⚠️ Potential issue | 🟡 Minor版本号需要更新。
README 中显示当前版本为
v0.1.3-rc.8，但 PR 标题表明这是v0.1.3-rc.9的准备工作。建议在合并前更新版本号。📝 建议的修改
-当前版本：`v0.1.3-rc.8` +当前版本：`v0.1.3-rc.9`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 5, The README currently lists the release as `v0.1.3-rc.8` but this PR prepares `v0.1.3-rc.9`; update the version string in README (replace `v0.1.3-rc.8` with `v0.1.3-rc.9`) so the documentation matches the PR/release, and double-check any other README occurrences of `v0.1.3-rc.8` are similarly updated.
🧹 Nitpick comments (1)
crates/rginx-app/tests/admin/commands.rs (1)
55-55: 建议让listen_addrs断言对顺序不敏感。当前断言把地址顺序写死，后续若仅输出顺序变化会造成误报失败。可以改为拆分后按集合比较。
可选改法（示例）
- assert!(stdout.contains(&format!("listen_addrs={http_addr},{https_addr}"))); + let status_line = stdout + .lines() + .find(|line| line.contains("kind=status")) + .expect("status line should exist"); + let addrs_field = status_line + .split_whitespace() + .find(|part| part.starts_with("listen_addrs=")) + .expect("listen_addrs field should exist"); + let mut actual = addrs_field + .trim_start_matches("listen_addrs=") + .split(',') + .map(str::to_string) + .collect::<Vec<_>>(); + actual.sort(); + let mut expected = vec![http_addr.to_string(), https_addr.to_string()]; + expected.sort(); + assert_eq!(actual, expected);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-app/tests/admin/commands.rs` at line 55, The test assertion currently checks listen_addrs with a fixed order using assert!(stdout.contains(&format!("listen_addrs={http_addr},{https_addr}"))); — change it to be order-insensitive by extracting the listen_addrs value from stdout, split on ',' into two items, and compare as a set (or sort both sides) against the expected addresses (use the same http_addr and https_addr variables) so the test passes regardless of which address appears first. Ensure the replacement locates the same stdout string and performs the set/sorted equality check instead of a substring contains check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker/nginx-compare/Dockerfile`:
- Around line 29-31: The Dockerfile RUN that installs Rust using
"default-toolchain stable" makes the Rust toolchain variable over time; change
that to pin a specific toolchain (e.g., the MSRV 1.85) or add an ARG like
RUST_VERSION and use that ARG when installing/setting the toolchain so builds
are reproducible; update the RUN line that calls rustup (and any place checking
rustc/cargo versions) to install and set the fixed version from the ARG and
document the default value.
In `@scripts/build-deb.sh`:
- Line 166: Replace hardcoded installation paths that use
"${STAGE_DIR}/usr/lib/systemd/system" with the Debian-standard
"${STAGE_DIR}/lib/systemd/system" in scripts/build-deb.sh; locate the
occurrences where the script constructs the systemd unit install directory
(strings containing "/usr/lib/systemd/system" and usages near the STAGE_DIR
variable) — update both the line at the shown diff and the similar occurrence
around line 180 so unit files are staged to "${STAGE_DIR}/lib/systemd/system"
instead of "/usr/lib/systemd/system".
In `@scripts/nginx_compare/common.py`:
- Around line 41-45: The current logic may raise the RLIMIT_NOFILE hard limit up
to target (when hard < target), causing setrlimit to fail for unprivileged users
and preventing raising the soft limit; change the computation so the hard limit
never exceeds the current hard value: keep desired_hard equal to hard (except
preserve resource.RLIM_INFINITY behavior), compute desired_soft as min(target,
desired_hard) (respecting RLIM_INFINITY), and only call
resource.setrlimit(resource.RLIMIT_NOFILE, (desired_soft, desired_hard)) when
soft < desired_soft; reference variables desired_hard, desired_soft and the call
to resource.setrlimit to locate where to change.
In `@scripts/nginx_compare/main.py`:
- Around line 16-18: 为命令行参数 --requests、--concurrency、--rounds 增加正数校验以避免
collect_rounds() 返回空列表或在 median_* 中崩溃：在 argparse 层为这三个参数使用一个只接受正整数的校验器（例如
positive_int）或在解析后立即检查 args.requests/args.concurrency/args.rounds > 0
并在不满足时打印错误并退出；明确提到受影响的符号包括 parser.add_argument(...) 的三个参数、collect_rounds() 和
median_*，确保在 CLI 层就拒绝非正数输入以避免后续执行器抛出难读异常。
---
Outside diff comments:
In `@crates/rginx-http/src/compression.rs`:
- Around line 66-73: The current error branch in the compression path masks a
body.collect() failure by returning an empty body with the original status;
instead change the Err(error) branch in the code handling body.collect() so it
logs the error and returns a clear error response (e.g., 500 or 502) rather than
using the original status. Specifically, modify the handler around
body.collect() so that on Err(error) you call Response::from_parts with
parts_without_compression_metadata(parts) but with a Response head/status set to
an explicit error status (not the original parts.status), keep the warning with
%error and content_coding.label(), and return full_body(Bytes::new()) for the
response body; ensure the failure is not swallowed by propagating an
error-status response to callers and caches instead of masking as a successful
original-status response.
In `@README.md`:
- Line 5: The README currently lists the release as `v0.1.3-rc.8` but this PR
prepares `v0.1.3-rc.9`; update the version string in README (replace
`v0.1.3-rc.8` with `v0.1.3-rc.9`) so the documentation matches the PR/release,
and double-check any other README occurrences of `v0.1.3-rc.8` are similarly
updated.
---
Nitpick comments:
In `@crates/rginx-app/tests/admin/commands.rs`:
- Line 55: The test assertion currently checks listen_addrs with a fixed order
using
assert!(stdout.contains(&format!("listen_addrs={http_addr},{https_addr}"))); —
change it to be order-insensitive by extracting the listen_addrs value from
stdout, split on ',' into two items, and compare as a set (or sort both sides)
against the expected addresses (use the same http_addr and https_addr variables)
so the test passes regardless of which address appears first. Ensure the
replacement locates the same stdout string and performs the set/sorted equality
check instead of a substring contains check.
🪄 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: 27dd5ad3-628f-456f-965c-9b26576e9182
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (67)
.codex.gitignoreARCHITECTURE_REMEDIATION_PLAN.mdARCHITECTURE_REMEDIATION_RELEASE_NOTE.mdCargo.tomlREADME.mdcrates/rginx-app/src/admin_cli/mod.rscrates/rginx-app/src/admin_cli/status.rscrates/rginx-app/src/cli.rscrates/rginx-app/src/main.rscrates/rginx-app/src/migrate_nginx/convert.rscrates/rginx-app/src/migrate_nginx/mod.rscrates/rginx-app/src/migrate_nginx/parser.rscrates/rginx-app/src/migrate_nginx/render.rscrates/rginx-app/src/migrate_nginx/tests.rscrates/rginx-app/src/migrate_nginx/tokenize.rscrates/rginx-app/tests/admin/commands.rscrates/rginx-app/tests/admin/snapshot.rscrates/rginx-app/tests/check.rscrates/rginx-app/tests/migrate.rscrates/rginx-app/tests/multi_listener.rscrates/rginx-app/tests/vhost.rscrates/rginx-config/src/compile/mod.rscrates/rginx-config/src/compile/tests.rscrates/rginx-core/src/config.rscrates/rginx-core/src/config/tests.rscrates/rginx-core/src/lib.rscrates/rginx-http/src/compression.rscrates/rginx-http/src/handler/dispatch.rscrates/rginx-http/src/handler/tests.rscrates/rginx-http/src/lib.rscrates/rginx-http/src/proxy/clients/tests.rscrates/rginx-http/src/proxy/forward/response.rscrates/rginx-http/src/proxy/health/registry.rscrates/rginx-http/src/proxy/tests/mod.rscrates/rginx-http/src/router.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/tls/sni.rscrates/rginx-http/src/transition.rscrates/rginx-runtime/src/admin.rscrates/rginx-runtime/src/health.rsdeploy/systemd/rginx.servicedocker/nginx-compare/Dockerfiledocs/large-file-split-plan.mddocs/nginx-comparison-snapshots/2026-04-10-trixie-smoke.mddocs/nginx-comparison.mddocs/ocsp-rasn-refactor-plan.mddocs/ocsp-stage0-baseline.mddocs/x509-parser-stage0-baseline.mddocs/x509-parser-to-rasn-plan.mdpackaging/apt/control.inpackaging/apt/postinstpackaging/apt/postrmpackaging/apt/prermscripts/build-deb.shscripts/nginx_compare/checkout.pyscripts/nginx_compare/common.pyscripts/nginx_compare/configs.pyscripts/nginx_compare/launch.pyscripts/nginx_compare/main.pyscripts/nginx_compare/render.pyscripts/nginx_compare/scenarios.pyscripts/run-nginx-compare-docker.shscripts/run-tls-gate.sh
💤 Files with no reviewable changes (21)
- crates/rginx-runtime/src/health.rs
- crates/rginx-http/src/transition.rs
- crates/rginx-http/src/proxy/clients/tests.rs
- scripts/run-tls-gate.sh
- docs/ocsp-stage0-baseline.md
- crates/rginx-http/src/proxy/tests/mod.rs
- docs/x509-parser-stage0-baseline.md
- crates/rginx-core/src/config/tests.rs
- docs/x509-parser-to-rasn-plan.md
- crates/rginx-http/src/proxy/health/registry.rs
- docs/nginx-comparison.md
- docs/nginx-comparison-snapshots/2026-04-10-trixie-smoke.md
- crates/rginx-app/src/migrate_nginx/tests.rs
- crates/rginx-app/src/migrate_nginx/tokenize.rs
- crates/rginx-app/src/migrate_nginx/render.rs
- crates/rginx-app/tests/migrate.rs
- crates/rginx-app/src/migrate_nginx/mod.rs
- crates/rginx-app/src/migrate_nginx/parser.rs
- crates/rginx-app/src/migrate_nginx/convert.rs
- docs/large-file-split-plan.md
- docs/ocsp-rasn-refactor-plan.md
Owner
Author
|
Handled the review feedback in commit Included fixes for:
Validation run after the fixes:
|
Owner
Author
|
Resolving all outstanding review threads that have already been addressed in follow-up commits, then proceeding with merge and tag publication for |
This was referenced Apr 12, 2026
This was referenced Apr 25, 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
Validation
Notes