Skip to content

Prevent errors in Member DELETE API arising from locks

Manoj M J [On PTO] requested to merge 385432-only-use-locking-mechanism-to-prevent-race-condition-in-group-member-removal-when-removed into master

What does this MR do and why?

Premise

In !96146 (merged), we had introduced a locking mechanism so that we prevent race conditions when deleting 2 different group owners in quick succession. GitLab requires a group to have at least 1 owner at all times, so if there is a race condition on deleting 2 group owners in quick succession via the API, the is this person the last owner of this group? check we perform deleting a member, could both return false for these 2 API calls, and we could end up in a state where a group is left with no owners at all.

Problem

While the fix in !96146 (merged) helps to resolve the problem caused due to the race condition, we had applied the locks even to non-owner member deletes.

This lead to a not so great user experience to API consumers where they were trying to delete members in quick succession, where the members they were deleting were not necessarily group owners. And because it all happens so fast, it is possible that the API call cannot obtain the lock and it fails with a 409 {message: 409 Conflict: Resource lock}.

See https://gitlab.com/gitlab-com/dev-sub-department/section-dev-request-for-help/-/issues/32 for details.

Fix

Actually, we only need to check and prevent the race condition when "Group owners" are being removed. If it is any project member or a group member who isn't an Owner that is being deleted, there isn't a need to prevent the race condition, because there isn't gonna be a is this member the last group owner? check being made for these members anyway.

So, this MR makes changes such that:

  • we try to obtain the lock only when the member to be deleted is a GroupMember and has OWNER access level.
  • For all ProjectMember and Group members that aren't owners, we do NOT process the delete within a lock.

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

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 #385432 (closed)

Edited by Manoj M J [On PTO]

Merge request reports