Repository navigation
feat(backups): delete, retention, PostgreSQL coverage and a findable destination (#13307) - #13487
Merged
Merged
Conversation
Contributor
✅ SSOT Configuration Compliance: Passing🎉 No hardcoded values detected that have SSOT config equivalents! |
This was referenced Aug 4, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thinking Path
Backend half of #13307, against the three decisions taken on the issue: Postgres in scope,
/var/lib/slm/backups, delete + retention.The ordering follows the issue's own principle — what protects data soonest, not what was asked first. Adding PostgreSQL matters most: the page is called "Backups", it creates something called a backup, and PostgreSQL — users, LLC work items, chat/session data — was simply absent. A restore would not bring any of it back, and you would find that out during recovery.
Two things surfaced while building it that were more dangerous than the missing features.
Deleting a backup could have deleted the live database.
_complete_backuprecords the remote path when the copy to SLM storage fails — for Redis that is/var/lib/redis/dump.rdbon the target node, the running database. A naivedelete_backupthat unlinksbackup.backup_pathwould destroy the data the backup exists to protect._unlink_backup_fileis therefore scoped toBACKUP_STORAGE_DIRand removes the record while leaving anything outside it alone.Moving the destination broke module import.
backup_service = BackupService()runs at import and used tomkdirunconditionally. That was safe while the destination was a home directory; against root-owned/var/lib/slm/backupsit raisesPermissionErrorandimport services.backupfails outright — taking the API down on any host where Ansible has not run yet, and in CI. Found by running it, not by reading it. The mkdir is now best-effort with a warning naming the role that owns the directory, and the copy step surfaces a real error at the point it matters.What Changed
Destination —
config.pydefaults to/var/lib/slm/backups;services/backup.pydrops its deadhasattr(settings, "backup_dir")fallback.settingshas always definedbackup_dir, so the/var/lib/slm/backupswritten there could never apply — two files naming different defaults, with the unreachable one being correct. One source now.Delete —
DELETE /backups/{backup_id}→ 204.@router.deletecount inapi/stateful.pywas 0, so nothing could reclaim space and retention was unimplementable. Refuses an in-progress backup (409); succeeds when the file is already gone, because a row pointing at a missing file is what a half-finished cleanup leaves behind and refusing would make that state unrecoverable through the API.Retention —
apply_retention(node_id, service_type, keep_count, max_age_days), defaults10/30 days, either dimension disabled with0. Runs after each successful backup rather than on a timer: a new good backup is the only moment it is safe to drop an old one, and it needs no scheduler this service does not have. Onlycompletedbackups count towardkeep_count— counting failures would let three bad nights evict the last good backup, precisely when it is the only thing between you and data loss.PostgreSQL —
execute_postgres_backup, dispatched forpostgres/postgresql. The dump command mirrorsroles/postgresql/templates/autobot-pg-backup.sh.j2exactly (su - postgres -c "pg_dumpall --clean --if-exists" | gzip), so there is one way this cluster is dumped and no password is embedded. Dumps to.partialand renames, so an interrupted run leaves nothing the copy step could pick up. The staged dump is removed from the node in afinally— it is a full copy of the cluster sitting in/tmp.Three helpers were Redis-specific and are now shared rather than duplicated:
_get_rdb_file_info→_get_remote_file_info,_copy_backup_to_storagegains asuffix(the.rdbextension was hardcoded and would have labelled every Postgres dump as a Redis one), and_complete_backuptakesremote_pathinstead of hardcoding/var/lib/redis/dump.rdb— which would have pointed a failed Postgres backup at Redis's live dump.Provisioning — the
slm_managerrole creates/var/lib/slm/backupsat 0750, not the 0755 its directory loop uses: a backup now contains a full Redis and PostgreSQL dump, so it carries every credential, user record and chat message the platform holds.Verification
The two destructive guards were invert-tested — each fails on exactly its own regression:
tests/api/test_scim.pyfails collection withModuleNotFoundError: services.system_secrets_vault. Pre-existing — reproduced on an untouched checkout, unrelated to this change.Tests load
services/backup.pyfrom its own path rather than importing it: the harness replacesconfigand much ofserviceswith MagicMocks, and asserting against those is vacuous — every attribute is truthy, so a test passes whether or not the code does anything. Where even that is not enough (the harness'sconfigstill leaks in), the property is pinned at source level with the reason recorded inline rather than asserted against a mock.Scope not delivered
The frontend half — rendering
backup_pathinBackupsView.vue(currently zero references, the direct fix for "no idea where they are created"), plus delete/retention controls and i18n across all 11 locales — is not in this PR and #13307 stays open. Export/import and remote destinations remain follow-ups per the decision on the issue.Refs #13307
Model Used
Opus 5 (1M context)