Skip to content

Only delete invalid approvals when resetting

Patrick Bajao requested to merge 395506-reset-invalid-approvals into master

What does this MR do and why?

When we resetting approvals asynchronously, there's a possible race condition wherein valid and relevant approvals get removed. This can happen when approval was given for the latest version of the MR and the MergeRequestResetApprovalsWorker get performed afterwards.

To prevent that from happening, we check each Approval#patch_id_sha against MergeRequest#current_patch_id_sha to ensure if approvals are still valid or not. Invalid approvals will get deleted while valid approvals will be kept.

This is behind reset_approvals_patch_id feature flag.

How to set up and validate locally

Use script from #395506 (comment 1356110000) or:

  1. Enable reset_approvals_patch_id feature flag: Feature.enable(:reset_approvals_patch_id).
  2. Create project with 2 approvers: Approver A and Approver B.
  3. Create MR.
  4. Approver A approves MR.
  5. Author updates MR changes.
  6. While the approvals aren't reset yet (as of this writing, there's a 10 second delay), Approver B approves MR.
  7. After the approvals are reset, approval of Approver A should be deleted while Approver B's approval should still be present since it was given when the most recent diff was already generated.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Related to #395506

Edited by Patrick Bajao

Merge request reports