Conversation
StarpTech
requested review from
a team,
JivusAyrus,
thisisnithin and
wilsonrivera
as code owners
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Tinypool-based worker pool and worker entry for federated graph composition, new serialized worker types, wiring into server lifecycle and repository composition flow, and an env/config flag plus dependency to control pool max threads. (≤50 words) Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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. 📝 Coding Plan
Comment Tip You can validate your CodeRabbit configuration file in your editor.If your editor has YAML language server, you can enable auto-completion and validation by adding |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2637 +/- ##
==========================================
- Coverage 46.52% 46.42% -0.10%
==========================================
Files 1043 1046 +3
Lines 141356 141488 +132
Branches 9680 9679 -1
==========================================
- Hits 65759 65680 -79
- Misses 73886 74095 +209
- Partials 1711 1713 +2
🚀 New features to boost your workflow:
|
Router image scan passed✅ No security vulnerabilities found in image: |
![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: 2
🧹 Nitpick comments (1)
controlplane/src/core/composition/composeGraphs.types.ts (1)
14-18: Carry downstream contract IDs through the worker payloads.These types preserve only
contractName, so the main thread has to map results back to contracts by mutable name after the worker returns. Passing the downstream federated graph ID here would remove that extra lookup and avoid coupling the handoff to a rename-sensitive identifier.♻️ Suggested shape change
export interface SerializedContractTagOptions { + contractGraphId: string; contractName: string; excludeTags: string[]; includeTags: string[]; } @@ export interface SerializedContractCompositionArtifact { + contractGraphId: string; contractName: string; artifact: SerializedComposedGraphArtifact; }Also applies to: 44-47, 57-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/composition/composeGraphs.types.ts` around lines 14 - 18, The current SerializedContractTagOptions type only carries contractName which forces the main thread to re-map results by mutable names; update SerializedContractTagOptions (and the other similar types referenced in this diff) to include a stable downstream identifier field (e.g., downstreamFederatedGraphId or downstreamContractId) in addition to contractName so workers emit the immutable downstream graph/contract id in payloads; modify any places that construct or consume SerializedContractTagOptions/related types to populate and use this new id instead of relying on contractName for final mapping.
🤖 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/build-server.ts`:
- Around line 540-544: The shutdown currently calls worker.close() which, if it
rejects, prevents the subsequent destroyComposeGraphsPool() from running; update
the shutdown sequence around the worker.close() / BullMQ close logic so teardown
of the composition pool is unconditional—either wrap the close calls in
Promise.allSettled([...]) or put the close logic inside a try/finally and call
destroyComposeGraphsPool() in the finally block; ensure references to
worker.close() (the worker shutdown code) are guarded and
destroyComposeGraphsPool() is always invoked regardless of any rejection.
In `@controlplane/src/core/composition/composeGraphs.worker.ts`:
- Around line 237-240: The router config is being built with randomUUID() which
yields an incorrect schemaVersionId; instead pass the persisted
federatedSchemaVersionId into buildRouterExecutionConfig (replace the
randomUUID() argument with the federatedSchemaVersionId from the
task/saveComposition flow) so that buildRouterExecutionConfig (and the resulting
routerExecutionConfig) uses the same schemaVersionId persisted to the DB before
it is serialized and uploaded by
composeAndUploadRouterConfig/composeRouterConfigWithFeatureFlags; alternatively,
if you prefer minimal change, after deserializing routerExecutionConfig in
composeAndDeployGraphs() explicitly set routerExecutionConfig.schemaVersionId =
federatedSchemaVersionId before calling
saveComposition()/composeAndUploadRouterConfig().
---
Nitpick comments:
In `@controlplane/src/core/composition/composeGraphs.types.ts`:
- Around line 14-18: The current SerializedContractTagOptions type only carries
contractName which forces the main thread to re-map results by mutable names;
update SerializedContractTagOptions (and the other similar types referenced in
this diff) to include a stable downstream identifier field (e.g.,
downstreamFederatedGraphId or downstreamContractId) in addition to contractName
so workers emit the immutable downstream graph/contract id in payloads; modify
any places that construct or consume SerializedContractTagOptions/related types
to populate and use this new id instead of relying on contractName for final
mapping.
🪄 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: 349f055d-8d79-49e9-8efe-844cff9c93ed
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
controlplane/package.jsoncontrolplane/src/core/build-server.tscontrolplane/src/core/composition/composeGraphs.pool.tscontrolplane/src/core/composition/composeGraphs.types.tscontrolplane/src/core/composition/composeGraphs.worker.tscontrolplane/src/core/repositories/FederatedGraphRepository.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
controlplane/src/core/composition/composeGraphs.pool.ts (2)
25-35: Make worker resolution deterministic.This currently picks
composeGraphs.worker.tswhenever that file exists, even if the current module is the compiled.jsbuild. If both source and build artifacts are present, the main thread and worker can end up using different module flavors. Deriving the extension fromimport.meta.urlis safer than probing for sibling files.Suggested fix
-import { existsSync } from 'node:fs'; import { fileURLToPath } from 'node:url'; @@ function getWorkerFilename() { - const sourceWorker = new URL('composeGraphs.worker.ts', import.meta.url); - if (existsSync(fileURLToPath(sourceWorker))) { - return { - filename: sourceWorker.href, - }; - } - + const currentPath = fileURLToPath(import.meta.url); + const extension = currentPath.endsWith('.ts') ? 'ts' : 'js'; return { - filename: new URL('composeGraphs.worker.js', import.meta.url).href, + filename: new URL(`composeGraphs.worker.${extension}`, import.meta.url).href, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/composition/composeGraphs.pool.ts` around lines 25 - 35, The getWorkerFilename function is non-deterministically choosing composeGraphs.worker.ts when that file happens to exist; remove the filesystem probe and instead derive the worker module extension from the current module's import.meta.url so the main thread and worker use the same module flavor. Replace the existsSync/fileURLToPath logic in getWorkerFilename with code that reads the extension of import.meta.url (e.g., ".ts" or ".js") and returns a URL for composeGraphs.worker with that same extension (using new URL('composeGraphs.worker' + ext, import.meta.url).href), ensuring the function always selects the worker file matching the current module type.
43-58: Block new submissions once teardown starts.
destroyComposeGraphsPool()clears the singleton beforeawait pool.destroy()completes. AcomposeGraphsInWorker()call that lands during that window will create a brand-new pool and keep accepting work while shutdown is in progress. A small closing flag/promise would make teardown deterministic.Suggested fix
let composeGraphsPool: WorkerPool | undefined; +let composeGraphsPoolClosing: Promise<void> | undefined; @@ function getComposeGraphsPool() { + if (composeGraphsPoolClosing) { + throw new Error('composeGraphs worker pool is shutting down'); + } + if (composeGraphsPool) { return composeGraphsPool; } @@ export async function destroyComposeGraphsPool() { - if (!composeGraphsPool) { + if (!composeGraphsPool) { + if (composeGraphsPoolClosing) { + await composeGraphsPoolClosing; + } return; } const pool = composeGraphsPool; composeGraphsPool = undefined; - await pool.destroy(); + composeGraphsPoolClosing = pool.destroy().finally(() => { + composeGraphsPoolClosing = undefined; + }); + await composeGraphsPoolClosing; }Also applies to: 97-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/composition/composeGraphs.pool.ts` around lines 43 - 58, The teardown races because destroyComposeGraphsPool() clears the composeGraphsPool global before awaiting pool.destroy(), allowing getComposeGraphsPool() (and thus composeGraphsInWorker()) to create a new pool during shutdown; fix by adding a shutdown flag/promise (e.g., composeGraphsPoolShuttingDown or composeGraphsPoolDestroying) checked in getComposeGraphsPool() to block or throw while teardown is in progress and set that flag at the start of destroyComposeGraphsPool(), await pool.destroy(), then clear the flag and the composeGraphsPool; update any call sites (composeGraphsInWorker) to respect the flag/promise so new submissions are rejected or queued until teardown completes.
🤖 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/composition/composeGraphs.pool.ts`:
- Around line 69-86: The deserializer deserializeComposedGraphArtifact is
dropping fieldConfigurations by hardcoding it to [], so update the serialization
shape and rehydration: add fieldConfigurations to
SerializedComposedGraphArtifact (with the correct type used by
ComposedFederatedGraph), ensure the composer writes fieldConfigurations into the
serialized artifact, and then set fieldConfigurations:
artifact.fieldConfigurations (or map/deserialize each entry if they need
conversion) inside deserializeComposedGraphArtifact instead of []. Make sure the
type imports/usages align with the existing ComposedFederatedGraph and
ComposedSubgraph types and handle undefined by defaulting to an empty array only
if truly absent.
---
Nitpick comments:
In `@controlplane/src/core/composition/composeGraphs.pool.ts`:
- Around line 25-35: The getWorkerFilename function is non-deterministically
choosing composeGraphs.worker.ts when that file happens to exist; remove the
filesystem probe and instead derive the worker module extension from the current
module's import.meta.url so the main thread and worker use the same module
flavor. Replace the existsSync/fileURLToPath logic in getWorkerFilename with
code that reads the extension of import.meta.url (e.g., ".ts" or ".js") and
returns a URL for composeGraphs.worker with that same extension (using new
URL('composeGraphs.worker' + ext, import.meta.url).href), ensuring the function
always selects the worker file matching the current module type.
- Around line 43-58: The teardown races because destroyComposeGraphsPool()
clears the composeGraphsPool global before awaiting pool.destroy(), allowing
getComposeGraphsPool() (and thus composeGraphsInWorker()) to create a new pool
during shutdown; fix by adding a shutdown flag/promise (e.g.,
composeGraphsPoolShuttingDown or composeGraphsPoolDestroying) checked in
getComposeGraphsPool() to block or throw while teardown is in progress and set
that flag at the start of destroyComposeGraphsPool(), await pool.destroy(), then
clear the flag and the composeGraphsPool; update any call sites
(composeGraphsInWorker) to respect the flag/promise so new submissions are
rejected or queued until teardown completes.
🪄 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: ecf46c28-0948-4e1b-be94-6ee7dbedfe72
📒 Files selected for processing (1)
controlplane/src/core/composition/composeGraphs.pool.ts
StarpTech
commented
Mar 12, 2026
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 `@controlplane/src/core/composition/composeGraphs.worker.ts`:
- Around line 67-69: Fix the missing space in the error message building in
composeGraphs.worker.ts: when constructing the message that uses
ROUTER_COMPATIBILITY_VERSIONS, ensure there's a space between "Cosmo." and
"Please" (e.g., add a trailing space to the first sentence or a leading space to
the second) so the concatenated string reads "Cosmo. Please set..."; locate the
concatenation where `Router compatibility version ${version} is not supported by
Cosmo.` is joined with `Please set one of the following valid versions:\n ` and
add the space.
🪄 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: c3e2b0db-1bc5-4926-88e6-19109b0e54b7
📒 Files selected for processing (3)
controlplane/src/core/composition/composeGraphs.pool.tscontrolplane/src/core/composition/composeGraphs.types.tscontrolplane/src/core/composition/composeGraphs.worker.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
controlplane/src/core/composition/composeGraphs.worker.ts (1)
268-274: Consider defensive error handling for SDL parsing.The
parse(subgraph.schemaSDL)call can throw if the SDL is malformed. While published subgraphs should have valid SDL, consider whether this worker should handle parse errors gracefully or let them propagate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/composition/composeGraphs.worker.ts` around lines 268 - 274, The parse(subgraph.schemaSDL) call inside toCompositionSubgraphs can throw on malformed SDL; wrap the parse call in a try/catch inside the toCompositionSubgraphs function (or a small helper) to handle parse errors for each SubgraphDTO: catch the error, include identifying context (e.g., subgraph.name and/or subgraph.routingUrl), log or propagate a clearer Error with that context, and either skip/omit the failing subgraph or rethrow a formatted error depending on desired behaviour so the failure is explicit and traceable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@controlplane/src/core/composition/composeGraphs.worker.ts`:
- Around line 268-274: The parse(subgraph.schemaSDL) call inside
toCompositionSubgraphs can throw on malformed SDL; wrap the parse call in a
try/catch inside the toCompositionSubgraphs function (or a small helper) to
handle parse errors for each SubgraphDTO: catch the error, include identifying
context (e.g., subgraph.name and/or subgraph.routingUrl), log or propagate a
clearer Error with that context, and either skip/omit the failing subgraph or
rethrow a formatted error depending on desired behaviour so the failure is
explicit and traceable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8e2a29fc-054c-4529-b135-9d30e8b8156d
📒 Files selected for processing (2)
controlplane/src/core/composition/composeGraphs.worker.tscontrolplane/src/core/repositories/FederatedGraphRepository.ts
…tion config validation in FederatedGraphRepository
…oved type handling in composition
…-cosmo-controlplane
…-cosmo-controlplane
…andling in composeGraphs.worker
… in serializeComposedGraphArtifact
Aenimus
reviewed
Mar 12, 2026
Aenimus
reviewed
Mar 12, 2026
…-cosmo-controlplane
Aenimus
reviewed
Mar 12, 2026
Aenimus
reviewed
Mar 12, 2026
Aenimus
reviewed
Mar 12, 2026
Aenimus
reviewed
Mar 12, 2026
JivusAyrus
requested changes
Mar 13, 2026
…-cosmo-controlplane
JivusAyrus
approved these changes
Mar 13, 2026
…-cosmo-controlplane
…-cosmo-controlplane
…-cosmo-controlplane
StarpTech
deleted the
dustin/eng-8238-mitigate-event-loop-block-in-cosmo-controlplane
branch
This was referenced Mar 16, 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
3 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 by CodeRabbit
New Features
Refactor
Chores
Checklist