Repository navigation
fix(maa-core): stop-task releases pooled controller & web handler stops blocking the runtime - #434
Open
TsangAsuna wants to merge 2 commits into
Open
TsangAsuna wants to merge 2 commits into
TsangAsuna wants to merge 2 commits into
Conversation
…ps blocking the runtime Two defects in stop-task -> restart-task: 1. Web UI stop deadlocked the runtime: handle_stop_task called the synchronous stop_task_impl directly on an axum worker thread. post_stop needs framework callbacks processed, which the blocked runtime could not do - the UI reported success while the tasker kept running and users had to kill the process from the tray. Wrapped in tokio::task::spawn_blocking. 2. Stale controller reuse crashed the next run: after a game crash the pooled Controller still reported connected(), so the next start reused the dead HWND and screencap failed on every frame, cascading into task and game failure. stop_task_impl now removes the instance entry from controller_pool, unsubscribes, and clears instance.controller, forcing a fresh controller on the next connect.
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
嘿——我发现了 9 个问题
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src-tauri/src/web_server.rs" line_range="1037" />
<code_context>
+ let maa_state = state.maa_state.clone();
+ let instance_id_for_task = instance_id.clone();
+
+ let result = tokio::task::spawn_blocking(move || {
+ stop_task_impl(&app_handle, &maa_state, &instance_id_for_task)
+ })
</code_context>
<issue_to_address>
**Untrusted sites can stop tasks**
When the web server is enabled and a user visits a malicious webpage while a task is running, `stop_task_impl` now runs to completion for requests to the unauthenticated stop route. A malicious webpage can obtain instance IDs from the readable `/api/maa/state` endpoint and POST to stop an active task; the server’s wildcard CORS policy allows the cross-origin request, so the user’s automation stops.
Require authentication and validate the requesting origin or a CSRF token on mutating web API routes.
</issue_to_address>
### Comment 2
<location path="src-tauri/src/commands/maa_core.rs" line_range="1694" />
<code_context>
if running {
tasker.unwrap().post_stop().map_err(|e| e.to_string())?;
}
+
</code_context>
<issue_to_address>
**Stop still waits under lock**
When a running task’s stop waits for a framework callback that needs the instances lock, `stop_task_impl` calls `post_stop` while holding the instances lock, so a callback that `post_stop` waits for blocks trying to acquire that lock and the Web UI stop still hangs despite running on the blocking pool.
Release the instances lock before calling `post_stop`, while preserving synchronization against task submissions and concurrent stop requests.
Also at `src-tauri/src/web_server.rs:1037-1040`.
</issue_to_address>
### Comment 3
<location path="src-tauri/src/commands/maa_core.rs" line_range="1711" />
<code_context>
+ if let Some(subscribers) = instance.controller_subscribers.take() {
+ subscribers.unsubscribe(instance_id);
+ }
+ instance.controller = None;
+
instance.stop_in_progress = true;
</code_context>
<issue_to_address>
**Stop hangs during controller drop**
When native handle destruction waits for a callback that needs the instances mutex, `instance.controller = None` can drop the final controller handle while the instances mutex is held; native destruction waits for the callback while the callback waits for the mutex, leaving the stop call stuck.
Move the controller and removed pool entry out of the locked section and drop them afterward.
</issue_to_address>
### Comment 4
<location path="src-tauri/src/commands/maa_core.rs" line_range="1703-1704" />
<code_context>
+ // 失效 HWND 上失败，进而把任务甚至游戏带崩。强制下一次连接重新
+ // 走 ControllerPool miss → 新建控制器，保证截屏通道是干净的。
+ if let Some(cfg) = instance.controller_config.take() {
+ let mut pool = state.controller_pool.lock().map_err(|e| e.to_string())?;
+ if pool.remove(&cfg).is_some() {
+ info!("ControllerPool: dropped controller after task stop, will recreate on next run");
+ }
</code_context>
<issue_to_address>
**Poisoned pool leaves stop incomplete**
When the controller pool mutex is poisoned, the pool lock’s `?` returns after `post_stop()` has been sent but before `stop_in_progress` is set or `begin_completion` runs, so cancelled queued jobs without terminal callbacks leave run cleanup unscheduled and later submissions stuck.
Make pool eviction best-effort and ensure stop finalization and completion scheduling still occur if that lock cannot be acquired.
</issue_to_address>
### Comment 5
<location path="src-tauri/src/commands/maa_core.rs" line_range="1711" />
<code_context>
+ if let Some(subscribers) = instance.controller_subscribers.take() {
+ subscribers.unsubscribe(instance_id);
+ }
+ instance.controller = None;
+
instance.stop_in_progress = true;
</code_context>
<issue_to_address>
**Stopped connections restore stale controllers**
When a controller connection is in flight while `stop_task_impl` runs, `stop_task_impl` leaves `controller_request` active, so `connect_controller_impl` can pass its current-request checks and restore the instance controller or pool entry after cleanup; a later connection reuses the stale handle.
Cancel the active controller request while stopping and reject any late controller publication.
</issue_to_address>
### Comment 6
<location path="src-tauri/src/commands/maa_core.rs" line_range="1702" />
<code_context>
+ // connected()，下次「停止后重开」会直接复用这个死句柄，截屏在
+ // 失效 HWND 上失败，进而把任务甚至游戏带崩。强制下一次连接重新
+ // 走 ControllerPool miss → 新建控制器，保证截屏通道是干净的。
+ if let Some(cfg) = instance.controller_config.take() {
+ let mut pool = state.controller_pool.lock().map_err(|e| e.to_string())?;
+ if pool.remove(&cfg).is_some() {
</code_context>
<issue_to_address>
**Dead controllers survive idle stops**
When the Tasker is already idle, or `post_stop()` returns an error, `stop_task_impl` exits before the controller cleanup runs, leaving the stale controller and pool entry available for the next connection, which can reuse the dead handle.
Run controller invalidation on every stop path, including idle and `post_stop()` error paths.
Also at `src-tauri/src/commands/maa_core.rs:1704`.
</issue_to_address>
### Comment 7
<location path="src-tauri/src/commands/maa_core.rs" line_range="1704" />
<code_context>
+ // 走 ControllerPool miss → 新建控制器，保证截屏通道是干净的。
+ if let Some(cfg) = instance.controller_config.take() {
+ let mut pool = state.controller_pool.lock().map_err(|e| e.to_string())?;
+ if pool.remove(&cfg).is_some() {
+ info!("ControllerPool: dropped controller after task stop, will recreate on next run");
+ }
</code_context>
<issue_to_address>
**Shared connections split across controllers**
When multiple instances share a controller configuration and one instance stops, `pool.remove(&cfg)` evicts the shared entry while other instances still use its controller; a later connection misses the pool and creates a second controller for the same configuration.
Evict the pooled controller only after coordinating with all instances that share it.
</issue_to_address>
### Comment 8
<location path="src-tauri/src/commands/maa_core.rs" line_range="1711" />
<code_context>
+ if let Some(subscribers) = instance.controller_subscribers.take() {
+ subscribers.unsubscribe(instance_id);
+ }
+ instance.controller = None;
+
instance.stop_in_progress = true;
</code_context>
<issue_to_address>
**Restart keeps stale controller**
When a stopped instance retains an initialized Tasker and is started again, `run_task_impl` reuses an initialized Tasker without calling `bind` again, so it continues using the controller bound before `instance.controller` was cleared; after a stale-handle failure, the restarted task still uses that stale controller.
Discard or rebind the Tasker when stopping clears its controller, so the next run uses the newly connected controller.
</issue_to_address>
### Comment 9
<location path="src-tauri/src/commands/maa_core.rs" line_range="1711" />
<code_context>
+ if let Some(subscribers) = instance.controller_subscribers.take() {
+ subscribers.unsubscribe(instance_id);
+ }
+ instance.controller = None;
+
instance.stop_in_progress = true;
</code_context>
<issue_to_address>
**Restart button becomes unavailable**
When a user stops a task and then tries to restart it from the dashboard, the backend reports the instance disconnected after this clear, and `DashboardView` requires `isConnected` to enable its start button; that path does not reconnect before calling `startTasks`, so users cannot restart there until they reconnect separately.
Reconnect the controller as part of the dashboard start flow, or keep the controller attached unless it actually needs replacement.
</issue_to_address>Original comment in English
Hey - I've found 9 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src-tauri/src/web_server.rs" line_range="1037" />
<code_context>
+ let maa_state = state.maa_state.clone();
+ let instance_id_for_task = instance_id.clone();
+
+ let result = tokio::task::spawn_blocking(move || {
+ stop_task_impl(&app_handle, &maa_state, &instance_id_for_task)
+ })
</code_context>
<issue_to_address>
**Untrusted sites can stop tasks**
When the web server is enabled and a user visits a malicious webpage while a task is running, `stop_task_impl` now runs to completion for requests to the unauthenticated stop route. A malicious webpage can obtain instance IDs from the readable `/api/maa/state` endpoint and POST to stop an active task; the server’s wildcard CORS policy allows the cross-origin request, so the user’s automation stops.
Require authentication and validate the requesting origin or a CSRF token on mutating web API routes.
</issue_to_address>
### Comment 2
<location path="src-tauri/src/commands/maa_core.rs" line_range="1694" />
<code_context>
if running {
tasker.unwrap().post_stop().map_err(|e| e.to_string())?;
}
+
</code_context>
<issue_to_address>
**Stop still waits under lock**
When a running task’s stop waits for a framework callback that needs the instances lock, `stop_task_impl` calls `post_stop` while holding the instances lock, so a callback that `post_stop` waits for blocks trying to acquire that lock and the Web UI stop still hangs despite running on the blocking pool.
Release the instances lock before calling `post_stop`, while preserving synchronization against task submissions and concurrent stop requests.
Also at `src-tauri/src/web_server.rs:1037-1040`.
</issue_to_address>
### Comment 3
<location path="src-tauri/src/commands/maa_core.rs" line_range="1711" />
<code_context>
+ if let Some(subscribers) = instance.controller_subscribers.take() {
+ subscribers.unsubscribe(instance_id);
+ }
+ instance.controller = None;
+
instance.stop_in_progress = true;
</code_context>
<issue_to_address>
**Stop hangs during controller drop**
When native handle destruction waits for a callback that needs the instances mutex, `instance.controller = None` can drop the final controller handle while the instances mutex is held; native destruction waits for the callback while the callback waits for the mutex, leaving the stop call stuck.
Move the controller and removed pool entry out of the locked section and drop them afterward.
</issue_to_address>
### Comment 4
<location path="src-tauri/src/commands/maa_core.rs" line_range="1703-1704" />
<code_context>
+ // 失效 HWND 上失败，进而把任务甚至游戏带崩。强制下一次连接重新
+ // 走 ControllerPool miss → 新建控制器，保证截屏通道是干净的。
+ if let Some(cfg) = instance.controller_config.take() {
+ let mut pool = state.controller_pool.lock().map_err(|e| e.to_string())?;
+ if pool.remove(&cfg).is_some() {
+ info!("ControllerPool: dropped controller after task stop, will recreate on next run");
+ }
</code_context>
<issue_to_address>
**Poisoned pool leaves stop incomplete**
When the controller pool mutex is poisoned, the pool lock’s `?` returns after `post_stop()` has been sent but before `stop_in_progress` is set or `begin_completion` runs, so cancelled queued jobs without terminal callbacks leave run cleanup unscheduled and later submissions stuck.
Make pool eviction best-effort and ensure stop finalization and completion scheduling still occur if that lock cannot be acquired.
</issue_to_address>
### Comment 5
<location path="src-tauri/src/commands/maa_core.rs" line_range="1711" />
<code_context>
+ if let Some(subscribers) = instance.controller_subscribers.take() {
+ subscribers.unsubscribe(instance_id);
+ }
+ instance.controller = None;
+
instance.stop_in_progress = true;
</code_context>
<issue_to_address>
**Stopped connections restore stale controllers**
When a controller connection is in flight while `stop_task_impl` runs, `stop_task_impl` leaves `controller_request` active, so `connect_controller_impl` can pass its current-request checks and restore the instance controller or pool entry after cleanup; a later connection reuses the stale handle.
Cancel the active controller request while stopping and reject any late controller publication.
</issue_to_address>
### Comment 6
<location path="src-tauri/src/commands/maa_core.rs" line_range="1702" />
<code_context>
+ // connected()，下次「停止后重开」会直接复用这个死句柄，截屏在
+ // 失效 HWND 上失败，进而把任务甚至游戏带崩。强制下一次连接重新
+ // 走 ControllerPool miss → 新建控制器，保证截屏通道是干净的。
+ if let Some(cfg) = instance.controller_config.take() {
+ let mut pool = state.controller_pool.lock().map_err(|e| e.to_string())?;
+ if pool.remove(&cfg).is_some() {
</code_context>
<issue_to_address>
**Dead controllers survive idle stops**
When the Tasker is already idle, or `post_stop()` returns an error, `stop_task_impl` exits before the controller cleanup runs, leaving the stale controller and pool entry available for the next connection, which can reuse the dead handle.
Run controller invalidation on every stop path, including idle and `post_stop()` error paths.
Also at `src-tauri/src/commands/maa_core.rs:1704`.
</issue_to_address>
### Comment 7
<location path="src-tauri/src/commands/maa_core.rs" line_range="1704" />
<code_context>
+ // 走 ControllerPool miss → 新建控制器，保证截屏通道是干净的。
+ if let Some(cfg) = instance.controller_config.take() {
+ let mut pool = state.controller_pool.lock().map_err(|e| e.to_string())?;
+ if pool.remove(&cfg).is_some() {
+ info!("ControllerPool: dropped controller after task stop, will recreate on next run");
+ }
</code_context>
<issue_to_address>
**Shared connections split across controllers**
When multiple instances share a controller configuration and one instance stops, `pool.remove(&cfg)` evicts the shared entry while other instances still use its controller; a later connection misses the pool and creates a second controller for the same configuration.
Evict the pooled controller only after coordinating with all instances that share it.
</issue_to_address>
### Comment 8
<location path="src-tauri/src/commands/maa_core.rs" line_range="1711" />
<code_context>
+ if let Some(subscribers) = instance.controller_subscribers.take() {
+ subscribers.unsubscribe(instance_id);
+ }
+ instance.controller = None;
+
instance.stop_in_progress = true;
</code_context>
<issue_to_address>
**Restart keeps stale controller**
When a stopped instance retains an initialized Tasker and is started again, `run_task_impl` reuses an initialized Tasker without calling `bind` again, so it continues using the controller bound before `instance.controller` was cleared; after a stale-handle failure, the restarted task still uses that stale controller.
Discard or rebind the Tasker when stopping clears its controller, so the next run uses the newly connected controller.
</issue_to_address>
### Comment 9
<location path="src-tauri/src/commands/maa_core.rs" line_range="1711" />
<code_context>
+ if let Some(subscribers) = instance.controller_subscribers.take() {
+ subscribers.unsubscribe(instance_id);
+ }
+ instance.controller = None;
+
instance.stop_in_progress = true;
</code_context>
<issue_to_address>
**Restart button becomes unavailable**
When a user stops a task and then tries to restart it from the dashboard, the backend reports the instance disconnected after this clear, and `DashboardView` requires `isConnected` to enable its start button; that path does not reconnect before calling `startTasks`, so users cannot restart there until they reconnect separately.
Reconnect the controller as part of the dashboard start flow, or keep the controller attached unless it actually needs replacement.
</issue_to_address>| let maa_state = state.maa_state.clone(); | ||
| let instance_id_for_task = instance_id.clone(); | ||
|
|
||
| let result = tokio::task::spawn_blocking(move || { |
Contributor
There was a problem hiding this comment.
🟡 中 · 不受信任的网站可以停止任务
启用 Web 服务器后，如果用户在任务运行期间访问恶意网页，针对未认证的停止路由发起的请求现在会让 stop_task_impl 执行完成。恶意网页可以从可读取的 /api/maa/state 端点获取实例 ID，然后发送 POST 请求停止正在运行的任务；服务器的通配符 CORS 策略允许跨源请求，因此用户的自动化任务会停止。
对会修改数据的 Web API 路由要求身份验证，并验证请求来源或 CSRF 令牌。
给 AI 智能体的提示
在 `src-tauri/src/web_server.rs` 第 1037 行：
**不受信任的网站可以停止任务**
启用 Web 服务器后，如果用户在任务运行期间访问恶意网页，针对未认证的停止路由发起的请求现在会让 `stop_task_impl` 执行完成。恶意网页可以从可读取的 `/api/maa/state` 端点获取实例 ID，然后发送 POST 请求停止正在运行的任务；服务器的通配符 CORS 策略允许跨源请求，因此用户的自动化任务会停止。
对会修改数据的 Web API 路由要求身份验证，并验证请求来源或 CSRF 令牌。Original comment in English
🟡 Medium · Untrusted sites can stop tasks
When the web server is enabled and a user visits a malicious webpage while a task is running, stop_task_impl now runs to completion for requests to the unauthenticated stop route. A malicious webpage can obtain instance IDs from the readable /api/maa/state endpoint and POST to stop an active task; the server’s wildcard CORS policy allows the cross-origin request, so the user’s automation stops.
Require authentication and validate the requesting origin or a CSRF token on mutating web API routes.
Prompt for AI agents
In `src-tauri/src/web_server.rs` at line 1037:
**Untrusted sites can stop tasks**
When the web server is enabled and a user visits a malicious webpage while a task is running, `stop_task_impl` now runs to completion for requests to the unauthenticated stop route. A malicious webpage can obtain instance IDs from the readable `/api/maa/state` endpoint and POST to stop an active task; the server’s wildcard CORS policy allows the cross-origin request, so the user’s automation stops.
Require authentication and validate the requesting origin or a CSRF token on mutating web API routes.| @@ -1693,6 +1693,23 @@ pub fn stop_task_impl( | |||
| if running { | |||
| tasker.unwrap().post_stop().map_err(|e| e.to_string())?; | |||
Contributor
There was a problem hiding this comment.
🟠 高 · 停止操作仍会在锁内等待
如果正在运行的任务在停止时等待一个需要获取实例锁的框架回调，stop_task_impl 会在持有实例锁的情况下调用 post_stop，因此 post_stop 等待的回调会因尝试获取该锁而阻塞；尽管停止操作运行在阻塞线程池中，Web UI 的停止操作仍会卡住。
在调用 post_stop 前释放实例锁，同时确保与任务提交和并发停止请求之间仍保持同步。
另见 src-tauri/src/web_server.rs:1037-1040。
给 AI 智能体的提示
在 `src-tauri/src/commands/maa_core.rs` 第 1694 行：
**停止操作仍会在锁内等待**
如果正在运行的任务在停止时等待一个需要获取实例锁的框架回调，`stop_task_impl` 会在持有实例锁的情况下调用 `post_stop`，因此 `post_stop` 等待的回调会因尝试获取该锁而阻塞；尽管停止操作运行在阻塞线程池中，Web UI 的停止操作仍会卡住。
在调用 `post_stop` 前释放实例锁，同时确保与任务提交和并发停止请求之间仍保持同步。
另见 `src-tauri/src/web_server.rs:1037-1040`。Original comment in English
🟠 High · Stop still waits under lock
When a running task’s stop waits for a framework callback that needs the instances lock, stop_task_impl calls post_stop while holding the instances lock, so a callback that post_stop waits for blocks trying to acquire that lock and the Web UI stop still hangs despite running on the blocking pool.
Release the instances lock before calling post_stop, while preserving synchronization against task submissions and concurrent stop requests.
Also at src-tauri/src/web_server.rs:1037-1040.
Prompt for AI agents
In `src-tauri/src/commands/maa_core.rs` at line 1694:
**Stop still waits under lock**
When a running task’s stop waits for a framework callback that needs the instances lock, `stop_task_impl` calls `post_stop` while holding the instances lock, so a callback that `post_stop` waits for blocks trying to acquire that lock and the Web UI stop still hangs despite running on the blocking pool.
Release the instances lock before calling `post_stop`, while preserving synchronization against task submissions and concurrent stop requests.
Also at `src-tauri/src/web_server.rs:1037-1040`.| if let Some(subscribers) = instance.controller_subscribers.take() { | ||
| subscribers.unsubscribe(instance_id); | ||
| } | ||
| instance.controller = None; |
Contributor
There was a problem hiding this comment.
🟠 高 · 重启后仍使用过期控制器
如果已停止的实例保留着已初始化的 Tasker 并再次启动，run_task_impl 会复用已初始化的 Tasker，而不会再次调用 bind，因此它会继续使用在 instance.controller 被清除之前绑定的控制器；发生过期句柄故障后，重启的任务仍会使用该过期控制器。
停止并清除控制器时，丢弃或重新绑定 Tasker，以便下一次运行使用新连接的控制器。
给 AI 智能体的提示
在 `src-tauri/src/commands/maa_core.rs` 第 1711 行：
**重启后仍使用过期控制器**
如果已停止的实例保留着已初始化的 Tasker 并再次启动，`run_task_impl` 会复用已初始化的 Tasker，而不会再次调用 `bind`，因此它会继续使用在 `instance.controller` 被清除之前绑定的控制器；发生过期句柄故障后，重启的任务仍会使用该过期控制器。
停止并清除控制器时，丢弃或重新绑定 Tasker，以便下一次运行使用新连接的控制器。Original comment in English
🟠 High · Restart keeps stale controller
When a stopped instance retains an initialized Tasker and is started again, run_task_impl reuses an initialized Tasker without calling bind again, so it continues using the controller bound before instance.controller was cleared; after a stale-handle failure, the restarted task still uses that stale controller.
Discard or rebind the Tasker when stopping clears its controller, so the next run uses the newly connected controller.
Prompt for AI agents
In `src-tauri/src/commands/maa_core.rs` at line 1711:
**Restart keeps stale controller**
When a stopped instance retains an initialized Tasker and is started again, `run_task_impl` reuses an initialized Tasker without calling `bind` again, so it continues using the controller bound before `instance.controller` was cleared; after a stale-handle failure, the restarted task still uses that stale controller.
Discard or rebind the Tasker when stopping clears its controller, so the next run uses the newly connected controller.| if let Some(subscribers) = instance.controller_subscribers.take() { | ||
| subscribers.unsubscribe(instance_id); | ||
| } | ||
| instance.controller = None; |
Contributor
There was a problem hiding this comment.
🟡 中 · 重启按钮会变得不可用
用户停止任务后尝试从仪表板重新启动时，后端会在清除控制器后报告实例已断开连接，而 DashboardView 要求 isConnected 为真才会启用启动按钮；该流程在调用 startTasks 前不会重新连接，因此用户必须另行重新连接才能在此处重启。
在仪表板启动流程中重新连接控制器，或者除非确实需要替换，否则不要解除控制器的关联。
给 AI 智能体的提示
在 `src-tauri/src/commands/maa_core.rs` 第 1711 行：
**重启按钮会变得不可用**
用户停止任务后尝试从仪表板重新启动时，后端会在清除控制器后报告实例已断开连接，而 `DashboardView` 要求 `isConnected` 为真才会启用启动按钮；该流程在调用 `startTasks` 前不会重新连接，因此用户必须另行重新连接才能在此处重启。
在仪表板启动流程中重新连接控制器，或者除非确实需要替换，否则不要解除控制器的关联。Original comment in English
🟡 Medium · Restart button becomes unavailable
When a user stops a task and then tries to restart it from the dashboard, the backend reports the instance disconnected after this clear, and DashboardView requires isConnected to enable its start button; that path does not reconnect before calling startTasks, so users cannot restart there until they reconnect separately.
Reconnect the controller as part of the dashboard start flow, or keep the controller attached unless it actually needs replacement.
Prompt for AI agents
In `src-tauri/src/commands/maa_core.rs` at line 1711:
**Restart button becomes unavailable**
When a user stops a task and then tries to restart it from the dashboard, the backend reports the instance disconnected after this clear, and `DashboardView` requires `isConnected` to enable its start button; that path does not reconnect before calling `startTasks`, so users cannot restart there until they reconnect separately.
Reconnect the controller as part of the dashboard start flow, or keep the controller attached unless it actually needs replacement.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62bc9c12f9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if let Some(subscribers) = instance.controller_subscribers.take() { | ||
| subscribers.unsubscribe(instance_id); | ||
| } | ||
| instance.controller = None; |
There was a problem hiding this comment.
When a screenshot subscription is active (for example the Dashboard/ScreenshotPanel live view), clearing the controller makes screenshot_service::capture_loop take its should_exit branch; because subscribers are still present, src-tauri/src/screenshot_service.rs:214-221 immediately respawns the loop with no sleep. The replacement task sees the same None and repeats until reconnect or subscriber expiry, producing a tight task-spawn loop that can peg the Tokio runtime just as stop runs; quiesce the stream or make the no-controller path wait rather than only dropping the controller.
Useful? React with 👍 / 👎.
| if let Some(subscribers) = instance.controller_subscribers.take() { | ||
| subscribers.unsubscribe(instance_id); | ||
| } | ||
| instance.controller = None; |
There was a problem hiding this comment.
After any normal stop from Dashboard, the task-stopped refresh records this instance as Disconnected; src/components/DashboardView.tsx:121,764-767 only renders/enables task control while connected, and its start path at line 379 calls startTasks without the Toolbar's reconnect logic. The control therefore disappears while a stop is still pending and remains unavailable after completion, so users cannot retry or restart without leaving Dashboard and reconnecting; either preserve a healthy controller or teach this flow to reconnect.
Useful? React with 👍 / 👎.
|
|
||
| let outcome = match result { | ||
| Ok(inner) => inner, | ||
| Err(join_err) => Err(format!("停止任务线程异常: {join_err}")), |
There was a problem hiding this comment.
When the blocking worker fails (for example by panicking), /tasks/stop returns this Chinese sentence as the API error, and apiPost surfaces the response text verbatim, so the message cannot follow the user's locale. Return a stable error code and translate it through src/i18n/locales instead.
AGENTS.md reference: AGENTS.md:L40-L41
Useful? React with 👍 / 👎.
…pool lock safely Addresses review findings on the stop-task patch: 1. Idle Tasker path skipped invalidation entirely. That is exactly the post-crash case: the game dies, the Tasker goes idle on its own, the user hits stop, and the old code returned Ok(()) before touching controller_config / controller / pool. The next run then reused the dead handle. Now that path invalidates too. 2. pool.remove ran unconditionally, unlike destroy_instance_impl and update_instance_controller which both skip removal when another instance still shares the same controller config. Without the check, stopping one instance split a shared device into two controllers competing for screencap. 3. The pool lock used , so a poisoned mutex aborted the stop path after post_stop but before stop_in_progress / begin_completion - leaving cleanup unscheduled. Removal is now best effort and logs a warning instead. 4. instance.controller was set to None while the instances lock was held; native controller destruction can block on callbacks that take that same lock. The handle is now taken inside the lock and dropped after it is released (same rule already documented at the destroy_instance_impl and update_instance_controller call sites). 5. controller_request is cancelled on stop, matching destroy_instance_impl, so a connect already in flight cannot publish a stale handle afterwards.
Author
验证方式本次补丁只动
回滚
|
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.
Two defects in "stop task" → "restart task"
Web UI stop was a no-op / deadlock:
handle_stop_taskcalled the synchronousstop_task_impldirectly on an axum worker thread.post_stopneeds framework callbacksto be processed, which the blocked runtime could not do — the UI reported success while
the tasker kept running, and users had to kill the process from the tray.
Fixed: wrap in
tokio::task::spawn_blocking.Stale controller reuse crashed the next run: after a game crash the pooled Controller
still reported
connected(), so the next start reused the dead HWND and screencap failedon every frame, cascading into task/game failure. Fixed:
stop_task_implnow removes theinstance's entry from
controller_pool, unsubscribes, and clearsinstance.controller,forcing a fresh controller on the next connect.
Rollback
Revert this commit (two files, no cross-PR state).
Test environment
Sourcery 摘要
通过在异步运行时之外执行阻塞的停止操作，并重置池化控制器状态以供下次运行，从而确保任务能够可靠停止。
Bug 修复：
Original summary in English
Sourcery 摘要
将阻塞式 Web 停止操作移出异步运行时，并重置池化控制器状态，以确保任务能够可靠停止，同时为下一次运行做好准备。
Bug 修复：
增强功能：
Original summary in English
Summary by Sourcery
Make task stopping reliable by moving blocking web stops off the async runtime and resetting pooled controller state for the next run.
Bug Fixes:
Enhancements: