diff --git a/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt b/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt index 3cc9352..de587e3 100644 --- a/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt +++ b/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt @@ -400,8 +400,21 @@ class ConversionViewModel @JvmOverloads constructor( activeWorkId?.let(workManager::cancelWorkById) } + /** + * Copies the staged result out to the destination the user picked. + * + * The existence check is not redundant with the one reattachment already made. That one ran + * inside a tag query which, for a result offered on launch, can be hours older than the tap — + * and `cacheDir` is exactly the directory the OS empties when it wants space, which is also + * what the sweep does to anything a day old. Without it the file's absence arrived as + * `staged.inputStream()` throwing, and `e.message` put a raw ENOENT path on screen. + */ fun save(destination: Uri) { val converted = _state.value as? ConversionState.Converted ?: return + if (!converted.staged.isFile) { + _state.value = ConversionState.Failed(STAGED_FILE_GONE_MESSAGE) + return + } viewModelScope.launch { runCatching { withContext(Dispatchers.IO) { diff --git a/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt b/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt index ebb8355..504510b 100644 --- a/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt +++ b/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt @@ -6,6 +6,23 @@ import android.provider.DocumentsContract import android.provider.OpenableColumns import java.io.File +/** + * What a save has to say when the staged file is not there any more. + * + * Reachable without anything going wrong: staging lives in `cacheDir`, which the OS reclaims + * whenever it wants the space, and [sweepStaging] collects anything a day old. A result offered by + * reattachment is the likeliest to meet it — the check that decided the file existed ran during a + * tag query that can be hours old by the time the Save button is tapped. + * + * A written sentence rather than the exception's message, which is what used to reach the screen: + * `/data/user/0/org.libremediaconverter/cache/conversions/4b4882….mp4: open failed: ENOENT (No such + * file or directory)` is a true statement about a path the user has never seen and cannot act on. + * Kept next to [OutputPublisher] because both ViewModels need it and staging is what it is about. + */ +const val STAGED_FILE_GONE_MESSAGE: String = + "The finished file is no longer in the cache, so there is nothing left to save. " + + "Start over to make it again." + /** * Staging and publication of conversion output. * diff --git a/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt b/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt index 7b0dc7b..849adeb 100644 --- a/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt +++ b/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt @@ -18,6 +18,7 @@ import kotlinx.coroutines.withContext import org.libremediaconverter.convert.ConversionDependencies import org.libremediaconverter.convert.InputFile import org.libremediaconverter.convert.InputQuery +import org.libremediaconverter.convert.STAGED_FILE_GONE_MESSAGE import org.libremediaconverter.model.ConcatStrategy import org.libremediaconverter.work.ConcatWorker import org.libremediaconverter.work.JobTags @@ -221,8 +222,20 @@ class JoinViewModel @JvmOverloads constructor( activeWorkId?.let(workManager::cancelWorkById) } + /** + * Copies the staged result out to the destination the user picked. + * + * The existence check is the same one `ConversionViewModel.save` makes, for the same reason: a + * join offered by reattachment was last seen during a tag query that may be hours old, and + * `cacheDir` is reclaimed by the OS and swept by this app. Without it the file's absence + * reached the screen as a raw ENOENT path. + */ fun save(destination: Uri) { val joined = _state.value as? JoinState.Joined ?: return + if (!joined.staged.isFile) { + _state.value = JoinState.Failed(STAGED_FILE_GONE_MESSAGE) + return + } viewModelScope.launch { runCatching { withContext(Dispatchers.IO) { diff --git a/app/src/main/java/org/libremediaconverter/work/Reattachment.kt b/app/src/main/java/org/libremediaconverter/work/Reattachment.kt index 106e3d3..3316012 100644 --- a/app/src/main/java/org/libremediaconverter/work/Reattachment.kt +++ b/app/src/main/java/org/libremediaconverter/work/Reattachment.kt @@ -104,10 +104,13 @@ sealed interface Reattachment { * result still worth offering from one already dealt with, which is why that check * carries the weight here. * - * It is also the seam for a neighbouring defect: a result the user dismissed with "Start - * over" currently keeps its staged file, so today it can be offered again on the next - * launch. Nothing here changes when that is fixed — the file stops existing and the job - * stops qualifying. + * It is also the seam a neighbouring fix acts through. "Start over" deletes the staged + * file, so a result the user dismissed stops qualifying here without this rule needing to + * know that happened — the file stops existing and the job falls out. What survives is the + * narrower gap that delete cannot close: `reset()` dispatches it to + * [kotlinx.coroutines.Dispatchers.IO] and it is cancelled with the Activity, so a + * dismissal on the way out of the app can leave the file behind. That is what + * `OutputPublisher.sweepStaging` is for, and its own KDoc names this case. * * Ranked, when more than one qualifies: * diff --git a/app/src/test/java/org/libremediaconverter/convert/MissingStagedFileTest.kt b/app/src/test/java/org/libremediaconverter/convert/MissingStagedFileTest.kt new file mode 100644 index 0000000..1692b00 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/MissingStagedFileTest.kt @@ -0,0 +1,107 @@ +package org.libremediaconverter.convert + +import android.app.Application +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import androidx.work.workDataOf +import kotlinx.coroutines.Dispatchers +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.join.JoinState +import org.libremediaconverter.join.JoinViewModel +import org.libremediaconverter.model.InputProbe +import org.libremediaconverter.work.ConcatWorker +import org.libremediaconverter.work.ConversionWorker +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import org.robolectric.Shadows.shadowOf +import java.io.ByteArrayOutputStream +import java.io.File + +/** + * What the user is told when the file went away between being offered and being saved. + * + * Not a corner: staging is `cacheDir`, which is what the OS empties when it wants space, and the + * sweep collects anything a day old. Reattachment is where the two are furthest apart — the check + * that decided the file existed ran inside a tag query on launch, and the Save button may not be + * tapped for hours. + * + * The real [OutputPublisher] rather than the recording stub, because the defect is what the *real* + * publish does with a staged file that is not there: `staged.inputStream()` throws, and `save()` + * put `e.message` on screen — a `/data/user/0/…/4b4882….mp4: open failed: ENOENT` path the user has + * never seen and can do nothing with. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class MissingStagedFileTest { + + private lateinit var app: Application + private lateinit var publisher: OutputPublisher + private lateinit var staged: File + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + publisher = OutputPublisher(app) + ConversionDependencies.publisher = { publisher } + ConversionDependencies.probe = { _, _ -> InputProbe() } + + staged = publisher.createStagingFile("holiday_converted.mp4").apply { writeBytes(ByteArray(4096)) } + installTestWorkManager( + app, + workDataOf( + ConversionWorker.KEY_OUTPUT_PATH to staged.absolutePath, + ConcatWorker.KEY_OUTPUT_PATH to staged.absolutePath, + ), + ) + // A destination that really opens, so the save gets far enough to reach the staged file. + // Without this the failure would be about the destination and the test would pass while + // saying nothing. + shadowOf(app.contentResolver).registerOutputStreamSupplier(DESTINATION) { ByteArrayOutputStream() } + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + @Test + fun `saving a conversion whose staged file has gone says so in a sentence`() { + val viewModel = ConversionViewModel(app, Dispatchers.Unconfined) + viewModel.onInputPicked(Uri.parse("content://test/holiday.mp4")) + awaitState(viewModel.state, "Ready") { it is ConversionState.Ready } + viewModel.convert() + awaitState(viewModel.state, "Converted") { it is ConversionState.Converted } + + assertTrue("the fixture must start with a real staged file", staged.delete()) + + viewModel.save(DESTINATION) + + val failed = awaitState(viewModel.state, "Failed") { it is ConversionState.Failed } + assertEquals(STAGED_FILE_GONE_MESSAGE, (failed as ConversionState.Failed).message) + } + + @Test + fun `saving a join whose staged file has gone says so in a sentence`() { + val viewModel = JoinViewModel(app, Dispatchers.Unconfined) + viewModel.onInputsPicked(listOf(Uri.parse("content://test/a.mp4"), Uri.parse("content://test/b.mp4"))) + awaitState(viewModel.state, "Ready") { it is JoinState.Ready } + viewModel.join() + awaitState(viewModel.state, "Joined") { it is JoinState.Joined } + + assertTrue("the fixture must start with a real staged file", staged.delete()) + + viewModel.save(DESTINATION) + + val failed = awaitState(viewModel.state, "Failed") { it is JoinState.Failed } + assertEquals(STAGED_FILE_GONE_MESSAGE, (failed as JoinState.Failed).message) + } + + private companion object { + val DESTINATION: Uri = Uri.parse("content://test/destination.mp4") + } +} diff --git a/app/src/test/java/org/libremediaconverter/convert/ReattachGuardsTest.kt b/app/src/test/java/org/libremediaconverter/convert/ReattachGuardsTest.kt new file mode 100644 index 0000000..68dc507 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/ReattachGuardsTest.kt @@ -0,0 +1,214 @@ +package org.libremediaconverter.convert + +import android.app.Application +import android.net.Uri +import android.os.Looper +import android.util.Log +import androidx.media3.common.util.UnstableApi +import androidx.work.Configuration +import androidx.work.WorkManager +import androidx.work.testing.SynchronousExecutor +import androidx.work.testing.WorkManagerTestInitHelper +import androidx.work.workDataOf +import kotlinx.coroutines.Dispatchers +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.model.InputProbe +import org.libremediaconverter.work.ConversionWorker +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import org.robolectric.Shadows.shadowOf +import java.io.File +import java.util.concurrent.CountDownLatch +import java.util.concurrent.Executor +import java.util.concurrent.TimeUnit + +/** + * The two decisions `reattach()` makes that [org.libremediaconverter.work.Reattachment] cannot. + * + * `Reattachment.choose` answers "which job", and twenty tests pin it. What it does not decide is + * whether the answer may still be used by the time it arrives, or how much of it the card is + * allowed to believe — and both of those live in the ViewModel, where nothing was asserting them. + * Deleting either guard left the whole suite green. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class ReattachGuardsTest { + + private lateinit var app: Application + private lateinit var publisher: RecordingPublisher + private lateinit var workManager: WorkManager + private lateinit var staged: File + private lateinit var queries: HoldableTaskExecutor + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + publisher = RecordingPublisher(app) + ConversionDependencies.publisher = { publisher } + ConversionDependencies.probe = { _, _ -> InputProbe() } + + staged = publisher.createStagingFile("holiday_converted.mp4").apply { writeBytes(ByteArray(4096)) } + queries = HoldableTaskExecutor() + WorkManagerTestInitHelper.initializeTestWorkManager( + app, + Configuration.Builder() + .setMinimumLoggingLevel(Log.ASSERT) + .setExecutor(SynchronousExecutor()) + .setTaskExecutor(queries) + .setWorkerFactory( + SucceedingWorkerFactory(workDataOf(ConversionWorker.KEY_OUTPUT_PATH to staged.absolutePath)), + ) + .build(), + ) + workManager = WorkManager.getInstance(app) + } + + @After + fun tearDown() { + queries.release() + ConversionDependencies.reset() + } + + /** + * The race the guard exists for: the tag query suspends, and while it is away the user picks a + * file of their own. Reattaching over that would throw away what they just did — and, worse, + * point the Save button at yesterday's file while the card named today's. + * + * Made deterministic by holding WorkManager's task executor rather than by hoping the pick wins: + * the query cannot complete until this test lets it, so the pick has landed before the guard is + * ever reached. + */ + @Test + fun `a file picked while the query was in flight is not reattached over`() { + finishAConversionWithNobodyWatching() + val picked = File(app.cacheDir, "beach.mp4").apply { writeBytes(ByteArray(2048)) } + + queries.hold() + val viewModel = ConversionViewModel(app, Dispatchers.Unconfined) + viewModel.onInputPicked(Uri.fromFile(picked)) + val ready = awaitState(viewModel.state, "Ready") { it is ConversionState.Ready } + // The URI, because that is what tells the two inputs apart: a reattached job's is + // Uri.EMPTY -- WorkManager never hands back the Data a request was enqueued with -- while a + // picked file's is the one the picker returned. + assertEquals(Uri.fromFile(picked), (ready as ConversionState.Ready).input.uri) + + queries.release() + settle() + + // The query really did run and really did reach the guard -- without this the assertion + // below would pass just as well against a reattachment that never arrived. + assertTrue("the reattach query should have been held, then run", queries.heldTasks > 0) + val current = viewModel.state.value + assertTrue("the user's pick must survive a late reattachment, got $current", current is ConversionState.Ready) + assertEquals(Uri.fromFile(picked), (current as ConversionState.Ready).input.uri) + assertEquals(2_048L, current.input.sizeBytes) + } + + /** + * Two finished jobs naming one staged file, which is exactly what the device produced before + * staging was keyed on the job id. + * + * The file is the user's either way, so it is still offered. Which job wrote it is not + * knowable, so the card must not borrow either job's input name: a card labelled with the other + * conversion's file is a confident lie, where a neutral label is merely thin. + */ + @Test + fun `a result two jobs both claim is offered without being attributed to either`() { + finishAConversionWithNobodyWatching(displayName = "holiday.mp4") + finishAConversionWithNobodyWatching(displayName = "beach.mp4") + + val viewModel = ConversionViewModel(app, Dispatchers.Unconfined) + val converted = awaitState(viewModel.state, "Converted") { it is ConversionState.Converted } + + converted as ConversionState.Converted + assertEquals( + "the bytes on disk are what the user gets back", + staged.absolutePath, + converted.staged.absolutePath, + ) + assertEquals( + "neither job's name may be claimed for the other's file", + "Media file", + converted.input.displayName, + ) + // The size travels in the same tags as the name, so it goes the same way rather than being + // reported as one job's number against the other job's file. + assertEquals(null, converted.input.sizeBytes) + } + + /** Pumps the main looper for long enough that anything already dispatched has run. */ + private fun settle() { + repeat(SETTLE_PUMPS) { + shadowOf(Looper.getMainLooper()).idle() + Thread.sleep(SETTLE_INTERVAL_MS) + } + } + + private fun finishAConversionWithNobodyWatching(displayName: String = "holiday.mp4") { + workManager.enqueue( + ConversionWorker.request( + inputUri = Uri.parse("content://test/$displayName"), + displayName = displayName, + sizeBytes = 4_096L, + ), + ).result.get() + } + + private companion object { + const val SETTLE_PUMPS = 60 + const val SETTLE_INTERVAL_MS = 5L + } +} + +/** + * WorkManager's task executor, with a brake the test can apply. + * + * The reattachment query is a suspending call the ViewModel makes in `init`, so a test that wants + * to act "while it is in flight" has to be able to stop it finishing. Holding the executor it runs + * on is the only seam for that: `jobSnapshots` takes no dispatcher, and racing it would make the + * assertion depend on which of two IO hops happened to return first. + * + * Never applied on the main thread. The test releases the brake from there, so a wait taken on that + * thread would deadlock the loop that was going to end it. The wait is bounded for the same class of + * reason: a wiring mistake should turn the test red, not hang the build. + */ +private class HoldableTaskExecutor : Executor { + + private val released = CountDownLatch(1) + + @Volatile + private var holding = false + + /** How many tasks were actually held. Zero means the brake never gripped anything. */ + @Volatile + var heldTasks = 0 + private set + + fun hold() { + holding = true + } + + fun release() { + holding = false + released.countDown() + } + + override fun execute(command: Runnable) { + if (holding && Looper.myLooper() != Looper.getMainLooper()) { + heldTasks++ + check(released.await(HOLD_TIMEOUT_SECONDS, TimeUnit.SECONDS)) { + "a held WorkManager task was never released" + } + } + command.run() + } + + private companion object { + const val HOLD_TIMEOUT_SECONDS = 10L + } +} diff --git a/app/src/test/java/org/libremediaconverter/work/JobSnapshotsTest.kt b/app/src/test/java/org/libremediaconverter/work/JobSnapshotsTest.kt new file mode 100644 index 0000000..4af2fda --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/work/JobSnapshotsTest.kt @@ -0,0 +1,183 @@ +package org.libremediaconverter.work + +import android.app.Application +import android.content.Context +import android.util.Log +import androidx.media3.common.util.UnstableApi +import androidx.work.Configuration +import androidx.work.ListenableWorker +import androidx.work.OneTimeWorkRequestBuilder +import androidx.work.WorkManager +import androidx.work.Worker +import androidx.work.WorkerFactory +import androidx.work.WorkerParameters +import androidx.work.testing.SynchronousExecutor +import androidx.work.testing.WorkManagerTestInitHelper +import androidx.work.workDataOf +import kotlinx.coroutines.runBlocking +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.io.File + +/** + * The edge that feeds the reattachment decision. + * + * [Reattachment.choose] is a pure function with twenty tests, and every input it reasons over is + * computed here — by the one part of reattachment that has to touch WorkManager and the + * filesystem. That asymmetry was the gap: the rule was pinned exhaustively while the values it + * ran on were pinned nowhere, so a regression in this file left the whole suite green. Two + * demonstrated ones: dropping the empty-file filter offered a zero-byte staged file as a savable + * result, and hardcoding [JobSnapshot.outputModifiedAt] to zero starved the newest-file tie-break + * of the only data it has. + * + * A real `WorkManager` and a real `cacheDir`, because both are what the code under test is for. + * The worker never runs: [EchoingWorkerFactory] stands in for a job that finished in a process + * that no longer exists, which is the only way a snapshot with an output path comes to exist at + * all. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class JobSnapshotsTest { + + private lateinit var app: Application + private lateinit var workManager: WorkManager + private lateinit var stagingDir: File + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + WorkManagerTestInitHelper.initializeTestWorkManager( + app, + Configuration.Builder() + .setMinimumLoggingLevel(Log.ASSERT) + .setExecutor(SynchronousExecutor()) + .setTaskExecutor(SynchronousExecutor()) + .setWorkerFactory(EchoingWorkerFactory) + .build(), + ) + workManager = WorkManager.getInstance(app) + stagingDir = File(app.cacheDir, "conversions").apply { mkdirs() } + stagingDir.listFiles()?.forEach { it.delete() } + } + + @Test + fun `a staged file with nothing in it is not an output`() { + // Zero bytes is what a job killed before its engine wrote anything leaves behind. Treating + // it as a result would publish it: the user taps Save and gets a zero-byte "conversion" + // rather than a message, which is worse than not being offered it. + val empty = stagedFile("empty.mp4", bytes = 0) + val real = stagedFile("real.mp4", bytes = 4096) + // Never created at all -- the OS reclaimed the cache, or the file was saved and deleted. + val reclaimed = File(stagingDir, "reclaimed.mp4") + listOf(empty, real, reclaimed).forEach(::finishedWithOutput) + + val snapshots = snapshots() + + assertEquals( + "only a file with bytes in it is a result", + mapOf( + empty.absolutePath to false, + real.absolutePath to true, + reclaimed.absolutePath to false, + ), + snapshots.associate { it.outputPath to it.outputExists }, + ) + // The path is still reported for all three. It is what the worker said; whether it still + // names anything is the separate question above. + assertEquals( + setOf(empty.absolutePath, real.absolutePath, reclaimed.absolutePath), + snapshots.mapNotNull { it.outputPath }.toSet(), + ) + // And a file that is not an output has no time either: an mtime read off a zero-byte + // leftover would feed the tie-break a moment nothing produced. + assertEquals(0L, snapshotFor(snapshots, empty).outputModifiedAt) + } + + @Test + fun `each result carries the time its own file was last written`() { + val older = stagedFile("older.mp4", bytes = 4096) + val newer = stagedFile("newer.mp4", bytes = 4096) + // Set explicitly rather than relying on the order the two were written: a filesystem is + // free to give both the same mtime, and then the fixture would be testing nothing. + assertTrue(older.setLastModified(OLDER_MS)) + assertTrue(newer.setLastModified(NEWER_MS)) + assertTrue( + "the two fixtures must really carry different times, got ${older.lastModified()}", + older.lastModified() < newer.lastModified(), + ) + listOf(older, newer).forEach(::finishedWithOutput) + + val snapshots = snapshots() + + // Compared against what the filesystem stored rather than against what was requested, + // because mtime granularity is the filesystem's business and not this test's claim. + assertEquals(older.lastModified(), snapshotFor(snapshots, older).outputModifiedAt) + assertEquals(newer.lastModified(), snapshotFor(snapshots, newer).outputModifiedAt) + + // Why the field exists, asserted through the rule that reads it: the tag query has no + // ORDER BY, so without a real time here an arbitrary winner would win every launch while + // the other result stayed unreachable for as long as its file existed. + assertEquals(newer.absolutePath, Reattachment.choose(snapshots)?.job?.outputPath) + } + + private fun snapshots(): List = runBlocking { + workManager.jobSnapshots( + tag = ConversionWorker::class.java.name, + outputPathKey = ConversionWorker.KEY_OUTPUT_PATH, + ) + } + + private fun snapshotFor(snapshots: List, output: File): JobSnapshot = + snapshots.single { it.outputPath == output.absolutePath } + + private fun stagedFile(name: String, bytes: Int): File = + File(stagingDir, name).apply { writeBytes(ByteArray(bytes)) } + + /** + * A conversion that finished with [output] as its result and nobody watching. + * + * Built rather than taken from `ConversionWorker.request`, because what has to reach + * `jobSnapshots` is the *output* `Data` of a finished job, and a request only carries input. + */ + private fun finishedWithOutput(output: File) { + workManager.enqueue( + OneTimeWorkRequestBuilder() + .setInputData(workDataOf(ConversionWorker.KEY_OUTPUT_PATH to output.absolutePath)) + .build(), + ).result.get() + } + + private companion object { + /** Two fixed moments a day apart, so the ordering is stated rather than raced for. */ + const val OLDER_MS = 1_700_000_000_000L + const val NEWER_MS = OLDER_MS + 24L * 60 * 60 * 1000 + } +} + +/** + * Stands in for whichever job finished before this process existed, reporting the output path it + * was handed. + * + * The real [ConversionWorker] cannot run here — it drives Media3 and FFmpeg through native + * libraries that do not exist on the JVM — and what `jobSnapshots` needs from it is only a + * SUCCEEDED `WorkInfo` carrying an output path. Echoing the input means one factory can produce + * several jobs with results of their own, which is what the ordering and aliasing cases need. + */ +private object EchoingWorkerFactory : WorkerFactory() { + override fun createWorker( + appContext: Context, + workerClassName: String, + workerParameters: WorkerParameters, + ): ListenableWorker = object : Worker(appContext, workerParameters) { + override fun doWork(): Result { + val path = inputData.getString(ConversionWorker.KEY_OUTPUT_PATH) + ?: return Result.success() + return Result.success(workDataOf(ConversionWorker.KEY_OUTPUT_PATH to path)) + } + } +} diff --git a/app/src/test/java/org/libremediaconverter/work/ReattachmentTest.kt b/app/src/test/java/org/libremediaconverter/work/ReattachmentTest.kt index 42725ff..43fcf5b 100644 --- a/app/src/test/java/org/libremediaconverter/work/ReattachmentTest.kt +++ b/app/src/test/java/org/libremediaconverter/work/ReattachmentTest.kt @@ -35,6 +35,16 @@ class ReattachmentTest { assertNull(Reattachment.choose(listOf(job(state = WorkInfo.State.FAILED)))) } + @Test + fun `a failed job is not reattached to even when it left a file behind`() { + // The fixture that matters, and the one every other FAILED case here was missing: a job + // killed mid-write leaves a partial in staging -- the 2 MB orphan the device pass found -- + // so the exclusion has to hold for a FAILED job that really does name a file on disk. + // Ranking it like a result would offer the user a truncated file with a Save button. + val partial = job(state = WorkInfo.State.FAILED, outputPath = STAGED, outputExists = true) + assertNull(Reattachment.choose(listOf(partial))) + } + @Test fun `a finished result still on disk is offered`() { val result = job(state = WorkInfo.State.SUCCEEDED, outputPath = "/cache/out.mp4", outputExists = true)