From e5ca2d72bd7c9aa9a345a2b257e90bcb4589e029 Mon Sep 17 00:00:00 2001 From: Xavier Roche Date: Mon, 27 Jul 2026 09:44:06 +0200 Subject: [PATCH 1/3] A stale .bak silently disables the #77 re-fetch backup back_finalize() moved an existing file aside with a bare RENAME before truncating it. Windows refuses to rename onto an existing target, so a .bak outliving a killed run made every later re-fetch of that URL take the tmpfile = NULL branch: no backup, and nothing to restore when the transfer aborted. Use hts_rename_over(), which clobbers, and log when the backup cannot be made at all. htscache.c's hts_rename() wrapper and its caller's hand-rolled old.zip unlink go the same way. Closes #758 Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Xavier Roche --- src/htsback.c | 12 +++++-- src/htscache.c | 34 ++++--------------- tests/96_local-update-stale-bak.test | 25 ++++++++++++++ tests/Makefile.am | 3 +- tests/local-crawl.sh | 49 +++++++++++++++++++++++----- 5 files changed, 85 insertions(+), 38 deletions(-) create mode 100644 tests/96_local-update-stale-bak.test diff --git a/src/htsback.c b/src/htsback.c index 646082a8..808500cc 100644 --- a/src/htsback.c +++ b/src/htsback.c @@ -3150,10 +3150,18 @@ void back_wait(struct_back * sback, httrackp * opt, cache_back * cache, */ back[i].tmpfile = NULL; if (fexist_utf8(back[i].url_sav)) { + /* clobber a .bak a killed run left behind: + refusing it disables the guard for good + (#758) */ if (create_back_tmpfile(opt, &back[i], "bak") != 0 || - RENAME(back[i].url_sav, back[i].tmpfile) != - 0) { + !hts_rename_over(back[i].url_sav, + back[i].tmpfile)) { + hts_log_print( + opt, LOG_WARNING | LOG_ERRNO, + "could not back up %s; an aborted " + "re-fetch will lose it", + back[i].url_sav); back[i].tmpfile = NULL; } } diff --git a/src/htscache.c b/src/htscache.c index e0611b44..a342d0cb 100644 --- a/src/htscache.c +++ b/src/htscache.c @@ -855,13 +855,6 @@ static htsblk cache_readex_new(httrackp * opt, cache_back * cache, return r; } -// lecture d'un fichier dans le cache -// si save==null alors test unqiquement -static int hts_rename(httrackp * opt, const char *a, const char *b) { - hts_log_print(opt, LOG_DEBUG, "Cache: rename %s -> %s (%p %p)", a, b, a, b); - return RENAME(a, b); -} - /* Open the cache ZIP via hts_fopen_utf8 so a non-ASCII path_log isn't mangled to ANSI (#630); 64-bit funcs keep multi-GB caches whole on Windows LLP64. */ static voidpf ZCALLBACK hts_zip_fopen_utf8(voidpf opaque, const void *filename, @@ -991,29 +984,16 @@ void cache_init(cache_back * cache, httrackp * opt) { OPT_GET_BUFF(opt), OPT_GET_BUFF_SIZE(opt), StringBuff(opt->path_log), "hts-cache/new.zip")))) { // a previous cache exists.. rename it - /* Remove OLD cache */ - if (fexist_utf8(fconcat(OPT_GET_BUFF(opt), OPT_GET_BUFF_SIZE(opt), - StringBuff(opt->path_log), - "hts-cache/old.zip"))) { - if (UNLINK(fconcat(OPT_GET_BUFF(opt), OPT_GET_BUFF_SIZE(opt), - StringBuff(opt->path_log), "hts-cache/old.zip")) != - 0) { - hts_log_print(opt, LOG_WARNING | LOG_ERRNO, - "Cache: error while moving previous cache"); - } - } - - /* Rename */ - if (hts_rename - (opt, - fconcat(OPT_GET_BUFF(opt), OPT_GET_BUFF_SIZE(opt), StringBuff(opt->path_log), - "hts-cache/new.zip"), fconcat(OPT_GET_BUFF(opt), OPT_GET_BUFF_SIZE(opt), - StringBuff(opt->path_log), - "hts-cache/old.zip")) != 0) { + /* hts_rename_over() drops any leftover old.zip itself. */ + if (!hts_rename_over( + fconcat(OPT_GET_BUFF(opt), OPT_GET_BUFF_SIZE(opt), + StringBuff(opt->path_log), "hts-cache/new.zip"), + fconcat(OPT_GET_BUFF(opt), OPT_GET_BUFF_SIZE(opt), + StringBuff(opt->path_log), "hts-cache/old.zip"))) { hts_log_print(opt, LOG_WARNING | LOG_ERRNO, "Cache: error while moving previous cache"); } else { - hts_log_print(opt, LOG_DEBUG, "Cache: successfully renamed"); + hts_log_print(opt, LOG_DEBUG, "Cache: rotated new.zip to old.zip"); } } } else { diff --git a/tests/96_local-update-stale-bak.test b/tests/96_local-update-stale-bak.test new file mode 100644 index 00000000..8c5cdba1 --- /dev/null +++ b/tests/96_local-update-stale-bak.test @@ -0,0 +1,25 @@ +#!/bin/bash +# +# A .bak left behind by a killed run must not disable the #77 re-fetch backup +# (#758). Both bigtrunc files are re-fetched under -M in pass 2: +# - slow.bin.bak is planted as a stale regular file; the backup has to clobber +# it, so the -M abort still restores the complete pass-1 copy. Only armed on +# Windows, where rename() refuses an existing target. +# - fast.bin.bak is planted as a directory, so no backup can be made at all; +# the engine must say so instead of truncating with no safety net. + +set -euo pipefail + +: "${top_srcdir:=..}" + +bash "$top_srcdir/tests/local-crawl.sh" \ + --plant-file bigtrunc/slow.bin.bak \ + --plant-dir bigtrunc/fast.bin.bak \ + --rerun-args '--update -M400000' \ + --log-found 'could not back up .*fast\.bin' \ + --log-not-found 'could not back up .*slow\.bin' \ + --log-not-found 'could not restore' \ + --found bigtrunc/slow.bin \ + --file-min-bytes bigtrunc/slow.bin 655360 \ + --file-not-matches bigtrunc/slow.bin 'stale-leftover' \ + httrack 'BASEURL/bigtrunc/index.html' -c2 diff --git a/tests/Makefile.am b/tests/Makefile.am index fad53a24..4cee4032 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -195,6 +195,7 @@ TESTS = \ 92_local-proxytrack-ndx-fields.test \ 93_local-changes.test \ 94_local-single-file.test \ - 95_local-sitemap.test + 95_local-sitemap.test \ + 96_local-update-stale-bak.test CLEANFILES = check-network_sh.cache diff --git a/tests/local-crawl.sh b/tests/local-crawl.sh index 5ab3f41c..f23f0e02 100755 --- a/tests/local-crawl.sh +++ b/tests/local-crawl.sh @@ -34,6 +34,9 @@ # httrack via --cookies-file, to exercise preloaded cookies. # --rerun-dead re-runs with the server stopped: the no-data rollback must # restore the previous hts-cache generation byte-identical. +# --plant-file/--plant-dir drop a regular file (holding $plant_poison) or a +# directory at PATH under the host root between the passes, to hand the second +# pass leftovers a killed run would have left (#758). set -u @@ -59,6 +62,7 @@ serverpid= crawlpid= wacz_poisoned= wacz_poison="stale-wacz-that-a-second-pass-must-replace" +plant_poison="stale-leftover-that-a-second-pass-must-clobber" function warning { echo "** $*" >&2 @@ -87,6 +91,19 @@ function cleanup { fi } +# The random port leaks into the mirror directory name, so resolve it at runtime. +hostroot= +function find_hostroot { + local cand + for cand in "${mirrorroot}/127.0.0.1_${port}" "${mirrorroot}/127.0.0.1"; do + if test -d "$cand"; then + hostroot="$cand" + return 0 + fi + done + die "could not find host root under $out" +} + function assert_equals { info "$1" if test ! "$2" == "$3"; then @@ -109,6 +126,7 @@ tmpdir=$(mktemp -d "${tmptopdir}/httrack_local.XXXXXX") || die "could not create # --- parse leading control flags -------------------------------------------- declare -a audit=() declare -a cookies=() +declare -a plants=() scheme=http pos=0 args=("$@") @@ -141,6 +159,10 @@ while test "$pos" -lt "$nargs"; do pos=$((pos + 1)) cookies+=("${args[$pos]}") ;; + --plant-file | --plant-dir) + plants+=("${args[$pos]}" "${args[$((pos + 1))]}") + pos=$((pos + 1)) + ;; --rerun-args) pos=$((pos + 1)) rerun_args="${args[$pos]}" @@ -282,6 +304,24 @@ if test -n "$wacz_validate" && test -n "${rerun}${rerun_args}"; then test -z "$wacz_poisoned" || echo "$wacz_poison" >"$wacz_poisoned" fi +# --- plant leftovers the second pass has to deal with ------------------------ +if test "${#plants[@]}" -gt 0; then + find_hostroot + i=0 + while test "$i" -lt "${#plants[@]}"; do + path="${hostroot}/${plants[$((i + 1))]}" + info "planting ${plants[$i]} ${plants[$((i + 1))]}" + rm -rf "$path" + if test "${plants[$i]}" = "--plant-dir"; then + mkdir -p "$path" || die "could not create $path" + else + echo "$plant_poison" >"$path" || die "could not write $path" + fi + result "OK" + i=$((i + 2)) + done +fi + # --- optional second pass: re-mirror into the same dir (cache/update path) ---- if test -n "$rerun"; then info "re-running httrack (update pass)" @@ -363,14 +403,7 @@ if test -n "$rerun_dead"; then fi # --- discover the single host root (127.0.0.1_ or 127.0.0.1) ----------- -hostroot= -for cand in "${mirrorroot}/127.0.0.1_${port}" "${mirrorroot}/127.0.0.1"; do - if test -d "$cand"; then - hostroot="$cand" - break - fi -done -test -n "$hostroot" || die "could not find host root under $out" +find_hostroot debug "host root: $hostroot" # --- optional WARC validation (stdlib validator, no warcio) ------------------ From 314f1ee5b6837a26a63b34440d5a3aaffa80e8eb Mon Sep 17 00:00:00 2001 From: Xavier Roche Date: Mon, 27 Jul 2026 09:58:18 +0200 Subject: [PATCH 2/3] Review follow-ups: log the clobber, arm the test harder Report when a leftover .bak is about to be replaced, so destroying the orphan of a killed run is not silent. Test 96 now asserts the -M abort actually fired and that fast.bin stays a file; test 37 gains a live update pass so the dead pass rotates onto an existing old.zip, which nothing covered. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Xavier Roche --- src/htsback.c | 22 +++++++++++++++------- src/htscache.c | 1 - tests/37_local-cache-outage.test | 6 ++++-- tests/96_local-update-stale-bak.test | 14 ++++++++------ tests/local-crawl.sh | 2 -- 5 files changed, 27 insertions(+), 18 deletions(-) diff --git a/src/htsback.c b/src/htsback.c index 808500cc..03972d6b 100644 --- a/src/htsback.c +++ b/src/htsback.c @@ -3150,13 +3150,21 @@ void back_wait(struct_back * sback, httrackp * opt, cache_back * cache, */ back[i].tmpfile = NULL; if (fexist_utf8(back[i].url_sav)) { - /* clobber a .bak a killed run left behind: - refusing it disables the guard for good - (#758) */ - if (create_back_tmpfile(opt, &back[i], "bak") != - 0 || - !hts_rename_over(back[i].url_sav, - back[i].tmpfile)) { + hts_boolean saved = HTS_FALSE; + + if (create_back_tmpfile(opt, &back[i], "bak") == + 0) { + /* clobber a .bak a killed run left behind, + or the guard stays off for good (#758) */ + if (fexist_utf8(back[i].tmpfile)) + hts_log_print( + opt, LOG_WARNING, + "replacing leftover backup %s", + back[i].tmpfile); + saved = hts_rename_over(back[i].url_sav, + back[i].tmpfile); + } + if (!saved) { hts_log_print( opt, LOG_WARNING | LOG_ERRNO, "could not back up %s; an aborted " diff --git a/src/htscache.c b/src/htscache.c index a342d0cb..9d1b86e0 100644 --- a/src/htscache.c +++ b/src/htscache.c @@ -984,7 +984,6 @@ void cache_init(cache_back * cache, httrackp * opt) { OPT_GET_BUFF(opt), OPT_GET_BUFF_SIZE(opt), StringBuff(opt->path_log), "hts-cache/new.zip")))) { // a previous cache exists.. rename it - /* hts_rename_over() drops any leftover old.zip itself. */ if (!hts_rename_over( fconcat(OPT_GET_BUFF(opt), OPT_GET_BUFF_SIZE(opt), StringBuff(opt->path_log), "hts-cache/new.zip"), diff --git a/tests/37_local-cache-outage.test b/tests/37_local-cache-outage.test index 76de0208..3497b3f2 100644 --- a/tests/37_local-cache-outage.test +++ b/tests/37_local-cache-outage.test @@ -1,12 +1,14 @@ #!/bin/bash # # An update run against a dead server must not destroy the cache: the no-data -# rollback restores the previous hts-cache generation (zip caches lost it). +# rollback restores the previous hts-cache generation (zip caches lost it). The +# live update pass leaves an old.zip behind, so the dead pass rotates onto an +# existing generation instead of into empty space. set -eu : "${top_srcdir:=..}" -bash "$top_srcdir/tests/local-crawl.sh" --errors 0 --rerun-dead \ +bash "$top_srcdir/tests/local-crawl.sh" --errors 0 --rerun --rerun-dead \ --found 'simple/basic.html' \ httrack 'BASEURL/simple/basic.html' diff --git a/tests/96_local-update-stale-bak.test b/tests/96_local-update-stale-bak.test index 8c5cdba1..78d01c99 100644 --- a/tests/96_local-update-stale-bak.test +++ b/tests/96_local-update-stale-bak.test @@ -1,12 +1,11 @@ #!/bin/bash # # A .bak left behind by a killed run must not disable the #77 re-fetch backup -# (#758). Both bigtrunc files are re-fetched under -M in pass 2: -# - slow.bin.bak is planted as a stale regular file; the backup has to clobber -# it, so the -M abort still restores the complete pass-1 copy. Only armed on -# Windows, where rename() refuses an existing target. -# - fast.bin.bak is planted as a directory, so no backup can be made at all; -# the engine must say so instead of truncating with no safety net. +# (#758): a leftover file (slow.bin.bak) has to be clobbered so the -M abort can +# still restore the pass-1 copy, and a leftover the engine cannot clobber +# (fast.bin.bak, planted as a directory) has to be reported instead of silently +# truncating with no safety net. Only the Windows leg arms the first half: +# POSIX rename() clobbers on its own. set -euo pipefail @@ -16,10 +15,13 @@ bash "$top_srcdir/tests/local-crawl.sh" \ --plant-file bigtrunc/slow.bin.bak \ --plant-dir bigtrunc/fast.bin.bak \ --rerun-args '--update -M400000' \ + --log-found 'More than 400000 bytes have been transferred.. giving up' \ + --log-found 'replacing leftover backup .*slow\.bin\.bak' \ --log-found 'could not back up .*fast\.bin' \ --log-not-found 'could not back up .*slow\.bin' \ --log-not-found 'could not restore' \ --found bigtrunc/slow.bin \ + --found bigtrunc/fast.bin \ --file-min-bytes bigtrunc/slow.bin 655360 \ --file-not-matches bigtrunc/slow.bin 'stale-leftover' \ httrack 'BASEURL/bigtrunc/index.html' -c2 diff --git a/tests/local-crawl.sh b/tests/local-crawl.sh index f23f0e02..5886f795 100755 --- a/tests/local-crawl.sh +++ b/tests/local-crawl.sh @@ -91,7 +91,6 @@ function cleanup { fi } -# The random port leaks into the mirror directory name, so resolve it at runtime. hostroot= function find_hostroot { local cand @@ -311,7 +310,6 @@ if test "${#plants[@]}" -gt 0; then while test "$i" -lt "${#plants[@]}"; do path="${hostroot}/${plants[$((i + 1))]}" info "planting ${plants[$i]} ${plants[$((i + 1))]}" - rm -rf "$path" if test "${plants[$i]}" = "--plant-dir"; then mkdir -p "$path" || die "could not create $path" else From 10e147436c5a2523d9afb3592465fcb6e11d1717 Mon Sep 17 00:00:00 2001 From: Xavier Roche Date: Mon, 27 Jul 2026 10:30:42 +0200 Subject: [PATCH 3/3] tests: renumber the stale-.bak test to 101 99 and 100 went to #783 and #780. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Xavier Roche --- ...al-update-stale-bak.test => 101_local-update-stale-bak.test} | 0 tests/Makefile.am | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) rename tests/{99_local-update-stale-bak.test => 101_local-update-stale-bak.test} (100%) diff --git a/tests/99_local-update-stale-bak.test b/tests/101_local-update-stale-bak.test similarity index 100% rename from tests/99_local-update-stale-bak.test rename to tests/101_local-update-stale-bak.test diff --git a/tests/Makefile.am b/tests/Makefile.am index ec29df77..57af82f8 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -201,6 +201,6 @@ TESTS = \ 96_local-refetch-keep.test \ 97_local-warc-update-keep.test \ 98_local-warc-segments.test \ - 99_local-update-stale-bak.test + 101_local-update-stale-bak.test CLEANFILES = check-network_sh.cache