Install map archives transactionally - #108
Closed
FrogAi wants to merge 2 commits into
Closed
Conversation
Contributor
Author
|
Closing this version because the 202-line transactional subsystem is disproportionate to the demonstrated failure, and its startup orphan sweep removes every .mapd-install-* directory, including a different live worker's staging directory while duplicate download ownership remains possible. Archive installation should be reassessed only after download lifecycle ownership is fixed, with a smaller scope. |
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.
Summary
Motivation
The existing extractor writes archive members directly into the live map directory. A malformed header, truncated body, gzip checksum failure, file write failure, or process error can therefore leave a mixture of old and partially extracted files. Those failures are logged, but the archive can still be removed and counted as downloaded.
The current file open mode also does not truncate existing files, so replacing a tile with a shorter payload can retain bytes from the old file. Because each downloaded archive contains one complete
offline/<latitude>/<longitude>group, installing it as one validated group is the correct ownership boundary.Behavior
Validation
68813e05confirmed direct live writes, non-truncating file opens, undifferentiated tar termination, and progress updates after extraction errors.c92bda11db7ba1936d58611182b5ecf0ec3efdbawith no material blockers. The reviewed matrix covers invalid gzip, malformed/truncated tar, corrupt trailers, path/type/duplicate rejection, write/sync/close failures, both rename failures, rollback failure preservation, exact shorter replacement, and successful replacement.make buildsuccessfully for exact headc64d04e. The workflow establishes compilation; the failure matrix is static source-path validation rather than executed fault injection or a tile replay.c92bda1installed twelve real production archives (1.7 KB to 55 MB, four continents) and confirmed byte-identical output trees against a verbatim baseline port, reproduced all four fixed baseline defects, and confirmed the CI claim. It raised three items, all addressed inc64d04e:defer, and the repo installs no signal handlers, so any kill or power loss stranded a.mapd-install-*tree (~40 MB each) that nothing ever swept — the existing sweep only covers<base>/tmp.downloadBoundsnow removes stale.mapd-install-*directories before starting. Verified: three interrupted installs left three orphans that survived both thetmpsweep and a later successful install on the previous head, and are reclaimed now.archiveInstallOpsinjection seam is removed (~30 of the original lines). It had a single caller always passing the default, no tests, and could not reach the failure paths it existed for —os.Openandgzip.NewReaderbypassed it entirely. Call sites now use theosfunctions directly.os.FileMode(header.Mode).Perm()is replaced with a fixed0o755, matching base. Verified: an archive whose group entry carries mode0555installed a read-onlyoffline/38on the previous head and installs0755now. The mode is umask-masked regardless, so honoring the archive had no upside.tar.TypeRegAcase (Go normalizes it beforeNext()returns) and aname == ""check already covered byclean == ".".Compatibility and scope
settings/download.go, because Own the map download progress lifecycle #107 inserts a cancellation check against the block this PR replaces. The verified resolution keeps both, with the cancel check first andinstallArchiveafter it, leaving theos.Removeandcontinueintact — that ordering matters so a cancel still discards an already-downloaded archive as Own the map download progress lifecycle #107 does on base. The resolved merge, and Install map archives transactionally #108+Own the map download progress lifecycle #107+Ignore invalid download paths instead of panicking #109 together, build and test clean. #109 and #110 merge without conflict.continueon install failure skips Own the map download progress lifecycle #107's bottom-of-loop progress publish, so the progress bar stalls through a run of failing archives. Worth a follow-up, not addressed here.