[PM-6426] Removing user-based alarms and fixing an issue found with setting steppedd alarm interavals

apps/browser/src/platform/services/browser-task-scheduler.service.spec.ts

@@ -4,7 +4,6 @@ import { BehaviorSubject, Observable } from "rxjs";
 import { ScheduledTaskNames } from "@bitwarden/common/platform/enums/scheduled-task-name.enum";
 import { ConsoleLogService } from "@bitwarden/common/platform/services/console-log.service";
 import { GlobalState, StateProvider } from "@bitwarden/common/platform/state";
-import { UserId } from "@bitwarden/common/types/guid";
 
 import { flushPromises, triggerOnAlarmEvent } from "../../autofill/spec/testing-utils";
 
@@ -37,15 +36,10 @@ function setupGlobalBrowserMock(overrides: Partial<chrome.alarms.Alarm> = {}) {
     ...overrides,
   };
 }
-const userUuid = "user-uuid" as UserId;
-function getAlarmNameMock(taskName: string) {
-  return `${taskName}__${userUuid}`;
-}
 
 describe("BrowserTaskSchedulerService", () => {
   const callback = jest.fn();
   const delayInMinutes = 2;
-  let activeUserIdMock$: BehaviorSubject<UserId>;
   let activeAlarmsMock$: BehaviorSubject<ActiveAlarm[]>;
   let logService: MockProxy<ConsoleLogService>;
   let stateProvider: MockProxy<StateProvider>;
@@ -73,14 +67,12 @@ describe("BrowserTaskSchedulerService", () => {
       }),
     ];
     activeAlarmsMock$ = new BehaviorSubject(activeAlarms);
-    activeUserIdMock$ = new BehaviorSubject(userUuid);
     logService = mock<ConsoleLogService>();
     globalStateMock = mock<GlobalState<any>>({
       state$: mock<Observable<any>>(),
       update: jest.fn((callback) => callback([], {} as any)),
     });
     stateProvider = mock<StateProvider>({
-      activeUserId$: activeUserIdMock$,
       getGlobal: jest.fn(() => globalStateMock),
     });
     browserTaskSchedulerService = new BrowserTaskSchedulerServiceImplementation(
@@ -126,7 +118,7 @@ describe("BrowserTaskSchedulerService", () => {
       );
 
       expect(chrome.alarms.create).toHaveBeenCalledWith(
-        getAlarmNameMock(ScheduledTaskNames.loginStrategySessionTimeout),
+        ScheduledTaskNames.loginStrategySessionTimeout,
         { delayInMinutes },
         expect.any(Function),
       );
@@ -144,43 +136,6 @@ describe("BrowserTaskSchedulerService", () => {
       expect(chrome.alarms.create).not.toHaveBeenCalled();
     });
 
-    it("clears a scheduled alarm if a user-specific alarm for the same task is being registered", async () => {
-      const mockAlarm = mock<chrome.alarms.Alarm>({
-        name: ScheduledTaskNames.loginStrategySessionTimeout,
-      });
-      chrome.alarms.get = jest
-        .fn()
-        .mockImplementation((name, callback) =>
-          callback(name === ScheduledTaskNames.loginStrategySessionTimeout ? mockAlarm : undefined),
-        );
-
-      await browserTaskSchedulerService.setTimeout(
-        ScheduledTaskNames.loginStrategySessionTimeout,
-        delayInMinutes * 60 * 1000,
-      );
-
-      expect(chrome.alarms.clear).toHaveBeenCalledWith(
-        ScheduledTaskNames.loginStrategySessionTimeout,
-        expect.any(Function),
-      );
-    });
-
-    it("creates an alarm that is not associated with a user", async () => {
-      activeUserIdMock$.next(undefined);
-      chrome.alarms.get = jest.fn().mockImplementation((_name, callback) => callback(undefined));
-
-      await browserTaskSchedulerService.setTimeout(
-        ScheduledTaskNames.loginStrategySessionTimeout,
-        delayInMinutes * 60 * 1000,
-      );
-
-      expect(chrome.alarms.create).toHaveBeenCalledWith(
-        ScheduledTaskNames.loginStrategySessionTimeout,
-        { delayInMinutes },
-        expect.any(Function),
-      );
-    });
-
     describe("when the task is scheduled to be triggered in less than 1 minute", () => {
       const delayInMs = 45000;
 
@@ -203,7 +158,7 @@ describe("BrowserTaskSchedulerService", () => {
         );
 
         expect(chrome.alarms.create).toHaveBeenCalledWith(
-          getAlarmNameMock(ScheduledTaskNames.loginStrategySessionTimeout),
+          ScheduledTaskNames.loginStrategySessionTimeout,
           { delayInMinutes: 0.5 },
           expect.any(Function),
         );
@@ -218,7 +173,7 @@ describe("BrowserTaskSchedulerService", () => {
         );
 
         expect(browser.alarms.create).toHaveBeenCalledWith(
-          getAlarmNameMock(ScheduledTaskNames.loginStrategySessionTimeout),
+          ScheduledTaskNames.loginStrategySessionTimeout,
           { delayInMinutes: 1 },
         );
       });
@@ -234,7 +189,7 @@ describe("BrowserTaskSchedulerService", () => {
         await flushPromises();
 
         expect(chrome.alarms.clear).toHaveBeenCalledWith(
-          getAlarmNameMock(ScheduledTaskNames.loginStrategySessionTimeout),
+          ScheduledTaskNames.loginStrategySessionTimeout,
           expect.any(Function),
         );
       });
@@ -263,18 +218,23 @@ describe("BrowserTaskSchedulerService", () => {
         );
 
         expect(chrome.alarms.create).toHaveBeenCalledWith(
-          `${getAlarmNameMock(ScheduledTaskNames.loginStrategySessionTimeout)}__0`,
+          `${ScheduledTaskNames.loginStrategySessionTimeout}__0`,
-          { periodInMinutes: 0.5 },
+          { periodInMinutes: 0.6666666666666666, delayInMinutes: 0.5 },
           expect.any(Function),
         );
         expect(chrome.alarms.create).toHaveBeenCalledWith(
-          `${getAlarmNameMock(ScheduledTaskNames.loginStrategySessionTimeout)}__1`,
+          `${ScheduledTaskNames.loginStrategySessionTimeout}__1`,
-          { periodInMinutes: 0.6666666666666666 },
+          { periodInMinutes: 0.6666666666666666, delayInMinutes: 0.6666666666666666 },
           expect.any(Function),
         );
         expect(chrome.alarms.create).toHaveBeenCalledWith(
-          `${getAlarmNameMock(ScheduledTaskNames.loginStrategySessionTimeout)}__2`,
+          `${ScheduledTaskNames.loginStrategySessionTimeout}__2`,
-          { periodInMinutes: 0.8333333333333333 },
+          { periodInMinutes: 0.6666666666666666, delayInMinutes: 0.8333333333333333 },
+          expect.any(Function),
+        );
+        expect(chrome.alarms.create).toHaveBeenCalledWith(
+          `${ScheduledTaskNames.loginStrategySessionTimeout}__3`,
+          { periodInMinutes: 0.6666666666666666, delayInMinutes: 1 },
           expect.any(Function),
         );
       });
@@ -315,7 +275,7 @@ describe("BrowserTaskSchedulerService", () => {
       );
 
       expect(chrome.alarms.create).toHaveBeenCalledWith(
-        getAlarmNameMock(ScheduledTaskNames.loginStrategySessionTimeout),
+        ScheduledTaskNames.loginStrategySessionTimeout,
         { periodInMinutes, delayInMinutes: 0.5 },
         expect.any(Function),
       );
@@ -329,7 +289,7 @@ describe("BrowserTaskSchedulerService", () => {
       );
 
       expect(chrome.alarms.create).toHaveBeenCalledWith(
-        getAlarmNameMock(ScheduledTaskNames.loginStrategySessionTimeout),
+        ScheduledTaskNames.loginStrategySessionTimeout,
         { periodInMinutes, delayInMinutes: periodInMinutes },
         expect.any(Function),
       );
@@ -380,27 +340,6 @@ describe("BrowserTaskSchedulerService", () => {
   });
 
   describe("triggering a task", () => {
-    it("clears an non user-based alarm if a separate user-based alarm has been set up", async () => {
-      jest.useFakeTimers();
-      activeUserIdMock$.next(undefined);
-      const delayInMs = 10000;
-      chrome.alarms.get = jest
-        .fn()
-        .mockImplementation((_name, callback) => callback(mock<chrome.alarms.Alarm>()));
-
-      await browserTaskSchedulerService.setTimeout(
-        ScheduledTaskNames.loginStrategySessionTimeout,
-        delayInMs,
-      );
-      jest.advanceTimersByTime(delayInMs);
-      await flushPromises();
-
-      expect(chrome.alarms.clear).toHaveBeenCalledWith(
-        ScheduledTaskNames.loginStrategySessionTimeout,
-        expect.any(Function),
-      );
-    });
-
     it("triggers a task when an onAlarm event is triggered", () => {
       const alarm = mock<chrome.alarms.Alarm>({
         name: ScheduledTaskNames.loginStrategySessionTimeout,
@@ -425,7 +364,7 @@ describe("BrowserTaskSchedulerService", () => {
       });
 
       expect(chrome.alarms.clear).toHaveBeenCalledWith(
-        getAlarmNameMock(ScheduledTaskNames.loginStrategySessionTimeout),
+        ScheduledTaskNames.loginStrategySessionTimeout,
         expect.any(Function),
       );
     });
@@ -438,7 +377,7 @@ describe("BrowserTaskSchedulerService", () => {
       });
 
       expect(browser.alarms.clear).toHaveBeenCalledWith(
-        getAlarmNameMock(ScheduledTaskNames.loginStrategySessionTimeout),
+        ScheduledTaskNames.loginStrategySessionTimeout,
       );
     });
   });

apps/browser/src/platform/services/browser-task-scheduler.service.ts

@@ -58,8 +58,7 @@ export class BrowserTaskSchedulerServiceImplementation
     this.validateRegisteredTask(taskName);
 
     const delayInMinutes = delayInMs / 1000 / 60;
-    const alarmName = await this.getActiveUserAlarmName(taskName);
+    await this.scheduleAlarm(taskName, {
-    await this.scheduleAlarm(alarmName, {
       delayInMinutes: this.getUpperBoundDelayInMinutes(delayInMinutes),
     });
 
@@ -67,8 +66,8 @@ export class BrowserTaskSchedulerServiceImplementation
     // The alarm previously scheduled will be used as a backup in case the setTimeout fails.
     if (delayInMinutes < 1) {
       return globalThis.setTimeout(async () => {
-        await this.clearScheduledAlarm(alarmName);
+        await this.clearScheduledAlarm(taskName);
-        await this.triggerTask(alarmName);
+        await this.triggerTask(taskName);
       }, delayInMs);
     }
   }
@@ -90,16 +89,15 @@ export class BrowserTaskSchedulerServiceImplementation
     this.validateRegisteredTask(taskName);
 
     const intervalInMinutes = intervalInMs / 1000 / 60;
-    const alarmName = await this.getActiveUserAlarmName(taskName);
     const initialDelayInMinutes = initialDelayInMs
       ? initialDelayInMs / 1000 / 60
       : intervalInMinutes;
 
     if (intervalInMinutes < 1) {
-      return this.setupSteppedIntervalAlarms(taskName, alarmName, intervalInMs);
+      return this.setupSteppedIntervalAlarms(taskName, intervalInMs);
     }
 
-    await this.scheduleAlarm(alarmName, {
+    await this.scheduleAlarm(taskName, {
       periodInMinutes: this.getUpperBoundDelayInMinutes(intervalInMinutes),
       delayInMinutes: this.getUpperBoundDelayInMinutes(initialDelayInMinutes),
     });
@@ -112,28 +110,28 @@ export class BrowserTaskSchedulerServiceImplementation
    * api does not support intervals less than 1 minute.
    *
    * @param taskName - The name of the task, separate of any user id.
-   * @param alarmName - The name of the alarm to create, could contain a user id.
    * @param intervalInMs - The interval in milliseconds.
    */
   private async setupSteppedIntervalAlarms(
     taskName: ScheduledTaskName,
-    alarmName: string,
     intervalInMs: number,
   ): Promise<number | NodeJS.Timeout> {
     const alarmMinDelayInMinutes = this.getAlarmMinDelayInMinutes();
     const intervalInMinutes = intervalInMs / 1000 / 60;
-    const numberOfAlarmsToCreate = Math.ceil(1 / intervalInMinutes);
+    const numberOfAlarmsToCreate = Math.ceil(Math.ceil(1 / intervalInMinutes) / 2) + 1;
+    const steppedAlarmPeriodInMinutes = alarmMinDelayInMinutes + intervalInMinutes;
     for (let alarmIndex = 0; alarmIndex < numberOfAlarmsToCreate; alarmIndex++) {
-      const steppedAlarmName = `${alarmName}__${alarmIndex}`;
+      const steppedAlarmName = `${taskName}__${alarmIndex}`;
-      const periodInMinutes = alarmMinDelayInMinutes + intervalInMinutes * alarmIndex;
+
+      const delayInMinutes = this.getUpperBoundDelayInMinutes(
+        alarmMinDelayInMinutes + intervalInMinutes * alarmIndex,
+      );
 
-      // We need to clear alarms based on the task name as well as the
-      // user-based alarm name to ensure duplicate alarms are not created.
-      await this.clearScheduledAlarm(`${taskName}__${alarmIndex}`);
       await this.clearScheduledAlarm(steppedAlarmName);
 
       await this.scheduleAlarm(steppedAlarmName, {
-        periodInMinutes: this.getUpperBoundDelayInMinutes(periodInMinutes),
+        periodInMinutes: steppedAlarmPeriodInMinutes,
+        delayInMinutes,
       });
     }
 
@@ -147,7 +145,7 @@ export class BrowserTaskSchedulerServiceImplementation
         return;
       }
 
-      await this.triggerTask(alarmName, intervalInMinutes);
+      await this.triggerTask(taskName, intervalInMinutes);
     }, intervalInMs);
 
     return intervalId;
@@ -168,8 +166,7 @@ export class BrowserTaskSchedulerServiceImplementation
       return;
     }
 
-    const alarmName = await this.getActiveUserAlarmName(taskName);
+    await this.clearScheduledAlarm(taskName);
-    await this.clearScheduledAlarm(alarmName);
   }
 
   /**
@@ -226,14 +223,6 @@ export class BrowserTaskSchedulerServiceImplementation
       return;
     }
 
-    // We should always prioritize user-based alarms over non-user-based alarms. If a non-user-based alarm
-    // exists when the user-based alarm is being created, we want to clear the non-user-based alarm.
-    const taskName = this.getTaskFromAlarmName(alarmName);
-    const existingTaskBasedAlarm = await this.getAlarm(taskName);
-    if (existingTaskBasedAlarm) {
-      await this.clearScheduledAlarm(taskName);
-    }
-
     await this.createAlarm(alarmName, createInfo);
     await this.setActiveAlarm(alarmName, createInfo);
   }
@@ -331,43 +320,10 @@ export class BrowserTaskSchedulerServiceImplementation
     if (handler) {
       handler();
     }
-
-    // We should always prioritize user-based alarms over non-user-based alarms. As a result, if a triggered
-    // alarm is not user-based, we want to verify if  the alarm should continue to exist. If an alarm exists
-    // for the same task that is user-based, we want to clear the non-user-based alarm.
-    if (alarmName === taskName) {
-      const taskName = this.getTaskFromAlarmName(alarmName);
-      const existingUserBasedAlarm = await this.getAlarm(taskName);
-      if (existingUserBasedAlarm) {
-        await this.clearScheduledAlarm(taskName);
-      }
-    }
   }
 
   /**
-   * Gets the active user id from state.
+   * Parses and returns the task name from an alarm name.
-   */
-  private async getActiveUserId(): Promise<string> {
-    return await firstValueFrom(this.stateProvider.activeUserId$);
-  }
-
-  /**
-   * Gets the active user alarm name by appending the active user id to the task name.
-   *
-   * @param taskName - The task name to append the active user id to.
-   */
-  private async getActiveUserAlarmName(taskName: ScheduledTaskName): Promise<string> {
-    const activeUserId = await this.getActiveUserId();
-    if (!activeUserId) {
-      return taskName;
-    }
-
-    return `${taskName}__${activeUserId}`;
-  }
-
-  /**
-   * Parses and returns the task name from an alarm name. If the alarm name
-   * contains a user id, it will return the task name without the user id.
    *
    * @param alarmName - The alarm name to parse.
    */