Thanks for working on this. I went through the full current diff and most of the cleanup is moving in the right direction: the executor cleanup in MDCTracerTest, the before() overrides for the REST tests, and the tighter timeout handling all address real flaky-test sources.

What this PR fixes

• User pain: these engine/api/core UTs were still vulnerable to leaked executors, unrecovered global state, REST test setup methods that did not actually override the base lifecycle, and overly brittle wait timing.
• Fix approach: the PR adds finally cleanup, replaces unbounded waits with timed waits, makes the REST setup methods truly override the base before() hook, and adjusts checkpoint cleanup assertions.
• In one sentence: most of the patch removes real instability, but one checkpoint test now hides storage failures instead of proving cleanup correctness.

Simple example:

• Before this change, RestApiHttpsTest defined its own setUp() method, but it did not override the base test lifecycle method, so the intended HTTPS-specific server init was not reliably attached to the real setup path.
• After this change, the method is renamed to before() with @Override, so the test-specific server configuration actually runs on the real base lifecycle.

Runtime chain I checked

checkpoint cleanup verification
  -> CheckpointStorageTest.testBatchJobWithCheckpoint() [113-138]
      -> startJob(jobId, BATCH_CONF_WITH_CHECKPOINT_PATH, false)
      -> wait until JobStatus.FINISHED
      -> checkpointStorage.getAllCheckpoints(jobId)
          -> HdfsStorage.getAllCheckpoints() [140-159]
             -> getFileNames(path)
             -> readPipelineState(...)
             -> throws CheckpointStorageException if no readable states remain
          -> LocalFileStorage.getAllCheckpoints() [122-146]
             -> FileUtils.listFiles(...)
             -> throws CheckpointStorageException on traversal failure

REST server lifecycle
  -> RestApiHttpsTest.before() [70-94]
  -> RestApiHttpBasicTest.before() [60-80]
  -> BaseServletTest.before() [36-55]
      -> test-specific HTTP/HTTPS config is now applied through the actual base lifecycle hook

Findings

Problem 1: CheckpointStorageTest now treats real storage failures as a successful cleanup result

• Location: seatunnel-engine/seatunnel-engine-server/src/test/java/org/apache/seatunnel/engine/server/checkpoint/CheckpointStorageTest.java:127-136
• Why this is a problem: the test is supposed to prove that checkpoint artifacts are cleaned up after the batch job finishes. On the current head, any CheckpointStorageException is treated as equivalent to “the directory was already cleaned up”.
• Concrete evidence from the real storage implementations:
  • seatunnel-engine/.../checkpoint-storage-hdfs/.../HdfsStorage.java:140-159 throws CheckpointStorageException not only for “nothing is left”, but also when all checkpoint files fail to read.
  • seatunnel-engine/.../checkpoint-storage-local-file/.../LocalFileStorage.java:122-146 throws CheckpointStorageException when directory traversal fails.
• Risk: this turns a real checkpoint-storage regression into a false green test result, which is worse than a flaky red because CI will report success while the cleanup path is actually broken.
• Suggested fix:
  • Option A: only treat the known “directory already removed / no-such-file” case as acceptable and fail on every other CheckpointStorageException.
  • Option B: explicitly assert the storage path state first, then distinguish “path removed” from “checkpoint count is 0” instead of swallowing the whole abstraction.
• Severity: high

Test stability assessment

• Rating: high risk
• Basis: the patch removes many flaky inputs, but CheckpointStorageTest now creates a false-positive success path on the engine checkpoint lifecycle, which is a blocker for this kind of test cleanup PR.

Conclusion: merge after fixes

1. Blocking items

• Problem 1: restore a precise checkpoint-cleanup assertion instead of swallowing all CheckpointStorageExceptions.

2. Suggested follow-up

• No additional non-blocking source issue from my side on the current head beyond that blocker.

Overall, the patch is close, and most of the cleanup work is good. Once the checkpoint assertion is made failure-specific again, I’d be happy to re-review the new head.

Thanks for the update. I re-reviewed the latest head from scratch and specifically re-checked the checkpoint-cleanup test path that I blocked in the previous round.

What this PR fixes:

• User pain: several engine/core tests were flaky because of leaked resources, weak lifecycle hooks, and brittle assertions.
• Fix approach: most of that cleanup was already in place; the latest head fixes the remaining false-positive checkpoint assertion.
• One-line summary: the current head closes the only blocker I had on the previous revision.

Runtime path re-checked:

CheckpointStorageTest.testBatchJobWithCheckpoint()
  -> checkpointStorage.getAllCheckpoints(jobId)
  -> HdfsStorage / LocalFileStorage
  -> distinguish "path already gone" from real storage failure

