From c5cd77e9ee582ef866482c770924bf823b57ed08 Mon Sep 17 00:00:00 2001 From: Christoph Haas Date: Fri, 4 Sep 2026 21:57:35 +0200 Subject: [PATCH] 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. --- ..._add_unique_name_constraint_to_packages.rb | 28 ++++++++ lib/tasks/import_debian.rake | 64 +++++++++++++++---- 2 files changed, 78 insertions(+), 14 deletions(-) create mode 100644 db/migrate/20260904120000_add_unique_name_constraint_to_packages.rb diff --git a/db/migrate/20260904120000_add_unique_name_constraint_to_packages.rb b/db/migrate/20260904120000_add_unique_name_constraint_to_packages.rb new file mode 100644 index 0000000..89efc03 --- /dev/null +++ b/db/migrate/20260904120000_add_unique_name_constraint_to_packages.rb @@ -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 \ No newline at end of file diff --git a/lib/tasks/import_debian.rake b/lib/tasks/import_debian.rake index f545d39..5569c07 100644 --- a/lib/tasks/import_debian.rake +++ b/lib/tasks/import_debian.rake @@ -54,6 +54,13 @@ BLACKLIST_DESCRIPTION_PATTERN = [ # Whether to delete a blacklisted package from the database 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 # source during a full import run (e.g. packages Debian has dropped # 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::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" repositories.each do |repository| @@ -137,24 +149,37 @@ namespace :debshots do next end # if blacklisted - db_package = Package.find_by name: package[:Package] - if db_package # package exists in the database - # check if the parsed package data has a newer version - current_version = DebImporter::Version.new(db_package.version) - new_version = DebImporter::Version.new(package[:Version]) + begin + db_package = Package.find_by name: package[:Package] + if db_package # package exists in the database + # check if the parsed package data has a newer version + current_version = DebImporter::Version.new(db_package.version) + new_version = DebImporter::Version.new(package[:Version]) - Rails.logger.debug "Comparing package version: old=#{db_package.version} new=#{new_version}" - if new_version > current_version - # update the package information from new data + Rails.logger.debug "Comparing package version: old=#{db_package.version} new=#{new_version}" + if new_version > current_version + # update the package information from new data + update_data(package, db_package) + Rails.logger.info "Updating package information" + stats_updated += 1 + end + else # new package - create it in the database + Rails.logger.debug "No such package in our database. Creating one." + db_package = Package.new + update_data(package, db_package) + stats_added += 1 + 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) - Rails.logger.info "Updating package information" stats_updated += 1 end - else # new package - create it in the database - Rails.logger.debug "No such package in our database. Creating one." - db_package = Package.new - update_data(package, db_package) - stats_added += 1 end # unless db_package @@ -385,3 +410,14 @@ def remove_orphaned_packages(seen_names, sources_incomplete) destroy_orphaned_packages(orphan_ids) 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