Add cache benchmarks, stress tests, and fault injection#74
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 (1)
📝 Walkthrough说明本次变更添加了缓存系统的测试套件、性能基准测试和压力测试工具，以验证缓存在并发流量、配置重加载和故障场景下的稳定性和正确性。 变更内容
估计代码审查工作量🎯 3 (Moderate) | ⏱️ ~25 分钟 可能相关的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 43 minutes and 43 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.
This PR successfully adds a comprehensive cache testing infrastructure including benchmarks, stress tests, and fault injection scenarios. The implementation is well-structured:
Added Components:
- Cache-specific benchmark harness (
run-cache-benchmark.py) for fill, hit, revalidate, and slice-hit paths - Stress test suite (
run-cache-stress.sh) covering large keysets, large objects, and reload concurrency - Fault injection tests for missing cache body files and corrupted shared index metadata
- Integration test for reload cache preservation under concurrent traffic
Quality Observations:
- Proper test isolation with ignore attributes and dedicated test runners
- Comprehensive coverage of cache failure modes
- Good use of concurrent testing patterns with appropriate synchronization
- Documentation updated to reflect completion of benchmark/stress testing milestone
The code is ready to merge. All tests are properly marked as ignored and require explicit execution via the provided scripts, preventing CI overhead.
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 establishes a robust testing and benchmarking framework for the rginx cache system, including new stress tests for concurrent reloads, large keysets, and large body handling, alongside fault injection scenarios for metadata corruption and missing files. It also introduces a Python-based benchmark script and a Bash-based stress test runner. Feedback identifies a bug in the benchmark script's cache size calculation that causes unintended LRU evictions, suggests enhancing the mock server's HTTP range parsing to support standard prefix/suffix formats, and recommends streaming response bodies in stress tests to mitigate potential memory pressure under high concurrency.
There was a problem hiding this comment.
Pull request overview
Adds a cache-focused performance/stress/fault-injection validation layer around the existing rginx HTTP cache, providing repeatable benchmarks plus ignored stress suites that can be driven by scripts.
Changes:
- Introduces a cache benchmark runner (
scripts/run-cache-benchmark.py) that stands up a local origin + realrginxprocess and measures fill/hit/revalidate/slice-hit paths. - Adds a cache stress driver script (
scripts/run-cache-stress.sh) to run ignored stress tests acrossrginx-http(unit-style stress) andrginxreload integration stress. - Adds regression tests for cache fault modes (missing body file cleanup; corrupted shared-index SQLite metadata) and documents the new run commands in
CACHE_ARCHITECTURE_GAPS.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/run-cache-stress.sh | New script to execute the ignored cache stress matrix across crates. |
| scripts/run-cache-benchmark.py | New real-process benchmark harness for cache fill/hit/revalidate/slice-hit scenarios. |
| docs/CACHE_ARCHITECTURE_GAPS.md | Documents the new benchmark/stress/fault-injection “first pass” and how to run it. |
| crates/rginx-http/src/cache/tests/stress.rs | Adds ignored multi-thread stress tests for large keysets and large cached bodies. |
| crates/rginx-http/src/cache/tests/storage_p2.rs | Adds shared-index fault-injection regression for corrupted SQLite metadata. |
| crates/rginx-http/src/cache/tests/mod.rs | Wires in the new stress test module. |
| crates/rginx-http/src/cache/tests/lookup/recovery.rs | Adds missing-body-file fault-injection regression (miss + index/file cleanup). |
| crates/rginx-app/tests/reload/cache.rs | Adds ignored reload+traffic stress test to ensure hot-cache hits survive concurrent reloads. |
💡 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/run-cache-benchmark.py`:
- Around line 86-87: The silent HTTP log override currently defines def
log_message(self, format: str, *args: object) -> None: return which triggers
linter warnings for unused parameters and shadowing the built-in name `format`;
rename the parameters to indicate intentional unused status (e.g., change
`format` to `_format` and `*args` to `*_args` or prefix them with underscores)
or add a brief comment above `log_message` documenting that it's intentionally a
no-op for benchmarks so linters and readers understand the intent; keep the
method body as an empty return to preserve the silent-logging behavior.
- Around line 436-439: The loop that collects results into durations uses
repeated durations.append(future.result()) inside the ThreadPoolExecutor block;
replace that loop by extending durations with a generator expression over
concurrent.futures.as_completed to collect all results at once (e.g.,
durations.extend(f.result() for f in concurrent.futures.as_completed(futures))).
Locate the block that creates futures with executor.submit(run_request, port,
request, timeout) and modify the post-submission collection to use
durations.extend(...) instead of the for future in ...: durations.append(...)
loop to reduce per-item append overhead.
- Line 708: 变量名 "slice" 遮蔽了 Python 内置的 slice 类型；在将 slice_requests(args.requests)
的结果赋值时重命名该变量（例如 requests_slice 或 req_slice），并同步更新后续对该变量的所有引用（大约在后续块引用处，原位于
slice_requests 调用后的 718-728 行）；确保函数调用 slice_requests(args.requests)
保持不变，只改变量名以避免命名冲突。
🪄 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: 23477fcb-b1cf-48f4-b618-37d67e3a8362
📒 Files selected for processing (8)
crates/rginx-app/tests/reload/cache.rscrates/rginx-http/src/cache/tests/lookup/recovery.rscrates/rginx-http/src/cache/tests/mod.rscrates/rginx-http/src/cache/tests/storage_p2.rscrates/rginx-http/src/cache/tests/stress.rsdocs/CACHE_ARCHITECTURE_GAPS.mdscripts/run-cache-benchmark.pyscripts/run-cache-stress.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: Test Fast
🧰 Additional context used
🪛 LanguageTool
docs/CACHE_ARCHITECTURE_GAPS.md
[uncategorized] ~65-~65: 动词的修饰一般为‘形容词（副词）+地+动词’。您的意思是否是：稳定"地"回归
Context: ...盖 - shared index 损坏和文件缺失这类缓存失败模式现在有了稳定的回归入口 ## 长期差距 ### 1. 将缓存写入路径从“全量收集后落盘”升...
(wb4)
🪛 Ruff (0.15.12)
scripts/run-cache-benchmark.py
[error] 86-86: Function argument format is shadowing a Python builtin
(A002)
[warning] 86-86: Unused method argument: format
(ARG002)
[warning] 86-86: Unused method argument: args
(ARG002)
[warning] 150-150: Too many return statements (9 > 6)
(PLR0911)
[warning] 190-190: Boolean-typed positional argument in function definition
(FBT001)
[error] 197-197: Starting a process with a partial executable path
(S607)
[warning] 327-329: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 342-342: Avoid specifying long messages outside the exception class
(TRY003)
[error] 379-379: subprocess call: check for execution of untrusted input
(S603)
[warning] 388-388: Consider moving this statement to an else block
(TRY300)
[warning] 415-418: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 420-423: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 425-428: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 439-439: Use list.extend to create a transformed list
(PERF401)
[warning] 451-451: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
[warning] 573-573: Too many statements (70 > 50)
(PLR0915)
[warning] 598-598: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 680-680: Avoid specifying long messages outside the exception class
(TRY003)
[error] 708-708: Variable slice is shadowing a Python builtin
(A001)
[warning] 729-729: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 745-745: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (9)
crates/rginx-http/src/cache/tests/storage_p2.rs (1)
98-152: LGTM! 共享元数据损坏的故障注入测试覆盖良好。该测试正确验证了当共享元数据数据库被损坏后，已同步到本地索引的缓存条目仍能正常命中。测试逻辑清晰：先建立共享缓存条目、确认第二个 manager 可以命中、然后注入损坏、最后验证本地命中不受影响。
scripts/run-cache-benchmark.py (1)
740-745: 入口点处理 KeyboardInterrupt 的模式可接受。当前实现使用退出码 130（SIGINT 标准退出码）是正确的。Linter 建议使用
raise ... from None来明确表示这是有意的异常转换，但对于 CLI 脚本这种写法是常见且可接受的。crates/rginx-http/src/cache/tests/mod.rs (1)
21-21: LGTM! 正确添加了 stress 测试模块声明。docs/CACHE_ARCHITECTURE_GAPS.md (1)
49-66: LGTM! 文档清晰地记录了新增的缓存测试体系。新增内容准确描述了本 PR 添加的 benchmark、stress 测试和故障注入回归覆盖，并提供了可执行的运行命令。这对后续维护和使用都很有帮助。
注：LanguageTool 关于"稳定的回归入口"的语法建议是误报，当前用法作为定语修饰"回归入口"是正确的。
crates/rginx-http/src/cache/tests/lookup/recovery.rs (1)
199-250: LGTM! 缓存 body 文件缺失的故障注入测试覆盖完善。测试正确验证了当缓存 body 文件缺失时：
- 请求被视为 MISS 而非 HIT
- 不会触发后台更新
- 索引条目被正确清理
- 大小计数归零
- 残留的元数据文件也被清理
Line 247 的
drop(index)确保在检查文件存在性之前释放索引锁，这是正确的做法。scripts/run-cache-stress.sh (1)
1-80: LGTM! Stress 测试运行脚本结构清晰。脚本提供了良好的 CLI 接口，支持
--iterations和--release选项，参数验证完整。使用矩阵驱动方式组织测试用例，便于扩展和维护。crates/rginx-app/tests/reload/cache.rs (1)
52-139: LGTM! Reload 并发热点缓存测试覆盖了关键场景。测试通过并行流量 + 重复 SIGHUP 重载来验证缓存条目在配置重载期间保持可用。关键验证点：
- 预热后的缓存条目始终返回 HIT
- 重载期间没有额外的 upstream 请求
- 没有请求失败或返回非预期状态
这对确保生产环境下配置重载不影响缓存命中率很有价值。
crates/rginx-http/src/cache/tests/stress.rs (2)
40-114: LGTM! 大 key 数并行填充与命中的 stress 测试设计合理。测试正确验证了：
- 256 个唯一 key 的并行填充全部返回 MISS
- 填充后索引条目数量正确
- 多轮并行命中全部返回 HIT 且 body 内容一致
使用
JoinSet管理并发任务是惯用做法，Bytes::clone()是 O(1) 操作，不会造成性能问题。
116-167: LGTM! 大对象持续并行命中的 stress 测试覆盖充分。测试使用 256KB body、48 并发、6 轮的配置验证了大响应体在高并发读取下的稳定性。
response_body_len辅助函数使用流式读取方式计算 body 长度，避免一次性加载到内存，这是正确的做法。
![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: ab098070c3
ℹ️ 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
dismissed
coderabbitai[bot]’s stale review
Addressed in commit 6045c8e; dismissing stale bot change request so the PR can merge.
Summary
Testing