Skip to content

feat(cache)!: make the per-call expiration non-nullable - #150

Open
cosmin-staicu wants to merge 1 commit into
refactor/compat-to-extensionsfrom
feat/non-nullable-expiration
Open

feat(cache)!: make the per-call expiration non-nullable#150
cosmin-staicu wants to merge 1 commit into
refactor/compat-to-extensionsfrom
feat/non-nullable-expiration

Conversation

@cosmin-staicu

Copy link
Copy Markdown
Member

Summary

Stacked on #146 — base branch is refactor/compat-to-extensions, so review that one first and this diff shows only the change on top.

TimeSpan? expiration / DateTimeOffset? expiration become TimeSpan / DateTimeOffset on every write across ICache, ICache<T>, IHashCache, IHashCache<T>, ISetCache, ISetCache<T>, their extension surfaces and every implementation. A value that cannot be honored is now rejected rather than absorbed.

Why

The nullable was a redundant third state. Each of these members already has a sibling overload with no expiration parameter, and null resolved through the exact same chain as omitting it:

expiration ?? policy?.DistributedExpiration ?? DefaultExpiration   // RedisCacheBase.ResolveExpiration

So SetAsync(key, v, (TimeSpan?)null, policy, ct) and SetAsync(key, v, policy, ct) were two spellings of one thing. Worse, the two nullable overloads made the null call site uncompilable. I verified this against the shape of ICache before #146 made expiration required:

error CS0121: The call is ambiguous between the following methods or properties:
'I.Set(string, int, Policy?)' and 'I.Set(string, int, System.TimeSpan?, Policy?)'

That is why the old Compat forwarders had to spell out (CachePolicy?)null in six places, and why #146 had to record SetAsync(pairs) with a single argument as a pre-existing shape that does not compile. Both of those go away here: with expiration non-nullable there is exactly one applicable overload for every call shape.

The ecosystem lands on the same split — nullable on the options object (where null means inherit), non-nullable on the per-call argument (where an overload without the parameter already says unspecified):

Library Per-call expiration Default when unspecified
IMemoryCache Set(key, value, TimeSpan absoluteExpirationRelativeToNow) — non-nullable, no default none
HybridCache (.NET 9+) HybridCacheEntryOptions?; Expiration is TimeSpan? 5 min (DefaultExpirationMinutes = 5)
FusionCache FusionCacheEntryOptions?, but public TimeSpan Duration { get; set; }non-nullable; only the tier overrides MemoryCacheDuration / DistributedCacheDuration are TimeSpan? 30 s
EasyCaching Set<T>(key, value, TimeSpan expiration) — mandatory n/a
CacheTower CacheSettings(TimeSpan timeToLive, TimeSpan? staleAfter) — TTL required n/a
LazyCache policy object 20 min
StackExchange.Redis TimeSpan? expiry = nullnull means no TTL none

FusionCache is the closest analogue: the one duration that must always resolve is non-nullable, and only the tier-specific overrides are nullable. That maps onto CachePolicy.LocalExpiration / DistributedExpiration here, which keep their nullables. Note also that the only library where a null per-call expiration means anything is StackExchange.Redis, where it means never expire — the opposite of what it meant on this surface.

Rejecting an expiration that cannot be honored

With the third state gone there is nothing left for a meaningless value to mean, so it is refused at the boundary: a TimeSpan that is not strictly positive, or a DateTimeOffset at or before the cache's current time, raises ArgumentOutOfRangeException (ParamName "expiration") and nothing is written.

Previously such a value was quietly treated as "no expiration" and, on TryAddAsync, answered falseindistinguishable from "somebody else holds the key", which is the one confusion that API's contract explicitly asks callers to design around. SetAsync handed a past deadline turned it into a delete. Both were sentinels doing the work the nullable had already failed to do.

TimeSpan.MaxValue and DateTimeOffset.MaxValue stay valid — they are how the providers spell "no TTL" — and SetInternalAsync's expiration > TimeSpan.Zero guard stays, now only reachable from a misconfigured provider default rather than from a caller.

Changes

  • CacheExpiration (new, UiPath.Caching.Abstractions) — ThrowIfNotPositive, ThrowIfNotFuture, ToDuration, each carrying the caller's parameter name via CallerArgumentExpression. Public so out-of-tree implementations can enforce the same contract.
  • RedisCacheBase gains PolicyDuration / PolicyDeadline for the no-expiration path, CallerDuration / CallerDeadline for the validated one, and OptionsDeadline for the HashCacheEntryOptions seam — the one place where null still means inherit. ResolveExpiration(TimeSpan?, …) / ResolveExpiration(DateTimeOffset?, …) are gone.
  • MultilayerCacheBase gains PolicyDeadline, CallerDeadline and CallerWrite — the last returning the (deadline, duration) pair the write path needs, because the deadline goes to the entry options while the duration drives the L1 cap and the rehydrate trigger.
  • Implementations — each write overload now resolves its own lifetime directly instead of threading a nullable through a shared body, with a private *CoreAsync holding the behavior. MultilayerCache and MultilayerHashCache lose the if (expiration.HasValue) … else … blocks entirely; the two files shrink by ~130 lines between them.
  • UiPathDistributedCache now expresses "no caller TTL and no adapter default" by calling the overload that carries no expiration, rather than by passing null. StoreAsync reads as the three cases it actually has.
  • Docsinterfaces.md gains an Expiration section with the resolution table and the rejection rule, and its code blocks track the new signatures; conditional-add.md's "a non-positive TTL claims nothing" bullet is corrected to say it is rejected, and why answering false there was the wrong shape.
  • Tests — new CacheExpirationTests (the guard, including that the unbounded sentinels stay valid) and CacheExpirationGuardTests (every write overload rejecting both bad shapes through a real in-memory cache, and GetOrAddAsync not calling the generator). The four tests that asserted the old silent no-op now assert the rejection instead, renamed accordingly.

Where nullability stays

Deliberately unchanged, because there null means inherit or absent rather than unspecified:

  • CachePolicy.LocalExpiration / LocalExpirationDisconnected / DistributedExpiration
  • the providers' DefaultExpiration, and UiPathDistributedCacheOptions.DefaultEntryExpiration
  • HashCacheEntryOptions.ExpireTime / TimeToLive
  • ICacheEntry.NewEntry(DateTimeOffset?), CacheEntryBuilder.BuildEntryOptions, MemorySetCache — read-side and internal-seam
  • TimeToLiveAsync / ExpireTimeAsync returns — a key genuinely can have no TTL

NullCache, NullHashCache and NullSetCache read no argument at all — not the key, not the type, not the expiration — so they enforce nothing and keep degrading to "caching is off, carry on". That is called out in the interface remarks and in CacheExpiration's docs rather than left to be discovered.

Compatibility

Breaking, deliberately — this is 2.0.0 material and no compat shims are provided.

  • Source-compatible for a caller passing a real TimeSpan / DateTimeOffset.
  • Source-breaking for a caller forwarding its own TimeSpan?, which now branches:
    if (ttl is { } t) await cache.SetAsync(key, v, t, policy, ct);
    else               await cache.SetAsync(key, v, policy, ct);
    This is the real ergonomic cost and it lands on adapter/forwarding code. The mitigation is the FusionCache one: keep exactly one nullable-accepting seam (the entry-options / policy objects above) rather than nullable on all 20-odd interface members.
  • Behavior-breaking for a caller that passed a non-positive duration or a past deadline and relied on the silent fallback. It now throws.
  • Binary-breaking for every implementor and caller — recompile. 123 entries leave PublicAPI.Shipped.txt (107 Abstractions, 14 Queue, 2 UiPath.Caching).
  • Two **BREAKING:** CHANGELOG entries under Unreleased.

Test plan

  • dotnet test1519 passed / 0 failed, net8.0 and net10.0, Debug and Release. 16 new tests (1513 → 1529 including skips).
  • Debug and Release builds clean; the only warnings are the 8 pre-existing CS0618 StackExchange.Redis obsoletions per TFM.
  • PublicAPI.Shipped.txt / PublicAPI.Unshipped.txt updated in all three packages; the RS0017 removals are the record of the break.
  • Forced GenerateDocumentationFile=true to check every cref I added — no new CS1574/CS1580, and the one CS1587 I introduced (a summary placed under [Fact] instead of above it) is fixed. Doc generation is off in this repo, so these would otherwise fail silently. The remaining CS1573/CS1574/CS1587 under that flag are all in files this PR does not touch.
  • CHANGELOG.md updated.
  • Compile-verified the CS0121 claim above, and that non-nullable-and-required clears it, against a standalone repro of the old overload shape.

One flake, pre-existing

