fix: Add updated resource and inserted resource workers #461
Conversation
WalkthroughThis PR expands the event processing capabilities by adding new workers for handling the Changes
Sequence Diagram(s)sequenceDiagram
participant E as Event Source
participant NRW as newResourceWorker
participant URT as upsertReleaseTargets
participant Q as Queue (EvaluateReleaseTarget)
E->>NRW: Trigger NewResource event
NRW->>URT: Call upsertReleaseTargets(resource)
URT-->>NRW: Return releaseTargets
NRW->>Q: Enqueue releaseTargets for evaluation
sequenceDiagram
participant E as Event Source
participant URW as updatedResourceWorker
participant DB as Database
participant DE as dispatchExitHooks
participant Q as Queue (UpdatedResource)
E->>URW: Trigger UpdatedResource event
URW->>DB: Fetch current release targets
URW->>URT: Call upsertReleaseTargets(resource)
URT-->>URW: Return new releaseTargets
URW->>DE: Dispatch exit hooks with (current vs new targets)
URW->>Q: Enqueue new releaseTargets for further processing
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🪧 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: 1
🧹 Nitpick comments (5)
packages/job-dispatch/src/resource/insert-resources.ts (1)
89-96: Variable rename improves clarity.
Renaming fromdeletedtotoDeletehighlights the intent more clearly. Verify that any references to the olddeletedvariable have been updated.packages/job-dispatch/src/resource/upsert.ts (1)
31-36: Consider queue payload size for performance
All resource data is passed into the queue. Ifr.configorr.metadatacan become large, it may impact queue performance. Consider enqueueing a minimal payload (e.g., resource ID) and fetching the details in the worker.apps/event-worker/src/workers/process-upserted-resources/dispatch-exit-hooks.ts (2)
9-43: Query performance in getSystemsForUnmatchedEnvs
This function filters and then queries environments and systems to identify environments that are no longer targeted. Though logically sound, consider verifying database indices onenvironment.idandsystem.idfor large-scale usage to avoid potential slowdowns.
45-73: Handle partial failures in dispatchExitHooksIfExitedSystem
The use ofPromise.allSettledis safe to avoid early rejection. However, errors for each event dispatch are neither logged nor retried. Consider logging or otherwise handling unfulfilled promises to diagnose failures.apps/event-worker/src/workers/process-upserted-resources/upsert-release-targets.ts (1)
8-39: Efficient environment and deployment matching
The function correctly filters environments byresourceSelectorand merges environment + deployment selectors. Consider parallelizing queries in a single batch if the resource has many deployments, though the current approach is clear.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/event-worker/src/workers/index.ts(2 hunks)apps/event-worker/src/workers/process-upserted-resources/dispatch-exit-hooks.ts(1 hunks)apps/event-worker/src/workers/process-upserted-resources/index.ts(1 hunks)apps/event-worker/src/workers/process-upserted-resources/upsert-release-targets.ts(1 hunks)packages/events/src/types.ts(2 hunks)packages/job-dispatch/src/resource/dispatch-resource.ts(1 hunks)packages/job-dispatch/src/resource/insert-resources.ts(2 hunks)packages/job-dispatch/src/resource/upsert.ts(2 hunks)packages/job-dispatch/src/resource/utils.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/job-dispatch/src/resource/utils.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/events/src/types.tspackages/job-dispatch/src/resource/insert-resources.tsapps/event-worker/src/workers/process-upserted-resources/index.tsapps/event-worker/src/workers/index.tsapps/event-worker/src/workers/process-upserted-resources/dispatch-exit-hooks.tsapps/event-worker/src/workers/process-upserted-resources/upsert-release-targets.tspackages/job-dispatch/src/resource/dispatch-resource.tspackages/job-dispatch/src/resource/upsert.ts
🧬 Code Definitions (5)
apps/event-worker/src/workers/process-upserted-resources/index.ts (3)
packages/events/src/index.ts (2)
createWorker(9-23)getQueue(26-32)apps/event-worker/src/workers/process-upserted-resources/upsert-release-targets.ts (1)
upsertReleaseTargets(41-62)apps/event-worker/src/workers/process-upserted-resources/dispatch-exit-hooks.ts (1)
dispatchExitHooks(75-90)
apps/event-worker/src/workers/index.ts (1)
apps/event-worker/src/workers/process-upserted-resources/index.ts (1)
processUpsertedResourceWorker(9-48)
apps/event-worker/src/workers/process-upserted-resources/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 (1)
resource(59-87)
apps/event-worker/src/workers/process-upserted-resources/upsert-release-targets.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 (1)
resource(59-87)
packages/job-dispatch/src/resource/upsert.ts (3)
packages/job-dispatch/src/resource/insert-resources.ts (1)
insertResources(84-117)packages/events/src/index.ts (1)
getQueue(26-32)packages/job-dispatch/src/resource/delete.ts (1)
deleteResources(55-74)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (15)
packages/events/src/types.ts (2)
24-25: Nicely added enum value.
DefiningProcessUpsertedResourceis consistent with the existing naming convention in theChannelenum. Good job!
47-47: New channel mapping aligns well with the schema.
The mapping of[Channel.ProcessUpsertedResource]toschema.Resourceis appropriate and follows the pattern used for other channels. No issues here.apps/event-worker/src/workers/index.ts (2)
10-10: Import statement for the new worker looks good.
The import forprocessUpsertedResourceWorkeris consistent with the file structure.
30-30: Correctly registered the worker.
MappingChannel.ProcessUpsertedResourcetoprocessUpsertedResourceWorkeris clear and coherent with the existing pattern.packages/job-dispatch/src/resource/insert-resources.ts (1)
116-116: Return value updated successfully.
Returning{ all: insertedResources, toDelete }is consistent with the renamed variable. Nothing else to address here.apps/event-worker/src/workers/process-upserted-resources/index.ts (1)
9-48: General implementation feedback.
Creating a worker that upserts release targets, deletes outdated records, dispatches exit hooks, and queues new targets is logical and consistent with the rest of the codebase. UsingPromise.allSettledis a good approach to ensure both operations complete independently. Overall, this file is well-structured and follows standard best practices.packages/job-dispatch/src/resource/upsert.ts (3)
4-4: Leverage new import for streamlined queue-based processing
The import ofChannelandgetQueueenables sending batches of resources to be processed asynchronously elsewhere. This approach reduces the complexity of the upsert flow while delegating follow-up actions to the event worker.
38-39: Ensure consistent resource cleanup before queuing
deleteResourcesis invoked after adding items to the queue. If both operations target the same resources, verify that the queue logic won't be negatively impacted by resources that might be concurrently deleted.
29-29:❓ Verification inconclusive
Validate the return structure from insertResources
The destructuring of{ all, toDelete }relies oninsertResourcesproviding these keys. Double-check thatinsertResourcesconsistently returns them to avoid runtime errors.
🏁 Script executed:
#!/bin/bash # Verify that insertResources always returns 'all' and 'toDelete'" rg -A 5 'export const insertResources'Length of output: 590
Action: Verify the return structure of
insertResourcesThe calling code in
packages/job-dispatch/src/resource/upsert.tsrelies on destructuring the result into{ all, toDelete }. However, the context retrieved frompackages/job-dispatch/src/resource/insert-resources.tsonly shows the early part of the function (setting uptoDelete) without a visible return that confirms both keys are included.
- File to review:
packages/job-dispatch/src/resource/insert-resources.ts- Focus: Ensure that the final return statement explicitly returns an object containing both
allandtoDelete(e.g.,return { all, toDelete }), and that theallproperty is properly computed.- Rationale: This validation prevents runtime errors due to missing properties when destructuring the result in
upsert.ts.Please double-check the full implementation of
insertResourcesto confirm the consistency of its return structure.apps/event-worker/src/workers/process-upserted-resources/dispatch-exit-hooks.ts (2)
1-8: Imports and initial setup look correct
These imports appear consistent with the rest of the codebase. Good practice using minimal dependencies for clarity.
75-90: Compositional design in dispatchExitHooks
This function neatly chains retrieving unmatched systems and then dispatching exit hooks. The design is readable and well-structured, ensuring minimal duplication.apps/event-worker/src/workers/process-upserted-resources/upsert-release-targets.ts (3)
1-4: Use of tx-is-present library
Thets-is-presentlibrary helps filter out nullish values from arrays. This is a clean approach to skip invalid entries in a functional style.
5-7: Importing minimal DB helpers
Imports ofandandeqkeep the query logic succinct. No concerns here.
41-63: Workspace retrieval and conflict handling
Fetching the workspace, then inserting release targets withonConflictDoNothing()ensures no duplicates. This is robust for idempotent upserts. The overall logic is coherent and well-structured.packages/job-dispatch/src/resource/dispatch-resource.ts (1)
4-4: Import usage appears consistent and correct.The newly imported functions (
and,desc,eq,takeFirstOrNull) are used appropriately for the environment/versions queries below. No issues detected with this change.
apps/event-worker/src/workers/process-upserted-resources/index.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/event-worker/src/workers/process-upserted-resources/index.ts (1)
19-20: Duplicate concern: object reference comparison.
UsingnewReleaseTargets.includes(rt)can yield false negatives if objects differ by reference. Compare by a unique field (e.g.,id) instead.
🧹 Nitpick comments (3)
packages/rule-engine/src/repositories/db-release-repository.ts (1)
226-239: Consider transaction handling or concurrent load.
The newupsertReleaseForAllVersionsmethod repeatedly callsupsertReleasein a loop. For large deployments or high concurrency, consider batching or using a single transaction for performance and consistency.apps/event-worker/src/workers/process-upserted-resources/index.ts (2)
28-29: Check concurrency impact.
CallingupsertReleaseForAllVersionsfor each target in parallel could be expensive if you have manynewReleaseTargets. Consider limiting concurrency or batching where practical.
55-59: Handle partial failures in settled promises.
Promise.allSettledeffectively ignores individual failures. Ensure error states are logged or retried if partial success is not acceptable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/event-worker/src/workers/process-upserted-resources/index.ts(1 hunks)packages/db/src/schema/release.ts(1 hunks)packages/job-dispatch/src/job-dispatch.ts(1 hunks)packages/job-dispatch/src/resource/upsert.ts(2 hunks)packages/rule-engine/src/repositories/db-release-repository.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/job-dispatch/src/job-dispatch.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/job-dispatch/src/resource/upsert.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/release.tspackages/rule-engine/src/repositories/db-release-repository.tsapps/event-worker/src/workers/process-upserted-resources/index.ts
🔇 Additional comments (2)
packages/db/src/schema/release.ts (1)
42-43: Exported type alias looks good.
This addition provides a clear, strongly typed reference to thereleaseTargettable.packages/rule-engine/src/repositories/db-release-repository.ts (1)
4-4: Imports align with usage.
The inclusion ofdescandtakeFirstis consistent with the newly added query logic and return handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/event-worker/src/workers/process-upserted-resources/upsert-release-targets.ts (2)
16-36: Consider reducing multiple concurrent queries to improve performance.For each environment-deployment pair, the code executes a separate query to match the resource. If there are many pairs, this may create a high volume of database queries. As an optional improvement, consider collecting relevant resourceSelectors first and using a single combined query (or chunk-based queries) to reduce overhead.
41-49: Include identifying information in error message.Currently, if
workspaceis not found, it throws"Workspace not found". Including the missing workspace ID or the resource ID can help with debugging and log correlation.You could do something like:
- if (workspace == null) throw new Error("Workspace not found"); + if (workspace == null) { + throw new Error(`Workspace not found for resource ${resource.id} (workspace ID: ${resource.workspaceId})`); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/event-worker/src/workers/process-upserted-resources/upsert-release-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/process-upserted-resources/upsert-release-targets.ts
🧬 Code Graph Analysis (1)
apps/event-worker/src/workers/process-upserted-resources/upsert-release-targets.ts (5)
packages/db/src/common.ts (1)
Tx(22-22)packages/db/src/schema/system.ts (1)
System(49-49)packages/rule-engine/src/types.ts (1)
ReleaseTargetIdentifier(59-63)packages/db/src/schema/resource.ts (1)
resource(59-87)packages/db/src/schema/workspace.ts (1)
workspace(18-27)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (2)
apps/event-worker/src/workers/process-upserted-resources/upsert-release-targets.ts (2)
1-7: All imports look appropriate and necessary.
They correctly reference types from the shared libraries and neatly organize dependencies without any obvious redundancies.
58-63:❓ Verification inconclusive
Verify that the unique conflict constraint is properly defined.
.onConflictDoNothing()ensures no duplicates are inserted, but it relies on the underlying table’s default unique constraints. If you intend to handle conflicts on specific fields, consider specifying them explicitly or confirming the necessary unique index on(resourceId, environmentId, deploymentId)in your schema.Run the following script to check whether the
releaseTargettable has an appropriate unique index:
🏁 Script executed:
#!/bin/bash # Searching for CREATE UNIQUE INDEX definitions on releaseTarget rg -A 5 "CREATE\s+UNIQUE\s+INDEX" | grep "releaseTarget"Length of output: 56
Action Required: Confirm Unique Constraint on
releaseTargetSchemaThe automated search did not return any unique index definition for the
releaseTargettable. To ensure that the.onConflictDoNothing()call behaves as intended, please verify manually (or update the schema) that a unique constraint is defined on the(resourceId, environmentId, deploymentId)fields. For better confidence, consider checking the migration or schema definition files for either a unique index or a table-level unique constraint covering these columns.
- Verify that the unique constraint/index on
(resourceId, environmentId, deploymentId)is present in the SQL migration or schema file.- If absent, explicitly define the constraint to avoid duplicate records when inserting with
.onConflictDoNothing().
adityachoudhari26
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
apps/webservice/src/app/api/v1/resource-providers/[providerId]/set/route.ts (1)
80-89: 💡 Verification agent🧩 Analysis chain
Function call updated to use new resource handling approach.
The implementation has been updated to use
handleResourceProviderScaninstead ofupsertResourceswhile maintaining the same parameter structure and expected response format.Let's verify that the new function has similar behavior to the old one:
🏁 Script executed:
#!/bin/bash # Search for all usages of the new function to confirm it's consistently adopted rg -A 2 "handleResourceProviderScan" # Check for any remaining usages of the old function rg -A 2 "upsertResources"Length of output: 3990
Action Required: Update Function Import and Call for Resource Handling
The intended update was to replace the usage of
upsertResourceswithhandleResourceProviderScanin this file. However, our verification shows that the file still imports and callsupsertResourcesrather than the new function. Please update the following in the file:
- Import Update: Change the import in
apps/webservice/src/app/api/v1/resource-providers/[providerId]/set/route.tsfrom:To the new function, for example:import { upsertResources } from "@ctrlplane/job-dispatch";import { handleResourceProviderScan } from "@ctrlplane/job-dispatch";- Function Call Update: Change the call from:
To:const resources = await upsertResources( db, resourcesToInsert.map((r) => ({ ...r, variables: r.variables?.map((v) => ({ ...v, value: v.value ?? null, })), })), );const resources = await handleResourceProviderScan( db, resourcesToInsert.map((r) => ({ ...r, variables: r.variables?.map((v) => ({ ...v, value: v.value ?? null, })), })), );Once these changes are made, please verify that the new function’s behavior matches the expected response format.
🧹 Nitpick comments (11)
packages/db/package.json (1)
42-42: Dependency Addition for Secrets Management:
The newly added dependency@ctrlplane/secretswith the version set to"workspace:*"appears to align with the project’s shift towards enhanced secrets management in the database context. Please ensure that:
- The package is correctly configured within your workspace setup.
- Its integration adheres to your security best practices.
- Any required initialization or configuration for managing secrets is documented or implemented elsewhere in the codebase.
apps/event-worker/src/workers/new-resource.ts (1)
1-18: LGTM! Well-structured worker for handling new resources.The new resource worker implementation correctly:
- Imports necessary dependencies from DB client and events package
- Sets up a queue for the
EvaluateReleaseTargetchannel- Creates a worker listening on the
NewResourcechannel- Processes incoming resources by upserting release targets
- Adds the resulting release targets to the queue with unique names
Consider adding explicit error handling to catch potential failures during the promise chain:
export const newResourceWorker = createWorker( Channel.NewResource, ({ data: resource }) => upsertReleaseTargets(db, resource).then(async (rts) => { await queue.addBulk( rts.map((rt) => ({ name: `${rt.resourceId}-${rt.environmentId}-${rt.deploymentId}`, data: rt, })), ); + }).catch(error => { + // Log error but don't rethrow to prevent worker from failing + console.error("Error processing new resource", { error, resourceId: resource.id }); }), );apps/event-worker/src/workers/upserted-resources/index.ts (1)
9-49: Consider partial failure handling.The worker relies on
Promise.allSettledand proceeds even if one of the operations (dispatching exit hooks or adding to the evaluation queue) fails. This might be acceptable if you want to ensure other tasks proceed regardless of individual failures. However, if partial success could cause inconsistencies (e.g., some release targets remain unprocessed or exit hooks not dispatched), consider additional guardrails or logging to examine individual rejections.apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts (1)
97-97: Validate merged changes.When calling
upsertResourceswith[_.merge(resource, body)], ensure that the_.mergebehavior correctly merges nested fields (e.g., arrays) as intended. Confirm that partial array updates don’t inadvertently overwrite data or produce unexpected results.apps/webservice/src/app/api/v1/resources/route.ts (2)
67-72: Bulk queueing for upserted resources.Enqueuing resources in bulk can improve performance. However, watch for any potential operational overhead or concurrency issues, particularly if these bulk additions happen frequently. Ensure the queue has capacity and error-handling to handle bursts of resource events.
74-74: Return meaningful response.Returning the count of created or updated resources is sufficient for a simple scenario. If clients need more granular status or require follow-up actions based on the queue's processing, consider returning additional metadata (e.g., a list of queued IDs).
packages/job-dispatch/src/resource/handle-provider-scan.ts (1)
11-11: Consider renaming the logger label for consistency.
Currently, the logger label is"upsert-resources", whereas the function name ishandleResourceProviderScan. Aligning them can reduce confusion and improve traceability.-const log = logger.child({ label: "upsert-resources" }); +const log = logger.child({ label: "handle-provider-scan" });packages/job-dispatch/src/resource/group-resources-by-hook.ts (1)
97-109: Optimize membership checks for large inputs.
The repeated use of.some(...)calls can lead to O(n²) performance when theexistingResourcesandresourcesToInsertarrays grow large. Consider using a map or set for O(1) membership checks:-const newResources = resourcesToInsert.filter( - (r) => - !existingResources.some( - (er) => - er.identifier === r.identifier && er.workspaceId === r.workspaceId, - ), -); -const toUpsert = resourcesToInsert.filter((r) => - existingResources.some( - (er) => - er.identifier === r.identifier && er.workspaceId === r.workspaceId, - ), -); +const existingKeySet = new Set( + existingResources.map((er) => `${er.workspaceId}:${er.identifier}`) +); + +const newResources = resourcesToInsert.filter((r) => { + const key = `${r.workspaceId}:${r.identifier}`; + return !existingKeySet.has(key); +}); + +const toUpsert = resourcesToInsert.filter((r) => { + const key = `${r.workspaceId}:${r.identifier}`; + return existingKeySet.has(key); +});apps/event-worker/src/workers/upserted-resources/dispatch-exit-hooks.ts (1)
71-72: Assess partial failures on event handling.
UsingPromise.allSettledis safe for concurrency, but partial failures must be handled carefully. If specific hook dispatches fail, consider retry or fallback logic to ensure resources are consistently removed from all target deployments.apps/event-worker/src/utils/upsert-release-targets.ts (2)
8-39: Consider handling potential duplicate outcomes and optimizing performance.Currently, the same resource could match multiple environment/deployment combinations if the
resourceSelectoroverlaps, leading to potentially redundant target inserts. AlthoughupsertReleaseTargets()usesonConflictDoNothing()to ignore duplicates at the DB layer, you might want to explicitly handle or deduplicate them here if you need more accurate post-insert data. Additionally, callingdb.query.resource.findFirst()for each environment-deployment pair can be costly for large sets. A single batched query or partitioning the environment/deployment logic could improve performance.
19-33: Evaluate async iteration pattern in a loop for large environment and deployment arrays.While the nested
.map(async ...)followed byawait Promise.all(...)is correct for concurrency, you may need to confirm it won't cause performance overhead ifsystem.environmentsandsystem.deploymentsare large. If performance issues arise, consider grouping environment/deployment checks in fewer queries or employing streaming/batching strategies.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (28)
apps/event-worker/src/utils/upsert-release-targets.ts(1 hunks)apps/event-worker/src/workers/index.ts(2 hunks)apps/event-worker/src/workers/new-resource.ts(1 hunks)apps/event-worker/src/workers/resource-scan/aws/eks.ts(1 hunks)apps/event-worker/src/workers/resource-scan/aws/vpc.ts(1 hunks)apps/event-worker/src/workers/resource-scan/azure/cluster-to-resource.ts(1 hunks)apps/event-worker/src/workers/resource-scan/google/cluster-to-resource.ts(1 hunks)apps/event-worker/src/workers/resource-scan/google/vm.ts(1 hunks)apps/event-worker/src/workers/resource-scan/google/vpc.ts(1 hunks)apps/event-worker/src/workers/resource-scan/index.ts(2 hunks)apps/event-worker/src/workers/upserted-resources/dispatch-exit-hooks.ts(1 hunks)apps/event-worker/src/workers/upserted-resources/index.ts(1 hunks)apps/pty-proxy/package.json(1 hunks)apps/pty-proxy/src/controller/agent-socket.ts(3 hunks)apps/webservice/src/app/api/v1/resource-providers/[providerId]/set/route.ts(2 hunks)apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts(2 hunks)apps/webservice/src/app/api/v1/resources/route.ts(2 hunks)packages/db/package.json(1 hunks)packages/db/src/index.ts(1 hunks)packages/db/src/schema/resource.ts(1 hunks)packages/db/src/upsert-resources.ts(1 hunks)packages/events/src/types.ts(2 hunks)packages/job-dispatch/package.json(0 hunks)packages/job-dispatch/src/resource/group-resources-by-hook.ts(2 hunks)packages/job-dispatch/src/resource/handle-provider-scan.ts(1 hunks)packages/job-dispatch/src/resource/index.ts(1 hunks)packages/job-dispatch/src/resource/insert-resource-metadata.ts(0 hunks)packages/job-dispatch/src/resource/insert-resource-variables.ts(0 hunks)
💤 Files with no reviewable changes (3)
- packages/job-dispatch/src/resource/insert-resource-variables.ts
- packages/job-dispatch/src/resource/insert-resource-metadata.ts
- packages/job-dispatch/package.json
✅ Files skipped from review due to trivial changes (6)
- apps/event-worker/src/workers/resource-scan/google/cluster-to-resource.ts
- apps/event-worker/src/workers/resource-scan/azure/cluster-to-resource.ts
- apps/event-worker/src/workers/resource-scan/google/vpc.ts
- apps/event-worker/src/workers/resource-scan/aws/vpc.ts
- apps/event-worker/src/workers/resource-scan/aws/eks.ts
- apps/event-worker/src/workers/resource-scan/google/vm.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/resource/index.tspackages/db/src/index.tspackages/events/src/types.tsapps/webservice/src/app/api/v1/resource-providers/[providerId]/set/route.tsapps/event-worker/src/workers/resource-scan/index.tspackages/job-dispatch/src/resource/group-resources-by-hook.tsapps/event-worker/src/workers/new-resource.tsapps/pty-proxy/src/controller/agent-socket.tsapps/event-worker/src/workers/upserted-resources/index.tsapps/event-worker/src/workers/upserted-resources/dispatch-exit-hooks.tsapps/webservice/src/app/api/v1/resources/route.tspackages/job-dispatch/src/resource/handle-provider-scan.tsapps/event-worker/src/workers/index.tsapps/webservice/src/app/api/v1/resources/[resourceId]/route.tspackages/db/src/upsert-resources.tsapps/event-worker/src/utils/upsert-release-targets.ts
🧬 Code Graph Analysis (1)
apps/webservice/src/app/api/v1/resource-providers/[providerId]/set/route.ts (1)
packages/job-dispatch/src/resource/upsert.ts (1)
upsertResources(18-60)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (25)
apps/pty-proxy/src/controller/agent-socket.ts (3)
12-12: Imports look consistent with the new resource management approach.
No issues found with importingupsertResourcesfrom@ctrlplane/db.
15-15: New event-based imports are aligned with the revised architecture.
The import ofChannelandgetQueuefrom@ctrlplane/eventsis consistent with an event-driven approach.
141-141: Verify that queued resource data is free of sensitive fields.
Storing an entire resource object could inadvertently expose secrets or sensitive configs in the queue. Please ensure such fields are redacted or encrypted before queuing.packages/db/src/upsert-resources.ts (4)
1-13: No concerns with import choices and type definitions.
Usage of “lodash” and “drizzle-orm” is clear. The type definition “ResourceWithMetadata” is well-structured for optional metadata.
14-49: Metadata update logic appears correct.
The approach to remove old keys and upsert new entries inresourceMetadatais straightforward and maintains consistency.
55-118: Variable encryption and conflict handling are well-managed.
Your use ofvariablesAES256()for sensitive data and the diff-based approach for created/updated/deleted variables is effective.
120-155: Upsert pattern and conflict resolution are appropriately handled.
The function combines resource upsert with metadata/variables updates in one place, simplifying resource management. Good use ofonConflictDoUpdate.apps/pty-proxy/package.json (1)
16-16: Dependency on @ctrlplane/events is consistent with the PR’s event-driven changes.
No issues found.packages/db/src/index.ts (1)
10-10: Looks good - Clean export addition for upsert resources functionality.The export follows the established pattern in this file and makes the upsert-resources functionality accessible to consumers of the db package.
packages/job-dispatch/src/resource/index.ts (1)
1-1: Export correctly updated for refactored functionality.The change properly updates the export to reference the new location of the functionality that was refactored from
upsert.jstohandle-provider-scan.js.packages/db/src/schema/resource.ts (1)
116-119: Well-structured type definition for resource upsertion.The new
ResourceToUpserttype correctly extendsInsertResourceand adds properly typed optional fields for metadata and variables. This provides good type safety for the upsert operations.packages/events/src/types.ts (2)
25-26: Good addition of new channel types for resource events.The new enum values follow the established naming convention and string value pattern.
48-49: Channel map properly updated for new event types.Both new entries in the ChannelMap are correctly typed as
schema.Resource, maintaining consistency with how other resource-related events are handled.apps/webservice/src/app/api/v1/resource-providers/[providerId]/set/route.ts (1)
12-12: Import updated to use new handler function.The import has been updated from
upsertResourcestohandleResourceProviderScan, aligning with the function change in line 80.apps/event-worker/src/workers/resource-scan/index.ts (2)
13-13: Import updated to use new handler function.The import has been updated from
upsertResourcestohandleResourceProviderScan, consistent with changes in other files.
99-99: Function call updated to use new resource handling approach.The implementation has been updated to use
handleResourceProviderScaninstead ofupsertResources, consistent with the import change and other files.apps/event-worker/src/workers/index.ts (3)
10-10: New resource worker imported.The import for the new resource worker matches the implementation we reviewed in the first file.
15-15: Upserted resource worker imported.A new
upsertedResourceWorkeris being imported, which will presumably handle resources that have been upserted.
31-32: New workers registered with their respective channels.Both new workers are properly registered in the workers object with their corresponding channel types:
UpsertedResourcechannel is associated withupsertedResourceWorkerNewResourcechannel is associated withnewResourceWorkerThis completes the integration of the new workers into the event processing system.
apps/event-worker/src/workers/upserted-resources/index.ts (1)
1-8: Good use of modular imports.These imports correctly pull in database utilities, event channels, and utility functions for release targets and exit hooks. No immediate issues with versioning or references appear, and this structure is concise.
apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts (2)
5-8: Consolidate or confirm usage of imports.Imports now mix
upsertResourcesfrom@ctrlplane/dbwithdeleteResourcesfrom@ctrlplane/job-dispatch. Confirm that the partial shift to@ctrlplane/dbfor upsertion was intentional, and thatdeleteResourcesis still needed from@ctrlplane/job-dispatch.
100-100: Queue updated resource for downstream processing.Queuing the updated resource in the
UpsertedResourcechannel is a good approach for event-driven processing. Validate that no additional checks are needed before enqueuing (e.g., verifying resource state or constraints) to avoid spurious queue entries.apps/webservice/src/app/api/v1/resources/route.ts (1)
5-8: Transition to@ctrlplane/dbfor upsert.Switching the upsert logic to
@ctrlplane/dbsuggests a deliberate unification of resource operations under a single module. Confirm that any prior logic in@ctrlplane/job-dispatchis fully deprecated or has been migrated to avoid duplication or drift in functionality.apps/event-worker/src/workers/upserted-resources/dispatch-exit-hooks.ts (1)
61-65: Clarify the exit condition logic.
WhenmatchedResource == null, the function returns early, indicating that the resource doesn't match any of the environments left in the system. Ensure this matches the intended logic for removing resources from deployments that no longer include them. If the resource still matches an environment in the system, you dispatch removal events. This could be counterintuitive; verify that logic aligns with your functional requirements.apps/event-worker/src/utils/upsert-release-targets.ts (1)
41-63: Verify that 'onConflictDoNothing()' and the returned insert rows meet your needs.The use of
.onConflictDoNothing()will ignore collisions rather than updating existing rows, and you’ll only get newly inserted rows in the return value. Ensure this behavior is intentional, especially if you want to track or handle existing release targets. Otherwise, consider using.onConflictDoUpdate()or retrieving the pre-existing rows if needed for further processing.
| >, | ||
| ) { | ||
| const { all } = await upsertResources(db, [ | ||
| const all = await upsertResources(db, [ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider wrapping upsert in a transaction if partial updates are a concern.
Currently, the call uses db directly. If a failure happens later in the upsert flow, partial updates may remain in the database. You may want to ensure an all-or-nothing update by using a transaction, if atomicity is needed.
| try { | ||
| const workspaceId = resourcesToInsert[0]?.workspaceId; | ||
| if (workspaceId == null) throw new Error("Workspace ID is required"); | ||
| if (!resourcesToInsert.every((r) => r.workspaceId === workspaceId)) | ||
| throw new Error("All resources must belong to the same workspace"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle the case of an empty resource list more gracefully.
If resourcesToInsert is an empty array, resourcesToInsert[0] will be undefined, triggering an error for a missing workspace. A more user-friendly approach would be to return early if there are no resources to upsert.
if (resourcesToInsert.length === 0) {
log.info("No resources to upsert");
return { all: [], deleted: [] };
}
const workspaceId = resourcesToInsert[0]?.workspaceId;
if (workspaceId == null) throw new Error("Workspace ID is required");📝 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 { | |
| const workspaceId = resourcesToInsert[0]?.workspaceId; | |
| if (workspaceId == null) throw new Error("Workspace ID is required"); | |
| if (!resourcesToInsert.every((r) => r.workspaceId === workspaceId)) | |
| throw new Error("All resources must belong to the same workspace"); | |
| try { | |
| if (resourcesToInsert.length === 0) { | |
| log.info("No resources to upsert"); | |
| return { all: [], deleted: [] }; | |
| } | |
| const workspaceId = resourcesToInsert[0]?.workspaceId; | |
| if (workspaceId == null) throw new Error("Workspace ID is required"); | |
| if (!resourcesToInsert.every((r) => r.workspaceId === workspaceId)) | |
| throw new Error("All resources must belong to the same workspace"); |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/job-dispatch/src/resource/handle-provider-scan.ts (1)
25-30: Avoid errors by returning early whenresourcesToInsertis empty.If
resourcesToInsertis an empty array, accessingresourcesToInsert[0]can produce an undefined workspace ID and trigger an error. A previous reviewer suggested returning early in this scenario to provide a more user-friendly outcome.
🧹 Nitpick comments (3)
packages/job-dispatch/src/resource/handle-provider-scan.ts (2)
13-16: Consider using a more explicit type instead ofany.Currently, the
valuefield invariablesis typed asany, which may obscure type checks and lead to runtime errors. If possible, specify a stricter type or union type to reflect the anticipated data shape.
21-24: Ensure that logged identifiers are safe to expose.Be mindful that
resourcesToInsert.map((r) => r.identifier)may contain user-generated strings that could inadvertently leak sensitive information. If there's any chance these identifiers could be PII, consider masking or removing them from logs.packages/db/package.json (1)
47-47: Consider partial imports to reduce bundle size.If you're only using a subset of Lodash functions, import them individually (e.g.,
lodash/sum) or use a tree-shaking build to minimize bundle size.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
packages/db/package.json(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/job-dispatch/src/resource/handle-provider-scan.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (1)
packages/db/package.json (1)
42-42: No issues with local workspace reference to@ctrlplane/secrets.The workspace-based version for
@ctrlplane/secretslooks acceptable. Just confirm that consistent versions exist across the monorepo for seamless integration.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/job-dispatch/src/resource/handle-provider-scan.ts (1)
25-29: Gracefully handle empty resource lists.
IfresourcesToInsertis empty,resourcesToInsert[0]will beundefined, and the error message "Workspace ID is required" may be confusing. A user-friendly approach would be to return early or provide a more descriptive error.Apply a similar fix as previously suggested:
+ if (resourcesToInsert.length === 0) { + log.info("No resources to upsert"); + return { all: [], deleted: [] }; + } const workspaceId = resourcesToInsert[0]?.workspaceId; if (workspaceId == null) throw new Error("Workspace ID is required");
🧹 Nitpick comments (7)
apps/event-worker/src/workers/updated-resources/index.ts (2)
21-26: Verify orphaned references.Deleting release targets without a cascading strategy might lead to orphaned references if any other tables rely on these targets.
44-49: Partial failure handling.
Promise.allSettledhelps avoid unhandled rejections, but partial failures might go unnoticed. If certain operations fail, consider logging or compensating actions to maintain consistency.- await Promise.allSettled([ + const results = await Promise.allSettled([ dispatchExitHooksPromise, addToEvaluateQueuePromise, ]); + results.forEach(({ status, reason }) => { + if (status === "rejected") { + // Log or handle partial failures + } + });packages/job-dispatch/src/resource/handle-provider-scan.ts (2)
13-16: Consider documenting the optional metadata and variables fields more thoroughly.
While this type definition is straightforward, it may help future maintainers if you document the expected structure and usage ofmetadataandvariablesmore explicitly, including any constraints on keys or sensitivity handling.
35-46: Ensure scalable queue usage for large resource batches.
When callingaddBulkwith many items, there could be performance or memory overhead. If you anticipate large batches, consider chunking or rate-limiting queue operations.apps/event-worker/src/workers/updated-resources/dispatch-exit-hooks.ts (2)
9-22: Guard against large sets of unmatched environments.
If there are many unmatched environment IDs, consider adding pagination or limiting logic to this query to mitigate performance overhead.
66-72: Consider customizing event failure handling.
UsingPromise.allSettledis good for parallel event handling, but any failures will be buried unless you iterate over the results and check forrejectedstates. Optionally, log or retry these failures.packages/job-dispatch/src/resource/group-resources-by-hook.ts (1)
72-83: Clarify naming in the doc comments.
The doc references “new,” “upsert,” and “delete,” but the return properties aretoInsert,toUpdate, andtoDelete. Ensuring consistent naming in the doc block helps avoid confusion.- * - new: Resources that don't exist in the database and need to be created - * - upsert: Resources that exist and need to be updated + * - toInsert: Resources that don't exist in the database and need to be created + * - toUpdate: Resources that exist and need to be updated * - toDelete: Existing resources that are no longer present in the input and should be deleted
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/event-worker/src/workers/index.ts(2 hunks)apps/event-worker/src/workers/updated-resources/dispatch-exit-hooks.ts(1 hunks)apps/event-worker/src/workers/updated-resources/index.ts(1 hunks)apps/pty-proxy/src/controller/agent-socket.ts(3 hunks)apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts(2 hunks)apps/webservice/src/app/api/v1/resources/route.ts(2 hunks)packages/events/src/types.ts(2 hunks)packages/job-dispatch/src/resource/group-resources-by-hook.ts(2 hunks)packages/job-dispatch/src/resource/handle-provider-scan.ts(1 hunks)packages/job-dispatch/src/resource/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/events/src/types.ts
- apps/pty-proxy/src/controller/agent-socket.ts
- apps/webservice/src/app/api/v1/resources/[resourceId]/route.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/event-worker/src/workers/index.tspackages/job-dispatch/src/resource/handle-provider-scan.tsapps/event-worker/src/workers/updated-resources/dispatch-exit-hooks.tspackages/job-dispatch/src/resource/index.tsapps/webservice/src/app/api/v1/resources/route.tspackages/job-dispatch/src/resource/group-resources-by-hook.tsapps/event-worker/src/workers/updated-resources/index.ts
🧬 Code Graph Analysis (6)
apps/event-worker/src/workers/index.ts (2)
apps/event-worker/src/workers/updated-resources/index.ts (1)
updatedResourceWorker(9-49)apps/event-worker/src/workers/new-resource.ts (1)
newResourceWorker(7-18)
packages/job-dispatch/src/resource/handle-provider-scan.ts (6)
packages/db/src/schema/resource.ts (1)
InsertResource(115-115)packages/db/src/common.ts (1)
Tx(22-22)packages/job-dispatch/src/resource/group-resources-by-hook.ts (1)
groupResourcesByHook(84-112)packages/db/src/upsert-resources.ts (1)
upsertResources(120-155)packages/events/src/index.ts (1)
getQueue(26-32)packages/job-dispatch/src/resource/delete.ts (1)
deleteResources(55-74)
apps/event-worker/src/workers/updated-resources/dispatch-exit-hooks.ts (4)
packages/db/src/common.ts (1)
Tx(22-22)packages/rule-engine/src/types.ts (1)
ReleaseTargetIdentifier(59-63)packages/db/src/schema/resource.ts (1)
resource(59-87)packages/job-dispatch/src/events/index.ts (1)
handleEvent(8-9)
apps/webservice/src/app/api/v1/resources/route.ts (4)
packages/job-dispatch/src/resource/group-resources-by-hook.ts (1)
groupResourcesByHook(84-112)packages/db/src/client.ts (1)
db(13-13)packages/db/src/upsert-resources.ts (1)
upsertResources(120-155)packages/events/src/index.ts (1)
getQueue(26-32)
packages/job-dispatch/src/resource/group-resources-by-hook.ts (2)
packages/db/src/common.ts (1)
Tx(22-22)packages/db/src/schema/resource.ts (1)
InsertResource(115-115)
apps/event-worker/src/workers/updated-resources/index.ts (4)
packages/events/src/index.ts (2)
createWorker(9-23)getQueue(26-32)packages/db/src/client.ts (1)
db(13-13)apps/event-worker/src/utils/upsert-release-targets.ts (1)
upsertReleaseTargets(41-63)apps/event-worker/src/workers/updated-resources/dispatch-exit-hooks.ts (1)
dispatchExitHooks(75-90)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (21)
packages/job-dispatch/src/resource/index.ts (2)
1-1: No concerns with the new export.Exporting from
handle-provider-scan.jsappears consistent with the broader refactoring strategy.
4-4: Export addition looks good.Exporting
group-resources-by-hook.jshere centralizes resource-related utilities. Ensure consumers of this module are updated accordingly, if necessary.apps/event-worker/src/workers/index.ts (3)
10-10: Import added for newResourceWorker.The import path for
newResourceWorkerlooks correct. Confirm that the associated worker code is covered by tests.
15-15: Import added for updatedResourceWorker.The import path for
updatedResourceWorkeris straightforward. No issues identified.
31-32: New channel mappings for resource workers.Registering both
Channel.UpdatedResourceandChannel.NewResourcein the workers object aligns with the PR objectives to handle resource events. Everything looks properly wired.apps/event-worker/src/workers/updated-resources/index.ts (6)
1-4: Imports look correct.All required functions and schemas are correctly imported from
@ctrlplane/dband@ctrlplane/events.
6-7: Utility imports are well-placed.
upsertReleaseTargetsanddispatchExitHooksare brought in as expected. Double-check they've been tested or verified in associated specs.
9-10: Worker registration is coherent.Creating the
updatedResourceWorkerunderChannel.UpdatedResourcealigns with the new channel strategy described in the PR.
11-14: Consider validating resource existence.If there's any possibility that the
resourceobject might be undefined or have an invalidid, consider handling it gracefully before queryingreleaseTarget.Do you need a check or a guard clause to prevent runtime errors when
resource.idis missing? If so, I can generate a script to search for relevant usage patterns throughout the codebase.
28-33: Ensure idempotency of exit hooks.If this worker retries due to failures, make sure that
dispatchExitHookscan handle repeated calls without duplicating side effects.Would you like a helper script to scan for previously queued exit hooks to confirm whether deduplication or idempotent logic is in place?
35-42: Queue naming scheme appears fine.Each job name is distinctive based on resource/deployment/environment IDs. This is a clear approach to track queued jobs. No further concerns.
apps/event-worker/src/workers/updated-resources/dispatch-exit-hooks.ts (2)
53-64: Handle the empty “environments” array in the OR condition.
Ifsystem.environmentsis empty,environments.map(...)yields an empty array, makingor()contain no conditions. This typically evaluates toFALSE, implying no match — which might be correct, but just ensure this is the intended behavior.
75-90: Validate concurrency if multiple exit hook dispatches run simultaneously.
If multiple workers triggerdispatchExitHooksin parallel, ensure the underlying data remains consistent, especially with data or environment changes that could interleave.packages/job-dispatch/src/resource/group-resources-by-hook.ts (1)
88-112: Consider partial or batched queries for large sets of resources.
findExistingResourcesmay return a large array, so grouping them intotoInsert,toUpdate, andtoDeletein a single pass is efficient. Just ensure that upstream code gracefully handles massive resource sets without running into query or memory limits.apps/webservice/src/app/api/v1/resources/route.ts (7)
5-9: New import references look consistent.These imports properly align with your refactoring to use
@ctrlplane/dbfor the upsert logic while leveraging@ctrlplane/eventsand@ctrlplane/job-dispatchfor event queue capabilities. No issues found here.
60-61: Confirm approach to deleted resources.The comments indicate that deleted resources are intentionally ignored in this endpoint. Please confirm that this does not conflict with other parts of the application’s lifecycle that might rely on detecting or handling deleted resources for consistency.
63-66: Resource processing logic appears correct.Grouping resources via
groupResourcesByHookand mapping each resource to includeworkspaceIdis appropriate. Consider verifying behavior under large resource arrays to ensure no performance setbacks or memory constraints.
68-71: Parallel upsert is well-structured.Using
Promise.allfor parallel upserts is efficient. This approach cleanly handles insert and update operations together. No additional concerns here.
72-76: Job payload creation looks good.The job names and data structures match the inserted/updated resources. This is consistent with typical queue-based workflows.
78-81: Double-check queue error handling.If a queue is offline or encounters issues, the
addBulkcalls may fail. Consider adding fallback or explicit error handling to manage partial job submission.
83-84: Returning the count is straightforward.Summing the inserted and updated resource counts is concise. If you need more detailed feedback for the caller, returning the list of inserted/updated resource IDs might be beneficial in the future. For now, this is acceptable.
| const deleted = await deleteResources(tx, toDelete); | ||
| return { all: [...insertedResources, ...updatedResources], deleted }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add post-deletion validations for downstream dependencies.
After deleting resources, confirm that any downstream or dependent records are properly cleaned up. If deleteResources already handles this comprehensively, you can ignore this remark. Otherwise, consider verifying that leftover references (e.g., foreign keys in unrelated tables) do not remain.
🏁 Script executed:
#!/bin/bash
# Description: List references to any resource IDs that might remain after calling deleteResources
rg 'FOREIGN KEY.*resource.id' -A 10Length of output: 7796
Ensure Downstream Dependency Cleanup After Resource Deletion
After deleting resources via deleteResources, please verify that all dependent records are appropriately handled. Our investigation of the database constraints revealed that some FK definitions—such as the one in packages/db/drizzle/0037_giant_stardust.sql for the resource_session table (using ON DELETE no action)—will not automatically clean up dependent rows. If deleteResources already guarantees the removal or proper handling of these dependent entries, then this remark may be disregarded. Otherwise, please add explicit post-deletion validations or cleanup logic to prevent orphaned records.
There was a problem hiding this comment.
these are soft delete
There was a problem hiding this comment.
🧩 Analysis chain
Thanks for the clarification. Let me check the implementation details to better understand how the soft delete works in this codebase.
The results show that resources in the codebase use a soft delete approach with a deleted_at timestamp field in the schema. This means that when resources are "deleted" via deleteResources, they aren't physically removed from the database but rather marked as deleted by setting this timestamp.
This approach elegantly preserves referential integrity since the records still exist in the database - addressing the concern about orphaned records. Foreign key constraints with ON DELETE no action aren't problematic in this context because the referenced records remain in the database.
@adityachoudhari26 Thanks for clarifying! Since you're using soft deletes, the concern about orphaned records is addressed - the resource records remain in the database with a deleted_at timestamp rather than being physically removed, maintaining referential integrity.
(_/)
(•ᴥ•)
/ >📝
🏁 Script executed:
#!/bin/bash
# Check for soft delete implementation in the resource schema and deletion logic
echo "Checking resource schema for soft delete fields:"
rg "deleted" packages/db/src/schema/resource.ts -A 2
echo -e "\nChecking delete resources implementation:"
rg "deleteResources" packages/job-dispatch/src/resource/delete.js -A 10
Length of output: 552
Let me find the actual implementation of the delete functionality to better understand how soft deletes work in this codebase.
🏁 Script executed:
#!/bin/bash
# First, locate the delete.js or delete.ts file
echo "Searching for delete implementation file:"
fd "delete\.(js|ts)$" packages/job-dispatch/src/ --exec ls -la {}
echo -e "\nChecking current implementation of deleteResources:"
# Try to find the deleteResources function in any file
rg "export.*deleteResources" packages/job-dispatch/src/ -A 10
Length of output: 1449
Let me examine the full implementation of the delete functionality to understand exactly how soft deletes work in this system.
🏁 Script executed:
#!/bin/bash
# Let's see the full implementation of deleteResources
echo "Full implementation of deleteResources:"
cat packages/job-dispatch/src/resource/delete.ts
echo -e "\nChecking for update query that might set the deleted_at field:"
rg "deleted_at" packages/job-dispatch/src/resource/delete.ts -A 5
echo -e "\nLooking for soft delete implementation:"
rg "set\(.*deleted" packages/job-dispatch/src/
Length of output: 2907
@adityachoudhari26 Thanks for clarifying. You're right - I can see that the implementation uses soft deletes by setting the deletedAt timestamp rather than physically removing records:
tx
.update(SCHEMA.resource)
.set({ deletedAt: new Date() })
.where(inArray(SCHEMA.resource.id, resourceIds))This approach effectively addresses the concern about orphaned records since the referenced rows remain in the database, maintaining referential integrity even with ON DELETE no action constraints.
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#461
File: packages/job-dispatch/src/resource/handle-provider-scan.ts:48-49
Timestamp: 2025-04-09T17:14:38.730Z
Learning: Resources in the codebase implement a soft delete mechanism where records are marked with a `deleted_at` timestamp rather than being physically removed from the database, preserving referential integrity with dependent records.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#461
File: packages/job-dispatch/src/resource/handle-provider-scan.ts:48-49
Timestamp: 2025-04-09T17:14:38.730Z
Learning: The codebase implements soft deletes for resources by setting a `deletedAt` timestamp rather than physically removing records, which maintains referential integrity with foreign key constraints while allowing the application to filter out deleted resources as needed.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
adityachoudhari26
changed the title
Summary by CodeRabbit
New Features
Refactor
Chores