From 698ebdfb3fec7ea9d8c14e7c264b67eea6dd0f08 Mon Sep 17 00:00:00 2001 From: Zhengchao An Date: Wed, 12 Aug 2026 16:43:04 +0800 Subject: [PATCH] fix(ecstore): map disk-representable StorageError variants in reverse conversion (#5980) The StorageError -> DiskError conversion dropped seven variants with exact DiskError counterparts (FaultyRemoteDisk, DiskAccessDenied, DriveIsRoot, IsNotRegular, VolumeNotEmpty, VolumeAccessDenied, FileAccessDenied) into the DiskError::other fallback, degrading them to an opaque Io error. Two of them sit on the quorum ignore-lists in disk/error_reduce.rs, so a degraded instance would stop matching the ignore list and count toward the dominant error in reduce_errs. Also mirror the StorageError-side io::Error downdrill in From for DiskError: recover a StorageError boxed through From for io::Error instead of wrapping it as Io. Add a round-trip identity test over every DiskError variant (DiskError -> StorageError -> DiskError) and a boxed-StorageError recovery test. --- crates/ecstore/src/disk/error.rs | 30 ++++++++++++++++++- crates/ecstore/src/error/mod.rs | 50 ++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 1 deletion(-) diff --git a/crates/ecstore/src/disk/error.rs b/crates/ecstore/src/disk/error.rs index 7c08115a2..c01161846 100644 --- a/crates/ecstore/src/disk/error.rs +++ b/crates/ecstore/src/disk/error.rs @@ -331,7 +331,14 @@ impl From for DiskError { } match e.downcast::() { Ok(disk_error) => disk_error, - Err(io_error) => DiskError::Io(io_error), + // Mirror `From for StorageError`: a StorageError boxed + // through `From for io::Error` must recover its typed + // classification instead of degrading to `DiskError::Io`, which + // quorum aggregation (`reduce_errs`) would count as a distinct error. + Err(io_error) => match io_error.downcast::() { + Ok(storage_error) => storage_error.into(), + Err(io_error) => DiskError::Io(io_error), + }, } } } @@ -953,6 +960,27 @@ mod tests { assert_eq!(original_disk_error, recovered_disk_error); } + #[test] + fn test_io_error_with_storage_error_inside() { + use crate::error::StorageError; + + // An io::Error boxing a disk-representable StorageError (as produced by + // `From for io::Error`) must recover the typed DiskError + // variant instead of degrading to an opaque DiskError::Io. + let io_with_storage_error: std::io::Error = StorageError::FaultyRemoteDisk.into(); + let recovered: DiskError = io_with_storage_error.into(); + assert_eq!(recovered, DiskError::FaultyRemoteDisk); + + let io_with_storage_error: std::io::Error = StorageError::FileAccessDenied.into(); + let recovered: DiskError = io_with_storage_error.into(); + assert_eq!(recovered, DiskError::FileAccessDenied); + + // A StorageError with no DiskError analog stays an opaque Io error. + let io_with_bucket_error: std::io::Error = StorageError::BucketNotFound("bucket".to_string()).into(); + let recovered: DiskError = io_with_bucket_error.into(); + assert!(matches!(recovered, DiskError::Io(_))); + } + #[test] fn test_io_error_different_kinds() { use std::io::ErrorKind; diff --git a/crates/ecstore/src/error/mod.rs b/crates/ecstore/src/error/mod.rs index a5d4dd0e9..2208f359e 100644 --- a/crates/ecstore/src/error/mod.rs +++ b/crates/ecstore/src/error/mod.rs @@ -358,6 +358,13 @@ impl From for DiskError { StorageError::VolumeNotFound => DiskError::VolumeNotFound, StorageError::VolumeExists => DiskError::VolumeExists, StorageError::FileNameTooLong => DiskError::FileNameTooLong, + StorageError::FaultyRemoteDisk => DiskError::FaultyRemoteDisk, + StorageError::DiskAccessDenied => DiskError::DiskAccessDenied, + StorageError::DriveIsRoot => DiskError::DriveIsRoot, + StorageError::IsNotRegular => DiskError::IsNotRegular, + StorageError::VolumeNotEmpty => DiskError::VolumeNotEmpty, + StorageError::VolumeAccessDenied => DiskError::VolumeAccessDenied, + StorageError::FileAccessDenied => DiskError::FileAccessDenied, _ => DiskError::other(val), } } @@ -1492,6 +1499,49 @@ mod tests { } } + // Every DiskError variant must survive DiskError -> StorageError -> DiskError + // unchanged. A variant that degrades to `DiskError::Io` on the way back loses + // its identity for quorum aggregation (`reduce_errs` classifies by variant + // equality), so ignore-list entries such as FaultyRemoteDisk and + // DiskAccessDenied would silently stop matching. + #[test] + fn test_disk_error_storage_error_round_trip_identity_all_variants() { + // DiskError codes are contiguous from 0x01, so enumerating via from_u32 + // covers every variant and picks up newly appended ones automatically. + let all_variants: Vec = (1u32..).map_while(DiskError::from_u32).collect(); + assert!( + all_variants.len() >= 42, + "DiskError variant enumeration shrank: got {}, expected at least 42", + all_variants.len() + ); + + for original in all_variants { + let storage_error: StorageError = original.clone().into(); + let round_tripped: DiskError = storage_error.into(); + + assert_eq!( + std::mem::discriminant(&original), + std::mem::discriminant(&round_tripped), + "round trip changed variant: {original:?} -> {round_tripped:?}" + ); + assert_eq!(original, round_tripped, "round trip not identical for {original:?}"); + } + + // Io is the only payload-carrying variant: a representative kind and + // message must both survive the round trip. + let io_original = DiskError::Io(IoError::new(ErrorKind::PermissionDenied, "denied")); + let storage_error: StorageError = io_original.clone().into(); + let io_round_tripped: DiskError = storage_error.into(); + assert_eq!(io_original, io_round_tripped); + match io_round_tripped { + DiskError::Io(inner) => { + assert_eq!(inner.kind(), ErrorKind::PermissionDenied); + assert_eq!(inner.to_string(), "denied"); + } + other => panic!("expected DiskError::Io, got {other:?}"), + } + } + #[test] fn test_storage_error_from_io_error() { // Test direct IO error conversion