Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR binds graph-specific configuration to each constructed Supergraph via a new SupergraphOptions snapshot, ensuring plugin-selected variants don’t inherit runtime settings from the configured supergraph and that per-variant telemetry/usage/persisted-doc behavior stays consistent.
Changes:
- Introduces
SupergraphOptions(traffic shaping, subscriptions, error masking, persisted docs, Hive target, etc.) and threads it through supergraph construction and executor/runtime wiring. - Adds per-runtime background-task scoping (persisted-doc reloaders + Hive usage reporting) with graceful shutdown semantics.
- Extends WebSocket operations to support persisted documents and to run
on_graphql_paramshooks; adds Hive target span attribute and related tests/docs.
Reviewed changes
Copilot reviewed 39 out of 41 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| plugin_examples/replace_schema/src/plugin.rs | Constructs plugin-owned supergraph with SupergraphOptions. |
| plugin_examples/replace_schema/README.md | Documents variant supergraph option requirements. |
| plugin_examples/feature_flags/src/plugin.rs | Builds cached variants with per-variant SupergraphOptions. |
| plugin_examples/feature_flags/README.md | Documents per-flag supergraph option snapshots. |
| lib/router-config/src/traffic_shaping.rs | Adds supergraph-scoped traffic shaping config type. |
| lib/router-config/src/subscriptions.rs | Adds supergraph-scoped subscriptions config type; enables cloning. |
| lib/internal/src/telemetry/traces/spans/tests.rs | Tests recording hive.target attribute. |
| lib/internal/src/telemetry/traces/spans/graphql.rs | Adds hive.target span field + recorder. |
| lib/internal/src/telemetry/traces/spans/attributes.rs | Adds HIVE_TARGET attribute constant. |
| lib/internal/src/telemetry/traces/mod.rs | Implements targeted Hive exporter partitioning by hive.target. |
| lib/internal/src/background_tasks/mod.rs | Adds graceful task registration and graceful shutdown path. |
| lib/hive-console-sdk/src/agent/usage_agent.rs | Fixes cancellation race via select! in flush loop. |
| lib/executor/src/plugins/hooks/on_supergraph_load.rs | Introduces SupergraphOptions and stores it in supergraph data. |
| lib/executor/src/executors/map.rs | Refactors executor config to use supergraph-scoped configs + callback runtime config. |
| lib/executor/src/executors/http.rs | Passes subscription buffer capacity directly to HTTP executor. |
| lib/executor/src/executors/graphql_transport_ws.rs | Defaults missing WS query to empty string. |
| lib/executor/src/executors/error.rs | Removes callback public URL parse errors (moved to config/build-time). |
| lib/executor/src/execution/plan.rs | Updates tests for new executor map constructor signature. |
| lib/executor/src/execution/operation_name.rs | Switches to SupergraphTrafficShapingConfig. |
| e2e/src/websocket.rs | Adds E2E tests for persisted docs + graphql params hooks over WS. |
| e2e/src/testkit/mod.rs | Ensures graceful background-task shutdown in test router drop. |
| e2e/src/telemetry/tracing/hive.rs | Updates expected Hive trace output to include hive.target. |
| e2e/src/storage/mod.rs | Updates expected error string for missing persisted-doc storage. |
| bin/router/src/shared_state.rs | Moves graph-bound runtimes out of shared state into per-supergraph runtime. |
| bin/router/src/schema_state.rs | Builds per-supergraph runtime, caches runtimes, validates/derives options, manages lifetimes. |
| bin/router/src/pipeline/websocket_server.rs | Runs graphql params hooks + persisted docs for WS; records hive target; uses per-runtime usage agent. |
| bin/router/src/pipeline/usage_reporting.rs | Adds per-runtime usage reporting background-task controller + tests. |
| bin/router/src/pipeline/persisted_documents/mod.rs | Adds per-runtime persisted-doc reload background-task controller. |
| bin/router/src/pipeline/mod.rs | Selects supergraph earlier; records hive target; uses per-runtime components. |
| bin/router/src/pipeline/execution_request.rs | Splits preparation into HTTP/WS; routes persisted docs via selected runtime. |
| bin/router/src/pipeline/execution.rs | Uses per-runtime headers plan + error masking runtime. |
| bin/router/src/lib.rs | Registers graceful background-task groups; uses graceful shutdown at router stop. |
| bin/router/src/http_utils/probes.rs | Makes readiness check async (awaits supergraph selection). |
| bin/router/src/error.rs | Removes init-time usage-reporting and persisted-doc endpoint errors (now runtime-scoped). |
| bin/router/Cargo.toml | Adds mockito dev-dependency for new tests. |
| .changeset/supergraph-options-api.md | Announces breaking API change: SupergraphOptions replaces QueryPlannerOptions. |
| .changeset/stop-usage-flush-cancellation-race.md | Documents SDK cancellation/flush fix. |
| .changeset/run_graphql_params_hooks_for_websockets.md | Documents WS graphql params hooks behavior and early-response handling. |
| .changeset/isolate-plugin-supergraph-options.md | Documents isolation of plugin-selected supergraph configuration. |
| .changeset/add_persisted_documents_support_to_websockets.md | Documents persisted documents support for WS operations. |
Suppressed comments (2)
lib/internal/src/telemetry/traces/mod.rs:1
- Spans are silently dropped when a trace’s
hive.targetisn’t present in the same export batch (e.g., root span exported earlier/later than children, or non-GraphQL traces). This can lead to partial/lost traces. Consider (mandatory) adding a fallback path (export with a default target, or return an error) and/or maintaining a bounded trace_id→target cache insideTargetedHiveExporterso later batches can still be routed correctly.
lib/internal/src/telemetry/traces/mod.rs:1 - A new OTLP
SpanExporteris constructed for every(target, spans)partition on every export call, which can be expensive and may create unnecessary connection churn under load or when many targets are active. Consider caching exporters per target (e.g.,Mutex<HashMap<String, HiveConsoleExporter<...>>>) and reusing them across exports, or otherwise reusing underlying HTTP clients/resources.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 41 changed files in this pull request and generated no new comments.
Suppressed comments (5)
lib/internal/src/telemetry/traces/mod.rs:1
TargetedHiveExporter::exportconstructs a brand-new OTLP exporter for every(target, spans)partition on every export call. This can add significant allocation/connection overhead under high throughput or when many targets are active. Consider caching exporters per target (e.g., aMutex<HashMap<String, HiveConsoleExporter<_>>>) and reusing them across exports, or otherwise reusing the underlyingSpanExporterwith per-request headers if supported.
lib/internal/src/telemetry/traces/mod.rs:1- Spans whose trace never includes a string-valued
hive.targetattribute are silently dropped (they never enterpartitions). If “missing target” traces can occur (e.g., early error paths, non-GraphQL spans, or spans emitted beforerecord_hive_targetruns), this will cause silent telemetry loss. Consider adding an explicit fallback behavior (e.g., export to a default target when configured, or emit a warning/metric when dropping spans due to missing target).
lib/router-config/src/subscriptions.rs:1 - The updated impl removed/omitted key behavior details from the doc comments (notably the default protocol behavior and the “subgraph-specific then fallback to
all” resolution order for WebSocket path). Restoring those doc details would help keep the public API/self-service clarity consistent after the refactor.
bin/router/src/schema_state.rs:257 - The
HiveTarget(String)error variant is used both for “invalid target format” and for “missing target”. As written, a missing target will be rendered using the “Invalid Hive Tracing target format …” message, which is misleading. Split this into distinct error variants (e.g.,MissingHiveTargetvsInvalidHiveTargetFormat(String)) or change the existing error message to correctly cover both cases.
if let Some(target) = snapshot.options.hive_target.as_deref() {
if !is_uuid_target_ref(target) && !is_slug_target_ref(target) {
return Err(RouterSupergraphRuntimeError::HiveTarget(target.to_string()));
}
} else if context
.hive
.as_ref()
.is_some_and(|hive| hive.tracing.enabled)
{
return Err(RouterSupergraphRuntimeError::HiveTarget(
"Hive tracing is enabled but no target was provided".to_string(),
));
}
bin/router/src/pipeline/persisted_documents/mod.rs:59
- The worker passes
CancellationToken::new()intotask.run(...), but that token is never cancelled; shutdown relies ontokio::select!dropping therun()future. If the reload task uses its token for orderly shutdown/cleanup or spawns sub-tasks keyed off that token, dropping may not be sufficient and can lead to slower/unclean shutdown. Prefer creating a per-worker token, pass it totask.run(token.clone()), and cancel it when eitherrouter_shutdownorsupergraph_lifetimefires (optionally awaitingrun()to completion after cancellation).
fn persisted_documents_worker(
registration: PersistedDocumentsWorkerRegistration,
router_shutdown: CancellationToken,
) -> Pin<Box<dyn Future<Output = ()> + Send>> {
Box::pin(async move {
match registration {
PersistedDocumentsWorkerRegistration::File(task, supergraph_lifetime) => {
tokio::select! {
_ = router_shutdown.cancelled() => {},
_ = supergraph_lifetime.cancelled() => {},
_ = task.run(CancellationToken::new()) => {},
}
}
PersistedDocumentsWorkerRegistration::Storage(task, supergraph_lifetime) => {
tokio::select! {
_ = router_shutdown.cancelled() => {},
_ = supergraph_lifetime.cancelled() => {},
_ = task.run(CancellationToken::new()) => {},
}
}
}
})
}
|
🐋 This PR was built and pushed to the following Docker images: Image Names: Platforms: Image Tags: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 41 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
lib/internal/src/telemetry/traces/mod.rs:244
TargetedHiveExporter::target_by_tracematches the Hive target attribute using a hard-coded string ("hive.target"). Since this PR introduces a shared constant for the attribute key, using it here avoids drift if the key is renamed and keeps the exporter consistent with the span instrumentation.
e2e/src/testkit/mod.rs:705- Typo in comment: "backgroun" → "background".
// shut down backgroun tasks
| #[derive(Debug)] | ||
| struct TargetedHiveExporter { | ||
| endpoint: String, | ||
| token: String, | ||
| timeout: Duration, | ||
| resource: Mutex<Option<Resource>>, | ||
| } | ||
|
|
There was a problem hiding this comment.
do we want this, thoughts?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 42 changed files in this pull request and generated no new comments.
Suppressed comments (2)
e2e/src/testkit/mod.rs:708
- The teardown comment has grammar/spelling issues (e.g., "this drop" / missing punctuation) that make it hard to understand quickly.
// this drop bg tasks synchronously. graceful_shutdown cannot be awaited here,
// and blocking another thread on it deadlocks because the graceful task handles
// still need this blocked ntex runtime to make progress immediate cancellation is intentional
// for generic test teardown; focused async tests cover final hive flush behavior though
lib/internal/src/telemetry/traces/mod.rs:244
TargetedHiveExporter::target_by_tracematches the Hive target attribute key using a hard-coded string ("hive.target"). This risks typos/drift vs the central attribute constants inspans::attributesand makes refactors harder.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 42 changed files in this pull request and generated no new comments.
Suppressed comments (3)
bin/router/src/schema_state.rs:682
- If runtime construction fails after registering a cache entry, the runtime-cache cleanup task is still holding a waiter for this
cache_id(it was registered in theinsertedbranch). Since the entry is removed fromruntime_cachehere, the waiter should be cancelled (sendRuntimeCacheCleanupMessage::Evicted(cache_id)) so the cleanup task doesn’t retain a dormant waiter and so the samecache_idcan be registered again on a later successful build.
self.runtime_cache
.lock()
.unwrap()
.retain(|(id, entry)| *id != cache_id || !Arc::ptr_eq(entry, &cell));
Err(error)
lib/internal/src/telemetry/traces/mod.rs:245
TargetedHiveExporter::target_by_tracehard-codes the attribute name string ("hive.target"). Since the attribute key is already defined centrally (spans::attributes::HIVE_TARGET) and used elsewhere, using the constant here avoids drift if the key ever changes and keeps the exporter consistent with span recording.
e2e/src/testkit/mod.rs:708- Spelling/grammar issues in this comment make it harder to understand (e.g. “this drop”, “bg”, missing punctuation). Since this explains an intentional shutdown behavior, it’s worth keeping it clear.
// this drop bg tasks synchronously. graceful_shutdown cannot be awaited here,
// and blocking another thread on it deadlocks because the graceful task handles
// still need this blocked ntex runtime to make progress immediate cancellation is intentional
// for generic test teardown; focused async tests cover final hive flush behavior though
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 42 changed files in this pull request and generated no new comments.
Suppressed comments (3)
lib/internal/src/telemetry/traces/mod.rs:276
- The authorization header value is built with
format!("******", self.token), which (a) attempts to moveself.tokenout of&selfand (b) doesn't contain a{}placeholder, so it won't compile and would also omit the required Bearer token. This should format a proper Bearer header using a borrowed token string.
lib/internal/src/telemetry/traces/mod.rs:233 TargetedHiveExporterstores the Hive token as a plainStringbut derivesDebug. If this type is ever logged or included in an error chain, the token could be exposed. Avoid derivingDebugfor credential-bearing structs (or implement a redactedDebug).
e2e/src/testkit/mod.rs:708- The drop comment has a few typos/grammar issues (e.g., "this drop", "bg tasks", missing punctuation), which makes it harder to understand the shutdown rationale when debugging tests. Please correct the wording.
// this drop bg tasks synchronously. graceful_shutdown cannot be awaited here,
// and blocking another thread on it deadlocks because the graceful task handles
// still need this blocked ntex runtime to make progress immediate cancellation is intentional
// for generic test teardown; focused async tests cover final hive flush behavior though
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 44 changed files in this pull request and generated no new comments.
Suppressed comments (2)
lib/internal/src/telemetry/traces/mod.rs:233
TargetedHiveExportercontains the Hive token. DerivingDebugrisks leaking that credential if the exporter is ever logged or included in a debug-formatted error. Prefer removing the derive or providing a redactedDebugimplementation.
e2e/src/testkit/mod.rs:708- The teardown comment has a few grammar issues that make it hard to read (e.g., “this drop bg tasks”, missing punctuation). Since it explains a subtle deadlock risk, it should be clarified.
// this drop bg tasks synchronously. graceful_shutdown cannot be awaited here,
// and blocking another thread on it deadlocks because the graceful task handles
// still need this blocked ntex runtime to make progress immediate cancellation is intentional
// for generic test teardown; focused async tests cover final hive flush behavior though
Fixes CI-only flakes in telemetry logging E2E tests caused by the non-blocking tracing writer racing with stdout capture. The capture helper now: - waits for pending logs to drain before redirecting stdout - keeps stdout redirected until logs emitted during the request are drained This prevents setup logs from leaking into the capture and request logs from arriving after capture ends. Furthermore, the text logging assertions in the telemetry now count only `router::request` lines, excluding unrelated output from the OTLP test collector that might pollute. Fix was also tested in #1380.
Closes #1331
Plugin-selected supergraphs now carry the immutable graph-specific configuration needed to build and execute their own router runtime. Requests, WebSocket operations, subscriptions, persisted-document resolution, subgraph execution, usage reports, and Hive traces use one pinned schema, options, and runtime generation instead of combining a selected schema with settings from the router's configured supergraph.
This keeps plugin variants isolated while preserving the existing YAML structure for configured supergraphs.
The implementation also adds persisted-document preparation and
on_graphql_paramshooks to GraphQL over WebSocket operations, since those operations must be prepared through the selected supergraph's persisted-document runtime before parsing and execution.Ownership model
The implementation keeps the existing schema and runtime split, but makes graph-specific configuration part of the schema generation:
Supergraphowns immutable schema data andSupergraphOptionsSupergraphSnapshotpins one schema and options generationSelectedSupergraphpins that snapshot together with itsArc<RouterSupergraphRuntime>RouterSupergraphRuntimeowns live and compiled state derived from the snapshot and shared router infrastructureRouterSharedStateretains only process-wide policy and infrastructureThis ensures a request cannot use the schema from one plugin variant with the endpoint overrides, persisted-document manifest, error masking, or Hive target from another variant.
SupergraphOptionsSupergraph::from_sdlandSupergraph::from_documentnow acceptSupergraphOptionsinstead ofQueryPlannerOptions:SupergraphOptionscontains:The options are immutable after construction and are stored in
SupergraphData, so every snapshot from an owner observes the same generation.Public
SupergraphOptionsdeliberately does not contain clients, caches, telemetry agents, storage runtimes, callback maps, or background tasks. Those values are live router infrastructure and remain inRouterSupergraphRuntimeor its router-owned construction context.Public
SupergraphOptionsalso deliberately does not deriveDebug. The current fields are configuration values, but avoiding a blanket debug representation prevents future sensitive references or credentials from being exposed accidentally.Configuration ownership
Supergraph-bound configuration
The following configuration now follows the selected supergraph:
query_planner.experimental_abstract_type_foldingtraffic_shaping.all,traffic_shaping.subgraphs, andmax_connections_per_hostoverride_subgraph_urlsheadersoverride_labelsdemand_controlsubscriptionssettingserror_maskingpersisted_documentstelemetry.hive.targetThe existing YAML paths and defaults do not change. Configured supergraphs (
ConfiguredSupergraph) deriveSupergraphOptionsfromHiveRouterConfigduring loading.Router-bound configuration
The rest remain process-wide because they control things like query-planning timeout and query-plan exposure, router-side traffic shaping and inbound request deduplication, authorization enforcement policy, etc.
The Hive token remains router-owned. This change routes multiple targets under one router credential; supporting different Hive accounts per selected graph would require a separate credential-routing design.
Configured and plugin-selected supergraphs
Configured sources
Configured supergraphs preserve the existing YAML and environment override behavior. Loading now follows this order:
SupergraphOptionsfrom the router configurationSupergraphSupergraphRouterSupergraphRuntimefrom the returned snapshot and router-owned contextConfigured-source loading remains asynchronous, matching the existing startup behavior. The router can start healthy but unready while the initial source is fetched and built. Load or construction failures are logged and retried according to the source configuration; readiness becomes available only after a complete generation is published.
The runtime is built eagerly before each publication, so a construction failure leaves the current configured generation untouched, or leaves the router unready when no generation has been published yet.
An end hook replacement keeps its own options. The router does not reapply configured graph settings after the hook, because doing so would combine the replacement schema with configuration from a different owner.
Plugin-selected variants
A plugin-owned variant must construct a complete
SupergraphOptionsvalue and retain itsArc<Supergraph>for as long as the variant remains selectable.Plugin-selected supergraphs no longer inherit graph-specific settings from the configured fallback.
SupergraphOptions::default()remains available when a variant intentionally wants the normal defaults.If a plugin-selected runtime cannot be built, selection fails closed. The router does not retry with the configured supergraph or its settings.
The
feature_flagsandreplace_schemaexamples now demonstrate explicit per-variant operation-name forwarding, error masking, and Hive targets, together with the owner-retention requirement.Runtime construction and caching
RouterSupergraphRuntime::buildno longer receives the completeHiveRouterConfig. It receives only:SupergraphSnapshotThe runtime builds and owns:
Removing the complete router config from runtime and HTTP executor constructors makes it harder for future code to accidentally read graph-specific values from global state.
Configured runtimes remain eagerly built and pinned by the configured slot. Plugin runtimes remain in the existing bounded FIFO cache with a capacity of 10 (we should either increase or make this cap configurable).
Runtime selection is now asynchronous because persisted-document storage initialization can be asynchronous. Each plugin cache entry contains a
tokio::sync::OnceCell, so concurrent first requests for the same supergraph share one initialization without holding a synchronous mutex across anawait.Eviction and retirement only release cache or owner references. They do not invalidate requests, WebSocket connections, or subscriptions that already retain the runtime, this allows them to finish gracefully.
Subgraph executors and subscriptions
SubgraphExecutorMapnow receives only graph-specific executor configuration plus resolved router callback config.Subscription configuration is split by ownership without changing YAML.
All configured and plugin runtimes continue sharing the same callback subscription map, so callback routing and heartbeat enforcement remain process-wide infrastructure.
Persisted documents
Persisted-document lookup now happens after supergraph selection and through the selected runtime.
For HTTP requests, operation preparation now follows this order:
SelectedSupergraphThis prevents an ID from being resolved through one manifest and executed against another schema.
Persisted-document runtime initialization moved out of
RouterSharedState. File watchers and storage pollers are registered per selected runtime, and selector compatibility with the configured GraphQL endpoint is validated while that runtime is built.WebSockets
Incoming GraphQL over WebSocket operations (both subscriptions and queries) now use the same GraphQL parameter preparation as HTTP requests.
This adds support for:
extensionson_graphql_paramsstart hooks and registered end callbacksThe WebSocket connection's already pinned
SelectedSupergraphsupplies the persisted-document runtime. An operation cannot switch manifests or supergraph generations within the connection.If a GraphQL parameters hook returns an early response, the router sends the GraphQL body as a WebSocket
nextmessage followed bycomplete. HTTP status and headers cannot be represented by the GraphQL over WebSocket protocol and are intentionally not forwarded.Hive usage reporting
The global usage agent was removed from
RouterSharedState. Each selected runtime now builds an agent from:HTTP and WebSocket reports are sent only through
SelectedSupergraph.runtime.hive_usage_agent.Hive target format is validated during runtime construction instead of per report. When Hive tracing is enabled, every usable selected runtime must have a target. A missing or invalid target makes runtime construction fail rather than attributing telemetry incorrectly.
Hive trace routing
A single startup-time
x-hive-target-refheader cannot route concurrent requests for different selected graphs. Mutating one exporter header per request would also race with concurrent requests and buffered export batches.The selected target is therefore recorded on each HTTP or WebSocket GraphQL operation span.
TargetedHiveExporterthen:x-hive-target-refheader and the global tokenTraces without a selected target are omitted from the Hive exporter. Other configured exporters are unaffected.
This keeps one global tracing subscriber and batching pipeline while making target routing deterministic under concurrency.
Runtime-scoped background work
The first implementation used one generic dynamic task registrar. It was replaced with two explicit task groups because there are only two runtime-scoped use cases and their shutdown semantics differ:
PersistedDocumentsBackgroundTasksaccepts only file-manifest watchers and storage-manifest pollersHiveUsageReportingBackgroundTasksaccepts only Hive usage agentsEach
RouterSupergraphRuntimeowns asupergraph_lifetimecancellation token. The token remains active while any configured slot, runtime-cache entry, HTTP request, WebSocket connection, or subscription retains that runtime. It is cancelled only when:Arc<RouterSupergraphRuntime>is droppedThis is intentionally later than cache eviction or
Supergraphowner retirement. Existing work can continue using immutable schema and runtime state after the owner stops being selectable.Persisted-document workers stop promptly when the runtime lifetime ends. Hive usage shutdown however:
UsageAgentExt::start_flush_intervalnow observes cancellation while waiting for the next interval, but does not let cancellation interrupt an active flush. This prevents a batch that has already been drained from the buffer from being lost by cancellation.BackgroundTasksManagernow distinguishes ordinary tasks from graceful tasks. Production router shutdown registers the two specialized groups as graceful and awaits their cleanup before telemetry shutdown.Validation and failures
Runtime construction compares graph-specific subgraph maps with the selected snapshot's endpoint map. Unknown names in traffic shaping, URL overrides, headers, demand control, callback subscriptions, WebSocket subscriptions, and error masking produce warnings.
They remain warnings rather than hard errors because configuration and schemas may be deployed on different schedules, and the router historically tolerated entries for future or temporarily absent subgraphs. An absent entry cannot affect an executor in the current snapshot.
Runtime construction also validates:
A failed plugin runtime is not cached and does not fall back to configured graph-specific settings. A failed configured reload is not published. Any workers registered before a partial failure are cancelled through that attempted runtime's lifetime token.
Migration
Router configuration
No YAML paths or defaults change. Existing configured-source routers continue deriving all graph-specific options from the same configuration.
Plugin API
This is a breaking executor API change:
Plugins that previously relied on a selected supergraph inheriting graph-specific router settings must now populate those fields explicitly. This is the intended semantic change: a plugin-owned supergraph is a complete schema and configuration generation.
TODOs