Repository navigation
Conversation
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.
If "extract_box.sh" or "add_locations.sh" fails partway through "generate_and_update_planet.sh", the loop keeps going with the previous box's "map.osm.pbf". "mapd generate" then writes the new box's tiles from the wrong box's roads and "upload_offline.sh" uploads them over the good ones. "compress_offline.sh" has the same problem with "tar". A failed "tar" leaves a broken ".tar.gz" behind and the script still exits 0, and its 16-thread limit doesn't actually limit anything, since "ch" is a regular file, so "read x < ch" returns right away.
"download_planet.sh" has a quieter version of the same problem. If an old "planet-daily.osm.pbf" is already there, "wget" saves the new one as "planet-daily.osm.pbf.1" and exits 0, so the run rebuilds every tile from the old planet.
This adds "set -e" to "generate_and_update_planet.sh" and "download_generate_and_update_planet.sh", replaces the compress loop with "xargs -P 16" under "set -e", and has "wget" write to "planet-daily.osm.pbf" with "-O". The "|| exit 1" on "mapd generate" is gone since "set -e" covers it.
Audit report
Cause
None of the pipeline scripts use "set -e", so a failed step only stops the run if something checks it, and only "mapd generate" had "|| exit 1". "extract_box.sh" and "add_locations.sh" write with "--overwrite", so after a failure the previous box's "box.osm.pbf" and "map.osm.pbf" are still there for the next step to read. For example, "osmium extract" with an unreadable input exits 1 and leaves the old "box.osm.pbf" byte for byte as it was. Since every tile is still a valid archive, devices download them without complaint and get empty tiles for that region.
"compress_offline.sh" ends with "echo", so it always exits 0 whatever "tar" did. Its throttle reads from "ch", which "touch" creates as a plain file, so "read" hits EOF immediately and every directory starts its "tar" at once.
What could break
I went through every command in these scripts and the ones they call for anything that returns non-zero in a working run, since "set -e" would stop on it. Real "osmium extract" and "add-locations-to-ways" on an all-ocean box exit 0, "mapd generate" on an empty map exits 0, "rm -r offline" always has a directory to remove, and the "for (( ))" headers don't trigger it. "set -e" isn't inherited by the child scripts, but each one ends with the command that matters, so its exit status still comes through.
One behavior change worth knowing about. If "rclone" fails after its own retries, the whole run now stops instead of moving to the next box. That seemed better than carrying on silently, but it's your call.
Archives
The "xargs" line archives the same directories with the same "tar -czvf" command, so the names and paths inside are unchanged ("offline/46/-124/..."). On real tiles generated from the Oregon extract, archives from the old script and the "xargs" line were identical with "diff -r". gzip writes a zero mtime here, so the output doesn't depend on timing.
Testing
The pipeline ran in "golang:1.26.2-bookworm" with stub "osmium", "rclone", "wget" and "mapd" so each step could be made to fail on purpose (results in the table above). A stub "tar" that records how many copies run at once gave a maximum of 100 for the old script and 16 for the new one over 100 directories. With real "tar" the new script made the same two archives with the same contents.
For "download_planet.sh" I ran the real "wget" against a local web server. With an old planet already in the folder, main left it in place and wrote "planet-daily.osm.pbf.1", and this change replaced it. With a download that fails, "wget" exits 8 either way, so under "set -e" the run stops before generating anything.
Not covered
I didn't run the real planet pipeline or upload anything.