Spec 014: Caching decorator — requirements, ADR 0021, and task breakdown (#291)

[iancooper]

Summary

Planning and design phase for the query caching decorator (issue #291) — a declarative [CacheableQuery] pipeline stage built on Microsoft's HybridCache. This PR contains specification artifacts only; no implementation code yet — it establishes the approved requirements, the accepted architecture, and the reviewed task breakdown that the implementation will follow.

What's here

• docs/adr/0021-caching-decorator-architecture.md (Accepted) — the single architectural decision: concrete decorator over HybridCache with GetOrCreateAsync as the pipeline short-circuit; pluggable ICacheKeyGenerator; TTL-only lifetime plus a well-known Bag-key tag seam; hit/miss metrics derived from traces (reusing the ADR 0018 subsystem) under an independent opt-out toggle; net8.0/net9.0-only packaging.
• specs/014-Caching-Decorator/requirements.md (Approved) — FR1–FR14 + acceptance criteria.
• specs/014-Caching-Decorator/ralph-tasks.md (Approved) — 31 dependency-ordered, unattended-TDD tasks covering the three-package split (caching decorator with no OTel dependency, core convention constants, diagnostics CacheMeter), the runtime-type key-generation trap with mandatory end-to-end QueryProcessor tests, and every fail-fast/boundary path.
• Review records — review-requirements.md, review-design.md, and review-tasks.md (adversarial review reached PASS on round 3).

Design highlights baked into the tasks

• Runtime-type key generation — decorators are closed over IQuery<TResult>, so key/IAmCacheable detection reflects on query.GetType(); each core behaviour is proven end-to-end through a real QueryProcessor (guards against the ADR 0020 false-green).
• Three-package split — Paramore.Darker.Caching records only a core Activity span attribute; the counter is derived in Paramore.Darker.Extensions.Diagnostics; convention constants live in core.
• Fail-fast — non-positive expiry (at pipeline build), missing HybridCache, and null/empty runtime CacheKey all surface ConfigurationException.
• Switchable backing cache — Microsoft HybridCache ↔ FusionCache purely via DI, proven with the real FusionCache package in tests.

Not in this PR

Implementation (the 31 tasks) follows in subsequent work on this branch / a follow-up.

Closes #291 once implementation lands.

🤖 Generated with Claude Code

[iancooper]

Code review follow-up — correctness fixes to the caching decorator

Ran a correctness-focused review over the caching changes on this branch. It surfaced four findings, now each fixed via TDD (failing test first, then minimal fix) and committed to this PR. Full filtered solution is green on net8.0 and net9.0 (26 caching tests + all other suites).

#	Severity	Finding	Fix	Commit
CR-1	High (silent wrong result)	DefaultCacheKeyGenerator serialized each property against its declared type, so data behind an interface/abstract-typed property collapsed to {} — distinct queries produced colliding cache keys and one query could be served another's cached result.	Serialize against value.GetType() (runtime type); boxed value types still emit as JSON primitives (existing determinism test confirms).	e399702
CR-2	Medium (crash)	Key generation reflected over indexer properties; GetValue with no index arg threw TargetParameterCountException, so any query exposing an indexer failed before the cache was consulted.	Exclude indexers from the property set (GetIndexParameters().Length == 0).	a5f574c
CR-3	Medium (deadlock)	The sync decorator's sync-over-async wait could deadlock under a single-threaded SynchronizationContext (classic ASP.NET / UI thread): the cache's internal continuation is captured onto the originating thread, which the blocking wait then holds.	Suppress the ambient SynchronizationContext around the cache call in a try/finally (the sync-over-async equivalent of ConfigureAwait(false)). Continuations resume on the thread pool; the synchronous L1 fast-path is preserved (no thread-pool hop on hits); the caller never observes the suppression.	30c935c
CR-4	Cleanup	Both decorators' remarks claimed the state overload was used to "avoid capturing variables in a closure", but the factory still closes over the hit/miss flag.	Reworded to describe accurately what is and isn't captured.	f36720c

Notes

• CR-1 is the one worth a second look — it returned wrong data with no error.
• CR-3 ships a deterministic deadlock regression test (When_executed_under_single_threaded_synchronization_context_should_not_deadlock) using a non-pumping SynchronizationContext double plus a join timeout, so it fails fast rather than hanging CI if the bug ever returns. Context suppression was chosen over a full Task.Run offload after weighing caller-expectation risk — it keeps the L1 hit off the thread pool and leaves any caller-owned TaskScheduler untouched.