Skip to content

fix: initialize providers across plugin startup phases - #9513

Open
JosephTian876 wants to merge 2 commits into
AstrBotDevs:masterfrom
JosephTian876:fix/init-providers-before-plugins
Open

fix: initialize providers across plugin startup phases#9513
JosephTian876 wants to merge 2 commits into
AstrBotDevs:masterfrom
JosephTian876:fix/init-providers-before-plugins

Conversation

@JosephTian876

@JosephTian876 JosephTian876 commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

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 / 运行截图或测试结果

pytest tests/test_plugin_manager.py tests/unit/test_core_lifecycle.py \
       tests/unit/test_command_plugin_activation.py tests/test_cli_plugin.py
104 passed, 1 deselected

ruff format --check .
502 files already formatted

ruff check .
All checks passed!

git diff --check
passed

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:

  • a plugin constructor sees the configured non-fallback Provider;
  • import-time plugin Provider adapters exist before full Provider initialization;
  • adapters registered only from Star.initialize() are loaded afterward;
  • a failed late Provider does not block a valid one;
  • repeated reconciliation does not recreate or reinitialize an existing Provider;
  • plugin activation failure remains isolated and still runs the post-activation reconciliation;
  • warning timing observes the final default selection.

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 master after the prerequisite merges.


Checklist / 检查清单

  • 😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc. / Not applicable: this is a bug fix and adds no user-facing feature.
  • 👀 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.
  • 😮 My changes do not introduce malicious code.

@dosubot dosubot Bot added size:S This PR changes 10-29 lines, ignoring generated files. area:core The bug / feature is about astrbot's core, backend area:provider The bug / feature is about AI Provider, Models, LLM Agent, LLM Agent Runner. labels Aug 2, 2026
@dosubot

dosubot Bot commented Aug 2, 2026

Copy link
Copy Markdown

📄 Knowledge review

Dosu skipped reviewing this PR because your organization has used its 200 included credits for the month. Your usage will reset on 2026-09-01. To have Dosu review this PR before then, ask your organization admin to upgrade to a pro account.


Leave Feedback Ask Dosu about AstrBot Add Dosu to your team

@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 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>

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.

Comment thread astrbot/core/core_lifecycle.py Outdated
Comment thread tests/unit/test_core_lifecycle.py Outdated
@JosephTian876
JosephTian876 force-pushed the fix/init-providers-before-plugins branch from 053c299 to 89a54ab Compare August 2, 2026 14:48
@JosephTian876

Copy link
Copy Markdown
Contributor Author

Thanks for the review.

On the warning ordering — good catch, fixed in 34419e8.

Only the provider loading needed to move. _warn_about_unset_default_chat_provider() is back where it was, after plugin_manager.reload(), so it still reports the finally-resolved configuration:

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 position

On adding a behavioural test — I'd rather not, and here is why.

In test_initialize_sets_up_all_components the provider manager is a MagicMock, so a test that had a fake plugin "resolve a provider" during initialize() would be asserting against a fake instance map that the test itself populated. It would pass whether or not the ordering is correct, unless it also re-implemented get_using_provider()'s resolution — at which point it tests the double, not the code.

The contract splits cleanly in two, and both halves are already covered:

  • "an empty instance map makes a configured provider resolve to provider_insts[0]" — that is ProviderManager behaviour, covered by the tests in fix: warn when a configured provider is silently substituted #9484 against the real get_using_provider().
  • "providers are loaded before plugins start" — that is lifecycle behaviour, and the ordering assertion added here covers it. Restoring the old order makes it fail.

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 Provider ... not found warnings that appeared on every restart are gone.

@JosephTian876
JosephTian876 force-pushed the fix/init-providers-before-plugins branch from 34419e8 to 043124d Compare September 1, 2026 10:01
@JosephTian876 JosephTian876 changed the title fix: initialize providers before plugins fix: initialize providers across plugin startup phases Sep 1, 2026
@JosephTian876

Copy link
Copy Markdown
Contributor Author

Thanks for the earlier review. I have rewritten this PR on top of current master after validating the original assumption against real plugins.

The updated implementation now handles both registration phases:

  • Providers registered during plugin module import are initialized before plugin activation.
  • Providers registered inside Star.initialize() are initialized by a missing-only catch-up pass immediately after activation.
  • Existing Provider instances are not reinitialized, and one late Provider failure does not prevent the remaining Providers from loading.
  • The unset-default warning remains after plugin activation and the catch-up pass, so it reflects the final resolved state.

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 104 passed, 1 deselected; the deselection is the existing Windows symlink-privilege case. Ruff and the full GitHub Actions matrix are green.

This PR currently depends on #9900 for rollback-safe plugin imports. Once #9900 lands, I will rebase this branch and drop the prerequisite commit.

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

Labels

area:core The bug / feature is about astrbot's core, backend area:provider The bug / feature is about AI Provider, Models, LLM Agent, LLM Agent Runner. size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant