Repository navigation
fix: prevent backup export from blocking the event loop - #10424
JosephTian876 wants to merge 4 commits into
Conversation
![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 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/core/backup/exporter.py" line_range="187" />
<code_context>
+ await progress_callback("main_db", 0, 100, "正在导出主数据库...")
+ main_data = await self._export_main_database()
+ main_db_json = json.dumps(
+ main_data, ensure_ascii=False, indent=2, default=str
+ )
+ await self._run_in_archive_thread(
</code_context>
<issue_to_address>
**Export work stalls the event loop**
When database or knowledge-base data is large, or a knowledge base has a large media tree, `export_all()` runs JSON serialization and `_generate_manifest()` synchronously on the event loop; the manifest scan also walks the media tree there. These operations delay heartbeats and other bot tasks despite ZIP writes being offloaded.
Offload JSON serialization and manifest generation, including its filesystem scan, from the event-loop thread.
Also at `astrbot/core/backup/exporter.py:208`, `astrbot/core/backup/exporter.py:232-233`, `astrbot/core/backup/exporter.py:282-283`.
</issue_to_address>
### Comment 2
<location path="astrbot/core/backup/exporter.py" line_range="282" />
<code_context>
- # 6. 生成 manifest
- if progress_callback:
- await progress_callback("manifest", 0, 100, "正在生成清单...")
- manifest = self._generate_manifest(main_data, kb_meta_data, dir_stats)
- manifest_json = json.dumps(manifest, ensure_ascii=False, indent=2)
- zf.writestr("manifest.json", manifest_json)
</code_context>
<issue_to_address>
**Manifest lists stale checksums**
When the same exporter instance is reused after an optional file or knowledge-base content is omitted, `_generate_manifest()` includes entries left in `self._checksums` from the prior export, so the new manifest lists checksums for members absent from its ZIP.
Reset `_checksums` for each export before collecting checksums.
Also at `astrbot/core/backup/exporter.py:365-366`.
</issue_to_address>
### Comment 3
<location path="astrbot/core/backup/exporter.py" line_range="347" />
<code_context>
+ )
+ while not close_task.done():
+ try:
+ await asyncio.shield(close_task)
+ except asyncio.CancelledError:
+ cancelled_while_cleaning = True
</code_context>
<issue_to_address>
**Failed exports leave partial ZIPs**
When archive opening or closing raises a `BaseException` other than `asyncio.CancelledError`, the `open_task.result()` or shielded `close_task` path propagates the exception past the partial-file removal, leaving a corrupt ZIP in the backup directory.
Ensure partial-file cleanup runs even when archive opening or closing raises a `BaseException`.
Also at `astrbot/core/backup/exporter.py:331`, `astrbot/core/backup/exporter.py:351`.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 3 findings to address first, and if the executor, cancellation, or finalization handling is wrong, the export could leave a corrupt or incomplete backup archive, or report failure after producing one. Reverting restores the previous export behavior, and the affected archive can be removed and regenerated from the unchanged source data.
Blocking findings: astrbot/core/backup/exporter.py:187, astrbot/core/backup/exporter.py:282, astrbot/core/backup/exporter.py:347
Sourcery's three findings are addressed in
|
export_all() ran every ZIP compression and directory walk on the asyncio event loop thread. On a 2.4 GB dataset that froze the whole bot for 76 s, and AstrBot's own diagnostic recorded a 119.578 s loop lag, well past its 15 s threshold. While the loop is frozen the reverse WebSocket to the OneBot implementation cannot answer, so the implementation times out after ~90 s and reconnects. Archive work now runs on a dedicated single-worker ThreadPoolExecutor owned by export_all. A single worker is used rather than the shared default pool because zipfile.ZipFile is not safe for concurrent use, and it doubles as a FIFO queue: the archive is opened and closed on that worker too, so a close can never overlap an in-flight write. A failed finalisation is reported as a failure instead of returning a path to an unusable archive. Backup format, ZIP layout, manifest.json, checksums, progress_callback semantics and every public signature are unchanged.
Cover the event-loop offload, archive structural stability, cancellation teardown, re-entrancy, and the rule that a failed finalisation must not be reported as a successful export. Each guard fails on the previous implementation.
Address the review findings on the non-blocking backup export: - JSON serialization (main DB, KB metadata, KB documents, manifest) and manifest generation now run on the archive worker instead of the event loop. The manifest walks every KB media directory, so it could freeze the loop just as the compression used to. - Clear self._checksums at the start of every export, so a reused exporter cannot carry the previous run's checksums into a new manifest that no longer contains those files. - Capture BaseException from the archive open and close paths into open_error/close_error instead of letting it escape the teardown and skip the partial-ZIP removal. A failed open with no other error is now reported instead of returning a path to a file that was never written. Each guard has a regression test that fails on the previous revision.
The exporter now also archives `mcp_server.json` from the AstrBot data directory. The archive structure test asserts an exact member list while the exporter still read that file from the process data root, so the assertion only held on a checkout without one. Point `get_astrbot_data_path` at the temporary data directory used by the test, matching how the neighbouring `get_backup_directories` patch already isolates the archive from real user data.
f342611 to
4958797
Compare
Rebased onto
|
Modifications / 改动点
AstrBotExporter.export_all()ran every ZIP compression and directory walk on the asyncio event loop thread. On the reporting instance (2.4 GB of plugin data) that froze the whole bot for 76 s, and AstrBot's own diagnostic recorded it:This is not merely a slow export. While the loop is frozen the reverse WebSocket to the OneBot implementation cannot answer; the implementation times out after ~90 s, declares the connection half-open and reconnects. On the reporting instance that reconnect then collided with an unrelated aiocqhttp API-client bookkeeping race, after which every outbound API call raised
ApiNotAvailableand the bot stopped responding entirely until it was restarted.Changes in
astrbot/core/backup/exporter.py:ThreadPoolExecutorowned byexport_all, created at the start and always shut down infinally._run_in_archive_thread()and routed every access to the sharedZipFile(5 ×writestr, the directory walk, attachments, knowledge-base media, FAISS index) through it. A single dedicated worker is used rather than the shared default pool (asyncio.to_thread) becausezipfile.ZipFileis not safe for concurrent use.ZipFileis now opened and closed on that worker instead of viawithon the event loop. A single-worker executor is a FIFO queue, so a close submitted after an in-flight write cannot overlap it. Previously thewithblock's__exit__could callZipFile.close()on the loop thread while a write was still running, raisingCan't close the ZIP file while there is an open writing handle on itand leaving an unusable partial archive.BaseException-safe, so anasyncio.CancelledError(which does not inherit fromException) can no longer skip the partial-archive cleanup.export_all()on the same instance fails immediately with a clear error instead of silently subverting the first call's executor.tests/test_backup.py: added regression tests.No changes to the backup format, ZIP layout,
manifest.json, checksums,progress_callbacksemantics, any public signature,importer.py, orBACKUP_MANIFEST_VERSION. No new dependencies.Screenshots or Test Results / 运行截图或测试结果
Real 2399 MB dataset, same machine, same AstrBot, only the exporter implementation differing:
The "before" row is the point: the heartbeat coroutine collected zero samples during a 76.7 s export, because the loop never got a chance to run. Export wall time is essentially unchanged (76.7 s → 73.1 s), i.e. the work moved off the loop rather than becoming faster.
Output is unchanged. Exporting the same real dataset with the old and new code produces archives matching on: member path list and order (3524 members), per-member CRC / uncompressed size / compress type, all recorded checksums,
manifest.jsonapart from the wall-clockexported_at, andstatistics.directories. Spot-checked members are byte-identical. With the clock frozen, the two archives are byte-identical.A failed finalisation no longer reports success. Patching the real
zipfile.ZipFile._write_end_recordto raiseOSError(ENOSPC):completedBadZipFileOSErrorwith the real errnoVerification:
uv run pytest tests/test_backup.py -q→ 103 passed (76 before this change; the extra 15 come from upstream tests merged into master during the rebase)uv run pytest tests/unit -q→ 1911 passed, 23 skippeduvx --from ruff==0.15.22 ruff format --check .→514 files already formatteduvx --from ruff==0.15.22 ruff check .→All checks passed!The new guards fail on the previous implementation (verified by swapping it back: 4 failed), so they genuinely guard the fix rather than merely passing.
Independent adversarial verification across three review rounds and three test rounds at hash-pinned revisions: cancellation (36 checks), concurrency (120 checks), failure paths and thread/handle leak checks (20 checks), byte-equivalence and edge inputs (34 checks: CJK/emoji filenames, zero-length files, empty/absent directories, deep nesting, files deleted mid-walk,
__pycache__/.pycexclusion), and the knowledge-base path (23 checks). 12 sequential exports grew neither thread count nor OS handles.This is NOT a breaking change.
Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能，已经通过 Issue / 邮件等方式和作者讨论过。
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试，并已在上方提供了“验证步骤”和“运行截图”。
📚 I checked the affected WebUI instructions and screenshots in
docs/zhanddocs/enagainst the changed navigation, page structure, and labels, and updated them in this PR (or explained why no documentation update is needed). For renamed, moved, or merged entry points, I included an old entry → new entry mapping in the documentation and changelog./ 我已对照变化后的 WebUI 入口、页面结构和术语，核对并在本 PR 中更新
docs/zh和docs/en的相关操作说明与截图（或说明无需更新文档的原因）。入口改名、移动或合并时，已在文档和 changelog 中补充 旧入口 → 新入口 对照。🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库，或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码。
Summary by Sourcery
Keep backup exports responsive and reliable by moving blocking archive work off the event loop and hardening failure and cleanup handling.
Bug Fixes:
Enhancements:
Tests: