Skip to content

perf(cache)!: share concurrent inner reads and serve an outage from the local tier - #223

Open
cosmin-staicu wants to merge 1 commit into
mainfrom
perf/coalesce-in-flight
Open

cosmin-staicu wants to merge 1 commit into
mainfrom
perf/coalesce-in-flight

Conversation

@cosmin-staicu

@cosmin-staicu cosmin-staicu commented Oct 6, 2026 •

Copy link
Copy Markdown
Member

Closes #222.

What changes

Concurrent inner reads are shared.

  • On a local miss, concurrent single-key reads of one key wait for a single inner-tier read and share its result or failure. This covers GetAsync, GetCacheEntryAsync, the hash reads, and the read GetOrAddAsync makes before its lock.
  • Multi-key reads (GetAsync(CacheKey[]), GetCacheEntriesAsync) still read every missing key themselves: they go out as one MGET, and splitting that per key is a separate change.
  • Hash reads fetch the whole entry once, and each caller filters its own fields.
  • Cancellation:
    • A caller that cancels stops waiting.
    • The shared read runs on its own token, cancelled only once every waiting caller has cancelled.

GetOrAddAsync keeps a value the inner tier refused.

  • The generator still runs under the local lock, and lock settings mean what they did.
  • When the inner write fails, the generated value is kept locally for LocalMaxExpirationDisconnected.
  • So the callers waiting on the lock reuse it instead of each running the generator again, one after another.

An outage is served from the local tier by default.

  • CacheOptions.ConnectionMonitorEnabled and UseLocalOnlyWhenDisconnected now default to true.
  • New ClearLocalOnReconnect, also true by default: when a Redis Pub/Sub broadcast reconnects, the local tier is cleared, since invalidations published during the outage never arrived. Redis Streams replay them, so the local tier is kept.
  • A Redis Streams consumer checks after a reconnect whether entries past its last delivery were trimmed, and treats a consumer group or stream removed while in use as a loss too. Either way, every local entry on that topic expires, in each cache that uses it. The topic signals it through the new IEventSubject<T>.Invalidate().
  • The monitor setting is app-wide, so the broadcast providers' connection monitor turns on too.

A refresh is broadcast as CacheRefreshed. CacheEventPublisher.CacheRefreshedAsync sent the CacheRemoved type. The library's receivers treat both alike; only a subscriber filtering by type sees the change.

GetOrAddAsync miss path, before and after:

flowchart LR
  A[callers miss locally] --> B[shared inner read]
  B -->|hit| C[all return the value]
  B -->|miss| D[local lock]
  D --> E[generator runs once]
  E --> F{inner write}
  F -->|ok| G[kept locally, waiters hit]
  F -->|refused| H[kept locally for the disconnected cap, waiters hit]
Loading

Breaking

A disconnected inner tier now serves the local copy instead of answering misses, and the connection monitor is on by default. For the previous behaviour, set UseLocalOnlyWhenDisconnected = false, or ConnectionMonitorEnabled = false.

IEventSubject<T> gains Invalidate(). A custom subject passed to the RedisPubSubTopic or RedisStreamsTopic constructor must implement it, calling OnEventsMissed() on each observer that implements the new public IMissedEventsObserver, as the library's change tokens do.

IMultilayerCacheOptions gains ClearLocalOnReconnect, which a custom implementation must add.

Tests

  • InFlightTests cover the coalescer:
    • one run per key
    • failure shared
    • a leaving caller doesn't cancel the others
    • cancelled once all callers leave
    • a fresh run after an abandoned one
  • InnerTierOutageTests cover:
    • shared cold reads (cache and hash, different fields)
    • one generator run when the inner write is refused (cache and hash)
    • local copy served while disconnected by default
    • clear on reconnect (default, on, off), only over Pub/Sub: kept over Streams and without a broadcast connection
    • Streams loss (real Redis): trimmed during an outage, replayed without loss, already-read entries trimmed, stream removed while in use, and the topic expiring its entries
    • the monitor default
  • Red-checked: each of the six changes, reverted on its own, fails its test.
  • Existing tests that assumed the old defaults now set them explicitly.
  • Full suite: net10.0 2171, net8.0 2140.

@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown

🔎 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.

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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Reconnect handling and abandoned shared reads contain correctness gaps that can clear or overwrite valid local data.

Review effort: Balanced
Findings: 3 High severity · 1 Medium severity · 1 Low severity

Open (5)
What changed in this PR

Adds shared inner-tier reads and resilient local-cache behavior during outages.

Changes:

  • Coalesces concurrent reads by key and type.
  • Retains generated values locally when inner writes fail.
  • Enables outage monitoring and local-tier recovery behavior by default.
File Description
tests/​UiPath.Caching.Tests/​SpanKeyReadTests.cs Preserves legacy disconnected-read expectations.
tests/​UiPath.Caching.Tests/​MultilayerHashCacheTests.cs Updates failed-write and monitor expectations.
tests/​UiPath.Caching.Tests/​MultilayerHashCacheRehydrateTests.cs Disables monitoring for unrelated tests.
tests/​UiPath.Caching.Tests/​MultilayerHashCachePerNameJitterTests.cs Isolates jitter tests from monitoring.
tests/​UiPath.Caching.Tests/​MultilayerCacheTests.cs Updates failed-write and monitor expectations.
tests/​UiPath.Caching.Tests/​MultilayerCachePerNamePolicyWiringTests.cs Isolates policy tests from monitoring.
tests/​UiPath.Caching.Tests/​InnerTierOutageTests.cs Covers outage and reconnect behavior.
tests/​UiPath.Caching.Tests/​InFlightTests.cs Covers shared-run concurrency and cancellation.
src/​UiPath.Caching/​PublicAPI.Unshipped.txt Records the new public option.
src/​UiPath.Caching/​MultilayerHashCache.cs Coalesces hash reads and retains failed writes.
src/​UiPath.Caching/​MultilayerCacheBase.cs Adds reconnect-triggered local clearing.
src/​UiPath.Caching/​MultilayerCache.cs Coalesces reads and retains failed writes.
src/​UiPath.Caching/​InMemoryRedisCacheOptions.cs Exposes reconnect clearing.
src/​UiPath.Caching/​InMemoryCacheOptions.cs Exposes reconnect clearing.
src/​UiPath.Caching/​InFlightKey.cs Defines coalescing identity.
src/​UiPath.Caching/​InFlight.cs Implements shared asynchronous work.
src/​UiPath.Caching/​IMultilayerCacheOptions.cs Adds the reconnect option contract.
src/​UiPath.Caching.Abstractions/​CacheOptions.cs Enables monitoring by default.
samples/​UiPath.Caching.Sample/​appsettings.all.json Updates sample configuration.
docs/​reference/​settings.md Documents new defaults and setting.
docs/​concepts.md Explains outage behavior.
CHANGELOG.md Records behavioral and breaking changes.

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

Comment thread src/UiPath.Caching/MultilayerCache.cs Outdated
Comment thread src/UiPath.Caching/MultilayerCacheBase.cs Outdated
Comment thread src/UiPath.Caching/MultilayerHashCache.cs Outdated
Comment thread src/UiPath.Caching/MultilayerCacheBase.cs
Comment thread samples/UiPath.Caching.Sample/appsettings.all.json Outdated
@cosmin-staicu
cosmin-staicu requested a balanced review from Copilot October 6, 2026 17:47
@cosmin-staicu 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 Oct 6, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Race conditions can restore stale values after cancellation or recovery, and batch reads bypass the advertised coalescing.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (5)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Batch cache misses bypass request coalescing

src/​UiPath.Caching/​MultilayerCache.cs:1236

This coalescer is only used by scalar reads. Both public batch overloads still send misses directly through _innerCache.GetCacheEntriesAsync (GetInnerAsync and GetCacheEntriesInnerAsync), so concurrent batch reads—or a batch read racing a scalar read—still issue multiple inner reads for the same key. That contradicts the stated coverage of GetAsync/GetCacheEntryAsync; route batch misses through the same per-key flights or explicitly narrow the advertised behavior.

Comment thread src/UiPath.Caching/MultilayerCacheBase.cs

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Flight identity ignores per-call policies, and recovery state transitions contain a race that can lose an outage marker.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Request coalescing ignores caller-specific cache policy

src/​UiPath.Caching/​InFlightKey.cs:4

The coalescing identity omits the caller-supplied CachePolicy. Both cache APIs accept a policy per read, and the shared operation uses the first caller's policy to populate L1 (MultilayerCache.cs:1248 and MultilayerHashCache.cs:574). Concurrent callers requesting different LocalExpiration values therefore silently apply whichever policy joined first; the existing caller-policy contract is explicitly covered in MultilayerCachePerNamePolicyWiringTests.cs:251-269. Include the effective policy in the flight identity, or coalesce only the inner fetch and apply each caller's local policy after awaiting it.

