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

Cancelling a running native session is not executed by any test, in any of the three engines #224

New Issue
Closed
opened 2026-09-06 02:52:59 +00:00 by JMR-dev · 1 comment
JMR-dev commented 2026-09-06 02:52:59 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Filed from the 2026-09-05 e2e read of the instrumented suite on main @ 4b02294.

Cancelling a conversion or join while it is running is not executed by any test on any source set. Three engines carry the code and none of it runs.

The lines

app/src/main/java/org/libremediaconverter/ffmpeg/FFmpegEngine.kt:61,74-77   cont.cancel() -> FFmpegKit.cancel(sessionId); output.delete()
app/src/main/java/org/libremediaconverter/ffmpeg/ConcatEngine.kt:76-80      the same, for a join
app/src/main/java/org/libremediaconverter/convert/Media3Engine.kt:120-123   handler.post { transformer.cancel() }

Also the if (!cont.isActive) return early exit in Media3Engine.pollProgress (:178).

Why nothing covers it

grep for cancel across app/src/androidTest/java returns only WorkManager.cancelWorkById — and every call site cancels work that is queued or already finished, never running:

  • ReattachOnLaunchTest.doesNotResurrectAConversionTheUserCancelled cancels a job enqueued with a one-hour initial delay, so it never starts.
  • ReattachOnLaunchTest.aConversionRequestIsFindableByItsWorkerClassName cancels immediately after enqueue.

On the JVM, WorkerCancellationTest and HardwareFallbackTest's cancellation case both drive fakes — a SoftwareTranscoder that records the call. Nothing has ever asked a real native session to stop.

This is docs/defect-audit.md D10's forcing condition, recorded as never run.

Why it is worth a device test

It is the one path where cancelling wrong is silently expensive rather than loudly broken. FFmpegKit.cancel(sessionId) takes a session id, and getting it wrong — cancelling session 0, or a stale id — leaves the native process encoding to completion while the UI says the job is cancelled. The battery and thermal cost is real and nothing would report it.

The staged partial is deleted on the same path (output.delete()), so a missed cancel also leaks a full-size file into cacheDir that only the 24-hour sweep will collect.

Shape

Headless — no Activity, no Compose. Enqueue through the real worker, wait for the first progress callback, cancel, then assert both:

  1. WorkInfo.State.CANCELLED, and
  2. the staged output is gone.

Flake note, and it decides the fixture. The committed clips are 3 s and a veryfast FFmpeg encode of 320x240 finishes in a few hundred milliseconds — HardwareFallbackTest completed a full transcode in 448 ms on the API 34 leg. Cancelling from a timer would race. Two ways out, both already available:

  • cancel from inside the first progress callback, which is deterministic, or
  • use QualityTier.BEST, which routes to FFmpeg with -preset medium and runs on every leg already.

Mutation: drop the FFmpegKit.cancel(sessionId) call and keep the output.delete(). The coroutine still completes as cancelled, so a test that only checks CANCELLED stays green — which is why the staged-file assertion is the one that bites. Verify both halves separately.

Not proposed

Cancelling the Media3 export in the same test. transformer.cancel() has to run on the engine's HandlerThread and its failure mode is different (a hung continuation rather than a runaway process); it deserves its own case, and on emulators the Media3 path is only reachable by pinning DeviceCodecs.PERMISSIVE the way ForcedFailureTest does — which puts it behind the same problem as the fallback ticket.

Splitting is cheap; conflating them would make one test that fails for two unrelated reasons.

_Filed from the 2026-09-05 e2e read of the instrumented suite on `main` @ `4b02294`._ Cancelling a conversion or join **while it is running** is not executed by any test on any source set. Three engines carry the code and none of it runs. ## The lines ``` app/src/main/java/org/libremediaconverter/ffmpeg/FFmpegEngine.kt:61,74-77 cont.cancel() -> FFmpegKit.cancel(sessionId); output.delete() app/src/main/java/org/libremediaconverter/ffmpeg/ConcatEngine.kt:76-80 the same, for a join app/src/main/java/org/libremediaconverter/convert/Media3Engine.kt:120-123 handler.post { transformer.cancel() } ``` Also the `if (!cont.isActive) return` early exit in `Media3Engine.pollProgress` (`:178`). ## Why nothing covers it `grep` for `cancel` across `app/src/androidTest/java` returns only `WorkManager.cancelWorkById` — and every call site cancels work that is **queued or already finished**, never running: - `ReattachOnLaunchTest.doesNotResurrectAConversionTheUserCancelled` cancels a job enqueued with a **one-hour initial delay**, so it never starts. - `ReattachOnLaunchTest.aConversionRequestIsFindableByItsWorkerClassName` cancels immediately after enqueue. On the JVM, `WorkerCancellationTest` and `HardwareFallbackTest`'s cancellation case both drive fakes — a `SoftwareTranscoder` that records the call. Nothing has ever asked a **real native session** to stop. This is `docs/defect-audit.md` **D10**'s forcing condition, recorded as never run. ## Why it is worth a device test It is the one path where cancelling wrong is silently expensive rather than loudly broken. `FFmpegKit.cancel(sessionId)` takes a session id, and getting it wrong — cancelling session 0, or a stale id — leaves the native process encoding to completion while the UI says the job is cancelled. The battery and thermal cost is real and nothing would report it. The staged partial is deleted on the same path (`output.delete()`), so a missed cancel also leaks a full-size file into `cacheDir` that only the 24-hour sweep will collect. ## Shape Headless — no Activity, no Compose. Enqueue through the real worker, wait for the first progress callback, cancel, then assert both: 1. `WorkInfo.State.CANCELLED`, and 2. the staged output is gone. **Flake note, and it decides the fixture.** The committed clips are 3 s and a `veryfast` FFmpeg encode of `320x240` finishes in a few hundred milliseconds — `HardwareFallbackTest` completed a full transcode in 448 ms on the API 34 leg. Cancelling from a timer would race. Two ways out, both already available: - cancel from **inside the first progress callback**, which is deterministic, or - use `QualityTier.BEST`, which routes to FFmpeg with `-preset medium` and runs on every leg already. *Mutation:* drop the `FFmpegKit.cancel(sessionId)` call and keep the `output.delete()`. The coroutine still completes as cancelled, so a test that only checks `CANCELLED` stays green — which is why the staged-file assertion is the one that bites. Verify both halves separately. ## Not proposed Cancelling the **Media3** export in the same test. `transformer.cancel()` has to run on the engine's `HandlerThread` and its failure mode is different (a hung continuation rather than a runaway process); it deserves its own case, and on emulators the Media3 path is only reachable by pinning `DeviceCodecs.PERMISSIVE` the way `ForcedFailureTest` does — which puts it behind the same problem as the fallback ticket. Splitting is cheap; conflating them would make one test that fails for two unrelated reasons.
JMR-dev commented 2026-09-06 04:53:39 +00:00 (Migrated from github.com)
Copy Link
Copy Source

