mirror of
https://github.com/rustfs/rustfs.git
synced 2026-08-06 05:17:42 +00:00
fix(cache): key the object body cache on data_dir for write-uniqueness (#4703)
* 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 <heihutu@gmail.com> * 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<Uuid>` 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 <heihutu@gmail.com> * 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 <heihutu@gmail.com> --------- Co-authored-by: heihutu <heihutu@gmail.com>
This commit is contained in:
@@ -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,
|
||||
});
|
||||
|
||||
@@ -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,
|
||||
});
|
||||
|
||||
@@ -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,
|
||||
});
|
||||
|
||||
@@ -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<ObjectDataCacheAdapter> {
|
||||
@@ -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()
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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,
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user