refactor(worker): remove organization existence check from workflow worker fixes NV-7910#11357
Conversation
…orker Co-authored-by: Dima Grossman <dima@grossman.io>
✅ Deploy Preview for dashboard-v2-novu-staging canceled.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -128,11 +118,4 @@ export class WorkflowWorker extends WorkflowWorkerService { | |||
| }); | |||
| }; | |||
There was a problem hiding this comment.
Previously, a missing organization caused an explicit early return with a clear log message. Now, if triggerEventUsecase.execute(data) throws because the organization doesn't exist, the job is swallowed by setSqsFailedHandler and permanently dropped (at-most-once). The spec has no test for this failure path — if the use-case doesn't handle a missing org gracefully and throws an unexpected error type, the failure will be invisible beyond the generic setSqsFailedHandler warning. Worth adding a test that stubs triggerEventUsecase.execute to throw and verifies the job is dropped (not retried).
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/worker/src/app/workflow/services/workflow.worker.ts
Line: 93-119
Comment:
**Missing-org path now silently drops jobs without a dedicated test**
Previously, a missing organization caused an explicit early return with a clear log message. Now, if `triggerEventUsecase.execute(data)` throws because the organization doesn't exist, the job is swallowed by `setSqsFailedHandler` and permanently dropped (at-most-once). The spec has no test for this failure path — if the use-case doesn't handle a missing org gracefully and throws an unexpected error type, the failure will be invisible beyond the generic `setSqsFailedHandler` warning. Worth adding a test that stubs `triggerEventUsecase.execute` to throw and verifies the job is dropped (not retried).
How can I resolve this? If you propose a fix, please make it concise.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
📝 WalkthroughWalkthroughWorkflowWorker removes its dependency on ChangesRemove WorkflowWorker organizationRepository dependency
🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
2 participants
Summary
Removes the redundant
organizationExistMongoDB lookup from the workflow worker job processor. Trigger jobs no longer skip processing when the organization document is missing at this stage.Changes
WorkflowWorkerorganizationExisthelper andCommunityOrganizationRepositorydependencyworkflow.worker.spec.tsconstructor call to match the new signatureWhy
The organization existence query is no longer needed before processing workflow trigger jobs.
Testing
Greptile Summary
This PR removes the
organizationExistMongoDB lookup and early-return guard fromWorkflowWorker, eliminating theCommunityOrganizationRepositorydependency from the constructor entirely. The intent is to treat any downstream failure (including a missing org) as a non-retryable error handled by the existing at-most-oncesetSqsFailedHandler, rather than a pre-checked silent skip.workflow.worker.ts: DropsCommunityOrganizationRepositoryimport/injection, removes the org-existence guard and its helper method. Jobs that previously silently acked on a missing org now proceed totriggerEventUsecase.execute().workflow.worker.spec.ts: Updates theWorkflowWorkerconstructor call to the new 5-argument signature.mockOrganizationRepositoryand theCommunityOrganizationRepositoryimport are intentionally kept sinceWorkflowQueueServicestill requires them.Confidence Score: 4/5
Safe to merge; the refactor is minimal and intentional, with the main trade-off being that jobs for missing organizations now fail loudly rather than being silently skipped.
The deletion is clean and self-contained. The key behavioral shift is that a job whose organization no longer exists will now reach triggerEventUsecase.execute() and, if it throws, be permanently dropped by the at-most-once setSqsFailedHandler rather than silently skipped upfront. Whether this is fully safe depends on whether the use-case handles a missing org gracefully — that path is not exercised by the updated tests.
workflow.worker.spec.ts has no test covering the scenario where triggerEventUsecase.execute is called with a non-existent organization, so the new error path is untested.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[SQS / BullMQ Job Received] --> B{Kill switch enabled?} B -- Yes --> C[Log warn, skip job] B -- No --> D[triggerEventUsecase.execute] D -- Success --> E[Job acked / complete] D -- Throws --> F[setSqsFailedHandler] F --> G[Log warning, drop job at-most-once] style C fill:#ffd700,color:#000 style G fill:#ff6b6b,color:#fff subgraph REMOVED [Removed in this PR] R1{organizationExist?} R2[Log warn, skip job] R1 -- No --> R2 endPrompt To Fix All With AI
Reviews (1): Last reviewed commit: "refactor(worker): remove organization ex..." | Re-trigger Greptile
What changed
The PR removes a redundant organization existence check from the workflow worker job processor. Previously, the worker would skip processing trigger jobs if the organization document wasn't found in MongoDB. This check has been eliminated along with the
organizationRepositorydependency, allowing jobs to proceed directly to execution after the kill-switch check.Affected areas
worker: Removed theorganizationRepositorydependency andorganizationExistmethod fromWorkflowWorker, eliminating the pre-processing organization check that was skipping job execution when the organization was missing.Testing
The test file was updated to match the new
WorkflowWorkerconstructor signature. Existing test logic for queue draining and worker lifecycle assertions remains unchanged. The PR was verified to pass Biome checks.