• Sean McGivern's avatar
    Fix feature flag race condition with stale reads · 1cd0540c
    Sean McGivern authored
    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.
    1cd0540c
active_support_cache_store_adapter.rb 1.15 KB