fix(redis): catch RedisClient::CommandError in Script EVALSHA rescue

TL;DR

Labkit::Redis::Script#eval catches Redis's NOSCRIPT response and retries the script via EVAL. The rescue was looking for the wrong class of exception, so on Redis Cluster the retry never ran and the exception escaped all the way up to GraphQL. One-line fix: catch both exception classes.

The fix

# before
rescue ::Redis::CommandError => e

# after
rescue ::Redis::CommandError, ::RedisClient::CommandError => e

The start_with?("NOSCRIPT") filter immediately after is unchanged, so non-NOSCRIPT errors still propagate exactly as before.

Why catch both classes?

The two exception classes are completely independent -- neither is a subclass of the other:

StandardError
  ├── Redis::BaseError                  ← from the "redis" gem
  │     └── Redis::CommandError
  │           └── Redis::NoScriptError
  │
  └── RedisClient::Error                ← from the "redis-client" gem
        └── RedisClient::CommandError
              └── RedisClient::NoScriptError

rescue ::Redis::CommandError matches the left subtree only. rescue ::RedisClient::CommandError matches the right subtree only. There is no common parent above StandardError.

Labkit::Redis::Script#eval accepts an opaque conn parameter and doesn't know what kind of Redis client it was handed. There are two shapes that matter:

conn is a...	A NOSCRIPT arrives as...
Redis (single-instance, high-level wrapper)	Redis::NoScriptError -- the redis gem's translator converts the underlying RedisClient exception
Redis::Cluster (cluster, high-level wrapper)	RedisClient::NoScriptError -- the cluster translator should convert it, but empirically does not (this is the bug we observed)

If we narrowed the rescue to only RedisClient::CommandError, we'd fix the cluster case and silently regress the single-instance case -- Redis::NoScriptError would start escaping the rescue and crashing callers that worked before. The original code worked for the left column; we need to keep it working there while also handling the right column.

We don't rescue StandardError -- that would catch network failures (Redis::ConnectionError, RedisClient::ConnectionError), timeouts, and bugs inside the script body itself. The start_with?("NOSCRIPT") filter is not a strong enough discriminator to safely re-raise everything that walks in. Catching the two CommandError classes narrows the rescue to "errors that are Redis protocol responses with a string code at the front," which is precisely the surface the filter is designed to discriminate.

The pair-rescue also incidentally future-proofs the code against churn in the gem stack: if a future redis-clustering release fixes its translation gap, the Redis::CommandError branch catches the (now-translated) exception and the RedisClient::CommandError branch becomes dead but harmless. If translation breaks in a new place, the RedisClient::CommandError branch catches it. Either way the labkit contract holds.

What's actually going on with the two hierarchies

Both trees describe the same Redis wire-protocol error. They look like synonyms but Ruby treats them as totally unrelated classes -- a rescue ::Redis::CommandError does not match RedisClient::NoScriptError.

The high-level redis gem normally hides this from us: when the low-level gem raises a RedisClient::* error, the high-level gem catches it and re-raises the corresponding Redis::* subclass instead (via an ERROR_MAPPING hash inside Redis::Client). So most code only ever sees the Redis::* tree, and rescue ::Redis::CommandError works fine.

That translation is the part that breaks on Redis Cluster. The cluster client (Redis::Cluster::Client.translate_error!) has the same intent and the same mapping table, but empirically in CI it sometimes lets a raw RedisClient::NoScriptError escape unchanged. When that happens, our rescue (which only knew about the Redis::* tree) misses it, the EVAL retry doesn't fire, and the exception bubbles out of labkit untouched.

We did not chase down why the cluster translator misses the case -- it's a behavioral gap somewhere in redis-clustering / redis-cluster-client / redis load order. The fix above is correct regardless of whether translation runs or not, so the chase isn't load-bearing.

How we know that's actually what happened (not a guess)

The Sentry log attached to gitlab-org/gitlab!236506 (merged) (note 3383644277) shows:

exception.class:   RedisClient::NoScriptError
exception.message: NOSCRIPT No matching script. Please use EVAL. (redis://rediscluster:7003)

GitLab's logger writes that exception.class field from Gitlab::ExceptionLogFormatter.format! (lib/gitlab/exception_log_formatter.rb lines 11-22):

• Line 12 sets 'exception.class' => exception.class.name -- that's the top-level exception's class. No cause-chain walking, no unwrapping.
• Lines 20-22 unconditionally emit a separate exception.cause_class field whenever exception.cause is present.

The reported entry has no exception.cause_class field. So the exception that propagated past labkit was a raw RedisClient::NoScriptError -- not a translated Redis::NoScriptError, and not an exception with the RedisClient version stashed as a cause. Since RedisClient::NoScriptError is not a subclass of Redis::CommandError, the rescue at lib/labkit/redis/script.rb:36 could not have caught it. That is the entire bug.

Why CI sees this but production hasn't (yet)

Both GitLab.com production and CI run rate-limiting on a Redis Cluster -- production is 4 shards × 3 nodes (12 VMs). This is not a topology-specific bug. Same gems, same code path, same cluster mode.

Production has reported zero instances so far. We have not characterized the gap precisely, but plausible contributors include:

• Long-lived production Redis nodes have warm script caches since labkit 2.0.0 deployed; CI containers spin up fresh per pipeline with cold caches, so NOSCRIPT fires much more often there.
• Whether the specific throttle in the reported trace is currently routed through the labkit adapter in production (cohort / feature-flag dependent).
• Sentry sampling.

Production will hit this eventually -- any SCRIPT FLUSH, node restart, or scaling event evicts the script cache on the affected node, and the next EVALSHA to that node raises NOSCRIPT. Without this fix, that error escapes to whatever called the rate limiter.

Test coverage

Two new specs in spec/labkit/redis/script_spec.rb. Both use the existing spy connection (no real Redis needed):

1. Reproduces the bug. Raises RedisClient::CommandError with a NOSCRIPT message from the spy. Without the production change in this MR, this test fails with the exact RedisClient::CommandError: NOSCRIPT... Jay reported; with the fix, it passes via the EVAL fallback. Verified by stashing the production change locally and watching the test fail, then popping and watching it pass.
2. Guards propagation. Raises a non-NOSCRIPT RedisClient::CommandError (WRONGTYPE) and asserts the exception still propagates unchanged through the widened rescue.

A real Redis Cluster reproduction was not feasible locally. The spy-based specs simulate the exact escape contract that labkit needs to honor: if a RedisClient::CommandError whose message starts with NOSCRIPT arrives at the rescue site, ship the body via EVAL instead of letting it escape.

Related Issues

Closes gitlab-com/gl-infra/production-engineering#29097 (closed)

Reported by @jay_mccure on gitlab-org/gitlab!236506 (merged) (note 3383644277).