Skip to content

fix: remove faulty size guard so REFRESH cleans up stale plugin/auth/meta config nodes - #7014

Open
dmsolr wants to merge 2 commits into
masterfrom
fix/refresh-stale-config-guard-6874
Open

fix: remove faulty size guard so REFRESH cleans up stale plugin/auth/meta config nodes#7014
dmsolr wants to merge 2 commits into
masterfrom
fix/refresh-stale-config-guard-6874

Conversation

@dmsolr

@dmsolr dmsolr commented Aug 31, 2026

Copy link
Copy Markdown
Member

What is the purpose of the change

Fixes #6874.

In AbstractNodeDataChangedListener#onCommonChanged, the REFRESH/MYSELF branch only removed stale plugin/app-auth/metadata config entries when the old set strictly outnumbered the new one (configDataNames.size() > changedList.size()). When a REFRESH replaced the set with equal or larger cardinality but different members (e.g. old=[A,B,C,D], new=[C,D,E,F]), the guard was false, so stale entries A and B were never removed from nacos/apollo/polaris config centers — only orphaned per-entry config nodes remained while the LIST node was overwritten.

This path is reached by syncAllByNamespaceId(REFRESH, ns) from NamespacePluginController and ConfigsExportImportController, which publish PLUGIN/AUTH/META REFRESH events spanning a whole namespace. For app-auth this is security-adjacent, since a deleted app key's config node could linger and be served to gateways that read it before the list update propagates.

Changes

  • Removed the faulty size guard so the stale-entry diff (removeAll) always runs.
  • Fixed the stale-entry delConfig call to use the correctly namespaced key (configKeyPrefix + name) instead of the bare entry name, which was also silently preventing cleanup even when the guard passed.
  • Added a regression test reproducing the exact scenario from the issue (old=[A,B,C,D] -> new=[C,D,E,F], equal cardinality) verifying stale entries are removed.

Does this PR introduce a user-facing change?

No.

…meta config nodes (#6874)

The REFRESH/MYSELF branch of AbstractNodeDataChangedListener#onCommonChanged only removed stale entries when the old set strictly outnumbered the new one, so refreshes with equal or larger cardinality (e.g. old=[A,B,C,D] -> new=[C,D,E,F]) left orphaned config nodes in nacos/apollo/polaris. Removed the size guard and fixed the stale-entry delConfig call to use the correct namespaced key.
@dmsolr
dmsolr requested review from Aias00 and a lite review from Copilot August 31, 2026 07:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes stale config-node cleanup during namespace-wide REFRESH/MYSELF sync events in AbstractNodeDataChangedListener#onCommonChanged, ensuring removed plugin/app-auth/metadata entries are correctly deleted from config centers even when the refreshed set’s cardinality stays the same (or increases). This aligns admin-side publish/delete behavior with how gateway-side watchers consume {prefix}.list and {prefix}{entryKey} nodes.

Changes:

  • Remove the incorrect old.size() > new.size() guard so stale-entry diffing/deletion always runs for REFRESH/MYSELF.
  • Fix stale-entry deletion to pass the fully qualified dataId (configKeyPrefix + staleName) into delConfig.
  • Add a regression test covering equal-cardinality replacement (e.g., A,B,C,D -> C,D,E,F) and asserting stale entries are deleted.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
shenyu-admin-listener/shenyu-admin-listener-api/src/main/java/org/apache/shenyu/admin/listener/AbstractNodeDataChangedListener.java Always computes/removes stale keys on REFRESH/MYSELF and deletes correctly namespaced per-entry nodes.
shenyu-admin-listener/shenyu-admin-listener-api/src/test/java/org/apache/shenyu/admin/listener/AbstractNodeDataChangedListenerTest.java Adds regression coverage for equal-cardinality REFRESH replacing membership and verifies stale node deletion.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] instance_info BaseResultMap omits instance_state mapping — null state on resultMap reads (root of N37 NPEs)

2 participants