Skip to content

fix(core): don't crash startup when a plugin registers an already-created Task - #10043

Open
YQteam-dyq wants to merge 3 commits into
AstrBotDevs:masterfrom
YQteam-dyq:fix/register-task-non-coroutine-crash
Open

fix(core): don't crash startup when a plugin registers an already-created Task#10043
YQteam-dyq wants to merge 3 commits into
AstrBotDevs:masterfrom
YQteam-dyq:fix/register-task-non-coroutine-crash

Conversation

@YQteam-dyq

@YQteam-dyq YQteam-dyq commented Sep 11, 2026

Copy link
Copy Markdown

Motivation / 动机

Context.register_task(task: Awaitable, desc: str) is typed as accepting any Awaitable, but AstrBotCoreLifecycle._load() assumes every registered item is a coroutine object and evaluates task.__name__:

extra_tasks.append(asyncio.create_task(task, name=task.__name__))

asyncio.Task is a valid Awaitable, but it has no __name__ attribute (it exposes get_name() instead). So a plugin that calls asyncio.create_task() on its own coroutine and then registers the resulting Task raises:

AttributeError: '_asyncio.Task' object has no attribute '__name__'

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 permits Awaitable. In practice the crash surfaces right after the WebUI banner and is followed by a cascade of LifespanFailureError / CancelledError noise 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 declared Awaitable contract instead of assuming coroutines:

  • Reuse an already-created asyncio.Task as-is, so _task_wrapper / stop() keep working with get_name() and cancel().
  • Keep scheduling coroutine objects exactly as before (task name still comes from __name__).
  • Wrap every other valid Awaitableasyncio.Future, or any object implementing __await__ — into a real asyncio.Task so it is actually scheduled, instead of being dropped.
  • Log a warning and skip only inputs that are not awaitable at all.
  • Clear _register_tasks after consuming it — it is a Context class variable that was never cleared, so any second load would re-schedule the very same tasks.

tests/unit/test_core_lifecycle.py — new TestAstrBotCoreLifecycleRegisteredTasks class with 5 cases covering: the coroutine path, the already-created-Task path (the reported regression), the asyncio.Future path, the custom-__await__ path, and the non-awaitable path.

  • This is NOT a breaking change. / 这不是一个破坏性变更。

Screenshots or Test Results / 运行截图或测试结果

$ python -m pytest tests/unit/test_core_lifecycle.py -q
31 passed, 1 warning in 7.92s

$ python -m ruff format --check .
504 files already formatted

$ python -m ruff check .
All checks passed!

Reverting only the core_lifecycle.py change (keeping the new tests) makes all five new tests fail, confirming they are genuine regression tests:

$ python -m pytest tests/unit/test_core_lifecycle.py -q -k RegisteredTasks
>           extra_tasks.append(asyncio.create_task(task, name=task.__name__))  # type: ignore
                                                              ^^^^^^^^^^^^^
E           AttributeError: '_CustomAwaitable' object has no attribute '__name__'. Did you mean: '__ne__'?
astrbot\core\core_lifecycle.py:322: AttributeError
=========================== short test summary info ============================
FAILED tests/unit/test_core_lifecycle.py::TestAstrBotCoreLifecycleRegisteredTasks::test_load_schedules_coroutine_registered_task
FAILED tests/unit/test_core_lifecycle.py::TestAstrBotCoreLifecycleRegisteredTasks::test_load_reuses_already_created_task - AttributeError: '_asyncio.Task' object has no attribute '__name__'.
FAILED tests/unit/test_core_lifecycle.py::TestAstrBotCoreLifecycleRegisteredTasks::test_load_skips_non_awaitable_registered_task - AttributeError: 'object' object has no attribute '__name__'.
FAILED tests/unit/test_core_lifecycle.py::TestAstrBotCoreLifecycleRegisteredTasks::test_load_schedules_future_registered_task - AttributeError: '_asyncio.Future' object has no attribute '__name__'.
FAILED tests/unit/test_core_lifecycle.py::TestAstrBotCoreLifecycleRegisteredTasks::test_load_schedules_custom_awaitable_registered_task - AttributeError: '_CustomAwaitable' object has no attribute '__name__'.
5 failed, 26 deselected, 1 warning in 21.77s

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

Handle plugin-registered awaitables safely during core lifecycle startup.

Bug Fixes:

  • Prevent startup crashes when plugins register already-created tasks or other supported awaitables.
  • Skip invalid non-awaitable registrations with a warning instead of aborting the application.

Enhancements:

  • Reuse existing asyncio tasks, wrap supported awaitables for lifecycle management, and clear registered tasks after loading.

Tests:

  • Add coverage for coroutine, task, future, custom awaitable, and invalid registered-task handling.

…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)

@sourcery-ai sourcery-ai Bot left a comment

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.

Hey - I've reviewed your changes and they look great!

Sourcery assessment

Approved.


Sourcery is free for open source - if you like our reviews please consider sharing them ✨

@kilisamemarisaaa kilisamemarisaaa left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 EterUltimate left a comment

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.

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

Comment thread astrbot/core/core_lifecycle.py Outdated
extra_tasks.append(asyncio.create_task(task, name=task.__name__))
else:
logger.warning(
f"忽略无法识别的插件注册任务(期望协程或 asyncio.Task): {task!r}",

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.

Per AGENTS.md, logs must be in English:

Suggested change
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 kilisamemarisaaa left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants