[OP-19930] Fix lost seeded custom styles when seeding multiple

app/workers/custom_styles/seed_remote_asset_job.rb

@@ -31,36 +31,93 @@
 module CustomStyles
   # Downloads a design asset seeded through OPENPROJECT_SEED_DESIGN_* as a remote URL.
   class SeedRemoteAssetJob < ApplicationJob
-    retry_on StandardError, wait: :polynomially_longer, attempts: 5
+    include GoodJob::ActiveJobExtensions::Concurrency
 
-    # Declared after retry_on StandardError so they take precedence
-    discard_on ActiveJob::DeserializationError
-    discard_on OpenProject::ServerSideRequestForgeryError
+    # RootSeeder wraps seeding in a single transaction.
+    # At the same time, CarrierWave only actually _saves_ the file in an after_commit callback.
+    # Rails will however only run after_commit callbacks on the last instance to save a given row.
+    # Since introduction of this job, we are now saving multiple times, so some uploads were lost.
+    # By enqueueing the job after the transaction commit, we ensure that the file is saved to fog/disk.
+    self.enqueue_after_transaction_commit = true
+
+    # Only one asset for a given CustomStyle may be stored at a time. During seed,
+    # GoodJob runs inline so jobs are mostly serial already, but retries and
+    # non-inline enqueues could create a race condition for the same record/fog uploads.
+    good_job_control_concurrency_with(
+      perform_limit: 1,
+      key: -> { "#{self.class.name}-#{arguments.first.id}" }
+    )
+
+    # ActiveJob matches retry_on/discard_on from bottom to top, so declare the
+    # broad StandardError handler first and more specific handlers after it.
+    retry_on StandardError, wait: :polynomially_longer, attempts: 5, report: true do |job, error|
+      job.log_discard(error)
+    end
+
+    retry_on GoodJob::ActiveJobExtensions::Concurrency::ConcurrencyExceededError,
+             wait: 5.seconds,
+             attempts: :unlimited
+
+    discard_on ActiveJob::DeserializationError do |job, error|
+      job.log_discard(error)
+    end
+
+    discard_on OpenProject::ServerSideRequestForgeryError do |job, error|
+      job.log_discard(error)
+    end
 
     queue_with_priority :low
 
     def perform(custom_style, key, url)
       download(custom_style, key, url)
 
       Rails.logger.info "Seeded design asset '#{key}' from #{url}."
+    rescue OpenProject::ServerSideRequestForgeryError, ActiveJob::DeserializationError
+      raise
     rescue StandardError => e
-      Rails.logger.error "Failed to seed design asset '#{key}' from #{url} " \
-                         "on attempt #{executions}: #{e.message}"
+      log_attempt_failure(key, url, e)
       raise
     end
 
+    def log_discard(error)
+      _custom_style, key, url = arguments
+      Rails.logger.error "Discarding design asset seed for '#{key}' from #{url} " \
+                         "after #{executions} attempt(s): #{error.message}"
+    end
+
     private
 
     def download(custom_style, key, url)
       response = OpenProject.httpx.get(url)
       response.raise_for_status
 
-      build_attachable_file(key.to_s, response.body.to_s) do |file|
-        custom_style.public_send("#{key}=", file)
-        custom_style.save!
+      style = store_asset(custom_style, key, response.body.to_s)
+      ensure_readable!(style, key)
+    end
+
+    def store_asset(custom_style, key, data)
+      CustomStyle.transaction do
+        style = CustomStyle.lock.find(custom_style.id)
+
+        build_attachable_file(key.to_s, data) do |file|
+          style.public_send("#{key}=", file)
+          style.save!
+        end
+
+        # CarrierWave defers the real fog/disk write to after_commit. Force it
+        # while the cached file is still on this instance so an enclosing
+        # transaction (or another save of the same row) cannot drop the upload.
+        style.public_send(:"store_#{key}!")
+        style
       end
     end
 
+    def ensure_readable!(style, key)
+      return if style.public_send(key).readable?
+
+      raise "Stored design asset '#{key}' is not readable in file storage"
+    end
+
     def build_attachable_file(file_name, data)
       Tempfile.open(file_name) do |tempfile|
         tempfile.binmode
@@ -78,5 +135,10 @@ def build_attachable_file(file_name, data)
         yield(file)
       end
     end
+
+    def log_attempt_failure(key, url, error)
+      Rails.logger.error "Failed to seed design asset '#{key}' from #{url} " \
+                         "on attempt #{executions}: #{error.message}"
+    end
   end
 end

spec/workers/custom_styles/seed_remote_asset_job_spec.rb

@@ -54,6 +54,19 @@
       expect(Rails.logger).to have_received(:info).with("Seeded design asset 'logo' from #{url}.")
     end
 
+    it "stores the file even when invoked inside an open transaction" do
+      # Mimics RootSeeder: CarrierWave only uploads in after_commit, and Rails
+      # drops after_commit on earlier instances of the same row in a transaction.
+      CustomStyle.transaction do
+        described_class.perform_now(custom_style, :favicon, url)
+        described_class.perform_now(custom_style, :logo, url)
+      end
+
+      custom_style.reload
+      expect(custom_style.favicon).to be_readable
+      expect(custom_style.logo).to be_readable
+    end
+
     context "when it is an svg" do
       let(:url) { "https://example.com/image.svg" }
 
@@ -76,21 +89,29 @@
       stub_request(:get, url).to_return(status: 404)
     end
 
-    it "swallows the error and reschedules itself instead" do
+    it "logs the failed attempt and reschedules itself" do
+      allow(Rails.logger).to receive(:error)
+
       expect { perform }.not_to raise_error
 
       expect(described_class).to have_been_enqueued.with(custom_style, :logo, url)
       expect(custom_style.reload.logo.file).to be_nil
+      expect(Rails.logger)
+        .to have_received(:error)
+        .with(a_string_starting_with("Failed to seed design asset 'logo' from #{url} on attempt 1: HTTP Error: 404"))
     end
 
-    it "logs the failed attempt" do
+    it "discards and logs after retries are exhausted" do
       allow(Rails.logger).to receive(:error)
 
-      perform
+      job = described_class.new(custom_style, :logo, url)
+      allow(job).to receive_messages(executions: 5, executions_for: 5)
 
+      expect { job.perform_now }.not_to raise_error
+      expect(described_class).not_to have_been_enqueued
       expect(Rails.logger)
         .to have_received(:error)
-        .with(a_string_starting_with("Failed to seed design asset 'logo' from #{url} on attempt 1: HTTP Error: 404"))
+        .with(a_string_starting_with("Discarding design asset seed for 'logo' from #{url} after 5 attempt(s): HTTP Error: 404"))
     end
   end
 
@@ -121,7 +142,7 @@
       expect(custom_style.reload.logo.file).to be_nil
     end
 
-    it "logs why it was blocked" do
+    it "logs why it was blocked and that the job is discarded" do
       allow(Rails.logger).to receive(:error)
 
       perform
@@ -130,6 +151,9 @@
         .to have_received(:error)
         .with(a_string_including("resolves only to private IP addresses",
                                  "OPENPROJECT_SSRF_PROTECTION_IP_ALLOWLIST"))
+      expect(Rails.logger)
+        .to have_received(:error)
+        .with(a_string_starting_with("Discarding design asset seed for 'logo' from #{url} after"))
     end
 
     context "when the IP address is on the SSRF allowlist", with_ssrf_ip_allowlist: %w[127.0.0.1] do