diff --git a/seaweed-volume/src/server/grpc_server.rs b/seaweed-volume/src/server/grpc_server.rs index 883c0081c..0a9491fb4 100644 --- a/seaweed-volume/src/server/grpc_server.rs +++ b/seaweed-volume/src/server/grpc_server.rs @@ -401,13 +401,15 @@ impl VolumeGrpcService { // Step 1: stop master from redirecting traffic here self.notify_master_volume_readonly(&info, true).await?; - // Step 2: mark local volume readonly + // Step 2: mark local volume readonly; Go's MarkVolumeReadonly errors + // when the volume left the store during the step-1 master round trip. { let mut store = self.state.store.write().unwrap(); - if let Some((_, vol)) = store.find_volume_mut(vid) { - vol.set_read_only_persist(can_delete, persist) - .map_err(|e| Status::internal(e.to_string()))?; - } + let (_, vol) = store + .find_volume_mut(vid) + .ok_or_else(|| Status::not_found(format!("volume {} not found", vid)))?; + vol.set_read_only_persist(can_delete, persist) + .map_err(|e| Status::internal(e.to_string()))?; self.state.volume_state_notify.notify_one(); } @@ -8046,6 +8048,37 @@ mod tests { assert!(err.message().contains("volume id 17 not found"), "{}", err); } + /// A volume can vanish between make_volume_readonly's lookup and its write + /// lock, a window spanning the step-1 master round trip; Go's + /// MarkVolumeReadonly answers "not found" there. Holding the + /// current_master_url write guard parks the call after its lookup, so + /// join!'s in-order polls land the unmount inside the window. + #[tokio::test] + async fn test_make_volume_readonly_answers_not_found_when_volume_vanished_under_lock() { + let (service, _tmp) = make_local_service_with_volume("", None); + let park = service.state.current_master_url.write().await; + + let (result, ()) = tokio::join!( + service.make_volume_readonly(VolumeId(1), false, true), + async { + assert!( + service + .state + .store + .write() + .unwrap() + .unmount_volume(VolumeId(1)), + "the volume must still be mounted when the lookup ran" + ); + drop(park); + } + ); + + let err = result.expect_err("marking a volume that vanished under the lock must fail"); + assert_eq!(err.code(), tonic::Code::NotFound, "{err:?}"); + assert!(err.message().contains("volume 1 not found"), "{err:?}"); + } + /// Same rule for EC volumes, which the heartbeat also expires under a store /// write: delete_expired_ec_volumes destroys a volume whose destroy time has /// passed, and volume_ec_shards_delete unmounts one on demand.