fix: Guard MaaCore runtime lifecycle state#549
Merged
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #549 +/- ##
==========================================
+ Coverage 71.86% 72.37% +0.50%
==========================================
Files 69 72 +3
Lines 6412 6620 +208
Branches 6412 6620 +208
==========================================
+ Hits 4608 4791 +183
- Misses 1477 1491 +14
- Partials 327 338 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
wangl-cc
changed the title
wangl-cc
changed the title
wangl-cc
marked this pull request as ready for review
May 24, 2026 20:25
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并留下了一些总体性的反馈：
- 在
Assistant::load/unload中，你在maa_sys::binding::load/unloadFFI 调用之前/之后显式地释放了生命周期互斥锁，这会导致在运行时处于加载/卸载过程中的同时可以并发创建新的 assistant；建议在整个操作期间都持有该锁（或者在文档中说明这种竞争条件为何是安全的），从而将运行时状态变更与 assistant 的创建/销毁串行化。 - 在
Assistant::new、new_with_callback和Drop中，生命周期互斥锁会在 FFI 调用期间一直保持，这在这些 FFI 调用如果同步触发回调且这些回调访问Assistant::load/unload时，可能导致死锁或异常行为；将加锁范围缩小到仅包裹计数器修改，或改用原子计数器，可以避免这种耦合。
面向 AI 智能体的提示词
Please address the comments from this code review:
## Overall Comments
- In `Assistant::load`/`unload`, you explicitly drop the lifecycle mutex before/after the `maa_sys::binding::load`/`unload` FFI calls, which allows new assistants to be created concurrently while the runtime is mid-load/unload; consider holding the lock for the entire operation (or documenting why that race is safe) so that runtime mutation and assistant creation/destruction are serialized.
- The lifecycle mutex is held across FFI calls in `Assistant::new`, `new_with_callback`, and `Drop`, which could deadlock or misbehave if those FFI calls ever invoke callbacks synchronously that touch `Assistant::load`/`unload`; minimizing the lock scope around just the counter mutation or switching to an atomic counter would avoid this coupling.
## Individual Comments
### Comment 1
<location path="crates/maa-core/src/lib.rs" line_range="96-99" />
<code_context>
+ /// Fails if any assistant instances are alive.
pub fn load(path: impl AsRef<std::path::Path>) -> Result<()> {
let path = path.as_ref();
+ let lifecycle = RUNTIME_LIFECYCLE
+ .lock()
+ .expect("Failed to lock MaaCore runtime lifecycle");
+ lifecycle.ensure_no_active_assistants()?;
#[cfg(target_os = "windows")]
</code_context>
<issue_to_address>
**issue (bug_risk):** Uses of `RUNTIME_LIFECYCLE` and `Error::ActiveAssistants` need cfg-gating to compile when the `runtime` feature is disabled.
Both `RUNTIME_LIFECYCLE` and `Error::ActiveAssistants` are currently `#[cfg(feature = "runtime")]`, so these references in `load()`/`unload()` will not compile when `runtime` is disabled.
Options:
- Make `RUNTIME_LIFECYCLE` and `Error::ActiveAssistants` available unconditionally, or
- Guard the `load`/`unload` logic that uses them with `#[cfg(feature = "runtime")]` and provide a fallback for non-runtime builds.
As written, the crate fails to build without the `runtime` feature enabled.
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据这些反馈改进后续的审查建议。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- In
Assistant::load/unload, you explicitly drop the lifecycle mutex before/after themaa_sys::binding::load/unloadFFI calls, which allows new assistants to be created concurrently while the runtime is mid-load/unload; consider holding the lock for the entire operation (or documenting why that race is safe) so that runtime mutation and assistant creation/destruction are serialized. - The lifecycle mutex is held across FFI calls in
Assistant::new,new_with_callback, andDrop, which could deadlock or misbehave if those FFI calls ever invoke callbacks synchronously that touchAssistant::load/unload; minimizing the lock scope around just the counter mutation or switching to an atomic counter would avoid this coupling.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Assistant::load`/`unload`, you explicitly drop the lifecycle mutex before/after the `maa_sys::binding::load`/`unload` FFI calls, which allows new assistants to be created concurrently while the runtime is mid-load/unload; consider holding the lock for the entire operation (or documenting why that race is safe) so that runtime mutation and assistant creation/destruction are serialized.
- The lifecycle mutex is held across FFI calls in `Assistant::new`, `new_with_callback`, and `Drop`, which could deadlock or misbehave if those FFI calls ever invoke callbacks synchronously that touch `Assistant::load`/`unload`; minimizing the lock scope around just the counter mutation or switching to an atomic counter would avoid this coupling.
## Individual Comments
### Comment 1
<location path="crates/maa-core/src/lib.rs" line_range="96-99" />
<code_context>
+ /// Fails if any assistant instances are alive.
pub fn load(path: impl AsRef<std::path::Path>) -> Result<()> {
let path = path.as_ref();
+ let lifecycle = RUNTIME_LIFECYCLE
+ .lock()
+ .expect("Failed to lock MaaCore runtime lifecycle");
+ lifecycle.ensure_no_active_assistants()?;
#[cfg(target_os = "windows")]
</code_context>
<issue_to_address>
**issue (bug_risk):** Uses of `RUNTIME_LIFECYCLE` and `Error::ActiveAssistants` need cfg-gating to compile when the `runtime` feature is disabled.
Both `RUNTIME_LIFECYCLE` and `Error::ActiveAssistants` are currently `#[cfg(feature = "runtime")]`, so these references in `load()`/`unload()` will not compile when `runtime` is disabled.
Options:
- Make `RUNTIME_LIFECYCLE` and `Error::ActiveAssistants` available unconditionally, or
- Guard the `load`/`unload` logic that uses them with `#[cfg(feature = "runtime")]` and provide a fallback for non-runtime builds.
As written, the crate fails to build without the `runtime` feature enabled.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the maa-core runtime-loading wrapper by tracking live Assistant instances and preventing MaaCore runtime load()/unload() while any assistants are still alive, avoiding use-after-unload across FFI calls (including Drop).
Changes:
- Add a global runtime lifecycle tracker (
active_assistants) gated behind theruntimefeature. - Reject
Assistant::load()andAssistant::unload()when there are live assistants. - Add a unit test verifying load/unload are rejected while an assistant is considered active.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
crates/maa-core/src/lib.rs |
Introduces lifecycle tracking, increments/decrements on Assistant create/drop, and guards runtime load/unload; adds regression test. |
crates/maa-core/src/error.rs |
Adds a new runtime-only error variant for “active assistants” lifecycle violations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
223
to
228
| #[cfg(feature = "runtime")] | ||
| let mut lifecycle = RUNTIME_LIFECYCLE | ||
| .lock() | ||
| .expect("Failed to lock MaaCore runtime lifecycle"); | ||
|
|
||
| let handle = unsafe { maa_sys::binding::AsstCreate() }; |
| #[error("The content returned by MaaCore is too large (length > {0})")] | ||
| ContentTooLarge(usize), | ||
| #[cfg(feature = "runtime")] | ||
| #[error("Cannot change MaaCore while Assistant instances are alive")] |
wangl-cc
changed the title
wangl-cc
force-pushed
the
codex/maa-core-runtime-lifecycle
branch
from
May 24, 2026 22:16
926becd to
9b6b052
Compare
wangl-cc
changed the title
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - 我在这里先给出一些整体性的反馈：
- 在
PendingAssistantRegistration::begin中，当达到 assistant 数量上限时，目前通过expect("Assistant count overflowed")触发 panic；建议改为返回一个专门的错误，这样在极端但仍然有效的高负载情况下，调用方就不会在Assistant::new*中遇到意料之外的 panic。 unregister_assistant直接调用fetch_sub(1)，只依赖debug_assert!做防护；可能更安全的做法是在自减前显式检查 assistant 计数是否为 0（并进行日志记录/中止），这样即使 API 被误用，也不会在 release 构建中因为下溢而悄悄破坏运行时状态。- 在你维护了独立的
RUNTIME_STATE的同时，Assistant::loaded()仍然转调到maa_sys::binding::loaded()；建议考虑使用该原子状态（或者至少在文档中说明它们之间的关系），以便调用方在使用新的锁定/状态切换语义时能获得一致的视图。
面向 AI 代理的提示
请根据以下代码审查意见进行修改：
## 整体意见
- 在 `PendingAssistantRegistration::begin` 中，当达到 assistant 数量上限时，目前通过 `expect("Assistant count overflowed")` 触发 panic；建议改为返回一个专门的错误，这样在极端但仍然有效的高负载情况下，调用方就不会在 `Assistant::new*` 中遇到意料之外的 panic。
- `unregister_assistant` 直接调用 `fetch_sub(1)`，只依赖 `debug_assert!` 做防护；可能更安全的做法是在自减前显式检查 assistant 计数是否为 0（并进行日志记录/中止），这样即使 API 被误用，也不会在 release 构建中因为下溢而悄悄破坏运行时状态。
- 在你维护了独立的 `RUNTIME_STATE` 的同时，`Assistant::loaded()` 仍然转调到 `maa_sys::binding::loaded()`；建议考虑使用该原子状态（或者至少在文档中说明它们之间的关系），以便调用方在使用新的锁定/状态切换语义时能获得一致的视图。帮我变得更有用！请对每条评论点击 👍 或 👎，我会根据你的反馈改进后续的审查。
Original comment in English
Hey - I've left some high level feedback:
- In
PendingAssistantRegistration::begin, hitting the assistant-count limit currently panics viaexpect("Assistant count overflowed"); consider returning a dedicated error instead so callers don’t see an unexpected panic fromAssistant::new*under extreme but valid load. unregister_assistantblindly doesfetch_sub(1)and only guards withdebug_assert!; it may be safer to explicitly check for a zero assistant count (and log/abort) before decrementing so underflow can’t silently corrupt the runtime state in release builds if the API is misused.Assistant::loaded()still delegates tomaa_sys::binding::loaded()while you maintain a separateRUNTIME_STATE; consider using the atomic state (or at least documenting the relationship) so callers get a view that’s consistent with the new locking/transition semantics.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `PendingAssistantRegistration::begin`, hitting the assistant-count limit currently panics via `expect("Assistant count overflowed")`; consider returning a dedicated error instead so callers don’t see an unexpected panic from `Assistant::new*` under extreme but valid load.
- `unregister_assistant` blindly does `fetch_sub(1)` and only guards with `debug_assert!`; it may be safer to explicitly check for a zero assistant count (and log/abort) before decrementing so underflow can’t silently corrupt the runtime state in release builds if the API is misused.
- `Assistant::loaded()` still delegates to `maa_sys::binding::loaded()` while you maintain a separate `RUNTIME_STATE`; consider using the atomic state (or at least documenting the relationship) so callers get a view that’s consistent with the new locking/transition semantics.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
wangl-cc
force-pushed
the
codex/maa-core-runtime-lifecycle
branch
from
May 24, 2026 22:22
9b6b052 to
b7739c6
Compare
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This was referenced May 29, 2026
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.
Summary
maa-corelifecycle module with separate runtime-loaded and linked implementations.LOCKEDbit for load/unload, aLOADEDbit for runtime availability, and low bits for active assistant count.Assistant::load()/Assistant::unload()while assistants or runtime changes are active, and rejectAssistant::new()when the runtime is unloaded or changing.Why
Assistantstores a raw MaaCore handle. Replacing or unloading the runtime-loaded MaaCore library while a handle is alive can leave later FFI calls, includingDrop, pointing at unloaded code. The safe wrapper now reserves active assistant handles before creating them and blocks runtime replacement or unload while those reservations are alive.The lock bit does not overwrite the loaded/count state, so existing assistants can still drop and decrement the active count while a runtime change is being rejected or unwound.
Validation
cargo +nightly fmtcargo test -p maa-corecargo check -p maa-core --no-default-featurescargo clippycargo x testSummary by Sourcery
通过集中管理生命周期跟踪并强制执行安全不变式，保护 MaaCore 运行时时期在并发加载、卸载和助手创建过程中的生命周期安全。
Bug 修复：
增强功能：
测试：
Original summary in English
Summary by Sourcery
Guard MaaCore runtime lifecycle against concurrent loads, unloads, and assistant creation by centralizing lifecycle tracking and enforcing safety invariants.
Bug Fixes:
Enhancements:
Tests: