fix: avoid lsp client cancel scope leaks#6961
Merged
Conversation
dosubot
bot
added
the
size:M
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
dosubot
bot
added
the
area:core
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request migrates the LSP client's background task management from anyio.TaskGroup to asyncio.Task for improved integration with asyncio. This includes updating the connect_to_server and shutdown methods to use asyncio.create_task and handle asyncio.CancelledError respectively. The integration tests were also refactored to use pathlib for path handling and sys.executable for running the server, and a new test was added to ensure proper cancellation behavior within anyio scopes. The review suggests updating the _read_responses method to consistently catch asyncio.CancelledError and to update an outdated comment to reflect the change in task management.
Comment on lines
+217
to
+223
| if self._reader_task: | ||
| self._reader_task.cancel() | ||
| try: | ||
| # Exit the TaskGroup, which cancels background tasks started within it | ||
| await self._task_group.__aexit__(None, None, None) | ||
| except anyio.get_cancelled_exc_class(): | ||
| await self._reader_task | ||
| except asyncio.CancelledError: | ||
| pass | ||
| self._task_group = None | ||
| self._reader_task = None |
Contributor
There was a problem hiding this comment.
While this correctly cancels and awaits the asyncio.Task, for consistency with the move to asyncio.Task for background work, the _read_responses method should also be updated.
Currently, _read_responses catches anyio.get_cancelled_exc_class(). It would be clearer and more consistent to explicitly catch asyncio.CancelledError there, since you are now using asyncio.create_task.
Additionally, the comment within that except block in _read_responses is now outdated and should be updated to reflect that cancellation happens via task.cancel() not a TaskGroup.
For example:
# in _read_responses()
...
except asyncio.CancelledError:
# Task was cancelled during shutdown.
pass![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
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/_internal/protocols/lsp/client.py" line_range="39" />
<code_context>
self._server_command: list[str] | None = None
- # anyio TaskGroup handle for background readers
- self._task_group: Any | None = None
+ self._reader_task: asyncio.Task[None] | None = None
@property
</code_context>
<issue_to_address>
**issue (bug_risk):** Background task exceptions are now effectively unobserved, changing failure semantics vs the previous TaskGroup usage.
With the previous `anyio.TaskGroup`, exceptions in `_read_responses` would propagate (e.g. via `__aexit__`), making failures visible to the caller. Using `asyncio.create_task`, exceptions will now only be logged by the event loop and ignored by this client. If failures in the reader should still drive client behavior (e.g. mark as disconnected, trigger reconnect/cleanup), attach a `done_callback` to `self._reader_task` that checks `task.exception()` and handles it appropriately.
</issue_to_address>
### Comment 2
<location path="tests/integration/test_lsp_integration.py" line_range="21-22" />
<code_context>
+SERVER_PATH = TEST_DIR / "fixtures" / "echo_lsp_server.py"
+
@pytest.mark.anyio
async def test_lsp_client_initialization():
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test case where the fail_after timeout actually fires to prove cancel scopes are not corrupted on cancellation.
The current test only covers the case where `fail_after` does not expire. Since the original issue was cancel scope corruption, please add a regression test where the `fail_after` timeout is actually hit (e.g., very small timeout or a non-responding command) and then assert that subsequent `fail_after` scopes still behave correctly. This will confirm there’s no lingering cancellation corruption after a cancelled connect/shutdown.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
dosubot
bot
added
size:L
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The switch from an AnyIO TaskGroup to
asyncio.create_taskfor_reader_taskties the client implementation to the asyncio backend; if this client is intended to work under other AnyIO backends (e.g. Trio), consider using AnyIO’s task APIs (e.g. a dedicated background task group) instead of rawasyncioprimitives to avoid backend-specific breakage.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The switch from an AnyIO TaskGroup to `asyncio.create_task` for `_reader_task` ties the client implementation to the asyncio backend; if this client is intended to work under other AnyIO backends (e.g. Trio), consider using AnyIO’s task APIs (e.g. a dedicated background task group) instead of raw `asyncio` primitives to avoid backend-specific breakage.
## Individual Comments
### Comment 1
<location path="astrbot/_internal/protocols/lsp/client.py" line_range="233-242" />
<code_context>
self._connected = False
- if self._task_group:
+ if self._reader_task:
+ self._reader_task.cancel()
try:
- # Exit the TaskGroup, which cancels background tasks started within it
- await self._task_group.__aexit__(None, None, None)
- except anyio.get_cancelled_exc_class():
+ await self._reader_task
+ except asyncio.CancelledError:
pass
- self._task_group = None
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid re-raising non-cancellation reader errors from `shutdown` after they’ve already been handled.
When `_read_responses` raises, `_handle_reader_task_done` already logs the error and sets `_connected = False`. Later, `await self._reader_task` in `shutdown` will re-raise that same exception, since only `CancelledError` is caught, causing `shutdown` to fail even though the error was already handled.
If `shutdown` is meant to be best-effort and not fail due to a previously handled reader error, consider broadening the exception handling, e.g.
```python
if self._reader_task:
self._reader_task.cancel()
try:
await self._reader_task
except asyncio.CancelledError:
pass
except Exception:
# optionally log at debug level or ignore, since it was already logged
pass
self._reader_task = None
```
This preserves error visibility via the done callback while preventing it from escaping `shutdown`.
```suggestion
"""Shutdown the LSP client."""
self._connected = False
if self._reader_task:
self._reader_task.cancel()
try:
await self._reader_task
except asyncio.CancelledError:
# Expected when we cancel the reader task during shutdown.
pass
except Exception:
# Any other exception from the reader task has already been
# handled and logged in _handle_reader_task_done, so we
# deliberately ignore it here to keep shutdown best-effort.
pass
self._reader_task = None
```
</issue_to_address>
### Comment 2
<location path="tests/unit/test_internal/test_lsp_client.py" line_range="32-33" />
<code_context>
+ patch.object(client, "send_notification", AsyncMock()),
+ patch("astrbot._internal.protocols.lsp.client.log") as mock_log,
+ ):
+ await client.connect_to_server(["python", "fake_lsp.py"], "file:///tmp")
+ await asyncio.sleep(0)
+
+ reader_task = client._reader_task
</code_context>
<issue_to_address>
**suggestion (testing):** Avoid relying on `asyncio.sleep(0)` scheduling for the reader-task behavior if possible.
Using `await asyncio.sleep(0)` here to let the reader task finish makes the test timing‑dependent and potentially flaky under load.
Instead, consider either:
- Calling `_handle_reader_task_done` directly with a completed/fake task, or
- After `connect_to_server`, capturing `reader_task = client._reader_task` and `await asyncio.wait_for(reader_task, timeout=1)` before asserting.
This will make the test more deterministic and robust against scheduling quirks.
Suggested implementation:
```python
patch.object(client, "send_notification", AsyncMock()),
patch("astrbot._internal.protocols.lsp.client.log") as mock_log,
):
await client.connect_to_server(["python", "fake_lsp.py"], "file:///tmp")
reader_task = client._reader_task
assert reader_task is not None
# Ensure the reader task has finished and propagated its failure
with pytest.raises(RuntimeError, match="reader crashed"):
await asyncio.wait_for(reader_task, timeout=1)
assert client.connected is False
mock_log.error.assert_called_once()
```
If `pytest` is not already imported at the top of `tests/unit/test_internal/test_lsp_client.py`, add:
- `import pytest`
If `asyncio` is not yet imported in this test file, add:
- `import asyncio`
</issue_to_address>
### Comment 3
<location path="tests/integration/test_lsp_integration.py" line_range="23-24" />
<code_context>
+HANGING_SERVER_PATH = TEST_DIR / "fixtures" / "hanging_lsp_server.py"
+
@pytest.mark.anyio
async def test_lsp_client_initialization():
</code_context>
<issue_to_address>
**suggestion (testing):** Make the final `fail_after` scope in the timeout regression test assert something observable.
To better capture the regression this is guarding against, the last `with anyio.fail_after(1):` should actually await something that would fail if the scope were corrupted (e.g. `await checkpoint()` or `await anyio.sleep(0.01)`). You could also add a trivial assertion afterward to make it explicit that the scope completes normally, so the test clearly verifies that later `fail_after` scopes remain usable post-timeout, not just that shutdown succeeds.
Suggested implementation:
```python
async with anyio.fail_after(1):
# Ensure the new fail_after scope remains functional after the earlier timeout.
# If the scope were corrupted, this checkpoint could hang and trigger the timeout.
await checkpoint()
await client.shutdown()
# Make it explicit that the final fail_after scope completed without timing out.
assert True
```
This edit assumes the timeout regression test already has an `async with anyio.fail_after(1):` block that only calls `await client.shutdown()`. If the exact code differs (e.g., different indentation, extra statements in the block, or a different context variable name), you should adjust the SEARCH block to match the existing code exactly so the replacement applies cleanly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
dosubot
bot
added
size:XL
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The switch from an AnyIO TaskGroup to
asyncio.create_taskinAstrbotLspClientcouples the client to the asyncio backend; if you intend to keep AnyIO backend flexibility (e.g., Trio), consider usinganyio.create_task_groupwith a non-leaking lifecycle or an AnyIO-native background task helper instead of raw asyncio tasks. - In
connect_to_server, a new_reader_taskis created unconditionally; ifconnect_to_servercan be called more than once during the lifetime of an instance, you may want to guard against or explicitly cancel any existing_reader_taskto avoid orphaned tasks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The switch from an AnyIO TaskGroup to `asyncio.create_task` in `AstrbotLspClient` couples the client to the asyncio backend; if you intend to keep AnyIO backend flexibility (e.g., Trio), consider using `anyio.create_task_group` with a non-leaking lifecycle or an AnyIO-native background task helper instead of raw asyncio tasks.
- In `connect_to_server`, a new `_reader_task` is created unconditionally; if `connect_to_server` can be called more than once during the lifetime of an instance, you may want to guard against or explicitly cancel any existing `_reader_task` to avoid orphaned tasks.
## Individual Comments
### Comment 1
<location path="astrbot/_internal/protocols/lsp/client.py" line_range="46-62" />
<code_context>
"""True if connected to an LSP server."""
return self._connected
+ def _handle_reader_task_done(self, task: asyncio.Task[None]) -> None:
+ try:
+ exception = task.exception()
+ except asyncio.CancelledError:
+ return
+
+ if exception is None:
+ if self._connected:
+ self._connected = False
+ log.warning("LSP reader task exited unexpectedly")
+ return
+
+ self._connected = False
+ log.error(
+ "LSP reader task failed",
+ exc_info=(type(exception), exception, exception.__traceback__),
</code_context>
<issue_to_address>
**suggestion:** Consider simplifying error logging for the reader task and ensuring the attribute state stays consistent.
`_handle_reader_task_done` manually passes `(type(exception), exception, exception.__traceback__)` to `exc_info`; using `exc_info=exception` or `exc_info=True` is more idiomatic and less error-prone. Also consider clearing `self._reader_task` when the task completes (successfully or with error) so callers don’t see a stale completed task and it’s clear the handle isn’t reused across connections.
```suggestion
def _handle_reader_task_done(self, task: asyncio.Task[None]) -> None:
# Clear the reader task handle so callers don't see a stale completed task.
if self._reader_task is task:
self._reader_task = None
try:
exception = task.exception()
except asyncio.CancelledError:
return
if exception is None:
if self._connected:
self._connected = False
log.warning("LSP reader task exited unexpectedly")
return
self._connected = False
# Let the logging framework capture the traceback from the exception.
log.error("LSP reader task failed", exc_info=exception)
```
</issue_to_address>
### Comment 2
<location path="docs/zh/dev/star/plugin.md" line_range="1486" />
<code_context>
- `persona_id: str` – 待删除的人格 ID
-- __Raises__
+- __Raises__
`Valueable` – 若 `persona_id` 不存在
#### `get_default_persona_v3`
</code_context>
<issue_to_address>
**issue (typo):** Exception type `Valueable` looks like a typo and should likely be `ValueError` for consistency.
Nearby, `create_persona`/`update_persona` both document `ValueError`, while `delete_persona` uses `Valueable`. Please change this to `ValueError` so the documented raised exception is accurate and consistent.
</issue_to_address>
### Comment 3
<location path="docs/zh/platform/kook.md" line_range="25" />
<code_context>
-1. 点击跳转 [Kook 开发者平台] ，完成以下步骤：
-2. 登录账号并完成实名认证；
-3. 点击「新建应用」，自定义 Bot 昵称；
-4. 进入应用后台，选择「机器人」模块，开启 **WebSocket 连接模式**，注意保存生成的 **Token**，后续配置Astrbot需要使用；
+1. 点击跳转 [Kook 开发者平台] ，完成以下步骤：
+2. 登录账号并完成实名认证；
</code_context>
<issue_to_address>
**suggestion (typo):** Project name `Astrbot` should be `AstrBot`, and adding a space before it would improve consistency.
Most references use `AstrBot` with a capital “B”, and Chinese copy usually inserts a space before an English product name. Please update this instance for consistent branding and readability.
```suggestion
4. 进入应用后台，选择「机器人」模块，开启 **WebSocket 连接模式**，注意保存生成的 **Token**，后续配置 AstrBot 需要使用；
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
test_lsp_client_connect_timeout_does_not_corrupt_anyio_cancel_scope, the finalwith anyio.fail_after(1):context manager has no body, which will cause a syntax error or at least a no-op block; either remove it or add the intended assertion/operation inside. - In
docs/zh/dev/star/guides/ai.md, thedelete_personasection still documentsValueableinstead ofValueError, which is inconsistent with the other docs and the actual exception type. - The new unit tests for the LSP client directly access the private
_reader_taskattribute; consider asserting behavior via public APIs (e.g., connection state, logging, or shutdown behavior) to reduce coupling to the internal implementation details ofAstrbotLspClient.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `test_lsp_client_connect_timeout_does_not_corrupt_anyio_cancel_scope`, the final `with anyio.fail_after(1):` context manager has no body, which will cause a syntax error or at least a no-op block; either remove it or add the intended assertion/operation inside.
- In `docs/zh/dev/star/guides/ai.md`, the `delete_persona` section still documents `Valueable` instead of `ValueError`, which is inconsistent with the other docs and the actual exception type.
- The new unit tests for the LSP client directly access the private `_reader_task` attribute; consider asserting behavior via public APIs (e.g., connection state, logging, or shutdown behavior) to reduce coupling to the internal implementation details of `AstrbotLspClient`.
## Individual Comments
### Comment 1
<location path="astrbot/_internal/protocols/lsp/client.py" line_range="46-51" />
<code_context>
"""True if connected to an LSP server."""
return self._connected
+ async def _stop_reader_task(self) -> None:
+ reader_task = self._reader_task
+ if reader_task is None:
+ return
+ self._reader_task = None
+ reader_task.cancel()
+ try:
+ await reader_task
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid awaiting the reader task from within the reader itself to prevent potential deadlocks.
If `shutdown()` / `_stop_reader_task()` can be called from code running inside `_read_responses()` (e.g., via an LSP notification handled on the reader task), `await self._stop_reader_task()` would effectively await the currently running task and deadlock. To avoid this, either (a) check `asyncio.current_task()` and skip awaiting when stopping yourself, or (b) change shutdown so it only signals cancellation here and defers awaiting the reader task to an outer coordinator task.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
_stop_reader_task, when called from inside the reader itself (as intest_lsp_stop_reader_task_does_not_await_current_task),await reader_taskwill end up awaiting the current task and raise aRuntimeError; add a guard to skip awaiting whenreader_task is asyncio.current_task()to avoid self-await deadlocks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_stop_reader_task`, when called from inside the reader itself (as in `test_lsp_stop_reader_task_does_not_await_current_task`), `await reader_task` will end up awaiting the current task and raise a `RuntimeError`; add a guard to skip awaiting when `reader_task is asyncio.current_task()` to avoid self-await deadlocks.
## Individual Comments
### Comment 1
<location path="astrbot/_internal/protocols/lsp/client.py" line_range="46-54" />
<code_context>
"""True if connected to an LSP server."""
return self._connected
+ async def _stop_reader_task(self) -> None:
+ reader_task = self._reader_task
+ if reader_task is None:
+ return
+ self._reader_task = None
+ reader_task.cancel()
+ try:
+ await reader_task
+ except asyncio.CancelledError:
+ pass
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid propagating reader task exceptions from `_stop_reader_task`, which can make `shutdown`/reconnect paths unexpectedly fail.
Since `_handle_reader_task_done` already handles/logs reader failures, re-raising them here makes `shutdown()` / reconnect brittle. If this method is intended as best-effort cleanup, consider swallowing non-cancellation errors so teardown paths don’t fail due to prior reader crashes:
```py
async def _stop_reader_task(self) -> None:
reader_task = self._reader_task
if reader_task is None:
return
self._reader_task = None
reader_task.cancel()
try:
await reader_task
except asyncio.CancelledError:
pass
except Exception:
# optionally log here, or rely on _handle_reader_task_done
pass
```
This keeps behavior closer to the previous TaskGroup-based teardown, where internal reader errors didn’t surface during shutdown.
</issue_to_address>
### Comment 2
<location path="astrbot/_internal/protocols/lsp/client.py" line_range="111-113" />
<code_context>
- self._task_group = anyio.create_task_group()
- await self._task_group.__aenter__()
- self._task_group.start_soon(self._read_responses)
+ # Start reading responses in the background.
+ self._reader_task = asyncio.create_task(self._read_responses())
+ self._reader_task.add_done_callback(self._handle_reader_task_done)
# Send initialize request
</code_context>
<issue_to_address>
**question (bug_risk):** Using `asyncio.create_task` directly changes the concurrency assumptions compared to the previous anyio TaskGroup usage.
The old `anyio.create_task_group()` approach worked across anyio backends (including trio). Using `asyncio.create_task` now assumes an active asyncio event loop and will break in pure-anyio/trio contexts.
If this client is only ever used in asyncio-native code, this is acceptable. Otherwise, consider keeping task creation on anyio primitives (e.g. a TaskGroup) or otherwise guaranteeing that `asyncio.create_task` is safe in all supported environments, since this change narrows where the client can be used.
</issue_to_address>
### Comment 3
<location path="astrbot/_internal/protocols/lsp/client.py" line_range="113" />
<code_context>
- self._task_group.start_soon(self._read_responses)
+ # Start reading responses in the background.
+ self._reader_task = asyncio.create_task(self._read_responses())
+ self._reader_task.add_done_callback(self._handle_reader_task_done)
# Send initialize request
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the reader task lifecycle by centralizing shutdown and error-handling logic inside `_read_responses` and `_stop_reader_task` and removing the `add_done_callback` path.
You can keep the new asyncio-based reader task while simplifying lifecycle management by localizing the logic and removing the callback.
### 1. Let `_read_responses` own its shutdown semantics
Move the “what happens when the reader exits” logic into `_read_responses` itself, so you don’t need `_handle_reader_task_done`:
```python
async def _read_responses(self) -> None:
try:
# existing read loop
async for message in self._reader: # or whatever your loop is
...
except asyncio.CancelledError:
# Normal shutdown; don't log as error
raise
except Exception as exc:
log.error("LSP reader task failed", exc_info=exc)
# You may want to signal failures to the rest of the client here.
else:
if self._connected:
log.warning("LSP reader task exited unexpectedly")
finally:
# Centralize state mutation here
if self._connected:
self._connected = False
if self._reader_task is asyncio.current_task():
self._reader_task = None
```
Now you don’t need `add_done_callback` or a separate `_handle_reader_task_done`.
### 2. Manage the task only from `connect_to_server` / `shutdown`
Keep `_stop_reader_task` but make it the only mechanism to cancel/await the reader, and remove the callback:
```python
async def _stop_reader_task(self) -> None:
task = self._reader_task
if task is None:
return
self._reader_task = None
task.cancel()
try:
await task
except asyncio.CancelledError:
pass
```
```python
async def connect_to_server(self, command: list[str], workspace_uri: str) -> None:
log.debug(f"Starting LSP server: {' '.join(command)}")
await self._stop_reader_task()
self._server_process = await anyio.open_process(
command,
stdin=-1,
stdout=-1,
stderr=-1,
)
self._reader = self._server_process.stdout
self._writer = self._server_process.stdin
self._server_command = command
self._connected = True
# Start reading responses in the background.
self._reader_task = asyncio.create_task(self._read_responses())
```
```python
async def shutdown(self) -> None:
self._connected = False
await self._stop_reader_task()
if self._server_process:
...
```
This keeps all existing behavior (background reader, cancellation on reconnect/shutdown) but removes the cross-cutting `done_callback` path and centralizes state changes and error handling into `_read_responses` and `_stop_reader_task`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+111
to
+113
| # Start reading responses in the background. | ||
| self._reader_task = asyncio.create_task(self._read_responses()) | ||
| self._reader_task.add_done_callback(self._handle_reader_task_done) |
Contributor
There was a problem hiding this comment.
question (bug_risk): Using asyncio.create_task directly changes the concurrency assumptions compared to the previous anyio TaskGroup usage.
The old anyio.create_task_group() approach worked across anyio backends (including trio). Using asyncio.create_task now assumes an active asyncio event loop and will break in pure-anyio/trio contexts.
If this client is only ever used in asyncio-native code, this is acceptable. Otherwise, consider keeping task creation on anyio primitives (e.g. a TaskGroup) or otherwise guaranteeing that asyncio.create_task is safe in all supported environments, since this change narrows where the client can be used.
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
_reader_tasklifecycle is managed withasyncio.Task/asyncio.create_taskinside an AnyIO-based client; if this client can run under non-asyncio AnyIO backends (e.g. Trio), consider usinganyio.create_task_grouporstart_task_sooninstead to keep compatibility with alternate event loops. _stop_reader_taskcurrently swallows all non-cancellation exceptions with a bareexcept Exception: pass; it would be helpful to at least log these failures so unexpected reader teardown errors are not completely hidden during debugging.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `_reader_task` lifecycle is managed with `asyncio.Task`/`asyncio.create_task` inside an AnyIO-based client; if this client can run under non-asyncio AnyIO backends (e.g. Trio), consider using `anyio.create_task_group` or `start_task_soon` instead to keep compatibility with alternate event loops.
- `_stop_reader_task` currently swallows all non-cancellation exceptions with a bare `except Exception: pass`; it would be helpful to at least log these failures so unexpected reader teardown errors are not completely hidden during debugging.
## Individual Comments
### Comment 1
<location path="tests/unit/test_internal/test_lsp_client.py" line_range="31-37" />
<code_context>
+ patch.object(client, "send_notification", AsyncMock()),
+ patch("astrbot._internal.protocols.lsp.client.log") as mock_log,
+ ):
+ await client.connect_to_server(["python", "fake_lsp.py"], "file:///tmp")
+ await asyncio.sleep(0)
+ await asyncio.sleep(0)
</code_context>
<issue_to_address>
**suggestion (testing):** Relying on `asyncio.sleep(0)` twice to let the reader task finish can be slightly flaky; consider a more explicit synchronization point.
In `test_lsp_reader_task_failure_marks_client_disconnected_and_logs`, those two `await asyncio.sleep(0)` calls are acting as a timing hack so the reader task and its done callback can run. To avoid timing‑sensitive flakiness, consider waiting on an explicit signal instead—for example, have the mocked `_read_responses` set an `asyncio.Event` before raising and await that here, or wait (with a short timeout) until either `client.connected is False` or `mock_log.error.called` is true. This will make the test more reliable in slower CI environments.
```suggestion
):
await client.connect_to_server(["python", "fake_lsp.py"], "file:///tmp")
# Wait explicitly for the reader task failure to be handled instead of relying on
# scheduling via repeated `asyncio.sleep(0)` calls.
for _ in range(100):
if client.connected is False and mock_log.error.called:
break
await asyncio.sleep(0.01)
else:
pytest.fail(
"Timed out waiting for LSP reader task failure to disconnect client and log error"
)
assert client.connected is False
mock_log.error.assert_called_once()
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+31
to
+37
| ): | ||
| await client.connect_to_server(["python", "fake_lsp.py"], "file:///tmp") | ||
| await asyncio.sleep(0) | ||
| await asyncio.sleep(0) | ||
|
|
||
| assert client.connected is False | ||
| mock_log.error.assert_called_once() |
Contributor
There was a problem hiding this comment.
suggestion (testing): Relying on asyncio.sleep(0) twice to let the reader task finish can be slightly flaky; consider a more explicit synchronization point.
In test_lsp_reader_task_failure_marks_client_disconnected_and_logs, those two await asyncio.sleep(0) calls are acting as a timing hack so the reader task and its done callback can run. To avoid timing‑sensitive flakiness, consider waiting on an explicit signal instead—for example, have the mocked _read_responses set an asyncio.Event before raising and await that here, or wait (with a short timeout) until either client.connected is False or mock_log.error.called is true. This will make the test more reliable in slower CI environments.
Suggested change
| ): | |
| await client.connect_to_server(["python", "fake_lsp.py"], "file:///tmp") | |
| await asyncio.sleep(0) | |
| await asyncio.sleep(0) | |
| assert client.connected is False | |
| mock_log.error.assert_called_once() | |
| ): | |
| await client.connect_to_server(["python", "fake_lsp.py"], "file:///tmp") | |
| # Wait explicitly for the reader task failure to be handled instead of relying on | |
| # scheduling via repeated `asyncio.sleep(0)` calls. | |
| for _ in range(100): | |
| if client.connected is False and mock_log.error.called: | |
| break | |
| await asyncio.sleep(0.01) | |
| else: | |
| pytest.fail( | |
| "Timed out waiting for LSP reader task failure to disconnect client and log error" | |
| ) | |
| assert client.connected is False | |
| mock_log.error.assert_called_once() |
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/_internal/protocols/lsp/client.py" line_range="46" />
<code_context>
"""True if connected to an LSP server."""
return self._connected
+ async def _stop_reader_task(self) -> None:
+ reader_task = self._reader_task
+ if reader_task is None:
</code_context>
<issue_to_address>
**issue (complexity):** Consider centralizing reader task lifecycle, error handling, and `_connected` state transitions so that `_read_responses`, `_stop_reader_task`, and the done callback each have a single, simple responsibility.
You can keep the new `asyncio.Task` behavior but reduce lifecycle complexity by:
1. Letting `_read_responses` own error handling and `_connected` transitions.
2. Making `_stop_reader_task` responsible only for cancelling/awaiting and clearing the task.
3. Making the done callback only clear the reference (no state changes / logging).
This removes duplicated logic and the “who flips `_connected` when” confusion.
Example refactor:
```python
import asyncio
import contextlib
class AstrbotLspClient(BaseAstrbotLspClient):
...
async def _read_responses(self) -> None:
try:
# existing body of _read_responses goes here
...
except asyncio.CancelledError:
# Normal shutdown path
raise
except Exception as exc:
# Centralized error handling & state transition
if self._connected:
self._connected = False
log.error("LSP reader task failed", exc_info=exc)
# swallow exception so it doesn't surface as an unhandled task error
return
else:
# Reader exited without being cancelled
if self._connected:
self._connected = False
log.warning("LSP reader task exited unexpectedly")
async def _stop_reader_task(self) -> None:
task = self._reader_task
if task is None or task.done():
self._reader_task = None
return
self._reader_task = None
task.cancel()
with contextlib.suppress(asyncio.CancelledError):
await task
def _handle_reader_task_done(self, task: asyncio.Task[None]) -> None:
# Only clear the reference; logging and _connected changes
# are centralized in _read_responses.
if self._reader_task is task:
self._reader_task = None
```
`connect_to_server` and `shutdown` stay the same structurally:
```python
async def connect_to_server(...):
...
await self._stop_reader_task()
...
self._connected = True
self._reader_task = asyncio.create_task(self._read_responses())
self._reader_task.add_done_callback(self._handle_reader_task_done)
async def shutdown(self) -> None:
self._connected = False
await self._stop_reader_task()
...
```
This way:
- Task lifecycle is encoded in one place (`_stop_reader_task`).
- `_read_responses` is the single owner of reader-related logging and `_connected` transitions.
- The callback becomes trivial and no longer duplicates teardown/error semantics.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="docs/zh/dev/plugin-platform-adapter.md" line_range="72" />
<code_context>
self.settings = platform_settings # platform_settings 平台设置。
-
+
async def send_by_session(self, session: MessageSesion, message_chain: MessageChain):
# 必须实现
await super().send_by_session(session, message_chain)
</code_context>
<issue_to_address>
**issue (typo):** `MessageSesion` 似乎是 `MessageSession` 的拼写错误
类型注解应为 `MessageSession`，否则示例代码会延续错误的类型名，建议改成正确拼写。
Suggested implementation:
```
async def send_by_session(self, session: MessageSession, message_chain: MessageChain):
```
1. 在该文档中如果还有其他地方使用了 `MessageSesion`，也应统一改为 `MessageSession`。
2. 确保示例代码所在的完整片段中，有相应的 `MessageSession` 类型导入或定义（例如 `from astrbot.core import MessageSession` 或项目中的实际导入路径），以避免读者复制示例代码时出现未定义类型错误。
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
test_lsp_client_connect_timeout_does_not_corrupt_anyio_cancel_scopethe finalwith anyio.fail_after(1):context manager has no body, which will raise a syntax error or be caught by linters—either remove it or add an explicit no-op (e.g., a comment orpass) inside the block.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `test_lsp_client_connect_timeout_does_not_corrupt_anyio_cancel_scope` the final `with anyio.fail_after(1):` context manager has no body, which will raise a syntax error or be caught by linters—either remove it or add an explicit no-op (e.g., a comment or `pass`) inside the block.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
Contributor
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/_internal/protocols/lsp/client.py" line_range="59-68" />
<code_context>
+ await reader_task
+ except asyncio.CancelledError:
+ pass
+ except Exception as exc:
+ log.debug("Ignoring failed LSP reader task during teardown", exc_info=exc)
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Failed or early-exiting reader leaves pending requests unresolved, which might be worth handling explicitly.
If `_read_responses` fails, `_connected` is set to `False` and we log, but any futures in `_pending_requests` are left unresolved, so callers can hang indefinitely waiting for responses. It would be safer to walk `_pending_requests` here and complete them with a failure (e.g., a custom exception) so callers see a clear error instead of hanging when the reader task dies.
Suggested implementation:
```python
import asyncio
import json
from typing import Any
class LspConnectionError(Exception):
"""Raised when the LSP connection fails or is closed unexpectedly."""
self._pending_requests: dict[int, Any] = {}
self._request_id = 0
self._server_command: list[str] | None = None
```
```python
try:
await reader_task
except asyncio.CancelledError:
pass
except Exception as exc:
log.debug("Ignoring failed LSP reader task during teardown", exc_info=exc)
# Fail all pending requests so callers don't hang waiting for responses
error = LspConnectionError("LSP reader task failed; the connection is closing")
for future in self._pending_requests.values():
if not future.done():
future.set_exception(error)
self._pending_requests.clear()
```
These changes assume:
1. `self._pending_requests` stores `asyncio.Future`-like objects (with `.done()` and `.set_exception()`), which is consistent with typical LSP client implementations.
2. The `log` object is already defined/imported elsewhere in this module (it’s being used in the existing `log.debug(...)` call). If not, you’ll need to add an appropriate logger definition/import.
If there are other code paths where the connection or reader is torn down (e.g., explicit `close()`/`shutdown()` methods), you may also want to reuse `LspConnectionError` there to keep behavior consistent for pending requests.
</issue_to_address>
### Comment 2
<location path="tests/integration/fixtures/hanging_lsp_server.py" line_range="8-9" />
<code_context>
+import time
+
+
+def main() -> None:
+ time.sleep(60)
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** The 60-second sleep in the hanging LSP server fixture might be longer than necessary and could prolong failures if timeouts are misconfigured.
For `test_lsp_client_connect_timeout_does_not_corrupt_anyio_cancel_scope`, it’s enough for the server to be slower than the `fail_after(0.1)` timeout, not to block for a full minute. Please reduce this (e.g., `time.sleep(5)` or lower) so that misconfigured timeouts or process cleanup don’t cause the suite to appear hung for so long.
```suggestion
def main() -> None:
time.sleep(5)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+59
to
68
| except Exception as exc: | ||
| log.debug("Ignoring failed LSP reader task during teardown", exc_info=exc) | ||
|
|
||
| def _handle_reader_task_done(self, task: asyncio.Task[None]) -> None: | ||
| if self._reader_task is task: | ||
| self._reader_task = None | ||
|
|
||
| async def connect(self) -> None: | ||
| """ | ||
| Connect to configured LSP servers. |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Failed or early-exiting reader leaves pending requests unresolved, which might be worth handling explicitly.
If _read_responses fails, _connected is set to False and we log, but any futures in _pending_requests are left unresolved, so callers can hang indefinitely waiting for responses. It would be safer to walk _pending_requests here and complete them with a failure (e.g., a custom exception) so callers see a clear error instead of hanging when the reader task dies.
Suggested implementation:
import asyncio
import json
from typing import Any
class LspConnectionError(Exception):
"""Raised when the LSP connection fails or is closed unexpectedly."""
self._pending_requests: dict[int, Any] = {}
self._request_id = 0
self._server_command: list[str] | None = None try:
await reader_task
except asyncio.CancelledError:
pass
except Exception as exc:
log.debug("Ignoring failed LSP reader task during teardown", exc_info=exc)
# Fail all pending requests so callers don't hang waiting for responses
error = LspConnectionError("LSP reader task failed; the connection is closing")
for future in self._pending_requests.values():
if not future.done():
future.set_exception(error)
self._pending_requests.clear()These changes assume:
self._pending_requestsstoresasyncio.Future-like objects (with.done()and.set_exception()), which is consistent with typical LSP client implementations.- The
logobject is already defined/imported elsewhere in this module (it’s being used in the existinglog.debug(...)call). If not, you’ll need to add an appropriate logger definition/import.
If there are other code paths where the connection or reader is torn down (e.g., explicit close()/shutdown() methods), you may also want to reuse LspConnectionError there to keep behavior consistent for pending requests.
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
test_lsp_client_connect_timeout_does_not_corrupt_anyio_cancel_scope, the finalwith anyio.fail_after(1):block has no body (andcheckpointis imported but unused), which will either fail linting or not actually assert anything about later cancel scopes—consider adding a minimal await/assert inside or removing the block and the unused import.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `test_lsp_client_connect_timeout_does_not_corrupt_anyio_cancel_scope`, the final `with anyio.fail_after(1):` block has no body (and `checkpoint` is imported but unused), which will either fail linting or not actually assert anything about later cancel scopes—consider adding a minimal await/assert inside or removing the block and the unused import.
## Individual Comments
### Comment 1
<location path="docs/en/dev/star/guides/ai.md" line_range="441" />
<code_context>
+- **Returns**
`Personality` – Default persona object in v3 format
::: details Persona / Personality 类型定义
</code_context>
<issue_to_address>
**suggestion (typo):** Consider translating the Chinese text `类型定义` in this English doc block title to keep the language consistent.
You could rename this to something like `::: details Persona / Personality type definitions` to keep the doc fully in English and avoid confusing readers.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/_internal/protocols/lsp/client.py" line_range="46" />
<code_context>
"""True if connected to an LSP server."""
return self._connected
+ async def _stop_reader_task(self) -> None:
+ reader_task = self._reader_task
+ if reader_task is None:
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the reader task lifecycle by removing the done-callback, centralizing disconnect state changes, and optionally encapsulating start/stop logic into small helpers.
You can keep the new behavior (asyncio task, better logging) but simplify the lifecycle surface area.
### 1. Drop the done-callback and simplify `_stop_reader_task`
You don’t really need `_handle_reader_task_done`, and `_stop_reader_task` can be linear:
```python
async def _stop_reader_task(self) -> None:
reader_task = self._reader_task
if reader_task is None:
return
self._reader_task = None
if not reader_task.done():
reader_task.cancel()
try:
await reader_task
except asyncio.CancelledError:
# Normal during shutdown
pass
except Exception as exc:
log.debug("Ignoring failed LSP reader task during teardown", exc_info=exc)
```
Then in `connect_to_server`:
```python
self._reader_task = asyncio.create_task(self._read_responses())
```
…and you can delete `_handle_reader_task_done` entirely.
This keeps the same semantics (cancel + await + ignore teardown errors) with fewer branches and no cross-coupled callback.
### 2. Centralize disconnect logic in `_read_responses`
The current `except` / `else` both manipulate `self._connected`. You can factor that into a helper, or at least a single place in `finally`:
```python
async def _read_responses(self) -> None:
try:
# existing read loop ...
...
except asyncio.CancelledError:
raise
except Exception as exc:
log.error("LSP reader task failed", exc_info=exc)
raise
else:
log.warning("LSP reader task exited unexpectedly")
finally:
if self._connected:
self._connected = False
```
If you want to preserve different logging for “error vs clean exit” but still ensure single-point state change, this keeps the same behavior while making the connection-state transition easier to reason about.
### 3. Optional: a small `_start_reader_task` helper
If you find yourself needing to restart the reader in other places, encapsulate the pattern:
```python
async def _start_reader_task(self) -> None:
await self._stop_reader_task()
self._reader_task = asyncio.create_task(self._read_responses())
```
Then `connect_to_server` becomes:
```python
await self._start_reader_task()
```
This keeps all reader lifecycle logic in two methods (`_start_reader_task` / `_stop_reader_task`) instead of dispersing it across callbacks and call sites.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new reader lifecycle is tied directly to
asyncio.Taskandasyncio.create_task, which makesAstrbotLspClienteffectively asyncio-only; if you intend to keep AnyIO backend flexibility, consider using an internalanyio.create_task_group()oranyio.lowlevel.spawn_taskinstead of hard-wiring asyncio primitives. - In
tests/integration/test_lsp_integration.py, the importedcheckpointfromanyio.lowlevelis never used and can be dropped to keep the test module minimal.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new reader lifecycle is tied directly to `asyncio.Task` and `asyncio.create_task`, which makes `AstrbotLspClient` effectively asyncio-only; if you intend to keep AnyIO backend flexibility, consider using an internal `anyio.create_task_group()` or `anyio.lowlevel.spawn_task` instead of hard-wiring asyncio primitives.
- In `tests/integration/test_lsp_integration.py`, the imported `checkpoint` from `anyio.lowlevel` is never used and can be dropped to keep the test module minimal.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
AstrbotLspClientwith a normal background reader task that can be cancelled and awaited cleanlyconnect_to_server()andshutdown()no longer corrupt AnyIO cancel scopes when run under timeout guardsUnit Testsworkflow at 30 minutesTest Plan
uv run ruff check astrbot/_internal/protocols/lsp/client.py tests/integration/test_lsp_integration.pyuv run python -m pytest tests/integration/test_lsp_integration.py tests/integration/test_mcp_integration.py tests/integration/test_acp_integration.py -q -o faulthandler_timeout=20uv run python -m pytest tests -q -o faulthandler_timeout=20bash ./scripts/run_pytests_ci.sh ./testsNotes
Summary by Sourcery
Ensure LSP client background reader tasks shut down cleanly without leaking cancel scopes and add regression coverage for timeout behavior.
Bug Fixes:
Enhancements:
Documentation:
Tests: