diff --git a/app/jobs/webhook_job.rb b/app/jobs/webhook_job.rb index 57d3739b7..b6004dc89 100644 --- a/app/jobs/webhook_job.rb +++ b/app/jobs/webhook_job.rb @@ -1,5 +1,15 @@ class WebhookJob < ApplicationJob queue_as :medium + + retry_on CustomExceptions::Webhook::RetriableError, wait: :polynomially_longer, attempts: 5 + discard_on CustomExceptions::Webhook::RetriableError do |job, error| + payload = job.arguments[1] + webhook_type = job.arguments[2] || :account_webhook + + Rails.logger.warn "Webhook retries exhausted for #{payload[:event]}: #{error.message}" + Webhooks::ErrorHandler.perform(payload, webhook_type, error) + end + # There are 3 types of webhooks, account, inbox and agent_bot def perform(url, payload, webhook_type = :account_webhook) Webhooks::Trigger.execute(url, payload, webhook_type) diff --git a/lib/custom_exceptions/webhook.rb b/lib/custom_exceptions/webhook.rb new file mode 100644 index 000000000..b97bf319d --- /dev/null +++ b/lib/custom_exceptions/webhook.rb @@ -0,0 +1,11 @@ +module CustomExceptions::Webhook # rubocop:disable Style/ClassAndModuleChildren + class RetriableError < CustomExceptions::Base + def initialize(message) + super(message: message) + end + + def message + @data[:message] + end + end +end diff --git a/lib/webhooks/error_handler.rb b/lib/webhooks/error_handler.rb new file mode 100644 index 000000000..cfcb03cfb --- /dev/null +++ b/lib/webhooks/error_handler.rb @@ -0,0 +1,58 @@ +class Webhooks::ErrorHandler + SUPPORTED_EVENTS = %w[message_created message_updated].freeze + + def initialize(payload, webhook_type, error) + @payload = payload + @webhook_type = webhook_type + @error = error + end + + def self.perform(payload, webhook_type, error) + new(payload, webhook_type, error).perform + end + + def perform + return unless SUPPORTED_EVENTS.include?(@payload[:event]) + return unless message + + case @webhook_type + when :agent_bot_webhook + handle_agent_bot_error + when :api_inbox_webhook + handle_api_inbox_error + end + end + + private + + def handle_agent_bot_error + conversation = message.conversation + return unless conversation&.pending? + + conversation.open! + Conversations::ActivityMessageJob.perform_later(conversation, activity_message_params(conversation)) + end + + def handle_api_inbox_error + Messages::StatusUpdateService.new(message, 'failed', @error.message).perform + end + + def activity_message_params(conversation) + { + account_id: conversation.account_id, + inbox_id: conversation.inbox_id, + message_type: :activity, + content: I18n.t('conversations.activity.agent_bot.error_moved_to_open') + } + end + + def message + return if message_id.blank? + + @message ||= Message.find_by(id: message_id) + end + + def message_id + @payload[:id] + end +end diff --git a/lib/webhooks/trigger.rb b/lib/webhooks/trigger.rb index 54bd7499d..3e2dac455 100644 --- a/lib/webhooks/trigger.rb +++ b/lib/webhooks/trigger.rb @@ -1,6 +1,4 @@ class Webhooks::Trigger - SUPPORTED_ERROR_HANDLE_EVENTS = %w[message_created message_updated].freeze - def initialize(url, payload, webhook_type) @url = url @payload = payload @@ -13,8 +11,11 @@ class Webhooks::Trigger def execute perform_request + rescue RestClient::NotFound + Rails.logger.warn "Webhook returned 404: #{@url}" + raise CustomExceptions::Webhook::RetriableError, "Webhook endpoint not found: #{@url}" rescue StandardError => e - handle_error(e) + Webhooks::ErrorHandler.perform(@payload, @webhook_type, e) Rails.logger.warn "Exception: Invalid webhook URL #{@url} : #{e.message}" end @@ -30,50 +31,6 @@ class Webhooks::Trigger ) end - def handle_error(error) - return unless SUPPORTED_ERROR_HANDLE_EVENTS.include?(@payload[:event]) - return unless message - - case @webhook_type - when :agent_bot_webhook - conversation = message.conversation - return unless conversation&.pending? - - conversation.open! - create_agent_bot_error_activity(conversation) - when :api_inbox_webhook - update_message_status(error) - end - end - - def create_agent_bot_error_activity(conversation) - content = I18n.t('conversations.activity.agent_bot.error_moved_to_open') - Conversations::ActivityMessageJob.perform_later(conversation, activity_message_params(conversation, content)) - end - - def activity_message_params(conversation, content) - { - account_id: conversation.account_id, - inbox_id: conversation.inbox_id, - message_type: :activity, - content: content - } - end - - def update_message_status(error) - Messages::StatusUpdateService.new(message, 'failed', error.message).perform - end - - def message - return if message_id.blank? - - @message ||= Message.find_by(id: message_id) - end - - def message_id - @payload[:id] - end - def webhook_timeout raw_timeout = GlobalConfig.get_value('WEBHOOK_TIMEOUT') timeout = raw_timeout.presence&.to_i diff --git a/spec/jobs/webhook_job_spec.rb b/spec/jobs/webhook_job_spec.rb index 81802a3c0..66935cb21 100644 --- a/spec/jobs/webhook_job_spec.rb +++ b/spec/jobs/webhook_job_spec.rb @@ -28,4 +28,137 @@ RSpec.describe WebhookJob do perform_enqueued_jobs { job } end end + + describe 'retry behavior for 404 errors' do + let!(:account) { create(:account) } + let!(:inbox) { create(:inbox, account: account) } + let!(:conversation) { create(:conversation, inbox: inbox) } + let!(:message) { create(:message, account: account, inbox: inbox, conversation: conversation) } + let(:payload) { { event: 'message_created', id: message.id } } + let(:webhook_type) { :api_inbox_webhook } + + it 'is configured to retry on CustomExceptions::Webhook::RetriableError' do + retry_handler = described_class.rescue_handlers.find do |handler| + handler[0] == 'CustomExceptions::Webhook::RetriableError' + end + + expect(retry_handler).to be_present + end + + context 'when webhook type is api_inbox_webhook' do + let(:webhook_type) { :api_inbox_webhook } + + it 'marks message as failed after retries are exhausted' do + allow(Webhooks::Trigger).to receive(:execute).and_raise( + CustomExceptions::Webhook::RetriableError.new('Webhook endpoint not found') + ) + + perform_enqueued_jobs { job } + + expect(message.reload.status).to eq('failed') + end + end + + context 'when webhook type is agent_bot_webhook' do + let(:webhook_type) { :agent_bot_webhook } + + before { conversation.update!(status: :pending) } + + it 'opens conversation and creates activity message after retries are exhausted' do + allow(Webhooks::Trigger).to receive(:execute).and_raise( + CustomExceptions::Webhook::RetriableError.new('Webhook endpoint not found') + ) + + perform_enqueued_jobs { job } + + expect(conversation.reload.status).to eq('open') + activity_message = conversation.messages.where(message_type: :activity).last + expect(activity_message.content).to eq(I18n.t('conversations.activity.agent_bot.error_moved_to_open')) + end + + it 'does not change message status' do + allow(Webhooks::Trigger).to receive(:execute).and_raise( + CustomExceptions::Webhook::RetriableError.new('Webhook endpoint not found') + ) + + expect { perform_enqueued_jobs { job } }.not_to(change { message.reload.status }) + end + + context 'when conversation is not pending' do + before { conversation.update!(status: :open) } + + it 'does not create activity message' do + allow(Webhooks::Trigger).to receive(:execute).and_raise( + CustomExceptions::Webhook::RetriableError.new('Webhook endpoint not found') + ) + + expect(Conversations::ActivityMessageJob).not_to receive(:perform_later) + + perform_enqueued_jobs { job } + end + end + end + + context 'when webhook type is account_webhook' do + let(:webhook_type) { :account_webhook } + + it 'does not update message status' do + allow(Webhooks::Trigger).to receive(:execute).and_raise( + CustomExceptions::Webhook::RetriableError.new('Webhook endpoint not found') + ) + + expect { perform_enqueued_jobs { job } }.not_to(change { message.reload.status }) + end + + it 'does not create activity message' do + allow(Webhooks::Trigger).to receive(:execute).and_raise( + CustomExceptions::Webhook::RetriableError.new('Webhook endpoint not found') + ) + + expect(Conversations::ActivityMessageJob).not_to receive(:perform_later) + + perform_enqueued_jobs { job } + end + end + + context 'when event is not message_created or message_updated' do + let(:payload) { { event: 'conversation_created', id: conversation.id } } + + it 'does not update any message status' do + allow(Webhooks::Trigger).to receive(:execute).and_raise( + CustomExceptions::Webhook::RetriableError.new('Webhook endpoint not found') + ) + + expect(Messages::StatusUpdateService).not_to receive(:new) + + perform_enqueued_jobs { job } + end + end + + context 'when payload has no id' do + let(:payload) { { event: 'message_created' } } + + it 'does not attempt to update message status' do + allow(Webhooks::Trigger).to receive(:execute).and_raise( + CustomExceptions::Webhook::RetriableError.new('Webhook endpoint not found') + ) + + expect(Messages::StatusUpdateService).not_to receive(:new) + + perform_enqueued_jobs { job } + end + end + + context 'when message does not exist' do + let(:payload) { { event: 'message_created', id: -1 } } + + it 'handles gracefully without raising' do + allow(Webhooks::Trigger).to receive(:execute).and_raise( + CustomExceptions::Webhook::RetriableError.new('Webhook endpoint not found') + ) + + expect { perform_enqueued_jobs { job } }.not_to raise_error + end + end + end end diff --git a/spec/lib/webhooks/trigger_spec.rb b/spec/lib/webhooks/trigger_spec.rb index c2593e30b..8d345abfc 100644 --- a/spec/lib/webhooks/trigger_spec.rb +++ b/spec/lib/webhooks/trigger_spec.rb @@ -174,4 +174,38 @@ describe Webhooks::Trigger do trigger.execute(url, payload, webhook_type) end end + + context 'when webhook returns 404' do + it 'raises CustomExceptions::Webhook::RetriableError' do + payload = { hello: :hello } + + expect(RestClient::Request).to receive(:execute) + .with( + method: :post, + url: url, + payload: payload.to_json, + headers: { content_type: :json, accept: :json }, + timeout: webhook_timeout + ).and_raise(RestClient::NotFound.new) + + expect { trigger.execute(url, payload, webhook_type) }.to raise_error(CustomExceptions::Webhook::RetriableError) + end + + it 'does not call handle_error for 404 responses' do + payload = { event: 'message_created', conversation: { id: conversation.id }, id: message.id } + + expect(RestClient::Request).to receive(:execute) + .with( + method: :post, + url: url, + payload: payload.to_json, + headers: { content_type: :json, accept: :json }, + timeout: webhook_timeout + ).and_raise(RestClient::NotFound.new) + + expect(Messages::StatusUpdateService).not_to receive(:new) + expect { trigger.execute(url, payload, webhook_type) }.to raise_error(CustomExceptions::Webhook::RetriableError) + expect(message.reload.status).to eq('sent') + end + end end