From a9d8f600760ded14b0bfea32d80a61833170a8c4 Mon Sep 17 00:00:00 2001 From: Daniel Bohlin Date: Mon, 11 May 2026 09:46:31 +0200 Subject: [PATCH] fix(cache): resolve CacheOptions from DI in singleton factories Singletons in AddCache used to close over the local CacheOptions `o`, so a second AddCache call's RegisterType<> registrations were silently ignored even though AppendPreviousRegistrations + RemoveAll clearly intended to support multi-call composition. Now every factory (IFetchQueue, IManagedCacheMonitor, IEternalCache, ITimeToLiveCache, ITimeToIdleCache, IScopeCache) resolves IOptions.Value from the service provider, so the last call's merged options are honored at first resolution. Reported by Eplicta 2026-04-14 (23 production exceptions from "No freshSpan provided and no DefaultFreshSpan configured for cache type EnvironmentOption[]"). Bumps Tharga.Cache 1.0.0 -> 1.1.0. Regression test added in AddCacheIdempotencyTests. --- .../AddCacheIdempotencyTests.cs | 23 ++++++++++ Tharga.Cache/CacheRegistrationExtensions.cs | 18 +++++--- Tharga.Cache/Tharga.Cache.csproj | 2 +- plan/feature.md | 46 +++++++++++++++++++ plan/plan.md | 41 +++++++++++++++++ 5 files changed, 123 insertions(+), 7 deletions(-) create mode 100644 plan/feature.md create mode 100644 plan/plan.md diff --git a/Tharga.Cache.Tests/AddCacheIdempotencyTests.cs b/Tharga.Cache.Tests/AddCacheIdempotencyTests.cs index 0e7b827..edd5df1 100644 --- a/Tharga.Cache.Tests/AddCacheIdempotencyTests.cs +++ b/Tharga.Cache.Tests/AddCacheIdempotencyTests.cs @@ -120,4 +120,27 @@ public void AddCache_CalledTwice_RegistersSingletonOnce() var cacheMonitorCount = services.Count(s => s.ServiceType == typeof(ICacheMonitor)); cacheMonitorCount.Should().Be(1); } + + [Fact] + public async Task AddCache_CalledTwice_SecondCallRegistrationHonoredAtRuntime() + { + //Arrange — first call registers one type, the resolved cache singleton + //must still see the second call's type registration (merged options). + //Regression for Eplicta 2026-04-14: singleton factories used to close + //over the local CacheOptions, so only the first call's `o` was bound. + var services = new ServiceCollection(); + services.AddLogging(); + + services.AddCache(o => o.RegisterType(t => t.DefaultFreshSpan = TimeSpan.FromMinutes(5))); + services.AddCache(o => o.RegisterType(t => t.DefaultFreshSpan = TimeSpan.FromMinutes(5))); + + var provider = services.BuildServiceProvider(); + var cache = provider.GetRequiredService(); + + //Act — second call's type, no explicit freshSpan. Must resolve via merged options. + var act = async () => await cache.GetAsync("test-key", () => Task.FromResult(42)); + + //Assert + await act.Should().NotThrowAsync(); + } } diff --git a/Tharga.Cache/CacheRegistrationExtensions.cs b/Tharga.Cache/CacheRegistrationExtensions.cs index 7fbba44..3b35c5b 100644 --- a/Tharga.Cache/CacheRegistrationExtensions.cs +++ b/Tharga.Cache/CacheRegistrationExtensions.cs @@ -34,15 +34,17 @@ public static void AddCache(this IServiceCollection serviceCollection, Action(s => s.GetService()); serviceCollection.TryAddSingleton(s => { + var opts = s.GetRequiredService>().Value; var cacheMonitor = s.GetService(); var loggerFactory = s.GetService(); var logger = loggerFactory?.CreateLogger(); - return new FetchQueue(cacheMonitor, o, logger); + return new FetchQueue(cacheMonitor, opts, logger); }); serviceCollection.TryAddSingleton(s => { + var opts = s.GetRequiredService>().Value; var persistLoader = s.GetService(); - var cacheMonitor = new CacheMonitor(persistLoader, o); + var cacheMonitor = new CacheMonitor(persistLoader, opts); return cacheMonitor; }); @@ -59,31 +61,35 @@ public static void AddCache(this IServiceCollection serviceCollection, Action(s => { + var opts = s.GetRequiredService>().Value; var cacheMonitor = s.GetService(); var persistLoader = s.GetService(); var fetchQueue = s.GetService(); - return new EternalCache(cacheMonitor, persistLoader, fetchQueue, o); + return new EternalCache(cacheMonitor, persistLoader, fetchQueue, opts); }); serviceCollection.TryAddSingleton(s => { + var opts = s.GetRequiredService>().Value; var cacheMonitor = s.GetService(); var persistLoader = s.GetService(); var fetchQueue = s.GetService(); - return new TimeToLiveCache(cacheMonitor, persistLoader, fetchQueue, o); + return new TimeToLiveCache(cacheMonitor, persistLoader, fetchQueue, opts); }); serviceCollection.TryAddSingleton(s => { + var opts = s.GetRequiredService>().Value; var cacheMonitor = s.GetService(); var persistLoader = s.GetService(); var fetchQueue = s.GetService(); - return new TimeToIdleCache(cacheMonitor, persistLoader, fetchQueue, o); + return new TimeToIdleCache(cacheMonitor, persistLoader, fetchQueue, opts); }); serviceCollection.TryAddScoped(s => { + var opts = s.GetRequiredService>().Value; var cacheMonitor = s.GetService(); var persistLoader = s.GetService(); var fetchQueue = s.GetService(); - return new EternalCache(cacheMonitor, persistLoader, fetchQueue, o); + return new EternalCache(cacheMonitor, persistLoader, fetchQueue, opts); }); serviceCollection.TryAddSingleton(); diff --git a/Tharga.Cache/Tharga.Cache.csproj b/Tharga.Cache/Tharga.Cache.csproj index 31d44c6..4526dd0 100644 --- a/Tharga.Cache/Tharga.Cache.csproj +++ b/Tharga.Cache/Tharga.Cache.csproj @@ -3,7 +3,7 @@ net8.0;net9.0;net10.0 enable - 1.0.0 + 1.1.0 Daniel Bohlin Thargelion AB Tharga Cache diff --git a/plan/feature.md b/plan/feature.md new file mode 100644 index 0000000..e371e58 --- /dev/null +++ b/plan/feature.md @@ -0,0 +1,46 @@ +# Feature: Fix AddCache singleton options capture + +## Goal + +`AddCache` is intended to support being called multiple times — its own `AppendPreviousRegistrations` helper plus the `RemoveAll>` + re-add pattern documents that intent. In practice the second call's registrations are silently lost because the singleton factories close over the local `CacheOptions o` instead of resolving `IOptions` from the service provider. The first call's `o` is the one bound to every singleton, regardless of what later calls register. + +Switch every affected factory to resolve `IOptions` from the SP. Singletons are constructed lazily on first resolution, after composition is complete, so they pick up the final merged options. + +## Source + +Eplicta request (2026-04-14, High) in `$DOC_ROOT/Tharga/Requests.md`. Reproduces in any app that combines its own `AddCache` with `AddQuilt4NetApplicationInsightsClient`, which calls `AddCache` transitively. Eplicta observes 23 production exceptions on `EnvironmentOption[]`: `InvalidOperationException: "No freshSpan provided and no DefaultFreshSpan configured for cache type ..."` even though the type *was* registered in the second call — just not in the `o` the first call's closure captured. + +## Scope + +In `Tharga.Cache/CacheRegistrationExtensions.cs`, the 6 factories that close over `o`: + +1. `IFetchQueue` (line 35) — uses `o.MaxConcurrentFetchCount` +2. `IManagedCacheMonitor` (line 42) — passes `o` to `CacheMonitor` +3. `IEternalCache` (line 60) +4. `ITimeToLiveCache` (line 67) +5. `ITimeToIdleCache` (line 74) +6. `IScopeCache` (line 81) — scoped, but same closure bug applies + +Note: the Eplicta request only enumerated the 4 cache types. The two infrastructure singletons (`IFetchQueue`, `IManagedCacheMonitor`) have the same closure-capture bug and need the same fix for consistency. Without this they would still see first-call options if `MaxConcurrentFetchCount` / `WatchDogInterval` ever change between calls. + +Out of scope: +- Public API changes +- `RegisterAllIPersistImplementations`, `InvokeAllPersistRegistrations` — these run eagerly during `AddCache` itself, so reading the local `o` is correct. +- `o.RegisterConfigurations(serviceCollection)` callbacks — eager, local `o` is correct. +- Tharga.Console 3.7.4 → 4.0.0 upgrade in `Tharga.Cache.Console` (deferred to a separate change). + +## Acceptance criteria + +- All 6 factories resolve `IOptions` from the SP. No factory closes over the local `o`. +- New regression test in `AddCacheIdempotencyTests.cs`: call `AddCache` twice with different `RegisterType<>` calls, build the SP, resolve a cache, and prove the **second** call's registrations are honored (e.g. `DefaultFreshSpan` set in the second call works at runtime). +- All existing tests in `Tharga.Cache.Tests` still pass. +- `Tharga.Cache.csproj` version bumped: `1.0.0` → `1.1.0` (minor; user-chosen). +- README updated only if it documents one-call-only registration (check `Tharga.Cache/README.md` for guidance about `AddCache` call count). + +## Done condition + +- Feature branch `feature/fix-addcache-options-capture` pushed +- PR opened from `feature/fix-addcache-options-capture` → `master` (GitHub Actions branching strategy) +- CI green +- User confirms the fix as done +- Eplicta workaround referenced in `Core/Eplicta.Aggregator/Program.cs` (per the Requests.md note) can be removed once the new `Tharga.Cache` package version is consumed — tracked as follow-up in `Requests.md` diff --git a/plan/plan.md b/plan/plan.md new file mode 100644 index 0000000..2a1cd0c --- /dev/null +++ b/plan/plan.md @@ -0,0 +1,41 @@ +# Plan: Fix AddCache singleton options capture + +## Steps + +- [x] **1. Write the regression test first (red)** + Added `AddCache_CalledTwice_SecondCallRegistrationHonoredAtRuntime` to `AddCacheIdempotencyTests.cs`. Calls `AddCache` twice with `RegisterType` then `RegisterType`, both with `DefaultFreshSpan`. Resolves `ITimeToLiveCache`, calls `GetAsync` with no explicit freshSpan, asserts it does not throw. + +- [x] **2. Confirmed test fails on master** + Failed with the exact Eplicta-reported exception: `InvalidOperationException: No freshSpan provided and no DefaultFreshSpan configured for cache type Int32` at `TimeCacheBase.GetDefaultFreshSpan[T]()`. Bug reproduced. + +- [x] **3. Fix the 6 factories in `CacheRegistrationExtensions.AddCache`** + All factories now call `var opts = s.GetRequiredService>().Value;` and pass `opts` instead of the captured `o`. Fixed: `IFetchQueue`, `IManagedCacheMonitor`, `IEternalCache`, `ITimeToLiveCache`, `ITimeToIdleCache`, `IScopeCache`. + +- [x] **4. Run the test suite** + - `Tharga.Cache.Tests`: 474/474 pass (regression test included, plus one re-run for a flaky time-critical concurrency test unrelated to this fix). + - `Tharga.Cache.Redis.Tests`: 2/2 pass. + - `Tharga.Cache.File.Tests`: 2 skipped (file-infra-dependent). + - `Tharga.Cache.MongoDB.Tests`: no tests discovered (integration-only project). + +- [x] **5. Check README for one-call guidance** + Grepped `README.md` and `Tharga.Cache/README.md` — no text suggesting `AddCache` must be called once. No README change needed. + +- [x] **6. Bump version `Tharga.Cache.csproj` 1.0.0 → 1.1.0** + Done. + +- [~] **7. Commit + push** + Conventional commit: `fix: resolve CacheOptions from DI in singleton factories so multi-call AddCache merges correctly`. Push branch. + +- [ ] **8. Open PR `feature/fix-addcache-options-capture` → `master`** + +- [ ] **9. Wait for CI green; hand to user** + Do not merge — PR is the review gate. User to confirm done, then per shared-instructions: archive `plan/feature.md` to `$DOC_ROOT/Tharga/plans/Toolkit/Cache/done/fix-addcache-options-capture.md`, delete `plan/`, mark the Eplicta request `Status: Done` with completion date and add the follow-up entry in `Requests.md` (Eplicta should bump Tharga.Cache to 1.1.0 and remove their workaround in `Core/Eplicta.Aggregator/Program.cs`). + +## Notes + +- Scope was expanded from the 4 caches Eplicta listed to all 6 factories. `IFetchQueue` (reads `MaxConcurrentFetchCount`) and `IManagedCacheMonitor` (passes `CacheOptions` to `CacheMonitor`) have the same closure-capture bug and were fixed for consistency. +- First-call-wins on type registrations preserved (`AppendPreviousRegistrations` uses `TryAdd`). Only the singleton-options binding changed — singletons now lazily resolve `IOptions` from the SP, picking up the merged view from the last `AddCache` call. + +## Last session + +Implementation complete on `feature/fix-addcache-options-capture`. All 6 factories rewritten, regression test added and passing, version bumped to 1.1.0. Pending: commit, push, open PR, wait for CI, user confirmation, then archive plan + close Eplicta request with follow-up.