JMR-dev/LibreMediaConverter
1
0
Fork 0
Code Issues 16 Pull Requests Actions Packages Projects Releases Wiki Activity

R37 — A reattachment can overwrite a pick the user has already made (surfaces as ReattachOnLaunchTest flake) #49

New Issue
Closed
opened 2026-08-23 04:53:26 +00:00 by JMR-dev · 8 comments
JMR-dev commented 2026-08-23 04:53:26 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Observed on CI during the overnight run, not by the review. Filed because everything gets a ticket.

R37 — ReattachOnLaunchTest.doesNotOverwriteAPickTheUserHasAlreadyMade is flaky on CI

severity: medium
verdict: CONFIRMED (observed red once; green on every other run of the same commit range)
where: app/src/androidTest/java/org/libremediaconverter/convert/ReattachOnLaunchTest.kt
scenario: E2E API 35 on PR #47 reported Tests 59/57 completed. (2 skipped) (1 failed) with this
test FAILED. PR #47 changes zero files under app/ — docs and tools/ only — so it
cannot be the cause. The same test passed on PR #8 (which introduced it), on PR #45,
and on PR #48.
evidence: Job https://github.com/JMR-dev/LibreMediaConverter/actions/runs/32617686626/job/97140992869
Execute …doesNotOverwriteAPickTheUserHasAlreadyMade: FAILED, runner
MemAvailable: 1162172 kB, native-crash section empty (so not an emulator abort).
why it is racy:
The test asserts that a user's pick, made while the WorkManager tag query is in
flight, survives a late reattachment. On the JVM that race is now made deterministic —
ReattachGuardsTest (#48) installs a holdable task executor so the query cannot
complete until the pick has landed, and asserts the brake actually gripped. The
instrumented version has no such control: it races a real WorkManager against a real
dispatcher on a shared CI runner, so it is timing-dependent by construction.
fix: Either give the instrumented test the same determinism (a controllable task executor),
or delete it as redundant now that ReattachGuardsTest pins the same behaviour
deterministically on the JVM — the review already noted this instrumented test was
"not executed, only read".
risk: Deleting coverage that looks real is the wrong reflex if the JVM twin does not actually
cover the same guard. Confirm the equivalence before removing anything.

Cut: backlog — a flaky test in CI is exactly the thing not to "fix" unattended at 4am on one
observation. The choice between hardening and deleting is a judgment call.

🤖 Generated with Claude Code

_Observed on CI during the overnight run, not by the review. Filed because everything gets a ticket._ ### R37 — `ReattachOnLaunchTest.doesNotOverwriteAPickTheUserHasAlreadyMade` is flaky on CI severity: medium verdict: CONFIRMED (observed red once; green on every other run of the same commit range) where: app/src/androidTest/java/org/libremediaconverter/convert/ReattachOnLaunchTest.kt scenario: E2E API 35 on PR #47 reported `Tests 59/57 completed. (2 skipped) (1 failed)` with this test FAILED. **PR #47 changes zero files under `app/`** — docs and `tools/` only — so it cannot be the cause. The same test passed on PR #8 (which introduced it), on PR #45, and on PR #48. evidence: Job https://github.com/JMR-dev/LibreMediaConverter/actions/runs/32617686626/job/97140992869 `Execute …doesNotOverwriteAPickTheUserHasAlreadyMade: FAILED`, runner `MemAvailable: 1162172 kB`, native-crash section empty (so not an emulator abort). why it is racy: The test asserts that a user's pick, made *while the WorkManager tag query is in flight*, survives a late reattachment. On the JVM that race is now made deterministic — `ReattachGuardsTest` (#48) installs a holdable task executor so the query cannot complete until the pick has landed, and asserts the brake actually gripped. **The instrumented version has no such control**: it races a real WorkManager against a real dispatcher on a shared CI runner, so it is timing-dependent by construction. fix: Either give the instrumented test the same determinism (a controllable task executor), or delete it as redundant now that `ReattachGuardsTest` pins the same behaviour deterministically on the JVM — the review already noted this instrumented test was "not executed, only read". risk: Deleting coverage that looks real is the wrong reflex if the JVM twin does not actually cover the same guard. Confirm the equivalence before removing anything. --- **Cut:** `backlog` — a flaky test in CI is exactly the thing not to "fix" unattended at 4am on one observation. The choice between hardening and deleting is a judgment call. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
JMR-dev commented 2026-08-24 21:13:44 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Another occurrence, on a PR that touches no Kotlin at all (#69 — a shell script, a workflow step and CLAUDE.md). That makes the "unrelated diff" evidence stronger: this fails on branches whose contents cannot plausibly affect it.

Run 32777393771, E2E API 35, failing at test 13 or so of 57:

org.libremediaconverter.convert.ReattachOnLaunchTest > doesNotOverwriteAPickTheUserHasAlreadyMade[test(AVD) - 15] FAILED
  java.lang.AssertionError: reattachment took the screen from the user:
    Converted(input=InputFile(uri=, displayName=holiday.mp4, sizeBytes=null, probe=null),
              staged=/data/user/0/org.libremediaconverter/cache/conversions/holiday_converted.mp4,
              engineUsed=, routeReason=, suggestedName=holiday_converted.mp4, mimeType=video/mp4)
    expected null, but was:<Converted(...)>

Note expected null, but was:<Converted> — the guard did not merely lose a race, it let a finished job take a screen the user had already moved on from. Worth checking whether the flake is a timing artefact of the test or the guard genuinely having a window.

Frequency is now the problem, not just the noise. This is the third sighting today, all on unrelated branches. #52 is decomposed into eight PRs (#57–#64) and each one runs this leg, so at the current rate it will redden several of them and cost a re-run each time. That moves it from "annoying" toward "blocking the chain".

One environment detail from this run, in case it is a clue rather than background noise — the job logged adb trouble during startup before the suite began:

ERROR | Unable to connect to adb daemon on port: 5037
The process '/usr/local/lib/android/sdk/platform-tools/adb' failed with exit code 1   (x3)
WARNING | Failed to load snapshot 'default_boot'

The suite then started and 56 of 57 passed, so this did not stop the run — but a slow or restarted adb is exactly the kind of thing that would widen a reattachment race.

Another occurrence, on a PR that touches no Kotlin at all (#69 — a shell script, a workflow step and `CLAUDE.md`). That makes the "unrelated diff" evidence stronger: this fails on branches whose contents cannot plausibly affect it. **Run [32777393771](https://github.com/JMR-dev/LibreMediaConverter/actions/runs/32777393771), E2E API 35**, failing at test 13 or so of 57: ``` org.libremediaconverter.convert.ReattachOnLaunchTest > doesNotOverwriteAPickTheUserHasAlreadyMade[test(AVD) - 15] FAILED java.lang.AssertionError: reattachment took the screen from the user: Converted(input=InputFile(uri=, displayName=holiday.mp4, sizeBytes=null, probe=null), staged=/data/user/0/org.libremediaconverter/cache/conversions/holiday_converted.mp4, engineUsed=, routeReason=, suggestedName=holiday_converted.mp4, mimeType=video/mp4) expected null, but was:<Converted(...)> ``` Note `expected null, but was:<Converted>` — the guard did not merely lose a race, it let a finished job take a screen the user had already moved on from. Worth checking whether the flake is a timing artefact of the test or the guard genuinely having a window. **Frequency is now the problem, not just the noise.** This is the third sighting today, all on unrelated branches. #52 is decomposed into eight PRs (#57–#64) and each one runs this leg, so at the current rate it will redden several of them and cost a re-run each time. That moves it from "annoying" toward "blocking the chain". One environment detail from this run, in case it is a clue rather than background noise — the job logged adb trouble during startup before the suite began: ``` ERROR | Unable to connect to adb daemon on port: 5037 The process '/usr/local/lib/android/sdk/platform-tools/adb' failed with exit code 1 (x3) WARNING | Failed to load snapshot 'default_boot' ``` The suite then started and 56 of 57 passed, so this did not stop the run — but a slow or restarted adb is exactly the kind of thing that would widen a reattachment race.
JMR-dev commented 2026-08-25 04:44:48 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Data point, with its limits stated: this has not recurred today.

I logged four sightings earlier and said the rate was turning it into a chain blocker. Since then roughly a dozen more CI runs have gone through on six PRs, and ReattachOnLaunchTest.doesNotOverwriteAPickTheUserHasAlreadyMade has appeared in none of the failures. Every failing gating leg in that window has been SafPickerRoundTripTest (#93) instead.

What this is not. A few hours of quiet is not evidence a flake is gone, and nothing in the codebase changed that would plausibly fix it — the reattachment guard is untouched since the sightings. Do not close this on the strength of it.

What it is worth. Two things:

  1. The rate is lower than "chain blocker" implied. My earlier framing came from four sightings clustered in one busy afternoon, which is exactly the sampling that makes a rare flake look common. Treat #93 as the blocker; this one is a real defect at an unknown, evidently lower rate.
  2. #93 could mask it from here on. Every failing run right now fails on SAF and gets gh run rerun --failed. A rerun that goes green tells you nothing about which test failed first, so once #93 is fixed it is worth re-checking whether this is still live rather than assuming the quiet continues.

The substance of the finding is unchanged and still worth fixing: the assertion is expected null, but was:<Converted>, so the guard let a finished job take a screen the user had already moved on from — not merely a lost race. The Unable to connect to adb daemon startup noise in the run that produced it is still the most promising lead, since a slow or restarted adb would widen exactly that window.

Data point, with its limits stated: **this has not recurred today.** I logged four sightings earlier and said the rate was turning it into a chain blocker. Since then roughly a dozen more CI runs have gone through on six PRs, and `ReattachOnLaunchTest.doesNotOverwriteAPickTheUserHasAlreadyMade` has appeared in **none** of the failures. Every failing gating leg in that window has been `SafPickerRoundTripTest` (#93) instead. **What this is not.** A few hours of quiet is not evidence a flake is gone, and nothing in the codebase changed that would plausibly fix it — the reattachment guard is untouched since the sightings. Do not close this on the strength of it. **What it is worth.** Two things: 1. **The rate is lower than "chain blocker" implied.** My earlier framing came from four sightings clustered in one busy afternoon, which is exactly the sampling that makes a rare flake look common. Treat #93 as the blocker; this one is a real defect at an unknown, evidently lower rate. 2. **#93 could mask it from here on.** Every failing run right now fails on SAF and gets `gh run rerun --failed`. A rerun that goes green tells you nothing about which test failed first, so once #93 is fixed it is worth re-checking whether this is still live rather than assuming the quiet continues. The substance of the finding is unchanged and still worth fixing: the assertion is `expected null, but was:<Converted>`, so the guard let a finished job take a screen the user had already moved on from — not merely a lost race. The `Unable to connect to adb daemon` startup noise in the run that produced it is still the most promising lead, since a slow or restarted adb would widen exactly that window.
JMR-dev commented 2026-08-25 21:04:27 +00:00 (Migrated from github.com)
Copy Link
Copy Source

It has recurred, and the reason it went quiet is now the interesting part.

ReattachOnLaunchTest.doesNotOverwriteAPickTheUserHasAlreadyMade failed the E2E API 36 gating leg on PR #110 — a diff of exactly one file, docs/api-37-emulator-crash.md. Nothing in that change can reach an instrumented test.

This is what the earlier comment predicted

I wrote, when noting it had gone quiet:

#93 could mask it from here on. Every failing run right now fails on SAF and gets
gh run rerun --failed. A rerun that goes green tells you nothing about which test failed
first, so once #93 is fixed it is worth re-checking whether this is still live rather than
assuming the quiet continues.

#93 was fixed and merged (#96) a few hours ago. The SAF failures stopped. This one reappeared. That
is the sequence the comment anticipated, and it is worth recording as a confirmed mechanism rather
than a coincidence: a dominant flake hides the ones beneath it, because the recovery for both is
the same rerun and the rerun erases which came first.

What it does not mean

Not that the rate went up. Two sightings today with a fix for a louder flake in between is not a
trend, and I have twice this session read a cluster as a rate and had to correct it. The honest
summary is: this was live before #93, it is live after #93, and the interval in between tells us
nothing.

The substance is unchanged

The assertion is still expected null, but was:<Converted> — the guard let a finished job take a
screen the user had already moved on from, which is not merely a lost race. And the earlier
observation still stands as the most promising lead: the run that produced the first sighting logged
Unable to connect to adb daemon three times during startup, and a slow or restarted adb would
widen exactly that window.

That lead is now stronger, not weaker. #102 collects three separate CI failure modes that are all
"system services stop answering under load", and #96 proved that shape once already. A reattachment
race widened by a stalled adb is the same illness in a fourth place. Worth checking whether these
sightings carry the load signature before treating this as a logic bug in the guard.

**It has recurred, and the reason it went quiet is now the interesting part.** `ReattachOnLaunchTest.doesNotOverwriteAPickTheUserHasAlreadyMade` failed the **E2E API 36** gating leg on PR #110 — a diff of exactly one file, `docs/api-37-emulator-crash.md`. Nothing in that change can reach an instrumented test. ### This is what the earlier comment predicted I wrote, when noting it had gone quiet: > **#93 could mask it from here on.** Every failing run right now fails on SAF and gets > `gh run rerun --failed`. A rerun that goes green tells you nothing about which test failed > first, so once #93 is fixed it is worth re-checking whether this is still live rather than > assuming the quiet continues. #93 was fixed and merged (#96) a few hours ago. The SAF failures stopped. This one reappeared. That is the sequence the comment anticipated, and it is worth recording as a confirmed mechanism rather than a coincidence: **a dominant flake hides the ones beneath it**, because the recovery for both is the same rerun and the rerun erases which came first. ### What it does not mean **Not that the rate went up.** Two sightings today with a fix for a louder flake in between is not a trend, and I have twice this session read a cluster as a rate and had to correct it. The honest summary is: this was live before #93, it is live after #93, and the interval in between tells us nothing. ### The substance is unchanged The assertion is still `expected null, but was:<Converted>` — the guard let a finished job take a screen the user had already moved on from, which is not merely a lost race. And the earlier observation still stands as the most promising lead: the run that produced the first sighting logged `Unable to connect to adb daemon` three times during startup, and a slow or restarted adb would widen exactly that window. That lead is now stronger, not weaker. **#102** collects three separate CI failure modes that are all "system services stop answering under load", and #96 proved that shape once already. A reattachment race widened by a stalled adb is the same illness in a fourth place. Worth checking whether these sightings carry the load signature before treating this as a logic bug in the guard.
JMR-dev commented 2026-08-26 01:49:32 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Measured rate, and a correction to how it was being measured

Census of every gating E2E leg-attempt since 2026-08-24 (advisory job excluded), classified by
reading each failing job's log and anchoring the test name to the FAILED marker on its own line.

The method is the first finding. Counting runs whose conclusion is failure sees 19 failures.
Counting leg-attempts sees 45. A leg that is re-run to green vanishes from the run-level count
entirely, so more than half of all CI failures today were invisible to the way this was being
counted. Every number below is per leg-attempt.

cause leg-attempts
SafPickerRoundTripTest (#93) — picker and/or rotation 29
<no test named> — abort or infra, nothing reported failed 6
failsOnPurposeToProveTheAdvisoryReportFires — deliberate probe for #83 4
doesNotOverwriteAPickTheUserHasAlreadyMade (this ticket) 3
transcodesH264ToH265AndReportsProgress (#102) 2
routesAFastMp4JobByDeviceCapability 1

Denominators: 45 failures / 400 leg-attempts overall; 13 / 140 since #96 landed at
2026-08-25T13:42Z.

This flake is still live, and it is not emulator-image-specific

Three occurrences, all on attempt 1, all passing on re-run:

when run leg
2026-08-24T21:03Z 32777393771 API 35
2026-08-25T20:54Z 32898081739 API 36
2026-08-26T01:23Z 32918773988 API 33

Two of the three are after #96, so the masking hypothesis holds: #93 was consuming the failure
budget, and this surfaced once that stopped. Rate since #96 is 2/140 leg-attempts (~1.4%).

Three different API levels is the new information. #108 is confined to API 37 and #93 was worst
on 34; this one has now appeared on 33, 35 and 36. That rules out an emulator-image quirk and points
at a genuine race between the reattach sweep and the pick the test has already made — which is what
the ticket's own scenario describes. ReattachOnLaunchTest.kt:219 / :228 is where it lands.

Correcting an earlier claim on this ticket

I previously recorded that this test "recurred on a docs-only PR after #96 landed". That was wrong
twice: the run I meant (32813816792, docs/readme-restart-claim) was eight hours before #96
merged, and its failure was SafPickerRoundTripTest, not this test. The conclusion happens to
survive — the two occurrences in the table above are genuinely post-#96 — but it was not what I had
looked at, and the evidence has been replaced with evidence that was.

Repro note for whoever takes this

Historical attempts cannot be read with gh run view --job <id> --log; it resolves by run and serves
the latest attempt, so a re-run silently hands you a green log for a red attempt. Use
gh api --allow-escape-sequences /repos/{owner}/{repo}/actions/jobs/{job_id}/logs with the job id
from /actions/runs/{run}/attempts/{n}/jobs. Without --allow-escape-sequences gh returns nothing
and exits 0.

## Measured rate, and a correction to how it was being measured Census of every gating E2E leg-attempt since 2026-08-24 (advisory job excluded), classified by reading each failing job's log and anchoring the test name to the `FAILED` marker on its own line. **The method is the first finding.** Counting *runs* whose conclusion is `failure` sees 19 failures. Counting *leg-attempts* sees **45**. A leg that is re-run to green vanishes from the run-level count entirely, so more than half of all CI failures today were invisible to the way this was being counted. Every number below is per leg-attempt. | cause | leg-attempts | |---|---| | `SafPickerRoundTripTest` (#93) — picker and/or rotation | **29** | | `<no test named>` — abort or infra, nothing reported failed | 6 | | `failsOnPurposeToProveTheAdvisoryReportFires` — deliberate probe for #83 | 4 | | **`doesNotOverwriteAPickTheUserHasAlreadyMade` (this ticket)** | **3** | | `transcodesH264ToH265AndReportsProgress` (#102) | 2 | | `routesAFastMp4JobByDeviceCapability` | 1 | Denominators: **45 failures / 400 leg-attempts** overall; **13 / 140** since #96 landed at 2026-08-25T13:42Z. ## This flake is still live, and it is not emulator-image-specific Three occurrences, all on attempt 1, all passing on re-run: | when | run | leg | |---|---|---| | 2026-08-24T21:03Z | 32777393771 | **API 35** | | 2026-08-25T20:54Z | 32898081739 | **API 36** | | 2026-08-26T01:23Z | 32918773988 | **API 33** | **Two of the three are after #96**, so the masking hypothesis holds: #93 was consuming the failure budget, and this surfaced once that stopped. Rate since #96 is 2/140 leg-attempts (~1.4%). **Three different API levels is the new information.** #108 is confined to API 37 and #93 was worst on 34; this one has now appeared on 33, 35 and 36. That rules out an emulator-image quirk and points at a genuine race between the reattach sweep and the pick the test has already made — which is what the ticket's own scenario describes. `ReattachOnLaunchTest.kt:219` / `:228` is where it lands. ## Correcting an earlier claim on this ticket I previously recorded that this test "recurred on a docs-only PR after #96 landed". That was wrong twice: the run I meant (32813816792, `docs/readme-restart-claim`) was **eight hours before** #96 merged, and its failure was `SafPickerRoundTripTest`, not this test. The conclusion happens to survive — the two occurrences in the table above are genuinely post-#96 — but it was not what I had looked at, and the evidence has been replaced with evidence that was. ## Repro note for whoever takes this Historical attempts cannot be read with `gh run view --job <id> --log`; it resolves by run and serves the *latest* attempt, so a re-run silently hands you a green log for a red attempt. Use `gh api --allow-escape-sequences /repos/{owner}/{repo}/actions/jobs/{job_id}/logs` with the job id from `/actions/runs/{run}/attempts/{n}/jobs`. Without `--allow-escape-sequences` gh returns nothing and exits 0.
JMR-dev commented 2026-08-26 01:56:59 +00:00 (Migrated from github.com)
Copy Link
Copy Source

This is not a flaky test. It is a product race, and the test is right.

I read the assertion message rather than the failure count. From run 32918773988 attempt 1, API 33:

java.lang.AssertionError: reattachment took the screen from the user:
  Converted(input=InputFile(uri=, displayName=holiday.mp4, sizeBytes=null, probe=null),
            staged=/data/user/0/org.libremediaconverter/cache/conversions/holiday_converted.mp4,
            suggestedName=holiday_converted.mp4, mimeType=video/mp4)
  expected null, but was:<Converted(... holiday_converted.mp4 ...)>

The state that took the screen is the old finished job, not a test artefact. The user picked
picked.mp4, the screen showed it, and then a reattachment to a previous conversion replaced it.
That is the exact thing doesNotOverwriteAPickTheUserHasAlreadyMade exists to forbid, and it is a
user-visible defect: open the app, pick a file, and watch a stale conversion from an earlier session
take the screen away from you.

The interleaving

ConversionViewModel.reattach() guards ownership at line 222:

if (_state.value !is ConversionState.Idle || activeWorkId != null) return@launch

and the comment immediately above it says:

Both this check and the assignment below run on the main dispatcher with no suspension point
between them, so nothing can interleave.

There is no assignment below. There is a call to observe(...), and observe is:

observer = viewModelScope.launch {
    workManager.getWorkInfoByIdFlow(id).collect { info ->
        ...
        _state.value = when (info.state) { ... }
    }
}

The write happens in a different coroutine, which must suspend on collect before it can emit
anything. So the guard runs at time T and the write lands at T+n, with the whole first WorkInfo
emission in between. The failing order is:

  1. init → reattach() suspends inside Reattachment.choose (a WorkManager query).
  2. User picks → onInputPicked suspends in withContext(pickDispatcher) { InputQuery.describe(...) }.
  3. choose resumes. Guard reads _state.value — still Idle, because step 2 has not written yet.
    Guard passes. activeWorkId is set, observe(...) launches, reattach's body returns.
  4. onInputPicked resumes → _state.value = Ready(picked.mp4). The user owns the screen.
  5. observe's collector receives its first WorkInfo (SUCCEEDED) → _state.value = Converted(holiday).

Step 5 overwrites step 4. This is a check-then-act with the act deferred into another coroutine, and
the comment asserting it cannot interleave is what made it hard to see.

onInputPicked is unguarded in the other direction too — it writes Ready without consulting
activeWorkId — so whichever coroutine resumes last wins.

Why it looks like a flake

The window is exactly the gap between observe() being called and its first emission arriving.
That is small on a fast machine and wide on a loaded CI runner, which is why it reads as
infrastructure noise. Measured: 3 occurrences in 400 gating leg-attempts, on API 33, 35 and
36 — three different images, which is what rules out an emulator quirk. Every occurrence was on
attempt 1 and passed on re-run, which is the signature of a race, not a broken test.

What a fix has to decide

The test encodes the answer already: the user's pick wins. The mechanism needs to make that true
rather than probable — a generation token that observe checks before every _state.value write is
the shape that fits this codebase, since activeWorkId is already the thing being tracked and is
already set synchronously at step 3.

Whatever lands, the comment at lines 219–222 must go with it. It is not merely stale; it states the
opposite of what the code does, and it is the reason this sat as "flaky CI" instead of a race.

Do not fix this by relaxing the test. It is one of the few tests in the suite that has caught a
real concurrency defect.

## This is not a flaky test. It is a product race, and the test is right. I read the assertion message rather than the failure count. From run `32918773988` attempt 1, API 33: ``` java.lang.AssertionError: reattachment took the screen from the user: Converted(input=InputFile(uri=, displayName=holiday.mp4, sizeBytes=null, probe=null), staged=/data/user/0/org.libremediaconverter/cache/conversions/holiday_converted.mp4, suggestedName=holiday_converted.mp4, mimeType=video/mp4) expected null, but was:<Converted(... holiday_converted.mp4 ...)> ``` The state that took the screen is **the old finished job**, not a test artefact. The user picked `picked.mp4`, the screen showed it, and then a reattachment to a *previous* conversion replaced it. That is the exact thing `doesNotOverwriteAPickTheUserHasAlreadyMade` exists to forbid, and it is a user-visible defect: open the app, pick a file, and watch a stale conversion from an earlier session take the screen away from you. ## The interleaving `ConversionViewModel.reattach()` guards ownership at line 222: ```kotlin if (_state.value !is ConversionState.Idle || activeWorkId != null) return@launch ``` and the comment immediately above it says: > Both this check and the assignment below run on the main dispatcher with no suspension point > between them, so nothing can interleave. **There is no assignment below.** There is a call to `observe(...)`, and `observe` is: ```kotlin observer = viewModelScope.launch { workManager.getWorkInfoByIdFlow(id).collect { info -> ... _state.value = when (info.state) { ... } } } ``` The write happens in a **different coroutine**, which must suspend on `collect` before it can emit anything. So the guard runs at time T and the write lands at T+n, with the whole first WorkInfo emission in between. The failing order is: 1. `init` → `reattach()` suspends inside `Reattachment.choose` (a WorkManager query). 2. User picks → `onInputPicked` suspends in `withContext(pickDispatcher) { InputQuery.describe(...) }`. 3. `choose` resumes. Guard reads `_state.value` — still `Idle`, because step 2 has not written yet. Guard **passes**. `activeWorkId` is set, `observe(...)` launches, `reattach`'s body returns. 4. `onInputPicked` resumes → `_state.value = Ready(picked.mp4)`. The user owns the screen. 5. `observe`'s collector receives its first `WorkInfo` (SUCCEEDED) → `_state.value = Converted(holiday)`. Step 5 overwrites step 4. This is a check-then-act with the act deferred into another coroutine, and the comment asserting it cannot interleave is what made it hard to see. `onInputPicked` is unguarded in the other direction too — it writes `Ready` without consulting `activeWorkId` — so whichever coroutine resumes last wins. ## Why it looks like a flake The window is exactly the gap between `observe()` being called and its first emission arriving. That is small on a fast machine and wide on a loaded CI runner, which is why it reads as infrastructure noise. Measured: **3 occurrences in 400 gating leg-attempts**, on API **33, 35 and 36** — three different images, which is what rules out an emulator quirk. Every occurrence was on attempt 1 and passed on re-run, which is the signature of a race, not a broken test. ## What a fix has to decide The test encodes the answer already: **the user's pick wins.** The mechanism needs to make that true rather than probable — a generation token that `observe` checks before every `_state.value` write is the shape that fits this codebase, since `activeWorkId` is already the thing being tracked and is already set synchronously at step 3. Whatever lands, the comment at lines 219–222 must go with it. It is not merely stale; it states the opposite of what the code does, and it is the reason this sat as "flaky CI" instead of a race. **Do not fix this by relaxing the test.** It is one of the few tests in the suite that has caught a real concurrency defect.
JMR-dev commented 2026-08-26 01:57:41 +00:00 (Migrated from github.com)
Copy Link
Copy Source

JoinViewModel has the same race, and nothing is watching it

Identical structure, same defect:

ConversionViewModel JoinViewModel
ownership guard :222 :105
observe(...) called :247 :124
write deferred into viewModelScope.launch { …collect… } :346 :164

Same check-then-act, same deferred write, same outcome: a reattachment can land on top of files the
user has just picked.

The difference is that nothing catches it. ReattachOnLaunchTest covers
reattachesToAJoinThatFinishedWhileTheViewModelWasGone — that reattachment works — but there is
no Join counterpart to doesNotOverwriteAPickTheUserHasAlreadyMade. The converter side has been
telling us about this defect for two days at roughly 1-in-130; the join side has been silent because
no one asked it.

So the fix is two changes and two tests, not one of each. The missing Join test should be written
before the fix and confirmed to fail — otherwise it is a test written against code that already
passes, which is the vacuous-green failure mode this repo has measured before (9 of 46 mutations,
five over a completely unguarded path).

## `JoinViewModel` has the same race, and nothing is watching it Identical structure, same defect: | | `ConversionViewModel` | `JoinViewModel` | |---|---|---| | ownership guard | :222 | **:105** | | `observe(...)` called | :247 | **:124** | | write deferred into `viewModelScope.launch { …collect… }` | :346 | **:164** | Same check-then-act, same deferred write, same outcome: a reattachment can land on top of files the user has just picked. **The difference is that nothing catches it.** `ReattachOnLaunchTest` covers `reattachesToAJoinThatFinishedWhileTheViewModelWasGone` — that reattachment *works* — but there is no Join counterpart to `doesNotOverwriteAPickTheUserHasAlreadyMade`. The converter side has been telling us about this defect for two days at roughly 1-in-130; the join side has been silent because no one asked it. So the fix is two changes and two tests, not one of each. The missing Join test should be written **before** the fix and confirmed to fail — otherwise it is a test written against code that already passes, which is the vacuous-green failure mode this repo has measured before (9 of 46 mutations, five over a completely unguarded path).
JMR-dev commented 2026-08-26 02:24:10 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Fourth occurrence, fourth API level — and it is now costing re-runs on unrelated PRs

2026-08-26T02:22Z, run 32922100069 attempt 1, E2E API 35, on #116 (a PR about the Failed-save
state, which touches none of this):

  expected: 59   received: 59   failed: 1   completed cleanly: yes
  failed tests:
    org.libremediaconverter.convert.ReattachOnLaunchTest.doesNotOverwriteAPickTheUserHasAlreadyMade

Running tally, all on attempt 1 and all green on re-run:

when run leg
2026-08-24T21:03Z 32777393771 API 35
2026-08-25T20:54Z 32898081739 API 36
2026-08-26T01:23Z 32918773988 API 33
2026-08-26T02:22Z 32922100069 API 35

Three of the four are since #96, and the spread is now API 33, 35 and 36. That is not an image
quirk, and the diagnosis above says why it is not: the ownership guard in reattach() runs before
observe() defers the write into a second coroutine, so whichever of the two resumes last wins.

The practical cost has changed too. This is no longer a curiosity on its own PR — it is now failing
gating legs on unrelated changes and being re-run by hand, which is exactly how a real defect gets
naturalised as "CI being flaky".

Worth recording: the report added by #111 named the failing test in one line here, with no log
archaeology. That is the second time it has paid for itself since merging.

## Fourth occurrence, fourth API level — and it is now costing re-runs on unrelated PRs `2026-08-26T02:22Z`, run `32922100069` attempt 1, **E2E API 35**, on #116 (a PR about the Failed-save state, which touches none of this): ``` expected: 59 received: 59 failed: 1 completed cleanly: yes failed tests: org.libremediaconverter.convert.ReattachOnLaunchTest.doesNotOverwriteAPickTheUserHasAlreadyMade ``` Running tally, all on attempt 1 and all green on re-run: | when | run | leg | |---|---|---| | 2026-08-24T21:03Z | 32777393771 | API 35 | | 2026-08-25T20:54Z | 32898081739 | API 36 | | 2026-08-26T01:23Z | 32918773988 | API 33 | | **2026-08-26T02:22Z** | **32922100069** | **API 35** | **Three of the four are since #96**, and the spread is now API 33, 35 and 36. That is not an image quirk, and the diagnosis above says why it is not: the ownership guard in `reattach()` runs before `observe()` defers the write into a second coroutine, so whichever of the two resumes last wins. The practical cost has changed too. This is no longer a curiosity on its own PR — it is now failing gating legs on unrelated changes and being re-run by hand, which is exactly how a real defect gets naturalised as "CI being flaky". Worth recording: the report added by #111 named the failing test in one line here, with no log archaeology. That is the second time it has paid for itself since merging.
JMR-dev commented 2026-08-26 04:34:54 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Post-fix check: zero recurrences, and that does not yet mean anything

Census of every gating E2E leg-attempt since #124 merged (2026-08-26T03:58:57Z):

  • 25 leg-attempts, 20 completed — 19 success, 1 failure.
  • The one failure is #108: hasReadColorBufferDma abort on API 37 taking down
    SafPickerRoundTripTest.pickingAFileThroughTheSystemPickerFillsInTheFileCard.
  • doesNotOverwriteAPickTheUserHasAlreadyMade: 0 occurrences.

That is not evidence the fix works, and I want it on record before someone later reads it as such.
The pre-fix rate was 2 in 140 post-#96 leg-attempts, ≈1.4%. At that rate:

completed leg-attempts P(zero, if the fix did nothing)
20 (where we are) 0.75
50 0.49
100 0.24
200 0.06
300 0.015

Seeing zero in 20 is what you would expect three times in four with no fix at all. It is
consistent with the fix working and equally consistent with it doing nothing; it discriminates
between them not at all.

CI becomes worth quoting somewhere around 200 leg-attempts — roughly 40 more full runs — and even
then it is a probabilistic argument, not a proof.

The evidence that actually closed this ticket is the deterministic JVM reproduction: the Join test
failed before any production change, quoted verbatim in #124, with the pick parked on a dispatcher the
test owns so the interleaving is forced rather than raced. Twelve of sixteen mutations bite. That is
what makes the fix trustworthy, and it did not need CI to say so.

Recording the arithmetic so the next person to look has a threshold instead of an impression.

## Post-fix check: zero recurrences, and that does not yet mean anything Census of every gating E2E leg-attempt since #124 merged (`2026-08-26T03:58:57Z`): - **25 leg-attempts**, 20 completed — 19 success, 1 failure. - The one failure is **#108**: `hasReadColorBufferDma` abort on API 37 taking down `SafPickerRoundTripTest.pickingAFileThroughTheSystemPickerFillsInTheFileCard`. - **`doesNotOverwriteAPickTheUserHasAlreadyMade`: 0 occurrences.** **That is not evidence the fix works**, and I want it on record before someone later reads it as such. The pre-fix rate was 2 in 140 post-#96 leg-attempts, ≈1.4%. At that rate: | completed leg-attempts | P(zero, *if the fix did nothing*) | |---|---| | **20 (where we are)** | **0.75** | | 50 | 0.49 | | 100 | 0.24 | | 200 | 0.06 | | 300 | 0.015 | Seeing zero in 20 is what you would expect three times in four **with no fix at all**. It is consistent with the fix working and equally consistent with it doing nothing; it discriminates between them not at all. CI becomes worth quoting somewhere around **200 leg-attempts** — roughly 40 more full runs — and even then it is a probabilistic argument, not a proof. **The evidence that actually closed this ticket is the deterministic JVM reproduction**: the Join test failed before any production change, quoted verbatim in #124, with the pick parked on a dispatcher the test owns so the interleaving is forced rather than raced. Twelve of sixteen mutations bite. That is what makes the fix trustworthy, and it did not need CI to say so. Recording the arithmetic so the next person to look has a threshold instead of an impression.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
above-cut
Worked autonomously overnight: local, JVM-verifiable, no product decision
 accessibility
Barrier affecting people with disabilities
 backlog
Held for manual review: product/UX call, CI/workflow, hardware, or unverifiable here
 bug
Something isn't working
 confirmed
Reviewer demonstrated the defect
 documentation
Improvements or additions to documentation
 duplicate
This issue or pull request already exists
 enhancement
New feature or request
 good first issue
Good for newcomers
 help wanted
Extra attention is needed
 invalid
This doesn't seem right
 plausible
Reviewer could not fully demonstrate it; treat as unproven
 question
Further information is requested
 sev:high
High severity
 sev:low
Low severity
 sev:medium
Medium severity
 wontfix
This will not be worked on
No labels backlog bug confirmed sev:medium
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
JMR-dev
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: JMR-dev/LibreMediaConverter#49