Forbid non-idempotent block in Redis pipelined/multi

Description of the proposal

This rubocop flags potential non-idempotent operations inside a redis.multi or redis.pipelined block.

The way it's implemented is by only allowing send nodes with multi as the receiver and nothing else, ie:

# bad
Gitlab::Redis::SharedState.with do |redis|
  redis.multi do |multi|
    multi.set(...)
    new = multi.incr(...) # variable assignment here is not allowed
  end
end

new.value

# good
Gitlab::Redis::SharedState.with do |redis|
  redis.multi do |multi|
    multi.set(...) # any method of multi is allowed
    multi.incr(...)
  end
end

Why?

This thread explains the situation perfectly.

Redis stores (ie Gitlab::Redis::SharedState) can be wrapped in a Multistore migration helper which means commands will be double read/written to the new Redis store. When using multi/pipelined command, variable assignment can result in a wrong value due to:

1. Result from a Redis command in multi/pipelined block is first stored in a future https://github.com/redis/redis-rb/tree/v4.8.1#futures
2. The variable then holds the value from the second Redis command.
   • This is because of the way Multistore implements write_both which writes to the default store first followed by the non-default store.

In the S2 incident, CachedQuota.track_consumption, the new_balance is first assigned from the SharedState's (original store) result, followed by reassigning to the ClusterSharedState's (new store we're migrating to) result. This makes new_balance store hold the wrong value from ClusterSharedState. The fix is introduced in !137734 (diffs).

In a nutshell, we shouldn't do any non-idempotent operation in the Redis pipelined / multi block.

Limitations

• This cop starts checking from blocks and filters only multi and pipelined command. At the time of writing, redis connection is the only receiver of .multi and .pipelined nodes.
• This cop will catch more false negatives as we only allow a pattern like:

  redis.multi do |m|
    m.call(...)
    m.call(...)
  end

  • This means we still need to resort to our judgment on whether the violating blocks are idempotent even though it's flagged by this cop.

Check-list

• Make sure this MR enables a static analysis check rule for new usage but ignores current offenses.
• Mention this proposal in the relevant Slack channels (e.g. #development, #backend, #frontend).
• If there is a choice to make between two potential styles, set up an emoji vote in the MR:
  • CHOICE_A: 🅰
  • CHOICE_B: 🅱
  • Vote for both choices, so they are visible to others.
• The MR doesn't have significant objections, and is getting a majority of 👍 vs 👎 (remember that we don't need to reach a consensus).
• (If applicable) One style is getting a majority of vote (compared to the other choice).
• (If applicable) Update the MR with the chosen style.
• Create a follow-up issue to fix the current offenses as a separate iteration: ISSUE_LINK
• Follow the review process as usual.
• Once approved and merged by a maintainer, mention it again:
  • In the relevant Slack channels (e.g. #development, #backend, #frontend).
  • (Optional depending on the impact of the change) In the Engineering Week in Review.

/cc @gitlab-org/maintainers/rails-backend