Skip to content

Add an optional Foyer-backed disk cache - #87

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

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

not merge to main branch, only as patch.

@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
@zhangstar333
zhangstar333 marked this pull request as draft September 28, 2026 05:19
@lance-gatekeeper lance-gatekeeper Bot removed the K-changes Latest Gatekeeper recommendation requests changes. label Sep 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant