fix: revert optimization change in compute policy release target worker #560
Conversation
WalkthroughThis update refactors the event worker's computation of policy target release targets by replacing custom inline transactional logic with a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API
participant DB
participant EventWorker
User->>API: Create new resource (no matching policy target)
API->>DB: Insert resource
API->>EventWorker: Trigger compute-policy-target-release-target-selector
EventWorker->>DB: Call computePolicyTargets (transactionally delete old and insert new computed release targets)
EventWorker->>DB: Query all release targets in workspace
EventWorker->>API: Dispatch evaluation jobs for all release targets
Possibly related PRs
Suggested reviewers
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
e2e/tests/api/release.spec.ts (1)
453-454: Remove console.log statements from test.Debug console.log statements should be removed from production test code.
- for (const release of releases) { - console.log(release); - }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/event-worker/src/workers/compute-policy-target-release-target-selector.ts(3 hunks)e2e/tests/api/release.spec.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/compute-policy-target-release-target-selector.tse2e/tests/api/release.spec.ts
🧬 Code Graph Analysis (2)
apps/event-worker/src/workers/compute-policy-target-release-target-selector.ts (3)
packages/db/src/schema/policy.ts (2)
policyTarget(64-78)policy(42-62)packages/db/src/client.ts (1)
db(15-15)apps/event-worker/src/utils/dispatch-evaluate-jobs.ts (1)
dispatchEvaluateJobs(5-20)
e2e/tests/api/release.spec.ts (3)
e2e/tests/fixtures.ts (1)
test(11-26)packages/db/src/schema/workspace.ts (1)
workspace(18-27)packages/db/src/schema/release.ts (2)
releaseTarget(20-42)release(113-124)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
🔇 Additional comments (6)
apps/event-worker/src/workers/compute-policy-target-release-target-selector.ts (5)
3-3: Import update properly reflects code changes.The
inArrayimport was removed as it's no longer used after the logic simplification.
69-69: Good optimization with eager loading.Eager loading the policy relation is an appropriate optimization that avoids an additional query later.
73-74: Direct property access improves code clarity.Destructuring the policy and workspaceId makes the code more readable and works well with the eager loading added above.
77-94: Simplified transaction logic for better reliability.The transaction now simply deletes all existing computed entries for the policy target without trying to compute differences. This approach is more robust and less error-prone.
108-123: Verify performance implications of processing all release targets.The code now queries and dispatches evaluation jobs for all release targets in the workspace rather than just those affected by changes. While this is simpler and more reliable, it could potentially lead to more processing than necessary in large workspaces.
Consider monitoring the performance of this change in production, especially for workspaces with many release targets. You might want to add metrics to track:
- Number of release targets being evaluated per policy target change
- Processing time for the evaluation queue
e2e/tests/api/release.spec.ts (1)
335-464: Good test coverage for the reverted optimization.This test properly verifies that releases are created when a new resource is added that doesn't match any policy target. It ensures the simplified computation logic correctly handles this case.
The test is comprehensive, covering:
- Policy creation with a specific target selector
- Deployment and version creation
- Resource creation with a non-matching identifier
- Verification of release target and release creation
This aligns perfectly with the changes in the compute-policy-target-release-target-selector worker.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/event-worker/src/workers/compute-policy-target-release-target-selector.ts (2)
27-27: Fix typo in variable nameThere's a typo in the variable name:
changedReleaseTaretIdsshould bechangedReleaseTargetIds(missing 'g').- const changedReleaseTaretIds = await computePolicyTargets( + const changedReleaseTargetIds = await computePolicyTargets(
41-41: Fix typo in variable referenceThe same typo exists when using the variable in the query condition.
- inArray(schema.releaseTarget.id, changedReleaseTaretIds), + inArray(schema.releaseTarget.id, changedReleaseTargetIds),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/event-worker/src/workers/compute-policy-target-release-target-selector.ts(2 hunks)packages/db/src/queries/compute-policy-targets.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/compute-policy-target-release-target-selector.tspackages/db/src/queries/compute-policy-targets.ts
🧬 Code Graph Analysis (1)
apps/event-worker/src/workers/compute-policy-target-release-target-selector.ts (4)
packages/db/src/queries/compute-policy-targets.ts (1)
computePolicyTargets(53-94)packages/db/src/client.ts (1)
db(15-15)packages/db/src/schema/policy.ts (1)
policyTarget(64-78)apps/event-worker/src/utils/dispatch-evaluate-jobs.ts (1)
dispatchEvaluateJobs(5-20)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (6)
apps/event-worker/src/workers/compute-policy-target-release-target-selector.ts (1)
27-47: Good refactoring to use centralized computation functionThe implementation now uses the centralized
computePolicyTargetsfunction, which improves code organization and maintenance. The refactoring correctly:
- Retrieves changed release target IDs
- Queries only the relevant release targets
- Filters out deleted resources
- Dispatches evaluation jobs appropriately
Overall, this is a good improvement to the codebase structure.
packages/db/src/queries/compute-policy-targets.ts (5)
7-51: Well-structured query with proper joins and selectorsThe
findMatchingReleaseTargetshelper function efficiently queries for release targets using appropriate joins across resource, deployment, and environment tables. The selector pattern provides a clean way to apply complex filtering conditions based on policy target selectors.
58-65: Good use of FOR UPDATE NOWAIT to prevent deadlocksThe implementation correctly uses FOR UPDATE NOWAIT to lock rows and fail immediately if they're already locked, which is a good practice to prevent deadlocks. This works well with the error handling in the caller code.
79-92: Efficient change detection using SetsThe code efficiently calculates created and deleted associations using Sets for fast lookups. The approach is clear and well-structured, with descriptive variable names that explain the intent of each step in the process.
86-90: Good optimization with conditional insertThe code correctly avoids unnecessary database operations by only performing the insert when there are matching release targets. The use of
onConflictDoNothing()ensures idempotence, which is important for robustness.
53-57:✅ Verification successful
Verify nested transaction behavior
The function receives a transaction object
dbbut then creates a new transaction with it. Nested transactions might not behave as expected across all database systems and ORMs.Consider either:
- Removing the inner transaction if the function is always called within a transaction
- Adding a parameter to control whether to use a transaction
🏁 Script executed:
#!/bin/bash # Check if this pattern is used elsewhere in the codebase rg -A 3 "transaction\(async \(" --type tsLength of output: 15034
Nested transactions are supported and consistently used; no changes needed
- A code search shows the same
db.transaction(async (tx) => …)pattern in 40+ places (API routers, workers, migrations, etc.).- With Postgres and Kysely, nested transactions become savepoints and behave predictably on rollback/commit.
No actionable fixes are required here.
Summary by CodeRabbit
New Features
Refactor