fix(weixin_oc): persist context_token for proactive cron sends#7595
Conversation
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The normalization of
context_tokensis duplicated in_load_account_stateand_save_account_state; consider extracting a small helper (e.g.,_normalize_context_tokens) to keep this logic consistent and easier to maintain. - In
_save_account_state, you both normalize into a local dict and then reassignself._context_tokensto that normalized version; if other in-memory code relies on the original mapping semantics, you may want to avoid mutating_context_tokenshere and instead only write the normalized version intoconfig/platform.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The normalization of `context_tokens` is duplicated in `_load_account_state` and `_save_account_state`; consider extracting a small helper (e.g., `_normalize_context_tokens`) to keep this logic consistent and easier to maintain.
- In `_save_account_state`, you both normalize into a local dict and then reassign `self._context_tokens` to that normalized version; if other in-memory code relies on the original mapping semantics, you may want to avoid mutating `_context_tokens` here and instead only write the normalized version into `config`/`platform`.
## Individual Comments
### Comment 1
<location path="astrbot/core/platform/sources/weixin_oc/weixin_oc_adapter.py" line_range="152" />
<code_context>
self._sync_buf = ""
self._qr_expired_count = 0
self._context_tokens: dict[str, str] = {}
+ self._context_tokens_dirty = False
self._typing_states: dict[str, TypingSessionState] = {}
self._last_inbound_error = ""
</code_context>
<issue_to_address>
**issue (bug_risk):** Resetting `_context_tokens_dirty` before persisting config can drop unsaved state if a later step fails.
In `_save_account_state`, `_context_tokens_dirty` is set to `False` before `_sync_client_state()` and `astrbot_config.save_config()`. If either fails, memory will indicate state is saved while the config may be stale. Please move the flag reset to after all persistence calls so failures keep `_context_tokens_dirty` as `True` and allow a proper retry.
</issue_to_address>
### Comment 2
<location path="astrbot/core/platform/sources/weixin_oc/weixin_oc_adapter.py" line_range="542" />
<code_context>
saved_base = str(self.config.get("weixin_oc_base_url", "")).strip()
if saved_base:
self.base_url = saved_base.rstrip("/")
+ raw_context_tokens = self.config.get("weixin_oc_context_tokens", {})
+ if isinstance(raw_context_tokens, dict):
+ normalized_context_tokens: dict[str, str] = {}
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting shared context-token normalization into a helper and using a single dirty-state flag so loading, saving, and update polling stay simpler and less duplicated.
You can keep the new behavior but cut down on duplication and branching by:
1. Extracting context-token normalization into a single helper.
2. Keeping `_save_account_state` focused on persisting (no surprise mutation beyond what’s needed).
3. Using a single “dirty” flag in `_poll_inbound_updates`.
### 1. Consolidate normalization logic
Instead of duplicating normalization in `_load_account_state` and `_save_account_state`, add a private helper:
```python
from collections.abc import Mapping
def _normalize_context_tokens(
self, raw: Mapping[object, object]
) -> dict[str, str]:
normalized: dict[str, str] = {}
for user_id, context_token in raw.items():
normalized_user_id = str(user_id).strip()
normalized_context_token = str(context_token).strip()
if not normalized_user_id or not normalized_context_token:
continue
normalized[normalized_user_id] = normalized_context_token
return normalized
```
Use it when loading:
```python
raw_context_tokens = self.config.get("weixin_oc_context_tokens", {})
if isinstance(raw_context_tokens, dict):
self._context_tokens = self._normalize_context_tokens(raw_context_tokens)
```
And when saving (note: no need to mutate `self._context_tokens` here):
```python
async def _save_account_state(self) -> None:
normalized_context_tokens = self._normalize_context_tokens(self._context_tokens)
self.config["weixin_oc_token"] = self.token or ""
self.config["weixin_oc_account_id"] = self.account_id or ""
self.config["weixin_oc_sync_buf"] = self._sync_buf
self.config["weixin_oc_base_url"] = self.base_url
self.config["weixin_oc_context_tokens"] = normalized_context_tokens
for platform in astrbot_config.get("platform", []):
if not isinstance(platform, dict):
continue
if platform.get("id") != self.config.get("id"):
continue
if platform.get("type") != self.config.get("type"):
continue
platform["weixin_oc_token"] = self.token or ""
platform["weixin_oc_account_id"] = self.account_id or ""
platform["weixin_oc_sync_buf"] = self._sync_buf
platform["weixin_oc_base_url"] = self.base_url
platform["weixin_oc_context_tokens"] = normalized_context_tokens
break
self._context_tokens_dirty = False
self._sync_client_state()
astrbot_config.save_config()
```
This keeps functionality while centralizing the normalization rules.
### 2. Simplify dirty-state handling in `_poll_inbound_updates`
You can avoid tracking both `should_save_state` and `_context_tokens_dirty` separately inside `_poll_inbound_updates` by collapsing them into a single local flag that incorporates the instance flag:
```python
async def _poll_inbound_updates(self) -> None:
...
if not self._is_successful_api_payload(data):
...
return
state_dirty = self._context_tokens_dirty
if data.get("get_updates_buf"):
self._sync_buf = str(data.get("get_updates_buf"))
state_dirty = True
for msg in data.get("msgs", []) if isinstance(data.get("msgs"), list) else []:
if self._shutdown_event.is_set():
return
if not isinstance(msg, dict):
continue
await self._handle_inbound_message(msg)
if state_dirty:
await self._save_account_state()
```
`_handle_inbound_message` can keep setting `self._context_tokens_dirty = True` when tokens change; `_poll_inbound_updates` just snapshots that into `state_dirty` and ensures a single clear condition to decide on saving.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request introduces persistence for weixin_oc_context_tokens in the WeixinOCAdapter, including dirty state tracking and batch saving. Feedback suggests removing redundant normalization logic in the save method and replacing a return with a break during shutdown to ensure state is persisted before the process terminates.
| if self._shutdown_event.is_set(): | ||
| return |
There was a problem hiding this comment.
Returning early when _shutdown_event is set prevents the updated _sync_buf (and any new context_tokens) from being persisted at line 1579. This can lead to duplicate message processing when the bot restarts, as the sync_buf remains at its previous state on disk. Using break instead of return ensures that the loop terminates but the state is still saved before the method finishes.
| if self._shutdown_event.is_set(): | |
| return | |
| if self._shutdown_event.is_set(): | |
| break |
|
根据代码审查机器人的意见,做了以下调整:
|
…otDevs#7595) * fix(weixin_oc): persist context_token for proactive cron sends * test(weixin_oc): add safety coverage for context token persistence * fix(weixin_oc): address review on context token state * chore: delete tests/unit/test_weixin_oc_adapter_state.py --------- Co-authored-by: Soulter <37870767+Soulter@users.noreply.github.com>
概要 / Summary
weixin_oc适配器持久化context_token状态,使计划任务等主动发送场景在会话或线程重建后仍可复用最近一次有效 token。context_tokenstate for theweixin_ocadapter so proactive cron sends can reuse the latest valid token after session or thread recreation.验证 / Verification
python -m ruff format astrbot/core/platform/sources/weixin_oc/weixin_oc_adapter.py tests/unit/test_weixin_oc_adapter_state.pypython -m ruff check astrbot/core/platform/sources/weixin_oc/weixin_oc_adapter.py tests/unit/test_weixin_oc_adapter_state.pypython -m pytest tests/unit/test_weixin_oc_adapter_state.py -q影响范围 / Scope
weixin_oc个人微信平台适配器路径。weixin_ocplatform adapter path.Fixes #7591
Summary by Sourcery
Persist and reuse valid weixin_oc context tokens across sessions while only saving account state when tokens or sync buffer change.
New Features:
Bug Fixes:
Tests:
补充说明 / Follow-up
根据代码审查机器人的意见,做了以下调整:
context_token的共用规范化逻辑_context_tokens_context_tokens_dirty改为持久化成功后再清除