Backport of 'Delete approval policy reads before their project rules' and two related orphaned-read fixes to 19.3

Sashi Kumar Kumaresanrequested to merge
625031-backport-19-3 into 19-3-stable-ee

What does this MR do and why?

This is a 19.3 backport of three bug fixes that are already merged to master and deployed to GitLab.com. They fix a bug where approval policy rules could silently disappear from merge requests after a policy sync job was interrupted or ran concurrently with another sync. The underlying issue, #625031 (closed), is closed and fixed by these three MRs. A related issue, #625029 (closed), is only partially addressed by this work; see the "Not included" note below.

The three MRs were cherry-picked in merge order and each commit records the original commit SHA. Each one is already part of 19.4.0:

  • !253939 (merged) "Reuse orphaned scan result policy reads on approval rule sync": the create service now refreshes and reuses a scan_result_policies row that was left behind without its approval rule, instead of returning early and never recreating the rule. This heals projects that are already stuck.
  • !253938 (merged) "Delete approval policy reads before their project rules": reorders the per-project delete so the read row is deleted before the approval_project_rules row. The foreign key cascade then removes the rule in the same statement, so an interrupted job can no longer leave a read without its rule. The project transfer worker gets the same reorder.
  • !253949 (merged) "Lock the approval project rule on MR sync and delete policy links before rules": the merge request sync now takes a FOR SHARE lock on the project rule row and stops if the row is gone, so a concurrent policy delete can no longer leave a merge request rule without its source row. Config-wide deletes now remove link rows first to keep the lock order consistent.

This backport is motivated by a self-managed Ultimate customer on 19.2 who hit this bug and opened a severity 2 request for help, https://gitlab.com/gitlab-com/request-for-help/-/work_items/5240. All three cherry-picks applied cleanly with no conflicts. There are no migrations and no feature flags involved, the changes are EE only, and in total ten files changed across the three commits: five application files and five spec files.

Not included: two related gaps are still open on master and are not part of this backport. A stale read row can end up with a different approval_policy_rule_id occupying the same uniqueness slot; that is being fixed separately in !255728 (closed). Also, Security::SyncProjectPolicyWorker still discards ActiveRecord::RecordInvalid without logging it.

MR acceptance checklist

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

  • This MR is backporting a bug fix, documentation update, or spec fix, previously merged in the default branch.
  • The MR that fixed the bug on the default branch has been deployed to GitLab.com (not applicable for documentation or spec changes).
  • The MR title is descriptive (e.g. "Backport of 'title of default branch MR'"). This is important, since the title will be copied to the patch blog post.
  • Required labels have been applied to this merge request
  • This MR has been approved by a maintainer (only one approval is required).
  • Ensure the e2e:test-on-omnibus-ee job has succeeded, or if it has failed, investigate the failures. If you determine the failures are unrelated, you may proceed. If you need assistance investigating, request help in the #s_developer_experience Slack channel to confirm the failures are unrelated to the merge request.

Note to the merge request author and maintainer

If you have questions about the patch release process, please:

Merge request reports