Three hand-rolled copies of the unlink-then-rename fallback, with two return conventions - #754
Merged
Merged
Conversation
replace_file() (htsback.c), wacz_rename_over() (htswarc.c) and the inline block in singlefile_rewrite_file() each worked around Windows' non-clobbering rename, with two opposite return conventions and only one of the three converting path separators. Copying the wrong one gets you an inverted success test. hts_rename_over() replaces all four call sites: hts_boolean return, fconv() on both paths. It lands in htsname.c rather than the htstools.c the issue named, to stay clear of an in-flight PR over that file. The wacz test now runs a cacheless second pass over a poisoned copy of the package the first pass wrote, so repackaging has to clobber an existing file. 43 and 74 also assert the failure warnings stay out of the log, which is all an inverted return leaves behind at those two sites. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <xroche@gmail.com>
This was referenced Jul 27, 2026
Issue #726 asked for htstools.c; it went to htsname.c only because #718 held the file at the time. Kept internal, not HTSEXT_API: htscore.h pulls in both headers, so every call site reaches it either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
This was referenced Jul 27, 2026
This was referenced Jul 27, 2026
xroche
added a commit
that referenced
this pull request
Jul 27, 2026
`back_finalize()` moves an existing file aside to `<file>.bak` before truncating it, so an aborted re-fetch can put the previous copy back. That rename was a bare `RENAME`, and Windows refuses to rename onto an existing target, so a `.bak` outliving a killed run made every later re-fetch of that URL fail the rename, take the `tmpfile = NULL` branch 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 `.bak` is not free. A run killed between the rename-aside and the finalize leaves the previous complete copy in `.bak` and a partial in the live file, and the next re-fetch now overwrites the good one. It still looks like the right trade: POSIX `rename()` 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 `.bak` is 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 static `hts_rename()` wrapper and the hand-rolled `old.zip` unlink 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 `-M` abort 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 existing `old.zip`, which nothing covered. Two older bugs on the same path came out of the review and are filed separately: #774 (the `.bak` name collides with a mirrored file of the same name, reproduced) and #775 (a failed `filecreate()` on a chunked re-fetch commits the backup away). Closes #758 Signed-off-by: Xavier Roche <roche@httrack.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
xroche
added a commit
that referenced
this pull request
Jul 27, 2026
…784) The unlink-then-rename fallback in `hts_rename_over()` is there because Windows' rename() refuses an existing target, but it fired on any failed rename, ENOENT on the source included. A caller moving a temp file it never managed to write lost the destination and got `HTS_FALSE` back, which reads as "nothing happened". #754 unified four hand-rolled copies into this helper, so every call site inherited it. The unlink now runs only for EEXIST, and only with a source that exists. EEXIST is the value that matters: the CRT maps ERROR_ALREADY_EXISTS there and keeps EACCES for a source another process holds, so accepting EACCES as well would have deleted the destination for a failure it had no part in, and the retry would then fail with the destination already gone. The source check is belt and braces for a CRT that reports neither. `hts_rename_utf8()` preserves errno across its free() calls now, since the gate reads it. The existing call sites all derive their source's existence from a create that had to succeed first, so the bug was latent rather than live, but three of those destinations are user data: a mirrored file, a finished .wacz, and a rewritten page nothing will re-fetch. What is left of the window after this is #790. The selftest probes what rename() does to an existing target and asserts against the regime it finds, so it runs three ways on Linux: native, then under an LD_PRELOAD rename() with Windows' shape, then under one reporting a locked source. The harness pins the expected regime per platform, so no leg can pass having exercised the other half. Seven mutants of the gate were each confirmed to fail it. Closes #779 Signed-off-by: Xavier Roche <roche@httrack.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.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
1 participant
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.
Three copies of the Windows unlink-then-rename dance had drifted into two return conventions, and only one of the three ran its paths through
fconv(). Copying the wrong one next time gets you an inverted success test.hts_rename_over()replaces all four call sites (htsback.chas two):hts_booleanreturn,fconv()on both paths. Converting everywhere is the conservative call. On POSIXfconvis a plain copy, and every path involved is bounded near 2 KB against an 8 KB buffer, so it cannot truncate. On Windows it is mostly defensive, since_wrenameaccepts forward slashes and the long-path branch normalizes them; the one input where it changes the outcome is a verbatim\\?\path, which skips normalization and today fails on a/.The helper lands in
htsname.crather than thehtstools.cthe issue suggested, because #718 is editing that file and this is not worth a conflict.htsname.calready hostsurl_savename_refname_remove(), the same shape of helper, and every caller seeshtsname.hthroughhtscore.h. Moving it later is one hunk.Equivalence is measured rather than argued. With both trees instrumented to log every (src, dst, result), the suite performs 31 rename-overs (18 from the backing engine, 11 single-file, 2 wacz) and the tuples come out identical before and after. The unlink fallback is dead code on POSIX, where
rename(2)clobbers, so I exercised it under anLD_PRELOADrename()that refuses an existing target the way Windows does: 21 refusals, every operation still succeeded, and the same probe with the fallback deleted fails every one of the five tests it covers.The wacz test only ever did the happy rename. It now runs a cacheless second pass over a poisoned copy of the first pass's package, so repackaging has to clobber a file that is already there. 43 and 74 also assert the failure warnings stay out of the log, which is all an inverted return leaves behind at those two sites; the other two fail loudly on their own. Two bare
RENAME()calls inhtsback.cstay as they are: the.delayedplaceholder move is deliberately non-clobbering, and the #77 backup creation would change behaviour if it started clobbering a stale.bak.Closes #726