feat: controlpanel move to a accept first strategy for batch - #2963
Merged
wilsonrivera merged 30 commits intoJun 16, 2026
Merged
Conversation
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:
WalkthroughThis PR introduces asynchronous batch publish operations via optional request flag and response job tracking. It adds database schema for job persistence and namespace-level locking, implements a cleanup worker to delete completed jobs, refactors the main handler to support both sync and async execution paths with a shared batch helper, and wires the worker infrastructure into the server bootstrap while correcting logging field names across workers. ChangesAsync Batch Publish Core
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (76.80%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2963 +/- ##
==========================================
+ Coverage 46.83% 48.02% +1.18%
==========================================
Files 1115 1139 +24
Lines 151058 156048 +4990
Branches 9883 10931 +1048
==========================================
+ Hits 70755 74935 +4180
- Misses 78501 79275 +774
- Partials 1802 1838 +36
🚀 New features to boost your workflow:
|
Router-nonroot 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: 6
🧹 Nitpick comments (2)
controlplane/src/core/repositories/BatchPublishJobDetailsRepository.ts (1)
85-88: ⚡ Quick winConsider logging lock deletion errors for observability.
The
.catch(() => {})silently swallows all errors during lock cleanup. While this correctly prevents cleanup failures from affecting the function's result, it hides potential database issues that could indicate problems with the locking infrastructure.📊 Suggested improvement
await this.db .delete(schema.batchPublishJobDetailsJobLocks) .where(eq(schema.batchPublishJobDetailsJobLocks.id, lock)) - .catch(() => {}); + .catch((err) => { + // Log but don't throw - cleanup failure shouldn't affect the function result + console.error('Failed to delete lock after job completion:', err); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controlplane/src/core/repositories/BatchPublishJobDetailsRepository.ts` around lines 85 - 88, The silent .catch(() => {}) on the delete of schema.batchPublishJobDetailsJobLocks in BatchPublishJobDetailsRepository hides DB errors; change it to catch the error and log it (including the lock id) without rethrowing so cleanup can't break flow—e.g., in the method containing the await this.db.delete(...).where(eq(schema.batchPublishJobDetailsJobLocks.id, lock)), replace the empty catch with catch(err => (this.logger ?? console).error(`Failed to delete lock ${lock}`, err)) or use the repository's existing logger instance to record the error and context.controlplane/src/core/bufservices/subgraph/publishFederatedSubgraphs.ts (1)
360-367: ⚡ Quick winAlign new TS shapes/signatures with repository typing guidelines.
Please switch
RunBatchPublishParamsto aninterfaceand add an explicit return type forrunBatchPublish.As per coding guidelines,
**/*.{ts,tsx}prefers interfaces for object shapes and requires explicit function parameter/return type annotations.Also applies to: 369-376
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controlplane/src/core/bufservices/subgraph/publishFederatedSubgraphs.ts` around lines 360 - 367, Change the exported type alias RunBatchPublishParams to an interface (interface RunBatchPublishParams { ... }) and update any other nearby object-shaped type aliases in the same area (the similar shape at lines 369-376) to interfaces as well; then add an explicit return type annotation to the runBatchPublish function signature (e.g., runBatchPublish(...): Promise<YourReturnType> or appropriate synchronous return type) so both parameter shapes and function return types follow the repository typing guidelines. Ensure you keep the same property names (opts, logger, authContext, disableResolvabilityValidation, items, shouldRefreshSubgraphs) and reference the existing UpdateSubgraphSchemaData and RouterOptions types when declaring the interface.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controlplane/migrations/0142_hot_sphinx.sql`:
- Around line 12-19: The lock table batch_publish_job_details_job_locks allows
namespace_id, job_id and organization_id to disagree; fix by enforcing same-org
constraints: either drop organization_id and derive org via joins from
namespace_id/job_id, or add composite foreign keys that bind organization_id to
the referenced rows (e.g., add FK (organization_id, namespace_id) referencing
the namespaces table (organization_id,id) and FK (organization_id, job_id)
referencing the jobs table (organization_id,id)), and remove any independent FKs
that permit cross-org rows; update or add constraint names similar to
batch_publish_job_details_job_locks_namespace_id_key so namespace_id and job_id
cannot point to a different organization than organization_id.
In `@controlplane/src/core/bufservices/subgraph/publishFederatedSubgraphs.ts`:
- Around line 277-315: Sentry.startSpan in
publishFederatedSubgraphs.runBatchPublish is invoked fire-and-forget but returns
the Promise from the async callback, so attach error handling to that returned
Promise instead of ignoring it; update the call site (the Sentry.startSpan(...)
invocation) to either await the returned Promise or append .catch(...) that
captures the error (Sentry.captureException) and updates
batchPublishJobDetailsRepo.update(jobId, { status: 'failed', failureReason: ...
}) for any unhandled rejections, and ensure Sentry.flush is still invoked in all
paths; reference the existing symbols Sentry.startSpan, runBatchPublish, and
batchPublishJobDetailsRepo.update to locate where to add the .catch or await.
In `@controlplane/src/core/workers/DeleteBatchPublishJobDetailsWorker.ts`:
- Around line 101-103: In the worker.on('stalled', (job) => ...) handler replace
the incorrect log field name joinId with jobId in the log.warn call; update the
call to log.warn({ jobId: job }) (or log.warn({ jobId: job.id }) if the job
object exposes an id property) so the stalled-job logs use the consistent jobId
field for observability and searchability.
- Around line 73-83: The handler in DeleteBatchPublishJobDetailsWorker currently
logs errors but swallows them, preventing BullMQ retries; update the handler in
DeleteBatchPublishJobDetailsWorker.handler so that after catching and logging
the error (use this.input.logger.error with
jobId/organizationId/batchPublishJobDetailsId/err) it rethrows the caught error
(throw err) to allow BullMQ to apply defaultJobOptions retries and backoff;
ensure you reference the BatchPublishJobDetailsRepository.delete call and
preserve the existing log before rethrowing.
- Around line 62-71: The constructor currently mutates the shared input by
setting this.input.logger = input.logger.child({ worker: WorkerName }) and the
caller creates a new DeleteBatchPublishJobDetailsWorker per job; instead, modify
DeleteBatchPublishJobDetailsWorker to store the shared input unchanged and add a
private instance field (e.g., this.logger) that is set to input.logger.child({
worker: WorkerName, jobId: ??? }) inside the constructor (without writing back
to input), then update createDeleteBatchPublishJobDetailsWorker to instantiate a
single DeleteBatchPublishJobDetailsWorker once and expose a bound handler (e.g.,
worker.handler.bind(worker)) to be reused for all jobs; ensure references to
input.logger, DeleteBatchPublishJobDetailsWorker,
createDeleteBatchPublishJobDetailsWorker, handler, and WorkerName are used to
find and update the code.
In `@package.json`:
- Line 93: The package.json override for "msgpackr" appears incorrect; verify
BullMQ's recommended fix (msgpackr@1.1.2) and update the "msgpackr" entry in
package.json (the overrides/block where "msgpackr" is set) to the correct
patched version or to a safe later major (e.g., 2.0.4) only if you confirm
compatibility with BullMQ; after changing the version run npm audit / your SCA
scanner and add the audit results to the PR so we document any vulnerabilities
found.
---
Nitpick comments:
In `@controlplane/src/core/bufservices/subgraph/publishFederatedSubgraphs.ts`:
- Around line 360-367: Change the exported type alias RunBatchPublishParams to
an interface (interface RunBatchPublishParams { ... }) and update any other
nearby object-shaped type aliases in the same area (the similar shape at lines
369-376) to interfaces as well; then add an explicit return type annotation to
the runBatchPublish function signature (e.g., runBatchPublish(...):
Promise<YourReturnType> or appropriate synchronous return type) so both
parameter shapes and function return types follow the repository typing
guidelines. Ensure you keep the same property names (opts, logger, authContext,
disableResolvabilityValidation, items, shouldRefreshSubgraphs) and reference the
existing UpdateSubgraphSchemaData and RouterOptions types when declaring the
interface.
In `@controlplane/src/core/repositories/BatchPublishJobDetailsRepository.ts`:
- Around line 85-88: The silent .catch(() => {}) on the delete of
schema.batchPublishJobDetailsJobLocks in BatchPublishJobDetailsRepository hides
DB errors; change it to catch the error and log it (including the lock id)
without rethrowing so cleanup can't break flow—e.g., in the method containing
the await this.db.delete(...).where(eq(schema.batchPublishJobDetailsJobLocks.id,
lock)), replace the empty catch with catch(err => (this.logger ??
console).error(`Failed to delete lock ${lock}`, err)) or use the repository's
existing logger instance to record the error and context.
🪄 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: 69575bd1-4e53-401c-bdc1-f2fe916a8aa0
⛔ Files ignored due to path filters (2)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
connect/src/wg/cosmo/platform/v1/platform_pb.tscontrolplane/migrations/0142_hot_sphinx.sqlcontrolplane/migrations/meta/0142_snapshot.jsoncontrolplane/migrations/meta/_journal.jsoncontrolplane/src/core/bufservices/subgraph/publishFederatedSubgraphs.tscontrolplane/src/core/build-server.tscontrolplane/src/core/repositories/BatchPublishJobDetailsRepository.tscontrolplane/src/core/repositories/SubgraphRepository.tscontrolplane/src/core/routes.tscontrolplane/src/core/workers/DeleteBatchPublishJobDetailsWorker.tscontrolplane/src/db/models.tscontrolplane/src/db/schema.tspackage.jsonproto/wg/cosmo/platform/v1/platform.proto
wilsonrivera
changed the title
wilsonrivera
changed the title
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controlplane/test/test-util.ts (1)
160-191:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExpose the new queue in
SetupTestreturn to complete the test contract.
deleteBatchPublishJobDetailsQueueis instantiated (Line 160) and passed to routes (Line 191), but it is not returned inSetupTest().queues(Line 417 onward). This leaves tests unable to access/control the new queue even though this layer’s goal is to expose it in test setup.Proposed fix
return { client: platformClient, nodeClient, server, users, blobStorage, baseAddress: addr, keycloakClient, authenticator, mailerClient, realm, queues: { readmeQueue, deleteOrganizationQueue, deleteOrganizationAuditLogsQueue, deactivateOrganizationQueue, deleteUserQueue, reactivateOrganizationQueue, + deleteBatchPublishJobDetailsQueue, }, };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controlplane/test/test-util.ts` around lines 160 - 191, The new queue instance deleteBatchPublishJobDetailsQueue is created and wired into routes but not exposed in the SetupTest return value, so update the SetupTest() return to include deleteBatchPublishJobDetailsQueue in the queues object (alongside readmeQueue, deleteOrganizationQueue, etc.); if there is a Queues type/interface used in the return, add deleteBatchPublishJobDetailsQueue to that type as well so tests can access and control the queue.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@controlplane/test/test-util.ts`:
- Around line 160-191: The new queue instance deleteBatchPublishJobDetailsQueue
is created and wired into routes but not exposed in the SetupTest return value,
so update the SetupTest() return to include deleteBatchPublishJobDetailsQueue in
the queues object (alongside readmeQueue, deleteOrganizationQueue, etc.); if
there is a Queues type/interface used in the return, add
deleteBatchPublishJobDetailsQueue to that type as well so tests can access and
control the queue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a97937b8-8a0f-407b-b6f3-ffae8da57bfa
📒 Files selected for processing (1)
controlplane/test/test-util.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controlplane/src/core/bufservices/subgraph/getBatchPublishJobStatus.ts`:
- Around line 38-56: The code initializes jobStatus to
BatchPublishJobStatus.PENDING and then switches on jobDetails?.status, which
leaves PENDING when jobDetails is undefined; change getBatchPublishJobStatus so
it first checks if jobDetails is falsy and sets jobStatus to a non-misleading
value (e.g., BatchPublishJobStatus.UNKNOWN or a NOT_FOUND/UNAVAILABLE enum
entry) before the switch, or remove the PENDING default and only set
PENDING/PROCESSING/FAILED/COMPLETED inside the switch; update any callers/return
paths to use that UNKNOWN/NOT_FOUND status to avoid showing a non-existent job
as PENDING.
🪄 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: 5650a2df-9d87-4cc5-ade4-0ac4734b1aec
⛔ Files ignored due to path filters (2)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**connect-go/gen/proto/wg/cosmo/platform/v1/platformv1connect/platform.connect.gois excluded by!**/gen/**
📒 Files selected for processing (6)
connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.tsconnect/src/wg/cosmo/platform/v1/platform_connect.tsconnect/src/wg/cosmo/platform/v1/platform_pb.tscontrolplane/src/core/bufservices/PlatformService.tscontrolplane/src/core/bufservices/subgraph/getBatchPublishJobStatus.tsproto/wg/cosmo/platform/v1/platform.proto
✅ Files skipped from review due to trivial changes (2)
- connect/src/wg/cosmo/platform/v1/platform_connect.ts
- connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts
wilsonrivera
marked this pull request as ready for review
June 12, 2026 10:00
…t-first-strategy-for-batch # Conflicts: # connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go # connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts # connect/src/wg/cosmo/platform/v1/platform_connect.ts # controlplane/test/test-util.ts
comatory
reviewed
Jun 15, 2026
comatory
left a comment
comatory
left a comment
Contributor
There was a problem hiding this comment.
I did a quick pass on this, mostly not sure about the new dependency.
I think it'd be better to do synchronous review on this one.
…t-first-strategy-for-batch
endigma
approved these changes
Jun 15, 2026
…t-first-strategy-for-batch
…t-first-strategy-for-batch # Conflicts: # controlplane/migrations/meta/0142_snapshot.json # controlplane/migrations/meta/_journal.json # controlplane/package.json # controlplane/src/core/build-server.ts # pnpm-lock.yaml
comatory
requested changes
Jun 16, 2026
comatory
left a comment
comatory
left a comment
Contributor
There was a problem hiding this comment.
Just few observations, otherwise looks fine to me 👍
comatory
approved these changes
Jun 16, 2026
wilsonrivera
deleted the
wilson/eng-9724-controlpanel-move-to-a-accept-first-strategy-for-batch
branch
June 16, 2026 19:23
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
Enhancements
Reliability
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.