Architecture remediation: nine review candidates (c1–c9)

CHANGELOG.md

@@ -126,8 +126,8 @@ Current milestones:
   `cacheNullValues` and early-expiration attributes (a writer that never filled
   the Bloom filter could leave a Bloom-enabled reader judging the key
   "definitely missing"). Write-only methods now honour their own declaration —
-  including `ttl()`, whose annotation default is 60 seconds, so such a method's
-  entries now expire after 60s where the cache-level TTL used to apply. A
+  including an explicitly set `ttl()`, which now overrides the cache-level TTL
+  on that path where it previously could not. A
   method that also declares `@RedisCacheable` keeps using the read-side
   declaration, because the read-through write-back is part of the read
   operation.
@@ -237,6 +237,101 @@ Current milestones:
   technical increment is **Bloom + TTL jitter + pluggable responsibility
   chain**.
 
+### Architecture remediation (2026-09-22 review, c1–c9)
+
+- **One owner for failure reporting (c1)** — package-private `FailureReport`
+  emits the sanctioned WARN/ERROR plus paired DEBUG line for every failure site,
+  so callers state what failed instead of choosing log levels and re-deriving
+  the fingerprint rule; no `keyFingerprint` concatenation or hand-paired DEBUG
+  remains in `src/main`. Levels, count-once accounting, the failure metric
+  dimensions and the raw-key privacy rule are unchanged; message rendering
+  (field order and punctuation) is now produced by the one owner.
+- **Metrics resolved at one non-null seam (c2)** — `resi-cache.metrics.enabled`
+  is read in exactly one place and handed to every caller as a non-null metrics
+  seam; when the opt-in is off (or no `MeterRegistry` bean exists) that seam
+  publishes nothing and the application's registry beans are not even resolved,
+  so an ambiguous registry set cannot fail an assembly that disabled metrics.
+  The opt-in is declared in
+  `additional-spring-configuration-metadata.json`. The key has no
+  `RedisProCacheProperties` field: it is fixed, and binding it would require a
+  tenth public nested type (`STABILITY.md` §4 churn) for an assembly detail.
+- ⚠️ **Protection health is no longer gated by the metrics opt-in (c2)** —
+  `RedisCacheHealthIndicator` now reports Redis connectivity and protection
+  degradation regardless of `resi-cache.metrics.enabled`; previously the
+  unrelated metrics switch could suppress the indicator.
+- **The disabled metrics seam does no work (c2)** — with
+  `resi-cache.metrics.enabled` off (the default) the resolved seam is a shared
+  stateless registry, and every consumer short-circuits on it rather than
+  registering through it: the chain's timer observer, the fired-counter observer
+  and the failure reporter return before building a key; the per-cache registry,
+  the handler attach hook, the refresh-task metrics and the Bloom filter take
+  their existing null path; and the migration engine skips its per-key metric
+  record. A disabled application therefore allocates and keeps no meter, no
+  timer and no per-cache entry, and the chain timer observer neither reads the
+  clock nor allocates a scope token on that path. Two earlier revisions of this
+  change had moved that work instead of removing it — the retention into the
+  timer observer's own per-cache-name map, and the per-key allocation into the
+  migration engine. Metric names, tag keys and tag values are unchanged.
+- **The degraded-protection warning fires once per context and reports the real
+  mode (c2)** — `RedisCacheHealthIndicator` reports the actual sync-protection
+  state, derived in one place by `SyncSupport`: `protection.degraded=local-only`
+  only when no distributed lock backend is present and
+  `resi-cache.sync-lock.local-only=true` was explicitly enabled,
+  `protection.degraded=fail-fast` when no backend is present without that opt-in
+  (so `sync=true` rejects instead of degrading), and no protection detail when a
+  backend exists. The associated WARN fires at most once per context instead of
+  on every `/actuator/health` probe, which matters where a load balancer or
+  orchestrator probes frequently; the state itself is reported in every health
+  response's details.
+- **Observer order owned by the observer class (c3)** — the four standard
+  observers declare `@Order(1..4)` on the class instead of on their `@Bean`
+  methods, and the factory registers observers in the Spring-resolved injection
+  order instead of re-sorting by an annotation only it could see; observers
+  ordered through `Ordered`, a `@Bean`-method `@Order`, a meta-annotation or a
+  proxy keep the position Spring gave them. The dispatch itself is typed at both
+  levels — the chain-level and node-level results reach the end hooks as
+  `CacheResult` / `HandlerResult` without a cast — and the chain observer
+  protocol no longer checks its own scope token's runtime type.
+- ⚠️ **`ChainObserver.beforeNode` removed (c3)** — the hook had no production
+  implementer. An extension that overrode it must move that work to
+  `onNodeStart` / `afterNode`; every other hook keeps its name and semantics.
+  See `STABILITY.md` §4 for the migration note.
+- ⚠️ **An annotated method without an explicit `ttl` now takes the configured TTL (c8)** —
+  `@RedisCacheable#ttl` and `@RedisCachePut#ttl` default to `0`, which `TtlPolicy` reads as
+  "no method-level declaration", so a method that does not set `ttl` falls through to
+  `resi-cache.default-ttl` (default 30 minutes) instead of the previous implicit 60 seconds.
+  An explicitly set positive `ttl` still wins and still applies its jitter.
+  `TtlPolicy.DEFAULT_TTL_SECONDS` and its `null`-`Duration` branch are gone, and a zero,
+  negative or `null` TTL all mean "no expiry", matching Spring Data Redis's own
+  `DefaultRedisCacheWriter.shouldExpireWithin`; a direct writer/SPI `put(…, null)` therefore
+  writes a persistent entry where it previously wrote a 60-second one.
+- **Handler identity declared once (c4)** — order slot, protection disable name,
+  metric/log tag and the ordering requirement of each slot are declared
+  alongside `HandlerOrder` and resolved by `cache/HandlerIdentity`; emitted tag
+  values are unchanged, and a handler with no declared identity keeps the
+  previous class-simple-name tag.
+- **One assembly root per boundary (c5)** — the runtime context and the operator
+  CLI each name the beans they own by class instead of regex package-scan
+  patterns, `resi-cache.enabled` is declared once, and the bean-backoff
+  invariant enumerates `@Bean` methods.
+- **One projection feeds both operation views (c6)** — the AOP operation and the
+  policy view are derived from one `RedisCacheAttributes` instance per
+  annotation, so a new annotation field has one mapping site and the policy
+  lookup no longer depends on declaration order; the Spring/policy view split
+  itself is retained.
+- **Unreachable degradation modes deleted (c7)** — dead null guards, the
+  `NullValueEncoder` wrapper and test-only factory surface are gone. The
+  engine-side rejection of a malformed `HandlerResult` is retained because
+  `STABILITY.md` §4 documents it.
+- **TTL precedence in one module (c8)** — `TtlPolicy` owns the ordered
+  resolution and both defaults; every path keeps its previous effective TTL and
+  no default changed. See [`COMPATIBILITY.md`](./COMPATIBILITY.md) and
+  [`docs/REFERENCE.md`](docs/REFERENCE.md).
+- **Synchronization lifecycle owned by its state (c9)** — the single-flight
+  lifecycle (enter → complete → exit → cleanup, exactly once) now lives with the
+  state it mutates and the seven-argument static re-entry is gone; lock
+  acquire/release order and failure paths are unchanged.
+
 ### Fixed
 
 - **`resi-cache.serializer.*` properties were silently dropped by the

COMPATIBILITY.md

@@ -56,9 +56,13 @@ baseline.
   sync operation fails fast unless `resi-cache.sync-lock.local-only=true` is
   explicitly configured. |
 | **Micrometer / Actuator** | Optional | Cache metrics require
-  `resi-cache.metrics.enabled=true` (default OFF) and a `MeterRegistry`.
-  `RedisCacheHealthIndicator` additionally requires Actuator, the
-  `HealthIndicator` class, and the same metrics property set to `true`. |
+  `resi-cache.metrics.enabled=true` (default OFF) and a `MeterRegistry`;
+  otherwise the resolved metrics seam is a no-op adapter.
+  `RedisCacheHealthIndicator` requires Actuator and the `HealthIndicator`
+  class; it is not gated on the metrics property, so an application with
+  Actuator and Redis assembles it and each `/actuator/health` probe issues a
+  synchronous Redis `connection.ping()` round trip (see the probe-cost note in
+  [`docs/OPERATIONS.md`](docs/OPERATIONS.md)). |
 | **Caffeine** | Bundled | Used internally for the local hash cache and
   bloom-filter bitset; not exposed as a multi-level cache. |
 
@@ -108,6 +112,42 @@ not require a cache flush.
   `clear` deletion counts and PUT_IF_ABSENT insertion. `withStatisticsCollector`
   fully rebinds statistics; lock-wait duration remains unreported (zero).
 - **Class-level cache annotations**: Spring operation resolution sees class-level ResiCache annotations, but the annotation chain does not apply their policy fields to methods without method-level annotations; this behavior is unchanged from `main`.
+- **`value` and `cacheNames` resolution**: the three annotations are not
+  `@AliasFor`-linked, so one declaration may set both attributes. There is one
+  resolution for both faces (`RedisCacheAttributesProjector.resolveCacheNames`)
+  and **`value` wins**; `cacheNames` is the fallback and only applies when
+  `value` is empty. A declaration that sets both targets the `value` cache.
+  This is the operation face's `main` behaviour, so the cache in use is
+  unchanged. The policy face changes for that same both-set declaration: on
+  `main` the operation targeted `value` while the policy snapshot was
+  registered under `cacheNames`, so the declared policy silently did not apply
+  to the cache that was used. Both faces now resolve to `value`, and the policy
+  applies to the cache in use.
+- **TTL default precedence**: one module owns the resolution (`TtlPolicy`; the
+  ordered rule is specified in [`docs/REFERENCE.md`](docs/REFERENCE.md)). A
+  method-level `ttl` greater than zero is the only declaration that overrides
+  the configured cache TTL. An annotated method that does not set `ttl` (the
+  attribute's value is then `0`, i.e. no declaration) expires its entries
+  after `resi-cache.default-ttl` (default `30m`, per-cache `caches.*.ttl`
+  overrides it), exactly like a plain Spring `@Cacheable` in `SELECTIVE` mode.
+  There is no second implicit TTL default: a zero or negative Duration
+  parameter means an entry without expiry, and a `null` parameter is treated
+  the same way — Spring Data Redis 4.0 expresses "no expiry" as
+  `Duration.ZERO` (`RedisCacheConfiguration`'s default `TtlFunction` is
+  `persistent()`, and `entryTtl` rejects `null`), so a write path never
+  carries a `null` TTL.
+- **Annotation TTL fallback (behaviour change)**: on the previous build line
+  the `@RedisCacheable`/`@RedisCachePut` `ttl` attribute defaulted to `60`
+  seconds, so an annotated method without an explicit `ttl` expired its
+  entries after `60s` even when `resi-cache.default-ttl` was configured. The
+  annotation-side implicit `60` is removed: such a method now uses the
+  configured default (`30m` unless overridden), and `60` is no longer
+  reachable from the resolution path. Methods that set `ttl` explicitly are
+  unaffected. Deployments relying on the old `60s` expiry for methods that
+  omit `ttl` must either set `ttl` explicitly or set `resi-cache.default-ttl`.
+  A cache configured with no expiry (a caller-supplied
+  `RedisCacheConfiguration`) stays without expiry instead of receiving a
+  `60s` entry lifetime.
 - **Refresh metadata**: the version-2 envelope persists the fields required by
   early-expiration policy and version CAS (`ttl`, `createdTime`, access/visit
   counters, `expired`, and `version`). `startNanoTime` is process-local and is

STABILITY.md

@@ -130,17 +130,34 @@ custom implementation must satisfy.
 ### Observers
 
 1. **Hook order** per chain execution: `onChainStart` → per node
-   [`onNodeStart` → `beforeNode` → `handler.handle(context, next)` →
+   [`onNodeStart` → `handler.handle(context, next)` →
    `afterNode` → `onNodeEnd`] → `onChainEnd`. Multiple observers run in registration
    (`@Order`) order for every hook.
 2. **Scope tokens**: each `on*Start` returns a per-call token the engine
    pairs back to the same observer's `on*End` in a `finally` block (on
    handler exception `onNodeEnd` receives a `null` result — recover the
    token, do not fabricate decisions). Tokens carry per-call state; observers
-   must be thread-safe and stateless between calls.
+   must be thread-safe and stateless between calls. The engine pairs by
+   observer registration index, so the token an `on*End` hook receives is
+   always the reference returned by *that* observer's matching `on*Start`: an
+   observer may cast its own token to its private token type without a
+   runtime type check.
 3. **Exception isolation**: observer hook failures are caught and logged by
    the engine; they never change chain control flow.
 
+⚠️ **BREAKING — `beforeNode` removed (0.x)**: the SPI hook
+`ChainObserver.beforeNode(CacheHandler, CacheContext)` no longer exists; it had
+zero production implementers and no other hook was renamed, retyped or
+reordered. Migration from an implementation that overrode it:
+
+- node pre-execution work (DEBUG log / counter increments / start markers) →
+  move the body to `onNodeStart`, returning `null` when the observer keeps no
+  per-call state;
+- work that needs the evaluated result → move the body to `afterNode`;
+- per-call state that a matching hook must recover → return it from
+  `onNodeStart` and read it back in `onNodeEnd` (the documented token pairing
+  above); the engine still calls `onNodeEnd` from a `finally` block.
+
 ### Context
 
 - `CacheContext` exposes a read-only `InputView` (operation, cacheName,

docs/ARCHITECTURE.md

@@ -26,8 +26,17 @@ RedisCacheAutoConfiguration
 `RedisCacheAutoConfiguration` is conditional on Redis classes and
 `resi-cache.enabled`; it does not add `@EnableCaching`. The internal component
 scan is deliberately limited to `io.github.davidhlp.spring.cache.redis.cache`
-and excludes tests and operator-only/configuration seams listed in the source.
-Host application packages are not scanned by the library.
+and excludes test classes plus the operator-boundary assembly root, which is
+named by class. Runtime bean ownership is never expressed as a name pattern:
+classes that only their boundary may register carry no component stereotype,
+and a class rename fails compilation instead of silently changing the
+assembled set. Host application packages are not scanned by the library.
+
+The operator CLI (`SerializationMigrationCli`) is the second assembly
+boundary: its context names the internal migration beans by class through
+`SerializationMigrationOperatorConfiguration` and excludes
+`RedisCacheAutoConfiguration` by class, so it never assembles the cache/AOP
+runtime and needs no enablement gate.
 
 ## Module ownership
 
@@ -55,7 +64,7 @@ classes can move or disappear without becoming a compatibility promise.
 | 100 | `BloomFilterHandler` | membership gate / penetration protection |
 | 200 | `SyncLockHandler` | distributed or explicit local-only synchronization |
 | 250 | `EarlyExpirationHandler` | hot-key refresh decision and scheduling |
-| 300 | `TtlHandler` | base TTL and jitter calculation |
+| 300 | `TtlHandler` | TTL decision application (precedence and jitter resolve in `TtlPolicy`) |
 | 400 | `NullValueHandler` | negative-result encoding |
 | 500 | `ActualCacheHandler` | actual cache operation |
 
@@ -64,6 +73,9 @@ classes can move or disappear without becoming a compatibility promise.
 assembles observers. `ChainEngine` owns advancement, flow decisions, observer
 hook ordering, and post-processing isolation. A custom handler must be supplied
 by the host application's component scan or as an application bean.
+`HandlerOrder` additionally carries each slot's protection disable name and its
+`handler` metric/log tag, which internal `cache/HandlerIdentity.java` resolves as
+one declaration so that renaming a handler class changes neither.
 
 ## Annotation and policy flow
 
@@ -77,8 +89,12 @@ The annotation path is intentionally split into two views:
    declaration; read-through write-back remains governed by the read side.
 
 The split is required by the Spring operation source and the chain-side policy
-resolver. It is not permission to reintroduce per-invocation parsing or to
-collapse the two operation representations without a new contract decision.
+resolver. Both views are projected from one `RedisCacheAttributes` instance per
+annotation, and the snapshot carries a `kind + cacheName` index built at
+registration time, so the two views cannot disagree and policy lookup does not
+depend on declaration order. The split is not permission to reintroduce
+per-invocation parsing or to collapse the two operation representations without
+a new contract decision.
 Class-level operation discovery and method-level policy application retain the
 current documented behavior in `COMPATIBILITY.md`.
 
@@ -89,9 +105,12 @@ current documented behavior in `COMPATIBILITY.md`.
   `CacheResult` carries typed operation outcomes internally.
 - `LoaderOrchestrator` owns the shared read → load → write-back protocol. A
   successful loaded value is returned even when write-back fails.
-- `CacheErrorHandler` owns count-once failure reporting for chain failures; the
-  failure metric uses finite operation/kind/strategy dimensions and diagnostics
-  omit raw keys at WARN/ERROR.
+- `FailureReport` owns the one failure-reporting shape: a WARN/ERROR carrying
+  only cacheName or the key fingerprint, and — when the report carries a
+  throwable — its exception type chain plus a paired DEBUG line holding the full
+  stack. A report without a throwable emits the WARN only; `CacheErrorHandler` owns count-once
+  reporting for chain failures on top of it, and the failure metric uses finite
+  operation/kind/strategy dimensions.
 - `SecureJacksonRedisSerializer` owns whitelist-backed serialization and the
   `{version, payload}` envelope. Refresh metadata required by policy/CAS is
   persisted; process-local monotonic time is not.