Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[deliver] introduce timeout for screenshots processing waiting time #21693

Merged
merged 3 commits into from Jan 15, 2024
Merged

[deliver] introduce timeout for screenshots processing waiting time #21693

merged 3 commits into from Jan 15, 2024

Conversation

mikhailmaslo
Copy link
Contributor

@mikhailmaslo mikhailmaslo commented Dec 4, 2023

Checklist

  • I've run bundle exec rspec from the root directory to see all new and existing tests pass
  • I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
  • I see several green ci/circleci builds in the "All checks have passed" section of my PR (connect CircleCI to GitHub if not)
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.
  • I've added or updated relevant unit tests.

Motivation and Context

Resolves #20619

I support two applications with 20+ languages and over 200 screenshots.

  • It addresses an open issue Deliver action keep in Waiting for all the screenshots to finish being processed forever #20619 which has gained significant community attention

  • fastlane doesn't have timeout on screenshots processing, so if some screenshots get stuck in a processing state, which happens quite often when there are hundreds of screenshots, the process will be endless.

  • Since fastlane doesn't have processing time timeout, a user has several options to handle stuck screenshots:

    • remove screenshots by themselves directly in App Store Connect, then fastlane will re-upload screenshots automatically
      • there's no info on which screenshot stuck - so the user ends up looking up all of the localizations and all the screenshots
      • if there are several multi-language applications that get regularly stuck, updating screenshots becomes a nightmare
      • it's not really an automated process
    • terminate fastlane process and repeat the deliver
      • screenshots get stuck regularly when there are hundreds of them, so retrying the whole bunch doesn't eventually succeed
    • slowing down the upload (for example, by setting DELIVER_NUMBER_OF_THREADS to 1) and hoping that App Store Connect will be able to process them
      • doesn't really work - screenshots stuck with the same regularity as with DELIVER_NUMBER_OF_THREADS equals to 10

As a result, neither of the attempted solutions doesn't really work for multi-language applications, so we need to introduce some workaround

Description

The proposed solution is based on existing logic, which removes & re-uploads screenshots that failed to upload.
The screenshot_processing_timeout parameter is introduced within the deliver action. This parameter allows users to define a timeout period after which screenshots that are still processing are considered stuck (failed) and then existing logic with removing & reuploading comes into play.

  • Added screenshot_processing_timeout parameter to deliver which defines the timeout of waiting before considering currently processing screenshots as failed ones.
  • Screenshots that considered as failed removed from App Store Connect, then uploading is repeated. By doing so, one retry is used
  • As a result, from the user perspective now it's possible to specify a reasonable timeout after which deliver attempts to re-upload processing screenshots

Chosen constants:

  • 1hr is default value for screenshot_processing_timeout. It is chosen with backward compatibility in mind. I assume for anyone who uses deliver, 1hr should be sufficient enough to not break existing logic

Testing Steps

To test implementation, you should add hundreds of screenshots to App Store Connect with deliver. In my case, I have about 20 localizations with 10-12 screenshots in each (5.5 and 6.5 screens)
I've tested with 2 minutes for screenshot_processing_timeout. In my experience, it's usually enough for spotting stuck screenshots inside the EU. However, screenshot_processing_timeout may depend on your location, bandwidth, etc
In my experience I've encountered 2-3 per ~200 screenshots, so it's about 1%


To test this branch, modify your Gemfile as:

gem 'fastlane', git: 'https://github.com/mikhailmaslo/fastlane.git', branch: 'feature/introduce-timeout-for-screenshots-upload'

And run bundle install to apply the changes.

@lacostej lacostej changed the title Introduce timeout for screenshots processing waiting time in deliver [deliver] Introduce timeout for screenshots processing waiting time Dec 11, 2023
Copy link
Member

@rogerluan rogerluan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @mikhailmaslo 👋

Congrats on your first PR to the fastlane project! 🎉

I left a few comments, mostly light suggestions/questions.

I faced timeout issues myself when uploading screenshots manually (lucky enough to never have faced them when using fastlane), so I think this will be a good improvement! I'm sure many fastlane users agree, given the popularity of the feature request 🙏 thanks for this!

PS: Thank you for the thorough PR description, I really appreciate those!! ❤️

deliver/lib/deliver/upload_screenshots.rb Outdated Show resolved Hide resolved
deliver/lib/deliver/upload_screenshots.rb Outdated Show resolved Hide resolved
deliver/spec/upload_screenshots_spec.rb Show resolved Hide resolved
Copy link
Member

@rogerluan rogerluan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks @mikhailmaslo ! 🚀

@rogerluan rogerluan changed the title [deliver] Introduce timeout for screenshots processing waiting time [deliver] introduce timeout for screenshots processing waiting time Dec 28, 2023
@mikhailmaslo
Copy link
Contributor Author

Hi @lacostej,

I've made some updates in the wording of the user message (thanks to @ rogerluan) and removed unused default parameter. Could you please take a look when you have a moment?

Here are the links:

Copy link
Collaborator

@lacostej lacostej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mikhailmaslo
Copy link
Contributor Author

Hi, @rogerluan!

I've just rebased to the up to date master and all tests have passed

Shell we merge this PR?)

@rogerluan rogerluan merged commit 627dca3 into fastlane:master Jan 15, 2024
3 checks passed
@rogerluan
Copy link
Member

Thanks for this @mikhailmaslo 🤗 it'll be included in the next release!

@mikhailmaslo mikhailmaslo deleted the feature/introduce-timeout-for-screenshots-upload branch January 15, 2024 08:56
@tokudu
Copy link

tokudu commented Feb 19, 2024

Is there an ETA for the next release that will include this change?

@harikishan79
Copy link

Need a ETA for this release please. I have been wating on this forever now.

@jjochen
Copy link
Contributor

jjochen commented Mar 8, 2024

Need a ETA for this release please. I have been wating on this forever now.

While you are waiting for the release, you could just use the master branch directly:

gem 'fastlane', github: 'fastlane/fastlane', branch: 'master'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deliver action keep in Waiting for all the screenshots to finish being processed forever
6 participants