From de6717e7f187a8ca1fd4f53f363f22938b89abc0 Mon Sep 17 00:00:00 2001 From: Tanmay Deep Sharma <32020192+tds-1@users.noreply.github.com> Date: Wed, 2 Jul 2025 11:53:03 +0530 Subject: [PATCH] fix: Add wait for image response builder (#11837) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Linear https://linear.app/chatwoot/issue/CW-4559/faradaybadrequesterror-the-server-responded-with-status-400#comment-e827f872 ## Description This PR adds retry logic and wait mechanisms for image processing in the Captain conversation response builder. The fix addresses issues where image attachments might not be immediately available when the response builder tries to process multimodal message content, causing failures in the AI assistant's response generation. ## Type of change Please delete options that are not relevant. - [x] Bug fix (non-breaking change which fixes an issue) ## How Has This Been Tested? - Captain assistant responses with image attachments - Large image processing scenarios - Network timeout conditions during image loading - Multiple retry attempts for failed image access - Conversation flow continuation after image processing failures Screenshot 2025-06-30 at 1 58 26 PM ## Checklist: - [x] My code follows the style guidelines of this project - [x] I have performed a self-review of my code - [x] I have commented on my code, particularly in hard-to-understand areas - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [x] Any dependent changes have been merged and published in downstream modules --------- Co-authored-by: Muhsin Keloth --- .../conversation/response_builder_job.rb | 9 +- .../open_ai_message_builder_service.rb | 1 + .../hook_execution_service.rb | 27 ++++- .../conversation/response_builder_job_spec.rb | 112 ++++++++++++++++++ .../open_ai_message_builder_service_spec.rb | 1 + 5 files changed, 141 insertions(+), 9 deletions(-) diff --git a/enterprise/app/jobs/captain/conversation/response_builder_job.rb b/enterprise/app/jobs/captain/conversation/response_builder_job.rb index 431945896..b207bd2a4 100644 --- a/enterprise/app/jobs/captain/conversation/response_builder_job.rb +++ b/enterprise/app/jobs/captain/conversation/response_builder_job.rb @@ -1,6 +1,7 @@ class Captain::Conversation::ResponseBuilderJob < ApplicationJob MAX_MESSAGE_LENGTH = 10_000 - retry_on ActiveStorage::FileNotFoundError, attempts: 3 + retry_on ActiveStorage::FileNotFoundError, attempts: 3, wait: 2.seconds + retry_on Faraday::BadRequestError, attempts: 3, wait: 2.seconds def perform(conversation, assistant) @conversation = conversation @@ -13,7 +14,7 @@ class Captain::Conversation::ResponseBuilderJob < ApplicationJob generate_and_process_response end rescue StandardError => e - raise e if e.is_a?(ActiveStorage::FileNotFoundError) + raise e if e.is_a?(ActiveStorage::FileNotFoundError) || e.is_a?(Faraday::BadRequestError) handle_error(e) ensure @@ -50,9 +51,7 @@ class Captain::Conversation::ResponseBuilderJob < ApplicationJob end def determine_role(message) - return 'system' if message.content.blank? - - message.message_type == 'incoming' ? 'user' : 'system' + message.message_type == 'incoming' ? 'user' : 'assistant' end def prepare_multimodal_message_content(message) diff --git a/enterprise/app/services/captain/open_ai_message_builder_service.rb b/enterprise/app/services/captain/open_ai_message_builder_service.rb index 3320ad537..43d2851c9 100644 --- a/enterprise/app/services/captain/open_ai_message_builder_service.rb +++ b/enterprise/app/services/captain/open_ai_message_builder_service.rb @@ -42,6 +42,7 @@ class Captain::OpenAiMessageBuilderService end def get_attachment_url(attachment) + return attachment.download_url if attachment.download_url.present? return attachment.external_url if attachment.external_url.present? attachment.file.attached? ? attachment.file_url : nil diff --git a/enterprise/app/services/enterprise/message_templates/hook_execution_service.rb b/enterprise/app/services/enterprise/message_templates/hook_execution_service.rb index 92ccba553..e8faaf74a 100644 --- a/enterprise/app/services/enterprise/message_templates/hook_execution_service.rb +++ b/enterprise/app/services/enterprise/message_templates/hook_execution_service.rb @@ -4,10 +4,29 @@ module Enterprise::MessageTemplates::HookExecutionService return unless should_process_captain_response? return perform_handoff unless inbox.captain_active? - Captain::Conversation::ResponseBuilderJob.perform_later( - conversation, - conversation.inbox.captain_assistant - ) + schedule_captain_response + end + + private + + def schedule_captain_response + job_args = [conversation, conversation.inbox.captain_assistant] + + if message.attachments.blank? + Captain::Conversation::ResponseBuilderJob.perform_later(*job_args) + else + wait_time = calculate_attachment_wait_time + Captain::Conversation::ResponseBuilderJob.set(wait: wait_time).perform_later(*job_args) + end + end + + def calculate_attachment_wait_time + attachment_count = message.attachments.size + base_wait = 1.second + + # Wait longer for more attachments or larger files + additional_wait = [attachment_count * 1, 4].min.seconds + base_wait + additional_wait end def should_process_captain_response? diff --git a/spec/enterprise/jobs/captain/conversation/response_builder_job_spec.rb b/spec/enterprise/jobs/captain/conversation/response_builder_job_spec.rb index ca8d4a6c0..c21205d52 100644 --- a/spec/enterprise/jobs/captain/conversation/response_builder_job_spec.rb +++ b/spec/enterprise/jobs/captain/conversation/response_builder_job_spec.rb @@ -56,4 +56,116 @@ RSpec.describe Captain::Conversation::ResponseBuilderJob, type: :job do end end end + + describe 'retry mechanisms for image processing' do + let(:conversation) { create(:conversation, inbox: inbox, account: account) } + let(:mock_llm_chat_service) { instance_double(Captain::Llm::AssistantChatService) } + let(:mock_message_builder) { instance_double(Captain::OpenAiMessageBuilderService) } + + before do + create(:message, conversation: conversation, content: 'Hello with image', message_type: :incoming) + allow(Captain::Llm::AssistantChatService).to receive(:new).and_return(mock_llm_chat_service) + allow(Captain::OpenAiMessageBuilderService).to receive(:new).with(message: anything).and_return(mock_message_builder) + allow(mock_message_builder).to receive(:generate_content).and_return('Hello with image') + allow(mock_llm_chat_service).to receive(:generate_response).and_return({ 'response' => 'Test response' }) + end + + context 'when ActiveStorage::FileNotFoundError occurs' do + it 'handles file errors and triggers handoff' do + allow(mock_message_builder).to receive(:generate_content) + .and_raise(ActiveStorage::FileNotFoundError, 'Image file not found') + + # For retryable errors, the job should handle them and proceed with handoff + described_class.perform_now(conversation, assistant) + + # Verify handoff occurred due to repeated failures + expect(conversation.reload.status).to eq('open') + end + + it 'succeeds when no error occurs' do + # Don't raise any error, should succeed normally + allow(mock_message_builder).to receive(:generate_content) + .and_return('Image content processed successfully') + + described_class.perform_now(conversation, assistant) + + expect(conversation.messages.outgoing.count).to eq(1) + expect(conversation.messages.outgoing.last.content).to eq('Test response') + end + end + + context 'when Faraday::BadRequestError occurs' do + it 'handles API errors and triggers handoff' do + allow(mock_llm_chat_service).to receive(:generate_response) + .and_raise(Faraday::BadRequestError, 'Bad request to image service') + + described_class.perform_now(conversation, assistant) + expect(conversation.reload.status).to eq('open') + end + + it 'succeeds when no error occurs' do + # Don't raise any error, should succeed normally + allow(mock_llm_chat_service).to receive(:generate_response) + .and_return({ 'response' => 'Response after retry' }) + + described_class.perform_now(conversation, assistant) + + expect(conversation.messages.outgoing.last.content).to eq('Response after retry') + end + end + + context 'when image processing fails permanently' do + before do + allow(mock_message_builder).to receive(:generate_content) + .and_raise(ActiveStorage::FileNotFoundError, 'Image permanently unavailable') + end + + it 'triggers handoff after max retries' do + # Since perform_now re-raises retryable errors, simulate the final failure after retries + allow(mock_message_builder).to receive(:generate_content) + .and_raise(StandardError, 'Max retries exceeded') + + expect(ChatwootExceptionTracker).to receive(:new).and_call_original + + described_class.perform_now(conversation, assistant) + + expect(conversation.reload.status).to eq('open') + end + end + + context 'when non-retryable error occurs' do + let(:standard_error) { StandardError.new('Generic error') } + + before do + allow(mock_llm_chat_service).to receive(:generate_response).and_raise(standard_error) + end + + it 'handles error and triggers handoff' do + expect(ChatwootExceptionTracker).to receive(:new) + .with(standard_error, account: account) + .and_call_original + + described_class.perform_now(conversation, assistant) + + expect(conversation.reload.status).to eq('open') + end + + it 'ensures Current.executed_by is reset' do + expect(Current).to receive(:executed_by=).with(assistant) + expect(Current).to receive(:executed_by=).with(nil) + + described_class.perform_now(conversation, assistant) + end + end + end + + describe 'job configuration' do + it 'has retry_on configuration for retryable errors' do + expect(described_class).to respond_to(:retry_on) + end + + it 'defines MAX_MESSAGE_LENGTH constant' do + expect(described_class::MAX_MESSAGE_LENGTH).to eq(10_000) + end + end end diff --git a/spec/enterprise/services/captain/open_ai_message_builder_service_spec.rb b/spec/enterprise/services/captain/open_ai_message_builder_service_spec.rb index 13c29f756..76e91ae7a 100644 --- a/spec/enterprise/services/captain/open_ai_message_builder_service_spec.rb +++ b/spec/enterprise/services/captain/open_ai_message_builder_service_spec.rb @@ -207,6 +207,7 @@ RSpec.describe Captain::OpenAiMessageBuilderService do attachment.update(external_url: nil) allow(attachment).to receive(:file).and_return(instance_double(ActiveStorage::Attached::One, attached?: true)) allow(attachment).to receive(:file_url).and_return('https://local.com/file.jpg') + allow(attachment).to receive(:download_url).and_return('') end it 'returns file_url' do