795b456d94a093df3c3ae6c3faa80bcd4e70ea3b
262
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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.
|
||
 JMR-devandClaude Opus 5
|
2bed40d080 |
Hold the three pickers to the constant they hand back
The format, quality and engine pickers are the same dozen lines with a
different enum substituted, and both ways they can go wrong are silent.
An onClick that closes over the picker's `selected` parameter instead of
the chip's own entry returns one constant for every chip; an inverted
`entry == selected` lights every chip but the right one. Neither throws,
neither changes the labels on screen, and a test that only asserted the
callback ran would pass over the first of them.
So each click test presses every chip in the row and compares the whole
recorded list against `entries`, which makes the constant load-bearing
rather than the click count, and each selection test asserts over every
chip rather than only the one that should be lit.
Verified by mutation, not by the suite going green:
- `onSelect(format)` -> `onSelect(OutputFormat.MP4_H264)` fails with
`expected:<[MP4_H264, MP4_H265, WEBM_VP9, ...]> but was:<[MP4_H264,
MP4_H264, MP4_H264, ...]>`
- `format == selected` -> `format != selected` fails both format
selection tests on `Selected = 'true'` for a chip that should not be
- `onSelect(preference)` -> `{}` fails with `expected:<[AUTO,
PREFER_HARDWARE, FORCE_SOFTWARE]> but was:<[]>`
- `selected.description` -> `QualityTier.FAST.description` fails the
quality prose test on the missing BEST line
Labels are read off the enums so a reword cannot redden this file for
the wrong reason. `EnginePreference` has no label of its own, so the
screen's own `label()` supplies that set. The one display literal with
no symbol behind it, the custom-spec line, was copied out of the source
byte for byte because it holds a U+2014 that would fail silently if
retyped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Jason Ross
|
175472ae88 |
Merge pull request #65 from JMR-dev/test/r38-1-screen-test-seam
Make the screen leaves nameable from a test, and give tests a tag vocabulary |
||
|
JMR-devandClaude Opus 5
|
df2e42a2b3 |
Name the screen leaves, and give the tests a tag vocabulary
Kotlin `private` on a top-level declaration is file-scoped, so every leaf composable in the two screens was invisible even to the JVM test source set, which is a friend of main. The only three declarations src/test could name were ConverterScreen, JoinScreen and AppRoot -- there was nothing to write a test against, which is why #52 could not be started as filed. `internal` is the same choice MainActivity already documents for Destination: the unit tests can name it, and it stays invisible to anything outside the module. Eleven declarations in ConverterScreen and FileRow in JoinScreen. The tag table is the other half. Tests reference a symbol rather than a literal, which is what keeps the five children that follow independent: "Cancel", "Start over" and "Save file" are each rendered by both screens and by more than one state branch, so rewording one would otherwise redden several PRs at once and no diff would explain why. There were zero testTag, semantics or contentDescription calls anywhere in main before this. Every tag is applied inside main. A tag a test hands down as a Modifier proves only that the test set it -- it would survive the affordance losing its own tag entirely, which is the vacuous shape CLAUDE.md records nine of in one review. That is why FileRow and DetailRow derive theirs from data they already hold rather than taking an index from the call site. TestTags is public rather than internal, and R38.8 is the reason. It reads the table from androidTest, and whether that is a friend source set of main under AGP 9 had no in-tree answer -- nothing referenced a main internal from there. Settled by compiling one: it is a friend, so internal would work today. Public anyway, because that friendship is AGP wiring rather than something this project states, and the KDoc now carries the measurement so nobody has to repeat it. Smoke tests cover each leaf: it renders, and its tag resolves to exactly one node. Counting rather than asserting existence is deliberate, since a duplicated tag fails differently depending on which finder a later test happens to use. The state-branch tags -- Convert, Cancel, Save file, Start over, the progress bars -- have no bite yet: reaching a branch needs the state seam R38.5 extracts, and the state matrix is R38.6/R38.7 by design. Five mutations, all red on the named test alone: FORMAT_CHIPS deleted, FILE_CARD_NAME deleted, detailRow's tag no longer derived from its label, FileRow's no longer derived from its name, and JOIN_MORE given JOIN's value -- the last caught only by the uniqueness check, which is what it is for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
64c1a60a97 |
Drain the escaped coroutine error before the next test starts
`ConversionViewModelProbeFailureTest.an OutOfMemoryError is not swallowed` deliberately lets a real error escape `viewModelScope.launch`, which has no exception handler by design -- the ViewModel's KDoc says an OOM raised in the probe should reach the thread's handler and take the process down. On the JVM something else takes it. kotlinx-coroutines-test installs a process-wide collector for uncaught coroutine errors, keeps whatever it catches, and hands the backlog to the next `runTest` that starts, which throws UncaughtExceptionsBeforeTest. Every Compose test is a `runTest`: that is how `createComposeRule` runs a composition. So the error lands on an unrelated test in an unrelated file, and the message names neither the test that caused it nor the error's origin. Nothing has hit it yet only because the sole Compose test in the repo happens to run before the ViewModel one. R38 adds six more Compose classes in exactly the two packages that surround it, and the first two of them made the suite fail in two different files on two consecutive runs of identical, green code -- the throw is on a real Dispatchers.IO thread, delivered after the state assertion that ends the test responsible, so which class catches it is a race. Drain it where a Compose rule is built. A @Before cannot: the rule's `runTest` wraps the statement that calls it, so it has already thrown. @BeforeClass cannot either, because Robolectric runs it outside the sandbox classloader, where the collector is a different object. Constructing the rule is early enough, since JUnit builds a fresh test-class instance -- and every @get:Rule field on it -- before evaluating any rule. This is containment, not the cure. The cure is a seam: give the probe hop an injectable dispatcher the way the constructor already does for cleanupDispatcher, so the error has somewhere to land. That is a production change and deserves its own commit. kotlinx-coroutines-test was already on the unit-test classpath through compose-ui-test-junit4; it is declared now because a file imports it. Pinned, like robolectric and the linters: org.jetbrains.kotlinx is not one of the groups the prerelease guard covers, so a float here would be free to take a milestone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
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 |