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 (23)
📝 WalkthroughWalkthrough本次变更为 rginx 引入并完整实现 HTTP 缓存子系统：持久化元数据、TTL 与 stale 策略、条件重验证（ETag/Last-Modified）、缓存清除/维护、运营快照与管理员 CLI、访问日志集成及后台维护任务。 Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant CacheManager
participant Storage
participant Upstream
rect rgba(100,200,100,0.5)
Note over Client,Upstream: 缓存命中路径
Client->>Handler: GET /resource
Handler->>CacheManager: lookup(request)
CacheManager->>Storage: read_cached_response(key)
Storage-->>CacheManager: HttpResponse
CacheManager-->>Handler: Hit(response, status=HIT)
Handler->>Client: 200 OK (x-cache: HIT)
end
rect rgba(100,150,200,0.5)
Note over Client,Upstream: 未命中并写入
Client->>Handler: GET /resource
Handler->>CacheManager: lookup(request)
CacheManager-->>Handler: Miss(context)
Handler->>Upstream: 请求上游
Upstream-->>Handler: 200 OK
Handler->>CacheManager: store(response)
CacheManager->>Storage: write_cached_response(...)
Storage-->>CacheManager: write success
CacheManager-->>Handler: stored (x-cache: MISS)
Handler->>Client: 200 OK (x-cache: MISS)
end
rect rgba(200,100,100,0.5)
Note over Client,Upstream: 过期/重验证与 304 流程
Client->>Handler: GET /resource (If-None-Match/If-Modified-Since)
Handler->>CacheManager: lookup_with_revalidation(request)
CacheManager->>Storage: read_cached_metadata()
Storage-->>CacheManager: metadata (ETag/Last-Modified)
CacheManager->>Upstream: conditional request
Upstream-->>Handler: 304 Not Modified
Handler->>CacheManager: complete_not_modified(304)
CacheManager->>Storage: write_cache_metadata(updated)
Storage-->>CacheManager: ok
CacheManager-->>Handler: Revalidated(response, x-cache: REVALIDATED)
Handler->>Client: 200 OK (x-cache: REVALIDATED)
end
rect rgba(200,150,100,0.5)
Note over Client,Upstream: 上游错误时的陈旧回退
Client->>Handler: GET /resource
Handler->>Upstream: 请求上游
Upstream--x Handler: network/error
Handler->>CacheManager: serve_stale_on_error()
CacheManager->>Storage: read_stale_response()
Storage-->>CacheManager: stale_response
CacheManager-->>Handler: Stale(response, x-cache: STALE)
Handler->>Client: 200 OK (x-cache: STALE)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 分钟 Possibly related PRs
诗篇
🚥 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 |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR implements comprehensive cache support including snapshots, admin/CLI commands, access log integration, and runtime cache lifecycle management. The implementation is solid overall with good test coverage across multiple layers.
Critical Issues Found (2)
Two performance regressions were identified in cache operations that can block concurrent access under load:
- Inactive cleanup (store.rs:203-212): Cloning entire cache index during iteration keeps lock held unnecessarily long
- Cache purge (store.rs:234-247): Collecting matching entries with full entry clones prolongs lock duration
Both issues stem from holding the mutex lock while iterating over cloned collections. The fixes collect only keys first, then perform operations, significantly reducing lock contention.
Summary
The cache implementation architecture looks sound with proper separation of concerns, error handling, and lifecycle management. The identified performance issues should be addressed before merge to ensure the cache layer performs efficiently under production load.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly enhances the HTTP caching system, implementing stale-content serving, revalidation logic, and Vary: Accept-Encoding support. It also introduces administrative tools for cache management and monitoring. Feedback is provided to refine the revalidation logic for expired entries, improve performance by removing unnecessary I/O serialization on fresh hits, and optimize the memory efficiency of the background cleanup task.
There was a problem hiding this comment.
Pull request overview
This PR completes cache support phases 4–6 by wiring cache into runtime/admin/CLI observability (snapshots/status, $cache_status access logs, cache stats), adding cache lifecycle operations (inactive cleanup, purge commands, reload preservation), and extending cache behavior (revalidation, stale handling, Vary support, cache lock) with regression coverage across layers.
Changes:
- Add cache snapshots/stats into runtime status + admin socket + CLI, and add cache purge commands.
- Implement runtime inactive-cache cleanup task and broaden cache behavior (revalidation + stale windows + cache locking).
- Extend tests/docs to cover reload preservation, admin/CLI cache commands, and
$cache_statusaccess log variable.
Reviewed changes
Copilot reviewed 57 out of 57 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/install.sh | Creates and logs a cache root directory during install. |
| crates/rginx-runtime/src/lib.rs | Registers runtime cache background module. |
| crates/rginx-runtime/src/cache.rs | Adds periodic inactive cache cleanup background task. |
| crates/rginx-runtime/src/bootstrap/shutdown/tests.rs | Extends shutdown tests to include cache task draining/abort. |
| crates/rginx-runtime/src/bootstrap/shutdown.rs | Ensures cache task is joined/aborted during graceful shutdown. |
| crates/rginx-runtime/src/bootstrap/mod.rs | Spawns cache cleanup task and wires it into shutdown flow. |
| crates/rginx-runtime/src/admin/service.rs | Adds cache stats and purge request handling to admin service. |
| crates/rginx-runtime/src/admin/model.rs | Extends admin request/response schema with cache stats and purge results. |
| crates/rginx-runtime/src/admin/mod.rs | Bumps admin snapshot schema version for cache additions. |
| crates/rginx-http/src/state/tests/support.rs | Adds config snapshot helper for cache zones in tests. |
| crates/rginx-http/src/state/tests/status.rs | Adds status snapshot test coverage for cache zone stats. |
| crates/rginx-http/src/state/tests/snapshots.rs | Adds snapshot/delta coverage for cache zone changes. |
| crates/rginx-http/src/state/snapshots/runtime.rs | Adds cache stats into runtime status snapshot struct. |
| crates/rginx-http/src/state/snapshots/mod.rs | Exposes cache snapshot types from snapshots module. |
| crates/rginx-http/src/state/snapshots/delta.rs | Adds Cache snapshot module and related delta fields (schema v3). |
| crates/rginx-http/src/state/snapshots/cache.rs | Introduces CacheStatsSnapshot type. |
| crates/rginx-http/src/state/snapshot_bus.rs | Tracks cache component versions and emits cache delta fields (schema v3). |
| crates/rginx-http/src/state/mod.rs | Adds cache state module + exports cache snapshot/purge types + version tracking. |
| crates/rginx-http/src/state/lifecycle/status.rs | Includes cache snapshot inside runtime status snapshot generation. |
| crates/rginx-http/src/state/lifecycle/reload.rs | Wires cache notifier + cache version sync + cache change marking on reload. |
| crates/rginx-http/src/state/helpers.rs | Extends prepare_state and adds cache change notifier builder. |
| crates/rginx-http/src/state/counters/versions.rs | Adds cache component version counter. |
| crates/rginx-http/src/state/cache.rs | Adds SharedState cache stats snapshot, purge, cleanup, and cache version syncing. |
| crates/rginx-http/src/proxy/forward/success.rs | Handles 304 revalidation refresh and stale-on-error serving in proxy success path. |
| crates/rginx-http/src/proxy/forward/cache.rs | Adds conditional header application + stale-on-error helper on forward cache context. |
| crates/rginx-http/src/proxy/forward/attempt.rs | Applies conditional headers to upstream requests and serves stale on upstream errors. |
| crates/rginx-http/src/lib.rs | Re-exports new cache snapshot/purge types from rginx-http crate. |
| crates/rginx-http/src/handler/tests/observability.rs | Updates access-log rendering test context to include cache status. |
| crates/rginx-http/src/handler/dispatch/mod.rs | Plumbs x-cache into access log context ($cache_status). |
| crates/rginx-http/src/handler/access_log.rs | Extends access log context and formatting to include cache status. |
| crates/rginx-http/src/cache/tests.rs | Extends cache tests for new fields/behaviors and adds helpers. |
| crates/rginx-http/src/cache/store.rs | Adds revalidation refresh, inactive cleanup, purge implementation, and richer freshness/stats. |
| crates/rginx-http/src/cache/request.rs | Includes normalized Accept-Encoding in rendered cache key. |
| crates/rginx-http/src/cache/policy.rs | Adds response freshness model, stale directives, must-revalidate, and limited Vary support. |
| crates/rginx-http/src/cache/mod.rs | Adds cache stats snapshots, purge API, fill locks, stale/revalidated statuses, and conditional logic. |
| crates/rginx-http/src/cache/load.rs | Loads new metadata fields and retains entries for stale/revalidate windows. |
| crates/rginx-http/src/cache/entry.rs | Extends cache metadata format and adds helpers to read/write metadata and rebuild responses. |
| crates/rginx-core/src/config/tests.rs | Adds regression test for $cache_status access log variable. |
| crates/rginx-core/src/config/access_log.rs | Adds $cache_status / $upstream_cache_status variable support in access log format. |
| crates/rginx-app/tests/reload/cache.rs | Adds integration test ensuring reload preserves cache when zone path reused. |
| crates/rginx-app/tests/reload.rs | Includes new reload cache test module. |
| crates/rginx-app/tests/admin/snapshot/core.rs | Updates admin snapshot tests for schema bump and cache field presence. |
| crates/rginx-app/tests/admin/delta_wait/delta.rs | Updates delta tests for schema v3 and cache delta fields. |
| crates/rginx-app/tests/admin/commands/status.rs | Extends status command test expectations to include cache output. |
| crates/rginx-app/tests/admin/commands/cache.rs | Adds integration test for cache and purge-cache commands. |
| crates/rginx-app/tests/admin/commands.rs | Registers new admin command tests module. |
| crates/rginx-app/tests/access_log.rs | Adds integration test verifying $cache_status appears in access logs. |
| crates/rginx-app/src/main.rs | Treats cache-related commands as admin-socket commands. |
| crates/rginx-app/src/cli/tests.rs | Adds CLI parsing tests for cache and purge-cache commands and cache snapshot module. |
| crates/rginx-app/src/cli.rs | Adds cache and purge-cache subcommands and cache snapshot module arg. |
| crates/rginx-app/src/admin_cli/status/runtime.rs | Includes cache summary counters in status output. |
| crates/rginx-app/src/admin_cli/status.rs | Prints cache stats within status command output. |
| crates/rginx-app/src/admin_cli/socket.rs | Adds response kind strings for cache stats/purge responses. |
| crates/rginx-app/src/admin_cli/snapshot.rs | Maps --include cache to SnapshotModule::Cache. |
| crates/rginx-app/src/admin_cli/mod.rs | Adds admin CLI handlers for cache stats and purge-cache. |
| crates/rginx-app/src/admin_cli/cache.rs | Implements cache and purge-cache admin CLI output formatting. |
| CACHE_SUPPORT_PLAN.md | Marks phases 4–6 complete and documents new cache capabilities. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 8e811f4489
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/rginx-http/src/proxy/forward/cache.rs (1)
8-11:⚠️ Potential issue | 🔴 Critical请修复
ForwardCacheLookup的大枚举变体，当前会阻断 CI。Line 8-11 的
Hit(HttpResponse)触发了clippy::large_enum_variant（已在流水线报错）。建议对大变体装箱以缩小枚举体积。建议修复
pub(super) enum ForwardCacheLookup { - Hit(HttpResponse), + Hit(Box<HttpResponse>), Proceed(ForwardCacheContext), } @@ - crate::cache::CacheLookup::Hit(response) => ForwardCacheLookup::Hit(response), + crate::cache::CacheLookup::Hit(response) => ForwardCacheLookup::Hit(Box::new(response)),Also applies to: 24-24
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/proxy/forward/cache.rs` around lines 8 - 11, The enum ForwardCacheLookup has a large variant Hit(HttpResponse) causing clippy::large_enum_variant; change the variant to store a boxed value (Hit(Box<HttpResponse>)) to reduce enum size, and update all construction sites that create ForwardCacheLookup::Hit to wrap the response with Box::new(...) and update all match arms/uses to dereference the Box (e.g., let resp = *boxed_resp or pattern-match the Box) so ForwardCacheContext/Proceed uses remain unchanged; also apply the same boxing fix to any other large-variant occurrences referenced (the ForwardCacheContext-related hit at the other spot).crates/rginx-http/src/cache/tests.rs (1)
17-73: 🛠️ Refactor suggestion | 🟠 Major把测试夹具提到共享 support，先把文件降回 soft limit。
这次新增的
test_index_entry/test_store_context再加上重复的 zone/context 构造，把测试文件推到了 431/400，CI 已经卡住。建议把这些构造器移到cache的测试辅助模块里，并复用CacheZoneRuntime初始化，避免后面每次扩CacheStoreContext都要在多处同步。Also applies to: 418-423
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/cache/tests.rs` around lines 17 - 73, 当前测试文件超出软限制：把重复的测试夹具（test_zone、test_index_entry、test_store_context）抽取到 cache 的共享测试辅助模块（例如 tests::support 或 cache::tests::support），在该模块中实现并导出用于创建 CacheZoneRuntime、CacheIndexEntry、CacheStoreContext 的工厂函数并复用同一 CacheZoneRuntime 初始化，随后在原测试文件中引用这些共享函数以删除重复定义并减少文件大小；同时将原文件中与 CacheStoreContext 相关的重复构造移除，确保导出的函数名（test_zone、test_index_entry、test_store_context 或更语义化的 create_* 名称）与现有测试调用一致。crates/rginx-http/src/cache/entry.rs (1)
51-61:⚠️ Potential issue | 🔴 Critical
cache_metadata参数过多，当前阻断 CI 合并。Line 51 的
cache_metadata有 9 个参数，超过仓库配置的阈值（clippy.toml:too-many-arguments-threshold = 8），触发clippy::too_many_arguments违规。由于仓库启用-D warnings，该违规阻断 CI 且必须在合并前修复。建议将
stale_if_error_until_unix_ms、stale_while_revalidate_until_unix_ms、must_revalidate三个关联参数聚合为CacheFreshnessMetadata结构体，这三个参数在所有调用点都来自同一个freshness对象，聚合后的设计语义清晰。建议修复
+#[derive(Debug, Clone, Copy, Serialize, Deserialize, Default)] +pub(super) struct CacheFreshnessMetadata { + #[serde(default)] + pub(super) stale_if_error_until_unix_ms: Option<u64>, + #[serde(default)] + pub(super) stale_while_revalidate_until_unix_ms: Option<u64>, + #[serde(default)] + pub(super) must_revalidate: bool, +} + pub(super) fn cache_metadata( key: String, status: StatusCode, headers: &HeaderMap, stored_at_unix_ms: u64, expires_at_unix_ms: u64, - stale_if_error_until_unix_ms: Option<u64>, - stale_while_revalidate_until_unix_ms: Option<u64>, - must_revalidate: bool, + freshness: CacheFreshnessMetadata, body_size_bytes: usize, ) -> CacheMetadata { CacheMetadata { key, status: status.as_u16(), headers: cached_headers(headers, body_size_bytes), stored_at_unix_ms, expires_at_unix_ms, - stale_if_error_until_unix_ms, - stale_while_revalidate_until_unix_ms, - must_revalidate, + stale_if_error_until_unix_ms: freshness.stale_if_error_until_unix_ms, + stale_while_revalidate_until_unix_ms: freshness.stale_while_revalidate_until_unix_ms, + must_revalidate: freshness.must_revalidate, body_size_bytes, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/cache/entry.rs` around lines 51 - 61, The function cache_metadata currently has nine parameters and trips clippy::too_many_arguments; fix it by introducing a new struct CacheFreshnessMetadata that groups stale_if_error_until_unix_ms, stale_while_revalidate_until_unix_ms, and must_revalidate, change cache_metadata signature to take a CacheFreshnessMetadata (e.g., freshness) instead of those three fields, update the CacheMetadata construction as needed to read from freshness, and update every caller (which currently pass those three values from a freshness object) to pass the new CacheFreshnessMetadata instance; ensure all references to stale_if_error_until_unix_ms, stale_while_revalidate_until_unix_ms, and must_revalidate are updated to freshness.<field> so the clippy warning is resolved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CACHE_SUPPORT_PLAN.md`:
- Around line 10-18: 顶部状态与分阶段正文在 revalidation 能力上不一致：顶部把 "REVALIDATED" 列为“阶段 3
已完成”，但正文直到 "阶段 5" 才引入 revalidation 语义；请统一表述，方案二选一：要么把顶部状态改为明确的“累计到当前版本的能力”并保留
REVALIDATED 在总览中以反映已包含的特性，要么将 "REVALIDATED" 从顶部移除并把该词语及相关说明恢复到 "阶段 5"
的摘要中；定位关键词："顶部状态"、"阶段 3"、"阶段 5"、"REVALIDATED" 并按选定方案调整对应段落以消除矛盾。
In `@crates/rginx-app/src/admin_cli/status/runtime.rs`:
- Around line 123-142: 该段格式不符合 cargo fmt 要求：在构造 tuple 键 "cache_entries" 和
"cache_current_size_bytes" 时对
status.cache.zones.iter().map(...).sum::<usize>().to_string() 的换行/缩进需要用 rustfmt
自动调整；请在包含该代码的模块（使用到 status.cache.zones、entry_count、current_size_bytes 的表达式）上运行
cargo fmt --all 并提交格式化后的变更，以让 rustfmt 调整 map/iter/sum/to_string 调用的换行和缩进通过 CI
检查。
In `@crates/rginx-app/src/cli/tests.rs`:
- Around line 128-129: The formatting check failed around the test that calls
Cli::try_parse_from(["rginx", "snapshot", "--include", "cache"]) in the cli
tests; run rustfmt to fix it by executing cargo fmt --all (or rustfmt on the
test module), re-run cargo fmt --all --check to confirm, and commit the
reformatted changes so the Cli::try_parse_from test and surrounding code pass CI
formatting.
- Around line 179-215: Add a negative test to ensure the purge-cache ArgGroup
enforces mutual exclusion: create a new test (e.g.,
cli_rejects_purge_cache_both_key_and_prefix) that calls Cli::try_parse_from with
both "--key" and "--prefix" supplied, and assert that parsing fails (Err) rather
than returning Command::PurgeCache; reference Cli::try_parse_from and the
Command::PurgeCache / PurgeCacheArgs shape to locate existing tests to mirror
structure and failure expectation.
In `@crates/rginx-app/tests/access_log.rs`:
- Around line 161-189: spawn_counting_response_server currently spawns a
listener thread that loops forever (listener / hits_task) and never shuts down;
change it to accept a shutdown handle (e.g., return a Sender or an
Arc<AtomicBool> alongside (SocketAddr, Arc<AtomicUsize>)) and make the spawned
loop check that signal so it breaks cleanly; specifically modify
spawn_counting_response_server to create a shutdown channel/flag, have the outer
thread loop check the flag or select on the channel before calling
listener.accept(), and return the shutdown handle so tests can signal the server
to close and join/stop the thread.
In `@crates/rginx-app/tests/admin/commands/cache.rs`:
- Around line 81-131: 测试辅助函数
cached_proxy_config、spawn_counting_response_server、response_header_value
在两个测试文件里重复出现，应该抽到一个公共测试模块：创建一个共享测试辅助模块（例如 tests/common 或
crates/rginx-app/tests/common.rs），把这三者实现移动到该模块并将它们声明为 pub（例如 pub fn
cached_proxy_config/... pub fn spawn_counting_response_server/... pub fn
response_header_value/...），在原先的测试文件（包括 tests/admin/commands/cache.rs 和
tests/reload/cache.rs）引入并使用该公共模块的函数（use crate::common::*
或相应路径），然后删除各自文件中的重复实现以避免代码漂移；确保导出/导入的路径和可见性与现有测试编译环境一致并运行测试以验证改动。
In `@crates/rginx-app/tests/reload.rs`:
- Around line 29-30: The CI failure is due to formatting differences in the
module declaration (#[path = "reload/cache.rs"] mod cache;) — run rustfmt by
executing "cargo fmt --all" on this branch to reformat the codebase, verify with
"cargo fmt --all --check" that formatting now passes, then commit and push the
reformatted files (including the updated declaration file) so the CI format
check succeeds.
In `@crates/rginx-core/src/config/access_log.rs`:
- Around line 244-245: The match arms handling AccessLogVariable (e.g.,
AccessLogVariable::CacheStatus and usages of fallback_access_log_option) should
be extracted into a dedicated helper/submodule that exposes parsing and
rendering helpers (for example access_log_variable::parse_variable and
access_log_variable::render_variable); change AccessLogFormat to only perform
template tokenization and delegate any variable parse/render logic to those
helpers so all current match branches are removed from access_log.rs and
centralized in the new helper; update callers in AccessLogFormat to call the
helper functions and keep existing function names (AccessLogVariable,
fallback_access_log_option) referenced so the refactor is minimally invasive.
In `@crates/rginx-core/src/config/tests.rs`:
- Around line 365-366: The test file fails `cargo fmt --all --check`; run
rustfmt (e.g., `cargo fmt --all`) to reformat the test source so the line
containing AccessLogFormat::parse("cache=$cache_status").expect("access log
format should parse"); in the tests module matches the project's formatter, then
re-run `cargo fmt --all --check` and commit the formatted changes.
In `@crates/rginx-http/src/cache/load.rs`:
- Around line 122-130: The current check sets beyond_stale_windows=true for
empty iterators (when both metadata.stale_if_error_until_unix_ms and
metadata.stale_while_revalidate_until_unix_ms are None), causing
expired-but-still-stale entries to be removed; update the logic in the load path
that computes beyond_stale_windows so that if there are no stale window
timestamps present it evaluates to false, otherwise require every present
timestamp to be before now (e.g., collect/flatten the Option timestamps from
metadata.stale_if_error_until_unix_ms and
metadata.stale_while_revalidate_until_unix_ms into a list and set
beyond_stale_windows = timestamps.non_empty() && timestamps.iter().all(|v| now >
*v)); keep the subsequent check using now, metadata.expires_at_unix_ms,
metadata.must_revalidate and remove_cache_files(zone, hash) intact.
In `@crates/rginx-http/src/cache/mod.rs`:
- Around line 506-546: After successfully decoding metadata in
load_lookup_metadata, validate that metadata.key == key and treat a mismatch as
corruption: log a warning (including zone/config name and entry.hash), call
remove_index_entry(zone, key), await remove_cache_files_if_unindexed(zone, key,
&entry.hash), and return None so we never build conditional headers or use this
metadata; apply the same guard to the other similar path noted (the later block
around stale_response_from_entry) to avoid issuing conditional requests or
returning a wrong cached body when index and on-disk metadata diverge.
In `@crates/rginx-http/src/cache/policy.rs`:
- Around line 73-76: request_requires_revalidation currently only inspects
Cache-Control (via cache_control_contains and cache_control_duration) and misses
requests that use the legacy Pragma: no-cache; update
request_requires_revalidation to also inspect the request's Pragma header (from
the same HeaderMap) and return true if the header contains the token "no-cache"
(case-insensitive or token-parsed), so requests with only Pragma: no-cache are
treated as requiring revalidation; keep existing checks (cache_control_contains
and cache_control_duration) and short-circuit if any indicates revalidation.
In `@crates/rginx-http/src/cache/request.rs`:
- Around line 58-60: The function normalized_accept_encoding currently calls
headers.get(ACCEPT_ENCODING) which only returns a single header value and will
miss repeated Accept-Encoding headers; update normalized_accept_encoding to use
headers.get_all(ACCEPT_ENCODING), iterate all values, convert each
to_str().ok(), trim and skip empties, join them (e.g., with a comma or canonical
separator), and then normalize/return the combined string (or None if empty).
Locate and modify normalized_accept_encoding to aggregate get_all results,
perform safe UTF-8 conversion and trimming for each header value, and produce a
deterministic normalized header string for cache-key consistency.
In `@crates/rginx-http/src/cache/store.rs`:
- Around line 143-169: Merged 304 handling writes updated metadata without
re-checking cacheability; after calling
merge_not_modified_headers(&cached_headers, response.headers()) and computing
freshness via response_freshness(&context, &merged_headers), call
response_is_storable(&context, &merged_headers) and
freshness_is_cacheable(&freshness) (or the existing cacheability helpers) and
abort (do not call cache_metadata/write_cache_metadata) if either check fails
(e.g., new Cache-Control: no-store/private, Set-Cookie, or incompatible Vary);
only proceed to build metadata and write_cache_metadata(&paths, &metadata) when
the merged headers and freshness are still cacheable.
- Around line 197-225: The cleanup functions (e.g.,
cleanup_inactive_entries_in_zone) remove entries from the in-memory index and
delete files but do not emit a change notification, causing admin
stats/snapshots to remain stale; after you finish mutating the index and
removing the files (after the removed loop and after calling
zone.record_inactive_cleanup), call zone.notify_changed() (or reuse the existing
remove_index_entry()/update_index_after_store() path that triggers
notifications) so the zone publishes the index change; apply the same fix to the
other cleanup function referenced (the one around lines 227-265) to ensure both
disk/index removals emit zone.notify_changed().
In `@crates/rginx-http/src/handler/tests/observability.rs`:
- Line 44: The test sets cache_status: Some("HIT") but never asserts it appears
in the rendered output; update the test in observability.rs to explicitly assert
the template rendered the cache status (for example, locate the struct/response
where cache_status: Some("HIT") is set and add an assertion like
assert!(rendered_output.contains("HIT")) or the exact rendered fragment
expected). Ensure you reference the same variable name cache_status when adding
the assertion so the test validates the $cache_status path is actually rendered.
In `@crates/rginx-http/src/proxy/forward/attempt.rs`:
- Around line 232-236: The code currently calls
state.record_upstream_bad_gateway_response (and the analogous
record_upstream_gateway_timeout_response) before attempting to return a stale
response, causing successful stale returns to be mis-counted as 502/504; change
the control flow so you first call cache.serve_stale_on_error().await and, if it
returns Some(stale), return it immediately, and only if stale is not available
then call state.record_upstream_bad_gateway_response /
state.record_upstream_gateway_timeout_response and return
cache.mark_response(bad_gateway(...)) /
cache.mark_response(gateway_timeout(...)). Apply the same reorder to the other
similar block that uses record_upstream_gateway_timeout_response and
cache.serve_stale_on_error().
In `@crates/rginx-http/src/state/helpers.rs`:
- Around line 57-73: 当前的 build_cache_notifier 会在每次缓存计数变更时对 snapshot_version 做
fetch_add 并调用 snapshot_notify.notify_waiters(), 导致高频命中/未命中路径触发全局 snapshot
唤醒；请改为只在结构性变更（例如 purge/cleanup/zone 重建）触发全局通知：在 build_cache_notifier 中移除对
snapshot_version.fetch_add/ snapshot_components.*.store/
snapshot_notify.notify_waiters 的每次调用；保留或新增一个明确的触发器函数（例如 notify_structural_change
或在 CacheZoneRuntime 中的相关 purge/rebuild 路径）来做上述 fetch_add 和 notify_waiters；而对高频的
record_hit/record_miss/record_bypass
操作（CacheZoneRuntime::record_hit/miss/bypass）仅更新原子计数或使用批量/节流策略并通过读取原子值提供统计，不做全局
notify。
In `@crates/rginx-http/src/state/mod.rs`:
- Around line 14-20: 该文件的新增 use / pub use 语句未通过 rustfmt；在
crates/rginx-http/src/state/mod.rs（包含 use crate::cache::..., use
crate::proxy::..., use crate::rate_limit::..., use crate::tls::... 以及 pub use
包装）运行格式化以通过 CI。修复方法：在仓库根目录执行 cargo fmt --all（或 cargo +stable fmt --all / 使用
rustfmt），将格式化后的变更添加并提交，确保 state::mod.rs 中的 use/pub use 导入被 rustfmt 调整后再推送；如果 CI
仍报错，手动运行 rustfmt on that file 直到 no changes.
In `@crates/rginx-http/src/state/tests/snapshots.rs`:
- Around line 154-162: The match expression formatting for the cache lookup
block is failing rustfmt; reformat the block (or run cargo fmt) so it satisfies
rustfmt — ensure the chained call to
active.cache.lookup(CacheRequest::from_request(&request), "https",
&policy).await and the subsequent match arms (CacheLookup::Miss(context) =>
context, CacheLookup::Hit(_) => panic!(...), CacheLookup::Bypass(status) =>
panic!(...)) are indented/line-broken according to rustfmt rules (align the
match keyword and its arms and keep the .await positioning consistent) so that
active.cache.lookup, CacheRequest::from_request, and the CacheLookup variants
are properly formatted.
In `@crates/rginx-http/src/state/tests/status.rs`:
- Around line 269-314: The new test function
status_snapshot_reports_cache_zone_stats has formatting differences causing CI
fail; run rustfmt (cargo fmt --all) to reformat this file and commit the changes
so the match expression around
active.cache.lookup(CacheRequest::from_request(&request), "https", &policy) and
the surrounding code conform to rustfmt style; ensure the file
crates/rginx-http/src/state/tests/status.rs is updated and passes cargo fmt
--check before pushing.
In `@crates/rginx-runtime/src/admin/service.rs`:
- Line 162: The match arm handling AdminRequest::GetCacheStats is misformatted
and fails cargo fmt --check; run rustfmt (cargo fmt) to fix formatting so the
arm reads as a properly wrapped/indented expression (the branch using
AdminRequest::GetCacheStats =>
AdminResponse::CacheStats(state.cache_stats_snapshot().await) should be
reformatted). Locate the match on AdminRequest in service.rs and reformat the
arm containing AdminResponse::CacheStats and state.cache_stats_snapshot().await
(or run rustfmt/cargo fmt to apply the correct formatting) and re-run cargo fmt
--check.
In `@crates/rginx-runtime/src/bootstrap/mod.rs`:
- Line 53: 该行赋值 let mut cache_task =
Some(tokio::spawn(cache::run(state.http.clone(), shutdown_tx.subscribe())));
因行长超限导致 cargo fmt 报错；请将该表达式按项目中 admin_task/health_task 的风格改为多行格式（拆分 tokio::spawn
调用参数和内部调用），保持相同的缩进和换行，以使用 cache_task、tokio::spawn、cache::run、state.http.clone()
和 shutdown_tx.subscribe() 这些标识符定位并重排代码使其通过 cargo fmt --all --check。
---
Outside diff comments:
In `@crates/rginx-http/src/cache/entry.rs`:
- Around line 51-61: The function cache_metadata currently has nine parameters
and trips clippy::too_many_arguments; fix it by introducing a new struct
CacheFreshnessMetadata that groups stale_if_error_until_unix_ms,
stale_while_revalidate_until_unix_ms, and must_revalidate, change cache_metadata
signature to take a CacheFreshnessMetadata (e.g., freshness) instead of those
three fields, update the CacheMetadata construction as needed to read from
freshness, and update every caller (which currently pass those three values from
a freshness object) to pass the new CacheFreshnessMetadata instance; ensure all
references to stale_if_error_until_unix_ms,
stale_while_revalidate_until_unix_ms, and must_revalidate are updated to
freshness.<field> so the clippy warning is resolved.
In `@crates/rginx-http/src/cache/tests.rs`:
- Around line 17-73:
当前测试文件超出软限制：把重复的测试夹具（test_zone、test_index_entry、test_store_context）抽取到 cache
的共享测试辅助模块（例如 tests::support 或 cache::tests::support），在该模块中实现并导出用于创建
CacheZoneRuntime、CacheIndexEntry、CacheStoreContext 的工厂函数并复用同一 CacheZoneRuntime
初始化，随后在原测试文件中引用这些共享函数以删除重复定义并减少文件大小；同时将原文件中与 CacheStoreContext
相关的重复构造移除，确保导出的函数名（test_zone、test_index_entry、test_store_context 或更语义化的 create_*
名称）与现有测试调用一致。
In `@crates/rginx-http/src/proxy/forward/cache.rs`:
- Around line 8-11: The enum ForwardCacheLookup has a large variant
Hit(HttpResponse) causing clippy::large_enum_variant; change the variant to
store a boxed value (Hit(Box<HttpResponse>)) to reduce enum size, and update all
construction sites that create ForwardCacheLookup::Hit to wrap the response with
Box::new(...) and update all match arms/uses to dereference the Box (e.g., let
resp = *boxed_resp or pattern-match the Box) so ForwardCacheContext/Proceed uses
remain unchanged; also apply the same boxing fix to any other large-variant
occurrences referenced (the ForwardCacheContext-related hit at the other spot).
🪄 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: 76cfd0e1-8b2a-4c2c-9705-5c3736db10ba
📒 Files selected for processing (57)
CACHE_SUPPORT_PLAN.mdcrates/rginx-app/src/admin_cli/cache.rscrates/rginx-app/src/admin_cli/mod.rscrates/rginx-app/src/admin_cli/snapshot.rscrates/rginx-app/src/admin_cli/socket.rscrates/rginx-app/src/admin_cli/status.rscrates/rginx-app/src/admin_cli/status/runtime.rscrates/rginx-app/src/cli.rscrates/rginx-app/src/cli/tests.rscrates/rginx-app/src/main.rscrates/rginx-app/tests/access_log.rscrates/rginx-app/tests/admin/commands.rscrates/rginx-app/tests/admin/commands/cache.rscrates/rginx-app/tests/admin/commands/status.rscrates/rginx-app/tests/admin/delta_wait/delta.rscrates/rginx-app/tests/admin/snapshot/core.rscrates/rginx-app/tests/reload.rscrates/rginx-app/tests/reload/cache.rscrates/rginx-core/src/config/access_log.rscrates/rginx-core/src/config/tests.rscrates/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/access_log.rscrates/rginx-http/src/handler/dispatch/mod.rscrates/rginx-http/src/handler/tests/observability.rscrates/rginx-http/src/lib.rscrates/rginx-http/src/proxy/forward/attempt.rscrates/rginx-http/src/proxy/forward/cache.rscrates/rginx-http/src/proxy/forward/success.rscrates/rginx-http/src/state/cache.rscrates/rginx-http/src/state/counters/versions.rscrates/rginx-http/src/state/helpers.rscrates/rginx-http/src/state/lifecycle/reload.rscrates/rginx-http/src/state/lifecycle/status.rscrates/rginx-http/src/state/mod.rscrates/rginx-http/src/state/snapshot_bus.rscrates/rginx-http/src/state/snapshots/cache.rscrates/rginx-http/src/state/snapshots/delta.rscrates/rginx-http/src/state/snapshots/mod.rscrates/rginx-http/src/state/snapshots/runtime.rscrates/rginx-http/src/state/tests/snapshots.rscrates/rginx-http/src/state/tests/status.rscrates/rginx-http/src/state/tests/support.rscrates/rginx-runtime/src/admin/mod.rscrates/rginx-runtime/src/admin/model.rscrates/rginx-runtime/src/admin/service.rscrates/rginx-runtime/src/bootstrap/mod.rscrates/rginx-runtime/src/bootstrap/shutdown.rscrates/rginx-runtime/src/bootstrap/shutdown/tests.rscrates/rginx-runtime/src/cache.rscrates/rginx-runtime/src/lib.rsscripts/install.sh
📜 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-app/tests/admin/commands/status.rs
🪛 GitHub Actions: CI
crates/rginx-app/tests/reload.rs
[error] 1-1: cargo fmt --all --check failed. Formatting differences detected in #[path] module attribute ordering.
crates/rginx-app/src/admin_cli/status/runtime.rs
[error] 1-1: cargo fmt --all --check failed. Formatting differences detected in cache entry count expression formatting.
crates/rginx-runtime/src/bootstrap/mod.rs
[error] 1-1: cargo fmt --all --check failed. Formatting differences detected in tokio::spawn assignment line wrapping.
crates/rginx-http/src/proxy/forward/cache.rs
[error] 8-11: Clippy error: large size difference between enum variants (clippy::large_enum_variant). Entire enum is at least 504 bytes.
crates/rginx-http/src/lib.rs
[error] 1-1: cargo fmt --all --check failed. Formatting differences detected in pub use re-export list wrapping.
crates/rginx-core/src/config/access_log.rs
[error] 1-1: modularization-gate failed: new production soft-limit violation (file has 303 lines; soft limit is 300).
crates/rginx-http/src/state/tests/snapshots.rs
[error] 1-1: cargo fmt --all --check failed. Formatting differences detected in match expression indentation/line wrapping for cache lookup.
crates/rginx-http/src/state/tests/status.rs
[error] 1-1: cargo fmt --all --check failed. Formatting differences detected in match expression indentation/line wrapping for cache lookup.
crates/rginx-core/src/config/tests.rs
[error] 1-1: cargo fmt --all --check failed. Formatting differences detected in test AccessLogFormat::parse assignment line wrapping.
crates/rginx-runtime/src/admin/service.rs
[error] 1-1: cargo fmt --all --check failed. Formatting differences detected in AdminRequest::GetCacheStats match arm braces/line wrapping.
crates/rginx-app/src/cli/tests.rs
[error] 1-1: cargo fmt --all --check failed. Formatting differences detected in test CLI parsing initialization line wrapping.
crates/rginx-http/src/proxy/forward/success.rs
[error] 47-53: Clippy error: this if statement can be collapsed (clippy::collapsible-if). -D clippy::collapsible-if implied by -D warnings.
crates/rginx-http/src/cache/tests.rs
[error] 1-1: modularization-gate failed: new test soft-limit violation (file has 431 lines; soft limit is 400).
crates/rginx-http/src/state/snapshot_bus.rs
[error] 1-1: cargo fmt --all --check failed. Formatting differences detected in changed_named_component_targets_since call wrapping.
crates/rginx-app/src/admin_cli/cache.rs
[error] 1-1: cargo fmt --all --check failed. Formatting differences detected (Rust code not rustfmt-formatted). Example diffs include line wrapping in AdminRequest enum construction and signature formatting, plus reordering/combining struct literal fields and expression formatting.
crates/rginx-app/tests/admin/commands/cache.rs
[error] 1-1: cargo fmt --all --check failed. Formatting differences detected in format! strings passed to send_raw_request.
crates/rginx-http/src/cache/policy.rs
[error] 1-1: cargo fmt --all --check failed. Formatting differences detected in split/map/filter/all call chain indentation.
crates/rginx-http/src/state/mod.rs
[error] 1-1: cargo fmt --all --check failed. Formatting differences detected in pub use ordering and re-export wrapping.
crates/rginx-http/src/cache/entry.rs
[error] 51-61: Clippy error: this function has too many arguments (9/8). -D clippy::too-many-arguments implied by -D warnings.
crates/rginx-http/src/cache/store.rs
[error] 1-1: cargo fmt --all --check failed. Formatting differences detected in argument ordering/wrapping for freshness computations and error mapping.
crates/rginx-http/src/cache/mod.rs
[error] 1-1: cargo fmt --all --check failed. Formatting differences detected across multiple sections (use statements order, match/if formatting, snapshot collection formatting, expression wrapping).
🔇 Additional comments (49)
crates/rginx-http/src/state/tests/support.rs (1)
141-155: 实现符合预期。这个测试辅助函数把 cache zone 注入逻辑集中封装，便于复用且行为可预测。
crates/rginx-http/src/proxy/forward/cache.rs (1)
47-58: 新增的条件请求头与 stale-on-error 封装清晰。空 store 的短路和可服务性判断都处理得很干净。
crates/rginx-runtime/src/lib.rs (1)
3-3: 模块接线正确。新增
mod cache;与本次运行时缓存维护任务的引入保持一致。crates/rginx-runtime/src/admin/mod.rs (1)
17-17: Schema 版本升级合理。与 admin snapshot 新增 cache 字段保持同步，兼容性语义正确。
crates/rginx-http/src/state/counters/versions.rs (1)
8-8: 新增 cache 组件版本计数是正确扩展。这与快照/增量链路新增 cache 模块保持一致。
crates/rginx-app/src/admin_cli/snapshot.rs (1)
84-84: 模块映射补全到位。
SnapshotModuleArg::Cache的转换路径正确接入了底层SnapshotModule::Cache。crates/rginx-app/tests/admin/commands/status.rs (1)
24-25: 断言补充有效。新增检查覆盖了
status输出中的 cache 汇总与分段记录，能更早发现回归。crates/rginx-app/src/admin_cli/status.rs (1)
1-1: 缓存状态已正确接入状态输出链路。Line 1 与 Line 17 的改动把 cache 状态打印接入
status命令主流程，结构清晰且与现有分段输出风格一致。Also applies to: 17-17
crates/rginx-http/src/state/snapshots/mod.rs (1)
2-2: 快照模块导出补齐合理。Line 2、Line 12 将 cache 快照纳入统一导出面，和现有 snapshots 组织方式保持一致。
Also applies to: 12-12
crates/rginx-app/tests/admin/snapshot/core.rs (1)
43-43: 快照版本与 cache 断言更新完整。Line 43/69 与 Line 62/83 同步覆盖了结构化响应和 JSON 输出，对 schema 升级与 cache 字段引入的回归保护到位。
Also applies to: 62-62, 69-69, 83-83
crates/rginx-app/tests/admin/commands.rs (1)
1-2: 命令测试模块扩展清晰。Line 1-2 将 cache 命令测试接入总入口，便于与现有命令测试并行回归。
crates/rginx-app/tests/admin/delta_wait/delta.rs (1)
32-32: Delta 的 cache 维度断言补充到位。这些改动同时校验了类型化返回与 JSON 输出中的 cache 变更信号，能有效防止 delta schema 回归。
Also applies to: 42-42, 65-65, 79-79, 87-87, 110-110
crates/rginx-core/src/config/tests.rs (1)
256-256:cache_status变量测试覆盖补充有效。你已同时补齐结构体初始化字段与独立渲染用例，能覆盖
$cache_status的基本渲染路径。Also applies to: 309-309, 354-354, 363-403
scripts/install.sh (1)
358-359: 缓存根目录初始化顺序与权限收敛实现合理先创建服务账号，再以
750+rginx:rginx初始化/var/cache/rginx，并输出安装日志，符合最小权限原则。Also applies to: 427-429, 436-436
crates/rginx-http/src/state/snapshots/runtime.rs (1)
6-7: 运行时快照新增cache字段与类型引入一致这里的模型扩展清晰，和状态聚合侧（构建
CacheStatsSnapshot）可以直接对齐。Also applies to: 35-35
crates/rginx-app/src/admin_cli/socket.rs (1)
39-39: 新增 admin 响应类型映射完整
cache_stats与cache_purge被纳入统一 kind 映射后，错误提示路径更可观测。Also applies to: 44-44
crates/rginx-app/src/main.rs (1)
64-65: 主流程分支覆盖了新增缓存管理子命令
Command::Cache与Command::PurgeCache(_)被纳入“运行时初始化前返回”的管理命令集合，路由一致性正确。crates/rginx-http/src/proxy/forward/attempt.rs (1)
12-12: 条件请求头注入时机正确在转发构建前注入条件头，能让上游重验证复用缓存元数据，路径选择合理。
Also applies to: 27-27
crates/rginx-http/src/handler/dispatch/mod.rs (1)
202-207:x-cache到访问日志上下文的透传实现完整从最终响应读取缓存状态并同时接入 gRPC 与普通访问日志上下文，链路闭环做得很好。
Also applies to: 237-237, 274-274
crates/rginx-http/src/state/snapshots/cache.rs (1)
1-8: 缓存统计快照模型定义简洁且可序列化
CacheStatsSnapshot仅暴露zones，作为跨层传输结构体足够直接。crates/rginx-http/src/state/tests/status.rs (1)
269-314: 缓存区状态快照测试覆盖到位这个用例从 miss→store→status snapshot 的链路验证了
entry_count与current_size_bytes，对新功能回归很有价值。crates/rginx-runtime/src/bootstrap/mod.rs (1)
101-112: LGTM!缓存任务正确集成到优雅关闭流程中，与现有的 admin、health、ocsp 任务遵循一致的模式。
crates/rginx-http/src/state/lifecycle/status.rs (1)
16-23: LGTM!缓存快照正确地在持有读锁期间构建，类型与
CacheStatsSnapshot和CacheManager::snapshot()的定义一致。这是对现有快照模式的干净扩展。crates/rginx-http/src/cache/load.rs (1)
146-157: LGTM!新增的 stale 相关字段正确传播到
CacheIndexEntry结构体。crates/rginx-http/src/state/lifecycle/reload.rs (3)
84-90: LGTM!缓存通知器的构建遵循与
peer_health_notifier相同的模式，正确集成到配置替换准备阶段。
122-122: LGTM!
sync_cache_versions在提交阶段的正确位置调用，与其他同步操作（sync_peer_health_versions、sync_upstream_stats）保持一致。
136-152: LGTM!缓存区域变更标记和快照组件更新正确集成到 reload 提交流程中。
crates/rginx-runtime/src/bootstrap/shutdown/tests.rs (2)
93-118: LGTM!测试正确扩展以覆盖
cache_task的优雅关闭场景，与其他后台任务的测试模式一致。
136-162: LGTM!超时中止测试正确验证了
cache_task在超时后被中止并设为None。crates/rginx-http/src/handler/access_log.rs (5)
27-27: LGTM!
cache_status字段以Option<&'a str>类型正确添加到AccessLogContext，与其他可选字段保持一致。
49-49: LGTM!
OwnedAccessLogContext中的cache_status字段与借用版本类型对应。
76-76: LGTM!
as_borrowed方法正确传播cache_status。
125-125: LGTM!结构化日志输出正确包含
cache_status，缺失时默认为"-"，与其他可选字段处理一致。
195-195: LGTM!
cache_status正确传递给AccessLogValues用于格式化输出。crates/rginx-app/tests/access_log.rs (2)
70-98: LGTM!测试逻辑清晰完整：验证
x-cache响应头从MISS变为HIT，确认上游只被请求一次，并验证访问日志包含正确的缓存状态。
118-130: LGTM!配置生成器函数正确设置了缓存区域、代理上游和访问日志格式，覆盖了缓存状态日志测试所需的所有配置。
crates/rginx-http/src/lib.rs (1)
28-41: 无法验证该条评论。文件crates/rginx-http/src/lib.rs的当前代码看起来格式完整。但由于沙箱环境中无法配置 Rust 工具链运行cargo fmt命令，无法确认是否存在实际的格式化差异。建议在本地环境或 CI 流水线中运行cargo fmt --all --check来确认是否需要格式修复。crates/rginx-app/src/admin_cli/mod.rs (1)
47-54: 缓存命令路由实现一致，LGTM
Cache与PurgeCache分支复用既有 admin 命令执行模式，错误传播与返回语义统一。crates/rginx-app/src/cli.rs (1)
78-89:purge-cache参数建模合理，LGTM
zone+ 可选 selector（key/prefix）的结构与命令语义一致，互斥约束也已在类型层声明。crates/rginx-runtime/src/cache.rs (1)
9-31: 后台清理任务实现完整，LGTM周期调度、shutdown 监听和退出日志都具备，任务生命周期处理清晰。
crates/rginx-runtime/src/bootstrap/shutdown.rs (1)
21-62:cache_task已正确接入关停编排，LGTM你把 cache 任务同时纳入了正常 join 和超时 abort-after-join 流程，和现有后台任务行为保持一致。
crates/rginx-app/tests/admin/commands/cache.rs (1)
9-79: 这条集成链路覆盖得很好。同一个测试里把
MISS -> HIT、cache管理命令，以及purge-cache后再次回源都串起来了，能直接兜住这次 CLI/admin 接线的关键回归。crates/rginx-app/tests/reload/cache.rs (1)
9-49: 这个回归点很关键。它验证了 reload 复用同一路径的 cache zone 时不会把已存在条目打回
MISS，正好覆盖了这次生命周期改动里最容易退化的行为。crates/rginx-http/src/state/snapshot_bus.rs (1)
198-245: 保留旧入口语义这点很好。
mark_snapshot_changed_components()现在只是委托到带cache标志的新实现，新增 cache 维度时没有破坏现有调用方，扩展路径比较稳。crates/rginx-http/src/state/snapshots/delta.rs (1)
72-89:ALL和normalize()同步更新是对的。这样
all()、normalize(None)和显式过滤的默认行为保持一致，不会把Cache变成“只有某些入口才会返回”的特殊模块。crates/rginx-http/src/state/cache.rs (1)
52-58: 版本同步策略看起来合理。
build_cache_zone_versions()只为当前配置里的 zone 继承旧版本，sync_cache_versions()再整体替换映射；这能在 reload 后保留已有 zone 的版本连续性，同时把已删除的 zone 清掉。Also applies to: 80-92
crates/rginx-http/src/cache/policy.rs (1)
32-34: 缓存键已正确区分 Accept-Encoding 变体，原有的主要关切不成立。查证发现
render_cache_key()在第 51-54 行显式将normalized_accept_encoding()并入缓存键，使用|ae:前缀区分不同的编码请求。因此不同的Accept-Encoding值会生成不同的缓存键，gzip/br/identity 等不同表示体不会共用同一对象。vary_is_supported()的逻辑也是正确的——其设计目的就是仅允许Accept-Encoding作为 Vary 维度。注：
request_requires_revalidation()确实缺少Pragma: no-cache检查（仅检查 Cache-Control），但这是独立的 HTTP/1.0 兼容性问题，与 Vary 处理无关。crates/rginx-http/src/cache/entry.rs (2)
22-36: 向后兼容处理正确。新增缓存字段都带了
#[serde(default)]，可兼容历史 metadata 文件；Clone派生也便于后续在重验证路径复用对象。Also applies to: 39-43
75-83: 读/构建/写入路径拆分清晰，且错误传播完整。
read_cache_metadata、build_cached_response、write_cache_metadata与headers_map()的职责边界明确，InvalidData映射也一致，整体可维护性有明显提升。Also applies to: 85-112, 114-118, 151-169, 197-209
|
Addressed the current review feedback and CI failures in Handled the cache revalidation and metadata-key validation issues, stale-on-error metric ordering, cleanup/purge notifications, snapshot notifier churn on cache counters, split the oversized cache tests module, and cleaned up the clippy / format failures. Local verification:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0836a20dd9
ℹ️ 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: 4
♻️ Duplicate comments (3)
crates/rginx-app/src/cli/tests.rs (1)
179-215:⚠️ Potential issue | 🟡 Minor补一条
--key与--prefix同时传入时应失败的测试。当前只覆盖了
purge-cache的两个正向解析分支，ArgGroup的互斥约束没有回归保护，后续容易被改坏。💡 建议补充的测试
+#[test] +fn cli_rejects_purge_cache_with_both_key_and_prefix() { + let parsed = Cli::try_parse_from([ + "rginx", + "purge-cache", + "--zone", + "default", + "--key", + "k1", + "--prefix", + "p1", + ]); + assert!(parsed.is_err(), "cli should reject --key with --prefix"); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-app/src/cli/tests.rs` around lines 179 - 215, Add a new unit test that verifies the mutual-exclusion of --key and --prefix for the purge-cache subcommand: create a test (e.g., cli_rejects_both_key_and_prefix_subcommand) that calls Cli::try_parse_from with ["rginx","purge-cache","--zone","default","--key","http:example.com:/demo","--prefix","http:example.com:/assets/"] and assert that parsing fails (use is_err() or expect_err). This ensures the ArgGroup/validation on Cli / Command::PurgeCache / PurgeCacheArgs rejects providing both --key and --prefix simultaneously.crates/rginx-app/tests/admin/commands/cache.rs (1)
75-125: 🧹 Nitpick | 🔵 Trivial建议将测试 helper 抽到共享模块，避免后续漂移。
这里的
cached_proxy_config、spawn_counting_response_server、response_header_value与其他缓存测试场景有重复，实现分散会增加维护成本。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-app/tests/admin/commands/cache.rs` around lines 75 - 125, This test defines helpers (cached_proxy_config, spawn_counting_response_server, response_header_value and READY_ROUTE_CONFIG) that are duplicated across cache tests; extract them into a shared test helper module (e.g., crates::rginx_app::tests::helpers or a new test util crate) as public test-only functions/constants, replace the local definitions with use/imports of those helpers, update references in this file to call the shared functions (keeping signatures the same) and remove the duplicated local implementations to avoid divergence.crates/rginx-http/src/cache/policy.rs (1)
91-94:⚠️ Potential issue | 🟡 Minor补上
Pragma: no-cache的兼容判断。Line 91 这里只看了
Cache-Control。只发送Pragma: no-cache的旧客户端/中间代理请求仍会被当成可直接命中，跳过本应发生的重验证。建议修改
pub(super) fn request_requires_revalidation(headers: &HeaderMap) -> bool { cache_control_contains(headers, &["no-cache"]) + || headers + .get(http::header::PRAGMA) + .and_then(|value| value.to_str().ok()) + .is_some_and(|value| { + value + .split(',') + .map(str::trim) + .any(|directive| directive.eq_ignore_ascii_case("no-cache")) + }) || cache_control_duration(headers, "max-age") == Some(Duration::ZERO) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/cache/policy.rs` around lines 91 - 94, The revalidation check in request_requires_revalidation currently only inspects Cache-Control; add compatibility for legacy Pragma by treating a Pragma header containing "no-cache" as requiring revalidation. Update request_requires_revalidation (which calls cache_control_contains/cache_control_duration) to also check headers.get("Pragma") (case-insensitive) for "no-cache" and return true when present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/rginx-http/src/cache/mod.rs`:
- Around line 548-576: stale_response_from_entry currently uses `.ok()?` which
causes lookup() to spin on corrupted/missing metadata/body; change it to handle
read errors explicitly: when read_cache_metadata or build_cached_response
returns Err, acquire the same io_lock and perform the same cleanup as
load_lookup_metadata (call remove_index_entry(zone, key) and await
remove_cache_files_if_unindexed(zone, key, &entry.hash)) and then return None
(or, better, transition this request to the existing fill-wait path used by
lookup() instead of continuing to spin). Locate stale_response_from_entry,
lookup(), load_lookup_metadata(), remove_index_entry and
remove_cache_files_if_unindexed to implement this error branch so corrupt stale
entries are cleaned and/or converted to waiter behavior rather than causing
busy-waiting.
In `@crates/rginx-http/src/cache/policy.rs`:
- Around line 61-74: The current cache policy wrongly rejects responses with
unknown/None upper bounds; change the logic in the function that evaluates body
sizes so only responses with a known upper bound exceeding
context.zone.config.max_entry_bytes are rejected. Specifically, update the check
that uses body_size.upper (the matches! block) to only return false when
Some(upper) > max_entry_bytes (not when upper is None), so responses with
unknown length remain eligible and will be size-checked later in
store_response(); keep the existing parse_content_length(...) and
body_size.exact checks as-is.
In `@crates/rginx-http/src/cache/tests/lookup.rs`:
- Around line 141-185: The test name
cache_manager_removes_expired_entries_on_lookup contradicts its assertions which
check that the index entry and on-disk files are still present after lookup;
rename the test to reflect the actual behavior (e.g.,
cache_manager_retains_expired_entries_for_revalidation_on_lookup or
cache_manager_marks_expired_but_keeps_on_disk_on_lookup) and update any
references to the test name; ensure the test function identifier
(cache_manager_removes_expired_entries_on_lookup) and any documentation/comments
around the test are changed to the new name so it no longer implies deletion,
while keeping the existing lookup call (manager.lookup(...)) and the final
assertions intact.
In `@crates/rginx-http/src/proxy/forward/success.rs`:
- Around line 22-52: The 304 revalidation and stale-on-error branches return
cached responses directly, which bypasses build_downstream_response and causes
active_peer to be dropped too early, loses IdleTimeoutBody/GrpcDeadlineBody
wrapping and skips sanitize_response_headers; update the code in the match for
cache_manager.complete_not_modified(...) and the branch that returns stale from
cache_store.serve_stale_on_error() to route the cached response through
build_downstream_response (or alternatively ensure active_peer is held until the
body is fully consumed, re-wrap the body with IdleTimeoutBody/GrpcDeadlineBody
and run sanitize_response_headers on the response headers) so that active_peer
lifecycle, timeout wrappers, and header sanitization are preserved.
---
Duplicate comments:
In `@crates/rginx-app/src/cli/tests.rs`:
- Around line 179-215: Add a new unit test that verifies the mutual-exclusion of
--key and --prefix for the purge-cache subcommand: create a test (e.g.,
cli_rejects_both_key_and_prefix_subcommand) that calls Cli::try_parse_from with
["rginx","purge-cache","--zone","default","--key","http:example.com:/demo","--prefix","http:example.com:/assets/"]
and assert that parsing fails (use is_err() or expect_err). This ensures the
ArgGroup/validation on Cli / Command::PurgeCache / PurgeCacheArgs rejects
providing both --key and --prefix simultaneously.
In `@crates/rginx-app/tests/admin/commands/cache.rs`:
- Around line 75-125: This test defines helpers (cached_proxy_config,
spawn_counting_response_server, response_header_value and READY_ROUTE_CONFIG)
that are duplicated across cache tests; extract them into a shared test helper
module (e.g., crates::rginx_app::tests::helpers or a new test util crate) as
public test-only functions/constants, replace the local definitions with
use/imports of those helpers, update references in this file to call the shared
functions (keeping signatures the same) and remove the duplicated local
implementations to avoid divergence.
In `@crates/rginx-http/src/cache/policy.rs`:
- Around line 91-94: The revalidation check in request_requires_revalidation
currently only inspects Cache-Control; add compatibility for legacy Pragma by
treating a Pragma header containing "no-cache" as requiring revalidation. Update
request_requires_revalidation (which calls
cache_control_contains/cache_control_duration) to also check
headers.get("Pragma") (case-insensitive) for "no-cache" and return true when
present.
🪄 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: 96301230-d4c6-4c73-a7f9-e32ec8bf81b9
📒 Files selected for processing (31)
crates/rginx-app/src/admin_cli/cache.rscrates/rginx-app/src/admin_cli/status/runtime.rscrates/rginx-app/src/cli/tests.rscrates/rginx-app/tests/admin/commands/cache.rscrates/rginx-app/tests/reload.rscrates/rginx-core/src/config/tests.rscrates/rginx-http/src/cache/entry.rscrates/rginx-http/src/cache/mod.rscrates/rginx-http/src/cache/policy.rscrates/rginx-http/src/cache/store.rscrates/rginx-http/src/cache/tests.rscrates/rginx-http/src/cache/tests/lookup.rscrates/rginx-http/src/cache/tests/mod.rscrates/rginx-http/src/cache/tests/notifications.rscrates/rginx-http/src/cache/tests/policy.rscrates/rginx-http/src/cache/tests/storage.rscrates/rginx-http/src/handler/tests/observability.rscrates/rginx-http/src/lib.rscrates/rginx-http/src/proxy/forward/attempt.rscrates/rginx-http/src/proxy/forward/cache.rscrates/rginx-http/src/proxy/forward/success.rscrates/rginx-http/src/state/cache.rscrates/rginx-http/src/state/lifecycle/reload.rscrates/rginx-http/src/state/mod.rscrates/rginx-http/src/state/snapshot_bus.rscrates/rginx-http/src/state/tests/snapshots.rscrates/rginx-http/src/state/tests/status.rscrates/rginx-runtime/src/admin/service.rscrates/rginx-runtime/src/bootstrap/mod.rscrates/rginx-runtime/src/bootstrap/shutdown.rscrates/rginx-runtime/src/bootstrap/shutdown/tests.rs
💤 Files with no reviewable changes (1)
- crates/rginx-http/src/cache/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-core/src/config/tests.rscrates/rginx-http/src/cache/tests/policy.rscrates/rginx-http/src/proxy/forward/attempt.rscrates/rginx-http/src/state/tests/status.rscrates/rginx-http/src/cache/policy.rscrates/rginx-http/src/cache/mod.rs
🔇 Additional comments (27)
crates/rginx-app/tests/reload.rs (1)
27-28: 模块引入位置正确，可以接受。
#[path = "reload/cache.rs"] mod cache;与当前测试目录结构一致，能把新的 cache/reload 用例纳入现有reload.rs测试入口。crates/rginx-core/src/config/tests.rs (2)
224-257: 实现对齐，没问题。这里补上的
cache_status与AccessLogValues新字段一致，不会影响原有 access-log 断言。
363-404: 新增用例覆盖到位。
$cache_status的解析和渲染都被单独验证了，和核心实现保持一致。crates/rginx-http/src/handler/tests/observability.rs (1)
5-50: 这条链路已经闭环。模板、上下文和最终断言都覆盖了
cache_status，能够直接验证$cache_status的渲染结果。crates/rginx-runtime/src/bootstrap/mod.rs (2)
8-8: 后台缓存任务接入得很一致。新增
cache导入并在与admin/health/ocsp相同的位置启动后台任务，生命周期和现有任务保持一致，没有看到问题。Also applies to: 53-54
108-113: ShutdownTasks 传递方式是对的。把
cache_task纳入 bundle 后，graceful_shutdown的调用面更干净，也避免继续膨胀参数列表。crates/rginx-runtime/src/bootstrap/shutdown/tests.rs (2)
93-118: 测试覆盖已同步到新后台任务。
cache_task的完成态和is_none()断言都补上了，能覆盖正常 shutdown 路径。
138-164: 超时路径也覆盖到了cache_task。这能验证 abort/join 后句柄会被清理掉，和其他后台任务行为一致。
crates/rginx-runtime/src/bootstrap/shutdown.rs (2)
14-29: ShutdownTasks 结构化收敛是合理的。这个 bundle 让 shutdown 调用方只传一组任务句柄，后续如果再扩展后台任务也更容易维护。
34-68:cache_task的 join/abort 路径补得完整。正常和超时分支都按同样语义处理了新任务，和 admin/health/ocsp 的行为保持一致。
crates/rginx-app/src/cli/tests.rs (2)
126-136: 实现看起来正确。
snapshot --include cache的解析结果与SnapshotModuleArg::Cache/Command::Snapshot的形状一致，没有看到问题。
172-177: 实现看起来正确。
cache子命令已经正确映射到Command::Cache，和 CLI 路由定义保持一致。crates/rginx-app/src/admin_cli/status/runtime.rs (1)
122-136: 实现看起来正确。新增的
cache_zones、cache_entries和cache_current_size_bytes汇总逻辑与缓存运行时快照字段一致，统计口径清晰。crates/rginx-http/src/proxy/forward/attempt.rs (1)
232-239: 修正方向正确。两个错误分支现在都先尝试返回 stale 响应，再回退到 502/504 和 504 计数，避免成功返回 stale 时污染错误指标。
Also applies to: 283-287
crates/rginx-http/src/lib.rs (1)
28-39: 实现看起来正确。新增的 cache 相关类型已经通过顶层
pub use暴露，和下游对rginx_http公共 API 的使用方式一致。crates/rginx-http/src/state/lifecycle/reload.rs (1)
84-90: 实现看起来正确。缓存 notifier、版本同步和 cache zone 变更标记都接入到了 reload 流程里，整体链路和现有的快照变更跟踪保持一致。
Also applies to: 122-123, 136-155
crates/rginx-http/src/state/tests/status.rs (1)
269-313: 这条回归测试是有价值的。它把
lookup -> store_response -> status_snapshot的链路串起来了，能直接覆盖新增的 cache 统计快照。crates/rginx-runtime/src/admin/service.rs (1)
122-140: 实现看起来正确。
GetSnapshot里按需附带 cache stats，GetCacheStats和三个 purge 请求也都正确路由到SharedState，和新增的 admin 协议一致。Also applies to: 162-164, 179-196
crates/rginx-http/src/cache/tests/policy.rs (1)
7-24: 这条 TTL 回归测试覆盖得很完整。
max-age=0、s-maxage优先级、无效max-age回退以及过期Expires的场景都被覆盖到了，和 freshness 逻辑对得上。crates/rginx-http/src/state/tests/snapshots.rs (1)
129-182: 缓存快照与增量链路测试覆盖完整，设计合理。这段测试把缓存写入后的统计与
SnapshotModule::Cache增量联动都校验到了，回归价值很高。crates/rginx-http/src/cache/tests/mod.rs (1)
17-113: 测试构造器抽象层次合适，可维护性提升明显。这些 helper 把 zone/policy/context 的装配收敛到一处，后续扩展缓存测试会更稳。
crates/rginx-http/src/state/mod.rs (1)
122-160: 缓存组件版本追踪接入完整，初始化链路清晰。
cache_component_versions的创建、notifier 注入与初始版本填充是闭环实现。crates/rginx-http/src/cache/tests/notifications.rs (1)
9-113: 通知语义测试边界定义准确。把“纯计数更新不通知”和“结构性变更会通知”拆开验证，并附带文件删除断言，这组测试很扎实。
crates/rginx-http/src/cache/tests/storage.rs (1)
14-167: 存储/重验证场景覆盖全面，回归价值高。尤其是
304 + no-store的驱逐与统计断言，能有效防止缓存一致性回退。crates/rginx-app/tests/admin/commands/cache.rs (1)
9-73: 命令级端到端验证路径完整，断言有效。覆盖了缓存命中、统计查询、按 key 清除以及清除后的回退行为，验证链路很完整。
crates/rginx-http/src/state/snapshot_bus.rs (2)
75-167: 快照增量已正确纳入缓存模块字段。
cache_version/cache_changed/changed_cache_zone_names的输出与模块过滤逻辑一致，schema_version=3也与新增字段匹配。
214-243: 组件变更打点 API 分层清晰。新增的
mark_snapshot_changed_components_with_cache让 cache 变更打点与旧接口兼容并存，设计上可读性不错。
|
补一轮 review follow-up，已推送 本次已处理：
本地已验证：
这轮我没有改两条建议：
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6512ef666
ℹ️ 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".
|
这轮剩余阻塞只来自 bot review 状态，不是代码或 CI 失败。 我这边的处理结论：
本地和远端必需检查均已通过；接下来我会把剩余 bot review 线程收口并合并。 |
vansour
dismissed
coderabbitai[bot]’s stale review
Remaining CodeRabbit review state is being dismissed to unblock merge: required CI checks passed, addressed findings were pushed in c6512ef, and non-adopted suggestions are documented on the PR as intentional design decisions.
Summary
$cache_statusaccess log supportTesting