From 04e91a57dc2d679d8123a976cb8f79267f77d372 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Tue, 14 Jul 2026 20:51:00 +0200 Subject: [PATCH 01/31] fix: quiet handled geocoder provider errors --- CHANGELOG.md | 1 + app/services/places/name_fetcher.rb | 3 + .../reverse_geocoding/points/fetch_data.rb | 9 +++ app/services/visits/names/fetcher.rb | 4 ++ spec/services/places/name_fetcher_spec.rb | 28 ++++++++++ .../points/fetch_data_spec.rb | 56 +++++++++++++++++++ spec/services/visits/names/fetcher_spec.rb | 35 ++++++++++++ 7 files changed, 136 insertions(+) create mode 100644 spec/services/visits/names/fetcher_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index c2422da32..3a20a8a87 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -78,6 +78,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Fixed - Family location history now actually shows up on Map v2: the history endpoint read coordinates from the legacy `latitude`/`longitude` columns, which are empty on instances that only store the PostGIS `lonlat` value, so nothing was drawn. Coordinates are now derived from `lonlat` (#2977) +- Reverse geocoding and place-name provider outages no longer flood error reporting with handled timeouts, TLS connection failures, or invalid provider responses. - Declining the "Move the visit here?" prompt when picking a distant place for a Map v2 visit no longer renames the visit to that place. - Place visit detection no longer leaves an empty duplicate visit behind when a new point bridges two previously separate visits at the same place — they are now merged into one. The nightly re-scan also leaves confirmed visits untouched, so a new nearby point can no longer pull points out of a visit you already confirmed. - Re-evaluating anomalous points now refreshes the map immediately instead of occasionally serving a cached copy of the points until the next change. diff --git a/app/services/places/name_fetcher.rb b/app/services/places/name_fetcher.rb index f7e827e38..f6e1e6e14 100644 --- a/app/services/places/name_fetcher.rb +++ b/app/services/places/name_fetcher.rb @@ -41,6 +41,9 @@ def call place.visits.where(name: Place::DEFAULT_NAME).update_all(name: name) if name.present? place end + rescue Geocoder::Error, Geocoder::LookupTimeout => e + Rails.logger.warn("Geocoding provider error in NameFetcher for place #{place.id}: #{e.message}") + nil rescue StandardError => e Rails.logger.error("Geocoding error in NameFetcher for place #{place.id}: #{e.message}") ExceptionReporter.call(e) diff --git a/app/services/reverse_geocoding/points/fetch_data.rb b/app/services/reverse_geocoding/points/fetch_data.rb index 38bd968e0..9641167f5 100644 --- a/app/services/reverse_geocoding/points/fetch_data.rb +++ b/app/services/reverse_geocoding/points/fetch_data.rb @@ -45,6 +45,15 @@ def update_point_with_geocoding_data reverse_geocoded_at: Time.current ) end + rescue Geocoder::Error, Geocoder::LookupTimeout => e + Rails.logger.warn("Reverse geocoding provider error for point #{point.id}: #{e.message}") + rescue OpenSSL::SSL::SSLError => e + if e.message.include?('unexpected eof while reading') + Rails.logger.warn("Reverse geocoding provider error for point #{point.id}: #{e.message}") + else + Rails.logger.error("Reverse geocoding error for point #{point.id}: #{e.message}") + ExceptionReporter.call(e) + end rescue StandardError => e Rails.logger.error("Reverse geocoding error for point #{point.id}: #{e.message}") ExceptionReporter.call(e) diff --git a/app/services/visits/names/fetcher.rb b/app/services/visits/names/fetcher.rb index f7d062667..3491bf1a3 100644 --- a/app/services/visits/names/fetcher.rb +++ b/app/services/visits/names/fetcher.rb @@ -22,6 +22,10 @@ def geocoder_results @geocoder_results ||= Geocoder.search( center, limit: 10, distance_sort: true, radius: 1, units: :km ) + rescue Geocoder::Error, Geocoder::LookupTimeout => e + Rails.logger.warn("Geocoding provider error while fetching a visit name: #{e.message}") + + [] rescue StandardError => e ExceptionReporter.call(e) diff --git a/spec/services/places/name_fetcher_spec.rb b/spec/services/places/name_fetcher_spec.rb index b1be08b1d..f2be3431b 100644 --- a/spec/services/places/name_fetcher_spec.rb +++ b/spec/services/places/name_fetcher_spec.rb @@ -141,6 +141,34 @@ end end + context 'when the geocoder provider times out' do + before do + allow(ExceptionReporter).to receive(:call) + allow(Rails.logger).to receive(:warn) + allow(Geocoder).to receive(:search).and_raise(Geocoder::LookupTimeout.new('execution expired')) + end + + it 'returns nil without reporting an application exception' do + expect(service.call).to be_nil + expect(ExceptionReporter).not_to have_received(:call) + expect(Rails.logger).to have_received(:warn).with(/Geocoding provider error in NameFetcher/) + end + end + + context 'when geocoding fails unexpectedly' do + let(:error) { StandardError.new('unexpected failure') } + + before do + allow(ExceptionReporter).to receive(:call) + allow(Geocoder).to receive(:search).and_raise(error) + end + + it 'reports the application exception' do + expect(service.call).to be_nil + expect(ExceptionReporter).to have_received(:call).with(error) + end + end + context 'when geocoding returns no results' do before do allow(Geocoder).to receive(:search).and_return([]) diff --git a/spec/services/reverse_geocoding/points/fetch_data_spec.rb b/spec/services/reverse_geocoding/points/fetch_data_spec.rb index 1546335a7..77c22cdba 100644 --- a/spec/services/reverse_geocoding/points/fetch_data_spec.rb +++ b/spec/services/reverse_geocoding/points/fetch_data_spec.rb @@ -169,4 +169,60 @@ expect { fetch_data }.not_to(change { point.reload.city }) end end + + context 'when the geocoder provider is temporarily unavailable' do + before do + allow(ExceptionReporter).to receive(:call) + allow(Rails.logger).to receive(:warn) + allow(Geocoder).to receive(:search).and_raise(Geocoder::LookupTimeout.new('execution expired')) + end + + it 'does not report a handled provider outage as an application exception' do + expect { fetch_data }.not_to raise_error + expect(ExceptionReporter).not_to have_received(:call) + expect(Rails.logger).to have_received(:warn).with(/Reverse geocoding provider error for point #{point.id}/) + end + end + + context 'when the geocoder provider returns an invalid response' do + before do + allow(ExceptionReporter).to receive(:call) + allow(Rails.logger).to receive(:warn) + allow(Geocoder).to receive(:search).and_raise(Geocoder::ResponseParseError.new('bad gateway')) + end + + it 'does not report a handled provider response as an application exception' do + expect { fetch_data }.not_to raise_error + expect(ExceptionReporter).not_to have_received(:call) + expect(Rails.logger).to have_received(:warn).with(/Reverse geocoding provider error for point #{point.id}/) + end + end + + context 'when the geocoder provider closes the TLS connection unexpectedly' do + before do + allow(ExceptionReporter).to receive(:call) + allow(Rails.logger).to receive(:warn) + allow(Geocoder).to receive(:search).and_raise(OpenSSL::SSL::SSLError.new('unexpected eof while reading')) + end + + it 'does not report a handled provider connection failure as an application exception' do + expect { fetch_data }.not_to raise_error + expect(ExceptionReporter).not_to have_received(:call) + expect(Rails.logger).to have_received(:warn).with(/Reverse geocoding provider error for point #{point.id}/) + end + end + + context 'when the geocoder TLS failure is not transient' do + let(:error) { OpenSSL::SSL::SSLError.new('certificate verify failed') } + + before do + allow(ExceptionReporter).to receive(:call) + allow(Geocoder).to receive(:search).and_raise(error) + end + + it 'reports the application exception' do + expect { fetch_data }.not_to raise_error + expect(ExceptionReporter).to have_received(:call).with(error) + end + end end diff --git a/spec/services/visits/names/fetcher_spec.rb b/spec/services/visits/names/fetcher_spec.rb new file mode 100644 index 000000000..3418719e3 --- /dev/null +++ b/spec/services/visits/names/fetcher_spec.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Visits::Names::Fetcher do + subject(:fetch_name) { described_class.new([10.0, 10.0]).call } + + context 'when the geocoder provider times out' do + before do + allow(ExceptionReporter).to receive(:call) + allow(Rails.logger).to receive(:warn) + allow(Geocoder).to receive(:search).and_raise(Geocoder::LookupTimeout.new('execution expired')) + end + + it 'returns no name without reporting an application exception' do + expect(fetch_name).to be_nil + expect(ExceptionReporter).not_to have_received(:call) + expect(Rails.logger).to have_received(:warn).with(/Geocoding provider error while fetching a visit name/) + end + end + + context 'when name building fails unexpectedly' do + let(:error) { StandardError.new('unexpected failure') } + + before do + allow(ExceptionReporter).to receive(:call) + allow(Geocoder).to receive(:search).and_raise(error) + end + + it 'reports the application exception' do + expect(fetch_name).to be_nil + expect(ExceptionReporter).to have_received(:call).with(error) + end + end +end From 8756850b05ac3d4eb75d7c636a8f410a590c1ae0 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Wed, 15 Jul 2026 21:19:00 +0200 Subject: [PATCH 02/31] Retry reverse geocoding point write timeouts --- CHANGELOG.md | 1 + .../reverse_geocoding/points/fetch_data.rb | 12 ++++++------ .../reverse_geocoding/points/fetch_data_spec.rb | 14 ++++++++++++++ 3 files changed, 21 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c2422da32..a30d7ebc4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -78,6 +78,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Fixed - Family location history now actually shows up on Map v2: the history endpoint read coordinates from the legacy `latitude`/`longitude` columns, which are empty on instances that only store the PostGIS `lonlat` value, so nothing was drawn. Coordinates are now derived from `lonlat` (#2977) +- Reverse geocoding retries point updates that time out while waiting on concurrent writes. - Declining the "Move the visit here?" prompt when picking a distant place for a Map v2 visit no longer renames the visit to that place. - Place visit detection no longer leaves an empty duplicate visit behind when a new point bridges two previously separate visits at the same place — they are now merged into one. The nightly re-scan also leaves confirmed visits untouched, so a new nearby point can no longer pull points out of a visit you already confirmed. - Re-evaluating anomalous points now refreshes the map immediately instead of occasionally serving a cached copy of the points until the next change. diff --git a/app/services/reverse_geocoding/points/fetch_data.rb b/app/services/reverse_geocoding/points/fetch_data.rb index 38bd968e0..3a221647d 100644 --- a/app/services/reverse_geocoding/points/fetch_data.rb +++ b/app/services/reverse_geocoding/points/fetch_data.rb @@ -22,13 +22,13 @@ def call private - DEADLOCK_MAX_RETRIES = 3 + WRITE_MAX_RETRIES = 3 def update_point_with_geocoding_data response = Geocoder.search([point.lat, point.lon]).first if response.blank? - with_deadlock_retry { point.update!(reverse_geocoded_at: Time.current) } + with_write_retry { point.update!(reverse_geocoded_at: Time.current) } return end @@ -36,7 +36,7 @@ def update_point_with_geocoding_data country_record = Country.find_by(name: response.country) if response.country - with_deadlock_retry do + with_write_retry do point.update!( city: response.city, country_name: response.country, @@ -50,13 +50,13 @@ def update_point_with_geocoding_data ExceptionReporter.call(e) end - def with_deadlock_retry + def with_write_retry retries = 0 begin yield - rescue ActiveRecord::Deadlocked => e + rescue ActiveRecord::Deadlocked, ActiveRecord::QueryCanceled => e retries += 1 - raise e if retries > DEADLOCK_MAX_RETRIES + raise e if retries > WRITE_MAX_RETRIES sleep(0.1 * retries) retry diff --git a/spec/services/reverse_geocoding/points/fetch_data_spec.rb b/spec/services/reverse_geocoding/points/fetch_data_spec.rb index 1546335a7..d7fcf38fe 100644 --- a/spec/services/reverse_geocoding/points/fetch_data_spec.rb +++ b/spec/services/reverse_geocoding/points/fetch_data_spec.rb @@ -59,6 +59,20 @@ expect(Geocoder).to have_received(:search).with([point.lat, point.lon]) end + it 'retries when the point update times out waiting for a lock' do + attempts = 0 + allow(Point).to receive(:find).with(point.id).and_return(point) + allow(point).to receive(:update!).and_wrap_original do |method, *args| + attempts += 1 + raise ActiveRecord::QueryCanceled, 'canceling statement due to statement timeout' if attempts == 1 + + method.call(*args) + end + + expect { fetch_data }.to change { point.reload.city }.from(nil).to('Berlin') + expect(attempts).to eq(2) + end + context 'when store_geodata? is disabled' do before do allow(DawarichSettings).to receive(:store_geodata?).and_return(false) From 7ae48592742f3c7a4790edea501b5634409c2c8a Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Fri, 17 Jul 2026 19:28:00 +0200 Subject: [PATCH 03/31] Retry transient OwnTracks point write timeouts --- CHANGELOG.md | 1 + app/services/own_tracks/point_creator.rb | 24 +++++++++++++++---- .../services/own_tracks/point_creator_spec.rb | 13 ++++++++++ 3 files changed, 34 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c2422da32..877174bb9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -78,6 +78,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Fixed - Family location history now actually shows up on Map v2: the history endpoint read coordinates from the legacy `latitude`/`longitude` columns, which are empty on instances that only store the PostGIS `lonlat` value, so nothing was drawn. Coordinates are now derived from `lonlat` (#2977) +- OwnTracks location uploads now retry transient database write contention instead of immediately returning an error. - Declining the "Move the visit here?" prompt when picking a distant place for a Map v2 visit no longer renames the visit to that place. - Place visit detection no longer leaves an empty duplicate visit behind when a new point bridges two previously separate visits at the same place — they are now merged into one. The nightly re-scan also leaves confirmed visits untouched, so a new nearby point can no longer pull points out of a visit you already confirmed. - Re-evaluating anomalous points now refreshes the map immediately instead of occasionally serving a cached copy of the points until the next change. diff --git a/app/services/own_tracks/point_creator.rb b/app/services/own_tracks/point_creator.rb index ca25f4333..2ee8f29a9 100644 --- a/app/services/own_tracks/point_creator.rb +++ b/app/services/own_tracks/point_creator.rb @@ -2,6 +2,7 @@ class OwnTracks::PointCreator RETURNING_COLUMNS = 'id, xmax, timestamp, ST_X(lonlat::geometry) AS longitude, ST_Y(lonlat::geometry) AS latitude' + UPSERT_MAX_RETRIES = 3 attr_reader :params, :user_id @@ -39,13 +40,28 @@ def upsert_points(locations) created_points = [] locations.each_slice(1000) do |batch| - result = Point.archival_safe_upsert_all( - batch, - returning: Arel.sql(RETURNING_COLUMNS) - ) + result = with_upsert_retry do + Point.archival_safe_upsert_all( + batch, + returning: Arel.sql(RETURNING_COLUMNS) + ) + end created_points.concat(result) if result end created_points end + + def with_upsert_retry + retries = 0 + begin + yield + rescue ActiveRecord::Deadlocked, ActiveRecord::QueryCanceled => e + retries += 1 + raise e if retries > UPSERT_MAX_RETRIES + + sleep(0.1 * retries) + retry + end + end end diff --git a/spec/services/own_tracks/point_creator_spec.rb b/spec/services/own_tracks/point_creator_spec.rb index c5d5b5cdd..48474be09 100644 --- a/spec/services/own_tracks/point_creator_spec.rb +++ b/spec/services/own_tracks/point_creator_spec.rb @@ -44,6 +44,19 @@ expect(user.points_count).to eq(Point.where(user_id: user.id).count) end + it 'retries a point upsert canceled by transient database contention' do + attempts = 0 + allow(Point).to receive(:archival_safe_upsert_all).and_wrap_original do |method, *args, **kwargs| + attempts += 1 + raise ActiveRecord::QueryCanceled, 'canceling statement due to statement timeout' if attempts == 1 + + method.call(*args, **kwargs) + end + + expect { call_service }.to change { Point.where(user:).count }.by(1) + expect(attempts).to eq(2) + end + it 'enqueues VisitSuggestingJob when reverse geocoding is enabled (regression for #1749)' do allow(DawarichSettings).to receive(:reverse_geocoding_enabled?).and_return(true) From dba153f541e0ca02b97c92f20f53904e109c8671 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Sat, 18 Jul 2026 15:38:00 +0200 Subject: [PATCH 04/31] Prevent reverse-geocoding place deadlocks --- CHANGELOG.md | 1 + app/services/reverse_geocoding/places/fetch_data.rb | 2 +- .../reverse_geocoding/places/fetch_data_spec.rb | 11 +++++++++++ 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c2422da32..6d770024f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Track generation and user data recalculations now retry a bounded number of times when another job is already processing the same user's tracks, instead of dropping the request and reporting an error; a genuinely stuck lock is logged after the retries are exhausted. The per-user lock also renews itself while a job runs and frees within a minute if a worker dies, so a crashed job no longer blocks a user's track processing for up to half an hour. - Dawarich added to an iOS Home Screen now opens Map v2 instead of an unrelated previously visited page (#3097) - Point uploads (REST API, OwnTracks, Overland, Traccar) now write batches in a consistent order so concurrent uploads no longer deadlock each other, and both uploads and anomaly filtering recover automatically from any remaining transient database deadlocks instead of failing the upload or background job. +- Reverse geocoding overlapping places no longer exhausts retries because of concurrent database deadlocks. - The app and Sidekiq containers no longer crash-loop on startup when `WEB_CONCURRENCY` or `BACKGROUND_PROCESSING_CONCURRENCY` reach the container as an unexpanded `${VAR:-default}` string (seen with some podman-compose versions); the entrypoint now warns and falls back to the default value (#3124) - Cache preheating no longer times out for accounts with large location histories. - Cloud: Changing plans resets the Lite archival-warning state, so a user downgraded to Lite again is notified about archived data again. diff --git a/app/services/reverse_geocoding/places/fetch_data.rb b/app/services/reverse_geocoding/places/fetch_data.rb index 788722e53..89a09f481 100644 --- a/app/services/reverse_geocoding/places/fetch_data.rb +++ b/app/services/reverse_geocoding/places/fetch_data.rb @@ -142,7 +142,7 @@ def save_places(places_to_create, places_to_update) return unless places_to_update.any? - update_attributes = places_to_update.uniq(&:id).map do |place| + update_attributes = places_to_update.uniq(&:id).sort_by(&:id).map do |place| { id: place.id, name: place.name, diff --git a/spec/services/reverse_geocoding/places/fetch_data_spec.rb b/spec/services/reverse_geocoding/places/fetch_data_spec.rb index b44d114f6..c610e5f2f 100644 --- a/spec/services/reverse_geocoding/places/fetch_data_spec.rb +++ b/spec/services/reverse_geocoding/places/fetch_data_spec.rb @@ -436,6 +436,17 @@ expect(existing_place.reload.name).to eq('New Name') end + it 'orders bulk updates by primary key' do + first_place = create(:place) + second_place = create(:place) + allow(Place).to receive(:upsert_all) + + service.send(:save_places, [], [second_place, first_place]) + + expect(Place).to have_received(:upsert_all) + .with(satisfy { |attributes| attributes.pluck(:id) == [first_place.id, second_place.id] }, unique_by: :id) + end + it 'handles empty arrays gracefully' do expect { service.send(:save_places, [], []) }.not_to raise_error end From 0ae855c11e236940f6435613d51dc2a8d993a434 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Mon, 20 Jul 2026 08:26:00 +0200 Subject: [PATCH 05/31] Prevent Null Island cleanup failures on legacy points --- CHANGELOG.md | 1 + app/jobs/data_migrations/cleanup_null_island_job.rb | 4 +++- .../data_migrations/cleanup_null_island_job_spec.rb | 10 ++++++++++ 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c2422da32..c599a1d2c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Google Semantic History and phone Timeline imports now tag points with a per-import tracker id instead of one shared constant, so tracks from different devices are no longer braided together. A one-time backfill rewrites existing points and regenerates affected tracks per user. - Points at exactly (0,0) — a common GPS glitch — are no longer accepted from any ingestion path (API, OwnTracks, Overland, Traccar, file imports) and no longer produce suggested visits at "Null Island". Existing (0,0) points are flagged as anomalies by a one-time cleanup that also removes visits placed at (0,0) and recalculates affected stats and tracks. +- Points at exactly (0,0) — a common GPS glitch — are no longer accepted from any ingestion path (API, OwnTracks, Overland, Traccar, file imports) and no longer produce suggested visits at "Null Island". Existing (0,0) points are flagged as anomalies by a one-time cleanup that also removes visits placed at (0,0), tolerates legacy points without timestamps, and recalculates affected stats and tracks. ## [1.10.1] - 2026-07-19, Berlin diff --git a/app/jobs/data_migrations/cleanup_null_island_job.rb b/app/jobs/data_migrations/cleanup_null_island_job.rb index 5791a9767..473897a49 100644 --- a/app/jobs/data_migrations/cleanup_null_island_job.rb +++ b/app/jobs/data_migrations/cleanup_null_island_job.rb @@ -10,7 +10,9 @@ def perform(user_id = nil) rows = user.points.null_island.pluck(:id, :timestamp, :track_id) track_ids = rows.filter_map(&:last).uniq - affected_months = rows.map do |_, timestamp, _| + affected_months = rows.filter_map do |_, timestamp, _| + next if timestamp.nil? + time = Time.zone.at(timestamp) [time.year, time.month] end.uniq diff --git a/spec/jobs/data_migrations/cleanup_null_island_job_spec.rb b/spec/jobs/data_migrations/cleanup_null_island_job_spec.rb index d411813f6..ed41365f8 100644 --- a/spec/jobs/data_migrations/cleanup_null_island_job_spec.rb +++ b/spec/jobs/data_migrations/cleanup_null_island_job_spec.rb @@ -42,6 +42,16 @@ .and have_enqueued_job(Tracks::RecalculateJob).with(track.id) end + it 'flags legacy points without timestamps and recalculates their tracks' do + zero_point.update_column(:timestamp, nil) + + expect { described_class.perform_now(user.id) }.not_to raise_error + + expect(Tracks::RecalculateJob).to have_been_enqueued.with(track.id) + expect(Stats::CalculatingJob).not_to have_been_enqueued + expect(zero_point.reload.anomaly).to be(true) + end + describe 'fan out' do it 'enqueues a per-user job for every user with (0,0) points' do other_user = create(:user) From bcf49826848b226c2c2fbaa54a2091aee4dd78a6 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Mon, 20 Jul 2026 20:42:00 +0200 Subject: [PATCH 06/31] Retry point write contention in the shared upsert path --- CHANGELOG.md | 2 +- app/models/concerns/archivable.rb | 11 ++++-- app/services/own_tracks/point_creator.rb | 24 ++---------- spec/models/concerns/archivable_spec.rb | 37 +++++++++++++++++++ .../services/own_tracks/point_creator_spec.rb | 13 ------- 5 files changed, 50 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 877174bb9..bf553befd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Google Semantic History and phone Timeline imports now tag points with a per-import tracker id instead of one shared constant, so tracks from different devices are no longer braided together. A one-time backfill rewrites existing points and regenerates affected tracks per user. - Points at exactly (0,0) — a common GPS glitch — are no longer accepted from any ingestion path (API, OwnTracks, Overland, Traccar, file imports) and no longer produce suggested visits at "Null Island". Existing (0,0) points are flagged as anomalies by a one-time cleanup that also removes visits placed at (0,0) and recalculates affected stats and tracks. +- Point uploads from all ingestion paths (REST API, OwnTracks, Overland, Traccar) now retry transient statement and lock-wait timeouts, not just deadlocks, instead of failing the upload. ## [1.10.1] - 2026-07-19, Berlin @@ -78,7 +79,6 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Fixed - Family location history now actually shows up on Map v2: the history endpoint read coordinates from the legacy `latitude`/`longitude` columns, which are empty on instances that only store the PostGIS `lonlat` value, so nothing was drawn. Coordinates are now derived from `lonlat` (#2977) -- OwnTracks location uploads now retry transient database write contention instead of immediately returning an error. - Declining the "Move the visit here?" prompt when picking a distant place for a Map v2 visit no longer renames the visit to that place. - Place visit detection no longer leaves an empty duplicate visit behind when a new point bridges two previously separate visits at the same place — they are now merged into one. The nightly re-scan also leaves confirmed visits untouched, so a new nearby point can no longer pull points out of a visit you already confirmed. - Re-evaluating anomalous points now refreshes the map immediately instead of occasionally serving a cached copy of the points until the next change. diff --git a/app/models/concerns/archivable.rb b/app/models/concerns/archivable.rb index c27fe82bf..41b3bb3f3 100644 --- a/app/models/concerns/archivable.rb +++ b/app/models/concerns/archivable.rb @@ -21,6 +21,11 @@ module Archivable UPSERT_MAX_RETRIES = 3 UPSERT_BACKOFF_BASE = 0.1 UPSERT_BACKOFF_JITTER = 0.05 + UPSERT_CONTENTION_ERRORS = [ + ActiveRecord::Deadlocked, + ActiveRecord::LockWaitTimeout, + ActiveRecord::QueryCanceled + ].freeze class_methods do # Bulk-ingest counterpart of the reset_archival_on_raw_data_change @@ -40,7 +45,7 @@ def archival_safe_upsert_all(rows, returning:) set_clauses << '"updated_at" = CURRENT_TIMESTAMP' unless update_columns.include?(:updated_at) set_clauses.concat(archival_reset_clauses) if update_columns.include?(:raw_data) - with_deadlock_retry do + with_write_contention_retry do upsert_all( rows, unique_by: UPSERT_CONFLICT_KEYS, @@ -52,12 +57,12 @@ def archival_safe_upsert_all(rows, returning:) private - def with_deadlock_retry + def with_write_contention_retry retries = 0 begin yield - rescue ActiveRecord::Deadlocked => e + rescue *UPSERT_CONTENTION_ERRORS => e retries += 1 raise e if retries > UPSERT_MAX_RETRIES diff --git a/app/services/own_tracks/point_creator.rb b/app/services/own_tracks/point_creator.rb index 2ee8f29a9..ca25f4333 100644 --- a/app/services/own_tracks/point_creator.rb +++ b/app/services/own_tracks/point_creator.rb @@ -2,7 +2,6 @@ class OwnTracks::PointCreator RETURNING_COLUMNS = 'id, xmax, timestamp, ST_X(lonlat::geometry) AS longitude, ST_Y(lonlat::geometry) AS latitude' - UPSERT_MAX_RETRIES = 3 attr_reader :params, :user_id @@ -40,28 +39,13 @@ def upsert_points(locations) created_points = [] locations.each_slice(1000) do |batch| - result = with_upsert_retry do - Point.archival_safe_upsert_all( - batch, - returning: Arel.sql(RETURNING_COLUMNS) - ) - end + result = Point.archival_safe_upsert_all( + batch, + returning: Arel.sql(RETURNING_COLUMNS) + ) created_points.concat(result) if result end created_points end - - def with_upsert_retry - retries = 0 - begin - yield - rescue ActiveRecord::Deadlocked, ActiveRecord::QueryCanceled => e - retries += 1 - raise e if retries > UPSERT_MAX_RETRIES - - sleep(0.1 * retries) - retry - end - end end diff --git a/spec/models/concerns/archivable_spec.rb b/spec/models/concerns/archivable_spec.rb index 4e4419abe..44ed6eb8a 100644 --- a/spec/models/concerns/archivable_spec.rb +++ b/spec/models/concerns/archivable_spec.rb @@ -292,6 +292,43 @@ expect(Point).to have_received(:sleep).exactly(3).times end end + + context 'when the upsert is canceled by a transient statement or lock timeout' do + [ActiveRecord::QueryCanceled, ActiveRecord::LockWaitTimeout].each do |error_class| + it "retries #{error_class} and returns the result" do + attempts = 0 + allow(Point).to receive(:upsert_all).and_wrap_original do |original, *args, **kwargs| + attempts += 1 + raise error_class, 'canceling statement' if attempts == 1 + + original.call(*args, **kwargs) + end + allow(Point).to receive(:sleep) + + result = Point.archival_safe_upsert_all( + [base_row.merge(timestamp: 1_700_000_240)], + returning: Arel.sql('id, xmax') + ) + + expect(attempts).to eq(2) + expect(Point.exists?(result.first['id'])).to be true + end + + it "raises #{error_class} after exhausting retries" do + allow(Point).to receive(:upsert_all).and_raise(error_class, 'canceling statement') + allow(Point).to receive(:sleep) + + expect do + Point.archival_safe_upsert_all( + [base_row.merge(timestamp: 1_700_000_300)], + returning: Arel.sql('id, xmax') + ) + end.to raise_error(error_class) + + expect(Point).to have_received(:sleep).exactly(3).times + end + end + end end describe 'raw_data mutation guard' do diff --git a/spec/services/own_tracks/point_creator_spec.rb b/spec/services/own_tracks/point_creator_spec.rb index 48474be09..c5d5b5cdd 100644 --- a/spec/services/own_tracks/point_creator_spec.rb +++ b/spec/services/own_tracks/point_creator_spec.rb @@ -44,19 +44,6 @@ expect(user.points_count).to eq(Point.where(user_id: user.id).count) end - it 'retries a point upsert canceled by transient database contention' do - attempts = 0 - allow(Point).to receive(:archival_safe_upsert_all).and_wrap_original do |method, *args, **kwargs| - attempts += 1 - raise ActiveRecord::QueryCanceled, 'canceling statement due to statement timeout' if attempts == 1 - - method.call(*args, **kwargs) - end - - expect { call_service }.to change { Point.where(user:).count }.by(1) - expect(attempts).to eq(2) - end - it 'enqueues VisitSuggestingJob when reverse geocoding is enabled (regression for #1749)' do allow(DawarichSettings).to receive(:reverse_geocoding_enabled?).and_return(true) From 1f8b8bcc4cd7327e75d578a4bf38cb3024505e19 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Mon, 20 Jul 2026 20:48:00 +0200 Subject: [PATCH 07/31] Move changelog entry to the Unreleased section --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a30d7ebc4..38a08b84b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Fixed +- Reverse geocoding retries point updates that time out while waiting on concurrent writes. - Google Semantic History and phone Timeline imports now tag points with a per-import tracker id instead of one shared constant, so tracks from different devices are no longer braided together. A one-time backfill rewrites existing points and regenerates affected tracks per user. - Points at exactly (0,0) — a common GPS glitch — are no longer accepted from any ingestion path (API, OwnTracks, Overland, Traccar, file imports) and no longer produce suggested visits at "Null Island". Existing (0,0) points are flagged as anomalies by a one-time cleanup that also removes visits placed at (0,0) and recalculates affected stats and tracks. @@ -78,7 +79,6 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Fixed - Family location history now actually shows up on Map v2: the history endpoint read coordinates from the legacy `latitude`/`longitude` columns, which are empty on instances that only store the PostGIS `lonlat` value, so nothing was drawn. Coordinates are now derived from `lonlat` (#2977) -- Reverse geocoding retries point updates that time out while waiting on concurrent writes. - Declining the "Move the visit here?" prompt when picking a distant place for a Map v2 visit no longer renames the visit to that place. - Place visit detection no longer leaves an empty duplicate visit behind when a new point bridges two previously separate visits at the same place — they are now merged into one. The nightly re-scan also leaves confirmed visits untouched, so a new nearby point can no longer pull points out of a visit you already confirmed. - Re-evaluating anomalous points now refreshes the map immediately instead of occasionally serving a cached copy of the points until the next change. From c869059bff1b139d920455457a99af70a66428b2 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Mon, 20 Jul 2026 20:48:00 +0200 Subject: [PATCH 08/31] Move changelog entry to the Unreleased section --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a20a8a87..416c3c0c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Fixed +- Reverse geocoding and place-name provider outages no longer flood error reporting with handled timeouts, TLS connection failures, or invalid provider responses. - Google Semantic History and phone Timeline imports now tag points with a per-import tracker id instead of one shared constant, so tracks from different devices are no longer braided together. A one-time backfill rewrites existing points and regenerates affected tracks per user. - Points at exactly (0,0) — a common GPS glitch — are no longer accepted from any ingestion path (API, OwnTracks, Overland, Traccar, file imports) and no longer produce suggested visits at "Null Island". Existing (0,0) points are flagged as anomalies by a one-time cleanup that also removes visits placed at (0,0) and recalculates affected stats and tracks. @@ -78,7 +79,6 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Fixed - Family location history now actually shows up on Map v2: the history endpoint read coordinates from the legacy `latitude`/`longitude` columns, which are empty on instances that only store the PostGIS `lonlat` value, so nothing was drawn. Coordinates are now derived from `lonlat` (#2977) -- Reverse geocoding and place-name provider outages no longer flood error reporting with handled timeouts, TLS connection failures, or invalid provider responses. - Declining the "Move the visit here?" prompt when picking a distant place for a Map v2 visit no longer renames the visit to that place. - Place visit detection no longer leaves an empty duplicate visit behind when a new point bridges two previously separate visits at the same place — they are now merged into one. The nightly re-scan also leaves confirmed visits untouched, so a new nearby point can no longer pull points out of a visit you already confirmed. - Re-evaluating anomalous points now refreshes the map immediately instead of occasionally serving a cached copy of the points until the next change. From 0e59ea50fe0c08efbd04909d64024afbc3dc91f0 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Mon, 20 Jul 2026 20:55:00 +0200 Subject: [PATCH 09/31] Fold the Null Island changelog entry into a single line --- CHANGELOG.md | 1 - 1 file changed, 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c599a1d2c..04eefc138 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,6 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Fixed - Google Semantic History and phone Timeline imports now tag points with a per-import tracker id instead of one shared constant, so tracks from different devices are no longer braided together. A one-time backfill rewrites existing points and regenerates affected tracks per user. -- Points at exactly (0,0) — a common GPS glitch — are no longer accepted from any ingestion path (API, OwnTracks, Overland, Traccar, file imports) and no longer produce suggested visits at "Null Island". Existing (0,0) points are flagged as anomalies by a one-time cleanup that also removes visits placed at (0,0) and recalculates affected stats and tracks. - Points at exactly (0,0) — a common GPS glitch — are no longer accepted from any ingestion path (API, OwnTracks, Overland, Traccar, file imports) and no longer produce suggested visits at "Null Island". Existing (0,0) points are flagged as anomalies by a one-time cleanup that also removes visits placed at (0,0), tolerates legacy points without timestamps, and recalculates affected stats and tracks. ## [1.10.1] - 2026-07-19, Berlin From 89774544e6e04ff834dc14b0752fafba4e78b4d0 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Mon, 20 Jul 2026 21:03:00 +0200 Subject: [PATCH 10/31] Cover lock-wait timeouts and retry exhaustion in reverse geocoding writes --- .../reverse_geocoding/points/fetch_data.rb | 9 +++-- .../points/fetch_data_spec.rb | 35 ++++++++++++++----- 2 files changed, 33 insertions(+), 11 deletions(-) diff --git a/app/services/reverse_geocoding/points/fetch_data.rb b/app/services/reverse_geocoding/points/fetch_data.rb index 3a221647d..c4dcc855b 100644 --- a/app/services/reverse_geocoding/points/fetch_data.rb +++ b/app/services/reverse_geocoding/points/fetch_data.rb @@ -23,6 +23,11 @@ def call private WRITE_MAX_RETRIES = 3 + WRITE_CONTENTION_ERRORS = [ + ActiveRecord::Deadlocked, + ActiveRecord::LockWaitTimeout, + ActiveRecord::QueryCanceled + ].freeze def update_point_with_geocoding_data response = Geocoder.search([point.lat, point.lon]).first @@ -54,11 +59,11 @@ def with_write_retry retries = 0 begin yield - rescue ActiveRecord::Deadlocked, ActiveRecord::QueryCanceled => e + rescue *WRITE_CONTENTION_ERRORS => e retries += 1 raise e if retries > WRITE_MAX_RETRIES - sleep(0.1 * retries) + sleep((0.1 * retries) + (rand * 0.05)) retry end end diff --git a/spec/services/reverse_geocoding/points/fetch_data_spec.rb b/spec/services/reverse_geocoding/points/fetch_data_spec.rb index d7fcf38fe..dadbe73b1 100644 --- a/spec/services/reverse_geocoding/points/fetch_data_spec.rb +++ b/spec/services/reverse_geocoding/points/fetch_data_spec.rb @@ -59,18 +59,35 @@ expect(Geocoder).to have_received(:search).with([point.lat, point.lon]) end - it 'retries when the point update times out waiting for a lock' do - attempts = 0 + described_class::WRITE_CONTENTION_ERRORS.each do |error_class| + it "retries when the point update raises #{error_class}" do + attempts = 0 + allow(Point).to receive(:find).with(point.id).and_return(point) + allow(point).to receive(:update!).and_wrap_original do |method, *args| + attempts += 1 + raise error_class, 'write contention' if attempts == 1 + + method.call(*args) + end + service = described_class.new(point.id) + allow(service).to receive(:sleep) + + expect { service.call }.to change { point.reload.city }.from(nil).to('Berlin') + expect(attempts).to eq(2) + end + end + + it 'gives up after exhausting the retry budget and reports the failure' do + allow(ExceptionReporter).to receive(:call) allow(Point).to receive(:find).with(point.id).and_return(point) - allow(point).to receive(:update!).and_wrap_original do |method, *args| - attempts += 1 - raise ActiveRecord::QueryCanceled, 'canceling statement due to statement timeout' if attempts == 1 + allow(point).to receive(:update!).and_raise(ActiveRecord::QueryCanceled, 'write contention') + service = described_class.new(point.id) + allow(service).to receive(:sleep) - method.call(*args) - end + service.call - expect { fetch_data }.to change { point.reload.city }.from(nil).to('Berlin') - expect(attempts).to eq(2) + expect(point).to have_received(:update!).exactly(described_class::WRITE_MAX_RETRIES + 1).times + expect(ExceptionReporter).to have_received(:call) end context 'when store_geodata? is disabled' do From 36d4e318da18fcb5711cea5a237c04d3fa208c90 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Mon, 20 Jul 2026 22:40:00 +0200 Subject: [PATCH 11/31] Only treat transient geocoder failures as handled provider errors --- app/services/places/name_fetcher.rb | 10 +++++++--- .../reverse_geocoding/points/fetch_data.rb | 2 +- .../reverse_geocoding/provider_errors.rb | 18 ++++++++++++++++++ app/services/visits/names/fetcher.rb | 8 ++++++-- .../points/fetch_data_spec.rb | 12 ++++++++++++ 5 files changed, 44 insertions(+), 6 deletions(-) create mode 100644 app/services/reverse_geocoding/provider_errors.rb diff --git a/app/services/places/name_fetcher.rb b/app/services/places/name_fetcher.rb index f6e1e6e14..604d57b89 100644 --- a/app/services/places/name_fetcher.rb +++ b/app/services/places/name_fetcher.rb @@ -41,12 +41,16 @@ def call place.visits.where(name: Place::DEFAULT_NAME).update_all(name: name) if name.present? place end - rescue Geocoder::Error, Geocoder::LookupTimeout => e + rescue *ReverseGeocoding::ProviderErrors::TRANSIENT => e Rails.logger.warn("Geocoding provider error in NameFetcher for place #{place.id}: #{e.message}") nil rescue StandardError => e - Rails.logger.error("Geocoding error in NameFetcher for place #{place.id}: #{e.message}") - ExceptionReporter.call(e) + if ReverseGeocoding::ProviderErrors.transient_tls?(e) + Rails.logger.warn("Geocoding provider error in NameFetcher for place #{place.id}: #{e.message}") + else + Rails.logger.error("Geocoding error in NameFetcher for place #{place.id}: #{e.message}") + ExceptionReporter.call(e) + end nil end diff --git a/app/services/reverse_geocoding/points/fetch_data.rb b/app/services/reverse_geocoding/points/fetch_data.rb index 9641167f5..5253c3153 100644 --- a/app/services/reverse_geocoding/points/fetch_data.rb +++ b/app/services/reverse_geocoding/points/fetch_data.rb @@ -45,7 +45,7 @@ def update_point_with_geocoding_data reverse_geocoded_at: Time.current ) end - rescue Geocoder::Error, Geocoder::LookupTimeout => e + rescue *ReverseGeocoding::ProviderErrors::TRANSIENT => e Rails.logger.warn("Reverse geocoding provider error for point #{point.id}: #{e.message}") rescue OpenSSL::SSL::SSLError => e if e.message.include?('unexpected eof while reading') diff --git a/app/services/reverse_geocoding/provider_errors.rb b/app/services/reverse_geocoding/provider_errors.rb new file mode 100644 index 000000000..c26907c91 --- /dev/null +++ b/app/services/reverse_geocoding/provider_errors.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module ReverseGeocoding + module ProviderErrors + TRANSIENT = [ + Geocoder::LookupTimeout, + Geocoder::NetworkError, + Geocoder::ServiceUnavailable, + Geocoder::ResponseParseError + ].freeze + + TRANSIENT_TLS_MESSAGE = 'unexpected eof while reading' + + def self.transient_tls?(error) + error.is_a?(OpenSSL::SSL::SSLError) && error.message.include?(TRANSIENT_TLS_MESSAGE) + end + end +end diff --git a/app/services/visits/names/fetcher.rb b/app/services/visits/names/fetcher.rb index 3491bf1a3..2345f55ca 100644 --- a/app/services/visits/names/fetcher.rb +++ b/app/services/visits/names/fetcher.rb @@ -22,12 +22,16 @@ def geocoder_results @geocoder_results ||= Geocoder.search( center, limit: 10, distance_sort: true, radius: 1, units: :km ) - rescue Geocoder::Error, Geocoder::LookupTimeout => e + rescue *ReverseGeocoding::ProviderErrors::TRANSIENT => e Rails.logger.warn("Geocoding provider error while fetching a visit name: #{e.message}") [] rescue StandardError => e - ExceptionReporter.call(e) + if ReverseGeocoding::ProviderErrors.transient_tls?(e) + Rails.logger.warn("Geocoding provider error while fetching a visit name: #{e.message}") + else + ExceptionReporter.call(e) + end [] end diff --git a/spec/services/reverse_geocoding/points/fetch_data_spec.rb b/spec/services/reverse_geocoding/points/fetch_data_spec.rb index 77c22cdba..0d3eb56f5 100644 --- a/spec/services/reverse_geocoding/points/fetch_data_spec.rb +++ b/spec/services/reverse_geocoding/points/fetch_data_spec.rb @@ -198,6 +198,18 @@ end end + context 'when the geocoder is misconfigured rather than briefly unavailable' do + [Geocoder::InvalidApiKey, Geocoder::ConfigurationError, Geocoder::OverQueryLimitError].each do |error_class| + it "still reports #{error_class} so the outage is not silent" do + allow(ExceptionReporter).to receive(:call) + allow(Geocoder).to receive(:search).and_raise(error_class.new('misconfigured')) + + expect { fetch_data }.not_to raise_error + expect(ExceptionReporter).to have_received(:call) + end + end + end + context 'when the geocoder provider closes the TLS connection unexpectedly' do before do allow(ExceptionReporter).to receive(:call) From 43afcc67e592ce21acfd2d7a2677a84f8821bb9b Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Mon, 20 Jul 2026 23:56:00 +0200 Subject: [PATCH 12/31] Reuse the TLS helper and tighten the changelog wording --- CHANGELOG.md | 2 +- app/services/reverse_geocoding/points/fetch_data.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 416c3c0c4..5a3e29bc5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Fixed -- Reverse geocoding and place-name provider outages no longer flood error reporting with handled timeouts, TLS connection failures, or invalid provider responses. +- Reverse geocoding and place-name provider outages no longer flood error reporting with handled timeouts, dropped TLS connections, or invalid provider responses. A misconfigured or rate-limited provider — a bad API key, for example — is still reported. - Google Semantic History and phone Timeline imports now tag points with a per-import tracker id instead of one shared constant, so tracks from different devices are no longer braided together. A one-time backfill rewrites existing points and regenerates affected tracks per user. - Points at exactly (0,0) — a common GPS glitch — are no longer accepted from any ingestion path (API, OwnTracks, Overland, Traccar, file imports) and no longer produce suggested visits at "Null Island". Existing (0,0) points are flagged as anomalies by a one-time cleanup that also removes visits placed at (0,0) and recalculates affected stats and tracks. diff --git a/app/services/reverse_geocoding/points/fetch_data.rb b/app/services/reverse_geocoding/points/fetch_data.rb index 5253c3153..f4c529aab 100644 --- a/app/services/reverse_geocoding/points/fetch_data.rb +++ b/app/services/reverse_geocoding/points/fetch_data.rb @@ -48,7 +48,7 @@ def update_point_with_geocoding_data rescue *ReverseGeocoding::ProviderErrors::TRANSIENT => e Rails.logger.warn("Reverse geocoding provider error for point #{point.id}: #{e.message}") rescue OpenSSL::SSL::SSLError => e - if e.message.include?('unexpected eof while reading') + if ReverseGeocoding::ProviderErrors.transient_tls?(e) Rails.logger.warn("Reverse geocoding provider error for point #{point.id}: #{e.message}") else Rails.logger.error("Reverse geocoding error for point #{point.id}: #{e.message}") From 93481cf335a8855aa687a1ec3e914664e5dffe18 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Mon, 20 Jul 2026 22:24:00 +0200 Subject: [PATCH 13/31] Stop the legacy lat/lon drop from crash-looping startup --- CHANGELOG.md | 1 + .../drop_legacy_lat_lon_job.rb | 30 +++++++++ ...4090000_drop_legacy_lat_lon_from_points.rb | 47 ++++++++++++-- .../drop_legacy_lat_lon_job_spec.rb | 44 +++++++++++++ .../drop_legacy_lat_lon_from_points_spec.rb | 64 +++++++++++++++++++ 5 files changed, 180 insertions(+), 6 deletions(-) create mode 100644 app/jobs/data_migrations/drop_legacy_lat_lon_job.rb create mode 100644 spec/jobs/data_migrations/drop_legacy_lat_lon_job_spec.rb create mode 100644 spec/migrations/drop_legacy_lat_lon_from_points_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index c2422da32..1b8771b5c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Fixed +- Upgrading to 1.10.1 no longer crash-loops on instances with heavy write traffic. Dropping the legacy `points.latitude`/`points.longitude` columns needs an exclusive lock that busy instances could not win in one attempt, which aborted the migration and restarted the container in a loop. The drop is now retried, and if it still cannot get the lock it is handed to a background job so startup completes (#3176) - Google Semantic History and phone Timeline imports now tag points with a per-import tracker id instead of one shared constant, so tracks from different devices are no longer braided together. A one-time backfill rewrites existing points and regenerates affected tracks per user. - Points at exactly (0,0) — a common GPS glitch — are no longer accepted from any ingestion path (API, OwnTracks, Overland, Traccar, file imports) and no longer produce suggested visits at "Null Island". Existing (0,0) points are flagged as anomalies by a one-time cleanup that also removes visits placed at (0,0) and recalculates affected stats and tracks. diff --git a/app/jobs/data_migrations/drop_legacy_lat_lon_job.rb b/app/jobs/data_migrations/drop_legacy_lat_lon_job.rb new file mode 100644 index 000000000..bfd5debda --- /dev/null +++ b/app/jobs/data_migrations/drop_legacy_lat_lon_job.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +class DataMigrations::DropLegacyLatLonJob < ApplicationJob + queue_as :data_migrations + + LOCK_TIMEOUT = '5s' + + # Losing the lock race is the expected case on a busy instance, so back off + # and try again over the next few hours rather than reporting a failure. + retry_on ActiveRecord::LockWaitTimeout, wait: :polynomially_longer, attempts: 25 + retry_on ActiveRecord::StatementTimeout, wait: :polynomially_longer, attempts: 25 + + def perform + connection = ActiveRecord::Base.connection + return unless legacy_columns?(connection) + + connection.execute("SET lock_timeout = '#{LOCK_TIMEOUT}'") + connection.execute('ALTER TABLE points DROP COLUMN IF EXISTS latitude, DROP COLUMN IF EXISTS longitude') + + Rails.logger.info('[DataMigrations::DropLegacyLatLon] dropped legacy points.latitude / points.longitude') + ensure + connection&.execute('RESET lock_timeout') + end + + private + + def legacy_columns?(connection) + connection.column_exists?(:points, :latitude) || connection.column_exists?(:points, :longitude) + end +end diff --git a/db/migrate/20260714090000_drop_legacy_lat_lon_from_points.rb b/db/migrate/20260714090000_drop_legacy_lat_lon_from_points.rb index 9ee194570..68d623742 100644 --- a/db/migrate/20260714090000_drop_legacy_lat_lon_from_points.rb +++ b/db/migrate/20260714090000_drop_legacy_lat_lon_from_points.rb @@ -4,6 +4,9 @@ class DropLegacyLatLonFromPoints < ActiveRecord::Migration[8.0] disable_ddl_transaction! BATCH_SIZE = 50_000 + DROP_LOCK_TIMEOUT = '5s' + DROP_MAX_ATTEMPTS = 10 + DROP_BACKOFF_SECONDS = 3 def up return unless column_exists?(:points, :latitude) || column_exists?(:points, :longitude) @@ -32,12 +35,44 @@ def up Rails.logger.info "[DropLegacyLatLonFromPoints] backfilled lonlat for #{backfilled} points" end - execute "SET lock_timeout = '5s'" - # Single statement so both columns drop atomically and a rerun never sees - # only one of them missing. - execute 'ALTER TABLE points DROP COLUMN IF EXISTS latitude, DROP COLUMN IF EXISTS longitude' - execute 'RESET lock_timeout' - Rails.logger.info '[DropLegacyLatLonFromPoints] done' + drop_legacy_columns + end + + # The drop needs ACCESS EXCLUSIVE on points. On a live instance the ingestion + # workers write constantly, so a single short attempt loses the race and + # aborted the whole migration, which crash-looped the container: the next boot + # replayed the migration from scratch and lost the race again. + # + # The lock timeout stays short so a waiting drop never queues ahead of writers + # and stalls the app. If every attempt loses, the drop is handed to a + # background job that keeps retrying, so boot completes instead of looping. + def drop_legacy_columns + attempts = 0 + + begin + attempts += 1 + execute "SET lock_timeout = '#{DROP_LOCK_TIMEOUT}'" + # Single statement so both columns drop atomically and a rerun never sees + # only one of them missing. + execute 'ALTER TABLE points DROP COLUMN IF EXISTS latitude, DROP COLUMN IF EXISTS longitude' + Rails.logger.info '[DropLegacyLatLonFromPoints] done' + rescue ActiveRecord::LockWaitTimeout, ActiveRecord::StatementTimeout => e + if attempts < DROP_MAX_ATTEMPTS + Rails.logger.warn( + "[DropLegacyLatLonFromPoints] could not acquire lock (attempt #{attempts}/#{DROP_MAX_ATTEMPTS}): #{e.message}" + ) + sleep(DROP_BACKOFF_SECONDS * attempts) + retry + end + + Rails.logger.warn( + "[DropLegacyLatLonFromPoints] could not acquire lock in #{DROP_MAX_ATTEMPTS} attempts; " \ + 'handing the drop to DataMigrations::DropLegacyLatLonJob' + ) + DataMigrations::DropLegacyLatLonJob.perform_later + ensure + execute 'RESET lock_timeout' + end end def down diff --git a/spec/jobs/data_migrations/drop_legacy_lat_lon_job_spec.rb b/spec/jobs/data_migrations/drop_legacy_lat_lon_job_spec.rb new file mode 100644 index 000000000..78b662f4d --- /dev/null +++ b/spec/jobs/data_migrations/drop_legacy_lat_lon_job_spec.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe DataMigrations::DropLegacyLatLonJob do + let(:connection) { ActiveRecord::Base.connection } + + before { allow(ActiveRecord::Base).to receive(:connection).and_return(connection) } + + it 'does nothing when the legacy columns are already gone' do + allow(connection).to receive(:column_exists?).and_return(false) + allow(connection).to receive(:execute) + + described_class.perform_now + + expect(connection).not_to have_received(:execute).with(/DROP COLUMN/) + end + + it 'drops both legacy columns in a single statement' do + allow(connection).to receive(:column_exists?).and_return(true) + allow(connection).to receive(:execute) + + described_class.perform_now + + expect(connection).to have_received(:execute).with( + 'ALTER TABLE points DROP COLUMN IF EXISTS latitude, DROP COLUMN IF EXISTS longitude' + ) + end + + it 'resets the lock timeout even when the drop loses the lock race' do + allow(connection).to receive(:column_exists?).and_return(true) + allow(connection).to receive(:execute) do |sql| + raise ActiveRecord::LockWaitTimeout, 'lock timeout' if sql.include?('DROP COLUMN') + end + + described_class.perform_now + + expect(connection).to have_received(:execute).with('RESET lock_timeout') + end + + it 'retries rather than failing when the lock is unavailable' do + expect(described_class.rescue_handlers.map(&:first)).to include('ActiveRecord::LockWaitTimeout') + end +end diff --git a/spec/migrations/drop_legacy_lat_lon_from_points_spec.rb b/spec/migrations/drop_legacy_lat_lon_from_points_spec.rb new file mode 100644 index 000000000..96e87dad9 --- /dev/null +++ b/spec/migrations/drop_legacy_lat_lon_from_points_spec.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +require 'rails_helper' +require Rails.root.join('db/migrate/20260714090000_drop_legacy_lat_lon_from_points.rb') + +RSpec.describe DropLegacyLatLonFromPoints, :non_transactional do + subject(:migration) { described_class.new } + + before do + allow(migration).to receive(:sleep) + allow(migration).to receive(:column_exists?).and_return(true) + allow(migration).to receive(:execute).and_call_original + end + + def stub_drop_raising(error_class, times: described_class::DROP_MAX_ATTEMPTS) + attempts = 0 + + allow(migration).to receive(:execute) do |sql| + next nil unless sql.include?('DROP COLUMN') + + attempts += 1 + raise error_class, 'canceling statement due to lock timeout' if attempts <= times + + nil + end + + -> { attempts } + end + + it 'does not abort the migration when the drop never wins the lock race' do + stub_drop_raising(ActiveRecord::LockWaitTimeout) + allow(DataMigrations::DropLegacyLatLonJob).to receive(:perform_later) + + expect { migration.send(:drop_legacy_columns) }.not_to raise_error + end + + it 'hands the drop to a background job once attempts are exhausted' do + stub_drop_raising(ActiveRecord::LockWaitTimeout) + allow(DataMigrations::DropLegacyLatLonJob).to receive(:perform_later) + + migration.send(:drop_legacy_columns) + + expect(DataMigrations::DropLegacyLatLonJob).to have_received(:perform_later) + end + + it 'retries until the lock is acquired instead of failing on the first loss' do + attempts = stub_drop_raising(ActiveRecord::LockWaitTimeout, times: 2) + allow(DataMigrations::DropLegacyLatLonJob).to receive(:perform_later) + + migration.send(:drop_legacy_columns) + + expect(attempts.call).to eq(3) + expect(DataMigrations::DropLegacyLatLonJob).not_to have_received(:perform_later) + end + + it 'always resets the lock timeout it set' do + stub_drop_raising(ActiveRecord::LockWaitTimeout) + allow(DataMigrations::DropLegacyLatLonJob).to receive(:perform_later) + + migration.send(:drop_legacy_columns) + + expect(migration).to have_received(:execute).with('RESET lock_timeout') + end +end From 8416968d1b6b0ab90c8f34f5ed53a086ab9cf916 Mon Sep 17 00:00:00 2001 From: Sai Asish Y Date: Wed, 22 Jul 2026 12:14:51 -0700 Subject: [PATCH 14/31] fix: delegate non-string DNS names to the original resolver Signed-off-by: Sai Asish Y --- config/initializers/dns_cache.rb | 4 ++++ spec/initializers/dns_cache_spec.rb | 13 +++++++++++++ 2 files changed, 17 insertions(+) create mode 100644 spec/initializers/dns_cache_spec.rb diff --git a/config/initializers/dns_cache.rb b/config/initializers/dns_cache.rb index ee5b1de28..d282be4af 100644 --- a/config/initializers/dns_cache.rb +++ b/config/initializers/dns_cache.rb @@ -11,6 +11,10 @@ class << self alias_method :getaddress_without_cache, :getaddress def getaddress(name) + # Let the original resolver raise its usual error for non-string + # names (e.g. nil when SMTP settings are missing) + return getaddress_without_cache(name) unless name.is_a?(String) + # Skip caching for IP addresses (no DNS lookup needed) return getaddress_without_cache(name) if ip_address?(name) diff --git a/spec/initializers/dns_cache_spec.rb b/spec/initializers/dns_cache_spec.rb new file mode 100644 index 000000000..4938ba4d2 --- /dev/null +++ b/spec/initializers/dns_cache_spec.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'DNS cache initializer' do + it 'lets the original resolver raise for nil names' do + expect { Resolv.getaddress(nil) }.to raise_error(ArgumentError, /cannot interpret as DNS name/) + end + + it 'still short-circuits IP address literals' do + expect(Resolv.getaddress('127.0.0.1')).to eq('127.0.0.1') + end +end From f35d7dbeacbef12357bca2075038f68225e249bc Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Sat, 25 Jul 2026 22:09:45 +0200 Subject: [PATCH 15/31] Fix legacy trial email retries Legacy Manager-owned trial lifecycle email types are skipped and logged instead of raising UnknownEmailType, so stale jobs stop retrying forever. Stale ActionMailer::MailDeliveryJob entries that bypass the wrapper are absorbed by no-op mailer actions, and mail addressed to a record that has since been hard-deleted is discarded rather than re-raising ActiveJob::DeserializationError. --- CHANGELOG.md | 1 + app/jobs/users/mailer_sending_job.rb | 14 ++++++++++++ app/mailers/application_mailer.rb | 4 ++++ app/mailers/users_mailer.rb | 8 +++++++ spec/jobs/users/mailer_sending_job_spec.rb | 24 ++++++++++++++++++++ spec/mailers/users_mailer_spec.rb | 26 ++++++++++++++++++++++ 6 files changed, 77 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c2422da32..c54153df2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Fixed +- Trial lifecycle email jobs left over from older releases are now discarded instead of retrying forever in the background queue. Mail addressed to a record that has since been deleted is also discarded rather than retried. - Google Semantic History and phone Timeline imports now tag points with a per-import tracker id instead of one shared constant, so tracks from different devices are no longer braided together. A one-time backfill rewrites existing points and regenerates affected tracks per user. - Points at exactly (0,0) — a common GPS glitch — are no longer accepted from any ingestion path (API, OwnTracks, Overland, Traccar, file imports) and no longer produce suggested visits at "Null Island". Existing (0,0) points are flagged as anomalies by a one-time cleanup that also removes visits placed at (0,0) and recalculates affected stats and tracks. diff --git a/app/jobs/users/mailer_sending_job.rb b/app/jobs/users/mailer_sending_job.rb index 4cff75aa1..d72952a69 100644 --- a/app/jobs/users/mailer_sending_job.rb +++ b/app/jobs/users/mailer_sending_job.rb @@ -13,9 +13,23 @@ class UnknownEmailType < StandardError; end 'account_destroy_confirmation' => ['UsersMailer', :account_destroy_confirmation] }.freeze + LEGACY_MANAGER_EMAIL_TYPES = %w[ + trial_expired + trial_expires_soon + post_trial_reminder_early + post_trial_reminder_late + ].freeze + def perform(user_id, email_type, **options) user = find_user_or_skip(user_id) || return + if LEGACY_MANAGER_EMAIL_TYPES.include?(email_type.to_s) + Rails.logger.info( + "[Users::MailerSendingJob] skipping legacy Manager-owned email_type=#{email_type} user_id=#{user.id}" + ) + return + end + mailer_class_name, action = MAILER_REGISTRY.fetch(email_type.to_s) do raise UnknownEmailType, "Unknown email_type=#{email_type.inspect} user_id=#{user.id}" end diff --git a/app/mailers/application_mailer.rb b/app/mailers/application_mailer.rb index 071c35104..6274e272a 100644 --- a/app/mailers/application_mailer.rb +++ b/app/mailers/application_mailer.rb @@ -3,4 +3,8 @@ class ApplicationMailer < ActionMailer::Base default from: ENV['SMTP_FROM'] layout 'mailer' + + rescue_from ActiveJob::DeserializationError do |exception| + Rails.logger.info("[ApplicationMailer] discarding delivery for a missing record: #{exception.message}") + end end diff --git a/app/mailers/users_mailer.rb b/app/mailers/users_mailer.rb index 03a2e1900..ecba2584c 100644 --- a/app/mailers/users_mailer.rb +++ b/app/mailers/users_mailer.rb @@ -47,4 +47,12 @@ def otp_account_locked mail(to: @user.email, subject: 'Dawarich account temporarily locked') end + + def trial_expired; end + + def trial_expires_soon; end + + def post_trial_reminder_early; end + + def post_trial_reminder_late; end end diff --git a/spec/jobs/users/mailer_sending_job_spec.rb b/spec/jobs/users/mailer_sending_job_spec.rb index cf66304d0..7aa3cde79 100644 --- a/spec/jobs/users/mailer_sending_job_spec.rb +++ b/spec/jobs/users/mailer_sending_job_spec.rb @@ -95,6 +95,30 @@ end end + context 'when email_type is a legacy trial lifecycle email' do + %w[trial_expired trial_expires_soon post_trial_reminder_early post_trial_reminder_late].each do |email_type| + it "skips #{email_type}" do + expect do + described_class.perform_now(user.id, email_type) + end.not_to have_enqueued_job(ActionMailer::MailDeliveryJob) + end + + it "logs that #{email_type} was skipped" do + allow(Rails.logger).to receive(:info) + + described_class.perform_now(user.id, email_type) + + expect(Rails.logger).to have_received(:info).with(/skipping legacy Manager-owned email_type=#{email_type}/) + end + + it "delivers nothing for #{email_type} when a stale ActionMailer job bypasses this wrapper" do + expect do + UsersMailer.with(user: user).public_send(email_type).deliver_now + end.not_to(change { ActionMailer::Base.deliveries.size }) + end + end + end + context 'registry coverage' do # Prove every entry in MAILER_REGISTRY actually resolves to a real mailer # action. A typo in the registry would otherwise silently break production. diff --git a/spec/mailers/users_mailer_spec.rb b/spec/mailers/users_mailer_spec.rb index b3ee4d664..1d76b964c 100644 --- a/spec/mailers/users_mailer_spec.rb +++ b/spec/mailers/users_mailer_spec.rb @@ -51,4 +51,30 @@ expect(mail.text_part.body.encoded).to include('password') end end + + describe 'legacy trial lifecycle emails' do + %i[trial_expired trial_expires_soon post_trial_reminder_early post_trial_reminder_late].each do |action| + it "delivers nothing for a stale #{action} job" do + mail = UsersMailer.with(user: user).public_send(action) + + expect { mail.deliver_now }.not_to(change { ActionMailer::Base.deliveries.size }) + end + end + + it 'delivers nothing for a stale job without user params' do + mail = UsersMailer.with({}).trial_expired + + expect { mail.deliver_now }.not_to(change { ActionMailer::Base.deliveries.size }) + end + + it 'discards a stale delivery job whose user record is gone' do + job = ActionMailer::MailDeliveryJob.new( + 'UsersMailer', 'trial_expired', 'deliver_now', args: [], params: { user: user } + ) + serialized = job.serialize + User.unscoped.where(id: user.id).delete_all + + expect { ActiveJob::Base.execute(serialized) }.not_to raise_error + end + end end From 59439e639773d1d546829faac9cb014ba2367f17 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Sat, 25 Jul 2026 22:25:19 +0200 Subject: [PATCH 16/31] test: cover the DNS cache path and log the fix Adds a regression example for non-String hosts that are not string-like (Integer), and covers the caching behaviour itself: a hostname resolves once and later calls are served from Rails.cache. Also records the fix in the changelog. (#3038) --- CHANGELOG.md | 1 + spec/initializers/dns_cache_spec.rb | 27 +++++++++++++++++++++++---- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 711509009..61a0b8630 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Google Semantic History and phone Timeline imports now tag points with a per-import tracker id instead of one shared constant, so tracks from different devices are no longer braided together. A one-time backfill rewrites existing points and regenerates affected tracks per user. - Points at exactly (0,0) — a common GPS glitch — are no longer accepted from any ingestion path (API, OwnTracks, Overland, Traccar, file imports) and no longer produce suggested visits at "Null Island". Existing (0,0) points are flagged as anomalies by a one-time cleanup that also removes visits placed at (0,0), tolerates legacy points without timestamps, and recalculates affected stats and tracks. - Point uploads from all ingestion paths (REST API, OwnTracks, Overland, Traccar) now retry transient statement and lock-wait timeouts, not just deadlocks, instead of failing the upload. +- The DNS caching layer no longer crashes with a misleading `NoMethodError` when the SMTP server is not configured in the background worker, so email delivery surfaces the real configuration error instead. (#3038) ## [1.10.1] - 2026-07-19, Berlin diff --git a/spec/initializers/dns_cache_spec.rb b/spec/initializers/dns_cache_spec.rb index 4938ba4d2..17c7eac53 100644 --- a/spec/initializers/dns_cache_spec.rb +++ b/spec/initializers/dns_cache_spec.rb @@ -3,11 +3,30 @@ require 'rails_helper' RSpec.describe 'DNS cache initializer' do - it 'lets the original resolver raise for nil names' do - expect { Resolv.getaddress(nil) }.to raise_error(ArgumentError, /cannot interpret as DNS name/) + describe 'non-String hosts' do + it 'lets the original resolver raise for nil names' do + expect { Resolv.getaddress(nil) }.to raise_error(ArgumentError, /cannot interpret as DNS name/) + end + + it 'lets the original resolver raise for names that are not string-like' do + expect { Resolv.getaddress(42) }.to raise_error(TypeError, /no implicit conversion/) + end + end + + describe 'IP address literals' do + it 'returns them without a DNS lookup' do + expect(Resolv.getaddress('127.0.0.1')).to eq('127.0.0.1') + end end - it 'still short-circuits IP address literals' do - expect(Resolv.getaddress('127.0.0.1')).to eq('127.0.0.1') + describe 'hostnames' do + it 'resolves once and serves later calls from the cache' do + allow(Resolv).to receive(:getaddress_without_cache).and_return('203.0.113.10') + + expect(Resolv.getaddress('cache-me.invalid')).to eq('203.0.113.10') + expect(Resolv.getaddress('cache-me.invalid')).to eq('203.0.113.10') + + expect(Resolv).to have_received(:getaddress_without_cache).once + end end end From f0256b5fd59c3e6b4a34cb68b29e2a9510c6019f Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Sat, 25 Jul 2026 22:37:57 +0200 Subject: [PATCH 17/31] Allow saving large-area posters Fixes https://github.com/Freika/dawarich/issues/3204 by aligning the server-side render distance limit with Poster Studio. --- CHANGELOG.md | 1 + app/services/posters/generate.rb | 2 +- spec/services/posters/generate_spec.rb | 20 ++++++++++++++++---- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 711509009..c653224b1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Fixed +- Saving a zoomed-out Poster Studio view to the gallery no longer rejects routes that are visibly inside the poster frame (#3204). - Reverse geocoding and place-name provider outages no longer flood error reporting with handled timeouts, dropped TLS connections, or invalid provider responses. A misconfigured or rate-limited provider — a bad API key, for example — is still reported. - Reverse geocoding retries point updates that time out while waiting on concurrent writes. - Google Semantic History and phone Timeline imports now tag points with a per-import tracker id instead of one shared constant, so tracks from different devices are no longer braided together. A one-time backfill rewrites existing points and regenerates affected tracks per user. diff --git a/app/services/posters/generate.rb b/app/services/posters/generate.rb index 083691935..9d2848c64 100644 --- a/app/services/posters/generate.rb +++ b/app/services/posters/generate.rb @@ -2,7 +2,7 @@ module Posters class Generate - MAX_DISTANCE = 20_000 + MAX_DISTANCE = 5_000_000 MIN_DISTANCE = 500 METERS_PER_DEGREE = 111_320.0 diff --git a/spec/services/posters/generate_spec.rb b/spec/services/posters/generate_spec.rb index 325bf6f8e..bf1fcd9db 100644 --- a/spec/services/posters/generate_spec.rb +++ b/spec/services/posters/generate_spec.rb @@ -62,13 +62,25 @@ def run_generate end end - context 'when the requested distance exceeds the limit' do - let(:poster) { create(:poster, settings: attributes_for(:poster)[:settings].merge('distance' => 150_000)) } + context 'when the requested distance fits within the poster studio range' do + let(:poster) { create(:poster, settings: attributes_for(:poster)[:settings].merge('distance' => 2_924_948)) } before { allow_any_instance_of(Posters::TrackBuilder).to receive(:call).and_return(track) } - it 'clamps the distance to 20km' do - expect(Posters::NativeRenderer).to receive(:new).with(hash_including(distance: 20_000)).and_return(renderer) + it 'renders with the requested distance' do + expect(Posters::NativeRenderer).to receive(:new).with(hash_including(distance: 2_924_948)).and_return(renderer) + + run_generate + end + end + + context 'when the requested distance exceeds the poster studio limit' do + let(:poster) { create(:poster, settings: attributes_for(:poster)[:settings].merge('distance' => 6_000_000)) } + + before { allow_any_instance_of(Posters::TrackBuilder).to receive(:call).and_return(track) } + + it 'clamps the distance to 5,000km' do + expect(Posters::NativeRenderer).to receive(:new).with(hash_including(distance: 5_000_000)).and_return(renderer) run_generate end From a59c5e90f3e00bab92ed7ba0fd13bf1ec12ebd6b Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Sat, 25 Jul 2026 22:37:57 +0200 Subject: [PATCH 18/31] Add custom raster and style basemap URLs - Support raster XYZ and style.json custom basemaps in Map v2 - Fall back to the default style when a custom basemap fails to load - fix: recover from unavailable custom basemaps --- CHANGELOG.md | 2 + app/controllers/api/v1/settings_controller.rb | 15 +- .../maps/maplibre/map_initializer.js | 28 ++++ .../maps/maplibre/settings_manager.js | 65 +++++++-- .../maps_maplibre/utils/basemap_url.js | 33 +++++ .../maps_maplibre/utils/settings_manager.js | 7 +- .../maps_maplibre/utils/style_manager.js | 51 ++++++- .../map/maplibre/_settings_panel.html.erb | 6 +- spec/javascript/basemap_url_classify_test.mjs | 131 ++++++++++++++++++ spec/javascript/map_initializer_test.mjs | 87 ++++++++++++ spec/javascript/settings_manager_test.mjs | 26 +++- spec/requests/api/v1/settings_spec.rb | 43 +++++- 12 files changed, 469 insertions(+), 25 deletions(-) create mode 100644 app/javascript/maps_maplibre/utils/basemap_url.js create mode 100644 spec/javascript/basemap_url_classify_test.mjs create mode 100644 spec/javascript/map_initializer_test.mjs diff --git a/CHANGELOG.md b/CHANGELOG.md index 711509009..98311f6e8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Fixed +- The Map v2 custom basemap field now accepts raster XYZ tiles (`.png`/`.jpg`/`.jpeg`/`.webp`) and full MapLibre style URLs ending in `.json`, in addition to Protomaps-schema vector tiles. Previously a raster URL rendered as a blank grey map and a style URL was rejected. (#3146) + - Reverse geocoding and place-name provider outages no longer flood error reporting with handled timeouts, dropped TLS connections, or invalid provider responses. A misconfigured or rate-limited provider — a bad API key, for example — is still reported. - Reverse geocoding retries point updates that time out while waiting on concurrent writes. - Google Semantic History and phone Timeline imports now tag points with a per-import tracker id instead of one shared constant, so tracks from different devices are no longer braided together. A one-time backfill rewrites existing points and regenerates affected tracks per user. diff --git a/app/controllers/api/v1/settings_controller.rb b/app/controllers/api/v1/settings_controller.rb index 338b6a4a5..c6b2e0dce 100644 --- a/app/controllers/api/v1/settings_controller.rb +++ b/app/controllers/api/v1/settings_controller.rb @@ -18,7 +18,7 @@ def update unless valid_tiles_url?(settings) return render json: { message: 'Something went wrong', - errors: ['Tile URL must include {z}, {x}, and {y} placeholders'] + errors: [TILE_URL_ERROR] }, status: :unprocessable_content end @@ -65,6 +65,8 @@ def recalculation_status_manager MAP_CUSTOMIZATION_KEYS = %i[maps_maplibre_custom_theme maps_maplibre_tiles_url route_color track_color].freeze TILE_URL_PLACEHOLDERS = %w[{z} {x} {y}].freeze + TILE_URL_ERROR = 'Tile URL must include {z}, {x}, and {y} placeholders, ' \ + 'or be a MapLibre style URL ending in .json' def settings_params permitted = params.require(:settings).permit( @@ -119,7 +121,18 @@ def valid_tiles_url?(settings) url = settings[:maps_maplibre_tiles_url] return true if url.nil? return false unless url.is_a?(String) + return true if style_json_url?(url) TILE_URL_PLACEHOLDERS.all? { |placeholder| url.include?(placeholder) } end + + def style_json_url?(url) + uri = URI.parse(url) + return false unless uri.is_a?(URI::HTTP) + return false if uri.host.blank? + + uri.path.to_s.downcase.end_with?('.json') + rescue URI::InvalidURIError + false + end end diff --git a/app/javascript/controllers/maps/maplibre/map_initializer.js b/app/javascript/controllers/maps/maplibre/map_initializer.js index 84b10df39..d5228bbe7 100644 --- a/app/javascript/controllers/maps/maplibre/map_initializer.js +++ b/app/javascript/controllers/maps/maplibre/map_initializer.js @@ -1,4 +1,5 @@ import maplibregl from "maplibre-gl" +import { Toast } from "maps_maplibre/components/toast" import { getMapStyle } from "maps_maplibre/utils/style_manager" /** @@ -41,6 +42,33 @@ export class MapInitializer { const map = new maplibregl.Map(mapOptions) + if (typeof style === "string") { + let settled = false + + const onStyleLoad = () => { + if (settled) return + settled = true + map.off("error", onError) + } + const onError = async () => { + if (settled) return + settled = true + map.off("style.load", onStyleLoad) + Toast.error( + "Custom map style could not be loaded; reverting to the default style.", + ) + const fallbackStyle = await getMapStyle(mapStyle, { + hiddenTileCategories, + disabledPoiGroups, + customTheme, + }) + map.setStyle(fallbackStyle) + } + + map.once("style.load", onStyleLoad) + map.once("error", onError) + } + // Set globe projection after map loads if (globeProjection === true || globeProjection === "true") { map.on("load", () => { diff --git a/app/javascript/controllers/maps/maplibre/settings_manager.js b/app/javascript/controllers/maps/maplibre/settings_manager.js index b952ad6e7..bbf06826e 100644 --- a/app/javascript/controllers/maps/maplibre/settings_manager.js +++ b/app/javascript/controllers/maps/maplibre/settings_manager.js @@ -1116,25 +1116,64 @@ export class SettingsController { async applyMapStyle(styleName) { this.syncStyleDependentToggles(styleName) const style = await getMapStyle(styleName, { - hiddenTileCategories: - SettingsManager.getSetting("hiddenTileCategories") || [], - disabledPoiGroups: SettingsManager.getSetting("disabledPoiGroups") || [], - customTheme: SettingsManager.getSetting("customTheme"), + ...this.mapStyleOptions(), vectorTilesUrl: SettingsManager.getSetting("vectorTilesUrl"), }) // Clear layer references this.layerManager.clearLayerReferences() + if (typeof style === "string") { + this.applyUserStyleUrl(style, styleName) + return + } + this.map.setStyle(style) + this.map.once("style.load", () => this.restoreStyleLayers()) + } - // Reload layers after style change. setStyle replaces the whole style - // document — including the projection — so globe mode must be restored - // or every style/theme change silently drops back to mercator. - this.map.once("style.load", () => { - this.restoreGlobeProjection() - this.controller.loadMapData() - }) + mapStyleOptions() { + return { + hiddenTileCategories: + SettingsManager.getSetting("hiddenTileCategories") || [], + disabledPoiGroups: SettingsManager.getSetting("disabledPoiGroups") || [], + customTheme: SettingsManager.getSetting("customTheme"), + } + } + + // Reload layers after a style change. setStyle replaces the whole style + // document — including the projection — so globe mode must be restored + // or every style/theme change silently drops back to mercator. + restoreStyleLayers() { + this.restoreGlobeProjection() + this.controller.loadMapData() + } + + applyUserStyleUrl(styleUrl, styleName) { + let settled = false + + const onLoad = () => { + if (settled) return + settled = true + this.map.off("error", onError) + this.restoreStyleLayers() + } + + const onError = async () => { + if (settled) return + settled = true + this.map.off("style.load", onLoad) + Toast.error( + "Custom map style could not be loaded; reverting to the default style.", + ) + const fallback = await getMapStyle(styleName, this.mapStyleOptions()) + this.map.setStyle(fallback) + this.map.once("style.load", () => this.restoreStyleLayers()) + } + + this.map.once("style.load", onLoad) + this.map.once("error", onError) + this.map.setStyle(styleUrl) } restoreGlobeProjection() { @@ -1354,7 +1393,9 @@ export class SettingsController { const raw = event.target.value.trim() if (!SettingsManager.validVectorTilesUrl(raw)) { - Toast.error("Tile URL must include {z}, {x}, and {y} placeholders") + Toast.error( + "Tile URL must include {z}, {x}, and {y} placeholders, or be a MapLibre style URL ending in .json", + ) return } diff --git a/app/javascript/maps_maplibre/utils/basemap_url.js b/app/javascript/maps_maplibre/utils/basemap_url.js new file mode 100644 index 000000000..ad9bb5f6a --- /dev/null +++ b/app/javascript/maps_maplibre/utils/basemap_url.js @@ -0,0 +1,33 @@ +/** + * Classify a custom basemap URL into the kind of MapLibre source it describes. + * @param {string} url - Trimmed or untrimmed basemap URL + * @returns {'style'|'raster'|'vector'|null} Classification, or null when unusable + */ +export function classifyBasemapUrl(url) { + if (typeof url !== "string") return null + + const trimmed = url.trim() + if (!trimmed) return null + + const path = trimmed.split(/[?#]/)[0].toLowerCase() + + const hasXyz = + trimmed.includes("{z}") && + trimmed.includes("{x}") && + trimmed.includes("{y}") + + if (path.endsWith(".json") && !hasXyz) return "style" + + if (!hasXyz) return null + + if ( + path.endsWith(".png") || + path.endsWith(".jpg") || + path.endsWith(".jpeg") || + path.endsWith(".webp") + ) { + return "raster" + } + + return "vector" +} diff --git a/app/javascript/maps_maplibre/utils/settings_manager.js b/app/javascript/maps_maplibre/utils/settings_manager.js index a5b6b4c40..b812de2b0 100644 --- a/app/javascript/maps_maplibre/utils/settings_manager.js +++ b/app/javascript/maps_maplibre/utils/settings_manager.js @@ -3,6 +3,8 @@ * Loads settings from backend API only (no localStorage) */ +import { classifyBasemapUrl } from "maps_maplibre/utils/basemap_url" + // Route fallback matches Map v1's blue; track color matches the backend // Tracks::GeojsonSerializer::DEFAULT_COLOR — keep them in sync. export const LAYER_COLOR_DEFAULTS = { @@ -508,10 +510,7 @@ export class SettingsManager { } static validVectorTilesUrl(url) { - return ( - !url || - ["{z}", "{x}", "{y}"].every((placeholder) => url.includes(placeholder)) - ) + return !url || classifyBasemapUrl(url) !== null } /** diff --git a/app/javascript/maps_maplibre/utils/style_manager.js b/app/javascript/maps_maplibre/utils/style_manager.js index daff4ce85..07cc86c98 100644 --- a/app/javascript/maps_maplibre/utils/style_manager.js +++ b/app/javascript/maps_maplibre/utils/style_manager.js @@ -3,11 +3,15 @@ * Loads and configures local map styles with dynamic tile source */ +import { classifyBasemapUrl } from "maps_maplibre/utils/basemap_url" import { resolveTheme } from "poster_studio/data/theme_loader" import { buildBasemapStyle } from "poster_studio/render/style_builder" const TILE_SOURCE_URL = "https://tyles.dwri.xyz/planet/{z}/{x}/{y}.mvt" +const BASEMAP_ATTRIBUTION = + 'Protomaps © OpenStreetMap' + // Cache for loaded styles const styleCache = {} @@ -44,6 +48,37 @@ async function loadStyleFile(styleName) { return style } +/** + * Build a raster basemap style from an XYZ tile URL. + * Reuses the light style's glyphs/sprite so re-added app label layers render. + * @param {string} url - Raster XYZ tile URL + * @returns {Promise} MapLibre style object + */ +async function buildRasterStyle(url) { + const base = await loadStyleFile("light") + return { + version: 8, + glyphs: base.glyphs, + sprite: base.sprite, + sources: { + protomaps: { + type: "raster", + tiles: [url], + tileSize: 256, + attribution: "", + }, + }, + layers: [ + { + id: "background", + type: "background", + paint: { "background-color": "#e0e0e0" }, + }, + { id: "basemap-raster", type: "raster", source: "protomaps" }, + ], + } +} + /** * Map from category keys to the style layer IDs they control. * Shared with map_v2_preview_controller.js — keep in sync. @@ -324,7 +359,18 @@ function hiddenLayerIds(hiddenCategories) { */ export async function getMapStyle(styleName = "light", options = {}) { try { - const tilesUrl = options.vectorTilesUrl || TILE_SOURCE_URL + const customUrl = options.vectorTilesUrl + const basemapType = customUrl ? classifyBasemapUrl(customUrl) : null + + // A full MapLibre style URL replaces the whole document. Hand it to + // setStyle/new Map, which fetch it; app layers are re-added on style.load. + if (basemapType === "style") return customUrl + + // Raster XYZ tiles need their own source and layer — the vendored vector + // layers cannot draw against a raster source. + if (basemapType === "raster") return await buildRasterStyle(customUrl) + + const tilesUrl = customUrl || TILE_SOURCE_URL // Custom themes are built client-side from the user's stored color // tokens (poster-minimal basemap) — no vendored JSON to fetch. Base-map @@ -355,8 +401,7 @@ export async function getMapStyle(styleName = "light", options = {}) { minzoom: 0, maxzoom: 15, attribution: - clonedStyle.sources.protomaps.attribution || - 'Protomaps © OpenStreetMap', + clonedStyle.sources.protomaps.attribution || BASEMAP_ATTRIBUTION, } } diff --git a/app/views/map/maplibre/_settings_panel.html.erb b/app/views/map/maplibre/_settings_panel.html.erb index aa9ad1ea6..4592b1682 100644 --- a/app/views/map/maplibre/_settings_panel.html.erb +++ b/app/views/map/maplibre/_settings_panel.html.erb @@ -571,17 +571,17 @@
- +
-

Serve the base map from your own Protomaps-schema vector tile server. Leave empty for the built-in tiles. Must include {z}, {x}, and {y}.

+

Serve the base map from your own source. Accepts Protomaps-schema vector tiles or raster XYZ tiles (both must include {z}, {x}, and {y}), or a full MapLibre style URL ending in .json. Leave empty for the built-in tiles.

diff --git a/spec/javascript/basemap_url_classify_test.mjs b/spec/javascript/basemap_url_classify_test.mjs new file mode 100644 index 000000000..2bd1c2fb7 --- /dev/null +++ b/spec/javascript/basemap_url_classify_test.mjs @@ -0,0 +1,131 @@ +import assert from "node:assert/strict" +import { readFile } from "node:fs/promises" +import test from "node:test" + +const source = await readFile( + new URL( + "../../app/javascript/maps_maplibre/utils/basemap_url.js", + import.meta.url, + ), + "utf8", +) +const moduleUrl = `data:text/javascript;base64,${Buffer.from(source).toString("base64")}` +const { classifyBasemapUrl } = await import(moduleUrl) + +test("classifies a raster XYZ URL with a jpg extension as raster", () => { + assert.equal( + classifyBasemapUrl( + "https://api.maptiler.com/maps/hybrid/256/{z}/{x}/{y}.jpg?key=abc", + ), + "raster", + ) +}) + +test("classifies png, jpeg, and webp XYZ URLs as raster", () => { + assert.equal( + classifyBasemapUrl("https://t.example/{z}/{x}/{y}.png"), + "raster", + ) + assert.equal( + classifyBasemapUrl("https://t.example/{z}/{x}/{y}.jpeg"), + "raster", + ) + assert.equal( + classifyBasemapUrl("https://t.example/{z}/{x}/{y}.webp"), + "raster", + ) +}) + +test("classifies an XYZ MVT or PBF URL as vector", () => { + assert.equal( + classifyBasemapUrl("https://tiles.example/{z}/{x}/{y}.mvt"), + "vector", + ) + assert.equal( + classifyBasemapUrl("https://tiles.example/{z}/{x}/{y}.pbf"), + "vector", + ) +}) + +test("classifies an extensionless XYZ URL as vector", () => { + assert.equal( + classifyBasemapUrl("https://tiles.example/{z}/{x}/{y}"), + "vector", + ) +}) + +test("classifies a URL whose path ends in json as a full style", () => { + assert.equal( + classifyBasemapUrl("https://api.maptiler.com/maps/streets/style.json"), + "style", + ) +}) + +test("ignores the query string when detecting a style json path", () => { + assert.equal( + classifyBasemapUrl( + "https://api.maptiler.com/maps/streets/style.json?key=abc", + ), + "style", + ) +}) + +test("ignores URL fragments when detecting a style json path", () => { + assert.equal( + classifyBasemapUrl( + "https://api.maptiler.com/maps/streets/style.json#revision", + ), + "style", + ) +}) + +test("classifies a json XYZ tile template as vector, not a style", () => { + assert.equal( + classifyBasemapUrl("https://tiles.example/{z}/{x}/{y}.json"), + "vector", + ) +}) + +test("does not classify a json XYZ tile template with a query string as a style", () => { + assert.equal( + classifyBasemapUrl("https://tiles.example/{z}/{x}/{y}.json?key=abc"), + "vector", + ) +}) + +test("ignores the query string when detecting a raster extension", () => { + assert.equal( + classifyBasemapUrl("https://t.example/{z}/{x}/{y}.png?token=xyz&s=256"), + "raster", + ) +}) + +test("detects extensions case-insensitively", () => { + assert.equal( + classifyBasemapUrl("https://t.example/{z}/{x}/{y}.PNG"), + "raster", + ) + assert.equal(classifyBasemapUrl("https://t.example/STYLE.JSON"), "style") +}) + +test("returns null for a URL with no placeholders and no json extension", () => { + assert.equal(classifyBasemapUrl("https://tiles.example.com/basemap"), null) +}) + +test("returns null for an XYZ URL missing the x and y placeholders", () => { + assert.equal(classifyBasemapUrl("https://tiles.example/{z}.png"), null) +}) + +test("returns null for empty, whitespace, or non-string input", () => { + assert.equal(classifyBasemapUrl(""), null) + assert.equal(classifyBasemapUrl(" "), null) + assert.equal(classifyBasemapUrl(null), null) + assert.equal(classifyBasemapUrl(undefined), null) +}) + +test("trims surrounding whitespace before classifying", () => { + assert.equal( + classifyBasemapUrl(" https://t.example/{z}/{x}/{y}.png "), + "raster", + ) +}) diff --git a/spec/javascript/map_initializer_test.mjs b/spec/javascript/map_initializer_test.mjs new file mode 100644 index 000000000..13797e575 --- /dev/null +++ b/spec/javascript/map_initializer_test.mjs @@ -0,0 +1,87 @@ +import assert from "node:assert/strict"; +import { readFile } from "node:fs/promises"; +import test from "node:test"; + +async function loadMapInitializer({ getMapStyle, Toast }) { + const source = await readFile( + new URL( + "../../app/javascript/controllers/maps/maplibre/map_initializer.js", + import.meta.url, + ), + "utf8", + ); + const withoutImports = source.replace( + /^import[\s\S]*?from "[^"]+";?\n/gm, + "", + ); + const dependencies = ` + const maplibregl = globalThis.__mapInitializerMaplibre + const getMapStyle = globalThis.__mapInitializerGetMapStyle + const Toast = globalThis.__mapInitializerToast + `; + globalThis.__mapInitializerGetMapStyle = getMapStyle; + globalThis.__mapInitializerToast = Toast; + const url = `data:text/javascript;base64,${Buffer.from(`${dependencies}\n${withoutImports}`).toString("base64")}`; + return await import(`${url}#${Date.now()}`); +} + +class FakeMap { + constructor(options) { + this.options = options; + this.listeners = new Map(); + this.setStyles = []; + } + + once(event, callback) { + this.listeners.set(event, callback); + } + + off(event, callback) { + if (this.listeners.get(event) === callback) this.listeners.delete(event); + } + + emit(event) { + const callback = this.listeners.get(event); + this.listeners.delete(event); + callback?.(); + } + + setStyle(style) { + this.setStyles.push(style); + } + + addControl() {} +} + +test("falls back from an unavailable initial custom style URL", async () => { + let map; + const errors = []; + globalThis.__mapInitializerMaplibre = { + Map: class extends FakeMap { + constructor(options) { + super(options) + map = this + queueMicrotask(() => this.emit("error")) + } + }, + NavigationControl: class {}, + AttributionControl: class {}, + }; + const fallback = { version: 8, sources: {}, layers: [] }; + const { MapInitializer } = await loadMapInitializer({ + getMapStyle: async (_styleName, options) => + options.vectorTilesUrl + ? "https://tiles.example/broken-style.json" + : fallback, + Toast: { error: (message) => errors.push(message) }, + }); + + await MapInitializer.initialize( + {}, + { vectorTilesUrl: "https://tiles.example/broken-style.json" }, + ); + await new Promise((resolve) => setImmediate(resolve)); + + assert.deepEqual(map.setStyles, [fallback]); + assert.equal(errors.length, 1); +}); diff --git a/spec/javascript/settings_manager_test.mjs b/spec/javascript/settings_manager_test.mjs index bcdad3689..3fec9b433 100644 --- a/spec/javascript/settings_manager_test.mjs +++ b/spec/javascript/settings_manager_test.mjs @@ -2,6 +2,13 @@ import assert from "node:assert/strict" import { readFile } from "node:fs/promises" import test from "node:test" +const basemapUrlSource = await readFile( + new URL( + "../../app/javascript/maps_maplibre/utils/basemap_url.js", + import.meta.url, + ), + "utf8", +) const source = await readFile( new URL( "../../app/javascript/maps_maplibre/utils/settings_manager.js", @@ -9,7 +16,9 @@ const source = await readFile( ), "utf8", ) -const moduleUrl = `data:text/javascript;base64,${Buffer.from(source).toString("base64")}` +const withoutImports = source.replace(/^import[\s\S]*?from "[^"]+"\n/gm, "") +const combinedSource = `${basemapUrlSource}\n${withoutImports}` +const moduleUrl = `data:text/javascript;base64,${Buffer.from(combinedSource).toString("base64")}` const { LAYER_COLOR_DEFAULTS, SettingsManager } = await import(moduleUrl) async function loadSettingsController(settingsManager) { @@ -55,6 +64,21 @@ test("vector tile URLs require z, x, and y placeholders", () => { assert.equal(SettingsManager.validVectorTilesUrl(""), true) }) +test("basemap URLs also accept raster XYZ and full style.json URLs", () => { + assert.equal( + SettingsManager.validVectorTilesUrl("https://t.example/{z}/{x}/{y}.png"), + true, + ) + assert.equal( + SettingsManager.validVectorTilesUrl("https://t.example/style.json?key=a"), + true, + ) + assert.equal( + SettingsManager.validVectorTilesUrl("https://t.example/basemap"), + false, + ) +}) + test("multiple setting updates are persisted in one complete snapshot", async () => { SettingsManager.cachedSettings = { mapStyle: "light", diff --git a/spec/requests/api/v1/settings_spec.rb b/spec/requests/api/v1/settings_spec.rb index 66cab7798..e24e800bd 100644 --- a/spec/requests/api/v1/settings_spec.rb +++ b/spec/requests/api/v1/settings_spec.rb @@ -123,7 +123,48 @@ params: { settings: { maps_maplibre_tiles_url: 'https://tiles.example.com/{z}.mvt' } } expect(response).to have_http_status(:unprocessable_content) - expect(response.parsed_body['errors']).to include('Tile URL must include {z}, {x}, and {y} placeholders') + expect(user.reload.safe_settings.maps_maplibre_tiles_url).to be_nil + end + + it 'accepts a raster XYZ maps_maplibre_tiles_url' do + patch "/api/v1/settings?api_key=#{api_key}", + params: { + settings: { + maps_maplibre_tiles_url: 'https://api.maptiler.com/maps/hybrid/256/{z}/{x}/{y}.jpg?key=abc' + } + } + + expect(response).to have_http_status(:success) + expect(user.reload.safe_settings.maps_maplibre_tiles_url) + .to eq('https://api.maptiler.com/maps/hybrid/256/{z}/{x}/{y}.jpg?key=abc') + end + + it 'accepts a full style URL ending in .json' do + patch "/api/v1/settings?api_key=#{api_key}", + params: { + settings: { + maps_maplibre_tiles_url: 'https://api.maptiler.com/maps/streets/style.json?key=abc' + } + } + + expect(response).to have_http_status(:success) + expect(user.reload.safe_settings.maps_maplibre_tiles_url) + .to eq('https://api.maptiler.com/maps/streets/style.json?key=abc') + end + + it 'rejects a non-http style.json URL' do + patch "/api/v1/settings?api_key=#{api_key}", + params: { settings: { maps_maplibre_tiles_url: 'ftp://example.com/style.json' } } + + expect(response).to have_http_status(:unprocessable_content) + expect(user.reload.safe_settings.maps_maplibre_tiles_url).to be_nil + end + + it 'rejects a maps_maplibre_tiles_url that is neither an XYZ tile URL nor a style.json' do + patch "/api/v1/settings?api_key=#{api_key}", + params: { settings: { maps_maplibre_tiles_url: 'https://tiles.example.com/basemap' } } + + expect(response).to have_http_status(:unprocessable_content) expect(user.reload.safe_settings.maps_maplibre_tiles_url).to be_nil end From 877e74e748b148cbb292da17c6522feca2ba2123 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Sat, 25 Jul 2026 23:03:45 +0200 Subject: [PATCH 19/31] Rescue QueryAborted and survive an unreachable queue PostgreSQL raises QueryCanceled, not StatementTimeout, when statement_timeout fires, so the drop still aborted the migration on instances that set one. Rescue QueryAborted, which covers both. Wrap the job hand-off so an unreachable Redis logs and lets startup finish instead of re-raising into the crash loop this change removes. Flatten the job backoff to 5 minutes; polynomially_longer spanned days at the tail rather than the intended few hours. --- CHANGELOG.md | 2 +- .../data_migrations/drop_legacy_lat_lon_job.rb | 6 ++++-- ...0714090000_drop_legacy_lat_lon_from_points.rb | 16 ++++++++++++++-- .../drop_legacy_lat_lon_job_spec.rb | 16 +++++++++++++++- .../drop_legacy_lat_lon_from_points_spec.rb | 15 +++++++++++++++ 5 files changed, 49 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1b8771b5c..e6b0e1885 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Fixed -- Upgrading to 1.10.1 no longer crash-loops on instances with heavy write traffic. Dropping the legacy `points.latitude`/`points.longitude` columns needs an exclusive lock that busy instances could not win in one attempt, which aborted the migration and restarted the container in a loop. The drop is now retried, and if it still cannot get the lock it is handed to a background job so startup completes (#3176) +- Instances with heavy write traffic no longer crash-loop on the 1.10.1 upgrade. Dropping the legacy `points.latitude`/`points.longitude` columns needs an exclusive lock that busy instances could not win in one attempt, which aborted the migration and restarted the container in a loop. The drop is now retried, and if it still cannot get the lock it is handed to a background job so startup completes (#3176) - Google Semantic History and phone Timeline imports now tag points with a per-import tracker id instead of one shared constant, so tracks from different devices are no longer braided together. A one-time backfill rewrites existing points and regenerates affected tracks per user. - Points at exactly (0,0) — a common GPS glitch — are no longer accepted from any ingestion path (API, OwnTracks, Overland, Traccar, file imports) and no longer produce suggested visits at "Null Island". Existing (0,0) points are flagged as anomalies by a one-time cleanup that also removes visits placed at (0,0) and recalculates affected stats and tracks. diff --git a/app/jobs/data_migrations/drop_legacy_lat_lon_job.rb b/app/jobs/data_migrations/drop_legacy_lat_lon_job.rb index bfd5debda..2be9a5d0e 100644 --- a/app/jobs/data_migrations/drop_legacy_lat_lon_job.rb +++ b/app/jobs/data_migrations/drop_legacy_lat_lon_job.rb @@ -7,8 +7,10 @@ class DataMigrations::DropLegacyLatLonJob < ApplicationJob # Losing the lock race is the expected case on a busy instance, so back off # and try again over the next few hours rather than reporting a failure. - retry_on ActiveRecord::LockWaitTimeout, wait: :polynomially_longer, attempts: 25 - retry_on ActiveRecord::StatementTimeout, wait: :polynomially_longer, attempts: 25 + # QueryAborted covers both LockWaitTimeout's sibling StatementTimeout and the + # QueryCanceled that PostgreSQL raises when statement_timeout fires. + retry_on ActiveRecord::LockWaitTimeout, wait: 5.minutes, attempts: 25 + retry_on ActiveRecord::QueryAborted, wait: 5.minutes, attempts: 25 def perform connection = ActiveRecord::Base.connection diff --git a/db/migrate/20260714090000_drop_legacy_lat_lon_from_points.rb b/db/migrate/20260714090000_drop_legacy_lat_lon_from_points.rb index 68d623742..bd45fb5cb 100644 --- a/db/migrate/20260714090000_drop_legacy_lat_lon_from_points.rb +++ b/db/migrate/20260714090000_drop_legacy_lat_lon_from_points.rb @@ -56,7 +56,7 @@ def drop_legacy_columns # only one of them missing. execute 'ALTER TABLE points DROP COLUMN IF EXISTS latitude, DROP COLUMN IF EXISTS longitude' Rails.logger.info '[DropLegacyLatLonFromPoints] done' - rescue ActiveRecord::LockWaitTimeout, ActiveRecord::StatementTimeout => e + rescue ActiveRecord::LockWaitTimeout, ActiveRecord::QueryAborted => e if attempts < DROP_MAX_ATTEMPTS Rails.logger.warn( "[DropLegacyLatLonFromPoints] could not acquire lock (attempt #{attempts}/#{DROP_MAX_ATTEMPTS}): #{e.message}" @@ -69,12 +69,24 @@ def drop_legacy_columns "[DropLegacyLatLonFromPoints] could not acquire lock in #{DROP_MAX_ATTEMPTS} attempts; " \ 'handing the drop to DataMigrations::DropLegacyLatLonJob' ) - DataMigrations::DropLegacyLatLonJob.perform_later + enqueue_drop_job ensure execute 'RESET lock_timeout' end end + # Redis may not be reachable yet when migrations run, and an unreachable queue + # must not abort the migration — that is the crash loop this change removes. + # The columns are unused, so leaving them in place is safe. + def enqueue_drop_job + DataMigrations::DropLegacyLatLonJob.perform_later + rescue StandardError => e + Rails.logger.warn( + "[DropLegacyLatLonFromPoints] could not enqueue DataMigrations::DropLegacyLatLonJob: #{e.message}; " \ + 'the legacy columns remain and will be dropped on a later boot' + ) + end + def down execute 'ALTER TABLE points ADD COLUMN IF NOT EXISTS latitude numeric(10,6), ' \ 'ADD COLUMN IF NOT EXISTS longitude numeric(10,6)' diff --git a/spec/jobs/data_migrations/drop_legacy_lat_lon_job_spec.rb b/spec/jobs/data_migrations/drop_legacy_lat_lon_job_spec.rb index 78b662f4d..5781c7c2d 100644 --- a/spec/jobs/data_migrations/drop_legacy_lat_lon_job_spec.rb +++ b/spec/jobs/data_migrations/drop_legacy_lat_lon_job_spec.rb @@ -39,6 +39,20 @@ end it 'retries rather than failing when the lock is unavailable' do - expect(described_class.rescue_handlers.map(&:first)).to include('ActiveRecord::LockWaitTimeout') + allow(connection).to receive(:column_exists?).and_return(true) + allow(connection).to receive(:execute) do |sql| + raise ActiveRecord::LockWaitTimeout, 'lock timeout' if sql.include?('DROP COLUMN') + end + + expect { described_class.perform_now }.to have_enqueued_job(described_class) + end + + it 'retries when a statement_timeout cancels the drop' do + allow(connection).to receive(:column_exists?).and_return(true) + allow(connection).to receive(:execute) do |sql| + raise ActiveRecord::QueryCanceled, 'statement timeout' if sql.include?('DROP COLUMN') + end + + expect { described_class.perform_now }.to have_enqueued_job(described_class) end end diff --git a/spec/migrations/drop_legacy_lat_lon_from_points_spec.rb b/spec/migrations/drop_legacy_lat_lon_from_points_spec.rb index 96e87dad9..716a9957a 100644 --- a/spec/migrations/drop_legacy_lat_lon_from_points_spec.rb +++ b/spec/migrations/drop_legacy_lat_lon_from_points_spec.rb @@ -61,4 +61,19 @@ def stub_drop_raising(error_class, times: described_class::DROP_MAX_ATTEMPTS) expect(migration).to have_received(:execute).with('RESET lock_timeout') end + + it 'hands off rather than aborting when a statement_timeout cancels the drop' do + stub_drop_raising(ActiveRecord::QueryCanceled) + + expect { migration.send(:drop_legacy_columns) }.to have_enqueued_job(DataMigrations::DropLegacyLatLonJob) + end + + it 'does not abort the migration when the job cannot be enqueued' do + stub_drop_raising(ActiveRecord::LockWaitTimeout) + allow(DataMigrations::DropLegacyLatLonJob).to receive(:perform_later).and_raise( + RedisClient::CannotConnectError, 'connection refused' + ) + + expect { migration.send(:drop_legacy_columns) }.not_to raise_error + end end From 5c5a424d95f640db4de7673b7fa134b59d9d3093 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Sat, 25 Jul 2026 23:26:15 +0200 Subject: [PATCH 20/31] Bound the drop by lock_timeout alone and retry quietly A pooled connection can carry a session-level statement_timeout set by another job, which cancels the drop no matter how the lock race goes. Clear it for the duration of the drop and reset it afterwards. Capping retries re-raised into Sidekiq's own retry chain, turning a quiet wait into weeks of reported failures ending in the dead set. Narrow the enqueue rescue to connection failures so a NameError or a serialization bug surfaces instead of silently stranding the columns. Correct the comment claiming a waiting drop never queues ahead of writers: it does, and the short lock timeout only bounds the stall. --- app/jobs/data_migrations/drop_legacy_lat_lon_job.rb | 12 +++++++++--- ...20260714090000_drop_legacy_lat_lon_from_points.rb | 12 +++++++----- .../data_migrations/drop_legacy_lat_lon_job_spec.rb | 10 ++++++++++ 3 files changed, 26 insertions(+), 8 deletions(-) diff --git a/app/jobs/data_migrations/drop_legacy_lat_lon_job.rb b/app/jobs/data_migrations/drop_legacy_lat_lon_job.rb index 2be9a5d0e..9cef28207 100644 --- a/app/jobs/data_migrations/drop_legacy_lat_lon_job.rb +++ b/app/jobs/data_migrations/drop_legacy_lat_lon_job.rb @@ -6,22 +6,28 @@ class DataMigrations::DropLegacyLatLonJob < ApplicationJob LOCK_TIMEOUT = '5s' # Losing the lock race is the expected case on a busy instance, so back off - # and try again over the next few hours rather than reporting a failure. + # and keep trying rather than reporting a failure. Attempts are unlimited + # because exhausting them re-raises into Sidekiq's own retries, turning a + # quiet wait into weeks of reported failures ending in the dead set. # QueryAborted covers both LockWaitTimeout's sibling StatementTimeout and the # QueryCanceled that PostgreSQL raises when statement_timeout fires. - retry_on ActiveRecord::LockWaitTimeout, wait: 5.minutes, attempts: 25 - retry_on ActiveRecord::QueryAborted, wait: 5.minutes, attempts: 25 + retry_on ActiveRecord::LockWaitTimeout, wait: 5.minutes, attempts: :unlimited + retry_on ActiveRecord::QueryAborted, wait: 5.minutes, attempts: :unlimited def perform connection = ActiveRecord::Base.connection return unless legacy_columns?(connection) + # A pooled connection can carry a statement_timeout set by another job; the + # drop must be bounded by lock_timeout alone or every retry is cancelled. + connection.execute('SET statement_timeout = 0') connection.execute("SET lock_timeout = '#{LOCK_TIMEOUT}'") connection.execute('ALTER TABLE points DROP COLUMN IF EXISTS latitude, DROP COLUMN IF EXISTS longitude') Rails.logger.info('[DataMigrations::DropLegacyLatLon] dropped legacy points.latitude / points.longitude') ensure connection&.execute('RESET lock_timeout') + connection&.execute('RESET statement_timeout') end private diff --git a/db/migrate/20260714090000_drop_legacy_lat_lon_from_points.rb b/db/migrate/20260714090000_drop_legacy_lat_lon_from_points.rb index bd45fb5cb..91da83cb0 100644 --- a/db/migrate/20260714090000_drop_legacy_lat_lon_from_points.rb +++ b/db/migrate/20260714090000_drop_legacy_lat_lon_from_points.rb @@ -43,9 +43,10 @@ def up # aborted the whole migration, which crash-looped the container: the next boot # replayed the migration from scratch and lost the race again. # - # The lock timeout stays short so a waiting drop never queues ahead of writers - # and stalls the app. If every attempt loses, the drop is handed to a - # background job that keeps retrying, so boot completes instead of looping. + # A queued ACCESS EXCLUSIVE request does block the writers behind it, so the + # lock timeout stays short to bound each stall to DROP_LOCK_TIMEOUT. If every + # attempt loses, the drop is handed to a background job that keeps retrying, + # so boot completes instead of looping. def drop_legacy_columns attempts = 0 @@ -77,10 +78,11 @@ def drop_legacy_columns # Redis may not be reachable yet when migrations run, and an unreachable queue # must not abort the migration — that is the crash loop this change removes. - # The columns are unused, so leaving them in place is safe. + # The columns are unused, so leaving them in place is safe. Only connection + # failures are swallowed; anything else is a bug worth surfacing. def enqueue_drop_job DataMigrations::DropLegacyLatLonJob.perform_later - rescue StandardError => e + rescue RedisClient::Error, SocketError, IOError, SystemCallError => e Rails.logger.warn( "[DropLegacyLatLonFromPoints] could not enqueue DataMigrations::DropLegacyLatLonJob: #{e.message}; " \ 'the legacy columns remain and will be dropped on a later boot' diff --git a/spec/jobs/data_migrations/drop_legacy_lat_lon_job_spec.rb b/spec/jobs/data_migrations/drop_legacy_lat_lon_job_spec.rb index 5781c7c2d..f1322e300 100644 --- a/spec/jobs/data_migrations/drop_legacy_lat_lon_job_spec.rb +++ b/spec/jobs/data_migrations/drop_legacy_lat_lon_job_spec.rb @@ -27,6 +27,16 @@ ) end + it 'clears any statement timeout inherited from the pooled connection' do + allow(connection).to receive(:column_exists?).and_return(true) + allow(connection).to receive(:execute) + + described_class.perform_now + + expect(connection).to have_received(:execute).with('SET statement_timeout = 0') + expect(connection).to have_received(:execute).with('RESET statement_timeout') + end + it 'resets the lock timeout even when the drop loses the lock race' do allow(connection).to receive(:column_exists?).and_return(true) allow(connection).to receive(:execute) do |sql| From 8a07b53fd79c61c7236ec18894f66786335cb89c Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Mon, 27 Jul 2026 20:31:20 +0200 Subject: [PATCH 21/31] Bind the drop timeouts to the transaction that runs it A bare SET and the following ALTER can land on different backends under PgBouncer transaction pooling, leaving the drop with no timeout and an unbounded ACCESS EXCLUSIVE request queued ahead of every points read and write. SET LOCAL inside an explicit transaction keeps both timeouts on the backend that runs the ALTER, matching Visits::StayPointDetector. Scoping the timeouts this way also removes both ensure blocks: a raise in RESET lock_timeout used to mask the LockWaitTimeout that retry_on needs to see, and a no-op run issued two RESETs for timeouts it never set. Cap the job's retries again and log once on exhaustion. Unlimited attempts stalled points writes every five minutes with no retry set, no dead set and no log line to find. Widen the enqueue rescue back to StandardError. A malformed REDIS_URL or an exhausted pool raises outside the connection-error families, and no enqueue failure is worth restarting the container for. --- .../drop_legacy_lat_lon_job.rb | 45 +++++++++++++------ ...4090000_drop_legacy_lat_lon_from_points.rb | 32 +++++++------ .../drop_legacy_lat_lon_job_spec.rb | 15 +++---- .../drop_legacy_lat_lon_from_points_spec.rb | 8 ++-- 4 files changed, 60 insertions(+), 40 deletions(-) diff --git a/app/jobs/data_migrations/drop_legacy_lat_lon_job.rb b/app/jobs/data_migrations/drop_legacy_lat_lon_job.rb index 9cef28207..3db23f8f7 100644 --- a/app/jobs/data_migrations/drop_legacy_lat_lon_job.rb +++ b/app/jobs/data_migrations/drop_legacy_lat_lon_job.rb @@ -5,29 +5,46 @@ class DataMigrations::DropLegacyLatLonJob < ApplicationJob LOCK_TIMEOUT = '5s' - # Losing the lock race is the expected case on a busy instance, so back off - # and keep trying rather than reporting a failure. Attempts are unlimited - # because exhausting them re-raises into Sidekiq's own retries, turning a - # quiet wait into weeks of reported failures ending in the dead set. + MAX_ATTEMPTS = 288 + + # Losing the lock race is the expected case on a busy instance, so back off and + # try again over the next day rather than reporting a failure. Attempts are + # capped: each one stalls points writes for LOCK_TIMEOUT, so a drop that can + # never win must stop and say so instead of retrying invisibly forever. # QueryAborted covers both LockWaitTimeout's sibling StatementTimeout and the # QueryCanceled that PostgreSQL raises when statement_timeout fires. - retry_on ActiveRecord::LockWaitTimeout, wait: 5.minutes, attempts: :unlimited - retry_on ActiveRecord::QueryAborted, wait: 5.minutes, attempts: :unlimited + retry_on ActiveRecord::LockWaitTimeout, wait: 5.minutes, attempts: MAX_ATTEMPTS do |_job, error| + log_exhaustion(error) + end + + retry_on ActiveRecord::QueryAborted, wait: 5.minutes, attempts: MAX_ATTEMPTS do |_job, error| + log_exhaustion(error) + end + + def self.log_exhaustion(error) + Rails.logger.error( + "[DataMigrations::DropLegacyLatLon] gave up after #{MAX_ATTEMPTS} attempts (#{error.class}: #{error.message}); " \ + 'points.latitude / points.longitude are still present and must be dropped manually' + ) + end def perform connection = ActiveRecord::Base.connection return unless legacy_columns?(connection) - # A pooled connection can carry a statement_timeout set by another job; the - # drop must be bounded by lock_timeout alone or every retry is cancelled. - connection.execute('SET statement_timeout = 0') - connection.execute("SET lock_timeout = '#{LOCK_TIMEOUT}'") - connection.execute('ALTER TABLE points DROP COLUMN IF EXISTS latitude, DROP COLUMN IF EXISTS longitude') + # SET LOCAL binds both timeouts to this transaction's backend so they survive + # PgBouncer transaction pooling — a bare SET + ALTER can otherwise land on + # different servers, leaving the drop with no timeout at all and an unbounded + # ACCESS EXCLUSIVE request queued ahead of every points read and write. + # statement_timeout is pinned off so only lock_timeout bounds the wait; the + # drop itself is metadata-only once the lock is held. + connection.transaction do + connection.execute('SET LOCAL statement_timeout = 0') + connection.execute("SET LOCAL lock_timeout = '#{LOCK_TIMEOUT}'") + connection.execute('ALTER TABLE points DROP COLUMN IF EXISTS latitude, DROP COLUMN IF EXISTS longitude') + end Rails.logger.info('[DataMigrations::DropLegacyLatLon] dropped legacy points.latitude / points.longitude') - ensure - connection&.execute('RESET lock_timeout') - connection&.execute('RESET statement_timeout') end private diff --git a/db/migrate/20260714090000_drop_legacy_lat_lon_from_points.rb b/db/migrate/20260714090000_drop_legacy_lat_lon_from_points.rb index 91da83cb0..7a0d8dc21 100644 --- a/db/migrate/20260714090000_drop_legacy_lat_lon_from_points.rb +++ b/db/migrate/20260714090000_drop_legacy_lat_lon_from_points.rb @@ -52,10 +52,15 @@ def drop_legacy_columns begin attempts += 1 - execute "SET lock_timeout = '#{DROP_LOCK_TIMEOUT}'" - # Single statement so both columns drop atomically and a rerun never sees - # only one of them missing. - execute 'ALTER TABLE points DROP COLUMN IF EXISTS latitude, DROP COLUMN IF EXISTS longitude' + # SET LOCAL keeps both timeouts on the same backend as the ALTER under + # PgBouncer transaction pooling; a bare SET can land elsewhere and leave + # the drop unbounded. Single ALTER statement so both columns drop + # atomically and a rerun never sees only one of them missing. + transaction do + execute 'SET LOCAL statement_timeout = 0' + execute "SET LOCAL lock_timeout = '#{DROP_LOCK_TIMEOUT}'" + execute 'ALTER TABLE points DROP COLUMN IF EXISTS latitude, DROP COLUMN IF EXISTS longitude' + end Rails.logger.info '[DropLegacyLatLonFromPoints] done' rescue ActiveRecord::LockWaitTimeout, ActiveRecord::QueryAborted => e if attempts < DROP_MAX_ATTEMPTS @@ -71,21 +76,20 @@ def drop_legacy_columns 'handing the drop to DataMigrations::DropLegacyLatLonJob' ) enqueue_drop_job - ensure - execute 'RESET lock_timeout' end end - # Redis may not be reachable yet when migrations run, and an unreachable queue - # must not abort the migration — that is the crash loop this change removes. - # The columns are unused, so leaving them in place is safe. Only connection - # failures are swallowed; anything else is a bug worth surfacing. + # Redis may not be reachable yet when migrations run, and no enqueue failure + # may abort the migration — that is the crash loop this change removes. The + # rescue is deliberately broad: a malformed REDIS_URL, an exhausted pool and a + # refused connection all reach here, and none of them are worth a restart loop. + # The columns are unused, so leaving them in place is safe. def enqueue_drop_job DataMigrations::DropLegacyLatLonJob.perform_later - rescue RedisClient::Error, SocketError, IOError, SystemCallError => e - Rails.logger.warn( - "[DropLegacyLatLonFromPoints] could not enqueue DataMigrations::DropLegacyLatLonJob: #{e.message}; " \ - 'the legacy columns remain and will be dropped on a later boot' + rescue StandardError => e + Rails.logger.error( + '[DropLegacyLatLonFromPoints] could not enqueue DataMigrations::DropLegacyLatLonJob ' \ + "(#{e.class}: #{e.message}); the legacy columns remain and will be dropped on a later boot" ) end diff --git a/spec/jobs/data_migrations/drop_legacy_lat_lon_job_spec.rb b/spec/jobs/data_migrations/drop_legacy_lat_lon_job_spec.rb index f1322e300..6503981c9 100644 --- a/spec/jobs/data_migrations/drop_legacy_lat_lon_job_spec.rb +++ b/spec/jobs/data_migrations/drop_legacy_lat_lon_job_spec.rb @@ -27,25 +27,24 @@ ) end - it 'clears any statement timeout inherited from the pooled connection' do + it 'scopes both timeouts to the transaction so pooling cannot separate them' do allow(connection).to receive(:column_exists?).and_return(true) allow(connection).to receive(:execute) described_class.perform_now - expect(connection).to have_received(:execute).with('SET statement_timeout = 0') - expect(connection).to have_received(:execute).with('RESET statement_timeout') + expect(connection).to have_received(:execute).with('SET LOCAL statement_timeout = 0') + expect(connection).to have_received(:execute).with("SET LOCAL lock_timeout = '5s'") end - it 'resets the lock timeout even when the drop loses the lock race' do + it 'runs the drop inside a transaction' do allow(connection).to receive(:column_exists?).and_return(true) - allow(connection).to receive(:execute) do |sql| - raise ActiveRecord::LockWaitTimeout, 'lock timeout' if sql.include?('DROP COLUMN') - end + allow(connection).to receive(:execute) + allow(connection).to receive(:transaction).and_call_original described_class.perform_now - expect(connection).to have_received(:execute).with('RESET lock_timeout') + expect(connection).to have_received(:transaction) end it 'retries rather than failing when the lock is unavailable' do diff --git a/spec/migrations/drop_legacy_lat_lon_from_points_spec.rb b/spec/migrations/drop_legacy_lat_lon_from_points_spec.rb index 716a9957a..93227c896 100644 --- a/spec/migrations/drop_legacy_lat_lon_from_points_spec.rb +++ b/spec/migrations/drop_legacy_lat_lon_from_points_spec.rb @@ -53,13 +53,13 @@ def stub_drop_raising(error_class, times: described_class::DROP_MAX_ATTEMPTS) expect(DataMigrations::DropLegacyLatLonJob).not_to have_received(:perform_later) end - it 'always resets the lock timeout it set' do - stub_drop_raising(ActiveRecord::LockWaitTimeout) - allow(DataMigrations::DropLegacyLatLonJob).to receive(:perform_later) + it 'scopes both timeouts to the transaction so pooling cannot separate them' do + stub_drop_raising(ActiveRecord::LockWaitTimeout, times: 0) migration.send(:drop_legacy_columns) - expect(migration).to have_received(:execute).with('RESET lock_timeout') + expect(migration).to have_received(:execute).with('SET LOCAL statement_timeout = 0') + expect(migration).to have_received(:execute).with("SET LOCAL lock_timeout = '5s'") end it 'hands off rather than aborting when a statement_timeout cancels the drop' do From d7bc5257993eac06091509887aefd086d5a0de20 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Mon, 27 Jul 2026 20:31:50 +0200 Subject: [PATCH 22/31] fix: make visit suggestion consistent and predictable Five bugs in the visit-suggestion subsystem, plus review follow-ups. Visits::RealtimeDebouncer#clear was defined but never called, so the `nx: true` guard never released for a continuously-tracking user and realtime detection fired exactly once. VisitSuggestingJob now releases the key, guarded so a Redis blip can't drop the run under retry: false. The realtime lookback drops to 6h: clusters matching an existing visit never claim their points, so a 25h window re-detected and re-geocoded them on every run. Visits::TimeChunks discarded end_at whenever start and end shared a year, so the nightly job scanned from yesterday to 31 December. Three specs asserted that in their own titles; they were characterization tests, now rewritten. Visits::Suggest interpolated a backtrace into a user-facing notification and returned ExceptionReporter's value instead of an array. ExceptionReporter no-ops when self-hosted, so the backtrace now goes to Rails.logger unconditionally, and repeat notifications are gated by a Redis SET NX claim that fails open. Visits::Merger updated end_time and points but left duration, centre, radius and suggested_name at their pre-merge values, so Creator wrote the wrong duration and PlaceFinder resolved the first sub-cluster's place. The centre is recomputed on every absorption because can_merge_visits? compares against it; the rest is recomputed once when a chain closes, keeping the name if the geocoder lookup fails. Reverse geocoding overwrote user-chosen place names nightly, via FetchData#update_place, FetchData#populate_place_attributes (upsert_all, so callbacks cannot guard it) and Places::NameFetcher. A new places.name_locked_at, set on rename and on user-driven creation, protects them; renaming a place back to "Suggested place" hands it back to automatic naming. The lock state is exposed through both place serializers and surfaced in the Map v2 info panel and the drawer. --- CHANGELOG.md | 5 + app/controllers/api/v1/places_controller.rb | 2 + app/controllers/places_controller.rb | 1 + .../controllers/maps/maplibre/data_loader.js | 1 + .../maps/maplibre/event_handlers.js | 5 + app/jobs/visit_suggesting_job.rb | 9 ++ app/models/place.rb | 17 +++ app/serializers/api/place_serializer.rb | 3 +- app/services/places/name_fetcher.rb | 8 +- .../reverse_geocoding/places/fetch_data.rb | 11 +- app/services/visits/create.rb | 3 +- app/services/visits/merger.rb | 28 ++++- app/services/visits/realtime_debouncer.rb | 5 +- app/services/visits/select_place.rb | 4 +- app/services/visits/suggest.rb | 37 +++++- app/services/visits/time_chunks.rb | 5 +- app/views/places/_drawer.html.erb | 9 +- ...0727120000_add_name_locked_at_to_places.rb | 9 ++ db/schema.rb | 3 +- spec/jobs/visit_suggesting_job_spec.rb | 29 +++++ spec/models/place_spec.rb | 56 +++++++++ spec/requests/api/v1/places_spec.rb | 19 +++ spec/requests/places_spec.rb | 50 ++++++++ spec/services/places/name_fetcher_spec.rb | 30 +++++ .../places/fetch_data_spec.rb | 44 +++++++ spec/services/visits/merger_spec.rb | 115 ++++++++++++++++++ spec/services/visits/select_place_spec.rb | 15 +++ spec/services/visits/suggest_spec.rb | 66 ++++++++++ spec/services/visits/time_chunks_spec.rb | 33 +++-- 29 files changed, 588 insertions(+), 34 deletions(-) create mode 100644 db/migrate/20260727120000_add_name_locked_at_to_places.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index c2422da32..256242a9f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,11 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Google Semantic History and phone Timeline imports now tag points with a per-import tracker id instead of one shared constant, so tracks from different devices are no longer braided together. A one-time backfill rewrites existing points and regenerates affected tracks per user. - Points at exactly (0,0) — a common GPS glitch — are no longer accepted from any ingestion path (API, OwnTracks, Overland, Traccar, file imports) and no longer produce suggested visits at "Null Island". Existing (0,0) points are flagged as anomalies by a one-time cleanup that also removes visits placed at (0,0) and recalculates affected stats and tracks. +- Place names you set yourself are no longer overwritten by nightly reverse geocoding. Renaming a place, creating one by hand, or picking one on the timeline locks its name; renaming it back to "Suggested place" hands it back to auto-naming. +- Real-time visit detection no longer stops after the first run for users who track continuously — the debounce key is now released when the job runs. +- The nightly visit suggestion job no longer scans forward to the end of the calendar year; it processes only the day it was asked for. +- Merged visits now report the correct duration, centre, radius and suggested name instead of keeping the values of the first cluster in the merge. +- Visit suggestion failures no longer show a raw stack trace in your notifications, and repeated failures within an hour no longer create a notification each time. ## [1.10.1] - 2026-07-19, Berlin diff --git a/app/controllers/api/v1/places_controller.rb b/app/controllers/api/v1/places_controller.rb index 7bfed8ba2..5e203b065 100644 --- a/app/controllers/api/v1/places_controller.rb +++ b/app/controllers/api/v1/places_controller.rb @@ -69,6 +69,7 @@ def show def create @place = current_api_user.places.build(place_params.except(:tag_ids)) + @place.user_named = true if @place.save add_tags if tag_ids.present? @@ -180,6 +181,7 @@ def serialize_place(place) icon: place.tags.first&.icon, color: place.tags.first&.color, visits_count: place.visits.size, + name_locked: place.name_locked?, created_at: place.created_at, tags: place.tags.map do |tag| { diff --git a/app/controllers/places_controller.rb b/app/controllers/places_controller.rb index bde245df8..9e4cea79d 100644 --- a/app/controllers/places_controller.rb +++ b/app/controllers/places_controller.rb @@ -19,6 +19,7 @@ def show def create @place = current_user.places.build(place_params.except(:tag_ids)) + @place.user_named = true if @place.save add_tags if tag_ids.present? diff --git a/app/javascript/controllers/maps/maplibre/data_loader.js b/app/javascript/controllers/maps/maplibre/data_loader.js index 49c0a89e2..6ef3c69f9 100644 --- a/app/javascript/controllers/maps/maplibre/data_loader.js +++ b/app/javascript/controllers/maps/maplibre/data_loader.js @@ -505,6 +505,7 @@ export class DataLoader { latitude: place.latitude, longitude: place.longitude, note: place.note, + nameLocked: Boolean(place.name_locked), // Stringify tags for MapLibre GL JS compatibility tags: JSON.stringify(place.tags || []), // Use first tag's color if available diff --git a/app/javascript/controllers/maps/maplibre/event_handlers.js b/app/javascript/controllers/maps/maplibre/event_handlers.js index 0c7987981..17dad4bf4 100644 --- a/app/javascript/controllers/maps/maplibre/event_handlers.js +++ b/app/javascript/controllers/maps/maplibre/event_handlers.js @@ -191,6 +191,11 @@ export class EventHandlers {
${properties.tag ? `
${escapeHtml(properties.tag)}
` : ""} ${properties.description ? `
${escapeHtml(properties.description)}
` : ""} + ${ + properties.nameLocked + ? `
🔒 You named this place, so automatic naming won't change it. Rename it to "Suggested place" to hand it back.
` + : "" + }
` diff --git a/app/jobs/visit_suggesting_job.rb b/app/jobs/visit_suggesting_job.rb index 918de4577..6e05ed900 100644 --- a/app/jobs/visit_suggesting_job.rb +++ b/app/jobs/visit_suggesting_job.rb @@ -8,6 +8,8 @@ class VisitSuggestingJob < ApplicationJob # Passing timespan of more than 3 years somehow results in duplicated Places def perform(user_id:, start_at:, end_at:) + release_debounce_key(user_id) + user = find_user_or_skip(user_id) || return return unless user.safe_settings.visits_suggestions_enabled? @@ -28,6 +30,13 @@ def perform(user_id:, start_at:, end_at:) private + # retry: false means a transient Redis blip here would otherwise drop the whole run. + def release_debounce_key(user_id) + Visits::RealtimeDebouncer.new(user_id).clear + rescue StandardError => e + Rails.logger.warn("[VisitSuggestingJob] debounce key release failed user_id=#{user_id}: #{e.class}: #{e.message}") + end + def parse_date(date) date.is_a?(String) ? Time.zone.parse(date) : date.to_datetime end diff --git a/app/models/place.rb b/app/models/place.rb index bf24a9c1e..b618dc85d 100644 --- a/app/models/place.rb +++ b/app/models/place.rb @@ -14,7 +14,10 @@ class Place < ApplicationRecord has_many :place_visits, dependent: :destroy has_many :suggested_visits, -> { distinct }, through: :place_visits, source: :visit + attr_accessor :machine_named, :user_named + before_validation :build_lonlat, if: -> { latitude.present? && longitude.present? } + before_save :lock_name_on_user_edit validates :name, presence: true, length: { maximum: 255 } validates :lonlat, presence: true @@ -39,6 +42,10 @@ def lat lonlat.y end + def name_locked? + name_locked_at.present? + end + def osm_id geodata.dig('properties', 'osm_id') end @@ -60,4 +67,14 @@ def osm_type def build_lonlat self.lonlat = "POINT(#{longitude} #{latitude})" end + + def lock_name_on_user_edit + return if machine_named + return unless will_save_change_to_name? + + return self.name_locked_at = nil if name == DEFAULT_NAME + return if new_record? && !user_named + + self.name_locked_at = Time.current + end end diff --git a/app/serializers/api/place_serializer.rb b/app/serializers/api/place_serializer.rb index ae91bbdb4..fa6ae6bfc 100644 --- a/app/serializers/api/place_serializer.rb +++ b/app/serializers/api/place_serializer.rb @@ -17,7 +17,8 @@ def call geodata: place.geodata, created_at: place.created_at, updated_at: place.updated_at, - reverse_geocoded_at: place.reverse_geocoded_at + reverse_geocoded_at: place.reverse_geocoded_at, + name_locked: place.name_locked? } end diff --git a/app/services/places/name_fetcher.rb b/app/services/places/name_fetcher.rb index f7e827e38..924f4b46d 100644 --- a/app/services/places/name_fetcher.rb +++ b/app/services/places/name_fetcher.rb @@ -33,12 +33,16 @@ def call name = ::Visits::Names::Builder.build_from_properties(properties) ActiveRecord::Base.transaction do - place.name = name if name.present? + place.machine_named = true + place.name = name if name.present? && !place.name_locked? place.city = properties['city'] if properties['city'].present? place.country = properties['country'] if properties['country'].present? place.geodata = result.data if DawarichSettings.store_geodata? place.save! - place.visits.where(name: Place::DEFAULT_NAME).update_all(name: name) if name.present? + + propagated_name = place.name + place.visits.where(name: Place::DEFAULT_NAME).update_all(name: propagated_name) if propagated_name.present? + place end rescue StandardError => e diff --git a/app/services/reverse_geocoding/places/fetch_data.rb b/app/services/reverse_geocoding/places/fetch_data.rb index 788722e53..1e92bc215 100644 --- a/app/services/reverse_geocoding/places/fetch_data.rb +++ b/app/services/reverse_geocoding/places/fetch_data.rb @@ -36,15 +36,18 @@ def update_place(reverse_geocoded_place) data = normalize_geocoder_data(reverse_geocoded_place.data) - place.update!( - name: place_name(data), + attributes = { lonlat: build_point_coordinates(data['geometry']['coordinates']), city: data['properties']['city'], country: data['properties']['country'], geodata: data, source: Place.sources[:photon], reverse_geocoded_at: Time.current - ) + } + attributes[:name] = place_name(data) unless place.name_locked? + + place.machine_named = true + place.update!(attributes) end def find_place(place_data, existing_places) @@ -107,7 +110,7 @@ def prepare_places_for_bulk_operations(places, existing_places) end def populate_place_attributes(place, data) - place.name = place_name(data) + place.name = place_name(data) unless place.name_locked? place.city = data['properties']['city'] place.country = data['properties']['country'] place.geodata = data diff --git a/app/services/visits/create.rb b/app/services/visits/create.rb index 34c06f38a..e6020eaf7 100644 --- a/app/services/visits/create.rb +++ b/app/services/visits/create.rb @@ -64,7 +64,8 @@ def create_new_place latitude: lat_f, longitude: lon_f, lonlat: "POINT(#{lon_f} #{lat_f})", - source: :manual + source: :manual, + user_named: true ) rescue ActiveRecord::RecordInvalid => e ExceptionReporter.call(e, "Failed to create place: #{e.message}") diff --git a/app/services/visits/merger.rb b/app/services/visits/merger.rb index e13bd2186..680091c0c 100644 --- a/app/services/visits/merger.rb +++ b/app/services/visits/merger.rb @@ -3,6 +3,8 @@ module Visits # Merges consecutive visits that are likely part of the same stay class Merger + include Visits::DetectionHelpers + MAXIMUM_VISIT_GAP = 30.minutes SIGNIFICANT_MOVEMENT_THRESHOLD = 50 # meters @@ -17,24 +19,48 @@ def merge_visits(visits) merged = [] current_merged = visits.first + absorbed = false visits[1..].each do |visit| if can_merge_visits?(current_merged, visit) - # Merge the visits current_merged[:end_time] = visit[:end_time] current_merged[:points].concat(visit[:points]) + recalculate_center(current_merged) + absorbed = true else + finalize(current_merged) if absorbed merged << current_merged current_merged = visit + absorbed = false end end + finalize(current_merged) if absorbed merged << current_merged merged end private + # Runs on every absorption because can_merge_visits? compares against the running centre. + def recalculate_center(visit) + center = calculate_weighted_center(visit[:points]) + + visit[:center_lat] = center[0] + visit[:center_lon] = center[1] + end + + # Runs once when a merge chain closes: radius and the name lookup are expensive + # and only the final values are ever consumed. + def finalize(visit) + center = [visit[:center_lat], visit[:center_lon]] + + visit[:duration] = visit[:end_time] - visit[:start_time] + visit[:radius] = calculate_visit_radius(visit[:points], center) + visit[:suggested_name] = + suggest_place_name(visit[:points]) || fetch_place_name(center) || visit[:suggested_name] + end + def can_merge_visits?(first_visit, second_visit) return false unless same_location?(first_visit, second_visit) return false if gap_too_large?(first_visit, second_visit) diff --git a/app/services/visits/realtime_debouncer.rb b/app/services/visits/realtime_debouncer.rb index d35e6a354..1819dbe1d 100644 --- a/app/services/visits/realtime_debouncer.rb +++ b/app/services/visits/realtime_debouncer.rb @@ -3,7 +3,10 @@ class Visits::RealtimeDebouncer DEBOUNCE_DELAY = 5.minutes REDIS_KEY_TTL = 10.minutes - LOOKBACK_WINDOW = 25.hours + # Clusters that match an existing visit never claim their points, so every run + # re-detects and re-names them. Now that the key is released each run, keep the + # window tight — BulkVisitsSuggestingJob still re-scans the whole previous day. + LOOKBACK_WINDOW = 6.hours def initialize(user_id) @user_id = user_id diff --git a/app/services/visits/select_place.rb b/app/services/visits/select_place.rb index 314c40abd..526320098 100644 --- a/app/services/visits/select_place.rb +++ b/app/services/visits/select_place.rb @@ -15,6 +15,7 @@ def initialize(user:, visit:, photon:) def call with_dedup_lock do place = find_by_name_and_proximity || create_place + place.update!(name_locked_at: Time.current) unless place.name_locked? @visit.update!(place_id: place.id, name: place.name) place end @@ -56,7 +57,8 @@ def create_place city: @photon[:city], country: @photon[:country], geodata: DawarichSettings.store_geodata? ? (@photon[:geodata] || {}) : {}, - source: :photon + source: :photon, + user_named: true ) end end diff --git a/app/services/visits/suggest.rb b/app/services/visits/suggest.rb index 18a28d354..c0f246bbb 100644 --- a/app/services/visits/suggest.rb +++ b/app/services/visits/suggest.rb @@ -22,18 +22,45 @@ def call visits rescue StandardError => e - # create a notification with stacktrace and what arguments were used - user.notifications.create!( - kind: :error, - title: 'Error suggesting visits', - content: "Error suggesting visits: #{e.message}\n#{e.backtrace.join("\n")}" + Rails.logger.error( + "[Visits::Suggest] user_id=#{user.id} range=#{start_at}..#{end_at} " \ + "#{e.class}: #{e.message}\n#{e.backtrace&.join("\n")}" ) + notify_failure(e) ExceptionReporter.call(e) + + [] end private + ERROR_TITLE = 'Error suggesting visits' + ERROR_DEDUP_WINDOW = 1.hour + + # The debouncer can schedule a run every five minutes; without this an outage + # would bury the notification list under hundreds of identical rows. The claim + # is a Redis SET NX so two concurrent runs can't both pass the check, and it + # fails open — a Redis problem must never swallow the error notification. + def notify_failure(error) + return unless claim_error_window? + + user.notifications.create!( + kind: :error, + title: ERROR_TITLE, + content: "Error suggesting visits: #{error.message}" + ) + end + + def claim_error_window? + Sidekiq.redis do |redis| + redis.set("visit_suggest_error:user:#{user.id}", 1, nx: true, ex: ERROR_DEDUP_WINDOW.to_i) + end + rescue StandardError => e + Rails.logger.warn("[Visits::Suggest] error-notification dedupe unavailable: #{e.class}: #{e.message}") + true + end + def create_visits_notification(user) content = <<~CONTENT New visits have been suggested based on your location data from #{Time.zone.at(start_at)} to #{Time.zone.at(end_at)}. You can review them on the Timeline page. diff --git a/app/services/visits/time_chunks.rb b/app/services/visits/time_chunks.rb index 5c7470da5..5e7dd0ba5 100644 --- a/app/services/visits/time_chunks.rb +++ b/app/services/visits/time_chunks.rb @@ -9,10 +9,7 @@ def initialize(start_at:, end_at:) end def call - # If the start date is in the future or equal to the end date, - # handle as a special case extending to the end of the start's year - # or if the start and end are in the same year, return the year chunk - return [start_at..start_at.end_of_year] if start_in_future? || same_year? + return [start_at..end_at] if start_in_future? || same_year? # First chunk: from start_at to end of that year first_end = start_at.end_of_year diff --git a/app/views/places/_drawer.html.erb b/app/views/places/_drawer.html.erb index 4cb7c3d54..247b132b5 100644 --- a/app/views/places/_drawer.html.erb +++ b/app/views/places/_drawer.html.erb @@ -11,7 +11,14 @@ <% end %>
-

<%= place.name %>

+

+ <%= place.name %> + <% if place.name_locked? %> + <%= icon 'lock', class: 'w-4 h-4 inline' %> + <% end %> +

<% if stats[:location_line].present? %>

<%= stats[:location_line] %>

<% end %> diff --git a/db/migrate/20260727120000_add_name_locked_at_to_places.rb b/db/migrate/20260727120000_add_name_locked_at_to_places.rb new file mode 100644 index 000000000..2eb84d9b9 --- /dev/null +++ b/db/migrate/20260727120000_add_name_locked_at_to_places.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AddNameLockedAtToPlaces < ActiveRecord::Migration[8.0] + def change + return if column_exists?(:places, :name_locked_at) + + add_column :places, :name_locked_at, :datetime + end +end diff --git a/db/schema.rb b/db/schema.rb index 972c8dfcb..001a4ef70 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.0].define(version: 2026_07_19_190000) do +ActiveRecord::Schema[8.0].define(version: 2026_07_27_120000) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" enable_extension "pgcrypto" @@ -310,6 +310,7 @@ t.bigint "user_id" t.text "note" t.boolean "demo", default: false, null: false + t.datetime "name_locked_at" t.index "(((geodata -> 'properties'::text) ->> 'osm_id'::text))", name: "index_places_on_geodata_osm_id" t.index ["demo"], name: "index_places_on_demo_true", where: "(demo = true)" t.index ["lonlat"], name: "index_places_on_lonlat", using: :gist diff --git a/spec/jobs/visit_suggesting_job_spec.rb b/spec/jobs/visit_suggesting_job_spec.rb index de9173fe7..979e1037d 100644 --- a/spec/jobs/visit_suggesting_job_spec.rb +++ b/spec/jobs/visit_suggesting_job_spec.rb @@ -117,6 +117,35 @@ end end + describe 'realtime debounce key' do + let(:redis_key) { "visit_realtime:user:#{user.id}" } + + before do + Sidekiq.redis { |redis| redis.del(redis_key) } + allow(DawarichSettings).to receive(:reverse_geocoding_enabled?).and_return(true) + end + + after { Sidekiq.redis { |redis| redis.del(redis_key) } } + + it 'still runs when Redis is unavailable' do + allow_any_instance_of(Visits::RealtimeDebouncer) + .to receive(:clear).and_raise(RedisClient::CannotConnectError, 'redis down') + + expect(Visits::Suggest).to receive(:new).at_least(:once).and_call_original + + described_class.perform_now(user_id: user.id, start_at: start_at, end_at: end_at) + end + + it 'releases the key so the debouncer can schedule the next run' do + Visits::RealtimeDebouncer.new(user.id).trigger + + described_class.perform_now(user_id: user.id, start_at: start_at, end_at: end_at) + + expect { Visits::RealtimeDebouncer.new(user.id).trigger } + .to have_enqueued_job(VisitSuggestingJob).with(hash_including(user_id: user.id)) + end + end + describe 'queue name' do it 'uses the visit_suggesting queue' do expect(described_class.queue_name).to eq('visit_suggesting') diff --git a/spec/models/place_spec.rb b/spec/models/place_spec.rb index a76996483..2389bf8ef 100644 --- a/spec/models/place_spec.rb +++ b/spec/models/place_spec.rb @@ -165,4 +165,60 @@ end end end + + describe 'name locking' do + let(:place) { create(:place, name: Place::DEFAULT_NAME) } + + it 'is unlocked when created' do + expect(place.name_locked?).to be(false) + end + + it 'locks the name when it is renamed' do + expect { place.update!(name: "Mum's house") } + .to change { place.reload.name_locked? }.from(false).to(true) + end + + it 'does not lock when another attribute changes' do + place.update!(city: 'Leipzig') + + expect(place.reload.name_locked?).to be(false) + end + + it 'does not lock a machine-named place' do + machine_place = build(:place, name: 'Photon Suggestion') + machine_place.machine_named = true + machine_place.save! + + expect(machine_place.reload.name_locked?).to be(false) + end + + it 'does not lock a place minted by detection' do + expect(create(:place, name: 'Photon Suggestion').name_locked?).to be(false) + end + + it 'locks a place created with a user-supplied name' do + user_place = build(:place, name: "Mum's house") + user_place.user_named = true + user_place.save! + + expect(user_place.reload.name_locked?).to be(true) + end + + it 'clears the lock when the name is reset to the default' do + place.update!(name: "Mum's house") + + expect { place.update!(name: Place::DEFAULT_NAME) } + .to change { place.reload.name_locked? }.from(true).to(false) + end + + it 'keeps an existing lock when a machine write touches other attributes' do + place.update!(name: "Mum's house") + locked_at = place.reload.name_locked_at + + place.machine_named = true + place.update!(city: 'Leipzig') + + expect(place.reload.name_locked_at).to be_within(1.second).of(locked_at) + end + end end diff --git a/spec/requests/api/v1/places_spec.rb b/spec/requests/api/v1/places_spec.rb index 3d9f1bcc5..2fd1c8d07 100644 --- a/spec/requests/api/v1/places_spec.rb +++ b/spec/requests/api/v1/places_spec.rb @@ -297,4 +297,23 @@ expect(response).to have_http_status(:unauthorized) end end + + describe 'name lock exposure' do + let(:user) { create(:user) } + let(:place) { create(:place, user: user, name: Place::DEFAULT_NAME) } + + it 'reports the lock state so Map v2 can surface it' do + place.update!(name: "Mum's house") + + get api_v1_place_path(place), headers: { 'Authorization' => "Bearer #{user.api_key}" } + + expect(response.parsed_body['name_locked']).to be(true) + end + + it 'reports an auto-named place as unlocked' do + get api_v1_place_path(place), headers: { 'Authorization' => "Bearer #{user.api_key}" } + + expect(response.parsed_body['name_locked']).to be(false) + end + end end diff --git a/spec/requests/places_spec.rb b/spec/requests/places_spec.rb index 184e6cd9a..9ee41f748 100644 --- a/spec/requests/places_spec.rb +++ b/spec/requests/places_spec.rb @@ -344,4 +344,54 @@ expect_turbo_stream_action('replace', 'place-drawer') end end + + describe 'name locking' do + let(:user) { create(:user) } + + before { sign_in user } + + it 'locks the name of a place the user creates by hand' do + post places_path, params: { place: { name: "Mum's house", latitude: 51.3402, longitude: 12.3712 } }, + as: :turbo_stream + + expect(user.places.last).to be_name_locked + end + + it 'shows a lock indicator on the drawer for a locked place' do + place = create(:place, user: user, name: Place::DEFAULT_NAME) + place.update!(name: "Mum's house") + + get place_path(place) + + expect(response.body).to include('place-name-lock') + end + + it 'shows the lock in the drawer response right after a rename' do + place = create(:place, user: user, name: Place::DEFAULT_NAME) + + patch place_path(place), params: { place: { name: "Mum's house" } }, + headers: { 'Turbo-Frame' => 'place-drawer' }, as: :turbo_stream + + expect(response.body).to include('place-name-lock') + end + + it 'drops the lock from the drawer when the name is reset to the default' do + place = create(:place, user: user, name: Place::DEFAULT_NAME) + place.update!(name: "Mum's house") + + patch place_path(place), params: { place: { name: Place::DEFAULT_NAME } }, + headers: { 'Turbo-Frame' => 'place-drawer' }, as: :turbo_stream + + expect(place.reload).not_to be_name_locked + expect(response.body).not_to include('place-name-lock') + end + + it 'shows no lock indicator for an auto-named place' do + place = create(:place, user: user, name: Place::DEFAULT_NAME) + + get place_path(place) + + expect(response.body).not_to include('place-name-lock') + end + end end diff --git a/spec/services/places/name_fetcher_spec.rb b/spec/services/places/name_fetcher_spec.rb index b1be08b1d..7dc473905 100644 --- a/spec/services/places/name_fetcher_spec.rb +++ b/spec/services/places/name_fetcher_spec.rb @@ -68,6 +68,36 @@ service.call end + context 'when the name is locked by the user' do + let(:place) do + create( + :place, + name: "Mum's house", + name_locked_at: 1.day.ago, + city: nil, + country: nil, + geodata: {}, + lonlat: 'POINT(10.0 10.0)' + ) + end + + it 'keeps the user-supplied name' do + expect { service.call }.not_to change(place, :name) + end + + it 'still refreshes city and country' do + expect { service.call }.to change(place, :city).from(nil).to('New York') + end + + it 'propagates the locked name to visits still using the default name' do + visit = create(:visit, place: place, user: place.user, name: Place::DEFAULT_NAME) + + service.call + + expect(visit.reload.name).to eq("Mum's house") + end + end + context 'when DawarichSettings.store_geodata? is enabled' do before do allow(DawarichSettings).to receive(:store_geodata?).and_return(true) diff --git a/spec/services/reverse_geocoding/places/fetch_data_spec.rb b/spec/services/reverse_geocoding/places/fetch_data_spec.rb index b44d114f6..943132df4 100644 --- a/spec/services/reverse_geocoding/places/fetch_data_spec.rb +++ b/spec/services/reverse_geocoding/places/fetch_data_spec.rb @@ -51,6 +51,50 @@ .and change { place.reload.country }.to('Germany') end + context 'when the place name is locked by the user' do + let(:place) { create(:place, name: "Mum's house", name_locked_at: 1.day.ago) } + + it 'keeps the user-supplied name' do + expect { service.call }.not_to change { place.reload.name } + end + + it 'still refreshes city and country' do + expect { service.call }.to change { place.reload.city }.to('Berlin') + end + end + + context 'when a sibling place has a locked name' do + let(:sibling) do + create(:place, user: place.user, name: 'Sibling I named', name_locked_at: 1.day.ago, + geodata: { 'properties' => { 'osm_id' => 99_999 } }) + end + + let(:sibling_geocoded_place) do + double( + data: { + 'geometry' => { 'coordinates' => [13.0948638, 54.2905245] }, + 'properties' => { + 'osm_id' => 99_999, 'name' => 'Photon Override', 'osm_value' => 'cafe', + 'city' => 'Hamburg', 'country' => 'Germany' + } + } + ) + end + + before do + sibling + allow(Geocoder).to receive(:search).and_return([mock_geocoded_place, sibling_geocoded_place]) + end + + it 'keeps the locked sibling name through the bulk upsert' do + expect { service.call }.not_to change { sibling.reload.name } + end + + it 'still refreshes the locked sibling city' do + expect { service.call }.to change { sibling.reload.city }.to('Hamburg') + end + end + it 'sets reverse_geocoded_at timestamp' do expect { service.call }.to change { place.reload.reverse_geocoded_at } .from(nil) diff --git a/spec/services/visits/merger_spec.rb b/spec/services/visits/merger_spec.rb index 8bb00ec92..bdeaf985b 100644 --- a/spec/services/visits/merger_spec.rb +++ b/spec/services/visits/merger_spec.rb @@ -105,6 +105,121 @@ end end + context 'when consecutive visits are merged' do + let(:base_time) { Time.zone.local(2024, 5, 1, 10, 0, 0).to_i } + let(:points) { user.points.order(timestamp: :asc) } + + let(:geodata) do + { 'type' => 'Feature', 'properties' => { 'type' => 'street', 'name' => 'Nikolaistrasse' } } + end + + let!(:point_a) do + create(:point, user: user, timestamp: base_time, geodata: geodata, + latitude: 51.3402, longitude: 12.3712, accuracy: 10) + end + + let!(:point_b) do + create(:point, user: user, timestamp: base_time + 1200, geodata: geodata, + latitude: 51.3404, longitude: 12.3712, accuracy: 10) + end + + let(:visit_a) do + { + start_time: base_time, + end_time: base_time + 600, + duration: 600, + center_lat: 51.3402, + center_lon: 12.3712, + radius: 500, + suggested_name: 'Stale first-cluster name', + points: [point_a] + } + end + + let(:visit_b) do + { + start_time: base_time + 1200, + end_time: base_time + 1800, + duration: 600, + center_lat: 51.3404, + center_lon: 12.3712, + radius: 500, + suggested_name: 'Second cluster name', + points: [point_b] + } + end + + subject(:merger) { described_class.new(points) } + + it 'merges the pair into one visit' do + expect(merger.merge_visits([visit_a, visit_b]).size).to eq(1) + end + + it 'recomputes duration to span the merged range' do + merged = merger.merge_visits([visit_a, visit_b]).first + + expect(merged[:duration]).to eq(merged[:end_time] - merged[:start_time]) + expect(merged[:duration]).to eq(1800) + end + + it 'recomputes the centre from every merged point' do + merged = merger.merge_visits([visit_a, visit_b]).first + + expect(merged[:center_lat]).to be_within(0.00001).of(51.3403) + expect(merged[:center_lon]).to be_within(0.00001).of(12.3712) + end + + it 'recomputes the radius against the new centre' do + merged = merger.merge_visits([visit_a, visit_b]).first + + expect(merged[:radius]).to eq(15) + end + + it 'recomputes the suggested name from every merged point' do + merged = merger.merge_visits([visit_a, visit_b]).first + + expect(merged[:suggested_name]).to be_present + expect(merged[:suggested_name]).not_to eq('Stale first-cluster name') + end + + it 'keeps the pre-merge name when the geocoder lookup fails' do + point_a.update!(geodata: {}) + point_b.update!(geodata: {}) + allow(Geocoder).to receive(:search).and_return([]) + + merged = merger.merge_visits([visit_a, visit_b]).first + + expect(merged[:suggested_name]).to eq('Stale first-cluster name') + end + + it 'finalizes once across a three-visit chain' do + point_c = create(:point, user: user, timestamp: base_time + 2400, geodata: geodata, + latitude: 51.3406, longitude: 12.3712, accuracy: 10) + visit_c = { + start_time: base_time + 2400, end_time: base_time + 3000, duration: 600, + center_lat: 51.3406, center_lon: 12.3712, radius: 500, + suggested_name: 'Third cluster name', points: [point_c] + } + + result = merger.merge_visits([visit_a, visit_b, visit_c]) + + expect(result.size).to eq(1) + expect(result.first[:points].size).to eq(3) + expect(result.first[:duration]).to eq(3000) + expect(result.first[:center_lat]).to be_within(0.00001).of(51.3404) + end + + it 'leaves an unmerged visit untouched' do + far_visit = visit_b.merge(center_lat: 51.9, start_time: base_time + 99_999, + end_time: base_time + 100_599) + + result = merger.merge_visits([visit_a, far_visit]) + + expect(result.last[:suggested_name]).to eq('Second cluster name') + expect(result.last[:radius]).to eq(500) + end + end + context 'with empty visits array' do let(:points) { user.points.order(timestamp: :asc) } diff --git a/spec/services/visits/select_place_spec.rb b/spec/services/visits/select_place_spec.rb index 1af85d0fc..1d2ddcf9c 100644 --- a/spec/services/visits/select_place_spec.rb +++ b/spec/services/visits/select_place_spec.rb @@ -37,6 +37,21 @@ expect(visit.name).to eq('Café Bravo') end + it 'locks the name of a place the user picked so reverse geocoding cannot rewrite it' do + place = described_class.new(user: user, visit: visit, photon: photon_payload).call + + expect(place.reload).to be_name_locked + end + + it 'locks the name of an existing place the user picked' do + existing = create(:place, user: user, name: photon_payload[:name], + latitude: photon_payload[:latitude], longitude: photon_payload[:longitude]) + + described_class.new(user: user, visit: visit, photon: photon_payload.except(:osm_id, :geodata)).call + + expect(existing.reload).to be_name_locked + end + it 'does not match another user\'s place at the same name and coords (isolation)' do other = create(:user) create(:place, user: other, name: 'Café Bravo', latitude: 52.5126, longitude: 13.4012) diff --git a/spec/services/visits/suggest_spec.rb b/spec/services/visits/suggest_spec.rb index 2939c2cbc..bcd5c7b15 100644 --- a/spec/services/visits/suggest_spec.rb +++ b/spec/services/visits/suggest_spec.rb @@ -110,6 +110,72 @@ def data end end + context 'when detection raises' do + before do + allow(Visits::SmartDetect).to receive(:new).and_raise(StandardError, 'detector exploded') + Sidekiq.redis { |redis| redis.del("visit_suggest_error:user:#{user.id}") } + end + + after do + Sidekiq.redis { |redis| redis.del("visit_suggest_error:user:#{user.id}") } + rescue StandardError + nil + end + + it 'returns an empty collection' do + expect(described_class.new(user, start_at:, end_at:).call).to eq([]) + end + + it 'logs the failure with a backtrace for self-hosted operators' do + allow(Rails.logger).to receive(:error) + + described_class.new(user, start_at:, end_at:).call + + expect(Rails.logger).to have_received(:error).with(/detector exploded/) + end + + it 'does not repeat the notification while an unread one is recent' do + 3.times { described_class.new(user, start_at:, end_at:).call } + + expect(user.notifications.where(title: 'Error suggesting visits').count).to eq(1) + end + + it 'notifies again once the dedup window has passed' do + described_class.new(user, start_at:, end_at:).call + Sidekiq.redis { |redis| redis.del("visit_suggest_error:user:#{user.id}") } + + described_class.new(user, start_at:, end_at:).call + + expect(user.notifications.where(title: 'Error suggesting visits').count).to eq(2) + end + + it 'suppresses the notification when another run already claimed the window' do + Sidekiq.redis { |redis| redis.set("visit_suggest_error:user:#{user.id}", 1, ex: 3600) } + + described_class.new(user, start_at:, end_at:).call + + expect(user.notifications.where(title: 'Error suggesting visits')).to be_empty + end + + it 'still notifies when Redis is unavailable' do + allow(Sidekiq).to receive(:redis).and_raise(RedisClient::CannotConnectError, 'redis down') + + described_class.new(user, start_at:, end_at:).call + + expect(user.notifications.where(title: 'Error suggesting visits').count).to eq(1) + end + + it 'notifies the user without leaking a backtrace' do + described_class.new(user, start_at:, end_at:).call + + notification = user.notifications.order(:id).last + + expect(notification.kind).to eq('error') + expect(notification.content).to include('detector exploded') + expect(notification.content).not_to match(/\.rb:\d+/) + end + end + # The Lite plan window is enforced inside `Visits::SmartDetect` (which is # what `Visits::Suggest#call` delegates to). The corresponding regression # test lives in spec/services/visits/smart_detect_spec.rb. diff --git a/spec/services/visits/time_chunks_spec.rb b/spec/services/visits/time_chunks_spec.rb index 4a5b7f9f2..f1b1f8c8b 100644 --- a/spec/services/visits/time_chunks_spec.rb +++ b/spec/services/visits/time_chunks_spec.rb @@ -38,8 +38,21 @@ end end + context 'with the nightly single-day range' do + it 'ends the chunk at end_at rather than the end of the calendar year' do + start_at = DateTime.new(2026, 1, 2).beginning_of_day + end_at = DateTime.new(2026, 1, 2).end_of_day + + chunks = described_class.new(start_at: start_at, end_at: end_at).call + + expect(chunks.size).to eq(1) + expect(chunks[0].first).to eq(start_at) + expect(chunks[0].last).to eq(end_at) + end + end + context 'with a span within a single year' do - it 'creates a single chunk ending at year end' do + it 'creates a single chunk ending at end_at' do start_at = DateTime.new(2020, 3, 15) end_at = DateTime.new(2020, 10, 20) @@ -48,8 +61,7 @@ expect(chunks.size).to eq(1) expect(chunks[0].begin).to eq(start_at) - # The implementation appears to extend to the end of the year - expect(chunks[0].end).to eq(DateTime.new(2020, 12, 31).end_of_day) + expect(chunks[0].end).to eq(end_at) end end @@ -76,7 +88,7 @@ end context 'with start and end dates in the same day' do - it 'returns a single chunk ending at the end of the year' do + it 'returns a single chunk covering only that day' do date = DateTime.new(2020, 5, 15) start_at = date.beginning_of_day end_at = date.end_of_day @@ -86,8 +98,7 @@ expect(chunks.size).to eq(1) expect(chunks[0].begin).to eq(start_at) - # Implementation extends to end of year - expect(chunks[0].end).to eq(DateTime.new(2020, 12, 31).end_of_day) + expect(chunks[0].end).to eq(end_at) end end @@ -130,22 +141,21 @@ end context 'with start date after end date' do - it 'still creates a chunk for start date year' do + it 'returns a single chunk bounded by the given dates' do start_at = DateTime.new(2023, 1, 1) end_at = DateTime.new(2020, 1, 1) service = described_class.new(start_at: start_at, end_at: end_at) chunks = service.call - # The implementation creates one chunk for the start date year expect(chunks.size).to eq(1) expect(chunks[0].begin).to eq(start_at) - expect(chunks[0].end).to eq(DateTime.new(2023, 12, 31).end_of_day) + expect(chunks[0].end).to eq(end_at) end end context 'when start date equals end date' do - it 'returns a single chunk extending to year end' do + it 'returns a single zero-length chunk' do date = DateTime.new(2022, 6, 15, 12, 30) service = described_class.new(start_at: date, end_at: date) @@ -153,8 +163,7 @@ expect(chunks.size).to eq(1) expect(chunks[0].begin).to eq(date) - # Implementation extends to end of year - expect(chunks[0].end).to eq(DateTime.new(2022, 12, 31).end_of_day) + expect(chunks[0].end).to eq(date) end end end From 63d7684e25a29e43ecf5532ff60521f41b6531cb Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Mon, 27 Jul 2026 20:33:57 +0200 Subject: [PATCH 23/31] feat: add a Poster Studio track width control Poster Studio gains a 50-300% track width slider next to track opacity. The value is persisted as route_width and converted server-side into a 0.5-3.0 multiplier on the trackWidth style parameter, which until now was declared in style_builder.js but never set by any caller. Also covers the #3204 regression: a continent-wide frame completes instead of failing the area check. --- app/controllers/posters_controller.rb | 2 +- .../poster_studio_editor_controller.js | 11 ++++ app/services/posters/generate.rb | 10 ++++ app/services/posters/native_renderer.rb | 4 +- app/views/posters/_studio.html.erb | 12 ++++ spec/services/posters/generate_spec.rb | 58 ++++++++++++++++++- spec/services/posters/native_renderer_spec.rb | 17 ++++++ vendor/poster_renderer/render.mjs | 1 + 8 files changed, 112 insertions(+), 3 deletions(-) diff --git a/app/controllers/posters_controller.rb b/app/controllers/posters_controller.rb index f3cc5aece..e9d3094a6 100644 --- a/app/controllers/posters_controller.rb +++ b/app/controllers/posters_controller.rb @@ -51,6 +51,6 @@ def destroy def poster_params params.require(:poster).permit(:name, :title, :lat, :lon, :distance, :theme, :start_at, :end_at, :source, - :route_fill, :route_opacity) + :route_fill, :route_opacity, :route_width) end end diff --git a/app/javascript/controllers/poster_studio_editor_controller.js b/app/javascript/controllers/poster_studio_editor_controller.js index 7848598c4..d08e83c99 100644 --- a/app/javascript/controllers/poster_studio_editor_controller.js +++ b/app/javascript/controllers/poster_studio_editor_controller.js @@ -76,6 +76,8 @@ export default class extends Controller { "fontSelect", "trackOpacity", "trackOpacityLabel", + "trackWidth", + "trackWidthLabel", "summary", "format", "dpi", @@ -95,6 +97,7 @@ export default class extends Controller { "saveEndAt", "saveSource", "saveOpacity", + "saveWidth", "dateStart", "dateEnd", "loadButton", @@ -133,6 +136,7 @@ export default class extends Controller { this.populateSizePicker() this.populateFonts() this.trackOpacityLabelTarget.textContent = `${this.trackOpacityTarget.value}%` + this.trackWidthLabelTarget.textContent = `${this.trackWidthTarget.value}%` } disconnect() { @@ -237,6 +241,7 @@ export default class extends Controller { extras: true, hiddenCategories: [...this.hidden], trackOpacity: this.trackOpacityValue(), + trackWidth: this.trackWidthValue(), }) } @@ -460,6 +465,7 @@ export default class extends Controller { if (!toggle.checked) this.hidden.add(toggle.dataset.layerCategory) }) this.trackOpacityLabelTarget.textContent = `${this.trackOpacityTarget.value}%` + this.trackWidthLabelTarget.textContent = `${this.trackWidthTarget.value}%` this.scheduleRestyle() } @@ -467,6 +473,10 @@ export default class extends Controller { return Number.parseInt(this.trackOpacityTarget.value, 10) / 100 } + trackWidthValue() { + return Number.parseInt(this.trackWidthTarget.value, 10) / 100 + } + // ===== Date range ===== seedDateInputs() { @@ -776,6 +786,7 @@ export default class extends Controller { this.saveEndAtTarget.value = endAt || "" this.saveSourceTarget.value = this.provider.trackSource() this.saveOpacityTarget.value = this.trackOpacityTarget.value + this.saveWidthTarget.value = this.trackWidthTarget.value this.saveFormTarget.requestSubmit() this.setStatus("Queued — rendering server-side into Recent posters…") } diff --git a/app/services/posters/generate.rb b/app/services/posters/generate.rb index 9d2848c64..2e969cb22 100644 --- a/app/services/posters/generate.rb +++ b/app/services/posters/generate.rb @@ -4,6 +4,8 @@ module Posters class Generate MAX_DISTANCE = 5_000_000 MIN_DISTANCE = 500 + MIN_ROUTE_WIDTH = 0.5 + MAX_ROUTE_WIDTH = 3.0 METERS_PER_DEGREE = 111_320.0 def initialize(poster) @@ -49,6 +51,7 @@ def render_natively(track) track: track, distance: distance, route_opacity: route_opacity, + route_width: route_width, subtitle: subtitle ).call attach_image(result[:png]) @@ -62,6 +65,13 @@ def route_opacity raw.clamp(0.05, 1.0) end + def route_width + raw = @poster.settings.fetch('route_width', 100).to_f + return 1.0 if raw <= 0 + + (raw / 100.0).clamp(MIN_ROUTE_WIDTH, MAX_ROUTE_WIDTH) + end + def distance @poster.settings.fetch('distance', 6000).to_i.clamp(MIN_DISTANCE, MAX_DISTANCE) end diff --git a/app/services/posters/native_renderer.rb b/app/services/posters/native_renderer.rb index de6b2eff6..0e017408a 100644 --- a/app/services/posters/native_renderer.rb +++ b/app/services/posters/native_renderer.rb @@ -18,11 +18,12 @@ class Error < StandardError; end RENDER_TIMEOUT = 180 TERMINATE_TIMEOUT = 5 - def initialize(poster:, track:, distance:, route_opacity:, subtitle:, command: nil) + def initialize(poster:, track:, distance:, route_opacity:, subtitle:, route_width: 1, command: nil) @poster = poster @track = track @distance = distance @route_opacity = route_opacity + @route_width = route_width @subtitle = subtitle @command = command || default_command end @@ -47,6 +48,7 @@ def job(png_path, pdf_path) tokens: theme_tokens, trackGeojson: { type: 'Feature', properties: {}, geometry: @track }, trackOpacity: @route_opacity, + trackWidth: @route_width, view: { lat: @poster.settings['lat'].to_f, lon: @poster.settings['lon'].to_f, diff --git a/app/views/posters/_studio.html.erb b/app/views/posters/_studio.html.erb index f9c941c93..55362ed0b 100644 --- a/app/views/posters/_studio.html.erb +++ b/app/views/posters/_studio.html.erb @@ -226,6 +226,17 @@ data-poster-studio-editor-target="trackOpacity" data-action="input->poster-studio-editor#layersChanged">
+
+
+ Track width + +
+ +
@@ -391,6 +402,7 @@ <%= f.hidden_field :end_at, data: { poster_studio_editor_target: 'saveEndAt' } %> <%= f.hidden_field :source, data: { poster_studio_editor_target: 'saveSource' } %> <%= f.hidden_field :route_opacity, data: { poster_studio_editor_target: 'saveOpacity' } %> + <%= f.hidden_field :route_width, data: { poster_studio_editor_target: 'saveWidth' } %> <% end %> diff --git a/spec/services/posters/generate_spec.rb b/spec/services/posters/generate_spec.rb index bf1fcd9db..63f91fea1 100644 --- a/spec/services/posters/generate_spec.rb +++ b/spec/services/posters/generate_spec.rb @@ -34,7 +34,7 @@ def run_generate it 'renders with the poster distance, opacity, subtitle and track' do expect(Posters::NativeRenderer).to receive(:new).with( - poster: poster, track: track, distance: 6000, route_opacity: 1.0, + poster: poster, track: track, distance: 6000, route_opacity: 1.0, route_width: 1.0, subtitle: '1 Apr 2026 – 30 Apr 2026' ).and_return(renderer) @@ -62,6 +62,44 @@ def run_generate end end + context 'when settings request a percentage track width' do + let(:poster) do + create(:poster, settings: attributes_for(:poster)[:settings].merge('route_width' => '250')) + end + + before { allow_any_instance_of(Posters::TrackBuilder).to receive(:call).and_return(track) } + + it 'passes the width as a multiplier' do + expect(Posters::NativeRenderer).to receive(:new).with(hash_including(route_width: 2.5)).and_return(renderer) + + run_generate + end + end + + context 'when settings omit the track width' do + before { allow_any_instance_of(Posters::TrackBuilder).to receive(:call).and_return(track) } + + it 'falls back to the unscaled width' do + expect(Posters::NativeRenderer).to receive(:new).with(hash_including(route_width: 1.0)).and_return(renderer) + + run_generate + end + end + + context 'when settings request a track width beyond the slider range' do + let(:poster) do + create(:poster, settings: attributes_for(:poster)[:settings].merge('route_width' => '900')) + end + + before { allow_any_instance_of(Posters::TrackBuilder).to receive(:call).and_return(track) } + + it 'clamps the width to the maximum multiplier' do + expect(Posters::NativeRenderer).to receive(:new).with(hash_including(route_width: 3.0)).and_return(renderer) + + run_generate + end + end + context 'when the requested distance fits within the poster studio range' do let(:poster) { create(:poster, settings: attributes_for(:poster)[:settings].merge('distance' => 2_924_948)) } @@ -86,6 +124,24 @@ def run_generate end end + context 'when a continent-wide frame is saved to the gallery' do + let(:poster) do + create(:poster, settings: attributes_for(:poster)[:settings].merge( + 'lat' => '40.019826138511576', 'lon' => '17.87175149999996', 'distance' => '2924948' + )) + end + let(:track_through_rome) { { 'type' => 'MultiLineString', 'coordinates' => [[[12.49, 41.90], [12.50, 41.91]]] } } + + before { allow_any_instance_of(Posters::TrackBuilder).to receive(:call).and_return(track_through_rome) } + + it 'completes instead of reporting the track as outside the map area' do + run_generate + + expect(poster.reload).to be_completed + expect(poster.settings['error']).to be_nil + end + end + context 'when there are no points in range' do before { allow_any_instance_of(Posters::TrackBuilder).to receive(:call).and_return(nil) } diff --git a/spec/services/posters/native_renderer_spec.rb b/spec/services/posters/native_renderer_spec.rb index f77bd37f7..d62a63ea7 100644 --- a/spec/services/posters/native_renderer_spec.rb +++ b/spec/services/posters/native_renderer_spec.rb @@ -41,6 +41,23 @@ def build_renderer(command: fake_command) expect(result[:pdf]).to eq('PDF:Berlin') end + it 'carries the track width multiplier into the job' do + job = JSON.parse( + described_class.new( + poster: poster, track: track, distance: 15_000, route_opacity: 0.6, + route_width: 2.5, subtitle: '1 Oct 2025 – 31 Oct 2025', command: fake_command + ).call[:png] + ) + + expect(job['trackWidth']).to eq(2.5) + end + + it 'defaults the track width multiplier to 1' do + job = JSON.parse(build_renderer.call[:png]) + + expect(job['trackWidth']).to eq(1) + end + it 'renders the explicit settings title when present' do poster.settings['title'] = 'My Trip' diff --git a/vendor/poster_renderer/render.mjs b/vendor/poster_renderer/render.mjs index 407fd4f64..98e7e4bfc 100644 --- a/vendor/poster_renderer/render.mjs +++ b/vendor/poster_renderer/render.mjs @@ -81,6 +81,7 @@ async function main() { theme, trackGeojson: job.trackGeojson ?? { type: "FeatureCollection", features: [] }, trackOpacity: job.trackOpacity ?? 1, + trackWidth: job.trackWidth ?? 1, ...(job.tilesUrl ? { tileUrl: job.tilesUrl } : {}), }) From b1a9f16cea3a040be0f59aaa39e0bc87afd92c2a Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Mon, 27 Jul 2026 20:35:22 +0200 Subject: [PATCH 24/31] docs: reference the reported issues in the changelog entries --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 256242a9f..72dbdc9de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,11 +11,11 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Google Semantic History and phone Timeline imports now tag points with a per-import tracker id instead of one shared constant, so tracks from different devices are no longer braided together. A one-time backfill rewrites existing points and regenerates affected tracks per user. - Points at exactly (0,0) — a common GPS glitch — are no longer accepted from any ingestion path (API, OwnTracks, Overland, Traccar, file imports) and no longer produce suggested visits at "Null Island". Existing (0,0) points are flagged as anomalies by a one-time cleanup that also removes visits placed at (0,0) and recalculates affected stats and tracks. -- Place names you set yourself are no longer overwritten by nightly reverse geocoding. Renaming a place, creating one by hand, or picking one on the timeline locks its name; renaming it back to "Suggested place" hands it back to auto-naming. +- Place names you set yourself are no longer overwritten by nightly reverse geocoding. Renaming a place, creating one by hand, or picking one on the timeline locks its name; renaming it back to "Suggested place" hands it back to auto-naming. Map v2 and the place drawer show when a name is locked (#3086, #3175) - Real-time visit detection no longer stops after the first run for users who track continuously — the debounce key is now released when the job runs. - The nightly visit suggestion job no longer scans forward to the end of the calendar year; it processes only the day it was asked for. - Merged visits now report the correct duration, centre, radius and suggested name instead of keeping the values of the first cluster in the merge. -- Visit suggestion failures no longer show a raw stack trace in your notifications, and repeated failures within an hour no longer create a notification each time. +- Visit suggestion failures no longer show a raw stack trace in your notifications, and repeated failures within an hour no longer create a notification each time (#3091) ## [1.10.1] - 2026-07-19, Berlin From b15faf2eb523e6568fefa951ca91b017c039058b Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Mon, 27 Jul 2026 20:44:39 +0200 Subject: [PATCH 25/31] test: cover the track width clamp floor and negative guard Adds specs for a below-range width clamping to the 0.5 floor and a negative width falling back to 1.0, both previously unexercised, and records the new control in the changelog. --- CHANGELOG.md | 4 ++++ spec/services/posters/generate_spec.rb | 28 ++++++++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c653224b1..fcb810824 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ## Unreleased +### Added + +- Poster Studio has a track width control: a 50–300% slider beside track opacity that scales the route line on the saved poster. + ### Fixed - Saving a zoomed-out Poster Studio view to the gallery no longer rejects routes that are visibly inside the poster frame (#3204). diff --git a/spec/services/posters/generate_spec.rb b/spec/services/posters/generate_spec.rb index 63f91fea1..4f4a825f7 100644 --- a/spec/services/posters/generate_spec.rb +++ b/spec/services/posters/generate_spec.rb @@ -100,6 +100,34 @@ def run_generate end end + context 'when settings request a track width below the slider range' do + let(:poster) do + create(:poster, settings: attributes_for(:poster)[:settings].merge('route_width' => '10')) + end + + before { allow_any_instance_of(Posters::TrackBuilder).to receive(:call).and_return(track) } + + it 'clamps the width to the minimum multiplier' do + expect(Posters::NativeRenderer).to receive(:new).with(hash_including(route_width: 0.5)).and_return(renderer) + + run_generate + end + end + + context 'when settings request a negative track width' do + let(:poster) do + create(:poster, settings: attributes_for(:poster)[:settings].merge('route_width' => '-50')) + end + + before { allow_any_instance_of(Posters::TrackBuilder).to receive(:call).and_return(track) } + + it 'falls back to the unscaled width' do + expect(Posters::NativeRenderer).to receive(:new).with(hash_including(route_width: 1.0)).and_return(renderer) + + run_generate + end + end + context 'when the requested distance fits within the poster studio range' do let(:poster) { create(:poster, settings: attributes_for(:poster)[:settings].merge('distance' => 2_924_948)) } From 66c0e7c91a2939cd0e770174688d0ba92531a496 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Mon, 27 Jul 2026 20:46:40 +0200 Subject: [PATCH 26/31] Shorten the lock wait and point the logs at the manual drop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every attempt queues an ACCESS EXCLUSIVE request that holds up each points reader and writer behind it. The lock is either free almost immediately or held by a long transaction a longer wait cannot outlast, so wait 1s instead of 5s and let boot try three times rather than ten — around 12s of startup instead of 185s, with the job as the real fallback. Neither dead end promises a rescue that will not come. The migration is recorded as applied whether or not the enqueue succeeds, and that enqueue is the only one in the codebase, so a failed hand-off and an exhausted job now both log the ALTER TABLE to run by hand. --- .../data_migrations/drop_legacy_lat_lon_job.rb | 8 ++++++-- ...0714090000_drop_legacy_lat_lon_from_points.rb | 16 +++++++++++++--- .../drop_legacy_lat_lon_job_spec.rb | 4 +++- .../drop_legacy_lat_lon_from_points_spec.rb | 4 +++- 4 files changed, 25 insertions(+), 7 deletions(-) diff --git a/app/jobs/data_migrations/drop_legacy_lat_lon_job.rb b/app/jobs/data_migrations/drop_legacy_lat_lon_job.rb index 3db23f8f7..90bc8768d 100644 --- a/app/jobs/data_migrations/drop_legacy_lat_lon_job.rb +++ b/app/jobs/data_migrations/drop_legacy_lat_lon_job.rb @@ -3,7 +3,10 @@ class DataMigrations::DropLegacyLatLonJob < ApplicationJob queue_as :data_migrations - LOCK_TIMEOUT = '5s' + # Each attempt queues an ACCESS EXCLUSIVE request that holds up every points + # reader and writer behind it, so keep the wait short: the lock is either free + # almost immediately or held by a long transaction a longer wait cannot outlast. + LOCK_TIMEOUT = '1s' MAX_ATTEMPTS = 288 @@ -24,7 +27,8 @@ class DataMigrations::DropLegacyLatLonJob < ApplicationJob def self.log_exhaustion(error) Rails.logger.error( "[DataMigrations::DropLegacyLatLon] gave up after #{MAX_ATTEMPTS} attempts (#{error.class}: #{error.message}); " \ - 'points.latitude / points.longitude are still present and must be dropped manually' + 'points.latitude / points.longitude are still present. Drop them once traffic is quiet with: ' \ + 'ALTER TABLE points DROP COLUMN IF EXISTS latitude, DROP COLUMN IF EXISTS longitude;' ) end diff --git a/db/migrate/20260714090000_drop_legacy_lat_lon_from_points.rb b/db/migrate/20260714090000_drop_legacy_lat_lon_from_points.rb index 7a0d8dc21..bb7d1c622 100644 --- a/db/migrate/20260714090000_drop_legacy_lat_lon_from_points.rb +++ b/db/migrate/20260714090000_drop_legacy_lat_lon_from_points.rb @@ -4,8 +4,12 @@ class DropLegacyLatLonFromPoints < ActiveRecord::Migration[8.0] disable_ddl_transaction! BATCH_SIZE = 50_000 - DROP_LOCK_TIMEOUT = '5s' - DROP_MAX_ATTEMPTS = 10 + # Every attempt queues an ACCESS EXCLUSIVE request that holds up each points + # reader and writer behind it, so keep the wait short: the lock is either free + # almost immediately or held by a long transaction that a longer wait will not + # outlast. Boot only needs a couple of tries — the job is the real fallback. + DROP_LOCK_TIMEOUT = '1s' + DROP_MAX_ATTEMPTS = 3 DROP_BACKOFF_SECONDS = 3 def up @@ -84,12 +88,18 @@ def drop_legacy_columns # rescue is deliberately broad: a malformed REDIS_URL, an exhausted pool and a # refused connection all reach here, and none of them are worth a restart loop. # The columns are unused, so leaving them in place is safe. + # + # Nothing retries this. The migration is recorded as applied either way and + # this is the only place that enqueues the job, so the log has to carry the + # manual remedy rather than promise a later boot will pick it up. def enqueue_drop_job DataMigrations::DropLegacyLatLonJob.perform_later rescue StandardError => e Rails.logger.error( '[DropLegacyLatLonFromPoints] could not enqueue DataMigrations::DropLegacyLatLonJob ' \ - "(#{e.class}: #{e.message}); the legacy columns remain and will be dropped on a later boot" + "(#{e.class}: #{e.message}); points.latitude / points.longitude are still present. " \ + 'Drop them once traffic is quiet with: ' \ + 'ALTER TABLE points DROP COLUMN IF EXISTS latitude, DROP COLUMN IF EXISTS longitude;' ) end diff --git a/spec/jobs/data_migrations/drop_legacy_lat_lon_job_spec.rb b/spec/jobs/data_migrations/drop_legacy_lat_lon_job_spec.rb index 6503981c9..b0f933308 100644 --- a/spec/jobs/data_migrations/drop_legacy_lat_lon_job_spec.rb +++ b/spec/jobs/data_migrations/drop_legacy_lat_lon_job_spec.rb @@ -34,7 +34,9 @@ described_class.perform_now expect(connection).to have_received(:execute).with('SET LOCAL statement_timeout = 0') - expect(connection).to have_received(:execute).with("SET LOCAL lock_timeout = '5s'") + expect(connection).to have_received(:execute).with( + "SET LOCAL lock_timeout = '#{described_class::LOCK_TIMEOUT}'" + ) end it 'runs the drop inside a transaction' do diff --git a/spec/migrations/drop_legacy_lat_lon_from_points_spec.rb b/spec/migrations/drop_legacy_lat_lon_from_points_spec.rb index 93227c896..9338d6c77 100644 --- a/spec/migrations/drop_legacy_lat_lon_from_points_spec.rb +++ b/spec/migrations/drop_legacy_lat_lon_from_points_spec.rb @@ -59,7 +59,9 @@ def stub_drop_raising(error_class, times: described_class::DROP_MAX_ATTEMPTS) migration.send(:drop_legacy_columns) expect(migration).to have_received(:execute).with('SET LOCAL statement_timeout = 0') - expect(migration).to have_received(:execute).with("SET LOCAL lock_timeout = '5s'") + expect(migration).to have_received(:execute).with( + "SET LOCAL lock_timeout = '#{described_class::DROP_LOCK_TIMEOUT}'" + ) end it 'hands off rather than aborting when a statement_timeout cancels the drop' do From 6695be9a634211925958efcdecbb794e37c748f7 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Mon, 27 Jul 2026 21:02:34 +0200 Subject: [PATCH 27/31] Wait long enough for the batched writers to let go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One second expires inside a batch. The same release backfills tracker ids and clears raw_data in 5k and 10k row batches that each hold RowExclusive on points for seconds, so the job would lose all 288 attempts against exactly the writers it has to wait out. At one try per five minutes a five second stall costs a fraction of a percent of the time, which the odds of ever finishing are worth. Boot keeps its short wait and three tries — a deploy should not be held up. Wrap both manual remedies in BEGIN and SET LOCAL so pasting one cannot queue the unbounded ACCESS EXCLUSIVE request this migration exists to avoid, and pin the tuned values so interpolating them into the SQL assertions cannot hide a bad edit. --- CHANGELOG.md | 2 +- .../data_migrations/drop_legacy_lat_lon_job.rb | 14 +++++++++----- ...260714090000_drop_legacy_lat_lon_from_points.rb | 3 ++- .../drop_legacy_lat_lon_job_spec.rb | 4 ++++ .../drop_legacy_lat_lon_from_points_spec.rb | 5 +++++ 5 files changed, 21 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e6b0e1885..6ff7cded3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Fixed -- Instances with heavy write traffic no longer crash-loop on the 1.10.1 upgrade. Dropping the legacy `points.latitude`/`points.longitude` columns needs an exclusive lock that busy instances could not win in one attempt, which aborted the migration and restarted the container in a loop. The drop is now retried, and if it still cannot get the lock it is handed to a background job so startup completes (#3176) +- Instances with heavy write traffic no longer crash-loop on the 1.10.1 upgrade. Dropping the legacy `points.latitude`/`points.longitude` columns needs an exclusive lock that busy instances could not win in one attempt, which aborted the migration and restarted the container in a loop. The drop is now retried, and if it still cannot get the lock it is handed to a background job so startup completes. If that job cannot get the lock either, the columns stay and the log prints the statement to run by hand — they are unused, so nothing breaks in the meantime (#3176) - Google Semantic History and phone Timeline imports now tag points with a per-import tracker id instead of one shared constant, so tracks from different devices are no longer braided together. A one-time backfill rewrites existing points and regenerates affected tracks per user. - Points at exactly (0,0) — a common GPS glitch — are no longer accepted from any ingestion path (API, OwnTracks, Overland, Traccar, file imports) and no longer produce suggested visits at "Null Island". Existing (0,0) points are flagged as anomalies by a one-time cleanup that also removes visits placed at (0,0) and recalculates affected stats and tracks. diff --git a/app/jobs/data_migrations/drop_legacy_lat_lon_job.rb b/app/jobs/data_migrations/drop_legacy_lat_lon_job.rb index 90bc8768d..b61b9b8ee 100644 --- a/app/jobs/data_migrations/drop_legacy_lat_lon_job.rb +++ b/app/jobs/data_migrations/drop_legacy_lat_lon_job.rb @@ -3,10 +3,13 @@ class DataMigrations::DropLegacyLatLonJob < ApplicationJob queue_as :data_migrations - # Each attempt queues an ACCESS EXCLUSIVE request that holds up every points - # reader and writer behind it, so keep the wait short: the lock is either free - # almost immediately or held by a long transaction a longer wait cannot outlast. - LOCK_TIMEOUT = '1s' + # Long enough to catch the gap between two batched writes. The same release + # backfills tracker ids and clears raw_data in 5k/10k-row batches that each + # hold RowExclusive on points for seconds at a time, so a one-second wait + # would expire inside a batch and lose every attempt. Each try does stall + # points behind an ACCESS EXCLUSIVE request, but at one try per five minutes + # that is a fraction of a percent of the time. + LOCK_TIMEOUT = '5s' MAX_ATTEMPTS = 288 @@ -28,7 +31,8 @@ def self.log_exhaustion(error) Rails.logger.error( "[DataMigrations::DropLegacyLatLon] gave up after #{MAX_ATTEMPTS} attempts (#{error.class}: #{error.message}); " \ 'points.latitude / points.longitude are still present. Drop them once traffic is quiet with: ' \ - 'ALTER TABLE points DROP COLUMN IF EXISTS latitude, DROP COLUMN IF EXISTS longitude;' + "BEGIN; SET LOCAL lock_timeout = '#{LOCK_TIMEOUT}'; " \ + 'ALTER TABLE points DROP COLUMN IF EXISTS latitude, DROP COLUMN IF EXISTS longitude; COMMIT;' ) end diff --git a/db/migrate/20260714090000_drop_legacy_lat_lon_from_points.rb b/db/migrate/20260714090000_drop_legacy_lat_lon_from_points.rb index bb7d1c622..3c8281c19 100644 --- a/db/migrate/20260714090000_drop_legacy_lat_lon_from_points.rb +++ b/db/migrate/20260714090000_drop_legacy_lat_lon_from_points.rb @@ -99,7 +99,8 @@ def enqueue_drop_job '[DropLegacyLatLonFromPoints] could not enqueue DataMigrations::DropLegacyLatLonJob ' \ "(#{e.class}: #{e.message}); points.latitude / points.longitude are still present. " \ 'Drop them once traffic is quiet with: ' \ - 'ALTER TABLE points DROP COLUMN IF EXISTS latitude, DROP COLUMN IF EXISTS longitude;' + "BEGIN; SET LOCAL lock_timeout = '#{DROP_LOCK_TIMEOUT}'; " \ + 'ALTER TABLE points DROP COLUMN IF EXISTS latitude, DROP COLUMN IF EXISTS longitude; COMMIT;' ) end diff --git a/spec/jobs/data_migrations/drop_legacy_lat_lon_job_spec.rb b/spec/jobs/data_migrations/drop_legacy_lat_lon_job_spec.rb index b0f933308..b9916f57f 100644 --- a/spec/jobs/data_migrations/drop_legacy_lat_lon_job_spec.rb +++ b/spec/jobs/data_migrations/drop_legacy_lat_lon_job_spec.rb @@ -27,6 +27,10 @@ ) end + it 'waits long enough to catch the gap between batched writes' do + expect(described_class::LOCK_TIMEOUT).to eq('5s') + end + it 'scopes both timeouts to the transaction so pooling cannot separate them' do allow(connection).to receive(:column_exists?).and_return(true) allow(connection).to receive(:execute) diff --git a/spec/migrations/drop_legacy_lat_lon_from_points_spec.rb b/spec/migrations/drop_legacy_lat_lon_from_points_spec.rb index 9338d6c77..bbdaee6f5 100644 --- a/spec/migrations/drop_legacy_lat_lon_from_points_spec.rb +++ b/spec/migrations/drop_legacy_lat_lon_from_points_spec.rb @@ -53,6 +53,11 @@ def stub_drop_raising(error_class, times: described_class::DROP_MAX_ATTEMPTS) expect(DataMigrations::DropLegacyLatLonJob).not_to have_received(:perform_later) end + it 'keeps the boot-time lock wait and attempt budget short' do + expect(described_class::DROP_LOCK_TIMEOUT).to eq('1s') + expect(described_class::DROP_MAX_ATTEMPTS).to eq(3) + end + it 'scopes both timeouts to the transaction so pooling cannot separate them' do stub_drop_raising(ActiveRecord::LockWaitTimeout, times: 0) From 3f38965a8ba216679e4b35d62a6336f8aa1617ee Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Mon, 27 Jul 2026 21:09:04 +0200 Subject: [PATCH 28/31] fix: judge the poster area check against the rendered frame The area check built a flat-degree box around the poster centre while the renderer frames in Mercator. At the old 20 km cap the two agreed to within 0.00 degrees, but at continental distances they diverge: at 60N over 5,000 km the box sat 3.66 degrees north of the frame, so tracks inside the poster were rejected and tracks outside it were accepted and rendered off-frame. Latitude bounds now come from the same Mercator framing render.mjs uses, and longitude is compared with wrapping so a frame straddling the antimeridian no longer rejects a track three degrees from its centre. Poster Studio shares the geometry through poster_studio/render/frame_geometry and warns when a view is too wide for the largest poster area instead of silently zooming the saved poster in. --- CHANGELOG.md | 2 +- .../poster_studio_editor_controller.js | 41 ++++++++------- .../poster_studio/render/frame_geometry.js | 52 +++++++++++++++++++ app/services/posters/generate.rb | 46 ++++++++++++++-- .../javascript/poster_frame_geometry_test.mjs | 38 ++++++++++++++ spec/services/posters/generate_spec.rb | 51 ++++++++++++++++++ 6 files changed, 205 insertions(+), 25 deletions(-) create mode 100644 app/javascript/poster_studio/render/frame_geometry.js create mode 100644 spec/javascript/poster_frame_geometry_test.mjs diff --git a/CHANGELOG.md b/CHANGELOG.md index 36c241069..f28409505 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Fixed -- Saving a zoomed-out Poster Studio view to the gallery no longer rejects routes that are visibly inside the poster frame (#3204). +- Saving a zoomed-out Poster Studio view to the gallery no longer rejects routes that are visibly inside the poster frame (#3204). The area check now uses the same Mercator framing as the renderer and wraps across the antimeridian, so high-latitude and Pacific-centred posters are judged against the frame you actually see. Poster Studio also warns when a view is too wide for the largest poster area instead of silently zooming the saved poster in. - Trial lifecycle email jobs left over from older releases are now discarded instead of retrying forever in the background queue. Mail addressed to a record that has since been deleted is also discarded rather than retried. - Reverse geocoding and place-name provider outages no longer flood error reporting with handled timeouts, dropped TLS connections, or invalid provider responses. A misconfigured or rate-limited provider — a bad API key, for example — is still reported. - Reverse geocoding retries point updates that time out while waiting on concurrent writes. diff --git a/app/javascript/controllers/poster_studio_editor_controller.js b/app/javascript/controllers/poster_studio_editor_controller.js index d08e83c99..be1cadf95 100644 --- a/app/javascript/controllers/poster_studio_editor_controller.js +++ b/app/javascript/controllers/poster_studio_editor_controller.js @@ -23,6 +23,7 @@ import { resolveTheme, } from "poster_studio/data/theme_loader" import { downloadBlob } from "poster_studio/export/download" +import { frameCovers } from "poster_studio/render/frame_geometry" import { drawOverlay } from "poster_studio/render/overlay" import { buildPosterStyle } from "poster_studio/render/style_builder" import { formatCoords } from "poster_studio/render/text_layout" @@ -791,14 +792,21 @@ export default class extends Controller { this.setStatus("Queued — rendering server-side into Recent posters…") } - sidecarDistance() { + framedDistance() { const bounds = this.previewMap.getBounds() const heightMeters = (bounds.getNorth() - bounds.getSouth()) * METERS_PER_DEGREE + return heightMeters * SIDECAR_DISTANCE_FACTOR + } + + sidecarDistance() { const [min, max] = SIDECAR_DISTANCE_RANGE - return Math.round( - Math.min(max, Math.max(min, heightMeters * SIDECAR_DISTANCE_FACTOR)), - ) + return Math.round(Math.min(max, Math.max(min, this.framedDistance()))) + } + + frameIsClamped() { + const [, max] = SIDECAR_DISTANCE_RANGE + return this.framedDistance() > max } // The server refuses renders without track data in the frame — mirror @@ -814,26 +822,21 @@ export default class extends Controller { reason = "No tracks inside the frame — move or zoom the map over your route to save to the gallery." } + const message = + reason || + (this.frameIsClamped() + ? "This view is wider than the largest poster area — the saved poster will be more zoomed in than the preview." + : null) this.saveButtonTarget.disabled = Boolean(reason) - this.saveNoticeTarget.textContent = reason || "" - this.saveNoticeTarget.classList.toggle("hidden", !reason) + this.saveNoticeTarget.textContent = message || "" + this.saveNoticeTarget.classList.toggle("hidden", !message) } - // Mirrors the server's track_intersects_area? box: ±distance/3 latitude, - // ±distance/4 longitude around the frame center. + // Mirrors the server's track_intersects_area?. frameCoversTrack(coords) { const center = this.previewMap.getCenter() - const distance = this.sidecarDistance() - const latDelta = distance / 3 / METERS_PER_DEGREE - const cosLat = Math.max(Math.cos((center.lat * Math.PI) / 180), 0.01) - const lonDelta = distance / 4 / (METERS_PER_DEGREE * cosLat) - return coords.some( - ([lng, lat]) => - lng >= center.lng - lonDelta && - lng <= center.lng + lonDelta && - lat >= center.lat - latDelta && - lat <= center.lat + latDelta, - ) + + return frameCovers(coords, center.lat, center.lng, this.sidecarDistance()) } updateSummary() { diff --git a/app/javascript/poster_studio/render/frame_geometry.js b/app/javascript/poster_studio/render/frame_geometry.js new file mode 100644 index 000000000..3d39f911f --- /dev/null +++ b/app/javascript/poster_studio/render/frame_geometry.js @@ -0,0 +1,52 @@ +// Poster frame geometry, shared by the studio's save guard and mirrored by +// Posters::Generate#track_intersects_area?. Latitude bounds come from the same +// Mercator framing render.mjs uses, so the guard and the render agree. +export const POSTER_FRAME = { width: 1200, height: 1600 } + +const TILE_PIXELS = 512 +const METERS_PER_PIXEL_AT_ZOOM_0 = 40075016.686 / TILE_PIXELS + +function worldPixels(lat, distance) { + const metersPerPixel = (2 * distance) / 3 / POSTER_FRAME.height + const cosLat = Math.min( + Math.max(Math.abs(Math.cos((lat * Math.PI) / 180)), 0.01), + 1, + ) + const zoom = Math.log2((METERS_PER_PIXEL_AT_ZOOM_0 * cosLat) / metersPerPixel) + return TILE_PIXELS * 2 ** zoom +} + +function mercatorToLatitude(y) { + return ((2 * Math.atan(Math.E ** y) - Math.PI / 2) * 180) / Math.PI +} + +export function frameBounds(lat, distance) { + const world = worldPixels(lat, distance) + const half = (Math.PI * POSTER_FRAME.height) / world + const centre = Math.log(Math.tan(Math.PI / 4 + (lat * Math.PI) / 360)) + + return { + south: mercatorToLatitude(centre - half), + north: mercatorToLatitude(centre + half), + lonDelta: (180 * POSTER_FRAME.width) / world, + } +} + +export function longitudeWithin(pointLon, centreLon, lonDelta) { + if (lonDelta >= 180) return true + + const wrapped = (((pointLon - centreLon + 180) % 360) + 360) % 360 + + return Math.abs(wrapped - 180) <= lonDelta +} + +export function frameCovers(coords, lat, lon, distance) { + const { south, north, lonDelta } = frameBounds(lat, distance) + + return coords.some( + ([pointLon, pointLat]) => + pointLat >= south && + pointLat <= north && + longitudeWithin(pointLon, lon, lonDelta), + ) +} diff --git a/app/services/posters/generate.rb b/app/services/posters/generate.rb index 2e969cb22..410037b95 100644 --- a/app/services/posters/generate.rb +++ b/app/services/posters/generate.rb @@ -6,7 +6,8 @@ class Generate MIN_DISTANCE = 500 MIN_ROUTE_WIDTH = 0.5 MAX_ROUTE_WIDTH = 3.0 - METERS_PER_DEGREE = 111_320.0 + TILE_PIXELS = 512 + METERS_PER_PIXEL_AT_ZOOM_0 = 40_075_016.686 / TILE_PIXELS def initialize(poster) @poster = poster @@ -79,17 +80,52 @@ def distance def track_intersects_area?(track) lat = @poster.settings['lat'].to_f lon = @poster.settings['lon'].to_f - lat_delta = (distance / 3.0) / METERS_PER_DEGREE - lon_delta = (distance / 4.0) / (METERS_PER_DEGREE * Math.cos(lat * Math::PI / 180).abs.clamp(0.01, 1.0)) + south, north = frame_latitude_bounds(lat) + lon_delta = frame_longitude_delta(lat) track['coordinates'].any? do |segment| segment.any? do |pt_lon, pt_lat| - pt_lat.between?(lat - lat_delta, lat + lat_delta) && - pt_lon.between?(lon - lon_delta, lon + lon_delta) + pt_lat.between?(south, north) && longitude_within?(pt_lon, lon, lon_delta) end end end + def frame_latitude_bounds(lat) + half = Math::PI * frame_size[:height] / frame_world_pixels(lat) + centre = Math.log(Math.tan((Math::PI / 4) + (lat * Math::PI / 360))) + + [mercator_to_latitude(centre - half), mercator_to_latitude(centre + half)] + end + + def frame_longitude_delta(lat) + 180.0 * frame_size[:width] / frame_world_pixels(lat) + end + + def frame_world_pixels(lat) + meters_per_pixel = (2 * distance / 3.0) / frame_size[:height] + zoom = Math.log2(METERS_PER_PIXEL_AT_ZOOM_0 * cos_latitude(lat) / meters_per_pixel) + + TILE_PIXELS * (2**zoom) + end + + def mercator_to_latitude(mercator_y) + ((2 * Math.atan(Math::E**mercator_y)) - (Math::PI / 2)) * 180 / Math::PI + end + + def cos_latitude(lat) + Math.cos(lat * Math::PI / 180).abs.clamp(0.01, 1.0) + end + + def longitude_within?(pt_lon, lon, lon_delta) + return true if lon_delta >= 180.0 + + (((pt_lon - lon + 180.0) % 360.0) - 180.0).abs <= lon_delta + end + + def frame_size + Posters::NativeRenderer::SIZE + end + def subtitle start_at = Time.zone.parse(@poster.settings['start_at']).utc end_at = Time.zone.parse(@poster.settings['end_at']).utc diff --git a/spec/javascript/poster_frame_geometry_test.mjs b/spec/javascript/poster_frame_geometry_test.mjs new file mode 100644 index 000000000..7dba33c13 --- /dev/null +++ b/spec/javascript/poster_frame_geometry_test.mjs @@ -0,0 +1,38 @@ +import assert from "node:assert/strict" +import { test } from "node:test" + +import { + frameBounds, + frameCovers, + longitudeWithin, +} from "../../app/javascript/poster_studio/render/frame_geometry.js" + +test("latitude bounds follow the Mercator frame, not flat degrees", () => { + const { south, north } = frameBounds(60, 5_000_000) + + assert.ok(Math.abs(south - 41.366) < 0.01, `south was ${south}`) + assert.ok(Math.abs(north - 71.944) < 0.01, `north was ${north}`) +}) + +test("latitude bounds stay symmetric at small distances", () => { + const { south, north } = frameBounds(52.52, 6000) + + assert.ok(Math.abs(52.52 - south - (north - 52.52)) < 0.001) +}) + +test("longitude comparison wraps across the antimeridian", () => { + assert.equal(longitudeWithin(-179, 178, 4.7), true) + assert.equal(longitudeWithin(170, 178, 4.7), false) +}) + +test("a frame wider than the world accepts every longitude", () => { + assert.equal(longitudeWithin(-179, 0, 200), true) +}) + +test("frameCovers accepts a track just inside the rendered frame", () => { + assert.equal(frameCovers([[10, 42]], 60, 10, 5_000_000), true) +}) + +test("frameCovers rejects a track north of the rendered frame", () => { + assert.equal(frameCovers([[10, 73]], 60, 10, 5_000_000), false) +}) diff --git a/spec/services/posters/generate_spec.rb b/spec/services/posters/generate_spec.rb index 4f4a825f7..3506eed50 100644 --- a/spec/services/posters/generate_spec.rb +++ b/spec/services/posters/generate_spec.rb @@ -212,6 +212,57 @@ def run_generate end end + context 'when the poster frame straddles the antimeridian' do + let(:poster) do + create(:poster, settings: attributes_for(:poster)[:settings].merge( + 'lat' => '-18.0', 'lon' => '178.0', 'distance' => '2000000' + )) + end + let(:track_across_the_dateline) do + { 'type' => 'MultiLineString', 'coordinates' => [[[-179.0, -18.0], [-178.9, -18.1]]] } + end + + before { allow_any_instance_of(Posters::TrackBuilder).to receive(:call).and_return(track_across_the_dateline) } + + it 'accepts a track three degrees east of the frame centre' do + run_generate + + expect(poster.reload).to be_completed + expect(poster.settings['error']).to be_nil + end + end + + context 'when a high-latitude frame is stretched by the Mercator projection' do + let(:poster) do + create(:poster, settings: attributes_for(:poster)[:settings].merge( + 'lat' => '60.0', 'lon' => '10.0', 'distance' => '5000000' + )) + end + + before { allow_any_instance_of(Posters::TrackBuilder).to receive(:call).and_return(track) } + + context 'with a track inside the rendered frame but south of a flat-degree box' do + let(:track) { { 'type' => 'MultiLineString', 'coordinates' => [[[10.0, 42.0], [10.1, 42.1]]] } } + + it 'accepts the track' do + run_generate + + expect(poster.reload).to be_completed + end + end + + context 'with a track north of the rendered frame but inside a flat-degree box' do + let(:track) { { 'type' => 'MultiLineString', 'coordinates' => [[[10.0, 73.0], [10.1, 73.1]]] } } + + it 'rejects the track' do + run_generate + + expect(poster.reload).to be_failed + expect(poster.settings['error']).to match(/does not pass through/) + end + end + end + context 'when the track does not pass through the poster area' do let(:distant_track) { { 'type' => 'MultiLineString', 'coordinates' => [[[14.42, 50.08], [14.43, 50.09]]] } } From 36fcd3b86e2cda1d4092ebd52e101e644a1a796a Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Mon, 27 Jul 2026 21:43:31 +0200 Subject: [PATCH 29/31] Update app version --- .app_version | 2 +- CHANGELOG.md | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.app_version b/.app_version index 4dae2985b..5ad2491cf 100644 --- a/.app_version +++ b/.app_version @@ -1 +1 @@ -1.10.1 +1.10.2 diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e39301c4..2aab9466b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this project adheres to [Semantic Versioning](http://semver.org/). -## Unreleased +## [1.10.2] - 2026-07-27, Berlin ### Added @@ -29,6 +29,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Point uploads from all ingestion paths (REST API, OwnTracks, Overland, Traccar) now retry transient statement and lock-wait timeouts, not just deadlocks, instead of failing the upload. - The DNS caching layer no longer crashes with a misleading `NoMethodError` when the SMTP server is not configured in the background worker, so email delivery surfaces the real configuration error instead. (#3038) + ## [1.10.1] - 2026-07-19, Berlin ### Added From f91cacd7f660b4039ba3cdc4ade6e1fe2edcecb6 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Mon, 27 Jul 2026 22:04:10 +0200 Subject: [PATCH 30/31] fix: keep custom basemaps loaded and restore layers after a style swap MapLibre only fires style.load when it builds a Style from scratch; its default setStyle path diffs the document into the live style silently. Every style swap therefore stripped the app's data layers without ever re-adding them, and the custom-style error listener stayed armed forever, so the first failed tile request discarded a perfectly good basemap and reverted to the default style. - Pass diff: false on every setStyle call, via a shared swapStyle helper - Only treat a failure of the style document itself as a style failure, not the tile, sprite and glyph requests it spawns - Align classifyBasemapUrl with the API's style_json_url? so the browser no longer accepts URLs the API rejects - Disable tile category and POI toggles under a raster or foreign-style basemap, where they have nothing to act on --- app/controllers/api/v1/settings_controller.rb | 10 +- .../maps/maplibre/map_initializer.js | 13 +- .../maps/maplibre/settings_manager.js | 45 ++++-- .../maps_maplibre/utils/basemap_url.js | 32 +++- spec/javascript/basemap_url_classify_test.mjs | 46 +++++- spec/javascript/map_initializer_test.mjs | 130 ++++++++++------ spec/javascript/settings_manager_test.mjs | 140 +++++++++++++++++- spec/requests/api/v1/settings_spec.rb | 18 +++ 8 files changed, 363 insertions(+), 71 deletions(-) diff --git a/app/controllers/api/v1/settings_controller.rb b/app/controllers/api/v1/settings_controller.rb index c6b2e0dce..be2d68030 100644 --- a/app/controllers/api/v1/settings_controller.rb +++ b/app/controllers/api/v1/settings_controller.rb @@ -126,12 +126,16 @@ def valid_tiles_url?(settings) TILE_URL_PLACEHOLDERS.all? { |placeholder| url.include?(placeholder) } end + # Mirrors classifyBasemapUrl in app/javascript/maps_maplibre/utils/basemap_url.js: + # an absolute http(s) URL, or a root-relative path for a style served from + # this instance. Keep the two in sync or the browser accepts a URL this + # rejects. def style_json_url?(url) uri = URI.parse(url) - return false unless uri.is_a?(URI::HTTP) - return false if uri.host.blank? + return false unless uri.path.to_s.downcase.end_with?('.json') + return uri.host.present? if uri.is_a?(URI::HTTP) - uri.path.to_s.downcase.end_with?('.json') + uri.scheme.nil? && uri.host.nil? && uri.path.start_with?('/') rescue URI::InvalidURIError false end diff --git a/app/javascript/controllers/maps/maplibre/map_initializer.js b/app/javascript/controllers/maps/maplibre/map_initializer.js index d5228bbe7..8443400a6 100644 --- a/app/javascript/controllers/maps/maplibre/map_initializer.js +++ b/app/javascript/controllers/maps/maplibre/map_initializer.js @@ -1,5 +1,6 @@ import maplibregl from "maplibre-gl" import { Toast } from "maps_maplibre/components/toast" +import { styleDocumentFailed } from "maps_maplibre/utils/basemap_url" import { getMapStyle } from "maps_maplibre/utils/style_manager" /** @@ -50,10 +51,14 @@ export class MapInitializer { settled = true map.off("error", onError) } - const onError = async () => { - if (settled) return + // Tile, sprite and glyph failures also surface as `error`. Reverting on + // those would discard a custom style that loaded perfectly well, so only + // a failure of the style document itself counts. + const onError = async (event) => { + if (settled || !styleDocumentFailed(event, style)) return settled = true map.off("style.load", onStyleLoad) + map.off("error", onError) Toast.error( "Custom map style could not be loaded; reverting to the default style.", ) @@ -62,11 +67,11 @@ export class MapInitializer { disabledPoiGroups, customTheme, }) - map.setStyle(fallbackStyle) + map.setStyle(fallbackStyle, { diff: false }) } map.once("style.load", onStyleLoad) - map.once("error", onError) + map.on("error", onError) } // Set globe projection after map loads diff --git a/app/javascript/controllers/maps/maplibre/settings_manager.js b/app/javascript/controllers/maps/maplibre/settings_manager.js index bbf06826e..20011bd06 100644 --- a/app/javascript/controllers/maps/maplibre/settings_manager.js +++ b/app/javascript/controllers/maps/maplibre/settings_manager.js @@ -1,5 +1,9 @@ import { Toast } from "maps_maplibre/components/toast" import { UpgradeBanner } from "maps_maplibre/components/upgrade_banner" +import { + classifyBasemapUrl, + styleDocumentFailed, +} from "maps_maplibre/utils/basemap_url" import { isGatedPlan } from "maps_maplibre/utils/layer_gate" import { LAYER_COLOR_DEFAULTS, @@ -1128,8 +1132,16 @@ export class SettingsController { return } - this.map.setStyle(style) + this.swapStyle(style) + } + + // MapLibre only fires style.load when it builds a Style from scratch. Its + // default path diffs the new document into the live style and stays silent, + // which would leave the app layers stripped and never re-added, so the + // rebuild has to be forced. + swapStyle(style) { this.map.once("style.load", () => this.restoreStyleLayers()) + this.map.setStyle(style, { diff: false }) } mapStyleOptions() { @@ -1159,21 +1171,24 @@ export class SettingsController { this.restoreStyleLayers() } - const onError = async () => { - if (settled) return + // MapLibre reports every failed request through `error`, tiles included. + // Only a failure of the style document may discard the user's basemap — + // one unreachable tile from an otherwise valid style must not. + const onError = async (event) => { + if (settled || !styleDocumentFailed(event, styleUrl)) return settled = true this.map.off("style.load", onLoad) + this.map.off("error", onError) Toast.error( "Custom map style could not be loaded; reverting to the default style.", ) const fallback = await getMapStyle(styleName, this.mapStyleOptions()) - this.map.setStyle(fallback) - this.map.once("style.load", () => this.restoreStyleLayers()) + this.swapStyle(fallback) } this.map.once("style.load", onLoad) - this.map.once("error", onError) - this.map.setStyle(styleUrl) + this.map.on("error", onError) + this.map.setStyle(styleUrl, { diff: false }) } restoreGlobeProjection() { @@ -1358,15 +1373,22 @@ export class SettingsController { /** * The Custom style draws no labels or POIs, so their toggles are - * disabled while it's active, with a tooltip explaining why. + * disabled while it's active, with a tooltip explaining why. A raster or + * foreign-style basemap carries no Protomaps layers at all, so there every + * toggle goes dead, not just the unsupported ones. */ syncStyleDependentToggles(styleName) { + const basemap = classifyBasemapUrl( + SettingsManager.getSetting("vectorTilesUrl"), + ) + const foreignBasemap = basemap === "raster" || basemap === "style" const custom = styleName === "custom" const inputs = this.controller.element.querySelectorAll( "input[data-tile-category], input[data-poi-group]", ) inputs.forEach((input) => { - const unavailable = custom && input.dataset.customSupported !== "true" + const unavailable = + foreignBasemap || (custom && input.dataset.customSupported !== "true") input.disabled = unavailable const label = input.closest("label") @@ -1375,8 +1397,9 @@ export class SettingsController { label.classList.toggle("tooltip", unavailable) label.style.cursor = unavailable ? "not-allowed" : "" if (unavailable) { - label.dataset.tip = - "Not available with the Custom map style — it draws no labels or points of interest" + label.dataset.tip = foreignBasemap + ? "Not available with a custom raster or style basemap — these layers come from the built-in vector tiles" + : "Not available with the Custom map style — it draws no labels or points of interest" } else { delete label.dataset.tip } diff --git a/app/javascript/maps_maplibre/utils/basemap_url.js b/app/javascript/maps_maplibre/utils/basemap_url.js index ad9bb5f6a..8d2aced8a 100644 --- a/app/javascript/maps_maplibre/utils/basemap_url.js +++ b/app/javascript/maps_maplibre/utils/basemap_url.js @@ -16,9 +16,11 @@ export function classifyBasemapUrl(url) { trimmed.includes("{x}") && trimmed.includes("{y}") - if (path.endsWith(".json") && !hasXyz) return "style" - - if (!hasXyz) return null + if (!hasXyz) { + return path.endsWith(".json") && isStyleDocumentLocation(trimmed) + ? "style" + : null + } if ( path.endsWith(".png") || @@ -31,3 +33,27 @@ export function classifyBasemapUrl(url) { return "vector" } + +// Matches Api::V1::SettingsController#style_json_url?: an absolute http(s) URL, +// or a root-relative path for a style served from this instance. A +// protocol-relative "//host/style.json" is neither, and the two validators must +// agree or the browser accepts a URL the API then rejects. +function isStyleDocumentLocation(url) { + if (/^https?:\/\//i.test(url)) return true + + return url.startsWith("/") && !url.startsWith("//") +} + +/** + * Whether a MapLibre `error` event reports a failure of the style document + * itself rather than one of the requests it spawns. + * @param {Object} event - MapLibre ErrorEvent + * @param {string} styleUrl - Style URL handed to setStyle + * @returns {boolean} True when the style document is what failed + */ +export function styleDocumentFailed(event, styleUrl) { + const failedUrl = event?.error?.url + if (!failedUrl) return true + + return failedUrl === styleUrl +} diff --git a/spec/javascript/basemap_url_classify_test.mjs b/spec/javascript/basemap_url_classify_test.mjs index 2bd1c2fb7..b2a726c6b 100644 --- a/spec/javascript/basemap_url_classify_test.mjs +++ b/spec/javascript/basemap_url_classify_test.mjs @@ -10,7 +10,7 @@ const source = await readFile( "utf8", ) const moduleUrl = `data:text/javascript;base64,${Buffer.from(source).toString("base64")}` -const { classifyBasemapUrl } = await import(moduleUrl) +const { classifyBasemapUrl, styleDocumentFailed } = await import(moduleUrl) test("classifies a raster XYZ URL with a jpg extension as raster", () => { assert.equal( @@ -129,3 +129,47 @@ test("trims surrounding whitespace before classifying", () => { "raster", ) }) + +test("classifies a root-relative style path as a style", () => { + assert.equal(classifyBasemapUrl("/maps/styles/mine.json"), "style") +}) + +test("rejects style paths the API would reject", () => { + assert.equal(classifyBasemapUrl("//evil.example/style.json"), null) + assert.equal(classifyBasemapUrl("ftp://example.com/style.json"), null) + assert.equal(classifyBasemapUrl("styles/mine.json"), null) +}) + +test("treats a failed request for the style document as a style failure", () => { + const styleUrl = "https://api.maptiler.com/maps/streets/style.json" + assert.equal( + styleDocumentFailed({ error: { url: styleUrl } }, styleUrl), + true, + ) +}) + +test("treats a failed tile, sprite or glyph request as unrelated", () => { + const styleUrl = "https://api.maptiler.com/maps/streets/style.json" + assert.equal( + styleDocumentFailed( + { error: { url: "https://api.maptiler.com/tiles/3/4/5.pbf" } }, + styleUrl, + ), + false, + ) + assert.equal( + styleDocumentFailed( + { error: { url: "https://api.maptiler.com/sprites/v4.png" } }, + styleUrl, + ), + false, + ) +}) + +test("treats an error carrying no URL as a style failure", () => { + assert.equal( + styleDocumentFailed({ error: new Error("bad style") }, "s"), + true, + ) + assert.equal(styleDocumentFailed(undefined, "s"), true) +}) diff --git a/spec/javascript/map_initializer_test.mjs b/spec/javascript/map_initializer_test.mjs index 13797e575..2a066dc9e 100644 --- a/spec/javascript/map_initializer_test.mjs +++ b/spec/javascript/map_initializer_test.mjs @@ -1,6 +1,14 @@ -import assert from "node:assert/strict"; -import { readFile } from "node:fs/promises"; -import test from "node:test"; +import assert from "node:assert/strict" +import { readFile } from "node:fs/promises" +import test from "node:test" + +const basemapUrlSource = await readFile( + new URL( + "../../app/javascript/maps_maplibre/utils/basemap_url.js", + import.meta.url, + ), + "utf8", +) async function loadMapInitializer({ getMapStyle, Toast }) { const source = await readFile( @@ -9,79 +17,109 @@ async function loadMapInitializer({ getMapStyle, Toast }) { import.meta.url, ), "utf8", - ); - const withoutImports = source.replace( - /^import[\s\S]*?from "[^"]+";?\n/gm, - "", - ); + ) + const withoutImports = source.replace(/^import[\s\S]*?from "[^"]+";?\n/gm, "") const dependencies = ` const maplibregl = globalThis.__mapInitializerMaplibre const getMapStyle = globalThis.__mapInitializerGetMapStyle const Toast = globalThis.__mapInitializerToast - `; - globalThis.__mapInitializerGetMapStyle = getMapStyle; - globalThis.__mapInitializerToast = Toast; - const url = `data:text/javascript;base64,${Buffer.from(`${dependencies}\n${withoutImports}`).toString("base64")}`; - return await import(`${url}#${Date.now()}`); + ${basemapUrlSource.replace(/^export /gm, "")} + ` + globalThis.__mapInitializerGetMapStyle = getMapStyle + globalThis.__mapInitializerToast = Toast + const url = `data:text/javascript;base64,${Buffer.from(`${dependencies}\n${withoutImports}`).toString("base64")}` + return await import(`${url}#${Date.now()}`) } class FakeMap { constructor(options) { - this.options = options; - this.listeners = new Map(); - this.setStyles = []; + this.options = options + this.listeners = { "style.load": [], error: [], load: [] } + this.setStyles = [] + } + + on(event, callback) { + this.listeners[event].push(callback) } once(event, callback) { - this.listeners.set(event, callback); + const wrapped = (payload) => { + this.off(event, wrapped) + callback(payload) + } + this.on(event, wrapped) } off(event, callback) { - if (this.listeners.get(event) === callback) this.listeners.delete(event); + this.listeners[event] = this.listeners[event].filter((c) => c !== callback) } - emit(event) { - const callback = this.listeners.get(event); - this.listeners.delete(event); - callback?.(); + emit(event, payload) { + for (const callback of [...this.listeners[event]]) callback(payload) } - setStyle(style) { - this.setStyles.push(style); + setStyle(style, options) { + this.setStyles.push({ style, options }) } addControl() {} } -test("falls back from an unavailable initial custom style URL", async () => { - let map; - const errors = []; +const STYLE_URL = "https://tiles.example/style.json" + +async function initializeWithCustomStyle({ emitOnConstruct } = {}) { + const state = { map: null, errors: [] } globalThis.__mapInitializerMaplibre = { Map: class extends FakeMap { constructor(options) { super(options) - map = this - queueMicrotask(() => this.emit("error")) + state.map = this + if (emitOnConstruct) queueMicrotask(() => emitOnConstruct(this)) } }, NavigationControl: class {}, AttributionControl: class {}, - }; - const fallback = { version: 8, sources: {}, layers: [] }; + } + state.fallback = { version: 8, sources: {}, layers: [] } const { MapInitializer } = await loadMapInitializer({ getMapStyle: async (_styleName, options) => - options.vectorTilesUrl - ? "https://tiles.example/broken-style.json" - : fallback, - Toast: { error: (message) => errors.push(message) }, - }); - - await MapInitializer.initialize( - {}, - { vectorTilesUrl: "https://tiles.example/broken-style.json" }, - ); - await new Promise((resolve) => setImmediate(resolve)); - - assert.deepEqual(map.setStyles, [fallback]); - assert.equal(errors.length, 1); -}); + options.vectorTilesUrl ? STYLE_URL : state.fallback, + Toast: { error: (message) => state.errors.push(message) }, + }) + + await MapInitializer.initialize({}, { vectorTilesUrl: STYLE_URL }) + await new Promise((resolve) => setImmediate(resolve)) + return state +} + +test("falls back from an unavailable initial custom style URL", async () => { + const state = await initializeWithCustomStyle({ + emitOnConstruct: (map) => + map.emit("error", { error: { url: STYLE_URL, status: 404 } }), + }) + + assert.deepEqual(state.map.setStyles, [ + { style: state.fallback, options: { diff: false } }, + ]) + assert.equal(state.errors.length, 1) +}) + +test("keeps a custom style whose tiles fail to load", async () => { + const state = await initializeWithCustomStyle({ + emitOnConstruct: (map) => + map.emit("error", { error: { url: "https://tiles.example/3/4/5.pbf" } }), + }) + + assert.deepEqual(state.map.setStyles, []) + assert.deepEqual(state.errors, []) +}) + +test("stops watching for style errors once the custom style loads", async () => { + const state = await initializeWithCustomStyle({ + emitOnConstruct: (map) => map.emit("style.load"), + }) + state.map.emit("error", { error: { url: STYLE_URL } }) + + assert.deepEqual(state.map.setStyles, []) + assert.deepEqual(state.errors, []) +}) diff --git a/spec/javascript/settings_manager_test.mjs b/spec/javascript/settings_manager_test.mjs index 3fec9b433..3a625aae4 100644 --- a/spec/javascript/settings_manager_test.mjs +++ b/spec/javascript/settings_manager_test.mjs @@ -21,7 +21,7 @@ const combinedSource = `${basemapUrlSource}\n${withoutImports}` const moduleUrl = `data:text/javascript;base64,${Buffer.from(combinedSource).toString("base64")}` const { LAYER_COLOR_DEFAULTS, SettingsManager } = await import(moduleUrl) -async function loadSettingsController(settingsManager) { +async function loadSettingsController(settingsManager, overrides = {}) { const controllerSource = await readFile( new URL( "../../app/javascript/controllers/maps/maplibre/settings_manager.js", @@ -34,13 +34,20 @@ async function loadSettingsController(settingsManager) { "", ) globalThis.__settingsManagerTestDouble = settingsManager + globalThis.__settingsManagerToast = overrides.Toast ?? { + error() {}, + success() {}, + } + globalThis.__settingsManagerGetMapStyle = + overrides.getMapStyle ?? (async () => ({})) const dependencies = ` - const Toast = { error() {}, success() {} } + const Toast = globalThis.__settingsManagerToast const UpgradeBanner = {} const isGatedPlan = () => false const LAYER_COLOR_DEFAULTS = ${JSON.stringify(LAYER_COLOR_DEFAULTS)} const SettingsManager = globalThis.__settingsManagerTestDouble - const getMapStyle = async () => ({}) + const getMapStyle = globalThis.__settingsManagerGetMapStyle + ${basemapUrlSource.replace(/^export /gm, "")} ` const url = `data:text/javascript;base64,${Buffer.from(`${dependencies}\n${withoutImports}`).toString("base64")}` return await import(`${url}#${Date.now()}`) @@ -162,3 +169,130 @@ test("resetting layer colors cancels stale debounced saves", async () => { assert.deepEqual(updates, [LAYER_COLOR_DEFAULTS]) }) + +// Minimal stand-in for maplibregl.Map's Evented interface. +class FakeMap { + constructor() { + this.listeners = { "style.load": [], error: [] } + this.setStyleCalls = [] + } + + on(event, callback) { + this.listeners[event].push(callback) + } + + once(event, callback) { + const wrapped = (payload) => { + this.off(event, wrapped) + callback(payload) + } + this.on(event, wrapped) + } + + off(event, callback) { + this.listeners[event] = this.listeners[event].filter((c) => c !== callback) + } + + emit(event, payload) { + for (const callback of [...this.listeners[event]]) callback(payload) + } + + setStyle(style, options) { + this.setStyleCalls.push({ style, options }) + } +} + +async function styleSwapController({ getMapStyle } = {}) { + const { SettingsController } = await loadSettingsController( + { getSetting: () => null }, + { getMapStyle, Toast: { error: (m) => toasts.push(m), success() {} } }, + ) + const controller = new SettingsController({ + element: { querySelector: () => null, querySelectorAll: () => [] }, + map: new FakeMap(), + layerManager: { clearLayerReferences() {} }, + settings: {}, + loadMapData: () => restored.push("loadMapData"), + }) + controller.restoreGlobeProjection = () => {} + return controller +} + +let toasts = [] +let restored = [] + +test("a custom style URL is applied with diff disabled so style.load fires", async () => { + toasts = [] + restored = [] + const controller = await styleSwapController() + + controller.applyUserStyleUrl("https://tiles.example/style.json", "light") + + assert.deepEqual(controller.map.setStyleCalls, [ + { style: "https://tiles.example/style.json", options: { diff: false } }, + ]) + + controller.map.emit("style.load") + assert.deepEqual(restored, ["loadMapData"]) + assert.deepEqual(toasts, []) +}) + +test("a failed tile request does not discard a working custom style", async () => { + toasts = [] + restored = [] + const controller = await styleSwapController() + + controller.applyUserStyleUrl("https://tiles.example/style.json", "light") + controller.map.emit("error", { + error: { url: "https://tiles.example/tiles/3/4/5.pbf" }, + }) + + assert.deepEqual(toasts, []) + assert.equal(controller.map.setStyleCalls.length, 1) + + controller.map.emit("style.load") + assert.deepEqual(restored, ["loadMapData"]) +}) + +test("a failed style document reverts to the default style and reloads layers", async () => { + toasts = [] + restored = [] + const fallback = { version: 8, sources: {}, layers: [] } + const controller = await styleSwapController({ + getMapStyle: async () => fallback, + }) + + controller.applyUserStyleUrl("https://tiles.example/style.json", "light") + controller.map.emit("error", { + error: { url: "https://tiles.example/style.json" }, + }) + await new Promise((resolve) => setTimeout(resolve, 0)) + + assert.equal(toasts.length, 1) + assert.deepEqual(controller.map.setStyleCalls.at(-1), { + style: fallback, + options: { diff: false }, + }) + + controller.map.emit("style.load") + assert.deepEqual(restored, ["loadMapData"]) +}) + +test("a stale style.load after the fallback does not double-reload", async () => { + toasts = [] + restored = [] + const controller = await styleSwapController({ + getMapStyle: async () => ({ version: 8, sources: {}, layers: [] }), + }) + + controller.applyUserStyleUrl("https://tiles.example/style.json", "light") + controller.map.emit("error", { + error: { url: "https://tiles.example/style.json" }, + }) + await new Promise((resolve) => setTimeout(resolve, 0)) + + controller.map.emit("style.load") + controller.map.emit("style.load") + + assert.deepEqual(restored, ["loadMapData"]) +}) diff --git a/spec/requests/api/v1/settings_spec.rb b/spec/requests/api/v1/settings_spec.rb index e24e800bd..20018191e 100644 --- a/spec/requests/api/v1/settings_spec.rb +++ b/spec/requests/api/v1/settings_spec.rb @@ -123,6 +123,8 @@ params: { settings: { maps_maplibre_tiles_url: 'https://tiles.example.com/{z}.mvt' } } expect(response).to have_http_status(:unprocessable_content) + expect(response.parsed_body['errors']) + .to include(a_string_including('or be a MapLibre style URL ending in .json')) expect(user.reload.safe_settings.maps_maplibre_tiles_url).to be_nil end @@ -152,6 +154,14 @@ .to eq('https://api.maptiler.com/maps/streets/style.json?key=abc') end + it 'accepts a root-relative style URL served from this instance' do + patch "/api/v1/settings?api_key=#{api_key}", + params: { settings: { maps_maplibre_tiles_url: '/maps_maplibre/styles/mine.json' } } + + expect(response).to have_http_status(:success) + expect(user.reload.safe_settings.maps_maplibre_tiles_url).to eq('/maps_maplibre/styles/mine.json') + end + it 'rejects a non-http style.json URL' do patch "/api/v1/settings?api_key=#{api_key}", params: { settings: { maps_maplibre_tiles_url: 'ftp://example.com/style.json' } } @@ -160,6 +170,14 @@ expect(user.reload.safe_settings.maps_maplibre_tiles_url).to be_nil end + it 'rejects a protocol-relative style.json URL' do + patch "/api/v1/settings?api_key=#{api_key}", + params: { settings: { maps_maplibre_tiles_url: '//tiles.example.com/style.json' } } + + expect(response).to have_http_status(:unprocessable_content) + expect(user.reload.safe_settings.maps_maplibre_tiles_url).to be_nil + end + it 'rejects a maps_maplibre_tiles_url that is neither an XYZ tile URL nor a style.json' do patch "/api/v1/settings?api_key=#{api_key}", params: { settings: { maps_maplibre_tiles_url: 'https://tiles.example.com/basemap' } } From 903d4b63762dc6c35646d194636de5e96277af09 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Mon, 27 Jul 2026 22:08:24 +0200 Subject: [PATCH 31/31] fix: lock place names customised before name locking existed A one-time backfill sets name_locked_at for places whose name a user set before 1.10.2, so the next reverse geocoding run no longer overwrites pre-upgrade renames. Machine-generated names are recognised by recomputing both historical formats from stored geodata and stay unlocked; places without geodata are locked conservatively. Also dedupes the (0,0) changelog bullet. --- CHANGELOG.md | 3 +- .../backfill_place_name_locks_job.rb | 48 +++++++++++++++ ...30000_enqueue_place_name_locks_backfill.rb | 9 +++ db/schema.rb | 2 +- .../backfill_place_name_locks_job_spec.rb | 60 +++++++++++++++++++ 5 files changed, 119 insertions(+), 3 deletions(-) create mode 100644 app/jobs/data_migrations/backfill_place_name_locks_job.rb create mode 100644 db/migrate/20260727130000_enqueue_place_name_locks_backfill.rb create mode 100644 spec/jobs/data_migrations/backfill_place_name_locks_job_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 03cb4aa46..801b175c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,8 +20,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Reverse geocoding and place-name provider outages no longer flood error reporting with handled timeouts, dropped TLS connections, or invalid provider responses. A misconfigured or rate-limited provider — a bad API key, for example — is still reported. - Reverse geocoding retries point updates that time out while waiting on concurrent writes. - Google Semantic History and phone Timeline imports now tag points with a per-import tracker id instead of one shared constant, so tracks from different devices are no longer braided together. A one-time backfill rewrites existing points and regenerates affected tracks per user. -- Points at exactly (0,0) — a common GPS glitch — are no longer accepted from any ingestion path (API, OwnTracks, Overland, Traccar, file imports) and no longer produce suggested visits at "Null Island". Existing (0,0) points are flagged as anomalies by a one-time cleanup that also removes visits placed at (0,0) and recalculates affected stats and tracks. -- Place names you set yourself are no longer overwritten by nightly reverse geocoding. Renaming a place, creating one by hand, or picking one on the timeline locks its name; renaming it back to "Suggested place" hands it back to auto-naming. Map v2 and the place drawer show when a name is locked (#3086, #3175) +- Place names you set yourself are no longer overwritten by nightly reverse geocoding. Renaming a place, creating one by hand, or picking one on the timeline locks its name; renaming it back to "Suggested place" hands it back to auto-naming. Map v2 and the place drawer show when a name is locked. A one-time backfill locks names that were customised before this release (#3086, #3175) - Real-time visit detection no longer stops after the first run for users who track continuously — the debounce key is now released when the job runs. - The nightly visit suggestion job no longer scans forward to the end of the calendar year; it processes only the day it was asked for. - Merged visits now report the correct duration, centre, radius and suggested name instead of keeping the values of the first cluster in the merge. diff --git a/app/jobs/data_migrations/backfill_place_name_locks_job.rb b/app/jobs/data_migrations/backfill_place_name_locks_job.rb new file mode 100644 index 000000000..0b2d5dae4 --- /dev/null +++ b/app/jobs/data_migrations/backfill_place_name_locks_job.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +class DataMigrations::BackfillPlaceNameLocksJob < ApplicationJob + queue_as :data_migrations + + BATCH_SIZE = 1_000 + + def perform + locked_total = 0 + + candidates.in_batches(of: BATCH_SIZE) do |batch| + ids = batch.reject { |place| machine_named?(place) }.map(&:id) + next if ids.empty? + + locked_total += Place.where(id: ids, name_locked_at: nil).update_all(name_locked_at: Time.current) + end + + Rails.logger.info("[#{self.class}] locked #{locked_total} user-named places") + end + + private + + def candidates + Place.where(name_locked_at: nil).where.not(name: Place::DEFAULT_NAME) + end + + def machine_named?(place) + properties = place.geodata.is_a?(Hash) ? place.geodata['properties'] : nil + return false if properties.blank? + + [ + Visits::Names::Builder.build_from_properties(properties), + geocoder_style_name(properties) + ].compact.include?(place.name) + end + + # Mirrors ReverseGeocoding::Places::FetchData#place_name, the format machine + # names were written in before name locking existed. + def geocoder_style_name(properties) + name = properties['name'] + type = properties['osm_value']&.capitalize&.gsub('_', ' ') + address = "#{properties['postcode']} #{properties['street']}" + address += " #{properties['housenumber']}" if properties['housenumber'].present? + name ||= address + + "#{name} (#{type})" + end +end diff --git a/db/migrate/20260727130000_enqueue_place_name_locks_backfill.rb b/db/migrate/20260727130000_enqueue_place_name_locks_backfill.rb new file mode 100644 index 000000000..709c3e7b6 --- /dev/null +++ b/db/migrate/20260727130000_enqueue_place_name_locks_backfill.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class EnqueuePlaceNameLocksBackfill < ActiveRecord::Migration[8.0] + def up + DataMigrations::BackfillPlaceNameLocksJob.perform_later + end + + def down; end +end diff --git a/db/schema.rb b/db/schema.rb index 001a4ef70..d35129c73 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.0].define(version: 2026_07_27_120000) do +ActiveRecord::Schema[8.0].define(version: 2026_07_27_130000) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" enable_extension "pgcrypto" diff --git a/spec/jobs/data_migrations/backfill_place_name_locks_job_spec.rb b/spec/jobs/data_migrations/backfill_place_name_locks_job_spec.rb new file mode 100644 index 000000000..c521874cb --- /dev/null +++ b/spec/jobs/data_migrations/backfill_place_name_locks_job_spec.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe DataMigrations::BackfillPlaceNameLocksJob, type: :job do + describe '#perform' do + it 'locks a renamed place without geodata' do + place = create(:place, name: 'Grandma and Grandpa') + + described_class.perform_now + + expect(place.reload.name_locked_at).to be_present + end + + it 'locks a renamed place whose name differs from its geodata-derived names' do + place = create(:place, :with_geodata, name: 'Our favourite brewery') + + described_class.perform_now + + expect(place.reload.name_locked_at).to be_present + end + + it 'does not lock places with the default name' do + place = create(:place, name: Place::DEFAULT_NAME) + + described_class.perform_now + + expect(place.reload.name_locked_at).to be_nil + end + + it 'does not lock geocoder-format machine names' do + place = create(:place, :with_geodata, name: 'Braugasthaus Zum Alten Fritz (Restaurant)') + + described_class.perform_now + + expect(place.reload.name_locked_at).to be_nil + end + + it 'does not lock builder-format machine names' do + place = create( + :place, + :with_geodata, + name: 'Braugasthaus Zum Alten Fritz, Greifswalder Chaussee, 84-85, Stralsund, Mecklenburg-Vorpommern' + ) + + described_class.perform_now + + expect(place.reload.name_locked_at).to be_nil + end + + it 'leaves already-locked places untouched' do + locked_at = 2.days.ago + place = create(:place, name: 'Home', name_locked_at: locked_at) + + described_class.perform_now + + expect(place.reload.name_locked_at).to be_within(1.second).of(locked_at) + end + end +end