Medium severity Recovery race clears a concurrent failure marker

src/​UiPath.Caching/​Redis/​ConnectionStateMonitor.cs:124

This check-and-reset is not atomic with the failure handler. If recovery observes all states connected, then another state fails and writes _down = 1 before this Exchange, the exchange clears that new outage marker and raises Recovered while a state is down; the later restore will not raise recovery, so outage entries can remain uncleared. Serialize failure/recovery transitions or revalidate without losing a concurrent failure marker.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Cold L1 misses still invoke the disconnected inner tier, and two configuration descriptions incorrectly call reconnect clearing inert.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Previously missed (2)

In code that hasn't changed since last review

Low severity Document conditional cache clearing with Redis broadcasting

docs/​reference/​settings.md:228

This setting is not inert for the in-memory provider when Redis broadcasting is enabled. InMemoryCacheProvider passes the Redis topic provider into MultilayerCacheBase; because that provider implements IConnectionState, the new app-wide monitor default subscribes to it and this option clears the in-memory cache after broadcast recovery. Document that conditional behavior rather than telling users the option has no effect.

Low severity Correct inaccurate comment about Redis reconnect cache clearing

samples/​UiPath.Caching.Sample/​appsettings.all.json:310

This comment is inaccurate when the in-memory provider uses Redis broadcasting: the topic provider is monitored as an IConnectionState, so reconnecting it clears this provider's local cache by default. Describe that conditional effect instead of calling the option inert.

Comment thread src/UiPath.Caching/MultilayerCacheBase.cs

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Initial recovery detection and abandoned-flight cache writes still contain correctness races.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Initialize outage state to detect immediate recovery

src/​UiPath.Caching/​Redis/​ConnectionStateMonitor.cs:21

_down starts at zero and is not set until a failure event or an evaluation of the lazy aggregate. If a monitored source is already disconnected when this monitor is created and emits a restore before the first poll/IsConnected read, RaiseIfRecovered suppresses the recovery. A cold L2 read can populate L1 during that initial broadcast outage without evaluating this monitor, so the stale entry is then never cleared. Initialize the outage marker from the initial aggregate state and cover an initially-down, immediately-restored source.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Default monitoring can prevent cold Redis providers from ever initiating a connection, and duplicate restore forwarding can clear L1 repeatedly.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity

Open (2)

Comment thread src/UiPath.Caching.Abstractions/CacheOptions.cs
Comment thread src/UiPath.Caching/Redis/ConnectionStateMonitor.cs Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Initial Redis connection failures can permanently prevent retries, and abandoned flights can retain resources indefinitely.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (2)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Abandoned flights leak indefinitely when work ignores cancellation

src/​UiPath.Caching/​InFlight.cs:92

When the last waiter leaves, this cancels the work but leaves the flight in _flights. If an inner implementation ignores cancellation and hangs, every abandoned unique key retains its key, state, task, and cancellation source indefinitely unless that exact key is read again. Conditionally remove the key/flight pair as soon as the waiter count reaches zero (while still allowing late work completion to remove safely).

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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The concurrent state-machine and breaking outage defaults warrant final human review despite extensive targeted tests.

Review effort: Balanced
Findings: None

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Stream-loss invalidation can issue concurrent and non-terminal IObserver notifications, and rapid consumer-group replacement can remain undetected.

Review effort: Balanced
Findings: 1 High severity · 2 Medium severity

Open (3)

Comment thread src/UiPath.Caching/Broadcast/Redis/RedisStreamsTopic.cs Outdated
Comment thread src/UiPath.Caching/Broadcast/KeyedSubject.cs Outdated
Comment thread src/UiPath.Caching/Broadcast/Redis/RedisStreamSubjectWriter.cs Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Cancellation can populate L1 after a canceled write, and a reconnect race can skip the Redis Streams gap check.

Review effort: Balanced
Findings: 1 High severity · 2 Medium severity

Open (3)
Resolved since last review (3)

Comment thread src/UiPath.Caching/Broadcast/Redis/RedisStreamSubjectWriter.cs Outdated
Comment thread src/UiPath.Caching/MultilayerCache.cs Outdated
Comment thread src/UiPath.Caching/MultilayerHashCache.cs Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Reconnect-gap detection and subject notification ordering still contain concurrency races.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity

Open (2)
Resolved since last review (3)

Comment thread src/UiPath.Caching/Broadcast/Redis/RedisStreamSubjectWriter.cs Outdated
Comment thread src/UiPath.Caching/Broadcast/KeyedSubject.cs

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

