dotCMS / dotCMS/core

PGListener lifecycle: unbounded rebuilds with no backoff, leak window before Thread.start(), stop() allocates, flag-based idempotency guard

Open
#36,802 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

dotCMS : Clustering OKR : Customer Support Team : Maintenance Type : Defect
Dominant language
Java
Stars
970
Forks
486
Avg merge
3d 33m
Merged PRs (30d)
170

Description

Problem Statement

The PGListener lifecycle inside JDBCPubSubImpl has four independent defects that together turned a transient connection failure into the unbounded rebuild storm analysed in spike #36544 (~3,687 listener instantiations in one 600s window).

1. Rebuilds are unbounded — no backoff, no rate limit, no cap. listener() constructs a fresh PGListener whenever the current one is not listening, and six call sites reach it, including publish() (:291) — so ordinary cache-invalidation traffic drives reconstruction. PostgresPubSubImpl already has the right shape: linear backoff min(delay + 1000, 10000), reset on success (:189, :99).

2. A real leak window between the constructor and Thread.start(). The connection is opened in the constructor (:128) but the thread only starts at :69. Since run()'s finally { stopListening(); } (:177-184) is what closes the connection, anything throwing in that window leaks it permanently — e.g. unwrap(PGConnection.class) failing at :122, or Thread.start() failing under a thread storm.

Note for anyone reading the original hypothesis in #36544: the ordinary death paths do not leak. runInternal()'s return on !connectionAlive() and the KILL_ON_FAILURES throw are both covered by that finally. subscribeTopic() failure is also safe — it closes before re-throwing (:155). This narrow pre-start() window is the only genuine leak.

3. stop() can construct a listener in order to destroy one. stop() is listener().stopListening() (:102). If the listener is already dead, listener() builds a complete replacement — borrow + LISTEN every topic + Thread.start() — purely to stop it again. Shutdown paths must not allocate.

4. The idempotency guard tests a flag, not the resource. isListening() reads runstate (:172-174), which lags reality by up to SLEEP_BETWEEN_RUNS (500ms) and diverges entirely when the connection dies underneath the thread. PostgresPubSubImpl checks the real resource: connection != null && !connection.isClosed() (:146).

Impact: any clustered install on the default provider. Amplifies #36544's pool exhaustion; independently causes connection churn and wasted LISTEN traffic even on a healthy cluster.

Steps to Reproduce

Harness and reproducer on branch issue-36544-pubsub-connection-churn-spike:

git fetch origin issue-36544-pubsub-connection-churn-spike
git checkout issue-36544-pubsub-connection-churn-spike
cd docker/docker-compose-examples/pubsub-connection-churn
docker compose up -d && ./churn.sh

Observe that pg_stat_statements.calls for LISTEN cluster_actions climbs ~1:1 with kill cycles, with no backoff between attempts:

SELECT calls, query FROM pg_stat_statements WHERE query ILIKE 'LISTEN %';

For defects 3 and 4 specifically, the integration reproducer isolates them (remove @Ignore to run):

./mvnw verify -pl :dotcms-integration -Dcoreit.test.skip=false \
    -Dit.test=JDBCPubSubImplConnectionChurnReproTest

repro_stopOnDeadListenerMustNotBuildAReplacement asserts that a second stop() does not replace the listener instance — it currently does.

Acceptance Criteria

  • Listener reconstruction applies incremental backoff with a ceiling, reset on success — pattern available at PostgresPubSubImpl.java:189.
  • A monotonic rebuild counter exists and is observable, so runaway reconstruction is visible rather than inferred from pg_stat_statements.
  • The connection opened in the PGListener constructor is closed on every failure path before the thread starts, including unwrap() failure (:122) and Thread.start() failure (:69). Acquisition and release must live in the same scope.
  • stop() on an already-stopped or never-started provider is a no-op: it does not construct a listener, borrow a connection, issue LISTEN, or start a thread.
  • unsubscribe() (:284-285) is reviewed for the same stop-then-immediately-rebuild pattern.
  • The reuse/idempotency guard is based on the connection's actual state rather than the runstate flag, so a listener whose connection died underneath it is not reported as listening.
  • repro_listenerChurnMustNotAccumulateConnectionsOrThreads and repro_stopOnDeadListenerMustNotBuildAReplacement both pass with @Ignore removed, and the class is registered in the appropriate integration suite.
  • Verified with the harness: under sustained connection kills, the LISTEN counter grows sub-linearly (backoff observable) and no PGListener Pub/Sub Thread threads accumulate.

dotCMS Version

Incident on 26.07.06-01 (commit 5efb47a). JDBCPubSubImpl default since #26706 / #27117 (Dec 2023) — released in 24.01.26, backported to LTS 22.03.14, 23.01.11, 23.10.24 v3. Code re-verified against main.

Severity

High - Major functionality broken

Links

  • Spike with full RCA: #36544 — see docs/core/incidents/36544-pubsub-connection-churn.md on branch issue-36544-pubsub-connection-churn-spike
  • Parent epic: #34837
  • Freshdesk ticket #38172
  • Reference implementation for backoff and idempotency: PostgresPubSubImpl.java:146, 189

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with JDBCPubSubImpl and PGListener, then compare the backoff and resource-state handling in PostgresPubSubImpl. Run JDBCPubSubImplConnectionChurnReproTest and inspect the listed churn harness and integration tests. Done means both reproducers pass, failure paths close connections, stop and unsubscribe avoid replacement construction, and sustained kills show bounded churn without accumulating threads.

Written by the indexing model from the issue text.

Assessment

Tech stack
java, postgresql
Domain
backend, databases, distributed-systems
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.