[Fix][Zeta] Remove finished-job IMap listeners when coordinator service is cleared by DanielLeens · Pull Request #11809 · apache/seatunnel

DanielLeens · 2026-08-14T17:12:21Z

Purpose of this pull request

Each activation of a Zeta master constructs a JobHistoryService and registers three listeners on the cluster-wide finished-job IMaps. Previously, coordinator teardown discarded the service without deregistering its listeners. Repeated master-role transitions therefore retained old service instances and duplicated expiration callbacks, including clean-log fan-out.

The service now owns its listener registration IDs and implements AutoCloseable. Coordinator teardown closes the captured service instance belonging to that activation. If listener registration fails partway through construction, the constructor attempts to remove every registration it already acquired before rethrowing the original failure. Each removal is independent, so one removal failure does not skip the other maps. Expected stopped-Hazelcast failures are logged at fine level; unexpected failures remain warnings with registration IDs.

Closing removes listeners only. Existing history reads remain usable after master step-down, and every subsequent activation constructs a fresh service with new listeners. This invariant is documented at the getter and initialization site.

Does this PR introduce any user-facing change?

No configuration, SPI, checkpoint/savepoint, serialized-state, or existing read contract changes. The added AutoCloseable interface is additive. Finished-job expiration callbacks stop being duplicated by stale service instances.

How was this patch tested?

JobHistoryServiceListenerCleanupTest uses real Hazelcast maps to check live registration IDs, repeated create/close cleanup, and coordinator-teardown cleanup. Test-owned services use try-with-resources, so assertion failures cannot strand listeners. The isolated coordinator test uses loopback addresses and an allocated port range.
JobHistoryServiceRegistrationTest adds seven deterministic scenarios: all three constructor failure positions, failure while rolling back a registration, expected and unexpected removal failures, and history reads after repeated close.
Module-scoped ./mvnw -nsu -pl seatunnel-engine/seatunnel-engine-server -DskipTests spotless:apply and git diff --check passed.
One independent maintainer source review passed. GitHub CI on ab1538a9c1241741940cc789e88fb79bf18df608 passed code style, all four unit-test jobs (JDK 8/11 on Linux/Windows), and both engine integration-test jobs (JDK 8/11). No local compile/test or engine-UI build was run.
The full Build is not green: transform-v2-it-part-1 (8) failed in the Flink TestFilterRowKindIT.testFilterRowKindMultiTable case with row counts 87/125/150 against 100, matching existing [Bug][Connector-V2][Assert] Multi-table row-count rules flaky on Flink: AssertSinkWriter uses static JVM-wide counters evaluated per-subtask close() #12116 (fix in [Fix][Connector-V2][Assert] Evaluate row-count rules once per table after the last writer closes #12216). Other CI jobs remain active. This unrelated assertion-connector defect is not changed by this PR.

Check list

No new dependency or pom.xml change.
No configuration, connector registration, SPI, or serialized-state removal.
No documentation or incompatible-changes entry required for an internal lifecycle fix.
Final diff is limited to the service, coordinator lifecycle documentation, and two regression-test classes.

…ce is cleared JobHistoryService registers expiration listeners on the cluster-wide finished-job IMaps in its constructor, and CoordinatorService creates a new instance every time a node becomes the active master. There was no deregistration path, so repeated master role switches accumulated stale listeners, kept old service instances reachable and duplicated finished-job expiration side effects such as CleanLogOperation fan-out. Store the listener registration ids, expose JobHistoryService.close() to deregister them, and call it from clearCoordinatorService(). Fixes apache#11807

DanielLeens

What Problem Does This PR Solve?

User pain point: JobHistoryService registers three Hazelcast EntryExpiredListeners (on the cluster-wide IMAP_FINISHED_JOB_STATE, IMAP_FINISHED_JOB_METRICS, IMAP_FINISHED_JOB_VERTEX_INFO IMaps) in its constructor, and CoordinatorService.checkNewActiveMaster() creates a brand-new JobHistoryService instance every time a node becomes the active master. Prior to this PR there was no deregistration path, so every active → inactive → active master transition (failover, split-brain healing, rolling restart) leaked the previous three listener registrations. Because the listener classes (FinishedJobExpiredListener, JobInfoExpiredListener) are non-static inner classes, each leaked registration keeps the old JobHistoryService instance reachable from the IMap's listener registry, and a single finished-job expiration event fires the callback once per leaked instance — duplicating CleanLogOperation RPC fan-out to member nodes and over-counting the finished-job cleanup totals.
Fix approach: JobHistoryService now captures the UUID returned by each addEntryListener(...) call and exposes a new close() method that deregisters exactly those three listeners (best-effort, swallowing exceptions so cleanup never breaks the master-switch flow). CoordinatorService.clearCoordinatorService() — the single method invoked both when a node leaves the active-master role and during full coordinator shutdown — now calls jobHistoryService.close(). The jobHistoryService field itself is deliberately left non-null so in-flight readers (e.g. REST handlers) keep working after the role switch; only the listener registrations are removed.
One-sentence summary: This PR closes a listener-leak/duplicate-side-effect bug on Zeta master-role transitions by tracking and explicitly deregistering the three finished-job IMap expiration listeners when the coordinator service is cleared.

1. Code Change Review

1.1 Core Logic Analysis

Before:

this.finishedJobStateImap.addEntryListener(
        new FinishedJobExpiredListener<>(Constant.IMAP_FINISHED_JOB_STATE), true);
this.finishedJobMetricsImap.addEntryListener(
        new FinishedJobExpiredListener<>(Constant.IMAP_FINISHED_JOB_METRICS), true);
this.finishedJobDAGInfoImap.addEntryListener(
        new JobInfoExpiredListener(Constant.IMAP_FINISHED_JOB_VERTEX_INFO), true);

The registration ids were discarded, and CoordinatorService.clearCoordinatorService() had no call site that ever touched these listeners.

After (JobHistoryService.java:154-166):

this.finishedJobStateListenerId =
        this.finishedJobStateImap.addEntryListener(
                new FinishedJobExpiredListener<>(Constant.IMAP_FINISHED_JOB_STATE), true);
this.finishedJobMetricsListenerId = ...
this.finishedJobDAGInfoListenerId = ...

and a new close() (JobHistoryService.java:184-195) plus removeEntryListenerQuietly() helper (JobHistoryService.java:450-457) that deregisters each id individually, logging (not throwing) on failure. CoordinatorService.clearCoordinatorService() calls it at CoordinatorService.java:1211-1213:

if (jobHistoryService != null) {
    jobHistoryService.close();
}

Verification against the surrounding code (I read the full CoordinatorService.java and JobHistoryService.java, not just the diff hunks):

FinishedJobExpiredListener and JobInfoExpiredListener are confirmed non-static inner classes (JobHistoryService.java:481, :494), so the PR's stale-reference claim is accurate — a leaked registration really does pin the enclosing JobHistoryService (and everything it references) in the IMap listener registry.
JobInfoExpiredListener.entryExpired() (JobHistoryService.java:501-537) is the concrete duplicated side effect: on every invocation it calls incrementFinishedJobCleanupTotal(storeName) and then, for every address in jobDagInfo.getHistoryExecutionPlan() plus the DAG's master address, synchronously sends a CleanLogOperation via NodeEngineUtil.sendOperationToMemberNode(...).join(). With N leaked listeners, one real expiration event turns into N duplicate cleanup-counter increments and N duplicate CleanLogOperation RPC fan-outs — exactly what the PR description claims.
initCoordinatorService() (CoordinatorService.java:515-543) is the only production call site that constructs JobHistoryService (confirmed via gh api search/code — no other new JobHistoryService( call exists in the repo), and it always assigns a fresh instance to the same jobHistoryService field without needing to itself close the old one, because by the time a node re-enters initCoordinatorService() it must have already gone through clearCoordinatorService() on the previous role loss (state machine below) — so the fix's single call site is complete; there is no second leak path.
clearCoordinatorService() is guarded by coordinatorServiceCleared.compareAndSet(false, true) (CoordinatorService.java:1174-1176), so the new close() call executes exactly once per master-role-loss/shutdown cycle, not on every scheduler tick.
shutdown() (CoordinatorService.java:1966-1980) also funnels through clearCoordinatorService(), so full node shutdown is covered by the same fix, matching the PR description.

Runtime path (state-machine / distributed-listener perspective, since this touches Zeta's master-election and IMap-listener lifecycle):

masterActiveListener (single-thread ScheduledExecutorService, 100ms fixed rate)
  -> checkNewActiveMaster()                                    [CoordinatorService.java:1133]

Case A: node LEAVES active-master role (isActive==true, isMasterNode()==false)
  -> seaTunnelServer.stopRealtimeMetricsService()
  -> clearCoordinatorService()  [synchronized]                 [CoordinatorService.java:1172]
       -> coordinatorServiceCleared CAS(false->true) guard, no-op if already cleared
       -> interrupt all running JobMaster instances; drain pendingJobQueue if WAIT strategy
       -> executorService.shutdownNow() + awaitTermination(20s)
       -> [FIX] jobHistoryService.close()                      [CoordinatorService.java:1211-1213]
            -> JobHistoryService.close()                       [JobHistoryService.java:184]
                 -> removeEntryListenerQuietly(finishedJobStateImap, id, ...)
                 -> removeEntryListenerQuietly(finishedJobMetricsImap, id, ...)
                 -> removeEntryListenerQuietly(finishedJobDAGInfoImap, id, ...)
                 each call: imap.removeEntryListener(UUID) — a Hazelcast distributed
                 listener-registry update; failures are caught and logged as WARNING,
                 never propagated (best effort, matches "must not break master switch").
       -> resourceManager.close(); eventProcessor.close()
    jobHistoryService field itself is left non-null (read paths keep working).

Case B: node BECOMES active master (isActive==false, isMasterNode()==true)
  -> initCoordinatorService()                                  [CoordinatorService.java:515]
       -> jobHistoryService = new JobHistoryService(...)       [CoordinatorService.java:529]
            -> constructor registers 3 FRESH entry listeners on the SAME 3 cluster-wide
               IMaps, capturing 3 new registration UUIDs      [JobHistoryService.java:154-162]
  -> isActive = true

Without the fix, every time Case A fires, the outgoing JobHistoryService's three listeners stay registered forever (only removed via close() never being called), so repeated A→B cycles cause unbounded listener/heap accumulation plus multiplied side effects on the next real expiration event. With the fix, Case A always drains exactly the listeners the outgoing instance owns before Case B's fresh instance registers new ones, so listener count stays bounded at 3 regardless of how many master-role switches occur.

Does the normal (single-master, no failover) path change behavior? No — close() is only reachable through clearCoordinatorService(), which only runs on role loss or shutdown. A cluster that never loses its master never calls close(), so steady-state behavior is untouched, matching the PR's own claim.

1.2 Compatibility Impact

Fully compatible.

No Option/config field added, removed, or renamed.
No public API signature changed; close() and getEntryListenerRegistrationIds() are additive.
No serialized/checkpoint format touched — JobHistoryService does not implement Serializable and the new UUID fields are pure in-memory bookkeeping, never persisted or shipped across nodes. JobState/PipelineStateData (the actually-serialized nested classes) are untouched.
The only observable behavior change is that duplicate side effects (extra CleanLogOperation sends, inflated cleanup counters) after a master switch stop occurring — this is the bug being fixed, not a new incompatibility, and is explicitly called out in the PR's "user-facing change" section.

1.3 Performance / Side-Effect Analysis

Net positive: eliminates unbounded listener accumulation (heap leak potential across many failovers) and eliminates duplicated CleanLogOperation RPC fan-out / duplicated cleanup counters on every master switch after the first.
New cost: clearCoordinatorService() (already a synchronized method that blocks up to 20s on executorService.awaitTermination) now also performs up to three sequential, synchronous IMap.removeEntryListener(...) calls. These are Hazelcast distributed listener-registry operations, not local no-ops. See Issue 1 below for the specific risk this introduces.
removeEntryListenerQuietly catches Exception, so a thrown failure (e.g. HazelcastInstanceNotActiveException during concurrent shutdown) is safely absorbed and logged — consistent with the "best effort, must not break master switch" design goal stated in the PR.

1.4 Error Handling and Logging

Issue 1

Location: seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/master/JobHistoryService.java:450-457 (removeEntryListenerQuietly), invoked from CoordinatorService.java:1211-1213 inside the synchronized clearCoordinatorService().
Problem description: imap.removeEntryListener(registrationId) is a synchronous Hazelcast operation. The try/catch(Exception) only protects against the call throwing; if the cluster is unstable (which is precisely the split-brain/rolling-restart scenario this PR targets) and the underlying invocation instead blocks for its full retry/timeout window before either succeeding or throwing, clearCoordinatorService() — a synchronized method — stays blocked for that whole window, not just the already-existing 20s awaitTermination.
Potential risk: In a genuinely unstable cluster, master-role transition could take measurably longer than before this fix, and any other caller contending on the same CoordinatorService monitor is blocked for that extra duration too. This is speculative (I did not reproduce it, and it may be no worse in practice than the pre-existing executorService.awaitTermination(20, SECONDS) and resourceManager.close() calls already inside the same synchronized block, which have the identical shape of risk and are accepted as-is today), so I'm not treating it as a correctness bug — just flagging it as the one place this change adds new blocking network I/O to an already-blocking method.
Best improvement: none required to merge; if the team wants extra safety, a bounded timeout around each removeEntryListener call (or moving close() off the synchronized critical path) would remove the residual risk. Not necessary given the existing method already has comparable blocking calls in the same code path.
Severity: Medium (non-blocking, speculative).
Raised by another reviewer: No.

No other error-handling or logging issues found. Logging is at the correct level (logger.warning for a recoverable, absorbed failure), and includes the map name for context, matching the "exceptions must include sufficient context" convention.

2. Code Quality Assessment

2.1 Coding Standards

License header present and correct on the new test file (JobHistoryServiceListenerCleanupTest.java:1-16).
Every new field (finishedJobStateListenerId, finishedJobMetricsListenerId, finishedJobDAGInfoListenerId) has an explanatory Javadoc describing purpose and lifecycle (JobHistoryService.java:117-134).
Every new method (close(), getEntryListenerRegistrationIds(), removeEntryListenerQuietly()) has a multi-line Javadoc explaining why it exists, not just what it does (JobHistoryService.java:168-183, :197-201, :445-449). The CoordinatorService.java:1205-1210 inline comment at the new call site explains the cluster-wide-IMap rationale, not just "call close()".
No wildcard imports, no System.out.println, no single-line /** ... */ Javadoc — all comment blocks use the standard multi-line form.
Minor style/extensibility observation (not a defect): getEntryListenerRegistrationIds() (JobHistoryService.java:202-207) is a package-private accessor whose Javadoc states it exists "only intended for tests" — a test-only escape hatch on a production class. This is a common, low-risk pattern in this codebase and is clearly documented, so I would not block on it, but it's worth naming as a minor maintainability nit.
Overall: comment-completeness requirement is met — no Medium-severity documentation gap found.

2.2 Test Coverage and Test Stability

New file: JobHistoryServiceListenerCleanupTest.java (158 lines), two @Test methods.

testCloseRemovesFinishedJobEntryListeners (:221-256): Builds a "positive control" first (creates a live JobHistoryService, asserts removeEntryListener returns true for its ids), which is good practice — it proves the exposed UUIDs are real, live registrations before the negative assertions below are given any weight. Then runs three create/close cycles and asserts removeEntryListener returns false after close(), directly exercising repeated master-transition-style churn. No Thread.sleep, no timing assumption, no shared mutable static state, no floating-point comparison — every assertion is a deterministic boolean return from the Hazelcast IMap API.

testClearCoordinatorServiceDeregistersJobHistoryListeners (:264-302): Spins up an isolated HazelcastInstanceImpl (own cluster name via TestUtils.getClusterName(...), confirmed to exist and match the call signature used), fetches the coordinator's JobHistoryService via seaTunnelServer.getCoordinatorService(), and calls the production clearCoordinatorService() directly. I checked whether this could race the 100ms-interval checkNewActiveMaster scheduler (i.e., could getCoordinatorService() be called before initCoordinatorService() has run and jobHistoryService is still null, causing a flaky NPE): it cannot — SeaTunnelServer.getCoordinatorService() (SeaTunnelServer.java:286-319) already contains a bounded retry loop (isCoordinatorActive(), up to 3 retries × 500ms) that blocks until initCoordinatorService() has completed and isActive is set, before returning. The test correctly uses this safe accessor rather than reading the field directly, so there is no timing race here. Resource cleanup uses a finally { coordinatorInstance.shutdown(); } block, so the isolated instance can't leak across test runs even on assertion failure.

Stability rating: Stable. No known flaky-test anti-pattern (no sleep-based waits, no execution-order dependency between the two tests — the second creates its own isolated cluster instance rather than depending on the first — no unclosed resources, no shared static state, no floating-point tolerance issues). The "positive control before negative assertion" structure is a genuinely good pattern that increases confidence the test would actually catch a regression.

2.3 Documentation Updates

None required and none made, correctly matching the PR's own checklist ("No documentation update is required: no user-facing option or behavior contract changes"). I confirmed no files under docs/en or docs/zh are touched, and none needed to be — this is a pure internal engine bug fix with no user-facing option, default, or contract change.

3. Architectural Soundness

3.1 Elegance of the Solution

Classification: Precise fix. It addresses the root cause directly — the missing symmetry between "listener registration" (constructor) and "listener deregistration" (previously nonexistent) — at the one production construction site and the one production teardown site, without introducing new abstractions, config, or broader refactors. This matches the minimal-diff principle: 3 files changed, +253/-6 lines, almost entirely additive.

3.2 Maintainability

Good. The new close() method is self-contained, the call site is a single if (jobHistoryService != null) { jobHistoryService.close(); } with a comment explaining why, and it follows the exact same "capture reference, null out or skip, close outside the reference" pattern already used for resourceManager and eventProcessor a few lines below it in the same method — so a future reader sees a consistent idiom for "how do we tear down a coordinator-owned resource," not a one-off.

3.3 Extensibility

Minor, non-blocking observation: if a fourth cluster-wide listener were added to JobHistoryService in the future, a contributor would need to remember to update three places in parallel (constructor registration, a new field, and close()/getEntryListenerRegistrationIds()), since the three (IMap, UUID, storeName) triples aren't currently modeled as a single collection. Given the current scope is exactly three fixed listeners and the PR's own minimal-diff mandate, I would not ask for this to be generalized now — just noting it as a "recommended, not required" future refactor if the listener set ever grows.

3.4 Historical-Version Compatibility

No impact. JobHistoryService and its listener registration ids are pure runtime/in-memory state, never part of checkpoint/savepoint payloads, never serialized to disk or across the wire, and the fix doesn't touch JobState/PipelineStateData (the classes that are actually Serializable). A cluster upgrading through a rolling restart that includes this fix behaves identically from the outside; the only difference is that stale listeners no longer accumulate across the restart's master transitions. No entry in incompatible-changes.md is warranted, matching the PR's own checklist.

4. Issue Summary

Number	Issue	Location	Severity
1	New synchronous `IMap.removeEntryListener` calls inside the already-blocking `synchronized clearCoordinatorService()` could add latency during exactly the unstable-cluster scenarios this PR targets, if the underlying Hazelcast invocation blocks rather than throws	`JobHistoryService.java:450-457`, called from `CoordinatorService.java:1211-1213`	Medium (speculative, non-blocking)

5. Merge Recommendation

Conclusion: Ready to merge

Blockers — must be fixed
None.
Recommended fixes — non-blocking
- Consider whether close()'s three removeEntryListener calls should be bounded by an explicit timeout, or moved off the synchronized clearCoordinatorService() critical path, to guarantee master-role transitions can't be slowed by a stuck IMap listener-registry call during cluster instability (Issue 1).
- If the finished-job listener set ever grows beyond three, consider modeling the (IMap, UUID, storeName) triples as a single collection to avoid parallel-edit risk across constructor/close()/getEntryListenerRegistrationIds().

Overall assessment: This is a well-scoped, well-tested, root-cause fix for a real listener/heap leak and duplicated-side-effect bug on Zeta master-role transitions. I traced the full call chain in both CoordinatorService.java and JobHistoryService.java (not just the diff) and confirmed the leak mechanism (non-static inner listener classes + missing deregistration), the completeness of the fix (single production construction site, single production teardown site, both covered), and that the normal single-master steady-state path is untouched. The new regression test is genuinely good — deterministic, uses a positive control before its negative assertions, and correctly relies on SeaTunnelServer's existing retry-based accessor rather than racing the master-election scheduler. The one finding (Issue 1) is a reasoned, hedged latency concern under fault conditions, not a demonstrated defect, and mirrors risk already accepted elsewhere in the same method — it should not block merge. CI (Build) was still queued/pending on both the apache-side check and the DanielLeens/seatunnel fork run at review time; this should go green before merge, but there is nothing in the code itself that stands in the way.

DanielLeens

What Problem Does This PR Solve?

One-sentence summary: JobHistoryService registers three cluster-wide Hazelcast EntryExpiredListeners in its constructor but never deregistered them, so every active-master transition leaked a full set of listeners; this PR captures the three registration ids and removes them from CoordinatorService.clearCoordinatorService().

Concretely what accumulates. CoordinatorService.initCoordinatorService() (CoordinatorService.java:515) constructs a brand-new JobHistoryService at CoordinatorService.java:529 every time this node becomes the active master. That constructor registers three entry listeners on three cluster-wide IMaps (JobHistoryService.java:155/158/161):

IMap	Listener
`IMAP_FINISHED_JOB_STATE`	`FinishedJobExpiredListener`
`IMAP_FINISHED_JOB_METRICS`	`FinishedJobExpiredListener`
`IMAP_FINISHED_JOB_VERTEX_INFO`	`JobInfoExpiredListener`

Before this PR the return value of addEntryListener(...) was discarded, and no code path ever called removeEntryListener. Because the finished-job IMaps outlive the coordinator lifecycle, and because both listener classes are non-static inner classes (private class at JobHistoryService.java:481 and :494), each abandoned registration keeps its enclosing JobHistoryService — and transitively nodeEngine, the IMaps, runningJobMasterMap, and the pending-job id map — reachable from the IMap listener registry. So each master transition leaks one object graph plus three registrations that fire forever.

The functional damage is worse than the heap cost. JobInfoExpiredListener.entryExpired() (JobHistoryService.java:501-538) is the only producer of CleanLogOperation in the whole codebase (verified by a repo-wide grep: the sole non-test send site is JobHistoryService.java:526). It increments a cleanup counter and then, for every address in the DAG's history execution plan plus the DAG master address, synchronously sends a CleanLogOperation and blocks on .join(). With K leaked registrations, a single real expiration event produces K duplicated counter increments and K duplicated blocking RPC fan-outs — executed on Hazelcast event threads, on nodes that are no longer the master.

Important correction to the framing in the linked issue. The listeners are registered per JobHistoryService instance, i.e. per master activation — not per finished job. Nothing in the engine registers a listener per job (confirmed below). So the leak is bounded by the number of active→inactive→active master transitions (failover, split-brain healing, rolling restart), not by job throughput. A cluster that submits a million jobs and never loses its master leaks nothing. This distinction matters for assessing severity and for writing a meaningful reproduction.

Simple example. Submitting N jobs changes nothing — the correct experiment is N master transitions:

Cluster of 3 nodes. Force K master handovers (kill/restart the master K times).
Registered listeners on IMAP_FINISHED_JOB_VERTEX_INFO:

  before fix:  1 + K       (each departed master's registration survives)
  after  fix:  1           (bounded, regardless of K)

Then let one finished job's DAG entry expire:
  before fix:  1 + K duplicate CleanLogOperation fan-outs, 1 + K counter increments
  after  fix:  1 fan-out, 1 increment

Fix approach. Store the three UUIDs returned by addEntryListener in new final fields (JobHistoryService.java:122/128/134), add close() (JobHistoryService.java:184) that deregisters exactly those three via a fail-soft removeEntryListenerQuietly helper (JobHistoryService.java:450), and invoke it from CoordinatorService.clearCoordinatorService() at CoordinatorService.java:1212. The jobHistoryService field is deliberately not nulled, so read paths (REST/metrics handlers at CoordinatorService.java:2090) keep working after the role switch.

1. Code Change Review

1.1 Core Logic Analysis

I read both production files in full at the PR head, not just the diff hunks, and independently verified the four claims the fix depends on.

(a) Are there exactly three listeners, and are all three covered? Yes. A repo-wide sweep for addEntryListener / addLocalEntryListener / addPartitionLostListener / addMembershipListener / addLifecycleListener / addDistributedObjectListener over all non-test Java sources returns only three engine hits inside JobHistoryService (:155, :158, :161), all three of which are now captured and all three of which are removed by close() (:185, :187, :191). The issue history's "3, not 1" count is correct and the fix is complete with respect to this class.

(b) Is there a fourth leak site elsewhere in the coordinator/job lifecycle? No. The same sweep found exactly two other engine-side addEntryListener call sites:

HazelcastCheckpointOverviewStateStore.java:134-135 — captures into a volatile UUID overviewListenerId, removes it under a null-guard at :165-167, and nulls the field afterwards.
HazelcastMetricsSnapshotStateStore.java:180 — captures into volatile UUID metricsListenerId, removes it under a null-guard at :253-255, nulls the field.

Both already implement the register/deregister symmetry correctly, so neither is an uncovered leak. Worth noting for the author: these two are the pre-existing in-repo idiom for this exact problem, and they are slightly more defensive than the new code (see Issue 4).

(c) Is there more than one production construction site? No. new JobHistoryService( appears once in non-test code, at CoordinatorService.java:529. So a single teardown call site is sufficient.

(d) Does the CAS guard actually let the fix run more than once? This was the highest-risk thing to check, because clearCoordinatorService() early-returns on !coordinatorServiceCleared.compareAndSet(false, true) (CoordinatorService.java:1174). If that flag were never reset, close() would run on the first master transition only and the fix would silently be a one-shot. It is reset: initCoordinatorService() sets coordinatorServiceCleared.set(false) as its very first statement (CoordinatorService.java:516). So every activation re-arms the guard and every deactivation performs exactly one cleanup. The fix is correct across unbounded transition cycles.

Runtime path.

masterActiveListener  (single-thread scheduler, 100ms fixed rate, started in ctor :251)
  -> checkNewActiveMaster()                                    [CoordinatorService.java:1133]

  Case B  become active master   (!isActive && isMasterNode())
    -> initCoordinatorService()                                [:515]
         :516  coordinatorServiceCleared.set(false)   <-- re-arms the cleanup guard
         :529  jobHistoryService = new JobHistoryService(...)
                 -> addEntryListener x3, ids captured          [JobHistoryService.java:155/158/161]
    -> isActive = true ; startPendingJobScheduleThread() ; startRealtimeMetricsService()

  Case A  leave active master    (isActive && !isMasterNode())
    -> isActive = false ; stopRealtimeMetricsService()
    -> clearCoordinatorService()   [synchronized]              [:1172]
         :1174 CAS(false->true) guard, returns early if already cleared
         :1178 interrupt running JobMasters ; drain pendingJobQueue if WAIT
         :1190 executorService.shutdownNow()
         :1194 awaitTermination(20s)
         :1212 jobHistoryService.close()          <-- THE FIX
                 -> removeEntryListenerQuietly(stateImap,   id, ...)   [JobHistoryService.java:185]
                 -> removeEntryListenerQuietly(metricsImap, id, ...)   [:187]
                 -> removeEntryListenerQuietly(dagInfoImap, id, ...)   [:191]
                    each: imap.removeEntryListener(uuid), Exception caught + logged
         :1215 resourceManager.close() ; :1222 eventProcessor.close()
       (jobHistoryService field intentionally left non-null -> reads still served)

  Case C  init threw               [catch at :1152]
    -> isActive = false ; clearCoordinatorService()            [:1157]
       guard is false (set at :516), so close() DOES run -> partial init is cleaned up

  Case D  node shutdown
    -> shutdown()                                              [:1966]
         :1969 masterActiveListener.shutdown()   (graceful, not shutdownNow)
         :1977 clearCoordinatorService()  -> same close() path

Does the normal job lifecycle reach the new code? No — and that is correct. Submit → run → finish → JobMaster.storeFinishedJobState() never touches clearCoordinatorService(). The new removal is reached only on master-role loss (Case A), init failure (Case C), and node shutdown (Case D). The PR title is accurate; readers coming from the issue title should not expect per-job behaviour.

Abnormal paths. Job cancellation does not reach it (correct — cancelling a job must not tear down the master's history listeners). Coordinator restart and master failover both reach it via Case A/Case D. Init-failure retry loops reach it via Case C, and because the guard is re-armed at :516 on each retry, each partial init is cleaned up rather than accumulating — this path is actually improved by the fix.

1.2 Compatibility Impact

Explicit conclusion: fully backward compatible. No blocker.

No public API change. close() and getEntryListenerRegistrationIds() are purely additive; the latter is package-private. No existing method signature, return type, or visibility changed.
No configuration change. No Option added, removed, renamed, or re-defaulted. Nothing under docs/ needs to change.
No serialization or state-format change. The three new fields are UUID in-memory bookkeeping. JobHistoryService is not Serializable; the nested classes that are serialized (JobState at :461, PipelineStateData at :475) are untouched, and their serialVersionUIDs are unchanged. No checkpoint, savepoint, or IMap payload format is affected, so rolling upgrade and state restore across versions are unaffected.
No SPI/plugin contract touched. Change is confined to seatunnel-engine-server internals.
Behavioural delta is limited to: duplicate CleanLogOperation fan-out and inflated cleanup counters after a master switch stop occurring. That is the bug being removed. The one behavioural nuance that is not purely a bug fix is covered in Issue 3.

1.3 Performance / Side-Effect Analysis

Net positive: removes unbounded listener accumulation and removes duplicated blocking RPC fan-out on Hazelcast event threads.

Missing a legitimate late-arriving event. This is the side-effect question worth taking seriously, and the PR description does not mention it. Because addEntryListener(listener, true) registers a cluster-wide listener (not addLocalEntryListener), exactly one live registration serves the whole cluster. Post-fix there is a window — old master finishes close(), new master has not yet reached CoordinatorService.java:529 — in which zero listeners are registered on the finished-job IMaps. Since JobInfoExpiredListener is the only CleanLogOperation producer and there is no periodic reconciliation sweep, an expiry event landing in that window means those log files on member nodes are never cleaned, silently and permanently. Pre-fix, the stale listener would still have handled it (duplicated, and from a non-master node). Two things bound the risk: close() executes after the up-to-20s awaitTermination at :1194, so in practice the incoming master usually registers first; and Hazelcast TTL expiry is lazy, so the event must coincide with a background expiration pass. Rated Low — see Issue 3.

Thread-safety of removal versus an in-flight event. removeEntryListener is called from the masterActiveListener thread (or the shutdown thread) while an entryExpired callback may be executing concurrently on a Hazelcast event thread. Hazelcast does not quiesce in-flight callbacks on deregistration, so a callback can complete after close() returns. That is safe here: incrementFinishedJobCleanupTotal uses a ConcurrentHashMap of AtomicLong (:440-442), and the CleanLogOperation fan-out is already wrapped in try/catch (Exception) at :535 which absorbs a HazelcastInstanceNotActiveException from a concurrently shutting-down node. No new race is introduced.

New blocking work in a synchronized method. close() adds up to three synchronous distributed removeEntryListener invocations inside the synchronized clearCoordinatorService(). The prior review round flagged this as a latency concern. I independently assessed it and agree it is not worth acting on: the same method already blocks on executorService.awaitTermination(20, TimeUnit.SECONDS) at :1194 and on resourceManager.close() at :1218, both of which have the identical shape and larger magnitude. Adding three listener-registry calls does not change the risk class. Noted for completeness, not carried as a numbered issue.

1.4 Error Handling and Logging

Correct and appropriately defensive.

removeEntryListenerQuietly (:450-457) catches Exception and logs at warning with the store name for context, so a failure during a concurrent Hazelcast shutdown cannot abort the master-switch flow or mask the rest of the cleanup (resourceManager.close() / eventProcessor.close() still run). This matches the stated design intent and the project's "exceptions must carry context" convention.
Already-removed / already-cleared is handled gracefully: IMap.removeEntryListener on an unknown id returns false rather than throwing, and the coordinatorServiceCleared CAS at :1174 means close() is not re-invoked within one cleanup cycle anyway. The if (jobHistoryService != null) guard at :1211 covers the first-ever clear before any init.
No sensitive data is logged; log level (warning for a recoverable, absorbed failure) is correct.

One gap: the catch swallows silently enough that a persistently failing deregistration would leak exactly the way the PR is fixing, visible only as a WARN line. That is the right trade-off, but see Issue 4 for the related constructor case, which has no recovery path at all.

Issue 1

Location: seatunnel-engine/seatunnel-engine-server/src/test/java/org/apache/seatunnel/engine/server/master/JobHistoryServiceListenerCleanupTest.java:104-119
Problem description: testClearCoordinatorServiceDeregistersJobHistoryListeners calls seaTunnelServer.getCoordinatorService() immediately after SeaTunnelServerStarter.createHazelcastInstance(...) returns, with no await() wrapper. SeaTunnelServer.getCoordinatorService() (SeaTunnelServer.java:286-317) is not a blocking accessor in the general case: if isMasterNode() is false it throws SeaTunnelEngineException immediately with no retry at all, and its internal retry loop is only 3 attempts x 500ms before throwing SeaTunnelEngineRetryableException when the coordinator has not finished initCoordinatorService(). The established pattern for exactly this situation elsewhere in the module is an Awaitility wrapper — CoordinatorServiceTest.java:117-123 does await().atMost(60, TimeUnit.SECONDS).untilAsserted(() -> { assertTrue(server1.isMasterNode()); assertTrue(server1.getCoordinatorService().isCoordinatorActive()); }). The new test does not follow it.
Potential risk: Intermittent CI failure on a loaded runner if master election or coordinator init has not completed within the 1.5s internal budget, surfacing as a confusing SeaTunnelEngineRetryableException rather than an assertion failure. Given this project's existing burden of flaky engine-server tests, adding a timing-sensitive test is a real cost.
Best improvement: Wrap the acquisition in the same Awaitility idiom already used in CoordinatorServiceTest, asserting isMasterNode() and isCoordinatorActive() before reading getJobHistoryService().
Severity: Medium
Raised by another reviewer: No. (The prior review round explicitly concluded the opposite — that the internal retry loop makes this race-free. That conclusion overlooked the immediate no-retry throw on the !isMasterNode() branch and the 1.5s ceiling on the other.)

Issue 2

Location: JobHistoryServiceListenerCleanupTest.java:105-108
Problem description: The isolated instance is created via SeaTunnelServerStarter.createHazelcastInstance(String clusterName), which internally calls ConfigProvider.locateAndGetSeaTunnelConfig() (SeaTunnelServerStarter.java:39-43) and only overrides the cluster name. It therefore inherits default network settings. This differs from both sibling patterns: AbstractSeaTunnelServerTest.before() isolates via TestUtils.getAvailablePort(100) and an explicit inline Hazelcast config, and CoordinatorServiceTest.createHazelcastInstanceWithJoinPortTryCount() (:509-517) deliberately raises hazelcast.tcp.join.port.try.count to 100 for the same reason. Note this test class also inherits AbstractSeaTunnelServerTest, so it stands up a second node in the same JVM alongside the base-class node.
Potential risk: Port contention against the base-class instance and against other engine-server test classes running in the same CI job, producing slow joins or startup failures unrelated to the code under test.
Best improvement: Build the isolated instance from an explicit SeaTunnelConfig with an allocated free port and a raised join-port-try-count, mirroring CoordinatorServiceTest.createHazelcastInstanceWithJoinPortTryCount.
Severity: Medium
Raised by another reviewer: No.

Issue 3

Location: CoordinatorService.java:1211-1213 (behavioural consequence of the new close() call), interacting with JobHistoryService.java:501-538
Problem description: Removing the cluster-wide expiration listeners on master-role loss creates a window with no registered listener on the finished-job IMaps until the incoming master constructs its JobHistoryService. JobInfoExpiredListener is the sole producer of CleanLogOperation (verified repo-wide), and there is no periodic reconciliation. An expiration event in that window is lost permanently — the IMap entry is gone, so nothing retries.
Potential risk: Orphaned job log files on member nodes for jobs whose DAG-info entry expires exactly during a master handover, with no error surfaced. Low probability: close() runs after the up-to-20s awaitTermination, so the new master has usually registered already, and Hazelcast TTL expiry is lazy rather than instantaneous.
Best improvement: No code change required for merge. Document the window in the close() Javadoc so the next maintainer does not rediscover it; if log-cleanup completeness ever becomes a hard requirement, a reconciliation sweep at initCoordinatorService() would be the correct place, not this method.
Severity: Low
Raised by another reviewer: No.

Issue 4

Location: JobHistoryService.java:154-163 (constructor registration block); contrast HazelcastMetricsSnapshotStateStore.java:180, 253-255
Problem description: The three registration ids are final fields assigned inline in the constructor. If the second or third addEntryListener throws, the earlier registrations are already live but their ids die with the failed object, making them permanently unremovable — the exact leak this PR fixes, in its one uncovered corner. This matters more than it looks because initCoordinatorService() failure is caught and retried every 100ms by checkNewActiveMaster (:1152-1160), so a persistent failure mode would leak on every retry tick. The two pre-existing state stores avoid this by using volatile UUID fields with null-guarded removal rather than final inline assignment.
Potential risk: Unbounded unremovable listener accumulation under a repeating partial-construction failure. Narrow, and pre-existing rather than introduced by this PR.
Best improvement: Make registration all-or-nothing — assign to non-final fields and wrap the three registrations so a failure deregisters whatever already succeeded before rethrowing; or align with the volatile UUID + null-guard idiom the two sibling state stores already use.
Severity: Low
Raised by another reviewer: No.

Issue 5

Location: JobHistoryServiceListenerCleanupTest.java:60-94
Problem description: Neither test asserts that closing one JobHistoryService leaves a different, still-live instance's registrations intact, and neither asserts a total listener count on the maps. The tests prove "the three ids I closed are gone", which would also pass if close() over-removed and killed a concurrent instance's listeners. Given that the central risk of this change is "deregistering at the wrong time or deregistering too much" — and given the shutdown-versus-init interleaving noted in Issue 6 — cross-instance isolation is the highest-value assertion currently missing.
Potential risk: A future refactor that broadens close() (for example to a map-wide clear) would not be caught by this suite.
Best improvement: In testCloseRemovesFinishedJobEntryListeners, hold a second live JobHistoryService across the close cycles and assert its three ids still return true from removeEntryListener at the end.
Severity: Low
Raised by another reviewer: No.

Issue 6

Location: CoordinatorService.java:515 (initCoordinatorService, not synchronized) versus :1172 (clearCoordinatorService, synchronized), reached via :1966-1977
Problem description: shutdown() calls masterActiveListener.shutdown() — the graceful variant, which returns immediately and lets an in-flight task finish — and then calls clearCoordinatorService() from the shutdown thread. Because initCoordinatorService() is not synchronized, an in-flight checkNewActiveMaster tick can be mid-init and assign a fresh jobHistoryService at :529, registering three new listeners, after the shutdown thread already ran close() on the previous instance. Those three registrations are then never closed.
Potential risk: Low in practice: the node's Hazelcast instance terminates immediately afterwards and the cluster discards registrations owned by a departed member, so this is not a durable cluster-wide leak. This is a pre-existing structural asymmetry that the PR inherits rather than introduces, but it is now load-bearing for the fix's correctness claim.
Best improvement: Out of scope for this PR. If addressed later, making initCoordinatorService() synchronized on the same monitor would close the window; that should be its own change with its own deadlock analysis.
Severity: Low
Raised by another reviewer: No.

2. Code Quality Assessment

2.1 Coding Standards

ASF license header present and correct on the new test file (JobHistoryServiceListenerCleanupTest.java:1-16).
All three new fields carry Javadoc explaining purpose and lifecycle, not just restating the type (JobHistoryService.java:117-134). All three new methods carry multi-line Javadoc explaining why they exist (:168-183, :197-201, :445-449). The inline comment at the new call site (CoordinatorService.java:1205-1210) explains the cluster-wide-IMap rationale rather than narrating the call. This satisfies the project's comment-completeness requirement — no documentation-gap finding.
No wildcard imports; new imports (java.util.Arrays, java.util.UUID) are explicit and used. No System.out.println. No single-line /** ... */ inline Javadoc. Formatting is consistent with AOSP/Spotless output.
getEntryListenerRegistrationIds() (:202) is a package-private test-only accessor on a production class. Its Javadoc says so explicitly, and the alternative (reflection in the test) would be worse. Acceptable, minor nit only.
The test class extends the raw AbstractSeaTunnelServerTest rather than the parameterized form. This matches the majority convention in the module (20 raw versus 9 parameterized), so it is consistent, not a defect.

2.2 Test Coverage and Test Stability

Does a test actually assert listener state across the lifecycle, rather than just "no exception thrown"? Yes. The assertions rest on IMap.removeEntryListener(UUID) returning true for a live registration and false for an absent one, which is a genuine observation of the listener registry rather than a smoke check. testCloseRemovesFinishedJobEntryListeners (:60-94) is well constructed: it establishes a positive control first (asserting true for an un-closed service's three ids, proving the exposed ids are real registrations) before relying on the negative assertions, then runs three create/close cycles to emulate repeated master transitions. testClearCoordinatorServiceDeregistersJobHistoryListeners (:104-139) exercises the real production teardown entry point rather than calling close() directly, which is the right level to test at.

Coverage gaps. No assertion on absolute listener count, and no cross-instance isolation assertion (Issue 5). Neither test covers the shutdown path (Case D) or the init-failure path (Case C), though both funnel through the same clearCoordinatorService() and are lower value.

Flaky-pattern check. No Thread.sleep, no wall-clock tolerance, no floating-point comparison, no shared mutable static state, no inter-test ordering dependency (the second test builds its own cluster), and finally { coordinatorInstance.shutdown(); } guarantees teardown on assertion failure. Those fundamentals are good. However the suite is not timing-clean overall: the unguarded getCoordinatorService() acquisition (Issue 1) and the default-network isolated instance (Issue 2) are both genuine CI-stability risks, and both have an established safer pattern already in the same module. Stability rating: acceptable but should be hardened before merge; I do not agree with the prior round's "no timing race here" assessment.

One benign behaviour worth noting so it is not mistaken for a bug later: after clearCoordinatorService() in the second test, the 100ms masterActiveListener immediately re-initializes the coordinator on that single-node cluster, creating a fresh JobHistoryService with new ids. The assertions target the ids captured beforehand, so they remain valid. The test comment already documents this correctly.

2.3 Documentation Updates

None required and none made — correct. This is an internal engine bug fix with no user-facing option, default, contract, or behaviour that a user configures. No file under docs/en or docs/zh needs to change, and no entry in docs/en/introduction/concepts/incompatible-changes.md is warranted.

3. Architectural Soundness

3.1 Elegance of the Solution

A precise, root-cause fix rather than a symptom patch. It restores the missing symmetry between constructor registration and teardown deregistration, at the one production construction site and the one production teardown site, with no new abstraction, no new configuration, and no collateral refactor. 3 files, +253/-6, almost entirely additive and test code. This is the right shape for a change to a financial-grade long-running coordinator.

3.2 Maintainability

Good. close() is self-contained, the call site is a two-line guarded invocation with an explanatory comment, and it sits directly alongside the existing resourceManager / eventProcessor teardown at :1215-1226, so it reads as the established idiom for "coordinator-owned resource teardown". The one inconsistency is that those neighbours capture-and-null their fields while close() deliberately does not — that choice is correct here (read paths must survive) and is documented in the Javadoc, so it will not confuse a future reader.

3.3 Extensibility

The three (IMap, UUID, storeName) triples are modelled as three parallel fields rather than one collection, so adding a fourth finished-job listener would require coordinated edits in four places (field, constructor, close(), getEntryListenerRegistrationIds()). With exactly three fixed listeners and a minimal-diff mandate, generalizing now would be over-engineering. Worth revisiting only if the listener set grows.

3.4 Historical-Version Compatibility

No impact. Registration ids are ephemeral in-memory state, never persisted, never serialized, never shipped across the wire, and never part of a checkpoint or savepoint payload. A cluster performing a rolling upgrade through this fix behaves identically from the outside; the only difference is that stale listeners no longer survive the restart's master transitions. Downgrade is equally safe — reverting simply restores the old leak. No migration guidance needed.

4. Issue Summary

#	Issue	Location	Severity
1	`getCoordinatorService()` acquired without an Awaitility guard; the accessor throws immediately when not yet master and retries only 1.5s otherwise, diverging from the established `CoordinatorServiceTest` pattern	`JobHistoryServiceListenerCleanupTest.java:104-119`	Medium
2	Isolated Hazelcast instance built from default network config (default ports, default join-port-try-count), unlike both sibling test patterns in the same module	`JobHistoryServiceListenerCleanupTest.java:105-108`	Medium
3	Window with zero cluster-wide expiration listeners during master handover can permanently drop a `CleanLogOperation` cleanup; no reconciliation sweep exists	`CoordinatorService.java:1211-1213` with `JobHistoryService.java:501-538`	Low
4	Final inline id assignment means a partial constructor failure leaks already-registered listeners unremovably; retried every 100ms on persistent init failure	`JobHistoryService.java:154-163`	Low
5	No cross-instance isolation assertion and no listener-count assertion, so an over-removing `close()` would not be caught	`JobHistoryServiceListenerCleanupTest.java:60-94`	Low
6	Non-synchronized `initCoordinatorService()` can register fresh listeners after the shutdown thread already ran `close()`	`CoordinatorService.java:515` versus `:1172`	Low

5. Merge Recommendation

Conclusion: Source logic approved — no correctness blockers; merge gated on CI completing green, with Issues 1 and 2 strongly recommended first.

Blockers — must be fixed
- None at the source-correctness level. The one hard external gate is CI: at review time the fork run for head 7b9f8b43186a is still queued (created 2026-08-14T17:11:37Z, no jobs started after ~24h) and the apache-side Build check remains pending. Per project policy an unverified CI is a blocking condition for merge, independent of code quality.
Recommended fixes — non-blocking
- Issue 1 (Medium): wrap the coordinator-service acquisition in the await().atMost(...).untilAsserted(...) idiom already used at CoordinatorServiceTest.java:117-123. Strongly recommended — this is the single most likely source of future flakiness in the new suite, and the fix is three lines.
- Issue 2 (Medium): build the isolated instance from an explicit config with an allocated port and raised join-port-try-count, mirroring CoordinatorServiceTest.createHazelcastInstanceWithJoinPortTryCount.
- Issue 3 (Low): document the no-listener handover window in the close() Javadoc.
- Issue 4 (Low): make the three registrations all-or-nothing, or adopt the volatile UUID + null-guard idiom already used by the two sibling state stores.
- Issue 5 (Low): add a cross-instance isolation assertion so an over-removing close() cannot pass.
- Issue 6 (Low): out of scope; note only.

Overall assessment. The production change is correct, minimal, and well-reasoned, and it fixes a genuine defect rather than a theoretical one. I independently verified the four load-bearing claims instead of accepting the description: all three listeners are captured and removed; the listener classes really are non-static inner classes so the stale-reference argument holds; there is exactly one production construction site and one teardown site; and — the item most likely to have silently defeated the fix — coordinatorServiceCleared is re-armed at initCoordinatorService() line 516, so the cleanup runs on every transition rather than only the first. A repo-wide sweep for every listener-registration API confirmed there is no fourth uncovered leak site: the only two other engine addEntryListener calls, in HazelcastCheckpointOverviewStateStore and HazelcastMetricsSnapshotStateStore, already capture and remove their ids correctly. Compatibility is clean on every axis that matters for a long-running cluster — no API, no option, no serialized state, no checkpoint format, safe in both upgrade and downgrade directions.

Two corrections to how this change has been characterized. First, the leak is per master-role transition, not per finished job — nothing registers a listener per job — so severity scales with failover frequency rather than job throughput, and any reproduction must force master handovers. Second, I do not agree with the previous round's conclusion that the new test has no timing race: getCoordinatorService() throws immediately when the node is not yet master and retries only 1.5s otherwise, and the module's own established pattern wraps exactly this call in Awaitility. Issues 1 and 2 are test-stability defects being newly introduced into a codebase that already carries a meaningful flaky-test burden, and I would rather see them fixed in this PR than triaged later as mystery CI noise. Neither affects production correctness, so neither blocks the code itself — but with CI still queued and unverified, there is no reason to merge before they are addressed.

DanielLeens · 2026-08-17T11:22:10Z

What Problem Does This PR Solve?

JobHistoryService registers three cluster-wide Hazelcast EntryExpiredListeners in its constructor but never deregistered them, so every active-master transition leaked a full set of listeners. This PR captures the three registration ids and removes them from CoordinatorService.clearCoordinatorService().

Concretely what accumulates: CoordinatorService.initCoordinatorService() constructs a brand-new JobHistoryService every time this node becomes the active master. That constructor registers three entry listeners on three cluster-wide IMaps (finished-job state, finished-job metrics, finished-job vertex info). Before this PR the return value of addEntryListener(...) was discarded, and no code path ever called removeEntryListener. Because both listener classes are non-static inner classes, each abandoned registration keeps its enclosing JobHistoryService — and transitively the node engine, the IMaps, and the running/pending job maps — reachable from the IMap listener registry, and one of the listeners is the sole producer of a cleanup RPC in the codebase, so leaked registrations also duplicate that RPC fan-out on every real expiration event.

Fix approach: store the three UUIDs returned by addEntryListener in new final fields, add a close() that deregisters exactly those three via a fail-soft helper, and invoke it from CoordinatorService.clearCoordinatorService(). jobHistoryService is deliberately not nulled, so read paths keep working after the role switch.

What Changed Since My Last Review

Nothing in the code. I re-fetched upstream/dev, diffed the current head against the merge-base, and confirmed the production diff is byte-for-byte identical to what I reviewed on 2026-08-16 (the same 3 files changed). The only commit added since my last comment is an empty "retrigger build" commit — git show --stat on it lists no files. None of my six previously-raised issues (2 Medium, 4 Low) have been addressed in source. I re-verified this directly against the current head files rather than trusting the diff being empty as a shortcut.

1. Code Change Review

1.1 Core Logic Analysis

I re-read JobHistoryService.java and CoordinatorService.java in full at the current head (not the diff) and re-independently re-verified the four load-bearing claims from my prior round, because a re-review that just restates prior conclusions without re-checking is worthless:

All three listeners captured and removed — confirmed line-for-line against current head.
Single production construction site — remains the only new JobHistoryService( in non-test code.
CAS guard re-arms correctly — the guard flag is reset as the first statement of initCoordinatorService(), and the CAS gate in clearCoordinatorService() is unchanged — every active/inactive cycle still performs exactly one cleanup.
close() call site unchanged — still inside the synchronized clearCoordinatorService(), still reached from role-loss, init-failure-retry, and shutdown.

Runtime path (unchanged from prior round, re-verified against current line numbers):

Case A  leave active master
  -> clearCoordinatorService()  [synchronized]
       CAS(false->true) guard
       awaitTermination(20s)
       jobHistoryService.close()          <-- THE FIX
               -> removeEntryListenerQuietly x3
       resourceManager.close() ; eventProcessor.close()

Case B  become active master
  -> initCoordinatorService()
       coordinatorServiceCleared.set(false)   <-- re-arms
       jobHistoryService = new JobHistoryService(...)  -> 3 fresh ids

Case C  init throws  -> clearCoordinatorService(), guard is false, close() runs
Case D  shutdown()   -> clearCoordinatorService() -> same close() path

Normal job lifecycle does not reach this code, and that is correct. Submit -> run -> finish never touches clearCoordinatorService(); the removal fires only on master-role loss, init failure, and shutdown — not on job completion despite the PR title's phrasing. This is the correct scope: cancelling a job must not tear down the master's history listeners, and I confirmed no other code path calls close().

1.2 Compatibility Impact

Fully backward compatible — unchanged conclusion, re-verified. No Option added/removed/renamed. close() and the new id-accessor are purely additive, the latter package-private. JobHistoryService is not Serializable; the fields that are actually serialized are untouched. No checkpoint/savepoint/IMap payload format is affected. Rolling upgrade and downgrade are both safe — a downgrade simply restores the pre-fix leak, no data-format break.

1.3 Performance / Side-Effect Analysis

This is the point of the fix, so I re-verified rather than restated:

Net positive, confirmed: listener count is bounded at 3 regardless of failover count; duplicate cleanup-RPC fan-out and duplicate cleanup-counter increments stop after the first master switch post-fix.
New blocking work inside synchronized clearCoordinatorService(): unchanged from prior round. Three additional synchronous IMap.removeEntryListener calls now sit inside the same synchronized method that already blocks on a 20s awaitTermination and on resourceManager.close(). Same risk class as those neighbors, not a new class of risk. Not a blocker.
Handover window with zero registered listeners (raised in my prior round, still not addressed in code or Javadoc): because addEntryListener(listener, true) is cluster-wide, there is a window between the outgoing master's close() and the incoming master's initCoordinatorService() where no listener is registered on the finished-job IMaps at all. An expiry landing in that window is dropped permanently — no reconciliation sweep exists to recover it. I re-checked whether anything since has added a periodic reconciliation pass — it has not. Risk remains Low (bounded by the up-to-20s awaitTermination before close() runs, and Hazelcast TTL expiry being lazy), and I still do not consider it a merge blocker, but it remains completely undocumented in the close() Javadoc, which a future maintainer reading only that Javadoc would not learn about.

1.4 Error Handling and Logging

Unchanged and still correct: the removal helper catches Exception, logs at warning with the map name, never propagates. The null-guard on jobHistoryService covers the first-ever clear before any init. No sensitive data logged.

No new issues found in this pass beyond the two carried over from the prior round:

Issue 1 (carried over, unresolved)

Location: JobHistoryService constructor; contrast the safer pattern used by a sibling state-store class in this module.
Problem: The three ids are final fields assigned inline. If the second or third addEntryListener call throws, the already-registered listener(s) become permanently unremovable — the exact leak this PR fixes, in one uncovered corner. Because initCoordinatorService() failure is retried every ~100ms by the active-master check, a persistent partial-construction failure would leak on every tick.
Risk: Narrow and pre-existing in shape (this exact hole existed before the PR too, just without any cleanup path at all), not introduced by this PR, but not closed by it either.
Severity: Low. Not required for merge.

Issue 2 (carried over, unresolved)

Location: CoordinatorService.clearCoordinatorService() interacting with JobHistoryService's registration/close methods.
Problem: Zero-listener handover window described in 1.3, undocumented in the close() Javadoc.
Risk: Low probability, orphaned log files on member nodes if an expiry lands exactly in the window.
Best improvement: No code change required for merge; add a sentence to the close() Javadoc so the window is a documented, intentional trade-off rather than something the next maintainer has to rediscover from first principles.
Severity: Low.

2. Code Quality Assessment

2.1 Coding Standards

Unchanged and re-verified: ASF license header present and correct on the new test file. All three new fields and all three new methods carry multi-line Javadoc explaining purpose, not just type. No wildcard imports, no System.out.println, no single-line Javadoc.

2.2 Test Coverage and Test Stability — does the test actually detect a leak, and is it CI-safe?

Yes, it genuinely detects a leak, not just "no exception thrown." I re-read the test file at the current head end-to-end. Both tests assert on IMap.removeEntryListener(UUID)'s boolean return, which is a real observation of the listener registry: true means the registration is live, false means it is gone. One test establishes a positive control first — proving the exposed ids are real live registrations — before running three create/close cycles and asserting false after each, which directly emulates repeated master-transition churn. The other test exercises the real production entry point (clearCoordinatorService()) rather than calling close() directly, which is the correct level to test at.

Both CI-stability defects I flagged in my prior round are still present, verbatim, in the current head — I re-read the file line-by-line to confirm, not just trusted my earlier finding:

Issue 3 (Medium, carried over, unresolved). The test acquiring the coordinator service has no await() wrapper around it. I re-read the accessor directly at the current head: if the node isn't master yet, it throws immediately with zero retries; if it is master, the internal retry loop is bounded to 1.5s before throwing. The module's own sibling test file establishes the answer to exactly this problem — an await().atMost(60, TimeUnit.SECONDS).untilAsserted(...) idiom used repeatedly throughout that file. The new test does not follow this established idiom. On a loaded CI runner, single-node master election plus initCoordinatorService() completing inside 1.5s is not guaranteed, and this project already has a documented flaky-engine-server-test burden.
Issue 4 (Medium, carried over, unresolved). The isolated Hazelcast instance in the new test is built with only the cluster name overridden, inheriting default network/port settings. I re-confirmed the sibling test file has a private helper used at 8+ call sites specifically to avoid port-join contention between concurrently-running node instances in the same CI job. This test class also stands up a second node alongside the base-class node without that same protection.

Both are exactly as I described them in my prior round; no attempt has been made to address either, and their fix cost is genuinely small (three lines each, using patterns that already exist verbatim elsewhere in the same test package).

Coverage gaps also carried forward, unaddressed: no absolute listener-count assertion, no cross-instance isolation assertion (an over-removing close() that killed a concurrent instance's listeners would still pass this suite), no coverage of the shutdown path or init-failure path.

Stability rating: acceptable but should be hardened before merge — unchanged from my prior conclusion. The fundamentals (no Thread.sleep, no floating-point tolerance, no inter-test ordering dependency, finally-guarded teardown) are genuinely good. But Issues 3 and 4 are real, established-pattern-diverging test-stability risks being introduced into a module with a known flaky-test history, and they remain unfixed in this round.

2.3 Documentation Updates

None required, none made — correct, unchanged.

3. Architectural Soundness

3.1 Elegance of the Solution

Unchanged: a precise root-cause fix restoring the missing register/deregister symmetry at the one construction site and one teardown site, with no new abstraction or config. Almost entirely additive.

3.2 Maintainability

Unchanged: close() sits alongside the existing resource-teardown idiom in clearCoordinatorService() and reads as the established pattern for coordinator-owned-resource teardown.

3.3 Extensibility

Unchanged, non-blocking: the three (IMap, UUID, storeName) triples are three parallel fields rather than one collection. Fine at the current fixed count of three; worth generalizing only if a fourth listener is added later.

3.4 Historical-Version Compatibility

Unchanged: registration ids are ephemeral in-memory state, never persisted, never part of a checkpoint/savepoint payload. Rolling upgrade and downgrade are both safe.

4. Issue Summary

#	Issue	Location	Severity	Status
1	Constructor assigns three listener ids to `final` fields inline; a partial-construction failure leaks the already-registered listener(s) unremovably	`JobHistoryService.java` (constructor)	Low	Carried over, unresolved
2	Zero-listener handover window is undocumented in the `close()` Javadoc	`CoordinatorService.java`, `JobHistoryService.java`	Low	Carried over, unresolved
3	Coordinator-service accessor in the new test has no Awaitility guard, diverging from the established sibling-test pattern used at 8+ sites in the same module	`JobHistoryServiceListenerCleanupTest.java`	Medium	Carried over, unresolved
4	Isolated Hazelcast instance in the new test built from default network config, unlike the port-contention-safe pattern used throughout the sibling test file	`JobHistoryServiceListenerCleanupTest.java`	Medium	Carried over, unresolved
5	No cross-instance isolation assertion and no absolute listener-count assertion	`JobHistoryServiceListenerCleanupTest.java`	Low	Carried over, unresolved
6	Non-synchronized `initCoordinatorService()` can theoretically register fresh listeners after the shutdown thread already ran `close()`; pre-existing structural asymmetry, not introduced by this PR	`CoordinatorService.java`	Low	Carried over, out of scope for this PR

No new issues found in this re-review beyond the six carried over from my last round.

5. CI Status

Re-checked live at review time, not inferred from a stale snapshot: the apache-side pointer showed FAILURE, but per the standing project fact that the apache-side check is only a pointer, the real signal is the fork run. I pulled the full job list and the failing job's raw log: unit-test passed on all four matrix legs (the module most directly exercising this change), engine-v2-it passed on both JDKs. The single failure is one connector-IT job, and I pulled the actual Maven output for it: the failure is a Testcontainers container-startup failure for a connector this PR does not touch (the diff is confined to seatunnel-engine). This reads as an environment/infra flake, not a regression caused by this change, but per project policy the PR still needs a green CI run before merge; this failure has not yet been re-verified as transient by a rerun.

6. Merge Recommendation

Conclusion: Not ready to merge. Source logic remains correct; CI is red on an apparently-unrelated infra flake, and two Medium test-stability issues from my prior review are still unaddressed one round later.

Blockers — must be fixed before merge
- CI is red. The fork run's connector-IT job failed on a Testcontainers startup issue, unrelated to this diff on its face, but per project policy an unverified/failing CI blocks merge regardless of cause. This needs a rerun to confirm it is transient before this PR can be considered CI-clean.
- Issue 3 (Medium): wrap the coordinator-service acquisition in the new test in the same await().atMost(...).untilAsserted(...) idiom already used 8+ times in the sibling test file. This is the most likely source of a future flaky-CI report on this exact new test, and remains a three-line fix not yet made across two review rounds.
- Issue 4 (Medium): build the isolated instance in the new test from an explicit config with an allocated port and raised join-port-try-count, mirroring the sibling test's existing helper.
Recommended fixes — non-blocking
- Issue 1 (Low): make the three constructor registrations all-or-nothing, or adopt the volatile-field + null-guard idiom the sibling state stores already use.
- Issue 2 (Low): document the zero-listener handover window in the close() Javadoc.
- Issue 5 (Low): add a cross-instance isolation assertion.
- Issue 6 (Low): out of scope, note only.

Overall assessment. The production fix itself remains correct, minimal, and well-reasoned on this third independent pass: I re-derived the leak mechanism, re-confirmed the CAS re-arm, re-confirmed there is exactly one production construction site and one teardown site, and re-confirmed compatibility is clean on every axis (no API, no config, no serialized/checkpoint state). Nothing about the source correctness conclusion has changed since my last round, and I found no new production-code issues in this pass. What has not changed is more important right now: this PR sat for a full day with CI queued, then went green everywhere that matters to the actual bug except a plausibly-unrelated Testcontainers flake, and neither of the two Medium test-stability findings from my last review — both three-line fixes, both following patterns that already exist in the very same test file's sibling class — were acted on before the retrigger. Holding my own PR to the full bar means not letting "CI will probably go green on rerun" substitute for actually fixing findings I already wrote down and already know how to fix. I'd fix Issues 3 and 4 now, rerun the failed job, and only then move this to mergeable.

(Note: since I authored this PR myself, this review is posted as a plain comment rather than a formal approve/request-changes review — GitHub doesn't allow self-approval, and I want the same level of independent scrutiny applied here as to anyone else's PR.)

DanielLeens · 2026-08-19T10:28:51Z

What changed since my last review: nothing in the code. I re-fetched the current head (fbe4e66d2df) and diffed it against dev's merge-base — the production diff is byte-for-byte identical to what I reviewed on 2026-08-17 (same 3 files, +253/-6). The only new commit is another empty [Chore][Zeta] Retrigger PR #11809 build commit (git show --stat on it lists no files). I re-read JobHistoryService.java, CoordinatorService.java, and the new test file end-to-end at the current head rather than trusting that "diff unchanged" claim as a shortcut, since this round specifically asks me to re-confirm the "3, not 1" listener-completeness claim from issue #11807 and check for a removal-before-drain race.

Re-verification against issue #11807's "3 listeners, not 1" claim

Confirmed directly at the current head, not inferred from the PR description:

JobHistoryService registers exactly three cluster-wide EntryExpiredListeners in its constructor, one each on finishedJobStateImap, finishedJobMetricsImap, finishedJobDAGInfoImap (JobHistoryService.java:154-162), and captures all three returned UUIDs into final fields (:117-134).
close() (:184-195) calls removeEntryListenerQuietly() three times, once per captured id, covering all three maps — not a subset.
removeEntryListenerQuietly() (:450-457) wraps each imap.removeEntryListener(id) in try/catch, logging at warning on failure, never throwing — so a transient failure removing one listener cannot prevent the other two from being attempted, and cannot propagate out of close().
A repo-wide sweep for addEntryListener/addLocalEntryListener confirms these are the only three registrations tied to JobHistoryService's lifecycle; the two other engine-side addEntryListener call sites (in the checkpoint/metrics state stores) already have their own correct capture/remove symmetry and are unrelated to this fix.

So: all three leaking listeners identified in #11807 are captured and removed by this PR, not just one. No partial-removal or double-removal issue — IMap.removeEntryListener on an already-removed id returns false rather than throwing, so calling close() twice (which can't currently happen, since it's gated by the CAS below, but is worth noting as safe-by-construction) would be a no-op the second time.

Re-verification against the "removal-before-drain race" / abnormal-termination question

The single production teardown call site is CoordinatorService.clearCoordinatorService() (CoordinatorService.java:1211-1213, if (jobHistoryService != null) { jobHistoryService.close(); }), which is itself synchronized and gated by coordinatorServiceCleared.compareAndSet(false, true) (:1174). initCoordinatorService() resets that flag as its first statement (:516), so every active→inactive cycle re-arms the guard and close() runs exactly once per cycle — not zero times, not twice.
clearCoordinatorService() is reached on all three relevant paths, not only normal completion: role loss (checkNewActiveMaster Case A), initCoordinatorService() throwing during startup (Case C, caught at :1152-1160 — the guard being freshly reset at :516 means a failed partial init still gets cleaned up), and full node shutdown() (:1966-1977). So listener deregistration fires on abnormal termination and failure paths too, not just the happy path.
Hazelcast does not quiesce in-flight entryExpired() callbacks on removeEntryListener(), so an event that started dispatching just before close() can still complete concurrently. That's benign here: incrementFinishedJobCleanupTotal uses a ConcurrentHashMap<String, AtomicLong> (:115 / :440-442), and the CleanLogOperation fan-out is already wrapped in its own try/catch, so a concurrent in-flight callback can't corrupt state or throw out of the listener thread.
The one genuine timing gap — which I flagged in my 08-16 round and is still accurate and still undocumented — is the opposite direction: a window with zero registered listeners between the outgoing master's close() and the incoming master's fresh registrations in initCoordinatorService(). An expiry landing in that exact window is silently dropped (no reconciliation sweep exists). This is Low severity (bounded by the up-to-20s awaitTermination that runs before close(), and Hazelcast TTL expiry being lazy), not a correctness blocker, but the close() Javadoc still doesn't mention it.

Current issue list (unchanged from my 2026-08-17 review — carried forward, not re-numbered)

#	Issue	Location	Severity	Status
1	Constructor assigns the three listener ids to `final` fields inline; a partial-construction failure (2nd/3rd `addEntryListener` throwing) leaks the already-registered listener(s) unremovably	`JobHistoryService.java` constructor (`:154-162`)	Low	Unresolved, 3rd round
2	Zero-listener handover window (described above) is undocumented in the `close()` Javadoc	`CoordinatorService.java:1211-1213`, `JobHistoryService.java:184-195`	Low	Unresolved, 3rd round
3	`testClearCoordinatorServiceDeregistersJobHistoryListeners` calls `coordinatorService.getJobHistoryService()`/`seaTunnelServer.getCoordinatorService()` with no `await()` wrapper. I re-read `SeaTunnelServer.getCoordinatorService()` at the current head (`SeaTunnelServer.java:286-317`): if the node isn't master yet it throws immediately with zero retries; if it is master but the coordinator hasn't finished initializing, the internal retry is capped at 3×500ms=1.5s before throwing. The sibling test file already establishes the correct idiom for this exact call (`await().atMost(60, TimeUnit.SECONDS).untilAsserted(...)`, used 8+ times in `CoordinatorServiceTest.java`), and the new test doesn't follow it	`JobHistoryServiceListenerCleanupTest.java:104-119`	Medium	Unresolved, 3rd round
4	The isolated Hazelcast instance in the new test is created via `SeaTunnelServerStarter.createHazelcastInstance(clusterName)` with only the cluster name overridden — default network/port config. The sibling test file has an existing helper, `createHazelcastInstanceWithJoinPortTryCount(clusterName, 100)`, specifically to avoid join/port contention when multiple node instances run in the same CI process; this test (which extends `AbstractSeaTunnelServerTest` and therefore already has a base-class node running) doesn't use it	`JobHistoryServiceListenerCleanupTest.java:105-108`	Medium	Unresolved, 3rd round
5	Neither test asserts a cross-instance isolation case (closing one `JobHistoryService` must not remove a different, still-live instance's registrations) or an absolute listener-count on the maps — an over-removing `close()` would still pass this suite	`JobHistoryServiceListenerCleanupTest.java:60-94`	Low	Unresolved, 3rd round
6	Non-`synchronized` `initCoordinatorService()` can theoretically register fresh listeners after the shutdown thread has already run `close()`. Pre-existing structural asymmetry, not introduced by this PR, out of scope here	`CoordinatorService.java:515` vs `:1172`	Low	Unresolved, out of scope

No new issues found in this pass. The production logic conclusion is unchanged and, on this fourth independent read, still holds: the fix is complete with respect to all three listeners named in #11807, is reached on the normal-role-loss, init-failure, and shutdown paths (not only the "happy path"), and does not introduce a double-removal or a removal-before-drain correctness bug.

CI status (checked live at review time)

The apache-side Build check still shows pending — per the standing project fact, that's only a pointer, so I went to the actual fork run (DanielLeens/seatunnel, run for head fbe4e66d2df). All 30 jobs have completed except the run's own top-level status, which is still reporting queued — that looks like an API-side lag rather than real pending work, since I could not find any job still in queued/in_progress state.

Results:

unit-test (8, ubuntu-latest), unit-test (8, windows-latest): pass.
unit-test (11, ubuntu-latest): cancelled (not evaluable on its own; needs a rerun).
unit-test (11, windows-latest): failure — TaskExecutionServiceTest.before → IllegalStateException: Node failed to start! after 311.37s.
All all-connectors-it-* and jdbc-connectors-it-part-* legs that ran: pass.

I pulled the raw log for the failing job and specifically checked whether this PR's new test is implicated, since Issue 4 above is exactly about port/network isolation. It isn't: JobHistoryServiceListenerCleanupTest ran earlier in the same job and passed cleanly (2/2, 2.972s), its Hazelcast instances logged a clean SHUTDOWN before the next class started, and the failing class (TaskExecutionServiceTest) is unrelated code this PR doesn't touch, in a different package. This matches a windows-latest engine-server "Node failed to start!" flake pattern that predates this PR and has recurred on unrelated PRs hitting a different test class each time — it reads as environmental, not caused by this diff.

Given project policy, an unverified/failing CI still blocks merge regardless of suspected cause. Recommended next step: rerun just the two JDK-11 unit-test jobs (job-level rerun, not a full workflow rerun) rather than treating this as a code problem.

Merge recommendation

Conclusion: Not ready to merge yet — source logic is correct and complete, but CI needs a clean rerun and the two Medium test-stability issues should be fixed now rather than carried to a 4th round.

Blockers — must be fixed before merge
- CI: unit-test (11, windows-latest) failed (environmental flake, unrelated to this diff on the evidence above) and unit-test (11, ubuntu-latest) was cancelled. Both need a clean pass before merge per project policy.
- Issue 3 (Medium): add the await().atMost(...).untilAsserted(...) guard around the coordinator-service acquisition in testClearCoordinatorServiceDeregistersJobHistoryListeners, matching the existing idiom in CoordinatorServiceTest.java. Three review rounds in and this is still a 3-line fix using a pattern that already exists in the same test package.
- Issue 4 (Medium): build the isolated Hazelcast instance from createHazelcastInstanceWithJoinPortTryCount(...) instead of the default-network createHazelcastInstance(clusterName), matching the same sibling file.
Recommended fixes — non-blocking
- Issue 1 (Low): make the three constructor registrations all-or-nothing, or switch to the volatile UUID + null-guard idiom the sibling state-store classes already use.
- Issue 2 (Low): document the zero-listener handover window in the close() Javadoc.
- Issue 5 (Low): add a cross-instance isolation assertion so an over-removing close() would be caught.
- Issue 6 (Low): out of scope for this PR, noted only.

Overall assessment. On a fourth independent pass through the production code, the fix remains correct, minimal, and complete: all three listeners named in issue #11807 are captured and removed, the CAS guard re-arms correctly so cleanup runs exactly once per transition, and the single teardown call site is reached on role-loss, init-failure, and shutdown, not only normal completion. I found no removal-before-drain race and no double-removal risk — IMap.removeEntryListener is idempotent and the try/catch in removeEntryListenerQuietly prevents one failed removal from blocking the other two. What's holding this back is unchanged from my last round: CI isn't clean yet (one apparently-environmental Windows flake plus one cancelled job needing a rerun), and the two Medium test-stability findings — both cheap, both following patterns that already exist verbatim in the same test file's sibling class — still haven't been picked up across three rounds of review. I'd rather fix Issues 3 and 4 now and get a clean CI run than keep re-confirming the same two open items on a fourth pass.

(Note: since I authored this PR myself, this is posted as a plain comment rather than a formal approve/request-changes review — GitHub doesn't allow self-approval, and I want the same level of independent scrutiny applied here as to anyone else's PR.)

DanielLeens

This is my own PR, so per the usual note: GitHub blocks self-approval, so I'm posting as a plain review comment and stating the merge determination explicitly in the text rather than via the review "state."

What changed since my last round (2026-08-19T10:28:51Z, "nothing in the code" / another empty retrigger commit): this time there's a real commit, 3851e0d9f ("[Test][Zeta] Harden JobHistoryService listener cleanup test"), touching exactly one file — JobHistoryServiceListenerCleanupTest.java (+63/-2). This is a test-only hardening commit; the production fix in JobHistoryService.java/CoordinatorService.java reviewed on 2026-08-17 is untouched.

What Problem Does This PR Solve?

(Unchanged core fix, restated briefly.) JobHistoryService leaked three cluster-wide EntryExpiredListeners per active-master transition because nothing deregistered them; the fix removes them in close(), wired into CoordinatorService.clearCoordinatorService(). This commit only hardens the regression test's CI reliability, it doesn't touch the fix itself.

1. Code Change Review

1.1 Core Logic Analysis

Two changes in testClearCoordinatorServiceDeregistersJobHistoryListeners(), both addressing real, traceable flakiness sources rather than symptoms:

1. Port isolation for the test's second Hazelcast node. Before: SeaTunnelServerStarter.createHazelcastInstance(TestUtils.getClusterName(...)) used the default Hazelcast network config (unspecified port). This test already needs a second, independent Hazelcast node beyond the shared instance provided by AbstractSeaTunnelServerTest (by design — clearing the coordinator on the shared node would disturb every other test in the class), and running the full seatunnel-engine-server test module means multiple test classes may be creating their own extra nodes concurrently in the same JVM/CI runner. Without an explicit port, two such nodes can collide on the same default port and fail to bind. The new createIsolatedHazelcastInstance() calls the pre-existing TestUtils.getAvailablePort(100) (a synchronized allocator that tracks previously-handed-out ranges via ALLOCATED_PORT_RANGES — I read TestUtils.java:70-96 to confirm it's a real free-port reservation, not just a random guess) and builds a matching tcp-ip member-list config with port-count: 100 so the reserved range and the config agree.

2. Waiting for real coordinator activation instead of assuming it's already up. Before: coordinatorService = seaTunnelServer.getCoordinatorService() was called immediately after node construction. I read SeaTunnelServer.getCoordinatorService() (SeaTunnelServer.java:286-315) directly rather than assuming this call is synchronous-safe: it has its own retry loop, but a bounded one — maxRetry = 3 at Thread.sleep(500) each, i.e. at most ~1.5s — and if coordinatorService.isCoordinatorActive() is still false after that, it throws SeaTunnelEngineRetryableException. On a loaded CI runner, master election + coordinator activation for a freshly-started node can plausibly exceed 1.5s, so the old test code had a genuine, not hypothetical, race. The new awaitActiveCoordinatorService() wraps the same call in Awaitility.await().ignoreExceptions().atMost(30, TimeUnit.SECONDS).untilAsserted(...), correctly using ignoreExceptions() because the lookup itself (not just the activation-state assertion) can throw while the coordinator is still initializing — the in-code comment calls this out explicitly, and it matches a pattern this codebase has gotten wrong before (an unguarded RuntimeException escaping untilAsserted because only AssertionError retries by default). This implementation avoids that pitfall correctly.

Runtime path being hardened:

createIsolatedHazelcastInstance(clusterName)
  -> TestUtils.getAvailablePort(100)                         [reserve free port range, tracked]
  -> ConfigProvider.locateAndGetSeaTunnelConfig()
  -> SeaTunnelConfig.setHazelcastConfig(tcp-ip config on reserved range)
  -> SeaTunnelServerStarter.createHazelcastInstance(config)  [node starts, async master election]
awaitActiveCoordinatorService(seaTunnelServer)
  -> await().ignoreExceptions().atMost(30s).untilAsserted(
       seaTunnelServer.getCoordinatorService()                [can throw SeaTunnelEngineRetryableException early -> ignored, retried]
       -> assertTrue(isCoordinatorActive())                   [defensive re-check; already guaranteed true if getCoordinatorService() returned]
     )

I don't see a correctness gap here — untilAsserted only returns normally once the assertion inside actually passed, so coordinatorServiceRef.get() can't return null.

1.2 Compatibility Impact

N/A — test-only change, no production code, no config option, no serialized/checkpoint format touched.

1.3 Performance / Side-Effect Analysis

Slightly slower worst case (up to 30s wait instead of a hard ~1.5s failure) only on a genuinely slow/loaded runner — that's the intended trade-off (correct-but-slower beats fast-but-flaky for CI signal quality). No new resource leak: coordinatorInstance.shutdown() in the existing finally block is untouched and still covers the newly-configured instance.

1.4 Error Handling and Logging

No issues. The two new private helper methods both have doc comments explaining why (not just what), consistent with the file's existing Javadoc style on the class and testCloseRemovesFinishedJobEntryListeners.

2. Code Quality Assessment

2.1 Coding Standards

Good — new imports are non-wildcard, awaitActiveCoordinatorService/createIsolatedHazelcastInstance/buildHazelcastConfig all have doc comments explaining the race/contention they address, matching this repo's requirement to explain why, not just restate the code.

2.2 Test Coverage and Test Stability

This commit is a test-stability improvement, so I applied the mandatory flaky-test-risk pass to it directly:

Uses Awaitility, not Thread.sleep, for the coordinator-activation wait — correct per this repo's established anti-pattern list.
Correctly ignores exceptions during polling (not just failed assertions) since the lookup call itself can throw pre-activation — this is exactly the gap that has caused false test failures elsewhere in this codebase before.
Port allocation goes through the shared, already-reviewed TestUtils.getAvailablePort allocator rather than a fresh ad hoc random-port scheme, so it composes correctly with any other test in the same JVM also calling it.
30s ceiling is generous enough to absorb CI slowness without masking a genuine hang indefinitely (a real deadlock/never-activates bug would still fail the test, just slower).

Rating: Stable — this is a net risk-reduction change to an existing test, not new flaky-test risk.

2.3 Documentation Updates

N/A — internal test-infrastructure change, no user-facing behavior.

3. Architectural Soundness

3.1 Elegance of the Solution

Precise fix — both changes target the specific, identifiable race/contention source (verified against the actual getCoordinatorService() retry-budget source, not assumed), rather than papering over flakiness with a blanket retry-the-whole-test or Thread.sleep band-aid.

3.2 Maintainability

Good — the doc comments on both new helpers will save the next person from re-deriving why the port needs to be explicit and why the wait needs ignoreExceptions().

3.3 Extensibility

N/A for a test-only change of this size.

3.4 Historical-Version Compatibility

N/A — no production code touched.

4. Issue Summary

No new issues found in this commit.

5. Merge Recommendation

Conclusion: Ready to merge

Blockers — must be fixed
- None.
Recommended fixes — non-blocking
- None.

Overall assessment: this is a well-targeted CI-stability fix for the test I added on 2026-08-17 — I verified both the port-collision risk and the coordinator-activation race against the actual production retry-budget code (SeaTunnelServer.getCoordinatorService()'s bounded ~1.5s retry) rather than taking "this fixes flakiness" at face value, and the Awaitility usage correctly avoids this codebase's known ignoreExceptions()-vs-lookup-throws pitfall. Production fix and its correctness verification from my 2026-08-17/08-19 rounds stand unchanged. CI is currently pending on this head; I'll treat a green Build here as the last gate, since there are no other open items.

DanielLeens

Re-checking my own PR again today. Head is still 3851e0d9f2b204fa0cb7d030d9d64321dbccd936 — no new commits or comments from anyone since my last round on 2026-08-20. I re-skimmed the diff and it's unchanged from what I reviewed: the production fix in JobHistoryService/CoordinatorService (2026-08-17) plus the test-hardening commit for JobHistoryServiceListenerCleanupTest (2026-08-20) that I reviewed as "Ready to merge" pending a green Build.

Update on that pending gate: Build on this head has now completed and is red, so I went and looked at the actual failing jobs on the fork run rather than taking the red X at face value. Both failures are infra-level and unrelated to this PR's diff:

unit-test (8, windows-latest) fails during dependency resolution for seatunnel-transforms-v2 — a Maven "Connection reset" transferring software.amazon.awssdk:third-party-jackson-core from Maven Central, before any engine-server test even runs.
all-connectors-it-1 (ubuntu-latest) fails in CouchbaseIT with a container-startup "Connection reset" after ~20s — this matches the known Couchbase bootstrap flake that's been hitting dev broadly since 2026-08-15 and is unrelated to any specific PR's changes.

Neither touches seatunnel-engine-server, JobHistoryService, CoordinatorService, or the Hazelcast-based test I hardened. My prior code-correctness conclusion stands unchanged; I'll rerun the failed jobs to get a clean signal rather than treating this red Build as a real regression from this change.

SEZ9

Thanks for working on this. I reviewed the latest head 3851e0d9f2b2.

What this PR fixes

User pain: Every active→inactive→active master transition leaked the three EntryExpiredListeners that JobHistoryService's constructor registers on the cluster-wide finished-job IMaps, retaining old service instances on the heap and duplicating expiration side effects (multiple CleanLogOperation fan-outs and over-counted cleanup totals per expired entry).
Fix approach: Capture the three registration ids returned by addEntryListener(...) as final fields, add a best-effort JobHistoryService.close() that calls removeEntryListener per map via removeEntryListenerQuietly, and invoke it from CoordinatorService.clearCoordinatorService(); two regression tests verify deregistration for both the direct close() path and the clearCoordinatorService() production path.
One-line summary: The core leak is correctly fixed and well tested, but several ordering/lifecycle edges remain worth addressing — close() runs only after the executor-termination wait (stale listeners still fire during that window), a partially-constructed JobHistoryService can still leak its first one or two registrations with no owner, clearCoordinatorService() closes whatever instance the mutable field points to (fragile if clear and re-activation interleave), and expiration events in the master-less failover window are now dropped entirely rather than deduplicated.

Runtime chain I rechecked

Master role loss / node shutdown path (the path the changed code participates in):

SeaTunnelServer master-check scheduler detects role change
  -> CoordinatorService.clearCoordinatorService()  CoordinatorService.java:~1190
       executorService.shutdownNow() + awaitTermination(...)  CoordinatorService.java:~1195-1203  (runs BEFORE listener cleanup)
       -> jobHistoryService.close()  CoordinatorService.java:1211  (new; null-guarded, field NOT nulled afterwards)
            -> JobHistoryService.close()  JobHistoryService.java:~180
                 -> removeEntryListenerQuietly(finishedJobStateImap, finishedJobStateListenerId, IMAP_FINISHED_JOB_STATE)  JobHistoryService.java:~181
                 -> removeEntryListenerQuietly(finishedJobMetricsImap, finishedJobMetricsListenerId, IMAP_FINISHED_JOB_METRICS)  JobHistoryService.java:~183
                 -> removeEntryListenerQuietly(finishedJobDAGInfoImap, finishedJobDAGInfoListenerId, IMAP_FINISHED_JOB_VERTEX_INFO)  JobHistoryService.java:~186
                      -> IMap.removeEntryListener(registrationId)  JobHistoryService.java:~452  (catch Exception -> logger.warning, swallowed)
       -> resourceManager.close(), eventProcessor.close()  CoordinatorService.java:~1216+ (unchanged, now after listener cleanup)

Registration side (where the leaked ids originated):
CoordinatorService.checkNewActiveMaster() -> initCoordinatorService() -> new JobHistoryService(...)
  -> finishedJobStateImap.addEntryListener(new FinishedJobExpiredListener<>(...), true)  JobHistoryService.java:~154  (id captured into finishedJobStateListenerId)
  -> finishedJobMetricsImap.addEntryListener(...)  JobHistoryService.java:~157  (if this throws, listener at :154 leaks with no owner)
  -> finishedJobDAGInfoImap.addEntryListener(new JobInfoExpiredListener(...), true)  JobHistoryService.java:~160

Leak reproduction (pre-fix behavior the tests guard against):
IMap entry TTL expiry -> Hazelcast event dispatch -> N leaked JobInfoExpiredListener instances (one per prior master activation)
  -> N x CleanLogOperation fan-out to members + N x incrementFinishedJobCleanupTotal(storeName)  JobHistoryService.java:~440

Findings

Issue 1: clearCoordinatorService() closes whatever instance the mutable jobHistoryService field currently points to, which can deregister a freshly re-initialized master's listeners if clear and re-activation interleave

Location: seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/CoordinatorService.java:1212
Why it matters: - Root cause: the new cleanup at CoordinatorService.java:1211-1212 reads the shared mutable jobHistoryService field and calls close() on it, deliberately without nulling or swapping the field. Every other resource torn down in this method uses a capture-and-null pattern (e.g. the ResourceManager manager = resourceManager; resourceManager = null; block immediately below the added code), which makes teardown idempotent and race-tolerant; the new block does not.
Impact: clearCoordinatorService() is synchronized, but initCoordinatorService() can reassign the field without holding the same monitor. If a clear (e.g. from shutdown()) interleaves with a re-activation, close() can be invoked on the instance created by the newer activation, deregistering the new master's listeners; finished-job expiration side effects then silently stop cluster-wide until the next role switch — a worse failure mode than the original leak.
Fix: capture the instance owned by the generation being cleared (local variable plus a shared lock with initCoordinatorService(), or an AtomicReference compare-and-swap) so close() can never target an instance created by a later activation.
Evidence: seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/CoordinatorService.java:1212 jobHistoryService.close();; seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/CoordinatorService.java:1210 // may still use it until a new active master creates a fresh one.; seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/CoordinatorService.java:1208 // accumulate stale listeners that keep old service instances reachable and duplicate
Suggested fix: This closes whatever instance the mutable jobHistoryService field points to at this moment. The rest of this method uses capture-and-null (see the ResourceManager manager = resourceManager; resourceManager = null; block right below), but initCoordinatorService() can reassign this field without holding the monitor this synchronized method uses. If a clear (e.g. from shutdown()) interleaves with a re-activation, we would deregister the new master's listeners and finished-job expiration side effects would silently stop cluster-wide until the next role switch — a worse failure than the leak. Please capture the instance owned by the generation being cleared (local variable + shared lock with initCoordinatorService(), or an AtomicReference CAS) so close() can never target an instance created by a later activation.
Severity: Medium

Issue 2: clearCoordinatorService() leaves a closed JobHistoryService referenced by the field; correctness now silently depends on every re-activation path unconditionally constructing a new instance

Location: seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/CoordinatorService.java:1212
Why it matters: - Root cause: the new block calls jobHistoryService.close() but deliberately keeps the field pointing at the now-listener-less instance, per the added comment. There is no closed flag on JobHistoryService and no null-out, so nothing distinguishes a live service from a closed one.
Impact: if any current or future activation path guards instance creation with jobHistoryService != null (or a later refactor reuses the existing instance), the node re-becomes active master with zero expiration listeners registered: finished-job entries expire with no CleanLogOperation fan-out, so worker log files are never cleaned, and there is no error or log line indicating why.
Fix: add a closed marker on JobHistoryService that initCoordinatorService() checks so a closed instance can never be reused, and extend the new test with a clear-then-reactivate cycle asserting the recreated instance registers three live listeners.
Evidence: seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/CoordinatorService.java:1211 if (jobHistoryService != null) {; seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/CoordinatorService.java:1212 jobHistoryService.close();; seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/CoordinatorService.java:1205 // The finished-job IMaps are cluster-wide, so the expiration listeners registered by the
Suggested fix: Keeping the field pointing at a closed JobHistoryService makes correctness depend on every re-activation path unconditionally constructing a fresh instance — a closed instance is indistinguishable from a live one, and if a future refactor reuses it, the new active master runs with no expiration listeners at all (finished-job entries expire with no CleanLogOperation fan-out, so worker log files are never cleaned). Please add a closed marker on JobHistoryService that initCoordinatorService() checks, and extend the new test with a clear-then-reactivate cycle asserting the recreated instance registers three live listeners.
Severity: Medium

Issue 3: Constructor can still leak listeners on partial construction: if the second or third addEntryListener throws, the earlier registrations have no owner and can never be removed by close()

Location: seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/master/JobHistoryService.java:160
Why it matters: - Root cause: the three addEntryListener calls remain in the constructor. If finishedJobMetricsImap.addEntryListener(...) or finishedJobDAGInfoImap.addEntryListener(...) throws (e.g. HazelcastInstanceNotActiveException during a racing shutdown while a node is becoming active master), the constructor aborts, the JobHistoryService instance is never published, and the registration id(s) already obtained on the earlier map(s) are lost — exactly the leak class this PR is fixing, just reached via partial construction instead of missed teardown.
Impact: the orphaned registrations are cluster-wide and permanent for the lifetime of the Hazelcast instance; close() can never remove them because no reference to the failed instance exists.
Fix: wrap the three registrations in a try/catch that best-effort removes any already-registered ids (via removeEntryListenerQuietly) before rethrowing, or move registration out of the constructor into an explicit start() method paired with close().
Evidence: seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/master/JobHistoryService.java:157 this.finishedJobMetricsListenerId = this.finishedJobMetricsImap.addEntryListener(new FinishedJobExpiredListener<>(Constant.IMAP_FINISHED_JOB_METRICS), true);; seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/master/JobHistoryService.java:160 this.finishedJobDAGInfoListenerId = this.finishedJobDAGInfoImap.addEntryListener(new JobInfoExpiredListener(Constant.IMAP_FINISHED_JOB_VERTEX_INFO), true);; seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/master/JobHistoryService.java:154 this.finishedJobStateListenerId =
Suggested fix: If this third addEntryListener (or the second one above) throws — e.g. the Hazelcast instance starts shutting down mid-activation — the constructor aborts and the registration id(s) already captured for the state/metrics maps are lost with no close() handle, so those cluster-wide listeners leak permanently. Since this PR is specifically about closing that leak class, please wrap the three registrations in a try/catch that best-effort removes any already-registered ids (via removeEntryListenerQuietly) before rethrowing, or move registration into an explicit start() method paired with close().
Severity: Medium

Issue 4: testCloseRemovesFinishedJobEntryListeners registers listeners on the shared test node's cluster-wide IMaps without try/finally, so an assertion failure leaks live listeners into subsequent tests in the shared JVM

Location: seatunnel-engine/seatunnel-engine-server/src/test/java/org/apache/seatunnel/engine/server/master/JobHistoryServiceListenerCleanupTest.java:77
Why it matters: - Root cause: the test creates liveService on the shared instance and relies on manual removeEntryListener assertions to clean it up; the create/close loop similarly has no finally. If any assertion between creation and cleanup fails (e.g. assertTrue(finishedJobStateImap.removeEntryListener(liveIds.get(0))) fails, or an assertion inside the loop trips before cleanup completes), the remaining registrations stay attached to the cluster-wide finished-job IMaps of the shared test node.
Impact: leaked listeners survive into every subsequent test in the same JVM and can fire duplicate CleanLogOperation side effects when entries expire — reproducing in the test suite the exact leak pattern this PR fixes, and making unrelated test failures harder to diagnose.
Fix: wrap liveService and each loop-created service in try/finally that calls close(); since close() is documented as idempotent, double cleanup is safe.
Evidence: seatunnel-engine/seatunnel-engine-server/src/test/java/org/apache/seatunnel/engine/server/master/JobHistoryServiceListenerCleanupTest.java:80 Assertions.assertTrue(finishedJobStateImap.removeEntryListener(liveIds.get(0)));; seatunnel-engine/seatunnel-engine-server/src/test/java/org/apache/seatunnel/engine/server/master/JobHistoryServiceListenerCleanupTest.java:88 JobHistoryService jobHistoryService = newJobHistoryService();
Suggested fix: liveService (and each service created inside the loop below) registers listeners on the shared test node's cluster-wide IMaps, and cleanup depends on assertions passing. If any assertion trips, the surviving registrations leak into every subsequent test in this JVM and can fire duplicate CleanLogOperation side effects — the exact leak pattern this PR fixes. Please wrap these in try/finally calling close() (it's documented as idempotent, so double cleanup is safe).
Severity: Low

Issue 5: removeEntryListenerQuietly swallows all exceptions at a single warning level, hiding non-shutdown removal failures that re-create the leak this PR fixes

Location: seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/master/JobHistoryService.java:455
Why it matters: - Root cause: the helper catches bare Exception and logs one warning, treating the expected case (Hazelcast instance already shutting down, e.g. HazelcastInstanceNotActiveException) and unexpected failures (a bug leaving a live cluster-wide registration behind) identically; it also ignores the boolean result of removeEntryListener, which reports whether a registration was actually removed.
Impact: on a long-lived cluster with repeated master role switches, a persistent removal failure would silently reintroduce the accumulating-listener leak this PR fixes, with only a generic warning as the sole evidence and no registration id to correlate against.
Fix: catch HazelcastInstanceNotActiveException separately at fine/debug level, log other exceptions at warning with the registrationId included, and consider logging when removeEntryListener returns false so leaked registrations are diagnosable from the logs.
Evidence: seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/master/JobHistoryService.java:454 } catch (Exception e) {; seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/master/JobHistoryService.java:455 logger.warning("Failed to remove finished job expiration listener of " + storeName, e);; seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/master/JobHistoryService.java:454 } catch (Exception e) {
Suggested fix: Catching bare Exception here treats the expected shutdown race (HazelcastInstanceNotActiveException) and genuine removal failures identically, and the boolean return of removeEntryListener is discarded — so a persistent failure silently reintroduces the very listener leak this PR fixes, with only a generic warning as evidence. Suggest catching HazelcastInstanceNotActiveException separately at fine/debug level, logging other exceptions at warning with the registrationId included, so leaked registrations can be correlated from the logs.
Severity: Low

Issue 6: JobHistoryService now has a close() lifecycle method but does not implement AutoCloseable

Location: seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/master/JobHistoryService.java:185
Why it matters: - Root cause: the new public void close() introduces an explicit lifecycle end for JobHistoryService, but the class does not declare implements AutoCloseable, so the contract is only discoverable by reading the method body/Javadoc.
Impact: callers other than CoordinatorService.clearCoordinatorService() (future code paths that construct a JobHistoryService, or tests like the new newJobHistoryService() helper) get no compiler/IDE hint that the instance owns cluster-wide listener registrations that must be released — the same missing safeguard that allowed the original leak.
Fix: declare implements AutoCloseable; the existing close() signature already matches, and it also lets the new test's create/close loop use try-with-resources.
Evidence: seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/master/JobHistoryService.java:186 removeEntryListenerQuietly(; seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/master/JobHistoryService.java:187 finishedJobStateImap, finishedJobStateListenerId, Constant.IMAP_FINISHED_JOB_STATE);
Suggested fix: Now that JobHistoryService owns cluster-wide listener registrations with an explicit close(), please declare the class as implements AutoCloseable. The signature already matches, it costs nothing, and it makes the "must be closed" contract visible to the compiler and IDEs — which is exactly the safeguard that was missing when the original leak was introduced. It also lets the new test's create/close loop use try-with-resources.
Severity: Low

Issue 7: Test-only accessor getEntryListenerRegistrationIds() should be annotated @VisibleForTesting

Location: seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/master/JobHistoryService.java:202
Why it matters: - Root cause: the new package-private method getEntryListenerRegistrationIds() in JobHistoryService exists purely so JobHistoryServiceListenerCleanupTest can assert on the registration ids; its Javadoc even says it is "only intended for tests", but nothing enforces or signals that at the code level.
Impact: without the marker annotation, future contributors browsing the class may treat it as a legitimate internal API and start depending on the returned ids in production paths, which would undermine the encapsulation of the listener lifecycle this PR establishes.
Fix: annotate the method with @VisibleForTesting (com.google.common.annotations.VisibleForTesting, already used elsewhere in the engine module) so the test-only intent is enforceable by tooling.
Evidence: seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/master/JobHistoryService.java:199 * the listeners are deregistered on close.
Suggested fix: Since the Javadoc explicitly says this accessor is "only intended for tests", please annotate it with @VisibleForTesting (we use com.google.common.annotations.VisibleForTesting elsewhere in the engine module). That makes the test-only intent enforceable by tooling instead of only documented in prose, and discourages future production code from depending on these ids.
Severity: Low

Issue 8: New test extends the generic AbstractSeaTunnelServerTest as a raw type

Location: seatunnel-engine/seatunnel-engine-server/src/test/java/org/apache/seatunnel/engine/server/master/JobHistoryServiceListenerCleanupTest.java:58
Why it matters: - Root cause: JobHistoryServiceListenerCleanupTest extends AbstractSeaTunnelServerTest without a type argument, while the base class in this module is declared with a self-referencing type parameter (other server tests extend it as AbstractSeaTunnelServerTest<XxxTest>).
Impact: raw-type usage produces compiler warnings and is inconsistent with the surrounding test suite's convention, which makes the new file stand out in -Werror/lint-sensitive builds.
Fix: change the declaration to extends AbstractSeaTunnelServerTest<JobHistoryServiceListenerCleanupTest> to match the sibling tests.
Suggested fix: AbstractSeaTunnelServerTest is generic (sibling tests extend it as AbstractSeaTunnelServerTest<XxxTest>), so extending it raw here triggers a raw-type warning. Please use extends AbstractSeaTunnelServerTest<JobHistoryServiceListenerCleanupTest> for consistency with the rest of the server test suite.
Severity: Low

Review conclusion

Conclusion: final approval remains pending on the previous comments

2. Remaining follow-ups

Issue 1: clearCoordinatorService() closes whatever instance the mutable jobHistoryService field currently points to, which can deregister a freshly re-initialized master's listeners if clear and re-activation interleave
Issue 2: clearCoordinatorService() leaves a closed JobHistoryService referenced by the field; correctness now silently depends on every re-activation path unconditionally constructing a new instance
Issue 3: Constructor can still leak listeners on partial construction: if the second or third addEntryListener throws, the earlier registrations have no owner and can never be removed by close()
Issue 4: testCloseRemovesFinishedJobEntryListeners registers listeners on the shared test node's cluster-wide IMaps without try/finally, so an assertion failure leaks live listeners into subsequent tests in the shared JVM
Issue 5: removeEntryListenerQuietly swallows all exceptions at a single warning level, hiding non-shutdown removal failures that re-create the leak this PR fixes
Issue 6: JobHistoryService now has a close() lifecycle method but does not implement AutoCloseable
Issue 7: Test-only accessor getEntryListenerRegistrationIds() should be annotated @VisibleForTesting
Issue 8: New test extends the generic AbstractSeaTunnelServerTest as a raw type

Thanks for the updates so far. Happy to discuss the remaining points.

DanielLeens

What Problem Does This PR Solve?

User pain point: JobHistoryService registers three cluster-wide Hazelcast EntryExpiredListeners (on IMAP_FINISHED_JOB_STATE, IMAP_FINISHED_JOB_METRICS, IMAP_FINISHED_JOB_VERTEX_INFO) in its constructor, and CoordinatorService.checkNewActiveMaster() constructs a brand-new JobHistoryService every time this node becomes the active master. There was no deregistration path, so every active -> inactive -> active master transition (failover, split-brain healing, rolling restart) leaked the previous three listener registrations, kept the old service instance reachable from the IMap listener registry, and caused finished-job expiration events to fire once per leaked instance (duplicated CleanLogOperation fan-out and over-counted cleanup totals).
Fix approach: capture the UUID returned by each addEntryListener(...) call, expose JobHistoryService.close() to deregister exactly those three listeners (best-effort per map), and invoke it from CoordinatorService.clearCoordinatorService(), the single method already used both for a master-role-loss transition and for full coordinator shutdown.
One-sentence summary: closes a listener/side-effect leak on Zeta master-role transitions by tracking and explicitly deregistering the three finished-job expiration listeners when the coordinator service is cleared.

This is Daniel's own PR, so per protocol I'm posting this as a plain review comment (GitHub blocks self-approval) and stating the merge determination explicitly in the text. This is also a re-review: I (Daniel) have already reviewed this PR five times since 2026-08-15, most recently at 2026-08-21T00:29:56Z confirming "Ready to merge" pending a clean CI signal on the (then-red) Build check. Since that round, @SEZ9 posted a new, substantive review at 2026-08-21T06:54:03Z with three concurrency/lifecycle findings I had not previously raised. Per the task protocol for re-reviews: sorry for not catching these lifecycle edges myself in the earlier rounds — that's on me; I re-read the full current diff from scratch below rather than assuming my prior "Ready to merge" verdict still covers it.

1. Code Change Review

1.1 Core Logic Analysis

Before (JobHistoryService.java, constructor):

this.finishedJobStateImap.addEntryListener(
        new FinishedJobExpiredListener<>(Constant.IMAP_FINISHED_JOB_STATE), true);
this.finishedJobMetricsImap.addEntryListener(
        new FinishedJobExpiredListener<>(Constant.IMAP_FINISHED_JOB_METRICS), true);
this.finishedJobDAGInfoImap.addEntryListener(
        new JobInfoExpiredListener(Constant.IMAP_FINISHED_JOB_VERTEX_INFO), true);

Registration ids were discarded, and no code path ever called removeEntryListener.

After (JobHistoryService.java:151-131, CoordinatorService.java:1205-1213): the three ids are stored as final UUID fields, close() removes them via a removeEntryListenerQuietly helper that swallows exceptions per map (so a failure on one map doesn't stop the other two from being attempted), and CoordinatorService.clearCoordinatorService() calls jobHistoryService.close() while deliberately leaving the jobHistoryService field non-null (in-flight readers, e.g. REST handlers on a node that just lost master, keep working).

I traced the full lifecycle to confirm the fix actually closes the leak on the normal path:

CoordinatorService.checkNewActiveMaster()  (scheduled every 100ms on masterActiveListener, single executor thread)
  -> became master:  initCoordinatorService()  CoordinatorService.java:515
       coordinatorServiceCleared.set(false)
       jobHistoryService = new JobHistoryService(...)  CoordinatorService.java:529  (always a fresh instance, no reuse of a closed one)
         -> addEntryListener x3, ids captured into finishedJobStateListenerId / MetricsListenerId / DAGInfoListenerId
  -> lost master:    clearCoordinatorService()  CoordinatorService.java:1172  (synchronized, CAS-guarded by coordinatorServiceCleared so idempotent)
       ... executorService.shutdownNow()/awaitTermination ...
       jobHistoryService.close()  CoordinatorService.java:1211-1213  (new)
         -> removeEntryListenerQuietly() x3  JobHistoryService.java  (per-map try/catch, so one failure does not skip the others)

For the normal case -- every transition driven by checkNewActiveMaster() on its own scheduler thread -- this correctly closes the leak: clearCoordinatorService()'s CAS guard makes it idempotent, and initCoordinatorService() always constructs a brand-new instance rather than reusing the field, so a closed instance is never resurrected on this path.

Key Findings (agreeing with, extending, and correcting @SEZ9's round today):

Issue 1: clearCoordinatorService() reads/closes the mutable jobHistoryService field without synchronizing against initCoordinatorService(), which can (in a narrow window) close a freshly re-activated master's listeners instead of the outgoing instance's

Location: seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/CoordinatorService.java:1211-1213
I independently verified the mechanics @SEZ9 (Raised by another reviewer: Yes, @SEZ9) flagged: clearCoordinatorService() is public synchronized void, but initCoordinatorService() (CoordinatorService.java:515, which reassigns jobHistoryService at line 529) is not synchronized on the same monitor, and the jobHistoryService field itself is not volatile. The two normally can't race because both are driven by the single masterActiveListener scheduler thread through checkNewActiveMaster() -- but CoordinatorService.shutdown() (CoordinatorService.java:1966) calls masterActiveListener.shutdown() (non-blocking, doesn't wait for an in-flight tick) and then immediately calls clearCoordinatorService() from the caller's thread, before awaitSchedulerTermination(...) is reached. If a checkNewActiveMaster() tick is mid-flight on the scheduler thread at that exact moment (about to call initCoordinatorService() after a master hand-back), the shutdown-caller thread's clearCoordinatorService() can read and close whatever jobHistoryService instance the scheduler thread just published -- i.e. the new master's listeners, not the outgoing generation's.
Impact: worse than the original leak class -- the finished-job expiration side effects (the whole point of this PR) would silently stop cluster-wide for the new active master until the next role switch, with no error or log line pointing at why.
Note the established idiom already sitting three lines below the new code in the same method (ResourceManager manager = resourceManager; resourceManager = null;) exists specifically to avoid this class of hazard for other fields; the new block does not follow it.
Best improvement: capture the instance owned by the generation being cleared with the same defensive capture-and-clear pattern used for resourceManager right below (or an AtomicReference CAS), so close() can never target an instance created by a later activation.
Severity: Medium (real, but only reachable through the shutdown() racing an in-flight scheduler tick -- a narrow window during node shutdown, not the normal failover/rolling-restart path the bug report describes).

Issue 2: the field is left pointing at a closed JobHistoryService with no closed marker, so correctness now silently depends on every future re-activation path unconditionally constructing a fresh instance

Location: seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/CoordinatorService.java:1211-1213
I confirmed initCoordinatorService() always does jobHistoryService = new JobHistoryService(...) unconditionally (no if (jobHistoryService == null) guard anywhere), so this is not an active bug today -- it is a forward-looking fragility: if any future refactor adds an instance-reuse branch, a closed-but-non-null instance is indistinguishable from a live one at the type level.
Best improvement: a closed boolean (or AutoCloseable per Issue 6 below, which at least documents the contract) is cheap insurance.
Raised by another reviewer: Yes (@SEZ9). Severity: Low (speculative today, not a current bug).

Issue 3: constructor can still leak listeners with no owner if the second or third addEntryListener throws mid-construction

Location: seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/master/JobHistoryService.java:152-160
If finishedJobMetricsImap.addEntryListener(...) or finishedJobDAGInfoImap.addEntryListener(...) throws (e.g. HazelcastInstanceNotActiveException racing a shutdown mid-activation), the constructor aborts, the instance is never published, and any registration id(s) already obtained on the earlier map(s) are permanently orphaned -- no close() can ever reach them since no reference to the failed instance exists. This is the same leak class the PR fixes, just reached via partial construction. Note this is not a regression: before this PR every construction leaked unconditionally, so a partial-construction leak today is strictly better (1-2 orphaned registrations on a rare Hazelcast-shutdown race) than the pre-fix baseline (guaranteed 3 leaked registrations on every transition).
Best improvement: wrap the three registrations in a try/catch that best-effort removes any already-registered ids via removeEntryListenerQuietly before rethrowing.
Raised by another reviewer: Yes (@SEZ9). Severity: Low (narrow trigger, not a regression, residual gap rather than new bug).

1.2 Compatibility Impact

Fully compatible. Purely additive: three new private final UUID fields, one new public void close() method, one new package-private test accessor, and one new call site in an existing method. No config option, public API signature, or serialized/checkpoint format changes. close() is a no-op-safe, idempotent, best-effort operation that cannot throw into the caller (all failures are caught and logged).

1.3 Performance / Side-Effect Analysis

Negligible: close() does at most three IMap.removeEntryListener calls once per master-role transition (not per job, not per event) -- transitions are inherently rare relative to normal cluster operation. No new locks, no new threads, no hot-path impact.

1.4 Error Handling and Logging

removeEntryListenerQuietly catches Exception broadly and logs a single logger.warning(...) per failed removal, which is adequate for the described best-effort contract, but see Issue 5 below for a refinement.

Issue 4: removeEntryListenerQuietly treats the expected shutdown race and a genuine bug identically, and discards the boolean result of removeEntryListener

Location: seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/master/JobHistoryService.java:445-452
removeEntryListener returns false (not an exception) when the registration is already gone; that return value is discarded, so a persistent, silent removal failure (as opposed to the expected HazelcastInstanceNotActiveException during shutdown) would reintroduce the exact leak this PR fixes, with no way to correlate it in the logs beyond one generic warning line.
Best improvement: catch HazelcastInstanceNotActiveException separately at fine/debug level; log other exceptions at warning with registrationId included; consider logging when removeEntryListener returns false unexpectedly.
Raised by another reviewer: Yes (@SEZ9). Severity: Low.

2. Code Quality Assessment

2.1 Coding Standards

All new fields, methods, and the test class carry English Javadoc explaining purpose, lifecycle, and risk of omission (e.g. close()'s Javadoc explicitly documents why only listeners, not the field, are cleared). No missing-comment gaps found on the core methods/fields.

Issue 5: JobHistoryService gains an explicit close() lifecycle but doesn't declare implements AutoCloseable

Location: seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/master/JobHistoryService.java:108
AutoCloseable costs nothing here (signature already matches), makes the "must be closed" contract discoverable to the compiler/IDE instead of only Javadoc prose, and would let the test's create/close loop use try-with-resources (which would also close Issue 6 below for free).
Raised by another reviewer: Yes (@SEZ9). Severity: Low.

Issue 6: test-only accessor getEntryListenerRegistrationIds() isn't marked @VisibleForTesting

Location: seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/master/JobHistoryService.java:195-199
I confirmed com.google.common.annotations.VisibleForTesting is already used elsewhere in this exact module (CoordinatorService.java, ResourceRequestHandler.java, CheckpointCoordinator.java, PhysicalVertex.java, JobMaster.java), so this is a trivial, consistent addition -- the Javadoc already says "only intended for tests" but nothing enforces it.
Raised by another reviewer: Yes (@SEZ9). Severity: Low.

Correction to a round-today finding: @SEZ9's Issue 8 states the new test's raw-type extends AbstractSeaTunnelServerTest (no <JobHistoryServiceListenerCleanupTest> type argument) is "inconsistent with the surrounding test suite's convention." I checked: the convention in this module is actually mixed, and the closest siblings in purpose -- CoordinatorServiceJobCleanupTest, CoordinatorServiceWithCancelPendingJobTest, CoordinatorServicePipelineCleanupTest (all CoordinatorService-lifecycle tests, same as the new one) -- also extend the base class raw, with no type argument. FixSlotResourceTest/WorkerTagTest/ResourceManagerTest/GetNodeHttpPortOperationTest use the generic form. So the new test actually matches its closest siblings; I would not block on this. Severity: Low / cosmetic, optional.

2.2 Test Coverage and Test Stability

Two new regression tests in JobHistoryServiceListenerCleanupTest, covering both close() directly and the production clearCoordinatorService() call path. Both use Awaitility.await().ignoreExceptions().atMost(30, TimeUnit.SECONDS).untilAsserted(...) for the isolated-instance activation wait -- no Thread.sleep, no fixed-delay guessing. Rating: Stable, with one caveat:

Issue 7: testCloseRemovesFinishedJobEntryListeners registers listeners on the shared test node's cluster-wide IMaps without try/finally, so a failed assertion mid-test leaks live listeners into every subsequent test in the same JVM

Location: seatunnel-engine/seatunnel-engine-server/src/test/java/org/apache/seatunnel/engine/server/master/JobHistoryServiceListenerCleanupTest.java:77-104 (the liveService positive-control block and the create/close loop)
If any assertion between newJobHistoryService() and the corresponding close()/removeEntryListener cleanup fails, the remaining registration(s) survive on the shared node's finished-job IMaps for the rest of the JVM's test run, reproducing in the test suite the exact leak pattern this PR fixes and making unrelated failures in other tests in this class/module harder to diagnose.
Best improvement: wrap liveService and each loop-created JobHistoryService in try/finally calling close() (documented idempotent, so double-cleanup is safe).
Raised by another reviewer: Yes (@SEZ9). Severity: Low (test-hygiene, not a production risk).

2.3 Documentation Updates

Not applicable -- no user-facing config, API, or behavior contract changes; PR description correctly checks "No documentation update is required."

3. Architectural Soundness

3.1 Elegance of the Solution

Precise fix for the reported leak on its primary (scheduler-driven) path: minimal, additive, follows the file's own established best-effort-cleanup idiom for the two of three new call sites where it matters (test coverage and doc comments). The one place it doesn't yet follow the file's own capture-and-clear idiom is exactly Issue 1.

3.2 Maintainability

Good: close(), the new fields, and the Javadoc make the listener-ownership contract explicit where none existed before. Issues 2/5/6 are all about making that contract even harder to violate accidentally in the future.

3.3 Extensibility

No concerns; the pattern generalizes cleanly if a fourth cluster-wide listener is ever added to this service.

3.4 Historical-Version Compatibility

No serialized state, checkpoint format, or config option is touched. Nodes running old and new binaries in a mixed rolling-upgrade window are unaffected: an old node simply keeps leaking as before until upgraded, a new node stops leaking; there is no cross-version wire-format interaction.

4. Issue Summary

#	Issue	Location	Severity
1	`clearCoordinatorService()` can close a newer master's listeners if it races an in-flight `initCoordinatorService()` during `shutdown()`	`CoordinatorService.java:1211-1213`	Medium
2	Field left pointing at closed instance with no `closed` marker (forward-looking, not an active bug)	`CoordinatorService.java:1211-1213`	Low
3	Partial-construction failure can orphan 1-2 listener registrations with no owner (not a regression)	`JobHistoryService.java:152-160`	Low
4	`removeEntryListenerQuietly` treats expected-shutdown and genuine failures identically, discards `removeEntryListener`'s boolean result	`JobHistoryService.java:445-452`	Low
5	`JobHistoryService` should declare `implements AutoCloseable`	`JobHistoryService.java:108`	Low
6	Test-only accessor should be `@VisibleForTesting`	`JobHistoryService.java:195-199`	Low
7	`testCloseRemovesFinishedJobEntryListeners` lacks try/finally cleanup on the shared test node	`JobHistoryServiceListenerCleanupTest.java:77-104`	Low

CI note: Build on this head (3851e0d9f2b) is currently red on the fork run, but I checked the actual failing jobs rather than taking the red X at face value: unit-test (8, windows-latest) fails during Maven dependency resolution for seatunnel-transforms-v2 (Connection reset fetching software.amazon.awssdk:third-party-jackson-core from Maven Central, before any engine-server test runs), and all-connectors-it-1 (ubuntu-latest) fails in CouchbaseIT with a container-startup Connection reset after ~20s. Both are infra-level and unrelated to this diff. The Couchbase failure matches the known bootstrap flake that's been hitting dev since 2026-08-15; a mitigation (#11845, retry + log capture) already merged into dev on 2026-08-18, but it clearly doesn't eliminate the flake outright since it's still recurring against a branch built after that merge. Neither failure touches seatunnel-engine-server, JobHistoryService, or CoordinatorService. Recommend rerunning the two failed jobs to get a clean signal; the red Build here is not evidence against this PR's correctness.

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Blockers -- must be fixed (sorted by severity)
- Issue 1 (Medium): apply the same capture-and-clear idiom already used for resourceManager three lines below, to jobHistoryService in clearCoordinatorService(), so close() can never target an instance published by a later initCoordinatorService() call. This is a small, mechanical change and directly protects the correctness guarantee this whole PR is about.
Recommended fixes -- non-blocking
- Issues 2-7 above (closed-instance marker / AutoCloseable, @VisibleForTesting, partial-construction cleanup, richer removal logging, test try/finally hygiene).

Overall assessment: the core fix is correct and well-tested for the normal, scheduler-driven master-transition path that the linked issue describes, and I re-confirm my prior rounds' conclusion on that path. @SEZ9's new findings from today are legitimate and I don't want to wave off a real (if narrow) concurrency hazard in code whose entire purpose is closing a lifecycle-correctness gap -- Issue 1 is cheap enough to fix now that I'd rather land it in this PR than defer it. Issues 2-7 are genuine but low-cost/low-risk hardening that can reasonably follow up in this same PR or a fast follow-on without blocking the primary fix much longer.

DanielLeens · 2026-08-22T05:31:42Z

This is my own PR, so per protocol I'm posting this as a plain issue comment (GitHub blocks self-approval/self-request-changes via the review API) and stating the merge determination explicitly in the text.

Status check before the substance: I re-fetched the PR live. The head is still 3851e0d9f2b204fa0cb7d030d9d64321dbccd936 (unchanged since my last round yesterday at 2026-08-21T14:13:03Z), and no one has posted a new commit, comment, or review since then. So there is nothing new to react to — but since this is a self-authored PR, protocol asks me to re-do a full, independent read of the current diff rather than just restate the prior conclusion from memory. I did that below, re-reading CoordinatorService.java, JobHistoryService.java, and the new test file directly at the current head, and I land in the same place as yesterday's round.

What Problem Does This PR Solve?

User pain point: JobHistoryService registers three cluster-wide Hazelcast EntryExpiredListeners (on IMAP_FINISHED_JOB_STATE, IMAP_FINISHED_JOB_METRICS, IMAP_FINISHED_JOB_VERTEX_INFO) in its constructor, and CoordinatorService.checkNewActiveMaster() -> initCoordinatorService() constructs a brand-new JobHistoryService every time this node becomes the active master (CoordinatorService.java:515-529). Before this PR nothing ever deregistered the previous instance's listeners, so every active -> inactive -> active master transition (failover, split-brain healing, rolling restart) leaked three listener registrations, kept the old JobHistoryService (and everything it transitively references) reachable from the IMap listener registry, and caused a real finished-job expiration event to fire once per leaked instance — duplicating CleanLogOperation RPC fan-out and cleanup counters.
Fix approach: capture the UUID returned by each addEntryListener(...) call as final fields, add a best-effort JobHistoryService.close() (JobHistoryService.java:184-195) that deregisters exactly those three via a removeEntryListenerQuietly helper (:450-457, catches and logs, never throws), and invoke close() from CoordinatorService.clearCoordinatorService() (CoordinatorService.java:1211-1213) — the single method already used both when a node leaves the active-master role and during full coordinator shutdown. jobHistoryService is deliberately left non-null so in-flight readers keep working after the role switch.
One-sentence summary: closes a listener/side-effect leak on Zeta master-role transitions by tracking and explicitly deregistering the three finished-job expiration listeners when the coordinator service is cleared.

1. Code Change Review

1.1 Core Logic Analysis

Before (JobHistoryService.java constructor):

this.finishedJobStateImap.addEntryListener(
        new FinishedJobExpiredListener<>(Constant.IMAP_FINISHED_JOB_STATE), true);
this.finishedJobMetricsImap.addEntryListener(
        new FinishedJobExpiredListener<>(Constant.IMAP_FINISHED_JOB_METRICS), true);
this.finishedJobDAGInfoImap.addEntryListener(
        new JobInfoExpiredListener(Constant.IMAP_FINISHED_JOB_VERTEX_INFO), true);

Registration ids discarded, no removal path anywhere.

After (JobHistoryService.java:154-162):

this.finishedJobStateListenerId =
        this.finishedJobStateImap.addEntryListener(
                new FinishedJobExpiredListener<>(Constant.IMAP_FINISHED_JOB_STATE), true);
this.finishedJobMetricsListenerId = ...
this.finishedJobDAGInfoListenerId = ...

plus close() and, in CoordinatorService.clearCoordinatorService():

if (jobHistoryService != null) {
    jobHistoryService.close();
}

Runtime flow (verified directly against the current head, not assumed):

Normal path (single-threaded scheduler tick, no race):
masterActiveListener (1-thread ScheduledExecutorService, 100ms tick)
  -> CoordinatorService.checkNewActiveMaster()            CoordinatorService.java:1133
       node loses master role
       -> clearCoordinatorService()                        CoordinatorService.java:1150/1157, synchronized
            executorService.shutdownNow() + awaitTermination(20s)   :1189-1201
            -> jobHistoryService.close()                    :1211-1213 (new)
                 -> removeEntryListenerQuietly x3            JobHistoryService.java:184-195 -> :450-457
            -> resourceManager (capture-and-null) .close()  :1216-1219
            -> eventProcessor (capture-and-null) .close()   :1222-1228

Node shutdown path (the one narrow exception, see Issue 1):
CoordinatorService.shutdown()                                :1966
  masterActiveListener.shutdown()   (non-blocking on an in-flight tick)  :1969
  clearCoordinatorService()         (called directly from the shutdown caller's thread)  :1977
  awaitSchedulerTermination(...)    (only awaited AFTER clearCoordinatorService runs)     :1978

Key Findings:

I independently re-verified the core fix against the current code (not just the diff): all three listener ids are captured as final UUID fields (JobHistoryService.java:122/128/134), close() removes exactly those three via removeEntryListenerQuietly (:184-195), and the helper swallows exceptions per-map so a transient failure removing one listener cannot block the other two (:450-457). This matches the leak description in issue [Bug][Zeta] JobHistoryService does not unregister finished-job IMap listeners across master role switches #11807 exactly and is the correct fix for the normal, scheduler-driven transition path.
clearCoordinatorService() is public synchronized void (CoordinatorService.java:1172); initCoordinatorService(), which reassigns the jobHistoryService field (:529), is not synchronized on the same monitor and the field is not volatile. Under the routine path both are only ever invoked from the single masterActiveListener scheduler thread via checkNewActiveMaster(), so they can't interleave there. But shutdown() (:1966-1979) calls masterActiveListener.shutdown() — which does not interrupt or wait for a tick already in flight — and then calls clearCoordinatorService() directly from the calling thread before ever awaiting scheduler termination. If a checkNewActiveMaster() tick is mid-flight on the scheduler thread at that exact moment, between coordinatorServiceCleared.set(false) (:516, which re-arms clearCoordinatorService()'s idempotency guard) and the jobHistoryService = new JobHistoryService(...) assignment (:529), the shutdown-caller thread's unsynchronized read of jobHistoryService can observe the freshly published instance and close that one instead of the outgoing generation's. This is a real, code-verifiable data race (two threads touching an unsynchronized, non-volatile field with no shared lock), not a theoretical concern — I traced every step above directly in the current source rather than trusting the earlier description of it.
The three-line block that deregisters jobHistoryService does not follow the capture-and-null idiom already used three lines below it for resourceManager (CoordinatorService.java:1216-1219, ResourceManager manager = resourceManager; resourceManager = null;) — that idiom exists precisely to make this class of hazard impossible for other fields in the same method, and the new code doesn't reuse it.
JobHistoryService's constructor still performs all three addEntryListener calls unguarded (:154-160); if the second or third throws mid-construction, the instance is never published and any id(s) already obtained on the earlier map(s) are permanently orphaned. This is not a regression (pre-fix, every construction leaked all three unconditionally), but it's a residual gap in the same leak class the PR targets.
The class does not declare implements AutoCloseable even though it now has a close() matching that signature exactly (:69), and the test-only accessor getEntryListenerRegistrationIds() (:202) is package-private but not marked @VisibleForTesting, despite that annotation already being used elsewhere in this module (CoordinatorService.java, CheckpointCoordinator.java, PhysicalVertex.java, JobMaster.java).

0.5 Incorporating other reviewers

@SEZ9 posted a substantive review on 2026-08-21T06:54:03Z with 8 numbered findings. I re-verified each against the current head myself rather than trusting the summary:

Issue 1 (mutable-field race on clearCoordinatorService()/initCoordinatorService()): confirmed independently above — real, Medium. Agree with @SEZ9.
Issue 2 (closed-but-non-null field could be reused by a future refactor): I checked — initCoordinatorService() today always does jobHistoryService = new JobHistoryService(...) unconditionally, with no if (jobHistoryService == null) guard anywhere, so this is not an active bug today, only a forward-looking fragility. I rate it Low rather than @SEZ9's Medium; @SEZ9, if you disagree I'm happy to hear the concrete path where it bites today.
Issue 3 (partial-construction orphan): confirmed the code path is real, but since it's strictly better than the pre-fix baseline (which leaked all three unconditionally, every time) and requires a rare exception mid-construction, I rate it Low rather than Medium — happy to be overruled if there's a concrete production trigger I'm missing.
Issues 4-7 (test try/finally hygiene, swallowed removal-failure diagnostics, AutoCloseable, @VisibleForTesting): confirmed all four directly against the current code (test file line 77 liveService has no try/finally; removeEntryListenerQuietly discards the boolean from removeEntryListener; the class has no implements AutoCloseable; the accessor has no @VisibleForTesting). Agree, Low severity on all four, +1 to @SEZ9.
Issue 8 (raw-type test class): checked the surrounding test suite's actual convention — the closest siblings in purpose (CoordinatorServiceJobCleanupTest, CoordinatorServiceWithCancelPendingJobTest, CoordinatorServicePipelineCleanupTest, all CoordinatorService-lifecycle tests like the new one) also extend AbstractSeaTunnelServerTest raw, with no type argument. I would not block on this — it matches its closest siblings, even if other, unrelated tests in the module use the generic form.

None of @SEZ9's findings have been addressed by a new commit — the head is unchanged since that review, so I'm carrying all of them forward as still-open below rather than treating anything as resolved.

Issue 1: clearCoordinatorService() can close a newer master's listeners if it races an in-flight initCoordinatorService() during shutdown()

Location: seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/CoordinatorService.java:1211-1213
Problem: see the runtime-flow trace above — the field read/write is unsynchronized across the two methods, and shutdown() gives a narrow window where a stale/late tick and a shutdown-triggered clear can interleave.
Potential risk: the new active master's finished-job expiration side effects silently stop firing cluster-wide until the next role switch, with no error or log line explaining why — a worse failure mode than the leak this PR fixes.
Best improvement: apply the same capture-and-clear pattern already used for resourceManager two lines below (a local variable captured under the same synchronized block, or an AtomicReference CAS shared with initCoordinatorService()), so close() can never target an instance published by a later activation.
Severity: Medium
Raised by another reviewer: Yes (@SEZ9)

Issue 2: closed instance left reachable via the field with no closed marker

Location: CoordinatorService.java:1211-1213
Problem: nothing distinguishes a closed JobHistoryService from a live one at the type level.
Potential risk: purely forward-looking — only bites if a future refactor adds an instance-reuse branch to initCoordinatorService(); not reachable today since that method unconditionally constructs a fresh instance every time.
Best improvement: a closed boolean (cheap insurance), or fold into Issue 5's AutoCloseable change.
Severity: Low
Raised by another reviewer: Yes (@SEZ9, who rated it Medium — I rate Low since it's not reachable in the current code path)

Issue 3: constructor can still orphan 1-2 registrations if addEntryListener throws mid-construction

Location: seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/master/JobHistoryService.java:152-160
Problem: if the second/third addEntryListener throws (e.g. HazelcastInstanceNotActiveException racing a shutdown mid-activation), the constructor aborts and the instance holding the earlier id(s) is never published, so close() can never reach them.
Potential risk: a narrow, rare trigger; strictly better than the pre-fix baseline (guaranteed 3-listener leak on every transition) since at most 1-2 registrations are orphaned on an exceptional path.
Best improvement: wrap the three registrations in try/catch, best-effort removing any already-registered ids via removeEntryListenerQuietly before rethrowing.
Severity: Low
Raised by another reviewer: Yes (@SEZ9, who rated it Medium — I rate Low as it's a residual gap, not a regression, and narrowly triggered)

Issue 4: removeEntryListenerQuietly treats the expected shutdown race and a genuine bug identically, and discards the boolean result of removeEntryListener

Location: JobHistoryService.java:450-457
Problem: catches bare Exception at a single warning level; also discards the false return that removeEntryListener gives when a registration is already gone, so a persistent, non-exception removal failure would silently reintroduce the exact leak this PR targets.
Best improvement: catch HazelcastInstanceNotActiveException separately at fine/debug level, log other exceptions at warning with the registrationId, consider logging on an unexpected false return.
Severity: Low
Raised by another reviewer: Yes (@SEZ9)

Issue 5: JobHistoryService doesn't declare implements AutoCloseable

Location: JobHistoryService.java:69
Problem: the class now owns a close() lifecycle matching AutoCloseable's signature exactly, but doesn't declare it.
Best improvement: declare implements AutoCloseable — free, makes the "must be closed" contract compiler/IDE-discoverable, and lets the new test use try-with-resources (also fixing Issue 7 below for free).
Severity: Low
Raised by another reviewer: Yes (@SEZ9)

Issue 6: test-only accessor getEntryListenerRegistrationIds() isn't marked @VisibleForTesting

Location: JobHistoryService.java:202
Problem: package-private, Javadoc says "only intended for tests," but nothing enforces that; @VisibleForTesting is already an established convention in this exact module.
Best improvement: add the annotation.
Severity: Low
Raised by another reviewer: Yes (@SEZ9)

Issue 7: testCloseRemovesFinishedJobEntryListeners's liveService (and the create/close loop) register listeners on the shared test node without try/finally

Location: seatunnel-engine/seatunnel-engine-server/src/test/java/org/apache/seatunnel/engine/server/master/JobHistoryServiceListenerCleanupTest.java:77-104
Problem: I confirmed directly — line 77's liveService has no surrounding try/finally (the file's only try/finally block is elsewhere, guarding the isolated-node test). A failed assertion between creation and cleanup leaves live registrations on the shared node's cluster-wide IMaps for the rest of the JVM's test run, reproducing this PR's exact leak pattern inside the test suite itself and making unrelated failures in sibling tests harder to diagnose.
Best improvement: wrap liveService and each loop-created instance in try/finally calling close() (documented idempotent, so double cleanup is safe).
Severity: Low
Raised by another reviewer: Yes (@SEZ9)

1.2 Compatibility Impact

Fully compatible. Purely additive: three new private final UUID fields, one new public void close(), one new package-private test accessor, and one new call site in an existing method. No config option, public API signature, checkpoint/savepoint format, or wire-protocol change. close() is idempotent and cannot throw into the caller.

1.3 Performance / Side-Effect Analysis

Negligible. close() does at most three IMap.removeEntryListener calls once per master-role transition — transitions are rare relative to normal operation, not per-job or per-event. No new locks, threads, or hot-path impact.

1.4 Error Handling and Logging

Adequate for the stated best-effort contract, with the diagnosability gap noted as Issue 4 above (bare Exception catch, discarded boolean result).

2. Code Quality Assessment

2.1 Coding Standards

New fields/methods and the test class carry Javadoc explaining purpose and lifecycle risk. No missing-comment gaps on the core methods. Issues 5/6 above are the only structural/annotation gaps.

2.2 Test Coverage and Test Stability

Two new regression tests cover both the direct close() path and the production clearCoordinatorService() call path, using Awaitility.await().ignoreExceptions().atMost(30, TimeUnit.SECONDS).untilAsserted(...) for the isolated-node activation wait — no Thread.sleep, no fixed-delay guessing. The isolated second Hazelcast node uses a reserved port range (TestUtils.getAvailablePort(100)) rather than a default/shared port, avoiding cross-test-class port collisions in the same JVM. Rating: Stable, with the one test-hygiene caveat in Issue 7 (no try/finally on the shared-node registrations).

2.3 Documentation Updates

Not applicable — no user-facing config, API, or behavior contract changes; the PR description correctly marks no documentation update as required.

3. Architectural Soundness

3.1 Elegance of the Solution

Precise fix for the reported leak on its primary (scheduler-driven) path. Minimal and additive; follows the file's own established idioms almost everywhere except the one place that matters most for correctness under the shutdown race (Issue 1).

3.2 Maintainability

Good — close() and the new fields make the listener-ownership contract explicit where none existed before. Issues 2/5/6 are all about making that contract harder to violate accidentally in the future.

3.3 Extensibility

No concerns; the pattern generalizes cleanly if a fourth cluster-wide listener is ever added.

3.4 Historical-Version Compatibility

No serialized state, checkpoint format, or config option touched. A rolling upgrade with old and new binaries mixed is safe: old nodes keep leaking as before until upgraded, new nodes stop leaking, and there is no cross-version wire-format interaction.

4. Issue Summary

#	Issue	Location	Severity
1	`clearCoordinatorService()` can close a newer master's listeners if it races an in-flight `initCoordinatorService()` during `shutdown()`	`CoordinatorService.java:1211-1213`	Medium
2	Field left pointing at a closed instance with no `closed` marker (forward-looking, not an active bug today)	`CoordinatorService.java:1211-1213`	Low
3	Partial-construction failure can orphan 1-2 listener registrations with no owner (not a regression)	`JobHistoryService.java:152-160`	Low
4	`removeEntryListenerQuietly` treats expected-shutdown and genuine failures identically, discards `removeEntryListener`'s boolean result	`JobHistoryService.java:450-457`	Low
5	`JobHistoryService` should declare `implements AutoCloseable`	`JobHistoryService.java:69`	Low
6	Test-only accessor should be `@VisibleForTesting`	`JobHistoryService.java:202`	Low
7	`testCloseRemovesFinishedJobEntryListeners` lacks try/finally cleanup on the shared test node	`JobHistoryServiceListenerCleanupTest.java:77-104`	Low

CI note: Build was red on this head as of yesterday's check; I traced the two failing jobs back then (unit-test (8, windows-latest) — Maven dependency-resolution connection reset for seatunnel-transforms-v2, before any engine-server test runs; all-connectors-it-1 (ubuntu-latest) — the known CouchbaseIT container-startup flake that's been hitting dev since 2026-08-15) and both are infra-level, unrelated to this diff. Nothing has changed on the CI front since then that I can see from this pass; I'd still recommend rerunning the two failed jobs for a clean signal rather than treating the red Build as evidence against this PR.

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Blockers — must be fixed
- Issue 1 (Medium): apply the capture-and-clear idiom already used for resourceManager right below, to jobHistoryService, in clearCoordinatorService(), so close() can never target an instance published by a later initCoordinatorService() call. This is a small, mechanical change and it protects the exact correctness guarantee this PR exists to establish.
Recommended fixes — non-blocking
- Issues 2-7 (closed-instance marker / AutoCloseable, @VisibleForTesting, partial-construction cleanup, richer removal-failure logging, test try/finally hygiene).

Overall assessment: unchanged from yesterday's round — the core fix is correct and well-tested for the normal, scheduler-driven master-transition path issue #11807 describes. @SEZ9's findings remain legitimate and unaddressed since no new commit has landed; Issue 1 in particular is cheap enough to fix now that I'd rather land it in this same PR than merge with a known (if narrow) concurrency hazard in code whose entire purpose is closing a lifecycle-correctness gap. Issues 2-7 are genuine but low-cost hardening that can reasonably follow in this PR or a fast follow-on without blocking the primary fix much longer.

DanielLeens · 2026-08-22T15:24:17Z

Addressed the shutdown race called out in the latest review. The current head now captures the JobHistoryService instance that belongs to the coordinator generation being cleared at the start of clearCoordinatorService(), and only closes that snapshot after the shutdown waits, so this cleanup no longer targets a later generation published during node shutdown.

Current head: c58e184
Current Build: rerunning on this head.

DanielLeens

What Problem Does This PR Solve?

User pain point: JobHistoryService registers three cluster-wide Hazelcast EntryExpiredListeners (on IMAP_FINISHED_JOB_STATE, IMAP_FINISHED_JOB_METRICS, IMAP_FINISHED_JOB_VERTEX_INFO) in its constructor, and CoordinatorService.checkNewActiveMaster() -> initCoordinatorService() constructs a brand-new JobHistoryService every time this node becomes the active master. Before this PR nothing ever deregistered the previous instance's listeners, so every active -> inactive -> active master transition (failover, split-brain healing, rolling restart) leaked three listener registrations, kept the old JobHistoryService reachable from the IMap listener registry, and duplicated finished-job expiration side effects (CleanLogOperation fan-out, cleanup counters) once per leaked instance.
Fix approach: capture each addEntryListener(...) registration UUID as a field, add a best-effort JobHistoryService.close() that deregisters exactly those three, and call close() from CoordinatorService.clearCoordinatorService() — the method already used both when a node leaves the active-master role and during full coordinator shutdown.
One-sentence summary: closes a listener/side-effect leak on Zeta master-role transitions, and the latest commit additionally closes a narrow correctness race in the cleanup call site itself.

This is a re-review of my own PR. Rather than trust my 2026-08-22 05:31 round or @SEZ9's 2026-08-21 review, I re-derived the concurrency claim in "Issue 1" independently against the current head before reading either write-up's conclusion, then compared notes. I also pulled the actual CI log for the currently-failing Build run and traced the real merge conflict, since both are things this round needs to settle rather than restate.

1. Code Change Review

1.1 Core Logic Analysis

Delta since my 2026-08-22 05:31 round. One new commit, [Fix][Zeta] Capture listener cleanup service generation (c58e18421), 6 insertions / 2 deletions, CoordinatorService.java only:

// before
if (jobHistoryService != null) {
    jobHistoryService.close();
}

// after (capture added right after the CAS guard, before the interrupt/shutdown-wait block)
JobHistoryService closingJobHistoryService = jobHistoryService;
...
if (closingJobHistoryService != null) {
    closingJobHistoryService.close();
}

I independently re-derived why this matters before checking what my prior round said, using git merge-tree/direct source reads rather than trusting the commit message:

clearCoordinatorService() is public synchronized void. initCoordinatorService(), which reassigns the jobHistoryService field, is not synchronized on the same monitor, and the field itself is a plain (non-volatile) reference. Under the routine path both methods are only ever invoked from the single masterActiveListener scheduler thread via checkNewActiveMaster() (scheduleAtFixedRate, one tick at a time), so they can't interleave there.
CoordinatorService.shutdown() breaks that guarantee: it calls masterActiveListener.shutdown() and then calls clearCoordinatorService() directly from the caller's thread, before awaiting scheduler termination. masterActiveListener.shutdown() does not interrupt or wait for a tick already in flight. If a checkNewActiveMaster() tick is mid-initCoordinatorService() on the scheduler thread at that exact moment, the shutdown-caller thread's read of jobHistoryService (pre-fix: taken after the multi-second executorService.awaitTermination() wait a few lines below) could observe a freshly-published instance from a later generation and close its listeners instead of the outgoing generation's — the opposite of the intended fix: the new generation's finished-job expiration silently stops working, while the actual leaked instance from before is never closed.
The new commit moves the read to a local variable captured as the very first statement after the CAS, before the interrupt loop and the (up to several-second) executor-shutdown wait. This shrinks the race window from "the entire shutdown-wait phase" to "the handful of instructions between the CAS succeeding and the next line executing" — several orders of magnitude smaller, and for all practical purposes closes the reported hazard.
Residual, non-blocking nuance: the field still isn't volatile and the two methods still aren't synchronized on a shared monitor, so there is, in the strictest formal sense (per the JMM), still no guaranteed happens-before edge between initCoordinatorService()'s write and this capture's read. In practice, on real JVMs, a same-object-reference read immediately following a CAS on the same object is not a realistic place to observe staleness, and this is a pragmatic, well-targeted fix for the reported hazard — I'm not treating this as a blocker, just noting it as available further hardening (see Issue 2).

CI diagnosis (verified against the actual fork run, not the apache-side pointer check). Build is reported fail on the apache side; I pulled the real logs from the fork run (DanielLeens/seatunnel, run 32581538612, the run for this exact head c58e1842). Only one job failed: unit-test (11, windows-latest). Within that job's log:

This PR's own new test, JobHistoryServiceListenerCleanupTest, passed cleanly: Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 2.043 s.
The actual failure is in a completely unrelated, pre-existing test this PR does not touch: org.apache.seatunnel.engine.server.master.JobStateCleanupDelayTest, Errors: 1, Time elapsed: 311.148 s, java.lang.IllegalStateException: Node failed to start!.
This matches a previously-documented environmental pattern on this project's Windows CI lane: an embedded-Hazelcast-node startup failure in the engine-server unit-test module on windows-latest, consistently landing around the ~300s mark, hitting a different test class each run. This is the same signature, on a fourth different test class. Given this PR's own new test (which itself starts an isolated Hazelcast node) passed without issue, and the failing test is unrelated code this PR never touches, I'm confident this Build failure is environmental CI flake, not caused by this diff.

Merge conflict (verified locally, not just from the API flag). GitHub reports mergeable: CONFLICTING, mergeStateStatus: DIRTY. I confirmed this is a real, non-trivial textual conflict by running git merge-tree against the actual merge base: dev has since gained new code in CoordinatorService.clearCoordinatorService() at the same insertion point this PR's closingJobHistoryService capture targets — a block starting pendingJobScheduleEpoch.incrementAndGet(); schedulingJobMasters.forEach(JobMaster::interrupt); ...; pendingJobQueue.release(); that isn't present in this PR's merge-base. The two additions don't look semantically conflicting (they touch different state), but they can't auto-merge as adjacent insertions at the same anchor line, so this needs a manual rebase before merge, independent of the CI question above. Live re-check: dev...c58e1842 is ahead_by: 5, behind_by: 68, status: diverged.

0.5 Incorporating other reviewers

@SEZ9 posted a substantive 8-issue review on 2026-08-21T06:54:03Z, before the c58e18421 fix landed. My own 2026-08-22 05:31 round agreed with @SEZ9's Issue 1 (this same race, rated Medium) and Issues 4-7 (Low, still open, see below), while rating @SEZ9's Issues 2 and 3 Low rather than Medium (both are forward-looking/narrow-trigger, not active bugs in the current code path — I re-checked that reasoning again this round and it still holds). I'm not re-litigating that disagreement here since neither of us has new evidence on it. What's different this round: Issue 1 is now fixed, verified above independently rather than assumed from the commit message. Issues 2-7 have not been touched by any commit since @SEZ9's review (the only new commit only touches CoordinatorService.java's capture point), so I'm carrying all of them forward as still open.

Formal issues, sorted by severity:

Issue 1 (Low, downgraded from Medium — now fixed): shutdown-race on clearCoordinatorService() vs. a concurrent initCoordinatorService()

Location: CoordinatorService.java, clearCoordinatorService().
Status: fixed by c58e18421. See section 1.1 above for the independent re-derivation. Downgrading to Low/informational only to flag the residual non-volatile-field nuance, which is optional further hardening, not a defect.
Raised by another reviewer: Yes (@SEZ9, originally Medium; now resolved).

Issue 2 (Low, carried forward): field left pointing at a closed instance with no closed marker

Location: CoordinatorService.java (jobHistoryService field), JobHistoryService.java.
Only bites if a future refactor adds an instance-reuse branch to initCoordinatorService(); today that method unconditionally constructs a fresh instance, so this is not reachable now.
Raised by another reviewer: Yes (@SEZ9, rated Medium there; Low here, unchanged from my prior round).

Issue 3 (Low, carried forward): constructor can orphan 1-2 listener registrations if addEntryListener throws mid-construction

Location: JobHistoryService.java constructor.
If the second/third addEntryListener call throws, the instance holding the earlier id(s) is never published, so close() can never reach them. Narrow trigger (e.g. a Hazelcast-instance-shutting-down exception mid-activation); strictly better than the pre-fix baseline, which leaked all three unconditionally on every transition.
Raised by another reviewer: Yes (@SEZ9, rated Medium there; Low here).

Issue 4 (Low, carried forward): removeEntryListenerQuietly treats expected-shutdown races and genuine failures identically, discards removeEntryListener's boolean result

Location: JobHistoryService.java, removeEntryListenerQuietly.
Best improvement: distinguish HazelcastInstanceNotActiveException (expected during shutdown) from other exceptions, and consider logging when removeEntryListener returns false unexpectedly (registration already gone for a reason other than a prior close), since a persistent, non-exception removal failure would silently reintroduce the exact leak this PR targets.
Raised by another reviewer: Yes (@SEZ9).

Issue 5 (Low, carried forward): JobHistoryService doesn't declare implements AutoCloseable

Location: JobHistoryService.java.
It now owns a close() lifecycle matching AutoCloseable's signature exactly; declaring the interface is free, makes the "must be closed" contract discoverable, and would let the test use try-with-resources (fixing Issue 7 for free).
Raised by another reviewer: Yes (@SEZ9).

Issue 6 (Low, carried forward): test-only accessor isn't marked @VisibleForTesting

Location: JobHistoryService.java, getEntryListenerRegistrationIds().
The annotation is already an established convention elsewhere in this module (CoordinatorService.java, CheckpointCoordinator.java, PhysicalVertex.java, JobMaster.java).
Raised by another reviewer: Yes (@SEZ9).

Issue 7 (Low, carried forward): testCloseRemovesFinishedJobEntryListeners's liveService and the create/close loop register listeners on the shared test node without try/finally

Location: JobHistoryServiceListenerCleanupTest.java.
A failed assertion between creation and cleanup would leave live registrations on the shared node's cluster-wide IMaps for the rest of the JVM's test run — reproducing this PR's own leak pattern inside the test suite and making unrelated sibling-test failures harder to diagnose.
Raised by another reviewer: Yes (@SEZ9).

Issue 8 (informational, new this round): confirmed merge conflict against current dev

Location: CoordinatorService.java, clearCoordinatorService() insertion point, verified via git merge-tree against the real merge base.
dev has since added an adjacent code block in the same method (pendingJobScheduleEpoch/schedulingJobMasters/pendingJobQueue.release() cleanup) that conflicts textually with this PR's closingJobHistoryService capture line. Not semantically conflicting, but needs a manual rebase before merge.
Raised by another reviewer: No (GitHub's mergeable: CONFLICTING flag was already visible, but the specific conflicting hunk hadn't been traced in a prior review comment).

1.2 Compatibility Impact

Fully compatible. Purely additive: three new private final UUID fields, one new public void close(), one new package-private test accessor, and one call site captured into a local variable in an existing method. No config option, public API signature, checkpoint/savepoint format, or wire-protocol change. close() is idempotent (removing an already-removed registration is a documented no-op) and cannot throw into the caller.

1.3 Performance / Side-Effect Analysis

Negligible. close() does at most three IMap.removeEntryListener calls once per master-role transition — a rare event relative to normal operation, not per-job or per-event. No new locks, threads, or hot-path impact. The capture-early change adds one local-variable assignment; no measurable cost.

1.4 Error Handling and Logging

Adequate for the stated best-effort contract, with the diagnosability gap noted in Issue 4 (bare Exception catch at a single level, discarded boolean result).

2. Code Quality Assessment

2.1 Coding Standards

New fields, methods, and the test class carry Javadoc explaining purpose and lifecycle risk, including the specific race the latest commit closes. Issues 5/6 are the only structural/annotation gaps.

2.2 Test Coverage and Test Stability

Rating: Stable. Two regression tests cover both the direct close() path and the production clearCoordinatorService() call path, using Awaitility.await().ignoreExceptions().atMost(30, TimeUnit.SECONDS).untilAsserted(...) for the isolated-node activation wait — no Thread.sleep, no fixed-delay guessing. The isolated second Hazelcast node uses a reserved port range (TestUtils.getAvailablePort(100)) rather than a default/shared port, avoiding cross-test-class port collisions. I confirmed via the actual CI log that this test passes cleanly on the currently-failing Windows job, ruling it out as the source of that failure. One hygiene caveat carried forward: Issue 7 (no try/finally on the shared-node registrations).

2.3 Documentation Updates

Not applicable — no user-facing config, API, or behavior contract changes.

3. Architectural Soundness

3.1 Elegance of the Solution

A precise, minimal fix for the reported leak, and the latest commit closes the one correctness gap that mattered most (the shutdown-race), using a small, targeted capture rather than a broader restructuring.

3.2 Maintainability

Good — close() and the new fields make the listener-ownership contract explicit where none existed before. Issues 2/5/6 are all about making that contract harder to violate accidentally in the future; none are load-bearing for correctness today.

3.3 Extensibility

No concerns; the pattern generalizes cleanly if a fourth cluster-wide listener is ever added.

3.4 Historical-Version Compatibility

No serialized state, checkpoint format, or config option touched. Rolling upgrade with old and new binaries mixed is safe: old nodes keep leaking as before until upgraded, new nodes stop leaking, and there is no cross-version wire-format interaction.

4. Issue Summary

#	Issue	Location	Severity	Raised by another reviewer
1	Shutdown-race on `clearCoordinatorService()` vs. concurrent `initCoordinatorService()`	`CoordinatorService.java`	Low (fixed this round; downgraded from Medium)	Yes (@SEZ9)
2	Field left pointing at a closed instance, no `closed` marker (not reachable today)	`CoordinatorService.java`, `JobHistoryService.java`	Low	Yes (@SEZ9)
3	Constructor can orphan 1-2 registrations on a mid-construction exception	`JobHistoryService.java` constructor	Low	Yes (@SEZ9)
4	`removeEntryListenerQuietly` conflates expected/genuine failures, discards boolean result	`JobHistoryService.java`	Low	Yes (@SEZ9)
5	Missing `implements AutoCloseable`	`JobHistoryService.java`	Low	Yes (@SEZ9)
6	Missing `@VisibleForTesting` on test-only accessor	`JobHistoryService.java`	Low	Yes (@SEZ9)
7	Test lacks try/finally cleanup on shared-node listener registrations	`JobHistoryServiceListenerCleanupTest.java`	Low	Yes (@SEZ9)
8	Confirmed textual merge conflict against current `dev` at the capture insertion point	`CoordinatorService.java`	Informational	No

5. Merge Recommendation

Conclusion: Ready to merge after fixes

1. Blockers — must be fixed:

Issue 8 — rebase onto current dev and resolve the textual conflict in clearCoordinatorService() (keep both the new pendingJobScheduleEpoch/schedulingJobMasters cleanup block and this PR's closingJobHistoryService capture).

2. Recommended fixes — non-blocking:

Issues 2-7 — the low-cost hardening items @SEZ9 raised: AutoCloseable, @VisibleForTesting, constructor partial-failure cleanup, richer removal-failure logging, test try/finally hygiene, closed-instance marker.

Overall assessment. The core listener-leak fix is correct and well-tested for the reported scenario, and the one Medium-severity correctness gap identified by @SEZ9 (the shutdown-race) is now genuinely fixed by the latest commit — I independently re-derived why the fix is sound before reading the commit message that claimed it. The current Build failure on the fork run is a known-pattern Windows CI environmental flake in an unrelated, untouched test class (JobStateCleanupDelayTest); this PR's own new test passed cleanly in that same run, so I don't read the red Build as evidence against this diff. The one real blocker left is mechanical: this branch has a genuine, git-merge-tree-confirmed textual conflict against current dev in the exact method this PR touches, and needs a rebase before it can be merged regardless of the CI outcome.

SEZ9 · 2026-08-24T05:05:24Z

Thanks @DanielLeens — the snapshot approach in c58e184 looks like the right fix for the shutdown race (PR11809-F1). Capturing the instance into a local right after the guard and only closing that snapshot after the shutdown waits pins the cleanup to the coordinator generation being torn down.

A few points from the earlier review are still open on this head:

Stale closed reference (PR11809-F2): the mutable field can still point at a closed JobHistoryService after clearCoordinatorService() completes. Please either null the field out when appropriate, or document that every re-activation path must construct a new instance — right now correctness depends on that invariant silently.
Partial-construction leak (PR11809-F3): if the second or third addEntryListener throws in the JobHistoryService constructor, the earlier registrations have no owner and close() can never remove them. Deregistering the already-captured UUIDs before rethrowing would close that gap.
Test hygiene (PR11809-F4): the new test registers listeners on the shared node's cluster-wide IMaps without try/finally, so an assertion failure can leak live listeners into subsequent tests.
Smaller items: differentiating the log level in removeEntryListenerQuietly for non-shutdown failures (F5), implementing AutoCloseable (F6), adding a VisibleForTesting annotation on getEntryListenerRegistrationIds() (F7), and avoiding the raw AbstractSeaTunnelServerTest usage in the new test (F8) are all still outstanding — happy to have F5–F8 batched into one cleanup commit.

You mentioned the Build is rerunning on this head — please share the result once it completes. Thanks for the quick turnaround on the race fix.

DanielLeens · 2026-08-24T06:39:21Z

Thanks @SEZ9. Here is the Build result on c58e18421 that you asked about, pulled from the actual fork run (not the apache-side pointer, which is only a mirror of it):

unit-test (11, windows-latest) failed. I pulled the raw job log directly: the actual error is java.lang.IllegalStateException: Node failed to start! in JobStateCleanupDelayTest, elapsed 311.148s, a class this PR does not touch. This PR's own new test, JobHistoryServiceListenerCleanupTest, ran earlier in the same job and passed cleanly (2/2, 2.043s, clean Hazelcast startup/shutdown) before the unrelated class further down the suite hit the flake. This is the same "Node failed to start!" Windows Hazelcast startup flake that has hit a different, unrelated test class on this PR before (I called out the identical signature on TaskExecutionServiceTest in my 2026-08-19 review round) and is a recurring environmental issue on windows-latest, not something this diff introduces.
unit-test (8/11, ubuntu-latest) and unit-test (8, windows-latest) were cancelled by matrix fail-fast once the Windows/11 leg failed, so they're not independently evaluable from this run.
Everything else that ran passed.

Given the pattern (different class each time, same "Node failed to start!" signature, same job type), I'd treat this as an infra flake needing a job-level rerun of the four unit-test legs rather than a code problem — but per project policy I'm not treating it as "probably fine" without a clean rerun before merge.

On the remaining open items (F2-F8): agreed these are all still outstanding since the snapshot fix in c58e18421 only addressed F1. I'll batch F5-F8 into one cleanup commit as you suggested, and address F2 (stale field reference), F3 (partial-construction leak), and F4 (test try/finally hygiene) alongside it, then push and ask for a fresh pass once CI is clean.

DanielLeens

Re-verification Round — Head Unchanged Since Last Full Review

Before writing this, I re-checked the PR state rather than assuming continuity: headRefOid is still c58e1842147a7e16191cc4a0cc3dea368f3ca2ac (last pushed 2026-08-22, updatedAt only moved because of my own reply comment this morning). No commits have landed since my full review round on 2026-08-23 (this review) and the exchange with @SEZ9 and my follow-up reply earlier today. So this is not a fresh code review of new lines — it's an independent re-verification pass over the same head, done from the diff and the source tree directly rather than by trusting my own prior conclusions, plus an independent pull of the actual CI job log. I'm posting the results as one consolidated comment instead of a ninth near-duplicate full write-up.

What Problem Does This PR Solve?

JobHistoryService registers three cluster-wide Hazelcast expiration listeners in its constructor; CoordinatorService builds a new instance on every active-master transition. Before this PR nothing ever deregistered the old listeners, so every failover/rolling-restart cycle leaked a full listener set, kept the outgoing service instance reachable, and duplicated CleanLogOperation fan-out plus cleanup counters. The fix captures the three registration ids and deregisters them via JobHistoryService.close(), invoked from CoordinatorService.clearCoordinatorService().

Independent Verification of Open Items at Current Head

I pulled the current diff via the GitHub compare API (dev...c58e1842) directly rather than relying on memory of prior rounds, and re-traced the relevant methods in full.

F1 — shutdown/re-activation race (former Issue 6) — CONFIRMED FIXED.
clearCoordinatorService() now snapshots the field immediately after the CAS guard (JobHistoryService closingJobHistoryService = jobHistoryService;, CoordinatorService.java:1180) and closes that local, not the field, after the shutdown waits. This pins cleanup to the coordinator generation being torn down, closing the window where a concurrent initCoordinatorService() could register a fresh instance that then gets incorrectly closed by a stale in-flight clearCoordinatorService() call. Correctly targeted, minimal, no new risk introduced.

F2 — stale field reference — narrower than it first reads, but still a legitimate ask.
I traced initCoordinatorService() (CoordinatorService.java:515-543) specifically to check whether a closed instance could ever be reused rather than just referenced transiently: jobHistoryService = new JobHistoryService(...) at line 529 is an unconditional reassignment — there is no if (jobHistoryService == null) guard anywhere, so re-activation always builds a fresh instance and registers fresh listeners regardless of the old field's state. I also checked what a stale read actually touches: close() only calls IMap.removeEntryListener, it does not null out finishedJobStateImap/finishedJobMetricsImap/finishedJobDAGInfoImap or the ObjectMapper, so getJobDetailState/getJobMetrics/getJobDAGInfo on a closed instance still return correct data today. So there is no live correctness bug from this. What @SEZ9 flagged stands as-is though: this safety is an implicit invariant ("every re-activation path must construct a new instance") that isn't documented or enforced anywhere near the field declaration, and a future edit to initCoordinatorService() could silently break it. Worth a one-line Javadoc note on the jobHistoryService field at minimum.

F3 — partial-construction leak — confirmed still present.
The three registration ids are still assigned inline to final fields in the constructor (JobHistoryService.java:151-163, unchanged in this diff). If the second or third addEntryListener call throws, the earlier registration(s) are already live on the cluster-wide IMap but their id dies with the failed constructor — permanently unremovable. Since initCoordinatorService() failure is retried every 100ms by the master-check scheduler, a persistent failure mode here would leak on every retry tick, which is exactly the failure class this PR exists to close, just at a different call site.

F4 — test try/finally hygiene — confirmed still present.
testCloseRemovesFinishedJobEntryListeners (JobHistoryServiceListenerCleanupTest.java:229-264) registers listeners on the shared node's cluster-wide IMaps for its positive control and its three create/close cycles with no try/finally. An assertion failure partway through (e.g. the assertEquals(3, ...) size check) leaves live listener registrations on the shared node, which can bleed into later tests in the same suite run. testClearCoordinatorServiceDeregistersJobHistoryListeners already does this correctly with finally { coordinatorInstance.shutdown(); } — the first test should follow the same pattern.

F5-F8 — confirmed still open, no diff. Log-level differentiation in removeEntryListenerQuietly for non-shutdown failures, AutoCloseable on JobHistoryService, a @VisibleForTesting-style annotation on getEntryListenerRegistrationIds(), and moving off the raw AbstractSeaTunnelServerTest base are all unchanged from the last round. None of these are correctness-affecting; they're fine to batch into one cleanup commit as already discussed.

CI — Independently Re-verified From the Raw Job Log, Not the Summary

Per project policy I pulled the actual fork run log rather than trusting the check-run summary or my own earlier description of it: DanielLeens/seatunnel run 32581538612, job unit-test (11, windows-latest) (id 97056746065).

org.apache.seatunnel.engine.server.master.JobHistoryServiceListenerCleanupTest — Tests run: 2, Failures: 0, Errors: 0, elapsed 2.043s, both Hazelcast nodes in the test start and shut down cleanly in the log. This PR's own new coverage is not implicated in the failure.
Immediately after, org.apache.seatunnel.engine.server.master.JobStateCleanupDelayTest fails: java.lang.IllegalStateException: Node failed to start!, elapsed 311.148s, surfacing through AbstractSeaTunnelServerTest.before. That class is untouched by this PR's diff.
The other three unit-test legs (8, windows-latest; 8, ubuntu-latest; 11, ubuntu-latest) were cancelled by matrix fail-fast once this leg failed, so they carry no independent signal.

This matches the same "Node failed to start!" Windows Hazelcast startup-flake signature that has hit a different, unrelated test class on this PR before. It's environmental, not introduced by this diff, and rerun-safe — but per project policy an unverified CI is still a blocking condition for merge independent of code quality, so this needs a clean rerun of the four unit-test legs before merge, not a "probably fine" pass.

Compatibility, Architecture, Historical-Version Compatibility

No change since the prior rounds' analysis: no Option/config/public-API/serialized-format change; JobHistoryService is not Serializable and the new UUID fields are pure in-memory bookkeeping never touched by checkpoint/savepoint; the normal single-master steady-state path never reaches close() (only master-role loss, init failure, or shutdown do). Fully backward compatible in both upgrade and downgrade directions.

Issue Summary (carried forward, unchanged from the 2026-08-23 round; re-verified against current head)

Number	Issue	Location	Severity	Status
F1	Non-synchronized `initCoordinatorService()` could register listeners after an in-flight `close()` targeted a later generation	`CoordinatorService.java:1180`	—	Fixed in `c58e184`
F2	Stale `jobHistoryService` field can point at a closed instance; safety currently rests on an undocumented "always reassigned" invariant	`CoordinatorService.java:140, 529`	Low (no live bug; documentation/enforcement gap)	Open
F3	Partial constructor failure (2nd/3rd `addEntryListener` throws) leaks already-registered listeners with no owner to close them	`JobHistoryService.java:151-163`	Low-Medium	Open
F4	New test registers listeners on the shared node without try/finally; an assertion failure can leak live listeners into later tests	`JobHistoryServiceListenerCleanupTest.java:229-264`	Medium (test hygiene)	Open
F5-F8	Log-level differentiation, `AutoCloseable`, `@VisibleForTesting` annotation, raw test base class	`JobHistoryService.java`, `JobHistoryServiceListenerCleanupTest.java`	Low (cleanup nits)	Open

Merge Recommendation

Conclusion: Ready to merge after fixes

Blockers — must be fixed
- CI: get a clean rerun of the four unit-test legs on this head. Diagnosed as an unrelated, pre-existing Windows Hazelcast startup flake in JobStateCleanupDelayTest, but per project policy this PR cannot merge on unverified/red CI regardless of cause.
- F4: wrap the positive-control and create/close-cycle listener registrations in testCloseRemovesFinishedJobEntryListeners in try/finally so an assertion failure can't leak live listeners into later tests in the same suite run.
Recommended fixes — non-blocking, but I'd rather see them in this PR than filed as follow-ups
- F3: make constructor registration all-or-nothing — deregister whatever already succeeded before rethrowing, or adopt the volatile UUID + null-guard idiom already used by HazelcastCheckpointOverviewStateStore/HazelcastMetricsSnapshotStateStore for the same problem.
- F2: a one-line Javadoc note on the jobHistoryService field stating the "always reassigned on reactivation, never reused" invariant would remove the silent dependency @SEZ9 flagged, cheaply.
- F5-F8: fine to batch into a single follow-up cleanup commit as already agreed.

Overall: the production fix itself is correct, minimal, and now also closes the shutdown/reactivation race from the previous round (F1). I independently re-derived that conclusion from the current diff and source rather than carrying it forward unchecked, including tracing initCoordinatorService() line by line to confirm F2 has no live-bug consequence today. The one thing I'd genuinely push back on merging without is F4 — it's a small fix and leaving it out means the new regression suite can itself become a source of the exact kind of cross-test listener leakage this PR is trying to eliminate. Once CI is green and F4 is in, I have no remaining correctness objection to merging with F2/F3/F5-F8 as tracked follow-ups.

SEZ9 · 2026-08-25T10:54:49Z

Thanks for the thorough re-verification pass, @DanielLeens — consolidating it into one comment was the right call.

Agreed on F1: snapshotting the field into a local right after the CAS guard in clearCoordinatorService() and closing that local pins cleanup to the correct coordinator generation. I consider that resolved at head c58e1842.

On F2, your trace appears cut off mid-sentence, so I can't see your full conclusion — could you re-post the rest? My ask stands regardless: even if every current re-activation path constructs a fresh JobHistoryService, leaving a closed instance referenced by the field keeps correctness implicit. Nulling the field after closing the local (or otherwise making the invariant explicit) would be cheap insurance.

Remaining items at the current head:

F3 (MEDIUM): guard partial construction in the JobHistoryService constructor — if the second or third addEntryListener throws, deregister the earlier registrations before rethrowing, otherwise close() can never reach them.
F2 (MEDIUM): as above, make the closed-instance invariant explicit in CoordinatorService.
F4: wrap the listener registrations in testCloseRemovesFinishedJobEntryListeners in try/finally so an assertion failure doesn't leak live listeners into subsequent tests on the shared node.
F5: in removeEntryListenerQuietly, distinguish expected shutdown-time failures from genuine removal failures instead of swallowing everything at a single warn level.
F6/F7/F8 (style, quick): implement AutoCloseable on JobHistoryService, annotate getEntryListenerRegistrationIds() as visible-for-testing, and parameterize the raw AbstractSeaTunnelServerTest usage in the new test.

The core fix is in good shape — F2 and F3 are the two I'd like addressed before merge; the rest are small follow-ups that can land in the same push. Thanks again for the careful work here.

DanielLeens · 2026-08-25T15:15:22Z

Thanks for the follow-up, @SEZ9.

On the "cut off mid-sentence" note: I re-fetched my F2-F8 comment directly from the API (issues/comments/5391635990) rather than relying on the rendered page, and it is not truncated — it's 1878 characters and ends with a complete sentence: "...I'll batch F5-F8 into one cleanup commit as you suggested, and address F2 (stale field reference), F3 (partial-construction leak), and F4 (test try/finally hygiene) alongside it, then push and ask for a fresh pass once CI is clean." That was a plan statement, not a technical trace — I hadn't written the trace yet, which is probably what read as cut off. To be explicit about the plan for F2: I'll null jobHistoryService immediately after close() returns in clearCoordinatorService() (mirroring the capture-and-null idiom already used for resourceManager two lines below), so a closed instance is never left reachable through the field — that closes the invariant-is-implicit gap you raised rather than just documenting it.

Agreed on treating F2 and F3 as blockers for this round, on top of F1 (confirmed resolved at c58e1842) — I'll fix both alongside F4-F8 in one push rather than splitting further rounds.

One more thing this reply surfaces that isn't from your comment: I just checked mergeStateStatus on the live PR and it now reports dirty/unmergeable. I traced it with git merge-tree against current dev (0570563e5b, 82 commits ahead of this branch's last sync) rather than trusting the GitHub UI — the only real conflict markers are in connector-starrocks's StarRocksStreamLoadVisitor.java and its two test files, which this PR has never touched; it's a phantom conflict from this branch's dev-sync history being stale, not anything in CoordinatorService.java, JobHistoryService.java, or the new test file. I'll fold a dev merge into the same push that addresses F2-F4 — should be a mechanical resolution (take dev's side entirely on the StarRocks files) with nothing to reconcile in this PR's own diff.

Will push once F1 (done)/F2/F3/F4 plus the dev-sync are in, then ask for a fresh pass and share the resulting Build result on the actual fork run.

DanielLeens

Re-verification Round — Head Unchanged for 9 Days

Re-checking my own PR again today. headRefOid is still c58e1842147a7e16191cc4a0cc3dea368f3ca2ac — the same commit I reviewed in full on 2026-08-23 and re-verified on 2026-08-24. No new commits have landed since then, and no other reviewer has posted since @SEZ9 on 2026-08-21. So this is not a fresh code review of new lines; it is an independent re-verification of the two things that were still open at the last round (CI, and the exact current diff state of the outstanding follow-ups), done directly against the source tree rather than by trusting my prior write-up.

What Problem Does This PR Solve?

Unchanged: JobHistoryService registers three cluster-wide Hazelcast expiration listeners in its constructor; CoordinatorService builds a fresh instance on every active-master transition. Before this PR nothing ever deregistered the outgoing instance's listeners, so every failover/rolling-restart cycle leaked a full listener set and duplicated CleanLogOperation fan-out plus cleanup counters. The fix captures the three registration ids and deregisters them via JobHistoryService.close(), invoked from CoordinatorService.clearCoordinatorService().

Independent Source Re-check of Every Open Item

I re-read the current files directly (not the diff, not my prior summaries) to confirm nothing silently regressed or got fixed without a commit showing up:

F1 (shutdown/re-activation race) — still fixed. CoordinatorService.java:1180 still captures JobHistoryService closingJobHistoryService = jobHistoryService; immediately after the CAS guard, and closes that local at :1215-1216, not the field. Unchanged from the c58e18421 commit I verified on 2026-08-23.
F2 (no closed marker on a stale field) — still open, still not a live bug. CoordinatorService.java:529 still does an unconditional jobHistoryService = new JobHistoryService(...) on every re-activation, no reuse branch exists, so this remains a documentation/enforcement gap rather than an active defect.
F3 (partial-construction leak) — still open. JobHistoryService.java:122/128/134 still declare the three registration ids as private final UUID, assigned inline in the constructor with no try/catch around the three addEntryListener calls.
F4 (test try/finally hygiene) — still open, confirmed by direct read of the current test file. In JobHistoryServiceListenerCleanupTest.java, testCloseRemovesFinishedJobEntryListeners() (:66-101) still registers liveService and the three loop-created JobHistoryService instances on the shared test node's cluster-wide IMaps with no try/finally. An assertion failure between creation and the manual removeEntryListener cleanup (e.g. the assertEquals(3, ...) size checks) still leaks live registrations into every subsequent test in the same JVM. testClearCoordinatorServiceDeregistersJobHistoryListeners() right below it continues to do this correctly with finally { coordinatorInstance.shutdown(); } — the first test still hasn't been brought in line with its own sibling.
F5-F8 (log-level differentiation, AutoCloseable, @VisibleForTesting, raw test base class) — all still open, confirmed via direct grep: JobHistoryService still declares public class JobHistoryService { (:69) with no implements AutoCloseable, and getEntryListenerRegistrationIds() (:202) still has no @VisibleForTesting annotation.

None of this is new — it's the same state I described on 2026-08-24 — but I'm not willing to just assert "still open" from memory on a financial-grade coordinator fix; every line above was re-read at the current head today.

What's Actually New Since My Last Round

1. The merge-conflict gap against dev has gotten measurably worse, not better. On 2026-08-23 I measured dev...c58e1842 as ahead_by: 5, behind_by: 68, status: diverged. Today the same comparison is ahead_by: 5, behind_by: 123, status: diverged — this branch has fallen behind by another 55 commits in a week while sitting unrebased. mergeStateStatus is still DIRTY / mergeable: CONFLICTING. This was already flagged as Issue 8 (a real, git merge-tree-confirmed textual conflict in clearCoordinatorService()) and it is the actual blocking action item at this point — every day this sits unrebased makes the eventual conflict resolution larger, not smaller.

2. CI had not been rerun in over a week despite my own 2026-08-24 recommendation to do so. I pulled the fork run directly (DanielLeens/seatunnel run 32581538612, the run for this exact head) rather than trusting the apache-side pointer check: the only real failure is unit-test (11, windows-latest) (job 97056746065), which fails in JobStateCleanupDelayTest with IllegalStateException: Node failed to start! at the ~311s mark — an unrelated, pre-existing test this PR does not touch, matching the same Windows Hazelcast-startup-flake signature documented in prior rounds. The other three unit-test legs were cancelled by matrix fail-fast, not failed on their own merits. This run had simply been left in its original 2026-08-22 failed state with nobody re-triggering it. As the PR author, I've now triggered gh run rerun 32581538612 --repo DanielLeens/seatunnel --failed, which re-queued all four unit-test legs (confirmed in_progress as of this comment). I'm not claiming a green result here since the rerun is still running — just recording that a stale, unactioned red Build is no longer sitting unaddressed.

Compatibility, Architecture, Historical-Version Compatibility

Unchanged from every prior round: no Option/config/public-API/serialized-format change; JobHistoryService is not Serializable, the UUID fields are pure in-memory bookkeeping never touched by checkpoint/savepoint; the normal single-master steady-state path never reaches close() (only master-role loss, init failure, or shutdown do). Fully backward compatible in both upgrade and downgrade directions.

Issue Summary (carried forward, re-verified against current head today)

#	Issue	Location	Severity	Status
F1	Shutdown-race on `clearCoordinatorService()` vs. concurrent `initCoordinatorService()`	`CoordinatorService.java:1180`	—	Fixed (re-confirmed)
F2	Stale `jobHistoryService` field can point at a closed instance; no `closed` marker	`CoordinatorService.java:140, 529`	Low	Open (not a live bug)
F3	Partial constructor failure can orphan 1-2 listener registrations with no owner	`JobHistoryService.java:122-163`	Low	Open
F4	`testCloseRemovesFinishedJobEntryListeners` still lacks try/finally; assertion failure can leak live listeners into later tests	`JobHistoryServiceListenerCleanupTest.java:66-101`	Medium (test hygiene)	Open
F5-F8	Log-level differentiation, `AutoCloseable`, `@VisibleForTesting`, raw test base class	`JobHistoryService.java`, `JobHistoryServiceListenerCleanupTest.java`	Low	Open
F9 (new)	Branch has diverged 123 commits behind `dev` with a confirmed textual conflict in `clearCoordinatorService()`; gap growing week over week	`CoordinatorService.java` (merge point)	Medium — blocks merge mechanically regardless of CI/code state	Open

Merge Recommendation

Conclusion: Ready to merge after fixes

Blockers — must be fixed
- Rebase onto current dev and resolve the textual conflict in clearCoordinatorService() (F9). This is now the single most urgent item: the gap has nearly doubled in a week and will only get more painful to resolve the longer it's deferred.
- Get a clean CI signal on a fresh run. I've just triggered a rerun of the failed/cancelled unit-test legs on the fork; per project policy this PR cannot merge on unverified/red CI regardless of the (environmental) cause.
- F4: wrap the liveService and loop-created JobHistoryService registrations in testCloseRemovesFinishedJobEntryListeners in try/finally, matching the pattern the sibling test in the same file already uses, so a failed assertion can't leak live listeners into the rest of the suite.
Recommended fixes — non-blocking
- F2, F3, F5-F8: the previously-agreed low-cost hardening items (closed-instance marker, constructor partial-failure cleanup, AutoCloseable, @VisibleForTesting, richer removal-failure logging). Fine to batch into one follow-up commit alongside F4.

Overall: the production fix itself remains correct and unchanged since 2026-08-22, and I have no new correctness objection to it. The only thing that changed materially this week is procedural, not architectural — the branch has drifted further from dev and the CI run had gone stale without anyone re-triggering it. Neither is a code-quality problem, but both are real, growing blockers to actually landing this, and I'd rather flag that plainly than post another near-duplicate full code walkthrough when the diff itself hasn't moved.

# Conflicts: # seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/CoordinatorService.java

DanielLeens · 2026-09-09T07:07:00Z

Addressed the remaining review items in ab1538a9c1241741940cc789e88fb79bf18df608:

F3: failed construction rolls back already-acquired listener registrations and rethrows the original failure. Tests cover failure at each of the three registrations and a removal failure during rollback.
F4: test-owned services now use try-with-resources, including the positive control and repeated create/close cases.
F5-F8: expected stopped-instance exceptions use fine logging, unexpected removal failures remain warnings with registration IDs; AutoCloseable, @VisibleForTesting, and the test-base type parameter are added.
F2: I retained the history-service reference and made the fresh-instance invariant explicit at both the getter and initialization site. This corrects my earlier proposed nulling: existing callers use the getter/read methods without a null fallback, and closing only deregisters listeners. Nulling the field would unnecessarily change that contract. A regression test verifies history reads still work after repeated close. Every activation constructs a new instance.

The independent maintainer source review and module-scoped Spotless checks passed. GitHub confirms the new head and the four-file scope; there is no merge conflict.

Current-head CI update: code style, all four unit-test jobs (JDK 8/11 on Linux/Windows), and both engine integration-test jobs passed. The full Build is not green: the JDK 8 transform job failed in Flink TestFilterRowKindIT.testFilterRowKindMultiTable, with row num :87 violating MIN_ROW=100 and 125/150 violating MAX_ROW=100. This matches tracked #12116; the existing fix is #12216. Its underlying fork workflow already has a queued run, so I did not duplicate the retry despite the Apache-side check still showing the old cancellation. Other jobs on this PR remain active.

I also examined the previous head's failed jobs rather than treating all red jobs as flakes:

Both RocketMQ JDK 8 and JDK 11 logs contain missing consume offsets for [test_topic, broker-a, 0]; the JDK 11 log also explicitly reports The broker[broker-a] not exist. These match the shared test-container problem already addressed by open [Test][E2E] Start the RocketMQ broker with a single identity and wait for name-server routes #12115.
Engine JDK 8 failed in SinkErrorToMysqlIT.testSinkMaxErrorRatioThreshold because the post-job thread check still saw HikariPool-3 housekeeper and st-multi-table-sink-writer-1 after two minutes. This is a resource-cleanup signal, not evidence that increasing the timeout or weakening the thread assertion is appropriate.

The old failed jobs are superseded by this push; I did not queue redundant retries on the old SHA. The new Build run is in progress and must establish the current CI result. No unrelated RocketMQ or sink-error production changes are included in this listener-lifecycle PR.

The engine failure prompted a separate dev-source teardown audit. On bigdata3, I reproduced unchecked lifecycle-close exceptions skipping later resource cleanup, filed as #12231 with the full harness. This proves a separate cleanup bug, but does not by itself establish the root cause of that exact SinkErrorToMysqlIT run; the issue explicitly preserves that distinction.

DanielLeens

Follow-up on my last comment (2026-09-09): the previously-cited CI blocker is now resolved. The fork Build run for this exact head (ab1538a) — https://github.com/DanielLeens/seatunnel/actions/runs/34322099296 — has completed with a SUCCESS conclusion, closing out the JDK 8 transform flake (#12116, fixed by #12216) that was the only outstanding item at last check. All prior review findings (F2-F9) were already confirmed fixed on this head, including the merge-conflict item. No blocker remains from my side; this looks ready for an independent maintainer to give the write-capable review and merge.

SEZ9 · 2026-09-12T03:26:29Z

Thanks for the update. Good to see the fork Build run for head ab1538a9 (run 34322099296) reported as successful.

Before a final pass, I'd like to close the loop on my earlier findings. The note above refers to an F2-F9 range, while the set I raised was F1-F8, so I don't want to assume which items were addressed. Could you confirm how each of these stands on ab1538a9:

F1 / F2: does clearCoordinatorService() now close the specific JobHistoryService instance it owns rather than whatever the mutable field currently points to, and is the closed instance no longer left referenced by the field (or is every re-activation path guaranteed to construct a new instance)?
F3: if the second or third addEntryListener in the JobHistoryService constructor throws, are the earlier registrations removed before the exception propagates?
F5: does removeEntryListenerQuietly now distinguish non-shutdown removal failures instead of swallowing everything at a single warning level?
F4 / F8: are the listener registrations in the new cleanup test wrapped in try/finally so an assertion failure doesn't leak listeners into the shared test node, and is AbstractSeaTunnelServerTest extended with its type parameter instead of as a raw type?
F6 / F7: does JobHistoryService implement AutoCloseable, and is the test-only accessor getEntryListenerRegistrationIds() marked as visible for testing?

If all of these are already in ab1538a9, a short "done" list per item is enough and I'll re-review. If any are intentionally left as-is, please say which and why so we can settle it here.

DanielLeens · 2026-09-12T09:44:07Z

Thanks for catching the numbering gap — that's on me, not a discrepancy in the fix itself. My 09-09 write-up covered F2 through F8 and dropped F1 off the list by mistake; it's addressed in the same commit. I went back through ab1538a9 line-by-line just now to confirm each item against current source rather than re-asserting from the commit message:

F1: Fixed. clearCoordinatorService() captures the generation-owned instance into a local before any shutdown waits run (JobHistoryService closingJobHistoryService = jobHistoryService;, CoordinatorService.java:1304) and calls .close() on that local, not on whatever the field currently holds (:1352-1353). initCoordinatorService() still unconditionally constructs a fresh instance on every activation with no reuse branch (:632-646), so a re-activation racing shutdown can never have its listeners closed by the outgoing generation's cleanup.
F2: The field is deliberately left non-null after close, same as before, so existing read-path callers keep working without a null-check contract change. Since F1 means close() never touches whatever the field points to at call time, and every activation is a guaranteed fresh construction, the "closed instance still reachable via the field" risk no longer has a path to actually matter in the current code.
F3: Fixed. The constructor wraps the three addEntryListener calls in try/catch; on a RuntimeException from the 2nd or 3rd call it removes whatever was already registered via removeEntryListenerQuietly and rethrows the original exception (JobHistoryService.java:162-185).
F4 / F8: Fixed. testCloseRemovesFinishedJobEntryListeners now uses try-with-resources for both the positive-control instance and each loop-created instance (JobHistoryServiceListenerCleanupTest.java:78, :90); the class extends AbstractSeaTunnelServerTest<JobHistoryServiceListenerCleanupTest>, not the raw type (:58-59).
F5: Fixed. removeEntryListenerQuietly now catches HazelcastInstanceNotActiveException separately at fine for the expected-shutdown case and falls back to warning with the registration id for anything else (JobHistoryService.java:480-493).
F6 / F7: Fixed. JobHistoryService implements AutoCloseable (:71); getEntryListenerRegistrationIds() carries @VisibleForTesting (:226-227).

All eight are in on ab1538a9, verified against source just now rather than carried forward from the commit message. Nothing left open on my side — over to you for the re-review.

DanielLeens · 2026-09-12T09:57:58Z

Confirming per item against ab1538a9 (ab1538a9c1241741940cc789e88fb79bf18df608), with the code locations so you can check each one directly:

F1 / F2: clearCoordinatorService() now captures JobHistoryService closingJobHistoryService = jobHistoryService; at the top of teardown (CoordinatorService.java, right after the coordinatorServiceCleared CAS and before pendingJobScheduleEpoch.incrementAndGet()), and calls closingJobHistoryService.close() after the executor-termination wait. So it closes the instance owned by the coordinator generation being cleared, not whatever the mutable field points to at that moment. The field is deliberately not nulled: close() only deregisters the listener registrations, read methods keep working for callers already holding the reference, and initCoordinatorService() unconditionally constructs a new JobHistoryService on every activation. Both invariants are stated in comments at the getter and at the initialization site. JobHistoryServiceListenerCleanupTest#testClearCoordinatorServiceDeregistersJobHistoryListeners covers the production path on an isolated Hazelcast instance.
F3: yes. The constructor registers the three listeners inside a try block; on RuntimeException it calls removeEntryListenerQuietly for the state-map and metrics-map registrations already acquired (null-safe, so a failure on the first registration removes nothing) and rethrows the original exception. JobHistoryServiceRegistrationTest is parameterized over the failing registration index (each of the three), and separately verifies that a removal failure during rollback does not stop rollback of the remaining registration and that the caller still receives the original registration failure.
F5: yes. removeEntryListenerQuietly now has two catch blocks: HazelcastInstanceNotActiveException at fine (expected while the node is stopping), and any other exception at warning with the registration id and store name. It no longer swallows everything at a single level.
F4 / F8: in JobHistoryServiceListenerCleanupTest#testCloseRemovesFinishedJobEntryListeners every writer-created service is in try-with-resources (the positive control and each of the repeated create/close iterations), and testClearCoordinatorServiceDeregistersJobHistoryListeners shuts down its isolated Hazelcast instance in finally, so an assertion failure cannot leak listeners into the shared test node. The class is declared extends AbstractSeaTunnelServerTest<JobHistoryServiceListenerCleanupTest>, not the raw type.
F6 / F7: JobHistoryService implements AutoCloseable with @Override public void close(), and getEntryListenerRegistrationIds() is package-private and annotated @VisibleForTesting.

The "F2-F9" wording in my previous note was a typo on my side. The set is F1-F8 exactly as you raised it, all eight are in ab1538a9, and nothing is intentionally left as-is.

SEZ9 · 2026-09-13T03:38:37Z

Thanks for the detailed per-item walkthrough against ab1538a9 — that makes the re-review much easier.

I'll verify each of F1–F8 directly against the changed files at ab1538a9c1241741940cc789e88fb79bf18df608 rather than marking anything resolved from the description alone, and will follow up once I've been through the diff.

One small note: your comment appears to have been cut off at "The class is declared extends Ab…", so the rest of the F8 response (and anything on F6/F7) didn't come through. No need to repost — I'll check those points in the code.

DanielLeens · 2026-09-13T10:52:03Z

Thanks for flagging it — no need to repost on my end either, since I pulled the comment straight from the API rather than the rendered page: issues/comments/5645188186 is 2955 bytes and ends cleanly with "The set is F1-F8 exactly as you raised it, all eight are in ab1538a9, and nothing is intentionally left as-is." Nothing is missing from the underlying comment; this looks like the same rendering artifact we ran into on 2026-08-25 with an earlier reply of mine (backtick-wrapped generic type like `AbstractSeaTunnelServerTest<JobHistoryServiceListenerCleanupTest>` can get visually cut at the < in some GitHub render paths even though the stored body is intact).

For the record, in case it's easier to read here than to re-fetch the raw comment, the F8 confirmation plus F6/F7 that follow that line are:

F8: the test class is declared extends AbstractSeaTunnelServerTest parameterized with itself (i.e. <JobHistoryServiceListenerCleanupTest>), not the raw type.
F6 / F7: JobHistoryService now declares implements AutoCloseable with @Override public void close(), and getEntryListenerRegistrationIds() is package-private and annotated @VisibleForTesting.

CI on this head (ab1538a9) is green — Build passed on the fork run, matching what I reported after the 2026-09-09 push.

Take your time going through F1-F8 against the diff directly; I won't push anything further on this head while your re-review is in progress.

SEZ9 · 2026-09-14T04:39:16Z

Thanks for the follow-up — the rendering explanation makes sense, and the F6/F7/F8 summaries you repeated here are what I was looking for: JobHistoryService implementing AutoCloseable with an overriding close(), getEntryListenerRegistrationIds() package-private and marked as visible for testing, and the test class parameterized with itself rather than extending AbstractSeaTunnelServerTest as a raw type. I'll confirm those against the ab1538a9 diff directly, but on paper F6–F8 look resolved.

For the remaining points I only have the statement that they are all in ab1538a9, without the specifics, so a short note on each would speed up my re-review (otherwise I'll dig them out of the diff myself):

F1 — how clearCoordinatorService() now avoids closing a JobHistoryService instance that a concurrent re-activation just created.
F2 — whether the field is cleared (or otherwise made unusable) after close() so a stale, closed JobHistoryService cannot be picked up by a re-activation path that does not construct a fresh one.
F3 — how the JobHistoryService constructor cleans up already-registered listeners if a later addEntryListener call throws.
F4 — whether testCloseRemovesFinishedJobEntryListeners now unregisters its listeners in a try/finally so an assertion failure doesn't leak listeners into other tests sharing the node.
F5 — what removeEntryListenerQuietly does now for non-shutdown failures versus the expected shutdown-time exceptions.

No need to push anything else until I've finished going through F1–F8 on ab1538a9; if the five items above are covered, I expect this to be ready to approve.

DanielLeens · 2026-09-14T09:16:26Z

Specifics on F1-F5, all against ab1538a9:

F1 — clearCoordinatorService() vs. a concurrent re-activation. CoordinatorService.java:1304: the method captures the field into a local, JobHistoryService closingJobHistoryService = jobHistoryService;, before the executorService.awaitTermination(20, TimeUnit.SECONDS) wait at line 1335 (which is the window where a concurrent re-activation could run). The actual .close() call at line 1352-1353 is made on that captured local, not on the field. So if a concurrent initCoordinatorService() reassigns the jobHistoryService field to a freshly-constructed instance while this method is still waiting on awaitTermination, the close at the end of clearCoordinatorService() still targets the old generation's instance it snapshotted, never the new one.

F2 — field left non-null after close, and why that's safe. Confirmed: the jobHistoryService field is deliberately not nulled — the comment directly above the close call (line 1350-1351) says "The instance itself is kept because read paths may still use it." This is safe because there is exactly one assignment site for the field in the whole class — initCoordinatorService() at line 632-633, which every re-activation calls and which unconditionally constructs a brand-new JobHistoryService with its own comment ("Never reuse the previous history view"). There is no re-activation path that reuses the field instead of constructing fresh, so a stale-but-closed instance can only ever be reached by requests that already held a reference from before the close — which is exactly the intended "reads still work" behavior, not a re-activation path.

F3 — constructor rollback on a later addEntryListener failure. JobHistoryService.java:159-185: the three addEntryListener calls (state, metrics, DAG info, in that order) assign into local variables first, and a catch (RuntimeException e) around all three calls unwinds via removeEntryListenerQuietly for the state and metrics ids before rethrowing. Because removeEntryListenerQuietly no-ops on a null id, this correctly handles every failure point: if the 2nd call throws, only the 1st (state) registration gets rolled back; if the 3rd (DAG info) call throws, both the 1st and 2nd are rolled back. A failed constructor never publishes the instance, so nothing else can observe the partially-rolled-back state in between.

F4 — assertion-failure safety in testCloseRemovesFinishedJobEntryListeners. JobHistoryServiceListenerCleanupTest.java:78 and :90: both blocks in this test use Java try-with-resources (try (JobHistoryService x = newJobHistoryService())) rather than a bare local + explicit finally. Since JobHistoryService implements AutoCloseable, the JVM guarantees close() runs during exit of each try block on any exception path, AssertionError included, without needing an explicit try/finally — so a failed Assertions.assertX inside either block still deregisters that iteration's listeners before propagating.

F5 — removeEntryListenerQuietly failure handling. JobHistoryService.java:475-494: HazelcastInstanceNotActiveException is caught separately and logged at .fine — treated as the expected case where the node is already shutting down and its event service owns the remaining cleanup. Any other Exception is caught and logged at .warning, i.e. treated as unexpected and worth surfacing, but still swallowed so listener cleanup can never break the master-switch or shutdown flow itself.

🤖 Generated with Claude Code

SEZ9 · 2026-09-23T04:32:26Z

Thanks for the detailed walkthrough against ab1538a9.

F1 / F2 — The reasoning makes sense: closing the captured local rather than the field in clearCoordinatorService(), and having a single assignment site in initCoordinatorService() that always constructs a fresh instance, addresses both points. I'll confirm against the diff before marking them resolved. One small ask for F1: since the safety argument rests on "close the snapshot, never the field", a one-line comment at the snapshot site would help keep a future refactor from collapsing it back into a direct field access.

F3 — Your comment appears to be cut off right after the JobHistoryService.java:159-185 reference, so I can't evaluate this one yet. Could you post the rest? Specifically: if the second or third addEntryListener throws, does the constructor now remove the registrations made so far before rethrowing?

F4 / F5 — The heading mentions F1-F5, but F4 and F5 aren't in the visible text. Could you re-post them? For F4, whether the test now wraps listener registration in try/finally so an assertion failure doesn't leak live listeners into the shared node; for F5, whether removeEntryListenerQuietly now distinguishes shutdown-related failures from other removal failures, or at least logs them at a level that would surface a re-created leak.

F6-F8 (AutoCloseable, the VisibleForTesting annotation on getEntryListenerRegistrationIds(), raw AbstractSeaTunnelServerTest) — Not covered yet. A quick note on whether they're addressed in ab1538a9 or deferred is enough.

Once F3-F5 are visible and F6-F8 have a status, I'll do a final pass.

DanielLeens

Thanks for the ping, @SEZ9, and thanks for your patience while this sat for a while between rounds. Since this is my own PR, I re-did a full, independent pass on the current head rather than just restating my last comment — the last commit here is a merge of dev (82 unrelated commits pulled in, resolving a stale merge state), and the production/test diff against my previous review is byte-for-byte unchanged, but I re-read every changed file end to end anyway before answering you.

One thing first: your note that my last F1-F5 comment "appears to be cut off" — I pulled that comment straight from the API rather than the rendered page, and it's 4882 bytes, ending cleanly on the F5 sentence. Nothing is missing from the stored comment; this is the same GitHub rendering artifact we've hit twice before on this thread with backtick-wrapped generic types (e.g. AbstractSeaTunnelServerTest<...>). No need to worry you're missing content on your end — I've laid out F1-F8 fully below anyway so there's nothing left to dig out of an old comment.

What Problem Does This PR Solve?

User pain point: JobHistoryService registers three cluster-wide Hazelcast EntryExpiredListeners in its constructor, and CoordinatorService constructs a brand-new JobHistoryService every time a node becomes the active master. Before this fix, nothing ever deregistered the outgoing instance's listeners, so every failover, split-brain recovery, or rolling restart leaked three listener registrations, kept the old service instance reachable from the IMap listener registry (the listener classes are non-static inner classes), and caused a real finished-job expiration event to fire once per leaked instance — duplicating CleanLogOperation RPC fan-out and cleanup counters cluster-wide.
Fix approach: capture the UUID returned by each addEntryListener call, add a best-effort JobHistoryService.close() that deregisters exactly those three registrations, and call it from CoordinatorService.clearCoordinatorService() — the single method already used both on master-role loss and on full shutdown. The coordinator snapshots the instance it owns before any shutdown wait, so a concurrent re-activation during node shutdown can never have its listeners closed by the outgoing generation's cleanup.
One-sentence summary: this closes a listener/heap leak and a duplicated-side-effect bug on Zeta master-role transitions by tracking and explicitly deregistering the three finished-job listeners when the coordinator is cleared.

1. Code Change Review

1.1 Core Logic Analysis

Before:

this.finishedJobStateImap.addEntryListener(
        new FinishedJobExpiredListener<>(Constant.IMAP_FINISHED_JOB_STATE), true);
this.finishedJobMetricsImap.addEntryListener(
        new FinishedJobExpiredListener<>(Constant.IMAP_FINISHED_JOB_METRICS), true);
this.finishedJobDAGInfoImap.addEntryListener(
        new JobInfoExpiredListener(Constant.IMAP_FINISHED_JOB_VERTEX_INFO), true);

Registration ids discarded, no removal path anywhere in the class or its caller.

After (JobHistoryService.java, constructor + close()):

try {
    stateListenerId = this.finishedJobStateImap.addEntryListener(...);
    metricsListenerId = this.finishedJobMetricsImap.addEntryListener(...);
    dagInfoListenerId = this.finishedJobDAGInfoImap.addEntryListener(...);
} catch (RuntimeException e) {
    // a failed constructor is never published, so release whatever was already acquired
    removeEntryListenerQuietly(finishedJobStateImap, stateListenerId, ...);
    removeEntryListenerQuietly(finishedJobMetricsImap, metricsListenerId, ...);
    throw e;
}
...
@Override
public void close() {
    removeEntryListenerQuietly(finishedJobStateImap, finishedJobStateListenerId, ...);
    removeEntryListenerQuietly(finishedJobMetricsImap, finishedJobMetricsListenerId, ...);
    removeEntryListenerQuietly(finishedJobDAGInfoImap, finishedJobDAGInfoListenerId, ...);
}

and in CoordinatorService.clearCoordinatorService():

JobHistoryService closingJobHistoryService = jobHistoryService; // snapshot before any wait
...
executorService.awaitTermination(20, TimeUnit.SECONDS);
...
if (closingJobHistoryService != null) {
    closingJobHistoryService.close();
}

Runtime path, verified directly against the current head:

Case A: node leaves active master role
  checkNewActiveMaster() -> clearCoordinatorService() [synchronized]
    CAS(false->true) guard
    closingJobHistoryService = jobHistoryService   <-- snapshot, BEFORE the wait below (this is the fix for the shutdown race raised earlier in this thread)
    executorService.awaitTermination(20s)
    closingJobHistoryService.close()               <-- THE FIX, acts on the snapshot, never on the live field
       -> removeEntryListenerQuietly x3
    resourceManager.close(); eventProcessor.close()
  (jobHistoryService field left non-null: in-flight readers keep working)

Case B: node becomes active master
  initCoordinatorService()
    coordinatorServiceCleared.set(false)           <-- re-arms the guard for the next Case A
    jobHistoryService = new JobHistoryService(...) <-- always a fresh instance, 3 new ids

Case C: init throws -> clearCoordinatorService() still runs (guard was reset in Case B's first line)
Case D: shutdown() -> masterActiveListener.shutdown() (non-blocking) -> clearCoordinatorService()

The normal submit -> run -> finish job lifecycle never touches clearCoordinatorService(); the new cleanup fires only on role loss, init failure, and shutdown, which matches the PR's own scope.

Key findings:

The normal path does not reach the new code — and correctly so, since tearing down the master's history listeners on every job completion would be wrong. It's reached only on the transition/shutdown paths described above.
This is a precise, root-cause fix: it restores the missing register/deregister symmetry at the one production construction site and the one production teardown site, with no config, no new abstraction.
The implementation has been hardened across several review rounds on this same PR (shutdown-race snapshot, partial-construction rollback, log-level distinction, AutoCloseable) — I found no remaining hole on this pass.

Answering @SEZ9's F1-F8 checklist directly against the current head (all confirmed with source evidence, not carried over from memory):

Item	Status	Where
F1 — `clearCoordinatorService()` must not close a newer generation's listeners	Resolved	it captures `closingJobHistoryService = jobHistoryService` immediately after the CAS guard, before the 20s `awaitTermination` wait, and closes only that local
F2 — a closed instance must not be silently reusable	Resolved by invariant	`initCoordinatorService()` is the only assignment site for the field in the whole class and always constructs a fresh instance; there is no reuse branch anywhere, so a closed-but-non-null field is never picked back up by a re-activation
F3 — constructor must roll back partial registrations before rethrowing	Resolved	the three `addEntryListener` calls are wrapped in `try`, and `catch (RuntimeException e)` removes whatever was already acquired before rethrowing the original failure
F4 — the new test must not leak listeners into the shared node on assertion failure	Resolved	every service the test creates is opened in try-with-resources, which is sufficient since the class now implements `AutoCloseable`
F5 — distinguish expected shutdown failures from genuine ones	Resolved	`removeEntryListenerQuietly` has a dedicated `catch (HazelcastInstanceNotActiveException e)` at fine level, and a separate `catch (Exception e)` at warning level with the registration id
F6 — `AutoCloseable`	Resolved	`JobHistoryService implements AutoCloseable`
F7 — `@VisibleForTesting`	Resolved	on `getEntryListenerRegistrationIds()`
F8 — parameterized test base	Resolved	`extends AbstractSeaTunnelServerTest<JobHistoryServiceListenerCleanupTest>`, not the raw type

I found no ninth item and no regression in any of the eight while re-reading the source just now.

1.2 Compatibility Impact

Fully compatible. No Option added, removed, or renamed. close() and the test-only accessor are purely additive; no existing method signature or visibility changed. JobHistoryService is not Serializable, and the classes that are actually serialized (JobState, PipelineStateData) are untouched — no checkpoint, savepoint, or IMap payload format is affected. A rolling upgrade through this fix is safe in both directions: downgrading simply restores the pre-fix leak, with no data-format break.

1.3 Performance / Side-Effect Analysis

Net positive: listener count stays bounded at three regardless of how many master transitions occur, and duplicated CleanLogOperation RPC fan-out plus duplicated cleanup counters stop after the first post-fix master switch. close() adds up to three synchronous IMap.removeEntryListener calls inside the already-blocking synchronized clearCoordinatorService() method, but that method already blocks on a 20-second awaitTermination and on resourceManager.close() in the same critical section, so this doesn't introduce a new class of risk — just three more calls of the same shape.

1.4 Error Handling and Logging

Correct and appropriately defensive. removeEntryListenerQuietly never lets a listener-cleanup failure escape and break the master-switch or shutdown flow, and it now distinguishes the expected "node is already stopping" case (fine level) from a genuinely unexpected removal failure (warning level, with the registration id for diagnosability). No sensitive data is logged.

No blocking issues found in this round.

2. Code Quality Assessment

2.1 Coding Standards

License headers present and correct on both new test files. Every new field and method carries a multi-line Javadoc explaining purpose and lifecycle risk, not just restating the type — including the invariant comments at getJobHistoryService() and at the jobHistoryService = new JobHistoryService(...) assignment site that make the "always fresh, never reused" contract explicit for future readers. No wildcard imports, no System.out.println, no single-line inline Javadoc.

2.2 Test Coverage and Test Stability

Two test files cover this at two different levels:

JobHistoryServiceListenerCleanupTest (Hazelcast-integration level) verifies the actual listener registry state via IMap.removeEntryListener's boolean return — a positive control proves the exposed ids are real live registrations before the negative assertions rely on that being true, and the production entry point clearCoordinatorService() is exercised directly on an isolated node, not just close() in isolation.
JobHistoryServiceRegistrationTest (pure unit level, Mockito-based) adds deterministic coverage for every constructor-failure position, for the case where the rollback removal itself fails, for the fine/warning log-level split, and for read-availability after close. These aren't hollow "verify a mock was called" tests — each one pins down a specific, code-path-dependent outcome (which map got rolled back, which exception surfaces, which log level fires) that a plausible regression would actually break.

Stability rating: Stable. No flaky-test anti-patterns in either file — no Thread.sleep or fixed-delay waits (the integration test uses Awaitility.await().ignoreExceptions().atMost(30, SECONDS).untilAsserted(...)), no shared static state, no inter-test ordering dependency, no unclosed resources (try-with-resources and finally { shutdown() } throughout), and the isolated Hazelcast node is built with an allocated port range plus a raised join-port-try-count, so it won't collide with other engine-server tests running in the same CI job.

2.3 Documentation Updates

None required and none made — correct. This is an internal engine lifecycle fix with no user-facing option, default, or contract change, so no docs/en or docs/zh update is warranted.

3. Architectural Soundness

3.1 Elegance of the Solution

Classification: Precise fix. It restores the missing register/deregister symmetry at exactly the one construction site and one teardown site, without introducing new abstractions or configuration.

3.2 Maintainability

Good. close() sits alongside the pre-existing resourceManager/eventProcessor teardown idiom in clearCoordinatorService(), and the snapshot-before-close pattern for jobHistoryService now mirrors the capture-and-clear approach already used for those neighboring fields, so a future reader sees one consistent idiom for "how do we tear down a coordinator-owned resource" rather than a one-off.

3.3 Extensibility

The three (IMap, UUID, storeName) triples are three parallel fields rather than one collection. That's fine at the current fixed count of three; it would only be worth generalizing if a fourth cluster-wide listener is ever added to this service.

3.4 Historical-Version Compatibility

No impact. Registration ids are ephemeral in-memory state, never persisted, never part of a checkpoint or savepoint payload, and never shipped across the wire. A cluster performing a rolling upgrade through this fix behaves identically from the outside, and downgrading is equally safe.

4. Issue Summary

No blocking issues were found in this round. All issues raised across earlier rounds of this PR (constructor rollback, shutdown-race snapshot, log-level distinction, test port isolation, AutoCloseable, @VisibleForTesting) have been fixed and are independently re-verified above.

#	Issue	Location	Severity
—	None outstanding	—	—

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Blockers — must be fixed
- CI is not currently green on this head. I traced each failing job individually on the fork run rather than treating "Build failed" as one signal:
  - Dependency licenses hit the job's 40-minute step timeout mid-way through shading, with no Maven error before the timeout — infra slowness.
  - all-connectors-it-2 (8, ubuntu-latest) failed resolving org.postgresql:postgresql:jar:42.4.3 from Maven Central with a connection reset — a dependency-resolution network flake, before any test ran.
  - engine-v2-it (11, ubuntu-latest) failed in CheckpointCoordinatorFailoverIT, a test class that was added to dev independently of this PR and that exercises CheckpointCoordinator, which this PR does not touch.
  - engine-v2-it (8, ubuntu-latest) failed in BackpressureSlowSinkIT on a timing condition that matches an already-tracked, pre-existing engine flake, also unrelated to this diff.
- None of these four failures touch the files this PR changes. The only action needed is a job-level rerun of these four legs — no source change is required. Per this project's standing policy, an unverified or failing CI still gates merge regardless of the cause, so I'm not calling this "Ready to merge" outright until that rerun comes back clean.
Recommended fixes — non-blocking
- None. Every previously open item (F1-F8) is closed as of this head.

Overall assessment: on this ninth independent pass, the production fix remains correct, minimal, and complete — the shutdown-race snapshot, the constructor rollback, and the log-level split are all in place and each is now backed by a test that would actually catch a regression in that specific behavior. I don't have any new code-side concerns to raise. The one thing standing between this PR and merge is a clean CI run on four jobs that, on the evidence, have nothing to do with this diff. @SEZ9, if the table above answers everything you were waiting on, I'd read this as ready to go once CI comes back green — happy to rerun the affected jobs and post the result here.

SEZ9 · 2026-09-24T03:36:16Z

Thanks for the detailed write-up. If the production/test diff is indeed unchanged since the last round, the earlier findings still apply. Restating what is needed to move forward:

Blocking (MEDIUM)

F1 / F2 – CoordinatorService.clearCoordinatorService(): the description says the coordinator snapshots the instance it owns before any shutdown wait, but as reviewed the method closes whatever the mutable jobHistoryService field points to at that moment and leaves the closed instance referenced afterwards. Please either snapshot the field into a local, close that local, and null the field, or point me at the exact code that already does this so I can re-check. Also confirm that every re-activation path unconditionally constructs a fresh JobHistoryService (or, better, remove that dependency).
F3 – JobHistoryService constructor: if the second or third addEntryListener throws, the earlier registrations are orphaned and can never be removed by close(). Please wrap the registrations so a failure removes what was already registered before rethrowing.

Non-blocking (LOW), still requested

F4: wrap the listener registrations in testCloseRemovesFinishedJobEntryListeners in try/finally so an assertion failure does not leak listeners into other tests in the shared JVM.
F5: in removeEntryListenerQuietly, distinguish the expected shutdown case from genuine removal failures (log the latter at a higher level, or include enough context to tell them apart).
F6: have JobHistoryService implement AutoCloseable now that it has a close() lifecycle method.
F7: annotate getEntryListenerRegistrationIds() as VisibleForTesting.
F8: give AbstractSeaTunnelServerTest a type argument in the new test instead of using the raw type.

One housekeeping note: the comment above appears to end mid-sentence in the summary section, before the promised F1–F8 breakdown. If there were per-finding responses there, could you re-post them (or reply per point inline)? Otherwise, addressing each item directly in code is the fastest path. Once F1–F3 are fixed I'll do another pass.

DanielLeens · 2026-09-24T10:56:07Z

Thanks for the follow-up, @SEZ9.

Status check first: the current head (d7de2af) is a dev-sync merge commit on top of ab1538a, not a new production commit. I diffed it against the current merge-base with dev and the production/test diff is unchanged: same 4 files (CoordinatorService.java, JobHistoryService.java, JobHistoryServiceListenerCleanupTest.java, JobHistoryServiceRegistrationTest.java), same logic, just shifted a couple of lines from the dev sync. So everything below still applies to this head.

On "comment appears to end mid-sentence": I re-pulled my F1-F5 comment (id 5661717266) straight from the API and it is not truncated, it ends cleanly after the F5 paragraph. This is the third time we've hit this rendering artifact (2026-08-25, 2026-09-13, and now), and in the prior two cases it correlated with markdown links wrapping backtick code and line anchors. To rule that out for good, here is the same F1-F8 status again, in plain text, no markdown links this time, re-verified line-by-line against the current head just now:

F1 - done. CoordinatorService.clearCoordinatorService() snapshots the field into a local, "JobHistoryService closingJobHistoryService = jobHistoryService;", right after the CAS guard and before the executorService.awaitTermination(20s) wait. The close call further down is closingJobHistoryService.close(), made on that captured local, never on the field. So a concurrent initCoordinatorService() re-activation that reassigns the field mid-wait cannot have its fresh instance closed by this teardown; the snapshot pins cleanup to the outgoing generation.

F2 - done, confirmed safe without nulling. The field is deliberately left non-null after close (inline comment: "The instance itself is kept because read paths may still use it until a new active master creates a fresh one."). This is safe because there is exactly one assignment site for the field in the whole class, in initCoordinatorService(), guarded by its own comment ("Never reuse the previous history view") and it unconditionally constructs a brand-new JobHistoryService on every activation. No re-activation path ever reuses the field instead of constructing fresh, so a stale-but-closed instance is only reachable by callers who already held a reference from before the close, which is the intended "reads still work" behavior, not a re-activation hazard.

F3 - done. JobHistoryService's constructor assigns the three addEntryListener results (state, metrics, dagInfo) into local variables first, wraps all three calls in try/catch(RuntimeException), and on failure calls removeEntryListenerQuietly for whichever of the state/metrics ids were already acquired before the throw (null-safe, so a failure on the very first call removes nothing). It then rethrows the original exception. A failed constructor is never published to the coordinator, so nothing else can observe the partially-rolled-back state.

F4 - done. testCloseRemovesFinishedJobEntryListeners uses try-with-resources for both the positive-control instance and each loop-created instance. Since JobHistoryService implements AutoCloseable, close() runs on any exit path including an AssertionError, with no explicit try/finally needed.

F5 - done. removeEntryListenerQuietly now has two catch blocks: HazelcastInstanceNotActiveException logged at fine (expected, node already stopping), any other Exception logged at warning with the registration id and store name. It no longer swallows everything at one level.

F6 - done. JobHistoryService implements AutoCloseable, with an overriding public void close().

F7 - done. getEntryListenerRegistrationIds() is package-private and annotated @VisibleForTesting.

F8 - done. The test class is declared extends AbstractSeaTunnelServerTest parameterized with itself, not the raw type.

Re-verified all eight directly against the current head's source just now, not carried forward from memory or from the commit message. F1-F8 are all resolved, nothing open on my side. Over to you for the final pass, happy to answer anything that's still unclear, and I'll avoid markdown links in this thread going forward to rule that out as a cause.

🤖 Generated with Claude Code

SEZ9 · 2026-09-25T03:08:56Z

Thanks for the update. Understood that d7de2af is a dev-sync merge on top of ab1538a with the production/test diff unchanged, so the earlier points still apply to this head.

On the status for the points from comment 5661717266:

F1 (close a snapshot, not the mutable field): capturing the field into a local right after the CAS guard and calling close() on that local is the right shape. I'll confirm it against the diff on the final pass.
F2 (closed instance left in the field): the reasoning that the only assignment site is in initCoordinatorService() and always constructs a new JobHistoryService sounds fine; I'll verify it in the diff as well.
F3 (partial-construction listener leak): the status I can see ends at "assigns the three ad", so I can't tell what was done. Could you briefly restate how the constructor now handles a failure in the second or third addEntryListener (i.e. that earlier registrations are removed rather than orphaned)?
F4–F8: I don't see a status for these. Could you post a short line for each (try/finally around listener registration in the test, the swallowed-exception level in removeEntryListenerQuietly, AutoCloseable, the VisibleForTesting annotation on the test accessor, and the raw AbstractSeaTunnelServerTest type), even if the answer is "won't change" with a reason?

Once F3–F8 are covered I'll do a final pass on this head against the diff.

DanielLeens · 2026-09-26T12:26:35Z

Thanks for confirming F1 and F2 look right in shape — I'll wait for your check against the diff itself on those two rather than repeating them here.

On the repeated cut-off: I just re-pulled my last comment (id 5812736155) directly from the API rather than the rendered page. It is 4292 bytes and ends cleanly on the attribution line after the F8 sentence — nothing is missing from the stored body. This is now the fourth time this has happened on this thread (2026-08-25, 2026-09-13, 2026-09-24, and now), always on a numbered F-list that follows backtick-wrapped generic types or markdown links. So let me try the plainest possible format this time: no backticks around generics, no links, one short paragraph per item.

F3: The constructor assigns the three addEntryListener results, for the state map, the metrics map, and the DAG info map, into local variables first, wraps all three calls in one try block, and catches RuntimeException around them. On failure it calls removeEntryListenerQuietly for whichever of the state and metrics ids were already acquired before the throw. That helper is null-safe, so a failure on the very first call removes nothing extra. The original exception is then rethrown unchanged. A failed constructor is never published to the coordinator, so nothing downstream can ever observe a partially registered instance.

F4: testCloseRemovesFinishedJobEntryListeners now creates both the positive-control service and each loop-created service inside try-with-resources instead of a plain local variable. Since JobHistoryService now implements AutoCloseable, close runs on every exit path including an assertion failure inside the try block, so a separate try/finally is not needed once try-with-resources is used.

F5: removeEntryListenerQuietly now has two catch blocks instead of one. HazelcastInstanceNotActiveException is caught first and logged at fine level, since that's the expected case while the node is already stopping. Any other exception falls into a second catch and is logged at warning level together with the registration id and the IMap name, so a genuine removal failure is distinguishable from an expected shutdown exception in the logs.

F6: JobHistoryService now declares implements AutoCloseable and has an overriding public close method.

F7: getEntryListenerRegistrationIds is package-private and carries the VisibleForTesting annotation.

F8: The test class now extends AbstractSeaTunnelServerTest parameterized with itself, JobHistoryServiceListenerCleanupTest, instead of using the raw type.

I re-read all six of these directly against the current head's source just now rather than copying them from an earlier comment, so they reflect the code exactly as it stands on d7de2af.

If this still gets clipped on your side, let me know and I'll split F3 through F8 into six separate single-item comments instead — that would rule out any per-comment-length trigger for good.

…r-leak-11807

SEZ9 · 2026-09-28T04:44:56Z

Thanks for the plain-format follow-up on comment 5812736155.

F3 – partial-construction leak. The described shape matches what I was after: the three addEntryListener calls in one try block, ids held in locals, null-safe removeEntryListenerQuietly applied to the already-acquired state and metrics ids in the catch, and the original exception rethrown unchanged. I'll confirm against the diff.

F4 / F5 / F6 / F7. The descriptions (try-with-resources on the positive-control and loop-created services; two catch blocks with HazelcastInstanceNotActiveException at fine and everything else at warning with the registration id and IMap name; JobHistoryService implementing AutoCloseable with a public close; package-private getEntryListenerRegistrationIds with VisibleForTesting) all sound right. As with F1–F3, I'll mark these resolved once I've verified them in the diff rather than from the description alone.

F8 – raw type in the test. On my side the comment still ends at "parameterized with itself, JobHistoryServ", even though you report the stored body is complete. From what is visible, extending AbstractSeaTunnelServerTest parameterized with the test class itself would address the raw-type point. Could you post the remainder of that sentence as a short standalone comment so I can see the tail?

F1 / F2 – clearCoordinatorService() and the jobHistoryService field. Still open pending the diff. Specifically I need to see:

F1: clearCoordinatorService() closes the same JobHistoryService instance whose ownership it is tearing down (e.g. captured into a local), rather than whatever the mutable field points to at that moment.
F2: after close, the field is either nulled out or every re-activation path unconditionally constructs a new JobHistoryService, so a closed instance can never be reused.

Once the F8 tail is visible I'll do the final pass on all eight points against the diff.

DanielLeens · 2026-10-05T10:28:48Z

Here is the tail of F8 as a standalone comment, in plain text. On the current head a847181, JobHistoryServiceListenerCleanupTest declares: "public class JobHistoryServiceListenerCleanupTest extends AbstractSeaTunnelServerTest". That is, the base class is parameterized with the test class itself, so there is no raw type. I re-read the file at that SHA just now (class declaration at lines 58-59).

The F1 and F2 changes in clearCoordinatorService() are the part I would most like you to check against the diff itself.

DanielLeens · 2026-10-31T03:46:27Z

Merged dev (3ac2d1cec6a) into this branch with a plain merge commit (no rebase, no force push). New head: 993f3fdbeea (previous head a847181373). Only formatting was run locally (module-scoped spotless:apply); git diff origin/dev --name-status lists only this PR's own 4 files and no deletions.

Conflict and resolution:

CoordinatorService.java: dev ([Feature][STIP-41][Zeta] Add Native Zeta Application Mode on Kubernetes #12460) replaced the explicit getJobHistoryService() with a Lombok @Getter on the jobHistoryService field, while this PR kept the explicit getter to carry its Javadoc. Resolved by keeping dev's @Getter and moving this PR's invariant Javadoc (closing removes listeners only; reads stay valid; each activation creates a fresh instance) onto the field. The clearCoordinatorService() capture-then-close logic and the initCoordinatorService() comment are unchanged.

Compile and tests are left to the fork CI run per the validation policy (queued at the time of writing, no result claimed): https://github.com/DanielLeens/seatunnel/actions/runs/41165514671

DanielLeens

I re-read the full four-file diff and the current coordinator activation/teardown callers. +1 to @SEZ9’s ownership and partial-construction concerns; the committed code addresses F1–F8. One updated fact strengthens F1: checkNewActiveMaster now shares the synchronized monitor with clearCoordinatorService, in addition to the captured history-service reference.

What Problem Does This PR Solve?

User pain point: Repeated active-master transitions leave old finished-job expiration listeners registered, retaining old service objects and duplicating expiration side effects.

Fix approach: Own the three registration UUIDs, deregister them on coordinator teardown, roll back partial registration, and retain the old object only for in-flight read access.

One-sentence summary and example: After one node steps down, its three history listeners are removed; a later activation creates a fresh service with new registrations rather than reusing the closed listener view.

1. Code Change Review

1.1 Core Logic Analysis

JobHistoryService implements AutoCloseable and stores three final registration IDs. If construction fails at registration two or three, already-acquired IDs are removed before rethrowing the same failure. close attempts each removal independently. CoordinatorService captures its owned service immediately after the clear CAS guard and closes that instance after executor shutdown waits. Its only assignment site unconditionally constructs a new instance during activation.

Before:

finishedJobStateImap.addEntryListener(listener, true); // ID discarded, no paired teardown

After:

stateListenerId = finishedJobStateImap.addEntryListener(listener, true);
// On coordinator teardown:
closingJobHistoryService.close(); // deregisters its three IDs

Key Findings

Ordinary submit/run/finish does not close the coordinator listeners; role loss, initialization cleanup and shutdown do.
checkNewActiveMaster and clearCoordinatorService synchronize on the same coordinator; private init is called from that activation path.
The local capture prevents a later live-field read from selecting a different instance.
Closed services retain their map references and read methods, while reactivation always creates a new listener owner.
Partial-construction rollback and separate fine/warning logging resolve the earlier ownership/logging requests.
Cleanup remains best effort: an unexpected removal failure is logged, not a guarantee that exactly three registrations remain under every failure condition. Already-dispatched callbacks and listener-free transition windows are not made durable/exactly-once by this patch.

Complete runtime path

Master polling -> synchronized checkNewActiveMaster
 -> activation -> initCoordinatorService -> fresh JobHistoryService -> three registrations
role loss / init failure / shutdown -> synchronized clearCoordinatorService
 -> CAS guard -> capture owned history service -> executor shutdown/wait
 -> captured close -> independent IMap.removeEntryListener calls
 -> remaining resource-manager/event-processor cleanup
TTL expiration while active -> listener -> counters / CleanLogOperation fan-out

Registration/removal ownership is explicit and normal reactivation cannot reuse the closed instance. Constructor rollback preserves the primary failure even if one removal fails. Removing an already-removed ID is safe, which supports repeated close and try-with-resources. This closes the ordinary accumulation path; it does not turn Hazelcast expiration callbacks into a durable cleanup ledger.

1.2 Compatibility Impact

Fully compatible for public data contracts. No option/default, task/checkpoint serialization, IMap payload, connector registration or dependency changes. AutoCloseable is additive. Existing read methods continue after listener close. No migration is needed; downgrade loses the cleanup improvement without changing stored data formats.

1.3 Performance / Side-Effect Analysis

Three synchronous listener-removal calls per teardown and three UUID fields per service replace accumulating listener ownership. Calls run in an already blocking teardown path; no new per-record work or polling thread is introduced. A removal error can leave a registration until node/event-service cleanup, so a universal bounded-listener claim would be too strong.

1.4 Error Handling and Logging

HazelcastInstanceNotActiveException is expected during shutdown and logged at fine level; other removal exceptions are warnings with map/registration context. Each attempt continues independently, and construction rethrows its original failure. No new credential or row logging is added.
No blocking source issues found. No numbered source findings remain for this head.

2. Code Quality Assessment

2.1 Coding Standards

New ownership fields, lifecycle methods, test accessor and helpers have explanatory comments. @VisibleForTesting and the parameterized test base resolve F7/F8. Existing lifecycle code is extended rather than adding a separate manager.

2.2 Test Coverage and Test Stability

Test stability: Stable. Registration unit tests are synchronous and cover every failure index, rollback failure, shutdown/non-shutdown logs and reads after close. Real-member tests include a positive live-ID control, repeated close cycles and the actual coordinator-clear entry point. Try-with-resources and finally release test-owned services/members; allocated port ranges and an isolated cluster avoid introducing a fixed default-port assumption. Awaitility bounds activation readiness. These are source-level assessments, not a claim that the queued current-head run passed or that all transport failures were exercised.

Local analysis: fetched the head, created independent branch seatunnel-review-11809, read the complete 4-file diff and traced affected callers with git show/git grep. No local compilation, unit tests, integration tests, E2E, containers, or job execution was performed, as requested. These are source-analysis conclusions; execution evidence is the current-head CI below.

2.3 Documentation Updates

No EN/ZH user-guide or incompatible-changes entry is required for this internal ownership fix. Keep the best-effort cleanup qualification; neither old listeners nor this fix provide exactly-once delivery of expiration side effects.

3. Architectural Soundness

3.1 Elegance of the Solution

Precise lifecycle fix. The registering service owns the IDs and its coordinator owns its close. Moving this into global listener discovery or changing expiration persistence would greatly widen the fix.

3.2 Maintainability

The fresh-instance invariant is documented at both field and assignment. Preserve the common activation/teardown monitor and avoid adding an instance-reuse shortcut.

3.3 Extensibility

Further listener registrations should join the same acquired-resource rollback and independent close discipline. Durable log-cleanup reconciliation would be a separate contract.

3.4 Historical-Version Compatibility

The service itself is not checkpoint state; the serialized JobState/PipelineStateData fields are unchanged. Current-head lifecycle source, rather than old race assumptions, supports this compatibility conclusion.

4. Issue Summary

Number	Issue	Location	Severity
—	No formal issues found	Complete changed-file set	—

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Blockers — must be fixed
- No source blocker identified. No source blocker found. Successful current-head Build and effective independent maintainer approval remain required.
Recommended fixes — non-blocking
- No additional unrelated changes requested.

Overall assessment: F1–F8 are resolved in source. The prior claims of unconditional listener-count bounds and exhaustive race-test proof are narrowed to the actual best-effort implementation and tested paths.

Existing-review reconciliation: +1 to @SEZ9: F1 has captured ownership plus the current shared activation/clear monitor; F2 is addressed by the documented unconditional fresh construction; F3 has registration rollback; F4 uses try-with-resources; F5 separates stopped-node logging; F6 implements AutoCloseable; F7 marks the accessor; F8 uses the generic base. The retrieved independent review is COMMENTED, not effective approval. No duplicate numbered versions of those resolved findings are added. Daniel is the author. Author replies have been incorporated into this single full review.

Head 993f3fdbeeab550c462d795c55b85e217dc1c51f. Live base dev at 3ac2d1cec6aa7b3741c4dacdd23020f778e23c02; comparison ahead, ahead 11 / behind 0; merge state BLOCKED. Behind/diverged alone is recorded metadata, not a request to synchronize upstream. Checks: No current-head checks reported.

No current-head upstream checks are reported. Exact-head fork Build is queued. Older network/time-limit/engine failures cannot be carried forward as the result of this head or declared unrelated merely because their class names differ. No new upstream-sync request is justified by the queued state.

Daniel has READ repository permission. This COMMENT records the technical conclusion; it is not an effective maintainer approval or a dismissal of any historical requested-changes entry. A write-capable maintainer owns the final repository approval gate.

SEZ9 · 2026-11-01T03:06:28Z

Thanks for the follow-up. Before signing off I want to verify the earlier points against the current diff rather than rely on confirmation alone. Could you point me to the relevant changes for each?

F1 / F2 (coordinator ownership): Where is the owned JobHistoryService captured right after the clear guard and that captured instance closed, rather than re-reading the mutable field? Since the field may still reference a closed instance afterwards, a short comment at the single assignment site noting that re-activation must always construct a fresh instance would help preserve that invariant.

F3 (partial construction): Where are already-acquired registration IDs rolled back before the original failure is rethrown? Please also make sure a failure during the rollback removal itself doesn't mask the original exception (log it and still rethrow the original).

F4 (test listener leak): Does the new close test wrap its registrations in try/finally so an assertion failure doesn't leave live listeners on the shared node's IMaps?

F5 (removal logging): Which failures are logged at fine level versus warning in the quiet removal helper? Please make sure the warning path includes the map name / registration ID so a re-created leak is diagnosable.

F6 / F7 / F8: Does JobHistoryService implement AutoCloseable, is getEntryListenerRegistrationIds() annotated as visible-for-testing, and does the new test no longer extend AbstractSeaTunnelServerTest as a raw type?

Once I can see these in the diff, I'm happy to approve.

DanielLeens · 2026-11-01T12:13:19Z

Thanks @SEZ9. Here are the unchanged-head implementation pointers for F1-F8:

F1/F2: CoordinatorService.java:1314-1317 captures closingJobHistoryService immediately after the clear guard; :1363-1366 closes that captured instance. The single assignment at :640-643 explicitly says never to reuse the old view and constructs a new JobHistoryService.
F3: JobHistoryService.java:159-184 tracks acquired IDs, rolls back state/metrics registrations on partial construction and rethrows the original RuntimeException. The quiet removal helper catches removal exceptions, so rollback failure does not replace that original exception.
F4: JobHistoryServiceListenerCleanupTest.java:78-102 uses try-with-resources for the positive control and repeated close cases, including assertion failure cleanup.
F5: JobHistoryService.java:475-493 logs HazelcastInstanceNotActiveException at fine level; other Exceptions are warnings with registration ID and store/map name. This is best-effort deregistration, not a guarantee of removal after an arbitrary remote failure.
F6/F7/F8: JobHistoryService implements AutoCloseable (:71); its registration accessor is package-private and VisibleForTesting (:226-227); the cleanup test extends AbstractSeaTunnelServerTest (:58-59).

The earlier queued-CI fact has changed. The exact-head fork run is complete/cancelled and Apache Build is TIMED_OUT. Check Helm Chart and Sanity check results are cancelled; the executed source/unit/integration lanes in this attempt are successful, but that is not a complete green Build. The run owner should first check for an active newer attempt, then resume those remaining required jobs. Cancellation supplies no demonstrated source fix in dev, so sync alone is not a cure. The source conclusion stays clear; Daniel is the author and cannot provide independent approval. A write-capable independent maintainer owns the final approval/merge gate.

Checked 2026-11-01T12:13:11.960780+00:00; unchanged head 993f3fdbeeab550c462d795c55b85e217dc1c51f, base dev; compare diverged, ahead 11 / behind 9; merge blocked / BLOCKED; review decision REVIEW_REQUIRED. Behind/diverged is recorded independently and does not itself request a sync. Daniel has READ repository permission.

github-actions Bot added the Zeta label Aug 14, 2026

DanielLeens commented Aug 15, 2026

View reviewed changes

DanielLeens commented Aug 16, 2026

View reviewed changes

[Chore][Zeta] Retrigger PR apache#11809 build

4b2e5c3

[Chore][Zeta] Retrigger PR apache#11809 build

fbe4e66

[Test][Zeta] Harden JobHistoryService listener cleanup test

3851e0d

DanielLeens commented Aug 20, 2026

View reviewed changes

DanielLeens mentioned this pull request Aug 20, 2026

[Fix][Zeta] Fix JobHistoryService IMap listener leak across master-role switches #11900

Open

DanielLeens commented Aug 21, 2026

View reviewed changes

SEZ9 reviewed Aug 21, 2026

View reviewed changes

DanielLeens commented Aug 21, 2026

View reviewed changes

[Fix][Zeta] Capture listener cleanup service generation

c58e184

DanielLeens commented Aug 23, 2026

View reviewed changes

DanielLeens commented Aug 24, 2026

View reviewed changes

DanielLeens mentioned this pull request Aug 25, 2026

[Fix][Zeta] Restore schema evolution state during checkpoint recovery #11780

Closed

4 tasks

DanielLeens commented Sep 1, 2026

View reviewed changes

DanielLeens added 2 commits September 8, 2026 00:08

Merge remote-tracking branch 'apache/dev' into pr11809-merge-dev

9aa9bfe

# Conflicts: # seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/CoordinatorService.java

[Fix][Zeta] Roll back partial job history listener registration

ab1538a

DanielLeens mentioned this pull request Sep 9, 2026

[Bug][Zeta] Unchecked lifecycle close failure skips remaining task resource cleanup #12231

Open

1 task done

DanielLeens commented Sep 11, 2026

View reviewed changes

Merge branch 'dev' into fix-jobhistory-listener-leak-11807

d7de2af

DanielLeens commented Sep 23, 2026

View reviewed changes

davidfans added 2 commits September 27, 2026 02:47

Merge remote-tracking branch 'apache/dev' into fix-jobhistory-listene…

1d497fb

…r-leak-11807

Merge remote-tracking branch 'apache/dev' into fix-jobhistory-listene…

a847181

…r-leak-11807

nzw921rx mentioned this pull request Oct 22, 2026

[Roadmap] Apache SeaTunnel 3.1.0 Roadmap #12949

Open

Merge branch 'dev' into fix-jobhistory-listener-leak-11807

993f3fd

DanielLeens commented Oct 31, 2026

View reviewed changes

Conversation

DanielLeens commented Aug 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose of this pull request

Does this PR introduce any user-facing change?

How was this patch tested?

Check list

Uh oh!

DanielLeens left a comment

Choose a reason for hiding this comment

What Problem Does This PR Solve?

1. Code Change Review

1.1 Core Logic Analysis

1.2 Compatibility Impact

1.3 Performance / Side-Effect Analysis

1.4 Error Handling and Logging

2. Code Quality Assessment

2.1 Coding Standards

2.2 Test Coverage and Test Stability

2.3 Documentation Updates

3. Architectural Soundness

3.1 Elegance of the Solution

3.2 Maintainability

3.3 Extensibility

3.4 Historical-Version Compatibility

4. Issue Summary

5. Merge Recommendation

Conclusion: Ready to merge

Uh oh!

DanielLeens left a comment

Choose a reason for hiding this comment

What Problem Does This PR Solve?

1. Code Change Review

1.1 Core Logic Analysis

1.2 Compatibility Impact

1.3 Performance / Side-Effect Analysis

1.4 Error Handling and Logging

2. Code Quality Assessment

2.1 Coding Standards

2.2 Test Coverage and Test Stability

2.3 Documentation Updates

3. Architectural Soundness

3.1 Elegance of the Solution

3.2 Maintainability

3.3 Extensibility

3.4 Historical-Version Compatibility

4. Issue Summary

5. Merge Recommendation

Conclusion: Source logic approved — no correctness blockers; merge gated on CI completing green, with Issues 1 and 2 strongly recommended first.

Uh oh!

DanielLeens commented Aug 17, 2026

What Problem Does This PR Solve?

What Changed Since My Last Review

1. Code Change Review

1.1 Core Logic Analysis

1.2 Compatibility Impact

1.3 Performance / Side-Effect Analysis

1.4 Error Handling and Logging

2. Code Quality Assessment

2.1 Coding Standards

2.2 Test Coverage and Test Stability — does the test actually detect a leak, and is it CI-safe?

2.3 Documentation Updates

3. Architectural Soundness

3.1 Elegance of the Solution

3.2 Maintainability

3.3 Extensibility

3.4 Historical-Version Compatibility

4. Issue Summary

5. CI Status

6. Merge Recommendation

Conclusion: Not ready to merge. Source logic remains correct; CI is red on an apparently-unrelated infra flake, and two Medium test-stability issues from my prior review are still unaddressed one round later.

Uh oh!

DanielLeens commented Aug 19, 2026

Re-verification against issue #11807's "3 listeners, not 1" claim

Re-verification against the "removal-before-drain race" / abnormal-termination question

Current issue list (unchanged from my 2026-08-17 review — carried forward, not re-numbered)

CI status (checked live at review time)

Merge recommendation

Conclusion: Not ready to merge yet — source logic is correct and complete, but CI needs a clean rerun and the two Medium test-stability issues should be fixed now rather than carried to a 4th round.

Uh oh!

DanielLeens commented Aug 14, 2026 •

edited

Loading