From 661e905dbd78101acdadb57bdad16ec1f1558501 Mon Sep 17 00:00:00 2001 From: Pranav Date: Mon, 4 Aug 2025 19:08:45 -0700 Subject: [PATCH] fix: Skip HookJob for inactive or irrelevant hooks (#12093) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On Aug 2, we had a P0 because of a sudden spike in Sidekiq jobs. The queue went up to 100k jobs and workers scaled from 400 threads to 1000+. Most of the jobs were HookJobs, and a large chunk of them were for Linear but they weren’t doing anything useful. Turns out, whenever there’s an update on a contact or conversation, we were triggering all account-level hooks without checking if the event was relevant. So if someone did a bulk import or ran an update, it would enqueue a huge number of unnecessary jobs. This PR adds two checks before enqueuing: - Whether the hook is active - Whether the event is relevant for that hook --- app/listeners/hook_listener.rb | 20 ++++++- spec/listeners/hook_listener_spec.rb | 82 +++++++++++++++++++++++++++- 2 files changed, 100 insertions(+), 2 deletions(-) diff --git a/app/listeners/hook_listener.rb b/app/listeners/hook_listener.rb index 3360e23da..936d104a0 100644 --- a/app/listeners/hook_listener.rb +++ b/app/listeners/hook_listener.rb @@ -37,10 +37,11 @@ class HookListener < BaseListener private def execute_hooks(event, message) - message.account.hooks.each do |hook| + message.account.hooks.find_each do |hook| # In case of dialogflow, we would have a hook for each inbox. # Which means we will execute the same hook multiple times if the below filter isn't there next if hook.inbox.present? && hook.inbox != message.inbox + next unless supported_hook_event?(hook, event.name) HookJob.perform_later(hook, event.name, message: message) end @@ -48,7 +49,24 @@ class HookListener < BaseListener def execute_account_hooks(event, account, event_data = {}) account.hooks.account_hooks.find_each do |hook| + next unless supported_hook_event?(hook, event.name) + HookJob.perform_later(hook, event.name, event_data) end end + + def supported_hook_event?(hook, event_name) + return false if hook.disabled? + + supported_events_map = { + 'slack' => ['message.created'], + 'dialogflow' => ['message.created', 'message.updated'], + 'google_translate' => ['message.created'], + 'leadsquared' => ['contact.updated', 'conversation.created', 'conversation.resolved'] + } + + return false unless supported_events_map.key?(hook.app_id) + + supported_events_map[hook.app_id].include?(event_name) + end end diff --git a/spec/listeners/hook_listener_spec.rb b/spec/listeners/hook_listener_spec.rb index 4caa4dafd..3ae031237 100644 --- a/spec/listeners/hook_listener_spec.rb +++ b/spec/listeners/hook_listener_spec.rb @@ -10,6 +10,8 @@ describe HookListener do account: account, inbox: inbox, conversation: conversation) end let!(:event) { Events::Base.new(event_name, Time.zone.now, message: message) } + let(:contact_event) { Events::Base.new('contact.updated', Time.zone.now, contact: conversation.contact) } + let(:conversation_event) { Events::Base.new('conversation.created', Time.zone.now, conversation: conversation) } describe '#message_created' do let(:event_name) { 'message.created' } @@ -42,10 +44,88 @@ describe HookListener do context 'when hook is configured' do it 'triggers hook job' do - hook = create(:integrations_hook, account: account) + hook = create(:integrations_hook, :dialogflow, account: account, inbox: inbox) expect(HookJob).to receive(:perform_later).with(hook, 'message.updated', message: message).once listener.message_updated(event) end end end + + describe 'hook job enqueuing behavior' do + let(:event_name) { 'message.created' } + + context 'when app_id is not in the allowed list' do + it 'does not enqueue the job' do + create(:integrations_hook, account: account, app_id: 'unsupported_app') + expect(HookJob).not_to receive(:perform_later) + + listener.message_created(event) + end + end + + context 'when hook is enabled and app_id is supported' do + it 'enqueues the job for slack' do + hook = create(:integrations_hook, account: account) + expect(HookJob).to receive(:perform_later).with(hook, event_name, message: message) + + listener.message_created(event) + end + + it 'enqueues the job for dialogflow' do + hook = create(:integrations_hook, :dialogflow, account: account, inbox: inbox) + expect(HookJob).to receive(:perform_later).with(hook, event_name, message: message) + + listener.message_created(event) + end + + it 'enqueues the job for google_translate' do + hook = create(:integrations_hook, :google_translate, account: account) + expect(HookJob).to receive(:perform_later).with(hook, event_name, message: message) + + listener.message_created(event) + end + end + + context 'with disabled hook' do + it 'does not enqueue job for disabled hooks' do + create(:integrations_hook, account: account, status: 'disabled', app_id: 'slack') + expect(HookJob).not_to receive(:perform_later) + + listener.message_created(event) + end + end + + context 'with unsupported app_id and event combination' do + it 'does not enqueue job for unsupported app_id' do + create(:integrations_hook, account: account, app_id: 'unsupported_app') + expect(HookJob).not_to receive(:perform_later) + + listener.message_created(event) + end + end + + context 'with leadsquared hook' do + let(:hook) { create(:integrations_hook, :leadsquared, account: account) } + + before do + account.enable_features(:crm_integration) + end + + it 'enqueues the job for conversation.created' do + expect(HookJob) + .to receive(:perform_later) + .with(hook, 'conversation.created', { conversation: conversation }) + + listener.conversation_created(conversation_event) + end + + it 'enqueues the job for contact.updated' do + expect(HookJob) + .to receive(:perform_later) + .with(hook, 'contact.updated', { contact: conversation.contact }) + + listener.contact_updated(contact_event) + end + end + end end