Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 selected for processing (79)
📝 WalkthroughWalkthrough为 rginx 引入可配置的反向代理响应缓存：包含配置模型、验证与编译、核心缓存类型与键模板、磁盘存储/索引加载、CacheManager 查找与存储、与代理转发流水线的集成及大量测试覆盖。 Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Dispatcher as Route Dispatcher
participant CacheManager as Cache Manager
participant Disk as Cache Storage
participant Upstream as Upstream Server
Client->>Dispatcher: HTTP Request (GET)
Dispatcher->>CacheManager: lookup(request, scheme, policy)
alt Cache Hit
CacheManager->>Disk: read metadata & body
Disk-->>CacheManager: cached response
CacheManager-->>Dispatcher: Hit(response)
Dispatcher-->>Client: 200 OK (x-cache: HIT)
else Cache Miss/Bypass
CacheManager-->>Dispatcher: Miss(store_ctx) / Bypass(status)
Dispatcher->>Upstream: forward request
Upstream-->>Dispatcher: upstream response
alt Cacheable and store_ctx present
Dispatcher->>CacheManager: store_response(store_ctx, response)
CacheManager->>Disk: write metadata + body (atomic)
Disk-->>CacheManager: write result
CacheManager-->>Dispatcher: stored / error
Dispatcher-->>Client: 200 OK (x-cache: MISS)
else Not storable or bypass
Dispatcher-->>Client: response (x-cache: BYPASS or MISS)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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: e7555133a4
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
This PR introduces an MVP route-level HTTP response cache: new cache zone + route cache policy configuration flows through config load/validate/compile into runtime, and the proxy pipeline is updated to do cache lookup/store with X-Cache markers.
Changes:
- Add cache zone + route cache policy models (core/config), plus config parsing/validation/compilation and
rginx checksummary output. - Implement an MVP response cache in
rginx-http(disk metadata/body storage + in-memory index) and wire it into the proxy forwarding flow (HIT/MISS/BYPASS/EXPIRED). - Add tests covering config validation/compile and proxy/cache behavior, plus a cache support phase plan doc.
Reviewed changes
Copilot reviewed 71 out of 71 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/rginx-runtime/src/health/tests.rs | Update snapshot construction for new cache_zones field. |
| crates/rginx-runtime/src/bootstrap/shutdown/tests.rs | Update snapshot/route construction for new cache fields. |
| crates/rginx-runtime/src/bootstrap/listeners/tests.rs | Update snapshot construction for new cache_zones field. |
| crates/rginx-http/src/transition/tests.rs | Update snapshot construction for new cache_zones field. |
| crates/rginx-http/src/state/tls_runtime/bindings/tests.rs | Update snapshot construction for new cache_zones field. |
| crates/rginx-http/src/state/tests/support.rs | Update test snapshots/routes for new cache fields. |
| crates/rginx-http/src/state/mod.rs | Add cache manager to shared state + module wiring. |
| crates/rginx-http/src/state/lifecycle/reload.rs | Reload path updates cache manager on config reload. |
| crates/rginx-http/src/state/helpers.rs | Build cache manager during state preparation. |
| crates/rginx-http/src/state/cache.rs | SharedState cache lookup/store helpers. |
| crates/rginx-http/src/router/tests.rs | Update route test fixtures for new Route.cache field. |
| crates/rginx-http/src/proxy/tests/mod.rs | Add proxy cache test module + snapshot updates. |
| crates/rginx-http/src/proxy/tests/cache.rs | Integration-style tests for MISS→HIT and BYPASS markers. |
| crates/rginx-http/src/proxy/health/registry/tests.rs | Update snapshot construction for new cache_zones field. |
| crates/rginx-http/src/proxy/forward/types.rs | Add downstream cache policy to request context/options. |
| crates/rginx-http/src/proxy/forward/success.rs | Apply cache store / cache status marking on upstream success. |
| crates/rginx-http/src/proxy/forward/streaming.rs | Signature update to pass downstream context by reference. |
| crates/rginx-http/src/proxy/forward/setup.rs | Signature update to pass downstream context by reference. |
| crates/rginx-http/src/proxy/forward/attempt.rs | Wire cache lookup before upstream attempt + cache store/status on outcomes. |
| crates/rginx-http/src/proxy/clients/tests.rs | Update snapshot construction for new cache_zones field. |
| crates/rginx-http/src/lib.rs | Expose new cache module within crate. |
| crates/rginx-http/src/handler/tests/support.rs | Update test config/route helpers for cache fields. |
| crates/rginx-http/src/handler/tests/routing/selection.rs | Update routing tests for new Route.cache field. |
| crates/rginx-http/src/handler/tests/routing/handle.rs | Update routing tests for new Route.cache field. |
| crates/rginx-http/src/handler/tests/routing/authorize.rs | Update routing tests for new Route.cache field. |
| crates/rginx-http/src/handler/dispatch/route.rs | Pass route cache policy into downstream request options. |
| crates/rginx-http/src/handler/dispatch/mod.rs | Carry route cache policy in execution context. |
| crates/rginx-http/src/cache/tests.rs | Unit tests for cache manager/key/TTL/storability behavior. |
| crates/rginx-http/src/cache/mod.rs | New cache implementation (disk entries + in-memory index + policy). |
| crates/rginx-http/Cargo.toml | Add serde_json dependency for cache metadata encoding. |
| crates/rginx-core/src/lib.rs | Re-export cache types (zone/policy/key template). |
| crates/rginx-core/src/config/tests.rs | Update config tests for new snapshot/route cache fields. |
| crates/rginx-core/src/config/snapshot.rs | Add cache_zones to runtime snapshot. |
| crates/rginx-core/src/config/route.rs | Add Route.cache policy field. |
| crates/rginx-core/src/config/cache.rs | New core cache model + cache key template parsing/rendering. |
| crates/rginx-core/src/config.rs | Wire mod cache and re-export cache types. |
| crates/rginx-config/src/validate/vhost.rs | Thread cache zone names through vhost validation. |
| crates/rginx-config/src/validate/tests/vhosts.rs | Update vhost validation test fixtures for new route cache field. |
| crates/rginx-config/src/validate/tests/route.rs | Update route validation test fixtures for new route cache field. |
| crates/rginx-config/src/validate/tests/cache.rs | New validation tests for cache zones and route cache policy constraints. |
| crates/rginx-config/src/validate/tests.rs | Add cache test module + base config now includes cache fields. |
| crates/rginx-config/src/validate/route.rs | Validate cache handler compatibility + route cache references. |
| crates/rginx-config/src/validate/cache.rs | New validators for cache zone definitions and route cache policy. |
| crates/rginx-config/src/validate.rs | Validate cache zones and pass cache zone name set into location/vhost validation. |
| crates/rginx-config/src/model/route.rs | Add LocationConfig.cache to config model. |
| crates/rginx-config/src/model/cache.rs | New config model structs for cache zone + route cache policy. |
| crates/rginx-config/src/model.rs | Add top-level cache_zones to config and re-export cache config types. |
| crates/rginx-config/src/load/tests.rs | Ensure config loader deserializes cache zone + route cache policy. |
| crates/rginx-config/src/compile/tests/vhosts/nezha.rs | Update compile test fixtures for new cache fields. |
| crates/rginx-config/src/compile/tests/vhosts/listener_conflicts.rs | Update compile test fixtures for new cache fields. |
| crates/rginx-config/src/compile/tests/vhosts.rs | Update compile test fixtures for new cache fields. |
| crates/rginx-config/src/compile/tests/upstream_transport.rs | Update compile test fixtures for new cache fields. |
| crates/rginx-config/src/compile/tests/upstream_tls.rs | Update compile test fixtures for new cache fields. |
| crates/rginx-config/src/compile/tests/upstream_server_name.rs | Update compile test fixtures for new cache fields. |
| crates/rginx-config/src/compile/tests/upstream_fallbacks.rs | Update compile test fixtures for new cache fields. |
| crates/rginx-config/src/compile/tests/upstream_defaults.rs | Update compile test fixtures for new cache fields. |
| crates/rginx-config/src/compile/tests/server_tls.rs | Update compile test fixtures for new cache fields. |
| crates/rginx-config/src/compile/tests/server_settings.rs | Update compile test fixtures for new cache fields. |
| crates/rginx-config/src/compile/tests/route/regex.rs | Update compile test fixtures for new cache fields. |
| crates/rginx-config/src/compile/tests/route.rs | Update compile test fixtures for new cache fields. |
| crates/rginx-config/src/compile/tests/listeners.rs | Update compile test fixtures for new cache fields. |
| crates/rginx-config/src/compile/tests/http3.rs | Update compile test fixtures for new cache fields. |
| crates/rginx-config/src/compile/tests/cache.rs | New compile tests verifying cache zones + route policy compilation and defaults. |
| crates/rginx-config/src/compile/tests.rs | Add cache module + update common test location fixture. |
| crates/rginx-config/src/compile/route.rs | Compile LocationConfig.cache into Route.cache. |
| crates/rginx-config/src/compile/mod.rs | Compile cache zones into snapshot and wire new compile module. |
| crates/rginx-config/src/compile/cache.rs | New compilation logic for cache zones and route cache policies. |
| crates/rginx-app/src/check/summary.rs | Include cache zone + cache-enabled route counts in rginx check summary. |
| crates/rginx-app/src/check/routes.rs | Track cache-enabled routes in route transport detail counts. |
| crates/rginx-app/src/check/render.rs | Render cache zone details + cache metrics in check output. |
| CACHE_SUPPORT_PLAN.md | Add cache support phased plan + status documentation. |
💡 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: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
crates/rginx-http/src/proxy/tests/mod.rs (1)
197-213: 🧹 Nitpick | 🔵 Trivial可使用已导入的
HashMap。文件顶部第 3 行已
use std::collections::{HashMap, VecDeque};，第 202 行可直接写成cache_zones: HashMap::new()，与同函数第 211 行已使用的upstreams（HashMap）保持风格一致。无功能影响。♻️ 建议的小幅修改
- cache_zones: std::collections::HashMap::new(), + cache_zones: HashMap::new(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/proxy/tests/mod.rs` around lines 197 - 213, The cache_zones field in snapshot_with_upstreams_map is constructed with a fully-qualified std::collections::HashMap::new(); change it to use the already-imported HashMap by replacing that expression with HashMap::new() so it matches the style used for the upstreams parameter and other code; update the initializer in function snapshot_with_upstreams_map (field cache_zones) accordingly.crates/rginx-http/src/handler/dispatch/mod.rs (1)
47-53: 🧹 Nitpick | 🔵 Trivial建议考虑用
Arc包装RouteCachePolicy以避免每请求克隆开销。
RouteExecutionContext在每个进入路由的请求上都会构造一次，route.cache.clone()也会随之执行（第 170 行）。RouteCachePolicy内部包含zone: String、methods: Vec<Method>、statuses: Vec<StatusCode>和key: CacheKeyTemplate（后者又包含raw: String和parts: Vec<CacheKeyTemplatePart>）。这些都是堆分配类型，每次克隆都会产生新的内存分配。建议将
Route.cache定义为Option<Arc<RouteCachePolicy>>，或在RouteExecutionContext中持有Option<Arc<RouteCachePolicy>>，使每请求路径仅做引用计数克隆，避免不必要的堆分配。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/handler/dispatch/mod.rs` around lines 47 - 53, RouteExecutionContext currently clones RouteCachePolicy per request (via route.cache.clone()), causing heavy heap allocations; change the type of the cache to use reference-counting instead: make Route.cache (or the cache field inside RouteExecutionContext) Option<Arc<RouteCachePolicy>> and update all call sites to clone the Arc (Arc::clone) instead of deep-cloning RouteCachePolicy; ensure you import std::sync::Arc and update any code that previously accessed fields or constructed RouteCachePolicy to account for the Arc (dereference or clone the Arc when needed) and replace route.cache.clone() with Arc::clone(&route.cache.as_ref().unwrap()) or equivalent safe handling for the Option.crates/rginx-http/src/state/lifecycle/reload.rs (1)
117-128:⚠️ Potential issue | 🟡 Minor缓存管理器应在新配置对外可见前完成切换。
当前代码在 line 124 释放
self.inner.write()临界区后，新的revision/config/clients立即对其他读者可见，但self.cache直到 line 126 才被更新。这个窗口内的请求会用新配置中的路由 cache 策略去查询旧的CacheManager。若新配置新增或重命名了 cache zone，旧
CacheManager.zones中不存在该名称，lookup()会直接返回CacheLookup::Bypass（见crates/rginx-http/src/cache/mod.rsline 143-144）。反向场景（删除 zone）也会造成策略与存储不一致。虽然窗口很短，但这是可避免的瑕疵——只需将 cache 的切换提前到
state.config = prepared.config.clone()之前，或至少在同一self.inner.write()临界区内完成。建议调整为：先更新self.cache，再更新self.inner中的配置，确保新配置对外暴露时缓存管理器已就位。🔧 建议的顺序调整
+ // Swap cache before exposing the new config so route policies and + // the cache manager stay consistent for any concurrent request. + *self.cache.write().unwrap_or_else(|poisoned| poisoned.into_inner()) = prepared.cache; + let next_revision = { let mut state = self.inner.write().await; let next_revision = state.revision + 1; state.revision = next_revision; state.config = prepared.config.clone(); state.clients = prepared.clients; next_revision }; - - *self.cache.write().unwrap_or_else(|poisoned| poisoned.into_inner()) = prepared.cache; *self.listener_tls_acceptors.write().await = merged_acceptors;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/state/lifecycle/reload.rs` around lines 117 - 128, The update sequence exposes new state.config/clients before swapping the cache; move the cache swap so the CacheManager is updated before the new config becomes visible: write prepared.cache into self.cache (the prepared.cache assignment currently following the inner.write() block) either before modifying state inside the same self.inner.write().await critical section or immediately before entering it; then update state.revision, state.config, and state.clients inside that same write lock, keep the listener acceptors assignment (*self.listener_tls_acceptors.write().await = merged_acceptors) and the revisions send (self.revisions.send(next_revision)) as-is after state and cache are swapped so readers never observe a config that references an out-of-date CacheManager.crates/rginx-config/src/model.rs (1)
30-45: 🧹 Nitpick | 🔵 Trivial建议：将
pub use cache::{...}上移以保持与其他模块导出的一致性。文件中所有其他子模块的
pub use声明（listener、route、runtime、server、tls、upstream、vhost）都集中放置在Config结构体定义之前。本次新增的pub use cache::{CacheRouteConfig, CacheZoneConfig};被放在了第 45 行（Config定义之后），破坏了文件原有的“先 use、再 struct 定义”的组织约定，建议挪到与其他pub use同一区域。♻️ 建议的调整
pub use vhost::VirtualHostConfig; +pub use cache::{CacheRouteConfig, CacheZoneConfig}; #[derive(Debug, Clone, Deserialize)] pub struct Config { @@ #[serde(default)] pub servers: Vec<VirtualHostConfig>, } -pub use cache::{CacheRouteConfig, CacheZoneConfig};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-config/src/model.rs` around lines 30 - 45, Move the pub re-export for the cache types so it sits with the other module exports before the Config struct: relocate the line `pub use cache::{CacheRouteConfig, CacheZoneConfig};` to the block where the other `pub use` statements for `listener`, `route`, `runtime`, `server`, `tls`, `upstream`, and `vhost` are declared so that all module exports appear prior to the `Config` struct definition.crates/rginx-http/src/proxy/forward/attempt.rs (1)
1-346: 🛠️ Refactor suggestion | 🟠 Major生产代码软上限超限，CI 已失败（modularization-gate）。
CI 报告
attempt.rs共 346 行，超出 300 行软上限。建议把缓存装饰逻辑（mark_cache_status_if_needed及缓存上下文构造）抽到独立子模块，例如proxy/forward/cache.rs，可以同时降低本文件复杂度并把第 12–31 行与第 336–346 行集中管理。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/proxy/forward/attempt.rs` around lines 1 - 346, The file is over the 300-line soft limit; extract the cache decoration logic into a new submodule (e.g., proxy::forward::cache): move the helper fn mark_cache_status_if_needed and the cache lookup/context construction around the top of forward_request (the logic that creates cache_lookup, cache_store, cache_status and the final call at the end) into that module (export a function like cache::prepare_cache_context(request: &Request<HttpBody>, state: &SharedState, downstream: &DownstreamRequestContext<'_>, cache_opt: Option<...>) -> (Option<crate::cache::CacheStore>, Option<crate::cache::CacheStatus>) and cache::wrap_response_with_status(state: &SharedState, response: HttpResponse, cache_status: Option<crate::cache::CacheStatus>) -> HttpResponse), update attempt.rs to call those two functions instead of inlining the logic, adjust imports/uses (e.g., replace direct mark_cache_status_if_needed and the cache construction with calls to cache::prepare_cache_context and cache::wrap_response_with_status) and run cargo build/tests to ensure visibility/paths and types (SharedState, HttpResponse, crate::cache::CacheLookup/CacheStatus) are correct.crates/rginx-http/src/proxy/forward/success.rs (1)
29-57:⚠️ Potential issue | 🟡 Minor在 upgrade 路径上仍调用
apply_cache_store，建议显式跳过缓存写入。WebSocket / HTTP/1.1 upgrade 响应（通常是
101 Switching Protocols）的语义不应进入缓存流程。当前实现依赖response_is_storable通过状态码白名单过滤掉101，但仍会进入store_cached_response→store_response，浪费一次锁与状态判断；更重要的是，一旦未来将101加入用户可配置statuses，就会错误地尝试缓存一个已经被劫持成 raw I/O 的连接体。建议在此分支上仅应用cache_status、丢弃cache_store：♻️ 建议
- return apply_cache_store( - context.state, - context.cache_store, - context.cache_status, - response, - ) - .await; + // Upgraded connections must never be persisted to cache. + return apply_cache_store(context.state, None, context.cache_status, response).await;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/proxy/forward/success.rs` around lines 29 - 57, This branch handles upgraded (101/connection-upgrade) responses; instead of calling apply_cache_store (which may acquire locks and attempt store), return the built response after only applying the cache_status and skipping any cache_store logic: in the upgrade branch where (context.downstream_upgrade, upstream_upgrade) are present, call a helper that applies just the cache_status (e.g., apply_cache_status(state, cache_status, response)) or, if that helper doesn't exist, modify apply_cache_store usage to pass a sentinel/no-op for cache_store and ensure no store_cached_response/store_response path is executed; update the branch around proxy_upgraded_connection, build_downstream_response and the apply_cache_store call accordingly so upgrades never attempt to write cached bodies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/rginx-app/src/check/render.rs`:
- Around line 57-67: The loop printing cache zone details should be extracted
out of print_route_transport_details into a new function
print_cache_zone_details that takes the cache zone slice (or
&summary.cache_zones) and prints each zone exactly as currently done; update
print_route_transport_details to call print_cache_zone_details (or have
print_check_success call it directly) so responsibilities match names, and
ensure print_check_success invokes print_cache_zone_details after route
transport output to allow future extensions (e.g., health/capacity stats).
In `@crates/rginx-app/src/check/routes.rs`:
- Line 15: The count of cache-enabled routes is duplicated:
RouteTransportCheckDetails::cache_enabled_routes already computes it, but
CheckSummary::cache_enabled_route_count recomputes the same iteration; remove
the duplicate by deleting the cache_enabled_route_count() function/logic and
make CheckSummary use the precomputed value from
route_transport.cache_enabled_routes when constructing the summary (i.e.,
replace any recomputation with a direct reference to
route_transport.cache_enabled_routes so render/output code continues to read the
same field).
In `@crates/rginx-config/src/compile/cache.rs`:
- Around line 50-72: The parsed methods and statuses vectors (constructed from
cache.methods and cache.statuses and stored on RouteCachePolicy.methods) can
contain duplicates (e.g., "GET","get"," GET ") after parsing; after
mapping/parsing but before assigning, deduplicate the collections—either by
collecting into a HashSet then back to a Vec or by normalizing, sorting (or
using a stable dedupe) and calling dedup()—so RouteCachePolicy.methods and the
statuses Vec contain unique entries; ensure you perform this dedupe in the same
block that builds methods and statuses so response_is_storable and
cache_request_bypass still work but without redundant comparisons or duplicate
values in metrics/logs.
In `@crates/rginx-config/src/compile/tests/cache.rs`:
- Around line 122-143: Add assertions to the test that validate the default
fallback for the cache key template and stale_if_error behavior: after obtaining
policy from snapshot.default_vhost.routes[0].cache (via compile_with_base),
assert that policy.key equals DEFAULT_CACHE_KEY (the expected
"{scheme}:{host}:{uri}" template) when key was None, and assert that
policy.stale_if_error (or policy.stale_if_error_secs if that is the field name
used in compile_route_cache) is None when stale_if_error_secs was None;
reference the DEFAULT_CACHE_KEY constant and the policy.stale_if_error field to
locate the correct symbols to assert against.
In `@crates/rginx-config/src/load/tests.rs`:
- Around line 10-27: Update the test function
load_from_str_deserializes_cache_zones_and_route_cache_policy to add assertions
for the missing fields: for the cache zone (config.cache_zones[0]) assert
inactive_secs == Some(600) and default_ttl_secs == Some(60), and for the route
cache (the variable cache from config.locations[0].cache) assert key.as_deref()
== Some("{scheme}:{host}:{uri}"); locate these checks near the existing
assertions that verify name and max_entry_bytes so the test validates
CacheZoneConfig and CacheRouteConfig fully.
In `@crates/rginx-config/src/validate/cache.rs`:
- Around line 50-55: The error message echoes the untrimmed cache.zone which can
mislead users; change the code around the cache_zone_names.contains(zone) check
so the error uses the trimmed value (the same variable used for lookup) instead
of cache.zone, e.g. compute let trimmed = cache.zone.trim(); use
cache_zone_names.contains(trimmed) and when returning Err(Error::Config(...))
include the trimmed value or an explicit note about surrounding whitespace;
update the Error::Config call that currently formats "{route_scope} references
undefined cache zone `{}`" to reference the trimmed name (or mention "contains
leading/trailing whitespace") so lookup and diagnostics are consistent.
In `@crates/rginx-config/src/validate/tests/cache.rs`:
- Around line 57-78: The test function
validate_rejects_unsupported_cache_method_and_invalid_zone_limits mixes two
independent failure scenarios; split it into two separate #[test] functions: one
(e.g., validate_rejects_max_entry_bytes_exceeding_max_size_bytes) that builds
config with CacheZoneConfig { max_entry_bytes: Some(2*1024), max_size_bytes:
Some(1024), ..cache_zone("default") } and asserts validate(&config).expect_err()
contains "max_entry_bytes must not exceed max_size_bytes", and another (e.g.,
validate_rejects_unsupported_cache_method) that builds config with
cache_zone("default"), sets policy = route_cache("default") with policy.methods
= Some(vec!["POST".to_string()]) and asserts validate(&config).expect_err()
contains "cache method `POST` is not supported"; keep using validate(),
cache_zone(), route_cache(), and the same assertion strings.
In `@crates/rginx-http/src/cache/mod.rs`:
- Around line 280-307: The store_response function currently calls
body.collect().await unconditionally for responses lacking a reliable
Content-Length/size_hint, causing OOM for large chunked bodies; update
store_response to avoid full buffering: when response_is_storable cannot prove
the size, either treat the response as non-cacheable (return the response
without collecting) or perform an incremental read of the body (read chunks,
accumulate length, and as soon as accumulated_len >
context.zone.config.max_entry_bytes stop buffering any further bytes and mark as
non-cacheable while continuing to stream the chunks downstream). Locate
store_response and the earlier response_is_storable logic to implement the
chosen approach, ensuring you reference context.zone.config.max_entry_bytes and
response_ttl/parts when deciding to skip caching.
- Around line 463-467: The current logic makes expires_ttl() return
Some(Duration::ZERO) for past Expires headers which then wins over
cache_control_max_age() and prevents fallback to default_ttl; document this
intentional behavior and its operational impact by adding a clear comment in
mod.rs near expires_ttl() and in the routing/cache docs explaining that an
expired Expires header yields TTL=0 (no caching) and can block default_ttl until
the header changes; also reference the existing behavior/test
response_ttl_respects_explicit_zero_or_expired_freshness and suggest a short
note for ops about the risk of stale upstream timestamps.
- Around line 112-135: CacheManager::from_config currently only creates
directories so after restart the in-memory CacheIndex is empty causing
persistent orphaned files and broken size accounting; fix by adding a startup
disk-scan step (either inside from_config or called right after construction)
that for each zone (CacheZoneRuntime) scans its path for matching *.meta.json
and body files, reconstructs entries into CacheIndex and restores
current_size_bytes, deletes orphaned metas/bodies, and triggers eviction if the
reconstructed current_size_bytes exceeds max_size_bytes; ensure this logic is
coordinated with existing lookup and write_cache_entry semantics so writes still
overwrite matching on-disk items and the reconstructed index prevents silent
leaks.
- Around line 348-360: The comparison in cache_request_bypass currently takes a
reference of the right operand (methods.iter().any(|method| method ==
&request.method)), triggering Clippy's op_ref; replace that check with a direct
contains call on the policy.methods Vec (e.g., use
policy.methods.contains(&request.method)) or otherwise compare by value (e.g.,
*method == request.method) inside the iterator; update the condition in the
cache_request_bypass function to use policy.methods.contains(&request.method) to
satisfy Clippy and simplify the code.
- Around line 436-461: In cache_control_max_age, don't let a single malformed
max-age token abort the whole function: replace the use of
value.trim().trim_matches('"').parse::<u64>().ok()? so that parse failures skip
that directive (continue) instead of returning None; locate this logic in
function cache_control_max_age (the loop over headers.get_all(CACHE_CONTROL) and
directive.split(',')) and change the parse to an explicit match/if let that
continues on Err, preserving other directives like s-maxage and allowing
fallback behavior.
- Around line 487-496: The cached_headers function is currently only filtering
CACHE_STATUS_HEADER and therefore persists RFC7230 hop-by-hop headers and
Content-Length; update cached_headers to drop all standard hop-by-hop names
(Connection, Keep-Alive, Proxy-Authenticate, Proxy-Authorization, TE, Trailer,
Transfer-Encoding, Upgrade) plus Content-Length, and additionally parse the
Connection header values and remove any header names it lists (case-insensitive)
before collecting. Ensure you still exclude CACHE_STATUS_HEADER and perform
comparisons case-insensitively when filtering header names so replayed responses
won't break framing or connection semantics.
- Around line 361-369: The content-type checks mix case-insensitive equality
with case-sensitive prefix checks, so values like "Application/grpc+proto" are
missed; in the request_is_grpc (the shown block) and response_is_grpc functions,
normalize the mime to a canonical case (e.g., lowercasing the trimmed mime
string) before comparing, then use starts_with("application/grpc+") and
starts_with("application/grpc-web") on that normalized value (or alternatively
use a case-insensitive starts_with helper), or extract a shared helper function
is_grpc_mime(mime: &str) and call it from both request_is_grpc and
response_is_grpc.
- Around line 532-548: write_cache_entry 存在并发写同一 key
时临时文件名冲突（metadata_tmp/body_tmp），会导致 metadata 与 body 对应错位；请将临时文件改为每次写入都带唯一后缀（如
UUID、PID+自增或使用 tempfile::NamedTempFile）而不是直接复用 paths.metadata/.body 派生名，确保在
write_cache_entry 中先写入各自唯一的临时文件（例如 body_tmp =
paths.body.with_extension(format!("body.{}.tmp", uuid))），然后只将各自的临时文件 rename
到最终文件（fs::rename(&own_body_tmp, &paths.body) / fs::rename(&own_metadata_tmp,
&paths.metadata)），并在错误分支清理/删除自身临时文件以避免残留。
In `@crates/rginx-http/src/cache/tests.rs`:
- Line 159: Replace the pattern
lock_index(&zone.index).entries.get(key).is_none() with the idiomatic
contains_key check to satisfy Clippy: use
!lock_index(&zone.index).entries.contains_key(key) instead; update the assertion
in the test that references lock_index, zone.index, entries and key to use
contains_key rather than get(...).is_none().
- Around line 211-221: The test hardcodes PathBuf::from("/tmp/rginx-cache-test")
in the CacheZone config which is platform-unsafe; change to create a temporary
directory with tempfile::tempdir(), use its path (tempdir.path().to_path_buf())
for the CacheZone::path, and keep the TempDir handle (e.g. bind tempdir to a
variable in the test scope) so it is not dropped while the test uses
CacheZoneRuntime (refer to CacheZoneRuntime, CacheZone and the local variable
zone). Ensure imports for tempfile are added and no hardcoded paths remain.
In `@crates/rginx-http/src/proxy/forward/attempt.rs`:
- Around line 23-31: The Hit branch in the match on cache_lookup returns the
cached Response immediately (CacheLookup::Hit(response)), skipping the same
downstream transformations applied on non-cache paths; modify the Hit handling
to pass the cached response through the same processing pipeline (i.e. call
build_downstream_response or at minimum invoke sanitize_response_headers and
wrap the body with IdleTimeoutBody, GrpcDeadlineBody and ActivePeerBody as done
for success paths) so cached responses receive header sanitization, idle timeout
protection, gRPC deadline enforcement and peer-active tracking before being
returned.
In `@crates/rginx-http/src/state/tls_runtime/bindings/tests.rs`:
- Line 101: The added field cache_zones: std::collections::HashMap::new() should
be simplified to use the existing import by replacing
std::collections::HashMap::new() with HashMap::new() to match upstreams:
HashMap::new() and keep style consistent; update the same occurrence mentioned
(also at the other spot) so both cache_zones initializers use HashMap::new() and
no further changes are required since HashMap is already imported at the top of
tests.rs.
In `@crates/rginx-http/src/transition/tests.rs`:
- Line 29: 在测试结构初始化处使用了完全路径 std::collections::HashMap::new()，但文件顶部已通过 use
std::collections::HashMap; 导入了 HashMap；请将字段初始化中的 cache_zones:
std::collections::HashMap::new(), 改为 cache_zones: HashMap::new(), 以与 upstreams:
HashMap::new() 保持一致风格并统一此 PR 中其他测试文件相同写法（定位符：cache_zones, upstreams, HashMap）。
In `@crates/rginx-runtime/src/bootstrap/shutdown/tests.rs`:
- Line 19: The test initializes ConfigSnapshot/Route fields correctly but for
style consistency use the imported HashMap alias: replace explicit
std::collections::HashMap::new() with HashMap::new() for the cache_zones field
(and any other occurrences such as at the other occurrence around the
vhosts/upstreams block) so the test's struct literal for ConfigSnapshot/Route
matches the surrounding style; locate the cache_zones field in the shutdown
tests and update its initializer accordingly.
---
Outside diff comments:
In `@crates/rginx-config/src/model.rs`:
- Around line 30-45: Move the pub re-export for the cache types so it sits with
the other module exports before the Config struct: relocate the line `pub use
cache::{CacheRouteConfig, CacheZoneConfig};` to the block where the other `pub
use` statements for `listener`, `route`, `runtime`, `server`, `tls`, `upstream`,
and `vhost` are declared so that all module exports appear prior to the `Config`
struct definition.
In `@crates/rginx-http/src/handler/dispatch/mod.rs`:
- Around line 47-53: RouteExecutionContext currently clones RouteCachePolicy per
request (via route.cache.clone()), causing heavy heap allocations; change the
type of the cache to use reference-counting instead: make Route.cache (or the
cache field inside RouteExecutionContext) Option<Arc<RouteCachePolicy>> and
update all call sites to clone the Arc (Arc::clone) instead of deep-cloning
RouteCachePolicy; ensure you import std::sync::Arc and update any code that
previously accessed fields or constructed RouteCachePolicy to account for the
Arc (dereference or clone the Arc when needed) and replace route.cache.clone()
with Arc::clone(&route.cache.as_ref().unwrap()) or equivalent safe handling for
the Option.
In `@crates/rginx-http/src/proxy/forward/attempt.rs`:
- Around line 1-346: The file is over the 300-line soft limit; extract the cache
decoration logic into a new submodule (e.g., proxy::forward::cache): move the
helper fn mark_cache_status_if_needed and the cache lookup/context construction
around the top of forward_request (the logic that creates cache_lookup,
cache_store, cache_status and the final call at the end) into that module
(export a function like cache::prepare_cache_context(request:
&Request<HttpBody>, state: &SharedState, downstream:
&DownstreamRequestContext<'_>, cache_opt: Option<...>) ->
(Option<crate::cache::CacheStore>, Option<crate::cache::CacheStatus>) and
cache::wrap_response_with_status(state: &SharedState, response: HttpResponse,
cache_status: Option<crate::cache::CacheStatus>) -> HttpResponse), update
attempt.rs to call those two functions instead of inlining the logic, adjust
imports/uses (e.g., replace direct mark_cache_status_if_needed and the cache
construction with calls to cache::prepare_cache_context and
cache::wrap_response_with_status) and run cargo build/tests to ensure
visibility/paths and types (SharedState, HttpResponse,
crate::cache::CacheLookup/CacheStatus) are correct.
In `@crates/rginx-http/src/proxy/forward/success.rs`:
- Around line 29-57: This branch handles upgraded (101/connection-upgrade)
responses; instead of calling apply_cache_store (which may acquire locks and
attempt store), return the built response after only applying the cache_status
and skipping any cache_store logic: in the upgrade branch where
(context.downstream_upgrade, upstream_upgrade) are present, call a helper that
applies just the cache_status (e.g., apply_cache_status(state, cache_status,
response)) or, if that helper doesn't exist, modify apply_cache_store usage to
pass a sentinel/no-op for cache_store and ensure no
store_cached_response/store_response path is executed; update the branch around
proxy_upgraded_connection, build_downstream_response and the apply_cache_store
call accordingly so upgrades never attempt to write cached bodies.
In `@crates/rginx-http/src/proxy/tests/mod.rs`:
- Around line 197-213: The cache_zones field in snapshot_with_upstreams_map is
constructed with a fully-qualified std::collections::HashMap::new(); change it
to use the already-imported HashMap by replacing that expression with
HashMap::new() so it matches the style used for the upstreams parameter and
other code; update the initializer in function snapshot_with_upstreams_map
(field cache_zones) accordingly.
In `@crates/rginx-http/src/state/lifecycle/reload.rs`:
- Around line 117-128: The update sequence exposes new state.config/clients
before swapping the cache; move the cache swap so the CacheManager is updated
before the new config becomes visible: write prepared.cache into self.cache (the
prepared.cache assignment currently following the inner.write() block) either
before modifying state inside the same self.inner.write().await critical section
or immediately before entering it; then update state.revision, state.config, and
state.clients inside that same write lock, keep the listener acceptors
assignment (*self.listener_tls_acceptors.write().await = merged_acceptors) and
the revisions send (self.revisions.send(next_revision)) as-is after state and
cache are swapped so readers never observe a config that references an
out-of-date CacheManager.
🪄 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: d55338bb-45c4-42c6-93d6-a9019b83ebba
📒 Files selected for processing (71)
CACHE_SUPPORT_PLAN.mdcrates/rginx-app/src/check/render.rscrates/rginx-app/src/check/routes.rscrates/rginx-app/src/check/summary.rscrates/rginx-config/src/compile/cache.rscrates/rginx-config/src/compile/mod.rscrates/rginx-config/src/compile/route.rscrates/rginx-config/src/compile/tests.rscrates/rginx-config/src/compile/tests/cache.rscrates/rginx-config/src/compile/tests/http3.rscrates/rginx-config/src/compile/tests/listeners.rscrates/rginx-config/src/compile/tests/route.rscrates/rginx-config/src/compile/tests/route/regex.rscrates/rginx-config/src/compile/tests/server_settings.rscrates/rginx-config/src/compile/tests/server_tls.rscrates/rginx-config/src/compile/tests/upstream_defaults.rscrates/rginx-config/src/compile/tests/upstream_fallbacks.rscrates/rginx-config/src/compile/tests/upstream_server_name.rscrates/rginx-config/src/compile/tests/upstream_tls.rscrates/rginx-config/src/compile/tests/upstream_transport.rscrates/rginx-config/src/compile/tests/vhosts.rscrates/rginx-config/src/compile/tests/vhosts/listener_conflicts.rscrates/rginx-config/src/compile/tests/vhosts/nezha.rscrates/rginx-config/src/load/tests.rscrates/rginx-config/src/model.rscrates/rginx-config/src/model/cache.rscrates/rginx-config/src/model/route.rscrates/rginx-config/src/validate.rscrates/rginx-config/src/validate/cache.rscrates/rginx-config/src/validate/route.rscrates/rginx-config/src/validate/tests.rscrates/rginx-config/src/validate/tests/cache.rscrates/rginx-config/src/validate/tests/route.rscrates/rginx-config/src/validate/tests/vhosts.rscrates/rginx-config/src/validate/vhost.rscrates/rginx-core/src/config.rscrates/rginx-core/src/config/cache.rscrates/rginx-core/src/config/route.rscrates/rginx-core/src/config/snapshot.rscrates/rginx-core/src/config/tests.rscrates/rginx-core/src/lib.rscrates/rginx-http/Cargo.tomlcrates/rginx-http/src/cache/mod.rscrates/rginx-http/src/cache/tests.rscrates/rginx-http/src/handler/dispatch/mod.rscrates/rginx-http/src/handler/dispatch/route.rscrates/rginx-http/src/handler/tests/routing/authorize.rscrates/rginx-http/src/handler/tests/routing/handle.rscrates/rginx-http/src/handler/tests/routing/selection.rscrates/rginx-http/src/handler/tests/support.rscrates/rginx-http/src/lib.rscrates/rginx-http/src/proxy/clients/tests.rscrates/rginx-http/src/proxy/forward/attempt.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/health/registry/tests.rscrates/rginx-http/src/proxy/tests/cache.rscrates/rginx-http/src/proxy/tests/mod.rscrates/rginx-http/src/router/tests.rscrates/rginx-http/src/state/cache.rscrates/rginx-http/src/state/helpers.rscrates/rginx-http/src/state/lifecycle/reload.rscrates/rginx-http/src/state/mod.rscrates/rginx-http/src/state/tests/support.rscrates/rginx-http/src/state/tls_runtime/bindings/tests.rscrates/rginx-http/src/transition/tests.rscrates/rginx-runtime/src/bootstrap/listeners/tests.rscrates/rginx-runtime/src/bootstrap/shutdown/tests.rscrates/rginx-runtime/src/health/tests.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). (1)
- GitHub Check: Agent
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-04-13T10:53:29.903Z
Learnt from: vansour
Repo: vansour/rginx PR: 43
File: crates/rginx-config/src/validate/server.rs:421-439
Timestamp: 2026-04-13T10:53:29.903Z
Learning: In `crates/rginx-config/src/validate/server.rs` (`validate_http3`), the check for `tls.session_tickets` when `http3.early_data = true` intentionally only rejects `Some(true)` and allows `None`. Requiring `Some(false)` breaks 0-RTT resumption and the `routes_http3_early_data_by_replay_safety` test and the CI `http3` gate in the current rustls/quinn integration. This is a deliberate design decision and should not be flagged as a bug until the runtime/session policy is updated to support that configuration.
Applied to files:
crates/rginx-config/src/validate/tests/route.rscrates/rginx-http/src/handler/tests/routing/authorize.rscrates/rginx-http/src/handler/tests/routing/selection.rscrates/rginx-config/src/validate/tests/vhosts.rscrates/rginx-http/src/state/tls_runtime/bindings/tests.rscrates/rginx-config/src/compile/tests/http3.rscrates/rginx-config/src/compile/tests/upstream_defaults.rscrates/rginx-config/src/compile/tests/server_tls.rscrates/rginx-config/src/compile/tests/upstream_server_name.rscrates/rginx-config/src/compile/tests/upstream_tls.rscrates/rginx-config/src/validate/tests/cache.rscrates/rginx-http/src/router/tests.rscrates/rginx-config/src/compile/tests/server_settings.rs
🪛 GitHub Actions: CI
crates/rginx-http/src/cache/mod.rs
[error] 349-349: Clippy error (op_ref): taken reference of right operand. Use request.method directly instead of &request.method. (implied -D clippy::op-ref)
crates/rginx-http/src/cache/tests.rs
[error] 159-159: Clippy error (unnecessary_get_then_check): unnecessary use of get(key).is_none(). Use !contains_key(key) instead. (implied -D clippy::unnecessary-get-then-check)
crates/rginx-http/src/proxy/forward/attempt.rs
[error] 1-1: modularization-gate: new production soft-limit violation. File has 346 lines; soft limit is 300.
🪛 LanguageTool
CACHE_SUPPORT_PLAN.md
[uncategorized] ~13-~13: 数词与名词之间一般应存在量词，可能缺少量词。
Context: ...YPASS、EXPIRED`。 本文档按阶段规划 rginx 的缓存支持。第一目标是实现用户可配置的 HTTP 反向代理响应缓存。现有内部缓存，例如 DNS 解析...
(wa5)
[uncategorized] ~29-~29: 数词与名词之间一般应存在量词，可能缺少量词。
Context: ...tion的请求。 - 默认不存储带Set-Cookie的响应。 - 第一版将Range` 请求视为 bypass。 - 采用“内存 metadata/...
(wa5)
🔇 Additional comments (47)
crates/rginx-core/src/config/cache.rs (1)
54-109: 模板变量语法已正确实现且与文档一致，无需修改。根据代码验证，模板语法实现与文档、测试、默认配置完全一致：
- 文档声明（docs/NEZHA_DASHBOARD.md）：明确指定
{host}、{scheme}等为模板变量语法，{{和}}用于转义字面量大括号- 实现一致性：解析器正确处理
{name}作为变量、{{/}}作为转义- 测试覆盖：所有单元测试和配置示例（
{scheme}:{host}:{uri}）均采用此语法- 默认配置（compile/cache.rs）：
DEFAULT_CACHE_KEY = "{scheme}:{host}:{uri}"不存在 Handlebars 风格
{{...}}变量的使用或配置，因此无缓存污染风险。> Likely an incorrect or invalid review comment.crates/rginx-http/src/proxy/health/registry/tests.rs (1)
125-125: 与上文同类问题（重复评论）同样建议复用顶部已导入的
HashMap，避免在每个测试 fixture 中使用std::collections::HashMap::new()全限定路径。crates/rginx-runtime/src/health/tests.rs (1)
55-55: 与前文同类问题（重复评论）文件顶部已
use std::collections::HashMap;，此处可写为cache_zones: HashMap::new(),与同 fixture 中第 76 行upstreams: HashMap::from(...)保持一致。crates/rginx-runtime/src/bootstrap/listeners/tests.rs (1)
39-39: 与前文同类问题（重复评论）文件已导入
HashMap，可用短路径HashMap::new()，与第 53 行upstreams: HashMap::new(),写法一致。crates/rginx-http/src/lib.rs (1)
3-3: LGTM新增内部
cache模块声明清晰且作用域为 crate 内部，符合 PR 中 cache MVP 的封装目标。crates/rginx-config/src/compile/tests/http3.rs (1)
12-12: LGTM为已有 HTTP/3 编译用例补齐新增的
cache_zones与LocationConfig.cache字段，与全仓 fixture 改动一致，未引入语义变化。Also applies to: 66-66, 117-117, 171-171
crates/rginx-http/src/handler/tests/routing/selection.rs (1)
140-140: LGTM两处手写构造的
Route字面量已补齐新增的cache: None字段，与Route结构定义一致，对 gRPC 路由选择测试不引入额外副作用。Also applies to: 186-186
crates/rginx-config/src/compile/tests/route/regex.rs (1)
6-6: 测试夹具补齐cache_zones字段，符合新 schema。两处
Config { ... }字面量显式初始化cache_zones: Vec::new()，与本 PR 引入的缓存配置编译路径一致，未影响原有路由匹配/排序断言。Also applies to: 122-122
crates/rginx-config/src/compile/tests/upstream_defaults.rs (1)
6-6: 与其他编译测试夹具保持一致，LGTM。显式将
cache_zones初始化为空Vec，未触及上游默认值相关的断言逻辑。Also applies to: 119-119
crates/rginx-http/src/handler/tests/routing/authorize.rs (1)
6-6:Route.cache = None初始化与新模型一致，LGTM。授权相关的请求/响应行为与断言均未改变，仅是对必填字段的填充。
Also applies to: 48-48
crates/rginx-config/src/validate/tests/route.rs (1)
116-116:LocationConfig字面量补齐cache: None，符合新 schema。仅影响该 push 的 location 构造，未改变重复路由/gRPC 约束相关断言。
crates/rginx-http/src/router/tests.rs (1)
13-13: 所有Route字面量统一补齐cache: None，LGTM。路由匹配、gRPC 约束等断言保持不变，仅扩展字段以匹配新模型。
Also applies to: 51-51, 71-71, 99-99, 183-183, 206-206, 238-238, 278-278
crates/rginx-http/src/handler/dispatch/route.rs (1)
48-48: 正确将路由级缓存策略透传至forward_request。
route.cache（Option<RouteCachePolicy>）通过DownstreamRequestOptions.cache进入forward/attempt.rs，与上下游集成（lookup/store、X-Cache标记）的实现路径一致；RouteExecutionContext在此处按值消费，无所有权问题。crates/rginx-http/src/handler/tests/routing/handle.rs (1)
45-46: LGTM！机械性补全
Route新增的cache: Option<RouteCachePolicy>字段，使该返回路由场景明确为不启用缓存，与本 PR 中其他测试夹具保持一致。crates/rginx-config/src/compile/tests/upstream_tls.rs (1)
10-10: LGTM！机械性补齐
Config.cache_zones与LocationConfig.cache新增字段，保持现有上游 TLS 测试用例缓存关闭，断言未变更。Also applies to: 80-80, 139-139, 200-200, 245-245, 315-315
crates/rginx-config/src/compile/tests/vhosts.rs (1)
13-13: LGTM！vhost 编译测试夹具机械补齐
cache_zones: Vec::new()，与新增的顶层缓存区配置字段对齐，无行为变更。Also applies to: 79-79, 134-134, 186-186, 215-215
crates/rginx-config/src/compile/tests/upstream_server_name.rs (1)
6-6: LGTM！补齐
cache_zones与LocationConfig.cache字段，测试断言（server_name_override 的标准化与拒绝逻辑）未变化。Also applies to: 67-67, 106-106, 174-174, 212-212, 273-273
crates/rginx-config/src/compile/tests/vhosts/listener_conflicts.rs (1)
47-47: LGTM！机械性补齐
cache_zones，监听冲突拒绝逻辑断言保持不变。crates/rginx-config/src/validate/tests/vhosts.rs (1)
146-167: LGTM！
LocationConfig显式补齐cache: None，与新增的Option<CacheRouteConfig>字段一致；该用例验证vhost tls without tls listener拒绝行为的断言保持不变。crates/rginx-config/src/model/route.rs (1)
53-54: LGTM!新增
cache字段使用#[serde(default)]，对现有配置文件保持向后兼容；CacheRouteConfig导入路径正确。crates/rginx-config/src/compile/route.rs (1)
68-122: LGTM!
cache字段被正确解构并通过compile_route_cache(cache)?编译，错误向上传播，与既有字段处理风格一致。crates/rginx-config/src/compile/tests/vhosts/nezha.rs (1)
28-61: LGTM!测试夹具同步补齐了新增的
cache_zones与cache字段，与新 schema 保持一致。crates/rginx-config/src/compile/tests/upstream_transport.rs (1)
6-276: LGTM!三处测试用例均按新 schema 显式设置
cache_zones: Vec::new()与LocationConfig.cache: None，未触及断言逻辑。crates/rginx-http/src/state/helpers.rs (1)
8-17: LGTM!
CacheManager的初始化通过?正确传播 IO/配置错误（包括from_config中的create_dir_all失败），并随PreparedState一同返回，与既有ProxyClients的装配模式一致。crates/rginx-config/src/compile/tests/server_tls.rs (1)
12-234: LGTM!三个 TLS 编译测试用例均机械性补齐
cache_zones与cache字段，未影响 TLS 断言逻辑。crates/rginx-config/src/compile/tests/listeners.rs (1)
6-73: LGTM!测试用例按新 schema 补齐
cache_zones与cache字段，断言部分未变更。crates/rginx-config/src/compile/tests/upstream_fallbacks.rs (1)
6-6: LGTM：补齐新增缓存字段的测试夹具更新。仅为对齐新增的
Config.cache_zones与LocationConfig.cache字段，未影响断言与被测逻辑，符合预期。Also applies to: 74-74, 128-128, 189-189, 237-237, 298-298
crates/rginx-config/src/compile/tests/route.rs (1)
8-8: LGTM：路由编译测试夹具同步缓存字段。仅是夹具补齐
cache_zones/cache字段，被测路由编译路径与断言未变。Also applies to: 36-36, 70-70, 98-98, 134-134, 171-171, 233-233, 298-298, 335-335
crates/rginx-http/src/handler/tests/support.rs (1)
33-33: LGTM：测试支撑代码与新 cache 字段对齐。
ConfigSnapshot.cache_zones/Route.cache初始化与新模型匹配，测试夹具仍保持禁用缓存的语义。Also applies to: 64-64
crates/rginx-config/src/compile/tests.rs (1)
5-11: LGTM：编译测试模块入口完成缓存测试集成。新模块
cache与导入扩展、test_location默认填充cache: None一致，能覆盖默认无缓存与显式缓存策略两类用例（详见同目录新增cache.rs）。Also applies to: 37-37, 56-56
crates/rginx-http/src/state/tests/support.rs (1)
21-21: LGTM：状态层测试夹具补齐缓存字段。
ConfigSnapshot.cache_zones、Route.cache默认值与现有用例语义一致（不启用路由缓存）。Also applies to: 94-94, 119-119
crates/rginx-config/src/validate/vhost.rs (1)
13-13: LGTM：vhost 校验透传缓存区名集合。
cache_zone_names仅作为透传参数传入validate_locations，未改变 vhost 自身的校验语义；与"缓存区仅在全局定义"的 MVP 设计一致。Also applies to: 55-55
crates/rginx-config/src/compile/tests/server_settings.rs (1)
6-6: LGTM!测试夹具仅按新增的
cache_zones与cache字段做了机械性补齐，未改动断言或调用流程，与compile/validate管线的扩展保持一致。Also applies to: 51-51, 79-79, 118-118, 146-146, 201-201, 229-229
crates/rginx-config/src/validate.rs (1)
7-7: LGTM!
validate_cache_zones先于validate_locations/validate_virtual_hosts运行，保证路由侧能对未声明 zone 的引用做存在性校验；签名扩展也已在两处调用点同步更新。Also applies to: 26-29, 38-38
crates/rginx-http/src/state/mod.rs (1)
14-20: LGTM!使用
Arc<StdRwLock<CacheManager>>在 reload 时整体替换 manager 是合适的；state/cache.rs内仅在同步作用域内持有锁并克隆管理器，规避了跨await持有 std 锁的隐患。CacheLookup/CacheStatus/CacheStoreContext的提前导入也支撑了state::cache子模块的super::*解析。Also applies to: 38-38, 82-82, 164-164
crates/rginx-http/src/proxy/forward/streaming.rs (1)
9-9: LGTM!
downstream改为借用形参后，函数体只读访问request_id等字段，调用方可在缓存查找/存储路径中复用DownstreamRequestContext而不必显式 clone，与Copy取消保持一致。crates/rginx-http/src/proxy/forward/types.rs (1)
3-18: LGTM!
RouteCachePolicy含String/Vec等非Copy字段，去掉Copy是必然结果；其它转发链路已配套改为按引用传递，整体一致。crates/rginx-config/src/model/cache.rs (1)
3-28: 新增的CacheZoneConfig和CacheRouteConfig与仓库内既有配置模型（如UpstreamConfig、LocationConfig等）保持一致，均不使用#[serde(deny_unknown_fields)]。这是仓库内的统一模式，而非不一致或缺陷。> Likely an incorrect or invalid review comment.crates/rginx-http/src/proxy/forward/setup.rs (1)
21-21: 已验证，RouteBufferingPolicy仍实现Copy特性，代码无问题。在
crates/rginx-core/src/config/route.rs中，RouteBufferingPolicy的定义为：#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] pub enum RouteBufferingPolicy { ... }第 84 行通过借用结构体按值传入
downstream.options.request_buffering是安全的。crates/rginx-http/src/state/cache.rs (1)
4-21: 移除该评论 -CacheManager::clone()已正确实现了共享状态语义。
CacheManager使用#[derive(Clone)]并持有Arc<HashMap<String, Arc<CacheZoneRuntime>>>字段。当克隆CacheManager时，内部的Arc会被克隆（O(1) 引用计数增长），所有克隆实例都指向同一份底层数据。lookup_cached_response和store_cached_response中的「读锁 → 克隆 → 释放锁 → await」模式正确无误：锁释放后，克隆的CacheManager仍持有对共享Arc的引用，后续的缓存操作（通过CacheZoneRuntime内部的Mutex进行同步）会正确地更新和读取同一份索引与磁盘状态。不存在状态不一致或静默失效的风险。> Likely an incorrect or invalid review comment.crates/rginx-config/src/compile/mod.rs (1)
47-99: LGTM!
cache_zones字段被正确地从Config解构出来，并通过cache::compile_cache_zones编译后注入ConfigSnapshot。字段顺序与crates/rginx-core/src/config/snapshot.rs中ConfigSnapshot的定义一致，编译流程整合完整。crates/rginx-config/src/validate/tests.rs (1)
1-200: LGTM!测试夹具同步更新为新的
Config.cache_zones与LocationConfig.cache字段，新增mod cache;与缓存配置类型的导入，与 PR 中 model/validate 的 schema 变更保持一致。crates/rginx-http/src/proxy/tests/cache.rs (2)
60-117: LGTM！miss→hit 测试通过预先入队
[OK, INTERNAL_SERVER_ERROR]两个状态码，巧妙地实现了对“第二次请求必须命中缓存而不再回源”的反向验证：若缓存未命中，第二次回源将返回 500，断言StatusCode::OK会立即暴露问题。x-cache: MISS/HIT的语义校验也清晰。
119-165: LGTM！通过附带
Authorization头部的GET请求验证x-cache: BYPASS行为，覆盖了缓存策略对认证请求的旁路处理。建议后续 phase 在此基础上补充对EXPIRED标记以及带stale_if_error的回源失败回退分支的端到端用例（可选）。crates/rginx-config/src/validate/route.rs (1)
94-118: LGTM！
validate_cache_handler_compatibility在validate_route_cache之前先校验“缓存策略仅可用于 Proxy 处理器”的约束，能在非 Proxy 路由（如 Return）配置缓存时给出明确错误信息；且当cache为None时两者都为 no-op，整体顺序与短路逻辑合理。cache_zone_names也被正确透传给super::cache::validate_route_cache。crates/rginx-app/src/check/summary.rs (1)
101-124: LGTM！
check_cache_zone_summaries显式按name排序，弥补了HashMap迭代顺序不稳定的问题，输出对快照/check 渲染友好且可复现；cache_enabled_route_count正确合并了default_vhost与vhosts，未重复统计（default_vhost与vhosts在ConfigSnapshot中是分离字段）。CACHE_SUPPORT_PLAN.md (1)
1-232: 文档与实现一致，无需修改。阶段 0–3 的当前实施状态描述与
validate/cache.rs、compile/cache.rs、cache/mod.rs、proxy/forward/{attempt,success}.rs的代码行为吻合（包括X-Cache: HIT/MISS/BYPASS/EXPIRED、默认 bypassAuthorization/Range、不存储Set-Cookie、stale不在 MVP 内等）。LanguageTool 提示均为 zh-CN 误报，可忽略。
vansour
force-pushed
the
feat/cache-support-mvp
branch
2 times, most recently
from
43d4ee3 to
4300378
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 430037842a
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 22
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/rginx-core/src/config/route.rs (1)
8-34: 🧹 Nitpick | 🔵 Trivial字段添加合理,可考虑用
Arc包装以降低克隆成本。
Route结构体在请求分发路径上会被.cloned()(见crates/rginx-http/src/handler/dispatch/mod.rs第 116 行),RouteCachePolicy内部包含Vec<Method>、Vec<StatusCode>与CacheKeyTemplate,每次请求都会做一次堆分配的深拷贝。若热路径上对克隆成本敏感,可将cache字段改为Option<Arc<RouteCachePolicy>>,使路由策略只读共享、克隆变为引用计数递增。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-core/src/config/route.rs` around lines 8 - 34, The Route struct's cache field is currently Option<RouteCachePolicy> causing expensive deep clones on each request; change it to Option<Arc<RouteCachePolicy>> by importing std::sync::Arc and updating the type in the Route definition (and any construction sites) so routes are cheaply cloned via Arc::clone; also update places that expect owned RouteCachePolicy (e.g., any code in dispatch or tests that dereferences or mutates cache) to handle &RouteCachePolicy (or Arc::as_ref()) and ensure RouteCachePolicy remains immutable or is wrapped appropriately if mutation is needed.crates/rginx-http/src/proxy/forward/types.rs (1)
3-18: 🧹 Nitpick | 🔵 Trivial移除
Copy合理；可选地用Arc共享缓存策略以降低克隆开销。
RouteCachePolicy内含字符串/集合等堆分配字段，去掉Copy是必要的。新字段cache: Option<rginx_core::RouteCachePolicy>在每次构造DownstreamRequestOptions时会随Clone一起复制；若策略对象较大或位于热路径（dispatch → forward → cache lookup），可考虑用Option<Arc<rginx_core::RouteCachePolicy>>，让 dispatch 阶段一次性Arc::clone即可贯穿整条转发链。- pub cache: Option<rginx_core::RouteCachePolicy>, + pub cache: Option<std::sync::Arc<rginx_core::RouteCachePolicy>>,非阻塞建议，需结合
rginx_core::RouteCachePolicy的实际字段规模与调用频度评估。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/proxy/forward/types.rs` around lines 3 - 18, DownstreamRequestOptions currently holds cache: Option<rginx_core::RouteCachePolicy> which forces heavy clones; change that field to cache: Option<std::sync::Arc<rginx_core::RouteCachePolicy>> and add use std::sync::Arc so the options struct remains Clone but cheap-to-clone (Arc::clone) in hot paths; update callers that construct DownstreamRequestOptions/DownstreamRequestContext to wrap the policy with Arc::new(...) or pass Arc clones, and where the policy is read use Arc deref/borrow as needed (keeping derives for Debug/Clone intact and removing any Copy impls if present).
♻️ Duplicate comments (1)
crates/rginx-http/src/cache/store.rs (1)
41-69:⚠️ Potential issue | 🔴 Critical未知大小响应仍会先全量缓冲到内存再判长（OOM 风险）。
store_response在第 50 行无条件body.collect().await，再到第 61 行才比较max_entry_bytes。如果上游使用分块编码或不带Content-Length，response_is_storable在外层无法依靠size_hint().exact()提前剔除，几 GB 的 chunked body 会被整体拉入内存。建议改为流式累计字节数，一旦超过context.zone.config.max_entry_bytes立即丢弃缓冲、把已有 chunk 透传给下游而不再缓存；或者在上层把缺乏精确 size hint 的响应直接判为不可缓存。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/cache/store.rs` around lines 41 - 69, store_response currently calls body.collect().await (in function store_response) before checking size, which buffers the entire response and can OOM for unknown-length (chunked) bodies; change the logic to stream the body instead of using body.collect(): iterate over body chunks, accumulate bytes up to context.zone.config.max_entry_bytes, and if the running total exceeds max_entry_bytes drop caching (stop buffering) and immediately return/forward a Response built from the original parts with the body sent through (i.e., do not call collect); ensure response_is_storable still gates obvious non-cache cases, and preserve the existing error handling and TTL logic when buffering completes successfully.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/rginx-config/src/compile/route.rs`:
- Line 122: 为 cache 的编译错误附加路由上下文：在调用点（route.rs 中的 `cache:
super::cache::compile_route_cache(cache)?`）把当前路由标识（如 `route_id` 或 matcher
字符串）传入，或在调用处用 anyhow/eyre 的 Context 包裹错误（例如 `.context(format!("route {}: failed
to compile cache", route_id))`）；同时在 `super::cache::compile_route_cache` 的签名中新增
`route_id: &str` 并在其所有返回错误处把该路由信息包含到错误消息里（例如把 “invalid cache method” 等错误格式化为包含
route 标识的消息），确保最终错误能定位到具体 route。
In `@crates/rginx-config/src/compile/tests/upstream_fallbacks.rs`:
- Line 6: 测试夹具里显式初始化的重复字段（如 cache_zones: Vec::new() 和 LocationConfig.cache:
None）应与新增模型字段保持一致并减少重复；实现或补充 Config 和 LocationConfig 的 Default（或提供一个 helper 构造函数
e.g. new_test_config / new_test_location）并在测试中用 Config::default() /
LocationConfig::default()（或 helper）替换这些重复初始化，以便所有相关测试（例如涉及
cache_zones、LocationConfig.cache 的 fixture）统一维护并自动包含未来新增字段。
In `@crates/rginx-config/src/compile/tests/upstream_server_name.rs`:
- Line 6: Many tests duplicate large Config/LocationConfig literals making
future optional-field additions noisy; add helper constructors (e.g. fn
default_test_config() -> Config and fn default_test_location() ->
LocationConfig) and update tests that currently inline Config/LocationConfig
(references: Config, LocationConfig in the upstream_server_name tests) to call
these helpers and override only the fields needed (or use ..Default::default()),
so new optional fields won't require editing every test; implement helpers in
the tests module or a shared test util and replace the repeated literals at the
call sites mentioned in the comment.
In `@crates/rginx-config/src/load/tests.rs`:
- Around line 18-29: Add assertions to cover the disk cache path and capacity
fields: in the test that inspects config.cache_zones and the deserialized route
cache (using config.cache_zones[0] and the local variable cache from
config.locations[0].cache), assert that cache_zones[0].path equals
"/var/cache/rginx/default" and cache_zones[0].max_size_bytes equals
Some(1048576), and likewise assert that the deserialized cache.path and
cache.max_size_bytes match those values so path and max_size_bytes are
validated.
In `@crates/rginx-config/src/model/cache.rs`:
- Around line 7-14: 在 crates/rginx-config/src/model/cache.rs 中，移除对 Option 字段上多余的
#[serde(default)] 属性：删除 max_size_bytes、inactive_secs、default_ttl_secs 和
max_entry_bytes 四个字段前的 #[serde(default)] 注解（保留字段类型为 Option<u64>
不变），以消除冗余并保持序列化行为一致。
- Around line 3-28: 为防止配置字段名拼写错误被静默忽略，给缓存配置结构添加 serde 的未知字段拒绝：在 CacheZoneConfig
和 CacheRouteConfig 的定义上各加上
#[serde(deny_unknown_fields)]（它们是反序列化的入口符号），以便反序列化时遇到多余/拼错字段立即报错；同时检查并在其他顶层配置结构（例如
RuntimeConfig、VirtualHostConfig、UpstreamConfig）按需统一应用相同的
#[serde(deny_unknown_fields)] 策略以保持校验一致性。
In `@crates/rginx-http/Cargo.toml`:
- Line 41: 在 Cargo.toml 中发现 serde_json 在 [dependencies] 与 [dev-dependencies]
重复声明（都使用 workspace = true）；请删除 [dev-dependencies] 区块中重复的 serde_json
条目，保留运行时依赖的声明（即在 [dependencies] 中的 serde_json = { workspace = true
}），以消除冗余并避免误导性表明该包仅用于测试。
In `@crates/rginx-http/src/cache/entry.rs`:
- Around line 63-100: read_cached_response currently does fs::read(&paths.body)
which loads the entire body into memory and holds the zone IO lock; change it to
open the file asynchronously with tokio::fs::File and stream its contents via
tokio_util::io::ReaderStream (or an equivalent async byte stream) and attach
that stream as the response body instead of full_body(Bytes::from(body));
specifically replace the fs::read usage of paths.body in read_cached_response
with File::open(&paths.body).await? wrapped in ReaderStream::new(file) and
convert that stream into the HTTP body (e.g., Body::wrap_stream or the project's
full_body streaming API), and ensure you release any zone.io_lock before
starting the streaming read so the lock isn't held for the whole file transfer.
- Around line 33-37: The CachedHeader struct currently serializes value: Vec<u8>
as a JSON numeric array; change its serde serialization to a compact text form
(e.g. base64) by annotating the field with #[serde(with = "base64_serde")] (or a
custom module like hex_serde) and ensure the corresponding serde helper
module/crate is added and imported; apply the same change to the other
occurrences mentioned (the similar header/metadata fields around lines 159-180)
so metadata is persisted as a base64/hex string instead of a byte-array, and
run/adjust any (de)serialization tests to use the new helper.
In `@crates/rginx-http/src/cache/load.rs`:
- Around line 50-58: The loop in scan_prefix_dir (used by load_index_from_disk)
currently uses entry? which aborts the whole zone scan on any io::Error; instead
change the handling of the per-entry result so a failed fs::read_dir entry
(entry?) is logged with tracing::warn! and skipped (continue) rather than
propagated, preserving the rest of the scan; keep the existing logic that calls
metadata_hash(&path), body_hash(&path), cache_paths(&zone.path, &hash) and
remove_cache_files(zone, &hash) for valid entries and only return Err for truly
unrecoverable errors at the directory level.
- Around line 97-102: The conversion from body_metadata.len() to usize can fail
and currently returns None without cleaning files; change the
usize::try_from(body_metadata.len()) call to handle the Err case by calling
remove_cache_files(zone, hash) before returning None, and keep the Ok(path) flow
identical so that when try_from succeeds you continue to compare with
metadata.body_size_bytes as before; update the code around the body_size binding
(the existing variable and the subsequent size check) to use this match/if-let
style error-handling so orphaned cache files are removed on conversion failure.
In `@crates/rginx-http/src/cache/mod.rs`:
- Around line 204-223: The current store_response (method store_response)
returns error.into_response from the Err branch (coming from
store::store_response) which can contain a failing body stream; instead, change
the Err branch to NOT forward error.into_response directly — either construct
and return a safe 5xx response (e.g. 502 Bad Gateway with a plain error body
produced via an error_response_body helper) and then call with_cache_status on
that, or propagate the error to upstream error handling; to implement this you
can extend CacheStoreError to indicate "body already consumed" if needed, but in
this function specifically replace the error.into_response usage with a
synthesized 5xx HttpResponse (or a propagated error) to avoid sending a response
whose body will immediately fail to downstream clients.
- Around line 134-140: The unknown-zone branch that returns
CacheLookup::Bypass(CacheStatus::Bypass) silently hides configuration errors:
update the block that checks let Some(zone) =
self.zones.get(&policy.zone).cloned() to emit a tracing::warn! (including
policy.zone and maybe route id) when the zone key is missing, and guard that
warn with a simple once_cell/HashSet or other frequency limiter so it doesn’t
spam on reloads; additionally adjust cache_request_bypass handling
(cache_request_bypass function and the CacheLookup::Bypass return sites) and the
stats/phases to differentiate "unknown_zone" vs "policy_bypass" counters (i.e.,
add a separate metric or enum variant for unknown_zone rather than lumping both
into a single BYPASS stat).
- Around line 149-172: When an entry is detected expired in the lookup (the
branch that currently returns Err(CacheStatus::Expired)), remove that entry from
index.entries under the same lock (use lock_index(&zone.index) /
index.entries.remove(&key)), decrement the zone.current_size_bytes accordingly,
then drop the index lock and schedule a best-effort async/file-delete task under
the io_lock (or push to the existing io cleanup queue) to remove the on-disk
meta.json/body; finally continue returning a CacheLookup::Miss using
CacheStoreContext::new as before. This ensures expired entries don't keep
counting toward current_size_bytes and their files are cleaned up without
blocking the critical index lock.
In `@crates/rginx-http/src/cache/policy.rs`:
- Around line 28-30: The check in response_is_storable currently treats
"no-cache" the same as "no-store" which is too strict; change the logic so
cache_control_contains(response.headers(), &["no-store", "private"]) is used to
decide non-storability and remove "no-cache" from that rejection list, and
instead surface "no-cache" by marking the stored entry as requiring revalidation
(e.g., set a requires_validation / must_revalidate flag on the cache entry or
response metadata when cache_control_contains(..., &["no-cache"]) is true).
Update response_is_storable and any CacheEntry/ResponseMetadata construction
code to record that flag so lookup/serve logic can enforce validation, or
document as an MVP exception if you intentionally keep the stricter behavior.
- Around line 41-46: 当前对 body size 的两处检查（response.body().size_hint().exact() 和
response.body().size_hint().upper()）是冗余的：当 exact() 为 Some(n) 时 upper() 也为
Some(n)，请合并为单一检查，删除对 exact() 的单独分支并仅使用 response.body().size_hint().upper()（匹配
Some(upper) if upper <= context.zone.config.max_entry_bytes as u64），保留对 None
的原有行为（返回 false），以改善可读性并保持现有逻辑（见 size_hint().exact(), size_hint().upper(),
context.zone.config.max_entry_bytes）。
In `@crates/rginx-http/src/cache/request.rs`:
- Around line 7-29: The GET/HEAD duplicate check in cache_request_bypass is
redundant because validate_route_cache already ensures policy.methods only
contains GET/HEAD; remove the explicit matches!(request.method, Method::GET |
Method::HEAD) branch so cache_request_bypass relies on
policy.methods.contains(&request.method) to gate allowed methods (update the
function cache_request_bypass accordingly), and keep the remaining header and
content-type checks as-is; optionally replace the string literals used in the
content-type comparison with a shared constant if you want clearer semantics.
In `@crates/rginx-http/src/cache/store.rs`:
- Around line 136-163: eviction_candidates currently can evict the item that was
just written if its size alone exceeds max_size_bytes; change the function
signature of eviction_candidates(index: &mut CacheIndex, max_size_bytes:
Option<usize>) -> Vec<String> to accept an extra parameter for the just-inserted
key (e.g. new_entry_key: Option<&str>) and skip removing any entry whose key
equals that new_entry_key when iterating/sorting entries; then update the caller
update_index_after_store (the code path that calls eviction_candidates during
store) to pass Some(&key) for the entry being stored so the newly inserted item
is not immediately evicted. Ensure behavior is unchanged when None is passed and
preserve the current last_access sorting and size accounting.
- Around line 81-103: The zone-level tokio::sync::Mutex io_lock currently
serializes all cache I/O so lookup hits (see lookup in mod.rs) block behind
store_response's write path (write_cache_entry) and its update_index_after_store
(which does eviction_candidates sorting and fs::remove_file), causing tail
latency; fix by decoupling read vs write: stop taking context.zone.io_lock in
the read path (lookup) and instead rely on atomic file rename + size checks when
reading cache files, keep io_lock only around write_cache_entry and
index/eviction updates (update_index_after_store), or implement a sharded lock
keyed by the cache key hash to reduce contention, and/or move costly
fs::remove_file eviction work out of update_index_after_store into a background
eviction worker (enqueue paths to remove and process them async) so write path
holds io_lock only briefly; modify store_response, write_cache_entry,
update_index_after_store, and remove_index_entry accordingly to implement one of
these approaches.
- Around line 83-103: 当前在 write_cache_entry 返回 Err 时无条件调用
remove_index_entry(&context.zone,
&context.key)，会把仍然有效的旧索引条目从内存移除导致磁盘残留和缓存静默失效。修改处理逻辑：让 write_cache_entry
返回能区分失败类型的结果（或在原有错误中携带标识），在错误分支只在确认是覆盖/最终文件被移除的失败（即对应 cleanup_failed_write 的
remove_final=true / “rename 已部分生效” 这种错误类型）时才调用 remove_index_entry；对于其他写入失败只记录
warning 并保留原有 index 条目（不调整 current_size_bytes），以保证旧文件继续可用并由后续淘汰流程清理。确保使用
write_cache_entry、remove_index_entry、cleanup_failed_write、update_index_after_store、context.key/context.zone
这些标识定位并实现该分支判断。
In `@crates/rginx-http/src/state/helpers.rs`:
- Line 8: CacheManager::from_config performs blocking IO (create_dir_all +
load_index_from_disk) each time prepare_state (including hot reload) runs;
update prepare_state to avoid always reloading: detect unchanged cache_zones in
the new config and reuse the existing CacheManager instead of calling
CacheManager::from_config, and only call CacheManager::from_config when the zone
set or paths change (so load_index_from_disk runs only on real changes). Also
consider adding a cleanup/rollback hook after create_dir_all when
prepare_listener_tls_acceptors or ProxyClients::from_config_with_health_notifier
fails (or document that create_dir_all is intentionally left as idempotent), and
mark load_index_from_disk as potentially expensive so future work can move it to
a blocking/task offloader if needed.
In `@crates/rginx-http/src/state/lifecycle/reload.rs`:
- Line 117: The update currently replaces self.cache before acquiring the
inner.write().await lock, creating a brief non-atomic window where readers may
observe new prepared.cache with old state.config/revision (or vice versa); move
the assignment that replaces self.cache (the write to self.cache using
prepared.cache) into the same critical section where you perform
inner.write().await and update state.config/clients/revision (i.e., perform
self.cache = prepared.cache inside the same locked block that updates
state.config = prepared.config.clone() and increments revision), or add a clear
comment stating the transient inconsistency is acceptable; ensure you keep the
poison-lock recovery pattern (unwrap_or_else(|poisoned| poisoned.into_inner()))
when modifying the cache.
---
Outside diff comments:
In `@crates/rginx-core/src/config/route.rs`:
- Around line 8-34: The Route struct's cache field is currently
Option<RouteCachePolicy> causing expensive deep clones on each request; change
it to Option<Arc<RouteCachePolicy>> by importing std::sync::Arc and updating the
type in the Route definition (and any construction sites) so routes are cheaply
cloned via Arc::clone; also update places that expect owned RouteCachePolicy
(e.g., any code in dispatch or tests that dereferences or mutates cache) to
handle &RouteCachePolicy (or Arc::as_ref()) and ensure RouteCachePolicy remains
immutable or is wrapped appropriately if mutation is needed.
In `@crates/rginx-http/src/proxy/forward/types.rs`:
- Around line 3-18: DownstreamRequestOptions currently holds cache:
Option<rginx_core::RouteCachePolicy> which forces heavy clones; change that
field to cache: Option<std::sync::Arc<rginx_core::RouteCachePolicy>> and add use
std::sync::Arc so the options struct remains Clone but cheap-to-clone
(Arc::clone) in hot paths; update callers that construct
DownstreamRequestOptions/DownstreamRequestContext to wrap the policy with
Arc::new(...) or pass Arc clones, and where the policy is read use Arc
deref/borrow as needed (keeping derives for Debug/Clone intact and removing any
Copy impls if present).
---
Duplicate comments:
In `@crates/rginx-http/src/cache/store.rs`:
- Around line 41-69: store_response currently calls body.collect().await (in
function store_response) before checking size, which buffers the entire response
and can OOM for unknown-length (chunked) bodies; change the logic to stream the
body instead of using body.collect(): iterate over body chunks, accumulate bytes
up to context.zone.config.max_entry_bytes, and if the running total exceeds
max_entry_bytes drop caching (stop buffering) and immediately return/forward a
Response built from the original parts with the body sent through (i.e., do not
call collect); ensure response_is_storable still gates obvious non-cache cases,
and preserve the existing error handling and TTL logic when buffering completes
successfully.
🪄 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: 811cd8e4-b145-41dd-8029-068dbfcc071d
📒 Files selected for processing (79)
CACHE_SUPPORT_PLAN.mdcrates/rginx-app/src/check/render.rscrates/rginx-app/src/check/routes.rscrates/rginx-app/src/check/summary.rscrates/rginx-config/src/compile/cache.rscrates/rginx-config/src/compile/mod.rscrates/rginx-config/src/compile/route.rscrates/rginx-config/src/compile/tests.rscrates/rginx-config/src/compile/tests/cache.rscrates/rginx-config/src/compile/tests/http3.rscrates/rginx-config/src/compile/tests/listeners.rscrates/rginx-config/src/compile/tests/route.rscrates/rginx-config/src/compile/tests/route/regex.rscrates/rginx-config/src/compile/tests/server_settings.rscrates/rginx-config/src/compile/tests/server_tls.rscrates/rginx-config/src/compile/tests/upstream_defaults.rscrates/rginx-config/src/compile/tests/upstream_fallbacks.rscrates/rginx-config/src/compile/tests/upstream_server_name.rscrates/rginx-config/src/compile/tests/upstream_tls.rscrates/rginx-config/src/compile/tests/upstream_transport.rscrates/rginx-config/src/compile/tests/vhosts.rscrates/rginx-config/src/compile/tests/vhosts/listener_conflicts.rscrates/rginx-config/src/compile/tests/vhosts/nezha.rscrates/rginx-config/src/load/tests.rscrates/rginx-config/src/model.rscrates/rginx-config/src/model/cache.rscrates/rginx-config/src/model/route.rscrates/rginx-config/src/validate.rscrates/rginx-config/src/validate/cache.rscrates/rginx-config/src/validate/route.rscrates/rginx-config/src/validate/tests.rscrates/rginx-config/src/validate/tests/cache.rscrates/rginx-config/src/validate/tests/route.rscrates/rginx-config/src/validate/tests/vhosts.rscrates/rginx-config/src/validate/vhost.rscrates/rginx-core/src/config.rscrates/rginx-core/src/config/cache.rscrates/rginx-core/src/config/route.rscrates/rginx-core/src/config/snapshot.rscrates/rginx-core/src/config/tests.rscrates/rginx-core/src/lib.rscrates/rginx-http/Cargo.tomlcrates/rginx-http/src/cache/entry.rscrates/rginx-http/src/cache/load.rscrates/rginx-http/src/cache/mod.rscrates/rginx-http/src/cache/policy.rscrates/rginx-http/src/cache/request.rscrates/rginx-http/src/cache/store.rscrates/rginx-http/src/cache/tests.rscrates/rginx-http/src/handler/dispatch/mod.rscrates/rginx-http/src/handler/dispatch/route.rscrates/rginx-http/src/handler/tests/routing/authorize.rscrates/rginx-http/src/handler/tests/routing/handle.rscrates/rginx-http/src/handler/tests/routing/selection.rscrates/rginx-http/src/handler/tests/support.rscrates/rginx-http/src/lib.rscrates/rginx-http/src/proxy/clients/tests.rscrates/rginx-http/src/proxy/forward/attempt.rscrates/rginx-http/src/proxy/forward/cache.rscrates/rginx-http/src/proxy/forward/failure.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/health/registry/tests.rscrates/rginx-http/src/proxy/tests/cache.rscrates/rginx-http/src/proxy/tests/mod.rscrates/rginx-http/src/router/tests.rscrates/rginx-http/src/state/cache.rscrates/rginx-http/src/state/helpers.rscrates/rginx-http/src/state/lifecycle/reload.rscrates/rginx-http/src/state/mod.rscrates/rginx-http/src/state/tests/support.rscrates/rginx-http/src/state/tls_runtime/bindings/tests.rscrates/rginx-http/src/transition/tests.rscrates/rginx-runtime/src/bootstrap/listeners/tests.rscrates/rginx-runtime/src/bootstrap/shutdown/tests.rscrates/rginx-runtime/src/health/tests.rs
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-04-13T10:53:29.903Z
Learnt from: vansour
Repo: vansour/rginx PR: 43
File: crates/rginx-config/src/validate/server.rs:421-439
Timestamp: 2026-04-13T10:53:29.903Z
Learning: In `crates/rginx-config/src/validate/server.rs` (`validate_http3`), the check for `tls.session_tickets` when `http3.early_data = true` intentionally only rejects `Some(true)` and allows `None`. Requiring `Some(false)` breaks 0-RTT resumption and the `routes_http3_early_data_by_replay_safety` test and the CI `http3` gate in the current rustls/quinn integration. This is a deliberate design decision and should not be flagged as a bug until the runtime/session policy is updated to support that configuration.
Applied to files:
crates/rginx-http/src/handler/tests/routing/authorize.rscrates/rginx-config/src/validate/tests/route.rscrates/rginx-http/src/handler/tests/routing/selection.rscrates/rginx-config/src/compile/tests/http3.rscrates/rginx-config/src/compile/tests/upstream_defaults.rscrates/rginx-config/src/compile/tests/server_tls.rscrates/rginx-config/src/compile/tests/upstream_server_name.rscrates/rginx-config/src/compile/tests/upstream_tls.rscrates/rginx-config/src/compile/tests/cache.rscrates/rginx-http/src/state/tls_runtime/bindings/tests.rscrates/rginx-config/src/compile/tests/server_settings.rscrates/rginx-config/src/load/tests.rscrates/rginx-runtime/src/bootstrap/shutdown/tests.rscrates/rginx-config/src/validate/vhost.rscrates/rginx-http/src/cache/tests.rscrates/rginx-config/src/validate/tests/vhosts.rscrates/rginx-http/src/cache/mod.rscrates/rginx-http/src/router/tests.rscrates/rginx-config/src/validate/tests/cache.rs
🪛 LanguageTool
CACHE_SUPPORT_PLAN.md
[uncategorized] ~13-~13: 数词与名词之间一般应存在量词，可能缺少量词。
Context: ...YPASS、EXPIRED`。 本文档按阶段规划 rginx 的缓存支持。第一目标是实现用户可配置的 HTTP 反向代理响应缓存。现有内部缓存，例如 DNS 解析...
(wa5)
[uncategorized] ~29-~29: 数词与名词之间一般应存在量词，可能缺少量词。
Context: ...tion的请求。 - 默认不存储带Set-Cookie的响应。 - 第一版将Range` 请求视为 bypass。 - 采用“内存 metadata/...
(wa5)
🔇 Additional comments (50)
crates/rginx-config/src/model/route.rs (1)
5-5: 新增字段与既有LocationConfig模式一致，LGTM。
cache: Option<CacheRouteConfig>与同结构中其它可选字段写法一致，缺省时反序列化为None，crates/rginx-app/src/check/routes.rs、crates/rginx-config/src/validate/tests.rs等下游也已正确处理。Also applies to: 53-54
crates/rginx-config/src/model.rs (1)
3-3: 模块挂载与字段新增 LGTM。
mod cache;与pub use cache::{CacheRouteConfig, CacheZoneConfig};与现有模块的命名/导出风格一致；cache_zones: Vec<CacheZoneConfig>使用#[serde(default)]，老配置文件无需改动即可继续加载。Also applies to: 12-12, 36-37
crates/rginx-config/src/compile/tests/vhosts/listener_conflicts.rs (1)
47-47: LGTM：仅是同步Config的新字段。crates/rginx-runtime/src/bootstrap/listeners/tests.rs (1)
39-39: LGTM：与rginx_core::ConfigSnapshot新增字段保持一致。注意此处使用
HashMap（运行时按 zone name 索引），与配置层的Vec<CacheZoneConfig>是不同的数据结构，符合预期。crates/rginx-http/src/lib.rs (1)
3-3: LGTM：将缓存子系统作为 crate 内部模块挂载。
mod cache;不对外导出，缓存子系统作为rginx-http的实现细节封装，避免污染公共 API 面，符合 MVP 阶段渐进暴露的策略。crates/rginx-app/src/check/routes.rs (1)
15-15: 与summary.rs::cache_enabled_route_count存在重复计数逻辑
RouteTransportCheckDetails::cache_enabled_routes与CheckSummary::cache_enabled_route_count各自独立遍历default_vhost+vhosts并判定route.cache.is_some()，违反 DRY。建议让CheckSummary直接复用此处已计算的route_transport.cache_enabled_routes，删除summary.rs中重复的计数函数，保持单一数据源。Also applies to: 51-53
crates/rginx-http/src/handler/dispatch/route.rs (1)
48-48: LGTM!将路由级
cache策略透传到DownstreamRequestOptions，类型一致（Option<RouteCachePolicy>），由于route: RouteExecutionContext按值传入，此处 move 也是安全的。crates/rginx-config/src/compile/route.rs (1)
122-122: validate 阶段已覆盖cache.zone交叉校验验证通过审查代码发现，
validate_route_cache函数（在crates/rginx-config/src/validate/cache.rs第46-54行）已完整实现了对cache.zone与cache_zone_names的交叉校验。若引用的 zone 不存在，会抛出错误references undefined cache zone，并配有单元测试验证此行为。无需额外处理。crates/rginx-http/src/transition/tests.rs (1)
28-29: LGTM！
snapshot()测试夹具补齐了新增的cache_zones字段并使用文件顶部已导入的HashMap，与upstreams: HashMap::new()风格一致；transition planning 测试逻辑未受影响。crates/rginx-http/src/handler/tests/routing/handle.rs (1)
45-64: LGTM！
Route字面量补齐了新增的cache: None字段，测试断言（status 299、无 LOCATION、bodyb"299\n"）保持不变。crates/rginx-config/src/validate/tests/vhosts.rs (1)
146-167: LGTM！
LocationConfig字面量补齐cache: None，与新增 schema 对齐；vhost tls without listener拒绝逻辑的断言保持不变。crates/rginx-http/src/state/tls_runtime/bindings/tests.rs (1)
100-111: LGTM！两个
ConfigSnapshot字面量都已补齐cache_zones: HashMap::new()，并使用文件顶部导入的HashMap；tls_binding_snapshots调用与断言保持不变。Also applies to: 139-150
crates/rginx-runtime/src/bootstrap/shutdown/tests.rs (1)
18-19: LGTM！
ConfigSnapshot.cache_zones与默认路由Route.cache均已显式初始化，并复用顶部use std::collections::HashMap;的导入；graceful shutdown 行为测试未受影响。Also applies to: 51-52
crates/rginx-config/src/validate/tests/route.rs (1)
115-137: LGTM！新增的
LocationConfig字面量补齐cache: None，与 schema 同步；不同 gRPC 约束允许重复 exact 路由的断言保持不变。crates/rginx-runtime/src/health/tests.rs (1)
54-55: LGTM！
ConfigSnapshot字面量补齐cache_zones: HashMap::new()，与 PR 中其它测试夹具风格一致；collect_probe_targets的断言与上游枚举行为未变。crates/rginx-config/src/compile/tests/upstream_tls.rs (1)
9-10: LGTM！三个
Config字面量补齐cache_zones: Vec::new()（配置层使用Vec，与编译后ConfigSnapshot使用HashMap的分层一致），三个LocationConfig同步补齐cache: None；compile/compile_with_base调用及对proxy.upstream、active_health_check的断言保持不变。Also applies to: 79-80, 138-139, 199-200, 244-245, 314-315
crates/rginx-config/src/compile/tests/vhosts/nezha.rs (1)
28-28: 测试夹具按新 schema 同步，无需评论。仅是为新增字段补默认值，逻辑保持不变。
Also applies to: 61-61
crates/rginx-http/src/handler/tests/routing/selection.rs (1)
140-140: 测试夹具按新Route.cache字段同步，逻辑无变更。Also applies to: 186-186
crates/rginx-http/src/handler/tests/routing/authorize.rs (1)
6-6: 仅同步新字段cache: None，无功能改动。Also applies to: 48-48
crates/rginx-config/src/compile/tests/listeners.rs (1)
6-6:Config.cache_zones与LocationConfig.cache默认值同步，无逻辑变更。Also applies to: 73-73
crates/rginx-http/src/router/tests.rs (1)
13-13: 所有路由夹具统一补齐cache: None，覆盖一致，无需变更。Also applies to: 51-51, 71-71, 99-99, 183-183, 206-206, 238-238, 278-278
crates/rginx-config/src/compile/tests/vhosts.rs (1)
13-13: 所有Config字面量补齐cache_zones: Vec::new()，与新 schema 对齐。Also applies to: 79-79, 134-134, 186-186, 215-215
crates/rginx-http/src/proxy/tests/mod.rs (1)
55-55: 新增cache子模块及空cache_zones字段初始化合理。
ConfigSnapshot::cache_zones用HashMap初始化，符合 core 中 zones 按名称索引的设计；mod cache;引入对应的代理缓存测试。Also applies to: 202-202
crates/rginx-http/src/state/helpers.rs (1)
11-17:PreparedState结构补充cache字段，错误传播路径符合既有约定。
?将CacheManager::from_config的Error::Server沿用现有错误流，且字段顺序与上下文构建顺序一致。crates/rginx-http/src/proxy/clients/tests.rs (1)
107-107: LGTM!测试夹具按
ConfigSnapshot的新cache_zones字段补齐,符合预期。crates/rginx-core/src/config/snapshot.rs (1)
4-14: LGTM!
cache_zones字段类型HashMap<String, Arc<CacheZone>>与upstreams风格一致,引入CacheZone的导入也匹配。crates/rginx-http/src/state/tests/support.rs (1)
21-21: LGTM!测试夹具按新增的
cache_zones/Route.cache字段补齐为HashMap::new()/None,与其他测试辅助保持一致。Also applies to: 94-94, 119-119
crates/rginx-http/src/handler/dispatch/mod.rs (1)
52-52: LGTM!
RouteExecutionContext.cache与route.cache.clone()的传递与下游build_route_response链路所需的缓存策略一致。克隆成本相关的优化建议已在crates/rginx-core/src/config/route.rs中给出,此处不再重复。Also applies to: 169-169
crates/rginx-core/src/config.rs (1)
2-15: LGTM!
cache模块声明与CacheKeyRenderContext、CacheKeyTemplate、CacheKeyTemplateError、CacheZone、RouteCachePolicy的 re-export 与下游 crate(rginx-config、rginx-http)的引用面一致。crates/rginx-config/src/compile/tests/route.rs (1)
8-8: LGTM!各测试用例的
Config.cache_zones与LocationConfig.cache已统一补齐为Vec::new()/None,与编译流水线新增字段对齐。关于减少字面量重复的建议见crates/rginx-config/src/compile/tests/upstream_server_name.rs中评论,不再重复。Also applies to: 36-36, 70-70, 98-98, 134-134, 171-171, 233-233, 298-298, 335-335
crates/rginx-config/src/compile/mod.rs (1)
57-57: 此问题已在校验层被正确处理，无需修改
validate::cache::validate_cache_zones（第 7-35 行）在校验阶段已明确处理重复检查：
- 第 11 行：对 zone 名称进行
trim()- 第 15-17 行：使用
HashSet::insert()并检查返回值，重名时返回Err(Error::Config("duplicate cache zone{name}"))因此，重名 zone 会在校验阶段被拒绝，永不到达编译阶段。名称仅靠首尾空白区分的两个 zone 也会被识别为重复（均在 trim 后比较）。编译阶段的 HashMap 收集不存在静默覆盖风险。
crates/rginx-app/src/check/render.rs (1)
8-14: 已按上次评审拆出print_cache_zone_details，职责划分清晰。
print_check_success中分别调用路由传输与缓存区详情打印，缓存区缺省max_size_bytes用"-"表示，输出格式与其他 check 行风格一致。Also applies to: 60-72
crates/rginx-http/src/proxy/forward/mod.rs (1)
7-23: 模块装配清晰，导入与子模块定义匹配。新增的
cache与failure子模块以及对应use项与上下文片段中实际导出的符号一致（ForwardCacheLookup、ForwardCacheContext、lookup_forward_cache、failed_to_build_request_response、upstream_unavailable_response），无重复或多余引入。crates/rginx-http/src/proxy/forward/setup.rs (1)
21-21: 改为按引用传参合理。由于
DownstreamRequestContext已不再实现Copy（见forward/types.rs），调用方需要在prepare_forward_request之后继续使用downstream（如attempt.rs第 96/120 行），改成&DownstreamRequestContext<'_>是必要且最小化改动。函数体只读访问options与request_id，与引用语义兼容。crates/rginx-config/src/validate.rs (1)
26-39: 验证流程接入正确。
validate_cache_zones先于route::validate_locations与vhost::validate_virtual_hosts调用，确保后续按引用使用的cache_zone_names已完成去重与字段校验（参考validate/cache.rs的实现）。两处下游校验都接收&cache_zone_names，无所有权问题。crates/rginx-config/src/validate/tests.rs (1)
1-12: 测试模块装配与夹具更新一致。
CacheRouteConfig和CacheZoneConfig通过本模块导入后，可被新增的mod cache子测试通过use super::*;复用，模式与现有同级子模块一致。base_config()/sample_vhost()显式补齐cache_zones、cache: None，避免编译期遗漏字段错误。Also applies to: 27-27, 88-88, 139-139
crates/rginx-http/src/state/mod.rs (1)
82-82:std::sync::RwLock的使用是安全的，guard 在.await前已释放。
SharedState.cache使用Arc<StdRwLock<CacheManager>>是正确的设计。在lookup_cached_response和store_cached_response中，代码模式为：let cache = { self.cache.read().unwrap_or_else(...).clone() }; cache.lookup(...).await块级作用域
{ }确保读锁在.await前释放，仅用于获取CacheManager的克隆（指针复制），不涉及长时间持锁。在CacheManager::lookup()和store_response()内部：
- 对同步数据结构
index的访问限制在同步代码段，锁立即释放- 磁盘 I/O（如
read_cached_response）通过AsyncMutex保护，不使用同步锁无需改用
tokio::sync::RwLock或重构。代码已正确避免在.await期间持有同步锁。crates/rginx-config/src/validate/tests/cache.rs (1)
57-81: 已按上一次评审建议拆分用例。
max_entry_bytes超限与不支持的POST方法已分别拆为validate_rejects_max_entry_bytes_exceeding_max_size_bytes与validate_rejects_unsupported_cache_method,失败定位更清晰。crates/rginx-config/src/compile/tests/cache.rs (1)
122-145: 已补齐默认回退断言。第二次编译现在显式断言了
policy.key.as_str() == "{scheme}:{host}:{uri}"与policy.stale_if_error == None,完整覆盖compile_route_cache的默认值路径,与上一次评审建议一致。crates/rginx-http/src/proxy/forward/streaming.rs (1)
9-9: 签名改为借用引用，与上层调用一致。将
downstream改为&DownstreamRequestContext<'_>后避免了所有权转移,与attempt.rs中后续仍需复用downstream的流程一致,内部仅做字段读取,语义未变。crates/rginx-core/src/lib.rs (1)
7-20: 新增缓存相关公共类型的 re-export。正确暴露了
CacheKeyRenderContext、CacheKeyTemplate、CacheKeyTemplateError、CacheZone、RouteCachePolicy,与rginx-http、rginx-config的使用方式对齐,无遗漏。crates/rginx-config/src/validate/route.rs (1)
94-118: 缓存与 Handler 兼容性检查接入正确。
validate_cache_handler_compatibility在调用validate_route_cache前先校验 handler 必须为Proxy,顺序合理(在 zone 名称未定义等错误前先暴露语义错误)。两个validate_locations调用点(validate.rs与validate/vhost.rs)都已传入cache_zone_names,签名变更的影响面已闭合。crates/rginx-http/src/proxy/forward/cache.rs (1)
13-36: lookup 适配层逻辑正确。
- 当
policy为None时短路返回Proceed,避免不必要的 lock/clone 读取(参考state/cache.rs::lookup_cached_response会克隆共享 cache)。Miss分支先取cache_status()再将contextmove 进store,顺序正确(CacheStatus派生Copy)。Bypass分支不携带store,与后续store_response的“无 store 即跳过写入”流程一致。crates/rginx-http/src/cache/policy.rs (1)
59-98: TTL 解析逻辑正确,边界处理符合预期。
s-maxage在跨多个 Cache-Control 值或同一值内多个指令时优先于max-age(在任意一处遇到s-maxage立即返回),与 HTTP 缓存语义一致。- 过期
Expires通过unwrap_or(Duration::ZERO)显式映射为零 TTL,而非回退到default_ttl,且注释清晰解释了原因。httpdate::parse_http_date失败回退为None,会落回default_ttl,符合“无法解析则视作无 freshness 元数据”的预期。如果想消除 Line 73-74、80 重复的
name.trim()调用可做小幅优化,但目前可读性可接受。crates/rginx-http/src/proxy/forward/attempt.rs (1)
12-23: 缓存命中分支仍直接返回，绕过下游响应处理。
ForwardCacheLookup::Hit(response)直接return response，未经过build_downstream_response/sanitize_response_headers/IdleTimeoutBody/GrpcDeadlineBody/ActivePeerBody等处理；与非缓存路径在finalize_upstream_success中获得的转换不一致。该问题在先前评审中已经提出，目前代码仍未修复。请确认是否计划在后续提交中处理，或在此 MVP 中给出明确取舍说明（例如缓存响应在store_response时已确保头部安全且使用full_body，所以无需再次包裹）。crates/rginx-http/src/proxy/forward/success.rs (1)
41-62: 升级响应分支显式丢弃cache_store行为合理。WebSocket 等升级响应（101）本身不应被缓存，第 50 行将
cache_store显式传入None、仅保留cache_status用于头部标记，与缓存语义一致。常规分支同时透传cache_store与cache_status，由apply_cache_store/store_cached_response内部统一负责盖戳，避免双重写入x-cache。CACHE_SUPPORT_PLAN.md (1)
1-12: 阶段计划文档与代码实现一致。文档第 8–11 行声明阶段 2/3 已完成 MVP（含磁盘元数据 + 内存索引 + 启动扫描 +
X-Cache输出），与crates/rginx-http/src/cache/mod.rs:108中的load_index_from_disk启动扫描以及with_cache_status写入HIT/MISS/BYPASS/EXPIRED的实现保持一致。crates/rginx-http/src/cache/entry.rs (1)
102-131: 临时文件 per-write 唯一后缀已修复并发覆盖问题。
next_temp_suffix()使用CACHE_TMP_COUNTER原子计数 +process::id()生成唯一后缀，配合cleanup_failed_write在各失败路径只删自己的临时文件，已经消除了之前metadata_tmp/body_tmp派生自固定 hash 时的并发交错风险。rename_body成功而rename_metadata失败那一支用remove_final=true清理已落盘的不一致状态也是正确的。crates/rginx-core/src/config/cache.rs (1)
54-130:CacheKeyTemplate::parse与render实现整体稳健。
- 大括号转义（
{{/}}）、UTF-8 边界处理（index += ch.len_utf8()）、变量未闭合 / 未转义}/ 空变量名 / 未知变量这几个错误分支都已覆盖；- 变量名经
trim()后再匹配，意味着{ scheme }等价于{scheme}，但保持大小写敏感（{Scheme}会拿到UnknownVariable）——这是一个"严格模式"，建议在CACHE_SUPPORT_PLAN.md的 cache key 部分简单说明；- 第 116 行
String::with_capacity(self.raw.len() + context.uri.len())的容量启发式偏向uri，对包含多个变量或 host/method 较长的模板会偏小，但仅影响一次Vec增长，可忽略。crates/rginx-config/src/compile/cache.rs (1)
16-43: 此问题已被 validator 处理。
validate_cache_zones()在crates/rginx-config/src/validate.rs中被调用，使用HashSet在执行.trim()后检测重复的 zone 名，若发现重复则返回错误："duplicate cache zone{name}"。因此重复的 zone 不会到达 compile 函数的 HashMap，无需额外的手动检测。
vansour
force-pushed
the
feat/cache-support-mvp
branch
from
4300378 to
61501e6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61501e6077
ℹ️ 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".
vansour
force-pushed
the
feat/cache-support-mvp
branch
from
61501e6 to
adb9ff2
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Update after
CodeRabbit is currently rate-limited on this PR, so its old |
Summary
Verification