fix: make project update flow atomic#8805
Conversation
dosubot
Bot
added
size:L
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
extract_dashboard_funcparameter andcall_extract_dashboardhelper are annotated asCallable[..., Awaitable[Any]], but the defaultextract_dashboardimplementation is synchronous; consider broadening these type hints (e.g.,Callable[..., Any]) to better reflect actual usage and avoid misleading annotations. - In
download_update_package, thepathparameter defaults to"temp.zip"without ensuring the parent directory exists; if this is ever called outsideupdate_project, consider mirroring theensure_dir(zip_path.parent)logic used for dashboard downloads to make the method more robust to different working directories.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `extract_dashboard_func` parameter and `call_extract_dashboard` helper are annotated as `Callable[..., Awaitable[Any]]`, but the default `extract_dashboard` implementation is synchronous; consider broadening these type hints (e.g., `Callable[..., Any]`) to better reflect actual usage and avoid misleading annotations.
- In `download_update_package`, the `path` parameter defaults to `"temp.zip"` without ensuring the parent directory exists; if this is ever called outside `update_project`, consider mirroring the `ensure_dir(zip_path.parent)` logic used for dashboard downloads to make the method more robust to different working directories.
## Individual Comments
### Comment 1
<location path="astrbot/core/utils/io.py" line_range="506-515" />
<code_context>
+ extract_dashboard(zip_path, extract_path)
+
+
+def extract_dashboard(zip_path: str | Path, extract_path: str | Path = "data") -> None:
+ """Extract a downloaded dashboard archive.
+
+ Args:
+ zip_path: Dashboard zip archive path.
+ extract_path: Directory where the archive contents should be extracted.
+
+ Returns:
+ None.
+ """
+
+ ensure_dir(extract_path)
with zipfile.ZipFile(zip_path, "r") as z:
z.extractall(extract_path)
</code_context>
<issue_to_address>
**🚨 issue (security):** Consider hardening dashboard extraction against path traversal in ZIP entries.
Because this helper is now the single extraction point, it’s worth validating entries before `extractall`. `zipfile` will write outside `extract_path` if filenames contain `../` or absolute paths. A common mitigation is to iterate over `z.infolist()`, compute `resolved = (Path(extract_path) / member.filename).resolve()`, verify `resolved.is_relative_to(Path(extract_path).resolve())` (or equivalent), raise on violation, and then call `z.extract(member, extract_path)` only for validated members. This preserves behavior for valid archives while preventing path traversal from malicious or corrupted ZIPs.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the update process for AstrBot Core and its dashboard by separating the download, verification, and application phases, ensuring that update packages are fully downloaded and verified before being applied. The feedback highlights critical performance issues where synchronous, I/O-heavy operations—specifically zip extraction, package verification (testzip), and applying the update package—are executed directly on the main asyncio event loop. To prevent the dashboard from becoming unresponsive, these blocking operations should be offloaded to separate threads using asyncio.to_thread.
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.
| async def call_extract_dashboard(*args, **kwargs): | ||
| result = extract_dashboard(*args, **kwargs) | ||
| if inspect.isawaitable(result): | ||
| return await result | ||
| return result |
There was a problem hiding this comment.
The extract_dashboard function is synchronous and performs heavy I/O and CPU-bound zip extraction. Calling it directly inside an async def function blocks the single-threaded asyncio event loop, which can make the entire dashboard unresponsive during the extraction process.
We should run it in a separate thread using asyncio.to_thread to keep the event loop responsive, while still safely handling potential coroutine/awaitable mocks in tests.
| async def call_extract_dashboard(*args, **kwargs): | |
| result = extract_dashboard(*args, **kwargs) | |
| if inspect.isawaitable(result): | |
| return await result | |
| return result | |
| async def call_extract_dashboard(*args, **kwargs): | |
| import asyncio | |
| if inspect.iscoroutinefunction(extract_dashboard): | |
| return await extract_dashboard(*args, **kwargs) | |
| result = await asyncio.to_thread(extract_dashboard, *args, **kwargs) | |
| if inspect.isawaitable(result): | |
| return await result | |
| return result |
1 participant
Summary
Tests
Note: full ruff check . is currently blocked by an existing untracked packages/astrbot-sdk ASYNC220 warning outside this PR.
Summary by Sourcery
Make the project update process atomic by separating download, verification, and application of core and dashboard updates, and by adding coverage for failure scenarios.
New Features:
Bug Fixes:
Enhancements:
Tests: