Application-scope IO work races every Robolectric test that shares cacheDir #159

New Issue

Closed

opened 2026-08-27 12:10:45 +00:00 by JMR-dev · 3 comments

JMR-dev commented 2026-08-27 12:10:45 +00:00 (Migrated from github.com)

Copy Link

Copy Source

LibreMediaConverterApp.onCreate ends with:

private val appScope = CoroutineScope(SupervisorJob() + Dispatchers.IO)
...
appScope.launch { OutputPublisher(this@LibreMediaConverterApp).sweepStaging() }

sweepStaging() reads stagingDir, whose getter is File(context.cacheDir, "conversions").apply { mkdirs() }.

Robolectric instantiates the application for every test that asks for one, so that background mkdirs() is in flight across the whole JVM suite — on a Dispatchers.IO thread, which the paused main looper does not control and no test awaits. Nothing joins it, so it lands whenever the machine gets to it.

How it surfaced

PR #149's OutputPublisherStagingTest > the sweep tolerates a staging path that is not a directory failed once on run 33069641674:

java.io.FileNotFoundException at OutputPublisherStagingTest.kt:112
468 tests completed, 1 failed

Line 112 was File(cacheDir, "conversions").writeBytes(ByteArray(8)), immediately after a deleteRecursively(). FileOutputStream answers FileNotFoundException for an existing directory, so a background mkdirs() had recreated the path inside that window.

The same 468 tests pass on this machine, including under --rerun-tasks. It is a timing race, and a loaded CI runner is where it shows.

Why it is worth a ticket rather than per-test defensiveness

#149 worked around it in its own fixture, because that test is the only one that asserts on the path's type and so the only one that can fail this way today. The mechanism is not confined to it:

• AppStartSweepTest, JobSnapshotsTest and SpaceArithmeticTest all name File(cacheDir, "conversions")
• any future test that counts files in staging, or asserts staging is absent, is exposed the same way

Every one of those would fail rarely, on CI, with an error that does not mention coroutines. That is the expensive kind of flake — the kind that gets re-run rather than read.

Shape of a fix

The problem is not the sweep; it is that a test cannot await it. Two directions, both worth weighing before either is taken:

1. Inject the scope or dispatcher. appScope is a private val built inline. Making it overridable — the way ConversionDependencies already does for the probe, codecs, software engine and publisher — lets a test supply an immediate or a controlled dispatcher and removes the race by construction. This matches the existing pattern in the codebase.
2. Do not launch it from onCreate at all. The sweep exists to collect a day-old file; nothing needs it to run before the first frame. Moving it behind an explicit call the app makes, and the test does not, is a smaller change but pushes the question onto whoever owns start-up.

Prefer whichever leaves LibreMediaConverterApp honest about when the sweep runs. What should not happen is a Thread.sleep in a test, or every future staging test carrying a retry loop.

Done when

A test that deletes cacheDir/conversions can rely on it staying deleted for the duration of the test body, without retrying — and #149's stagingPathAsRegularFile() retry loop can be deleted, which is the concrete check that this is fixed.

