2a68f0313422cb49d31bbbc7fe74dedbaf0ad549
6
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  JMR-devandClaude Opus 5
|
2a68f03134 |
Give every job a staging path of its own
`<cacheDir>/conversions/` is shared by the convert tab, the join tab and `ConcatEngine`, and
until now none of the three named a file that belonged to one job. A conversion derived its name
from the input's display name, so two `holiday.mp4` from different folders wrote the same file.
A join used the constant `joined.<ext>`, so any two joins of one format did. The list file was
the constant `concat_list.txt`, so any two joins at all did, and one of them would read the
other's input list.
The naming half is not a hypothesis. Two independent conversions on a Pixel each computed
`cache/conversions/input_converted.mp4`, the second silently overwrote the first, and a tag query
in a fresh process then returned **two SUCCEEDED `WorkInfo`s naming that one file** with one file
on disk. That is the collision reaching the point where it makes a *fix* ambiguous rather than
just a file: `Reattachment` can offer the bytes, because they are the user's either way, but it
cannot say which job produced them.
`StagingNames` keys the name on the WorkManager request id. That id is what stays still across a
retry -- `WorkerWrapper` builds `WorkerParameters` from the `WorkSpec` id and only increments
`runAttemptCount` -- which matters more here than uniqueness does, and matters more since the
previous commit made retries routine. A failed attempt deletes its staged file on the way out,
and that only collects the partial the *previous* attempt left when the name has not moved.
Opaque rather than sanitised, deliberately. The staged name is never shown to anyone: `save()`
recomputes a suggested name and the user picks the real one in the SAF dialog. So there was
nothing to lose by dropping the display name, and something to gain -- a provider-supplied
display name can contain a separator, be empty, or be four kilobytes long, and `File(stagingDir,
"../escape_converted.mp4")` resolves to a path outside staging. That was reachable before this
commit and is now unreachable by construction rather than by a sanitiser that has to be right
about every case. There is a test for exactly that name.
The extension stays, and is not decoration: `FFmpegConcatCommand` names no output muxer, so
FFmpeg infers it from the output path. A fully opaque name would quietly produce the wrong
container.
`ConcatEngine`'s list file is derived from the output it belongs to rather than taking another
parameter, so the two cannot drift apart, a directory listing shows which list belongs to which
join, and the sweep ages them together.
Three neighbouring comments claimed things that are no longer true, and are corrected rather than
left to mislead the next reader:
- `Reattachment.Ambiguous` said it "resolves on its own once each job stages under a name of
its own". It now does -- for work enqueued from here on. The case is **kept**, because the
queue outlives the change: WorkManager holds finished work for about a week, and the jobs
likeliest to be sitting in it when this code first runs are the ones named the old way.
Behaviour is unchanged and `ReattachmentTest` is untouched.
- `OutputPublisher.sweepStaging` justified its age rule partly on there being "no per-job
namespacing". There is now, and the rule still stands on its own: per-job names stop two jobs
from sharing a file, and say nothing about whether a file's job is still running, which is the
question a sweep actually asks. Same for `StagingSweep` and the note in
`LibreMediaConverterApp`.
- Both ViewModels' `reattach()` explained aliasing as something nothing prevented. Narrowed to
what is still true of work already in the queue.
`ConcatEngineTest` asks `StagingNames` for the list file's name instead of spelling out
`concat_list.txt`. That is the difference between a test and a tautology: a literal there would
have gone on passing after the rename while asserting that a file nothing creates does not exist.
The same trap was live in the two worker tests from the previous commits, whose staged-file
assertions computed a path of their own -- they now assert on the staging directory being empty,
which cannot go vacuous when a name moves.
`PerJobStagingTest` drives the real worker, because the naming function was never the part that
was wrong: what was wrong was which name the worker asked for. Two jobs converting one file must
leave two files; a second attempt at one job must not leave a second; and a display name that
climbs out of staging must not. The first and third fail before the change with "each job must
have staged its own file, found [input_converted.mp4] expected:<2> but was:<1>" and "the output
belongs in staging expected:<1> but was:<0>" -- the latter because the file had landed in
`cacheDir` instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-dev
|
eb37f9ebea |
Merge branch 'fix/reattach-unfinished-work' into scratch/integrate-d2-d3
# Conflicts: # app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt |
||
|
JMR-devandClaude Opus 5
|
ec969c41dc |
Reattach to conversions and joins the ViewModel did not start
The queue surviving process death is the stated reason this app uses WorkManager, and
the ViewModel was where that protection stopped. `activeWorkId` and `observer` are
plain fields, so a process reclaimed after a conversion finished came back to Idle
while the output sat in `cacheDir` with nothing in the UI able to reach it. The
realistic window is not a crash mid-transcode -- it is the job finishing, the user not
saving yet, and the process being reclaimed hours later as an ordinary background one.
Both ViewModels now query their own worker's class name on init and pick up what they
find. Nothing is persisted for it, and nothing needed to be: `WorkRequest.Builder`
seeds every request's tag set with `workerClass.name` (`tags = mutableSetOf(
workerClass.name)`, work-runtime 2.11.2), and R8 keeps those names through
work-runtime's own consumer rule, `-keepnames class * extends
androidx.work.ListenableWorker`. A UUID in a `SavedStateHandle` would have been both
more machinery and less: it cannot find work enqueued by a previous install.
What the query cannot return is the job's input. `WorkInfo` hands back id, state, tags,
progress, output and run-attempt count -- never the `Data` a request was enqueued with
-- so a ViewModel could see that a conversion existed and where its output went, but
not which file it was converting. The display name and size therefore ride on tags too,
which is the whole of the production change to the request builders. The picked `Uri`
deliberately does not: nothing in a reattached state reads it, and a `content://` grant
taken by a picker in a process that no longer exists is not something to hand back as
though it still worked.
The decision is a pure function on the JVM test stack, following `FailureOutcome`:
`Reattachment.choose` takes what WorkManager reported and answers which job, if any.
Cancelled work is excluded -- the user already said no. Failed work is excluded, which
matters more than it looks now that a device has shown an interrupted worker coming
back FAILED rather than retried, its restart's `setForeground` refused as a background
foreground-service start: nothing marks a failure as seen, so it would otherwise
reappear on every launch. A success whose staged file is gone is excluded, because a
Save button that fails on tap is worse than no button. Live work outranks a finished
result, since a running job holds a foreground notification and someone opening the app
while that notification is in the shade is looking for that conversion.
Where several jobs qualify, the newest staged file wins, and that is not a detail.
Losing a tie is not the same as waiting for the next launch: the tag query has no
`ORDER BY`, so its order is unspecified but stable, and an arbitrary winner would keep
winning every launch while the other result stayed unreachable for as long as its file
existed. It is reachable today -- dismiss one result with "Start over", which leaves its
file behind, then convert something else and do not save it. `WorkInfo` carries no
timestamp of any kind, but the edge is already stat'ing the file, so the file's own
mtime is the ordering. A clock that moves backwards makes it a heuristic; an order that
is unspecified and repeats itself is worse.
Aliases are the tie that does not resolve, and that case is not hypothetical. A tag
query on a device returned two SUCCEEDED jobs whose output paths were both
`.../conversions/input_converted.mp4`, with one file on disk -- the staging name is
derived from the input's display name, so a later job overwrites an earlier one's output
and both go on reporting it. Checking the file does not separate them, and neither does
its mtime, since they share it. So the two halves are separated instead. The file is
offered, because it is the user's file either way and losing it is the defect being
fixed; the label is not, because saying which job produced it would be a guess.
`Reattachment.Ambiguous` says so and the card falls back to a neutral name rather than
borrowing the other job's. Aliases whose tags are identical -- the ordinary case, the
same file converted twice -- stay attributed, since nothing turns on which wrote it.
Age is not filtered on, and cannot be: WorkManager keeps finished work about a week and
prunes on its own schedule, so a query in a fresh process routinely returns jobs from
earlier sessions. Whether the staged file is still there is the only signal separating a
result still worth offering from one already dealt with, which is why that check carries
the weight.
Two smaller behaviours fall out of reattaching rather than starting:
- A reattached job that is cancelled lands on Idle rather than Ready. Ready would put
a Convert button over an input URI that belongs to a dead process. `observe()` takes
the cancelled destination as a defaulted parameter, so a job started here is
unchanged.
- A FAILED job's message falls back when blank, not only when absent. A worker killed
before it can report leaves no output data at all, and an exception's message can be
the empty string; both used to reach the screen as a failure with nothing said.
Deliberately not fixed here, each being its own change: the save dialog's suggested name
and MIME still come from the current picker rather than from the job that ran, so a
reattached job in a non-default format is offered the default extension; a result
dismissed with "Start over" still keeps its staged file, so it can be offered again next
launch -- the file check closes that for free once the file is deleted; and
`setForeground` still sits outside `doWork`'s try, so its throw bypasses the retry
decision entirely.
Tested where it can be. 28 JVM tests cover the choice and the tag round trip, including
the ordering, the aliasing rules and a display name that looks like another tag. The
reattachment itself is instrumented: a ViewModel constructed against the real
WorkManager is the next launch, with no memory of the work. `WorkManagerTestInitHelper`
is deliberately not used -- its `setDelegate` replaces the singleton for the whole
process, which would quietly turn `ConversionWorkerTest` into a synchronous test double
depending on class order.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
cfd705af05 |
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>
|
||
|
JMR-devandClaude Opus 5
|
496f1c7e73 |
Apply ktlintFormat
Tool output only, no hand edits, so this is safe to read with whitespace diffing off. It is its own commit for exactly that reason: a whole-repo reformat folded into the commit that configured the formatter would have made both unreviewable. What it did, mostly: trailing commas on wrapped argument lists, signatures collapsed onto one line where they fit inside 120 columns, import order, and four genuinely unused imports removed. Unit tests pass unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
dd2fcf0a19 |
Rename the project to LibreMediaConverter
Done now rather than later: the application ID is permanent once published -- Play treats a change as an entirely different app -- so this is the last cheap moment to choose it. applicationId / namespace dev.jasonmross.mediaconverter -> org.libremediaconverter source tree java/dev/jasonmross/mediaconverter -> java/org/libremediaconverter gradle project AndroidMediaConverter -> LibreMediaConverter theme Theme.MediaConverter -> Theme.LibreMediaConverter compose theme MediaConverterTheme -> LibreMediaConverterTheme display name "Media Converter" -> "LibreMediaConverter" org.* rather than dev.jasonmross.* because "Libre" signals a project rather than a personal app, and a project-owned namespace lets maintainership move later without the identifier contradicting reality. The source trees moved with git mv so history follows the files instead of showing 42 deletions beside 42 additions. Verified after the rename: 66 unit tests, and 40 instrumented tests on an API 36 emulator, 0 failures. The built APK reports org.libremediaconverter, and no stale jasonmross, AndroidMediaConverter or MediaConverterTheme identifiers remain anywhere in the tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |