fix(goosefs): decouple OpenDAL root from URL path to avoid ObjectStore cache-key collision

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;
@@ -27,7 +26,34 @@ const DEFAULT_GOOSEFS_PORT: u16 = 9200;
 /// - `host:port` is the GooseFS Master address (default port: 9200)
 /// - `/path` is the filesystem path within GooseFS
 ///
-/// Configuration priority: storage_options > environment variables > URL authority > defaults
+/// 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.
+/// - 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`.
+///
+/// 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;
 
@@ -79,6 +105,13 @@ impl GooseFsStoreProvider {
             .or_else(|| std::env::var(env_key).ok())
             .filter(|v| !v.is_empty())
     }
+
+    /// 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())
+    }
 }
 
 #[async_trait::async_trait]
@@ -90,16 +123,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<String, String> = 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 +195,33 @@ impl ObjectStoreProvider for GooseFsStoreProvider {
         })
     }
 
-    /// Extract the path relative to the root of the GooseFS filesystem.
-    ///
-    /// 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://host:port/data/file.lance` → root="/data/file.lance", extract_path=""
-    fn extract_path(&self, _url: &Url) -> Result<Path> {
-        Ok(Path::from(""))
-    }
+    // `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 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<String, String>>,
+        storage_options: Option<&HashMap<String, String>>,
     ) -> Result<String> {
-        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 +230,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 +274,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<String, String> =
+            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 +363,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");
+    }
 }