diff --git a/seaweed-volume/src/server/grpc_server.rs b/seaweed-volume/src/server/grpc_server.rs index a139055ed..f89c54cbb 100644 --- a/seaweed-volume/src/server/grpc_server.rs +++ b/seaweed-volume/src/server/grpc_server.rs @@ -11808,4 +11808,71 @@ mod tests { assert!(err.message().contains("has no live entries"), "{err:?}"); assert!(!std::path::Path::new(&format!("{data}/1.dat")).exists()); } + + // Among storage errors, the filer requeues a delete only on "is read only" + // (Go's DeleteVolumeNeedle text). + #[tokio::test] + async fn test_batch_delete_on_read_only_volume_says_is_read_only() { + let request = || { + Request::new(volume_server_pb::BatchDeleteRequest { + file_ids: vec!["1,b00003344".to_string()], + skip_cookie_check: false, + }) + }; + + let (service, _tmp) = make_local_service_with_volume("", None); + service + .state + .store + .write() + .unwrap() + .find_volume_mut(VolumeId(1)) + .unwrap() + .1 + .set_no_write_or_delete(true); + let resp = service.batch_delete(request()).await.unwrap().into_inner(); + assert_eq!(resp.results.len(), 1); + assert_eq!(resp.results[0].status, 500); + assert_eq!(resp.results[0].error, "volume 1 is read only"); + + // Controls: a writable volume, and one read-only but deletable. + let (service, _tmp) = make_local_service_with_volume("", None); + let resp = service.batch_delete(request()).await.unwrap().into_inner(); + assert_eq!(resp.results[0].status, 202, "{:?}", resp.results[0].error); + + let (service, _tmp) = make_local_service_with_volume("", None); + service + .state + .store + .write() + .unwrap() + .find_volume_mut(VolumeId(1)) + .unwrap() + .1 + .set_read_only_persist(true, false) + .unwrap(); + let resp = service.batch_delete(request()).await.unwrap().into_inner(); + assert_eq!(resp.results[0].status, 202, "{:?}", resp.results[0].error); + } + + // Go says "volume N not found[ on host:port]" with the same statuses; the + // bare "not found" is kept on purpose: the filer skips it, whereas Go's + // text is booked permanent and outranks a retryable sibling replica. + #[tokio::test] + async fn test_batch_delete_on_missing_volume_says_not_found() { + let (service, _tmp) = make_local_service_with_volume("", None); + for (skip_cookie_check, status) in [(false, 404), (true, 500)] { + let resp = service + .batch_delete(Request::new(volume_server_pb::BatchDeleteRequest { + file_ids: vec!["2,b00003344".to_string()], + skip_cookie_check, + })) + .await + .unwrap() + .into_inner(); + assert_eq!(resp.results[0].status, status, "skip={skip_cookie_check}"); + assert_eq!(resp.results[0].error, "not found"); + } + } + } diff --git a/seaweed-volume/src/server/mod.rs b/seaweed-volume/src/server/mod.rs index f18fc7745..3ba09a18b 100644 --- a/seaweed-volume/src/server/mod.rs +++ b/seaweed-volume/src/server/mod.rs @@ -27,7 +27,9 @@ impl From for Status { VolumeError::NotFound | VolumeError::VolumeNotFound(_) | VolumeError::Tier(TierError::NotFound(_)) => Status::not_found(message), - VolumeError::ReadOnly | VolumeError::NotEmpty => Status::failed_precondition(message), + VolumeError::ReadOnly(_) | VolumeError::NotEmpty => { + Status::failed_precondition(message) + } VolumeError::InsufficientSpace { .. } => Status::resource_exhausted(message), VolumeError::AlreadyExists => Status::already_exists(message), _ => Status::internal(message), @@ -69,7 +71,14 @@ mod tests { Code::NotFound ); assert_eq!(code(VolumeError::NotFound), Code::NotFound); - assert_eq!(code(VolumeError::ReadOnly), Code::FailedPrecondition); + assert_eq!( + code(VolumeError::ReadOnly(VolumeId(7))), + Code::FailedPrecondition + ); + assert_eq!( + VolumeError::ReadOnly(VolumeId(7)).to_string(), + "volume 7 is read only" + ); assert_eq!( code(VolumeError::InsufficientSpace { vid: VolumeId(7), diff --git a/seaweed-volume/src/storage/store.rs b/seaweed-volume/src/storage/store.rs index e94612e65..5c0a53246 100644 --- a/seaweed-volume/src/storage/store.rs +++ b/seaweed-volume/src/storage/store.rs @@ -779,7 +779,7 @@ impl Store { .is_disk_space_low .load(Ordering::Relaxed) { - return Err(VolumeError::ReadOnly); + return Err(VolumeError::ReadOnly(vid)); } let (_, vol) = self.find_volume_mut(vid).ok_or(VolumeError::NotFound)?; @@ -795,7 +795,7 @@ impl Store { // Match Go's DeleteVolumeNeedle: check noWriteOrDelete before proceeding. let (_, vol) = self.find_volume(vid).ok_or(VolumeError::NotFound)?; if vol.is_no_write_or_delete() { - return Err(VolumeError::ReadOnly); + return Err(VolumeError::ReadOnly(vid)); } let (_, vol) = self.find_volume_mut(vid).ok_or(VolumeError::NotFound)?; diff --git a/seaweed-volume/src/storage/volume.rs b/seaweed-volume/src/storage/volume.rs index 7072cb454..359dbd983 100644 --- a/seaweed-volume/src/storage/volume.rs +++ b/seaweed-volume/src/storage/volume.rs @@ -61,8 +61,8 @@ pub enum VolumeError { #[error("volume already exists")] AlreadyExists, - #[error("volume is read-only")] - ReadOnly, + #[error("volume {0} is read only")] + ReadOnly(VolumeId), #[error("volume is unavailable: {0}")] Unavailable(String), @@ -2422,7 +2422,7 @@ impl Volume { return Err(e); } if self.is_read_only() { - return Err(VolumeError::ReadOnly); + return Err(VolumeError::ReadOnly(self.id)); } Ok(()) } @@ -2803,7 +2803,7 @@ impl Volume { return Err(e); } if self.no_write_or_delete { - return Err(VolumeError::ReadOnly); + return Err(VolumeError::ReadOnly(self.id)); } self.do_delete_request(n) } @@ -4155,7 +4155,7 @@ impl Volume { needle_blob: &[u8], ) -> Result<(), VolumeError> { if self.is_read_only() { - return Err(VolumeError::ReadOnly); + return Err(VolumeError::ReadOnly(self.id)); } let dat_file = self .dat_file @@ -4176,7 +4176,7 @@ impl Volume { ) -> Result<(), VolumeError> { // nm.put on a read-only volume fails only after the blob is appended to .dat. if self.is_read_only() { - return Err(VolumeError::ReadOnly); + return Err(VolumeError::ReadOnly(self.id)); } // Storage guard: negativity-only (Go parity). See parse_needle_at. if size.0 < 0 { @@ -6187,7 +6187,7 @@ mod tests { assert!( matches!( v.write_needle(&mut later, true, false), - Err(VolumeError::ReadOnly) + Err(VolumeError::ReadOnly(_)) ), "later writes must not append past the record whose index is in doubt" ); @@ -8394,7 +8394,7 @@ mod tests { false, ) .unwrap_err(); - assert!(matches!(err, VolumeError::ReadOnly)); + assert!(matches!(err, VolumeError::ReadOnly(_))); let deleted_size = v .delete_needle(&mut Needle { @@ -9318,7 +9318,7 @@ mod tests { false, ) .unwrap_err(); - assert!(matches!(err, VolumeError::ReadOnly)); + assert!(matches!(err, VolumeError::ReadOnly(_))); let deleted = v .delete_needle(&mut Needle { @@ -9359,7 +9359,7 @@ mod tests { false, ) .unwrap_err(); - assert!(matches!(err, VolumeError::ReadOnly)); + assert!(matches!(err, VolumeError::ReadOnly(_))); let deleted = v .delete_needle(&mut Needle { @@ -9426,7 +9426,7 @@ mod tests { }) .unwrap_err(); assert!( - matches!(err, VolumeError::ReadOnly), + matches!(err, VolumeError::ReadOnly(_)), "plain readonly must reject deletes" ); diff --git a/seaweed-volume/tests/http_integration.rs b/seaweed-volume/tests/http_integration.rs index 43897c9b2..3ed07d003 100644 --- a/seaweed-volume/tests/http_integration.rs +++ b/seaweed-volume/tests/http_integration.rs @@ -477,6 +477,74 @@ async fn delete_then_get_returns_404() { ); } +// Go answers both with 500 and an error containing "volume N is read only" +// (the write behind "failed to write to local disk: "). +#[tokio::test] +async fn write_and_delete_on_read_only_volume_say_is_read_only() { + let (state, _tmp) = test_state(); + let uri = "/1,01637037d6"; + + let request = |method: &str, body: &[u8]| { + Request::builder() + .method(method) + .uri(uri) + .body(Body::from(body.to_vec())) + .unwrap() + }; + let error_of = |body: Vec| -> String { + let json: serde_json::Value = serde_json::from_slice(&body).unwrap(); + json["error"].as_str().unwrap_or_default().to_string() + }; + + let response = build_admin_router(state.clone()) + .oneshot(request("POST", b"written before read-only")) + .await + .unwrap(); + assert_eq!(response.status(), StatusCode::CREATED); + + state + .store + .write() + .unwrap() + .find_volume_mut(VolumeId(1)) + .unwrap() + .1 + .set_no_write_or_delete(true); + + let response = build_admin_router(state.clone()) + .oneshot(request("POST", b"refused")) + .await + .unwrap(); + assert_eq!(response.status(), StatusCode::INTERNAL_SERVER_ERROR); + let error = error_of(body_bytes(response).await); + assert!(error.contains("volume 1 is read only"), "{error}"); + + let response = build_admin_router(state.clone()) + .oneshot(request("DELETE", b"")) + .await + .unwrap(); + assert_eq!(response.status(), StatusCode::INTERNAL_SERVER_ERROR); + assert_eq!( + error_of(body_bytes(response).await), + "Deletion Failed: volume 1 is read only" + ); + + // The needle is still there to delete once the volume is writable again. + state + .store + .write() + .unwrap() + .find_volume_mut(VolumeId(1)) + .unwrap() + .1 + .set_no_write_or_delete(false); + let response = build_admin_router(state.clone()) + .oneshot(request("DELETE", b"")) + .await + .unwrap(); + assert_eq!(response.status(), StatusCode::ACCEPTED); +} + // ============================================================================ // 6. HEAD returns headers without body // ============================================================================