fix 模块 pod 替换导致 JVM 进程内模块实例消失 #247
Conversation
…ties Add BizOpsPodCoordinator class to handle saving, removing, and accessing biz identities and their model versions. Update InstallBizHandler and UninstallBizHandler to utilize the new coordinator for managing biz identity access and lifecycle. Also, introduce bizModelVersion in ArkBizMeta for better tracking of biz versions.
- Introduced ArkletLogger to log access control failures. - Cleaned up unused imports in BizOpsPodCoordinator. - Added unit tests for BizOpsPodCoordinator functionality.
Improve the access control logic in the canAccess method to handle various cases for bizModelVersion and bizIdentity. This change ensures compatibility with older versions and prevents the execution of outdated commands. Case 1: Allow access if bizModelVersion is empty. Case 2: Allow access if no record exists for bizIdentity. Case 3: Allow access if the version matches the current request. Only deny access when bizModelVersion is not empty and does not match.
WalkthroughAdds a pod-level coordinator for biz operations (BizOpsPodCoordinator), integrates it into install/uninstall handlers (post-install save, pre-uninstall access check and post-uninstall cleanup), extends ArkBizMeta with bizModelVersion, adds comprehensive concurrency tests, and bumps root POM revision to 1.4.3. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant H as InstallBizHandler
participant S as Biz Operation Service
participant I as BizIdentityUtils
participant P as BizOpsPodCoordinator
C->>H: Install request (bizName, bizVersion, bizModelVersion)
H->>S: install(...)
S-->>H: result(success/failure)
alt success
H->>I: generateBizIdentity(bizName,bizVersion)
I-->>H: bizIdentity
H->>P: save(bizIdentity, bizModelVersion)
P-->>H: ack
H-->>C: success
else failure
H-->>C: failure
end
sequenceDiagram
autonumber
participant C as Client
participant H as UninstallBizHandler
participant I as BizIdentityUtils
participant P as BizOpsPodCoordinator
participant S as Biz Operation Service
participant L as Logger
C->>H: Uninstall request (bizName, bizVersion, bizModelVersion)
H->>I: generateBizIdentity(bizName,bizVersion)
I-->>H: bizIdentity
H->>P: canAccess(bizIdentity, bizModelVersion)
alt access denied
H->>L: log error ("expired/unauthorized")
H-->>C: failed response
else access allowed
H->>S: uninstall(...)
S-->>H: result(success/failure)
alt success
H->>P: remove(bizIdentity, bizModelVersion)
P-->>H: ack
H-->>C: success
else failure
H-->>C: failure
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/meta/bizops/ArkBizMeta.java (2)
32-32: New field added; document null/empty semantics.Please clarify whether
nulland""are equivalent on the wire for bizModelVersion (align with BizOpsPodCoordinator which normalizes empty to ""). Add brief Javadoc note.
89-105: Accessor LGTM; consider @nullable hint.If
bizModelVersioncan be absent from older clients, annotate getter/setter with@Nullable(or equivalent) to make intent explicit.arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/builtin/handler/InstallBizHandler.java (1)
74-77: Post‑success save works; consider pre‑save with rollback to shrink race window.Tiny gap exists between install success and
save(...)where a legacy uninstall could sneak in. Option: pre‑save, then rollback mapping on failure.Example:
-InstallBizClientResponse r = convertClientResponse(getOperationService().install(convertInstallRequest(input))); -... -if (ResponseCode.SUCCESS.equals(r.getCode())) { - BizOpsPodCoordinator.save(bizIdentity, bizModelVersion); +// pre-save to gate stale commands, then rollback if install fails +BizOpsPodCoordinator.save(bizIdentity, bizModelVersion); +InstallBizClientResponse r = convertClientResponse(getOperationService().install(convertInstallRequest(input))); +... +if (ResponseCode.SUCCESS.equals(r.getCode())) { return Output.ofSuccess(r); } else { + BizOpsPodCoordinator.remove(bizIdentity, bizModelVersion); return Output.ofFailed(r, "install biz not success!"); }arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/coordinate/BizOpsPodCoordinator.java (2)
79-88: Micro: avoid double map get; consider stricter empty‑version policy.
- Read once for consistency.
- Optional: if recorded version is non‑empty and request is empty, deny (behind a compat flag) to harden against stale commands.
Apply:
public static boolean canAccess(String bizIdentity, String bizModelVersion) { - return StringUtils.isEmpty(bizModelVersion) - || StringUtils.isEmpty(bizIdentityLockMap.get(bizIdentity)) - || bizIdentityLockMap.get(bizIdentity).equals(bizModelVersion); + final String recorded = bizIdentityLockMap.get(bizIdentity); + if (StringUtils.isEmpty(bizModelVersion)) { + // compatibility: allow only if no recorded version + return StringUtils.isEmpty(recorded); + } + return StringUtils.isEmpty(recorded) || recorded.equals(bizModelVersion); }
33-41: Scope note: coordinator is per‑JVM; cross‑pod/state loss on restart.Confirm this meets the intended “pod‑level” guard only. If cross‑pod protection is desired, persist to shared store (e.g., ConfigMap/Redis) or propagate via controller.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/builtin/handler/InstallBizHandler.java(6 hunks)arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/builtin/handler/UninstallBizHandler.java(3 hunks)arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/coordinate/BizOpsPodCoordinator.java(1 hunks)arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/meta/bizops/ArkBizMeta.java(2 hunks)arklet-core/src/test/java/com/alipay/sofa/koupleless/arklet/core/command/coordinate/BizOpsPodCoordinatorTest.java(1 hunks)pom.xml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/builtin/handler/InstallBizHandler.java (1)
arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/coordinate/BizOpsPodCoordinator.java (1)
BizOpsPodCoordinator(33-89)
arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/builtin/handler/UninstallBizHandler.java (5)
arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/coordinate/BizOpsPodCoordinator.java (1)
BizOpsPodCoordinator(33-89)arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/meta/Output.java (1)
Output(28-152)arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/meta/bizops/ArkBizMeta.java (1)
ArkBizMeta(28-124)koupleless-common/src/main/java/com/alipay/sofa/koupleless/common/log/ArkletLogger.java (1)
ArkletLogger(29-407)koupleless-common/src/main/java/com/alipay/sofa/koupleless/common/log/ArkletLoggerFactory.java (1)
ArkletLoggerFactory(28-76)
arklet-core/src/test/java/com/alipay/sofa/koupleless/arklet/core/command/coordinate/BizOpsPodCoordinatorTest.java (1)
arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/coordinate/BizOpsPodCoordinator.java (1)
BizOpsPodCoordinator(33-89)
🔇 Additional comments (12)
arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/builtin/handler/InstallBizHandler.java (2)
59-61: Implements ArkBizOps — good alignment.
88-90: Formatting-only changes — OK.pom.xml (1)
12-12: Version bump OK; verify no hard‑pinned child versions remain.
rg returned "No files were searched". From the repository root re-run and paste outputs:
git grep -n -E '\s1.4.[^<]' || true
git grep -n -F '1.4.2' || true
Fallback (if git grep unavailable):
rg -n --hidden -uu '\s1.4.[^<]' -g '!/target/'
rg -n --hidden -uu '1.4.2' -g '!/target/'arklet-core/src/test/java/com/alipay/sofa/koupleless/arklet/core/command/coordinate/BizOpsPodCoordinatorTest.java (9)
1-38: LGTM! Well-structured test class with proper documentation.The Apache license header, imports, and class structure are appropriate. The Javadoc provides clear context about the test purpose.
40-61: Proper test isolation using reflection to clear static state.The setUp/tearDown methods correctly use reflection to clear the static
bizIdentityLockMapto ensure test isolation. This is a good practice when testing classes with static state.
66-89: LGTM! Good test coverage for normal save operation.Both test methods properly validate the save functionality with realistic business identifiers and use
canAccessto verify the saved state. The test data formats match real-world scenarios.
94-118: LGTM! Comprehensive null and empty string handling tests.These tests properly verify that the coordinator handles null and empty
bizModelVersionvalues correctly, converting them to empty strings as expected by the implementation.
123-137: LGTM! Override behavior correctly tested.The test properly validates that newer versions overwrite older ones and that access control works correctly after the update.
142-193: LGTM! Comprehensive remove operation testing.All three remove scenarios are well-tested:
- Normal removal and verification of subsequent access behavior
- Attempting to remove non-existent entries (should not throw exceptions)
- Version mismatch removal (should not affect existing entries)
The test logic correctly validates the expected behavior based on the
canAccessimplementation.
198-297: LGTM! Thorough canAccess method testing.Excellent coverage of all canAccess scenarios:
- Null/empty
bizModelVersionhandling- Different business identity versions
- Empty values in the map
- Version matching and mismatching
- Non-existent business identities
All assertions align with the coordinator's documented behavior.
302-339: LGTM! Comprehensive concurrency stress test.The test properly validates thread safety with:
- 50 threads performing 100 operations each
- Proper use of
CountDownLatchfor synchronization- Appropriate timeout values (30s for operations, 5s for cleanup)
- Verification that no exceptions are thrown during concurrent access
The test design effectively stresses the
ConcurrentHashMapimplementation.
344-378: LGTM! Multi-identity concurrency test validates isolation.This test effectively validates that concurrent operations on different business identities don't interfere with each other. The assertions within the executor threads properly verify version updates and access control behavior.
| String bizIdentity = BizIdentityUtils.generateBizIdentity(input.getBizName(), input.getBizVersion()); | ||
| String bizModelVersion = input.getBizModelVersion(); | ||
| if (!BizOpsPodCoordinator.canAccess(bizIdentity, bizModelVersion)) { | ||
| LOGGER.error( | ||
| "can not access biz because the command is expired. bizIdentity: {}, bizModelVersion: {}", | ||
| bizIdentity, bizModelVersion); | ||
| return Output.ofFailed("can not access biz because the command is expired"); | ||
| } | ||
| ClientResponse res = getOperationService().uninstall(input.getBizName(), input.getBizVersion()); |
There was a problem hiding this comment.
Access check allows empty bizModelVersion — consider stricter rule behind a flag.
Current logic permits uninstall when request version is empty, even if a non‑empty version is recorded; this weakens protection against stale commands. Suggest denying when map has a non‑empty version and request is empty, gated by a compat flag.
Proposed guard:
-if (!BizOpsPodCoordinator.canAccess(bizIdentity, bizModelVersion)) {
+if (!BizOpsPodCoordinator.canAccess(bizIdentity, bizModelVersion)) {
...
}and inside coordinator (see separate comment) refine empty‑version handling or make it configurable.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/builtin/handler/UninstallBizHandler.java
around lines 52 to 60, the current access check allows an empty bizModelVersion
in the request to pass when a non-empty version is recorded, weakening
protection; update the guard to deny access when the stored coordinator version
is non-empty and the request bizModelVersion is empty, unless a
backward-compatibility flag is enabled. Implement this by: 1) querying
BizOpsPodCoordinator for the recorded version for bizIdentity before the
canAccess call, 2) if recordedVersion is non-empty and
input.getBizModelVersion() is empty, return Output.ofFailed(...) (or call
canAccess only after checking the compat flag), and 3) add a feature
flag/compatibility toggle (checked here or via BizOpsPodCoordinator) to allow
the old behavior when explicitly enabled.
| if (ResponseCode.SUCCESS.equals(res.getCode())) { | ||
| BizOpsPodCoordinator.remove(bizIdentity, bizModelVersion); | ||
| return Output.ofSuccess(res); |
There was a problem hiding this comment.
Possible NPE on remove when bizModelVersion is null (ConcurrentHashMap disallows null value).
BizOpsPodCoordinator.remove(bizIdentity, bizModelVersion) will throw if bizModelVersion == null (legacy clients). Fix in coordinator by normalizing null/empty before calling Map.remove.
Apply coordinator fix shown in that file’s comment; alternatively, add a local guard:
- BizOpsPodCoordinator.remove(bizIdentity, bizModelVersion);
+ if (!StringUtils.isEmpty(bizModelVersion)) {
+ BizOpsPodCoordinator.remove(bizIdentity, bizModelVersion);
+ } else {
+ // legacy client: best-effort cleanup; safe to remove by key
+ BizOpsPodCoordinator.remove(bizIdentity, StringUtils.EMPTY_STRING);
+ }Committable suggestion skipped: line range outside the PR's diff.
| public static void save(String bizIdentity, String bizModelVersion) { | ||
| if (StringUtils.isEmpty(bizModelVersion)) { | ||
| bizModelVersion = StringUtils.EMPTY_STRING; | ||
| } | ||
| bizIdentityLockMap.put(bizIdentity, bizModelVersion); | ||
| } |
There was a problem hiding this comment.
Validate bizIdentity to avoid CHM NPE; normalize inputs.
ConcurrentHashMap disallows null keys; add a guard for bizIdentity and normalize bizModelVersion eagerly.
Apply:
public static void save(String bizIdentity, String bizModelVersion) {
+ if (StringUtils.isEmpty(bizIdentity)) {
+ // ignore invalid key or consider throwing IllegalArgumentException
+ return;
+ }
if (StringUtils.isEmpty(bizModelVersion)) {
bizModelVersion = StringUtils.EMPTY_STRING;
}
bizIdentityLockMap.put(bizIdentity, bizModelVersion);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static void save(String bizIdentity, String bizModelVersion) { | |
| if (StringUtils.isEmpty(bizModelVersion)) { | |
| bizModelVersion = StringUtils.EMPTY_STRING; | |
| } | |
| bizIdentityLockMap.put(bizIdentity, bizModelVersion); | |
| } | |
| public static void save(String bizIdentity, String bizModelVersion) { | |
| if (StringUtils.isEmpty(bizIdentity)) { | |
| // ignore invalid key or consider throwing IllegalArgumentException | |
| return; | |
| } | |
| if (StringUtils.isEmpty(bizModelVersion)) { | |
| bizModelVersion = StringUtils.EMPTY_STRING; | |
| } | |
| bizIdentityLockMap.put(bizIdentity, bizModelVersion); | |
| } |
🤖 Prompt for AI Agents
In
arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/coordinate/BizOpsPodCoordinator.java
around lines 50 to 55, the method saves into a ConcurrentHashMap without
validating the key which can cause a NullPointerException; update the method to
first validate bizIdentity (return or throw IllegalArgumentException if
null/blank) and normalize bizModelVersion eagerly (replace null with
StringUtils.EMPTY_STRING and trim it), then perform the put into
bizIdentityLockMap only with the validated non-null key and normalized value.
...ain/java/com/alipay/sofa/koupleless/arklet/core/command/coordinate/BizOpsPodCoordinator.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/coordinate/BizOpsPodCoordinator.java (2)
50-58: Resolved: save() now validates key and normalizes value — LGTM.This addresses the earlier CHM null-key and normalization concern.
69-78: Resolved: remove() handles null/empty version without NPE — LGTM.Branching to remove by key for empty version fixes the previous remove(key, null) issue.
🧹 Nitpick comments (4)
arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/coordinate/BizOpsPodCoordinator.java (4)
33-34: Make utility class non‑instantiable.Prevent accidental instantiation.
public class BizOpsPodCoordinator { + private BizOpsPodCoordinator() { + // no-op + }
91-99: Minor: avoid potential typo in comment ("arktcl").Should this be "arkctl"? Please confirm and adjust.
35-40: Name nit: bizIdentityLockMap is not a lock.Consider a clearer name like bizModelVersionByBizIdentity.
80-101: Add denial observability.When canAccess() returns false, consider emitting a debug log with bizIdentity and versions for incident triage.
I can draft a minimal logging change if desired.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/coordinate/BizOpsPodCoordinator.java(1 hunks)
🔇 Additional comments (1)
arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/coordinate/BizOpsPodCoordinator.java (1)
90-101: Add tests for null identity and concurrent-remove race
- canAccess(null, non-empty) must not throw; add a test asserting the chosen policy (allow or deny) explicitly.
- canAccess must tolerate the map entry being removed between reads — add a concurrent test that removes the entry mid-check to ensure no NPE/race.
Location: arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/coordinate/BizOpsPodCoordinator.java (lines 90–101).
| public static boolean canAccess(String bizIdentity, String bizModelVersion) { | ||
| // 判断逻辑说明： | ||
| // Case 1: bizModelVersion 为空 - 兼容性处理，允许访问（兼容旧版本 module-controller，arktcl, | ||
| // pod-not-exist 和 pod 紧急删除场景） | ||
| // Case 2: bizIdentityLockMap 中没有该 bizIdentity 的记录，允许访问（安装时不带 | ||
| // BizModelVersion，卸载时带上 BizModelVersion） | ||
| // Case 3: bizIdentityLockMap 中的版本与当前请求的版本匹配 - 版本一致，确认卸载的是该 Biz，允许访问 | ||
| // 只有当 bizModelVersion 不为空且存在 bizModelVersion 且不匹配时，才拒绝访问（防止旧的卸载命令执行） | ||
| return StringUtils.isEmpty(bizModelVersion) | ||
| || StringUtils.isEmpty(bizIdentityLockMap.get(bizIdentity)) | ||
| || bizIdentityLockMap.get(bizIdentity).equals(bizModelVersion); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix NPE risks in canAccess: null key to CHM and TOCTOU on double get.
- If bizIdentity is null/empty, ConcurrentHashMap.get(null) throws.
- Two consecutive get calls can race with a concurrent remove(), making the second equals dereference NPE.
Proposed fix: guard bizIdentity and read the map once; compare safely.
- public static boolean canAccess(String bizIdentity, String bizModelVersion) {
- // 判断逻辑说明：
- // Case 1: bizModelVersion 为空 - 兼容性处理，允许访问（兼容旧版本 module-controller，arktcl,
- // pod-not-exist 和 pod 紧急删除场景）
- // Case 2: bizIdentityLockMap 中没有该 bizIdentity 的记录，允许访问（安装时不带
- // BizModelVersion，卸载时带上 BizModelVersion）
- // Case 3: bizIdentityLockMap 中的版本与当前请求的版本匹配 - 版本一致，确认卸载的是该 Biz，允许访问
- // 只有当 bizModelVersion 不为空且存在 bizModelVersion 且不匹配时，才拒绝访问（防止旧的卸载命令执行）
- return StringUtils.isEmpty(bizModelVersion)
- || StringUtils.isEmpty(bizIdentityLockMap.get(bizIdentity))
- || bizIdentityLockMap.get(bizIdentity).equals(bizModelVersion);
- }
+ public static boolean canAccess(String bizIdentity, String bizModelVersion) {
+ // Case 1: legacy / compatibility
+ if (StringUtils.isEmpty(bizModelVersion)) {
+ return true;
+ }
+ // Guard against null/blank identity (avoid CHM NPE). Choose safer default: deny.
+ if (StringUtils.isEmpty(bizIdentity)) {
+ return false;
+ }
+ final String recorded = bizIdentityLockMap.get(bizIdentity);
+ // Case 2: no record or empty record -> allow
+ // Case 3: exact match -> allow
+ return StringUtils.isEmpty(recorded) || bizModelVersion.equals(recorded);
+ }Optional: if you prefer permissive behavior for blank bizIdentity, return true instead — please confirm desired policy.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/coordinate/BizOpsPodCoordinator.java"
echo "PWD: $(pwd)"
if [ -f "$file" ]; then
echo "=== FOUND: $file ==="
nl -ba "$file" | sed -n '1,260p'
else
echo "=== FILE NOT FOUND: $file ==="
echo "Searching repo for class / filename..."
rg -n --hidden --glob '!**/target/**' 'BizOpsPodCoordinator' -S || true
rg -n --hidden --glob '!**/target/**' 'BizOpsPodCoordinator.java' -S || true
fi
echo
echo "=== Search for bizIdentityLockMap declaration/usages ==="
rg -n --hidden --glob '!**/target/**' 'bizIdentityLockMap' -S || true
echo
echo "=== Search for canAccess(...) call sites ==="
rg -n --hidden --glob '!**/target/**' 'canAccess\(' -S || trueLength of output: 189
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/coordinate/BizOpsPodCoordinator.java"
echo "PWD: $(pwd)"
if [ -f "$file" ]; then
echo "=== FOUND: $file ==="
sed -n '1,240p' "$file"
else
echo "=== FILE NOT FOUND: $file ==="
echo "Searching for BizOpsPodCoordinator..."
rg -n --hidden --glob '!**/target/**' 'class\s+BizOpsPodCoordinator' -S || true
fi
echo
echo "=== Search for bizIdentityLockMap declaration/usages ==="
rg -n --hidden --glob '!**/target/**' 'bizIdentityLockMap' -S || true
echo
echo "=== Search for canAccess(...) call sites ==="
rg -n --hidden --glob '!**/target/**' 'canAccess\(' -S || trueLength of output: 3944
Fix NPE & TOCTOU in canAccess — guard null identity and read map once
ConcurrentHashMap.get(null) throws NPE; calling get() twice races with concurrent remove() and can cause a .equals(...) NPE. Read the map once and validate bizIdentity before lookup.
- public static boolean canAccess(String bizIdentity, String bizModelVersion) {
- // 判断逻辑说明：
- // Case 1: bizModelVersion 为空 - 兼容性处理，允许访问（兼容旧版本 module-controller，arktcl,
- // pod-not-exist 和 pod 紧急删除场景）
- // Case 2: bizIdentityLockMap 中没有该 bizIdentity 的记录，允许访问（安装时不带
- // BizModelVersion，卸载时带上 BizModelVersion）
- // Case 3: bizIdentityLockMap 中的版本与当前请求的版本匹配 - 版本一致，确认卸载的是该 Biz，允许访问
- // 只有当 bizModelVersion 不为空且存在 bizModelVersion 且不匹配时，才拒绝访问（防止旧的卸载命令执行）
- return StringUtils.isEmpty(bizModelVersion)
- || StringUtils.isEmpty(bizIdentityLockMap.get(bizIdentity))
- || bizIdentityLockMap.get(bizIdentity).equals(bizModelVersion);
- }
+ public static boolean canAccess(String bizIdentity, String bizModelVersion) {
+ // Case 1: legacy / compatibility
+ if (StringUtils.isEmpty(bizModelVersion)) {
+ return true;
+ }
+ // Guard against null/blank identity (avoid CHM NPE). Choose safer default: deny.
+ if (StringUtils.isEmpty(bizIdentity)) {
+ return false;
+ }
+ final String recorded = bizIdentityLockMap.get(bizIdentity);
+ // Case 2: no record or empty record -> allow
+ // Case 3: exact match -> allow
+ return StringUtils.isEmpty(recorded) || bizModelVersion.equals(recorded);
+ }If permissive behavior for blank bizIdentity is preferred, change the blank-identity branch to return true — confirm policy.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static boolean canAccess(String bizIdentity, String bizModelVersion) { | |
| // 判断逻辑说明： | |
| // Case 1: bizModelVersion 为空 - 兼容性处理，允许访问（兼容旧版本 module-controller，arktcl, | |
| // pod-not-exist 和 pod 紧急删除场景） | |
| // Case 2: bizIdentityLockMap 中没有该 bizIdentity 的记录，允许访问（安装时不带 | |
| // BizModelVersion，卸载时带上 BizModelVersion） | |
| // Case 3: bizIdentityLockMap 中的版本与当前请求的版本匹配 - 版本一致，确认卸载的是该 Biz，允许访问 | |
| // 只有当 bizModelVersion 不为空且存在 bizModelVersion 且不匹配时，才拒绝访问（防止旧的卸载命令执行） | |
| return StringUtils.isEmpty(bizModelVersion) | |
| || StringUtils.isEmpty(bizIdentityLockMap.get(bizIdentity)) | |
| || bizIdentityLockMap.get(bizIdentity).equals(bizModelVersion); | |
| } | |
| public static boolean canAccess(String bizIdentity, String bizModelVersion) { | |
| // Case 1: legacy / compatibility | |
| if (StringUtils.isEmpty(bizModelVersion)) { | |
| return true; | |
| } | |
| // Guard against null/blank identity (avoid CHM NPE). Choose safer default: deny. | |
| if (StringUtils.isEmpty(bizIdentity)) { | |
| return false; | |
| } | |
| final String recorded = bizIdentityLockMap.get(bizIdentity); | |
| // Case 2: no record or empty record -> allow | |
| // Case 3: exact match -> allow | |
| return StringUtils.isEmpty(recorded) || bizModelVersion.equals(recorded); | |
| } |
🤖 Prompt for AI Agents
In
arklet-core/src/main/java/com/alipay/sofa/koupleless/arklet/core/command/coordinate/BizOpsPodCoordinator.java
around lines 90 to 101, canAccess currently calls
bizIdentityLockMap.get(bizIdentity) twice and will NPE if bizIdentity is null
and can cause a TOCTOU .equals NPE when the map entry is removed concurrently;
fix by first validating bizIdentity (guard against null/blank according to
policy — if blank identity should be allowed, return true), then read
bizIdentityLockMap.get(bizIdentity) once into a local variable and use that
local for the emptiness check and equals comparison so no double lookup or
race-induced NPE occurs.
UninstallBizHandler
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #247 +/- ##
============================================
+ Coverage 73.20% 73.27% +0.07%
+ Complexity 196 195 -1
============================================
Files 105 105
Lines 2668 2668
Branches 285 285
============================================
+ Hits 1953 1955 +2
+ Misses 573 570 -3
- Partials 142 143 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
koupleless/koupleless#405
Summary by CodeRabbit
New Features
Tests
Chores