DEV: Improve external upload debugging (#28627)

* Do not delete created external upload stubs for 2 days
  instead of 1 hour if enable_upload_debug_mode is true,
  this aids with server-side debugging.
* If using an API call, return the detailed error message
  if enable_upload_debug_mode is true. In this case the user
  is not using the UI, so a more detailed message is appropriate.
* Add a prefix to log messages in ExternalUploadHelpers, to
  make it easier to find these in logster.

app/models/external_upload_stub.rb

@@ -21,7 +21,7 @@ class ExternalUploadStub < ActiveRecord::Base
           where(
             "status = ? AND created_at <= ?",
             ExternalUploadStub.statuses[:created],
-            CREATED_EXPIRY_HOURS.hours.ago,
+            (SiteSetting.enable_upload_debug_mode ? 48 : CREATED_EXPIRY_HOURS).hours.ago,
           )
         end
 

lib/external_upload_helpers.rb

@@ -136,7 +136,8 @@ module ExternalUploadHelpers
       ExternalUploadStub.find_by(unique_identifier: unique_identifier, created_by: current_user)
     return render_404 if external_upload_stub.blank?
 
-    return render_404 if !multipart_upload_exists?(external_upload_stub)
+    message = check_multipart_upload_exists(external_upload_stub)
+    return render_404(message) if message.present?
 
     store = multipart_store(external_upload_stub.upload_type)
 
@@ -152,7 +153,7 @@ module ExternalUploadHelpers
     render json: { presigned_urls: presigned_urls }
   end
 
-  def multipart_upload_exists?(external_upload_stub)
+  def check_multipart_upload_exists(external_upload_stub)
     store = multipart_store(external_upload_stub.upload_type)
     begin
       store.list_multipart_parts(
@@ -161,6 +162,7 @@ module ExternalUploadHelpers
         max_parts: 1,
       )
     rescue Aws::S3::Errors::NoSuchUpload => err
+      return(
         debug_upload_error(
           err,
           I18n.t(
@@ -168,9 +170,12 @@ module ExternalUploadHelpers
             additional_detail: "path: #{external_upload_stub.key}",
           ),
         )
-      return false
+      )
     end
-    true
+
+    # Intentional to indicate that there is no error, if there is a message
+    # then there is an error.
+    nil
   end
 
   def abort_multipart
@@ -226,7 +231,8 @@ module ExternalUploadHelpers
       ExternalUploadStub.find_by(unique_identifier: unique_identifier, created_by: current_user)
     return render_404 if external_upload_stub.blank?
 
-    return render_404 if !multipart_upload_exists?(external_upload_stub)
+    message = check_multipart_upload_exists(external_upload_stub)
+    return render_404(message) if message.present?
 
     store = multipart_store(external_upload_stub.upload_type)
     parts =
@@ -353,9 +359,14 @@ module ExternalUploadHelpers
   end
 
   def debug_upload_error(err, friendly_message)
-    return if !SiteSetting.enable_upload_debug_mode
-    Discourse.warn_exception(err, message: friendly_message)
-    (Rails.env.development? || Rails.env.test?) ? friendly_message : I18n.t("upload.failed")
+    return I18n.t("upload.failed") if !SiteSetting.enable_upload_debug_mode
+    Discourse.warn_exception(err, message: "[ExternalUploadError] #{friendly_message}")
+
+    if Rails.env.local? || is_api?
+      friendly_message
+    else
+      I18n.t("upload.failed")
+    end
   end
 
   def multipart_store(upload_type)
@@ -385,7 +396,11 @@ module ExternalUploadHelpers
     metadata.permit("sha1-checksum").to_h
   end
 
-  def render_404
+  def render_404(message = nil)
+    if message
+      render_json_error(message, status: 404)
+    else
       raise Discourse::NotFound
     end
+  end
 end

spec/jobs/clean_up_uploads_spec.rb

@@ -404,4 +404,20 @@ RSpec.describe Jobs::CleanUpUploads do
     Jobs::CleanUpUploads.new.execute(nil)
     expect(ExternalUploadStub.pluck(:id)).to contain_exactly(external_stub1.id, external_stub3.id)
   end
+
+  it "does not delete create external upload stubs for 2 days if debug mode is on" do
+    SiteSetting.enable_upload_debug_mode = true
+    external_stub1 =
+      Fabricate(
+        :external_upload_stub,
+        status: ExternalUploadStub.statuses[:created],
+        created_at: 2.hours.ago,
+      )
+    Jobs::CleanUpUploads.new.execute(nil)
+    expect(ExternalUploadStub.pluck(:id)).to contain_exactly(external_stub1.id)
+
+    SiteSetting.enable_upload_debug_mode = false
+    Jobs::CleanUpUploads.new.execute(nil)
+    expect(ExternalUploadStub.pluck(:id)).to be_empty
+  end
 end