Skip to content

Use named cache ttl#1223

Merged
donaldgray merged 3 commits into
developfrom
feature/named_cache_ttl
Jul 14, 2026
Merged

Use named cache ttl#1223
donaldgray merged 3 commits into
developfrom
feature/named_cache_ttl

Conversation

@donaldgray

Copy link
Copy Markdown
Member

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

  • Caching of Adjuncts + Assets
  • Caching of PathElements
  • Caching of Policies

@donaldgray
donaldgray requested a review from a team as a code owner July 13, 2026 11:39
@donaldgray

Copy link
Copy Markdown
Member Author

Note

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

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

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

@donaldgray
donaldgray merged commit 128aa15 into develop Jul 14, 2026
23 of 26 checks passed
@donaldgray
donaldgray deleted the feature/named_cache_ttl branch July 14, 2026 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant