Look at notes created just before merge when deciding if an MR can be reverted

On MySQL, at least, `Note#created_at` doesn't seem to store fractional seconds, while `MergeRequest::Metrics#merged_at` does. This breaks the optimization assumption that we only need to search for notes created *after* the MR has been merged. Unsynchronized system clocks also make this a dangerous assumption to make. Adding a minute of leeway still optimizes away most notes, but allows both cases to be handled more gracefully. If the system clocks are more than a minute out, we'll still be broken, of course.

Look at notes created just before merge when deciding if an MR can be reverted
b02f9b61 · Nick Thomas · e2a56af9 · b02f9b61 · b02f9b61
Unverified Commit b02f9b61 authored Jan 24, 2018 by Nick Thomas
Hide whitespace changes
Inline Side-by-side

Showing with 19 additions and 3 deletions

merge_request.rb app/models/merge_request.rb +7 -1

merge_request_spec.rb spec/models/merge_request_spec.rb +12 -2

No files found.
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -989,8 +989,14 @@ class MergeRequest < ActiveRecord::Base
    merged_at = metrics&.merged_at
    notes_association = notes_with_associations

+    # It is not guaranteed that Note#created_at will be strictly later than
+    # MergeRequestMetric#merged_at. Nanoseconds on MySQL may break this
+    # comparison, as will a HA environment if clocks are not *precisely*
+    # synchronized. Add a minute's leeway to compensate for both possibilities
+    cutoff = merged_at - 1.minute
+
    if merged_at
-      notes_association = notes_association.where('created_at > ?', merged_at)
+      notes_association = notes_association.where('created_at >= ?', cutoff)
    end

    !merge_commit.has_been_reverted?(current_user, notes_association)

--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1127,9 +1127,19 @@ describe MergeRequest do
          end
        end

-        context 'when the revert commit is mentioned in a note before the MR was merged' do
+        context 'when the revert commit is mentioned in a note just before the MR was merged' do
          before do
-            subject.notes.last.update!(created_at: subject.metrics.merged_at - 1.second)
+            subject.notes.last.update!(created_at: subject.metrics.merged_at - 30.seconds)
+          end
+
+          it 'returns false' do
+            expect(subject.can_be_reverted?(current_user)).to be_falsey
+          end
+        end
+
+        context 'when the revert commit is mentioned in a note long before the MR was merged' do
+          before do
+            subject.notes.last.update!(created_at: subject.metrics.merged_at - 2.minutes)
          end

          it 'returns true' do