Skip to content

Add an optional Foyer-backed disk cache - #87

Open
zhangstar333 wants to merge 1 commit into
lance-format:mainfrom
zhangstar333:lance_foyer_with_index
Open

zhangstar333 wants to merge 1 commit into
lance-format:mainfrom
zhangstar333:lance_foyer_with_index

Conversation

@zhangstar333

@zhangstar333 zhangstar333 commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

No description provided.

@zhangstar333
zhangstar333 force-pushed the lance_foyer_with_index branch 3 times, most recently from cc6ff8c to bd32bf6 Compare September 24, 2026 08:26
index cache

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ 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.

Comment thread src/foyer_data_cache.rs

fn key(&self, store_prefix: &str, location: &Path, block_index: u64) -> String {
format!(
"{CACHE_KEY_VERSION}\0{}\0{store_prefix}\0{}\0{block_index}",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lance-gatekeeper lance-gatekeeper Bot added the K-changes Latest Gatekeeper recommendation requests changes. label Sep 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

K-changes Latest Gatekeeper recommendation requests changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant