Merge pull request #40 from Johnpii1/main

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

Author: ogazboiz

src/services/notificationService.ts

@@ -509,6 +509,16 @@ export async function runNotificationCleanup(): Promise<void> {
   await runCleanup();
 }
 
+function runScheduledCleanup(): void {
+  void (async () => {
+    try {
+      await runCleanup();
+    } catch (error) {
+      logger.error("Notification cleanup scheduled run failed", { error });
+    }
+  })();
+}
+
 /**
  * Starts a periodic scheduler to clean up old notifications based on retention policy.
  */
@@ -529,10 +539,10 @@ export function startNotificationCleanupScheduler(): void {
   );
 
   // Run once immediately on start to clear any backlog
-  void runCleanup();
+  runScheduledCleanup();
 
   cleanupInterval = setInterval(() => {
-    void runCleanup();
+    runScheduledCleanup();
   }, intervalMs);
 
   logger.info("Notification cleanup scheduler started", {

src/tests/notificationCleanup.test.ts

@@ -1,5 +1,23 @@
 import { jest } from "@jest/globals";
 
+const mockLoggerError = jest.fn();
+
+jest.unstable_mockModule("../utils/logger.js", () => ({
+  default: {
+    error: mockLoggerError,
+    info: jest.fn(),
+    warn: jest.fn(),
+    debug: jest.fn(),
+  },
+}));
+
+jest.unstable_mockModule("../services/cacheService.js", () => ({
+  cacheService: {
+    setNotExists: jest.fn(async () => true),
+    delete: jest.fn(async () => undefined),
+  },
+}));
+
 // Use unstable_mockModule for robust ESM mocking of the connection module
 jest.unstable_mockModule("../db/connection.js", () => ({
   query: jest.fn(),
@@ -11,14 +29,26 @@ jest.unstable_mockModule("../db/connection.js", () => ({
 
 // Use dynamic imports TO ENSURE mocks are applied BEFORE the module is loaded
 const { query } = await import("../db/connection.js");
-const { notificationService } =
-  await import("../services/notificationService.js");
+const {
+  notificationService,
+  startNotificationCleanupScheduler,
+  stopNotificationCleanupScheduler,
+} = await import("../services/notificationService.js");
 
 const mockedQuery = query as jest.MockedFunction<typeof query>;
 
 describe("Notification Cleanup Strategy", () => {
   beforeEach(() => {
     jest.clearAllMocks();
+    jest.useRealTimers();
+    stopNotificationCleanupScheduler();
+    delete process.env.NOTIFICATION_CLEANUP_INTERVAL_MS;
+  });
+
+  afterEach(() => {
+    stopNotificationCleanupScheduler();
+    jest.useRealTimers();
+    delete process.env.NOTIFICATION_CLEANUP_INTERVAL_MS;
   });
 
   describe("deleteOldNotifications", () => {
@@ -87,6 +117,36 @@ describe("Notification Cleanup Strategy", () => {
     });
   });
 
+  describe("startNotificationCleanupScheduler", () => {
+    it("should catch and log thrown cleanup errors without stopping future runs", async () => {
+      jest.useFakeTimers();
+      process.env.NOTIFICATION_CLEANUP_INTERVAL_MS = "1000";
+
+      const deleteOldSpy = jest
+        .spyOn(notificationService, "deleteOldNotifications")
+        .mockRejectedValueOnce(new Error("cleanup failed"))
+        .mockResolvedValue(0);
+      const deleteReadSpy = jest
+        .spyOn(notificationService, "deleteReadAndArchived")
+        .mockResolvedValue(0);
+
+      startNotificationCleanupScheduler();
+      for (let i = 0; i < 10; i += 1) {
+        await Promise.resolve();
+      }
+
+      expect(mockLoggerError).toHaveBeenCalledWith(
+        "Notification cleanup scheduled run failed",
+        { error: expect.any(Error) },
+      );
+
+      await jest.advanceTimersByTimeAsync(1000);
+
+      expect(deleteOldSpy).toHaveBeenCalledTimes(2);
+      expect(deleteReadSpy).toHaveBeenCalledTimes(1);
+    });
+  });
+
   describe("archiveNotifications", () => {
     it("should set status to archived and read to true for the given ids", async () => {
       mockedQuery.mockResolvedValue({ rowCount: 2 } as any);