Allow asynchronous rebase operations to be monitored

This MR introduces tracking of the `rebase_jid` for merge requests. As
with `merge_ongoing?`, `rebase_in_progress?` will now return true if a
rebase is proceeding in sidekiq.

After one release, we should remove the Gitaly-based lookup of rebases.
It is much better to track this kind of thing via the database.

3 files changed, 125 insertions, 31 deletions

diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index a2547755510..fe6d68aff3f 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -7,6 +7,8 @@ describe MergeRequest do
   include ProjectForksHelper
   include ReactiveCachingHelpers
 
+  using RSpec::Parameterized::TableSyntax
+
   subject { create(:merge_request) }
 
   describe 'associations' do
@@ -1996,6 +1998,47 @@ describe MergeRequest do
     end
   end
 
+  describe '#rebase_async' do
+    let(:merge_request) { create(:merge_request) }
+    let(:user_id) { double(:user_id) }
+    let(:rebase_jid) { 'rebase-jid' }
+
+    subject(:execute) { merge_request.rebase_async(user_id) }
+
+    it 'atomically enqueues a RebaseWorker job and updates rebase_jid' do
+      expect(RebaseWorker)
+        .to receive(:perform_async)
+        .with(merge_request.id, user_id)
+        .and_return(rebase_jid)
+
+      expect(merge_request).to receive(:lock!).and_call_original
+
+      execute
+
+      expect(merge_request.rebase_jid).to eq(rebase_jid)
+    end
+
+    it 'refuses to enqueue a job if a rebase is in progress' do
+      merge_request.update_column(:rebase_jid, rebase_jid)
+
+      expect(RebaseWorker).not_to receive(:perform_async)
+      expect(Gitlab::SidekiqStatus)
+        .to receive(:running?)
+        .with(rebase_jid)
+        .and_return(true)
+
+      expect { execute }.to raise_error(ActiveRecord::StaleObjectError)
+    end
+
+    it 'refuses to enqueue a job if the MR is not open' do
+      merge_request.update_column(:state, 'foo')
+
+      expect(RebaseWorker).not_to receive(:perform_async)
+
+      expect { execute }.to raise_error(ActiveRecord::StaleObjectError)
+    end
+  end
+
   describe '#mergeable?' do
     let(:project) { create(:project) }
 
@@ -2946,40 +2989,64 @@ describe MergeRequest do
   end
 
   describe '#rebase_in_progress?' do
-    shared_examples 'checking whether a rebase is in progress' do
-      let(:repo_path) do
-        Gitlab::GitalyClient::StorageSettings.allow_disk_access do
-          subject.source_project.repository.path
-        end
-      end
-      let(:rebase_path) { File.join(repo_path, "gitlab-worktree", "rebase-#{subject.id}") }
+    where(:rebase_jid, :jid_valid, :result) do
+      'foo' | true  | true
+      'foo' | false | false
+      ''    | true  | false
+      nil   | true  | false
+    end
 
-      before do
-        system(*%W(#{Gitlab.config.git.bin_path} -C #{repo_path} worktree add --detach #{rebase_path} master))
+    with_them do
+      let(:merge_request) { create(:merge_request) }
+
+      subject { merge_request.rebase_in_progress? }
+
+      it do
+        # Stub out the legacy gitaly implementation
+        allow(merge_request).to receive(:gitaly_rebase_in_progress?) { false }
+
+        allow(Gitlab::SidekiqStatus).to receive(:running?).with(rebase_jid) { jid_valid }
+
+        merge_request.rebase_jid = rebase_jid
+
+        is_expected.to eq(result)
       end
+    end
+  end
 
-      it 'returns true when there is a current rebase directory' do
-        expect(subject.rebase_in_progress?).to be_truthy
+  describe '#gitaly_rebase_in_progress?' do
+    let(:repo_path) do
+      Gitlab::GitalyClient::StorageSettings.allow_disk_access do
+        subject.source_project.repository.path
       end
+    end
+    let(:rebase_path) { File.join(repo_path, "gitlab-worktree", "rebase-#{subject.id}") }
+
+    before do
+      system(*%W(#{Gitlab.config.git.bin_path} -C #{repo_path} worktree add --detach #{rebase_path} master))
+    end
 
-      it 'returns false when there is no rebase directory' do
-        FileUtils.rm_rf(rebase_path)
+    it 'returns true when there is a current rebase directory' do
+      expect(subject.rebase_in_progress?).to be_truthy
+    end
 
-        expect(subject.rebase_in_progress?).to be_falsey
-      end
+    it 'returns false when there is no rebase directory' do
+      FileUtils.rm_rf(rebase_path)
 
-      it 'returns false when the rebase directory has expired' do
-        time = 20.minutes.ago.to_time
-        File.utime(time, time, rebase_path)
+      expect(subject.rebase_in_progress?).to be_falsey
+    end
 
-        expect(subject.rebase_in_progress?).to be_falsey
-      end
+    it 'returns false when the rebase directory has expired' do
+      time = 20.minutes.ago.to_time
+      File.utime(time, time, rebase_path)
 
-      it 'returns false when the source project has been removed' do
-        allow(subject).to receive(:source_project).and_return(nil)
+      expect(subject.rebase_in_progress?).to be_falsey
+    end
 
-        expect(subject.rebase_in_progress?).to be_falsey
-      end
+    it 'returns false when the source project has been removed' do
+      allow(subject).to receive(:source_project).and_return(nil)
+
+      expect(subject.rebase_in_progress?).to be_falsey
     end
   end
 
diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb
index a82ecb4fd63..ced853caab4 100644
--- a/spec/requests/api/merge_requests_spec.rb
+++ b/spec/requests/api/merge_requests_spec.rb
@@ -2033,6 +2033,9 @@ describe API::MergeRequests do
 
       expect(response).to have_gitlab_http_status(202)
       expect(RebaseWorker.jobs.size).to eq(1)
+
+      expect(merge_request.reload).to be_rebase_in_progress
+      expect(json_response['rebase_in_progress']).to be(true)
     end
 
     it 'returns 403 if the user cannot push to the branch' do
@@ -2043,6 +2046,16 @@ describe API::MergeRequests do
 
       expect(response).to have_gitlab_http_status(403)
     end
+
+    it 'returns 409 if a rebase is already in progress' do
+      Sidekiq::Testing.fake! do
+        merge_request.rebase_async(user.id)
+
+        put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/rebase", user)
+      end
+
+      expect(response).to have_gitlab_http_status(409)
+    end
   end
 
   describe 'Time tracking' do
diff --git a/spec/services/merge_requests/rebase_service_spec.rb b/spec/services/merge_requests/rebase_service_spec.rb
index 7e2f03d1097..ee9caaf2f47 100644
--- a/spec/services/merge_requests/rebase_service_spec.rb
+++ b/spec/services/merge_requests/rebase_service_spec.rb
@@ -6,10 +6,12 @@ describe MergeRequests::RebaseService do
   include ProjectForksHelper
 
   let(:user) { create(:user) }
+  let(:rebase_jid) { 'fake-rebase-jid' }
   let(:merge_request) do
-    create(:merge_request,
+    create :merge_request,
            source_branch: 'feature_conflict',
-           target_branch: 'master')
+           target_branch: 'master',
+           rebase_jid: rebase_jid
   end
   let(:project) { merge_request.project }
   let(:repository) { project.repository.raw }
@@ -23,11 +25,11 @@ describe MergeRequests::RebaseService do
   describe '#execute' do
     context 'when another rebase is already in progress' do
       before do
-        allow(merge_request).to receive(:rebase_in_progress?).and_return(true)
+        allow(merge_request).to receive(:gitaly_rebase_in_progress?).and_return(true)
       end
 
       it 'saves the error message' do
-        subject.execute(merge_request)
+        service.execute(merge_request)
 
         expect(merge_request.reload.merge_error).to eq 'Rebase task canceled: Another rebase is already in progress'
       end
@@ -36,6 +38,13 @@ describe MergeRequests::RebaseService do
         expect(service.execute(merge_request)).to match(status: :error,
                                                         message: described_class::REBASE_ERROR)
       end
+
+      it 'clears rebase_jid' do
+        expect { service.execute(merge_request) }
+          .to change { merge_request.rebase_jid }
+          .from(rebase_jid)
+          .to(nil)
+      end
     end
 
     shared_examples 'sequence of failure and success' do
@@ -43,14 +52,19 @@ describe MergeRequests::RebaseService do
         allow(repository).to receive(:gitaly_operation_client).and_raise('Something went wrong')
 
         service.execute(merge_request)
+        merge_request.reload
 
-        expect(merge_request.reload.merge_error).to eq described_class::REBASE_ERROR
+        expect(merge_request.reload.merge_error).to eq(described_class::REBASE_ERROR)
+        expect(merge_request.rebase_jid).to eq(nil)
 
         allow(repository).to receive(:gitaly_operation_client).and_call_original
+        merge_request.update!(rebase_jid: rebase_jid)
 
         service.execute(merge_request)
+        merge_request.reload
 
-        expect(merge_request.reload.merge_error).to eq nil
+        expect(merge_request.merge_error).to eq(nil)
+        expect(merge_request.rebase_jid).to eq(nil)
       end
     end
 
@@ -72,7 +86,7 @@ describe MergeRequests::RebaseService do
       it 'saves a generic error message' do
         subject.execute(merge_request)
 
-        expect(merge_request.reload.merge_error).to eq described_class::REBASE_ERROR
+        expect(merge_request.reload.merge_error).to eq(described_class::REBASE_ERROR)
       end
 
       it 'returns an error' do