`LibreMediaConverterApp.onCreate` ends with: ```kotlin private val appScope = CoroutineScope(SupervisorJob() + Dispatchers.IO) ... appScope.launch { OutputPublisher(this@LibreMediaConverterApp).sweepStaging() } ``` `sweepStaging()` reads `stagingDir`, whose getter is `File(context.cacheDir, "conversions").apply { mkdirs() }`. **Robolectric instantiates the application for every test that asks for one**, so that background `mkdirs()` is in flight across the whole JVM suite — on a `Dispatchers.IO` thread, which the paused main looper does not control and no test awaits. Nothing joins it, so it lands whenever the machine gets to it. ## How it surfaced PR #149's `OutputPublisherStagingTest > the sweep tolerates a staging path that is not a directory` failed **once** on run `33069641674`: ``` java.io.FileNotFoundException at OutputPublisherStagingTest.kt:112 468 tests completed, 1 failed ``` Line 112 was `File(cacheDir, "conversions").writeBytes(ByteArray(8))`, immediately after a `deleteRecursively()`. `FileOutputStream` answers `FileNotFoundException` for an existing directory, so a background `mkdirs()` had recreated the path inside that window. The same 468 tests pass on this machine, including under `--rerun-tasks`. It is a timing race, and a loaded CI runner is where it shows. ## Why it is worth a ticket rather than per-test defensiveness #149 worked around it in its own fixture, because that test is the only one that asserts on the path's *type* and so the only one that can fail this way today. The mechanism is not confined to it: - `AppStartSweepTest`, `JobSnapshotsTest` and `SpaceArithmeticTest` all name `File(cacheDir, "conversions")` - any future test that counts files in staging, or asserts staging is absent, is exposed the same way Every one of those would fail rarely, on CI, with an error that does not mention coroutines. That is the expensive kind of flake — the kind that gets re-run rather than read. ## Shape of a fix The problem is not the sweep; it is that a test cannot await it. Two directions, both worth weighing before either is taken: 1. **Inject the scope or dispatcher.** `appScope` is a `private val` built inline. Making it overridable — the way `ConversionDependencies` already does for the probe, codecs, software engine and publisher — lets a test supply an immediate or a controlled dispatcher and removes the race by construction. This matches the existing pattern in the codebase. 2. **Do not launch it from `onCreate` at all.** The sweep exists to collect a day-old file; nothing needs it to run before the first frame. Moving it behind an explicit call the app makes, and the test does not, is a smaller change but pushes the question onto whoever owns start-up. Prefer whichever leaves `LibreMediaConverterApp` honest about when the sweep runs. What should not happen is a `Thread.sleep` in a test, or every future staging test carrying a retry loop. ## Done when A test that deletes `cacheDir/conversions` can rely on it staying deleted for the duration of the test body, without retrying — and #149's `stagingPathAsRegularFile()` retry loop can be deleted, which is the concrete check that this is fixed.

JMR-dev commented 2026-09-02 04:59:32 +00:00 (Migrated from github.com)

Copy Link

Copy Source

