Skip to content

fix(admin): null-safe plugin lookup when publishing selector deletions - #7364

Merged
Aias00 merged 3 commits into
apache:masterfrom
Sean-Walker0:fix/selector-event-publisher-null-plugin-name
Oct 1, 2026
Merged

Aias00 merged 3 commits into
apache:masterfrom
Sean-Walker0:fix/selector-event-publisher-null-plugin-name

Conversation

@Sean-Walker0

Copy link
Copy Markdown
Contributor

Found by code audit (no existing issue — happy to file one if maintainers prefer).

SelectorEventPublisher#onDeleted(Collection, List) resolves each selector's plugin name via pluginMap.get(selectorDO.getPluginId()) and immediately calls pluginName.equals(PluginEnum.DIVIDE.getName()). Callers do not guarantee the map covers every selector: SelectorServiceImpl#deleteByNamespaceId builds it from pluginMapper.selectByIds(...), which silently drops ids whose plugin row no longer exists — so deleting a dangling selector (one whose plugin was already removed, reachable because selectByIdSet does not join plugins) throws NullPointerException inside the event publisher and fails the whole deletion request. The sibling RuleEventPublisher#onDeleted performs the same plugin lookup null-safely via Optional, confirming the anticipated state.

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 test -pl shenyu-admin -am and ./mvnw checkstyle:check -pl shenyu-admin (module-scoped; full build left to CI).

Modifications

  • Flip the comparison to constant-first: PluginEnum.DIVIDE.getName().equals(pluginName) — null-safe, identical semantics for present plugin names, and the deletion events now publish with a null plugin name instead of crashing.

Verifying this change

  • New testOnDeletedSelectorReferencingMissingPlugin deletes a selector whose pluginId is absent from the plugins list and asserts both deletion events still publish. It fails on current master with NullPointerException: Cannot invoke "String.equals(Object)" because "pluginName" is null and passes with this change.
  • Full shenyu-admin module suite green (502 test classes); checkstyle green.

Notes

  • Behavior change: deleting selectors that reference an already-deleted plugin now succeeds and publishes its events instead of returning 500.
  • Orthogonal to open PRs: no open PR touches SelectorEventPublisher (checked against the file lists of all 198 open PRs).

SelectorEventPublisher.onDeleted(Collection, List) resolved each
selector's plugin name from a map built of the supplied plugins and
called pluginName.equals(DIVIDE) on it. Callers do not guarantee the
map covers every selector: SelectorServiceImpl#deleteByNamespaceId
builds it from pluginMapper.selectByIds, which silently drops ids whose
plugin row no longer exists, so deleting a dangling selector (one whose
plugin was already removed) threw NullPointerException inside the event
publisher and failed the whole deletion. The sibling
RuleEventPublisher.onDeleted performs the same lookup null-safely via
Optional. Compare constant-first, which keeps the divide branch intact
and lets the deletion events publish with a null plugin name.

The new test fails on current master with the exact NPE and passes
with this change.

@Aias00 Aias00 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.

The null-safe comparison prevents the admin-side NPE, but the resulting SelectorData still carries pluginName=null. Bootstrap deletion is keyed by plugin name: CommonPluginDataSubscriber, BaseDataCache, and MatchDataCache use selectorData.getPluginName() to remove selector, rule, and match-cache entries. A null-name delete therefore cannot reliably clear the stale data and may only move the failure downstream. Please preserve the plugin name for deletion or add a selector-id fallback in the data plane, with a regression test proving a missing-plugin selector delete actually removes bootstrap caches.

@Sean-Walker0

Copy link
Copy Markdown
Contributor Author

Confirmed the downstream behavior — you are right that the null name moves the problem:

  • BaseDataCache#removeSelectData calls selectorMap.computeIfPresent(data.getPluginName(), ...); on the concurrent map a null key throws NPE, so the subscriber callback itself can fail rather than merely skip.
  • MatchDataCache#removeSelectorData(null, id) / #removeRuleDataBySelector(null, id) hit SELECTOR_DATA_MAP.get(null) → null → silent no-op, leaving stale match-cache entries.

On the two remedies:

(a) preserve the plugin name — I could not find a source when the plugin row is already deleted: SelectorDO carries only pluginId, and pluginMapper.selectByIds is the sole lookup feeding onDeleted. In the normal cascade (BatchNamespacePluginDeletedEvent) the event payload does carry the deleted PluginDOs, so names exist there; the null case is precisely the dangling-selector path where no row remains anywhere.

(b) selector-id fallback in the data plane — I sketched it: in BaseDataCache#removeSelectData, when pluginName is blank, sweep every plugin bucket and drop entries with the matching selector id (buckets are immutable List.copyOf results, so via computeIfPresent per key); same blank-name sweep in MatchDataCache#removeSelectorData(pluginName, id) and #removeRuleDataBySelector. However, all three files are currently modified by open PRs — BaseDataCache by #7345, MatchDataCache by #6336, CommonPluginDataSubscriber by #7319/#7088 — so folding the fallback into this admin-scoped PR would create textual conflicts with four in-flight patches.

How would you like to proceed?

  1. Keep this PR admin-scoped (it fixes the reported 500 and still publishes the delete events), and I send the data-plane fallback as a dedicated follow-up PR built on top of Fixes #6796: Sort cached selector and rule batches once per key #7345/Refactor Selector #6336/fix(web): reload replaced extension jars #7088 once they land (or immediately, if you prefer it standalone); or
  2. Fold the fallback into this PR now despite the overlap.

Happy to implement either on your word — the regression test proving a blank-name selector delete purges BaseDataCache/MatchDataCache is straightforward either way.

Follow-up to the admin-side null-safe plugin lookup: a dangling
selector's deletion event now reaches the bootstrap with a null
pluginName, and every data-plane removal keyed by that name fails.
ConcurrentHashMap rejects null keys, so BaseDataCache#removeSelectData's
computeIfPresent throws NPE inside the subscriber, and the entry stays
cached under the plugin name the selector had before its plugin row
vanished.

When the plugin name is blank, BaseDataCache#removeSelectData,
MatchDataCache#removeSelectorData and #removeRuleDataBySelector now
sweep every plugin bucket and drop entries with the matching selector
id, #removeRuleData sweeps by rule id, and the empty-placeholder
cleanups become no-ops. The subscriber skips the handler lookup
instead of calling handlerMap.get(null), for rules as well because
RuleEventPublisher already publishes null-name deletes null-safely.

The regression tests cache a selector under its historical plugin
name and prove a blank-name delete purges BaseDataCache and both
MatchDataCache maps without touching other plugins' entries; all
five fail on the previous commit (two assertion failures, three
NPEs) and pass with this change.
@Sean-Walker0

Copy link
Copy Markdown
Contributor Author

Went with option 2 — the selector-id fallback is folded into this PR now (commit ec1c36f), since it is the part that actually keeps the bootstrap caches consistent.

