Skip to content

fix(storage): keep a bound handle's re-reads on its own session - #1103

Merged
aaltshuler merged 4 commits into
ModernRelay:mainfrom
ragnorc:fix/storage-binding-reads
Oct 20, 2026
Merged

aaltshuler merged 4 commits into
ModernRelay:mainfrom
ragnorc:fix/storage-binding-reads

Conversation

@ragnorc

@ragnorc ragnorc commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

Why

#1090 binds one storage provider to a graph handle, so its control objects and Lance reads all go to the target's endpoint with its credentials. Five re-reads still opened __manifest, tables or the graph index without the handle's session. On a process whose ambient AWS_* settings name another endpoint, those requests went there instead.

Pointing the ambient endpoints at a recording decoy, unmodified main sent 52 requests to it. With unsigned requests and conditional puts disabled in the ambient environment as well, the workload failed at the first @card mutate.

What

Each re-read now uses the handle's bound session or adapter:

Re-read Stray requests with the fix reverted
The @card cardinality check's live re-read before publish 6 × LIST __manifest/_versions/; the mutate fails
Branch delete's identifier check LIST __manifest/_versions/; the delete fails
Snapshots built without a manifest dataset (merge bases, commit-addressed reads, change-feed pages, proposals, collector views) HEAD of a table's detached manifest; the merge fails
Historical CSR reads 1 × GET __graph_index/csr-current.bin
Lance's create_branch, which probes the new branch on a default session in Lance 13 12 × LIST tree/<branch>/_versions/; the branch create fails

The session-less ManifestCoordinator openers now compile only in test builds, so a production caller cannot reach the ambient store again. A check of the CLI and server, as releases build them, fails with E0599 when main's session-less re-read is put back. That is why they leave forbidden_apis' public read-surface registry.

A unit test also pins the change feed's branch-head re-prove to a fresh session. Pointed at the data session, its cache hits go from 1 to 9.

Already correct on main, so not changed here: Azure option capture, the S3 option classes and the proxy refusal.

Tests

  • crates/omnigraph/tests/bound_storage.rs (new) puts two recording forwarders in front of RustFS and points a child process's ambient endpoints at them. It requires zero decoy requests through writes, reads, the cardinality re-read, a racing publish, a three-way merge, branch create and delete, historical and commit-addressed reads, the change feed, export, optimize and cleanup. It is enrolled in the RustFS job as the required cell bound_handle_never_reaches_the_ambient_endpoint.
  • Each fix reverted alone turns it red, with the counts above.
  • Touched packages with failpoint features: 3263 passed. RustFS suites: 125 passed, including bound_storage, lance_surface_guards, s3_cluster and s3_storage::.
  • The DST storage-cost golden is unchanged. Both clippy graphs with -D warnings, fmt, check-docs, typos, check-agents-md and check-ci-cells pass.

docs/dev/lance.md gains a "Bound branch create" fence row, checked against Lance 13; testing.md and architecture.md name the owner. One changelog.d note is included.

A graph handle opened on a captured storage binding still built some of its
clients from the process-wide Lance session or from the root URI, so an
ambient `AWS_ENDPOINT*` setting sent their requests to another endpoint:

- the live re-read a write takes before publishing (the `@card` edge count)
  opened `__manifest` with the session-less `ManifestCoordinator::open`;
- a branch delete reopened `__manifest` without a session to compare the
  branch identifier, then retired the ref and wrote its archive there;
- a catalog `Snapshot` without a manifest dataset (merge bases and the
  snapshots of commit records behind commit-addressed reads, the change
  feed, proposals, unattached snapshots and collector views) opened its
  tables on the process-wide session;
- a historical CSR read loaded the graph-index artifact through an adapter
  derived from the root URI;
- Lance's `create_branch` probes the new branch location on a default
  session built from the store parameters it is given, which a bound
  dataset leaves empty.

A catalog `Snapshot` now carries the control session it was read with. The
branch-control reopen and the live re-read use the coordinator's and the
handle's session, the direct graph-index builder uses the handle's adapter,
and a native branch create, at the head or at a commit, passes the
binding's captured options. The session-less catalog openers compile only
in test builds, so a build of the CLI or server refuses a new production
caller.

`bound_storage.rs` runs a bound workload in a child process whose ambient
endpoint variables name a recording decoy, and requires the decoy to see
no request. Before this change 52 requests reached it; each of the five
fixes, reverted alone, turns it red. The RustFS job runs it.
The change feed re-proves a named branch's head after its table opens, on
a zero-cache session over the store's registry. Lance keys a cached
manifest by dataset URI and version, plus the ETag where the store reports
one, so the store's data session, warmed by an earlier page, must not be
the one that answers. The test warms a data session with the branch's own
`__manifest`, runs the re-prove and requires the data session's metadata
cache statistics to be unchanged; through the data session it records
eight hits.
The testing guide names the decoy workload and the change feed's re-prove
test, the Lance guide records `create_branch`'s default-session probe as a
compatibility fence, and the architecture guide states that a catalog
snapshot keeps the control session it was read with. A release note
records the requests that now reach a captured destination.
The decoy forwarders relay plain HTTP, and the test panicked when
`OMNIGRAPH_S3_TEST_BUCKET` named AWS itself, with no endpoint override, or
an HTTPS-compatible endpoint, so an ordinary engine test run against a
supported S3 configuration failed. It now skips there with the reason,
under the `skipping s3` prefix the RustFS job's skip guard refuses, so the
job's configured run still has to execute it.
@aaltshuler
aaltshuler added this pull request to the merge queue Oct 20, 2026
Merged via the queue into ModernRelay:main with commit 7fd9cc9 Oct 20, 2026
32 checks passed
ragnorc added a commit to ragnorc/omnigraph that referenced this pull request Oct 21, 2026
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