diff --git a/seaweed-volume/src/server/grpc_server.rs b/seaweed-volume/src/server/grpc_server.rs index f89c54cbb..1aaa41721 100644 --- a/seaweed-volume/src/server/grpc_server.rs +++ b/seaweed-volume/src/server/grpc_server.rs @@ -913,10 +913,8 @@ impl VolumeServer for VolumeGrpcService { store.has_ec_volume(file_id.volume_id) }; - // Cookie validation (unless skip_cookie_check). EC volumes always - // take this branch: the distributed read is the only source of the - // on-disk cookie and size, and Go's DeleteEcShardNeedle compares - // the fid cookie against it even when the caller asked to skip. + // EC volumes read even when skipping: Go's DeleteEcShardNeedle + // does, and its ErrorDeleted is what turns a repeat delete into 304. if !req.skip_cookie_check || is_ec_volume { let original_cookie = n.cookie; if !is_ec_volume { @@ -935,43 +933,53 @@ impl VolumeServer for VolumeGrpcService { } } } else { - // Go's ReadEcShardNeedle fills the needle — the local .ecx - // alone can't supply the cookie or the manifest flag. - match crate::server::store_ec::read_ec_shard_needle_distributed( + use crate::server::store_ec::EcMiss; + let read = crate::server::store_ec::read_ec_shard_needle_or_miss( &self.state, file_id.volume_id, n.id, ) - .await - { - Ok(Some(ec_needle)) => n = ec_needle, - Ok(None) => { + .await; + let error = match read { + Ok(Ok(ec_needle)) => { + n = ec_needle; + None + } + Ok(Err(EcMiss::Deleted)) if req.skip_cookie_check => { results.push(volume_server_pb::DeleteResult { file_id: fid_str.clone(), - status: 404, - error: format!("ec needle {} not found", fid_str), + status: 304, + error: String::new(), size: 0, version: 0, }); continue; } - Err(e) => { - results.push(volume_server_pb::DeleteResult { - file_id: fid_str.clone(), - status: 404, - error: e.to_string(), - size: 0, - version: 0, - }); - continue; + Ok(Err(EcMiss::Deleted)) => { + Some(crate::storage::volume::VolumeError::Deleted.to_string()) } + Ok(Err(EcMiss::NotFound)) => Some( + "locate in local ec volume: FindNeedleFromEcx: needle not found" + .to_string(), + ), + Ok(Err(EcMiss::VolumeNotFound)) => { + Some(format!("ec shard {} not found", file_id.volume_id)) + } + Err(e) => Some(e.to_string()), + }; + if let Some(error) = error { + // Skipping, Go meets the miss inside DeleteEcShardNeedle: a 500. + results.push(volume_server_pb::DeleteResult { + file_id: fid_str.clone(), + status: if req.skip_cookie_check { 500 } else { 404 }, + error, + size: 0, + version: 0, + }); + continue; } } - // Go's inner check is `cookie != 0 && cookie != n.Cookie`: a - // zero fid cookie skips validation, which can only happen - // here when skip_cookie_check was already requested. - if (!req.skip_cookie_check || original_cookie.0 != 0) && n.cookie != original_cookie - { + if !req.skip_cookie_check && n.cookie != original_cookie { results.push(volume_server_pb::DeleteResult { file_id: fid_str.clone(), status: 400, @@ -983,8 +991,8 @@ impl VolumeServer for VolumeGrpcService { } } - // Reject chunk manifest needles - if n.is_chunk_manifest() { + // Go never reads the needle when skipping, so its manifest check can't fire. + if !req.skip_cookie_check && n.is_chunk_manifest() { results.push(volume_server_pb::DeleteResult { file_id: fid_str.clone(), status: 406, @@ -1075,8 +1083,7 @@ impl VolumeServer for VolumeGrpcService { } else { // EC volume deletion: forward the tombstone to a holder of the // needle's primary shard (Go's DeleteEcShardNeedle → - // VolumeEcBlobDelete). The cookie was already validated - // against the distributed read above. + // VolumeEcBlobDelete). match crate::server::store_ec::delete_ec_shard_needle_distributed( &self.state, file_id.volume_id, @@ -10429,6 +10436,146 @@ mod tests { assert_eq!(err.message(), "needle not found 12345"); } + /// Volume 1 as EC only, all 14 shards local: needle 11 (cookie 0x3344) and + /// chunk manifest needle 12 (cookie 0x5566). + async fn ec_1_with_a_manifest_needle() -> (VolumeGrpcService, TempDir) { + let (service, tmp) = make_local_service_with_volume("", None); + { + let mut store = service.state.store.write().unwrap(); + let (_, volume) = store.find_volume_mut(VolumeId(1)).unwrap(); + let mut manifest = Needle { + id: NeedleId(12), + cookie: Cookie(0x5566), + data: b"[]".to_vec(), + data_size: 2, + ..Needle::default() + }; + manifest.set_is_chunk_manifest(); + volume.write_needle(&mut manifest, true, false).unwrap(); + volume.sync_to_disk().unwrap(); + } + generate_and_mount_ec_1(&service, (0..14).collect()).await; + service + .state + .store + .write() + .unwrap() + .unmount_volume(VolumeId(1)) + .unwrap(); + (service, tmp) + } + + async fn batch_delete_1( + service: &VolumeGrpcService, + needle_id: u64, + cookie: u32, + skip_cookie_check: bool, + ) -> (i32, String) { + let fid = needle::FileId::new(VolumeId(1), NeedleId(needle_id), Cookie(cookie)); + let resp = service + .batch_delete(Request::new(volume_server_pb::BatchDeleteRequest { + file_ids: vec![fid.to_string()], + skip_cookie_check, + })) + .await + .unwrap() + .into_inner(); + assert_eq!(resp.results.len(), 1, "{:?}", resp.results); + let r = &resp.results[0]; + (r.status, r.error.clone()) + } + + const GO_EC_NEEDLE_NOT_FOUND: &str = + "locate in local ec volume: FindNeedleFromEcx: needle not found"; + + /// Go's TestBatchDelete_AlreadyDeletedEcNeedleIsNotAnError: the filer skips + /// the cookie check, and a repeat delete must not read as a failure. + #[tokio::test] + async fn test_batch_delete_already_deleted_ec_needle_is_not_an_error() { + let (service, _tmp) = ec_1_with_a_manifest_needle().await; + service + .state + .store + .write() + .unwrap() + .find_ec_volume_mut(VolumeId(1)) + .unwrap() + .journal_delete(NeedleId(11)) + .unwrap(); + + assert_eq!( + batch_delete_1(&service, 11, 0x3344, true).await, + (304, String::new()) + ); + assert_eq!( + batch_delete_1(&service, 11, 0x3344, false).await, + (404, "already deleted".to_string()) + ); + } + + /// A deletion only the peer holding the needle's shard knows of is still 304. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn test_batch_delete_peer_reported_ec_deletion_is_not_an_error() { + let (service, tmp) = make_local_service_with_volume("", None); + let _peer = put_ec_1_needle_shard_on_a_peer(&service, &tmp, true).await; + + assert_eq!( + batch_delete_1(&service, 11, 0x3344, true).await, + (304, String::new()) + ); + } + + #[tokio::test] + async fn test_batch_delete_missing_ec_needle_reports_go_error() { + let (service, _tmp) = ec_1_with_a_manifest_needle().await; + + assert_eq!( + batch_delete_1(&service, 12345, 0x3344, false).await, + (404, GO_EC_NEEDLE_NOT_FOUND.to_string()) + ); + assert_eq!( + batch_delete_1(&service, 12345, 0x3344, true).await, + (500, GO_EC_NEEDLE_NOT_FOUND.to_string()) + ); + } + + /// Go discards the fid cookie when skipping, so a mismatch deletes. + #[tokio::test] + async fn test_batch_delete_ec_cookie_is_checked_only_when_asked() { + let (service, _tmp) = ec_1_with_a_manifest_needle().await; + + assert_eq!( + batch_delete_1(&service, 11, 0x9999, false).await, + (400, "File Random Cookie does not match.".to_string()) + ); + let (status, error) = batch_delete_1(&service, 11, 0x9999, true).await; + assert_eq!((status, error.as_str()), (202, "")); + assert_eq!( + batch_delete_1(&service, 11, 0x3344, true).await, + (304, String::new()) + ); + } + + /// The filer deletes a manifest chunk's own fid, with the cookie check skipped. + #[tokio::test] + async fn test_batch_delete_ec_manifest_is_refused_only_with_the_cookie_check() { + let (service, _tmp) = ec_1_with_a_manifest_needle().await; + + assert_eq!( + batch_delete_1(&service, 12, 0x5566, false).await, + ( + 406, + "ChunkManifest: not allowed in batch delete mode.".to_string() + ) + ); + let (status, error) = batch_delete_1(&service, 12, 0x5566, true).await; + assert_eq!((status, error.as_str()), (202, "")); + assert_eq!( + batch_delete_1(&service, 12, 0x5566, false).await, + (404, "already deleted".to_string()) + ); + } + /// Batch atomicity: mount pre-validates the ENTIRE shard_ids before /// acquiring the write lock or mounting anything. A batch like [0, 32] /// must fail with InvalidArgument and mount NOTHING — not the valid