Test the edge that feeds reattachment, and stop it reporting ENOENT

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

+    /**
+     * Copies the staged result out to the destination the user picked.
+     *
+     * The existence check is not redundant with the one reattachment already made. That one ran
+     * inside a tag query which, for a result offered on launch, can be hours older than the tap —
+     * and `cacheDir` is exactly the directory the OS empties when it wants space, which is also
+     * what the sweep does to anything a day old. Without it the file's absence arrived as
+     * `staged.inputStream()` throwing, and `e.message` put a raw ENOENT path on screen.
+     */
    fun save(destination: Uri) {
        val converted = _state.value as? ConversionState.Converted ?: return
+        if (!converted.staged.isFile) {
+            _state.value = ConversionState.Failed(STAGED_FILE_GONE_MESSAGE)
+            return
+        }
        viewModelScope.launch {
            runCatching {
                withContext(Dispatchers.IO) {
@@ -6,6 +6,23 @@ import android.provider.DocumentsContract
 import android.provider.OpenableColumns
 import java.io.File

+/**
+ * What a save has to say when the staged file is not there any more.
+ *
+ * Reachable without anything going wrong: staging lives in `cacheDir`, which the OS reclaims
+ * whenever it wants the space, and [sweepStaging] collects anything a day old. A result offered by
+ * reattachment is the likeliest to meet it — the check that decided the file existed ran during a
+ * tag query that can be hours old by the time the Save button is tapped.
+ *
+ * A written sentence rather than the exception's message, which is what used to reach the screen:
+ * `/data/user/0/org.libremediaconverter/cache/conversions/4b4882….mp4: open failed: ENOENT (No such
+ * file or directory)` is a true statement about a path the user has never seen and cannot act on.
+ * Kept next to [OutputPublisher] because both ViewModels need it and staging is what it is about.
+ */
+const val STAGED_FILE_GONE_MESSAGE: String =
+    "The finished file is no longer in the cache, so there is nothing left to save. " +
+        "Start over to make it again."
+
 /**
 * Staging and publication of conversion output.
 *
@@ -18,6 +18,7 @@ import kotlinx.coroutines.withContext
 import org.libremediaconverter.convert.ConversionDependencies
 import org.libremediaconverter.convert.InputFile
 import org.libremediaconverter.convert.InputQuery
+import org.libremediaconverter.convert.STAGED_FILE_GONE_MESSAGE
 import org.libremediaconverter.model.ConcatStrategy
 import org.libremediaconverter.work.ConcatWorker
 import org.libremediaconverter.work.JobTags
@@ -221,8 +222,20 @@ class JoinViewModel @JvmOverloads constructor(
        activeWorkId?.let(workManager::cancelWorkById)
    }

+    /**
+     * Copies the staged result out to the destination the user picked.
+     *
+     * The existence check is the same one `ConversionViewModel.save` makes, for the same reason: a
+     * join offered by reattachment was last seen during a tag query that may be hours old, and
+     * `cacheDir` is reclaimed by the OS and swept by this app. Without it the file's absence
+     * reached the screen as a raw ENOENT path.
+     */
    fun save(destination: Uri) {
        val joined = _state.value as? JoinState.Joined ?: return
+        if (!joined.staged.isFile) {
+            _state.value = JoinState.Failed(STAGED_FILE_GONE_MESSAGE)
+            return
+        }
        viewModelScope.launch {
            runCatching {
                withContext(Dispatchers.IO) {
@@ -104,10 +104,13 @@ sealed interface Reattachment {
         * result still worth offering from one already dealt with, which is why that check
         * carries the weight here.
         *
-         * It is also the seam for a neighbouring defect: a result the user dismissed with "Start
-         * over" currently keeps its staged file, so today it can be offered again on the next
-         * launch. Nothing here changes when that is fixed — the file stops existing and the job
-         * stops qualifying.
+         * It is also the seam a neighbouring fix acts through. "Start over" deletes the staged
+         * file, so a result the user dismissed stops qualifying here without this rule needing to
+         * know that happened — the file stops existing and the job falls out. What survives is the
+         * narrower gap that delete cannot close: `reset()` dispatches it to
+         * [kotlinx.coroutines.Dispatchers.IO] and it is cancelled with the Activity, so a
+         * dismissal on the way out of the app can leave the file behind. That is what
+         * `OutputPublisher.sweepStaging` is for, and its own KDoc names this case.
         *
         * Ranked, when more than one qualifies:
         *