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 12 minutes and 46 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 (3)
📝 WalkthroughWalkthrough将测试拆分为快速/慢速脚本并在 CI 中串联；重构 rginx-http 的 state 为多个子模块并新增快照/流量/上游统计与 TLS 运行时快照；引入配置转换规划与验证（热重载 vs 重启边界）；重构运行时监听器工作组与两阶段重载（prepare/commit）；大量测试与文档新增；版本升至 0.1.3-rc.5。 Changes
Sequence Diagram(s)sequenceDiagram
participant AdminClient as Admin/API 客户端
participant Runtime as rginx-runtime
participant State as SharedState
participant Bootstrap as Bootstrap/ListenerGroups
participant Snapshot as SnapshotBus
AdminClient->>Runtime: 请求 SIGHUP / reload
Runtime->>State: prepare_reload() (编译、plan_config_transition)
State-->>Runtime: 返回 PendingReload / 验证结果
alt 验证通过
Runtime->>State: commit_reload(next_config)
State->>State: replace config，准备 retired_listeners，合并 acceptors
State->>Snapshot: mark_snapshot_changed_components()
Runtime->>Bootstrap: reconcile_listener_worker_groups()
Bootstrap->>Bootstrap: retire/add listener groups，start/stop accept loops
Bootstrap->>State: retire_listener_runtime()/sync_listener_active_connections()
State->>Snapshot: notify_snapshot_waiters()
Runtime-->>AdminClient: reload 成功
else 验证失败
Runtime-->>AdminClient: reload 拒绝（RestartRequired）
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 分钟 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 the v0.1.3-rc.5 release by landing the rc.5-cycle runtime refactors (state/proxy modularization), adding a single-source config transition planner (reload vs restart boundary), enabling explicit listener hot add/remove with draining semantics, and updating CI/release tooling + docs to match.
Changes:
- Introduce
transitionplanner + validation for reload/restart boundaries and wire it into reload paths. - Add listener supervisor-style orchestration in runtime bootstrap (active vs draining listener groups) to support listener add/remove during SIGHUP reload.
- Split test execution into fast/slow entrypoints and update release preflight + CI workflows; bump version to
0.1.3-rc.5and add docs.
Reviewed changes
Copilot reviewed 31 out of 32 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/test-slow.sh | New slow-layer test script running integration tests serially. |
| scripts/test-fast.sh | New fast-layer test script running crate-local/unit tests serially. |
| scripts/prepare-release.sh | Switch release preflight from cargo test --workspace to layered fast/slow scripts. |
| README.md | Update version to rc.5; document layered tests and reload/restart boundaries. |
| docs/runtime-architecture.md | New internal architecture doc describing state/proxy/transition/supervisor layers. |
| docs/reload-semantics.md | New reload/restart semantics doc, including listener drain behavior. |
| docs/refactor-plan.md | New refactor plan doc outlining staged maintenance phases. |
| crates/rginx-runtime/src/reload.rs | Split reload into prepare/commit; add transition validation before commit. |
| crates/rginx-runtime/src/bootstrap.rs | Implement active/draining listener worker groups and reload-time listener reconciliation. |
| crates/rginx-http/src/transition.rs | New config transition planner + validation and unit tests. |
| crates/rginx-http/src/state/upstreams.rs | Move upstream stats/snapshot + record hooks into dedicated state module. |
| crates/rginx-http/src/state/traffic.rs | Move traffic counters/snapshots + change tracking into dedicated state module. |
| crates/rginx-http/src/state/tls_runtime.rs | Move TLS runtime snapshot building, OCSP inspection, cert inspection into dedicated module. |
| crates/rginx-http/src/state/snapshot_bus.rs | Move snapshot/version/delta/wait plumbing into dedicated module. |
| crates/rginx-http/src/state/lifecycle.rs | Consolidate reload history, replace/commit, retired listener runtime hooks, background task lifecycle. |
| crates/rginx-http/src/state/helpers.rs | Adjust prepared state construction; remove inline transition validation helper. |
| crates/rginx-http/src/state/connections.rs | Add dedicated connection guards + connection/tls handshake counters integration. |
| crates/rginx-http/src/proxy/mod.rs | Slim proxy entrypoint; extract TLS error mapping and move tests into module. |
| crates/rginx-http/src/proxy/error_mapping.rs | New module for upstream TLS verification labels + TLS failure classification. |
| crates/rginx-http/src/lib.rs | Export transition planner API from the crate root. |
| crates/rginx-config/src/compile/server.rs | Change explicit listener IDs to stable name-derived IDs (listener:<name>). |
| crates/rginx-app/tests/reload.rs | Expand reload integration coverage for listener add/remove + draining semantics. |
| crates/rginx-app/tests/failover.rs | Add coverage ensuring non-idempotent requests don’t fail over on timeout. |
| crates/rginx-app/tests/check.rs | Update expected restart-boundary field labels for explicit listeners. |
| crates/rginx-app/tests/admin.rs | Add admin wait-timeout test; add snapshot/delta coverage for listener lifecycle after reload. |
| crates/rginx-app/src/main.rs | Align check output to use transition boundary summary data. |
| Cargo.toml | Bump workspace version to 0.1.3-rc.5. |
| Cargo.lock | Update lockfile versions to 0.1.3-rc.5. |
| .github/workflows/release.yml | Use layered fast/slow test scripts in release verification. |
| .github/workflows/ci.yml | Split CI tests into fast/slow jobs and add dependency ordering. |
💡 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: 9
🧹 Nitpick comments (1)
crates/rginx-config/src/compile/server.rs (1)
119-131: 统一 listener ID 规范化方式。规范化后的 listener 名称唯一性已通过
crates/rginx-config/src/validate/server.rs的验证逻辑得到保障（使用HashSet在编译前 fail-fast）。但规范化方式存在不一致：
validate/server.rs使用to_lowercase()compile/server.rs(line 130) 使用to_ascii_lowercase()虽然对 ASCII 输入无实际差异，但应统一为
to_ascii_lowercase()以保持代码一致性。建议同时补充测试用例覆盖大小写/空白变体的碰撞场景（如"HTTP"与"http"）。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-config/src/compile/server.rs` around lines 119 - 131, The listener normalization is inconsistent: compile::explicit_listener_id uses to_ascii_lowercase() while validation uses to_lowercase(); change the validation path in validate/server.rs to use to_ascii_lowercase() (the same normalization used by explicit_listener_id) so IDs match exactly (the validation logic that builds the HashSet and uniqueness checks should use the same normalized form), and add a unit test covering collision scenarios (e.g., "HTTP" vs "http" and variants with surrounding whitespace) to ensure the normalized values collide 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-app/tests/failover.rs`:
- Around line 31-36: Add an assertion that the first upstream was actually hit
before checking failover behavior: after the request is made in the test that
uses spawn_response_server_with_hits(), assert that
slow_hits.load(Ordering::SeqCst) > 0 and still assert
fast_hits.load(Ordering::SeqCst) == 0 to ensure the request attempted the slow
peer before timing out and not hitting the fast peer; apply the same extra
assertion for the similar test block covering lines 43-56.
In `@crates/rginx-app/tests/reload.rs`:
- Around line 127-135: The test uses a fixed
std::thread::sleep(Duration::from_millis(100)) to assume the background request
reached the draining listener; replace this flaky sleep with an explicit ready
signal: have the background thread (that calls fetch_text_response_with_timeout
or the spawn_delayed_response_server) send a "ready" message over the existing
mpsc::channel (tx) as soon as it has read the request headers or forwarded to
upstream, then in the main test block wait on rx for that ready signal before
calling server.write_config(...) and server.send_signal(libc::SIGHUP); update
fetch_text_response_with_timeout or the delayed server helper to emit the ready
signal at the correct point so the reload happens only after the request is
guaranteed to be in drain.
In `@crates/rginx-http/src/proxy/tests.rs`:
- Line 1915: Remove the hardcoded TEST_SERVER_KEY_PEM constant and instead
generate a temporary key/cert pair at test startup (use the repo's existing
runtime test-certificate generation code/pattern), write them to a tempdir or
return in-memory bytes, and update tests that reference TEST_SERVER_KEY_PEM to
call a helper (e.g., create_test_cert() or get_temp_test_cert()) that returns
the private key and cert paths/bytes; ensure the helper is used by the tests
that previously relied on TEST_SERVER_KEY_PEM so no PEM literals remain in the
source.
In `@crates/rginx-http/src/state/lifecycle.rs`:
- Around line 16-40: status_snapshot currently holds self.inner.read().await
while calling tls_runtime_snapshot_for_config_with_ocsp_statuses (and
mtls_status_snapshot), which does file/OCSP parsing and can block the read lock;
instead, acquire the read lock only long enough to clone the minimal pieces
needed (e.g. state.revision, state.config.clone() or the specific config fields
used, self.config_path.clone(), total counts, and any data needed for
upstream_tls/ocsp lookup like ocsp_statuses via self.ocsp_statuses.read()
clone), drop the lock, then call
tls_runtime_snapshot_for_config_with_ocsp_statuses, mtls_status_snapshot and
upstream_tls_status_snapshots using those cloned values and build the
RuntimeStatusSnapshot; keep references to the functions/fields used:
status_snapshot, self.inner.read().await,
tls_runtime_snapshot_for_config_with_ocsp_statuses, mtls_status_snapshot,
upstream_tls_status_snapshots, and ocsp_statuses so the change is localized.
In `@crates/rginx-http/src/state/snapshot_bus.rs`:
- Around line 184-208: mark_snapshot_changed_components currently increments
snapshot_version and calls snapshot_notify.notify_waiters immediately, which can
wake listeners before callers like record_downstream_request /
record_upstream_request have written entries into traffic_component_versions /
upstream_component_versions; move the notify_waiters call so it runs only after
the target-version maps are updated, or alternatively implement a single helper
that performs "allocate new snapshot_version, update the target map(s)
(traffic_component_versions / upstream_component_versions), then call
snapshot_notify.notify_waiters" atomically and have record_downstream_request
and record_upstream_request use that helper; ensure references to
snapshot_version, snapshot_components.*, traffic_component_versions,
upstream_component_versions and snapshot_notify.notify_waiters are updated
accordingly.
In `@crates/rginx-http/src/state/tls_runtime.rs`:
- Around line 215-222: selected_as_default_for_listeners currently treats a
listener as providing a default cert even when
listener.server.default_certificate points to one of
config.default_vhost.server_names, causing duplicate "default" entries; change
the logic so a listener is only marked in selected_as_default_for_listeners when
listener.server.default_certificate is None (i.e., only when no explicit
default_certificate was configured). Update the branch around
selected_as_default_for_listeners to remove the is_some_and(...) check and only
return vec![listener.name.clone()] when
listener.server.default_certificate.is_none(); keep
build_vhost_certificate_snapshot as-is so the default vhost certificate remains
the authoritative default.
In `@crates/rginx-runtime/src/bootstrap.rs`:
- Around line 79-89: The code calls commit_reload(&state, pending).await before
reconcile_listener_worker_groups(...).await? which makes the new config take
effect even if spawn_listener_worker_group (and other steps in
reconcile_listener_worker_groups) later fail; either move all listener
preparation/validation (the work that can fail in spawn_listener_worker_group
and related helpers) to before calling commit_reload so commit_reload is only
called when those steps are guaranteed to succeed, or add a strong rollback
path: if reconcile_listener_worker_groups returns Err, revert state.http to the
previous config, stop/cleanup any partially started listener groups and worker
tasks started during reconcile_listener_worker_groups, and ensure run() does not
exit in a half-applied state (i.e., perform cleanup/restore instead of
propagating the error with ?). Use the unique symbols commit_reload,
reconcile_listener_worker_groups, spawn_listener_worker_group, state.http and
run to locate where to implement either the pre-commit validation or the
post-commit rollback/cleanup.
In `@docs/reload-semantics.md`:
- Around line 75-76: Links in docs/reload-semantics.md point to absolute local
filesystem paths (/root/github/...), which will break in GitHub/site rendering;
update the two links to use repository-relative relative paths such as
./refactor-plan.md and ./runtime-architecture.md. Locate the markdown list
entries referencing refactor-plan.md and runtime-architecture.md and replace the
hrefs so they read [refactor-plan.md](./refactor-plan.md) and
[runtime-architecture.md](./runtime-architecture.md) (ensure any surrounding
link text remains unchanged).
In `@README.md`:
- Around line 138-158: The README's "热更新边界" section contradicts later examples
and the "配置检查" snippet: update the example and limitation text to match the new
semantics (explicit listeners may be added/removed hot, only listen and
listeners[].listen require restart), remove or modify the old
`reload_requires_restart_for=listen,listeners,...` example and any statements
claiming the entire listener collection requires restart, and ensure references
to `listen`, `listeners[].listen`, `runtime.worker_threads`, and
`runtime.accept_workers` are accurate; also sync the README with
docs/reload-semantics.md and docs/runtime-architecture.md so all three places
state the same restart boundaries.
---
Nitpick comments:
In `@crates/rginx-config/src/compile/server.rs`:
- Around line 119-131: The listener normalization is inconsistent:
compile::explicit_listener_id uses to_ascii_lowercase() while validation uses
to_lowercase(); change the validation path in validate/server.rs to use
to_ascii_lowercase() (the same normalization used by explicit_listener_id) so
IDs match exactly (the validation logic that builds the HashSet and uniqueness
checks should use the same normalized form), and add a unit test covering
collision scenarios (e.g., "HTTP" vs "http" and variants with surrounding
whitespace) to ensure the normalized values collide 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: 899cc307-0c6f-4e82-bd21-5813ec195b28
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
.github/workflows/ci.yml.github/workflows/release.ymlCargo.tomlREADME.mdcrates/rginx-app/src/main.rscrates/rginx-app/tests/admin.rscrates/rginx-app/tests/check.rscrates/rginx-app/tests/failover.rscrates/rginx-app/tests/reload.rscrates/rginx-config/src/compile/server.rscrates/rginx-http/src/lib.rscrates/rginx-http/src/proxy/error_mapping.rscrates/rginx-http/src/proxy/mod.rscrates/rginx-http/src/proxy/tests.rscrates/rginx-http/src/state.rscrates/rginx-http/src/state/connections.rscrates/rginx-http/src/state/helpers.rscrates/rginx-http/src/state/lifecycle.rscrates/rginx-http/src/state/snapshot_bus.rscrates/rginx-http/src/state/tls_runtime.rscrates/rginx-http/src/state/traffic.rscrates/rginx-http/src/state/upstreams.rscrates/rginx-http/src/transition.rscrates/rginx-runtime/src/bootstrap.rscrates/rginx-runtime/src/reload.rsdocs/refactor-plan.mddocs/reload-semantics.mddocs/runtime-architecture.mdscripts/prepare-release.shscripts/test-fast.shscripts/test-slow.sh
Owner
Author
|
Addressed the actionable automated review comments in commit Handled in this update:
Validation rerun after the fixes:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
crates/rginx-http/src/proxy/tests.rs (3)
1659-1795: 考虑提取公共的ConfigSnapshot构建逻辑。
snapshot_with_upstream、snapshot_with_upstreams、snapshot_with_upstream_policy、snapshot_with_active_health函数中Server、Listener、VirtualHost的构建逻辑大量重复。可以提取一个通用的构建器函数减少重复。♻️ 可选的重构方案
fn default_server() -> rginx_core::Server { rginx_core::Server { listen_addr: "127.0.0.1:8080".parse().unwrap(), default_certificate: None, trusted_proxies: Vec::new(), keep_alive: true, max_headers: None, max_request_body_bytes: None, max_connections: None, header_read_timeout: None, request_body_read_timeout: None, response_write_timeout: None, access_log_format: None, tls: None, } } fn snapshot_with_upstreams_map( upstreams: HashMap<String, Arc<Upstream>>, ) -> rginx_core::ConfigSnapshot { let server = default_server(); rginx_core::ConfigSnapshot { runtime: rginx_core::RuntimeSettings { shutdown_timeout: Duration::from_secs(1), worker_threads: None, accept_workers: 1, }, server: server.clone(), listeners: vec![rginx_core::Listener { id: "default".to_string(), name: "default".to_string(), server, tls_termination_enabled: false, proxy_protocol_enabled: false, }], default_vhost: rginx_core::VirtualHost { id: "server".to_string(), server_names: Vec::new(), routes: Vec::new(), tls: None, }, vhosts: Vec::new(), upstreams, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/proxy/tests.rs` around lines 1659 - 1795, The three snapshot helpers duplicate Server/Listener/VirtualHost construction; extract helper constructors like default_server(), default_listener(server: rginx_core::Server) and default_vhost() and a central builder snapshot_with_upstreams_map(upstreams: HashMap<String, Arc<Upstream>>) that builds the common Runtime/Server/Listeners/DefaultVhost/vhosts and accepts upstreams, then refactor snapshot_with_upstream, snapshot_with_upstreams and snapshot_with_upstream_policy to call these helpers (create Upstream/HashMap as before, then pass into snapshot_with_upstreams_map) to remove the repeated Server/Listener/VirtualHost code.
462-558: 建议使用tempfilecrate 管理临时文件。当前测试在函数末尾手动删除临时文件。如果测试在清理前 panic，临时文件会残留。使用
tempfile::NamedTempFile或tempfile::TempDir可以确保在任何情况下自动清理。♻️ 建议的改进方案
+use tempfile::TempDir; #[test] fn proxy_clients_can_select_insecure_and_custom_ca_modes() { - let unique = SystemTime::now() - .duration_since(UNIX_EPOCH) - .expect("system time should be after unix epoch") - .as_nanos(); - let path = std::env::temp_dir().join(format!("rginx-custom-ca-select-{unique}.pem")); - std::fs::write(&path, TEST_CA_CERT_PEM).expect("PEM file should be written"); + let temp_dir = TempDir::new().expect("temp dir should be created"); + let path = temp_dir.path().join("custom-ca.pem"); + std::fs::write(&path, TEST_CA_CERT_PEM).expect("PEM file should be written"); // ... rest of test ... - std::fs::remove_file(path).expect("temp PEM file should be removed"); + // temp_dir 超出作用域时自动清理 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/proxy/tests.rs` around lines 462 - 558, Tests create temporary PEM files and manually remove them, which can leak files if a test panics; replace manual file creation/removal with tempfile::NamedTempFile (or TempDir) in the tests load_custom_ca_store_accepts_pem_files and proxy_clients_can_select_insecure_and_custom_ca_modes: create a NamedTempFile, write TEST_CA_CERT_PEM to it, use its path (or persist if necessary) when calling load_custom_ca_store and when constructing UpstreamTls::CustomCa { ca_cert_path: ... }, and remove the explicit std::fs::remove_file calls so cleanup is automatic on drop.
1878-1911:spawn_status_server缺少优雅关闭机制。当前实现在无限循环中接受连接，测试完成后线程继续运行。对于长时间运行的测试套件，这可能导致线程资源泄漏。
考虑使用
oneshotchannel 或原子标志实现关闭信号：♻️ 建议的改进方案
+use std::sync::atomic::{AtomicBool, Ordering}; -async fn spawn_status_server(statuses: Arc<Mutex<VecDeque<StatusCode>>>) -> SocketAddr { +async fn spawn_status_server( + statuses: Arc<Mutex<VecDeque<StatusCode>>>, +) -> (SocketAddr, Arc<AtomicBool>) { let listener = TcpListener::bind(("127.0.0.1", 0)).expect("test status listener should bind"); let listen_addr = listener.local_addr().expect("listener addr should exist"); + let shutdown = Arc::new(AtomicBool::new(false)); + let shutdown_clone = shutdown.clone(); + listener.set_nonblocking(true).expect("set nonblocking should succeed"); thread::spawn(move || { - loop { - let Ok((mut stream, _)) = listener.accept() else { - break; - }; + while !shutdown_clone.load(Ordering::Relaxed) { + match listener.accept() { + Ok((mut stream, _)) => { /* handle connection */ } + Err(ref e) if e.kind() == std::io::ErrorKind::WouldBlock => { + thread::sleep(Duration::from_millis(10)); + continue; + } + Err(_) => break, + } // ... } }); - listen_addr + (listen_addr, shutdown) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/proxy/tests.rs` around lines 1878 - 1911, spawn_status_server currently spawns a background thread that loops forever on listener.accept, so tests leak threads; change spawn_status_server to accept or create a shutdown signal (e.g., a tokio::sync::oneshot::Receiver or an Arc<AtomicBool> flag) and have the spawned thread check that signal inside the accept loop (or use nonblocking accept/select so the thread can break when signaled), then close the TcpListener and join the thread; update the function signature/return to expose the shutdown sender/flag (or a JoinHandle) so callers can trigger shutdown and await the thread; adjust the inner closures that use statuses to keep the same behavior while responding to the shutdown signal.
🤖 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/state/lifecycle.rs`:
- Around line 214-244: The snapshot mixes two sources: handshake_failures_total
is aggregated from per-listener data (see listener_traffic_counters and the loop
that increments handshake_failures_total), while the handshake_failures_*
category fields are taken from the global counters_snapshot(), causing
inconsistent snapshots after reloads. Fix by making both total and category
fields come from the same source: either (A) compute all handshake_failures_* by
summing per-listener counters inside the same loop that builds
authenticated_connections/authenticated_requests and then set
handshake_failures_total as the sum of those per-listener category sums, or (B)
use the global counters_snapshot() for all handshake_failures_* and set
handshake_failures_total from the same counters (e.g., sum the
handshake_failures_* fields from counters or use a global total if available).
Update the code around MtlsStatusSnapshot construction so
handshake_failures_total and handshake_failures_missing_client_cert / unknown_ca
/ bad_certificate / certificate_revoked / verify_depth_exceeded / other all use
the same counters source.
- Around line 155-172: The reload path leaves newly added/removed
traffic/upstream targets with version 0 (in
build_traffic_component_versions/build_upstream_name_versions) so delta
consumers miss them; fix commit_prepared() so after applying prepared config and
computing next_revision it either (A) assigns next_revision to any newly added
or removed targets (ensure build_* sets target versions to next_revision instead
of 0) or (B) invokes the existing marker helpers to mark per-target changes —
call mark_traffic_targets_changed() for listeners/vhosts/routes and
mark_named_component_target_changed() for upstreams (in addition to the existing
mark_snapshot_changed_components call) so
changed_traffic_targets_since/changed_named_component_targets_since and
ultimately snapshot_delta_since will include those targets.
In `@docs/reload-semantics.md`:
- Line 48: Line 48 contains an awkward phrase "已经进入中的请求"; replace it with a
clearer alternative such as "已经进入处理中的请求" or "已开始处理的请求" (update the sentence
where the phrase "已经进入中的请求" appears in docs/reload-semantics.md to use one of
these alternatives to remove the grammatical error and avoid ambiguity).
---
Nitpick comments:
In `@crates/rginx-http/src/proxy/tests.rs`:
- Around line 1659-1795: The three snapshot helpers duplicate
Server/Listener/VirtualHost construction; extract helper constructors like
default_server(), default_listener(server: rginx_core::Server) and
default_vhost() and a central builder snapshot_with_upstreams_map(upstreams:
HashMap<String, Arc<Upstream>>) that builds the common
Runtime/Server/Listeners/DefaultVhost/vhosts and accepts upstreams, then
refactor snapshot_with_upstream, snapshot_with_upstreams and
snapshot_with_upstream_policy to call these helpers (create Upstream/HashMap as
before, then pass into snapshot_with_upstreams_map) to remove the repeated
Server/Listener/VirtualHost code.
- Around line 462-558: Tests create temporary PEM files and manually remove
them, which can leak files if a test panics; replace manual file
creation/removal with tempfile::NamedTempFile (or TempDir) in the tests
load_custom_ca_store_accepts_pem_files and
proxy_clients_can_select_insecure_and_custom_ca_modes: create a NamedTempFile,
write TEST_CA_CERT_PEM to it, use its path (or persist if necessary) when
calling load_custom_ca_store and when constructing UpstreamTls::CustomCa {
ca_cert_path: ... }, and remove the explicit std::fs::remove_file calls so
cleanup is automatic on drop.
- Around line 1878-1911: spawn_status_server currently spawns a background
thread that loops forever on listener.accept, so tests leak threads; change
spawn_status_server to accept or create a shutdown signal (e.g., a
tokio::sync::oneshot::Receiver or an Arc<AtomicBool> flag) and have the spawned
thread check that signal inside the accept loop (or use nonblocking
accept/select so the thread can break when signaled), then close the TcpListener
and join the thread; update the function signature/return to expose the shutdown
sender/flag (or a JoinHandle) so callers can trigger shutdown and await the
thread; adjust the inner closures that use statuses to keep the same behavior
while responding to the shutdown signal.
🪄 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: 7dad8b44-3f37-497d-a369-492899bd7ae6
📒 Files selected for processing (14)
README.mdcrates/rginx-app/tests/failover.rscrates/rginx-app/tests/reload.rscrates/rginx-config/src/validate/server.rscrates/rginx-config/src/validate/tests.rscrates/rginx-http/src/proxy/tests.rscrates/rginx-http/src/state/connections.rscrates/rginx-http/src/state/lifecycle.rscrates/rginx-http/src/state/snapshot_bus.rscrates/rginx-http/src/state/tls_runtime.rscrates/rginx-http/src/state/traffic.rscrates/rginx-http/src/state/upstreams.rscrates/rginx-runtime/src/bootstrap.rsdocs/reload-semantics.md
✅ Files skipped from review due to trivial changes (2)
- crates/rginx-config/src/validate/server.rs
- crates/rginx-http/src/state/tls_runtime.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- README.md
- crates/rginx-runtime/src/bootstrap.rs
- crates/rginx-app/tests/reload.rs
Owner
Author
|
Follow-up fixes from the latest automated review have been pushed in This update includes:
Validation rerun after the update:
|
Owner
Author
|
已处理本轮剩余的 AI review 项，并已手动 resolve 对应线程。 本次补充：
验证：
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
docs/reload-semantics.md (1)
49-49: 可选：改进表述的自然度。"新连接最终会变成不可达" 在语义上可以理解，但表述略显生硬。建议改为更自然的表达方式，例如 "该 listener 将不再接受新连接" 或 "新连接将无法建立到该 listener"，以提升文档的可读性。
✏️ 可选的改进建议
-- 新连接最终会变成不可达 +- 该 listener 将不再接受新连接或者：
-- 新连接最终会变成不可达 +- 新连接将无法建立到该 listener🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reload-semantics.md` at line 49, Replace the awkward phrase "新连接最终会变成不可达" in docs/reload-semantics.md with a more natural expression; locate the exact string and change it to one of the suggested alternatives such as "该 listener 将不再接受新连接" or "新连接将无法建立到该 listener" to improve readability while preserving the original meaning.crates/rginx-http/src/proxy/tests.rs (2)
506-546: 重复的Server/ConfigSnapshot构造建议统一走现有 helper。这些区块与 Line 1651-1713 的 helper 职责重叠，当前重复较多，后续字段变更会带来高维护成本。建议统一改为
snapshot_with_upstreams(...)/snapshot_with_upstreams_map(...)。♻️ 精简示例
- let server = rginx_core::Server { - listen_addr: "127.0.0.1:8080".parse().unwrap(), - default_certificate: None, - trusted_proxies: Vec::new(), - keep_alive: true, - max_headers: None, - max_request_body_bytes: None, - max_connections: None, - header_read_timeout: None, - request_body_read_timeout: None, - response_write_timeout: None, - access_log_format: None, - tls: None, - }; - let snapshot = rginx_core::ConfigSnapshot { - runtime: rginx_core::RuntimeSettings { - shutdown_timeout: std::time::Duration::from_secs(1), - worker_threads: None, - accept_workers: 1, - }, - server: server.clone(), - listeners: vec![rginx_core::Listener { - id: "default".to_string(), - name: "default".to_string(), - server, - tls_termination_enabled: false, - proxy_protocol_enabled: false, - }], - default_vhost: rginx_core::VirtualHost { - id: "server".to_string(), - server_names: Vec::new(), - routes: Vec::new(), - tls: None, - }, - vhosts: Vec::new(), - upstreams: HashMap::from([ - ("insecure".to_string(), Arc::new(insecure)), - ("custom".to_string(), Arc::new(custom)), - ]), - }; + let snapshot = snapshot_with_upstreams([ + ("insecure".to_string(), Arc::new(insecure)), + ("custom".to_string(), Arc::new(custom)), + ]);Also applies to: 596-637, 713-754, 796-837, 901-942, 1464-1504
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/proxy/tests.rs` around lines 506 - 546, The test builds nearly identical rginx_core::Server and rginx_core::ConfigSnapshot structs inline; replace this duplicated construction with the existing helper functions snapshot_with_upstreams(...) or snapshot_with_upstreams_map(...) used around lines ~1651-1713. Update the test to call the appropriate helper and pass the upstreams (insecure and custom) so Server, RuntimeSettings, Listener, default_vhost and other shared fields come from the helper instead of being redefined here; do the same refactor for the other similar blocks (lines noted in the comment) to remove duplication.
554-560: 临时文件/目录建议统一用TempDir托管，避免失败路径遗留。这里混用了手动删除和 RAII；一旦断言提前失败，Line 644-645 / Line 946-951 的清理不会执行。建议和本文件其它测试保持一致，使用
TempDir自动清理。🧹 建议修复
- let unique = SystemTime::now() - .duration_since(UNIX_EPOCH) - .expect("system time should be after unix epoch") - .as_nanos(); - let path = std::env::temp_dir().join(format!("rginx-custom-ca-override-{unique}.pem")); + let temp_dir = TempDir::new().expect("temp dir should be created"); + let path = temp_dir.path().join("custom-ca-override.pem"); std::fs::write(&path, TEST_CA_CERT_PEM).expect("PEM file should be written"); @@ - std::fs::remove_file(path).expect("temp PEM file should be removed"); + // TempDir drop 自动清理- let unique = SystemTime::now() - .duration_since(UNIX_EPOCH) - .expect("system time should be after unix epoch") - .as_nanos(); - let dir = std::env::temp_dir().join(format!("rginx-client-identity-cache-{unique}")); - std::fs::create_dir_all(&dir).expect("temp dir should be created"); + let temp_dir = TempDir::new().expect("temp dir should be created"); + let dir = temp_dir.path().to_path_buf(); @@ - std::fs::remove_file(first_cert).expect("first cert should be removed"); - std::fs::remove_file(first_key).expect("first key should be removed"); - std::fs::remove_file(second_cert).expect("second cert should be removed"); - std::fs::remove_file(second_key).expect("second key should be removed"); - std::fs::remove_dir(dir).expect("temp dir should be removed"); + // TempDir drop 自动清理Also applies to: 644-645, 844-851, 946-951
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/proxy/tests.rs` around lines 554 - 560, Replace the manual temp-file creation and manual cleanup with a TempDir so cleanup is RAII-managed: create a tempfile::TempDir (e.g., let tmp = tempfile::tempdir()?) and build the PEM path by joining tmp.path() with the filename (instead of using SystemTime::now() -> unique and std::env::temp_dir()). Write TEST_CA_CERT_PEM to that path as before, and remove any explicit std::fs::remove_file / manual deletion code that cleans up path (the cleanup at the earlier referenced lines). Update all occurrences that use the manual "unique" + std::env::temp_dir() pattern (the instances creating variable path and writing the PEM in this file) to use the TempDir-backed path so tests auto-clean on failure.
🤖 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/tests.rs`:
- Around line 1806-1823: The spawned detached thread blocks indefinitely on
stream.read(&mut buffer); set read and write timeouts on the TcpStream before
performing I/O (e.g., via stream.set_read_timeout(...) and
stream.set_write_timeout(...)) inside the thread closure, handle timeout or
WouldBlock/TimedOut errors from stream.read and stream.write_all by returning
early (or closing the connection) instead of blocking, and keep the existing
status selection logic (statuses.lock().unwrap_or_else(...), StatusCode usage)
unchanged; ensure flush errors are ignored gracefully after applying the
timeouts.
In `@crates/rginx-runtime/src/bootstrap.rs`:
- Around line 583-587: The test currently binds a temporary socket via
bind_std_listener to get a free port, drops std_listener, then rebinds to the
same listen_addr which creates a race that can yield AddrInUse; instead reuse
the originally bound socket instead of dropping and rebinding—keep std_listener
and pass it directly into whatever API creates the runtime listener (e.g.,
convert the std_listener into the async listener or call the function that
accepts an existing std::net::TcpListener), or change the code paths around
bind_std_listener, std_listener, and listen_addr to call from_std/into_std
variants so the same socket is reused; update both occurrences (the block with
std_listener/listen_addr and the similar block at 606-610) to preserve and reuse
the bound socket rather than dropping then rebinding.
- Around line 419-425: The decrement+notify logic inside the spawned worker is
not panic-safe because it only runs after rginx_http::serve(...).await returns;
create a small Drop-guard inside the async closure (e.g., a local struct Guard
with Drop impl) instantiated before calling rginx_http::serve so that its Drop
always runs on task exit or panic and performs remaining_workers.fetch_sub(1,
Ordering::AcqRel) and, if the result was 1, calls
drain_completion_notify.notify_waiters(); ensure the guard lives for the whole
task lifetime (declare it at top of the closure) and remove or avoid duplicating
the manual decrement after serve returns.
---
Nitpick comments:
In `@crates/rginx-http/src/proxy/tests.rs`:
- Around line 506-546: The test builds nearly identical rginx_core::Server and
rginx_core::ConfigSnapshot structs inline; replace this duplicated construction
with the existing helper functions snapshot_with_upstreams(...) or
snapshot_with_upstreams_map(...) used around lines ~1651-1713. Update the test
to call the appropriate helper and pass the upstreams (insecure and custom) so
Server, RuntimeSettings, Listener, default_vhost and other shared fields come
from the helper instead of being redefined here; do the same refactor for the
other similar blocks (lines noted in the comment) to remove duplication.
- Around line 554-560: Replace the manual temp-file creation and manual cleanup
with a TempDir so cleanup is RAII-managed: create a tempfile::TempDir (e.g., let
tmp = tempfile::tempdir()?) and build the PEM path by joining tmp.path() with
the filename (instead of using SystemTime::now() -> unique and
std::env::temp_dir()). Write TEST_CA_CERT_PEM to that path as before, and remove
any explicit std::fs::remove_file / manual deletion code that cleans up path
(the cleanup at the earlier referenced lines). Update all occurrences that use
the manual "unique" + std::env::temp_dir() pattern (the instances creating
variable path and writing the PEM in this file) to use the TempDir-backed path
so tests auto-clean on failure.
In `@docs/reload-semantics.md`:
- Line 49: Replace the awkward phrase "新连接最终会变成不可达" in docs/reload-semantics.md
with a more natural expression; locate the exact string and change it to one of
the suggested alternatives such as "该 listener 将不再接受新连接" or "新连接将无法建立到该
listener" to improve readability while preserving the original meaning.
🪄 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: 02543df6-0a12-4cdc-93f9-d5c7b8e9fd32
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
crates/rginx-http/Cargo.tomlcrates/rginx-http/src/proxy/tests.rscrates/rginx-http/src/state/connections.rscrates/rginx-http/src/state/counters.rscrates/rginx-http/src/state/lifecycle.rscrates/rginx-http/src/state/traffic.rscrates/rginx-http/src/state/upstreams.rscrates/rginx-runtime/src/bootstrap.rsdocs/reload-semantics.md
✅ Files skipped from review due to trivial changes (2)
- crates/rginx-http/Cargo.toml
- crates/rginx-http/src/state/connections.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/rginx-http/src/state/upstreams.rs
- crates/rginx-http/src/state/lifecycle.rs
Owner
Author
|
补一轮收口，已处理并 resolve 最新 AI review。 落地在
验证：
|
This was referenced Apr 10, 2026
Merged
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
Prepare
v0.1.3-rc.5for release.This PR includes the staged maintenance and runtime work that landed during the
rc.5cycle:state.rsinto domain-focused modules while keepingSharedStateas the facadeproxy/mod.rsinto an entrypoint and move proxy-specific logic/tests into dedicated modules0.1.3-rc.5Validation
Validated locally with:
./scripts/test-fast.sh./scripts/test-slow.sh./scripts/run-tls-gate.shcargo clippy --workspace --all-targets --all-features --locked -- -D warnings./scripts/prepare-release.sh --tag v0.1.3-rc.5Release Notes
release/v0.1.3-rc.5v0.1.3-rc.5