hts_rename_over() deletes its destination when the source is missing - #784
Merged
Conversation
The unlink-then-rename fallback exists for Windows, whose rename() refuses an existing target. It fired on any failed rename, so a caller asking to move a temp file it never managed to write lost the destination and got HTS_FALSE back, which reads as "nothing happened". Gate the unlink on the failure it was added for: EACCES/EEXIST, and a source that exists. The CRT does not promise ENOENT over EACCES when both hold, so the source check is not redundant. hts_rename_utf8() now preserves errno across its free() calls, which the gate depends on. Closes #779 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
The contract said dst is never removed unless it is replaced. The retry after the unlink can still fail, so say what actually holds and point at #790. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
Review of the first commit found the gate too wide: the CRT maps ERROR_ALREADY_EXISTS to EEXIST and reserves EACCES for a source another process holds, so accepting EACCES would remove dst for a failure dst had no part in, and the retry would then fail with dst already gone. The selftest now probes what rename() does to an existing target and asserts against the regime it finds, so the interposed legs cover the fallback and a failure the unlink cannot fix. Follow-up in #791. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
…/rename-over-enoent # Conflicts: # src/htstools.c # src/htstools.h
The all-or-nothing precheck was documented as a workaround for hts_rename_over deleting its destination, which it no longer does. The loop still earns its place: a swap stopping halfway mixes two runs' segments. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
Two ways the LD_PRELOAD legs broke outside a plain Linux build: ASan aborts when a preloaded library loads ahead of its runtime, and --disable-shared (what the MSan job uses) builds no module to preload at all. The first is safe to waive here, the shim allocates nothing. The second is a real skip rather than a failure, told apart from a missing module by libtool's own dlname, so a build that should have one still fails loudly. 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
Every other network crawl runs under a deadline through local-crawl.sh. This test drives its own FTP fixture directly, so a wedge would hang the job until CI cancels it and discards the log (#796). 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
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.
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 gotHTS_FALSEback, 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