fix: improve backup script error handling and validation

- Add prerequisite checks before backup operations
- Validate container status before attempting backups
- Load .env file for database credentials (security improvement)
- Remove hardcoded default password from Nextcloud backup
- Use centralized config for container names
- Add --single-transaction flag for database dumps
- Improve cleanup with better reporting of removed files
- Add help command and environment variable documentation

bin/backup.sh

@@ -1,36 +1,67 @@
 #!/usr/bin/env bash
 # Backup utility for all services
-# Usage: ./bin/backup.sh [service]
+# Usage: ./bin/backup.sh [command] [service]
 
 set -euo pipefail
 
-# Source shared library
+# Source shared library and config
 SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+PROJECT_ROOT="$(dirname "$SCRIPT_DIR")"
 source "$SCRIPT_DIR/lib/common.sh"
+source "$PROJECT_ROOT/config.sh"
 
-readonly BACKUP_ROOT="${BACKUP_ROOT:-./backups}"
 readonly TIMESTAMP=$(date +%Y%m%d-%H%M%S)
 
+# ============================================================================
+# Pre-flight checks
+# ============================================================================
+check_prerequisites() {
+  if ! require_command "docker"; then
+    log_error "Docker is required for backup operations"
+    exit 1
+  fi
+}
+
+check_container_running() {
+  local container_name="$1"
+  local service_name="$2"
+
+  if ! check_container_health "$container_name"; then
+    log_warn "$service_name container ($container_name) is not running"
+    log_warn "Some backup operations may fail"
+    return 1
+  fi
+  return 0
+}
+
+# ============================================================================
+# Backup functions
+# ============================================================================
 backup_minecraft() {
   log_info "Backing up Minecraft server..."
 
   local backup_dir="$BACKUP_ROOT/minecraft/$TIMESTAMP"
   ensure_dir "$backup_dir"
 
+  # Check if container is running (warning only)
+  check_container_running "$CONTAINER_MINECRAFT" "Minecraft" || true
+
   # Backup world data
-  if [[ -d "minecraft/data" ]]; then
+  if [[ -d "$PROJECT_ROOT/minecraft/data" ]]; then
     log_info "  Archiving world data..."
-    tar -czf "$backup_dir/world-data.tar.gz" -C minecraft data 2>/dev/null || {
+    tar -czf "$backup_dir/world-data.tar.gz" -C "$PROJECT_ROOT/minecraft" data 2>/dev/null || {
       log_error "  Failed to backup world data"
       return 1
     }
     log_info "  ✓ World data backed up"
+  else
+    log_warn "  No world data directory found"
   fi
 
   # Backup configs
-  if [[ -d "minecraft/configs" ]]; then
+  if [[ -d "$PROJECT_ROOT/minecraft/configs" ]]; then
     log_info "  Archiving configs..."
-    tar -czf "$backup_dir/configs.tar.gz" -C minecraft configs 2>/dev/null || {
+    tar -czf "$backup_dir/configs.tar.gz" -C "$PROJECT_ROOT/minecraft" configs 2>/dev/null || {
       log_warn "  Failed to backup configs"
     }
   fi
@@ -40,6 +71,7 @@ backup_minecraft() {
 Minecraft Backup
 Created: $(date)
 Location: $backup_dir
+Container: $CONTAINER_MINECRAFT
 Contents:
   - World data
   - Configuration files
@@ -54,6 +86,9 @@ backup_teamspeak() {
   local backup_dir="$BACKUP_ROOT/teamspeak/$TIMESTAMP"
   ensure_dir "$backup_dir"
 
+  # Check if container is running
+  check_container_running "$CONTAINER_TEAMSPEAK" "TeamSpeak" || true
+
   # Export Docker volume
   if docker volume ls | grep -q teamspeak_data; then
     log_info "  Exporting volume data..."
@@ -63,6 +98,8 @@ backup_teamspeak() {
       return 1
     }
     log_info "  ✓ Volume data backed up"
+  else
+    log_warn "  No TeamSpeak volume found"
   fi
 
   log_info "  ✓ Backup complete: $backup_dir"
@@ -74,12 +111,40 @@ backup_nextcloud() {
   local backup_dir="$BACKUP_ROOT/nextcloud/$TIMESTAMP"
   ensure_dir "$backup_dir"
 
-  # Backup database
+  # Load Nextcloud environment if available
+  local nextcloud_env="$PROJECT_ROOT/nextcloud/.env"
+  if [[ -f "$nextcloud_env" ]]; then
+    log_info "  Loading Nextcloud environment..."
+    load_env "$nextcloud_env"
+  else
+    log_warn "  No .env file found at $nextcloud_env"
+    log_warn "  Using default credentials (not recommended)"
+  fi
+
+  # Validate required environment variables
+  if [[ -z "${MYSQL_PASSWORD:-}" ]]; then
+    log_warn "  MYSQL_PASSWORD not set, database backup may fail"
+  fi
+
+  # Check if database container is running
+  if ! check_container_running "$CONTAINER_NEXTCLOUD_DB" "Nextcloud DB"; then
+    log_error "  Database container must be running for backup"
+    return 1
+  fi
+
+  # Backup database (use environment variable, no default password)
   log_info "  Backing up database..."
-  docker exec nextcloud-db mariadb-dump -unextcloud -p"${MYSQL_PASSWORD:-ChangeDb123!}" nextcloud \
+  if [[ -n "${MYSQL_PASSWORD:-}" ]]; then
-    > "$backup_dir/database.sql" 2>/dev/null || {
+    docker exec "$CONTAINER_NEXTCLOUD_DB" mariadb-dump \
-    log_error "  Database backup failed"
+      -u"${MYSQL_USER:-nextcloud}" \
-  }
+      -p"$MYSQL_PASSWORD" \
+      --single-transaction \
+      "${MYSQL_DATABASE:-nextcloud}" > "$backup_dir/database.sql" 2>/dev/null || {
+      log_error "  Database backup failed"
+    }
+  else
+    log_error "  Skipping database backup: MYSQL_PASSWORD not set"
+  fi
 
   # Export volumes
   for vol in nextcloud_html nextcloud_data nextcloud_config nextcloud_apps; do
@@ -97,6 +162,7 @@ backup_nextcloud() {
 Nextcloud Backup
 Created: $(date)
 Location: $backup_dir
+Containers: $CONTAINER_NEXTCLOUD, $CONTAINER_NEXTCLOUD_DB, $CONTAINER_NEXTCLOUD_REDIS
 Contents:
   - MariaDB database dump
   - Application volumes
@@ -106,31 +172,84 @@ EOF
   log_info "  ✓ Backup complete: $backup_dir"
 }
 
+# ============================================================================
+# Utility functions
+# ============================================================================
 list_backups() {
   log_info "Available backups:"
   echo
 
+  local found=0
   for service in minecraft teamspeak nextcloud; do
     if [[ -d "$BACKUP_ROOT/$service" ]]; then
+      found=1
       echo "=== $service ==="
-      ls -lh "$BACKUP_ROOT/$service" | tail -n +2
+      ls -lh "$BACKUP_ROOT/$service" 2>/dev/null | tail -n +2 || echo "  (empty)"
       echo
     fi
   done
+
+  if [[ $found -eq 0 ]]; then
+    log_info "No backups found in $BACKUP_ROOT"
+  fi
 }
 
 cleanup_old_backups() {
-  local keep_days="${1:-7}"
+  local keep_days="${1:-$BACKUP_RETENTION_DAYS}"
+
+  if [[ ! -d "$BACKUP_ROOT" ]]; then
+    log_info "No backup directory found"
+    return 0
+  fi
 
   log_info "Cleaning up backups older than $keep_days days..."
 
-  find "$BACKUP_ROOT" -type f -name "*.tar.gz" -mtime +"$keep_days" -delete
+  local count_before
-  find "$BACKUP_ROOT" -type d -empty -delete
+  count_before=$(find "$BACKUP_ROOT" -type f -name "*.tar.gz" 2>/dev/null | wc -l)
 
-  log_info "  ✓ Cleanup complete"
+  find "$BACKUP_ROOT" -type f -name "*.tar.gz" -mtime +"$keep_days" -delete 2>/dev/null || true
+  find "$BACKUP_ROOT" -type f -name "*.sql" -mtime +"$keep_days" -delete 2>/dev/null || true
+  find "$BACKUP_ROOT" -type f -name "manifest.txt" -mtime +"$keep_days" -delete 2>/dev/null || true
+  find "$BACKUP_ROOT" -type d -empty -delete 2>/dev/null || true
+
+  local count_after
+  count_after=$(find "$BACKUP_ROOT" -type f -name "*.tar.gz" 2>/dev/null | wc -l)
+
+  local removed=$((count_before - count_after))
+  log_info "  ✓ Cleanup complete (removed $removed archive(s))"
+}
+
+# ============================================================================
+# Main
+# ============================================================================
+show_usage() {
+  cat <<EOF
+Usage: $0 <command> [options]
+
+Commands:
+  backup [service]      Create backup (default: all)
+  list                  List available backups
+  cleanup [days]        Remove backups older than N days (default: $BACKUP_RETENTION_DAYS)
+
+Services:
+  minecraft, teamspeak, nextcloud, all
+
+Examples:
+  $0 backup minecraft
+  $0 backup all
+  $0 list
+  $0 cleanup 30
+
+Environment:
+  BACKUP_ROOT           Backup directory (default: ./backups)
+  BACKUP_RETENTION_DAYS Days to keep backups (default: 7)
+EOF
+  exit 1
 }
 
 main() {
+  check_prerequisites
+
   local action="${1:-backup}"
   local service="${2:-all}"
 
@@ -152,8 +271,8 @@ main() {
           backup_nextcloud || true
           ;;
         *)
-          echo "Usage: $0 backup [minecraft|teamspeak|nextcloud|all]"
+          log_error "Unknown service: $service"
-          exit 1
+          show_usage
           ;;
       esac
       ;;
@@ -161,26 +280,14 @@ main() {
       list_backups
       ;;
     cleanup)
-      cleanup_old_backups "${service:-7}"
+      cleanup_old_backups "${service:-$BACKUP_RETENTION_DAYS}"
+      ;;
+    -h|--help|help)
+      show_usage
       ;;
     *)
-      cat <<EOF
+      log_error "Unknown command: $action"
-Usage: $0 <command> [options]
+      show_usage
-
-Commands:
-  backup [service]      Create backup (default: all)
-  list                  List available backups
-  cleanup [days]        Remove backups older than N days (default: 7)
-
-Services:
-  minecraft, teamspeak, nextcloud, all
-
-Examples:
-  $0 backup minecraft
-  $0 list
-  $0 cleanup 30
-EOF
-      exit 1
       ;;
   esac
 }