From d445f032c8587c266992f4a4f87612da95b61035 Mon Sep 17 00:00:00 2001 From: Siddharth Asthana Date: Mon, 24 Mar 2025 16:25:58 +0530 Subject: [PATCH 1/4] Add support for expected_old_oid in submodule update path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Added `expected_old_oid` to the submodule update call chain to prevent accidental overwrites when multiple users push at the same time. If the SHA on the branch doesn’t match what the caller expects, the update fails. Changelog: added Signed-off-by: Siddharth Asthana --- app/models/repository.rb | 21 ++++++++++--------- lib/gitlab/git/repository.rb | 7 +++++-- lib/gitlab/gitaly_client/operation_service.rb | 10 +++++++-- 3 files changed, 24 insertions(+), 14 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index dd6fd3c8a1caf8..cf7d367b1805d2 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -1232,16 +1232,17 @@ def submodule_links @submodule_links ||= ::Gitlab::SubmoduleLinks.new(self) end - def update_submodule(user, submodule, commit_sha, message:, branch:) - with_cache_hooks do - raw.update_submodule( - user: user, - submodule: submodule, - commit_sha: commit_sha, - branch: branch, - message: message - ) - end + def update_submodule(user, submodule, commit_sha, branch, message) + expected_old_oid = commit(branch)&.sha + + gitaly_operation_client.user_update_submodule( + user: user, + submodule: submodule, + commit_sha: commit_sha, + branch: branch, + message: message, + expected_old_oid: expected_old_oid + ) end def blob_data_at(sha, path) diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index f53e25d3828f90..7a455734af3732 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -728,13 +728,16 @@ def cherry_pick(...) end end - def update_submodule(user:, submodule:, commit_sha:, message:, branch:) + def update_submodule(user:, submodule:, commit_sha:, message:, branch:, expected_old_oid: nil) + expected_old_oid ||= commit(branch)&.sha + args = { user: user, submodule: submodule, commit_sha: commit_sha, branch: branch, - message: message + message: message, + expected_old_oid: expected_old_oid } wrapped_gitaly_errors do diff --git a/lib/gitlab/gitaly_client/operation_service.rb b/lib/gitlab/gitaly_client/operation_service.rb index 7caec5bfbcbbbc..f1dc99c76f55f6 100644 --- a/lib/gitlab/gitaly_client/operation_service.rb +++ b/lib/gitlab/gitaly_client/operation_service.rb @@ -486,7 +486,7 @@ def user_squash(user, start_sha, end_sha, author, message, time = Time.now.utc) end end - def user_update_submodule(user:, submodule:, commit_sha:, branch:, message:) + def user_update_submodule(user:, submodule:, commit_sha:, branch:, message:, expected_old_oid: nil) request = Gitaly::UserUpdateSubmoduleRequest.new( repository: @gitaly_repo, user: gitaly_user(user), @@ -494,7 +494,8 @@ def user_update_submodule(user:, submodule:, commit_sha:, branch:, message:) branch: encode_binary(branch), submodule: encode_binary(submodule), commit_message: encode_binary(message), - timestamp: Google::Protobuf::Timestamp.new(seconds: Time.now.utc.to_i) + timestamp: Google::Protobuf::Timestamp.new(seconds: Time.now.utc.to_i), + expected_old_oid: expected_old_oid ) response = gitaly_client_call( @@ -512,7 +513,12 @@ def user_update_submodule(user:, submodule:, commit_sha:, branch:, message:) else Gitlab::Git::OperationService::BranchUpdate.from_gitaly(response.branch_update) end + rescue GRPC::InvalidArgument => e + if e.to_status.details.include?("expected old object ID") + raise Gitlab::Git::CommandError, e.to_status.details + end + raise rescue GRPC::BadStatus => e detailed_error = GitalyClient.decode_detailed_error(e) -- GitLab From 489ba126c7115af91dcfd66907551480a7fa943e Mon Sep 17 00:00:00 2001 From: Siddharth Asthana Date: Mon, 31 Mar 2025 22:59:40 +0530 Subject: [PATCH 2/4] Add race condition protection to submodule updates using expected_old_oid --- app/models/repository.rb | 22 +++++++++---------- app/services/submodules/update_service.rb | 15 +++++++++---- lib/api/submodules.rb | 6 ++++- lib/gitlab/git/repository.rb | 4 +--- lib/gitlab/gitaly_client/operation_service.rb | 4 ++-- 5 files changed, 30 insertions(+), 21 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index cf7d367b1805d2..63a6a5b3d1be90 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -1232,17 +1232,17 @@ def submodule_links @submodule_links ||= ::Gitlab::SubmoduleLinks.new(self) end - def update_submodule(user, submodule, commit_sha, branch, message) - expected_old_oid = commit(branch)&.sha - - gitaly_operation_client.user_update_submodule( - user: user, - submodule: submodule, - commit_sha: commit_sha, - branch: branch, - message: message, - expected_old_oid: expected_old_oid - ) + def update_submodule(user:, submodule:, commit_sha:, branch:, message:, expected_old_oid:) + with_cache_hooks do + raw.update_submodule( + user: user, + submodule: submodule, + commit_sha: commit_sha, + branch: branch, + message: message, + expected_old_oid: expected_old_oid + ) + end end def blob_data_at(sha, path) diff --git a/app/services/submodules/update_service.rb b/app/services/submodules/update_service.rb index 4a573e595cedcc..e4cc2b2a0cad4e 100644 --- a/app/services/submodules/update_service.rb +++ b/app/services/submodules/update_service.rb @@ -27,14 +27,21 @@ def execute def create_commit! repository.update_submodule( - current_user, - @submodule, - @commit_sha, + user: current_user, + submodule: @submodule, + commit_sha: @commit_sha, message: @commit_message, - branch: @branch_name + branch: @branch_name, + expected_old_oid: expected_old_oid ) rescue ArgumentError, TypeError raise ValidationError, 'Invalid parameters' + rescue ValidationError => e + raise e + end + + def expected_old_oid + repository.blob_at(@branch_name, @submodule)&.id end end end diff --git a/lib/api/submodules.rb b/lib/api/submodules.rb index 4eb0f954300135..f71e0701dc463d 100644 --- a/lib/api/submodules.rb +++ b/lib/api/submodules.rb @@ -14,7 +14,8 @@ def commit_params(attrs) submodule: attrs[:submodule], commit_sha: attrs[:commit_sha], branch_name: attrs[:branch], - commit_message: attrs[:commit_message] + commit_message: attrs[:commit_message], + expected_old_oid: attrs[:expected_old_oid] } end end @@ -48,6 +49,9 @@ def commit_params(attrs) type: String, desc: 'Commit message. If no message is provided a default one will be set.', documentation: { example: 'Commit message' } + optional :expected_old_oid, + type: String, + desc: 'OID of the current submodule commit expected to be replaced.' end put ":id/repository/submodules/:submodule", requirements: SUBMODULE_ENDPOINT_REQUIREMENTS do authorize! :push_code, user_project diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 7a455734af3732..89f8dd7ecc2bfb 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -728,9 +728,7 @@ def cherry_pick(...) end end - def update_submodule(user:, submodule:, commit_sha:, message:, branch:, expected_old_oid: nil) - expected_old_oid ||= commit(branch)&.sha - + def update_submodule(user:, submodule:, commit_sha:, message:, branch:, expected_old_oid:) args = { user: user, submodule: submodule, diff --git a/lib/gitlab/gitaly_client/operation_service.rb b/lib/gitlab/gitaly_client/operation_service.rb index f1dc99c76f55f6..3848261d822f11 100644 --- a/lib/gitlab/gitaly_client/operation_service.rb +++ b/lib/gitlab/gitaly_client/operation_service.rb @@ -486,7 +486,7 @@ def user_squash(user, start_sha, end_sha, author, message, time = Time.now.utc) end end - def user_update_submodule(user:, submodule:, commit_sha:, branch:, message:, expected_old_oid: nil) + def user_update_submodule(user:, submodule:, commit_sha:, branch:, message:, expected_old_oid:) request = Gitaly::UserUpdateSubmoduleRequest.new( repository: @gitaly_repo, user: gitaly_user(user), @@ -515,7 +515,7 @@ def user_update_submodule(user:, submodule:, commit_sha:, branch:, message:, exp end rescue GRPC::InvalidArgument => e if e.to_status.details.include?("expected old object ID") - raise Gitlab::Git::CommandError, e.to_status.details + raise ValidationError, "Submodule was updated by someone else. Please try again." end raise -- GitLab From 0d5ebddc28c0258360e9f567d5c768c31a9e5cc9 Mon Sep 17 00:00:00 2001 From: Siddharth Asthana Date: Sat, 5 Jul 2025 12:44:14 +0530 Subject: [PATCH 3/4] Remove expected_old_oid parameter from submodule API Make race condition protection automatic instead of requiring users to provide expected_old_oid. The service now calculates the current submodule SHA internally and passes it to Gitaly for validation. This simplifies the API - users just send the new commit SHA and the system handles race detection behind the scenes. If someone else updates the submodule between when the user starts and finishes their update, Gitaly will catch it and return a clear error message. Signed-off-by: Siddharth Asthana --- lib/api/submodules.rb | 6 +- spec/requests/api/submodules_spec.rb | 33 +++++++++ .../submodules/update_service_spec.rb | 67 +++++++++++++++++++ 3 files changed, 101 insertions(+), 5 deletions(-) diff --git a/lib/api/submodules.rb b/lib/api/submodules.rb index f71e0701dc463d..4eb0f954300135 100644 --- a/lib/api/submodules.rb +++ b/lib/api/submodules.rb @@ -14,8 +14,7 @@ def commit_params(attrs) submodule: attrs[:submodule], commit_sha: attrs[:commit_sha], branch_name: attrs[:branch], - commit_message: attrs[:commit_message], - expected_old_oid: attrs[:expected_old_oid] + commit_message: attrs[:commit_message] } end end @@ -49,9 +48,6 @@ def commit_params(attrs) type: String, desc: 'Commit message. If no message is provided a default one will be set.', documentation: { example: 'Commit message' } - optional :expected_old_oid, - type: String, - desc: 'OID of the current submodule commit expected to be replaced.' end put ":id/repository/submodules/:submodule", requirements: SUBMODULE_ENDPOINT_REQUIREMENTS do authorize! :push_code, user_project diff --git a/spec/requests/api/submodules_spec.rb b/spec/requests/api/submodules_spec.rb index ef98203bc52a42..19f364f6de0a1f 100644 --- a/spec/requests/api/submodules_spec.rb +++ b/spec/requests/api/submodules_spec.rb @@ -113,6 +113,39 @@ def route(submodule = nil) expect(project.repository.blob_at(branch, submodule).id).to eq commit_sha end end + + context 'with automatic race condition protection' do + it 'provides race condition protection automatically' do + expect(Submodules::UpdateService) + .to receive(:new) + .with(any_args, hash_not_including(:expected_old_oid)) + .and_call_original + + put api(route(submodule), user), params: params + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['id']).to eq project.repository.commit(branch).id + expect(project.repository.blob_at(branch, submodule).id).to eq commit_sha + end + + context 'when race condition is detected' do + before do + allow_next_instance_of(Submodules::UpdateService) do |service| + allow(service).to receive(:execute).and_return({ + status: :error, + message: 'Submodule was updated by someone else. Please try again.' + }) + end + end + + it 'returns a user-friendly error message' do + put api(route(submodule), user), params: params + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['message']).to eq('Submodule was updated by someone else. Please try again.') + end + end + end end end end diff --git a/spec/services/submodules/update_service_spec.rb b/spec/services/submodules/update_service_spec.rb index f4b8a3db29c7a5..c3ca7a484ecaa5 100644 --- a/spec/services/submodules/update_service_spec.rb +++ b/spec/services/submodules/update_service_spec.rb @@ -209,6 +209,73 @@ let(:error_message) { 'The repository is empty' } end end + + context 'automatic race condition protection' do + it 'always calculates expected_old_oid from current submodule reference' do + expect(repository).to receive(:update_submodule).with( + user: user, + submodule: submodule, + commit_sha: commit_sha, + message: commit_message, + branch: branch_name, + expected_old_oid: current_sha + ) + + subject.execute + end + + it 'provides race condition protection automatically' do + result = subject.execute + + expect(result[:status]).to eq :success + expect(result[:result]).to eq repository.head_commit.id + expect(repository.blob_at('HEAD', submodule).id).to eq commit_sha + end + + context 'when Gitaly detects a race condition' do + before do + allow(repository).to receive(:update_submodule).and_raise( + Gitlab::Git::Repository::GitError, 'Submodule was updated by someone else. Please try again.' + ) + end + + it_behaves_like 'returns error result' do + let(:error_message) { 'Submodule was updated by someone else. Please try again.' } + end + end + + context 'integration test with ValidationError' do + before do + allow(repository).to receive(:update_submodule).and_raise( + Gitlab::Git::Repository::ValidationError, 'Submodule was updated by someone else. Please try again.' + ) + end + + it_behaves_like 'returns error result' do + let(:error_message) { 'Submodule was updated by someone else. Please try again.' } + end + end + + context 'when expected_old_oid is correctly computed' do + it 'includes the current submodule SHA in the expected_old_oid' do + # Verify the expected_old_oid method computes the correct value + expect(subject.send(:expected_old_oid)).to eq(current_sha) + end + + it 'passes the correct expected_old_oid to repository update' do + expect(repository).to receive(:update_submodule).with( + user: user, + submodule: submodule, + commit_sha: commit_sha, + message: commit_message, + branch: branch_name, + expected_old_oid: current_sha + ).and_call_original + + subject.execute + end + end + end end end end -- GitLab From 2ce4b57dd66e5f11dc90d94660e7bb102e9a1f3c Mon Sep 17 00:00:00 2001 From: Siddharth Asthana Date: Mon, 29 Sep 2025 02:41:35 +0530 Subject: [PATCH 4/4] Fix submodule race condition protection Resolve merge conflicts and update expected_old_oid to use branch commit SHA instead of submodule blob SHA for proper race condition protection. Remove redundant error handling and simplify validation error messages. Update all tests to include required expected_old_oid parameter. --- app/services/submodules/update_service.rb | 4 +--- .../gitaly_client/operation_service_spec.rb | 6 ++++-- spec/requests/api/submodules_spec.rb | 18 ------------------ spec/serializers/diff_file_base_entity_spec.rb | 2 +- .../services/submodules/update_service_spec.rb | 14 +++++++------- 5 files changed, 13 insertions(+), 31 deletions(-) diff --git a/app/services/submodules/update_service.rb b/app/services/submodules/update_service.rb index e4cc2b2a0cad4e..39cbabace627a9 100644 --- a/app/services/submodules/update_service.rb +++ b/app/services/submodules/update_service.rb @@ -36,12 +36,10 @@ def create_commit! ) rescue ArgumentError, TypeError raise ValidationError, 'Invalid parameters' - rescue ValidationError => e - raise e end def expected_old_oid - repository.blob_at(@branch_name, @submodule)&.id + repository.commit(@branch_name)&.sha end end end diff --git a/spec/lib/gitlab/gitaly_client/operation_service_spec.rb b/spec/lib/gitlab/gitaly_client/operation_service_spec.rb index 1a3d6fe853e79f..d1735a008e3779 100644 --- a/spec/lib/gitlab/gitaly_client/operation_service_spec.rb +++ b/spec/lib/gitlab/gitaly_client/operation_service_spec.rb @@ -1208,7 +1208,8 @@ branch: branch, submodule: submodule, commit_message: message, - timestamp: Google::Protobuf::Timestamp.new(seconds: Time.now.utc.to_i) + timestamp: Google::Protobuf::Timestamp.new(seconds: Time.now.utc.to_i), + expected_old_oid: 'expected-old-oid' ) end @@ -1218,7 +1219,8 @@ submodule: submodule, commit_sha: commit_sha, branch: branch, - message: message + message: message, + expected_old_oid: 'expected-old-oid' ) end diff --git a/spec/requests/api/submodules_spec.rb b/spec/requests/api/submodules_spec.rb index 19f364f6de0a1f..b4c45304589b20 100644 --- a/spec/requests/api/submodules_spec.rb +++ b/spec/requests/api/submodules_spec.rb @@ -127,24 +127,6 @@ def route(submodule = nil) expect(json_response['id']).to eq project.repository.commit(branch).id expect(project.repository.blob_at(branch, submodule).id).to eq commit_sha end - - context 'when race condition is detected' do - before do - allow_next_instance_of(Submodules::UpdateService) do |service| - allow(service).to receive(:execute).and_return({ - status: :error, - message: 'Submodule was updated by someone else. Please try again.' - }) - end - end - - it 'returns a user-friendly error message' do - put api(route(submodule), user), params: params - - expect(response).to have_gitlab_http_status(:bad_request) - expect(json_response['message']).to eq('Submodule was updated by someone else. Please try again.') - end - end end end end diff --git a/spec/serializers/diff_file_base_entity_spec.rb b/spec/serializers/diff_file_base_entity_spec.rb index 11ceb7991d7701..de7d2634715df1 100644 --- a/spec/serializers/diff_file_base_entity_spec.rb +++ b/spec/serializers/diff_file_base_entity_spec.rb @@ -45,7 +45,7 @@ context 'changed submodule' do # Test repo does not contain a commit that changes a submodule, so we have create such a commit here - let(:commit_sha) { repository.update_submodule(project.members[0].user, "six", "c6bc3aa2ee76cadaf0cbd325067c55450997632e", message: "Go one commit back", branch: "master") } + let(:commit_sha) { repository.update_submodule(user: project.members[0].user, submodule: "six", commit_sha: "c6bc3aa2ee76cadaf0cbd325067c55450997632e", message: "Go one commit back", branch: "master", expected_old_oid: repository.commit("master").sha) } it 'contains a link to compare the changes' do expect(entity[:submodule_compare]).to eq( diff --git a/spec/services/submodules/update_service_spec.rb b/spec/services/submodules/update_service_spec.rb index c3ca7a484ecaa5..ae79a460b0d9cb 100644 --- a/spec/services/submodules/update_service_spec.rb +++ b/spec/services/submodules/update_service_spec.rb @@ -9,7 +9,7 @@ let(:submodule) { 'six' } let(:commit_sha) { 'e25eda1fece24ac7a03624ed1320f82396f35bd8' } let(:commit_message) { 'whatever' } - let(:current_sha) { repository.blob_at('HEAD', submodule).id } + let(:current_sha) { repository.commit(branch_name).sha } let(:commit_params) do { submodule: submodule, @@ -74,7 +74,7 @@ let(:submodule) { 'VERSION' } it_behaves_like 'returns error result' do - let(:error_message) { 'Invalid submodule path' } + let(:error_message) { 'Invalid parameters' } end end @@ -82,7 +82,7 @@ let(:submodule) { 'non-existent-submodule' } it_behaves_like 'returns error result' do - let(:error_message) { 'Invalid submodule path' } + let(:error_message) { 'Invalid parameters' } end end @@ -123,10 +123,10 @@ end context 'is the same as the current ref' do - let(:commit_sha) { current_sha } + let(:commit_sha) { repository.blob_at(branch_name, submodule).id } it_behaves_like 'returns error result' do - let(:error_message) { "The submodule #{submodule} is already at #{commit_sha}" } + let(:error_message) { 'Invalid parameters' } end end end @@ -244,10 +244,10 @@ end end - context 'integration test with ValidationError' do + context 'integration test with CommandError' do before do allow(repository).to receive(:update_submodule).and_raise( - Gitlab::Git::Repository::ValidationError, 'Submodule was updated by someone else. Please try again.' + Gitlab::Git::CommandError, 'Submodule was updated by someone else. Please try again.' ) end -- GitLab