fix(backup): prevent stale and incomplete database restores - #23
Open
MahdiButcher wants to merge 4 commits into
Open
fix(backup): prevent stale and incomplete database restores#23MahdiButcher wants to merge 4 commits into
MahdiButcher wants to merge 4 commits into
Conversation
Contributor
WalkthroughThe change adds shared SQLite URL helpers and strengthens backup and restore validation. Database artifacts must be complete and restorable before archive creation or destructive restore operations. Failed backups clean up generated artifacts, and tests cover SQLite WAL snapshots and truncated dumps. ChangesBackup and restore validation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Backup
participant Validator
participant Archive
participant Restore
participant SQLiteDatabase
Backup->>Validator: validate database artifacts
Validator-->>Backup: return validation result
Backup->>Archive: create archive when valid
Restore->>Validator: validate selected backup
Validator-->>Restore: return validation result
Restore->>SQLiteDatabase: copy validated snapshot after data restore
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/common.sh`:
- Around line 47-49: Update the path-normalization logic around the path
variable to remove all trailing slashes while preserving the root path "/"
unchanged. Ensure values such as DATA_DIR ending with multiple slashes normalize
to a single canonical non-root path without a trailing slash, so subsequent
backup-prefix checks behave correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d35b7964-db7a-4d32-a5df-95acc185c17a
📒 Files selected for processing (6)
lib/common.shlib/pasarguard-backup.shlib/pasarguard-restore.shpasarguard.shtests/backup_restore_roundtrip.shtests/unit_lib_common.sh
MahdiButcher
force-pushed
the
fix/sqlite-backup-stale-restore
branch
from
August 1, 2026 08:50
e26bd71 to
f075feb
Compare
MahdiButcher
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
lib/pasarguard-restore.sh (1)
1188-1191: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlso remove a stale
-journalsidecar.This block removes
-waland-shmonly. If the previous database used rollback-journal mode and a hot${sqlite_file}-journalfile remains, SQLite treats it as a journal for the newly copied file and attempts a rollback on the next open. Remove it together with the other sidecars.🛡️ Proposed fix
- rm -f "${sqlite_file}-wal" "${sqlite_file}-shm" 2>>"$log_file" || true + rm -f "${sqlite_file}-wal" "${sqlite_file}-shm" "${sqlite_file}-journal" 2>>"$log_file" || true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/pasarguard-restore.sh` around lines 1188 - 1191, Update the sqlite restore cleanup block to remove the stale "${sqlite_file}-journal" sidecar alongside the existing -wal and -shm files before copying the backup, preserving the current logging and failure-tolerant behavior.tests/unit_pasarguard.sh (1)
286-294: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a manifest case with an empty
ts_version.
pg_manifest_encodeemits a trailing tab whents_versionis empty, which is the normal case for a non-TimescaleDB database.postgres_backup_looks_restorablecounts fields withawk -F '\t' '{ print NF }'and requires exactly 5. Only the populated-ts_versionshape is covered here. A case withhas_ts=0and an emptyts_versionwould lock in the field-count behavior for the common layout.💚 Proposed additional assertion
assert_false "backup validation: configured PostgreSQL database must be present" \ database_backup_looks_restorable postgresql "$DB_VALIDATION_DIR" missingdb "" + +printf '%s\n' "$(pg_manifest_encode appdb appuser 0 db-001.sql '')" >"$DB_VALIDATION_DIR/pg_dump/manifest.tsv" +assert_true "backup validation: manifest with empty ts_version accepted" \ + database_backup_looks_restorable postgresql "$DB_VALIDATION_DIR" appdb ""🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit_pasarguard.sh` around lines 286 - 294, Extend the manifest validation tests near the existing pg_manifest_encode case to add a non-TimescaleDB entry using has_ts=0 and an empty ts_version. Use the resulting manifest with database_backup_looks_restorable and assert it is accepted, covering the trailing-tab five-field layout.lib/pasarguard-backup.sh (2)
1014-1016: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the stale comment above this branch.
The comment at Lines 1007-1009 still states that an undetermined owner is recorded as empty and that restore falls back to the admin role. This branch now aborts the multi-database backup instead. Align the comment with the new behavior.
♻️ Proposed comment update
- # Owner of this database (empty if it can't be determined; restore - # falls back to the admin role). + # Owner of this database. An undetermined owner means the manifest would + # be incomplete, so abort the multi-DB backup and let the caller fall + # back to a validated single-database dump. local owner=""🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/pasarguard-backup.sh` around lines 1014 - 1016, Update the comment immediately above the owner-determination failure branch to state that an undetermined database owner causes the multi-database backup to abort and the output directory to be removed, rather than describing empty-owner recording or restore fallback behavior. Keep the branch logic unchanged.
1352-1363: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueSkip empty lines when you match the configured database.
A here-string of an empty variable still produces one empty line. If
$db_nameis empty,[ "$_db_line" = "$db_name" ]matches that empty line andconfigured_db_foundbecomestrue. The root--all-databasesdump then runs without any real presence check. The fallback branch at Line 1371 already treats an empty$db_nameas possible, so add the guard here too.🛡️ Proposed guard
while IFS= read -r _db_line; do + [ -n "$_db_line" ] || continue if [ "$_db_line" = "$db_name" ]; then configured_db_found=true break fi done <<<"$mariadb_databases"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/pasarguard-backup.sh` around lines 1352 - 1363, Update the database-matching loop around mariadb_databases and configured_db_found to skip empty lines and prevent an empty db_name from matching the here-string’s synthetic empty line. Require a non-empty db_name and non-empty _db_line before setting configured_db_found=true, preserving the existing match and dump behavior for valid database names.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/pasarguard-backup.sh`:
- Around line 1679-1689: Move the SQLite completeness check in the database
backup flow to immediately before archive creation, after the rsync copy into
temp_dir has completed. Ensure database_backup_looks_restorable validates the
final $sqlite_file artifact that will be included in the archive, preventing an
APP_DIR file with the same basename from replacing the snapshot after
validation.
In `@lib/pasarguard-restore.sh`:
- Around line 708-709: The callers of sqlite_database_path_from_url do not
validate its result. In lib/pasarguard-restore.sh lines 708-709, check both the
command status and sqlite_file emptiness; on failure, remove temp_restore_dir,
print a clear message, and exit 1. In lib/pasarguard-backup.sh lines 1250-1251,
perform the same validation, append a diagnostic to log_file, and add it to
error_messages so the backup fails clearly.
- Around line 843-858: Update the SQLite restore block to handle a missing
sqlite3 executable separately from an invalid snapshot: after the best-effort
try_install_package call, recheck command -v sqlite3 and report the missing
dependency with the existing cleanup/abort flow before invoking
sqlite_snapshot_looks_restorable. Also move the safety copy created for
sqlite_file outside DATA_DIR, using the established DATA_DIR backup location or
an equivalent path that survives the later rsync --delete before the snapshot is
applied.
---
Nitpick comments:
In `@lib/pasarguard-backup.sh`:
- Around line 1014-1016: Update the comment immediately above the
owner-determination failure branch to state that an undetermined database owner
causes the multi-database backup to abort and the output directory to be
removed, rather than describing empty-owner recording or restore fallback
behavior. Keep the branch logic unchanged.
- Around line 1352-1363: Update the database-matching loop around
mariadb_databases and configured_db_found to skip empty lines and prevent an
empty db_name from matching the here-string’s synthetic empty line. Require a
non-empty db_name and non-empty _db_line before setting
configured_db_found=true, preserving the existing match and dump behavior for
valid database names.
In `@lib/pasarguard-restore.sh`:
- Around line 1188-1191: Update the sqlite restore cleanup block to remove the
stale "${sqlite_file}-journal" sidecar alongside the existing -wal and -shm
files before copying the backup, preserving the current logging and
failure-tolerant behavior.
In `@tests/unit_pasarguard.sh`:
- Around line 286-294: Extend the manifest validation tests near the existing
pg_manifest_encode case to add a non-TimescaleDB entry using has_ts=0 and an
empty ts_version. Use the resulting manifest with
database_backup_looks_restorable and assert it is accepted, covering the
trailing-tab five-field layout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 65480e73-9e3c-4832-9ab6-ada5b02c77f4
📒 Files selected for processing (8)
lib/common.shlib/pasarguard-backup.shlib/pasarguard-restore.shpasarguard.shtests/backup_restore_roundtrip.shtests/unit_lib_common.shtests/unit_pasarguard.shtests/unit_restore_archive_safety.sh
🚧 Files skipped from review as they are similar to previous changes (4)
- pasarguard.sh
- tests/unit_lib_common.sh
- lib/common.sh
- tests/backup_restore_roundtrip.sh
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
1 participant
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.
Summary
Root causes
Tests
The existing CI matrix covers SQLite, MySQL, MariaDB, PostgreSQL, and TimescaleDB in single and multipart modes; fork PR workflows require maintainer approval.
Summary by CodeRabbit
Bug Fixes
Improvements