feat(jni,io): default-open Session reuse + ObjectStoreRegistry single-flight by LuciferYang · Pull Request #6839 · lance-format/lance

LuciferYang · 2026-05-19T11:48:53Z

Coalesces concurrent cold Dataset::open against the same URI so N parallel opens reuse one ObjectStore instead of each rebuilding the credential chain and HTTP client.

Two changes, both required:

lance-io — registry single-flight. ObjectStoreRegistry::get_store gains a per-CacheKey tokio::sync::Mutex so concurrent cold builds serialize behind one in-flight build(). The lock map entry is freed via a BuildLockSession RAII guard that drops the inner mutex first, then removes the HashMap entry if it observes strong_count == 1, making panics / errors / cancellations all reclaim the slot. Failures are not cached (one-Err-per-N-waiters retry); an opportunistic sweep gated on cold_attempt_nth % 64 == 0 amortizes HashMap shrinkage.

lance-jni — default-open registry sharing. BlockingDataset::open without an explicit Session now uses a process-wide LazyLock<Arc<ObjectStoreRegistry>> (the Session itself is still per-call so metadata/index caches stay isolated). This is what makes the lance-io fix actually coalesce across JNI default opens — without it each open had its own registry and had nothing to coalesce against.

Bare-URI opens (empty storage options, no provider, no namespace handler) collapse onto one cache entry per URI: the first caller's resolved default-credential chain becomes the credentials used by every subsequent caller for the lifetime of that Arc<ObjectStore>. This is documented in java/README.md along with three opt-outs:

env LANCE_JNI_DISABLE_DEFAULT_REGISTRY_SHARING={1,true,yes}
pass tenant-distinguishing storage options
supply an explicit Session

Benchmark

144 concurrent opens against the same URI, semaphore-gated to 12, S3 → MinIO :9000, BENCH_SHARED_SESSION=1, run via the standalone reproducer in the Tests section below:

	wall	p50	p99	hits / misses
baseline `ad9f38227`	9.50 s	784 ms	828 ms	0 / 144
this PR	0.18 s	2.3 ms	144 ms	143 / 1

no-shared mode (each open creates its own Session) is unchanged between branches, as expected.

Tests

lance-io: hit/miss, single-flight coalescing, panic + error RAII cleanup, sanitized_authority (IPv6 / userinfo / non-default port), opportunistic sweep counter.
lance-jni: env-var truthy whitelist + warn path, DisableSharingTestGuard ordering, end-to-end test driving 144 concurrent default-opens through the JNI layer and asserting a single ObjectStore build.
Standalone reproducer for the benchmark above, drop into rust/lance/examples/concurrent_open_bench.rs:

concurrent_open_bench.rs

// SPDX-License-Identifier: Apache-2.0
// SPDX-FileCopyrightText: Copyright The Lance Authors

//! Concurrent `Dataset::open` reproducer
//!
//! Reproduces the JNI default-open path bottleneck: when many threads call
//! `Dataset::open(uri)` concurrently with no shared `Session`, every call
//! constructs a fresh `ObjectStoreRegistry` and a cold object-store client,
//! which dominates wall time even when most opens hit the same URI.
//!
//! Usage:
//! ```bash
//! BENCH_URI=s3://bucket/foo.lance \
//!   BENCH_CONCURRENCY=12 BENCH_TOTAL=144 \
//!   BENCH_STORAGE_OPTS='aws_endpoint=http://localhost:9000,aws_access_key_id=...' \
//!   cargo run --release --example concurrent_open_bench
//! ```
//!
//! Set `BENCH_SHARED_SESSION=1` to thread one `Arc<Session>` through every
//! `DatasetBuilder` — i.e. the candidate fix — and compare wall time + the
//! registry's hit/miss counters against the baseline.

#![allow(clippy::print_stdout)]

use std::collections::HashMap;
use std::sync::Arc;
use std::time::{Duration, Instant};

use lance::dataset::builder::DatasetBuilder;
use lance::session::Session;
use tokio::sync::Semaphore;

fn parse_storage_opts(s: &str) -> HashMap<String, String> {
    s.split(',')
        .filter_map(|kv| {
            let kv = kv.trim();
            if kv.is_empty() {
                return None;
            }
            let mut it = kv.splitn(2, '=');
            Some((it.next()?.trim().to_string(), it.next()?.trim().to_string()))
        })
        .collect()
}

