feat: computed policy env and deployment targets #529
Conversation
WalkthroughThis set of changes introduces new event worker modules and supporting infrastructure for computing policy target selectors in a concurrent and modular fashion. Three new workers are added to handle environment, deployment, and all-selector computations, along with corresponding event channels and queue logic. The database schema is extended with tables for computed policy target environments and deployments. The policy applicability logic is refactored into a class-based API, enabling more flexible and granular queries. Various parts of the API and worker logic are updated to use the new event channels and policy query interfaces, improving modularity and maintainability. Changes
Sequence Diagram(s)sequenceDiagram
participant Producer as Job Producer (e.g., new policy)
participant WorkerAll as computePolicyTargetAllSelectorsWorker
participant WorkerEnv as computePolicyTargetEnvironmentSelectorWorker
participant WorkerDep as computePolicyTargetDeploymentSelectorWorker
participant WorkerRT as computePolicyTargetReleaseTargetSelectorWorker
Producer->>WorkerAll: Enqueue job (id)
WorkerAll->>WorkerEnv: Enqueue job (id)
WorkerAll->>WorkerDep: Enqueue job (id)
WorkerAll->>WorkerRT: Enqueue job (id)
Note over WorkerAll: Waits for all three jobs to be enqueued
Possibly related PRs
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: 6
🔭 Outside diff range comments (1)
apps/event-worker/src/workers/compute-policy-taret-all-selectors.ts (1)
1-21:⚠️ Potential issueTypo in filename needs correction
The filename contains a typo:
compute-policy-taret-all-selectors.tsshould becompute-policy-target-all-selectors.tsto maintain consistency with the rest of the codebase.The filename needs to be corrected throughout the project, including import statements that reference it.
Effective fan-out pattern for selector computation
This worker efficiently uses Promise.all to concurrently dispatch jobs to specific selector workers. The implementation is clean and focuses on its dispatching responsibility, which follows good separation of concerns.
🧹 Nitpick comments (11)
packages/events/src/types.ts (1)
37-40: Enum order / discoverabilityThe three new channels work fine semantically, but they break the informal alphabetical / semantic grouping that the previous block followed (all Compute…Selector channels were grouped together and sorted). Re-ordering keeps cognitive load low when scanning the file.
ComputeEnvironmentResourceSelector = "compute-environment-resource-selector", ComputeDeploymentResourceSelector = "compute-deployment-resource-selector", ComputePolicyTargetReleaseTargetSelector = "compute-policy-target-release-target-selector", - ComputePolicyTargetEnvironmentSelector = "compute-policy-target-environment-selector", - ComputePolicyTargetDeploymentSelector = "compute-policy-target-deployment-selector", - ComputePolicyTargetAllSelectors = "compute-policy-target-all-selectors", + ComputePolicyTargetAllSelectors = "compute-policy-target-all-selectors", + ComputePolicyTargetDeploymentSelector = "compute-policy-target-deployment-selector", + ComputePolicyTargetEnvironmentSelector = "compute-policy-target-environment-selector",(Only a move – no functional change.)
packages/db/src/schema/policy.ts (2)
97-110: Missing secondary indexes may hurt look-ups
computed_policy_target_environmentis queried both by
policyTargetId(already covered by the PK) andenvironmentId(reverse look-ups: “which policy targets apply to this env?”).Adding a simple index on
environment_idwill keep those queries from doing seq-scans when the table grows.export const computedPolicyTargetEnvironment = pgTable( "computed_policy_target_environment", { … }, (t) => ({ pk: primaryKey({ columns: [t.policyTargetId, t.environmentId] }), + byEnvironment: index("cpt_env_by_environment").on(t.environmentId), }), );Same applies to the deployment table below.
112-125: Consider exporting the generated typesDown-stream code (rule-engine, API) will probably want to refer to
InferSelectModel<typeof computedPolicyTargetDeployment>/Environment>.
Exporting those right here reduces repetition and keeps types single-sourced.export type ComputedPolicyTargetEnvironment = InferSelectModel< typeof computedPolicyTargetEnvironment >; export type ComputedPolicyTargetDeployment = InferSelectModel< typeof computedPolicyTargetDeployment >;packages/api/src/router/deployment-version-checks/version-selector.ts (1)
6-11: Unused import after refactor
and,eq,selectorare still imported but onlyand/eqare used after
the refactor; consider droppingselectorto silence eslint/tsc warnings.apps/event-worker/src/workers/compute-policy-target-environment-selector.ts (1)
8-32: Query only needed columns & use implicit select typeThe helper currently selects all environment + system columns and then
projects onlyenvironment.id. This moves unnecessary bytes over the wire.- const environments = await tx - .select() + const environments = await tx + .select({ id: schema.environment.id })Performance boost is marginal now but scales better.
packages/api/src/router/deployment-version-checks/approvals.ts (1)
148-153: Add retry/back-off when re-queuing evaluation jobs
addBulkfires jobs immediately. If the downstream worker is still processing locks for the same release target, we may create a tight retry loop.
Consider supplyingopts: { delay: 5_000, attempts: 5, backoff: { type: "exponential", delay: 2_000 } }(BullMQ signature) or similar.apps/event-worker/src/workers/compute-policy-target-deployment-selector.ts (2)
8-32: Avoid fetching full deployment rows – select only the IDsOnly
deployment.idis needed to buildmatchingDeployments. Pulling entire rows increases network and memory use.- const deployments = await tx - .select() + const deployments = await tx + .select({ id: schema.deployment.id }) .from(schema.deployment) ... - return deployments.map((d) => ({ + return deployments.map((d) => ({ policyTargetId: policyTarget.id, - deploymentId: d.deployment.id, + deploymentId: d.id, }));
80-88: Re-queue loop can be infinite – add delay or maxAttemptsIf the row stays locked for an extended period (e.g., long-running migration) the worker will enqueue itself continuously. Provide exponential back-off or pass
opts: { attempts: 5, backoff: { type: "exponential", delay: 1000 } }.await getQueue(Channel.ComputePolicyTargetDeploymentSelector).add( job.name, job.data, + { backoff: { type: "exponential", delay: 1_000 }, attempts: 5 }, );packages/rule-engine/src/db/get-applicable-policies.ts (3)
120-133: Stale JSDoc – parameters no longer match implementationThe docstring references
workspaceIdandrepo, butreleaseTarget()now only acceptsreleaseTargetId. This can confuse consumers and automated documentation.Please update the comment block accordingly.
105-109: Deduplication in JS could be pushed to SQL
_.uniqByruns in memory after fetching potentially large result sets.
Usingselect distinct on (policy.id)(Postgres) or grouping in SQL would cut down transferred rows and CPU.Not blocking, but worth considering for large workspaces.
111-114: Fluent API leak – confusing method nameInside
EnvironmentAndDeploymentApplicablePolicies,environmentAndDeployment()simply returns an object wrappingwithoutResourceScope().
Call-sites already are in the environment+deployment context, so this extra hop reads oddly:getApplicablePolicies().environmentAndDeployment(target).withoutResourceScope();Consider exposing
withoutResourceScope()directly from the class and lettingApplicablePolicies.environmentAndDeployment()return the instance itself:// in ApplicablePolicies - environmentAndDeployment(target) { - return new EnvironmentAndDeploymentApplicablePolicies(this.tx, target); - } + environmentAndDeployment(target) { + return new EnvironmentAndDeploymentApplicablePolicies(this.tx, target); + } // in EnvironmentAndDeploymentApplicablePolicies - environmentAndDeployment() { - return { withoutResourceScope: () => this.withoutResourceScope() }; - } + // consumers now call .withoutResourceScope() directly
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
apps/event-worker/src/workers/compute-policy-taret-all-selectors.ts(1 hunks)apps/event-worker/src/workers/compute-policy-target-deployment-selector.ts(1 hunks)apps/event-worker/src/workers/compute-policy-target-environment-selector.ts(1 hunks)apps/event-worker/src/workers/compute-systems-release-targets.ts(1 hunks)apps/event-worker/src/workers/index.ts(2 hunks)apps/event-worker/src/workers/new-policy.ts(1 hunks)apps/event-worker/src/workers/update-policy.ts(1 hunks)packages/api/src/router/deployment-version-checks/approvals.ts(4 hunks)packages/api/src/router/deployment-version-checks/utils.ts(1 hunks)packages/api/src/router/deployment-version-checks/version-selector.ts(2 hunks)packages/db/src/schema/policy.ts(2 hunks)packages/db/src/schema/rules/approval-base.ts(1 hunks)packages/events/src/types.ts(2 hunks)packages/rule-engine/src/db/get-applicable-policies.ts(1 hunks)packages/rule-engine/src/manager/version-manager.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.
apps/event-worker/src/workers/update-policy.tsapps/event-worker/src/workers/compute-systems-release-targets.tsapps/event-worker/src/workers/new-policy.tspackages/api/src/router/deployment-version-checks/utils.tspackages/events/src/types.tsapps/event-worker/src/workers/compute-policy-taret-all-selectors.tsapps/event-worker/src/workers/index.tspackages/db/src/schema/rules/approval-base.tspackages/rule-engine/src/manager/version-manager.tspackages/db/src/schema/policy.tsapps/event-worker/src/workers/compute-policy-target-environment-selector.tspackages/api/src/router/deployment-version-checks/approvals.tspackages/api/src/router/deployment-version-checks/version-selector.tsapps/event-worker/src/workers/compute-policy-target-deployment-selector.tspackages/rule-engine/src/db/get-applicable-policies.ts
🧬 Code Graph Analysis (5)
apps/event-worker/src/workers/update-policy.ts (1)
packages/events/src/index.ts (1)
getQueue(28-34)
apps/event-worker/src/workers/compute-systems-release-targets.ts (1)
packages/events/src/index.ts (1)
getQueue(28-34)
apps/event-worker/src/workers/new-policy.ts (1)
packages/events/src/index.ts (1)
getQueue(28-34)
packages/rule-engine/src/manager/version-manager.ts (1)
packages/rule-engine/src/db/get-applicable-policies.ts (1)
getApplicablePolicies(23-52)
packages/rule-engine/src/db/get-applicable-policies.ts (1)
packages/db/src/common.ts (1)
Tx(22-22)
⏰ 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 (9)
packages/db/src/schema/rules/approval-base.ts (1)
58-64: Type definition aligns well with existing schema structureThe newly added
BaseApprovalRecordInserttype properly reflects the core fields frombaseApprovalRecordFieldswhile appropriately excluding auto-generated fields likeid,createdAt, andupdatedAt. The optional fields are correctly marked with?. This type will provide better type safety when inserting approval records.apps/event-worker/src/workers/new-policy.ts (1)
12-12:❓ Verification inconclusive
Channel migration to centralized selector computation
The change from the previous channel to
ComputePolicyTargetAllSelectorsaligns with the broader refactoring of policy target selector computations to a more centralized approach.
🏁 Script executed:
#!/bin/bash # Verify that the corresponding worker exists to handle the ComputePolicyTargetAllSelectors channel rg -A 5 "ComputePolicyTargetAllSelectors" --type tsLength of output: 51
#!/bin/bash # Search across all file types for any references to ComputePolicyTargetAllSelectors rg -n "ComputePolicyTargetAllSelectors"
Confirm presence of ComputePolicyTargetAllSelectors worker
I ran a search for any handler or subscriber for theComputePolicyTargetAllSelectorschannel and found no matching references. Please manually verify that:
- The
Channelenum definesComputePolicyTargetAllSelectors.- A corresponding worker (e.g. in
apps/event-worker/src/workers/…) is implemented and registered to process jobs from this queue.Without a handler, enqueued tasks will never be processed.
apps/event-worker/src/workers/compute-systems-release-targets.ts (1)
178-178: Queue channel update for centralized selector processingThe migration to
Channel.ComputePolicyTargetAllSelectorsaligns with the centralized approach for computing policy targets that's being implemented across the codebase. This change ensures consistency with the updates in other worker files.apps/event-worker/src/workers/update-policy.ts (1)
14-14: Consistent channel update for policy computationsUpdated to use
ComputePolicyTargetAllSelectorsfor job processing, maintaining consistency with the channel changes in other worker files. This ensures all policy target computation jobs are routed through the new centralized worker.packages/api/src/router/deployment-version-checks/utils.ts (1)
4-4: Imports updated to reflect removed functionThis import change is part of removing the
getApplicablePoliciesWithoutResourceScopefunction, which aligns with moving to the new class-based policy applicability API. The updated import now only includes what's needed for the remaining functions.packages/rule-engine/src/manager/version-manager.ts (1)
163-165: Updated to use new class-based policy applicability APIThis change follows the broader refactoring pattern, moving from a direct function call with parameters to a more flexible method chaining style. The logic remains functionally equivalent while providing better reusability and more granular querying options.
apps/event-worker/src/workers/index.ts (1)
59-64: New policy target selector workers properly registeredThe new workers for environment, deployment, and all selectors computations are correctly registered with their corresponding channels in the workers object.
packages/events/src/types.ts (1)
77-80: KeepChannelMapin sync with the enumChannelMap is correctly updated, 👍🏼.
Double-check that each worker enqueues/dequeues with{ id: string }– if any worker needs extra fields later you’ll have to widen the type here.packages/api/src/router/deployment-version-checks/version-selector.ts (1)
31-34:❓ Verification inconclusive
Verify builder chain actually executes the query
getApplicablePolicies().environmentAndDeployment(...).withoutResourceScope()
returns an object in the latest rule-engine refactor; you still need to
call.run()(or.execute()) to resolve the promise with the array of
policies. OtherwisemergePolicieswill receive a builder and type-checking
will fail.Please make sure the final call returns
Promise<Policy[]>.
🏁 Script executed:
#!/bin/bash # Grep the builder to see what the terminal method is called rg -A3 -n "class .*ApplicablePolicies" packages/rule-engine | headLength of output: 68
I’ll dig into the rules-engine code to find how
getApplicablePoliciesand.withoutResourceScope()are implemented. Running:
🏁 Script executed:
#!/bin/bash # Locate the definition of getApplicablePolicies echo "=== getApplicablePolicies ===" rg -n "getApplicablePolicies" -n packages/rule-engine | sed -n '1,20p' # Locate the definition of withoutResourceScope echo -e "\n=== withoutResourceScope ===" rg -n "withoutResourceScope" -n packages/rule-engine | sed -n '1,20p'Length of output: 703
Digging into implementations and usages to see how the builder chain resolves:
🏁 Script executed:
#!/bin/bash # Show the implementation of getApplicablePolicies echo "=== getApplicablePolicies implementation ===" sed -n '1,200p' packages/rule-engine/src/db/get-applicable-policies.ts # Show how it's used in version-manager.ts echo -e "\n=== Usage in version-manager.ts ===" sed -n '140,200p' packages/rule-engine/src/manager/version-manager.ts # Check where environmentAndDeployment and withoutResourceScope are defined echo -e "\n=== Search for builder methods ===" rg -n "environmentAndDeployment\|withoutResourceScope" -n packages/rule-engineLength of output: 3990
I need to inspect the API-side builder to see what
getApplicablePolicies()returns and whereenvironmentAndDeployment/withoutResourceScopeare defined. Gathering:
🏁 Script executed:
#!/bin/bash # 1. Locate version-selector.ts echo "=== File location ===" fd version-selector.ts # 2. Show imports and usage in version-selector.ts echo -e "\n=== version-selector.ts (first 200 lines) ===" sed -n '1,200p' packages/api/src/router/deployment-version-checks/version-selector.ts # 3. Find builder method definitions in API echo -e "\n=== environmentAndDeployment definitions ===" rg -n "environmentAndDeployment" -n packages/api echo -e "\n=== withoutResourceScope definitions ===" rg -n "withoutResourceScope" -n packages/apiLength of output: 742
Verify that the query builder is executed before merging
It looks likeconst policies = await getApplicablePolicies() .environmentAndDeployment({ environmentId, deploymentId }) .withoutResourceScope(); const mergedPolicy = mergePolicies(policies);is still dealing with a builder object, not the resolved array. You need to append the terminal call (e.g.
.run()or.execute()) so thatpoliciesis aPolicy[]andmergePoliciestype-checks correctly.• Check
packages/api/src/router/deployment-version-checks/version-selector.ts(around lines 31–34)
• Ensure the final expression is aPromise<Policy[]>, for example:const policies = await getApplicablePolicies() .environmentAndDeployment({ environmentId, deploymentId }) .withoutResourceScope() .run();
| import { computePolicyTargetAllSelectorsWorker } from "./compute-policy-taret-all-selectors.js"; | ||
| import { computePolicyTargetDeploymentSelectorWorker } from "./compute-policy-target-deployment-selector.js"; | ||
| import { computePolicyTargetEnvironmentSelectorWorker } from "./compute-policy-target-environment-selector.js"; |
There was a problem hiding this comment.
Typo in imported module filename
There's a typo in the module filename: compute-policy-taret-all-selectors.js should be compute-policy-target-all-selectors.js.
-import { computePolicyTargetAllSelectorsWorker } from "./compute-policy-taret-all-selectors.js";
+import { computePolicyTargetAllSelectorsWorker } from "./compute-policy-target-all-selectors.js";📝 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.
| import { computePolicyTargetAllSelectorsWorker } from "./compute-policy-taret-all-selectors.js"; | |
| import { computePolicyTargetDeploymentSelectorWorker } from "./compute-policy-target-deployment-selector.js"; | |
| import { computePolicyTargetEnvironmentSelectorWorker } from "./compute-policy-target-environment-selector.js"; | |
| import { computePolicyTargetAllSelectorsWorker } from "./compute-policy-target-all-selectors.js"; | |
| import { computePolicyTargetDeploymentSelectorWorker } from "./compute-policy-target-deployment-selector.js"; | |
| import { computePolicyTargetEnvironmentSelectorWorker } from "./compute-policy-target-environment-selector.js"; |
| try { | ||
| await db.transaction(async (tx) => { | ||
| await tx.execute( | ||
| sql` | ||
| SELECT * from ${schema.computedPolicyTargetEnvironment} | ||
| WHERE ${eq(schema.computedPolicyTargetEnvironment.policyTargetId, policyTarget.id)} | ||
| FOR UPDATE NOWAIT | ||
| `, | ||
| ); | ||
|
|
||
| await tx | ||
| .delete(schema.computedPolicyTargetEnvironment) | ||
| .where( | ||
| eq( | ||
| schema.computedPolicyTargetEnvironment.policyTargetId, | ||
| policyTarget.id, | ||
| ), | ||
| ); | ||
|
|
||
| const matchingEnvironments = await findMatchingEnvironments( | ||
| tx, | ||
| policyTarget, | ||
| workspaceId, | ||
| ); | ||
|
|
||
| if (matchingEnvironments.length === 0) return; | ||
| await tx | ||
| .insert(schema.computedPolicyTargetEnvironment) | ||
| .values(matchingEnvironments) | ||
| .onConflictDoNothing(); | ||
| }); | ||
| } catch (e: any) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Row-locking strategy can still race
FOR UPDATE NOWAIT on computed_policy_target_environment only locks existing
rows. When the first worker for a brand-new policyTarget arrives, the
table is empty → no row is locked → two workers can both proceed, resulting
in duplicate inserts (ignored thanks to ON CONFLICT DO NOTHING) but still
wasting work and opening a small window for inconsistent reads.
Locking the policy_target row instead guarantees single-writer semantics:
- SELECT * FROM ${schema.computedPolicyTargetEnvironment}
- WHERE ${eq(schema.computedPolicyTargetEnvironment.policyTargetId, policyTarget.id)}
+ SELECT id FROM ${schema.policyTarget}
+ WHERE ${eq(schema.policyTarget.id, policyTarget.id)}
FOR UPDATE NOWAITOptionally add a small retry back-off when re-queuing to avoid hot-looping.
📝 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.
| try { | |
| await db.transaction(async (tx) => { | |
| await tx.execute( | |
| sql` | |
| SELECT * from ${schema.computedPolicyTargetEnvironment} | |
| WHERE ${eq(schema.computedPolicyTargetEnvironment.policyTargetId, policyTarget.id)} | |
| FOR UPDATE NOWAIT | |
| `, | |
| ); | |
| await tx | |
| .delete(schema.computedPolicyTargetEnvironment) | |
| .where( | |
| eq( | |
| schema.computedPolicyTargetEnvironment.policyTargetId, | |
| policyTarget.id, | |
| ), | |
| ); | |
| const matchingEnvironments = await findMatchingEnvironments( | |
| tx, | |
| policyTarget, | |
| workspaceId, | |
| ); | |
| if (matchingEnvironments.length === 0) return; | |
| await tx | |
| .insert(schema.computedPolicyTargetEnvironment) | |
| .values(matchingEnvironments) | |
| .onConflictDoNothing(); | |
| }); | |
| } catch (e: any) { | |
| try { | |
| await db.transaction(async (tx) => { | |
| await tx.execute( | |
| sql` | |
| SELECT id FROM ${schema.policyTarget} | |
| WHERE ${eq(schema.policyTarget.id, policyTarget.id)} | |
| FOR UPDATE NOWAIT | |
| `, | |
| ); | |
| await tx | |
| .delete(schema.computedPolicyTargetEnvironment) | |
| .where( | |
| eq( | |
| schema.computedPolicyTargetEnvironment.policyTargetId, | |
| policyTarget.id, | |
| ), | |
| ); | |
| const matchingEnvironments = await findMatchingEnvironments( | |
| tx, | |
| policyTarget, | |
| workspaceId, | |
| ); | |
| if (matchingEnvironments.length === 0) return; | |
| await tx | |
| .insert(schema.computedPolicyTargetEnvironment) | |
| .values(matchingEnvironments) | |
| .onConflictDoNothing(); | |
| }); | |
| } catch (e: any) { |
| const policies = await getApplicablePolicies() | ||
| .environmentAndDeployment({ environmentId, deploymentId }) | ||
| .withoutResourceScope(); | ||
| const mergedPolicy = mergePolicies(policies); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate policy look-up – consider a single query inside a transaction
getApplicablePolicies() is executed twice, once for the status query (lines 70-73) and again during mutation (lines 123-126). This can be expensive and, more importantly, risks inconsistent reads if policies change between the two calls.
Wrap both the approval record insertion and policy fetch inside a single ctx.db.transaction, returning the merged result at the end, e.g.
-await ctx.db
- .insert(SCHEMA.policyRuleAnyApprovalRecord)
- .values(baseApprovalRecord);
-
-const policies = await getApplicablePolicies()
- .environmentAndDeployment({ environmentId, deploymentId })
- .withoutResourceScope();
-
-await createUserApprovalRecords(ctx.db, policies, baseApprovalRecord);
+await ctx.db.transaction(async (tx) => {
+ await tx.insert(SCHEMA.policyRuleAnyApprovalRecord).values(baseApprovalRecord);
+
+ const policies = await getApplicablePolicies(tx)
+ .environmentAndDeployment({ environmentId, deploymentId })
+ .withoutResourceScope();
+
+ await createUserApprovalRecords(tx, policies, baseApprovalRecord);
+});This ensures atomicity and halves the number of DB round-trips.
Also applies to: 123-126
| type PolicyWithUserApprovals = SCHEMA.Policy & { | ||
| versionUserApprovals: SCHEMA.PolicyRuleUserApproval[]; | ||
| }; | ||
| const createUserApprovalRecords = async ( | ||
| tx: Tx, | ||
| policies: PolicyWithUserApprovals[], | ||
| baseApprovalRecord: SCHEMA.BaseApprovalRecordInsert, | ||
| ) => { | ||
| const userApprovalRules = policies | ||
| .flatMap((p) => p.versionUserApprovals) | ||
| .filter((a) => a.userId === baseApprovalRecord.userId); | ||
|
|
||
| const records = userApprovalRules.map((rule) => ({ | ||
| ...baseApprovalRecord, | ||
| ruleId: rule.id, | ||
| })); | ||
|
|
||
| await tx.insert(SCHEMA.policyRuleUserApprovalRecord).values(records); | ||
| }; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against inserting zero-length arrays into policyRuleUserApprovalRecord
When the current user has no matching PolicyRuleUserApproval entries, records becomes an empty array.
Several SQL drivers (including Drizzle’s Postgres adapter) throw when .values([]) is invoked.
Add a fast-return to avoid an unnecessary round-trip / potential error.
const records = userApprovalRules.map((rule) => ({
...baseApprovalRecord,
ruleId: rule.id,
}));
- await tx.insert(SCHEMA.policyRuleUserApprovalRecord).values(records);
+ if (records.length > 0) {
+ await tx.insert(SCHEMA.policyRuleUserApprovalRecord).values(records);
+ }📝 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.
| type PolicyWithUserApprovals = SCHEMA.Policy & { | |
| versionUserApprovals: SCHEMA.PolicyRuleUserApproval[]; | |
| }; | |
| const createUserApprovalRecords = async ( | |
| tx: Tx, | |
| policies: PolicyWithUserApprovals[], | |
| baseApprovalRecord: SCHEMA.BaseApprovalRecordInsert, | |
| ) => { | |
| const userApprovalRules = policies | |
| .flatMap((p) => p.versionUserApprovals) | |
| .filter((a) => a.userId === baseApprovalRecord.userId); | |
| const records = userApprovalRules.map((rule) => ({ | |
| ...baseApprovalRecord, | |
| ruleId: rule.id, | |
| })); | |
| await tx.insert(SCHEMA.policyRuleUserApprovalRecord).values(records); | |
| }; | |
| type PolicyWithUserApprovals = SCHEMA.Policy & { | |
| versionUserApprovals: SCHEMA.PolicyRuleUserApproval[]; | |
| }; | |
| const createUserApprovalRecords = async ( | |
| tx: Tx, | |
| policies: PolicyWithUserApprovals[], | |
| baseApprovalRecord: SCHEMA.BaseApprovalRecordInsert, | |
| ) => { | |
| const userApprovalRules = policies | |
| .flatMap((p) => p.versionUserApprovals) | |
| .filter((a) => a.userId === baseApprovalRecord.userId); | |
| const records = userApprovalRules.map((rule) => ({ | |
| ...baseApprovalRecord, | |
| ruleId: rule.id, | |
| })); | |
| if (records.length > 0) { | |
| await tx.insert(SCHEMA.policyRuleUserApprovalRecord).values(records); | |
| } | |
| }; |
| const version = await getVersionWithMetadata(ctx.db, deploymentVersionId); | ||
| const { deploymentId } = version; | ||
|
|
||
| const baseApprovalRecord = { | ||
| deploymentVersionId, | ||
| userId: ctx.session.user.id, | ||
| status, | ||
| reason, | ||
| approvedAt: | ||
| status === SCHEMA.ApprovalStatus.Approved ? new Date() : undefined, | ||
| }; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Potential race condition – missing unique index / conflict handling on policyRuleAnyApprovalRecord
If two tabs submit an approval simultaneously, duplicate any-approval rows may be inserted.
Consider enforcing a (deploymentVersionId, userId) unique constraint and using onConflictDoUpdate or letting the mutation surface a friendly error to the caller.
Without this, users can accidentally spam approval history.
🏁 Script executed:
#!/bin/bash
set -e
echo "1. Searching for policyRuleAnyApprovalRecord usage in code..."
rg --context 3 "policyRuleAnyApprovalRecord"
echo
echo "2. Locating Prisma schema file..."
SCHEMA_FILE=$(find . -type f -name "schema.prisma" | head -n1)
if [[ -z "$SCHEMA_FILE" ]]; then
echo "⚠️ schema.prisma not found."
exit 1
else
echo "Found schema at: $SCHEMA_FILE"
fi
echo
echo "3. Inspecting PolicyRuleAnyApprovalRecord model definition for unique constraints..."
rg -n "model PolicyRuleAnyApprovalRecord" -A 20 "$SCHEMA_FILE"Length of output: 6530
🏁 Script executed:
#!/bin/bash
set -e
echo "1. Displaying the policyRuleAnyApprovalRecord Drizzle schema:"
sed -n '1,200p' packages/db/src/schema/rules/approval-any.ts || echo "⚠️ File not found: approval-any.ts"
echo
echo "2. Searching for uniqueIndex across db schemas:"
rg --context 2 "uniqueIndex" -n packages/db || echo "No uniqueIndex usages found."
echo
echo "3. Searching for generic index definitions:"
rg --context 2 "index(" -n packages/db || echo "No index definitions found."
echo
echo "4. Locating any migrations directories:"
find . -type d -name migrations || echo "No migrations directories found."
echo
echo "5. Listing TypeScript migration files:"
find . -type f -path "*/migrations/*.ts" || echo "No migration files found."Length of output: 23962
Handle insert conflicts on policyRuleAnyApprovalRecord
We’ve confirmed there is already a unique index on (deploymentVersionId, userId) in packages/db/src/schema/rules/approval-any.ts. However, the insert in packages/api/src/router/deployment-version-checks/approvals.ts (lines 107–118) doesn’t handle conflicts. A concurrent approval request will trigger a unique‐constraint violation and surface as a 500 error.
Action items:
- In the insert call, add
.onConflictDoUpdate()(or.onConflictDoNothing()) to merge or dedupe duplicate submissions, or - Wrap the insert in a try/catch to intercept unique‐constraint errors and return a clear, friendly error to the client.
This ensures race conditions can’t spam your approval history or crash the endpoint.
| try { | ||
| await db.transaction(async (tx) => { | ||
| await tx.execute( | ||
| sql` | ||
| SELECT * from ${schema.computedPolicyTargetDeployment} | ||
| WHERE ${eq(schema.computedPolicyTargetDeployment.policyTargetId, policyTarget.id)} | ||
| FOR UPDATE NOWAIT | ||
| `, | ||
| ); | ||
|
|
||
| await tx | ||
| .delete(schema.computedPolicyTargetDeployment) | ||
| .where( | ||
| eq( | ||
| schema.computedPolicyTargetDeployment.policyTargetId, | ||
| policyTarget.id, | ||
| ), | ||
| ); | ||
|
|
||
| const matchingDeployments = await findMatchingDeployments( | ||
| tx, | ||
| policyTarget, | ||
| workspaceId, | ||
| ); | ||
|
|
||
| if (matchingDeployments.length === 0) return; | ||
| await tx | ||
| .insert(schema.computedPolicyTargetDeployment) | ||
| .values(matchingDeployments) | ||
| .onConflictDoNothing(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Transaction could still deadlock – lock the policyTarget row first
Multiple workers may lock computedPolicyTargetDeployment rows for the same policy target, then delete/insert.
Locking the parent policyTarget row first gives a single canonical lock order and eliminates cyclical waits.
await db.transaction(async (tx) => {
+ // Acquire lock on the parent – ensures global ordering.
+ await tx
+ .select()
+ .from(schema.policyTarget)
+ .where(eq(schema.policyTarget.id, policyTarget.id))
+ .forUpdate();
await tx.execute(
sql`
SELECT * from ${schema.computedPolicyTargetDeployment}
1 participant
Summary by CodeRabbit
New Features
Improvements
Refactor
Bug Fixes