6cd17f25aa01c3d20527add32a9313d522bf13ef
109
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 JMR-dev
|
6cd17f25aa |
Pin shellcheck, because the unpinned one disagreed with the local run
The step added in the previous commit went red on its own PR, and the reason is the one
CLAUDE.md already gives for pinning ktlint, detekt and JaCoCo: "a new rule in a linter
makes files nobody touched stop passing, so CI goes red on a PR whose diff cannot explain
it." Here it was not even a new rule, just a different version of the same tool.
The runner's ambient shellcheck is 0.9.0. The container used to check locally was 0.11.0.
They disagree about how to report `on_signal`, which is installed as the INT and TERM trap
eleven lines below its declaration and so is never called by name:
0.11.0 SC2329, once, on the function declaration -- "never invoked"
0.9.0 SC2317, seven times, one per command in the body -- "appears to be unreachable"
The disable directive named SC2329, so 0.11.0 was silent and 0.9.0 reported seven findings.
Nothing about the script was wrong; the local check simply was not the check CI ran.
Two changes, because either alone still leaves a way to be surprised:
- CI runs shellcheck from an image pinned by digest, so an upgrade is a line in this
file that someone chose, not something that arrives on a Tuesday. The version is
still printed, so a finding out of nowhere can be tied to that line.
- The directive names SC2317 and SC2329 both, so a contributor whose distro ships 0.9.0
gets the same answer locally as CI gives. Verified against both images: clean under
0.9.0 and clean under 0.11.0.
CLAUDE.md now says to check with the pinned digest rather than with whatever is installed,
which is what would have caught this before the push.
|
||
|
JMR-dev
|
baaaa934e0 |
Check the shell, and stop one-off issues falling off the board
Two gaps, both found the same way -- by something going wrong quietly.
`gh issue create` does not touch the project board. The issue is created, carries its
labels, and is invisible in the Kanban, which looks exactly like a ticket nobody filed.
On 2026-08-24 eight issues filed as a scripted batch all reached the board and one filed
as a one-off minutes later did not; it surfaced only because someone went looking for it.
A batch carries the board step inside its loop. One-offs are where it slips, so
tools/github/file-issue.sh is for one-offs.
Three things it does that a two-command shell snippet would not:
- Resolves the project, Status field and option ids BY NAME, every run. Caching them
is the obvious optimisation and the wrong one -- a renamed or reordered column would
then have this writing a stale id into the board with no error anywhere.
- Reads the item back. A mutation returning 200 says the request was accepted, not that
the board shows what was asked for; the read-back is the only step that checks the
claim this script exists to make. It is a GraphQL query because REST cannot do it --
the `fields` array REST returns on a project item carries Title and nothing else, so
a REST-only check reports every item's Status as unset.
- Exits 3, loudly, with the issue number on a line of its own, when the issue was
created but the board step failed. That exact combination is the failure being
prevented; it must never be the quiet path.
Shell was the other language here with nothing checking it -- four scripts, one of them
the CI entry point. shellcheck now runs in the Static analysis job over
`git ls-files '*.sh'`, so a script added later is covered without editing the workflow,
and it runs at full severity with `info` included.
That raises two findings today and both are the tool being wrong, so both are answered
with a targeted `disable` carrying its reason rather than by lowering the severity:
run-e2e.sh's `on_signal` is reported as never invoked when it is installed as the INT and
TERM trap eleven lines below it, and the `$names` inside file-issue.sh's queries are
GraphQL variables that must not expand -- expanding them would send the shell's idea of
$owner to the API instead of declaring a parameter. A blanket --severity=warning would
have hidden both, and the next real finding with them.
The gradle step gains `if: !cancelled()` so a shellcheck failure cannot cost the
ktlint/detekt/lint lists -- the same reason that step already passes --continue.
Not covered, deliberately: shellcheck here reads .sh files, not the inline `run:` blocks
in the workflows, where a good deal of this repo's bash actually lives. actionlint does
read them, and finds one pre-existing info-level issue in build.yml. Wiring it in means
pinning a container digest, because every action here is pinned by SHA and actionlint's
usual installer is a curl-pipe-bash off a moving branch. Its own ticket, not this commit.
|
||
 Jason Ross
