fix: 钉钉连接超时后自动重连失败

[Blueteemo]

问题描述

钉钉连接每12小时超时，SDK 重连后无法回复消息。

修复方案

改进
un() 方法，添加重试逻辑。当 SDK task 意外退出时，自动重启并重试（最多5次，每次间隔10秒）。

关联 Issue

Fixes #7861

Summary by Sourcery

Improve the DingTalk adapter run loop to make the SDK task resilient to unexpected termination and connection timeouts.

Bug Fixes:

• Ensure the DingTalk SDK task is automatically restarted with limited retries when it exits unexpectedly after connection timeouts.

Enhancements:

• Add retry counting, logging, and controlled backoff delays to DingTalk client startup to avoid silent failures and repeated crash loops.

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• In start_client, the finally block always runs and will cancel the task when shutdown_event is set even in the ‘normal close’ path where you just returned because the task is still running; this contradicts the comment and likely prevents a clean shutdown—consider only cancelling when you explicitly want to abort (e.g., on retry/failure paths).
• The task variable is referenced in the finally block but may not be defined if an exception is raised before loop.create_task is called; initialize task to None before the try and guard the task.cancel()/task.done() checks accordingly.
• Calling task.cancel() from a worker thread can be unsafe; instead of directly cancelling the task from start_client, use loop.call_soon_threadsafe(task.cancel) to interact with the asyncio loop from another thread.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `start_client`, the `finally` block always runs and will cancel the task when `shutdown_event` is set even in the ‘normal close’ path where you just returned because the task is still running; this contradicts the comment and likely prevents a clean shutdown—consider only cancelling when you explicitly want to abort (e.g., on retry/failure paths).
- The `task` variable is referenced in the `finally` block but may not be defined if an exception is raised before `loop.create_task` is called; initialize `task` to `None` before the `try` and guard the `task.cancel()`/`task.done()` checks accordingly.
- Calling `task.cancel()` from a worker thread can be unsafe; instead of directly cancelling the task from `start_client`, use `loop.call_soon_threadsafe(task.cancel)` to interact with the asyncio loop from another thread.

## Individual Comments

### Comment 1
<location path="astrbot/core/platform/sources/dingtalk/dingtalk_adapter.py" line_range="756-753" />
<code_context>
+        MAX_RETRIES = 5
+        retry_count = 0
+
         def start_client(loop: asyncio.AbstractEventLoop) -> None:
-            try:
-                self._shutdown_event = threading.Event()
-                task = loop.create_task(self.client_.start())
-                self._shutdown_event.wait()
-                if task.done():
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against `task` being referenced in `finally` before assignment if `create_task` raises.

If `loop.create_task(self.client_.start())` raises (e.g., due to a closing loop), control flows to `except` and then `finally`, where `task` is referenced without being defined, leading to an `UnboundLocalError` that masks the original failure. Consider initializing `task: asyncio.Task | None = None` before the `try` and guarding with `if task is not None and not task.done():` in `finally`.
</issue_to_address>

---

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

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[gemini-code-assist]

Code Review

This pull request implements a retry loop for the DingTalk adapter to handle unexpected client task exits. The review feedback identifies a potential deadlock where the thread could block indefinitely if the task crashes, a risk of UnboundLocalError for the task variable, and the need to handle CancelledError when retrieving task exceptions.

[Blueteemo]

@sourcery-ai review

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• The retry logic in start_client duplicates the same logging, counter increment, and time.sleep(10) handling in both the normal exception path and the task.exception() path; consider extracting a small helper to encapsulate the retry step and reduce complexity.
• Using time.sleep(10) inside the start_client loop blocks the worker thread entirely; if you expect this logic to evolve or run alongside other threaded work, consider using a non-blocking wait (e.g., another threading.Event().wait(timeout) or a configurable backoff) to keep it more flexible and testable.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The retry logic in `start_client` duplicates the same logging, counter increment, and `time.sleep(10)` handling in both the normal exception path and the `task.exception()` path; consider extracting a small helper to encapsulate the retry step and reduce complexity.
- Using `time.sleep(10)` inside the `start_client` loop blocks the worker thread entirely; if you expect this logic to evolve or run alongside other threaded work, consider using a non-blocking wait (e.g., another `threading.Event().wait(timeout)` or a configurable backoff) to keep it more flexible and testable.

## Individual Comments

### Comment 1
<location path="astrbot/core/platform/sources/dingtalk/dingtalk_adapter.py" line_range="756" />
<code_context>
+        MAX_RETRIES = 5
+        retry_count = 0
+
         def start_client(loop: asyncio.AbstractEventLoop) -> None:
-            try:
-                self._shutdown_event = threading.Event()
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the retry and shutdown handling into a small helper and a flat retry loop to simplify control flow and remove duplicated logic.

You can keep the same behavior while simplifying control flow, removing duplication, and avoiding `nonlocal` by:

- Classifying a single `client_.start()` run into a small result enum.
- Putting retry policy in a tight loop that uses that result.
- Centralizing the graceful-shutdown check and retry increment/sleep.

For example:

```python
def _is_graceful_shutdown(exc: BaseException) -> bool:
    return "Graceful shutdown" in str(exc)
```

Split “run once” from “retry loop” and keep retry state local:

```python
def _run_client_once(
    loop: asyncio.AbstractEventLoop,
    shutdown_event: threading.Event,
) -> Literal["ok", "graceful_shutdown", "error"]:
    task = loop.create_task(self.client_.start())
    task.add_done_callback(lambda _: shutdown_event.set())
    shutdown_event.wait()

    if not task.done():
        # 正常关闭路径：线程被唤醒但 task 仍在运行
        return "ok"

    try:
        exc = task.exception()
    except asyncio.CancelledError:
        logger.info("钉钉适配器 task 已取消")
        return "graceful_shutdown"

    if exc is None:
        return "ok"

    if _is_graceful_shutdown(exc):
        logger.info("钉钉适配器已被关闭")
        return "graceful_shutdown"

    logger.error(f"钉钉 SDK task 异常退出: {exc}")
    return "error"
```

Then the retry loop becomes a single, flat structure with no `nonlocal` and shared retry handling:

```python
def start_client(loop: asyncio.AbstractEventLoop) -> None:
    MAX_RETRIES = 5
    RETRY_DELAY = 10

    for attempt in range(1, MAX_RETRIES + 1):
        shutdown_event = threading.Event()
        task: asyncio.Task | None = None
        try:
            self._shutdown_event = shutdown_event
            result = _run_client_once(loop, shutdown_event)

            if result in ("ok", "graceful_shutdown"):
                return  # 正常退出或优雅关闭

            # result == "error": 统一的重试逻辑
            if attempt < MAX_RETRIES:
                logger.info(f"钉钉适配器尝试重连 ({attempt}/{MAX_RETRIES})...")
                time.sleep(RETRY_DELAY)
                continue

            logger.error("钉钉适配器重连失败，已达最大重试次数")
            return

        except Exception as e:
            if _is_graceful_shutdown(e):
                logger.info("钉钉适配器已被关闭")
                return

            logger.error(f"钉钉机器人启动失败: {e}")
            if attempt < MAX_RETRIES:
                logger.info(f"钉钉适配器尝试重连 ({attempt}/{MAX_RETRIES})...")
                time.sleep(RETRY_DELAY)
            else:
                logger.error("钉钉适配器重连失败，已达最大重试次数")
                return
        finally:
            if task is not None and not task.done():
                task.cancel()
```

Key benefits while preserving functionality:

- No `nonlocal` or shared `retry_count`.
- A single, explicit retry loop with one place handling retries.
- Graceful shutdown detection centralized in `_is_graceful_shutdown`.
- Task lifecycle and classification confined to `_run_client_once`, making it easier to reason about all exit paths.
</issue_to_address>

---

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

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[Blueteemo]

您好，关于重试逻辑的说明：

1. SDK 内部已有 while True 重连循环，正常断连会自动重连
2. 我们的重试是针对 SDK task 意外崩溃的兜底机制（如未捕获的异常）
3. MAX_RETRIES = 5 足够处理瞬时问题（如网络波动）
4. 如果 SDK 运行 12 小时后崩溃，重试计数器会重置（因为之前运行成功）

当前方案是合理的，不需要改为无限重试。