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

S3 — sweepStaging's re-read race has no seam to provoke it #143

New Issue
Closed
opened 2026-08-27 03:07:10 +00:00 by JMR-dev · 1 comment
JMR-dev commented 2026-08-27 03:07:10 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Child 3 of 3 decomposing #133. Independent of S1 (#141). Touches the same file as S2 (#142) — take them in either order, but not in parallel.

Why this exists

app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt:256-269

StagingSweep.collectable(entries, nowMs).forEach { name ->
    val file = File(dir, name)
    // Re-read the timestamp rather than trusting the snapshot above. Between the
    // listing and here, a worker resumed by WorkManager -- which runs in this same
    // process -- could have started writing this very file, and unlinking an inode a
    // running job still holds open would end with the job reporting success for a
    // path that no longer exists.
    if (StagingSweep.isCollectable(file.lastModified(), nowMs)) file.delete()   // :267, 1 of 2 branches cold
}

StagingSweep.collectable is pure and well covered — StagingSweepTest has seven tests. This is the
guard around it, and the branch that never fires is the one that matters: a file that was
collectable in the listing and is not by the time the delete is reached.

That is the entire failure this re-read prevents, and it is the one thing standing between the sweep
and a live job's output. defect-audit.md D2 and D8 are the entries this line came out of.

Scope

Something that can move a file's mtime between the listing and the re-read. Two shapes, both small:

protected open fun entriesIn(dir: File): Array<File>? = dir.listFiles()

— override it to touch a file on the way out; or hoist the listing into an overridable call and do
the same. Either leaves StagingSweep's rule untouched, which is the point: this is about the guard,
not the policy.

