fix(cache): scan every primary for streams, not only the one SCAN lands on by robert-mihaiAdam · Pull Request #193 · UiPath/dotnet-caching

robert-mihaiAdam · 2026-09-17T14:41:11Z

Summary

RedisStreamHealthMaintainer discovered streams with a keyless SCAN sent on IDatabase. A keyless command carries no slot for the client to route by, so it reaches whichever single server the multiplexer picks and its cursor walks that server's keyspace alone. On a cluster, every broadcast stream whose slot lives on another primary was never discovered — and so never MINID-trimmed, never group-reaped and never deleted.

Found in production: two stream keys on the same Azure Managed Redis (OSS cluster policy, 2 primaries split at slot 8192), same process, same options — the one above the split was trimmed to the intended ~1h window, the one below it had retained two weeks of events.

Changes

IRedisConnector gains GetPrimaries(), with a default implementation returning empty so no existing implementer breaks.
RedisConnector.GetPrimaries() filters GetServers() to connected primaries, using the same "only if the connection is already established" guard as GetEndPoints.
GetAllStreamsAsync scans every primary, each with its own cursor, and falls back to the routed database when none can be enumerated — which is exactly the single-server behavior it replaces.
Regression test: two primaries with disjoint keyspaces, asserting both are scanned and both streams reach StreamInfoAsync. Verified to fail on main and pass with the fix.
The shared test fixture now stubs GetPrimaries() to empty explicitly, alongside its existing Database stub — left unconfigured, the auto-mocked connector hands back generated servers whose SCAN answers with nothing.

Every other maintainer command (StreamInfoAsync, StreamGroupInfoAsync, StreamTrimByMinIdAsync, the quarantine hash ops, LockTakeAsync) is keyed, so the client already routes those to the owning shard. Only discovery was affected.

Notes for review

Age: present since the maintainer was first added (0bf3de0, June 2024). It is invisible on a single-shard server, and the only prior coverage mocked IDatabase.ExecuteAsync, which cannot model shards — so it passed identically on 1 or 20 primaries.
Why it stayed hidden: it fails partially (streams on the scanned shard are maintained, so the feature looks healthy), the writer-side MAXLEN ~ MaxLength caps growth so nothing alerts, and resizing a cluster remaps slots — so which streams go unmaintained moves over time.
Impact when it bites: the retention window becomes "the last MaxLength entries" (32,768 by default) instead of MaintainerTrimInterval. Bounded, but stream keys carry no TTL, so that memory is not reclaimable under a volatile-* eviction policy.
IServer.ExecuteAsync rather than IServer.Keys() because Keys() has no TYPE filter, and the maintainer relies on TYPE stream to avoid walking the whole keyspace.

Test plan

Unit tests added/updated — new regression test, verified red on main / green with the fix
Integration tests pass locally (dotnet test) — full suite 1770 passed, 0 failed, 12 skipped (Docker-gated); solution builds with 0 warnings
CHANGELOG.md updated

Linked issues

Fixes #

Contributor declaration

I signed off my commits per the DCO (git commit -s).
I am contributing on behalf of my employer, or in the course of employment / using employer resources.

🤖 Generated with Claude Code

github-actions · 2026-09-17T14:41:35Z

🔎 Maintainer heads-up: automated triage flagged this PR as potentially material, so it may need a signed CLA in addition to the DCO sign-off.

Strong signals

adds public API surface (PublicAPI.Unshipped.txt in src/UiPath.Caching)

This is advisory only — the bot does not decide. Please judge against the CLA criteria (material, product-critical, patent-sensitive, corporate contributor, broad commercial use). Note that thresholds can be gamed by splitting PRs, so use your judgement.

If a CLA is needed → add the cla-required label (a contributor comment with signing steps is posted automatically).
If it is not needed → replace needs-cla-review with cla-not-required so later pushes don't re-flag it.

Copilot

Copilot review overview

🟡 Changes recommended

The production primary-filtering implementation lacks direct test coverage.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

Add RedisConnector regression test for filtering and lazy connection · New

What changed in this PR

Fixes Redis stream maintenance so every connected primary is scanned in clustered deployments.

Changes:

Exposes connected Redis primaries through IRedisConnector.
Scans each primary independently, retaining the previous fallback behavior.
Adds regression tests, public API metadata, and release notes.

File	Description
`RedisStreamTopicMonitorTests.cs`	Tests multi-primary scanning and fallback.
`RedisConnector.cs`	Enumerates connected primary servers.
`IRedisConnector.cs`	Adds the primary enumeration API.
`PublicAPI.Unshipped.txt`	Records the new public API.
`RedisStreamHealthMaintainer.cs`	Scans streams across all primaries.
`CHANGELOG.md`	Documents the cluster maintenance fix.

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

…d cover the gaps Discovery changes from the review of #193: Keys from every primary landed in one list, so a slot caught mid-migration -- which answers on both its source and its target -- was processed twice in a pass: doubled stream and group metrics, and a second delete against a key the first visit had already removed. They merge into a set now. The shards share nothing but that merge, so their scans run concurrently. A pass costs the slowest shard rather than the sum, which is what keeps it inside the lock it holds for MaintainerCheckInterval as shard counts grow. QuarantineKey is built after the merge, on one thread, so a custom IRedisKeyStrategy is still only ever called sequentially. Tests: the maintainer fixture reported no primaries, so every behaviour test ran the routed-database fallback -- the one branch a real connector never takes. It now reports one, and the fallback keeps its own test. RedisConnector.GetPrimaries had no test at all; its replica filter, its connected filter and its not-yet-connected guard now have one each. Docs: broadcast.md described discovery as a single SCAN. It now explains the per-primary scan and the fallback, and troubleshooting carries the symptom this produced -- some streams trimmed and others not, on one cluster under one config. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C9ahiMBR7wow2ok8AQgbFf Signed-off-by: Cosmin Staicu <cosmin.staicu@uipath.com>

cosmin-staicu · 2026-09-19T08:37:23Z

Reviewed this and pushed one commit on top of yours (852efef) — the branch is also rebased onto main now that the dependabot backlog is merged, including StackExchange.Redis 3.1.31 → 3.2.1.

The blocker

IServer.ExecuteAsync(command, args, flags) builds its message with db: -1 (RedisServer.cs:1087) and is documented as being for commands that are not database-specific. SCAN is one, so ExecuteMessage resolves it to a known RedisCommand, Message.RequiresDatabase(SCAN) is true, and the constructor throws before anything reaches the wire:

RedisCommandException: A target database is required for SCAN

That throw propagates out of GetAllStreamsAsync into the catch in CheckStreamsAsync, so on any deployment reporting at least one connected primary the maintainer discovered zero streams — nothing trimmed, no group reaped, no stream deleted. Strictly worse than the single-shard scan it replaced, and it reproduces on standalone as readily as on cluster.

Probed directly against 3.1.31, 3.2.1 and 3.2.15:

call	3.1.31	3.2.1	3.2.15
`ExecuteAsync(cmd, args, flags)`	throws	throws	ok
`ExecuteAsync(null, cmd, args, flags)`	ok	ok	ok
`IDatabase.ExecuteAsync(cmd, args, flags)`	ok	ok	ok

Upstream fixed the three-argument form in 3.2.15 — two days ago — but the database-scoped overload is correct on every version, so that's what this uses. It also applies the multiplexer's configured default database, which means the scan reaches the same keyspace the routed IDatabase did; the three-argument form would not have, even where it didn't throw.

The unit test passed because Substitute.For<IServer>() intercepts the call before StackExchange.Redis builds the message — the same class of blind spot the description attributes to the original bug, one layer up. There's now an integration test driving the maintainer against a real server; it fails on the previous code and passes on this.

Also in the commit

De-duplication. Keys from every primary went into one list, so a slot caught mid-migration — answering on both source and target — was processed twice per pass: doubled stream/group metrics and a second KeyDeleteAsync against an already-deleted key. They merge into a set now.
Concurrent scans. A pass now costs the slowest shard rather than the sum, which matters because the lock TTL is MaintainerCheckInterval and didn't grow with the shard count. QuarantineKey is deliberately built after the merge, on one thread, so a custom IRedisKeyStrategy is never called concurrently.
Error isolation + observability. A shard that can't be scanned costs only its own streams for that cycle instead of unwinding the pass, and both degraded modes — a failing primary, and a connector reporting none — are logged. The original went unnoticed since June 2024 precisely because it failed silently and partially.
Tests. The fixture reported no primaries, so every behaviour test exercised the routed-database fallback — the one branch a real connector never takes. It now reports one, and the fallback keeps its own test. RedisConnector.GetPrimaries had no test at all; its replica filter, connected filter and not-yet-connected guard now have one each.
Docs. broadcast.md described discovery as a single SCAN; it now covers the per-primary scan and the fallback, and troubleshooting carries the symptom this produced — some streams trimmed and others not, on one cluster under one config.

Two things I left alone

The lock TTL. Scaling it with shard count looked tempting but would regress the cadence: the success path never releases the lock, so expiry is what schedules the next pass — an N× TTL means maintenance every N intervals. Shortened the pass instead.
CheckStreamGroupWithConsumersAsync deletes a consumer group on quarantineValue.HasValue with no interval check, unlike CheckEmptyStreamGroupAsync. Two overlapping passes therefore collapse the two-cycle quarantine into one. That predates this PR and changes behaviour on the stale-LastDeliveredId path, so it wants its own issue rather than riding along here.

