From 8f351ae4d14fc2abcc83716a192dfacff6dc65ef Mon Sep 17 00:00:00 2001 From: Craig Jennings Date: Thu, 25 Jun 2026 01:37:41 -0400 Subject: feat: lock backups internally so cron needs no flock wrapper rsyncshot now acquires an exclusive, non-blocking lock (fd 200) before the backup phase, so a slow backup can't overlap the next cron tick. Only real backups lock. Help, list, status, and dryrun don't, so they're never blocked. The lock file defaults to /run/lock/rsyncshot.lock, a root-only directory that avoids the symlink-truncation risk of a predictable name in world-writable /tmp. It falls back to /var/lock then /tmp, and is configurable via LOCKFILE. It's deliberately a different file from the old /tmp/rsyncshot.lock cron wrappers, so existing wrapped crontabs keep working until the wrapper is removed. setup() no longer wraps cron entries in flock, since the script locks itself. --- tests/cases/test_cron.sh | 30 +++++++++++++ tests/cases/test_locking.sh | 107 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 137 insertions(+) create mode 100644 tests/cases/test_locking.sh (limited to 'tests/cases') 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 @@ -118,6 +118,35 @@ EOF assert_contains "$crontab_content" "rsyncshot" "should also have rsyncshot jobs" || return 1 } +# ------------------------------------------------------------------------------ +# 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 # ------------------------------------------------------------------------------ @@ -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 -- cgit v1.2.3