mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-01-23 02:36:05 +00:00
fix(reminder): prevent duplicate notifications from worker race condition
Add service-level deduplication to prevent the same reminder from triggering multiple notifications when the worker's 10-second polling interval races with state updates. The fix tracks recently processed reminder IDs and filters them out before emitting. IDs are cleared when snoozing (to allow re-trigger at new time) and auto-cleaned after 60 seconds to prevent memory leaks. Closes #5925
This commit is contained in:
parent
6d1f5a7113
commit
5f724ad16e
2 changed files with 171 additions and 1 deletions
|
|
@ -156,4 +156,131 @@ describe('ReminderService', () => {
|
|||
expect(snoozedReminder.remindAt).toBeGreaterThanOrEqual(beforeSnooze + snoozeTime);
|
||||
});
|
||||
});
|
||||
|
||||
describe('duplicate reminder prevention', () => {
|
||||
let messageHandler: (msg: MessageEvent) => Promise<void>;
|
||||
let taskServiceSpy: jasmine.SpyObj<TaskService>;
|
||||
|
||||
beforeEach(async () => {
|
||||
// Initialize service first to register the message handler
|
||||
await service.init();
|
||||
|
||||
// Capture the message handler registered on the worker
|
||||
const call = mockWorker.addEventListener.calls
|
||||
.all()
|
||||
.find((c) => c.args[0] === 'message');
|
||||
messageHandler = call?.args[1] as unknown as (msg: MessageEvent) => Promise<void>;
|
||||
|
||||
// Get TaskService spy to configure task responses
|
||||
taskServiceSpy = TestBed.inject(TaskService) as jasmine.SpyObj<TaskService>;
|
||||
});
|
||||
|
||||
it('should not emit duplicate reminders when worker sends same reminder twice', async () => {
|
||||
const testReminder = {
|
||||
id: 'reminder-dup-1',
|
||||
relatedId: 'task-dup-1',
|
||||
title: 'Duplicate Test',
|
||||
remindAt: Date.now() - 1000,
|
||||
type: 'TASK' as const,
|
||||
};
|
||||
|
||||
// Mock task exists and is not done
|
||||
taskServiceSpy.getByIdOnce$.and.returnValue(
|
||||
of({ id: 'task-dup-1', isDone: false } as any),
|
||||
);
|
||||
|
||||
const emissions: any[] = [];
|
||||
service.onRemindersActive$.subscribe((reminders) => {
|
||||
emissions.push(reminders);
|
||||
});
|
||||
|
||||
// Simulate worker sending same reminder twice (race condition)
|
||||
await messageHandler({ data: [testReminder] } as MessageEvent);
|
||||
await messageHandler({ data: [testReminder] } as MessageEvent);
|
||||
|
||||
// Should only have one emission
|
||||
expect(emissions.length).toBe(1);
|
||||
expect(emissions[0].length).toBe(1);
|
||||
expect(emissions[0][0].id).toBe('reminder-dup-1');
|
||||
});
|
||||
|
||||
it('should allow reminder to emit again after snooze', async () => {
|
||||
const reminderId = service.addReminder(
|
||||
'TASK',
|
||||
'task-snooze-emit',
|
||||
'Snooze Emit Test',
|
||||
Date.now() - 1000,
|
||||
);
|
||||
|
||||
const testReminder = {
|
||||
id: reminderId,
|
||||
relatedId: 'task-snooze-emit',
|
||||
title: 'Snooze Emit Test',
|
||||
remindAt: Date.now() - 1000,
|
||||
type: 'TASK' as const,
|
||||
};
|
||||
|
||||
// Mock task exists and is not done
|
||||
taskServiceSpy.getByIdOnce$.and.returnValue(
|
||||
of({ id: 'task-snooze-emit', isDone: false } as any),
|
||||
);
|
||||
|
||||
const emissions: any[] = [];
|
||||
service.onRemindersActive$.subscribe((reminders) => {
|
||||
emissions.push(reminders);
|
||||
});
|
||||
|
||||
// First activation
|
||||
await messageHandler({ data: [testReminder] } as MessageEvent);
|
||||
expect(emissions.length).toBe(1);
|
||||
|
||||
// Snooze the reminder (this should clear the processed state)
|
||||
service.snooze(reminderId, 5000);
|
||||
|
||||
// Update the reminder time for the "new" activation
|
||||
const snoozedReminder = { ...testReminder, remindAt: Date.now() - 500 };
|
||||
|
||||
// Second activation after snooze should work
|
||||
await messageHandler({ data: [snoozedReminder] } as MessageEvent);
|
||||
expect(emissions.length).toBe(2);
|
||||
});
|
||||
|
||||
it('should clean up processed state after timeout', async () => {
|
||||
jasmine.clock().install();
|
||||
|
||||
const testReminder = {
|
||||
id: 'reminder-cleanup-1',
|
||||
relatedId: 'task-cleanup-1',
|
||||
title: 'Cleanup Test',
|
||||
remindAt: Date.now() - 1000,
|
||||
type: 'TASK' as const,
|
||||
};
|
||||
|
||||
taskServiceSpy.getByIdOnce$.and.returnValue(
|
||||
of({ id: 'task-cleanup-1', isDone: false } as any),
|
||||
);
|
||||
|
||||
const emissions: any[] = [];
|
||||
service.onRemindersActive$.subscribe((reminders) => {
|
||||
emissions.push(reminders);
|
||||
});
|
||||
|
||||
// First activation
|
||||
await messageHandler({ data: [testReminder] } as MessageEvent);
|
||||
expect(emissions.length).toBe(1);
|
||||
|
||||
// Second activation should be blocked
|
||||
await messageHandler({ data: [testReminder] } as MessageEvent);
|
||||
expect(emissions.length).toBe(1);
|
||||
|
||||
// Advance time by 61 seconds (cleanup happens at 60s)
|
||||
jasmine.clock().tick(61000);
|
||||
|
||||
// Third activation should work after cleanup
|
||||
await messageHandler({ data: [testReminder] } as MessageEvent);
|
||||
expect(emissions.length).toBe(2);
|
||||
|
||||
jasmine.clock().uninstall();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -49,6 +49,9 @@ export class ReminderService {
|
|||
|
||||
private _w: Worker;
|
||||
private _reminders: Reminder[] = [];
|
||||
// Track recently processed reminder IDs to prevent duplicate emissions from worker race condition
|
||||
private _recentlyProcessedReminderIds = new Set<string>();
|
||||
private _cleanupTimeouts = new Map<string, ReturnType<typeof setTimeout>>();
|
||||
|
||||
constructor() {
|
||||
// this._triggerPauseAfterUpdate$.subscribe((v) => Log.log('_triggerPauseAfterUpdate$', v));
|
||||
|
|
@ -161,6 +164,8 @@ export class ReminderService {
|
|||
this._reminders[i] = Object.assign({}, this._reminders[i], reminderChanges);
|
||||
// Update worker immediately to prevent race condition with 10s check interval
|
||||
this._updateRemindersInWorker(this._reminders);
|
||||
// Clear from processed set so snooze can trigger notification at new time
|
||||
this._clearProcessedReminder(reminderId);
|
||||
}
|
||||
this._saveModel(this._reminders);
|
||||
}
|
||||
|
|
@ -174,6 +179,8 @@ export class ReminderService {
|
|||
this._reminders.splice(i, 1);
|
||||
// Update worker immediately to prevent race condition with 10s check interval
|
||||
this._updateRemindersInWorker(this._reminders);
|
||||
// Clean up tracking state
|
||||
this._clearProcessedReminder(reminderIdToRemove);
|
||||
this._saveModel(this._reminders);
|
||||
} else {
|
||||
// throw new Error('Unable to find reminder with id ' + reminderIdToRemove);
|
||||
|
|
@ -233,12 +240,22 @@ export class ReminderService {
|
|||
}
|
||||
}),
|
||||
);
|
||||
const finalReminders = remindersWithData.filter(
|
||||
const validReminders = remindersWithData.filter(
|
||||
(reminder): reminder is Reminder => !!reminder,
|
||||
);
|
||||
|
||||
// Filter out reminders that were recently processed to prevent duplicate notifications
|
||||
// from worker race conditions (worker may emit same reminder before state update reaches it)
|
||||
const finalReminders = validReminders.filter(
|
||||
(reminder) => !this._recentlyProcessedReminderIds.has(reminder.id),
|
||||
);
|
||||
|
||||
Log.log(`ReminderService: ${finalReminders.length} valid reminder(s) to show`);
|
||||
if (finalReminders.length > 0) {
|
||||
// Mark these reminders as processed
|
||||
finalReminders.forEach((reminder) => {
|
||||
this._markReminderAsProcessed(reminder.id);
|
||||
});
|
||||
this._onRemindersActive$.next(finalReminders);
|
||||
}
|
||||
}
|
||||
|
|
@ -289,4 +306,30 @@ export class ReminderService {
|
|||
|
||||
throw new Error('Cannot get related model for reminder');
|
||||
}
|
||||
|
||||
private _markReminderAsProcessed(reminderId: string): void {
|
||||
this._recentlyProcessedReminderIds.add(reminderId);
|
||||
|
||||
// Clear any existing cleanup timeout for this ID
|
||||
const existingTimeout = this._cleanupTimeouts.get(reminderId);
|
||||
if (existingTimeout) {
|
||||
clearTimeout(existingTimeout);
|
||||
}
|
||||
|
||||
// Auto-cleanup after 60 seconds to prevent memory leaks
|
||||
const timeoutId = setTimeout(() => {
|
||||
this._recentlyProcessedReminderIds.delete(reminderId);
|
||||
this._cleanupTimeouts.delete(reminderId);
|
||||
}, 60000);
|
||||
this._cleanupTimeouts.set(reminderId, timeoutId);
|
||||
}
|
||||
|
||||
private _clearProcessedReminder(reminderId: string): void {
|
||||
this._recentlyProcessedReminderIds.delete(reminderId);
|
||||
const existingTimeout = this._cleanupTimeouts.get(reminderId);
|
||||
if (existingTimeout) {
|
||||
clearTimeout(existingTimeout);
|
||||
this._cleanupTimeouts.delete(reminderId);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue