Skip to content

[Fix] Prevent NPE and scan starvation in expired token cleanup when the flow rule has been removed - #3664

Open
konlue wants to merge 1 commit into
alibaba:1.8from
konlue:fix/regular-expire-strategy-npe
Open

konlue wants to merge 1 commit into
alibaba:1.8from
konlue:fix/regular-expire-strategy-npe

Conversation

@konlue

@konlue konlue commented Oct 11, 2026

Copy link
Copy Markdown

Describe what this PR does / why we need it

RegularExpireStrategy#clearToken cleans up expired tokens, but it dereferences the rule lookup without a null check:

long resourceTimeout = ClusterFlowRuleManager.getFlowRuleById(node.getFlowId()).getClusterConfig().getResourceTimeout();

After a rule hot update removes a rule (ClusterFlowRuleManager#clearAndResetRulesFor removes both the rule and its concurrency counter), stale TokenCacheNodes of that flowId can still remain in the token cache — tokens outlive single requests, and clients release them later. The line above then throws:

java.lang.NullPointerException: Cannot invoke "com.alibaba.csp.sentinel.slots.block.flow.FlowRule.getClusterConfig()"
because the return value of "com.alibaba.csp.sentinel.cluster.flow.rule.ClusterFlowRuleManager.getFlowRuleById(java.lang.Long)" is null

The impact is worse than the NPE itself. The cleanup loop scans from the head of the key list every round, the exception is swallowed by ClearExpiredTokenTask#run's catch block, and the offending element is never removed (only removeToken removes it, which is never reached). So every subsequent round aborts on the same node:

  • all expired tokens located behind it starve forever;
  • the concurrency counters of the removed rule only grow (never released);
  • once a new rule for that resource is deployed, the residual counters keep occupying available concurrency and requests stay BLOCKED.

This PR adds the missing guards so the cleanup task keeps working across rule deletions.

Does this pull request fix one issue?

Fixes #3663

Describe how you did it

  1. RegularExpireStrategy#clearToken: take the rule into a local variable; when getFlowRuleById returns null, the token can never be released by its client anymore (the release path needs the rule), so recycle it directly via removeToken(key, node) and continue — the scan keeps going and the concurrency counter is released through the usual path.
  2. ConcurrentClusterFlowChecker#releaseConcurrentToken: guard the same-origin race — inside the rule-refresh window the concurrency counter can already have been removed by clearAndResetRulesFor, so CurrentConcurrencyManager.get(flowId) may return null; in that case log and return RELEASE_OK instead of NPE-ing at nowCalls.getAndAdd(...).

Both guards follow the conventions of the sibling code paths, which already null-check the same sources (removeToken checks CurrentConcurrencyManager.get(...); releaseConcurrentToken checks getFlowRuleById). This PR only fills in the two missing spots.

Describe how to verify it

Added RegularExpireStrategyNpeTest (JUnit 4, plain in-memory, no real cluster server needed), 3 cases:

  • clearTokenShouldNotThrowWhenRuleHasBeenDeleted: the rule has been removed by a hot update → cleanup must not throw and the token must be recycled. Before this fix it fails with the NPE shown above.
  • clearTokenShouldContinueScanningBehindNodeWithAbsentRule: an expired token located behind an absent-rule node must also be recycled, and its concurrency counter must be released (5 → 4 with acquireCount=1). Before this fix the scan aborts on the absent-rule node and this token starves forever.
  • releaseConcurrentTokenShouldNotThrowWhenCounterHasBeenRemoved: the counter has already been removed by a concurrent rule refresh → the release path must not throw and must return RELEASE_OK.

Run mvn test -DskipTests=false on sentinel-cluster-server-default: 3/3 green with this fix (red without it), and the full module suite passes with no regression.

Special notes for reviews

  • For an absent rule I chose "recycle the token and continue" instead of "skip and keep the token": once the rule is gone the token can never be released through the normal path, so keeping it would leak the cache entry and the concurrency counter forever. If maintainers prefer a softer behavior (keep the node and skip until timeout), happy to adjust.
  • When the concurrency counter is absent, releaseConcurrentToken returns RELEASE_OK; a dedicated status would extend the public TokenResultStatus contract, so I kept the smaller change.

…e flow rule is removed

- RegularExpireStrategy#clearToken: null-check the rule looked up by flowId
  and recycle the token directly when the rule has been removed by a hot
  update, so the scan keeps going instead of aborting on the same node
  every round (which starves all following expired tokens and leaks the
  concurrency counters)
- ConcurrentClusterFlowChecker#releaseConcurrentToken: guard against the
  concurrency counter already being removed by a concurrent rule refresh
@CLAassistant

CLAassistant commented Oct 11, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

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

Labels

None yet

Projects

None yet

2 participants