fix(system): 修复队列完成后电源操作因先请求前端关闭而永远无法执行的问题 - #612
Open
1w1w11w1 wants to merge 1 commit into
Open
Conversation
审查者指南修复队列完成后的系统电源操作流程：关机、强制关机、重启、休眠、睡眠和注销现在直接执行，不再等待前端退出；仅 KillSelf 保留原有的前端关闭流程，并通过回归测试锁定两类行为边界。 队列完成后直接执行系统电源操作的时序图sequenceDiagram
participant Queue as QueueCompletion
participant System as SystemService
participant Power as PowerExecutor
participant Frontend as Frontend
Queue->>System: set_power(mode)
System->>Power: execute(mode)
Power-->>System: power action started
Note over Frontend,System: Frontend close is not requested
文件级变更
可能关联的问题
提示和命令与 Sourcery 交互
自定义你的使用体验访问你的控制面板以：
获取帮助Original review guide in EnglishReviewer's Guide修复队列完成后的系统电源操作流程：关机、强制关机、重启、休眠、睡眠和注销现在直接执行，不再等待前端退出；仅 KillSelf 保留原有的前端关闭流程，并通过回归测试锁定两类行为边界。 Sequence diagram for direct system power execution after queue completionsequenceDiagram
participant Queue as QueueCompletion
participant System as SystemService
participant Power as PowerExecutor
participant Frontend as Frontend
Queue->>System: set_power(mode)
System->>Power: execute(mode)
Power-->>System: power action started
Note over Frontend,System: Frontend close is not requested
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
嘿——我发现了 1 个问题
给 AI 代理的提示
请处理此次代码审查中的评论：
## 单独评论
### 评论 1
<location path="tests/services/test_set_power_execution.py" line_range="14-18" />
<code_context>
+ "app.services.system.power.execute", new_callable=AsyncMock
+ ) as execute,
+ ):
+ await System.set_power("ShutdownForce")
+
+ execute.assert_awaited_once_with("ShutdownForce")
</code_context>
<issue_to_address>
**issue (testing)：** 回归测试在未模拟 `kill_emulator_processes` 的情况下调用 `System.set_power("ShutdownForce")`，因此会扫描主机进程表，并终止名称包含模拟器相关关键词的所有进程。在存在匹配的模拟器进程的机器上运行该测试，可能会终止该进程，使测试具有破坏性或不稳定性。
**触发条件：** 当测试在开发者机器或 CI 机器上运行，且其中存在名称包含 `Nemu`、`emulator` 或 `MuMu` 的进程时。
**建议修复：** 在调用 `set_power` 之前，在测试中使用 `AsyncMock` 对 `System.kill_emulator_processes` 进行 patch。
```suggestion
patch.object(
System, "kill_emulator_processes", new_callable=AsyncMock
),
patch(
"app.services.system.power.execute", new_callable=AsyncMock
) as execute,
):
await System.set_power("ShutdownForce")
```
</issue_to_address>Sourcery 评估
需要人工审查。 首先需要处理 1 个发现的问题；如果执行顺序的变更不正确，排队中的关机、重启或注销操作可能会在不应执行时执行，从而立即导致服务中断或用户会话丢失，而这类后果无法通过回滚撤销。受影响的操作范围有限，未来的行为可以通过回滚恢复，但任何已经触发的电源操作都已生效。
阻塞性发现：tests/services/test_set_power_execution.py:18
Original comment in English
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/services/test_set_power_execution.py" line_range="14-18" />
<code_context>
+ "app.services.system.power.execute", new_callable=AsyncMock
+ ) as execute,
+ ):
+ await System.set_power("ShutdownForce")
+
+ execute.assert_awaited_once_with("ShutdownForce")
</code_context>
<issue_to_address>
**issue (testing):** The regression test calls `System.set_power("ShutdownForce")` without mocking `kill_emulator_processes`, so it scans the host process table and kills every process whose name contains emulator-related keywords. Running the test on a machine with a matching emulator process can terminate that process and make the test destructive or flaky.
**Triggers:** When the test runs on a developer or CI machine with a process whose name contains `Nemu`, `emulator`, or `MuMu`.
**Suggested fix:** Patch `System.kill_emulator_processes` with an `AsyncMock` in the test before invoking `set_power`.
```suggestion
patch.object(
System, "kill_emulator_processes", new_callable=AsyncMock
),
patch(
"app.services.system.power.execute", new_callable=AsyncMock
) as execute,
):
await System.set_power("ShutdownForce")
```
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and if the sequencing change is wrong, a queued Shutdown, Reboot, or Logoff can execute when it should not, causing an immediate outage or user session loss that reverting cannot undo. The affected operation is bounded and future behavior can be restored by reverting, but any power action already triggered has already taken effect.
Blocking findings: tests/services/test_set_power_execution.py:18
Comment on lines
+14
to
+18
| patch( | ||
| "app.services.system.power.execute", new_callable=AsyncMock | ||
| ) as execute, | ||
| ): | ||
| await System.set_power("ShutdownForce") |
There was a problem hiding this comment.
issue (testing)： 回归测试在未模拟 kill_emulator_processes 的情况下调用 System.set_power("ShutdownForce")，因此会扫描主机进程表，并终止名称包含模拟器相关关键词的所有进程。在存在匹配的模拟器进程的机器上运行该测试，可能会终止该进程，使测试具有破坏性或不稳定性。
触发条件： 当测试在开发者机器或 CI 机器上运行，且其中存在名称包含 Nemu、emulator 或 MuMu 的进程时。
建议修复： 在调用 set_power 之前，在测试中使用 AsyncMock 对 System.kill_emulator_processes 进行 patch。
Suggested change
| patch( | |
| "app.services.system.power.execute", new_callable=AsyncMock | |
| ) as execute, | |
| ): | |
| await System.set_power("ShutdownForce") | |
| patch.object( | |
| System, "kill_emulator_processes", new_callable=AsyncMock | |
| ), | |
| patch( | |
| "app.services.system.power.execute", new_callable=AsyncMock | |
| ) as execute, | |
| ): | |
| await System.set_power("ShutdownForce") |
Original comment in English
issue (testing): The regression test calls System.set_power("ShutdownForce") without mocking kill_emulator_processes, so it scans the host process table and kills every process whose name contains emulator-related keywords. Running the test on a machine with a matching emulator process can terminate that process and make the test destructive or flaky.
Triggers: When the test runs on a developer or CI machine with a process whose name contains Nemu, emulator, or MuMu.
Suggested fix: Patch System.kill_emulator_processes with an AsyncMock in the test before invoking set_power.
Suggested change
| patch( | |
| "app.services.system.power.execute", new_callable=AsyncMock | |
| ) as execute, | |
| ): | |
| await System.set_power("ShutdownForce") | |
| patch.object( | |
| System, "kill_emulator_processes", new_callable=AsyncMock | |
| ), | |
| patch( | |
| "app.services.system.power.execute", new_callable=AsyncMock | |
| ) as execute, | |
| ): | |
| await System.set_power("ShutdownForce") |
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
1 participant
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.
Closes #611
摘要
KillSelf退出路径保留先请求前端关闭的既有行为，该 helper 现仅服务此场景验证
python -m pytest tests/services/test_power_countdown.py tests/services/test_set_power_execution.py -q：5 passedpython -m pytest tests --collect-only -q：626 collected，exit 0ruff check/ruff format --check：通过