Skip to content

fix:plugin uninstall file handle leak#8654

Open
lingyun14beta wants to merge 3 commits into
AstrBotDevs:masterfrom
lingyun14beta:fixenhance-plugin-uninstall-cleanup
Open

fix:plugin uninstall file handle leak#8654
lingyun14beta wants to merge 3 commits into
AstrBotDevs:masterfrom
lingyun14beta:fixenhance-plugin-uninstall-cleanup

Conversation

@lingyun14beta
Copy link
Copy Markdown
Contributor

@lingyun14beta lingyun14beta commented Jun 7, 2026

在 Windows 上卸载含有字体文件(如 .ttc)的插件时,字体文件句柄未被释放,导致 shutil.rmtree 删除目录时触发 [WinError 5] 拒绝访问,卸载失败。

Modifications / 改动点

astrbot/core/utils/io.py

  • 新增 _force_remove_file_windows:对被占用的文件带重试地强制删除
  • 新增 _on_error_windowsrmtree 的 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 在删除目录前依次调用上述两个方法
  • This is NOT a breaking change. / 这不是一个破坏性变更。

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.txt and pyproject.toml.
    / 我确保没有引入新依赖库,或者引入了新依赖库的同时将其添加到 requirements.txtpyproject.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:

  • Fix Windows plugin uninstall failures caused by locked font files by cancelling plugin tasks and releasing font handles before directory removal.
  • Improve directory removal on Windows by retrying deletion of locked files and handling permission errors for shutil.rmtree.

@dosubot dosubot Bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jun 7, 2026
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 1 issue, and left some high level feedback:

  • 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.
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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
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:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 task

Then _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 font

Then _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.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +1575 to +1586
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)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

任务取消安全性与 Frame 兼容性:

  1. 防止自我取消: 如果卸载操作是由运行插件代码的 Task 触发的(例如插件自定义的命令或按钮),asyncio.all_tasks() 会包含当前 Task。如果当前 Task 的模块名正好匹配 plugin_module_prefix,它会把自己取消掉,导致卸载流程在半途异常中止。我们应该显式地在取消列表中排除 asyncio.current_task()
  2. Frame 兼容性: 仅检查 cr_frame 可能会遗漏基于生成器的协程(使用 gi_frame)。同时检查 cr_framegi_frame 可以确保所有匹配的 Task 都能被正确识别并清理。
Suggested change
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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant