persistence follow-up gates: publication vs observed head, enforceable fences - #913
Conversation
…e fences The five semantic gaps the post-#912 review surfaced, each closed with a falsifier rather than a doc comment. 1. A reconciled retry no longer claims a publication version. #912 named `Reconciled.current_head` carefully and then `seal_cycle` adopted it as `SealedCycle.version` anyway, so retrying cycle 1 at head V5 recorded cycle 1 as sealed into V5. `publication_version` is now Some ONLY for a fresh Committed; `observed_head` is Some only when a sink observed one (None on NoChange, which calls no sink). 2. `FleetRecovery::checkpoint_bound` turns the latecomer rule from an instruction in a doc comment into an API that caps the durable bound strictly below any foreign landing. 3. An ABI-malformed artifact is `CommitError::InvalidArtifact` — permanent. Reporting it as retryable `Io` meant "fail, regenerate identically, fail" without bound. 4. Store identity is lexical, so `x/./s.lance` cannot claim a second slot beside `x/s.lance`, and the registry claim is an RAII value taken before the first `.await` — a cancelled open can no longer leak a reservation. 5. Startup seeding streams frame rows to a max instead of materialising the whole timeline; the schema guard checks types and nullability, not just column names. The append/reopen/reconcile branch — which carries the whole no-rollback contract and cannot be made to fail on demand through real Lance — now has a `#[cfg(test)]` fault-injection seam and three falsifiers covering its unpublished, published-but-unacknowledged, and reconcile-unavailable arms. `scan_sealed` is payload-free by contract: stated on the trait and modelled by all seven fakes, which previously cloned payloads and so proved a property the real writer does not hold. Tests: cycle_sink 19, cycle_driver 26, persist_sink 22, supervisor integration probes green; fmt clean; clippy adds no new warnings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KCGhDYoQBXs3poaR7sFuqp
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_b590e10e-a739-454f-9bb7-100cfcf4fa9c) |
|
Warning Review limit reached
Next review available in: 36 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis change hardens cycle persistence. It separates publication and observed-head metadata, adds bounded recovery, makes sealed scans payload-free, rejects malformed artifacts permanently, enforces writer ownership, validates schemas, and reconciles append failures. ChangesPersistence hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d4167043f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let mut s = String::from(prefix); | ||
| if absolute { | ||
| s.push('/'); | ||
| } | ||
| s.push_str(&parts.join("/")); |
There was a problem hiding this comment.
On Windows, passing a supported UNC dataset path such as //server/share/cycles.lance sets absolute but reconstructs the path with only one leading slash, producing /server/share/cycles.lance. The writer consequently opens or creates a different local path—or fails—instead of the network-share dataset requested by the caller. Preserve the UNC prefix during lexical normalization, or keep the original path for Lance I/O while using a separately normalized registry key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
crates/lance-graph/src/graph/cycle_sink.rs (2)
1604-1641: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test can pass without exercising the claim-release path.
Lines 1627-1633 return early when the bogus open unexpectedly succeeds. On a platform where object-store reports
NotFoundfor that path, the test asserts only the two success-and-drop halves. The RAII release on anopenERROR path is then never checked, and the comment says so.The release-on-error invariant is the point of the
WriterClaimchange. Drive it with a deterministic error instead. A schema mismatch is a guaranteed post-claim failure insideopen, and it does not depend on object-store behaviour: write a valid dataset with a different schema at a path, then open it withLanceCycleWritertwice and assert the second error is the schema refusal rather than "already owns".🤖 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 `@crates/lance-graph/src/graph/cycle_sink.rs` around lines 1604 - 1641, Replace the environment-dependent bogus-path branch in a_failed_open_leaves_no_leaked_reservation with a deterministic schema-mismatch setup: create a valid dataset at the target path using a different schema, then call LanceCycleWriter::open twice. Assert both attempts fail for the schema mismatch and that the second error does not contain “already owns,” removing the early-return path.
241-262: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRecover from a poisoned registry mutex instead of failing every store.
acquiremaps a poisonedOPEN_WRITERSlock toWriteFailed("writer registry poisoned").Dropusesif let Ok(...)and silently skips the removal.OPEN_WRITERSis a single process-wide mutex for all stores. If one panic poisons it, every laterLanceCycleWriter::openfails for every path, and every live claim leaks its slot permanently.The guarded region only performs
insertandremoveon aHashSet<String>, so the protected data cannot be left in a torn state. Recovering the guard is safe here.♻️ Proposed poison recovery
impl WriterClaim { fn acquire(identity: String) -> Result<Self, WriteFailed> { - let mut set = OPEN_WRITERS - .lock() - .map_err(|_| WriteFailed("writer registry poisoned".into()))?; + // The guarded data is a plain name set; a panic cannot tear it, so + // the poison flag is recovered rather than propagated to every store. + let mut set = OPEN_WRITERS.lock().unwrap_or_else(|e| e.into_inner()); if !set.insert(identity.clone()) { return Err(WriteFailed(format!( "a live LanceCycleWriter already owns {identity} in this process — \ one logical writer per store (drop it first)" ))); } Ok(Self(identity)) } } impl Drop for WriterClaim { fn drop(&mut self) { - if let Ok(mut set) = OPEN_WRITERS.lock() { - set.remove(&self.0); - } + OPEN_WRITERS + .lock() + .unwrap_or_else(|e| e.into_inner()) + .remove(&self.0); } }🤖 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 `@crates/lance-graph/src/graph/cycle_sink.rs` around lines 241 - 262, Update WriterClaim::acquire and WriterClaim::drop to recover the OPEN_WRITERS mutex guard with into_inner() when it is poisoned, rather than returning WriteFailed or silently skipping removal. Keep the existing insert, duplicate-ownership error, and remove behavior unchanged so subsequent writers can proceed and claims are released.
🤖 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.
Inline comments:
In `@crates/lance-graph-planner/src/persist_sink.rs`:
- Around line 275-283: Update the retry-classification documentation on
PersistError::Commit to include CommitError::InvalidArtifact alongside Fenced,
Io, Ambiguous, and HashConflict, explicitly identifying it as permanent and
non-retryable so callers stop regeneration attempts for malformed
artifacts.</code>
In `@crates/lance-graph-supervisor/src/cycle_driver.rs`:
- Around line 843-875: Update the durable after_cycle write path to call
FleetRecovery::checkpoint_bound(recovered_through) and persist its result
instead of storing recovered_through directly. Locate the existing checkpoint
write caller and ensure the bounded value is used for the durable checkpoint
while preserving the existing recovery flow.
In `@crates/lance-graph/src/graph/cycle_sink.rs`:
- Around line 201-231: Separate registry identity from the I/O path: keep the
normalized result of store_identity as the registry key, but retain the caller’s
original path for dataset opening and all raw_append/reopen operations. Update
store_identity’s documentation to describe registry-key normalization only, and
adjust a_second_spelling_of_the_same_store_is_refused to verify the
normalized-key conflict without expecting normalized object-store I/O.
- Around line 331-357: Update guard_schema to reject schemas with extra columns
by comparing got.fields().len() with expected.fields().len() before returning
Ok. Return a WriteFailed error for any field-count mismatch, while preserving
the existing per-field compatibility checks.
---
Nitpick comments:
In `@crates/lance-graph/src/graph/cycle_sink.rs`:
- Around line 1604-1641: Replace the environment-dependent bogus-path branch in
a_failed_open_leaves_no_leaked_reservation with a deterministic schema-mismatch
setup: create a valid dataset at the target path using a different schema, then
call LanceCycleWriter::open twice. Assert both attempts fail for the schema
mismatch and that the second error does not contain “already owns,” removing the
early-return path.
- Around line 241-262: Update WriterClaim::acquire and WriterClaim::drop to
recover the OPEN_WRITERS mutex guard with into_inner() when it is poisoned,
rather than returning WriteFailed or silently skipping removal. Keep the
existing insert, duplicate-ownership error, and remove behavior unchanged so
subsequent writers can proceed and claims are released.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 34405319-8eef-4d02-b74d-7bd5b6cbcfe1
📒 Files selected for processing (13)
.claude/board/EPIPHANIES.md.claude/board/PR_ARC_INVENTORY.md.claude/plans/persistence-artifact-backed-commit-v1.md.claude/plans/persistence-cycle-wal-bootstrap-v1.mdcrates/lance-graph-planner/examples/blw_fusion.rscrates/lance-graph-planner/examples/blw_tenant.rscrates/lance-graph-planner/src/persist_sink.rscrates/lance-graph-supervisor/examples/measure_wal_curve.rscrates/lance-graph-supervisor/src/cycle_driver.rscrates/lance-graph-supervisor/tests/d_ign_b_lenses.rscrates/lance-graph-supervisor/tests/probe_ignition.rscrates/lance-graph-supervisor/tests/probe_ignition_64k.rscrates/lance-graph/src/graph/cycle_sink.rs
| /// An artifact payload violates the concrete writer's binary ABI (the | ||
| /// canonical witness row is exactly 512 bytes). **PERMANENT, not | ||
| /// retryable:** nothing was written, and re-submitting or regenerating | ||
| /// the same malformed batch can never succeed — classifying this as | ||
| /// [`Io`](CommitError::Io) sends the caller into an endless regenerate | ||
| /// loop. Fix the producer. (The [`persist_cycle`] artifact gate tests | ||
| /// payload PRESENCE only; the size ABI is enforced by the writer — the | ||
| /// typed `IntentOnly | Artifact512` split is the Phase-B refinement.) | ||
| InvalidArtifact { row: u64, len: usize }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add InvalidArtifact to the PersistError::Commit retry-class doc.
The doc on PersistError::Commit at lines 438-444 enumerates the honest sub-states for retry logic: Fenced, Io, Ambiguous, and HashConflict. It does not mention InvalidArtifact. A caller that builds its retry policy from that list finds no rule for the new variant. The whole purpose of InvalidArtifact is to stop the endless regenerate loop, so the classification must be reachable from the type the caller actually matches on.
📝 Proposed doc addition on PersistError::Commit
/// The durable commit did not yield an outcome — see [`CommitError`] for
/// the honest sub-states ([`Fenced`](CommitError::Fenced) = regenerate
/// against the new head; [`Io`](CommitError::Io) = nothing landed, safe
/// regenerate; [`Ambiguous`](CommitError::Ambiguous) = re-submit the SAME
/// frozen batch, reconciliation decides; [`HashConflict`](CommitError::HashConflict)
- /// = fail closed).
+ /// = fail closed; [`InvalidArtifact`](CommitError::InvalidArtifact) =
+ /// PERMANENT, never retry — fix the producer).
Commit(CommitError),🤖 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 `@crates/lance-graph-planner/src/persist_sink.rs` around lines 275 - 283,
Update the retry-classification documentation on PersistError::Commit to include
CommitError::InvalidArtifact alongside Fenced, Io, Ambiguous, and HashConflict,
explicitly identifying it as permanent and non-retryable so callers stop
regeneration attempts for malformed artifacts.</code>
| /// [`Self::checkpoint_bound`] is the enforceable form of this rule. | ||
| pub foreign_min_cycle: Option<CycleId>, | ||
| } | ||
|
|
||
| impl FleetRecovery { | ||
| /// The `after_cycle` bound the caller may DURABLY checkpoint after this | ||
| /// pass — the ENFORCEABLE form of the latecomer fence, replacing the | ||
| /// doc-comment instruction with an API the caller cannot mis-read. | ||
| /// | ||
| /// `recovered_through` is the highest cycle this pass fully recovered | ||
| /// (what the caller would naively store). With no foreign landings it | ||
| /// passes through unchanged. With a foreign landing first seen at cycle | ||
| /// `c`, the bound is capped strictly BELOW `c` (`c − 1`, or `None` when | ||
| /// `c` is the first cycle) — so the next `scan_sealed(bound)` still | ||
| /// returns the unrecovered latecomer's tail instead of silencing it. | ||
| #[must_use] | ||
| pub fn checkpoint_bound(&self, recovered_through: Option<CycleId>) -> Option<CycleId> { | ||
| match (recovered_through, self.foreign_min_cycle) { | ||
| (rt, None) => rt, | ||
| (None, Some(_)) => None, | ||
| (Some(rt), Some(f)) => { | ||
| if rt.0 < f.0 { | ||
| Some(rt) | ||
| } else if f.0 == 0 { | ||
| None | ||
| } else { | ||
| Some(CycleId(f.0 - 1)) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find every call site of FleetRecovery::checkpoint_bound.
rg -n -C3 '\bcheckpoint_bound\s*\(' --type=rustRepository: AdaWorldAPI/lance-graph
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -u
# Locate and inspect the relevant FleetRecovery code and nearby test module.
file="crates/lance-graph-supervisor/src/cycle_driver.rs"
if [ -f "$file" ]; then
echo "== file size =="
wc -l "$file"
echo "== outline matches FleetRecovery/checkpoint =="
ast-grep outline "$file" --match 'FleetRecovery|checkpoint_bound' --view expanded || true
echo "== relevant source 760-920 =="
sed -n '760,920p' "$file" | cat -n -v
echo "== recovered_through occurrences =="
rg -n -C3 '\brecovered_through\b|checkpoint_bound|foreign_min_cycle|after_cycle|scan_sealed' "$file"
else
echo "missing $file"
fi
echo "== repository-wide FleetRecovery definitions/usages =="
rg -n -C2 '\bFleetRecovery\b|\bforeign_min_cycle\b|\brecovered_through\b|\back_cycle\b|checkpoint_bound' --type=rust || trueRepository: AdaWorldAPI/lance-graph
Length of output: 17674
Point persists durable bounds through checkpoint_bound.
checkpoint_bound is not called by any Rust caller. Update the durable after_cycle write path to store checkpoint_bound(recovered_through) instead of the naive recovered cycle so the latecomer fence takes effect.
🤖 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 `@crates/lance-graph-supervisor/src/cycle_driver.rs` around lines 843 - 875,
Update the durable after_cycle write path to call
FleetRecovery::checkpoint_bound(recovered_through) and persist its result
instead of storing recovered_through directly. Locate the existing checkpoint
write caller and ensure the bounded value is used for the durable checkpoint
while preserving the existing recovery flow.
| fn guard_schema(dataset_path: &str, ds: &Dataset) -> Result<(), WriteFailed> { | ||
| let expected = cycle_store_schema(); | ||
| let got = ds.schema(); | ||
| for field in expected.fields() { | ||
| let Some(g) = got.field(field.name()) else { | ||
| return Err(WriteFailed(format!( | ||
| "store at {dataset_path} is missing column `{}` — not this \ | ||
| writer's layout (a pre-Phase-A store is rejected, not \ | ||
| reinterpreted; migrate or discard it explicitly)", | ||
| field.name() | ||
| ))); | ||
| }; | ||
| if g.data_type() != *field.data_type() || g.nullable != field.is_nullable() { | ||
| return Err(WriteFailed(format!( | ||
| "store at {dataset_path} column `{}` is {:?} (nullable={}) but this \ | ||
| writer's layout requires {:?} (nullable={}) — rejected, not \ | ||
| reinterpreted", | ||
| field.name(), | ||
| g.data_type(), | ||
| g.nullable, | ||
| field.data_type(), | ||
| field.is_nullable() | ||
| ))); | ||
| } | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the Lance schema type used by guard_schema for a field-count accessor.
set -euo pipefail
rg -nP -C3 '\bfn\s+field\s*\(|\bpub\s+fields\b|\bfn\s+fields\s*\(' --type=rust -g '!target/**' | rg -i 'schema' | head -30Repository: AdaWorldAPI/lance-graph
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -a 'cycle_sink\.rs|Cargo\.toml' . | sed 's#^\./##' | head -100
echo "== cycle_sink relevant sections =="
if fd -a 'cycle_sink\.rs' . | grep -q .; then
f="$(fd 'cycle_sink\.rs' . | head -1)"
echo "FILE=$f"
wc -l "$f"
sed -n '1,120p' "$f" | cat -n
sed -n '280,380p' "$f" | cat -n
fi
echo "== lance deps =="
rg -n "lance|lance_dataset" Cargo.toml crates -g 'Cargo.toml' -C2 || true
echo "== schema usages =="
rg -n "guard_schema|cycle_store_schema|raw_append|ds\.append|append\(" crates/lance-graph/src -g '*.rs' -C2 || trueRepository: AdaWorldAPI/lance-graph
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== cycle_store_schema and raw_append sections =="
cat -n crates/lance-graph/src/graph/cycle_sink.rs | sed -n '180,260p'
cat -n crates/lance-graph/src/graph/cycle_sink.rs 2>/tmp/sep.txt || true
cat -n crates/lance-graph/src/graph/cycle_sink.rs | sed -n '600,820p'
cat -n crates/lance-graph/src/graph/cycle_sink.rs | sed -n '820,960p'
echo "== Lockfile Lance Schema references =="
rg -n "name = \"lance\"|source =|lance-schema|Schema" Cargo.lock crates/lance-graph/Cargo.lock --max-count 80 || true
echo "== local vendored lance sources schema API =="
fd -a 'schema|Schema' Cargo.lock crates/lance-graph/Cargo.lock /tmp 2>/dev/null | head -1 || true
rg -n "struct Schema|impl Schema|pub fn fields|schema.fields" --glob '*.rs' --glob '!target/**' crates/lance-graph/src/graph/cycle_sink.rs crates/lance-graph/Cargo.lock 2>/dev/null || trueRepository: AdaWorldAPI/lance-graph
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Cargo.lock Lance package entries (concise) =="
python3 - <<'PY'
from pathlib import Path
p=Path('Cargo.lock')
if not p.exists():
print('Cargo.lock missing')
raise SystemExit
text=p.read_text()
for i,blk in enumerate(text.split('\n[[package]]\n')):
if i==0: blk='[package]'+'\n'+blk
name='?'
version='?'
for line in blk.splitlines():
if line.startswith('name = ') and 'lance' in line:
name=line.split('=',1)[1].strip().strip('"')
if line.startswith('version = '):
version=line.split('=',1)[1].strip().strip('"')
if 'lance' in name:
print(f'{name} {version}')
PY
echo "== local registry/cargo cache packages if present =="
for d in "$CARGO_HOME/registry/src" "$HOME/.cargo/registry/src" "/usr/local/cargo/registry/src"; do
[ -d "$d" ] || continue
echo "$d"
fd -a 'schema\.(rs|rs\.rs)$' "$d" -g '*lance*' | sed -n '1,20p' || true
rg -n "crate_name = \"lance_schema\"|pub struct Schema|impl Schema|pub fn fields|fields\\(" "$d" -g '*lance*' -g '*.rs' | sed -n '1,80p' || true
done
echo "== Deterministic guard_schema behavior from source =="
python3 - <<'PY'
from pathlib import Path
src=Path('crates/lance-graph/src/graph/cycle_sink.rs').read_text()
start=src.index('/// Refuse a store whose schema')
end=src.index('\n /// The highest durable cycle', start)
guard=src[start:end]
checks={
'checks field names via got.field': 'got.field(field.name())' in guard,
'checks data_type': 'g.data_type() != *field.data_type()' in guard,
'checks nullable': 'g.nullable != field.is_nullable()' in guard,
'recommends field count comparison': 'fields().len()' in guard or 'fields_len' in guard,
}
print('\n'.join(f'{k}: {v}' for k,v in checks.items()))
print(guard)
PYRepository: AdaWorldAPI/lance-graph
Length of output: 1156
🌐 Web query:
Lance 9.0.0 Rust Dataset::open Schema fields accessor
💡 Result:
In Lance 9.0.0 (as part of the underlying Lance core and LanceDB ecosystem), you can access the dataset's schema by using the schema method on a Dataset object [1][2]. Once you have opened a dataset, the schema method returns a reference to a Schema object [1][2]. The Schema struct contains a fields accessor, which is a vector of Field objects (pub fields: Vec) [3]. Example usage: // Assuming you have an opened dataset variable let schema = dataset.schema; // Access the fields for field in &schema.fields { println!("Field name: {}, Data type: {:?}", field.name, field.data_type); } You can also use the field method on the Schema object to retrieve a specific field by its path, which handles nested field paths as well [3]. For operations involving LanceDB tables, note that tables have their own schema access patterns, typically via an async schema method on the Table object [4].
Citations:
- 1: https://docs.rs/lance/latest/lance/dataset/struct.Dataset.html
- 2: https://github.com/lancedb/lance/blob/119f87b3/rust/lance/src/dataset.rs
- 3: https://github.com/lancedb/lance/blob/119f87b3/rust/lance-core/src/datatypes/schema.rs
- 4: https://docs.rs/lancedb/latest/lancedb/table/struct.Table.html
🌐 Web query:
lance 9.0.0 Dataset append schema mismatch record batch schema
💡 Result:
In Lance (and LanceDB), a schema mismatch error during a dataset append operation typically occurs when the schema of the incoming record batch deviates from the existing dataset's registered schema [1][2][3]. The Lance write path performs strict compatibility checks when in append mode [1]. An error will be triggered if the incoming data: 1. Changes the data type of an existing column (e.g., attempting to append Float64 data to a column defined as Float32) [3]. 2. Alters the nullability or order of fields in a way that violates the established schema [1][2]. 3. Introduces structural differences, such as missing non-nullable columns or incompatible nested field definitions [1][2]. Specific technical scenarios leading to this error include: - Extension Type Handling: A known class of issues involves mismatch between Arrow extension types (like pa.json_()) and their underlying physical representations (e.g., LargeBinary) in the dataset [4][5]. Even when data content is logically correct, the metadata mismatch can cause the write path to reject the batch [4][5]. - Subschema Inserts: While recent versions have added support for inserting "subschemas" (allowing new rows to omit nullable columns, which are then backfilled with nulls), these operations still require the types and structures of provided columns to match the existing dataset exactly [6]. - Legacy/Storage Version Issues: Mismatches can sometimes arise from changes in internal storage versions or when internal field IDs do not align correctly during complex schema evolutions [7][1]. To troubleshoot or resolve these errors: - Inspect the schema of the incoming batch (batch.schema) and compare it explicitly against the dataset schema (dataset.schema()) to identify the exact field causing the mismatch [2][3]. - Ensure that the column order and types match, even if field names are correct [3]. - If you are adding new columns or changing existing ones, use dedicated evolution methods like add_columns or merge instead of attempting a direct append of a mismatched schema [8][9][10]. - Check for library versions; some issues related to schema metadata have been fixed in recent releases (e.g., those involving pa.json_() handling) [4][5]. If you are performing schema changes (like adding columns), it is recommended to do so when no other concurrent write operations are taking place, as schema evolutions may conflict with active appends [9][10].
Citations:
- 1: https://github.com/lancedb/lance/blob/119f87b3/rust/lance/src/dataset/write.rs
- 2: LanceDBError: Append with different schema lancedb/lancedb#449
- 3: bug(node, lancedb): unable to 'add' to a table created with a
Float64vector lancedb/lancedb#1293 - 4: bug(rust):
table.add()fails with schema mismatch when appending PyArrow JSON extension type data lancedb/lancedb#3144 - 5: Bug(rust):
dataset.update()fails with schema mismatch on datasets containingpa.json_()columns lance-format/lance#6329 - 6: feat: allow inserting subschemas lance-format/lance#3041
- 7: feat: support complex schemas in append lance-format/lance#2209
- 8: https://docs.lancedb.com/tables/schema
- 9: http://lance.org/guide/data_evolution/
- 10: https://lance.org/guide/data_evolution/
🌐 Web query:
"pub struct Schema" "fields: Vec<Field>" "lance" "9.0.0"
💡 Result:
In the Lance library (specifically within the lance-core crate), the Schema struct is defined as follows: pub struct Schema { pub fields: Vec, pub metadata: HashMap<String, String>, } This structure represents the top-level schema of a Lance dataset [1]. The fields vector contains the top-level Field definitions for the table [1]. The Field struct, which defines individual columns or nested fields, typically includes the following members [2][3][4]: - name: String - id: i32 - parent_id: i32 - logical_type: LogicalType - metadata: HashMap<String, String> - encoding: Option - nullable: bool - children: Vec (or Vec depending on version) - dictionary: Option - unenforced_primary_key_position: Option - (In some versions) unenforced_clustering_key_position: Option Lance uses this schema information to manage columnar storage, data versioning, and ACID-compliant schema evolution [5][6]. While Schema is used internally in the Rust implementation, users typically interact with schemas through high-level APIs in Python, TypeScript/JavaScript, or Rust that leverage Apache Arrow for data definition [7].
Citations:
- 1: https://github.com/lancedb/lance/blob/119f87b3/rust/lance-core/src/datatypes/schema.rs
- 2: https://github.com/lancedb/lance/blob/119f87b3/rust/lance-core/src/datatypes/field.rs
- 3: https://docs.rs/lance/latest/lance/datatypes/struct.Field.html
- 4: https://docs.rs/lance-core/latest/lance_core/datatypes/struct.Field.html
- 5: https://docs.lancedb.com/tables/schema
- 6: https://docs.lancedb.com/lance
- 7: https://docs.lancedb.com/tables
🌐 Web query:
docs.rs lance 9.0.0 Dataset schema .fields .field
💡 Result:
In the Lance library (referenced in version 9.0.0 via the lance crate), the Dataset struct and its associated Schema structure provide mechanisms for accessing field information [1][2]. The Dataset struct has a method .schema() which returns a reference to the Schema object associated with the dataset [1]. The Schema struct defines the dataset's fields in the following way [2]: pub struct Schema { pub fields: Vec, pub metadata: HashMap<String, String>, } Key methods available on Schema to interact with fields include [2]: - .field(name: &str) -> Option<&Field>: Retrieves a field by its path (dot-separated for nested fields). Field names containing dots must be quoted (e.g., parent."child.with.dot"). - .field_id(column: &str) -> Option<i32>: Retrieves the ID of a field by its path. Additionally, the Field struct (defined in lance::datatypes) represents an individual column and provides its own methods for navigating and interacting with the schema, such as .sub_field(path_components: &[&str]) and .field_by_id(id: impl Into<i32>) [3][4].
Citations:
- 1: https://docs.rs/lance/latest/lance/dataset/struct.Dataset.html
- 2: https://github.com/lancedb/lance/blob/119f87b3/rust/lance-core/src/datatypes/schema.rs
- 3: https://docs.rs/lance/latest/lance/datatypes/struct.Field.html
- 4: https://docs.rs/lance-core/latest/lance_core/datatypes/struct.Field.html
Reject stores with extra columns before open succeeds.
guard_schema only checks that every expected column names a compatible stored column. A store that adds columns still passes, then the first ds.append(reader, None) fails later from Lance schema mismatch after the writer reports success. Add a field-count equality check using the stored Schema.fields.len() before returning Ok.
🤖 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 `@crates/lance-graph/src/graph/cycle_sink.rs` around lines 331 - 357, Update
guard_schema to reject schemas with extra columns by comparing
got.fields().len() with expected.fields().len() before returning Ok. Return a
WriteFailed error for any field-count mismatch, while preserving the existing
per-field compatibility checks.
Three review findings, all real. **First-Create ambiguity (the symmetric half of the append path).** A failed Append leaves `ds == Some`, so a reconciliation read has a store to answer from. A failed Create does not: afterwards `ds == None` is exactly as consistent with "the manifest published and this reopen cannot see it yet" as with "nothing happened" — and the writer was reading that as absence, i.e. `Io/nothing published`, i.e. an invitation to regenerate, i.e. a SECOND Create against an unresolved first one. `create_unknown` makes the doubt explicit and sticky. While it is set, NOT-FOUND is never proof of absence (`Ambiguous`), and `commit_cycle` refuses to Create again. It clears only when storage proves the store exists — a successful commit, a reconciled batch, or any readable frame. `bootstrap()` is the sanctioned way out: explicit infrastructure creation, separated from semantic cycle publication. It publishes an empty dataset and so moves the head — a batch frozen against the pre-bootstrap horizon comes back `Fenced`, which is honest and safe (nothing of it is durable). Four falsifiers: published Create + lost ack + visible reopen → Reconciled, one frame; published Create + lost ack + NOT-FOUND reopen → Ambiguous, and the same frozen batch later reconciles without a second dataset; unpublished Create → Ambiguous, retry refuses to Create, bootstrap resolves it, exactly one frame across the whole episode; bootstrap on an existing store publishes nothing. The injection now models a lost Create ack faithfully — the write reaches storage and the HANDLE never comes back. **The taxonomy no longer folds at the supervisor boundary.** The outer caller sees only `CycleError::Seal`, whose doc said "nothing published, regenerate". That is true of two of the five commit errors and harmful for two others: regenerating an `Ambiguous` cycle risks a second publication, regenerating an `InvalidArtifact` one loops forever on an identical malformed batch. `SealFailure::recovery() -> SealRecovery` carries the decision out — Regenerate / ResubmitFrozen / Permanent / Escalate — with a falsifier that checks all five causes and that the classifier is not a constant. **`store_identity` no longer touches the I/O path.** Normalizing the string we hand to Lance was a silent behaviour change on backends where spelling is significant: `s3://bucket//x` is a different object key from `s3://bucket/x`, and a UNC path's leading `\\server\share` does not survive separator collapsing. The normalized form is the registry key only; `Dataset::open` gets the caller's string verbatim. The doc now states what the lexical claim does NOT cover (`..`, symlinks, `file://` vs bare, object-store URI equivalence) rather than implying it away. Also: the schema guard rejects unknown EXTRA columns, not just missing ones; `AppliedCycle.version` is renamed `publication_version` (a bare `version` beside a `SealedCycle` that now distinguishes the two is the same ambiguity one layer up); `run_cycle`'s two contradictory borrow paragraphs are down to the honest one; `max_cycle` is documented as O(1) memory but still O(history) I/O, not "bounded". Tests: cycle_sink 23, cycle_driver 27, persist_sink 22, supervisor probes — green; fmt clean; clippy adds no new warnings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KCGhDYoQBXs3poaR7sFuqp
Follow-up to #912, from merged
main(0bb2a1fe). #912 stays the baseline; this closes the five semantic gaps its review surfaced, each with a falsifier rather than a doc comment.1. A reconciled retry no longer claims a publication version
#912 deliberately named
CommitOutcome::Reconciled.current_head— notversion— because it is the store head at reconciliation time. One layer up,seal_cycleadopted it asSealedCycle.versionanyway, so retrying cycle 1 while the head stood at V5 recorded cycle 1 as sealed into V5. The careful name survived; the meaning did not.SealedCyclenow carries:publication_version: Option<DatasetVersion>—Someonly for a freshCommitted. A position that was never observed stays unknown; the durable identity is(cycle, batch_hash)and the position is an audit-path read.observed_head: Option<DatasetVersion>—Someonly when a sink actually observed one.NoneonNoChange, which calls no sink at all: itsheadis the caller's assertedbase_version, and laundering that into an "observed" head is exactly the confusion being removed.2. The latecomer fence is now enforceable
FleetRecovery::foreign_min_cycleshipped with the right rule in its doc comment — "the caller must never raise its durableafter_cyclebound to or past this cycle." A rule stated in prose to a caller holding a plainOption<CycleId>is an instruction, not a guard.checkpoint_bound(recovered_through)returns the bound the caller may store, capped strictly below any foreign landing.3. A malformed artifact is permanent, not retryable
A 511-byte payload violates the writer's 512-byte ABI and was reported as
CommitError::Io— the variant that means "nothing published, safe to regenerate". A caller obeying that contract regenerates the identical malformed batch forever.CommitError::InvalidArtifact { row, len }is permanent by construction.4. Single-writer ownership is structural, not string equality
store_identitycollapses.segments, duplicate separators and trailing slashes, sox/./s.lancecannot claim a second slot besidex/s.lance. Deliberately lexical, not filesystem canonicalization —.., symlinks and cross-scheme aliases are stated as out of scope rather than implied away, and remain the deployment lease's problem.WriterClaimacquired before the first.await, so anopenthat errors or is cancelled mid-Dataset::openreleases its slot throughDrop. Previously no RAII owner existed at that point and the reservation leaked.5. Honest startup and schema guards
committed_throughthrough a streaming frame-projected fold (max_cycle) instead of materialising the whole timeline. O(1) memory, still O(history) I/O — stated that way, not called "bounded".Fault injection
The append → reopen → reconcile branch carries the whole no-rollback contract and cannot be made to fail on demand through real Lance. A
#[cfg(test)]fault-injection seam now covers its three arms: append fails unpublished (→Io, then the same batch commits), append publishes but the acknowledgement is lost (→Reconciled, exactly one durable frame), and the reconciliation read itself fails (→Ambiguous, resolved by re-submitting the same frozen batch).Also
scan_sealedis payload-free by contract — stated on the trait and modelled by all seven fakes, which previously cloned payloads and so proved a property the real writer does not hold.persist_cycle'sNoChange.headprovenance is documented, andcontent_hash's inclusion ofbase_versionis documented as deliberate: a re-derived frame is a different assertion and must fail closed rather than launder the divergence.Deferred, named
Createambiguity — an unknownCreateoutcome still treats a laterNotFoundas absence. This is the symmetric half of the append path and is Phase-A correctness, not later work; it lands on this branch before merge.IntentOnly | Artifact512(the gate still tests payload presence),run_cycle's borrow-over-.awaitprose, landing-row rollup.Tests
cycle_sink 19, cycle_driver 26, persist_sink 22, supervisor integration probes — all green.
cargo fmtclean;cargo clippyadds no new warnings (the 10 pre-existing ones were verified againstmainby stashing).🤖 Generated with Claude Code
https://claude.ai/code/session_01KCGhDYoQBXs3poaR7sFuqp
Summary by CodeRabbit
New Features
Bug Fixes