diff --git a/app/src/androidTest/java/org/libremediaconverter/join/ConcatEngineTest.kt b/app/src/androidTest/java/org/libremediaconverter/join/ConcatEngineTest.kt index 745b8aa..4a7d6cb 100644 --- a/app/src/androidTest/java/org/libremediaconverter/join/ConcatEngineTest.kt +++ b/app/src/androidTest/java/org/libremediaconverter/join/ConcatEngineTest.kt @@ -13,6 +13,7 @@ import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import org.libremediaconverter.convert.MediaProbe +import org.libremediaconverter.convert.StagingNames import org.libremediaconverter.ffmpeg.ConcatEngine import org.libremediaconverter.model.ConcatStrategy import java.io.File @@ -152,9 +153,12 @@ class ConcatEngineTest { fun theListFileIsCleanedUpAfterJoining(): Unit = runBlocking { val out = output("joined_cleanup.mp4") engine.join(listOf(Uri.fromFile(clipA), Uri.fromFile(clipB)), out) + // Asked of StagingNames rather than spelled out: the list file used to be the constant + // concat_list.txt, and a literal here would have gone on passing vacuously once the name + // moved -- it would be asserting that a file nothing creates does not exist. assertTrue( "the concat list file was left behind", - !File(out.parentFile, "concat_list.txt").exists(), + !File(out.parentFile, StagingNames.concatListFor(out.name)).exists(), ) } diff --git a/app/src/main/java/org/libremediaconverter/LibreMediaConverterApp.kt b/app/src/main/java/org/libremediaconverter/LibreMediaConverterApp.kt index c6332cd..c25cd5f 100644 --- a/app/src/main/java/org/libremediaconverter/LibreMediaConverterApp.kt +++ b/app/src/main/java/org/libremediaconverter/LibreMediaConverterApp.kt @@ -43,7 +43,7 @@ class LibreMediaConverterApp : Application() { // // - Conversion and join outputs are written continuously, so a running job keeps // its own mtime fresh and never looks abandoned. - // - concat_list.txt is the one file written once and then only read, so it is the + // - A join's list file is the one written once and then only read, so it is the // one that has to be reasoned about rather than observed. A WorkManager attempt // is capped by the six-hour-per-day foreground-service budget and a retry starts // doWork() again from the top, rewriting the list file -- so no single attempt diff --git a/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt b/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt index 6cc9978..59dc96b 100644 --- a/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt +++ b/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt @@ -182,8 +182,8 @@ class ConversionViewModel @JvmOverloads constructor( if (_state.value !is ConversionState.Idle || activeWorkId != null) return@launch // Only a job that is the sole explanation for its staged file gets to name the input. - // When several jobs report the same file — which nothing prevents while the staging - // name is derived from the input's display name — the file is still the user's, but + // When several jobs report the same file — which staging on the job id has stopped for + // new work, but not for work already in the queue — the file is still the user's, but // saying which of them produced it would be a guess, so the card falls back to a // neutral label rather than borrowing the other job's. val tags = (reattachment as? Reattachment.Certain)?.job?.tags.orEmpty() diff --git a/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt b/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt index 59a847b..6636dcb 100644 --- a/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt +++ b/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt @@ -73,9 +73,10 @@ open class OutputPublisher(private val context: Context) { * Activity. [StagingSweep] owns the rule and its reasoning. * * Deliberately not the `clearStaging()` this replaces. That deleted the directory's - * whole contents, and the convert tab, the join tab and `ConcatEngine`'s - * `concat_list.txt` all share this directory with no per-job namespacing, so a blanket - * delete could destroy a live job's file. + * whole contents, and the convert tab, the join tab and `ConcatEngine`'s list file all + * share this directory — so a blanket delete could destroy a live job's file. Per-job + * staging names ([StagingNames]) stop two jobs from *sharing* a file; they say nothing + * about whether a file's job is still running, which is the question here. * * [nowMs] is a parameter so the clock is the caller's, not a hidden global. */ diff --git a/app/src/main/java/org/libremediaconverter/convert/StagingNames.kt b/app/src/main/java/org/libremediaconverter/convert/StagingNames.kt new file mode 100644 index 0000000..c00f4a4 --- /dev/null +++ b/app/src/main/java/org/libremediaconverter/convert/StagingNames.kt @@ -0,0 +1,52 @@ +package org.libremediaconverter.convert + +import java.util.UUID + +/** + * Gives every job a staging path of its own. + * + * `/conversions/` is shared by the convert tab, the join tab and + * [org.libremediaconverter.ffmpeg.ConcatEngine]'s list file, and until this existed none of the + * three named a file that belonged to one job. A conversion derived its name from the input's + * display name, so two `holiday.mp4` from different folders collided; a join used the constant + * `joined.`, so any two joins of one format collided; the list file was the constant + * `concat_list.txt`, so any two joins at all collided. + * + * The collision was not theoretical. Two independent conversions on a Pixel each produced + * `cache/conversions/input_converted.mp4`, and a tag query in a fresh process returned two + * SUCCEEDED `WorkInfo`s naming that one file — which is what leaves reattachment unable to say + * which job the bytes on disk belong to. + * + * ## Why the job id, and why opaque + * + * The WorkManager request id is stable across retries: `WorkerWrapper` builds `WorkerParameters` + * from the `WorkSpec` id and only increments `runAttemptCount`. That matters more than uniqueness + * does — a retry runs `doWork()` from the top, and the delete on the way out of a failed attempt + * only collects the previous attempt's partial when the name has not moved. + * + * The staged name is never shown to anyone: `save()` recomputes a suggested name from the job's + * own spec, and the user picks the real one in the SAF dialog. So there is nothing to lose by + * making it opaque, and something to gain — the alternative was sanitising a provider-supplied + * display name, which can contain a separator, be empty, or be four kilobytes long. Naming the job + * retires that question instead of answering it. + * + * The extension is kept, and is not decoration. `FFmpegConcatCommand` names no output muxer, so + * FFmpeg infers it from the path; a name without the right extension would quietly produce the + * wrong container. + */ +object StagingNames { + + /** The staging filename for the job with this [jobId], producing a file of type [extension]. */ + fun forJob(jobId: UUID, extension: String): String = "$jobId.$extension" + + /** + * The concat list file that belongs to the output staged as [outputName]. + * + * Derived from the output rather than taken as another parameter, so the two cannot drift + * apart and so a directory listing shows which list belongs to which join. It ages with its + * output too, which is what [StagingSweep] needs. + */ + fun concatListFor(outputName: String): String = outputName.substringBeforeLast('.', outputName) + CONCAT_LIST_SUFFIX + + private const val CONCAT_LIST_SUFFIX = ".concat_list.txt" +} diff --git a/app/src/main/java/org/libremediaconverter/convert/StagingSweep.kt b/app/src/main/java/org/libremediaconverter/convert/StagingSweep.kt index 63b5b4a..b54e39b 100644 --- a/app/src/main/java/org/libremediaconverter/convert/StagingSweep.kt +++ b/app/src/main/java/org/libremediaconverter/convert/StagingSweep.kt @@ -12,9 +12,13 @@ package org.libremediaconverter.convert * * The rule replaces an unconditional `clearStaging()` that deleted the directory's whole * contents. That was hazardous: `/conversions/` is shared by the convert tab, the - * join tab and [org.libremediaconverter.ffmpeg.ConcatEngine]'s `concat_list.txt`, and any - * two of them can be live at once, so a blanket delete could take a file out from under a - * running job. Age is the narrowing. + * join tab and [org.libremediaconverter.ffmpeg.ConcatEngine]'s list file, and any two of + * them can be live at once, so a blanket delete could take a file out from under a running + * job. Age is the narrowing. + * + * Per-job staging names ([StagingNames]) do not change that. They stop two jobs from writing + * one file; they do nothing about a sweep deleting a file whose job is still running, which + * is what age is for. */ object StagingSweep { @@ -26,7 +30,7 @@ object StagingSweep { * * Twenty-four hours, chosen against the longest a live file can plausibly go untouched * rather than against how quickly cache should be reclaimed. Output files are written - * continuously, so a running job refreshes their mtime by itself. `concat_list.txt` is + * continuously, so a running job refreshes their mtime by itself. A join's list file is * the exception — written once and then only read for the rest of the join — so the * period has to exceed a whole join. WorkManager caps a single attempt at the * six-hour-per-day foreground-service budget and then stops the worker, and a retry diff --git a/app/src/main/java/org/libremediaconverter/ffmpeg/ConcatEngine.kt b/app/src/main/java/org/libremediaconverter/ffmpeg/ConcatEngine.kt index 35ea0de..1afff26 100644 --- a/app/src/main/java/org/libremediaconverter/ffmpeg/ConcatEngine.kt +++ b/app/src/main/java/org/libremediaconverter/ffmpeg/ConcatEngine.kt @@ -8,6 +8,7 @@ import com.arthenica.ffmpegkit.FFmpegKitConfig import com.arthenica.ffmpegkit.ReturnCode import kotlinx.coroutines.suspendCancellableCoroutine import org.libremediaconverter.convert.MediaProbe +import org.libremediaconverter.convert.StagingNames import org.libremediaconverter.model.ConcatPlanner import org.libremediaconverter.model.ConcatStrategy import org.libremediaconverter.model.OutputFormat @@ -43,7 +44,12 @@ class ConcatEngine(private val context: Context) { // The demuxer reads its input list from a file, which must live somewhere // FFmpeg can read; app cache is a real path, so it just works. - val listFile = File(output.parentFile, "concat_list.txt").apply { + // + // Named after the output rather than by the constant "concat_list.txt" it used to use. + // The constant meant any two joins running at once shared one list file, so one of them + // read the other's inputs -- and it is why a blanket sweep of the staging directory was + // never safe. See StagingNames. + val listFile = File(output.parentFile, StagingNames.concatListFor(output.name)).apply { writeText(FFmpegConcatCommand.listFileContents(paths)) } diff --git a/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt b/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt index 25113f4..a610a40 100644 --- a/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt +++ b/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt @@ -93,14 +93,13 @@ class JoinViewModel @JvmOverloads constructor( // below, and both run on the main dispatcher, so nothing can interleave. if (_state.value !is JoinState.Idle || activeWorkId != null) return@launch - // Every join stages under the same constant name, so two finished joins always - // report the same file and no tag of either can be trusted to describe it. That is - // what Ambiguous means here, and the count falls back rather than being borrowed — - // which costs nothing in practice, since the count is only rendered while a job is - // live and a live job names no file to be aliased on. What it does not cover is the - // stream-copy-or-re-encode line on Joined, which comes from the picked job's output - // and can therefore belong to the other one. Same cause, same fix: one staging name - // per job. + // Joins used to stage under one constant name, so two finished joins always reported + // the same file and no tag of either could be trusted to describe it. That is what + // Ambiguous means here, and the count falls back rather than being borrowed — which + // costs nothing in practice, since the count is only rendered while a job is live and + // a live job names no file to be aliased on. Staging on the job id has closed that for + // new work, including the stream-copy-or-re-encode line on Joined, which comes from + // the picked job's output; joins already in the queue keep the old shape. val tags = (reattachment as? Reattachment.Certain)?.job?.tags.orEmpty() // Placeholders, and safe only because of where they can go. Joining reads nothing // but the size of this list, Waiting and Joined read none of it, and a reattached diff --git a/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt b/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt index fed999b..6c8d81c 100644 --- a/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt +++ b/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt @@ -12,6 +12,7 @@ import androidx.work.WorkerParameters import androidx.work.workDataOf import kotlinx.coroutines.CancellationException import org.libremediaconverter.convert.ConversionDependencies +import org.libremediaconverter.convert.StagingNames import org.libremediaconverter.ffmpeg.ConcatEngine import org.libremediaconverter.model.OutputFormat @@ -49,7 +50,10 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker // Named before anything below can throw, so every exit has the handle to clean up with. // See the same line in ConversionWorker. - val staged = publisher.createStagingFile("joined.${format.extension}") + // + // Keyed on this job's id. The constant "joined." this replaces meant any two joins of + // the same format wrote one file, and ConcatEngine's list file collided harder still. + val staged = publisher.createStagingFile(StagingNames.forJob(id, format.extension)) return try { // Inside the try: a foreground start refused because the app is in the background -- diff --git a/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt b/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt index 1691f2f..845ea24 100644 --- a/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt +++ b/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt @@ -13,6 +13,7 @@ import androidx.work.workDataOf import com.arthenica.ffmpegkit.FFmpegKitConfig import kotlinx.coroutines.CancellationException import org.libremediaconverter.convert.ConversionDependencies +import org.libremediaconverter.convert.StagingNames import org.libremediaconverter.model.AudioCodec import org.libremediaconverter.model.Container import org.libremediaconverter.model.ContainerCapabilities @@ -73,7 +74,10 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo // This only builds a path -- nothing is written until an engine opens it -- so naming it // early costs nothing, and it is what lets the catch collect a partial an earlier attempt // left behind under the same name. - val staged = publisher.createStagingFile(outputNameFor(displayName, spec)) + // + // Keyed on this job's id rather than on the input's display name: two conversions of files + // that happen to share a name are two jobs, and used to be one file. See StagingNames. + val staged = publisher.createStagingFile(StagingNames.forJob(id, spec.extension)) return try { // Inside the try, and that placement is the whole point. setForeground() throws @@ -280,11 +284,15 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo private const val TAG = "ConversionWorker" /** - * The staged and suggested filename. + * The name to suggest in the save dialog. * * The extension comes from the container and whether a video track survives, so Matroska * yields `.mkv` or `.mka` and MP4 yields `.mp4` or `.m4a` without a preset having to * enumerate both. + * + * No longer the staged name as well. Staging is keyed on the job id -- see [StagingNames] + * -- so this is only ever the string offered to the user, which is also what makes it safe + * for it to carry a display name the app does not control. */ fun outputNameFor(inputName: String, spec: OutputSpec): String = inputName.substringBeforeLast('.', inputName) + "_converted.${spec.extension}" diff --git a/app/src/main/java/org/libremediaconverter/work/Reattachment.kt b/app/src/main/java/org/libremediaconverter/work/Reattachment.kt index 8e249b3..106e3d3 100644 --- a/app/src/main/java/org/libremediaconverter/work/Reattachment.kt +++ b/app/src/main/java/org/libremediaconverter/work/Reattachment.kt @@ -59,15 +59,20 @@ sealed interface Reattachment { * * Observed on a device rather than imagined: a tag query in a fresh process returned two * SUCCEEDED jobs whose output paths were both `…/conversions/input_converted.mp4`, with one - * file on disk. Nothing gives a job a staging path of its own — the name is derived from the - * input's display name — so a later conversion overwrites an earlier one's output while both - * jobs go on reporting that path as their result. + * file on disk. Nothing gave a job a staging path of its own — the name came from the input's + * display name — so a later conversion overwrote an earlier one's output while both jobs went + * on reporting that path as their result. * * The file is not the ambiguous part: whichever entry is picked, the user is offered the * bytes actually on disk, which is the thing that would otherwise be lost. What cannot be * recovered is which job wrote them, so the caller is told not to describe it. A card * labelled with the other job's input would be a confident lie, where a neutral label is - * merely thin. It resolves on its own once each job stages under a name of its own. + * merely thin. + * + * [org.libremediaconverter.convert.StagingNames] has since keyed staging on the job id, so + * nothing enqueued from now on can alias. This stays because the queue outlives the change: + * WorkManager keeps finished work for about a week, and the jobs most likely to be sitting in + * it when this code first runs are exactly the ones named the old way. */ data class Ambiguous(override val job: JobSnapshot) : Reattachment @@ -114,8 +119,8 @@ sealed interface Reattachment { * Live work outranks a finished result because a running job is holding a foreground * notification: someone opening the app while that notification is in the shade expects * to find that conversion, not a result from yesterday. It is also the right answer when - * the running job is overwriting the older one's staged file, which a shared staging name - * allows. + * the running job is overwriting the older one's staged file, which work enqueued before + * per-job staging names can still do. * * Within a rank the **newest staged file** wins, and that is not a detail. Losing a tie * is not the same as waiting for the next launch: the query has no `ORDER BY`, so its diff --git a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt index 28d8d8b..4419919 100644 --- a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt +++ b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt @@ -8,6 +8,7 @@ import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner import org.robolectric.RuntimeEnvironment import java.io.File +import java.util.UUID /** * What a pure function cannot say: the file is really gone. @@ -72,9 +73,15 @@ class OutputPublisherStagingTest { @Test fun `the sweep collects an orphan and leaves a live job alone`() { - val orphan = publisher.createStagingFile("orphan.mp4").apply { writeBytes(ByteArray(4096)) } - val liveOutput = publisher.createStagingFile("joined.mp4").apply { writeBytes(ByteArray(4096)) } - val liveList = publisher.createStagingFile("concat_list.txt").apply { writeText("file 'a.mp4'\n") } + // Named the way the app names them, so the sweep is exercised against real shapes. + val liveJob = StagingNames.forJob(UUID.randomUUID(), "mp4") + val orphan = publisher.createStagingFile( + StagingNames.forJob(UUID.randomUUID(), "mp4"), + ).apply { writeBytes(ByteArray(4096)) } + val liveOutput = publisher.createStagingFile(liveJob).apply { writeBytes(ByteArray(4096)) } + val liveList = publisher.createStagingFile( + StagingNames.concatListFor(liveJob), + ).apply { writeText("file 'a.mp4'\n") } assertTrue(orphan.setLastModified(System.currentTimeMillis() - StagingSweep.GRACE_PERIOD_MS - 60_000)) publisher.sweepStaging() diff --git a/app/src/test/java/org/libremediaconverter/convert/StagingNamesTest.kt b/app/src/test/java/org/libremediaconverter/convert/StagingNamesTest.kt new file mode 100644 index 0000000..f495c08 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/StagingNamesTest.kt @@ -0,0 +1,89 @@ +package org.libremediaconverter.convert + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotEquals +import org.junit.Assert.assertTrue +import org.junit.Test +import org.libremediaconverter.model.OutputFormat +import java.io.File +import java.util.UUID + +/** + * The rule that gives every job a staging path of its own. + * + * A pure function for the same reason as [StagingSweep] and + * [org.libremediaconverter.work.FailureOutcome]: what it prevents cannot be provoked here. The + * collision needs two jobs alive at once, one of them resumed by WorkManager after a process + * restart, which is a device and an `am kill`. What *is* checkable is the property that makes the + * collision impossible, and that is what these pin. + */ +class StagingNamesTest { + + @Test + fun `two jobs converting the same file stage under different names`() { + // The collision, seen on a device: two independent jobs both computed + // cache/conversions/input_converted.mp4, and a tag query in a fresh process returned two + // SUCCEEDED WorkInfos naming that one file. + assertNotEquals( + StagingNames.forJob(JOB_A, MP4.extension), + StagingNames.forJob(JOB_B, MP4.extension), + ) + } + + @Test + fun `the same job stages under the same name on every attempt`() { + // Load-bearing, not incidental. A retry runs doWork() from the top, and the catch on the + // way out deletes the staged file -- which only collects the previous attempt's partial if + // the name is the same. WorkManager builds WorkerParameters from the WorkSpec id and only + // increments runAttemptCount, so the id is what stays still across a retry. + assertEquals( + StagingNames.forJob(JOB_A, MP4.extension), + StagingNames.forJob(JOB_A, MP4.extension), + ) + } + + @Test + fun `the extension is the output's, because that is what infers the muxer`() { + // Not cosmetic. FFmpegConcatCommand names no output muxer, so FFmpeg infers it from the + // path -- an opaque name without the right extension would silently produce the wrong + // container. + assertTrue(StagingNames.forJob(JOB_A, MP4.extension).endsWith(".mp4")) + assertTrue(StagingNames.forJob(JOB_A, OutputFormat.MKV_H265.extension).endsWith(".mkv")) + assertTrue(StagingNames.forJob(JOB_A, OutputFormat.M4A_AAC.extension).endsWith(".m4a")) + } + + @Test + fun `a staging name is a bare filename and nothing else`() { + // The alternative to an opaque name was sanitising the provider-supplied display name, + // which can contain a separator, be empty, or be four kilobytes long. This is what makes + // that whole question moot. + val name = StagingNames.forJob(JOB_A, MP4.extension) + assertEquals("a staging name must not be a path", name, File(name).name) + assertTrue("a staging name must not be empty", name.isNotEmpty()) + } + + @Test + fun `each join gets a list file of its own`() { + // ConcatEngine used a constant, so any two joins at once shared one concat_list.txt and + // one of them read the other's input list. + assertNotEquals( + StagingNames.concatListFor(StagingNames.forJob(JOB_A, MP4.extension)), + StagingNames.concatListFor(StagingNames.forJob(JOB_B, MP4.extension)), + ) + } + + @Test + fun `a list file is named after the output it belongs to`() { + // So the pair is obvious in a directory listing, and so the sweep ages them together. + val output = StagingNames.forJob(JOB_A, MP4.extension) + val list = StagingNames.concatListFor(output) + assertEquals("$JOB_A.concat_list.txt", list) + assertTrue(list.startsWith(output.substringBeforeLast('.'))) + } + + private companion object { + val MP4 = OutputFormat.MP4_H264 + val JOB_A: UUID = UUID.fromString("00000000-0000-4000-8000-00000000000a") + val JOB_B: UUID = UUID.fromString("00000000-0000-4000-8000-00000000000b") + } +} diff --git a/app/src/test/java/org/libremediaconverter/work/DeniedForegroundStartTest.kt b/app/src/test/java/org/libremediaconverter/work/DeniedForegroundStartTest.kt index c2495c4..2308a44 100644 --- a/app/src/test/java/org/libremediaconverter/work/DeniedForegroundStartTest.kt +++ b/app/src/test/java/org/libremediaconverter/work/DeniedForegroundStartTest.kt @@ -15,12 +15,12 @@ import com.google.common.util.concurrent.ListenableFuture import kotlinx.coroutines.runBlocking import org.junit.After import org.junit.Assert.assertEquals -import org.junit.Assert.assertFalse import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import org.libremediaconverter.convert.ConversionDependencies import org.libremediaconverter.convert.OutputPublisher +import org.libremediaconverter.convert.StagingNames import org.libremediaconverter.convert.installTestWorkManager import org.libremediaconverter.model.OutputFormat import org.robolectric.RobolectricTestRunner @@ -92,12 +92,20 @@ class DeniedForegroundStartTest { fun `a denied foreground start collects the partial file the killed attempt left behind`() { // Exactly the 2 MB orphan the device pass found. A process killed mid-transcode leaves a // partial in staging, and the attempt WorkManager schedules to recover it stages under the - // same name; reaching staged.delete() is what collects it. - val staged = stagedFile().apply { writeBytes(ByteArray(PARTIAL_BYTES)) } + // same name -- the job id does not move across a retry -- so reaching staged.delete() is + // what collects it. + stagedFile().writeBytes(ByteArray(PARTIAL_BYTES)) runBlocking { conversionWorker().doWork() } - assertFalse("a denied restart must not orphan the previous attempt's partial", staged.exists()) + // Asserted against the whole directory rather than one path. A path this test computes + // itself can stop matching the one the worker computes, and then the assertion passes by + // asking whether a file nobody wrote is absent. + assertEquals( + "a denied restart must not orphan the previous attempt's partial", + emptyList(), + stagedNames(), + ) } @Test @@ -146,7 +154,9 @@ class DeniedForegroundStartTest { .build() /** The staging path the worker will compute, asked for rather than spelled out here. */ - private fun stagedFile(): File = publisher.createStagingFile(ConversionWorker.outputNameFor(DISPLAY_NAME, SPEC)) + private fun stagedFile(): File = publisher.createStagingFile(StagingNames.forJob(CONVERSION_ID, SPEC.extension)) + + private fun stagedNames(): List = stagedFile().parentFile?.listFiles().orEmpty().map { it.name }.sorted() private companion object { val INPUT: Uri = Uri.parse("content://test/holiday.mp4") diff --git a/app/src/test/java/org/libremediaconverter/work/PerJobStagingTest.kt b/app/src/test/java/org/libremediaconverter/work/PerJobStagingTest.kt new file mode 100644 index 0000000..de6df39 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/work/PerJobStagingTest.kt @@ -0,0 +1,152 @@ +package org.libremediaconverter.work + +import android.app.Application +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import androidx.work.Data +import androidx.work.testing.TestListenableWorkerBuilder +import androidx.work.workDataOf +import kotlinx.coroutines.runBlocking +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.convert.ConversionDependencies +import org.libremediaconverter.convert.OutputPublisher +import org.libremediaconverter.convert.SoftwareTranscoder +import org.libremediaconverter.convert.installTestWorkManager +import org.libremediaconverter.model.ConversionRequest +import org.libremediaconverter.model.DeviceCodecs +import org.libremediaconverter.model.EnginePreference +import org.libremediaconverter.model.InputProbe +import org.libremediaconverter.model.OutputFormat +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.io.File +import java.util.UUID + +/** + * That two jobs cannot write the same staged file. + * + * Observed rather than imagined: two independent conversions on a Pixel each computed + * `cache/conversions/input_converted.mp4`, the second overwrote the first, and a tag query in a + * fresh process then returned two SUCCEEDED `WorkInfo`s naming that one file — which is what + * makes reattachment ambiguous about which job produced what is on disk. + * + * These drive the real worker rather than the naming function, because the naming function was + * never the part that was wrong. What was wrong is which name the worker asked for. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class PerJobStagingTest { + + private lateinit var app: Application + private lateinit var publisher: OutputPublisher + private lateinit var stagingDir: File + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + publisher = AlwaysRoomPublisher(app) + ConversionDependencies.publisher = { publisher } + ConversionDependencies.probe = { _, _ -> InputProbe() } + ConversionDependencies.deviceCodecs = { DeviceCodecs.PERMISSIVE } + ConversionDependencies.software = { WritingTranscoder } + installTestWorkManager(app, Data.EMPTY) + + stagingDir = publisher.createStagingFile("anything").parentFile!! + stagingDir.listFiles()?.forEach { it.delete() } + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + @Test + fun `two conversions of the same file stage under names of their own`() { + runBlocking { conversionWorker(JOB_A).doWork() } + runBlocking { conversionWorker(JOB_B).doWork() } + + // Two jobs, two files. One file here means the second job overwrote the first's output + // while both went on reporting that path as their result. + assertEquals( + "each job must have staged its own file, found ${stagedNames()}", + 2, + stagedNames().size, + ) + } + + @Test + fun `a second attempt at one job reuses the first attempt's staging path`() { + runBlocking { conversionWorker(JOB_A, runAttemptCount = 0).doWork() } + val first = stagedNames() + + runBlocking { conversionWorker(JOB_A, runAttemptCount = 1).doWork() } + + // A per-attempt name would leak one file per retry, and would stop the catch on the way + // out of a failed attempt from collecting the partial the previous one left. + assertEquals("a retry must not stage under a new name", first, stagedNames()) + } + + @Test + fun `a display name that tries to climb out of staging cannot`() { + // Display names come from a document provider and are not this app's to trust: one can + // contain a separator, be empty, or be four kilobytes long. Deriving the staged path from + // it put all of that on a filesystem path. Naming the job instead retires the question + // rather than answering it with a sanitiser. + runBlocking { conversionWorker(JOB_A, displayName = "../escape.mp4").doWork() } + + assertEquals("the output belongs in staging", 1, stagedNames().size) + assertFalse( + "nothing may be written outside the staging directory", + File(app.cacheDir, "escape_converted.mp4").exists(), + ) + } + + private fun stagedNames(): List = stagingDir.listFiles().orEmpty().map { it.name }.sorted() + + private fun conversionWorker( + id: UUID, + runAttemptCount: Int = 0, + displayName: String = DISPLAY_NAME, + ): ConversionWorker = TestListenableWorkerBuilder( + context = app, + inputData = workDataOf( + ConversionWorker.KEY_INPUT_URI to INPUT.toString(), + ConversionWorker.KEY_DISPLAY_NAME to displayName, + ConversionWorker.KEY_SIZE_BYTES to INPUT_BYTES, + ConversionWorker.KEY_CONTAINER to SPEC.container.name, + ConversionWorker.KEY_VIDEO_CODEC to SPEC.videoCodec.name, + ConversionWorker.KEY_AUDIO_CODEC to SPEC.audioCodec.name, + ConversionWorker.KEY_ENGINE_PREFERENCE to EnginePreference.FORCE_SOFTWARE.name, + ), + runAttemptCount = runAttemptCount, + ).setId(id).build() + + private companion object { + val INPUT: Uri = Uri.parse("file:///tmp/input.mp4") + const val DISPLAY_NAME = "input.mp4" + const val INPUT_BYTES = 1024L + val SPEC = OutputFormat.MP4_H265.spec + val JOB_A: UUID = UUID.fromString("00000000-0000-4000-8000-00000000000a") + val JOB_B: UUID = UUID.fromString("00000000-0000-4000-8000-00000000000b") + } +} + +/** An engine that only writes the file, which is the whole of what these tests look at. */ +private object WritingTranscoder : SoftwareTranscoder { + override suspend fun run( + request: ConversionRequest, + inputPath: String, + output: File, + durationMs: Long, + onProgress: (Int) -> Unit, + ) { + output.writeBytes(ByteArray(OUTPUT_BYTES)) + } + + private const val OUTPUT_BYTES = 512 +} diff --git a/app/src/test/java/org/libremediaconverter/work/WorkerCancellationTest.kt b/app/src/test/java/org/libremediaconverter/work/WorkerCancellationTest.kt index e314dc3..a04f345 100644 --- a/app/src/test/java/org/libremediaconverter/work/WorkerCancellationTest.kt +++ b/app/src/test/java/org/libremediaconverter/work/WorkerCancellationTest.kt @@ -11,7 +11,6 @@ import kotlinx.coroutines.CancellationException import kotlinx.coroutines.runBlocking import org.junit.After import org.junit.Assert.assertEquals -import org.junit.Assert.assertFalse import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test @@ -86,9 +85,10 @@ class WorkerCancellationTest { runCatching { runBlocking { worker.doWork() } } - // The engine stub writes before it throws, so this file really existed. Rethrowing without - // deleting would trade one defect for another. - assertFalse("a cancelled attempt must not leave its partial behind", stagedFile().exists()) + // The engine stub writes before it throws, so a file really existed. Asserted against the + // whole directory rather than one path, so a name this test computes drifting from the + // worker's cannot turn it into a question about a file nobody wrote. + assertEquals("a cancelled attempt must not leave its partial behind", emptyList(), stagedNames()) } @Test @@ -105,7 +105,7 @@ class WorkerCancellationTest { ), result, ) - assertFalse("a failed attempt must not leave its partial behind", stagedFile().exists()) + assertEquals("a failed attempt must not leave its partial behind", emptyList(), stagedNames()) } /** @@ -134,8 +134,8 @@ class WorkerCancellationTest { ).setId(JOB_ID).build() } - /** The staging path the worker will compute, asked for rather than spelled out here. */ - private fun stagedFile(): File = publisher.createStagingFile(ConversionWorker.outputNameFor(DISPLAY_NAME, SPEC)) + private fun stagedNames(): List = + publisher.createStagingFile("anything").parentFile?.listFiles().orEmpty().map { it.name }.sorted() private companion object { val INPUT: Uri = Uri.parse("file:///tmp/holiday.mp4")