What the data plane does on a blank plugin name (null or empty, StringUtils#isBlank):

  • BaseDataCache#removeSelectData — instead of selectorMap.computeIfPresent(null, …) (NPE on the concurrent map), it sweeps every plugin bucket and drops entries whose id matches, via computeIfPresent per key so the immutable List.copyOf buckets are still replaced wholesale.
  • MatchDataCache#removeSelectorData(name, id) / #removeRuleDataBySelector(name, id) — same bucket sweep by selector id; #removeRuleData(name, ruleId) gets the matching sweep by rule id (the rule branch has the identical exposure because RuleEventPublisher.onDeleted already publishes null-name deletes null-safely).
  • #removeEmptySelectorData / #removeEmptyRuleData — become no-ops: the null-id placeholders belong to real plugins' buckets and a blank-name delete must not touch them.
  • CommonPluginDataSubscriber#removeCacheData — skips the handlerMap lookup rather than calling get(null) (also an NPE on the concurrent map), so the deletion now runs to completion instead of being aborted mid-way.

Why the sweep targets the historical name's bucket correctly: a null-name selector can never have been cached (selectorAccept/cacheSelectorData would have thrown on the null key), so its stale entry necessarily sits under the plugin name it had while the plugin row still existed — exactly what the per-bucket sweep removes.

Regression coverage (all five fail on the previous commit — two assertion failures and three NPEs — and pass with the fix):

  • CommonPluginDataSubscriberTest#testUnSelectorSubscribeWithMissingPluginName — the requested proof: caches a selector under its real plugin name, publishes a blank-name delete, asserts BaseDataCache and both MatchDataCache maps are purged while an unrelated plugin keeps its entries.
  • #testUnRuleSubscribeWithMissingPluginName — same shape for the rule path (match-cache purge was the pre-fix observable failure).
  • Per-cache sweeps in BaseDataCacheTest and MatchDataCacheTest.

Full shenyu-plugin-base suite (137 tests) + checkstyle green. The textual overlap with #7345/#6336/#7319/#7088 is unchanged and will be resolved by whichever lands first.

@Aias00 Aias00 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.

Re-review → approving

You took the selector-id fallback option, and the data plane now actually clears stale entries for a selector whose plugin row is gone — which was the whole point of the objection. Approving.

Admin side. SelectorEventPublisher:99 flips to PluginEnum.DIVIDE.getName().equals(pluginName), the constant-on-the-left form, so a null lookup no longer NPEs and the removeByKey branch is simply skipped. testOnDeletedSelectorReferencingMissingPlugin exercises onDeleted with an empty plugin list.

Data plane. Three places handled, which is the right set:

  • BaseDataCache.removeSelectData:51-64 — blank plugin name now sweeps every plugin bucket by selector id (selectorMap.keySet().forEach(...)), delegating to the new private removeSelectData(pluginName, selectorId). This is the piece that makes the delete actually land: previously computeIfPresent(null, ...) was a no-op (and ConcurrentHashMap would have thrown on a null key in the handler map), so the stale selector stayed cached forever.
  • MatchDataCache — removeSelectorData, removeRuleData and removeRuleDataBySelector each gained the same blank → sweep-all-buckets path, and removeEmptySelectorData / removeEmptyRuleData now short-circuit on blank instead of looking up a null key.
  • CommonPluginDataSubscriber.removeCacheData — the handlerMap.get(...) call is now guarded by StringUtils.isNotBlank, avoiding the null-key NPE on the concurrent map.

The tests prove the end-to-end property I asked for rather than just "does not throw": testUnSelectorSubscribeWithMissingPluginName caches a selector under its real plugin name, sends a deletion event with no plugin name, and asserts obtainSelectorData(mockPluginName1) and both MatchDataCache lookups are now null while the unrelated plugin keeps its entry. That is exactly the "bootstrap caches are really cleared" evidence. testRemoveSelectDataSweepsAllBucketsWhenPluginNameIsMissing and the two MatchDataCacheTest cases cover the fallback in isolation.

CI

Red checks are not from this change — every one of them fails while compiling shenyu-plugin-mcp-server:

[ERROR] .../ShenyuStreamableHttpServerTransportProvider.java:[841,24]
        constructor MessageHandlingResult cannot be applied to given types;
        required: int,java.lang.Object,java.lang.String,io.modelcontextprotocol.json.McpJsonMapper
        found:    int,io.modelcontextprotocol.spec.McpSchema.JSONRPCMessage,java.lang.String

Same failure appears on #7375 which touches only the logging plugin, so it is a master-level break from the MCP SDK gaining a constructor parameter. Please re-run once it is fixed; nothing here can be validated by these runs in the meantime.

Non-blocking

  1. handler.removeSelector(...) is now skipped entirely for a dangling selector. That is correct for the NPE, but it means any plugin that releases resources in removeSelector (connection pools, scheduled tasks, native handles) will not be invoked for this path — the cache is clean, the handler state is not. A fallback that resolves the plugin name from the cached SelectorData (you already have it in BaseDataCache) and still calls the handler would close that gap. Worth a follow-up rather than blocking.
  2. The sweep is O(plugins × paths) per dangling delete. Fine for a rare path, but if an operator deletes a plugin with many orphaned selectors in one burst this becomes a full-map scan per selector. Batching (collect ids, sweep once) would remove that; noting it because the same pattern is now in three methods.
  3. removeEmptySelectorData / removeEmptyRuleData returning early on blank leaves empty per-plugin maps behind. Harmless memory-wise, but the "empty bucket" cleanup that those methods exist for is skipped precisely in the dangling case — the one case that creates empty buckets.
  4. The sweeps iterate ConcurrentHashMap views while calling computeIfPresent / removeIf. That is legal (weakly consistent iterators, no CME), and the operations are idempotent, so a concurrent writer can only cause a missed entry that a later delete or restart clears. Just recording it so nobody "fixes" it by copying to a list and reintroducing a lock.

@Aias00
Aias00 merged commit f39dfdd into apache:master Oct 1, 2026
39 checks passed
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.

2 participants