[deliver] introduce timeout for screenshots processing waiting time (…

…#21693)

* Introduce `timeout` to deliver for screenshots processing waiting time

* Remove timeout default value

* Make logs more readable

Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>

---------

Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>

deliver/lib/deliver/options.rb

@@ -162,6 +162,11 @@ def self.available_options
                                      description: "Clear all previously uploaded screenshots before uploading the new ones",
                                      type: Boolean,
                                      default_value: false),
+        FastlaneCore::ConfigItem.new(key: :screenshot_processing_timeout,
+                                     env_name: "DELIVER_SCREENSHOT_PROCESSING_TIMEOUT",
+                                     description: "Timeout in seconds to wait before considering screenshot processing as failed, used to handle cases where uploads to the App Store are stuck in processing",
+                                     type: Integer,
+                                     default_value: 3600),
         FastlaneCore::ConfigItem.new(key: :sync_screenshots,
                                      env_name: "DELIVER_SYNC_SCREENSHOTS",
                                      description: "Sync screenshots with local ones. This is currently beta option so set true to 'FASTLANE_ENABLE_BETA_DELIVER_SYNC_SCREENSHOTS' environment variable as well",

deliver/lib/deliver/upload_screenshots.rb

@@ -55,7 +55,7 @@ def upload(options, screenshots)
         localizations = version.get_app_store_version_localizations
       end
 
-      upload_screenshots(localizations, screenshots_per_language)
+      upload_screenshots(localizations, screenshots_per_language, options[:screenshot_processing_timeout])
 
       Helper.show_loading_indicator("Sorting screenshots uploaded...")
       sort_screenshots(localizations)
@@ -109,7 +109,7 @@ def delete_screenshots(localizations, screenshots_per_language, tries: 5)
       end
     end
 
-    def upload_screenshots(localizations, screenshots_per_language, tries: 5)
+    def upload_screenshots(localizations, screenshots_per_language, timeout_seconds, tries: 5)
       tries -= 1
 
       # Upload screenshots
@@ -158,15 +158,16 @@ def upload_screenshots(localizations, screenshots_per_language, tries: 5)
       UI.verbose('Uploading jobs are completed')
 
       Helper.show_loading_indicator("Waiting for all the screenshots to finish being processed...")
-      states = wait_for_complete(iterator)
+      states = wait_for_complete(iterator, timeout_seconds)
       Helper.hide_loading_indicator
-      retry_upload_screenshots_if_needed(iterator, states, total_number_of_screenshots, tries, localizations, screenshots_per_language)
+      retry_upload_screenshots_if_needed(iterator, states, total_number_of_screenshots, tries, timeout_seconds, localizations, screenshots_per_language)
 
       UI.message("Successfully uploaded all screenshots")
     end
 
     # Verify all screenshots have been processed
-    def wait_for_complete(iterator)
+    def wait_for_complete(iterator, timeout_seconds)
+      start_time = Time.now
       loop do
         states = iterator.each_app_screenshot.map { |_, _, app_screenshot| app_screenshot }.each_with_object({}) do |app_screenshot, hash|
           state = app_screenshot.asset_delivery_state['state']
@@ -177,16 +178,22 @@ def wait_for_complete(iterator)
         is_processing = states.fetch('UPLOAD_COMPLETE', 0) > 0
         return states unless is_processing
 
+        if Time.now - start_time > timeout_seconds
+          UI.important("Screenshot upload reached the timeout limit of #{timeout_seconds} seconds. We'll now retry uploading the screenshots that couldn't be uploaded in time.")
+          return states
+        end
+
         UI.verbose("There are still incomplete screenshots - #{states}")
         sleep(5)
       end
     end
 
     # Verify all screenshots states on App Store Connect are okay
-    def retry_upload_screenshots_if_needed(iterator, states, number_of_screenshots, tries, localizations, screenshots_per_language)
+    def retry_upload_screenshots_if_needed(iterator, states, number_of_screenshots, tries, timeout_seconds, localizations, screenshots_per_language)
       is_failure = states.fetch("FAILED", 0) > 0
+      is_processing = states.fetch('UPLOAD_COMPLETE', 0) > 0
       is_missing_screenshot = !screenshots_per_language.empty? && !verify_local_screenshots_are_uploaded(iterator, screenshots_per_language)
-      return unless is_failure || is_missing_screenshot
+      return unless is_failure || is_missing_screenshot || is_processing
 
       if tries.zero?
         iterator.each_app_screenshot.select { |_, _, app_screenshot| app_screenshot.error? }.each do |localization, _, app_screenshot|
@@ -200,7 +207,7 @@ def retry_upload_screenshots_if_needed(iterator, states, number_of_screenshots,
         iterator.each_app_screenshot do |_, _, app_screenshot|
           app_screenshot.delete! unless app_screenshot.complete?
         end
-        upload_screenshots(localizations, screenshots_per_language, tries: tries)
+        upload_screenshots(localizations, screenshots_per_language, timeout_seconds, tries: tries)
       end
     end
 

deliver/spec/upload_screenshots_spec.rb

@@ -79,7 +79,7 @@
           allow(described_class).to receive(:calculate_checksum).and_return('checksum')
 
           expect(app_screenshot_set).to receive(:upload_screenshot).with(path: local_screenshot.path, wait_for_processing: false)
-          subject.upload_screenshots([localization], screenshots_per_language)
+          subject.upload_screenshots([localization], screenshots_per_language, 3600)
         end
       end
 
@@ -100,7 +100,7 @@
 
           allow(FastlaneCore::Helper).to receive(:show_loading_indicator).and_return(true)
           expect(app_screenshot_set).to receive(:upload_screenshot).with(path: local_screenshot.path, wait_for_processing: false)
-          subject.upload_screenshots([localization], screenshots_per_language)
+          subject.upload_screenshots([localization], screenshots_per_language, 3600)
         end
       end
     end
@@ -122,7 +122,7 @@
 
         allow(FastlaneCore::Helper).to receive(:show_loading_indicator).and_return(true)
         expect(app_screenshot_set).to receive(:upload_screenshot).with(path: local_screenshot.path, wait_for_processing: false)
-        subject.upload_screenshots([localization], screenshots_per_language)
+        subject.upload_screenshots([localization], screenshots_per_language, 3600)
       end
     end
 
@@ -145,7 +145,7 @@
 
         allow(FastlaneCore::Helper).to receive(:show_loading_indicator).and_return(true)
         expect(app_screenshot_set).to_not(receive(:upload_screenshot))
-        subject.upload_screenshots([localization], screenshots_per_language)
+        subject.upload_screenshots([localization], screenshots_per_language, 3600)
       end
     end
 
@@ -166,7 +166,7 @@
 
         allow(FastlaneCore::Helper).to receive(:show_loading_indicator).and_return(true)
         expect(app_screenshot_set).to receive(:upload_screenshot).exactly(10).times
-        subject.upload_screenshots([localization], screenshots_per_language)
+        subject.upload_screenshots([localization], screenshots_per_language, 3600)
       end
     end
 
@@ -194,7 +194,7 @@
 
         allow(FastlaneCore::Helper).to receive(:show_loading_indicator).and_return(true)
         expect(app_screenshot_set).to_not(receive(:upload_screenshot))
-        subject.upload_screenshots([localization], screenshots_per_language)
+        subject.upload_screenshots([localization], screenshots_per_language, 3600)
       end
     end
 
@@ -224,7 +224,7 @@
 
         allow(FastlaneCore::Helper).to receive(:show_loading_indicator).and_return(true)
         expect(app_screenshot_set).to_not(receive(:upload_screenshot))
-        subject.upload_screenshots([localization], screenshots_per_language)
+        subject.upload_screenshots([localization], screenshots_per_language, 3600)
       end
     end
   end
@@ -242,8 +242,10 @@
                               get_app_screenshot_sets: [app_screenshot_set])
         iterator = Deliver::AppScreenshotIterator.new([localization])
 
+        allow(Time).to receive(:now).and_return(0)
+
         expect(::Kernel).to_not(receive(:sleep))
-        expect(subject.wait_for_complete(iterator)).to eq('COMPLETE' => 1)
+        expect(subject.wait_for_complete(iterator, 3600)).to eq('COMPLETE' => 1)
       end
     end
 
@@ -258,9 +260,29 @@
                               get_app_screenshot_sets: [app_screenshot_set])
         iterator = Deliver::AppScreenshotIterator.new([localization])
 
