feat: delete resource worker #498
Conversation
WalkthroughThis change introduces an asynchronous, event-driven approach to resource deletion across the system. A new worker, Changes
Sequence Diagram(s)sequenceDiagram
participant API
participant Queue
participant Worker
participant DB
API->>Queue: Enqueue DeleteResource job (resourceId, resource)
Queue->>Worker: Dispatch DeleteResource job
Worker->>DB: Begin transaction
Worker->>DB: Soft delete resource (set deletedAt)
Worker->>DB: Delete related deployments, env resources, release targets
Worker->>DB: Commit transaction
Worker->>Queue: Dispatch DeploymentResourceRemoved events (for exit hooks)
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 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 (4)
🔇 Additional comments (3)
✨ 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: 6
🧹 Nitpick comments (1)
apps/event-worker/src/workers/delete-resource.ts (1)
44-61: Consider adding error logging for hook failures.Using
Promise.allSettledensures all hooks are processed regardless of individual failures, which is excellent. However, consider adding logging for rejected promises to help with debugging.- await Promise.allSettled(handleEventPromises); + const results = await Promise.allSettled(handleEventPromises); + // Log any errors for observability + results + .filter((result): result is PromiseRejectedResult => result.status === "rejected") + .forEach((result, index) => { + console.error(`Failed to handle event for deployment ${deployments[index].id}:`, result.reason); + });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/event-worker/src/workers/delete-resource.ts(1 hunks)apps/event-worker/src/workers/index.ts(2 hunks)apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts(1 hunks)apps/webservice/src/app/api/v1/workspaces/[workspaceId]/resources/identifier/[identifier]/route.ts(2 hunks)packages/api/src/router/resources.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/resource-builder.ts(3 hunks)packages/events/src/types.ts(1 hunks)packages/job-dispatch/src/resource/handle-provider-scan.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/db/src/selectors/compute/resource-builder.tspackages/db/src/selectors/compute/environment-builder.tspackages/db/src/selectors/compute/deployment-builder.tsapps/event-worker/src/workers/index.tsapps/webservice/src/app/api/v1/workspaces/[workspaceId]/resources/identifier/[identifier]/route.tspackages/job-dispatch/src/resource/handle-provider-scan.tspackages/api/src/router/resources.tspackages/events/src/types.tsapps/webservice/src/app/api/v1/resources/[resourceId]/route.tsapps/event-worker/src/workers/delete-resource.ts
🧬 Code Graph Analysis (3)
apps/event-worker/src/workers/index.ts (1)
apps/event-worker/src/workers/delete-resource.ts (1)
deleteResourceWorker(63-74)
packages/job-dispatch/src/resource/handle-provider-scan.ts (2)
packages/events/src/index.ts (1)
getQueue(28-34)packages/db/src/selectors/index.ts (1)
selector(18-18)
packages/events/src/types.ts (5)
packages/db/src/schema/resource.ts (2)
Resource(105-105)ResourceVariable(417-417)packages/db/src/schema/deployment-variables.ts (1)
DeploymentVariable(48-48)packages/db/src/schema/environment.ts (1)
Environment(86-86)packages/validators/src/resources/conditions/resource-condition.ts (1)
ResourceCondition(29-39)packages/db/src/schema/deployment.ts (1)
Deployment(105-105)
⏰ 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 (17)
packages/events/src/types.ts (2)
42-44: Type updates look goodThe type definitions for the
NewEnvironment,NewResource, andNewPolicychannels have been appropriately updated with standard type imports.
54-56: Channel additions for resource lifecycle eventsGood addition of the
UpdatedResourceandDeleteResourcechannel types, which aligns with the event-driven approach for resource management. These new channel types enable asynchronous processing of resource updates and deletions.apps/event-worker/src/workers/index.ts (2)
6-6: Worker import added correctlyThe import for the new
deleteResourceWorkerhas been properly added.
38-38: Delete resource worker registrationThe
deleteResourceWorkerhas been correctly registered for theChannel.DeleteResourcechannel, completing the event-driven architecture for resource deletion.apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts (1)
141-141: Successful implementation of asynchronous resource deletionThe code has been updated to use the event-driven deletion approach via the
DeleteResourcechannel instead of direct synchronous deletion. This aligns with the broader architectural shift in the system.packages/db/src/selectors/compute/resource-builder.ts (1)
5-5: Import modification looks goodThe addition of
isNotNullto the imports is appropriate for the new filter condition being added.apps/webservice/src/app/api/v1/workspaces/[workspaceId]/resources/identifier/[identifier]/route.ts (2)
6-6: Import changes correctly reflect new approachThe imports have been updated to support the asynchronous deletion pattern, removing the direct
deleteResourcesimport and adding the event queue functionality.
99-99: Successful implementation of asynchronous resource deletionThe code has been updated to use the event-driven deletion approach via the
DeleteResourcechannel instead of direct synchronous deletion. This change is consistent with the modifications in other API routes.packages/job-dispatch/src/resource/handle-provider-scan.ts (2)
45-47: Effective implementation of batch deletion via event queueThe code has been updated to queue deletions in bulk through the
DeleteResourcechannel, which aligns with the new asynchronous deletion pattern. Good use ofaddBulkfor efficient batch processing.
54-57: Return value properly includes deleted resourcesThe updated return structure preserves the information about which resources were deleted, ensuring downstream operations have access to this data even though the actual deletion now happens asynchronously.
apps/event-worker/src/workers/delete-resource.ts (6)
1-10: Well-organized imports.The imports are appropriately grouped and organized, keeping related imports together.
11-15: Good implementation of soft deletion.Using a transaction to set the
deletedAttimestamp is the correct approach for implementing soft deletion.
17-22: Appropriate returning of deleted entities.Good practice to return the deleted release targets, as they're needed for subsequent operations in the deletion flow.
23-32: Efficient parallel deletion of computed resources.Using
Promise.allto handle multiple deletions in parallel is an efficient approach.
33-43: Effective batch deletion strategy.Using
inArrayfor batch deletion of computed release targets is an efficient approach.
63-74: Well-structured transactional approach to resource deletion.The worker implementation uses a transaction to ensure all deletion operations are atomic, which is crucial for maintaining data consistency. The sequence of operations is logical and ensures all related entities are properly cleaned up.
packages/api/src/router/resources.ts (1)
698-706: Good shift to asynchronous deletion pattern.Changing from synchronous deletion to an asynchronous job-based approach is a good architectural decision. This improves API responsiveness and allows for better error handling and retry mechanisms.
| isResourceMatchingDeployment, | ||
| inArray(SCHEMA.resource.id, this.resourceIds), | ||
| isNotNull(SCHEMA.resource.deletedAt), | ||
| ), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the query filter logic for deleted resources
The added condition isNotNull(SCHEMA.resource.deletedAt) would only match resources that have been deleted (where deletedAt is not null). This seems counterintuitive for a method that finds matching environment-deployment pairs, as typically you'd want to exclude deleted resources, not include only deleted ones.
🏁 Script executed:
#!/bin/bash
# Check if this filtering approach is consistent with other similar queries in the codebase
# Look for similar resource filtering patterns in the codebase
rg -A 3 "isNotNull\(.*\.deletedAt\)" --type tsLength of output: 1896
#!/bin/bash
# Display relevant context around deletedAt filter in resource-builder.ts
sed -n '1,200p' packages/db/src/selectors/compute/resource-builder.ts#!/bin/bash
# Display lines around the deletedAt filter in resource-builder.ts for context
sed -n '80,160p' packages/db/src/selectors/compute/resource-builder.tsReplace isNotNull with isNull to exclude deleted resources
The current filter
isNotNull(SCHEMA.resource.deletedAt),matches only deleted resources. For methods that compute active environment–deployment pairs, you should use isNull to exclude those with a non‑null deletedAt.
Please update in all selector builders:
• packages/db/src/selectors/compute/resource‑builder.ts (around line 112)
• packages/db/src/selectors/compute/deployment‑builder.ts
• packages/db/src/selectors/compute/environment‑builder.ts
Suggested diff:
- isNotNull(SCHEMA.resource.deletedAt),
+ isNull(SCHEMA.resource.deletedAt),
Summary by CodeRabbit