fix(cas): override list_digests and delete_blob in decorator Cas impls (#143)#157
fix(cas): override list_digests and delete_blob in decorator Cas impls (#143)#157poyrazK wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 56 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughTieredCas, BloomCas, and ReplicatedCas now provide concrete list_digests and delete_blob implementations instead of relying on the Cas trait's no-op defaults. TieredCas adds hot-tier deletion via a new HotTier::delete method. GC sweep tests cover all three decorators, and CHANGELOG.md documents the fix. ChangesCAS decorator delete/list overrides
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant GC as gc::sweep
participant Decorator as CAS Decorator
participant Inner as Inner/Warm Store
participant Replicas as Replica Pool
GC->>Decorator: list_digests()
alt TieredCas
Decorator->>Inner: list_digests() (warm)
else BloomCas
Decorator->>Inner: list_digests() (inner)
else ReplicatedCas
Decorator->>Replicas: list_digests() per node
Replicas-->>Decorator: digest lists
Decorator->>Decorator: dedupe union
end
Decorator-->>GC: digests
GC->>Decorator: delete_blob(digest)
alt TieredCas
Decorator->>Decorator: hot_delete(digest)
Decorator->>Inner: delete_blob(digest) (warm)
else BloomCas
Decorator->>Inner: delete_blob(digest)
Decorator->>Decorator: leave bloom stale
else ReplicatedCas
Decorator->>Replicas: delete_blob(digest) fan-out
Replicas-->>Decorator: Ok(())
end
Decorator-->>GC: Ok(())
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
crates/brokkr-cas/src/tiered.rs (1)
540-588: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider splitting tests into a separate file to stay under 500 lines.
This file is now 589 lines. As per coding guidelines, a file >500 lines is a code smell. The test module (lines 388–589) is a natural extraction boundary — moving it to
tiered_tests.rsor atests/submodule would bring the implementation well under the limit.🤖 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/brokkr-cas/src/tiered.rs` around lines 540 - 588, The test module in TieredCas has grown past the file size guideline and should be extracted from the main implementation. Move the existing #[tokio::test] regression tests (including tiered_cas_delete_blob_drops_from_warm_and_hot, tiered_cas_delete_blob_on_missing_digest_is_ok, and tiered_cas_list_digests_delegates_to_warm) into a separate test file or tests submodule, and keep tiered.rs focused on the TieredCas implementation while preserving the same test coverage and imports.Source: Coding guidelines
crates/brokkr-cas/src/replicated.rs (2)
526-603: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAvoid
block_oninside an async test context.Line 546 uses
futures::executor::block_oninside a#[tokio::test]async function (within afilterclosure). This works only becauseInMemoryCasfutures complete immediately, but it's a latent deadlock risk with tokio's current-thread runtime. Consider collecting holder info with.awaitbefore filtering.♻️ Proposed restructuring
// Sanity: R=2 → exactly 2 of 3 replicas hold the blob. - let holders_pre: Vec<String> = backends - .iter() - .zip(["a", "b", "c"].iter()) - .filter(|(c, _)| { - futures::executor::block_on(c.find_missing_blobs(&[d.clone()])) - .map(|m| m.is_empty()) - .unwrap_or(false) - }) - .map(|(_, id)| id.to_string()) - .collect(); + let mut holders_pre = Vec::new(); + for (c, id) in backends.iter().zip(["a", "b", "c"].iter()) { + let m = c.find_missing_blobs(&[d.clone()]).await.unwrap(); + if m.is_empty() { + holders_pre.push(id.to_string()); + } + } assert_eq!(holders_pre.len(), 2, "R=2 should leave exactly 2 holders");🤖 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/brokkr-cas/src/replicated.rs` around lines 526 - 603, The `replicated_cas_delete_blob_fans_out_to_replicas` test is using `futures::executor::block_on` inside a `#[tokio::test]` async context, which can deadlock on Tokio’s current-thread runtime. Refactor the holder check to avoid blocking in the `filter` closure by awaiting `find_missing_blobs` in normal async flow first, then filtering the results; use the existing `pool_with`, `topology`, and `find_missing_blobs` logic to preserve the same assertion.
297-345: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog per-replica delete failures for operational visibility.
delete_blobintentionally discards all per-replica results to preserve the idempotentOk(())contract. However, completely silencing errors means operators have no signal when replicas fail to delete. Atracing::warn!for failed deletes would improve observability without changing the return value.♻️ Proposed addition
async fn delete_blob(&self, digest: &Digest) -> Result<(), CasError> { let replicas = self.replicas_for(digest); let deletes = replicas.iter().filter_map(|r| { self.pool.get(&r.node_id).map(|cas| { let d = digest.clone(); async move { cas.delete_blob(&d).await } }) }); - // Drain the fan-out: we deliberately drop the per-replica - // results. The trait contract is Ok(()) either way. - join_all(deletes).await; + // Drain the fan-out. We preserve the idempotent Ok(()) contract + // but log per-replica failures for operational visibility. + let results = join_all(deletes).await; + for (r, res) in replicas.iter().zip(results) { + if let Err(e) = res { + tracing::warn!(node_id = %r.node_id, error = %e, "delete_blob failed on replica"); + } + } Ok(()) }🤖 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/brokkr-cas/src/replicated.rs` around lines 297 - 345, The `delete_blob` method in `replicated.rs` is dropping all per-replica outcomes, so delete failures are completely hidden. Update the fan-out in `delete_blob` to inspect each replica’s result after `join_all`, and emit a `tracing::warn!` with the replica/node identifier and digest whenever a per-replica delete returns an error. Keep the overall method returning `Ok(())` so the idempotent `Cas` contract remains unchanged.crates/brokkr-cas/src/gc.rs (1)
435-448: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid
futures::executor::block_oninside a tokio test; use an async loop instead.
block_oninside a#[tokio::test]creates a separate executor and blocks the tokio runtime thread. It works today becauseInMemoryCasis purely in-memory, but it's an anti-pattern that could deadlock if the backend ever touches tokio I/O. Additionally,unwrap_or(false)silently swallows errors — in a test,unwrap()would fail loudly.♻️ Proposed refactor: replace `block_on` + `filter` with an async loop
// Every replica the ring selected for `dead` is now // missing it. - let holders_with_dead: Vec<usize> = backends - .iter() - .enumerate() - .filter(|(_, c)| { - futures::executor::block_on(c.find_missing_blobs(&[dead.clone()])) - .map(|m| m.is_empty()) - .unwrap_or(false) - }) - .map(|(i, _)| i) - .collect(); + let mut holders_with_dead: Vec<usize> = Vec::new(); + for (i, c) in backends.iter().enumerate() { + let missing = c.find_missing_blobs(&[dead.clone()]).await.unwrap(); + if missing.is_empty() { + holders_with_dead.push(i); + } + } assert!( holders_with_dead.is_empty(), "replicas still hold the deleted blob: {holders_with_dead:?}" );🤖 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/brokkr-cas/src/gc.rs` around lines 435 - 448, The replica check in the GC test uses futures::executor::block_on inside a tokio-driven test and also swallows backend errors with unwrap_or(false). Refactor the holders_with_dead logic in gc.rs to use an async loop or async iterator over backends so find_missing_blobs is awaited directly in the tokio context, and make the test fail loudly on errors by unwrapping or otherwise propagating the result instead of defaulting to false.
🤖 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/brokkr-cas/src/gc.rs`:
- Around line 435-448: The replica check in the GC test uses
futures::executor::block_on inside a tokio-driven test and also swallows backend
errors with unwrap_or(false). Refactor the holders_with_dead logic in gc.rs to
use an async loop or async iterator over backends so find_missing_blobs is
awaited directly in the tokio context, and make the test fail loudly on errors
by unwrapping or otherwise propagating the result instead of defaulting to
false.
In `@crates/brokkr-cas/src/replicated.rs`:
- Around line 526-603: The `replicated_cas_delete_blob_fans_out_to_replicas`
test is using `futures::executor::block_on` inside a `#[tokio::test]` async
context, which can deadlock on Tokio’s current-thread runtime. Refactor the
holder check to avoid blocking in the `filter` closure by awaiting
`find_missing_blobs` in normal async flow first, then filtering the results; use
the existing `pool_with`, `topology`, and `find_missing_blobs` logic to preserve
the same assertion.
- Around line 297-345: The `delete_blob` method in `replicated.rs` is dropping
all per-replica outcomes, so delete failures are completely hidden. Update the
fan-out in `delete_blob` to inspect each replica’s result after `join_all`, and
emit a `tracing::warn!` with the replica/node identifier and digest whenever a
per-replica delete returns an error. Keep the overall method returning `Ok(())`
so the idempotent `Cas` contract remains unchanged.
In `@crates/brokkr-cas/src/tiered.rs`:
- Around line 540-588: The test module in TieredCas has grown past the file size
guideline and should be extracted from the main implementation. Move the
existing #[tokio::test] regression tests (including
tiered_cas_delete_blob_drops_from_warm_and_hot,
tiered_cas_delete_blob_on_missing_digest_is_ok, and
tiered_cas_list_digests_delegates_to_warm) into a separate test file or tests
submodule, and keep tiered.rs focused on the TieredCas implementation while
preserving the same test coverage and imports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: abffe408-bc8c-4102-9d39-83fb102f203f
📒 Files selected for processing (5)
CHANGELOG.mdcrates/brokkr-cas/src/bloom_cas.rscrates/brokkr-cas/src/gc.rscrates/brokkr-cas/src/replicated.rscrates/brokkr-cas/src/tiered.rs
…s (issue jackthepunished#143) The Cas trait ships default no-op implementations of list_digests (returns []) and delete_blob (returns Ok(())). TieredCas, BloomCas, and ReplicatedCas did not override them, so gc::sweep run through any advertised composition reported Ok(0) while leaving every blob on disk, and delete_blob on a decorator silently no-oped (issue jackthepunished#143). Production overrides: - TieredCas::list_digests delegates to warm (hot is a cache, not authoritative). TieredCas::delete_blob drops from hot via a new HotTier::delete helper (mirrors the unlink+recycle path used by eviction), then delegates to warm. - BloomCas::list_digests delegates to inner. BloomCas::delete_blob delegates to inner and intentionally does NOT touch the bloom: a Bloom filter cannot support per-entry remove. The stale bit is harmless because find_missing_blobs always asks the backend on a bloom-yes answer; the next rebuild_from re-tightens. This matches the design intent in docs/phase-3-plan.md:281-283. - ReplicatedCas::delete_blob fans out to every replica the ring selected for the digest (no quorum — the trait contract is idempotent Ok(())). ReplicatedCas::list_digests unions every replica's enumeration, since per-digest primary selection is impossible without an input digest. A heal-in-progress can briefly over-report; the fan-out delete is idempotent on 'not found' so the over-report is benign. Tests: - Three regression tests per decorator (delete drops from hot+both-tiers / delegates to inner / fans out to replicas, missing-digest Ok(()) idempotence, list_digests delegation). - Three end-to-end sweep tests in gc.rs that would have failed pre-fix at the 'sweep returns 1' line (sweep_works_through_tiered/ bloom/replicated_decorator). - Existing gc::tests::sweep_is_idempotent remains the regression-checker for the idempotent Ok contract. CHANGELOG: one [Unreleased] ### Fixed entry. No new dependency, no public API change. Pre-existing nonzero_exit_is_not_cached macOS failure (/bin/false not present on macOS) is unrelated and reproduces identically on origin/main.
ee040a7 to
e483c23
Compare
1 participant
Motivation
Issue #143 —
Cas::list_digestsandCas::delete_blobshipdefault no-op implementations in the trait. The three Phase 3
decorator types —
TieredCas,BloomCas,ReplicatedCas— donot override them, so
gc::sweeprun through any advertisedcomposition deletes nothing yet reports
Ok(0). The diskfills while the operator is told GC is fine.
TieredCasalsohas no hot-tier invalidation path: a
delete_blobleaves thehot cache lying about presence until LRU eviction.
Fixes #143.
What changed
Single commit
fix(cas): override list_digests and delete_blob in decorator Cas impls (issue #143). Production overrides inthree files + a
HotTier::deletehelper, plus regressiontests.
crates/brokkr-cas/src/tiered.rs:HotTier::delete(&Digest) -> boolhelper (mirrorsevict_until_under_capacity's unlink+recycle path).TieredCas::list_digestsdelegates to warm (hot is a cache,not authoritative).
TieredCas::delete_blobdrops from hot first, then delegatesto warm.
crates/brokkr-cas/src/bloom_cas.rs:BloomCas::list_digestsdelegates to inner.BloomCas::delete_blobdelegates to inner and does nottouch the bloom — by design, per
docs/phase-3-plan.md:281-283. ABloomcannot supportper-entry remove; the stale bit is harmless because
find_missing_blobsalways asks the backend on a bloom-yes.The next
rebuild_fromre-tightens.crates/brokkr-cas/src/replicated.rs:ReplicatedCas::delete_blobfans out to every replica thering selected for the digest. No quorum — the trait contract
is idempotent
Ok(())and per-replicadelete_blobisitself idempotent.
ReplicatedCas::list_digestsunions every replica'senumeration, deduped. Per-digest primary selection is
impossible without an input digest, so the natural correct
answer is "ask every node". A heal-in-progress can briefly
over-report; the fan-out delete is idempotent on "not found"
so the over-report is benign.
crates/brokkr-cas/src/gc.rs:(
sweep_works_through_{tiered,bloom,replicated}_decorator)that run
sweepthrough each decorator and assert it returns1 (one unreachable digest) and the dead blob is gone from
the underlying store. Pre-fix these would have failed at the
sweep returns 1line.Three regression tests per decorator (delete drops from the
right place, missing-digest
Ok(())idempotence, list_digestsdelegation) — 9 unit tests in
tiered.rs/bloom_cas.rs/replicated.rs.CHANGELOG.md: one[Unreleased] ### Fixedentry.How it was tested
cargo fmt --all -- --check— clean.cargo clippy -p brokkr-cas --all-targets -- -D warnings—clean.
cargo test -p brokkr-cas --lib— 101/101 pass (12 new).cargo test -p brokkr-cas(incl. integration tests) — pass.cargo test --workspace— all green except thepre-existing
brokkr-control::tests::end_to_end::nonzero_exit_is_not_cachedmacOS failure (
/bin/falseis not on macOS). Verified toreproduce identically on
origin/main(nobrokkr-caschanges) so it is unrelated to this PR — per CLAUDE.md
hard rule 7, not fixing as part of this commit.
Related
docs/phase-3-plan.md§5.2 (the documented bloom-stalenessdesign intent this fix relies on)
docs/phase-3-plan.md§5.6 (peer-repair as the convergencemechanism for the replicated list_digests union)
surface)
Summary by CodeRabbit