Merge branch 'mk/fix-callback-canceling-in-namespace-move-dir' into 'master'

Fix namespace move callback behavior, especially to fix Geo replication of namespace moves during certain exceptions

Closes gitlab-ee#6252

See merge request gitlab-org/gitlab-ce!19297

3 files changed, 38 insertions, 6 deletions

diff --git a/app/models/concerns/storage/legacy_namespace.rb b/app/models/concerns/storage/legacy_namespace.rb
index f66bdd529f1..ae6595320cb 100644
--- a/app/models/concerns/storage/legacy_namespace.rb
+++ b/app/models/concerns/storage/legacy_namespace.rb
@@ -34,13 +34,12 @@ module Storage
       begin
         send_update_instructions
         write_projects_repository_config
-
-        true
-      rescue
-        # Returning false does not rollback after_* transaction but gives
-        # us information about failing some of tasks
-        false
+      rescue => e
+        # Raise if development/test environment, else just notify Sentry
+        Gitlab::Sentry.track_exception(e, extra: { full_path_was: full_path_was, full_path: full_path, action: 'move_dir' })
       end
+
+      true # false would cancel later callbacks but not rollback
     end
 
     # Hooks
diff --git a/changelogs/unreleased/mk-fix-callback-canceling-in-namespace-move-dir.yml b/changelogs/unreleased/mk-fix-callback-canceling-in-namespace-move-dir.yml
new file mode 100644
index 00000000000..8e71377d93f
--- /dev/null
+++ b/changelogs/unreleased/mk-fix-callback-canceling-in-namespace-move-dir.yml
@@ -0,0 +1,5 @@
+---
+title: Fix namespace move callback behavior, especially to fix Geo replication of namespace moves during certain exceptions.
+merge_request: 19297
+author:
+type: fixed
diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb
index c1b385aaf76..8ff16cbd7cc 100644
--- a/spec/models/namespace_spec.rb
+++ b/spec/models/namespace_spec.rb
@@ -205,6 +205,34 @@ describe Namespace do
         expect(gitlab_shell.exists?(project.repository_storage, "#{namespace.path}/#{project.path}.git")).to be_truthy
       end
 
+      context 'when #write_projects_repository_config raises an error' do
+        context 'in test environment' do
+          it 'raises an exception' do
+            expect(namespace).to receive(:write_projects_repository_config).and_raise('foo')
+
+            expect do
+              namespace.update(path: namespace.full_path + '_new')
+            end.to raise_error('foo')
+          end
+        end
+
+        context 'in production environment' do
+          it 'does not cancel later callbacks' do
+            expect(namespace).to receive(:write_projects_repository_config).and_raise('foo')
+            expect(namespace).to receive(:move_dir).and_wrap_original do |m, *args|
+              move_dir_result = m.call(*args)
+
+              expect(move_dir_result).to be_truthy # Must be truthy, or else later callbacks would be canceled
+
+              move_dir_result
+            end
+            expect(Gitlab::Sentry).to receive(:should_raise?).and_return(false) # like prod
+
+            namespace.update(path: namespace.full_path + '_new')
+          end
+        end
+      end
+
       context 'with subgroups', :nested_groups do
         let(:parent) { create(:group, name: 'parent', path: 'parent') }
         let(:new_parent) { create(:group, name: 'new_parent', path: 'new_parent') }