refactor: modularize runtime and http stages 4-5#57
Merged
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 23 minutes and 5 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: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (15)
📝 WalkthroughWalkthrough此 PR 执行了大规模的代码模块化重构，将七个大型单文件模块（ Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Pull request overview
This PR advances the repository’s modularization roadmap by splitting large “god files” in rginx-runtime and rginx-http into smaller, responsibility-oriented modules, and then updating the modularization plan/baseline accordingly.
Changes:
- Complete stage 4: modularize
rginx-runtimelistener/bootstrap, OCSP refresher, and admin runtime code into directory modules. - Complete stage 5: modularize
rginx-httpcompression, handler dispatch + gRPC handling, proxy forwarding/request-body, and state snapshots/counters into submodules. - Update modularization plan documentation and remove stage-5 legacy baseline exemptions.
Reviewed changes
Copilot reviewed 79 out of 79 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/modularization_baseline.json | Removes now-unneeded legacy ceilings after modularization. |
| docs/ARCHITECTURE_CODEBASE_MODULARIZATION_PLAN.md | Marks stages 4–5 as completed and documents the module breakdown. |
| crates/rginx-runtime/src/ocsp/tests.rs | Updates OCSP tests to match the new module layout. |
| crates/rginx-runtime/src/ocsp/state.rs | Extracts state interactions for OCSP refresh metrics / TLS acceptor refresh. |
| crates/rginx-runtime/src/ocsp/spec.rs | Extracts config→OCSP refresh spec derivation. |
| crates/rginx-runtime/src/ocsp/scheduler.rs | Extracts OCSP refresh loop/scheduling into its own module. |
| crates/rginx-runtime/src/ocsp/refresh.rs | Extracts OCSP HTTP fetch helpers. |
| crates/rginx-runtime/src/ocsp/persist.rs | Extracts OCSP cache file write/clear helpers. |
| crates/rginx-runtime/src/ocsp/mod.rs | New OCSP module root wiring submodules together. |
| crates/rginx-runtime/src/ocsp/client.rs | Extracts OCSP HTTP client construction / root loading. |
| crates/rginx-runtime/src/ocsp.rs | Removes the old monolithic OCSP implementation file. |
| crates/rginx-runtime/src/bootstrap/listeners/tests.rs | Updates listener bootstrap tests for new module layout. |
| crates/rginx-runtime/src/bootstrap/listeners/reconcile.rs | Extracts reconcile/prune logic for listener worker groups. |
| crates/rginx-runtime/src/bootstrap/listeners/prepare.rs | Extracts preparation/binding construction for listener worker groups. |
| crates/rginx-runtime/src/bootstrap/listeners/mod.rs | New listeners module root wiring submodules together. |
| crates/rginx-runtime/src/bootstrap/listeners/join.rs | Extracts join/abort-join logic for listener worker tasks. |
| crates/rginx-runtime/src/bootstrap/listeners/group.rs | Extracts ListenerWorkerGroup definition and helpers. |
| crates/rginx-runtime/src/bootstrap/listeners/drain.rs | Extracts shutdown/abort helpers for groups. |
| crates/rginx-runtime/src/bootstrap/listeners/bind_udp.rs | Extracts UDP binding + inherited socket normalization. |
| crates/rginx-runtime/src/bootstrap/listeners/bind_tcp.rs | Extracts TCP bind helper. |
| crates/rginx-runtime/src/bootstrap/listeners/activate.rs | Extracts activation/spawn logic (workers + drain guard). |
| crates/rginx-runtime/src/bootstrap/listeners.rs | Removes the old monolithic listeners implementation file. |
| crates/rginx-runtime/src/admin/socket.rs | Extracts admin socket path/guard/permissions utilities. |
| crates/rginx-runtime/src/admin/service.rs | Refactors admin service to use new model/socket modules. |
| crates/rginx-runtime/src/admin/model.rs | Extracts admin request/response model types. |
| crates/rginx-runtime/src/admin/mod.rs | New admin module root wiring model/service/socket. |
| crates/rginx-http/src/state/snapshots/upstreams.rs | New upstream snapshot types in dedicated module. |
| crates/rginx-http/src/state/snapshots/traffic.rs | New traffic snapshot types in dedicated module. |
| crates/rginx-http/src/state/snapshots/tls.rs | New TLS snapshot types (+ OCSP refresh spec type) in dedicated module. |
| crates/rginx-http/src/state/snapshots/runtime.rs | New runtime status snapshot types in dedicated module. |
| crates/rginx-http/src/state/snapshots/reload.rs | New reload snapshot types in dedicated module. |
| crates/rginx-http/src/state/snapshots/mod.rs | New snapshots module root exporting snapshot types. |
| crates/rginx-http/src/state/snapshots/http.rs | New HTTP counters + mTLS snapshot types module. |
| crates/rginx-http/src/state/snapshots/delta.rs | New snapshot delta + module selection enums module. |
| crates/rginx-http/src/state/snapshots/active.rs | New ActiveState type in snapshots module. |
| crates/rginx-http/src/state/mod.rs | Switches from include!(snapshots.rs) to mod snapshots; and splits counters via multiple includes. |
| crates/rginx-http/src/state/counters/versions.rs | Extracts snapshot component version counters. |
| crates/rginx-http/src/state/counters/upstreams.rs | Extracts upstream stats counters/index building helpers. |
| crates/rginx-http/src/state/counters/traffic.rs | Extracts traffic stats counters/index building helpers. |
| crates/rginx-http/src/state/counters/rolling.rs | Extracts rolling-window counter implementation. |
| crates/rginx-http/src/state/counters/http.rs | Extracts HTTP counters + reload history snapshot helpers. |
| crates/rginx-http/src/state/counters/grpc.rs | Extracts gRPC traffic counters + snapshot helper. |
| crates/rginx-http/src/proxy/request_body/streaming.rs | Extracts streaming request-body relay implementation. |
| crates/rginx-http/src/proxy/request_body/replay.rs | Extracts replayable body + retry policy helpers. |
| crates/rginx-http/src/proxy/request_body/prepare.rs | Refactors request-body preparation to use extracted submodules/types. |
| crates/rginx-http/src/proxy/request_body/model.rs | Extracts request-body preparation model + errors/types. |
| crates/rginx-http/src/proxy/request_body/mod.rs | New request_body module root wiring model/prepare/replay/streaming/limits. |
| crates/rginx-http/src/proxy/request_body/limits.rs | Extracts request-body limit error detection helper. |
| crates/rginx-http/src/proxy/mod.rs | Removes now-unneeded import after request-body refactor. |
| crates/rginx-http/src/proxy/forward/types.rs | Extracts downstream request context/options types. |
| crates/rginx-http/src/proxy/forward/success.rs | Extracts success-path finalization (upgrade/non-upgrade). |
| crates/rginx-http/src/proxy/forward/streaming.rs | Extracts streaming body completion finalization. |
| crates/rginx-http/src/proxy/forward/setup.rs | Extracts forward-request preparation (timeouts, grpc-web, client select, body prep). |
| crates/rginx-http/src/proxy/forward/mod.rs | Rewires forward module to call extracted attempt/setup/success/streaming pieces. |
| crates/rginx-http/src/proxy/forward/grpc.rs | Adds grpc_response_deadline helper and keeps grpc-related utilities. |
| crates/rginx-http/src/proxy/forward/attempt.rs | Extracts the multi-peer attempt/failover loop as the forward_request entrypoint. |
| crates/rginx-http/src/handler/mod.rs | Simplifies handler root and wires new dispatch/grpc modules. |
| crates/rginx-http/src/handler/grpc/observability.rs | Extracts gRPC observability parsing + access log body wrapper. |
| crates/rginx-http/src/handler/grpc/mod.rs | New grpc module root exporting submodule APIs. |
| crates/rginx-http/src/handler/grpc/metadata.rs | Extracts gRPC metadata parsing from request headers/path. |
| crates/rginx-http/src/handler/grpc/grpc_web.rs | Extracts grpc-web observability trailer parsing (binary + text). |
| crates/rginx-http/src/handler/grpc/error.rs | Extracts grpc/grpc-web error response builder. |
| crates/rginx-http/src/handler/dispatch/select.rs | Extracts host/vhost/route selection helpers. |
| crates/rginx-http/src/handler/dispatch/route.rs | Extracts route execution (proxy/return) + early-data rejection response. |
| crates/rginx-http/src/handler/dispatch/response.rs | Extracts downstream response finalization + helpers. |
| crates/rginx-http/src/handler/dispatch/mod.rs | New dispatch module root implementing request handling via submodules. |
| crates/rginx-http/src/handler/dispatch/date.rs | Extracts cached HTTP Date header generation. |
| crates/rginx-http/src/handler/dispatch/authorize.rs | Extracts access-control and rate-limit enforcement. |
| crates/rginx-http/src/handler/dispatch.rs | Removes the old monolithic dispatch implementation file. |
| crates/rginx-http/src/handler/access_log.rs | Updates imports to align with new dispatch/grpc module structure. |
| crates/rginx-http/src/compression/options.rs | Extracts compression option struct / route mapping. |
| crates/rginx-http/src/compression/mod.rs | New compression module root orchestrating encoding and eligibility. |
| crates/rginx-http/src/compression/encode.rs | Extracts gzip/brotli byte compression helpers. |
| crates/rginx-http/src/compression/content_type.rs | Extracts response eligibility checks based on headers/content-type. |
| crates/rginx-http/src/compression/accept_encoding.rs | Extracts Accept-Encoding parsing and Vary header merging. |
| crates/rginx-http/src/compression.rs | Removes the old monolithic compression implementation file. |
💡 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: 51
🤖 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/compression/accept_encoding.rs`:
- Around line 110-126: The parser parse_accept_encoding_item currently only
matches "q=" in lowercase; update it to perform case-insensitive parameter name
matching per RFC 9110 (e.g., for parts like "Q=0.5" or "q=0.5") by comparing the
parameter name in a case-insensitive fashion before extracting the value and
parsing it as f32; ensure you trim the value and still return None on parse
failure, referencing parse_accept_encoding_item and the loop over parts to
implement this change (for example by lowercasing the parameter name or using a
case-insensitive prefix check and then parsing the substring after '=').
In `@crates/rginx-http/src/compression/content_type.rs`:
- Around line 32-57: The code recomputes trimming/splitting: in
content_type_is_eligible you already extract `mime` but then call
`is_compressible_content_type(content_type)` which repeats
`split(';').next().unwrap_or(...).trim()`; update the call to pass the
already-trimmed `mime` into `is_compressible_content_type`, or change
`is_compressible_content_type` to document/accept a pre-trimmed mime (and remove
its internal split/trim) so the duplicate parsing is eliminated; adjust
references accordingly in `content_type_is_eligible` and
`is_compressible_content_type`.
In `@crates/rginx-http/src/compression/mod.rs`:
- Around line 69-80: 在 compress_bytes 的匹配的两个 fallback 分支（Ok(_) 和
Err(error)）中不要直接用原 parts 返回 buffered body；改为先调用
parts_without_compression_metadata(parts) 清除压缩相关头，再在返回前显式设置 Content-Length 为
collected.len()，然后使用 Response::from_parts(new_parts, full_body(collected)) 返回。参照
compress_bytes、full_body、parts_without_compression_metadata 和
Response::from_parts 以定位修改点，并保证三条分支（成功压缩、无收益、压缩失败）在头部状态上一致。
- Around line 84-88: Replace the temporary String allocation and panicking
conversion when setting CONTENT_LENGTH by using HeaderValue::from with the byte
length directly: in the parts.headers.insert call that currently uses
HeaderValue::from_str(&compressed.len().to_string()).expect(...), call
HeaderValue::from(compressed.len()) instead (using HeaderValue::from and
compressed.len()) so you avoid to_string() allocation and the expect().
In `@crates/rginx-http/src/handler/dispatch/date.rs`:
- Around line 33-35: current_unix_epoch_seconds currently swallows the
SystemTime error and returns 0 (producing misleading "Date: Thu, 01 Jan 1970");
change it to log a warning when duration_since(UNIX_EPOCH) fails (use warn! with
the error or context) before returning a fallback (still 0 if you don't
implement a cached fallback), i.e. replace the unwrap_or(0) path in
current_unix_epoch_seconds() with explicit error handling that calls warn!
(including the error message) and then returns the fallback; if you prefer
stronger behavior, consider using a cached previous timestamp instead of 0 and
reference the same function name current_unix_epoch_seconds for the change.
- Around line 6-21: The global Mutex-based cache (HTTP_DATE_CACHE and
current_http_date) causes lock contention on the hot response path; replace it
with a lock-free read path using arc_swap::ArcSwap<HeaderValue> plus an
AtomicU64 for the last-updated epoch. Concretely, remove
OnceLock<Mutex<CachedHttpDate>>/CachedHttpDate, add a static
ArcSwap<HeaderValue> (holding Arc<HeaderValue>) and a static AtomicU64
last_epoch; in current_http_date() first atomically load last_epoch and compare
to current_unix_epoch_seconds(); if equal return cache.load_full()/clone (no
lock), otherwise build a new Arc<HeaderValue> for the new second and update
cache.store(new_arc) and last_epoch.store(new_seconds); ensure construction and
store are atomic enough for correctness and drop the Mutex usage. Use symbols
ArcSwap, AtomicU64, current_http_date, and HTTP_DATE_CACHE replacement when
making changes.
In `@crates/rginx-http/src/handler/dispatch/mod.rs`:
- Around line 76-86: The code currently accepts any client-provided
x-request-id; change the logic in the request_id extraction block so the
extracted value is validated against a safe pattern and length (e.g. allow only
[A-Za-z0-9_-], max 128 bytes) and only used if it passes validation, otherwise
call state.next_request_id(); ensure HeaderValue::from_str is only called on the
validated/fallback id and handle conversion errors by falling back to
state.next_request_id(); update the insertion via
request.headers_mut().insert("x-request-id", ...) to use the validated/fallback
id.
- Around line 70-95: 当前代码在热路径中对同一 URI 做了两次字符串化——先构造 path via
request.uri().path_and_query() -> String，再独立做 request_path =
request.uri().path().to_string()，导致多余分配。修复方法：只对 URI 做一次到 String 的转换（保留变量 path
或改名为 full_path_str），然后用 split_once('?') 或从该字符串切片派生 request_path（或通过
path.split_at/查找 '?' 的索引并切片）以避免第二次 to_string()。在本文件中关注变量名
method、request_version, request_headers, path 和 request_path，并删除对
request.uri().path().to_string() 的重复调用，确保其他对 request_path 的使用点改为使用从 path 派生的值。
- Around line 105-132: The dispatch function mixes vhost/route selection
(select_vhost_for_request, router::select_route_in_vhost_with_context) with
multiple metric/telemetry calls (state.record_downstream_request,
state.record_mtls_request, state.record_grpc_request), making the handler noisy;
extract these recording responsibilities into a single helper (e.g.,
record_request_admission) placed in select.rs or a new metrics.rs that accepts
(state, listener_id, &selected_vhost_id, selected_route_id: Option<&str>,
tls_client_identity: Option<...>, grpc_request: Option<&GrpcRequest>) and inside
it perform the existing logic to compute selected_route_id.as_deref(),
response_compression_options handling if needed, call record_downstream_request,
record_mtls_request and record_grpc_request appropriately; then replace the
inlined metric calls in dispatch with a single call to that helper so handle
focuses on selection → execute → finalize.
- Around line 62-68: 当前代码使用 config.listener(listener_id).cloned().expect(...) 会在
listener 被移入 retired_listeners（由 retire_listener_runtime() 管理）时触发 panic；请改为使用
state.current_listener(listener_id).await which falls back to retired_listeners
(same behavior as accept loop) — i.e. replace the ConfigSnapshot lookup
(config.listener(...)) with a call to state.current_listener(listener_id).await
and keep the existing expect(...) or, if preferred, replace expect with graceful
short-circuiting (return 503/421 and log/metric) to avoid panics when the
listener is retired; reference functions/types: state.current_listener,
ConfigSnapshot.listener, retired_listeners, retire_listener_runtime.
In `@crates/rginx-http/src/handler/dispatch/response.rs`:
- Line 48: 当前代码在每次调用 response.headers_mut().insert("x-request-id",
request_id_header) 时重复构造 HeaderName；请在模块级创建一个静态常量，例如 pub static X_REQUEST_ID:
HeaderName = HeaderName::from_static("x-request-id")，然后在代码中用
response.headers_mut().insert(X_REQUEST_ID.clone(),
request_id_header)（或按类型要求直接使用 X_REQUEST_ID）替换原有字符串字面量，从而在热路径复用 HeaderName
实例并避免重复解析。
- Around line 55-68: The function response_body_bytes_sent should take a &Method
instead of &str to avoid stringly-typed comparisons; change the signature to
accept method: &Method and update its HEAD check to method == &Method::HEAD (or
method == Method::HEAD) and keep the same header-parsing logic for
CONTENT_LENGTH, then update all call sites that currently pass method.as_str()
to pass the Method reference (remove the .as_str()). Ensure uses of Method
equality and imports remain correct (Method, HttpResponse, CONTENT_LENGTH) so
type checks pass.
In `@crates/rginx-http/src/handler/dispatch/route.rs`:
- Around line 19-27: build_route_response currently takes many parameters
(state, active, listener, client_address, request_id) which should be grouped
into a single context to simplify the API; define a new struct
RouteDispatchContext containing SharedState, ActiveState,
ListenerRequestContext<'_>, ClientAddress, and request_id, update the signature
of build_route_response to accept (request: Request<HttpBody>, route:
RouteExecutionContext, ctx: RouteDispatchContext) so callers only pass the
request and route (and the new context), and update all call sites and uses
inside build_route_response to read those fields from RouteDispatchContext
(preserving RouteExecutionContext.route.action usage).
- Around line 58-68: The response builder currently adds action.location for any
status and inserts it raw, which can leak URLs for non-3xx responses and panic
if the value contains invalid header characters; update the code that constructs
the response (the http::Response::builder() block that uses action.status,
action.location and builder.body(full_body(body))) to only add the "location"
header when action.status.is_redirection() is true, and validate/construct the
header with HeaderValue::from_str(&action.location) (handling Err by omitting
the header or returning an appropriate error) instead of passing the raw string
to builder.header to avoid panics from invalid characters.
- Around line 52-56: The RouteAction::Return branch currently falls back to
"Redirect" when action.body is None even if action.status.canonical_reason() is
None; change the body generation in the RouteAction::Return handling so that if
action.body is None you check the status: if status.is_redirection() (3xx) use
"Redirect\n", otherwise fall back to the numeric status string with newline
(e.g. format!("{}\n", action.status.as_u16())) or an empty body per desired
behavior; ensure you update the content_length calculation to use the new body
value and keep references to action.body, action.status.canonical_reason(), and
the RouteAction::Return match arm to locate where to implement this logic.
In `@crates/rginx-http/src/handler/dispatch/select.rs`:
- Around line 17-24: select_vhost_for_request currently calls
request_host(headers, uri).to_string(), causing an unnecessary heap allocation;
change it to pass the &str directly to router::select_vhost by using the return
value of request_host(headers, uri) (no to_string) so the third parameter to
router::select_vhost remains &str; update the function body in
select_vhost_for_request (which takes config: &ConfigSnapshot and returns
&VirtualHost) to forward request_host(...) directly to router::select_vhost to
eliminate the allocation.
In `@crates/rginx-http/src/handler/grpc/error.rs`:
- Around line 114-129: In encode_grpc_web_error_body: ensure trailer header
names are written lowercase by calling .as_str().to_ascii_lowercase() (or
equivalent) before pushing to trailer_block, switch line separators to use '\n'
(or normalize to '\r\n' consistently) when appending header lines, and replace
the unchecked cast (trailer_block.len() as u32) with a checked conversion using
u32::try_from(trailer_block.len()).expect("trailer block too large for
grpc-web") so any overflow is explicit.
- Around line 66-70: 目前对 message 使用 sanitize_grpc_message 后仍可能包含非 ASCII 字节，导致
HeaderValue::from_str(&message).expect(...) 在遇到高位字节时 panic；请在构造
grpc_message_value 时改为对 message 进行符合 gRPC 规范的 UTF-8 百分号编码（或至少剔除/替换所有不可见或非
0x20–0x7E/HT 字节），然后使用 HeaderValue::from_bytes(encoded_bytes) 来创建头部，替换原先对
HeaderValue::from_str 的调用（参考 sanitize_grpc_message、grpc_message_value
的使用位置）；确保任何编码或替换失败都以安全路径（不 panic）处理并记录/忽略不合法字符。
In `@crates/rginx-http/src/handler/grpc/grpc_web.rs`:
- Around line 198-225: The loop in decode_grpc_web_text_observability_chunk is
doing per-byte extend_from_slice(&[*byte]) which is slow; change it to append
filtered bytes in bulk by either (a) reserve data.len() on carryover and push
bytes with carryover.put_u8(*byte) inside the filter loop, or (b) collect the
non-whitespace bytes into a temporary Vec/Bytes and then
carryover.extend_from_slice(&temp) once; update imports to include bytes::BufMut
if you use put_u8. This eliminates the one-byte slice allocation/memcpy in the
hot path while keeping the rest of the function (complete_len calculation,
base64 decode, and error handling) unchanged.
- Around line 59-89: finish 的文本分支对 decoded/empty 两种情况分别调用 observe_binary_chunk
导致重复，建议将分支合并：在 finish 中对 is_text 情况先调用
decode_grpc_web_text_observability_final(&mut self.encoded_carryover), match
其返回的 Ok(Some(decoded)) / Ok(None) 为一个统一的借用切片（decoded.as_slice() 或 &[]），然后只做一次
self.observe_binary_chunk(slice, true, grpc) 并保留错误处理；非文本分支仍然调用
self.observe_binary_chunk(&[], true, grpc)（或将其合并为统一路径），以减少重复代码，同时继续使用 same error
handling and setting self.disabled on Err.
In `@crates/rginx-http/src/handler/grpc/metadata.rs`:
- Around line 21-23: grpc 子模块的 grpc_protocol 函数不应直接调用兄弟模块的私有工具
crate::handler::dispatch::header_value，造成反向依赖；请将该纯字符串/Header 提取工具上移到
handler::mod 或新建独立的 handler::headers 工具模块（例如 pub fn header_value(...)），在
grpc::metadata::grpc_protocol 中改为使用新移动的公用函数或直接使用 headers.get(...) 取值（参考
grpc::error 已用的方式），并更新相应的引用以移除对 crate::handler::dispatch::header_value 的依赖。
- Around line 25-33: The current MIME matching uses starts_with on the variable
mime (checks like starts_with("application/grpc")) which can false-positive
match longer subtypes (e.g., "application/grpcfoo"); change the logic to verify
a subtype boundary after matching the "application/<subtype>" prefix: first
strip the "application/" prefix, check the exact subtype ("grpc", "grpc-web",
"grpc-web-text") and then ensure the remaining characters are either empty or
start with a valid delimiter (e.g., '+', ';', ',', or end-of-string). Update the
three branches that inspect mime (the starts_with checks) to use this
exact-subtype + delimiter validation so only true gRPC media types are accepted.
In `@crates/rginx-http/src/proxy/request_body/mod.rs`:
- Around line 1-22: Replace the wildcard import use super::*; with an explicit
list of only the parent-module symbols this file actually uses (e.g. use
super::{ProxyError, UpstreamPeer, UpstreamRequest, PeerConnection,
RequestMetadata} — replace these example names with the real identifiers
referenced by this module), remove the wildcard, and update the import line
accordingly; audit the file to find each symbol currently relied on from the
parent proxy module and add it to the explicit use list (add or remove items as
you discover them), then run a build/tests to ensure no missing imports remain.
In `@crates/rginx-http/src/proxy/request_body/streaming.rs`:
- Around line 70-73: The code calls clone_box_error(error.as_ref()) twice in the
Some(Err(error)) arm (used in frame_tx.send(...) and in break Err(...)), causing
duplicate boxing; fix by cloning once into a local variable (e.g. let boxed_err
= clone_box_error(error.as_ref())) and then use boxed_err for both
frame_tx.send(Err(boxed_err.clone_or_move?)) and break Err(boxed_err) (or move
it into break and clone only if BoxError is not cheaply clonable), adjusting
ownership accordingly; references: frame_tx, clone_box_error, and the
Some(Err(error)) match arm.
- Around line 55-83: The relay_streaming_request_body loop currently ignores
failures from frame_tx.send(...) so the spawned task keeps reading the upstream
body even after the downstream RelayedRequestBody has been dropped; update the
loop in relay_streaming_request_body to check the Result of frame_tx.send(...)
and short-circuit (break) when send returns Err to stop reading and exit the
task early (choose to break with Ok(()) to treat drop as graceful or propagate
an error if you prefer), and add a brief comment by the frame_tx.send call
explaining why we short-circuit when the receiver is gone; reference symbols:
relay_streaming_request_body, frame_tx.send, RelayedRequestBody::new.
In `@crates/rginx-http/src/state/counters/grpc.rs`:
- Around line 1-84: The record_status method on GrpcTrafficCounters currently
matches raw string literals (e.g. "0","1",...,"14") which is brittle; change it
to normalize and parse the incoming status (trim whitespace, then parse to u32)
and switch on the parsed integer, or map parsed values to a central set of
status constants defined alongside snapshots (e.g. in the same module as
state/snapshots/traffic.rs). Update GrpcTrafficCounters::record_status to accept
Option<&str>, return early on None or parse errors (increment
status_other_total), and use a match on the parsed u32 to increment the
corresponding AtomicU64 fields (status_0_total, status_1_total, ...,
status_14_total) so all magic literals are centralized and robust to formatting
variations.
- Around line 35-65: The current record_status method conflates a missing
grpc-status (None) with unknown status codes by sending both to
status_other_total; add a new atomic counter field e.g., status_missing_total
and update record_status (function name record_status) to increment
status_missing_total when status is None while keeping status_other_total for
the _ (unknown Some(x)) branch; update any constructor/Default impl to
initialize the new status_missing_total and adjust callers/tests that inspect
these counters accordingly.
In `@crates/rginx-http/src/state/counters/http.rs`:
- Around line 1-122: The snapshot() implementation in HttpCounters repeats the
pattern `field: self.field.load(Ordering::Relaxed)` for many fields; introduce a
declaration macro (e.g. macro_rules! snapshot_fields!) and use it inside
HttpCounters::snapshot to expand each field mapping, so you list the fields once
in the macro invocation and generate `field: self.field.load(Ordering::Relaxed)`
entries; update HttpCounters::snapshot to call that macro (keeping
Ordering::Relaxed) to reduce boilerplate while preserving the same
HttpCountersSnapshot construction and semantics.
In `@crates/rginx-http/src/state/counters/rolling.rs`:
- Around line 1-127: This file currently relies on include! to inherit symbols
from the parent module (e.g., Mutex, VecDeque, StatusCode, SystemTime,
UNIX_EPOCH, MAX_RECENT_WINDOW_SECS, RECENT_WINDOW_SECS,
RecentTrafficStatsSnapshot, RecentUpstreamStatsSnapshot), so fix it by
converting the counters into a proper submodule and/or adding explicit imports:
create a proper mod counters { mod rolling; } in the parent and update this file
to be a real module that declares the needed use statements (use
std::sync::Mutex; use std::collections::VecDeque; use std::time::{SystemTime,
UNIX_EPOCH}; use crate::state::MAX_RECENT_WINDOW_SECS; use
crate::state::RECENT_WINDOW_SECS; use
crate::state::snapshots::{RecentTrafficStatsSnapshot,
RecentUpstreamStatsSnapshot}; use http::StatusCode;), or alternatively add those
use lines at the top of this file; ensure structs and functions like
RollingCounter, RecentTrafficStatsCounters, RecentUpstreamStatsCounters,
window_now_secs, and trim_old_buckets compile without relying on
include!-injected scope.
- Around line 31-48: increment_at currently assumes incoming second is
non-decreasing and may push_back out-of-order buckets when the clock is rolled
back; before matching on buckets.back_mut() in increment_at (after
trim_old_buckets), detect rollback by comparing second with buckets.back().0
and, if buckets.back().0 > second, pop_back repeatedly until the back is <=
second (or buckets is empty) to restore time order, then proceed to increment
the last bucket if equal or push_back a new (second,1); refer to increment_at,
trim_old_buckets, buckets and MAX_RECENT_WINDOW_SECS.
In `@crates/rginx-http/src/state/counters/upstreams.rs`:
- Around line 39-73: The build_upstream_stats_map logic reuses peer counters by
looking up current.peers.get(&peer.url), which will drop stats when peer.url
changes; update the codebase by adding a clear comment/docstring in or above
build_upstream_stats_map (and optionally near the UpstreamPeerStats/peer.url
definition) stating that peer URL is the unique identity and any change to
peer.url on reload is treated as a new peer whose counters are reset, so
operators know this behavior (or alternatively implement a stable identity
lookup if you intend to preserve counters across URL renames). Ensure the
comment references build_upstream_stats_map, peer.url, UpstreamPeerStats and
current.peers so it’s easy to find and understand.
In `@crates/rginx-http/src/state/mod.rs`:
- Around line 52-58: The current include!("counters/*.rs") and
include!("helpers.rs") should be converted to real submodules to match the
snapshots refactor: replace those include! lines with declarations like mod
counters; pub(super) use counters::*; (or fine-grained re-exports) and mod
helpers; pub(super) use helpers::*;, then update each file under counters/ and
helpers.rs to be standalone modules by adding their own use imports (e.g.,
Mutex, StatusCode, SystemTime, Snapshot types, etc.) and removing any
parent-module-dependent assumptions so rust-analyzer and cross-file navigation
work correctly.
In `@crates/rginx-http/src/state/snapshots/delta.rs`:
- Around line 66-71: 常量 ALL 的数组长度被硬编码为 [Self; 5]，新增 SnapshotModule
变体时容易忘记同步长度；请将 pub const ALL: [Self; 5] = [...] 改为不含固定长度的静态切片，例如 pub const ALL:
&'static [Self] = &[Self::Status, Self::Counters, Self::Traffic,
Self::PeerHealth, Self::Upstreams]; 并把 pub fn all() 改为直接调用
Self::ALL.to_vec()（或移除 all() 让调用方直接用 Self::ALL.to_vec()），以消除手动维护长度的风险；相关符号：ALL
常量与 all() 方法。
- Around line 73-76: SnapshotModule::normalize currently treats Some(&[]) as an
explicit empty selection while None means "all", which is confusing for JSON
clients; update normalize to treat an empty slice the same as None (i.e., return
all modules) by checking for include.is_none() || include.unwrap().is_empty()
and then returning Self::ALL, and also update the AdminRequest::GetSnapshot {
include } documentation to explicitly state that an omitted include or an empty
array both select all modules; refer to SnapshotModule::normalize and
AdminRequest::GetSnapshot { include } when making these changes.
In `@crates/rginx-http/src/state/snapshots/tls.rs`:
- Around line 11-46: TlsListenerStatusSnapshot has inconsistent serialization
for Option fields: some (HTTP/3 group) use #[serde(skip_serializing_if =
"Option::is_none")] while others (default_certificate, versions,
session_resumption_enabled, session_tickets_enabled, session_cache_size,
session_ticket_count, client_auth_mode, client_auth_verify_depth) do not,
causing mixed null vs missing semantics; update the struct
TlsListenerStatusSnapshot by adding #[serde(skip_serializing_if =
"Option::is_none")] to each of those Option fields (default_certificate,
versions, session_resumption_enabled, session_tickets_enabled,
session_cache_size, session_ticket_count, client_auth_mode,
client_auth_verify_depth) to make snapshot JSON sparse and consistent with the
HTTP/3 fields, and consider applying the same change to
TlsCertificateStatusSnapshot and TlsOcspStatusSnapshot for uniform behavior.
- Around line 93-102: The TlsOcspRefreshSpec struct is placed in the snapshots
module and publicly exported despite being an internal-only type used only by
the OCSP refresh scheduler (refresh_ocsp_staples); rename or relocate it to
reduce confusion: either rename TlsOcspRefreshSpec to a non-snapshot-sounding
internal name (e.g., TlsOcspRefreshCmd) and keep it private, or move the struct
out of the snapshots module into an internal tls/ocsp module and make it
non-pub; update all references (notably any usages in refresh_ocsp_staples) to
the new name/location and remove any snapshot-specific derives/exports so it no
longer appears as part of admin snapshots.
In `@crates/rginx-http/src/state/snapshots/traffic.rs`:
- Around line 5-20: GrpcTrafficSnapshot 目前只显式列出部分 gRPC 状态码（fields:
requests_total, protocol_grpc_total, protocol_grpc_web_total,
protocol_grpc_web_text_total, status_0_total, status_1_total, status_3_total,
status_4_total, status_7_total, status_8_total, status_12_total,
status_14_total, status_other_total），新增具体状态码会改变 wire
协议；为避免兼容性问题，在将来添加细化状态码字段时请通过 schema_version 升级流程预留兼容方案：确保新加字段在
Default::default() 下对老消费者透明、在序列化/反序列化时保持向后兼容，并在新增具体状态累积字段时同步更新任何 recent_*
或其它累积/派生字段（确保 recent_* 与累积字段同时演进且有版本标记以供下游消费判断）。
In `@crates/rginx-runtime/src/admin/service.rs`:
- Around line 89-96: The response JSON serialization failure currently maps
serde_json::to_vec(&response) errors to io::ErrorKind::InvalidData; change this
to represent an internal/server error by returning
io::Error::new(io::ErrorKind::Other, ...) or using io::Error::other(...) so
callers can distinguish input (InvalidData from request parsing) vs internal
serialization faults; update the error construction around the
serde_json::to_vec(&response) call (in the same block that calls
dispatch_request and writes via writer.write_all/flush) to use ErrorKind::Other
and keep the same descriptive message.
In `@crates/rginx-runtime/src/admin/socket.rs`:
- Around line 28-40: prepare_path currently unconditionally removes an existing
path which may delete non-socket files; change it to use
fs::symlink_metadata(path) instead of path.exists() to avoid following symlinks,
inspect metadata.file_type() (on Unix check S_IFSOCK via
metadata.file_type().is_socket() or raw_mode & S_IFSOCK) and only call
fs::remove_file(path) if the existing target is a Unix domain socket, otherwise
return an io::Error (e.g., io::ErrorKind::AlreadyExists) so callers can handle
it explicitly; keep the create_dir_all(parent)? behavior unchanged and ensure
error propagation.
- Around line 69-72: The current flow calls set_admin_socket_permissions (which
uses fs::set_permissions) after UnixListener::bind, leaving a window where the
socket file is created with default umask; mitigate by removing the post-bind
fs::set_permissions approach and instead apply permissions directly to the
socket file descriptor to avoid the TOCTOU window: after creating the listener
via UnixListener::bind (the call in service.rs), call a new variant of
set_admin_socket_permissions that accepts the listener's raw fd (use
listener.as_raw_fd()) and invoke nix::unistd::fchmod on that fd to set 0o600
immediately; alternatively (if changing to fchmod is undesirable) temporarily
set a stricter umask before calling UnixListener::bind and restore it
afterwards, or ensure the parent directory has restrictive perms (0700) as a
deployment safeguard.
In `@crates/rginx-runtime/src/bootstrap/listeners/join.rs`:
- Around line 5-23: The current join_listener_worker_groups function
short-circuits on the first Err from join_listener_worker_group which leaves
later active groups unjoined and draining_listener_groups unprocessed; change
the flow to iterate all groups while capturing the first error (e.g., store the
first Err in a local variable), continue calling join_listener_worker_group for
every active group and every draining group, call
http_state.remove_retired_listener_runtime(&listener_id).await for each drained
group, clear the maps as before, then at the end return the saved error if any
(or Ok(()) if none) so all cleanup runs before returning; reference
join_listener_worker_groups, join_listener_worker_group, active_listener_groups,
draining_listener_groups, and http_state.remove_retired_listener_runtime when
applying the change.
In `@crates/rginx-runtime/src/bootstrap/listeners/prepare.rs`:
- Around line 100-138: The loop in prepare_added_listener_bindings builds
`prepared` (Vec) but returns early on errors from checks or from calls that use
`?` (e.g., bind_std_listener, bind_std_udp_sockets,
prepare_listener_worker_group) without explicitly cleaning up previously bound
sockets; change the error paths so that before any early return you explicitly
drop/clear the `prepared` collection (or otherwise close the sockets) to
deterministically release resources—specifically, handle the checks that
currently `return Err(...)` and replace the `?`-propagated calls around
bind_std_listener, bind_std_udp_sockets and prepare_listener_worker_group with
error-handling that first drops `prepared` and then returns the error so
previous bindings are released immediately.
- Around line 35-47: 统一对 accept_workers 应用下限保护：在 prepare.rs 的新建绑定路径中调用
bind_std_udp_sockets 时不要直接传入 config.runtime.accept_workers，而是像继承路径那样先做 let
desired_udp_socket_count = config.runtime.accept_workers.max(1) 并传入该变量；同时在
prepare_added_listener_bindings 中处理新建绑定（原第 128 行）也对传入 bind_std_udp_sockets 的
accept_workers 做 .max(1) 规范化，确保无论继承还是新建绑定都至少请求 1 个 UDP 套接字。
In `@crates/rginx-runtime/src/bootstrap/listeners/reconcile.rs`:
- Around line 31-49: Add a brief comment above the
remove→retire_listener_runtime→initiate_shutdown→push-to-draining sequence
stating that the order is intentional to stop exposing the listener to state
before initiating shutdown to prevent new connections, then replace the brittle
expect(...) when updating remaining active_listener_groups with a defensive if
let Some(next_listener) pattern that logs a tracing::error! including
listener_id when next_by_id lacks the entry (and skip updating that group), so
the function won't panic if invariants change in future refactors; alternatively
consider using an entry() style merge if you prefer to update/remove in one
atomic step.
- Around line 64-82: The loop in prune_draining_listener_groups manually tracks
an index and calls Vec::remove(index) which shifts elements O(n); replace this
with a more efficient Vec API (e.g., use swap_remove when order doesn't matter
or collect finished indices and remove them in reverse) to avoid repeated
shifting. Modify prune_draining_listener_groups to iterate
draining_listener_groups, detect groups where ListenerWorkerGroup::is_finished()
is true, take ownership of the group via swap_remove (or use retain_mut with a
closure that joins finished groups), then call join_listener_worker_group(&mut
group).await and http_state.remove_retired_listener_runtime(&listener_id).await;
ensure listener_id is still captured before swap_remove and preserve existing
error logging/tracing behavior.
In `@crates/rginx-runtime/src/ocsp/mod.rs`:
- Around line 24-26: The current pub async fn run(state: SharedState, shutdown:
watch::Receiver<bool>) merely forwards to scheduler::run and adds no logic;
replace this wrapper by re-exporting the scheduler function (pub use
scheduler::run) so callers reference the same implementation and avoid duplicate
signatures — ensure scheduler::run is pub and its signature matches SharedState
and watch::Receiver<bool> so the re-export compiles.
In `@crates/rginx-runtime/src/ocsp/persist.rs`:
- Around line 19-25: The rename failure can leave temp files (temp_path) behind
in write_ocsp_cache_file and clear_ocsp_cache_file; update both functions so
that if tokio::fs::rename(&temp_path, path).await fails you perform a
best-effort cleanup by calling tokio::fs::remove_file(&temp_path).await
(ignore/remove any error from the cleanup) before returning the original error,
and add a brief comment in clear_ocsp_cache_file explaining why you choose
write+rename vs. simpler tokio::fs::remove_file(path).await (or change to
remove_file if you prefer file disappearance semantics).
In `@crates/rginx-runtime/src/ocsp/refresh.rs`:
- Around line 12-18: 当前循环对每个 responder_url 都调用 request_body.clone() 导致不必要的复制；在
refresh.rs 的循环（responder_urls / errors Vec / fetch_ocsp_response_from_url
调用）中改为在最后一次重试将 request_body 移动（move）给
fetch_ocsp_response_from_url，而在非最后一次重试使用廉价的引用计数克隆（例如将请求类型改为 Bytes 并使用
Bytes::clone()），或按需只在需要复用时 clone，从而避免对每个 URL 做完全复制。
- Around line 41-67: The current timeout only wraps client.request(request) so
reading the response body via body.frame().await can hang; wrap the full fetch
(request + body read loop) in the same timeout or enforce a separate total read
timeout: create an async block that performs client.request(request).await,
checks response.status(), runs the while let Some(frame) = body.frame().await
loop collecting payload (respecting rginx_http::MAX_OCSP_RESPONSE_BYTES), and
then call tokio::time::timeout(OCSP_FETCH_TIMEOUT, that_async_block).await to
ensure OCSP_FETCH_TIMEOUT covers both the response headers and the body read
(references: OCSP_FETCH_TIMEOUT, client.request,
response.into_body()/body.frame(), payload,
rginx_http::MAX_OCSP_RESPONSE_BYTES).
In `@crates/rginx-runtime/src/ocsp/scheduler.rs`:
- Around line 21-28: Subscribe to updates before performing the initial OCSP
refresh and consume the interval's immediate first tick so the manual refresh
isn't duplicated: move the call to state.subscribe_updates() to before calling
refresh_ocsp_staples(&state, &client).await, create the
tokio::time::interval(OCSP_REFRESH_INTERVAL) as before, then await one tick
(e.g., interval.tick().await) to push the first scheduled tick forward, and only
afterwards perform the initial refresh (calling refresh_ocsp_staples) and enter
the loop that selects on interval.tick() and revisions.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 98070f06-90de-4493-9506-f2927b8299d7
📒 Files selected for processing (79)
crates/rginx-http/src/compression.rscrates/rginx-http/src/compression/accept_encoding.rscrates/rginx-http/src/compression/content_type.rscrates/rginx-http/src/compression/encode.rscrates/rginx-http/src/compression/mod.rscrates/rginx-http/src/compression/options.rscrates/rginx-http/src/handler/access_log.rscrates/rginx-http/src/handler/dispatch.rscrates/rginx-http/src/handler/dispatch/authorize.rscrates/rginx-http/src/handler/dispatch/date.rscrates/rginx-http/src/handler/dispatch/mod.rscrates/rginx-http/src/handler/dispatch/response.rscrates/rginx-http/src/handler/dispatch/route.rscrates/rginx-http/src/handler/dispatch/select.rscrates/rginx-http/src/handler/grpc.rscrates/rginx-http/src/handler/grpc/error.rscrates/rginx-http/src/handler/grpc/grpc_web.rscrates/rginx-http/src/handler/grpc/metadata.rscrates/rginx-http/src/handler/grpc/mod.rscrates/rginx-http/src/handler/grpc/observability.rscrates/rginx-http/src/handler/mod.rscrates/rginx-http/src/proxy/forward/attempt.rscrates/rginx-http/src/proxy/forward/grpc.rscrates/rginx-http/src/proxy/forward/mod.rscrates/rginx-http/src/proxy/forward/setup.rscrates/rginx-http/src/proxy/forward/streaming.rscrates/rginx-http/src/proxy/forward/success.rscrates/rginx-http/src/proxy/forward/types.rscrates/rginx-http/src/proxy/mod.rscrates/rginx-http/src/proxy/request_body/limits.rscrates/rginx-http/src/proxy/request_body/mod.rscrates/rginx-http/src/proxy/request_body/model.rscrates/rginx-http/src/proxy/request_body/prepare.rscrates/rginx-http/src/proxy/request_body/replay.rscrates/rginx-http/src/proxy/request_body/streaming.rscrates/rginx-http/src/state/counters.rscrates/rginx-http/src/state/counters/grpc.rscrates/rginx-http/src/state/counters/http.rscrates/rginx-http/src/state/counters/rolling.rscrates/rginx-http/src/state/counters/traffic.rscrates/rginx-http/src/state/counters/upstreams.rscrates/rginx-http/src/state/counters/versions.rscrates/rginx-http/src/state/mod.rscrates/rginx-http/src/state/snapshots.rscrates/rginx-http/src/state/snapshots/active.rscrates/rginx-http/src/state/snapshots/delta.rscrates/rginx-http/src/state/snapshots/http.rscrates/rginx-http/src/state/snapshots/mod.rscrates/rginx-http/src/state/snapshots/reload.rscrates/rginx-http/src/state/snapshots/runtime.rscrates/rginx-http/src/state/snapshots/tls.rscrates/rginx-http/src/state/snapshots/traffic.rscrates/rginx-http/src/state/snapshots/upstreams.rscrates/rginx-runtime/src/admin/mod.rscrates/rginx-runtime/src/admin/model.rscrates/rginx-runtime/src/admin/service.rscrates/rginx-runtime/src/admin/socket.rscrates/rginx-runtime/src/bootstrap/listeners.rscrates/rginx-runtime/src/bootstrap/listeners/activate.rscrates/rginx-runtime/src/bootstrap/listeners/bind_tcp.rscrates/rginx-runtime/src/bootstrap/listeners/bind_udp.rscrates/rginx-runtime/src/bootstrap/listeners/drain.rscrates/rginx-runtime/src/bootstrap/listeners/group.rscrates/rginx-runtime/src/bootstrap/listeners/join.rscrates/rginx-runtime/src/bootstrap/listeners/mod.rscrates/rginx-runtime/src/bootstrap/listeners/prepare.rscrates/rginx-runtime/src/bootstrap/listeners/reconcile.rscrates/rginx-runtime/src/bootstrap/listeners/tests.rscrates/rginx-runtime/src/ocsp.rscrates/rginx-runtime/src/ocsp/client.rscrates/rginx-runtime/src/ocsp/mod.rscrates/rginx-runtime/src/ocsp/persist.rscrates/rginx-runtime/src/ocsp/refresh.rscrates/rginx-runtime/src/ocsp/scheduler.rscrates/rginx-runtime/src/ocsp/spec.rscrates/rginx-runtime/src/ocsp/state.rscrates/rginx-runtime/src/ocsp/tests.rsdocs/ARCHITECTURE_CODEBASE_MODULARIZATION_PLAN.mdscripts/modularization_baseline.json
💤 Files with no reviewable changes (7)
- crates/rginx-http/src/compression.rs
- crates/rginx-runtime/src/ocsp.rs
- crates/rginx-http/src/handler/grpc.rs
- crates/rginx-http/src/state/counters.rs
- crates/rginx-http/src/state/snapshots.rs
- crates/rginx-runtime/src/bootstrap/listeners.rs
- crates/rginx-http/src/handler/dispatch.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: TLS Gate
- GitHub Check: Test Slow
- GitHub Check: Agent
🔇 Additional comments (44)
scripts/modularization_baseline.json (1)
11-11: 基线文件验证通过，所有遗留豁免条目准确无误。五个 legacy_production_size_ceilings 条目已全部验证：
- 所有文件存在且行数与基线完全一致（certificate.rs 547、http3.rs 501/854、registry.rs 858、ocsp/mod.rs 827）
- 已模块化的 stage 4/5 目录确认存在（rginx-http 侧：compression、dispatch、grpc、forward、counters、snapshots；rginx-runtime 侧：admin、listeners、ocsp）
- rginx-http 的 OCSP 模块与 rginx-runtime 的 OCSP 模块确实路径不同，各自独立维护
基线无漂移风险，当前状态一致。
crates/rginx-http/src/state/snapshots/active.rs (1)
1-12: LGTM
ActiveState的拆分清晰，导入与字段语义一致。crates/rginx-runtime/src/ocsp/spec.rs (1)
1-8: LGTM薄包装函数对
rginx_http::tls_ocsp_refresh_specs_for_config的转发实现清晰，符合阶段 4 的拆分意图。crates/rginx-http/src/proxy/request_body/limits.rs (1)
1-15: LGTM错误源链遍历与下转换逻辑正确，
source()返回Option<&(dyn Error + 'static)>满足downcast_ref的'static约束。docs/ARCHITECTURE_CODEBASE_MODULARIZATION_PLAN.md (1)
183-211: LGTM阶段 4 和阶段 5 的“实际落地”小节与 PR 的实际拆分一一对应，完成时间标记一致。
Also applies to: 215-254
crates/rginx-http/src/proxy/forward/grpc.rs (1)
66-79: LGTM仅在 gRPC 请求路径计算绝对截止时间，并把超时消息生成委托到
error::grpc_timeout_message，与现有effective_upstream_request_timeout/parse_grpc_timeout的语义一致。crates/rginx-http/src/state/counters/versions.rs (1)
1-8: 无需修改。该文件通过include!("counters/versions.rs")方式纳入父模块crates/rginx-http/src/state/mod.rs，而非mod声明。include!()会在编译时将文件内容逐字展开到父模块中，因此父模块的导入声明对被包含文件可见。父模块已在第 4 行导入了AtomicU64，所以不存在编译错误。> Likely an incorrect or invalid review comment.crates/rginx-http/src/proxy/forward/success.rs (1)
15-59: LGTM升级配对逻辑合理:仅在 downstream 与 upstream 同时为 upgrade 时调用
hyper::upgrade::on(&mut response)抢占 upstream 升级、注册连接槽并 spawn 后台代理任务;非升级路径则保留active_peer守卫到下游响应。active_peer的所有权在两条分支中各自正确转交,不存在守卫提前释放的风险。crates/rginx-http/src/proxy/forward/types.rs (1)
3-17: LGTM将下游请求的可选项与上下文从
forward/mod.rs抽离为独立的 POD 结构,Copy 语义保证零开销传递,有助于模块边界清晰化。crates/rginx-http/src/compression/options.rs (1)
36-48: LGTM
min_bytes()与response_compression_disabled()的策略派生逻辑清晰:Force在未配置阈值时回退为 1(仍然尊重显式阈值),Auto/Off回退为 256;只要压缩或缓冲任一为Off即视为禁用,与RouteBufferingPolicy::Off通常意味着流式直传的语义一致。crates/rginx-runtime/src/bootstrap/listeners/bind_udp.rs (2)
51-78: LGTMsocket2 调用顺序正确:
SO_REUSEADDR→ 条件性SO_REUSEPORT(必须在bind之前)→bind→ 转换后再设set_nonblocking(true)。unsafe块通过socket.as_raw_fd()直接调用setsockopt的范围最小化,失败路径正确返回Error::Io(last_os_error)。
16-40: LGTM
normalize_inherited_udp_sockets的边界处理覆盖到位:1→N 主动报错(避免无SO_REUSEPORT的祖先 socket 在缩放后破坏负载分发),N→M 多余 socket 直接truncate释放 FD,N<M 时按reuse_port语义补齐,行为符合 HTTP/3 accept worker 重启场景的需求。crates/rginx-runtime/src/bootstrap/listeners/bind_tcp.rs (1)
5-9: LGTM简洁的 TCP 绑定辅助函数,统一通过
?透传rginx_core::Result,与bind_udp模块对称,适合放在 listeners 子模块中复用。crates/rginx-http/src/proxy/mod.rs (1)
34-34: 导入变更与模块拆分一致移除
RequestBodyLimitError后,该错误类型由proxy/request_body/limits.rs内部封装为request_body_limit_error(...),这里不再需要直接依赖。crates/rginx-runtime/src/ocsp/tests.rs (1)
8-37: LGTM测试用
OcspClient使用空RootCertStore加https_or_http(),因下方测试均通过http://协议访问本地 responder,不会触发 TLS 校验,可以接受。install_default_crypto_provider()在多次测试中重复调用是幂等的,符合 rustls 全局 provider 单次安装语义。crates/rginx-runtime/src/bootstrap/listeners/tests.rs (1)
55-68: 测试辅助函数实现合理
listener_group_with_socket通过Arc::new包装std_listener并初始化joined_tasks: 0与空std_udp_sockets，与重构后的ListenerWorkerGroup字段保持一致，便于在prepare_added_listener_bindings_*测试中复用。crates/rginx-http/src/handler/access_log.rs (1)
1-7: 导入收敛得当将原先的
use super::*;替换为显式导入ClientAddress、Version、AccessLogFormat、AccessLogValues，缩小命名空间且与dispatch/grpc分离后的模块边界一致，无逻辑变化。crates/rginx-runtime/src/bootstrap/listeners/mod.rs (1)
1-28: 模块拆分与ListenerGroupMap别名定义清晰子模块声明顺序与
pub(super) use重新导出范围合理；#[cfg(test)]仅暴露bind_std_listener等绑定辅助给测试树，避免污染生产命名空间。ListenerGroupMap作为HashMap<String, ListenerWorkerGroup>的别名，便于 reconcile/drain/join 子模块共用签名。crates/rginx-runtime/src/admin/service.rs (2)
98-175:dispatch_request抽离干净，关注点分离合理将
AdminRequest→AdminResponse的纯映射独立成dispatch_request，让handle_connection只负责 IO 与序列化，便于今后单独对 dispatch 做 unit test（无需构造 UnixStream）。Ok(match { ... })的写法在match各分支自身不需要返回Result时也可读，未来若有任何分支需要?，可调整为外层Ok(response)。
22-26:service::run的可见性收敛已确认安全变更已确认无风险：
service模块本身为私有（mod service无pub前缀），外部无法访问admin::service::runadmin/mod.rs通过同名公共函数pub async fn run作为对外接口，间接调用service::run- 无
pub use service再暴露，仅暴露 model 和 socket 子模块- 无测试、CLI 或其他外部代码直接调用
service::run改为
pub(super)是合理的可见性精化，无需修改。crates/rginx-http/src/handler/mod.rs (1)
45-46:tests子模块物理位置已确认存在已验证
crates/rginx-http/src/handler/tests.rs文件和crates/rginx-http/src/handler/tests/目录同时存在，#[cfg(test)] mod tests;的引用有效，cargo test -p rginx-http不会因此失败。crates/rginx-http/src/state/counters/grpc.rs (1)
1-16: 导入不缺失，无需修改该文件通过
include!()宏从state/mod.rs中被包含。state/mod.rs第 4 行已导入AtomicU64和Ordering，第 42 行已导出GrpcTrafficSnapshot。在include!()模式下，父模块的所有导入自动在被包含文件的作用域内可用，因此grpc.rs中无需重复声明这些导入。代码可以正常编译。> Likely an incorrect or invalid review comment.crates/rginx-http/src/state/counters/upstreams.rs (1)
1-87: 导入可通过include!()宏正确解析 — 无需修改
upstreams.rs通过include!()宏包含在mod.rs中，所有所需的类型都已在作用域内可用：
AtomicU64、Arc、HashMap、ConfigSnapshot直接在mod.rs中导入RecentUpstreamStatsCounters在rolling.rs中定义，而rolling.rs在upstreams.rs之前被包含（行 53 vs 56）rginx_core::Upstream通过mod.rs中的use rginx_core导入可访问不存在编译失败的风险。
crates/rginx-runtime/src/ocsp/mod.rs (1)
3-8: 这些导入不是未使用的——sibling 模块已通过super::正确引用scheduler.rs、refresh.rs 和 tests.rs 都通过
use super::{ OcspClient, build_ocsp_client, ... }来引用这些导入。mod.rs 中的这些 use 语句充当了为 sibling 模块提供访问路径的作用，这是 Rust 中的标准模块重新导出模式。> Likely an incorrect or invalid review comment.crates/rginx-http/src/compression/content_type.rs (1)
6-30: 响应压缩资格判断逻辑清晰、覆盖了关键反例。对 1xx / 204 / 304 / 206 /
Content-Range/ 已含Content-Encoding等情况的过滤是恰当的；对Content-Type不可解析为 UTF-8 时直接拒绝压缩也避免了在错误编码上做 MIME 匹配的隐患。crates/rginx-runtime/src/bootstrap/listeners/drain.rs (1)
1-17: 小巧、职责单一的拆分，无需改动。crates/rginx-http/src/compression/encode.rs (1)
16-24: Brotli 流终结行为已确认——Drop 会自动执行 FINISH。
CompressorWriter的 Drop 实现确实会自动调用BROTLI_OPERATION_FINISH，因此第 22 行块作用域结束时流会被正确终结，代码模式是安全的。但需注意：Drop 期间发生的任何错误会被静默忽略（不会 panic），这意味着如果终结失败，错误信息丢失。如果需要显式处理终结错误，建议改用into_inner()替代隐式 Drop，以便捕获并处理潜在的压缩流关闭失败。crates/rginx-http/src/state/counters/traffic.rs (1)
141-197: 不存在 vhost_order / route_order 重复条目的风险。vhost ID 由编译阶段的结构性保证决定：
default_vhost.id固定为常数"server"config.vhosts[i].id固定为format!("servers[{i}]")这两种格式不可能相同，因此不存在配置错误导致重复的场景。Route ID 继承父 vhost ID 前缀（
"{vhost_id}/routes[{index}]|..."），自动保证全局唯一。虽然代码中使用了
vhost_orderVec 和vhost_idHashMap，但由于 ID 绝不会重复，HashMap 覆盖和 Vec 重复条目的问题不会发生。lifecycle.rs 中使用 BTreeSet 仅用于高效比较配置变更，不是为了修复重复 ID 问题。> Likely an incorrect or invalid review comment.crates/rginx-runtime/src/bootstrap/listeners/activate.rs (1)
27-31: 配置验证阶段已确保accept_workers >= 1，因此该边界情况在实践中不可能发生。
validate_runtime()中的验证会拒绝显式设置为 0 的accept_workers，而compile_runtime_accept_workers()将None默认为 1。这意味着最终的RuntimeSettings.accept_workers总是至少为 1。在prepare_listener_worker_group()中，worker_listeners通过循环0..accept_workers创建，所以worker_listeners.len()总是至少为 1。因此，remaining_workers永远不会从 0 开始。crates/rginx-runtime/src/ocsp/state.rs (1)
1-23: LGTM!三个
pub(super)辅助函数仅做轻量包装与条件分支：成功/失败计数透传给SharedState，refresh_tls_acceptors_if_needed在tls_acceptors_changed为 false 时短路返回Ok(())，否则将底层错误转换为带上下文的String。逻辑清晰，无并发或正确性隐患。crates/rginx-http/src/handler/dispatch/authorize.rs (1)
1-61: LGTM!两个 gatekeeping 助手职责单一、命名清晰:
authorize_route处理 ACL 拒绝,enforce_rate_limit处理速率限制拒绝。被拒绝路径都通过grpc_error_response(...).unwrap_or_else(...)兜底为对应的 HTTP 响应,日志字段(client_ip/peer_addr/route)便于排障。无明显问题。crates/rginx-http/src/proxy/forward/streaming.rs (1)
1-81: LGTM!错误分类清晰:超限 → 413、上游响应后 downstream body 失效 → 400、其他 → 502,且每个分支都对应记录了 metric (
record_upstream_payload_too_large_response/record_upstream_bad_request_response/record_upstream_bad_gateway_response),日志字段(request_id、upstream、peer、logical_peer)便于排障。oneshotRecvError(中继任务异常退出)兜底为 502 也合理。crates/rginx-http/src/proxy/request_body/replay.rs (2)
37-46: LGTM!
is_end_stream和size_hint的实现与poll_frame的状态机一致(空 body / 空 trailers 时即视为流结束;size_hint对未发送的 body 给出精确长度),is_idempotent_method与 RFC 7231 中的幂等方法集合一致(GET/HEAD/PUT/DELETE/OPTIONS/TRACE,排除 POST/PATCH/CONNECT)。can_retry_peer_request同时校验can_failover()与剩余 peer 数,逻辑正确。
24-35: if-let 链式语法完全兼容。项目 rust-version 为 1.94.1，edition 为 2024，均完全支持 if-let 链式语法。代码无问题。
crates/rginx-http/src/state/snapshots/reload.rs (2)
9-24: LGTM!
ReloadResultSnapshot与ReloadStatusSnapshot字段全部pub,clone 友好且已派生PartialEq/Eq,ReloadStatusSnapshot::default()可直接用作初始零值。结构与state/counters/http.rs::ReloadHistory::snapshot()的产出一致。
3-7: 为ReloadOutcomeSnapshot添加#[serde(rename_all = "snake_case")]以保持一致性。同目录下的
SnapshotModule(delta.rs) 已使用#[serde(rename_all = "snake_case")],但ReloadOutcomeSnapshot缺少此属性。当前该枚举将序列化为外部标签格式且保留原始大小写:
{"Success":{"revision":42}}{"Failure":{"error":"..."}}应添加属性改为 snake_case 风格,与模块内其他枚举保持一致:
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "snake_case")] pub enum ReloadOutcomeSnapshot { Success { revision: u64 }, Failure { error: String }, }crates/rginx-http/src/proxy/forward/mod.rs (1)
7-31: 模块拆分清晰，无明显问题
forward子模块按照attempt / setup / streaming / success / types / grpc / response / error的职责拆分合理，对外仅 re-exportforward_request和两个上下文结构体，内部细节通过pub(super)隔离。crates/rginx-runtime/src/admin/socket.rs (1)
42-57:AdminSocketGuard的 dev/ino 校验做得很到位通过保存绑定时刻的
(device, inode)，在 Drop 时只删除仍然指向同一文件的路径，避免在管理员替换或符号链接更换后误删新的 socket，是一个值得保留的保护性细节。crates/rginx-http/src/handler/grpc/error.rs (1)
85-102: [Rewritten review comment]
[Exactly ONE classification tag]crates/rginx-http/src/state/snapshots/upstreams.rs (1)
1-61: LGTM。字段命名一致（
*_total累积计数 +recent_60s/recent_window双窗口），Eq + PartialEq派生在所有u64/String/bool字段上都安全可用，recent_window的skip_serializing_if处理也合理。crates/rginx-runtime/src/admin/model.rs (1)
1-65: LGTM。请求/响应类型与
AdminSnapshot的 serde 配置一致，skip_serializing_if = "Option::is_none"在所有可选字段上都已标注，type/data标签格式适合通过 Unix socket 进行 JSON IPC。关于
WaitForSnapshotChange请求的处理已确认：在service.rs::dispatch_request中，该请求通过复用AdminResponse::SnapshotVersion变体返回响应，超时参数正确转换为Duration。这是合理的设计选择。crates/rginx-http/src/handler/grpc/grpc_web.rs (2)
234-234: MSRV 完全支持，无需修改。
is_multiple_of在 Rust 1.87.0（2025-05-15）稳定化。项目的rust-version = "1.94.1"已远高于 1.87.0，该方法完全可用。不需要改为% 4 != 0。> Likely an incorrect or invalid review comment.
20-26:for_protocol的 case 敏感性不构成问题。
grpc_protocol()函数在 metadata.rs 第 24 行已通过mime.to_ascii_lowercase()明确规范化了传入的 Content-Type 头，确保即使客户端发送"Application/GRPC-Web"或"application/GRPC-WEB"等混合大小写的值，也会被正规化为小写，再与小写模式匹配，最终返回小写的协议标识符（"grpc-web"、"grpc-web-text"等）。因此for_protocol()接收的协议字符串总是小写的，不存在观测能力被静默关闭的风险。crates/rginx-http/src/handler/dispatch/response.rs (1)
38-48: HEAD 响应处理已正确同步Content-Length；Server头覆盖是反向代理的预期行为。
TRANSFER_ENCODING已在上游处理：在proxy/forward/response.rs中的build_downstream_response函数调用sanitize_response_headers时（第 26 行），会通过remove_hop_by_hop_headers删除所有 hop-by-hop 头（包括TRANSFER_ENCODING），然后才将响应传递给finalize_downstream_response。因此到达该函数时，TRANSFER_ENCODING已被规范化处理，不存在冲突风险。
Content-Length保留无误：测试用例finalize_downstream_response_strips_head_body_after_final_transforms验证了 HEAD 响应会保留原始Content-Length头（第 160 行注释明确指出"HEAD response should preserve content length"），同时体为空（第 171 行）。这符合 HTTP 规范。
Server头的无条件覆盖（第 47 行）是反向代理的预期设计：代理声称自身为响应源。如需条件性保留上游 Server 头，应在配置层或调用层显式控制该行为。
vansour
dismissed stale reviews from coderabbitai[bot] and coderabbitai[bot]
Addressed in 861ca86; follow-up CI passed and CodeRabbit rerun was rate-limited.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 861ca865d9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This was referenced Apr 27, 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
rginx-runtimelistener/bootstrap, OCSP, and admin runtime code into directory modulesrginx-httpcompression, handler dispatch/gRPC, proxy forwarding/request-body, and state snapshot/counter modulesTesting