Fix code owner approval reset race condition
Summary
Fixes a race condition where concurrent diff creation causes previous_diff to shift, making entries_since_merge_request_commit compare the wrong commit range and return empty results — preventing selective code owner approval removal.
Problem
When a push happens to an MR's source branch, previous_diff (a positional query: ORDER BY id DESC OFFSET 1 scoped to diff_type: :regular) is used to determine which code owner files changed since the last push. A concurrent process that creates an additional regular MergeRequestDiff record (e.g., overlapping RefreshService.reload_diff() calls from concurrent UpdateMergeRequestsWorker jobs) can shift what previous_diff returns.
Note:
ReloadMergeHeadDiffServicecreatesmerge_headdiffs (diff_type = 2), which are excluded by the-> { regular }scope on themerge_request_diffsassociation. It cannot cause this race condition.
Example: 3 diffs — A, B, C on the same MR:
- We are on Diff A.
- Diff B gets generated (push).
- Reset approval worker gets enqueued.
- Diff C gets generated (concurrent process).
- Reset approval worker executes:
previous_diffreturns B (not A) → compares C→B instead of C→A → misses CODEOWNERS changes → approval survives when it should have been removed.
Solution
Capture push.oldrev at enqueue time and pass it through to ResetApprovalsService. The service uses this stable SHA instead of querying previous_diff at runtime, making it immune to concurrent diff state changes.
A resolve_previous_diff_head_sha method validates the oldrev against the MR's known merge_request_diffs.head_commit_sha values before using it. If the oldrev doesn't match any known diff (e.g., unexpected state), a warning is logged and the service falls back to the existing previous_diff.head_commit_sha behavior. Both the enqueue path and the resolve path are gated behind the feature flag for safe rollback.
Key changes:
ApprovalServicepassespush.oldrevthrough the worker toResetApprovalsService(feature-flag gated)ResetApprovalsService#resolve_previous_diff_head_shavalidatesoldrevagainstmerge_request_diffs.head_commit_sha, with warning log and fallback toprevious_diff.head_commit_sha(feature-flag gated)- The 10-second
perform_indelay is kept — it mitigates a separate race condition whereSyncCodeOwnerApprovalRulesfrom a concurrent push may not have finished syncing approval rules yet. Removing the delay is planned as a follow-up once we add a proper sync-completion check.
Feature Flag
use_oldrev_for_approval_reset — gitlab_com_derisk type, group::code review.
- When enabled: uses
push.oldrevfor stable SHA comparison withperform_in(10.seconds)delay - When disabled: falls back to existing
previous_diffbehavior withperform_in(10.seconds)delay
Disabling the flag fully reverts to old behavior, even for already-enqueued jobs that carry an oldrev argument.
Rollout issue: #594940
Related MRs
- !229090 — Split-out refactor: replaces TTL-based
temporarily_unapprove!with deterministic Redis set-based sync tracking (independent improvement) - !226061 — Fixes a separate race condition on MR creation where
SyncCodeOwnerApprovalRulesWorkerruns before the diff exists
Test plan
reset_approvals_service_spec.rb: Tests concurrent scenario — withoutoldrevapprovals survive (the bug), witholdrevapprovals are correctly removed (the fix). Also tests warning log whenoldrevdoesn't match any known diff.approval_service_spec.rb: Feature flag specs test both enabled/disabled paths.merge_request_reset_approvals_worker_spec.rb: Verifies the worker passes the parameter through correctly.
Relates to #373846