fix: automate safe backup and database restore - #24
Conversation
WalkthroughThe change adds SQLite URL utilities, validates database backup artifacts, strengthens restore safety, supports TimescaleDB version conversion, preserves destination credentials and compose settings, expands round-trip and unit tests, and adds CI coverage for TimescaleDB upgrades. ChangesDatabase backup and restore
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant RestoreCommand
participant CompatibilityContainer
participant TimescaleDB
RestoreCommand->>CompatibilityContainer: Prepare version-compatible dumps
CompatibilityContainer->>TimescaleDB: Restore and upgrade database
TimescaleDB-->>CompatibilityContainer: Return upgraded dump
CompatibilityContainer-->>RestoreCommand: Return validated dump
RestoreCommand->>TimescaleDB: Restore destination database
Possibly related PRs
Suggested reviewers: 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}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
lib/pasarguard-backup.sh (1)
1764-1774: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMake the containment test robust against interior duplicate slashes.
normalize_posix_pathcollapses only leading and trailing slashes.sqlite_filecomes from the URL andnormalized_data_dircomes fromDATA_DIR, so the two strings can differ in interior separators, for example/var/lib//pasarguard/db.sqlite3against/var/lib/pasarguard. The prefix test then fails, rsync copies the live database file intopasarguard_data/, and therm -fsafeguard at Lines 1779-1784 is skipped because it repeats the same condition. Collapse interior duplicate slashes innormalize_posix_pathso both operands compare equal.♻️ Proposed change in `lib/common.sh`
normalize_posix_path() { local path="$1" + while [[ "$path" == *//* ]]; do + path="${path//\/\///}" + done while [[ "$path" == //* ]]; do path="${path#/}" done🤖 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 1764 - 1774, Update normalize_posix_path in lib/common.sh to collapse duplicate interior slashes as well as its existing leading and trailing slash normalization, ensuring sqlite_file and normalized_data_dir use equivalent canonical paths for both containment checks and the subsequent rm -f safeguard.pasarguard.sh (1)
1070-1071: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCheck the helper exit status before writing
.env.
sqlite_absolute_database_urlreturns 1 when the driver is invalid or the path is not absolute (lib/common.shLines 88-89). Command substitution discards that status, soSQLALCHEMY_DATABASE_URLbecomes an empty string and.envreceivesSQLALCHEMY_DATABASE_URL = "". The installation then completes without an error message and the panel cannot connect. Fail fast instead.🛡️ Proposed guard
- SQLALCHEMY_DATABASE_URL=$(sqlite_absolute_database_url "$db_driver_scheme" "$DATA_DIR/db.sqlite3") + if ! SQLALCHEMY_DATABASE_URL=$(sqlite_absolute_database_url "$db_driver_scheme" "$DATA_DIR/db.sqlite3"); then + colorized_echo red "Failed to build the SQLite database URL for $DATA_DIR/db.sqlite3" + exit 1 + fi sed -i "s~\(SQLALCHEMY_DATABASE_URL = \).*~\1\"${SQLALCHEMY_DATABASE_URL}\"~" "$APP_DIR/.env"🤖 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 `@pasarguard.sh` around lines 1070 - 1071, Check the exit status of sqlite_absolute_database_url before running sed to update .env. In the surrounding database URL setup, preserve the helper’s failure status and abort with an error when it returns nonzero, preventing SQLALCHEMY_DATABASE_URL from being written as an empty value.lib/pasarguard-restore.sh (2)
141-159: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMatch the role token instead of the third whitespace field.
pg_dumpallquotes a role name that needs quoting. For a role name that contains a space, the output isALTER ROLE "my role" WITH ..., so$3is"my. The comparison then fails and the archivedALTER ROLEfor the destination admin role reaches the destination. Compare the text after the statement prefix instead.♻️ Proposed refactor
pg_filter_global_passwords | awk -v role="$destination_role" ' BEGIN { quoted_role = role gsub(/"/, "\"\"", quoted_role) quoted_role = "\"" quoted_role "\"" } { - statement_role = $3 - sub(/;$/, "", statement_role) - if (($1 == "CREATE" || $1 == "ALTER") && $2 == "ROLE" && - (statement_role == role || statement_role == quoted_role)) { - next - } + if ($0 ~ /^(CREATE|ALTER)[[:space:]]+ROLE[[:space:]]/) { + rest = $0 + sub(/^(CREATE|ALTER)[[:space:]]+ROLE[[:space:]]+/, "", rest) + if (rest == role || rest == role ";" || index(rest, role " ") == 1 || + rest == quoted_role || rest == quoted_role ";" || index(rest, quoted_role " ") == 1) { + next + } + } print } '🤖 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 141 - 159, Update pg_filter_globals_for_destination to match the complete role token after the CREATE/ALTER ROLE statement prefix rather than relying on awk’s $3 field. Ensure quoted role names containing spaces, such as "my role", are captured and compared against destination_role so their statements are filtered correctly.
274-287: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRaise the readiness budget and remove the unused loop variable.
The compatibility image bootstraps a new cluster on first start, so 45 seconds can expire on slow storage. The restore then aborts even though the destination is untouched.
attemptis never read, which Shellcheck reports as SC2034.Also consider an interrupt-safe cleanup. If the operator interrupts the restore between
docker runandcleanup_timescaledb_compat_container, the temporary container and volume remain on the host.♻️ Proposed refactor
local ready=false - local attempt=0 - for attempt in $(seq 1 45); do + local waited=0 + while [ "$waited" -lt 180 ]; do if docker exec "$compat_container" pg_isready -q -U "$admin_user" -d postgres >/dev/null 2>&1; then ready=true break fi sleep 1 + waited=$((waited + 1)) done🤖 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 274 - 287, Update the readiness loop in the compatibility-container restore flow to use a substantially longer startup budget, and remove the unused attempt variable while preserving the existing readiness checks. Add interrupt-safe cleanup around the interval after the temporary container and volume are created, ensuring cleanup_timescaledb_compat_container runs if the restore is interrupted before normal cleanup.Source: Linters/SAST tools
tests/backup_restore_roundtrip.sh (2)
750-775: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the archived password variable instead of the literal.
The negative checks hard-code
apppass. That literal must stay equal to the archivedDB_PASSWORDfor the assertion to test anything. Reference the variable so the assertion cannot silently pass after a fixture change.♻️ Proposed refactor
- if docker exec -e MYSQL_PWD="apppass" "$CONTAINER_NAME" mysql -h 127.0.0.1 -u "$DB_USER" "$DB_NAME" -e "SELECT 1;" >/dev/null 2>&1; then + if docker exec -e MYSQL_PWD="$DB_PASSWORD" "$CONTAINER_NAME" mysql -h 127.0.0.1 -u "$DB_USER" "$DB_NAME" -e "SELECT 1;" >/dev/null 2>&1; thenApply the same change to the MariaDB and PostgreSQL checks.
🤖 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/backup_restore_roundtrip.sh` around lines 750 - 775, Replace the hard-coded "apppass" credentials in the negative authentication checks within the mysql, mariadb, and postgresql|timescaledb cases with the archived DB password variable, preserving the existing failure messages and control flow.
66-72: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the wait for the holder process.
waitblocks with no limit. If the python holder stalls before it observes the stop file, the CI job hangs until the workflow timeout instead of failing with the captured logs. Send a signal as a fallback.♻️ Proposed refactor
stop_sqlite_holder() { if [ -n "$SQLITE_HOLDER_PID" ] && kill -0 "$SQLITE_HOLDER_PID" 2>/dev/null; then touch "$SQLITE_HOLDER_STOP" - wait "$SQLITE_HOLDER_PID" + local waited=0 + while kill -0 "$SQLITE_HOLDER_PID" 2>/dev/null && [ "$waited" -lt 30 ]; do + sleep 0.2 + waited=$((waited + 1)) + done + kill -TERM "$SQLITE_HOLDER_PID" 2>/dev/null || true + wait "$SQLITE_HOLDER_PID" 2>/dev/null || true fi SQLITE_HOLDER_PID="" }🤖 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/backup_restore_roundtrip.sh` around lines 66 - 72, Update stop_sqlite_holder to bound waiting for SQLITE_HOLDER_PID: after creating SQLITE_HOLDER_STOP, wait only for a finite timeout, then send a termination signal to the holder if it has not exited and reap it while preserving the existing PID cleanup.
🤖 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 @.github/workflows/backup-restore.yml:
- Around line 65-66: Update both checkout steps using actions/checkout@v4 in the
workflow to set persist-credentials to false, ensuring neither step leaves
GITHUB_TOKEN in the local Git configuration.
In `@lib/pasarguard-backup.sh`:
- Around line 1081-1100: Update write_timescaledb_single_dump_version to
distinguish an empty source_version, which indicates the TimescaleDB extension
is absent, from a failed query or invalid non-empty version. Treat the empty
result as a successful no-op without creating version metadata, while continuing
to return 1 for query failures or malformed non-empty versions.
In `@lib/pasarguard-restore.sh`:
- Around line 1635-1638: Update the rsync exclusions in the restore flow to also
exclude the split-zip staging artifact matching *_combined.zip and the null-byte
cleanup artifact pasarguard_env_cleaned, preventing either file from being
copied into APP_DIR while preserving all existing exclusions.
- Around line 1366-1377: Update the abort paths in the restore flow, including
the pg_layout “none” and incomplete “multi” checks and both compose-snapshot
validation aborts, to invoke the existing application-service restart helper
used by the nearby successful abort paths before cleanup and exit. Preserve the
current error messages, logging, cleanup, and exit behavior.
- Around line 246-272: Update the compatibility container setup around
compat_image and the docker run invocation to support overridden non-HA
TimescaleDB images. Select the data directory based on whether
requested_compat_image is supplied and the PostgreSQL major version: use the HA
path by default, /var/lib/postgresql/data for non-HA versions below 18, and
/var/lib/postgresql/<pg_major>/docker for version 18+. Mount compat_volume to
that selected path and set the matching PGDATA value when required.
---
Nitpick comments:
In `@lib/pasarguard-backup.sh`:
- Around line 1764-1774: Update normalize_posix_path in lib/common.sh to
collapse duplicate interior slashes as well as its existing leading and trailing
slash normalization, ensuring sqlite_file and normalized_data_dir use equivalent
canonical paths for both containment checks and the subsequent rm -f safeguard.
In `@lib/pasarguard-restore.sh`:
- Around line 141-159: Update pg_filter_globals_for_destination to match the
complete role token after the CREATE/ALTER ROLE statement prefix rather than
relying on awk’s $3 field. Ensure quoted role names containing spaces, such as
"my role", are captured and compared against destination_role so their
statements are filtered correctly.
- Around line 274-287: Update the readiness loop in the compatibility-container
restore flow to use a substantially longer startup budget, and remove the unused
attempt variable while preserving the existing readiness checks. Add
interrupt-safe cleanup around the interval after the temporary container and
volume are created, ensuring cleanup_timescaledb_compat_container runs if the
restore is interrupted before normal cleanup.
In `@pasarguard.sh`:
- Around line 1070-1071: Check the exit status of sqlite_absolute_database_url
before running sed to update .env. In the surrounding database URL setup,
preserve the helper’s failure status and abort with an error when it returns
nonzero, preventing SQLALCHEMY_DATABASE_URL from being written as an empty
value.
In `@tests/backup_restore_roundtrip.sh`:
- Around line 750-775: Replace the hard-coded "apppass" credentials in the
negative authentication checks within the mysql, mariadb, and
postgresql|timescaledb cases with the archived DB password variable, preserving
the existing failure messages and control flow.
- Around line 66-72: Update stop_sqlite_holder to bound waiting for
SQLITE_HOLDER_PID: after creating SQLITE_HOLDER_STOP, wait only for a finite
timeout, then send a termination signal to the holder if it has not exited and
reap it while preserving the existing PID cleanup.
🪄 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: 0225be4c-a191-4b1d-9ed3-ad6ae99c854c
📒 Files selected for processing (9)
.github/workflows/backup-restore.ymllib/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
1 participant
Supersedes #23. This PR includes every fix from #23 and adds automatic cross-database restore handling.
Summary
Verification
Summary by CodeRabbit
New Features
Bug Fixes
Tests