From c5c4c5323bde1abfed61e117bb1b90ae4cab4cf5 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sat, 22 Aug 2026 23:06:37 -0500 Subject: [PATCH] Test the edge that feeds reattachment, and stop it reporting ENOENT Reattachment.choose has twenty tests and every mutation aimed at it bites. Everything that computes its inputs had none, and five mutations there passed the whole 257-test suite. Four are closed here, each verified by applying the mutation and watching the new test go red. jobSnapshots() is the half that has to touch WorkManager and the filesystem, so it is where the untested values live. JobSnapshotsTest drives it against a real WorkManager and a real cacheDir: - A zero-byte staged file is not an output. Relaxing the filter to `exists()` -- which is what a job killed before its engine wrote anything leaves behind -- made the snapshot claim a result, and the user would meet a Save button for a zero-byte "conversion". The same case pins that the path is still reported and that the mtime stays 0 for a file that is not a result. - Each result carries its own file's mtime. Hardcoding it to zero starves the newest-file tie-break of the only data it has, which is precisely the failure the tie-break exists to prevent: the query has no ORDER BY, so an arbitrary winner keeps winning every launch. Timestamps are set with setLastModified and compared against what the filesystem stored, because mtime granularity is not this test's claim to make. ReattachGuardsTest covers the two decisions the ViewModel makes that the pure rule cannot: - A file picked while the query was still in flight is not reattached over. Deleting the guard turns the user's pick into yesterday's job -- with the Save button pointing at a file the card does not name. Made deterministic by holding WorkManager's task executor rather than by racing two IO hops: the query cannot finish until the pick has landed. The test also asserts the brake really gripped, so a reattachment that never arrived cannot pass for one that was refused. - An Ambiguous result is offered without being attributed. Two finished jobs naming one staged file is what the device produced before staging was keyed on the job id; taking the first job's tags labels the file with the other conversion's name, which is the confident lie the KDoc rejects. The neutral label and the absent size are both pinned. ReattachmentTest's FAILED exclusion was only ever tested with pathless FAILED jobs, so a narrow regression ranking a FAILED job that carries a file like a result passed all 257 tests. The live shape is the 2 MB orphan the device pass found: a job killed mid-write leaves a partial, and under that regression the user is offered a truncated file with a Save button. One fixture with outputPath and outputExists set closes it. save() re-checks the staged file, in both ViewModels. The check reattachment made ran inside a tag query that can be hours older than the tap, and cacheDir is what the OS empties when it wants space and what the sweep collects after a day. The file's absence used to arrive as staged.inputStream() throwing, and e.message put "/data/user/0/.../4b4882....mp4: open failed: ENOENT" on screen -- a true statement about a path the user has never seen and cannot act on. It now reads as a sentence with an action in it. The message is one constant next to OutputPublisher because both ViewModels need it and staging is what it is about. Reattachment's KDoc claimed a defect that was fixed in the commit before it -- that "Start over" keeps its staged file -- which would send a maintainer to re-fix D2. Rewritten to say what is actually true: the delete happens, and the gap it leaves is the reset() whose delete is cancelled with the Activity, which is the sweep's job and is named in the sweep's own KDoc. R1 / #10, R2 / #11, R24 / #33, R25 / #34 Co-Authored-By: Claude Opus 5 (1M context) --- .../convert/ConversionViewModel.kt | 13 ++ .../convert/OutputPublisher.kt | 17 ++ .../libremediaconverter/join/JoinViewModel.kt | 13 ++ .../libremediaconverter/work/Reattachment.kt | 11 +- .../convert/MissingStagedFileTest.kt | 107 +++++++++ .../convert/ReattachGuardsTest.kt | 214 ++++++++++++++++++ .../work/JobSnapshotsTest.kt | 183 +++++++++++++++ .../work/ReattachmentTest.kt | 10 + 8 files changed, 564 insertions(+), 4 deletions(-) create mode 100644 app/src/test/java/org/libremediaconverter/convert/MissingStagedFileTest.kt create mode 100644 app/src/test/java/org/libremediaconverter/convert/ReattachGuardsTest.kt create mode 100644 app/src/test/java/org/libremediaconverter/work/JobSnapshotsTest.kt 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)