+        allow(Time).to receive(:now).and_return(0)
+
         expect_any_instance_of(Object).to receive(:sleep).with(kind_of(Numeric)).once
         expect(app_screenshot).to receive(:asset_delivery_state).and_return({ 'state' => 'UPLOAD_COMPLETE' }, { 'state' => 'COMPLETE' })
-        expect(subject.wait_for_complete(iterator)).to eq('COMPLETE' => 1)
+        expect(subject.wait_for_complete(iterator, 3600)).to eq('COMPLETE' => 1)
+      end
+    end
+
+    context 'when timeout is exceeded' do
+      it 'should exit the loop and return the current states' do
+        app_screenshot = double('Spaceship::ConnectAPI::AppScreenshot', asset_delivery_state: { 'state' => 'UPLOAD_COMPLETE' })
+        app_screenshot_set = double('Spaceship::ConnectAPI::AppScreenshotSet',
+                                    screenshot_display_type: Spaceship::ConnectAPI::AppScreenshotSet::DisplayType::APP_IPHONE_55,
+                                    app_screenshots: [app_screenshot])
+        localization = double('Spaceship::ConnectAPI::AppStoreVersionLocalization',
+                              locale: 'en-US',
+                              get_app_screenshot_sets: [app_screenshot_set])
+        iterator = Deliver::AppScreenshotIterator.new([localization])
+        states = { 'UPLOAD_COMPLETE' => 1 }
+
+        allow(Time).to receive(:now).and_return(0, 3601)
+
+        expect(subject.wait_for_complete(iterator, 3600)).to eq(states)
       end
     end
   end
