hts_rename_over() can lose its destination when the retried rename fails - #816
Merged
Conversation
The unlink-then-rename fallback leaves nothing in place of dst between the unlink and the retry, so a retry that fails too loses it. Park dst under a free scratch name instead, drop it once the move succeeded, and put it back otherwise. Closes #790 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
…rantee The move back out of the parked name was unchecked, so a retry that failed for a reason that still applied left dst absent with the content orphaned under a name nothing reported. Check it, retry once, and name the parked copy in the log; hts_rename_over() takes an httrackp for that. The aside probe used fexist(), which is not UTF-8 and consults the ANSI codepage on Windows while the renames beside it are wide. It also reads a directory as a free name, so the park now skips a name whose rename refuses rather than giving up on it. The header claimed a failure leaves dst as it was, which the crash window between the two renames does not give. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
#822 converted 01_engine-renameover.test's assertions to the here-string form, since grep -q SIGPIPEs the echo feeding it and an assertion that held reports 141. This branch had added its restore-outcome and retry legs in the old pipe form. Resolved on master's form, with those legs ported into it rather than kept alongside. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
Windows refuses every rename onto an existing target, so the fallback is production code there rather than the rare path it is on POSIX. A directory at the destination was renamed aside like a file, the move then succeeded, and UNLINK could not drop the parked directory, so the call reported success where master had reported failure and left an orphan behind. 101_local-update-stale-bak plants exactly that shape and caught it on both Windows legs. Park a regular file only. A directory in the way is refused, as before. 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
Lost when the master merge took this file wholesale; #816 gave the function an httrackp parameter and the three call sites here reverted to the old form. 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
* A 304 during --update leaks the whole previous htsblk back_wait() handles a 304 by replacing the response struct with the cache entry, carrying only the socket and keep-alive members across via back_connxfr(). The struct assignment drops every owned pointer the live response still held without freeing any of them: the 8 KB header buffer on every update, plus the two WARC header stashes when --warc-file is on. An update over a 10k-page site drops roughly 80 MB in one run. back_clear_entry() already knew how to tear those down, so the frees move into a helper that both it and the 304 path call. The new test runs the two-pass mini304 crawl with LeakSanitizer on, which the sanitized CI job otherwise disables. The fresh first pass is the control: it has no cache entry to read back and is clean either way. The update pass reports 16 KB in 2 objects on master, one per unchanged URL, and nothing with the fix. Closes #782 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com> * Trim the test header Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com> * Review fixes: reuse deleteaddr(), and cover the WARC limb back_free_response() was reimplementing deleteaddr(), which already frees adr and headers and NULLs both; call it instead so the two cannot drift. Test 114 never passed --warc-file, so the warc_free_request() limb ran with both pointers NULL on every path it exercised and deleting it kept the test green. A third pass turns the archive on, and it now fails with the 835 and 238 byte stashes when that call goes away. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com> * Register the new leak test as an expected Windows skip The Win32/x64 job pins the exact set of tests allowed to skip, so an all-skipped suite cannot report green. 114_local-update-304-leak needs a LeakSanitizer build and MSVC has no equivalent, so it skips there and tripped the gate with fail=0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com> * A 304 revisit archives none of the exchange it stands for The cache entry overwrites the whole htsblk in back_wait(), taking the stashed request and response headers with it, so the revisit record gets a synthesized "HTTP/1.1 200 OK" block and no request record. Both blocks now move across the swap, but only for a real 304: the engine also forces HTTP_NOT_MODIFIED by itself, and those have no 304 to carry. Since a 304 declares no Content-Type, warc_write_transaction() takes the resource type from the caller so revisit CDXJ lines keep their mime. cache_read_including_broken() returns the entry's response struct after back_clear_entry() has freed it; adr, headers and location come back NULL now, as the normal cache_readex() path already returns them. Closes #826 Signed-off-by: Xavier Roche <roche@httrack.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Do not expand an empty bodyargs array under macOS bash 3.2 Test 122 asks local-crawl.sh only for the revisit checks, so it is the first caller to leave bodyargs empty. Bash before 4.4 calls that unbound under set -u, which passes on Linux and fails the macOS leg. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com> * Restore the opt argument to hts_rename_over call sites Lost when the master merge took this file wholesale; #816 gave the function an httrackp parameter and the three call sites here reverted to the old form. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com> --------- 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
…archived as one the server never sent (#849) * A 304 during --update leaks the whole previous htsblk back_wait() handles a 304 by replacing the response struct with the cache entry, carrying only the socket and keep-alive members across via back_connxfr(). The struct assignment drops every owned pointer the live response still held without freeing any of them: the 8 KB header buffer on every update, plus the two WARC header stashes when --warc-file is on. An update over a 10k-page site drops roughly 80 MB in one run. back_clear_entry() already knew how to tear those down, so the frees move into a helper that both it and the 304 path call. The new test runs the two-pass mini304 crawl with LeakSanitizer on, which the sanitized CI job otherwise disables. The fresh first pass is the control: it has no cache entry to read back and is clean either way. The update pass reports 16 KB in 2 objects on master, one per unchanged URL, and nothing with the fix. Closes #782 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com> * Trim the test header Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com> * Review fixes: reuse deleteaddr(), and cover the WARC limb back_free_response() was reimplementing deleteaddr(), which already frees adr and headers and NULLs both; call it instead so the two cannot drift. Test 114 never passed --warc-file, so the warc_free_request() limb ran with both pointers NULL on every path it exercised and deleting it kept the test green. A third pass turns the archive on, and it now fails with the 835 and 238 byte stashes when that call goes away. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com> * Register the new leak test as an expected Windows skip The Win32/x64 job pins the exact set of tests allowed to skip, so an all-skipped suite cannot report green. 114_local-update-304-leak needs a LeakSanitizer build and MSVC has no equivalent, so it skips there and tripped the gate with fail=0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com> * A 304 revisit archives none of the exchange it stands for The cache entry overwrites the whole htsblk in back_wait(), taking the stashed request and response headers with it, so the revisit record gets a synthesized "HTTP/1.1 200 OK" block and no request record. Both blocks now move across the swap, but only for a real 304: the engine also forces HTTP_NOT_MODIFIED by itself, and those have no 304 to carry. Since a 304 declares no Content-Type, warc_write_transaction() takes the resource type from the caller so revisit CDXJ lines keep their mime. cache_read_including_broken() returns the entry's response struct after back_clear_entry() has freed it; adr, headers and location come back NULL now, as the normal cache_readex() path already returns them. Closes #826 Signed-off-by: Xavier Roche <roche@httrack.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Do not expand an empty bodyargs array under macOS bash 3.2 Test 122 asks local-crawl.sh only for the revisit checks, so it is the first caller to leave bodyargs empty. Bash before 4.4 calls that unbound under set -u, which passes on Linux and fails the macOS leg. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com> * Restore the opt argument to hts_rename_over call sites Lost when the master merge took this file wholesale; #816 gave the function an httrackp parameter and the three call sites here reverted to the old form. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com> * A 304 revisit names no peer IP, and an engine-forced not-modified is archived as one the server never sent back_connxfr() moves the socket and keep-alive members across the cache-entry swap in the 304 path of back_wait(), but not the peer address, so every revisit record came out with no WARC-IP-Address (#838). Carry it across the same swap. Separately, several engine hacks (the same-size heuristic, the #176 error-ignore-on-update path, ...) force HTTP_NOT_MODIFIED themselves when the server actually said something else. That statuscode flows into the same 304 branch, and warc_write_backtransaction() turned it into a revisit record whose WARC-Profile claims a server-not-modified 304 that was never exchanged (#839). The fix reuses the existing server_sent_304 signal (already used to gate the request/response header carry-over): its persisted proxy is warc_resphdr != NULL, present only when the swap actually moved real 304 headers across. When it's absent, no server exchange happened this run to archive, and there's no freshly re-fetched body either to dedup against a same-run predecessor, so the fix skips the record entirely rather than writing an identical-payload-digest revisit with nothing behind it to refer to. tests/134 and tests/135 extend the mini304/errmask fixtures into a two-pass --warc-file crawl and assert against tests/warc-validate.py, which gained --expect-ip and --no-record-for for these checks. Both reproduce their bug when run against the pre-fix code. Closes #838 Closes #839 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com> * Fix review findings: an engine-forced not-modified must still count as unbacked Writing no WARC record at all for an engine-forced not-modified (the #839 fix in the previous commit) dropped w->unbacked_revisits accounting along with it, so an all-forced pass wrongly looked fully backed: warc_commit's swap guard let it replace the previous archive with an (almost) empty one, reopening #777's exact shape, and warc_cdx_flush then left a stale .cdx pointing into the truncated file. Fix: emit a self-referencing identical-payload-digest revisit instead of nothing, still counted as unbacked, so the guard keeps protecting the previous archive exactly as it does for a real 304. Classifying a notmodified htsblk by warc_resphdr's presence was too broad: it also matched back_add()'s cache-priority branch, which never sent a request at all and is unrelated to the back_wait() hacks #839 targets. Replaced it with an explicit hts_boolean, warc_forced_notmodified, set only inside back_wait()'s 304-swap block from the already-computed server_sent_304, leaving every other notmodified path (including back_add()'s) at its default and thus at its prior behavior. tests/134 now asserts the exact peer IP instead of mere presence, since a zeroed-but-AF_INET address also satisfies a non-empty check. tests/135 gained a second scenario: an all-forced-not-modified rerun against the same archive name, which is the shape that lets the swap guard's bug actually destroy the previous archive; the mixed-fixture scenario alone couldn't see it, since a real 304 alongside it already keeps the guard's counter above zero. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com> * Cover the cache-priority path and gate the digest revisit on OpenSSL warc_forced_notmodified had exactly one assignment site, inside back_wait()'s 304-swap block. back_add()'s cache-priority branch (-C1, a lock-file resume, or opt->state.stop) sets notmodified too but never sent a request, so it fell through to the zero default and got classified as a genuine 304, emitting a server-not-modified revisit around a synthesized "HTTP/1.1 200 OK" block. Set the flag there as well: no request there ever means no server 304 to claim. The identical-payload-digest revisit for an engine-forced not-modified also assumed a digest was available, but payload_digest_b32() returns nothing without OpenSSL. Gate the profile on have_pdig; write no record when it's absent, since there's neither a digest to point a revisit at nor a real exchange to record as a response, keeping w->unbacked_revisits incremented either way so the archive-protection guard still holds. tests/136 drives the cache-priority path with -C1 and checks the same WARC-Profile assertion as tests/135. Both new branches (135 and 136) split on HTTPS_SUPPORT so the no-OpenSSL CI leg exercises the no-digest path instead of skipping it; 135's archive-kept scenario drops --wacz for the same reason, a package needs OpenSSL for its SHA-256 digests and would otherwise skip the one thing it exists to catch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com> * An empty body looks digest-less too, and the profile tests never checked the real 304 has_payload requires body_len > 0, so a genuinely empty body (a real, zero-byte file) fell into the same "no digest" branch as a build without OpenSSL and got silently dropped from the archive instead of a proper identical-payload-digest revisit. A zero-length body has a well-defined digest (sha1 of nothing); compute it directly for that case instead of reusing has_payload's gate. tests/135 asserted mini304's pages were revisits but never which profile, so forcing warc_forced_notmodified true unconditionally (a regression that would mislabel or suppress a real 304) still passed. Added the server-not-modified assertion for both mini304 pages, and a matching zero-length fixture (errmask/empty.dat) plus its identical-payload-digest assertion. tests/136 gained a body-hex check on the fresh archive: its update-pass checks only assert absence, so a warc_write_transaction stub that writes nothing at all went undetected on the no-openssl leg. The unbacked-revisits counter still increments even when nothing gets written (load-bearing: it's what keeps the swap guard from letting an all-unchanged pass overwrite a good archive with an empty one), so the "this pass revisited N URL(s)" log line no longer claims those URLs got a named body; it now says the archive doesn't hold their current content, true whether a record was written or not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com> --------- 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.
hts_rename_over()falls back to unlink-then-rename because Windows'rename()refuses an existing target. Between the unlink and the retry nothing stands in for the destination, so a retry that fails too loses it, and three of the four call sites hand it user data: a mirrored file, a finished.wacz, a rewritten page nothing will re-fetch. The fallback now parks the destination under a free sibling name and drops that copy only once the move has succeeded. If the move fails it puts the destination back, retrying once, and names the parked copy in the log when the move back fails too. That log line is why the function now takes anhttrackp. Without it the survivor has no name anywhere on disk or in the log, and an--updatepurge deletes it unnoticed.What this does not buy: the window is no shorter than master's. It is one rename wide either way. It is only less destructive, because the old content sits somewhere on disk at every instant instead of being unlinked. A crash between the two renames still leaves
dstabsent with its content beside it, so this is not atomic, and the header no longer claims it is. On WindowsMoveFileEx(MOVEFILE_REPLACE_EXISTING)is atomic and would remove this fallback and its crash window entirely. That is a design change, so it is noted here rather than done.Which failures reach the fallback at all is unchanged from #784. POSIX
rename()clobbers, so the fallback is dead code there and the selftest drives it directly. TheLD_PRELOADshim covers the Windows-shaped path, and a new mode fails the move back so both the retry and the parked-copy outcome get exercised.The aside name is deterministic rather than unique per attempt, and the probe is check-then-act. On Windows
rename()refuses an existing target so it fails safe, and on POSIX the fallback is unreachable outside the selftest, so I left it.Closes #790