Skip to content

<fix>[multi]: batch guard NPE quality issues #3378

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
zstack-robot-1 wants to merge 1 commit into
base: 5.5.6
Choose a base branch
from
Open

<fix>[multi]: batch guard NPE quality issues #3378

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion compute/src/main/java/org/zstack/compute/allocator/HostAllocatorChain.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,10 @@ private void runFlow(AbstractHostAllocatorFlow flow) {
}
} catch (Throwable t) {
logger.warn("unhandled throwable", t);
completion.fail(inerr(ORG_ZSTACK_COMPUTE_ALLOCATOR_10019, t.toString()));
String errMsg = t != null ? t.toString() : "unknown error";
if (completion != null) {
completion.fail(inerr(ORG_ZSTACK_COMPUTE_ALLOCATOR_10019, errMsg));
}
Comment on lines +149 to +152
Copy link

@coderabbitai coderabbitai bot Feb 20, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

dryRun 异常时未回调，可能导致调用方挂起。

dryRun() 场景下 completion 为空，当前 catch 只检查 completion，会吞掉异常导致调用方永远收不到失败回调。

🛠️ 建议修复 
-            String errMsg = t != null ? t.toString() : "unknown error";
-            if (completion != null) {
-                completion.fail(inerr(ORG_ZSTACK_COMPUTE_ALLOCATOR_10019, errMsg));
-            }
+            String errMsg = t != null ? t.toString() : "unknown error";
+            if (isDryRun) {
+                if (dryRunCompletion != null) {
+                    dryRunCompletion.fail(inerr(ORG_ZSTACK_COMPUTE_ALLOCATOR_10019, errMsg));
+                }
+            } else if (completion != null) {
+                completion.fail(inerr(ORG_ZSTACK_COMPUTE_ALLOCATOR_10019, errMsg));
+            }
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@compute/src/main/java/org/zstack/compute/allocator/HostAllocatorChain.java`
around lines 149 - 152, The catch block in HostAllocatorChain swallows
exceptions when completion is null (dryRun), causing callers to hang; update the
handler so that if completion != null it calls
completion.fail(inerr(ORG_ZSTACK_COMPUTE_ALLOCATOR_10019, errMsg)), but if
completion == null (dryRun) rethrow a runtime exception (e.g., new
CloudRuntimeException or similar) created with
inerr(ORG_ZSTACK_COMPUTE_ALLOCATOR_10019, errMsg) or wrap the original Throwable
as the cause, ensuring the error surfaces to the caller instead of being
swallowed.

}
}

Expand Down
2 changes: 1 addition & 1 deletion compute/src/main/java/org/zstack/compute/host/HostBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1443,7 +1443,7 @@ public String getName() {

@Override
protected String getDeduplicateString() {
return String.format("connect-host-%s", self.getUuid());
return String.format("connect-host-%s", self == null ? "unknown" : self.getUuid());
}
});
}
Expand Down
6 changes: 6 additions & 0 deletions compute/src/main/java/org/zstack/compute/vm/VmCapabilitiesJudger.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,12 @@ private void checkPrimaryStorageCapabilities(VmCapabilities capabilities, VmInst
q.add(PrimaryStorageVO_.uuid, SimpleQuery.Op.EQ, rootVolume.getPrimaryStorageUuid());
String type = q.findValue();

if (type == null) {
capabilities.setSupportLiveMigration(false);
capabilities.setSupportVolumeMigration(false);
return;
}

PrimaryStorageType psType = PrimaryStorageType.valueOf(type);

if (vm.getState() != VmInstanceState.Running) {
Expand Down
3 changes: 2 additions & 1 deletion core/src/main/java/org/zstack/core/db/UpdateQueryImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,8 @@ public UpdateQuery set(SingularAttribute attr, Object val) {
@Override
public UpdateQuery condAnd(SingularAttribute attr, Op op, Object val) {
if ((op == Op.IN || op == Op.NOT_IN) && !(val instanceof Collection)) {
throw new CloudRuntimeException(String.format("for operation IN or NOT IN, a Collection value is expected, but %s got", val.getClass()));
throw new CloudRuntimeException(String.format("for operation IN or NOT IN, a Collection value is expected, but %s got",
val == null ? "null" : val.getClass()));
}

Cond cond = new Cond();
Expand Down
3 changes: 3 additions & 0 deletions core/src/main/java/org/zstack/core/log/LogSafeGson.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,9 @@ private static <T> JsonSerializer<T> getSerializer() {
}

public static JsonElement toJsonElement(Object o) {
if (o == null) {
return JsonNull.INSTANCE;
}
return logSafeGson.toJsonTree(o, getGsonType(o.getClass()));
}

Expand Down
2 changes: 1 addition & 1 deletion header/src/main/java/org/zstack/header/vm/VmAbnormalLifeCycleStruct.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ boolean match(VmAbnormalLifeCycleStruct struct) {
boolean match(VmAbnormalLifeCycleStruct struct) {
return struct.getOriginalState() == VmInstanceState.Paused
&& struct.getCurrentState() == VmInstanceState.Stopped
&& struct.getCurrentHostUuid().equals(struct.getOriginalHostUuid());
&& Objects.equals(struct.getCurrentHostUuid(), struct.getOriginalHostUuid());
}
},
VmMigrateToAnotherHost {
Expand Down
5 changes: 5 additions & 0 deletions identity/src/main/java/org/zstack/identity/AuthorizationManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,11 @@ public APIMessage intercept(APIMessage msg) throws ApiMessageInterceptionExcepti
session = evaluateSession(msg);
}

if (session == null) {
throw new ApiMessageInterceptionException(err(ORG_ZSTACK_IDENTITY_10012, IdentityErrors.INVALID_SESSION,
"evaluated session is null for message[%s]", msg.getMessageName()));
}

logger.trace(String.format("authorizing message[%s] with user[accountUuid:%s, uuid:%s] session",
msg.getMessageName(),
session.getAccountUuid(),
Expand Down
6 changes: 4 additions & 2 deletions plugin/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3974,15 +3974,17 @@ public void done() {
mon.connect(new Completion(releaseLock) {
@Override
public void success() {
String monUuid = mon.getSelf() == null ? "unknown" : mon.getSelf().getUuid();
logger.debug(String.format("successfully reconnected the mon[uuid:%s] of the ceph primary" +
" storage[uuid:%s, name:%s]", mon.getSelf().getUuid(), self.getUuid(), self.getName()));
" storage[uuid:%s, name:%s]", monUuid, self.getUuid(), self.getName()));
releaseLock.done();
}

@Override
public void fail(ErrorCode errorCode) {
String monUuid = mon.getSelf() == null ? "unknown" : mon.getSelf().getUuid();
logger.warn(String.format("failed to reconnect the mon[uuid:%s] server of the ceph primary" +
" storage[uuid:%s, name:%s], %s", mon.getSelf().getUuid(), self.getUuid(), self.getName(), errorCode));
" storage[uuid:%s, name:%s], %s", monUuid, self.getUuid(), self.getName(), errorCode));
releaseLock.done();
}
});
Expand Down
4 changes: 2 additions & 2 deletions plugin/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageMonBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ public void fail(ErrorCode errorCode) {

@Override
public String getName() {
return String.format("connect-ceph-primary-storage-mon-%s", self.getUuid());
return String.format("connect-ceph-primary-storage-mon-%s", self == null ? "unknown" : self.getUuid());
}
});
}
Expand Down Expand Up @@ -420,7 +420,7 @@ public void fail(ErrorCode errorCode) {

@Override
public String getName() {
return String.format("ping-ceph-primary-storage-%s", self.getUuid());
return String.format("ping-ceph-primary-storage-%s", self == null ? "unknown" : self.getUuid());
}
});
}
Expand Down
7 changes: 6 additions & 1 deletion ...lstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageAllocatorFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,12 @@ private String getHostUuidFromAllocateMsg(AllocatePrimaryStorageSpaceMsg msg) {
throw new OperationFailureException(
argerr(ORG_ZSTACK_STORAGE_PRIMARY_LOCAL_10023, "invalid uri, correct example is file://$URL;hostUuid://$HOSTUUID or volume://$VOLUMEUUID "));
}
hostUuid = uriParsers.get(protocol).parseUri(msg.getRequiredInstallUri()).hostUuid;
AbstractUriParser parser = uriParsers.get(protocol);
if (parser == null) {
throw new OperationFailureException(
argerr(ORG_ZSTACK_STORAGE_PRIMARY_LOCAL_10023, "unsupported protocol[%s] in uri[%s]", protocol, msg.getRequiredInstallUri()));
}
hostUuid = parser.parseUri(msg.getRequiredInstallUri()).hostUuid;
}

if (hostUuid != null) {
Expand Down
3 changes: 2 additions & 1 deletion ...n/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageKvmBackend.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3797,7 +3797,8 @@ public void success(CheckInitializedFileRsp rsp) {

@Override
public void fail(ErrorCode errorCode) {
completion.fail(operr(ORG_ZSTACK_STORAGE_PRIMARY_LOCAL_10081, "cannot find flag file [%s] on host [%s], because: %s", makeInitializedFilePath(), hostUuid, errorCode.getCause().getDetails()));
String causeDetails = errorCode.getCause() != null ? errorCode.getCause().getDetails() : errorCode.getDetails();
completion.fail(operr(ORG_ZSTACK_STORAGE_PRIMARY_LOCAL_10081, "cannot find flag file [%s] on host [%s], because: %s", makeInitializedFilePath(), hostUuid, causeDetails));
}
});
}
Expand Down