<chore>[cleanup]: fix empty catches, add NRF annotations, expand wildcard imports#3467
Open
zstack-robot-2 wants to merge 2 commits into5.5.12from
Open
<chore>[cleanup]: fix empty catches, add NRF annotations, expand wildcard imports#3467zstack-robot-2 wants to merge 2 commits into5.5.12from
zstack-robot-2 wants to merge 2 commits into5.5.12from
Conversation
…card imports 1. Empty catch blocks (8 files): added proper logging or interrupt handling 2. NoRollbackFlow annotations (~2800): added // DEBT comments with context 3. Wildcard imports (~2200 files): expanded to explicit imports Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Walkthrough该拉取请求系统地将整个代码库中数百个Java文件的通配符导入（import xyz.*）替换为显式导入。受影响的模块包括compute、configuration、console、core、header、identity、image和network等，涵盖allocator、cluster、host、VM、zone、configuration、cloudbus、workflow等多个子包。除了导入语句的细粒度化和部分添加的DEBT注释外，无运行时逻辑、控制流或公共API变更。 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (17)
compute/src/main/java/org/zstack/compute/vm/VmExpungeMemoryVolumeFlow.java (2)
32-32:⚠️ Potential issue | 🟡 Minor日志记录器使用了错误的类。
logger使用VmExpungeRootVolumeFlow.class而非当前类VmExpungeMemoryVolumeFlow.class，这会导致日志输出时类名不正确，影响问题定位。🔧 建议修复
- private static final CLogger logger = Utils.getLogger(VmExpungeRootVolumeFlow.class); + private static final CLogger logger = Utils.getLogger(VmExpungeMemoryVolumeFlow.class);🤖 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/vm/VmExpungeMemoryVolumeFlow.java` at line 32, The logger is instantiated with the wrong class reference; change the Utils.getLogger(...) call in VmExpungeMemoryVolumeFlow to use VmExpungeMemoryVolumeFlow.class instead of VmExpungeRootVolumeFlow.class so the private static final CLogger logger reflects the current class (update the Utils.getLogger argument where the logger field is declared).
40-42:⚠️ Potential issue | 🔴 Critical严重缺陷：空检查后缺少
return语句，将导致 NPE。当
getAllVolumes()返回null时，调用trigger.next()后代码继续执行到第 44 行，对null调用.stream()将抛出NullPointerException。🐛 建议修复
if (spec.getVmInventory().getAllVolumes() == null) { trigger.next(); + return; }🤖 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/vm/VmExpungeMemoryVolumeFlow.java` around lines 40 - 42, The null-check in VmExpungeMemoryVolumeFlow currently calls trigger.next() when spec.getVmInventory().getAllVolumes() == null but does not return, causing a subsequent .stream() on null and an NPE; update the block in VmExpungeMemoryVolumeFlow so that after invoking trigger.next() you immediately return from the method (or flow) to avoid further processing of spec.getVmInventory().getAllVolumes(), ensuring no .stream() is called on a null collection.compute/src/main/java/org/zstack/compute/vm/VmExpungeCacheVolumeFlow.java (3)
32-32:⚠️ Potential issue | 🟡 Minor日志记录器使用了错误的类。
logger使用VmExpungeRootVolumeFlow.class而非当前类VmExpungeCacheVolumeFlow.class。🔧 建议修复
- private static final CLogger logger = Utils.getLogger(VmExpungeRootVolumeFlow.class); + private static final CLogger logger = Utils.getLogger(VmExpungeCacheVolumeFlow.class);🤖 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/vm/VmExpungeCacheVolumeFlow.java` at line 32, The logger was initialized with the wrong class; update the Utils.getLogger call in VmExpungeCacheVolumeFlow to pass VmExpungeCacheVolumeFlow.class instead of VmExpungeRootVolumeFlow.class so the static field logger correctly identifies the current class (reference: the static field logger and the Utils.getLogger(...) invocation).
40-42:⚠️ Potential issue | 🔴 Critical严重缺陷：空检查后缺少
return语句，将导致 NPE。与
VmExpungeMemoryVolumeFlow相同的问题：当getAllVolumes()返回null时，代码继续执行到第 44 行，对null调用.stream()将抛出NullPointerException。🐛 建议修复
if (spec.getVmInventory().getAllVolumes() == null) { trigger.next(); + return; }🤖 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/vm/VmExpungeCacheVolumeFlow.java` around lines 40 - 42, In VmExpungeCacheVolumeFlow, the null check for spec.getVmInventory().getAllVolumes() calls trigger.next() but fails to return, so execution continues and later calls .stream() on null; fix by adding a return immediately after trigger.next() in the block that checks spec.getVmInventory().getAllVolumes() == null to prevent further processing (i.e., ensure the method exits after calling trigger.next()).
64-65:⚠️ Potential issue | 🟡 Minor日志消息中使用了错误的 UUID。
日志描述的是 cache volume 失败，但输出的是
getRootVolumeUuid()而非实际失败的 cache volume UUID (vol.getUuid())。🔧 建议修复
logger.warn(String.format("failed to expunge the cache volume[uuid:%s] of the vm[uuid:%s, name:%s], %s", - spec.getVmInventory().getRootVolumeUuid(), spec.getVmInventory().getUuid(), + vol.getUuid(), spec.getVmInventory().getUuid(), spec.getVmInventory().getName(), reply.getError()));🤖 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/vm/VmExpungeCacheVolumeFlow.java` around lines 64 - 65, The log in VmExpungeCacheVolumeFlow incorrectly prints the root volume UUID; update the logger.warn call in the catch/failure path to use the actual cache volume UUID (vol.getUuid()) instead of spec.getVmInventory().getRootVolumeUuid(), ensuring the message parameters (spec.getVmInventory().getUuid(), spec.getVmInventory().getName(), and the exception/message) remain in the same order and match the format string.header/src/main/java/org/zstack/header/identity/role/RoleIdentity.java (1)
26-26:⚠️ Potential issue | 🟡 Minor修正错误消息中的语法错误。
错误消息中存在语法错误："if not found" 应改为 "is not found"。根据编码规范，代码（包括错误消息）应使用正确的、无拼写错误的英文。
🔤 建议的修复
- throw new IllegalArgumentException("Role identity: " + typeName + "if not found"); + throw new IllegalArgumentException("Role identity: " + typeName + " is not found");注意：同时在 "is" 前添加了一个空格以改善消息格式。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/identity/role/RoleIdentity.java` at line 26, Fix the typo in the IllegalArgumentException message in RoleIdentity: change the message thrown in the RoleIdentity class (the line that constructs the exception using variable typeName) from "if not found" to "is not found" and ensure there is a space before "is" so the final message reads "Role identity: " + typeName + " is not found".identity/src/main/java/org/zstack/identity/login/LoginManagerImpl.java (1)
216-233:⚠️ Potential issue | 🟡 Minor把
DEBT注释写成原因，而不是位置。这两处新增注释现在只写了
in doLogIn，描述的是位置，不是这里继续使用NoRollbackFlow的具体原因。这样后续做 NRF 清理时，无法据此判断缺的是哪类回滚语义，也不利于按注释检索和分批治理。既然这个 PR 的目标是补齐NoRollbackFlow债务标记，建议把每个 flow 各自“为什么不能安全回滚”写清楚。As per coding guidelines, “对于较长的注释，需要仔细校对并随代码更新，确保内容正确。”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@identity/src/main/java/org/zstack/identity/login/LoginManagerImpl.java` around lines 216 - 233, 将位于 doLogIn 中的 DEBT 注释从仅写位置（"in doLogIn"）改为说明为何该 flow 使用 NoRollbackFlow（不能或不需要回滚的具体原因）；在 run-before-login-extensions 的 NoRollbackFlow（涉及 matchedAuthExtensions 和 loginContext）上写明：哪些状态/资源不能回滚、回滚会引入何种不一致或副作用、以及为何需要继续下一步而不是回滚。更新该注释以便将来按语义检索和分批治理。core/src/main/java/org/zstack/core/encrypt/EncryptManagerImpl.java (3)
70-71:⚠️ Potential issue | 🟡 Minor变量命名违反编码规范。
变量
old_key和new_key使用了下划线命名法，应改为 lowerCamelCase 风格。根据编码规范："方法名、参数名、成员变量和局部变量：使用 lowerCamelCase 风格。"♻️ 建议的命名修正
- String old_key = EncryptGlobalConfig.ENCRYPT_ALGORITHM.value(); - String new_key = msg.getEncryptKey(); + String oldKey = EncryptGlobalConfig.ENCRYPT_ALGORITHM.value(); + String newKey = msg.getEncryptKey();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/zstack/core/encrypt/EncryptManagerImpl.java` around lines 70 - 71, Rename the local variables old_key and new_key in EncryptManagerImpl (the block using EncryptGlobalConfig.ENCRYPT_ALGORITHM.value() and msg.getEncryptKey()) to lowerCamelCase (oldKey and newKey) to comply with naming conventions; update every reference to these variables within the method to use oldKey/newKey and ensure no other code relies on the underscore names.
77-103:⚠️ Potential issue | 🔴 Critical关键问题：SQL注入漏洞和N+1查询性能问题。
此代码段存在多个严重问题：
SQL注入漏洞（关键安全问题）：
- 第77行和第82行使用字符串拼接构建SQL语句，直接将
className和uuid插入SQL中- 虽然
className来自反射，但仍存在安全风险；uuid来自查询结果，应使用参数化查询N+1查询问题（关键性能问题）：
- 第81-103行的循环内嵌套数据库查询，每个uuid执行2-3次数据库访问
- 违反编码规范："禁止循环里套查询，避免嵌套查询带来的性能问题"
- 对于大量记录会导致严重的性能瓶颈
查询方法使用不当：
- 第91行使用
createQuery()但SQL语法看起来像原生SQL（直接使用表名），应使用createNativeQuery()🔒 建议的安全和性能优化方案
推荐重构为批量处理方式：
`@Transactional` private void handle(APIUpdateEncryptKeyMsg msg) { Set<Method> map = Platform.encryptedMethodsMap; logger.debug("decrypt passwords with old key and encrypt with new key"); EncryptRSA rsa = new EncryptRSA(); for (Method method : map) { Class<?> tempClass = method.getDeclaringClass(); String className = tempClass.getSimpleName(); // 使用参数化查询，一次性获取所有需要的数据 String jpql = "SELECT e.uuid, e.password FROM " + className + " e"; TypedQuery<?> query = dbf.getEntityManager().createQuery(jpql, tempClass); List<?> results = query.getResultList(); // 批量处理 for (Object result : results) { try { // 提取uuid和password // 解密、加密、更新操作 // 建议使用参数化的批量更新 } catch (Exception e) { logger.error(String.format("Failed to re-encrypt password for entity %s, uuid: %s", className, "uuid_value"), e); } } } try { rsa.updateKey(msg.getEncryptKey()); } catch (Exception e) { logger.error("Failed to update encryption key", e); throw new RuntimeException("Encryption key update failed", e); } APIUpdateEncryptKeyEvent evt = new APIUpdateEncryptKeyEvent(msg.getId()); bus.publish(evt); }注意：由于涉及加密密钥更新的关键业务逻辑，建议：
- 使用事务确保数据一致性
- 考虑添加回滚机制
- 记录详细的错误日志以便追踪
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/zstack/core/encrypt/EncryptManagerImpl.java` around lines 77 - 103, The loop in EncryptManagerImpl that builds SQL with string concatenation (variables className, uuidList/uuid, paramName) causes SQL injection and N+1 queries; refactor handle(APIUpdateEncryptKeyMsg msg) to fetch all target rows in a single parameterized JPQL/Criteria query (use EntityManager.createQuery with entity aliases instead of string concatenation or NativeQuery), iterate results in-memory to decrypt/re-encrypt via rsa.decrypt1/rsa.encrypt, and perform a parameterized batch update (or persist merged entities) rather than per-uuid ad-hoc SQL; also replace createQuery/createNativeQuery misuse (use createNativeQuery only for true native SQL) and add transaction boundaries and proper logging for exceptions.
97-101:⚠️ Potential issue | 🟠 Major事务中的异常处理不当可能导致数据不一致。
在
@Transactional方法中捕获异常后继续执行可能导致严重问题：
- 第97-101行： 加密失败后继续处理下一条记录，可能导致部分记录使用旧密钥、部分使用新密钥
- 第107-111行： 密钥更新失败后仍然发送成功事件，造成数据与配置不一致
根据编码规范："在事务操作中，捕获异常后要确保触发手动回滚。"
建议：
- 捕获具体的异常类型而非
Exception- 关键失败应抛出异常触发事务回滚
- 或实现补偿机制确保数据一致性
🛡️ 建议的异常处理改进
try { String password = (String) rsa.decrypt1(preEncrypttxt); String newencrypttxt = (String) rsa.encrypt(password, msg.getEncryptKey()); String sql3 = "update "+className+" set "+paramName+" = :newencrypttxt where uuid = :uuid"; Query query = dbf.getEntityManager().createQuery(sql3); query.setParameter("newencrypttxt", newencrypttxt); query.setParameter("uuid", uuidList.get(i)); query.executeUpdate(); - } catch (Exception e) { - logger.debug("sql exec error"); - logger.debug(String.format("error is : %s", e.getMessage())); - e.printStackTrace(); + } catch (Exception e) { + logger.error(String.format("Failed to re-encrypt password for %s, uuid: %s", + className, uuidList.get(i)), e); + throw new RuntimeException("Encryption update failed, rolling back transaction", e); } } } try { rsa.updateKey(msg.getEncryptKey()); - } catch (Exception e) { - logger.debug("update key in encryptrsa error"); - logger.debug(String.format("error is : %s", e.getMessage())); - e.printStackTrace(); + } catch (Exception e) { + logger.error("Failed to update encryption key", e); + throw new RuntimeException("Encryption key update failed", e); }Also applies to: 107-111
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/zstack/core/encrypt/EncryptManagerImpl.java` around lines 97 - 101, The catch-all Exception handling in EncryptManagerImpl swallows errors (logger.debug + e.printStackTrace) inside the transactional flow; change these catch blocks to catch specific exceptions (e.g., SQLException, EncryptionException) and on critical failures rethrow a runtime exception (or the original wrapped) so the transaction will roll back, replace e.printStackTrace with logger.error including the exception, and ensure the key-update success path (the block around the other catch at lines ~107-111) does not emit success events when an exception occurred; in short, locate the try/catch in EncryptManagerImpl, remove broad Exception catches, log errors at error level with exception details, and rethrow to trigger transactional rollback or implement a compensating action if silent continuation is required.identity/src/main/java/org/zstack/identity/IAMIdentityResourceGenerator.java (2)
53-53:⚠️ Potential issue | 🟡 Minor重命名或重组策略：
readAPIs列表应避免包含非读取操作
APIUpdateUserMsg使用HttpMethod.PUT方法（更新操作），但被添加到名为readAPIs的列表和"read-apis-for-normal-account"策略中。虽然设计意图是允许普通用户更新自己的账户信息，但这个命名产生了语义矛盾——按照 API 设计规范，PUT 操作不应归入"read"（读取）类别。建议：
- 将策略重命名为
"basic-apis-for-normal-account"或"user-self-service-apis"，更准确地反映其包含的操作类型。- 或为
APIUpdateUserMsg创建单独的策略，与读取操作区分。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@identity/src/main/java/org/zstack/identity/IAMIdentityResourceGenerator.java` at line 53, The readAPIs list currently contains APIUpdateUserMsg (an update/PUT operation) and is used to build the "read-apis-for-normal-account" policy; rename or re-scope to avoid mixing non-read operations: either (A) rename the policy and variable (e.g., change readAPIs -> basicApisForNormalAccount or userSelfServiceApis and rename the policy string "read-apis-for-normal-account" accordingly) so it accurately reflects included methods, or (B) remove APIUpdateUserMsg from readAPIs and create a separate list/policy (e.g., selfServiceUpdateApis / "user-self-service-apis") that includes APIUpdateUserMsg; update any references that construct policies from readAPIs or the old policy name to the new identifiers (readAPIs, APIUpdateUserMsg, and the policy string).
60-60:⚠️ Potential issue | 🟡 Minor
.replace("\\", "")移除所有反斜杠会破坏 JSON 的转义字符。虽然当前
actions仅包含 API 类名（如org.zstack.core.foo.APIBar），不包含特殊字符，但这种直接后处理 JSON 的做法是脆弱的。如果未来数据包含需要转义的字符（如包含引号的字符串、特殊符号等），反斜杠的移除将破坏有效的 JSON 转义序列（\",\\,\uXXXX等），导致后续PolicyStatement.fromJSON()反序列化失败。建议查明为何需要移除反斜杠（是否存在双重序列化问题），并修复根本原因而非后处理 JSON 字符串。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@identity/src/main/java/org/zstack/identity/IAMIdentityResourceGenerator.java` at line 60, The line assigning readAPIsForNormalAccountJSONStatement currently strips all backslashes which will break valid JSON escape sequences; remove the .replace("\\", "") and fix the root cause by ensuring JSONObjectUtil.toJsonString(list(s)) is fed the actual list (no double-serialization) so it emits a correct JSON string that PolicyStatement.fromJSON() can consume; locate usages around readAPIsForNormalAccountJSONStatement and JSONObjectUtil.toJsonString, eliminate any double-encoding/unnecessary prior to-JSON conversion (or deserialize then reserialize once), and add a unit/test to verify that strings with quotes and escapes round-trip through PolicyStatement.fromJSON().network/src/main/java/org/zstack/network/l3/FirstAvailableIpAllocatorStrategy.java (1)
3-3:⚠️ Potential issue | 🟡 Minor发现未使用的导入。
org.apache.commons.math3.analysis.function.Add在此文件中未被使用，应当移除。🧹 建议修复
package org.zstack.network.l3; -import org.apache.commons.math3.analysis.function.Add; import org.zstack.core.db.Q;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/src/main/java/org/zstack/network/l3/FirstAvailableIpAllocatorStrategy.java` at line 3, Remove the unused import org.apache.commons.math3.analysis.function.Add from FirstAvailableIpAllocatorStrategy.java; locate the import statement at the top of the file (reference symbol: class FirstAvailableIpAllocatorStrategy) and delete that import line so only necessary imports remain.network/src/main/java/org/zstack/network/l3/L3NetworkManagerImpl.java (1)
593-620:⚠️ Potential issue | 🟡 Minor这批
DEBT注释还是模板文本。Line 593 的
in syncManagementServiceTypeWhileCreate已经和紧邻的add-l3-to-sdn-controllerflow 对不上，Line 619 的reason TBD也还是占位。建议把同类新增注释统一改成真实原因，不要只留下位置名或模板文案。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/src/main/java/org/zstack/network/l3/L3NetworkManagerImpl.java` around lines 593 - 620, Replace the placeholder DEBT comments in L3NetworkManagerImpl (specifically inside the syncManagementServiceTypeWhileCreate sequence around the NoRollbackFlow named "add-l3-to-sdn-controller" and the subsequent NoRollbackFlow) with concise, accurate explanations: state why rollback is intentionally omitted, any known risks, and the conditions that justify NoRollbackFlow (e.g., external SDN controller side-effects in createL3Network, inability to undo external changes in controllerL3.createL3Network), and remove generic "reason TBD" text so the comment matches the actual flow purpose and location.compute/src/main/java/org/zstack/compute/host/HostManagerImpl.java (1)
535-607:⚠️ Potential issue | 🟡 Minor这些
DEBT注释还不是最终可用文案。Line 535 和 Line 556 的
in callPlugins已经和后面的 flow 不匹配，Line 593/606 的reason TBD也还是占位。建议把这一组注释统一改成具体原因，否则这次 debt 标注后续没有追踪价值。🤖 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/host/HostManagerImpl.java` around lines 535 - 607, The DEBT comments attached to the NoRollbackFlow instances (including the flow with __name__ "send-connect-host-message", the skip-check flow for baremetal2, and the flow named "check-host-os-version") are placeholders and inconsistent with the actual flows; update each DEBT comment to a clear, specific reason describing why the flow is NoRollbackFlow (e.g., "no rollback because this flow only sends async connect message and relies on host-side cleanup", "skip baremetal2 architecture comparison: architecture managed elsewhere", "validation-only flow; cannot rollback state changes"), and ensure the text near the flow definitions (the comments around the run/skip methods) matches the flow's behavior so future reviewers can trace intent and follow-up actions.compute/src/main/java/org/zstack/compute/vm/VmAllocateHostAndPrimaryStorageFlow.java (1)
229-250:⚠️ Potential issue | 🟡 Minor把
DEBT注释补成真正的原因。
in rollback只是在描述位置，没有说明为什么这里必须保留NoRollbackFlow。按这次 PR 自己定义的// DEBT: NoRollbackFlow — <reason>目标，这两处后续仍然无法支撑 debt 追踪。🤖 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/vm/VmAllocateHostAndPrimaryStorageFlow.java` around lines 229 - 250, 在 VmAllocateHostAndPrimaryStorageFlow 中把占位的 “DEBT: NoRollbackFlow — in rollback” 注释替换为能追踪根因的具体说明：在 NoRollbackFlow 的 run 方法（类/方法标识：VmAllocateHostAndPrimaryStorageFlow, NoRollbackFlow.run）里对每个 VolumeSpec 跳过已创建卷的回收并发送 IncreasePrimaryStorageCapacityMsg 给 PrimaryStorage（使用 IncreasePrimaryStorageCapacityMsg、PrimaryStorageConstant.SERVICE_ID、bus.send(...) 等符号），请明确写出为什么该步骤不能在 rollback 中执行（例如：已删除的卷在删除时已归还容量/回滚无法安全撤销对容量的修改或会导致重复归还），并在两个相同的注释位置分别替换为同样具体的原因和关联的风险与假设，以便后续 debt 跟踪与审查。network/src/main/java/org/zstack/network/l3/L3BasicNetwork.java (1)
503-529:⚠️ Potential issue | 🟡 Minor这里的 debt 标记已经出现错误定位。
Line 503 和 Line 528 写的是
in getName，但相邻代码是删除 L3 的 flow，不是getName()。这说明这批DEBT: NoRollbackFlow注释里还有模板/拷贝残留；同文件里其它新增的reason TBD也建议一起补成真实原因。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/src/main/java/org/zstack/network/l3/L3BasicNetwork.java` around lines 503 - 529, The DEBT comments near the NoRollbackFlow instances in L3BasicNetwork are incorrect/misleading (they say "in getName" but the flow is "remove-from-sdn-controller"); update those DEBT markers to reflect the actual issue and add concrete reasons (e.g., "NoRollbackFlow: ignore SDN controller delete failures to avoid blocking L3 deletion") and replace any other "reason TBD" entries in this file with specific rationale; search for the anonymous NoRollbackFlow (the instance with String __name__ = "remove-from-sdn-controller") and nearby flows in L3BasicNetwork and revise the inline DEBT/comment templates to accurately describe why NoRollbackFlow is used and what the risk/tech-debt is.
🧹 Nitpick comments (18)
core/src/main/java/org/zstack/core/agent/AgentManagerImpl.java (1)
125-126: DEBT 注释的原因描述不够清晰。当前的注释
// DEBT: NoRollbackFlow — in url中 "in url" 的含义不够明确。建议使用更具描述性的原因说明，例如：
// DEBT: NoRollbackFlow — echo is idempotent, no rollback needed// DEBT: NoRollbackFlow — init config is overwrite-safe这样可以帮助后续维护者理解为什么这里使用
NoRollbackFlow以及是否需要改进。不过考虑到这是一个大规模清理 PR（约 2,800 处标记），如果团队已有统一的标记规范，可以忽略此建议。
Also applies to: 145-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/zstack/core/agent/AgentManagerImpl.java` around lines 125 - 126, The DEBT comment on the NoRollbackFlow usage is ambiguous; update the comment near the flow(new NoRollbackFlow() { ... }) in AgentManagerImpl to state a clear, concrete reason why rollback is not required (e.g., "DEBT: NoRollbackFlow — echo is idempotent, no rollback needed" or "DEBT: NoRollbackFlow — init config is overwrite-safe"), and apply the same clearer wording for the other occurrence around the flow(...) at the indicated region so future maintainers understand why NoRollbackFlow was chosen.network/src/main/java/org/zstack/network/l2/L2VlanNetworkFactory.java (1)
91-92: DEBT 注释格式可考虑改进（可选）。当前注释
// DEBT: NoRollbackFlow — in rollback中的原因 "in rollback" 语义不够清晰。考虑到这是一个大规模清理 PR（约 2,800 处），当前格式可以接受。如果后续有时间，建议将原因改为更具描述性的说明，例如解释为什么此处使用 NoRollbackFlow 或需要后续处理什么。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/src/main/java/org/zstack/network/l2/L2VlanNetworkFactory.java` around lines 91 - 92, The DEBT comment near the NoRollbackFlow instantiation in L2VlanNetworkFactory is vague ("in rollback"); update the comment to a clearer, self-contained explanation of why NoRollbackFlow is used here and what follow-up is required (e.g., "DEBT: using NoRollbackFlow because X avoids Y during rollback; TODO: revisit to implement compensating action Z"), so locate the anonymous NoRollbackFlow block and replace the short note with a descriptive comment that documents the rationale and any next steps.image/src/main/java/org/zstack/image/ImageBase.java (1)
492-492: DEBT 注释格式不一致。此处的 DEBT 注释使用了
in scripts，而其他位置（如 line 539, 1053, 1069, 1087）统一使用reason TBD。建议统一格式以保持代码一致性。♻️ 建议的修改
- // DEBT: NoRollbackFlow — in scripts + // DEBT: NoRollbackFlow — reason TBD chain.then(new NoRollbackFlow() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@image/src/main/java/org/zstack/image/ImageBase.java` at line 492, The DEBT comment in ImageBase.java is inconsistent; replace the comment containing "DEBT: NoRollbackFlow — in scripts" so it matches the project's standard (e.g., use "reason TBD"). Locate the comment that references NoRollbackFlow and update the trailing text to "reason TBD" so it matches other DEBT comments (lines referencing NoRollbackFlow in this file), keeping the same comment style and placement.core/src/main/java/org/zstack/core/captcha/CaptchaImpl.java (1)
116-120: 建议：使用 logger 替代e.printStackTrace()。文件中已定义
logger（第46行），但异常处理中使用的是e.printStackTrace()。这会将堆栈信息输出到 stderr 而非结构化日志，不利于生产环境排查问题。此问题在
refreshCaptcha方法（第141-145行）中也存在。♻️ 建议的修复方案
} catch (FileNotFoundException e) { - e.printStackTrace(); + logger.warn("Failed to generate captcha image", e); } catch (IOException e) { - e.printStackTrace(); + logger.warn("IO error while generating captcha image", e); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/zstack/core/captcha/CaptchaImpl.java` around lines 116 - 120, 在 CaptchaImpl 类中捕获异常时使用了 e.printStackTrace(); 请在所有相关方法（包括 refreshCaptcha 和出现 printStackTrace 的其他 catch 块）改为使用类中已定义的 logger（变量名 logger），例如用 logger.error("Failed to ...", e) 或 logger.warn(...) 并传入异常对象以记录堆栈信息，确保日志信息包含有意义的上下文描述字符串并保留原始异常作为参数。core/src/main/java/org/zstack/core/cloudbus/EventFacadeImpl.java (1)
163-163: 建议添加泛型参数提升类型安全性。此行使用了原始类型（raw type）
Map ret = new HashMap()，而方法返回类型（第150行）声明为Map<String, String>。建议添加泛型参数以提升类型安全性并消除编译器警告。注：这是预存在的问题，与本次导入清理无关，可以在后续优化中处理。
♻️ 建议的类型参数修复
- Map ret = new HashMap(); + Map<String, String> ret = new HashMap<>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/zstack/core/cloudbus/EventFacadeImpl.java` at line 163, Replace the raw Map usage to match the method's declared return type: change the local variable ret in EventFacadeImpl (currently declared as Map ret = new HashMap()) to use generics, e.g. Map<String, String> ret = new HashMap<>(); — ensure both the variable declaration and the HashMap instantiation include the <String, String> type parameters to remove raw-type warnings and improve type safety.compute/src/main/java/org/zstack/compute/host/PostHostExtensionPointForNuma.java (2)
39-39: DEBT 注释应当更具体地说明技术债务的内容当前的 DEBT 注释仅仅重复了代码位置信息（"in createPostHostConnectFlow"），但没有说明：
- 为什么此处使用 NoRollbackFlow
- 这个技术债务的具体问题是什么
- 未来需要如何改进
建议改进注释，例如：
// DEBT: NoRollbackFlow — NUMA topology update failures are currently ignored; // consider adding rollback logic to revert host connection state on persistent failures这样可以让后续维护者清楚地了解需要改进的方向。
🤖 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/host/PostHostExtensionPointForNuma.java` at line 39, Replace the vague DEBT comment inside class PostHostExtensionPointForNuma (near createPostHostConnectFlow) with a concrete note that explains why NoRollbackFlow is used, what the technical debt is (e.g., NUMA topology update failures are ignored and can leave host state inconsistent), and a suggested remediation (e.g., add rollback logic to revert host connection state or retry/compensate on persistent failures); reference createPostHostConnectFlow and NoRollbackFlow in the comment so future maintainers know the impacted code paths and the intended improvement.
79-114: 建议提取重复的比较逻辑第 87-90 行和第 106-109 行包含几乎完全相同的 NUMA 节点比较逻辑。这违反了 DRY 原则，增加了维护成本。
建议将比较逻辑提取为独立的方法：
♻️ 建议的重构方案
+ private boolean isNumaNodeEqual(HostNumaNodeVO node1, HostNumaNodeVO node2) { + return node1.getNodeCPUs().equals(node2.getNodeCPUs()) + && node1.getNodeDistance().equals(node2.getNodeDistance()) + && Objects.equals(node1.getNodeMemSize(), node2.getNodeMemSize()) + && Objects.equals(node1.getNodeID(), node2.getNodeID()); + } + oldHostNumaNodes.stream() .filter(oldNumaNode -> newHostNumaNodes.stream() - .noneMatch(newNumaNode -> { - logger.trace(String.format("oldNumaNode.getNodeCPUs(): %s, newNumaNode.getNodeCPUs(): %s, equals: %s", oldNumaNode.getNodeCPUs(), newNumaNode.getNodeCPUs(), newNumaNode.getNodeCPUs().equals(oldNumaNode.getNodeCPUs()))); - logger.trace(String.format("oldNumaNode.getNodeDistance(): %s, newNumaNode.getNodeDistance(): %s, equals: %s", oldNumaNode.getNodeDistance(), newNumaNode.getNodeDistance(), newNumaNode.getNodeDistance().equals(oldNumaNode.getNodeDistance()))); - logger.trace(String.format("oldNumaNode.getNodeMemSize(): %s, newNumaNode.getNodeMemSize(): %s, equals: %s", oldNumaNode.getNodeMemSize(), newNumaNode.getNodeMemSize(), Objects.equals(newNumaNode.getNodeMemSize(), oldNumaNode.getNodeMemSize()))); - logger.trace(String.format("oldNumaNode.getNodeID(): %s, newNumaNode.getNodeID(): %s, equals: %s", oldNumaNode.getNodeID(), newNumaNode.getNodeID(), Objects.equals(newNumaNode.getNodeID(), oldNumaNode.getNodeID()))); - - return newNumaNode.getNodeCPUs().equals(oldNumaNode.getNodeCPUs()) - && newNumaNode.getNodeDistance().equals(oldNumaNode.getNodeDistance()) - && Objects.equals(newNumaNode.getNodeMemSize(), oldNumaNode.getNodeMemSize()) - && Objects.equals(newNumaNode.getNodeID(), oldNumaNode.getNodeID()); - })) + .noneMatch(newNumaNode -> isNumaNodeEqual(newNumaNode, oldNumaNode))) .forEach(oldNumaNode -> { logger.trace("remove old numa node: " + JSONObjectUtil.toJsonString(oldNumaNode)); dbf.removeByPrimaryKey(oldNumaNode.getId(), HostNumaNodeVO.class); }); newHostNumaNodes.stream() .filter(newNumaNode -> oldHostNumaNodes.stream() - .noneMatch(oldNumaNode -> { - logger.trace(String.format("oldNumaNode.getNodeCPUs(): %s, newNumaNode.getNodeCPUs(): %s, equals: %s", oldNumaNode.getNodeCPUs(), newNumaNode.getNodeCPUs(), newNumaNode.getNodeCPUs().equals(oldNumaNode.getNodeCPUs()))); - logger.trace(String.format("oldNumaNode.getNodeDistance(): %s, newNumaNode.getNodeDistance(): %s, equals: %s", oldNumaNode.getNodeDistance(), newNumaNode.getNodeDistance(), newNumaNode.getNodeDistance().equals(oldNumaNode.getNodeDistance()))); - logger.trace(String.format("oldNumaNode.getNodeMemSize(): %s, newNumaNode.getNodeMemSize(): %s, equals: %s", oldNumaNode.getNodeMemSize(), newNumaNode.getNodeMemSize(), Objects.equals(newNumaNode.getNodeMemSize(), oldNumaNode.getNodeMemSize()))); - logger.trace(String.format("oldNumaNode.getNodeID(): %s, newNumaNode.getNodeID(): %s, equals: %s", oldNumaNode.getNodeID(), newNumaNode.getNodeID(), Objects.equals(newNumaNode.getNodeID(), oldNumaNode.getNodeID()))); - - return oldNumaNode.getNodeCPUs().equals(newNumaNode.getNodeCPUs()) - && oldNumaNode.getNodeDistance().equals(newNumaNode.getNodeDistance()) - && Objects.equals(oldNumaNode.getNodeMemSize(), newNumaNode.getNodeMemSize()) - && Objects.equals(oldNumaNode.getNodeID(), newNumaNode.getNodeID()); - })) + .noneMatch(oldNumaNode -> isNumaNodeEqual(oldNumaNode, newNumaNode))) .forEach(newNumaNode -> { logger.trace("persist newNumaNode: " + JSONObjectUtil.toJsonString(newNumaNode)); dbf.persist(newNumaNode); });这样可以提高代码的可维护性，并确保比较逻辑的一致性。
🤖 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/host/PostHostExtensionPointForNuma.java` around lines 79 - 114, Extract the duplicated NUMA node comparison into a single helper method (e.g., numaNodesEqual(HostNumaNodeVO a, HostNumaNodeVO b)) that encapsulates the equality checks for getNodeCPUs(), getNodeDistance(), getNodeMemSize(), and getNodeID() and optionally the trace logging, then replace the two inline noneMatch/noneMatch lambdas in the streams over oldHostNumaNodes and newHostNumaNodes with calls to this helper; update the streams that call dbf.removeByPrimaryKey(oldNumaNode.getId(), HostNumaNodeVO.class) and dbf.persist(newNumaNode) to use the new method so comparison logic is centralized and DRY.console/src/main/java/org/zstack/console/ManagementServerConsoleProxyBackend.java (2)
584-584: DEBT 注释可以更清晰地说明原因注释 "in rollback" 仅说明了位置，但未解释为什么该流程不需要回滚逻辑。建议补充具体原因，例如重连操作是幂等的，或该操作本身是补偿性动作，无需额外回滚。
💡 建议的改进
- // DEBT: NoRollbackFlow — in rollback + // DEBT: NoRollbackFlow — reconnection is idempotent, no state to rollback🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/src/main/java/org/zstack/console/ManagementServerConsoleProxyBackend.java` at line 584, 在 ManagementServerConsoleProxyBackend 类中将现有的注释 "DEBT: NoRollbackFlow — in rollback" 扩展为解释原因：说明是哪一个具体流程/方法使用了 NoRollbackFlow（例如重连或会话恢复流程），并简要写明为什么不需要回滚逻辑（例如该操作是幂等的，或者该流程本身为补偿性动作且回滚会产生副作用），以便后续维护者理解设计权衡并判断未来是否需要变更。
489-489: DEBT 注释可以更具体当前注释使用了占位符 "reason TBD"（待定），建议补充具体原因。该 NoRollbackFlow 执行的是端口可用性验证，不涉及状态修改，因此无需回滚逻辑。
💡 建议的改进
- // DEBT: NoRollbackFlow — reason TBD + // DEBT: NoRollbackFlow — verification only, no state changes to rollback🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/src/main/java/org/zstack/console/ManagementServerConsoleProxyBackend.java` at line 489, Replace the placeholder DEBT comment on the NoRollbackFlow with a concrete explanation: indicate that this NoRollbackFlow inside ManagementServerConsoleProxyBackend performs only port availability validation and does not modify any persistent state or resources, therefore rollback logic is unnecessary; update the comment near the NoRollbackFlow instantiation to state this explicitly (mentioning port availability/validation) so future readers understand why no rollback is required.console/src/main/java/org/zstack/console/AbstractConsoleProxyBackend.java (2)
185-185: 需要补充技术债务的具体原因DEBT 注释中的 "reason TBD" 是一个占位符，应该在代码合并前填写具体原因。添加 DEBT 注释的目的是记录技术债务的上下文，帮助未来的维护者理解为什么此处可以接受无回滚的设计。
建议说明：
- 为什么在新建控制台失败时，不需要回滚已删除的旧控制台？
- 这种情况下系统如何恢复？（例如：用户可以重试、管理节点会自动重连等）
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/src/main/java/org/zstack/console/AbstractConsoleProxyBackend.java` at line 185, The DEBT comment "// DEBT: NoRollbackFlow — reason TBD" in AbstractConsoleProxyBackend should be replaced with a concrete explanation of why the flow intentionally has no rollback and how the system recovers; update the comment near the NoRollbackFlow usage in AbstractConsoleProxyBackend to state (1) why deleting the old console before creating the new one is acceptable (e.g., old console is ephemeral, state is idempotent, or deletion leaves no persistent side effects), and (2) the recovery path (e.g., caller can retry creation, management node will detect missing console and reconnect or recreate, and relevant cleanup is safe), referencing the specific flow or method name surrounding the comment so future maintainers can trace behavior. Ensure the comment is concise but includes the two points (rationale and recovery/mitigation) and mentions any assumptions (idempotency, retries, or monitoring) relied upon by the NoRollbackFlow.
165-165: 建议提供更具体的技术债务原因当前的 DEBT 注释只说明了代码位置（"in grantConsoleAccess"），但没有解释为什么此处使用 NoRollbackFlow 是可接受的技术债务。建议补充具体原因，例如：是否因为旧控制台即将被替换所以无需回滚？还是因为删除失败可以通过其他机制恢复？
清晰的技术债务文档有助于后续维护者理解设计决策。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/src/main/java/org/zstack/console/AbstractConsoleProxyBackend.java` at line 165, 在 AbstractConsoleProxyBackend 类中针对原有注释 "DEBT: NoRollbackFlow — in grantConsoleAccess" 补充具体的技术债务原因：在 grantConsoleAccess 方法或相邻注释中说明为什么使用 NoRollbackFlow 是可接受（例如：该流程对资源创建是幂等的、失败可以通过补偿流程 recoverConsoleAccess/cleanupConsoleResources 恢复、该旧控制台即将被替换且不会长期维护，或有外部监控/重试机制保证一致性），并列出预期的修复条件或替代方案（什么时候应移除该 DEBT）。请直接修改该注释位置（保留 "DEBT" 标签以便追踪），包含具体理由、影响范围和短期/长期解决路径以便后续维护者理解决策。core/src/main/java/org/zstack/core/errorcode/ElaborationManagerImpl.java (1)
131-131: 把reason TBD补成具体原因。这些
DEBT: NoRollbackFlow注释现在只是占位，后续只能看出“这里有债务”，看不出为什么必须保留NoRollbackFlow、风险边界是什么、以及什么条件下可以清理。既然这次清理的目标之一就是给 NRF 标注原因，建议把这里补到至少和 Line 108、Line 326 这种上下文颗粒度一致。Also applies to: 147-147, 180-180, 203-203, 237-237, 254-254
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/zstack/core/errorcode/ElaborationManagerImpl.java` at line 131, 当前代码中在 ElaborationManagerImpl 内多处保留了占位式注释 "DEBT: NoRollbackFlow — reason TBD"，需要将其替换为具体原因说明；请在每个出现该占位注释的地方（类 ElaborationManagerImpl 中那些标注 NoRollbackFlow 的注释点）明确写出为什么必须使用 NoRollbackFlow、其风险边界是什么、以及在何种条件下可以移除或改造该 flow，保持注释颗粒度和已有更详细注释（同一文件内已写明上下文的注释）一致；对文件中所有类似注释点都进行同样处理，确保每条注释包含：问题背景、为何不能回滚、潜在影响/风险、以及清理或改进的触发条件或替代方案。core/src/main/java/org/zstack/core/encrypt/EncryptManagerImpl.java (2)
73-73: 使用原始类型缺少泛型参数。建议使用
Class<?>替代原始类型Class以提高类型安全性。♻️ 建议的类型优化
- Class tempClass = method.getDeclaringClass(); + Class<?> tempClass = method.getDeclaringClass();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/zstack/core/encrypt/EncryptManagerImpl.java` at line 73, Replace the raw Class usage with a generic Class<?> for the variable tempClass returned from method.getDeclaringClass(); update the declaration "Class tempClass" to "Class<?> tempClass" (and adjust any related variable declarations/usages in EncryptManagerImpl that rely on tempClass) to restore generic type-safety while preserving existing logic around method.getDeclaringClass().
75-75: 避免使用魔法值。硬编码的字符串
"password"应定义为常量。根据编码规范："避免使用魔法值（Magic Value）：直接使用未经定义的数值或字符串应替换为枚举或常量。"♻️ 建议的常量定义
在类开头添加常量：
private static final String PASSWORD_PARAM = "password";然后修改使用处：
- String paramName = "password"; + String paramName = PASSWORD_PARAM;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/zstack/core/encrypt/EncryptManagerImpl.java` at line 75, Replace the hard-coded string "password" in EncryptManagerImpl with a well-named constant: add a private static final String PASSWORD_PARAM = "password" near the top of the EncryptManagerImpl class and change the local declaration where paramName is set (currently String paramName = "password") to use PASSWORD_PARAM instead (e.g., String paramName = PASSWORD_PARAM) so the magic value is centralized and reusable.identity/src/main/java/org/zstack/identity/AccountManagerImpl.java (1)
427-427: 把NoRollbackFlow的具体债务原因写出来。这两条注释现在只重复了方法名，没有说明“为什么这里不能回滚/缺了什么补偿路径”，后续检索到这里只能再回头读完整个 flow，达不到这次批量加 NRF 注释的目的。建议把原因补成针对各自 flow 的一句话说明。
✏️ 可参考的写法
- // DEBT: NoRollbackFlow — in doChangeResourceOwnerInQueue + // DEBT: NoRollbackFlow — explain why rollback is intentionally skipped for quota validation ... - // DEBT: NoRollbackFlow — in doChangeResourceOwnerInQueue + // DEBT: NoRollbackFlow — explain why ownership transfer currently has no compensation pathAlso applies to: 457-457
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@identity/src/main/java/org/zstack/identity/AccountManagerImpl.java` at line 427, 在 doChangeResourceOwnerInQueue 方法里把当前的“// DEBT: NoRollbackFlow — in doChangeResourceOwnerInQueue”注释替换为一句具体说明，明确写出该 NoRollbackFlow 的债务原因（例如说明为何该 flow 无法回滚或缺少补偿步骤：比如操作不可逆、缺少事务回滚点、依赖外部不可补偿资源或需手动补偿的步骤），并对另一个相同注释位置（在相应的 NoRollbackFlow 使用处）做同样处理；引用符号：NoRollbackFlow 和 doChangeResourceOwnerInQueue，以便后续检索时能直接看到“为什么不能回滚/需要补偿”的简短描述。compute/src/main/java/org/zstack/compute/allocator/HostAllocatorManagerImpl.java (1)
548-549: 避免用占位词记录NoRollbackFlow债务
reason TBD/in rollback这类描述不足以解释这里为什么要用NoRollbackFlow。后续如果要审计 host allocation 链路的回滚语义，这些注释基本起不到定位和决策作用。建议补成具体原因，例如“该步骤无外部副作用”或“容量归还已由前一个 Flow 的 rollback 负责”。Also applies to: 603-604
🤖 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/HostAllocatorManagerImpl.java` around lines 548 - 549, Replace the placeholder comment "DEBT: NoRollbackFlow — reason TBD" that precedes chain.then(new NoRollbackFlow()) in HostAllocatorManagerImpl with a concise, concrete explanation of why NoRollbackFlow is used (e.g., "NoRollbackFlow because this step has no external side effects" or "NoRollbackFlow because capacity rollback is handled by the previous Flow's rollback"), and apply the same change to the other occurrence; mention the exact rollback rationale and, if relevant, reference an issue/PR ID for future auditability so reviewers can understand why rollback is intentionally omitted.core/src/main/java/org/zstack/core/config/GlobalConfigFacadeImpl.java (1)
117-118: 把NoRollbackFlow的债务注释写成具体原因这两处 DEBT 注释现在一个写成了位置说明（而且这里实际在
handle(APIRefreshGuestOsMetadataMsg)里，不是在handleMessage里），另一个还是占位词reason TBD。后面排查为什么这里必须保留NoRollbackFlow时，这种注释几乎提供不了有效信息。建议直接写成不可回滚的具体约束或副作用原因。Also applies to: 148-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/zstack/core/config/GlobalConfigFacadeImpl.java` around lines 117 - 118, 把占位的 DEBT 注释替换为能说明为何这里必须使用 NoRollbackFlow 的具体原因：在 chain.then(new NoRollbackFlow() { … }) 的两处出现（其中一处在 handle(APIRefreshGuestOsMetadataMsg) 内而非 handleMessage），写明不可回滚的具体约束或副作用（例如外部资源已变更/已发送不可撤回的消息/事务边界不能回滚、并指出会影响的状态变量或后续流程）。确保注释包含触发不可回滚的条件、潜在风险和参考的状态/资源名称，便于后来排查为何不能改为可回滚流。configuration/src/main/java/org/zstack/configuration/DiskOfferingBase.java (1)
164-166: DEBT 注释位置略有不规范。Line 165 的 DEBT 注释放置在
}).then(中间，格式上不太规范。建议将注释放在.then(new NoRollbackFlow() {的上一行，与 Line 149 和 Line 183 的格式保持一致。不过这只是格式问题，不影响功能。
建议的格式调整
} - // DEBT: NoRollbackFlow — reason TBD - }).then(new NoRollbackFlow() { + }) + // DEBT: NoRollbackFlow — reason TBD + .then(new NoRollbackFlow() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@configuration/src/main/java/org/zstack/configuration/DiskOfferingBase.java` around lines 164 - 166, Move the DEBT comment so it sits on its own line immediately above the `.then(new NoRollbackFlow() {` invocation for consistent formatting; locate the `}).then(new NoRollbackFlow() {` occurrence in DiskOfferingBase.java and reposition the `// DEBT: NoRollbackFlow — reason TBD` comment to the previous line (matching the style used around lines with other similar comments such as above the `.then(` at line 149 and line 183).
| } | ||
| }); | ||
|
|
||
| // DEBT: NoRollbackFlow — in stopFailedToMigrateVms |
There was a problem hiding this comment.
这些 DEBT 注释的定位写错了。
in stopFailedToMigrateVms、in getSyncLevel 和当前 flow 位置对不上，后续排查时会误导成债务来自别的方法。这里最好改成直接描述当前 NoRollbackFlow 不能安全回滚的真实原因，或者至少写成当前 flow 名称。
As per coding guidelines, "对于较长的注释，需要仔细校对并随代码更新，确保内容正确。"
Also applies to: 1391-1392, 1411-1412
🤖 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/host/HostBase.java` at line 630,
当前文件中多处以 "DEBT: NoRollbackFlow — in stopFailedToMigrateVms"（以及类似的 “in
getSyncLevel”）写法将债务注释指向了错误的方法，容易误导后续排查；请在含有 NoRollbackFlow 的流（例如当前 flow
的类/方法）内把这些注释改为准确描述该 NoRollbackFlow 无法安全回滚的真实原因，或至少改成写明当前 flow 的名称（例如使用当前 flow
名称替代 "stopFailedToMigrateVms"/"getSyncLevel"），并同步更新其它相同位置的注释（提到的其他位置也要一致改为当前
flow 名称或准确原因）。
| flow(new VmReleaseNetworkServiceOnChangeIPFlow()); | ||
| } | ||
|
|
||
| // DEBT: NoRollbackFlow — in rollback |
There was a problem hiding this comment.
这些 DEBT 注释还是占位符。
in rollback 和 reason TBD 只说明了位置或“还没想好”，没有说明这里为什么必须使用 NoRollbackFlow、缺少什么补偿动作。当前文件后面很多新增注释都沿用了这个模式，这会让这批 debt 标记几乎无法用于后续收敛。建议把原因写成可执行的信息，比如“外部状态已提交且当前没有补偿接口”这类具体描述。
Also applies to: 1572-1572, 1605-1605
🤖 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/vm/VmInstanceBase.java` at line
1555, 这些 DEBT 注释只是占位符（“in rollback”“reason TBD”），需要改成可执行的说明：在使用 NoRollbackFlow
的每个位置（引用 NoRollbackFlow、rollback 的地方，例如 VmInstanceBase.java
中当前出现的三处）说明为什么不能回滚、缺少哪些补偿动作或接口、以及潜在的后续解决方案；例如写明“外部系统已提交且无补偿
API”“变更是不可逆的资源分配，需手动补偿”等。请在所有出现 NoRollbackFlow
的注释中用具体原因替换占位符并列出缺失的补偿点和建议的修复方向（例如新增补偿接口或记录可重试的补偿任务），以便后续可验证和收敛。
|
|
||
| final FlowChain chain = FlowChainBuilder.newSimpleFlowChain(); | ||
| chain.setName(String.format("destroy-vm-%s", self.getUuid())); | ||
| // DEBT: NoRollbackFlow — in getName |
There was a problem hiding this comment.
这两处 DEBT 注释的指向写错了。
in getName 和 in onPlatformUpdated 都和紧跟着的 NoRollbackFlow 实际职责对不上，后续排查时会被误导。这里应直接描述该 flow 不可回滚的真实原因，而不是引用不相关的方法名。
Also applies to: 6052-6052
🤖 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/vm/VmInstanceBase.java` at line
3384, The DEBT comments referencing "in getName" and "in onPlatformUpdated" are
incorrect and misleading next to the NoRollbackFlow declarations; update those
two comments (near the NoRollbackFlow instances) to remove the wrong method
names and instead state the actual reason the flow is non-rollback (e.g., side
effects that cannot be reversed, external state changes, or idempotency
constraints). Locate the NoRollbackFlow usages in class VmInstanceBase (the ones
currently annotated with "in getName" and "in onPlatformUpdated") and replace
those misleading annotations with a concise explanation of the flow's
irreversibility and any relevant context needed for future debugging.
Comment on lines
+1559
to
1560
| // DEBT: NoRollbackFlow — in rollback | ||
| flow(new NoRollbackFlow() { |
There was a problem hiding this comment.
这里的 DEBT 注释说明不准确。
in rollback、in getSyncSignature 和下面实际包裹的 flow 没有对应关系，读代码的人很难从注释判断为什么这里必须保留 NoRollbackFlow。建议把注释改成当前步骤的不可回滚原因，或关联到具体 flow 名称 / issue。
As per coding guidelines, "对于较长的注释，需要仔细校对并随代码更新，确保内容正确。"
Also applies to: 1692-1693, 1705-1706
🤖 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/vm/VmInstanceManagerImpl.java`
around lines 1559 - 1560, The DEBT comment before the anonymous NoRollbackFlow
is inaccurate and misleading; replace it with a concise explanation of why this
particular flow must be non-rollback (e.g., the specific side-effect it performs
or external resource it commits) and, if relevant, reference the exact flow name
or an issue ID (rather than “in rollback” or “in getSyncSignature”) so readers
can trace the rationale; update the same style of comment for the other
NoRollbackFlow occurrences that mention getSyncSignature to state the concrete
non-rollback reason tied to those flows or link to the tracking issue.
Comment on lines
+261
to
262
| // DEBT: NoRollbackFlow — in rollback | ||
| flow(new NoRollbackFlow() { |
There was a problem hiding this comment.
把 DEBT: NoRollbackFlow 注释改成“原因”，不要写成位置。
这几处 DEBT 现在写的是所在方法/位置，而不是为什么这里必须保留 NoRollbackFlow。这样后续排查时仍然不知道缺失的补偿动作是什么、是否由外层流程兜底。既然这个 PR 的目标是补 // DEBT: NoRollbackFlow — <reason>，这里建议改成具体原因，比如“dry-run 不持久化资源，无需回滚”或“失败时由外层 DeleteVolume 清理”。
Also applies to: 292-293, 413-414, 438-439
🤖 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/vm/VmInstantiateOtherDiskFlow.java`
around lines 261 - 262, Replace vague "DEBT: NoRollbackFlow — in rollback"
comments in VmInstantiateOtherDiskFlow with a concrete reason why NoRollbackFlow
is required (e.g., "DEBT: NoRollbackFlow — dry-run does not persist resources,
no rollback needed" or "DEBT: NoRollbackFlow — failures are cleaned by outer
DeleteVolume flow"), updating every NoRollbackFlow instance in this class so
each comment explains what compensating action is missing or which outer flow is
responsible for cleanup; search for all usages of NoRollbackFlow in
VmInstantiateOtherDiskFlow and change the comment to the specific cause for not
implementing rollback.
Comment on lines
+201
to
202
| // DEBT: NoRollbackFlow — in getName | ||
| chain.then(new NoRollbackFlow() { |
There was a problem hiding this comment.
DEBT 注释写成了错误的位置说明。
这里标的是删除级联 flow，但注释写成 in getName，后续基本无法据此定位真实债务点。建议改成当前删除步骤为什么不能回滚，或者直接写当前 flow 的语义名称。
As per coding guidelines, "对于较长的注释，需要仔细校对并随代码更新，确保内容正确。"
Also applies to: 217-218
🤖 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/zone/ZoneBase.java` around lines 201
- 202, The "DEBT" comment next to the NoRollbackFlow is incorrect/misleading
("in getName") — update the comment at the two occurrences that wrap
chain.then(new NoRollbackFlow()) in class ZoneBase to either (a) state the real
reason this delete cascade flow must not roll back (e.g., side effects or
external state that can't be reverted) or (b) replace it with a clear semantic
name describing the flow's purpose (e.g., "DEBT: non-rollback delete cascade due
to X" or "DEBT: cleanup-delete-flow - non-rollback"); ensure the comment sits
immediately above the chain.then(new NoRollbackFlow()) block and accurately
reflects the flow semantics so future readers can locate and understand the
debt.
Comment on lines
152
to
154
| if (msg.getDeletionMode() == APIDeleteMessage.DeletionMode.Permissive) { | ||
| // DEBT: NoRollbackFlow — in updateInstanceOffering | ||
| chain.then(new NoRollbackFlow() { |
There was a problem hiding this comment.
DEBT 注释内容不准确。
Line 153 的注释 // DEBT: NoRollbackFlow — in updateInstanceOffering 描述不正确。这段代码位于 handle(APIDeleteInstanceOfferingMsg msg) 方法中，处理的是删除操作的级联检查流程，而非 updateInstanceOffering。
建议修改为与其他文件一致的格式：// DEBT: NoRollbackFlow — reason TBD
建议的修复
- // DEBT: NoRollbackFlow — in updateInstanceOffering
+ // DEBT: NoRollbackFlow — reason TBD🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@configuration/src/main/java/org/zstack/configuration/InstanceOfferingBase.java`
around lines 152 - 154, The DEBT comment is incorrect: in InstanceOfferingBase
inside the handle(APIDeleteInstanceOfferingMsg msg) method the comment "// DEBT:
NoRollbackFlow — in updateInstanceOffering" wrongly references
updateInstanceOffering; update the comment near the chain.then(new
NoRollbackFlow() { ... }) to a consistent form such as "// DEBT: NoRollbackFlow
— reason TBD" to reflect this is the delete cascade check flow (NoRollbackFlow)
rather than an update flow.
core/src/main/java/org/zstack/core/errorcode/GlobalErrorCodeI18nServiceImpl.java
Outdated
Show resolved
Hide resolved
Comment on lines
+11
to
+15
| import javax.xml.bind.annotation.XmlAccessType; | ||
| import javax.xml.bind.annotation.XmlAccessorType; | ||
| import javax.xml.bind.annotation.XmlElement; | ||
| import javax.xml.bind.annotation.XmlRootElement; | ||
| import javax.xml.bind.annotation.XmlType; |
There was a problem hiding this comment.
不应手动修改自动生成的文件
此文件的头部注释（第2-5行）明确标注这是 JAXB 自动生成的代码，并在第4行警告："Any modifications to this file will be lost upon recompilation of the source schema."（重新编译源 schema 时，对此文件的任何修改都将丢失）
手动修改生成的代码违反了标准实践：
- 下次重新生成 schema 时，这些导入修改会被覆盖
- 生成的代码应通过生成器配置来控制，而非手动编辑
- 造成维护负担和代码不一致性
建议的解决方案：
- 在 JAXB 生成配置中指定使用显式导入（如果生成器支持）
- 或将自动生成的文件从本次清理范围中排除
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/src/main/java/org/zstack/core/errorcode/schema/Error.java` around lines
11 - 15, This file is autogenerated (JAXB) and should not be manually edited;
revert any manual changes in Error (class in Error.java) and restore the
original generated header and imports, then either update the JAXB generation
configuration to emit the desired explicit imports (if supported) or remove
Error.java from the manual changes/commit (exclude it from the cleanup) so
future schema regenerations won't overwrite hand edits.
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Set; | ||
| import java.util.UUID; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify that UUID is not used in AccountAPIRequestChecker.java
# Search for UUID usage (excluding the import line itself)
rg -n '\bUUID\b' --type java identity/src/main/java/org/zstack/identity/AccountAPIRequestChecker.java | grep -v '^26:import'Repository: MatheMatrix/zstack
Length of output: 174
移除未使用的 UUID 导入
java.util.UUID 未在此文件中使用，应移除该导入。
建议的修复
import java.util.Map;
import java.util.Set;
-import java.util.UUID;
import static org.zstack.utils.clouderrorcode.CloudOperationsErrorCode.*;📝 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.
Suggested change
| import java.util.UUID; | |
| import java.util.ArrayList; | |
| import java.util.Collection; | |
| import java.util.HashMap; | |
| import java.util.HashSet; | |
| import java.util.List; | |
| import java.util.Map; | |
| import java.util.Set; | |
| import static org.zstack.utils.clouderrorcode.CloudOperationsErrorCode.*; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@identity/src/main/java/org/zstack/identity/AccountAPIRequestChecker.java` at
line 26, 删除未使用的导入：在 AccountAPIRequestChecker 类中移除不再使用的导入语句 "import
java.util.UUID;"，确保不影响其他引用并运行静态检查或编译以验证没有遗漏的依赖项。
- Revert 4 auto-generated files (JAXB/testlink schema) that should not be manually modified - Add exception object to logger.warn in GlobalErrorCodeI18nServiceImpl Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/src/main/java/org/zstack/core/errorcode/GlobalErrorCodeI18nServiceImpl.java (1)
130-155: 在递归遍历错误链时添加循环引用防护。
localizeErrorCode()方法通过递归处理getCause()和ErrorCodeList.getCauses()。根据 ZStack 的设计，ErrorCode的cause字段可以形成链式结构。若在错误构造或包装时意外创建循环引用（如 A → B → A），将导致递归无限进行，最终抛出StackOverflowError。建议添加已访问集合以防止循环：使用
IdentityHashSet追踪已处理的ErrorCode实例，避免重复处理。♻️ 可选：添加循环引用检测
`@Override` public void localizeErrorCode(ErrorCode error, String locale) { + localizeErrorCode(error, locale, Collections.newSetFromMap(new IdentityHashMap<>())); +} + +private void localizeErrorCode(ErrorCode error, String locale, Set<ErrorCode> visited) { if (error == null || locale == null) { return; } + if (!visited.add(error)) { + return; // already processed, avoid infinite loop + } if (error.getGlobalErrorCode() != null) { String message = getLocalizedMessage(error.getGlobalErrorCode(), locale, error.getFormatArgs()); if (message != null) { error.setMessage(message); } } if (error.getCause() != null) { - localizeErrorCode(error.getCause(), locale); + localizeErrorCode(error.getCause(), locale, visited); } if (error instanceof ErrorCodeList) { List<ErrorCode> causes = ((ErrorCodeList) error).getCauses(); if (causes != null) { for (ErrorCode cause : causes) { - localizeErrorCode(cause, locale); + localizeErrorCode(cause, locale, visited); } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/zstack/core/errorcode/GlobalErrorCodeI18nServiceImpl.java` around lines 130 - 155, The method localizeErrorCode(ErrorCode, String) can infinite-recurse if ErrorCode instances form a cycle via getCause() or ErrorCodeList.getCauses(); fix by introducing a visited identity set (e.g., IdentityHashSet or Collections.newSetFromMap(new IdentityHashMap<>())) and an internal helper localizeErrorCode(ErrorCode, String, Set<ErrorCode>) that adds each ErrorCode to visited before recursing and skips processing when an instance is already present; update the public localizeErrorCode(ErrorCode, String) to create the visited set and call the helper, and ensure you check visited before recursing on getCause() and iterating ErrorCodeList.getCauses().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@core/src/main/java/org/zstack/core/errorcode/GlobalErrorCodeI18nServiceImpl.java`:
- Around line 130-155: The method localizeErrorCode(ErrorCode, String) can
infinite-recurse if ErrorCode instances form a cycle via getCause() or
ErrorCodeList.getCauses(); fix by introducing a visited identity set (e.g.,
IdentityHashSet or Collections.newSetFromMap(new IdentityHashMap<>())) and an
internal helper localizeErrorCode(ErrorCode, String, Set<ErrorCode>) that adds
each ErrorCode to visited before recursing and skips processing when an instance
is already present; update the public localizeErrorCode(ErrorCode, String) to
create the visited set and call the helper, and ensure you check visited before
recursing on getCause() and iterating ErrorCodeList.getCauses().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: a21d249e-fc7d-4ffb-be4b-ed07cee19a00
📒 Files selected for processing (1)
core/src/main/java/org/zstack/core/errorcode/GlobalErrorCodeI18nServiceImpl.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
技术基础清理，三类改动：
new NoRollbackFlow()添加// DEBT: NoRollbackFlow — <reason>注释，标记技术债import xxx.*展开为具体类导入（排除 sdk/ 自动生成代码）验证
ievs outere）风险
sync from gitlab !9327