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

A save that finishes after Start over puts Saved back on a cleared screen #123

New Issue
Open
opened 2026-08-26 03:37:58 +00:00 by JMR-dev · 1 comment
JMR-dev commented 2026-08-26 03:37:58 +00:00 (Migrated from github.com)
Copy Link
Copy Source

A save that finishes after "Start over" puts Saved back on a screen the user cleared

Found while fixing #49, and deliberately not fixed there, because closing it means choosing
between two defensible answers and a race fix is the wrong place to decide that by accident.

The interleaving

ConversionViewModel.save (and JoinViewModel.save, identically) writes its result after a hop:

viewModelScope.launch {
    runCatching {
        withContext(Dispatchers.IO) { publisher.publish(pending.staged, destination); pending.staged.delete() }
    }.onSuccess {
        pendingStaged = null
        _state.value = ConversionState.Saved(pending.suggestedName)
    }.onFailure { e -> _state.value = ConversionState.Failed(...) }
}

The screen stays on Converted for the whole of that copy, and Converted renders both "Save"
and "Start over". So:

  1. User taps Save. The copy starts on Dispatchers.IO.
  2. User taps Start over. reset() runs on the main thread: state to Idle, staged file deleted.
  3. The copy finishes. onSuccess writes Saved(name) over Idle.

The user asked for a blank screen and got a success message for work they had just dismissed.

Why #49's fix does not cover it

#49 added ScreenOwnership: every deferred write checks the claim it was made under. save is the
one deferred write deliberately left out, and the KDoc on ConversionViewModel.ownership says so
and says exactly this much.

The reason it was left out is that guarding it is not obviously right. publish may genuinely have
copied the bytes to the user's destination before reset() deleted the staged copy. Dropping the
write reports nothing for a file that really was saved; keeping it reports success for something the
user dismissed. Neither is clearly the lesser evil.

What a fix probably looks like

The question is about what the screen should offer during a save, not about the write itself:

  • disable "Start over" while a save is in flight (the copy is short, and this removes the race
    rather than arbitrating it), or
  • guard the write with the ownership token and accept that a completed-but-dismissed save is
    silent, or
  • let it land and make Saved reachable from Idle as a transient message rather than a state.

The first looks likeliest to be right, and it is a UI change rather than a ViewModel one.

Testing note

ScreenOwnership's claim in reset() is currently unreddenable by the JVM suite, and that is
the same fact as this ticket: the only deferred write that could land on top of a reset() is
save's, and save is the exempt one. Whatever closes this should also make that claim bite —
RecordingPublisher is subclassable and a publish that blocks on a latch is enough to construct
the interleaving deterministically, the same way ParkedPickDispatcher does for a pick.

## A save that finishes after "Start over" puts `Saved` back on a screen the user cleared Found while fixing #49, and deliberately **not** fixed there, because closing it means choosing between two defensible answers and a race fix is the wrong place to decide that by accident. ### The interleaving `ConversionViewModel.save` (and `JoinViewModel.save`, identically) writes its result after a hop: ```kotlin viewModelScope.launch { runCatching { withContext(Dispatchers.IO) { publisher.publish(pending.staged, destination); pending.staged.delete() } }.onSuccess { pendingStaged = null _state.value = ConversionState.Saved(pending.suggestedName) }.onFailure { e -> _state.value = ConversionState.Failed(...) } } ``` The screen stays on `Converted` for the whole of that copy, and `Converted` renders **both** "Save" and "Start over". So: 1. User taps Save. The copy starts on `Dispatchers.IO`. 2. User taps Start over. `reset()` runs on the main thread: state to `Idle`, staged file deleted. 3. The copy finishes. `onSuccess` writes `Saved(name)` over `Idle`. The user asked for a blank screen and got a success message for work they had just dismissed. ### Why #49's fix does not cover it #49 added `ScreenOwnership`: every deferred write checks the claim it was made under. `save` is the one deferred write deliberately left out, and the KDoc on `ConversionViewModel.ownership` says so and says exactly this much. The reason it was left out is that guarding it is not obviously right. `publish` may genuinely have copied the bytes to the user's destination before `reset()` deleted the staged copy. Dropping the write reports nothing for a file that really was saved; keeping it reports success for something the user dismissed. Neither is clearly the lesser evil. ### What a fix probably looks like The question is about what the screen should offer *during* a save, not about the write itself: - disable "Start over" while a save is in flight (the copy is short, and this removes the race rather than arbitrating it), or - guard the write with the ownership token and accept that a completed-but-dismissed save is silent, or - let it land and make `Saved` reachable from `Idle` as a transient message rather than a state. The first looks likeliest to be right, and it is a UI change rather than a ViewModel one. ### Testing note `ScreenOwnership`'s claim in `reset()` is currently **unreddenable by the JVM suite**, and that is the same fact as this ticket: the only deferred write that could land on top of a `reset()` is `save`'s, and `save` is the exempt one. Whatever closes this should also make that claim bite — `RecordingPublisher` is subclassable and a `publish` that blocks on a latch is enough to construct the interleaving deterministically, the same way `ParkedPickDispatcher` does for a pick.
JMR-dev commented 2026-08-26 03:44:57 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Verified on main (b49295d), and there is a second bad outcome

The interleaving is real on main today, independently of #124. save():

 9:   viewModelScope.launch {
11:       withContext(Dispatchers.IO) { publisher.publish(...); staged.delete() }
15:   }.onSuccess {
18:       _state.value = ConversionState.Saved(pending.suggestedName)     <- unguarded
19:   }.onFailure { e ->
33:       _state.value = ConversionState.Failed(e.message ?: "...", pending)  <- also unguarded

reset():

 8:   viewModelScope.launch(cleanupDispatcher) { publisher.discardStaged(staged) }
10:   _state.value = ConversionState.Idle

So the deferred write is unguarded on both arms, and the ticket's scenario describes only the
success one.

The failure arm is the worse case. reset() does not merely set Idle — it launches
discardStaged(staged) on cleanupDispatcher while publish may still be reading that same file.
If the delete wins, publish throws, and the user gets a red error message on a screen they
deliberately cleared, for a save they cancelled. That is strictly more alarming than a stray
success, and it is reachable by the same three taps.

It also means the two dispatchers are racing over the file itself, not just over _state. Whichever
option is chosen, that needs an answer too — "disable Start over while a save is in flight" happens
to close both, which is a point in its favour beyond the one the ticket gives.

On the unreddenable claim

The ticket's closing note is the important part and should not get lost: ScreenOwnership's claim in
reset() cannot currently be reddened by the JVM suite, because save is the only deferred write
that could land on a reset() and save is the exempt one. That is an assertion with no mutation
behind it — the exact shape this repo has measured before (9 of 46 mutations vacuous, five over a
completely unguarded path).

Disclosing it rather than letting it read as covered is the right call. Whatever closes this ticket
should make that claim bite, via a RecordingPublisher subclass whose publish blocks on a latch —
the same construction ParkedPickDispatcher already uses for a pick.

## Verified on `main` (`b49295d`), and there is a second bad outcome The interleaving is real on `main` today, independently of #124. `save()`: ``` 9: viewModelScope.launch { 11: withContext(Dispatchers.IO) { publisher.publish(...); staged.delete() } 15: }.onSuccess { 18: _state.value = ConversionState.Saved(pending.suggestedName) <- unguarded 19: }.onFailure { e -> 33: _state.value = ConversionState.Failed(e.message ?: "...", pending) <- also unguarded ``` `reset()`: ``` 8: viewModelScope.launch(cleanupDispatcher) { publisher.discardStaged(staged) } 10: _state.value = ConversionState.Idle ``` So the deferred write is unguarded on **both** arms, and the ticket's scenario describes only the success one. **The failure arm is the worse case.** `reset()` does not merely set `Idle` — it launches `discardStaged(staged)` on `cleanupDispatcher` while `publish` may still be reading that same file. If the delete wins, `publish` throws, and the user gets a **red error message on a screen they deliberately cleared**, for a save they cancelled. That is strictly more alarming than a stray success, and it is reachable by the same three taps. It also means the two dispatchers are racing over the file itself, not just over `_state`. Whichever option is chosen, that needs an answer too — "disable Start over while a save is in flight" happens to close both, which is a point in its favour beyond the one the ticket gives. ## On the unreddenable claim The ticket's closing note is the important part and should not get lost: `ScreenOwnership`'s claim in `reset()` cannot currently be reddened by the JVM suite, because `save` is the only deferred write that could land on a `reset()` and `save` is the exempt one. That is an assertion with no mutation behind it — the exact shape this repo has measured before (9 of 46 mutations vacuous, five over a completely unguarded path). Disclosing it rather than letting it read as covered is the right call. Whatever closes this ticket should make that claim bite, via a `RecordingPublisher` subclass whose `publish` blocks on a latch — the same construction `ParkedPickDispatcher` already uses for a pick.
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 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#123