feat(cache): upgrade shared index to shared metadata db#71
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 (15)
📝 WalkthroughWalkthrough本PR将共享缓存索引的持久化机制从JSON文件迁移至SQLite数据库，引入 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 30 minutes and 13 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.
Summary
This PR successfully upgrades the shared cache index from a JSON-based sidecar file to a SQLite-backed metadata store with generation-based synchronization. The implementation includes proper transaction management, legacy state migration, and incremental updates for cache operations.
Key Changes
- ✅ SQLite backend with WAL mode and proper transaction handling
- ✅ Generation-based synchronization to track shared index updates
- ✅ Legacy JSON index import and cleanup
- ✅ Incremental shared metadata operations for all cache mutations
- ✅ Comprehensive error handling and logging
Review Outcome
No blocking issues identified. The implementation follows Rust best practices and handles edge cases appropriately. The migration path from legacy JSON format is well-designed, and the SQLite schema is properly initialized with version checking.
Status: ✅ Approved for merge
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 migrates the cache indexing system from a JSON-based sidecar file to a SQLite-backed store to improve metadata management and synchronization. It introduces the rusqlite dependency and implements a migration path for legacy JSON index data. Additionally, a new architectural document, CACHE_ARCHITECTURE_GAPS.md, was added to track long-term goals for the cache engine. Feedback focused on enhancing transaction safety within the SQLite implementation by using IMMEDIATE transactions to prevent potential deadlocks or SQLITE_BUSY errors during concurrent write operations.
![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: 3d85cb9885
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR replaces the cache shared-index JSON sidecar with a SQLite-backed shared metadata store, using generation-based sync and incremental updates, and adds documentation/tests around the new cache metadata architecture.
Changes:
- Introduce a SQLite
SharedIndexStore(.rginx-index.sqlite3) with generation tracking and apply-on-write operations for entries and admission counts. - Update cache flows (store, purge, inactive cleanup, revalidation cleanup, lookup error paths) to emit incremental shared-metadata updates instead of rewriting a full JSON index.
- Add legacy
.rginx-index.jsonbootstrap import + new tests, and document current cache architecture gaps.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/README.md | Adds the new cache architecture gaps document to the docs index. |
| docs/CACHE_ARCHITECTURE_GAPS.md | New document describing long-term cache architecture gaps and code anchors. |
| crates/rginx-http/src/cache/tests/storage_p2.rs | Adds tests for purge propagation via shared metadata + legacy sidecar import bootstrap. |
| crates/rginx-http/src/cache/tests/storage_p1.rs | Updates test runtime construction for the new shared index store field and generation semantics. |
| crates/rginx-http/src/cache/tests/mod.rs | Updates test zone runtime construction to include shared_index_store. |
| crates/rginx-http/src/cache/store/revalidate.rs | Switches shared-index updates to targeted removal operations on revalidate cleanup. |
| crates/rginx-http/src/cache/store/maintenance.rs | Emits incremental shared-index operations for cleanup/purge/store/evict, and refactors admission attempt API. |
| crates/rginx-http/src/cache/store.rs | Applies admission-count shared-index operations immediately after admission attempt. |
| crates/rginx-http/src/cache/shared/index_file.rs | Implements SQLite-backed shared index store + legacy JSON loader/deleter. |
| crates/rginx-http/src/cache/shared.rs | Adds generation-based syncing and shared-index operation application; bootstraps SQLite store (with legacy import). |
| crates/rginx-http/src/cache/runtime.rs | Uses targeted shared-index entry removal when invalidating entries during stale serve failures. |
| crates/rginx-http/src/cache/mod.rs | Updates cache runtime struct to carry shared_index_store and removes mtime-based sync field. |
| crates/rginx-http/src/cache/manager.rs | Plumbs new bootstrap return values into cache zone runtime and invalidation paths. |
| crates/rginx-http/src/cache/lookup.rs | Uses targeted shared-index entry removal when discarding bad cache entries. |
| crates/rginx-http/Cargo.toml | Adds rusqlite dependency. |
| README.md | Links the new cache architecture gaps document. |
| Cargo.toml | Adds workspace rusqlite dependency (bundled SQLite). |
| Cargo.lock | Locks new rusqlite/sqlite dependency graph. |
💡 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: 4
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/cache/store/maintenance.rs (1)
7-79: 🛠️ Refactor suggestion | 🟠 Major把共享索引操作规划从这个模块里拆出去。
这几个函数现在同时负责本地索引变更、磁盘删除、
SharedIndexOperation收集/下发、统计和通知，update_index_after_store尤其已经超出单一职责；CI 也已经因为这个文件触发了 modularization gate。建议至少把 shared-index operation 的构建提取成私有 helper/plan，或把 purge/cleanup/store-update 的 shared-index 逻辑拆到独立子模块。Also applies to: 81-127, 129-209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/cache/store/maintenance.rs` around lines 7 - 79, The function cleanup_inactive_entries_in_zone is doing too many responsibilities (local index mutation, disk deletes, building SharedIndexOperation list, stats and notifications); extract the shared-index operation planning into a dedicated private helper (e.g., plan_shared_operations_for_removed_entries) or a small submodule and call it from cleanup_inactive_entries_in_zone so this function only (1) computes removals, (2) performs disk IO, and (3) applies the already-built shared plan via apply_zone_shared_index_operations; similarly refactor related logic in update_index_after_store to delegate construction of SharedIndexOperation items (the SharedIndexOperation variants: RemoveEntry, RemoveAdmissionCount, etc.) to the same helper/submodule, return a Vec<SharedIndexOperation> and keep notification/stats (record_inactive_cleanup/notify_changed) in the caller.crates/rginx-http/src/cache/shared.rs (1)
54-110:⚠️ Potential issue | 🔴 Critical本地索引更新和 shared generation 推进不在同一个临界区里，会丢状态。
像
crates/rginx-http/src/cache/store/maintenance.rsLine 129-209、crates/rginx-http/src/cache/store.rsLine 102-105 这些路径都会先改zone.index，然后在另一个await点里才进入apply_zone_shared_index_operations()。如果这之间另一个请求先跑进sync_zone_shared_index_if_needed()并拿到shared_index_sync_lock，它会用旧的 shared snapshot 覆盖本地索引；随后当前请求再把操作写进 SQLite 并把shared_index_generation提升到新值，但本地内存不会再回补。结果就是本进程可能长期丢掉自己刚写入的 entry，或者把自己刚 purge 掉的 entry 又留回内存，直到下一次外部 generation 变化才有机会自愈。这里需要把“本地索引变更 + shared store 更新/同步”纳入同一个同步协议，而不是允许 sync 插在两者之间。Also applies to: 113-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/cache/shared.rs` around lines 54 - 110, The bug is that local in-memory index updates and advancing the shared_generation are not done under the same critical section, so sync_zone_shared_index_if_needed() can run between them and overwrite local changes; fix by making the "modify local zone.index + persist to shared store + bump zone.shared_index_generation" a single synchronized operation guarded by the same shared_index_sync_lock used in sync_zone_shared_index_if_needed(). Concretely: ensure callers that currently mutate zone.index and then await apply_zone_shared_index_operations() (e.g. code paths referenced in maintenance.rs and store.rs and functions that call apply_zone_shared_index_operations()) acquire zone.shared_index_sync_lock.lock().await around the entire sequence (local mutation, calling the persistence function that writes SQLite/shared store, and storing the new generation via zone.shared_index_generation.store(...)) or refactor into a new helper (e.g. apply_and_advance_shared_index()) that takes the lock internally and performs local mutation + run_blocking persistence + generation store atomically; also remove any await points between local mutation and generation bump so the lock actually covers both steps.crates/rginx-http/src/cache/shared/index_file.rs (1)
21-507: 🛠️ Refactor suggestion | 🟠 Major这个文件已经触发 modularization gate，需要拆分。
CI 已经因为硬限制失败：
507 > 500。当前文件同时承载了 SQLite store、legacy JSON 迁移、record 编解码和通用 helper，职责过多。建议至少拆成store、legacy、record三块，把迁移逻辑和 SQLite 操作解耦；这也是当前 PR 合并前必须处理的门槛问题。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/cache/shared/index_file.rs` around lines 21 - 507, The file is too large and must be split into modules; extract SQLite store logic into a new store module containing SharedIndexStore impl methods (open_raw_connection, open_connection, initialize_schema, recreate, replace_all, apply_operations, generation, path, read/write meta helpers used there), move legacy JSON migration functions (load_legacy_shared_index_from_disk, delete_legacy_shared_index_file, legacy_shared_index_path, index_from_legacy_shared_file, LegacySharedIndexFile/LegacySharedIndexEntry/LegacySharedAdmissionCount) into a legacy module, and move record serialization/deserialization types and helpers (SharedIndexEntryRecord, SharedVaryHeader, record_from_cache_index_entry, cache_index_entry_from_record) into a record module; update public visibility and function references (shared_index_store, shared_index_path, shared_fill_lock_path, shared_index_generation, apply_shared_index_operations) so callers use the new modules without logic changes, and keep small helpers io_error/invalid_data_error adjacent to code that uses them or in a common util module. Ensure tests and imports are updated to the new module paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Line 51: 将 Cargo.toml 中对 rusqlite 的依赖从 version = "0.37" 更新为最新发布版本
"0.39.0"，同时保留 features = ["bundled"] 配置；在文件中找到依赖条目 rusqlite = { version =
"0.37", features = ["bundled"] } 并仅替换版本号为 "0.39.0"，然后运行 cargo update / cargo
build 验证无破坏性变更并修复因升级引入的任何编译警告或 API 变化（若出现，检查 rusqlite 的 changelog/upgrade
notes）。
In `@crates/rginx-http/src/cache/shared/index_file.rs`:
- Around line 107-118: The code currently compares LegacySharedIndexFile.version
against SHARED_INDEX_SCHEMA_VERSION (cast to u8), which ties legacy JSON import
to the SQLite schema version; instead define and use a separate legacy JSON
version constant (e.g., LEGACY_SHARED_INDEX_VERSION: u8) and replace the
comparison in the LegacySharedIndexFile deserialization block (the check that
currently uses SHARED_INDEX_SCHEMA_VERSION as u8) to compare file.version to
that new constant so legacy .rginx-index.json files remain importable across
schema bumps.
- Around line 136-178: load_shared_index_from_disk currently reads generation
(read_meta_u64) and then separately queries entries and admission_counts on the
same Connection which can produce inconsistent snapshots under concurrent
writes; wrap all three reads in a single rusqlite::Transaction (use
connection.read_only_transaction() / connection.transaction() as appropriate)
and replace calls to connection.prepare / read_meta_u64 with transaction.prepare
and the helper read_meta_u64_tx so the generation, entries scan, and
admission_counts scan are performed against the same snapshot, then build the
CacheIndex and return LoadedSharedIndex as before (update usages of
cache_index_entry_from_record and insertion into index to work with the
transaction-scoped query results).
- Around line 250-259: recreate() currently unconditionally calls drop_schema()
before replace_all(), creating a transient window where tables are missing and
causing "table not found" for concurrent readers; change recreate() to validate
the existing schema first and only call drop_schema() when validation fails
(i.e., schema is corrupt or incompatible), otherwise skip DROP and proceed to
replace_all() which already clears data inside a transaction; implement or call
a schema validation function (e.g., check_schema or validate_schema) after
open_raw_connection() and before deciding to drop, and ensure
initialize_schema() and replace_all() are idempotent so normal rebuilds do not
require DROP; update references in recreate(), drop_schema(), replace_all(), and
initialize_schema() accordingly.
---
Outside diff comments:
In `@crates/rginx-http/src/cache/shared.rs`:
- Around line 54-110: The bug is that local in-memory index updates and
advancing the shared_generation are not done under the same critical section, so
sync_zone_shared_index_if_needed() can run between them and overwrite local
changes; fix by making the "modify local zone.index + persist to shared store +
bump zone.shared_index_generation" a single synchronized operation guarded by
the same shared_index_sync_lock used in sync_zone_shared_index_if_needed().
Concretely: ensure callers that currently mutate zone.index and then await
apply_zone_shared_index_operations() (e.g. code paths referenced in
maintenance.rs and store.rs and functions that call
apply_zone_shared_index_operations()) acquire
zone.shared_index_sync_lock.lock().await around the entire sequence (local
mutation, calling the persistence function that writes SQLite/shared store, and
storing the new generation via zone.shared_index_generation.store(...)) or
refactor into a new helper (e.g. apply_and_advance_shared_index()) that takes
the lock internally and performs local mutation + run_blocking persistence +
generation store atomically; also remove any await points between local mutation
and generation bump so the lock actually covers both steps.
In `@crates/rginx-http/src/cache/shared/index_file.rs`:
- Around line 21-507: The file is too large and must be split into modules;
extract SQLite store logic into a new store module containing SharedIndexStore
impl methods (open_raw_connection, open_connection, initialize_schema, recreate,
replace_all, apply_operations, generation, path, read/write meta helpers used
there), move legacy JSON migration functions
(load_legacy_shared_index_from_disk, delete_legacy_shared_index_file,
legacy_shared_index_path, index_from_legacy_shared_file,
LegacySharedIndexFile/LegacySharedIndexEntry/LegacySharedAdmissionCount) into a
legacy module, and move record serialization/deserialization types and helpers
(SharedIndexEntryRecord, SharedVaryHeader, record_from_cache_index_entry,
cache_index_entry_from_record) into a record module; update public visibility
and function references (shared_index_store, shared_index_path,
shared_fill_lock_path, shared_index_generation, apply_shared_index_operations)
so callers use the new modules without logic changes, and keep small helpers
io_error/invalid_data_error adjacent to code that uses them or in a common util
module. Ensure tests and imports are updated to the new module paths.
In `@crates/rginx-http/src/cache/store/maintenance.rs`:
- Around line 7-79: The function cleanup_inactive_entries_in_zone is doing too
many responsibilities (local index mutation, disk deletes, building
SharedIndexOperation list, stats and notifications); extract the shared-index
operation planning into a dedicated private helper (e.g.,
plan_shared_operations_for_removed_entries) or a small submodule and call it
from cleanup_inactive_entries_in_zone so this function only (1) computes
removals, (2) performs disk IO, and (3) applies the already-built shared plan
via apply_zone_shared_index_operations; similarly refactor related logic in
update_index_after_store to delegate construction of SharedIndexOperation items
(the SharedIndexOperation variants: RemoveEntry, RemoveAdmissionCount, etc.) to
the same helper/submodule, return a Vec<SharedIndexOperation> and keep
notification/stats (record_inactive_cleanup/notify_changed) in the caller.
🪄 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: 63235a9d-b240-4394-9532-c194d3a3595c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Cargo.tomlREADME.mdcrates/rginx-http/Cargo.tomlcrates/rginx-http/src/cache/lookup.rscrates/rginx-http/src/cache/manager.rscrates/rginx-http/src/cache/mod.rscrates/rginx-http/src/cache/runtime.rscrates/rginx-http/src/cache/shared.rscrates/rginx-http/src/cache/shared/index_file.rscrates/rginx-http/src/cache/store.rscrates/rginx-http/src/cache/store/maintenance.rscrates/rginx-http/src/cache/store/revalidate.rscrates/rginx-http/src/cache/tests/mod.rscrates/rginx-http/src/cache/tests/storage_p1.rscrates/rginx-http/src/cache/tests/storage_p2.rsdocs/CACHE_ARCHITECTURE_GAPS.mddocs/README.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). (1)
- GitHub Check: Agent
🧰 Additional context used
🪛 GitHub Actions: CI
crates/rginx-http/src/cache/shared/index_file.rs
[error] 1-1: modularization-gate: new production hard-limit violation — file has 507 lines; hard limit is 500.
crates/rginx-http/src/cache/store/maintenance.rs
[error] 1-1: modularization-gate: new production soft-limit violation — file has 340 lines; soft limit is 300.
🪛 LanguageTool
docs/CACHE_ARCHITECTURE_GAPS.md
[uncategorized] ~31-~31: 动词的修饰一般为‘形容词（副词）+地+动词’。您的意思是否是：长"地"响应
Context: ...件，而不是依赖完整 body 收集 - 让缓存路径能够覆盖更大的响应体和更长的响应生命周期 - 让“是否可缓存”不再强依赖完整 body 先入内存 ##...
(wb4)
[uncategorized] ~49-~49: 动词的修饰一般为‘形容词（副词）+地+动词’。您的意思是否是：重"地"扫描
Context: ...增量、低抖动后台维护 当前淘汰和 inactive cleanup 都依赖较重的扫描、排序和批处理删除。 - 当前问题： - cache zone 变大后...
(wb4)
[uncategorized] ~60-~60: 您的意思是“"不"齐”？
Context: ...台波动 - 让缓存维护任务在高负载下仍保持可预测的延迟特征 ### 4. 补齐 range / stale / revalidate / policy 的长...
(BU)
🔇 Additional comments (10)
docs/README.md (1)
7-8: LGTM!文档索引更新正确，描述与实际文档内容一致。
README.md (1)
147-147: LGTM!README 文档链接添加正确。
crates/rginx-http/Cargo.toml (1)
40-40: LGTM!通过 workspace 引用添加
rusqlite依赖，与 PR 目标一致。crates/rginx-http/src/cache/tests/storage_p1.rs (1)
230-257: 测试重构正确，LGTM!测试现在正确地：
- 创建共享的
Arc<CacheZone>并在CacheZoneRuntime和shared_index_store之间复用- 将
shared_index_generation初始化为0，符合新的 generation-based 同步模型- 移除了
shared_index_last_modified_unix_ms，与 SQLite 迁移一致crates/rginx-http/src/cache/tests/mod.rs (1)
31-59: 测试辅助函数重构正确，LGTM!
test_zone_with_notifier的更新与storage_p1.rs保持一致，确保所有缓存测试使用统一的SharedIndexStore初始化模式。crates/rginx-http/src/cache/store/revalidate.rs (2)
3-4: 导入更新正确，LGTM!从
persist_zone_shared_index改为remove_zone_shared_index_entry，符合增量更新的设计目标。
98-106: 增量共享索引更新逻辑正确。在重验证失败路径中，现在只移除特定 key 的共享索引条目，而不是重新持久化整个 zone 索引。通过
remove_zone_shared_index_entry调用apply_zone_shared_index_operations，仅执行RemoveEntry和RemoveAdmissionCount两个操作，这是更高效的增量更新方式，与 PR 的 SQLite 迁移目标一致。docs/CACHE_ARCHITECTURE_GAPS.md (1)
1-101: 文档结构清晰，代码锚点路径完整。LGTM！文档完整阐述了缓存架构的五个长期演进方向，层次分明。已验证文档中列出的所有代码锚点路径均存在于仓库中：
- 10 个涉及缓存核心逻辑的文件路径全部有效
- 包括
crates/rginx-http/src/cache/下的 lookup、store、maintenance、revalidate、shared、request 等模块- 以及
crates/rginx-config/下的缓存配置模型和验证模块代码锚点准确，可作为后续架构升级的切入点。
crates/rginx-http/src/cache/tests/storage_p2.rs (1)
97-193: 这组回归测试很有价值。它把“跨 manager 的 purge 传播”和“遗留 sidecar 导入 SQLite 并可再次加载”这两条最容易回归的路径都锁住了，和这次迁移的风险点对得很准。
crates/rginx-http/src/cache/shared/index_file.rs (1)
267-313: 并发写入的 generation 递增是安全的。此代码的所有调用点都经过
zone.shared_index_sync_lock异步互斥锁保护（见shared.rs第125-126行），确保任何时刻只有一个写入者执行apply_operations。因此，即使使用 deferred 事务，也不存在两个 writer 并发读取同一 generation 值的竞态条件。应用层的同步已经保证了 generation 的单调递增。建议的TransactionBehavior::Immediate修改虽然不会造成问题，但对此场景来说是不必要的。> Likely an incorrect or invalid review comment.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Addressed the review feedback in Changes:
Local verification:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
vansour
dismissed
coderabbitai[bot]’s stale review
Feedback addressed in commit efca238; dismissing stale automated review so the PR can merge.
Summary
.rginx-index.jsonstate into the new store and document the remaining cache architecture gapsTesting