Skip to content

feat(server): serve one standalone graph to the same account on loopback - #1075

Closed
ragnorc wants to merge 7 commits into
ModernRelay:mainfrom
ragnorc:feat/local-server-foundations
Closed

ragnorc wants to merge 7 commits into
ModernRelay:mainfrom
ragnorc:feat/local-server-foundations

Conversation

@ragnorc

@ragnorc ragnorc commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Disposition — 2026-10-20

The second serving model is discontinued. OmniGraph retains one cluster-backed
server model, including on localhost: the ordinary applied state, authentication,
policies, admission, routes and deployment lifecycle.

This PR is closed without merging. The standalone/local serving APIs, generated
local policies and credentials, and local-server discovery/election/reuse
registry are not proceeding. This decision supersedes the implementation and
sequencing proposed below; draft #1017 is being revised accordingly. No part of
this serving stack has shipped. General improvements to the existing server
may be proposed independently without introducing another serving mode.

Historical proposal

What & why

omnigraph-server can serve one standalone graph to clients of the same account on loopback, for an embedder such as the CLI's coming serve command:

  • AppState::local(LocalScope::Graph(binding), options) opens the graph read-only or read-write behind one generated bearer token (revealed once, held as a hash) and a Cedar policy generated for one actor, installed on the engine as well as at the HTTP layer. Every PolicyAction has a local class by an exhaustive match, so an action added later is never granted until it is classified. A read-only local server refuses every write-class route in ingress before reading its body; no local server mounts /cluster/*; MCP ad-hoc reads are on, write tools only when it writes. A root inside a cluster is refused.
  • serve_local(state, listener, extend) runs the deployment's serving lifecycle, now one private Lifecycle both entry points share, on a loopback listener the caller bound. It writes nothing to stdout, disarms the shutdown watchdog when it returns, and reports ServeOutcome::Uncertain instead of exiting.
  • install_log_subscriber(writer) puts the server's restricted log filter on another writer.

Backing issue / RFC

Checklist

  • Change is focused (one logical change)
  • Tests: in-source class/credential tests; auth_policy read-only and read-write local servers and cluster refusal; mcp tool lists; boot_settings::local_serving (loopback refusal, stdout left to the caller, clean drain, uncertain outcome in a child process); a disarmed watchdog test
  • Public docs: control-plane public embedding APIs; server logs note; changelog.d/local-server-embedding.added.md
  • Reviewed against docs/dev/invariants.md: no new writer fencing is claimed; a read-write local server is one writer process under the existing boundary

Local verification

  • RUST_MIN_STACK=16777216 cargo test -p omnigraph-server -p omnigraph-policy — all pass
  • cargo clippy -p omnigraph-server -p omnigraph-policy --all-targets --locked -- -D warnings -W clippy::dbg_macro, cargo fmt --all --check, python3 scripts/check-docs.py, typos — clean

Notes for reviewers

No CLI command uses this yet; omnigraph serve needs the RFC's targets. The deployment's own serving path is unchanged apart from sharing the lifecycle type.

AppState::local opens a standalone graph read-only or read-write behind a
generated bearer token and a Cedar policy generated for one actor, which
it installs on the engine as well as at the HTTP layer. Every policy
action has a local-server class, so an action added later is never
granted until it is given one. A read-only local server refuses every
write-class route in ingress before reading its body. No local server
mounts the cluster deployment routes; each enables MCP ad-hoc reads and
lists write tools only when it writes. A root inside a cluster is
refused.

serve_local runs the deployment's serving lifecycle, now one private
type both entry points share, on a loopback listener the caller bound.
It writes nothing to stdout, disarms the shutdown watchdog when it
returns, and reports an uncertain outcome instead of exiting.
install_log_subscriber puts the server's restricted log filter on
another writer.
@ragnorc
ragnorc marked this pull request as ready for review October 18, 2026 20:30
@ragnorc
ragnorc requested a review from aaltshuler as a code owner October 18, 2026 20:30
The shutdown listener registered its SIGTERM and SIGINT handlers only
when its spawned task first ran, and `serve_local` installed the
lifecycle only after the caller had announced readiness. A signal in
either window met the default disposition and ended the process without
the drain.

The lifecycle now registers both handlers before it returns, and
`start_local` installs it so a caller announces between `start_local`
and `LocalServing::serve`. `serve_local` remains the two calls in one.

ragnorc commented Oct 18, 2026

Copy link
Copy Markdown
Contributor Author

omnigraph-server can open one standalone graph and serve it on a loopback listener. AppState::local builds one bearer token, keeps only its SHA-256 hash, and installs a Cedar policy for one actor. serve_local runs the same shutdown lifecycle as a deployment, writes nothing to stdout, and returns ServeOutcome::Uncertain instead of exiting the process.

That shape is right. One lifecycle, one grant table, no new writer lock. The public binding and the read-write mode do not match draft RFC pull request #1017 at 7c72c7e.

Request changes.

Blocking

  1. LocalBinding is a normalized URI string. The RFC says it is not.

LocalBinding::from_root stores the normalized root. AppState::local then calls Omnigraph::open or Omnigraph::open_read_only. Those calls use storage_for_uri. For s3://, s3_from_root_uri reads AWS_ENDPOINT_URL_S3 and AWS_ENDPOINT_URL. For az://, it uses CanonicalAzureRoot::from_env. The process environment can send the open to a different endpoint than the target names.

The RFC local-embedding contract says LocalBinding is the direct binding the resolver captured: the canonical root and the storage provider settings. It says the value is never a bare URI string, so ambient variables cannot redirect catalog, admission, or Lance access. The RFC also says this interface is final when it lands. from_root(&str) freezes the wrong value.

Open through a store selection the caller already captured. Do not read the endpoint from the environment inside AppState::local. RFC 0059 (#675) is still a draft, so do not wait for its full target type, and do not publish a string constructor as the stable API.

Inline, crates/omnigraph-server/src/local.rs line 42 (RIGHT): LocalBinding::from_root keeps only the normalized root. AppState::local then calls Omnigraph::open / open_read_only on that string. Those opens go through storage_for_uri. s3_from_root_uri reads AWS_ENDPOINT_URL_S3 and AWS_ENDPOINT_URL. az:// uses CanonicalAzureRoot::from_env. The process environment can move the open to a different endpoint than the target names. Draft RFC #1017 says LocalBinding carries the canonical root and the storage provider settings, and that it is never a bare URI string, so ambient variables cannot redirect the open. The same RFC says this interface is final when it lands. from_root(&str) freezes the wrong value. Open through a store selection the caller already captured. Do not read the endpoint again inside AppState::local. RFC 0059 is still a draft. Do not wait for its full target type, and do not publish this string constructor as the stable API.

  1. Read-write serving still ends the process on an uncertain write. The RFC holds that mode until that stop is gone.

LocalAccess::ReadWrite opens the graph for write and grants change, branch, and commit actions. serve_local still shuts down when operations.fatal() fires, then returns Uncertain. The new test serve_local_runs_the_lifecycle_and_leaves_stdout_to_its_caller_uncertain shows the child writes Ok(Uncertain) and exits 2.

Issue #1067 is open. Main's #1070 keeps the server up after a definite storage failure before publication. An uncertain completion still stops the process. The RFC says read-write local servers wait for the change that keeps the server up, so one storage error does not take the graph away from the client. The changelog and docs/dev/control-plane.md present read-write as available now.

Refuse ReadWrite from AppState::local until that shutdown is gone. Remove the read-write claim from the changelog until then. Keep the enum variant if #1077 needs the name. The refusal is the gate.

Inline, crates/omnigraph-server/src/local.rs line 30 (RIGHT): ReadWrite opens the graph for write and the grant table allows change, branch, and commit actions. serve_local still treats operations.fatal() as shutdown and returns Uncertain. The new uncertain test shows Ok(Uncertain) and exit 2. Issue #1067 is open. An uncertain completion still stops the process. Draft RFC #1017 says read-write local servers wait until that stop is gone, so one storage error does not take the graph away from the client. The changelog presents this mode as available now. Refuse ReadWrite in AppState::local until the server stays up. Keep the variant if #1077 needs the name. Remove the read-write claim from the changelog until the gate is open.

  1. docs/dev/invariants.md still says the server is cluster-only.

This change crosses that support boundary. Update the sentence in this pull request so it matches the library that ships here. The omnigraph-server binary stays cluster-only. AppState::local and serve_local serve one standalone graph on loopback and take no cluster admission. Do not document omnigraph serve in that sentence. That command is not in this pull request.

Optional

  1. The read-only engine backstop is not tested, and open_read_only does not keep the handle read-only.

OpenMode::ReadOnly only skips open-time writes. The handle does not store the mode. A later mutate_as runs unless Cedar denies it. The RFC asks for a test where the engine refuses a write even when a route would admit it. The new HTTP test shows ingress returns 403 before the body. Add one mutate_as on the handle from AppState::local(ReadOnly) that expects OmniError::Policy. That pins the layer ingress does not cover.

  1. The embedding page does not tell the caller to add the loopback guard.

serve_local rejects a non-loopback bind. It does not check Host or Origin on graph routes. The RFC puts that guard in the CLI extend layer, so a deployment never gains it. docs/dev/control-plane.md is the embedder contract and does not say this. State that a caller which serves a browser adds that guard in extend: an exact loopback Host, a refused Origin, no CORS header, and Cross-Origin-Resource-Policy: same-origin.

RFC deltas that are not defects here

Siblings

#1077 is stacked on this head and adds LocalScope::Cluster. Review that pull request for cluster admission, the read-only forbid, and lock release. Those are not gaps in this pull request.

#1073, #1074, and #1076 edit crates/omnigraph-server/src/lib.rs, handlers.rs, and tests/integration/auth_policy.rs beside this pull request. They do not change this contract. Expect a textual merge conflict. #1059, #1012, and draft #675 do not touch this serving path. #675 is the reason LocalBinding must not freeze a bare URI first.

Tradeoffs and liability

The shared Lifecycle removes a second copy of the shutdown path. A deployment never disarms the watchdog. A dropped sender still exits at the original deadline. serve_local disarms after drain, so the embedder process stays alive. That split is correct.

The grant table is one exhaustive match over PolicyAction. A new action fails compilation until it has a class. ALL_ACTIONS is a second list. If it lags the match, the new action is not granted. That fails closed. Later local policies should extend this match, not add another generator.

No new lock, queue, or shadow catalog. A read-write local server is one writer process under the current boundary. The control-plane page says so. That matches the deny-list.

The cost is the new public types and the two entry points. That is the right size for one embedding door. The cost becomes permanent if LocalBinding and ReadWrite ship in the wrong shape, because the RFC says this interface does not change later.

This change spends its complexity on one shutdown path and one grant table. It does not spend it on a second storage selector. That is the wrong cut. Token generation and policy compile already happen once per open. The binding and the uncertain-write gate are the parts that have to be right before the API is public.

Validation

Reviewed head 16de8b9. Base 0a62bb7. RFC head 7c72c7e.

RUST_MIN_STACK=16777216 cargo test -p omnigraph-server --lib -- local::tests:: shutdown_signal_tests::

6 passed, 2 ignored. The ignored tests are subprocess helpers. The parent tests run them. This includes the new disarmed-watchdog test and the existing deadline test.

RUST_MIN_STACK=16777216 cargo test -p omnigraph-server --test integration -- a_read_only_local_server a_read_write_local_server a_local_server_refuses a_local_server_reads_ad_hoc

4 passed.

RUST_MIN_STACK=16777216 cargo test -p omnigraph-server --test boot_settings -- local_serving::

3 passed, 1 ignored (the child helper). The runs cover the non-loopback refusal, a clean drain, and Ok(Uncertain) with exit 2.

RUST_MIN_STACK=16777216 cargo test -p omnigraph-server --test boot_settings -- startup_listener_finish_retains_open_ownership -- --exact

1 passed. This is the existing deployment startup owner on the shared lifecycle.

cargo test -p omnigraph-policy --lib -- load_graph_accepts_per_graph_only_policy load_graph_rejects_server_scoped_action

2 passed. graph_from_config and server_from_config use the same kind check as the file loaders.

Not run: the full server suite, Clippy, cargo fmt, S3, or Azure. The ambient-endpoint finding is from s3_from_root_uri and CanonicalAzureRoot::from_env, not from a two-endpoint run.

This pull request adds a serving API. It does not fix a query result. GQT cannot express a listener, a bearer token, or process shutdown. No before/after GQT regression applies. The server tests above are the owner.

ragnorc commented Oct 18, 2026

Copy link
Copy Markdown
Contributor Author

Request changes.

An embedder needs one loopback server for one standalone graph, with one token and one shutdown path. This pull request adds that door. c087b789 then splits start_local from LocalServing::serve and registers SIGTERM and SIGINT before the listener task runs. A signal after the caller announces now drains the server. Three earlier blockers are still open at 6a4b02cf. The new release note claims a wider startup window than the code covers.

Earlier findings

Reviewed against 16de8b9d, comment #1075 (comment). The later commits are c087b789 and 6a4b02cf. local.rs is unchanged.

  1. LocalBinding is a normalized URI string. Still open. LocalBinding stores root: String. from_root keeps the normalized root. AppState::local then calls Omnigraph::open or Omnigraph::open_read_only on that string. Those calls use storage_for_uri. s3_from_root_uri reads AWS_ENDPOINT_URL_S3 and AWS_ENDPOINT_URL. azure_from_root_uri uses CanonicalAzureRoot::from_env. The process environment can send the open to a different endpoint than the target names. Draft RFC rfc: graph access and cross-process writes #1017, at 7c72c7e7, says LocalBinding carries the canonical root and the storage provider settings, and that it is never a bare URI string. The same RFC says this interface is final when it lands. Open through a store selection the caller already captured. Do not read the endpoint again inside AppState::local. RFC 0059 is still a draft. Do not publish this string constructor as the stable API.

  2. Read-write serving still ends the process on an uncertain write. Still open. LocalAccess::ReadWrite still opens the graph for write and grants change, branch, and commit actions. serve_local still treats operations.fatal() as shutdown and returns Uncertain. serve_local_runs_the_lifecycle_and_leaves_stdout_to_its_caller_uncertain still expects Ok(Uncertain) and exit 2. Issue bug: one storage write with an unknown outcome stops the server until a manual unlock #1067 is open. Main's fix(server): keep serving after a definite storage failure before publication #1070 keeps the server up after a definite storage failure before publication. An uncertain completion still stops the process. The RFC puts read-write local servers in phase 4, after that stop is gone, so one storage error does not take the graph away from the client. changelog.d/local-server-embedding.added.md still presents read-write as available now. Refuse ReadWrite in AppState::local until the server stays up. Keep the variant if feat(server): serve a cluster to the same account on loopback #1077 needs the name. Remove the read-write claim from the changelog until the gate is open.

  3. docs/dev/invariants.md still says the server is cluster-only. Still open. Line 166 is unchanged. Neither later commit edits that file. This change crosses that support boundary. Update the sentence here. The omnigraph-server binary stays cluster-only. AppState::local and serve_local serve one standalone graph on loopback and take no cluster admission. Do not document omnigraph serve in that sentence. That command is not in this pull request.

  4. The read-only engine backstop is not tested. Still open. Optional. open_read_only skips open-time writes. The handle does not store the mode. A later mutate_as runs unless Cedar denies it. The HTTP test shows ingress returns 403 before the body. Add one mutate_as on the handle from AppState::local(ReadOnly) that expects OmniError::Policy.

  5. The embedding page does not tell the caller to add the loopback guard. Still open. Optional. serve_local rejects a non-loopback bind. It does not check Host or Origin on graph routes. The RFC puts that guard in the CLI extend layer, so a deployment never gains it. docs/dev/control-plane.md still does not say this. State that a caller which serves a browser adds that guard in extend: an exact loopback Host, a refused Origin, no CORS header, and Cross-Origin-Resource-Policy: same-origin.

New delta

c087b789 registers both handlers inside Lifecycle::start, before that function returns, and stores the listener task so drop aborts it. start_local installs the lifecycle and returns. LocalServing::serve serves. serve_local is the two calls in one. 6a4b02cf only restores the log-filter paragraph in docs/user/operations/server.md to its earlier length. The filter is unchanged.

The split matches the window the RFC gives the caller: bind, install the lifecycle, write the record, then serve. The new test blocks the only runtime thread between start_local and serve, sends SIGTERM, and requires Ok(Clean) and exit 0. That is the right proof for this bug. GQT cannot express a listener, a bearer token, or a process signal.

Blocking. The release note overstates that window. changelog.d/server-signal-handlers-at-start.fixed.md says the handlers are registered before the server opens anything, and that a signal during startup always takes the graceful path.

main calls load_server_settings before serve. load_cluster_settings calls admit_serving_snapshot, which opens the cluster store and takes admission. Lifecycle::start runs later, as the first step of serve_config. A signal during that admission still meets the default disposition.

AppState::local opens the graph before start_local. A signal during that open still meets the default disposition.

The code does what the commit message says. After Lifecycle::start returns, a signal is observed even when the listener task has not been polled. Say that in the note. Drop "before it opens anything" and "always".

Optional. The embedder page does not state the process-lifetime signal change. Tokio 1.52.3 installs the libc handler for the rest of the process and never removes it (signal/unix.rs, the Signal caveat). LocalServing's rustdoc says a dropped unserved value leaves a later SIGTERM or SIGINT unable to end the process by default. docs/dev/control-plane.md does not say this. A caller that keeps running after start_local, or after serve returns, has to install its own handlers. State that next to the announce-window sentence.

No other behavior bug showed up in the delta. Drop aborts the listener task. A deployment still does not disarm, so its watchdog still bounds admission release. serve_local still disarms after a clean drain. That split stays correct.

RFC deltas that are not defects here

RFC head is still 7c72c7e7ed2b42fb0e5b3f5140d8235444ddcf33.

start_local and LocalServing are not in the RFC sketch. The RFC says this pull request settles the signatures. The split is how the caller writes the record before announcing. Keep it.

serve_local still returns Result. Err is the right result for a non-loopback listener and for a state that AppState::local did not build. The control-plane page names the loopback refusal. Name the foreign-state error in the same sentence. This is not a behavior bug.

Actor views, a graph inside a cluster, records, election, the Host guard, omnigraph serve, the access route, omnigraph-expect-actor, and error outcome stay out of this pull request on purpose. #1077 owns LocalScope::Cluster. The refusal LocalOpenError::ClusterGraph stays fail-closed until that pull request supplies the conversion.

Siblings

#1077 is stacked on this head. Its history contains c087b789 and 6a4b02cf. Review cluster admission, the read-only forbid, and lock release there.

#1073, #1074, and #1076 edit crates/omnigraph-server/src/lib.rs, handlers.rs, and tests/integration/auth_policy.rs beside this pull request. Expect a textual merge. They do not change this contract.

#1078, #1080, and #1081 are the CLI stack. #1059 and #1012 do not touch this serving path. Draft #675 is why LocalBinding must not freeze a bare URI first.

Tradeoffs and liability

The shared Lifecycle is one shutdown path. Registering the handlers before start returns removes the announce-window death. That is the liability this delta removes.

Tokio never restores the default disposition. The first start_local or serve therefore changes SIGTERM and SIGINT for the rest of the process, including after a dropped LocalServing and after serve returns. That is new liability for a long-lived embedder. The rustdoc states the drop case. The embedder page should state it too. Do not add a second signal implementation to work around Tokio.

The grant table is still one exhaustive match over PolicyAction. ALL_ACTIONS is a second list. If it lags the match, the new action is not granted. That fails closed. Later local policies should extend this match.

No new lock, queue, or shadow catalog. A read-write local server would be one writer process under the current boundary. The control-plane page says so. That matches the deny-list. The mode should not ship until #1067 lands.

The public types are the right size for one embedding door. They become permanent if LocalBinding and ReadWrite ship in this shape, because the RFC says the interface does not change later.

Validation

Reviewed head 6a4b02cffdfd79c90ac709720ba450a907b12e0f. Base 0a62bb7b2aa5b99b012f413807c216277f7f9596. RFC head 7c72c7e7ed2b42fb0e5b3f5140d8235444ddcf33. Pull request was open and this was still the head immediately before posting. No other review covers this head.

RUST_MIN_STACK=16777216 cargo test -p omnigraph-server -p omnigraph-policy --lib -- --test-threads=8 local::tests:: shutdown_signal_tests:: load_graph_accepts_per_graph_only_policy load_graph_rejects_server_scoped_action

Policy: 2 passed. Server lib: 6 passed, 2 ignored. The ignored tests are subprocess helpers. The parent tests run them.

RUST_MIN_STACK=16777216 cargo test -p omnigraph-server --test boot_settings -- --test-threads=4 local_serving:: startup_listener_finish_retains_open_ownership

5 passed, 2 ignored. The ignored tests are the child helpers. This includes a_signal_after_start_local_stops_the_local_server_gracefully, the non-loopback refusal, a clean drain, Ok(Uncertain) with exit 2, and the existing deployment startup owner on the shared lifecycle.

RUST_MIN_STACK=16777216 cargo test -p omnigraph-server --test integration -- --test-threads=4 a_read_only_local_server a_read_write_local_server a_local_server_refuses a_local_server_reads_ad_hoc

4 passed.

Regression for the signal fix. GQT cannot express a listener, a bearer token, or a process signal, so the owner is boot_settings. The pull request test cannot run at 16de8b9d, because start_local does not exist there. A temporary probe at that commit announces, sleeps, then calls serve_local, and the parent sends SIGTERM as soon as the listening file exists. The probe expects exit 0. It was not pushed.

RUST_MIN_STACK=16777216 cargo test -p omnigraph-server --test boot_settings -- --test-threads=1 local_serving::signal_before_serve_local_is_a_graceful_shutdown --exact --nocapture

At 16de8b9d: FAILED. ExitStatus(unix_wait_status(15)). The child had no exit code. Expected Some(0). SIGTERM killed the process. 0 passed, 1 failed.

At 6a4b02cf: a_signal_after_start_local_stops_the_local_server_gracefully passed, inside the boot_settings run above.

Not run: the full server suite, Clippy, cargo fmt, S3, or Azure. The ambient-endpoint finding is from s3_from_root_uri and CanonicalAzureRoot::from_env, not from a two-endpoint run.

Inline anchors

  • crates/omnigraph-server/src/local.rs:42 — blocking, LocalBinding still a URI string.
  • crates/omnigraph-server/src/local.rs:264 — blocking, ReadWrite still opens for write.
  • changelog.d/server-signal-handlers-at-start.fixed.md:1 — blocking, release note overstates the window.
  • crates/omnigraph-server/tests/integration/auth_policy.rs:3673 — optional, add the engine mutate_as backstop.
  • docs/dev/control-plane.md:557 — optional, state the loopback guard on extend.
  • docs/dev/control-plane.md:559 — optional, state that Tokio keeps the signal handlers for the rest of the process.
  • docs/dev/invariants.md:166 — blocking, file is outside this diff, so the cluster-only sentence is only in the summary.

A write whose outcome is unknown still stops the server that ran it, so
`AppState::local` refuses `LocalAccess::ReadWrite` with
`LocalOpenError::ReadWriteUnavailable` until it no longer does. The
read-write path stays and stays tested: the `test-util` feature's
`with_local_read_write` scope opens it for tests alone, enabled from
dev-dependencies only (this crate's own included) and checked by the
test-util guard, and a build with the feature still refuses outside the
scope.

`LocalBinding::local(path)` replaces the string constructor. It takes a
root on this machine's file system, made absolute, and refuses a URI: a
remote root's endpoint and credentials would come from the process
environment when it opens, which a captured binding must not depend on.

A read-only local server's engine refuses a direct write, now tested on
the handle. The developer guide says what the read-write gate is, that
an embedder a browser can reach adds the loopback guard in `extend`, and
that Tokio keeps the signal handlers for the rest of the process. The
invariants say the library serves one standalone graph on loopback
while the binary stays cluster-only, and the release notes no longer
claim read-write or a wider signal window than the code covers.
@ragnorc
ragnorc requested a review from azimafroozeh as a code owner October 19, 2026 00:03
@ragnorc

ragnorc commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

Addressed at 4708c68.

Blocking

  1. LocalBinding.
    • LocalBinding::local(path) replaces the string constructor. It takes a root on this machine's file system, made absolute, and refuses a URI, file:// included.
    • Remote roots wait for a binding that carries the caller's resolved storage settings, so ambient endpoint variables can't redirect an open. No string constructor is published.
    • Test: local::tests::a_local_binding_is_an_absolute_path_and_never_a_uri.
  2. Read-write.
    • AppState::local refuses ReadWrite with LocalOpenError::ReadWriteUnavailable, and the changelog drops the read-write claim.
    • The read-write path stays tested: the test-util feature's with_local_read_write scope opens it for tests alone.
    • That feature is enabled from dev-dependencies only, this crate's own included, and forbidden_apis' test-util guard now covers omnigraph-server.
    • A build with the feature still refuses outside the scope, so CLI test builds where the feature unifies stay gated.
    • Tests: local::tests::read_write_opens_only_inside_the_test_scope, and a gate assertion in auth_policy::a_read_write_local_server_acts_as_its_actor_and_grants_no_more.
  3. invariants.md. It now says the binary is cluster-only and the library serves one standalone graph read-only on loopback.

New delta: the signal release note now says when the handlers register (when the lifecycle starts) and drops "before it opens anything" and "always".

Optional:

  • The engine backstop is tested on the handle: local::tests::a_read_only_local_servers_engine_refuses_a_direct_write expects OmniError::Policy.
  • The control-plane page now covers the loopback guard in extend, Tokio keeping the handlers for the process's life, and the foreign-state refusal.

ragnorc commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

Request changes.

An embedder needs one loopback server for one standalone graph, with one token and one shutdown path. This pull request adds that door. 4708c683 closes the three open blockers and the release-note blocker from the review at 6a4b02cf. One new sentence is wrong: a read-only server tells the caller to start it with --write.

Earlier findings

Reviewed against 6a4b02cf, comment #1075 (comment). The first review was at 16de8b9d.

  1. LocalBinding was a normalized URI string. Resolved. LocalBinding::local takes a filesystem path, makes it absolute, and refuses any text that contains ://, including file://. The field stays private. No string constructor is public. A remote root cannot reach Omnigraph::open_read_only, so this path does not read AWS_ENDPOINT_URL, AWS_ENDPOINT_URL_S3, or CanonicalAzureRoot::from_env. Test: local::tests::a_local_binding_is_an_absolute_path_and_never_a_uri.

  2. Read-write serving still stopped the process on an uncertain write. Resolved. AppState::local returns LocalOpenError::ReadWriteUnavailable when LocalAccess::ReadWrite is asked outside the test scope. changelog.d/local-server-embedding.added.md says read-write is refused. The variant remains, so feat(server): serve a cluster to the same account on loopback #1077 can keep the name. Tests: local::tests::read_write_opens_only_inside_the_test_scope and the gate at the start of auth_policy::a_read_write_local_server_acts_as_its_actor_and_grants_no_more.

  3. docs/dev/invariants.md still said the server is cluster-only. Resolved. The support boundary now says the binary is cluster-only, and that AppState::local and serve_local serve one standalone graph read-only on loopback and take no cluster admission. It does not name omnigraph serve.

  4. The release note overstated the signal window. Resolved. changelog.d/server-signal-handlers-at-start.fixed.md now says the handlers register when the lifecycle starts. For serve that is after settings and cluster admission. For an embedder it is start_local, after AppState::local opened the graph. The note no longer says the handlers register before the server opens anything.

  5. The read-only engine backstop was not tested. Resolved. local::tests::a_read_only_local_servers_engine_refuses_a_direct_write calls mutate_as on the handle from a read-only AppState::local and expects OmniError::Policy.

  6. The embedding page did not name the loopback guard. Resolved. docs/dev/control-plane.md tells a browser-reachable caller to add the guard in extend: an exact loopback Host, a refused Origin, no CORS header, and Cross-Origin-Resource-Policy: same-origin. One extra clause is optional finding 2.

  7. The embedding page did not state the process-lifetime signal change. Resolved. The same page says Tokio keeps the SIGTERM and SIGINT handlers for the rest of the process, including after serve returns and after an unserved LocalServing is dropped. LocalServing's rustdoc says the same for the drop.

New delta

4708c683 is the whole delta since 6a4b02cf. It gates read-write, replaces from_root with local, updates the two notes and the two developer pages, and adds omnigraph-server to the test-util guard in forbidden_apis.

The gate is in the right place. read_write_available is false in a normal build. Under test-util it is true only inside with_local_read_write, which is a Tokio task-local scope. A build that unifies the feature still refuses the mode outside that scope. The feature is empty, enabled from [dev-dependencies] only, including this crate's own dev-dependency. TEST_UTIL_SEAM_PACKAGES now names omnigraph-server beside omnigraph-cluster. That matches the existing seam. It does not add a second way to open the mode.

The path constructor closes the ambient-endpoint hole without freezing a URI string. The stored value is still a string, and it is a local absolute path. See the RFC section for the remaining disagreement, which is the interim the last review asked for.

The uncertain-write shutdown is unchanged, and a production caller can no longer enter it through LocalAccess::ReadWrite. The test scope can. That is how the read-write tests stay alive until issue #1067.

Blocking

  1. The read-only refusal tells the caller to start the server with --write.

ingress::admit answers every write on a read-only local server with 403 and this text: "this local server is read-only; start it with --write to change the graph".

AppState::local refuses ReadWrite outside the test scope. No command in this pull request accepts --write. Draft RFC #1017 puts --write in phase 4, after issue #1067. A client that follows the sentence cannot change the graph.

The recovery text was written when the mode was going to ship. The gate did not change the text. a_read_only_local_server_reads_and_refuses_every_write_before_its_body only checks that the error contains "read-only", so the --write clause can stay.

Say that the server is read-only and that a read-write local server is not available yet. Do not name --write. Extend the test so the error text does not contain --write.

#1077 already contains this head, so it carries the same sentence until it takes this fix. #1076, #1078, #1080, and #1081 also edit ingress.rs from main. The sentence belongs in this pull request.

Inline, crates/omnigraph-server/src/ingress.rs line 128 (RIGHT): This 403 tells the caller to start the server with --write. AppState::local refuses LocalAccess::ReadWrite outside the test scope, and no command in this pull request accepts --write. Draft RFC #1017 puts that flag in phase 4, after issue #1067. Say that the server is read-only and that a read-write local server is not available yet. Do not name --write. Extend a_read_only_local_server_reads_and_refuses_every_write_before_its_body so the error text does not contain --write.

Optional

  1. LocalBinding::root can keep a parent segment. local calls std::path::absolute, then normalize_root_uri. absolute does not fold ... normalize_local_path only trims trailing slashes. A temporary probe at this head, not pushed, stored /tmp/a/../graph.omni. absolutize_lexically in omnigraph-storage already folds . and .. without touching the filesystem. Call that function before the root is stored. Re-export it beside normalize_root_uri if the server crate cannot see it. Do not write a second folder in local.rs. root() is the captured destination, and this interface is final when it lands. Two spellings of one directory become two destinations for a later record. Extend a_local_binding_is_an_absolute_path_and_never_a_uri so the stored root has no parent segment. Open still reaches the directory when the skipped component exists, so this is not a redirect to another machine.

  2. Drop "as omnigraph serve does" from the loopback-guard sentence. The guard instruction itself is right. That command is not in this pull request and not on main. The earlier review asked not to document it here.

Where the code and the RFC disagree

RFC head is still 7c72c7e7ed2b42fb0e5b3f5140d8235444ddcf33. These are not defects in this phase.

Siblings

#1077 is stacked on this head (4708c683 is in its history). Review cluster admission, the read-only forbid on cluster policy, and lock release there.

#1073, #1074, and #1076 edit lib.rs and auth_policy.rs from main. #1076, #1078, #1080, and #1081 also edit ingress.rs. Expect textual merges. They do not define this local contract.

#1059 does not touch these files. #1012 edits auth_policy.rs and forbidden_apis.rs, so those two files can conflict. Draft #675 is why this binding must not freeze a remote URI. Refusing remote roots leaves room for a later constructor that carries provider settings.

Tradeoffs and liability

The shared Lifecycle stays one shutdown path. The read-write gate removes the liability of a public mode that stops the process when a write outcome is unknown. The cost is a test-only scope so the path can be tested before it is available. A later mode should use this gate, not a new feature.

The path constructor removes ambient endpoint redirect for this API. The cost is a public type that stores a path string until RFC 0059 supplies provider settings. Further bindings should be constructors on this private field. A parent segment is the spelling gap that is still open. It is optional above.

The grant table is still one exhaustive match on PolicyAction. ALL_ACTIONS is a second list. If it lags the match, the missing action is not granted. That fails closed.

No new lock, queue, or shadow catalog. The server binary stays cluster-only. A read-only local server is not a writer process.

The --write sentence puts a refused mode back into the HTTP contract. That is the liability this delta adds. One sentence removes it.

Registering the handlers before Lifecycle::start returns removes the announce-window death. Tokio keeps those handlers for the process. The control-plane page and the release note now say so. Do not add a second signal implementation.

Validation

Reviewed head 4708c6833433e8bbbdd39a5da2901f386420de66. Base 0a62bb7b2aa5b99b012f413807c216277f7f9596. RFC head 7c72c7e7ed2b42fb0e5b3f5140d8235444ddcf33. The pull request was open and this was still the head immediately before posting. No review submission covers this head. The author's note at #1075 (comment) records the intended fixes. It is not a review of the result.

Local, at this head:

RUST_MIN_STACK=16777216 cargo test -p omnigraph-server --lib -- --test-threads=8 local::tests:: shutdown_signal_tests::

9 passed, 2 ignored. The ignored tests are subprocess helpers. The parent tests run them. This includes the binding test, the read-write gate, the engine mutate_as backstop, the disarmed watchdog, and the existing deadline test.

RUST_MIN_STACK=16777216 cargo test -p omnigraph-server --test boot_settings -- --test-threads=4 local_serving:: startup_listener_finish_retains_open_ownership

5 passed, 2 ignored. The ignored tests are the child helpers. This includes the signal after start_local, the non-loopback refusal, a clean drain, Ok(Uncertain) with exit 2, and the deployment startup owner on the shared lifecycle.

RUST_MIN_STACK=16777216 cargo test -p omnigraph-server --test integration -- --test-threads=4 a_read_only_local_server a_read_write_local_server a_local_server_refuses a_local_server_reads_ad_hoc

4 passed. The read-write test asserts the production gate before it opens the mode inside the test scope.

RUST_MIN_STACK=16777216 cargo test -p omnigraph-engine --test forbidden_apis -- --test-threads=8 test_util_pin_covers_the_cluster_seam test_util_pin_follows_local_features_and_inherited_aliases test_util_pin_allows_only_the_two_test_crate_enables split_crate_test_util_is_enabled_only_by_dev_dependencies

4 passed.

Regression. GQT cannot express a listener, a bearer token, AppState::local, or a process signal. The owner is the server library tests. The head tests cannot compile at 6a4b02cf, because LocalBinding::local and ReadWriteUnavailable do not exist there. Two temporary probes were added at 6a4b02cf, run, and removed. They were not pushed.

RUST_MIN_STACK=16777216 cargo test -p omnigraph-server --lib -- --test-threads=8 probe_remote_uri_is_refused probe_read_write_is_refused --nocapture

At 6a4b02cf: FAILED. 0 passed, 2 failed.

  • probe_remote_uri_is_refused: remote URI accepted: LocalBinding { root: "s3://bucket/graph" }.
  • probe_read_write_is_refused: read-write was accepted and then failed inside open, with Lance Not found for nonexistent/graph.omni/__manifest. The mode was accepted. The open ran.

At 4708c683, on the restored tree:

RUST_MIN_STACK=16777216 cargo test -p omnigraph-server --lib -- --test-threads=8 local::tests::a_local_binding_is_an_absolute_path_and_never_a_uri local::tests::read_write_opens_only_inside_the_test_scope

2 passed.

Parent-segment probe, temporary, at 4708c683, then removed. The assertion required no .. in LocalBinding::local("/tmp/a/../graph.omni"). FAILED: parent segment kept: /tmp/a/../graph.omni. The URI refusals in that test had already passed. Ask for the folded assertion in the existing test.

CI on this commit, run https://github.com/ModernRelay/omnigraph/actions/runs/39179057728 : Test Workspace, Lint (clippy), Format (rustfmt), GQ Logic Tests, and the S3 and Azure jobs passed. Devin Review skipped (trial expired). Local runs above are the focused suites. The full server suite was not re-run locally. No two-endpoint cloud run was done. The ambient-endpoint proof is the constructor refusal, plus the before probe that accepted s3://bucket/graph.

…-write

A read-only local server refused every write with "start it with --write
to change the graph", though `AppState::local` refuses read-write outside
the test scope and no command accepts `--write` yet. The refusal now says
the server is read-only and that a read-write local server is not
available yet, and the route test checks that it names no `--write`.

`LocalBinding::local` folds `.` and `..` with the storage crate's lexical
absolutizer, now re-exported beside `normalize_root_uri`, so one
directory has one stored spelling. The embedding page no longer cites a
command this change does not ship.
@ragnorc

ragnorc commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

Addressed at 60851ab.

Blocking

  1. The read-only refusal. ingress::admit now answers "this local server is read-only, and a read-write local server is not available yet", with no --write. a_read_only_local_server_reads_and_refuses_every_write_before_its_body asserts the text contains "read-only" and not --write. feat(server): serve a cluster to the same account on loopback #1077 and feat(cli): serve a target for you on loopback #1083 take this through their merges.

Optional

  1. LocalBinding::local folds . and .. with omnigraph_storage::absolutize_lexically, re-exported beside normalize_root_uri through omnigraph-core and the engine's storage module. No second folder exists. a_local_binding_is_an_absolute_path_and_never_a_uri now expects /tmp/a/../b/./graph.omni to store /tmp/b/graph.omni.
  2. "as omnigraph serve does" is gone from the loopback-guard sentence on the control-plane page.

RFC differences: #1017 now names LocalBinding::local for local roots, ClusterGraph rather than promotion, and start_local/LocalServing::release.

Validation: local::tests::, the four a_read_only_local_server/a_read_write_local_server/a_local_server integration tests, clippy on core, engine and server with -D warnings, and check-docs.

Brings in the access route from ModernRelay#1073; its tests and the local server's
sit side by side at the end of auth_policy.rs.
ragnorc added a commit to ragnorc/omnigraph that referenced this pull request Oct 19, 2026
Brings in main through ModernRelay#1075: the access route from ModernRelay#1073, whose tests
follow the local cluster server's at the end of auth_policy.rs.
ragnorc added a commit to ragnorc/omnigraph that referenced this pull request Oct 19, 2026
Brings in ModernRelay#1075's fix through ModernRelay#1077: the read-only refusal names no
--write, and a local binding folds `..` before it is stored.
The engine re-exports the storage crate's lexical absolutizer for the
local server's binding; the split-crate re-export guard lists every such
item by name, so it now names this one.

ragnorc commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

An embedder needs one loopback server for one standalone graph, with one token and one shutdown path. This pull request adds that door. 60851abf removes the false --write instruction and stores one spelling for a path that contains . or ... The merge of main keeps the access route beside that door. The re-export pin matches the guard.

Recommendation: approve. No blocking finding remains.

Earlier findings

Reviewed against 4708c683, comment #1075 (comment). The later commits are 60851abf, d73f3a7d, and c4f844cb.

  1. The read-only refusal told the caller to pass --write. Resolved. ingress::admit now answers "this local server is read-only, and a read-write local server is not available yet". a_read_only_local_server_reads_and_refuses_every_write_before_its_body requires the text to contain "read-only" and to omit --write. A temporary copy of that assertion, not pushed, failed at 4708c683 on POST /mutate with the old sentence. The same test passes at this head.

  2. LocalBinding::root could keep a parent segment. Resolved. LocalBinding::local calls omnigraph::storage::absolutize_lexically before normalize_root_uri. The helper is the storage crate's function, re-exported through omnigraph-core and the engine storage module. No second folder exists. a_local_binding_is_an_absolute_path_and_never_a_uri expects /tmp/a/../b/./graph.omni to store /tmp/b/graph.omni. A temporary copy of that assertion, not pushed, failed at 4708c683 with stored /tmp/a/../b/graph.omni. c4f844cb adds the re-export to SPLIT_CRATE_REEXPORTS. graph_manifest_writer_methods_are_not_public_escape_hatches passes.

  3. The loopback-guard sentence named omnigraph serve. Resolved. docs/dev/control-plane.md ends that sentence at Cross-Origin-Resource-Policy: same-origin.

New delta

60851abf is the behavior fix. d73f3a7d merges main at 1998e945 (#1073, the access route). c4f844cb pins the re-export.

The merge touches four files on both sides: lib.rs, auth_policy.rs, mcp.rs, and docs/user/operations/server.md. The result keeps the local-server routes and the GET /access route, the local tests and the access tests, the MCP access tool rows, and the access row in the route table. No test function from either parent is missing. The read-only refusal text survives the merge. GET /access is a read. authorize returns a denial as Ok, so a local policy that grants only read and export can answer the route.

object_store::path::Path rejects a . or .. segment on parse, and percent-encodes one on Path::from. The storage open already folds with absolutize_lexically inside object_path, so an open of a stored path that still contained .. would still reach the parent directory. The stored string is the destination. The fold belongs on the binding. The open and the stored root now use one function.

The fold does not call canonicalize. A symlink in the path can make the lexical root differ from a filesystem walk. The open uses the same lexical root, so the server opens the spelling it stores.

Blocking

None.

Optional

  1. The public comment still describes the old normalization. LocalBinding::local says the root is made absolute and normalized as the engine normalizes roots. normalize_root_uri trims a trailing slash and does not fold ... The parent fold is a separate step, and it does not resolve a symlink. docs/dev/control-plane.md says only "made absolute". State both facts on the function and on that page. root() is the destination a later record will use.

Inline, crates/omnigraph-server/src/local.rs line 75 (RIGHT): LocalBinding::local folds . and .. with absolutize_lexically before normalize_root_uri. normalize_root_uri trims a trailing slash and does not fold a parent segment. This comment still says the root is made absolute and normalized as the engine normalizes roots. Say that the stored root has no . or .., and that the fold does not resolve a symlink. docs/dev/control-plane.md says only "made absolute". root() is the destination a later record will use.

Where the code and the RFC disagree

RFC head is e15d2f5b85a759b053dda73082a5c9e7fff8e121. The decision log of 2026-10-19 aligns the sketch with this phase.

Siblings

#1073 is merged. This pull request's merge of main takes that route without dropping the local contract.

#1077, #1083, and #1084 contain c4f844cb, so they carry the refusal text and the folded root. Review cluster admission, the read-only forbid, and lock release on #1077.

#1074, #1076, #1078, #1080, and #1081 do not contain this head. They do not contain the --write sentence. Expect a textual merge where they edit ingress.rs, lib.rs, or auth_policy.rs. They do not define this local contract.

#1059 does not touch these files. #1012 can conflict on auth_policy.rs and forbidden_apis.rs. #1072 and #1086 do not touch this serving path. Draft #675 is why this binding still refuses a remote URI.

Tradeoffs and liability

The shared Lifecycle stays one shutdown path. The read-write gate stays closed in production. The 403 no longer points at a flag this tree does not accept. That removes the false recovery instruction.

The path constructor now has one stored spelling for . and ... The cost is one re-export of a doc(hidden) helper, pinned by name, with no second implementation. A symlink can still produce two spellings of one directory. The open uses that same spelling, so the server does not redirect to a different lexical path than the one it stores. A later record should use root().

The grant table stays one exhaustive match on PolicyAction. ALL_ACTIONS is a second list. A missing action is not granted. That fails closed.

No new lock, queue, or shadow catalog. The server binary stays cluster-only. A read-only local server is not a writer process.

Validation

Reviewed head c4f844cb17002518c962000622c6f21e14a84f23. Base 1998e945edb03507004300048be22260e0823b88. RFC head e15d2f5b85a759b053dda73082a5c9e7fff8e121. The pull request was open and this was still the head immediately before the post attempt. No review submission covers this head.

GQT cannot express a listener, a bearer token, an HTTP error body, or LocalBinding. The owners are the server library test and auth_policy. Temporary assertions were added at 4708c683, run, and removed. They were not pushed.

RUST_MIN_STACK=16777216 cargo test -p omnigraph-server --lib -- --test-threads=8 local::tests::a_local_binding_is_an_absolute_path_and_never_a_uri --exact

At 4708c683: FAILED. 0 passed, 1 failed. Stored root /tmp/a/../b/graph.omni. Expected /tmp/b/graph.omni.

RUST_MIN_STACK=16777216 cargo test -p omnigraph-server --test integration -- --test-threads=4 auth_policy::a_read_only_local_server_reads_and_refuses_every_write_before_its_body --exact

At 4708c683: FAILED. 0 passed, 1 failed. POST /mutate returned "this local server is read-only; start it with --write to change the graph" with code forbidden.

At c4f844cb:

RUST_MIN_STACK=16777216 cargo test -p omnigraph-server --lib -- --test-threads=8 local::tests:: shutdown_signal_tests::

9 passed, 2 ignored. The ignored tests are subprocess helpers. The parent tests run them. This includes the folded-root assertion, the read-write gate, the engine mutate_as backstop, and the disarmed watchdog.

RUST_MIN_STACK=16777216 cargo test -p omnigraph-engine --test forbidden_apis -- --test-threads=8 graph_manifest_writer_methods_are_not_public_escape_hatches --exact

1 passed. The re-export pin matches the source.

RUST_MIN_STACK=16777216 cargo test -p omnigraph-server --test boot_settings -- --test-threads=4 local_serving:: startup_listener_finish_retains_open_ownership

5 passed, 2 ignored. The ignored tests are the child helpers.

RUST_MIN_STACK=16777216 cargo test -p omnigraph-server --test integration -- --test-threads=4 a_read_only_local_server a_read_write_local_server a_local_server_refuses a_local_server_reads_ad_hoc

4 passed. The read-only test rejects a body that contains --write.

CI on this commit: Test Workspace, Lint (clippy), Format (rustfmt), S3, and Azure passed in https://github.com/ModernRelay/omnigraph/actions/runs/39276914562. GQT ordinary passed. GQT (dst) in https://github.com/ModernRelay/omnigraph/actions/runs/39276914393 failed case::load_csv_integer_endpoint_memory.gqt on a wall-time budget (worker 1.15s, case 10s): 403 passed, 1 failed. That case is not in this diff. The gate job then failed because that qualification failed. This is not a result defect of the local server.

Not run locally: the full server suite, a two-endpoint cloud open, a symlink path, or Windows. The ambient-endpoint hole stays closed because a URI never reaches open_read_only. The symlink note is from absolutize_lexically, which does not call canonicalize.

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