Skip to content

Look at system notes created just before merge as well as after, when deciding if an MR can be reverted

Nick Thomas requested to merge (removed):42377-fix-merge-request-can-be-reverted into master

What does this MR do?

Relaxes an optimization from https://gitlab.com/gitlab-org/gitlab-ce/commit/f3cf8cc8d1625ae1cd532474191739cd36419425 , scanning notes created up to a minute before a merge request was merged when checking to see if it can be reverted.

Are there points in the code the reviewer needs to double check?

A minute is a fairly arbitrary value. It could be a second to handle just the nanosecond case, or we could add even more leeway to be kinder to people running HA with very unsynchronized clocks. I like a minute, but 15 or 30 seconds could also work fine.

It might be better to store an explicit reverted_at value that we fill when processing commits and notice that one reverts a given MR. Using system notes in this way is a bit suspect. If we want to do this, we should make it happen in a later MR, though, as this spec intermittently fails on at least EE master on a regular basis at the moment.

Why was this MR needed?

On MySQL, at least, Note#created_at doesn't seem to store fractional seconds, while MergeRequest::Metrics#merged_at does (or at least, they are present in some code paths. They're both t.datetime in the schema). 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.

Screenshots (if relevant)

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

Closes #42377 (closed)

Edited by Nick Thomas

Merge request reports