fix(mem-wal): reject mismatched shard specs before epoch claim

[u70b3]

Summary

• reject incoming/stored shard_spec_id mismatches before advancing a shard
  manifest version or writer epoch
• revalidate shard identity after write conflicts while preserving the
  distinguishable sealed-shard error
• cover new and matching claims, manual-shard identity, side-effect-free
  rejection, and concurrent claims with different specs

flowchart TD
    A["Read latest manifest"]
    B{"Sealed?"}
    C{"Spec identity matches?"}
    D["Return sealed error"]
    E["Return InvalidInput<br/>manifest unchanged"]
    F["Write next version / epoch"]
    G{"Write conflict?"}
    A --> B
    B -- Yes --> D
    B -- No --> C
    C -- No --> E
    C -- Yes --> F
    F --> G
    G -- Yes --> A

Loading

shard_spec_id=0 remains the immutable identity for manually created shards;
it is not treated as a wildcard.

Fixes #7945.

Test Plan

• cargo fmt --all -- --check
• cargo test -p lance dataset::mem_wal::manifest::tests --lib -- --nocapture
• cargo clippy --all --tests --benches -- -D warnings

[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?