fix: guard desktop-managed core restart#9098
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to detect if the backend process is managed by AstrBot Desktop via the ASTRBOT_DESKTOP_MANAGED environment variable. When enabled, manual restarts and updates from the WebUI/core are blocked, returning error messages or raising exceptions. Unit tests have been added to verify these restrictions. The feedback suggests logging the error message before raising a RuntimeError in the background reboot thread to prevent silent crashes and ensure proper log visibility.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if is_desktop_managed_backend(): | ||
| raise RuntimeError(DESKTOP_MANAGED_RESTART_MESSAGE) |
There was a problem hiding this comment.
When _reboot is called from core_lifecycle.restart(), it is executed inside a background daemon thread. If is_desktop_managed_backend() is true, raising a RuntimeError directly in this background thread will cause it to crash silently (with the traceback only printed to sys.stderr), which might bypass the application's configured logging system (LogBroker/LogManager).
Since the core lifecycle managers have already been terminated by the time _reboot is called, the application will be left in a partially terminated, unresponsive state without any clear indication in the application log files.
To improve observability and make troubleshooting easier, please log the error message using logger.error before raising the exception.
| if is_desktop_managed_backend(): | |
| raise RuntimeError(DESKTOP_MANAGED_RESTART_MESSAGE) | |
| if is_desktop_managed_backend(): | |
| logger.error(DESKTOP_MANAGED_RESTART_MESSAGE) | |
| raise RuntimeError(DESKTOP_MANAGED_RESTART_MESSAGE) |
cd21726 to
e448502
Compare
e448502 to
2c51a55
Compare
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
updates._service_error, relying onstr(exc) == DESKTOP_MANAGED_RESTART_MESSAGEis brittle; consider using a dedicated exception subclass, error code, or flag on the exception to distinguish this case more robustly instead of matching on the message text. - The
is_desktop_managed_backendhelper currently checksos.environ.get("ASTRBOT_DESKTOP_MANAGED") == "1"; if you expect other truthy values or want more flexibility, consider normalizing the env var to a boolean (e.g., via a small parser or reading once at startup) rather than hard-coding a single string value.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `updates._service_error`, relying on `str(exc) == DESKTOP_MANAGED_RESTART_MESSAGE` is brittle; consider using a dedicated exception subclass, error code, or flag on the exception to distinguish this case more robustly instead of matching on the message text.
- The `is_desktop_managed_backend` helper currently checks `os.environ.get("ASTRBOT_DESKTOP_MANAGED") == "1"`; if you expect other truthy values or want more flexibility, consider normalizing the env var to a boolean (e.g., via a small parser or reading once at startup) rather than hard-coding a single string value.
## Individual Comments
### Comment 1
<location path="astrbot/dashboard/api/updates.py" line_range="62-63" />
<code_context>
def _service_error(exc: UpdateServiceError) -> JSONResponse:
logger.error(f"Dashboard update operation failed: {exc}", exc_info=True)
+ if str(exc) == DESKTOP_MANAGED_RESTART_MESSAGE:
+ return JSONResponse(
+ {"status": "error", "message": str(exc), "data": None},
+ status_code=200,
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid relying on the string representation of the exception for control flow.
Using `str(exc)` to detect the desktop-managed case is fragile: any change to the message text or localization will break this logic and could misclassify errors. Prefer a stable signal such as an `error_code` field on `UpdateServiceError` or a dedicated exception subclass so control flow doesn’t depend on presentation text.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
2c51a55 to
89049e0
Compare
|
@sourcery-ai review |
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="tests/test_dashboard.py" line_range="2690-2691" />
<code_context>
+ assert response.status_code == 400
+ data = await response.get_json()
+ assert data["status"] == "error"
+ assert "desktop" in data["message"].lower()
+ assert restart_called is False
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** The message assertion is relatively loose; consider asserting the full desktop-managed message or a more specific substring to catch regressions in the user-facing guidance.
Because the check only looks for the word "desktop", the test will still pass if the message becomes vague or changes meaning. To better protect the UX, consider asserting against the shared `DESKTOP_MANAGED_RESTART_MESSAGE` constant, or at least a more specific key phrase like "restart or update from the desktop app" so the test fails when the guidance changes or disappears.
Suggested implementation:
```python
assert data["status"] == "error"
assert DESKTOP_MANAGED_RESTART_MESSAGE in data["message"]
assert restart_called is False
```
To complete this change, ensure that `DESKTOP_MANAGED_RESTART_MESSAGE` is imported at the top of `tests/test_dashboard.py` from the module where it is defined, for example:
```python
from <your_module> import DESKTOP_MANAGED_RESTART_MESSAGE
```
Replace `<your_module>` with the actual module path where the constant is declared (likely the module that constructs the response message). If the response is expected to match the constant exactly, you could further tighten the check to `assert data["message"] == DESKTOP_MANAGED_RESTART_MESSAGE`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
89049e0 to
a74c2de
Compare
Background
AstrBot Desktop can serve the bundled WebUI to an external browser. In that path, the page does not have the Tauri desktop bridge, so the original WebUI fallback can call core restart/update APIs directly:
POST /api/stat/restart-corePOST /api/update/doWhen the backend process is managed by the desktop shell, those APIs must not self-reboot or self-update the core process. Otherwise, a browser-triggered restart/update can spawn a backend process outside Tauri's process management. This is especially risky on Windows with reboot behavior changes such as #9073, where a reboot can start a new console process.
Summary
astrbot.core.desktop_runtimewith a sharedASTRBOT_DESKTOP_MANAGED=1guard._reboot()fallback guard and log the desktop-managed rejection before raising.StatServiceandUpdateService.Behavior
When
ASTRBOT_DESKTOP_MANAGED=1is set:AstrBotUpdator._reboot()raise a runtime error instead of replacing/spawning the process.Normal non-desktop AstrBot behavior is unchanged.
Related Desktop PR
The paired desktop shell PR sets
ASTRBOT_DESKTOP_MANAGED=1for packaged desktop-managed backend processes:Tests
uv run ruff check astrbot/core/desktop_runtime.py astrbot/core/updator.py astrbot/dashboard/api/updates.py astrbot/dashboard/services/stat_service.py astrbot/dashboard/services/update_service.py tests/test_dashboard.py uv run pytest tests/test_dashboard.py -k "desktop_managed_backend or test_do_update"CI status at last check: all reported checks passed, no failing checks.
Summary by Sourcery
Guard core restart and update logic when the backend is managed by the desktop shell, preventing WebUI-triggered reboots and updates from escaping desktop process management.
New Features:
Bug Fixes:
Enhancements:
Tests: