fix:plugin uninstall file handle leak#8654
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
_force_remove_file_windowsyou calltime.sleepbut there is no correspondingimport timein this function or in the surrounding diff, so please ensuretimeis imported to avoid a NameError at runtime. - The
_release_plugin_font_handlesimplementation mutates arbitrary referrer dicts obtained viagc.get_referrers, which is quite invasive and may break unrelated objects; consider narrowing the scope (e.g., only touching known structures or callingclose()/cleanup APIs onFreeTypeFontinstances) to reduce the risk of unintended side effects.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_force_remove_file_windows` you call `time.sleep` but there is no corresponding `import time` in this function or in the surrounding diff, so please ensure `time` is imported to avoid a NameError at runtime.
- The `_release_plugin_font_handles` implementation mutates arbitrary referrer dicts obtained via `gc.get_referrers`, which is quite invasive and may break unrelated objects; consider narrowing the scope (e.g., only touching known structures or calling `close()`/cleanup APIs on `FreeTypeFont` instances) to reduce the risk of unintended side effects.
## Individual Comments
### Comment 1
<location path="astrbot/core/star/star_manager.py" line_range="1572" />
<code_context>
delete_data=delete_data,
)
+ async def _cancel_plugin_tasks(self, plugin_module_prefix: str) -> None:
+ """取消并等待属于指定插件模块的所有 asyncio tasks。"""
+ plugin_tasks = []
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the global task and GC introspection logic with explicit per-plugin registries for tasks and fonts to simplify uninstall handling.
The main complexity comes from the global introspection in `_cancel_plugin_tasks` and `_release_plugin_font_handles`. Both can be simplified by explicit per‑plugin tracking while keeping the same behavior.
### 1. Replace `_cancel_plugin_tasks` introspection with explicit tracking
Instead of scanning `asyncio.all_tasks()` and inspecting frames, track tasks when they are created. For example, add a per‑plugin registry and a helper:
```python
# somewhere on the manager
self._plugin_tasks: dict[str, set[asyncio.Task]] = defaultdict(set)
def create_plugin_task(self, plugin_name: str, coro: Coroutine) -> asyncio.Task:
task = asyncio.create_task(coro)
self._plugin_tasks[plugin_name].add(task)
task.add_done_callback(lambda t, name=plugin_name: self._plugin_tasks[name].discard(t))
return task
```
Then `_cancel_plugin_tasks` becomes much simpler and explicit:
```python
async def _cancel_plugin_tasks(self, plugin_name: str) -> None:
tasks = list(self._plugin_tasks.get(plugin_name, ()))
if not tasks:
return
for t in tasks:
if not t.done():
t.cancel()
try:
await asyncio.wait_for(
asyncio.gather(*tasks, return_exceptions=True),
timeout=5.0,
)
except asyncio.TimeoutError:
logger.warning(
f"插件 {plugin_name} 的部分 tasks 在 5 秒内未能取消,可能导致文件句柄未释放。"
)
self._plugin_tasks.pop(plugin_name, None)
```
Callers inside plugin code (or the plugin manager) should use `create_plugin_task` instead of `asyncio.create_task`, preserving all existing behavior while avoiding frame inspection and global task scanning.
### 2. Replace GC sweeping for fonts with an explicit font registry
Instead of walking `gc.get_objects()` / `get_referrers`, ensure fonts loaded by a plugin are tracked when created. For example, centralize font loading for plugins:
```python
# on the plugin object or manager
class Plugin:
def __init__(self, ...):
self.fonts: list[ImageFont.FreeTypeFont] = []
def load_plugin_font(self, plugin: Plugin, path: str, *args, **kwargs) -> ImageFont.FreeTypeFont:
font = ImageFont.truetype(path, *args, **kwargs)
plugin.fonts.append(font)
return font
```
Then `_release_plugin_font_handles` can directly release/clear them:
```python
def _release_plugin_font_handles(self, plugin, plugin_dir: str) -> None:
# clear plugin references first
plugin.star_cls = None
plugin.module = None
plugin.star_cls_type = None
for font in getattr(plugin, "fonts", []):
try:
font.close() # if available in your Pillow version
except Exception:
pass
plugin.fonts.clear()
```
If you can’t modify plugin code directly, you can still centralize font creation in the manager (e.g. exposing a `get_font` helper that plugins are required to use) rather than sweeping the entire object graph.
This keeps the uninstall semantics (cancel plugin tasks, release plugin font handles) but removes the dependence on CPython GC internals and global introspection, making the behavior easier to reason about and maintain.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| delete_data=delete_data, | ||
| ) | ||
|
|
||
| async def _cancel_plugin_tasks(self, plugin_module_prefix: str) -> None: |
There was a problem hiding this comment.
issue (complexity): Consider replacing the global task and GC introspection logic with explicit per-plugin registries for tasks and fonts to simplify uninstall handling.
The main complexity comes from the global introspection in _cancel_plugin_tasks and _release_plugin_font_handles. Both can be simplified by explicit per‑plugin tracking while keeping the same behavior.
1. Replace _cancel_plugin_tasks introspection with explicit tracking
Instead of scanning asyncio.all_tasks() and inspecting frames, track tasks when they are created. For example, add a per‑plugin registry and a helper:
# somewhere on the manager
self._plugin_tasks: dict[str, set[asyncio.Task]] = defaultdict(set)
def create_plugin_task(self, plugin_name: str, coro: Coroutine) -> asyncio.Task:
task = asyncio.create_task(coro)
self._plugin_tasks[plugin_name].add(task)
task.add_done_callback(lambda t, name=plugin_name: self._plugin_tasks[name].discard(t))
return taskThen _cancel_plugin_tasks becomes much simpler and explicit:
async def _cancel_plugin_tasks(self, plugin_name: str) -> None:
tasks = list(self._plugin_tasks.get(plugin_name, ()))
if not tasks:
return
for t in tasks:
if not t.done():
t.cancel()
try:
await asyncio.wait_for(
asyncio.gather(*tasks, return_exceptions=True),
timeout=5.0,
)
except asyncio.TimeoutError:
logger.warning(
f"插件 {plugin_name} 的部分 tasks 在 5 秒内未能取消,可能导致文件句柄未释放。"
)
self._plugin_tasks.pop(plugin_name, None)Callers inside plugin code (or the plugin manager) should use create_plugin_task instead of asyncio.create_task, preserving all existing behavior while avoiding frame inspection and global task scanning.
2. Replace GC sweeping for fonts with an explicit font registry
Instead of walking gc.get_objects() / get_referrers, ensure fonts loaded by a plugin are tracked when created. For example, centralize font loading for plugins:
# on the plugin object or manager
class Plugin:
def __init__(self, ...):
self.fonts: list[ImageFont.FreeTypeFont] = []
def load_plugin_font(self, plugin: Plugin, path: str, *args, **kwargs) -> ImageFont.FreeTypeFont:
font = ImageFont.truetype(path, *args, **kwargs)
plugin.fonts.append(font)
return fontThen _release_plugin_font_handles can directly release/clear them:
def _release_plugin_font_handles(self, plugin, plugin_dir: str) -> None:
# clear plugin references first
plugin.star_cls = None
plugin.module = None
plugin.star_cls_type = None
for font in getattr(plugin, "fonts", []):
try:
font.close() # if available in your Pillow version
except Exception:
pass
plugin.fonts.clear()If you can’t modify plugin code directly, you can still centralize font creation in the manager (e.g. exposing a get_font helper that plugins are required to use) rather than sweeping the entire object graph.
This keeps the uninstall semantics (cancel plugin tasks, release plugin font handles) but removes the dependence on CPython GC internals and global introspection, making the behavior easier to reason about and maintain.
There was a problem hiding this comment.
Code Review
This pull request introduces mechanisms to cleanly uninstall plugins, specifically addressing file locking issues on Windows. It cancels active asyncio tasks associated with the plugin, releases PIL font handles, and implements a robust file deletion utility with retries and permission handling for Windows environments. The review feedback suggests preventing self-cancellation of the uninstallation task itself and improving frame compatibility by checking both cr_frame and gi_frame when identifying plugin tasks.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| for task in asyncio.all_tasks(): | ||
| if task.done(): | ||
| continue | ||
| coro = task.get_coro() | ||
| frame = getattr(coro, "cr_frame", None) | ||
| if frame is None: | ||
| continue | ||
| mod_name = frame.f_globals.get("__name__", "") | ||
| if mod_name == plugin_module_prefix or mod_name.startswith( | ||
| plugin_module_prefix + "." | ||
| ): | ||
| plugin_tasks.append(task) |
There was a problem hiding this comment.
任务取消安全性与 Frame 兼容性:
- 防止自我取消: 如果卸载操作是由运行插件代码的 Task 触发的(例如插件自定义的命令或按钮),
asyncio.all_tasks()会包含当前 Task。如果当前 Task 的模块名正好匹配plugin_module_prefix,它会把自己取消掉,导致卸载流程在半途异常中止。我们应该显式地在取消列表中排除asyncio.current_task()。 - Frame 兼容性: 仅检查
cr_frame可能会遗漏基于生成器的协程(使用gi_frame)。同时检查cr_frame和gi_frame可以确保所有匹配的 Task 都能被正确识别并清理。
| for task in asyncio.all_tasks(): | |
| if task.done(): | |
| continue | |
| coro = task.get_coro() | |
| frame = getattr(coro, "cr_frame", None) | |
| if frame is None: | |
| continue | |
| mod_name = frame.f_globals.get("__name__", "") | |
| if mod_name == plugin_module_prefix or mod_name.startswith( | |
| plugin_module_prefix + "." | |
| ): | |
| plugin_tasks.append(task) | |
| current_task = asyncio.current_task() | |
| for task in asyncio.all_tasks(): | |
| if task.done() or task is current_task: | |
| continue | |
| coro = task.get_coro() | |
| frame = getattr(coro, "cr_frame", None) or getattr(coro, "gi_frame", None) | |
| if frame is None: | |
| continue | |
| mod_name = frame.f_globals.get("__name__", "") | |
| if mod_name == plugin_module_prefix or mod_name.startswith( | |
| plugin_module_prefix + "." | |
| ): | |
| plugin_tasks.append(task) |
在 Windows 上卸载含有字体文件(如
.ttc)的插件时,字体文件句柄未被释放,导致shutil.rmtree删除目录时触发[WinError 5] 拒绝访问,卸载失败。Modifications / 改动点
astrbot/core/utils/io.py_force_remove_file_windows:对被占用的文件带重试地强制删除_on_error_windows:rmtree的 Windows 错误回调,对WinError 5/32的文件走重试,目录走原有权限修复逻辑remove_dir在 Windows 上使用上述增强回调astrbot/core/star/star_manager.py_cancel_plugin_tasks:卸载前取消插件创建的 asyncio tasks,防止协程帧持有插件实例进而持有字体对象_release_plugin_font_handles:断开StarMetadata上的已知引用链(star_cls/module/star_cls_type),并用gc.get_objects()扫描所有存活的FreeTypeFont对象,找出路径在插件目录下的,通过gc.get_referrers强制断开引用,触发 GC 释放文件句柄uninstall_plugin在删除目录前依次调用上述两个方法Screenshots or Test Results / 运行截图或测试结果
Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能,已经通过 Issue / 邮件等方式和作者讨论过。
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试,并已在上方提供了“验证步骤”和“运行截图”。
🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库,或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码。
Summary by Sourcery
Ensure Windows plugin uninstallation can delete font files by releasing font handles and hardening directory removal.
Bug Fixes: