fix: Add wait for image response builder (#11837)
## 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 <img width="1231" alt="Screenshot 2025-06-30 at 1 58 26 PM" src="https://github.com/user-attachments/assets/41723b83-e0f0-43d6-899f-7028cf2e5b94" /> ## 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 <muhsinkeramam@gmail.com>
This commit is contained in:
co-authored by
Muhsin Keloth
parent
7d39f890e4
commit
de6717e7f1
@@ -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)
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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?
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user