The description is now slightly out of date — the "Notes for review" bullet reasons about IServer.ExecuteAsync vs IServer.Keys(), and it's the overload choice within ExecuteAsync that turned out to matter. Worth a tweak before merge.

Verification: 0 warnings; 1775 tests green without Redis; against a live server the only failures are the two pre-existing RedisPlannedMaintenanceIntegrationTests, which fail identically on a clean tree.

🤖 Generated with Claude Code

https://claude.ai/code/session_01C9ahiMBR7wow2ok8AQgbFf

Copilot

Copilot review overview

🟢 Approval recommended

The clustered discovery fix is scoped, failure-isolated, documented, and covered by connector, maintainer, and live Redis tests.

Review effort: Balanced
Findings: None

Resolved since last review (1)

Add RedisConnector regression test for filtering and lazy connection

…ds on RedisStreamHealthMaintainer discovered streams with a keyless SCAN sent on IDatabase. A keyless command carries no slot for the client to route by, so it reaches whichever single server the multiplexer picks and its cursor walks that server's keyspace alone. On a cluster every broadcast stream whose slot lived on another primary was never discovered, and so never MINID-trimmed, never group-reaped and never deleted -- held only by the MAXLEN each XADD carries, which leaves the last MaxLength entries in place of the intended trim window, on a key that has no TTL and so is not reclaimable under a volatile-* policy. IRedisConnector gains GetPrimaries(), defaulted to empty so no existing implementer breaks, and RedisConnector filters GetServers() to connected primaries. The maintainer scans each one with its own cursor and falls back to the routed database when none can be enumerated, which is the single-server behavior it replaces. Every other maintainer command is keyed, so the client already routes those to the owning shard. Present since the maintainer was first added and invisible until now: a single-shard server behaves correctly, and the only coverage mocked IDatabase.ExecuteAsync, which cannot model shards. The regression test stands two primaries up with disjoint keyspaces and asserts both streams are checked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: robert-mihaiAdam <robert.adam@uipath.com>

Keeps a one-line summary on the new public interface member; the rationale lives in the commit message and the CHANGELOG entry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: robert-mihaiAdam <robert.adam@uipath.com>

… set Review follow-ups to the per-primary scan. The scan went out through IServer.ExecuteAsync(command, args, flags). That overload builds the message with db -1 and is documented for commands that are not database-specific. SCAN is database-specific, so StackExchange.Redis rejects it in the Message constructor -- "A target database is required for SCAN" -- and nothing ever reached the wire. Any deployment reporting at least one connected primary therefore found no streams at all: nothing trimmed, no group reaped, no stream deleted, which is a worse outcome than the single-shard scan it replaced. Passing a null database to the database-scoped overload applies the multiplexer's configured default, so the scan covers the keyspace the routed IDatabase used to. Checked against a live server on 3.1.31, 3.2.1 and 3.2.15; upstream taught the three-argument overload to supply the default database in 3.2.15, and passing it explicitly is what keeps this correct on the versions before that. Keys from every primary went into one list, so a slot caught mid-migration -- it answers on both its source and its target -- was handled twice in a single pass: stream and group metrics counted double, and the second visit issued a delete against a key the first had already removed. The keys merge into a set now. Nothing is shared between the shards but that merge, so they scan concurrently. A pass costs the slowest shard instead of the sum of all of them, which is what keeps it inside the lock it holds for MaintainerCheckInterval as shard counts grow. QuarantineKey is built after the merge, on one thread, so a custom IRedisKeyStrategy is still only ever called sequentially. A shard that cannot be scanned costs its own streams for that cycle rather than unwinding the whole pass, and both degraded modes -- a primary that fails, and a connector that reports none -- are logged, because silence is what let the original bug run undetected since June 2024. Tests: the maintainer fixture reported no primaries, so every behaviour test exercised the routed-database fallback, the one branch a real connector never reaches. It reports one now, and the fallback keeps a test of its own. RedisConnector.GetPrimaries had no test whatsoever; its replica filter, its connected filter and its not-yet-connected guard each have one. Message construction is where this failed and a mocked IServer cannot model it, so the maintainer is driven against a real server as well. Docs: broadcast.md presented discovery as a single SCAN. It now covers the per-primary scan and the fallback, and troubleshooting carries the symptom this produced -- some streams trimmed and others not, on one cluster under one configuration. Signed-off-by: Cosmin Staicu <cosmin.staicu@uipath.com>

Three tests produced a connection failure by pointing at 127.0.0.1:6399 and trusting that nothing answers there. Nothing does on CI, so they passed. But 6399 is the obvious second port for a local Redis, and with one running the connection succeeds, no warning is logged, and StartAsync_RetriesAndWarns and StartAsync_ClampsNegativeRetryDelay fail on "Warnings.Count to be greater than or equal to 2, but found 0". None of the three carries the fixture's skip guard, so they run under a plain dotnet test too, and the failure reads as a broken retry loop rather than as the wrong port. StopAsync_HaltsRetryLoop rested on the same assumption and concealed it: it compares the warning count before and after StopAsync, and against a reachable port both counts are zero, so it passed while asserting nothing. Each test now binds a loopback port of its own. The listener completes the TCP handshake and then never speaks RESP, so the attempt still fails for real, on connectTimeout, down the same code path -- except that the port belongs to the test and no other process can be answering on it. StopAsync_HaltsRetryLoop asserts the loop was running before it stops it, so it can no longer pass vacuously. Signed-off-by: Cosmin Staicu <cosmin.staicu@uipath.com>

Touches only the comments the two follow-up commits on this branch introduced, bringing them to the style 90c4551 set for the original fix. No behaviour change. Signed-off-by: Cosmin Staicu <cosmin.staicu@uipath.com>

sonarqubecloud · 2026-09-21T19:43:21Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

robert-mihaiAdam requested review from alinahornet, cosmin-staicu, cosminvlad, litheon, lucianaparaschivei and razvalex as code owners September 17, 2026 14:41

github-actions Bot added the needs-cla-review A maintainer should assess whether a signed CLA is required (see CONTRIBUTING.md) label Sep 17, 2026

cosmin-staicu requested a balanced review from Copilot September 19, 2026 07:05

Copilot started reviewing on behalf of cosmin-staicu September 19, 2026 07:05 View session

Copilot AI reviewed Sep 19, 2026

View reviewed changes

Comment thread src/UiPath.Caching/Redis/RedisConnector.cs

cosmin-staicu force-pushed the fix/stream-maintainer-scan-all-shards branch from bec3493 to 7b98adc Compare September 19, 2026 08:11

cosmin-staicu force-pushed the fix/stream-maintainer-scan-all-shards branch from 8e57869 to 852efef Compare September 19, 2026 08:36

cosmin-staicu requested a balanced review from Copilot September 19, 2026 08:46

Copilot started reviewing on behalf of cosmin-staicu September 19, 2026 08:46 View session

Copilot AI reviewed Sep 19, 2026

View reviewed changes

This was referenced Sep 19, 2026

build(deps): bump StackExchange.Redis from 3.2.1 to 3.3.0 #194

Open

fix(cache): read the quarantine timestamp back before deleting a group #195

Open

cosmin-staicu added cla-not-required Maintainer reviewed: no CLA required for this contribution and removed needs-cla-review A maintainer should assess whether a signed CLA is required (see CONTRIBUTING.md) labels Sep 19, 2026

This was referenced Sep 19, 2026

feat(cache): take maintenance notices from push frames as well as pub/sub #196

Open

feat(cache): open a health window on an announced disruption, and make the timeout bind #197

Open

cosmin-staicu force-pushed the fix/stream-maintainer-scan-all-shards branch 2 times, most recently from 704b000 to 545a7f5 Compare September 21, 2026 05:23

cosmin-staicu previously approved these changes Sep 21, 2026

View reviewed changes

robert-mihaiAdam and others added 3 commits September 21, 2026 22:27

cosmin-staicu added 2 commits September 21, 2026 22:27

refactor(cache): trim the comments the shard-scan follow-ups added

61472a0

Touches only the comments the two follow-up commits on this branch introduced, bringing them to the style 90c4551 set for the original fix. No behaviour change. Signed-off-by: Cosmin Staicu <cosmin.staicu@uipath.com>

cosmin-staicu dismissed their stale review via 61472a0 September 21, 2026 19:36

cosmin-staicu force-pushed the fix/stream-maintainer-scan-all-shards branch from 545a7f5 to 61472a0 Compare September 21, 2026 19:36

cosmin-staicu approved these changes Sep 21, 2026

View reviewed changes

Navigation Menu

fix(cache): scan every primary for streams, not only the one SCAN lands on - #193

fix(cache): scan every primary for streams, not only the one SCAN lands on#193
robert-mihaiAdam wants to merge 5 commits into
mainfrom
fix/stream-maintainer-scan-all-shards

robert-mihaiAdam commented Sep 17, 2026

Uh oh!

github-actions Bot commented Sep 17, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

cosmin-staicu commented Sep 19, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

sonarqubecloud Bot commented Sep 21, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

robert-mihaiAdam commented Sep 17, 2026

Summary

Changes

Notes for review

Test plan

Linked issues

Contributor declaration

Uh oh!

github-actions Bot commented Sep 17, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

cosmin-staicu commented Sep 19, 2026

The blocker

Also in the commit

Two things I left alone

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

sonarqubecloud Bot commented Sep 21, 2026

Quality Gate passed

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants