LGTM. The two bugs are real and the fix is at the right layer.

Evidence

• The scheduler really is killed by the old code: startSelectMasterTask (ShenyuClusterService:69) uses scheduleAtFixedRate, so the throw new ShenyuException(...) from the catch block suppresses every subsequent execution per the ScheduledExecutorService contract. Not rethrowing is the correct fix.
• The cleanup moved into finally gated on selected closes both services on every master-loss path (renewal returned false, renewal threw, interrupt), which is strictly better than before: master only closed them on the exception path, so a normal renewed == false exit left UpstreamCheckService and InstanceCheckService tasks running on a node that no longer held the lock.
• Ordering is preserved and now explicit: services are closed before releaseMaster() (:109-123), so no other node can take the lock while this one is still writing instance state.
• ShenyuException had only two occurrences in the file (the import and the rethrow), so dropping the import still compiles.
• Per-instance try/catch in syncDB is the right granularity: one failing row no longer starves the rest, and the outer try/catch keeps the schedule alive. That also protects the new close() → syncDB() call on an interrupted thread.

Non-blocking

1. shutdown() (:136-137) calls shutdown(), so a task blocked in TimeUnit.SECONDS.sleep(selectPeriod) survives for up to one period and then runs the whole finally (close + releaseMaster). shutdownNow() would actually exercise the InterruptedException branch you just added — please pick one deliberately and say which in the PR body.
2. Now that selected exists, releaseMaster() at :122 is still invoked when this node never acquired the lock (pre-existing). Gating it on selected makes the pair symmetric — only if your lock implementation is idempotent for non-owners, which it appears to be.
3. Swallowing every exception turns a persistent failure into one log line per period with no metric. A counter would make the #6495 scenario visible in ops instead of only in logs.
4. The branch has no CI run (no checks reported), so the new scheduler tests have never executed in CI. Please make sure pr_build is green before merge.
5. System.setProperty("net.bytebuddy.experimental", "true") in a static block is a smell that will outlive the JDK it was added for; a one-line comment with the JDK/byte-buddy versions would save the next person from deleting it.

Approved.

Solid hardening for #6608. ShenyuClusterService.doSelectMaster no longer throws ShenyuException on failure (which cancelled the periodic schedule); instead it tracks a consecutive-error count via AtomicLong, restores the interrupt flag on InterruptedException, and only releases the master lock and closes the check services when this node was actually selected. shutdown() now properly stops the executor. InstanceCheckService.syncDB() isolates per-instance DB failures so one bad write cannot abort the whole sync. The new ShenyuClusterServiceTest and InstanceCheckServiceTest cover retry-after-exception, cleanup-on-renewal-failure, and error counting.

Minor note: CI checks have not reported on this branch yet; please confirm the workflow passes before merging.

Re-reviewed at 8d1c571e. The scheduler-hardening core is right, but two things should change before this lands.

What is correct

• doSelectMaster() no longer throws out of a ScheduledExecutorService task. A ScheduledThreadPoolExecutor silently cancels all future executions when a periodic task throws, so the old throw new ShenyuException(...) was the actual bug behind #6608 — the master selector stopped running for the lifetime of the process while the admin still looked healthy.
• Gating the finally block on selected fixes a real defect: previously a plain "did not get the lock" return still called releaseMaster() and closed both check services, so a standby node could release nothing and tear down services it never started.
• Handling InterruptedException separately and restoring the interrupt flag, instead of treating it as a generic error, is correct.
• The Objects.nonNull(executorService) guard in shutdown() makes shutdown-before-start safe.
• Per-entry try/catch in syncDB() is the right shape.

Must fix: global JVM mutation from a test class

static {
    // Required for ByteBuddy 1.14.x running on preview/newer JDKs (e.g. JDK 22+) without official bytecode support
    System.setProperty("net.bytebuddy.experimental", "true");
}

A static initializer in a test class changes behaviour for the whole surefire JVM, i.e. every other test that happens to run in the same fork, in an order nobody controls. It also hides a genuine toolchain problem: if ByteBuddy cannot handle the JDK, the fix belongs in the build, not in one test class. Put it in the surefire configuration (<systemPropertyVariables> or argLine) so it is visible and applies uniformly, or drop it — pr_build on master runs these tests today without it.

Must coordinate with #7749

This PR and #7749 both modify InstanceCheckService:

• handleBeatInfo() — the InstanceInfoUtils.normalize(...) call on the existing-instance branch is byte-for-byte the same change in both PRs.
• syncDB() — this PR adds only a per-entry try/catch; #7749 replaces the whole method with a separate syncExecutor, a ReentrantLock guard, a snapshot copy and per-entry isolation, which supersedes it.

Landing both will conflict, and if this one lands second it partially reverts #7749's design. Please drop both InstanceCheckService changes from this PR and keep only the ShenyuClusterService hardening, or explicitly rebase on #7749 once it merges.

Non-blocking notes

• selectMasterErrorCount is only reset when selectMaster() returns true. A node that keeps losing the election (returns false, no exception) never resets it, so a later single exception reports an inflated "consecutive failures" count. Decide whether the metric means consecutive exceptions or consecutive failed elections, and reset in the matching branch.
• getSelectMasterErrorCount() exists only for assertions. Mark it @VisibleForTesting (or drop it and assert on behaviour) so it does not look like a supported API.
• The finally block closes both check services whenever selected is true. If upstreamCheckService.setup() succeeds but instanceCheckService.setup() throws, the paired close should still be safe — worth confirming close() is idempotent on both types, since that half-initialised path is now reachable.
• attempts.await(5, TimeUnit.SECONDS) is the same order of magnitude as clusterProperties.getSelectPeriod(). If the test property is still the default, the assertion can flake under load. Setting selectPeriod to ~1s in setUp() and asserting on the latch would make it deterministic.

CI

There is no workflow run for this branch: GET /repos/apache/shenyu/actions/runs?head_sha=8d1c571e returns total_count: 0, and gh pr checks reports nothing. This applies to your other open PRs as well, so Actions looks disabled on the fork. Please enable it and push again.

Re-review: #7765 [ISSUE #6608] Harden periodic tasks against uncaught exceptions

Verdict: approved — both blockers from my previous review are gone, and the remaining notes were addressed rather than argued with.

Both blockers resolved

1. The global JVM mutation is gone. The static { System.setProperty("net.bytebuddy.experimental", "true"); } initializer is no longer in ShenyuClusterServiceTest. That was the one thing I would not merge: a static initializer in a test class silently rewrites behaviour for every other test that shares the surefire fork, in an order nobody controls, and it hides a toolchain problem behind a per-class workaround.

2. The InstanceCheckService overlap with #7749 is gone. The PR now touches only ShenyuClusterService and its test, so the normalize(...) duplication and the syncDB() conflict with #7749's syncExecutor / ReentrantLock / snapshot design no longer exist. Diff is two files, which is what a scheduler-hardening fix should be.

Notes addressed

• selectMasterErrorCount semantics are now decided and consistent. selectMasterErrorCount.set(0) runs immediately after selectMaster(...) returns, before the if (!selected) return;, so the counter means consecutive exceptions, not consecutive failed elections — a node that keeps losing the lock but never throws reports 0. That is the sensible reading, and it is now the documented one.
• getSelectMasterErrorCount() is package-private with "visible for testing" in its Javadoc, so it no longer reads as supported API.
• The finally block is now failure-isolated. close() on each check service and releaseMaster() each have their own try/catch, so a throwing upstreamCheckService.close() can no longer skip instanceCheckService.close() or the release. That is strictly better than what the old catch-then-throw did, and it also answers my question about the half-initialised path: selected is already true when instanceCheckService.setup() throws, so both closes still run.
• InterruptedException is handled separately with the interrupt flag restored, and the catch no longer rethrows, which is the actual fix for #6608: a ScheduledThreadPoolExecutor silently cancels all future executions of a periodic task that throws, so the old throw new ShenyuException(...) stopped master election for the life of the process while the admin still looked healthy.

Test coverage is now behavioural, not incidental

Four new cases, each targeting one branch of the reworked flow:

• testMasterSelectionContinuesAfterException — the #6608 regression itself: first election throws, the second runs. Asserts atLeast(2) selections, and never() on releaseMaster / setup / close, which is the correct expectation because the throwing election never acquired leadership.
• testMasterRenewalExceptionCleansUpAndRetries — leadership acquired, then checkMasterStatus() throws. Asserts both services were set up and closed, and releaseMaster() happened exactly once. This is the case where getting the selected gate wrong would leak a lock.
• testSelectMasterFailureWaitsForNextPeriod — a plain "did not get the lock" must touch nothing: no setup, no close, no release. That is the defect the selected flag was introduced for, and it is now pinned.
• testSelectMasterErrorCountTracking — two consecutive throws leave the counter >= 2.

@AfterEach clusterService.shutdown() is the right addition: these tests start a real scheduler, so without it a failing assertion would leave a thread running and pollute the next test.

The 1L select period in setUp() plus the 5s latch await gives a margin of 5× the period, which is what makes these deterministic under load rather than the previous await(5s) against a default period.

Non-blocking

• testMasterRenewalExceptionCleansUpAndRetries asserts verify(clusterSelectMasterService, times(1)).releaseMaster(). With a 1s period and a fast test, a second election could in principle win before the assertion runs and produce a second release. The attempts latch of 2 fires on the second selection attempt, which returns false, so it should be stable — but atLeast(1) would be immune to timing without weakening the intent.
• doSelectMaster still sleeps clusterProperties.getSelectPeriod() between renewal checks, so selectPeriod doubles as both election retry interval and leadership renewal interval. Pre-existing, and out of scope here, but it does mean the two cannot be tuned independently.

CI

pr_build and the lint/CodeQL jobs pass. build (shenyu-integrated-test-http) fails on the pre-existing master regression (CryptorResponsePluginTest.testWhenDoesntExistFiled), which #7970 fixes — this PR needs #7970 merged (or a rebase after it lands) before the check can go green.

This is the right direction for "harden periodic tasks": a single transient exception in doSelectMaster used to close the upstream/instance checkers, stop the scale lifecycle service and rethrow ShenyuException from a scheduled task, which is exactly the kind of uncaught failure that silently kills the master-election loop. Now the failure is counted, logged with the consecutive-failure count, and the task survives to the next tick; the cleanup moved into a finally that is correctly gated on selected, so a node that never held the lock no longer calls releaseMaster().

Two things worth tightening before merge:

1. Lock-leak window. selected is only assigned when selectMaster(...) returns normally. If the lock is acquired inside selectMaster and the call then throws (e.g. Redis timeout after SET NX), selected stays false and the finally block skips releaseMaster(), leaking the master lock until TTL expiry. The old code released unconditionally in finally, so this is a regression on that specific edge. Consider tracking "attempted" separately from "selected", or releasing whenever the lock may have been taken.

2. No escalation. A permanently broken election (Redis down) now logs an error every period forever and never surfaces anywhere else. Consider escalating after N consecutive failures (health endpoint / metric) so the condition is observable rather than only in logs.

The InterruptedException handling and the null-guard on executorService in shutdown() are both correct.

LGTM with the lock-leak edge addressed or explicitly justified.

Re-reviewed after integration with master. The election failure counter/logging remains, and all master-owned upstream, instance and scaling resources retain guarded cleanup. The continuation regression observes election attempts directly and verifies that an unsuccessful election does not release or close resources it never owned. All 9 ShenyuClusterServiceTest cases pass on the integrated tree. Checkstyle and git diff --check pass. Required GitHub checks must pass on this new head before merge.