fix: avoid accidentally deleting new appNamespaceCache of same appid and name with old appNamesppace.

[wyxmttk]

What's the purpose of this PR

This PR addresses a cache eviction bug in AppNamespaceServiceWithCache that occurs when an AppNamespace is deleted and immediately recreated.

The Problem: When an administrator deletes an AppNamespace and recreates it with the same name and appid within a short window (e.g. 1 minute), the scheduled cleanup task (running every 60s) for the old deleted namespace mistakenly removes the cache key associated with the new namespace. This happens because the cache key is derived from appId + namespaceName, leading to a collision.

The Fix: Updated the cache cleanup logic to ensure that the cache is only evicted if it truly belongs to the deleted namespace, preventing accidental deletion of the new namespace's cache.

Which issue(s) this PR fixes:

Fixes #5502

Brief changelog

Fix: Resolve an issue where the cache of a newly created AppNamespace could be accidentally deleted by the cleanup task of a deleted AppNamespace with the same name and appid.

Summary by CodeRabbit

• Bug Fixes
  • Prevented accidental cache deletion when recreating AppNamespace with the same name and appid.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[github-actions]

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

[coderabbitai]

Walkthrough

This PR fixes a race condition in AppNamespaceServiceWithCache where rapid deletion and recreation of an AppNamespace with identical name and appid could cause unintended cache deletion. The fix adds ID verification during cache removal to ensure only the correct entry is deleted, preventing accidental data loss.

Changes

Cohort / File(s)	Summary
Documentation CHANGES.md	Added changelog entry documenting the bugfix for preventing accidental cache deletion during AppNamespace recreation scenarios.
Cache Deletion Logic Fix apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/AppNamespaceServiceWithCache.java	Replaced specific java.util.* imports with wildcard import. Modified handleDeletedAppNamespaces() to retrieve cached AppNamespace by key and verify ID match before removal, preventing deletion of newly created entries with the same name.
Test Coverage apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/AppNamespaceServiceWithCacheTest.java	Added doubleCheckForAppNamespaceCache() test method via reflection to validate cache synchronization and integrity across old and new AppNamespace entries. Added helper invokePrivateMethod() for reflective invocation of private methods.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

• The core fix (ID verification in handleDeletedAppNamespaces()) is straightforward but requires careful review to confirm it resolves the race condition scenario
• Verify that the doubleCheckForAppNamespaceCache() test adequately covers the rapid delete-and-recreate workflow described in the issue

Suggested labels

size:M, lgtm

Suggested reviewers

• nobodyiam

Poem

🐰 A swift fix hops into the cache so bright,
No more mistaken deletes in the night!
With ID checks guarding each precious key,
AppNamespaces recreate wild and free! 🍃

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main bug fix: preventing accidental deletion of new AppNamespace cache when recreating with same appId and name, which is the core change in this PR.
Linked Issues check	✅ Passed	The code changes directly address the issue #5502 by adding ID matching validation in handleDeletedAppNamespaces to prevent cache deletion of newly created namespaces, with comprehensive test coverage.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing the cache eviction bug: core logic fix, test validation, imports cleanup, and changelog entry—no unrelated modifications present.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

• Provide your own instructions using the high_level_summary_instructions setting.
• Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
• Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
4. 📊 Contributor Summary — Include a Markdown table showing contributions:
   | Contributor | Lines Added | Lines Removed | Files Changed |
5. ✔️ Additional Notes — Add any extra reviewer context.
   Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/AppNamespaceServiceWithCache.java (1)

40-40: Wildcard java.util.* import is functionally fine but may diverge from existing style

Switching to java.util.* brings Objects into scope and works, but if the project convention prefers explicit imports, consider importing java.util.Objects instead and keeping the previous specific java.util imports for consistency.

-import java.util.*;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.Objects;

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 17746f6 and b8c06ec.

📒 Files selected for processing (3)

• CHANGES.md (1 hunks)
• apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/AppNamespaceServiceWithCache.java (2 hunks)
• apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/AppNamespaceServiceWithCacheTest.java (2 hunks)

🧰 Additional context used

🧠 Learnings (1)

📓 Common learnings

Learnt from: nobodyiam
Repo: apolloconfig/apollo PR: 5228
File: apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/NamespaceService.java:356-363
Timestamp: 2024-10-01T07:29:12.146Z
Learning: In the `NamespaceService#save` method, implementing a soft limit for namespace counts is acceptable, even if there may be potential race conditions due to concurrent access.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: build (8)

🔇 Additional comments (4)

CHANGES.md (1)

25-25: Changelog entry correctly captures the bugfix

The description and linked issue match the behavior this PR addresses; this is a good, minimal update to the 2.5.0 notes.

apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/AppNamespaceServiceWithCache.java (1)

262-266: Deleted-cache handling now correctly avoids removing a newly recreated namespace

The additional check against appNamespace.getId() ensures you only evict from appNamespaceCache when the cached entry actually corresponds to the deletedId. This closes the race where a deleted AppNamespace’s cleanup could remove the cache entry for a newly created AppNamespace with the same appId and namespaceName, while still correctly cleaning up genuinely deleted entries.

apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/AppNamespaceServiceWithCacheTest.java (2)

41-41: Static import anyList is appropriate for the new mocking logic

The static import for anyList is used cleanly in the new stubbing of findAllById and keeps the test setup concise.

---

315-364: New regression test accurately captures and guards against the cache-deletion bug

doubleCheckForAppNamespaceCache carefully walks through the sequence: load old namespace (id=1), load new namespace (id=2) with the same appId and namespaceName, then invoke updateAndDeleteCache and assert that the cache still points to id=2. Using invokePrivateMethod to reach scanNewAppNamespaces and updateAndDeleteCache keeps the test focused on cache behavior without altering the production API and provides solid protection against regressions of issue #5502.

[wyxmttk]

I have read the CLA Document and I hereby sign the CLA

[spaceluke]

LGTM