Findings:

• No blocking source-level issue remains.
• CheckpointStorageTest.java:134-142 now only treats FileNotFoundException / NoSuchFileException as acceptable cleanup outcomes and rethrows every other CheckpointStorageException, so the test no longer hides real storage regressions.
• Test stability rating for the latest change: stable.

CI:

• The old red Build badge is stale. head_sha=9d566674... maps to the latest Apache workflow runs and they are successful.

Conclusion: can merge

1. Blocking items

• None.

2. Suggested follow-up

• None.

Overall, this is mergeable.

+1 if CI passes

Thanks for the follow-up. I re-reviewed the latest head from scratch, including the updated SeaTunnelEngineClusterRoleTest assertion thread, and I also rechecked the earlier flaky-test cleanup paths in MDCTracerTest, CoordinatorServiceTest, CheckpointStorageTest, and the REST API tests.

I also agree with @dybyte's latest merge signal: on the current head I do not see a remaining source-level blocker, and the discussion around the cancel-state assertion is now resolved in code.

What this PR fixes

• User pain: several engine/core/api tests were flaky because of leaked resources, weak lifecycle hooks, brittle waits, and incomplete cleanup.
• Fix approach: the PR tightens Awaitility usage, restores proper lifecycle overrides, adds missing resource cleanup, and keeps the checkpoint-cleanup assertion precise.
• One-line summary: the current head keeps the meaningful anti-flake fixes without hiding real failures.

Simple example:

• Before this PR, some REST test setup methods were not overriding the real base lifecycle hook, and some async assertions depended on short timing windows.
• On the current head, the setup hooks run through the correct lifecycle path and the waits are explicit enough to tolerate slow CI without swallowing real errors.

Runtime / test path rechecked

REST test lifecycle
  -> BaseServletTest.before()
  -> RestApiHttpsTest.before() / RestApiHttpBasicTest.before() / RestApiHttpsForTruststoreTest.before()
      -> test-specific server config is now attached to the real lifecycle hook

checkpoint cleanup verification
  -> CheckpointStorageTest.testBatchJobWithCheckpoint()
      -> wait for FINISHED
      -> checkpointStorage.getAllCheckpoints(jobId)
      -> accept only already-removed-path cases, rethrow other storage failures

client cancel path
  -> SeaTunnelEngineClusterRoleTest.testGetJobStatus()
      -> wait for RUNNING
      -> jobClient.cancelJob(jobId)
      -> wait for terminal `CANCELED` on this normal cancel scenario

Findings

• I did not find a remaining code blocker on the latest head.
• The previous checkpoint-storage false-positive risk is still fixed on this revision.
• The updated cancel-path assertion thread is resolved on the latest code.
• The touched test changes do not introduce a new obvious flaky-test anti-pattern on the current head.

CI

I also checked the current failed Build on this head. The failing jobs include:

• engine-v2-it (8, ubuntu-latest): SplitClusterPendingJobLifecycleFailoverIT.testPendingJobLifecycleInMasterFailover
• all-connectors-it-3 (8, ubuntu-latest): SqlServerCDCIT.testDialectCheckDisabledCDCTable
• jdbc-connectors-it-part-7 (11, ubuntu-latest): JdbcIrisIT startup timeout

Those failures do not overlap the test files changed in this PR, so I do not see evidence that this patch introduced them.

Merge conclusion: can merge

1. Blocking items

• No blocking code issue from my side on the latest revision.

2. Suggested follow-up

• Please rerun once the unrelated CI failures are cleared so the branch can merge with a green Build signal.

Thanks again for iterating on the flaky-test cleanup here.

Thanks for the update. I re-reviewed the latest head from scratch and focused on what these test changes actually improve, which paths they exercise, and whether they introduce new flaky-test patterns.

What this PR solves

• User pain: a set of Engine / Core / API tests still rely on short waits, brittle timing, or even the wrong job id, so CI can go red for reasons that are not real product regressions.
• Fix approach: replace brittle waits with condition-based assertions, widen overly tight timeouts, add safer cleanup, and fix incorrect test wiring such as the wrong jobId in CheckpointTimeOutTest.
• One-line summary: the direction is correct and I do not see a new source-level regression from the test changes themselves, but the current Build is still red so the merge gate is not clear yet.

Simple example: CheckpointTimeOutTest previously started the job with System.currentTimeMillis() but asserted against a different jobId variable. The latest head correctly uses the same jobId for both start and assertion, which makes the test validate the right job instead of a phantom one.

1. Code change review

1.1 Core logic analysis

This PR only changes tests, not production code.

Call paths covered by the changed tests:

coordinator scheduling / master switch tests
  -> CoordinatorService
      -> pending queue / active coordinator state
      -> Awaitility waits for stable state

checkpoint timeout test
  -> startJob(jobId, conf)
      -> CoordinatorService.getJobStatus(jobId)
      -> assert RUNNING / FAILED on the same job id

engine client role test
  -> cluster bootstrap
      -> submit job
      -> query job detail / cluster-role behavior

Key findings:

• The changes are aimed at making tests wait for the real state transition instead of sleeping and hoping.
• CoordinatorServiceWithCancelPendingJobTest is better now that it no longer does a fixed Thread.sleep(5000) after seeing PENDING.
• CheckpointTimeOutTest fixes a real test bug by using the same jobId for job submission and status assertions.
• I do not see a new flaky-test anti-pattern introduced by the latest head.

Before / after examples:

// before
Thread.sleep(5000);

// after
await().atMost(120, TimeUnit.SECONDS)
    .untilAsserted(() -> {
        JobStatus status = jobMaster.getJobStatus();
        Assertions.assertTrue(PENDING.equals(status) || RUNNING.equals(status));
    });

// before
startJob(System.currentTimeMillis(), CONF_PATH);

// after
startJob(jobId, CONF_PATH);

1.2 Compatibility impact

Conclusion: fully compatible.

• API / config / protocol / serialization: no production impact
• Historical behavior: unchanged outside test execution

1.3 Performance / side effects

• Production CPU / memory / GC / concurrency: no impact, because this PR only changes tests.
• The only trade-off is CI failure latency: some waits now allow more time before failing, which is acceptable if it reduces false negatives.

1.4 Error handling and logging

Issue 1: the current Build is still red, so the merge gate is not clear yet

• Location: current GitHub Build check; fork run nzw921rx/seatunnel run 27190043790
• Problem description:
  I checked the current CI signal behind this head. The red Build is not coming from the test files changed in this PR. In the unit-test logs, the actual failing assertion is in seatunnel-engine-client SeaTunnelClientTest.testGetMultiTableJobMetrics at SeaTunnelClientTest.java:692, and the same Build also has a failed connector-file-sftp-it (11, ubuntu-latest) job.
• Potential risk:
  From a source review perspective I do not see a new regression introduced here, but the project gate is still red, so this revision is not ready to merge as-is.
• Best improvement suggestion:
  Please get the current Build green first. If the same failures keep reproducing, I would treat SeaTunnelClientTest.testGetMultiTableJobMetrics and connector-file-sftp-it as separate flaky/infra follow-up items rather than blockers caused by this diff.
• Severity: High
• Already raised by others: No

2. Code quality evaluation

2.1 Code style

The test changes stay within the existing style. Using condition-based waits instead of fixed sleeps is an improvement.

2.2 Test coverage and stability

• This PR is about stabilizing existing tests rather than adding new product coverage.
• Stability rating: Stable
  • CoordinatorServiceWithCancelPendingJobTest.java:146-154 removes a fixed sleep and waits on a real state condition instead.
  • The updated Awaitility usage does not depend on external ports, random ordering, or fire-and-forget async execution.
  • CheckpointStorageTest.java:120-132 only tolerates FileNotFoundException / NoSuchFileException during async cleanup, which matches the real deletion window rather than masking arbitrary failures.

2.3 Documentation

No docs update is needed because this PR does not change user-facing runtime behavior.

3. Architecture

3.1 Solution elegance

This is a precise test-stability cleanup, not a workaround piled onto production code.

3.2 Maintainability

Better than before: fewer sleeps, clearer waiting conditions, safer resource cleanup.

3.3 Extensibility

The pattern here is reusable for other flaky tests: wait on the real system state, not on wall-clock guesses.

3.4 Historical compatibility

No historical compatibility concern.

4. Issue summary

5. Merge conclusion

Conclusion: can merge after fixes

1. Blocking items

• Issue 1: please get the current Build green first. On the fork run behind this head (nzw921rx/seatunnel run 27190043790), the failed integration job is connector-file-sftp-it (11, ubuntu-latest), and the unit-test logs show a failure in the untouched seatunnel-engine-client test SeaTunnelClientTest.testGetMultiTableJobMetrics (SeaTunnelClientTest.java:692).

2. Suggested but non-blocking follow-up

• I do not have a new source-level blocker in the changed test files themselves.

+1 to @dybyte's earlier approval direction on the code path itself: after re-checking the latest head, I also do not see a new regression introduced by these test-only changes. The remaining gate is the red Build signal rather than a new issue in the modified tests.