From 38e1f5b48664f92e62a4782b3840aef06ed001ac Mon Sep 17 00:00:00 2001
From: Sebastian Serth <Sebastian.Serth@hpi.de>
Date: Thu, 4 Nov 2021 01:00:54 +0100
Subject: [PATCH 1/9] Show error details of available_images

---
 app/controllers/execution_environments_controller.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/controllers/execution_environments_controller.rb b/app/controllers/execution_environments_controller.rb
index 7ae70014..889dfe71 100644
--- a/app/controllers/execution_environments_controller.rb
+++ b/app/controllers/execution_environments_controller.rb
@@ -135,8 +135,8 @@ class ExecutionEnvironmentsController < ApplicationController
   def set_docker_images
     @docker_images ||= ExecutionEnvironment.pluck(:docker_image)
     @docker_images += Runner.strategy_class.available_images
-  rescue Runner::Error::InternalServerError => e
-    flash[:warning] = e.message
+  rescue Runner::Error => e
+    flash[:warning] = html_escape e.message
   ensure
     @docker_images = @docker_images.sort.uniq
   end

From ecf470bddd10e9dbfa8bb1e56f575cc30ef5eadd Mon Sep 17 00:00:00 2001
From: Sebastian Serth <Sebastian.Serth@hpi.de>
Date: Thu, 4 Nov 2021 01:03:12 +0100
Subject: [PATCH 2/9] Refactor sync_environment methods

* Add delete_environment method
* Change return value to allow raising an exception
---
 .../execution_environments_controller.rb      |  2 +
 app/models/execution_environment.rb           | 29 ++++++++++++---
 app/models/runner.rb                          | 16 +++++---
 lib/runner/strategy.rb                        |  4 ++
 lib/runner/strategy/docker_container_pool.rb  | 37 ++++++++++++++-----
 lib/runner/strategy/null.rb                   |  8 +++-
 lib/runner/strategy/poseidon.rb               | 22 +++++++++--
 .../execution_environments_controller_spec.rb |  9 ++++-
 spec/lib/runner/strategy/poseidon_spec.rb     |  8 ++--
 spec/models/runner_spec.rb                    |  2 +-
 10 files changed, 106 insertions(+), 31 deletions(-)

diff --git a/app/controllers/execution_environments_controller.rb b/app/controllers/execution_environments_controller.rb
index 889dfe71..1ee81ad4 100644
--- a/app/controllers/execution_environments_controller.rb
+++ b/app/controllers/execution_environments_controller.rb
@@ -177,6 +177,8 @@ class ExecutionEnvironmentsController < ApplicationController
 
     success = ExecutionEnvironment.all.map do |execution_environment|
       Runner.strategy_class.sync_environment(execution_environment)
+    rescue Runner::Error
+      false
     end
     if success.all?
       redirect_to ExecutionEnvironment, notice: t('execution_environments.index.synchronize_all.success')
diff --git a/app/models/execution_environment.rb b/app/models/execution_environment.rb
index 247accc8..c51d697a 100644
--- a/app/models/execution_environment.rb
+++ b/app/models/execution_environment.rb
@@ -33,10 +33,11 @@ class ExecutionEnvironment < ApplicationRecord
   before_validation :clean_exposed_ports
   validates :exposed_ports, array: {numericality: {greater_than_or_equal_to: 0, less_than: 65_536, only_integer: true}}
 
-  def set_default_values
-    set_default_values_if_present(permitted_execution_time: 60, pool_size: 0)
-  end
-  private :set_default_values
+  after_destroy :delete_runner_environment
+  after_save :working_docker_image?, if: :validate_docker_image?
+
+  after_rollback :delete_runner_environment, on: :create
+  after_rollback :sync_runner_environment, on: %i[update destroy]
 
   def to_s
     name
@@ -86,5 +87,23 @@ class ExecutionEnvironment < ApplicationRecord
   rescue Runner::Error => e
     errors.add(:docker_image, "error: #{e}")
   end
-  private :working_docker_image?
+
+  def delete_runner_environment
+    Runner.strategy_class.remove_environment(self)
+  rescue Runner::Error => e
+    unless errors.include?(:docker_image)
+      errors.add(:docker_image, "error: #{e}")
+      raise ActiveRecord::RecordInvalid.new(self)
+    end
+  end
+
+  def sync_runner_environment
+    previous_saved_environment = self.class.find(id)
+    Runner.strategy_class.sync_environment(previous_saved_environment)
+  rescue Runner::Error => e
+    unless errors.include?(:docker_image)
+      errors.add(:docker_image, "error: #{e}")
+      raise ActiveRecord::RecordInvalid.new(self)
+    end
+  end
 end
diff --git a/app/models/runner.rb b/app/models/runner.rb
index 6e308e4f..3e005185 100644
--- a/app/models/runner.rb
+++ b/app/models/runner.rb
@@ -140,16 +140,20 @@ class Runner < ApplicationRecord
     rescue Runner::Error::EnvironmentNotFound
       # Whenever the environment could not be found by the runner management, we
       # try to synchronize it and then forward a more specific error to our callee.
-      if strategy_class.sync_environment(execution_environment)
+      begin
+        strategy_class.sync_environment(execution_environment)
+      rescue Runner::Error
+        # An additional error was raised during synchronization
+        raise Runner::Error::EnvironmentNotFound.new(
+          "The execution environment with id #{execution_environment.id} was not found by the runner management. "\
+          'In addition, it could not be synced so that this probably indicates a permanent error.'
+        )
+      else
+        # No error was raised during synchronization
         raise Runner::Error::EnvironmentNotFound.new(
           "The execution environment with id #{execution_environment.id} was not found yet by the runner management. "\
           'It has been successfully synced now so that the next request should be successful.'
         )
-      else
-        raise Runner::Error::EnvironmentNotFound.new(
-          "The execution environment with id #{execution_environment.id} was not found by the runner management."\
-          'In addition, it could not be synced so that this probably indicates a permanent error.'
-        )
       end
     end
   end
diff --git a/lib/runner/strategy.rb b/lib/runner/strategy.rb
index ce58a947..2a03f39c 100644
--- a/lib/runner/strategy.rb
+++ b/lib/runner/strategy.rb
@@ -13,6 +13,10 @@ class Runner::Strategy
     raise NotImplementedError
   end
 
+  def self.remove_environment(_environment)
+    raise NotImplementedError
+  end
+
   def self.request_from_management(_environment)
     raise NotImplementedError
   end
diff --git a/lib/runner/strategy/docker_container_pool.rb b/lib/runner/strategy/docker_container_pool.rb
index 42c66f66..d97c5fce 100644
--- a/lib/runner/strategy/docker_container_pool.rb
+++ b/lib/runner/strategy/docker_container_pool.rb
@@ -15,16 +15,35 @@ class Runner::Strategy::DockerContainerPool < Runner::Strategy
   end
 
   def self.sync_environment(environment)
-    # There is no dedicated sync mechanism yet. However, we need to emit a warning when the pool was previously
-    # empty for this execution environment. In this case the validation command probably was not executed.
-    return true unless environment.pool_size_previously_changed?
-
-    case environment.pool_size_previously_was
-      when nil, 0
-        false
-      else
-        true
+    # Force a database commit and start a new transaction.
+    if environment.class.connection.transaction_open?
+      environment.class.connection.commit_db_transaction
+      environment.class.connection.begin_db_transaction
     end
+
+    url = "#{config[:url]}/docker_container_pool/refill_environment/#{environment.id}"
+    Rails.logger.debug { "#{Time.zone.now.getutc.inspect}: Refilling execution environment at #{url}" }
+    response = Faraday.post(url)
+    return true if response.success?
+
+    raise Runner::Error::UnexpectedResponse.new("Could not refill execution environment in DockerContainerPool, got response: #{response.as_json}")
+  rescue Faraday::Error => e
+    raise Runner::Error::FaradayError.new("Request to DockerContainerPool failed: #{e.inspect}")
+  ensure
+    Rails.logger.debug { "#{Time.zone.now.getutc.inspect}: Finished refilling environment" }
+  end
+
+  def self.remove_environment(environment)
+    url = "#{config[:url]}/docker_container_pool/purge_environment/#{environment.id}"
+    Rails.logger.debug { "#{Time.zone.now.getutc.inspect}: Purging execution environment at #{url}" }
+    response = Faraday.delete(url)
+    return true if response.success?
+
+    raise Runner::Error::UnexpectedResponse.new("Could not delete execution environment in DockerContainerPool, got response: #{response.as_json}")
+  rescue Faraday::Error => e
+    raise Runner::Error::FaradayError.new("Request to DockerContainerPool failed: #{e.inspect}")
+  ensure
+    Rails.logger.debug { "#{Time.zone.now.getutc.inspect}: Finished purging environment" }
   end
 
   def self.request_from_management(environment)
diff --git a/lib/runner/strategy/null.rb b/lib/runner/strategy/null.rb
index 38ff522f..1c5f34a8 100644
--- a/lib/runner/strategy/null.rb
+++ b/lib/runner/strategy/null.rb
@@ -5,7 +5,13 @@
 class Runner::Strategy::Null < Runner::Strategy
   def self.initialize_environment; end
 
-  def self.sync_environment(_environment); end
+  def self.sync_environment(_environment)
+    raise Runner::Error.new
+  end
+
+  def self.remove_environment(_environment)
+    raise Runner::Error.new
+  end
 
   def self.request_from_management(_environment)
     SecureRandom.uuid
diff --git a/lib/runner/strategy/poseidon.rb b/lib/runner/strategy/poseidon.rb
index c84365bf..8a83e62f 100644
--- a/lib/runner/strategy/poseidon.rb
+++ b/lib/runner/strategy/poseidon.rb
@@ -21,14 +21,28 @@ class Runner::Strategy::Poseidon < Runner::Strategy
 
   def self.sync_environment(environment)
     url = "#{config[:url]}/execution-environments/#{environment.id}"
+    Rails.logger.debug { "#{Time.zone.now.getutc.inspect}: Synchronizing execution environment at #{url}" }
     response = http_connection.put url, environment.to_json
     return true if [201, 204].include? response.status
 
-    Rails.logger.warn("Could not create execution environment in Poseidon, got response: #{response.as_json}")
-    false
+    raise Runner::Error::UnexpectedResponse.new("Could not synchronize execution environment in Poseidon, got response: #{response.as_json}")
   rescue Faraday::Error => e
-    Rails.logger.warn("Could not create execution environment because of Faraday error: #{e.inspect}")
-    false
+    raise Runner::Error::FaradayError.new("Could not synchronize execution environment because of Faraday error: #{e.inspect}")
+  ensure
+    Rails.logger.debug { "#{Time.zone.now.getutc.inspect}: Finished synchronizing execution environment" }
+  end
+
+  def self.remove_environment(environment)
+    url = "#{config[:url]}/execution-environments/#{environment.id}"
+    Rails.logger.debug { "#{Time.zone.now.getutc.inspect}: Deleting execution environment at #{url}" }
+    response = http_connection.delete url
+    return true if response.status == 204
+
+    raise Runner::Error::UnexpectedResponse.new("Could not delete execution environment in Poseidon, got response: #{response.as_json}")
+  rescue Faraday::Error => e
+    raise Runner::Error::FaradayError.new("Could not delete execution environment because of Faraday error: #{e.inspect}")
+  ensure
+    Rails.logger.debug { "#{Time.zone.now.getutc.inspect}: Finished deleting execution environment" }
   end
 
   def self.request_from_management(environment)
diff --git a/spec/controllers/execution_environments_controller_spec.rb b/spec/controllers/execution_environments_controller_spec.rb
index 2e3de1d3..b2b376ec 100644
--- a/spec/controllers/execution_environments_controller_spec.rb
+++ b/spec/controllers/execution_environments_controller_spec.rb
@@ -46,7 +46,10 @@ describe ExecutionEnvironmentsController do
   end
 
   describe 'DELETE #destroy' do
-    before { delete :destroy, params: {id: execution_environment.id} }
+    before do
+      allow(Runner.strategy_class).to receive(:remove_environment).and_return(true)
+      delete :destroy, params: {id: execution_environment.id}
+    end
 
     expect_assigns(execution_environment: :execution_environment)
 
@@ -55,6 +58,10 @@ describe ExecutionEnvironmentsController do
       expect { delete :destroy, params: {id: execution_environment.id} }.to change(ExecutionEnvironment, :count).by(-1)
     end
 
+    it 'removes the execution environment from the runner management' do
+      expect(Runner.strategy_class).to have_received(:remove_environment)
+    end
+
     expect_redirect(:execution_environments)
   end
 
diff --git a/spec/lib/runner/strategy/poseidon_spec.rb b/spec/lib/runner/strategy/poseidon_spec.rb
index cec90a31..fb31429b 100644
--- a/spec/lib/runner/strategy/poseidon_spec.rb
+++ b/spec/lib/runner/strategy/poseidon_spec.rb
@@ -141,11 +141,11 @@ describe Runner::Strategy::Poseidon do
     end
 
     shared_examples 'returns false when the api request failed' do |status|
-      it "returns false on status #{status}" do
+      it "raises an exception on status #{status}" do
         faraday_connection = instance_double 'Faraday::Connection'
         allow(described_class).to receive(:http_connection).and_return(faraday_connection)
         allow(faraday_connection).to receive(:put).and_return(Faraday::Response.new(status: status))
-        expect(action.call).to be_falsey
+        expect { action.call }.to raise_exception Runner::Error::UnexpectedResponse
       end
     end
 
@@ -157,11 +157,11 @@ describe Runner::Strategy::Poseidon do
       include_examples 'returns false when the api request failed', status
     end
 
-    it 'returns false if Faraday raises an error' do
+    it 'raises an exception if Faraday raises an error' do
       faraday_connection = instance_double 'Faraday::Connection'
       allow(described_class).to receive(:http_connection).and_return(faraday_connection)
       allow(faraday_connection).to receive(:put).and_raise(Faraday::TimeoutError)
-      expect(action.call).to be_falsey
+      expect { action.call }.to raise_exception Runner::Error::FaradayError
     end
   end
 
diff --git a/spec/models/runner_spec.rb b/spec/models/runner_spec.rb
index c5d7bf90..8c156555 100644
--- a/spec/models/runner_spec.rb
+++ b/spec/models/runner_spec.rb
@@ -233,7 +233,7 @@ describe Runner do
       end
 
       it 'raises an error when the environment could not be synced' do
