fix(clustering): surface silent cache-transport failures and make rewire retryable (#36803)

[wezell]

Proposed Changes

Fixes #36803 — cluster cache invalidations were dropped silently when the pub/sub transport failed, and a persistently failing rewire was logged and forgotten (root cause analysis in #36544).

• PubSubCacheTransport.send() no longer discards invalidations silently: increments a dropped-message counter and logs at WARN, rate-limited (CACHE_TRANSPORT_DROP_WARN_INTERVAL_MILLIS, default 30s) so a sustained outage cannot flood the log.
• PubSubCacheTransport.init() is idempotent: a rewire on an already-healthy transport is a no-op instead of a tear-down/rebuild of the pub/sub listener — this removes the rebuild-per-rewire-pass churn measured in Spike: Cluster re-wire re-inits Postgres pub/sub transport and leaks DB connections → pool exhaustion + startup crash-loop #36544 (LISTEN cluster_actions ×3,687).
• ClusterFactory.addMeToCacheIfNeeded() now reports success/failure instead of swallowing exceptions. KNOWN_SERVERS is only updated on success, and rewireClusterIfNeeded() retries whenever there is a pending failure, so a failed transport init is always retried on the next heartbeat. Consecutive failures are tracked and exposed via ClusterFactory.getRewireFailures() (reset on success).
• New cache-transport health check (CacheTransportHealthCheck, registered in CoreHealthCheckProvider): reports unhealthy when the transport is uninitialized or rewires are failing persistently; exposes initialized / droppedInvalidations / startupDroppedInvalidations / failedInvalidations / rewireFailures / awaitingInitialization as structured data. Nodes with no real transport always report healthy, and a node whose transport has not come up yet is treated as still starting for a grace period rather than reported unhealthy.
• Micrometer gauges in the existing CacheMetrics binder:
  • dotcms.cache.transport.invalidations.dropped
  • dotcms.cache.transport.invalidations.dropped.startup
  • dotcms.cache.transport.invalidations.failed
  • dotcms.cache.transport.initialized
  • dotcms.cache.transport.rewire.failures
• CacheTransport gains default long getDroppedMessages(), default long getStartupDroppedMessages() and default long getFailedMessages() (0 for all other transports).

Detail in comments

Two sections were moved out of this description to keep it readable:

• Review follow-ups — what changed in each of the seven post-review commits and why, including the two suggestions that were deliberately not implemented as written.
• Deployment impact — measured log and health-status noise per node boot, before and after the startup-grace change. Read this one if you care whether shipping this generates alerts.

Scope note for reviewers

The second follow-up commit widens this PR past the cache subsystem the title implies. QueuingPubSubWrapper and DotPubSubProvider are shared by every pub/sub topic in the JVM — cache invalidation, OSGi restart (OsgiRestartTopic), and cluster management (ClusterManagementTopic) — so those paths now execute the new code too.

The changes there are additive and behaviour-preserving:

• DotPubSubProvider.getFailedPublishCount(String) is a new default method returning 0, so no existing provider changes behaviour or needs updating.
• In QueuingPubSubWrapper.publish(), the submitted task's body moved into publishAndRecordOutcome(). The publish call itself, the dedupe cache, the submitter, and the unconditional true return are all unchanged — the only additions are counting a false return and catching a throw that was previously discarded by the submitter.
• Failure counts are keyed per topic, so a non-cache topic's failures cannot inflate the cache transport's metric (covered by test_failures_are_attributed_per_topic).

Worth a second pair of eyes on that file specifically, since a regression there would affect OSGi restarts and cluster management rather than just cache invalidation.

Readiness and alerting decisions (AC: "decide and document")

The health check defaults to MONITOR_MODE: it reports degradation but never fails readiness probes. A node that cannot send invalidations can still serve traffic, and gating readiness on the transport risks a cold-start deadlock (transport init happens during cluster wiring). Operators who prefer to drain such nodes can opt in with health.check.cache-transport.mode=PRODUCTION. It is never a liveness check — restarting pods on transport failure is what amplified the #36544 incident.

Failed invalidations deliberately do not make the check unhealthy on their own. The count is cumulative and never resets, so alarming on "greater than zero" would pin a node DOWN forever after one transient publish error — the same false-positive class fixed for rewire failures. Alert on the rate of increase of dotcms.cache.transport.invalidations.failed instead. Rewire failures do flip the check, but only past a threshold, because that counter resets on success and the rewire is retried every heartbeat.

Testing

• PubSubCacheTransportTest (unit, 10 cases): drops are counted and not published before init; publish works after init; init() is idempotent (single start() across repeated calls, re-inits after shutdown()); a synchronous publish failure is counted as failed rather than dropped; a successful publish counts neither; an async provider's self-reported failures surface without being double counted; the first init() retires pre-init drops into the startup counter; drops after the first init() survive a later re-init instead of being laundered into it. Plus the atomic init guard: 16 threads racing into init() produce exactly one start()/subscribe(), and a throwing start() leaves the transport retryable.
• ClusterFactoryRewireTest (unit, 5 cases, new): steady state does not rewire; a pending failure forces a retry even when membership looks unchanged; membership join and leave both rewire; a node missing from its own alive set rewires; the first heartbeat rewires.
• QueuingPubSubWrapperTest (unit, 5 cases): a false return from the wrapped provider is counted even though publish() reported success; a thrown failure is counted rather than lost on the submitter thread; failures are attributed to their own topic and do not inflate the cache topic; successful publishes count nothing and an unused topic reports 0.
• CacheTransportHealthCheckTest (unit, 10 cases, new): see 5aecea7292 in Review follow-ups.
• All 30 pass locally. Every test written for a review finding was confirmed to fail without its fix, so none of them is a test that passes either way: the concurrency one observes 3 start() calls with synchronized removed (3/3 runs), the case-normalization one reports expected:<1> but was:<0>, and the retry one fails when the pendingFailures clause is dropped. Note that running :dotcms-core unit tests requires -Dmaven.build.cache.enabled=false — the build-cache extension skips dependency:properties, which populates ${net.bytebuddy:byte-buddy-agent:jar}, leaving a literal -javaagent: path that crashes the surefire fork. Pre-existing and unrelated to this PR (an untouched DotPubSubEventTest fails identically).
• Two-node cluster, built from this branch. Cluster forms and both nodes report UP; PING/PONG bidirectional; a cache invalidation made on node 1 is observed on node 2 (node 2 served ORIGINAL from cache, then UPDATED-VIA-NODE1 after the invalidation); exactly one initing PubSubCacheTransport per node, confirming the idempotent init() in the real rewire loop; an uninitialized transport is detected and MONITOR_MODE keeps readiness passing; the drop WARN is throttled as intended (2 warnings for 2,841 drops); a NullTransport node reports UP with the per-transport fields omitted; message and structured data agree on every poll.
• The post-init failure path was verified live, which is the novel part of 343865b39e: cutting node 2's link to the database produced failedInvalidations: 2 with initialized: true and droppedInvalidations unchanged, matching exactly two Unable to send pubsub log lines — no double counting, correct per-topic attribution, node 1 unaffected at failed: 0. This ran through the default queued path (DOT_PUBSUB_USE_QUEUE defaults true). Node 2 recovered fully on reconnect.
• Re-verified on a rebuilt image after the grace period was added — zero DEGRADED cache-transport polls across 6,110 samples on two boots, with the counter split reading dropped: 0 / startupDropped: 2841 and 0 / 6. Full numbers in Deployment impact.
• Not verified live: the rewire counter. ServerHeartbeatJob.execute() calls LicenseUtil.updateLicenseHeartbeat() before ClusterFactory.rewireClusterIfNeeded(), and the former throws when the database is down, so REWIRE_FAILURES cannot move during a database outage. It only moves when the database is healthy and setCluster()/testCluster() fails — a narrow window I could not reproduce locally. The threshold and reset logic are covered by unit tests instead.

Known gaps

• RedisStreamsPubSubImpl.publish() always returns true (fire-and-forget async xadd), so it reports no failures. Left as-is; its own design decision.
• This surfaces silent transport failures; it does not fix the crash-loop or the missing already-ran guard in StartupTasksExecutor — Pub/sub listener holds a permanent connection from the shared JDBC pool, exhausting jdbc/dotCMSPool #36801 / PGListener lifecycle: unbounded rebuilds with no backoff, leak window before Thread.start(), stop() allocates, flag-based idempotency guard #36802 still own those.
• Three pre-existing problems were found while verifying this branch on a local cluster, and filed separately. None is caused by this PR, and none blocks it — the health check is unaffected by all three — but the first two do mean the gauges this PR adds are not yet consumable:
  • Micrometer metrics subsystem never initializes, so no dotcms.* metric is ever exported #36995 — the Micrometer metrics subsystem never initializes, so no dotcms.* meter is ever registered.
  • /dotmgt/metrics has no servlet mapping and returns 404, breaking the metrics-monitoring compose example #36996 — /dotmgt/metrics has no servlet mapping and returns 404, breaking docker/docker-compose-examples/single-node-metrics-monitoring.
  • DOT_PUBSUB_PROVIDER_OVERRIDE cannot be set via environment variable, silently falling back to the JDBC provider #36997 — DOT_PUBSUB_PROVIDER_OVERRIDE cannot be set by environment variable, so a container meant to use Redis pub/sub can silently run on JDBC.
• Startup drops are reported here, not eliminated. That invalidations are broadcast at all before the transport is up is arguably its own gap (a booting node in an existing cluster has nothing to send them through), but fixing boot ordering is well outside this PR.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XFuXwYdNsb7irpMEeSGTyj

This PR fixes: #36803

[claude]

Claude finished @dsolistorres's task in 4m 2s —— View job

---

PR Review — complete

• Read the diff against origin/main
• Review cache-transport changes (PubSubCacheTransport, QueuingPubSubWrapper, DotPubSubProvider)
• Review ClusterFactory rewire/retry logic
• Review new CacheTransportHealthCheck + CacheMetrics
• Review tests

---

I reviewed the full diff and traced the runtime paths the new counters depend on. No blocking issues found — the change is correct and unusually well tested. Details below.

Verifications performed

• No double-counting of failed sends. PubSubCacheTransport.getFailedMessages() sums its own synchronous count with pubsub.getFailedPublishCount(topic). With the default DOT_PUBSUB_USE_QUEUE=true, QueuingPubSubWrapper.publish() returns true synchronously, so the send() branch never increments failedMessages and only the wrapper counts; with the queue disabled the raw provider returns the real boolean and the wrapper's default getFailedPublishCount returns 0. Exactly one path counts each attempt. ✅
• Topic-key normalization is consistent. Write side keys on event.getTopic() (lowercased by DotPubSubEvent.Builder.withTopic, DotPubSubEvent.java:216) and read side on this.topic.getTopic() (String.valueOf(getKey()), not normalized), but both pass through QueuingPubSubWrapper.topicKey() which lowercases. Lookups match. ✅
• Success is not misread as failure for the default provider. JDBCPubSubImpl.publish() runs SELECT pg_notify(?,?) (JDBCPubSubImpl.java:40), so statement.execute() returns true on success → publish() returns true. false is only returned on an exception, and that path already logs a WARN (JDBCPubSubImpl.java:309). Consuming the boolean does not turn healthy sends into counted failures. ✅
• No transient DOWN during a normal rewire. ChainableCacheAdministratorImpl.setCluster() only calls init() when shouldReinit() || !isInitialized() (:190), and the now-idempotent init() is a no-op on a healthy transport, so a rewire does not tear the transport down — the health check's "was initialized and is NOT anymore" branch can't fire spuriously on a healthy node. ✅
• getTransport() is on the DotCacheAdministrator interface (DotCacheAdministrator.java:120), so the health-check and CacheMetrics resolution via the interface (rather than casting getImplementationObject()) is sound. ✅
• ThreadLocal<TransportSnapshot> has no leak. HealthCheckBase.check() always calls buildStructuredData() after performCheck() on the same thread (even when performCheck() throws — HealthCheckBase.java:77/:98), and buildStructuredData removes the ThreadLocal in a finally. ✅
• shouldRewire() preserves prior behavior and adds only the pendingFailures > 0 retry clause; getCurrentServer() is resolved once and passed in — equivalent to the inline condition. ✅
• Init guard rationale is correct. Setting initialized only after start()/subscribe() return (rather than a compareAndSet up front) is the right call — the CAS form would strand a thrown start() as permanently "initialized", re-introducing the silent failure. The dedicated test covers this.

Notes (non-blocking, no action required)

• The three "Known gaps" in the PR body (Micrometer metrics subsystem never initializes, so no dotcms.* metric is ever exported #36995 metrics registry never initializes, /dotmgt/metrics has no servlet mapping and returns 404, breaking the metrics-monitoring compose example #36996 /dotmgt/metrics 404, DOT_PUBSUB_PROVIDER_OVERRIDE cannot be set via environment variable, silently falling back to the JDBC provider #36997 DOT_PUBSUB_PROVIDER_OVERRIDE) mean the new gauges aren't consumable yet, but that's pre-existing and out of scope. The health check is unaffected and is the operative signal.
• RedisStreamsPubSubImpl.publish() remains fire-and-forget (return true) and reports no failures — correctly called out as an accepted gap.

No issues found.
· issue-36803-silent-cache-transport-failures

[dsolistorres]

Review follow-ups

Moved out of the PR description to keep it readable. Seven commits added after review; full rationale in each commit message.

Seven commits added after review. Details in the commit messages.

b7aa686d45 — remove false positives from the monitoring

• CacheMetrics had no NullTransport guard. NullTransport.isInitialized() is false once it has been shut down, so a node with no real transport published transport.initialized=0 and alerted as though it were dropping invalidations — while the health check reported the same node healthy. Both now share one activeTransport() helper and agree.
• Both call sites resolved the transport by casting getImplementationObject() to ChainableCacheAdministratorImpl. getTransport() is on the DotCacheAdministrator interface and CommitListenerCacheWrapper delegates it, whereas the cast throws ClassCastException for any other administrator — NullCacheAdministrator.getImplementationObject() returns itself, which is the unit-test path — swallowed into a null that happened to give the right answer. Both now use the interface method, as ClusterResource does. The cast in addMeToCacheIfNeeded stays: setCluster()/testCluster() are genuinely not on the interface.
• A single rewire failure no longer reports DOWN. testCluster() can throw on a momentary database hiccup while the transport stays initialized and invalidations keep flowing. The check now tolerates health.check.cache-transport.rewire-failure-threshold (default 3) consecutive failures, and always reports the count.
• The rewire counter now has a guaranteed path back to zero. rewireClusterIfNeeded() only fired when the alive-server set changed, so a failure followed by membership settling back to the stale KNOWN_SERVERS was never retried — the transport stayed broken and the check would have reported DOWN indefinitely. It now also retries whenever REWIRE_FAILURES > 0.
• performCheck() and buildStructuredData() share one immutable snapshot. Each previously resolved the transport and re-read the counters independently, which let a single health response contradict itself — a message saying the transport is initialized next to structured data saying it is not.

343865b39e — count invalidations that fail after init

The original change instrumented only the pre-init() drop path. A transport that initialized successfully and then started failing every publish stayed completely invisible: no drops recorded, isInitialized() still true, rewireFailures 0, health check green, other nodes serving stale content.

• Every provider signals a failed send by returning false rather than throwing — JDBCPubSubImpl (the default) and PostgresPubSubImpl on an exception or an execute() that returns false, RedisPubSubImpl when stopped — and send() discarded that boolean.
• Checking it in send() is necessary but not sufficient. DOT_PUBSUB_USE_QUEUE defaults to true, so the provider is normally a QueuingPubSubWrapper whose publish() returns true immediately and completes the real send on a submitter thread, discarding the result — send() would read true 100% of the time in the default configuration. The wrapper now records the outcome of the task it submits, and PubSubCacheTransport sums its own synchronous failures with the provider-reported ones. Exactly one of the two counts any given attempt, so nothing is double counted.
• Counted per topic, because one provider instance is shared by every topic in the JVM (cache invalidation, OSGi restart, cluster management) and a JVM-wide total could not be attributed to the cache transport.
• getFailedMessages() is kept distinct from getDroppedMessages(): dropped means "the transport is down", failed means "the transport believes it is up but sends are erroring". Both lose invalidations, but only the first is visible from isInitialized().

6c19c292eb — stop reporting startup drops as failures

Found by running a two-node cluster from this branch (see Deployment impact below). Boot order guarantees dropped invalidations: caches are invalidated by startup tasks and the starter import long before ClusterFactory wires the cluster and calls init(). A first boot against an empty database dropped 2,841, and the cumulative counter carried that burst for the life of the node.

• Drops from before the first successful init() are retired into a separate startupDroppedMessages counter, so getDroppedMessages() reports only invalidations lost while the transport was expected to be carrying them. Retiring is first-init-only on purpose: a transport that came up, went down, lost invalidations and recovered has lost real ones, and retiring on every re-init would launder genuine loss into the benign startup bucket.
• The health check treats a never-yet-initialized transport as still starting up for health.check.cache-transport.initialization-grace-period-seconds (default 120). The grace period ends permanently once the check has seen an initialized transport — losing one that had been working is reported immediately.
• dotcms.cache.transport.initialized is left raw, with no grace period: a gauge states the current fact, and alert rules add the duration clause (for: 2m). Alert on the health check if you want the grace period applied for you.

5aecea7292 — unit-test the health check

Addresses the review's medium finding: the largest new file in the PR had no coverage. Ten cases over NullTransport, unresolvable cache layer, the grace-period boundaries in both directions, the rewire threshold either side, structured-data keys, the MONITOR_MODE default, and liveness exclusion. Writing them caught a real defect in 6c19c292eb — retireStartupDrops() ran on every init() — which was fixed before that commit landed.

47f54bf7fc — make the init guard atomic

The review's second medium: init() guarded with a check-then-act, so two threads could both read initialized == false and each run start() + subscribe().

The reviewer asked for verification that concurrent invocation is possible before changing anything. It is not, today — init() is reached only through ChainableCacheAdministratorImpl.setCluster() ← addMeToCacheIfNeeded() ← rewireCluster() ← ClusterFactory.rewireClusterIfNeeded(), each with exactly one call site, and the last is static synchronized. Every entry point (ServerHeartbeatJob, FreeServerFromClusterJob, ClusterFactory.initialize() from LicenseManager and two startup tasks) funnels through that monitor. Fixed anyway, because rewireCluster() is public static and not itself synchronized, so the guarantee is not local to this class and a future caller could bypass it.

init() and shutdown() are now synchronized — once per cluster rewire, so the monitor costs nothing.

Deliberately not the compareAndSet form suggested in review. Claiming the flag up front (if (!initialized.compareAndSet(false, true)) return; with the work after) means a thrown start() stays flagged as initialized: isInitialized() returns true, shouldReinit() returns false, setCluster() never retries, and the new health check reports a healthy transport that never subscribed — reintroducing the exact silent failure this issue removes. The flag is set only after start() and subscribe() return. test_failed_init_leaves_the_transport_retryable is the test that would fail under the suggested form.

633580ee3f — normalize the failure-count topic key

Writes into QueuingPubSubWrapper.failedByTopic and reads out of it arrive by routes that disagree on case: recordFailure() keys on the event's topic, which DotPubSubEvent.Builder.withTopic has already lowercased, while PubSubCacheTransport.getFailedMessages() looks up this.topic.getTopic() — a bare String.valueOf(getKey()) with no normalization. They match only because every current key is already lowercase (CACHE_TOPIC is "dotcache_topic"). One uppercase character in a future key and the lookup misses silently, reporting zero failures while invalidations are lost.

Normalized on both sides inside the wrapper, which owns the map, rather than with a .toLowerCase() at the call site as review suggested — that would leave every caller needing to know the wrapper's key convention, and would fix only the cache topic. DotPubSubProvider now documents that the key is matched case-insensitively. Latent fragility, not a live defect.

fb8db88d58 — cover the rewire-retry decision

The rewire retry was the least-protected change in this PR: only the health check's consumption of REWIRE_FAILURES was tested, not the producer, and it is the logic that guarantees a failed transport init keeps being retried.

The decision was an inline condition behind clusterReady(), isEnterprise() and two APILocator calls, so it is extracted into a pure package-private shouldRewire(aliveServers, knownServers, currentServer, pendingFailures). Behaviour unchanged; the rationale that was a comment is now the method's javadoc. Five cases, including the one that motivated the clause — a pending failure forces a retry even when membership has settled back to the stale KNOWN_SERVERS, where the membership comparison alone reports "nothing changed". That test asserts its own precondition first so it cannot pass for the wrong reason.

[dsolistorres]

Deployment impact

Moved out of the PR description to keep it readable. This is the evidence for the alerting-noise question: what an operator actually sees on a node boot, measured before and after the startup-grace change.

Verified on a local two-node cluster (PostgreSQL + OpenSearch, pubsub transport) built from this branch, because the failure mode this PR adds monitoring for is exactly the kind that generates deployment noise if it fires spuriously.

Measured, before the grace period was added — two node boots, one against an empty database and one against a populated one:

Signal	Fresh boot	Populated DB
Init window (first poll → transport initialized)	11s	3s
cache-transport WARN from the MONITOR_MODE conversion	1	1
Throttled drop WARN (at a 5s test interval; production default is 30s)	2	1
Invalidations dropped before init	2,841	6
Overall /dotmgt/health status during the window	DEGRADED	DEGRADED
Readiness/liveness HTTP status	unaffected	unaffected

The conversion WARN is one per boot, not one per poll — the check is polled less often than the window lasts. HTTP status was unaffected throughout: HealthStateManager treats DEGRADED as ready and the check is never a liveness check, so no probe failure and no restart. But deriveOverallStatus() propagates any DEGRADED component to the overall status, so a monitor alerting on status != UP would have gone yellow on every pod start.

Measured again after the fix, on a rebuilt image and a fresh two-node cluster (/dotmgt/health polled once a second from before the container started, 6,110 samples carrying a cache-transport component):

	Fresh boot	Populated DB / joining node
cache-transport DEGRADED polls	0 of 4,304	0 of 1,806
Polls inside the grace window, reported UP with awaitingInitialization: true	17 (11:31:46 → 11:32:06)	24 (13:26:07 → 13:26:33)
cache-transport WARN from the MONITOR_MODE conversion	0	0
Throttled drop WARN (5s test interval)	2	1
droppedInvalidations in steady state	0	0
startupDroppedInvalidations	2,841	6
initing PubSubCacheTransport occurrences	1	1

Both counters now read exactly as intended: the alertable one is 0 on a healthy node and the boot burst is parked in the startup bucket, matching the one-off INFO the transport logs (2841 cache invalidation(s) were dropped before it came up).

Two things the re-measurement corrected in the estimate above. The suppressed window is 17–24 consecutive polls, not the 3–11s inferred from log timestamps — the health result is cached and served for a while after init() succeeds, so the uninitialized verdict persists past the log line. And each of those polls would have been a DEGRADED cache-transport component under the previous commit, so the avoided noise is meaningfully larger than first estimated.

Cluster function was re-verified on the same run: bidirectional PING/PONG between both server IDs, one init() per node, and a cluster-wide cache flush issued on node 1 returned 200 with failedInvalidations: 0 and droppedInvalidations: 0 on both nodes afterwards.

The pre-existing gaps listed under Known gaps do not change this. The Micrometer registry is never initialized in the current build, so none of the gauges above are exported yet and no gauge-based alert can fire regardless of what this PR registers.