feat(cache)!: make the per-call expiration non-nullable - #150
Open
cosmin-staicu wants to merge 1 commit into
Open
feat(cache)!: make the per-call expiration non-nullable#150cosmin-staicu wants to merge 1 commit into
cosmin-staicu wants to merge 1 commit into
Conversation
…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>
cosmin-staicu
requested review from
alinahornet,
cosminvlad,
litheon,
lucianaparaschivei and
razvalex
as code owners
September 3, 2026 10:15
|
🔎 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
Other 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.
|
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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? expirationbecomeTimeSpan/DateTimeOffseton every write acrossICache,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
expirationparameter, andnullresolved through the exact same chain as omitting it:So
SetAsync(key, v, (TimeSpan?)null, policy, ct)andSetAsync(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 ofICachebefore #146 madeexpirationrequired:That is why the old
Compatforwarders had to spell out(CachePolicy?)nullin six places, and why #146 had to recordSetAsync(pairs)with a single argument as a pre-existing shape that does not compile. Both of those go away here: withexpirationnon-nullable there is exactly one applicable overload for every call shape.The ecosystem lands on the same split — nullable on the options object (where
nullmeans inherit), non-nullable on the per-call argument (where an overload without the parameter already says unspecified):IMemoryCacheSet(key, value, TimeSpan absoluteExpirationRelativeToNow)— non-nullable, no defaultHybridCache(.NET 9+)HybridCacheEntryOptions?;ExpirationisTimeSpan?DefaultExpirationMinutes = 5)FusionCacheEntryOptions?, butpublic TimeSpan Duration { get; set; }— non-nullable; only the tier overridesMemoryCacheDuration/DistributedCacheDurationareTimeSpan?Set<T>(key, value, TimeSpan expiration)— mandatoryCacheSettings(TimeSpan timeToLive, TimeSpan? staleAfter)— TTL requiredTimeSpan? expiry = null→nullmeans no TTLFusionCache 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/DistributedExpirationhere, 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
TimeSpanthat is not strictly positive, or aDateTimeOffsetat or before the cache's current time, raisesArgumentOutOfRangeException(ParamName"expiration") and nothing is written.Previously such a value was quietly treated as "no expiration" and, on
TryAddAsync, answeredfalse— indistinguishable from "somebody else holds the key", which is the one confusion that API's contract explicitly asks callers to design around.SetAsynchanded a past deadline turned it into a delete. Both were sentinels doing the work the nullable had already failed to do.TimeSpan.MaxValueandDateTimeOffset.MaxValuestay valid — they are how the providers spell "no TTL" — andSetInternalAsync'sexpiration > TimeSpan.Zeroguard 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 viaCallerArgumentExpression. Public so out-of-tree implementations can enforce the same contract.RedisCacheBasegainsPolicyDuration/PolicyDeadlinefor the no-expiration path,CallerDuration/CallerDeadlinefor the validated one, andOptionsDeadlinefor theHashCacheEntryOptionsseam — the one place wherenullstill means inherit.ResolveExpiration(TimeSpan?, …)/ResolveExpiration(DateTimeOffset?, …)are gone.MultilayerCacheBasegainsPolicyDeadline,CallerDeadlineandCallerWrite— 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.*CoreAsyncholding the behavior.MultilayerCacheandMultilayerHashCachelose theif (expiration.HasValue) … else …blocks entirely; the two files shrink by ~130 lines between them.UiPathDistributedCachenow expresses "no caller TTL and no adapter default" by calling the overload that carries no expiration, rather than by passing null.StoreAsyncreads as the three cases it actually has.interfaces.mdgains 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 answeringfalsethere was the wrong shape.CacheExpirationTests(the guard, including that the unbounded sentinels stay valid) andCacheExpirationGuardTests(every write overload rejecting both bad shapes through a real in-memory cache, andGetOrAddAsyncnot 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
nullmeans inherit or absent rather than unspecified:CachePolicy.LocalExpiration/LocalExpirationDisconnected/DistributedExpirationDefaultExpiration, andUiPathDistributedCacheOptions.DefaultEntryExpirationHashCacheEntryOptions.ExpireTime/TimeToLiveICacheEntry.NewEntry(DateTimeOffset?),CacheEntryBuilder.BuildEntryOptions,MemorySetCache— read-side and internal-seamTimeToLiveAsync/ExpireTimeAsyncreturns — a key genuinely can have no TTLNullCache,NullHashCacheandNullSetCacheread 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 inCacheExpiration's docs rather than left to be discovered.Compatibility
Breaking, deliberately — this is 2.0.0 material and no compat shims are provided.
TimeSpan/DateTimeOffset.TimeSpan?, which now branches:PublicAPI.Shipped.txt(107Abstractions, 14Queue, 2UiPath.Caching).**BREAKING:**CHANGELOG entries under Unreleased.Test plan
dotnet test— 1519 passed / 0 failed,net8.0andnet10.0, Debug and Release. 16 new tests (1513 → 1529 including skips).CS0618StackExchange.Redis obsoletions per TFM.PublicAPI.Shipped.txt/PublicAPI.Unshipped.txtupdated in all three packages; the RS0017 removals are the record of the break.GenerateDocumentationFile=trueto check everycrefI added — no newCS1574/CS1580, and the oneCS1587I 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 remainingCS1573/CS1574/CS1587under that flag are all in files this PR does not touch.One flake, pre-existing
RedisStreamSubjectWriterTestsfailed once per full-suite run early on —Valid_event_is_written_to_channel_and_acknowledged(Debug,net8.0) andStreamReadGroupAsync_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
git commit -s).Note
The employer box is left for the author to confirm.
Follow-up, not in this PR
ICacheOptions.DefaultExpirationisTimeSpan?at the bottom of the resolution chain, wherenullmeans "store with no TTL at all". That is whyDistributedCacheCollectionExtensionshas to throw at registration and offer anAllowUnboundedEntriesescape hatch, and all three provider options already default it to 1 h. Two things also model "the default TTL" — that property andCachePolicy.DistributedExpirationon the default policy — withBuildDefaultCachePolicyFromMultilayerexisting 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