Repository navigation
fix: preserve persistent data in backups - #10661
Conversation
There was a problem hiding this comment.
Hey - I've found 5 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/core/backup/importer.py" line_range="555" />
<code_context>
+ # Replace restores also invalidate device trust on the target instance.
+ for table_name, model_class in (
+ MAIN_DB_MODELS | EXCLUDED_MAIN_DB_MODELS
+ ).items():
try:
await session.execute(delete(model_class))
</code_context>
<issue_to_address>
**Failed restores erase existing records**
When the process stops or database insertion fails after the clear commits and before the import transaction commits, `import_all` commits `_clear_main_db()` before `_import_main_database()` starts its separate transaction, so a crash or import failure between them leaves the newly registered tables empty along with the other cleared tables. The restore reports failure, but the target data is already lost.
Make clearing and importing the main database one transaction, or stage the restored data and swap it in only after the import succeeds.
</issue_to_address>
### Comment 2
<location path="astrbot/core/backup/constants.py" line_range="51" />
<code_context>
# 主数据库模型类映射
MAIN_DB_MODELS: dict[str, type[SQLModel]] = {
"platform_stats": PlatformStat,
+ "provider_stats": ProviderStat,
"conversations": ConversationV2,
"cron_jobs": CronJob,
</code_context>
<issue_to_address>
**Database errors erase backed-up records**
When a query for one of the newly registered tables fails, `_export_main_database` catches a failed table query and records an empty list; a replace restore then clears that target table and imports no rows, while the backup and restore still report success.
Propagate table export failures or mark the backup as incomplete so it cannot be mistaken for a complete backup.
Also at `astrbot/core/backup/constants.py:65-66`.
</issue_to_address>
### Comment 3
<location path="astrbot/core/backup/constants.py" line_range="98" />
<code_context>
"webchat": get_astrbot_webchat_path(), # WebChat 数据
"temp": get_astrbot_temp_path(), # 临时文件
"skills": get_astrbot_skills_path(), # Skills
+ "workspaces": get_astrbot_workspaces_path(), # Session and project files
}
</code_context>
<issue_to_address>
**Workspace restore can leave partial files**
When a workspace file write or extraction fails after the existing target directory has been moved aside, `_import_directories` moves the target workspaces directory to `.bak` and then writes files one by one; a write failure leaves an incomplete live directory and the importer records only a warning. The restore can therefore finish with missing workspace files while the prior files are no longer at their original path.
Extract into a staging directory and atomically swap it into place only after every file succeeds, restoring the previous directory on failure.
</issue_to_address>
### Comment 4
<location path="astrbot/core/backup/constants.py" line_range="98" />
<code_context>
"webchat": get_astrbot_webchat_path(), # WebChat 数据
"temp": get_astrbot_temp_path(), # 临时文件
"skills": get_astrbot_skills_path(), # Skills
+ "workspaces": get_astrbot_workspaces_path(), # Session and project files
}
</code_context>
<issue_to_address>
**Empty workspace restore leaves stale files**
When the backup has an empty workspaces directory and the target has workspace files, `_import_directories` skips directories with no archived files before clearing or replacing the target, so the old workspace files remain after the restore.
Preserve empty directories in the archive or clear listed target directories during replace restores even when they contain no archived files.
Also at `astrbot/core/backup/constants.py:41`.
</issue_to_address>
### Comment 5
<location path="astrbot/core/backup/constants.py" line_range="98" />
<code_context>
"webchat": get_astrbot_webchat_path(), # WebChat 数据
"temp": get_astrbot_temp_path(), # 临时文件
"skills": get_astrbot_skills_path(), # Skills
+ "workspaces": get_astrbot_workspaces_path(), # Session and project files
}
</code_context>
<issue_to_address>
**Workspace files silently miss backups**
When a workspace file cannot be read during export, `_export_directories` logs and skips a workspace file it cannot read, then completes the export with the remaining files; the archive is returned as a successful backup without reporting that persistent workspace data is missing.
Fail the export or surface an incomplete-backup result when any workspace file is skipped.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 5 findings to address first, and restoring a backup can add the source instance’s API-key records to the target, making those keys valid there, and replaces target workspace files, deleting files absent from the backup. Reverting the code does not undo an access grant or recover files already deleted; keys must be revoked and lost files restored separately.
Blocking findings: astrbot/core/backup/importer.py:555, astrbot/core/backup/constants.py:51, astrbot/core/backup/constants.py:98, astrbot/core/backup/constants.py:98, astrbot/core/backup/constants.py:98
| # Replace restores also invalidate device trust on the target instance. | ||
| for table_name, model_class in ( | ||
| MAIN_DB_MODELS | EXCLUDED_MAIN_DB_MODELS | ||
| ).items(): |
There was a problem hiding this comment.
🟠 High · Failed restores erase existing records
When the process stops or database insertion fails after the clear commits and before the import transaction commits, import_all commits _clear_main_db() before _import_main_database() starts its separate transaction, so a crash or import failure between them leaves the newly registered tables empty along with the other cleared tables. The restore reports failure, but the target data is already lost.
Make clearing and importing the main database one transaction, or stage the restored data and swap it in only after the import succeeds.
Prompt for AI agents
In `astrbot/core/backup/importer.py` at line 555:
**Failed restores erase existing records**
When the process stops or database insertion fails after the clear commits and before the import transaction commits, `import_all` commits `_clear_main_db()` before `_import_main_database()` starts its separate transaction, so a crash or import failure between them leaves the newly registered tables empty along with the other cleared tables. The restore reports failure, but the target data is already lost.
Make clearing and importing the main database one transaction, or stage the restored data and swap it in only after the import succeeds.| # 主数据库模型类映射 | ||
| MAIN_DB_MODELS: dict[str, type[SQLModel]] = { | ||
| "platform_stats": PlatformStat, | ||
| "provider_stats": ProviderStat, |
There was a problem hiding this comment.
🟠 High · Database errors erase backed-up records
When a query for one of the newly registered tables fails, _export_main_database catches a failed table query and records an empty list; a replace restore then clears that target table and imports no rows, while the backup and restore still report success.
Propagate table export failures or mark the backup as incomplete so it cannot be mistaken for a complete backup.
Also at astrbot/core/backup/constants.py:65-66.
Prompt for AI agents
In `astrbot/core/backup/constants.py` at line 51:
**Database errors erase backed-up records**
When a query for one of the newly registered tables fails, `_export_main_database` catches a failed table query and records an empty list; a replace restore then clears that target table and imports no rows, while the backup and restore still report success.
Propagate table export failures or mark the backup as incomplete so it cannot be mistaken for a complete backup.
Also at `astrbot/core/backup/constants.py:65-66`.
Backups still omit session aliases, provider usage statistics, OpenAPI access keys, and MCP configuration after #10644. Restoring on another instance loses those records and configuration even though the restore reports success.
Modifications
Register
UmoAlias,ProviderStat, andApiKeyfor both export and restore. Preserve key hashes, scopes, expiry, usage, and revocation metadata so existing clients continue to authenticate and revoked keys remain unusable.Export
data/mcp_server.jsonwith a checksum and restore it while retaining an existing target configuration as.json.bak. Backups without this file keep the target's MCP configuration.Explicitly exclude
DashboardTrustedDevicefrom backup and clear target device trust during replace restores, requiring device verification again.Add a model coverage test that requires every main database table to have an explicit backup or exclusion policy, plus real SQLite-to-ZIP-to-SQLite and cross-root file regression tests.
Document backup coverage and legacy behavior in both English and Chinese WebUI instructions. No navigation, UI labels, screenshots, API schemas, or dependencies change.
This is not a breaking change to the backup format. Existing archives remain importable.
Older archives cannot recover records or files they never contained. Export a new backup from the original instance before migrating. Replace restores clear the newly registered tables even when an old archive has no records for them.
Test Results
python -m pytest tests/test_backup.py tests/unit/test_cron_manager.py -q: 135 passed, with 3 existing warnings (audioopdeprecation and two upload cleanup coroutine warnings).ruff format .: passed.ruff check .: passed.git diff --check: passed.Regression tests cover replacement of existing rows, active and revoked API keys, device trust invalidation, MCP checksums and configuration backups, fresh and existing targets, and archives without MCP configuration.
Checklist
Summary by Sourcery
Restore persistent database records, workspace content, and MCP configuration reliably while enforcing explicit backup coverage and trusted-device invalidation.
Bug Fixes:
Enhancements:
Documentation:
Tests: