From 8228c81b5cce3fd83c3863bc97f74e5f96a24f52 Mon Sep 17 00:00:00 2001 From: Emma Thorpe Date: Tue, 25 Aug 2026 12:51:42 +0100 Subject: [PATCH] fix: flush and unmount even when the sync is interrupted rsync itself is safe under interruption. It writes to a hidden temporary file and renames it into place only once complete, and by default deletes any partial file when interrupted -- verified both in the manual and by killing a transfer and inspecting what was left, which was nothing. --partial is deliberately absent and there is now a test asserting it stays that way. An unclean kill can leave a hidden .track.mp3.XXXXXX behind; it is unplayable, it is not in the source, and the next run's --delete removes it. The script was not safe. Ctrl-C killed it before the sync and the unmount, leaving a journal-less FAT filesystem holding dirty buffers -- which is the exact corruption the script exists to prevent, arrived at by the most likely route a person would take. INT and TERM are now trapped. Both the normal path and the interrupt path call the same finish function, so the flush and the unmount cannot drift apart, and an interrupted run exits 130 rather than pretending to have succeeded. The test is structural rather than timed. Reproducing a mid-transfer signal needs a payload large enough to be slow, and a test that depends on winning a race is a test that fails in CI for reasons that have nothing to do with the code. The behaviour was verified by hand: SIGTERM mid-transfer gave exit 130, the flush ran, and the destination held no short files and no leftover temporaries. --- README.md | 21 +++++++++++++++++ tests/test_sync_to_ipod.py | 24 ++++++++++++++++++++ tools/sync-to-ipod.sh | 46 +++++++++++++++++++++++++++----------- 3 files changed, 78 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index e2275e6..cab2446 100644 --- a/README.md +++ b/README.md @@ -185,6 +185,27 @@ is what a percentage and an estimate that mean something cost — and renders th rest itself. Piped to a log it prints a plain line every thirty seconds instead, with no carriage returns, and a summary at the end either way. +### If the sync is interrupted + +No partially copied track is ever left under a name Rockbox would play. rsync +writes to a hidden temporary file and only renames it into place once the file +is complete, and *"by default, rsync will delete any partially transferred file +if the transfer is interrupted"*. `--partial` is deliberately not used, and +there is a test asserting it never will be. + +After an unclean kill or a power cut a hidden `.track.mp3.XXXXXX` can survive. +It is not playable, it is not in the source, and the next run's `--delete` +removes it. + +The real risk is not rsync's, it is the filesystem's: FAT32 has no journal, so +buffers that never reach the card are corruption. The script therefore traps +`INT` and `TERM` and still flushes and unmounts on the way out, exiting 130. +Both the normal path and the interrupt path call the same function, so they +cannot drift apart. + +The one case nothing can help with is pulling the cable or the card mid-write. +Wait for the unmount. + ### Making it faster over a network mount The transfer is metadata-bound, not throughput-bound: 49,600 files means 49,600 diff --git a/tests/test_sync_to_ipod.py b/tests/test_sync_to_ipod.py index 386599a..1813068 100644 --- a/tests/test_sync_to_ipod.py +++ b/tests/test_sync_to_ipod.py @@ -204,3 +204,27 @@ def test_directory_timestamps_are_not_set(mirror, tmp_path): script = SCRIPT.read_text() assert "--omit-dir-times" in script + + +def test_an_interrupt_is_trapped_so_the_filesystem_is_flushed(): + """Ctrl-C during a transfer would otherwise skip the sync and the unmount, + leaving a journal-less FAT filesystem with dirty buffers -- which is the + corruption this script exists to prevent. + + Structural rather than timed: reproducing a mid-transfer signal needs a + payload large enough to be slow, and a test that depends on losing a race + is a test that fails in CI for no reason. + """ + script = SCRIPT.read_text() + + assert "trap interrupted INT TERM" in script + assert "exit 130" in script + + +def test_the_flush_and_unmount_happen_on_every_exit_path(): + script = SCRIPT.read_text() + + # Both the normal path and the interrupt path go through the same function, + # so one cannot drift from the other. + assert script.count("finish\n") >= 2 + assert "--partial" not in script, "rsync must delete partial files, not keep them" diff --git a/tools/sync-to-ipod.sh b/tools/sync-to-ipod.sh index 71f7c8f..ed119d4 100755 --- a/tools/sync-to-ipod.sh +++ b/tools/sync-to-ipod.sh @@ -204,21 +204,41 @@ else printf 'sync-to-ipod: %s files to copy\n' "$total" >&2 fi +# Flushing and unmounting is the whole reason this is a script, so it has to +# happen on the way out whichever way that is. Ctrl-C during a transfer would +# otherwise leave a FAT filesystem with dirty buffers and no journal, which is +# the corruption this exists to avoid. +finish() { + sync + if $unmount; then + device=$(findmnt -no SOURCE --target "$destination" 2>/dev/null || true) + if [ -n "$device" ]; then + printf 'sync-to-ipod: unmounting %s\n' "$device" >&2 + if command -v udisksctl >/dev/null 2>&1; then + udisksctl unmount -b "$device" || umount -- "$destination" || true + else + umount -- "$destination" || true + fi + printf 'sync-to-ipod: safe to disconnect\n' >&2 + fi + else + printf 'sync-to-ipod: still mounted; unmount before disconnecting\n' >&2 + fi +} + +interrupted() { + trap - INT TERM + printf '\nsync-to-ipod: interrupted -- rsync leaves no partial files, but the\n' >&2 + printf 'sync-to-ipod: filesystem still needs flushing before you pull anything\n' >&2 + finish + exit 130 +} + +trap interrupted INT TERM + rsync "${options[@]}" --out-format='%l %n' "$mirror/" "$destination/" | python3 "$here/rsync_progress.py" --total "$total" --bytes "$total_bytes" status=${PIPESTATUS[0]} [ "$status" -eq 0 ] || die "rsync exited $status" -sync -if $unmount; then - device=$(findmnt -no SOURCE --target "$destination") - printf 'sync-to-ipod: unmounting %s\n' "$device" >&2 - if command -v udisksctl >/dev/null 2>&1; then - udisksctl unmount -b "$device" - else - umount -- "$destination" - fi - printf 'sync-to-ipod: safe to disconnect\n' >&2 -else - printf 'sync-to-ipod: still mounted; unmount before disconnecting\n' >&2 -fi +finish