From 7437f99c45be4fed66eeadaa3b17073bc5502ca9 Mon Sep 17 00:00:00 2001 From: houseme Date: Sat, 11 Jul 2026 08:00:52 +0800 Subject: [PATCH] fix(cache): key the object body cache on data_dir for write-uniqueness (#4703) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor(object-data-cache): derive Default for ObjectDataCacheGetRequest The GET request literal is hand-listed field-by-field across ~13 test sites in two crates. Adding `mod_time_unix_nanos` in backlog#1111 had to touch every one and still missed a literal, producing a compile error caught only in a later CI lane. Derive `Default` and spread the engine-crate literals so the next field addition is absorbed rather than fanned out. Co-Authored-By: heihutu * fix(cache): key the object body cache on data_dir for write-uniqueness The cache key's correctness rested on being write-unique, but its only write-scoped component was `mod_time` — a wall-clock timestamp that is not monotonic and can be absent. Two writes that collide on MD5 (same etag+size) and land on an equal mod_time (clock skew, same-tick, or absent) derived the same key, so a node that never saw the overwrite could serve the previous body for up to the TTL, and the same collision turned the fill-after-invalidation race into a serving bug. This is the store's strong-read-after-write guarantee leaning on a probabilistic argument. Add `data_dir` — the xl.meta directory UUID ecstore regenerates on every body write — as the primary write-unique anchor: - surface `data_dir: Option` on ObjectInfo, copied from FileInfo in the single GET-path constructor `from_file_info`; - carry it into ObjectDataCacheKey as `data_dir_u128` (held as u128 to keep the engine crate free of a uuid dependency), derived in the one planner site both the ecstore hook and the usecase layer share, so both produce an identical key by construction; - keep `mod_time` as a second anchor and `etag+size` as belt-and-braces; an absent data_dir falls back to the prior behavior — strict improvement, no regression. Two writes distinct only by data_dir now derive different keys even under an MD5 collision with identical mod_time — the case mod_time alone cannot cover. Blast radius is compiler-guarded: ObjectInfo derives Default and every real construction site uses `..Default::default()`, so only from_file_info and one full-literal test needed the field. The three P0 body_cache_hook_e2e regressions, engine (80), and app (36) suites pass unchanged; a mutation that severs the planner wiring fails planner_key_changes_with_data_dir. Refs: backlog#1111, backlog#1118 Co-Authored-By: heihutu * fix(cache): thread data_dir field through the merged mutation-hook test Merging main (which landed the object-mutation-hook work, backlog#1131) brought in a GetRequest test literal that predates the data_dir field. Spread it via `..Default::default()` — the derive(Default) added here means this is the last such hand-listed literal to need touching. Co-Authored-By: heihutu --------- Co-authored-by: heihutu --- crates/ecstore/src/config/com.rs | 1 + crates/ecstore/src/object_api/types.rs | 6 ++ crates/object-data-cache/src/cache.rs | 28 +++--- crates/object-data-cache/src/key.rs | 97 +++++++++++++++---- rustfs/src/app/object_data_cache/adapter.rs | 1 + rustfs/src/app/object_data_cache/body.rs | 1 + .../src/app/object_data_cache/invalidation.rs | 3 + .../app/object_data_cache/mutation_hook.rs | 7 +- rustfs/src/app/object_data_cache/planner.rs | 52 +++++++++- rustfs/src/app/object_usecase.rs | 5 + 10 files changed, 161 insertions(+), 40 deletions(-) diff --git a/crates/ecstore/src/config/com.rs b/crates/ecstore/src/config/com.rs index 1c84b6b60..5733b26f3 100644 --- a/crates/ecstore/src/config/com.rs +++ b/crates/ecstore/src/config/com.rs @@ -1759,6 +1759,7 @@ mod tests { parity_blocks: 0, data_blocks: 0, version_id: None, + data_dir: None, delete_marker: false, transitioned_object: Default::default(), restore_ongoing: false, diff --git a/crates/ecstore/src/object_api/types.rs b/crates/ecstore/src/object_api/types.rs index 2f28a2828..92ac8a399 100644 --- a/crates/ecstore/src/object_api/types.rs +++ b/crates/ecstore/src/object_api/types.rs @@ -172,6 +172,10 @@ pub struct ObjectInfo { pub parity_blocks: usize, pub data_blocks: usize, pub version_id: Option, + /// xl.meta directory UUID for this version, regenerated on every body write. + /// A write-unique token: the object data cache keys on it so an overwrite + /// cannot be served the previous body under an MD5 collision (backlog#1111). + pub data_dir: Option, pub delete_marker: bool, pub transitioned_object: TransitionedObject, pub restore_ongoing: bool, @@ -211,6 +215,7 @@ impl Clone for ObjectInfo { parity_blocks: self.parity_blocks, data_blocks: self.data_blocks, version_id: self.version_id, + data_dir: self.data_dir, delete_marker: self.delete_marker, transitioned_object: self.transitioned_object.clone(), restore_ongoing: self.restore_ongoing, @@ -515,6 +520,7 @@ impl ObjectInfo { parity_blocks: fi.erasure.parity_blocks, data_blocks: fi.erasure.data_blocks, version_id, + data_dir: fi.data_dir, delete_marker: fi.deleted, mod_time: fi.mod_time, size: fi.size, diff --git a/crates/object-data-cache/src/cache.rs b/crates/object-data-cache/src/cache.rs index 229138f32..77a7df06c 100644 --- a/crates/object-data-cache/src/cache.rs +++ b/crates/object-data-cache/src/cache.rs @@ -144,12 +144,13 @@ impl ObjectDataCache { record_plan_decision(self.backend.as_metric_label(), self.config.mode, "cacheable", "eligible", request.size); ObjectDataCacheGetPlan::Cacheable { - key: ObjectDataCacheKey::with_mod_time( + key: ObjectDataCacheKey::with_write_anchors( request.bucket, request.object, request.version_id.as_deref(), request.etag, request.size, + request.data_dir_u128, request.mod_time_unix_nanos, request.body_variant, ), @@ -393,7 +394,11 @@ const fn invalidation_outcome(result: &ObjectDataCacheInvalidationResult) -> &'s } /// Protocol-neutral GET request metadata for cache planning. -#[derive(Debug, Clone)] +/// +/// Derives `Default` so tests construct it with `..Default::default()`; a new +/// field then does not have to be added to every literal (which is how the +/// `mod_time` seam bug arose during backlog#1111). +#[derive(Debug, Clone, Default)] pub struct ObjectDataCacheGetRequest<'a> { /// Bucket name. pub bucket: &'a str, @@ -405,9 +410,11 @@ pub struct ObjectDataCacheGetRequest<'a> { pub etag: &'a str, /// Object size in bytes. pub size: u64, + /// Resolved version's `data_dir` UUID as a `u128`, or `None`. Primary + /// write-unique key component (backlog#1111 / ODC-06). + pub data_dir_u128: Option, /// Resolved version's modification time as Unix nanoseconds, or `0` when - /// absent. Carried into the key so it is write-unique (backlog#1111 / - /// ODC-06). + /// absent. Second write-unique anchor. pub mod_time_unix_nanos: i128, /// Supported response body variant. pub body_variant: ObjectDataCacheBodyVariant, @@ -546,7 +553,7 @@ mod tests { ObjectDataCacheInvalidationReason, ObjectDataCacheInvalidationResult, ObjectDataCacheLookup, }; use crate::config::{ObjectDataCacheConfig, ObjectDataCacheMode}; - use crate::key::{ObjectDataCacheBodyVariant, ObjectDataCacheIdentity}; + use crate::key::ObjectDataCacheIdentity; use bytes::Bytes; use metrics_util::MetricKind; use metrics_util::debugging::{DebugValue, DebuggingRecorder}; @@ -576,11 +583,9 @@ mod tests { ObjectDataCacheGetRequest { bucket, object, - version_id: None, etag, size, - mod_time_unix_nanos: 0, - body_variant: ObjectDataCacheBodyVariant::FullObjectPlainV1, + ..Default::default() } } @@ -846,11 +851,9 @@ mod tests { let plan = cache.plan_get(ObjectDataCacheGetRequest { bucket: "bucket", object: "object", - version_id: None, etag: "etag", size: 5, - mod_time_unix_nanos: 0, - body_variant: ObjectDataCacheBodyVariant::FullObjectPlainV1, + ..Default::default() }); let fill = cache.fill_body(&plan, Bytes::from_static(b"oops")).await; @@ -900,11 +903,10 @@ mod tests { let request = ObjectDataCacheGetRequest { bucket: "bucket", object: "object", - version_id: None, etag: "etag", size: 5, mod_time_unix_nanos: 42, - body_variant: ObjectDataCacheBodyVariant::FullObjectPlainV1, + ..Default::default() }; let ObjectDataCacheGetPlan::Cacheable { key } = cache.plan_get(request) else { panic!("plan should be cacheable"); diff --git a/crates/object-data-cache/src/key.rs b/crates/object-data-cache/src/key.rs index 0c0116514..8bb2d38fa 100644 --- a/crates/object-data-cache/src/key.rs +++ b/crates/object-data-cache/src/key.rs @@ -32,10 +32,15 @@ pub enum ObjectDataCacheBodyVariant { /// two same-length payloads that collide on MD5 would derive the identical key, /// so a GET on a node that never observed the overwrite could serve the old /// bytes for up to the TTL, and the same collision turns the -/// fill-after-invalidation race (backlog#1118) into a serving bug. Including -/// the resolved version's modification time distinguishes two writes even under -/// an MD5 collision, because an overwrite advances `mod_time`. `etag + size` -/// stay in the key as belt-and-braces. +/// fill-after-invalidation race (backlog#1118) into a serving bug. +/// +/// The write-unique anchor is `data_dir` — the xl.meta directory UUID that +/// ecstore regenerates on *every* body write. Two distinct writes of the same +/// object therefore never share a `data_dir`, so the key changes on overwrite +/// even when `etag + size` collide (an MD5 collision) *and* `mod_time` is equal +/// (clock skew or same-tick writes). `mod_time_unix_nanos` remains as a second +/// anchor for reads where `data_dir` is unavailable, and `etag + size` stay as +/// belt-and-braces. #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct ObjectDataCacheKey { /// Bucket name. @@ -48,10 +53,15 @@ pub struct ObjectDataCacheKey { pub etag: Arc, /// Object size in bytes. pub size: u64, + /// Resolved version's `data_dir` UUID as a `u128` (`Uuid::as_u128`), or + /// `None` when the read did not resolve one. This is the primary + /// write-unique component: ecstore regenerates `data_dir` on every body + /// write, so it is distinct across overwrites regardless of content or + /// timestamp. Held as `u128` to keep this crate free of a `uuid` dependency. + pub data_dir_u128: Option, /// Resolved version's modification time as Unix nanoseconds - /// (`OffsetDateTime::unix_timestamp_nanos`), or `0` when absent. This is the - /// write-unique component: an overwrite advances `mod_time`, so the key - /// changes even when `etag + size` are unchanged (an MD5 collision). + /// (`OffsetDateTime::unix_timestamp_nanos`), or `0` when absent. Second + /// write-unique anchor, used when `data_dir` is unavailable. pub mod_time_unix_nanos: i128, /// Cached body semantics. pub body_variant: ObjectDataCacheBodyVariant, @@ -67,16 +77,17 @@ pub struct ObjectDataCacheIdentity { } impl ObjectDataCacheKey { - /// Creates a stable object data cache key with an explicit modification - /// time. This is the full constructor; the production GET planner uses it so - /// the key is write-unique (backlog#1111 / ODC-06). + /// Creates a stable object data cache key with explicit write anchors + /// (`data_dir` and `mod_time`). This is the full constructor; the production + /// GET planner uses it so the key is write-unique (backlog#1111 / ODC-06). #[allow(clippy::too_many_arguments)] - pub fn with_mod_time( + pub fn with_write_anchors( bucket: impl Into>, object: impl Into>, version_id: Option<&str>, etag: impl Into>, size: u64, + data_dir_u128: Option, mod_time_unix_nanos: i128, body_variant: ObjectDataCacheBodyVariant, ) -> Self { @@ -86,14 +97,15 @@ impl ObjectDataCacheKey { version_id: version_id.map_or_else(|| Arc::::from(NULL_VERSION_ID), Arc::::from), etag: etag.into(), size, + data_dir_u128, mod_time_unix_nanos, body_variant, } } - /// Creates a stable object data cache key with no modification time - /// (`mod_time_unix_nanos == 0`). Retained for callers that key purely by - /// content identity (index/backend tests). + /// Creates a stable object data cache key with no write anchors (`data_dir` + /// absent, `mod_time_unix_nanos == 0`). Retained for callers that key purely + /// by content identity (index/backend tests). pub fn new( bucket: impl Into>, object: impl Into>, @@ -102,7 +114,7 @@ impl ObjectDataCacheKey { size: u64, body_variant: ObjectDataCacheBodyVariant, ) -> Self { - Self::with_mod_time(bucket, object, version_id, etag, size, 0, body_variant) + Self::with_write_anchors(bucket, object, version_id, etag, size, None, 0, body_variant) } /// Returns true when this key targets the canonical unversioned body variant. @@ -151,33 +163,36 @@ mod tests { // ODC-06 (backlog#1111): two writes with identical etag + size (an MD5 // collision on an unversioned overwrite) must derive different keys once // the modification time differs, so a stale node cannot serve old bytes. - let old = ObjectDataCacheKey::with_mod_time( + let old = ObjectDataCacheKey::with_write_anchors( "bucket", "object", None, "etag", 42, + None, 1_000, ObjectDataCacheBodyVariant::FullObjectPlainV1, ); - let new = ObjectDataCacheKey::with_mod_time( + let new = ObjectDataCacheKey::with_write_anchors( "bucket", "object", None, "etag", 42, + None, 2_000, ObjectDataCacheBodyVariant::FullObjectPlainV1, ); assert_ne!(old, new, "keys differing only by mod_time must not collide"); // Same mod_time still collapses to one key (a true re-read of one write). - let new_again = ObjectDataCacheKey::with_mod_time( + let new_again = ObjectDataCacheKey::with_write_anchors( "bucket", "object", None, "etag", 42, + None, 2_000, ObjectDataCacheBodyVariant::FullObjectPlainV1, ); @@ -185,10 +200,54 @@ mod tests { } #[test] - fn key_new_defaults_mod_time_to_zero() { + fn key_distinguishes_writes_by_data_dir() { + // ODC-06 (backlog#1111): `data_dir` is regenerated on every body write, + // so two writes distinct only by `data_dir` — identical etag + size (MD5 + // collision) AND identical mod_time (clock skew or same-tick writes) — + // must still derive different keys. This is the case mod_time alone + // cannot cover. + let first = ObjectDataCacheKey::with_write_anchors( + "bucket", + "object", + None, + "etag", + 42, + Some(1), + 1_000, + ObjectDataCacheBodyVariant::FullObjectPlainV1, + ); + let second = ObjectDataCacheKey::with_write_anchors( + "bucket", + "object", + None, + "etag", + 42, + Some(2), + 1_000, + ObjectDataCacheBodyVariant::FullObjectPlainV1, + ); + assert_ne!(first, second, "keys differing only by data_dir must not collide"); + + // Absent data_dir falls back to the remaining anchors (no regression). + let no_data_dir = ObjectDataCacheKey::with_write_anchors( + "bucket", + "object", + None, + "etag", + 42, + None, + 1_000, + ObjectDataCacheBodyVariant::FullObjectPlainV1, + ); + assert_ne!(first, no_data_dir); + } + + #[test] + fn key_new_defaults_write_anchors() { let key = ObjectDataCacheKey::new("bucket", "object", None, "etag", 42, ObjectDataCacheBodyVariant::FullObjectPlainV1); assert_eq!(key.mod_time_unix_nanos, 0); + assert_eq!(key.data_dir_u128, None); } #[test] diff --git a/rustfs/src/app/object_data_cache/adapter.rs b/rustfs/src/app/object_data_cache/adapter.rs index a5691ac72..b3f5a79e9 100644 --- a/rustfs/src/app/object_data_cache/adapter.rs +++ b/rustfs/src/app/object_data_cache/adapter.rs @@ -521,6 +521,7 @@ mod tests { version_id: None, etag: "etag", size: 32 * 1024 * 1024, + data_dir_u128: None, mod_time_unix_nanos: 0, body_variant: rustfs_object_data_cache::ObjectDataCacheBodyVariant::FullObjectPlainV1, }); diff --git a/rustfs/src/app/object_data_cache/body.rs b/rustfs/src/app/object_data_cache/body.rs index f54c96a77..3087f12a9 100644 --- a/rustfs/src/app/object_data_cache/body.rs +++ b/rustfs/src/app/object_data_cache/body.rs @@ -124,6 +124,7 @@ mod tests { version_id: None, etag: "etag", size: 5, + data_dir_u128: None, mod_time_unix_nanos: 0, body_variant: ObjectDataCacheBodyVariant::FullObjectPlainV1, }); diff --git a/rustfs/src/app/object_data_cache/invalidation.rs b/rustfs/src/app/object_data_cache/invalidation.rs index 0c2a621b6..3e9a1be6f 100644 --- a/rustfs/src/app/object_data_cache/invalidation.rs +++ b/rustfs/src/app/object_data_cache/invalidation.rs @@ -180,6 +180,7 @@ mod tests { version_id: None, etag: "etag", size: 5, + data_dir_u128: None, mod_time_unix_nanos: 0, body_variant: ObjectDataCacheBodyVariant::FullObjectPlainV1, }); @@ -207,6 +208,7 @@ mod tests { version_id: None, etag: "etag-a", size: 5, + data_dir_u128: None, mod_time_unix_nanos: 0, body_variant: ObjectDataCacheBodyVariant::FullObjectPlainV1, }); @@ -216,6 +218,7 @@ mod tests { version_id: None, etag: "etag-b", size: 5, + data_dir_u128: None, mod_time_unix_nanos: 0, body_variant: ObjectDataCacheBodyVariant::FullObjectPlainV1, }); diff --git a/rustfs/src/app/object_data_cache/mutation_hook.rs b/rustfs/src/app/object_data_cache/mutation_hook.rs index c4e588eca..e1a4f4b8f 100644 --- a/rustfs/src/app/object_data_cache/mutation_hook.rs +++ b/rustfs/src/app/object_data_cache/mutation_hook.rs @@ -57,8 +57,7 @@ mod tests { use super::*; use bytes::Bytes; use rustfs_object_data_cache::{ - ObjectDataCacheBodyVariant, ObjectDataCacheConfig, ObjectDataCacheFillResult, ObjectDataCacheGetRequest, - ObjectDataCacheLookup, ObjectDataCacheMode, + ObjectDataCacheConfig, ObjectDataCacheFillResult, ObjectDataCacheGetRequest, ObjectDataCacheLookup, ObjectDataCacheMode, }; fn fill_enabled_adapter() -> Arc { @@ -78,11 +77,9 @@ mod tests { ObjectDataCacheGetRequest { bucket, object, - version_id: None, etag: "etag", size: 5, - mod_time_unix_nanos: 0, - body_variant: ObjectDataCacheBodyVariant::FullObjectPlainV1, + ..Default::default() } } diff --git a/rustfs/src/app/object_data_cache/planner.rs b/rustfs/src/app/object_data_cache/planner.rs index 6a273ac2d..1495fb794 100644 --- a/rustfs/src/app/object_data_cache/planner.rs +++ b/rustfs/src/app/object_data_cache/planner.rs @@ -81,9 +81,14 @@ pub(crate) fn build_get_object_body_cache_plan( .version_id .filter(|version_id| !version_id.is_nil()) .map(|version_id| version_id.to_string()); - // ODC-06 (backlog#1111): carry the resolved version's modification time into - // the key so an unversioned overwrite (which advances mod_time) cannot be - // served the stale body under an MD5 collision. Absent mod_time maps to 0. + // ODC-06 (backlog#1111): make the key write-unique. `data_dir` is the xl.meta + // directory UUID that ecstore regenerates on every body write, so it is + // distinct across overwrites regardless of content (MD5 collision) or + // timestamp; it is the primary anchor. `mod_time` is a second anchor for the + // rare read where `data_dir` is unresolved. Both derive here — the one place + // the ecstore hook and the usecase layer share — so both sites produce an + // identical key by construction. + let data_dir_u128 = request.info.data_dir.map(|data_dir| data_dir.as_u128()); let mod_time_unix_nanos = request .info .mod_time @@ -95,6 +100,7 @@ pub(crate) fn build_get_object_body_cache_plan( version_id, etag, size, + data_dir_u128, mod_time_unix_nanos, body_variant: ObjectDataCacheBodyVariant::FullObjectPlainV1, }; @@ -334,6 +340,46 @@ mod tests { assert_ne!(old_key, new_key, "keys differing only by mod_time must not collide"); } + #[test] + fn planner_key_changes_with_data_dir() { + // ODC-06 (backlog#1111): data_dir is regenerated on every body write, so + // an overwrite yields a different key even when etag + size AND mod_time + // are identical — the case mod_time alone cannot cover. Also confirms + // data_dir actually reaches the key (data_dir_u128 == the UUID's u128). + let adapter = enabled_adapter(); + let dir_a = uuid::Uuid::from_u128(0xA); + let dir_b = uuid::Uuid::from_u128(0xB); + let mut info = crate::storage::storage_api::StorageObjectInfo { + etag: Some("etag".to_string()), + size: 4, + actual_size: 4, + mod_time: Some(time::OffsetDateTime::from_unix_timestamp_nanos(1_000).unwrap()), + data_dir: Some(dir_a), + ..Default::default() + }; + let make_request = |info: &crate::storage::storage_api::StorageObjectInfo| { + build_get_object_body_cache_plan( + &adapter, + GetObjectBodyCacheRequest { + bucket: "bucket", + key: "object", + info, + response_content_length: 4, + has_range: false, + part_number: None, + encryption_applied: false, + }, + ) + }; + let key_a = cacheable_key(&make_request(&info)); + assert_eq!(key_a.data_dir_u128, Some(dir_a.as_u128()), "data_dir must reach the key"); + + // Same etag/size/mod_time, only data_dir differs → different key. + info.data_dir = Some(dir_b); + let key_b = cacheable_key(&make_request(&info)); + assert_ne!(key_a, key_b, "keys differing only by data_dir must not collide"); + } + #[test] fn plan_is_cacheable_for_plain_full_object() { let adapter = enabled_adapter(); diff --git a/rustfs/src/app/object_usecase.rs b/rustfs/src/app/object_usecase.rs index 27b75dcf8..6e5e26df5 100644 --- a/rustfs/src/app/object_usecase.rs +++ b/rustfs/src/app/object_usecase.rs @@ -7178,6 +7178,7 @@ mod tests { version_id: None, etag, size, + data_dir_u128: None, mod_time_unix_nanos: 0, body_variant: rustfs_object_data_cache::ObjectDataCacheBodyVariant::FullObjectPlainV1, }); @@ -7589,6 +7590,7 @@ mod tests { version_id: None, etag: "etag", size: 5, + data_dir_u128: None, mod_time_unix_nanos: 0, body_variant: rustfs_object_data_cache::ObjectDataCacheBodyVariant::FullObjectPlainV1, }); @@ -7650,6 +7652,7 @@ mod tests { version_id: None, etag: "etag", size: 5, + data_dir_u128: None, mod_time_unix_nanos: 0, body_variant: rustfs_object_data_cache::ObjectDataCacheBodyVariant::FullObjectPlainV1, }); @@ -7798,6 +7801,7 @@ mod tests { version_id: None, etag: "etag", size: 5, + data_dir_u128: None, mod_time_unix_nanos: 0, body_variant: rustfs_object_data_cache::ObjectDataCacheBodyVariant::FullObjectPlainV1, }); @@ -7864,6 +7868,7 @@ mod tests { version_id: None, etag: "etag", size: 5, + data_dir_u128: None, mod_time_unix_nanos: 0, body_variant: rustfs_object_data_cache::ObjectDataCacheBodyVariant::FullObjectPlainV1, });