Skip to content

feat(cache)!: floor the resolved expiration at one hour - #151

Open
cosmin-staicu wants to merge 1 commit into
feat/non-nullable-expirationfrom
feat/default-expiration-floor
Open

feat(cache)!: floor the resolved expiration at one hour#151
cosmin-staicu wants to merge 1 commit into
feat/non-nullable-expirationfrom
feat/default-expiration-floor

Conversation

@cosmin-staicu

Copy link
Copy Markdown
Member

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 under CachePolicy.DistributedExpiration and 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:

Before After
nobody touches it 1 h, from the property initializer 1 h
explicitly DefaultExpiration = null (code or config binding) unbounded — no TTL at all 1 h
DefaultExpiration = TimeSpan.MaxValue overflow on the deadline path unbounded

The second row is the bug. CachePolicyFromMultilayerOptions.Build copies DistributedExpiration = src.DefaultExpiration straight through, MultilayerCacheBase.HardcodedDefaults covers only the five Lock fields, and ApplyJitter(null, …) returns null, so the chain bottomed out in CacheClock returning DateTimeOffset.MaxValue — a key written into shared Redis that never expires. It was reachable enough that DistributedCacheRegistrationTests sets DefaultExpiration = null on purpose to exercise the one guard that caught it, and that guard covers only the IDistributedCache adapter: ICache / IHashCache / ISetCache wrote the unbounded key without a word.

HardcodedDefaults already 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 repeating TimeSpan.FromHours(1).
  • The floor is applied on the write paths only: MultilayerCacheBase.ResolveWriteDuration, RedisCacheBase.PolicyDuration / PolicyDeadline / OptionsDeadline, and MultilayerSetCache.LocalWriteExpiration for the memory-only set cache, which extends neither base. ResolveWriteDuration returns TimeSpan rather than TimeSpan? as a result.
  • CacheClock gained a saturating AddSaturating, used by both ToDateTimeOffset overloads: a duration that would run past the representable range lands on DateTimeOffset.MaxValue instead of throwing. Strictly better than the previous overflow, and what makes TimeSpan.MaxValue usable as the configured spelling of unbounded.
  • AddDistributedCache loses 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.AllowUnboundedEntries keeps its meaning and is now the only way to reach an unbounded entry through that adapter without naming a lifetime.
  • Docsinterfaces.md's Expiration table gains the floor row and the TimeSpan.MaxValue row, plus a paragraph making "omission never means forever" explicit; settings.md's three DefaultExpiration rows, DistributedExpiration, DefaultEntryExpiration and AllowUnboundedEntries say what null now 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:

Failed  RedisCacheTests.GetCacheEntry_returns_max_value_when_remote_has_no_ttl_and_no_default_v7

CacheClock is constructed from DefaultPolicy.DistributedExpiration, and the same clock materializes the expiration a read found (Clock.ToDateTimeOffset(await ttlTask) in RedisCache/RedisHashCache). With the floor in the policy, a key that genuinely has no TTL in Redis was reported as expiring in an hour — ICacheEntry.Expiration would have been lying. So the floor lives on the write helpers and HardcodedDefaults carries a comment saying why DistributedExpiration is deliberately absent from it.

Compatibility

Breaking, deliberately — 2.0.0 material.

  • Anyone relying on DefaultExpiration = null (or a config value bound to null) to get entries that never expire now gets 1 h. Migration is TimeSpan.MaxValue, or AllowUnboundedEntries for the IDistributedCache adapter.
  • AddDistributedCache stops 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 changes TimeSpan?TimeSpan (one entry moves in PublicAPI.Shipped.txt); CachePolicy.DefaultDistributedExpiration is added.
  • Two **BREAKING:** CHANGELOG entries under Unreleased.

Test plan

  • dotnet test1520 passed / 0 failed, net8.0 and net10.0, Debug and Release.
  • New: the multilayer write takes the floor when nothing configures a TTL, and stores DateTimeOffset.MaxValue when the default is configured TimeSpan.MaxValue (MultilayerCachePerNamePolicyWiringTests); the Redis refresh does the same pair (RedisCacheTests).
  • Rewritten: 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_fast is untouched and still passes — a configured -1s beats the floor and is still rejected.
  • Three RedisHashCacheTests refresh tests reached the PERSIST branch via DefaultExpiration = null; they now ask for TimeSpan.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.
  • Debug and Release builds clean; only the pre-existing CS0618 StackExchange.Redis obsoletions. Forced GenerateDocumentationFile=true — no new CS1573/CS1574/CS1587.
  • PublicAPI.*.txt updated; CHANGELOG.md updated.

Linked issues

Fixes #

Contributor declaration

  • I signed off my commit per the DCO (git commit -s).
  • I am contributing on behalf of my employer, or in the course of employment / using employer resources. (If checked, your employer may hold IP rights in this work, which can require a signed CLA — a maintainer will follow up. See CONTRIBUTING.md.)

Note

The employer box is left for the author to confirm.

Still open

ICacheOptions.DefaultExpiration and CachePolicy.DistributedExpiration remain two properties modelling one thing, with CachePolicyFromMultilayerOptions.Build bridging 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

…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>
@github-actions github-actions Bot added the needs-cla-review A maintainer should assess whether a signed CLA is required (see CONTRIBUTING.md) label Sep 3, 2026
@github-actions

github-actions Bot commented Sep 3, 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.Abstractions, src/UiPath.Caching)
  • changes shipped public API — possible removal/breaking change (PublicAPI.Shipped.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 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.

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

Comment on lines +229 to +230
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;
Comment on lines +200 to +205
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;
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-cla-review A maintainer should assess whether a signed CLA is required (see CONTRIBUTING.md)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants