From 0c59f54d1bb6f61f0b3840fd3b51af6bebdb47b8 Mon Sep 17 00:00:00 2001 From: forwardxu Date: Fri, 3 Jul 2026 17:24:56 +0800 Subject: [PATCH 1/3] fix(goosefs): decouple OpenDAL root from URL path to avoid ObjectStore cache key collision Previously GooseFsStoreProvider baked the URL path into the OpenDAL Operator root and also into the ObjectStoreRegistry cache prefix. That made two datasets under the same master (e.g. goosefs://host:9200/a.lance and goosefs://host:9200/b.lance) collide in the registry cache: the second open reused the first Operator whose root still pointed at the first dataset, so queries on B silently returned A's data. Rework the provider to follow the S3-style model already used by tos/oss: - Operator root is cluster-wide (default "/", overridable via goosefs_root storage option or GOOSEFS_ROOT env var), never per-URL. - extract_path returns the URL path (percent-decoded) as the per-request object key, so a single shared Operator can route requests to the correct dataset. - calculate_object_store_prefix keys the registry on scheme+authority (plus the custom root when non-default), so datasets under the same master intentionally share one Operator without cross-contamination. Add regression tests covering: - extract_path for basic, root, deep and percent-encoded URLs; - prefix sharing across datasets on the same master; - prefix isolation across masters; - prefix inclusion of a custom goosefs_root; - master-address resolution from URL, default port and storage options; - root resolution defaults and overrides. --- .../src/object_store/providers/goosefs.rs | 176 +++++++++++++++--- 1 file changed, 147 insertions(+), 29 deletions(-) diff --git a/rust/lance-io/src/object_store/providers/goosefs.rs b/rust/lance-io/src/object_store/providers/goosefs.rs index d6173571551..7139ec14521 100644 --- a/rust/lance-io/src/object_store/providers/goosefs.rs +++ b/rust/lance-io/src/object_store/providers/goosefs.rs @@ -27,6 +27,16 @@ const DEFAULT_GOOSEFS_PORT: u16 = 9200; /// - `host:port` is the GooseFS Master address (default port: 9200) /// - `/path` is the filesystem path within GooseFS /// +/// Path handling model (S3-style): +/// - The OpenDAL `root` is fixed to `/` (or a user-supplied cluster-wide base) +/// so that a single [`Operator`] can serve every dataset under the same +/// master. This keeps the `ObjectStoreRegistry` cache correct: two URLs +/// like `goosefs://host:9200/a.lance` and `goosefs://host:9200/b.lance` +/// share one store and each request carries its own object key. +/// - [`Self::extract_path`] returns the URL path (percent-decoded) as the key +/// passed to `ObjectStore::get`, `put`, etc. — mirroring how `s3://bucket/k` +/// yields key `k`. +/// /// Configuration priority: storage_options > environment variables > URL authority > defaults #[derive(Default, Debug)] pub struct GooseFsStoreProvider; @@ -79,6 +89,20 @@ impl GooseFsStoreProvider { .or_else(|| std::env::var(env_key).ok()) .filter(|v| !v.is_empty()) } + + /// Resolve the OpenDAL `root` for this Operator. + /// + /// The root is intentionally cluster-wide (not per-URL) so that many + /// datasets under the same master can share a single cached Operator. + /// + /// Priority: + /// 1. `storage_options["goosefs_root"]` + /// 2. `GOOSEFS_ROOT` environment variable + /// 3. `"/"` (default: expose the whole filesystem) + fn resolve_root(storage_options: &StorageOptions) -> String { + Self::resolve_option(storage_options, "goosefs_root", "GOOSEFS_ROOT") + .unwrap_or_else(|| "/".to_string()) + } } #[async_trait::async_trait] @@ -90,16 +114,15 @@ impl ObjectStoreProvider for GooseFsStoreProvider { // Resolve master address let master_addr = Self::resolve_master_addr(&base_path, &storage_options)?; - // Extract root path from URL - let root = base_path.path().to_string(); + // Resolve a stable cluster-wide root. The URL path is *not* used here + // because it varies per dataset; per-request keys are supplied by + // `extract_path` instead. + let root = Self::resolve_root(&storage_options); // Build OpenDAL config map let mut config_map: HashMap = HashMap::new(); config_map.insert("master_addr".to_string(), master_addr); - - if !root.is_empty() && root != "/" { - config_map.insert("root".to_string(), root); - } + config_map.insert("root".to_string(), root); // Optional: write_type if let Some(wt) = @@ -163,26 +186,45 @@ impl ObjectStoreProvider for GooseFsStoreProvider { }) } - /// Extract the path relative to the root of the GooseFS filesystem. + /// Extract the object key relative to the OpenDAL root. /// - /// For GooseFS, the entire URL path is set as the OpenDAL `root` in `new_store`, - /// so the relative path returned here must be empty to avoid path duplication. + /// GooseFS now behaves like S3: the URL path is the per-request key that + /// gets appended to a cluster-wide root by OpenDAL's + /// `build_rooted_abs_path`. The returned [`Path`] is the percent-decoded + /// URL path with the leading `/` stripped. /// - /// `goosefs://host:port/data/file.lance` → root="/data/file.lance", extract_path="" - fn extract_path(&self, _url: &Url) -> Result { - Ok(Path::from("")) + /// `goosefs://host:port/data/file.lance` → key `data/file.lance` + /// `goosefs://host:port/` → key `""` + fn extract_path(&self, url: &Url) -> Result { + // `Path::from_url_path` percent-decodes the input and normalizes + // separators, matching how `S3StoreProvider` (default trait impl) + // treats bucket-relative keys. + Path::from_url_path(url.path()).map_err(|e| { + Error::invalid_input(format!("Invalid path in URL '{}': {}", url.path(), e)) + }) } - /// Calculate the object store prefix for caching. + /// Calculate the object store prefix used as the registry cache key. /// - /// Format: `goosefs$host:port` - /// This ensures different GooseFS clusters get separate caches. + /// Format: `goosefs$host:port`. Because the OpenDAL root is now cluster- + /// wide (not per-URL), all datasets under the same master intentionally + /// share the same cached [`ObjectStore`]; the URL path is disambiguated + /// by [`Self::extract_path`] on each request. This is analogous to how + /// two `s3://bucket/a` and `s3://bucket/b` URLs share one store. fn calculate_object_store_prefix( &self, url: &Url, - _storage_options: Option<&HashMap>, + storage_options: Option<&HashMap>, ) -> Result { - Ok(format!("{}${}", url.scheme(), url.authority())) + // If a custom `goosefs_root` is provided, include it in the prefix so + // that stores built with different roots don't accidentally collide. + let opts = StorageOptions(storage_options.cloned().unwrap_or_default()); + let root = Self::resolve_root(&opts); + if root == "/" { + Ok(format!("{}${}", url.scheme(), url.authority())) + } else { + Ok(format!("{}${}#{}", url.scheme(), url.authority(), root)) + } } } @@ -191,38 +233,42 @@ mod tests { use super::*; #[test] - fn test_goosefs_store_path() { + fn test_goosefs_extract_path_basic() { let provider = GooseFsStoreProvider; - let url = Url::parse("goosefs://10.0.0.1:9200/data/embeddings.lance").unwrap(); let path = provider.extract_path(&url).unwrap(); - // extract_path returns empty because the full path is used as OpenDAL root - assert_eq!(path.to_string(), ""); + assert_eq!(path.to_string(), "data/embeddings.lance"); } #[test] - fn test_goosefs_store_root_path() { + fn test_goosefs_extract_path_root() { let provider = GooseFsStoreProvider; - let url = Url::parse("goosefs://10.0.0.1:9200/").unwrap(); let path = provider.extract_path(&url).unwrap(); assert_eq!(path.to_string(), ""); } #[test] - fn test_goosefs_store_deep_path() { + fn test_goosefs_extract_path_deep() { let provider = GooseFsStoreProvider; - let url = Url::parse("goosefs://master:9200/a/b/c/d.lance").unwrap(); let path = provider.extract_path(&url).unwrap(); - // All path components are in the OpenDAL root, extract_path is empty - assert_eq!(path.to_string(), ""); + assert_eq!(path.to_string(), "a/b/c/d.lance"); } #[test] - fn test_calculate_object_store_prefix() { + fn test_goosefs_extract_path_percent_decoded() { + // The URL contains a percent-encoded space; extract_path must decode + // it once so the ObjectStore layer does not double-encode later. let provider = GooseFsStoreProvider; + let url = Url::parse("goosefs://master:9200/dir/with%20space/f.lance").unwrap(); + let path = provider.extract_path(&url).unwrap(); + assert_eq!(path.to_string(), "dir/with space/f.lance"); + } + #[test] + fn test_calculate_object_store_prefix_default_root() { + let provider = GooseFsStoreProvider; let url = Url::parse("goosefs://10.0.0.1:9200/data").unwrap(); let prefix = provider.calculate_object_store_prefix(&url, None).unwrap(); assert_eq!(prefix, "goosefs$10.0.0.1:9200"); @@ -231,12 +277,69 @@ mod tests { #[test] fn test_calculate_object_store_prefix_with_hostname() { let provider = GooseFsStoreProvider; - let url = Url::parse("goosefs://myhost:9200/data").unwrap(); let prefix = provider.calculate_object_store_prefix(&url, None).unwrap(); assert_eq!(prefix, "goosefs$myhost:9200"); } + /// Regression test: two URLs pointing at different datasets under the + /// same master must produce the *same* cache prefix so they share one + /// Operator, and correctness must come from `extract_path` returning + /// distinct keys — never from a per-URL root baked into the prefix. + #[test] + fn test_prefix_shared_across_datasets_same_master() { + let provider = GooseFsStoreProvider; + let url_a = Url::parse("goosefs://10.0.0.1:9200/repro/a.lance").unwrap(); + let url_b = Url::parse("goosefs://10.0.0.1:9200/repro/b.lance").unwrap(); + + let pa = provider + .calculate_object_store_prefix(&url_a, None) + .unwrap(); + let pb = provider + .calculate_object_store_prefix(&url_b, None) + .unwrap(); + assert_eq!(pa, pb, "same master must share one cache prefix"); + + // Extracted keys must differ so the shared Operator can route + // requests to the correct dataset. + assert_ne!( + provider.extract_path(&url_a).unwrap(), + provider.extract_path(&url_b).unwrap(), + "distinct URLs must yield distinct object keys", + ); + } + + /// Different masters must never share a cache entry. + #[test] + fn test_prefix_isolated_across_masters() { + let provider = GooseFsStoreProvider; + let u1 = Url::parse("goosefs://host-a:9200/x.lance").unwrap(); + let u2 = Url::parse("goosefs://host-b:9200/x.lance").unwrap(); + assert_ne!( + provider.calculate_object_store_prefix(&u1, None).unwrap(), + provider.calculate_object_store_prefix(&u2, None).unwrap(), + ); + } + + /// A user-supplied `goosefs_root` participates in the cache prefix so + /// stores rooted at different subtrees don't collide. + #[test] + fn test_prefix_includes_custom_root() { + let provider = GooseFsStoreProvider; + let url = Url::parse("goosefs://host:9200/x.lance").unwrap(); + + let default_prefix = provider.calculate_object_store_prefix(&url, None).unwrap(); + let custom_opts: HashMap = + HashMap::from([("goosefs_root".to_string(), "/tenant-a".to_string())]); + let custom_prefix = provider + .calculate_object_store_prefix(&url, Some(&custom_opts)) + .unwrap(); + + assert_eq!(default_prefix, "goosefs$host:9200"); + assert_eq!(custom_prefix, "goosefs$host:9200#/tenant-a"); + assert_ne!(default_prefix, custom_prefix); + } + #[test] fn test_resolve_master_addr_from_url() { let url = Url::parse("goosefs://10.0.0.1:9200/data").unwrap(); @@ -263,4 +366,19 @@ mod tests { let addr = GooseFsStoreProvider::resolve_master_addr(&url, &storage_options).unwrap(); assert_eq!(addr, "10.0.0.2:9200,10.0.0.3:9200"); } + + #[test] + fn test_resolve_root_defaults_to_slash() { + let opts = StorageOptions(HashMap::new()); + assert_eq!(GooseFsStoreProvider::resolve_root(&opts), "/"); + } + + #[test] + fn test_resolve_root_from_storage_options() { + let opts = StorageOptions(HashMap::from([( + "goosefs_root".to_string(), + "/tenant-a".to_string(), + )])); + assert_eq!(GooseFsStoreProvider::resolve_root(&opts), "/tenant-a"); + } } From 438faa17f04d8df7f6657beb98891d3b4a8ed71a Mon Sep 17 00:00:00 2001 From: forwardxu Date: Mon, 6 Jul 2026 11:37:26 +0800 Subject: [PATCH 2/3] refactor(goosefs): drop duplicated extract_path and consolidate config docs Address review feedback: - Remove GooseFsStoreProvider::extract_path override; the trait default in ObjectStoreProvider is byte-for-byte identical (Path::from_url_path with the same error mapping), so overriding it is pure duplication. Behavior is unchanged and the existing extract_path unit tests still pass by resolving to the trait default. - Move all supported config keys (goosefs_master_addr / goosefs_root / goosefs_write_type / goosefs_block_size / goosefs_chunk_size / goosefs_auth_type / goosefs_auth_username) into a single table at the top of the file, alongside the existing path-handling model. This puts the goosefs_root semantics next to the rest of the configuration surface instead of hiding them on a private helper. The per-function doc on resolve_root is trimmed to a pointer at the file-level docs. - Drop the now-unused object_store::path::Path import. --- .../src/object_store/providers/goosefs.rs | 55 +++++++++---------- 1 file changed, 26 insertions(+), 29 deletions(-) diff --git a/rust/lance-io/src/object_store/providers/goosefs.rs b/rust/lance-io/src/object_store/providers/goosefs.rs index 7139ec14521..9db54abee7e 100644 --- a/rust/lance-io/src/object_store/providers/goosefs.rs +++ b/rust/lance-io/src/object_store/providers/goosefs.rs @@ -4,7 +4,6 @@ use std::collections::HashMap; use std::sync::Arc; -use object_store::path::Path; use object_store_opendal::OpendalStore; use opendal::{Operator, services::GooseFs}; use url::Url; @@ -33,11 +32,28 @@ const DEFAULT_GOOSEFS_PORT: u16 = 9200; /// master. This keeps the `ObjectStoreRegistry` cache correct: two URLs /// like `goosefs://host:9200/a.lance` and `goosefs://host:9200/b.lance` /// share one store and each request carries its own object key. -/// - [`Self::extract_path`] returns the URL path (percent-decoded) as the key +/// - Path extraction relies on the default [`ObjectStoreProvider::extract_path`] +/// implementation, which returns the URL path (percent-decoded) as the key /// passed to `ObjectStore::get`, `put`, etc. — mirroring how `s3://bucket/k` /// yields key `k`. /// -/// Configuration priority: storage_options > environment variables > URL authority > defaults +/// Supported configuration keys (via `storage_options` or environment variables, +/// resolved with priority: `storage_options` > env var > URL authority > default): +/// +/// | storage_options key | env var | purpose | +/// |---------------------------|-------------------------|-----------------------------------------------------------------------------------------------| +/// | `goosefs_master_addr` | `GOOSEFS_MASTER_ADDR` | Master gRPC address, e.g. `host:9200`. Supports HA: `addr1:port,addr2:port`. | +/// | `goosefs_root` | `GOOSEFS_ROOT` | Cluster-wide OpenDAL root shared by all datasets under the same master. Defaults to `/`. | +/// | `goosefs_write_type` | `GOOSEFS_WRITE_TYPE` | GooseFS write type (e.g. `MUST_CACHE`, `CACHE_THROUGH`, `THROUGH`, `ASYNC_THROUGH`). | +/// | `goosefs_block_size` | `GOOSEFS_BLOCK_SIZE` | GooseFS block size (bytes). Distinct from Lance's own `block_size`. | +/// | `goosefs_chunk_size` | `GOOSEFS_CHUNK_SIZE` | GooseFS chunk size (bytes) used by the client. | +/// | `goosefs_auth_type` | `GOOSEFS_AUTH_TYPE` | Authentication mode: `nosasl` or `simple`. | +/// | `goosefs_auth_username` | `GOOSEFS_AUTH_USERNAME` | Username for `simple` auth mode. | +/// +/// Note on `goosefs_root`: it is deliberately cluster-wide (not per-URL) so +/// that many datasets under the same master share a single cached [`Operator`]. +/// A custom root also participates in the [`ObjectStoreRegistry`] cache prefix, +/// so stores rooted at different subtrees do not collide. #[derive(Default, Debug)] pub struct GooseFsStoreProvider; @@ -90,15 +106,8 @@ impl GooseFsStoreProvider { .filter(|v| !v.is_empty()) } - /// Resolve the OpenDAL `root` for this Operator. - /// - /// The root is intentionally cluster-wide (not per-URL) so that many - /// datasets under the same master can share a single cached Operator. - /// - /// Priority: - /// 1. `storage_options["goosefs_root"]` - /// 2. `GOOSEFS_ROOT` environment variable - /// 3. `"/"` (default: expose the whole filesystem) + /// Resolve the OpenDAL `root` for this Operator. See the file-level docs on + /// [`GooseFsStoreProvider`] for the semantics of `goosefs_root`. fn resolve_root(storage_options: &StorageOptions) -> String { Self::resolve_option(storage_options, "goosefs_root", "GOOSEFS_ROOT") .unwrap_or_else(|| "/".to_string()) @@ -186,23 +195,11 @@ impl ObjectStoreProvider for GooseFsStoreProvider { }) } - /// Extract the object key relative to the OpenDAL root. - /// - /// GooseFS now behaves like S3: the URL path is the per-request key that - /// gets appended to a cluster-wide root by OpenDAL's - /// `build_rooted_abs_path`. The returned [`Path`] is the percent-decoded - /// URL path with the leading `/` stripped. - /// - /// `goosefs://host:port/data/file.lance` → key `data/file.lance` - /// `goosefs://host:port/` → key `""` - fn extract_path(&self, url: &Url) -> Result { - // `Path::from_url_path` percent-decodes the input and normalizes - // separators, matching how `S3StoreProvider` (default trait impl) - // treats bucket-relative keys. - Path::from_url_path(url.path()).map_err(|e| { - Error::invalid_input(format!("Invalid path in URL '{}': {}", url.path(), e)) - }) - } + // `extract_path` uses the default `ObjectStoreProvider` trait implementation: + // it percent-decodes the URL path and returns it as the object key, exactly + // like S3 does for `s3://bucket/key`. Overriding it here would only + // duplicate that behavior. See the file-level doc comment above for the + // full path-handling model. /// Calculate the object store prefix used as the registry cache key. /// From e3d1f3652114291d4779ef47b51d43effbb21fd0 Mon Sep 17 00:00:00 2001 From: forwardxu Date: Mon, 6 Jul 2026 11:43:21 +0800 Subject: [PATCH 3/3] docs(goosefs): fix rustdoc broken intra-doc links CI runs rustdoc with -D warnings, which upgraded a broken intra-doc link into a hard error: error: unresolved link to `ObjectStoreRegistry` --> rust/lance-io/src/object_store/providers/goosefs.rs:55:46 `ObjectStoreRegistry` lives in the top-level `lance` crate and is not in scope from `lance-io`, so rustdoc cannot resolve the [`...`] link syntax. `Operator` is re-exported from `opendal` and is likewise not a local item, so the same syntax would trip the next rustdoc run. Downgrade both references from intra-doc links to plain code spans (backticks only). They are still rendered as inline code but are no longer resolved as links, which matches how the surrounding docs already refer to `ObjectStoreRegistry`. The other bracketed links in this file (`Operator`-free ones like `ObjectStore`, `ObjectStoreProvider::extract_path`, `GooseFsStoreProvider`, `Self::extract_path`) all resolve to items `use`d in this module and remain untouched. Verified locally with: RUSTDOCFLAGS="-D warnings" cargo doc -p lance-io --features goosefs --no-deps --- rust/lance-io/src/object_store/providers/goosefs.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rust/lance-io/src/object_store/providers/goosefs.rs b/rust/lance-io/src/object_store/providers/goosefs.rs index 9db54abee7e..5d2a648522b 100644 --- a/rust/lance-io/src/object_store/providers/goosefs.rs +++ b/rust/lance-io/src/object_store/providers/goosefs.rs @@ -28,7 +28,7 @@ const DEFAULT_GOOSEFS_PORT: u16 = 9200; /// /// Path handling model (S3-style): /// - The OpenDAL `root` is fixed to `/` (or a user-supplied cluster-wide base) -/// so that a single [`Operator`] can serve every dataset under the same +/// so that a single `Operator` can serve every dataset under the same /// master. This keeps the `ObjectStoreRegistry` cache correct: two URLs /// like `goosefs://host:9200/a.lance` and `goosefs://host:9200/b.lance` /// share one store and each request carries its own object key. @@ -51,8 +51,8 @@ const DEFAULT_GOOSEFS_PORT: u16 = 9200; /// | `goosefs_auth_username` | `GOOSEFS_AUTH_USERNAME` | Username for `simple` auth mode. | /// /// Note on `goosefs_root`: it is deliberately cluster-wide (not per-URL) so -/// that many datasets under the same master share a single cached [`Operator`]. -/// A custom root also participates in the [`ObjectStoreRegistry`] cache prefix, +/// that many datasets under the same master share a single cached `Operator`. +/// A custom root also participates in the `ObjectStoreRegistry` cache prefix, /// so stores rooted at different subtrees do not collide. #[derive(Default, Debug)] pub struct GooseFsStoreProvider;