-
-
Notifications
You must be signed in to change notification settings - Fork 360
1.10.2 #3199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
1.10.2 #3199
Changes from 37 commits
04e91a5
8756850
7ae4859
dba153f
0ae855c
bcf4982
1f8b8bc
c869059
0e59ea5
8977454
2c40d7a
845a935
f8ab3f0
4a749a5
eec1be0
1ec0814
93481cf
36d4e31
43afcc6
9604b76
8416968
f35d7db
5fff92f
dfe9d2d
59439e6
bf04182
f0256b5
a59c5e9
877e74e
5c5a424
8a07b53
d7bc525
63d7684
b1a9f16
c57ad0b
b15faf2
66c0e7c
3f0b82e
e788640
6695be9
5d35267
3f38965
7fe2c4d
abea28d
3f38cad
d2f2bbd
36fcd3b
f91cacd
ffd8baf
70597ac
903d4b6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| class DataMigrations::DropLegacyLatLonJob < ApplicationJob | ||
| queue_as :data_migrations | ||
|
|
||
| # 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 | ||
|
|
||
| # 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: 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. Drop them once traffic is quiet with: ' \ | ||
| "BEGIN; SET LOCAL lock_timeout = '#{LOCK_TIMEOUT}'; " \ | ||
| 'ALTER TABLE points DROP COLUMN IF EXISTS latitude, DROP COLUMN IF EXISTS longitude; COMMIT;' | ||
| ) | ||
| end | ||
|
|
||
| def perform | ||
| connection = ActiveRecord::Base.connection | ||
| return unless legacy_columns?(connection) | ||
|
|
||
| # 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') | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def legacy_columns?(connection) | ||
| connection.column_exists?(:points, :latitude) || connection.column_exists?(:points, :longitude) | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
Comment on lines
+71
to
+78
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Mark machine-originated name changes explicitly. This callback treats every unmarked name change as a user edit. However, 🤖 Prompt for AI Agents |
||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -36,15 +36,18 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+39
to
+50
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== file outline =="
ast-grep outline app/services/reverse_geocoding/places/fetch_data.rb || true
echo "== relevant source =="
sed -n '1,220p' app/services/reverse_geocoding/places/fetch_data.rb | cat -n
echo "== git diff stat =="
git diff --stat || true
echo "== searches for helper/usages =="
rg -n "with_deadlock_retry|Deadlocked|Deadlock|lock_wait|query_cancellation|upsert_all|machine_named" app/services/reverse_geocoding -S || trueRepository: Freika/dawarich Length of output: 9138 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== reverse geocoding points helper =="
sed -n '1,120p' app/services/reverse_geocoding/points/fetch_data.rb | cat -n
echo "== model machine_named references =="
rg -n "machine_named|name_locked" app -S || true
echo "== ActiveRecord retry exceptions definitions usages =="
rg -n "ActiveRecord::ConnectionTimedOut|QueryCanceled|Deadlock|deadlock|query_cancellation|lock_wait_timeout" rails Gemfile . -S || trueRepository: Freika/dawarich Length of output: 10493 🌐 Web query:
💡 Result: In Rails 8.1, ActiveRecord provides built-in mechanisms for handling retries, but they are focused on idempotent queries encountering specific connection errors rather than a general-purpose "retry_exceptions" configuration for all database exceptions [1][2][3]. Key distinctions for the exceptions mentioned: 1. ActiveRecord::ConnectionTimeoutError: This exception is raised when a connection cannot be obtained from the pool within the allowed timeout period [4]. It is not automatically retried by the adapter's connection retry mechanism because it indicates pool exhaustion rather than a transient connection failure [5][6]. 2. ActiveRecord::Deadlocked: This exception occurs when the database engine kills a transaction to resolve a deadlock [7]. Rails does not automatically retry deadlocked transactions at the adapter level because retrying them requires restarting the entire transaction block, which is not inherently safe to do automatically [7]. For deadlocks, the standard Rails pattern is to use ActiveJob's retry mechanism or manual rescue blocks [8]. For example: retry_on ActiveRecord::Deadlocked, wait: 5.seconds, attempts: 3 [8] 3. ActiveRecord::QueryCanceled: This generally relates to query timeouts or external cancellations. It is not part of the automatic connection-level retry logic [5][6]. Automatic Retry Logic Rails 8.1 automatically retries certain idempotent queries (such as simple association loads and Citations:
Apply the bounded contention-retry policy to the primary place update.
Proposed fix- place.update!(attributes)
+ with_deadlock_retry { place.update!(attributes) }📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| def find_place(place_data, existing_places) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -107,7 +110,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -142,7 +145,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.