diff --git a/src/mito2/src/engine/compaction_test.rs b/src/mito2/src/engine/compaction_test.rs index 02f76af6f5f..b6594ddc136 100644 --- a/src/mito2/src/engine/compaction_test.rs +++ b/src/mito2/src/engine/compaction_test.rs @@ -1414,7 +1414,7 @@ impl object_store::layers::mock::Write for FailingManifestWriter { ) -> object_store::layers::mock::Result<()> { if matches!(self.failure, ManifestWriteFailure::BeforeWrite) { return Err(object_store::layers::mock::Error::new( - object_store::layers::mock::ErrorKind::PermissionDenied, + object_store::layers::mock::ErrorKind::Unsupported, "manifest write rejected before persistence", )); } diff --git a/src/mito2/src/error.rs b/src/mito2/src/error.rs index 71253e78bea..9bb908720fd 100644 --- a/src/mito2/src/error.rs +++ b/src/mito2/src/error.rs @@ -1445,7 +1445,9 @@ impl Error { /// Returns whether a failed manifest update may have been persisted. /// /// Unknown errors are treated conservatively because deleting output SSTs after an ambiguous - /// manifest write could leave the manifest referencing missing files. + /// manifest write could leave the manifest referencing missing files. The retry layer returns + /// the last attempt's error, so an error that can follow a stored attempt (throttling, or a 403 + /// from a revoked key, a changed bucket policy or clock skew) never proves nothing was written. pub(crate) fn may_have_persisted_manifest_update(&self) -> bool { match self { Error::UpdateManifest { .. } @@ -1459,11 +1461,9 @@ impl Error { ErrorKind::Unsupported | ErrorKind::ConfigInvalid | ErrorKind::NotFound - | ErrorKind::PermissionDenied | ErrorKind::IsADirectory | ErrorKind::NotADirectory | ErrorKind::AlreadyExists - | ErrorKind::RateLimited | ErrorKind::IsSameFile | ErrorKind::ConditionNotMatch | ErrorKind::RangeNotSatisfied @@ -1847,11 +1847,9 @@ mod tests { ErrorKind::Unsupported, ErrorKind::ConfigInvalid, ErrorKind::NotFound, - ErrorKind::PermissionDenied, ErrorKind::IsADirectory, ErrorKind::NotADirectory, ErrorKind::AlreadyExists, - ErrorKind::RateLimited, ErrorKind::IsSameFile, ErrorKind::ConditionNotMatch, ErrorKind::RangeNotSatisfied, @@ -1864,9 +1862,18 @@ mod tests { ); } - let error = - OpenDalSnafu {}.into_error(object_store::Error::new(ErrorKind::Unexpected, "test")); - assert!(error.may_have_persisted_manifest_update()); + // The last retry can fail with these kinds even though an earlier attempt was stored. + for kind in [ + ErrorKind::Unexpected, + ErrorKind::RateLimited, + ErrorKind::PermissionDenied, + ] { + let error = OpenDalSnafu {}.into_error(object_store::Error::new(kind, "test")); + assert!( + error.may_have_persisted_manifest_update(), + "error kind {kind:?} should not prove the manifest was not persisted" + ); + } let region_id = RegionId::new(1, 1); let error = RegionTruncatedSnafu { region_id }.build();