From 8d6d55e41c4f233391bf9356699f3d31e974f728 Mon Sep 17 00:00:00 2001 From: yoldas Date: Thu, 12 Sep 2024 09:59:21 +0100 Subject: [PATCH 01/32] Add migration for unique-together index on ancestor and descendant --- ...2000109_add_unique_index_to_asset_links.rb | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 db/migrate/20240912000109_add_unique_index_to_asset_links.rb diff --git a/db/migrate/20240912000109_add_unique_index_to_asset_links.rb b/db/migrate/20240912000109_add_unique_index_to_asset_links.rb new file mode 100644 index 0000000000..631d249400 --- /dev/null +++ b/db/migrate/20240912000109_add_unique_index_to_asset_links.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +require 'csv' +# This migration adds a unique-together index on ancestor_id and descendant_id +# in order to prevent duplicate links between the same ancestor and descendant +# labware. +# +# Before this migration, the database allowed duplicate asset_links between the +# same ancestor and descendant labware. Therefore, the migration will fail if +# there are any duplicate links in the database. To fix this, they must be +# removed before running the migration using the rake task: +# +# bundle exec rake 'support:remove_duplicate_asset_links[csv_file_path]' +# +# The rake task will write the removed records into a CSV file that can be used +# for recovery if necessary. +# +# If the migration is rolled back, the index will be removed. The duplicate +# records removed before can be restored from a CSV using the rake task: +# +# bundle exec rake 'support:restore_removed_asset_links[csv_file_path]' +# +class AddUniqueIndexToAssetLinks < ActiveRecord::Migration[6.1] + def change + add_index :asset_links, [:ancestor_id, :descendant_id], unique: true, name: 'index_asset_links_on_ancestor_and_descendant' + end +end From 5896e32b06bda9e97b091be368a2624fcb303e4a Mon Sep 17 00:00:00 2001 From: yoldas Date: Thu, 12 Sep 2024 16:27:25 +0100 Subject: [PATCH 02/32] Applied migration to db schema --- db/migrate/20240912000109_add_unique_index_to_asset_links.rb | 5 ++++- db/schema.rb | 3 ++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/db/migrate/20240912000109_add_unique_index_to_asset_links.rb b/db/migrate/20240912000109_add_unique_index_to_asset_links.rb index 631d249400..09c3e418a2 100644 --- a/db/migrate/20240912000109_add_unique_index_to_asset_links.rb +++ b/db/migrate/20240912000109_add_unique_index_to_asset_links.rb @@ -22,6 +22,9 @@ # class AddUniqueIndexToAssetLinks < ActiveRecord::Migration[6.1] def change - add_index :asset_links, [:ancestor_id, :descendant_id], unique: true, name: 'index_asset_links_on_ancestor_and_descendant' + add_index :asset_links, + %i[ancestor_id descendant_id], + unique: true, + name: 'index_asset_links_on_ancestor_and_descendant' end end diff --git a/db/schema.rb b/db/schema.rb index 207c08263d..647e3fd66b 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.define(version: 2024_08_13_130010) do +ActiveRecord::Schema.define(version: 2024_09_12_000109) do create_table "aliquot_indices", id: :integer, charset: "utf8mb4", collation: "utf8mb4_unicode_ci", options: "ENGINE=InnoDB ROW_FORMAT=DYNAMIC", force: :cascade do |t| t.integer "aliquot_id", null: false @@ -115,6 +115,7 @@ t.integer "count" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.index ["ancestor_id", "descendant_id"], name: "index_asset_links_on_ancestor_and_descendant", unique: true t.index ["ancestor_id", "direct"], name: "index_asset_links_on_ancestor_id_and_direct" t.index ["descendant_id", "direct"], name: "index_asset_links_on_descendant_id_and_direct" end From ab1f587025415ae8f53978e8bdf5933fd8d049ac Mon Sep 17 00:00:00 2001 From: yoldas Date: Mon, 23 Sep 2024 02:46:48 +0100 Subject: [PATCH 03/32] Include column names in the unique index --- db/migrate/20240912000109_add_unique_index_to_asset_links.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/db/migrate/20240912000109_add_unique_index_to_asset_links.rb b/db/migrate/20240912000109_add_unique_index_to_asset_links.rb index 09c3e418a2..a5639500e5 100644 --- a/db/migrate/20240912000109_add_unique_index_to_asset_links.rb +++ b/db/migrate/20240912000109_add_unique_index_to_asset_links.rb @@ -20,11 +20,13 @@ # # bundle exec rake 'support:restore_removed_asset_links[csv_file_path]' # +# Note that the column names in the index name below is used for finding the +# reason of the database unique constraint violation by the AssetLink model. class AddUniqueIndexToAssetLinks < ActiveRecord::Migration[6.1] def change add_index :asset_links, %i[ancestor_id descendant_id], unique: true, - name: 'index_asset_links_on_ancestor_and_descendant' + name: 'index_asset_links_on_ancestor_id_and_descendant_id' end end From 775d85eecb334d0c90dcd5c177fb2a7c71d81e46 Mon Sep 17 00:00:00 2001 From: yoldas Date: Mon, 23 Sep 2024 02:56:00 +0100 Subject: [PATCH 04/32] Apply migration to schema --- db/schema.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/schema.rb b/db/schema.rb index 647e3fd66b..e5f088aa24 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -115,7 +115,7 @@ t.integer "count" t.datetime "created_at", null: false t.datetime "updated_at", null: false - t.index ["ancestor_id", "descendant_id"], name: "index_asset_links_on_ancestor_and_descendant", unique: true + t.index ["ancestor_id", "descendant_id"], name: "index_asset_links_on_ancestor_id_and_descendant_id", unique: true t.index ["ancestor_id", "direct"], name: "index_asset_links_on_ancestor_id_and_direct" t.index ["descendant_id", "direct"], name: "index_asset_links_on_descendant_id_and_direct" end From 1b0fb4e41008f281ae2416eb683610a21438af17 Mon Sep 17 00:00:00 2001 From: yoldas Date: Mon, 23 Sep 2024 02:59:26 +0100 Subject: [PATCH 05/32] Override create_edge on asset_links to handle race conditions --- app/models/asset_link.rb | 73 ++++ .../asset_links_race_conditions_spec.rb | 346 ++++++++++++++++++ 2 files changed, 419 insertions(+) create mode 100644 spec/models/asset_links_race_conditions_spec.rb diff --git a/app/models/asset_link.rb b/app/models/asset_link.rb index c3191408d7..d1c98adf90 100644 --- a/app/models/asset_link.rb +++ b/app/models/asset_link.rb @@ -76,4 +76,77 @@ def has_#{name}? end end end + + # Creates an edge between the ancestor and descendant nodes using save. + # + # This method first attempts to find an existing link between the ancestor + # and descendant. If no link is found, it builds a new edge and saves it. + # If a link is found, it makes the link an edge and saves it. + # + # This method is overridden to handle race conditions in finding an + # existing link and has_duplicates validation. It also assumes that there + # is a unique-together index on ancestor_id and descendant_id columns. + # + # @param ancestor [Dag::Standard::EndPoint] The ancestor node. + # @param descendant [Dag::Standard::EndPoint] The descendant node. + # @return [Boolean] Returns true if the edge is successfully created or + # already exists, false otherwise. + # @raise [ActiveRecord::RecordNotUnique] Raises an exception if the unique + # constraint violation does not involve the expected columns. + def self.create_edge(ancestor, descendant) + # Two processes try to find an existing link. + link = find_link(ancestor, descendant) + # Either or both may find no link and try to create a new edge. + if link.nil? + edge = build_edge(ancestor, descendant) + return true if save_edge_or_handle_error(edge) + # Losing process finds the edge created by the winning process. + link = find_link(ancestor, descendant) + end + + link.make_direct + link.changed? ? link.save : true + end + + # Saves the edge between the ancestor and descendant nodes or handles errors. + # + # @param edge [AssetLink] The edge object containing the errors. + # @return [Boolean] Returns true if the edge is successfully saved, false + # otherwise. + def self.save_edge_or_handle_error(edge) + begin + # Winning process successfully saves the edge (direct link). + return true if edge.save + # has_duplicate validation may see it for the losing process before + # hitting the DB. + return false unless unique_validation_error?(edge) # Bubble up. + edge.errors.clear # Clear all errors and use the existing link. + rescue ActiveRecord::RecordNotUnique => e + # Unique constraint violation is triggered for the losing process after + # hitting the DB. + raise unless unique_violation_error?(edge, e) # Bubble up. + end + false + end + + # Checks if the validation error includes a specific message indicating a + # unique link already exists. + # + # @param edge [AssetLink] The edge object containing the errors. + # @return [Boolean] Returns true if the errors include the message "Link + # already exists between these points", false otherwise. + def self.unique_validation_error?(edge) + edge.errors[:base].include?('Link already exists between these points') + end + + # Checks if the unique constraint violation involves the specified columns. + # + # @param edge [AssetLink] The edge object containing the column names. + # @param exception [ActiveRecord::RecordNotUnique] The exception raised due + # to the unique constraint violation. + # @return [Boolean] Returns true if the exception message includes both the + # ancestor and descendant column names, false otherwise. + def self.unique_violation_error?(edge, exception) + [edge.ancestor_id_column_name, edge.descendant_id_column_name].all? { |col| exception.message.include?(col) } + end end diff --git a/spec/models/asset_links_race_conditions_spec.rb b/spec/models/asset_links_race_conditions_spec.rb new file mode 100644 index 0000000000..f79465ac32 --- /dev/null +++ b/spec/models/asset_links_race_conditions_spec.rb @@ -0,0 +1,346 @@ +# frozen_string_literal: true + +require 'rails_helper' + +# Exception raised when a socket operation times out. +class SocketTimeoutError < StandardError +end + +# Exception raised when a process operation times out. +class ProcessTimeoutError < StandardError +end + +# Exception raised when a process exits with a non-zero status. +class ProcessStatusError < StandardError +end + +RSpec.describe AssetLink, type: :model do + describe '.create_edge' do + # Used in IPC when one end of the duplex pipe is waiting for the other end + # to send a message with a timeout in seconds. + # + # @param socket [Socket] The socket to wait for. + # @param length [Integer] The length of the message to wait for. + # @param timeout [Integer] The timeout in seconds, default is 10. + # @param message [String] The message to raise if times out, default is 'socket timeout'. + # @return [String] The message received from the socket. + # @raise SocketTimeoutError + + def wait_readable_with_timeout(socket, length, timeout = 10, message = 'socket timeout') + raise SocketTimeoutError, message unless socket.wait_readable(timeout) + socket.recv(length) + end + + # Used to reap a child process with a timeout in seconds. + # + # @param pid [Integer] The process ID to wait for. + # @param timeout [Integer] The timeout in seconds, default is 10. + # @param message [String] The message to raise if times out, default is 'process timeout'. + # @return [Process::Status] The status of the process. + # @raise ProcessTimeoutError + # rubocop:disable Metrics/MethodLength + def wait_process_with_timeout(pid, timeout = 10, message = 'process timeout') + start_time = Time.zone.now + loop do + begin + pid2, status = Process.waitpid2(pid, Process::WNOHANG) + return status if pid2 + rescue Errno::ECHILD # No child process + return nil + end + if Time.zone.now - start_time > timeout + begin + Process.kill('TERM', pid) # Send TERM signal. + sleep 1 # Wait for the process to terminate. + Process.kill('KILL', pid) # Send KILL signal. + Process.waitpid(pid) # Reap + rescue Errno::ECHILD + # No child process + end + raise ProcessTimeoutError, message + end + sleep 0.1 + end + end + # rubocop:enable Metrics/MethodLength + + # Wait for the given processes to finish with a timeout in seconds. + # @param pids [Array] The process IDs to wait for. + # @return [void] + def wait_for_processes(pids) + pids.each do |pid| + Process.waitpid(pid) + # status = wait_process_with_timeout(pid, 10, "Timeout waiting for process #{pid} to finish.") + # raise ProcessStatusError, + # "Forked process #{pid} failed with exit status #{status.exitstatus}" if status.exitstatus != 0 + end + end + + # rubocop:disable RSpec/ExampleLength + it 'handles race condition at find_link' do + # In this example, the first and second processes are forked from the + # main process and they are in race condition to find an existing link + # between the ancestor and descendant and create an edge if no link is + # found. Both first and second processes finds no link, but the second + # process creates the edge first. The first also tries to create the + # edge based on the result of the find_link method. However, it must + # not be able to create it. + + ActiveRecord::Base.connection.reconnect! + ancestor = create(:labware) + descendant = create(:labware) + ActiveRecord::Base.connection.commit_db_transaction + + # Create a duplex pipe for inter-process communication. + first_socket, second_socket = Socket.pair(:UNIX, :STREAM) + + # Fork the first process. + pid1 = + fork do + ActiveRecord::Base.connection.reconnect! + + call_count = 0 # Track the calls to the find_link method. + + # Patch the find_link method in the first process. + allow(described_class).to receive(:find_link).and_wrap_original do |method, *args| + call_count += 1 + link = method.call(*args) + if call_count == 1 + expect(link).to be_nil # The first process should not find any link initally. + + signal = 'paused' + first_socket.send(signal, 0) # Signal that the first process is paused now. Zero flags. + + # Wait for the second process to send 'resume'. + wait_readable_with_timeout( + first_socket, + 'resume'.length, + 10, + 'Timeout waiting for the second process to send resume.' + ) + elsif call_count == 2 + # The first process now should find the link created by the second process. + expect(link).not_to be_nil + end + link + end + + described_class.create_edge(ancestor, descendant) + ActiveRecord::Base.connection.close + end + + # Fork the second process. + pid2 = + fork do + ActiveRecord::Base.connection.reconnect! # Reconnect to the database in the forked process. + + # Wait for the first process to signal that it is paused after calling find_link. + wait_readable_with_timeout( + second_socket, + 'paused'.length, + 10, + 'Timeout waiting for the first process to send paused.' + ) + + described_class.create_edge(ancestor, descendant) + # Signal the first process to resume now. Although it has found no link + # in its last method call, actually there is one now. + signal = 'resume' + second_socket.send(signal, 0) # Zero flags. + ActiveRecord::Base.connection.close + end + + # Wait for both processes to finish and check their exit statuses + wait_for_processes([pid1, pid2]) + + # Check that the edge was created as expected. + expect(described_class.where(ancestor: ancestor, descendant: descendant).count).to eq(1) + link = described_class.last + expect(link.ancestor).to eq(ancestor) + expect(link.descendant).to eq(descendant) + expect(link.direct).to be_truthy + expect(link.count).to eq(1) + + ActiveRecord::Base.connection.close + end + # rubocop:enable RSpec/ExampleLength + + # rubocop:disable RSpec/ExampleLength + it 'handles race condition at has_duplicates' do + # In this example, the first and second processes are forked from the + # main process. Neither of them finds and existing link between the + # ancestor and descendant. Both try to create the edge. One of them + # succeeds and the other one fails due to the has_duplicates validation. + # Failing process should use the existing link. + + ActiveRecord::Base.connection.reconnect! + ancestor = create(:labware) + descendant = create(:labware) + ActiveRecord::Base.connection.commit_db_transaction + + # Create a duplex pipe for inter-process communication. + first_socket, second_socket = Socket.pair(:UNIX, :STREAM) + + # Fork the first process. + pid1 = + fork do + ActiveRecord::Base.connection.reconnect! + call_count = 0 # Track the calls to the find_link method. + # Patch the find_link method in the first process. + # Check link, trigger the second process, pause, and then wait for'resume' signal. + allow(described_class).to receive(:find_link).and_wrap_original do |method, *args| + call_count += 1 + link = method.call(*args) + if call_count == 1 + expect(link).to be_nil # The first process should not find any link initally. + signal = 'paused' + first_socket.send(signal, 0) # Signal that the first process is paused now. Zero flags. + # Wait for the second process to send 'resume'. + wait_readable_with_timeout( + first_socket, + 'resume'.length, + 10, + 'Timeout waiting for the second process to send resume.' + ) + end + link + end + # Now the first process should be prevented from saving because of has_duplicates validation. + result = described_class.create_edge(ancestor, descendant) + expect(result).to be_truthy + expect(described_class).to have_received(:unique_validation_error?) + + ActiveRecord::Base.connection.close + end + + # Fork the second process. + pid2 = + fork do + ActiveRecord::Base.connection.reconnect! + # Wait for the first process to signal that it is paused after calling find_link. + wait_readable_with_timeout( + second_socket, + 'paused'.length, + 10, + 'Timeout waiting for the first process to send paused.' + ) + described_class.create_edge(ancestor, descendant) + signal = 'resume' + second_socket.send(signal, 0) # Zero flags. + ActiveRecord::Base.connection.close + end + + # Wait for both processes to finish and check their exit statuses + wait_for_processes([pid1, pid2]) + + # Check that the edge was created as expected. + expect(described_class.where(ancestor: ancestor, descendant: descendant).count).to eq(1) + link = described_class.last + expect(link.ancestor).to eq(ancestor) + expect(link.descendant).to eq(descendant) + expect(link.direct).to be_truthy + expect(link.count).to eq(1) # How many different paths between the nodes. + end + # rubocop:enable RSpec/ExampleLength + + # rubocop:disable RSpec/ExampleLength + it 'handles unique constraint violation' do + # In this example, the first and second processes are forked from the + # main process. Neither of them finds and existing link between the + # ancestor and descendant. Both try to create the edge. One of them + # succeeds and the other one fails due to the unique constraint + # violation. Failing process should use the existing link. + + ActiveRecord::Base.connection.reconnect! + ancestor = create(:labware) + descendant = create(:labware) + ActiveRecord::Base.connection.commit_db_transaction + + # Create a duplex pipe for inter-process communication. + first_socket, second_socket = Socket.pair(:UNIX, :STREAM) + + # Fork the first process. + pid1 = + fork do + ActiveRecord::Base.connection.reconnect! + call_count = 0 # Track the calls to the find_link method. + # Patch the find_link method in the first process. + # Check link, trigger the second process, pause, and then wait for'resume' signal. + allow(described_class).to receive(:find_link).and_wrap_original do |method, *args| + call_count += 1 + link = method.call(*args) + if call_count == 1 + expect(link).to be_nil # The first process should not find any link initally. + signal = 'paused' + first_socket.send(signal, 0) # Signal that the first process is paused now. Zero flags. + # Wait for the second process to send 'resume'. + wait_readable_with_timeout( + first_socket, + 'resume'.length, + 10, + 'Timeout waiting for the second process to send resume.' + ) + end + link + end + # Validation error is not triggered in this case because the other + # process has not saved the link yet. Therefore, the save call will + # pass the validations and hit the database to trigger the unique + # constraint violation just after the other process saves the link. + # rubocop:disable RSpec/AnyInstance + allow_any_instance_of(Dag::CreateCorrectnessValidator).to receive(:has_duplicates).and_return(false) + # rubocop:enable RSpec/AnyInstance + + allow(described_class).to receive(:save_edge_or_handle_error).and_wrap_original do |method, *args| + edge = args[0] + message = + "Duplicate entry '#{ancestor.id}-#{descendant.id}' " \ + "for key 'index_asset_links_on_ancestor_id_and_descendant_id'" + exception = ActiveRecord::RecordNotUnique.new(message) + allow(edge).to receive(:save).and_raise(exception) + + method.call(*args) + end + + described_class.create_edge(ancestor, descendant) + expect(described_class).to have_received(:unique_violation_error?) + ActiveRecord::Base.connection.close + end + + # Fork the second process. + pid2 = + fork do + ActiveRecord::Base.connection.reconnect! # Reconnect to the database in the forked process. + + # Wait for the first process to signal that it is paused after calling find_link. + wait_readable_with_timeout( + second_socket, + 'paused'.length, + 10, + 'Timeout waiting for the first process to send paused.' + ) + + described_class.create_edge(ancestor, descendant) + # Signal the first process to resume now. Although it has found no link + # in its last method call, actually there is one now. + signal = 'resume' + second_socket.send(signal, 0) # Zero flags. + ActiveRecord::Base.connection.close + end + + # Wait for both processes to finish and check their exit statuses + wait_for_processes([pid1, pid2]) + + # Check that the edge was created as expected. + expect(described_class.where(ancestor: ancestor, descendant: descendant).count).to eq(1) + link = described_class.last + expect(link.ancestor).to eq(ancestor) + expect(link.descendant).to eq(descendant) + expect(link.direct).to be_truthy + expect(link.count).to eq(1) + + ActiveRecord::Base.connection.close + end + # rubocop:enable RSpec/ExampleLength + end +end From d720c8e9b4f24721ad8532a37fdc04262b10f674 Mon Sep 17 00:00:00 2001 From: yoldas Date: Mon, 23 Sep 2024 18:20:48 +0100 Subject: [PATCH 06/32] Bubble up non-duplication errors --- app/models/asset_link.rb | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/app/models/asset_link.rb b/app/models/asset_link.rb index d1c98adf90..dd175b6468 100644 --- a/app/models/asset_link.rb +++ b/app/models/asset_link.rb @@ -99,7 +99,8 @@ def self.create_edge(ancestor, descendant) # Either or both may find no link and try to create a new edge. if link.nil? edge = build_edge(ancestor, descendant) - return true if save_edge_or_handle_error(edge) + result = save_edge_or_handle_error(edge) + return result unless result.nil? # Bubble up. # Losing process finds the edge created by the winning process. link = find_link(ancestor, descendant) end @@ -114,19 +115,16 @@ def self.create_edge(ancestor, descendant) # @return [Boolean] Returns true if the edge is successfully saved, false # otherwise. def self.save_edge_or_handle_error(edge) - begin - # Winning process successfully saves the edge (direct link). - return true if edge.save - # has_duplicate validation may see it for the losing process before - # hitting the DB. - return false unless unique_validation_error?(edge) # Bubble up. - edge.errors.clear # Clear all errors and use the existing link. - rescue ActiveRecord::RecordNotUnique => e - # Unique constraint violation is triggered for the losing process after - # hitting the DB. - raise unless unique_violation_error?(edge, e) # Bubble up. - end - false + # Winning process successfully saves the edge (direct link). + return true if edge.save + # has_duplicate validation may see it for the losing process before + # hitting the DB. + return false unless unique_validation_error?(edge) # Bubble up. + edge.errors.clear # Clear all errors and use the existing link. + rescue ActiveRecord::RecordNotUnique => e + # Unique constraint violation is triggered for the losing process after + # hitting the DB. + raise unless unique_violation_error?(edge, e) # Bubble up. end # Checks if the validation error includes a specific message indicating a From 6b9b83d3530e00a5cc928e11fe37968cbfbfe010 Mon Sep 17 00:00:00 2001 From: yoldas Date: Mon, 23 Sep 2024 21:14:27 +0100 Subject: [PATCH 07/32] Remove custom exception class definitions from test --- .../asset_links_race_conditions_spec.rb | 36 ++++++++----------- 1 file changed, 15 insertions(+), 21 deletions(-) diff --git a/spec/models/asset_links_race_conditions_spec.rb b/spec/models/asset_links_race_conditions_spec.rb index f79465ac32..5f96890a3a 100644 --- a/spec/models/asset_links_race_conditions_spec.rb +++ b/spec/models/asset_links_race_conditions_spec.rb @@ -2,20 +2,10 @@ require 'rails_helper' -# Exception raised when a socket operation times out. -class SocketTimeoutError < StandardError -end - -# Exception raised when a process operation times out. -class ProcessTimeoutError < StandardError -end - -# Exception raised when a process exits with a non-zero status. -class ProcessStatusError < StandardError -end - RSpec.describe AssetLink, type: :model do describe '.create_edge' do + # Helpers + # Used in IPC when one end of the duplex pipe is waiting for the other end # to send a message with a timeout in seconds. # @@ -24,10 +14,9 @@ class ProcessStatusError < StandardError # @param timeout [Integer] The timeout in seconds, default is 10. # @param message [String] The message to raise if times out, default is 'socket timeout'. # @return [String] The message received from the socket. - # @raise SocketTimeoutError - + # @raise StandardError def wait_readable_with_timeout(socket, length, timeout = 10, message = 'socket timeout') - raise SocketTimeoutError, message unless socket.wait_readable(timeout) + raise StandardError, message unless socket.wait_readable(timeout) socket.recv(length) end @@ -37,7 +26,7 @@ def wait_readable_with_timeout(socket, length, timeout = 10, message = 'socket t # @param timeout [Integer] The timeout in seconds, default is 10. # @param message [String] The message to raise if times out, default is 'process timeout'. # @return [Process::Status] The status of the process. - # @raise ProcessTimeoutError + # @raise StandardError # rubocop:disable Metrics/MethodLength def wait_process_with_timeout(pid, timeout = 10, message = 'process timeout') start_time = Time.zone.now @@ -57,7 +46,7 @@ def wait_process_with_timeout(pid, timeout = 10, message = 'process timeout') rescue Errno::ECHILD # No child process end - raise ProcessTimeoutError, message + raise StandardError, message end sleep 0.1 end @@ -69,13 +58,15 @@ def wait_process_with_timeout(pid, timeout = 10, message = 'process timeout') # @return [void] def wait_for_processes(pids) pids.each do |pid| - Process.waitpid(pid) - # status = wait_process_with_timeout(pid, 10, "Timeout waiting for process #{pid} to finish.") - # raise ProcessStatusError, - # "Forked process #{pid} failed with exit status #{status.exitstatus}" if status.exitstatus != 0 + status = wait_process_with_timeout(pid, 10, "Timeout waiting for process #{pid} to finish.") + if status.exitstatus != 0 + raise StandardError, "Forked process #{pid} failed with exit status #{status.exitstatus}" + end end end + # Examples + # rubocop:disable RSpec/ExampleLength it 'handles race condition at find_link' do # In this example, the first and second processes are forked from the @@ -205,6 +196,8 @@ def wait_for_processes(pids) end link end + allow(described_class).to receive(:unique_validation_error?).and_call_original + # Now the first process should be prevented from saving because of has_duplicates validation. result = described_class.create_edge(ancestor, descendant) expect(result).to be_truthy @@ -301,6 +294,7 @@ def wait_for_processes(pids) method.call(*args) end + allow(described_class).to receive(:unique_violation_error?).and_call_original described_class.create_edge(ancestor, descendant) expect(described_class).to have_received(:unique_violation_error?) From 480e3d368437428faa5967f9b305aadecaa079f7 Mon Sep 17 00:00:00 2001 From: yoldas Date: Mon, 23 Sep 2024 21:28:31 +0100 Subject: [PATCH 08/32] Update comment about return values --- app/models/asset_link.rb | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/app/models/asset_link.rb b/app/models/asset_link.rb index dd175b6468..1def9ab0c4 100644 --- a/app/models/asset_link.rb +++ b/app/models/asset_link.rb @@ -91,8 +91,9 @@ def has_#{name}? # @param descendant [Dag::Standard::EndPoint] The descendant node. # @return [Boolean] Returns true if the edge is successfully created or # already exists, false otherwise. - # @raise [ActiveRecord::RecordNotUnique] Raises an exception if the unique - # constraint violation does not involve the expected columns. + # @raise [ActiveRecord::RecordNotUnique] Re-raises any exception if it is + # not a constraint violation that involves ancestor_id and descendant_id + # columns. def self.create_edge(ancestor, descendant) # Two processes try to find an existing link. link = find_link(ancestor, descendant) @@ -112,8 +113,11 @@ def self.create_edge(ancestor, descendant) # Saves the edge between the ancestor and descendant nodes or handles errors. # # @param edge [AssetLink] The edge object containing the errors. - # @return [Boolean] Returns true if the edge is successfully saved, false - # otherwise. + # @return [Boolean] Returns true if the edge is successfully saved, + # nil if the error is unique validation or constraint violation, + # false if the error is another validation error. + # @raise [ActiveRecord::RecordNotUnique] Re-raises an exception if the + # exception caught is not a unique constraint violation. def self.save_edge_or_handle_error(edge) # Winning process successfully saves the edge (direct link). return true if edge.save From dc6a704698d0d89489e80eaa681d062548eb8a35 Mon Sep 17 00:00:00 2001 From: yoldas Date: Tue, 24 Sep 2024 18:20:51 +0100 Subject: [PATCH 09/32] Add guard against nil before converting link to edge --- app/models/asset_link.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/app/models/asset_link.rb b/app/models/asset_link.rb index 1def9ab0c4..8267b761f6 100644 --- a/app/models/asset_link.rb +++ b/app/models/asset_link.rb @@ -106,8 +106,10 @@ def self.create_edge(ancestor, descendant) link = find_link(ancestor, descendant) end - link.make_direct - link.changed? ? link.save : true + unless link.nil? + link.make_direct + link.changed? ? link.save : true + end end # Saves the edge between the ancestor and descendant nodes or handles errors. From 80d377ad34bcba93b783fe9e3589711ee753b318 Mon Sep 17 00:00:00 2001 From: yoldas Date: Tue, 24 Sep 2024 18:26:14 +0100 Subject: [PATCH 10/32] Replace the recover proecedure with auditing as it is wrong to put duplicate records back into the table. --- .../20240912000109_add_unique_index_to_asset_links.rb | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/db/migrate/20240912000109_add_unique_index_to_asset_links.rb b/db/migrate/20240912000109_add_unique_index_to_asset_links.rb index a5639500e5..13a495bbaa 100644 --- a/db/migrate/20240912000109_add_unique_index_to_asset_links.rb +++ b/db/migrate/20240912000109_add_unique_index_to_asset_links.rb @@ -13,12 +13,7 @@ # bundle exec rake 'support:remove_duplicate_asset_links[csv_file_path]' # # The rake task will write the removed records into a CSV file that can be used -# for recovery if necessary. -# -# If the migration is rolled back, the index will be removed. The duplicate -# records removed before can be restored from a CSV using the rake task: -# -# bundle exec rake 'support:restore_removed_asset_links[csv_file_path]' +# for auditing purposes. # # Note that the column names in the index name below is used for finding the # reason of the database unique constraint violation by the AssetLink model. From bac2c98d498e5a1954abbd24301b014120aca34b Mon Sep 17 00:00:00 2001 From: yoldas Date: Tue, 24 Sep 2024 18:29:41 +0100 Subject: [PATCH 11/32] Rubocop --- app/models/asset_link.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/models/asset_link.rb b/app/models/asset_link.rb index 8267b761f6..837d25080d 100644 --- a/app/models/asset_link.rb +++ b/app/models/asset_link.rb @@ -106,10 +106,10 @@ def self.create_edge(ancestor, descendant) link = find_link(ancestor, descendant) end - unless link.nil? - link.make_direct - link.changed? ? link.save : true - end + return if link.nil? + + link.make_direct + link.changed? ? link.save : true end # Saves the edge between the ancestor and descendant nodes or handles errors. From 03124f48d84171c9f36903c3a5e878ae84b4e185 Mon Sep 17 00:00:00 2001 From: yoldas Date: Tue, 24 Sep 2024 19:07:09 +0100 Subject: [PATCH 12/32] Clean the database after tests because of the new DB connections in the main and forked child processes --- spec/models/asset_links_race_conditions_spec.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/spec/models/asset_links_race_conditions_spec.rb b/spec/models/asset_links_race_conditions_spec.rb index 5f96890a3a..b208c639c8 100644 --- a/spec/models/asset_links_race_conditions_spec.rb +++ b/spec/models/asset_links_race_conditions_spec.rb @@ -67,6 +67,11 @@ def wait_for_processes(pids) # Examples + # We need to explicitly clean the database after examples here because we + # are establishing new database connections in the main and forked child + # processes. + after { DatabaseCleaner.clean_with(:truncation) } + # rubocop:disable RSpec/ExampleLength it 'handles race condition at find_link' do # In this example, the first and second processes are forked from the From dee9a7e549ede3a6ba397bf29e209002ba584236 Mon Sep 17 00:00:00 2001 From: yoldas Date: Tue, 24 Sep 2024 20:42:26 +0100 Subject: [PATCH 13/32] Removed after block --- spec/models/asset_links_race_conditions_spec.rb | 5 ----- 1 file changed, 5 deletions(-) diff --git a/spec/models/asset_links_race_conditions_spec.rb b/spec/models/asset_links_race_conditions_spec.rb index b208c639c8..5f96890a3a 100644 --- a/spec/models/asset_links_race_conditions_spec.rb +++ b/spec/models/asset_links_race_conditions_spec.rb @@ -67,11 +67,6 @@ def wait_for_processes(pids) # Examples - # We need to explicitly clean the database after examples here because we - # are establishing new database connections in the main and forked child - # processes. - after { DatabaseCleaner.clean_with(:truncation) } - # rubocop:disable RSpec/ExampleLength it 'handles race condition at find_link' do # In this example, the first and second processes are forked from the From c4a13a86f43ccbeea0086eecb0535560b7dfd455 Mon Sep 17 00:00:00 2001 From: yoldas Date: Tue, 24 Sep 2024 20:54:18 +0100 Subject: [PATCH 14/32] Add after all cleanup --- spec/models/asset_links_race_conditions_spec.rb | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/spec/models/asset_links_race_conditions_spec.rb b/spec/models/asset_links_race_conditions_spec.rb index 5f96890a3a..1513b305fc 100644 --- a/spec/models/asset_links_race_conditions_spec.rb +++ b/spec/models/asset_links_race_conditions_spec.rb @@ -67,6 +67,15 @@ def wait_for_processes(pids) # Examples + # We need to explicitly clean the database after examples here because we + # are establishing new database connections in the main and forked child + # processes. + # rubocop:disable RSpec/BeforeAfterAll + after(:all) do + DatabaseCleaner.clean_with(:truncation) + end + # rubocop:enable RSpec/BeforeAfterAll + # rubocop:disable RSpec/ExampleLength it 'handles race condition at find_link' do # In this example, the first and second processes are forked from the From 165dd5eeeb1b911332a8accc135a91c6ccd86331 Mon Sep 17 00:00:00 2001 From: yoldas Date: Tue, 24 Sep 2024 21:15:31 +0100 Subject: [PATCH 15/32] Delete test asset link and labware records --- spec/models/asset_links_race_conditions_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/models/asset_links_race_conditions_spec.rb b/spec/models/asset_links_race_conditions_spec.rb index 1513b305fc..3604f812e0 100644 --- a/spec/models/asset_links_race_conditions_spec.rb +++ b/spec/models/asset_links_race_conditions_spec.rb @@ -72,7 +72,8 @@ def wait_for_processes(pids) # processes. # rubocop:disable RSpec/BeforeAfterAll after(:all) do - DatabaseCleaner.clean_with(:truncation) + AssetLink.delete_all + Labware.delete_all end # rubocop:enable RSpec/BeforeAfterAll From 6f50b2c37938da7df0550bafc254ee83ee1777c8 Mon Sep 17 00:00:00 2001 From: yoldas Date: Tue, 24 Sep 2024 21:33:06 +0100 Subject: [PATCH 16/32] Debug side effect --- spec/models/asset_links_race_conditions_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/asset_links_race_conditions_spec.rb b/spec/models/asset_links_race_conditions_spec.rb index 3604f812e0..cc804dcce8 100644 --- a/spec/models/asset_links_race_conditions_spec.rb +++ b/spec/models/asset_links_race_conditions_spec.rb @@ -2,7 +2,7 @@ require 'rails_helper' -RSpec.describe AssetLink, type: :model do +RSpec.xdescribe AssetLink, type: :model do describe '.create_edge' do # Helpers From f04d1cf2210fbd8049aaa0d42ddc1c8d5cc8998a Mon Sep 17 00:00:00 2001 From: yoldas Date: Tue, 24 Sep 2024 21:45:47 +0100 Subject: [PATCH 17/32] Debug 2 --- app/models/asset_link.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/asset_link.rb b/app/models/asset_link.rb index 837d25080d..5f75a4fc28 100644 --- a/app/models/asset_link.rb +++ b/app/models/asset_link.rb @@ -94,7 +94,7 @@ def has_#{name}? # @raise [ActiveRecord::RecordNotUnique] Re-raises any exception if it is # not a constraint violation that involves ancestor_id and descendant_id # columns. - def self.create_edge(ancestor, descendant) + def self.xcreate_edge(ancestor, descendant) # Two processes try to find an existing link. link = find_link(ancestor, descendant) # Either or both may find no link and try to create a new edge. From bc857a28f633703deec1b708cf52df48ddf7fef5 Mon Sep 17 00:00:00 2001 From: yoldas Date: Tue, 24 Sep 2024 22:04:47 +0100 Subject: [PATCH 18/32] Revert debug --- app/models/asset_link.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/asset_link.rb b/app/models/asset_link.rb index 5f75a4fc28..837d25080d 100644 --- a/app/models/asset_link.rb +++ b/app/models/asset_link.rb @@ -94,7 +94,7 @@ def has_#{name}? # @raise [ActiveRecord::RecordNotUnique] Re-raises any exception if it is # not a constraint violation that involves ancestor_id and descendant_id # columns. - def self.xcreate_edge(ancestor, descendant) + def self.create_edge(ancestor, descendant) # Two processes try to find an existing link. link = find_link(ancestor, descendant) # Either or both may find no link and try to create a new edge. From 0aec8882965d0b6cb858492f937349d6dc64a65b Mon Sep 17 00:00:00 2001 From: yoldas Date: Tue, 24 Sep 2024 23:55:47 +0100 Subject: [PATCH 19/32] Debug handles race condition at find_link --- spec/models/asset_links_create_edge_spec.rb | 79 +++++++++++++++++++++ 1 file changed, 79 insertions(+) create mode 100644 spec/models/asset_links_create_edge_spec.rb diff --git a/spec/models/asset_links_create_edge_spec.rb b/spec/models/asset_links_create_edge_spec.rb new file mode 100644 index 0000000000..a432a6a54c --- /dev/null +++ b/spec/models/asset_links_create_edge_spec.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe AssetLink, type: :model do + describe '.create_edge' do + # rubocop:disable RSpec/ExampleLength + it 'handles race condition at find_link' do + # Parent + ActiveRecord::Base.connection.reconnect! + ancestor = create(:labware) + descendant = create(:labware) + ActiveRecord::Base.connection.commit_db_transaction + + first_socket, second_socket = UNIXSocket.pair + + # First child + pid1 = + fork do + ActiveRecord::Base.connection.reconnect! + ancestor = Labware.find(ancestor.id) + descendant = Labware.find(descendant.id) + find_link_call_count = 0 + allow(described_class).to receive(:find_link).and_wrap_original do |m, *args| + find_link_call_count += 1 + link = m.call(*args) + if find_link_call_count == 1 + expect(link).to be_nil + message = 'paused' + first_socket.send(message, 0) + message = 'child1: Timeout waiting for resume message' + raise StandardError, message unless first_socket.wait_readable(10) + message = 'resume' + first_socket.recv(message.size) + elsif find_link_call_count == 2 + expect(link).not_to be_nil + end + link + end + described_class.create_edge(ancestor, descendant) + ActiveRecord::Base.connection.close + end + + # Second child + pid2 = + fork do + ActiveRecord::Base.connection.reconnect! + ancestor = Labware.find(ancestor.id) + descendant = Labware.find(descendant.id) + message = 'child2: Timeout waiting for paused message' + raise StandardError, message unless second_socket.wait_readable(10) + message = 'paused' + second_socket.recv(message.size) + described_class.create_edge(ancestor, descendant) + message = 'resume' + second_socket.send(message, 0) + ActiveRecord::Base.connection.close + end + + # Parent + [pid1, pid2].each + .with_index(1) do |pid, index| + Timeout.timeout(10) { Process.waitpid(pid) } + rescue Timeout::Error + raise StandardError, "parent: Timeout waiting for child#{index} to finish" + end + + expect(described_class.where(ancestor: ancestor, descendant: descendant).count).to eq(1) + link = described_class.last + expect(link.ancestor).to eq(ancestor) + expect(link.descendant).to eq(descendant) + expect(link.direct).to be_truthy + expect(link.count).to eq(1) + + ActiveRecord::Base.connection.close + end + # rubocop:enable RSpec/ExampleLength + end +end From a69265a32a5ccd364593d8fabd40df1e75a013cf Mon Sep 17 00:00:00 2001 From: yoldas Date: Wed, 25 Sep 2024 00:18:18 +0100 Subject: [PATCH 20/32] Debug2 handles race condition at find_link --- spec/models/asset_links_create_edge_spec.rb | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/spec/models/asset_links_create_edge_spec.rb b/spec/models/asset_links_create_edge_spec.rb index a432a6a54c..7ba45792c7 100644 --- a/spec/models/asset_links_create_edge_spec.rb +++ b/spec/models/asset_links_create_edge_spec.rb @@ -4,12 +4,18 @@ RSpec.describe AssetLink, type: :model do describe '.create_edge' do + + after do + @ancestor.destroy if @ancestor + @descendant.destroy if @descendant + end + # rubocop:disable RSpec/ExampleLength it 'handles race condition at find_link' do # Parent ActiveRecord::Base.connection.reconnect! - ancestor = create(:labware) - descendant = create(:labware) + @ancestor = ancestor = create(:labware) + @descendant = descendant = create(:labware) ActiveRecord::Base.connection.commit_db_transaction first_socket, second_socket = UNIXSocket.pair From 4ebd702b5a0f35588f642bcafd400eb0d69e3547 Mon Sep 17 00:00:00 2001 From: yoldas Date: Wed, 25 Sep 2024 01:31:39 +0100 Subject: [PATCH 21/32] Rewrite test for unique validation error --- spec/models/asset_links_create_edge_spec.rb | 111 +++++++++++++++++--- 1 file changed, 96 insertions(+), 15 deletions(-) diff --git a/spec/models/asset_links_create_edge_spec.rb b/spec/models/asset_links_create_edge_spec.rb index 7ba45792c7..c94cb2689f 100644 --- a/spec/models/asset_links_create_edge_spec.rb +++ b/spec/models/asset_links_create_edge_spec.rb @@ -3,14 +3,14 @@ require 'rails_helper' RSpec.describe AssetLink, type: :model do + # rubocop:disable RSpec/ExampleLength,RSpec/InstanceVariable describe '.create_edge' do - after do - @ancestor.destroy if @ancestor - @descendant.destroy if @descendant + @ancestor&.destroy + @descendant&.destroy + @edge&.destroy end - # rubocop:disable RSpec/ExampleLength it 'handles race condition at find_link' do # Parent ActiveRecord::Base.connection.reconnect! @@ -24,8 +24,6 @@ pid1 = fork do ActiveRecord::Base.connection.reconnect! - ancestor = Labware.find(ancestor.id) - descendant = Labware.find(descendant.id) find_link_call_count = 0 allow(described_class).to receive(:find_link).and_wrap_original do |m, *args| find_link_call_count += 1 @@ -44,6 +42,7 @@ link end described_class.create_edge(ancestor, descendant) + ActiveRecord::Base.connection.close end @@ -51,8 +50,6 @@ pid2 = fork do ActiveRecord::Base.connection.reconnect! - ancestor = Labware.find(ancestor.id) - descendant = Labware.find(descendant.id) message = 'child2: Timeout waiting for paused message' raise StandardError, message unless second_socket.wait_readable(10) message = 'paused' @@ -64,22 +61,106 @@ end # Parent + exits = [] [pid1, pid2].each .with_index(1) do |pid, index| - Timeout.timeout(10) { Process.waitpid(pid) } + Timeout.timeout(10) do + _pid, status = Process.wait2(pid) + exits << status.exitstatus + end rescue Timeout::Error raise StandardError, "parent: Timeout waiting for child#{index} to finish" end + expect(exits).to eq([0, 0]) + expect(described_class.where(ancestor: ancestor, descendant: descendant).count).to eq(1) - link = described_class.last - expect(link.ancestor).to eq(ancestor) - expect(link.descendant).to eq(descendant) - expect(link.direct).to be_truthy - expect(link.count).to eq(1) + @edge = edge = described_class.last + expect(edge.ancestor).to eq(ancestor) + expect(edge.descendant).to eq(descendant) + expect(edge.direct).to be_truthy + expect(edge.count).to eq(1) ActiveRecord::Base.connection.close end - # rubocop:enable RSpec/ExampleLength + + it 'handles unique validation error' do + # Parent + ActiveRecord::Base.connection.reconnect! + @ancestor = ancestor = create(:labware) + @descendant = descendant = create(:labware) + ActiveRecord::Base.connection.commit_db_transaction + + first_socket, second_socket = UNIXSocket.pair + + # First child + pid1 = + fork do + ActiveRecord::Base.connection.reconnect! + find_link_call_count = 0 + allow(described_class).to receive(:find_link).and_wrap_original do |m, *args| + find_link_call_count += 1 + link = m.call(*args) + if find_link_call_count == 1 + expect(link).to be_nil + message = 'paused' + first_socket.send(message, 0) + message = 'child1: Timeout waiting for resume message' + raise StandardError, message unless first_socket.wait_readable(10) + message = 'resume' + first_socket.recv(message.size) + end + link + end + + unique_validation_error_return_value = nil + allow(described_class).to receive(:unique_validation_error?).and_wrap_original do |m, *args| + unique_validation_error_return_value = m.call(*args) + end + + result = described_class.create_edge(ancestor, descendant) + expect(result).to be_truthy + expect(described_class).to have_received(:unique_validation_error?) + expect(unique_validation_error_return_value).to be_truthy + + ActiveRecord::Base.connection.close + end + + # Second child + pid2 = + fork do + ActiveRecord::Base.connection.reconnect! + message = 'child2: Timeout waiting for paused message' + raise StandardError, message unless second_socket.wait_readable(10) + message = 'paused' + second_socket.recv(message.size) + described_class.create_edge(ancestor, descendant) + message = 'resume' + second_socket.send(message, 0) + ActiveRecord::Base.connection.close + end + + # Parent + exits = [] + [pid1, pid2].each + .with_index(1) do |pid, index| + Timeout.timeout(10) do + _pid, status = Process.wait2(pid) + exits << status.exitstatus + end + rescue Timeout::Error + raise StandardError, "parent: Timeout waiting for child#{index} to finish" + end + + expect(exits).to eq([0, 0]) + + expect(described_class.where(ancestor: ancestor, descendant: descendant).count).to eq(1) + @edge = edge = described_class.last + expect(edge.ancestor).to eq(ancestor) + expect(edge.descendant).to eq(descendant) + expect(edge.direct).to be_truthy + expect(edge.count).to eq(1) + end end + # rubocop:enable RSpec/ExampleLength,RSpec/InstanceVariable end From 9ea98ae8f0e609ccb027330e42b3d50f5a925990 Mon Sep 17 00:00:00 2001 From: yoldas Date: Wed, 25 Sep 2024 02:12:38 +0100 Subject: [PATCH 22/32] Rewrite example handles unique constraint violation --- spec/models/asset_links_create_edge_spec.rb | 120 ++++++++++++++++---- 1 file changed, 99 insertions(+), 21 deletions(-) diff --git a/spec/models/asset_links_create_edge_spec.rb b/spec/models/asset_links_create_edge_spec.rb index c94cb2689f..56be4a5d70 100644 --- a/spec/models/asset_links_create_edge_spec.rb +++ b/spec/models/asset_links_create_edge_spec.rb @@ -5,6 +5,22 @@ RSpec.describe AssetLink, type: :model do # rubocop:disable RSpec/ExampleLength,RSpec/InstanceVariable describe '.create_edge' do + def wait_for_child_processes(pids) + exits = [] + pids + .each + .with_index(1) do |pid, index| + Timeout.timeout(10) do + _pid, status = Process.wait2(pid) + exits << status.exitstatus + end + rescue Timeout::Error + raise StandardError, "parent: Timeout waiting for child#{index} to finish" + end + + expect(exits).to eq([0, 0]) + end + after do @ancestor&.destroy @descendant&.destroy @@ -61,18 +77,7 @@ end # Parent - exits = [] - [pid1, pid2].each - .with_index(1) do |pid, index| - Timeout.timeout(10) do - _pid, status = Process.wait2(pid) - exits << status.exitstatus - end - rescue Timeout::Error - raise StandardError, "parent: Timeout waiting for child#{index} to finish" - end - - expect(exits).to eq([0, 0]) + wait_for_child_processes([pid1, pid2]) expect(described_class.where(ancestor: ancestor, descendant: descendant).count).to eq(1) @edge = edge = described_class.last @@ -141,18 +146,89 @@ end # Parent - exits = [] - [pid1, pid2].each - .with_index(1) do |pid, index| - Timeout.timeout(10) do - _pid, status = Process.wait2(pid) - exits << status.exitstatus + wait_for_child_processes([pid1, pid2]) + + expect(described_class.where(ancestor: ancestor, descendant: descendant).count).to eq(1) + @edge = edge = described_class.last + expect(edge.ancestor).to eq(ancestor) + expect(edge.descendant).to eq(descendant) + expect(edge.direct).to be_truthy + expect(edge.count).to eq(1) + end + + it 'handles unique constraint violation' do + # Parent + ActiveRecord::Base.connection.reconnect! + @ancestor = ancestor = create(:labware) + @descendant = descendant = create(:labware) + ActiveRecord::Base.connection.commit_db_transaction + + first_socket, second_socket = UNIXSocket.pair + + # First child + pid1 = + fork do + ActiveRecord::Base.connection.reconnect! + find_link_call_count = 0 + allow(described_class).to receive(:find_link).and_wrap_original do |m, *args| + find_link_call_count += 1 + link = m.call(*args) + if find_link_call_count == 1 + expect(link).to be_nil + message = 'paused' + first_socket.send(message, 0) + message = 'child1: Timeout waiting for resume message' + raise StandardError, message unless first_socket.wait_readable(10) + message = 'resume' + first_socket.recv(message.size) + end + link end - rescue Timeout::Error - raise StandardError, "parent: Timeout waiting for child#{index} to finish" + + # rubocop:disable RSpec/AnyInstance + allow_any_instance_of(Dag::CreateCorrectnessValidator).to receive(:has_duplicates).and_return(false) + # rubocop:enable RSpec/AnyInstance + + allow(described_class).to receive(:save_edge_or_handle_error).and_wrap_original do |method, *args| + edge = args[0] + message = + "Duplicate entry '#{ancestor.id}-#{descendant.id}' " \ + "for key 'index_asset_links_on_ancestor_id_and_descendant_id'" + exception = ActiveRecord::RecordNotUnique.new(message) + allow(edge).to receive(:save).and_raise(exception) + + method.call(*args) + end + + unique_violation_error_return_value = nil + allow(described_class).to receive(:unique_violation_error?).and_wrap_original do |m, *args| + unique_violation_error_return_value = m.call(*args) + end + + result = described_class.create_edge(ancestor, descendant) + expect(result).to be_truthy + expect(described_class).to have_received(:unique_violation_error?) + expect(unique_violation_error_return_value).to be_truthy + + ActiveRecord::Base.connection.close end - expect(exits).to eq([0, 0]) + # Second child + pid2 = + fork do + ActiveRecord::Base.connection.reconnect! + message = 'child2: Timeout waiting for paused message' + raise StandardError, message unless second_socket.wait_readable(10) + message = 'paused' + second_socket.recv(message.size) + described_class.create_edge(ancestor, descendant) + message = 'resume' + second_socket.send(message, 0) + ActiveRecord::Base.connection.close + end + + # Parent + wait_for_child_processes([pid1, pid2]) expect(described_class.where(ancestor: ancestor, descendant: descendant).count).to eq(1) @edge = edge = described_class.last @@ -160,6 +236,8 @@ expect(edge.descendant).to eq(descendant) expect(edge.direct).to be_truthy expect(edge.count).to eq(1) + + ActiveRecord::Base.connection.close end end # rubocop:enable RSpec/ExampleLength,RSpec/InstanceVariable From b393fddeb16286dc9544bd60c823e35e02ec5ee1 Mon Sep 17 00:00:00 2001 From: yoldas Date: Wed, 25 Sep 2024 02:33:15 +0100 Subject: [PATCH 23/32] Update tests for create_edge method --- spec/models/asset_links_create_edge_spec.rb | 66 ++++++++++++++++----- 1 file changed, 51 insertions(+), 15 deletions(-) diff --git a/spec/models/asset_links_create_edge_spec.rb b/spec/models/asset_links_create_edge_spec.rb index 56be4a5d70..cffbf33376 100644 --- a/spec/models/asset_links_create_edge_spec.rb +++ b/spec/models/asset_links_create_edge_spec.rb @@ -4,7 +4,13 @@ RSpec.describe AssetLink, type: :model do # rubocop:disable RSpec/ExampleLength,RSpec/InstanceVariable + # Test the overridden create_edge class method. describe '.create_edge' do + # Wait for child processes to finish. + # + # @param pids [Array] Process IDs + # @raise [Timeout::Error] If a child process does not finish within 10 seconds. + # @return [void] def wait_for_child_processes(pids) exits = [] pids @@ -21,6 +27,7 @@ def wait_for_child_processes(pids) expect(exits).to eq([0, 0]) end + # Delete created records. after do @ancestor&.destroy @descendant&.destroy @@ -28,32 +35,43 @@ def wait_for_child_processes(pids) end it 'handles race condition at find_link' do + # In this example, the first and second processes are forked from the + # main process and they are in race condition to find an existing link + # between the ancestor and descendant and create an edge if no link is + # found. Both first and second processes finds no link, but the second + # process creates the edge first. The first also tries to create the + # edge based on the result of the find_link method. Only one link must + # be created. + # Parent ActiveRecord::Base.connection.reconnect! @ancestor = ancestor = create(:labware) @descendant = descendant = create(:labware) ActiveRecord::Base.connection.commit_db_transaction + # Create a duplex pipe for IPC. first_socket, second_socket = UNIXSocket.pair # First child pid1 = fork do ActiveRecord::Base.connection.reconnect! + find_link_call_count = 0 + # Patch find_link method allow(described_class).to receive(:find_link).and_wrap_original do |m, *args| find_link_call_count += 1 link = m.call(*args) if find_link_call_count == 1 - expect(link).to be_nil + expect(link).to be_nil # No link found message = 'paused' - first_socket.send(message, 0) + first_socket.send(message, 0) # Notify the second child message = 'child1: Timeout waiting for resume message' raise StandardError, message unless first_socket.wait_readable(10) - message = 'resume' + message = 'resume' # Wait for the second child to create the edge first_socket.recv(message.size) elsif find_link_call_count == 2 - expect(link).not_to be_nil + expect(link).not_to be_nil # Found one now end link end @@ -68,10 +86,10 @@ def wait_for_child_processes(pids) ActiveRecord::Base.connection.reconnect! message = 'child2: Timeout waiting for paused message' raise StandardError, message unless second_socket.wait_readable(10) - message = 'paused' + message = 'paused' # Wait for the first child to call find_link second_socket.recv(message.size) described_class.create_edge(ancestor, descendant) - message = 'resume' + message = 'resume' # Notify the first child second_socket.send(message, 0) ActiveRecord::Base.connection.close end @@ -90,12 +108,19 @@ def wait_for_child_processes(pids) end it 'handles unique validation error' do + # In this example, the first and second processes are forked from the + # main process. Neither of them finds and existing link between the + # ancestor and descendant. Both try to create the edge. One of them + # succeeds and the other one fails due to the has_duplicates validation. + # Failing process should use the existing link. + # Parent ActiveRecord::Base.connection.reconnect! @ancestor = ancestor = create(:labware) @descendant = descendant = create(:labware) ActiveRecord::Base.connection.commit_db_transaction + # Create a duplex pipe for IPC. first_socket, second_socket = UNIXSocket.pair # First child @@ -107,17 +132,18 @@ def wait_for_child_processes(pids) find_link_call_count += 1 link = m.call(*args) if find_link_call_count == 1 - expect(link).to be_nil - message = 'paused' + expect(link).to be_nil # No link found + message = 'paused' # Notify the second child first_socket.send(message, 0) message = 'child1: Timeout waiting for resume message' raise StandardError, message unless first_socket.wait_readable(10) - message = 'resume' + message = 'resume' # Wait for the second child to create the edge first_socket.recv(message.size) end link end + # Patch unique_validation_error? method unique_validation_error_return_value = nil allow(described_class).to receive(:unique_validation_error?).and_wrap_original do |m, *args| unique_validation_error_return_value = m.call(*args) @@ -137,10 +163,10 @@ def wait_for_child_processes(pids) ActiveRecord::Base.connection.reconnect! message = 'child2: Timeout waiting for paused message' raise StandardError, message unless second_socket.wait_readable(10) - message = 'paused' + message = 'paused' # Wait for the first child to call find_link second_socket.recv(message.size) described_class.create_edge(ancestor, descendant) - message = 'resume' + message = 'resume' # Notify the first child second_socket.send(message, 0) ActiveRecord::Base.connection.close end @@ -157,12 +183,19 @@ def wait_for_child_processes(pids) end it 'handles unique constraint violation' do + # In this example, the first and second processes are forked from the + # main process. Neither of them finds and existing link between the + # ancestor and descendant. Both try to create the edge. One of them + # succeeds and the other one fails due to the database unique constraint + # violation. Failing process should use the existing link. + # Parent ActiveRecord::Base.connection.reconnect! @ancestor = ancestor = create(:labware) @descendant = descendant = create(:labware) ActiveRecord::Base.connection.commit_db_transaction + # Create a duplex pipe for IPC. first_socket, second_socket = UNIXSocket.pair # First child @@ -175,20 +208,22 @@ def wait_for_child_processes(pids) link = m.call(*args) if find_link_call_count == 1 expect(link).to be_nil - message = 'paused' + message = 'paused' # Notify the second child first_socket.send(message, 0) message = 'child1: Timeout waiting for resume message' raise StandardError, message unless first_socket.wait_readable(10) - message = 'resume' + message = 'resume' # Wait for the second child to create the edge first_socket.recv(message.size) end link end + # Patch has_duplicates method. # rubocop:disable RSpec/AnyInstance allow_any_instance_of(Dag::CreateCorrectnessValidator).to receive(:has_duplicates).and_return(false) # rubocop:enable RSpec/AnyInstance + # Patch save_edge_or_handle_error method. allow(described_class).to receive(:save_edge_or_handle_error).and_wrap_original do |method, *args| edge = args[0] message = @@ -200,6 +235,7 @@ def wait_for_child_processes(pids) method.call(*args) end + # Patch unique_violation_error? method. unique_violation_error_return_value = nil allow(described_class).to receive(:unique_violation_error?).and_wrap_original do |m, *args| unique_violation_error_return_value = m.call(*args) @@ -219,10 +255,10 @@ def wait_for_child_processes(pids) ActiveRecord::Base.connection.reconnect! message = 'child2: Timeout waiting for paused message' raise StandardError, message unless second_socket.wait_readable(10) - message = 'paused' + message = 'paused' # Wait for the first child to call find_link second_socket.recv(message.size) described_class.create_edge(ancestor, descendant) - message = 'resume' + message = 'resume' # Notify the first child second_socket.send(message, 0) ActiveRecord::Base.connection.close end From 0ac25f7227f305c21e394e470d7afc9b1300bf69 Mon Sep 17 00:00:00 2001 From: yoldas Date: Wed, 25 Sep 2024 02:34:05 +0100 Subject: [PATCH 24/32] Deleted old version of the tests --- .../asset_links_race_conditions_spec.rb | 350 ------------------ 1 file changed, 350 deletions(-) delete mode 100644 spec/models/asset_links_race_conditions_spec.rb diff --git a/spec/models/asset_links_race_conditions_spec.rb b/spec/models/asset_links_race_conditions_spec.rb deleted file mode 100644 index cc804dcce8..0000000000 --- a/spec/models/asset_links_race_conditions_spec.rb +++ /dev/null @@ -1,350 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -RSpec.xdescribe AssetLink, type: :model do - describe '.create_edge' do - # Helpers - - # Used in IPC when one end of the duplex pipe is waiting for the other end - # to send a message with a timeout in seconds. - # - # @param socket [Socket] The socket to wait for. - # @param length [Integer] The length of the message to wait for. - # @param timeout [Integer] The timeout in seconds, default is 10. - # @param message [String] The message to raise if times out, default is 'socket timeout'. - # @return [String] The message received from the socket. - # @raise StandardError - def wait_readable_with_timeout(socket, length, timeout = 10, message = 'socket timeout') - raise StandardError, message unless socket.wait_readable(timeout) - socket.recv(length) - end - - # Used to reap a child process with a timeout in seconds. - # - # @param pid [Integer] The process ID to wait for. - # @param timeout [Integer] The timeout in seconds, default is 10. - # @param message [String] The message to raise if times out, default is 'process timeout'. - # @return [Process::Status] The status of the process. - # @raise StandardError - # rubocop:disable Metrics/MethodLength - def wait_process_with_timeout(pid, timeout = 10, message = 'process timeout') - start_time = Time.zone.now - loop do - begin - pid2, status = Process.waitpid2(pid, Process::WNOHANG) - return status if pid2 - rescue Errno::ECHILD # No child process - return nil - end - if Time.zone.now - start_time > timeout - begin - Process.kill('TERM', pid) # Send TERM signal. - sleep 1 # Wait for the process to terminate. - Process.kill('KILL', pid) # Send KILL signal. - Process.waitpid(pid) # Reap - rescue Errno::ECHILD - # No child process - end - raise StandardError, message - end - sleep 0.1 - end - end - # rubocop:enable Metrics/MethodLength - - # Wait for the given processes to finish with a timeout in seconds. - # @param pids [Array] The process IDs to wait for. - # @return [void] - def wait_for_processes(pids) - pids.each do |pid| - status = wait_process_with_timeout(pid, 10, "Timeout waiting for process #{pid} to finish.") - if status.exitstatus != 0 - raise StandardError, "Forked process #{pid} failed with exit status #{status.exitstatus}" - end - end - end - - # Examples - - # We need to explicitly clean the database after examples here because we - # are establishing new database connections in the main and forked child - # processes. - # rubocop:disable RSpec/BeforeAfterAll - after(:all) do - AssetLink.delete_all - Labware.delete_all - end - # rubocop:enable RSpec/BeforeAfterAll - - # rubocop:disable RSpec/ExampleLength - it 'handles race condition at find_link' do - # In this example, the first and second processes are forked from the - # main process and they are in race condition to find an existing link - # between the ancestor and descendant and create an edge if no link is - # found. Both first and second processes finds no link, but the second - # process creates the edge first. The first also tries to create the - # edge based on the result of the find_link method. However, it must - # not be able to create it. - - ActiveRecord::Base.connection.reconnect! - ancestor = create(:labware) - descendant = create(:labware) - ActiveRecord::Base.connection.commit_db_transaction - - # Create a duplex pipe for inter-process communication. - first_socket, second_socket = Socket.pair(:UNIX, :STREAM) - - # Fork the first process. - pid1 = - fork do - ActiveRecord::Base.connection.reconnect! - - call_count = 0 # Track the calls to the find_link method. - - # Patch the find_link method in the first process. - allow(described_class).to receive(:find_link).and_wrap_original do |method, *args| - call_count += 1 - link = method.call(*args) - if call_count == 1 - expect(link).to be_nil # The first process should not find any link initally. - - signal = 'paused' - first_socket.send(signal, 0) # Signal that the first process is paused now. Zero flags. - - # Wait for the second process to send 'resume'. - wait_readable_with_timeout( - first_socket, - 'resume'.length, - 10, - 'Timeout waiting for the second process to send resume.' - ) - elsif call_count == 2 - # The first process now should find the link created by the second process. - expect(link).not_to be_nil - end - link - end - - described_class.create_edge(ancestor, descendant) - ActiveRecord::Base.connection.close - end - - # Fork the second process. - pid2 = - fork do - ActiveRecord::Base.connection.reconnect! # Reconnect to the database in the forked process. - - # Wait for the first process to signal that it is paused after calling find_link. - wait_readable_with_timeout( - second_socket, - 'paused'.length, - 10, - 'Timeout waiting for the first process to send paused.' - ) - - described_class.create_edge(ancestor, descendant) - # Signal the first process to resume now. Although it has found no link - # in its last method call, actually there is one now. - signal = 'resume' - second_socket.send(signal, 0) # Zero flags. - ActiveRecord::Base.connection.close - end - - # Wait for both processes to finish and check their exit statuses - wait_for_processes([pid1, pid2]) - - # Check that the edge was created as expected. - expect(described_class.where(ancestor: ancestor, descendant: descendant).count).to eq(1) - link = described_class.last - expect(link.ancestor).to eq(ancestor) - expect(link.descendant).to eq(descendant) - expect(link.direct).to be_truthy - expect(link.count).to eq(1) - - ActiveRecord::Base.connection.close - end - # rubocop:enable RSpec/ExampleLength - - # rubocop:disable RSpec/ExampleLength - it 'handles race condition at has_duplicates' do - # In this example, the first and second processes are forked from the - # main process. Neither of them finds and existing link between the - # ancestor and descendant. Both try to create the edge. One of them - # succeeds and the other one fails due to the has_duplicates validation. - # Failing process should use the existing link. - - ActiveRecord::Base.connection.reconnect! - ancestor = create(:labware) - descendant = create(:labware) - ActiveRecord::Base.connection.commit_db_transaction - - # Create a duplex pipe for inter-process communication. - first_socket, second_socket = Socket.pair(:UNIX, :STREAM) - - # Fork the first process. - pid1 = - fork do - ActiveRecord::Base.connection.reconnect! - call_count = 0 # Track the calls to the find_link method. - # Patch the find_link method in the first process. - # Check link, trigger the second process, pause, and then wait for'resume' signal. - allow(described_class).to receive(:find_link).and_wrap_original do |method, *args| - call_count += 1 - link = method.call(*args) - if call_count == 1 - expect(link).to be_nil # The first process should not find any link initally. - signal = 'paused' - first_socket.send(signal, 0) # Signal that the first process is paused now. Zero flags. - # Wait for the second process to send 'resume'. - wait_readable_with_timeout( - first_socket, - 'resume'.length, - 10, - 'Timeout waiting for the second process to send resume.' - ) - end - link - end - allow(described_class).to receive(:unique_validation_error?).and_call_original - - # Now the first process should be prevented from saving because of has_duplicates validation. - result = described_class.create_edge(ancestor, descendant) - expect(result).to be_truthy - expect(described_class).to have_received(:unique_validation_error?) - - ActiveRecord::Base.connection.close - end - - # Fork the second process. - pid2 = - fork do - ActiveRecord::Base.connection.reconnect! - # Wait for the first process to signal that it is paused after calling find_link. - wait_readable_with_timeout( - second_socket, - 'paused'.length, - 10, - 'Timeout waiting for the first process to send paused.' - ) - described_class.create_edge(ancestor, descendant) - signal = 'resume' - second_socket.send(signal, 0) # Zero flags. - ActiveRecord::Base.connection.close - end - - # Wait for both processes to finish and check their exit statuses - wait_for_processes([pid1, pid2]) - - # Check that the edge was created as expected. - expect(described_class.where(ancestor: ancestor, descendant: descendant).count).to eq(1) - link = described_class.last - expect(link.ancestor).to eq(ancestor) - expect(link.descendant).to eq(descendant) - expect(link.direct).to be_truthy - expect(link.count).to eq(1) # How many different paths between the nodes. - end - # rubocop:enable RSpec/ExampleLength - - # rubocop:disable RSpec/ExampleLength - it 'handles unique constraint violation' do - # In this example, the first and second processes are forked from the - # main process. Neither of them finds and existing link between the - # ancestor and descendant. Both try to create the edge. One of them - # succeeds and the other one fails due to the unique constraint - # violation. Failing process should use the existing link. - - ActiveRecord::Base.connection.reconnect! - ancestor = create(:labware) - descendant = create(:labware) - ActiveRecord::Base.connection.commit_db_transaction - - # Create a duplex pipe for inter-process communication. - first_socket, second_socket = Socket.pair(:UNIX, :STREAM) - - # Fork the first process. - pid1 = - fork do - ActiveRecord::Base.connection.reconnect! - call_count = 0 # Track the calls to the find_link method. - # Patch the find_link method in the first process. - # Check link, trigger the second process, pause, and then wait for'resume' signal. - allow(described_class).to receive(:find_link).and_wrap_original do |method, *args| - call_count += 1 - link = method.call(*args) - if call_count == 1 - expect(link).to be_nil # The first process should not find any link initally. - signal = 'paused' - first_socket.send(signal, 0) # Signal that the first process is paused now. Zero flags. - # Wait for the second process to send 'resume'. - wait_readable_with_timeout( - first_socket, - 'resume'.length, - 10, - 'Timeout waiting for the second process to send resume.' - ) - end - link - end - # Validation error is not triggered in this case because the other - # process has not saved the link yet. Therefore, the save call will - # pass the validations and hit the database to trigger the unique - # constraint violation just after the other process saves the link. - # rubocop:disable RSpec/AnyInstance - allow_any_instance_of(Dag::CreateCorrectnessValidator).to receive(:has_duplicates).and_return(false) - # rubocop:enable RSpec/AnyInstance - - allow(described_class).to receive(:save_edge_or_handle_error).and_wrap_original do |method, *args| - edge = args[0] - message = - "Duplicate entry '#{ancestor.id}-#{descendant.id}' " \ - "for key 'index_asset_links_on_ancestor_id_and_descendant_id'" - exception = ActiveRecord::RecordNotUnique.new(message) - allow(edge).to receive(:save).and_raise(exception) - - method.call(*args) - end - allow(described_class).to receive(:unique_violation_error?).and_call_original - - described_class.create_edge(ancestor, descendant) - expect(described_class).to have_received(:unique_violation_error?) - ActiveRecord::Base.connection.close - end - - # Fork the second process. - pid2 = - fork do - ActiveRecord::Base.connection.reconnect! # Reconnect to the database in the forked process. - - # Wait for the first process to signal that it is paused after calling find_link. - wait_readable_with_timeout( - second_socket, - 'paused'.length, - 10, - 'Timeout waiting for the first process to send paused.' - ) - - described_class.create_edge(ancestor, descendant) - # Signal the first process to resume now. Although it has found no link - # in its last method call, actually there is one now. - signal = 'resume' - second_socket.send(signal, 0) # Zero flags. - ActiveRecord::Base.connection.close - end - - # Wait for both processes to finish and check their exit statuses - wait_for_processes([pid1, pid2]) - - # Check that the edge was created as expected. - expect(described_class.where(ancestor: ancestor, descendant: descendant).count).to eq(1) - link = described_class.last - expect(link.ancestor).to eq(ancestor) - expect(link.descendant).to eq(descendant) - expect(link.direct).to be_truthy - expect(link.count).to eq(1) - - ActiveRecord::Base.connection.close - end - # rubocop:enable RSpec/ExampleLength - end -end From ca872412d15d7bbc707ae643fc88ba47075c02b0 Mon Sep 17 00:00:00 2001 From: yoldas Date: Wed, 25 Sep 2024 17:29:09 +0100 Subject: [PATCH 25/32] Remove redundant require --- db/migrate/20240912000109_add_unique_index_to_asset_links.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/db/migrate/20240912000109_add_unique_index_to_asset_links.rb b/db/migrate/20240912000109_add_unique_index_to_asset_links.rb index 13a495bbaa..786eb1b9b1 100644 --- a/db/migrate/20240912000109_add_unique_index_to_asset_links.rb +++ b/db/migrate/20240912000109_add_unique_index_to_asset_links.rb @@ -1,6 +1,5 @@ # frozen_string_literal: true -require 'csv' # This migration adds a unique-together index on ancestor_id and descendant_id # in order to prevent duplicate links between the same ancestor and descendant # labware. From 80954a053c26c2e29e05ced6eec8a06fe7b39fb3 Mon Sep 17 00:00:00 2001 From: yoldas Date: Wed, 2 Oct 2024 09:55:35 +0100 Subject: [PATCH 26/32] Add UAT action to generate randomised FluidX barcodes --- .../uat_actions/generate_fluidx_barcodes.rb | 101 ++++++++++++++++++ 1 file changed, 101 insertions(+) create mode 100644 app/uat_actions/uat_actions/generate_fluidx_barcodes.rb diff --git a/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb b/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb new file mode 100644 index 0000000000..bd5e18854c --- /dev/null +++ b/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb @@ -0,0 +1,101 @@ +# frozen_string_literal: true + +# UAT action to generate randomised FluidX barcodes. +class UatActions::GenerateFluidxBarcodes < UatActions + self.title = 'Generate FluidX Barcodes' + self.description = 'Generate randomised FluidX barcodes' + self.category = :auxiliary_data + + validates :barcode_count, numericality: { only_integer: true, greater_than: 0, less_than_or_equal_to: 96 } + validates :barcode_prefix, + presence: true, + length: { + is: 2 + }, + format: { + with: /\A[A-Z]{2}\z/, + message: 'only allows two uppercase letters' + } + validates :barcode_index, numericality: { only_integer: true, greater_than: 0, less_than_or_equal_to: 900 } + + form_field :barcode_count, + :number_field, + label: 'Number of barcodes', + help: 'The number of FluidX barcodes that should be generated', + options: { + min: 1, + max: 96, + value: 1 + } + form_field :barcode_prefix, + :text_field, + label: 'Barcode prefix', + help: 'The prefix to be used for the barcodes', + options: { + maxlength: 2, + oninput: 'javascript:this.value=this.value.toUpperCase().replace(/[^A-Z]/g, "")', + value: 'TS' + } + form_field :barcode_index, + :number_field, + label: 'Barcode index', + help: 'The starting index to make a sequential tail for the barcodes', + options: { + min: 1, + max: 900, + value: 1 + } + + def perform + random = Array.new(6) { rand(0..9) }.join # uniform distribution of digits + report['barcodes'] = generate_barcodes(barcode_count.to_i, barcode_prefix, random, barcode_index.to_i) + true + end + + private + + # Generates an array of barcodes with the specified count, prefix, common + # random part, and starting index for the sequential tail. It attempts to + # generate unique barcodes, iterating up to 5 times before giving up. + # + # @param count [Integer] the number of barcodes to generate + # @param prefix [String] the prefix to be used for the barcodes + # @param random [String] a common random part for the barcodes + # @param index [Integer] the starting index for the sequential tail + # @return [Array] an array of unique barcodes + # @raise [StandardError] if it fails to generate unique barcodes + def generate_barcodes(count, prefix, random, index) + barcodes = [] + 5.times do # Max 5 iterations to generate unique barcodes. + barcodes.concat(filter_barcodes(build_barcodes(count, prefix, random, index))) + return barcodes if barcodes.size == barcode_count.to_i + count = barcode_count.to_i - barcodes.size + index += count + end + raise StandardError, 'Failed to generate unique barcodes' + end + + # Filters out the barcodes that already exist in the database. + # + # @param barcodes [Array] an array of barcodes + # @return [Array] an array of unique barcodes + def filter_barcodes(barcodes) + barcodes - Barcode.where(barcode: barcodes).pluck(:barcode) + end + + # Builds an array of barcodes with the specified count, prefix, common + # random part, and starting index for the sequential tail. + # + # @param count [Integer] the number of barcodes to generate + # @param prefix [String] the prefix to be used for the barcodes + # @param random [String] a common random part for the barcodes + # @param index [Integer] the starting index for the suffix + # @return [Array] an array of barcodes + def build_barcodes(count, prefix, random, index) + (index...(index + count)).map do |counter| + suffix = counter.to_s.rjust(2, '0') # Min 2 suffix digits + random = random[0, 8 - suffix.length] # 8 FluidX digits minus suffix + "#{prefix}#{random}#{suffix}" + end + end +end From 5bb6bb8c41596fe68e05fbf7f59d9924c656f2f2 Mon Sep 17 00:00:00 2001 From: yoldas Date: Wed, 2 Oct 2024 10:22:42 +0100 Subject: [PATCH 27/32] Fill errors collection instead of raising exception --- .../uat_actions/generate_fluidx_barcodes.rb | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb b/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb index bd5e18854c..b11cb6e713 100644 --- a/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb +++ b/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb @@ -46,10 +46,22 @@ class UatActions::GenerateFluidxBarcodes < UatActions value: 1 } + # This method is called from the save method after validations have passed. + # If the return value is true, the report hash populated by the action is + # used for rendering the response. If the return value is false, the errors + # collection is used. + # + # @return [Boolean] true if the action was successful; false otherwise def perform random = Array.new(6) { rand(0..9) }.join # uniform distribution of digits - report['barcodes'] = generate_barcodes(barcode_count.to_i, barcode_prefix, random, barcode_index.to_i) - true + barcodes = generate_barcodes(barcode_count.to_i, barcode_prefix, random, barcode_index.to_i) + if barcodes.size == barcode_count.to_i + report['barcodes'] = barcodes + true + else + errors.add(:base, 'Failed to generate unique barcodes') + false + end end private @@ -72,7 +84,6 @@ def generate_barcodes(count, prefix, random, index) count = barcode_count.to_i - barcodes.size index += count end - raise StandardError, 'Failed to generate unique barcodes' end # Filters out the barcodes that already exist in the database. From 8531eae25bacaad6c09896bacb31c4dbf36ac260 Mon Sep 17 00:00:00 2001 From: yoldas Date: Wed, 2 Oct 2024 10:27:42 +0100 Subject: [PATCH 28/32] Continue index in the next iteration --- app/uat_actions/uat_actions/generate_fluidx_barcodes.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb b/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb index b11cb6e713..84cd8bec58 100644 --- a/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb +++ b/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb @@ -81,8 +81,8 @@ def generate_barcodes(count, prefix, random, index) 5.times do # Max 5 iterations to generate unique barcodes. barcodes.concat(filter_barcodes(build_barcodes(count, prefix, random, index))) return barcodes if barcodes.size == barcode_count.to_i - count = barcode_count.to_i - barcodes.size - index += count + count = barcode_count.to_i - barcodes.size # More to generate. + index += barcode_count.to_i # Continue index. end end From f7a918abb6ae4d341bc2e2f3cc359f42b88b4600 Mon Sep 17 00:00:00 2001 From: yoldas Date: Wed, 2 Oct 2024 22:23:53 +0100 Subject: [PATCH 29/32] Separate random part and index with zero --- .../uat_actions/generate_fluidx_barcodes.rb | 33 +++++++++++++------ 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb b/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb index 84cd8bec58..7f0441e756 100644 --- a/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb +++ b/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb @@ -1,10 +1,17 @@ # frozen_string_literal: true # UAT action to generate randomised FluidX barcodes. +# : Ten characters in total +# : two uppercase letters +# : six random digits; may be truncated from the end to fit the length +# : +# : one digit that separates random part and index, i.e. '0' +# : sequential tail, 1 to 3 digits, e.g., '9', '99', and '999' class UatActions::GenerateFluidxBarcodes < UatActions self.title = 'Generate FluidX Barcodes' self.description = 'Generate randomised FluidX barcodes' self.category = :auxiliary_data + self.max_number_of_iterations = 5 validates :barcode_count, numericality: { only_integer: true, greater_than: 0, less_than_or_equal_to: 96 } validates :barcode_prefix, @@ -64,21 +71,27 @@ def perform end end + # Returns a default copy of the UatAction which will be used to fill in the form. + # + # @return [UatActions::GenerateFluidxBarcodes] a default object for rendering a form + def self.default + new(barcode_count: '1', barcode_prefix: 'TS', barcode_index: '1') + end + private - # Generates an array of barcodes with the specified count, prefix, common - # random part, and starting index for the sequential tail. It attempts to - # generate unique barcodes, iterating up to 5 times before giving up. + # Generates an array of barcodes with the specified count, prefix, random + # part, and starting index for the sequential tail. It attempts to generate + # unique barcodes, iterating up to max_number_of_iterations before giving up. # # @param count [Integer] the number of barcodes to generate # @param prefix [String] the prefix to be used for the barcodes - # @param random [String] a common random part for the barcodes + # @param random [String] random part for the barcodes # @param index [Integer] the starting index for the sequential tail # @return [Array] an array of unique barcodes - # @raise [StandardError] if it fails to generate unique barcodes def generate_barcodes(count, prefix, random, index) barcodes = [] - 5.times do # Max 5 iterations to generate unique barcodes. + max_number_of_iterations.times do barcodes.concat(filter_barcodes(build_barcodes(count, prefix, random, index))) return barcodes if barcodes.size == barcode_count.to_i count = barcode_count.to_i - barcodes.size # More to generate. @@ -94,17 +107,17 @@ def filter_barcodes(barcodes) barcodes - Barcode.where(barcode: barcodes).pluck(:barcode) end - # Builds an array of barcodes with the specified count, prefix, common - # random part, and starting index for the sequential tail. + # Builds an array of barcodes with the specified count, prefix, random + # part, and starting index for the sequential tail. # # @param count [Integer] the number of barcodes to generate # @param prefix [String] the prefix to be used for the barcodes - # @param random [String] a common random part for the barcodes + # @param random [String] random part for the barcodes # @param index [Integer] the starting index for the suffix # @return [Array] an array of barcodes def build_barcodes(count, prefix, random, index) (index...(index + count)).map do |counter| - suffix = counter.to_s.rjust(2, '0') # Min 2 suffix digits + suffix = '0' + counter.to_s random = random[0, 8 - suffix.length] # 8 FluidX digits minus suffix "#{prefix}#{random}#{suffix}" end From e81b4a0e78d6119ce668be94cf66cf10257b1115 Mon Sep 17 00:00:00 2001 From: yoldas Date: Wed, 2 Oct 2024 22:53:35 +0100 Subject: [PATCH 30/32] Deleted file as it does not belong to this PR --- .../uat_actions/generate_fluidx_barcodes.rb | 125 ------------------ 1 file changed, 125 deletions(-) delete mode 100644 app/uat_actions/uat_actions/generate_fluidx_barcodes.rb diff --git a/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb b/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb deleted file mode 100644 index 7f0441e756..0000000000 --- a/app/uat_actions/uat_actions/generate_fluidx_barcodes.rb +++ /dev/null @@ -1,125 +0,0 @@ -# frozen_string_literal: true - -# UAT action to generate randomised FluidX barcodes. -# : Ten characters in total -# : two uppercase letters -# : six random digits; may be truncated from the end to fit the length -# : -# : one digit that separates random part and index, i.e. '0' -# : sequential tail, 1 to 3 digits, e.g., '9', '99', and '999' -class UatActions::GenerateFluidxBarcodes < UatActions - self.title = 'Generate FluidX Barcodes' - self.description = 'Generate randomised FluidX barcodes' - self.category = :auxiliary_data - self.max_number_of_iterations = 5 - - validates :barcode_count, numericality: { only_integer: true, greater_than: 0, less_than_or_equal_to: 96 } - validates :barcode_prefix, - presence: true, - length: { - is: 2 - }, - format: { - with: /\A[A-Z]{2}\z/, - message: 'only allows two uppercase letters' - } - validates :barcode_index, numericality: { only_integer: true, greater_than: 0, less_than_or_equal_to: 900 } - - form_field :barcode_count, - :number_field, - label: 'Number of barcodes', - help: 'The number of FluidX barcodes that should be generated', - options: { - min: 1, - max: 96, - value: 1 - } - form_field :barcode_prefix, - :text_field, - label: 'Barcode prefix', - help: 'The prefix to be used for the barcodes', - options: { - maxlength: 2, - oninput: 'javascript:this.value=this.value.toUpperCase().replace(/[^A-Z]/g, "")', - value: 'TS' - } - form_field :barcode_index, - :number_field, - label: 'Barcode index', - help: 'The starting index to make a sequential tail for the barcodes', - options: { - min: 1, - max: 900, - value: 1 - } - - # This method is called from the save method after validations have passed. - # If the return value is true, the report hash populated by the action is - # used for rendering the response. If the return value is false, the errors - # collection is used. - # - # @return [Boolean] true if the action was successful; false otherwise - def perform - random = Array.new(6) { rand(0..9) }.join # uniform distribution of digits - barcodes = generate_barcodes(barcode_count.to_i, barcode_prefix, random, barcode_index.to_i) - if barcodes.size == barcode_count.to_i - report['barcodes'] = barcodes - true - else - errors.add(:base, 'Failed to generate unique barcodes') - false - end - end - - # Returns a default copy of the UatAction which will be used to fill in the form. - # - # @return [UatActions::GenerateFluidxBarcodes] a default object for rendering a form - def self.default - new(barcode_count: '1', barcode_prefix: 'TS', barcode_index: '1') - end - - private - - # Generates an array of barcodes with the specified count, prefix, random - # part, and starting index for the sequential tail. It attempts to generate - # unique barcodes, iterating up to max_number_of_iterations before giving up. - # - # @param count [Integer] the number of barcodes to generate - # @param prefix [String] the prefix to be used for the barcodes - # @param random [String] random part for the barcodes - # @param index [Integer] the starting index for the sequential tail - # @return [Array] an array of unique barcodes - def generate_barcodes(count, prefix, random, index) - barcodes = [] - max_number_of_iterations.times do - barcodes.concat(filter_barcodes(build_barcodes(count, prefix, random, index))) - return barcodes if barcodes.size == barcode_count.to_i - count = barcode_count.to_i - barcodes.size # More to generate. - index += barcode_count.to_i # Continue index. - end - end - - # Filters out the barcodes that already exist in the database. - # - # @param barcodes [Array] an array of barcodes - # @return [Array] an array of unique barcodes - def filter_barcodes(barcodes) - barcodes - Barcode.where(barcode: barcodes).pluck(:barcode) - end - - # Builds an array of barcodes with the specified count, prefix, random - # part, and starting index for the sequential tail. - # - # @param count [Integer] the number of barcodes to generate - # @param prefix [String] the prefix to be used for the barcodes - # @param random [String] random part for the barcodes - # @param index [Integer] the starting index for the suffix - # @return [Array] an array of barcodes - def build_barcodes(count, prefix, random, index) - (index...(index + count)).map do |counter| - suffix = '0' + counter.to_s - random = random[0, 8 - suffix.length] # 8 FluidX digits minus suffix - "#{prefix}#{random}#{suffix}" - end - end -end From c9a058a87c68603da5317d8d7703430ad329c951 Mon Sep 17 00:00:00 2001 From: yoldas Date: Wed, 2 Oct 2024 22:58:42 +0100 Subject: [PATCH 31/32] Rubocop --- spec/models/asset_links_create_edge_spec.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/models/asset_links_create_edge_spec.rb b/spec/models/asset_links_create_edge_spec.rb index cffbf33376..63bff1b4b5 100644 --- a/spec/models/asset_links_create_edge_spec.rb +++ b/spec/models/asset_links_create_edge_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' RSpec.describe AssetLink, type: :model do - # rubocop:disable RSpec/ExampleLength,RSpec/InstanceVariable + # rubocop:disable RSpec/InstanceVariable,Metrics/MethodLength,RSpec/ExampleLength,RSpec/MultipleExpectations # Test the overridden create_edge class method. describe '.create_edge' do # Wait for child processes to finish. @@ -97,7 +97,7 @@ def wait_for_child_processes(pids) # Parent wait_for_child_processes([pid1, pid2]) - expect(described_class.where(ancestor: ancestor, descendant: descendant).count).to eq(1) + expect(described_class.where(ancestor:, descendant:).count).to eq(1) @edge = edge = described_class.last expect(edge.ancestor).to eq(ancestor) expect(edge.descendant).to eq(descendant) @@ -174,7 +174,7 @@ def wait_for_child_processes(pids) # Parent wait_for_child_processes([pid1, pid2]) - expect(described_class.where(ancestor: ancestor, descendant: descendant).count).to eq(1) + expect(described_class.where(ancestor:, descendant:).count).to eq(1) @edge = edge = described_class.last expect(edge.ancestor).to eq(ancestor) expect(edge.descendant).to eq(descendant) @@ -266,7 +266,7 @@ def wait_for_child_processes(pids) # Parent wait_for_child_processes([pid1, pid2]) - expect(described_class.where(ancestor: ancestor, descendant: descendant).count).to eq(1) + expect(described_class.where(ancestor:, descendant:).count).to eq(1) @edge = edge = described_class.last expect(edge.ancestor).to eq(ancestor) expect(edge.descendant).to eq(descendant) @@ -276,5 +276,5 @@ def wait_for_child_processes(pids) ActiveRecord::Base.connection.close end end - # rubocop:enable RSpec/ExampleLength,RSpec/InstanceVariable + # rubocop:enable RSpec/InstanceVariable,Metrics/MethodLength,RSpec/ExampleLength,RSpec/MultipleExpectations end From a9ff092137bf82aa40e0b4e689f479de05a8d4ab Mon Sep 17 00:00:00 2001 From: yoldas Date: Thu, 3 Oct 2024 09:25:44 +0100 Subject: [PATCH 32/32] Deleting the part-1 test as it is conflicting with part-2 --- .../remove_duplicate_asset_links_spec.rb | 51 ------------------- 1 file changed, 51 deletions(-) delete mode 100644 spec/tasks/support/remove_duplicate_asset_links_spec.rb diff --git a/spec/tasks/support/remove_duplicate_asset_links_spec.rb b/spec/tasks/support/remove_duplicate_asset_links_spec.rb deleted file mode 100644 index b34eebdc98..0000000000 --- a/spec/tasks/support/remove_duplicate_asset_links_spec.rb +++ /dev/null @@ -1,51 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -# rubocop:disable RSpec/ExampleLength, RSpec/MultipleExpectations,RSpec/MultipleMemoizedHelpers -RSpec.describe 'support:remove_duplicate_asset_links', type: :task do - let(:clear_tasks) { Rake.application.clear } - let(:load_tasks) { Rails.application.load_tasks } - let(:task_reenable) { Rake::Task[self.class.top_level_description].reenable } - let(:task_invoke) { Rake::Task[self.class.top_level_description].invoke(csv_file_path) } - let(:csv_file_path) { 'tmp/deleted_asset_links.csv' } - let(:links) { create_list(:asset_link, 5) } - let(:duplicate_links) do - links.map do |link| - duplicate = AssetLink.new(ancestor: link.ancestor, descendant: link.descendant, created_at: 1.day.ago) - duplicate.save!(validate: false) # Needs to be saved without validation - duplicate - end - end - - before do - clear_tasks - load_tasks - task_reenable - links - duplicate_links - end - - after { File.delete(csv_file_path) if File.exist?(csv_file_path) } - - it 'removes all duplicate links except the most recently created ones' do - expect(AssetLink.count).to eq(links.size + duplicate_links.size) - task_invoke - expect(AssetLink.count).to eq(links.size) # most recent links should be kept - expect(AssetLink.exists?(links.first.id)).to be true - expect(AssetLink.exists?(duplicate_links.first.id)).to be false - end - - it 'exports the removed duplicates to a CSV file' do - task_invoke - expect(File.exist?(csv_file_path)).to be true - csv = CSV.read(Rails.root.join(csv_file_path)) - expect(csv.size).to eq(duplicate_links.size + 1) # With header. - expect(csv.first).to eq(AssetLink.column_names) - (1..duplicate_links.size).each do |i| - expected_row = duplicate_links[i - 1].attributes.values.map { |value| value&.to_s } - expect(csv[i]).to eq(expected_row) - end - end -end -# rubocop:enable RSpec/ExampleLength, RSpec/MultipleExpectations,RSpec/MultipleMemoizedHelpers