docs/coverage-wave2
282
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  JMR-devandClaude Opus 5
|
da6f2807e9 |
Stop an interrupted sweep leaking the emulator, the AVD and the port
Four corrections to the harness, none of which changes what a successful sweep does. R17 / #26 -- no trap. Ctrl-C during a sweep (now up to five boots long) left headless qemu on console port 5560 and an lmc_e2e_apiNN AVD behind. The next run's `emulator -port` then collides with the orphan and `emu_adb` can resolve to it -- on a workstation with the Pixel plugged in, exactly the ambiguity the ANDROID_SERIAL pinning exists to prevent. `cleanup` (stop_emulator + delete_created_avds, KEEP_AVD honoured) is now on EXIT, INT and TERM. It is idempotent and the normal path calls it explicitly before the summary, so cleanup output cannot land after the summary and the EXIT trap finds nothing to redo. The interrupt path passes a 6-second grace rather than 30: Ctrl-C has already reached the emulator through the foreground process group, so that wait is only for it to finish writing, and `kill -9` follows regardless. `exit "$overall"` stays the last line, so the exit code an EXIT trap could have swallowed is still the one that escapes. The emulator logs in $LOG_DIR are deliberately kept -- they are the only evidence a failed boot leaves. R31 / #40 -- `kill -9 "${EMU_PID:-0}"`. EMU_PID is empty, not unset, if the background launch never produced a job, so `:-0` converted "nothing to kill" into pid 0, which POSIX reads as the sender's whole process group. The `kill -0` wait loop had the same shape and would have spent its full grace period testing the group. All three sites now take a bare `$EMU_PID` behind one `[ -n ... ] || return 0` guard. boot_emulator's own `kill -0` is left alone: it runs only after the assignment and cannot reach the group form. R33 / #42 -- ensure_avd wrote CI's RAM and disk pins to a hardcoded $HOME/.android/avd/... path and checked nothing. With ANDROID_AVD_HOME (or ANDROID_USER_HOME, or ANDROID_SDK_HOME) set, the sed failed and the level ran on at default RAM and userdata, which surfaces much later as "not enough space" and reads as a device problem. `avd_config_path` now looks in every directory avdmanager honours -- no precedence is asserted, the existence check decides -- and a level that cannot be found or written fails instead of running unpinned. R34 / #43 -- disable_region_sampling's "one blocking wait on the device" did not wait: `adb shell stop` does not clear sys.boot_completed, so the property still read 1 and the loop returned at once. Deleted, and the comment now names the service-check loop below it as the actual wait -- which polls the better thing anyway, since `Can't find service: package` is the failure it exists to prevent. That loop also says so when it gives up after 150 s instead of proceeding silently. Deliberately not doing the `setprop sys.boot_completed 0` variant: the loop tested for an empty value, so a 0 would not have made it wait either, and the `!= 1` form it would need is an unbounded loop inside `adb shell` with no timeout. Checked with `bash -n` and with two stub harnesses in place of a device (shellcheck is not installed here): one drives the extracted lifecycle functions against fake binaries and asserts pid 0 really does hit the sender's process group, that an empty EMU_PID now signals nothing and returns at once, that SIGINT cleans up once and exits 130 within seconds, that KEEP_AVD survives the trap path, and that an explicit exit status survives the EXIT trap; the other runs the real script end to end on the boot-failure path, which stops short of e2e-run.sh, and checks the pins land in config.ini, the created AVD is removed, a misplaced config.ini fails the level, and `set -u` is not tripped anywhere. No emulator was booted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
614af35647 |
Hold the corrections themselves to the standard they impose
Three defects in the three preceding commits, found on review. A commit set whose subject is stale dates and inferred status cannot carry either. Dates. Both correction blocks were stamped 2026-08-23. The commits are dated 2026-08-22, as is every other date in these two files and the review that produced them -- a day in the future, in the one place a reader checks to see how fresh a correction is. Corrected to the commit date, and the D6 note now carries one too. Coherence. The Status line was changed to say fix status "tracks main, re-checked at 18c53a3" while "Last verified: 2026-08-22, against main at 903b43c" stood two lines below it, unchanged. A reader would take the whole document as anchored to |
||
|
JMR-devandClaude Opus 5
|
01cbc94888 |
Say what the FFmpeg format tests have actually been run against
R18 / #27. The front-page status line said the FFmpeg format tests "have been written but not yet executed on a device". They have been executed, repeatedly and green, and this is the one line a contributor uses to decide whether the FFmpeg path is trustworthy -- understating it costs more than a stale detail elsewhere would. FFmpegEngineTest's nine format tests -- mp3, gif, matroska, flac, wav, opus, H.264, H.265, and the one asserting a failure surfaces as an exception rather than a silent empty file -- were present at every commit cited below, checked by counting @Test in that file at each: - Physical Pixel 10 Pro XL, API 37, 2026-08-21 at |
||
|
JMR-devandClaude Opus 5
|
7e7f1301a7 |
Re-date the audit's testing section, which its own follow-up falsified
R14 / #23, R15 / #24. "On testing these" proposed a plan; the twelve fixes then executed it, so the section describes a state that no longer exists. It is re-dated rather than deleted -- the reasoning is why the test stack looks the way it does -- with each stale claim marked where it stands. R15 / #24, four statements, each checked against this checkout rather than against another document: - "exactly one dependency, testImplementation(libs.junit)". There are five: junit, robolectric, androidx.work.testing, the Compose BOM platform and compose-ui-test-junit4. - "a testOptions { unitTests.isIncludeAndroidResources = true } block, which this module does not currently have at all". app/build.gradle.kts:105-110. - "work-testing, compose-ui-test-junit4 and espresso-core ... have zero users." By import, work-testing has seven files under app/src/test and androidx.compose.ui.test has one. espresso-core really is still at zero, so that third is kept as the only part still standing. - The preamble's "OutputPublisher, both ViewModels, both Workers and MainActivity have no JVM unit tests at all". 25 JVM test files were added over that set, 180 tests to 257. That last one is also the derivation of CLAUDE.md's ~31% coverage figure, so the reasoning is kept verbatim and only its tense and scope are fixed: the ~31% is what those ~1,200 untested lines produced at |
||
|
JMR-devandClaude Opus 5
|
9f0bc9d19b |
Correct the defect audit's status metadata, which went stale in hours
R3 / #12, R12 / #21, R13 / #22. Three status claims in the audit were false against `main` at |
||
|
JMR-devandClaude Opus 5
|
792286a2d7 |
Stop three claims in the API 37 doc outrunning their evidence
Three corrections, all narrowing: - angle_indirect and swangle_indirect are not two independent renderers here. Both logged gles_mode_selected:swangle with the same adapter, differing only in the Vulkan backend underneath -- unlike at API 33-36, where angle_indirect resolves to ANGLE on llvmpipe. What is 7-for-7 is the host-GLES-versus-not split, not "two renderers agree". - "Disabling SystemUI stops the crashes entirely" was one 180-second measurement on a device that had been up twelve minutes. The harness path reproduces a rate collapse, not a zero: its own quiet check printed 1 abort in 45 s and 4 across the run. A 47-second Gradle run survives that; a five-minute one might not. - "Reproduced twice" conflated two routes. The 49/2/0/2 came back from a hand-driven sequence and from the harness, which corroborates the numbers, but the harness path itself has one green measurement. Also records what the doc never said: from 37.1 onward Google ships only 16 KB-page x86_64 images, so page-size alignment is a prerequisite for that path rather than a detail. All 20 libraries in the committed FFmpeg AAR are 0x4000-aligned, checked before the first ps16k boot -- which is why 37.1 reproducing the abort means the gralloc bug and not a page-size mismatch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
739bffa5a0 |
Re-derive the API 37 emulator failure: it is the renderer, not the image
docs/api-37-emulator-crash.md claimed "Both swiftshader_indirect and host crash... The crash is in the gralloc mapper, below the renderer." Re-measured, seven runs, one variable each: that is wrong. The mapper is below the renderer, but whether its bad path is reached is not. -gpu host gles_mode_selected:host never boots (57-71 aborts, looping) -gpu swangle_indirect gles_mode_selected:swangle boots, 85 s (1 abort) -gpu angle_indirect gles_mode_selected:swangle boots, 112 s (2 aborts) The old claim rested on two samples of two different things, neither of them ANGLE: the local swiftshader_indirect sample was void, because on this host every SwiftShader-GLES launch segfaults the emulator before the guest matters (the execheap bug in docs/local-emulator.md, not understood when that file was written), and the CI sample was a single swiftshader_indirect run. Also re-derived, and null: android-37.1 rev 8 -- a stable REL image the doc's own "new image revision" trigger was too narrow to catch -- fails identically; -feature -GLDMA,-GLDMA2,-GLDirectMem is accepted and changes nothing; the image's advancedFeatures.ini is byte-identical to API 36's but for one camera line; and there is still no ATD image above API 36. The mechanism, end to end: SystemUI registers a nav-bar luma-sampling listener, SurfaceFlinger's RegionSamplingThread locks a GraphicBuffer, Gralloc5 routes into GoldfishMapper::readFromHost, which asserts, and init SIGKILLs zygote in response -- so the framework restarts under the test run. Disabling SystemUI removes the listener and the aborts stop dead: 0 in 180 s, against 10-11 per 150 s. So run-e2e.sh now covers API 37: renderer chosen per level (33-36 need host, 37 must not have it), dotted image labels, SystemUI disabled followed by a deliberate stop/start, and an abort count printed on every 37 row. The result is 49 tests, 2 failures, 0 errors, 2 skipped, reproduced twice. The two failures are Media3EngineTest on c2.goldfish.h264.decoder; API 35 under the identical renderer is 49/0/0/2 green, so they are the image and not the renderer. CI's matrix should still stop at 36, for reasons now written down rather than assumed. CLAUDE.md is left alone; a replacement bullet is proposed in the doc. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Jason Ross
|
18c53a3830 |
Merge pull request #9 from JMR-dev/tools/local-emulator
Find out why the emulators segfault, and make them run |
||
|
Jason Ross
|
fc2afff32e | Merge branch 'main' into tools/local-emulator | ||
|
Jason Ross
|
9c056f5f5d |
Merge pull request #8 from JMR-dev/feat/defect-fixes-base
Fix ten defects in the untested framework edge |
||
|
JMR-dev
|
63d53b0bb8 | Merge remote-tracking branch 'origin/feat/defect-fixes-base' into scratch/integrate-d2-d3 | ||
|
JMR-devandClaude Opus 5
|
c2e6344aad |
Let WorkManager keep sole ownership of the progress notification
`publishProgress` posted with `NotificationManager.notify(NOTIFICATION_ID, …)` directly, on the
very id WorkManager owns through `setForeground`, using a notification built `setOngoing(true)`.
Two posters for one id is a race about which of them wrote last, and on a Pixel 10 Pro XL it was
lost on attempt 3 of 12 while cancelling a `BEST`-tier job:
attempt 3: terminal state = CANCELLED
attempt 3: +300ms active=0 id1001=false ongoing=null <- WorkManager tore it down
attempt 3: +700ms active=1 id1001=true ongoing=true <- a progress tick put it back
attempt 3: +5000ms active=1 id1001=true ongoing=true
Still there ten minutes later, with no app process left in the world to withdraw it. The orphan's
record carried `flags=ONGOING_EVENT|ONLY_ALERT_ONCE` and **no `FOREGROUND_SERVICE`**, which is
what identifies the poster rather than merely suggesting one: WorkManager's own post goes out
with that flag and this one did not.
Whether the user could then swipe it away is deliberately not claimed here. An earlier reading
said "cannot dismiss", on the strength of `isClearable()` returning false — but that is false
purely because `FLAG_ONGOING_EVENT` is set, the record carries neither `FOREGROUND_SERVICE` nor
`NO_CLEAR`, and API 34+ lets an ongoing notification with no foreground service behind it be
swiped. The SystemUI test was impossible behind a secure lock screen and was never done. The
resurrection is what is reproduced, and it is what this fixes.
Progress now goes through `setForegroundAsync` — the non-suspending half of the same call
`doWork` already makes, which is the supported way to update a running worker's foreground
notification. That is not merely a different mechanism for the same post: it hands the id back to
its owner, so the notification WorkManager withdraws when the job ends is the same one the last
progress update wrote, and there is nothing left holding a second reference to it.
Two guards, and they are independent on purpose:
- **`isStopped` short-circuits the whole function.** A tick arriving after the stop has nobody
left to report to, so a stopped worker publishes nothing at all — the `setProgressAsync`
included, which WorkManager refuses for finished work anyway.
- **`setForegroundAsync` refuses on its own** for work whose state is already terminal. That
closes the window between the `isStopped` check and the update reaching the task thread,
which a check alone can only narrow.
The `~1/sec` throttle is unchanged, in effect and in reason: FFmpeg's statistics callback and
Media3's progress polling both fire several times a second, and pushing every one of them janks
the system UI. Routing them through WorkManager does not make them cheap, so the interval stays
exactly where it was. It is reordered into an early return rather than a nested `if`, which is
the only difference.
The initial `setForeground(...)` in `doWork` is untouched and stays a suspending call inside the
`try`. That placement is the whole of the previous commit on this file: a denied background
foreground-service start throws there, and `FailureOutcome` turns it into a retry rather than a
terminal failure. Converting it to `setForegroundAsync` for symmetry would have moved that
exception into an unobserved future and undone it silently.
The future `publishProgress` gets is not awaited — it is called from an engine callback, which is
not a coroutine, and a progress update is not worth blocking one for. Both of its failure modes
are benign, so a refusal is logged rather than propagated: dropping it entirely would make the
one that actually happens, a job finishing mid-update, invisible.
`ProgressNotificationTest` drives the real worker with a recording `ForegroundUpdater` — the same
seam `DeniedForegroundStartTest` uses — and asserts on both halves, because either alone passes
against something wrong. The positive half is that the update reached WorkManager on the id it
already holds, carrying `Notification.EXTRA_PROGRESS` of 42; a test that only checked nothing was
posted directly would pass just as well against a `publishProgress` that had been deleted. The
negative half is that Robolectric's notification manager holds nothing at all, asserted over the
whole manager rather than one id, so a renamed constant cannot make it pass by asking about a
notification nobody posts.
All three were red before the change: `one throttled progress update expected expected:<1> but
was:<0>` (nothing had reached WorkManager), `and must resurrect nothing expected:<0> but was:<1>`
(the device's resurrection, on the JVM), and `50 ticks inside one throttle window must not be 0
updates`. Each guard was then removed on its own to check the test that names it bites: without
`isStopped` the stopped worker publishes twice — `a stopped worker must publish nothing
expected:<1> but was:<2>` — and without the throttle fifty ticks become fifty updates.
`ConcatWorker` reports no progress at all and owns id 1002 by itself, so it has none of this and
none of it is added.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
b86df47c43 |
Tell an unknown input size apart from an empty file
`queryFile` started at `var size = 0L` and only moved off it when a provider answered the
`OpenableColumns.SIZE` column, which the platform documents providers *may* omit. So "this file
is empty" and "nobody would tell me how big it is" reached `OutputPublisher.hasSpaceFor` as the
same number, and `hasSpaceFor(0)` is not a space check -- it is "is there 128 MB free", which any
phone with a working camera passes.
The reachable value is not an inference. On a Pixel 10 Pro XL, `contentResolver.query` on a
`file://` URI returns null outright, so the cursor block never runs at all and the default
survives untouched: `queryFile gave displayName='input' sizeBytes=0`. Robolectric reproduces that
exactly -- null query, and a descriptor that reports 4321 bytes for the same file -- which is why
every test here is a JVM test rather than a device one.
`InputQuery` replaces the two copies of `queryFile`, which were byte for byte identical in
`ConversionViewModel` and `JoinViewModel`, so a fix to either would have been a fix to half the
app. It asks for the size twice: what the provider says, and then what the file itself says
through `openFileDescriptor(uri, "r").statSize`. The second needs no cooperation beyond the input
being openable, which a conversion is about to require anyway, and it is what answers the device
case above. Only when both decline is the answer null, and `InputFile.sizeBytes` is `Long?` so
that null cannot be spelled the same way as zero again.
**What an unknown size does was the decision, and it is deliberately not a refusal.**
`OutputPublisher.hasSpaceForUnknownSize()` produces the same number the defect produced by
accident -- with no size to reserve for, the headroom is all there is left to check -- and that is
worth saying plainly rather than dressing up. What changed is that it is now the answer to a
question that was asked. `hasSpaceFor` means "there is room for this many bytes" and nothing else
claims it.
Refusing was the obvious alternative and would have been worse than the bug: it turns "no
provider answered the SIZE column" into "this file cannot be converted", for a user who can do
nothing about either. A fixed floor was the other, and there is no honest number for it -- a
1 GB floor refuses a 10 MB conversion on a device with 500 MB free, which is the same failure in
a costume. `SpaceCheckTest` pins the choice from both sides: an unmeasurable input must not end
the job, and a full disk must still refuse it.
That second half is why the default answers *through* `hasSpaceFor`. `FakeFailures.FullDisk` in
the instrumented suite overrides `hasSpaceFor` and nothing else, so the delegation is the only
reason it still refuses an unknown-size job. Replacing the delegation with a bare `true` leaves
the full-disk test red with `expected:<Failure {error : Not enough free space to convert.}> but
was:<Failure {error : FFmpegKit failed to start on brand: robolectric...}>` -- the job sailed past
the guard and died at the engine instead.
Both workers get the same shape. The size arrives as input `Data`, which has no null, so the
absence of the key *is* the unknown -- `getLong(key, 0L)` was the other half of the conflation.
When it is absent the worker measures the input itself, which it can do because it holds the URI:
that covers a `request(...)` built by hand and work enqueued before the size became optional, and
it costs an ordinary job nothing because it runs only on the fallback. `ConversionWorker.request`
writes neither the `Data` entry nor the `JobTags.sizeBytes` tag for a size nobody knows, since a
tag reading `size-bytes:0` would come back through `Reattachment` as a confident claim that the
user's file is empty -- and `reattach()`'s `?: 0L` is gone for the same reason.
A join's total is `InputQuery.total`, which is null the moment a *single* input cannot be sized.
Summing the ones that answered was the competing reading and is rejected: a lower bound is
indistinguishable from a real total once it reaches the space check, so the guard would reserve
for half the job and pass. Reverting it to `sumOf { it ?: 0L }` records `[1111]` for a two-file
join whose second input nothing can measure.
Work already in the queue keeps the old conflation, and there is no fixing it. The previous
`request()` always wrote `putLong(KEY_SIZE_BYTES, sizeBytes)`, so a job enqueued before this
commit for a file nothing could size carries the key *present* and set to zero -- which reads
back as a declared size of zero and is trusted, exactly as before. Its `lmc.size-bytes:0` tag
reads back the same way, so `reattach()` shows such a card "0 B" rather than "Size unknown".
Nothing can separate that from a genuinely empty file after the fact, and a rule that treated a
declared zero as suspect would only rebuild the conflation facing the other way. WorkManager
keeps finished work for about a week, so this is a bounded window that clears itself; new work
never enters it.
`hasSpaceFor`'s KDoc claimed peak usage was "roughly input + output at once" while the arithmetic
reserved `input + 128 MB`. The arithmetic is what stays and the doc now says why: `bytes` is the
input's size standing in for the output's, generous for the ordinary conversion (which is asked
for precisely because it shrinks its input) and short for a re-encode to a bulkier codec; the
128 MB absorbs that error and the transient double copy while `publish` runs. Reserving
`input + output` outright would refuse jobs that fit. This is a pre-flight check that stops an
obviously impossible job from spending minutes finding out, not a guarantee -- a conversion that
runs out of space anyway still fails through its engine.
The measurement side of that line is untouched on purpose. `StorageManager.getAllocatableBytes`
is a separate entry with its own device evidence and its own `informational += "UsableSpace"` in
the lint block; this commit is about the number going *in*. `hasSpaceFor(bytes: Long)` keeps its
signature and stays open, so nothing overriding it had to change.
The file card says "Size unknown" rather than `0 B`. Handled at the call site rather than inside
`formatBytes`, because a formatter that invented a number would be the defect on screen; the card
already degrades in words for a file nothing could read.
Five of the seven new tests were red before a line of production code moved, with the numbers
they were about: `expected:<4321> but was:<0>` for a picked file, `expected null, but was:<0>` for
one nothing can measure, `expected:<[1111, 2222]> but was:<[0, 0]>` for the join picker, and
`expected:<[4321]> but was:<[0]>` and `expected:<[3333]> but was:<[0]>` for what the two workers
asked the space check. The tests assert on the *question* rather than the verdict, which matters:
one that only checked whether the job ran would have passed against the defect, since the defect
is that the guard is vacuous rather than that it refuses.
The remaining two needed the new call to exist first, so each was proved by mutation instead.
Answering the unknown with `hasSpaceFor(0L)` inline leaves `expected:<[]> but was:<[0]>` in both
workers; refusing it instead leaves `an unknown size must not end the job; got Failure {error :
Not enough free space to convert.}`. Making the worker always measure rather than trust a declared
size leaves `expected:<[9999]> but was:<[4321]>`.
`join()`'s own use of `InputQuery.total` is tested separately from the function, because
`StagingCleanupSupport` already records what that distinction costs: a tool can be provably right
while nothing calls it. `SucceedingWorkerFactory` now keeps the input `Data` of every request that
reaches a worker -- the only place it is legible, since `WorkInfo` hands back a job's tags and its
output and never the `Data` it was built with -- and the test reads the enqueued total off it.
Restoring `inputs.sumOf { it.sizeBytes ?: 0L }` leaves every other test in the change green and
this one red with `a total that could not be worked out must not be enqueued as a number`.
`ConversionViewModelProbeFailureTest` expected `InputFile(INPUT, "input", 0L)` for an authority no
provider serves. It expects `sizeBytes = null` now, which is the behaviour change stated where a
reader will meet it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Jason Ross
|
37bf884c02 | Merge branch 'main' into feat/defect-fixes-base | ||
|
Jason Ross
|
5f1999ef6c |
Merge pull request #7 from JMR-dev/docs/defect-audit
Write down what is actually wrong with this app, and how we know |
||
|
Jason Ross
|
6d250e6c8e | Merge branch 'main' into docs/defect-audit | ||
|
Jason Ross
|
9279816a0d |
Merge pull request #6 from JMR-dev/chore/ignore-claude-dir
Keep Claude Code's agent worktrees out of the repository |
||
|
JMR-devandClaude Opus 5
|
cf13e0225c |
Keep Claude Code's agent worktrees out of the repository
.claude/worktrees/ holds complete working copies -- during a parallel agent run there were six, each a full checkout with its own build output. None of it is tracked, so it sat in `git status` as untracked noise, and a `git add -A` at the wrong moment would have committed the repository into itself. scheduled_tasks.lock is equally machine-local. Both are named individually rather than ignoring .claude/ wholesale. That directory is also where shared project config lives -- settings.json, agents/, skills/ -- and ignoring the parent would have pre-emptively hidden files a project would normally commit, for no benefit today. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
ef3d87e12d |
Write down what is actually wrong with this app, and how we know
detekt reports zero findings and there is no baseline, no @Suppress and no tools:ignore anywhere -- so the static-analysis gate is green and honest, and it is not where the defects are. They are in the Android-framework edge the linters cannot see into: OutputPublisher, both ViewModels, both Workers and MainActivity, which between them have no JVM unit tests at all and account for most of the ~31% coverage figure. Sixteen entries. Each records what is wrong, how confident we are that it is wrong, how to provoke it, and what a fix would have to decide. The confidence labels are load-bearing: four entries were driven on a physical Pixel 10 Pro XL running API 37, and they are marked differently from the ones that are still inspection only. The device pass earned its keep by contradicting us. D1 -- the one defect that was already known and deferred, the UsableSpace lint finding -- did not reproduce. getAllocatableBytes measured 500 MiB SMALLER than usableSpace, and writing 3 GB into the app's own cache moved both numbers identically, so no cache counted as reclaimable at 66% free. The entry keeps the falsified prediction next to the measurement that killed it, because that is the useful part. Two entries, D15 and D16, were found while fixing others and are recorded rather than folded in silently. D16 is the one worth reading: two individually correct fixes compose into a gap neither of them owns. Entry bodies describe each defect as found and are deliberately not rewritten as fixes land. This is the record of what was wrong, not a changelog; the summary table carries the fix status. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
49535998b5 | Merge branch 'fix/worker-durability-and-naming' into scratch/integrate-d2-d3 | ||
|
JMR-devandClaude Opus 5
|
159320dfa0 |
Name a finished file after the job that made it
Six places decided what a converted or joined file should be called, and not one of them asked
the job. `JoinViewModel` reported `JoinState.Saved("joined.mp4")` whatever the format;
`JoinScreen` opened `CreateDocument("video/mp4")` and launched it with `"joined.mp4"`. On the
convert side `save()` and `suggestedOutputName()` built the name from `_settings.value.spec`, and
the screen took the MIME type from the same place -- the picker as it stands *now*, which is not
the spec the job ran with.
All six are right today, and all six are right by accident. The join screen has no format picker,
so the three MP4 literals agree with `ConcatWorker.request`'s default. The conversion pickers are
drawn only in the `Ready` state, so the settings cannot move between enqueue and save. Neither
accident is load-bearing anywhere it is written down.
One of them has already stopped holding, quietly. A job picked up by `reattach()` ran with a spec
that was never in this ViewModel's settings, because those settings belong to a process that no
longer exists -- so a reattached MP3 conversion is offered `.mp4` and `video/mp4` today. The
previous commit made that path more reachable rather than less: `Reattachment` exists precisely
to find work this ViewModel did not start.
The fix is to ask the only thing that knows. The spec travels to the worker as input `Data` and
`WorkInfo` hands input `Data` back to nobody, so the worker is the single point at which the
input's name and the spec that ran are both in scope. Both workers now report the two derived
strings -- the name to suggest and the type to open the dialog with -- in their output `Data`,
and they ride on `ConversionState.Converted` and `JoinState.Joined` from there. `save()` reports
what it saved rather than recomputing it, and both screens read the name and the MIME type off
the state they already collect, remembering their `CreateDocument` contract against that type
instead of a literal. The MIME type is not cosmetic: some providers rewrite a document's
extension to match it, so an MP3 offered as `video/webm` can arrive with the wrong one.
`suggestedOutputName()` is deleted rather than repaired. With the answer on the state there is no
caller left for it, and an accessor recomputing the same string would only be a second place for
it to be wrong -- which is what it was.
`ConcatWorker` gains `DEFAULT_FORMAT` and `outputNameFor(format)`. Three copies of "MP4" is the
shape this entry is about, and the input-Data default, `request`'s parameter default and the
ViewModel's fallback for a job that predates this change are exactly three copies.
Work already in the queue carries neither string, and WorkManager keeps finished work for about a
week, so that is the ordinary case for a few days rather than a corner. Those fall back to the
old derivation, which is a guess -- but it is the same guess the app was already making, it is
confined to jobs enqueued before this commit, and for such a job there is genuinely nothing
better to hand. New work never reaches it. The join's fallback is not even a guess: the format
`ConcatWorker.request` has always defaulted to is the format such a job really used.
`reattach()`'s KDoc carried this as a known wart it was deliberately leaving alone. That
paragraph is now a description of the fix rather than of a defect.
Tested at both ends, since either alone would pass while the other was wrong.
`WorkerOutputNamingTest` runs the real worker on an MP3 job -- nothing like the default preset, so
a name built from the picker is visibly wrong rather than accidentally right -- and compares the
whole success `Result`, which is what makes a missing key fail rather than go unnoticed. The two
ViewModel tests drive a finished job and then move the picker, which is what a reattached job
amounts to from the ViewModel's point of view.
Restoring just the two name sources -- `save()`'s `outputNameFor(displayName, _settings.value.spec)`
and `JoinState.Saved("joined.mp4")` -- turns them red with
`expected:<holiday_converted.mp3> but was:<input_converted.webm>` and
`expected:<joined.mkv> but was:<joined.mp4>`. The convert side loses the extension *and* the name:
`_settings.value` had been moved to WebM, and the display name a reattached job cannot supply had
already fallen back to the placeholder.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
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-devandClaude Opus 5
|
dcdcbfd3af |
Let a cancelled conversion stay cancelled
Both workers' outer `catch (e: Throwable)` caught `CancellationException` along with everything else and answered it with a `Result`. `runMedia3OrFallBack` goes out of its way to rethrow cancellation rather than fall back to software, and then the catch above it converted it anyway. A coroutine that reports completion inside a scope which has already been cancelled is structured concurrency's one rule broken, and it is the kind of break that stays quiet: nothing downstream complains, and the next thing to hold a resource across that boundary is the thing that finds out. Nothing on screen disagreed today, which is why this is a low-severity entry rather than a bug report. WorkManager cancels the worker's coroutine through `WorkerWrapper.interrupt`, which cancels `workerJob` with a `WorkerStoppedException`; the surrounding `withContext(workerJob)` then throws that whatever the worker returned, and `launch()` resolves it as `ResetWorkerStatus`. The returned `Result` is read only when nothing stopped the worker at all. So the change is about the shape of the code rather than about a symptom. One behaviour does move, and it is worth naming rather than discovering later. A cancellation that is *not* WorkManager stopping us -- FFmpegKit reporting `ReturnCode.isCancel`, which cancels the continuation -- now leaves `doWork` as a cancellation, and `WorkerWrapper` resolves a self-cancelled worker as `Resolution.Failed()` with no output data instead of the `Result.failure(KEY_ERROR ...)` it used to build. Both ViewModels already fall back on blank output data, deliberately and with a test, so the user sees "Conversion failed." either way. That is also the honest answer: the only route to `isCancel` is a cancellation someone asked for. The `staged.delete()` on that path is kept, and moved into the new branch rather than left to the one below it. A cancelled attempt leaves a partial in staging, the next attempt starts `doWork()` from the top rather than resuming it, and this catch holds the only handle to the file. Reaching any of this from a JVM test needed one more thing: `doWork` called `MediaProbe.probe` directly, and it was the last caller bypassing `ConversionDependencies.probe`. FFprobe's loader throws a bare `java.lang.Error` with no native library present, so no unit test could reach a single line below it. The seam's own KDoc says this is what it is for -- coverage of the branches that only run when something goes wrong -- and the default is the same real probe, so the app and the instrumented tests are unchanged. `WorkerCancellationTest` drives the real worker through a real `WorkManager` to the software engine, forced with `FORCE_SOFTWARE` because it is the one preference that decides without consulting the input, so the test does not depend on a routing rule it is not about. The engine stub writes bytes before it throws, which is what makes the delete assertion mean something: a stub that only threw would let a missing `delete()` pass. Three cases -- cancellation propagates, cancellation still deletes, and an ordinary failure is still answered with a `Result` carrying its message, which is the half that would break if the rethrow were widened past cancellation. Before the fix the first of those failed with "cancellation must leave doWork as cancellation, not as a Result; got null" -- `runCatching` had nothing to report, because `doWork` had returned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
5c27801c77 |
Retry work the system refused to start, instead of failing it terminally
`ConversionWorker`'s KDoc says the queue survives process death, and that is the app's stated
reason for choosing WorkManager at all. A Pixel 10 Pro XL says otherwise. An 18-minute
transcode was killed mid-write with `kill -9`; 119 seconds later, on a natural dispatch with
no `cmd jobscheduler run` anywhere in the session, WorkManager recovered it by itself and the
system refused it:
WM-ForceStopRunnable: Found unfinished work, scheduling it.
ActivityManager: Background started FGS: Disallowed [callingPackage:
org.libremediaconverter; uidState: CEM; BFGS denied: true; code:DENIED]
WM-WorkerWrapper: android.app.ForegroundServiceStartNotAllowedException
WM-WorkerWrapper: Worker result FAILURE
WM-Processor: Processor 3d1c9862 executed; reschedule = false
`Found unfinished work` is the clean process-death recovery path, not a force-stop. The job was
ordinary -- `Priority: 300 [DEFAULT]`, not expedited -- so there was no allowance it could have
carried. `HAS_FOREGROUND_EXEMPTION` was set on it and it was still denied; that flag governs the
runtime guarantee once a service is running, not permission to start one.
The cause is structural rather than subtle. `setForeground(...)` sat above `return try {`, so its
throw escaped `doWork()` without reaching `handleTimeoutIfNeeded`, `Result.retry()`, the
`workDataOf(KEY_ERROR ...)` or `staged.delete()`. Three separate costs, all measured on the
device: `reschedule = false`, so nothing ran again despite `run_attempt_count=2`; output `Data`
of `X'ABEF000100000000'`, a header with zero entries, so the screen said "Conversion failed."
with nothing to add; and 2 MB of a partial `long_input2_converted.mp4` left in staging.
`ConcatWorker` had the same shape.
So `setForeground` moves inside the `try` in both workers, and the staging handle is named just
above it -- `createStagingFile` only builds a path, nothing is written until an engine opens it,
so naming it early costs nothing and makes the catch total. `MediaProbe.probe` was outside the
`try` for the same reason and had the same problem; it is now inside too. On a retry the staged
name is unchanged, so the delete on the way out collects the partial the killed attempt left
behind rather than orphaning it.
What to return was the real decision. `Result.retry()`, because the denial is about *when* the
job ran and not about the job: the file is fine, the settings are fine, and the one thing that
grants an app permission to start a foreground service is being in the foreground, which the
user supplies by opening the app. But WorkManager never gives up on its own, so an unbounded
retry means a job nobody comes back for waking the device forever while the screen says "paused"
and never explains itself. `FailureOutcome` therefore bounds it at ten attempts, chosen against
the default backoff rather than as a round number: exponential from
`WorkRequest.DEFAULT_BACKOFF_DELAY_MILLIS` (30 s), doubling, clamped at `MAX_BACKOFF_MILLIS`
(5 h), which spans about 8 h 30 m before the eleventh attempt fails with a message the user can
act on. The counter is `runAttemptCount`, which counts every attempt and not only denied ones --
WorkManager exposes no other -- so a transcode already retried ten times by the six-hour budget
will fail on its first denial rather than getting ten of its own. That is accepted rather than
overlooked, and the timeout branch ignores the count entirely so the budget's own retries are
untouched.
The rule goes in `FailureOutcome` rather than beside it, which is what its KDoc asks for: isolate
a decision whose triggering condition cannot be provoked in a test. It now reads the stop reason
*and* the cause, with precedence stated rather than left to branch order -- once something has
stopped the worker, the exception it was holding describes that stop and not a reason of its own.
The match is on `ForegroundServiceStartNotAllowedException` exactly, never on its
`IllegalStateException` supertype: a muxer that was never started throws one of those too, and
matching the supertype would retry every ordinary failure for eight hours.
Expedited work is still not used, and this is not an argument for it. It maps to JobScheduler
expedited jobs with a short quota, which is the wrong shape for a multi-minute transcode; the
exemption it buys is for starting, and the quota it costs would be paid by every job.
Tested at both levels, because only one of them bites. `FailureOutcomeTest` gains six cases for
the rule -- retry at the bound, give up past it, an unrelated `IllegalStateException` that must
not be mistaken for a denial, a stop reason winning over the exception, and the budget timeout
ignoring the count. `DeniedForegroundStartTest` covers the wiring, which is where the defect
actually was: a `ForegroundUpdater` whose future completes exceptionally makes `setForeground`
throw exactly what the platform throws, because `WorkForegroundUpdater`'s own comment says it
propagates the exception to the caller and `ListenableFuture.await()` unwraps the
`ExecutionException` on the way. Moving `setForeground` back above the `try` turns all four of
those red with a bare `android.app.ForegroundServiceStartNotAllowedException`, which is the same
line the device logged.
Four comments claimed the old behaviour and are corrected with it. `ConversionState.Waiting`
said the budget had run out; `observe()`'s ENQUEUED branch said the budget was the likely cause,
where a denied restart is now the likelier of the two; and `Reattachment.choose` justified
excluding FAILED partly on interrupted workers coming back FAILED "rather than retried", which is
the sentence this commit falsifies. The exclusion stands on its own and says so now.
The bound's own KDoc says what giving up does *not* buy, too. `FOREGROUND_DENIED` reports through
a FAILED job and `Reattachment` excludes FAILED, so a user who was not watching at the eleventh
attempt meets an empty screen rather than the message. What the bound reliably buys is the end of
the retrying.
Both `Waiting` screens change their wording. The state is `ENQUEUED` with a run attempt behind
it, which cannot distinguish the two causes that now reach it, and the old copy named only the
six-hour budget -- which is now the less likely of the two. "Keeping the app open helps it along"
covers both, and for a denied start it is not filler but the actual remedy.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
22c7914395 |
Find out why the emulators segfault, and make them run
CLAUDE.md has said "Emulators segfault on this host -- qemu dies on every AVD" since the E2E matrix landed, and the PR that introduced it called the failure "exit 139 across three AVDs and both GPU backends, environmental". That is accurate about the symptom and wrong about the cause, and the cost of being wrong was the whole instrumented suite being unrunnable here. SwiftShader's Reactor JIT writes generated GLES shader code onto the heap and mprotects it executable. Fedora's SELinux policy denies that -- execheap is not granted to unconfined_t and selinuxuser_execheap is off -- so the mprotect fails and the emulator takes SIGSEGV the moment it calls the routine it just generated. The AVC denial and the core are the same event, one second apart. The predictor is mechanical and held 7 for 7 across every -gpu mode: a run crashes if and only if it dlopens gles_swiftshader/libGLESv2.so. host, angle_indirect and swangle_indirect boot. auto, off, guest and swiftshader_indirect crash -- and auto is the default, which is why the failure looked universal rather than renderer-specific. tools/local-emulator/run-e2e.sh picks a renderer that works and refuses the ones that do not. It reuses .github/scripts/e2e-run.sh rather than forking it, so the local and CI diagnostics cannot drift; the one change there adds an optional E2E_EXTRA_GRADLE_ARGS that is unset in CI, so CI runs byte-identical commands. The API 33-36 sweep has now been run and is written down. All four levels are green on a local emulator and match the physical Pixel 10 Pro XL baseline exactly: 49 tests, 0 failures, 0 errors, 2 skipped, every level. Those counts come from the result XML, not the UTP console counter, which double-counts skips and reported "Finished 51 tests" on all four. No boot log dlopens SwiftShader GLES and the sweep window holds no AVC denial and no qemu core -- which is confirmation of the mode matrix's first row rather than new coverage, since every one of these runs is -gpu host. The table is still seven modes measured once each. Two things the sweep surfaced that the doc now records: pre-build before sweeping, or a fresh checkout spends API 33's 20-minute wrapper budget compiling and wedges before a test runs; and the device pinning is untested by this run, because the Pixel dropped off USB five seconds before it started. Still offered for review rather than applied: the CLAUDE.md correction the doc drafts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
07933c80c5 | Merge branch 'fix/native-boundary-guards' into scratch/integrate-d2-d3 | ||
|
JMR-devandClaude Opus 5
|
7db320018a |
Correct the JDK claim and decide the backup rules
D11's documentation and scaffold items, less the one row that belongs to another change stream. README's "Requires JDK 17+ (AGP 9 will not run on older)" was wrong twice over, and `f496291` already corrected the same claim in CLAUDE.md. The floor is not AGP's, and 17 is not what compiles anything: Gradle 9.7.1's own `SupportedJavaVersions` carries MINIMUM_CLIENT_JAVA_VERSION = 8 and MINIMUM_DAEMON_JAVA_VERSION = 17, and this repo then overrides the daemon upward to 25 in gradle-daemon-jvm.properties. So the honest statement is that the launcher floor is 8, the daemon is 25 whatever JAVA_HOME says, and the app's bytecode is 25 -- which is what `./gradlew --version` shows on this machine right now, launcher 21 against daemon 25. The data_extraction_rules TODO is filled in rather than deleted, because `android:allowBackup="true"` makes it a live question and the answer is not "nothing to say". The app stores nothing of its own -- no settings, no history -- so WorkManager's queue is the entire backup payload, and restoring it is wrong rather than merely useless: every row names a content:// grant and a cacheDir path that do not survive reaching another device, and cacheDir is not backed up at all. Since `ec969c4` the ViewModel queries WorkManager by tag on launch, so those rows would not sit inert either -- a fresh install would come up reattached to a job the user never ran on it. WorkManager declares no exclusion of its own, so nothing upstream prevents it. allowBackup stays true. The decision belongs in the rules file, where it is per-file and legible to whoever adds real user data later, rather than in an app-wide switch that would also turn off device-to-device transfer. Each file is named instead of excluding the "database" domain in one line. Lint's FullBackupContent detector skips an <exclude> that carries no path without checking it, so the one-line spelling could have silently protected nothing; the enumerated paths are ones the gate actually verifies, and they are present in the built APK's compiled resource. backup_rules.xml and android:fullBackupContent are deleted rather than filled in. That attribute is only read on Android 11 and lower and minSdk is 33, so it could never have applied here -- an equally empty template that, unlike the other one, had no live question behind it. Not touched: OutputPublisher's hasSpaceFor KDoc, which the audit lists under D11. That code belongs to a parked branch and another change stream. The stale com/example/androidmediaconverter package directory needs no commit: it is empty, and git has never tracked it because git cannot track an empty directory. Removed from the working copy directly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
97fdc49f01 |
Guard the native boundary against what it actually throws
D14: picking a file died instead of reporting an unreadable one when FFmpegKit's native library could not load. `probeWithFFprobe` guarded its call with `catch (e: Exception)`, and `ConversionViewModel.onInputPicked` guarded nothing, so the failure escaped a `viewModelScope.launch` -- which has no handler, and on a device ends the process. All three of the obvious narrow guards catch nothing, which is why this needed reading the AAR rather than guessing. `NativeLoader.loadLibrary` catches the `UnsatisfiedLinkError` that `System.loadLibrary` raises and rethrows a *bare* `java.lang.Error` wrapping it, so `UnsatisfiedLinkError` never escapes and the escaping type carries no information at all. Every touch of the class after the first is a different type again -- `NoClassDefFoundError` -- so a guard written for the first shape lets the second pick onwards crash, which is the harder half to notice. Both are in the test output verbatim. `catch (Throwable)` was the wrong answer for the reason the audit gave: it would swallow a genuine `OutOfMemoryError` in a method that spawns a native process, turning "this device is out of memory" into "this file looks unreadable" and letting the app act on it. So the line is drawn by a named predicate, `isNativeLoadFailure`, rather than by the catch clause -- every class-loading shape is a `LinkageError`, and nothing that means the JVM is failing is one. That disjointness is what makes the guard narrow. This is consistent with the position `config/detekt/detekt.yml` already takes for `TooGenericExceptionCaught`: the boundary's failure types are undocumented, so guessing crashes the app on a file it could have reported. One level up the opposite mistake is available too, and the predicate is what lets both be avoided at once. `TooGenericExceptionThrown` is relaxed for the test source sets only. A test that reproduces a failed native load has to throw what the library throws, and a tidier subclass would leave it passing against a defect it no longer reproduces. Main source is untouched by that and throws nothing generic. `ConversionDependencies.probe`'s KDoc is rewritten rather than left. It said this hazard was "deliberately not fixed here ... its own commit, with its own test", which this is -- leaving it would have replaced one true comment with a false one, which is the same defect class as the D11 work. Both halves are covered independently: reverting the `MediaProbe` catch reds only the two `MediaProbeNativeLoadTest` cases, reverting the ViewModel guard reds only the two injected-seam cases, and widening the ViewModel guard to `Throwable` reds the OutOfMemoryError case -- so the narrowness is pinned, not just the catch. Audited the sibling boundaries named in the audit and left all three alone: `FFmpegEngine` and `ConcatEngine` both construct and run under `catch (e: Throwable)` in their workers, and `Media3Engine` has no native loader of this kind and already routes failures through `runCatching`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
a6baf41866 | Merge branch 'fix/rotation-and-partial-publish' into scratch/integrate-d2-d3 | ||
|
JMR-devandClaude Opus 5
|
dbfc463c6d |
Delete the half-written destination instead of leaving it under the user's name
publish() streamed the staged file into the SAF destination with copyTo and had no
answer for a copy that failed partway. The destination volume filling up is the obvious
way in; a provider giving out mid-write is the other. Either way the bytes it had
managed stayed at the name the user picked, while the UI said "Could not save the file".
The user was left holding a truncated file they had just been told was never written,
and nothing in the app would ever tidy it up -- staging cleanup reaches
<cacheDir>/conversions and nowhere else, by design.
So a failed copy now deletes the document. The interesting part is not the delete, it is
what stops it, because removing a file the user already had would be a far worse defect
than the one being fixed.
Only a document URI. DocumentsContract.deleteDocument is the only delete this code has
any right to attempt and it is defined on document URIs, so isDocumentUri() gates it.
That is not a formality: it asks the package manager whether anything answers
ACTION_DOCUMENTS_PROVIDER for the authority, so a file:// path, a MediaStore item or a
content URI from an ordinary provider all fall out here untouched.
Only a destination that was empty when we started. The size is read BEFORE the stream
is opened -- opening for write truncates, so afterwards the question can no longer be
asked -- and the delete runs only when the answer was positively zero. Every
destination that reaches publish() today comes from the SAF CreateDocument contract, so
in practice it is a document this app created seconds earlier; but publish() cannot
verify that from a Uri, and a provider that hands back an EXISTING document for a name
the user re-picked would otherwise have its file deleted rather than merely truncated.
Truncated is bad. Gone is worse, and it is the user's file either way.
That guard fails towards doing nothing. A provider that does not report _size, a query
that returns no row, a resolver call that throws -- all of them land in "not known to
be empty", so the fix is conservative rather than universal: it will not clean up
behind such a provider, and it will not delete anything of theirs either. The defect is
closed for providers that answer a size query; ExternalStorageProvider backs its
documents with real files, so a freshly created one reports 0, but that is reasoning
about it rather than a run against it. Stated here rather than implied, because
"fixed" would overclaim what was verified.
The original failure is what the caller sees. Cleanup runs in its own runCatching and a
throw from it is attached to the original exception as a suppressed one. deleteDocument
reports failure two different ways -- false, or a rethrown RuntimeException -- and
neither is worth failing the save over, because the save has already failed.
ConversionViewModel.save() reports e.message, and "could not delete the half-written
file" is not what to tell someone whose disk just filled up.
Two smaller decisions in the control flow. The whole `use` is guarded, not just copyTo:
a close() that throws while flushing IS the disk-full case and it arrives after copyTo
has returned successfully, so guarding only the copy would miss exactly the failure this
commit is about. The cost is that a file whose every byte reached the provider before a
failing flush is deleted too, which is the right way round -- a flush that failed means
the bytes are not durably there. And openOutputStream's own failure is deliberately
OUTSIDE the guard: nothing has been written at that point, so there is nothing of ours to
remove.
Nothing else in the class moves. hasSpaceFor, discardStaged and sweepStaging are
untouched, and so is save(): the staged file is still deliberately kept on a failed save,
for the reason its own comment gives -- it may be the only copy of an hour of
transcoding, and now the destination genuinely does not have it either.
Seven tests, JVM, Robolectric. The failure has to be injected, which is what shapes them:
Robolectric's ShadowContentResolver consults its registered-stream map before it reaches
any provider, so a test can hand out a stream that writes 512 bytes to a real file and
then throws "No space left on device" while a fake provider answers the size query and
the delete against that same file. The provider is registered twice under two authorities
and two component names, once with the ACTION_DOCUMENTS_PROVIDER intent filter and once
without, which is the only way to have a content URI that is not a document URI. The
delete is observed by the wire names DocumentsContract.deleteDocument actually sends --
"android:deleteDocument" and the "uri" extra -- because both constants are hidden from
the public SDK.
fails partway leaves nothing RED before the fix: expected the delete, got []
close that fails while flushing RED before: the file was still there
cleanup that fails RED before: suppressed was []
already held bytes is not deleted green before the fix -- see below
not a document is left alone green before the fix -- see below
cannot be opened at all green before, and must stay so
succeeds, every byte, no delete green before, and must stay so
The last four are green against the unfixed code for a reason worth writing down: the old
publish() never deleted anything, so every guard passes trivially. They pin the guards
rather than the defect, and pinning is only worth something if the pin is real, so both
were reverted on their own:
- dropping `if (destinationWasEmpty)` fails "a destination that already held bytes is
not deleted" with "a document this app did not create must survive"
- dropping the isDocumentUri() check fails "a destination that is not a document is
left alone" with "deleteDocument has no business on a URI that is not a document
expected:<[]> but was:<[content://...test.plain/document/holiday_plain.mp4]>"
Both were restored. 193 unit tests green.
UnopenableUriTest's unwritable-destination case (an authority with no provider behind it)
is unchanged and keeps passing by two independent routes: the size query on a dead
authority yields "not known to be empty", and the failure itself comes out of
openOutputStream, which sits outside the guard. It is an instrumented test and was
compile-verified here, not executed -- instrumented tests do not run on this host
(CLAUDE.md). Its JVM twin is in the list above.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
ce4d0ff7d4 |
Keep the selected tab through the recreation targetSdk 37 guarantees
AppRoot held the selected tab in `remember`, which survives recomposition and nothing
else. MainActivity declares no configChanges, so every rotation and every resize
destroys and recreates the Activity, and the tab went back to Convert each time.
The KDoc directly above that line is the argument for why it matters: from targetSdk 37
Android ignores screenOrientation, resizableActivity and the aspect-ratio limits on any
display at least 600dp wide, and the Android 16 opt-out is gone, so the app is resized
and rotated whether or not it is ready. The shell was written for that case and then
lost its own state to it. Both ViewModels are Activity-scoped and come back intact, so
a conversion in flight was never at risk -- only the tab, which is what makes this
visibly wrong rather than merely stale.
rememberSaveable, with a Saver that writes the constant's NAME. Three ways to make an
enum saveable and the reasons for this one:
- autoSaver already accepts it. An enum is Serializable, so plain
`rememberSaveable { mutableStateOf(Destination.CONVERT) }` compiles, works, and
passes the restoration test below unchanged. That is a reason to be explicit, not a
reason not to be: nothing in the declaration says Destination has to stay
Serializable, so the implicit route keeps working right up until someone makes it a
value class or a sealed interface -- and then stops, silently, on a path only a
rotation reaches.
- The ordinal is a position, not an identity. Inserting a tab between Convert and Join
would redefine every value already written down. A name only changes when someone
renames a constant, which is an edit that shows up in a diff. It also reads as
itself in a Bundle dump.
- An unknown name restores to null, which rememberSaveable treats as "nothing saved"
and falls back to Convert. That is exactly what a downgrade or a renamed constant
leaves behind, and Convert is the right answer for it.
The test runs on the JVM, which took two changes to reach.
AppRoot and Destination are `internal` rather than `private` -- the unit test source set
is a friend of main, so this stays invisible outside the module -- and AppRoot takes its
`content` as a defaulted parameter instead of calling Content() directly. Nothing in the
app passes it. It is there because both screens resolve a ViewModel, which builds a
WorkManager and a media probe, and none of that has anything to do with which tab is
selected; the test hands in a tagged Box and drives the shell alone. Content() stays
private and is still what the app gets.
compose-ui-test-junit4 joins the JVM test source set. It was already in the catalog for
androidTest, it is inside the prerelease guard via its androidx. group, and its version
comes from the BOM, so this adds no new pinning argument. It is there because
createComposeRule() runs under Robolectric: a red test in androidTest is one nobody on
this host can execute (CLAUDE.md), which is not a loop anyone can work in.
ui-test-manifest is NOT repeated on that source set. It supplies the ComponentActivity
the rule launches, and the existing debugImplementation entry already puts it in the
merged manifest the unit tests build against -- checked by removing the line and
watching AppRootRestorationTest stay green, rather than assumed.
What the test does and does not prove. StateRestorationTester's
emulateSavedInstanceStateRestore() disposes the composition and rebuilds it, so anything
held only by `remember` is gone -- that is what makes it bite. It saves into an in-memory
map rather than parcelling through a Bundle, so it cannot tell a name from an ordinal
from autoSaver. The saved representation is pinned separately by three pure-JVM tests
over the Saver itself, which is where the choice above is actually held down.
Verified by writing it red first, against the restructured AppRoot with `remember` still
in place: all three restoration tests failed at the post-restore assertion with
"Expected exactly '1' node but could not find any node that satisfies:
(TestTag = 'content:JOIN')", while every assertion before the restore -- including the
bar's own selected state -- passed. Six new tests, 186 green in all.
Not addressed here, and deliberately: MainActivity still declares no configChanges, and
should not. Handling the configuration change is not the same as keeping one enum, and
Compose's saved-state machinery is the mechanism the platform intends for it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-dev
|
308333cb49 | Merge commit '8bc2a33' into scratch/integrate-d2-d3 | ||
|
JMR-devandClaude Opus 5
|
8bc2a337d0 |
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> |
||
|
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>
|
||
|
Jason Ross
|
903b43c30a |
Merge pull request #5 from JMR-dev/chore/lint-and-format-parity
Add lint, formatting and static analysis; move the toolchain to Java 25 |
||
|
JMR-devandClaude Opus 5
|
39e0900928 |
Adapt LibreMail's emulator instrumentation for the E2E matrix
The E2E legs could fail with almost nothing to show for it. The previous handler
was a single line of semicolons printing meminfo and 60 lines of crash logcat,
and it only ran when gradle RETURNED non-zero -- a hang left nothing at all, and
`adb logcat -d` at the end only holds whatever survived in the ring buffer, which
a chatty run evicts.
The two failure shapes want different evidence, so they are handled separately:
FAILED -- gradle returned non-zero. The test reports already say which test and
why, so this captures the surrounding state: guest memory and
storage, whether the app even installed, native crashes, and the
runner's own kvm/memory/disk.
WEDGED -- gradle never returned and the wrapper timeout killed it. There are no
reports, so the evidence has to come off the live device: which test
was in flight per the TestRunner logcat, whether the binder services
are published, and SIGQUIT thread dumps of both processes. That last
one is the point -- ART writes full stacks to logcat and /data/anr,
which is what separates a deadlocked test from a stuck MediaCodec
from a device that stopped answering. dumpsys media.player is in
there because both engines transcode through MediaCodec, so a hung
conversion shows up in it.
Logcat is now streamed to a file from the start of the step and uploaded whichever
way the leg goes, since the leg worth reading is usually the one that went red once
and green on re-run -- by which time the emulator is gone.
It is a script rather than inline YAML because it has to be. The action splits its
`script` input on newlines and runs each line as its own `sh -c`, so functions and
`if` blocks cannot survive there; that constraint is what produced the one-line
handler in the first place. One line calls the script now.
The wrapper timeout is 1200s against measured ~5-minute healthy legs, so it cannot
trip on a slow-but-working run, and sits far enough under the 60-minute cap to
leave room for the capture. It wraps only the foreground gradle client, never the
emulator the action owns, so it cannot hang the leg itself.
Not adopted from LibreMail: the hand-provisioned AVD boot, its SDK-integrity
installer and its focus gate. Those answer failures this repo has not had, and
replacing a boot path that works to fix problems we do not have is how a working
matrix breaks. Every emulator setting here -- ram-size, disk-size, the ABI filter,
swiftshader -- is untouched, along with the reasoning already written next to it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
3c7b4b9063 |
Stop the prerelease guard from rejecting AGP's own test platform
All four E2E legs failed to resolve :app:connectedDebugAndroidTest: Could not find com.google.testing.platform:android-device-provider-local:0.0.9-alpha04 That is AGP's Unified Test Platform, the thing that actually runs instrumented tests, and the guard added two commits ago was rejecting it. The guard was written as a blanket rule over every configuration, and AGP resolves its own tooling through this project's configurations. There is no stable version to move to, and there never has been: every module in com.google.testing.platform has only -dev and -alpha releases, going back to 0.0.1-dev. Nor is the version ours to choose -- AGP 9.3.1 pins it through com.android.tools.utp:android-test-plugin-host-additional-test-output:32.3.1. So an allowlist entry would have been the first of several, one per prerelease tool AGP happens to depend on, discovered one red matrix at a time. Scoping the guard to the groups this project actually floats fixes the class rather than the instance. It also retires the detekt exception: we do not float dev.detekt, so the guard now has no opinion about it, where before it passed only because "alpha.6" has a dot the pattern missed. Worth naming the shape of this bug, because it is the second time in this branch that a change looked complete locally and was not. Nothing that runs without a device resolves the UTP configurations -- unit tests, ktlint, detekt, lint, assembleDebug and assembleRelease were all green while connectedAndroidTest could not resolve at all. Verified now by resolving that exact configuration directly, and by confirming the guard still does its job: lifecycle 2.+ resolves to 2.11.0, not 2.12.0-alpha01. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
8727fccba1 |
Declare read-only permissions on the status-check workflow
CodeQL flagged the new static-analysis job for relying on the repository's default GITHUB_TOKEN scope. Fair, and the repo already holds the opposite opinion elsewhere: build.yml's release job spells out contents: write with a comment saying the token's reach should be visible at the point of use. Set at workflow level rather than on the one job that was flagged, because none of these four write anything -- they read the code, build it and attach reports. It also means a repository default that widens later cannot quietly widen these jobs with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
fd6e5325cf |
Raise Kotlin to 2.4.10 so the bytecode can join the toolchain on Java 25
The previous commit settled for Java 24 everywhere because Kotlin 2.2.10 refuses jvmTarget 25. That was the wrong constraint to accept, for two reasons. The first is that 24 turned out to be unbuyable. Adoptium's repository carries 8, 11, 17, 21, 25 and 26 -- no 24, because it is a non-LTS that went end of life in July 2025. The builds passed only because Gradle quietly auto-provisioned 24.0.2+12 through foojay, and .idea/misc.xml had been pointed at a temurin-24 that cannot be installed. A toolchain nobody can install is not pinned, it is lucky. The second is that the cap was never on the toolchain at all. Kotlin's ceiling applies to jvmTarget -- the bytecode -- and the JDK running the build is a separate axis. Conflating them is what steered this at 24 in the first place. So the fix is the one the sibling repo already uses: put KGP on the root buildscript classpath, where AGP's built-in Kotlin picks it up instead of the 2.2.10 it bundles. Kotlin 2.4.10 supports jvmTarget through 26, which lifts the ceiling above the toolchain rather than under it. The Compose compiler plugin is versioned in lockstep and reads the same catalog entry, so the two cannot drift, and the module now applies both by id() because they come from the classpath rather than from plugin resolution. Checked rather than assumed, since a silent downgrade would look identical to success: compiled classes report major version 69, which is Java 25. D8 dexes them, R8 minifies them, and ktlint, detekt, lint, the unit tests and the androidTest compile are all green on top. 25 is the right landing place independent of all this: it is LTS, it is in the Adoptium repository, and temurin-25-jdk is already installed here -- so the daemon runs on a real system JDK rather than a provisioned copy of an unpatched one. Two catalog plugin aliases went with it. android-application and kotlin-compose now resolve from the buildscript classpath, so leaving aliases behind would have left two entries that read like the source of truth and control nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
a7aa003020 |
Fix three ways the floating versions could have gone wrong quietly
All three shared a failure mode: the build stays green while doing something other than what the config says. composeBom was "2026.+". The Compose BOM numbers as YYYY.MM.PP, so the year is the major -- that float stops finding releases on 1 January 2027 and keeps building happily against a frozen BOM, with nothing in CI or the diff to say so. Bare "+" now, which is safe only because the prerelease guard is there. smart-exception was floating on "0.+". Under semver a 0.x minor may break, and this library is load-bearing precisely where breakage hides: the ffmpeg-kit wrapper reaches for smartexception.java.Exceptions only when a call FAILS, so a moved class shows up as an R8 missing-class error at release, or as a crash on the error path -- the least-exercised code in the app, by its own comment. Pinned, with that written down. It was noticed while the float was being written and shipped anyway, which is the actual mistake here. The prerelease guard permitted detekt's alpha by accident. The pattern wanted digits straight after the marker word, and detekt reads "2.0.0-alpha.6" with a dot -- so it passed on punctuation. Had it read "alpha6" the build would have broken with no way to see why from the config. There is now an explicit prereleasePermitted set, and the pattern tolerates both spellings, so the exemption is a decision instead of a coincidence. Also corrects a comment that was confidently wrong: componentSelection rejects STATIC prerelease versions too, not only floating ones. Naming "2.12.0-alpha01" in the catalog does not get you that alpha, it fails to resolve -- verified, not assumed, because the obvious guess is the opposite. Prereleases are taken by adding the group to prereleasePermitted. Two stale references to Gradle 9.5 updated to 9.7.1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
e9542d2223 |
Let the libraries float on minor and patch
Library versions now read "1.+" instead of "1.19.0". Three groups stay pinned, and the reasons differ: agp/kotlin/ksp are version-locked to each other -- AGP 9.3.1's POM declares kotlin-gradle-plugin 2.2.10, so a float that picked up Kotlin 2.4.x would put the Compose compiler ahead of the Kotlin AGP actually compiles with. ktlint/detekt/jacoco because a linter is not a library. A library bump that misbehaves usually still compiles; a new lint rule makes files nobody touched stop passing, turning a PR red for something absent from its diff. Upgrading those is worth a commit that reads the new findings. The FFmpeg AAR is a committed file, not a coordinate. The componentSelection block is the part that makes this safe rather than the part that makes it work. Gradle resolves "+" to the highest version it can find and does not skip prereleases, and androidx routinely publishes alphas numbered above the current stable: lifecycle 2.12.0-alpha01, work 2.12.0-rc01, navigation 2.10.0-rc01, datastore 1.3.0-alpha10, annotation 1.11.0-alpha01 all outrank the releases this app uses. Without the guard, five dependencies would have moved onto unreleased code on the next build with nothing in the diff to say so. With it, every float resolves to exactly the version that was pinned before -- checked against :app:dependencies, not assumed. So this changes nothing today. Every library was already at its newest stable when the catalog was audited; floating is about what happens next month, not this commit. Trying a prerelease is still possible: name the exact version, which pins it rather than floating it. That is the right way round -- an alpha should be a deliberate act with a version number attached to it. Verified: ktlint, detekt, lint, unit tests, androidTest compile, assembleDebug all green, and the configuration cache still reuses across runs of the same task set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
3841f58c74 |
Put the whole toolchain on Java 24, and take Gradle to 9.7.1
Java was scattered across four numbers that nobody had chosen together: the
daemon ran on 25 (pinned in gradle-daemon-jvm.properties), CI installed 17, the
IDE was set to 25, and the app compiled to 17 bytecode. Now all four say 24.
24 rather than 25 because 25 is not reachable end to end. Kotlin 2.2.10 refuses
jvmTarget 25 outright -- "available targets are 1.8 ... 23, 24" -- so the app's
bytecode could never have joined a 25 toolchain, and "everything on the same
version" would have stayed false in the one place it is hardest to notice. 24 is
the highest number all four can actually hold. Checked, not assumed: D8 dexes
Java 24 class files, and R8 full mode minifies them, so the shipped artifact
builds on this too.
Floating where floating is native:
- java-version: '24' -- setup-java resolves the newest 24.x at run time.
- toolchainVersion=24 -- Gradle reports it as "Compatible with Java 24, any
vendor", and provisions whatever 24.x it finds or downloads.
The Gradle wrapper deliberately does NOT float, because it cannot: distributionUrl
names one archive and distributionSha256Sum is the checksum of that exact file.
That pairing is the wrapper's integrity check, and it is the same reasoning the
workflows already apply to action SHAs. Set via `./gradlew wrapper`, not by hand,
so the checksum matches the URL.
Dependencies were audited against Google Maven and Maven Central rather than
guessed at, and almost everything was already current: AGP, the Compose BOM,
core-ktx, activity, lifecycle, navigation, work, datastore, media3, room,
documentfile, annotation, espresso, androidx-junit, junit and ktlint are all at
their newest stable. Only two had moved -- detekt to 2.0.0-alpha.6 and JaCoCo to
0.8.15 -- and both are here.
Kotlin stays at 2.2.10 and that is now recorded as a verified fact rather than a
warning: the AGP 9.3.1 POM declares kotlin-gradle-plugin 2.2.10 at runtime scope,
which is what AGP's built-in Kotlin actually compiles with. Android lint suggests
2.4.10 and taking that suggestion breaks the build unless KGP is also forced onto
the root buildscript classpath. agp, kotlin and ksp move together or not at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
f4962913e1 |
Correct the JDK claim, and finish the @UnstableApi propagation
Two things the first pass got wrong. CLAUDE.md said "use a JDK 17-21, AGP 9 does not support 25+". That was carried over from the sibling repo and is not true here: gradle-daemon-jvm.properties pins toolchainVersion=25, so Gradle provisions and runs the daemon on Java 25 whatever JAVA_HOME says -- JAVA_HOME only picks the launcher. `gradlew --version` prints both, and shows them differing on this machine right now. It also means CI's java-version: '17' is not the JDK that compiles anything, and that the daemon JVM is the same on a laptop as on a runner, which is a better guarantee than the one the file claimed. Marking ConversionDependencies @UnstableApi propagates to its callers, and FakeFailures in androidTest calls it. That is a warning rather than an error in Kotlin, and lint does not read the androidTest source set, so the previous commit compiled clean while leaving one file inconsistent with the very pattern it described. Marked now. Left alone deliberately: gradlew.bat. The new `*.bat text eol=crlf` attribute governs how it is checked out from here on, which is the point of adding it, and the file already has CRLF in both the tree and the index. Rewriting the stored bytes of the wrapper script to prove the attribute works is not this branch's business. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
65a94b4ec1 |
Clear the 35 findings the new tools reported
detekt found 29 and Android lint 6, on a codebase neither had ever seen. Each
one was either fixed or relaxed with the reason written next to it; nothing was
suppressed to make the build quiet.
Fixed, because the tool was right:
- ConversionDependencies constructs Media3Engine, which is @UnstableApi, and
was not marked. Every other type here that touches Media3 propagates the
marker rather than swallowing it with @OptIn, so this one does too. Lint
was the only thing that had ever noticed.
- MediaProbe converted microseconds to milliseconds with a bare 1000, twice,
in a file that also handles a seconds-based duration from a different API.
US_PER_MS and MS_PER_SECOND now say which is which -- that confusion is a
real bug source in media code, not a style question.
- Foreground service types compared SDK_INT against 34 and 35 as raw ints
while the doc comment above spelled the version names out. VERSION_CODES
says it in the code.
- take(3) is a product decision about how many alternatives an error offers.
It means nothing until it is named; MAX_SUGGESTIONS does.
- setProgress(100, ...) is a percentage max, now PERCENT_MAX.
Relaxed, because the rule did not fit:
- The model package is excluded from ReturnCount and CyclomaticComplexMethod
ONLY. It is the decision layer: ConversionRouter.route scores 17 because
the app can give 17 distinct answers to "which engine, and why", each with
its own user-visible reason, and route's own comment records that their
ORDER decides which message is shown. Counting those as complexity measures
how many answers exist, not how hard the code is to follow. Everything else
-- LongMethod, NestedBlockDepth, ComplexCondition -- still applies there.
- A flat `when` used as a lookup table scores a point per entry, so
MediaProbe's demuxer-name to Container map read as complexity 21 with no
nesting and no state. ignoreSingleWhenExpression is the rule's own answer.
- TooGenericExceptionCaught off. MediaProbe, ConversionWorker and ConcatWorker
sit in front of native code that reports a malformed file as anything from
IllegalArgumentException to a bare RuntimeException, undocumented.
Enumerating that list means guessing, and a wrong guess crashes the app on a
file it could have reported as unreadable. SwallowedException stays on, so
these still have to log and handle.
- SI thresholds in the byte formatter, via ignoreNumbers. Each literal sits on
the line with the unit string it belongs to; BYTES_PER_MB would need a Long
and a Double and say nothing the line does not.
- allowedFunctionsPerObject, which the first pass simply missed.
Lint's three version-freshness nags are off. They do not describe this code --
they go red the day someone else publishes a release, which turns a PR red for
something its author cannot see in their diff, and they want the network at
lint time. Upgrades here are deliberate; Kotlin in particular is pinned to AGP's
bundled KGP and is not free to follow the newest release.
UsableSpace is informational rather than disabled, because it is a real finding
that this commit is choosing not to act on. hasSpaceFor reads File.usableSpace,
which ignores reclaimable cache, so the app can refuse a conversion it had room
for. StorageManager.getAllocatableBytes is the better answer, but it changes
when a job is rejected and can throw -- a behaviour change to a safety check,
which deserves its own commit and its own test rather than a drive-by here.
informational keeps it in every lint report instead of hiding it.
Also adds the CI gate and a CLAUDE.md. Coverage is reported and not gated: the
measured baseline is 31% of lines, which is exactly why LibreMail's 0.84 floor
was evidence about LibreMail and not a number to copy.
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
|
93b1fbd5a9 |
Give this project the lint and formatting setup LibreMail already has
There was none: no .editorconfig, no static analysis, and CI ran only tests. Style was whatever the IDE happened to do, which is fine until two of them disagree. The split is LibreMail's, because it is the one that avoids arguments between tools: ktlint owns formatting, detekt owns static analysis with its formatting ruleset left off. Neither can contradict the other about the same line. Adapted rather than copied. LibreMail is Gradle 9.6 / JDK 21 with the configuration cache off; this is Gradle 9.5 / JDK 17 with it on, and Kotlin lives under src/main/java rather than src/main/kotlin -- so its plugin versions were evidence, not proof. Verified here before committing: both plugins resolve, ktlint reads src/main/java, and the run stores a configuration cache entry rather than tripping over it. detekt.yml carries only what applies. The Compose relaxations transfer intact -- a @Composable function is legitimately long, PascalCase, and full of dp literals no matter which app it is in. LibreMail's ForbiddenImport guard and its LargeClass exclusions do not: they name an AppLog facade and two test files that exist over there and nowhere here, and config that guards nothing is worse than no config, because the next reader has to work out that it is dead. detekt 2.0 is an alpha. That is not a preference: stable 1.23.x stops at Gradle 8.12 and this project is on 9.5, so there is no other line to be on. Coverage is reported, not gated. A floor needs a measured baseline, and the JVM test stack here is still junit-only -- a number picked before measuring would either fail on day one or mean nothing. Android lint gets warningsAsErrors because the other two tools fail on any finding, and a gate that stays green while its report fills up is not a gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Jason Ross
|
20a8049719 |
Merge pull request #4 from JMR-dev/feat/remux-and-codec-matrix
feat(media format) - Let the user pick a container and codecs independently and remux without re-encoding |
||
|
JMR-devandClaude Opus 5
|
b5d5fcfeff |
Merge Media3's MP4-only correction into the remux branch
CI on the parent branch proved that four of the five containers the router claimed for Media3 cannot be written by Transformer at all: WebmMuxer, OggMuxer, WavMuxer and AacMuxer each throw UnsupportedOperationException from addMetadataEntry, which MuxerWrapper calls for every metadata entry on the track format. Consequences here beyond the merge itself: - MEDIA3_MUXABLE_VIDEO and MEDIA3_MUXABLE_AUDIO drop to a single MP4 entry. Every other container is already on its way to FFmpeg before those maps are consulted. - Reason.WEBM_CODEC_UNSUPPORTED is removed. WebM now fails the container check first, so nothing could ever produce that reason, and a routing reason no code path can reach is worse than no reason at all. - Media3Muxers gains null branches for the six containers this branch adds. MOV is among them despite being MP4's own family: Mp4Muxer exposes no QuickTime file format. - The README no longer claims Media3 writes five containers. The remux behaviour this branch exists for is unaffected: MKV -> MP4 was always the hardware direction, because Media3 reads Matroska but has never been able to write it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |