[hengyuss]

Convert registry instance events into complete upstream snapshots after
updating the database. Previously, syncData contained only the changed
instance, which could cause snapshot reconciliation to remove other
valid upstreams. Publish empty snapshots when the last instance is removed.

Use the health-check lock for cache updates and removal. This prevents
health checks from moving upstreams between healthy and unhealthy caches
during reconciliation, causing removed instances to survive cleanup.

Add regression tests for snapshot publication, stale upstream removal,
health-state preservation, and concurrent health checks.

Fixes: #7165

Make sure that:

• You have read the contribution guidelines.
• You submit test cases (unit or integration tests) that back your changes.
• Your local test passed ./mvnw clean install -Dmaven.javadoc.skip=true.

[Aias00]

Review

Solid two-part fix with good test coverage.

Admin (DiscoveryDataChangedEventSyncListener) — registry events carry only one instance, but gateways consume a complete snapshot. The listener now re-queries the full stored list (selectByDiscoveryHandlerId) and publishes that as the event payload, so a single-instance change no longer clobbers the whole gateway snapshot. The testDeletingLastInstancePublishesEmptySnapshot case is a nice touch.

Loadbalancer (UpstreamCacheManager / UpstreamCheckTask) — submit / removeByKey now run under a shared lock with health checks, and removeStaleUpstreams purges nodes missing from the new snapshot from the healthy/unhealthy maps as well (not just the main UPSTREAM_MAP). The concurrency test testConfigurationUpdateWaitsForHealthCheck convincingly proves a config update blocks while an in-flight health check holds the lock.

Non-blocking considerations (not blockers):

• Publishing a full snapshot on every registry event (ADDED/UPDATED/DELETED) means a DB re-query + full payload per instance change. Fine at typical scale, but worth watching under high churn.
• withLock serializes all config updates with health checks, so a slow health check now stalls config propagation. Acceptable trade-off for correctness — just be aware of the latency coupling.

Approving.

[Aias00]

🟠 Non-blocking: this re-queries the DB and republishes the full upstream snapshot on every registry event (ADDED/UPDATED/DELETED). Under high instance churn that's one selectByDiscoveryHandlerId + one full payload per individual change. Correct, and fine at typical scale — just flagging it as the thing to watch if a registry starts flapping.

[Aias00]

🟠 Non-blocking: withLock serializes all config updates (submit/removeByKey) with health checks on the shared lock. A slow health-check pass now stalls config propagation until it releases. Reasonable trade-off for correctness — just be aware of the latency coupling between health checks and config updates.

[Aias00]

Re-reviewed: reconciling upstream snapshots without retaining removed references, plus letting the check task drive the reconciliation, fixes stale upstreams lingering in the cache. Tests included; the red jobs are the known flakes — approving.

[sunnysabor]

The latest head 0830d53 fails pr_build before tests run: Checkstyle reports DiscoveryDataChangedEventSyncListenerTest.java:166:22 mismatched input ( expecting ;. In the current file, prepareRegistryEvent() opens at line 164 but is not closed after the contextInfo.getSelectorId() stubbing; the following @Test testOnChangeIsSafeWhenListenerIsAddedConcurrently is therefore parsed as a declaration inside that method. Please add the missing } before the next @Test, then rerun the module checks and CI. The earlier approval predates this merge commit and does not cover this head.

[sunnysabor]

The latest head resolves the previous test-method syntax error, and the snapshot/cache tests pass in isolation. I found one remaining build blocker: DiscoveryDataChangedEventSyncListenerTest imports org.mockito.Mockito.* at line 52, which violates this repository’s Checkstyle AvoidStarImport rule. Reproduced with ./mvnw -B -ntp -pl shenyu-admin,shenyu-loadbalancer -am -Dtest=UpstreamCacheManagerTest,UpstreamCheckTaskTest,DiscoveryDataChangedEventSyncListenerTest -DfailIfNoTests=false test; Checkstyle fails on that import before the admin tests run. Please replace it with the explicit Mockito static imports used by this test and rerun the focused reactor.

[sunnysabor]

Re-reviewed current head a2e0dde0. The requested explicit Mockito static imports are now in place. Local validation passed with ./mvnw -B -ntp -T 1C -pl shenyu-admin,shenyu-loadbalancer -am -Dtest=UpstreamCacheManagerTest,UpstreamCheckTaskTest,DiscoveryDataChangedEventSyncListenerTest -Dsurefire.failIfNoSpecifiedTests=false -Djacoco.skip=true -Dmaven.javadoc.skip=true -Drat.skip=true test: 25 tests passed and Checkstyle reported 0 violations. The hosted pr_build is still blocked before the admin module by the unchanged HeartbeatFailureIsolationTest in shenyu-client-core (the #7401 baseline issue); the changed tests pass in the focused reactor. The upstream snapshot reconciliation and health-check concurrency changes remain covered by the previously reviewed tests.

[Aias00]

The full-snapshot reconciliation fixes stale upstream retention. Please monitor the cost of querying and publishing a full snapshot per registry event and the shared locking with health checks; current build failures also need resolution before merge.

[Aias00]

Reviewed current head a2e0dde0c7e0.

The stale-upstream reconciliation design is implemented, prior build blockers are fixed, and exact-head targeted admin/loadbalancer tests passed.

Architecture note: The snapshot correctness fix is present. Full database snapshots per event and shared-lock latency remain non-blocking operational considerations.

Validation: exact-head targeted Maven reactor tests passed on Java 17.

./mvnw -pl shenyu-admin,shenyu-loadbalancer -am test -Dtest=DiscoveryDataChangedEventSyncListenerTest,UpstreamCacheManagerTest,UpstreamCheckTaskTest -Dsurefire.failIfNoSpecifiedTests=false -Drat.skip=true -Djacoco.skip=true -DskipRemoteResources=true -Dmaven.javadoc.skip=true

GitHub still reports failing checks: build, pr_build. This review does not claim those checks have passed.

[Aias00]

Re-reviewing after the branch was cleaned up. My earlier Changes Requested was only about mergeability/CI state, which is now resolved, so this re-approval is on the substantive change.

The fix is solid:

• UpstreamCacheManager.submit now reconciles against the new snapshot via removeStaleUpstreams, dropping upstreams that are no longer present from UPSTREAM_MAP, healthy and unhealthy maps — this is the real fix for "retaining removed instances".
• The config update path is now serialized with health checks through UpstreamCheckTask.withLock (synchronized(lock)), and removeByKey/submit are guarded by it. The new testConfigurationUpdateWaitsForHealthCheck validates the mutual exclusion.
• DiscoveryDataChangedEventSyncListener.syncData0 re-publishes a complete snapshot from DB after each registry event, which is the correct contract for gateway consumers.

Tests are thorough (parameterized ADDED/UPDATED/DELETED, delete-last-instance empty snapshot, stale removal from healthy/unhealthy, concurrency guard).

Non-blocking note: publishing a full DB-backed snapshot on every individual registry event adds one selectByDiscoveryHandlerId round-trip per instance change. Under very high churn this is a per-event DB query; if it shows up in profiling you may want to debounce or only refetch on DELETED. Not a blocker.

APPROVED.

[Aias00]

Hi @hengyuss, this PR currently has merge conflicts with the master base branch, so CI cannot validate it against the latest code. Please rebase/merge the latest master and resolve the conflicts, then push the updated branch. Once the conflicts are cleared, CI will re-run automatically and we can continue the review. Thanks!

[Aias00]

Summary

Fixes #7165. This PR fixes upstream snapshot reconciliation so that removed discovery instances do not survive in the gateway cache. Two related changes: (1) DiscoveryDataChangedEventSyncListener now publishes a complete upstream snapshot from the database (all instances for the handler) rather than relying on the single changed instance, and publishes an empty snapshot when the last instance is removed; (2) UpstreamCacheManager.submit/removeByKey now run under a health-check lock, and submit actively removes stale upstreams (present in UPSTREAM_MAP, healthyUpstream, or unhealthyUpstream but absent from the incoming snapshot). UpstreamCheckTask.withLock provides the shared lock.

What the change does

• DiscoveryDataChangedEventSyncListener.java — added a clarifying comment; the listener already rebuilds the snapshot from discoveryUpstreamMapper.selectByDiscoveryHandlerId(...), so syncData.setUpstreamDataList(...) is the full set.
• UpstreamCacheManager.java:
  • removeByKey (line 138) and submit (line 151) now wrap their bodies in task.withLock(...).
  • submit delegates to a new doSubmit, which calls a new removeStaleUpstreams(selectorId, actualUpstreamList, existUpstreamList) (line 181) that strips any upstream not in the incoming snapshot from UPSTREAM_MAP, healthyUpstream, and unhealthyUpstream.
• UpstreamCheckTask.java:318 — new void withLock(Runnable) using synchronized (lock); the existing removeFromMap/triggerRemoveOne/triggerRemoveAll are already synchronized (lock), so the nested calls are reentrant.
• Tests: DiscoveryDataChangedEventSyncListenerTest (complete-snapshot + empty-snapshot publishing), UpstreamCacheManagerTest (snapshot removes missing healthy/unhealthy upstreams; preserves remaining unhealthy), UpstreamCheckTaskTest (config update waits for health check under the lock).

Verification

• Master-vs-head: UpstreamCheckTask gained withLock (master had only lock + getCheckStarted); UpstreamCacheManager gained doSubmit/removeStaleUpstreams; lock field (Object lock = new Object()) and all triggerRemove*/removeFromMap are synchronized (lock) — confirmed at head, so withLock calls are reentrant and deadlock-free.
• submit receives a complete snapshot: DivideUpstreamDataHandler.handlerDiscoveryUpstreamData submits discoverySyncData.getUpstreamDataList() (the full per-selector set from the listener), so removeStaleUpstreams does not wrongly drop valid upstreams. The gray-only branch (line 54) is consistent with the existing "submit replaces the selector's set" contract.
• Local unit tests (run from the head tree): UpstreamCacheManagerTest — 14 tests, 0 failures; UpstreamCheckTaskTest — 11 tests, 0 failures.
• CI: all relevant checks PASS — pr_build, Analyze (java), check-license-header, rat_check, scope, sql-matrix, agent_gateway_migrations. The build (shenyu-integrated-test-k8s-ingress-*) jobs failed at the "Build k8s Cluster" step (infrastructure provisioning), and the remaining e2e-* failures are the known flakes (e2e-logging-rocketmq-compose, *-sync-compose, e2e-storage httpbin.org). None fail in a code path this PR touches.

Notes (non-blocking)

• removeStaleUpstreams compares by upstreamMapKey; if two upstreams share a key but differ only in health state, they are treated as the same entry. The provided testSnapshotPreservesRemainingUnhealthyUpstream covers the unhealthy-preservation scenario, so this is acceptable.
• Consider a short code comment on removeStaleUpstreams explaining that it relies on submit always receiving the authoritative full snapshot (already true for the discovery path, but worth stating for future callers).

Conclusion

APPROVE. The fix correctly reconciles upstream snapshots under a shared lock, removes stale instances without deadlock, and is backed by passing regression tests and a clean relevant-CI result. The only CI reds are k8s-cluster provisioning and known e2e flakes, unrelated to this code.

[Aias00]

Ping on state: approved on the code, but the branch conflicts with master and the k8s-ingress / e2e checks are red.

Please rebase onto master and push. If the e2e failures persist after the rebase, check whether they are the shared e2e flakiness tracked in #7970 before digging into this PR — the build job here only fails because it gates on the e2e result.

[Aias00]

Re-approving after the merge. The conflict resolution in UpstreamCacheManager is clean — submit delegates to doSubmit under withLock, and removeStaleUpstreams runs before updateUpstreamMetadata, so the preserved-metadata path never sees the entries that are about to be dropped.

I verified the locking is sound rather than just plausible: every mutator in UpstreamCheckTask (putToMap, removeFromMap, triggerRemoveAll) synchronizes on the same lock object, and it is an intrinsic monitor, so calling removeFromMap / triggerRemoveOne from inside withLock re-enters safely. There is no this-vs-lock inversion, so no deadlock.

One thing to be aware of, which is a deliberate trade-off rather than a defect: check() holds lock across doHealthCheck(...) and waitFinish() (lines 189-195), i.e. an entire probe round bounded by checkTimeout. Now that submit and removeByKey take the same lock, a configuration update can block behind a full health-check round. That is precisely what makes the snapshot atomic, but on the data-sync path the worst case is now a whole round rather than a map operation. If that turns out to matter, narrowing the critical section (or at least documenting the bound) is a reasonable follow-up.

Test nit: both new tests wrap submit inside task.withLock(...), so they pin the stale-removal behaviour but not that submit itself acquires the lock — the atomicity claim is asserted by construction rather than observed. A test that calls submit bare and races it against a round would close that gap.

Approving.