[lymerin]

Fixes #6479

What does this PR do?

This PR fixes stale discovery upstream data remaining in the gateway after a discovery-bound selector is deleted.

The main changes are:

• handle the existing selector-level DISCOVER_UPSTREAM DELETE event through path-based sync;
• handle ZooKeeper node deletion events correctly;
• reconcile HTTP discovery upstream snapshots so removed selectors are unsubscribed;
• forward upstream removal events to plugin handlers;
• remove stale upstream state from Divide, WebSocket, gRPC, and TCP plugins;
• add TCP selector id-to-name mapping for deletion handling;
• validate plugin names before publishing discovery upstream removal events from Admin.

If Admin cannot resolve the plugin name for a discovery-bound selector, it rejects the entire selector-delete batch with ShenyuAdminException before deleting rows or publishing removal events. The error identifies the affected selector and states that no selectors in the batch were deleted.

The existing selector-level DISCOVER_UPSTREAM DELETE event also propagates through Nacos, etcd, Consul, Polaris, and Apollo. Etcd and Consul use the shared path-based handler; Nacos, Polaris, and Apollo use the shared node-based handler.

Event semantics and relationship to #7172

Upstream instance changes and selector removal are separate flows:

• Upstream instance changes: registry ADDED, UPDATED, and DELETED events are published as DISCOVER_UPSTREAM UPDATE. [#7172]( fix: reconcile upstream snapshots without retaining removed instances #7172) proposes publishing the complete remaining upstream list, including an empty list after the last instance is removed. This PR does not change that producer or turn an individual instance deletion into DISCOVER_UPSTREAM DELETE.
• Selector-level discovery removal: SelectorServiceImpl#unbindDiscovery already calls DiscoveryProcessor#removeSelectorUpstream, which publishes DISCOVER_UPSTREAM DELETE for that selector’s discovery-upstream record. This PR fixes the gateway’s handling of that existing event. It does not introduce a new DELETE producer or replace SELECTOR DELETE or PROXY_SELECTOR DELETE.

For HTTP full snapshots, DiscoveryUpstreamDataRefresh compares the previous and current selector identities and unsubscribes records that disappear or are renamed. An existing selector with an empty upstream list remains in the Admin snapshot and is processed as an update, not mistaken for a removed selector.

The DiscoveryUpstreamDataSubscriber#unSubscribe(DiscoveryUpstreamKey) API change is documented in RELEASE-NOTES.md.

Why is this needed?

Before this change, the existing discovery upstream deletion event was ignored by path-based gateway sync or reached a no-op unsubscribe handler. Deleted selectors could therefore leave gateway-side upstream state behind and continue to affect routing.

HTTP full-snapshot synchronization also needed to detect discovery-upstream selector records that disappeared from the latest snapshot.

Tests

Added or updated tests for:

• path-based discovery upstream deletion;
• ZooKeeper delete event propagation;
• HTTP snapshot reconciliation when one selector disappears or the snapshot becomes empty;
• plugin upstream cache removal;
• TCP selector id/name resolution and cleanup;
• gRPC cache/client invalidation.

Make sure that:

• You have read the [contribution guidelines](https://shenyu.apache.org/community/contributor-guide).
• You submit test cases (unit or integration tests) that back your changes.
• Your local test passed ./mvnw clean install -Dmaven.javadoc.skip=true.

[Aias00]

Thanks for taking on #6479 — stale discovery upstreams surviving a selector deletion is one of those bugs that is invisible in tests and painful in production, and this PR wires the whole chain end to end rather than patching one hop:

• admin side resolves (and now requires) the plugin name before publishing a removal (AbstractPathDataChangedListener, SelectorServiceImpl), which is the root cause of the malformed .../discoveryUpstream//<selectorId> paths;
• gateway side finally acts on a removal, per protocol: divide/websocket drop their UpstreamCacheManager entry, gRPC invalidates ApplicationConfigCache/GrpcClientCache, and TCP gets the selector-id → selector-name mapping it needed (UpstreamProvider#registerSelector / getSelectorName) because the upstream cache there is keyed by name while the sync key carries the id;
• the three sync channels are covered differently and correctly for each: ZK now uses oldData for NODE_DELETED when the payload is gone, the websocket handler maps doDelete to DiscoveryUpstreamKey, and the HTTP refresh diffs the previous snapshot so a selector that disappears from the config (or whose name changed) is properly removed.

The DiscoveryUpstreamKey record is the right abstraction — it captures exactly what a removal needs (plugin + id + optional name) instead of smuggling a partially-filled DiscoverySyncData around, and it is why CommonDiscoveryUpstreamDataSubscriber#unSubscribe can finally be implemented instead of being an //ignore.

Requesting changes for two things

1. The branch has conflicts and zero CI evidence. GitHub reports no workflow runs on fix/6479-discovery-upstream-delete-cache and the merge state is dirty. This patch spans seven modules (admin listener api, admin, plugin-base, divide, grpc, tcp, websocket, protocol-tcp, sync-data-api, http, websocket, zookeeper) and changes delete semantics in production paths, so please rebase onto current master and get a green pr_build before this is merged — I cannot approve it on reading alone.

2. SelectorServiceImpl#unbindDiscovery now throws IllegalStateException (SelectorServiceImpl.java:332). The intent — "do not publish a removal we cannot address" — is right, and validating the whole batch before touching any row (the two-phase ResolvedDiscovery structure) is a nice touch. But the consequence is that a batch delete that contains one selector with unresolvable plugin metadata now fails and rolls back entirely, and the admin REST layer sees a raw IllegalStateException rather than a typed error. Given that this can happen from stale/orphan discovery rows, users could end up unable to delete selectors at all. Options:

• degrade gracefully: skip the discovery unbind for that selector, log at ERROR, and continue deleting the rest of the batch (the row-level cleanup then still happens for the healthy ones), or
• keep failing, but throw a typed admin exception (the ones already mapped to a clean error response) instead of IllegalStateException.

Either is fine for me; please pick one and make the behaviour visible in the message shown to operators.

Worth noting, not blocking

• Public API change. DiscoveryUpstreamDataSubscriber#unSubscribe(DiscoverySyncData) became unSubscribe(DiscoveryUpstreamKey). I checked every occurrence in the repo: CommonDiscoveryUpstreamDataSubscriber is the only implementation and every other call site goes through the sync services you already updated, so the build is fine — but this is source-incompatible for anyone implementing the interface downstream, so it belongs in the release notes.
• Coverage of the other sync channels. Discovery-upstream deletion is now propagated only over zookeeper, websocket and http. Nacos, etcd, consul, polaris and apollo still have no delete path for this group (pre-existing). A short note in the PR description or a follow-up issue would help whoever hits it next.
• The blank-pluginName guard in AbstractPathDataChangedListener is the right safety net; selectorId has the same failure shape (buildDiscoveryUpstreamPath would produce .../divide/null). No logging detail lost if you extend the same check to it.
• In DiscoveryUpstreamDataRefresh#refresh, an empty snapshot now triggers subscriber.refresh() and an unSubscribe per previously known key. Correct, just slightly redundant work; also note previousSnapshot being a plain HashMap is fine only because you made refresh synchronized — worth a comment so nobody removes the modifier later.
• TcpUpstreamDataHandler registering the selector id → name mapping inside handlerDiscoveryUpstreamData is the piece that makes removeDiscoveryUpstreamData work after a gateway restart with no prior selector event. Good; please keep the invariant documented on UpstreamProvider#registerSelector.

Once the branch is rebased with green CI and the IllegalStateException handling is settled, I am happy to take another look.

[lymerin]

Thanks for taking on #6479 — stale discovery upstreams surviving a selector deletion is one of those bugs that is invisible in tests and painful in production, and this PR wires the whole chain end to end rather than patching one hop:

• admin side resolves (and now requires) the plugin name before publishing a removal (AbstractPathDataChangedListener, SelectorServiceImpl), which is the root cause of the malformed .../discoveryUpstream//<selectorId> paths;
• gateway side finally acts on a removal, per protocol: divide/websocket drop their UpstreamCacheManager entry, gRPC invalidates ApplicationConfigCache/GrpcClientCache, and TCP gets the selector-id → selector-name mapping it needed (UpstreamProvider#registerSelector / getSelectorName) because the upstream cache there is keyed by name while the sync key carries the id;
• the three sync channels are covered differently and correctly for each: ZK now uses oldData for NODE_DELETED when the payload is gone, the websocket handler maps doDelete to DiscoveryUpstreamKey, and the HTTP refresh diffs the previous snapshot so a selector that disappears from the config (or whose name changed) is properly removed.

The DiscoveryUpstreamKey record is the right abstraction — it captures exactly what a removal needs (plugin + id + optional name) instead of smuggling a partially-filled DiscoverySyncData around, and it is why CommonDiscoveryUpstreamDataSubscriber#unSubscribe can finally be implemented instead of being an //ignore.

Requesting changes for two things

1. The branch has conflicts and zero CI evidence. GitHub reports no workflow runs on fix/6479-discovery-upstream-delete-cache and the merge state is dirty. This patch spans seven modules (admin listener api, admin, plugin-base, divide, grpc, tcp, websocket, protocol-tcp, sync-data-api, http, websocket, zookeeper) and changes delete semantics in production paths, so please rebase onto current master and get a green pr_build before this is merged — I cannot approve it on reading alone.

2. SelectorServiceImpl#unbindDiscovery now throws IllegalStateException (SelectorServiceImpl.java:332). The intent — "do not publish a removal we cannot address" — is right, and validating the whole batch before touching any row (the two-phase ResolvedDiscovery structure) is a nice touch. But the consequence is that a batch delete that contains one selector with unresolvable plugin metadata now fails and rolls back entirely, and the admin REST layer sees a raw IllegalStateException rather than a typed error. Given that this can happen from stale/orphan discovery rows, users could end up unable to delete selectors at all. Options:

• degrade gracefully: skip the discovery unbind for that selector, log at ERROR, and continue deleting the rest of the batch (the row-level cleanup then still happens for the healthy ones), or
• keep failing, but throw a typed admin exception (the ones already mapped to a clean error response) instead of IllegalStateException.

Either is fine for me; please pick one and make the behaviour visible in the message shown to operators.

Worth noting, not blocking

• Public API change. DiscoveryUpstreamDataSubscriber#unSubscribe(DiscoverySyncData) became unSubscribe(DiscoveryUpstreamKey). I checked every occurrence in the repo: CommonDiscoveryUpstreamDataSubscriber is the only implementation and every other call site goes through the sync services you already updated, so the build is fine — but this is source-incompatible for anyone implementing the interface downstream, so it belongs in the release notes.
• Coverage of the other sync channels. Discovery-upstream deletion is now propagated only over zookeeper, websocket and http. Nacos, etcd, consul, polaris and apollo still have no delete path for this group (pre-existing). A short note in the PR description or a follow-up issue would help whoever hits it next.
• The blank-pluginName guard in AbstractPathDataChangedListener is the right safety net; selectorId has the same failure shape (buildDiscoveryUpstreamPath would produce .../divide/null). No logging detail lost if you extend the same check to it.
• In DiscoveryUpstreamDataRefresh#refresh, an empty snapshot now triggers subscriber.refresh() and an unSubscribe per previously known key. Correct, just slightly redundant work; also note previousSnapshot being a plain HashMap is fine only because you made refresh synchronized — worth a comment so nobody removes the modifier later.
• TcpUpstreamDataHandler registering the selector id → name mapping inside handlerDiscoveryUpstreamData is the piece that makes removeDiscoveryUpstreamData work after a gateway restart with no prior selector event. Good; please keep the invariant documented on UpstreamProvider#registerSelector.

Once the branch is rebased with green CI and the IllegalStateException handling is settled, I am happy to take another look.

Addressed each point:

1. Resolved the conflicts against master; pr_build passed.
2. Kept batch deletion atomic, replaced IllegalStateException with ShenyuAdminException, and made the error state that no selectors in the batch were deleted.
3. Documented the unSubscribe API change in the release notes.
4. Clarified in the PR description that Nacos, etcd, Consul, Polaris, and Apollo do have discovery-upstream deletion paths through the shared handlers.
5. Added the blank selectorId guard.
6. Documented why previousSnapshot uses HashMap under synchronized refresh.
7. Retained TCP’s selector ID-to-name registration for deletion after a gateway restart.

[Aias00]

Thanks for the follow-up — both blocking points are resolved, so flipping to approve.

What I re-checked

1. Branch state / CI. mergeable: MERGEABLE against master, and the last commit 4bdda53 is a merge of master rather than a stale branch, so the "dirty, zero runs" situation I objected to is gone. Workflows are running on that sha (ci, it, it-k8s, e2e, k8s-examples-http, CodeQL) with no failures so far.

2. SelectorServiceImpl#unbindDiscovery. Now throws ShenyuAdminException instead of IllegalStateException, and I confirmed this is actually the "typed exception mapped to a clean response" option rather than just a rename: ShenyuAdminException extends ShenyuException, and ExceptionHandlers:124-132 has @ExceptionHandler(ShenyuException.class) returning ShenyuAdminResult.error(message) — so the operator sees Cannot delete selector batch: plugin name for selector <id> ... No selectors in this batch were deleted; restore the plugin name and retry instead of a 500 with a stack trace. That is exactly the visibility I asked for. Keeping the batch atomic is fine now that the failure is explicit and actionable, and testDeleteValidatesWholeBatchBeforeUnbindingDiscovery proves no rows move for the healthy selectors either.

Two observations worth recording, not requests:

• The resolution chain is wider than before, in a good way. pluginMap -> pluginMapper.selectById -> discoveryDO.getPluginName() means the case that motivated my concern (orphaned discovery rows whose plugin row is gone) mostly resolves instead of failing; the exception is now a last resort rather than a common path.
• The pluginDOS mutation closes a latent NPE. SelectorEventPublisher:98-99 does pluginMap.get(...).equals(...) unguarded, so a selector whose plugin id was absent from the batch list used to NPE on delete. Injecting the synthetic PluginDO(id, name) at :338-343 fixes that too. This is also why pluginMapper.selectByIds(...) is now copied into a mutable ArrayList at :302 — worth keeping in mind for whoever later tries to "simplify" that copy.

3. Non-blocking items. The blank-selectorId guard (AbstractPathDataChangedListener:101) is in, with namespaceId added to the warning, and the 5 focused tests cover DELETE and UPDATE for each blank field. previousSnapshot now carries the // Only the synchronized refresh method accesses this shared mutable snapshot comment. UpstreamProvider#registerSelector keeps the id -> name mapping; its javadoc says what it does but not why the invariant matters (deletion after a restart with no prior selector event) — one more line there would help the next reader. Release notes carry the unSubscribe signature change.

Behavioural notes worth having in the record

• DiscoveryUpstreamDataRefresh#refresh is now synchronized and diffs previousSnapshot against the incoming snapshot, so a selector that disappears or is renamed emits unSubscribe with the previous key. The rename case matters because TCP keys its upstream cache by selector name — see TcpUpstreamDataHandler#removeDiscoveryUpstreamData, which prefers the locally registered getSelectorName(selectorId) and falls back to key.selectorName().
• TcpBootstrapFactory now drops upstream state in three more places (the createBootstrapServerIfAbsent failure path, removeAndShutdown, clearCache), which keeps UpstreamProvider from outliving its BootstrapServer.
• AbstractPathDataSyncService#discoveryUpstreamHandlerEvent handles DELETE explicitly by rebuilding the key from the path segments ([length-2] plugin, [length-1] selector) instead of waiting for a payload that is already gone — that is the piece that makes the path-based channels work on delete.
• CommonDiscoveryUpstreamDataSubscriber#onSubscribe/unSubscribe now guard null plugin names, so a malformed event degrades to a no-op instead of a handlerMap.get(null) lookup.

Coverage stays as before: zookeeper, websocket and http have explicit delete paths, the rest propagate through the shared handlers. Nothing here blocks merge.

[lymerin]

The RocketMQ logging e2e failed because testRocketMQHello did not consume the access log within 30 seconds: [failed job](https://github.com/apache/shenyu/actions/runs/36415934554/job/108915531794). The same PR changes passed e2e before the latest master merge: [successful run](https://github.com/apache/shenyu/actions/runs/36309256388).

Could a maintainer rerun the failed e2e workflow? I won't push an unrelated change just to retrigger CI. If it fails again, I'll investigate the reproducible failure before requesting a merge.

[hengyuss]

Thanks for working on this issue.

After comparing this PR with #7172, I do not think the new DISCOVER_UPSTREAM DELETE chain is the correct way to
remove discovery upstreams.

PR #7172 establishes discovery upstream synchronization as snapshot reconciliation:

Registry ADDED / UPDATED / DELETED
    -> Admin updates the database
    -> Admin queries the complete remaining upstream list
    -> Publish DISCOVER_UPSTREAM UPDATE
    -> Gateway reconciles its local cache with the snapshot

This means a registry DELETED event is an internal Admin-side event. It should not be propagated to the gateway as an
upstream DELETE event.

For example:

Previous upstreams: [A, B]
Delete A
Published snapshot: [B]

The gateway removes A by comparing [A, B] with [B].

Deleting the last instance should still publish an empty UPDATE snapshot:

Previous upstreams: [A]
Delete A
Published snapshot: []

The gateway then clears the selector's upstream cache through the same reconciliation path.

Therefore, the following chain introduced by this PR represents the wrong abstraction for upstream instance deletion:

DISCOVER_UPSTREAM DELETE
-> DiscoveryUpstreamDataSubscriber.unSubscribe()
-> DiscoveryUpstreamDataHandler.removeDiscoveryUpstreamData()

There are two different concepts that should not be mixed:

1. An upstream instance is removed

   This should be handled by a complete DISCOVER_UPSTREAM UPDATE snapshot, as implemented by fix: reconcile upstream snapshots without retaining removed instances #7172.

2. An entire selector is removed

   Incremental synchronization already publishes SELECTOR DELETE, which invokes the plugin's removeSelector() method.
   Divide, WebSocket, and gRPC already clean their plugin-specific caches through this path.

The actual remaining issue is full-snapshot reconciliation, especially for HTTP sync:

Previous selectors: [S1, S2]
Current selectors: [S2]

SelectorDataRefresh does not invoke removeSelector(S1), and DiscoveryUpstreamDataRefresh does not detect that S1
disappeared. This can leave the deleted selector's upstream cache behind.

That problem should be fixed by reconciling the previous and current snapshots, rather than introducing a new
discovery-upstream DELETE lifecycle.

I suggest:

• Keep upstream instance removal based on the complete UPDATE snapshot from fix: reconcile upstream snapshots without retaining removed instances #7172.

• Do not introduce removeDiscoveryUpstreamData() for ordinary upstream removal.

• Do not replace unSubscribe(DiscoverySyncData) with the incompatible unSubscribe(DiscoveryUpstreamKey) API.

• Fix HTTP synchronization by calculating:

  removed = previousSnapshot - currentSnapshot

• For removed selectors, invoke the existing selector/plugin cleanup path, or introduce a compatible snapshot
  reconciliation API.

• Add regression tests for:

  S1: [A, B] -> S1: [B]
  S1: [A] -> S1: []
  [S1, S2] -> [S2]
  [S1] -> []

• Handle WebSocket MYSELF/REFRESH reconciliation separately if reconnect recovery is also in scope.

In summary, the stale-cache problem is valid, but the DELETE chain introduced by this PR conflicts with the snapshot-
based upstream synchronization model established by #7172. I suggest redesigning this PR around snapshot
reconciliation before merging.
@lymerin what do you think?

[lymerin]

Thanks for working on this issue.

After comparing this PR with #7172, I do not think the new DISCOVER_UPSTREAM DELETE chain is the correct way to remove discovery upstreams.

PR #7172 establishes discovery upstream synchronization as snapshot reconciliation:

Registry ADDED / UPDATED / DELETED
    -> Admin updates the database
    -> Admin queries the complete remaining upstream list
    -> Publish DISCOVER_UPSTREAM UPDATE
    -> Gateway reconciles its local cache with the snapshot

This means a registry DELETED event is an internal Admin-side event. It should not be propagated to the gateway as an upstream DELETE event.

For example:

Previous upstreams: [A, B] Delete A Published snapshot: [B]

The gateway removes A by comparing [A, B] with [B].

Deleting the last instance should still publish an empty UPDATE snapshot:

Previous upstreams: [A] Delete A Published snapshot: []

The gateway then clears the selector's upstream cache through the same reconciliation path.

Therefore, the following chain introduced by this PR represents the wrong abstraction for upstream instance deletion:

DISCOVER_UPSTREAM DELETE -> DiscoveryUpstreamDataSubscriber.unSubscribe() -> DiscoveryUpstreamDataHandler.removeDiscoveryUpstreamData()

There are two different concepts that should not be mixed:

1. An upstream instance is removed
   This should be handled by a complete DISCOVER_UPSTREAM UPDATE snapshot, as implemented by fix: reconcile upstream snapshots without retaining removed instances #7172.
2. An entire selector is removed
   Incremental synchronization already publishes SELECTOR DELETE, which invokes the plugin's removeSelector() method.
   Divide, WebSocket, and gRPC already clean their plugin-specific caches through this path.

The actual remaining issue is full-snapshot reconciliation, especially for HTTP sync:

Previous selectors: [S1, S2] Current selectors: [S2]

SelectorDataRefresh does not invoke removeSelector(S1), and DiscoveryUpstreamDataRefresh does not detect that S1 disappeared. This can leave the deleted selector's upstream cache behind.

That problem should be fixed by reconciling the previous and current snapshots, rather than introducing a new discovery-upstream DELETE lifecycle.

I suggest:

• Keep upstream instance removal based on the complete UPDATE snapshot from fix: reconcile upstream snapshots without retaining removed instances #7172.
• Do not introduce removeDiscoveryUpstreamData() for ordinary upstream removal.
• Do not replace unSubscribe(DiscoverySyncData) with the incompatible unSubscribe(DiscoveryUpstreamKey) API.
• Fix HTTP synchronization by calculating:
  removed = previousSnapshot - currentSnapshot
• For removed selectors, invoke the existing selector/plugin cleanup path, or introduce a compatible snapshot
  reconciliation API.
• Add regression tests for:
  S1: [A, B] -> S1: [B]
  S1: [A] -> S1: []
  [S1, S2] -> [S2]
  [S1] -> []
• Handle WebSocket MYSELF/REFRESH reconciliation separately if reconnect recovery is also in scope.

In summary, the stale-cache problem is valid, but the DELETE chain introduced by this PR conflicts with the snapshot- based upstream synchronization model established by #7172. I suggest redesigning this PR around snapshot reconciliation before merging. @lymerin what do you think?

I agree with the instance-level model in #7172: registry ADDED, UPDATED, and DELETED events should publish DISCOVER_UPSTREAM UPDATE with the remaining upstream list, including an empty list. #7289 does not change that producer or turn an instance deletion into DELETE.

The DISCOVER_UPSTREAM DELETE handled here is an existing selector-level event. SelectorServiceImpl#unbindDiscovery already calls removeSelectorUpstream, which publishes it; #7289 fixes the gateway path that ignored the event or reached a no-op unSubscribe—the failure reported in #6479. Existing SELECTOR DELETE and PROXY_SELECTOR DELETE handling remain unchanged.

The proposed HTTP previous − current reconciliation is also in this PR. Its tests cover [S1, S2] → [S2] and [S1] → []. An existing selector with an empty upstream list stays in the Admin snapshot, so it is not treated as a removed selector.

I’ve clarified these separate flows in the PR description. The unSubscribe signature change is documented in the release notes; the instance-level [A, B] → [B] and [A] → [] cases belong to #7172.

Thanks for working on this issue.

After comparing this PR with #7172, I do not think the new DISCOVER_UPSTREAM DELETE chain is the correct way to remove discovery upstreams.

PR #7172 establishes discovery upstream synchronization as snapshot reconciliation:

Registry ADDED / UPDATED / DELETED
    -> Admin updates the database
    -> Admin queries the complete remaining upstream list
    -> Publish DISCOVER_UPSTREAM UPDATE
    -> Gateway reconciles its local cache with the snapshot

This means a registry DELETED event is an internal Admin-side event. It should not be propagated to the gateway as an upstream DELETE event.

For example:

Previous upstreams: [A, B] Delete A Published snapshot: [B]

The gateway removes A by comparing [A, B] with [B].

Deleting the last instance should still publish an empty UPDATE snapshot:

Previous upstreams: [A] Delete A Published snapshot: []

The gateway then clears the selector's upstream cache through the same reconciliation path.

Therefore, the following chain introduced by this PR represents the wrong abstraction for upstream instance deletion:

DISCOVER_UPSTREAM DELETE -> DiscoveryUpstreamDataSubscriber.unSubscribe() -> DiscoveryUpstreamDataHandler.removeDiscoveryUpstreamData()

There are two different concepts that should not be mixed:

1. An upstream instance is removed
   This should be handled by a complete DISCOVER_UPSTREAM UPDATE snapshot, as implemented by fix: reconcile upstream snapshots without retaining removed instances #7172.
2. An entire selector is removed
   Incremental synchronization already publishes SELECTOR DELETE, which invokes the plugin's removeSelector() method.
   Divide, WebSocket, and gRPC already clean their plugin-specific caches through this path.

The actual remaining issue is full-snapshot reconciliation, especially for HTTP sync:

Previous selectors: [S1, S2] Current selectors: [S2]

SelectorDataRefresh does not invoke removeSelector(S1), and DiscoveryUpstreamDataRefresh does not detect that S1 disappeared. This can leave the deleted selector's upstream cache behind.

That problem should be fixed by reconciling the previous and current snapshots, rather than introducing a new discovery-upstream DELETE lifecycle.

I suggest:

• Keep upstream instance removal based on the complete UPDATE snapshot from fix: reconcile upstream snapshots without retaining removed instances #7172.
• Do not introduce removeDiscoveryUpstreamData() for ordinary upstream removal.
• Do not replace unSubscribe(DiscoverySyncData) with the incompatible unSubscribe(DiscoveryUpstreamKey) API.
• Fix HTTP synchronization by calculating:
  removed = previousSnapshot - currentSnapshot
• For removed selectors, invoke the existing selector/plugin cleanup path, or introduce a compatible snapshot
  reconciliation API.
• Add regression tests for:
  S1: [A, B] -> S1: [B]
  S1: [A] -> S1: []
  [S1, S2] -> [S2]
  [S1] -> []
• Handle WebSocket MYSELF/REFRESH reconciliation separately if reconnect recovery is also in scope.

In summary, the stale-cache problem is valid, but the DELETE chain introduced by this PR conflicts with the snapshot- based upstream synchronization model established by #7172. I suggest redesigning this PR around snapshot reconciliation before merging. @lymerin what do you think?

PR #7172 resolves instance lifecycle within a selector, while PR #7289 resolves selector lifecycle within discovery sync.
I compared #7289 with #7172 and ran the relevant tests on both branches. I agree with the snapshot model for upstream instance changes, but the proposed redesign is based on a misunderstanding of which event #7289 handles.

1. Registry instance deletion remains an UPDATE snapshot. A registry DELETED event is handled inside Admin. DiscoveryDataChangedEventSyncListener publishes DISCOVER_UPSTREAM UPDATE containing the complete remaining upstream list, including an empty list after the last instance is deleted. fix(discovery): remove stale gateway cache on upstream deletion #7289 does not change this publisher or turn that event into DISCOVER_UPSTREAM DELETE. The tests on fix: reconcile upstream snapshots without retaining removed instances #7172 verify that deleting A from [A, B] publishes [B] and leaves only B in the gateway cache; deleting A from [A] publishes [] and clears the cached instance. The incorrect result you describe—deleting the entire selector when only A is removed—does not occur through fix(discovery): remove stale gateway cache on upstream deletion #7289’s DELETE handler.

2. The discovery-upstream DELETE event is selector-scoped and predates this PR. Admin’s removeSelectorUpstream() already published DISCOVER_UPSTREAM DELETE when unbinding a selector-level discovery. Before fix(discovery): remove stale gateway cache on upstream deletion #7289, CommonDiscoveryUpstreamDataSubscriber#unSubscribe() ignored that event. This PR makes that existing selector-removal event effective; it does not introduce a registry-instance DELETE lifecycle. I agree that SELECTOR DELETE already cleans some plugin caches, including the Divide, WebSocket, and gRPC paths you identified. That does not make the previously published but ignored discovery-upstream removal event an instance-deletion event.

3. The HTTP omission you identified is fixed by snapshot comparison. DiscoveryUpstreamDataRefresh now compares the previous and current selector snapshots. In the fix(discovery): remove stale gateway cache on upstream deletion #7289 tests, [S1, S2] → [S2] unsubscribes S1 but not S2, while [S1] → [] unsubscribes S1 once. A separate plugin-handler test starts with an upstream entry and verifies that removing its discovery selector leaves no Divide upstream cache entry. Thus the HTTP fix is snapshot reconciliation for missing selectors; it does not replace fix: reconcile upstream snapshots without retaining removed instances #7172’s UPDATE reconciliation for missing instances.

4. removeDiscoveryUpstreamData() is not used for ordinary instance removal. Instance changes continue through onSubscribe() with the complete UPDATE snapshot. The removal method is invoked for the selector identity carried by an existing discovery-upstream DELETE event, or for a selector missing from an HTTP snapshot. For that reason, I am not replacing this path with instance-level snapshot logic.

5. I am retaining DiscoveryUpstreamKey intentionally. This removal operation needs the selector’s identity, not a partially populated DiscoverySyncData. All implementations and call sites in this repository have been migrated. I acknowledge the source- and binary-compatibility impact for downstream implementations; it is explicitly documented in RELEASE-NOTES.md. A compatibility bridge back to DiscoverySyncData would recreate the partial DTO that this change removes.

6. The four requested regression scenarios are covered by the two PRs according to their respective responsibilities. fix: reconcile upstream snapshots without retaining removed instances #7172 tests instance snapshots [A, B] → [B] and [A] → [], including the resulting cache state. fix(discovery): remove stale gateway cache on upstream deletion #7289 tests HTTP selector snapshots [S1, S2] → [S2] and [S1] → [], and separately tests removal from a plugin upstream cache. I ran DiscoveryDataChangedEventSyncListenerTest and UpstreamCacheManagerTest on fix: reconcile upstream snapshots without retaining removed instances #7172, and DiscoveryUpstreamDataRefreshTest and DivideUpstreamDataHandlerTest on fix(discovery): remove stale gateway cache on upstream deletion #7289; the assertions described above all passed.

7. WebSocket MYSELF/REFRESH reconnection reconciliation is separate. fix(discovery): remove stale gateway cache on upstream deletion #7289 handles its DELETE event and retains the existing refresh() hook, but does not claim to implement a new reconciliation algorithm for reconnection. I am not expanding this PR to cover that case.

So I agree with your requirement that [A, B] → [B] and [A] → [] use the full UPDATE snapshot from #7172. They still do. #7289 addresses the different case of a discovery-bound selector being removed, plus selectors disappearing from an HTTP snapshot. The two PRs are complementary rather than conflicting, so I do not think #7289 needs to be redesigned around instance-level snapshot reconciliation.

[hengyuss]

Thanks for working on this issue.
After comparing this PR with #7172, I do not think the new DISCOVER_UPSTREAM DELETE chain is the correct way to remove discovery upstreams.
PR #7172 establishes discovery upstream synchronization as snapshot reconciliation:

Registry ADDED / UPDATED / DELETED
    -> Admin updates the database
    -> Admin queries the complete remaining upstream list
    -> Publish DISCOVER_UPSTREAM UPDATE
    -> Gateway reconciles its local cache with the snapshot

This means a registry DELETED event is an internal Admin-side event. It should not be propagated to the gateway as an upstream DELETE event.
For example:
Previous upstreams: [A, B] Delete A Published snapshot: [B]
The gateway removes A by comparing [A, B] with [B].
Deleting the last instance should still publish an empty UPDATE snapshot:
Previous upstreams: [A] Delete A Published snapshot: []
The gateway then clears the selector's upstream cache through the same reconciliation path.
Therefore, the following chain introduced by this PR represents the wrong abstraction for upstream instance deletion:
DISCOVER_UPSTREAM DELETE -> DiscoveryUpstreamDataSubscriber.unSubscribe() -> DiscoveryUpstreamDataHandler.removeDiscoveryUpstreamData()
There are two different concepts that should not be mixed:

1. An upstream instance is removed
   This should be handled by a complete DISCOVER_UPSTREAM UPDATE snapshot, as implemented by fix: reconcile upstream snapshots without retaining removed instances #7172.
2. An entire selector is removed
   Incremental synchronization already publishes SELECTOR DELETE, which invokes the plugin's removeSelector() method.
   Divide, WebSocket, and gRPC already clean their plugin-specific caches through this path.

The actual remaining issue is full-snapshot reconciliation, especially for HTTP sync:
Previous selectors: [S1, S2] Current selectors: [S2]
SelectorDataRefresh does not invoke removeSelector(S1), and DiscoveryUpstreamDataRefresh does not detect that S1 disappeared. This can leave the deleted selector's upstream cache behind.
That problem should be fixed by reconciling the previous and current snapshots, rather than introducing a new discovery-upstream DELETE lifecycle.
I suggest:

• Keep upstream instance removal based on the complete UPDATE snapshot from fix: reconcile upstream snapshots without retaining removed instances #7172.
• Do not introduce removeDiscoveryUpstreamData() for ordinary upstream removal.
• Do not replace unSubscribe(DiscoverySyncData) with the incompatible unSubscribe(DiscoveryUpstreamKey) API.
• Fix HTTP synchronization by calculating:
  removed = previousSnapshot - currentSnapshot
• For removed selectors, invoke the existing selector/plugin cleanup path, or introduce a compatible snapshot
  reconciliation API.
• Add regression tests for:
  S1: [A, B] -> S1: [B]
  S1: [A] -> S1: []
  [S1, S2] -> [S2]
  [S1] -> []
• Handle WebSocket MYSELF/REFRESH reconciliation separately if reconnect recovery is also in scope.

In summary, the stale-cache problem is valid, but the DELETE chain introduced by this PR conflicts with the snapshot- based upstream synchronization model established by #7172. I suggest redesigning this PR around snapshot reconciliation before merging. @lymerin what do you think?

I agree with the instance-level model in #7172: registry ADDED, UPDATED, and DELETED events should publish DISCOVER_UPSTREAM UPDATE with the remaining upstream list, including an empty list. #7289 does not change that producer or turn an instance deletion into DELETE.

The DISCOVER_UPSTREAM DELETE handled here is an existing selector-level event. SelectorServiceImpl#unbindDiscovery already calls removeSelectorUpstream, which publishes it; #7289 fixes the gateway path that ignored the event or reached a no-op unSubscribe—the failure reported in #6479. Existing SELECTOR DELETE and PROXY_SELECTOR DELETE handling remain unchanged.

The proposed HTTP previous − current reconciliation is also in this PR. Its tests cover [S1, S2] → [S2] and [S1] → []. An existing selector with an empty upstream list stays in the Admin snapshot, so it is not treated as a removed selector.

I’ve clarified these separate flows in the PR description. The unSubscribe signature change is documented in the release notes; the instance-level [A, B] → [B] and [A] → [] cases belong to #7172.

Thanks for working on this issue.
After comparing this PR with #7172, I do not think the new DISCOVER_UPSTREAM DELETE chain is the correct way to remove discovery upstreams.
PR #7172 establishes discovery upstream synchronization as snapshot reconciliation:

Registry ADDED / UPDATED / DELETED
    -> Admin updates the database
    -> Admin queries the complete remaining upstream list
    -> Publish DISCOVER_UPSTREAM UPDATE
    -> Gateway reconciles its local cache with the snapshot

This means a registry DELETED event is an internal Admin-side event. It should not be propagated to the gateway as an upstream DELETE event.
For example:
Previous upstreams: [A, B] Delete A Published snapshot: [B]
The gateway removes A by comparing [A, B] with [B].
Deleting the last instance should still publish an empty UPDATE snapshot:
Previous upstreams: [A] Delete A Published snapshot: []
The gateway then clears the selector's upstream cache through the same reconciliation path.
Therefore, the following chain introduced by this PR represents the wrong abstraction for upstream instance deletion:
DISCOVER_UPSTREAM DELETE -> DiscoveryUpstreamDataSubscriber.unSubscribe() -> DiscoveryUpstreamDataHandler.removeDiscoveryUpstreamData()
There are two different concepts that should not be mixed:

1. An upstream instance is removed
   This should be handled by a complete DISCOVER_UPSTREAM UPDATE snapshot, as implemented by fix: reconcile upstream snapshots without retaining removed instances #7172.
2. An entire selector is removed
   Incremental synchronization already publishes SELECTOR DELETE, which invokes the plugin's removeSelector() method.
   Divide, WebSocket, and gRPC already clean their plugin-specific caches through this path.

The actual remaining issue is full-snapshot reconciliation, especially for HTTP sync:
Previous selectors: [S1, S2] Current selectors: [S2]
SelectorDataRefresh does not invoke removeSelector(S1), and DiscoveryUpstreamDataRefresh does not detect that S1 disappeared. This can leave the deleted selector's upstream cache behind.
That problem should be fixed by reconciling the previous and current snapshots, rather than introducing a new discovery-upstream DELETE lifecycle.
I suggest:

• Keep upstream instance removal based on the complete UPDATE snapshot from fix: reconcile upstream snapshots without retaining removed instances #7172.
• Do not introduce removeDiscoveryUpstreamData() for ordinary upstream removal.
• Do not replace unSubscribe(DiscoverySyncData) with the incompatible unSubscribe(DiscoveryUpstreamKey) API.
• Fix HTTP synchronization by calculating:
  removed = previousSnapshot - currentSnapshot
• For removed selectors, invoke the existing selector/plugin cleanup path, or introduce a compatible snapshot
  reconciliation API.
• Add regression tests for:
  S1: [A, B] -> S1: [B]
  S1: [A] -> S1: []
  [S1, S2] -> [S2]
  [S1] -> []
• Handle WebSocket MYSELF/REFRESH reconciliation separately if reconnect recovery is also in scope.

In summary, the stale-cache problem is valid, but the DELETE chain introduced by this PR conflicts with the snapshot- based upstream synchronization model established by #7172. I suggest redesigning this PR around snapshot reconciliation before merging. @lymerin what do you think?

PR #7172 resolves instance lifecycle within a selector, while PR #7289 resolves selector lifecycle within discovery sync. I compared #7289 with #7172 and ran the relevant tests on both branches. I agree with the snapshot model for upstream instance changes, but the proposed redesign is based on a misunderstanding of which event #7289 handles.

1. Registry instance deletion remains an UPDATE snapshot. A registry DELETED event is handled inside Admin. DiscoveryDataChangedEventSyncListener publishes DISCOVER_UPSTREAM UPDATE containing the complete remaining upstream list, including an empty list after the last instance is deleted. fix(discovery): remove stale gateway cache on upstream deletion #7289 does not change this publisher or turn that event into DISCOVER_UPSTREAM DELETE. The tests on fix: reconcile upstream snapshots without retaining removed instances #7172 verify that deleting A from [A, B] publishes [B] and leaves only B in the gateway cache; deleting A from [A] publishes [] and clears the cached instance. The incorrect result you describe—deleting the entire selector when only A is removed—does not occur through fix(discovery): remove stale gateway cache on upstream deletion #7289’s DELETE handler.
2. The discovery-upstream DELETE event is selector-scoped and predates this PR. Admin’s removeSelectorUpstream() already published DISCOVER_UPSTREAM DELETE when unbinding a selector-level discovery. Before fix(discovery): remove stale gateway cache on upstream deletion #7289, CommonDiscoveryUpstreamDataSubscriber#unSubscribe() ignored that event. This PR makes that existing selector-removal event effective; it does not introduce a registry-instance DELETE lifecycle. I agree that SELECTOR DELETE already cleans some plugin caches, including the Divide, WebSocket, and gRPC paths you identified. That does not make the previously published but ignored discovery-upstream removal event an instance-deletion event.
3. The HTTP omission you identified is fixed by snapshot comparison. DiscoveryUpstreamDataRefresh now compares the previous and current selector snapshots. In the fix(discovery): remove stale gateway cache on upstream deletion #7289 tests, [S1, S2] → [S2] unsubscribes S1 but not S2, while [S1] → [] unsubscribes S1 once. A separate plugin-handler test starts with an upstream entry and verifies that removing its discovery selector leaves no Divide upstream cache entry. Thus the HTTP fix is snapshot reconciliation for missing selectors; it does not replace fix: reconcile upstream snapshots without retaining removed instances #7172’s UPDATE reconciliation for missing instances.
4. removeDiscoveryUpstreamData() is not used for ordinary instance removal. Instance changes continue through onSubscribe() with the complete UPDATE snapshot. The removal method is invoked for the selector identity carried by an existing discovery-upstream DELETE event, or for a selector missing from an HTTP snapshot. For that reason, I am not replacing this path with instance-level snapshot logic.
5. I am retaining DiscoveryUpstreamKey intentionally. This removal operation needs the selector’s identity, not a partially populated DiscoverySyncData. All implementations and call sites in this repository have been migrated. I acknowledge the source- and binary-compatibility impact for downstream implementations; it is explicitly documented in RELEASE-NOTES.md. A compatibility bridge back to DiscoverySyncData would recreate the partial DTO that this change removes.
6. The four requested regression scenarios are covered by the two PRs according to their respective responsibilities. fix: reconcile upstream snapshots without retaining removed instances #7172 tests instance snapshots [A, B] → [B] and [A] → [], including the resulting cache state. fix(discovery): remove stale gateway cache on upstream deletion #7289 tests HTTP selector snapshots [S1, S2] → [S2] and [S1] → [], and separately tests removal from a plugin upstream cache. I ran DiscoveryDataChangedEventSyncListenerTest and UpstreamCacheManagerTest on fix: reconcile upstream snapshots without retaining removed instances #7172, and DiscoveryUpstreamDataRefreshTest and DivideUpstreamDataHandlerTest on fix(discovery): remove stale gateway cache on upstream deletion #7289; the assertions described above all passed.
7. WebSocket MYSELF/REFRESH reconnection reconciliation is separate. fix(discovery): remove stale gateway cache on upstream deletion #7289 handles its DELETE event and retains the existing refresh() hook, but does not claim to implement a new reconciliation algorithm for reconnection. I am not expanding this PR to cover that case.

So I agree with your requirement that [A, B] → [B] and [A] → [] use the full UPDATE snapshot from #7172. They still do. #7289 addresses the different case of a discovery-bound selector being removed, plus selectors disappearing from an HTTP snapshot. The two PRs are complementary rather than conflicting, so I do not think #7289 needs to be redesigned around instance-level snapshot reconciliation.

Thanks for the clarification. I misunderstood the event scope: instance changes use full UPDATE snapshots, while the
DELETE event handled by #7289 is selector-scoped. My previous assessment was therefore incorrect. I’ll review the PR
again with this distinction in mind.

[hengyuss]

pls resolve the conflicts. Thanks @lymerin

[lymerin]

pls resolve the conflicts. Thanks @lymerin

I've resolved the merge conflicts and all tests are now passing.

[Aias00]

Re-reviewed after the new commits. This closes a real gap: discovery-bound upstreams could stay cached in the gateway after the selector was deleted. Routing DISCOVER_UPSTREAM DELETE through path-based sync, handling ZooKeeper node deletions, reconciling HTTP discovery snapshots so removed selectors are unsubscribed, and adding removeDiscoveryUpstreamData(DiscoveryUpstreamKey) per plugin handler (divide / grpc / websocket / tcp) is a coherent fix, and the defensive checks for blank pluginName / selectorId are welcome. Test coverage across every touched module is solid.

One thing to keep visible: this changes the public SPI DiscoveryUpstreamDataSubscriber#unSubscribe(DiscoverySyncData) to unSubscribe(DiscoveryUpstreamKey), which breaks third-party sync-data implementations. You documented it under "API Changes" in RELEASE-NOTES — approving on the basis that this was a deliberate, documented decision for this release.