diff --git a/crates/filemeta/src/filemeta.rs b/crates/filemeta/src/filemeta.rs index 41b799c28..a49063eec 100644 --- a/crates/filemeta/src/filemeta.rs +++ b/crates/filemeta/src/filemeta.rs @@ -356,6 +356,14 @@ impl FileMeta { .versions .partition_point(|existing| existing.header.sorts_before(&new_shallow.header)); self.versions.insert(insert_pos, new_shallow); + // `partition_point` only returns the canonical slot when `versions` is + // already canonically ordered, and nothing establishes that: `FileMeta::load` + // replays the on-disk order verbatim, and metadata written before the + // canonical-order fix ordered equal-`mod_time` ties by insertion rather than + // by `sorts_before`. Re-sort so an insert leaves the list canonical either + // way, matching what `set_idx` already does on the replace path. Cheap: after + // a correctly placed insert the slice is a single sorted run. + self.sort_by_mod_time(); Ok(()) // if !ver.valid() { @@ -1215,6 +1223,57 @@ mod test { assert!(fm.versions[0].header.sorts_before(&fm.versions[1].header)); } + /// `add_version_filemata` positions an inserted version with + /// `partition_point(sorts_before)`, which only yields the canonical slot when + /// `versions` is already canonically ordered. Nothing establishes that + /// precondition on load: `FileMeta::unmarshal_msg` pushes versions in file + /// order, and metadata written before the canonical-order fix ordered + /// equal-`mod_time` ties by insertion instead of by `sorts_before`. An insert + /// into such a list must still leave it canonical, the same way `set_idx` + /// already re-sorts on the replace path. + #[test] + fn add_version_filemata_canonicalizes_versions_loaded_out_of_order() { + let mod_time = OffsetDateTime::from_unix_timestamp(1_700_000_000).expect("valid test timestamp"); + let first = version_for_ordering(VersionType::Object, Uuid::from_u128(70), mod_time, 70); + let second = version_for_ordering(VersionType::Object, Uuid::from_u128(80), mod_time, 80); + let (early, late) = if first.header().sorts_before(&second.header()) { + (first, second) + } else { + (second, first) + }; + + // Persist the pair the wrong way round to emulate a pre-canonical-order + // xl.meta, bypassing add_version_filemata so the bad order really lands on + // the wire. + let mut legacy = FileMeta::new(); + legacy + .versions + .push(FileMetaShallowVersion::try_from(late).expect("shallow later version")); + legacy + .versions + .push(FileMetaShallowVersion::try_from(early).expect("shallow earlier version")); + + let mut loaded = + FileMeta::load(&legacy.marshal_msg().expect("serialize legacy metadata")).expect("reload legacy metadata"); + assert!( + !loaded.versions[0].header.sorts_before(&loaded.versions[1].header), + "fixture must reach add_version_filemata non-canonically ordered" + ); + + loaded + .add_version_filemata(version_for_ordering(VersionType::Delete, Uuid::from_u128(90), mod_time, 90)) + .expect("insert equal-time delete marker"); + + assert_eq!(loaded.versions.len(), 3); + assert!( + loaded + .versions + .windows(2) + .all(|pair| pair[0].header.sorts_before(&pair[1].header)), + "insert must leave the version list canonically ordered" + ); + } + /// Regression for backlog#799 B16: replication reset state must be /// persisted under both internal prefixes, never as a bare ARN. A bare-ARN /// key (produced by `ObjectInfo::replication_state`) has no internal prefix,