From df4995b8948b28018c63bb4e1a9db0451da03583 Mon Sep 17 00:00:00 2001
From: Eliah Rusin <hello@franchb.com>
Date: Fri, 25 Sep 2026 17:05:53 +0300
Subject: [PATCH] volume server: VolumeMarkReadonly answers NotFound when the
 volume vanished under the lock (#11443)

* volume server: VolumeMarkReadonly answers NotFound when the volume vanished under the lock

make_volume_readonly looked the volume up, notified the master (step 1),
then took the store write lock (step 2) and marked the volume only `if
let Some(..)`. When the volume left the store during step 1 -- a master
round trip, during which an unmount or a heartbeat expiry can land --
the missing else meant the RPC reported success for a volume the server
no longer has, and step 3 told the master again that it is read-only.

Go's Store.MarkVolumeReadonly (weed/storage/store.go) returns
"volume %d not found" when findVolume comes back nil, and
makeVolumeReadonly (weed/server/volume_grpc_admin.go) returns that error
before the step-3 notification. The Rust step 2 now does the same:
find_volume_mut(vid) -> Status::not_found("volume {vid} not found"), and
the `?` skips step 3, as it already did for a set_read_only_persist
failure. The scrub caller already matches NotFound to skip such a
volume instead of failing the whole report; it now actually gets it.
volume_mark_writable already returns NotFound under its write lock.

The regression test opens the step-1 window deterministically: step 1
awaits the current_master_url read lock, so the test holds its write
guard, lets make_volume_readonly park there after its own lookup
succeeded, unmounts the volume, then releases the guard. With no master
configured the notification is a no-op, so the write lock in step 2 is
the only place left that can notice the volume is gone.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* volume server: trim comments on the vanished-volume mark-readonly path

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Co-authored-by: Chris Lu <chrislusf@users.noreply.github.com>
Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
---
 seaweed-volume/src/server/grpc_server.rs | 43 +++++++++++++++++++++---
 1 file changed, 38 insertions(+), 5 deletions(-)

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.