FIX: Make migrations from S3 more robust; fix bare URL migration (PR #10093)

[johnsonm]

Improve handling of URLs to (more) correctly process URLs present in s3 or a clone when migrating to local, including changing to an upload: psuedo-protocol URL, tagging audio and video files, and in the process not creating non-URLs that point to nothing.

Also, fix batch_migrate_from_s3 to take both a max number of posts to actually modify in a migration (the original but failed intent of limit) and a limit of total posts to consider for migration (for cheaper database queries). Because a primary goal of the max limit is debugging, also use it to cause verbose output.

Finally, in the case of a truly unexpected error, print the error and stop processing, so that error cascades do not become hidden failures stopping a successful migration, but can be resolved.

GitHub

[discoursebot]

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/migrate-from-s3-problems/119064/6

[discoursebot]

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/migrating-s3-spaces-non-image-uploads-to-local/132484/4

[johnsonm]

The only failures I’ve seen with this code running so far on my live site are:

• Not migrating videos larger than the current max file size; not a bug.
• Transient failures from digital ocean spaces that are resolved by the workaround I have at https://github.com/johnsonm/discourse/commit/7dfac12a2ea6ec04ba4e0616b4e0dbd1d806cff7 — I can pull that commit into this PR or not, as you wish. I don’t know the right number of retries in general, and I didn’t implement exponential backoff on the retries yet since it was intended as a quick hack to see if it would work around my problem.

I think this is ready for review, and valuable generally.

[discoursebot]

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

Migrate_from_s3 problems

[johnsonm]

I’ve run into the need to retry download enough that it’s worth pulling in that workaround. It would be better in theory to do exponential backoff, but as far as I can tell this simple workaround hasn’t failed yet. Good enough, I hope. It can always be improved later to be exponential, but really any S3 clone shouldn’t be that unreliable.

[johnsonm]

It looks like there might be inconsistent behavior of the upload mocks. They aren’t acting like I would have thought; the hash sometimes changes and sometimes doesn’t. This is the source of the occasional test failures, it seems.

[johnsonm]

https://meta.discourse.org/t/requesting-help-with-failure-finding-discourseredis-connector-running-tests/155599 addressed and test failures understood and resolved. I believe this is ready to merge.

[CvX]

lib/tasks/uploads.rake

@@ -174,40 +230,156 @@ def migrate_from_s3(limit: nil)
           if sha1 = Upload.sha1_from_short_url(url)
             if upload = Upload.find_by(sha1: sha1)
               if upload.url.start_with?("//")
-                file = FileHelper.download("http:#{upload.url}", max_file_size: max_file_size, tmp_file_name: "from_s3", follow_redirect: true)
-                filename = upload.original_filename
-                origin = upload.origin
-                upload.destroy
-
-                new_upload = UploadCreator.new(file, filename, origin: origin).create_for(post.user_id || -1)
+                file = download_s3_upload(upload, max_file_size)
+                raise "#{url} not found for #{post_tag}" if file.nil?

                raise "#{url} not found for #{post_tag}" unless file

[CvX]

lib/tasks/uploads.rake

@@ -174,40 +230,156 @@ def migrate_from_s3(limit: nil)
           if sha1 = Upload.sha1_from_short_url(url)
             if upload = Upload.find_by(sha1: sha1)
               if upload.url.start_with?("//")
-                file = FileHelper.download("http:#{upload.url}", max_file_size: max_file_size, tmp_file_name: "from_s3", follow_redirect: true)
-                filename = upload.original_filename
-                origin = upload.origin
-                upload.destroy
-
-                new_upload = UploadCreator.new(file, filename, origin: origin).create_for(post.user_id || -1)
+                file = download_s3_upload(upload, max_file_size)
+                raise "#{url} not found for #{post_tag}" if file.nil?
+                new_upload = create_local_upload(upload, file, post.user_id || -1)
                 if new_upload&.save
+                  raise "Error: upload url #{url} changed to #{new_upload.short_url}, should be unchanged." if url != new_upload.short_url
                   updated = true
-                  url = new_upload.url
                 end
               end
+            else
+              puts "Missing upload for #{url} (#{sha1}) in #{post_tag}"
             end
           end
 
-          url
-        rescue
           url
         end
       end
 
       if updated
-        post.save!
+        post.save!(validate: false)
         post.rebake!
-        putc "#"
+        posts_modified += 1
+        if max
+          # max is for debugging purposes, so be verbose

I’d rather have a separate variable for this (verbose?) which could be automatically set to true if max is non-nil or via ENV["VERBOSE"]. You could consider using a helper function for logging, like: https://github.com/discourse/discourse/blob/630ec7b49cd98358736c5445302c509c62c68653/lib/shrink_uploaded_image.rb#L238-L240

[CvX]

lib/tasks/uploads.rake

@@ -174,40 +230,156 @@ def migrate_from_s3(limit: nil)
           if sha1 = Upload.sha1_from_short_url(url)
             if upload = Upload.find_by(sha1: sha1)
               if upload.url.start_with?("//")
-                file = FileHelper.download("http:#{upload.url}", max_file_size: max_file_size, tmp_file_name: "from_s3", follow_redirect: true)
-                filename = upload.original_filename
-                origin = upload.origin
-                upload.destroy
-
-                new_upload = UploadCreator.new(file, filename, origin: origin).create_for(post.user_id || -1)
+                file = download_s3_upload(upload, max_file_size)
+                raise "#{url} not found for #{post_tag}" if file.nil?
+                new_upload = create_local_upload(upload, file, post.user_id || -1)
                 if new_upload&.save
+                  raise "Error: upload url #{url} changed to #{new_upload.short_url}, should be unchanged." if url != new_upload.short_url
                   updated = true
-                  url = new_upload.url
                 end
               end
+            else
+              puts "Missing upload for #{url} (#{sha1}) in #{post_tag}"
             end
           end
 
-          url
-        rescue
           url
         end
       end
 
       if updated
-        post.save!
+        post.save!(validate: false)
         post.rebake!
-        putc "#"
+        posts_modified += 1
+        if max
+          # max is for debugging purposes, so be verbose
+          puts "\nModified #{posts_modified}/#{max}: #{post_tag}\n"
+        else
+          putc "#"
+        end
+        if max && posts_modified >= max
+          puts "Modification limit #{max} reached, halting migration"
+          return
+        end
+        # don't spam the default queue, limit rate of modification
+        wait_for_queue_tasks max_queue_tasks
       else
         putc "."
       end
 
-    rescue
-      putc "X"
+    rescue => e
+      # Unanticipated failure should be investigated before going further with a migration to limit harm
+      puts "Unhandled failure in #{post_tag} processing: #{e}"
+      exit 1
     end
   end
 
-  puts "Done!"
+  puts "All Post uploads migrated.  Migrating profile uploads..."
+
+  profiles_migrated = 0
+  migrate_profiles = UserProfile
+    .where("card_background_upload_id != 0 OR profile_background_upload_id != 0")
+  migrate_profiles = migrate_profiles.limit(limit) if limit
+
+  migrate_profiles.find_each do |profile|
+    puts "Migrating profile for #{profile.user.username} (#{profile.user.id})" if max
+    migrate_card = false
+    migrate_profile = false
+    if profile.card_background_upload&.url&.start_with?('//')
+      migrate_card = true
+    end
+    if profile.profile_background_upload&.url&.start_with?('//')
+      migrate_profile = true
+    end
+
+    if migrate_card
+      background_upload = profile.card_background_upload
+      puts "Card: #{background_upload.url}" if max
+      file = download_s3_upload(background_upload, max_file_size)
+      raise "#{url} not found for #{profile.user.name}" if file.nil?
+    end
+    if migrate_profile
+      profile_upload = profile.profile_background_upload
+      puts "Background: #{profile_upload.url}" if max
+      file = download_s3_upload(profile_upload, max_file_size)
+      raise "#{url} not found for #{profile.user.name}" if file.nil?
+    end
+
+    # These need to both be after the download but before setting new in case the same image was provided for both
+    profile.clear_card_background if migrate_card
+    profile.clear_profile_background if migrate_profile
+
+    if migrate_card
+      new_background = create_local_upload(background_upload, file, profile.user.id || -1)
+      profile.upload_card_background(new_background)
+    end
+    if migrate_profile
+      new_profile = create_local_upload(profile_upload, file, profile.user.id || -1)
+      profile.upload_profile_background(new_profile)
+    end
+
+    if migrate_card || migrate_profile
+      profile.save!
+      profiles_migrated += 1
+    end
+    if max && profiles_migrated >= max
+      puts "Modification limit #{max} reached, halting migration"
+      return
+    end
+    wait_for_queue_tasks max_queue_tasks
+  end
+
+  puts "All profiles uploads migrated.  Migrating other non-post uploads..."

The full list (I hope it’s full ) of non-post upload types is for example in the ShrinkUploadedImage class:

https://github.com/discourse/discourse/blob/630ec7b49cd98358736c5445302c509c62c68653/lib/shrink_uploaded_image.rb#L165-L173

This section of the script would break all those types of upload references (other than the profile uploads handled above)

[CvX]

lib/tasks/uploads.rake

@@ -362,6 +534,52 @@ def clean_up_uploads
   puts "Done!"
 end
 
+################################################################################
+#                         report missing uploads                               #
+################################################################################
+
+# This might be used to repair after various failures
+task "uploads:report_missing_uploads" => :environment do

@davidtaylorhq Can I get your eyes on this? (the name of the task and what it does)

[CvX]

lib/tasks/uploads.rake

@@ -107,18 +119,40 @@ def guess_filename(url, raw)
     filename ||= raw[/<a class="attachment" href="(?:https?:)?#{Regexp.escape(url)}">([^<]+)<\/a>/, 1].presence
     filename ||= File.basename(url)
     filename
-  rescue
+  rescue => e
+    puts "Error guessing filename: #{e}"
     nil
   ensure
     f.try(:close!) rescue nil
   end
 end
 
-def migrate_all_from_s3(limit: nil)
-  RailsMultisite::ConnectionManagement.each_connection { migrate_from_s3(limit: limit) }
+def download_s3_upload(upload, max_file_size)
+  download_attempts_remaining = 3
+  file = nil
+  while file.nil? && download_attempts_remaining > 0
+    file = FileHelper.download("http:#{upload.url}", max_file_size: max_file_size, tmp_file_name: "from_s3", follow_redirect: true)
+    # work-around for intermittent download failures
+    # This can be a temporary error if download from S3 fails
+    sleep 1 if file.nil?
+    download_attempts_remaining -= 1
+  end
+  file
+end
+
+def create_local_upload(upload, file, user_id)
+  filename = upload.original_filename
+  origin = upload.origin
+  upload.destroy
+
+  UploadCreator.new(file, filename, origin: origin).create_for(user_id)
+end

This re-creates uploads, preserving only the actual file and user_id, filename, origin attributes?

I’m fine with creating new upload records (there was even an idea to make them immutable, to have a permanent record of changes) but all original attributes should be preserved.

[CvX]

lib/tasks/uploads.rake

@@ -93,8 +93,20 @@ task "uploads:migrate_from_s3" => :environment do
   ENV["RAILS_DB"] ? migrate_from_s3 : migrate_all_from_s3
 end
 
-task "uploads:batch_migrate_from_s3", [:limit] => :environment do |_, args|
-  ENV["RAILS_DB"] ? migrate_from_s3(limit: args[:limit]) : migrate_all_from_s3(limit: args[:limit])
+# limit is the limit to consider migrating, max is the max to actually migrate
+# This is to manage resources during testing; limit 1000 max 1 is a reasonable test

limit is the limit to consider migrating, max is the max to actually migrate

I think I know what both of these do by reading the code, but this comment doesn’t make it clear. max is the number of uploads to be migrated? And limit is used in SQL queries? (does that need to be configurable?)

[CvX]

lib/tasks/uploads.rake

@@ -93,8 +93,20 @@ task "uploads:migrate_from_s3" => :environment do
   ENV["RAILS_DB"] ? migrate_from_s3 : migrate_all_from_s3
 end
 
-task "uploads:batch_migrate_from_s3", [:limit] => :environment do |_, args|
-  ENV["RAILS_DB"] ? migrate_from_s3(limit: args[:limit]) : migrate_all_from_s3(limit: args[:limit])
+# limit is the limit to consider migrating, max is the max to actually migrate
+# This is to manage resources during testing; limit 1000 max 1 is a reasonable test
+task "uploads:batch_migrate_from_s3", [:max, :limit] => :environment do |_, args|
+  ENV["RAILS_DB"] ? migrate_from_s3(max: args[:max], limit: args[:limit]) : migrate_all_from_s3(max: args[:max], limit: args[:limit])
+end
+
+def wait_for_queue_tasks(max_queue_tasks)
+  while Sidekiq::Stats.new.enqueued > max_queue_tasks
+    sleep 1
+  end
+end
+
+def tag(post)

Naming is hard but it would be better to use something that isn’t a model name in Discourse.

[CvX]

From meta:

It feels like it makes sense to start with posts, since you want to rebake posts to change the URL in the cooked post after each migration; migrating all the uploads and then starting the rebakes would leave the site very broken for potentially a long time for a large site

You can process Uploads one by one, and rebake posts in-place, like downsize_uploads/ShrinkUploadedImage does.

---

Ideally we would extract the code from those two files into a class that’d be responsible for updating all objects related to the given upload (posts, user profiles, avatars, etc.)