You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
As discussed internally with @tomcrane and @JackLewis-digirati , will merge this without review due to team annual leave.
Confirming I used Claude Opus 4.8 for review, which noted 2 issues that I've resolved.
Ran the tests (25 pass, including the config-binding one) and read the whole diff. The change is sound — no correctness bugs. Two small things worth tidying before the PR.
What the branch does
CacheGroupSettings.Overrides previously existed but had no production callers — the only GetTtl(string named) uses on develop were in its own tests, and the InfoJsonTtlSecs key in the example appsettings was dead config. This branch turns it into a real feature: named becomes an optional argument threaded through GetTtl/GetMemoryCacheOptions, so an override takes precedence over the CacheDuration bucket, and six call sites (asset, adjunct, both orchestration variants, policy, customer-path) opt in via CacheOverrideKeys constants.
Things I specifically checked and am happy with:
Null-object negative caching still wins. In AssetCachingHelper and MemoryAssetTracker, the null-result paths set entry.AbsoluteExpirationRelativeToNow to the Short ttl inside the factory, which is applied after the entry options — so a long configured override can't accidentally pin a null result in cache for hours.
The case-insensitive comparer survives config binding. That was my main worry (the binder can replace an initialised dictionary), and GetTtl_Named_IsCaseInsensitive_WhenBoundFromConfiguration covers it and passes.
Overrides is now initialised, so GetTtl(duration, named) no longer risks a NullReferenceException when a source configures ttls but no overrides — the old GetTtl(string) would have thrown there.
No config regression. Nothing was reading InfoJsonTtlSecs, and the stale copies of that key live only in untracked local appsettings.Development.*.json files, where they'll now simply be ignored.
Two things to fix
CacheOverrideKeys.Known is dead code, and its doc comment overpromises. It's declared as "used to validate configured overrides" (CacheOverrideKeys.cs:53) but nothing references it — a typo'd key in config is silently ignored, which is exactly the failure mode the class comment warns about. Either wire it in (a startup check that logs a warning for unrecognised keys under Caching:TimeToLive:*:Overrides would be cheap and would make the "renaming a value breaks existing overrides" caveat much less scary), or drop the set and reword the comment. I'd lean towards wiring it in, since a silently-ignored ttl override is hard to notice in production.
New compiler warning. CacheSettingsTests.cs:126 uses Dictionary<string, string?> in a project without nullable annotations enabled, producing CS8632. AddInMemoryCollection accepts Dictionary<string, string> fine here, so just dropping the ? clears it.
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
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.
What does this change?
Allow named overrides of caching ttl. There was plumbing for this from a long time ago but wasn't used, this wires it up in a few named places