From 8f36ebe2b8c4008d29675221c65af3f6f0216207 Mon Sep 17 00:00:00 2001 From: evazion Date: Mon, 8 Nov 2021 17:40:19 -0600 Subject: [PATCH] Fix #4914: RuntimeError corrupting uploads Bug: If a media asset got stuck in the 'processing' state during upload, then it would stay stuck forever and the file couldn't be uploaded again later. Fix: Mark stuck assets as failed before raising the "Upload failed" error. Once the asset is marked as failed, it can be uploaded again later. Also, only wait for assets to finish processing if they were uploaded less than 5 minutes ago. If a processing asset is more than 5 minutes old, consider it stuck and mark it as failed immediately. Assets getting stuck in the processing state is a 'this should never happen' error. Normally if any kind of exception is raised while uploading the asset, the asset will be set to the 'failed' state. The only way an asset can get stuck is if it fails and the exception handler doesn't run, or the exception handler itself fails. This might happen if the process is unexpectedly killed, or possibly if the HTTP request times out and a TimeoutError is raised at an inopportune time. See below for discussion of issues with Timeout. [1]: https://vaneyckt.io/posts/the_disaster_that_is_rubys_timeout_method/ [2]: https://jvns.ca/blog/2015/11/27/why-rubys-timeout-is-dangerous-and-thread-dot-raise-is-terrifying/ [3]: https://adamhooper.medium.com/in-ruby-dont-use-timeout-77d9d4e5a001 [4]: https://ruby-doc.org/core-3.0.2/Thread.html#method-c-handle_interrupt-label-Guarding+from+Timeout-3A-3AError --- app/models/media_asset.rb | 22 ++++++++++++++++++---- test/functional/uploads_controller_test.rb | 14 ++++++++++++++ 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/app/models/media_asset.rb b/app/models/media_asset.rb index 4b88b720c..8336a5e5f 100644 --- a/app/models/media_asset.rb +++ b/app/models/media_asset.rb @@ -1,10 +1,19 @@ class MediaAsset < ApplicationRecord + class Error < StandardError; end + has_one :media_metadata, dependent: :destroy has_one :pixiv_ugoira_frame_data, class_name: "PixivUgoiraFrameData", foreign_key: :md5, primary_key: :md5 delegate :metadata, to: :media_metadata delegate :is_non_repeating_animation?, :is_greyscale?, :is_rotated?, to: :metadata + # Processing: The asset's files are currently being resized and distributed to the backend servers. + # Active: The asset has been successfully uploaded and is ready to use. + # Deleted: The asset's files have been deleted by moving them to a trash folder. They can be undeleted + # by moving them out of the trash folder. (Not implemented yet). + # Expunged: The asset's files have been permanently deleted. + # Failed: The asset failed to upload. The asset may be in a partially uploaded state, with some + # files missing or incompletely transferred. enum status: { processing: 100, active: 200, @@ -150,20 +159,25 @@ class MediaAsset < ApplicationRecord # XXX If the asset is still being processed by another thread, wait up # to 30 seconds for it to finish. - if media_asset.processing? + if media_asset.processing? && media_asset.created_at > 5.minutes.ago 30.times do break if !media_asset.processing? sleep 1 media_asset.reload end + end - # If the asset is still processing after 30 seconds, or if it moved - # from the processing state to the failed state, then fail. - raise "Upload failed" if !media_asset.active? + # If the asset is stuck in the processing state, or if a processing asset moved to the + # failed state, then mark the asset as failed so the user can try the upload again later. + if !media_asset.active? + media_asset.update!(status: :failed) + raise Error, "Upload failed, try again (upload was stuck in 'processing' state)" end media_asset rescue Exception + # If resizing or distributing the file to the backend servers failed, then mark the asset as + # failed so the user can try the upload again later. media_asset&.update!(status: :failed) raise end diff --git a/test/functional/uploads_controller_test.rb b/test/functional/uploads_controller_test.rb index 34327158d..1284c472b 100644 --- a/test/functional/uploads_controller_test.rb +++ b/test/functional/uploads_controller_test.rb @@ -284,6 +284,20 @@ class UploadsControllerTest < ActionDispatch::IntegrationTest end end + context "when re-uploading a media asset stuck in the 'processing' state" do + should "mark the asset as failed" do + asset = create(:media_asset, file: File.open("test/files/test.jpg"), status: "processing") + file = Rack::Test::UploadedFile.new("test/files/test.jpg") + + post_auth uploads_path, @user, params: { upload: { file: file, tag_string: "abc", rating: "e", }} + upload = Upload.last + + assert_redirected_to upload + assert_match("MediaAsset::Error", upload.reload.status) + assert_equal("failed", asset.reload.status) + end + end + context "uploading a file from your computer" do should_upload_successfully("test/files/test.jpg") should_upload_successfully("test/files/test.png")