Skip to content

[Backend] notificationService cleanup interval lacks a try/catch around its async callback#40

Open
Johnpii1 wants to merge 2 commits into
LabsCrypt:mainfrom
Johnpii1:main
Open

[Backend] notificationService cleanup interval lacks a try/catch around its async callback#40
Johnpii1 wants to merge 2 commits into
LabsCrypt:mainfrom
Johnpii1:main

Conversation

@Johnpii1

Copy link
Copy Markdown

close #15

Description
Add a guarded helper runScheduledCleanup() that wraps runCleanup() in a void IIFE with try/catch and logs failures via the shared logger.error path.
Replace the immediate startup call and the setInterval callback to invoke runScheduledCleanup() so scheduled failures do not become unhandled rejections and do not stop future runs.
Add unit test coverage that mocks a thrown error from deleteOldNotifications, asserts the error is logged, and verifies a later scheduled run still executes.
Update test scaffolding to mock cacheService and logger and to start/stop the scheduler during test setup/teardown.

Testing
Ran npm test -- --runInBand src/tests/notificationCleanup.test.ts and the test file passed (11 tests passed, 0 failed).
Ran npm run typecheck and the TypeScript check completed with no errors.

@ogazboiz ogazboiz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clean, merging once ci is green. runScheduledCleanup() now wraps await runCleanup() in try/catch with logger.error and is used for both the startup run and the setInterval callback (was void runCleanup(), which could throw an unhandled rejection and take down the process). a rejection can no longer escape, the interval keeps firing, and errors are logged not swallowed. the new test covering "catch and log without stopping future runs" passes. nice fix.

small thing for next time: this is pushed from your fork's main branch, a descriptive branch (e.g. fix/notification-cleanup-trycatch) is cleaner and plays better with CI association.

if you want to keep contributing, join us on Telegram: https://t.me/+DOylgFv1jyJlNzM0

ps: CI is red right now on the migrate step, but that is a pre-existing main-branch issue (a commonjs migration file under our ESM package), nothing to do with your change. once that is sorted this lands.

@Johnpii1

Copy link
Copy Markdown
Author

Okay

@Johnpii1

Copy link
Copy Markdown
Author

I Will fix the ci

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Backend] notificationService cleanup interval lacks a try/catch around its async callback

2 participants