From 1e924f87f704a01e9d9909dcf47311ba93892e4d Mon Sep 17 00:00:00 2001 From: Martin Bruse Date: Mon, 7 Sep 2026 08:14:55 +0200 Subject: [PATCH] fix(storage): forward on_piece_completed through the storage middlewares A storage middleware is transparent: it has an opinion about a couple of methods and everything else has to reach the storage underneath. The three in storage::middleware override pread/pwrite/remove/ensure_len/take/remove_dir/init and stop there, so on_piece_completed - the one method with a default that a storage is likely to implement - never got past the middleware. Its default does nothing, which is exactly the wrong thing to do on a storage's behalf. It is the callback a storage uses to make a piece durable or visible once it is written and hash-checked: write it under a temporary name and move it into place, flush it, promote it out of a staging area. Wrap such a storage in TimingStorage to find out why it is slow, or in the write-through cache, and the call goes to the middleware's default instead - so nothing is ever promoted, and the storage quietly stops working in the one configuration you added the middleware to debug. Forward it in all three, and check every wrapper around a storage against a probe that records the completions it was told about, so one that doesn't forward is caught. Box already forwards it and now says so in a test: what a torrent holds is a Box, so a method it drops is one no storage ever gets asked. Co-Authored-By: Claude Opus 5 --- .../librqbit/src/storage/middleware/slow.rs | 21 +++++ .../librqbit/src/storage/middleware/timing.rs | 21 +++++ .../storage/middleware/write_through_cache.rs | 29 +++++++ crates/librqbit/src/storage/mod.rs | 15 +++- crates/librqbit/src/storage/test_util.rs | 84 +++++++++++++++++++ 5 files changed, 169 insertions(+), 1 deletion(-) create mode 100644 crates/librqbit/src/storage/test_util.rs diff --git a/crates/librqbit/src/storage/middleware/slow.rs b/crates/librqbit/src/storage/middleware/slow.rs index e8c8571bd..2a269ba95 100644 --- a/crates/librqbit/src/storage/middleware/slow.rs +++ b/crates/librqbit/src/storage/middleware/slow.rs @@ -12,6 +12,7 @@ use std::{ time::Duration, }; +use librqbit_core::lengths::ValidPieceIndex; use parking_lot::Mutex; use crate::{ @@ -128,4 +129,24 @@ impl TorrentStorage for SlowStorage { ) -> anyhow::Result<()> { self.underlying.init(shared, metadata) } + + fn on_piece_completed(&self, piece_index: ValidPieceIndex) -> anyhow::Result<()> { + self.underlying.on_piece_completed(piece_index) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::storage::test_util::{Probe, assert_forwards_defaults}; + + #[test] + fn test_the_defaulted_methods_reach_the_underlying_storage() { + let storage = SlowStorage { + underlying: Probe::default(), + pwrite_all_bufread: Mutex::new(Box::new(std::iter::empty())), + pread_exact_bufread: Mutex::new(Box::new(std::iter::empty())), + }; + assert_forwards_defaults(&storage, &storage.underlying); + } } diff --git a/crates/librqbit/src/storage/middleware/timing.rs b/crates/librqbit/src/storage/middleware/timing.rs index e71e490b9..2dca7deaa 100644 --- a/crates/librqbit/src/storage/middleware/timing.rs +++ b/crates/librqbit/src/storage/middleware/timing.rs @@ -2,6 +2,8 @@ A storage middleware that logs the time underlying storage operations took. */ +use librqbit_core::lengths::ValidPieceIndex; + use crate::{ ManagedTorrentShared, storage::{StorageFactory, StorageFactoryExt, TorrentStorage}, @@ -116,4 +118,23 @@ impl TorrentStorage for TimingStorage { ) -> anyhow::Result<()> { self.underlying.init(shared, metadata) } + + fn on_piece_completed(&self, piece_index: ValidPieceIndex) -> anyhow::Result<()> { + self.underlying.on_piece_completed(piece_index) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::storage::test_util::{Probe, assert_forwards_defaults}; + + #[test] + fn test_the_defaulted_methods_reach_the_underlying_storage() { + let storage = TimingStorage { + name: "test".to_owned(), + underlying: Probe::default(), + }; + assert_forwards_defaults(&storage, &storage.underlying); + } } diff --git a/crates/librqbit/src/storage/middleware/write_through_cache.rs b/crates/librqbit/src/storage/middleware/write_through_cache.rs index 1dd0e4f7f..0ec10a56b 100644 --- a/crates/librqbit/src/storage/middleware/write_through_cache.rs +++ b/crates/librqbit/src/storage/middleware/write_through_cache.rs @@ -137,4 +137,33 @@ impl TorrentStorage for WriteThroughCacheStorage { ) -> anyhow::Result<()> { self.underlying.init(shared, metadata) } + + fn on_piece_completed(&self, piece_index: ValidPieceIndex) -> anyhow::Result<()> { + self.underlying.on_piece_completed(piece_index) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::storage::test_util::{ + NUM_PIECES, PIECE_LEN, Probe, assert_forwards_defaults, lengths, + }; + + #[test] + fn test_the_defaulted_methods_reach_the_underlying_storage() { + let storage = WriteThroughCacheStorage { + lru: RwLock::new(LruCache::new(NonZeroUsize::new(1).unwrap())), + lengths: lengths(), + file_infos: vec![crate::file_info::FileInfo { + relative_filename: "test.dat".into(), + offset_in_torrent: 0, + len: PIECE_LEN as u64 * NUM_PIECES as u64, + piece_range: 0..NUM_PIECES, + attrs: Default::default(), + }], + underlying: Probe::default(), + }; + assert_forwards_defaults(&storage, &storage.underlying); + } } diff --git a/crates/librqbit/src/storage/mod.rs b/crates/librqbit/src/storage/mod.rs index 0ef696f53..53030fb93 100644 --- a/crates/librqbit/src/storage/mod.rs +++ b/crates/librqbit/src/storage/mod.rs @@ -30,6 +30,9 @@ pub mod examples; #[cfg(feature = "storage_middleware")] pub mod middleware; +#[cfg(test)] +pub(crate) mod test_util; + use std::{ any::{Any, TypeId}, io::IoSlice, @@ -222,7 +225,9 @@ mod tests { use std::any::TypeId; use super::{ - BoxStorageFactory, StorageFactory, StorageFactoryExt, filesystem::FilesystemStorageFactory, + BoxStorageFactory, StorageFactory, StorageFactoryExt, + filesystem::FilesystemStorageFactory, + test_util::{Probe, assert_forwards_defaults}, }; use crate::torrent_state::{ManagedTorrentShared, TorrentMetadata}; @@ -279,4 +284,12 @@ mod tests { .is_type_id(TypeId::of::>()) ); } + + // What a torrent holds is a Box, so a method this impl doesn't + // forward is one no storage in rqbit ever gets asked. + #[test] + fn test_the_defaulted_methods_survive_boxing() { + let boxed: Box = Box::default(); + assert_forwards_defaults(&boxed, &boxed); + } } diff --git a/crates/librqbit/src/storage/test_util.rs b/crates/librqbit/src/storage/test_util.rs new file mode 100644 index 000000000..159aa4f21 --- /dev/null +++ b/crates/librqbit/src/storage/test_util.rs @@ -0,0 +1,84 @@ +// A storage that records what reached it, to check the wrappers around one against. +// +// A middleware is transparent: anything it has no opinion about has to reach what it +// wraps. The methods that make that easy to get wrong are the ones [`TorrentStorage`] +// gives a default, because forgetting one still compiles and then answers something +// plausible of its own instead of asking the storage underneath. + +use std::path::Path; + +use librqbit_core::{ + constants::CHUNK_SIZE, + lengths::{Lengths, ValidPieceIndex}, +}; +use parking_lot::Mutex; + +use crate::{ManagedTorrentShared, TorrentMetadata, storage::TorrentStorage}; + +pub(crate) const PIECE_LEN: u32 = CHUNK_SIZE * 2; +pub(crate) const NUM_PIECES: u32 = 2; + +pub(crate) fn lengths() -> Lengths { + Lengths::new(PIECE_LEN as u64 * NUM_PIECES as u64, PIECE_LEN).unwrap() +} + +pub(crate) fn piece(id: u32) -> ValidPieceIndex { + lengths().validate_piece_index(id).unwrap() +} + +#[derive(Default)] +pub(crate) struct Probe { + /// The pieces on_piece_completed() was called for, in order. + pub completed: Mutex>, +} + +impl TorrentStorage for Probe { + fn init( + &mut self, + _shared: &ManagedTorrentShared, + _metadata: &TorrentMetadata, + ) -> anyhow::Result<()> { + Ok(()) + } + + fn pread_exact(&self, _file_id: usize, _offset: u64, _buf: &mut [u8]) -> anyhow::Result<()> { + Ok(()) + } + + fn pwrite_all(&self, _file_id: usize, _offset: u64, _buf: &[u8]) -> anyhow::Result<()> { + Ok(()) + } + + fn remove_file(&self, _file_id: usize, _filename: &Path) -> anyhow::Result<()> { + Ok(()) + } + + fn remove_directory_if_empty(&self, _path: &Path) -> anyhow::Result<()> { + Ok(()) + } + + fn ensure_file_length(&self, _file_id: usize, _length: u64) -> anyhow::Result<()> { + Ok(()) + } + + fn take(&self) -> anyhow::Result> { + anyhow::bail!("not used") + } + + fn on_piece_completed(&self, piece_index: ValidPieceIndex) -> anyhow::Result<()> { + self.completed.lock().push(piece_index.get()); + Ok(()) + } +} + +// Everything a wrapper has to pass through to the storage it wraps and can't decide on +// its own. Called from each middleware's own tests, where its private fields are. +pub(crate) fn assert_forwards_defaults(storage: &S, probe: &Probe) { + storage.on_piece_completed(piece(1)).unwrap(); + assert_eq!( + *probe.completed.lock(), + vec![1], + "on_piece_completed() didn't reach the storage: a store that makes a piece \ + visible there never gets told" + ); +}