fix: initialize providers across plugin startup phases - #9513
fix: initialize providers across plugin startup phases#9513JosephTian876 wants to merge 2 commits into
Conversation
📄 Knowledge reviewDosu skipped reviewing this PR because your organization has used its |
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/core/core_lifecycle.py" line_range="257-259" />
<code_context>
+ # 根据配置实例化各个 Provider
+ # 必须先于插件初始化:插件可能在 initialize() 中发起 LLM 调用,
+ # 若此时 Provider 尚未装载,会解析到配置数组中的第一个 Provider。
+ self._default_chat_provider_warning_emitted = False
+ await self.provider_manager.initialize()
+ self._warn_about_unset_default_chat_provider()
+
# 初始化插件管理器
</code_context>
<issue_to_address>
**issue (bug_risk):** Warn-about-unset-default is now triggered before plugins can set a default provider.
With this ordering, the warning now reflects only the base configuration, ignoring any default provider a plugin might set during `initialize()`/`reload()`. That can produce misleading warnings when a default is effectively configured after plugins load. Consider either emitting the warning after `plugin_manager.reload()` or updating `_warn_about_unset_default_chat_provider()` to consider plugin-driven configuration so it reflects the final resolved default.
</issue_to_address>
### Comment 2
<location path="tests/unit/test_core_lifecycle.py" line_range="533" />
<code_context>
+ # Providers must be ready before plugins start: a plugin may issue an
+ # LLM call from initialize(), and an empty instance map would make that
+ # call resolve to an unrelated provider.
+ assert startup_order == ["providers", "plugins"]
+
# Verify pipeline scheduler loaded
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test that exercises the plugin–provider interaction that motivated this change, not just the call order.
To ensure the regression is fully covered, please add a test where a plugin mock performs a provider lookup or LLM call during `initialize()` and assert it resolves to the correct provider (with no fallback to index 0). This will verify the behavioral contract, not just the startup ordering.
Suggested implementation:
```python
# Verify pipeline scheduler loaded
assert lifecycle.pipeline_scheduler_mapping is not None
@pytest.mark.asyncio
async def test_plugin_initialize_resolves_correct_provider(
lifecycle,
mock_provider_manager,
mock_plugin_manager,
):
"""
Ensure that when a plugin performs a provider lookup / LLM call during
initialize(), it is resolved against the correct provider from the
provider manager's instance map and does not fall back to index 0.
"""
# Arrange: set up multiple providers and configure the non-zero-index
# provider as the one the plugin should use.
provider_index_0 = MagicMock(name="provider_index_0")
provider_target = MagicMock(name="provider_target")
mock_provider_manager.instance_map = {
"provider-index-0": provider_index_0,
"provider-target": provider_target,
}
# Whatever mechanism your lifecycle uses to choose the "correct" provider,
# configure it to point at the non-zero-index entry.
mock_provider_manager.default_provider_id = "provider-target"
async def plugin_initialize():
# During initialize(), the plugin performs a provider lookup / LLM call.
# This is the interaction that motivated the startup-order assertion:
# providers must be ready so this call hits the right provider.
resolved_provider = lifecycle.provider_manager.get_provider("provider-target")
resolved_provider.llm_call("hello from initialize")
plugin = MagicMock()
plugin.initialize = AsyncMock(side_effect=plugin_initialize)
mock_plugin_manager.plugins = [plugin]
# Act: run lifecycle startup so providers are initialized before plugins.
# This should ensure that the plugin's initialize() sees a fully-populated
# instance_map and resolves to the correct provider.
await lifecycle.startup()
# Assert: provider lookup / LLM call went to the correct provider,
# not the first one in instance_map.
provider_target.llm_call.assert_called_once_with("hello from initialize")
provider_index_0.llm_call.assert_not_called()
```
To integrate this test with the existing codebase, you will likely need to:
1. Ensure `pytest`, `MagicMock`, and `AsyncMock` are imported at the top of `tests/unit/test_core_lifecycle.py` if they are not already:
- `import pytest`
- `from unittest.mock import MagicMock, AsyncMock`
2. Adjust the fixture names in the test signature (`lifecycle`, `mock_provider_manager`, `mock_plugin_manager`) to match the actual fixtures or setup helpers defined in this test module (e.g. `core_lifecycle`, `provider_manager`, `plugin_manager`, etc.).
3. If the provider manager uses a different API than `instance_map`, `default_provider_id`, or `get_provider()`, adapt those calls:
- Use the real attribute that holds provider instances (e.g. `providers`, `providers_by_id`, etc.).
- Use the real way defaults are configured (e.g. `set_default_provider("provider-target")`).
- Replace `get_provider("provider-target")` with the appropriate lookup or LLM-call entry point your plugins actually use.
4. If plugins are not attached via `mock_plugin_manager.plugins`, modify the test to register the `plugin` using the real plugin registration mechanism used elsewhere in the lifecycle tests, so that `lifecycle.startup()` will invoke `plugin.initialize()`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
053c299 to
89a54ab
Compare
|
Thanks for the review. On the warning ordering — good catch, fixed in 34419e8. Only the provider loading needed to move. self._default_chat_provider_warning_emitted = False
await self.provider_manager.initialize() # moved earlier
self.plugin_manager = PluginManager(...)
await self.plugin_manager.reload()
self._warn_about_unset_default_chat_provider() # unchanged positionOn adding a behavioural test — I'd rather not, and here is why. In The contract splits cleanly in two, and both halves are already covered:
Together they cover the regression. Happy to add more if you disagree. For what it is worth, the change is also verified end to end on my own instance: startup ordering is now providers → plugins, and the |
34419e8 to
043124d
Compare
|
Thanks for the earlier review. I have rewritten this PR on top of current The updated implementation now handles both registration phases:
The behavioral coverage requested in the review is now included. Tests use the real Provider/Context registration path and cover late registration, failure isolation, idempotence, current-provider restoration, callback ordering, and plugin activation failure. The focused local suite reports This PR currently depends on #9900 for rollback-safe plugin imports. Once #9900 lands, I will rebase this branch and drop the prerequisite commit. |
AstrBot currently constructs and initializes plugin instances before configured Providers are available. A plugin that resolves its configured Provider during startup can therefore see an empty runtime or fall back to the wrong model.
Moving all Provider initialization before plugin loading is not compatible: plugin modules can register Provider adapters during import, and existing plugins also register adapters from
Star.initialize(). This update uses explicit startup phases and a missing-only reconciliation pass to preserve both extension patterns.Modifications / 改动点
Discover/import plugin modules before the first Provider pass so import-time adapters are registered.
Initialize configured Providers before constructing and activating plugin instances.
After activation, load only enabled, configured Provider IDs whose adapter was registered during
Star.initialize()and whose instance does not already exist.Isolate a failing late Provider so other pending Providers still load.
Restore persisted chat/STT/TTS selections when a late Provider becomes available, without re-running MCP initialization.
Keep the missing-only pass idempotent: existing instances are never constructed or initialized twice.
Reconcile live Provider instances if an activation failure rolls back the plugin adapter that created them.
Keep the unset-default warning after the final activation/selection state is known.
This is NOT a breaking change. / 这不是一个破坏性变更。
Screenshots or Test Results / 运行截图或测试结果
The deselected CLI test requires Windows symlink privileges and is unrelated to this change.
The new behavioral regressions address the previous review feedback directly. They exercise real startup boundaries and verify:
Star.initialize()are loaded afterward;This corrects the earlier PR description's assumption that plugins do not register Providers. Both import-time and activation-time registration are now covered.
No dependency or API/OpenAPI schema changes are introduced. Runtime plugin unload/update Provider reconciliation is a separate pre-existing lifecycle issue and is intentionally not changed by this startup-only PR.
Dependency
Depends on #9900, which makes plugin import/decorator side effects rollback-safe. Until #9900 is merged, this PR branch contains that prerequisite commit as the first commit. It will be rebased onto
masterafter the prerequisite merges.Checklist / 检查清单
requirements.txtandpyproject.toml.