Skip to content

refactor(cold): require &mut self for ColdStorage write methods#42

Open
prestwich wants to merge 1 commit intomainfrom
prestwich/eng-1979-cold-storage-mut-writes
Open

refactor(cold): require &mut self for ColdStorage write methods#42
prestwich wants to merge 1 commit intomainfrom
prestwich/eng-1979-cold-storage-mut-writes

Conversation

@prestwich
Copy link
Member

Summary

  • Change ColdStorage write methods (append_block, append_blocks, truncate_above, drain_above) from &self to &mut self so the borrow checker enforces exclusive write access at compile time
  • Add Clone as a supertrait bound on ColdStorage (all backends are cheaply clonable via Arc-wrapped internals)
  • Wrap the task runner's backend in tokio::sync::RwLock<B> — reads acquire the read lock, writes acquire the write lock
  • Stream producers clone the backend to avoid holding the read lock during long-lived streams (up to 60s)

Closes ENG-1979

Test plan

  • cargo clippy clean on all 4 affected crates with both --all-features and --no-default-features
  • mem_backend_conformance passes
  • mdbx_backend_conformance passes
  • All unified storage integration tests pass (including drain_above_*)
  • ./scripts/test-postgres.sh (requires Postgres)

🤖 Generated with Claude Code

…1979)

Change append_block, append_blocks, truncate_above, and drain_above
from &self to &mut self so the borrow checker enforces exclusive write
access at compile time. Add Clone as a supertrait bound.

Adapt the task runner by wrapping the backend in tokio::sync::RwLock<B>.
Reads acquire the read lock, writes acquire the write lock, and stream
producers clone the backend to avoid holding the lock during long-lived
streams.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@prestwich prestwich requested a review from Fraser999 March 24, 2026 17:27
Copy link
Contributor

Choose a reason for hiding this comment

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

Committed by mistake?

Copy link
Contributor

Choose a reason for hiding this comment

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

Committed by mistake?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should impl ColdStorage for EitherCold also include an override for fn drain_above?

/// that write.
///
pub trait ColdStorage: Send + Sync + 'static {
pub trait ColdStorage: Clone + Send + Sync + 'static {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might be helpful to split the single trait into ColdStorageRead: Clone + Send + Sync + 'static and ColdStorageWrite: Send + 'static.

We can then avoid the task runner having to hold the backend in a RwLock: it would hold a single, non-cloneable instance of the backend as the write portion, and would have a cloneable instance of the backend as the read portion.

This would help enforce the architectural intent: the task runner is the only one that's capable of calling write methods, and the read tasks and stream producers receive a clone of the ColdStorageRead, which ensures they can't call any write methods (we'd only have impl<B: ColdStorageRead> ColdStorageTaskInner<B> { ... } for the read-only tasks.)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants