Skip to content

Avoid sub-group destruction in group link synchronization

What does this MR do and why?

TLDR;

See #440682 (comment 1867958863) I believe there is a very hard-to-reproduce race condition causing this bug. Avoiding deleting sub-groups may fix this. If there is no regression from it, I think we should make the change. Even if it doesn't fix the but. Because as @dblessing puts it:

But I do think it's a problem that we are destroying sub-resources with memberships. I don't think the sync service should be doing this. We should probably be specifying skip_subresources: true for all cases.

Explaination

With SAML each users's login triggers GroupSamlGroupSyncWorker. It's how we implement the hand-shaking process I believe.

When the authorization on higher level group changes it gets deleted first here and deletes all the subgroups membership as well if skip_subresources: true is not set. Therefore in a brief moment during the process, the user might have faulty membership to the project.

If an code owner approval rules where the user is an approver gets updated during that moment, he could be deleted from the approver's list because of the brief moment he has faulty project membership. As the GroupSamlGroupSyncWorker and MergeRequests::SyncCodeOwnerApprovalRulesWorker could be running concurrently.

I think setting skip_subresources: true could fix this. It would be beneficial anyway if it doesn't cause any regression.

Related to #440682

Edited by Patrick Cyiza

Merge request reports