hydrate: Dataset als EIN Zip, und VersionedGraph::hydrate_from - #985
Merged
Conversation
Two things, and the second is why the first was possible. ARCHIVE — the missing distribution shape `copy::hydrate_dir` hydrates a dataset that lives in the object store as a TREE of objects. That is right when the store IS the dataset's home; it is wrong for distribution, where the artifact is a versioned, checksum-pinned release: a tree has no single identity to pin, no atomic publish upstream, and its listing can interleave with a producer's write. Operator ruling 2026-08-22: "bitte als zip, nicht dass wir ein Verzeichnis mit einzelnen Dateien shippen". Zip rather than tar, concretely: a zip ends with a central directory, so entries can be enumerated and sought without a sequential scan — which is also what lets this validate the WHOLE index before extracting a single byte. `archive::hydrate_archive` composes what the crate already owns — `hydrate_file` for the pinned fetch, `publish_by_rename` + `StagingKind::Dir` for the atomic publish — and adds only the middle: expanding one verified container into private staging under a containment rule. Every entry must live under the declared root, no `..`, no absolute path; the first escaping entry rejects the whole archive, because a dataset missing one file is not a partial success but an unopenable table that `count_rows` reports as wrong-sized. BUILD — the crate did not compile at main Measured on a clean checkout of origin/main (#981): cargo build -p lance-graph-hydrate error[E0599]: no method named `get` found for reference `&dyn ObjectStore` ... 5 errors, in the LIB, not the tests `object_store 0.13.2` — already pinned in the lockfile on main, unmoved by this branch — moved `get`/`put` onto an extension trait. The fix is two `use` lines. `cargo fmt` also rewrites four files, and clippy had a live warning, so the crate was merged unbuilt, unformatted and unlinted. The finding is not the breakage; it is the mechanism that hid it. A crate with zero consumers is in nobody's build graph, so a semver-COMPATIBLE upstream change invalidated it with no gate firing. Its own lib.rs records the absence of a consumer as a cost tradeoff — it is also a verification hole. Filed as EPIPHANIES E-A-CRATE-WITH-ZERO-CONSUMERS-IS-BUILT-BY-NOTHING-AND-CAN-BE-MERGED-BROKEN-1 and ISSUES ISS-HYDRATE-CRATE-HAS-NO-BUILD-GATE, whose durable fix is the next commit on this branch: `VersionedGraph::hydrate_from`, which puts the crate into lance-graph's build graph and closes ISS-REMOTE-URI-CONSTRUCTORS-PREDATE-THE-HYDRATION-DOCTRINE at the same time. Measured, not asserted: 39 tests green (33 pre-existing, 6 new); clippy --all-targets -D warnings clean; disabling the containment check and the file counter turns exactly their two tests red and leaves the other four green, so both new guards are load-bearing. Board hygiene in this commit: LATEST_STATE contract inventory, the epiphany, the issue.
…ght this VersionedGraph::hydrate_from(store, archive, local_base, sha256, root) is the shape ISS-REMOTE-URI-CONSTRUCTORS-PREDATE-THE-HYDRATION-DOCTRINE names. The existing local/s3/azure/gcs address a store WHERE IT SITS; for the three remote ones that makes the object store the store, which the hydration doctrine explicitly does not (object store = SOURCE, local mmap-capable dir = THE STORE). They are kept — addressing a remote store directly is legitimate when a caller means it, and at >1 replica it is the better choice — and this adds the doctrine's own shape beside them. Ensure-hydrated, not hydrate-or-fail: an existing destination is the WARM PATH, returned as Hydration::AlreadyLocal rather than the underlying error. The distinction is returned rather than discarded, because "downloaded 380 MB" and "found it already there" are different facts and one boot line for both hides the one that matters. GraphError gains its own Hydration variant so a caller can tell a checksum mismatch (never retry) from a transport error (retry). THE CORRECTION — my previous commit named the wrong cause That commit explained the un-compiling crate by saying a crate with zero consumers is in nobody's build graph. That is wrong, and it is now corrected on the board rather than quietly left standing. crates/lance-graph-hydrate is listed in [workspace] members (root Cargo.toml:25); cargo build --workspace would compile it. The actual mechanism, measured: NO workflow in this repo runs --workspace or --all. Every gate names one crate by path (--manifest-path crates/<name>/Cargo.toml) — build.yml ×1, rust-test.yml ×14, style.yml ×9. style.yml:152 says so itself in a comment about `cargo fmt --all`. The gate is a hand-maintained ALLOWLIST, so adding a crate to [workspace] members adds it to nothing. Scope, and it is not one crate: of 25 members, ELEVEN appear in no workflow. Two (lance-graph-catalog, lance-graph-planner) are deps of lance-graph, so their libs compile inside a gated build but their tests never run. NINE are gated by nothing at all. lance-graph-hydrate was simply the one a semver-compatible upstream release happened to break. Why the wrong cause was attractive: the crate's own lib.rs prominently records having zero consumers, so a consumer-shaped explanation was pre-loaded by the file being read. It fit the symptom and contradicted nothing visible — exactly when a claim needs its own check. The check was one grep of Cargo.toml. Fixed here: lance-graph-hydrate gets its rust-test.yml + style.yml steps, and becomes a dependency of lance-graph so its lib also compiles inside an existing gate. NOT fixed blind: the other eight. Some exclusions may be deliberate, and adding eight jobs without knowing which trades a silent hole for silent cost — one decision is needed (a single --workspace job, or a recorded rationale per omission), and that is the operator's. ISSUES ISS-CI-GATE-IS-AN-ALLOWLIST-NINE-MEMBERS-UNGATED carries the measurement. Measured, with all five profile variables: lance-graph-hydrate 39 tests green, clippy --all-targets -D warnings clean, fmt clean; lance-graph --lib green incl. two new hydrate_from falsifiers — one that can stay silent (an already-hydrated store returns AlreadyLocal against a remote object that does not exist, so the assertion is evidence about control flow) and one that can fire (the same absent object with no local store must error, which an implementation swallowing every error as AlreadyLocal would fail); cargo clippy -p lance-graph --lib --tests -D warnings clean. Board hygiene in this commit: LATEST_STATE, the superseding epiphany, the new issue, and the superseded marker on the old one.
Rebasing onto `main` after #984 merged dropped this branch's duplicate `ObjectStoreExt` / rustfmt / clippy hunks — they are in `main` now — but left its BOARD entries describing a world that no longer exists. Correcting them before they land, rather than landing false statements and fixing them after: - `ISSUES.md` `ISS-CI-GATE-IS-AN-ALLOWLIST-NINE-MEMBERS-UNGATED` is RESOLVED. The outcome is recorded ABOVE its original text, which is kept verbatim — it was accurate when filed, and an entry rewritten to match the present hides what was known when. - `LATEST_STATE.md` said `lance-graph-hydrate` is "gated for the first time" here and that "eight more members are still ungated". Both were true when written and are false now. - `EPIPHANIES.md` `E-THE-GATE-IS-A-HAND-MAINTAINED-ALLOWLIST-NOT-THE-WORKSPACE-1` gets an appended outcome paragraph; its analysis is untouched. What #984 actually did, against what these entries predicted: BOTH of the two options they offered — a workflow line per member (each measured locally before its gate was armed) AND `cargo build --workspace` as the net that covers future members without a line. And the count in all three was wrong: eleven ungated members, not nine. The check behind it extracted with `"crates/[a-z0-9-]+"` — no underscore — so `crates/surreal_container` was never in the list, and `tools/dto-class-check` is not under `crates/` at all. That is carried forward in the entries rather than quietly corrected, because a membership check blind to two of its inputs is the same defect class those entries describe, one level up: in the instrument instead of the workflow. Still open, unchanged: no `cargo test --workspace` job. Measured at 14 GB across 86 binaries versus 3.5 GB for the compile — the same order as a runner's free disk. A new member's TESTS still need a line. Verified after the rebase and the conflict resolution (both conflicts were the same import, resolved to `main`'s already-merged wording): hydrate 39 tests green, clippy `--all-targets -D warnings` clean, rustfmt clean.
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_59634cf5-0e3f-4cde-99c1-cf7a1c069e91) |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e22b1ba154
ℹ️ 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".
Comment on lines
+204
to
+205
| Err(lance_graph_hydrate::HydrateArchiveError::AlreadyPublished(_)) => { | ||
| Ok((Self::local(local_base), Hydration::AlreadyLocal)) |
There was a problem hiding this comment.
When local_base persists across deployments and expected_sha256_hex changes, this arm treats the old directory as AlreadyLocal without checking which archive produced it. The new pinned artifact is therefore never fetched and the process silently serves the previous graph version; persist and compare the archive identity (or otherwise reject an unverifiable warm directory) before returning success.
Useful? React with 👍 / 👎.
Comment on lines
+239
to
+243
| if let Some(p) = dest.parent() { | ||
| std::fs::create_dir_all(p)?; | ||
| } | ||
| let mut out = std::fs::File::create(&dest)?; | ||
| bytes += std::io::copy(&mut entry, &mut out)?; |
There was a problem hiding this comment.
If a checksum-valid zip contains a single regular entry named exactly root (for example all_lanes.lance) rather than a directory beneath it, under accepts it, this branch creates a file, and the later rename publishes that file as publish_dir while returning Fresh. Any subsequent graph operation then fails because its base is not a directory; validate that the staged root is a directory or require file entries to be descendants of it.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Schließt
ISS-REMOTE-URI-CONSTRUCTORS-PREDATE-THE-HYDRATION-DOCTRINE— die Issue benennt die fehlende Form wörtlich: „Something of the shapehydrate_from(remote) -> VersionedGraph".archive::hydrate_archive— die Distributionsformcopy::hydrate_dirhydriert ein Dataset, das im Object Store als Baum von Objekten liegt. Das ist richtig, wenn der Store das Zuhause des Datasets ist, und falsch für Distribution: ein Baum hat keine Identität zum Pinnen, keinen atomaren Publish stromaufwärts, und sein Listing kann mit dem Schreiben eines Produzenten verschränken.Operator-Ansage, 2026-08-22: „bitte als zip, nicht dass wir ein Verzeichnis mit einzelnen Dateien shippen".
Zip statt Tar, konkret: ein Zip endet mit einem Central Directory — Einträge lassen sich aufzählen und einzeln anspringen, ohne den Rest zu lesen. Ein Tar hat keinen Index, jeder Zugriff ist ein sequentieller Scan. Genau das erlaubt hier auch, den ganzen Index zu prüfen, bevor ein einziges Byte geschrieben wird.
Neu ist nur die Mitte: einen verifizierten Container unter einer Containment-Regel in privates Staging entpacken. Der Fetch ist
file::hydrate_file(SHA-256-gepinnt), der Publishpublish::publish_by_renamemitStagingKind::Dir. Der erste Eintrag, derrootverlässt, verwirft das ganze Archiv — ein Dataset mit einer fehlenden Datei ist kein Teilerfolg, sondern eine nicht öffenbare Tabelle, diecount_rowsals falsch dimensioniert meldet.VersionedGraph::hydrate_fromlocal/s3/azure/gcsadressieren den Store dort, wo er liegt; bei den drei Remote-Varianten wird der Object Store damit selbst zum Store, was die Doktrin ausdrücklich nicht tut (Object Store = Quelle, lokales mmap-fähiges Verzeichnis = DER Store). Sie bleiben — einen Remote-Store direkt zu adressieren ist legitim, wenn ein Aufrufer es meint, und bei >1 Replica die bessere Wahl. Dies stellt die Form der Doktrin daneben.Ensure-hydrated, nicht hydrate-or-fail: ein bereits vorhandenes Ziel ist der warme Pfad (
Hydration::AlreadyLocal), kein Fehler. Die Unterscheidung wird zurückgegeben statt verworfen — „380 MB geladen" und „war schon da" sind verschiedene Ereignisse, und eine Boot-Zeile für beide verbirgt die, auf die es ankommt.GraphError::Hydrationist eine eigene Variante, damit ein Aufrufer Checksum-Mismatch (nie retryen) von Transportfehler (retryen) unterscheiden kann.Falsifiziert
6 Archive-Tests, beide Wächter durch Abschalten als tragend bewiesen: Containment-Check und Datei-Zähler ausgeschaltet → genau die zwei zugehörigen Tests rot, die anderen vier grün.
_transactions/,_versions/, Datendatei) → publiziert, Bytes stimmen, kein Staging-RestAlreadyPublishedohne Netzzugriff (das Remote-Objekt existiert absichtlich nicht; ein Fetch hätte anders gemeldet)Dazu zwei
hydrate_from-Falsifikatoren, bewusst auf das beschränkt, was diese Funktion beiträgt — die Archiv-Mechanik ist eine Crate tiefer falsifiziert.Gesamt: 39 Tests im Crate,
lance-graph --libgrün,clippy --all-targets -D warningssauber, rustfmt sauber.Board
ISS-CI-GATE-IS-AN-ALLOWLIST-NINE-MEMBERS-UNGATEDist durch #984 RESOLVED — das Ergebnis steht über dem unveränderten Originaltext, weil ein auf die Gegenwart getrimmter Eintrag verbirgt, was wann bekannt war. Die Epiphanie behält ihre Analyse und bekommt einen Ergebnis-Absatz.Mit dabei, absichtlich nicht still korrigiert: die Zahl in diesen Einträgen war falsch — elf ungegatete Member, nicht neun. Das Prüf-Regex war
"crates/[a-z0-9-]+", ohne Unterstrich, also warcrates/surreal_containernie in der Liste undtools/dto-class-checkliegt nicht untercrates/. Eine Mitgliedsprüfung, die zwei ihrer Eingaben nicht sieht, ist dieselbe Defektklasse, die diese Einträge beschreiben — eine Ebene höher, im Instrument statt im Workflow.Offen
Kein Konsument ruft
hydrate_frombisher. Das Crate ist mit #984 gegated, kompiliert also in CI; dieser PR gibt ihm die Mechanik, für die es gemintet wurde.Generated by Claude Code