Skip to content

Fix feature flag race condition with stale reads [RUN ALL RSPEC] [RUN AS-IF-FOSS]

Sean McGivern requested to merge fix-feature-flag-race-condition into master

On GitLab.com we sometimes observe that a feature flag has been changed - and we have the response in chatops to demonstrate that - but the behaviour of the application doesn't reflect that. While we cannot prove that this is the case, we believe that this is due to a combination of these factors:

  1. We have some feature flags (Gitaly feature flags in particular) that are read very frequently. All Gitaly feature flags are read on every Gitaly call.
  2. We use database load balancing, so a read from a secondary may be slightly behind the primary.
  3. The feature flag library we use (Flipper) deletes the feature flag from the cache when it's changed, and relies on the next read to re-populate the cache. As these reads use what is essentially Rails.cache.fetch, the last writer 'wins'.

That could lead to this situation:

  1. Client A is using the primary. It performs fetch, finds nothing, executes its block to get the value.
  2. Client B does the same, but using a secondary and missing out the write performed by client A.
  3. Client A finishes its block and writes to Redis.
  4. Client B does the same, clobbering client A's write.

The solution in this change is simple: instead of deleting the feature flag from the cache when updating it, we always immediately write the new value back. That way any clients using Rails.cache.fetch won't attempt to write until the cache has expired after an hour, by which point the secondaries would have caught up anyway.

This is behind a feature flag because it is a somewhat risky change, but using a feature flag is safe here as this only controls how we handle feature flag writes.

For #325452 (closed).

Edited by Sean McGivern

Merge request reports

Loading