fix(cache): co-locate a stream with its quarantine hash, and refuse a key whose braces form no tag

[cosmin-staicu]

The bug

RedisStreamHealthMaintainer built its quarantine key by rendering a prefix through GetRedisKey and concatenating the stream key onto it:

_quarantineKeyPrefix = hashKeyStrategy.GetRedisKey("caching:meta:stream$");
ret.Add(new StreamContext(key, string.Concat(_quarantineKeyPrefix, key)));

GetRedisKey renders a complete key, so with ShardKeyEnabled the rendered prefix carried its own {...}, leaving app:h:{caching:meta:stream$}app:st:topic. Every quarantine hash inherited that one constant tag and landed on a single slot.

Separately, CheckEmptyStreamAsync deletes the stream and its quarantine hash in one command:

await Database.KeyDeleteAsync([context.StreamKey, context.QuarantineKey], CommandFlags.DemandMaster);

Redis Cluster answers a multi-key command with CROSSSLOT unless every key hashes to the same slot, and these two never did — so the empty-stream cleanup has never completed on a cluster.

The fix

Build the key in one call. The quarantine key is the marker, the stream key and a terminator, rendered once:

stream1          → tst:h:caching:meta:stream$stream1$
tst:st:{topicA}  → tst:h:caching:meta:stream$tst:st:{topicA}$

It spreads with the stream key that names it, and is injective because the stream key appears verbatim. The trailing terminator matters because CacheKey calls Trim() even under sensitive casing (CacheKey.cs:29) — without it the composed name ended in the stream key, and topic collapsed onto topic.

Delete one key per command. Co-locating the pair is not soundly achievable: the configured IRedisKeyStrategy and any connection key prefix both shape the key that reaches Redis, and either can put the two on different slots whatever this library composes. RedisCacheOptions.KeyPrefix is already in play in this class — ParseStreamScan strips it so WithKeyPrefix can re-add it — and a brace-free prefix alone is enough to break a tag-matching scheme. So CheckEmptyStreamAsync issues two single-key deletes and nothing depends on a shared slot. The cost is one extra round trip on a rare cleanup path; an orphaned quarantine hash carries the TTL CheckStreamWithGroupsAsync sets.

The wrapping rule, in one place

