fix: 修复inst_map未互斥访问导致的重载状态不一致, 增加前端检测启用逻辑 #2817
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
你好 - 我已审阅了你的更改，它们看起来很棒！
AI 代理提示
请处理此代码审查中的评论：
## Individual Comments
### Comment 1
<location> `astrbot/core/provider/manager.py:82-80` </location>
<code_context>
"""
- if provider_id not in self.inst_map:
- raise ValueError(f"提供商 {provider_id} 不存在，无法设置。")
+ async with self._inst_map_lock:
+ if provider_id not in self.inst_map:
+ raise ValueError(f"提供商 {provider_id} 不存在，无法设置。")
+ if not umo:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** 锁只覆盖了 inst_map 的访问，但 curr_provider_inst 的赋值在 umo 分支之外。
如果并发访问 curr_provider_inst，在锁之外设置它可能会导致竞态条件。考虑始终在锁内赋值 curr_provider_inst 以确保线程安全。
</issue_to_address>
### Comment 2
<location> `astrbot/dashboard/routes/config.py:522-525` </location>
<code_context>
+ )
+
+ # 先等待加载
+ target = await prov_mgr.get_provider_by_id(provider_id)
if not target:
logger.warning(
</code_context>
<issue_to_address>
**suggestion (bug_risk):** 切换到异步提供商查找是正确的，但请考虑超时错误处理。
考虑为 `get_provider_by_id` 实现超时或取消处理，以防止 API 在调用延迟时挂起。
```suggestion
# 先等待加载
import asyncio
try:
target = await asyncio.wait_for(
prov_mgr.get_provider_by_id(provider_id),
timeout=10 # seconds, adjust as needed
)
except asyncio.TimeoutError:
logger.warning(
f"Timeout while waiting for provider with id '{provider_id}' to load."
)
return (
Response()
.error(
f"Timeout while loading provider with id '{provider_id}'"
)
.__dict__
)
if not target:
logger.warning(
```
</issue_to_address>
### Comment 3
<location> `dashboard/src/views/ProviderPage.vue:833-837` </location>
<code_context>
- return 'default';
+ case 'available': return 'success'
+ case 'unavailable': return 'error'
+ case 'disabled': return 'warning'
+ case 'not_loaded': return 'warning'
+ default: return 'info'
}
</code_context>
<issue_to_address>
**suggestion:** 将 'disabled' 和 'not_loaded' 都映射到 'warning' 可能会降低清晰度。
考虑为 'disabled' 和 'not_loaded' 分配不同的颜色，以提高用户对这些不同状态的理解。
```suggestion
case 'available': return 'success'
case 'unavailable': return 'error'
case 'disabled': return 'warning'
case 'not_loaded': return 'error'
default: return 'info'
```
</issue_to_address>
### Comment 4
<location> `astrbot/core/provider/manager.py:394` </location>
<code_context>
async def reload(self, provider_config: dict):
provider_id = provider_config["id"]
# 只有禁用的直接终止
if not provider_config["enable"]:
async with self._inst_map_lock:
if provider_id in self.inst_map:
await self.terminate_provider(provider_id)
return
# 备份一份旧实例
old_instance = None
async with self._inst_map_lock:
old_instance = self.inst_map.get(provider_id)
try:
# 使用临时 ID 加载新实例
temp_config = provider_config.copy()
temp_id = f"{provider_id}_reload_temp"
temp_config["id"] = temp_id
await self.load_provider(temp_config)
# 新实例加载成功，替换旧实例
async with self._inst_map_lock:
if temp_id in self.inst_map:
new_instance = self.inst_map[temp_id]
del self.inst_map[temp_id]
new_instance.provider_config["id"] = provider_id
self.inst_map[provider_id] = new_instance
if old_instance:
if old_instance in self.provider_insts:
idx = self.provider_insts.index(old_instance)
self.provider_insts[idx] = new_instance
if old_instance in self.stt_provider_insts:
idx = self.stt_provider_insts.index(old_instance)
self.stt_provider_insts[idx] = new_instance
if old_instance in self.tts_provider_insts:
idx = self.tts_provider_insts.index(old_instance)
self.tts_provider_insts[idx] = new_instance
if old_instance in self.embedding_provider_insts:
idx = self.embedding_provider_insts.index(old_instance)
self.embedding_provider_insts[idx] = new_instance
# 更新当前实例引用
if self.curr_provider_inst == old_instance:
self.curr_provider_inst = new_instance
if self.curr_stt_provider_inst == old_instance:
self.curr_stt_provider_inst = new_instance
if self.curr_tts_provider_inst == old_instance:
self.curr_tts_provider_inst = new_instance
# 锁外清理旧实例
if old_instance and hasattr(old_instance, "terminate"):
try:
await old_instance.terminate()
except Exception as e:
logger.warning(f"清理旧 Provider 实例时出错: {e}")
except Exception as e:
# 清理临时实例
async with self._inst_map_lock:
if temp_id in self.inst_map:
temp_instance = self.inst_map[temp_id]
del self.inst_map[temp_id]
if hasattr(temp_instance, "terminate"):
try:
await temp_instance.terminate()
except Exception:
pass
raise e
# 清理已移除的提供商
config_ids = [provider["id"] for provider in self.providers_config]
async with self._inst_map_lock:
for key in list(self.inst_map.keys()):
if key not in config_ids:
await self.terminate_provider(key)
if len(self.provider_insts) == 0:
self.curr_provider_inst = None
elif self.curr_provider_inst is None and len(self.provider_insts) > 0:
self.curr_provider_inst = self.provider_insts[0]
logger.info(
f"自动选择 {self.curr_provider_inst.meta().id} 作为当前提供商适配器。"
)
if len(self.stt_provider_insts) == 0:
self.curr_stt_provider_inst = None
elif self.curr_stt_provider_inst is None and len(self.stt_provider_insts) > 0:
self.curr_stt_provider_inst = self.stt_provider_insts[0]
logger.info(
f"自动选择 {self.curr_stt_provider_inst.meta().id} 作为当前语音转文本提供商适配器。"
)
if len(self.tts_provider_insts) == 0:
self.curr_tts_provider_inst = None
elif self.curr_tts_provider_inst is None and len(self.tts_provider_insts) > 0:
self.curr_tts_provider_inst = self.tts_provider_insts[0]
logger.info(
f"自动选择 {self.curr_tts_provider_inst.meta().id} 作为当前文本转语音提供商适配器。"
)
</code_context>
<issue_to_address>
**issue (code-quality):** 在 ProviderManager.reload 中发现低代码质量 - 18% ([`low-code-quality`](https://docs.sourcery.ai/Reference/Default-Rules/comments/low-code-quality/))
<br/><details><summary>解释</summary>此函数的质量得分低于 25% 的质量阈值。
该分数是方法长度、认知复杂度和工作记忆的组合。
你如何解决这个问题？
重构此函数以使其更短、更具可读性可能是有益的。
- 通过将部分功能提取到自己的函数中来减少函数长度。这是你能做的最重要的事情——理想情况下，一个函数应该少于 10 行。
- 减少嵌套，例如通过引入守卫子句来提前返回。
- 确保变量的作用域紧密，以便使用相关概念的代码在函数中坐在一起而不是分散开来。</details>
</issue_to_address>
### Comment 5
<location> `astrbot/dashboard/routes/config.py:483-489` </location>
<code_context>
async def check_one_provider_status(self):
"""API: check a single LLM Provider's status by id"""
provider_id = request.args.get("id")
if not provider_id:
return self._error_response(
"Missing provider_id parameter", 400, logger.warning
)
logger.info(f"API call: /config/provider/check_one id={provider_id}")
try:
prov_mgr = self.core_lifecycle.provider_manager
# 配置里没有
provider_config = None
for config in self.config["provider"]:
if config.get("id") == provider_id:
provider_config = config
break
if not provider_config:
logger.warning(
f"Provider config with id '{provider_id}' not found in configuration."
)
return (
Response()
.error(
f"Provider with id '{provider_id}' not found in configuration"
)
.__dict__
)
# 没启用
if not provider_config.get("enable", False):
logger.info(f"Provider with id '{provider_id}' is disabled.")
return (
Response()
.ok(
{
"id": provider_id,
"model": provider_config.get("model", "Unknown Model"),
"type": provider_config.get(
"provider_type", "Unknown Type"
),
"name": provider_config.get("name", provider_id),
"status": "disabled",
"error": "Provider is disabled",
}
)
.__dict__
)
# 先等待加载
target = await prov_mgr.get_provider_by_id(provider_id)
if not target:
logger.warning(
f"Provider with id '{provider_id}' is enabled but not loaded in provider_manager."
)
return (
Response()
.ok(
{
"id": provider_id,
"model": provider_config.get("model", "Unknown Model"),
"type": provider_config.get(
"provider_type", "Unknown Type"
),
"name": provider_config.get("name", provider_id),
"status": "not_loaded",
"error": "Provider is enabled but failed to load. Check logs for details.",
}
)
.__dict__
)
# 已加载
result = await self._test_single_provider(target)
return Response().ok(result).__dict__
except Exception as e:
return self._error_response(
f"Critical error checking provider {provider_id}: {e}", 500
)
</code_context>
<issue_to_address>
**suggestion (code-quality):** 使用内置函数 `next` 代替 for 循环 ([`use-next`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-next/))
```suggestion
provider_config = next(
(
config
for config in self.config["provider"]
if config.get("id") == provider_id
),
None,
)
```
</issue_to_address>帮助我更有用！请点击每个评论上的 👍 或 👎，我将使用这些反馈来改进你的评论。
Original comment in English
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `astrbot/core/provider/manager.py:82-80` </location>
<code_context>
"""
- if provider_id not in self.inst_map:
- raise ValueError(f"提供商 {provider_id} 不存在，无法设置。")
+ async with self._inst_map_lock:
+ if provider_id not in self.inst_map:
+ raise ValueError(f"提供商 {provider_id} 不存在，无法设置。")
+ if not umo:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Locking only covers inst_map access, but curr_provider_inst assignment is outside umo branch.
If curr_provider_inst is accessed concurrently, setting it outside the lock may cause race conditions. Consider always assigning curr_provider_inst within the lock for thread safety.
</issue_to_address>
### Comment 2
<location> `astrbot/dashboard/routes/config.py:522-525` </location>
<code_context>
+ )
+
+ # 先等待加载
+ target = await prov_mgr.get_provider_by_id(provider_id)
if not target:
logger.warning(
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Switching to async provider lookup is correct, but consider error handling for timeouts.
Consider implementing a timeout or cancellation handling for get_provider_by_id to prevent the API from hanging if the call is delayed.
```suggestion
# 先等待加载
import asyncio
try:
target = await asyncio.wait_for(
prov_mgr.get_provider_by_id(provider_id),
timeout=10 # seconds, adjust as needed
)
except asyncio.TimeoutError:
logger.warning(
f"Timeout while waiting for provider with id '{provider_id}' to load."
)
return (
Response()
.error(
f"Timeout while loading provider with id '{provider_id}'"
)
.__dict__
)
if not target:
logger.warning(
```
</issue_to_address>
### Comment 3
<location> `dashboard/src/views/ProviderPage.vue:833-837` </location>
<code_context>
- return 'default';
+ case 'available': return 'success'
+ case 'unavailable': return 'error'
+ case 'disabled': return 'warning'
+ case 'not_loaded': return 'warning'
+ default: return 'info'
}
</code_context>
<issue_to_address>
**suggestion:** Mapping both 'disabled' and 'not_loaded' to 'warning' may reduce clarity.
Consider assigning different colors to 'disabled' and 'not_loaded' to improve user understanding of these distinct states.
```suggestion
case 'available': return 'success'
case 'unavailable': return 'error'
case 'disabled': return 'warning'
case 'not_loaded': return 'error'
default: return 'info'
```
</issue_to_address>
### Comment 4
<location> `astrbot/core/provider/manager.py:394` </location>
<code_context>
async def reload(self, provider_config: dict):
provider_id = provider_config["id"]
# 只有禁用的直接终止
if not provider_config["enable"]:
async with self._inst_map_lock:
if provider_id in self.inst_map:
await self.terminate_provider(provider_id)
return
# 备份一份旧实例
old_instance = None
async with self._inst_map_lock:
old_instance = self.inst_map.get(provider_id)
try:
# 使用临时 ID 加载新实例
temp_config = provider_config.copy()
temp_id = f"{provider_id}_reload_temp"
temp_config["id"] = temp_id
await self.load_provider(temp_config)
# 新实例加载成功，替换旧实例
async with self._inst_map_lock:
if temp_id in self.inst_map:
new_instance = self.inst_map[temp_id]
del self.inst_map[temp_id]
new_instance.provider_config["id"] = provider_id
self.inst_map[provider_id] = new_instance
if old_instance:
if old_instance in self.provider_insts:
idx = self.provider_insts.index(old_instance)
self.provider_insts[idx] = new_instance
if old_instance in self.stt_provider_insts:
idx = self.stt_provider_insts.index(old_instance)
self.stt_provider_insts[idx] = new_instance
if old_instance in self.tts_provider_insts:
idx = self.tts_provider_insts.index(old_instance)
self.tts_provider_insts[idx] = new_instance
if old_instance in self.embedding_provider_insts:
idx = self.embedding_provider_insts.index(old_instance)
self.embedding_provider_insts[idx] = new_instance
# 更新当前实例引用
if self.curr_provider_inst == old_instance:
self.curr_provider_inst = new_instance
if self.curr_stt_provider_inst == old_instance:
self.curr_stt_provider_inst = new_instance
if self.curr_tts_provider_inst == old_instance:
self.curr_tts_provider_inst = new_instance
# 锁外清理旧实例
if old_instance and hasattr(old_instance, "terminate"):
try:
await old_instance.terminate()
except Exception as e:
logger.warning(f"清理旧 Provider 实例时出错: {e}")
except Exception as e:
# 清理临时实例
async with self._inst_map_lock:
if temp_id in self.inst_map:
temp_instance = self.inst_map[temp_id]
del self.inst_map[temp_id]
if hasattr(temp_instance, "terminate"):
try:
await temp_instance.terminate()
except Exception:
pass
raise e
# 清理已移除的提供商
config_ids = [provider["id"] for provider in self.providers_config]
async with self._inst_map_lock:
for key in list(self.inst_map.keys()):
if key not in config_ids:
await self.terminate_provider(key)
if len(self.provider_insts) == 0:
self.curr_provider_inst = None
elif self.curr_provider_inst is None and len(self.provider_insts) > 0:
self.curr_provider_inst = self.provider_insts[0]
logger.info(
f"自动选择 {self.curr_provider_inst.meta().id} 作为当前提供商适配器。"
)
if len(self.stt_provider_insts) == 0:
self.curr_stt_provider_inst = None
elif self.curr_stt_provider_inst is None and len(self.stt_provider_insts) > 0:
self.curr_stt_provider_inst = self.stt_provider_insts[0]
logger.info(
f"自动选择 {self.curr_stt_provider_inst.meta().id} 作为当前语音转文本提供商适配器。"
)
if len(self.tts_provider_insts) == 0:
self.curr_tts_provider_inst = None
elif self.curr_tts_provider_inst is None and len(self.tts_provider_insts) > 0:
self.curr_tts_provider_inst = self.tts_provider_insts[0]
logger.info(
f"自动选择 {self.curr_tts_provider_inst.meta().id} 作为当前文本转语音提供商适配器。"
)
</code_context>
<issue_to_address>
**issue (code-quality):** Low code quality found in ProviderManager.reload - 18% ([`low-code-quality`](https://docs.sourcery.ai/Reference/Default-Rules/comments/low-code-quality/))
<br/><details><summary>Explanation</summary>The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines.
- Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.</details>
</issue_to_address>
### Comment 5
<location> `astrbot/dashboard/routes/config.py:483-489` </location>
<code_context>
async def check_one_provider_status(self):
"""API: check a single LLM Provider's status by id"""
provider_id = request.args.get("id")
if not provider_id:
return self._error_response(
"Missing provider_id parameter", 400, logger.warning
)
logger.info(f"API call: /config/provider/check_one id={provider_id}")
try:
prov_mgr = self.core_lifecycle.provider_manager
# 配置里没有
provider_config = None
for config in self.config["provider"]:
if config.get("id") == provider_id:
provider_config = config
break
if not provider_config:
logger.warning(
f"Provider config with id '{provider_id}' not found in configuration."
)
return (
Response()
.error(
f"Provider with id '{provider_id}' not found in configuration"
)
.__dict__
)
# 没启用
if not provider_config.get("enable", False):
logger.info(f"Provider with id '{provider_id}' is disabled.")
return (
Response()
.ok(
{
"id": provider_id,
"model": provider_config.get("model", "Unknown Model"),
"type": provider_config.get(
"provider_type", "Unknown Type"
),
"name": provider_config.get("name", provider_id),
"status": "disabled",
"error": "Provider is disabled",
}
)
.__dict__
)
# 先等待加载
target = await prov_mgr.get_provider_by_id(provider_id)
if not target:
logger.warning(
f"Provider with id '{provider_id}' is enabled but not loaded in provider_manager."
)
return (
Response()
.ok(
{
"id": provider_id,
"model": provider_config.get("model", "Unknown Model"),
"type": provider_config.get(
"provider_type", "Unknown Type"
),
"name": provider_config.get("name", provider_id),
"status": "not_loaded",
"error": "Provider is enabled but failed to load. Check logs for details.",
}
)
.__dict__
)
# 已加载
result = await self._test_single_provider(target)
return Response().ok(result).__dict__
except Exception as e:
return self._error_response(
f"Critical error checking provider {provider_id}: {e}", 500
)
</code_context>
<issue_to_address>
**suggestion (code-quality):** Use the built-in function `next` instead of a for-loop ([`use-next`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-next/))
```suggestion
provider_config = next(
(
config
for config in self.config["provider"]
if config.get("id") == provider_id
),
None,
)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
同步了样式
|
|
可以给一下复现这个问题的操作步骤吗 |
这个简单, 在这个地方: 然后接下来注册一堆Provider, 再全部禁用, 然后重启AstrBot清空实例表 |
|
刚刚触发的, 不需要修改配置 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation / 动机
Modifications / 改动点
Verification Steps / 验证步骤
如图
修改温度保存后:
Screenshots or Test Results / 运行截图或测试结果
Compatibility & Breaking Changes / 兼容性与破坏性变更
Checklist / 检查清单
requirements.txt和pyproject.toml文件相应位置。/ I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations inrequirements.txtandpyproject.toml.Sourcery 摘要
在 ProviderManager 中，强制实现对提供者实例的线程安全访问和一致的状态管理，改进提供者重新加载和终止逻辑，并增强前端和 API 行为，以正确处理已禁用 (disabled) 和未加载 (not_loaded) 的提供者状态。
错误修复：
功能增强：
Original summary in English
Summary by Sourcery
Enforce thread-safe access and consistent state management for provider instances in ProviderManager, improve provider reload and termination logic, and enhance front-end and API behavior to correctly handle disabled and not_loaded provider statuses.
Bug Fixes:
Enhancements: