Repository navigation
[Fix-18644][Master] Fix serial command races and restart identity reuse - #18645
Conversation
18a0527 to
fee692b
Compare
SbloodyS
added
first time contributor
|
Will there be any issues if the service restarts quickly and assumes the role of the previous leader? |
Thanks for asking. Yes—in our reproduction, the restarted Master became ACTIVE without acquiring ownership of the existing coordinator selector. That selector still belonged to the previous process's etcd lease and was subsequently removed. A matching address does not transfer the old process's lease to the new process. Deployment and operational contextWe run a self-hosted DolphinScheduler 3.4.2 deployment with:
We encountered scheduling problems during Master maintenance, restarts and replica-count changes. The controlled reproduction kept DS at 3.4.2 throughout; it did not involve a mixed-version upgrade. We temporarily reduced the deployment to 1 Master afterward. The proposed HA patch has not been deployed to this environment. Restarting a Master at the same addressIn a separate controlled run on 2026-09-16 (all times below are UTC+8):
The recreated Master 0 became ACTIVE while the coordinator selector was still attached to the predecessor's lease. Both Masters logged ACTIVE and started their coordinators: The etcd watch recorded deletion of the old selector, followed by Master 1 writing a new selector. Selected decoded fields: More than 7 minutes later, both coordinators were still processing the same workflow instance: There are recurring entries between these points, so this was sustained coordinator overlap. These logs do not establish duplicate business writes. The overlap also involved the etcd REMOVE payload issue and failure to demote after losing an election; address reuse was the initial ownership problem, not the entire failure chain. Related scale-down observationIn an earlier round, we scaled from 2 Masters to 1. This exposed the separate REMOVE-event problem tracked in #18640/#18641; it is not the same experiment as the restart above. Before recovery, the database snapshot showed an unassigned workflow waiting to run: The remaining Master logged: We initiated a restart of the remaining Master at approximately 15:00:47, using the same DS 3.4.2 image. The delayed instance then completed: Database readback confirmed instance 111167 was SUCCESS at 15:03:15.105, and the command queue was empty at 15:04:21. Instance 111168 also completed at 15:03:24.923. Their elapsed times were approximately 195 and 85 seconds, compared with about 5 seconds for adjacent executions. These durations include the recovery process.
This screenshot shows the final SUCCESS records after recovery. The fault-time SERIAL_WAIT state is established by the database snapshot; we did not capture a UI screenshot at that point. Supplementary MySQL checkWe also tested the original HA implementation with a real MySQL 8.0.46 Registry. The successor became ACTIVE, but selector row 1 still belonged to the predecessor client, with its value and update time unchanged. This was an isolated fixture deliberately retaining the predecessor, not a production JDBC restart. This is why I think the new process should acquire its own selector rather than adopt the predecessor's role based on its address. Adding an instance timestamp addresses that ownership ambiguity. Handling a lost election and stopping the old coordinator are separate parts of the fix. |
|
While checking the demotion and re-election path, I found a related lifecycle issue in the existing Master coordinators. The exception → standby → retry path in this PR can exercise that sequence, so I have pushed a separate follow-up commit (3bb22b2) rather than treating the role-transition tests as sufficient. The change keeps the existing dedicated-thread structure. Each run has its own stop flag. If the previous run is still finishing, The added tests use real coordinators with deliberately blocked DAO/handler calls, including repeated role changes through This addresses overlap within the same coordinator instance. It does not cancel an already-issued database operation or provide fencing between different Masters. If the old database call never returns, its replacement remains pending rather than running concurrently. |
|
I tested a smaller candidate to check which changes are needed for this fix. 7fab651 now narrows this PR to the instance timestamp, ownership-based role reconciliation, election/role-update synchronization, and the coordinator thread handoff protection from 3bb22b2. It removes immediate demotion on election exceptions, the One clarification to my earlier coordinator comment: the overlap does not require the exception → standby → retry path. Without the handoff protection, tests also reproduced it during normal loss and reacquisition of leadership. A blocked DAO call was allowed to return after reactivation, and both the old and new threads continued polling. This is why I am keeping the handoff protection while removing the broader lifecycle changes. ValidationThe combined candidate passed 26 targeted tests on JDK 8. The final upstream revision was also clean-built through the 132-module Master/Alert reactor on actual Zulu JDK 8 We also tested a 3.4.2 backport in the same Kubernetes / etcd deployment described above, including a leader restart at the same address and scaling from 2 Masters to 1. The backport also covers the serial-coordinator restartability fix already present in dev (#18623). It is not a deployment of dev jars into 3.4.2. Selected fields from the validation logs follow. Times are UTC+8 on 2026-09-16; thread-dump details are omitted for readability. The thread lists contain only the serial and task-group coordinators. The rebuilt Master-0 remained standby while Master-1 owned the selector. After Master-1 was removed, Master-0 acquired the selector and started both coordinators. Natural scheduled executions also completed successfully. For example, selected database fields show: The error-command count remained at its baseline of 12 throughout the recorded checkpoints. The original image and single-Master topology were restored afterward. An earlier attempt restored the original deployment before completing the restart check because the test's 90-second wait was shorter than the configured 120-second termination grace period. The results above are from the subsequent run with the corrected timeout. These observations support the reduced scope for the reported restart and scale-down scenarios. The snapshots do not prove that no transient overlap occurred between observations, and the patch does not provide cross-Master fencing. The separate etcd REMOVE-value fix remains tracked in #18641. |
|
The current code looks too messy and feels overly AI-driven, making it difficult to tell exactly what problem it’s meant to solve. |
@ruanwenjun Thanks for the suggestions. I've updated the implementation to use a single election worker and simplified the lifecycle logic. Demotion and normal Master shutdown now share the same cleanup path: both coordinators are asked to stop before we wait for either worker to finish. |
|
I've updated the PR description to match the current implementation and completed another deployment test of I used a minimal backport to our DolphinScheduler 3.4.2 deployment, running Java 8, Kubernetes 1.33.13 and a 3-member etcd 3.6.14 cluster. The sequence was 1→3 Masters, restarting the elected Master at the same address, then scaling back to 1. The restart reached the retained-selector window. Selected observations from September 17, UTC+8: The recreated process did not inherit its predecessor’s role. Post-restart thread dumps showed both coordinator workers only on master-1. After scale-down, master-0 became the owner and its slots converged 3→2→1 without restarting that surviving Pod. Three SERIAL_WAIT canary instances completed at each stage: 9 workflow runs / 27 Shell tasks, with one actual output marker per task log. The original image and complete StatefulSet configuration were restored afterward. This supports the tested maintenance sequence. Complete live Java shutdown completion was not captured; controlled-DAO tests cover the coordinator shutdown interleavings separately. This was not a partition or all-backend test. One separate issue remained visible: the broad member-prefix subscription also delivers selector events to the heartbeat parser. With the restored DELETE payload, selector REMOVE logs a caught parse error. Member removal and election still completed in this run. |
f4c272e to
3afd40f
Compare
3753a84 to
50f8ad9
Compare
50f8ad9 to
0b43cce
Compare
|
@michaellx1057 OK, I understand so “close” can be understood as a graceful shutdown, while “requestStop” is a forced stop. We would need to use transactions at the upper layer to ensure that even if multiple coordinators exist, the same work is not processed repeatedly. Even if we terminate the thread directly, we must ensure that no data in an intermediate state is generated. |
|
Thanks, @ruanwenjun. I agree that preventing duplicate work across Masters also needs database-level coordination. I’m keeping that work separate while refining the leadership handoff and reducing the remaining flag checks. I’ll verify the blocked-election and shutdown interleavings before posting the next patch. |
michaellx1057
changed the title
Thanks, @ruanwenjun. I've now included the database protection in this PR as a separate commit, The Serial handlers claim a WAITING row before inserting its Command in the same transaction. Discarding also checks WAITING; if another handler has already launched the record, the stale group stops and the next poll reads fresh state. Failed inserts and workflow-state updates roll back the corresponding claim or deletion. On both MySQL 8.0.46 and PostgreSQL 18.4, the original handlers produced 2 Commands from 2 stale WAITING snapshots; the updated handlers produced 1. I also checked both launch/discard orderings and rollback paths. These checks use real database transactions, with stop RPC stubbed. The local validation passed 81 targeted tests on JDK 8, 30 isolated etcd runs and 27 database checks per engine. I've updated the PR description with the results and limits. The new CI workflows are awaiting maintainer approval. Could you approve the runs when convenient? I'll re-request review once CI passes. |
ruanwenjun
left a comment
ruanwenjun
left a comment
There was a problem hiding this comment.
Since we’ve already controlled concurrency at the database layer using transactions, I don’t think any changes are needed at the higher levels.
|
@michaellx1057 And then do we still need |
Thanks, @ruanwenjun. I've removed the separate The tradeoff is that Serial may keep handling work while TaskGroup drains; a stuck TaskGroup call also delays Serial's stop. This accepts sequential graceful shutdown, with the database guards handling races over the same serial record. All 48 comparison runs across both stop variants on MySQL and PostgreSQL passed. The simplified version also passed 77 targeted test executions on JDK 8, including two shutdown scenarios repeated three times each. |
ruanwenjun
left a comment
ruanwenjun
left a comment
There was a problem hiding this comment.
Please remove the unrelated change.
|
Thanks @ruanwenjun. I've reworked the HA handling to use two workers: one serializes role changes and Coordinator callbacks, while the other performs Registry operations. This lets Master process its own selector removal even when an earlier Registry check is blocked. Each Registry lock is acquired and released on the same thread, and version checks prevent superseded results from publishing ACTIVE. Master still demotes on its own selector removal; Alert checks ownership first. Unresolved ownership recovery continues without requiring another REMOVE notification. Shutdown cancels pending recovery and rejects late results. The database claim protection remains in place. There are still trade-offs: blocked Registry calls can delay recovery, and delayed watch notifications can leave a temporary overlap between nodes. Coordinator shutdown remains synchronous, so blocked DAO work can also delay completion. This does not provide cluster-wide fencing. Local validation passed:
These covered startup races, stale results, delayed REMOVE, recovery after 20 consecutive read failures, and shutdown interleavings. The cross-Registry runs confirmed demotion, takeover, rejection of old results and lock release. We observed no overlap between old and replacement Coordinator workers on the same Master in these runs. These runs used real Registry services and Coordinator threads with test DAOs. |
|
Thanks @ruanwenjun. I've switched the HA queue to Retries use delayed events. Master still demotes first; Alert checks ownership first. Close wakes the role worker without interrupting admitted callbacks or Registry calls. The database guards and coordinator shutdown/restart constraints remain unchanged. I've also documented the worker/lock contract and the blocking API. JDK 8 checks passed: 108 targeted tests, with the 40-test HA suite run 3 times. The runtime implementation also passed 42 isolated etcd runs and 9 cross-Registry runs; the PR description records the validation scope and rounds. |
michaellx1057
changed the title
Run ownership reconciliation on a single election worker. Request both coordinator workers to stop before joining either on demotion and normal Master shutdown. Add deterministic lifecycle and shutdown regression coverage.
- Reconcile ownership before publishing roles - Stop both coordinators without waiting for blocked election calls - Track deferred recovery across demotion and close
- Claim WAITING rows before inserting Commands in one transaction - Reject stale discards and retry groups from fresh snapshots - Roll back failed inserts and workflow state updates
Use the requested DAO/Mapper name and record stale WAITING snapshots in the shared launch/discard transaction helpers.
Remove the separate requestStop API and keep worker interruption and drain in close. Retain database claim/discard protection and cover close during a PRIORITY RPC and Master shutdown with pending recovery.
Serialize role changes and coordinator callbacks on one worker while keeping Registry lock operations on a separate worker. Reject superseded results, retain ownership recovery, and handle cancellation and listener self-close. Verified on JDK 8 with 117 targeted tests, 48 isolated etcd runs, and 9 ZooKeeper/JDBC compatibility runs. Business DAOs in HA probes are stubbed.
Replace custom HA role scheduling with the existing EventBus. Keep one role worker for transitions and callbacks and one Registry worker for single ownership checks. Preserve current-result admission, bounded ordinary retries and unresolved own-removal recovery. Wake the role worker on close without interrupting admitted callbacks or Registry calls. Document the concurrency contract and cover repeated external close calls waiting for the same listener. Validated on JDK 8 with 108 targeted tests and three runs of the 40-test HA suite. The runtime implementation passed 42 isolated etcd runs and 9 cross-Registry runs before the final documentation/test-only cleanup.
a7c1144 to
c7480c8
Compare
|
@SbloodyS PTAL. |
Was this PR generated or assisted by AI?
YES — Codex assisted with the analysis, implementation, tests and this description.
Purpose of the pull request
Related to #18644.
This PR distinguishes HA server instances at the same address and protects SerialCommand transitions at the database layer. A restarted process should not treat its predecessor's selector as its own. When coordinators hold stale WAITING snapshots, only the transaction that claims the row may insert its Command.
The HA worker/EventBus and Coordinator lifecycle changes have been removed from this PR. Startup/watch races and Coordinator lifecycle fixes are left for a separate follow-up.
Brief change log
serverIdentify.Registry paths, the Registry SPI, database schema and persisted queue states are unchanged.
Verify this pull request
The reduced patch was rebuilt and tested using Zulu JDK 8 (
1.8.0_512, class version 52).git diff --checkpassedThe database checks use production handlers, DAOs, mapper XML and real Spring/JDBC transactions. Stale WAITING snapshots were checked under WAIT, DISCARD and PRIORITY. The 2- and 3-transaction claim races use WAIT to exercise the shared
launchSerialCommandpath. The checks also cover both launch/discard orderings, insertion-failure/zero-row rollback and workflow-update rollback. Stop RPCs are stubbed; these are database integration checks, not complete Master deployment tests. The previous combined HA validation is not used as acceptance evidence for this reduced patch.The timestamp is not a strict uniqueness guarantee for same-millisecond construction or clock rollback. The database guards protect the same SerialCommand when competing writers use the updated handlers; they do not establish cluster-wide fencing or global ordering across different records. No production environment was changed during this validation.
Pull Request Notice
Pull Request Notice