From bd148b20f7aecac2e26855740fc73945b62147f3 Mon Sep 17 00:00:00 2001 From: cxymds Date: Fri, 17 Jul 2026 15:00:09 +0800 Subject: [PATCH] fix(ecstore): add fallible erasure construction --- crates/ecstore/src/api/mod.rs | 4 +- crates/ecstore/src/disk/error.rs | 28 +- crates/ecstore/src/disk/local.rs | 31 +- crates/ecstore/src/erasure/coding/encode.rs | 10 +- crates/ecstore/src/erasure/coding/erasure.rs | 324 +++++++++++++++++- crates/ecstore/src/erasure/coding/mod.rs | 2 +- crates/ecstore/src/error/mod.rs | 28 +- crates/ecstore/src/set_disk/ops/heal.rs | 3 +- crates/ecstore/src/set_disk/ops/multipart.rs | 3 +- crates/ecstore/src/set_disk/ops/object.rs | 38 +- crates/ecstore/src/set_disk/read.rs | 44 +-- scripts/check_architecture_migration_rules.sh | 49 +++ 12 files changed, 501 insertions(+), 63 deletions(-) diff --git a/crates/ecstore/src/api/mod.rs b/crates/ecstore/src/api/mod.rs index d0cbfb23c..84e6b9c76 100644 --- a/crates/ecstore/src/api/mod.rs +++ b/crates/ecstore/src/api/mod.rs @@ -308,8 +308,8 @@ pub mod error { pub mod erasure { pub use crate::erasure::coding::{ - BitrotReader, BitrotWriter, BitrotWriterWrapper, CustomWriter, Erasure, ReedSolomonEncoder, calc_shard_size, - calc_shard_size_legacy, + BitrotReader, BitrotWriter, BitrotWriterWrapper, CustomWriter, Erasure, ErasureConstructionError, ReedSolomonEncoder, + calc_shard_size, calc_shard_size_legacy, }; } diff --git a/crates/ecstore/src/disk/error.rs b/crates/ecstore/src/disk/error.rs index 606ec7456..bb6c07b5f 100644 --- a/crates/ecstore/src/disk/error.rs +++ b/crates/ecstore/src/disk/error.rs @@ -141,7 +141,7 @@ pub enum DiskError { ErasureReadQuorum, #[error("io error {0}")] - Io(io::Error), + Io(#[source] io::Error), #[error("source stalled")] SourceStalled, @@ -153,6 +153,12 @@ pub enum DiskError { InvalidPath, } +impl From for DiskError { + fn from(error: crate::erasure::coding::ErasureConstructionError) -> Self { + Self::Io(error.into_io_error()) + } +} + impl DiskError { pub fn other(error: E) -> Self where @@ -601,6 +607,26 @@ mod tests { use super::*; use std::collections::HashMap; + #[test] + fn other_preserves_erasure_construction_source_chain() { + use crate::erasure::coding::ErasureConstructionError; + use std::error::Error as _; + + let error = DiskError::from(ErasureConstructionError::ModernEncoder { + source: reed_solomon_erasure::Error::TooManyShards, + }); + let io_source = error.source().expect("DiskError::Io must expose its io::Error source"); + assert!(io_source.is::()); + let construction_source = io_source + .source() + .expect("io::Error must expose the erasure construction error"); + assert!(construction_source.is::()); + let encoder_source = construction_source + .source() + .expect("construction error must expose the encoder error"); + assert!(encoder_source.is::()); + } + #[test] fn test_disk_error_variants() { let errors = vec![ diff --git a/crates/ecstore/src/disk/local.rs b/crates/ecstore/src/disk/local.rs index 17d4e3579..f81fdcaeb 100644 --- a/crates/ecstore/src/disk/local.rs +++ b/crates/ecstore/src/disk/local.rs @@ -5938,12 +5938,13 @@ impl DiskAPI for LocalDisk { }; let erasure = &fi.erasure; - let codec_erasure = coding::Erasure::new_with_options( + let codec_erasure = coding::Erasure::try_new_with_options( erasure.data_blocks, erasure.parity_blocks, erasure.block_size, fi.uses_legacy_checksum, - ); + ) + .map_err(DiskError::from)?; for (i, part) in fi.parts.iter().enumerate() { let checksum_info = erasure.get_checksum_info(part.number); let checksum_algo = if fi.uses_legacy_checksum && checksum_info.algorithm == HashAlgorithm::HighwayHash256S { @@ -13532,6 +13533,32 @@ mod test { assert!(!is_bitrot_verification_error(&io::Error::other("unrelated io failure"))); } + #[tokio::test] + async fn local_disk_verify_file_preserves_erasure_construction_error() { + use crate::erasure::coding::ErasureConstructionError; + use tempfile::tempdir; + + let root_dir = tempdir().expect("temp dir should be created"); + let endpoint = Endpoint::try_from(root_dir.path().to_string_lossy().as_ref()).expect("endpoint should parse"); + let disk = LocalDisk::new(&endpoint, false).await.expect("local disk should be created"); + let volume = "verify-volume"; + ensure_test_volume(&disk, volume).await; + + let mut file_info = FileInfo::new("invalid.bin", 2, 2); + file_info.erasure.block_size = 0; + let error = match disk.verify_file(volume, "invalid.bin", &file_info).await { + Ok(_) => panic!("invalid local-disk erasure metadata must be rejected"), + Err(error) => error, + }; + + assert!(error.to_string().contains("block_size must be greater than zero")); + let io_source = std::error::Error::source(&error).expect("DiskError::Io must expose its io::Error source"); + let construction_source = io_source + .source() + .expect("io::Error must expose the erasure construction error"); + assert!(construction_source.is::()); + } + #[tokio::test] async fn local_disk_read_file_verifier_reports_bitrot_mismatch() { use crate::erasure::coding::BitrotWriter; diff --git a/crates/ecstore/src/erasure/coding/encode.rs b/crates/ecstore/src/erasure/coding/encode.rs index 8509275b8..117f55da8 100644 --- a/crates/ecstore/src/erasure/coding/encode.rs +++ b/crates/ecstore/src/erasure/coding/encode.rs @@ -841,6 +841,12 @@ mod tests { use std::time::Duration; use tokio::io::{AsyncWrite, AsyncWriteExt}; + fn erasure_with_zero_block_size() -> Erasure { + let mut erasure = Erasure::default(); + erasure.data_shards = 1; + erasure + } + #[derive(Clone, Default)] struct DeferredCommitWriter { buffered: Vec, @@ -1500,7 +1506,7 @@ mod tests { HashAlgorithm::HighwayHash256S, ))]; - let erasure = Arc::new(Erasure::new(1, 0, 0)); + let erasure = Arc::new(erasure_with_zero_block_size()); let reader = tokio::io::BufReader::new(Cursor::new(b"payload".to_vec())); let err = erasure .encode(reader, &mut writers, 1) @@ -1660,7 +1666,7 @@ mod tests { let writer = DeferredCommitWriter::new(committed); let mut writers = vec![Some(bitrot_writer(writer, 16))]; - let erasure = Arc::new(Erasure::new(1, 0, 0)); + let erasure = Arc::new(erasure_with_zero_block_size()); let reader = tokio::io::BufReader::new(Cursor::new(b"payload".to_vec())); let err = erasure .encode_batched(reader, &mut writers, 1) diff --git a/crates/ecstore/src/erasure/coding/erasure.rs b/crates/ecstore/src/erasure/coding/erasure.rs index a3f334f9a..c7c0511bc 100644 --- a/crates/ecstore/src/erasure/coding/erasure.rs +++ b/crates/ecstore/src/erasure/coding/erasure.rs @@ -25,6 +25,62 @@ use tokio::io::AsyncRead; use tracing::warn; use uuid::Uuid; +const MODERN_MAX_TOTAL_SHARDS: usize = ::ORDER; + +/// Errors returned when constructing an [`Erasure`] codec. +#[derive(Debug, thiserror::Error)] +pub enum ErasureConstructionError { + /// Erasure coding requires at least one data shard. + #[error("data_shards must be greater than zero")] + ZeroDataShards, + + /// Erasure coding requires a non-zero logical block size. + #[error("block_size must be greater than zero")] + ZeroBlockSize, + + /// The total shard count cannot be represented by `usize`. + #[error("data_shards ({data_shards}) + parity_shards ({parity_shards}) overflows usize")] + ShardCountOverflow { data_shards: usize, parity_shards: usize }, + + /// The modern GF(2^8) codec cannot represent this shard configuration. + #[error("modern codec does not support {data_shards} data shards and {parity_shards} parity shards")] + UnsupportedModernShardCount { data_shards: usize, parity_shards: usize }, + + /// The legacy SIMD codec cannot represent this shard configuration. + #[error("legacy codec does not support {data_shards} data shards and {parity_shards} parity shards")] + UnsupportedLegacyShardCount { data_shards: usize, parity_shards: usize }, + + /// The modern encoder rejected dimensions that passed the preflight checks. + #[error("failed to construct modern Reed-Solomon encoder")] + ModernEncoder { + #[source] + source: reed_solomon_erasure::Error, + }, + + /// The legacy encoder wrapper failed to initialize. + #[error("failed to construct legacy Reed-Solomon encoder")] + LegacyEncoder { + #[source] + source: io::Error, + }, +} + +#[derive(Debug, thiserror::Error)] +#[error("{source}")] +struct ErasureConstructionIoSource { + #[source] + source: ErasureConstructionError, +} + +impl ErasureConstructionError { + pub(crate) fn into_io_error(self) -> io::Error { + // `io::Error::source` forwards to the wrapped error's source rather + // than exposing the wrapped error itself. This adapter keeps the + // construction error as an observable link in the source chain. + io::Error::other(ErasureConstructionIoSource { source: self }) + } +} + /// Legacy calc_shard_size formula: (block_size.div_ceil(data_shards) + 1) & !1 /// Matches main branch and filemeta::ErasureInfo for old-version files. pub fn calc_shard_size_legacy(block_size: usize, data_shards: usize) -> usize { @@ -233,12 +289,9 @@ impl Clone for ReedSolomonEncoder { } impl ReedSolomonEncoder { - /// Create a new Reed-Solomon encoder with specified data and parity shards. - pub fn new(data_shards: usize, parity_shards: usize) -> io::Result { + fn try_new_typed(data_shards: usize, parity_shards: usize) -> Result { let encoder = if parity_shards > 0 { - ReedSolomon::new(data_shards, parity_shards) - .map_err(|e| io::Error::other(format!("Failed to create Reed-Solomon encoder: {e:?}"))) - .map(Some)? + Some(ReedSolomon::new(data_shards, parity_shards)?) } else { None }; @@ -250,6 +303,12 @@ impl ReedSolomonEncoder { }) } + /// Create a new Reed-Solomon encoder with specified data and parity shards. + pub fn new(data_shards: usize, parity_shards: usize) -> io::Result { + Self::try_new_typed(data_shards, parity_shards) + .map_err(|error| io::Error::other(format!("Failed to create Reed-Solomon encoder: {error:?}"))) + } + /// Encode data shards with parity. pub fn encode(&self, shards: SmallVec<[&mut [u8]; 16]>) -> io::Result<()> { let mut shards_vec: Vec<&mut [u8]> = shards.into_vec(); @@ -464,28 +523,105 @@ impl Erasure { /// * `data_shards` - Number of data shards. /// * `parity_shards` - Number of parity shards. /// * `block_size` - Block size for each shard. + /// + /// # Panics + /// Panics when the dimensions are invalid or unsupported. RustFS production + /// paths use [`Self::try_new`] instead. pub fn new(data_shards: usize, parity_shards: usize, block_size: usize) -> Self { - Self::new_with_options(data_shards, parity_shards, block_size, false) + match Self::try_new(data_shards, parity_shards, block_size) { + Ok(erasure) => erasure, + Err(error) => panic!("invalid erasure codec configuration: {error}"), + } } /// Create a new Erasure instance with legacy format support. /// /// When `uses_legacy` is true, uses main-branch shard_size formula and reed-solomon-simd /// for decode/reconstruct (for reading and healing old-version files). + /// + /// # Panics + /// Panics when the dimensions are invalid or unsupported. RustFS production + /// paths use [`Self::try_new_with_options`] instead. pub fn new_with_options(data_shards: usize, parity_shards: usize, block_size: usize, uses_legacy: bool) -> Self { + match Self::try_new_with_options(data_shards, parity_shards, block_size, uses_legacy) { + Ok(erasure) => erasure, + Err(error) => panic!("invalid erasure codec configuration: {error}"), + } + } + + /// Tries to create a modern Erasure codec. + /// + /// # Errors + /// Returns [`ErasureConstructionError`] when dimensions are zero, + /// overflow the shard count, exceed the modern codec's supported range, or + /// the underlying encoder rejects the configuration. + pub fn try_new(data_shards: usize, parity_shards: usize, block_size: usize) -> Result { + Self::try_new_with_options(data_shards, parity_shards, block_size, false) + } + + /// Tries to create an Erasure codec using the selected format backend. + /// + /// # Errors + /// Returns [`ErasureConstructionError`] when dimensions are zero, + /// overflow the shard count, are unsupported by the selected codec, or the + /// selected encoder fails to initialize. + pub fn try_new_with_options( + data_shards: usize, + parity_shards: usize, + block_size: usize, + uses_legacy: bool, + ) -> Result { + if data_shards == 0 { + return Err(ErasureConstructionError::ZeroDataShards); + } + if block_size == 0 { + return Err(ErasureConstructionError::ZeroBlockSize); + } + + let total_shards = data_shards + .checked_add(parity_shards) + .ok_or(ErasureConstructionError::ShardCountOverflow { + data_shards, + parity_shards, + })?; + + if parity_shards > 0 { + if uses_legacy + && (total_shards > reed_solomon_simd::engine::GF_ORDER + || !reed_solomon_simd::ReedSolomonEncoder::supports(data_shards, parity_shards)) + { + return Err(ErasureConstructionError::UnsupportedLegacyShardCount { + data_shards, + parity_shards, + }); + } + if !uses_legacy && total_shards > MODERN_MAX_TOTAL_SHARDS { + return Err(ErasureConstructionError::UnsupportedModernShardCount { + data_shards, + parity_shards, + }); + } + } + let encoder = if !uses_legacy && parity_shards > 0 { - Some(ReedSolomonEncoder::new(data_shards, parity_shards).expect("operation should succeed")) + Some( + ReedSolomonEncoder::try_new_typed(data_shards, parity_shards) + .map_err(|source| ErasureConstructionError::ModernEncoder { source })?, + ) } else { None }; let legacy_encoder = if uses_legacy && parity_shards > 0 { - Some(LegacyReedSolomonEncoder::new(data_shards, parity_shards).expect("operation should succeed")) + Some( + LegacyReedSolomonEncoder::new(data_shards, parity_shards) + .map_err(|source| ErasureConstructionError::LegacyEncoder { source })?, + ) } else { None }; - Erasure { + Ok(Erasure { data_shards, parity_shards, block_size, @@ -493,7 +629,7 @@ impl Erasure { legacy_encoder, uses_legacy, _id: Uuid::nil(), // Unused in hot paths; avoid CSPRNG syscall - } + }) } /// Encode data into data and parity shards. @@ -946,6 +1082,14 @@ mod tests { use std::task::{Context, Poll}; use tokio::io::ReadBuf; + fn erasure_with_invalid_dimensions(data_shards: usize, parity_shards: usize, block_size: usize) -> Erasure { + let mut erasure = Erasure::default(); + erasure.data_shards = data_shards; + erasure.parity_shards = parity_shards; + erasure.block_size = block_size; + erasure + } + fn optional_shards(shards: &[Bytes]) -> Vec>> { shards.iter().map(|shard| Some(shard.to_vec())).collect() } @@ -1010,6 +1154,156 @@ mod tests { } } + #[test] + fn try_new_rejects_zero_and_overflowing_dimensions_with_typed_errors() { + let Err(zero_data) = Erasure::try_new(0, 2, 64) else { + panic!("zero data shards must be rejected"); + }; + assert!(matches!(zero_data, ErasureConstructionError::ZeroDataShards)); + + let Err(zero_block) = Erasure::try_new(2, 2, 0) else { + panic!("zero block size must be rejected"); + }; + assert!(matches!(zero_block, ErasureConstructionError::ZeroBlockSize)); + + let Err(overflow) = Erasure::try_new(usize::MAX, 1, 64) else { + panic!("overflowing shard counts must be rejected"); + }; + assert!(matches!( + overflow, + ErasureConstructionError::ShardCountOverflow { + data_shards: usize::MAX, + parity_shards: 1, + } + )); + } + + #[test] + fn try_new_rejects_backend_specific_unsupported_dimensions() { + let Err(modern) = Erasure::try_new(256, 1, 64) else { + panic!("modern dimensions beyond GF(2^8) must be rejected"); + }; + assert!(matches!( + modern, + ErasureConstructionError::UnsupportedModernShardCount { + data_shards: 256, + parity_shards: 1, + } + )); + + let Err(legacy) = Erasure::try_new_with_options(60_000, 5_000, 64, true) else { + panic!("unsupported legacy SIMD dimensions must be rejected"); + }; + assert!(matches!( + legacy, + ErasureConstructionError::UnsupportedLegacyShardCount { + data_shards: 60_000, + parity_shards: 5_000, + } + )); + } + + #[test] + fn try_new_accepts_exact_backend_shard_limits() { + let modern_data_shards = MODERN_MAX_TOTAL_SHARDS - 1; + let modern = + Erasure::try_new(modern_data_shards, 1, 64).expect("the modern codec's exact field-size boundary must remain valid"); + assert_eq!(modern.total_shard_count(), MODERN_MAX_TOTAL_SHARDS); + assert!(modern.encoder.is_some()); + assert!(modern.legacy_encoder.is_none()); + + let legacy_data_shards = reed_solomon_simd::engine::GF_ORDER / 2; + let legacy_parity_shards = reed_solomon_simd::engine::GF_ORDER - legacy_data_shards; + let legacy = Erasure::try_new_with_options(legacy_data_shards, legacy_parity_shards, 64, true) + .expect("the legacy codec's exact field-size boundary must remain valid"); + assert_eq!(legacy.total_shard_count(), reed_solomon_simd::engine::GF_ORDER); + assert!(legacy.encoder.is_none()); + assert!(legacy.legacy_encoder.is_some()); + } + + #[test] + fn try_new_accepts_zero_parity_without_backend_limits() { + let no_parity = Erasure::try_new(2, 0, 64).expect("zero parity is a valid generic codec configuration"); + assert_eq!(no_parity.total_shard_count(), 2); + assert!(no_parity.encoder.is_none()); + assert!(no_parity.legacy_encoder.is_none()); + + let large_modern = Erasure::try_new(257, 0, 64).expect("zero parity must not inherit the modern backend shard limit"); + assert_eq!(large_modern.total_shard_count(), 257); + assert!(large_modern.encoder.is_none()); + assert!(large_modern.legacy_encoder.is_none()); + + let legacy_data_shards = reed_solomon_simd::engine::GF_ORDER + 1; + let large_legacy = Erasure::try_new_with_options(legacy_data_shards, 0, 64, true) + .expect("zero parity must not inherit the legacy backend shard limit"); + assert_eq!(large_legacy.total_shard_count(), legacy_data_shards); + assert!(large_legacy.encoder.is_none()); + assert!(large_legacy.legacy_encoder.is_none()); + } + + #[test] + fn try_new_accepts_generic_layouts_above_storage_drive_limits() { + let erasure = Erasure::try_new(2, 3, 64).expect("generic 2+3 erasure coding must remain supported"); + assert_eq!(erasure.total_shard_count(), 5); + assert!(erasure.encoder.is_some()); + assert!(erasure.legacy_encoder.is_none()); + + let modern = Erasure::try_new(9, 8, 64).expect("the codec must not impose the storage layer's drive limit"); + assert_eq!(modern.total_shard_count(), 17); + assert!(modern.encoder.is_some()); + assert!(modern.legacy_encoder.is_none()); + + let legacy = Erasure::try_new_with_options(9, 8, 64, true) + .expect("the legacy codec must not impose the storage layer's drive limit"); + assert_eq!(legacy.total_shard_count(), 17); + assert!(legacy.encoder.is_none()); + assert!(legacy.legacy_encoder.is_some()); + } + + #[test] + fn try_new_with_options_initializes_only_the_selected_backend() { + let modern = Erasure::try_new_with_options(4, 2, 64, false).expect("modern codec should construct"); + assert!(modern.encoder.is_some()); + assert!(modern.legacy_encoder.is_none()); + + let legacy = Erasure::try_new_with_options(4, 2, 64, true).expect("legacy codec should construct"); + assert!(legacy.encoder.is_none()); + assert!(legacy.legacy_encoder.is_some()); + } + + #[test] + fn construction_errors_preserve_encoder_sources() { + let modern = ErasureConstructionError::ModernEncoder { + source: reed_solomon_erasure::Error::TooManyShards, + }; + assert!( + std::error::Error::source(&modern) + .and_then(|source| source.downcast_ref::()) + .is_some() + ); + + let legacy = ErasureConstructionError::LegacyEncoder { + source: io::Error::other("legacy encoder failure"), + }; + assert!( + std::error::Error::source(&legacy) + .and_then(|source| source.downcast_ref::()) + .is_some() + ); + } + + #[test] + #[should_panic(expected = "invalid erasure codec configuration")] + fn new_panics_on_invalid_dimensions() { + let _ = Erasure::new(0, 1, 64); + } + + #[test] + #[should_panic(expected = "invalid erasure codec configuration")] + fn new_with_options_panics_on_invalid_dimensions() { + let _ = Erasure::new_with_options(1, 1, 0, true); + } + #[test] fn has_valid_dimensions_rejects_zero_block_size_or_data_shards() { // Well-formed erasure metadata is accepted. @@ -1018,11 +1312,9 @@ mod tests { // Corrupted on-disk metadata with a zero block_size or zero data_shards // must be rejected before it reaches the shard/offset divisions. - // (parity_shards is kept 0 for the zero-data_shards cases so the - // Reed-Solomon encoder is not constructed with an invalid shard count.) - assert!(!Erasure::new(4, 2, 0).has_valid_dimensions()); - assert!(!Erasure::new(0, 0, 64).has_valid_dimensions()); - assert!(!Erasure::new(0, 0, 0).has_valid_dimensions()); + assert!(!erasure_with_invalid_dimensions(4, 2, 0).has_valid_dimensions()); + assert!(!erasure_with_invalid_dimensions(0, 0, 64).has_valid_dimensions()); + assert!(!erasure_with_invalid_dimensions(0, 0, 0).has_valid_dimensions()); } #[tokio::test] @@ -1762,7 +2054,7 @@ mod tests { use std::io::Cursor; use std::sync::{Arc, Mutex}; - let erasure = Arc::new(Erasure::new(1, 0, 0)); + let erasure = Arc::new(erasure_with_invalid_dimensions(1, 0, 0)); let mut reader = Cursor::new(b"payload".to_vec()); let observed = Arc::new(Mutex::new(None)); let observed_clone = observed.clone(); diff --git a/crates/ecstore/src/erasure/coding/mod.rs b/crates/ecstore/src/erasure/coding/mod.rs index 21d283c42..ef60a8a1f 100644 --- a/crates/ecstore/src/erasure/coding/mod.rs +++ b/crates/ecstore/src/erasure/coding/mod.rs @@ -20,4 +20,4 @@ pub mod erasure; pub mod heal; pub use bitrot::*; -pub use erasure::{Erasure, ReedSolomonEncoder, calc_shard_size, calc_shard_size_legacy}; +pub use erasure::{Erasure, ErasureConstructionError, ReedSolomonEncoder, calc_shard_size, calc_shard_size_legacy}; diff --git a/crates/ecstore/src/error/mod.rs b/crates/ecstore/src/error/mod.rs index 6dbe8e02c..f23e1b863 100644 --- a/crates/ecstore/src/error/mod.rs +++ b/crates/ecstore/src/error/mod.rs @@ -215,11 +215,17 @@ pub enum StorageError { #[error("method not allowed")] MethodNotAllowed, #[error("Io error: {0}")] - Io(std::io::Error), + Io(#[source] std::io::Error), #[error("Lock error: {0}")] Lock(#[from] rustfs_lock::LockError), } +impl From for StorageError { + fn from(error: crate::erasure::coding::ErasureConstructionError) -> Self { + Self::Io(error.into_io_error()) + } +} + impl StorageError { pub fn other(error: E) -> Self where @@ -1176,6 +1182,26 @@ mod tests { use super::*; use std::io::{Error as IoError, ErrorKind}; + #[test] + fn other_preserves_erasure_construction_source_chain() { + use crate::erasure::coding::ErasureConstructionError; + use std::error::Error as _; + + let error = StorageError::from(ErasureConstructionError::ModernEncoder { + source: reed_solomon_erasure::Error::TooManyShards, + }); + let io_source = error.source().expect("StorageError::Io must expose its io::Error source"); + assert!(io_source.is::()); + let construction_source = io_source + .source() + .expect("io::Error must expose the erasure construction error"); + assert!(construction_source.is::()); + let encoder_source = construction_source + .source() + .expect("construction error must expose the encoder error"); + assert!(encoder_source.is::()); + } + // Regression for #952 (ECA-11): an all-`DiskNotFound` slice (every drive in // every set unreachable) must NOT be classified as "all not found", // otherwise ListObjects silently returns an empty listing and masks a full diff --git a/crates/ecstore/src/set_disk/ops/heal.rs b/crates/ecstore/src/set_disk/ops/heal.rs index d10b145b6..4df265558 100644 --- a/crates/ecstore/src/set_disk/ops/heal.rs +++ b/crates/ecstore/src/set_disk/ops/heal.rs @@ -163,12 +163,13 @@ impl SetDisks { let erasure = if !latest_meta.deleted && !latest_meta.is_remote() { // Initialize erasure coding; use legacy mode for old-version files - coding::Erasure::new_with_options( + coding::Erasure::try_new_with_options( latest_meta.erasure.data_blocks, latest_meta.erasure.parity_blocks, latest_meta.erasure.block_size, latest_meta.uses_legacy_checksum, ) + .map_err(DiskError::from)? } else { coding::Erasure::default() }; diff --git a/crates/ecstore/src/set_disk/ops/multipart.rs b/crates/ecstore/src/set_disk/ops/multipart.rs index 9d4d2d1d9..52612d149 100644 --- a/crates/ecstore/src/set_disk/ops/multipart.rs +++ b/crates/ecstore/src/set_disk/ops/multipart.rs @@ -355,7 +355,8 @@ impl crate::storage_api_contracts::multipart::MultipartOperations for SetDisks { let tmp_part_path = Arc::new(format!("{tmp_part}/{part_suffix}")); let result: Result = async { - let erasure = coding::Erasure::new(fi.erasure.data_blocks, fi.erasure.parity_blocks, fi.erasure.block_size); + let erasure = coding::Erasure::try_new(fi.erasure.data_blocks, fi.erasure.parity_blocks, fi.erasure.block_size) + .map_err(Error::from)?; let writer_setup_stage_start = rustfs_io_metrics::put_stage_metrics_enabled().then(Instant::now); let mut writers = Vec::with_capacity(shuffle_disks.len()); diff --git a/crates/ecstore/src/set_disk/ops/object.rs b/crates/ecstore/src/set_disk/ops/object.rs index 884321530..9ae7c200f 100644 --- a/crates/ecstore/src/set_disk/ops/object.rs +++ b/crates/ecstore/src/set_disk/ops/object.rs @@ -24,6 +24,11 @@ use super::super::*; use crate::disk::OldCurrentSize; use crate::object_api::GetObjectBodySource; +fn erasure_from_file_info(fi: &FileInfo, uses_legacy: bool) -> Result { + coding::Erasure::try_new_with_options(fi.erasure.data_blocks, fi.erasure.parity_blocks, fi.erasure.block_size, uses_legacy) + .map_err(Error::from) +} + /// Length of the full plaintext body when — and only when — this read's output /// is exactly the object's complete plaintext, so the app-layer body cache may /// serve it in place of the erasure read. @@ -273,12 +278,7 @@ impl crate::storage_api_contracts::object::ObjectIO for SetDisks { } } - let erasure = coding::Erasure::new_with_options( - fi.erasure.data_blocks, - fi.erasure.parity_blocks, - fi.erasure.block_size, - fi.uses_legacy_checksum, - ); + let erasure = erasure_from_file_info(&fi, fi.uses_legacy_checksum)?; let read_length = erasure.shard_file_offset(0, object_size, object_size); let total_shards = data_shards + fi.erasure.parity_blocks; let (_disks, files) = Self::shuffle_disks_and_parts_metadata_by_index(&disks, &files, &fi); @@ -741,7 +741,7 @@ impl SetDisks { let tmp_object = format!("{}/{}/part.1", tmp_dir, fi.data_dir.unwrap()); let result: Result<(ObjectInfo, Option)> = async { - let erasure = coding::Erasure::new(fi.erasure.data_blocks, fi.erasure.parity_blocks, fi.erasure.block_size); + let erasure = erasure_from_file_info(&fi, false)?; let put_object_size = known_put_object_storage_size(data.size()); let is_inline_buffer = @@ -2597,6 +2597,30 @@ fn drop_failed_writer_disks(disks: &mut [Option], writers: &[Option] committed } +#[cfg(test)] +mod erasure_construction_tests { + use super::*; + use crate::erasure::coding::ErasureConstructionError; + use std::error::Error as _; + + #[test] + fn object_file_info_mapping_preserves_construction_error() { + let mut fi = FileInfo::new("object", 2, 2); + fi.erasure.block_size = 0; + + let error = match erasure_from_file_info(&fi, false) { + Ok(_) => panic!("invalid object erasure metadata must be rejected"), + Err(error) => error, + }; + assert!(error.to_string().contains("block_size must be greater than zero")); + let io_source = error.source().expect("StorageError::Io must expose its io::Error source"); + let construction_source = io_source + .source() + .expect("io::Error must expose the erasure construction error"); + assert!(construction_source.is::()); + } +} + #[cfg(test)] mod b3_write_quorum_tests { use super::drop_failed_writer_disks; diff --git a/crates/ecstore/src/set_disk/read.rs b/crates/ecstore/src/set_disk/read.rs index 4a3015ecd..a48f6cb7d 100644 --- a/crates/ecstore/src/set_disk/read.rs +++ b/crates/ecstore/src/set_disk/read.rs @@ -388,15 +388,13 @@ impl SetDisks { return Ok(None); } - let erasure = coding::Erasure::new_with_options( + let erasure = coding::Erasure::try_new_with_options( fi.erasure.data_blocks, fi.erasure.parity_blocks, fi.erasure.block_size, fi.uses_legacy_checksum, - ); - if erasure.data_shards == 0 { - return Ok(None); - } + ) + .map_err(Error::from)?; let checksum_info = fi.erasure.get_checksum_info(part.number); let checksum_algo = if fi.uses_legacy_checksum && checksum_info.algorithm == HashAlgorithm::HighwayHash256S { @@ -618,21 +616,13 @@ impl SetDisks { object, offset, length, end_offset, part_index, last_part_index, last_part_relative_offset, "Multipart read bounds" ); - let erasure = coding::Erasure::new_with_options( + let erasure = coding::Erasure::try_new_with_options( fi.erasure.data_blocks, fi.erasure.parity_blocks, fi.erasure.block_size, fi.uses_legacy_checksum, - ); - - // Erasure params come from on-disk metadata; zero values must fail the read - // instead of panicking on the block/shard divisions below. - if !erasure.has_valid_dimensions() { - return Err(Error::other(format!( - "invalid erasure metadata for {bucket}/{object}: block_size={}, data_blocks={}", - erasure.block_size, erasure.data_shards - ))); - } + ) + .map_err(Error::from)?; let part_indices: Vec = (part_index..=last_part_index).collect(); debug!(bucket, object, ?part_indices, "Multipart part indices to stream"); @@ -1061,22 +1051,13 @@ impl SetDisks { metrics_size_bucket: &'static str, prefer_data_blocks_first_reader_setup: bool, ) -> Result { - let erasure = coding::Erasure::new_with_options( + let erasure = coding::Erasure::try_new_with_options( fi.erasure.data_blocks, fi.erasure.parity_blocks, fi.erasure.block_size, fi.uses_legacy_checksum, - ); - - // Erasure params come from on-disk metadata; zero values must fail the read - // instead of panicking on the block/shard divisions inside the codec - // streaming reader below. Mirrors the legacy multipart guard. - if !erasure.has_valid_dimensions() { - return Err(Error::other(format!( - "invalid erasure metadata for {bucket}/{object}: block_size={}, data_blocks={}", - erasure.block_size, erasure.data_shards - ))); - } + ) + .map_err(Error::from)?; let (disks, files) = Self::shuffle_disks_and_parts_metadata_by_index(disks, files, fi); @@ -1918,7 +1899,12 @@ mod metadata_cache_tests { ) .await .expect_err("invalid erasure metadata must fail before reader setup"); - assert!(err.to_string().contains("invalid erasure metadata")); + assert!(err.to_string().contains("block_size must be greater than zero")); + let io_source = std::error::Error::source(&err).expect("StorageError::Io must expose its io::Error source"); + let construction_source = io_source + .source() + .expect("io::Error must expose the erasure construction error"); + assert!(construction_source.is::()); assert!(output.is_empty()); } diff --git a/scripts/check_architecture_migration_rules.sh b/scripts/check_architecture_migration_rules.sh index 2980a2e1f..ed9fa9e71 100755 --- a/scripts/check_architecture_migration_rules.sh +++ b/scripts/check_architecture_migration_rules.sh @@ -140,6 +140,7 @@ ECSTORE_STORAGE_API_ROOT_REEXPORT_HITS_FILE="${TMP_DIR}/ecstore_storage_api_root ECSTORE_STORAGE_API_ROOT_CONSUMER_HITS_FILE="${TMP_DIR}/ecstore_storage_api_root_consumer_hits.txt" ECSTORE_TEST_DIRECT_API_HITS_FILE="${TMP_DIR}/ecstore_test_direct_api_hits.txt" ECSTORE_BENCH_DIRECT_API_HITS_FILE="${TMP_DIR}/ecstore_bench_direct_api_hits.txt" +ERASURE_PANICKING_CONSTRUCTOR_HITS_FILE="${TMP_DIR}/erasure_panicking_constructor_hits.txt" LOCAL_STORAGE_API_RAW_CONTRACT_PATH_HITS_FILE="${TMP_DIR}/local_storage_api_raw_contract_path_hits.txt" STORE_API_DELETE_DTO_REEXPORTS_FILE="${TMP_DIR}/store_api_delete_dto_reexports.txt" STORE_API_DELETE_DTO_INTERNAL_HITS_FILE="${TMP_DIR}/store_api_delete_dto_internal_hits.txt" @@ -1151,6 +1152,54 @@ if [[ -s "$ECSTORE_BENCH_DIRECT_API_HITS_FILE" ]]; then report_failure "ECStore benches must route ECStore and storage-api symbols through crates/ecstore/benches/storage_api/mod.rs: $(paste -sd '; ' "$ECSTORE_BENCH_DIRECT_API_HITS_FILE")" fi +( + cd "$ROOT_DIR" + find rustfs/src crates -type f -name '*.rs' -print0 | + xargs -0 perl -ne ' + next unless $ARGV =~ m{^(?:rustfs/src/|crates/[^/]+/src/)}; + if (!defined $current_file || $ARGV ne $current_file) { + $current_file = $ARGV; + $line = 0; + $pending_test_item = 0; + $in_test_item = 0; + $test_indent = ""; + } + $line++; + if ($in_test_item) { + $in_test_item = 0 if /^(\s*)}\s*[\]),;]*\s*(?:\/\/.*)?$/ && $1 eq $test_indent; + next; + } + if (/^(\s*)#\s*\[\s*cfg\s*\(\s*test\s*\)\s*\]\s*(.*)$/) { + $test_indent = $1; + my $item = $2; + if ($item =~ /^\s*(?:\/\/.*)?$/) { + $pending_test_item = 1; + } elsif ($item !~ /;\s*(?:\/\/.*)?$/ && $item =~ /\{/ && $item !~ /}\s*(?:\/\/.*)?$/) { + $in_test_item = 1; + } elsif ($item !~ /;\s*(?:\/\/.*)?$/ && $item !~ /\{/) { + $pending_test_item = 1; + } + next; + } + if ($pending_test_item) { + if (/;\s*(?:\/\/.*)?$/) { + $pending_test_item = 0; + } elsif (/\{/) { + $pending_test_item = 0; + $in_test_item = 1 unless /}\s*(?:\/\/.*)?$/; + } + next; + } + if (/^\s*(?!\/\/).*\bErasure::new(?:_with_options)?\s*\(/) { + print "$ARGV:$line:$_"; + } + ' || true +) >"$ERASURE_PANICKING_CONSTRUCTOR_HITS_FILE" + +if [[ -s "$ERASURE_PANICKING_CONSTRUCTOR_HITS_FILE" ]]; then + report_failure "production code must use fallible Erasure constructors: $(paste -sd '; ' "$ERASURE_PANICKING_CONSTRUCTOR_HITS_FILE")" +fi + ( cd "$ROOT_DIR" find crates/ecstore/src -type f -name '*.rs' -print0 |