fn percentile(latencies: &[Duration], p: f64) -> Duration {
    if latencies.is_empty() {
        return Duration::ZERO;
    }
    let mut sorted: Vec<_> = latencies.to_vec();
    sorted.sort();
    let idx = ((p / 100.0) * (sorted.len() - 1) as f64).round() as usize;
    sorted[idx]
}

fn env_usize(key: &str, default: usize) -> usize {
    std::env::var(key)
        .ok()
        .and_then(|s| s.parse().ok())
        .unwrap_or(default)
}

#[tokio::main(flavor = "multi_thread")]
async fn main() -> Result<(), Box<dyn std::error::Error>> {
    let uri = std::env::var("BENCH_URI").map_err(|_| "BENCH_URI must be set")?;
    let concurrency = env_usize("BENCH_CONCURRENCY", 12);
    let total = env_usize("BENCH_TOTAL", 144);
    let storage_opts = std::env::var("BENCH_STORAGE_OPTS")
        .map(|s| parse_storage_opts(&s))
        .unwrap_or_default();
    let shared_session = std::env::var("BENCH_SHARED_SESSION").is_ok();
    let warm = std::env::var("BENCH_WARM").is_ok();

    println!("=== concurrent_open_bench ===");
    println!("URI               = {}", uri);
    println!("concurrency       = {}", concurrency);
    println!("total opens       = {}", total);
    println!(
        "storage_opts keys = {:?}",
        storage_opts.keys().collect::<Vec<_>>()
    );
    println!("shared_session    = {}", shared_session);
    println!("warm (1 prep open)= {}", warm);
    println!();

    let shared = if shared_session {
        Some(Arc::new(Session::default()))
    } else {
        None
    };

    // Hold the warm dataset alive so its Arc<ObjectStore> keeps the registry's
    // weak entry upgradeable for the entire concurrent run — otherwise the first
    // open's strong Arc dies before the next acquires the permit.
    let _warm_dataset_keepalive = if warm {
        let mut b = DatasetBuilder::from_uri(&uri).with_storage_options(storage_opts.clone());
        if let Some(s) = shared.as_ref() {
            b = b.with_session(s.clone());
        }
        let t0 = Instant::now();
        let ds = b.load().await?;
        println!("warmup open = {:?}", t0.elapsed());
        println!();
        Some(ds)
    } else {
        None
    };

    let semaphore = Arc::new(Semaphore::new(concurrency));
    let mut handles = Vec::with_capacity(total);
    let wall_start = Instant::now();

    for i in 0..total {
        let uri = uri.clone();
        let storage_opts = storage_opts.clone();
        let semaphore = semaphore.clone();
        let shared = shared.clone();
        handles.push(tokio::spawn(async move {
            let _permit = semaphore.acquire_owned().await.unwrap();
            let t0 = Instant::now();
            let mut builder = DatasetBuilder::from_uri(&uri).with_storage_options(storage_opts);
            if let Some(s) = shared {
                builder = builder.with_session(s);
            }
            let res = builder.load().await;
            let elapsed = t0.elapsed();
            (i, res.is_ok(), elapsed, res.err().map(|e| e.to_string()))
        }));
    }

    let mut latencies = Vec::with_capacity(total);
    let mut errors: Vec<(usize, String)> = Vec::new();
    for h in handles {
        let (i, ok, elapsed, err) = h.await?;
        latencies.push(elapsed);
        if !ok {
            errors.push((i, err.unwrap_or_default()));
        }
    }
    let wall = wall_start.elapsed();

    println!("--- results ---");
    println!("wall            = {:?}", wall);
    println!("succeeded       = {} / {}", total - errors.len(), total);
    println!("p50 per-open    = {:?}", percentile(&latencies, 50.0));
    println!("p95 per-open    = {:?}", percentile(&latencies, 95.0));
    println!("p99 per-open    = {:?}", percentile(&latencies, 99.0));
    println!(
        "max per-open    = {:?}",
        latencies.iter().max().copied().unwrap_or_default()
    );

    if let Some(s) = shared {
        let stats = s.store_registry().stats();
        println!("registry hits   = {}", stats.hits);
        println!("registry misses = {}", stats.misses);
        println!("registry active = {}", stats.active_stores);
    } else {
        println!("(no shared session — registry stats unavailable)");
    }

    if !errors.is_empty() {
        println!();
        println!("--- first 5 errors ---");
        for (i, e) in errors.iter().take(5) {
            println!("  task {}: {}", i, e);
        }
    }
    Ok(())
}

Summary by CodeRabbit

New Features
- Added a Java environment-variable option to disable default shared object-store registry reuse for dataset opens.
- Improved concurrent dataset opening by coalescing cold object-store builds and adding more build-state metrics (in-progress builds and failures).
Bug Fixes
- Prevented URL-embedded credentials (userinfo) from affecting object-store cache keys/prefixes, logs, and backend routing.
- Ensured failed/cancelled cold builds don’t pollute caches or leave lingering build locks.
Documentation
- Documented the Java configuration and clarified object-store registry cache-isolation requirements.

claude

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

codecov · 2026-05-19T12:30:32Z

Codecov Report

❌ Patch coverage is 94.96222% with 20 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
rust/lance-io/src/object_store/providers.rs	94.04%	4 Missing and 16 partials ⚠️

📢 Thoughts on this report? Let us know!

claude

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

Xuanwo · 2026-06-21T11:45:01Z

+    ///
+    /// Steady-state should be 0 — non-zero indicates either an in-progress
+    /// thundering herd (expected during cold start) or a stuck builder.
+    pub build_locks: usize,


Adding these public fields changes the source shape of a public stats struct, so downstream code that constructs it with a struct literal will stop compiling.

Good catch. ObjectStoreRegistryStats is a pub struct with pub fields and had no #[non_exhaustive], so adding build_locks/build_failures would break any external struct-literal construction. I've added #[non_exhaustive] so future field additions stay source-compatible. It's #[doc(hidden)] and only ever constructed in-crate via stats(); external code reads it (and can still ..Default::default()), never literal-constructs it. Fixed in aa80cab.

Xuanwo · 2026-06-21T11:45:01Z

+    ///
+    /// # Userinfo / IPv6 handling
+    ///
+    /// Overrides that build the prefix from URL authority MUST use


The new authority-stripping contract is not enforced across the registered authority-based providers, so URL userinfo can still remain in cache keys and be emitted by the new cache-key debug logs.

You're right — the contract wasn't enforced everywhere. I've routed every registered authority-based provider's prefix through sanitized_authority:

huggingface calculate_object_store_prefix (was raw url.authority())

shared-memory — both the prefix and the shared_backend_for backend-routing key, so the two notions of "authority identity" stay consistent

the unknown-scheme fallback in object_store.rs

S3/GCS/OSS/Tencent already inherit the sanitized default impl (no override). Azure stays on url.authority() intentionally — it parses the userinfo position as the container name, per the trait doc — and the aws.rs --x-s3 check is a boolean test, not a cache-key/log surface. Added userinfo-stripping tests for the HF and shared-memory paths (plus a backend-consistency test). Fixed in aa80cab.

Xuanwo · 2026-06-21T11:45:01Z

+        // (guard released *before* cleanup) is encoded in
+        // `BuildLockSession::drop`, so the cold path can't accidentally
+        // invert it via `let` reordering.
+        let lock = self.acquire_build_lock(&cache_key);


A waiter cancelled while queued on the per-key build lock can leave the lock-map entry behind. Timeout-heavy callers can accumulate one stale entry per cache key even though later success, error, and panic paths clean up.

Thanks — confirmed and fixed. The root cause was that cleanup keyed off Arc::strong_count: a waiter cancelled while parked in lock_owned().await dropped without reclaiming the entry, because the pending future still held an Arc clone, so the count never fell to the idle value.

I replaced the heuristic with an explicit BuildLockEntry::waiters count. acquire_build_lock increments it under the build_locks mutex, and the RAII BuildLockSession — now constructed before the lock_owned().await, with no .await between the increment and the construction — decrements it on every unwinding exit. So a waiter cancelled while still queued drops a live session and reclaims the entry, and cleanup no longer depends on Arc::strong_count or async drop order.

Covered by two tests: a white-box one that drops a guard: None session (the exact cancelled-while-queued state) and asserts the entry is reclaimed even with another Arc to the lock alive, and an end-to-end one that aborts a queued waiter and asserts the count drops 2 → 1 → 0. Fixed in aa80cab.

