diff --git a/.gitignore b/.gitignore index 7a91330..3a672cb 100644 --- a/.gitignore +++ b/.gitignore @@ -12,6 +12,10 @@ # see docs/development.adoc) /data/*.pmtiles +# Per-country .osm.pbf cache for offline.pmtiles rebuilds (also large, +# also local-only — see PmtilesBuilder.cached_extract) +/data/geofabrik_cache/ + # Offline Wikipedia ZIM for kiwix-serve (also large, also local-only — # same story, see docs/development.adoc) /data/kiwix/* diff --git a/docs/development.adoc b/docs/development.adoc index 894eef3..4641f28 100644 --- a/docs/development.adoc +++ b/docs/development.adoc @@ -99,10 +99,16 @@ There's no standalone per-country file to individually delete — the *whole current selection*. "Remove a country" and "refresh a country" are both just "change the selection, then Apply" (unticking a country and applying rebuilds without it; nothing to separately -delete). Every Apply re-extracts the entire selection from scratch, not -just what changed — fine given extraction is fast (byte-range requests -against remote archives, not per-tile fetches), but worth revisiting if -someone selects half of Europe at once and finds it slow. +delete). terrain.pmtiles re-extracts its selection from scratch on +every Apply — fine given that extraction is byte-range requests +against a remote archive, not full downloads. offline.pmtiles, by +contrast, downloads a whole country's `.osm.pbf` from Geofabrik (can be +multi-GB), so it keeps a per-country cache under +`data/geofabrik_cache/` instead of re-fetching on every Apply — a +country already cached is only re-downloaded if Geofabrik's own +`Last-Modified` for it has moved on since (checked with a cheap `HEAD`, +see `PmtilesBuilder.cached_extract`), so growing or shrinking the +selection only touches whatever's actually new to it. This wraps exactly the manual workflows documented above (`lib/maptool/pmtiles_builder.rb`). offline.pmtiles downloads each diff --git a/lib/maptool/pmtiles_builder.rb b/lib/maptool/pmtiles_builder.rb index 9f9cd50..7abe4bb 100644 --- a/lib/maptool/pmtiles_builder.rb +++ b/lib/maptool/pmtiles_builder.rb @@ -179,17 +179,17 @@ module Maptool # finishes — raises BuildError (the caller turns this into an error # response) if nothing's selected yet, or one's already running. # - # download_extract exists as an injectable seam so tests can exercise - # the rest of this (scratch dir, osmium/planetiler invocation, atomic - # rename) without a real network call — same reasoning test_helper.rb - # already gives for keeping /api/plugins route tests off the real - # Overpass API. - def self.build_offline(data_dir:, download_extract: method(:download_extract)) + # download_extract/fetch_last_modified exist as injectable seams so + # tests can exercise the rest of this (scratch dir, osmium/planetiler + # invocation, atomic rename, the per-country cache below) without a + # real network call — same reasoning test_helper.rb already gives for + # keeping /api/plugins route tests off the real Overpass API. + def self.build_offline(data_dir:, download_extract: method(:download_extract), fetch_last_modified: method(:fetch_geofabrik_last_modified)) codes = selected_regions(data_dir) raise BuildError, "select at least one region first" if codes.empty? begin_run!(OFFLINE_MUTEX, OFFLINE_STATUS) - Thread.new { run_job(OFFLINE_MUTEX, OFFLINE_STATUS) { extract_offline(codes, data_dir, download_extract) } } + Thread.new { run_job(OFFLINE_MUTEX, OFFLINE_STATUS) { extract_offline(codes, data_dir, download_extract, fetch_last_modified) } } end # fetch_manifest: same injectable-seam reasoning as download_extract @@ -228,13 +228,11 @@ module Maptool # tiles.openfreemap.org's Liberty style already expects, so # public/js/map.js's offlineStyle() can reuse ONLINE_STYLE_PATCH # unchanged instead of a separate protomaps-themes-base palette. - def self.extract_offline(codes, data_dir, download_extract) + def self.extract_offline(codes, data_dir, download_extract, fetch_last_modified) scratch_dir = Dir.mktmpdir("maptool-offline") sources = codes.map do |code| slug = GEOFABRIK_SLUGS.fetch(code) { raise BuildError, "no Geofabrik source known for region #{code}" } - dest = File.join(scratch_dir, "#{code}.osm.pbf") - last_modified = download_extract.call(geofabrik_url(slug), dest) - { "code" => code, "slug" => slug, "last_modified" => last_modified, "path" => dest } + cached_extract(code, slug, data_dir, download_extract, fetch_last_modified) end osm_path = @@ -261,6 +259,41 @@ module Maptool end private_class_method :geofabrik_url + # Per-country .osm.pbf cache, kept across builds under data_dir (not + # scratch_dir, which is wiped every run) — changing the region + # selection re-downloads only the countries that are new to it, and + # re-running the same selection re-downloads nothing unless Geofabrik's + # own Last-Modified for that country moved on. A stamp file next to + # each cached extract records the Last-Modified it was fetched with; + # a HEAD check (cheap) decides whether the multi-GB GET (not cheap) is + # skippable. + def self.cached_extract(code, slug, data_dir, download_extract, fetch_last_modified) + cache_dir = geofabrik_cache_dir(data_dir) + FileUtils.mkdir_p(cache_dir) + path = File.join(cache_dir, "#{code}.osm.pbf") + stamp_path = "#{path}.last_modified" + url = geofabrik_url(slug) + + if File.exist?(path) && File.exist?(stamp_path) + current = begin + fetch_last_modified.call(url) + rescue BuildError + nil + end + return { "code" => code, "slug" => slug, "last_modified" => current, "path" => path } if current && current == File.read(stamp_path) + end + + last_modified = download_extract.call(url, path) + File.write(stamp_path, last_modified.to_s) + { "code" => code, "slug" => slug, "last_modified" => last_modified, "path" => path } + end + private_class_method :cached_extract + + def self.geofabrik_cache_dir(data_dir) + File.join(data_dir, "geofabrik_cache") + end + private_class_method :geofabrik_cache_dir + # Real network implementation of the download_extract seam above — # streams the extract to dest (some of these are multi-GB — Net::HTTP's # plain #get buffers the whole response body in memory first, which diff --git a/test/test_pmtiles_builder.rb b/test/test_pmtiles_builder.rb index 7c45401..d91bcb7 100644 --- a/test/test_pmtiles_builder.rb +++ b/test/test_pmtiles_builder.rb @@ -102,8 +102,12 @@ class PmtilesBuilderTest < Minitest::Test # download_extract stub: writes a marker file (same "fake" content the # pmtiles/osmium/java fixtures use) instead of hitting Geofabrik, and # returns a fixed Last-Modified so sidecar-meta assertions are stable. - def fake_download(last_modified: "Wed, 10 Sep 2026 08:00:01 GMT") - ->(_url, dest) { + # `calls` (an Array, shared across builds if the caller wants it) records + # every URL actually downloaded, for asserting the per-country cache + # skipped (or didn't skip) a re-download. + def fake_download(last_modified: "Wed, 10 Sep 2026 08:00:01 GMT", calls: nil) + ->(url, dest) { + calls << url if calls File.write(dest, "fake") last_modified } @@ -189,6 +193,73 @@ class PmtilesBuilderTest < Minitest::Test assert_equal %w[SK AT], codes end + # The actual bug report this covers: hitting "Apply" after tweaking the + # region selection used to re-download every selected country's .osm.pbf + # from scratch, even ones already fetched by an earlier build. + def test_build_offline_skips_redownloading_an_unchanged_cached_country + select_regions("SK") + calls = [] + Maptool::PmtilesBuilder.build_offline(data_dir: @data_dir, download_extract: fake_download(calls: calls)).join + assert_equal 1, calls.size + + Maptool::PmtilesBuilder.build_offline( + data_dir: @data_dir, + download_extract: fake_download(calls: calls), + fetch_last_modified: ->(_url) { "Wed, 10 Sep 2026 08:00:01 GMT" }, + ).join + + assert_equal 1, calls.size + assert_equal "done", Maptool::PmtilesBuilder.offline_status[:state] + end + + def test_build_offline_redownloads_a_country_whose_extract_changed + select_regions("SK") + calls = [] + Maptool::PmtilesBuilder.build_offline(data_dir: @data_dir, download_extract: fake_download(calls: calls)).join + + Maptool::PmtilesBuilder.build_offline( + data_dir: @data_dir, + download_extract: fake_download(calls: calls, last_modified: "Thu, 11 Sep 2026 08:00:01 GMT"), + fetch_last_modified: ->(_url) { "Thu, 11 Sep 2026 08:00:01 GMT" }, + ).join + + assert_equal 2, calls.size + end + + # A cache-freshness check that itself fails (Geofabrik unreachable) should + # fall back to re-downloading rather than aborting the build. + def test_build_offline_redownloads_when_the_cache_check_fails + select_regions("SK") + calls = [] + Maptool::PmtilesBuilder.build_offline(data_dir: @data_dir, download_extract: fake_download(calls: calls)).join + + fetch_last_modified = ->(_url) { raise Maptool::PmtilesBuilder::BuildError, "could not reach geofabrik" } + Maptool::PmtilesBuilder.build_offline( + data_dir: @data_dir, + download_extract: fake_download(calls: calls), + fetch_last_modified: fetch_last_modified, + ).join + + assert_equal 2, calls.size + assert_equal "done", Maptool::PmtilesBuilder.offline_status[:state] + end + + def test_build_offline_only_downloads_a_newly_added_country + select_regions("SK") + calls = [] + Maptool::PmtilesBuilder.build_offline(data_dir: @data_dir, download_extract: fake_download(calls: calls)).join + + select_regions("SK", "AT") + Maptool::PmtilesBuilder.build_offline( + data_dir: @data_dir, + download_extract: fake_download(calls: calls), + fetch_last_modified: ->(_url) { "Wed, 10 Sep 2026 08:00:01 GMT" }, + ).join + + assert_equal 1, calls.count { |url| url.include?("slovakia") } + assert_equal 1, calls.count { |url| url.include?("austria") } + end + def test_offline_meta_is_nil_before_any_build assert_nil Maptool::PmtilesBuilder.offline_meta(@data_dir) end