From 822a39043a5773ac7e22293af87d16b9a38bf38c Mon Sep 17 00:00:00 2001 From: fdegier Date: Mon, 13 Oct 2025 15:48:16 +0200 Subject: [PATCH] Catch branch creation errors Changelog: fixed EE: true --- .../ci/workloads/run_workload_service.rb | 21 ++++- .../ai/components/duo_workflow_action.vue | 4 + .../duo_workflows/start_workflow_service.rb | 2 +- .../ai/components/duo_workflow_action_spec.js | 93 +++++++++++++++---- .../api/ai/duo_workflows/workflows_spec.rb | 27 ++++++ .../ci/workloads/run_workload_service_spec.rb | 51 ++++++++++ 6 files changed, 173 insertions(+), 25 deletions(-) diff --git a/app/services/ci/workloads/run_workload_service.rb b/app/services/ci/workloads/run_workload_service.rb index 8acad44b0b2284..fda3e812ee5de0 100644 --- a/app/services/ci/workloads/run_workload_service.rb +++ b/app/services/ci/workloads/run_workload_service.rb @@ -25,7 +25,15 @@ def initialize( def execute validate_source! - @ref = @create_branch ? create_repository_branch(@source_branch) : default_branch + + if @create_branch + branch_response = create_repository_branch(@source_branch) + return branch_response unless branch_response.success? + + @ref = branch_response.payload[:branch_name] + else + @ref = default_branch + end @workload_definition.add_variable(:CI_WORKLOAD_REF, @ref) service = ::Ci::CreatePipelineService.new(@project, @current_user, ref: @ref) @@ -66,14 +74,19 @@ def create_included_ci_variables(workload) def create_repository_branch(source_branch) branch_name = "workloads/#{SecureRandom.hex[0..10]}" - raise "Branch already exists" if @project.repository.branch_exists?(branch_name) + if @project.repository.branch_exists?(branch_name) + return ServiceResponse.error(message: 'Branch already exists') + end branch = @project.repository.branch_exists?(source_branch) ? source_branch : default_branch repo_branch = @project.repository.add_branch(@current_user, branch_name, branch, skip_ci: true) - raise "Error in git branch creation" unless repo_branch + return ServiceResponse.error(message: 'Error in git branch creation') unless repo_branch - branch_name + ServiceResponse.success(payload: { branch_name: branch_name }) + rescue Gitlab::Git::CommandError => e + Gitlab::ErrorTracking.track_exception(e) + ServiceResponse.error(message: 'Failed to create branch') end def ci_job_yaml diff --git a/ee/app/assets/javascripts/ai/components/duo_workflow_action.vue b/ee/app/assets/javascripts/ai/components/duo_workflow_action.vue index 5ac56d19856e1b..42061e0a6f179b 100644 --- a/ee/app/assets/javascripts/ai/components/duo_workflow_action.vue +++ b/ee/app/assets/javascripts/ai/components/duo_workflow_action.vue @@ -175,6 +175,10 @@ export default { axios .post(this.duoWorkflowInvokePath, requestData) .then(({ data }) => { + if (data.workload && data.workload.id === null && data.workload.message) { + throw new Error(data.workload.message); + } + this.$emit('agent-flow-started', data); createAlert(this.successAlert(data.id)); diff --git a/ee/app/services/ai/duo_workflows/start_workflow_service.rb b/ee/app/services/ai/duo_workflows/start_workflow_service.rb index 789f52ced79b7e..12801e840314b7 100644 --- a/ee/app/services/ai/duo_workflows/start_workflow_service.rb +++ b/ee/app/services/ai/duo_workflows/start_workflow_service.rb @@ -44,8 +44,8 @@ def execute ) response = service.execute - workload = response.payload if response.success? + workload = response.payload @workflow.workflows_workloads.create(project_id: project.id, workload_id: workload.id) ServiceResponse.success(payload: { workload_id: workload.id }) else diff --git a/ee/spec/frontend/ai/components/duo_workflow_action_spec.js b/ee/spec/frontend/ai/components/duo_workflow_action_spec.js index 1a53b7d8356b9e..6ccb2479afe9a8 100644 --- a/ee/spec/frontend/ai/components/duo_workflow_action_spec.js +++ b/ee/spec/frontend/ai/components/duo_workflow_action_spec.js @@ -2,6 +2,7 @@ import Vue, { nextTick } from 'vue'; import { shallowMount } from '@vue/test-utils'; import VueApollo from 'vue-apollo'; import { GlButton } from '@gitlab/ui'; +import MockAdapter from 'axios-mock-adapter'; import waitForPromises from 'helpers/wait_for_promises'; import getDuoWorkflowStatusCheck from 'ee/ai/graphql/get_duo_workflow_status_check.query.graphql'; import axios from '~/lib/utils/axios_utils'; @@ -16,6 +17,7 @@ Vue.use(VueApollo); describe('DuoWorkflowAction component', () => { let wrapper; + let mock; const projectId = 1; const duoWorkflowInvokePath = `/api/v4/ai/duo_workflows/workflows`; @@ -98,14 +100,20 @@ describe('DuoWorkflowAction component', () => { const findButton = () => wrapper.findComponent(GlButton); beforeEach(() => { - jest.spyOn(axios, 'post'); + mock = new MockAdapter(axios); window.gon = { api_version: 'v4', }; - axios.post.mockResolvedValue({ data: mockCreateFlowResponse }); + mock.onPost(duoWorkflowInvokePath).reply(() => { + return [200, mockCreateFlowResponse]; + }); mockGetHealthCheckHandler = jest.fn().mockResolvedValue(mockDuoWorkflowStatusCheckEnabled); }); + afterEach(() => { + mock.restore(); + }); + describe('rendering', () => { describe('when duoWorkflowStatusCheck is not enabled', () => { beforeEach(async () => { @@ -217,7 +225,7 @@ describe('DuoWorkflowAction component', () => { describe('when button is clicked', () => { beforeEach(async () => { - axios.post.mockImplementation(() => new Promise(() => {})); // Never resolves + mock.onPost(duoWorkflowInvokePath).reply(() => new Promise(() => {})); // Never resolves findButton().vm.$emit('click'); await nextTick(); @@ -241,9 +249,7 @@ describe('DuoWorkflowAction component', () => { describe('when the flow fails to start', () => { beforeEach(async () => { - const error = new Error('API error'); - - axios.post.mockRejectedValue(error); + mock.onPost(duoWorkflowInvokePath).reply(500); findButton().vm.$emit('click'); await waitForPromises(); @@ -281,12 +287,15 @@ describe('DuoWorkflowAction component', () => { it('emits prompt-validation-error', () => { expect(wrapper.emitted('prompt-validation-error')).toEqual([[invalidGoal]]); - expect(axios.post).not.toHaveBeenCalled(); }); it('does not show loading state for validation errors', () => { expect(findButton().props('loading')).toBe(false); }); + + it('does not make API call', () => { + expect(mock.history.post).toHaveLength(0); + }); }); describe('when the goal matches the promptVaidatorRegex', () => { @@ -301,7 +310,10 @@ describe('DuoWorkflowAction component', () => { it('does not emit prompt-validation-error when goal matches regex', () => { expect(wrapper.emitted('prompt-validation-error')).toBeUndefined(); - expect(axios.post).toHaveBeenCalled(); + }); + + it('makes API call', () => { + expect(mock.history.post).toHaveLength(1); }); }); @@ -312,7 +324,8 @@ describe('DuoWorkflowAction component', () => { }); it('makes API call with correct data', () => { - expect(axios.post).toHaveBeenCalledWith(duoWorkflowInvokePath, expectedRequestData); + const request = mock.history.post[0]; + expect(JSON.parse(request.data)).toEqual(expectedRequestData); }); }); @@ -339,7 +352,8 @@ describe('DuoWorkflowAction component', () => { }); it('includes additional_context array in the request params', () => { - expect(axios.post).toHaveBeenCalledWith(duoWorkflowInvokePath, { + const request = mock.history.post[0]; + expect(JSON.parse(request.data)).toEqual({ ...expectedRequestData, additional_context: additionalContext, }); @@ -357,7 +371,8 @@ describe('DuoWorkflowAction component', () => { }); it('includes empty additional_context array in the request params', () => { - expect(axios.post).toHaveBeenCalledWith(duoWorkflowInvokePath, { + const request = mock.history.post[0]; + expect(JSON.parse(request.data)).toEqual({ ...expectedRequestData, additional_context: [], }); @@ -373,7 +388,8 @@ describe('DuoWorkflowAction component', () => { }); it('includes empty array as additional_context in the request params', () => { - expect(axios.post).toHaveBeenCalledWith(duoWorkflowInvokePath, { + const request = mock.history.post[0]; + expect(JSON.parse(request.data)).toEqual({ ...expectedRequestData, additional_context: [], }); @@ -391,7 +407,8 @@ describe('DuoWorkflowAction component', () => { }); it('includes currentRef as source_branch in the request params', () => { - expect(axios.post).toHaveBeenCalledWith(duoWorkflowInvokePath, { + const request = mock.history.post[0]; + expect(JSON.parse(request.data)).toEqual({ ...expectedRequestData, source_branch: currentRef, }); @@ -407,7 +424,8 @@ describe('DuoWorkflowAction component', () => { }); it('includes sourceBranch as source_branch in the request params', () => { - expect(axios.post).toHaveBeenCalledWith(duoWorkflowInvokePath, { + const request = mock.history.post[0]; + expect(JSON.parse(request.data)).toEqual({ ...expectedRequestData, source_branch: sourceBranch, }); @@ -423,7 +441,8 @@ describe('DuoWorkflowAction component', () => { }); it('prioritizes sourceBranch over currentRef', () => { - expect(axios.post).toHaveBeenCalledWith(duoWorkflowInvokePath, { + const request = mock.history.post[0]; + expect(JSON.parse(request.data)).toEqual({ ...expectedRequestData, source_branch: sourceBranch, }); @@ -439,7 +458,8 @@ describe('DuoWorkflowAction component', () => { }); it('does not include source_branch in the request params', () => { - expect(axios.post).toHaveBeenCalledWith(duoWorkflowInvokePath, expectedRequestData); + const request = mock.history.post[0]; + expect(JSON.parse(request.data)).toEqual(expectedRequestData); }); }); }); @@ -481,11 +501,44 @@ describe('DuoWorkflowAction component', () => { }); }); - describe('when request fails', () => { - const error = new Error('API error'); + describe('when request succeeds but workload fails', () => { + const mockFailedWorkloadResponse = { + id: 563, + project_id: 1, + workload: { + id: null, + message: 'Branch already exists', + }, + }; + + beforeEach(async () => { + mock.onPost(duoWorkflowInvokePath).reply(200, mockFailedWorkloadResponse); + await createComponent(); + findButton().vm.$emit('click'); + await waitForPromises(); + }); + it('shows error alert with generic message', () => { + expect(createAlert).toHaveBeenCalledWith({ + message: 'Error occurred when starting the flow.', + captureError: true, + error: expect.any(Error), + }); + }); + + it('does not emit agent-flow-started event', () => { + expect(wrapper.emitted('agent-flow-started')).toBeUndefined(); + }); + + it('removes loading state after error', () => { + expect(findButton().props('loading')).toBe(false); + }); + }); + + describe('when request fails', () => { beforeEach(async () => { - axios.post.mockRejectedValue(error); + mock.onPost(duoWorkflowInvokePath).reply(500); + await createComponent(); findButton().vm.$emit('click'); await waitForPromises(); }); @@ -494,7 +547,7 @@ describe('DuoWorkflowAction component', () => { expect(createAlert).toHaveBeenCalledWith({ message: 'Error occurred when starting the flow.', captureError: true, - error, + error: expect.any(Error), }); }); diff --git a/ee/spec/requests/api/ai/duo_workflows/workflows_spec.rb b/ee/spec/requests/api/ai/duo_workflows/workflows_spec.rb index 55d1ef4f645a64..d6da8d060ab6da 100644 --- a/ee/spec/requests/api/ai/duo_workflows/workflows_spec.rb +++ b/ee/spec/requests/api/ai/duo_workflows/workflows_spec.rb @@ -403,6 +403,33 @@ end end + context 'when branch creation fails during CI execution' do + let(:params) do + { + project_id: project.id, + start_workflow: true, + goal: 'Print hello world', + source_branch: 'feature-branch' + } + end + + before do + allow_next_instance_of(Ci::Workloads::RunWorkloadService) do |service| + allow(service).to receive(:execute).and_return( + ServiceResponse.error(message: 'Error in git branch creation') + ) + end + end + + it 'returns error message about branch creation failure' do + post api(path, user), params: params + + expect(response).to have_gitlab_http_status(:created) + expect(json_response.dig('workload', 'id')).to be_nil + expect(json_response.dig('workload', 'message')).to eq('Error in git branch creation') + end + end + context 'when duo_workflow_use_composite_identity feature flag is disabled' do it 'uses regular OAuth token' do stub_feature_flags(duo_workflow_use_composite_identity: false) diff --git a/spec/services/ci/workloads/run_workload_service_spec.rb b/spec/services/ci/workloads/run_workload_service_spec.rb index b6892766d32c79..449a27110311bd 100644 --- a/spec/services/ci/workloads/run_workload_service_spec.rb +++ b/spec/services/ci/workloads/run_workload_service_spec.rb @@ -155,6 +155,57 @@ expect(result).to be_success end end + + context 'when branch creation fails' do + before do + allow(project.repository).to receive(:add_branch).and_return(nil) + end + + it 'returns an error response without creating a workload' do + result = execute + + expect(result).to be_error + expect(result.message).to eq('Error in git branch creation') + expect(Ci::Workloads::Workload.count).to eq(0) + end + end + + context 'when branch name already exists' do + before do + allow(project.repository).to receive(:branch_exists?) + .with(match(%r{^workloads/\w+})) + .and_return(true) + allow(project.repository).to receive(:branch_exists?) + .with(project.default_branch_or_main) + .and_return(true) + end + + it 'returns an error response without creating a workload' do + result = execute + + expect(result).to be_error + expect(result.message).to eq('Branch already exists') + expect(Ci::Workloads::Workload.count).to eq(0) + end + end + + context 'when git command raises an error' do + before do + allow(project.repository).to receive(:add_branch) + .and_raise(Gitlab::Git::CommandError.new('git error')) + end + + it 'tracks the exception and returns an error response without creating a workload' do + expect(Gitlab::ErrorTracking).to receive(:track_exception) + .with(instance_of(Gitlab::Git::CommandError)) + + result = execute + + expect(result).to be_error + expect(result.message).to eq('Failed to create branch') + expect(Ci::Workloads::Workload.count).to eq(0) + end + end end end -- GitLab