From 7520a4a76d0a2ca902db72494754b381a4eef7ae Mon Sep 17 00:00:00 2001 From: Charlotte Van Petegem Date: Thu, 10 Oct 2024 22:31:03 +0200 Subject: [PATCH] Transcode to a temporary file which is only moved when finished --- app/jobs/create_transcoded_item_job.rb | 2 +- app/models/audio_file.rb | 8 ++++++ test/controllers/tracks_controller_test.rb | 5 ++-- test/jobs/create_transcoded_item_job_test.rb | 12 +++++---- test/models/audio_file_test.rb | 26 ++++++++++++++++++++ test/test_helper.rb | 19 ++++++++++++++ 6 files changed, 63 insertions(+), 9 deletions(-) diff --git a/app/jobs/create_transcoded_item_job.rb b/app/jobs/create_transcoded_item_job.rb index ce7821d2..6bf863a1 100644 --- a/app/jobs/create_transcoded_item_job.rb +++ b/app/jobs/create_transcoded_item_job.rb @@ -8,7 +8,7 @@ def perform(audio_file, codec_conversion) uuid = TranscodedItem.uuid_for(codec_conversion) path = TranscodedItem.path_for(codec_conversion, uuid) FileUtils.mkdir_p Pathname.new(path).parent - audio_file.convert(codec_conversion, path) + audio_file.convert_with_tmpfile(codec_conversion, path) TranscodedItem.transaction do # Check that the transcoded item was not created while we were executing diff --git a/app/models/audio_file.rb b/app/models/audio_file.rb index 9e53a59d..36013d24 100644 --- a/app/models/audio_file.rb +++ b/app/models/audio_file.rb @@ -37,6 +37,14 @@ def check_self false end + def convert_with_tmpfile(codec_conversion, out_file_name) + tmp_path = File.join(Dir.tmpdir, "accentor_transcode-#{id}-#{codec_conversion.id}-#{SecureRandom.uuid}") + convert(codec_conversion, tmp_path) + FileUtils.mv tmp_path, out_file_name + ensure + FileUtils.rm_f tmp_path + end + def convert(codec_conversion, out_file_name) parameters = codec_conversion.ffmpeg_params.split _stdout, status = Open3.capture2( diff --git a/test/controllers/tracks_controller_test.rb b/test/controllers/tracks_controller_test.rb index 53493d34..d1dd37d1 100644 --- a/test/controllers/tracks_controller_test.rb +++ b/test/controllers/tracks_controller_test.rb @@ -272,12 +272,11 @@ class TracksControllerTest < ActionDispatch::IntegrationTest class TracksControllerAudioTest < ActionDispatch::IntegrationTest setup do sign_in_as(create(:user)) - AudioFile.alias_method :old_convert, :convert - AudioFile.define_method :convert, ->(_codec_conversion, out_file_name) { FileUtils.cp full_path, out_file_name } + install_audio_file_convert_stub end teardown do - AudioFile.alias_method :convert, :old_convert + uninstall_audio_file_convert_stub end test 'should return not_found if codec_conversion does not exit ' do diff --git a/test/jobs/create_transcoded_item_job_test.rb b/test/jobs/create_transcoded_item_job_test.rb index c439f80f..dc1e828c 100644 --- a/test/jobs/create_transcoded_item_job_test.rb +++ b/test/jobs/create_transcoded_item_job_test.rb @@ -7,12 +7,11 @@ class CreateTranscodedItemJobTest < ActiveJob::TestCase @audio_file = create(:audio_file, location:, filename: '/base.flac', codec: flac) @codec_conversion = create(:codec_conversion) - AudioFile.alias_method :old_convert, :convert - AudioFile.define_method :convert, ->(_codec_conversion, out_file_name) { FileUtils.cp full_path, out_file_name } + install_audio_file_convert_stub end teardown do - AudioFile.alias_method :convert, :old_convert + uninstall_audio_file_convert_stub end test 'should create a new TranscodedItem with existing file' do @@ -34,11 +33,14 @@ class CreateTranscodedItemJobTest < ActiveJob::TestCase test 'should abort when TranscodedItem was created while converting' do audio_file = @audio_file codec_conversion = @codec_conversion - AudioFile.define_method :convert, lambda { |_codec_conversion, _out_file_name| + AudioFile.define_method :convert, lambda { |_codec_conversion, out_file_name| + FileUtils.touch out_file_name TranscodedItem.create!(audio_file:, codec_conversion:, uuid: '0000-0000-0000') } - FileUtils.stubs(:rm_f).once + # Once during `convert_with_tmpfile`, once when noticing that the convert + # wasn't necessary anymore. + FileUtils.stubs(:rm_f).twice CreateTranscodedItemJob.perform_now(@audio_file, @codec_conversion) end diff --git a/test/models/audio_file_test.rb b/test/models/audio_file_test.rb index b27a328d..a13151bb 100644 --- a/test/models/audio_file_test.rb +++ b/test/models/audio_file_test.rb @@ -51,4 +51,30 @@ def setup @audio_file.convert(codec_conversion, TranscodedItem.path_for(codec_conversion, SecureRandom.uuid)) end end + + test 'convert_with_tmpfile should clean up after itself' do + codec_conversion = create(:codec_conversion) + path = TranscodedItem.path_for(codec_conversion, SecureRandom.uuid) + sent_filename = nil + FileUtils.stubs(:mv).once.with { |tmp_path, out_file_name| out_file_name == path && tmp_path == sent_filename } + with_stubbed_audio_file_convert ->(_codec_conversion, out_file_name) { sent_filename = out_file_name } do + @audio_file.convert_with_tmpfile(codec_conversion, path) + end + + assert_not_equal sent_filename, path + end + + test 'convert_with_tmpfile should clean up after itself even when raising' do + sent_filename = nil + FileUtils.stubs(:rm_f).once.with { |tmp_path| tmp_path == sent_filename } + with_stubbed_audio_file_convert lambda { |_codec_conversion, out_file_name| + sent_filename = out_file_name + raise AudioFile::FailedTranscode, 'oh no' + } do + codec_conversion = create(:codec_conversion) + assert_raises AudioFile::FailedTranscode, 'oh no' do + @audio_file.convert_with_tmpfile(codec_conversion, TranscodedItem.path_for(codec_conversion, SecureRandom.uuid)) + end + end + end end diff --git a/test/test_helper.rb b/test/test_helper.rb index 59ee1dbe..c08fa700 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -15,9 +15,28 @@ require 'rails/test_help' require 'mocha/minitest' +module AudioFileTestHelper + def install_audio_file_convert_stub(implementation = nil) + AudioFile.alias_method :old_convert, :convert + AudioFile.define_method :convert, implementation || ->(_codec_conversion, out_file_name) { FileUtils.cp full_path, out_file_name } + end + + def uninstall_audio_file_convert_stub + AudioFile.alias_method :convert, :old_convert + end + + def with_stubbed_audio_file_convert(implementation = nil) + install_audio_file_convert_stub implementation + yield + ensure + uninstall_audio_file_convert_stub + end +end + class ActiveSupport::TestCase include FactoryBot::Syntax::Methods include ActiveJob::TestHelper + include AudioFileTestHelper # Run tests in parallel with specified workers parallelize(workers: :number_of_processors)