A reconnect can still race past the Streams gap check, and custom stream subjects silently skip loss invalidation.

Review effort: Balanced
Findings: 2 Medium severity

Open (2)
Resolved since last review (2)

Comment thread src/UiPath.Caching/Broadcast/Redis/RedisStreamSubjectWriter.cs
Comment thread src/UiPath.Caching/Broadcast/Redis/RedisStreamsTopic.cs Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The cross-cutting concurrency, connection recovery, and Redis Streams loss-handling changes warrant final human validation.

Review effort: Balanced
Findings: None

Resolved since last review (2)

@cosmin-staicu
cosmin-staicu force-pushed the perf/coalesce-in-flight branch from 8456307 to d3df8dd Compare October 7, 2026 05:53

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Custom public subjects cannot invoke the internal observer invalidation capability, and an additional interface break is undocumented.

Review effort: Balanced
Findings: 2 High severity

Open (2)

Comment thread src/UiPath.Caching/Broadcast/IInvalidatable.cs Outdated
Comment thread src/UiPath.Caching/IMultilayerCacheOptions.cs

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Detached shared-read failures can remain unobserved, and Redis 6 gap detection invalidates L1 even when replay succeeds.

Review effort: Balanced
Findings: 2 Medium severity

Open (2)
Resolved since last review (2)

Comment thread src/UiPath.Caching/Broadcast/Redis/StreamIds.cs
Comment thread src/UiPath.Caching/InFlight.cs

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The race-sensitive concurrency, recovery, Redis Streams, and breaking API changes warrant final human validation.

Review effort: Balanced
Findings: None

Resolved since last review (2)

…he local tier

On a local miss, concurrent reads of one key now wait for a single
inner-tier read and share its result or failure: GetAsync,
GetCacheEntryAsync, the hash reads, and the read GetOrAddAsync makes
before its lock. Reads share only when their key, type and local
lifetimes match. A caller that cancels stops waiting; the shared read is
cancelled once every caller waiting on it has cancelled, and its local
copy is committed only while a caller still waits.

When the inner tier refuses a GetOrAddAsync write, the generated value is
kept locally for LocalMaxExpirationDisconnected, so the callers waiting on
the local lock reuse it instead of each running the generator again.

CacheOptions.ConnectionMonitorEnabled and UseLocalOnlyWhenDisconnected
default to true, so an outage serves and keeps values locally, and a hit
read while the tier is down is kept for the disconnected cap. The new
ClearLocalOnReconnect, true by default, clears the local tier once a
Redis Pub/Sub broadcast is connected again, since invalidations
published meanwhile never arrived. Redis Streams replay them, so their
local tier is kept. RedisConnector.IsConnected is false only when a
multiplexer exists and reports itself down, so a cold connector still
sends its first command.

A Redis Streams consumer now checks after a reconnect whether entries
past its last delivery were trimmed, and treats a group or stream
removed while in use as a loss too. Either way, every local entry on that
topic expires, in each cache that uses it, through the new
IEventSubject<T>.Invalidate().

Multi-key reads stored each hit locally as a key-value pair, so no later
read found it; they now keep the value. A refresh is broadcast as
CacheRefreshed; it was sent as CacheRemoved.

Closes #222.

BREAKING CHANGE: a disconnected inner tier now serves the local copy
instead of answering misses, and the connection monitor is on by
default. Set UseLocalOnlyWhenDisconnected or ConnectionMonitorEnabled to
false for the previous behavior. IEventSubject<T> gains Invalidate(); a
custom subject passed to a topic constructor must implement it, calling
the new public IMissedEventsObserver on its observers. IMultilayerCacheOptions
gains ClearLocalOnReconnect, which a custom implementation must add.

Signed-off-by: Cosmin Staicu <cosmin.staicu@uipath.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Forced Pub/Sub reconnects can retain stale L1 entries, and subject completion can race with subscription.

Review effort: Balanced
Findings: 2 Medium severity

Open (2)

Comment on lines 50 to +52
public void OnNext(T value)
{
if (_completed)
lock (_notifyLock)
TrackEvent(EventReconnected);
ResetIsConnected();
OnReconnected.TryRaise(_telemetryProvider, handler => handler(sender, e));
RaiseIfRecovered();
@sonarqubecloud

sonarqubecloud Bot commented Oct 7, 2026

Copy link
Copy Markdown

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-not-required Maintainer reviewed: no CLA required for this contribution

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Coalesce concurrent inner reads and keep GetOrAdd values when the inner write fails

2 participants