LuciferYang · 2026-06-21T11:50:01Z

will respond to the review comments tomorrow.

LuciferYang · 2026-06-22T07:09:48Z

need rebase after #7384 merged

…-flight Coalesce concurrent cold `Dataset.open` calls in the JNI layer so 144 parallel opens against the same URI reuse one `ObjectStore` instead of each rebuilding the credential chain and HTTP client. `BlockingDataset::open` without an explicit `Session` now constructs a fresh per-call `Session` whose `ObjectStoreRegistry` is the process-wide `GLOBAL_OBJECT_STORE_REGISTRY` (a `LazyLock<Arc<...>>`). Sharing only the registry — not the `Session` — keeps the asymmetric caching shape: the registry coalesces expensive `ObjectStore` builds (credential probe, IMDS, TLS handshake), while metadata/index caches remain per-call so callers do not share eviction policy or cache size with other tenants. Bare-URI opens (empty storage options, no provider, no namespace commit handler) collapse onto a single cache entry per URI: the first caller's resolved default-credential chain becomes the credentials used by every subsequent caller for the lifetime of that `Arc<ObjectStore>`. This cross-tenant credential bleed is intentional under the bare-URI invariant; callers that need per-tenant isolation can opt out via `LANCE_JNI_DISABLE_DEFAULT_REGISTRY_SHARING={1,true,yes}`, pass tenant-distinguishing storage options, or supply an explicit `Session`. The opt-out env var is parsed once into an `AtomicU8` tri-state (`UNINIT/ENABLED/DISABLED`) on first read, with a truthy whitelist (`1|true|yes` — anything else falls back to enabled and warns). A `DisableSharingTestGuard` RAII helper resets the atomic for unit tests so the env-var probe can be exercised deterministically. `ObjectStoreRegistry::get_store` adds a per-key build-lock so concurrent cold builds for the same `CacheKey` serialize behind one in-flight `build()` instead of all racing the credential chain. The lock map is `Mutex<HashMap<CacheKey, Arc<tokio::sync::Mutex<()>>>>`; each caller acquires its slot via a `BuildLockSession` RAII guard whose `Drop` releases the inner mutex first, then opportunistically removes the HashMap entry if it observes `strong_count == 1`. This ordering guarantees that panics, errors, and cancellations all reclaim the build-lock entry without leaking a permanently-locked key. Failures are not cached — when a cold build returns `Err`, the next waiter retries from scratch (one-Err-per-N-waiters retry contract). An opportunistic sweep gated by `cold_attempt_nth.is_multiple_of(64)` amortizes HashMap shrinkage across cold-path attempts so a long-lived process does not accumulate stale entries from short-lived URIs. The `CacheKey` uses `sanitized_authority` derived from `Url::host()` (IPv6 brackets preserved, port preserved, userinfo stripped) so URIs that differ only in embedded credentials still collapse onto the same cache slot. - `lance-io`: hit/miss, single-flight coalescing, panic-path RAII cleanup, error-path RAII cleanup, `sanitized_authority` over IPv6 / userinfo / non-default port, opportunistic sweep counter. - `lance-jni`: `parse_disable_value` truthy whitelist + warn path, `DisableSharingTestGuard` ordering, end-to-end coalescing test that drives 144 concurrent default-opens through the JNI layer and asserts a single `ObjectStore` is built. `java/README.md` documents the `LANCE_JNI_DISABLE_DEFAULT_REGISTRY_SHARING` env var, the bare-URI credential-bleed invariant, and the three opt-out paths (env var, tenant-distinguishing storage options, explicit `Session`).

…w_pq

- ObjectStoreRegistryStats: add #[non_exhaustive] so future field additions stay source-compatible (review comment 1). - Sanitize URL authority into the cache-key prefix across every registered authority-based provider (huggingface, shared-memory prefix + backend key, and the unknown-scheme fallback), so embedded userinfo can no longer leak into cache keys or the cache-key debug logs (review comment 2). - Make per-key build-lock cleanup cancellation-safe via an explicit waiter count on BuildLockEntry: acquire increments, the RAII BuildLockSession (now constructed before the lock_owned() await) decrements on every unwinding exit. A waiter cancelled while queued no longer orphans its build_locks entry; cleanup no longer depends on Arc::strong_count or async drop order (review comment 3). Adds tests for each: userinfo stripping (huggingface + shared-memory), backend-routing consistency, and the cancelled-while-queued waiter (white-box guard-less drop + end-to-end waiter-count observation).

Merging main brought in the new GooseFS object-store provider, whose calculate_object_store_prefix used raw url.authority() — re-introducing the userinfo-in-cache-key leak that was fixed for the other authority-based providers. Route it through sanitized_authority (host[:port] preserved, userinfo stripped) and add a userinfo-stripping test. Also sync java/lance-jni/Cargo.lock (rayon) to match the merged manifests so --locked builds stay consistent.

coderabbitai · 2026-07-15T03:40:29Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 549628a0-44e4-4401-934d-15e3376cb3f2

📥 Commits

Reviewing files that changed from the base of the PR and between 45eb72a and 6d5317b.

📒 Files selected for processing (1)

rust/lance-io/src/object_store/storage_options.rs

📝 Walkthrough

Walkthrough

The changes sanitize object-store cache keys, add per-key cold-build coordination and cleanup statistics, and integrate optional process-wide registry sharing into JNI default dataset opens through an environment-controlled selection path.

Changes

Object-store registry behavior

Layer / File(s)	Summary
Sanitize object-store cache keys `rust/lance-io/src/object_store.rs`, `rust/lance-io/src/object_store/providers*.rs`	Cache prefixes and shared-memory backend keys remove URL userinfo while preserving host, port, and IPv6 formatting; regression tests cover provider-specific and shared-memory behavior.
Coordinate registry cold builds `rust/lance-io/src/object_store/providers.rs`	`ObjectStoreRegistry` serializes same-key cold builds, rechecks cached stores, tracks build locks and failures, sweeps stale entries periodically, and cleans lock state during success, failure, cancellation, and panic paths.
Integrate shared registry into JNI opens `java/lance-jni/src/lib.rs`, `java/lance-jni/src/blocking_dataset.rs`, `java/lance-jni/Cargo.toml`, `java/README.md`	JNI default opens select a process-wide registry unless `LANCE_JNI_DISABLE_DEFAULT_REGISTRY_SHARING` disables sharing; parsing, selection, open wiring, tests, and configuration documentation are added.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant JNI Dataset.open
  participant LanceSession
  participant ObjectStoreRegistry
  participant ObjectStoreProvider
  JNI Dataset.open->>LanceSession: create default session
  LanceSession->>ObjectStoreRegistry: get_store(cache_key)
  ObjectStoreRegistry->>ObjectStoreProvider: build store once for cold key
  ObjectStoreProvider-->>ObjectStoreRegistry: ObjectStore
  ObjectStoreRegistry-->>LanceSession: shared Arc<ObjectStore>
  LanceSession-->>JNI Dataset.open: open result

Suggested reviewers: xuanwo

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is concise and accurately summarizes the JNI/session reuse and ObjectStoreRegistry single-flight changes.
Linked Issues check	✅ Passed	The PR implements per-key build locks in ObjectStoreRegistry get_store, serializing cold builds and reusing the resulting ObjectStore as requested in `#6838`.
Out of Scope Changes check	✅ Passed	No clear out-of-scope changes stand out; the JNI sharing, prefix sanitization, docs, and tests all support the same cache-isolation fix.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 9

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@java/lance-jni/src/blocking_dataset.rs`:
- Around line 183-218: Update DisableSharingTestGuard and the production flag
lookup to use a test-only thread-local override before consulting
DISABLE_DEFAULT_REGISTRY_SHARING. Make DisableSharingTestGuard::set and Drop set
and restore the current thread’s override, preserving nested LIFO behavior
without mutating the process-global atomic during tests.
- Around line 3939-3983: Convert the three parse_disable_value
tests—parse_disable_value_accepts_truthy, parse_disable_value_accepts_falsy, and
parse_disable_value_returns_none_for_unrecognized—to rstest parameterized tests.
Define one case per input and expected result, use rstest’s case/parameter
attributes to report each value independently, and preserve the existing truthy,
falsy, and None expectations.
- Around line 163-180: Update select_default_open_registry to prevent global
registry reuse for unkeyed opens when credential identity is unavailable.
Require a stable tenant/credential identity before selecting
crate::GLOBAL_OBJECT_STORE_REGISTRY, or make bare-URI sharing explicitly opt-in
while preserving fresh-registry isolation otherwise; apply the same policy to
the related call site.
- Around line 3985-4069: Replace
jni_default_open_coalesces_concurrent_cold_builds’ direct registry.get_store
calls with concurrent BlockingDataset::open calls through the JNI/default-open
path, using the existing test provider setup and asserting one build plus shared
store identity. Ensure the test exercises the registry selected by
BlockingDataset::open rather than independently selecting and invoking a
registry, and remove the duplicated provider-level single-flight coverage.

In `@rust/lance-io/src/object_store/providers.rs`:
- Around line 1049-1053: Update the regression test loop over handles to extract
each result’s error instead of only checking is_err(). Assert that the error has
the InvalidInput variant and contains the exact message “simulated cold-build
failure,” while preserving the existing waiter count behavior.
- Around line 543-553: Update release_build_lock to assert that cache_key has an
existing build-lock entry and that its waiter count is greater than zero before
decrementing, using descriptive debug_assert! messages. Preserve removal when
the count reaches zero, and add a safe fallback for release-build behavior so a
missing entry or invalid count cannot underflow or be silently ignored.
- Around line 433-443: Update the sweep scheduling around self.misses and
cold_attempt_nth to use the zero-based value returned by fetch_add directly,
removing wrapping_add and adjusting the cadence condition so every 64th attempt
remains eligible. Update the related comments to describe zero-based sequencing
and eliminate the wrapping-overflow rationale; apply the same change to the
corresponding logic near the additional referenced location.
- Around line 414-421: Rename the actively used BuildLockSession binding from
_session to session in this lock acquisition flow, and update the session.guard
assignment and related references consistently. Keep the existing
cancellation-safety and Drop behavior unchanged.
- Around line 984-987: Update the documentation comment for the failure provider
near its invocation-count tracking to state that the test verifies one build per
caller, rather than claiming concurrent failures collapse into a single
underlying call. Keep the description of the simulated non-panic build failure
unchanged.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: e98ebbe7-c679-4a64-aa5d-bd8ef795a900

📥 Commits

Reviewing files that changed from the base of the PR and between cf3d850 and a62e577.

⛔ Files ignored due to path filters (1)

java/lance-jni/Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (9)

java/README.md
java/lance-jni/Cargo.toml
java/lance-jni/src/blocking_dataset.rs
java/lance-jni/src/lib.rs
rust/lance-io/src/object_store.rs
rust/lance-io/src/object_store/providers.rs
rust/lance-io/src/object_store/providers/goosefs.rs
rust/lance-io/src/object_store/providers/huggingface.rs
rust/lance-io/src/object_store/providers/shared_memory.rs

lance-io/providers.rs: - rename `_session` -> `session` (it is read via `session.guard`) - drop `wrapping_add` in sweep scheduling; gate on the 0-based `fetch_add` value directly (`% N == N-1`), no self-owned counter arithmetic to overflow - `release_build_lock`: `debug_assert!` the entry exists and its waiter count is positive, `saturating_sub` + missing-entry no-op as the release-build fallback instead of silently underflowing - fix stale FailingProvider doc comment (failures serialize + retry per caller, they do not collapse into one call) - assert the exact `InvalidInput` variant and message in the serialized-failure test instead of bare `is_err()` lance-jni/blocking_dataset.rs: - make the test-only sharing override thread-local so parallel tests cannot observe each other's temporary setting - parameterize the `parse_disable_value` cases with `rstest` - replace the duplicated registry unit test with `open_wires_selected_registry_into_session`, which drives the real `BlockingDataset::open` path and pins that the selected registry is wired into the Session (global when sharing on, fresh when off); provider single-flight stays covered in providers.rs Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

coderabbitai

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

rust/lance-io/src/object_store/providers.rs (1)
574-592: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add #[cfg_attr(coverage, coverage(off))] to test-only accessor helpers.

build_locks_len and build_lock_waiters are #[cfg(test)] observability helpers used purely to assert internal state in tests; per repo convention they should be excluded from coverage instrumentation.
♻️ Proposed fix
     #[cfg(test)]
+    #[cfg_attr(coverage, coverage(off))]
     fn build_locks_len(&self) -> usize {
         self.build_locks
             .lock()
             .unwrap_or_else(|poisoned| poisoned.into_inner())
             .len()
     }
 
     /// Current waiter count for `cache_key`, or `None` if no entry exists.
     /// Lets tests observe the per-key acquire/release count directly — the
     /// signal that drives cleanup — rather than only the map's key count.
     #[cfg(test)]
+    #[cfg_attr(coverage, coverage(off))]
     fn build_lock_waiters(&self, cache_key: &CacheKey) -> Option<usize> {
As per coding guidelines, "Exclude test utilities from coverage with #[cfg_attr(coverage, coverage(off))]."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance-io/src/object_store/providers.rs` around lines 574 - 592, Add
#[cfg_attr(coverage, coverage(off))] to both test-only accessor methods,
build_locks_len and build_lock_waiters, while preserving their existing
#[cfg(test)] attributes and behavior.
Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@rust/lance-io/src/object_store/providers.rs`:
- Around line 574-592: Add #[cfg_attr(coverage, coverage(off))] to both
test-only accessor methods, build_locks_len and build_lock_waiters, while
preserving their existing #[cfg(test)] attributes and behavior.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: ddce404b-c3f3-4a62-8a7f-73f2289c18a0

📥 Commits

Reviewing files that changed from the base of the PR and between a62e577 and 45eb72a.

⛔ Files ignored due to path filters (1)

java/lance-jni/Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (3)

java/lance-jni/Cargo.toml
java/lance-jni/src/blocking_dataset.rs
rust/lance-io/src/object_store/providers.rs

…ontract a MUST The registry keys its ObjectStore cache solely on `provider_id()`, so it is the only isolation boundary between providers. Spell out the security consequence on the trait method: a provider that vends principal-specific credentials MUST encode that identity in its ID, because two providers returning the same ID share one cached ObjectStore (and thus one resolved credential chain). Doc-only; no behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

claude Bot reviewed May 19, 2026

View reviewed changes

github-actions Bot added enhancement New feature or request A-java Java bindings + JNI labels May 19, 2026

LuciferYang mentioned this pull request May 19, 2026

refactor: remove Java-side dataset cache, rely on Rust-side Session lance-format/lance-spark#353

Merged

LuciferYang marked this pull request as draft May 19, 2026 11:53

LuciferYang force-pushed the feat/session-reuse-singleflight branch 2 times, most recently from d26a288 to 9e5b879 Compare May 19, 2026 11:58

LuciferYang marked this pull request as ready for review May 19, 2026 12:11

LuciferYang marked this pull request as draft May 20, 2026 12:56

LuciferYang marked this pull request as ready for review May 20, 2026 12:56

claude Bot reviewed May 20, 2026

View reviewed changes

Xuanwo reviewed Jun 21, 2026

View reviewed changes

github-actions Bot added A-deps Dependency updates A-encoding Encoding, IO, file reader/writer labels Jun 22, 2026

LuciferYang added 4 commits July 14, 2026 19:47

chore: retrigger CI for flaky test_create_index_nulls::case_4_ivf_hns…

dc4a110

…w_pq

LuciferYang force-pushed the feat/session-reuse-singleflight branch from dd40497 to a62e577 Compare July 15, 2026 03:40

coderabbitai Bot reviewed Jul 15, 2026

View reviewed changes

Uh oh!

Conversation

LuciferYang commented May 19, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmark

Tests

Summary by CodeRabbit

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Claude Code Review

Uh oh!

codecov Bot commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Claude Code Review

Uh oh!

Xuanwo Jun 21, 2026

Choose a reason for hiding this comment

Uh oh!

LuciferYang Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

Xuanwo Jun 21, 2026

Choose a reason for hiding this comment

Uh oh!

LuciferYang Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

Xuanwo Jun 21, 2026

Choose a reason for hiding this comment

Uh oh!

LuciferYang Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

LuciferYang commented Jun 21, 2026

Uh oh!

LuciferYang commented Jun 22, 2026

Uh oh!

coderabbitai Bot commented Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

LuciferYang commented May 19, 2026 •

edited by coderabbitai Bot

Loading

codecov Bot commented May 19, 2026 •

edited

Loading

coderabbitai Bot commented Jul 15, 2026 •

edited

Loading