[u70b3]

Summary

• reject an incoming shard_spec_id that differs from the immutable identity already stored in a shard manifest, before advancing manifest version or writer epoch
• revalidate sealed status and shard identity after write conflicts, preserving the distinguishable sealed-shard error
• keep rejection side-effect free and include the shard id plus incoming and stored spec ids in the error
• cover matching automatic identity, both current 0/1 mismatch directions, sealed manifests, and concurrent first claims

Scope

This is an independent defensive invariant over the identities Lance currently emits: 0 identifies manually managed shards and 1 is the sole automatic sharding spec. The check does not require monotonic spec allocation and does not define revisions or activation.

#8112 (merged) is the complementary creation-side half. It resolves a dataset writer's default 0 to the current automatic spec at the Dataset::mem_wal_writer boundary, before a fresh manifest is created; this PR prevents any caller from changing an identity once the manifest exists. The layers compose: #8112 guards the API entry point, while this PR guards the stored manifest at claim_epoch — catching direct ShardWriter::open callers that bypass the API boundary.

shard_spec_id=0 is an identity, not a wildcard.

Fixes #7945.

Testing

Rebased onto 39412d741ca1542b68430078892508ee4b47e60b on 2026-11-01. The new claim tests use a non-empty MemWAL dir_id, and conflict injection derives its destination through shard_manifest_path.

• cargo fmt --all -- --check
• cargo clippy --profile ci --all --tests --benches -- -D warnings
• All 36 existing manifest module tests passed in an isolated Rust test executable importing this checkout's original manifest.rs and util.rs and linking the repository-built dependencies. This includes deterministic conflicting-spec/sealed-manifest races, benign retry, bounded exhaustion, fencing, and pruning coverage.

The monolithic cargo test -p lance ... --lib build was stopped at this environment's 8 GiB memory limit. The 36-test result above is module-level validation; see the successful fresh-head CI runs below for full-project validation.

Fresh-head CI passed: Rust, Python, and Java JNI.

Behavior change / rollout

Previously, a writer opening an existing shard with a mismatched shard_spec_id (e.g. the default 0 opening an auto-sharded shard stored as 1) was silently tolerated past claim_epoch. It now fails fast with InvalidInput naming the shard id and both spec ids — this is the fix intent of #7945. With #8112 merged, writers going through Dataset::mem_wal_writer have their default resolved before claiming, so the hard error only affects callers that bypass the API boundary with an explicitly mismatched id.

[coderabbitai]

📝 Walkthrough

Walkthrough

claim_epoch now centralizes sealed-manifest and shard-spec mismatch validation, applies it before and after write conflicts, and adds single-threaded and concurrent tests for matching and mismatched shard specifications.

Changes

Shard claim validation

Layer / File(s)	Summary
Centralized claim validation and conflict handling rust/lance/src/dataset/mem_wal/manifest.rs	validate_claimable rejects sealed manifests and shard-spec mismatches. claim_epoch applies it before epoch calculation and after failed writes.
Claim behavior tests rust/lance/src/dataset/mem_wal/manifest.rs	Tests verify matching claims, mismatched specifications, concurrent claim consistency, and sealed-manifest refusal.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• lance-format/lance#7896: Updates the same claim_epoch fencing and epoch-claim validation logic.

Suggested reviewers: hamersaw

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: rejecting mismatched shard specs before epoch claim.
Description check	✅ Passed	The description matches the code changes and issue scope.
Linked Issues check	✅ Passed	The changes satisfy #7945 by rejecting spec mismatches, revalidating after conflicts, and preserving successful claims.
Out of Scope Changes check	✅ Passed	No clear out-of-scope changes are indicated beyond the manifest validation fix and related tests.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Note

Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.

🟡 Other comments (1)

rust/lance/src/dataset/mem_wal/manifest.rs-445-447 (1)

445-447: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Document sealed-manifest refusal in # Errors.

claim_epoch now returns an error for sealed manifests, but this public API’s error contract only documents epoch conflicts, shard-spec conflicts, and contention. Document the sealed refusal and its no-claim effect. As per coding guidelines, “Ensure doc comments match actual semantics; distinguish mutates-in-place (&mut self) from returns-new-value behavior.”

🤖 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/src/dataset/mem_wal/manifest.rs` around lines 445 - 447, Update
the public `claim_epoch` documentation comment’s `# Errors` section to include
refusal for sealed manifests and state that this path makes no claim or
mutation. Keep the existing documentation for epoch conflicts, shard-spec
conflicts, and contention, and accurately describe the method’s in-place `&mut
self` 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.

