Conversation
WalkthroughJobQueueService now requires ExecutionContextService, runs each job inside runWithContext seeding CURRENT_USER and ABILITY (via createMongoAbility), and updates queue retry options (retryLimit 5, retryBackoff enabled, retryDelayMax 5min). Tests updated for constructor change. Notification job name changed to "NotificationJob". Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor PGBoss as PG Boss
participant JQS as JobQueueService
participant ECS as ExecutionContextService
participant Handler as JobHandler
participant Logger as Logger
PGBoss->>JQS: deliver(job)
Note right of JQS: prepare context (CURRENT_USER, ABILITY)
rect rgba(200,230,255,0.18)
JQS->>ECS: runWithContext(context, fn)
activate ECS
ECS->>Handler: handle(job.data)
Handler-->>ECS: result / throws
deactivate ECS
end
alt success
JQS->>Logger: log(JOB_DONE, job.id)
else failure
JQS->>Logger: log(JOB_FAILED, job.id, error)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/src/core/services/job-queue/job-queue.service.spec.ts(2 hunks)apps/api/src/core/services/job-queue/job-queue.service.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/api/src/core/services/job-queue/job-queue.service.tsapps/api/src/core/services/job-queue/job-queue.service.spec.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/api/src/core/services/job-queue/job-queue.service.tsapps/api/src/core/services/job-queue/job-queue.service.spec.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/no-jest-mock.mdc)
Don't use
jest.mock()to mock dependencies in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under test.
**/*.spec.{ts,tsx}: Usesetupfunction instead ofbeforeEachin test files
setupfunction must be at the bottom of the rootdescribeblock in test files
setupfunction creates an object under test and returns it
setupfunction should accept a single parameter with inline type definition
Don't use shared state insetupfunction
Don't specify return type ofsetupfunction
Files:
apps/api/src/core/services/job-queue/job-queue.service.spec.ts
🧬 Code graph analysis (1)
apps/api/src/core/services/job-queue/job-queue.service.ts (1)
packages/logging/src/services/logger/logger.service.ts (1)
error(107-109)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1998 +/- ##
==========================================
- Coverage 45.88% 45.52% -0.36%
==========================================
Files 1012 1002 -10
Lines 28554 28211 -343
Branches 7480 7439 -41
==========================================
- Hits 13101 12844 -257
Misses 14248 14248
+ Partials 1205 1119 -86
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
stalniy
force-pushed
the
feat/bg-jobs-exec-context
branch
from
41e185e to
1cee9bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/core/services/job-queue/job-queue.service.spec.ts (1)
18-21: Update test expectations for the new retry configurationThe production code now registers queues with
{ retryLimit: 5, retryBackoff: true, retryDelayMax: 5 * 60 }, but the tests still expectretryLimit: 10and don’t assert the new flags (Lines 18-21, 49-55). As written, the spec will fail. Please align the expectations with the implementation.Also applies to: 48-55
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/api/src/core/services/job-queue/job-queue.service.spec.ts(2 hunks)apps/api/src/core/services/job-queue/job-queue.service.ts(4 hunks)apps/api/src/notifications/services/notification-handler/notification.handler.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/no-jest-mock.mdc)
Don't use
jest.mock()to mock dependencies in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under test.
**/*.spec.{ts,tsx}: Usesetupfunction instead ofbeforeEachin test files
setupfunction must be at the bottom of the rootdescribeblock in test files
setupfunction creates an object under test and returns it
setupfunction should accept a single parameter with inline type definition
Don't use shared state insetupfunction
Don't specify return type ofsetupfunction
Files:
apps/api/src/core/services/job-queue/job-queue.service.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/api/src/core/services/job-queue/job-queue.service.spec.tsapps/api/src/notifications/services/notification-handler/notification.handler.tsapps/api/src/core/services/job-queue/job-queue.service.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/api/src/core/services/job-queue/job-queue.service.spec.tsapps/api/src/notifications/services/notification-handler/notification.handler.tsapps/api/src/core/services/job-queue/job-queue.service.ts
🧬 Code graph analysis (2)
apps/api/src/notifications/services/notification-handler/notification.handler.ts (2)
apps/api/src/core/services/job-queue/job-queue.service.ts (1)
JOB_NAME(188-188)apps/api/src/core/services/domain-events/domain-events.service.ts (1)
JOB_NAME(6-6)
apps/api/src/core/services/job-queue/job-queue.service.ts (1)
packages/logging/src/services/logger/logger.service.ts (1)
error(107-109)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
apps/api/src/notifications/services/notification-handler/notification.handler.ts
Show resolved
Hide resolved
stalniy
force-pushed
the
feat/bg-jobs-exec-context
branch
from
1cee9bc to
53cf669
Compare
stalniy
force-pushed
the
feat/bg-jobs-exec-context
branch
from
53cf669 to
7573320
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/api/src/core/services/job-queue/job-queue.service.ts (1)
107-126: Await context seeding to ensure initialization completes before handler execution.The
executionContextService.set()calls on lines 107 and 126 are not awaited. Sinceset()returns a promise (confirmed by the test mock at line 263 in the spec file), the handler may start before the context is fully initialized, and any rejection will be silently ignored.Apply this diff to await both context-seeding calls:
- this.executionContextService.set("CURRENT_USER", { + await this.executionContextService.set("CURRENT_USER", { id: "bg-job-user", bio: "", email: "bg-job-user@akash.network", emailVerified: false, stripeCustomerId: "", subscribedToNewsletter: false, createdAt: new Date(), lastActiveAt: new Date(), lastIp: null, lastUserAgent: null, lastFingerprint: null, youtubeUsername: null, twitterUsername: null, githubUsername: null, userId: "system:bg-job-user", username: "___bg_job_user___", trial: false }); - this.executionContextService.set("ABILITY", createMongoAbility<MongoAbility>()); + await this.executionContextService.set("ABILITY", createMongoAbility<MongoAbility>());
🧹 Nitpick comments (1)
apps/api/src/core/services/job-queue/job-queue.service.spec.ts (1)
262-265: Replicate context isolation in the mock
The realExecutionContextService.runWithContextusesAsyncLocalStorage.run(new Map(), …)to isolate each invocation. The current mock just callscb()directly, so tests won’t catch cross‐run state leakage. Either use the real service in your specs or update the mock to simulate per‐run storage (e.g. invokestorage.run(new Map(), …)).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/api/src/core/services/job-queue/job-queue.service.spec.ts(5 hunks)apps/api/src/core/services/job-queue/job-queue.service.ts(4 hunks)apps/api/src/notifications/services/notification-handler/notification.handler.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/notifications/services/notification-handler/notification.handler.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/no-jest-mock.mdc)
Don't use
jest.mock()to mock dependencies in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under test.
**/*.spec.{ts,tsx}: Usesetupfunction instead ofbeforeEachin test files
setupfunction must be at the bottom of the rootdescribeblock in test files
setupfunction creates an object under test and returns it
setupfunction should accept a single parameter with inline type definition
Don't use shared state insetupfunction
Don't specify return type ofsetupfunction
Files:
apps/api/src/core/services/job-queue/job-queue.service.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/api/src/core/services/job-queue/job-queue.service.spec.tsapps/api/src/core/services/job-queue/job-queue.service.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/api/src/core/services/job-queue/job-queue.service.spec.tsapps/api/src/core/services/job-queue/job-queue.service.ts
🧬 Code graph analysis (1)
apps/api/src/core/services/job-queue/job-queue.service.ts (1)
packages/logging/src/services/logger/logger.service.ts (1)
error(107-109)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
🔇 Additional comments (8)
apps/api/src/core/services/job-queue/job-queue.service.spec.ts (3)
6-6: LGTM!The ExecutionContextService type import is correctly added to support the updated constructor signature.
20-22: LGTM!The retry configuration expectations are correctly updated to reflect the production changes:
retryBackoff: true,retryDelayMax: 300(5 minutes in seconds), andretryLimit: 5.Also applies to: 52-54, 58-60, 131-133
268-273: LGTM!The constructor call correctly includes
executionContextServiceas the third parameter, matching the updated production signature.apps/api/src/core/services/job-queue/job-queue.service.ts (5)
1-1: LGTM!CASL ability imports are correctly added to support the execution context seeding with permissions.
7-7: LGTM!ExecutionContextService import is correctly added.
19-19: LGTM!The ExecutionContextService dependency is correctly injected as a constructor parameter.
41-43: LGTM!Retry configuration is appropriately tuned:
retryLimit: 5(reduced from 10),retryBackoff: true(exponential backoff enabled), andretryDelayMax: 5 * 60(5-minute cap).
131-144: LGTM!The error handling correctly logs job failures and re-throws to trigger PgBoss retry logic.
3 participants
Why
Due to the nature of nodejs and DI, we need to run bg jobs in execution context to ensure that it's safe to use application services (which may depend on executionContext) inside background jobs.
For now, use empty ability (to ensure nothing bad happens) and fake user (to ensure feature flags service will produce stable values during gradual rollout). Just to minimize the changes because the final/proper decision in this regard should be done via a discussion proposal with enforcement on CI level. See #1997
Summary by CodeRabbit
New Features
Tests