Skip to content

Replace read_blob with descriptor-first read_blob_at facade#367

Merged
ragnorc merged 15 commits into
mainfrom
claude/console-grade-read-surface-a0d9h4
Jul 18, 2026
Merged

Replace read_blob with descriptor-first read_blob_at facade#367
ragnorc merged 15 commits into
mainfrom
claude/console-grade-read-surface-a0d9h4

Conversation

@ragnorc

@ragnorc ragnorc commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

The blob read path gets the correct-by-design shape ahead of the HTTP
blob endpoint:

  • New public Omnigraph::read_blob_at(target, type, id, prop) resolves one
    pinned snapshot of any ReadTarget (capture_read_view), classifies the
    cell from its persisted blob-v2 descriptor BEFORE any payload API, and
    returns an engine-owned BlobRead: internal blobs stream through a
    bounded, range-capable BlobReader; External references return their
    absolute URI with ZERO server-side store resolution (Lance 9.0.0-rc.1's
    take_blobs eagerly builds a store client for external URIs and issues a
    remote HEAD when the recorded size is zero). BlobVersionTag carries the
    identity tuple (stable_table_id, incarnation_id, table_version, row_id)
    for rename-stable, drop/re-add-safe cache validators.
  • lance::dataset::BlobFile no longer leaks through the public API (the
    SnapshotScanner boundary shape); the old current-branch-only read_blob
    is removed and every caller migrated.
  • Blob-v2 descriptor decoding is extracted into one shared
    crate::blob_descriptor decoder used by both the read facade and the
    merge/rewrite null-check, and its field shape (kind, position, size,
    blob_id, blob_uri) plus BlobKind discriminants are pinned by surface
    guard 21.
  • Row lookup uses a typed exact-id predicate (first_row_id_for_id,
    scanner.filter_expr) instead of SQL-text flattening of the caller's id;
    blob errors are typed (unknown type/row and null cell are NotFound, a
    non-Blob property is BadRequest) instead of uniform bad-requests.
  • The filtered BlobsDescriptions probe is pinned green: the upstream
    projection assertion that motivated /query's blob-column exclusion is
    fixed on the rc.1 pin. The null-column /query response is retained as
    shipped contract; the exec/query.rs comment now says so.

forbidden_apis: read_blob_at classified read-only, .into_arc() moved to
blob_read.rs, storage_layer .dataset() forwarding count 25 -> 26.

Co-Authored-By: Claude Fable 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_0149LrcotcFUeveBZBA4ww5T


Note

Medium Risk
Touches read paths, streaming HTTP semantics, and ingest validation for blob columns; behavior is heavily tested but external-reference and range/conditional handling are user-visible contract.

Overview
Adds end-to-end Blob property access on top of a redesigned engine read path: read_blob_at replaces read_blob, pins reads to branch/snapshot, decodes blob-v2 descriptors first (internal streaming vs external URI with no server-side fetch), and exposes BlobReader / BlobVersionTag instead of Lance’s BlobFile.

Server & CLI: GET/HEAD /graphs/{id}/blob streams internal payloads with ETag, ranges (206/416), 304, and 302 for external refs; omnigraph blob get / blob stat mirror that for embedded and remote graphs (no redirect follow on external URIs). Shared blob_etag lives in omnigraph-api-types.

Ingest hardening: create/overwrite validates canonical {data, uri} blob input and probes readable external URIs at load time so later keyed writes don’t break on schema drift.

Tests, parity matrix rows, OpenAPI, and user docs are updated accordingly.

Reviewed by Cursor Bugbot for commit f6123ce. Bugbot is set up for automated code reviews on this repo. Configure here.

Greptile Summary

This PR replaces the blob read API with a descriptor-first, snapshot-pinned facade. The main changes are:

  • Adds bounded internal blob streaming and external-reference classification.
  • Adds authenticated GET and HEAD blob routes with ranges and cache validators.
  • Adds embedded and remote blob get and blob stat CLI commands.
  • Centralizes blob descriptor decoding and ETag generation.
  • Updates tests, OpenAPI output, and user documentation.

Confidence Score: 5/5

The latest fixes look safe to merge.

  • The validator fix handles comma-separated and weak entity tags.
  • No additional qualifying production issue was found in the descriptor compatibility path.

Important Files Changed

Filename Overview
crates/omnigraph-server/src/handlers.rs Adds authenticated blob GET and HEAD handling with streaming, ranges, redirects, and cache validators.
crates/omnigraph/src/db/omnigraph/blob_read.rs Adds the snapshot-pinned descriptor-first blob read facade and bounded reader.
crates/omnigraph/src/blob_descriptor.rs Centralizes decoding of persisted blob descriptors.
crates/omnigraph-cli/src/client.rs Adds embedded and remote blob retrieval and metadata operations.

Reviews (9): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile

Context used:

  • Context used - AGENTS.md (source)
  • Context used - CLAUDE.md (source)

claude added 3 commits July 17, 2026 19:44
The blob read path gets the correct-by-design shape ahead of the HTTP
blob endpoint:

