feat(manifest): support PO and PQL manifest for feature flags#2739
Open
feat(manifest): support PO and PQL manifest for feature flags#2739
Conversation
StarpTech
requested review from
Noroth,
SkArchon,
devsergiy,
endigma and
jensneuse
as code owners
Contributor
WalkthroughDecouples manifest warmup from mux construction and makes the pql manifest store run an eager worker processing coalesced updates. Adds feature-flag-aware persisted-operation validation/storage, manifest regeneration on feature-flag lifecycle events, repository/schema changes, extended tests, and documentation/CLI updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2739 +/- ##
===========================================
+ Coverage 45.74% 64.01% +18.27%
===========================================
Files 1035 556 -479
Lines 139075 70524 -68551
Branches 8631 4699 -3932
===========================================
- Hits 63613 45144 -18469
+ Misses 73735 23968 -49767
+ Partials 1727 1412 -315
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
router/core/graph_server.go (1)
1533-1551:⚠️ Potential issue | 🟠 MajorRegister the re-warm listener only after startup is committed.
There are still error exits after this block in
buildGraphMux, andnewGraphServercan fail later as well. On those pathsShutdown()never runs, soshutdownStartedis never closed and this callback stays attached to the sharedpqlStore, retaining the partially-built mux and firing on later manifest updates. Please register it only after server construction has succeeded, or keep an explicit unregister handle and release it on error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/core/graph_server.go` around lines 1533 - 1551, The AddListener callback on pqlStore is being attached too early in buildGraphMux (via pqlStore.AddListener using shutdownStarted and WarmupCaches) which can leak a partially-built mux if subsequent newGraphServer/build paths fail; either defer registering this listener until after server construction/startup is committed, or store and use an explicit unregister handle returned by AddListener and call that unregister in any error/early-exit paths (including failures in newGraphServer/buildGraphMux) so the listener is removed if construction fails.
🧹 Nitpick comments (1)
router/internal/persistedoperation/pqlmanifest/store_test.go (1)
109-124: Clean up the store worker in these subtests.
NewStorenow launchesrun()eagerly, so each temporary store in this file keeps one worker goroutine alive until process exit. Addingt.Cleanup(store.Close)next to eachNewStore(...)keeps the suite from accumulating stale workers.♻️ Suggested cleanup
store := NewStore(zap.NewNop()) + t.Cleanup(store.Close) done := make(chan struct{}) defer close(done)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/internal/persistedoperation/pqlmanifest/store_test.go` around lines 109 - 124, NewStore now starts a background worker via run(), so each test-created store must be closed to avoid leaked goroutines; after constructing a store with NewStore(...) in these subtests (e.g., the test using store := NewStore(zap.NewNop()) and AddListener/Load), register cleanup by calling t.Cleanup(store.Close) immediately after NewStore so the worker is stopped when the test finishes; locate uses of NewStore in this file (and any temporary stores) and add t.Cleanup(store.Close) next to them to ensure proper teardown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router/internal/persistedoperation/pqlmanifest/store.go`:
- Around line 42-80: The Store currently only prunes closed listeners inside the
run() worker when updateCh receives, so closed listener channels remain in
s.listeners until the next update; change AddListener to either return an
explicit unregister function or start a goroutine that watches the provided done
<-chan struct{} and removes that listener from s.listeners immediately under
s.mu when done is closed (use the existing listener type and mutate s.listeners
like run() does, pruning the matching pointer). Ensure the removal logic locks
s.mu, finds and removes the exact *listener entry (or marks it dead) to avoid
leaking mux/planner/executor state, and keep run()'s pruning as a fallback.
---
Outside diff comments:
In `@router/core/graph_server.go`:
- Around line 1533-1551: The AddListener callback on pqlStore is being attached
too early in buildGraphMux (via pqlStore.AddListener using shutdownStarted and
WarmupCaches) which can leak a partially-built mux if subsequent
newGraphServer/build paths fail; either defer registering this listener until
after server construction/startup is committed, or store and use an explicit
unregister handle returned by AddListener and call that unregister in any
error/early-exit paths (including failures in newGraphServer/buildGraphMux) so
the listener is removed if construction fails.
---
Nitpick comments:
In `@router/internal/persistedoperation/pqlmanifest/store_test.go`:
- Around line 109-124: NewStore now starts a background worker via run(), so
each test-created store must be closed to avoid leaked goroutines; after
constructing a store with NewStore(...) in these subtests (e.g., the test using
store := NewStore(zap.NewNop()) and AddListener/Load), register cleanup by
calling t.Cleanup(store.Close) immediately after NewStore so the worker is
stopped when the test finishes; locate uses of NewStore in this file (and any
temporary stores) and add t.Cleanup(store.Close) next to them to ensure proper
teardown.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ddf4f8d-cf49-4b98-9ff9-d2663f2e9cb2
📒 Files selected for processing (5)
router-tests/operations/pql_manifest_test.gorouter/core/graph_server.gorouter/internal/persistedoperation/pqlmanifest/storage_fetcher_test.gorouter/internal/persistedoperation/pqlmanifest/store.gorouter/internal/persistedoperation/pqlmanifest/store_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router/internal/persistedoperation/pqlmanifest/store.go`:
- Around line 66-69: The Close method on Store currently calls close(s.updateCh)
directly which can panic if Close is called more than once; make Close
idempotent by adding a sync.Once field (e.g., closeOnce) to the Store struct and
use closeOnce.Do to perform the close of updateCh (and any other shutdown logic)
so repeated Close() calls are safe; update references in the Store struct and
replace direct close(s.updateCh) in Store.Close with closeOnce.Do(func() {
close(s.updateCh) }).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a3d54129-0bf1-4868-ad51-497f71f30004
📒 Files selected for processing (3)
router/core/graph_server.gorouter/internal/persistedoperation/pqlmanifest/store.gorouter/internal/persistedoperation/pqlmanifest/store_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- router/internal/persistedoperation/pqlmanifest/store_test.go
- router/core/graph_server.go
…sisted operations
StarpTech
requested review from
a team,
JivusAyrus,
thisisnithin and
wilsonrivera
as code owners
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
… on feature flag enable/disable
StarpTech
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
controlplane/migrations/0137_peaceful_marauders.sql (1)
1-5: Consider adding an index onfeature_flag_idfor efficient FF-based lookups.The join table only has a composite primary key on
(persisted_operation_id, feature_flag_id). If the control plane queries operations by feature flag (e.g., when regenerating manifests after FF toggle/delete), lookups byfeature_flag_idalone will require a full table scan.📇 Proposed index addition
CREATE TABLE IF NOT EXISTS "persisted_operation_to_feature_flags" ( "persisted_operation_id" uuid NOT NULL, "feature_flag_id" uuid NOT NULL, CONSTRAINT "persisted_operation_to_feature_flags_persisted_operation_id_feature_flag_id_pk" PRIMARY KEY("persisted_operation_id","feature_flag_id") ); +--> statement-breakpoint +CREATE INDEX IF NOT EXISTS "po2ff_feature_flag_id_idx" ON "persisted_operation_to_feature_flags" ("feature_flag_id");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/migrations/0137_peaceful_marauders.sql` around lines 1 - 5, Add a non-unique index on feature_flag_id in the persisted_operation_to_feature_flags join table to speed up lookups filtered by feature_flag_id; specifically, add a CREATE INDEX (e.g., idx_persisted_operation_to_feature_flags_feature_flag_id) on the persisted_operation_to_feature_flags(feature_flag_id) to complement the existing composite primary key persisted_operation_to_feature_flags_persisted_operation_id_feature_flag_id_pk so queries that select by feature_flag_id do not require a full table scan.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controlplane/src/core/bufservices/feature-flag/deleteFeatureFlag.ts`:
- Around line 161-173: The current catch around
OperationsRepository.generateAndUploadManifest swallows failures so
deleteFeatureFlag (and the identical loop in enableFeatureFlag.ts) can return
success while manifests were not regenerated; change the handler to surface
errors instead: either enqueue a guaranteed retry job from the catch or
(simpler) collect any caught errors while iterating federatedGraphs and after
the loop throw a single aggregated error (including graph.id/name and the caught
error messages) so the deleteFeatureFlag flow fails and can be retried; update
the same logic in the enableFeatureFlag loop to match.
In `@controlplane/src/core/repositories/FederatedGraphRepository.ts`:
- Around line 963-987: The current getFeatureFlagSchemaVersions query returns
any composable composition row for a baseSchemaVersionId and can surface
old/failed FF compositions; change it to restrict to the latest successful
composition per featureFlagId by adding a condition that graphCompositions.id
(or graphCompositions.schemaVersionId if you prefer) equals the latest
composition id for that featureFlagId where isFeatureFlagComposition=true and
isComposable=true: implement this as a subquery or join that selects
MAX(graphCompositions.id) grouped by featureFlagId (or MAX(schemaVersionId) tied
to composedSchemaVersionId) from graphCompositions (or from
federatedGraphsToFeatureFlagSchemaVersions -> graphCompositions) and use that
value in the WHERE (or innerJoin) so getFeatureFlagSchemaVersions only returns
rows for the most-recent successful composition per featureFlagId (reference
symbols: getFeatureFlagSchemaVersions,
federatedGraphsToFeatureFlagSchemaVersions, graphCompositions, featureFlags,
schemaVersion).
In `@controlplane/src/core/repositories/OperationsRepository.ts`:
- Around line 92-107: The current logic in OperationsRepository that builds
ffInserts and only inserts new persistedOperationToFeatureFlags rows must
instead replace the association set for the affected persisted operations:
delete existing junction rows for the persistedOperationIds in inserted (use
drizzle's inArray helper) and then insert the new ffInserts so stale links are
removed; update the import to include inArray from drizzle and perform a delete
where persistedOperationId is inArray(insertedIds) before doing the insert (use
persistedOperationToFeatureFlags and persistedOperationId identifiers to locate
the table/column).
In `@docs-website/router/feature-flags.mdx`:
- Around line 74-76: Replace the em dash in the sentence "An operation targeting
one feature flag may be invalid on another — the router handles this
gracefully." with a period and split into two sentences so it matches docs
style; e.g. change to "An operation targeting one feature flag may be invalid on
another. The router handles this gracefully." Update the sentence in the Router
behavior section (the text containing "An operation targeting one feature flag
may be invalid on another") accordingly.
---
Nitpick comments:
In `@controlplane/migrations/0137_peaceful_marauders.sql`:
- Around line 1-5: Add a non-unique index on feature_flag_id in the
persisted_operation_to_feature_flags join table to speed up lookups filtered by
feature_flag_id; specifically, add a CREATE INDEX (e.g.,
idx_persisted_operation_to_feature_flags_feature_flag_id) on the
persisted_operation_to_feature_flags(feature_flag_id) to complement the existing
composite primary key
persisted_operation_to_feature_flags_persisted_operation_id_feature_flag_id_pk
so queries that select by feature_flag_id do not require a full table scan.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9bb85847-ae74-486f-b505-e1f5cbd1d4d5
📒 Files selected for processing (17)
controlplane/migrations/0137_peaceful_marauders.sqlcontrolplane/migrations/meta/0137_snapshot.jsoncontrolplane/migrations/meta/_journal.jsoncontrolplane/src/core/bufservices/feature-flag/deleteFeatureFlag.tscontrolplane/src/core/bufservices/feature-flag/enableFeatureFlag.tscontrolplane/src/core/bufservices/persisted-operation/publishPersistedOperations.tscontrolplane/src/core/repositories/FederatedGraphRepository.tscontrolplane/src/core/repositories/OperationsRepository.tscontrolplane/src/db/schema.tscontrolplane/src/types/index.tscontrolplane/test/persisted-operations.test.tsdocs-website/cli/feature-flags/disable-feature-flag.mdxdocs-website/cli/feature-flags/enable-feature-flag.mdxdocs-website/cli/operations/push.mdxdocs-website/router/feature-flags.mdxdocs-website/router/persisted-queries/persisted-operations.mdxrouter-tests/operations/pql_manifest_test.go
✅ Files skipped from review due to trivial changes (4)
- docs-website/cli/feature-flags/enable-feature-flag.mdx
- docs-website/cli/feature-flags/disable-feature-flag.mdx
- docs-website/router/persisted-queries/persisted-operations.mdx
- controlplane/migrations/meta/_journal.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controlplane/src/core/repositories/OperationsRepository.ts (1)
79-113:⚠️ Potential issue | 🔴 CriticalMake the upsert and FF-link replacement atomic.
These writes are one state transition, but they currently commit separately. If a manifest regeneration reads after Line 109 and before Line 113, an FF-only operation temporarily looks non-servable and can be omitted from the uploaded manifest. A failure on Line 113 leaves that bad state behind permanently.
🔒 Suggested fix
- const inserted = await this.db - .insert(federatedGraphPersistedOperations) - .values(inserts) - .onConflictDoUpdate({ - target: [ - federatedGraphPersistedOperations.federatedGraphId, - federatedGraphPersistedOperations.clientId, - federatedGraphPersistedOperations.operationId, - ], - set: { updatedAt: now, updatedById: userId, validOnBaseGraph: sql`excluded.valid_on_base_graph` }, - }) - .returning({ - id: federatedGraphPersistedOperations.id, - operationId: federatedGraphPersistedOperations.operationId, - }); - - // Replace feature flag validity links for the affected persisted operations - const insertedIds = inserted.map((row) => row.id); - const ffInserts: (typeof persistedOperationToFeatureFlags.$inferInsert)[] = []; - for (const row of inserted) { - const op = operations.find((o) => o.operationId === row.operationId); - if (op) { - for (const ffId of op.validOnFeatureFlagIds ?? []) { - ffInserts.push({ persistedOperationId: row.id, featureFlagId: ffId }); - } - } - } - // Delete stale links before inserting the current set - if (insertedIds.length > 0) { - await this.db - .delete(persistedOperationToFeatureFlags) - .where(inArray(persistedOperationToFeatureFlags.persistedOperationId, insertedIds)); - } - if (ffInserts.length > 0) { - await this.db.insert(persistedOperationToFeatureFlags).values(ffInserts).onConflictDoNothing(); - } + await this.db.transaction(async (tx) => { + const inserted = await tx + .insert(federatedGraphPersistedOperations) + .values(inserts) + .onConflictDoUpdate({ + target: [ + federatedGraphPersistedOperations.federatedGraphId, + federatedGraphPersistedOperations.clientId, + federatedGraphPersistedOperations.operationId, + ], + set: { + updatedAt: now, + updatedById: userId, + validOnBaseGraph: sql`excluded.valid_on_base_graph`, + }, + }) + .returning({ + id: federatedGraphPersistedOperations.id, + operationId: federatedGraphPersistedOperations.operationId, + }); + + const insertedIds = inserted.map((row) => row.id); + const ffInserts: (typeof persistedOperationToFeatureFlags.$inferInsert)[] = []; + for (const row of inserted) { + const op = operations.find((o) => o.operationId === row.operationId); + if (op) { + for (const ffId of op.validOnFeatureFlagIds ?? []) { + ffInserts.push({ persistedOperationId: row.id, featureFlagId: ffId }); + } + } + } + + if (insertedIds.length > 0) { + await tx + .delete(persistedOperationToFeatureFlags) + .where(inArray(persistedOperationToFeatureFlags.persistedOperationId, insertedIds)); + } + if (ffInserts.length > 0) { + await tx.insert(persistedOperationToFeatureFlags).values(ffInserts).onConflictDoNothing(); + } + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/repositories/OperationsRepository.ts` around lines 79 - 113, The upsert into federatedGraphPersistedOperations and the subsequent deletion/insertion of persistedOperationToFeatureFlags must be executed in a single atomic transaction to avoid transient inconsistent state; wrap the entire sequence (the this.db.insert(...) upsert that produces inserted, the delete on persistedOperationToFeatureFlags for insertedIds, and the insert of ffInserts) inside a single this.db.transaction (or equivalent transactional API), use the transaction handle (e.g., tx) for all DB calls instead of this.db so the returned inserted rows come from the same transaction, and commit/return after both the upsert and feature-flag link replacement succeed so partial failures cannot leave the DB in a bad state.
🧹 Nitpick comments (1)
controlplane/src/core/repositories/OperationsRepository.ts (1)
277-293: Add an explicit return type toisServableOperation.This new helper currently relies on inference, but the repo’s TypeScript guidelines require explicit return types on functions. Please annotate it with the Drizzle predicate type the repo uses here.
As per coding guidelines, "Use explicit type annotations for function parameters and return types in TypeScript".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/repositories/OperationsRepository.ts` around lines 277 - 293, Add an explicit return type to the helper isServableOperation: change its signature to declare the Drizzle predicate return type (e.g., isServableOperation(): SQL.Expression<boolean> { ... }) so the function no longer relies on inference; keep the existing body using or, eq, exists, and, this.db.select(...), persistedOperationToFeatureFlags, featureFlags and federatedGraphPersistedOperations unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs-website/router/feature-flags.mdx`:
- Around line 61-62: The sentence in the docs for the `wgc operations push` flow
is misleading about validation order; change the wording so it states that the
upload short-circuits feature-flag schema checks when an operation already
passes validation against the base graph schema, i.e., the control plane only
validates against feature flag composition schemas if base validation fails, and
an operation is accepted as soon as it validates on the base schema or any
subsequent FF schema.
---
Outside diff comments:
In `@controlplane/src/core/repositories/OperationsRepository.ts`:
- Around line 79-113: The upsert into federatedGraphPersistedOperations and the
subsequent deletion/insertion of persistedOperationToFeatureFlags must be
executed in a single atomic transaction to avoid transient inconsistent state;
wrap the entire sequence (the this.db.insert(...) upsert that produces inserted,
the delete on persistedOperationToFeatureFlags for insertedIds, and the insert
of ffInserts) inside a single this.db.transaction (or equivalent transactional
API), use the transaction handle (e.g., tx) for all DB calls instead of this.db
so the returned inserted rows come from the same transaction, and commit/return
after both the upsert and feature-flag link replacement succeed so partial
failures cannot leave the DB in a bad state.
---
Nitpick comments:
In `@controlplane/src/core/repositories/OperationsRepository.ts`:
- Around line 277-293: Add an explicit return type to the helper
isServableOperation: change its signature to declare the Drizzle predicate
return type (e.g., isServableOperation(): SQL.Expression<boolean> { ... }) so
the function no longer relies on inference; keep the existing body using or, eq,
exists, and, this.db.select(...), persistedOperationToFeatureFlags, featureFlags
and federatedGraphPersistedOperations unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cc817ed7-4874-4b8e-ad29-d422992f623e
📒 Files selected for processing (4)
controlplane/src/core/repositories/OperationsRepository.tscontrolplane/test/persisted-operations.test.tsdocs-website/cli/feature-flags/disable-feature-flag.mdxdocs-website/router/feature-flags.mdx
✅ Files skipped from review due to trivial changes (1)
- docs-website/cli/feature-flags/disable-feature-flag.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- controlplane/test/persisted-operations.test.ts
Comment on lines
+61
to
+62
| When you push persisted operations with `wgc operations push`, the control plane validates each operation against the base graph schema **and** all enabled feature flag composition schemas. An operation is accepted if it is valid on at least one schema. This allows you to persist operations that use fields introduced by a feature flag subgraph. | ||
|
|
Contributor
There was a problem hiding this comment.
Tighten this sentence to match the actual validation flow.
The upload path short-circuits FF schema lookup when an operation already passes base validation, so this currently reads as if every operation is always checked against every feature-flag schema.
✏️ Suggested wording
-When you push persisted operations with `wgc operations push`, the control plane validates each operation against the base graph schema **and** all enabled feature flag composition schemas. An operation is accepted if it is valid on at least one schema. This allows you to persist operations that use fields introduced by a feature flag subgraph.
+When you push persisted operations with `wgc operations push`, the control plane validates each operation against the base graph schema first. If an operation fails there, the control plane checks it against the enabled feature flag composition schemas. An operation is accepted if it is valid on at least one checked schema. This allows you to persist operations that use fields introduced by a feature flag subgraph.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| When you push persisted operations with `wgc operations push`, the control plane validates each operation against the base graph schema **and** all enabled feature flag composition schemas. An operation is accepted if it is valid on at least one schema. This allows you to persist operations that use fields introduced by a feature flag subgraph. | |
| When you push persisted operations with `wgc operations push`, the control plane validates each operation against the base graph schema first. If an operation fails there, the control plane checks it against the enabled feature flag composition schemas. An operation is accepted if it is valid on at least one checked schema. This allows you to persist operations that use fields introduced by a feature flag subgraph. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs-website/router/feature-flags.mdx` around lines 61 - 62, The sentence in
the docs for the `wgc operations push` flow is misleading about validation
order; change the wording so it states that the upload short-circuits
feature-flag schema checks when an operation already passes validation against
the base graph schema, i.e., the control plane only validates against feature
flag composition schemas if base validation fails, and an operation is accepted
as soon as it validates on the base schema or any subsequent FF schema.
JivusAyrus
requested changes
Apr 6, 2026
JivusAyrus
approved these changes
Apr 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Previously, persisted operations were validated only against the base federated graph schema at upload time, making feature flags and persisted operations incompatible for FF-specific queries. Operations using fields introduced by a feature flag subgraph would be rejected during upload.
This PR makes them work together end-to-end: the controlplane validates against all enabled FF schemas, tracks which schemas each operation is valid on, and dynamically filters the PQL manifest based on feature flag state. The router already handles per-mux schema validation gracefully, and now correctly re-warms all feature flag muxes when the manifest is updated.
Changes
Controlplane
Validation against feature flag schemas — when publishing persisted operations, the controlplane now validates each operation against the base graph schema first, then falls back to all enabled FF composition schemas. An operation is accepted if valid on at least one. If all operations pass base validation, FF schemas are never fetched (short-circuit).
Validity tracking — each operation is stored with a
validOnBaseGraphboolean and links in a newpersistedOperationToFeatureFlagsjunction table recording which FFs validated it.Manifest filtering — the PQL manifest now only includes servable operations: those valid on the base graph, or linked to at least one enabled FF. This is non-destructive — operations stay in the DB and blob storage regardless of FF state.
Manifest regeneration on FF state changes — toggling or deleting a feature flag now regenerates the PQL manifest for all affected federated graphs. FF-only operations appear/disappear automatically based on FF state.
Error messages — when an operation is rejected, the error lists which schemas were checked (base graph + FF names) and shows the base graph's validation error.
Router
Per-mux manifest re-warm — previously,
SetOnUpdateonly supported a single callback, so only the last-built mux (random feature flag) got its plan cache re-warmed on manifest update. Now each mux registers its own warmup function, and the graph server composes them into a singleSetOnUpdatecallback that runs all warmups sequentially. This provides natural backpressure — at most one warmup runs at a time.Per-mux schema validation — the manifest is shared across all muxes. Operations valid on a FF schema but invalid on the base schema are resolved from the manifest on both muxes, but per-mux schema validation rejects them on the base mux. Warmup skips invalid operations with a warning.
Config reload — on config reload, the new graph server registers a new composed callback via
SetOnUpdate, atomically replacing the old one. No listener lists or cleanup needed.Database
New column on
federated_graph_persisted_operations:valid_on_base_graph(boolean NOT NULL DEFAULT true) — backward compatible, existing operations default to base-validNew junction table
persisted_operation_to_feature_flags:persisted_operation_id→ FK cascade to operationsfeature_flag_id→ FK cascade to feature flagsBehavior Matrix
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.
Summary by CodeRabbit
New Features
Documentation