Move snippet commit ousitde update transaction

At the moment, when we update the snippet, we
also commit the information to the repository.
We do this inside a db transaction.

Nevertheless, because of this, some info is
not available to the different thread handling
the commit action.

This creates a race condition with the storage.

In this commit we move the commit action
outside the snippet db transaction.

app/services/snippets/update_service.rb

@@ -35,28 +35,43 @@ module Snippets
     private
 
     def save_and_commit(snippet)
-      snippet.with_transaction_returning_status do
-        snippet.save.tap do |saved|
-          break false unless saved
-
-          # In order to avoid non migrated snippets scenarios,
-          # if the snippet does not have a repository we created it
-          # We don't need to check if the repository exists
-          # because `create_repository` already handles it
-          if Feature.enabled?(:version_snippets, current_user)
-            create_repository_for(snippet)
-          end
-
-          # If the snippet repository exists we commit always
-          # the changes
-          create_commit(snippet) if snippet.repository_exists?
-        end
-      rescue => e
-        snippet.errors.add(:repository, e.message)
-        log_error(e.message)
+      return false unless snippet.save
+
+      # In order to avoid non migrated snippets scenarios,
+      # if the snippet does not have a repository we created it
+      # We don't need to check if the repository exists
+      # because `create_repository` already handles it
+      if Feature.enabled?(:version_snippets, current_user)
+        create_repository_for(snippet)
+      end
+
+      # If the snippet repository exists we commit always
+      # the changes
+      create_commit(snippet) if snippet.repository_exists?
+
+      true
+    rescue => e
+      # Restore old attributes
+      unless snippet.previous_changes.empty?
+        snippet.previous_changes.each { |attr, value| snippet[attr] = value[0] }
+        snippet.save
+      end
 
-        false
+      snippet.errors.add(:repository, 'Error updating the snippet')
+      log_error(e.message)
+
+      # If the commit action failed we remove it because
+      # we don't want to leave empty repositories
+      # around, to allow cloning them.
+      if repository_empty?(snippet)
+        snippet.repository.remove
+        snippet.snippet_repository&.delete
       end
+
+      # Purge any existing value for repository_exists?
+      snippet.repository.expire_exists_cache
+
+      false
     end
 
     def create_repository_for(snippet)
@@ -81,5 +96,13 @@ module Snippets
          file_path: params[:file_name],
          content: params[:content] }]
     end
+
+    # Because we are removing repositories we don't want to remove
+    # any existing repository with data. Therefore, we cannot
+    # rely on cached methods for that check in order to avoid losing
+    # data.
+    def repository_empty?(snippet)
+      snippet.repository._uncached_exists? && !snippet.repository._uncached_has_visible_content?
+    end
   end
 end

changelogs/unreleased/fj-213436-move-update-outside-transaction.yml

+---
+title: Fix race condition updating snippet without repository
+merge_request: 28851
+author:
+type: fixed

spec/features/projects/snippets/user_updates_snippet_spec.rb

@@ -54,11 +54,9 @@ describe 'Projects > Snippets > User updates a snippet', :js do
   end
 
   context 'when the git operation fails' do
-    let(:error_message) { 'foobar' }
-
     before do
       allow_next_instance_of(Snippets::UpdateService) do |instance|
-        allow(instance).to receive(:create_commit).and_raise(StandardError, error_message)
+        allow(instance).to receive(:create_commit).and_raise(StandardError)
       end
 
       fill_in('project_snippet_title', with: 'Snippet new title')
@@ -67,7 +65,7 @@ describe 'Projects > Snippets > User updates a snippet', :js do
     end
 
     it 'renders edit page and displays the error' do
-      expect(page.find('.flash-container span').text).to eq(error_message)
+      expect(page.find('.flash-container span').text).to eq('Error updating the snippet')
       expect(page).to have_content('Edit Snippet')
     end
   end

spec/features/snippets/user_edits_snippet_spec.rb

@@ -85,11 +85,9 @@ describe 'User edits snippet', :js do
   end
 
   context 'when the git operation fails' do
-    let(:error_message) { 'foobar' }
-
     before do
       allow_next_instance_of(Snippets::UpdateService) do |instance|
-        allow(instance).to receive(:create_commit).and_raise(StandardError, error_message)
+        allow(instance).to receive(:create_commit).and_raise(StandardError)
       end
 
       fill_in 'personal_snippet_title', with: 'New Snippet Title'
@@ -98,7 +96,7 @@ describe 'User edits snippet', :js do
     end
 
     it 'renders edit page and displays the error' do
