Map pipelined markdown cache replies to their field names

What does this MR do and why?

Fixes a 500 that fires when GitLab preloads cached rendered markdown for many commits at once — the commits list page and the compare page — while the Redis cache runs in cluster mode:

NameError: `@some invalid variable name' is not allowed as an instance variable name

See this surge of errors on Sentry.

The string in the error is an actual commit title, not a placeholder.

Root cause

The pipelined read in Gitlab::MarkdownCache::Redis::Store#read used mapped_hmget. That command does not ask Redis for a map: it sends a plain HMGET and attaches a client-side block that zips the requested field names against the positional reply. In a pipeline, that block is applied by RedisClient::Pipeline#_coerce!.

On a cluster, _coerce! is only invoked from RedisClient::Cluster::Pipeline#send_pipeline. When a node's sub-pipeline raises RedirectionNeeded — a MOVED or ASK reply from at least one command routed to that node — Cluster::Pipeline#execute copies that node's raw replies straight into the result array and never calls _coerce!. mapped_hmget then returns a positional Array instead of a Hash.

Gitlab::MarkdownCache::Redis::Extension.preload_markdown_cache! does fields[object.cache_key].each do |field_name, value|. Destructuring an Array of Strings makes field_name the first element — the rendered title_html — so instance_variable_set("@#{field_name}", value) raises. title_html is first because cache_markdown_field :title, pipeline: :single_line is declared first on Commit, and single-line rendering of a plain commit title produces no HTML tags, so it reads as ordinary text.

This needs Redis Cluster and a MOVED/ASK redirection, so it tracks slot migration, a failover that changes slot ownership, or a stale client slot map — not steady state. It is self-limiting, because the client updates its slot map while handling the redirection, so it arrives in short bursts around topology changes. When it does fire it is not one key: every command routed to that node in the pipeline gets a raw reply, though preload_markdown_cache! raises on the first one it reaches.

How it is fixed

The pipelined path no longer depends on the client's reply transformation. Store#read sends hmget, and the new Store#map_reply pairs the positional reply with the field list. bulk_read keeps the Store instances so each one maps its own reply.

hmget carries no transformation block, so the returned element is identical whether or not _coerce! runs — the pipelined path is immune rather than merely less likely to break. The non-pipelined read still uses mapped_hmget, because single commands go through the cluster router, which does forward the transformation block through redirection handling.

No feature flag: happy-path behaviour is unchanged, and the mapping is exactly what the Redis client was already doing.

How to set up and validate locally

The second commit adds a regression spec that forces the pipeline to hand back bare positional arrays, the way a cluster does on redirection, and asserts the markdown still preloads. Against the previous implementation it fails with the reported error:

Failure/Error: instance_variable_set("@#{field_name}", value)
NameError:
  `@2228224' is not allowed as an instance variable name

Full run:

bundle exec rspec spec/lib/gitlab/markdown_cache/redis/ spec/models/commit_collection_spec.rb
# 42 examples, 0 failures

One existing expectation in spec/lib/gitlab/markdown_cache/redis/extension_spec.rb moved from mapped_hmget to hmget. Reproducing the failure against a real cluster would need a live Redis Cluster with a slot migration mid-pipeline, which is why the spec stubs the reply shape instead.

Note

The skipped coercion on redirection is an upstream bug in redis-cluster-client (0.17.0, the latest release at time of writing, alongside redis 5.4.1 and redis-client 0.30.0) and is worth reporting there. This MR does not depend on that being fixed.