Merge complementary cached article sections - #106
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
There was a problem hiding this comment.
🟡 Changes recommended
Tier-aware TTL handling during merges/status refreshes and missing negative-case tests leave correctness/operational behavior mismatched with the PR’s stated intent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR extends the hybrid article cache to retain cached HEAD payloads across successful status-only refreshes, and to merge complementary cached HEAD/BODY sections into a complete ARTICLE entry when compatible.
Changes:
- Adds HEAD+BODY → ARTICLE merge support during
upsert_ingest_for_slotwhen an existing entry and a new entry contain complementary sections. - Adjusts status-only refresh logic so it no longer overwrites retained HEAD payloads.
- Adds unit tests for retained HEAD survival across status updates and for the happy-path complementary merge.
File summaries
| File | Description |
|---|---|
| src/cache/hybrid.rs | Attempts an in-place merge of an existing cached section with a newly ingested complementary section before falling back to size/semantic overwrite rules. |
| src/cache/hybrid_codec.rs | Implements section-merge logic on DiskCachedArticle, narrows when status-only refresh overwrites payload, and adds merge/status-refresh tests. |
Review details
Suppressed comments (1)
src/cache/hybrid_codec.rs:692
record_backend_has_statusno longer updatestierunless the payload isMissing/AvailabilityOnly. With the new goal of preserving HEAD across status updates, this means a successful status refresh from a higher-tier backend won't extend the entry's tier-aware TTL, potentially increasing repeat queries against that backend.
if matches!(
self.payload,
CachedPayload::Missing | CachedPayload::AvailabilityOnly
) {
self.status_code = status_code;
self.payload = CachedPayload::AvailabilityOnly;
self.tier = tier;
}
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
da90e7c to
5f6a4cd
Compare
beb575e to
8526697
Compare
Summary
Branch-added tests