feat(cache): tighten tail cache control semantics#73
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 (6)
📝 Walkthrough概览此次变更对缓存系统进行了重大增强，包括：引入 变更清单
序列图sequenceDiagram
participant Client
participant Manager as CacheManager
participant Lookup as lookup_decision
participant Policy as ResponseFreshness
participant Store as cache_store
participant Runtime as can_serve_stale
Client->>Manager: lookup(request, ...)
Manager->>Manager: compute request_forces_revalidation<br/>(from Cache-Control: no-cache)
Manager->>Lookup: lookup_decision(request, entry, policy)
alt Request bypasses cache
Lookup-->>Manager: Miss (no-store, If-Range detected)
else Fresh cache exists
Lookup->>Lookup: Check !requires_revalidation<br/>!request_forces_revalidation
alt Eligible for fresh hit
Lookup-->>Manager: Hit
Manager->>Client: Return cached response
else Revalidation needed
Lookup-->>Manager: Miss (Revalidated)
end
else Stale cache exists
Lookup->>Runtime: can_serve_stale(entry, policy,<br/>request_forces_revalidation)
Runtime->>Runtime: Check request/entry<br/>revalidation flags
alt Revalidation forced
Runtime-->>Lookup: false (deny stale)
Lookup-->>Manager: Miss
else Stale window valid
Runtime->>Runtime: Match status code<br/>against use_stale{403,404,429,...}
alt Condition matched
Runtime-->>Lookup: true (allow stale)
Lookup-->>Manager: Updating/BackgroundUpdate
else No match
Runtime-->>Lookup: false
Lookup-->>Manager: Miss
end
end
end
Client->>Manager: (After response received) store(response, ...)
Manager->>Policy: response_freshness(response)
Policy->>Policy: Extract no-cache → requires_revalidation<br/>Extract must-revalidate → must_revalidate
Policy-->>Manager: ResponseFreshness{requires_revalidation, must_revalidate, ...}
Manager->>Store: Persist requires_revalidation<br/>into CacheIndexEntry
Store-->>Manager: Index stored
代码审查工作量估计🎯 4 (Complex) | ⏱️ ~60 minutes 可能相关的PR
诗
🚥 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. Review rate limit: 0/1 reviews remaining, refill in 38 minutes and 19 seconds.Comment |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
The implementation correctly tightens cache control semantics by separating no-cache from must-revalidate directives, properly bypassing cache for request no-store and If-Range headers, and extending use_stale conditions. The changes follow HTTP caching specifications and maintain consistency across the codebase with comprehensive test coverage.
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 refines cache control semantics by separating no-cache from must-revalidate, extending stale-serving support to 403, 404, and 429 status codes, and implementing bypasses for no-store and If-Range headers. Feedback suggests narrowing the If-Range bypass logic to align with RFC 9110 and refactoring duplicate Cache-Control parsing logic into a shared utility.
There was a problem hiding this comment.
Pull request overview
This PR tightens HTTP cache control semantics in rginx-http by separating response no-cache from must-revalidate / proxy-revalidate, adding request-side bypass rules for no-store and If-Range, and extending stale-serve conditions (including 403/404/429) with new regression coverage.
Changes:
- Split cached-response freshness semantics into
requires_revalidation(no-cache) vsmust_revalidate(must-revalidate/proxy-revalidate) and propagate through metadata/index formats. - Bypass cache for request
Cache-Control: no-storeandIf-Range, and block stale serving/background updating on forced revalidation paths. - Extend
use_staleto includeHttp403/Http404/Http429and add tests for revalidation/background-update/stale-serve behavior.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/CACHE_ARCHITECTURE_GAPS.md | Documents the newly completed cache-control semantics tightening work. |
| crates/rginx-http/src/proxy/tests/mod.rs | Registers new proxy cache stale-serve regression test module. |
| crates/rginx-http/src/proxy/tests/cache_stale.rs | Adds proxy-level test ensuring stale serve can be configured for 429. |
| crates/rginx-http/src/cache/tests/policy.rs | Adds unit test verifying no-cache vs proxy-revalidate separation. |
| crates/rginx-http/src/cache/tests/mod.rs | Updates test helpers for new requires_revalidation + request flag fields. |
| crates/rginx-http/src/cache/tests/lookup/recovery.rs | Adds regression tests for fresh-hit vs revalidate behavior based on new flags. |
| crates/rginx-http/src/cache/tests/lookup/keys.rs | Adds tests for request no-store bypass and If-Range bypass. |
| crates/rginx-http/src/cache/tests/lookup/background.rs | Adds tests ensuring background update is disabled for forced revalidation / must-revalidate paths. |
| crates/rginx-http/src/cache/store/revalidate.rs | Preserves requires_revalidation when refreshing 304 Not Modified responses. |
| crates/rginx-http/src/cache/store/helpers.rs | Persists requires_revalidation in stored metadata and treats it as cacheable. |
| crates/rginx-http/src/cache/store.rs | Writes requires_revalidation into the cache index entry on store. |
| crates/rginx-http/src/cache/shared/index_file/codec.rs | Extends shared index record format to include requires_revalidation with backward-compatible defaulting. |
| crates/rginx-http/src/cache/runtime/support.rs | Adds stale condition matching for 403/404/429. |
| crates/rginx-http/src/cache/runtime.rs | Blocks stale serving when request forces revalidation or entry requires/must revalidate. |
| crates/rginx-http/src/cache/request.rs | Adds request-side bypass for no-store and If-Range plus directive parsing helper. |
| crates/rginx-http/src/cache/policy.rs | Implements new response freshness fields (requires_revalidation, must_revalidate) and updates parsing. |
| crates/rginx-http/src/cache/mod.rs | Extends cache context/index entry structs with new requires_revalidation and request flag. |
| crates/rginx-http/src/cache/manager.rs | Plumbs request forced-revalidation flag through lookup/store contexts. |
| crates/rginx-http/src/cache/lookup.rs | Prevents fresh hits and stale serving based on requires_revalidation / request forced revalidation. |
| crates/rginx-http/src/cache/load.rs | Loads requires_revalidation from on-disk metadata with compatibility fallback. |
| crates/rginx-http/src/cache/index.rs | Includes requires_revalidation in entry equality checks. |
| crates/rginx-http/src/cache/entry.rs | Adds metadata compatibility parsing for requires_revalidation when reading JSON metadata. |
| crates/rginx-core/src/config/cache.rs | Extends core CacheUseStaleCondition enum with 403/404/429 variants. |
| crates/rginx-config/src/model/cache.rs | Extends config model CacheUseStaleConditionConfig enum with 403/404/429 variants. |
| crates/rginx-config/src/compile/tests/cache.rs | Updates compile tests to cover new use_stale condition variants. |
| crates/rginx-config/src/compile/cache.rs | Compiles new use_stale enum variants into core representation. |
💡 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: 2
🤖 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/tests/cache.rs`:
- Around line 291-293: The test currently asserts that
CacheUseStaleConditionConfig includes Http404 and Http429 but misses Http403;
update the test in the cache.rs compile tests to include
crate::model::CacheUseStaleConditionConfig::Http403 alongside the existing
Http404 and Http429 assertions (same test block that references
CacheUseStaleConditionConfig::Http404 and ::Http429) so the new enum variant is
covered and future regressions are caught.
In `@crates/rginx-http/src/proxy/tests/cache_stale.rs`:
- Line 123: The test currently uses a fragile fixed wait
(tokio::time::sleep(Duration::from_millis(10)).await) which can flake under CI;
replace this timing-based assertion by making the cache entry deterministically
expired instead — e.g. set the cache TTL/expiry for the entry directly or
advance the Tokio test clock (tokio::time::pause/advance) or mutate the stored
entry timestamp so the cache thinks it is stale; locate the sleep call in the
cache_stale test (file crates/rginx-http/src/proxy/tests/cache_stale.rs) and
remove the fixed sleep, then trigger the expiration deterministically via the
cache TTL/expiry API or test clock so assertions no longer depend on real-time
delays.
🪄 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: 044a1111-1890-4c17-a7cf-732e2cb27702
📒 Files selected for processing (26)
crates/rginx-config/src/compile/cache.rscrates/rginx-config/src/compile/tests/cache.rscrates/rginx-config/src/model/cache.rscrates/rginx-core/src/config/cache.rscrates/rginx-http/src/cache/entry.rscrates/rginx-http/src/cache/index.rscrates/rginx-http/src/cache/load.rscrates/rginx-http/src/cache/lookup.rscrates/rginx-http/src/cache/manager.rscrates/rginx-http/src/cache/mod.rscrates/rginx-http/src/cache/policy.rscrates/rginx-http/src/cache/request.rscrates/rginx-http/src/cache/runtime.rscrates/rginx-http/src/cache/runtime/support.rscrates/rginx-http/src/cache/shared/index_file/codec.rscrates/rginx-http/src/cache/store.rscrates/rginx-http/src/cache/store/helpers.rscrates/rginx-http/src/cache/store/revalidate.rscrates/rginx-http/src/cache/tests/lookup/background.rscrates/rginx-http/src/cache/tests/lookup/keys.rscrates/rginx-http/src/cache/tests/lookup/recovery.rscrates/rginx-http/src/cache/tests/mod.rscrates/rginx-http/src/cache/tests/policy.rscrates/rginx-http/src/proxy/tests/cache_stale.rscrates/rginx-http/src/proxy/tests/mod.rsdocs/CACHE_ARCHITECTURE_GAPS.md
📜 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). (2)
- GitHub Check: Test Fast
- 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-http/src/cache/runtime.rscrates/rginx-http/src/proxy/tests/cache_stale.rscrates/rginx-config/src/compile/tests/cache.rs
🪛 LanguageTool
docs/CACHE_ARCHITECTURE_GAPS.md
[uncategorized] ~34-~34: 您的意思是“"不"齐”？
Context: ... 修复了“旧 entry 删除误删并发新写入文件”的竞态窗口 ### 已完成：补齐一轮 range / stale / revalidate / `...
(BU)
🔇 Additional comments (32)
crates/rginx-http/src/cache/mod.rs (2)
67-82: 重验证语义拆分清晰，字段位置合理。
request_forces_revalidation放在CacheStoreContext上，能明确表达“本次请求”维度的强制重验证状态，和条目元数据语义解耦得很好。
181-190: 索引条目增加requires_revalidation合理。将
requires_revalidation独立于must_revalidate持久化到索引条目，能避免语义混淆，后续命中/陈旧判定会更可控。crates/rginx-http/src/cache/request.rs (2)
33-39: 请求绕过规则补齐到位。对
Cache-Control: no-store和If-Range直接走 bypass，符合更严格的尾部缓存控制语义。
233-242:Cache-Control指令匹配实现稳健。支持多值头、逗号分隔指令、大小写无关匹配，并兼容
key=value形式，足以覆盖该场景的解析需求。crates/rginx-http/src/proxy/tests/mod.rs (1)
55-57: 测试模块接入正确。
cache_stale子模块已注册，新增的陈旧缓存回归测试会被纳入测试集。crates/rginx-http/src/cache/runtime/support.rs (1)
53-76: 状态码到use_stale条件映射完整。新增 403/404/429 的匹配分支实现直接、可读性好，和既有 5xx 处理风格一致。
crates/rginx-http/src/cache/index.rs (1)
43-53:stable_eq补充重验证字段比较是正确收敛。把
requires_revalidation纳入稳定等价条件后，索引更新与条件删除的判定一致性更强。crates/rginx-http/src/cache/store/helpers.rs (2)
16-30: 元数据透传正确补全。
freshness.requires_revalidation已写入CacheMetadataInput，确保后续索引与磁盘层能保留该语义。
58-63: 可缓存性判定调整合理。将
requires_revalidation视为可缓存条件，能够支持 TTL 为 0 但可用于重验证的响应。crates/rginx-config/src/compile/cache.rs (1)
242-254: 新增use_stale条件编译映射正确。
Http403、Http404、Http429的编译分支完整，和运行时状态匹配扩展保持一致。crates/rginx-http/src/cache/tests/mod.rs (1)
68-79: 测试夹具字段补齐完整且默认值合理。新增重验证相关字段均设置了显式默认值，能保证历史测试场景保持稳定并支持新语义覆盖。
Also applies to: 83-120, 158-167
crates/rginx-http/src/cache/runtime.rs (1)
83-86: 重验证闸门收紧实现正确。这里在请求强制重验证或条目声明需重验证时统一禁止 stale，语义清晰，符合本次改动目标。
crates/rginx-core/src/config/cache.rs (1)
75-77:use_stale条件枚举扩展合理。新增
Http403、Http404、Http429与本次策略扩展方向一致。crates/rginx-http/src/cache/store.rs (1)
148-149: 索引持久化字段补齐到位。
requires_revalidation已在 store 路径写入CacheIndexEntry，与其他模块的读取/判定逻辑一致。crates/rginx-config/src/model/cache.rs (1)
95-97: 配置模型枚举扩展与核心层保持一致。新增状态码条件定义清晰，便于配置侧表达更细粒度
use_stale策略。crates/rginx-http/src/cache/tests/lookup/keys.rs (2)
146-176:no-store绕过缓存测试覆盖到位。该用例能稳定保护请求头触发 bypass 的行为，回归价值高。
313-344:If-Range触发 range-cache bypass 的测试设计正确。该测试准确锚定了范围请求的强制绕过分支。
crates/rginx-http/src/cache/store/revalidate.rs (1)
42-43: 304 重验证路径的requires_revalidation传播完整。字段从 metadata 构建到 index 更新都已打通，语义一致性很好。
Also applies to: 126-127
crates/rginx-http/src/cache/load.rs (1)
29-31: 磁盘加载兼容策略实现稳健。
Option<bool>+unwrap_or(must_revalidate)的回退方式能兼容旧缓存元数据并保持保守重验证语义。Also applies to: 157-157, 176-176
crates/rginx-http/src/cache/tests/policy.rs (1)
50-68: 测试覆盖语义分离准确。这组断言清晰验证了
no-cache只触发requires_revalidation，而proxy-revalidate只触发must_revalidate，与目标行为一致。crates/rginx-http/src/cache/tests/lookup/background.rs (1)
54-95: 负向路径测试设计到位。新增用例准确锁定了“强制 revalidate / must-revalidate 下不得走
UPDATING”这一关键约束，回归保护价值很高。Also applies to: 97-151
crates/rginx-http/src/cache/tests/lookup/recovery.rs (1)
226-270: 恢复路径回归测试完整且语义清晰。这两条用例把
requires_revalidation与must_revalidate在 fresh 场景下的差异行为验证得很明确。Also applies to: 272-316
crates/rginx-http/src/cache/manager.rs (1)
78-89:request_forces_revalidation透传实现正确。你把该信号统一注入到所有非 FreshHit 的存储上下文路径，语义闭环完整。
Also applies to: 159-174, 188-203, 247-263
crates/rginx-http/src/cache/policy.rs (1)
24-30: 响应新鲜度语义拆分实现合理。
requires_revalidation与must_revalidate的拆分清晰，且保留了 ignore-header 语义门控。Also applies to: 100-119
crates/rginx-http/src/cache/shared/index_file/codec.rs (1)
21-23: 索引编解码兼容策略设计稳妥。新增字段持久化与 legacy 回退逻辑完整，兼顾了语义演进和历史数据兼容。
Also applies to: 56-58, 103-104, 139-140, 167-168
crates/rginx-http/src/proxy/tests/cache_stale.rs (1)
73-139: 429 stale 回退场景覆盖很好。该测试把
use_stale = Http429的端到端行为（MISS→STALE）验证完整，回归价值明确。docs/CACHE_ARCHITECTURE_GAPS.md (1)
34-48: 文档更新与实现语义对齐良好。这段对长尾控制项收敛结果的描述准确，能有效降低后续维护中的语义歧义。
crates/rginx-http/src/cache/entry.rs (3)
43-45:#[serde(default)]用在新字段上是正确的兼容性处理。旧缓存元数据缺少
requires_revalidation时仍可安全反序列化，升级后不会因字段缺失导致读取失败。
49-70:RawCacheMetadata过渡反序列化方案设计合理。Line 190 的
raw.requires_revalidation.unwrap_or(raw.must_revalidate)能平滑兼容历史元数据，同时保持既有must_revalidate语义。Also applies to: 176-193
99-100:requires_revalidation在写入链路中已完整透传。从
CacheMetadataInput到CacheMetadata的映射闭环完整，避免了新语义在持久化路径中丢失。Also applies to: 127-128
crates/rginx-http/src/cache/lookup.rs (2)
25-30: Fresh-hit 条件切换为requires_revalidation符合语义拆分目标。同时保留
request_forces_revalidation的阻断，命中路径与新缓存控制语义一致。
36-41: 陈旧响应判定已统一复用stale_allowed_for_entry，且重验证约束完整。Line 279-281 一次性拦截
request_forces_revalidation、requires_revalidation、must_revalidate，并在各等待/更新分支复用，降低语义漂移风险。Also applies to: 65-70, 79-85, 273-283
Summary
no-cachefrommust-revalidate/proxy-revalidatecache semanticsCache-Control: no-storeandIf-Range, and block stale serving on forced revalidation pathsuse_stalewith 403 / 404 / 429 and add metadata compatibility plus regression coverageTesting