[storage/journal] prevent read-blocking during contiguous journal prune#3812
Open
roberto-bayardo wants to merge 3 commits into
Open
[storage/journal] prevent read-blocking during contiguous journal prune#3812roberto-bayardo wants to merge 3 commits into
roberto-bayardo wants to merge 3 commits into
Conversation
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
commonware-mcp | 19c9731 | May 20 2026, 11:37 PM |
Benchmark resultsTip ✅ PASSED: No benchmark exceeded the regression threshold. Benchmark comparison table
Baseline commit(s): |
Contributor
There was a problem hiding this comment.
Pull request overview
Splits the fixed segmented journal's prune into a two-phase operation (unlink_before + commit_prune) so that the contiguous fixed journal can perform the slow storage remove calls under an upgradable read lock, allowing concurrent readers to continue using already-open section handles during physical removal. The runtime Storage::remove contract is updated to require that existing Blob handles remain readable after a named remove.
Changes:
- Introduce
unlink_before/commit_pruneonManager(and re-expose them on segmentedJournal), with the existing one-shotprunereimplemented on top. - Rework
contiguous::fixed::Journal::pruneto downgrade the write lock to upgradable during unlink and re-upgrade only for the in-memory commit. - Add a
BlockingContexttest harness plus two new tests covering reader concurrency during prune and remove-failure recovery; document the newremovecontract in the runtime crate.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| storage/src/journal/segmented/manager.rs | Splits prune into unlink_before (storage-only) and commit_prune (map mutation). |
| storage/src/journal/segmented/fixed.rs | Re-exports the two-phase prune to the contiguous layer. |
| storage/src/journal/contiguous/fixed.rs | Uses upgradable-read downgrade/upgrade around unlink; adds BlockingContext and two new tests. |
| storage/Cargo.toml | Adds governor dev-dependency used by BlockingContext's clock impls. |
| runtime/src/lib.rs | Documents that named remove must not invalidate already-open Blob handles. |
| Cargo.lock | Records new governor dependency for the storage crate. |
Comments suppressed due to low confidence (1)
storage/src/journal/segmented/manager.rs:279
commit_pruneis exposed atpub(super)/pub(in crate::journal)and unconditionally removes entries belowminfromself.blobswithout verifying thatunlink_beforewas previously called for the samemin. If a caller invokescommit_prunewithout a matching successfulunlink_before(e.g. in a future refactor), the blobs are silently dropped from the map while remaining present in storage, leaking them and potentially confusing re-init. The doc comment notes the precondition, but nothing enforces it. Consider, at minimum, adebug_assert!that the range is empty in storage, or restructuring the API so the two steps cannot be invoked independently.
/// Commit a successful unlink by removing sections less than `min` from the in-memory map.
///
/// This must only be called after [Self::unlink_before] succeeds for the same `min`.
/// Returns true if any section handles were removed.
pub(super) fn commit_prune(&mut self, min: u64) -> bool {
let sections: Vec<u64> = self
.blobs
.range(..min)
.map(|(§ion, _)| section)
.collect();
let pruned = !sections.is_empty();
for section in sections {
let blob = self.blobs.remove(§ion).unwrap();
drop(blob);
debug!(section, "pruned blob");
self.tracked.dec();
self.pruned.inc();
}
if pruned {
self.oldest_retained_section = min;
}
pruned
}
Comment on lines
707
to
712
| /// If no `name` is provided, the entire partition is removed. | ||
| /// If a `name` is provided, existing [Blob] handles for that blob must remain readable | ||
| /// until they are dropped, but the blob must be removed from future namespace lookups. | ||
| /// | ||
| /// An Ok result indicates the blob is durably removed. | ||
| fn remove( |
Collaborator
Author
There was a problem hiding this comment.
Added a test to confirm this behavior that should run on all storage backends.
roberto-bayardo
force-pushed
the
non-read-blocking-prune
branch
from
6df45ae to
8b5dd16
Compare
Deploying monorepo with
|
| Latest commit: |
19c9731
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://329b83e5.monorepo-eu0.pages.dev |
| Branch Preview URL: | https://non-read-blocking-prune.monorepo-eu0.pages.dev |
roberto-bayardo
force-pushed
the
non-read-blocking-prune
branch
4 times, most recently
from
9e8f5f5 to
79b40ed
Compare
Collaborator
Author
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 79b40ed. Configure here.
roberto-bayardo
force-pushed
the
non-read-blocking-prune
branch
3 times, most recently
from
6996cb5 to
66f9a7b
Compare
roberto-bayardo
force-pushed
the
non-read-blocking-prune
branch
from
66f9a7b to
62299e4
Compare
roberto-bayardo
force-pushed
the
non-read-blocking-prune
branch
from
9efe6bb to
36eb3f4
Compare
roberto-bayardo
changed the title
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #3812 +/- ##
==========================================
- Coverage 95.72% 95.70% -0.02%
==========================================
Files 472 473 +1
Lines 189348 193373 +4025
Branches 4597 4680 +83
==========================================
+ Hits 181256 185073 +3817
- Misses 6568 6728 +160
- Partials 1524 1572 +48
... and 49 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
prune()to serialize with mutators via an upgradable read guard, then upgrade only for the short in-memory commit.Storage::removecontract for already-open blob handles.Context
#2857
Testing
just test -p commonware-storage test_fixed_journal_reads_during_prune_unlink test_fixed_journal_prune_remove_failure_reopens_contiguous test_fixed_journal_partial_prune_remove_failure_reopens_suffixjust test -p commonware-storage journal::contiguous::fixedjust test -p commonware-storage journal::segmented