Skip to content

[Storage] Make mutating contiguous journal methods &mut#3570

Open
danlaine wants to merge 3 commits into
mainfrom
danlaine/contiguous-2
Open

[Storage] Make mutating contiguous journal methods &mut#3570
danlaine wants to merge 3 commits into
mainfrom
danlaine/contiguous-2

Conversation

@danlaine
Copy link
Copy Markdown
Collaborator

@danlaine danlaine commented Apr 10, 2026

Motivation

The contiguous journal types (fixed::Journal, variable::Journal) and their consumers (merkle::Journaled, Metadata, Queue, qmdb::store::Db, etc.) previously used interior mutability (UpgradableAsyncRwLock, RwLock, AsyncMutex) to allow mutating methods like append, sync, prune, and rewind to take &self. This added locking overhead on every operation and obscured the actual mutability requirements at the type level.

This PR removes the interior-mutability wrappers and changes all mutating methods to take &mut self instead, letting the borrow checker enforce exclusive access statically.

Changes

Persistable trait (storage/src/lib.rs)

commit and sync now require &mut self.

fixed::Journal

  • Removed the Inner struct and UpgradableAsyncRwLock wrapper. Fields (journal, size, metadata, pruning_boundary, items_per_blob) are now directly on Journal.
  • append, append_many, sync, rewind, prune, clear_to_size now take &mut self.
  • size and pruning_boundary are now const fn (no lock acquisition, no async).
  • read, bounds, and replay are promoted from test/reader-only helpers to public methods on Journal directly.
  • The Reader type becomes a thin borrowed wrapper (&Journal) instead of holding a lock guard. It still implements the Reader trait for use via the Contiguous trait.
  • The lock downgrade/upgrade dance in append_many (for syncing full sections mid-append) is replaced by direct &mut self calls.
  • Removed the position_to_section helper (inlined).

variable::Journal

Parallel changes to fixed::Journal: removed Inner/UpgradableAsyncRwLock, flattened fields onto Journal, changed mutating methods to &mut self, made size synchronous.

merkle::Journaled

  • Removed Inner struct and RwLock wrapper. mem and pruned_to_pos are now direct fields.
  • Removed sync_lock: AsyncMutex (no longer needed since sync takes &mut self).
  • sync, prune, and related methods take &mut self.
  • root is now const fn.
  • All self.inner.read().mem / self.inner.get_mut().mem accesses become self.mem.

Metadata

  • Removed AsyncMutex around State. state is now a direct field.
  • sync takes &mut self.
  • Mutable accessors use &mut self.state instead of self.state.get_mut().

Queue

  • ack, ack_up_to, size, is_empty are no longer async (they were only async because of the lock).
  • Updated shared::Reader and shared::Writer wrappers accordingly.

qmdb::store::Db

  • commit, sync, prune take &mut self.
  • bounds and size are now const fn (synchronous).

journal::authenticated

  • commit and sync take &mut self.
  • sync can no longer use try_join! to sync journal and merkle concurrently (since both require &mut self), so they are now sequential.

@danlaine danlaine self-assigned this Apr 10, 2026
@danlaine danlaine added this to Tracker Apr 10, 2026
@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented Apr 10, 2026

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
commonware-mcp 376d5d6 May 08 2026, 01:44 PM

@danlaine danlaine moved this to Ready for Review in Tracker Apr 13, 2026
@danlaine danlaine marked this pull request as ready for review April 13, 2026 17:02
@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented Apr 17, 2026

Deploying monorepo with  Cloudflare Pages  Cloudflare Pages

Latest commit: 376d5d6
Status: ✅  Deploy successful!
Preview URL: https://20072431.monorepo-eu0.pages.dev
Branch Preview URL: https://danlaine-contiguous-2.monorepo-eu0.pages.dev

View logs

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 5cacf40. Configure here.

Comment thread storage/src/journal/authenticated.rs
@danlaine danlaine force-pushed the danlaine/contiguous-2 branch from 377e7c1 to c0e3b70 Compare April 29, 2026 18:48
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 6, 2026

Benchmark results

Tip

PASSED: No benchmark exceeded the regression threshold.

Benchmark comparison table
Benchmark Baseline (main) Current Delta Threshold Status
qmdb::merkleize/variant=any::unordered::fixed::mmr keys=10000 ch=false sync=false 1.670 ms 1.530 ms -8.37% 10.00% ✅ PASS
qmdb::merkleize/variant=current::ordered::fixed::mmb chunk=256 keys=10000 ch=true sync=false 2.921 ms 2.819 ms -3.50% 10.00% ✅ PASS

Baseline commit(s): b155ef4dd5ee

@danlaine danlaine force-pushed the danlaine/contiguous-2 branch from 12c60ae to 77c6202 Compare May 6, 2026 20:36
@danlaine danlaine requested a review from roberto-bayardo May 6, 2026 20:37
@codecov
Copy link
Copy Markdown

codecov Bot commented May 8, 2026

Codecov Report

❌ Patch coverage is 97.34807% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.81%. Comparing base (b155ef4) to head (376d5d6).

Files with missing lines Patch % Lines
storage/src/journal/contiguous/fixed.rs 98.45% 2 Missing and 3 partials ⚠️
storage/src/ordinal/storage.rs 66.66% 4 Missing ⚠️
storage/src/journal/authenticated.rs 83.33% 3 Missing ⚠️
storage/src/journal/contiguous/variable.rs 98.95% 1 Missing and 2 partials ⚠️
storage/src/merkle/persisted/full.rs 95.38% 3 Missing ⚠️
storage/src/qmdb/store/db.rs 93.47% 3 Missing ⚠️
storage/src/queue/shared.rs 66.66% 2 Missing ⚠️
storage/src/lib.rs 0.00% 1 Missing ⚠️
@@            Coverage Diff             @@
##             main    #3570      +/-   ##
==========================================
- Coverage   95.81%   95.81%   -0.01%     
==========================================
  Files         466      466              
  Lines      185899   185720     -179     
  Branches     4443     4442       -1     
==========================================
- Hits       178119   177939     -180     
- Misses       6356     6358       +2     
+ Partials     1424     1423       -1     
Files with missing lines Coverage Δ
storage/src/metadata/storage.rs 90.53% <100.00%> (-0.18%) ⬇️
storage/src/ordinal/mod.rs 100.00% <100.00%> (ø)
storage/src/qmdb/any/db.rs 92.51% <100.00%> (ø)
storage/src/qmdb/any/mod.rs 99.28% <100.00%> (-0.01%) ⬇️
storage/src/qmdb/any/sync/mod.rs 97.36% <100.00%> (ø)
storage/src/qmdb/current/db.rs 96.17% <100.00%> (ø)
storage/src/qmdb/current/mod.rs 98.97% <100.00%> (-0.01%) ⬇️
storage/src/qmdb/current/sync/mod.rs 85.59% <100.00%> (ø)
storage/src/qmdb/immutable/mod.rs 97.42% <100.00%> (ø)
storage/src/qmdb/immutable/sync/mod.rs 98.19% <100.00%> (ø)
... and 13 more

... and 7 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b155ef4...376d5d6. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Ready for Review

Development

Successfully merging this pull request may close these issues.

1 participant