[FF] patch_id_sha_fallback_when_diff_missing -- derive the MR patch ID before the diff row exists

Summary

Roll out the feature currently behind the patch_id_sha_fallback_when_diff_missing feature flag.

  • DRI: @marc_shaw
  • Team Slack channel: #g_code_review

Introduced in !252477 (merged).

MergeRequest#current_patch_id_sha returns nil while a merge request is still preparing, because the diff row does not exist yet. MergeRequests::ApprovalService stamps that nil onto the approval, and Approval.with_invalid_patch_id_sha treats nil as invalid, so the next approval reset deletes an approval that nothing invalidated. With the flag on, the patch ID is computed from the base and head SHAs instead, which returns the same value the diff reports once it is created.

Note

Process and guidance live in the docs - this issue is just the commands and a place to track the rollout. Feature flag controls · Feature flag lifecycle

What could go wrong?

Blast radius is latency, not correctness. The new branch only runs when a merge request has no persisted merge_request_diffs row, which is the few hundred milliseconds between MergeRequests::CreateService returning and NewMergeRequestWorker creating the diff. Once the row exists the method is byte-for-byte the old behaviour.

In that window the fallback resolves diff_base_sha and diff_head_sha, which go to branch_merge_base_commit and source_branch_head, then calls Repository#get_patch_id. So up to three Gitaly calls on a request that previously made none. The two callers that can hit it:

  • MergeRequests::ApprovalService - an approval on a merge request that is still preparing. This is the case the fix is for.
  • MergeRequests::ResetApprovalsService - only if the reset runs before the diff row lands, which is rare because the reset is delayed 10 seconds.

Failure modes are contained. Repository#get_patch_id rescues Gitlab::Git::CommandError, NoRepository and CommandTimedOut and returns nil, so a Gitaly problem degrades to exactly today's behaviour rather than erroring the approve request.

Dashboards to watch on https://dashboards.gitlab.net:

  • Gitaly CommitService RPS and latency, specifically GetPatchID
  • Rails latency for POST /api/:version/projects/:id/merge_requests/:iid/approve and the GraphQL approve mutation
  • MergeRequestResetApprovalsWorker duration and error rate

Rollout

Run all production /chatops in #production and cross-post the results to #g_code_review. Background: incremental rollout process, feature actors.

The actor is the merge request's target project.

Non-production

/chatops gitlab run feature set patch_id_sha_fallback_when_diff_missing 50 --actors --dev --pre --staging --staging-ref
/chatops gitlab run feature set patch_id_sha_fallback_when_diff_missing true --dev --pre --staging --staging-ref

Production - start with a single project, then percentage of actors (wait at least 15 minutes between steps, watch the dashboards above):

/chatops gitlab run feature set --project=gitlab-org/gitlab patch_id_sha_fallback_when_diff_missing true
/chatops gitlab run feature set patch_id_sha_fallback_when_diff_missing 1 --actors
/chatops gitlab run feature set patch_id_sha_fallback_when_diff_missing 10 --actors
/chatops gitlab run feature set patch_id_sha_fallback_when_diff_missing 50 --actors
/chatops gitlab run feature set patch_id_sha_fallback_when_diff_missing 100 --actors

Verification

The bug needs two races to coincide, so it does not show up on an idle instance. What to check instead:

  • New approvals rows for merge requests approved shortly after creation should have a non-null patch_id_sha. Before this change they were null.
  • Reports of an approved this merge request system note followed by reset approvals from ... by pushing to the branch on a merge request whose diff did not change should stop.

Before global rollout

Confirm the relevant gotchas before going to 100% - see enabling a feature for GitLab.com:

Cleanup

Remove the flag once deemed stable - see cleaning up. Remove the flag and its YAML definition from the codebase, then:

/chatops gitlab run release check <merge-request-url> <milestone>
/chatops gitlab run feature delete patch_id_sha_fallback_when_diff_missing --dev --pre --staging --staging-ref --production

Rollback

/chatops gitlab run feature set patch_id_sha_fallback_when_diff_missing false                                         # production
/chatops gitlab run feature set patch_id_sha_fallback_when_diff_missing false --dev --pre --staging --staging-ref     # non-production
/chatops gitlab run feature delete patch_id_sha_fallback_when_diff_missing --dev --pre --staging --staging-ref --production  # remove entirely