feat(cache)!: floor the resolved expiration at one hour - #151
feat(cache)!: floor the resolved expiration at one hour#151cosmin-staicu wants to merge 1 commit into
Conversation
…annot mean "forever" `CachePolicy.DefaultDistributedExpiration` (1 hour) is the new floor under `CachePolicy.DistributedExpiration` and the providers' `DefaultExpiration`, applied on every write that carries no caller expiration. The whole chain was nullable with nothing underneath it, so `DefaultExpiration = null` — set in code or bound from configuration — wrote entries with no TTL into shared storage, and `HardcodedDefaults` in `MultilayerCacheBase` already established the pattern for exactly this problem on the lock fields. The 1 hour that four options classes each declared as a property initializer now comes from that one constant. Unbounded entries stay available and now have to be asked for: configure a lifetime of `TimeSpan.MaxValue`, which is already what the providers read as "no TTL" (`SET` with none, `PERSIST` on refresh). `CacheClock` saturates a duration that would run past the representable range to `DateTimeOffset.MaxValue` rather than throwing, so `TimeSpan.MaxValue` works on the deadline path too. The floor sits on the write paths only — `MultilayerCacheBase.ResolveWriteDuration`, `RedisCacheBase.PolicyDuration` / `PolicyDeadline` / `OptionsDeadline`, and `MultilayerSetCache.LocalWriteExpiration` for the memory-only set cache, which extends neither base. It is deliberately not merged into the resolved default policy: `CacheClock` is built from that policy and also materializes the expiration a *read* found, so a key with genuinely no TTL in Redis has to keep reporting `DateTimeOffset.MaxValue` instead of a fabricated `now + default`. `GetCacheEntry_returns_max_value_when_remote_has_no_ttl_and_no_default_v7` catches that, and did when the floor was briefly in the policy. `ResolveWriteDuration` returns `TimeSpan` rather than `TimeSpan?` as a result. `AddDistributedCache` loses its "would store entries without an expiration" registration throw: that state no longer exists. The non-positive check stays, since zero or negative is a value someone configured rather than one they left unset, and `AllowUnboundedEntries` keeps its meaning as the adapter's way to honor `IDistributedCache`'s "until removed" literally. Signed-off-by: Cosmin Staicu <cosmin.staicu@uipath.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011QeZZPMo8bFYz98AuPZU2V Signed-off-by: Cosmin Staicu <cosmin.staicu@uipath.com>
|
🔎 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
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.
|
There was a problem hiding this comment.
🟡 Changes recommended
Unbounded set writes can overflow, jitter changes unbounded semantics, and fallback-based writes disable rehydration.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Introduces a one-hour fallback expiration for writes without a configured TTL while preserving explicit unbounded lifetimes.
Changes:
- Centralizes the default expiration in
CachePolicy. - Applies fallback expiration and saturating deadline conversion.
- Updates registration behavior, tests, API baselines, and documentation.
File summaries
| File | Description |
|---|---|
tests/UiPath.Caching.Tests/Redis/RedisHashCacheTests.cs |
Updates unbounded refresh tests. |
tests/UiPath.Caching.Tests/Redis/RedisCacheTests.cs |
Tests bounded and unbounded refreshes. |
tests/UiPath.Caching.Tests/MultilayerCachePerNamePolicyWiringTests.cs |
Tests multilayer fallback behavior. |
tests/UiPath.Caching.Tests/Config/DistributedCacheRegistrationTests.cs |
Updates null-default registration expectations. |
src/UiPath.Caching/Redis/RedisCacheOptions.cs |
References the centralized default. |
src/UiPath.Caching/Redis/RedisCacheBase.cs |
Floors Redis write expirations. |
src/UiPath.Caching/PublicAPI.Unshipped.txt |
Records the updated API. |
src/UiPath.Caching/PublicAPI.Shipped.txt |
Removes the old signature. |
src/UiPath.Caching/MultilayerCacheBase.cs |
Floors multilayer write durations. |
src/UiPath.Caching/InMemoryRedisCacheOptions.cs |
References the centralized default. |
src/UiPath.Caching/InMemoryCacheOptions.cs |
References the centralized default. |
src/UiPath.Caching/Distributed/UiPathDistributedCacheOptions.cs |
Documents adapter expiration semantics. |
src/UiPath.Caching/Config/DistributedCacheCollectionExtensions.cs |
Revises registration validation. |
src/UiPath.Caching/CacheClock.cs |
Adds saturating deadline conversion. |
src/UiPath.Caching.Queue/MultilayerSetCache.cs |
Floors memory-only set expirations. |
src/UiPath.Caching.Queue/InMemoryQueueCacheOptions.cs |
Centralizes the queue default. |
src/UiPath.Caching.Abstractions/PublicAPI.Unshipped.txt |
Records the new public field. |
src/UiPath.Caching.Abstractions/Config/CachePolicy.cs |
Defines the one-hour fallback. |
docs/reference/settings.md |
Documents configuration behavior. |
docs/reference/interfaces.md |
Documents expiration resolution. |
CHANGELOG.md |
Records breaking changes. |
Review details
- Files reviewed: 21/21 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| requested ??= FromTtl(policy?.DistributedExpiration | ||
| ?? (_inner is NullSetCache ? _defaultExpiration ?? CachePolicy.DefaultDistributedExpiration : null)); |
| ?? CachePolicy.DefaultDistributedExpiration; | ||
| // ApplyJitter is nullable in, nullable out; it only returns null for a null input, so the | ||
| // coalesce is the compiler's price for a resolved value rather than a real branch. | ||
| return ApplyJitter(resolved, policy.JitterMaxDuration, _clock.UtcNow) ?? resolved; |
| var resolved = policy.DistributedExpiration | ||
| ?? _multiLayerCacheOptions.DefaultExpiration | ||
| ?? CachePolicy.DefaultDistributedExpiration; | ||
| // ApplyJitter is nullable in, nullable out; it only returns null for a null input, so the | ||
| // coalesce is the compiler's price for a resolved value rather than a real branch. | ||
| return ApplyJitter(resolved, policy.JitterMaxDuration, _clock.UtcNow) ?? resolved; |
Summary
Stacked on #150 (which is itself stacked on #146) — base branch is
feat/non-nullable-expiration, so review that one first and this diff shows only the change on top.No, we did not hardcode 1 h in the policy. This PR does.
CachePolicy.DefaultDistributedExpiration(1 hour) becomes the floor underCachePolicy.DistributedExpirationand the providers'DefaultExpiration, applied on every write that carries no caller expiration.Why
The 1 hour existed only as a C# property initializer, repeated on four options classes. Nothing in the resolution chain enforced it, so "not set" had two different meanings:
DefaultExpiration = null(code or config binding)DefaultExpiration = TimeSpan.MaxValueThe second row is the bug.
CachePolicyFromMultilayerOptions.BuildcopiesDistributedExpiration = src.DefaultExpirationstraight through,MultilayerCacheBase.HardcodedDefaultscovers only the fiveLockfields, andApplyJitter(null, …)returnsnull, so the chain bottomed out inCacheClockreturningDateTimeOffset.MaxValue— a key written into shared Redis that never expires. It was reachable enough thatDistributedCacheRegistrationTestssetsDefaultExpiration = nullon purpose to exercise the one guard that caught it, and that guard covers only theIDistributedCacheadapter:ICache/IHashCache/ISetCachewrote the unbounded key without a word.HardcodedDefaultsalready establishes the pattern for exactly this problem — "merge a hardcoded policy in last so every field resolves" — which is why the fix is a floor rather than something new.Changes
CachePolicy.DefaultDistributedExpiration(new,public static readonly TimeSpan= 1 h) — one statement of the value. The four options classes' initializers now reference it instead of repeatingTimeSpan.FromHours(1).MultilayerCacheBase.ResolveWriteDuration,RedisCacheBase.PolicyDuration/PolicyDeadline/OptionsDeadline, andMultilayerSetCache.LocalWriteExpirationfor the memory-only set cache, which extends neither base.ResolveWriteDurationreturnsTimeSpanrather thanTimeSpan?as a result.CacheClockgained a saturatingAddSaturating, used by bothToDateTimeOffsetoverloads: a duration that would run past the representable range lands onDateTimeOffset.MaxValueinstead of throwing. Strictly better than the previous overflow, and what makesTimeSpan.MaxValueusable as the configured spelling of unbounded.AddDistributedCacheloses its"would store entries without an expiration"registration throw — that state no longer exists. The non-positive check stays: zero or negative is a value someone configured, not one they left unset.UiPathDistributedCacheOptions.AllowUnboundedEntrieskeeps its meaning and is now the only way to reach an unbounded entry through that adapter without naming a lifetime.interfaces.md's Expiration table gains the floor row and theTimeSpan.MaxValuerow, plus a paragraph making "omission never means forever" explicit;settings.md's threeDefaultExpirationrows,DistributedExpiration,DefaultEntryExpirationandAllowUnboundedEntriessay whatnullnow resolves to.The one thing worth a second look
The floor must not go into the resolved default policy. I tried that first — it is the obvious reading of "hardcode it in the policy" — and it broke a read:
CacheClockis constructed fromDefaultPolicy.DistributedExpiration, and the same clock materializes the expiration a read found (Clock.ToDateTimeOffset(await ttlTask)inRedisCache/RedisHashCache). With the floor in the policy, a key that genuinely has no TTL in Redis was reported as expiring in an hour —ICacheEntry.Expirationwould have been lying. So the floor lives on the write helpers andHardcodedDefaultscarries a comment saying whyDistributedExpirationis deliberately absent from it.Compatibility
Breaking, deliberately — 2.0.0 material.
DefaultExpiration = null(or a config value bound to null) to get entries that never expire now gets 1 h. Migration isTimeSpan.MaxValue, orAllowUnboundedEntriesfor theIDistributedCacheadapter.AddDistributedCachestops throwing for a configuration it used to reject. Registration that previously failed fast now succeeds with a bounded default — strictly fewer startup failures.MultilayerCacheBase.ResolveWriteDuration's return type changesTimeSpan?→TimeSpan(one entry moves inPublicAPI.Shipped.txt);CachePolicy.DefaultDistributedExpirationis added.**BREAKING:**CHANGELOG entries under Unreleased.Test plan
dotnet test— 1520 passed / 0 failed,net8.0andnet10.0, Debug and Release.DateTimeOffset.MaxValuewhen the default is configuredTimeSpan.MaxValue(MultilayerCachePerNamePolicyWiringTests); the Redis refresh does the same pair (RedisCacheTests).Null_default_expiration_without_policy_fails_fast→…_is_bounded_by_the_library_default, since the registration it asserted against is now valid.Non_positive_policy_distributed_expiration_fails_fastis untouched and still passes — a configured-1sbeats the floor and is still rejected.RedisHashCacheTestsrefresh tests reached thePERSISTbranch viaDefaultExpiration = null; they now ask forTimeSpan.MaxValue, so their real subjects (extended props, transaction failure, transaction exception) stay intact and stop depending on "unset" meaning "unbounded".Refresh_no_expiration_no_default's three theory branches had become identical once the per-call expiration stopped being nullable in feat(cache)!: make the per-call expiration non-nullable #150 — it was testing the same path three times. Replaced with the two cases that actually differ now.CS0618StackExchange.Redis obsoletions. ForcedGenerateDocumentationFile=true— no newCS1573/CS1574/CS1587.PublicAPI.*.txtupdated; CHANGELOG.md updated.Linked issues
Fixes #
Contributor declaration
git commit -s).Note
The employer box is left for the author to confirm.
Still open
ICacheOptions.DefaultExpirationandCachePolicy.DistributedExpirationremain two properties modelling one thing, withCachePolicyFromMultilayerOptions.Buildbridging them. The floor makes that harmless — neither can now resolve to unbounded by omission — so collapsing them is a tidy-up rather than a correctness fix, and it wants its own PR.🤖 Generated with Claude Code
https://claude.ai/code/session_011QeZZPMo8bFYz98AuPZU2V