Delete staged output the user never saved, instead of waiting for the OS

Every conversion writes a full-size file into <cacheDir>/conversions/. save()
published it and deleted it, but reset() -- what the "Start over" button on the
Converted and Joined states calls -- dropped the File reference and left the file
behind. Converting something and deciding not to save it is an ordinary path
through the UI, so it leaked a full-size copy every time. cacheDir is evictable,
so this was never unbounded growth; it was the app relying on the OS to clean up
after it, and on a device under no storage pressure "eventually" means never.

OutputPublisher.clearStaging() was written for exactly this and called from
nowhere. It is NOT wired up here -- it is deleted. It emptied the directory
unconditionally, and the convert tab, the join tab and ConcatEngine's
concat_list.txt all share that directory with no per-job namespacing (D8), so a
blanket delete could take a file out from under a running job. Two narrower
methods replace it:

  discardStaged(file)  one file, guarded. The handle reaches the ViewModel as a
                       path string in WorkInfo.outputData and becomes a File with
                       nothing checking where it points, so this compares the
                       CANONICAL parent against the staging dir -- the naive
                       string comparison accepts conversions/../elsewhere.

  sweepStaging(now)    age-based, for orphans no ViewModel is left to clean up.

The cleanup handle is a ViewModel field, not something read back out of the state
machine, because the state machine cannot answer it on the path that needs it
most: a failed save lands on Failed(message), which carries no file reference at
all. On that path the file is deliberately kept -- it may be the only copy of an
hour of transcoding and the destination did not receive it, so deleting to tidy a
cache directory would destroy the work. It stays collectable by a later reset()
or by the sweep.

The sweep runs once per process from a new Application subclass, off the main
thread. The reason it cannot race a live job is the grace period, not ordering:
WorkManager initialises through androidx.startup's InitializationProvider, a
ContentProvider, so it is already up before onCreate() and can be resuming a
worker in this same process while the sweep runs. StagingSweep only collects a
file nothing has written to for 24 hours. Outputs are written continuously and
keep their own mtime fresh; concat_list.txt is the one file written once and then
only read, and a WorkManager attempt is capped by the six-hour foreground-service
budget with retries restarting doWork() from the top, so no attempt can hold a
file still for a day. sweepStaging() also re-reads each timestamp immediately
before deleting, closing the window between listing the directory and acting on
the list -- unlinking an inode a running job still holds open would end with the
job reporting success for a path that no longer exists.

The rule itself is a pure function over (name, lastModifiedMs) pairs and a clock.
Timestamps are values rather than Files so the tests measure the arithmetic --
the grace boundary, and a clock that moved backwards -- rather than the
filesystem's mtime granularity.

Tests cover the tool AND the wiring, because the wiring is where the defect was.
A pure rule test and a Robolectric test of OutputPublisher both stay green when
the discardStaged call is deleted from reset(), which would have made the number
read as coverage of a bug that was still there. So both ViewModels are driven --
through a real WorkManager, to Converted/Joined -- and then asserted on the
filesystem: Start over deletes the staged file; a successful save leaves nothing
to delete twice; a failed save keeps the file and a later reset collects it, which
pins the argued decision above rather than leaving it as a comment. Verified by
deleting the discardStaged line from both reset() methods: 4 of the 6 fail with
"reset() should have discarded exactly the staged file expected:<[...]> but
was:<[]>", and the two save-path tests correctly stay green.

Three things made that reachable, all reusing what was already here:

  - Both ViewModels now resolve their publisher through ConversionDependencies,
    like the workers already did. They were the only place bypassing the seam.
  - MediaProbe joins that seam too. It spawns FFprobe, and FFmpegKit's loader
    throws a bare java.lang.Error when the native library is absent -- which its
    own `catch (e: Exception)` cannot catch, so every JVM test died on the file
    pick. Instrumented tests are unaffected and still get the real probe.

    That error path is a LATENT PRODUCTION HAZARD, recorded in the KDoc and
    deliberately not fixed here: onInputPicked does not catch it either, so a
    missing .so would surface as an uncaught error rather than the "could not read
    this file" the code was written to give. It cannot fire on a device that ships
    the libraries, so widening MediaProbe's catch to Throwable would change the
    pick path on the strength of a condition no user meets. Its own commit.
  - reset()'s cleanup dispatcher is a constructor parameter defaulting to
    Dispatchers.IO, which makes the delete assertable and states the ordering --
    Idle is published synchronously, the delete is dispatched -- as a decision
    rather than an accident. @JvmOverloads keeps the single-argument constructor
    that viewModel()'s AndroidViewModelFactory looks up reflectively.

androidx-work-testing was already in the catalog and already inside the prerelease
guard via its androidx. group, so it needed no new pinning argument.

Robolectric is added for the one assertion no pure function can make: that the
file is really gone from a real cacheDir. It is PINNED at 4.16.1 and belongs with
ktlint/detekt/jacoco rather than the floating libraries. The prerelease guard in
app/build.gradle.kts only covers androidx., junit and com.arthenica, so
org.robolectric is unguarded and a "4.+" would resolve to 4.17-beta-3; beyond
that, a bump changes which android-all jar the tests execute against, which is the
same "a tool moved under a diff that cannot explain it" failure the linters are
pinned for.

Two things Robolectric needed. testOptions did not exist in this module at all;
it now sets isIncludeAndroidResources so the merged manifest and resource table
reach the JVM tests, and grants --enable-native-access, which Java 25 otherwise
warns about four times per run when Robolectric's native runtime calls
System.load(). And robolectric.properties pins sdk=36: Robolectric defaults to the
manifest's targetSdk of 37, there is no android-all jar for 37, and the class
fails to initialise before any test body runs. 36 is where CI's emulator matrix
already stops, so this does not widen the gap -- API 37 was already a manual check
on the Pixel 10 Pro XL before each release.

Known and left alone: a conversion that fails inside the worker never reaches
Converted, so pendingStaged is never set and any partial output relies on the
sweep alone. reset()'s delete is also fire-and-forget on viewModelScope, so it is
cancelled if the Activity finishes first. The sweep is the backstop for both.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
2026-08-22 18:40:29 -05:00
co-authored by Claude Opus 5
parent 903b43c30a
commit cfd705af05
15 changed files with 865 additions and 10 deletions
@@ -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,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")
}
}