RedisStreamSubjectWriterTests failed once per full-suite run early on — Valid_event_is_written_to_channel_and_acknowledged (Debug, net8.0) and StreamReadGroupAsync_invalid_event (Release, net10.0), a different test each time, both passing in three consecutive isolated runs of that file. This is the same file #146 documents as an unresolved load-dependent flake (it names a third test there, Unknown_command_quarantine_is_lifted_when_the_connection_reconnects), so the family is broader than that PR recorded. Neither test has any contact with expiration. The final Debug and Release runs above are both fully green.

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.

Follow-up, not in this PR

ICacheOptions.DefaultExpiration is TimeSpan? at the bottom of the resolution chain, where null means "store with no TTL at all". That is why DistributedCacheCollectionExtensions has to throw at registration and offer an AllowUnboundedEntries escape hatch, and all three provider options already default it to 1 h. Two things also model "the default TTL" — that property and CachePolicy.DistributedExpiration on the default policy — with BuildDefaultCachePolicyFromMultilayer existing only to bridge them. Collapsing them, or making the property non-nullable with an explicit unbounded opt-in, would delete the registration guard by construction; it touches options binding, CacheClock, and the provider defaults, so it wants its own PR.

🤖 Generated with Claude Code

https://claude.ai/code/session_011QeZZPMo8bFYz98AuPZU2V

…e that cannot be honored

`TimeSpan? expiration` / `DateTimeOffset? expiration` become `TimeSpan` /
`DateTimeOffset` on every write across `ICache`, `ICache<T>`, `IHashCache`,
`IHashCache<T>`, `ISetCache`, `ISetCache<T>`, their extension surfaces and every
implementation.

The nullable was a redundant third state. Each of these members already has a
sibling overload with no `expiration` parameter, and `null` resolved through the
exact same chain as omitting it: `expiration ?? policy.DistributedExpiration ??
options.DefaultExpiration`. It also made `SetAsync(key, value, null)` ambiguous
(CS0121) between the `TimeSpan?` and `DateTimeOffset?` overloads, which is why
the forwarders had to spell out `(CachePolicy?)null` — and why `SetAsync(pairs)`
with a single argument did not compile at all.

With the third state gone there is nothing left for a value that cannot be
honored to mean, so it is refused at the boundary instead of absorbed: a
duration that is not strictly positive, or a deadline at or before the cache's
current time, raises `ArgumentOutOfRangeException` and nothing is written.
Previously such a value was quietly treated as "no expiration" and, on
`TryAddAsync`, answered `false` — indistinguishable from "somebody else holds
the key", the one confusion that API's contract asks callers to design around.
`TimeSpan.MaxValue` / `DateTimeOffset.MaxValue` stay valid: they are how the
providers spell "no TTL".

The new public `CacheExpiration` (`ThrowIfNotPositive`, `ThrowIfNotFuture`,
`ToDuration`) holds the guard for out-of-tree implementations. `RedisCacheBase`
and `MultilayerCacheBase` grew `PolicyDuration` / `PolicyDeadline` for the
no-expiration path and `CallerDuration` / `CallerDeadline` / `CallerWrite` for
the validated one, which is what lets each write overload resolve its lifetime
directly rather than threading a nullable through a shared body. The
implementations shrink accordingly — `MultilayerCache` and `MultilayerHashCache`
lose the `if (expiration.HasValue) … else …` blocks entirely.

Nullability stays where it means *inherit*: `CachePolicy.LocalExpiration` /
`DistributedExpiration`, the providers' `DefaultExpiration`, and
`HashCacheEntryOptions.ExpireTime` / `TimeToLive`. Reads stay nullable too —
`TimeToLiveAsync` / `ExpireTimeAsync` still return `null` for a key with no TTL.
`NullCache`, `NullHashCache` and `NullSetCache` read no argument at all — not
the key, not the type, not the expiration — so they enforce nothing and keep
degrading to "caching is off, carry on".

`UiPathDistributedCache` now expresses "no caller TTL and no adapter default" by
calling the overload that carries no expiration, rather than by passing null.

123 entries leave `PublicAPI.Shipped.txt` (107 in `Abstractions`, 14 in `Queue`,
2 in `UiPath.Caching`); `ICacheEntry.NewEntry(DateTimeOffset?)` keeps its
nullable, being a read-side derive rather than a write.

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 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.Queue, src/UiPath.Caching)
  • changes shipped public API — possible removal/breaking change (PublicAPI.Shipped.txt in src/UiPath.Caching.Abstractions, src/UiPath.Caching.Queue, src/UiPath.Caching)

Other signals

  • large production change (+602 lines under src/)

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 Sep 3, 2026
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.

1 participant