qr0: timeout + per-destination isolation for rsync sync_dist

Wrap each rsync in sync_dist in run_with_timeout (new SYNC_TIMEOUT config,
default 300s) so a hung or unreachable mirror cannot block the whole sync,
matching every other external call. Make destinations isolated: under
set -euo pipefail a single failing destination used to abort the loop and
silently skip the rest. Now each destination runs under a localized set +e
(the project's refresh_splash idiom), results are collected per destination,
a clear pass/fail summary is logged, and sync returns non-zero if any
destination failed while still attempting all of them.

SYNC_TIMEOUT is plumbed like TAR_TIMEOUT: shuriken.default.conf,
apply_config_defaults, print_config, verbose config log, and positive-integer
validation in both validate_config (generate path) and validate_sync_config
(sync path). No CLI flag, matching TAR_TIMEOUT.

Tests: a sync where one destination fails still attempts the others and exits
non-zero with the summary; SYNC_TIMEOUT=0 is rejected as a positive integer.
Adds install_rsync_spy_failing_one helper and updates the print_config
expected blocks.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

9 files changed, 224 insertions, 2 deletions

diff --git a/bin/shuriken b/bin/shuriken
index cc8cac2..2e2f8ee 100755
--- a/bin/shuriken
+++ b/bin/shuriken
@@ -5399,6 +5399,10 @@ apply_config_defaults() {
     THUMB_SUBDIVIDE_PERCENT="${THUMB_SUBDIVIDE_PERCENT:-30}"
     THUMB_FEATURE_PERCENT="${THUMB_FEATURE_PERCENT:-10}"
     SYNC_DELETE="${SYNC_DELETE:-yes}"
+    # Per-destination rsync timeout (seconds), mirroring TAR_TIMEOUT/
+    # IMAGEMAGICK_TIMEOUT. Each destination in sync_dist is wrapped in
+    # run_with_timeout so a hung/unreachable mirror cannot block the whole sync.
+    SYNC_TIMEOUT="${SYNC_TIMEOUT:-300}"
     # Default 'yes': a tarball of the incoming dir is included in the dist unless
     # disabled. This must match the documented default in shuriken.default.conf
     # (TARBALL_INCLUDE=yes) -- it previously drifted to 'no' here. 'yes' is the
@@ -5463,6 +5467,7 @@ print_config() {
     print_shell_assignment TAR_TIMEOUT "$TAR_TIMEOUT"
     print_shell_array_assignment TAR_OPTS "${tar_opts[@]}"
     print_shell_assignment SYNC_DELETE "$SYNC_DELETE"
+    print_shell_assignment SYNC_TIMEOUT "$SYNC_TIMEOUT"
     print_shell_array_assignment SYNC_DESTINATIONS "${sync_destinations[@]}"
     print_shell_assignment ORIGINAL_BASEPATH "$ORIGINAL_BASEPATH"
 }
@@ -5510,6 +5515,9 @@ sync_dist() {
     local destination
     local -a rsync_args=(-av)
     local -a sync_destinations=()
+    local -a succeeded=()
+    local -a failed=()
+    local -i status=0
 
     resolve_sync_destinations sync_destinations
 
@@ -5517,10 +5525,43 @@ sync_dist() {
         rsync_args+=(--delete)
     fi
 
+    # Mirror to every destination with per-destination isolation: a timeout or
+    # rsync error on one mirror must NOT abort the others (under the top-level
+    # "set -euo pipefail" a bare failing rsync would otherwise kill the loop and
+    # silently skip the remaining destinations). We wrap each rsync in
+    # run_with_timeout (SYNC_TIMEOUT) like every other external call, and use the
+    # project's localized "set +e" idiom (see refresh_splash / bash-best-
+    # practices) to capture each destination's status instead of aborting. The
+    # "if ! run_with_timeout ...; then" form is avoided here because its non-zero
+    # branch would still leave $? ambiguous for the summary; the explicit status
+    # capture keeps the per-destination result unambiguous.
     for destination in "${sync_destinations[@]}"; do
         log_info "Syncing $DIST_DIR/ to $destination"
-        rsync "${rsync_args[@]}" "$DIST_DIR/" "$destination"
+        set +e
+        run_with_timeout "rsync to $destination" "$SYNC_TIMEOUT" \
+            rsync "${rsync_args[@]}" "$DIST_DIR/" "$destination"
+        status=$?
+        set -e
+        if (( status == 0 )); then
+            succeeded+=("$destination")
+        else
+            # run_with_timeout already prints a timeout/error line; add a
+            # per-destination notice so the operator sees which mirror failed.
+            failed+=("$destination")
+            log_warning "Sync to $destination failed (status $status)"
+        fi
     done
+
+    # Log a clear pass/fail summary and return non-zero if ANY destination
+    # failed, while having ATTEMPTED all of them.
+    if (( ${#succeeded[@]} > 0 )); then
+        log_info "Sync succeeded for: ${succeeded[*]}"
+    fi
+    if (( ${#failed[@]} > 0 )); then
+        log_warning "Sync failed for: ${failed[*]}"
+        return 1
+    fi
+    return 0
 }
 
 # Inlined from src/lib/config.staging.source.sh
@@ -6045,6 +6086,7 @@ validate_common_config() {
     validate_positive_integer_config_var IMAGE_JOBS || return
     validate_positive_integer_config_var IMAGEMAGICK_TIMEOUT || return
     validate_positive_integer_config_var TAR_TIMEOUT || return
+    validate_positive_integer_config_var SYNC_TIMEOUT || return
     validate_yes_no_config_var SHUFFLE || return
     validate_yes_no_config_var SPLASH_PAGE || return
     validate_yes_no_config_var STATS_PAGE || return
@@ -6120,6 +6162,11 @@ validate_rsync() {
 validate_sync_config() {
     require_config_var DIST_DIR || return
     validate_yes_no_config_var SYNC_DELETE || return
+    # SYNC_TIMEOUT bounds each per-destination rsync in sync_dist, so it must be
+    # a positive integer on the sync path too (the generate path validates it via
+    # validate_config). Without this, a bogus value would only surface as a
+    # confusing "timeout: invalid time interval" at run time.
+    validate_positive_integer_config_var SYNC_TIMEOUT || return
     validate_sync_destinations || return
 
     if [[ ! -d "$DIST_DIR" || ! -r "$DIST_DIR" || ! -x "$DIST_DIR" ]]; then
@@ -6440,6 +6487,7 @@ log_configured_action() {
     log_verbose "Effective stats page setting: $STATS_PAGE"
     log_verbose "Effective tarball setting: $TARBALL_INCLUDE"
     log_verbose "Effective sync delete setting: $SYNC_DELETE"
+    log_verbose "Effective sync timeout: ${SYNC_TIMEOUT}s"
     log_verbose "Effective force generation setting: $SHURIKEN_FORCE_GENERATE"
 }
 
diff --git a/src/lib/action.source.sh b/src/lib/action.source.sh
index 36fef40..f3e6c2b 100644
--- a/src/lib/action.source.sh
+++ b/src/lib/action.source.sh
@@ -163,6 +163,7 @@ log_configured_action() {
     log_verbose "Effective stats page setting: $STATS_PAGE"
     log_verbose "Effective tarball setting: $TARBALL_INCLUDE"
     log_verbose "Effective sync delete setting: $SYNC_DELETE"
+    log_verbose "Effective sync timeout: ${SYNC_TIMEOUT}s"
     log_verbose "Effective force generation setting: $SHURIKEN_FORCE_GENERATE"
 }
 
diff --git a/src/lib/config.print.source.sh b/src/lib/config.print.source.sh
index 62ff145..fce3ea3 100644
--- a/src/lib/config.print.source.sh
+++ b/src/lib/config.print.source.sh
@@ -46,6 +46,7 @@ print_config() {
     print_shell_assignment TAR_TIMEOUT "$TAR_TIMEOUT"
     print_shell_array_assignment TAR_OPTS "${tar_opts[@]}"
     print_shell_assignment SYNC_DELETE "$SYNC_DELETE"
+    print_shell_assignment SYNC_TIMEOUT "$SYNC_TIMEOUT"
     print_shell_array_assignment SYNC_DESTINATIONS "${sync_destinations[@]}"
     print_shell_assignment ORIGINAL_BASEPATH "$ORIGINAL_BASEPATH"
 }
diff --git a/src/lib/config.source.sh b/src/lib/config.source.sh
index 9dd48ee..caa2fe3 100644
--- a/src/lib/config.source.sh
+++ b/src/lib/config.source.sh
@@ -52,6 +52,10 @@ apply_config_defaults() {
     THUMB_SUBDIVIDE_PERCENT="${THUMB_SUBDIVIDE_PERCENT:-30}"
     THUMB_FEATURE_PERCENT="${THUMB_FEATURE_PERCENT:-10}"
     SYNC_DELETE="${SYNC_DELETE:-yes}"
+    # Per-destination rsync timeout (seconds), mirroring TAR_TIMEOUT/
+    # IMAGEMAGICK_TIMEOUT. Each destination in sync_dist is wrapped in
+    # run_with_timeout so a hung/unreachable mirror cannot block the whole sync.
+    SYNC_TIMEOUT="${SYNC_TIMEOUT:-300}"
     # Default 'yes': a tarball of the incoming dir is included in the dist unless
     # disabled. This must match the documented default in shuriken.default.conf
     # (TARBALL_INCLUDE=yes) -- it previously drifted to 'no' here. 'yes' is the
diff --git a/src/lib/config.sync.source.sh b/src/lib/config.sync.source.sh
index f4df169..35a7185 100644
--- a/src/lib/config.sync.source.sh
+++ b/src/lib/config.sync.source.sh
@@ -40,6 +40,9 @@ sync_dist() {
     local destination
     local -a rsync_args=(-av)
     local -a sync_destinations=()
+    local -a succeeded=()
+    local -a failed=()
+    local -i status=0
 
     resolve_sync_destinations sync_destinations
 
@@ -47,8 +50,41 @@ sync_dist() {
         rsync_args+=(--delete)
     fi
 
+    # Mirror to every destination with per-destination isolation: a timeout or
+    # rsync error on one mirror must NOT abort the others (under the top-level
+    # "set -euo pipefail" a bare failing rsync would otherwise kill the loop and
+    # silently skip the remaining destinations). We wrap each rsync in
+    # run_with_timeout (SYNC_TIMEOUT) like every other external call, and use the
+    # project's localized "set +e" idiom (see refresh_splash / bash-best-
+    # practices) to capture each destination's status instead of aborting. The
+    # "if ! run_with_timeout ...; then" form is avoided here because its non-zero
+    # branch would still leave $? ambiguous for the summary; the explicit status
+    # capture keeps the per-destination result unambiguous.
     for destination in "${sync_destinations[@]}"; do
         log_info "Syncing $DIST_DIR/ to $destination"
-        rsync "${rsync_args[@]}" "$DIST_DIR/" "$destination"
+        set +e
+        run_with_timeout "rsync to $destination" "$SYNC_TIMEOUT" \
+            rsync "${rsync_args[@]}" "$DIST_DIR/" "$destination"
+        status=$?
+        set -e
+        if (( status == 0 )); then
+            succeeded+=("$destination")
+        else
+            # run_with_timeout already prints a timeout/error line; add a
+            # per-destination notice so the operator sees which mirror failed.
+            failed+=("$destination")
+            log_warning "Sync to $destination failed (status $status)"
+        fi
     done
+
+    # Log a clear pass/fail summary and return non-zero if ANY destination
+    # failed, while having ATTEMPTED all of them.
+    if (( ${#succeeded[@]} > 0 )); then
+        log_info "Sync succeeded for: ${succeeded[*]}"
+    fi
+    if (( ${#failed[@]} > 0 )); then
+        log_warning "Sync failed for: ${failed[*]}"
+        return 1
+    fi
+    return 0
 }
diff --git a/src/lib/config.validate.source.sh b/src/lib/config.validate.source.sh
index 7b1421b..f0b14bf 100644
--- a/src/lib/config.validate.source.sh
+++ b/src/lib/config.validate.source.sh
@@ -291,6 +291,7 @@ validate_common_config() {
     validate_positive_integer_config_var IMAGE_JOBS || return
     validate_positive_integer_config_var IMAGEMAGICK_TIMEOUT || return
     validate_positive_integer_config_var TAR_TIMEOUT || return
+    validate_positive_integer_config_var SYNC_TIMEOUT || return
     validate_yes_no_config_var SHUFFLE || return
     validate_yes_no_config_var SPLASH_PAGE || return
     validate_yes_no_config_var STATS_PAGE || return
@@ -366,6 +367,11 @@ validate_rsync() {
 validate_sync_config() {
     require_config_var DIST_DIR || return
     validate_yes_no_config_var SYNC_DELETE || return
+    # SYNC_TIMEOUT bounds each per-destination rsync in sync_dist, so it must be
+    # a positive integer on the sync path too (the generate path validates it via
+    # validate_config). Without this, a bogus value would only surface as a
+    # confusing "timeout: invalid time interval" at run time.
+    validate_positive_integer_config_var SYNC_TIMEOUT || return
     validate_sync_destinations || return
 
     if [[ ! -d "$DIST_DIR" || ! -r "$DIST_DIR" || ! -x "$DIST_DIR" ]]; then
diff --git a/src/shuriken.default.conf b/src/shuriken.default.conf
index 28da780..eaa30a4 100644
--- a/src/shuriken.default.conf
+++ b/src/shuriken.default.conf
@@ -63,6 +63,10 @@ TAR_OPTS=(-c)
 # SYNC_DESTINATIONS=( '/path/with spaces/' ). A scalar string is rejected with
 # an error because word-splitting would break destinations containing spaces.
 SYNC_DELETE=yes
+# Timeout in seconds for each rsync to a single destination. If one mirror hangs
+# or times out, the remaining destinations are still attempted and --sync still
+# reports failure.
+SYNC_TIMEOUT=300
 SYNC_DESTINATIONS=(
 #    admin@fishfinger.buetow.org:/var/www/htdocs/example.org/
 #    admin@blowfish.buetow.org:/var/www/htdocs/example.org/
diff --git a/tests/cli.sh b/tests/cli.sh
index 28aacbb..2280a7e 100755
--- a/tests/cli.sh
+++ b/tests/cli.sh
@@ -1494,6 +1494,7 @@ TARBALL_SUFFIX=.tar
 TAR_TIMEOUT=120
 TAR_OPTS=( -c )
 SYNC_DELETE=yes
+SYNC_TIMEOUT=300
 SYNC_DESTINATIONS=( )
 ORIGINAL_BASEPATH=''
 EOF
@@ -1545,6 +1546,7 @@ TARBALL_SUFFIX=.tar
 TAR_TIMEOUT=120
 TAR_OPTS=( -c )
 SYNC_DELETE=yes
+SYNC_TIMEOUT=300
 SYNC_DESTINATIONS=( )
 ORIGINAL_BASEPATH=''
 EOF
@@ -1689,6 +1691,7 @@ TARBALL_SUFFIX=.tar
 TAR_TIMEOUT=120
 TAR_OPTS=( -c )
 SYNC_DELETE=yes
+SYNC_TIMEOUT=300
 SYNC_DESTINATIONS=( )
 ORIGINAL_BASEPATH=''
 EOF
@@ -1735,6 +1738,7 @@ TARBALL_SUFFIX=.tar
 TAR_TIMEOUT=120
 TAR_OPTS=( -c )
 SYNC_DELETE=yes
+SYNC_TIMEOUT=300
 SYNC_DESTINATIONS=( )
 ORIGINAL_BASEPATH=''
 EOF
@@ -1808,6 +1812,7 @@ TARBALL_SUFFIX=.tar
 TAR_TIMEOUT=120
 TAR_OPTS=( -c )
 SYNC_DELETE=yes
+SYNC_TIMEOUT=300
 SYNC_DESTINATIONS=( )
 ORIGINAL_BASEPATH=''
 EOF
@@ -6155,6 +6160,85 @@ test_sync_array_destination_preserves_spaces() {
     test::teardown
 }
 
+test_sync_isolates_failing_destination_and_reports_failure() {
+    # Regression for qr0: a failed destination must NOT abort the loop. The first
+    # destination fails, but rsync must still be invoked for the second, and the
+    # overall --sync must exit non-zero with a clear pass/fail summary.
+    local config_file
+    local dist_dir
+    local fake_bin
+    local rsync_log
+    local rsync_output
+    local sync_output
+    local fail_dest='admin@one.example:/var/www/one/'
+    local ok_dest='admin@two.example:/var/www/two/'
+
+    test::setup
+    fake_bin="$TEST_TMPDIR/bin"
+    config_file="$TEST_TMPDIR/shuriken.conf"
+    dist_dir="$TEST_TMPDIR/dist"
+    rsync_log="$TEST_TMPDIR/rsync.log"
+
+    test::install_rsync_spy_failing_one "$fake_bin"
+    mkdir -p "$dist_dir"
+    printf 'generated\n' > "$dist_dir/index.html"
+    {
+        printf 'DIST_DIR=%q\n' "$dist_dir"
+        printf 'SYNC_DESTINATIONS=(\n'
+        printf '    %q\n' "$fail_dest"
+        printf '    %q\n' "$ok_dest"
+        printf ')\n'
+    } > "$config_file"
+
+    sync_output=$(
+        cd "$TEST_TMPDIR"
+        PATH="$fake_bin:$PATH" TEST_RSYNC_LOG="$rsync_log" \
+            TEST_RSYNC_FAIL_DEST="$fail_dest" \
+            test::capture_failure_output "$TEST_SHURIKEN" --sync
+    )
+
+    # Both destinations were attempted despite the first one failing.
+    rsync_output=$(<"$rsync_log")
+    test::assert_contains "arg3=$fail_dest" "$rsync_output"
+    test::assert_contains "arg3=$ok_dest" "$rsync_output"
+    # The summary reports the successful and failed mirrors.
+    test::assert_contains "Sync succeeded for: $ok_dest" "$sync_output"
+    test::assert_contains "Sync failed for: $fail_dest" "$sync_output"
+    test::teardown
+}
+
+test_sync_timeout_rejects_non_positive_integer() {
+    # SYNC_TIMEOUT is validated as a positive integer like TAR_TIMEOUT.
+    local config_file
+    local dist_dir
+    local fake_bin
+    local output
+
+    test::setup
+    fake_bin="$TEST_TMPDIR/bin"
+    config_file="$TEST_TMPDIR/shuriken.conf"
+    dist_dir="$TEST_TMPDIR/dist"
+
+    test::install_rsync_spy "$fake_bin"
+    mkdir -p "$dist_dir"
+    printf 'generated\n' > "$dist_dir/index.html"
+    {
+        printf 'DIST_DIR=%q\n' "$dist_dir"
+        printf 'SYNC_TIMEOUT=%q\n' '0'
+        printf 'SYNC_DESTINATIONS=(%q)\n' 'admin@one.example:/var/www/one/'
+    } > "$config_file"
+
+    output=$(
+        cd "$TEST_TMPDIR"
+        PATH="$fake_bin:$PATH" TEST_RSYNC_LOG="$TEST_TMPDIR/rsync.log" \
+            test::capture_failure_output "$TEST_SHURIKEN" --sync
+    )
+
+    test::assert_contains 'SYNC_TIMEOUT' "$output"
+    test::assert_path_absent "$TEST_TMPDIR/rsync.log"
+    test::teardown
+}
+
 test_positional_commands_fail_without_deprecation() {
     local output
     local old_command
@@ -7158,6 +7242,12 @@ main() {
         '--sync array destination preserves spaces' \
         test_sync_array_destination_preserves_spaces
     test::run_case \
+        '--sync isolates a failing destination and reports failure' \
+        test_sync_isolates_failing_destination_and_reports_failure
+    test::run_case \
+        '--sync rejects non-positive SYNC_TIMEOUT' \
+        test_sync_timeout_rejects_non_positive_integer
+    test::run_case \
         'positional commands fail without deprecation output' \
         test_positional_commands_fail_without_deprecation
     test::run_case \
diff --git a/tests/helpers.sh b/tests/helpers.sh
index e377737..325a13c 100755
--- a/tests/helpers.sh
+++ b/tests/helpers.sh
@@ -674,6 +674,38 @@ RSYNC
     chmod 0755 "$bin_dir/rsync"
 }
 
+# An rsync spy that logs every invocation (like install_rsync_spy) but exits
+# non-zero when the destination (last argument) equals TEST_RSYNC_FAIL_DEST.
+# Used to prove sync_dist isolates a failing destination: it must still attempt
+# the remaining mirrors and report overall failure.
+test::install_rsync_spy_failing_one() {
+    local -r bin_dir="$1"; shift
+
+    mkdir -p "$bin_dir"
+
+    cat > "$bin_dir/rsync" <<'RSYNC'
+#!/usr/bin/env bash
+set -euo pipefail
+
+{
+    printf 'argc=%s\n' "$#"
+    i=0
+    for arg in "$@"; do
+        printf 'arg%s=%q\n' "$i" "$arg"
+        i=$(( i + 1 ))
+    done
+} >> "$TEST_RSYNC_LOG"
+
+# The destination is the final positional argument.
+dest="${@: -1}"
+if [[ "$dest" == "${TEST_RSYNC_FAIL_DEST:-}" ]]; then
+    printf 'rsync: simulated failure for %s\n' "$dest" >&2
+    exit 23
+fi
+RSYNC
+    chmod 0755 "$bin_dir/rsync"
+}
+
 test::install_hanging_tar() {
     local -r bin_dir="$1"; shift