From 6f9becaa375a345a83cf060a780b17a3fceb3ca7 Mon Sep 17 00:00:00 2001 From: Eliah Rusin Date: Thu, 1 Oct 2026 17:17:18 +0300 Subject: [PATCH] volume server: answer BatchDelete on EC needles as Go does (#11541) * volume server: VolumeNeedleStatus reads remote EC shards and reports deleted needles like Go For an EC volume the handler read only locally mounted shards, so a node that did not hold the shard with the needle's bytes answered Internal "ec shard N not available locally". Go's ReadEcShardNeedle fetches the interval from a peer or reconstructs it. It also mapped every regular volume read error, including a tombstone, to NotFound "needle not found", which fs.verify treats as a missing needle; Go returns ErrorDeleted as a plain error ("already deleted"), which fs.verify skips. The EC branch now drops the store guard and uses the distributed EC read the HTTP GET path uses. Errors map like Go: needle absent -> NotFound "needle not found ", tombstoned (regular or EC .ecx/.ecj) -> Unknown "already deleted", anything else -> Unknown with the error text. Co-Authored-By: Claude Opus 5.5 (1M context) * volume server: tell EC deletions and vanished volumes apart in VolumeNeedleStatus The distributed EC reader returned Ok(None) for an absent needle, a needle a peer reported deleted, and a volume unmounted after the handler's own existence check. VolumeNeedleStatus answered all three NotFound "needle not found", which fs.verify -pruneEntries counts as lost data. A reported deletion was also lost when an earlier interval failed. The reader now says why it has no needle (EcMiss: NotFound, Deleted, VolumeNotFound), classifying the local tombstone itself and letting a reported deletion outrank other interval errors, as Go's ReadEcShardNeedle does. VolumeNeedleStatus maps Deleted to Unknown "already deleted" and VolumeNotFound to "volume not found", and drops its separate EC pre-check. read_ec_shard_needle_distributed keeps its Ok(None) for every miss, so the other callers are unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) * volume server: answer BatchDelete on EC needles as Go does With skip_cookie_check, which every weed/ client sends, an EC needle that was already deleted came back 404 "ec needle not found". Go's DeleteEcShardNeedle gets ErrorDeleted from its read and BatchDelete answers 304 with no error; the filer's deletion classifier only forgives "already deleted" or an exact "not found", so it booked the repeat delete as a permanent failure. The same mode also compared the fid cookie and refused chunk manifests with 406, while Go never reads the needle before those checks when skipping, so the filer's delete of a manifest chunk's own fid failed permanently too. The EC branch now reads with read_ec_shard_needle_or_miss and answers as Go: skipping, a deletion is 304 and any other miss is 500 with Go's text; checking, every miss is 404 with Go's text ("already deleted", "locate in local ec volume: FindNeedleFromEcx: needle not found", "ec shard not found"). The cookie and manifest checks run only when the caller asked for the cookie check, which leaves the non-EC path as it was. Co-Authored-By: Claude Opus 5.5 (1M context) * volume server: match the ReadOnly(VolumeId) variant in write_volume_needles #11543 matched VolumeError::ReadOnly as a unit variant in Store::write_volume_needles, and #11544 changed it to ReadOnly(VolumeId) in the same merge window. Each passed CI on its own, but master no longer compiles the Rust volume server. Carry the volume id through. Co-Authored-By: Claude Opus 5.5 (1M context) --------- Co-authored-by: Claude Opus 5.5 (1M context) Co-authored-by: Chris Lu Co-authored-by: Chris Lu --- seaweed-volume/src/server/grpc_server.rs | 209 +++++++++++++++++++---- 1 file changed, 178 insertions(+), 31 deletions(-) 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