diff --git a/seaweed-volume/src/server/grpc_server.rs b/seaweed-volume/src/server/grpc_server.rs index 1371a4820..2b05765c6 100644 --- a/seaweed-volume/src/server/grpc_server.rs +++ b/seaweed-volume/src/server/grpc_server.rs @@ -1516,9 +1516,16 @@ impl VolumeServer for VolumeGrpcService { .find_volume(vid) .ok_or_else(|| Status::not_found(format!("not found volume id {}", vid)))?; - // Check compaction revision + // Check compaction revision. Compare against the volume's live + // super_block.compaction_revision (matching Go's v.CompactionRevision, + // which is the embedded SuperBlock field). last_compact_revision() is + // a separate bookkeeping value recorded just before a compaction starts + // (for makeup-diff catch-up) and is intentionally left behind the live + // revision afterward — it is not interchangeable with the live value and + // must not be used here, or this check spuriously fails on every volume + // that has ever been compacted even once. if req.compaction_revision != u32::MAX - && v.last_compact_revision() != req.compaction_revision as u16 + && u32::from(v.super_block.compaction_revision) != req.compaction_revision { return Err(Status::failed_precondition(format!( "volume {} is compacted", @@ -5658,6 +5665,105 @@ mod tests { } } + // Regression test for comparing the wrong compaction-revision field. + // last_compact_revision() is bookkeeping recorded just before a compaction + // starts (for makeup-diff catch-up) and is intentionally left behind + // super_block.compaction_revision once the compaction commits. copy_file's + // precondition check must compare against the live super_block value + // (matching Go's v.CompactionRevision), not the stale bookkeeping one, or + // it fails "volume N is compacted" on every volume that has ever been + // compacted even once. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn test_copy_file_accepts_live_revision_after_compaction() { + let (service, _tmp) = make_local_service_with_volume("", None); + + // Drive a real compaction to completion so the two revision fields + // actually diverge the way they do in production, rather than poking + // the struct fields directly. + let (revision_before, revision_after, last_compact_revision_after) = { + let mut store = service.state.store.write().unwrap(); + let (_, v) = store.find_volume_mut(VolumeId(1)).unwrap(); + let before = v.super_block.compaction_revision; + + v.compact_by_index(0, 0, |_| true).unwrap(); + v.commit_compact().unwrap(); + + (before, v.super_block.compaction_revision, v.last_compact_revision()) + }; + + assert_eq!(revision_before, 0, "fresh volume starts at revision 0"); + assert_eq!( + revision_after, 1, + "live super_block.compaction_revision must advance after commit_compact" + ); + assert_eq!( + last_compact_revision_after, 0, + "last_compact_revision() is recorded pre-compaction and must stay \ + behind the live revision — this divergence is exactly what made \ + the old check fail permanently" + ); + + // The live revision (1) must be accepted: this is what + // read_volume_file_status would report to a caller right now. + let response = service + .copy_file(Request::new(volume_server_pb::CopyFileRequest { + volume_id: 1, + ext: ".dat".to_string(), + compaction_revision: revision_after as u32, + stop_offset: u64::MAX, + collection: String::new(), + is_ec_volume: false, + ignore_source_file_not_found: false, + })) + .await; + assert!( + response.is_ok(), + "copy_file must accept the live compaction_revision, got: {:?}", + response.err() + ); + + // A genuinely stale/wrong revision must still be rejected — the fix + // corrects which field is compared, it does not disable the check. + let stale_response = service + .copy_file(Request::new(volume_server_pb::CopyFileRequest { + volume_id: 1, + ext: ".dat".to_string(), + compaction_revision: 99, + stop_offset: u64::MAX, + collection: String::new(), + is_ec_volume: false, + ignore_source_file_not_found: false, + })) + .await; + let err = match stale_response { + Ok(_) => panic!("a genuinely stale revision must be rejected"), + Err(e) => e, + }; + assert_eq!(err.code(), tonic::Code::FailedPrecondition); + assert!(err.message().contains("is compacted")); + + // A request revision that only collides after truncating to u16 must be + // rejected: 65537 as u16 == 1 == the live revision, but as u32 they differ. + // Compare in u32 space (matching Go) so this cannot spuriously pass. + let truncating_response = service + .copy_file(Request::new(volume_server_pb::CopyFileRequest { + volume_id: 1, + ext: ".dat".to_string(), + compaction_revision: u32::from(revision_after) + (1 << 16), + stop_offset: u64::MAX, + collection: String::new(), + is_ec_volume: false, + ignore_source_file_not_found: false, + })) + .await; + let err = match truncating_response { + Ok(_) => panic!("a revision that only matches after u16 truncation must be rejected"), + Err(e) => e, + }; + assert_eq!(err.code(), tonic::Code::FailedPrecondition); + assert!(err.message().contains("is compacted")); + } + #[tokio::test] async fn test_volume_ec_shards_generate_persists_expire_at_sec() { let ttl = crate::storage::needle::ttl::TTL::read("3m").unwrap(); diff --git a/seaweed-volume/src/server/heartbeat.rs b/seaweed-volume/src/server/heartbeat.rs index 1601d7efb..c0fcb3cdc 100644 --- a/seaweed-volume/src/server/heartbeat.rs +++ b/seaweed-volume/src/server/heartbeat.rs @@ -886,7 +886,7 @@ fn build_heartbeat_with_ec_status( replica_placement: vol.super_block.replica_placement.to_byte() as u32, version: vol.super_block.version.0 as u32, ttl: vol.super_block.ttl.to_u32(), - compact_revision: vol.last_compact_revision() as u32, + compact_revision: vol.super_block.compaction_revision as u32, modified_at_second: vol.last_modified_ts() as i64, disk_type: loc.disk_type.to_string(), disk_id: disk_id as u32,