feat: policy target computation#490
Conversation
WalkthroughThis change introduces a new many-to-many mapping between policy targets and release targets through the addition of the Changes
Sequence Diagram(s)sequenceDiagram
participant User/API
participant WebService
participant DB
participant Selector
participant JobDispatch
User/API->>WebService: Create/Update Resource or Policy
WebService->>DB: Upsert resource/policy
WebService->>Selector: Recompute selectors (env, deployment, policy)
Selector->>DB: Update computed selector tables
WebService->>JobDispatch: Replace release targets & dispatch exit hooks
JobDispatch->>DB: Upsert release targets (transactional)
JobDispatch->>DB: Update computed policy-target-release-target mappings
JobDispatch->>JobDispatch: Enqueue evaluation jobs
sequenceDiagram
participant Worker
participant DB
participant Selector
participant JobDispatch
Worker->>DB: Fetch affected deployment/resource/environment
Worker->>Selector: Recompute selectors as needed
Selector->>DB: Update computed selector tables
Worker->>JobDispatch: Upsert release targets & dispatch exit hooks
JobDispatch->>DB: Upsert release targets (transactional)
JobDispatch->>DB: Update computed policy-target-release-target mappings
JobDispatch->>JobDispatch: Enqueue evaluation jobs
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 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...
⏰ Context from checks skipped due to timeout of 90000ms (5)
🔇 Additional comments (11)
✨ Finishing Touches
🪧 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
|
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (3)
apps/webservice/src/app/api/v1/resources/route.ts (1)
77-98: 🛠️ Refactor suggestionImproved efficiency with policy target computation, but missing error handling.
This change updates the asynchronous flow to include policy-related selector computation, ensuring policy targets are properly updated when resources change. The Promise chaining pattern ensures that queue jobs are only added after all selector replacements are complete.
While the implementation is more efficient than sequential awaits, it's missing error handling. If any selector replacement fails, the error will be unhandled and might prevent queue job additions. Consider adding a
.catch()handler:Promise.all([ selector(db) .compute() .allEnvironments(workspaceId) .resourceSelectors() .replace(), selector(db) .compute() .allDeployments(workspaceId) .resourceSelectors() .replace(), selector(db) .compute() .allPolicies(workspaceId) .resourceSelectors() .replace(), -]).then(() => +]).then(() => Promise.all([ getQueue(Channel.NewResource).addBulk(insertJobs), getQueue(Channel.UpdatedResource).addBulk(updateJobs), - ]), + ]), +).catch((error) => { + console.error("Error during selector replacement or queue operations:", error); + // Consider if you want to log this only or also throw to propagate the error +}),packages/job-dispatch/src/resource/handle-provider-scan.ts (1)
47-68: 🛠️ Refactor suggestionUpdated selector computation includes policies, but consider error handling.
The change adds policy target computation alongside environment and deployment selectors, ensuring that all relevant selectors are updated before adding jobs to the queue. The Promise chaining approach is efficient and maintains correct sequencing.
Although there's existing error handling in the outer try/catch block (lines 22-76), this specific Promise chain should have its own error handler to prevent unhandled promise rejections that could bypass the outer catch block.
Promise.all([ selector(tx) .compute() .allEnvironments(workspaceId) .resourceSelectors() .replace(), selector(tx) .compute() .allDeployments(workspaceId) .resourceSelectors() .replace(), selector(tx) .compute() .allPolicies(workspaceId) .resourceSelectors() .replace(), ]).then(() => Promise.all([ getQueue(Channel.NewResource).addBulk(insertJobs), getQueue(Channel.UpdatedResource).addBulk(updateJobs), - ]), + ]), +).catch((error) => { + log.error("Error during selector replacement or queue operations:", { error }); + // No need to rethrow as the outer catch will handle it if neededapps/pty-proxy/src/controller/agent-socket.ts (1)
141-157: 🛠️ Refactor suggestionAdded policy target computation with proper chaining, but missing error handling.
This change adds policy target computation to ensure that policy selectors are properly updated when an agent's resource is updated. The Promise chaining approach ensures that queue operations occur only after all selector replacements are complete.
The implementation lacks error handling. If any selector replacement fails, the error will be unhandled and might prevent the queue addition. Add a
.catch()handler to properly manage potential errors:Promise.all([ selector(db) .compute() .allEnvironments(this.workspaceId) .resourceSelectors() .replace(), selector(db) .compute() .allDeployments(this.workspaceId) .resourceSelectors() .replace(), selector(db) .compute() .allPolicies(this.workspaceId) .resourceSelectors() .replace(), -]).then(() => getQueue(Channel.UpdatedResource).add(res.id, res)); +]).then(() => getQueue(Channel.UpdatedResource).add(res.id, res)) + .catch((error) => { + logger.error("Error during selector replacement or queue operations:", { error, resourceId: res.id }); + // Consider if you want to log this only or also throw to propagate the error + });
🧹 Nitpick comments (1)
packages/db/src/selectors/compute/policy-builder.ts (1)
166-334: WorkspacePolicyBuilder offers a convenient scoped approach.
Similar logic asPolicyBuilderbut scoped byworkspaceId. The stepwise delete-then-insert pattern is consistent, preserving clarity. As an optional future enhancement, consider logging or chunking large sets to handle performance constraints in very large workspaces.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
apps/event-worker/src/workers/new-deployment.ts(3 hunks)apps/event-worker/src/workers/update-deployment.ts(2 hunks)apps/event-worker/src/workers/update-environment.ts(1 hunks)apps/event-worker/src/workers/update-resource-variable.ts(2 hunks)apps/pty-proxy/src/controller/agent-socket.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/db/package.json(1 hunks)packages/db/src/policies/create.ts(2 hunks)packages/db/src/policies/update.ts(2 hunks)packages/db/src/schema/policy.ts(3 hunks)packages/db/src/selectors/compute/compute.ts(2 hunks)packages/db/src/selectors/compute/policy-builder.ts(1 hunks)packages/db/src/selectors/query/builder.ts(1 hunks)packages/db/src/selectors/query/deployment-selector.ts(1 hunks)packages/job-dispatch/src/resource/handle-provider-scan.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.
apps/event-worker/src/workers/update-resource-variable.tspackages/db/src/selectors/compute/compute.tspackages/job-dispatch/src/resource/handle-provider-scan.tspackages/db/src/policies/update.tsapps/pty-proxy/src/controller/agent-socket.tsapps/webservice/src/app/api/v1/resources/[resourceId]/route.tsapps/event-worker/src/workers/new-deployment.tspackages/db/src/selectors/query/builder.tsapps/webservice/src/app/api/v1/resources/route.tspackages/db/src/selectors/query/deployment-selector.tsapps/event-worker/src/workers/update-environment.tsapps/event-worker/src/workers/update-deployment.tspackages/db/src/schema/policy.tspackages/db/src/policies/create.tspackages/db/src/selectors/compute/policy-builder.ts
🧬 Code Graph Analysis (8)
apps/event-worker/src/workers/update-resource-variable.ts (4)
packages/db/src/selectors/index.ts (1)
selector(18-18)packages/db/src/client.ts (1)
db(15-15)packages/db/src/schema/resource.ts (1)
resource(59-87)packages/events/src/index.ts (1)
getQueue(28-34)
apps/event-worker/src/workers/new-deployment.ts (4)
packages/db/src/common.ts (1)
Tx(22-22)packages/db/src/selectors/index.ts (1)
selector(18-18)packages/db/src/client.ts (1)
db(15-15)packages/db/src/schema/job.ts (1)
job(74-106)
packages/db/src/selectors/query/builder.ts (5)
packages/db/src/common.ts (1)
Tx(22-22)packages/db/src/selectors/query/builder-types.ts (1)
WhereBuilder(22-31)packages/validators/src/deployments/conditions/deployment-condition.ts (1)
DeploymentCondition(14-19)packages/db/src/schema/deployment.ts (1)
Deployment(104-104)packages/db/src/selectors/query/deployment-selector.ts (1)
DeploymentOutputBuilder(29-40)
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-environment.ts (2)
packages/db/src/selectors/index.ts (1)
selector(18-18)packages/db/src/client.ts (1)
db(15-15)
apps/event-worker/src/workers/update-deployment.ts (2)
packages/db/src/client.ts (1)
db(15-15)packages/db/src/selectors/index.ts (1)
selector(18-18)
packages/db/src/schema/policy.ts (3)
packages/db/src/schema/deployment.ts (1)
deployment(68-92)packages/db/src/schema/environment.ts (1)
environment(59-84)packages/db/src/schema/resource.ts (1)
resource(59-87)
packages/db/src/policies/create.ts (2)
packages/logger/src/index.ts (1)
logger(44-44)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)
🔇 Additional comments (27)
packages/db/package.json (1)
43-43: Appropriate dependency addition for logging features.Adding the
@ctrlplane/loggerworkspace dependency aligns well with the PR's goal of introducing logging capabilities within the database package for error tracking during selector replacement operations.apps/event-worker/src/workers/update-environment.ts (1)
223-227: Newly added policy selector computation looks good.Adding the recomputation of policy environment selectors before processing release targets ensures that policy relationships are updated properly when environment selectors change. This follows the pattern of ensuring policy selector recomputation occurs as a prerequisite step before queuing evaluation jobs or dispatching events.
packages/db/src/selectors/query/builder.ts (3)
1-6: Imports correctly updated to support new deployment functionality.The imports have been properly updated to include DeploymentCondition type and Deployment entity, which are necessary for the newly implemented deployments() method.
7-14: Import structure for DeploymentOutputBuilder looks good.The import of DeploymentOutputBuilder from the deployment-selector.js file is correctly added to support the new deployments() method implementation.
21-26: Well-implemented deployments() method.The deployments() method is implemented correctly, returning a new WhereBuilder with the appropriate type parameters (DeploymentCondition and Deployment) and using the transaction object with DeploymentOutputBuilder. This follows the same pattern as the other selector methods in the class.
packages/db/src/selectors/compute/compute.ts (2)
10-10: Good import addition for the new policy builders.The import for PolicyBuilder and WorkspacePolicyBuilder from the policy-builder.js file is correctly added to support the new policy-related methods.
31-37: Well-structured policy selector computation methods.The two new methods - allPolicies() and policies() - follow the consistent pattern established by the existing methods in the class. They properly instantiate and return the appropriate builder objects with the transaction object and provided identifiers.
packages/db/src/policies/create.ts (4)
4-5: LGTM! Added logger import.Properly importing the logger module for error handling.
9-9: LGTM! Added selector import.Correct import statement for the selector module used in policy target computation.
14-16: LGTM! Created scoped logger.Good practice to create a module-scoped logger with appropriate context.
98-123: Well-implemented policy target computation with proper error handling.The implementation correctly computes policy targets for deployments, environments, and resources after policy creation, with appropriate error handling for each operation. The non-blocking approach using individual
.catch()handlers for each operation is particularly good.This is a good implementation pattern that:
- Doesn't block the function completion with
await- Handles errors individually for each operation
- Provides specific error messages that include the policy ID
- Allows the function to return the policy data regardless of selector computation failures
The asynchronous operations are correctly separated from the main function flow, ensuring that selector computations don't delay the response while still maintaining data consistency.
apps/event-worker/src/workers/update-resource-variable.ts (1)
1-1: Good addition of the selector import.The import of
selectorfrom "@ctrlplane/db" is used appropriately for policy resource selector computation.packages/db/src/policies/update.ts (3)
4-5: Good import additions for the new functionality.The logger and selector imports are correctly added to support the new policy target computation features.
Also applies to: 9-9
12-14: Good practice with structured logging.Creating a child logger with a module identifier is a good practice for structured logging, making it easier to filter and trace logs.
203-231:Details
✅ Verification successful
Well-implemented policy selector computation with proper error handling.
The code correctly implements computation and replacement of deployment, environment, and resource selectors for policies, with good error handling that allows the main function to complete even if selector computations encounter issues.
However, I noticed these operations are not awaited, meaning the function returns before selectors are fully recomputed. This is acceptable if callers don't need to wait for these operations to complete.
🏁 Script executed:
#!/bin/bash # Check if any callers of updatePolicyInTx are awaiting its result and then performing operations that depend on selectors being updated # Search for usages of updatePolicyInTx function rg -A 5 "updatePolicyInTx" --type tsLength of output: 1069
Asynchronous selector updates verified – No action required
The code in
packages/db/src/policies/update.tscorrectly triggers asynchronous replacements for deployment, environment, and resource selectors with proper error handling. The non-awaited.replace()operations are by design; our verification confirms that callers (e.g., inpackages/api/src/router/policy.ts) invokeupdatePolicyInTxwithout awaiting its result, meaning they do not depend on immediate completion of these operations. This is acceptable given the intended eventual consistency of selector updates.apps/event-worker/src/workers/update-deployment.ts (1)
3-6: Good import additions for database operations.The new imports are correctly added to support the policy computation functionality.
apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts (1)
5-5: Good addition of the selector import.The import of
selectorfrom "@ctrlplane/db" is correctly added to support the selector computation functionality.apps/event-worker/src/workers/new-deployment.ts (2)
4-4: Import usage looks consistent.
No issues identified with addingselectorto the imports, given that it’s used below.
51-62: Recompute logic appears correct.
The newrecomputeAllPolicyDeploymentsfunction correctly fetches the system, verifies its existence, and then performs an all-policies deployment-selector replacement. Error handling is straightforward and should suffice, though you might consider logging or detailing why the system is not found.packages/db/src/selectors/query/deployment-selector.ts (3)
1-12: Import statements are well-structured.
They correctly bring in types, operators, and references needed for building deployment queries.
13-27: Recursive condition builder looks correct.
It properly handles empty conditions, compound “and/or” operators, and the negation flag. The fallback tosql\FALSE`` when no conditions exist helps avoid generating invalid queries.
29-40: Output builder design is straightforward and maintainable.
Returningundefinedwhen no condition is present prevents unnecessary filtering. This pattern is simple and consistent with the rest of the code.packages/db/src/schema/policy.ts (4)
24-26: New imports for referencing related entities look correct.
They align with the new computed tables referencingdeployment,environment, andresource.
68-79: Composite primary key for computedPolicyDeployment.
Defining a composite PK (policyId,deploymentId) with cascading deletes on both references is appropriate for a many-to-many join table.
81-92: computedPolicyEnvironment structure matches the deployment variant.
Similarly uses a composite PK and consistent cascade. This maintains uniformity across computed policy schemas.
94-105: computedPolicyResource schema is consistent.
All three new computed tables follow the same pattern, ensuring a cohesive approach to storing policy relationships.packages/db/src/selectors/compute/policy-builder.ts (1)
8-64: PolicyBuilder class logic is structurally sound.
Each of the three methods (environmentSelectors, deploymentSelectors, resourceSelectors) cleanly performs:
- A delete of existing computed records tied to the policy IDs.
- A query for policies and processing their targets.
- A query for matching environments/deployments/resources.
- Flattened insertion data via ReplaceBuilder.
This mechanism is easy to follow and appears to align with how the system’s policy-to-target relationships are recalculated.
| const recomputeAllPolicyDeployments = async (systemId: string) => { | ||
| const system = await db.query.system.findFirst({ | ||
| where: eq(schema.system.id, systemId), | ||
| }); | ||
| if (system == null) throw new Error(`System not found: ${systemId}`); | ||
| const { workspaceId } = system; | ||
| await selector(db) | ||
| .compute() | ||
| .allPolicies(workspaceId) | ||
| .deploymentSelectors() | ||
| .replace(); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for the recomputeAllPolicyDeployments function.
While the function implementation correctly computes policy deployment selectors, it lacks error handling in the selector replacement chain. If the replacement operation fails, the error will propagate to the caller.
const recomputeAllPolicyDeployments = async (systemId: string) => {
const system = await db.query.system.findFirst({
where: eq(schema.system.id, systemId),
});
if (system == null) throw new Error(`System not found: ${systemId}`);
const { workspaceId } = system;
await selector(db)
.compute()
.allPolicies(workspaceId)
.deploymentSelectors()
- .replace();
+ .replace()
+ .catch((error) => {
+ console.error(
+ `Error replacing deployment selectors for workspace ${workspaceId}:`,
+ error
+ );
+ });
};📝 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 recomputeAllPolicyDeployments = async (systemId: string) => { | |
| const system = await db.query.system.findFirst({ | |
| where: eq(schema.system.id, systemId), | |
| }); | |
| if (system == null) throw new Error(`System not found: ${systemId}`); | |
| const { workspaceId } = system; | |
| await selector(db) | |
| .compute() | |
| .allPolicies(workspaceId) | |
| .deploymentSelectors() | |
| .replace(); | |
| }; | |
| const recomputeAllPolicyDeployments = async (systemId: string) => { | |
| const system = await db.query.system.findFirst({ | |
| where: eq(schema.system.id, systemId), | |
| }); | |
| if (system == null) throw new Error(`System not found: ${systemId}`); | |
| const { workspaceId } = system; | |
| await selector(db) | |
| .compute() | |
| .allPolicies(workspaceId) | |
| .deploymentSelectors() | |
| .replace() | |
| .catch((error) => { | |
| console.error( | |
| `Error replacing deployment selectors for workspace ${workspaceId}:`, | |
| error | |
| ); | |
| }); | |
| }; |
There was a problem hiding this comment.
we do the catch below. I dont' want the recompute stuff to happen if it errors
There was a problem hiding this comment.
@adityachoudhari26 You're absolutely right. I see now that you already have error handling at the worker level with the .catch() block that logs errors. Your current design makes sense - allowing errors to propagate up from recomputeAllPolicyDeployments so the entire operation stops if something fails rather than swallowing the error at the lower level.
Sorry for the unnecessary suggestion!
(_/)
(•ㅅ•)
/ づ
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts (1)
105-125: Asynchronous recomputation before queueing resource updates.You're launching the selector recomputation in the background (with Promise.all). This returns immediately to the client rather than waiting for these calls to complete, which can be acceptable if you deliberately want asynchronous behavior. However, note:
- Partial Success: If one recompute call fails, the others can succeed, but you never enqueue the updated resource if the
.then()chain doesn't run. The.catch()only logs the error.- Client Awareness: The request completes successfully for the client even if these computations fail later. If you need synchronous confirmation, consider
await Promise.all([...])before returning.If the background approach is intentional, this is fine. Otherwise, consider blocking until all recomputations succeed or returning an error if they fail.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/event-worker/src/workers/new-deployment.ts(3 hunks)apps/event-worker/src/workers/update-deployment.ts(2 hunks)apps/event-worker/src/workers/update-resource-variable.ts(2 hunks)apps/pty-proxy/src/controller/agent-socket.ts(2 hunks)apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts(2 hunks)apps/webservice/src/app/api/v1/resources/route.ts(3 hunks)packages/db/drizzle/0091_wise_agent_zero.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/policy-relations.ts(2 hunks)packages/db/src/schema/policy.ts(3 hunks)packages/db/src/selectors/compute/policy-builder.ts(1 hunks)packages/job-dispatch/src/resource/handle-provider-scan.ts(2 hunks)packages/rule-engine/src/db/get-applicable-policies.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/db/drizzle/0091_wise_agent_zero.sql
🚧 Files skipped from review as they are similar to previous changes (9)
- apps/webservice/src/app/api/v1/resources/route.ts
- packages/job-dispatch/src/resource/handle-provider-scan.ts
- packages/db/drizzle/meta/_journal.json
- apps/event-worker/src/workers/update-resource-variable.ts
- apps/event-worker/src/workers/update-deployment.ts
- apps/pty-proxy/src/controller/agent-socket.ts
- apps/event-worker/src/workers/new-deployment.ts
- packages/db/src/schema/policy.ts
- packages/db/src/selectors/compute/policy-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.
apps/webservice/src/app/api/v1/resources/[resourceId]/route.tspackages/db/src/schema/policy-relations.tspackages/rule-engine/src/db/get-applicable-policies.ts
🧬 Code Graph Analysis (2)
packages/db/src/schema/policy-relations.ts (1)
packages/db/src/schema/policy.ts (5)
policyTarget(52-66)policy(34-50)computedPolicyTargetDeployment(68-79)computedPolicyTargetEnvironment(83-94)computedPolicyTargetResource(98-109)
packages/rule-engine/src/db/get-applicable-policies.ts (2)
packages/rule-engine/src/types.ts (1)
ReleaseTargetIdentifier(82-86)packages/db/src/schema/policy.ts (5)
PolicyTarget(192-192)ComputedPolicyTargetDeployment(80-81)ComputedPolicyTargetEnvironment(95-96)ComputedPolicyTargetResource(110-111)policy(34-50)
⏰ 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 (14)
apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts (3)
5-9: Imports look appropriate.The additions from
@ctrlplane/dband@ctrlplane/loggerare consistent with the new selector logic and logging functionality.
17-17: Child logger initialization is clear.Defining a child logger for this module improves log clarity and traceability.
102-104: Minor spacing and structure.No functional impact here, but the extra blank/modified lines are fine as a structural choice.
packages/db/src/schema/policy-relations.ts (5)
3-9: New imports aligned with computed policy targets.These imports match the newly introduced computed tables and existing policy entities.
33-45: PolicyTarget relations to computed entities.Declaring
computedDeployments,computedEnvironments, andcomputedResourcesas many-to-one relations is correct for bridging with the new join tables. This is consistent with the policy → policyTarget → computed tables structure.
46-55: computedPolicyTargetDeploymentRelations references.Linking
policyTargetIdtopolicyTarget.idin the relation is correct in code. However, ensure the underlying table schema truly referencespolicyTarget.idrather thanpolicy.id. (Some of the prior snippets suggest a mismatch where the table referencespolicy.id. Confirm the migration definitions align with this code to avoid runtime foreign key errors.)
56-65: computedPolicyTargetEnvironmentRelations references.Same verification concern for
policyTargetId→policyTarget.id. Double-check the underlying table definitions.
66-75: computedPolicyTargetResourceRelations references.Again, confirm that
policy_target_idreferencespolicy_target.idin the database schema, matching the code’spolicyTargetId → policyTarget.id.packages/rule-engine/src/db/get-applicable-policies.ts (6)
3-3: New imports are appropriate.The updated import from
@ctrlplane/db(and, desc, eq) is typical for filtering and sorting queries.
10-16: Documentation updates.The expanded doc comments clearly describe how the function checks selectors. Improvements to descriptive wording help clarify expected usage.
22-24: Doc parameter and return explanation.Providing explicit param/return details helps ensure that maintainers understand the function’s purpose.
26-69: Synchronous match logic using precomputed arrays.Shifting to in-memory checks using
computedDeployments,computedEnvironments, andcomputedResourcesis succinct and avoids repetitive DB queries. This should improve performance. The short-circuit pattern (returning false if no selectors) and direct.some(...)checks for each dimension are clear and match the updated system design.
105-110: Eager loading of computed policy targets.Requesting
computedDeployments,computedEnvironments, andcomputedResourcesalongside eachpolicyTargetensuresmatchPolicyTargetForResourcecan operate entirely on pre-fetched, in-memory data. This is aligned with the new design.
121-125: Filtering policies using the new utility.Replacing any old asynchronous logic with a direct
.some(...)check is simpler and more predictable. Nicely done.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/db/src/selectors/compute/replace-builder.ts (1)
16-32: Good addition of error handling, but logging can be improved.Adding a try-catch block with error logging improves the robustness of the replace operation. However, there are a few areas for improvement:
- Consider using a structured logging library instead of console.log/error for better log filtering and formatting in production.
- Line 23 duplicates the same message from line 17, creating redundant logs.
- The log at line 21 outputs the entire
valsarray which could be very verbose for large datasets. Consider logging only the count:console.log("replace builder values finished", vals.length).try { - console.log("replace builder insert", this.table.name); + console.log(`replace builder starting for ${this.table.name}`); await this.deletePrevious(tx); console.log("replace builder delete finished"); const vals = await this.values(tx); - console.log("replace builder values finished", vals); + console.log(`replace builder values finished, count: ${vals.length}`); if (vals.length === 0) return; - console.log("replace builder insert", this.table.name); + console.log(`replace builder inserting ${vals.length} values into ${this.table.name}`); return tx .insert(this.table) .values(vals) .onConflictDoNothing() .returning(); } catch (e) { - console.error(e, `Error replacing ${this.table.name}, ${String(e)}`); + console.error(`Error replacing ${this.table.name}:`, e); throw e; }packages/api/src/router/policy.ts (1)
282-285: Transaction handling has been modifiedThe update mutation now calls
updatePolicyInTxdirectly withctx.dbinstead of wrapping it in an explicit transaction. This change aligns with the broader system refactor whereupdatePolicyInTxnow handles asynchronous recomputation and replacement of policy selectors.The commented-out code should be removed for clarity and to avoid confusion about the current implementation. There's no need to keep this reference in the production code.
.mutation(({ ctx, input }) => updatePolicyInTx(ctx.db, input.id, input.data), ), - // ctx.db.transaction((tx) => updatePolicyInTx(tx, input.id, input.data)), - // ),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/api/src/router/policy.ts(1 hunks)packages/db/drizzle/0091_bizarre_jetstream.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/policy.ts(3 hunks)packages/db/src/selectors/compute/replace-builder.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/db/drizzle/meta/_journal.json
- packages/db/drizzle/0091_bizarre_jetstream.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/db/src/schema/policy.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/api/src/router/policy.tspackages/db/src/selectors/compute/replace-builder.ts
🧬 Code Graph Analysis (1)
packages/api/src/router/policy.ts (1)
packages/db/src/policies/update.ts (1)
updatePolicyInTx(151-233)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (1)
packages/api/src/router/policy.ts (1)
282-282: Verify error handling for selector recomputationLooking at the implementation of
updatePolicyInTxfrom the related code snippets, the function now performs asynchronous selector recomputation with.catch()blocks that log errors but don't propagate them back to the caller. This means API consumers won't be notified if selector recomputation fails.Consider whether this is the intended behavior or if errors should be propagated to API consumers. The current approach might lead to scenarios where the API reports success even when part of the operation (selector recomputation) has failed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/job-dispatch/src/deployment-update.ts (1)
196-199: Consider handling potential queue operation failures.The code correctly enqueues a job with deployment data and oldSelector for policy target recomputation, but doesn't handle potential errors from the queue operation.
Consider wrapping this in a try/catch block to handle potential queue failures:
+try { getQueue(Channel.UpdateDeployment).add(updatedDeployment.id, { ...updatedDeployment, oldSelector: prevDeployment.resourceSelector, }); +} catch (error) { + // Log the error but continue with the deployment update + console.error("Failed to enqueue deployment update job", error); +}packages/db/src/selectors/compute/policy-builder.ts (2)
8-73: Consider performance optimizations for policy target computation.The PolicyBuilder implementation correctly builds the relationship between policy targets and release targets, but has potential performance concerns with its approach to querying and processing targets.
Consider these improvements:
Batch processing: The current implementation processes each policy target individually, which could lead to N+1 query problems. Consider using a SQL JOIN operation to match targets in a single query.
Error handling: Add logging to help diagnose issues during the asynchronous computation.
import { and, eq, inArray } from "drizzle-orm/pg-core/expressions"; import type { Tx } from "../../common.js"; import * as SCHEMA from "../../schema/index.js"; +import { logger } from "@ctrlplane/logger"; import { QueryBuilder } from "../query/builder.js"; import { ReplaceBuilder } from "./replace-builder.js"; +const log = logger.child({ + module: "policy-builder", +}); export class PolicyBuilder { constructor( private readonly tx: Tx, private readonly ids: string[], ) {} releaseTargetSelectors() { return new ReplaceBuilder( this.tx, SCHEMA.computedPolicyTargetReleaseTarget, async (tx) => { + try { const targets = await tx .select() .from(SCHEMA.policyTarget) .where(inArray(SCHEMA.policyTarget.policyId, this.ids)); + log.debug(`Deleting computed targets for ${targets.length} policy targets`); await tx.delete(SCHEMA.computedPolicyTargetReleaseTarget).where( inArray( SCHEMA.computedPolicyTargetReleaseTarget.policyTargetId, targets.map((t) => t.id), ), ); + } catch (error) { + log.error("Failed to delete previous policy target mappings", { error }); + throw error; + } }, async (tx) => { + try { 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) => { 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, })); }); const fulfilled = await Promise.all(targetPromises); + const flatResult = fulfilled.flat(); + log.debug(`Generated ${flatResult.length} policy target mappings for ${targets.length} policy targets`); + return flatResult; + } catch (error) { + log.error("Failed to compute new policy target mappings", { error }); + throw error; + } - return fulfilled.flat(); }, ); } }
75-149: WorkspacePolicyBuilder follows similar pattern - consider consistent improvements.The WorkspacePolicyBuilder implementation shares the same performance considerations as PolicyBuilder, but for workspace-scoped policies.
Apply similar optimizations as suggested for PolicyBuilder:
- Add error handling and logging
- Consider more efficient query patterns for large workspaces
Additionally, there's duplicated logic between the two builders. Consider extracting common functionality into a shared method or base class.
export class WorkspacePolicyBuilder { constructor( private readonly tx: Tx, private readonly workspaceId: string, ) {} releaseTargetSelectors() { return new ReplaceBuilder( this.tx, SCHEMA.computedPolicyTargetReleaseTarget, async (tx) => { + try { const targets = await tx .select() .from(SCHEMA.policyTarget) .innerJoin( SCHEMA.policy, eq(SCHEMA.policyTarget.policyId, SCHEMA.policy.id), ) .where(eq(SCHEMA.policy.workspaceId, this.workspaceId)); + log.debug(`Deleting computed targets for ${targets.length} policy targets in workspace ${this.workspaceId}`); await tx.delete(SCHEMA.computedPolicyTargetReleaseTarget).where( inArray( SCHEMA.computedPolicyTargetReleaseTarget.policyTargetId, targets.map((t) => t.policy_target.id), ), ); + } catch (error) { + log.error("Failed to delete previous policy target mappings for workspace", { + workspaceId: this.workspaceId, + error + }); + throw error; + } }, // Similar changes for the values function... ); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
apps/event-worker/src/workers/new-deployment.ts(3 hunks)apps/event-worker/src/workers/update-deployment.ts(2 hunks)apps/event-worker/src/workers/update-environment.ts(1 hunks)apps/event-worker/src/workers/update-resource-variable.ts(2 hunks)apps/pty-proxy/src/controller/agent-socket.ts(1 hunks)apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts(2 hunks)apps/webservice/src/app/api/v1/resources/route.ts(3 hunks)packages/db/drizzle/0091_open_unus.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/policies/create.ts(2 hunks)packages/db/src/policies/update.ts(2 hunks)packages/db/src/schema/policy-relations.ts(2 hunks)packages/db/src/schema/policy.ts(3 hunks)packages/db/src/schema/release.ts(2 hunks)packages/db/src/selectors/compute/policy-builder.ts(1 hunks)packages/job-dispatch/src/deployment-update.ts(2 hunks)packages/job-dispatch/src/resource/handle-provider-scan.ts(2 hunks)packages/rule-engine/src/db/get-applicable-policies.ts(2 hunks)packages/rule-engine/src/manager/version-manager.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/db/src/schema/release.ts
- packages/db/drizzle/0091_open_unus.sql
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/db/drizzle/meta/_journal.json
- apps/webservice/src/app/api/v1/resources/route.ts
- apps/event-worker/src/workers/update-environment.ts
- packages/db/src/policies/create.ts
- apps/pty-proxy/src/controller/agent-socket.ts
- packages/db/src/policies/update.ts
- apps/event-worker/src/workers/update-resource-variable.ts
- apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts
- packages/db/src/schema/policy-relations.ts
- apps/event-worker/src/workers/update-deployment.ts
- apps/event-worker/src/workers/new-deployment.ts
- packages/db/src/schema/policy.ts
- packages/job-dispatch/src/resource/handle-provider-scan.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/deployment-update.tspackages/rule-engine/src/manager/version-manager.tspackages/db/src/selectors/compute/policy-builder.tspackages/rule-engine/src/db/get-applicable-policies.ts
🧬 Code Graph Analysis (3)
packages/job-dispatch/src/deployment-update.ts (1)
packages/events/src/index.ts (1)
getQueue(28-34)
packages/db/src/selectors/compute/policy-builder.ts (3)
packages/db/src/common.ts (1)
Tx(22-22)packages/db/src/selectors/compute/replace-builder.ts (1)
ReplaceBuilder(6-26)packages/db/src/selectors/query/builder.ts (1)
QueryBuilder(19-45)
packages/rule-engine/src/db/get-applicable-policies.ts (2)
packages/db/src/common.ts (1)
Tx(22-22)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 (4)
packages/job-dispatch/src/deployment-update.ts (1)
9-9: New import for queuing deployment update events.This new import introduces the necessary components for adding deployment update events to the queue.
packages/rule-engine/src/manager/version-manager.ts (1)
166-169: Updated parameter usage with simplified API.The code now correctly passes only the releaseTarget ID to the getApplicablePolicies function, aligning with the simplified API that now uses precomputed policy-target relationships.
packages/rule-engine/src/db/get-applicable-policies.ts (2)
23-56: Well-implemented precomputed policy lookup.The updated implementation efficiently retrieves policies using the precomputed relationships. This approach is more performant than the previous implementation that required complex filtering and matching logic.
58-88: Consider why this alternative implementation is commented out.There's a commented-out alternative implementation that uses direct joins instead of eager loading. This approach might be more efficient in some cases, particularly for large datasets.
Could you clarify why the direct join approach was not chosen? Consider documenting this decision with a comment explaining the tradeoffs, or removing the commented code if it's not needed.
/* + * Alternative implementation using direct joins + * This may be more efficient for large datasets but doesn't eagerly load + * related policy data (denyWindows, deploymentVersionSelector, etc.) export const getApplicablePolicies = withSpan( "getApplicablePolicies", async (span, tx: Tx, releaseTargetId: string) => { span.setAttribute("release.target.id", releaseTargetId); const rows = await tx .select() .from(schema.computedPolicyTargetReleaseTarget) .innerJoin( schema.policyTarget, eq( schema.computedPolicyTargetReleaseTarget.policyTargetId, schema.policyTarget.id, ), ) .innerJoin( schema.policy, eq(schema.policyTarget.policyId, schema.policy.id), ) .where( eq( schema.computedPolicyTargetReleaseTarget.releaseTargetId, releaseTargetId, ), ); return rows.map((r) => r.policy); }, ); */
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/event-worker/src/workers/new-deployment-version.ts (1)
44-46: Consider partial failures
If oneupsertReleaseTargetscall fails, others still proceed. Consider grouping them in a transaction or handling partial failures more explicitly, depending on your tolerance for partial success.apps/event-worker/src/workers/update-deployment.ts (2)
15-27: Check handling for rejected promises
Promise.allSettledprevents throwing but also silently discards errors unless you handle them. If you want to log these errors or retry, consider more explicit handling.
89-97: Further handling of partial upserts
Similar to other occurrences, consider the failure mode forPromise.all(). If a single resource fails to upsert, do you want to stop or continue?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/event-worker/src/workers/new-deployment-version.ts(2 hunks)apps/event-worker/src/workers/new-deployment.ts(1 hunks)apps/event-worker/src/workers/new-resource.ts(2 hunks)apps/event-worker/src/workers/update-deployment.ts(1 hunks)apps/event-worker/src/workers/update-environment.ts(2 hunks)apps/event-worker/src/workers/update-resource-variable.ts(1 hunks)packages/api/src/router/deployment-version-checks.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(1 hunks)packages/rule-engine/src/db/get-applicable-policies.ts(2 hunks)packages/rule-engine/src/manager/version-manager.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/rule-engine/src/manager/version-manager.ts
- packages/db/src/selectors/compute/policy-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/environment-builder.tsapps/event-worker/src/workers/new-resource.tsapps/event-worker/src/workers/new-deployment-version.tsapps/event-worker/src/workers/update-deployment.tspackages/api/src/router/deployment-version-checks.tsapps/event-worker/src/workers/new-deployment.tspackages/db/src/selectors/compute/deployment-builder.tsapps/event-worker/src/workers/update-environment.tspackages/rule-engine/src/db/get-applicable-policies.tsapps/event-worker/src/workers/update-resource-variable.ts
🧬 Code Graph Analysis (5)
apps/event-worker/src/workers/update-deployment.ts (9)
packages/logger/src/index.ts (1)
logger(44-44)packages/db/src/client.ts (1)
db(15-15)packages/db/src/common.ts (2)
Tx(22-22)takeFirst(9-13)packages/db/src/schema/deployment.ts (2)
deployment(68-92)Deployment(104-104)packages/db/src/schema/resource.ts (1)
resource(59-87)packages/job-dispatch/src/events/index.ts (1)
handleEvent(8-9)packages/db/src/selectors/index.ts (1)
selector(18-18)packages/events/src/index.ts (2)
createWorker(10-25)getQueue(28-34)apps/event-worker/src/utils/upsert-release-targets.ts (1)
upsertReleaseTargets(12-64)
apps/event-worker/src/workers/new-deployment.ts (6)
packages/logger/src/index.ts (1)
logger(44-44)packages/events/src/index.ts (2)
createWorker(10-25)getQueue(28-34)packages/db/src/selectors/index.ts (1)
selector(18-18)packages/db/src/client.ts (1)
db(15-15)packages/db/src/common.ts (1)
takeFirst(9-13)apps/event-worker/src/utils/upsert-release-targets.ts (1)
upsertReleaseTargets(12-64)
apps/event-worker/src/workers/update-environment.ts (8)
packages/db/src/common.ts (2)
Tx(22-22)takeFirst(9-13)packages/db/src/schema/resource.ts (1)
resource(59-87)packages/db/src/schema/deployment.ts (1)
deployment(68-92)packages/job-dispatch/src/events/index.ts (1)
handleEvent(8-9)packages/db/src/selectors/index.ts (1)
selector(18-18)packages/db/src/schema/environment.ts (1)
environment(59-84)apps/event-worker/src/utils/upsert-release-targets.ts (1)
upsertReleaseTargets(12-64)packages/events/src/index.ts (1)
getQueue(28-34)
packages/rule-engine/src/db/get-applicable-policies.ts (1)
packages/db/src/common.ts (1)
Tx(22-22)
apps/event-worker/src/workers/update-resource-variable.ts (7)
packages/logger/src/index.ts (1)
logger(44-44)packages/events/src/index.ts (2)
createWorker(10-25)getQueue(28-34)packages/db/src/schema/resource.ts (1)
resource(59-87)packages/db/src/client.ts (1)
db(15-15)packages/db/src/common.ts (1)
takeFirst(9-13)packages/db/src/selectors/index.ts (1)
selector(18-18)apps/event-worker/src/utils/upsert-release-targets.ts (1)
upsertReleaseTargets(12-64)
🔇 Additional comments (38)
packages/rule-engine/src/db/get-applicable-policies.ts (3)
25-26: Function signature simplified to use precomputed relationship tableThe function parameter has been appropriately simplified to only require the release target ID, which aligns with the PR objective of using precomputed policy target mappings.
28-48: Efficient implementation using precomputed join tableThis implementation now directly leverages the precomputed
computedPolicyTargetReleaseTargettable rather than performing on-the-fly selector matching. The eager loading of related policy data (deny windows, approvals) is comprehensive and will ensure all necessary data is available for policy evaluation.
50-50: Clean data transformationThe mapping function efficiently extracts only the policy objects from the complex nested query result, which provides a clean API for consumers of this function.
packages/db/src/selectors/compute/deployment-builder.ts (2)
37-37: Good defensive programming with null checkAdding this null check prevents potential errors when attempting to query with a null resource selector, avoiding unnecessary database operations when no selector is available.
106-106: Consistent null check implementationThis null check mirrors the one added to the
DeploymentBuilderclass, maintaining consistency in how null selectors are handled across similar implementations.packages/db/src/selectors/compute/environment-builder.ts (2)
37-37: Effective null check for resource selectorThis null check prevents potential runtime errors and unnecessary database operations when the environment's resource selector is null or undefined.
100-100: Consistent null handling for workspace environment builderThis implementation maintains consistency with both the
EnvironmentBuilderclass and the corresponding deployment builder implementations.apps/event-worker/src/workers/new-resource.ts (1)
11-21: Well-structured selector recomputation implementationThis implementation properly handles selector recomputation before proceeding with release target upserts:
- Concurrently recomputes resource selectors for environments and deployments using
Promise.all- Sequentially recomputes policy release target selectors
This approach aligns with the PR objective of implementing precomputed policy target mappings and follows a consistent pattern seen in other worker implementations.
apps/event-worker/src/workers/new-deployment-version.ts (3)
1-1: Trivial import for eq
No issues found.
6-6: Import upsertReleaseTargets
No issues found.
29-42: Verify behavior when no resources are found
If no rows match incomputedDeploymentResource, theresourcesarray will be empty and no release targets will be created. Confirm whether this scenario is acceptable or if it should be treated as an error.apps/event-worker/src/workers/update-deployment.ts (10)
1-1: Add type import for Tx
No issues found.
4-9: Import statements
No issues found, usage is standard.
11-11: Import upsertReleaseTargets
No issues found.
13-13: Logger child
No issues found.
29-64: Potential concurrency or transactional approach
This function queries existing resources, runs selector replacement, then queries again to compute the diff. If multiple processes are recomputing simultaneously, you might need to ensure consistent data with transactions or queued updates.
69-74: Early return if unchanged
Skipping full recomputation for unchanged selectors is an efficient approach.
76-82: TakeFirst usage
This throws if exactly one record is not found. If that is desired behavior, it's fine; otherwise consider a friendlier error.
83-88: Recompute release target selectors
This follows the pattern used in other workers. No issues found.
98-98: Calling dispatchExitHooks
Implementation is consistent with the event-driven approach. No issues found.
100-102: Error handling
Logging and rethrowing the error is straightforward and ensures visibility.packages/api/src/router/deployment-version-checks.ts (8)
1-1: Import Tx
No issues found.
5-5: Import DB operators
No issues found; these operators are used consistently in subsequent queries.
10-10: Import mergePolicies
No issues found.
17-52: New helper function for resource-scope filtering
The logic of checkingresourceSelectorfornulland returning policies fromcomputedPolicyTargetReleaseTargetlooks correct.
106-109: Querying resource-scope-exempt policies
No issues found. Retrieves relevant policies for the release target.
110-110: Merging policies
Straightforward usage ofmergePolicies, no issues noted.
112-113: Passing mergedPolicy to VersionReleaseManager
Implementation aligns with the system logic, providing an optional policy to the evaluation method.
119-120: Returning evaluation results
Returning the result object withapprovedandrejectionReasonsis clear and consistent with the system’s expectations.apps/event-worker/src/workers/new-deployment.ts (3)
1-7: Good centralization of imports and logger setupThe addition of the selector API, takeFirst utility, and centralized logger setup provides better structure to the worker. The upsertReleaseTargets utility import aligns with the architectural goal of centralizing release target creation logic.
9-9: Well-scoped logger instanceCreating a child logger with the module name provides better context in logs and follows good logging practices.
14-55: Robust implementation with improved architectureThe refactored worker implementation adopts a more maintainable approach:
- Uses the selector API to compute deployment resource selectors and policy release target selectors
- Properly retrieves system and workspace information
- Queries computed deployment resources directly
- Leverages the upsertReleaseTargets utility for consistent release target creation
- Includes comprehensive error handling with proper logging
This approach aligns with the broader architectural changes to use precomputed mappings between resources, deployments, and policies.
apps/event-worker/src/workers/update-resource-variable.ts (2)
1-9: Good import structure and logger configurationThe import structure follows the same pattern as other workers, with the necessary utilities imported from their respective packages and a properly configured logger instance.
14-42: Well-structured implementation with efficient parallel processingThe worker implementation follows a consistent pattern with other refactored workers:
- Properly retrieves the resource and workspace information
- Efficiently computes resource selectors for all environments and deployments in parallel
- Computes policy release target selectors
- Uses the standardized upsertReleaseTargets utility
- Includes proper error handling with contextual logging
The use of Promise.all for parallel computation of environment and deployment resource selectors is particularly efficient.
apps/event-worker/src/workers/update-environment.ts (4)
4-11: Good import structure and utility inclusionThe import structure aligns with other worker refactorings, bringing in the selector API and other necessary utilities. The inclusion of upsertReleaseTargets maintains consistency with the architectural approach.
18-37: Well-designed resource exit handlingThe refactored
dispatchExitHooksfunction efficiently handles resources that no longer match the environment by:
- Retrieving all relevant deployments in a single query
- Creating appropriate events for each deployment-resource pair
- Using Promise.allSettled to handle all event processing in parallel without failing if individual events fail
This provides robust cleanup for resources that exit an environment's scope.
39-75: Excellent resource difference computation functionThe
recomputeResourcesAndReturnDifffunction elegantly handles the complex task of determining which resources have entered or exited an environment's scope:
- Queries current matching resources before recomputation
- Recomputes the environment's resource selectors
- Queries the newly matching resources
- Computes the difference to find exited resources
This approach is cleaner and more maintainable than the previous implementation that required complex multi-part queries with old and new selectors.
96-129: Optimized and well-structured worker implementationThe main worker implementation includes several important improvements:
- Early return when selectors haven't changed, preventing unnecessary computation
- Efficient resource difference computation using the helper function
- Proper workspace retrieval and policy target recomputation
- Consistent use of upsertReleaseTargets for release target creation
- Comprehensive error handling with contextual logging
This implementation follows the same architectural pattern as other refactored workers while handling the unique complexities of environment updates.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts (3)
57-73: There are duplicate optional() calls in schema definition.The schema definition contains redundant optional() calls for some fields, which doesn't affect functionality but reduces code clarity.
const patchSchema = z.object({ - name: z.string().optional().optional(), - version: z.string().optional().optional(), - kind: z.string().optional().optional(), - identifier: z.string().optional().optional(), - workspaceId: z.string().optional().optional(), + name: z.string().optional(), + version: z.string().optional(), + kind: z.string().optional(), + identifier: z.string().optional(), + workspaceId: z.string().optional(), metadata: z.record(z.string()).optional(), variables: z .array(
107-116: Improved selector computation with async handling.The implementation correctly computes and replaces selectors for environments, deployments, and policies, which aligns with the PR objective of maintaining the policy target join table.
However, consider further optimization by parallelizing all three selector computations:
const cb = selector().compute(); await Promise.all([ cb.allEnvironments(workspaceId).resourceSelectors().replace(), cb.allDeployments(workspaceId).resourceSelectors().replace(), + cb.allPolicies(resource.workspaceId).releaseTargetSelectors().replace(), ]); -await cb - .allPolicies(resource.workspaceId) - .releaseTargetSelectors() - .replace(); await getQueue(Channel.UpdatedResource).add(res.id, res);
118-125: Enhanced error handling with proper logging.The error handling implementation is appropriate and includes logging, which is a significant improvement.
Consider enhancing the error logging to include more context for debugging purposes:
} catch (err) { const error = err instanceof Error ? err : new Error(String(err)); - log.error(`Error updating resource: ${error}`); + log.error({ err, resourceId: params.resourceId }, `Error updating resource: ${error.message}`); return NextResponse.json( { error: "Failed to update resource" }, { status: 500 }, ); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/event-worker/src/workers/new-deployment-version.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 (3)
- packages/job-dispatch/src/resource/handle-provider-scan.ts
- apps/event-worker/src/workers/new-deployment-version.ts
- apps/webservice/src/app/api/v1/resources/route.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/webservice/src/app/api/v1/resources/[resourceId]/route.ts
🔇 Additional comments (3)
apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts (3)
5-9: Good addition of selector and logging imports.The code now properly imports
selectorfor selector computation andloggerfor structured logging, aligning with the overall PR objective of implementing policy target computation.
17-18: Well-structured module-scoped logger implementation.Creating a child logger with the module name improves error traceability and follows logging best practices.
89-106: Well-implemented error handling and resource validation.The code now properly checks if the resource exists and is not deleted before proceeding with updates. The error handling with try/catch is a major improvement, and the explicit check after upsert enhances reliability.
| .replace(), | ||
| ]); | ||
| await getQueue(Channel.UpdatedResource).add(res.id, res); | ||
| selector() |
There was a problem hiding this comment.
added these back
| const manager = new VersionReleaseManager(ctx.db, releaseTarget); | ||
| const { chosenCandidate, rejectionReasons } = await manager.evaluate({ | ||
| const result = await manager.evaluate({ | ||
| policy: mergedPolicy ?? undefined, |
|
|
||
| if (updatedPolicy == null) throw new Error("Policy not found"); | ||
|
|
||
| selector() |
There was a problem hiding this comment.
for policy create and update, we should probably not await. this is because the UI would pause while the all these computations are happening. instead, just let the UI proceed. if there are errors we can toast and log or something
| "rest": "pnpm with-env psql $POSTGRES_URL -c \"DROP SCHEMA IF EXISTS public CASCADE; CREATE SCHEMA public;\"" | ||
| }, | ||
| "dependencies": { | ||
| "@ctrlplane/logger": "workspace:*", |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/event-worker/src/workers/update-resource-variable.ts (1)
39-42: Good error handling implementation.The try-catch block with proper logging addresses the previously raised concerns about error handling in the Promise chain.
🧹 Nitpick comments (5)
apps/event-worker/src/workers/updated-resources/index.ts (2)
28-31: Consider adding error logging for the policy selector operation.While the concurrent operations above use Promise.all (which will throw on first error), this sequential operation doesn't have explicit error handling. If it fails, the error might be difficult to trace in logs.
await upsertReleaseTargets(db, resource); - await cb - .allPolicies(resource.workspaceId) - .releaseTargetSelectors() - .replace(); + try { + await cb + .allPolicies(resource.workspaceId) + .releaseTargetSelectors() + .replace(); + } catch (error) { + logger.error( + `Failed to replace policy release target selectors for workspace ${resource.workspaceId}`, + { error, resourceId: resource.id } + ); + throw error; + }
63-66: Consider adding more detailed error reporting in Promise.allSettled.Promise.allSettled suppresses errors which is good for ensuring all operations complete, but you should log any rejected promises to maintain observability.
await Promise.allSettled([ dispatchExitHooksPromise, addToEvaluateQueuePromise, - ]); + ]).then(results => { + const errors = results.filter(r => r.status === 'rejected'); + if (errors.length > 0) { + logger.error(`${errors.length} operations failed after release target updates for resource ${resource.id}`, { + errors: errors.map((r, i) => ({ + operation: i === 0 ? 'dispatchExitHooks' : 'addToEvaluateQueue', + error: (r as PromiseRejectedResult).reason + })) + }); + } + });packages/job-dispatch/src/resource/replace-release-targets.ts (3)
56-60: Consider adding error handling for database operations.While the function generally handles operations well, there's no specific error handling for database operations. If the insert fails, the error will propagate up but without contextual information.
- const newReleaseTargets = await db - .insert(SCHEMA.releaseTarget) - .values(targets) - .onConflictDoNothing() - .returning(); + let newReleaseTargets; + try { + newReleaseTargets = await db + .insert(SCHEMA.releaseTarget) + .values(targets) + .onConflictDoNothing() + .returning(); + } catch (error) { + console.error(`Failed to insert release targets for resource ${resource.id}`, { + error, + resourceId: resource.id, + targetCount: targets.length + }); + throw error; + }
76-77: Enhance error handling for event processing.Using Promise.allSettled is good to ensure all events are processed, but you should check and log any failures to maintain observability.
- const handleEventPromises = events.map(handleEvent); - await Promise.allSettled(handleEventPromises); + const handleEventPromises = events.map(handleEvent); + const results = await Promise.allSettled(handleEventPromises); + + const failures = results.filter(r => r.status === 'rejected'); + if (failures.length > 0) { + console.warn(`${failures.length}/${events.length} exit hooks failed for resource ${resource.id}`, { + resourceId: resource.id, + errors: failures.map((r, i) => ({ + index: i, + deploymentId: exitedDeployments[i]?.id, + error: (r as PromiseRejectedResult).reason + })) + }); + }
1-80: Consider adding JSDoc documentation for the exported function.This utility function performs complex operations and would benefit from clear documentation explaining its purpose, parameters, and return value.
+/** + * Replaces release targets for a resource and dispatches exit hooks for removed deployments. + * + * This function: + * 1. Fetches current release targets for the resource + * 2. Identifies valid deployment targets based on resource selectors + * 3. Creates new release targets for valid targets + * 4. Determines which deployments have been exited + * 5. Dispatches DeploymentResourceRemoved events for exited deployments + * + * @param db - Database transaction or client + * @param resource - The resource for which to replace release targets + * @returns Array of newly created release targets + */ export const replaceReleaseTargetsAndDispatchExitHooks = async ( db: Tx, resource: SCHEMA.Resource, ) => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/event-worker/src/workers/new-deployment.ts(1 hunks)apps/event-worker/src/workers/new-resource.ts(2 hunks)apps/event-worker/src/workers/update-deployment.ts(1 hunks)apps/event-worker/src/workers/update-environment.ts(2 hunks)apps/event-worker/src/workers/update-resource-variable.ts(1 hunks)apps/event-worker/src/workers/updated-resources/index.ts(2 hunks)packages/job-dispatch/src/resource/handle-provider-scan.ts(2 hunks)packages/job-dispatch/src/resource/replace-release-targets.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/job-dispatch/src/resource/handle-provider-scan.ts
- apps/event-worker/src/workers/new-resource.ts
- apps/event-worker/src/workers/new-deployment.ts
- apps/event-worker/src/workers/update-environment.ts
- 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/updated-resources/index.tspackages/job-dispatch/src/resource/replace-release-targets.tsapps/event-worker/src/workers/update-resource-variable.ts
🧬 Code Graph Analysis (1)
packages/job-dispatch/src/resource/replace-release-targets.ts (3)
packages/db/src/common.ts (1)
Tx(22-22)packages/db/src/schema/resource.ts (1)
resource(59-87)packages/job-dispatch/src/events/index.ts (1)
handleEvent(8-9)
🔇 Additional comments (6)
apps/event-worker/src/workers/updated-resources/index.ts (1)
22-26: Good implementation of concurrent selector computation.The code correctly initializes a selector computation instance and concurrently replaces resource selectors for both environments and deployments in the workspace. This efficient approach reduces the overall execution time.
apps/event-worker/src/workers/update-resource-variable.ts (3)
14-23: Good simplification of resource querying.The code has been improved by directly querying the resource by ID instead of using complex multi-table joins. The use of
takeFirstproperly handles fetching a single result.
25-30: Proper concurrent execution of selector computation.The code initializes the selector computation builder and properly uses Promise.all to concurrently replace resource selectors for environments and deployments, which is more efficient.
32-38: Good implementation of release target updating and job dispatching.The code now properly upserts release targets and replaces release target selectors for policies before creating and enqueueing jobs for evaluation.
packages/job-dispatch/src/resource/replace-release-targets.ts (2)
17-41: Well-structured complex join query to identify valid deployment targets.The multi-table join elegantly identifies valid deployment targets based on resource selectors by joining computed environment resources with environments, deployments, and computed deployment resources.
43-55: Good filtering logic for target construction.The code properly filters rows where either the deployment has no resource selector or the computed deployment resource exists, ensuring that only valid targets are included.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/job-dispatch/src/job-dispatch.ts (1)
96-101: Consider removing commented-out code instead of keeping it.The commented-out job dispatch queue code appears to be intentionally disabled as part of a broader architectural change where job dispatching responsibilities are now handled by event workers and utility modules. While commenting out code can be helpful during transitions, it's generally better to remove unused code completely once the refactoring is complete to avoid confusion and technical debt.
If this is temporary or you're keeping it for reference, consider adding an explanatory comment about why it's commented out and when it might be removed.
- // await getQueue(Channel.DispatchJob).addBulk( - // validJobsWithResolvedVariables.map((wf) => ({ - // name: wf.id, - // data: { jobId: wf.id }, - // })), - // );apps/jobs/src/policy-checker/index.ts (1)
58-58: Add a clarifying comment about why this functionality is disabled.The code comments out the call to
triggerPolicyEvaluation()but doesn't explain the rationale behind this change. While this appears to be part of the broader refactoring to use precomputed policy target mappings (as mentioned in the PR description), adding an explanatory comment would improve code maintainability.- // await triggerPolicyEvaluation(); + // Policy evaluation is now handled by event workers using the precomputed + // policy target to release target mappings in the computedPolicyTargetReleaseTarget table
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
apps/event-worker/src/utils/upsert-release-targets.ts(1 hunks)apps/jobs/src/policy-checker/index.ts(1 hunks)apps/pty-proxy/src/controller/agent-socket.ts(1 hunks)packages/db/drizzle/0092_many_fenris.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/policies/create.ts(2 hunks)packages/db/src/schema/policy.ts(3 hunks)packages/job-dispatch/src/job-dispatch.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/db/drizzle/meta/_journal.json
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/pty-proxy/src/controller/agent-socket.ts
- packages/db/src/policies/create.ts
- packages/db/src/schema/policy.ts
- apps/event-worker/src/utils/upsert-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/job-dispatch/src/job-dispatch.tsapps/jobs/src/policy-checker/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (1)
packages/db/drizzle/0092_many_fenris.sql (1)
1-8: Schema design is correct and implementation is solid.The join table is well-structured for its purpose, with appropriate primary and foreign key constraints and cascading deletes to maintain referential integrity. No issues found.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores