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

Apple retry uploads #707

Merged
merged 11 commits into from
Jul 25, 2023
Merged

Apple retry uploads #707

merged 11 commits into from
Jul 25, 2023

Conversation

svevang
Copy link
Member

@svevang svevang commented Jul 17, 2023

closes #685

This sets up a series of changes to help with upload retries:

  • shifts some of the attention from Delivery + DeliveryFile over to the PodcastContainer. The PodcastContainer's files attributes is a proxy to the validation and processing status on the Delivery/DeliveryFile models. It appears that Apple retains the overall container state and discards the delivery state, so likely building transitions in a state machine around the PodcastContainer will be better in the long run.
  • Fixes a case where the duplicate status in the delivery was treated as an error and not properly covered by the success/error conditionals
  • expands a skip_delivery? method and related helpers on the container to clarify retry logic.

app/models/apple/episode.rb Show resolved Hide resolved

# We don't get back the full episode model in the response.
# So poll for current state
poll_episode_state(api, show, episodes)
Copy link
Member Author

Choose a reason for hiding this comment

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

Tracking a few of these: There was a remote state transition, but the local data was not updated to reflect that. So make sure the local data is up to date.

podcast_delivery_files.all?(&:processed?) &&
!podcast_delivery_files.all?(&:processed_errors?) &&
!audio_asset_state_finished?)
podcast_container.delivery_settled? && !audio_asset_state_finished?
Copy link
Member Author

Choose a reason for hiding this comment

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

so now only looking the episode's audio_asset_state and at the podcast container (which manages / coordinates the deliveries).

# Only query for episodes that don't have a podcast container
# The container. Assume that if we have a container record, we don't need to poll.
eps_without_container = episodes.filter { |ep| ep.apple_persisted? && ep.missing_container? }

Copy link
Member Author

Choose a reason for hiding this comment

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

Trying to move special guards and state checks into more top level concerns (like PodcastContainer#delivery_settled?)


# Get the latest state of the podcast containers
# which should include synced files
Apple::PodcastContainer.poll_podcast_container_state(api, eps)
Copy link
Member Author

Choose a reason for hiding this comment

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

Again, missing local state, this time in the files Apple attribute -- which gives a readout of the successfully processed/delivered files.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm hitting this in prod (w/ test creds) just now. A podcast has episodes missing their container state, and upon retry I can't push that sync process forward. That should be fixed by this and other patches in this PR.

@@ -327,14 +327,14 @@ def delivered?
def processed_errors?
return false unless asset_processing_state.present?

processed_validation_failed? || processed_duplicate?
Copy link
Member Author

Choose a reason for hiding this comment

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

Uploading a duplicate file is not really an error.

Copy link
Member

Choose a reason for hiding this comment

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

+1

@svevang svevang marked this pull request as ready for review July 18, 2023 15:05
@svevang svevang requested a review from cavis July 19, 2023 20:12
Copy link
Member

@cavis cavis left a comment

Choose a reason for hiding this comment

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

👍 looks good to me!

app/models/apple/episode.rb Show resolved Hide resolved
pdfs = feeder_episode.apple_podcast_delivery_files

pdfs.present? && pdfs.to_a.flatten.all?(&:apple_complete?)
feeder_episode.apple_podcast_container.skip_delivery?
Copy link
Member

Choose a reason for hiding this comment

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

Bit odd conceptually (the upload is complete when we're skipping the delivery?) Though I guess it's all the same - we skip the delivery when the upload is complete. Same check.

@@ -327,14 +327,14 @@ def delivered?
def processed_errors?
return false unless asset_processing_state.present?

processed_validation_failed? || processed_duplicate?
Copy link
Member

Choose a reason for hiding this comment

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

+1

@@ -385,8 +385,8 @@ def drafting?
apple_json&.dig("attributes", "publishingState") == "DRAFTING"
end

def apple_upload_complete?
feeder_episode.apple_podcast_container.skip_delivery?
def container_upload_complete?
Copy link
Member Author

Choose a reason for hiding this comment

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

@cavis I hope this clarifies. I renamed to emphasize the container upload semantics here. We need to deliver the audio file (Delivery, DeliveryFiles) and the container upload (PodcastContaner files attribute) will be complete (a file marked present).

The overall Apple::Episode#synced_with_apple? covers the rest. That method filters the episodes that go into the publishing routines in Apple::Publisher.

@svevang svevang merged commit 8e41df1 into main Jul 25, 2023
1 check passed
@svevang svevang deleted the apple-retry-uploads branch July 25, 2023 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Retry Apple uploads
2 participants