Wrapping a key that already holds a brace pairs the added { with it and leaves a tag that is only a prefix of the key, quietly collapsing unrelated keys onto one slot — bla{}bla wrapped to {bla{}bla} gives Redis the tag bla{. An empty key is no better: {} is a brace pair Redis reads as no tag at all.

RedisHashTag.EnsureTag now owns the rule the library already applied in StreamSuffixShardedChannelStrategy: reuse a valid tag, wrap a non-empty brace-free key, refuse anything else. Both ShardPrefixRedisKeyStrategy and StreamSuffixShardedChannelStrategy go through it.

Behaviour changes

• ShardPrefixRedisKeyStrategy now throws on a key it cannot tag — bla{bla, bla}bla, bla{}bla, and the empty key. bla{bla previously wrapped to a correct tag by accident; the others silently truncated it or produced no tag. No regression for the caches: RedisCache.ToRedisKey already rejects an empty CacheKey before the strategy is reached.
• Quarantine keys change shape, orphaning existing entries. They are maintenance metadata with a TTL of MaintainerQuarantineInterval × 10 (10h by default), so they expire on their own.

Tests

net10.0 1715 pass, net8.0 1694 pass, 0 failures (Redis integration tests skipped — no Docker). New coverage pins the rendered quarantine key under both ShardKeyEnabled settings, its injectivity including keys differing only by surrounding whitespace, that the empty-stream cleanup uses single-key deletes and never the multi-key overload, and the refused shapes in both strategies.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NsPw6MZHPGmpbo6WDuLzF1

[Copilot]

🟡 Changes recommended

Unresolved findings include a critical quarantine-key collision and moderate separator-normalization and empty-key issues.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This pull request fixes Redis Cluster slot affinity for stream quarantine metadata and centralizes hash-tag validation.

Changes:

• Builds quarantine keys using the stream’s hash tag while preserving casing.
• Rejects malformed hash-tag patterns and skips invalid streams.
• Adds coverage for affinity, malformed keys, and skip behavior.

File summaries

File	Summary and findings
tests/UiPath.Caching.Tests/ShardPrefixRedisKeyStrategyTests.cs	Adds coverage for malformed hash-tag rejection.
tests/UiPath.Caching.Tests/Broadcast/RedisStreamTopicMonitorTests.cs	Tests quarantine affinity and stream-skip behavior.
src/UiPath.Caching/Redis/ShardPrefixRedisKeyStrategy.cs	Uses centralized hash-tag validation.
src/UiPath.Caching/Redis/RedisHashTag.cs	Centralizes tag handling. Findings: summary wording is inaccurate (nit, 1 vote); empty keys can produce {} and recreate CROSSSLOT behavior (moderate, 1 vote).
src/UiPath.Caching/Broadcast/Redis/StreamSuffixShardedChannelStrategy.cs	Reuses centralized hash-tag validation.
src/UiPath.Caching/Broadcast/Redis/RedisStreamHealthMaintainer.cs	Generates slot-affine quarantine keys. Findings: normalize the separator marker (moderate, 1 vote); quarantine keys collide for distinct keys such as x and {x} (critical, 1 vote).

Review details

Suppressed comments (3)

src/UiPath.Caching/Broadcast/Redis/RedisStreamHealthMaintainer.cs:91

• PrefixRedisKeyStrategy and PrefixStrategy normalize the configured separator to lowercase, but this marker is built from the raw separator and is then preserved by CacheKeyCasing.Sensitive. With Separator = 'B', the quarantine key contains cachingBmetaBstream while the stream and keyspace prefixes use b, producing a layout inconsistent with the rest of the Redis keys. Normalize the separator when constructing the marker.

        _quarantineMarker = string.Join(_cacheOptions.Separator, "caching", "meta", "stream") + "$";

src/UiPath.Caching/Redis/RedisHashTag.cs:20

• The summary is inaccurate for the first branch: when key already has a valid tag, Redis hashes only the content inside that tag, not all of key. Please describe the actual contract—reuse an existing valid tag or wrap a brace-free key—so callers do not infer that the whole key participates in hashing.

    /// <summary><paramref name="key"/> rendered so Redis Cluster hashes all of it, reusing a tag it already carries.</summary>

src/UiPath.Caching/Redis/RedisHashTag.cs:29

• An empty brace-free key reaches this branch and returns {}. Redis treats an empty {} pair as no hash tag, so a quarantine key built for an empty stream key hashes the whole quarantine key instead of the stream key (when sharding is disabled), recreating the CROSSSLOT failure. Treat an empty key as unmaintainable and throw/skip it rather than wrapping it.

        if (ContainsNoBraces(key))
        {
            return "{" + key + "}";

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Unresolved moderate issues affect key identity and slot-affinity guarantees.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

src/UiPath.Caching/Broadcast/Redis/RedisStreamHealthMaintainer.cs:94

• CacheOptions.Separator is only rejected when it is whitespace (src/UiPath.Caching/Redis/Guard.cs:5-12), so { and } are valid configurations. This marker puts that separator before the stream's tag; with a brace separator, default stream keys contain unusable braces and are skipped, while a custom pre-tagged stream makes the marker's first brace become Redis's tag. The new quarantine/stream slot-affinity guarantee therefore still fails for a permitted separator; reject brace separators or use a brace-free marker encoding.

        _quarantineMarker = string.Join(_cacheOptions.Separator, "caching", "meta", "stream") + "$";

src/UiPath.Caching/Redis/RedisHashTag.cs:29

• ContainsNoBraces also accepts the empty string, so this returns {} for an empty key. Redis treats {} as an empty tag and hashes the whole key instead, meaning the quarantine key will not share the empty stream key's slot (and the outer shard strategy may reject it). A custom stream-key strategy or search pattern can expose an empty stream key, so handle it explicitly rather than claiming the wrapping guarantees affinity.

        if (ContainsNoBraces(key))
        {
            return "{" + key + "}";

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

One or more issues must be addressed before approval.

Review details

Suppressed comments (1)

src/UiPath.Caching/Redis/RedisHashTag.cs:29

• An empty key passes ContainsNoBraces and is wrapped as {}. Because HasValidTag treats an empty brace pair as invalid, Redis hashes this key by its full name rather than by a tag; an empty stream key would therefore be cross-slot with its quarantine hash when KeyDeleteAsync sends both keys together. Reject empty input before this branch (or otherwise handle it without claiming slot affinity).

        if (ContainsNoBraces(key))
        {
            return "{" + key + "}";

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Empty keys and custom Redis key strategies can still produce quarantine keys on a different cluster slot.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

Empty Redis keys still produce an unusable hash tag and can trigger the CROSSSLOT failure being fixed.

Review details

Suppressed comments (1)

src/UiPath.Caching/Redis/RedisHashTag.cs:29

• An empty Redis key is brace-free, so this returns {}. Redis ignores an empty hash tag and hashes the entire quarantine key, while the empty stream key hashes the empty byte sequence; the resulting multi-key delete is therefore still CROSSSLOT. Empty Redis keys are valid and can be found by a custom MaintainerSearchPattern, so reject this case and let the maintainer's existing exception handling skip it.

        if (ContainsNoBraces(key))
        {
            return "{" + key + "}";

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Sharded maintenance can abort when discovered stream names or configured separators contain unmatched braces.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/UiPath.Caching/Broadcast/Redis/RedisStreamHealthMaintainer.cs:144

• With ShardKeyEnabled, this composed name is passed to ShardPrefixRedisKeyStrategy.EnsureTag. A valid Redis stream name such as stream} (discoverable through MaintainerSearchPattern or a custom stream strategy) therefore throws while GetAllStreamsAsync builds contexts, and the outer catch aborts the entire maintenance pass. A configured {/} separator causes the same failure for ordinary streams because separators are only required to be non-whitespace. Since deletion no longer requires slot affinity, encode the stream identity into a brace-free representation before applying the hash strategy, or otherwise keep internal metadata names out of this rejection path, and add regression coverage.

        _hashKeyStrategy.GetRedisKey(new CacheKey(
            _quarantineMarker + streamKey.ToString() + StreamKeyTerminator,
            CacheKeyCasing.Sensitive));

• Files reviewed: 7/7 changed files
• Comments generated: 3
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Malformed-brace streams can abort maintenance, and deletion ordering can permanently orphan metadata.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 3
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Brace separators can abort maintenance, and legacy quarantine hashes are not guaranteed to expire.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 2
• Review effort level: Balanced

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Copilot]

🟢 Approval recommended

The revised implementation addresses Redis Cluster slot constraints and the identified edge cases with focused regression coverage.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Balanced