Add an optional Foyer-backed disk cache - #87
zhangstar333 wants to merge 1 commit into
Conversation
cc6ff8c to
bd32bf6
Compare
bd32bf6 to
bb20814
Compare
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The optional Foyer cache addresses repeated remote reads, but its persistent namespace can join different backing stores or authorization contexts that share a Lance store prefix. Please give both data blocks and serialized index entries a stable origin and access scope, or enforce one such scope for the cache directory before reusing its entries across opens.
|
|
||
| fn key(&self, store_prefix: &str, location: &Path, block_index: u64) -> String { | ||
| format!( | ||
| "{CACHE_KEY_VERSION}\0{}\0{store_prefix}\0{}\0{block_index}", |
There was a problem hiding this comment.
The key treats store_prefix as a complete origin identity. In the pinned Lance S3 provider the default prefix is s3$bucket, ignoring endpoint and credential options, so distinct origins can generate the same size and block keys. get_ranges then serves a persisted hit without contacting the second origin; it can return another endpoint's bytes or bypass that origin's read permissions. The directory lock only prevents simultaneous owners, not sequential reuse. Upstream #7721 identifies this prefix collision and is closed without a merge.
Include a stable, non-secret origin and authorization identity in both cache tiers, or restrict a directory to one such identity and reject conflicting opens.
Executed regression test
Add this to the existing foyer_data_cache.rs test module:
#[tokio::test]
async fn repro_cross_origin_cache_hit() {
let directory = tempfile::tempdir().unwrap();
let location = Path::from("table.lance/data/part-0.lance");
let first_origin = Arc::new(InMemory::new());
first_origin
.put(&location, Bytes::from_static(b"private").into())
.await
.unwrap();
let first_cache = FoyerDataCache::try_new(directory.path(), 1024 * 1024, 64 * 1024)
.await
.unwrap();
let (first_store, first_scope) = wrap_for_test(&first_cache, first_origin);
assert_eq!(
first_store.get_ranges(&location, &[0..7]).await.unwrap(),
vec![Bytes::from_static(b"private")]
);
drop(first_store);
drop(first_scope);
first_cache.cache.close().await.unwrap();
drop(first_cache);
crate::foyer_cache::wait_for_directory_release(directory.path()).await;
let second_cache = FoyerDataCache::try_new(directory.path(), 1024 * 1024, 64 * 1024)
.await
.unwrap();
let (second_store, _) = wrap_for_test(&second_cache, Arc::new(InMemory::new()));
let result = second_store.get_ranges(&location, &[0..7]).await;
assert!(result.is_err(), "second origin unexpectedly returned {result:?}");
}cargo test --lib repro_cross_origin_cache_hit -- --nocapture failed: the empty second origin should produce an error, but returned Ok([b"private"]) from the first owner's persisted cache.
No description provided.