|
1779f20a03 |
Merge pull request #56 from JMR-dev/ci/api37-split
Split the API 37 CI leg so the part that works can gate |
||
|
JMR-devandClaude Opus 5
|
225ecdd7e6 |
Split the API 37 leg so the part that works can gate
CI has never run the API level this app targets. The reason it did not was never "API 37 is untestable" -- it was that two tests fail on the emulator image, so one row would be permanently red or permanently allow-listed. This splits that row instead of choosing between those two. E2E API 37 gates. It runs 55 of the suite's 57 instrumented tests and must be green. E2E API 37 Media3 hardware transcode runs the other two, reports, and never blocks (continue-on-error). Both are driven off ONE marker, @FailsOnEmulatorApi37: the gating job passes notAnnotation, the advisory job passes annotation. Two lists would drift, and drift is silent in both directions -- a test that ends up in neither job reads as green. Excluding by class was not an option either: Media3EngineTest has four tests and two of them pass here, so notClass would have thrown away real coverage. The advisory job is named for what it runs, not for what we think is wrong. Both its tests drive a full H.264 -> H.265 hardware transcode, which is what distinguishes them from the two Media3EngineTest cases that pass -- those never decode video. The goldfish-decoder theory sits in a comment inside the job, where it can be corrected without renaming a check people have learned to look for; docs/api-37-emulator-crash.md keeps measurement and inference apart. The SystemUI disable moves into .github/scripts/e2e-run.sh behind E2E_DISABLE_SYSTEM_UI, unset everywhere but the two API 37 jobs, so the other four legs run byte-identical commands -- the same shape as E2E_EXTRA_GRADLE_ARGS. It runs BEFORE the streamed logcat starts, deliberately: `adb shell stop` would end that logcat and nothing restarts it, so a disable placed after it would cost the leg its diagnostics for the part of the run that matters. The body is probe v2 from api37-debug.yml -- the version measured 4/4 -- not the older one-round form: three rounds, waits for system_server to actually be gone, verifies against `pm list packages -d`, and requires a 45 s window with zero new aborts. The weaker probe reported success on a run that then started SystemUI eight more times. The caveat is written next to the row rather than left implicit: this leg runs with SystemUI disabled and the framework restarted under it, a device configuration no other leg and no Pixel run uses. Anything that touches system UI must not trust it, and the Pixel check before each release is still the only API 37 run with SystemUI intact. docs/api-37-emulator-crash.md's "So should CI take API 37?" said no on three reasons. Two were claims about CI that had never been measured; the section now carries the eight runs that measured them, and the third reason is what the split answers. docs/local-emulator.md and api37-debug.yml's header carried the same "the matrix stops at 36" claim and are corrected with it. CLAUDE.md is left alone deliberately -- its "CI's matrix therefore stops at API 36" clause is now false, and that correction is parked in the doc's existing "Correction owed to CLAUDE.md" section, where two others are already waiting. Making E2E API 37 an actually-required check is a repository-settings change and must come after this is on main: adding a required context that does not exist on the default branch blocks every PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
b3a705e3da |
Measure the API 36 control and record what CI cannot measure
Three additions to docs/api-37-emulator-crash.md, all from a CI investigation run through .github/workflows/api37-debug.yml. A third measured bullet: API 36 against API 37, back to back, same two tests, same renderer, same SystemUI-disable path. 37.0 fails both on c2.goldfish.h264.decoder (32660148155); 36 passes both in 4.603 s with the same decoder in its logcat (32660152961). That falsifies "the stripped configuration is what breaks these tests" -- a reading the other measurements never addressed, because they all compare against a device that still had SystemUI. It carries its two uncontrolled variables rather than dropping them: API 36's framework restart happened with zero aborts logged where API 37's had two, so a restart under an active abort loop is still uncontrolled; and the images differ on the encoder side, which is a second reason "broken h264 decoder" is the wrong shape of claim. The decoder-mechanism bullet is unchanged and still labelled inference. This adds a measurement next to it; it does not retract anything. The intact-SystemUI counterfactual is unmeasurable on a GitHub runner, and now says why. Seven dispatches, zero verdicts, with a mechanism rather than bad luck: while the framework crash-loops the guest cannot reliably create per-user private directories, so an app installed during the loop has no cache dir and the fixture copy dies in @Before before any codec exists. googlesdksetup and nexuslauncher hit the same thing. The result XML masks it behind an UninitializedPropertyAccessException in tearDown, which reads as a defect in this repository and is not one. Abort cadence corrected. "Roughly every 20 s" was the watchdog's sampling interval, not the cadence: measured gaps are 20-90 s, median 60-70 s, three to five per run, with sys.boot_completed held at 1 throughout. The wrong figure lived in api37-debug.yml's own comments, so that line is corrected too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Jason Ross
|
577dae998b |
Merge pull request #55 from JMR-dev/ci/api37-debug
Verify the SystemUI disable instead of trusting what pm reported |
||
|
JMR-devandClaude Opus 5
|
acc71bcaee |
Verify the SystemUI disable instead of trusting what pm reported
Four dispatches of one configuration -- API 37.0, swiftshader_indirect,
SystemUI disabled -- came back three green and one not, and the odd one out
was not a different failure so much as the same run without the fix applied.
In 32646029143 `pm disable-user` reported `new state: disabled-user` and
SystemUI then started eight more times:
14:41:24 ActivityManager: Start proc 6412:com.android.systemui ... GradientColorWallpaper
14:45:05 ActivityManager: Start proc 17299:com.android.systemui ... GradientColorWallpaper
with ten more RegionSampling aborts and a surfaceflinger pid that never sat
still (489, 1570, 3524, 4396, 6038, 7987, 9732, 11520, 13208, 15048). The
framework is being SIGKILLed every twenty seconds while this runs, so a
package-state change can go down with the system_server that accepted it.
Two things were wrong, and the second is why the first went unnoticed:
- one disable attempt was treated as sufficient
- the wait after `adb shell stop` was not a wait. It asked `service check`
0.3 s later and got `found` from the system_server that was still on its
way out, so it never waited for anything. Both the good and the bad run
printed `services back after 5 s`, which is how a broken fix looked
identical to a working one.
Now: up to three rounds of disable -> take the framework down and confirm
system_server is actually gone -> bring it back -> verify the package is in
`pm list packages -d` -> require a 45 s window with zero new aborts. Nothing
is believed because a command said so.
Also adds measure_baseline, default true. The 45 s pre-measurement is what
makes the rate comparable with the local figures, but it is 45 s of
crash-looping before the disable has to land, which is a worse starting
point than a real leg would have.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Jason Ross
|
b97d7c36a3 |
Merge pull request #54 from JMR-dev/ci/api37-debug
Resolve adb by path in the API 37 watchdog |
||
|
JMR-devandClaude Opus 5
|
93398c4616 |
Resolve adb by path in the API 37 watchdog
The first three dispatches came back with every watchdog sample reading `boot=? surfaceflinger=none zygote64=none dma_aborts=0`, on runs where the device demonstrably booted and the action's own adb was working two steps away. The watchdog was not measuring anything. The emulator action puts platform-tools on PATH with core.addPath, which writes GITHUB_PATH and therefore only affects LATER steps. The watchdog is started before the action -- that is the whole point of it -- so it inherits the runner's own PATH, where a bare `adb` is not necessarily anything. Every call failed into `2>/dev/null` and the sampler dutifully recorded the silence as zero. It now resolves adb by path, preferring ANDROID_HOME, re-resolving on every iteration in case platform-tools arrives later, and echoing the path it settled on. The launch step prints ANDROID_HOME and `command -v adb` for the same reason: a repeat of this failure should be one line to spot, not three runs of quiet zeros. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Jason Ross
|
19a1e66277 |
Merge pull request #53 from JMR-dev/ci/api37-debug
Add a dispatch-only workflow for the API 37 CI question |
||
|
JMR-devandClaude Opus 5
|
8fdad6e20b |
Add a dispatch-only workflow for the API 37 CI question
status_check.yml stops its E2E matrix at 36 and says the android-37.0 image is why. That is established locally under -gpu host and under ANGLE, and it is not established for CI: runners use -gpu swiftshader_indirect, and the one local measurement of that mode was void for a local reason -- Fedora denies execheap to SwiftShader's JIT, so the emulator died before the guest mattered. What CI does at API 37 has therefore never actually been measured. This is that E2E job with the matrix replaced by workflow_dispatch inputs, so a hypothesis costs a dispatch rather than a commit: renderer, API level, image target, channel, SystemUI disable, boot timeout, whether the suite runs at all, and free-form emulator and Gradle arguments. It triggers on nothing else and gates nothing. It calls .github/scripts/e2e-run.sh rather than forking it, and pins the same disk-size, ram-size, action SHAs and KVM setup as the job it copies, so a run here measures the renderer and not a different device. The watchdog is load-bearing rather than decorative. The emulator action calls killEmulator() from its own catch block, so a run whose emulator never boots is torn down before any script: line executes and leaves nothing behind -- which is the exact failure shape API 37 is suspected of. It starts before the action, samples sys.boot_completed, the surfaceflinger and zygote pids and the hasReadColorBufferDma abort count every 20 s, and keeps a rolling copy of the crash buffer so the last read survives the teardown. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Jason Ross
|
742703d360 |
Merge pull request #51 from JMR-dev/docs/definition-of-done
Write down that testable code is not done until it is tested |
||
|
JMR-devandClaude Opus 5
|
6c34fad17c |
Write down that testable code is not done until it is tested
Stated as a project norm: if a piece is unit testable it gets unit tests, and if it is e2e testable it gets e2e tests, before it counts as done. Both clauses, not either/or. Recorded here rather than left as a habit because the recent review measured what happens without it. Forty-six mutations were run against a 257-test suite; thirty-six bit and NINE were vacuous, five of those passing the entire suite while a reattachment code path sat completely unguarded. That code had shipped, been reviewed, and looked tested. "The suite is green" was true and meant nothing. The convention also names the two things that make it enforceable rather than aspirational. Unit-testable is broader than it looks, because the pure-seam pattern converts device-bound logic into a testable function plus a thin edge, and Robolectric now covers the rest including Compose. And e2e is genuinely runnable locally since the emulator renderer cause was found -- until last night, "run the instrumented suite" was not a request anyone could act on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Jason Ross
|
9c4f14202f |
Merge pull request #50 from JMR-dev/docs/coverage-figure
Measure the coverage figure instead of carrying it forward |
||
|
JMR-devandClaude Opus 5
|
2747bb8627 |
Measure the coverage figure instead of carrying it forward
CLAUDE.md has said "~31% of lines" since the lint/format work landed. Measured on main today it is 29.8% (629/2113 lines, 408/1424 branches). The number went DOWN, which is worth stating rather than quietly correcting. The JVM suite went from 11 test files to 43 over the same period, so the intuition -- and the review finding that prompted this, which called the direction certain -- was that coverage must have risen. It did not: main source grew from 4,114 to 5,715 lines as the fixes added Reattachment, JobSnapshots, JobTags, InputQuery, StagingNames, StagingSweep, NativeLoadFailure and an Application class. The denominator outran the numerator. That is not an argument against the tests. It is an argument against quoting a coverage percentage from memory, which is exactly how the stale figure survived. The line now carries the measurement, its date, and the instruction to re-measure. R30 / #39 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Jason Ross
|
6d6d2189ca |
Merge pull request #47 from JMR-dev/tools/api-37-emulator
Re-derive the API 37 emulator failure, and harden the local sweep |
||
|
JMR-dev
|
f8e6bfa2a3 | Merge remote-tracking branch 'origin/main' into tools/api-37-emulator | ||
|
Jason Ross
|
5a1b8832d3 |
Merge pull request #48 from JMR-dev/fix/review-app-gaps
Close eleven review findings in app code |
||
|
JMR-dev
|
0896cef758 | Merge remote-tracking branch 'origin/main' into fix/review-app-gaps | ||
|
JMR-devandClaude Opus 5
|
d3975617a8 |
Put a gate on the three pieces of wiring that had none
Three separate mutations passed the full 257-test suite, all for the same reason: the tool was tested and the thing that calls it was not. The join half of per-job staging. Reverting ConcatWorker to the constant the audit's own D8 table names -- "joined.<ext>", one string for every join of a format -- left everything green: PerJobStagingTest drives only the conversion worker, and StagingNamesTest pins only the pure function. The new case drives two real ConcatWorkers with different ids and reads what they asked for rather than what is on disk, because ConcatEngine is native, so neither join gets past it here and the catch on the way out deletes what it staged. The recorder moves into WorkerStubs, which is what that file is for, and the enum test that already had a private copy now uses it. The process-start sweep. Deleting the one line in LibreMediaConverterApp.onCreate() -- the only reason that class exists, and the backstop for every leak discardStaged cannot reach -- left everything green too. The test stages one file a day old and one written now, calls onCreate() again, and asserts both halves: the abandoned one is collected and the live one is not. The second half is what says this is a sweep rather than the clearStaging() it replaced, which could take a file out from under a running job. The mtime is set explicitly, because "written long enough ago" is not something a test can wait for when the period is twenty-four hours. Casting the Robolectric application to LibreMediaConverterApp is an assertion in itself: it fails if android:name ever stops pointing here, in which case the swept line would be correct code that never runs. The backup and device-transfer exclusions. Reverting data_extraction_rules.xml to the template's boilerplate left the unit tests green AND lintDebug green -- it is a resource, so nothing was reading it -- and the failure it causes is one nobody meets in development. WorkManager's queue is the app's whole backup payload, and its rows name content:// grants and cacheDir paths that do not survive a transfer; reattachment queries by tag on launch, so a fresh install would come up attached to a job the user never ran on it. The test reads the compiled resource table, so what it pins is what the APK carries, and it asserts domain and path for all four entries in both sections -- an <exclude> with no path is skipped unchecked by lint's own detector, so half an entry could protect nothing. Its KDoc records the one thing it does not cover: the manifest attribute that points the system at the file. R8 / #17, R9 / #18, R11 / #20 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
7ae660ee37 | Merge remote-tracking branch 'origin/main' into tools/api-37-emulator | ||
|
Jason Ross
|
e76547fe3c |
Merge pull request #45 from JMR-dev/docs/review-corrections
Correct six documentation claims the overnight review falsified |
||
|
JMR-devandClaude Opus 5
|
775a44753b |
Say that the default sweep is red on purpose, and narrow two claims
R16 / #25 -- the branch put API 37 into the default APIS list, where it is permanently two
failures short of green, so a bare `run-e2e.sh` exits 1 by design and nothing said so.
Somebody running it from habit, a wrapper or a hook gets a red exit forever and either
stops reading exit codes or debugs a normal state.
Documented rather than suppressed. The script's own comment already argued that an
expected-red level belongs in the exit code -- reversing that is the branch owner's call,
not a correction -- and the review's alternative needs an exact-set comparison of the
failing test names before it can subtract 37's contribution, which is a new mechanism that
cannot be validated without a device. So:
- the header now states the exit code (0 all green / 1 any level red / 2 refused to
start), says a bare run is 1 by design and why, and gives `run-e2e.sh 33 34 35 36` as
the sweep that can be green;
- a red sweep prints one note after the summary saying the same thing, because the exit
code is read in the terminal and not in the docs -- but ONLY when 37.x is the only level
that went red. `overall` is set by any red level, so a note keyed on "37 was in the
list" would have called a genuine API 34 failure "by design", which is the defect this
is meant to prevent, one layer up. mark_red records which level it was, where the loop
already knows;
- docs/local-emulator.md says it where the default is documented.
R27 / #36 --
|
||
|
JMR-devandClaude Opus 5
|
3534c6d996 |
Clean up the empty document a refused open leaves, and stop the space sums wrapping
publish() opened the destination stream outside its guarded region, justified by "nothing has been written at that point, so there is nothing of ours to remove". That reasoning is wrong about what exists: SAF's CreateDocument contract creates the document before publish() is ever called -- which is why every fixture in OutputPublisherPublishTest starts as an existing empty file. A provider that then hands out no stream, because it dropped between the picker and the write or simply returns null, left a zero-byte file at the name the user chose while the screen said the save had failed. The open moves inside the try, so the same two bounds that already govern a failed copy govern this: only a document URI, and only a destination positively known to be empty. The dead-provider case is untouched and now demonstrably by the guard rather than by the placement -- nothing answers for that authority, so no size can be read, and "I could not tell" still refuses to authorise a delete. Its test comment said the old thing and now says that one. The space arithmetic overflows in two places, both live on main and independent of the allocatable-versus-usable question that stays parked: - hasSpaceFor computed `free > required + headroom`. A request within 128 MiB of Long.MAX_VALUE wraps that sum negative, and every free-space measurement beats a negative number, so the check answers "plenty of room" to the largest request it can be handed. Rewritten as `free - headroom > required` with both operands clamped at zero, which is the form the parked branch's StagingSpace.hasRoomFor already argues for. - InputQuery.total folded a join's inputs with nothing stopping the sum from wrapping, and that is the reachable half: no single file overflows, three four-exabyte inputs do. It saturates at Long.MAX_VALUE now, which the check above then refuses. SpaceArithmeticTest ties the two together in the shape the defect had -- the total that came out negative is handed straight to the space check -- and keeps one allowed case so the refusals cannot pass by refusing everything. The negative-size clamp is deliberately left unasserted, with a comment saying why: it only changes the answer when free space is below the headroom, which a test reading the host's real cache volume cannot arrange. R6 / #15, R23 / #32 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
a6cf4f4ff4 |
Stop three enum reads escaping doWork, and pin what the attempt bound buys
Both workers read enums out of their input Data with Enum.valueOf, and all three reads sit
ABOVE the try. A name this build does not define -- which is what a downgrade or a
rollback with work still in the queue produces, since WorkManager keeps work for about a
week -- threw IllegalArgumentException straight out of doWork(). That is D13's signature
verbatim: FAILURE with reschedule = false, output Data with zero entries so the screen
says "Conversion failed." and nothing else, and no staged.delete(), so the partial stays
in cache. readSpec() twelve lines below already handles exactly this case, and its KDoc
says why.
So all three take readSpec's shape: entries.firstOrNull { it.name == name } ?: default.
Consistency argues for it as much as correctness does -- the file already contains the
right answer to this question, three times.
WorkerEnumFallbackTest reaches each read. Two of them pin the value that replaces the
unknown name rather than only that nothing threw: a quality tier falling back to something
arbitrary would convert at a setting nobody chose, and a join's format decides the
extension its output is staged with, which is where FFmpeg infers the container from. The
engine-preference case asserts through the space check instead, because predicting which
engine AUTO picks would tie the test to a routing rule it is not about. Against the
unfixed code all three fail with "No enum constant ...".
MAX_FOREGROUND_START_ATTEMPTS had no test of its value. Both existing cases are written
against the symbol, which pins the relationship and leaves the number free: changed to 2,
the job gives up about ninety seconds after process death -- exactly the long conversion
the retry exists to protect -- and all 257 tests stayed green.
The new assertion is the property the KDoc argues, not the literal: summed against
WorkRequest's own DEFAULT_BACKOFF_DELAY_MILLIS and MAX_BACKOFF_MILLIS, the attempts have
to span at least eight hours, which is what makes "the user will have opened the app by
then" a claim rather than a hope. A deliberate re-tune that keeps the property passes; the
accident does not, and reports the span it got (0.025 hours at 2).
R22 / #31, R10 / #19
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
961cfa72a2 |
Derive the suite size instead of writing it down in two documents
R4 / #13 and R20 / #29 are one defect: an absolute test total in an unregenerated document, written the same day it went stale. This branch was cut at |
||
|
JMR-devandClaude Opus 5
|
c5c4c5323b |
Test the edge that feeds reattachment, and stop it reporting ENOENT
Reattachment.choose has twenty tests and every mutation aimed at it bites. Everything that computes its inputs had none, and five mutations there passed the whole 257-test suite. Four are closed here, each verified by applying the mutation and watching the new test go red. jobSnapshots() is the half that has to touch WorkManager and the filesystem, so it is where the untested values live. JobSnapshotsTest drives it against a real WorkManager and a real cacheDir: - A zero-byte staged file is not an output. Relaxing the filter to `exists()` -- which is what a job killed before its engine wrote anything leaves behind -- made the snapshot claim a result, and the user would meet a Save button for a zero-byte "conversion". The same case pins that the path is still reported and that the mtime stays 0 for a file that is not a result. - Each result carries its own file's mtime. Hardcoding it to zero starves the newest-file tie-break of the only data it has, which is precisely the failure the tie-break exists to prevent: the query has no ORDER BY, so an arbitrary winner keeps winning every launch. Timestamps are set with setLastModified and compared against what the filesystem stored, because mtime granularity is not this test's claim to make. ReattachGuardsTest covers the two decisions the ViewModel makes that the pure rule cannot: - A file picked while the query was still in flight is not reattached over. Deleting the guard turns the user's pick into yesterday's job -- with the Save button pointing at a file the card does not name. Made deterministic by holding WorkManager's task executor rather than by racing two IO hops: the query cannot finish until the pick has landed. The test also asserts the brake really gripped, so a reattachment that never arrived cannot pass for one that was refused. - An Ambiguous result is offered without being attributed. Two finished jobs naming one staged file is what the device produced before staging was keyed on the job id; taking the first job's tags labels the file with the other conversion's name, which is the confident lie the KDoc rejects. The neutral label and the absent size are both pinned. ReattachmentTest's FAILED exclusion was only ever tested with pathless FAILED jobs, so a narrow regression ranking a FAILED job that carries a file like a result passed all 257 tests. The live shape is the 2 MB orphan the device pass found: a job killed mid-write leaves a partial, and under that regression the user is offered a truncated file with a Save button. One fixture with outputPath and outputExists set closes it. save() re-checks the staged file, in both ViewModels. The check reattachment made ran inside a tag query that can be hours older than the tap, and cacheDir is what the OS empties when it wants space and what the sweep collects after a day. The file's absence used to arrive as staged.inputStream() throwing, and e.message put "/data/user/0/.../4b4882....mp4: open failed: ENOENT" on screen -- a true statement about a path the user has never seen and cannot act on. It now reads as a sentence with an action in it. The message is one constant next to OutputPublisher because both ViewModels need it and staging is what it is about. Reattachment's KDoc claimed a defect that was fixed in the commit before it -- that "Start over" keeps its staged file -- which would send a maintainer to re-fix D2. Rewritten to say what is actually true: the delete happens, and the gap it leaves is the reset() whose delete is cancelled with the Activity, which is the sweep's job and is named in the sweep's own KDoc. R1 / #10, R2 / #11, R24 / #33, R25 / #34 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
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> |