-        allow(strategy_class).to receive(:sync_environment).with(runner.execution_environment).and_return(false)
+        allow(strategy_class).to receive(:sync_environment).with(runner.execution_environment).and_raise(Runner::Error::EnvironmentNotFound)
         expect { runner.send(:request_new_id) }.to raise_error(Runner::Error::EnvironmentNotFound, /#{environment_id}.*could not be synced/)
       end
     end

From c1ac401a49fe612f6a5ff73707db1b2b5648a8f5 Mon Sep 17 00:00:00 2001
From: Sebastian Serth <Sebastian.Serth@hpi.de>
Date: Thu, 4 Nov 2021 01:04:52 +0100
Subject: [PATCH 3/9] Add retries to working_docker_image?

* Also synchronize images during save
---
 .../execution_environments_controller.rb      | 15 ++------
 app/models/execution_environment.rb           | 25 +++++++++----
 .../execution_environments_controller_spec.rb | 36 ++++++++++++++-----
 spec/models/execution_environment_spec.rb     |  6 ++--
 4 files changed, 51 insertions(+), 31 deletions(-)

diff --git a/app/controllers/execution_environments_controller.rb b/app/controllers/execution_environments_controller.rb
index 1ee81ad4..4dc00502 100644
--- a/app/controllers/execution_environments_controller.rb
+++ b/app/controllers/execution_environments_controller.rb
@@ -15,9 +15,7 @@ class ExecutionEnvironmentsController < ApplicationController
   def create
     @execution_environment = ExecutionEnvironment.new(execution_environment_params)
     authorize!
-    create_and_respond(object: @execution_environment) do
-      sync_to_runner_management
-    end
+    create_and_respond(object: @execution_environment)
   end
 
   def destroy
@@ -165,9 +163,7 @@ class ExecutionEnvironmentsController < ApplicationController
   end
 
   def update
-    update_and_respond(object: @execution_environment, params: execution_environment_params) do
-      sync_to_runner_management
-    end
+    update_and_respond(object: @execution_environment, params: execution_environment_params)
   end
 
   def sync_all_to_runner_management
@@ -186,11 +182,4 @@ class ExecutionEnvironmentsController < ApplicationController
       redirect_to ExecutionEnvironment, alert: t('execution_environments.index.synchronize_all.failure')
     end
   end
-
-  def sync_to_runner_management
-    unless Runner.management_active? && Runner.strategy_class.sync_environment(@execution_environment)
-      t('execution_environments.form.errors.not_synced_to_runner_management')
-    end
-  end
-  private :sync_to_runner_management
 end
diff --git a/app/models/execution_environment.rb b/app/models/execution_environment.rb
index c51d697a..b5f16157 100644
--- a/app/models/execution_environment.rb
+++ b/app/models/execution_environment.rb
@@ -79,13 +79,24 @@ class ExecutionEnvironment < ApplicationRecord
   private :validate_docker_image?
 
   def working_docker_image?
-    runner = Runner.for(author, self)
-    output = runner.execute_command(VALIDATION_COMMAND)
-    errors.add(:docker_image, "error: #{output[:stderr]}") if output[:stderr].present?
-  rescue Runner::Error::NotAvailable => e
-    Rails.logger.info("The Docker image could not be verified: #{e}")
-  rescue Runner::Error => e
-    errors.add(:docker_image, "error: #{e}")
+    sync_runner_environment
+    retries = 0
+    begin
+      runner = Runner.for(author, self)
+      output = runner.execute_command(VALIDATION_COMMAND)
+      errors.add(:docker_image, "error: #{output[:stderr]}") if output[:stderr].present?
+    rescue Runner::Error => e
+      # In case of an Runner::Error, we retry multiple times before giving up.
+      # The time between each retry increases to allow the runner management to catch up.
+      if retries < 5 && !Rails.env.test?
+        retries += 1
+        sleep retries
+        retry
+      elsif errors.exclude?(:docker_image)
+        errors.add(:docker_image, "error: #{e}")
+        raise ActiveRecord::RecordInvalid.new(self)
+      end
+    end
   end
 
   def delete_runner_environment
diff --git a/spec/controllers/execution_environments_controller_spec.rb b/spec/controllers/execution_environments_controller_spec.rb
index b2b376ec..9aee3e5a 100644
--- a/spec/controllers/execution_environments_controller_spec.rb
+++ b/spec/controllers/execution_environments_controller_spec.rb
@@ -8,7 +8,6 @@ describe ExecutionEnvironmentsController do
 
   before do
     allow(controller).to receive(:current_user).and_return(user)
-    allow(controller).to receive(:sync_to_runner_management).and_return(nil)
     allow(Runner.strategy_class).to receive(:available_images).and_return([])
   end
 
@@ -16,7 +15,14 @@ describe ExecutionEnvironmentsController do
     context 'with a valid execution environment' do
       let(:perform_request) { proc { post :create, params: {execution_environment: FactoryBot.build(:ruby).attributes} } }
 
-      before { perform_request.call }
+      before do
+        allow(Rails.env).to receive(:test?).and_return(false, true)
+        allow(Runner.strategy_class).to receive(:sync_environment).and_return(true)
+        runner = instance_double 'runner'
+        allow(Runner).to receive(:for).and_return(runner)
+        allow(runner).to receive(:execute_command).and_return({})
+        perform_request.call
+      end
 
       expect_assigns(docker_images: Array)
       expect_assigns(execution_environment: ExecutionEnvironment)
@@ -26,21 +32,25 @@ describe ExecutionEnvironmentsController do
       end
 
       it 'registers the execution environment with the runner management' do
-        expect(controller).to have_received(:sync_to_runner_management)
+        expect(Runner.strategy_class).to have_received(:sync_environment)
       end
 
       expect_redirect(ExecutionEnvironment.last)
     end
 
     context 'with an invalid execution environment' do
-      before { post :create, params: {execution_environment: {}} }
+      before do
+        allow(Runner.strategy_class).to receive(:sync_environment).and_return(true)
+        allow(Rails.env).to receive(:test?).and_return(false, true)
+        post :create, params: {execution_environment: {}}
+      end
 
       expect_assigns(execution_environment: ExecutionEnvironment)
       expect_status(200)
       expect_template(:new)
 
       it 'does not register the execution environment with the runner management' do
-        expect(controller).not_to have_received(:sync_to_runner_management)
+        expect(Runner.strategy_class).not_to have_received(:sync_environment)
       end
     end
   end
@@ -171,7 +181,11 @@ describe ExecutionEnvironmentsController do
   describe 'PUT #update' do
     context 'with a valid execution environment' do
       before do
-        allow(controller).to receive(:sync_to_runner_management).and_return(nil)
+        allow(Rails.env).to receive(:test?).and_return(false, true)
+        allow(Runner.strategy_class).to receive(:sync_environment).and_return(true)
+        runner = instance_double 'runner'
+        allow(Runner).to receive(:for).and_return(runner)
+        allow(runner).to receive(:execute_command).and_return({})
         put :update, params: {execution_environment: FactoryBot.attributes_for(:ruby), id: execution_environment.id}
       end
 
@@ -180,19 +194,23 @@ describe ExecutionEnvironmentsController do
       expect_redirect(:execution_environment)
 
       it 'updates the execution environment at the runner management' do
-        expect(controller).to have_received(:sync_to_runner_management)
+        expect(Runner.strategy_class).to have_received(:sync_environment)
       end
     end
 
     context 'with an invalid execution environment' do
-      before { put :update, params: {execution_environment: {name: ''}, id: execution_environment.id} }
+      before do
+        allow(Runner.strategy_class).to receive(:sync_environment).and_return(true)
+        allow(Rails.env).to receive(:test?).and_return(true, false, true)
+        put :update, params: {execution_environment: {name: ''}, id: execution_environment.id}
+      end
 
       expect_assigns(execution_environment: ExecutionEnvironment)
       expect_status(200)
       expect_template(:edit)
 
       it 'does not update the execution environment at the runner management' do
-        expect(controller).not_to have_received(:sync_to_runner_management)
+        expect(Runner.strategy_class).not_to have_received(:sync_environment)
       end
     end
   end
diff --git a/spec/models/execution_environment_spec.rb b/spec/models/execution_environment_spec.rb
index 5b41f4b8..8bfb0b07 100644
--- a/spec/models/execution_environment_spec.rb
+++ b/spec/models/execution_environment_spec.rb
@@ -8,7 +8,7 @@ describe ExecutionEnvironment do
   it 'validates that the Docker image works' do
     allow(execution_environment).to receive(:validate_docker_image?).and_return(true)
     allow(execution_environment).to receive(:working_docker_image?).and_return(true)
-    execution_environment.update(docker_image: FactoryBot.attributes_for(:ruby)[:docker_image])
+    execution_environment.update(FactoryBot.build(:ruby).attributes)
     expect(execution_environment).to have_received(:working_docker_image?)
   end
 
@@ -146,10 +146,12 @@ describe ExecutionEnvironment do
   end
 
   describe '#working_docker_image?' do
+    let(:execution_environment) { FactoryBot.create(:ruby) }
     let(:working_docker_image?) { execution_environment.send(:working_docker_image?) }
     let(:runner) { instance_double 'runner' }
 
     before do
+      allow(execution_environment).to receive(:sync_runner_environment).and_return(true)
       allow(Runner).to receive(:for).with(execution_environment.author, execution_environment).and_return runner
     end
 
@@ -176,7 +178,7 @@ describe ExecutionEnvironment do
     context 'when the Docker client produces an error' do
       it 'adds an error' do
         allow(runner).to receive(:execute_command).and_raise(Runner::Error)
-        working_docker_image?
+        expect { working_docker_image? }.to raise_error(ActiveRecord::RecordInvalid)
         expect(execution_environment.errors[:docker_image]).to be_present
       end
     end

From 94170ea742f6a07dac4f454b696c0cee79614035 Mon Sep 17 00:00:00 2001
From: Sebastian Serth <Sebastian.Serth@hpi.de>
Date: Thu, 4 Nov 2021 01:05:08 +0100
Subject: [PATCH 4/9] DCP: Improve error handling for destroy_at_management

---
 lib/runner/strategy/docker_container_pool.rb | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/runner/strategy/docker_container_pool.rb b/lib/runner/strategy/docker_container_pool.rb
index d97c5fce..f82394c1 100644
--- a/lib/runner/strategy/docker_container_pool.rb
+++ b/lib/runner/strategy/docker_container_pool.rb
@@ -68,7 +68,10 @@ class Runner::Strategy::DockerContainerPool < Runner::Strategy
   def destroy_at_management
     url = "#{self.class.config[:url]}/docker_container_pool/destroy_container/#{container.id}"
     Rails.logger.debug { "#{Time.zone.now.getutc.inspect}: Destroying runner at #{url}" }
-    Faraday.delete(url)
+    response = Faraday.delete(url)
+    return true if response.success?
+
+    raise Runner::Error::UnexpectedResponse.new("Could not delete execution environment in DockerContainerPool, got response: #{response.as_json}")
   rescue Faraday::Error => e
     raise Runner::Error::FaradayError.new("Request to DockerContainerPool failed: #{e.inspect}")
   ensure

From 79da2781e31e2f9f9ac69c7434d0ba404c31f82a Mon Sep 17 00:00:00 2001
From: Sebastian Serth <Sebastian.Serth@hpi.de>
Date: Thu, 4 Nov 2021 01:05:28 +0100
Subject: [PATCH 5/9] Restructure methods in execution_environment.rb

---
 app/models/execution_environment.rb | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/app/models/execution_environment.rb b/app/models/execution_environment.rb
index b5f16157..1b3342e5 100644
--- a/app/models/execution_environment.rb
+++ b/app/models/execution_environment.rb
@@ -19,8 +19,9 @@ class ExecutionEnvironment < ApplicationRecord
 
   scope :with_exercises, -> { where('id IN (SELECT execution_environment_id FROM exercises)') }
 
+  before_validation :clean_exposed_ports
+
   validate :valid_test_setup?
-  validate :working_docker_image?, if: :validate_docker_image?
   validates :docker_image, presence: true
   validates :memory_limit,
     numericality: {greater_than_or_equal_to: MINIMUM_MEMORY_LIMIT, only_integer: true}, presence: true
@@ -30,7 +31,6 @@ class ExecutionEnvironment < ApplicationRecord
   validates :pool_size, numericality: {only_integer: true}, presence: true
   validates :run_command, presence: true
   validates :cpu_limit, presence: true, numericality: {greater_than: 0, only_integer: true}
-  before_validation :clean_exposed_ports
   validates :exposed_ports, array: {numericality: {greater_than_or_equal_to: 0, less_than: 65_536, only_integer: true}}
 
   after_destroy :delete_runner_environment
@@ -59,10 +59,15 @@ class ExecutionEnvironment < ApplicationRecord
     exposed_ports.join(', ')
   end
 
+  private
+
+  def set_default_values
+    set_default_values_if_present(permitted_execution_time: 60, pool_size: 0)
+  end
+
   def clean_exposed_ports
     self.exposed_ports = exposed_ports.uniq.sort
   end
-  private :clean_exposed_ports
 
   def valid_test_setup?
     if test_command? ^ testing_framework?
@@ -71,12 +76,10 @@ class ExecutionEnvironment < ApplicationRecord
           attribute: I18n.t('activerecord.attributes.execution_environment.testing_framework')))
     end
   end
-  private :valid_test_setup?
 
   def validate_docker_image?
     docker_image.present? && !Rails.env.test?
   end
-  private :validate_docker_image?
 
   def working_docker_image?
     sync_runner_environment

From 054d35b8d3b39f6a1afd9198037e2209585f9f58 Mon Sep 17 00:00:00 2001
From: Sebastian Serth <Sebastian.Serth@hpi.de>
Date: Thu, 4 Nov 2021 10:40:01 +0100
Subject: [PATCH 6/9] Add environments method to all strategies

---
 lib/runner/strategy.rb                       |  4 ++++
 lib/runner/strategy/docker_container_pool.rb |  4 ++++
 lib/runner/strategy/null.rb                  |  4 ++++
 lib/runner/strategy/poseidon.rb              | 21 ++++++++++++++++++++
 4 files changed, 33 insertions(+)

diff --git a/lib/runner/strategy.rb b/lib/runner/strategy.rb
index 2a03f39c..313d35fa 100644
--- a/lib/runner/strategy.rb
+++ b/lib/runner/strategy.rb
@@ -9,6 +9,10 @@ class Runner::Strategy
     raise NotImplementedError
   end
 
+  def self.environments
+    raise NotImplementedError
+  end
+
   def self.sync_environment(_environment)
     raise NotImplementedError
   end
