feat: release target concurrency prevalidation rule #553
Conversation
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant Manager as VersionReleaseManager
participant Rules as getRules
participant Concurrency as ReleaseTargetConcurrencyRule
participant Engine as VersionRuleEngine
Manager->>Rules: getRules(policy, releaseTarget.id)
Rules->>Concurrency: new ReleaseTargetConcurrencyRule(releaseTarget.id)
Rules-->>Manager: [ReleaseTargetConcurrencyRule, ...otherRules]
Manager->>Engine: evaluate(rules)
loop For each pre-validation rule
Engine->>Concurrency: await rule.passing()
Concurrency->>Concurrency: Query DB for active jobs
Concurrency-->>Engine: { passing: true/false, rejectionReason? }
end
sequenceDiagram
participant JobUpdate as updateJob
participant DB as Database
participant Queue as EventQueue
JobUpdate->>JobUpdate: getIsJobJustCompleted(previousStatus, newStatus)
alt Job just completed
JobUpdate->>DB: getReleaseTarget(jobId)
DB-->>JobUpdate: releaseTarget data
JobUpdate->>Queue: enqueue EvaluateReleaseTarget with releaseTarget key and data
else Job not completed
JobUpdate-->>JobUpdate: return early
end
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 0
🧹 Nitpick comments (1)
packages/rule-engine/src/rules/release-target-concurrency-rule.ts (1)
1-42: Consider adding a comment describing the rule's purpose.While the code is well-structured and self-documenting, a brief class-level JSDoc comment explaining the rule's purpose and behavior would enhance maintainability for future developers.
import { and, eq, notInArray, takeFirstOrNull } from "@ctrlplane/db"; import { db } from "@ctrlplane/db/client"; import * as schema from "@ctrlplane/db/schema"; import { exitedStatus } from "@ctrlplane/validators/jobs"; import type { PreValidationRule } from "../types"; +/** + * Rule that prevents concurrent jobs for the same release target. + * Fails validation if there are any active (non-exited status) jobs + * already associated with the specified release target ID. + */ export class ReleaseTargetConcurrencyRule implements PreValidationRule { public readonly name = "ReleaseTargetConcurrencyRule";
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/rule-engine/src/manager/version-manager-rules.ts(2 hunks)packages/rule-engine/src/manager/version-manager.ts(1 hunks)packages/rule-engine/src/manager/version-rule-engine.ts(1 hunks)packages/rule-engine/src/rules/release-target-concurrency-rule.ts(1 hunks)packages/rule-engine/src/types.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/types.tspackages/rule-engine/src/manager/version-rule-engine.tspackages/rule-engine/src/manager/version-manager.tspackages/rule-engine/src/rules/release-target-concurrency-rule.tspackages/rule-engine/src/manager/version-manager-rules.ts
🧬 Code Graph Analysis (1)
packages/rule-engine/src/manager/version-manager-rules.ts (4)
packages/rule-engine/src/types.ts (2)
FilterRule(31-36)PreValidationRule(43-46)packages/rule-engine/src/manager/version-rule-engine.ts (1)
Version(11-17)packages/rule-engine/src/rules/release-target-concurrency-rule.ts (1)
ReleaseTargetConcurrencyRule(8-42)packages/db/src/schema/policy.ts (1)
policy(42-62)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (11)
packages/rule-engine/src/types.ts (1)
45-45: Good update to support asynchronous rule validation.The interface now allows
passing()to return either a synchronousPreValidationResultor aPromise<PreValidationResult>, enabling asynchronous validation operations like database queries. This change correctly enables the new release target concurrency implementation.packages/rule-engine/src/manager/version-rule-engine.ts (1)
65-65: Correctly addedawaitto handle async validation.Adding the
awaitkeyword ensures proper handling of the potentially asynchronouspassing()method that was updated in thePreValidationRuleinterface. This is necessary for the new concurrency rule to work correctly.packages/rule-engine/src/manager/version-manager.ts (1)
168-168: Properly addedreleaseTargetIdparameter to rule creation.The
evaluatemethod now correctly passesthis.releaseTarget.idto thegetRulesfunction, enabling the creation of the release target concurrency validation rule with the appropriate ID to check for active jobs.packages/rule-engine/src/manager/version-manager-rules.ts (3)
4-4: Added import for the new concurrency rule.The import for
ReleaseTargetConcurrencyRuleis correctly added to support the implementation.
70-73: UpdatedgetRulesfunction signature correctly.The function now accepts a
releaseTargetIdparameter, which is necessary for creating the concurrency rule that checks for active jobs on a specific release target.
74-78: Properly integrated the concurrency rule with existing validation.The implementation correctly prepends a new instance of
ReleaseTargetConcurrencyRuleto the existing list of version approval rules. This ensures concurrency validation happens before other approval rules are evaluated, which is an appropriate order of operations.packages/rule-engine/src/rules/release-target-concurrency-rule.ts (5)
1-5: Good job with the imports.The imports are well-organized, separating external database-related utilities from schema definitions and validators. The
exitedStatusimport will be used effectively to filter out jobs that have already completed.
8-11: Clear class definition with descriptive name.The class properly implements the
PreValidationRuleinterface and has a clear, self-documenting name that indicates its purpose. The readonly name property provides good identification for logging and debugging.
11-11: Constructor is concise and effective.Good use of TypeScript's private readonly parameter to ensure the releaseTargetId is immutable and properly encapsulated within the class.
13-34: Well-structured database query.The database query is well-constructed with clear joins between the necessary tables to find active jobs for the specified release target. The use of
limit(1)andtakeFirstOrNullis efficient since you only need to know if any active job exists.The Promise chain approach is clean and aligns with the coding guidelines that allow for various error handling patterns beyond strict try/catch blocks.
35-41: Clear response handling with informative error message.The method provides appropriate responses for both success and failure cases. The rejection reason includes the releaseTargetId, which will be helpful for debugging and error reporting.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/job-dispatch/src/job-creation.ts (1)
31-33: 🛠️ Refactor suggestion
_.mergemutatesjobAgent.config– may leak state between jobsUsing
_.merge(jobAgent.config, runbook.jobAgentConfig)directly modifiesjobAgent.configin-place.
If the samejobAgentobject is reused elsewhere in the current transaction (or cached after the call), its config will now include runbook-specific overrides, leading to very hard-to-trace bugs.- jobAgentConfig: _.merge(jobAgent.config, runbook.jobAgentConfig), + // clone first to avoid mutating values coming from the DB layer + jobAgentConfig: _.merge({}, jobAgent.config, runbook.jobAgentConfig),
🧹 Nitpick comments (2)
packages/job-dispatch/src/job-creation.ts (1)
14-17: Typo in log message hurts observability
"Triger triggered runbook job …"contains a misspelling (Triger). Small, but log lines are often grepped verbatim; the typo makes searches harder.-logger.info(`Triger triggered runbook job ${runbook.name}`, { +logger.info(`Triggering runbook job ${runbook.name}`, {packages/job-dispatch/src/job-update.ts (1)
84-91: Minor: replace linear-search array withSetfor O(1) lookup
exitedStatus.includes(…)is fine for small arrays, but since this helper can be called on every job update, switching to aSetremoves the repeated linear scan.-const getIsJobJustCompleted = ( - previousStatus: JobStatus, - newStatus: JobStatus, -) => { - const isPreviousStatusExited = exitedStatus.includes(previousStatus); - const isNewStatusExited = exitedStatus.includes(newStatus); - return !isPreviousStatusExited && isNewStatusExited; -}; +const exitedStatusSet = new Set<JobStatus>(exitedStatus); +const getIsJobJustCompleted = ( + previousStatus: JobStatus, + newStatus: JobStatus, +) => + !exitedStatusSet.has(previousStatus) && exitedStatusSet.has(newStatus);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/job-dispatch/src/job-creation.ts(1 hunks)packages/job-dispatch/src/job-failure.ts(0 hunks)packages/job-dispatch/src/job-update.ts(3 hunks)
💤 Files with no reviewable changes (1)
- packages/job-dispatch/src/job-failure.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/job-creation.tspackages/job-dispatch/src/job-update.ts
🧬 Code Graph Analysis (1)
packages/job-dispatch/src/job-update.ts (6)
packages/db/src/schema/job.ts (1)
JobStatus(141-141)packages/validators/src/jobs/index.ts (1)
exitedStatus(35-43)packages/db/src/client.ts (1)
db(15-15)packages/db/src/common.ts (1)
takeFirst(9-13)packages/db/src/schema/release.ts (1)
releaseTarget(20-42)packages/events/src/index.ts (1)
getQueue(28-34)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: Typecheck
🔇 Additional comments (1)
packages/job-dispatch/src/job-update.ts (1)
6-6:✅ Verification successful
Ensure
Channel.EvaluateReleaseTargetexistsThe new import expects
Channelto expose anEvaluateReleaseTargetmember.
A missing enum entry will surface only at runtime in JavaScript output, so please double-check.Run:
🏁 Script executed:
#!/usr/bin/env bash # Expect at least one occurrence of the enum/key. rg -A3 -N "enum.+Channel" | grep -n "EvaluateReleaseTarget"Length of output: 57
🏁 Script executed:
#!/usr/bin/env bash # Locate the definition of Channel in the events package rg -n "export enum Channel" -A5 rg -n "export type Channel" -A5 # Search for any occurrence of EvaluateReleaseTarget rg -n "EvaluateReleaseTarget"Length of output: 2500
Confirmed: Channel.EvaluateReleaseTarget Is Defined
TheChannelenum inpackages/events/src/types.ts(line 32) includes anEvaluateReleaseTargetmember, so importingChannel.EvaluateReleaseTargetfrom@ctrlplane/eventsis valid—no changes needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/job-dispatch/src/job-update.ts (1)
151-156: Properly handles missing release targetsThe implementation now includes a null check before enqueueing the evaluation task, addressing the concern raised in the previous review. This ensures that non-release jobs won't cause errors during the update process.
Consider adding a debug log when no release target is found, similar to what was suggested in the previous review:
const releaseTarget = await getReleaseTarget(db, jobId); - if (releaseTarget == null) return updatedJob; + if (releaseTarget == null) { + log.debug(`Job ${jobId} is not part of a release pipeline – skipping release-target evaluation`); + return updatedJob; + } await getQueue(Channel.EvaluateReleaseTarget).add( `${releaseTarget.resourceId}-${releaseTarget.environmentId}-${releaseTarget.deploymentId}`, releaseTarget, );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/job-dispatch/src/job-update.ts(3 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/job-dispatch/src/job-update.ts
🧬 Code Graph Analysis (1)
packages/job-dispatch/src/job-update.ts (6)
packages/db/src/schema/job.ts (1)
JobStatus(141-141)packages/validators/src/jobs/index.ts (1)
exitedStatus(35-43)packages/db/src/client.ts (1)
db(15-15)packages/db/src/common.ts (1)
takeFirstOrNull(15-20)packages/db/src/schema/release.ts (1)
releaseTarget(20-42)packages/events/src/index.ts (1)
getQueue(28-34)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (4)
packages/job-dispatch/src/job-update.ts (4)
3-3: Added new import for better error handlingThe addition of
takeFirstOrNullis a good improvement for error handling as it allows gracefully handling cases where no matching records are found instead of throwing exceptions.
84-91: Good extraction of job completion detection logicThis helper function cleanly encapsulates the logic for detecting when a job transitions into a completed state. The implementation correctly checks if a job has moved from a non-exited state to an exited state.
93-114: Well-structured database query with proper error handlingThis function properly retrieves release target information using a series of joins, and importantly, uses
takeFirstOrNullto handle cases where no release target is found (non-release jobs). This addresses the issues raised in previous reviews.
145-149: Clean implementation of job completion detectionThe code now uses the extracted helper function to clearly check for job completion, making the logic more readable and maintainable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/api/src/router/release-target.ts (1)
164-174: Consider adding error handling for the data transformation.While the data transformation using lodash is well-structured, it assumes that
jobsByTarget[0]will always exist when accessingreleaseTarget. Although this is likely true given the query structure, adding a defensive check would improve robustness.- const releaseTarget = jobsByTarget[0]!.release_target; + const releaseTarget = jobsByTarget[0]?.release_target; + if (!releaseTarget) { + return null; + }Then filter out nulls before returning:
- .value(); + .filter(Boolean) + .value();apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/_components/release-cell/DeploymentVersionEnvironmentCell.tsx (1)
198-201: Consider adding null check before flatMap operation.While
targetsWithActiveJobs ?? []provides a default empty array if the data is null, it would be more explicit to check for null/undefined before calling flatMap.- const allActiveJobs = (targetsWithActiveJobs ?? []).flatMap((t) => t.jobs); + const allActiveJobs = targetsWithActiveJobs + ? targetsWithActiveJobs.flatMap((t) => t.jobs ?? []) + : [];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/_components/release-cell/DeploymentVersionEnvironmentCell.tsx(5 hunks)packages/api/src/router/release-target.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/api/src/router/release-target.tsapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/_components/release-cell/DeploymentVersionEnvironmentCell.tsx
🧬 Code Graph Analysis (1)
packages/api/src/router/release-target.ts (4)
packages/api/src/trpc.ts (1)
protectedProcedure(173-173)packages/validators/src/jobs/index.ts (1)
exitedStatus(35-43)packages/db/src/schema/job.ts (1)
job(75-107)packages/db/src/schema/release.ts (1)
releaseTarget(20-42)
⏰ 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: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (8)
packages/api/src/router/release-target.ts (3)
1-1: Appropriate use of lodash for data transformation.The addition of lodash is well-justified for the complex data transformation operations performed in the new
activeJobsprocedure.
5-8: Import additions support the new query functionality.The added imports (
notInArrayandexitedStatus) are appropriately used to filter out completed jobs in the query.
83-175: Well-implemented active jobs query with proper authorization.The new
activeJobsprocedure follows good patterns:
- Reuses the same input validation and authorization logic as the existing procedures
- Properly joins necessary tables to fetch active jobs
- Uses the
exitedStatusarray to filter for non-completed jobs- Effectively transforms the data using lodash for grouping by target
The query properly handles optional filter parameters and returns a well-structured response that includes both release target data and its associated jobs.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/_components/release-cell/DeploymentVersionEnvironmentCell.tsx (5)
7-7: Added necessary icon import.The
IconClockaddition is appropriate for the new blocked state visualization.
50-83: Well-implemented blocking component with clear user feedback.The
BlockedByActiveJobsCellcomponent provides:
- Clear visual indication of the blocked state with an appropriate icon
- Helpful message explaining why the version is blocked
- A link to the deployment's releases page where users can view active jobs
This provides good UX by not only showing that something is blocked but also directing users to where they can see more information.
133-140: Added query for active jobs with proper loading state handling.The implementation of the
activeJobsquery is appropriate, fetching data based on the current environment and deployment context.
153-158: Correctly updated loading condition.The loading state was properly updated to include the new query's loading status.
198-203: Clear implementation of blocking logic.The implementation:
- Flattens all jobs from all targets
- Checks if any jobs belong to a different version than the current one
- Renders the blocking component when appropriate
This correctly implements the concurrency constraint in the UI.
Summary by CodeRabbit
New Features
Bug Fixes
Chores