Fix two data races in the branch names cache

What does this MR do?

Expiry of the branch name cache could race with checking inclusion, in such a way that a branch may be marked as non-existent when it does in fact exist.

This MR uses Redis transactions to atomically take the existence of the set at the same time as the smembers / sismembers call, so we can distinguish between "an empty value exists in the cache" and "the cache is empty".

Does this MR meet the acceptance criteria?

Conformity

• Does this MR need a changelog?
  • I have included a changelog entry.
• Documentation (if required)
• Code review guidelines
• Merge request performance guidelines
• Style guides
• Database guides
• Separation of EE specific content

Availability and Testing

• Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process.
• Tested in all supported browsers
• Informed Infrastructure department of a default or new setting change, if applicable per definition of done

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

• Label as security and @ mention @gitlab-com/gl-security/appsec
• The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
• Security reports checked/validated by a reviewer from the AppSec team

Related to #326066 (closed)

marked this merge request as draft

mentioned in issue #326066 (closed)

marked this merge request as ready

added 361 commits

• 2a3855bb...83dc54eb - 360 commits from branch master
• 098ab2e7 - Fix two data races in the branch names cache

Compare with previous version

changed the description

changed milestone to %13.11

added Category:Source Code Management devopscreate groupsource code typebug labels

changed title from Repository set cache races to Fix two data races in the branch names cache

marked the checklist item I have included a changelog entry. as completed

added 1 commit

• bee2ba50 - Fix two data races in the branch names cache

Compare with previous version

added backend label

Reviewer roulette

Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category (e.g. frontend or backend), and by a maintainer in all other categories.

To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.

To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.

Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.

Category	Reviewer	Maintainer
backend	Felipe Artur (@felipe_artur) (UTC-3, 3 hours behind @nick.thomas)	Igor Drozdov (@igor.drozdov) (UTC+3, 3 hours ahead of @nick.thomas)

If needed, you can retry the danger-review job that generated this comment.

Generated by Danger

added 1 commit

• 5aa5f3f4 - Fix two data races in the branch names cache

Compare with previous version

As ever, it's a pain to write specs that exercise the race condition precisely .

The use of redis.multi in the fix for both races is a simple fix; it relies on redis's transactional behaviour to make the "check-then-use" idiom safe: https://redis.io/topics/transactions

The most important part on that page is:

It can never happen that a request issued by another client is served in the middle of the execution of a Redis transaction

@digitalmoksha do you mind acting as reviewer for this MR? I think you're involved with #326066 (closed) from AMER . I'm just finishing up for the day, the changed specs did pass locally though .

These are just theoretical races at the moment - I've not seen evidence that they're the cause of the problems we're seeing. Still, I think they're worth fixing, and if that evidence does come along we can look at turning it into a hotfix.

requested review from @digitalmoksha

Not really involved, but happy to take a look

It looks good to me. There are several CI errors, but I don't think they are related.

@stanhu would you have time to review this? Or who would be a better redis expert?

requested review from @stanhu

added 1 commit

• fafb29e2 - Fix two data races in the branch names cache

Compare with previous version

resolved all threads

approved this merge request

merged

mentioned in commit 982571d2

added workflowstaging label

added workflowproduction label and removed workflowstaging label

added releasedcandidate label

mentioned in issue #273131 (closed)