Merged
Conversation
There was a problem hiding this comment.
我是 MonkeyCode AI 编程助手,你可以在 GitHub 仓库的 PR 中 at @MonkeyCode-AI 来呼唤我。
任务执行细节请参考: https://monkeycode-ai.com/tasks/public?id=51c23f00-0de7-418f-ade3-572fa8d85a34
代码审查结果
实现较完整但存在关键健壮性缺陷,建议修复错误处理与参数校验后再合并。
✨ 代码亮点
- 将 VM 空闲治理拆分为休眠/通知/回收三段延迟队列,并加了 Redis 防抖键避免高频重复入队。
- Loki Tail 从轮询切到 WebSocket,包含历史补齐、断线重连、心跳与去重,实时性和完整性明显提升。
- 回收流程新增任务状态收敛与通知事件构建,补齐了 VM 生命周期结束后的业务闭环。
| 🚨 Critical | 💡 Suggestion | |
|---|---|---|
| 0 | 2 | 0 |
Comment on lines
+935
to
+937
| if err != nil || info == nil || info.Status != taskflow.VirtualMachineStatusSleeping { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Warning
resumeVMIfSleeping 在查询 VM 状态失败时直接返回 nil,会把真实错误(网络异常、鉴权失败、taskflow 不可用)静默吞掉,导致后续流程继续执行并在更深层失败,增加排障难度,也可能让休眠 VM 未被恢复却继续执行端口/终端操作。
建议: 仅在“查询成功且非 sleeping”时返回 nil;查询失败应向上返回错误。
Suggested change
| if err != nil || info == nil || info.Status != taskflow.VirtualMachineStatusSleeping { | |
| return nil | |
| } | |
| if err != nil { | |
| return fmt.Errorf("query vm info for resume: %w", err) | |
| } | |
| if info == nil || info.Status != taskflow.VirtualMachineStatusSleeping { | |
| return nil | |
| } |
Comment on lines
+433
to
+440
| go func() { | ||
| ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) | ||
| defer cancel() | ||
| if err := h.hostUsecase.RefreshIdleTimers(ctx, req.VMID); err != nil { | ||
| h.logger.With("error", err).ErrorContext(ctx, "failed to refresh idel timers") | ||
| } | ||
| }() | ||
| return c.Success(nil) |
There was a problem hiding this comment.
Warning
VMActivity 当前无任何参数校验,req.VMID 为空时仍异步执行并立即返回 c.Success(nil)。这会让调用方误以为活动上报成功,实际后台只会记录错误日志,造成状态不一致和问题隐蔽。
建议: 至少在入口校验 vm_id 非空,对无效请求返回错误;再执行异步刷新逻辑。
Suggested change
| go func() { | |
| ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) | |
| defer cancel() | |
| if err := h.hostUsecase.RefreshIdleTimers(ctx, req.VMID); err != nil { | |
| h.logger.With("error", err).ErrorContext(ctx, "failed to refresh idel timers") | |
| } | |
| }() | |
| return c.Success(nil) | |
| if req.VMID == "" { | |
| return fmt.Errorf("vm_id is required") | |
| } | |
| go func() { | |
| ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) | |
| defer cancel() | |
| if err := h.hostUsecase.RefreshIdleTimers(ctx, req.VMID); err != nil { | |
| h.logger.With("error", err).ErrorContext(ctx, "failed to refresh idel timers") | |
| } | |
| }() | |
| return c.Success(nil) |
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
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.
No description provided.