From cc412b73bccfa140a1a3eb34c8fc36b5979427ab Mon Sep 17 00:00:00 2001
From: Felix Auringer <48409110+felixauringer@users.noreply.github.com>
Date: Thu, 24 Jun 2021 12:42:43 +0200
Subject: [PATCH] Introduce more error types

---
 app/errors/runner/error.rb                     |  6 ++++++
 lib/runner/strategy/docker_container_pool.rb   | 14 +++++++-------
 lib/runner/strategy/poseidon.rb                | 18 +++++++++---------
 .../strategy/docker_container_pool_spec.rb     | 14 +++++++-------
 spec/lib/runner/strategy/poseidon_spec.rb      | 12 ++++++------
 5 files changed, 35 insertions(+), 29 deletions(-)

diff --git a/app/errors/runner/error.rb b/app/errors/runner/error.rb
index ba6aeee1..41b54c31 100644
--- a/app/errors/runner/error.rb
+++ b/app/errors/runner/error.rb
@@ -16,6 +16,12 @@ class Runner
 
     class RunnerNotFound < Error; end
 
+    class FaradayError < Error; end
+
+    class UnexpectedResponse < Error; end
+
+    class WorkspaceError < Error; end
+
     class Unknown < Error; end
   end
 end
diff --git a/lib/runner/strategy/docker_container_pool.rb b/lib/runner/strategy/docker_container_pool.rb
index 7003f901..84d5219e 100644
--- a/lib/runner/strategy/docker_container_pool.rb
+++ b/lib/runner/strategy/docker_container_pool.rb
@@ -12,9 +12,9 @@ class Runner::Strategy::DockerContainerPool < Runner::Strategy
     container_id = JSON.parse(Faraday.get("#{config[:pool][:location]}/docker_container_pool/get_container/#{environment.id}").body)['id']
     container_id.presence || raise(Runner::Error::NotAvailable.new("DockerContainerPool didn't return a container id"))
   rescue Faraday::Error => e
-    raise Runner::Error::Unknown.new("Faraday request to DockerContainerPool failed: #{e.inspect}")
+    raise Runner::Error::FaradayError.new("Request to DockerContainerPool failed: #{e.inspect}")
   rescue JSON::ParserError => e
-    raise Runner::Error::Unknown.new("DockerContainerPool returned invalid JSON: #{e.inspect}")
+    raise Runner::Error::UnexpectedResponse.new("DockerContainerPool returned invalid JSON: #{e.inspect}")
   end
 
   def initialize(runner_id, _environment)
@@ -40,7 +40,7 @@ class Runner::Strategy::DockerContainerPool < Runner::Strategy
         rescue IOError => e
           # TODO: try catch i/o exception and log failed attempts
           # Does this fix the issue @Sebastian? What exceptions did you have in mind?
-          raise Runner::Error::Unknown.new("Could not create workspace file #{file.filepath}: #{e.inspect}")
+          raise Runner::Error::WorkspaceError.new("Could not create file #{file.filepath}: #{e.inspect}")
         end
       end
     end
@@ -50,7 +50,7 @@ class Runner::Strategy::DockerContainerPool < Runner::Strategy
   def destroy_at_management
     Faraday.get("#{self.class.config[:pool][:location]}/docker_container_pool/destroy_container/#{container.id}")
   rescue Faraday::Error => e
-    raise Runner::Error::Unknown.new("Faraday request to DockerContainerPool failed: #{e.inspect}")
+    raise Runner::Error::FaradayError.new("Request to DockerContainerPool failed: #{e.inspect}")
   end
 
   def attach_to_execution(command)
@@ -88,7 +88,7 @@ class Runner::Strategy::DockerContainerPool < Runner::Strategy
     unclean_path = local_workspace_path.join(path)
     clean_path = File.expand_path(unclean_path)
     unless clean_path.to_s.start_with? local_workspace_path.to_s
-      raise Runner::Error::Unknown.new("Local filepath #{clean_path.inspect} not allowed")
+      raise Runner::Error::WorkspaceError.new("Local filepath #{clean_path.inspect} not allowed")
     end
 
     Pathname.new(clean_path)
@@ -97,10 +97,10 @@ class Runner::Strategy::DockerContainerPool < Runner::Strategy
   def clean_workspace
     FileUtils.rm_r(local_workspace_path.children, secure: true)
   rescue Errno::ENOENT => e
-    raise Runner::Error::Unknown.new("The workspace directory does not exist and cannot be deleted: #{e.inspect}")
+    raise Runner::Error::WorkspaceError.new("The workspace directory does not exist and cannot be deleted: #{e.inspect}")
   rescue Errno::EACCES => e
     # TODO: Why was this rescued before @Sebastian?
-    raise Runner::Error::Unknown.new("Not allowed to clean workspace #{local_workspace_path}: #{e.inspect}")
+    raise Runner::Error::WorkspaceError.new("Not allowed to clean workspace #{local_workspace_path}: #{e.inspect}")
   end
 
   def local_workspace_path
diff --git a/lib/runner/strategy/poseidon.rb b/lib/runner/strategy/poseidon.rb
index 1938c229..fba0b475 100644
--- a/lib/runner/strategy/poseidon.rb
+++ b/lib/runner/strategy/poseidon.rb
@@ -23,14 +23,14 @@ class Runner::Strategy::Poseidon < Runner::Strategy
       when 200
         response_body = parse response
         runner_id = response_body[:runnerId]
-        runner_id.presence || raise(Runner::Error::Unknown.new('Poseidon did not send a runner id'))
+        runner_id.presence || raise(Runner::Error::UnexpectedResponse.new('Poseidon did not send a runner id'))
       when 404
         raise Runner::Error::EnvironmentNotFound.new
       else
         handle_error response
     end
   rescue Faraday::Error => e
-    raise Runner::Error::Unknown.new("Faraday request to runner management failed: #{e.inspect}")
+    raise Runner::Error::FaradayError.new("Request to Poseidon failed: #{e.inspect}")
   end
 
   def self.handle_error(response)
@@ -51,7 +51,7 @@ class Runner::Strategy::Poseidon < Runner::Strategy
           raise Runner::Error::InternalServerError.new("Poseidon sent #{response_body[:errorCode]}: #{response_body[:message]}")
         end
       else
-        raise Runner::Error::Unknown.new("Poseidon sent unexpected response status code #{response.status}")
+        raise Runner::Error::UnexpectedResponse.new("Poseidon sent unexpected response status code #{response.status}")
     end
   end
 
@@ -59,7 +59,7 @@ class Runner::Strategy::Poseidon < Runner::Strategy
     JSON.parse(response.body).deep_symbolize_keys
   rescue JSON::ParserError => e
     # Poseidon should not send invalid json
-    raise Runner::Error::Unknown.new("Error parsing response from Poseidon: #{e.message}")
+    raise Runner::Error::UnexpectedResponse.new("Error parsing response from Poseidon: #{e.message}")
   end
 
   def initialize(runner_id, _environment)
@@ -82,7 +82,7 @@ class Runner::Strategy::Poseidon < Runner::Strategy
     Runner.destroy(@allocation_id) if response.status == 400
     self.class.handle_error response
   rescue Faraday::Error => e
-    raise Runner::Error::Unknown.new("Faraday request to runner management failed: #{e.inspect}")
+    raise Runner::Error::FaradayError.new("Request to Poseidon failed: #{e.inspect}")
   end
 
   def attach_to_execution(command)
@@ -99,7 +99,7 @@ class Runner::Strategy::Poseidon < Runner::Strategy
     response = Faraday.delete runner_url
     self.class.handle_error response unless response.status == 204
   rescue Faraday::Error => e
-    raise Runner::Error::Unknown.new("Faraday request to runner management failed: #{e.inspect}")
+    raise Runner::Error::FaradayError.new("Request to Poseidon failed: #{e.inspect}")
   end
 
   private
@@ -115,7 +115,7 @@ class Runner::Strategy::Poseidon < Runner::Strategy
         if websocket_url.present?
           return websocket_url
         else
-          raise Runner::Error::Unknown.new('Poseidon did not send websocket url')
+          raise Runner::Error::UnexpectedResponse.new('Poseidon did not send websocket url')
         end
       when 400
         Runner.destroy(@allocation_id)
@@ -123,7 +123,7 @@ class Runner::Strategy::Poseidon < Runner::Strategy
 
     self.class.handle_error response
   rescue Faraday::Error => e
-    raise Runner::Error::Unknown.new("Faraday request to runner management failed: #{e.inspect}")
+    raise Runner::Error::FaradayError.new("Request to Poseidon failed: #{e.inspect}")
   end
 
   def runner_url
@@ -134,7 +134,7 @@ class Runner::Strategy::Poseidon < Runner::Strategy
     def decode(raw_event)
       JSON.parse(raw_event.data)
     rescue JSON::ParserError => e
-      raise Runner::Error::Unknown.new("The websocket message from Poseidon could not be decoded to JSON: #{e.inspect}")
+      raise Runner::Error::UnexpectedResponse.new("The websocket message from Poseidon could not be decoded to JSON: #{e.inspect}")
     end
 
     def encode(data)
diff --git a/spec/lib/runner/strategy/docker_container_pool_spec.rb b/spec/lib/runner/strategy/docker_container_pool_spec.rb
index d5c5a455..c8dabb91 100644
--- a/spec/lib/runner/strategy/docker_container_pool_spec.rb
+++ b/spec/lib/runner/strategy/docker_container_pool_spec.rb
@@ -20,7 +20,7 @@ describe Runner::Strategy::DockerContainerPool do
   shared_examples 'Faraday error handling' do
     it 'raises a runner error' do
       allow(Faraday).to receive(:get).and_raise(Faraday::TimeoutError)
-      expect { action.call }.to raise_error(Runner::Error::Unknown, /Faraday/)
+      expect { action.call }.to raise_error(Runner::Error::FaradayError)
     end
   end
 
@@ -59,7 +59,7 @@ describe Runner::Strategy::DockerContainerPool do
       let(:response_body) { '{hello}' }
 
       it 'raises an error' do
-        expect { action.call }.to raise_error(Runner::Error::Unknown)
+        expect { action.call }.to raise_error(Runner::Error::UnexpectedResponse)
       end
     end
 
@@ -128,7 +128,7 @@ describe Runner::Strategy::DockerContainerPool do
 
       it 'raises an error in case of an IOError' do
         allow(File).to receive(:open).and_raise(IOError)
-        expect { container_pool.copy_files(files) }.to raise_error(Runner::Error::Unknown, /#{files.first.filepath}/)
+        expect { container_pool.copy_files(files) }.to raise_error(Runner::Error::WorkspaceError, /#{files.first.filepath}/)
       end
 
       it 'does not create a directory for it' do
@@ -195,11 +195,11 @@ describe Runner::Strategy::DockerContainerPool do
     before { allow(container_pool).to receive(:local_workspace_path).and_return(local_workspace) }
 
     it 'raises an error for relative paths outside of the workspace' do
-      expect { container_pool.send(:local_path, '../exercise.py') }.to raise_error(Runner::Error::Unknown, %r{tmp/exercise.py})
+      expect { container_pool.send(:local_path, '../exercise.py') }.to raise_error(Runner::Error::WorkspaceError, %r{tmp/exercise.py})
     end
 
     it 'raises an error for absolute paths outside of the workspace' do
-      expect { container_pool.send(:local_path, '/test') }.to raise_error(Runner::Error::Unknown, %r{/test})
+      expect { container_pool.send(:local_path, '/test') }.to raise_error(Runner::Error::WorkspaceError, %r{/test})
     end
 
     it 'removes .. from the path' do
@@ -225,13 +225,13 @@ describe Runner::Strategy::DockerContainerPool do
 
     it 'raises an error if the workspace does not exist' do
       allow(local_workspace).to receive(:children).and_raise(Errno::ENOENT)
-      expect { container_pool.send(:clean_workspace) }.to raise_error(Runner::Error::Unknown, /not exist/)
+      expect { container_pool.send(:clean_workspace) }.to raise_error(Runner::Error::WorkspaceError, /not exist/)
     end
 
     it 'raises an error if it lacks permission for deleting an entry' do
       allow(local_workspace).to receive(:children).and_return(['test.py'])
       allow(FileUtils).to receive(:remove_entry_secure).and_raise(Errno::EACCES)
-      expect { container_pool.send(:clean_workspace) }.to raise_error(Runner::Error::Unknown, /Not allowed/)
+      expect { container_pool.send(:clean_workspace) }.to raise_error(Runner::Error::WorkspaceError, /Not allowed/)
     end
   end
 
diff --git a/spec/lib/runner/strategy/poseidon_spec.rb b/spec/lib/runner/strategy/poseidon_spec.rb
index add7037e..3de5d957 100644
--- a/spec/lib/runner/strategy/poseidon_spec.rb
+++ b/spec/lib/runner/strategy/poseidon_spec.rb
@@ -95,7 +95,7 @@ describe Runner::Strategy::Poseidon do
       let(:response_status) { 1337 }
 
       it 'raises an error' do
-        expect { action.call }.to raise_error(Runner::Error::Unknown, /#{response_status}/)
+        expect { action.call }.to raise_error(Runner::Error::UnexpectedResponse, /#{response_status}/)
       end
     end
   end
@@ -109,7 +109,7 @@ describe Runner::Strategy::Poseidon do
 
       it 'raises an error' do
         %i[post patch delete].each {|message| allow(Faraday).to receive(message).and_raise(Faraday::TimeoutError) }
-        expect { action.call }.to raise_error(Runner::Error::Unknown, /Faraday/)
+        expect { action.call }.to raise_error(Runner::Error::FaradayError)
       end
     end
   end
@@ -146,7 +146,7 @@ describe Runner::Strategy::Poseidon do
       let(:response_status) { 200 }
 
       it 'raises an error' do
-        expect { action.call }.to raise_error(Runner::Error::Unknown)
+        expect { action.call }.to raise_error(Runner::Error::UnexpectedResponse)
       end
     end
 
@@ -155,7 +155,7 @@ describe Runner::Strategy::Poseidon do
       let(:response_status) { 200 }
 
       it 'raises an error' do
-        expect { action.call }.to raise_error(Runner::Error::Unknown)
+        expect { action.call }.to raise_error(Runner::Error::UnexpectedResponse)
       end
     end
 
@@ -209,7 +209,7 @@ describe Runner::Strategy::Poseidon do
       let(:response_status) { 200 }
 
       it 'raises an error' do
-        expect { action.call }.to raise_error(Runner::Error::Unknown)
+        expect { action.call }.to raise_error(Runner::Error::UnexpectedResponse)
       end
     end
 
@@ -218,7 +218,7 @@ describe Runner::Strategy::Poseidon do
       let(:response_status) { 200 }
 
       it 'raises an error' do
-        expect { action.call }.to raise_error(Runner::Error::Unknown)
+        expect { action.call }.to raise_error(Runner::Error::UnexpectedResponse)
       end
     end