A stale .bak silently disables the re-fetch backup on Windows - #776
Merged
Conversation
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) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
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) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
96 was taken by #763 on master. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
master took 97 and 98 for the WARC tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
xroche
added a commit
that referenced
this pull request
Jul 27, 2026
master took 97 and 98 for the WARC tests, and 99 is claimed by #776. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
xroche
added a commit
that referenced
this pull request
Jul 27, 2026
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
xroche
added a commit
that referenced
this pull request
Jul 27, 2026
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
back_finalize()moves an existing file aside to<file>.bakbefore truncating it, so an aborted re-fetch can put the previous copy back. That rename was a bareRENAME, and Windows refuses to rename onto an existing target, so a.bakoutliving a killed run made every later re-fetch of that URL fail the rename, take thetmpfile = NULLbranch with nothing logged, and truncate the live file with no backup at all. The #77 guard was off for that file, silently and for good.hts_rename_over()(#754) unlinks and retries, which fixes it.Worth a look, because clobbering a stale
.bakis not free. A run killed between the rename-aside and the finalize leaves the previous complete copy in.bakand a partial in the live file, and the next re-fetch now overwrites the good one. It still looks like the right trade: POSIXrename()has behaved exactly this way since #77 landed, so the alternative is leaving Windows with a guard that stays dead until someone deletes the file by hand, and a leftover.bakis engine garbage that nothing advertises or ever restores. The clobber is logged now, so it is at least visible.Any failure to create the backup is logged too, instead of quietly disabling the safety net.
htscache.c's statichts_rename()wrapper and the hand-rolledold.zipunlink at its only call site go the same way, which removes a copy of the unlink-then-rename idiom rather than adding a fifth.Test 101 plants both kinds of leftover before the update pass: a stale file, which has to be clobbered so the
-Mabort can still restore the pass-1 copy, and a directory, which cannot be clobbered and has to be reported instead. Only the Windows leg arms the first half, since POSIX clobbers on its own; the second is armed everywhere. Test 37 gains a live update pass so the dead pass rotates onto an existingold.zip, which nothing covered.Two older bugs on the same path came out of the review and are filed separately: #774 (the
.bakname collides with a mirrored file of the same name, reproduced) and #775 (a failedfilecreate()on a chunked re-fetch commits the backup away).Closes #758