First observed occurrence, from CI rather than inspection — and a note on what the wave-3 test push (#167-#178) does to the exposure.

Where: PR #191, Unit tests job, 2026-09-02.

OutputPublisherStagingTest > a file that stops being collectable between the listing and the delete survives FAILED
    java.lang.AssertionError at OutputPublisherStagingTest.kt:184
584 tests completed, 1 failed

Line 184 is assertTrue("a file a live job started writing after the listing must not be unlinked", orphan.exists()). The test creates orphan in the shared staging directory with a deliberately old mtime, then touches it to now from inside an overridden snapshot() so the re-read guard has to refuse the delete. The file was gone.

Not caused by the change under test. #191 is docs-only, and OutputPublisher.kt is untouched across the whole wave — git diff d354f64..origin/main -- .../OutputPublisher.kt is empty. The ten PRs ahead of it all had the Unit tests job pass, so this is the first occurrence, not a new steady state.

Not reproducible locally: three consecutive --rerun-tasks full-suite runs on the same tree, all green.

Why it is worth logging here rather than shrugging at

This ticket's thesis is that application-scope IO races every Robolectric test sharing cacheDir. orphan vanishing between snapshot() and the assertion is that thesis with a name and a line number.

And wave 3 widened the window. It added eight Robolectric classes, of which three write to the shared staging directory:

new class	instantiates Application	touches staging
NotificationProgressTextTest	yes	yes
AdaptiveShellTest	yes	yes
ConcatFailureTest	yes	yes
UnreadableJoinInputTest, Media3MuxerGuardTest	yes	no
ForegroundTypeRegimeTest, HardwareFallbackTest, MediaProbeMergeTest	no	no

ConcatFailureTest is the most pointed: its success-path test deliberately leaves a staged file behind, because that file is the join's output and deleting it would be the bug. So the suite now ends with more residue in the shared directory than it used to.

I am not proposing a fix in this comment, and specifically not proposing that the three new tests use their own directories — that would treat the symptom and leave the ticket's actual subject untouched, which is that the directory is shared at all. But the exposure is measurably larger than when this was filed, so the cost of leaving it open has gone up.

Related: #125 (the suite can deadlock in Room/WorkManager) is the other open ticket about this suite's shared-state behaviour.

First observed occurrence, from CI rather than inspection — and a note on what the wave-3 test push (#167-#178) does to the exposure. **Where:** PR #191, `Unit tests` job, 2026-09-02. ``` OutputPublisherStagingTest > a file that stops being collectable between the listing and the delete survives FAILED java.lang.AssertionError at OutputPublisherStagingTest.kt:184 584 tests completed, 1 failed ``` Line 184 is `assertTrue("a file a live job started writing after the listing must not be unlinked", orphan.exists())`. The test creates `orphan` in the shared staging directory with a deliberately old mtime, then touches it to *now* from inside an overridden `snapshot()` so the re-read guard has to refuse the delete. **The file was gone.** **Not caused by the change under test.** #191 is docs-only, and `OutputPublisher.kt` is untouched across the whole wave — `git diff d354f64..origin/main -- .../OutputPublisher.kt` is empty. The ten PRs ahead of it all had the `Unit tests` job pass, so this is the first occurrence, not a new steady state. **Not reproducible locally**: three consecutive `--rerun-tasks` full-suite runs on the same tree, all green. ### Why it is worth logging here rather than shrugging at This ticket's thesis is that application-scope IO races every Robolectric test sharing `cacheDir`. `orphan` vanishing between `snapshot()` and the assertion is that thesis with a name and a line number. And wave 3 widened the window. It added eight Robolectric classes, of which **three write to the shared staging directory**: | new class | instantiates Application | touches staging | |---|---|---| | `NotificationProgressTextTest` | yes | yes | | `AdaptiveShellTest` | yes | yes | | `ConcatFailureTest` | yes | yes | | `UnreadableJoinInputTest`, `Media3MuxerGuardTest` | yes | no | | `ForegroundTypeRegimeTest`, `HardwareFallbackTest`, `MediaProbeMergeTest` | no | no | `ConcatFailureTest` is the most pointed: its success-path test deliberately **leaves** a staged file behind, because that file is the join's output and deleting it would be the bug. So the suite now ends with more residue in the shared directory than it used to. I am not proposing a fix in this comment, and specifically not proposing that the three new tests use their own directories — that would treat the symptom and leave the ticket's actual subject untouched, which is that the directory is shared at all. But the exposure is measurably larger than when this was filed, so the cost of leaving it open has gone up. Related: #125 (the suite can deadlock in Room/WorkManager) is the other open ticket about this suite's shared-state behaviour.

JMR-dev commented 2026-09-02 23:18:51 +00:00 (Migrated from github.com)

Copy Link

Copy Source

Second observation, on a different test in the same class — 2026-09-02, PR #209 (a one-line FileCardTest addition that touches nothing in OutputPublisher).

OutputPublisherStagingTest > a file that stops being collectable between the listing and the delete survives FAILED
    java.lang.AssertionError at OutputPublisherStagingTest.kt:184
585 tests completed, 1 failed

Run 33693639356.

This is worth adding because it widens the mechanism beyond what the ticket predicted. The original observation was a FileNotFoundException from a background mkdirs() recreating conversions/ inside a deleteRecursively() window — a race on the directory's existence. This one is a race on a file inside it: the assertion at :184 is orphan.exists(), and the orphan had been unlinked.

The shape fits a second sweep. The test's own fixture makes the file old enough to collect, then touches it to now from inside an overridden snapshot() so the re-read guard has to refuse the delete. An application-scope sweepStaging() running concurrently took its own listing before that touch, so from its point of view the file was collectable and it deleted it — which is exactly the ticket's mechanism, one layer further in.

So the affected set is larger than the three tests listed in the issue body: it is not only tests that assert on the path's type, but any test asserting that a file under conversions/ still exists. StagingCleanupSupport-based tests and ReattachedCleanupTest are in that group.

Evidence it is the race rather than the change under test: OutputPublisherStagingTest passes locally on three consecutive --rerun-tasks runs, and the same test on the same tree passed in #206, #208 and #210's legs.

**Second observation, on a different test in the same class** — 2026-09-02, PR #209 (a one-line `FileCardTest` addition that touches nothing in `OutputPublisher`). ``` OutputPublisherStagingTest > a file that stops being collectable between the listing and the delete survives FAILED java.lang.AssertionError at OutputPublisherStagingTest.kt:184 585 tests completed, 1 failed ``` Run [33693639356](https://github.com/JMR-dev/LibreMediaConverter/actions/runs/33693639356/job/100457706346). This is worth adding because it **widens the mechanism beyond what the ticket predicted**. The original observation was a `FileNotFoundException` from a background `mkdirs()` recreating `conversions/` inside a `deleteRecursively()` window — a race on the directory's *existence*. This one is a race on a *file inside it*: the assertion at `:184` is `orphan.exists()`, and the orphan had been unlinked. The shape fits a second sweep. The test's own fixture makes the file old enough to collect, then touches it to `now` from inside an overridden `snapshot()` so the re-read guard has to refuse the delete. An application-scope `sweepStaging()` running concurrently took its own listing *before* that touch, so from its point of view the file was collectable and it deleted it — which is exactly the ticket's mechanism, one layer further in. So the affected set is larger than the three tests listed in the issue body: it is not only tests that assert on the *path's type*, but any test asserting that a file under `conversions/` still exists. `StagingCleanupSupport`-based tests and `ReattachedCleanupTest` are in that group. Evidence it is the race rather than the change under test: `OutputPublisherStagingTest` passes locally on three consecutive `--rerun-tasks` runs, and the same test on the same tree passed in #206, #208 and #210's legs.

JMR-dev commented 2026-09-03 00:03:00 +00:00 (Migrated from github.com)

Copy Link

Copy Source

It reproduces on the development host now, which it did not earlier today. That is a change in this ticket's own terms — the body records it as a CI-load symptom ("The same 468 tests pass on this machine, including under --rerun-tasks. It is a timing race, and a loaded CI runner is where it shows").

Measured 2026-09-02 while writing #201, on a tree with eight wave-4 test classes added:

condition	runs	result
full suite, new test class present	3	all green
full suite, same tree, new test class removed	3	1 failure

OutputPublisherStagingTest > a file that stops being collectable between the listing and the delete survives FAILED
    java.lang.AssertionError at OutputPublisherStagingTest.kt:184

The experiment was run to answer "did my change cause this", and the answer is no — it failed on the arm without the new file. What it incidentally establishes is that the rate is now high enough to hit locally, somewhere around one run in six, where three consecutive clean --rerun-tasks runs earlier in the same session found nothing.

Why it is getting worse is mechanical rather than mysterious. Robolectric instantiates the Application for every test that asks for one, and each instantiation schedules another appScope.launch { OutputPublisher(...).sweepStaging() } on Dispatchers.IO. So the number of concurrent sweepers grows with the number of Robolectric test classes, and a test wave increases it by construction. Wave 4 has added eight so far, with three tickets left.

The consequence worth flagging: this is no longer only a retry cost on CI (#190). It now intermittently fails the local gate that CLAUDE.md requires before a change counts as done, which makes "run the gate, believe the result" unreliable for everyone.

I have not attempted a fix — the three plausible shapes (suppress the sweep under test, make it joinable, or give the test an unshared directory) trade off differently against what the sweep is for, and that is your call rather than mine.

**It reproduces on the development host now, which it did not earlier today.** That is a change in this ticket's own terms — the body records it as a CI-load symptom ("The same 468 tests pass on this machine, including under `--rerun-tasks`. It is a timing race, and a loaded CI runner is where it shows"). Measured 2026-09-02 while writing #201, on a tree with eight wave-4 test classes added: | condition | runs | result | |---|---|---| | full suite, new test class present | 3 | all green | | full suite, same tree, new test class removed | 3 | **1 failure** | ``` OutputPublisherStagingTest > a file that stops being collectable between the listing and the delete survives FAILED java.lang.AssertionError at OutputPublisherStagingTest.kt:184 ``` The experiment was run to answer "did my change cause this", and the answer is no — it failed on the arm *without* the new file. What it incidentally establishes is that the rate is now high enough to hit locally, somewhere around one run in six, where three consecutive clean `--rerun-tasks` runs earlier in the same session found nothing. **Why it is getting worse is mechanical rather than mysterious.** Robolectric instantiates the `Application` for every test that asks for one, and each instantiation schedules another `appScope.launch { OutputPublisher(...).sweepStaging() }` on `Dispatchers.IO`. So the number of concurrent sweepers grows with the number of Robolectric test classes, and a test wave increases it by construction. Wave 4 has added eight so far, with three tickets left. The consequence worth flagging: this is no longer only a retry cost on CI (#190). It now intermittently fails the local gate that `CLAUDE.md` requires before a change counts as done, which makes "run the gate, believe the result" unreliable for everyone. I have not attempted a fix — the three plausible shapes (suppress the sweep under test, make it joinable, or give the test an unshared directory) trade off differently against what the sweep is *for*, and that is your call rather than mine.

Sign in to join this conversation.

No Branch/Tag Specified

Branches Tags

main

fix/102-picker-back-press-overshoot

fix/268-saf-picker-determinism

feat/ogg-vorbis-libvorbis

feat/expedited-conversion-work

fix/gate-cache-in-worktrees

chore/gate-runs-shellcheck

test/publish-delete-arm-real-provider

docs/e8-instrumented-coverage

docs/api37-carrier-count-drift

docs/e7-second-constraint

test/publish-to-a-real-saf-destination

fix/api37-report-match-line

fix/api37-task-snapshot-crash

test/join-failure-message-on-device

docs/e2e-read-findings-e7

test/cancelling-a-running-export

test/reattach-to-a-running-job

test/content-uri-reaches-ffmpeg

fix/launcher-wiring-waits-for-the-pick

fix/cancel-tests-need-a-slower-encode

test/cancelling-a-running-join

test/cancelling-a-running-session

test/notification-cancel-action

test/ffmpeg-progress-is-observed

test/fallback-asserts-the-path

test/flac-and-opus-assert-their-format

docs/e2e-read-findings

docs/wave4-coverage-numbers

fix/injectable-startup-sweep-scope

test/session-outcome-seam

test/launcher-callback-identity

test/theme-follows-system-dark

test/audio-drop-arm

fix/rotation-waits-for-recreation

fix/convert-guards-on-ready

test/retry-save-mime

test/hardware-progress-reaches-workmanager

test/ffprobe-mapping-seam

test/device-codec-enumeration-seam

test/unknown-container-row

test/null-message-fallbacks

test/cancel-reaches-workmanager

docs/coverage-wave3-recovery

test/concat-engine-seam

docs/coverage-wave3

test/mediaprobe-merge-seam

test/adaptive-shell-wiring

test/aac-audio-args

test/notification-progress-text

test/media3-muxer-guard

test/hardware-fallback-and-cancellation

test/unprobeable-join-clip

test/one-branch-outcomes

test/foreground-type-regimes

fix/bound-wedge-diagnostics

docs/coverage-wave2

test/screen-wiring

test/viewmodel-setters

test/join-state-mapping

test/conversion-state-mapping

test/dedupe-user-messages

fix/restore-stack-merges

test/refused-jobs

test/concatworker-failure-arms

test/container-capabilities-audio

test/readspec-enum-fallbacks

test/outputpublisher-seams

test/mediaprobe-track-seam

test/outputpublisher-partial-branches

test/fake-provider-scaffolding

docs/coverage-read-findings

chore/gitignore-kotlin

test/bound-the-hangs

docs/coverage-remeasure

ci/baseline-counter-precision

fix/invalid-suggestion-chip

ci/wedged-leg-report

fix/reattachment-overwrites-pick

test/theme-live-branches

fix/failed-save-retry

fix/empty-composition-crash

ci/advisory-failure-report

docs/seven-run-counts

test/release-permission-guard

ci/build-workflow-permissions

docs/api37-point-release

docs/benchmark-populate-path

fix/dead-assertion-probe-test

ci/actionlint

test/device-codecs-encode-consequence

fix/sdkmanager-pipefail

docs/readme-restart-claim

fix/saf-picker-root-discovery

fix/probe-dispatcher-seam

test/media3engine-mime-tables

test/mediaprobe-pure-helpers

fix/codec-vocabulary-drift

docs/robolectric-rationale-correction

docs/api37-advisory-counts

test/r38-8-saf-e2e

test/r38-7-join-states

test/r38-6-conversion-states

test/r38-5-state-seam

fix/jacoco-robolectric-coverage

docs/instrumented-tests-correction

test/r38-2-filecard

test/r38-4-advanced-picker

test/r38-3-pickers

tools/file-issue-script

tools/api-37-emulator

fix/review-app-gaps

docs/review-corrections

No results found.

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

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

JMR-dev

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: JMR-dev/LibreMediaConverter#159