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

Retry asset wait file processing timeouts #1150

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/models/apple/config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ def self.build_apple_config(podcast, key)
def self.mark_as_delivered!(apple_publisher)
apple_publisher.episodes_to_sync.each do |episode|
if episode.podcast_container&.needs_delivery? == false
episode.feeder_episode.apple_has_delivery!
episode.feeder_episode.apple_mark_as_delivered!
end
end
end
Expand Down
18 changes: 15 additions & 3 deletions app/models/apple/episode.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,8 @@ class Episode
EPISODE_ASSET_WAIT_TIMEOUT = 15.minutes.freeze
EPISODE_ASSET_WAIT_INTERVAL = 10.seconds.freeze

# Cleans up old delivery/delivery files iff the episode is to be delivered
# Cleans up old delivery/delivery files iff the episode is to be uploaded
def self.prepare_for_delivery(episodes)
episodes = episodes.select { |ep| ep.needs_delivery? }

episodes.map do |ep|
Rails.logger.info("Preparing episode #{ep.feeder_id} for delivery", {episode_id: ep.feeder_id})
ep.feeder_episode.apple_prepare_for_delivery!
Expand Down Expand Up @@ -575,5 +573,19 @@ def apple_episode_delivery_statuses
alias_method :delivery_files, :podcast_delivery_files
alias_method :delivery_status, :apple_episode_delivery_status
alias_method :delivery_statuses, :apple_episode_delivery_statuses
alias_method :apple_status, :apple_episode_delivery_status

# Delegate methods to feeder_episode
def method_missing(method_name, *arguments, &block)
if feeder_episode.respond_to?(method_name)
feeder_episode.send(method_name, *arguments, &block)
else
super
end
end

def respond_to_missing?(method_name, include_private = false)
feeder_episode.respond_to?(method_name) || super
end
end
end
18 changes: 18 additions & 0 deletions app/models/apple/episode_delivery_status.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,23 @@ def increment_asset_wait
def reset_asset_wait
self.class.update_status(episode, asset_processing_attempts: 0)
end

def mark_as_uploaded!
self.class.update_status(episode, uploaded: true)
end

def mark_as_not_uploaded!
self.class.update_status(episode, uploaded: false)
end

# Whether the media file has been uploaded to Apple
# is a subset of whether the episode has been delivered
def mark_as_delivered!
self.class.update_status(episode, delivered: true, uploaded: true)
end

def mark_as_not_delivered!
self.class.update_status(episode, delivered: false, uploaded: false)
end
end
end
6 changes: 0 additions & 6 deletions app/models/apple/podcast_delivery.rb
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,6 @@ def self.create_podcast_deliveries(api, episodes)
end

# Don't create deliveries for containers that already have deliveries.
# An alternative workflow would be to swap out the existing delivery and
# upload different audio.
#
# The overall publishing workflow dependes on the assumption that there is
# a delivery present. If we don't create a delivery here, we short-circuit
# subsequent steps (no uploads, no audio linking).
episodes = select_episodes_for_delivery(episodes)
podcast_containers = episodes.map(&:podcast_container)

Expand Down
81 changes: 56 additions & 25 deletions app/models/apple/publisher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -131,33 +131,49 @@ def publish!
def deliver_and_publish!(eps)
Rails.logger.tagged("Apple::Publisher#deliver_and_publish!") do
eps.each_slice(PUBLISH_CHUNK_LEN) do |eps|
# Soft delete any existing delivery and delivery files
prepare_for_delivery!(eps)
eps.filter(&:apple_needs_upload?).tap do |eps|
upload_media!(eps)
end

# only create if needed
sync_episodes!(eps)
sync_podcast_containers!(eps)
process_and_deliver!(eps)

wait_for_versioned_source_metadata(eps)
raise_delivery_processing_errors(eps)
end
end
end

sync_podcast_deliveries!(eps)
sync_podcast_delivery_files!(eps)
def upload_media!(eps)
# Soft delete any existing delivery and delivery files
prepare_for_delivery!(eps)

# upload and mark as uploaded
execute_upload_operations!(eps)
mark_delivery_files_uploaded!(eps)
# only create if needed
sync_episodes!(eps)
sync_podcast_containers!(eps)

wait_for_upload_processing(eps)
wait_for_versioned_source_metadata(eps)

increment_asset_wait!(eps)
wait_for_asset_state(eps)
sync_podcast_deliveries!(eps)
sync_podcast_delivery_files!(eps)

publish_drafting!(eps)
reset_asset_wait!(eps)
# upload and mark as uploaded, then update the audio container reference
execute_upload_operations!(eps)
mark_delivery_files_uploaded!(eps)
update_audio_container_reference!(eps)

raise_delivery_processing_errors(eps)
end
end
# finally mark the episode as uploaded
mark_as_uploaded!(eps)
end