Other comments:
In `@rust/lance/src/dataset/mem_wal/manifest.rs`:
- Around line 445-447: Update the public `claim_epoch` documentation comment’s
`# Errors` section to include refusal for sealed manifests and state that this
path makes no claim or mutation. Keep the existing documentation for epoch
conflicts, shard-spec conflicts, and contention, and accurately describe the
method’s in-place `&mut self` behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: QUIET

Plan: Pro Plus

Run ID: eddae87c-eea7-4d35-949b-1203ad196d84

📥 Commits

Reviewing files that changed from the base of the PR and between 3a72f8a and 5cd2b6a.

📒 Files selected for processing (1)

• rust/lance/src/dataset/mem_wal/manifest.rs

[codecov]

Codecov Report

❌ Patch coverage is 94.54545% with 3 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
rust/lance/src/dataset/mem_wal/manifest.rs	94.54%	1 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

[hamersaw]

I think this change has the right intent, but will not fix underlying issues correctly. It's meant to disallow claiming a table with a different shard_spec_id than the base table. However, Lance currently only uses 0 and 1 as valid values to not manual and automatic sharding configuration. IMO this is part of a larger discussion, we should update the shard spec configuration so that IDs are monotonically increasing. That would make this change valid.

[u70b3]

Thanks, I think I understand the concern. The manifest-side equality check is only meaningful if every distinct sharding-spec revision has a unique ID. Today the public initialization path assigns 1 to every automatic sharding configuration, so two different automatic specs remain indistinguishable (0 remains the manual-shard sentinel).

It sounds like the prerequisites are:

1. Allocate sharding spec IDs monotonically and never reuse them.
2. Ensure a writer claims with the active spec ID from the base-table metadata.
3. Keep the claim-time validation so a stale or mismatched spec cannot advance the epoch and fence the incumbent writer.

Would you prefer that I expand this PR to implement the spec-ID lifecycle, or split the monotonic allocation/base-table validation into a prerequisite PR and rebase this change on it?

[u70b3]

Following up on my previous comment: I split the prerequisite work into #8112.

#8112 introduces monotonic spec-ID allocation (max(existing) + 1), reserves spec ID 0 for manual sharding, validates reserved and duplicate IDs at MemWAL metadata boundaries, and resolves/validates the writer's spec against the active table spec before any shard claim. Manual sharding is restricted to spec ID 0.

The current initializer only creates the first spec, so an empty history produces spec ID 1. A future spec-revision API will provide the complete append-only history.

Once #8112 lands, I'll rebase #7949 on top of it. #7949 will remain focused on claim-time manifest validation.

[u70b3]

Scope and dependency reset completed and force-pushed on latest main.

I narrowed the claim to the identities current Lance code actually emits: manual 0 and sole automatic 1. The manifest guard is independently useful under that invariant: a manual caller cannot claim/fence an automatic shard, an automatic caller cannot claim/fence a manual shard, and a concurrent first claim cannot be followed by the opposite identity. Rejection remains side-effect free and is revalidated after write conflicts.

This PR no longer claims that monotonic spec allocation is a prerequisite or attempts to solve future sharding revisions/activation. #8112 was separately reduced to resolving the writer's default 0 before a fresh automatic manifest is created. Either PR is independently valid; together they cover creation-time resolution and stored-manifest immutability.

The issue reproducer and tests now use only 0/1; the public error docs also include sealed/no-mutation behavior. Fourteen manifest tests and full workspace clippy pass after the latest-main rebase.

The existing CHANGES_REQUESTED review predates this scope decision. Please re-review the independent defensive invariant.

[u70b3]

@hamersaw PTAL when you have a chance — the open CHANGES_REQUESTED predates the Aug 3 scope reset; the branch is current with main and all checks pass.

[Xuanwo]

@hamersaw Following the scope reset, this change now only guards the current manual 0 / automatic 1 identities. #8112 has merged and resolves the writer identity at the dataset entry point, so this remains the complementary check at claim_epoch. Could the earlier monotonic-allocation request be revisited for this bounded claim guard? For a fresh head, we'll retain the current dir_id and fencing behavior and complete current-head CI before requesting approval; this is not a request to approve the current head.

[lance-gatekeeper]

✅ Gate recommendation: approve.

The rebased guard still rejects mismatched stored identities before epoch mutation and after write conflicts, preserving sealed-shard precedence and the current fencing protocol. The claim tests now use a non-empty dir_id and derive the injected manifest location through the production path helper, matching the current storage layout. Merged #8112 continues to resolve dataset-writer identity before claiming; this complementary guard protects direct or stale callers. The bounded manual 0 / automatic 1 invariant does not require a spec-revision or monotonic-allocation lifecycle.