From 8a9563e53dbf3a0af92c3c72d82f03ff64742ea0 Mon Sep 17 00:00:00 2001 From: Eliah Rusin Date: Thu, 1 Oct 2026 18:13:17 +0300 Subject: [PATCH] volume: test that only the disk holding the replaced replica gets the free-slot credit (#11486) * volume server: split volume_copy into phases and type the delete-after-status gate volume_copy was one ~400-line handler, and the rule that an existing local replica is deleted only after the source's ReadVolumeFileStatus succeeded was held by statement order alone. The keep_remote_data=true that the pre-copy delete and the failed-copy rollback must share was kept in sync by a comment pointing from one to the other. The handler is now a ~60-line orchestrator over connect_to_copy_source, SourceVolumeStatus::fetch, delete_existing_replica, plan_copy_destination and a VolumeCopyJob whose run() drives preallocate_dat, transfer_files, finish_copied_files and mount_and_reply, with cleanup_failed_copy on error. delete_existing_replica takes a &SourceVolumeStatus, which only fetch can construct (private field in a child module), so the delete cannot be called before the status RPC. Both deletes go through delete_replica_keep_remote. Pure refactor: call order, status codes and messages, cancellation checks, throttling, progress reports and cleanup are unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) * volume server: find space for a VolumeCopy before deleting the replica it replaces VolumeCopy deleted an existing local replica as soon as the source answered ReadVolumeFileStatus and only then looked for a location with room for the copy. With no usable location (disk full, low-disk, wrong disk type) the call errored after the delete, leaving the node with neither the old replica nor the new one. Plan the destination first, as Go does: find_free_location_replacing credits the location holding the replaced volume with that volume's slot, so a disk at its volume limit that holds the replica still accepts the copy. Only then delete the replica and write the .note (still after the delete, as in Go). delete_existing_replica now takes the planned CopyDestination, so the delete cannot precede the plan. find_free_location_predicate keeps its behaviour. Co-Authored-By: Claude Opus 5.5 (1M context) * volume: describe the replace-credit test against the current VolumeCopy flow Co-Authored-By: Claude Opus 5.5 (1M context) --------- Co-authored-by: Claude Opus 5.5 (1M context) Co-authored-by: Chris Lu --- seaweed-volume/src/storage/store.rs | 46 +++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/seaweed-volume/src/storage/store.rs b/seaweed-volume/src/storage/store.rs index 56f58aaae..ea3da0104 100644 --- a/seaweed-volume/src/storage/store.rs +++ b/seaweed-volume/src/storage/store.rs @@ -2483,6 +2483,52 @@ mod tests { assert_eq!(selected, Some(0)); } + // VolumeCopy picks a disk before deleting the replica it replaces, so only + // the location actually holding that replica may count its slot as free. + #[test] + fn test_find_free_location_predicate_credits_only_the_holding_location() { + let tmp1 = TempDir::new().unwrap(); + let dir1 = tmp1.path().to_str().unwrap(); + let tmp2 = TempDir::new().unwrap(); + let dir2 = tmp2.path().to_str().unwrap(); + + let mut store = Store::new(NeedleMapKind::InMemory); + for dir in [dir1, dir2] { + store + .add_location( + dir, + dir, + 1, + DiskType::HardDrive, + MinFreeSpace::Percent(0.0), + Vec::new(), + ) + .unwrap(); + } + for vid in [81, 82] { + store + .add_volume(VolumeId(vid), DiskType::HardDrive, &VolumeSpec::default()) + .unwrap(); + } + let loc_of = |vid| store.find_volume(VolumeId(vid)).unwrap().0; + assert_ne!(loc_of(81), loc_of(82), "fixture must fill both locations"); + + let hdd = |loc: &DiskLocation| loc.disk_type == DiskType::HardDrive; + assert_eq!(store.find_free_location_predicate(hdd, None), None); + assert_eq!( + store.find_free_location_predicate(hdd, Some(VolumeId(99))), + None + ); + assert_eq!( + store.find_free_location_predicate(hdd, Some(VolumeId(81))), + Some(loc_of(81)) + ); + assert_eq!( + store.find_free_location_predicate(hdd, Some(VolumeId(82))), + Some(loc_of(82)) + ); + } + #[test] fn test_delete_expired_ec_volumes_removes_expired_entries() { let tmp = TempDir::new().unwrap();