Improve thread safety of Ci::BuildTraceChunk data stores

This commit fixes two issues:

1. ||= with a class instance variable is not thread-safe. We now
just avoid the memoization and use constants.
2. get_store_class was being called with symbols in some cases and
strings in other cases. We now standardize using symbols.

Relates to https://gitlab.com/gitlab-org/gitlab/-/issues/271253

app/models/ci/build_trace_chunk.rb

@@ -22,20 +22,26 @@ module Ci
 
     FailedToPersistDataError = Class.new(StandardError)
 
-    # Note: The ordering of this enum is related to the precedence of persist store.
+    # Note: The ordering of this hash is related to the precedence of persist store.
     # The bottom item takes the highest precedence, and the top item takes the lowest precedence.
-    enum data_store: {
+    DATA_STORES = {
       redis: 1,
       database: 2,
       fog: 3
-    }
+    }.freeze
+
+    STORE_TYPES = DATA_STORES.keys.map do |store|
+      [store, "Ci::BuildTraceChunks::#{store.capitalize}".constantize]
+    end.to_h.freeze
+
+    enum data_store: DATA_STORES
 
     scope :live, -> { redis }
     scope :persisted, -> { not_redis.order(:chunk_index) }
 
     class << self
       def all_stores
-        @all_stores ||= self.data_stores.keys
+        STORE_TYPES.keys
       end
 
       def persistable_store
@@ -44,12 +50,14 @@ module Ci
       end
 
       def get_store_class(store)
-        @stores ||= {}
+        store = store.to_sym
+
+        raise "Unknown store type: #{store}" unless STORE_TYPES.key?(store)
 
         # Can't memoize this because the feature flag may alter this
-        return fog_store_class.new if store.to_sym == :fog
+        return fog_store_class.new if store == :fog
 
-        @stores[store] ||= "Ci::BuildTraceChunks::#{store.capitalize}".constantize.new
+        STORE_TYPES[store].new
       end
 
       ##

changelogs/unreleased/sh-build-trace-chunk-store-thread-safety.yml

+---
+title: Improve thread safety of Ci::BuildTraceChunk data stores
+merge_request: 46717
+author:
+type: fixed

spec/models/ci/build_trace_chunk_spec.rb

@@ -100,15 +100,15 @@ RSpec.describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state do
     subject { described_class.all_stores }
 
     it 'returns a correctly ordered array' do
-      is_expected.to eq(%w[redis database fog])
+      is_expected.to eq(%i[redis database fog])
     end
 
     it 'returns redis store as the lowest precedence' do
-      expect(subject.first).to eq('redis')
+      expect(subject.first).to eq(:redis)
     end
 
     it 'returns fog store as the highest precedence' do
-      expect(subject.last).to eq('fog')
+      expect(subject.last).to eq(:fog)
     end
   end
 
@@ -163,6 +163,30 @@ RSpec.describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state do
     end
   end
 
+  describe '#get_store_class' do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:data_store, :expected_store) do
+      :redis | Ci::BuildTraceChunks::Redis
+      :database | Ci::BuildTraceChunks::Database
+      :fog | Ci::BuildTraceChunks::Fog
+    end
+
+    with_them do
+      context "with store" do
+        it 'returns an instance of the right class' do
+          expect(expected_store).to receive(:new).twice.and_call_original
+          expect(described_class.get_store_class(data_store.to_s)).to be_a(expected_store)
+          expect(described_class.get_store_class(data_store.to_sym)).to be_a(expected_store)
+        end
+      end
+    end
+
+    it 'raises an error' do
+      expect { described_class.get_store_class('unknown') }.to raise_error('Unknown store type: unknown')
+    end
+  end
+
   describe '#append' do
     subject { build_trace_chunk.append(new_data, offset) }