Stop concurrent imports from racing on package names
Production imports kept aborting with ActiveRecord::RecordNotUnique on packages_name_key and - on environments that never got the constraint - silently created duplicate packages. Root cause: overlapping update_from_deb_repos runs (scheduled + manual) both do find-by-name then insert; whichever wins the race aborts the whole run, whichever loses gets a duplicate if the constraint was missing. Three things so the mechanism stops being the problem: - An advisory lock serializes update_from_deb_repos runs. A second run that finds the lock held simply skips with a warning instead of racing the first one. The lock is session-scoped, so it is released automatically when the process exits. - A per-package rescue for ActiveRecord::RecordNotUnique. If a create still races (e.g. a package being created by a web upload), the package is re-fetched and updated instead of aborting the run. - A real migration for packages_name_key, which previously only existed where it had been added by hand. From now on db:migrate creates it everywhere, new databases included. Also verified with a reproduction: a single sequential run of the deployed importer against a fresh database (both architectures, ~54k packages) creates zero duplicates.
This commit is contained in:
parent
3f37e2d857
commit
c5cd77e9ee
2 changed files with 78 additions and 14 deletions
|
|
@ -0,0 +1,28 @@
|
||||||
|
# frozen_string_literal: true
|
||||||
|
|
||||||
|
# Packages must be unique by name - the same package is listed once per
|
||||||
|
# architecture/component in the Debian archive and the importer looks it up
|
||||||
|
# by name before inserting, so anything that lets two rows for one name appear
|
||||||
|
# (e.g. concurrent import runs) breaks the site. Production had accidentally
|
||||||
|
# relied on a constraint that was only ever added ad-hoc on some databases
|
||||||
|
# and never tracked as a migration; this makes db:migrate the single source
|
||||||
|
# of truth.
|
||||||
|
class AddUniqueNameConstraintToPackages < ActiveRecord::Migration[8.1]
|
||||||
|
def up
|
||||||
|
add_unique_constraint :packages, :name, name: :packages_name_key unless constraint_exists?
|
||||||
|
end
|
||||||
|
|
||||||
|
def down
|
||||||
|
remove_unique_constraint :packages, :name, name: :packages_name_key if constraint_exists?
|
||||||
|
end
|
||||||
|
|
||||||
|
private
|
||||||
|
|
||||||
|
def constraint_exists?
|
||||||
|
select_value(<<~SQL.squish)
|
||||||
|
SELECT 1
|
||||||
|
FROM pg_constraint
|
||||||
|
WHERE conrelid = 'packages'::regclass AND conname = 'packages_name_key' AND contype = 'u'
|
||||||
|
SQL
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
@ -54,6 +54,13 @@ BLACKLIST_DESCRIPTION_PATTERN = [
|
||||||
# Whether to delete a blacklisted package from the database
|
# Whether to delete a blacklisted package from the database
|
||||||
REMOVE_BLACKLISTED_PACKAGE = true
|
REMOVE_BLACKLISTED_PACKAGE = true
|
||||||
|
|
||||||
|
# Postgres advisory lock used to serialize update_from_deb_repos runs.
|
||||||
|
# Without this, a scheduled and a manual run (or two scheduled runs) can
|
||||||
|
# overlap: both do find_by+insert for the same name, and the second one
|
||||||
|
# either violates packages_name_key (aborting the whole import) or - on
|
||||||
|
# instances that never had that constraint - silently creates a duplicate.
|
||||||
|
IMPORT_ADVISORY_LOCK_KEY = 0x0DE65507
|
||||||
|
|
||||||
# Whether to delete packages that were not found in any configured
|
# Whether to delete packages that were not found in any configured
|
||||||
# source during a full import run (e.g. packages Debian has dropped
|
# source during a full import run (e.g. packages Debian has dropped
|
||||||
# entirely, or ones from a suite/architecture no longer configured).
|
# entirely, or ones from a suite/architecture no longer configured).
|
||||||
|
|
@ -84,6 +91,11 @@ namespace :debshots do
|
||||||
Rails.logger.level = Logger::INFO
|
Rails.logger.level = Logger::INFO
|
||||||
#Rails.logger.level = Logger::DEBUG
|
#Rails.logger.level = Logger::DEBUG
|
||||||
|
|
||||||
|
unless import_lock_acquired?
|
||||||
|
Rails.logger.warn 'Another import run already in progress (advisory lock). Skipping.'
|
||||||
|
exit 0
|
||||||
|
end
|
||||||
|
|
||||||
Rails.logger.info "Importing Debian package information"
|
Rails.logger.info "Importing Debian package information"
|
||||||
|
|
||||||
repositories.each do |repository|
|
repositories.each do |repository|
|
||||||
|
|
@ -137,6 +149,7 @@ namespace :debshots do
|
||||||
next
|
next
|
||||||
end # if blacklisted
|
end # if blacklisted
|
||||||
|
|
||||||
|
begin
|
||||||
db_package = Package.find_by name: package[:Package]
|
db_package = Package.find_by name: package[:Package]
|
||||||
if db_package # package exists in the database
|
if db_package # package exists in the database
|
||||||
# check if the parsed package data has a newer version
|
# check if the parsed package data has a newer version
|
||||||
|
|
@ -156,6 +169,18 @@ namespace :debshots do
|
||||||
update_data(package, db_package)
|
update_data(package, db_package)
|
||||||
stats_added += 1
|
stats_added += 1
|
||||||
end
|
end
|
||||||
|
rescue ActiveRecord::RecordNotUnique
|
||||||
|
# Lost a create-race: another process inserted this package name
|
||||||
|
# between our find and our insert (a web upload, or an import on
|
||||||
|
# an instance without the advisory lock). Treat it as found and
|
||||||
|
# update instead of aborting the whole run.
|
||||||
|
Rails.logger.warn "Package '#{package[:Package]}' was created " \
|
||||||
|
'concurrently. Updating it instead.'
|
||||||
|
if (db_package = Package.find_by(name: package[:Package]))
|
||||||
|
update_data(package, db_package)
|
||||||
|
stats_updated += 1
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
# unless db_package
|
# unless db_package
|
||||||
# Rails.logger.debug "No such package in our database. Creating one."
|
# Rails.logger.debug "No such package in our database. Creating one."
|
||||||
|
|
@ -385,3 +410,14 @@ def remove_orphaned_packages(seen_names, sources_incomplete)
|
||||||
|
|
||||||
destroy_orphaned_packages(orphan_ids)
|
destroy_orphaned_packages(orphan_ids)
|
||||||
end
|
end
|
||||||
|
|
||||||
|
# Try to take the advisory lock guarding update_from_deb_repos. The lock lives
|
||||||
|
# on this database connection/session, so it is released automatically when the
|
||||||
|
# process exits - no explicit unlock needed. Returns true if this run is the
|
||||||
|
# one that may proceed.
|
||||||
|
def import_lock_acquired?
|
||||||
|
result = ActiveRecord::Base.connection.execute(
|
||||||
|
"SELECT pg_try_advisory_lock(#{IMPORT_ADVISORY_LOCK_KEY})"
|
||||||
|
)
|
||||||
|
result.first['pg_try_advisory_lock'] == true
|
||||||
|
end
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue