Increase verbosity of remote asset job

Author: oliverguenther

app/workers/custom_styles/seed_remote_asset_job.rb

@@ -31,33 +31,79 @@
 module CustomStyles
   # Downloads a design asset seeded through OPENPROJECT_SEED_DESIGN_* as a remote URL.
   class SeedRemoteAssetJob < ApplicationJob
+    # Separate error class for non-retriable failures (e.g. HTTP 4xx) that we should log, then discard.
+    class PermanentFailure < StandardError; end
+
+    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_on StandardError, wait: :polynomially_longer, attempts: 5
 
     # 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
+
+    discard_on PermanentFailure 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 PermanentFailure, OpenProject::ServerSideRequestForgeryError, ActiveJob::DeserializationError
+      raise
+    rescue HTTPX::HTTPError => e
+      log_attempt_failure(key, url, e)
+      raise PermanentFailure, e.message if e.status.to_i.between?(400, 499)
+
+      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}: #{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 PermanentFailure,
+                "Stored design asset '#{key}' is not readable in file storage"
+        end
       end
     end
 
@@ -78,5 +124,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,24 @@
       stub_request(:get, url).to_return(status: 404)
     end
 
-    it "swallows the error and reschedules itself instead" do
+    it "discards the job without retrying" do
       expect { perform }.not_to raise_error
 
-      expect(described_class).to have_been_enqueued.with(custom_style, :logo, url)
+      expect(described_class).not_to have_been_enqueued
       expect(custom_style.reload.logo.file).to be_nil
     end
 
-    it "logs the failed attempt" do
+    it "logs the failed attempt and the discard" do
       allow(Rails.logger).to receive(:error)
 
       perform
 
       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"))
+      expect(Rails.logger)
+        .to have_received(:error)
+        .with(a_string_starting_with("Discarding design asset seed for 'logo' from #{url}: HTTP Error: 404"))
     end
   end
 
@@ -121,7 +124,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 +133,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}:"))
     end
 
     context "when the IP address is on the SSRF allowlist", with_ssrf_ip_allowlist: %w[127.0.0.1] do