Move lockfile operations one lever up

lib/rmt.rb

@@ -6,5 +6,4 @@ module RMT
   DEFAULT_MIRROR_DIR = File.expand_path(File.join(__dir__, '../public/repo/')).freeze
   DEFAULT_MIRROR_URL_PREFIX = '/repo/'.freeze
 
-  ExecutionLockedError = Class.new(StandardError)
 end

lib/rmt/cli/base.rb

@@ -1,3 +1,5 @@
+require 'rmt/lockfile'
+
 class RMT::CLI::Base < Thor
 
   class << self
@@ -36,13 +38,11 @@ def dispatch(command, given_args, given_opts, config)
     def handle_exceptions
       yield
     rescue RMT::Deduplicator::HardlinkException => e
-      RMT::Lockfile.remove_file
       raise RMT::CLI::Error.new(
         "Could not create deduplication hardlink: #{e.message}.",
         RMT::CLI::Error::ERROR_OTHER
       )
     rescue Mysql2::Error => e
-      RMT::Lockfile.remove_file
       if e.message =~ /^Access denied/
         raise RMT::CLI::Error.new(
           "Cannot connect to database server. Make sure its credentials are configured in '/etc/rmt.conf'.",
@@ -58,19 +58,15 @@ def handle_exceptions
         raise e
       end
     rescue ActiveRecord::NoDatabaseError
-      RMT::Lockfile.remove_file
       raise RMT::CLI::Error.new(
         "The RMT database has not yet been initialized. Run 'systemctl start rmt-migration' to setup the database.",
         RMT::CLI::Error::ERROR_DB
       )
     rescue RMT::SCC::CredentialsError, ::SUSE::Connect::Api::InvalidCredentialsError
-      RMT::Lockfile.remove_file
       raise RMT::CLI::Error.new(
         "The SCC credentials are not configured correctly in '/etc/rmt.conf'. You can obtain them from https://scc.suse.com/organization",
         RMT::CLI::Error::ERROR_SCC
       )
-    rescue RMT::ExecutionLockedError
-      puts 'Process is locked'
     end
 
     # These methods are needed to properly format the hint outputs for `rmt-cli repos custom`. This is a workaround

lib/rmt/cli/import.rb

@@ -2,13 +2,21 @@ class RMT::CLI::Import < RMT::CLI::Base
 
   desc 'data PATH', 'Read SCC data from given path'
   def data(path)
+    RMT::Lockfile.create_file
     needs_path(path) do
       RMT::SCC.new(options).import(path)
     end
+    RMT::Lockfile.remove_file
+  rescue RMT::Lockfile::ExecutionLockedError
+    puts 'Process is locked'
+  rescue StandardError
+    RMT::Lockfile.remove_file
+    raise
   end
 
   desc 'repos PATH', 'Mirror repos from given path'
   def repos(path)
+    RMT::Lockfile.create_file
     needs_path(path) do
       repos_file = File.join(path, 'repos.json')
       unless File.exist?(repos_file)
@@ -28,6 +36,12 @@ def repos(path)
         mirror!(repo, repository_url: repo_json['url'], to_offline: true)
       end
     end
+    RMT::Lockfile.remove_file
+  rescue RMT::Lockfile::ExecutionLockedError
+    puts 'Process is locked'
+  rescue StandardError
+    RMT::Lockfile.remove_file
+    raise
   end
 
 end

lib/rmt/cli/main.rb

@@ -7,9 +7,14 @@ class RMT::CLI::Main < RMT::CLI::Base
 
   desc 'sync', 'Sync database with SUSE Customer Center'
   def sync
+    RMT::Lockfile.create_file
     RMT::SCC.new(options).sync
-  rescue RMT::ExecutionLockedError
+    RMT::Lockfile.remove_file
+  rescue RMT::Lockfile::ExecutionLockedError
     puts 'Process is locked'
+  rescue StandardError
+    RMT::Lockfile.remove_file
+    raise
   end
 
   desc 'products', 'List and modify products'
@@ -20,14 +25,19 @@ def sync
 
   desc 'mirror', 'Mirror repositories'
   def mirror
+    RMT::Lockfile.create_file
     repos = Repository.where(mirroring_enabled: true)
     if repos.empty?
       warn 'There are no repositories marked for mirroring.'
       return
     end
     repos.each { |repo| mirror!(repo) }
-  rescue RMT::ExecutionLockedError
+    RMT::Lockfile.remove_file
+  rescue RMT::Lockfile::ExecutionLockedError
     puts 'Process is locked'
+  rescue StandardError
+    RMT::Lockfile.remove_file
+    raise
   end
 
   desc 'import', 'Import commands for Offline Sync'

lib/rmt/lockfile.rb

@@ -1,22 +1,20 @@
 class RMT::Lockfile
-  LOCKFILE_LOCATION = File.expand_path('../../tmp/pids/rmt.pid', __dir__).freeze
+  LOCKFILE_LOCATION = File.expand_path('../../tmp/rmt.lock', __dir__).freeze
+  ExecutionLockedError = Class.new(StandardError)
 
   class << self
     def create_file
       if File.exist?(LOCKFILE_LOCATION)
-        raise RMT::ExecutionLockedError
+        raise RMT::Lockfile::ExecutionLockedError
       else
         dirname = File.dirname(LOCKFILE_LOCATION)
         FileUtils.mkdir_p(dirname) unless File.directory?(dirname)
         File.open(LOCKFILE_LOCATION, 'w') { |f| f.write(Process.pid) }
-        true
       end
     end
 
     def remove_file
       File.delete(LOCKFILE_LOCATION) if File.exist?(LOCKFILE_LOCATION)
-
-      true
     end
   end
 end

lib/rmt/mirror.rb

@@ -27,7 +27,6 @@ def initialize(mirroring_base_dir:, repository_url:, local_path:, logger:, mirro
   end
 
   def mirror
-    RMT::Lockfile.create_file
     create_directories
     mirror_license
     # downloading license doesn't require an auth token
@@ -37,7 +36,6 @@ def mirror
 
     replace_directory(@temp_licenses_dir, File.join(@repository_dir, '../product.license/')) if Dir.exist?(@temp_licenses_dir)
     replace_directory(File.join(@temp_metadata_dir, 'repodata'), File.join(@repository_dir, 'repodata'))
-    RMT::Lockfile.remove_file
   ensure
     remove_tmp_directories
   end
@@ -69,7 +67,6 @@ def create_directories
       @temp_licenses_dir = Dir.mktmpdir
       @temp_metadata_dir = Dir.mktmpdir
     rescue StandardError => e
-      RMT::Lockfile.remove_file
       raise RMT::Mirror::Exception.new("Can not create a temporary directory: #{e}")
     end
   end
@@ -82,15 +79,13 @@ def mirror_metadata
     begin
       local_filename = @downloader.download('repodata/repomd.xml')
     rescue RMT::Downloader::Exception => e
-      RMT::Lockfile.remove_file
       raise RMT::Mirror::Exception.new("Repodata download failed: #{e}")
     end
 
     begin
       @downloader.download('repodata/repomd.xml.key')
       @downloader.download('repodata/repomd.xml.asc')
     rescue RMT::Downloader::Exception
-      RMT::Lockfile.remove_file
       @logger.info('Repository metadata signatures are missing')
     end
 
@@ -124,7 +119,6 @@ def mirror_license
     rescue RMT::Downloader::Exception
       FileUtils.remove_entry(@temp_licenses_dir)
       @logger.info('No product license found')
-      RMT::Lockfile.remove_file
       return
     end
 
@@ -135,7 +129,6 @@ def mirror_license
         @downloader.download(filename)
       end
     rescue RMT::Downloader::Exception => e
-      RMT::Lockfile.remove_file
       raise RMT::Mirror::Exception.new("Error during mirroring metadata: #{e.message}")
     end
   end

lib/rmt/scc.rb

@@ -13,7 +13,6 @@ def initialize(options = {})
   end
 
   def sync
-    RMT::Lockfile.create_file
     credentials_set? || (raise CredentialsError, 'SCC credentials not set.')
 
     cleanup_database
@@ -32,7 +31,6 @@ def sync
     Repository.remove_suse_repos_without_tokens!
 
     update_subscriptions(scc_api_client.list_subscriptions)
-    RMT::Lockfile.remove_file
   end
 
   def export(path)
@@ -58,7 +56,6 @@ def export(path)
   end
 
   def import(path)
-    RMT::Lockfile.create_file
     missing_files = %w[products repositories subscriptions]
       .map { |data| "organizations_#{data}.json" }
       .reject { |filename| File.exist?(File.join(path, filename)) }
@@ -81,7 +78,6 @@ def import(path)
     Repository.remove_suse_repos_without_tokens!
 
     update_subscriptions(JSON.parse(File.read(File.join(path, 'organizations_subscriptions.json')), symbolize_names: true))
-    RMT::Lockfile.remove_file
   end
 
   protected

spec/lib/rmt/cli/import_spec.rb

@@ -16,6 +16,30 @@
         command
       end
     end
+
+    context 'with existing lockfile' do
+      before { allow(RMT::Lockfile).to receive(:create_file).and_raise(RMT::Lockfile::ExecutionLockedError) }
+
+      it 'handles lockfile exception' do
+        FakeFS.with_fresh do
+          FileUtils.mkdir_p path
+
+          expect { command }.to output(/Process is locked/).to_stdout.and output('').to_stderr
+        end
+      end
+    end
+
+    context 'with unexpected error being raised' do
+      it 'removes lockfile and re-raises error' do
+        FakeFS.with_fresh do
+          FileUtils.mkdir_p path
+          allow_any_instance_of(RMT::SCC).to receive(:import).with(path).and_raise(RuntimeError)
+          expect(RMT::Lockfile).to receive(:remove_file)
+
+          expect { command }.to raise_error(RuntimeError)
+        end
+      end
+    end
   end
 
   describe 'repos' do
@@ -28,6 +52,12 @@
     let(:repo1_local_path) { repo_url_to_local_path(path, repo1.external_url) }
     let(:repo2_local_path) { repo_url_to_local_path(path, repo2.external_url) }
     let(:mirror_double) { instance_double 'RMT::Mirror' }
+    let(:repo_settings) do
+      [
+        { url: repo1.external_url, auth_token: repo1.auth_token.to_s },
+        { url: repo2.external_url, auth_token: repo2.auth_token.to_s }
+      ]
+    end
 
     context 'no repos.json file' do
       it 'warns that repos.json does not exist' do
@@ -58,13 +88,6 @@
     end
 
     context 'without exception' do
-      let(:repo_settings) do
-        [
-          { url: repo1.external_url, auth_token: repo1.auth_token.to_s },
-          { url: repo2.external_url, auth_token: repo2.auth_token.to_s }
-        ]
-      end
-
       before do
         expect(mirror_double).to receive(:mirror).twice
         expect(RMT::Mirror).to receive(:from_url).with(repo1_local_path, repo1.auth_token, base_dir: RMT::DEFAULT_MIRROR_DIR,
@@ -94,14 +117,7 @@
 
 
     context 'with existing lockfile' do
-      let(:repo_settings) do
-        [
-          { url: repo1.external_url, auth_token: repo1.auth_token.to_s },
-          { url: repo2.external_url, auth_token: repo2.auth_token.to_s }
-        ]
-      end
-
-      before { allow(RMT::Lockfile).to receive(:create_file).and_raise(RMT::ExecutionLockedError) }
+      before { allow(RMT::Lockfile).to receive(:create_file).and_raise(RMT::Lockfile::ExecutionLockedError) }
 
       it 'handles lockfile exception' do
         FakeFS.with_fresh do
@@ -113,6 +129,19 @@
       end
     end
 
+    context 'with unexpected error being raised' do
+      it 'removes lockfile and re-raises error' do
+        FakeFS.with_fresh do
+          FileUtils.mkdir_p path
+          File.write("#{path}/repos.json", repo_settings.to_json)
+          allow(Repository).to receive(:make_local_path).and_raise(RuntimeError)
+          expect(RMT::Lockfile).to receive(:remove_file)
+
+          expect { command }.to raise_error(RuntimeError)
+        end
+      end
+    end
+
     context 'with exceptions during mirroring' do
       let(:mirror_error_double) { instance_double 'RMT::Mirror' }
       let(:repo_settings) do

spec/lib/rmt/cli/main_spec.rb

@@ -5,6 +5,12 @@
 RSpec.describe RMT::CLI::Main do
   subject(:command) { described_class.start(argv) }
 
+  around do |example|
+    FakeFS.with_fresh do
+      example.run
+    end
+  end
+
   let(:argv) { [] }
 
   describe '.start' do
@@ -20,7 +26,7 @@
 
       context 'with execution locked exception thrown' do
         it do
-          allow_any_instance_of(RMT::SCC).to receive(:sync).and_raise(RMT::ExecutionLockedError)
+          allow(RMT::Lockfile).to receive(:create_file).and_raise(RMT::Lockfile::ExecutionLockedError)
           expect { command }.to output("Process is locked\n").to_stdout
         end
       end
@@ -33,8 +39,10 @@
         before { create :repository, :with_products, mirroring_enabled: false }
 
         it 'outputs a warning' do
-          expect_any_instance_of(RMT::Mirror).not_to receive(:mirror)
-          expect { command }.to output("There are no repositories marked for mirroring.\n").to_stderr.and output('').to_stdout
+          FakeFS.with_fresh do
+            expect_any_instance_of(RMT::Mirror).not_to receive(:mirror)
+            expect { command }.to output("There are no repositories marked for mirroring.\n").to_stderr.and output('').to_stdout
+          end
         end
       end
 
@@ -67,13 +75,24 @@
         let!(:repository) { create :repository, :with_products, mirroring_enabled: true } # rubocop:disable RSpec/LetSetup
 
         before do
-          allow(RMT::Lockfile).to receive(:create_file).and_raise(RMT::ExecutionLockedError)
+          allow(RMT::Lockfile).to receive(:create_file).and_raise(RMT::Lockfile::ExecutionLockedError)
         end
 
         it do
           expect { command }.to output(/Process is locked\n/).to_stdout
         end
       end
+
+      context 'with unexpected error being raised' do
+        it 'removes lockfile and re-raises error' do
+          FakeFS.with_fresh do
+            allow(Repository).to receive(:where).and_raise(RuntimeError)
+            expect(RMT::Lockfile).to receive(:remove_file)
+
+            expect { command }.to raise_error(RuntimeError)
+          end
+        end
+      end
     end
 
     describe 'help' do