diff --git a/lib/runner/strategy/docker_container_pool.rb b/lib/runner/strategy/docker_container_pool.rb
index f82394c1..0ec17c2b 100644
--- a/lib/runner/strategy/docker_container_pool.rb
+++ b/lib/runner/strategy/docker_container_pool.rb
@@ -14,6 +14,10 @@ class Runner::Strategy::DockerContainerPool < Runner::Strategy
     FileUtils.mkdir_p(File.expand_path(config[:workspace_root]))
   end
 
+  def self.environments
+    pool_size.keys.map {|key| {id: key} }
+  end
+
   def self.sync_environment(environment)
     # Force a database commit and start a new transaction.
     if environment.class.connection.transaction_open?
diff --git a/lib/runner/strategy/null.rb b/lib/runner/strategy/null.rb
index 1c5f34a8..4702e6f4 100644
--- a/lib/runner/strategy/null.rb
+++ b/lib/runner/strategy/null.rb
@@ -5,6 +5,10 @@
 class Runner::Strategy::Null < Runner::Strategy
   def self.initialize_environment; end
 
+  def self.environments
+    raise Runner::Error.new
+  end
+
   def self.sync_environment(_environment)
     raise Runner::Error.new
   end
diff --git a/lib/runner/strategy/poseidon.rb b/lib/runner/strategy/poseidon.rb
index 8a83e62f..195d32db 100644
--- a/lib/runner/strategy/poseidon.rb
+++ b/lib/runner/strategy/poseidon.rb
@@ -19,6 +19,27 @@ class Runner::Strategy::Poseidon < Runner::Strategy
     nil
   end
 
+  def self.environments
+    url = "#{config[:url]}/execution-environments"
+    Rails.logger.debug { "#{Time.zone.now.getutc.inspect}: Getting list of execution environments at #{url}" }
+    response = http_connection.get url
+
+    case response.status
+      when 200
+        response_body = parse response
+        execution_environments = response_body[:executionEnvironments]
+        execution_environments.presence || raise(Runner::Error::UnexpectedResponse.new("Could not get the list of execution environments in Poseidon, got response: #{response.as_json}"))
+      when 404
+        raise Runner::Error::EnvironmentNotFound.new
+      else
+        handle_error response
+    end
+  rescue Faraday::Error => e
+    raise Runner::Error::FaradayError.new("Could not get the list of execution environments because of Faraday error: #{e.inspect}")
+  ensure
+    Rails.logger.debug { "#{Time.zone.now.getutc.inspect}: Finished getting the list of execution environments" }
+  end
+
   def self.sync_environment(environment)
     url = "#{config[:url]}/execution-environments/#{environment.id}"
     Rails.logger.debug { "#{Time.zone.now.getutc.inspect}: Synchronizing execution environment at #{url}" }

From 79e8caea450f7d4727fbc8f75ce1970bff233895 Mon Sep 17 00:00:00 2001
From: Sebastian Serth <Sebastian.Serth@hpi.de>
Date: Thu, 4 Nov 2021 10:40:55 +0100
Subject: [PATCH 7/9] Remove outdated execution environments after syncing all

---
 .../execution_environments_controller.rb      | 29 +++++++++++++++++--
 .../execution_environments_controller_spec.rb | 13 +++++++--
 2 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/app/controllers/execution_environments_controller.rb b/app/controllers/execution_environments_controller.rb
index 4dc00502..5bc8328e 100644
--- a/app/controllers/execution_environments_controller.rb
+++ b/app/controllers/execution_environments_controller.rb
@@ -171,11 +171,36 @@ class ExecutionEnvironmentsController < ApplicationController
 
     return unless Runner.management_active?
 
-    success = ExecutionEnvironment.all.map do |execution_environment|
+    success = []
+
+    begin
+      # Get a list of all existing execution environments and mark them as a potential candidate for removal
+      environments_to_remove = Runner.strategy_class.environments.pluck(:id)
+      success << true
+    rescue Runner::Error => e
+      Rails.logger.debug { "Runner error while getting all execution environments: #{e.message}" }
+      environments_to_remove = []
+      success << false
+    end
+
+    success += ExecutionEnvironment.all.map do |execution_environment|
+      # Sync all current execution environments and prevent deletion of those just synced
+      environments_to_remove -= [execution_environment.id]
       Runner.strategy_class.sync_environment(execution_environment)
-    rescue Runner::Error
+    rescue Runner::Error => e
+      Rails.logger.debug { "Runner error while synchronizing execution environment with id #{execution_environment.id}: #{e.message}" }
       false
     end
+
+    success += environments_to_remove.map do |execution_environment_id|
+      # Remove execution environments not synced. We temporarily use a record which is not persisted
+      execution_environment = ExecutionEnvironment.new(id: execution_environment_id)
+      Runner.strategy_class.remove_environment(execution_environment)
+    rescue Runner::Error => e
+      Rails.logger.debug { "Runner error while deleting execution environment with id #{execution_environment.id}: #{e.message}" }
+      false
+    end
+
     if success.all?
       redirect_to ExecutionEnvironment, notice: t('execution_environments.index.synchronize_all.success')
     else
diff --git a/spec/controllers/execution_environments_controller_spec.rb b/spec/controllers/execution_environments_controller_spec.rb
index 9aee3e5a..001f1fa4 100644
--- a/spec/controllers/execution_environments_controller_spec.rb
+++ b/spec/controllers/execution_environments_controller_spec.rb
@@ -216,7 +216,8 @@ describe ExecutionEnvironmentsController do
   end
 
   describe '#sync_all_to_runner_management' do
-    let(:execution_environments) { FactoryBot.build_list(:ruby, 3) }
+    let(:execution_environments) { %i[ruby java python].map {|environment| FactoryBot.create(environment) } }
+    let(:outdated_execution_environments) { %i[node_js html].map {|environment| FactoryBot.build_stubbed(environment) } }
 
     let(:codeocean_config) { instance_double(CodeOcean::Config) }
     let(:runner_management_config) { {runner_management: {enabled: true, strategy: :poseidon}} }
@@ -229,11 +230,19 @@ describe ExecutionEnvironmentsController do
     end
 
     it 'copies all execution environments to the runner management' do
-      allow(ExecutionEnvironment).to receive(:all).and_return(execution_environments)
+      allow(Runner::Strategy::Poseidon).to receive(:environments).and_return(outdated_execution_environments)
+      expect(Runner::Strategy::Poseidon).to receive(:environments).once
 
       execution_environments.each do |execution_environment|
         allow(Runner::Strategy::Poseidon).to receive(:sync_environment).with(execution_environment).and_return(true)
         expect(Runner::Strategy::Poseidon).to receive(:sync_environment).with(execution_environment).once
+        expect(Runner::Strategy::Poseidon).not_to receive(:remove_environment).with(execution_environment)
+      end
+
+      outdated_execution_environments.each do |execution_environment|
+        allow(Runner::Strategy::Poseidon).to receive(:remove_environment).with(execution_environment).and_return(true)
+        expect(Runner::Strategy::Poseidon).to receive(:remove_environment).with(execution_environment).once
+        expect(Runner::Strategy::Poseidon).not_to receive(:sync_environment).with(execution_environment)
       end
 
       post :sync_all_to_runner_management

From b179dadce65acec9a7719f8fcd55c300a11a906d Mon Sep 17 00:00:00 2001
From: Sebastian Serth <Sebastian.Serth@hpi.de>
Date: Thu, 4 Nov 2021 11:25:32 +0100
Subject: [PATCH 8/9] Mock CodeOcean::Config for Poseidon strategy

---
 spec/lib/runner/strategy/poseidon_spec.rb | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/spec/lib/runner/strategy/poseidon_spec.rb b/spec/lib/runner/strategy/poseidon_spec.rb
index fb31429b..42ec928e 100644
--- a/spec/lib/runner/strategy/poseidon_spec.rb
+++ b/spec/lib/runner/strategy/poseidon_spec.rb
@@ -9,6 +9,16 @@ describe Runner::Strategy::Poseidon do
   let(:error_message) { 'test error message' }
   let(:response_body) { nil }
 
+  let(:codeocean_config) { instance_double(CodeOcean::Config) }
+  let(:runner_management_config) { {runner_management: {enabled: true, strategy: :poseidon, url: 'https://runners.example.org', unused_runner_expiration_time: 180}} }
+
+  before do
+    # Ensure to reset the memorized helper
+    Runner.instance_variable_set :@strategy_class, nil
+    allow(CodeOcean::Config).to receive(:new).with(:code_ocean).and_return(codeocean_config)
+    allow(codeocean_config).to receive(:read).and_return(runner_management_config)
+  end
+
   # All requests handle a BadRequest (400) response the same way.
   shared_examples 'BadRequest (400) error handling' do
     context 'when Poseidon returns BadRequest (400)' do

From fb92d382ac120807bd3fe09863c3e70a04498f92 Mon Sep 17 00:00:00 2001
From: Sebastian Serth <Sebastian.Serth@hpi.de>
Date: Tue, 9 Nov 2021 17:37:11 +0100
Subject: [PATCH 9/9] Skip verification of Docker image if pool size is empty

---
 app/models/execution_environment.rb                        | 3 ++-
 spec/controllers/execution_environments_controller_spec.rb | 4 ++--
 spec/models/execution_environment_spec.rb                  | 6 ++++++
 3 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/app/models/execution_environment.rb b/app/models/execution_environment.rb
index 1b3342e5..2f16db6b 100644
--- a/app/models/execution_environment.rb
+++ b/app/models/execution_environment.rb
@@ -78,7 +78,8 @@ class ExecutionEnvironment < ApplicationRecord
   end
 
   def validate_docker_image?
-    docker_image.present? && !Rails.env.test?
+    # We only validate the code execution with the provided image if there is at least one container to test with.
+    pool_size.positive? && docker_image.present? && !Rails.env.test?
   end
 
   def working_docker_image?
diff --git a/spec/controllers/execution_environments_controller_spec.rb b/spec/controllers/execution_environments_controller_spec.rb
index 001f1fa4..32acd572 100644
--- a/spec/controllers/execution_environments_controller_spec.rb
+++ b/spec/controllers/execution_environments_controller_spec.rb
@@ -13,7 +13,7 @@ describe ExecutionEnvironmentsController do
 
   describe 'POST #create' do
     context 'with a valid execution environment' do
-      let(:perform_request) { proc { post :create, params: {execution_environment: FactoryBot.build(:ruby).attributes} } }
+      let(:perform_request) { proc { post :create, params: {execution_environment: FactoryBot.build(:ruby, pool_size: 1).attributes} } }
 
       before do
         allow(Rails.env).to receive(:test?).and_return(false, true)
@@ -186,7 +186,7 @@ describe ExecutionEnvironmentsController do
         runner = instance_double 'runner'
         allow(Runner).to receive(:for).and_return(runner)
         allow(runner).to receive(:execute_command).and_return({})
-        put :update, params: {execution_environment: FactoryBot.attributes_for(:ruby), id: execution_environment.id}
+        put :update, params: {execution_environment: FactoryBot.attributes_for(:ruby, pool_size: 1), id: execution_environment.id}
       end
 
       expect_assigns(docker_images: Array)
diff --git a/spec/models/execution_environment_spec.rb b/spec/models/execution_environment_spec.rb
index 8bfb0b07..4a6e2ff3 100644
--- a/spec/models/execution_environment_spec.rb
+++ b/spec/models/execution_environment_spec.rb
@@ -138,8 +138,14 @@ describe ExecutionEnvironment do
       expect(execution_environment.send(:validate_docker_image?)).to be false
     end
 
+    it 'is false when the pool size is empty' do
+      expect(execution_environment.pool_size).to be 0
+      expect(execution_environment.send(:validate_docker_image?)).to be false
+    end
+
     it 'is true otherwise' do
       execution_environment.docker_image = FactoryBot.attributes_for(:ruby)[:docker_image]
+      execution_environment.pool_size = 1
       allow(Rails.env).to receive(:test?).and_return(false)
       expect(execution_environment.send(:validate_docker_image?)).to be true
     end