def process_and_deliver!(eps)
increment_asset_wait!(eps)

wait_for_upload_processing(eps)
wait_for_asset_state(eps)

mark_as_delivered!(eps)

publish_drafting!(eps)
reset_asset_wait!(eps)
end

def prepare_for_delivery!(eps)
Expand Down Expand Up @@ -231,9 +247,7 @@ def wait_for_upload_processing(eps)

def increment_asset_wait!(eps)
Rails.logger.tagged("##{__method__}") do
eps = eps.filter { |e| e.podcast_delivery_files.any?(&:api_marked_as_uploaded?) }

# Mark the episodes as waiting again for asset processing
eps = eps.filter { |e| e.feeder_episode.apple_status.uploaded? }
eps.each { |ep| ep.apple_episode_delivery_status.increment_asset_wait }
end
end
Expand Down Expand Up @@ -361,16 +375,33 @@ def mark_delivery_files_uploaded!(eps)
Rails.logger.tagged("##{__method__}") do
pdfs = eps.map(&:podcast_delivery_files).flatten
::Apple::PodcastDeliveryFile.mark_uploaded(api, pdfs)
end
end

def update_audio_container_reference!(eps)
Rails.logger.tagged("##{__method__}") do
# link the podcast container with the audio to the episode
res = Apple::Episode.update_audio_container_reference(api, eps)
# update the feeder episode to indicate that delivery is no longer needed

Rails.logger.info("Updated remote container references for episodes.", {count: res.length})
end
end

def mark_as_delivered!(eps)
Rails.logger.tagged("##{__method__}") do
eps.each do |ep|
Rails.logger.info("Marking episode as no longer needing delivery", {episode_id: ep.feeder_episode.id})
ep.feeder_episode.apple_has_delivery!
ep.feeder_episode.apple_mark_as_delivered!
end
end
end

Rails.logger.info("Updated remote container references for episodes.", {count: res.length})
def mark_as_uploaded!(eps)
Rails.logger.tagged("##{__method__}") do
eps.each do |ep|
Rails.logger.info("Marking episode as no longer needing delivery", {episode_id: ep.feeder_episode.id})
ep.feeder_episode.apple_mark_as_uploaded!
end
end
end

Expand Down
44 changes: 27 additions & 17 deletions app/models/concerns/apple_delivery.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,14 @@ module AppleDelivery
class_name: "Apple::PodcastDelivery"
has_many :apple_podcast_delivery_files, through: :apple_podcast_deliveries, source: :podcast_delivery_files,
class_name: "Apple::PodcastDeliveryFile"
has_many :apple_episode_delivery_statuses, -> { order(created_at: :desc) }, dependent: :destroy, class_name: "Apple::EpisodeDeliveryStatus"
has_many :apple_episode_delivery_statuses, -> { order(created_at: :desc) }, class_name: "Apple::EpisodeDeliveryStatus"

alias_method :podcast_container, :apple_podcast_container
alias_method :apple_status, :apple_episode_delivery_status
end

def publish_to_apple?
podcast.apple_config&.publish_to_apple?
podcast.apple_config&.publish_to_apple? || false
end

def apple_update_delivery_status(attrs)
Expand All @@ -34,29 +34,27 @@ def apple_episode_delivery_status
end

def apple_needs_delivery?
return true if apple_episode_delivery_status.nil?

apple_episode_delivery_status.delivered == false
end

def apple_needs_delivery!
apple_update_delivery_status(delivered: false)
def apple_needs_upload?
apple_episode_delivery_status.uploaded == false
end

def apple_has_delivery!
apple_update_delivery_status(delivered: true)
def apple_mark_as_not_delivered!
apple_episode_delivery_status.mark_as_not_delivered!
end

def measure_asset_processing_duration
statuses = apple_episode_delivery_statuses.to_a

last_status = statuses.shift
return nil unless last_status&.asset_processing_attempts.to_i.positive?
def apple_mark_as_delivered!
apple_episode_delivery_status.mark_as_delivered!
end

start_status = statuses.find { |status| status.asset_processing_attempts.to_i.zero? }
return nil unless start_status
def apple_mark_as_uploaded!
apple_episode_delivery_status.mark_as_uploaded!
end

Time.now - start_status.created_at
def apple_mark_as_not_uploaded!
apple_episode_delivery_status.mark_as_not_uploaded!
end

def apple_prepare_for_delivery!
Expand All @@ -68,7 +66,19 @@ def apple_prepare_for_delivery!
end

def apple_mark_for_reupload!
apple_needs_delivery!
apple_mark_as_not_delivered!
end

def measure_asset_processing_duration
statuses = apple_episode_delivery_statuses.to_a

last_status = statuses.shift
return nil unless last_status&.asset_processing_attempts.to_i.positive?

