Merge #2567

2567: Guard pool metadata fetch result persistence with settings status r=rvl a=KtorZ

# Issue Number

<!-- Put here a reference to the issue that this PR relates to and which requirements it tackles. Jira issues of the form ADP- will be auto-linked. -->

ADP-634

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->


- Guard pool metadata fetch result persistence with settings status

  Since #2432 (concurrent pool metadata fetching), the fetching of
  metadata is now done in new short-lived threads forked from the
  'fetchMetadata' thread worker. Killing the parent thread
  'fetchMetadata' does not actually kill child threads which may
  still complete afterwards.

  As a consequence, when changing settings from smash (or direct) to
  none, in-flight requests spawned in short-lived thread may still
  resolve even if the 'fetchMetadata' loop has been terminated.

  This commit makes sure to only write the result of requests if and
  only if the settings haven't changed since the parent thread was
  launched. This should prevent short-lived thread to update the
  database after the parent is killed when changing settings.

Co-authored-by: KtorZ <matthias.benkort@gmail.com>
Co-authored-by: Piotr Stachyra <piotr.stachyra@iohk.io>

lib/shelley/src/Cardano/Wallet/Shelley/Pools.hs

@@ -764,6 +764,7 @@ monitorMetadata gcStatus tr sp db@(DBLayer{..}) = do
         -> IO Void
     fetchMetadata manager strategies = do
         inFlights <- STM.atomically $ newTBQueue maxInFlight
+        settings <- atomically readSettings
         endlessly [] $ \keys -> do
             refs <- nub . (\\ keys) <$> atomically (unfetchedPoolMetadataRefs limit)
             when (null refs) $ do
@@ -772,9 +773,13 @@ monitorMetadata gcStatus tr sp db@(DBLayer{..}) = do
             forM refs $ \k@(pid, url, hash) -> k <$ withAvailableSeat inFlights (do
                 fetchFromRemote trFetch strategies manager pid url hash >>= \case
                     Nothing ->
-                        atomically $ putFetchAttempt (url, hash)
+                        atomically $ do
+                            settings' <- readSettings
+                            when (settings == settings') $ putFetchAttempt (url, hash)
                     Just meta -> do
-                        atomically $ putPoolMetadata hash meta
+                        atomically $ do
+                            settings' <- readSettings
+                            when (settings == settings') $ putPoolMetadata hash meta
                 )
       where
         -- Twice 'maxInFlight' so that, when removing keys currently in flight,

test/e2e/Rakefile

@@ -246,7 +246,7 @@ task :get_docker_logs do
   cmd "sudo chmod a+rw #{LOGS}/wallet.log"
 end
 
-task :run_on, [:env, :installation, :sync_strategy] do |task, args|
+task :run_on, [:env, :installation, :sync_strategy, :skip_configs] do |task, args|
   puts "\n>> Setting up env and running tests..."
   puts "TESTS_E2E_STATEDIR=#{STATE}"
   env = args[:env]
@@ -260,7 +260,7 @@ task :run_on, [:env, :installation, :sync_strategy] do |task, args|
   end
 
   Rake::Task[:fixture_wallets_decode].invoke
-  Rake::Task[:get_latest_configs].invoke(env)
+  Rake::Task[:get_latest_configs].invoke(env) unless args[:skip_configs]
   Rake::Task[:start_node_and_wallet].invoke(env, installation)
 
   if sync_strategy == "no-sync"

test/e2e/env.rb

@@ -17,3 +17,4 @@
 ENV['TESTS_E2E_BINDIR'] ||= "./bins"
 
 ENV['TESTS_E2E_TOKEN_METADATA'] ||= "https://metadata.cardano-testnet.iohkdev.io/"
+ENV['TESTS_E2E_SMASH'] ||= "https://smash.cardano-testnet.iohkdev.io"

test/e2e/spec/e2e_spec.rb

@@ -41,8 +41,6 @@
     end
 
     after(:all) do
-      settings = CardanoWallet.new.misc.settings
-      s = settings.update({:pool_metadata_source => "none"})
       @nighly_byron_wallets.each do |wid|
         BYRON.wallets.delete wid
       end
@@ -344,29 +342,6 @@
             tx['status'] == "in_ledger"
           end
         end
-
-        it "Pool metadata is updated when settings are updated" do
-          skip "ADP-634 - metadata not cleaned properly"
-          settings = CardanoWallet.new.misc.settings
-          pools = SHELLEY.stake_pools
-
-          s = settings.update({:pool_metadata_source => "direct"})
-          expect(s).to have_http 204
-
-          eventually "Pools have metadata when 'pool_metadata_source' => 'direct'" do
-            sps = pools.list({stake: 1000})
-            sps.select{|p| p['metadata']}.size > 0
-          end
-
-          s = settings.update({:pool_metadata_source => "none"})
-          expect(s).to have_http 204
-
-          eventually "Pools have no metadata when 'pool_metadata_source' => 'none'" do
-            sps = pools.list({stake: 1000})
-            sps.select{|p| p['metadata']}.size == 0
-          end
-
-        end
       end
 
       describe "Coin Selection" do

test/e2e/spec/shelley_spec.rb

@@ -252,8 +252,47 @@
   describe CardanoWallet::Shelley::StakePools do
 
     after(:each) do
+      settings = CardanoWallet.new.misc.settings
+      s = settings.update({:pool_metadata_source => "none"})
       teardown
     end
+
+    it "ADP-634 - Pool metadata is updated when settings are updated" do
+      settings = CardanoWallet.new.misc.settings
+      pools = SHELLEY.stake_pools
+
+      s = settings.update({:pool_metadata_source => "direct"})
+      expect(s).to have_http 204
+
+      eventually "Pools have metadata when 'pool_metadata_source' => 'direct'" do
+        sps = pools.list({stake: 1000})
+        sps.select{|p| p['metadata']}.size > 0
+      end
+
+      s = settings.update({:pool_metadata_source => "none"})
+      expect(s).to have_http 204
+
+      eventually "Pools have no metadata when 'pool_metadata_source' => 'none'" do
+        sps = pools.list({stake: 1000})
+        sps.select{|p| p['metadata']}.size == 0
+      end
+
+      s = settings.update({:pool_metadata_source => ENV['TESTS_E2E_SMASH']})
+      expect(s).to have_http 204
+
+      eventually "Pools have metadata when 'pool_metadata_source' => '#{ENV['TESTS_E2E_SMASH']}'" do
+        sps = pools.list({stake: 1000})
+        sps.select{|p| p['metadata']}.size > 0
+      end
+
+      s = settings.update({:pool_metadata_source => "none"})
+      expect(s).to have_http 204
+
+      eventually "Pools have no metadata when 'pool_metadata_source' => 'none'" do
+        sps = pools.list({stake: 1000})
+        sps.select{|p| p['metadata']}.size == 0
+      end
+    end
 
     describe "Stake Pools GC Maintenance" do
       matrix = [{"direct" => "not_applicable"},