fix(core): don't crash startup when a plugin registers an already-created Task - #10043
fix(core): don't crash startup when a plugin registers an already-created Task#10043YQteam-dyq wants to merge 3 commits into
Conversation
…ated Task Context.register_task() is typed as taking an Awaitable, but AstrBotCoreLifecycle._load() assumed every entry was a coroutine object and evaluated task.__name__. asyncio.Task is a valid Awaitable yet has no __name__ (it exposes get_name()), so a plugin that called create_task() itself before registering raised AttributeError: '_asyncio.Task' object has no attribute '__name__'. - reuse an already-created asyncio.Task instead of re-wrapping it - keep scheduling coroutine objects unchanged - warn and skip unrecognized awaitables instead of crashing - clear _register_tasks after consuming it (class variable, never cleared)
…d awaitable paths
kilisamemarisaaa
left a comment
There was a problem hiding this comment.
Context.register_task() still accepts Awaitable, but this branch drops valid non-coroutine awaitables and then clears their only registration reference. asyncio.Future is an Awaitable, as are objects implementing __await__; both enter the warning branch instead of being scheduled. The new Future test currently labels that valid input as "unrecognized" and locks in the contract regression.
I reproduced this at head cb67b698115fac33f937fbc3d27b49751e35c8e7 with a custom awaitable for which inspect.isawaitable(item) is true: after _load(), it had not run, _register_tasks was empty, and one warning had been emitted. The focused lifecycle suite passes, so the missing behavior is not otherwise covered.
Please either preserve the declared contract by wrapping/scheduling every awaitable (while continuing to reuse an existing asyncio.Task so _task_wrapper can use get_name()), or explicitly narrow register_task()'s public type and documentation to Coroutine | asyncio.Task and make the warning test use a genuinely non-awaitable value. The original Task.__name__ startup crash is real and the Task-specific fix is sound; this request is about avoiding a new silent-drop path for other inputs the API currently promises to accept.
EterUltimate
left a comment
There was a problem hiding this comment.
The fix itself is correct and well-tested — reusing an already-created asyncio.Task, keeping the coroutine path, and clearing Context._register_tasks after consuming it are all right (it is indeed a class variable, astrbot/core/star/context.py:129, so a second _load() would re-schedule the same tasks without the clear).
One repo-convention issue: per AGENTS.md, all logs must be in English. The new warning below is in Chinese — please apply the suggestion (the CI Actions checks also have not reported on this PR yet, likely waiting for first-contribution workflow approval from a maintainer).
| extra_tasks.append(asyncio.create_task(task, name=task.__name__)) | ||
| else: | ||
| logger.warning( | ||
| f"忽略无法识别的插件注册任务(期望协程或 asyncio.Task): {task!r}", |
There was a problem hiding this comment.
Per AGENTS.md, logs must be in English:
| f"忽略无法识别的插件注册任务(期望协程或 asyncio.Task): {task!r}", | |
| f"Skipping unrecognized plugin-registered task (expected a coroutine or asyncio.Task): {task!r}", |
Context.register_task() accepts any Awaitable, but _load() only handled coroutines and asyncio.Task. asyncio.Future and objects implementing __await__ fell into the warning branch, and _register_tasks.clear() then discarded the only reference, so those plugin tasks disappeared silently. Every valid awaitable is now converted into a real asyncio.Task before it is scheduled, so _task_wrapper() and stop() can keep relying on get_name() and cancel(). Coroutines are still wrapped unchanged, an already-created asyncio.Task is reused as-is, and only genuinely non-awaitable input is warned about and skipped. The tests use a genuinely non-awaitable value for the warning path and cover asyncio.Future plus a custom __await__ object to lock in the Awaitable contract.
kilisamemarisaaa
left a comment
There was a problem hiding this comment.
Re-reviewed head 405060d. The implementation now preserves the declared Awaitable contract: existing Tasks are reused, coroutines remain scheduled directly, Futures and custom __await__ objects are wrapped in named Tasks, and only genuinely non-awaitable values are warned and skipped. The added tests cover all of these paths and clear the registration list. This resolves my previous finding; I found no remaining correctness blocker.
Motivation / 动机
Context.register_task(task: Awaitable, desc: str)is typed as accepting anyAwaitable, butAstrBotCoreLifecycle._load()assumes every registered item is a coroutine object and evaluatestask.__name__:asyncio.Taskis a validAwaitable, but it has no__name__attribute (it exposesget_name()instead). So a plugin that callsasyncio.create_task()on its own coroutine and then registers the resulting Task raises:That exception propagates through
_load()→core_lifecycle.start()→initial_loader.start()→asyncio.run(), so the entire process exits during startup. A single plugin can take the whole app down, and the trigger is easy to hit because the API's own type hint explicitly permitsAwaitable. In practice the crash surfaces right after the WebUI banner and is followed by a cascade ofLifespanFailureError/CancelledErrornoise that hides the real cause.The same line also breaks on any other non-coroutine awaitable (e.g.
asyncio.Future), with the identical failure mode.Modifications / 改动点
astrbot/core/core_lifecycle.py—_load()now honours the declaredAwaitablecontract instead of assuming coroutines:asyncio.Taskas-is, so_task_wrapper/stop()keep working withget_name()andcancel().__name__).Awaitable—asyncio.Future, or any object implementing__await__— into a realasyncio.Taskso it is actually scheduled, instead of being dropped._register_tasksafter consuming it — it is aContextclass variable that was never cleared, so any second load would re-schedule the very same tasks.tests/unit/test_core_lifecycle.py— newTestAstrBotCoreLifecycleRegisteredTasksclass with 5 cases covering: the coroutine path, the already-created-Taskpath (the reported regression), theasyncio.Futurepath, the custom-__await__path, and the non-awaitable path.Screenshots or Test Results / 运行截图或测试结果
Reverting only the
core_lifecycle.pychange (keeping the new tests) makes all five new tests fail, confirming they are genuine regression tests:Checklist / 检查清单
requirements.txtandpyproject.toml. / 我确保没有引入新依赖库,或者引入了新依赖库的同时将其添加到requirements.txt和pyproject.toml文件相应位置。Summary by Sourcery
Handle plugin-registered awaitables safely during core lifecycle startup.
Bug Fixes:
Enhancements:
Tests: