fix: stabilize release creation and add e2e #518
Conversation
WalkthroughThis set of changes refactors and enhances the release creation and evaluation workflow. The transaction logic in the release target computation worker is updated to always return all relevant release targets, not just newly created ones, and downstream job dispatching is adjusted accordingly. The evaluation worker now acquires row-level locks to prevent concurrent modifications. The updated resources worker is reworked to enqueue jobs for all deployments and environments in a workspace rather than just the single resource. The API route for updating resources now enqueues a job upon successful update. The end-to-end release tests are expanded and refactored for more comprehensive and isolated coverage. Some logging statements are removed for clarity, and minor YAML configuration adjustments are made. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Worker
participant DB
participant Queue
Client->>API: PATCH /resources/:id
API->>DB: Update resource
API->>Queue: Enqueue UpdatedResource job
API-->>Client: Return updated resource
Queue->>Worker: UpdatedResource job
Worker->>DB: Fetch workspace (by resource.workspaceId)
Worker->>Queue: Enqueue jobs for deployments and environments
Worker->>DB: Compute release targets (transaction)
DB-->>Worker: All release targets (after insert if needed)
Worker->>Queue: Dispatch evaluate jobs for all release targets
Queue->>Worker: EvaluateReleaseTarget job
Worker->>DB: Lock releaseTarget row (FOR UPDATE NOWAIT)
Worker->>DB: Evaluate and process releaseTarget
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:
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: 3
🧹 Nitpick comments (4)
apps/event-worker/src/workers/compute-systems-release-targets.ts (2)
83-87: Variable shadowing – rename innerreleaseTargetsfor clarityThe constant declared on line 83 shadows the one defined in the outer scope (returned from the transaction).
While TypeScript allows this, using the same identifier for two different shapes (Promise<…>vs. rawReleaseTargetRow[]) increases cognitive load and invites bugs during future edits.-const releaseTargets = await db.transaction(async (tx) => { +const allReleaseTargets = await db.transaction(async (tx) => {
136-141: Minor performance optimisation – avoid second queryYou already have
previousReleaseTargets+created; concatenating these two arrays yields the exact result you fetch again, saving one round-trip.-return tx.query.releaseTarget.findMany({ … }); +return [...previousReleaseTargets, ...created];e2e/tests/api/release.spec.ts (2)
35-46: DRY up repeated deployment bootstrap logicEvery test block repeats ~12 lines to create a deployment & first version.
Extracting a helper (e.g.createDeploymentWithVersion(api, systemId)) would:
- Remove >70 duplicated lines
- Centralise error handling & assertion logic
- Shorten test cases, improving readability
Also applies to: 117-128, 231-240, 343-352, 467-476, 591-602
323-325: Remove strayconsole.logstatementsDebug prints inside committed tests clutter CI logs and can leak sensitive payloads.
Please delete them or guard with a verbose flag.- for (const release of releases) { - console.log(release); - }Also applies to: 447-449, 573-574
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/event-worker/src/workers/compute-systems-release-targets.ts(3 hunks)apps/event-worker/src/workers/evaluate-release-target.ts(2 hunks)apps/event-worker/src/workers/updated-resources/index.ts(2 hunks)apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts(1 hunks)e2e/tests/api/release.spec.ts(5 hunks)e2e/tests/api/release.spec.yaml(1 hunks)packages/rule-engine/src/manager/variables/variables.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/rule-engine/src/manager/variables/variables.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.tsapps/event-worker/src/workers/evaluate-release-target.tsapps/event-worker/src/workers/updated-resources/index.tsapps/event-worker/src/workers/compute-systems-release-targets.tse2e/tests/api/release.spec.ts
🧬 Code Graph Analysis (3)
apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts (2)
packages/events/src/index.ts (1)
getQueue(28-34)packages/db/src/schema/resource.ts (1)
resource(59-87)
apps/event-worker/src/workers/evaluate-release-target.ts (1)
packages/db/src/schema/release.ts (1)
releaseTarget(20-42)
apps/event-worker/src/workers/compute-systems-release-targets.ts (2)
packages/db/src/client.ts (1)
db(15-15)apps/event-worker/src/utils/dispatch-evaluate-jobs.ts (1)
dispatchEvaluateJobs(5-11)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (8)
e2e/tests/api/release.spec.yaml (1)
23-23: Syntax simplification looks good.The unquoted version of
containsis valid YAML syntax and maintains the same functionality while being cleaner.apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts (1)
138-140: Good enhancement to trigger downstream processing.This change now ensures that whenever a resource is updated, a job is enqueued to process related entities. This ties in well with the updated resource worker implementation.
I notice you're using the original resource object (before update) in the enqueued job. Verify this is the intended behavior:
#!/bin/bash # Check for any other occurrences of UpdatedResource channel usage to confirm similar patterns rg -A 3 "Channel\.UpdatedResource"apps/event-worker/src/workers/evaluate-release-target.ts (2)
4-4: Import addition for SQL functionality.Adding the
sqlimport is necessary for the row-level locking implementation below.
189-196: Great addition of row-level locking.The
FOR UPDATE NOWAITclause prevents concurrent modifications to the same release target, which could lead to race conditions. The NOWAIT option makes it fail immediately instead of blocking if the row is already locked.Error handling for lock conflicts (code 55P03) is already properly implemented in the catch block.
apps/event-worker/src/workers/updated-resources/index.ts (3)
3-5: Added necessary imports for database operations.These imports are required for the enhanced implementation below that queries the workspace and related entities.
30-36: Good database query to fetch the entire workspace context.This query efficiently retrieves the workspace along with all its systems, environments, and deployments in a single database call, which is more efficient than making multiple separate queries.
The error handling for a missing workspace is also appropriate.
37-53: Comprehensive approach to downstream processing.This implementation ensures that all deployments and environments related to the updated resource are processed, not just the resource itself. The deduplication options with TTL are a good optimization to prevent unnecessary duplicate work.
The nested loops are clear and maintainable. Each queue operation has proper error handling through the worker framework.
e2e/tests/api/release.spec.ts (1)
212-219: Assumption on release ordering may break
releases.at(0)presumes API ordering (latest first). If the backend ever changes sort order, tests will silently assert the wrong release.
Fetch bycreatedAtDESC or locate byversion.taginstead of relying on index 0.Also applies to: 577-583, 702-708
| await dispatchEvaluateJobs(releaseTargets); | ||
| } catch (e: any) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Type safety: dispatchEvaluateJobs expects identifiers only
dispatchEvaluateJobs serialises every field in rts onto the queue.
If tx.query.releaseTarget.findMany includes heavy columns (timestamps, JSON blobs), you’ll bloat the job payload.
Consider projecting only { deploymentId, environmentId, resourceId } before dispatching.
-await dispatchEvaluateJobs(releaseTargets);
+await dispatchEvaluateJobs(
+ releaseTargets.map(({ deploymentId, environmentId, resourceId }) => ({
+ deploymentId,
+ environmentId,
+ resourceId,
+ })),
+);There was a problem hiding this comment.
this is not really a concern
There was a problem hiding this comment.
@adityachoudhari26 Understood! I appreciate the clarification. I'll defer to your judgment on this since you have a better understanding of the system's requirements and performance characteristics.
| await page.waitForTimeout(1_000); | ||
|
|
||
| const importedResource = importedEntities.resources.at(0)!; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fixed waitForTimeout calls lead to flaky & slow suites
Hard sleeps gamble on timing and scale linearly with test count ( >30 s per run here).
Prefer polling for the desired condition:
await expect
.poll(async () => {
const res = await api.GET("/v1/release-targets/{id}/releases", { /* … */ });
return res.data?.length ?? 0;
})
.toBe(expectedCount, { timeout: 15_000 });This waits just long enough and fails fast when the condition is unmet.
Also applies to: 199-200, 307-308, 388-389, 513-514, 656-657, 682-683
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/event-worker/src/workers/updated-resources/index.ts (2)
30-35: Workspace query with related entities looks good, but consider adding error details.The implementation now correctly queries the entire workspace with related systems, environments, and deployments. This ensures comprehensive processing of all related entities when a resource is updated.
When throwing the "Workspace not found" error, consider including the resource's workspaceId in the error message to aid debugging.
- if (workspace == null) throw new Error("Workspace not found"); + if (workspace == null) throw new Error(`Workspace not found for workspaceId: ${resource.workspaceId}`);
51-56: Bulk enqueuing is more efficient but consider error handling.Switching to bulk operations with
addBulk()is more efficient than individual queue additions. However, there's no error handling for the queue operations. Consider adding try/catch blocks to handle potential queue errors gracefully.Also, it might be worth adding span attributes for the number of jobs being enqueued to aid in monitoring and debugging.
+ span.setAttribute("deployment.jobs.count", deploymentJobs.length); + span.setAttribute("environment.jobs.count", environmentJobs.length); + try { await getQueue(Channel.ComputeDeploymentResourceSelector).addBulk( deploymentJobs, ); await getQueue(Channel.ComputeEnvironmentResourceSelector).addBulk( environmentJobs, ); + } catch (error) { + span.setAttribute("error", true); + span.recordException(error); + throw error; + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/event-worker/src/workers/updated-resources/index.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/updated-resources/index.ts
🧬 Code Graph Analysis (1)
apps/event-worker/src/workers/updated-resources/index.ts (6)
packages/db/src/schema/workspace.ts (1)
workspace(18-27)packages/db/src/client.ts (1)
db(15-15)packages/db/src/schema/resource.ts (1)
resource(59-87)packages/db/src/schema/deployment.ts (1)
deployment(69-93)packages/db/src/schema/environment.ts (1)
environment(59-84)packages/events/src/index.ts (1)
getQueue(28-34)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (3)
apps/event-worker/src/workers/updated-resources/index.ts (3)
3-5: Imported database dependencies correctly for the refactored implementation.The imports have been appropriately updated to support the new database query pattern, including the
eqoperator for equality conditions, the database client, and the schema definitions.
37-42: LGTM: Deployment job mapping is correctly implemented.The code efficiently maps all deployments from all systems in the workspace to job objects that can be enqueued. The flatMap approach is a clean way to handle the nested data structure.
44-49: LGTM: Environment job mapping follows the same pattern as deployments.The implementation for environment jobs mirrors the deployment job mapping, maintaining consistency in the codebase.
| const workspace = await db.query.workspace.findFirst({ | ||
| where: eq(schema.workspace.id, resource.workspaceId), | ||
| with: { systems: { with: { environments: true, deployments: true } } }, | ||
| }); | ||
|
|
||
| if (workspace == null) throw new Error("Workspace not found"); | ||
|
|
||
| const deploymentJobs = workspace.systems.flatMap((system) => | ||
| system.deployments.map((deployment) => ({ | ||
| name: deployment.id, | ||
| data: deployment, | ||
| })), | ||
| ); | ||
|
|
||
| await getQueue(Channel.ComputeEnvironmentResourceSelector).add( | ||
| resource.id, | ||
| resource, | ||
| const environmentJobs = workspace.systems.flatMap((system) => | ||
| system.environments.map((environment) => ({ | ||
| name: environment.id, | ||
| data: environment, | ||
| })), | ||
| ); | ||
|
|
||
| await getQueue(Channel.ComputeDeploymentResourceSelector).addBulk( | ||
| deploymentJobs, | ||
| ); | ||
| await getQueue(Channel.ComputeEnvironmentResourceSelector).addBulk( | ||
| environmentJobs, | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider potential performance impact for large workspaces.
The new implementation fetches all systems, environments, and deployments for a workspace, which could be resource-intensive for workspaces with many entities. Consider implementing pagination or limiting the query if performance becomes an issue.
Additionally, you might want to add some basic logging to indicate the number of jobs being processed, which would be helpful for monitoring.
🏁 Script executed:
#!/bin/bash
# Check if there are workspaces with many systems, deployments, or environments that might cause performance issues
# Find workspaces with large numbers of systems
echo "Workspaces with many systems:"
rg -l "systems:" --type ts | xargs grep -l "findFirst" | xargs grep -l "workspace" | head -n 5
# Check if there are other places in the codebase where similar bulk queries are performed
echo "Similar bulk queries in the codebase:"
rg -A 3 "with: \{ systems: \{ with:" --type tsLength of output: 769
Add performance guardrails and logging for bulk job processing
This handler eagerly loads every system, environment, and deployment for a workspace—which could exhaust memory or slow down the worker when a workspace grows large. To mitigate:
• In apps/event-worker/src/workers/updated-resources/index.ts (around line 30):
– Add a log before enqueuing to report job counts:
ts const deploymentCount = deploymentJobs.length; const environmentCount = environmentJobs.length; console.log( `Enqueuing ${deploymentCount} deployment jobs and ${environmentCount} environment jobs for workspace ${resource.workspaceId}` );
– If either array is large, batch the calls to addBulk in configurable chunks (e.g., 100 jobs per batch) to avoid overwhelming the queue client or worker:
ts const chunkSize = 100; for (let i = 0; i < deploymentJobs.length; i += chunkSize) { await getQueue(Channel.ComputeDeploymentResourceSelector) .addBulk(deploymentJobs.slice(i, i + chunkSize)); } // Same for environmentJobs…
• For very large workspaces, consider paginating the DB query (using take/skip) or filtering by recent changes rather than loading everything at once.
These changes will keep memory and response times bounded and give you visibility into workload spikes.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores