Architecture remediation: nine review candidates (c1–c9) - #31
Merged
Merged
Conversation
The runtime root now owns the `resi-cache.enabled` gate alone and imports the enablement validation it protects; the second auto-configuration entry and its duplicate gate are gone. Internal classes registered by a boundary root's explicit import carry no component stereotype, so the runtime scan no longer needs class-name patterns for them, and the operator CLI names the beans it needs by class instead of scanning the runtime package. A class rename in either boundary now fails compilation instead of silently emptying a context: the operator root is excluded by class, and the CLI excludes the runtime auto-configuration by class rather than by a property string.
The contract test stops string-syncing exclusion patterns: it asserts the runtime scan names its one excluded boundary class by identity, that the only name pattern left is the same-package test-class filter, and that the auto-configuration imports resource registers exactly the runtime root by class name. The backoff invariant now enumerates @bean methods instead of a hand-maintained list, so a new default bean without @ConditionalOnMissingBean fails. The CLI contract test proves the operator context assembles the migration beans without the runtime auto-config.
23 failure paths in 13 files each re-applied the same prose rule by hand
("WARN/ERROR carries cacheName-or-fingerprint, never raw key or exception
message; the full stack stays at DEBUG"), so the rule only existed per site and
the two remediation commits that introduced it (e598693, 34fc53d) had to patch
sites one by one. FailureReport now takes what failed (text + raw key + throwable)
and emits the sanctioned pair; callers no longer choose levels, pair DEBUG with
WARN, or format fingerprints.
The privacy rule becomes structural: a raw key only enters as a key and is
fingerprinted by the seam, a throwable only as a throwable and rendered as its
type chain. Typed exception messages take the same fingerprint helper. Metric
(one report per failure), classification and count-once semantics are untouched,
and each call site keeps its own logger so log categories stay stable.
Two wording tokens move to the shared context suffix: LoaderOrchestrator's
documented CacheErrorHandler bypass keeps its behaviour (single redacted WARN,
no metric) but now reports "cause=" like every other site, and the sites whose
fingerprint label was ad hoc use the canonical "keyFingerprint=".
FailureLogKeyPrivacyTest enumerated the rule one method at a time (591 lines, one appender plus one fragment list per site) and asserted DEBUG wording the seam now owns, so it grew with every new failure path. FailureReportTest tests the rule once (one WARN/ERROR plus one stacked DEBUG, raw key and exception message never rendered, context assembled from what is available) and keeps thin per-site coverage for the shapes production actually crosses: Bloom fail-open, refresh retries, local-only degradation and observer isolation.
LockStack.close has no cache key in scope (it only knows the handles), so the release-failure report keeps the context-less form it always had: one ERROR with the type chain plus the stacked DEBUG pair.
TTL precedence used to be decided by branch order inside TtlHandler while the same 60-second default was also declared by the annotations, the Spring @cACHEpUT adapter's builder and the handler fallback. Add package-private TtlPolicy as the single resolution point (annotation > Duration parameter > fallback default; zero/negative parameter means permanent) and let TtlHandler only apply the decision, keeping its three debug log messages and the jitter counter semantics unchanged. The annotation default and the Spring @cACHEpUT builder default stay at 60s because both are reachable and dropping either would change behaviour; the conflict with resi-cache.default-ttl is recorded once, in TtlPolicy, as an open product decision.
TtlPolicyTest pins every reachable input combination without a handler chain: attribute set / unset (the annotation's own 60s default, proven through projector -> operation -> policy), explicit ttl=0 with and without a Duration parameter, zero/negative parameter, no declaration at all, and the configured 30m default. The jitter bounds tests move with the method they cover, and TtlHandlerTest keeps the handler-level application and counter cases (including the parameter path, which must not count as jittered).
The local-only degradation WARN is emitted through the elected role's logger (Leader carries the key context), not SyncSupport's, and the no-throwable warn is asserted on rendered messages instead of the concatenated text.
AnnotationParser built every RedisCache annotation twice: RedisCacheAttributes for the policy operation and an independent BuilderPopulator field list for the Spring-facing operation, so the two graphs could disagree on any shared field (they did: the AOP face preferred `value`, the projection preferred `cacheNames`) and one new field had to be threaded through both. Both faces now derive from one RedisCacheAttributes instance per annotation, and the shared AOP field set is declared once (applyToSpringCommonFields) beside the policy COMMON_SINKS mapping, so the values cannot diverge. The snapshot indexes policy operations by kind + cacheName when it is built, so RedisCacheRegister.get no longer depends on a backwards scan whose overwrite semantics lived in a comment; "last declaration wins" is stated and implemented once in ParsedAnnotations.PolicyIndex. Deleted the dead injector seam: RedisCacheAttributesProjector and SpringCacheableAdapter carried @component with no injectors, and the two-argument AnnotationParser constructor that accepted them had no caller. Javadoc that understated what a new field costs (projector, RedisCacheAttributeSink, BuilderPopulator) now records the measured inventory. AnnotationAopBehaviorMatrixTest pins both faces coming from one projection, the shared value/cacheNames resolution, and last-wins for repeated declarations.
The nested proxy-eligibility gate is a member configuration class, and Spring processes member classes only for a @Component-annotated configuration, so dropping @configuration from the outer class silently removed the proxy advisor and interceptor (caught by the default-assembly contract test). The outer keeps the stereotype; the inner gate stays without one so the scan registers the outer exactly once. The contract test now reads the aliased `classes` attribute it actually declares.
…ergence Closes the ratified T8 (c8) "preserve + document" second half: TtlPolicy already concentrates TTL precedence in code, and this records it in the canonical docs without touching any default value. Precedence semantics (owner: docs/REFERENCE.md) state the verified order — annotation ttl>0 (default 60s, jittered) > Duration parameter carrying resi-cache.default-ttl (only when no method-level TTL is set; zero/negative parameter = permanent) > null-parameter fallback 60s — all resolved once in TtlPolicy. The 60s-vs-configured-default divergence is recorded as a supported-behaviour limitation in COMPATIBILITY.md; the two pages cross-reference instead of duplicating. Every reachable per-input outcome is preserved unchanged (annotated -> 60s; plain Spring / ttl=0 -> configured default; null Duration -> 60s). Verified against TtlPolicy.resolve and RedisProCacheProperties; docs-only change, scripts/ci/check-docs-contracts.sh passes.
…ssembled collaborators The null-handling branches in RedisProCache, RedisProCacheWriter, CacheOperationResolver and LoaderOrchestrator existed only so tests could pass null; production wires the collaborators unconditionally (verified against RedisProCacheConfiguration and PublicSurfaceContractTest / external- consumer surface). HandlerResult is untouched so the public shouldTerminate() contract stays intact. - make the loader-path collaborators (operationResolver, bloomGate, syncSupport, syncLockTimeout) requireNonNull at construction, matching production assembly; drop ResiCacheFeatures.none() and per-caller null guards - fold the NullValueEncoder wrapper into CacheValueCodec.toValueBytes (Java null and NullValue.INSTANCE now share one null-placeholder branch) - delete the two CacheHandlerChainFactory convenience constructors - drop ChainEngine's re-check of HandlerResult's constructor-enforced invariant and the duplicate shouldHandle dispatch in AbstractCacheHandler - adapt the affected unit/integration tests to pass real collaborators
NullValueEncoderTest exercised the wrapper deleted last commit; its null-vs-NullValue decision is now covered by CacheValueCodecTest. HandlerResultProtocolTest only asserted ChainEngine's null-decision re-check, which is unreachable by construction (HandlerResult's record constructor rejects a null decision; the test reached it solely by mocking a final record). shouldTerminate() coverage is unaffected — HandlerResult and its existing TtlHandler/BloomFilter/ActualCacheHandler tests are unchanged.
Route WARN/DEBUG failure pairs through the internal FailureReport/FailureDiagnostics seam so callers stop re-deriving the key-privacy rule by hand.
Remove dead null-guard branches, the NullValueEncoder wrapper, convenience ctors and test-only surface; keep public HandlerResult.shouldTerminate() and beforeNode per the ratified decision.
Collapse the four regex bean-ownership declarations onto per-boundary roots, declare resi-cache.enabled once, and make the backoff invariant enumerate bean methods.
Collapse the two positionally-associated operation graphs into one operation model; record the contract decision dissolving the ARCHITECTURE.md operation-representation prohibition and reconcile the projector/sink Javadoc with reality.
Resolve TTL from annotation/parameter/configured in one module with the precedence visible in its interface; TtlHandler applies the decision. Preserve per-path behaviour; record the precedence rule in REFERENCE.md and COMPATIBILITY.md.
…an method The factory's observerOrder() reads the class-level @order, but @order lived on the four @bean methods, so the sort was a no-op and the "registration (@order) order" STABILITY.md §4 promises was only incidentally satisfied by injection order. Move @order(1..4) onto MDCStamp/ChainDebugLog/ChainTimer/ FiredCounter observer classes so the factory sort is the single declaration actually in force, and correct the factory/config Javadoc that claimed Spring pre-sorts the injected list.
Add a behavioral ChainObserverTest case that injects three @order observers in reverse order through the real factory and asserts they fire in @order order (hook sequence via observable effect, no reflection). Update RedisProCacheConfigurationContractTest to assert @order on the observer classes (the location the factory reads) instead of the inert bean methods.
…ion-surface test c7 made LoaderOrchestrator's constructor require bloomGate/syncSupport/syncLockTimeout (unreachable null branches deleted). boundCallbacks_productionEntryUsesOnlyLoaderAndKey still passed nulls and now NPEs. Reuse the migrated bound() helper (real no-op collaborators) so the case keeps its intent: production orchestrate crosses only (loader, key). Assertions unchanged.
…P face c6 made AOP and policy operations derive from one RedisCacheAttributes projection so the two faces cannot disagree, but AnnotationAopBehaviorMatrixTest. bothFacesComeFromOneProjection failed: for an unset `unless` the policy builder set the raw "" while the AOP builder (and SpringCacheableAdapter) guard text fields with hasText and leave it null — two representations from one projection. Route the policy applyTo(Cacheable/Put) `unless` through BuilderPopulator.applyText, the same seam the AOP face and SpringAnnotationAdapter already use, so a blank unless is null on both faces. Real values are unaffected.
resi-cache.metrics.enabled is now read in exactly one place (ResolvedMetrics.resolve), and a null Environment no longer skips the opt-in check, so test and production assembly cross the same rule. The seam is never null: the disabled case is a shared no-op CompositeMeterRegistry, which removes the per-caller registry null re-checks in the chain factory, the timer/fired observers, the failure reporter and the migration engine. RedisCacheHealthIndicator no longer borrows the metrics switch as its activation gate and drops the properties field it never read; its health details and ordering are unchanged.
Factory tests no longer pass a null Environment to bypass the opt-in: they resolve the seam with an explicit Environment (disabled by default, enabled where the fired counter is asserted). Observer and reporter tests hand out the shared no-op seam instead of null, and the assembly contract now asserts the disabled case is that seam. The health indicator test follows its two-argument constructor.
REFERENCE.md names resi-cache.metrics.enabled (default false, read once by ResolvedMetrics, declared in the additional configuration metadata), OPERATIONS.md states that the disabled case is a no-op seam and that the Redis health indicator is not metrics-gated, and COMPATIBILITY.md drops the stale statement that the indicator requires the metrics property.
Fold the re-entrant 7-argument SyncSupport.executeRoleWork static into an executeRoleWork method on the SyncStateAccess owner, and move the SyncState registry impl next to the roles it mutates so state and lifecycle read in one file. Extract the once-only publication ordering (enter -> complete -> exit -> cleanup) into Leader.finishPublication as a stated protocol. Behavior, log strings/levels, lock pairing and the public nested role types are unchanged; add a SyncRoleTest check that the lifecycle registers each step exactly once.
The class-level @order must reach Spring's list injection, not just an instance-level comparator: if it stopped being honoured, the order would fall back to bean-name order and the debug log would read the request id before it is stamped.
… hook The observer dispatch helper now carries each level's own result type (CacheResult at chain level, HandlerResult at node level), so the two downcasts in ChainEngine are gone; scope tokens stay Object, paired back positionally per observer. MDCStampChainObserver and ChainTimerChainObserver no longer re-check their own token's runtime type: the engine's index pairing makes the token their own reference, so the private token type is a cast, not a defensive instanceof. beforeNode had zero production implementers (grep over src/main: 12 hits, all javadoc plus the hook declaration and its dispatch call, no @OverRide in any adapter); it is removed from the SPI and the engine's node loop is onNodeStart -> handler.handle -> afterNode -> onNodeEnd. Order and content of every remaining dispatch are unchanged. BREAKING CHANGE: ChainObserver.beforeNode(CacheHandler, CacheContext) is removed. Move node pre-execution work to onNodeStart (or afterNode when the evaluated result is needed); per-call state moves with the documented onNodeStart/onNodeEnd token pairing. STABILITY.md §4 Observers records the migration path. Co-Authored-By: Claude Code <noreply@anthropic.com>
The `ttl` attribute of `@RedisCacheable`/`@RedisCachePut` defaulted to 60 seconds and `TtlPolicy` reads "policy ttl > 0 wins", so an annotated method that never set `ttl` took 60 s and pre-empted the configured cache TTL. Re-point the unset sentinel at 0 — the encoding `resolve` already reads as "no method-level declaration" and the one `@RedisCacheEvict.ttl()` already uses — so an unset attribute falls through to `resi-cache.default-ttl` (30m unless overridden, per-cache `caches.*.ttl` still applies). Collapse `RedisCachePutOperation.Builder.ttl` (60) onto the same sentinel as `RedisCacheableOperation`/`RedisCacheEvictOperation` (0), leaving the annotation as the only place a method-level TTL can be declared. Delete `TtlPolicy.DEFAULT_TTL_SECONDS` and the branch that applied it when the Duration parameter is null. That branch is not reachable on a write path: TtlHandler only handles PUT/PUT_IF_ABSENT, whose Duration SDR 4.0 computes from `RedisCacheConfiguration`'s `TtlFunction` — `persistent()` (i.e. `Duration.ZERO`, not null) when no TTL is configured, and `entryTtl` rejects null — while ResiCache's own null-TTL writes are GET/REMOVE/CLEAN. A null parameter now means the same thing SDR's own writer means by it (no expiry): permanent, like zero and negative. `TtlPolicy` keeps the precedence visible as annotation > parameter (the configured default as Spring computes it) > permanent. Co-Authored-By: Claude Code <noreply@anthropic.com>
`docs/REFERENCE.md` stated the annotation `ttl` default as 60 and that a null Duration parameter falls back to a 60-second default, and `COMPATIBILITY.md` recorded the 60s-versus-30m divergence as an unresolved product decision kept as-is. Both are now false. State the decided resolution in `REFERENCE.md` — annotation `ttl` greater than zero is the only declaration that overrides the configured default; an unset attribute falls through to `resi-cache.default-ttl`; a zero, negative or null parameter means no expiry. Record the behaviour change and its migration options in `COMPATIBILITY.md`, including the corrected claim that a caller-supplied `RedisCacheConfiguration` yields a zero (`persistent()`) parameter, not a null one. Co-Authored-By: Claude Code <noreply@anthropic.com>
The disabled path was an empty `CompositeMeterRegistry`, which publishes nothing but still retains every meter id and tag string it is asked for in its own meter map. With dynamically named caches that map grows for the life of the context. Replace it with `DisabledMetricsRegistry`: a stateless `MeterRegistry` over Micrometer's public `io.micrometer.core.instrument.noop.*` types, whose constructor installs a deny-all `MeterFilter` so the base class short-circuits before its own `meterMap.put` and nothing is retained. Because it holds no state it can be a single JVM-wide instance, which is what `docs/REFERENCE.md` already claimed; the Javadoc and the reference now state the real sharing behaviour. Covered by `DisabledMetricsRegistryTest` (same instance across resolutions, meter table stays empty over repeated dynamic-name registrations) and the updated no-registry choice assertion in the configuration contract test. Co-Authored-By: Claude Code <noreply@anthropic.com>
`main` built the Spring operation with `ann.value().length > 0 ? ann.value() : ann.cacheNames()` at all three `parseRedisCache*` sites, so `value` won on the operation face. The c6 unification routes both faces through `RedisCacheAttributesProjector.resolveCacheNames`, which preferred `cacheNames` - a silent precedence flip for a declaration that sets both attributes (`@RedisCacheable(value="a", cacheNames="b")`). Both-set behaviour, stated explicitly: - on `main`: the operation targeted cache `a` (`value`), while the policy snapshot was registered under `b` (`cacheNames`), so the declared policy silently did not apply to the cache that was actually used; - now: both faces resolve to `a` (`value`). The cache in use is unchanged from `main`, and the policy now applies to it; the `cacheNames` alias no longer carries a second policy snapshot. Only the both-set declaration changes; declaring a single attribute - the overwhelmingly common case - is unaffected. The single resolution for both faces is kept (the c6 goal). The rule is recorded in COMPATIBILITY.md and in the three annotations' `value` / `cacheNames` Javadoc, and pinned by `AnnotationAopBehaviorMatrixTest.aliasResolutionIsSharedByBothFaces`, `AnnotationPolicySnapshotTest.bothCacheNameAttributesSet_valueWinsOnBothFaces` and the projector's `value_wins_over_cacheNames`. Co-Authored-By: Claude Code <noreply@anthropic.com>
`CacheHandlerChain.handlerTag` is evaluated on the request hot path - per node per request by the Engine post-processing log and by `FiredCounterChainObserver.afterNode` - and the argument is evaluated even when DEBUG is off. Since c9 it is `HandlerIdentity.of(handler).tag()`, i.e. a reflective `getAnnotation(HandlerPriority.class)` plus a record allocation per call. Identity depends only on the handler class, so memoize the resolution in a `ClassValue` keyed by class: one reflective lookup per class for the life of the loader, and no static map holding strong class references across classloaders. Tag values are unchanged, including the class-simple-name fallback for handlers with no declared identity. `HandlerIdentityContractTest.identityIsResolvedOncePerHandlerClass` pins stable tags and same-instance resolution for both the annotated and the class-name-derived path. Co-Authored-By: Claude Code <noreply@anthropic.com>
Deleting `NullValueEncoder` folded the null decision into `CacheValueCodec.toValueBytes` and dropped the DEBUG line `main` emitted at that decision. Log wording is frozen, so losing it is an observable change; the codec has no cacheName/key context to log from. The decision now lives once, in `CacheValueCodec.isNullDecision`, and both byte production and the log use it. The two `toValueBytes` call sites (`ActualCacheHandler` cached-hit and PUT_IF_ABSENT-existing) each hold the context, so a private `encodeForReturn(value, context)` in `ActualCacheHandler` carries the line with `main`'s exact wording and level - the codec itself stays context-free. Scope note: the restored line fires on the codec's whole decision, i.e. also when the value in hand is `NullValue.INSTANCE`. `main`'s trigger was the narrower `value == null` (its encoder saw a non-null `NullValue` and logged nothing); both cases mean the same thing - the null sentinel is being returned - and the cached-null read path (`CachedValue` payload `null`) is the case `main` did log, so that observable line is restored byte-for-byte. `ActualCacheNullReturnLogTest` pins the line for a null payload and for `NullValue.INSTANCE`, and its absence for a normal value. Co-Authored-By: Claude Code <noreply@anthropic.com>
Removing the `@ConditionalOnProperty(resi-cache.metrics.enabled)` gate is already documented, but the load it adds is not: every application that has Actuator, Redis and ResiCache now assembles `RedisCacheHealthIndicator`, so each `/actuator/health` probe runs a synchronous `connection.ping()`. State the per-probe round trip, that it is per application instance, and that the previous behaviour was no probe traffic at all when metrics were off; point at the connection-pool/probe-interval sizing consequence. The Actuator row in COMPATIBILITY.md carries the same note. No code change. Co-Authored-By: Claude Code <noreply@anthropic.com>
Replacing `FailureLogKeyPrivacyTest` (591 lines) with `FailureReportTest` (294 lines) kept the seam plus BloomSupport, RefreshRetryPolicy, ChainEngine (observer) and SyncSupport, but dropped the per-site guards: `FailureReport` makes the key *argument* safe, yet nothing stopped a call site from concatenating the raw key into the free-text argument, and no test failed if one did. Restore one thin guard per dropped site, each asserting that the raw key placed in that site's context does not appear in its captured WARN/ERROR text (including the throwable message chain, which is where a leak would land). Sites covered: `RedisBloomIFilter` (add/check/delete), `DistributedLockManager` (acquire timeout WARN, interrupted ERROR, release retry WARN, release-exhausted ERROR, interrupted-during-retry ERROR), `ChainEngine` post-processing (execution and predicate), `EarlyRefresh` async refresh, `SyncRoleLockExecutor` lock acquire and release, and `SerializationMigrationEngine` forward plus rollback rejected key - the last of which the pre-replacement suite did not cover either (byte[] keys, never guarded before). Sites already covered elsewhere by message assertions (`DistributedLockManagerIntegrationTest` interrupt, `SyncSupportTest`, `BloomFailureLogKeyPrivacyTest` check) are not duplicated. Class list, before -> after: - dropped: `FailureLogKeyPrivacyTest` (591 lines) - kept: `FailureReportTest` (294 lines, unchanged) - added: `FailureLogKeyPrivacyTest` (473 lines, 12 site guards) Verified non-vacuous by mutation: making the seam emit the raw key turns 6 of the 12 guards red. Co-Authored-By: Claude Code <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91b4af3ddd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The guard removal in 9e4c2fd made the no-op sink reach three writers that still built their keys and filled their own maps, so "metrics off" retained state again: one TimerKey per distinct cacheName, one fired counter per handler class, one failure counter per tag combo. Derive the flag once at construction from DisabledMetricsRegistry, the sink that owns the meaning of "disabled" (isDisabledSeam), and return before key construction, map write and Noop* allocation. No caller sees a null seam and no site re-reads resi-cache.metrics.enabled. Regression: ChainObserverTest.noopSeam_retainsNoTimers drives 100 distinct cacheNames through the production ResolvedMetrics disabled path; the two no-op seam tests now assert the maps stay empty instead of only "no throw". Co-Authored-By: Claude Code <noreply@anthropic.com>
The un-gating in 9e4c2fd made the indicator unconditionally assembled, so its existing WARN was re-emitted on every health probe. The degradation is construction-constant (LockManager list plus the local-only property), so one latched WARN per context carries the same information without a per-probe log volume that scales with the probe cadence. Level, wording and both protection.degraded detail keys are unchanged, and every probe still reports the degraded state in its response; the indicator is not re-gated on any property. Co-Authored-By: Claude Code <noreply@anthropic.com>
…ontract Four javadocs still asserted that a null MeterRegistry means metrics are off. ResolvedMetrics has returned the shared stateless DisabledMetricsRegistry.INSTANCE since the seam commit, so those claims were false and read as evidence that disabled-path retention was impossible. State the current contract instead: the seam is never null, metrics-off is signalled by the shared INSTANCE and can be queried with isDisabledSeam, and registrations on it publish and retain nothing. Comment text only. Co-Authored-By: Claude Code <noreply@anthropic.com>
…fixes The disabled-seam work replaced "registry == null" with a never-null shared no-op sink, but two files still documented the retired contract. Restate both truthfully: production never injects null, the disabled sink allocates no-op meters that publish and retain nothing, and the null branch survives only for the test/defensive path. Also roll up the two behaviour notes into CHANGELOG.md: the disabled seam keeps no meter, timer or per-cache entry, and the degraded-protection warning fires once per context instead of once per health probe. Co-Authored-By: Claude Code <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11b0a75462
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
record(outcome) is the engine's only MeterRegistry touch and is reached 1-4 times per scanned key. On the shared disabled seam the deny-all filter stops retention but not the work: each call rebuilt the varargs tag array, constructed a Meter.Id, walked the registry filter under the meter map lock and allocated a NoopCounter that was discarded. Derive the seam identity once in the constructor and return before that work, byte-for-byte the pattern already used by ChainTimerChainObserver, FiredCounterChainObserver and CacheFailureReporter. The guard sits at the single choke point, so all eight call sites are covered without a per-site enabled check. Co-Authored-By: Claude Code <noreply@anthropic.com>
…stry The class's null-registry short-circuits were dead code, because the seam is never null. Widening the two shared registration helpers to reject the disabled seam makes all seven fields null on that path, so every existing null guard goes live again with no new field and no per-call check. That removes two residues at once: seven Meter.Id builds, filter walks, Noop* allocations and meter-map-lock acquisitions per distinct cache name, and the per-operation cost of holding Noop* meters, which made every get, put, evict and clear pay two clock reads, a try/finally and a virtual no-op record through the guards that were meant to short-circuit it. The null-registry path keeps its behaviour: isDisabledSeam(null) is false, so a null argument still yields the same null fields. Co-Authored-By: Claude Code <noreply@anthropic.com>
…on path Three more consumers registered through the shared disabled sink and then kept non-null Noop* fields, which defeated the null short-circuit they already had. Widen the single hook each class owns so the disabled seam falls into the null path the class already handles: the handler base's semantic counter (read inside the per-node chain loop), the refresh-task metrics' three counters (read on every submit, completion and cancellation) and the bloom filter's two failure counters (read on the failure path). The null arm stays, so the null-registry path the tests exercise directly is byte-identical. The two small package-private observers exist only so the cold sites can be asserted on; the disabled path has no other observable output, since the seam returns non-null no-op meters and retains nothing. Co-Authored-By: Claude Code <noreply@anthropic.com>
The disabled-seam round guarded the timer observer's onNodeEnd but left onNodeStart constructing a TimerScope unconditionally, so every handler node of every cache operation still read the clock and allocated a token that the chain then discarded - the default configuration, one allocation per node. Return null on the disabled path, which is what ChainEngine already pairs back and what this observer did before the non-null seam existed. The observer's class Javadoc and the onNodeEnd comment now state both null-token sources (disabled seam, or a failed start hook) instead of only the latter. Red-first: ChainObserverTest.noopSeam_doesNotAllocateScopeToken fails with "expected: null" against the pre-fix onNodeStart and passes after. Co-Authored-By: Claude Code <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d330729716
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
SyncSupport.isDegraded() was true exactly when no lock backend existed and local-only was NOT enabled — the fail-fast state — yet the indicator labeled that observation protection.degraded=local-only, while the explicit local-only degradation it names reported nothing. SyncSupport now owns one ProtectionMode derivation (DISTRIBUTED / LOCAL_ONLY / FAIL_FAST); the indicator attaches the matching detail per mode, warns at most once per mode, and omits protection details when a backend exists. CHANGELOG and OPERATIONS updated to the shipped semantics.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Closes #32
Problem
The 2026-09-22 architecture review surfaced nine remediation candidates (c1–c9); this branch implements all nine as ticket branches merged in three waves, plus the post-review follow-up and the review findings recorded below.
What changes
FailureReportseam replaces hand-rolled WARN/ERROR + fingerprint pairs at ~23 sites (13 files); key-privacy rule, count-once semantics and metric dimensions preserved; per-site privacy guards kept inFailureLogKeyPrivacyTest(verified non-vacuous by mutation).resi-cache.metrics.enabledis read in exactly one place (ResolvedMetrics), declared in configuration metadata; the disabled path yields a zero-allocation no-op registry, so the null re-checks are gone; test and production paths cross the same resolve.@Order. The dispatch sort is real (it previously read a class@Orderthat nothing declared);@Bean-method@Orderremoved; factory Javadoc corrected; dispatch-order behaviour test added.HandlerOrder. Order slot, kebab disable name and metric/log tag declared once, frozen values pinned byHandlerIdentityContractTest; ordering contract (TTL before Actual, Actual last) checkable; skipAll rule guarded through the real factory.resi-cache.enableddeclared once; newSerializationMigrationOperatorConfigurationallowlisted.RedisCacheAttributesinstance per annotation with akind + cacheNameindex; the Spring-operation/policy-view split is retained (Spring requires it).NullValueEncoderwrapper, convenience ctors, test-only surface) — package-private only: publicHandlerResult.shouldTerminate()and theChainObserverSPI kept.TtlPolicy. The annotation's implicit60removed, so the resolution has one implicit default — the configured one.Behaviour changes (⚠️ , accepted by the maintainer)
ChainObserver.beforeNoderemoved, dispatch typed (ObserverDispatch<CacheResult>at chain level,ObserverDispatch<HandlerResult>at node level).beforeNodemust move toonNodeStart/afterNode;STABILITY.md§4 carries the migration. Scope tokens stayObject(observer-private per-call state).60→ the configured value.@RedisCacheable#ttl/@RedisCachePut#ttldefault to0("no method-level declaration"), so an annotated method without an explicitttlexpires afterresi-cache.default-ttl(30m) instead of 60 s. Zero, negative andnullnow all mean "no expiry", matching Spring Data Redis'sDefaultRedisCacheWriter.shouldExpireWithin; an explicit positivettlstill wins with its jitter. Recorded inCOMPATIBILITY.md,docs/REFERENCE.mdand the annotation Javadoc; every input combination is pinned by a test.SyncSupport.protectionMode():fail-fastwhen no lock backend exists andlocal-only=false(sync=true operations fail fast on the first cache miss),local-onlyonly whenlocal-only=trueis explicitly enabled (single-JVMsynchronized, not multi-instance protection), no detail when a lock manager is present (5bf6adfb); the degraded WARN latches to once per mode. Documented in OPERATIONS/COMPATIBILITY/REFERENCE,CHANGELOG.md.Follow-up after review — findings all fixed
HandlerResult(38f1ba1d) withHandlerResultProtocolTest; it conflicted with the documented SPI contract (STABILITY.md§4 "Handlers" item 7). Constructor-level rejection unchanged.69e3c063): with metrics off, multipleMeterRegistrybeans no longer fail startup, and the disabled seam is per-resolution rather than a process-wide static sink.453100da); the four standard adapters still declare@Order(1..4)on the class.MeterFilterand stateless no-op seam (R1/R7), a full sweep ofMeterRegistrytouches found eight sites that still did work with metrics off —SerializationMigrationEngine.record,RedisProCacheMetricsRegistry,AbstractCacheHandler.attachMeterRegistry,RefreshTaskMetrics,RedisBloomIFilter.init,ChainTimerChainObserver.onNodeStart/onNodeEnd, the fired-counter observer and the failure reporter — each now short-circuits on the disabled signal derived once at construction (DisabledMetricsRegistry.isDisabledSeam(registry)), with no per-call registry checks and no reintroduced nulls. Red-first evidence: a recordingMeterFiltermeasured four seam counter calls for four migrated keys before the guard, zero after; the timer-observer map assertions read100/1/2entries before,0after.value/cacheNamesprecedence restored tomain's with the policy now applying to the cache actually used (R2);handlerTag()resolved once per handler class, off the request hot path (R3);FailureLogKeyPrivacyTestrestored with 12 site guards (R4); the cached-null DEBUG line restored withmain's wording (R5); the health indicator's per-probe Redis cost documented indocs/OPERATIONS.md(R6); failure-report shape qualified for throwable-less reports (a0f03676); CHANGELOG roll-up (44a0b59f, wording5a02c38e).Open items — deliberately not fixed
LocalBloomIFilter's cacheName-keyedlocalFilters/locksmaps are populated independently of the metrics switch — pre-existing onmainwith a different trigger, reported for a follow-up.RedisProCachecall site allocates a capturingSupplier/Runnablebefore entering the metrics seam — once per operation, not per meter. This is not meter work, and no seam-side change removes it.Maintainer ratification points
ResolvedMetricsseam plusadditional-spring-configuration-metadata.json, not bound as aRedisProCachePropertiesfield — binding a fixed contract key would add a tenth public nested type (allowlist +STABILITY.md§4 churn) for an assembly detail.mainemitted for every handler; removing it would change frozen tag values.docs/ARCHITECTURE.mdas "one projection, both views".SerializationMigrationOperatorConfiguration, an allowlisted assembly root pinned bySerializationMigrationCliContractTest.Verification
Current head
5bf6adfb— PR Pipeline run 35708615591 green end to end:./mvnw clean verify -Bwith the JaCoCo coverage gate andcheck-test-names.sh, unit tests without Docker, lint/checkstyle, docs ↔ source consistency, quality advisories, bench module, package,ci-ok. Earlier full-gate runs on pushed headsd039c16c/91b4af3d/11b0a754/d3307297(remote JDK 21;clean verify, checkstyle,check-test-names.sh,check-docs-contracts.sh,check-external-consumer.shall green) are superseded by this head; regression guards were confirmed red-first against the pre-fix implementations.