- New public Omnigraph::read_blob_at(target, type, id, prop) resolves one
  pinned snapshot of any ReadTarget (capture_read_view), classifies the
  cell from its persisted blob-v2 descriptor BEFORE any payload API, and
  returns an engine-owned BlobRead: internal blobs stream through a
  bounded, range-capable BlobReader; External references return their
  absolute URI with ZERO server-side store resolution (Lance 9.0.0-rc.1's
  take_blobs eagerly builds a store client for external URIs and issues a
  remote HEAD when the recorded size is zero). BlobVersionTag carries the
  identity tuple (stable_table_id, incarnation_id, table_version, row_id)
  for rename-stable, drop/re-add-safe cache validators.
- lance::dataset::BlobFile no longer leaks through the public API (the
  SnapshotScanner boundary shape); the old current-branch-only read_blob
  is removed and every caller migrated.
- Blob-v2 descriptor decoding is extracted into one shared
  crate::blob_descriptor decoder used by both the read facade and the
  merge/rewrite null-check, and its field shape (kind, position, size,
  blob_id, blob_uri) plus BlobKind discriminants are pinned by surface
  guard 21.
- Row lookup uses a typed exact-id predicate (first_row_id_for_id,
  scanner.filter_expr) instead of SQL-text flattening of the caller's id;
  blob errors are typed (unknown type/row and null cell are NotFound, a
  non-Blob property is BadRequest) instead of uniform bad-requests.
- The filtered BlobsDescriptions probe is pinned green: the upstream
  projection assertion that motivated /query's blob-column exclusion is
  fixed on the rc.1 pin. The null-column /query response is retained as
  shipped contract; the exec/query.rs comment now says so.

forbidden_apis: read_blob_at classified read-only, .into_arc() moved to
blob_read.rs, storage_layer .dataset() forwarding count 25 -> 26.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0149LrcotcFUeveBZBA4ww5T
Reference-retaining writes (stage_create / stage_overwrite) previously
stored external blob descriptors with size 0, so every later take_blobs
on such a cell paid a remote HEAD against the external store just to
learn the payload size. The size fact is knowable exactly once, at
ingest: annotate_external_blob_sizes now HEADs each external URI cell
once at write time and passes explicit position/size through the logical
blob input's already-supported optional children, which Lance records
into the persisted descriptor.

Reads become descriptor-complete (read_blob_at reports Some(size) for
newly written external references with no external I/O), and an
unreadable external URI now fails the load loudly at ingest instead of
deferring the failure to the first read. Caller-declared ranges and
prepared descriptors pass through untouched, and legacy size-0
descriptors still decode (pinned by blob_descriptor unit tests — the
facade maps the zero size to "unknown", never to null).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0149LrcotcFUeveBZBA4ww5T
Expose the engine's descriptor-first blob read over HTTP as
GET/HEAD /graphs/{id}/blob?type=&id=&prop=&branch= (bearer + read, branch
scope, not admission-gated):

- Internal blobs stream in bounded 4 MiB chunks (Body::from_stream over
  sequential BlobReader::read_range — memory per response is constant in
  blob size), with exact Content-Length, Accept-Ranges: bytes, and a
  strong ETag digested from the identity tuple (stable_table_id,
  incarnation_id, table_version, row_id, property) — rename-stable and
  drop/re-add ABA-safe. If-None-Match answers 304; If-Range is honored.
- Single-range requests (start-end, start-, -suffix) answer 206 with
  Content-Range; an unsatisfiable range answers 416 with bytes */total;
  multi-range requests are unsupported by documented contract and answer
  the full body.
- External-URI blobs answer 302 Found with the stored absolute URI for
  GET and HEAD alike; the server never resolves, proxies, or validates
  the external location at read time (classification is
  descriptor-first, proven by a route test whose referenced file is
  deleted after load).
- Content-Type is sniffed from at most the first 512 bytes with a small
  local magic-byte matcher (HEAD and GET sniff identically so their
  headers agree); Lance stores no media type, so the sniff is
  presentation, not contract.
- Errors are typed end to end: unknown type/id and null cells are 404, a
  non-Blob property is 400.

OpenAPI spec regenerated with the new path pinned in EXPECTED_PATHS,
the bearer-security list, and the flat-path exclusions; endpoint
documented in docs/user/operations/server.md (inventory row + Blob
content subsection under Streaming).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0149LrcotcFUeveBZBA4ww5T
Comment thread crates/omnigraph-server/src/handlers.rs Outdated
Comment thread crates/omnigraph/src/db/omnigraph/blob_read.rs

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e5f7ec6a90

