refactor: move upsert release targets to selector utility #492
Conversation
|
Caution Review failedThe pull request is closed. """ WalkthroughThis change removes the legacy Changes
Sequence Diagram(s)sequenceDiagram
participant Worker
participant ComputeBuilder
participant ResourceBuilder
participant DB
participant PolicyBuilder
Worker->>ComputeBuilder: .resources([resourceId])
ComputeBuilder->>ResourceBuilder: new ResourceBuilder(tx, [resourceId])
ResourceBuilder->>DB: Validate resources in one workspace
ResourceBuilder->>ResourceBuilder: _preHook()
ResourceBuilder->>DB: Delete previous release targets for resources
ResourceBuilder->>DB: Query environments, deployments, selectors
ResourceBuilder->>DB: Insert new release targets
ResourceBuilder->>ResourceBuilder: _postHook()
ResourceBuilder->>PolicyBuilder: Trigger policy selector recomputation
ResourceBuilder-->>Worker: Return new release targets
Worker->>DB: Enqueue evaluation jobs for release targets
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (2)
packages/job-dispatch/src/resource/handle-provider-scan.ts (1)
47-48: 💡 Verification agent🧩 Analysis chain
Verify that event triggers maintain the same behavior.
The code now relies on the event queue system (NewResource and UpdatedResource channels) to trigger subsequent processing. Ensure that the ResourceBuilder is invoked appropriately when these events are processed.
🏁 Script executed:
#!/bin/bash # Let's check how NewResource and UpdatedResource events are handled # Look for event handlers for NewResource channel echo "Checking NewResource event handlers:" rg -A 5 "Channel.NewResource" --glob "apps/event-worker/src/**/*.ts" # Look for event handlers for UpdatedResource channel echo "Checking UpdatedResource event handlers:" rg -A 5 "Channel.UpdatedResource" --glob "apps/event-worker/src/**/*.ts" # Check if these handlers use the ResourceBuilder echo "Checking for ResourceBuilder usage in event handlers:" rg -A 5 "ResourceBuilder" --glob "apps/event-worker/src/**/*.ts"Length of output: 2293
Ensure ResourceBuilder is invoked when processing queued events
The handlers for Channel.NewResource and Channel.UpdatedResource currently process incoming resources but don’t call ResourceBuilder, which may break existing build logic. After you fetch or compute the resource, import and invoke ResourceBuilder.build(resource) (or the appropriate method) in both workers.
Files to update:
- apps/event-worker/src/workers/new-resource.ts
- apps/event-worker/src/workers/updated-resources/index.tsExample diff in each worker:
import { createWorker } from "../lib/worker"; +import { ResourceBuilder } from "packages/resource-builder"; export const newResourceWorker = createWorker( Channel.NewResource, async ({ data: resource }) => { + await ResourceBuilder.build(resource); const releaseTargets = await selector() .compute() .resources([resource.id]) .releaseTargets(); …Make the analogous change in updatedResourceWorker so that every new or updated resource triggers ResourceBuilder.
apps/webservice/src/app/api/v1/resources/route.ts (1)
85-86: 💡 Verification agent🧩 Analysis chain
Verify that event triggers maintain the same behavior.
The code now relies on the event queue system (NewResource and UpdatedResource channels) to trigger subsequent processing. Ensure that the ResourceBuilder is invoked appropriately when these events are processed.
🏁 Script executed:
#!/bin/bash # Let's check the connection between these queue events and the ResourceBuilder # First, let's verify how these events are processed echo "Checking queue processors for NewResource and UpdatedResource:" rg "Channel\.(NewResource|UpdatedResource)" --glob "apps/event-worker/src/workers/**/*.ts" # Then, check if the ResourceBuilder is used in the processing echo "Checking ResourceBuilder usage in related workers:" rg "ResourceBuilder" --glob "apps/event-worker/src/workers/**/*.ts" # Let's also check for any potentially missed selector.resources() calls echo "Checking for selector.resources() usage:" rg "selector.*resources\(" --glob "apps/**/*.ts" --glob "packages/**/*.ts"Length of output: 1020
Ensure ResourceBuilder is invoked in your queue processors
It looks like you’ve wired up the
NewResourceandUpdatedResourcechannels tonewResourceWorkerandupdatedResourceWorker(inapps/event-worker/src/workers/index.ts), but neither handler actually calls theResourceBuilder. To preserve your existing post‐insert/update logic, please integrate the builder in both places:• apps/event-worker/src/workers/new-resource.ts
• apps/event-worker/src/workers/updated-resources/index.tsFor example, at the end of each handler:
import { /* … */ } from '...'; + import { ResourceBuilder } from 'path/to/ResourceBuilder'; export const newResourceWorker = async (job) => { // existing logic… + await ResourceBuilder.build(job.data); };And similarly in the updated‐resources worker. Don’t forget to add or update tests to cover these flows.
🧹 Nitpick comments (13)
apps/event-worker/src/workers/update-resource-variable.ts (1)
41-49: Consider running the read & recompute in the same transaction
db.query.resource.findFirstand the subsequentselector().compute()call run in two separate implicit transactions.
Between these calls another worker could mutate release targets, resulting in a stalecurrentDeploymentslist and therefore missed or spurious exit‑hooks. Executing both steps inside an explicit transaction (and passing the sametxtoselector()) would guarantee consistency.packages/db/src/schema/environment-relations.ts (1)
24-25: Explicitly declare the join fields forcomputedResourcesFor many‑to‑one relations Drizzle can usually infer the FK, but being explicit avoids silent breakage when the FK or PK changes:
- computedResources: many(computedEnvironmentResource), + computedResources: many(computedEnvironmentResource, { + fields: [environment.id], + references: [computedEnvironmentResource.environmentId], + }),It also improves IntelliSense & type‑safety.
apps/event-worker/src/workers/new-deployment.ts (2)
34-37: Guard against empty resource listIf
resourceIdsends up empty (e.g., no computed resources yet),.resources([])may throw or generate a no‑op SQL that still triggers hooks. Early‑returning here avoids pointless work and queue noise.if (resourceIds.length === 0) { log.debug("No resources to upsert release targets for"); return; }
34-41: Pass a transaction to keep reads & writes consistentSimilar to the
update‑resource‑variableworker, consider wrapping:
- the
environment.findManyread, and- the
selector().compute().resources(...).releaseTargets().replace()mutationin a single transaction (
db.transaction()), then supply thetxtoselector(tx).
This prevents race conditions where other workers alter selectors between the read and the write.packages/db/src/selectors/compute/deployment-builder.ts (2)
19-23: Guard against emptyidsto avoid invalidIN ()SQLIf
this.idsis empty the generatedWHERE IN ()clause can throw in some SQL engines.
A quick early‑return prevents an unnecessary round‑trip.-private async _deletePrevious(tx: Tx) { - await tx - .delete(SCHEMA.computedDeploymentResource) - .where(inArray(SCHEMA.computedDeploymentResource.deploymentId, this.ids)); +private async _deletePrevious(tx: Tx) { + if (this.ids.length === 0) return; + await tx + .delete(SCHEMA.computedDeploymentResource) + .where(inArray(SCHEMA.computedDeploymentResource.deploymentId, this.ids)); }
31-50: N + 1 query pattern—consider batching for scalability
_valuesperforms oneSELECT … FROM resourceper deployment.
In a workspace with hundreds of deployments this becomes an N + 1 pattern and slows down the replace cycle.Options:
- Build one combined query that evaluates all selectors in SQL (preferred, but complex).
- Chunk
deploymentsand execute withPromise.allSettled+ concurrency limit to avoid flooding the pool.- Cache selector → resource set mappings inside the loop.
Even a modest optimisation (e.g.
p‑mapwith concurrency 10) would reduce DB stress noticeably.apps/event-worker/src/workers/update-environment.ts (1)
74-81: Duplicate exit‑hook events
exitedResourcesmay contain duplicates becausecurrentResourcesmirrorscurrentReleaseTargetsone‑to‑one.
As with the resource worker, deduplicate before dispatching to avoid event storms.-const exitedResources = currentResources.filter(…); +const exitedResources = _.uniqBy( + currentResources.filter(…), + "id", +);packages/db/src/selectors/compute/resource-builder.ts (3)
10-17: Avoid defaulting to an empty workspaceId – useundefinedand assert later
""is a valid (albeit unusual) string value; using it to represent “not‑initialised” can mask accidental bugs during debugging or type‑narrowing. Preferundefinedor make the fieldprivate workspaceId!: string;and rely on the type‑checker to ensure it’s always set by_preHook.- private workspaceId: string; + private workspaceId!: string; ... - this.workspaceId = "";
38-42: Possible N+1 delete & missed deduplication
DELETE … WHERE id IN (<array>)is fine, but if the caller accidentally provides duplicates you still only issue one statement. However the query planner may build a very largeIN (…)list.
UseArray.from(new Set(this.resourceIds))to deduplicate, or switch toDELETE FROM … WHERE resource_id = ANY($1::uuid[]).
94-97: Policy recomputation may become a hotspotTriggering a full workspace policy recomputation (
policyComputer.releaseTargetSelectors().replace()) for every resource‑level change can lead to quadratic behaviour in busy workspaces.
Consider debouncing via a job queue or at least limiting the recomputation to policies that reference one ofthis.resourceIds.apps/event-worker/src/workers/update-deployment.ts (1)
60-61: Dispatching exit hooks outside a DB transaction may violate ordering guarantees
dispatchExitHooksfires after the replacement transaction has already committed (good), but there is still a window where the worker crashes after committing but before dispatching, causing hooks to be dropped.Consider publishing the exit‑hook events inside the same transaction to a durable outbox table and letting a separate relay push them to the event bus (transactional outbox pattern).
packages/db/src/selectors/compute/policy-builder.ts (2)
16-28: Inefficient deletion strategyYou first fetch
targetsonly to delete rows whosepolicyTargetIdmatches.
Postgres supportsDELETE … USING:DELETE FROM computed_policy_target_release_target c USING policy_target p WHERE p.policy_id = ANY($1) AND c.policy_target_id = p.id;which avoids the round‑trip and the large
IN (…)list.
Consider porting this pattern with Drizzle’ssql.raw.
117-161: Potentially heavy per‑target queries – batch insteadFor each policy target you run an independent three‑way join over
releaseTarget×resource×deployment×environment.
On large workspaces this becomes O(n × joins). You can:
- Build a single
SELECT … WHERE policy_target_id IN (…)with OR‑combined selector predicates.- Or at least run the inner loop in batches instead of
Promise.all(targets.map(...)).This will materially reduce round‑trips and planning time.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
apps/event-worker/src/utils/replace-release-targets.ts(0 hunks)apps/event-worker/src/workers/new-deployment.ts(2 hunks)apps/event-worker/src/workers/new-resource.ts(1 hunks)apps/event-worker/src/workers/update-deployment.ts(3 hunks)apps/event-worker/src/workers/update-environment.ts(2 hunks)apps/event-worker/src/workers/update-resource-variable.ts(2 hunks)apps/event-worker/src/workers/updated-resources/dispatch-exit-hooks.ts(0 hunks)apps/event-worker/src/workers/updated-resources/index.ts(2 hunks)apps/pty-proxy/src/controller/agent-socket.ts(1 hunks)apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts(1 hunks)apps/webservice/src/app/api/v1/resources/route.ts(1 hunks)packages/db/src/schema/deployment.ts(3 hunks)packages/db/src/schema/environment-relations.ts(3 hunks)packages/db/src/selectors/compute/compute.ts(2 hunks)packages/db/src/selectors/compute/deployment-builder.ts(2 hunks)packages/db/src/selectors/compute/environment-builder.ts(2 hunks)packages/db/src/selectors/compute/policy-builder.ts(2 hunks)packages/db/src/selectors/compute/replace-builder.ts(1 hunks)packages/db/src/selectors/compute/resource-builder.ts(1 hunks)packages/job-dispatch/src/resource/handle-provider-scan.ts(1 hunks)packages/job-dispatch/src/resource/index.ts(0 hunks)packages/job-dispatch/src/resource/replace-release-targets.ts(0 hunks)
💤 Files with no reviewable changes (4)
- packages/job-dispatch/src/resource/index.ts
- packages/job-dispatch/src/resource/replace-release-targets.ts
- apps/event-worker/src/workers/updated-resources/dispatch-exit-hooks.ts
- apps/event-worker/src/utils/replace-release-targets.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/db/src/selectors/compute/replace-builder.tspackages/db/src/selectors/compute/compute.tsapps/webservice/src/app/api/v1/resources/route.tsapps/pty-proxy/src/controller/agent-socket.tsapps/event-worker/src/workers/new-resource.tspackages/job-dispatch/src/resource/handle-provider-scan.tspackages/db/src/schema/deployment.tsapps/event-worker/src/workers/new-deployment.tspackages/db/src/schema/environment-relations.tsapps/event-worker/src/workers/update-resource-variable.tsapps/event-worker/src/workers/updated-resources/index.tspackages/db/src/selectors/compute/resource-builder.tspackages/db/src/selectors/compute/policy-builder.tspackages/db/src/selectors/compute/deployment-builder.tsapps/event-worker/src/workers/update-environment.tsapps/webservice/src/app/api/v1/resources/[resourceId]/route.tspackages/db/src/selectors/compute/environment-builder.tsapps/event-worker/src/workers/update-deployment.ts
🧬 Code Graph Analysis (11)
packages/db/src/selectors/compute/replace-builder.ts (1)
packages/db/src/common.ts (1)
Tx(22-22)
packages/db/src/selectors/compute/compute.ts (1)
packages/db/src/selectors/compute/resource-builder.ts (1)
ResourceBuilder(10-109)
apps/event-worker/src/workers/new-resource.ts (3)
packages/db/src/selectors/compute/resource-builder.ts (1)
releaseTargets(99-108)packages/db/src/selectors/index.ts (1)
selector(18-18)packages/db/src/schema/resource.ts (1)
resource(59-87)
packages/db/src/schema/deployment.ts (1)
packages/db/src/schema/resource.ts (1)
resource(59-87)
apps/event-worker/src/workers/new-deployment.ts (5)
packages/db/src/selectors/compute/compute.ts (1)
environments(20-22)packages/db/src/selectors/query/builder.ts (1)
environments(28-33)packages/db/src/client.ts (1)
db(15-15)packages/db/src/selectors/compute/resource-builder.ts (1)
releaseTargets(99-108)packages/db/src/selectors/index.ts (1)
selector(18-18)
packages/db/src/selectors/compute/resource-builder.ts (6)
packages/db/src/common.ts (1)
Tx(22-22)packages/db/src/selectors/compute/compute.ts (1)
resources(40-42)packages/db/src/selectors/compute/environment-builder.ts (1)
WorkspaceEnvironmentBuilder(85-139)packages/db/src/selectors/compute/deployment-builder.ts (1)
WorkspaceDeploymentBuilder(83-137)packages/db/src/selectors/compute/policy-builder.ts (1)
WorkspacePolicyBuilder(91-175)packages/db/src/selectors/compute/replace-builder.ts (1)
ReplaceBuilder(6-31)
packages/db/src/selectors/compute/policy-builder.ts (4)
packages/db/src/common.ts (1)
Tx(22-22)packages/db/src/selectors/query/builder.ts (1)
QueryBuilder(19-45)packages/db/src/selectors/compute/resource-builder.ts (1)
releaseTargets(99-108)packages/db/src/selectors/compute/replace-builder.ts (1)
ReplaceBuilder(6-31)
packages/db/src/selectors/compute/deployment-builder.ts (4)
packages/db/src/common.ts (1)
Tx(22-22)packages/db/src/selectors/compute/compute.ts (2)
deployments(28-30)resources(40-42)packages/db/src/selectors/query/builder.ts (2)
deployments(21-26)resources(35-40)packages/db/src/selectors/compute/replace-builder.ts (1)
ReplaceBuilder(6-31)
apps/event-worker/src/workers/update-environment.ts (4)
packages/db/src/client.ts (1)
db(15-15)packages/db/src/schema/environment.ts (1)
environment(59-84)packages/db/src/selectors/index.ts (1)
selector(18-18)packages/events/src/index.ts (1)
getQueue(28-34)
packages/db/src/selectors/compute/environment-builder.ts (3)
packages/db/src/common.ts (1)
Tx(22-22)packages/db/src/selectors/query/builder.ts (1)
resources(35-40)packages/db/src/selectors/compute/replace-builder.ts (1)
ReplaceBuilder(6-31)
apps/event-worker/src/workers/update-deployment.ts (3)
packages/db/src/client.ts (1)
db(15-15)packages/db/src/selectors/index.ts (1)
selector(18-18)packages/events/src/index.ts (1)
getQueue(28-34)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (25)
packages/db/src/selectors/compute/compute.ts (2)
11-11: LGTM: New import for ResourceBuilder from a relative path.The new import correctly references the resource-builder.js file that contains the ResourceBuilder class.
39-42: LGTM: Added new method for resource-specific computations.The new
resources(ids: string[])method follows the established pattern of other builder methods in this class (environments, deployments, policies), providing a consistent interface for resource-specific selector computations.packages/db/src/selectors/compute/replace-builder.ts (5)
10-10: LGTM: Added preHook parameter to the constructor.Adding this lifecycle hook parameter allows for operations to be performed before the main database operations, enhancing the flexibility of the ReplaceBuilder.
13-13: LGTM: Added postHook parameter to the constructor.Adding this lifecycle hook parameter allows for operations to be performed after the main database operations, completing the pre/post hook pattern.
18-18: LGTM: Calling preHook before database operations.The preHook is now properly awaited before proceeding with deletion of previous entries, allowing for any necessary setup operations.
21-22: LGTM: Early return for empty values.This optimization returns an empty array when there are no values to insert, avoiding an unnecessary database operation.
27-28: LGTM: Calling postHook and returning results.The postHook is properly awaited after the database insertion, and the results are correctly returned from the transaction.
packages/job-dispatch/src/resource/handle-provider-scan.ts (2)
4-4: LGTM: Simplified import for upsertResources.The import for upsertResources has been simplified, removing unnecessary imports that are no longer used after the refactoring.
35-38: Implementation follows the new selector pattern.The resource upsert operations are now using the simplified approach without explicit selector computations. This aligns with the refactoring to move release target updates to the ResourceBuilder in the selector utility.
apps/webservice/src/app/api/v1/resources/route.ts (2)
5-5: LGTM: Simplified imports.The imports have been simplified to remove the selector and replaceReleaseTargetsAndDispatchExitHooks utilities that are no longer used, keeping only the necessary imports for the refactored implementation.
Also applies to: 9-9
72-75: Implementation follows the new selector pattern.The resource upsert operations now use the simplified approach without explicit selector computations. This aligns with the refactoring to move release target updates to the ResourceBuilder in the selector utility.
apps/pty-proxy/src/controller/agent-socket.ts (2)
12-12: Import simplification reflects architectural improvementsThe simplified import statement is part of the larger refactoring effort that removes explicit resource selector and release target manipulation from this class. By removing the
selectorimport, the code now relies on the event queue system to handle release target computation through the newResourceBuilderclass.
128-146: Simplification of resource update logic improves separation of concernsThe
updateResourcemethod has been simplified to focus solely on its core responsibilities: upserting resources, adding the updated resource to the event queue, and maintaining internal state. The previous implementation likely contained explicit selector recomputation and release target replacement logic that has been removed.This change properly delegates the complex computation of release targets to the event worker system, adhering to better separation of concerns.
apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts (2)
5-5: Import simplification reflects architectural improvementsThe imports have been updated to remove reference to selector and release target replacement utilities, aligning with the architectural changes in this PR where release target computation is now handled by the new
ResourceBuilderclass.Also applies to: 8-8
101-107: PATCH handler simplification improves API endpoint focusThe PATCH handler has been simplified to focus on its primary responsibility: upserting resources and queuing update events. The removed code likely contained explicit selector recomputation and release target replacement logic.
This change properly delegates the complex computation of release targets to the event worker system, resulting in a cleaner and more maintainable API endpoint with better separation of concerns.
apps/event-worker/src/workers/new-resource.ts (2)
21-25: Improved release target computation with fluent selector APIThe worker now uses a concise and elegant fluent interface to compute release targets directly for the specific resource. This replaces more complex logic that likely involved separate steps for recomputing environment and deployment resource selectors.
The new approach leverages the
ResourceBuilderclass (referenced in the AI summary) which encapsulates the release target computation logic, making the worker code more focused and maintainable.
26-26: Consistent pattern for handling release targetsThe updated code maintains the same pattern for mapping release targets to evaluation jobs, ensuring backwards compatibility while benefiting from the improved release target computation logic.
packages/db/src/schema/deployment.ts (2)
115-115: Enhanced deployment relations support resource computationAdding the
computedResourcesrelation to thedeploymentRelationsestablishes a proper many-to-many relationship between deployments and resources through thecomputedDeploymentResourcetable. This provides the necessary schema support for the new selector-based approach to resource management.
131-143: Complete bidirectional relation mapping improves ORM functionalityThe new
computedDeploymentResourceRelationsestablishes proper bidirectional relationships between the junction table and both thedeploymentandresourcetables. This enables the ORM to navigate relationships in both directions, which is essential for theResourceBuilderto efficiently coordinate resource-related computations.This schema enhancement provides the foundation for the refactored release target computation approach.
packages/db/src/schema/environment-relations.ts (1)
37-49: Verify this relation is re‑exported
computedEnvironmentResourceRelationsis defined here but consumers must be able to import it frompackages/db/src/schema/index.ts(or the relevant barrel). Make sure it is re‑exported, otherwise downstream selectors will fail at runtime.packages/db/src/selectors/compute/policy-builder.ts (1)
30-75: Null selector handling may yield invalid SQL
qb.resources().where(t.resourceSelector).sql()blindly receivesnull.
UnlessWhereBuilder.where()explicitly convertsnullinto a tautology, the generated SQL becomes… AND (NULL)which Postgres treats as false, silently excluding rows.Either:
const resourceCond = t.resourceSelector == null ? sql`TRUE` : qb.resources().where(t.resourceSelector).sql();or guard earlier.
Same applies to deployment/environment selectors below.
packages/db/src/selectors/compute/environment-builder.ts (4)
17-55: Well-structured refactoring of EnvironmentBuilder with clear method separationThe extraction of inline anonymous functions into properly named private methods significantly improves code readability and maintainability. The implementation correctly:
- Deletes previous computed environment resources
- Computes new environment-resource mappings based on selectors
- Provides hooks for pre and post-processing
60-64: Clean implementation of ReplaceBuilder integrationThese parameter updates correctly wire the newly extracted private methods to the ReplaceBuilder, providing a more maintainable and explicit approach compared to inline anonymous functions.
94-128: Good consistency in WorkspaceEnvironmentBuilder implementationThe WorkspaceEnvironmentBuilder follows the same pattern as EnvironmentBuilder, maintaining consistency across the codebase. The workspace-scoped implementation correctly:
- Retrieves environments in the workspace with
getEnvsInWorkspace- Deletes computed resources for those environments
- Computes new mappings based on resource selectors
- Provides pre and post hooks similar to EnvironmentBuilder
133-137: Consistent method references for WorkspaceEnvironmentBuilderThe update to use the new private methods maintains consistency with the EnvironmentBuilder implementation and improves readability over inline anonymous functions.
| const environments = await db.query.environment.findMany({ | ||
| where: eq(schema.environment.systemId, job.data.systemId), | ||
| with: { computedResources: true }, | ||
| }); | ||
|
|
||
| const computedDeploymentResources = await db | ||
| .select() | ||
| .from(schema.computedDeploymentResource) | ||
| .innerJoin( | ||
| schema.resource, | ||
| eq(schema.computedDeploymentResource.resourceId, schema.resource.id), | ||
| ) | ||
| .where(eq(schema.computedDeploymentResource.deploymentId, job.data.id)); | ||
| const resources = computedDeploymentResources.map((r) => r.resource); | ||
|
|
||
| const releaseTargetPromises = resources.map(async (r) => | ||
| replaceReleaseTargets(db, r), | ||
| const resourceIds = environments.flatMap((e) => | ||
| e.computedResources.map((r) => r.resourceId), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Deduplicate resourceIds to avoid redundant recomputations
Multiple environments often reference the same resource. Passing duplicates down the compute chain may:
- perform identical heavy SQL work several times, and
- cause
releaseTargets().replace()to execute unnecessary INSERT/DELETE statements.
-const resourceIds = environments.flatMap((e) =>
- e.computedResources.map((r) => r.resourceId),
-);
+const resourceIds = Array.from(
+ new Set(
+ environments.flatMap((e) =>
+ e.computedResources.map((r) => r.resourceId),
+ ),
+ ),
+);This small change can drastically cut database load in large systems.
📝 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.
| const environments = await db.query.environment.findMany({ | |
| where: eq(schema.environment.systemId, job.data.systemId), | |
| with: { computedResources: true }, | |
| }); | |
| const computedDeploymentResources = await db | |
| .select() | |
| .from(schema.computedDeploymentResource) | |
| .innerJoin( | |
| schema.resource, | |
| eq(schema.computedDeploymentResource.resourceId, schema.resource.id), | |
| ) | |
| .where(eq(schema.computedDeploymentResource.deploymentId, job.data.id)); | |
| const resources = computedDeploymentResources.map((r) => r.resource); | |
| const releaseTargetPromises = resources.map(async (r) => | |
| replaceReleaseTargets(db, r), | |
| const resourceIds = environments.flatMap((e) => | |
| e.computedResources.map((r) => r.resourceId), | |
| ); | |
| const environments = await db.query.environment.findMany({ | |
| where: eq(schema.environment.systemId, job.data.systemId), | |
| with: { computedResources: true }, | |
| }); | |
| const resourceIds = Array.from( | |
| new Set( | |
| environments.flatMap((e) => | |
| e.computedResources.map((r) => r.resourceId), | |
| ), | |
| ), | |
| ); |
| name: `${rt.resourceId}-${rt.environmentId}-${rt.deploymentId}`, | ||
| data: rt, | ||
| })); | ||
| await getQueue(Channel.EvaluateReleaseTarget).addBulk(jobs); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Job‑name format is inconsistent with other workers and may cause duplicate‐key collisions
Other workers (e.g. update‑environment.ts) use the database‑generated rt.id as the BullMQ job name, while this worker concatenates three columns.
If both patterns target the same queue (Channel.EvaluateReleaseTarget) BullMQ will treat the two variants as distinct jobs for the same release target, producing duplicates or collisions depending on the queue’s jobId strategy.
-const jobs = rts.map((rt) => ({
- name: `${rt.resourceId}-${rt.environmentId}-${rt.deploymentId}`,
- data: rt,
-}));
+const jobs = rts.map((rt) => ({
+ name: rt.id, // keeps one canonical identifier across workers
+ data: rt,
+}));📝 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.
| const jobs = rts.map((rt) => ({ | |
| name: `${rt.resourceId}-${rt.environmentId}-${rt.deploymentId}`, | |
| data: rt, | |
| })); | |
| await getQueue(Channel.EvaluateReleaseTarget).addBulk(jobs); | |
| const jobs = rts.map((rt) => ({ | |
| name: rt.id, // keeps one canonical identifier across workers | |
| data: rt, | |
| })); | |
| await getQueue(Channel.EvaluateReleaseTarget).addBulk(jobs); |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
apps/event-worker/src/workers/updated-resources/index.ts (2)
33-33:⚠️ Potential issueFix potential duplicate events by deduplicating deployments.
currentDeploymentsis built with a simple.map(), meaning a deployment that owns multiple release targets will appear multiple times in the array. This can lead to duplicate "deployment.resource.removed" events being dispatched.-const currentDeployments = currentReleaseTargets.map((rt) => rt.deployment); +const currentDeployments = _.uniqBy( + currentReleaseTargets.map((rt) => rt.deployment), + "id", +);
40-43:⚠️ Potential issueUse consistent job naming to prevent queue collisions.
The job naming pattern (
${rt.resourceId}-${rt.environmentId}-${rt.deploymentId}) differs from other workers that use the database-generatedrt.idas the BullMQ job name. This inconsistency could cause duplicate jobs or collisions in the queue.-const jobs = rts.map((rt) => ({ - name: `${rt.resourceId}-${rt.environmentId}-${rt.deploymentId}`, - data: rt, -})); +const jobs = rts.map((rt) => ({ + name: rt.id, // keeps one canonical identifier across workers + data: rt, +}));packages/db/src/selectors/compute/resource-builder.ts (2)
48-51:⚠️ Potential issuePotential race condition in resource matching deployment logic.
The check against
computedDeploymentResourcemay exclude deployments with valid selectors if the related CDR rows weren't yet computed. Consider performing the join based on the actual selector condition rather than relying on the pre-computed table.-const isResourceMatchingDeployment = or( - isNull(SCHEMA.deployment.resourceSelector), - eq(SCHEMA.computedDeploymentResource.resourceId, SCHEMA.resource.id), -); +// Use a more direct approach that evaluates the selector +// This is a simplified example - actual implementation would depend on your query builder +const isResourceMatchingDeployment = or( + isNull(SCHEMA.deployment.resourceSelector), + /* Your query builder expression to directly evaluate deployment.resourceSelector against resource */ +);
21-31: 💡 Verification agent🧩 Analysis chain
Avoid nested transactions in recomputeResourceSelectors.
The
replace()method of the environment and deployment builders likely creates nested transactions, which can lead to performance issues with savepoints in PostgreSQL. Consider passing the transaction object explicitly to prevent nesting.Run this to verify if the builders' replace() methods create nested transactions:
🏁 Script executed:
#!/bin/bash # Check if replace() methods create nested transactions rg -A 5 "replace\(\)" --type=ts packages/db/src/selectors/compute/Length of output: 1677
🏁 Script executed:
#!/bin/bash # Display the ReplaceBuilder implementation to confirm nested transaction usage cat packages/db/src/selectors/compute/replace-builder.tsLength of output: 1036
🏁 Script executed:
#!/bin/bash # Display the WorkspaceEnvironmentBuilder and WorkspaceDeploymentBuilder implementations sed -n '1,200p' packages/db/src/selectors/compute/resource-builder.tsLength of output: 3622
Prevent nested transactions in recomputeResourceSelectors
We’ve confirmed that
ReplaceBuilder.replace()always wraps its logic in a newtx.transaction(), so calling it inside thereleaseTargetstransaction creates nested savepoints in PostgreSQL. To avoid this overhead:• In
packages/db/src/selectors/compute/replace-builder.ts, add an inline variant (e.g.replaceInline(tx: Tx)) that runs the same hooks and insert/delete logic without thetx.transaction(...)wrapper.
• Inpackages/db/src/selectors/compute/resource-builder.ts, updaterecomputeResourceSelectorsto pass the existingtxinto this inline method instead of calling.replace().
• Ensure bothWorkspaceEnvironmentBuilderandWorkspaceDeploymentBuilderexpose the new inline API.Example diff for
resource-builder.ts:--- a/packages/db/src/selectors/compute/resource-builder.ts +++ b/packages/db/src/selectors/compute/resource-builder.ts async recomputeResourceSelectors(tx: Tx) { const envComputer = new WorkspaceEnvironmentBuilder(tx, this.workspaceId); const deploymentComputer = new WorkspaceDeploymentBuilder( tx, this.workspaceId, ); - await Promise.all([ - envComputer.resourceSelectors().replace(), - deploymentComputer.resourceSelectors().replace(), - ]); + await Promise.all([ + envComputer.resourceSelectors().replaceInline(tx), + deploymentComputer.resourceSelectors().replaceInline(tx), + ]); }apps/event-worker/src/workers/update-deployment.ts (2)
49-54: 🛠️ Refactor suggestionRefine exited resources calculation to prevent false positives.
The current approach might misclassify resources as "exited" if they appear in release targets for other deployments. Filter by this deployment's ID when checking for exited resources.
-const exitedResources = currentResources.filter( - (r) => - !rts.some( - (rt) => rt.resourceId === r.id && rt.deploymentId === data.id, - ), -); +// More efficient approach using a Set for look-up +const remainingResourceIds = new Set( + rts.filter((rt) => rt.deploymentId === data.id) + .map((rt) => rt.resourceId) +); +const exitedResources = currentResources.filter( + (r) => !remainingResourceIds.has(r.id) +);
45-48:⚠️ Potential issueCompute release targets for newly selected resources.
The code only recomputes release targets for resources that were already attached to the deployment (
currentResources). If the updated selector adds new resources, they won't be processed until another job touches them.-const rts = await selector() - .compute() - .resources(currentResources) - .releaseTargets(); +// Get all resources that should be matched by the new selector +const newlyMatchedResources = await db.query.resource.findMany({ + where: and( + eq(schema.resource.workspaceId, data.workspaceId), + // Use an appropriate query builder to apply the resourceSelector + selector().queryBuilder().resources().where(resourceSelector).sql() + ), +}); +// Combine current and newly matched resources for complete computation +const allResources = _.uniqBy([...currentResources, ...newlyMatchedResources], 'id'); +const rts = await selector() + .compute() + .resources(allResources) + .releaseTargets();apps/event-worker/src/workers/update-environment.ts (1)
65-68:⚠️ Potential issueInclude newly matched resources in computation.
The computation only includes resources that were already attached to the environment. If the updated selector matches new resources, they won't be processed until another job touches them.
-const rts = await selector() - .compute() - .resources(currentResources) - .releaseTargets(); +// Also get resources newly matched by the updated selector +const newlyMatchedResources = await db.query.resource.findMany({ + where: and( + eq(schema.resource.workspaceId, environment.workspaceId), + // Use an appropriate query builder to apply the resourceSelector + selector().queryBuilder().resources().where(environment.resourceSelector).sql() + ), +}); +// Combine current and newly matched resources for complete computation +const allResources = _.uniqBy([...currentResources, ...newlyMatchedResources], 'id'); +const rts = await selector() + .compute() + .resources(allResources) + .releaseTargets();
🧹 Nitpick comments (2)
packages/db/src/selectors/compute/resource-builder.ts (1)
9-19: Add early return for empty resources array.If
this.resourcesis empty, the code will attempt to extract the workspace ID from an empty array, which might cause issues. Consider adding an early return or check for empty resources.constructor( private readonly tx: Tx, private readonly resources: SCHEMA.Resource[], ) { + if (resources.length === 0) { + throw new Error("At least one resource is required"); + } const workspaceIds = new Set(this.resources.map((r) => r.workspaceId)); if (workspaceIds.size !== 1) throw new Error("All resources must be in the same workspace"); this.workspaceId = Array.from(workspaceIds)[0]!; }apps/event-worker/src/workers/update-environment.ts (1)
73-79: Optimize exited resources calculation with Set.The current filtering approach can be inefficient for larger arrays. Consider using a Set for O(1) lookups instead of nested array filtering.
-const exitedResources = currentResources.filter( - (r) => - !rts.some( - (rt) => - rt.resourceId === r.id && rt.environmentId === environment.id, - ), -); +// More efficient approach using a Set for look-up +const remainingResourceIds = new Set( + rts.filter((rt) => rt.environmentId === environment.id) + .map((rt) => rt.resourceId) +); +const exitedResources = currentResources.filter( + (r) => !remainingResourceIds.has(r.id) +);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/event-worker/src/workers/new-deployment.ts(2 hunks)apps/event-worker/src/workers/new-resource.ts(1 hunks)apps/event-worker/src/workers/update-deployment.ts(3 hunks)apps/event-worker/src/workers/update-environment.ts(2 hunks)apps/event-worker/src/workers/update-resource-variable.ts(2 hunks)apps/event-worker/src/workers/updated-resources/index.ts(2 hunks)packages/db/src/selectors/compute/compute.ts(3 hunks)packages/db/src/selectors/compute/resource-builder.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/event-worker/src/workers/new-resource.ts
- packages/db/src/selectors/compute/compute.ts
- apps/event-worker/src/workers/update-resource-variable.ts
- apps/event-worker/src/workers/new-deployment.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/event-worker/src/workers/update-environment.tspackages/db/src/selectors/compute/resource-builder.tsapps/event-worker/src/workers/updated-resources/index.tsapps/event-worker/src/workers/update-deployment.ts
🧬 Code Graph Analysis (2)
apps/event-worker/src/workers/update-environment.ts (4)
packages/db/src/client.ts (1)
db(15-15)packages/db/src/schema/environment.ts (1)
environment(59-84)packages/db/src/selectors/index.ts (1)
selector(18-18)packages/events/src/index.ts (1)
getQueue(28-34)
packages/db/src/selectors/compute/resource-builder.ts (5)
packages/db/src/common.ts (1)
Tx(22-22)packages/db/src/selectors/compute/compute.ts (1)
resources(41-43)packages/db/src/selectors/compute/environment-builder.ts (1)
WorkspaceEnvironmentBuilder(85-139)packages/db/src/selectors/compute/deployment-builder.ts (1)
WorkspaceDeploymentBuilder(83-137)packages/db/src/selectors/compute/policy-builder.ts (1)
WorkspacePolicyBuilder(91-175)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (5)
apps/event-worker/src/workers/updated-resources/index.ts (2)
9-20: LGTM! Inline function to dispatch exit hooks.The new approach to dispatch exit hooks as independent promises with
allSettledis a good improvement that allows the system to continue even if some event dispatches fail.
35-38: LGTM! Clean selector-based approach.Using the selector API with the new resource-scoped computation is cleaner and more maintainable than the previous approach with explicit release target manipulation.
packages/db/src/selectors/compute/resource-builder.ts (1)
98-112: LGTM! Atomic transaction with clear phases.Good structuring of the transaction with clear phases: recompute selectors, delete existing targets, find matches, insert new targets, and update policies. This ensures atomicity and prevents partial updates.
apps/event-worker/src/workers/update-deployment.ts (1)
40-45: LGTM! Clean resource fetching approach.Using the join with
with: { resource: true }is a clean approach to fetch the release targets and their associated resources in a single query.apps/event-worker/src/workers/update-environment.ts (1)
59-64: LGTM! Effective resource/release target query.The approach to query current release targets with their resources is clean and avoids unnecessary database round-trips.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/db/src/selectors/compute/environment-builder.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/db/src/selectors/compute/environment-builder.ts
🔇 Additional comments (1)
packages/db/src/selectors/compute/environment-builder.ts (1)
19-28: Guard against emptyenvironmentIdsbefore generatingIN (...)clauseIf
this.environmentsis an empty array, theinArray(...)call will emit anIN ()predicate.
Depending on the SQL dialect / Drizzle implementation, this can either:
- produce invalid SQL (
WHERE ... IN ()) and throw at runtime, or- silently match zero rows but still create an unnecessary statement.
A lightweight defensive guard prevents both issues and saves an unnecessary round–trip:
-await tx - .delete(SCHEMA.computedEnvironmentResource) - .where( - inArray( - SCHEMA.computedEnvironmentResource.environmentId, - this.environmentIds, - ), - ); +if (this.environmentIds.length) { + await tx + .delete(SCHEMA.computedEnvironmentResource) + .where( + inArray( + SCHEMA.computedEnvironmentResource.environmentId, + this.environmentIds, + ), + ); +}Please double‑check how
drizzle-ormhandlesIN ()and adjust accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/db/src/selectors/compute/resource-builder.ts (2)
30-32: Consider adding an early check for empty resources arrayWhile the implementation handles an empty result set later (line 118), consider adding an early return in
releaseTargets()ifthis.resourceIds.length === 0to avoid unnecessary database operations.releaseTargets() { return this.tx.transaction(async (tx) => { + if (this.resourceIds.length === 0) return []; await this.deleteExistingReleaseTargets(tx); const vals = await this.findMatchingEnvironmentDeploymentPairs(tx); if (vals.length === 0) return [];
1-131: Consider adding error handling for database operationsWhile the coding guidelines don't strictly enforce try/catch blocks, consider adding some error handling for the critical database operations, especially when multiple operations need to succeed together.
For example, if
recomputePolicyReleaseTargetsfails after successfully inserting release targets, you might want to log the error or take specific recovery actions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/db/src/selectors/compute/compute.ts(3 hunks)packages/db/src/selectors/compute/resource-builder.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/db/src/selectors/compute/compute.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/db/src/selectors/compute/resource-builder.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (5)
packages/db/src/selectors/compute/resource-builder.ts (5)
20-28: LGTM: Good constructor implementation with proper validationThe constructor correctly validates that all resources belong to the same workspace and extracts the workspace ID. The error message is clear and descriptive.
40-60: Good documentation for complex matching logicThe detailed documentation for
findMatchingEnvironmentDeploymentPairsclearly explains the matching conditions and the relationships between the tables being joined. The note about assuming up-to-date computed environment resource selectors is particularly helpful.
61-107: Verify deployment selector matching logic still meets requirementsThe previous review raised concerns about the LEFT JOIN with
computedDeploymentResourcepotentially filtering out deployments unintentionally. Although marked as resolved, please verify that this implementation correctly handles the following scenario:
- A deployment with a non-null selector but no matching
computedDeploymentResourcerow yetThis edge case could occur during race conditions where selector computation is still in progress.
109-112: Good use of existing policy builderLeveraging the
WorkspacePolicyBuilderfor policy-related computations maintains separation of concerns and avoids duplicating logic.
114-130: Well-structured transaction flow with proper handling of empty resultsThe main
releaseTargetsmethod:
- Properly uses a transaction for atomic operations
- Correctly handles the case of no matching pairs
- Uses
onConflictDoNothingto handle potential duplicates gracefully- Returns the results to the caller
This structure ensures data consistency and handles edge cases appropriately.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/db/src/selectors/compute/policy-builder.ts (1)
57-61: Guard against NULL selectors when building dynamic predicates
qb.resources().where(t.resourceSelector).sql()(and the deployment/environment equivalents) will be called even whent.resourceSelectorisnull, which may produce an invalid SQL fragment depending onQueryBuilder’s implementation.
Consider short‑circuiting or generating a tautology (TRUE) when a selector isnull.Also applies to: 132-136
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/db/src/selectors/compute/compute.ts(3 hunks)packages/db/src/selectors/compute/deployment-builder.ts(1 hunks)packages/db/src/selectors/compute/policy-builder.ts(2 hunks)packages/db/src/selectors/compute/replace-builder.ts(0 hunks)packages/db/src/selectors/compute/resource-builder.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/db/src/selectors/compute/replace-builder.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/db/src/selectors/compute/compute.ts
- packages/db/src/selectors/compute/resource-builder.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/db/src/selectors/compute/deployment-builder.tspackages/db/src/selectors/compute/policy-builder.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint
| const promises = deployments.map(async (d) => { | ||
| const { system } = d; | ||
| const { workspaceId } = system; | ||
| if (d.resourceSelector == null) return []; | ||
| const resources = await tx.query.resource.findMany({ | ||
| where: and( | ||
| eq(SCHEMA.resource.workspaceId, workspaceId), | ||
| this._queryBuilder.resources().where(d.resourceSelector).sql(), | ||
| ), | ||
| }); | ||
|
|
||
| const fulfilled = await Promise.all(promises); | ||
| return fulfilled.flat(); | ||
| }, | ||
| ); | ||
| return resources.map((r) => ({ | ||
| deploymentId: d.id, | ||
| resourceId: r.id, | ||
| })); | ||
| }); | ||
|
|
||
| const fulfilled = await Promise.all(promises); | ||
| return fulfilled.flat(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Concurrent queries on the same transaction may dead‑lock the pg client
Promise.all() fires multiple queries in parallel against a single tx (one connection).
pg (used under the hood by Drizzle) does not support overlapping queries on the same client and will throw
Error: Client has already been released / cannot run multiple commands on the same socket.
Refactor to run the queries sequentially (or batch them in SQL):
- const promises = deployments.map(async (d) => {
- … // same for workspace variant
- });
-
- const fulfilled = await Promise.all(promises);
- return fulfilled.flat();
+ const matches: Array<{ deploymentId: string; resourceId: string }> = [];
+ for (const d of deployments) {
+ if (d.resourceSelector == null) continue;
+ const res = await tx.query.resource.findMany({
+ where: and(
+ eq(SCHEMA.resource.workspaceId, d.system.workspaceId),
+ this._queryBuilder.resources().where(d.resourceSelector).sql(),
+ ),
+ });
+ matches.push(
+ ...res.map((r) => ({ deploymentId: d.id, resourceId: r.id })),
+ );
+ }
+ return matches;(The same pattern applies to lines 102‑118).
Also applies to: 102-118
| const targetPromises = this.targets.map(async (t) => { | ||
| if ( | ||
| t.resourceSelector == null && | ||
| t.deploymentSelector == null && | ||
| t.environmentSelector == null | ||
| ) | ||
| return []; | ||
| const releaseTargets = await tx | ||
| .select() | ||
| .from(SCHEMA.releaseTarget) | ||
| .innerJoin( | ||
| SCHEMA.resource, | ||
| eq(SCHEMA.releaseTarget.resourceId, SCHEMA.resource.id), | ||
| ) | ||
| .innerJoin( | ||
| SCHEMA.deployment, | ||
| eq(SCHEMA.releaseTarget.deploymentId, SCHEMA.deployment.id), | ||
| ) | ||
| .innerJoin( | ||
| SCHEMA.environment, | ||
| eq(SCHEMA.releaseTarget.environmentId, SCHEMA.environment.id), | ||
| ) | ||
| .where( | ||
| and( | ||
| qb.resources().where(t.resourceSelector).sql(), | ||
| qb.deployments().where(t.deploymentSelector).sql(), | ||
| qb.environments().where(t.environmentSelector).sql(), | ||
| ), | ||
| ); | ||
| }, | ||
| async (tx) => { | ||
| const targets = await tx | ||
| .select() | ||
| .from(SCHEMA.policyTarget) | ||
| .where(inArray(SCHEMA.policyTarget.policyId, this.ids)); | ||
|
|
||
| const qb = new QueryBuilder(tx); | ||
| const targetPromises = targets.map(async (t) => { | ||
| if ( | ||
| t.resourceSelector == null && | ||
| t.deploymentSelector == null && | ||
| t.environmentSelector == null | ||
| ) | ||
| return []; | ||
| const releaseTargets = await tx | ||
| .select() | ||
| .from(SCHEMA.releaseTarget) | ||
| .innerJoin( | ||
| SCHEMA.resource, | ||
| eq(SCHEMA.releaseTarget.resourceId, SCHEMA.resource.id), | ||
| ) | ||
| .innerJoin( | ||
| SCHEMA.deployment, | ||
| eq(SCHEMA.releaseTarget.deploymentId, SCHEMA.deployment.id), | ||
| ) | ||
| .innerJoin( | ||
| SCHEMA.environment, | ||
| eq(SCHEMA.releaseTarget.environmentId, SCHEMA.environment.id), | ||
| ) | ||
| .where( | ||
| and( | ||
| qb.resources().where(t.resourceSelector).sql(), | ||
| qb.deployments().where(t.deploymentSelector).sql(), | ||
| qb.environments().where(t.environmentSelector).sql(), | ||
| ), | ||
| ); | ||
| return releaseTargets.map((rt) => ({ | ||
| policyTargetId: t.id, | ||
| releaseTargetId: rt.release_target.id, | ||
| })); | ||
| }); | ||
|
|
||
| return releaseTargets.map((rt) => ({ | ||
| policyTargetId: t.id, | ||
| releaseTargetId: rt.release_target.id, | ||
| })); | ||
| }); | ||
| const fulfilled = await Promise.all(targetPromises); | ||
| return fulfilled.flat(); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Serialise selector evaluations to avoid connection contention
Promise.all() runs selector queries in parallel on the single transaction tx, risking the same concurrency problem described for the deployment builder.
Replace the parallel map with a for…of loop (or merge the logic into a single SQL statement with OR’ed predicates).
This change applies to both the per‑policy section (34‑72) and the workspace variant (115‑145).
Also applies to: 115-145
Serialise selector evaluations to avoid connection contention
Kysely transactions share a single connection and will error if you fire overlapping queries on the same tx. Running your selector queries in parallel via Promise.all risks “already a query in progress” or similar driver errors. Switch to sequential iteration (for…of) or merge into one SQL statement with OR‑ed predicates.
Affected locations:
- packages/db/src/selectors/compute/policy-builder.ts: lines 34–72
- packages/db/src/selectors/compute/policy-builder.ts (workspace variant): lines 115–145
Example sequential refactor:
const results: { policyTargetId: string; releaseTargetId: string }[] = [];
for (const t of this.targets) {
if (!t.resourceSelector && !t.deploymentSelector && !t.environmentSelector) {
continue;
}
const releaseTargets = await tx
.select()
.from(SCHEMA.releaseTarget)
// …same joins…
.where(
and(
qb.resources().where(t.resourceSelector).sql(),
qb.deployments().where(t.deploymentSelector).sql(),
qb.environments().where(t.environmentSelector).sql(),
),
);
for (const rt of releaseTargets) {
results.push({
policyTargetId: t.id,
releaseTargetId: rt.release_target.id,
});
}
}
return results;There was a problem hiding this comment.
I tested this and it didn't seem to cause any issues and I tested with two targets in the system
There was a problem hiding this comment.
@adityachoudhari26 Thanks for testing this and sharing your results. If it's working fine with multiple targets in your testing environment, that's good to know. The theoretical concern about transaction connection contention is more significant with larger numbers of concurrent queries, but if your testing shows the current implementation is stable for your use cases, that's what matters most.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/event-worker/src/workers/update-deployment.ts (1)
50-53: New resources added by the updated selector are still not covered
This was raised earlier and remains: computing release targets only forcurrentResourcesignores any new resources now matched byresourceSelector, leading to an inconsistent state until another worker touches them.
Please compute over the selector‑derived resource set instead of the pre‑existing one.
🧹 Nitpick comments (1)
apps/event-worker/src/workers/update-deployment.ts (1)
60-60: Consider awaitingdispatchExitHooksto avoid unhandled rejections
dispatchExitHooksreturns a promise. If it rejects, the worker will still resolve (the error is swallowed), potentially missing critical exit‑hook processing. Addawaitso that failures surface.- await dispatchExitHooks(data, exitedResources); + await dispatchExitHooks(data, exitedResources);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/event-worker/src/workers/new-deployment.ts(2 hunks)apps/event-worker/src/workers/update-deployment.ts(3 hunks)apps/event-worker/src/workers/update-environment.ts(2 hunks)apps/event-worker/src/workers/update-resource-variable.ts(2 hunks)apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts(2 hunks)apps/webservice/src/app/api/v1/resources/route.ts(2 hunks)packages/job-dispatch/src/resource/handle-provider-scan.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts
- apps/event-worker/src/workers/update-resource-variable.ts
- apps/event-worker/src/workers/new-deployment.ts
- apps/event-worker/src/workers/update-environment.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/job-dispatch/src/resource/handle-provider-scan.tsapps/webservice/src/app/api/v1/resources/route.tsapps/event-worker/src/workers/update-deployment.ts
🧬 Code Graph Analysis (3)
packages/job-dispatch/src/resource/handle-provider-scan.ts (2)
packages/job-dispatch/src/resource/delete.ts (1)
deleteResources(55-74)packages/db/src/selectors/index.ts (1)
selector(18-18)
apps/webservice/src/app/api/v1/resources/route.ts (2)
packages/db/src/selectors/index.ts (1)
selector(18-18)packages/events/src/index.ts (1)
getQueue(28-34)
apps/event-worker/src/workers/update-deployment.ts (3)
packages/db/src/client.ts (1)
db(15-15)packages/db/src/selectors/index.ts (1)
selector(18-18)packages/events/src/index.ts (1)
getQueue(28-34)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
🔇 Additional comments (1)
apps/event-worker/src/workers/update-deployment.ts (1)
40-49:resources(currentResources)expects IDs, not full objectsUnless the compute builder overloads the method, passing full
Resourceobjects will result in a type/runtime error or an unnecessary object‑to‑ID coercion.- const rts = await computeBuilder - .resources(currentResources) + const rts = await computeBuilder + .resources(currentResources.map((r) => r.id)) .releaseTargets();
|
|
||
| const insertJobs = insertedResources.map((r) => ({ name: r.id, data: r })); | ||
| const updateJobs = updatedResources.map((r) => ({ name: r.id, data: r })); | ||
| const deleted = await deleteResources(tx, toDelete); |
There was a problem hiding this comment.
deleteResources() currently returns void, so deleted is always undefined
The returned object { all: [...], deleted } will therefore expose an undefined value and may break consumers expecting a list/count of deleted resources.
- const deleted = await deleteResources(tx, toDelete);
+ await deleteResources(tx, toDelete);If callers need this information, change deleteResources() to return the affected resources and update its implementation accordingly.
📝 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.
| const deleted = await deleteResources(tx, toDelete); | |
| await deleteResources(tx, toDelete); |
| await selector(tx).compute().allResourceSelectors(workspaceId); | ||
|
|
||
| await selector().compute().allResourceSelectors(workspaceId); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Running selector computation twice is costly and probably redundant
allResourceSelectors(workspaceId) performs non‑trivial DB writes.
Invoking it inside the same transaction (selector(tx) …) and again outside the transaction (selector() …) doubles the work, increases lock contention, and may introduce race conditions if the second call starts before the outer transaction commits.
Unless you have a very specific consistency requirement, one call should be enough:
- await selector(tx).compute().allResourceSelectors(workspaceId);
- await selector().compute().allResourceSelectors(workspaceId);
+ await selector(tx).compute().allResourceSelectors(workspaceId);If you truly need a post‑commit recomputation, consider a follow‑up job instead of an immediate synchronous call.
📝 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.
| await selector(tx).compute().allResourceSelectors(workspaceId); | |
| await selector().compute().allResourceSelectors(workspaceId); | |
| await selector(tx).compute().allResourceSelectors(workspaceId); |
| selector() | ||
| .compute() | ||
| .allResourceSelectors(workspaceId) | ||
| .then(() => { | ||
| getQueue(Channel.NewResource).addBulk(insertJobs); | ||
| getQueue(Channel.UpdatedResource).addBulk(updateJobs); | ||
| }); |
There was a problem hiding this comment.
Un‑awaited promise hides failures and may drop follow‑up jobs
The .then() chain is fire‑and‑forget; any rejection in allResourceSelectors() or addBulk() will be swallowed, and the request will still return HTTP 200.
Wrap the whole sequence in await (inside the existing try / catch) so that errors propagate and you log a consistent response.
- selector()
- .compute()
- .allResourceSelectors(workspaceId)
- .then(() => {
- getQueue(Channel.NewResource).addBulk(insertJobs);
- getQueue(Channel.UpdatedResource).addBulk(updateJobs);
- });
+ await selector().compute().allResourceSelectors(workspaceId);
+ await Promise.all([
+ getQueue(Channel.NewResource).addBulk(insertJobs),
+ getQueue(Channel.UpdatedResource).addBulk(updateJobs),
+ ]);📝 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.
| selector() | |
| .compute() | |
| .allResourceSelectors(workspaceId) | |
| .then(() => { | |
| getQueue(Channel.NewResource).addBulk(insertJobs); | |
| getQueue(Channel.UpdatedResource).addBulk(updateJobs); | |
| }); | |
| await selector().compute().allResourceSelectors(workspaceId); | |
| await Promise.all([ | |
| getQueue(Channel.NewResource).addBulk(insertJobs), | |
| getQueue(Channel.UpdatedResource).addBulk(updateJobs), | |
| ]); |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/job-dispatch/src/resource/handle-provider-scan.ts (1)
46-46:⚠️ Potential issue
deleteResources()function returns void, makingdeletedalways undefinedThe
deleteResources()function doesn't return any value, but you're assigning its result to adeletedvariable that's later included in the returned object. This will causedeletedto always beundefined, potentially breaking consumers expecting a list or count of deleted resources.- const deleted = await deleteResources(tx, toDelete); + await deleteResources(tx, toDelete);And then update the return statement accordingly:
- return { all: [...insertedResources, ...updatedResources], deleted }; + return { all: [...insertedResources, ...updatedResources] };If you need to track deleted resources, consider modifying the
deleteResources()function to return the deleted resources.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.json(1 hunks)packages/job-dispatch/src/resource/handle-provider-scan.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/job-dispatch/src/resource/handle-provider-scan.ts
🧬 Code Graph Analysis (1)
packages/job-dispatch/src/resource/handle-provider-scan.ts (2)
packages/job-dispatch/src/resource/delete.ts (1)
deleteResources(55-74)packages/db/src/selectors/index.ts (1)
selector(18-18)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
| await Promise.all(promises); | ||
| await cb.allPolicies(workspaceId).releaseTargetSelectors().replace(); | ||
|
|
||
| await selector().compute().allResourceSelectors(workspaceId); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using the existing transaction for selector computation
The current implementation creates a new transaction when calling selector().compute().allResourceSelectors(workspaceId). Since you already have an active transaction tx, it would be more efficient to reuse it rather than creating a new one.
- await selector().compute().allResourceSelectors(workspaceId);
+ await selector(tx).compute().allResourceSelectors(workspaceId);Using the existing transaction will:
- Reduce database lock contention
- Improve performance by avoiding unnecessary transaction overhead
- Ensure data consistency within the same transaction
- Prevent potential race conditions if the second call starts before the outer transaction commits
📝 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.
| await selector().compute().allResourceSelectors(workspaceId); | |
| await selector(tx).compute().allResourceSelectors(workspaceId); |
…lplane into move-upsert-release-targets
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/db/src/selectors/compute/resource-builder.ts (1)
133-250: Consider reducing code duplication between Resource and WorkspaceResourceBuilder.Both classes share very similar implementation for
findMatchingEnvironmentDeploymentPairsand other methods. Consider extracting common logic to a base class or shared utility functions.+// Create a helper function or base class with shared logic +function findMatchingPairs(tx: Tx, resourceIds: string[]) { + const isResourceMatchingEnvironment = eq( + SCHEMA.computedEnvironmentResource.resourceId, + SCHEMA.resource.id, + ); + // ... rest of the implementation + return tx.select()... +} // Then in both classes: private findMatchingEnvironmentDeploymentPairs(tx: Tx, resourceIds?: string[]) { - // Duplicate implementation + return findMatchingPairs(tx, resourceIds ?? this.resourceIds); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/event-worker/src/workers/update-deployment.ts(3 hunks)apps/event-worker/src/workers/update-environment.ts(2 hunks)packages/db/src/schema/deployment.ts(4 hunks)packages/db/src/schema/environment-relations.ts(3 hunks)packages/db/src/selectors/compute/compute.ts(3 hunks)packages/db/src/selectors/compute/resource-builder.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/db/src/schema/environment-relations.ts
- packages/db/src/schema/deployment.ts
- packages/db/src/selectors/compute/compute.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/event-worker/src/workers/update-deployment.tsapps/event-worker/src/workers/update-environment.tspackages/db/src/selectors/compute/resource-builder.ts
🧬 Code Graph Analysis (1)
apps/event-worker/src/workers/update-environment.ts (4)
packages/db/src/schema/environment.ts (1)
environment(59-84)packages/db/src/client.ts (1)
db(15-15)packages/db/src/selectors/index.ts (1)
selector(18-18)packages/events/src/index.ts (1)
getQueue(28-34)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (19)
apps/event-worker/src/workers/update-deployment.ts (8)
3-3: LGTM: Import selection reflects refactored approach.Import of
eqandselectorfrom@ctrlplane/dbsupports the shift to selector-based utility.
12-23: LGTM: Simplified exit hook dispatch function.The function no longer needs a database transaction parameter, aligning with the new selector-based approach.
25-29: LGTM: Updated comments accurately reflect new implementation.The documentation has been updated to describe the new workflow:
- Query current release targets with resources
- Recompute release targets for current resources
40-46: LGTM: More efficient query with eager loading.Using
findFirstwith nestedwithclauses efficiently loads the deployment with its system and release targets (including resources) in a single query.
47-51: LGTM: Clear extraction of current state.Extracting current release targets and resources before recomputation provides a clean baseline for comparison.
52-61: LGTM: Workspace-scoped recomputation of release targets.Using
allResources(workspaceId)ensures all affected resources are included in the recomputation, not just those previously attached to the deployment.
56-61: Fix for the past issue with exited resources calculation.The filter now correctly checks both resource ID and deployment ID, preventing false positives when a resource appears in multiple deployments.
65-65: LGTM: Simplified exit hook invocation.Exit hooks are now dispatched directly without a transaction parameter, consistent with the simplified function signature.
apps/event-worker/src/workers/update-environment.ts (6)
4-4: LGTM: Import selection reflects refactored approach.Import of
eqandselectorfrom@ctrlplane/dbsupports the shift to selector-based utility.
56-57: LGTM: Early return for unchanged selectors.Efficiently skips processing when resource selectors haven't changed.
59-65: LGTM: Efficient query with eager loading.Using
findFirstwith nestedwithclauses efficiently loads the environment with its system and release targets (including resources) in a single query.
66-68: LGTM: Clear extraction of current state.Extracting current release targets and resources before recomputation provides a clean baseline for comparison.
69-75: LGTM: Comprehensive recomputation approach.The approach correctly:
- Computes environment resource selectors first
- Then computes release targets for all resources in the workspace
This ensures complete and consistent recomputation, including any newly matched resources.
80-86: LGTM: Proper identification of exited resources.The filter correctly checks both resource ID and environment ID when determining which resources have exited.
packages/db/src/selectors/compute/resource-builder.ts (5)
1-6: LGTM: Clear imports and dependencies.The imports are well-organized, focusing on database expressions and necessary schema elements.
7-131: LGTM: Well-designed ResourceBuilder class.The ResourceBuilder class effectively:
- Validates resources belong to same workspace
- Deletes existing release targets
- Computes new matches via complex but correct joining logic
- Updates policy release targets
- Handles everything in a single transaction
The implementation aligns with the PR objectives to consolidate release target computation in the selector utility.
61-107: Resource matching logic correctly handles deployment selectors.The query joins resources with computed environment resources, environments, deployments, and (optionally) computed deployment resources. It correctly handles deployments with null selectors as matching all environment resources.
114-130: LGTM: Transactional release target update.The method effectively orchestrates all steps within a transaction, ensuring atomicity:
- Delete existing targets
- Find new matches
- Insert new targets
- Recompute policy release targets
228-249: LGTM: Proper workspace-level release target computation.The WorkspaceResourceBuilder implementation correctly:
- Fetches all resources in the workspace
- Recomputes all their release targets in one transaction
- Updates policies afterward
This enables the efficient workspace-scoped computation seen in the worker implementations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/rule-engine/src/utils/merge-policies.ts (1)
45-47: Updated structure for deploymentVersionSelectorThis change modifies the structure of the merged policy object to match the new pattern used throughout the system. The nested structure aligns with the broader refactoring effort to standardize selector handling.
Consider adding a comment explaining why this nesting structure is necessary, which would help future developers understand the design decision.
return { ...merged, + // Nest deploymentVersionSelector to match the expected structure in the selector API deploymentVersionSelector: { deploymentVersionSelector: mergedVersionSelector, }, };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/rule-engine/src/manager/version-manager.ts(2 hunks)packages/rule-engine/src/utils/merge-policies.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/rule-engine/src/utils/merge-policies.tspackages/rule-engine/src/manager/version-manager.ts
🧬 Code Graph Analysis (1)
packages/rule-engine/src/manager/version-manager.ts (2)
packages/db/src/selectors/query/deployment-selector.ts (1)
sql(35-39)packages/db/src/selectors/index.ts (1)
selector(18-18)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (3)
packages/rule-engine/src/manager/version-manager.ts (3)
10-10: Good addition of the selector importThis import addition aligns with the PR objective of moving release target operations to the selector utility.
123-127: Improved approach using selector API to generate SQLThe selector API provides a more consistent and maintainable way to generate SQL conditions compared to the previous helper function approach. This change aligns well with the broader refactoring effort to centralize selector logic.
134-134: Properly integrating the generated SQLUsing the SQL generated by the selector API in the query's where clause is a good replacement for the previous approach using schema.deploymentVersionMatchesCondition.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
apps/event-worker/src/workers/update-environment.ts (1)
35-37: 🛠️ Refactor suggestionCapture & log rejected exit‑hook dispatches
Re‑use the logging strategy proposed for the other workers so failures inside
handleEventdon’t disappear.
♻️ Duplicate comments (4)
apps/event-worker/src/workers/new-deployment.ts (1)
39-41: 🛠️ Refactor suggestionDeduplicate resources to avoid redundant recomputations.
Multiple environments often reference the same resource. Passing duplicates down the compute chain may:
- Perform identical heavy SQL work multiple times
- Cause unnecessary INSERT/DELETE statements during release target computation
-const resources = environments.flatMap((e) => - e.computedResources.map((r) => r.resource), -); +const resources = Array.from( + new Set( + environments.flatMap((e) => + e.computedResources.map((r) => r.resource), + ), + ), +);apps/event-worker/src/workers/updated-resources/index.ts (1)
9-9: Job‑ID format inconsistency still present
Past feedback suggested using the canonicalrt.idas the BullMQ job name to avoid duplicate‑key collisions between workers.
dispatchEvaluateJobs()still constructs the composite
"${rt.resourceId}-${rt.environmentId}-${rt.deploymentId}".If other workers (or future refactors) enqueue the same release‑target with a different
jobId, BullMQ will treat them as independent jobs for the same work item.Consider switching
dispatchEvaluateJobstoname: rt.id(or a hash thereof) to keep a single authoritative identifier.
(See previous review for a diff.)Also applies to: 43-43
apps/event-worker/src/workers/update-resource-variable.ts (1)
61-63: Potential job‑name collision carried over
dispatchEvaluateJobs(rts)inherits the composite job‑name format.
See earlier comment inupdated-resources/index.ts– usingrt.idremains the recommended fix.apps/event-worker/src/workers/update-environment.ts (1)
84-86: Job‑name format inconsistencySame remark as in the other workers regarding
dispatchEvaluateJobsand the composite job‑name.
🧹 Nitpick comments (1)
apps/event-worker/src/workers/update-resource-variable.ts (1)
64-68: Micro‑nit: drop lodash for deduplicationFor a small one‑liner you can avoid the lodash dependency:
const exitedDeployments = [ ...new Map( currentDeployments .filter((d) => !rts.some((rt) => rt.deploymentId === d.id)) .map((d) => [d.id, d]), ).values(), ];Not critical, but shaving unnecessary dependencies keeps bundle size lean.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/event-worker/src/utils/dispatch-evaluate-jobs.ts(1 hunks)apps/event-worker/src/workers/new-deployment-version.ts(2 hunks)apps/event-worker/src/workers/new-deployment.ts(2 hunks)apps/event-worker/src/workers/new-resource.ts(2 hunks)apps/event-worker/src/workers/update-deployment-variable.ts(2 hunks)apps/event-worker/src/workers/update-deployment.ts(3 hunks)apps/event-worker/src/workers/update-environment.ts(3 hunks)apps/event-worker/src/workers/update-resource-variable.ts(2 hunks)apps/event-worker/src/workers/updated-resources/index.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/event-worker/src/utils/dispatch-evaluate-jobs.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/event-worker/src/workers/update-deployment.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/event-worker/src/workers/new-deployment-version.tsapps/event-worker/src/workers/update-deployment-variable.tsapps/event-worker/src/workers/new-resource.tsapps/event-worker/src/workers/new-deployment.tsapps/event-worker/src/workers/update-resource-variable.tsapps/event-worker/src/workers/update-environment.tsapps/event-worker/src/workers/updated-resources/index.ts
🧬 Code Graph Analysis (3)
apps/event-worker/src/workers/new-deployment-version.ts (1)
apps/event-worker/src/utils/dispatch-evaluate-jobs.ts (1)
dispatchEvaluateJobs(5-11)
apps/event-worker/src/workers/update-deployment-variable.ts (1)
apps/event-worker/src/utils/dispatch-evaluate-jobs.ts (1)
dispatchEvaluateJobs(5-11)
apps/event-worker/src/workers/update-resource-variable.ts (7)
packages/logger/src/index.ts (1)
logger(48-48)packages/db/src/schema/deployment.ts (1)
deployment(69-93)packages/job-dispatch/src/events/index.ts (1)
handleEvent(8-9)packages/db/src/schema/resource.ts (1)
resource(59-87)packages/db/src/selectors/index.ts (1)
selector(18-18)packages/db/src/client.ts (1)
db(15-15)apps/event-worker/src/utils/dispatch-evaluate-jobs.ts (1)
dispatchEvaluateJobs(5-11)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (13)
apps/event-worker/src/workers/new-deployment-version.ts (2)
4-6: Clean import refactoring.The removal of
getQueueimport and addition ofdispatchEvaluateJobsutility import aligns with the refactoring goal of centralizing release target evaluation job dispatching.
23-23: Good simplification by using the utility function.Replacing direct queue management with the
dispatchEvaluateJobsutility function reduces code duplication and makes the worker more focused on its core responsibility.apps/event-worker/src/workers/update-deployment-variable.ts (2)
4-6: Clean import refactoring.The removal of
getQueueimport and addition ofdispatchEvaluateJobsutility import aligns with the refactoring goal of centralizing release target evaluation job dispatching.
32-32: Good simplification by using the utility function.Replacing direct queue management with the
dispatchEvaluateJobsutility function reduces code duplication and makes the worker more focused on its core responsibility.apps/event-worker/src/workers/new-resource.ts (3)
1-6: Proper import organization.Good job changing
schemato a type-only import and removing unused imports. Adding the necessary imports for the selector API and the dispatch utility function supports the new implementation approach.
8-13: Well-structured helper function for recomputation.The new
recomputeReleaseTargetshelper function effectively uses the selector API to compute release targets for a specific resource. The implementation follows a clear pattern:
- Create a compute builder
- Compute all resource selectors for the workspace
- Return release targets for the specific resource
This separation of concerns improves maintainability and readability.
30-31: Simplified worker logic.The worker implementation is now much cleaner and more focused, delegating the recomputation and job dispatching to specialized utility functions.
apps/event-worker/src/workers/new-deployment.ts (4)
1-1: Good import addition.Adding the
selectorimport supports the refactored implementation that uses the selector API.
9-16: Well-structured helper function for recomputation.The new
recomputeReleaseTargetshelper function effectively uses the selector API to compute release targets for a deployment and a list of resources. The implementation follows a clear pattern:
- Create a compute builder
- Compute resource selectors for the deployment
- Return release targets for the given resources
This separation of concerns improves maintainability and readability.
34-37: Improved query efficiency.The updated query fetches environments by systemId with their computed resources and associated resource entities in a single database call, which is more efficient than the previous approach.
43-43: Clean usage of helper function.Good job using the new helper function to compute release targets for the deployment and resources.
apps/event-worker/src/workers/updated-resources/index.ts (1)
45-48: Duplicate‑event issue fixed – nice!
_.uniqByoncurrentDeploymentsguarantees each deployment is considered once, eliminating the duplicate
“deployment.resource.removed” events highlighted in a previous review.
No further action required.apps/event-worker/src/workers/update-environment.ts (1)
39-47: Verify the breadth of recomputation to avoid superfluous evaluations
computeBuilder.allResources(workspaceId).releaseTargets()returns every release‑target in the workspace,
but only the environment’s selectors were recomputed (computeBuilder.environments([environment]).resourceSelectors()).If other environments live in the same workspace their release‑targets will be enqueued as evaluation jobs
even though nothing changed for them, potentially wasting compute and queue bandwidth.Please double‑check that
releaseTargets()internally filters to the touched environment, or limit the scope explicitly:return computeBuilder .resources(currentResources) // or a derived list .releaseTargets();
| const dispatchExitHooks = async ( | ||
| deployments: SCHEMA.Deployment[], | ||
| exitedResource: SCHEMA.Resource, | ||
| ) => { | ||
| const events = deployments.map((deployment) => ({ | ||
| action: "deployment.resource.removed" as const, | ||
| payload: { deployment, resource: exitedResource }, | ||
| })); | ||
|
|
||
| const handleEventPromises = events.map(handleEvent); | ||
| await Promise.allSettled(handleEventPromises); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Surface & log failures from handleEvent instead of silently swallowing them
Promise.allSettled() is great for keeping the worker alive, but any rejection is currently discarded.
If an exit‑hook fails you won’t see it in the logs and downstream invariants may silently drift.
-const handleEventPromises = events.map(handleEvent);
-await Promise.allSettled(handleEventPromises);
+const results = await Promise.allSettled(events.map(handleEvent));
+results
+ .filter((r): r is PromiseRejectedResult => r.status === "rejected")
+ .forEach((r) =>
+ logger.warn({ err: r.reason }, "exit‑hook dispatch failed"),
+ );This preserves fire‑and‑forget semantics while still giving operators visibility into failures.
(You'll need to import logger at the top of the file.)
📝 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.
| const dispatchExitHooks = async ( | |
| deployments: SCHEMA.Deployment[], | |
| exitedResource: SCHEMA.Resource, | |
| ) => { | |
| const events = deployments.map((deployment) => ({ | |
| action: "deployment.resource.removed" as const, | |
| payload: { deployment, resource: exitedResource }, | |
| })); | |
| const handleEventPromises = events.map(handleEvent); | |
| await Promise.allSettled(handleEventPromises); | |
| }; | |
| const dispatchExitHooks = async ( | |
| deployments: SCHEMA.Deployment[], | |
| exitedResource: SCHEMA.Resource, | |
| ) => { | |
| const events = deployments.map((deployment) => ({ | |
| action: "deployment.resource.removed" as const, | |
| payload: { deployment, resource: exitedResource }, | |
| })); | |
| const results = await Promise.allSettled(events.map(handleEvent)); | |
| results | |
| .filter((r): r is PromiseRejectedResult => r.status === "rejected") | |
| .forEach((r) => | |
| logger.warn({ err: r.reason }, "exit‑hook dispatch failed"), | |
| ); | |
| }; |
| const dispatchExitHooks = async ( | ||
| deployments: schema.Deployment[], | ||
| exitedResource: schema.Resource, | ||
| ) => { | ||
| const events = deployments.map((deployment) => ({ | ||
| action: "deployment.resource.removed" as const, | ||
| payload: { deployment, resource: exitedResource }, | ||
| })); | ||
|
|
||
| const handleEventPromises = events.map(handleEvent); | ||
| await Promise.allSettled(handleEventPromises); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Log and examine individual exit‑hook failures
Same observation as in the updated‑resources worker: rejections from handleEvent are suppressed.
Apply the same logging pattern so operational issues don’t get lost.
Summary by CodeRabbit
Refactor
Chores
Bug Fixes