feat(querier): RFC 0033 green (freshness) — cached read path + write-through, .2/.4/.5 - #511
Conversation
…through, .2/.4/.5 load_or_derive resolves a tenant's TemplateMap through the published artifact when its folded_files frontier set-equals the live audit listing (hit: one LIST + one artifact GET, zero audit GETs) and falls back to the fresh fold over that SAME listing on every non-hit disposition (absent / stale / torn / unknown version — §3.3's LIST-before-GET-is-compared, one listing for both the validity check and the fallback fold), then write-through-publishes best-effort with the CAS expectation observed by the read (§3.5: publish failure and lost races are telemetry-only, never a query failure; §3.2 abstention when the artifact would not be smaller than the audit bytes folded). A tenant-mismatched artifact body still fails the query loudly. Both query call sites now share one acquisition per query: the resolves_to alias fold acquires at compile time and hands the map to the row-rendering pass, so the alias map and the registry can never come from different frontiers. QueryResult::registry_bytes_read is amended per RFC 0033 §3.6 to template-map acquisition bytes (fold bytes on a miss — byte-for-byte the pre-0033 figure — artifact GET bytes on a hit), with RFC 0031 §3.6's wording amended to match. RFC0033.2 (staleness detected via the answer itself, republish at the new frontier, removal direction = set equality), RFC0033.4 (no artifact ⇒ byte-for-byte today's fold and figures; deletion costs one re-derivation; the audit walk never sees the artifact) and RFC0033.5 (tenant-scoped artifacts, hits serve only their own tenant, foreign body fails end-to-end) replace their stubs; .6/.7 stay red. Invariants: §3.6 — the audit stream stays the source of truth (every doubtful read refolds it); §3.7 — the artifact is tenant-scoped under tenant_id=<enc> with the row-vs-path backstop exercised end-to-end. Pinned contracts (rfc0017_6 limited.stats == counted.stats, the RFC 0031 single-pass totals, bench comparative fixtures) pass unmodified. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe querier now resolves audit frontiers once, classifies and reuses cached template maps, derives aliases and registries from a shared scan on misses, threads acquired maps through query rendering, and enables RFC0033 tests for staleness, byte accounting, and tenant isolation. ChangesCached template-map query flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Query
participant TemplateMap
participant AuditStore
participant Renderer
Query->>TemplateMap: load_or_derive tenant map
TemplateMap->>AuditStore: resolve frontier and read events
AuditStore-->>TemplateMap: events and measured bytes
TemplateMap-->>Query: template map and acquisition outcome
Query->>Renderer: render rows with acquired map
Renderer-->>Query: records and registry bytes
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
Implements the RFC 0033 “freshness + write-through” cached template-map acquisition in the querier query path, and updates the comparative-evaluation byte-contract wording to match the new acquisition semantics.
Changes:
- Adds
load_or_derive(LIST → GET → frontier set-equality → fold fallback + best-effort write-through) and threads a single acquiredTemplateMapthrough bothresolves_tocompilation and record rendering. - Refactors audit scanning to make “one listing, shared by freshness check + fold” structural via
ResolvedAuditSet. - Activates and implements RFC0033 scenarios .2/.4/.5 integration tests; amends RFC 0031 doc text for
registry_bytes_read.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/rfcs/0031-comparative-evaluation-loki.md | Documents amended meaning of registry_bytes_read as template-map acquisition bytes post-RFC0033. |
| crates/ourios-querier/tests/it/rfc0033_cached_template_map.rs | Implements RFC0033.2/.4/.5 integration scenarios for freshness, advisory behavior, and tenant isolation. |
| crates/ourios-querier/src/template_registry.rs | Updates registry derivation docs to reflect cached template-map acceleration/fallback semantics. |
| crates/ourios-querier/src/template_map.rs | Introduces cached read path (load_or_derive), cache outcome types, and write-through abstention logic. |
| crates/ourios-querier/src/lib.rs | Wires cached acquisition into query compilation + rendering with a single per-query acquisition. |
| crates/ourios-querier/src/audit_scan.rs | Adds ResolvedAuditSet to couple a single listing with the subsequent fold read. |
| crates/ourios-querier/src/alias_store.rs | Updates alias-store derivation docs to reflect cached template-map acceleration/fallback semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/ourios-querier/src/template_map.rs (2)
239-278: 🩺 Stability & Availability | 🔵 TrivialReal backend errors and genuine corruption collapse into the same
Unreadable/MissReason::Tornoutcome.Both branches map any non-not-found error (a transient S3 timeout, permission issue, etc.) to
Unreadable, which is later reported asMissReason::Torn. This is safe by design (RFC 0033 §1: the fresh fold is always correct), but it means a sustained backend outage silently degrades every query on that tenant to a full audit-tree fold with no distinguishing signal from genuine artifact corruption. Since lookup-outcome telemetry is already deferred to the RFC0033 §3.7 slice (per the module docs and PR scope), this is just a heads-up for that follow-up: consider surfacing the underlying error kind (not just the discarded classification) so the eventual metrics/logs can distinguish "backend flaky" from "artifact torn".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ourios-querier/src/template_map.rs` around lines 239 - 278, Preserve the current safe fallback behavior in fetch_artifact, but retain the underlying non-not-found backend error information instead of discarding it when classifying failures as Unreadable. Carry enough error-kind/context through the eventual MissReason::Torn and RFC 0033 §3.7 lookup telemetry path to distinguish backend failures from genuine artifact corruption, without expanding the current telemetry scope beyond that follow-up.
566-601: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd a property test for the
TemplateMapreconstruction round trip.
write_through/publish_bytesnow maketo_json→ (store) →from_jsona hot-path operation on effectively every cache-miss query, rather than an occasionally-exercised admin path. As per path instructions,**/crates/ourios-{miner,parquet,querier}/**/*.rs: "Use property tests (proptest) for anything with an invariant... Reconstruction is always a property test." Theto_json/from_jsonpair (lines 416-527) is exactly this kind of reconstruction invariant (arbitraryTemplateRegistry/AliasMapcombinations should serialize and parse back to an equivalentTemplateMap), and I don't see proptest coverage for it in the reviewed context.proptest! { #[test] fn template_map_json_round_trips(registry in arb_registry(), aliases in arb_alias_map(), files in arb_folded_files()) { let tenant = tenant(); let map = TemplateMap { tenant: tenant.clone(), folded_files: files, registry, aliases }; let bytes = map.to_json().expect("serialize"); let read = TemplateMap::from_json(&bytes, &tenant).expect("parse"); prop_assert!(matches!(read, ArtifactRead::Valid(rt) if rt.folded_files() == map.folded_files() && rt.registry() == map.registry() && rt.alias_map().classes(&tenant) == map.alias_map().classes(&tenant))); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ourios-querier/src/template_map.rs` around lines 566 - 601, Add a proptest property covering the TemplateMap JSON reconstruction invariant. Reuse or define generators for TemplateRegistry, AliasMap, and folded files, construct a TemplateMap with a tenant, serialize it via to_json, parse it with from_json, and assert a valid ArtifactRead preserves the folded files, registry, and tenant-scoped alias classes.Source: Path instructions
crates/ourios-querier/src/lib.rs (1)
816-843: 📐 Maintainability & Code Quality | 🔵 TrivialRecord
CacheOutcomeat both template-map acquisition sites (crates/ourios-querier/src/lib.rs:834and:1197).load_or_derivealready classifies hit/miss/stale, but both call sites discard_outcome, so the query path still drops the signal needed for cache-hit telemetry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ourios-querier/src/lib.rs` around lines 816 - 843, Retain the CacheOutcome returned by template_map::load_or_derive at both template-map acquisition sites in the query paths, replacing the discarded _outcome binding. Record or propagate that outcome through the existing query telemetry mechanism so cache hit, miss, and stale classifications are emitted for each acquisition, while preserving the shared acquired map behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/ourios-querier/src/lib.rs`:
- Around line 816-843: Retain the CacheOutcome returned by
template_map::load_or_derive at both template-map acquisition sites in the query
paths, replacing the discarded _outcome binding. Record or propagate that
outcome through the existing query telemetry mechanism so cache hit, miss, and
stale classifications are emitted for each acquisition, while preserving the
shared acquired map behavior.
In `@crates/ourios-querier/src/template_map.rs`:
- Around line 239-278: Preserve the current safe fallback behavior in
fetch_artifact, but retain the underlying non-not-found backend error
information instead of discarding it when classifying failures as Unreadable.
Carry enough error-kind/context through the eventual MissReason::Torn and RFC
0033 §3.7 lookup telemetry path to distinguish backend failures from genuine
artifact corruption, without expanding the current telemetry scope beyond that
follow-up.
- Around line 566-601: Add a proptest property covering the TemplateMap JSON
reconstruction invariant. Reuse or define generators for TemplateRegistry,
AliasMap, and folded files, construct a TemplateMap with a tenant, serialize it
via to_json, parse it with from_json, and assert a valid ArtifactRead preserves
the folded files, registry, and tenant-scoped alias classes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 99f19bbf-01e3-40c2-bdb7-6597c133ebab
📒 Files selected for processing (7)
crates/ourios-querier/src/alias_store.rscrates/ourios-querier/src/audit_scan.rscrates/ourios-querier/src/lib.rscrates/ourios-querier/src/template_map.rscrates/ourios-querier/src/template_registry.rscrates/ourios-querier/tests/it/rfc0033_cached_template_map.rsdocs/rfcs/0031-comparative-evaluation-loki.md
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
…e tenant-mismatch assertion Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
.1 fold+format (#509), .3 publish (#510), .2/.4/.5 freshness + write-through (#511), .6 comparative cold-vs-warm (#512, local arm 55.8x past the 10x gate), .7 observability (#513). The headline-corpus warm measurement runs as comparative dispatch run #20. Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
What
RFC 0033's freshness + write-through green slice — the cache enters the query path, discharging scenarios RFC0033.2, .4, .5.
load_or_deriveper §3.3: LIST → GET → frontier set-equality. One listing serves both the validity check and the fallback fold, made structural byaudit_scan::ResolvedAuditSet. IO per outcome documented and tested: hit = 1 LIST + 1 artifact GET (zero audit GETs); miss/stale = the fold over the same listing + best-effort write-through publish (CAS at the observed ETag;LostRacefine; publish failure is telemetry, never a query failure). Tenant-mismatched body fails loudly, no fallback.resolves_tocompile path andcollect_records' rendering share a singleAcquiredTemplateMap— one frontier, both folds, per query.registry_bytes_readcontract amended to template-map acquisition bytes (audit fold on miss — byte-for-byte the pre-0033 figure — artifact GET on hit), with the RFC 0031 §3.6 dated amendment block.resolves_torow counts; republish frontier asserted; removal direction proves set-equality-not-subset); .4 advisory (no-artifact acquisition == summed audit bytes on disk, exact; deletion mid-flight costs only re-derivation; audit walk blind to the artifact); .5 tenant isolation end-to-end (planted foreign artifact fails the victim's query loudly, the other tenant unaffected).Pinned contracts proved undisturbed
rfc0017_6(limited.stats == counted.stats), the RFC 0031 single-pass byte-total tests, and the bench comparative fixture tests — all unmodified, all green (330/330 across querier+bench, re-verified after rebasing over the artifact PR's hardening rounds and the publish merge).Remaining
.6 (comparative cold-vs-warm — the dispatch measurement that unfreezes
M_L2) and .7 (observability) — next slices.Checks run
cargo fmt --all --check,cargo clippy --all-targets --all-features -- -D warnings(workspace-wide), strict rustdoc (-D private-intra-doc-links, clean),cargo nextest run -p ourios-querier -p ourios-bench(330 passed).🤖 Generated with Claude Code
https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
Summary by CodeRabbit
Performance
registry_bytes_readnow reflects whether registry data came from an artifact GET (cache hit) or audit-based derivation (cache miss).Reliability
Documentation
Tests