diff --git a/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt b/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt index 6519fad..b8344fe 100644 --- a/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt +++ b/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt @@ -193,6 +193,25 @@ class ConversionViewModel @JvmOverloads constructor( private var observer: Job? = null private var activeWorkId: UUID? = null + /** + * Who is allowed to write to this screen — see [ScreenOwnership] for the rule and why + * cancelling the superseded coroutine is not one. + * + * Every write below that lands after a suspension point is guarded by it: the two in + * [onInputPicked] and the one in [observe]. + * + * [save] is the one left out, deliberately — and not because it is safe in both directions. + * Nothing can overwrite what it writes: it is reachable only from [ConversionState.Converted] + * or a [ConversionState.Failed] carrying its file, so the only observation that could belongs + * to a job already in a terminal state, which will not emit again. What it can still do is + * land on top of a [reset] taken while its copy was in flight, putting `Saved` on a screen the + * user has just cleared. Guarding it would drop that write instead, reporting nothing for a + * file that may genuinely have reached the user's destination. Which of those two is right is + * a question about what the screen should offer during a save, not about this race, so it is + * filed as issue #123 rather than decided here in passing. + */ + private val ownership = ScreenOwnership() + /** * The staged output this ViewModel is responsible for deleting. * @@ -244,6 +263,10 @@ class ConversionViewModel @JvmOverloads constructor( * `Data` — see [ConversionState.Converted]. */ private fun reattach() { + // Read before the launch, and before the query it is about to suspend in. This is the + // claim the answer will belong to: anything the user does from here on supersedes it, and + // reading it on the far side of the query would read whatever superseded it instead. + val token = ownership.current viewModelScope.launch { val reattachment = Reattachment.choose( workManager.jobSnapshots( @@ -254,8 +277,17 @@ class ConversionViewModel @JvmOverloads constructor( // The query suspends, so by now the user may have picked a file or started a // conversion of their own. Either owns the screen; reattaching over it would throw - // away what they just did. Both this check and the assignment below run on the main - // dispatcher with no suspension point between them, so nothing can interleave. + // away what they just did. + // + // This catches a pick that has already *landed*, and only that. It used to claim that + // "both this check and the assignment below run on the main dispatcher with no + // suspension point between them, so nothing can interleave" — which was the exact + // opposite of what happens. There is no assignment below. There is observe(), which + // launches a *separate* coroutine that must suspend on `collect` before it can write + // anything, so the check happens at one moment and the write lands at another with a + // whole pick able to fit in between. That was issue #49, and believing this comment is + // why it read as flaky CI for two days. What actually holds the line is the token + // observe() carries: see [ScreenOwnership]. 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. @@ -281,7 +313,7 @@ class ConversionViewModel @JvmOverloads constructor( activeWorkId = reattachment.job.id // No initial state of our own: the flow's first emission carries the job's real // state, so observe() maps it exactly as it would for a conversion started here. - observe(reattachment.job.id, input, cancelled = ConversionState.Idle) + observe(reattachment.job.id, input, cancelled = ConversionState.Idle, token = token) } } @@ -297,25 +329,34 @@ class ConversionViewModel @JvmOverloads constructor( fun setQuality(quality: QualityTier) = _settings.update { it.copy(quality = quality) } fun setEnginePreference(preference: EnginePreference) = _settings.update { it.copy(enginePreference = preference) } + /** + * The tap is the claim, which is why [ScreenOwnership.claim] is called here and not inside the + * `launch`. A claim made in the coroutine would only be immediate for as long as + * `Dispatchers.Main.immediate` happened to run it inline, and a deferred claim leaves the same + * gap this closes: it is the difference between the user owning the screen from the moment + * they tapped and owning it from whenever their coroutine got around to running. + */ fun onInputPicked(uri: Uri) { + val token = ownership.claim() viewModelScope.launch { // Both the metadata query and the probe touch disk, and the probe spawns FFprobe. // Neither belongs on the main thread. val file = withContext(pickDispatcher) { InputQuery.describe(getApplication(), uri) } + // Every write below the hop above is guarded, this one included: two picks in quick + // succession suspend here together, and without this the slower one would land last + // and put the file the user did not choose on screen. + if (!ownership.stillHeldBy(token)) return@launch // Show the file as soon as its name and size are known. Probing now runs FFprobe on // every pick, which is a native process spawn, and making the whole screen wait on it // would read as the app having ignored the tap. _state.value = ConversionState.Ready(file) val probe = withContext(pickDispatcher) { probeOrUnreadable(uri) } - // Only fill in the probe if the user has not moved on in the meantime. - _state.update { current -> - if (current is ConversionState.Ready && current.input.uri == uri) { - ConversionState.Ready(file.copy(probe = probe)) - } else { - current - } - } + // Only fill in the probe if the user has not moved on in the meantime. The claim is + // what says whether they have -- it covers a second pick of the same URI, which a + // comparison of URIs cannot, and every state a later claim could have written. + if (!ownership.stillHeldBy(token)) return@launch + _state.value = ConversionState.Ready(file.copy(probe = probe)) } } @@ -366,10 +407,13 @@ class ConversionViewModel @JvmOverloads constructor( quality = settings.quality, enginePreference = settings.enginePreference, ) + // Tapping Convert claims the screen for this job, which is what supersedes the pick's + // still-in-flight probe and any reattachment that has not finished asking. + val token = ownership.claim() activeWorkId = request.id workManager.enqueue(request) _state.value = ConversionState.Converting(input, 0) - observe(request.id, input) + observe(request.id, input, token = token) } /** @@ -377,12 +421,27 @@ class ConversionViewModel @JvmOverloads constructor( * picked file, ready to convert again. For one picked up by [reattach] there is no picked * file — the URI that job holds belongs to a process that no longer exists — so it lands * on Idle instead, rather than offering a Convert button over a file nothing can open. + * @param token the claim this observation belongs to. Nothing here can write until `collect` + * has resumed with a `WorkInfo`, which is some time after the caller decided to observe, so + * the claim is checked again at the last possible moment rather than trusted from then. This + * is issue #49's fix and the only thing standing between a superseded observation and the + * user's screen — see [ScreenOwnership]. */ - private fun observe(id: UUID, input: InputFile, cancelled: ConversionState = ConversionState.Ready(input)) { + private fun observe( + id: UUID, + input: InputFile, + cancelled: ConversionState = ConversionState.Ready(input), + token: Long, + ) { observer?.cancel() observer = viewModelScope.launch { workManager.getWorkInfoByIdFlow(id).collect { info -> if (info == null) return@collect + // Ahead of the `when`, not merely ahead of the assignment: the SUCCEEDED branch + // takes ownership of the staged file, and a superseded observation must not do + // that either. The state and `pendingStaged` are meant to refer to the same file + // or to no file, and this is where that stays true. + if (!ownership.stillHeldBy(token)) return@collect _state.value = when (info.state) { WorkInfo.State.RUNNING -> ConversionState.Converting( input, @@ -528,6 +587,10 @@ class ConversionViewModel @JvmOverloads constructor( * existed, this delete was the only thing a failed save could lead to — which was the defect. */ fun reset() { + // Start over is a claim like any other. The cancel below is a request honoured at the next + // suspension point, so a collector already on its way to a write has nothing left to + // honour it at; the claim is what actually stops that write landing on top of Idle. + ownership.claim() observer?.cancel() observer = null activeWorkId = null diff --git a/app/src/main/java/org/libremediaconverter/convert/ScreenOwnership.kt b/app/src/main/java/org/libremediaconverter/convert/ScreenOwnership.kt new file mode 100644 index 0000000..527f073 --- /dev/null +++ b/app/src/main/java/org/libremediaconverter/convert/ScreenOwnership.kt @@ -0,0 +1,57 @@ +package org.libremediaconverter.convert + +/** + * Which of the things writing to a screen is still allowed to. + * + * Both ViewModels are a state machine written to from several coroutines that each suspend before + * they write: a pick hops to a dispatcher for the metadata query, a reattachment hops for the tag + * query, and an observation of a WorkManager job cannot write at all until its `collect` has + * resumed with a `WorkInfo`. Whoever resumes last wins, which is how issue #49 let a finished job + * from an earlier session take a screen the user had already picked a file on. + * + * The rule this makes enforceable is one line: **every write that lands after a suspension point + * checks the claim it was made under, and drops itself if that claim has been superseded.** The + * claim is taken synchronously, when the user acts; the check happens immediately before the + * write. Superseded work is *dropped*, not reordered — a dropped write cannot come back later. + * + * Cancelling the superseded coroutine is not a substitute and was never going to be. `Job.cancel` + * is a request, honoured at the next suspension point; a collector that has already resumed and is + * on its way to `_state.value = …` has no suspension point left to honour it at, so the write + * lands anyway. Cancellation also cannot help at all in the case #49 actually reported, where + * nothing supersedes the observation until after it has been launched. Both ViewModels still + * cancel their old observer, because leaving a collector running is a leak — but the guarantee + * does not rest on it. + * + * **Confined to the main dispatcher, and that confinement is the atomicity argument.** Every + * claim and every check runs there, with no suspension point between a check and the write it + * guards, so a claim can never land between the two. Nothing here is synchronized and nothing is + * `@Volatile`: making the field visible across threads would invite exactly the off-main use this + * cannot support, and would replace an argument that holds with one that only looks like it does. + */ +internal class ScreenOwnership { + + private var claims = 0L + + /** + * The claim in force now. + * + * Read by work that is about to suspend and will want to know, when it comes back, whether + * the screen it was reading is still the screen it is writing to. Read it *before* the + * suspension, not after — reading it afterwards would return whatever claim superseded it, + * which is the bug rather than the check for it. + */ + val current: Long get() = claims + + /** + * Takes the screen, invalidating every write still in flight under an older claim. + * + * Called synchronously from the user's action rather than from inside the coroutine it + * starts. A claim made inside a `launch` is only immediate while the dispatcher happens to + * run it inline, and a deferred claim is no claim at all: it would leave the same gap this + * exists to close. + */ + fun claim(): Long = ++claims + + /** Whether [token] is still the claim in force, and may therefore write. */ + fun stillHeldBy(token: Long): Boolean = token == claims +} diff --git a/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt b/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt index 7e2d8e6..3d0cd2a 100644 --- a/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt +++ b/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt @@ -20,6 +20,7 @@ import org.libremediaconverter.convert.InputFile import org.libremediaconverter.convert.InputQuery import org.libremediaconverter.convert.PendingSave import org.libremediaconverter.convert.STAGED_FILE_GONE_MESSAGE +import org.libremediaconverter.convert.ScreenOwnership import org.libremediaconverter.model.ConcatStrategy import org.libremediaconverter.work.ConcatWorker import org.libremediaconverter.work.JobTags @@ -76,6 +77,15 @@ class JoinViewModel @JvmOverloads constructor( app: Application, /** Where [reset] runs its delete. See the same parameter on `ConversionViewModel`. */ private val cleanupDispatcher: CoroutineDispatcher = Dispatchers.IO, + /** + * Where the metadata query behind a pick runs. See the same parameter on `ConversionViewModel`. + * + * The join side had no such seam, and the gap was not cosmetic: the one write `onInputsPicked` + * makes lands *after* this hop, so a test that wants to ask what happens while a pick is still + * in flight had no way to hold one there. Issue #49's race is exactly that question, and it + * went unasked on this side for as long as the dispatcher was a literal. + */ + private val pickDispatcher: CoroutineDispatcher = Dispatchers.IO, ) : AndroidViewModel(app) { private val workManager = WorkManager.getInstance(app) @@ -90,6 +100,20 @@ class JoinViewModel @JvmOverloads constructor( private var observer: Job? = null private var activeWorkId: UUID? = null + /** + * Who is allowed to write to this screen -- see [ScreenOwnership], which carries the rule and + * the reason cancelling the superseded coroutine is not one. + * + * The convert side had issue #49 reported against it four times in two days; this side has the + * identical shape and was never reported, because nothing was watching. Every write below that + * lands after a suspension point is guarded: the one in [onInputsPicked] and the one in + * [observe]. [save] is the one left out, deliberately and with the same limit its counterpart + * in `ConversionViewModel` spells out: nothing can overwrite what it writes, but it can still + * land on top of a [reset] taken while its copy was in flight. Which way that should go is a + * question about the save screen rather than about this race -- issue #123. + */ + private val ownership = ScreenOwnership() + /** * The staged output this ViewModel is responsible for deleting. * @@ -116,6 +140,10 @@ class JoinViewModel @JvmOverloads constructor( * the rules about which job and why. */ private fun reattach() { + // Read before the launch, and before the query it is about to suspend in: this is the + // claim the answer belongs to. Reading it on the far side of the query would read whatever + // superseded it, which is the bug rather than the check for it. + val token = ownership.current viewModelScope.launch { val reattachment = Reattachment.choose( workManager.jobSnapshots( @@ -125,8 +153,15 @@ class JoinViewModel @JvmOverloads constructor( ) ?: return@launch // The query suspends, so the user may have picked files or started a join in the - // meantime. Theirs wins. No suspension point between this check and the assignment - // below, and both run on the main dispatcher, so nothing can interleave. + // meantime. Theirs wins. + // + // This catches a pick that has already *landed*, and only that. It used to claim there + // was "no suspension point between this check and the assignment below", which was the + // opposite of what happens: there is no assignment below, only observe(), which + // launches a separate coroutine that cannot write until its `collect` resumes. The + // check happens at one moment and the write lands at another, with a whole pick able + // to fit in between -- issue #49. The token observe() carries is what holds that line; + // see [ScreenOwnership]. if (_state.value !is JoinState.Idle || activeWorkId != null) return@launch // Joins used to stage under one constant name, so two finished joins always reported @@ -146,19 +181,29 @@ class JoinViewModel @JvmOverloads constructor( InputFile(Uri.EMPTY, "", sizeBytes = null) } activeWorkId = reattachment.job.id - observe(reattachment.job.id, inputs, cancelled = JoinState.Idle) + observe(reattachment.job.id, inputs, cancelled = JoinState.Idle, token = token) } } + /** + * The tap is the claim, which is why it is taken here rather than inside the `launch` -- and + * above the early return, so the refusal below is covered by it too. A claim made in the + * coroutine is only immediate while `Dispatchers.Main.immediate` happens to run it inline, and + * a deferred claim leaves exactly the gap this closes. + */ fun onInputsPicked(uris: List) { + val token = ownership.claim() if (uris.size < 2) { _state.value = JoinState.Failed("Pick at least two files to join.") return } viewModelScope.launch { - val files = withContext(Dispatchers.IO) { + val files = withContext(pickDispatcher) { uris.map { InputQuery.describe(getApplication(), it) } } + // Guarded like every other write that lands after a hop: two picks in quick succession + // suspend here together, and the slower one would otherwise land last. + if (!ownership.stillHeldBy(token)) return@launch _state.value = JoinState.Ready(files) } } @@ -172,10 +217,13 @@ class JoinViewModel @JvmOverloads constructor( // did answer would hand the space check a lower bound it would read as a total. totalBytes = InputQuery.total(inputs.map { it.sizeBytes }), ) + // Tapping Join claims the screen for this job, superseding any reattachment that has not + // finished asking. + val token = ownership.claim() activeWorkId = request.id workManager.enqueue(request) _state.value = JoinState.Joining(inputs) - observe(request.id, inputs) + observe(request.id, inputs, token = token) } /** @@ -183,12 +231,25 @@ class JoinViewModel @JvmOverloads constructor( * files, ready to join again. For one picked up by [reattach] there are no picked files — * what that job holds are URIs granted to a process that no longer exists — so it lands on * Idle rather than offering to re-join files nothing can open. + * @param token the claim this observation belongs to. Nothing here can write until `collect` + * has resumed with a `WorkInfo`, which is some time after the caller decided to observe, so + * the claim is checked again at the last possible moment rather than trusted from then. See + * [ScreenOwnership], and issue #49. */ - private fun observe(id: UUID, inputs: List, cancelled: JoinState = JoinState.Ready(inputs)) { + private fun observe( + id: UUID, + inputs: List, + cancelled: JoinState = JoinState.Ready(inputs), + token: Long, + ) { observer?.cancel() observer = viewModelScope.launch { workManager.getWorkInfoByIdFlow(id).collect { info -> if (info == null) return@collect + // Ahead of the `when`, not merely ahead of the assignment: the SUCCEEDED branch + // takes ownership of the staged file, and a superseded observation must not do + // that either. + if (!ownership.stillHeldBy(token)) return@collect _state.value = when (info.state) { WorkInfo.State.RUNNING, WorkInfo.State.BLOCKED -> JoinState.Joining(inputs) WorkInfo.State.ENQUEUED -> @@ -301,6 +362,10 @@ class JoinViewModel @JvmOverloads constructor( * this button, so deletion is what the user chose rather than all this state could do. */ fun reset() { + // Start over is a claim like any other. The cancel below is a request honoured at the next + // suspension point, so a collector already on its way to a write has nothing left to + // honour it at; the claim is what stops that write landing on top of Idle. + ownership.claim() observer?.cancel() observer = null activeWorkId = null diff --git a/app/src/test/java/org/libremediaconverter/convert/ParkedPickDispatcher.kt b/app/src/test/java/org/libremediaconverter/convert/ParkedPickDispatcher.kt new file mode 100644 index 0000000..3a79e54 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/ParkedPickDispatcher.kt @@ -0,0 +1,63 @@ +package org.libremediaconverter.convert + +import kotlinx.coroutines.CoroutineDispatcher +import java.util.concurrent.ConcurrentLinkedQueue +import kotlin.coroutines.CoroutineContext + +/** + * A dispatcher that holds a pick in flight until the test lets it finish. + * + * Issue #49 is about what a ViewModel does *while* a pick is between the tap and the write it + * eventually makes. Both ViewModels put a blocking hop there — the metadata query, and on the + * convert side the probe as well — and both hops go through an injectable dispatcher. Handing + * them this one turns "the pick has been made but has not landed yet" from a window a test has + * to race into a state it can simply sit in. + * + * Nothing here is a fake pick. The real `InputQuery.describe` still runs, on this thread, + * whenever [runAll] is called; the only thing under the test's control is *when*. + * + * Confined to the thread that drives the test. Both ViewModels reach `withContext(pickDispatcher)` + * from a coroutine on the main dispatcher, so [dispatch] is only ever called from there — the + * queue is concurrent anyway, because a dispatcher that quietly dropped a block from another + * thread would fail as a hang rather than as an assertion. + */ +class ParkedPickDispatcher : CoroutineDispatcher() { + + private val parked = ConcurrentLinkedQueue() + + /** + * How many blocks are waiting. + * + * Asserted on before the interesting part of a test, because "the pick was in flight" is a + * premise rather than a detail: a zero here means the pick had already landed and whatever + * the test went on to prove was proved about a different situation. + */ + val parkedCount: Int get() = parked.size + + override fun dispatch(context: CoroutineContext, block: Runnable) { + parked += block + } + + /** + * Removes everything parked, oldest first, and hands it to the caller to run. + * + * What [runAll] cannot express: two picks are two hops through this dispatcher, and the defect + * they can produce is the *first* one finishing last. Running them in the order they arrived + * is the one order in which nothing goes wrong, so a test has to be able to choose. + */ + fun takeParked(): List = generateSequence { parked.poll() }.toList() + + /** + * Runs everything parked, and everything that parks as a result. + * + * The loop is not defensive: `ConversionViewModel.onInputPicked` makes two hops through this + * dispatcher — the metadata query, then the probe — and the second is only enqueued once the + * first has run. Draining once would leave the probe parked for the rest of the process. + */ + fun runAll() { + while (true) { + val next = parked.poll() ?: return + next.run() + } + } +} diff --git a/app/src/test/java/org/libremediaconverter/convert/PickOwnershipTest.kt b/app/src/test/java/org/libremediaconverter/convert/PickOwnershipTest.kt new file mode 100644 index 0000000..bd910c3 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/PickOwnershipTest.kt @@ -0,0 +1,125 @@ +package org.libremediaconverter.convert + +import android.app.Application +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import androidx.work.workDataOf +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +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 java.io.File + +/** + * The half of issue #49 that is not about reattachment at all. + * + * `onInputPicked` makes two writes and both of them land after a hop off the main thread, so both + * belong to whichever pick was in flight rather than to whichever pick the user last made. Nothing + * was enforcing that. Two taps in quick succession — an easy thing to do while a `content://` + * metadata query is slow — put the loser's file on screen if its query happened to come back + * second, which is the same defect the ticket reported against reattachment with a different + * coroutine on the losing side. + * + * Both cases below were measured rather than assumed: deleting either guard turns the matching + * test red, and deleting the probe one turns nine other tests red with it. Neither was ever + * reported, because a pick that loses to another pick still shows *a* file the user chose -- which + * is what made it worth closing alongside #49 rather than leaving as a second thing to find. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class PickOwnershipTest { + + private lateinit var app: Application + private lateinit var parkedPick: ParkedPickDispatcher + private lateinit var viewModel: ConversionViewModel + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + ConversionDependencies.publisher = { RecordingPublisher(app) } + ConversionDependencies.probe = { _, _ -> PROBE } + installTestWorkManager(app, workDataOf(ConversionWorker.KEY_OUTPUT_PATH to "/dev/null")) + + parkedPick = ParkedPickDispatcher() + viewModel = ConversionViewModel(app, pickDispatcher = parkedPick) + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + /** + * Two taps, and the first one's metadata query is the slow one. + * + * The order is chosen rather than raced: both queries are parked, and this runs the second + * before the first. Without an ownership check the straggler writes last and the screen ends + * up showing a file the user moved off two taps ago. + */ + @Test + fun `the slower of two picks does not land on top of the faster one`() { + viewModel.onInputPicked(FIRST) + viewModel.onInputPicked(SECOND) + + val queries = parkedPick.takeParked() + assertEquals("both picks should be in flight", 2, queries.size) + // The second pick's query comes back first; the first pick's is the straggler. + queries[1].run() + queries[0].run() + + val current = viewModel.state.value + assertEquals( + "a pick the user has already replaced took the screen: $current", + SECOND, + (current as ConversionState.Ready).input.uri, + ) + } + + /** + * The second of `onInputPicked`'s two writes, which lands a whole probe later. + * + * The probe hop is a native process spawn, so it is the longest gap in a pick and the easiest + * one to pick again during. This used to be guarded by comparing URIs against the state, which + * answers a narrower question than the one that matters — it cannot tell a second pick of the + * same file from the first, and it reads a state that a later claim may not have written yet, + * which is exactly this case: the newer pick has claimed the screen but its own query has not + * come back, so the state still names the older file and the comparison waves it through. + */ + @Test + fun `a probe from a pick the user has moved off does not fill the card in`() { + viewModel.onInputPicked(FIRST) + parkedPick.takeParked().single().run() + assertEquals(FIRST, (viewModel.state.value as ConversionState.Ready).input.uri) + + // The user picks again while the first pick is still probing. + viewModel.onInputPicked(SECOND) + val pending = parkedPick.takeParked() + assertEquals("the first probe and the second query should both be waiting", 2, pending.size) + pending[0].run() + + assertNull( + "a probe belonging to a pick the user replaced must not reach the card", + (viewModel.state.value as ConversionState.Ready).input.probe, + ) + + // And the pick that did win still fills its own card in, probe included. + pending[1].run() + parkedPick.runAll() + val settled = viewModel.state.value as ConversionState.Ready + assertEquals(SECOND, settled.input.uri) + assertNotNull("the winning pick's own probe still has to land", settled.input.probe) + } + + private companion object { + val FIRST: Uri = Uri.fromFile(File("/tmp/first.mp4")) + val SECOND: Uri = Uri.fromFile(File("/tmp/second.mp4")) + val PROBE = InputProbe(videoCodec = "h264") + } +} diff --git a/app/src/test/java/org/libremediaconverter/convert/ReattachmentOwnershipTest.kt b/app/src/test/java/org/libremediaconverter/convert/ReattachmentOwnershipTest.kt new file mode 100644 index 0000000..b118cff --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/ReattachmentOwnershipTest.kt @@ -0,0 +1,156 @@ +package org.libremediaconverter.convert + +import android.app.Application +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import androidx.work.WorkManager +import androidx.work.workDataOf +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 java.io.File + +/** + * Issue #49, on the JVM and without the race. + * + * `ReattachOnLaunchTest.doesNotOverwriteAPickTheUserHasAlreadyMade` has been catching this on + * devices since 2026-08-24 — four occurrences, spread across API 33, 35 and 36, which is what + * ruled out an emulator-image quirk. Every one was on attempt 1 and every one passed on re-run, + * which is why it was read as flaky infrastructure for two days. It is not. The assertion it fails + * on is `expected null, but was:`: a finished job from an earlier session taking a + * screen the user had already picked a file on. + * + * The defect is a check-then-act whose act is deferred into another coroutine. `reattach()` reads + * `_state.value` and then calls `observe()`, which *launches* a collector that has to suspend on + * `getWorkInfoByIdFlow(...).collect` before it can write anything. So the check happens at one + * moment and the write lands at another: + * + * 1. `init` starts the tag query and suspends in it. + * 2. The user picks a file; `onInputPicked` suspends in its metadata query. + * 3. The query comes back. `_state.value` is still `Idle` — step 2 has not written yet — so the + * guard passes and an observation of the old job is launched. + * 4. The pick lands. `Ready(picked)`. The user owns the screen. + * 5. The observation's first `WorkInfo` arrives and writes `Converted(yesterday)` over it. + * + * The comment above that guard claimed "no suspension point between this check and the assignment + * below, so nothing can interleave". There is no assignment below, and the check and the write + * are in different coroutines. + * + * [ReattachGuardsTest] covers the case where the pick has already *landed*, which the plain guard + * does catch. This covers the one where it is still in flight, which it does not. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class ReattachmentOwnershipTest { + + private lateinit var app: Application + private lateinit var publisher: RecordingPublisher + private lateinit var workManager: WorkManager + private lateinit var staged: File + + @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)) } + installTestWorkManager(app, workDataOf(ConversionWorker.KEY_OUTPUT_PATH to staged.absolutePath)) + workManager = WorkManager.getInstance(app) + // The situation reattachment exists for: a conversion that finished in a process that is + // gone, with its output still in the cache and nothing in the UI holding its id. + workManager.enqueue( + ConversionWorker.request( + inputUri = Uri.parse("content://test/holiday.mp4"), + displayName = "holiday.mp4", + sizeBytes = 4_096L, + ), + ).result.get() + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + /** + * The race, made into a state the test can sit in rather than one it has to catch. + * + * The pick is parked on a dispatcher this test owns, so it stays in flight — issued, not yet + * written — for as long as the assertions need it to be. Everything else is production: a + * real `WorkManager` holding a real finished job, the real `reattach`, the real `observe`. + * + * Determinism comes from where Robolectric leaves the main looper. `reattach`'s tag query hops + * to a real [kotlinx.coroutines.Dispatchers.IO] thread, so its continuation can only come back + * as a message posted to the main looper — and that looper is paused, so it cannot run until + * something pumps it. `onInputPicked` is an ordinary synchronous call from this thread. The + * pick is therefore *always* issued before the guard runs; none of it is left to timing, which + * is the whole point of writing it here rather than relying on the rare device sighting. + */ + @Test + fun `a conversion found while the user was picking never reaches the screen`() { + val picked = Uri.fromFile(File(app.cacheDir, "beach.mp4").apply { writeBytes(ByteArray(2048)) }) + val parkedPick = ParkedPickDispatcher() + val viewModel = ConversionViewModel(app, pickDispatcher = parkedPick) + viewModel.onInputPicked(picked) + + assertEquals( + "the pick must still be in flight, or this proves something about a different situation", + 1, + parkedPick.parkedCount, + ) + + // The control, and the reason this test does not rest on a settle window being long + // enough. A second ViewModel with nothing to supersede it reattaches to the same job + // through the same code; when it has arrived, the whole query-guard-observe-write path has + // demonstrably run to completion. `viewModel` started its own reattachment first, so it + // has had at least as long. Waiting on this rather than on a sleep is what makes the + // assertion below "it did not happen" rather than "it had not happened yet". + reattachmentHasRunToCompletion() + + val current = viewModel.state.value + assertTrue("reattachment took the screen from the user: $current", current is ConversionState.Idle) + + // And the pick, when it lands, is what stays there. + parkedPick.runAll() + val ready = awaitState(viewModel.state, "Ready") { it is ConversionState.Ready } + assertEquals(picked, (ready as ConversionState.Ready).input.uri) + reattachmentHasRunToCompletion() + assertEquals("the user's pick must survive a late reattachment", ready, viewModel.state.value) + } + + /** + * The other half of the contract: a reattachment nobody has superseded still takes the screen. + * + * Without this, dropping every reattachment on the floor would pass the test above. Same job, + * same WorkManager, same production path — only the pick is missing. + */ + @Test + fun `a conversion nobody has superseded still reaches the screen`() { + val converted = awaitState(ConversionViewModel(app).state, "Converted") { + it is ConversionState.Converted + } + + assertEquals(staged.absolutePath, (converted as ConversionState.Converted).staged.absolutePath) + assertEquals("holiday.mp4", converted.input.displayName) + } + + /** + * Drives a throwaway ViewModel through a whole reattachment, and returns once it has landed. + * + * [awaitState] pumps the main looper, which is what runs every reattachment continuation + * waiting on it — this one's, and the one belonging to the ViewModel under test, which was + * posted earlier and therefore runs first. + */ + private fun reattachmentHasRunToCompletion() { + awaitState(ConversionViewModel(app).state, "Converted") { it is ConversionState.Converted } + } +} diff --git a/app/src/test/java/org/libremediaconverter/join/JoinPickOwnershipTest.kt b/app/src/test/java/org/libremediaconverter/join/JoinPickOwnershipTest.kt new file mode 100644 index 0000000..ccf7ec5 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/join/JoinPickOwnershipTest.kt @@ -0,0 +1,87 @@ +package org.libremediaconverter.join + +import android.app.Application +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import androidx.work.workDataOf +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.convert.ConversionDependencies +import org.libremediaconverter.convert.ParkedPickDispatcher +import org.libremediaconverter.convert.RecordingPublisher +import org.libremediaconverter.convert.installTestWorkManager +import org.libremediaconverter.work.ConcatWorker +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment + +/** + * `PickOwnershipTest`'s case on the join side. + * + * `onInputsPicked` makes one write and it lands after a hop off the main thread, so it belongs to + * whichever pick was in flight rather than to whichever set of files the user last chose. Two + * selections in quick succession — likelier here than on the convert side, since a join picks + * several files at a time and the metadata query is per file — put the loser's files on screen if + * its query came back second. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class JoinPickOwnershipTest { + + private lateinit var app: Application + private lateinit var parkedPick: ParkedPickDispatcher + private lateinit var viewModel: JoinViewModel + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + ConversionDependencies.publisher = { RecordingPublisher(app) } + installTestWorkManager(app, workDataOf(ConcatWorker.KEY_OUTPUT_PATH to "/dev/null")) + + parkedPick = ParkedPickDispatcher() + viewModel = JoinViewModel(app, pickDispatcher = parkedPick) + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + /** + * Two selections, with the first one's metadata query the slow one. + * + * The order is chosen rather than raced: both queries are parked, and this runs the second + * before the first. + */ + @Test + fun `the slower of two selections does not land on top of the faster one`() { + viewModel.onInputsPicked(FIRST) + viewModel.onInputsPicked(SECOND) + + val queries = parkedPick.takeParked() + assertEquals("both selections should be in flight", 2, queries.size) + // The second selection's query comes back first; the first one's is the straggler. + queries[1].run() + queries[0].run() + + val current = viewModel.state.value + assertEquals( + "a selection the user has already replaced took the screen: $current", + SECOND, + (current as JoinState.Ready).inputs.map { it.uri }, + ) + } + + private companion object { + val FIRST = listOf( + Uri.parse("content://test/first-a.mp4"), + Uri.parse("content://test/first-b.mp4"), + ) + val SECOND = listOf( + Uri.parse("content://test/second-a.mp4"), + Uri.parse("content://test/second-b.mp4"), + ) + } +} diff --git a/app/src/test/java/org/libremediaconverter/join/JoinReattachmentOwnershipTest.kt b/app/src/test/java/org/libremediaconverter/join/JoinReattachmentOwnershipTest.kt new file mode 100644 index 0000000..b47ec3e --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/join/JoinReattachmentOwnershipTest.kt @@ -0,0 +1,166 @@ +package org.libremediaconverter.join + +import android.app.Application +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import androidx.work.WorkManager +import androidx.work.workDataOf +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.convert.ConversionDependencies +import org.libremediaconverter.convert.ParkedPickDispatcher +import org.libremediaconverter.convert.RecordingPublisher +import org.libremediaconverter.convert.awaitState +import org.libremediaconverter.convert.installTestWorkManager +import org.libremediaconverter.model.ConcatStrategy +import org.libremediaconverter.work.ConcatWorker +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.io.File + +/** + * Issue #49 on the join side, where nothing was watching for it. + * + * `reattach()` checks that the screen is still free, and then hands the answer to `observe()`, + * which writes from a *different* coroutine that has to suspend on `collect` before it can write + * anything at all. So the check happens at one moment and the write lands at another, with a + * whole pick able to fit in between: + * + * 1. `init` starts the tag query and suspends in it. + * 2. The user picks files; `onInputsPicked` suspends in its metadata query. + * 3. The query comes back. The screen is still `Idle` — step 2 has not written yet — so the + * guard passes and an observation of the old job is launched. + * 4. The pick lands. `Ready(picked)`. The user owns the screen. + * 5. The observation's first `WorkInfo` arrives and writes `Joined(yesterday's file)` over it. + * + * The comment above that guard used to say "no suspension point between this check and the + * assignment below, so nothing can interleave". There is no assignment below, and the two lines + * are in different coroutines. + * + * The convert side has been failing this on CI for two days — four occurrences across three API + * levels, each read as flaky infrastructure. `JoinViewModel` has the identical shape and no test + * at all, which is why this one was written before the fix rather than after it. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class JoinReattachmentOwnershipTest { + + private lateinit var app: Application + private lateinit var publisher: RecordingPublisher + private lateinit var workManager: WorkManager + private lateinit var staged: File + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + publisher = RecordingPublisher(app) + ConversionDependencies.publisher = { publisher } + + staged = publisher.createStagingFile("joined-yesterday.mp4").apply { writeBytes(ByteArray(4096)) } + installTestWorkManager( + app, + workDataOf( + ConcatWorker.KEY_OUTPUT_PATH to staged.absolutePath, + ConcatWorker.KEY_STRATEGY to ConcatStrategy.STREAM_COPY.name, + ), + ) + workManager = WorkManager.getInstance(app) + // The situation reattachment exists for: a join that finished in a process that is gone, + // with its output still in the cache and nothing in the UI holding its id. + workManager.enqueue( + ConcatWorker.request( + inputs = listOf( + Uri.parse("content://test/yesterday-a.mp4"), + Uri.parse("content://test/yesterday-b.mp4"), + ), + totalBytes = 8_192L, + ), + ).result.get() + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + /** + * The race, made into a state the test can sit in rather than one it has to catch. + * + * The pick is parked on a dispatcher this test owns, so it is in flight — issued, not yet + * written — for as long as the assertions need it to be. Everything else is real: a real + * `WorkManager` holding a real finished job, the production `reattach`, the production + * `observe`. + * + * Determinism comes from where Robolectric leaves the main looper. `reattach`'s tag query + * hops to a real [kotlinx.coroutines.Dispatchers.IO] thread, so its continuation can only + * come back as a message posted to the main looper — and that looper is paused, so it cannot + * run until something pumps it. `onInputsPicked` is an ordinary synchronous call from this + * thread. The pick is therefore always issued before the guard runs, with nothing left to + * timing. + */ + @Test + fun `a join found while the user was picking never reaches the screen`() { + val parkedPick = ParkedPickDispatcher() + val viewModel = JoinViewModel(app, pickDispatcher = parkedPick) + viewModel.onInputsPicked(PICKED) + + assertEquals( + "the pick must still be in flight, or this proves something about a different situation", + 1, + parkedPick.parkedCount, + ) + + // The control, and the reason this test does not rest on a settle window being long + // enough. A second ViewModel with nothing to supersede it reattaches to the same job + // through the same code; when it has arrived, the whole query-guard-observe-write path + // has demonstrably run to completion. `viewModel` started its own reattachment first, so + // it has had at least as long. Waiting on this rather than on a sleep is what makes the + // assertion below "it did not happen" instead of "it had not happened yet". + reattachmentHasRunToCompletion() + + val current = viewModel.state.value + assertTrue("reattachment took the screen from the user: $current", current is JoinState.Idle) + + // And the pick, when it lands, is what stays there. + parkedPick.runAll() + val ready = awaitState(viewModel.state, "Ready") { it is JoinState.Ready } + assertEquals(PICKED, (ready as JoinState.Ready).inputs.map { it.uri }) + reattachmentHasRunToCompletion() + assertEquals("the user's pick must survive a late reattachment", ready, viewModel.state.value) + } + + /** + * The other half of the contract: a reattachment nobody has superseded still takes the screen. + * + * Without this, dropping every reattachment on the floor would pass the test above. It is the + * same job, the same WorkManager and the same production path — only the pick is missing. + */ + @Test + fun `a join nobody has superseded still reaches the screen`() { + val joined = awaitState(JoinViewModel(app).state, "Joined") { it is JoinState.Joined } + + assertEquals(staged.absolutePath, (joined as JoinState.Joined).staged.absolutePath) + } + + /** + * Drives a throwaway ViewModel through a whole reattachment, and returns once it has landed. + * + * [awaitState] pumps the main looper, which is what runs every reattachment continuation + * waiting on it — this one's and the one belonging to the ViewModel under test, which was + * posted earlier and therefore runs first. + */ + private fun reattachmentHasRunToCompletion() { + awaitState(JoinViewModel(app).state, "Joined") { it is JoinState.Joined } + } + + private companion object { + val PICKED = listOf( + Uri.parse("content://test/clip-one.mp4"), + Uri.parse("content://test/clip-two.mp4"), + ) + } +}