start_status = statuses.find { |status| status.asset_processing_attempts.to_i.zero? }
return nil unless start_status

Time.now - start_status.created_at
end

def apple_episode
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,10 @@ def change
needs_delivery_episodes = []
apple_episodes.each do |apple_episode|
if (apple_episode.podcast_container.nil? || apple_episode.podcast_container.needs_delivery?) || apple_episode.apple_hosted_audio_asset_container_id.blank?
apple_episode.feeder_episode.apple_needs_delivery!
apple_episode.feeder_episode.apple_mark_as_not_delivered!
needs_delivery_episodes << apple_episode.feeder_episode
else
apple_episode.feeder_episode.apple_has_delivery!
apple_episode.feeder_episode.apple_mark_as_delivered!
end
end

Expand Down
12 changes: 12 additions & 0 deletions db/migrate/20241029181938_new_file_uploaded_state.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
class NewFileUploadedState < ActiveRecord::Migration[7.2]
def change
add_column :apple_episode_delivery_statuses, :uploaded, :boolean, default: false

# Set the new column to match the delivered column
execute(<<~SQL
UPDATE apple_episode_delivery_statuses
SET uploaded = delivered
SQL
)
end
end
1 change: 1 addition & 0 deletions db/schema.rb

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion test/factories/apple_episode_api_response.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,14 @@

after(:build) do |response_container, evaluator|
response_container["api_response"] =
{"request_metadata" => {"apple_episode_id" => evaluator.apple_episode_id, "item_guid" => evaluator.item_guid},
{"request_metadata" => {"apple_episode_id" => evaluator.apple_episode_id, "guid" => evaluator.item_guid},
"api_url" => evaluator.api_url,
"api_parameters" => {},
"api_response" => {"ok" => evaluator.ok,
"err" => evaluator.err,
"val" => {"data" => {"id" => "123",
"attributes" => {
"appleHostedAudioAssetContainerId" => nil,
"appleHostedAudioAssetVendorId" => evaluator.apple_hosted_audio_asset_container_id,
"publishingState" => evaluator.publishing_state,
"guid" => evaluator.item_guid,
Expand Down
32 changes: 22 additions & 10 deletions test/factories/apple_episode_factory.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,20 @@
# set up transient api_response
transient do
feeder_episode { create(:episode) }
api_response { build(:apple_episode_api_response) }
api_response { build(:apple_episode_api_response, item_guid: feeder_episode.item_guid) }
apple_hosted_audio_asset_container_id { "456" }
end

# set a complete episode factory varient
factory :uploaded_apple_episode do
feeder_episode do
ep = create(:episode)
ep.apple_has_delivery!
ep
end
feeder_episode { create(:episode) }
transient do
apple_hosted_audio_asset_container_id { "456" }
api_response do
build(:apple_episode_api_response,
publishing_state: "PUBLISH",
item_guid: feeder_episode.item_guid,
apple_hosted_audio_asset_container_id: apple_hosted_audio_asset_container_id,
apple_hosted_audio_state: Apple::Episode::AUDIO_ASSET_SUCCESS)
end
end
Expand All @@ -32,11 +32,23 @@
api_marked_as_uploaded: true,
upload_operations_complete: true)

create(:content, episode: apple_episode.feeder_episode, position: 1, status: "complete")
create(:content, episode: apple_episode.feeder_episode, position: 2, status: "complete")
v1 = apple_episode.feeder_episode.cut_media_version!
feeder_episode = apple_episode.feeder_episode

# The content model calls Episode#publish!
# and that triggers a call to Episode#apple_mark_for_reupload!
# This modifies state to indicate that the episode needs to be reuploaded
create(:content, episode: feeder_episode, position: 1, status: "complete")
create(:content, episode: feeder_episode, position: 2, status: "complete")
v1 = feeder_episode.cut_media_version!

# Now model the case where the episode is uploaded.
# First we've gathered file metadata from the CDN
feeder_episode.apple_update_delivery_status(source_size: 1.megabyte,
source_url: "https://cdn.example.com/episode.mp3",
source_media_version_id: v1.id)

apple_episode.delivery_status.update!(delivered: true, source_media_version_id: v1.id)
# Then we've delivered (and necessarily uploaded)
feeder_episode.apple_mark_as_delivered!
end
end

Expand Down
2 changes: 1 addition & 1 deletion test/models/apple/episode_delivery_status_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ class Apple::EpisodeDeliveryStatusTest < ActiveSupport::TestCase
episode.destroy
assert_equal episode, delivery_status.episode
assert_difference "Apple::EpisodeDeliveryStatus.count", +1 do
episode.apple_needs_delivery!
episode.apple_mark_as_not_delivered!
end
assert_equal episode, episode.apple_episode_delivery_statuses.first.episode
end
Expand Down
Loading