From ffe5c920c66a862726f160d707bd0ea1850a3a5b Mon Sep 17 00:00:00 2001 From: xCyanGrizzly Date: Wed, 22 Jul 2026 10:04:18 +0200 Subject: [PATCH] Fix backup service restart handling --- .env.example | 2 +- .../scope-correction-implementation-report.md | 73 +++++++++++ scripts/backup/restore-path-assertions.sh | 38 ++++++ scripts/backup/restore.sh | 45 ++++++- scripts/backup/run-backup-assertions.sh | 114 ++++++++++++++++++ scripts/backup/run-backup.sh | 7 +- 6 files changed, 273 insertions(+), 6 deletions(-) create mode 100644 scripts/backup/run-backup-assertions.sh diff --git a/.env.example b/.env.example index a3fddd1..223add6 100644 --- a/.env.example +++ b/.env.example @@ -39,7 +39,7 @@ LOG_LEVEL="info" # Disaster recovery backups BACKUP_MOUNT_PATH="/mnt/dragonsstash-backups" -BACKUP_STAGING_PATH="/var/lib/dragons-stash-backup/staging" +BACKUP_STAGING_PATH="/var/lib/dragons-stash/backup-staging" BACKUP_REPOSITORY="/backup/restic" BACKUP_RESTIC_PASSWORD_FILE="/etc/dragons-stash/restic-password" BACKUP_RETENTION_DAYS=30 diff --git a/.superpowers/sdd/scope-correction-implementation-report.md b/.superpowers/sdd/scope-correction-implementation-report.md index 4abb08c..39965a2 100644 --- a/.superpowers/sdd/scope-correction-implementation-report.md +++ b/.superpowers/sdd/scope-correction-implementation-report.md @@ -165,3 +165,76 @@ future work outside the backup scope. - None for implementation scope. - Git Bash was available and used for shell syntax/assertion checks, so Docker fallback was not needed for the final syntax verification. + +--- + +# Important Operational Findings Fix + +**Date:** 2026-07-22 + +## Fix + +Addressed the two Important operational findings from final review: + +- `scripts/backup/run-backup.sh` + - Backup wrapper restart failures now make an otherwise successful backup + exit non-zero. + - Existing non-zero backup failures remain preserved if service restart also + fails. + - Added `scripts/backup/run-backup-assertions.sh` to assert both exit-code + cases with a fake Docker/Compose environment. + +- `scripts/backup/restore.sh` + - `restore-live` now captures the managed services that were running before + live restore using `docker compose --profile full ps --status running`. + - Live restore stops only those previously running managed services. + - Successful live restore starts only those previously running services, so + the profile-gated optional `bot` is not started if it was not running. + - Failure handling leaves services stopped and still rolls back only the + PostgreSQL database plus both TDLib volumes. + - App health wait now runs only when `app` was previously running. + - Extended `scripts/backup/restore-path-assertions.sh` to assert subset + stop/start behavior and skipped health checks when `app` was not running. + +Addressed the Minor staging-path documentation mismatch by aligning +`.env.example` with the backup README example: +`/var/lib/dragons-stash/backup-staging`. + +## Verification + +- Red checks before implementation: + - `docker run --rm -v "${PWD}:/work" -w /work ubuntu:24.04 bash scripts/backup/run-backup-assertions.sh` + - Failed as expected: backup reported success when restart failed. + - `docker run --rm -v "${PWD}:/work" -w /work ubuntu:24.04 bash scripts/backup/restore-path-assertions.sh` + - Failed as expected: restore-live stopped the fixed `app worker bot` + service set instead of the running subset. + +- Focused assertions after implementation: + - `docker run --rm -v "${PWD}:/work" -w /work ubuntu:24.04 bash scripts/backup/run-backup-assertions.sh` + - Passed. + - `docker run --rm -v "${PWD}:/work" -w /work ubuntu:24.04 bash scripts/backup/restore-path-assertions.sh` + - Passed. + +- `git diff --check` + - Passed. + +- Docker Bash syntax check: + - `docker run --rm -v "${PWD}:/work" -w /work ubuntu:24.04 bash -n scripts/backup/run-backup.sh scripts/backup/restore.sh scripts/backup/container-entrypoint.sh scripts/backup/restore-path-assertions.sh scripts/backup/run-backup-assertions.sh` + - Passed. + +- `npx prisma validate` + - Passed. + +- `npm run build` + - Passed. + +- `npm run lint` + - Failed on unrelated existing React lint issues in `src/` and mirrored + `.worktrees/worker-improvements` files; no failures were in touched backup + files. + +## Concerns + +- Local `bash` is unavailable because the Windows `bash` command resolves to a + WSL shim with no installed distribution; shell checks used Docker fallback. +- Full `npm run lint` remains blocked by pre-existing unrelated lint errors. diff --git a/scripts/backup/restore-path-assertions.sh b/scripts/backup/restore-path-assertions.sh index d5f5711..fdc9c94 100644 --- a/scripts/backup/restore-path-assertions.sh +++ b/scripts/backup/restore-path-assertions.sh @@ -51,6 +51,13 @@ if [[ "${1:-}" == "compose" && "${2:-}" == "config" && "${3:-}" == "--format" && exit 0 fi +if [[ "${1:-}" == "compose" && "${2:-}" == "--profile" && "${3:-}" == "full" && "${4:-}" == "ps" && "${5:-}" == "--status" && "${6:-}" == "running" && "${7:-}" == "-q" ]]; then + case " ${RESTORE_ASSERT_RUNNING_SERVICES:-app worker bot} " in + *" ${8:-} "*) printf '%s-container\n' "${8:-}" ;; + esac + exit 0 +fi + if [[ "${1:-}" == "volume" && "${2:-}" == "ls" ]]; then if [[ " $* " == *"com.docker.compose.volume=tdlib_state"* ]]; then printf 'dragonsstash_tdlib_state\n' @@ -102,6 +109,8 @@ EOF cat > "$fake_bin/curl" <<'EOF' #!/usr/bin/env bash set -Eeuo pipefail +printf '%s\t' "$@" >> "$DRAGONS_STASH_CURL_CALLS" +printf '\n' >> "$DRAGONS_STASH_CURL_CALLS" exit 0 EOF chmod +x "$fake_bin/curl" @@ -126,6 +135,7 @@ run_restore_live() { shift DRAGONS_STASH_DOCKER_CALLS="$TMP_ROOT/docker-calls-$name.log" \ + DRAGONS_STASH_CURL_CALLS="$TMP_ROOT/curl-calls-$name.log" \ "$@" "$RESTORE_SCRIPT" restore-live snapshot-scope --confirm-replace-live-data } @@ -163,6 +173,32 @@ assert_live_restore_uses_include_filters() { assert_restore_call_is_filtered "$calls" } +assert_live_restore_restarts_only_previously_running_services() { + local calls="$TMP_ROOT/docker-calls-live-running-subset.log" + local stop_call + local start_call + + setup_fake_environment + RESTORE_ASSERT_RUNNING_SERVICES="app worker" run_restore_live live-running-subset bash >/dev/null + + stop_call="$(grep $'compose\t--profile\tfull\tstop' "$calls" || true)" + start_call="$(grep $'compose\t--profile\tfull\tup\t-d' "$calls" || true)" + + [[ "$stop_call" == $'compose\t--profile\tfull\tstop\tapp\tworker\t' ]] \ + || fail "restore-live stopped services other than the running app/worker subset: $stop_call" + [[ "$start_call" == $'compose\t--profile\tfull\tup\t-d\tapp\tworker\t' ]] \ + || fail "restore-live started services other than the running app/worker subset: $start_call" +} + +assert_live_restore_skips_health_when_app_was_not_running() { + local calls="$TMP_ROOT/curl-calls-live-without-app.log" + + setup_fake_environment + RESTORE_ASSERT_RUNNING_SERVICES="worker" run_restore_live live-without-app bash >/dev/null + + [[ ! -s "$calls" ]] || fail "restore-live waited for app health even though app was not previously running" +} + assert_unexpected_volume_content_is_rejected() { local output="$TMP_ROOT/unexpected-output.log" @@ -176,6 +212,8 @@ assert_unexpected_volume_content_is_rejected() { assert_restore_uses_include_filters assert_live_restore_uses_include_filters +assert_live_restore_restarts_only_previously_running_services +assert_live_restore_skips_health_when_app_was_not_running assert_unexpected_volume_content_is_rejected printf 'restore-path assertions passed\n' diff --git a/scripts/backup/restore.sh b/scripts/backup/restore.sh index 2889c2a..5746edd 100755 --- a/scripts/backup/restore.sh +++ b/scripts/backup/restore.sh @@ -24,6 +24,7 @@ LIVE_RESTORE_ACTIVE=0 LIVE_REPLACEMENT_STARTED=0 LIVE_WORKER_VOLUME="" LIVE_BOT_VOLUME="" +declare -a RUNNING_LIVE_SERVICES=() usage() { cat <<'EOF' @@ -258,6 +259,33 @@ compose_volume_name() { printf '%s\n' "${matches[0]}" } +capture_running_live_services() { + local service + local container_ids + + RUNNING_LIVE_SERVICES=() + for service in "${LIVE_SERVICES[@]}"; do + if ! container_ids="$(docker compose --profile full ps --status running -q "$service")"; then + printf 'Unable to determine whether Compose service %s is running.\n' "$service" >&2 + return 1 + fi + + if [[ -n "$container_ids" ]]; then + RUNNING_LIVE_SERVICES+=("$service") + fi + done +} + +live_service_was_running() { + local requested_service="$1" + local service + + for service in "${RUNNING_LIVE_SERVICES[@]}"; do + [[ "$service" == "$requested_service" ]] && return 0 + done + return 1 +} + replace_volume() { local source_dir="$1" local volume_name="$2" @@ -347,7 +375,9 @@ live_restore_failure() { local rollback_ok=1 trap - EXIT if ((LIVE_RESTORE_ACTIVE)); then - docker compose --profile full stop "${LIVE_SERVICES[@]}" || true + if ((${#RUNNING_LIVE_SERVICES[@]} > 0)); then + docker compose --profile full stop "${RUNNING_LIVE_SERVICES[@]}" || true + fi if ((LIVE_REPLACEMENT_STARTED)); then restore_live_volume_archive "$LIVE_WORKER_VOLUME" tdlib_state || rollback_ok=0 restore_live_volume_archive "$LIVE_BOT_VOLUME" tdlib_bot_state || rollback_ok=0 @@ -384,7 +414,10 @@ restore_live() { SAFETY_DUMP="$LIVE_STAGING_DIR/pre-restore-database.dump" trap live_restore_failure EXIT LIVE_RESTORE_ACTIVE=1 - docker compose --profile full stop "${LIVE_SERVICES[@]}" + capture_running_live_services + if ((${#RUNNING_LIVE_SERVICES[@]} > 0)); then + docker compose --profile full stop "${RUNNING_LIVE_SERVICES[@]}" + fi create_safety_dump restore_snapshot_subset "$snapshot_id" "$container_staging_dir" validate_restored_tree "$LIVE_STAGING_DIR" @@ -398,8 +431,12 @@ restore_live() { replace_volume "$RESTORED_TDLIB_WORKER" "$worker_volume" replace_volume "$RESTORED_TDLIB_BOT" "$bot_volume" restore_database - docker compose --profile full up -d "${LIVE_SERVICES[@]}" - wait_for_health + if ((${#RUNNING_LIVE_SERVICES[@]} > 0)); then + docker compose --profile full up -d "${RUNNING_LIVE_SERVICES[@]}" + fi + if live_service_was_running app; then + wait_for_health + fi LIVE_RESTORE_ACTIVE=0 trap - EXIT printf 'Live restore completed. Staging directory retained at: %s\n' "$LIVE_STAGING_DIR" diff --git a/scripts/backup/run-backup-assertions.sh b/scripts/backup/run-backup-assertions.sh new file mode 100644 index 0000000..a8b6a95 --- /dev/null +++ b/scripts/backup/run-backup-assertions.sh @@ -0,0 +1,114 @@ +#!/usr/bin/env bash +set -Eeuo pipefail + +SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd -P)" +PROJECT_ROOT="$(cd -- "$SCRIPT_DIR/../.." && pwd -P)" +BACKUP_SCRIPT="$PROJECT_ROOT/scripts/backup/run-backup.sh" +TMP_ROOT="$(mktemp -d)" + +cleanup() { + rm -rf -- "$TMP_ROOT" +} +trap cleanup EXIT + +fail() { + printf 'ASSERTION FAILED: %s\n' "$*" >&2 + exit 1 +} + +setup_fake_environment() { + local fake_bin="$TMP_ROOT/bin" + + mkdir -p -- "$fake_bin" "$TMP_ROOT/mount" "$TMP_ROOT/staging" + printf 'not-secret\n' > "$TMP_ROOT/restic-password" + + cat > "$fake_bin/docker" <<'EOF' +#!/usr/bin/env bash +set -Eeuo pipefail + +printf '%s\t' "$@" >> "$DRAGONS_STASH_DOCKER_CALLS" +printf '\n' >> "$DRAGONS_STASH_DOCKER_CALLS" + +if [[ "${1:-}" == "compose" && "${2:-}" == "ps" && "${3:-}" == "--status" && "${4:-}" == "running" && "${5:-}" == "-q" ]]; then + case "${6:-}" in + app) printf 'app-container\n' ;; + esac + exit 0 +fi + +if [[ "${1:-}" == "compose" && "${2:-}" == "stop" ]]; then + exit 0 +fi + +if [[ "${1:-}" == "compose" && "${2:-}" == "start" ]]; then + exit "${BACKUP_ASSERT_START_EXIT:-0}" +fi + +if [[ " $* " == *" backup backup "* ]]; then + exit "${BACKUP_ASSERT_BACKUP_EXIT:-0}" +fi + +exit 0 +EOF + chmod +x "$fake_bin/docker" + + cat > "$fake_bin/mountpoint" <<'EOF' +#!/usr/bin/env bash +set -Eeuo pipefail +exit 0 +EOF + chmod +x "$fake_bin/mountpoint" + + cat > "$fake_bin/flock" <<'EOF' +#!/usr/bin/env bash +set -Eeuo pipefail +exit 0 +EOF + chmod +x "$fake_bin/flock" + + export PATH="$fake_bin:$PATH" + export BACKUP_MOUNT_PATH="$TMP_ROOT/mount" + export BACKUP_STAGING_PATH="$TMP_ROOT/staging" + export BACKUP_RESTIC_PASSWORD_FILE="$TMP_ROOT/restic-password" + export BACKUP_RETENTION_DAYS="30" + export BACKUP_REPOSITORY="/backup/restic" +} + +run_backup() { + local name="$1" + shift + + DRAGONS_STASH_DOCKER_CALLS="$TMP_ROOT/docker-calls-$name.log" \ + "$@" "$BACKUP_SCRIPT" +} + +assert_restart_failure_makes_successful_backup_fail() { + local output="$TMP_ROOT/restart-fails-output.log" + + setup_fake_environment + if BACKUP_ASSERT_START_EXIT=9 run_backup restart-fails bash >"$output" 2>&1; then + fail "backup reported success when restart failed" + fi + grep -q 'Failed to restart one or more previously running services' "$output" \ + || fail "restart failure did not produce an explicit error" +} + +assert_backup_failure_exit_is_preserved_when_restart_also_fails() { + local output="$TMP_ROOT/backup-and-restart-fail-output.log" + local exit_code + + setup_fake_environment + set +e + BACKUP_ASSERT_BACKUP_EXIT=42 BACKUP_ASSERT_START_EXIT=9 run_backup backup-and-restart-fail bash >"$output" 2>&1 + exit_code=$? + set -e + + [[ "$exit_code" -eq 42 ]] || fail "expected backup failure exit 42 to be preserved; got $exit_code" + grep -q 'Failed to restart one or more previously running services' "$output" \ + || fail "restart failure did not produce an explicit error" +} + +assert_restart_failure_makes_successful_backup_fail +assert_backup_failure_exit_is_preserved_when_restart_also_fails + +printf 'run-backup assertions passed\n' diff --git a/scripts/backup/run-backup.sh b/scripts/backup/run-backup.sh index d0e7909..4f46861 100755 --- a/scripts/backup/run-backup.sh +++ b/scripts/backup/run-backup.sh @@ -71,12 +71,17 @@ capture_running_services() { restart_running_services() { local exit_code=$? + local restart_exit=0 trap - EXIT if ((${#RUNNING_SERVICES[@]} > 0)); then - if ! docker compose start "${RUNNING_SERVICES[@]}"; then + docker compose start "${RUNNING_SERVICES[@]}" || restart_exit=$? + if ((restart_exit != 0)); then printf 'Failed to restart one or more previously running services: %s\n' "${RUNNING_SERVICES[*]}" >&2 + if ((exit_code == 0)); then + exit_code="$restart_exit" + fi fi fi