[farazshaikh]

Summary

Follow-up to #8522, stacked on #8773. Lets a registered ObjectStoreProvider declare which commit handler its store should use, instead of the handler being inferred purely from the URL scheme string.

Changes

• CommitHandlerType { ConditionalPut, Unsafe } (new, in lance-io) — a small enum a provider returns to declare the concurrency guarantee its store actually offers. ConditionalPut is the Default.
• ObjectStoreProvider::commit_handler() (new trait method, default ConditionalPut) — a provider whose store cannot offer atomic create-if-not-exists overrides this to return Unsafe.
• commit_handler_from_url now takes an optional &ObjectStoreRegistry. For a scheme the built-in resolver does not recognize, it consults the registered provider's commit_handler() rather than guessing; with no registered provider it keeps the conflict-safe put-if-not-exists default.
• The write path threads the session's registry through resolve_commit_handler.

Why

#8773 made the default for unrecognized schemes conflict-safe. This adds the escape hatch Weston asked for: a provider that genuinely can't do put-if-not-exists can opt into UnsafeCommitHandler explicitly, so selection is driven by the provider's real capability rather than a hardcoded scheme list. Per the review discussion, providers return a small fixed set (ConditionalPut / Unsafe); users are not expected to supply arbitrary commit handlers.

Test

test_registered_provider_declares_commit_handler: a provider declaring Unsafe routes to UnsafeCommitHandler, and an unregistered custom scheme still defaults to ConditionalPutCommitHandler.

Verified locally: cargo test -p lance-table, cargo clippy -p lance-io -p lance-table -p lance -p lance-namespace-impls -- -D warnings clean.

[farazshaikh]

Reworked so the commit-handler capability is carried on the constructed store rather than re-queried from the registry.

1. ObjectStore now holds a commit_handler_type: CommitHandlerType, stamped in ObjectStoreRegistry::build_store from provider.commit_handler(). Every store the registry builds or caches carries the capability its provider declared.

2. Commit-handler resolution derives the default from that captured value instead of a registry lookup. Registering an Unsafe-declaring provider for a known scheme such as memory is now honored, because the choice follows the store, not the scheme name. This also closes the mutable-registry race, since a cached store keeps the capability it was built with. Covered by test_provider_commit_handler_travels_with_store (known-scheme override plus cached store) and test_commit_handler_from_url_honors_store_capability.

3. The builder and manifest-namespace paths now pass the capability of the store they construct instead of no registry, so their defaults consult the provider too.

Explicit caller-supplied handlers keep precedence, and file/cos/s3+ddb keep their dedicated scheme handlers, since they need scheme-specific parameters and do not reduce to the two-variant enum.

I do not have permission to add the breaking-change label on this repo. Could a maintainer apply it?

[lance-gatekeeper]

❌ Gate recommendation: request changes.

The store-coupling revision fixes the general registry and cache paths, but handler selection still crosses the authority boundary in two cases: dedicated-scheme overrides bypass the provider declaration, and manifest rewrites derive capability from a different store than the one they commit through. Resolve handlers from the exact committing store in both paths.

Please mark this PR with the breaking-change label.

[lance-gatekeeper]

The wildcard-only dispatch still ignores the constructed store’s declaration for registered overrides of file/file-object-store and cos. ObjectStoreRegistry::insert intentionally permits overriding an existing scheme, so an Unsafe provider under file:// is paired with ConditionalPutCommitHandler on Unix (RenameCommitHandler on Windows). Valid writes can fail, or a backend that cannot enforce create-if-absent can retain the manifest-overwrite risk this capability is meant to declare.

Keep dedicated built-in behavior in the provider/store capability (or an equivalent per-store handler descriptor) and dispatch from the exact store, not the URI string. This is the remaining projection of the earlier authority finding.

Reproducer

At 9e7ae4997, I added and ran:

#[tokio::test]
async fn registered_file_provider_honors_store_capability() {
    let handler = commit_handler_from_url(
        "file:///tmp/ds",
        &None,
        CommitHandlerType::Unsafe,
    )
    .await
    .unwrap();
    assert_eq!(format!("{:?}", handler), "UnsafeCommitHandler");
}

CARGO_TARGET_DIR=/home/agent/tmp/gate8774-target cargo test -p lance-table registered_file_provider_honors_store_capability --lib failed with left ConditionalPutCommitHandler, right UnsafeCommitHandler.

[lance-gatekeeper]

This derives the handler from self.object_store, but commit_manifest_overwrite deliberately obtains dataset.object_store(None) and commits through that different store. The public constructors accept the namespace store and session independently, so capability A can select a handler that operates on store B. In the reproduced state, the committing store declares ConditionalPut while this method returns UnsafeCommitHandler; concurrent manifest rewrites can therefore blind-overwrite.

Resolve the handler from the dataset’s exact object store after obtaining it, or carry that store and handler together through the commit.

Reproducer

I used an UnsafeMemoryProvider that delegates store construction and prefix calculation to MemoryStoreProvider but returns CommitHandlerType::Unsafe, then ran this test at 9e7ae4997:

#[tokio::test]
async fn manifest_handler_follows_store_used_for_commit() {
    let root = "memory://gate-repro/namespace";
    let namespace_registry = Arc::new(ObjectStoreRegistry::default());
    namespace_registry.insert("memory", Arc::new(UnsafeMemoryProvider));
    let (object_store, base_path) = ObjectStore::from_uri_and_params(
        namespace_registry,
        root,
        &ObjectStoreParams::default(),
    )
    .await
    .unwrap();

    let dataset_registry = Arc::new(ObjectStoreRegistry::default());
    let session = Arc::new(Session::new(0, 0, dataset_registry));
    let namespace = ManifestNamespace::from_directory(
        root.to_string(), None, Some(session), object_store, base_path,
        true, false, None,
    )
    .await
    .unwrap();

    let dataset = namespace.manifest_dataset.get().await.unwrap();
    let committing_store = dataset.object_store(None).await.unwrap();
    assert_eq!(
        committing_store.commit_handler_type(),
        CommitHandlerType::ConditionalPut,
    );
    let handler = namespace.manifest_commit_handler().await.unwrap();
    assert_eq!(format!("{:?}", handler), "ConditionalPutCommitHandler");
}

CARGO_TARGET_DIR=/home/agent/tmp/gate8774-target cargo test -p lance-namespace-impls manifest_handler_follows_store_used_for_commit --lib failed with left UnsafeCommitHandler, right ConditionalPutCommitHandler.

[lance-gatekeeper]

Fixed in 27e14268e: the handler is now derived inside the retry loop from the same dataset object store used for staging and commit.

[farazshaikh]

Rebased onto v12.0.0-beta.4 (the branch was conflicting) and addressed the two points.

1. Manifest rewrites: fixed. manifest_commit_handler now resolves from the dataset's own object store, the exact store the rewrite commits through, instead of self.object_store. So a registered override for that scheme governs the commit. The store is fetched inside the retry loop, and the handler is now derived from that same instance.

2. Dedicated-scheme handling (file, cos, s3+ddb): kept as scheme arms deliberately. Their handlers (rename, TencentCosCommitHandler, and the s3+ddb external-manifest handler built from the ddbTableName query) cannot be represented by the two-variant CommitHandlerType, and lance-io cannot reference lance-table's handler types since it is the lower layer. Per the earlier maintainer guidance the capability enum is intentionally limited to the two general object-store choices; honoring an override of these built-in schemes needs the deferred store-owned handler-selection refactor, and Tachyon (the driver here) is read-only. I would rather not expand the enum or invert the crate dependency in this PR, but I am happy to take whichever direction a maintainer prefers.

I still do not have permission to add the breaking-change label. Could a maintainer apply it?

[lance-gatekeeper]

❌ Gate recommendation: request changes.

1 fixed / 1 remains. The author follow-up correctly moves manifest-handler selection to the committing store. Dedicated-scheme overrides still bypass the provider declaration, however, despite the registry’s documented built-in override contract. That can pair a replacement store with commit semantics it cannot support.

A viable revision can distinguish specialized built-in providers from replacements in the store-stamped strategy, or reject those overrides before writes; neither requires reversing the crate dependency.

Please mark this PR with the breaking-change label.