[Fix-18274][Registry] Handle expired JDBC heartbeat sessions by qiuyanjun888 · Pull Request #18416 · apache/dolphinscheduler

qiuyanjun888 · 2026-07-15T04:34:09Z

Was this PR generated or assisted by AI?

YES. This pull request was assisted by Hermes Agent / OpenAI Codex for code changes, focused tests, review feedback analysis, and local verification. The scope and final submission were directed by the contributor.

Purpose of the pull request

Closes #18274.

When the JDBC registry database is unavailable longer than the session timeout, another server can purge the stale heartbeat row. After recovery, the still-running client previously ignored the zero-row heartbeat update and incorrectly treated the refresh as successful.

This is an independent alternative related to #18275. It addresses the outstanding technical concerns discussed there by not recreating or upserting an expired heartbeat, which could revive a failed-over identity. Instead, a missing heartbeat enters the existing disconnect state machine so the owning service can terminate.

Brief change log

Treat a zero-row JDBC heartbeat update as an expired registry session.
Stop heartbeat refresh work after the registry server reaches DISCONNECTED.
Persist the current heartbeat timestamp and add focused state-machine regression tests.

Verify this pull request

This change added tests and can be verified as follows:

./mvnw clean -pl dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc -am -DskipITs -Dtest=JdbcRegistryServerTest,JdbcRegistryDataChangeListenerAdapterTest -Dsurefire.failIfNoSpecifiedTests=false test
Result: 4 tests, 0 failures, 0 errors, and BUILD SUCCESS across the selected reactor.
./mvnw -pl dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc spotless:apply

Pull Request Notice

This pull request contains no incompatible change.

SbloodyS · 2026-07-15T07:02:17Z

When updateById(clone) returns false, the new RegistryException is caught by the generic error handler. If the server is currently STARTED, the catch block only moves it to SUSPENDED; it does not immediately invoke onDisConnected().

This can happen after a long JVM pause or heartbeat scheduler starvation:

Another registry server considers the session expired and removes its heartbeat, ephemeral data, and locks.
The original server resumes while its local state is still STARTED.
updateById() returns false.
The original server enters SUSPENDED and remains active until at least the next heartbeat cycle.

A missing heartbeat row is definitive evidence that the session has expired, not a transient database error. Continuing to run after that point can overlap with a server that has already taken over, leaving a split-brain window.

Please handle this condition separately and transition directly to DISCONNECTED, triggering the disconnection callback exactly once.

The new test sets the server state to SUSPENDED and lastSuccessHeartbeat to zero, so it bypasses the problematic STARTED branch. Please also add a test verifying that a zero-row heartbeat update while STARTED causes an immediate disconnection.

qiuyanjun888 · 2026-07-17T00:30:19Z

When updateById(clone) returns false, the new RegistryException is caught by the generic error handler. If the server is currently STARTED, the catch block only moves it to SUSPENDED; it does not immediately invoke onDisConnected().

This can happen after a long JVM pause or heartbeat scheduler starvation:

Another registry server considers the session expired and removes its heartbeat, ephemeral data, and locks.

The original server resumes while its local state is still STARTED.

updateById() returns false.

The original server enters SUSPENDED and remains active until at least the next heartbeat cycle.

A missing heartbeat row is definitive evidence that the session has expired, not a transient database error. Continuing to run after that point can overlap with a server that has already taken over, leaving a split-brain window.

Please handle this condition separately and transition directly to DISCONNECTED, triggering the disconnection callback exactly once.

The new test sets the server state to SUSPENDED and lastSuccessHeartbeat to zero, so it bypasses the problematic STARTED branch. Please also add a test verifying that a zero-row heartbeat update while STARTED causes an immediate disconnection.

Thanks for your suggestions! I have already implemented, can you please help review?

SbloodyS · 2026-07-17T01:54:28Z

Thanks for addressing the previous feedback. A zero-row heartbeat update now disconnects the server immediately, and the added test covers the STARTED path.

However, there is still a shutdown race:

refreshClientsHeartbeat() passes the initial state check while the server is STARTED.
close() concurrently changes the state to STOPPED and deletes the heartbeat rows.
The in-flight updateById() returns false.
This branch unconditionally overwrites STOPPED with DISCONNECTED and invokes onDisConnected() during a normal shutdown.

Previously, the exception handler inspected the current state, so STOPPED was left unchanged. Please make the transition to DISCONNECTED conditional/atomic so that close() cannot race with the heartbeat task and have its terminal state overwritten. A regression test coordinating an in-flight heartbeat update with close() would also be useful.

qiuyanjun888 · 2026-07-20T03:55:32Z

Addressed the shutdown race described in #18416 (comment) in commit 8d9599e.

The transition to DISCONNECTED is now conditional and synchronized with close(), so a completed STOPPED transition cannot be overwritten and the disconnection callback is not invoked when close wins.
Added a latch-controlled regression test that holds an in-flight heartbeat update, completes close(), then returns a zero-row update result.

Validation:

JdbcRegistryServerTest: 5 tests, 0 failures, 0 errors
module spotless:check: BUILD SUCCESS

Could you please take another look?

SbloodyS

There is still a shutdown race in the other state transitions.

close() and transitionToDisconnected() are synchronized, but the successful heartbeat and exception paths still update jdbcRegistryServerState directly:

SUSPENDED -> STARTED after a successful refresh
STARTED -> SUSPENDED after a refresh exception

For example:

The heartbeat thread observes SUSPENDED.
close() changes the state to STOPPED.
The heartbeat thread writes STARTED and invokes onReconnected().

The exception path can similarly overwrite STOPPED with SUSPENDED. Since the field is also neither volatile nor consistently accessed under the same lock, state visibility is not guaranteed.

Please make all state transitions atomic and use one synchronization strategy for every read/write, such as synchronized transition methods or an AtomicReference with compare-and-set. Connection callbacks should only run when their corresponding transition succeeds.

Please also add regression coverage for close() racing with:

A successful heartbeat while the server is SUSPENDED.
A heartbeat exception while the server is STARTED.

In both cases, STOPPED must remain the final state and no reconnect/disconnect callback should be triggered after shutdown.

qiuyanjun888 · 2026-07-30T05:12:08Z

Please make all state transitions atomic and use one synchronization strategy for every read/write. Connection callbacks should only run when their corresponding transition succeeds.

Thanks for pointing this out. Fixed in commit 79cc4a0cde.

The server state now uses one synchronization strategy consistently:

start(), getServerState(), the close() state update, and all heartbeat state transitions synchronize on the same server instance.
Successful heartbeats use a guarded SUSPENDED -> STARTED transition.
Heartbeat exceptions use a guarded STARTED -> SUSPENDED transition.
Expired heartbeat records use a guarded STARTED/SUSPENDED -> DISCONNECTED transition.
Reconnect and disconnect callbacks run inside their corresponding synchronized transition, only after the expected source state is confirmed. If close() wins and sets STOPPED, later heartbeat results cannot overwrite it or emit reconnect/disconnect callbacks.

I also added the two requested race regressions:

close() racing with a successful heartbeat while SUSPENDED.
close() racing with a heartbeat exception while STARTED.

Both assert that STOPPED remains final and that no reconnect/disconnect callback fires after shutdown.

Verification:

./mvnw clean -pl dolphinscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-jdbc -am -DskipITs -Dtest=JdbcRegistryServerTest -Dsurefire.failIfNoSpecifiedTests=false test

Result: 7 tests, 0 failures, 0 errors; reactor build succeeded.

SbloodyS

LGTM

SbloodyS · 2026-09-01T07:23:37Z

PTAL @ruanwenjun

ruanwenjun · 2026-09-02T01:55:54Z

+                    log.error("The client heartbeat has expired: {}", jdbcRegistryClientHeartbeatDTO.getId());
+                    transitionToDisconnected();
+                    return;


Suggested change

log.error("The client heartbeat has expired: {}", jdbcRegistryClientHeartbeatDTO.getId());

transitionToDisconnected();

return;

throw xxException()

Should consider session timeout.

I checked this against the session lifecycle. updateById returning false means the heartbeat row no longer exists, and the normal purge path removes that row only after its stored session timeout has elapsed. The session has therefore already expired; waiting for another local timeout would keep a stale server active and reintroduce the split-brain window this PR fixes. I kept the immediate DISCONNECTED transition, with the STARTED zero-row regression test covering this case.

Only a session timeout can cause the status to change to "disconnected".

Addressed in 71e8a14. A failed heartbeat update now enters SUSPENDED first; the server transitions to DISCONNECTED only after the configured session timeout. The regression test covers the STARTED -> SUSPENDED -> DISCONNECTED sequence.

…15T021630Z

ruanwenjun · 2026-09-14T02:57:11Z

+                    log.error("The client heartbeat has expired: {}", jdbcRegistryClientHeartbeatDTO.getId());
+                    transitionToDisconnected();
+                    return;


Only a session timeout can cause the status to change to "disconnected".

ruanwenjun · 2026-09-14T02:59:41Z

 @Slf4j
 public class JdbcRegistryServer implements IJdbcRegistryServer {

+    private static final AtomicReferenceFieldUpdater<JdbcRegistryServer, JdbcRegistryServerState> SERVER_STATE_UPDATER =


Use AtomicReference, don't write field name jdbcRegistryServerState here.

Addressed in 71e8a14. Server state is now stored in an AtomicReference, so reads and compare-and-set transitions no longer depend on AtomicReferenceFieldUpdater or a field-name string.

ruanwenjun · 2026-09-14T03:02:40Z

+    private void transitionToStarted() {
+        if (SERVER_STATE_UPDATER.compareAndSet(
+                this, JdbcRegistryServerState.SUSPENDED, JdbcRegistryServerState.STARTED)) {
+            doTriggerReconnectedListener();
+        }
+    }
+
+    private void transitionToSuspended() {
+        SERVER_STATE_UPDATER.compareAndSet(
+                this, JdbcRegistryServerState.STARTED, JdbcRegistryServerState.SUSPENDED);
+    }
+
+    private void transitionToDisconnected() {
+        while (true) {
+            JdbcRegistryServerState currentState = jdbcRegistryServerState;
+            if (currentState != JdbcRegistryServerState.STARTED
+                    && currentState != JdbcRegistryServerState.SUSPENDED) {
+                return;
+            }
+            if (SERVER_STATE_UPDATER.compareAndSet(
+                    this, currentState, JdbcRegistryServerState.DISCONNECTED)) {
+                doTriggerOnDisConnectedListener();
+                return;
+            }
+        }
+    }


Need to handle the status set failed case.

Addressed in 71e8a14. CAS failures are now handled explicitly: the SUSPENDED transition records the current state, and the DISCONNECTED transition logs the failed attempt and retries after re-reading the state. The listener runs only after a successful state change.

All compareAndSet failed should be handled. Please don't reply to me with an AI response, you should at least know what the reviewer means.

ruanwenjun · 2026-09-15T01:51:23Z

    public void close() {
-        jdbcRegistryServerState = JdbcRegistryServerState.STOPPED;
+        synchronized (this) {
+            if (serverState.getAndSet(JdbcRegistryServerState.STOPPED) == JdbcRegistryServerState.STOPPED) {


Nit, use compareAndSet.

ruanwenjun · 2026-09-15T01:53:43Z

+    private void transitionToStarted() {
+        if (SERVER_STATE_UPDATER.compareAndSet(
+                this, JdbcRegistryServerState.SUSPENDED, JdbcRegistryServerState.STARTED)) {
+            doTriggerReconnectedListener();
+        }
+    }
+
+    private void transitionToSuspended() {
+        SERVER_STATE_UPDATER.compareAndSet(
+                this, JdbcRegistryServerState.STARTED, JdbcRegistryServerState.SUSPENDED);
+    }
+
+    private void transitionToDisconnected() {
+        while (true) {
+            JdbcRegistryServerState currentState = jdbcRegistryServerState;
+            if (currentState != JdbcRegistryServerState.STARTED
+                    && currentState != JdbcRegistryServerState.SUSPENDED) {
+                return;
+            }
+            if (SERVER_STATE_UPDATER.compareAndSet(
+                    this, currentState, JdbcRegistryServerState.DISCONNECTED)) {
+                doTriggerOnDisConnectedListener();
+                return;
+            }
+        }
+    }


All compareAndSet failed should be handled. Please don't reply to me with an AI response, you should at least know what the reviewer means.

ruanwenjun · 2026-09-15T01:56:48Z

-                jdbcRegistryServerState = JdbcRegistryServerState.STARTED;
-                doTriggerReconnectedListener();
-            }
+            transitionToStarted();


Don't add such method, looks so strange, this only transite from SUSPENDED to STARTED.

ruanwenjun · 2026-09-15T01:57:07Z

+    private void transitionToSuspended() {
+        if (!serverState.compareAndSet(JdbcRegistryServerState.STARTED, JdbcRegistryServerState.SUSPENDED)) {
+            log.debug("Failed to transition JdbcRegistryServer to SUSPENDED; current state is {}", serverState.get());
+        }
+    }


Please remove this kind of method.

qiuyanjun888 requested review from Gallardot and ruanwenjun as code owners July 15, 2026 04:34

github-actions Bot added backend test labels Jul 15, 2026

github-actions Bot assigned qiuyanjun888 Jul 15, 2026

SbloodyS added the bug Something isn't working label Jul 17, 2026

SbloodyS added this to the 3.5.0 milestone Jul 17, 2026

SbloodyS reviewed Jul 27, 2026

View reviewed changes

qiuyanjun888 and others added 4 commits July 30, 2026 13:00

[Fix-18274][Registry] Handle expired JDBC heartbeat sessions

0a31733

[Fix-18274][Registry] Disconnect expired JDBC sessions immediately

2cf868b

[Fix-18274][Registry] Preserve stopped state during heartbeat

cbdfd1d

[Fix-18274][Registry] Make heartbeat state transitions atomic

79cc4a0

qiuyanjun888 force-pushed the Fix-18274-alt-20260715T021630Z branch from 1a62c2a to 79cc4a0 Compare July 30, 2026 05:06

Merge branch 'dev' into Fix-18274-alt-20260715T021630Z

eed6305

SbloodyS previously approved these changes Sep 1, 2026

View reviewed changes

Merge branch 'dev' into Fix-18274-alt-20260715T021630Z

93e0b2f

ruanwenjun reviewed Sep 2, 2026

View reviewed changes

SbloodyS modified the milestones: 3.4.3, 3.5.0 Sep 7, 2026

ruanwenjun mentioned this pull request Sep 8, 2026

[Bug] [Master] WorkflowSerialCoordinator cannot restart after HA failover, serial dispatch dies silently #18624

Closed

3 tasks

qiuyanjun888 added 2 commits September 12, 2026 20:45

Merge remote-tracking branch 'upstream/dev' into Fix-18274-alt-202607…

7a87311

…15T021630Z

[Fix-18274][Registry] Address JDBC registry state review

fddfbe1

qiuyanjun888 dismissed SbloodyS’s stale review via fddfbe1 September 12, 2026 12:57

qiuyanjun888 requested a review from ruanwenjun September 12, 2026 12:58

qiuyanjun888 requested a review from SbloodyS September 12, 2026 12:58

ruanwenjun reviewed Sep 14, 2026

View reviewed changes

qiuyanjun888 requested a review from ruanwenjun September 14, 2026 04:28

qiuyanjun888 and others added 2 commits September 14, 2026 12:32

[Fix-18274][Registry] Address JDBC registry review comments

71e8a14

Merge branch 'dev' into Fix-18274-alt-20260715T021630Z

d7d0910

ruanwenjun requested changes Sep 15, 2026

View reviewed changes

Conversation

qiuyanjun888 commented Jul 15, 2026

Was this PR generated or assisted by AI?

Purpose of the pull request

Brief change log

Verify this pull request

Pull Request Notice

Uh oh!

SbloodyS commented Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qiuyanjun888 commented Jul 17, 2026

Uh oh!

SbloodyS commented Jul 17, 2026

Uh oh!

qiuyanjun888 commented Jul 20, 2026

Uh oh!

SbloodyS left a comment

Choose a reason for hiding this comment

Uh oh!

qiuyanjun888 commented Jul 30, 2026

Uh oh!

SbloodyS left a comment

Choose a reason for hiding this comment

Uh oh!

SbloodyS commented Sep 1, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SbloodyS commented Jul 15, 2026 •

edited

Loading