init upsert resource worker #460
Conversation
WalkthroughThe changes introduce a dedicated worker for processing upserted resources by adding the Changes
Sequence Diagram(s)sequenceDiagram
participant R as Resource
participant W as processUpsertedResourceWorker
participant DB as Database
participant U as upsertReleaseTargets
participant E as dispatchExitHooks
participant Q as EvaluationQueue
R->>W: Trigger upserted resource event
W->>DB: Query current release targets by resource.id
DB-->>W: Return current release targets
W->>U: Call upsertReleaseTargets(resource)
U-->>W: Return new release targets
W->>DB: Delete obsolete release targets
W->>E: Dispatch exit hooks concurrently
W->>Q: Add new release targets to evaluation queue
W-->>R: Complete processing
Possibly related PRs
Suggested reviewers
Poem
✨ 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
|
![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 (7)
apps/event-worker/src/workers/upsert-resource/worker.ts (1)
12-35: Consider validatingjob.data.resourcebefore usage.The code directly destructures
resourcefromjob.data. If the job payload is malformed or missing theresourceproperty, an unhandled error may occur. Adding optional checks or assertions can provide clearer feedback.try { const { data } = job; + if (!data || !data.resource) { + log.error("Invalid job data received, missing `resource`"); + return; + } const { resource } = data;apps/event-worker/src/workers/upsert-resource/resource-db-upsert.ts (2)
11-31: Upsert logic is straightforward and efficient.Using
onConflictDoUpdatewithbuildConflictUpdateColumnsis an effective approach. Consider adding any relevant columns (e.g.,'lockedAt') if needed for conflict resolution, but that may be optional depending on your requirements.
42-49: Review handling of sensitive variables.Storing sensitive data in plain text carries risk if the database is compromised. Evaluate whether additional encryption or a secret vault approach would be beneficial when
v.sensitiveis true.apps/event-worker/src/workers/upsert-resource/existing-resource.ts (2)
1-16: Imports and type definitions are consistent.Type
ResourceToInsertis repeated here for convenience, matching usage inresource-db-upsert.ts. Keeping them in a shared location could reduce duplication, but it’s acceptable if intentionally segregated.
84-132:handleExistingResourceflow is coherent but note partial failures.The concurrent calls in
Promise.allSettledensure that errors in one part do not halt the entire process, but also imply that unhandled failures might leave partial state changes. This is typically acceptable in event-driven systems but confirm that partial updates are safe.apps/event-worker/src/workers/upsert-resource/upsert-release-targets.ts (2)
5-40:getReleaseTargetInsertsForSystemusage is correct but be mindful of repeated queries.The code individually queries
db.query.resource.findFirstfor each environment-deployment pair. For large sets, consider bulk-finding or caching. Otherwise, logic is sound.
41-62: Upsert of release targets is clean.Using
.onConflictDoNothing()helps avoid duplication, and returning the inserted rows is helpful for subsequent usage. Consider logging the generated release targets for debugging or monitoring..returning(); +// Optional: add a debug log to confirm how many targets were inserted +console.debug("Inserted release targets", releaseTargetInserts.length);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/event-worker/src/workers/index.ts(2 hunks)apps/event-worker/src/workers/upsert-resource/existing-resource.ts(1 hunks)apps/event-worker/src/workers/upsert-resource/new-resource.ts(1 hunks)apps/event-worker/src/workers/upsert-resource/resource-db-upsert.ts(1 hunks)apps/event-worker/src/workers/upsert-resource/upsert-release-targets.ts(1 hunks)apps/event-worker/src/workers/upsert-resource/worker.ts(1 hunks)packages/events/src/types.ts(2 hunks)packages/job-dispatch/src/resource/upsert.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/index.tspackages/job-dispatch/src/resource/upsert.tsapps/event-worker/src/workers/upsert-resource/worker.tsapps/event-worker/src/workers/upsert-resource/upsert-release-targets.tsapps/event-worker/src/workers/upsert-resource/new-resource.tspackages/events/src/types.tsapps/event-worker/src/workers/upsert-resource/resource-db-upsert.tsapps/event-worker/src/workers/upsert-resource/existing-resource.ts
🧬 Code Definitions (7)
apps/event-worker/src/workers/index.ts (1)
apps/event-worker/src/workers/upsert-resource/worker.ts (1)
upsertResourceWorker(12-35)
packages/job-dispatch/src/resource/upsert.ts (1)
packages/db/src/schema/resource.ts (1)
InsertResource(115-115)
apps/event-worker/src/workers/upsert-resource/upsert-release-targets.ts (4)
packages/db/src/common.ts (1)
Tx(22-22)packages/rule-engine/src/types.ts (1)
ReleaseTargetIdentifier(74-78)packages/db/src/schema/resource.ts (1)
resource(59-87)packages/db/src/schema/workspace.ts (1)
workspace(18-27)
apps/event-worker/src/workers/upsert-resource/new-resource.ts (5)
packages/db/src/schema/resource.ts (2)
InsertResource(115-115)resource(59-87)packages/db/src/common.ts (1)
Tx(22-22)apps/event-worker/src/workers/upsert-resource/resource-db-upsert.ts (1)
dbUpsertResource(11-51)apps/event-worker/src/workers/upsert-resource/upsert-release-targets.ts (1)
upsertReleaseTargets(41-62)packages/events/src/index.ts (1)
getQueue(26-32)
packages/events/src/types.ts (1)
packages/db/src/schema/resource.ts (1)
InsertResource(115-115)
apps/event-worker/src/workers/upsert-resource/resource-db-upsert.ts (2)
packages/db/src/schema/resource.ts (2)
InsertResource(115-115)resource(59-87)packages/db/src/common.ts (3)
Tx(22-22)buildConflictUpdateColumns(30-46)takeFirst(9-13)
apps/event-worker/src/workers/upsert-resource/existing-resource.ts (4)
packages/db/src/common.ts (1)
Tx(22-22)packages/rule-engine/src/types.ts (1)
ReleaseTargetIdentifier(74-78)apps/event-worker/src/workers/upsert-resource/resource-db-upsert.ts (1)
dbUpsertResource(11-51)apps/event-worker/src/workers/upsert-resource/upsert-release-targets.ts (1)
upsertReleaseTargets(41-62)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (13)
apps/event-worker/src/workers/index.ts (2)
14-14: Clean import for the new workerThe import statement for the new
upsertResourceWorkerfollows the existing import pattern, maintaining consistency with the codebase.
30-30: Proper worker registration for the new channelThe
upsertResourceWorkeris correctly registered withChannel.UpsertResource, following the established pattern for worker registration in this file.packages/job-dispatch/src/resource/upsert.ts (1)
19-22: Appropriate encapsulation changeRemoving the
exportkeyword fromResourceToInserttype appropriately restricts it to the module scope. This is good practice for types that should only be used within the module.apps/event-worker/src/workers/upsert-resource/new-resource.ts (2)
9-12: Well-defined resource typeThe
ResourceToInserttype appropriately extends the baseInsertResourceschema with optional fields for metadata and variables, maintaining type safety throughout the resource creation process.
14-23: Well-structured handler function with clear workflowThe
handleNewResourcefunction follows a clean workflow:
- Inserts/updates the resource
- Upserts associated release targets
- Adds release targets to the evaluation queue with unique names
The approach handles the complete lifecycle of resource creation with proper asynchronous flow.
packages/events/src/types.ts (2)
25-25: Consistent channel enumerationThe new
UpsertResourceenum value follows the naming convention pattern of other channels in the enum, using kebab-case for the string value.
47-52: Complete channel map type definitionThe type definition for
Channel.UpsertResourcein theChannelMapis complete, including all necessary fields for resource processing with appropriate types. This ensures type safety throughout the event handling pipeline.apps/event-worker/src/workers/upsert-resource/worker.ts (1)
1-11: Imports and logger initialization look good.No obvious syntax or security issues. The logger child configuration is clear and consistent with typical usage patterns.
apps/event-worker/src/workers/upsert-resource/resource-db-upsert.ts (2)
1-9: Resource type structure is clear.Defining
ResourceToInsertto extend the baseInsertResourcewhile adding optionalmetadataandvariablesis a clean approach for capturing additional resource-related data.
32-41: Consider handling conflicts for metadata entries.While inserting metadata directly works for new entries, existing keys could cause unique constraints or duplication issues if the schema enforces them. If such conflicts are possible, you may consider an upsert or cleanup step for metadata.
apps/event-worker/src/workers/upsert-resource/existing-resource.ts (2)
18-53: FunctiongetSystemsForUnmatchedEnvsis well-structured.Fetching the missing environment IDs, then retrieving systems tied to those environments, is logical. Consider verifying that large environment lists won't cause performance bottlenecks.
54-83: Hooks dispatch logic appears robust.The function checks if the resource still matches the relevant environment filters before dispatching exit hooks, avoiding unnecessary calls. This approach helps keep the system in sync.
apps/event-worker/src/workers/upsert-resource/upsert-release-targets.ts (1)
1-4: Initial imports are standard.No concerns observed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/job-dispatch/src/resource/upsert.ts (1)
46-54: Queueing upserted resources for post-processing is sound.Adding resources to the queue in bulk for asynchronous processing is a clean approach. Tagging jobs by resource ID looks fine; ensure no collisions occur if IDs can be repeated across tenants.
apps/event-worker/src/workers/process-upserted-resource/process-upserted-resource.ts (1)
9-48: Verify object comparison inreleaseTargetsToDelete.
Line 18 usesnewReleaseTargets.includes(rt), which performs reference equality rather than matching by ID fields. Ifrtobjects differ by reference even though they share the same ID, they may not match. Consider comparing byrt.idinstead.Check for partial failures with
Promise.allSettled.
Promise.allSettledensures the function won’t throw on individual rejections, but rejections might be silently ignored. Consider logging or handling errors for better traceability.packages/job-dispatch/src/resource/db-upsert-resource.ts (1)
76-125: Confirm concurrency requirements for upsert.
dbUpsertResourceconcurrently upserts metadata and variables, which is generally efficient. Just confirm no race conditions if multiple upserts for the same resource happen in parallel.apps/event-worker/src/workers/process-upserted-resource/upsert-release-targets.ts (1)
8-39: Consider consolidating the nested loop to reduce query overhead.
The code currently makes a separate database query for each environment–deployment pair, potentially leading to high overhead when handling large numbers of environments or deployments. A possible improvement is to batch or consolidate these lookups into fewer queries if feasible, improving performance.apps/event-worker/src/workers/process-upserted-resource/dispatch-exit-hooks.ts (2)
45-73: Handle empty environment arrays gracefully.
When mappingenvironments.map(...)insideor(...), an empty array might yield an empty condition list. Consider adding a quick guard to skip the query if there are no environments. This ensures we avoid potential edge cases.
75-90: Consider error handling or logging for rejected event dispatches.
Promise.allSettledensures all dispatch calls proceed, but any rejections are currently swallowed. Adding logs or handling for rejections can help diagnose unexpected behavior in production.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
apps/event-worker/src/workers/index.ts(2 hunks)apps/event-worker/src/workers/process-upserted-resource/dispatch-exit-hooks.ts(1 hunks)apps/event-worker/src/workers/process-upserted-resource/process-upserted-resource.ts(1 hunks)apps/event-worker/src/workers/process-upserted-resource/upsert-release-targets.ts(1 hunks)apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts(2 hunks)packages/api/src/router/environment.ts(1 hunks)packages/api/src/router/resources.ts(2 hunks)packages/db/src/schema/resource.ts(1 hunks)packages/events/src/types.ts(2 hunks)packages/job-dispatch/src/index.ts(1 hunks)packages/job-dispatch/src/resource/db-upsert-resource.ts(1 hunks)packages/job-dispatch/src/resource/delete-resource.ts(1 hunks)packages/job-dispatch/src/resource/delete.ts(0 hunks)packages/job-dispatch/src/resource/dispatch-resource.ts(0 hunks)packages/job-dispatch/src/resource/find-existing-resources.ts(2 hunks)packages/job-dispatch/src/resource/index.ts(0 hunks)packages/job-dispatch/src/resource/insert-resource-metadata.ts(0 hunks)packages/job-dispatch/src/resource/insert-resource-variables.ts(0 hunks)packages/job-dispatch/src/resource/upsert.ts(2 hunks)packages/job-dispatch/src/resource/utils.ts(0 hunks)
💤 Files with no reviewable changes (6)
- packages/job-dispatch/src/resource/insert-resource-metadata.ts
- packages/job-dispatch/src/resource/index.ts
- packages/job-dispatch/src/resource/utils.ts
- packages/job-dispatch/src/resource/dispatch-resource.ts
- packages/job-dispatch/src/resource/delete.ts
- packages/job-dispatch/src/resource/insert-resource-variables.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/events/src/types.ts
- apps/event-worker/src/workers/index.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/schema/resource.tspackages/job-dispatch/src/index.tspackages/job-dispatch/src/resource/find-existing-resources.tspackages/api/src/router/resources.tspackages/api/src/router/environment.tsapps/event-worker/src/workers/process-upserted-resource/process-upserted-resource.tspackages/job-dispatch/src/resource/upsert.tspackages/job-dispatch/src/resource/delete-resource.tsapps/webservice/src/app/api/v1/resources/[resourceId]/route.tsapps/event-worker/src/workers/process-upserted-resource/upsert-release-targets.tspackages/job-dispatch/src/resource/db-upsert-resource.tsapps/event-worker/src/workers/process-upserted-resource/dispatch-exit-hooks.ts
🧬 Code Definitions (5)
apps/event-worker/src/workers/process-upserted-resource/process-upserted-resource.ts (3)
packages/events/src/index.ts (2)
createWorker(9-23)getQueue(26-32)apps/event-worker/src/workers/process-upserted-resource/upsert-release-targets.ts (1)
upsertReleaseTargets(41-62)apps/event-worker/src/workers/process-upserted-resource/dispatch-exit-hooks.ts (1)
dispatchExitHooks(75-90)
packages/job-dispatch/src/resource/upsert.ts (3)
packages/job-dispatch/src/resource/find-existing-resources.ts (1)
findExistingResources(52-70)packages/job-dispatch/src/resource/db-upsert-resource.ts (1)
dbUpsertResource(76-125)packages/job-dispatch/src/resource/delete-resource.ts (1)
deleteResource(52-72)
apps/event-worker/src/workers/process-upserted-resource/upsert-release-targets.ts (4)
packages/db/src/common.ts (1)
Tx(22-22)packages/rule-engine/src/types.ts (1)
ReleaseTargetIdentifier(74-78)packages/db/src/schema/resource.ts (1)
resource(59-87)packages/db/src/schema/workspace.ts (1)
workspace(18-27)
packages/job-dispatch/src/resource/db-upsert-resource.ts (3)
packages/job-dispatch/src/resource/upsert.ts (1)
ResourceToInsert(13-16)packages/db/src/schema/resource.ts (2)
InsertResource(115-115)resource(59-87)packages/db/src/common.ts (3)
Tx(22-22)buildConflictUpdateColumns(30-46)takeFirst(9-13)
apps/event-worker/src/workers/process-upserted-resource/dispatch-exit-hooks.ts (3)
packages/db/src/common.ts (1)
Tx(22-22)packages/rule-engine/src/types.ts (1)
ReleaseTargetIdentifier(74-78)packages/db/src/schema/resource.ts (2)
resource(59-87)Resource(105-105)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
🔇 Additional comments (29)
packages/db/src/schema/resource.ts (1)
174-174: Well-structured type export for resource metadata.The new
ResourceMetadatatype alias provides better type safety when working with resource metadata entities, which aligns well with TypeScript best practices.packages/job-dispatch/src/index.ts (1)
26-27: Good module restructuring for better code organization.Replacing the general resource module export with specific exports for delete and upsert operations provides more granular control and improves code organization. This change supports the new upsert resource worker functionality indicated in the PR objectives.
apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts (2)
7-7: Import updated to match the new resource management approach.The import statement has been correctly updated to use the more specific
deleteResourcefunction, aligning with the changes in the job-dispatch package.
123-123: Function call refactored to use resource ID directly.The deletion logic has been simplified to pass the resource ID directly to
deleteResourceinstead of passing an array of resources. This matches the new function signature and provides a more direct approach.packages/api/src/router/environment.ts (2)
5-5: Import statement streamlined for better readability.The updated import statement includes
upsertEnvwhich is relevant to the environment management functionality.
291-295: Event dispatch for environment updates preserved.The code correctly maintains the environment update event dispatch while removing the detailed resource selector processing logic. This aligns with moving that responsibility to the new dedicated upsert resource worker.
packages/api/src/router/resources.ts (2)
25-25: Good use of the newly introduceddeleteResourceimport.This import seems correct and aligns with the refactored single-resource deletion API.
698-705: Consider partial failure handling for batched deletions.If any resource deletion fails,
Promise.allwill reject immediately, potentially leaving other resources undeleted. This may be acceptable, but verify if partial-deletion scenarios should be handled differently or if an all-or-nothing approach is desired.packages/job-dispatch/src/resource/upsert.ts (6)
4-9: Imports for queuing and resource helpers look good.These imports correctly reflect the new architecture for upsert logic, event queuing, and resource existence checks.
32-41: Verify logic for identifying resources to delete.Filtering out existing resources not included in
resourcesToInserttriggers deletions. Confirm that excluding resources from this array is always the intended signal to remove them, especially if resources were renamed or repurposed.
42-45: Concurrent upsert is valid but double-check error handling.Using
Promise.allfor upserts is concise; however, if one upsert fails, the entire batch rejects. Verify that this collective failure is acceptable and consistent with the desired behavior.
55-58: Parallel deletions are okay but confirm race conditions.Deleting resources in parallel is efficient; ensure no follow-up writes rely on fully removed states. Also verify the desired outcome if one deletion fails mid-batch.
59-62: Synchronized promise resolution.The combined
Promise.allusage for adding upsert jobs and performing deletions is clear and consistent. Good concurrency handling.
64-64: Return consolidated result object.Returning both upserted and deleted resources is intuitive and helpful for downstream logic. No issues.
packages/job-dispatch/src/resource/delete-resource.ts (4)
1-10: New imports and status definitions are correct.The references to the DB schema, job status, and event handlers appear accurate for the new deletion workflow.
12-24: Resource relationship deletion logic looks consistent.Removing entries from
resourceRelationshipby matching both sides (from/to) ensures no stale links remain. This approach handles scenarios where the resource is a dependency or a dependent.
25-50: Job cancellation approach is logical.Fetching pending or in-progress jobs for the associated resource and marking them as cancelled prevents orphaned jobs. This aligns well with typical tidy-up patterns after resource removal.
52-72: End-to-end resource deletion with event handling is well-structured.The combined steps:
- Retrieve resource,
- Emit deletion events,
- Clean up relationships/jobs,
- Delete resource.
All happen in one flow. This implementation is sensible and thorough.
packages/job-dispatch/src/resource/find-existing-resources.ts (2)
5-5: No concerns for this import.
Imports are concise and consistent with usage in this file.
52-70: Consider error handling for Promise failures.
This function aggregates database queries viaPromise.all. If one query fails, the entire chain will reject. Verify that this behavior is desirable and ensure it won’t leave the system in a partial or inconsistent state. Consider adding logging or try/catch to handle failures more gracefully.apps/event-worker/src/workers/process-upserted-resource/process-upserted-resource.ts (1)
1-7: Imports look good.
All imported modules appear relevant and correctly used in subsequent code.packages/job-dispatch/src/resource/db-upsert-resource.ts (6)
1-2: No issues with the Tx import.
This matches the project’s transaction pattern.
3-11: Approved usage of conflict update helper.
buildConflictUpdateColumnsusage is aligned with your approach for handling unique constraints.
12-16: VariableInsert definition is straightforward.
No correctness issues identified.
18-21: ResourceToInsert extension is valid.
Enables optionalmetadataandvariables, which is consistent with the upsert logic.
23-49: Validate possible edge cases for empty metadata.
While the upsert logic looks sound, confirm that inserting/updating with an empty object or key collisions doesn’t lead to unexpected results.
51-74: Same caution for variable collisions.
Ensure that partially updated variables with the same key but different sensitivity or type are handled correctly.apps/event-worker/src/workers/process-upserted-resource/upsert-release-targets.ts (1)
41-62: No issues identified; approach seems solid.
Fetching the workspace and inserting release targets viaonConflictDoNothing()appears logically consistent. The usage ofthrow new Error("Workspace not found")provides clear failure feedback, and the final return matches the expected inserted records.apps/event-worker/src/workers/process-upserted-resource/dispatch-exit-hooks.ts (1)
9-43: Logic for identifying unmatched environments is coherent.
Building sets of environment IDs and filtering them out is a clear approach. The subsequent queries to retrieve systems and their respective environments and deployments appear correct.
Summary by CodeRabbit