From b870a48734c336a64eeb29de399a8521399f32a2 Mon Sep 17 00:00:00 2001 From: Tanmay Deep Sharma <32020192+tds-1@users.noreply.github.com> Date: Wed, 28 Jan 2026 17:35:13 +0530 Subject: [PATCH] perf: limit the number of notifications per user to 300 (#13234) ## Linear issue https://linear.app/chatwoot/issue/CW-6289/limit-the-number-of-notifications-per-user-to-300 ## Description Limits the number of notifications per user to 300 by introducing an async trim job that runs after each notification creation. This prevents unbounded notification growth that was causing DB CPU spikes. ## Type of change - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] This change requires a documentation update ## How Has This Been Tested? - Added unit tests for TrimUserNotificationsJob ## Checklist: - [ ] My code follows the style guidelines of this project - [ ] I have performed a self-review of my code - [ ] I have commented on my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes - [ ] Any dependent changes have been merged and published in downstream modules --- > [!NOTE] > Implements a dedicated purge job to control notification volume and scheduling. > > - Introduces `Notification::RemoveOldNotificationJob` (queue: `purgable`) to delete notifications older than 1 month and trim each user to the 300 most recent (deterministic by `created_at DESC, id DESC`) > - Adds daily cron (`remove_old_notification_job` at 22:30 UTC, queue `purgable`) in `config/schedule.yml` > - Removes ad-hoc triggering of the purge from `TriggerScheduledItemsJob` > - Adds/updates specs covering enqueue queue, old-notification deletion, per-user trimming, and combined behavior > > Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 9ea2b48e36df96cd15d4119d1dd7dcf5250695de. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot). --------- Co-authored-by: Vishnu Narayanan Co-authored-by: Shivam Mishra --- .../remove_old_notification_job.rb | 52 +++++++++++++++- app/jobs/trigger_scheduled_items_job.rb | 3 - config/schedule.yml | 7 +++ .../remove_old_notification_job_spec.rb | 62 ++++++++++++++++--- spec/jobs/trigger_scheduled_items_job_spec.rb | 5 -- 5 files changed, 108 insertions(+), 21 deletions(-) diff --git a/app/jobs/notification/remove_old_notification_job.rb b/app/jobs/notification/remove_old_notification_job.rb index 770461744..d7ec9d42f 100644 --- a/app/jobs/notification/remove_old_notification_job.rb +++ b/app/jobs/notification/remove_old_notification_job.rb @@ -1,8 +1,54 @@ class Notification::RemoveOldNotificationJob < ApplicationJob - queue_as :low + queue_as :purgable + + NOTIFICATION_LIMIT = 300 + OLD_NOTIFICATION_THRESHOLD = 1.month def perform - Notification.where('created_at < ?', 1.month.ago) - .find_each(batch_size: 1000, &:delete) + remove_old_notifications + trim_user_notifications + end + + private + + def remove_old_notifications + Notification.where('created_at < ?', OLD_NOTIFICATION_THRESHOLD.ago) + .delete_all + end + + def trim_user_notifications + # Find users with more than NOTIFICATION_LIMIT notifications + user_ids_exceeding_limit.each do |user_id| + trim_notifications_for_user(user_id) + end + end + + def user_ids_exceeding_limit + Notification.group(:user_id) + .having('COUNT(*) > ?', NOTIFICATION_LIMIT) + .pluck(:user_id) + end + + def trim_notifications_for_user(user_id) + # Find the cutoff notification (the 301st when we want to keep top 300) + # Order by created_at DESC, then id DESC for deterministic ordering + cutoff = Notification.where(user_id: user_id) + .order(created_at: :desc, id: :desc) + .offset(NOTIFICATION_LIMIT) + .limit(1) + .pick(:created_at, :id) + + return unless cutoff + + cutoff_time, cutoff_id = cutoff + + # Delete notifications older than cutoff, or same timestamp but lower/equal ID + # Since we order by id DESC, higher IDs are kept (come first), lower IDs deleted + # This avoids race conditions: notifications created after finding the cutoff + # will have timestamps > cutoff_time and won't be incorrectly deleted + Notification.where(user_id: user_id) + .where('created_at < ? OR (created_at = ? AND id <= ?)', + cutoff_time, cutoff_time, cutoff_id) + .delete_all end end diff --git a/app/jobs/trigger_scheduled_items_job.rb b/app/jobs/trigger_scheduled_items_job.rb index 15b0011c8..0ec368c54 100644 --- a/app/jobs/trigger_scheduled_items_job.rb +++ b/app/jobs/trigger_scheduled_items_job.rb @@ -19,9 +19,6 @@ class TriggerScheduledItemsJob < ApplicationJob # Job to sync whatsapp templates Channels::Whatsapp::TemplatesSyncSchedulerJob.perform_later - - # Job to clear notifications which are older than 1 month - Notification::RemoveOldNotificationJob.perform_later end end diff --git a/config/schedule.yml b/config/schedule.yml index c039719fa..cae9b94c8 100644 --- a/config/schedule.yml +++ b/config/schedule.yml @@ -59,3 +59,10 @@ periodic_assignment_job: cron: '*/30 * * * *' class: 'AutoAssignment::PeriodicAssignmentJob' queue: scheduled_jobs + +# executed daily at 2230 UTC +# removes old notifications (>1 month) and trims to 300 per user +remove_old_notification_job: + cron: '30 22 * * *' + class: 'Notification::RemoveOldNotificationJob' + queue: purgable diff --git a/spec/jobs/notification/remove_old_notification_job_spec.rb b/spec/jobs/notification/remove_old_notification_job_spec.rb index 1605d2da6..be3f8d035 100644 --- a/spec/jobs/notification/remove_old_notification_job_spec.rb +++ b/spec/jobs/notification/remove_old_notification_job_spec.rb @@ -1,23 +1,65 @@ require 'rails_helper' RSpec.describe Notification::RemoveOldNotificationJob do - let(:user) { create(:user) } - let(:conversation) { create(:conversation) } + let(:account) { create(:account) } + let(:user) { create(:user, account: account) } + let(:conversation) { create(:conversation, account: account) } it 'enqueues the job' do expect do described_class.perform_later end.to have_enqueued_job(described_class) - .on_queue('low') + .on_queue('purgable') end - it 'removes old notifications which are older than 1 month' do - create(:notification, user: user, notification_type: 'conversation_creation', primary_actor: conversation, created_at: 2.months.ago) - create(:notification, user: user, notification_type: 'conversation_creation', primary_actor: conversation, created_at: 1.month.ago) - create(:notification, user: user, notification_type: 'conversation_creation', primary_actor: conversation, created_at: 1.day.ago) - create(:notification, user: user, notification_type: 'conversation_creation', primary_actor: conversation, created_at: 1.hour.ago) + describe 'removing old notifications' do + it 'removes notifications older than 1 month' do + create(:notification, user: user, notification_type: 'conversation_creation', primary_actor: conversation, + created_at: 2.months.ago) + create(:notification, user: user, notification_type: 'conversation_creation', primary_actor: conversation, + created_at: 1.month.ago) + create(:notification, user: user, notification_type: 'conversation_creation', primary_actor: conversation, + created_at: 1.day.ago) + create(:notification, user: user, notification_type: 'conversation_creation', primary_actor: conversation, + created_at: 1.hour.ago) - described_class.perform_now - expect(Notification.count).to eq(2) + described_class.perform_now + expect(Notification.count).to eq(2) + end + end + + describe 'trimming user notifications' do + it 'does not delete notifications when user has fewer than 300' do + create_list(:notification, 50, user: user, account: account, primary_actor: conversation) + + expect { described_class.perform_now }.not_to(change(Notification, :count)) + end + + it 'trims to 300 notifications per user keeping the most recent' do + old_notifications = create_list(:notification, 50, user: user, account: account, primary_actor: conversation, + created_at: 2.days.ago) + recent_notifications = create_list(:notification, 300, user: user, account: account, primary_actor: conversation, + created_at: 1.hour.ago) + + described_class.perform_now + + expect(Notification.where(user_id: user.id).count).to eq(300) + expect(Notification.where(id: old_notifications.map(&:id))).to be_empty + expect(Notification.where(id: recent_notifications.map(&:id)).count).to eq(300) + end + end + + describe 'combined functionality' do + it 'removes old notifications and trims user notifications in one job' do + # User with old and excess notifications + create_list(:notification, 100, user: user, account: account, primary_actor: conversation, created_at: 2.months.ago) + create_list(:notification, 250, user: user, account: account, primary_actor: conversation, created_at: 1.day.ago) + + described_class.perform_now + + # All old notifications removed, remaining trimmed to 300 + expect(Notification.where(user_id: user.id).count).to eq(250) + expect(Notification.where('created_at < ?', 1.month.ago)).to be_empty + end end end diff --git a/spec/jobs/trigger_scheduled_items_job_spec.rb b/spec/jobs/trigger_scheduled_items_job_spec.rb index 4409e7ee0..6ccb41916 100644 --- a/spec/jobs/trigger_scheduled_items_job_spec.rb +++ b/spec/jobs/trigger_scheduled_items_job_spec.rb @@ -30,11 +30,6 @@ RSpec.describe TriggerScheduledItemsJob do described_class.perform_now end - it 'triggers Notification::RemoveOldNotificationJob' do - expect(Notification::RemoveOldNotificationJob).to receive(:perform_later).once - described_class.perform_now - end - context 'when unexecuted Scheduled campaign jobs' do let!(:twilio_sms) { create(:channel_twilio_sms, account: account) } let!(:twilio_inbox) { create(:inbox, channel: twilio_sms, account: account) }