The FFmpegEngine half is done — PR #236, merged as ad2a75d. Leaving this open for the two engines it does not cover.

Two findings from doing it are worth having here rather than only in the commit, because both would otherwise be rediscovered by whoever takes ConcatEngine:

1. The output-file assertion cannot fail, so it would be a vacuous test. The obvious check — "the partial output is gone after cancelling" — passes whether or not the cancel reaches the session. invokeOnCancellation deletes the path, and on POSIX unlinking a file ffmpeg still holds open leaves ffmpeg writing to the unlinked inode; the path stays gone either way. Removing FFmpegKit.cancel and keeping output.delete() passes it every time.

What separates them is the session's own verdict — ReturnCode.isCancel(session.getReturnCode()). A cancelled session ends with the cancel code; a completed one does not. It is a fact about the session rather than about timing.

Note for ConcatEngine: it has no output.delete() at all (ConcatEngine.kt:80 is cont.invokeOnCancellation { FFmpegKit.cancel(session.getSessionId()) }). Whether that is deliberate is a separate question from this ticket, but it means the file-based assertion is even less available there.

2. Cancelling from the first progress callback loses the race. That is what this ticket suggested, and it was tried first. It failed with state=COMPLETED rc=0: every committed fixture is 2–3 s at 320×240, and the encode finishes before the first statistics callback is delivered and acted on. The progress callback proves the session is running but arrives too late to interrupt it.

FFmpegKit.listSessions() shows the session RUNNING far earlier, so wait on that instead. QualityTier.BEST helps for the same reason — -preset medium leaves more of the encode ahead of the cancel.

What is left

  • ConcatEngine.kt:80 — same shape, same approach should work; see the note above about the missing delete.
  • Media3Engine.kt:120-123 — transformer.cancel() on the engine's HandlerThread. Still deserves its own case, and #223 has since added a reason: reaching the Media3 path on an emulator needs the device profile pinned, and pinning it makes the export behave differently (the goldfish decoder handles the 4:4:4 fixture that a real device rejects). Read #223's class KDoc before designing it.

Verification standard used, for consistency: four consecutive green local API 34 runs plus a mutation that must go red, and all five CI legs green.

**The `FFmpegEngine` half is done** — PR #236, merged as `ad2a75d`. Leaving this open for the two engines it does not cover. Two findings from doing it are worth having here rather than only in the commit, because both would otherwise be rediscovered by whoever takes `ConcatEngine`: **1. The output-file assertion cannot fail, so it would be a vacuous test.** The obvious check — "the partial output is gone after cancelling" — passes whether or not the cancel reaches the session. `invokeOnCancellation` deletes the path, and on POSIX unlinking a file ffmpeg still holds open leaves ffmpeg writing to the *unlinked inode*; the path stays gone either way. Removing `FFmpegKit.cancel` and keeping `output.delete()` passes it every time. What separates them is the session's own verdict — `ReturnCode.isCancel(session.getReturnCode())`. A cancelled session ends with the cancel code; a completed one does not. It is a fact about the session rather than about timing. **Note for `ConcatEngine`: it has no `output.delete()` at all** (`ConcatEngine.kt:80` is `cont.invokeOnCancellation { FFmpegKit.cancel(session.getSessionId()) }`). Whether that is deliberate is a separate question from this ticket, but it means the file-based assertion is even less available there. **2. Cancelling from the first progress callback loses the race.** That is what this ticket suggested, and it was tried first. It failed with `state=COMPLETED rc=0`: every committed fixture is 2–3 s at 320×240, and the encode finishes before the first statistics callback is delivered and acted on. The progress callback proves the session is running but arrives too late to interrupt it. `FFmpegKit.listSessions()` shows the session `RUNNING` far earlier, so wait on that instead. `QualityTier.BEST` helps for the same reason — `-preset medium` leaves more of the encode ahead of the cancel. ## What is left - **`ConcatEngine.kt:80`** — same shape, same approach should work; see the note above about the missing delete. - **`Media3Engine.kt:120-123`** — `transformer.cancel()` on the engine's `HandlerThread`. Still deserves its own case, and #223 has since added a reason: reaching the Media3 path on an emulator needs the device profile pinned, and pinning it makes the export behave differently (the goldfish decoder handles the 4:4:4 fixture that a real device rejects). Read #223's class KDoc before designing it. Verification standard used, for consistency: four consecutive green local API 34 runs plus a mutation that must go red, and all five CI legs green.
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 bug 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#224