ℹ️ 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 thread crates/omnigraph-server/src/handlers.rs Outdated
The blob route previously addressed only a branch head, while the engine
facade already accepts any ReadTarget. BlobQuery gains a snapshot param;
the handler mirrors run_query's target and policy-branch resolution
exactly, including resolving a snapshot's branch for Cedar when a policy
and actor are present. A branch-omitted request now authorizes against
main rather than no branch — the same shape /read and /query use.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0149LrcotcFUeveBZBA4ww5T
Comment thread crates/omnigraph-server/src/handlers.rs Outdated
claude added 4 commits July 18, 2026 13:52
blob get streams one row's Blob payload to stdout or --out, with
--offset/--length mapping to a Range header remotely and a bounded
BlobReader range embedded; blob stat reports kind/size/etag metadata with
zero payload I/O (HEAD remote, descriptor read embedded). The verbs use a
no-redirect HTTP client so an external reference's 302 is surfaced as the
same typed error the embedded arm produces from the descriptor — external
URIs are never fetched. The identity-derived ETag moves from the server
into omnigraph-api-types so both arms share one derivation; the server
delegates to it.

The snapshot test fixture is inline-only: it documents a pre-existing
engine bug where a merge-mode load onto a blob table holding an
external-reference row fails in Lance's blob structural decoder.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0149LrcotcFUeveBZBA4ww5T
parity_configs is extracted into parity_configs_from so rows can run on
non-default schema/data fixtures; existing callers delegate unchanged.
The blob rows seed a served graph with inline and external rows and
byte-mirror it into the local twin, which is what makes the shared
identity-derived ETag an exact cross-arm pin. Rows cover full and ranged
get, snapshot addressing, internal and external stat, and the external
and out-of-range failure paths. KNOWN_DIVERGENCES stays empty.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0149LrcotcFUeveBZBA4ww5T
… ref

An external-URI cell in a reference-retaining load leaves the table in a
state where every later keyed write fails: strict insert (Append load)
with 'did not produce an inserted-row key filter' and upsert (Merge load)
inside Lance's legacy merger with a blob structural decode error. This
test currently fails at the strict-insert step with that exact message;
the following commit carries the root-cause fix.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0149LrcotcFUeveBZBA4ww5T
annotate_external_blob_sizes injected position/size children into the
logical blob input so Lance would record exact sizes into external
descriptors. But create/overwrite persist the batch schema as the
table's logical blob shape, so an external-bearing load diverged the
persisted schema from the canonical {data, uri} input. Every later
keyed write then failed can_use_create_plan's field-type comparison and
fell off Lance's v2 merge fast path: strict inserts lost the
inserted-row key filter and upserts hit the legacy merger's blob
structural decode error. A binary from before the annotation handles
both, confirming the regression.

The annotation becomes validate_external_blob_references: each external
URI is still probed once at ingest so an unreadable reference fails the
load loudly, but the batch passes through untouched. External
descriptors keep Lance's BlobRange{0,0} size-unknown encoding, which
the shared decoder already maps to an unknown size — nothing in the
engine fetches external payloads, so the recorded size had no consumer
beyond stat display. Graphs written by interim builds of this branch
retain the divergent shape and need re-creation; no released version is
affected.

The CLI blob snapshot test drops its inline-only workaround, so the
merge-over-committed-external path is covered at both the engine and
CLI layers.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0149LrcotcFUeveBZBA4ww5T

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit f823be0. Configure here.

Comment thread crates/omnigraph/src/table_store.rs Outdated
claude added 7 commits July 18, 2026 15:01
Lance accepts both the canonical {data, uri} logical blob input and a
4-child {data, uri, position, size} variant, and create/overwrite
persist whichever shape the batch carries. A divergent persisted shape
breaks every later keyed write (the regression fixed two commits ago),
and until now that divergence surfaced only on the NEXT keyed write,
with a confusing filter error. The reference-retaining write boundary
now asserts the canonical shape and fails the offending write itself,
before any durable effect — closing the class at the boundary instead
of relying on the absence of perturbing code.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0149LrcotcFUeveBZBA4ww5T
An update matching a row whose Blob cell holds an external URI reads
the external object at update time and stores the payload inline; the
reference is not preserved through row rewrites. This is forced by the
substrate (MergeInsert exposes no WriteParams hook for external
references) and has been the behavior since the keyed-write
materializer landed — recording it so it is contract, not surprise.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0149LrcotcFUeveBZBA4ww5T
The blob route compared the whole trimmed header to the ETag, so a
comma-separated list containing the current tag, or its weak W/ form,
streamed a full 200 instead of answering 304. If-None-Match now splits
the list and compares weakly per RFC 9110 §13.1.2. If-Range keeps the
single-validator strong comparison §13.1.5 requires. Flagged by both
review bots on the PR.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0149LrcotcFUeveBZBA4ww5T
ObjectStore::from_uri_and_params failures at ingest were mapped to
OmniError::Lance, which the server boundary renders as a 500 — but a
malformed URI is caller input, not a substrate fault. It now lands in
the same manifest bad-request taxonomy as the unreadable-object case,
so an HTTP load with a bogus URI answers 400. Flagged by review on the
PR.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0149LrcotcFUeveBZBA4ww5T
@ragnorc
ragnorc merged commit 55d3d21 into main Jul 18, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants