Skip to content

fix: preserve persistent data in backups - #10661

Merged
Soulter merged 2 commits into
masterfrom
codex/fix-backup-data-coverage
Oct 21, 2026
Merged

Soulter merged 2 commits into
masterfrom
codex/fix-backup-data-coverage

Conversation

@Soulter

@Soulter Soulter commented Oct 21, 2026 •

Copy link
Copy Markdown
Member

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, and ApiKey for 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.json with 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 DashboardTrustedDevice from 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 (audioop deprecation 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

  • Bug fix following the backup omissions identified while reviewing fix: include scheduled tasks in backups #10644; no unrelated feature additions.
  • Tested with verification results above.
  • Updated affected English and Chinese backup instructions; no navigation or screenshot changes are needed.
  • No new dependencies.
  • No malicious code introduced.

Summary by Sourcery

Restore persistent database records, workspace content, and MCP configuration reliably while enforcing explicit backup coverage and trusted-device invalidation.

Bug Fixes:

  • Preserve session aliases, provider usage statistics, and OpenAPI access keys during backup export and restore.
  • Back up and restore MCP server configuration with checksums while retaining the target configuration when archives omit it.
  • Include workspace files in backups and ensure replace restores clear trusted-device records so device verification is required again.

Enhancements:

  • Define explicit backup or exclusion policies for all main database tables, excluding trusted devices from archives.
  • Document backup coverage and legacy archive behavior in English and Chinese WebUI instructions.

Documentation:

  • Document the records and configuration covered by backups, workspace backup requirements, trusted-device behavior, and legacy archive limitations.

Tests:

  • Add coverage checks and end-to-end SQLite, archive, cross-instance, MCP configuration, persistent-record, trusted-device, and workspace backup regression tests.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Sourcery is free for open source - if you like our reviews please consider sharing them ✨

# Replace restores also invalidate device trust on the target instance.
for table_name, model_class in (
MAIN_DB_MODELS | EXCLUDED_MAIN_DB_MODELS
).items():

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 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,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 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`.

Comment thread astrbot/core/backup/constants.py Outdated
Comment thread astrbot/core/backup/constants.py Outdated
Comment thread astrbot/core/backup/constants.py Outdated
@Soulter
Soulter merged commit 5701a1d into master Oct 21, 2026
23 checks passed
@Soulter
Soulter deleted the codex/fix-backup-data-coverage branch October 21, 2026 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant