diff options
| -rwxr-xr-x | rsyncshot | 60 | ||||
| -rwxr-xr-x | tests/cases/test_cron.sh | 30 | ||||
| -rw-r--r-- | tests/cases/test_locking.sh | 107 | ||||
| -rwxr-xr-x | tests/test_rsyncshot.sh | 1 |
4 files changed, 189 insertions, 9 deletions
@@ -149,11 +149,16 @@ RM="rm" # ------------------------------------------------------------------------------ # CONCURRENCY CONTROL # ------------------------------------------------------------------------------ -# Use flock to prevent multiple instances from running simultaneously. -# This is important when cron might trigger a new run before the previous -# one completes (e.g., slow network, large backup). +# rsyncshot acquires an exclusive lock internally (see acquire_lock) before the +# backup phase, so a slow backup overrunning the next cron tick can't overlap. +# Cron entries therefore do NOT need an external flock wrapper. +# +# LOCKFILE is the lock path. When unset, it's chosen at runtime (below): the +# first of /run/lock, /var/lock, /tmp that exists and is writable. /run/lock and +# /var/lock are root-only, which avoids the symlink-truncation risk of a +# predictable name in world-writable /tmp. Override in the config file if needed. -FLOCKCHECK="flock -x /tmp/rsyncshot.lock -c" +LOCKFILE="${LOCKFILE:-}" # ------------------------------------------------------------------------------ # DEFAULT CRON SCHEDULE @@ -242,6 +247,18 @@ require_destination_configured() derive_paths +# Choose a default lock file if neither the config nor the environment set one. +# Prefer root-only dirs over world-writable /tmp. +if [ -z "$LOCKFILE" ]; then + for _lockdir in /run/lock /var/lock /tmp; do + if [ -d "$_lockdir" ] && [ -w "$_lockdir" ]; then + LOCKFILE="$_lockdir/rsyncshot.lock" + break + fi + done + : "${LOCKFILE:=/tmp/rsyncshot.lock}" +fi + # ============================================================================== # UTILITY FUNCTIONS # ============================================================================== @@ -314,6 +331,23 @@ log() } # ------------------------------------------------------------------------------ +# acquire_lock() - Take an exclusive, non-blocking lock for the backup phase +# ------------------------------------------------------------------------------ +# Opens LOCKFILE on fd 200 and flock -n's it. The lock is held until the script +# exits (fd 200 auto-closes), so a second concurrent backup fails fast rather +# than overlapping. Only the real backup path calls this; help, list, status, +# and dryrun never lock, so they're never blocked by a running backup. + +acquire_lock() +{ + exec 200>"$LOCKFILE" || error "cannot open lock file $LOCKFILE" + if ! flock -n 200; then + log "Another rsyncshot instance is already running (lock held on $LOCKFILE) - exiting." + exit 1 + fi +} + +# ------------------------------------------------------------------------------ # run_cmd() - Execute a command locally or remotely based on mode # ------------------------------------------------------------------------------ # Arguments: @@ -738,10 +772,10 @@ EOF CRONTEMP=$(mktemp) crontab -l 2>/dev/null | grep -v "rsyncshot" > "$CRONTEMP" || true { - echo "# rsyncshot automated backups" - echo "$CRON_H $FLOCKCHECK '$SCRIPTLOC hourly 22 >> $LOGFILE 2>&1'" - echo "$CRON_D $FLOCKCHECK '$SCRIPTLOC daily 6 >> $LOGFILE 2>&1'" - echo "$CRON_W $FLOCKCHECK '$SCRIPTLOC weekly 51 >> $LOGFILE 2>&1'" + echo "# rsyncshot automated backups (locks internally; no external wrapper needed)" + echo "$CRON_H $SCRIPTLOC hourly 22 >> $LOGFILE 2>&1" + echo "$CRON_D $SCRIPTLOC daily 6 >> $LOGFILE 2>&1" + echo "$CRON_W $SCRIPTLOC weekly 51 >> $LOGFILE 2>&1" } >> "$CRONTEMP" crontab "$CRONTEMP" $RM "$CRONTEMP" @@ -807,7 +841,7 @@ if ! command -v rsync &> /dev/null; then fi if ! command -v flock &> /dev/null; then - echo "ERROR: flock is not installed (required for cron job locking)." 1>&2 + echo "ERROR: flock is not installed (required for concurrency locking)." 1>&2 echo "" echo "Install flock using your package manager:" echo " Arch Linux: sudo pacman -S util-linux" @@ -888,6 +922,14 @@ if [ ! -f "$EXCLUDES" ]; then error "Exclude file $EXCLUDES not found."; fi # --- Refuse to operate against a filesystem root --- require_destination_configured +# --- Acquire exclusive lock before any destructive or mutating work --- +# Real backups only; dryrun and read-only commands are never blocked. Taken +# before the mode-specific validation so even the local mount attempt is +# serialized. +if [ "$DRYRUN" = false ]; then + acquire_lock +fi + # --- Mode-specific destination validation --- if [ "$MODE" = "remote" ]; then # Verify SSH identity file exists if specified diff --git a/tests/cases/test_cron.sh b/tests/cases/test_cron.sh index 5c9bbfa..7e38060 100755 --- a/tests/cases/test_cron.sh +++ b/tests/cases/test_cron.sh @@ -119,6 +119,35 @@ EOF } # ------------------------------------------------------------------------------ +# Test: Cron entries no longer wrap rsyncshot in an external flock +# ------------------------------------------------------------------------------ +# rsyncshot locks internally now, so setup should write bare cron commands. +test_cron_no_flock_wrapper() { + setup_test_env + save_crontab + + crontab -r 2>/dev/null || true + + cat > "$TEST_CONFIG_DIR/config" << EOF +REMOTE_HOST="" +MOUNTDIR="$TEST_BACKUP_DIR" +EOF + create_test_includes + create_test_excludes + + sudo INSTALLHOME="$TEST_CONFIG_DIR" SCRIPTLOC="/tmp/rsyncshot-test" RSYNCSHOT_SKIP_MOUNT_CHECK=1 "$SCRIPT_PATH" setup >/dev/null 2>&1 || true + + local crontab_content + crontab_content=$(crontab -l 2>/dev/null) + + restore_crontab + teardown_test_env + + assert_not_contains "$crontab_content" "flock" "cron entries should not wrap rsyncshot in flock (internal locking)" || return 1 + assert_contains "$crontab_content" "rsyncshot" "cron should still contain rsyncshot entries" || return 1 +} + +# ------------------------------------------------------------------------------ # Run tests # ------------------------------------------------------------------------------ run_cron_tests() { @@ -129,6 +158,7 @@ run_cron_tests() { run_test "setup adds cron jobs" test_setup_adds_cron_jobs run_test "repeated setup doesn't duplicate entries" test_no_duplicate_cron_entries run_test "setup preserves existing cron jobs" test_preserves_existing_cron + run_test "cron entries have no flock wrapper" test_cron_no_flock_wrapper } # Run if executed directly diff --git a/tests/cases/test_locking.sh b/tests/cases/test_locking.sh new file mode 100644 index 0000000..4667d58 --- /dev/null +++ b/tests/cases/test_locking.sh @@ -0,0 +1,107 @@ +#!/usr/bin/env bash +# ============================================================================== +# Concurrency Locking Tests +# ============================================================================== +# rsyncshot acquires an exclusive lock before the backup phase. These tests hold +# the lock in the test process (a separate open file description), then run the +# script with LOCKFILE pointed at the same file, so the script's flock -n fails +# exactly as a second concurrent backup would. + +source "$(dirname "${BASH_SOURCE[0]}")/../lib/test_helpers.sh" + +# ------------------------------------------------------------------------------ +# A real backup refuses to start while another holds the lock +# ------------------------------------------------------------------------------ +test_refuses_when_lock_held() { + setup_test_env + create_test_config >/dev/null + create_test_includes >/dev/null + create_test_excludes >/dev/null + + local lock="$TEST_DIR/rsyncshot.lock" + exec 9>"$lock" + if ! flock -n 9; then + echo "FAIL: test could not acquire the lock to set up the scenario" + exec 9>&- + teardown_test_env + return 1 + fi + + local output rc + output=$(INSTALLHOME="$TEST_CONFIG_DIR" LOCKFILE="$lock" RSYNCSHOT_SKIP_MOUNT_CHECK=1 "$SCRIPT_PATH" manual 1 2>&1) + rc=$? + exec 9>&- # release the lock for later tests + + assert_exit_code 1 "$rc" "should exit 1 when another instance holds the lock" || { teardown_test_env; return 1; } + assert_contains "$output" "already running" "should report the lock contention" || { teardown_test_env; return 1; } + + teardown_test_env +} + +# ------------------------------------------------------------------------------ +# A dryrun is read-only and must NOT be blocked by a held lock +# ------------------------------------------------------------------------------ +test_dryrun_not_blocked_by_lock() { + setup_test_env + create_test_config >/dev/null + create_test_includes >/dev/null + create_test_excludes >/dev/null + + local lock="$TEST_DIR/rsyncshot.lock" + exec 9>"$lock" + if ! flock -n 9; then + echo "FAIL: test could not acquire the lock to set up the scenario" + exec 9>&- + teardown_test_env + return 1 + fi + + local rc + INSTALLHOME="$TEST_CONFIG_DIR" LOCKFILE="$lock" RSYNCSHOT_SKIP_MOUNT_CHECK=1 "$SCRIPT_PATH" dryrun manual 1 >/dev/null 2>&1 + rc=$? + exec 9>&- + + assert_exit_code 0 "$rc" "dryrun should not be blocked by a held lock" || { teardown_test_env; return 1; } + + teardown_test_env +} + +# ------------------------------------------------------------------------------ +# Two sequential real backups both succeed (lock is released on exit) +# ------------------------------------------------------------------------------ +test_sequential_runs_each_acquire_lock() { + setup_test_env + create_test_config >/dev/null + create_test_includes >/dev/null + create_test_excludes >/dev/null + + local lock="$TEST_DIR/rsyncshot.lock" + INSTALLHOME="$TEST_CONFIG_DIR" LOCKFILE="$lock" RSYNCSHOT_SKIP_MOUNT_CHECK=1 "$SCRIPT_PATH" manual 2 >/dev/null 2>&1 + local rc1=$? + INSTALLHOME="$TEST_CONFIG_DIR" LOCKFILE="$lock" RSYNCSHOT_SKIP_MOUNT_CHECK=1 "$SCRIPT_PATH" manual 2 >/dev/null 2>&1 + local rc2=$? + + assert_exit_code 0 "$rc1" "first run should succeed and release the lock" || { teardown_test_env; return 1; } + assert_exit_code 0 "$rc2" "second run should acquire the freed lock and succeed" || { teardown_test_env; return 1; } + + teardown_test_env +} + +# ------------------------------------------------------------------------------ +# Run tests +# ------------------------------------------------------------------------------ +run_locking_tests() { + echo "" + echo "Running locking tests..." + echo "------------------------------------------------------------" + + run_test "refuses to run when the lock is held" test_refuses_when_lock_held + run_test "dryrun is not blocked by the lock" test_dryrun_not_blocked_by_lock + run_test "sequential runs each acquire the lock" test_sequential_runs_each_acquire_lock +} + +# Run if executed directly +if [[ "${BASH_SOURCE[0]}" == "${0}" ]]; then + run_locking_tests + print_summary +fi diff --git a/tests/test_rsyncshot.sh b/tests/test_rsyncshot.sh index 95a0032..857dd95 100755 --- a/tests/test_rsyncshot.sh +++ b/tests/test_rsyncshot.sh @@ -95,6 +95,7 @@ run_test_file "$SCRIPT_DIR/cases/test_functions.sh" "functions" run_test_file "$SCRIPT_DIR/cases/test_includes.sh" "includes" run_test_file "$SCRIPT_DIR/cases/test_dryrun.sh" "dryrun" run_test_file "$SCRIPT_DIR/cases/test_rsync_flags.sh" "rsync_flags" +run_test_file "$SCRIPT_DIR/cases/test_locking.sh" "locking" if [ "$QUICK" = false ]; then run_test_file "$SCRIPT_DIR/cases/test_backup.sh" "backup" |