[harshkha3417]

What is the purpose of the change

Fixes #7744.

InstanceCheckService previously scheduled both the periodic in-memory liveness checker and syncDB on a single-threaded executor. Slow or blocked database access during synchronization caused the liveness check to be starved, keeping timed-out instances marked as ONLINE in memory.

Brief change log

• Separated syncExecutor onto a dedicated single-threaded scheduler so database synchronization cannot block in-memory heartbeat checks.
• Synchronized syncDB, wrapped it in try-catch error handling, and used point-in-time snapshot copies for database persistence.
• Updated close() to shut down both executors cleanly.
• Added tests in InstanceCheckServiceTest verifying that a blocked database synchronization does not prevent the liveness checker from updating states on time.

Verifying this change

• mvn checkstyle:check -pl shenyu-admin passes with 0 violations.
• mvn test -pl shenyu-admin -Dtest=InstanceCheckServiceTest passes cleanly.

[harshkha3417]

Hi @Aias00 ,
Fixes #7744, fixes #7748.

1. Solves [BUG] Slow instance database synchronization blocks liveness checks on the shared scheduler #7744 by isolating database synchronization onto a dedicated single-threaded scheduler (syncExecutor), preventing slow persistence from starving time-sensitive in-memory heartbeat checks.
2. Solves [BUG] Instance checker skips executor shutdown when its final database sync fails #7748 by guaranteeing that executor and syncExecutor shutdown and cleanup run even if the final database sync fails during close().

[Aias00]

LGTM on the isolation itself, and the snapshot copy is a better answer than I expected. One coordination issue and two nits.

What I verified

• copyInstanceInfoVO covers all nine fields of InstanceInfoVO (id does not exist on that VO), and InstanceInfoServiceImpl.createOrUpdate derives its lookup from ip/port/type/namespace and its updateById from the DO it just fetched — it never reads an id off the VO. So the copy is safe for both the insert and the update path, and it closes the window where doCheck mutates instanceState in the middle of the JDBC call.
• The new test actually proves the bug: it blocks createOrUpdate, then invokes doCheck on the calling thread and requires the entry to reach OFFLINE while the sync is still parked. On master both tasks share one thread, so that assertion is precisely what would fail.
• close() now shuts both executors down before the final sync and clears the map in finally, so a failing final sync no longer leaks the cached entries.

Coordination issue (please address)

#7765, same author, rewrites this same syncDB method (it adds a per-instance try/catch) and is already approved. The two will conflict textually, and the merged result should keep #7765's per-instance granularity: with a whole-loop catch here, one failing row aborts the remainder of the sync, which is the opposite of what #7765 fixes. Please rebase one on the other and say in the PR which granularity wins.

Nits

1. synchronized syncDB() plus close() calling syncDB() means @PreDestroy blocks for the duration of any in-flight sync; with a hung JDBC call that is an unbounded shutdown delay. A guarded tryLock or a timeout would bound it.
2. setup() still allocates fresh executors on every call, so a second call (cluster re-election) leaks the previous pair. #7765 closing on master loss covers this in practice, but an explicit close-if-present at the top of setup() would make it airtight.

CI: no workflow run on the branch. Please make sure pr_build is green before merge.

[harshkha3417]

Thanks for the detailed review @Aias00!

I've addressed all the feedback and rebased the branch onto the latest master:

1. Coordination with [ISSUE #6608] Harden periodic tasks against uncaught exceptions cancelling schedules #7765: doSyncDB() now keeps the per-instance try/catch granularity over a defensive snapshot copy (copyInstanceInfoVO(vo)), ensuring a single row failure does not abort subsequent records while preserving snapshot isolation.
2. Bounded shutdown lock: Replaced synchronized with ReentrantLock and added syncDB(timeout, unit). close() now uses a 5-second tryLock bounded timeout to prevent blocking @PreDestroy indefinitely if a sync is stalled on JDBC.
3. Prevent executor leak on re-election: Added closeExecutors() at the beginning of setup() to shut down existing executors before re-initializing.
4. Tests & verification: Added unit tests covering per-instance error handling, executor shutdown on re-election, and bounded shutdown lock acquisition. All tests pass with 0 checkstyle violations.

[konlue]

@harshkha3417 PTAL. Heads-up for after #7742 (approved — adds id to InstanceInfoVO ) merges: the copyInstanceInfoVO snapshot introduced here enumerates all VO fields and will leave the new id null. Harmless today ( createOrUpdate never reads the VO id), but to keep the complete-copy invariant, please add copy.setId(vo.getId()) once #7742 lands — happy to follow up with the one-liner myself if you prefer.

[Aias00]

Approved.

Good isolation fix for #7744. Splitting the DB sync into its own single-thread executor means a slow or hung syncDB can no longer block the heartbeat liveness check that shared the original executor. The ReentrantLock (with tryLock and a timed variant) prevents overlapping syncs, doSyncDB() snapshots and copies each VO to decouple the DB layer from concurrent cache mutations, and setup() now closes stale executors on re-election to avoid leaks. close() shuts both executors and performs a bounded final sync. Tests cover executor isolation, re-election cleanup, and the non-blocking timed-lock path.

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

[harshkha3417]

Thanks for the heads-up @konlue!

Understood — since #7742 hasn't landed on master yet, id isn't on InstanceInfoVO today. If #7742 merges before this PR, I'll gladly rebase and add copy.setId(vo.getId()); if this PR merges first, feel free to add the one-liner alongside #7742 as you offered. Thanks again!

[harshkha3417]

All CI checks have passed successfully. The PR is ready for merge whenever you have a moment, @Aias00. Thank you!

[Aias00]

Re-approving after I rebased this branch onto current master (the force-push dismissed the previous approval).

What I did:

• Rebased the 3 commits onto master (de93952) as the maintainer, since #7785 and #7721 had touched the same file and left the PR CONFLICTING.
• The only conflict was a single import block in InstanceCheckServiceTest.java: master added org.apache.shenyu.common.utils.GsonUtils, this PR added org.apache.shenyu.common.enums.InstanceStatusEnum. Both are used (1 and 2 references respectively), so I kept both in checkstyle alphabetical order.
• InstanceCheckService.java auto-merged cleanly. I confirmed the two sides are disjoint: master changes needlessHeartBeat/metadata refresh only, this PR changes setup, syncDB/doSyncDB, close and adds the syncExecutor/syncLock fields.
• Verified no conflict markers remain, authorship preserved on all three commits, and the diff versus master is unchanged in substance (81 lines in InstanceCheckService.java, 158 in the test).

The original review stands: splitting the DB sync into its own single-thread executor so a slow syncDB cannot block the heartbeat liveness check, ReentrantLock with tryLock to prevent overlapping syncs, snapshot-and-copy in doSyncDB, and setup() closing stale executors on re-election to avoid leaks. Functional review unaffected by the rebase.

[harshkha3417]

CI Failure Root Cause & Resolution

The failure in the CI build and prbuild checks was caused by an upstream conflict between two recently merged commits on master:

1. Root Cause:

   • [ISSUE #7716] Normalize instance information across reporting paths. #7785 (7086afaff): Introduced canonical JSON normalization via InstanceInfoUtils.normalize(), which normalizes non-JSON text payloads to "{}".
   • fix(admin): refresh instance metadata on repeated heartbeats (#7692) #7721 (315bcbc3d): Added tests in InstanceCheckServiceTest using plain text ("version=1.0"). On the initial beat, InstanceCheckService normalized "version=1.0" to "{}", leading to test failures when asserting against "version=1.0".
   • Additionally, heartbeat refreshes for already-cached instances in InstanceCheckService.handleBeatInfo() were setting raw strings without calling InstanceInfoUtils.normalize().

2. Fix:

   • Applied InstanceInfoUtils.normalize(...) to heartbeat updates on existing identities in InstanceCheckService.handleBeatInfo().
   • Updated the test payloads in InstanceCheckServiceTest (testHandleBeatInfoRefreshesInstanceInfoOnExistingIdentity and testHandleBeatInfoKeepsInstanceInfoWhenBeatCarriesBlank) to use valid JSON object strings ("{\"version\":\"1.0\"}" and "{\"version\":\"2.0\"}").

[harshkha3417]

@Aias00 CI run approval is needed...

[Aias00]

Re-reviewed at 0e6bef7b. The scheduler isolation is the right fix for #7744 and the implementation is sound. One coordination problem needs your attention before this can merge.

What I verified:

• syncExecutor is now a separate single-thread ScheduledThreadPoolExecutor, so a slow instanceInfoService.createOrUpdate can no longer delay the heartbeat sweep. That is exactly the issue.
• setup() calls closeExecutors() first, so a second setup() (cluster mode re-election after losing leadership) does not leak the previous pair of executors. shutdown() without awaitTermination is acceptable here because both pools are single-threaded and the tasks are short.
• syncDB(timeout, unit) guards re-entry with ReentrantLock#tryLock and only unlocks when the lock was actually acquired — the locked flag makes the finally correct on InterruptedException.
• doSyncDB() iterates a snapshot and hands copyInstanceInfoVO(vo) to the service, so a heartbeat arriving mid-sync cannot mutate the object being persisted. Good call, this was a real race.
• Per-entry try/catch keeps one bad instance from aborting the whole sweep.
• close() closes the executors before the final flush, so no new sweep can start while the last one runs, and instanceHealthBeatInfo.clear() is in a finally.

Non-blocking notes:

• copyInstanceInfoVO() hand-copies nine fields. Any field added to InstanceInfoVO later will be silently dropped from persistence. A copy constructor on InstanceInfoVO, or a BeanUtils.copyProperties with an explicit ignore list, would fail loudly instead.
• close() relies on the lock with a 5s budget; if the periodic sweep is stuck in a slow DB call, the flush is skipped and the exception is swallowed by doSyncDB(). LOG.warn when syncDB(5, SECONDS) returns false during shutdown would make that visible.

Conflict with #7765

#7765 touches the same InstanceCheckService lines: the handleBeatInfo normalize call and a simplified syncDB() that wraps each createOrUpdate in try/catch but has no lock, no snapshot and no separate executor. Whichever lands second will conflict, and if #7765 lands second it would partially revert this design. Please coordinate so this PR is the one that owns syncDB(), and #7765 keeps only the scheduler-hardening part.

CI

There is no workflow run for this branch: GET /repos/apache/shenyu/actions/runs?head_sha=0e6bef7b returns total_count: 0 and gh pr checks reports nothing. Same for your other open PRs, so Actions appears to be disabled on your fork. Please enable it and push again — without a pr_build run this cannot pass the merge gate.

[harshkha3417]

Thanks @Aias00 for the review!

I have addressed all review comments in this PR (which owns InstanceCheckService):

1. Shutdown Timeout Visibility: In InstanceCheckService.close(), added a LOG.warn(...) alert when syncDB(5, TimeUnit.SECONDS) returns false due to lock acquisition timeout.
2. Field Coverage in Copy: Added copy.setId(vo.getId()) to copyInstanceInfoVO to ensure full field fidelity, and added testCopyInstanceInfoVO() test coverage.
3. Rebased onto latest master.

[harshkha3417]

/retest

[Aias00]

LGTM. Splitting the db sync onto its own executor is the right fix — scheduleWithFixedDelay on a single-thread pool meant a slow createOrUpdate stalled the liveness check, which is exactly the false-offline path this PR targets.

Verified:

• syncDB(timeout, unit) returns the lock state and doSyncDB swallows per-row failures so one bad instance no longer aborts the whole cycle.
• close() now shuts the executors down first and bounds the final sync with a 5s tryLock, so shutdown cannot hang on a hung DB write.
• setup() closing stale executors before recreating them fixes the re-election leak.
• Copying the VO before createOrUpdate avoids handing a live map entry to the persistence layer.
• InstanceInfoUtils.normalize exists on master and the imports are all present.

Nit: the per-cycle copyInstanceInfoVO allocation is O(instances) every 10s. Fine for now, but a reuse buffer would avoid the churn.

[Aias00]

Approved, but the checks are red on a shared e2e case rather than on this change.

build (shenyu-integrated-test-http) fails in CachePluginTest, and it is not caused by this PR:

[ERROR] CachePluginTest.testMemoryModle:54 expected: <true> but was: <false>
[ERROR] CachePluginTest.testRedisModle:73  expected: <true> but was: <false>
[ERROR] Tests run: 103, Failures: 2, Errors: 0, Skipped: 5

The same two cases are red on every other PR that touches this module right now, so this is the shared e2e case rather than your change. build is only an aggregate gate, so a single shared case turns the whole check red.

Could you keep this rebased and re-run once the http e2e case is fixed upstream? Nothing else blocks my approval.

[yuluo-yx]

/retest

[harshkha3417]

@Aias00 @yuluo-yx Conflict is resolved now...