Same precedent as S2 (#142) — WorkerStubs.kt's publishers override one method to force one condition.
Note the interaction: sweepStaging already takes nowMs as a parameter specifically so the clock
is the caller's, so the seam needed here is the listing, not the time.

Done means

A file that the listing reports as collectable, whose mtime is then advanced before the delete is
reached, is left on disk. Assert on the file existing, not on a call count.

Mutation: delete the if and delete unconditionally. The test must go red on the surviving file.

Worth checking while here

:271 — canonicalOrAbsolute's getOrDefault(absoluteFile) fallback is 9 instructions never
executed, and belongs to discardStaged rather than the sweep. C6 (#140) of #132 names it; if that child
has not landed, it is a two-line addition here rather than a reason to open anything further.

_Child 3 of 3 decomposing #133. Independent of S1 (#141). **Touches the same file as S2 (#142)** — take them in either order, but not in parallel._ ### Why this exists ``` app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt:256-269 ``` ```kotlin StagingSweep.collectable(entries, nowMs).forEach { name -> val file = File(dir, name) // Re-read the timestamp rather than trusting the snapshot above. Between the // listing and here, a worker resumed by WorkManager -- which runs in this same // process -- could have started writing this very file, and unlinking an inode a // running job still holds open would end with the job reporting success for a // path that no longer exists. if (StagingSweep.isCollectable(file.lastModified(), nowMs)) file.delete() // :267, 1 of 2 branches cold } ``` `StagingSweep.collectable` is pure and well covered — `StagingSweepTest` has seven tests. This is the guard *around* it, and the branch that never fires is the one that matters: **a file that was collectable in the listing and is not by the time the delete is reached.** That is the entire failure this re-read prevents, and it is the one thing standing between the sweep and a live job's output. `defect-audit.md` **D2** and **D8** are the entries this line came out of. ### Scope Something that can move a file's mtime *between* the listing and the re-read. Two shapes, both small: ```kotlin protected open fun entriesIn(dir: File): Array<File>? = dir.listFiles() ``` — override it to touch a file on the way out; or hoist the listing into an overridable call and do the same. Either leaves `StagingSweep`'s rule untouched, which is the point: this is about the guard, not the policy. Same precedent as S2 (#142) — `WorkerStubs.kt`'s publishers override one method to force one condition. Note the interaction: `sweepStaging` already takes `nowMs` as a parameter *specifically* so the clock is the caller's, so the seam needed here is the **listing**, not the time. ### Done means A file that the listing reports as collectable, whose mtime is then advanced before the delete is reached, is **left on disk**. Assert on the file existing, not on a call count. **Mutation:** delete the `if` and delete unconditionally. The test must go red on the surviving file. ### Worth checking while here `:271` — `canonicalOrAbsolute`'s `getOrDefault(absoluteFile)` fallback is 9 instructions never executed, and belongs to `discardStaged` rather than the sweep. C6 (#140) of #132 names it; if that child has not landed, it is a two-line addition here rather than a reason to open anything further.
JMR-dev commented 2026-08-27 03:58:31 +00:00 (Migrated from github.com)
Copy Link
Copy Source

The seam this ticket proposed does not reach the branch. Recording the measurement, because the mistake is easy to repeat and the test looks right while making it.

This ticket suggested:

A protected open fun entriesIn(dir: File) hook, or hoisting the listing into an overridable call, both do it without touching the delete logic.

I cut exactly that, wrote the race test — a file aged past the grace period, touched to now from inside the override — and it passed. Then the mutation came back green: deleting if (StagingSweep.isCollectable(...)) outright left the test passing.

The reason is the ordering:

val listing = dir.listFiles() ?: return
val entries = listing.map { StagingSweep.Entry(it.name, it.lastModified()) }   // <- snapshot
StagingSweep.collectable(entries, nowMs).forEach { name ->
    val file = File(dir, name)
    if (StagingSweep.isCollectable(file.lastModified(), nowMs)) file.delete()  // <- the guard
}

An entriesIn seam fires before the snapshot, so the touch lands in entries itself, collectable never proposes the file, and the loop body is never entered. The test passes for the wrong reason — it demonstrates the first read protecting the file, not the second.

The race is a file that was collectable when the snapshot was taken and is not by the time the delete comes round. So the seam has to sit at the snapshot:

protected open fun snapshot(listing: Array<File>): List<StagingSweep.Entry> =
    listing.map { StagingSweep.Entry(it.name, it.lastModified()) }

With that, deleting the guard reddens the test. Done in PR #151, with the reasoning on the seam's own KDoc so it is not moved back.

Worth noting generally: this is the second seam in this batch where the obvious placement was one step off, and in both cases the only thing that caught it was running the mutation. A green race test is close to meaningless on its own — the whole point of the branch is that it fires rarely.

**The seam this ticket proposed does not reach the branch.** Recording the measurement, because the mistake is easy to repeat and the test looks right while making it. This ticket suggested: > A `protected open fun entriesIn(dir: File)` hook, or hoisting the listing into an overridable call, both do it without touching the delete logic. I cut exactly that, wrote the race test — a file aged past the grace period, touched to `now` from inside the override — and it passed. Then the mutation came back **green**: deleting `if (StagingSweep.isCollectable(...))` outright left the test passing. The reason is the ordering: ```kotlin val listing = dir.listFiles() ?: return val entries = listing.map { StagingSweep.Entry(it.name, it.lastModified()) } // <- snapshot StagingSweep.collectable(entries, nowMs).forEach { name -> val file = File(dir, name) if (StagingSweep.isCollectable(file.lastModified(), nowMs)) file.delete() // <- the guard } ``` An `entriesIn` seam fires **before** the snapshot, so the touch lands in `entries` itself, `collectable` never proposes the file, and the loop body is never entered. The test passes for the wrong reason — it demonstrates the *first* read protecting the file, not the second. The race is a file that **was** collectable when the snapshot was taken and is not by the time the delete comes round. So the seam has to sit at the snapshot: ```kotlin protected open fun snapshot(listing: Array<File>): List<StagingSweep.Entry> = listing.map { StagingSweep.Entry(it.name, it.lastModified()) } ``` With that, deleting the guard reddens the test. Done in PR #151, with the reasoning on the seam's own KDoc so it is not moved back. Worth noting generally: this is the second seam in this batch where the obvious placement was one step off, and in both cases the only thing that caught it was running the mutation. A green race test is close to meaningless on its own — the whole point of the branch is that it fires rarely.
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 enhancement sev:low
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#143