Merge the staging-cleanup fix, and pin the seam the two changes share

The two changes meet at one line. `pendingStaged` is recorded in the `SUCCEEDED` branch of
each ViewModel's `observe()` collector, and reattachment reaches `Converted`/`Joined`
through that same collector rather than by building the state itself -- so a job picked up
from a previous process arrives with its cleanup handle already set, and "Start over" on it
deletes the staged file exactly as it does for a conversion run in this process. Nothing had
to be added for that; it falls out of routing reattachment through `observe()`.

Which is precisely why it needed a test. The claim is structural -- one assignment, in one
function, that both changes assume -- and the conflict here was `JoinViewModel`, where the
cleanup change edits a collector body that the reattachment change had moved out of `join()`
into a private `observe()`. Resolving that by putting the assignment back in `join()` would
compile, pass every test either branch brought, and silently leak a full-size file on the one
path both changes were written for. `ReattachedCleanupTest` fails if it lands anywhere else.

Verified the way the cleanup commit verified its own wiring: deleting `pendingStaged = staged`
from `ConversionViewModel.observe()` fails "start over on a reattached conversion deletes the
staged file" alongside the two `ConversionViewModelCleanupTest` cases that share the line.
Restored, all 183 JVM tests pass.

The reattachment path also gains JVM coverage it could not have had before this merge, since
Robolectric and the `ConversionDependencies` seams arrived with it: a ViewModel constructed
after a job has already finished, with nothing left that observed it, now demonstrably reaches
`Converted` with the display name recovered from the job's tags -- on a machine where no
instrumented test can run.

Conflict resolution: both sides kept in `JoinViewModel`, with the cleanup handle declared
alongside the other fields and the reattachment `init` after them. Nothing else conflicted;
`ConversionViewModel` merged clean because the reattachment change touches the `CANCELLED` and
`FAILED` branches while the cleanup change touches `SUCCEEDED`. No build file, manifest or
`OutputPublisher` line in this merge is mine -- they arrive from the cleanup commit verbatim.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
2026-08-22 18:51:42 -05:00
co-authored by Claude Opus 5
16 changed files with 999 additions and 10 deletions
+27
View File
@@ -102,6 +102,23 @@ android {
informational += "UsableSpace"
}
testOptions {
unitTests {
// Robolectric needs the merged manifest and the compiled resource table to build
// an Android runtime on the JVM. Without this, AGP hands the unit tests a stub
// android.jar with no resources and Robolectric cannot start.
isIncludeAndroidResources = true
all {
// Robolectric's native runtime calls System.load(), which Java 25 reports as
// a restricted method -- four lines of warning on every test run, and a hard
// failure in some later JDK. Granting it explicitly says the native access is
// known and wanted rather than leaving the JVM to guess.
it.jvmArgs("--enable-native-access=ALL-UNNAMED")
}
}
}
packaging {
jniLibs {
// Uncompressed .so, so the APK zip-aligns them on 16 KB boundaries.
@@ -263,6 +280,16 @@ dependencies {
debugImplementation(libs.compose.ui.tooling)
testImplementation(libs.junit)
// An Android runtime on the JVM. Everything else in src/test is a pure function; this is
// here for the one thing a pure function cannot assert -- that a staged file is really
// gone from a real cacheDir. Instrumented tests do not run on the development host, so
// without it that assertion could only be written where nobody can execute it.
testImplementation(libs.robolectric)
// Already in the catalog for androidTest, and already inside the prerelease guard via its
// androidx. group. WorkManagerTestInitHelper + SynchronousExecutor are what let a JVM test
// drive a ViewModel through a real WorkManager to SUCCEEDED, which is where the cleanup
// handle is set -- the wiring the leak actually lived in.
testImplementation(libs.androidx.work.testing)
androidTestImplementation(platform(libs.compose.bom))
androidTestImplementation(libs.androidx.junit)
+6
View File
@@ -20,7 +20,13 @@
off by default on new installs. -->
<uses-permission android:name="android.permission.POST_NOTIFICATIONS" />
<!--
The application class exists only to sweep abandoned staging files once per process.
See LibreMediaConverterApp for why process start is where that has to happen, and why
the sweep cannot take a file out from under a running worker.
-->
<application
android:name=".LibreMediaConverterApp"
android:allowBackup="true"
android:dataExtractionRules="@xml/data_extraction_rules"
android:fullBackupContent="@xml/backup_rules"
@@ -0,0 +1,58 @@
package org.libremediaconverter
import android.app.Application
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.SupervisorJob
import kotlinx.coroutines.launch
import org.libremediaconverter.convert.OutputPublisher
/**
* Exists for one reason: to sweep abandoned files out of `<cacheDir>/conversions/` once per
* process.
*
* Every other cleanup path in the app depends on a ViewModel still being alive to run it.
* The cases that leak are exactly the ones where it is not — the process is reclaimed
* between a conversion finishing and the user saving it, a worker fails before its output
* ever becomes a `Converted` state, or a `reset()`'s delete is cancelled along with the
* Activity. Process start is the one moment those leftovers are reliably observable.
*/
class LibreMediaConverterApp : Application() {
/**
* Deliberately process-lifetime and never cancelled: the work it carries is a single
* short task that should outlive nothing in particular and be interrupted by nothing.
* A `SupervisorJob` so a failure here could never take a sibling down with it.
*/
private val appScope = CoroutineScope(SupervisorJob() + Dispatchers.IO)
override fun onCreate() {
super.onCreate()
// Off the main thread: this lists a directory and stats each entry, and it runs on
// the path that decides how long the launcher icon stays unresponsive.
//
// Why this cannot race a live job -- and note the argument is NOT about ordering.
// WorkManager initialises through androidx.startup's InitializationProvider, which
// is a ContentProvider, so it is already up before onCreate() is called and can be
// resuming a worker on its own executor while this runs. Workers run in this same
// process, so "nothing has started yet" would simply be false.
//
// The grace period is what makes it safe. StagingSweep only collects a file nothing
// has written to for a full day:
//
// - Conversion and join outputs are written continuously, so a running job keeps
// its own mtime fresh and never looks abandoned.
// - concat_list.txt is the one file written once and then only read, so it is the
// one that has to be reasoned about rather than observed. A WorkManager attempt
// is capped by the six-hour-per-day foreground-service budget and a retry starts
// doWork() again from the top, rewriting the list file -- so no single attempt
// can hold a file untouched for twenty-four hours.
// - A worker resuming right now writes its files at attempt start, which makes
// them zero seconds old, not a day.
//
// sweepStaging() also re-reads each timestamp immediately before deleting, which
// closes the window between listing the directory and acting on the listing.
appScope.launch { OutputPublisher(this@LibreMediaConverterApp).sweepStaging() }
}
}
@@ -8,6 +8,7 @@ import androidx.lifecycle.viewModelScope
import androidx.media3.common.util.UnstableApi
import androidx.work.WorkInfo
import androidx.work.WorkManager
import kotlinx.coroutines.CoroutineDispatcher
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.Job
import kotlinx.coroutines.flow.MutableStateFlow
@@ -79,10 +80,29 @@ sealed interface ConversionState {
}
@UnstableApi
class ConversionViewModel(app: Application) : AndroidViewModel(app) {
class ConversionViewModel @JvmOverloads constructor(
app: Application,
/**
* Where [reset] runs its delete.
*
* A parameter so a test can make the cleanup run inline and assert on the result. It
* also makes the ordering an explicit choice rather than an accident: the state flips
* to `Idle` synchronously while the delete is dispatched, and naming the dispatcher is
* what says that was decided rather than inherited.
*
* `@JvmOverloads` keeps the single-argument constructor that `viewModel()`'s default
* `AndroidViewModelFactory` looks up reflectively; without it the app would crash on
* the first screen.
*/
private val cleanupDispatcher: CoroutineDispatcher = Dispatchers.IO,
) : AndroidViewModel(app) {
private val workManager = WorkManager.getInstance(app)
private val publisher = OutputPublisher(app)
// Through ConversionDependencies, like the workers, rather than `OutputPublisher(app)`
// direct: the ViewModels were the only place bypassing the seam, which left the
// cleanup wiring impossible to substitute in a test.
private val publisher = ConversionDependencies.publisher(app)
private val _state = MutableStateFlow<ConversionState>(ConversionState.Idle)
val state: StateFlow<ConversionState> = _state.asStateFlow()
@@ -90,6 +110,16 @@ class ConversionViewModel(app: Application) : AndroidViewModel(app) {
private var observer: Job? = null
private var activeWorkId: UUID? = null
/**
* The staged output this ViewModel is responsible for deleting.
*
* A field rather than something read back out of [_state], because the state machine
* cannot answer the question on the path that needs it most: a failed [save] lands on
* [ConversionState.Failed], which carries a message and no file at all. By then the
* only remaining reference would have been lost.
*/
private var pendingStaged: File? = null
/** Conversion settings, kept separate from the job state machine. */
private val _settings = MutableStateFlow(ConversionSettings())
val settings: StateFlow<ConversionSettings> = _settings.asStateFlow()
@@ -190,7 +220,7 @@ class ConversionViewModel(app: Application) : AndroidViewModel(app) {
// would read as the app having ignored the tap.
_state.value = ConversionState.Ready(file)
val probe = withContext(Dispatchers.IO) { MediaProbe.probe(getApplication(), uri) }
val probe = withContext(Dispatchers.IO) { ConversionDependencies.probe(getApplication(), 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) {
@@ -258,9 +288,13 @@ class ConversionViewModel(app: Application) : AndroidViewModel(app) {
if (path == null) {
ConversionState.Failed("Conversion reported success but produced no file.")
} else {
val staged = File(path)
// Take responsibility for the file at the same moment the state
// starts referring to it, so the two cannot disagree.
pendingStaged = staged
ConversionState.Converted(
input = input,
staged = File(path),
staged = staged,
engineUsed = info.outputData
.getString(ConversionWorker.KEY_ENGINE_USED).orEmpty(),
routeReason = info.outputData
@@ -299,6 +333,8 @@ class ConversionViewModel(app: Application) : AndroidViewModel(app) {
converted.staged.delete()
}
}.onSuccess {
// publish() already deleted it; nothing left to clean up.
pendingStaged = null
_state.value = ConversionState.Saved(
ConversionWorker.outputNameFor(
converted.input.displayName,
@@ -306,15 +342,35 @@ class ConversionViewModel(app: Application) : AndroidViewModel(app) {
),
)
}.onFailure { e ->
// Deliberately NOT cleared. A failed save may mean the staged file is the
// only copy of an hour of transcoding, and the user's destination did not
// receive it -- deleting here would destroy the work to tidy up a cache
// directory. It stays collectable: by a later reset(), or by the sweep once
// it is old enough to be certain nobody is coming back for it.
_state.value = ConversionState.Failed(e.message ?: "Could not save the file.")
}
}
}
/**
* Returns to [ConversionState.Idle], deleting anything staged on the way out.
*
* "Start over" on a finished conversion is an ordinary path through the UI, and it used
* to drop the only reference to a full-size file in cache. The delete runs on
* [Dispatchers.IO] because it touches the filesystem, and is fire-and-forget: it is
* cancelled with [viewModelScope] if the Activity finishes first, so it is a best
* effort rather than a guarantee. `OutputPublisher.sweepStaging` is the backstop for
* the times it does not run.
*/
fun reset() {
observer?.cancel()
observer = null
activeWorkId = null
val staged = pendingStaged
pendingStaged = null
if (staged != null) {
viewModelScope.launch(cleanupDispatcher) { publisher.discardStaged(staged) }
}
_state.value = ConversionState.Idle
}
@@ -41,10 +41,61 @@ open class OutputPublisher(private val context: Context) {
} ?: error("Could not open destination for writing: $destination")
}
fun clearStaging() {
stagingDir.listFiles()?.forEach { it.delete() }
/**
* Deletes one staged file, if it really is one of ours.
*
* This is what a ViewModel's `reset()` calls when the user taps "Start over" on a
* finished-but-unsaved conversion, which is otherwise a full-size copy left in cache
* for the OS to reclaim whenever it feels like it.
*
* The guard is not decoration. The handle reaches the ViewModel as a path string in
* `WorkInfo.outputData` and is turned straight into a `File`, so this is the one place
* that checks where it points before deleting. Comparing the *canonical* parent rather
* than the path as written is what makes `conversions/../something` fail: the naive
* string comparison accepts it.
*
* @return true if a file was deleted. False covers both "not in staging" and "already
* gone", which the caller has no reason to tell apart — a `reset()` after a
* successful save is an ordinary second call.
*/
open fun discardStaged(staged: File): Boolean {
val parent = staged.parentFile?.canonicalOrAbsolute() ?: return false
if (parent != stagingDir.canonicalOrAbsolute()) return false
return staged.delete()
}
/**
* Deletes staged files old enough to have been abandoned.
*
* The backstop for everything `discardStaged` cannot reach: a process killed between
* finishing a conversion and saving it, a worker that failed before its output ever
* became a `Converted` state, or a `reset()` whose delete was cancelled with the
* Activity. [StagingSweep] owns the rule and its reasoning.
*
* Deliberately not the `clearStaging()` this replaces. That deleted the directory's
* whole contents, and the convert tab, the join tab and `ConcatEngine`'s
* `concat_list.txt` all share this directory with no per-job namespacing, so a blanket
* delete could destroy a live job's file.
*
* [nowMs] is a parameter so the clock is the caller's, not a hidden global.
*/
open fun sweepStaging(nowMs: Long = System.currentTimeMillis()) {
val dir = stagingDir
val listing = dir.listFiles() ?: return
val entries = listing.map { StagingSweep.Entry(it.name, it.lastModified()) }
StagingSweep.collectable(entries, nowMs).forEach { name ->
val file = File(dir, name)
// Re-read the timestamp rather than trusting the snapshot above. Between the
// listing and here, a worker resumed by WorkManager -- which runs in this same
// process -- could have started writing this very file, and unlinking an inode a
// running job still holds open would end with the job reporting success for a
// path that no longer exists.
if (StagingSweep.isCollectable(file.lastModified(), nowMs)) file.delete()
}
}
private fun File.canonicalOrAbsolute(): File = runCatching { canonicalFile }.getOrDefault(absoluteFile)
private companion object {
const val SPACE_HEADROOM_BYTES = 128L * 1024 * 1024
}
@@ -0,0 +1,55 @@
package org.libremediaconverter.convert
/**
* Decides which staging entries are old enough to collect.
*
* A pure function rather than a loop inside [OutputPublisher], for the reason
* [org.libremediaconverter.work.FailureOutcome] documents: the decision is worth verifying
* and the situation that provokes it is not reproducible. Here the untestable part is the
* clock — an orphan is only interesting a day after it was written, and a filesystem's
* mtime granularity is not something a test should be measuring. Timestamps therefore
* arrive as values.
*
* The rule replaces an unconditional `clearStaging()` that deleted the directory's whole
* contents. That was hazardous: `<cacheDir>/conversions/` is shared by the convert tab, the
* join tab and [org.libremediaconverter.ffmpeg.ConcatEngine]'s `concat_list.txt`, and any
* two of them can be live at once, so a blanket delete could take a file out from under a
* running job. Age is the narrowing.
*/
object StagingSweep {
/** One directory entry, reduced to what the decision actually needs. */
data class Entry(val name: String, val lastModifiedMs: Long)
/**
* How stale a staging file has to be before it is assumed abandoned.
*
* Twenty-four hours, chosen against the longest a live file can plausibly go untouched
* rather than against how quickly cache should be reclaimed. Output files are written
* continuously, so a running job refreshes their mtime by itself. `concat_list.txt` is
* the exception — written once and then only read for the rest of the join — so the
* period has to exceed a whole join. WorkManager caps a single attempt at the
* six-hour-per-day foreground-service budget and then stops the worker, and a retry
* rewrites the list file, so no attempt can hold a file still for a day.
*/
const val GRACE_PERIOD_MS: Long = 24L * 60 * 60 * 1000
/** The names in [entries] that may be deleted, in the order they were given. */
fun collectable(entries: List<Entry>, nowMs: Long, gracePeriodMs: Long = GRACE_PERIOD_MS): List<String> = entries
.filter { isCollectable(it.lastModifiedMs, nowMs, gracePeriodMs) }
.map { it.name }
/**
* True if a file last written at [lastModifiedMs] is collectable at [nowMs].
*
* Exposed separately so the caller can re-check a single entry immediately before
* deleting it, closing the window between listing a directory and acting on the list.
*
* A negative age — a file dated in the future, because the clock moved backwards — is
* deliberately not collectable. It carries no information about whether the file is in
* use, and keeping a file costs cache while deleting one can cost the user an hour of
* transcoding.
*/
fun isCollectable(lastModifiedMs: Long, nowMs: Long, gracePeriodMs: Long = GRACE_PERIOD_MS): Boolean =
nowMs - lastModifiedMs >= gracePeriodMs
}
@@ -7,6 +7,7 @@ import org.libremediaconverter.codec.AndroidDeviceCodecs
import org.libremediaconverter.ffmpeg.FFmpegEngine
import org.libremediaconverter.model.ConversionRequest
import org.libremediaconverter.model.DeviceCodecs
import org.libremediaconverter.model.InputProbe
import org.libremediaconverter.model.OutputFormat
import java.io.File
@@ -74,10 +75,34 @@ object ConversionDependencies {
@Volatile
var deviceCodecs: () -> DeviceCodecs = { AndroidDeviceCodecs.get() }
/**
* Reading an input's codecs, container and duration.
*
* Here for a reason the others are not, and the reason is worth recording rather than
* just working around. [MediaProbe] spawns FFprobe, and when FFmpegKit's native library
* cannot load its initialiser throws a bare `java.lang.Error` — which
* `probeWithFFprobe`'s own `catch (e: Exception)` does not catch, and which
* `ConversionViewModel.onInputPicked` does not catch either. Picking a file would then
* fail with an uncaught error rather than the "could not read this file" the code was
* written to give.
*
* **That is a latent production hazard, found here and deliberately not fixed here.**
* It cannot fire on a device that ships the `.so` files, which is every real install,
* so making `MediaProbe` catch `Throwable` would be a behaviour change to the pick path
* on the strength of a condition no user meets — its own commit, with its own test.
* What this seam does is narrower: it keeps the JVM out of that path, which is what
* makes the ViewModel reachable from a unit test at all.
*
* Instrumented tests and the app itself get the real probe, exactly as before.
*/
@Volatile
var probe: (Context, Uri) -> InputProbe = { context, uri -> MediaProbe.probe(context, uri) }
fun reset() {
hardware = { Media3Engine(it) }
software = { FFmpegEngine() }
publisher = { OutputPublisher(it) }
deviceCodecs = { AndroidDeviceCodecs.get() }
probe = { context, uri -> MediaProbe.probe(context, uri) }
}
}
@@ -8,6 +8,7 @@ import androidx.lifecycle.viewModelScope
import androidx.media3.common.util.UnstableApi
import androidx.work.WorkInfo
import androidx.work.WorkManager
import kotlinx.coroutines.CoroutineDispatcher
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.Job
import kotlinx.coroutines.flow.MutableStateFlow
@@ -15,8 +16,8 @@ import kotlinx.coroutines.flow.StateFlow
import kotlinx.coroutines.flow.asStateFlow
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
import org.libremediaconverter.convert.ConversionDependencies
import org.libremediaconverter.convert.InputFile
import org.libremediaconverter.convert.OutputPublisher
import org.libremediaconverter.model.ConcatStrategy
import org.libremediaconverter.work.ConcatWorker
import org.libremediaconverter.work.JobTags
@@ -36,10 +37,17 @@ sealed interface JoinState {
}
@UnstableApi
class JoinViewModel(app: Application) : AndroidViewModel(app) {
class JoinViewModel @JvmOverloads constructor(
app: Application,
/** Where [reset] runs its delete. See the same parameter on `ConversionViewModel`. */
private val cleanupDispatcher: CoroutineDispatcher = Dispatchers.IO,
) : AndroidViewModel(app) {
private val workManager = WorkManager.getInstance(app)
private val publisher = OutputPublisher(app)
// Through ConversionDependencies, like the workers, rather than `OutputPublisher(app)`
// direct -- see the same line in ConversionViewModel.
private val publisher = ConversionDependencies.publisher(app)
private val _state = MutableStateFlow<JoinState>(JoinState.Idle)
val state: StateFlow<JoinState> = _state.asStateFlow()
@@ -47,6 +55,16 @@ class JoinViewModel(app: Application) : AndroidViewModel(app) {
private var observer: Job? = null
private var activeWorkId: UUID? = null
/**
* The staged output this ViewModel is responsible for deleting.
*
* Held here rather than read back out of [_state] for the same reason as in
* `ConversionViewModel`: a failed [save] lands on [JoinState.Failed], which carries a
* message and no file, so the state machine cannot answer this on the one path that
* most needs it.
*/
private var pendingStaged: File? = null
init {
reattach()
}
@@ -147,7 +165,11 @@ class JoinViewModel(app: Application) : AndroidViewModel(app) {
if (path == null) {
JoinState.Failed("Joining reported success but produced no file.")
} else {
JoinState.Joined(File(path), strategy)
val staged = File(path)
// Take responsibility for the file at the same moment the state
// starts referring to it, so the two cannot disagree.
pendingStaged = staged
JoinState.Joined(staged, strategy)
}
}
@@ -179,17 +201,34 @@ class JoinViewModel(app: Application) : AndroidViewModel(app) {
joined.staged.delete()
}
}.onSuccess {
// publish() already deleted it; nothing left to clean up.
pendingStaged = null
_state.value = JoinState.Saved("joined.mp4")
}.onFailure { e ->
// Deliberately NOT cleared -- see the same branch in ConversionViewModel.
// A failed save can leave the staged file as the only copy of the work, so
// it is left for a later reset() or for the sweep to collect once its age
// makes it certain nobody is coming back for it.
_state.value = JoinState.Failed(e.message ?: "Could not save the file.")
}
}
}
/**
* Returns to [JoinState.Idle], deleting anything staged on the way out.
*
* Best effort, not a guarantee: the delete is cancelled with [viewModelScope] if the
* Activity finishes first. `OutputPublisher.sweepStaging` is the backstop.
*/
fun reset() {
observer?.cancel()
observer = null
activeWorkId = null
val staged = pendingStaged
pendingStaged = null
if (staged != null) {
viewModelScope.launch(cleanupDispatcher) { publisher.discardStaged(staged) }
}
_state.value = JoinState.Idle
}
@@ -0,0 +1,118 @@
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.assertFalse
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
/**
* That `reset()` actually deletes — the wiring, not the tool.
*
* D2 was never that `OutputPublisher` could not delete a file. It was that "Start over"
* dropped the reference without calling anything. So this drives the real ViewModel through
* a real `WorkManager` to `Converted` and then asserts on the filesystem.
*/
@UnstableApi
@RunWith(RobolectricTestRunner::class)
class ConversionViewModelCleanupTest {
private lateinit var app: Application
private lateinit var publisher: RecordingPublisher
private lateinit var staged: File
@Before
fun setUp() {
app = RuntimeEnvironment.getApplication()
publisher = RecordingPublisher(app)
ConversionDependencies.publisher = { publisher }
// MediaProbe spawns FFprobe, whose loader throws a bare java.lang.Error with no
// native library present. Without this the pick dies before the test starts.
ConversionDependencies.probe = { _, _ -> InputProbe() }
staged = publisher.createStagingFile("holiday.mp4").apply { writeBytes(ByteArray(4096)) }
installTestWorkManager(app, workDataOf(ConversionWorker.KEY_OUTPUT_PATH to staged.absolutePath))
}
@After
fun tearDown() {
ConversionDependencies.reset()
}
@Test
fun `start over on a finished conversion deletes the staged file`() {
val viewModel = convertedViewModel()
assertTrue("the conversion should have produced a staged file", staged.exists())
viewModel.reset()
assertEquals(ConversionState.Idle, viewModel.state.value)
assertEquals("reset() should have discarded exactly the staged file", listOf(staged), publisher.discarded)
assertFalse("Start over must not leave a full-size copy in cache", staged.exists())
}
@Test
fun `reset after a successful save does not try to delete again`() {
val viewModel = convertedViewModel()
viewModel.save(DESTINATION)
awaitState(viewModel.state, "Saved") { it is ConversionState.Saved }
// save() deletes the staged file itself, through File.delete() rather than through
// the publisher, so assert the disappearance as well as the absent second discard.
assertFalse("a successful save should have removed the staged file", staged.exists())
viewModel.reset()
// Discarding again would be a delete aimed at a path this ViewModel no longer owns.
assertEquals(emptyList<File>(), publisher.discarded)
assertFalse(staged.exists())
}
@Test
fun `a failed save keeps the staged file, and a later reset collects it`() {
val viewModel = convertedViewModel()
publisher.publishFailure = IllegalStateException("destination volume full")
viewModel.save(DESTINATION)
awaitState(viewModel.state, "Failed") { it is ConversionState.Failed }
// The deliberate decision, pinned: the staged file may be the only copy of an hour
// of transcoding, and the destination did not receive it.
assertTrue("a failed save must not destroy the only copy", staged.exists())
assertEquals(emptyList<File>(), publisher.discarded)
// Failed carries no file reference at all, so this only works because the handle is
// a ViewModel field rather than something read back out of the state machine.
viewModel.reset()
assertEquals(listOf(staged), publisher.discarded)
assertFalse(staged.exists())
}
/** A ViewModel driven all the way to [ConversionState.Converted]. */
private fun convertedViewModel(): ConversionViewModel {
// Unconfined so reset()'s delete runs inline instead of on a real IO thread.
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 }
return viewModel
}
private companion object {
val DESTINATION: Uri = Uri.parse("content://test/destination.mp4")
}
}
@@ -0,0 +1,93 @@
package org.libremediaconverter.convert
import org.junit.Assert.assertFalse
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
/**
* What a pure function cannot say: the file is really gone.
*
* [StagingSweepTest] pins the rule; this pins the effect. Robolectric gives each test a
* real, empty `cacheDir` on a temp path, so this drives the actual [OutputPublisher] over
* the actual filesystem — the same calls `reset()` makes, without needing a ViewModel (both
* of those construct a `WorkManager`, which is not initialised on the JVM classpath).
*
* The instrumented suite cannot run on the development host, so this is the only place the
* "Start over leaks a full-size copy" defect can be caught before CI.
*/
@RunWith(RobolectricTestRunner::class)
class OutputPublisherStagingTest {
private lateinit var cacheDir: File
private lateinit var publisher: OutputPublisher
@Before
fun setUp() {
val context = RuntimeEnvironment.getApplication()
cacheDir = context.cacheDir
publisher = OutputPublisher(context)
}
@Test
fun `discarding a staged output actually removes it`() {
// This is the leak in D2: convert, decline to save, tap "Start over".
val staged = publisher.createStagingFile("holiday.mp4").apply { writeBytes(ByteArray(4096)) }
assertTrue("the staged file should exist to begin with", staged.exists())
assertTrue("discard should report that it deleted the file", publisher.discardStaged(staged))
assertFalse("the staged file should be gone after the reset path runs", staged.exists())
}
@Test
fun `discarding a file that is already gone is not an error`() {
// reset() after a successful save, or two resets in a row. Neither should throw.
val staged = publisher.createStagingFile("already_published.mp4")
assertFalse(publisher.discardStaged(staged))
}
@Test
fun `a file outside the staging directory is refused`() {
// The handle can originate in WorkInfo.outputData, which is a string the ViewModel
// turns straight into a File. Nothing else checks where it points.
val outsider = File(cacheDir, "someone_elses.bin").apply { writeBytes(ByteArray(16)) }
assertFalse(publisher.discardStaged(outsider))
assertTrue("a file outside staging must survive", outsider.exists())
}
@Test
fun `a path that climbs out of the staging directory is refused`() {
// The naive parent check -- comparing path strings -- passes this one.
val outsider = File(cacheDir, "climbed_to.bin").apply { writeBytes(ByteArray(16)) }
val escaping = File(cacheDir, "conversions/../climbed_to.bin")
assertFalse(publisher.discardStaged(escaping))
assertTrue("a traversal must not delete outside staging", outsider.exists())
}
@Test
fun `the sweep collects an orphan and leaves a live job alone`() {
val orphan = publisher.createStagingFile("orphan.mp4").apply { writeBytes(ByteArray(4096)) }
val liveOutput = publisher.createStagingFile("joined.mp4").apply { writeBytes(ByteArray(4096)) }
val liveList = publisher.createStagingFile("concat_list.txt").apply { writeText("file 'a.mp4'\n") }
assertTrue(orphan.setLastModified(System.currentTimeMillis() - StagingSweep.GRACE_PERIOD_MS - 60_000))
publisher.sweepStaging()
assertFalse("an abandoned output should be collected", orphan.exists())
assertTrue("a live job's output must survive", liveOutput.exists())
assertTrue("a live join's list file must survive", liveList.exists())
}
@Test
fun `the sweep tolerates a staging directory that does not exist yet`() {
File(cacheDir, "conversions").deleteRecursively()
publisher.sweepStaging()
}
}
@@ -0,0 +1,134 @@
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 kotlinx.coroutines.Dispatchers
import org.junit.After
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertTrue
import org.junit.Before
import org.junit.Test
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 java.io.File
/**
* Where the two staging fixes meet: a result nobody started is still a result somebody owns.
*
* Reattachment hands the user back a job this ViewModel did not start, which means the
* ViewModel inherits the staged file along with it — and "Start over" on that file has to
* delete it exactly as it would for a conversion run in this process. Neither fix implies
* the other: cleanup only reaches a file whose handle was recorded, and reattachment only
* records one because it goes through the same `observe()` the normal success path does.
* That is a structural claim about one function, which is precisely the kind that a merge
* resolving the two changes into different places would quietly break. Hence a test rather
* than a comment.
*
* These run a real `WorkManager` and a real `OutputPublisher` against a real `cacheDir`, so
* what is asserted at the end is the filesystem.
*/
@UnstableApi
@RunWith(RobolectricTestRunner::class)
class ReattachedCleanupTest {
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)
}
@After
fun tearDown() {
ConversionDependencies.reset()
}
/**
* The defect end to end, on the JVM: work that finished with no ViewModel left to see it,
* and a ViewModel created afterwards that finds it anyway.
*/
@Test
fun `a conversion that finished before this ViewModel existed is picked up`() {
finishAConversionWithNobodyWatching()
val viewModel = ConversionViewModel(app, Dispatchers.Unconfined)
val converted = awaitState(viewModel.state, "Converted") { it is ConversionState.Converted }
converted as ConversionState.Converted
assertEquals(staged.absolutePath, converted.staged.absolutePath)
// The name came back through the job's tags — the only channel WorkManager returns,
// since WorkInfo never carries the Data a request was enqueued with.
assertEquals("holiday.mp4", converted.input.displayName)
assertEquals(4_096L, converted.input.sizeBytes)
}
@Test
fun `start over on a reattached conversion deletes the staged file`() {
finishAConversionWithNobodyWatching()
val viewModel = ConversionViewModel(app, Dispatchers.Unconfined)
awaitState(viewModel.state, "Converted") { it is ConversionState.Converted }
assertTrue("the reattached result should still be on disk", staged.exists())
viewModel.reset()
assertEquals(ConversionState.Idle, viewModel.state.value)
assertEquals(
"a reattached result is still this ViewModel's to discard",
listOf(staged),
publisher.discarded,
)
assertFalse("Start over must not leave a full-size copy in cache", staged.exists())
}
@Test
fun `start over on a reattached join deletes the staged file`() {
workManager.enqueue(
ConcatWorker.request(
inputs = listOf(Uri.parse("content://test/one.mp4"), Uri.parse("content://test/two.mp4")),
totalBytes = 8_192L,
),
).result.get()
val viewModel = JoinViewModel(app, Dispatchers.Unconfined)
awaitState(viewModel.state, "Joined") { it is JoinState.Joined }
viewModel.reset()
assertEquals(listOf(staged), publisher.discarded)
assertFalse(staged.exists())
}
/**
* A job from a process that is gone: enqueued and finished before any ViewModel exists, so
* nothing observed it and nothing recorded its output.
*/
private fun finishAConversionWithNobodyWatching() {
workManager.enqueue(
ConversionWorker.request(
inputUri = Uri.parse("content://test/holiday.mp4"),
displayName = "holiday.mp4",
sizeBytes = 4_096L,
),
).result.get()
}
}
@@ -0,0 +1,106 @@
package org.libremediaconverter.convert
import android.content.Context
import android.net.Uri
import android.os.Looper
import android.util.Log
import androidx.work.Configuration
import androidx.work.Data
import androidx.work.ListenableWorker
import androidx.work.Worker
import androidx.work.WorkerFactory
import androidx.work.WorkerParameters
import androidx.work.testing.SynchronousExecutor
import androidx.work.testing.WorkManagerTestInitHelper
import kotlinx.coroutines.flow.StateFlow
import org.robolectric.Shadows.shadowOf
import java.io.File
import java.util.concurrent.TimeUnit
/**
* Shared scaffolding for the two ViewModel cleanup tests.
*
* These tests exist because [StagingSweepTest] and [OutputPublisherStagingTest] both prove
* the *tool* works while saying nothing about whether anything calls it — and the wiring is
* where D2 actually lived. Deleting the `discardStaged` line from either `reset()` left all
* of the earlier tests green.
*/
/**
* A real [OutputPublisher] that records what it was asked to discard.
*
* It still really deletes, so the assertions are about the filesystem rather than about a
* mock's memory. [publish] is stubbed because the SAF destination is not what these tests
* are about, and because making it throw is the only way to reach the failed-save branch
* deterministically.
*/
open class RecordingPublisher(context: Context) : OutputPublisher(context) {
val discarded = mutableListOf<File>()
/** When set, [publish] throws it — the failed-save path. */
var publishFailure: Throwable? = null
override fun publish(staged: File, destination: Uri) {
publishFailure?.let { throw it }
}
override fun discardStaged(staged: File): Boolean {
discarded += staged
return super.discardStaged(staged)
}
}
/**
* Stands in for whichever worker is enqueued and succeeds immediately with [outputData].
*
* The real workers cannot run here: both drive FFmpeg or Media3 through native libraries
* that do not exist on the JVM. What the ViewModel actually needs from them is one
* `SUCCEEDED` `WorkInfo` carrying an output path, and that is exactly what this produces —
* through a real `WorkManager`, so the ViewModel's own observer, its `SUCCEEDED` branch and
* its cleanup handle are all the production ones.
*/
class SucceedingWorkerFactory(private val outputData: Data) : WorkerFactory() {
override fun createWorker(
appContext: Context,
workerClassName: String,
workerParameters: WorkerParameters,
): ListenableWorker = object : Worker(appContext, workerParameters) {
override fun doWork(): Result = Result.success(outputData)
}
}
/** Installs a synchronous test WorkManager whose workers succeed with [outputData]. */
fun installTestWorkManager(context: Context, outputData: Data) {
WorkManagerTestInitHelper.initializeTestWorkManager(
context,
Configuration.Builder()
.setMinimumLoggingLevel(Log.ASSERT)
.setExecutor(SynchronousExecutor())
.setTaskExecutor(SynchronousExecutor())
.setWorkerFactory(SucceedingWorkerFactory(outputData))
.build(),
)
}
/**
* Waits for [predicate] to hold, pumping the main looper as it goes.
*
* Both ViewModels hop to a real `Dispatchers.IO` for file metadata and resume on the main
* looper, which Robolectric leaves paused. So neither a bare read of `state.value` nor a
* single `idle()` is enough, and the timeout is generous because it only has to be longer
* than a few file stats — in practice this converges in milliseconds.
*/
fun <T> awaitState(state: StateFlow<T>, description: String, predicate: (T) -> Boolean): T {
val deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(AWAIT_TIMEOUT_SECONDS)
while (System.nanoTime() < deadline) {
shadowOf(Looper.getMainLooper()).idle()
val current = state.value
if (predicate(current)) return current
Thread.sleep(POLL_INTERVAL_MS)
}
throw AssertionError("Timed out waiting for $description; state was ${state.value}")
}
private const val AWAIT_TIMEOUT_SECONDS = 10L
private const val POLL_INTERVAL_MS = 5L
@@ -0,0 +1,77 @@
package org.libremediaconverter.convert
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertTrue
import org.junit.Test
/**
* The orphan-collection rule.
*
* Timestamps are passed in as values rather than read off real files on purpose. The
* interesting part of this decision is clock arithmetic — the boundary, and a clock that
* has moved backwards — and a test that created real files would be measuring the
* filesystem's mtime granularity instead of the rule.
*/
class StagingSweepTest {
private val now = 1_700_000_000_000L
private val grace = StagingSweep.GRACE_PERIOD_MS
@Test
fun `an orphan older than the grace period is collectable`() {
// Left behind by a process that died, or by a "Start over" whose delete never ran.
val entries = listOf(StagingSweep.Entry("orphan.mp4", now - grace - 1))
assertEquals(listOf("orphan.mp4"), StagingSweep.collectable(entries, now))
}
@Test
fun `a file written moments ago is left alone`() {
// The in-flight guard. A live job's output has its mtime refreshed by every write,
// so a running conversion always looks young; deleting it would destroy the job.
val entries = listOf(StagingSweep.Entry("in_progress.mp4", now - 1_000))
assertEquals(emptyList<String>(), StagingSweep.collectable(entries, now))
}
@Test
fun `the grace boundary itself collects`() {
// Pins the comparison: age >= grace collects, age one millisecond short does not.
assertTrue(StagingSweep.isCollectable(lastModifiedMs = now - grace, nowMs = now))
assertFalse(StagingSweep.isCollectable(lastModifiedMs = now - grace + 1, nowMs = now))
}
@Test
fun `a file dated in the future is left alone`() {
// The clock moved backwards — an RTC correction, or the user setting the date. The
// age is negative, which says nothing about whether the file is still in use, so
// the safe answer is to keep it and let a later sweep decide.
val entries = listOf(StagingSweep.Entry("tomorrow.mp4", now + grace))
assertEquals(emptyList<String>(), StagingSweep.collectable(entries, now))
}
@Test
fun `an empty directory yields nothing`() {
assertEquals(emptyList<String>(), StagingSweep.collectable(emptyList(), now))
}
@Test
fun `a mixed directory names only the orphans`() {
// The whole point of narrowing the old clearStaging(): a sweep that runs while a
// join is live must not take the list file out from under it.
val entries = listOf(
StagingSweep.Entry("orphan.mp4", now - grace - 1),
StagingSweep.Entry("concat_list.txt", now - 5_000),
StagingSweep.Entry("joined.mp4", now - 5_000),
StagingSweep.Entry("older_orphan.webm", now - grace * 7),
)
assertEquals(listOf("orphan.mp4", "older_orphan.webm"), StagingSweep.collectable(entries, now))
}
@Test
fun `a shorter grace period can be asked for explicitly`() {
// The caller owns the period; the constant is only a default.
val entries = listOf(StagingSweep.Entry("recent.mp4", now - 60_000))
assertEquals(emptyList<String>(), StagingSweep.collectable(entries, now))
assertEquals(listOf("recent.mp4"), StagingSweep.collectable(entries, now, gracePeriodMs = 30_000))
}
}
@@ -0,0 +1,113 @@
package org.libremediaconverter.join
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.assertFalse
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.RecordingPublisher
import org.libremediaconverter.convert.awaitState
import org.libremediaconverter.convert.installTestWorkManager
import org.libremediaconverter.work.ConcatWorker
import org.robolectric.RobolectricTestRunner
import org.robolectric.RuntimeEnvironment
import java.io.File
/**
* The join tab's half of the same defect.
*
* Carried separately rather than parameterised with the convert tab, because the two are
* independent ViewModels that can each hold a staged file at the same time — the reason
* `clearStaging()` could not simply be wired up.
*/
@UnstableApi
@RunWith(RobolectricTestRunner::class)
class JoinViewModelCleanupTest {
private lateinit var app: Application
private lateinit var publisher: RecordingPublisher
private lateinit var staged: File
@Before
fun setUp() {
app = RuntimeEnvironment.getApplication()
publisher = RecordingPublisher(app)
ConversionDependencies.publisher = { publisher }
staged = publisher.createStagingFile("joined.mp4").apply { writeBytes(ByteArray(4096)) }
installTestWorkManager(app, workDataOf(ConcatWorker.KEY_OUTPUT_PATH to staged.absolutePath))
}
@After
fun tearDown() {
ConversionDependencies.reset()
}
@Test
fun `start over on a finished join deletes the staged file`() {
val viewModel = joinedViewModel()
assertTrue("the join should have produced a staged file", staged.exists())
viewModel.reset()
assertEquals(JoinState.Idle, viewModel.state.value)
assertEquals(listOf(staged), publisher.discarded)
assertFalse("Start over must not leave a full-size copy in cache", staged.exists())
}
@Test
fun `reset after a successful save does not try to delete again`() {
val viewModel = joinedViewModel()
viewModel.save(DESTINATION)
awaitState(viewModel.state, "Saved") { it is JoinState.Saved }
// save() deletes the staged file itself, through File.delete() rather than through
// the publisher, so assert the disappearance as well as the absent second discard.
assertFalse("a successful save should have removed the staged file", staged.exists())
viewModel.reset()
assertEquals(emptyList<File>(), publisher.discarded)
assertFalse(staged.exists())
}
@Test
fun `a failed save keeps the staged file, and a later reset collects it`() {
val viewModel = joinedViewModel()
publisher.publishFailure = IllegalStateException("destination volume full")
viewModel.save(DESTINATION)
awaitState(viewModel.state, "Failed") { it is JoinState.Failed }
assertTrue("a failed save must not destroy the only copy", staged.exists())
assertEquals(emptyList<File>(), publisher.discarded)
viewModel.reset()
assertEquals(listOf(staged), publisher.discarded)
assertFalse(staged.exists())
}
/** A ViewModel driven all the way to [JoinState.Joined]. */
private fun joinedViewModel(): JoinViewModel {
// Unconfined so reset()'s delete runs inline instead of on a real IO thread.
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 }
return viewModel
}
private companion object {
val DESTINATION: Uri = Uri.parse("content://test/destination.mp4")
}
}
@@ -0,0 +1,12 @@
# Robolectric defaults to the manifest's targetSdk, which is 37 here, and there is no
# android-all jar for 37 -- Robolectric 4.16.1 stops at 36 and fails the whole class with
# "Package targetSdkVersion=37 > maxSdkVersion=36" before any test body runs.
#
# 36 is where CI's emulator matrix already stops, for the unrelated reason in
# docs/api-37-emulator-crash.md, so this does not widen the gap between what is verified
# automatically and what is not: API 37 was already a manual check on the Pixel 10 Pro XL
# before each release, and still is.
#
# Set here rather than in a @Config on each class so a later Robolectric test does not have
# to rediscover it. Remove it once Robolectric ships an android-all jar for 37.
sdk=36
+19
View File
@@ -67,6 +67,19 @@ detekt = "2.0.0-alpha.6"
# so the agent version that reads Kotlin 2.2.10 bytecode is stated, not implied.
jacoco = "0.8.15"
# Robolectric. PINNED, and it belongs with ktlint/detekt/jacoco above rather than with
# the floating libraries, for two reasons that both point the same way.
#
# First, the prerelease guard in app/build.gradle.kts only covers the groups this project
# floats -- "androidx.", "junit", "com.arthenica" -- so org.robolectric is unguarded, and
# a "4.+" here would resolve straight to 4.17-beta-3, which is the newest thing published.
# Second, Robolectric is not a library the app ships: it is the JVM's Android runtime, and
# a version bump changes which android-all jar the tests execute against. That is the same
# "a tool moved under a diff that cannot explain it" failure the linters are pinned for.
#
# 4.16.1 is the newest RELEASED version; the 4.17 line is beta-only at the time of writing.
robolectric = "4.16.1"
[libraries]
androidx-core-ktx = { group = "androidx.core", name = "core-ktx", version.ref = "coreKtx" }
androidx-activity-compose = { group = "androidx.activity", name = "activity-compose", version.ref = "activityCompose" }
@@ -115,6 +128,12 @@ junit = { group = "junit", name = "junit", version.ref = "junit" }
androidx-junit = { group = "androidx.test.ext", name = "junit", version.ref = "androidxJunit" }
androidx-espresso-core = { group = "androidx.test.espresso", name = "espresso-core", version.ref = "espressoCore" }
# Robolectric — an Android runtime for the JVM test source set, so file-lifecycle behaviour
# that needs a real Context can be verified without a device. The instrumented suite cannot
# run on the development host at all (see CLAUDE.md), so an androidTest-only red test is not
# a TDD loop anyone here can execute.
robolectric = { group = "org.robolectric", name = "robolectric", version.ref = "robolectric" }
[plugins]
# com.android.application and org.jetbrains.kotlin.plugin.compose are deliberately absent.
# They come from the root buildscript classpath (see build.gradle.kts) so that a newer KGP