Cache repair deletes the old cache before a rename it never checks - #817
Merged
Conversation
Both zip-repair paths removed the damaged cache, moved repair.zip onto its name without looking at the result, and announced a successful recovery either way. A refused move left the entries under a name nothing reads and no cache at all. Go through hts_rename_over(), which keeps the destination when it cannot replace it, and report the failure instead of claiming success. Closes #786 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
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>
The Windows job iterates a fixed glob, so a 106_engine-* test never ran there; name it in the list. The move it exercises is the one whose failure mode is Windows-specific. 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>
…hing On Windows the rename destination always exists, so every cache repair takes hts_rename_over()'s fallback; without #790 that fallback still removes the cache before a retry that can fail. Stacking the two keeps the repair path away from the destructive shape. Carries the #824 fix in the same block: unzRepair() reports Z_OK with no entries for an input that never had a local file header, and its output can be short on a full disk, so both call sites committed an empty or truncated archive over the cache. Both now go through one cache_repair() that refuses a recovery holding nothing or failing to open. Closes #824 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>
Carries the here-string reconciliation with #822 and the aside directory-refusal fix. TESTS tail keeps both new entries in order. Signed-off-by: Xavier Roche <roche@httrack.com>
Resolve the tests/Makefile.am TESTS-tail conflict: keep 106, 110 and 111 in numeric order. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
Resolve tests/renamefail.c: keep the repair/repair-eexist narrowing (#786) that master's simpler locked-only shim lacks; the aside-fails and rename-over regimes carry through unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
Register 106_engine-repair-rename.test in tests-list.mk after the #845 TESTS-list restructure moved it out of tests/Makefile.am. 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.
Both cache-repair paths removed the damaged cache, moved
repair.ziponto its name without looking at the result, and printed "successfully recovered" either way. When the move failed, the recovered entries sat under a name nothing reads and the user had no cache at all. Both now go through onecache_repair()that reports why it failed, and-#Rexits nonzero rather than claiming a recovery it did not perform.This also folds #824, which lives in the same block.
unzRepair()returnsZ_OKwith zero entries for an input that never had a local file header, and its output can be short on a full disk, so both sites committed an empty 22-byte archive over a cache whose bytes were still carvable by hand. A recovery that holds no entry, or that does not open, no longer replaces anything.Based on #816, and that is a correctness requirement rather than a merge convenience. On Windows the destination always exists, so every repair takes
hts_rename_over()'s fallback, which without #816 still removes the cache before a retry that can fail. On its own this PR closes the window on POSIX, where the singlerename()is atomic; on Windows it only reports the failure and keepsrepair.zipfor a manual retry until #816 lands.The test drives both call sites through a rename shim, including the Windows-shaped refusal that goes through the fallback.
Closes #786
Closes #824