fix(engine): close the 2 Lance 7.0.0 alignment failures (immutable PK + native namespace) - #236
Conversation
… 7's immutable unenforced primary key Lance 7 (dataset/transaction.rs) makes the unenforced primary key immutable once set: any write touching the reserved `lance-schema:unenforced-primary-key` field metadata after the PK is set errors "cannot be changed once set" — even re-applying the same value. `migrate_v1_to_v2` previously relied on the old Lance 6 idempotency (re-applying the annotation was a no-op-ish bump), which it needs for crash-recovery: a v1 graph that crashes after the field-set but before the stamp bump re-enters the migration with the PK already present. Under Lance 7 that re-entry now errors, so a real pre-v0.4.0 graph crashing in that window could never complete its migration. Guard the field-set with `schema().unenforced_primary_key().is_empty()` so a genuine first-set still runs but a re-set is skipped — restoring crash-idempotency by construction. (Fresh graphs bake the PK into manifest_schema() at init and never run this migration.) The existing test_publish_migrates_pre_stamp_manifest_to_current_version is the regression guard: red under Lance 7 before this change, green after. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…TableNotFound) `test_directory_namespace_direct_publish_cannot_replace_native_omnigraph_write_path` pokes Lance's NATIVE DirectoryNamespace (not omnigraph's production write path, which is the manifest merge_insert publisher) to document that it cannot replace omnigraph's authority. Lance 7's DirectoryNamespace routes list/describe/create_table_version through `check_table_status`, which now reports an omnigraph-manifest-tracked table as absent — so all three return TableNotFound for `node:Person` (observed). The native namespace is now fully decoupled from omnigraph's manifest: it cannot enumerate, inspect, or publish over omnigraph's tables. This strengthens the guard's thesis. Realigned the assertions to the v7 behavior and kept the authority check (omnigraph's refresh ignores the direct append; row_count stays 0). Test-only; no production impact. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…gnment stanza The #229 stanza verified a clean engine *build* but not the test suite, and claimed "no Lance API surface omnigraph uses changed." Two runtime behaviors did, caught only by the full test suite: - the unenforced primary key is immutable once set in v7 (transaction.rs) — broke the v1→v2 manifest migration's crash-idempotency; fixed by an is-set guard; - the native DirectoryNamespace returns TableNotFound for omnigraph manifest-tracked tables (dir.rs) — test-only; the surface guard was realigned. Corrects the over-broad "no surface changed" claim, adds both findings, and notes the lesson: a clean build is not a clean alignment — run cargo test --workspace before declaring a Lance bump done. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let assert_table_not_found = |what: &str, dbg: String| { | ||
| assert!( | ||
| dbg.contains("TableNotFound") && dbg.contains("node:Person"), | ||
| "{what}: expected TableNotFound for node:Person, got: {dbg}" | ||
| ); | ||
| }; | ||
| assert_table_not_found( | ||
| "list_table_versions", | ||
| format!( | ||
| "{:?}", | ||
| namespace | ||
| .list_table_versions(ListTableVersionsRequest { | ||
| id: Some(vec!["node:Person".to_string()]), | ||
| descending: Some(true), | ||
| ..Default::default() | ||
| }) | ||
| .await | ||
| .unwrap_err() | ||
| ), | ||
| ); | ||
| assert_table_not_found( | ||
| "describe_table_version", | ||
| format!( | ||
| "{:?}", | ||
| namespace | ||
| .describe_table_version(DescribeTableVersionRequest { | ||
| id: Some(vec!["node:Person".to_string()]), | ||
| version: Some(person_version as i64), | ||
| ..Default::default() | ||
| }) | ||
| .await | ||
| .unwrap_err() | ||
| ), | ||
| ); | ||
| assert_table_not_found( | ||
| "create_table_version", | ||
| format!( | ||
| "{:?}", | ||
| namespace | ||
| .create_table_version(version_metadata.to_create_table_version_request( | ||
| "node:Person", | ||
| person_version, | ||
| 1, | ||
| None, | ||
| )) | ||
| .await | ||
| .unwrap_err() | ||
| ), | ||
| ); |
There was a problem hiding this comment.
All three new assertions rely on format!("{:?}", err) and look for "TableNotFound" + "node:Person" as a substring. This works today, but the Debug representation of Lance's error types is not a stability guarantee — a refactor of the error enum (or even just a wrapping layer) can change the rendered string without breaking the public API. If that happens, the assertions silently fail to match and start printing misleading diagnostics. A typed match on the error variant, or at least to_string() (which exercises Display, a more-stable surface), would be more durable here.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| async fn migrate_v1_to_v2(dataset: &mut Dataset) -> Result<()> { | ||
| dataset | ||
| .update_field_metadata() | ||
| .update( | ||
| "object_id", | ||
| [(OBJECT_ID_PK_KEY.to_string(), "true".to_string())], | ||
| ) | ||
| .map_err(|e| OmniError::Lance(e.to_string()))? | ||
| .await | ||
| .map_err(|e| OmniError::Lance(e.to_string()))?; | ||
| if dataset.schema().unenforced_primary_key().is_empty() { | ||
| dataset | ||
| .update_field_metadata() | ||
| .update( | ||
| "object_id", | ||
| [(OBJECT_ID_PK_KEY.to_string(), "true".to_string())], | ||
| ) | ||
| .map_err(|e| OmniError::Lance(e.to_string()))? | ||
| .await | ||
| .map_err(|e| OmniError::Lance(e.to_string()))?; | ||
| } | ||
| set_stamp(dataset, 2).await | ||
| } |
There was a problem hiding this comment.
is_empty() but doesn't verify the PK is the expected field
dataset.schema().unenforced_primary_key().is_empty() correctly short-circuits when a PK is already registered — but it doesn't assert that the registered PK is object_id. If a future migration or tooling error left a different field annotated as the PK, is_empty() would return false, the update_field_metadata call would be silently skipped, and the manifest dataset would enter v2 with the wrong CAS key — breaking merge-insert row-level conflict detection for that graph permanently. In practice the only writer is omnigraph's own manifest_schema() / this migration, so it can't happen today; but adding a debug_assert_eq!(dataset.schema().unenforced_primary_key(), "object_id") in the else branch would make the invariant machine-checked and surface any future drift at test time rather than silently at conflict-resolution time.
| async fn migrate_v1_to_v2(dataset: &mut Dataset) -> Result<()> { | ||
| dataset | ||
| .update_field_metadata() | ||
| .update( | ||
| "object_id", | ||
| [(OBJECT_ID_PK_KEY.to_string(), "true".to_string())], | ||
| ) | ||
| .map_err(|e| OmniError::Lance(e.to_string()))? | ||
| .await | ||
| .map_err(|e| OmniError::Lance(e.to_string()))?; | ||
| if dataset.schema().unenforced_primary_key().is_empty() { | ||
| dataset | ||
| .update_field_metadata() | ||
| .update( | ||
| "object_id", | ||
| [(OBJECT_ID_PK_KEY.to_string(), "true".to_string())], | ||
| ) | ||
| .map_err(|e| OmniError::Lance(e.to_string()))? | ||
| .await | ||
| .map_err(|e| OmniError::Lance(e.to_string()))?; | ||
| } | ||
| set_stamp(dataset, 2).await | ||
| } |
There was a problem hiding this comment.
test_publish_migrates_pre_stamp_manifest_to_current_version exercises the guard by stripping the stamp from a fresh graph (which already has the PK baked in from manifest_schema()). That's sufficient to turn the test red under Lance 7 and green after the fix, but it never exercises the crash-recovery half of the scenario described in the PR: a genuine pre-v0.4.0 dataset (no PK annotation at all) where the migration ran once, set the PK, crashed before stamping, and must now complete idempotently on re-entry. AGENTS.md rule 12 asks for a regression that reproduces "the predicted symptom" — the actual failing mode for the legacy-graph recovery path (v1 stamp + PK present from a prior crashed run) has no dedicated test. Extending the existing test, or adding a small test_migrate_v1_to_v2_is_idempotent_when_pk_already_set, would close that gap.
… not just non-empty (#239) Greptile follow-up (#236): `migrate_v1_to_v2` guarded the field-set with `unenforced_primary_key().is_empty()`, which skips the set whenever *any* field is the PK — including the (corrupt/unexpected) case where a field other than `object_id` carries it. That would silently leave merge-insert row-level CAS keyed on the wrong column, and Lance 7 forbids changing the PK afterward. Match on the specific PK field instead: `["object_id"]` is the idempotent crash-recovery no-op, `[]` sets it (the genuine pre-v0.4.0 first migration), and any other PK refuses loudly. Defensive — Lance won't let a fresh graph reach the error branch — but correct by construction. The idempotent re-entry path stays covered by test_publish_migrates_pre_stamp_manifest_to_current_version (28 manifest tests green). Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
The Lance 6→7 bump (#229) merged green-on-build but left 2 of ~140 engine tests
failing — a clean build masked two runtime behavior changes. Both reproduced on
main(independent of any other branch). Diagnosed from Lance 7's own source.1. Production bug — immutable unenforced primary key
omnigraph marks
__manifest.object_idas Lance's unenforced PK for merge-insertrow-level CAS. Lance 7 (
dataset/transaction.rs:2472-2480) makes that keyimmutable once set — any re-write errors "cannot be changed once set", even
re-applying the same value. The v1→v2 internal-schema migration relied on Lance 6's
idempotent re-apply for crash-recovery (a crash after the field-set but before
the stamp bump re-enters the migration with the PK already present). Under v7 that
re-entry errors, so a real pre-v0.4.0 graph crashing in that window could never
finish migrating.
Fix: guard the set with
schema().unenforced_primary_key().is_empty()— agenuine first-set still runs, a re-set is skipped. Crash-idempotency restored by
construction. (
db/manifest/migrations.rs.) The existingtest_publish_migrates_pre_stamp_manifest_to_current_versionis the red→greenregression guard.
2. Test-only — native DirectoryNamespace decoupled
A surface-guard test pokes Lance's native
DirectoryNamespace(omnigraphproduction never uses it — its publisher writes
__manifestdirectly). Lance 7'scheck_table_statusnow reports omnigraph's manifest tables absent, solist/describe/create all return
TableNotFound(observed). No productionimpact — the guard was realigned to v7's behavior, which only strengthens its
thesis (the native namespace can't even enumerate omnigraph's tables now).
3. Docs —
lance.md7.0.0 stanzaThe #229 stanza claimed "no Lance API surface omnigraph uses changed" (it verified
the build, not the suite). Corrected: documents both runtime changes and the lesson
— a clean build is not a clean alignment; run
cargo test --workspacebeforedeclaring a Lance bump done. Notes MR-A (
delete_where→ staged two-phase) is nowunblocked by v7 — separate follow-up.
Verification
db::manifest::tests: 28/28 green (was 26 + 2 failing).cargo test --workspace --locked: 1386 passed, 0 failed — the regressions areclosed and nothing else broke.
🤖 Generated with Claude Code
Greptile Summary
This PR fixes two Lance 7.0.0 alignment failures that were missed by the original build-only verification in #229. The production fix guards
migrate_v1_to_v2withschema().unenforced_primary_key().is_empty()to restore crash-idempotency: Lance 7 now errors on any re-write of the unenforced PK (even re-applying the same value), so a crash between the field-set and the stamp bump would have permanently broken migration on pre-v0.4.0 graphs.migrations.rs: adds theis_empty()guard so a crashed-then-retried v1→v2 migration skips the PK set and proceeds to stamp — crash-idempotency is restored by construction.tests.rs: realignstest_directory_namespace_direct_publish_cannot_replace_native_omnigraph_write_pathto Lance 7'sTableNotFoundbehavior (the nativeDirectoryNamespaceno longer enumerates omnigraph's manifest-tracked tables at all, widening the decoupling the test was already asserting).docs/dev/lance.md: corrects the build(deps): bump Lance 6.0.1 → 7.0.0 (correct-by-design substrate alignment) #229 stanza's "no API surface changed" claim to compile-time only, documents both runtime behavior changes, and records the lesson to runcargo test --workspacebefore declaring a Lance bump done.Confidence Score: 4/5
The production fix is correct and targeted — the
is_empty()guard restores crash-idempotency without altering normal migration behavior. The test-side changes faithfully track Lance 7's new runtime behavior with no impact on production paths.The migration guard is logically sound, but it checks only that some PK is present rather than asserting it is the expected
object_idfield. The crash-recovery scenario that motivated the fix also lacks a dedicated regression test — the existing test simulates it via stamp-stripping a fresh graph, not via a genuine pre-v0.4.0 dataset with no PK.Both comments on
migrations.rsare worth a second look — the missingdebug_assertfor PK field identity, and the absence of a targeted crash-recovery regression test.Important Files Changed
unenforced_primary_key().is_empty()guard tomigrate_v1_to_v2to restore crash-idempotency under Lance 7's immutable-PK constraint; the guard doesn't verify the existing PK is the expectedobject_idfield, though this can't go wrong in practice todaytest_directory_namespace_direct_publish_cannot_replace_native_omnigraph_write_pathto Lance 7'sTableNotFoundbehavior; uses fragile{:?}debug-format string matching for all three new assertionsFlowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["migrate_v1_to_v2 called"] --> B{"dataset.schema()\n.unenforced_primary_key()\n.is_empty()?"} B -- "true (no PK set)" --> C["update_field_metadata:\nset object_id as unenforced PK"] C --> D["set_stamp(dataset, 2)"] B -- "false (PK already set)" --> D D --> E["✓ Migration complete"] subgraph "Crash-recovery scenario (fixed)" F["1st run: PK empty → set PK → CRASH\nbefore stamp bump"] --> G["2nd run: PK present\n→ old code: errors (Lance 7)\n→ new code: skips → stamps ✓"] end style C fill:#d4edda style D fill:#d4edda style G fill:#d4eddaReviews (1): Last reviewed commit: "docs(lance): document the 2 runtime beha..." | Re-trigger Greptile