-      expect(page.find('.flash-container span').text).to eq(error_message)
+      expect(page.find('.flash-container span').text).to eq('Error updating the snippet')
       expect(page).to have_content('Edit Snippet')
     end
   end

spec/services/snippets/update_service_spec.rb

@@ -139,18 +139,80 @@ describe Snippets::UpdateService do
           subject
         end
       end
+    end
 
-      it 'returns error when the commit action fails' do
-        error_message = 'foobar'
+    shared_examples 'commit operation fails' do
+      let_it_be(:gitlab_shell) { Gitlab::Shell.new }
 
-        allow_next_instance_of(SnippetRepository) do |instance|
-          allow(instance).to receive(:multi_files_action).and_raise(SnippetRepository::CommitError, error_message)
-        end
+      before do
+        allow(service).to receive(:create_commit).and_raise(SnippetRepository::CommitError)
+      end
 
+      it 'returns error' do
         response = subject
 
         expect(response).to be_error
-        expect(response.payload[:snippet].errors[:repository].to_sentence).to eq error_message
+        expect(response.payload[:snippet].errors[:repository].to_sentence).to eq 'Error updating the snippet'
+      end
+
+      context 'when repository is empty' do
+        before do
+          allow(service).to receive(:repository_empty?).and_return(true)
+        end
+
+        it 'destroys the created repository in disk' do
+          subject
+
+          expect(gitlab_shell.repository_exists?(snippet.repository.storage, "#{snippet.disk_path}.git")).to be_falsey
+        end
+
+        it 'destroys the SnippetRepository object' do
+          subject
+
+          expect(snippet.reload.snippet_repository).to be_nil
+        end
+
+        it 'expires the repository exists method cache' do
+          response = subject
+
+          expect(response).to be_error
+          expect(response.payload[:snippet].repository_exists?).to be_falsey
+        end
+      end
+
+      context 'when repository is not empty' do
+        before do
+          allow(service).to receive(:repository_empty?).and_return(false)
+        end
+
+        it 'does not destroy the repository' do
+          subject
+
+          expect(gitlab_shell.repository_exists?(snippet.repository.storage, "#{snippet.disk_path}.git")).to be_truthy
+        end
+
+        it 'does not destroy the snippet repository' do
+          subject
+
+          expect(snippet.reload.snippet_repository).not_to be_nil
+        end
+
+        it 'expires the repository exists method cache' do
+          response = subject
+
+          expect(response).to be_error
+          expect(response.payload[:snippet].repository_exists?).to be_truthy
+        end
+      end
+
+      it 'rolls back any snippet modifications' do
+        option_keys = options.stringify_keys.keys
+        orig_attrs = snippet.attributes.select { |k, v| k.in?(option_keys) }
+
+        subject
+
+        current_attrs = snippet.attributes.select { |k, v| k.in?(option_keys) }
+        expect(orig_attrs).to eq current_attrs
       end
     end
 
@@ -186,12 +248,13 @@ describe Snippets::UpdateService do
           response = subject
 
           expect(response).to be_error
-          expect(response.payload[:snippet].errors[:repository].to_sentence).to eq error_message
+          expect(response.payload[:snippet].errors[:repository].to_sentence).to eq 'Error updating the snippet'
         end
       end
 
       it 'returns error if snippet does not have a snippet_repository' do
         allow(snippet).to receive(:snippet_repository).and_return(nil)
+        allow(snippet).to receive(:track_snippet_repository).and_return(nil)
 
         expect(subject).to be_error
       end
@@ -219,11 +282,13 @@ describe Snippets::UpdateService do
       it_behaves_like 'public visibility level restrictions apply'
       it_behaves_like 'snippet update data is tracked'
       it_behaves_like 'updates repository content'
+      it_behaves_like 'commit operation fails'
 
       context 'when snippet does not have a repository' do
         let!(:snippet) { create(:project_snippet, author: user, project: project) }
 
         it_behaves_like 'creates repository and creates file'
+        it_behaves_like 'commit operation fails'
       end
     end
 
@@ -235,11 +300,13 @@ describe Snippets::UpdateService do
       it_behaves_like 'public visibility level restrictions apply'
       it_behaves_like 'snippet update data is tracked'
       it_behaves_like 'updates repository content'
+      it_behaves_like 'commit operation fails'
 
       context 'when snippet does not have a repository' do
         let!(:snippet) { create(:personal_snippet, author: user, project: project) }
 
         it_behaves_like 'creates repository and creates file'
+        it_behaves_like 'commit operation fails'
       end
     end
   end