Increase verbosity of remote asset job

Author: oliverguenther

app/workers/custom_styles/seed_remote_asset_job.rb

@@ -31,33 +31,70 @@
 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
+
+    # 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}" }
+    )
+
+    retry_on GoodJob::ActiveJobExtensions::Concurrency::ConcurrencyExceededError,
+             wait: 5.seconds,
+             attempts: :unlimited
+
+    # Retry transient HTTP failures a few times, after which we discard with a log entry.
+    retry_on StandardError, wait: :polynomially_longer, attempts: 5, report: true do |job, error|
+      job.log_discard(error)
+    end
 
     # Declared after retry_on StandardError so they take precedence
-    discard_on ActiveJob::DeserializationError
-    discard_on OpenProject::ServerSideRequestForgeryError
+    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!
+      CustomStyle.transaction do
+        style = CustomStyle.lock.find(custom_style.id)
+
+        build_attachable_file(key.to_s, response.body.to_s) do |file|
+          style.public_send("#{key}=", file)
+          style.save!
+        end
+
+        unless style.public_send(key).readable?
+          raise "Stored design asset '#{key}' is not readable in file storage"
+        end
       end
     end
 
@@ -78,5 +115,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

@@ -76,21 +76,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 +129,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 +138,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