@@ -279,7 +301,7 @@
         states = { 'FAILD' => 0, 'COMPLETE' => 1 }
 
         expect(subject).to_not(receive(:upload_screenshots))
-        subject.retry_upload_screenshots_if_needed(iterator, states, 1, 1, [], {})
+        subject.retry_upload_screenshots_if_needed(iterator, states, 1, 1, 0, [], {})
       end
     end
 
@@ -297,7 +319,7 @@
 
         expect(subject).to receive(:upload_screenshots).with(any_args)
         expect(app_screenshot).to receive(:delete!)
-        subject.retry_upload_screenshots_if_needed(iterator, states, 1, 1, [], {})
+        subject.retry_upload_screenshots_if_needed(iterator, states, 1, 1, 0, [], {})
       end
     end
 
@@ -314,7 +336,7 @@
         states = { 'FAILED' => 1, 'COMPLETE' => 0 }
 
         expect(subject).to receive(:upload_screenshots).with(any_args)
-        subject.retry_upload_screenshots_if_needed(iterator, states, 999, 1, [], {})
+        subject.retry_upload_screenshots_if_needed(iterator, states, 999, 1, 0, [], {})
       end
     end
 
@@ -336,7 +358,7 @@
 
         expect(subject).to_not(receive(:upload_screenshots).with(any_args))
         expect(UI).to receive(:user_error!)
-        subject.retry_upload_screenshots_if_needed(iterator, states, 1, 0, [], {})
+        subject.retry_upload_screenshots_if_needed(iterator, states, 1, 0, 0, [], {})
       end
     end
 
@@ -357,7 +379,30 @@
 
         expect(subject).to_not(receive(:upload_screenshots).with(any_args))
         expect(UI).to_not(receive(:user_error!))
-        subject.retry_upload_screenshots_if_needed(iterator, states, number_of_screenshots, 0, [], screenshots_per_language)
+        subject.retry_upload_screenshots_if_needed(iterator, states, number_of_screenshots, 0, 0, [], screenshots_per_language)
+      end
+    end
+
+    context 'when screenshots are in UPLOAD_COMPLETE state' do
+      it 'should trigger retry logic' do
+        app_screenshot = double('Spaceship::ConnectAPI::AppScreenshot',
+                                'complete?' => false,
+                                'error?' => false,
+                                file_name: '5.5_1.jpg',
+                                error_messages: ['error_message'])
+        app_screenshot_set = double('Spaceship::ConnectAPI::AppScreenshotSet',
+                                    screenshot_display_type: Spaceship::ConnectAPI::AppScreenshotSet::DisplayType::APP_IPHONE_55,
+                                    app_screenshots: [app_screenshot])
+        localization = double('Spaceship::ConnectAPI::AppStoreVersionLocalization',
+                              locale: 'en-US',
+                              get_app_screenshot_sets: [app_screenshot_set])
+        localizations = [localization]
+        iterator = Deliver::AppScreenshotIterator.new(localizations)
+        states = { 'UPLOAD_COMPLETE' => 1 }
+
+        expect(subject).to receive(:upload_screenshots).with(any_args)
+        expect(app_screenshot).to receive(:delete!)
+        subject.retry_upload_screenshots_if_needed(iterator, states, 1, 1, 0, localizations, {})
       end
     end
   end