OpsDeck/mobile
Watch
2
0
Fork
You've already forked mobile
 0
Code Issues 1 Pull requests Projects Releases Packages Wiki Activity Actions

fix(download): a final name the filesystem let us claim, not one a check saw free #7

Merged
julian merged 2 commits from fix/download-name-claim into main 2026-08-24 16:56:14 +02:00
Conversation 3 Commits 2 Files changed 3 +191 -13
julian commented 2026-08-13 23:37:16 +02:00
Owner
Copy link

Closes #5 — the S29 follow-up and the four nits the #4 review trail left behind.

S29 — the claim

uniqueName over exists() then renameTo was check-then-claim: two transfers really can run at once (one per page on the back stack, each with its own ViewModel and its own guard), both resolve the same free candidate, and the loser's rename silently replaces the winner — the exact loss the numbering exists to prevent.

claimFinalName makes the claim atomic: createNewFile() (O_CREAT|O_EXCL) is one operation — whoever creates the placeholder owns the name, and every other transfer numbers past it. A lost race comes back as false, strikes that name, and asks uniqueName for the next. (The named racers are two ViewModels in one process, so an in-memory reserved set would have closed this race too; the filesystem claim costs no more and also holds against writers this process cannot see.)

The move — corrected by review

The first cut moved the bytes with Files.move(…, REPLACE_EXISTING) and argued that was safe because the target was our own placeholder. Review proved it was the bug: REPLACE_EXISTING is not rename(2) — the library unlinks an existing target first and renames onto the freed name, so for a stretch of every call the just-claimed name did not exist, another transfer's exists() read it as free, and both landed under one name (507 lost transfers in 1000 rounds of eight concurrent claims). The claim was real; the move handed it back.

Now ATOMIC_MOVE: a bare rename(2) that replaces the placeholder in one step and never vacates the name. Not renameTo, though on Android it is the same syscall — on the desk, where the unit tests run, renameTo refuses an existing target outright, and the concurrency test would be exercising a different move than the one that ships.

A fill the filesystem refuses (a scanner briefly holding the fresh placeholder — observed on the desk as AccessDeniedException) releases the placeholder, strikes the name, and claims the next candidate; the last refusal rides along as the cause if the loop ever exhausts.

One loss accepted, said out loud: a process death between claim and move strands a zero-byte file under the final name, and later transfers number past it for good. No sweep takes it back — in the saved-downloads root a zero-byte file is indistinguishable from a genuinely empty download, and deleting a saved file is the loss this code exists to prevent (the same principle as the stray-partials decision below). The operator can see it and delete it; code must not.

The test that catches it

200 rounds of eight transfers finishing the same basename at once behind a CyclicBarrier: one distinct final name per transfer, every transfer's own bytes on disk under its own name, and nothing else in the directory — no leftover partial, no stranded placeholder. It fails on the REPLACE_EXISTING move in roughly half the rounds; the sequential test (kept) never could. The losing threads also walk createNewFile() == false, so lost and the retry loop are covered.

Nits

  • runAction and submitForm rethrow the coroutine's own cancellation out of their runCatching, as probe() already did. The token guard absorbed it — a cancelled action's token is already retired — but that was the guard doing the rule's job by accident; the rule now has zero exceptions.
  • downloadJob nulls itself on completion, same pattern and same === guard as actionJob/submitJob.
  • sweepOrphanParts' KDoc stops claiming Q+ cleans up failures it cannot see: a process death strands an IS_PENDING row too, and that row is the platform's to expire (~a week), not ours to sweep.
  • The stray partials intermediate #4 builds left in the downloads root stay (the "decide" item): only machines that ran those commits have them, and a one-time deleter aimed at the saved-files directory is precisely the category of code the sweep's design exists to forbid — the file browser legitimately serves files named like partials.

versionName 0.31.0 → 0.31.1 (bug fix). compileDebugKotlin and testDebugUnitTest green locally (12/12 in DownloadNamingTest, concurrency test rerun clean).

Note for merge order: #6 (spotless) is open; this branch's imports are already in the sorted order that rule enforces, so whichever lands second should rebase clean.

🤖 Generated with Claude Code

Closes #5 — the S29 follow-up and the four nits the #4 review trail left behind. ## S29 — the claim `uniqueName` over `exists()` then `renameTo` was check-then-claim: two transfers really can run at once (one per page on the back stack, each with its own ViewModel and its own guard), both resolve the same free candidate, and the loser's rename silently replaces the winner — the exact loss the numbering exists to prevent. `claimFinalName` makes the claim atomic: `createNewFile()` (`O_CREAT|O_EXCL`) is one operation — whoever creates the placeholder owns the name, and every other transfer numbers past it. A lost race comes back as `false`, strikes that name, and asks `uniqueName` for the next. (The named racers are two ViewModels in one process, so an in-memory reserved set would have closed this race too; the filesystem claim costs no more and also holds against writers this process cannot see.) ## The move — corrected by review The first cut moved the bytes with `Files.move(…, REPLACE_EXISTING)` and argued that was safe because the target was our own placeholder. Review proved it was the bug: `REPLACE_EXISTING` is not `rename(2)` — the library unlinks an existing target first and renames onto the freed name, so for a stretch of every call the just-claimed name did not exist, another transfer's `exists()` read it as free, and both landed under one name (507 lost transfers in 1000 rounds of eight concurrent claims). The claim was real; the move handed it back. Now `ATOMIC_MOVE`: a bare `rename(2)` that replaces the placeholder in one step and never vacates the name. Not `renameTo`, though on Android it is the same syscall — on the desk, where the unit tests run, `renameTo` refuses an existing target outright, and the concurrency test would be exercising a different move than the one that ships. A fill the filesystem refuses (a scanner briefly holding the fresh placeholder — observed on the desk as `AccessDeniedException`) releases the placeholder, strikes the name, and claims the next candidate; the last refusal rides along as the cause if the loop ever exhausts. **One loss accepted, said out loud:** a process death between claim and move strands a zero-byte file under the final name, and later transfers number past it for good. No sweep takes it back — in the saved-downloads root a zero-byte file is indistinguishable from a genuinely empty download, and deleting a saved file is the loss this code exists to prevent (the same principle as the stray-partials decision below). The operator can see it and delete it; code must not. ## The test that catches it 200 rounds of eight transfers finishing the same basename at once behind a `CyclicBarrier`: one distinct final name per transfer, every transfer's own bytes on disk under its own name, and nothing else in the directory — no leftover partial, no stranded placeholder. It fails on the `REPLACE_EXISTING` move in roughly half the rounds; the sequential test (kept) never could. The losing threads also walk `createNewFile() == false`, so `lost` and the retry loop are covered. ## Nits - `runAction` and `submitForm` rethrow the coroutine's own cancellation out of their `runCatching`, as `probe()` already did. The token guard absorbed it — a cancelled action's token is already retired — but that was the guard doing the rule's job by accident; the rule now has zero exceptions. - `downloadJob` nulls itself on completion, same pattern and same `===` guard as `actionJob`/`submitJob`. - `sweepOrphanParts`' KDoc stops claiming Q+ cleans up failures it cannot see: a process death strands an `IS_PENDING` row too, and that row is the platform's to expire (~a week), not ours to sweep. - The stray partials intermediate #4 builds left in the downloads root **stay** (the "decide" item): only machines that ran those commits have them, and a one-time deleter aimed at the saved-files directory is precisely the category of code the sweep's design exists to forbid — the file browser legitimately serves files named like partials. `versionName` 0.31.0 → 0.31.1 (bug fix). `compileDebugKotlin` and `testDebugUnitTest` green locally (12/12 in `DownloadNamingTest`, concurrency test rerun clean). Note for merge order: #6 (spotless) is open; this branch's imports are already in the sorted order that rule enforces, so whichever lands second should rebase clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
S29: the final name had the same race the temp name lost. uniqueName over
exists() is a poll - two transfers really can run at once, one per page on
the back stack, each with its own ViewModel and its own guard - and when
both resolve the same candidate, the loser's renameTo silently replaces
the winner: the exact loss the numbering exists to prevent. The claim is
now createNewFile, whose existence check and creation are one atomic
operation; the move then replaces a placeholder that is ours by
construction, and a lost race comes back as false and strikes that name
for the next uniqueName round. Not Files.move without REPLACE_EXISTING,
whose refusal is a stat before a rename inside the library - the same two
steps, closer together. Pinned by a test that claims past taken names and
placeholders and checks nobody's bytes moved.

Nits from the same trail: the action and submit POSTs rethrow the
coroutine's own cancellation out of their runCatching, as probe() already
did (the token guard absorbed it, but that was the guard doing the rule's
job by accident); downloadJob nulls itself on completion like its two
siblings; sweepOrphanParts' KDoc stops claiming Q+ cleans up failures it
cannot see - a process death strands an IS_PENDING row too, and that row
is the platform's to expire, not ours.

The stray partials intermediate #4 builds left in the downloads root stay:
only machines that ran those commits have them, and a deleter aimed at the
saved-files directory is precisely what the sweep's design forbids.

Closes #5.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thisilike requested changes 2026-08-23 20:57:47 +02:00
Dismissed
thisilike left a comment
Copy link

The claim is handed back before the bytes land — S29 still loses files

I did not take the reasoning on trust; I built the race and ran it. On the pristine PR head, 507 of 1000 rounds (8 concurrent claimFinalName calls on the same name, one temp file each) end with two transfers under the same final name and one transfer's bytes gone. That is the exact loss the PR says it closes.

The cause is one line — StandardCopyOption.REPLACE_EXISTING.

Files.move with REPLACE_EXISTING is not rename(2). In sun.nio.fs.UnixCopyFile.move (the ojluni copy Android ships — the same java.nio.file that minSdk 26 makes available at all) an existing target is unlinked first, and only then is the source renamed over the now-free name. So the winner's own placeholder is deleted before the move, the name goes absent mid-call, and a loser's exists() → createNewFile() walks straight into that hole and "claims" a name the winner is holding.

Measured directly — one thread spinning on exists() while another moves onto the same target 20 000 times:

REPLACE_EXISTING: target absent in 56106 of 337030 polls
ATOMIC_MOVE:      target absent in      0 of 276001 polls

~17% of the call's wall clock is a window where the claimed name does not exist. The KDoc rejects plain Files.move because "its refusal is a stat before a rename inside the library — the same two steps, closer together", and then reaches for the one flag that turns those two steps into three, the middle one being unlink of the very thing being defended. The claim is genuinely atomic; the move throws it away.

Two more symptoms the same probe turned up: a saved file vanishing (FileNotFoundException: a (6).txt (No such file or directory) on reading back a path claimFinalName had just returned — the unlink window seen from the other side), and the numbering ending up sparser than the file count.

The fix is one word

Swap REPLACE_EXISTING for ATOMIC_MOVE, or go back to temp.renameTo(target). Both are a bare rename(2), which replaces the target atomically and never vacates the name. I ran each at 1000 rounds × 8 threads: zero duplicates, zero lost bodies, zero leftovers. renameTo is arguably the better of the two — it is the call that already shipped, so it adds no new exception surface (ATOMIC_MOVE can raise AtomicMoveNotSupportedException on a filesystem that will not do it, and that would only ever show up on a device).

Worth noting once the mechanics are right: the placeholder is what makes this work, and the placeholder also means a process death between createNewFile and the rename leaves a zero-byte file under the final name in getExternalFilesDir(DIRECTORY_DOWNLOADS) — outside .parts/, so sweepParts never sees it, indistinguishable from a real empty download, and poisoning that name for every later transfer. Tiny window, but the PR body explicitly declines to write a root-directory cleaner while this change is what creates the litter such a cleaner would exist for. The honest resolution is probably to teach the sweep about zero-byte files in dir older than PART_ORPHAN_MS, and to stop the KDoc claiming "an unclaimed name must not survive as a zero-byte download" when one can.

The test is green on the broken code

That is the second finding, and it is why the first one shipped. Every assertion in a claimed name lands the bytes and leaves every other file alone passes verbatim against the old uniqueName + renameTo — including the zero-byte-placeholder case, which is just exists(), exactly what the old check already honoured. Nothing in it can reach createNewFile() == false, so lost, the retry, and the error("cannot claim …") backstop have no coverage at all. "Pinned by a test" names three behaviours that all predate the change; the one behaviour the PR exists for is the one nothing exercises. A test that stays green while half the transfers lose their bytes is not a regression net.

The concurrency is testable on the host JVM — this is roughly what I ran:

@Test
fun `concurrent claims never lose bytes`() {
    repeat(200) {
        val dir = Files.createTempDirectory("dl").toFile()
        try {
            val n = 8
            val gate = CyclicBarrier(n)
            val landed = ConcurrentHashMap<String, MutableList<String>>()
            val threads = (0 until n).map { i ->
                Thread {
                    val temp = File(dir, ".a.txt.$i.part").apply { writeText("body-$i") }
                    gate.await()
                    val t = claimFinalName(dir, "a.txt", temp)
                    landed.computeIfAbsent(t.name) {
                        Collections.synchronizedList(mutableListOf())
                    }.add(t.readText())
                }
            }
            threads.forEach { it.start() }
            threads.forEach { it.join() }
            // one name per transfer, and every transfer's own bytes under it
            assertEquals(n, landed.size)
            assertEquals((0 until n).map { "body-$it" }.toSet(), landed.values.flatten().toSet())
        } finally {
            dir.deleteRecursively()
        }
    }
}

If a threaded test in this suite is unwanted, the cheaper seam is a create: (File) -> Boolean = File::createNewFile parameter — a stub that returns false once covers lost and the retry deterministically.

One premise worth correcting even after the fix

"The filesystem is the only thing both transfers share" is not true here. dir is getExternalFilesDir(DIRECTORY_DOWNLOADS) — app-private — and the two racers the issue and the KDoc name are two SchemaViewModels in one process. They share a class loader; a synchronized reserved-name set would have closed this, needed no O_EXCL/FUSE argument, and would have been trivially testable. createNewFile is a fine choice and survives more than that, but the paragraph justifying it is arguing a threat model that does not apply, and the untestability that followed from it is what let the broken move through.

Checked and good

Not rubber-stamped — each of these I chased to the point where it could have been wrong:

  • CancellationException rethrow (runAction 1392, submitForm 2046) — correct, and safe for a non-obvious reason. The rethrow skips result.fold, and neither launch has a finally, so it is load-bearing that a cancellation always arrives with the token already retired. It does: all four cancel sites (start 570-573, endRun 1893-1896) call submitJob?.cancel() / actionJob?.cancel() before claim.release()/releaseRun, and none of those statements suspend, so the release always wins the race to the main thread. I also went looking for a CancellationException from something other than the job's own cancellation — a withTimeout would have wedged the claim silently — and there is none in the schema or network path (withTimeoutOrNull appears only in AlertsMessagingService). Chaining .onFailure onto runCatching { … } binds where you'd want, and kotlinx.coroutines.CancellationException is already imported at line 54.
  • downloadJob self-nulling — genuinely the same shape as actionJob (1470-1473) and submitJob (2122-2124), === guard included. invokeOnCompletion firing off-thread was my worry; the body's tail resumes on Dispatchers.Main after withContext(Dispatchers.IO) returns, so completion and the read in openAction are both on Main. Assigning downloadJob after launch is fine even under Main.immediate: a body that completed before the assignment gets its handler invoked immediately after it.
  • claimFinalName runs on IO, not Main — the whole download is inside withContext(Dispatchers.IO) (1024), so up to 999 exists() calls plus a create and a rename are not a StrictMode violation. Worth stating since the function does noticeably more filesystem work than the two calls it replaced.
  • sweepOrphanParts KDoc — accurate now. MediaStore does set DATE_EXPIRES to ~7 days on a pending insert, and the pre-Q path really does delete temp on every visible failure, claimFinalName's throw included (the outer catch (e: Throwable) at 1284 covers it).
  • The stray-partials decision — agreed, and for the reason given. A root sweep aimed at .{name}.{random}.part shapes in a directory the file browser legitimately serves from is worse than the litter. (It does sit awkwardly beside the zero-byte corpse this PR introduces, which is the same category of thing.)
  • versionName 0.31.1 — single occurrence in the tree, no changelog or README to keep in step.
  • DownloadNamingTest 11/11 green locally, as claimed (--offline testDebugUnitTest, tests="11" failures="0"). The claim is true; it just does not mean what it is being asked to mean.

Everything above is worth keeping. Blocking only on the one line, and on a test that would have caught it.

## The claim is handed back before the bytes land — S29 still loses files I did not take the reasoning on trust; I built the race and ran it. On the pristine PR head, **507 of 1000 rounds** (8 concurrent `claimFinalName` calls on the same `name`, one temp file each) end with two transfers under the **same** final name and one transfer's bytes gone. That is the exact loss the PR says it closes. The cause is one line — `StandardCopyOption.REPLACE_EXISTING`. `Files.move` with `REPLACE_EXISTING` is not `rename(2)`. In `sun.nio.fs.UnixCopyFile.move` (the ojluni copy Android ships — the same `java.nio.file` that `minSdk 26` makes available at all) an existing target is `unlink`ed **first**, and only then is the source `rename`d over the now-free name. So the winner's own placeholder is deleted before the move, the name goes absent mid-call, and a loser's `exists()` → `createNewFile()` walks straight into that hole and "claims" a name the winner is holding. Measured directly — one thread spinning on `exists()` while another moves onto the same target 20 000 times: ``` REPLACE_EXISTING: target absent in 56106 of 337030 polls ATOMIC_MOVE: target absent in 0 of 276001 polls ``` ~17% of the call's wall clock is a window where the claimed name does not exist. The KDoc rejects plain `Files.move` because "its refusal is a `stat` before a `rename` inside the library — the same two steps, closer together", and then reaches for the one flag that turns those two steps into three, the middle one being `unlink` of the very thing being defended. The claim is genuinely atomic; the move throws it away. Two more symptoms the same probe turned up: a saved file **vanishing** (`FileNotFoundException: a (6).txt (No such file or directory)` on reading back a path `claimFinalName` had just returned — the unlink window seen from the other side), and the numbering ending up sparser than the file count. ### The fix is one word Swap `REPLACE_EXISTING` for `ATOMIC_MOVE`, or go back to `temp.renameTo(target)`. Both are a bare `rename(2)`, which replaces the target atomically and never vacates the name. I ran each at **1000 rounds × 8 threads: zero duplicates, zero lost bodies, zero leftovers**. `renameTo` is arguably the better of the two — it is the call that already shipped, so it adds no new exception surface (`ATOMIC_MOVE` can raise `AtomicMoveNotSupportedException` on a filesystem that will not do it, and that would only ever show up on a device). Worth noting once the mechanics are right: the placeholder is what makes this work, and the placeholder also means a process death between `createNewFile` and the rename leaves a **zero-byte file under the final name** in `getExternalFilesDir(DIRECTORY_DOWNLOADS)` — outside `.parts/`, so `sweepParts` never sees it, indistinguishable from a real empty download, and poisoning that name for every later transfer. Tiny window, but the PR body explicitly declines to write a root-directory cleaner while this change is what creates the litter such a cleaner would exist for. The honest resolution is probably to teach the sweep about zero-byte files in `dir` older than `PART_ORPHAN_MS`, and to stop the KDoc claiming "an unclaimed name must not survive as a zero-byte download" when one can. ## The test is green on the broken code That is the second finding, and it is why the first one shipped. Every assertion in `a claimed name lands the bytes and leaves every other file alone` passes **verbatim against the old `uniqueName` + `renameTo`** — including the zero-byte-placeholder case, which is just `exists()`, exactly what the old check already honoured. Nothing in it can reach `createNewFile() == false`, so `lost`, the retry, and the `error("cannot claim …")` backstop have no coverage at all. "Pinned by a test" names three behaviours that all predate the change; the one behaviour the PR exists for is the one nothing exercises. A test that stays green while half the transfers lose their bytes is not a regression net. The concurrency is testable on the host JVM — this is roughly what I ran: ```kotlin @Test fun `concurrent claims never lose bytes`() { repeat(200) { val dir = Files.createTempDirectory("dl").toFile() try { val n = 8 val gate = CyclicBarrier(n) val landed = ConcurrentHashMap<String, MutableList<String>>() val threads = (0 until n).map { i -> Thread { val temp = File(dir, ".a.txt.$i.part").apply { writeText("body-$i") } gate.await() val t = claimFinalName(dir, "a.txt", temp) landed.computeIfAbsent(t.name) { Collections.synchronizedList(mutableListOf()) }.add(t.readText()) } } threads.forEach { it.start() } threads.forEach { it.join() } // one name per transfer, and every transfer's own bytes under it assertEquals(n, landed.size) assertEquals((0 until n).map { "body-$it" }.toSet(), landed.values.flatten().toSet()) } finally { dir.deleteRecursively() } } } ``` If a threaded test in this suite is unwanted, the cheaper seam is a `create: (File) -> Boolean = File::createNewFile` parameter — a stub that returns `false` once covers `lost` and the retry deterministically. ## One premise worth correcting even after the fix "The filesystem is the only thing both transfers share" is not true here. `dir` is `getExternalFilesDir(DIRECTORY_DOWNLOADS)` — app-private — and the two racers the issue and the KDoc name are two `SchemaViewModel`s **in one process**. They share a class loader; a `synchronized` reserved-name set would have closed this, needed no `O_EXCL`/FUSE argument, and would have been trivially testable. `createNewFile` is a fine choice and survives more than that, but the paragraph justifying it is arguing a threat model that does not apply, and the untestability that followed from it is what let the broken move through. ## Checked and good Not rubber-stamped — each of these I chased to the point where it could have been wrong: - **`CancellationException` rethrow (`runAction` 1392, `submitForm` 2046)** — correct, and safe for a non-obvious reason. The rethrow skips `result.fold`, and neither launch has a `finally`, so it is load-bearing that a cancellation always arrives with the token already retired. It does: all four cancel sites (`start` 570-573, `endRun` 1893-1896) call `submitJob?.cancel()` / `actionJob?.cancel()` **before** `claim.release()`/`releaseRun`, and none of those statements suspend, so the release always wins the race to the main thread. I also went looking for a `CancellationException` from something other than the job's own cancellation — a `withTimeout` would have wedged the claim silently — and there is none in the schema or network path (`withTimeoutOrNull` appears only in `AlertsMessagingService`). Chaining `.onFailure` onto `runCatching { … }` binds where you'd want, and `kotlinx.coroutines.CancellationException` is already imported at line 54. - **`downloadJob` self-nulling** — genuinely the same shape as `actionJob` (1470-1473) and `submitJob` (2122-2124), `===` guard included. `invokeOnCompletion` firing off-thread was my worry; the body's tail resumes on `Dispatchers.Main` after `withContext(Dispatchers.IO)` returns, so completion and the read in `openAction` are both on Main. Assigning `downloadJob` after `launch` is fine even under `Main.immediate`: a body that completed before the assignment gets its handler invoked immediately after it. - **`claimFinalName` runs on IO**, not Main — the whole download is inside `withContext(Dispatchers.IO)` (1024), so up to 999 `exists()` calls plus a create and a rename are not a StrictMode violation. Worth stating since the function does noticeably more filesystem work than the two calls it replaced. - **`sweepOrphanParts` KDoc** — accurate now. MediaStore does set `DATE_EXPIRES` to ~7 days on a pending insert, and the pre-Q path really does delete `temp` on every visible failure, `claimFinalName`'s throw included (the outer `catch (e: Throwable)` at 1284 covers it). - **The stray-partials decision** — agreed, and for the reason given. A root sweep aimed at `.{name}.{random}.part` shapes in a directory the file browser legitimately serves from is worse than the litter. (It does sit awkwardly beside the zero-byte corpse this PR introduces, which is the same category of thing.) - **`versionName` 0.31.1** — single occurrence in the tree, no changelog or README to keep in step. - **`DownloadNamingTest` 11/11 green locally**, as claimed (`--offline testDebugUnitTest`, `tests="11" failures="0"`). The claim is true; it just does not mean what it is being asked to mean. Everything above is worth keeping. Blocking only on the one line, and on a test that would have caught it.
app/src/main/java/cloud/imhof/opsdeck/ui/schema/SchemaViewModel.kt Outdated
@ -321,0 +337,4 @@
* from that instant every other transfer's `exists()` sees it and numbers
* past it. The move then replaces a placeholder that is ours by
* construction — `REPLACE_EXISTING` is not the silent overwrite it looks
* like, it is scoped to a name nobody else can hold. (`Files.move` WITHOUT
thisilike commented 2026-08-23 20:57:47 +02:00
Owner
Copy link

This is exactly backwards, and it is the sentence that let the bug through. REPLACE_EXISTING is a silent overwrite of a name another transfer can hold, because its implementation vacates the name before renaming onto it.

And the parenthetical rejects plain Files.move for being "a stat before a rename inside the library" — the flag chosen instead makes it three steps in that same library, the middle one an unlink of the very placeholder being defended. Plain Files.move was too weak; this is actively worse than the renameTo it replaced.

This is exactly backwards, and it is the sentence that let the bug through. `REPLACE_EXISTING` **is** a silent overwrite of a name another transfer can hold, because its implementation vacates the name before renaming onto it. And the parenthetical rejects plain `Files.move` for being "a `stat` before a `rename` inside the library" — the flag chosen instead makes it three steps in that same library, the middle one an `unlink` of the very placeholder being defended. Plain `Files.move` was too weak; this is actively worse than the `renameTo` it replaced.
app/src/main/java/cloud/imhof/opsdeck/ui/schema/SchemaViewModel.kt
@ -321,0 +352,4 @@
val candidate = uniqueName(name) { it in lost || File(dir, it).exists() }
val target = File(dir, candidate)
val claimed = try {
target.createNewFile()
thisilike commented 2026-08-23 20:57:47 +02:00
Owner
Copy link

Once the move is fixed, the placeholder is what makes the claim work — and it is also a new class of litter. A process death between this line and the rename leaves a zero-byte file under the final name in getExternalFilesDir(DIRECTORY_DOWNLOADS): outside .parts/, so sweepParts never sees it, indistinguishable from a real empty download, and it poisons that name for every later transfer (backup.tar.gz (2), (3), forever).

Microseconds wide, so not blocking — but the PR body declines a root-directory cleaner in the same breath as introducing the litter one would exist for, and line 368's "an unclaimed name must not survive as a zero-byte download" only holds for the IOException path. Either teach the sweep about zero-byte files in dir older than PART_ORPHAN_MS, or say out loud that the corpse is accepted.

Once the move is fixed, the placeholder is what makes the claim work — and it is also a new class of litter. A process death between this line and the rename leaves a **zero-byte file under the final name** in `getExternalFilesDir(DIRECTORY_DOWNLOADS)`: outside `.parts/`, so `sweepParts` never sees it, indistinguishable from a real empty download, and it poisons that name for every later transfer (`backup.tar.gz (2)`, `(3)`, forever). Microseconds wide, so not blocking — but the PR body declines a root-directory cleaner in the same breath as introducing the litter one would exist for, and line 368's "an unclaimed name must not survive as a zero-byte download" only holds for the `IOException` path. Either teach the sweep about zero-byte files in `dir` older than `PART_ORPHAN_MS`, or say out loud that the corpse is accepted.
app/src/main/java/cloud/imhof/opsdeck/ui/schema/SchemaViewModel.kt Outdated
@ -321,0 +363,4 @@
return@repeat
}
try {
Files.move(temp.toPath(), target.toPath(), StandardCopyOption.REPLACE_EXISTING)
thisilike commented 2026-08-23 20:57:47 +02:00
Owner
Copy link

This line is the bug. REPLACE_EXISTING is not rename(2): ojluni's UnixCopyFile.move unlinks an existing target first, then renames onto the freed name. So this deletes the placeholder the line above just claimed, and for the duration of the call the name does not exist — another transfer's exists() reads it as free, createNewFile() succeeds, and both land under the same name.

Measured: one thread spinning on exists() while another moves onto the target 20 000 times reports the target absent in 56106 of 337030 polls with REPLACE_EXISTING, and 0 of 276001 with ATOMIC_MOVE. Eight concurrent claimFinalName calls on one name lose a transfer's bytes in 507 of 1000 rounds as written.

if (!temp.renameTo(target)) throw IOException("rename")

or StandardCopyOption.ATOMIC_MOVE. Both are a bare rename(2) — atomic replace, no vacating. 1000 rounds x 8 threads clean on either. renameTo is the safer pick: it is the call that already shipped, so it adds no new exception surface (ATOMIC_MOVE can raise AtomicMoveNotSupportedException, and only ever on a device).

**This line is the bug.** `REPLACE_EXISTING` is not `rename(2)`: ojluni's `UnixCopyFile.move` `unlink`s an existing target *first*, then renames onto the freed name. So this deletes the placeholder the line above just claimed, and for the duration of the call the name does not exist — another transfer's `exists()` reads it as free, `createNewFile()` succeeds, and both land under the same name. Measured: one thread spinning on `exists()` while another moves onto the target 20 000 times reports the target **absent in 56106 of 337030 polls** with `REPLACE_EXISTING`, and **0 of 276001** with `ATOMIC_MOVE`. Eight concurrent `claimFinalName` calls on one name lose a transfer's bytes in **507 of 1000 rounds** as written. ```kotlin if (!temp.renameTo(target)) throw IOException("rename") ``` or `StandardCopyOption.ATOMIC_MOVE`. Both are a bare `rename(2)` — atomic replace, no vacating. 1000 rounds x 8 threads clean on either. `renameTo` is the safer pick: it is the call that already shipped, so it adds no new exception surface (`ATOMIC_MOVE` can raise `AtomicMoveNotSupportedException`, and only ever on a device).
app/src/test/java/cloud/imhof/opsdeck/DownloadNamingTest.kt
@ -108,0 +131,4 @@
// this very instant — it counts as taken exactly like a file
assertTrue(File(downloads, "a (3).txt").createNewFile())
val third = File(downloads, ".a.txt.3.part").apply { writeText("three") }
assertEquals("a (4).txt", claimFinalName(downloads, "a.txt", third).name)
thisilike commented 2026-08-23 20:57:47 +02:00
Owner
Copy link

Every assertion in this test passes verbatim against the old uniqueName + renameTo. A zero-byte placeholder counting as taken is just exists(), which the old check already honoured — so all three behaviours the PR body lists as "pinned" predate the change, and nothing here can reach createNewFile() == false. lost, the retry loop and the error("cannot claim …") backstop have zero coverage.

Proof that this matters: this test is green right now, while 8 concurrent claims lose a transfer's bytes in 507 of 1000 rounds.

The race is testable on the host JVM — CyclicBarrier, 8 threads, one temp file each, assert one distinct name per transfer and each transfer's own bytes under its own name (full version in the review body). If a threaded test is unwanted here, add a create: (File) -> Boolean = File::createNewFile parameter and stub one false — that covers lost and the retry deterministically.

Every assertion in this test passes **verbatim against the old `uniqueName` + `renameTo`**. A zero-byte placeholder counting as taken is just `exists()`, which the old check already honoured — so all three behaviours the PR body lists as "pinned" predate the change, and nothing here can reach `createNewFile() == false`. `lost`, the retry loop and the `error("cannot claim …")` backstop have zero coverage. Proof that this matters: this test is green right now, while 8 concurrent claims lose a transfer's bytes in 507 of 1000 rounds. The race is testable on the host JVM — `CyclicBarrier`, 8 threads, one temp file each, assert one distinct name per transfer and each transfer's own bytes under its own name (full version in the review body). If a threaded test is unwanted here, add a `create: (File) -> Boolean = File::createNewFile` parameter and stub one `false` — that covers `lost` and the retry deterministically.
Review of #7 proved the claim was being handed back mid-move:
Files.move with REPLACE_EXISTING is not rename(2) — the library unlinks
an existing target first and renames onto the freed name, so for a
stretch of every call the just-claimed placeholder did not exist,
another transfer's exists() read the name as free, and both landed under
it. Measured at 507 lost transfers in 1000 rounds of eight concurrent
claims. The atomic claim was real; the move threw it away.

ATOMIC_MOVE is the bare rename(2) that replaces the placeholder in one
step. Not renameTo, though on Android it is the same syscall: on the
desk, where the unit tests run, renameTo refuses an existing target
outright, and the new concurrency test would be exercising a different
move than the one that ships.

A fill the filesystem refuses (a scanner holding the fresh placeholder —
observed on this desk as AccessDeniedException under MoveFileEx) now
releases the placeholder, strikes the name, and claims the next
candidate; the last refusal rides along as the cause if the loop ever
exhausts.

The test that should have caught this now exists: 200 rounds of eight
transfers finishing the same basename at once behind a CyclicBarrier,
asserting one distinct final name per transfer, every transfer's own
bytes on disk, and nothing else in the directory. It fails on the old
move in roughly half the rounds; the sequential test never could. The
losing threads also cover the createNewFile() == false path, so lost
and the retry loop are no longer unexercised.

Accepted and now documented instead of half-claimed: a process death
between claim and move strands a zero-byte file under the final name.
No sweep takes it back — in the saved-downloads root a zero-byte file
is indistinguishable from a genuinely empty download, and deleting a
saved file is the loss this code exists to prevent. Also dropped the
KDoc premise that the filesystem is the only thing the racers share;
the named racers are two ViewModels in one process, and the filesystem
claim is kept because it costs no more and holds against writers this
process cannot see.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
julian commented 2026-08-23 22:57:15 +02:00
Author
Owner
Copy link

Reworked in 4d18c66 — every finding addressed, PR description updated to match.

The move (blocking): you were right, and the KDoc paragraph defending REPLACE_EXISTING was exactly backwards — the flag's unlink-then-rename vacates the claimed name mid-call. Swapped for a bare rename(2), with one deviation from your preferred fix: ATOMIC_MOVE, not renameTo. Reason: the unit tests run on a Windows host, where renameTo refuses an existing target outright — the first cut of this rework used renameTo and your concurrency test failed instantly with expected:<8> but was:<0>, because the test would have been exercising a different move than the one that ships. ATOMIC_MOVE is rename(2) on Android/Linux and MoveFileEx(MOVEFILE_REPLACE_EXISTING) on the desk: replaces in one step on both, never vacates. Your AtomicMoveNotSupportedException concern is covered — it is an IOException, and a refused fill no longer throws: it deletes the placeholder, strikes the name into lost, and claims the next candidate (the last refusal becomes the cause if the loop ever exhausts). That retry path is not theoretical: on this desk, Defender briefly holding the fresh placeholder made ATOMIC_MOVE fail with AccessDeniedException in the first green-on-Linux-shaped run of the concurrency test.

The test (blocking): your test, essentially verbatim — 200 rounds × 8 threads behind a CyclicBarrier, asserting one distinct final name per transfer and every transfer's own bytes, plus a third assertion that the directory holds exactly the names handed back (no leftover partial, no stranded placeholder; deliberately not asserted dense, since a struck name legitimately leaves a numbering hole). Temp files are written before the threads start so a thread dying short of the barrier cannot hang the other seven. The losing threads walk createNewFile() == false, so lost and the retry loop are covered without the stub seam. 12/12 green across three runs.

The zero-byte corpse: accepted and said out loud rather than swept. A zero-byte file in the saved-downloads root is indistinguishable from a genuinely empty download, and deleting a saved file is the loss this whole function exists to prevent — the same principle that keeps the stray-partials deleter out, now applied consistently. KDoc states the loss, its window, and that it is the operator's to delete; the "an unclaimed name must not survive as a zero-byte download" claim is now scoped to the refusal path, where it is actually true.

The premise: dropped. The KDoc now says the named racers are two ViewModels in one process and a reserved-name set would have closed that race; createNewFile is kept because it costs no more and also holds against writers this process cannot see.

Reworked in 4d18c66 — every finding addressed, PR description updated to match. **The move (blocking):** you were right, and the KDoc paragraph defending `REPLACE_EXISTING` was exactly backwards — the flag's unlink-then-rename vacates the claimed name mid-call. Swapped for a bare `rename(2)`, with one deviation from your preferred fix: `ATOMIC_MOVE`, not `renameTo`. Reason: the unit tests run on a Windows host, where `renameTo` refuses an existing target outright — the first cut of this rework used `renameTo` and your concurrency test failed instantly with `expected:<8> but was:<0>`, because the test would have been exercising a different move than the one that ships. `ATOMIC_MOVE` is `rename(2)` on Android/Linux and `MoveFileEx(MOVEFILE_REPLACE_EXISTING)` on the desk: replaces in one step on both, never vacates. Your `AtomicMoveNotSupportedException` concern is covered — it is an `IOException`, and a refused fill no longer throws: it deletes the placeholder, strikes the name into `lost`, and claims the next candidate (the last refusal becomes the cause if the loop ever exhausts). That retry path is not theoretical: on this desk, Defender briefly holding the fresh placeholder made `ATOMIC_MOVE` fail with `AccessDeniedException` in the first green-on-Linux-shaped run of the concurrency test. **The test (blocking):** your test, essentially verbatim — 200 rounds × 8 threads behind a `CyclicBarrier`, asserting one distinct final name per transfer and every transfer's own bytes, plus a third assertion that the directory holds exactly the names handed back (no leftover partial, no stranded placeholder; deliberately not asserted dense, since a struck name legitimately leaves a numbering hole). Temp files are written before the threads start so a thread dying short of the barrier cannot hang the other seven. The losing threads walk `createNewFile() == false`, so `lost` and the retry loop are covered without the stub seam. 12/12 green across three runs. **The zero-byte corpse:** accepted and said out loud rather than swept. A zero-byte file in the saved-downloads root is indistinguishable from a genuinely empty download, and deleting a saved file is the loss this whole function exists to prevent — the same principle that keeps the stray-partials deleter out, now applied consistently. KDoc states the loss, its window, and that it is the operator's to delete; the "an unclaimed name must not survive as a zero-byte download" claim is now scoped to the refusal path, where it is actually true. **The premise:** dropped. The KDoc now says the named racers are two ViewModels in one process and a reserved-name set would have closed that race; `createNewFile` is kept because it costs no more and also holds against writers this process cannot see.
thisilike approved these changes 2026-08-24 14:05:08 +02:00
thisilike left a comment
Copy link

The move is fixed, and I proved it rather than read it

I did not take the rework on trust either. On the PR head (4d18c66) I flipped the one word back — ATOMIC_MOVE → REPLACE_EXISTING, nothing else — and ran the suite three times: concurrent claims of one name never lose a transfer's bytes failed 3/3, expected:<8> but was:<7>. Restored the word: 5/5 green. So the test does catch the exact regression it was written for, on a Linux host, reliably — that was the part of the last round that mattered most, and it holds.

Then I pushed harder than the shipped test does: 3000 rounds × 16 threads (15× the committed load, ~48 000 threads) — clean, 1.5 s. And I instrumented both retry branches to check the coverage claim is real rather than nominal: one class run hits createNewFile() == false 752 times, so lost and the retry loop are genuinely walked, not just reachable in principle.

CI parity checked too, since .forgejo/workflows/build.yml only fires on push to main and this PR therefore gets no pre-merge run: lintDebug clean (the only output is the baseline's own meta-warnings plus a pre-existing ModifierParameter, nothing from this diff — NewApi in particular is silent, java.nio.file is API 26 and minSdk is 26), and testReleaseUnitTest — the variant CI actually runs, not the debug one you quoted — green across every class.

Approving. Four follow-ups below, none blocking.

1. The refusal branch has no coverage on the host CI uses

The instrumented run recorded zero refusals on Linux. That is expected — unlink of an open file just works here — but it means refused, target.delete(), and the IllegalStateException("cannot claim a name …", refused) backstop are exercised only incidentally, on your desk, by Defender. The branch you added in answer to the last review is the one part of the new code nothing pins.

It is deterministic in three lines: hand the claim a temp that does not exist, and every fill is refused with NoSuchFileException. I wrote it and ran it — green, 13/13:

@Test
fun `a fill the filesystem refuses leaves no placeholder behind`() {
    val downloads = Files.createTempDirectory("downloads").toFile()
    try {
        val gone = File(downloads, ".a.txt.gone.part")
        val e = try {
            claimFinalName(downloads, "a.txt", gone); null
        } catch (t: IllegalStateException) { t }
        assertTrue(e!!.message!!.startsWith("cannot claim a name for a.txt"))
        assertTrue(e.cause is java.nio.file.NoSuchFileException)
        // the refusal path released every placeholder it claimed
        assertEquals(emptyList<String>(), downloads.list()!!.toList())
    } finally {
        downloads.deleteRecursively()
    }
}

That one test covers the strike, the delete, the exhaustion, the cause riding along, and the "no corpse" property — all four, on the host CI runs.

2. The accepted loss is wider than the KDoc scopes it to

One loss is accepted: a process death between the claim and the rename …

Process death is not the only way. target.delete()'s result is ignored, and the refusal you actually observed — a scanner holding the fresh placeholder — is precisely the state in which a Windows delete also fails. When it does, the placeholder survives as a zero-byte file under the final name with no crash involved, poisoning that number exactly as the process-death case does. Two consequences: the KDoc's "one loss, and it needs a process death" is too narrow, and the third assertion (assertEquals(landed.keys, downloads.list()!!.toSet())) is then a real flake on the one host where the refusal has been seen.

The shipping impact is nil — Android unlinks an open file — so the code needs nothing. The sentence does: the loss is a claim this transfer could not release, whatever stopped it.

3. Worth writing down: this only ships to API 26–28

claimFinalName is reached from downloadToAppDownloads, which is the else of SDK_INT >= Q (line 1051). Everything from Android 10 up takes downloadViaMediaStore, where the final name is settled by MediaStore's own insert — an atomic dedupe, no check-then-claim anywhere in that path, so there is genuinely nothing to fix there. I checked, because "the PR fixed the legacy path and left the modern one racing" would have been blocking.

It does mean the 507/1000 loss I measured last round was only ever reachable on Android 8–9. Fine — but the KDoc argues from "two pages on the back stack", which reads as a statement about any device, and the next person to touch the Q+ path deserves the qualifier where the reasoning lives.

4. "on the desk, where these tests run"

They also run on cth-ubuntu-latest, twice per push to main (lintDebug, testReleaseUnitTest), where renameTo would have been fine. The reason for ATOMIC_MOVE is sound and I would not change it — a test that must be green on both hosts has to exercise the move that ships — but the clause puts the tests in one place and there are two.

Checked and good

Re-verified at the new head, not carried over from the last round:

  • The cancellation rethrow still cannot strand the spinner. runAction (1417) and submitForm (2068) now throw past result.fold, and neither launch has a finally — so it is load-bearing that the release always wins. Both cancel sites hold: start (595-599) and endRun (1918-1921) each cancel() the jobs and then claim.release() / releaseRun(error) with no suspension in between, and releaseRun clears uploading, runningAction, submittingForm, uploadPercent in one update. A cancellation dispatched from the IO thread queues behind that. Nothing sticks.
  • downloadJob self-nulling is right against abandonDownload(), which nulls the field itself — the handler's === then finds a field that is null or a later job and no-ops. Assigning after launch is safe under Main.immediate: a body that finished first gets its handler invoked on registration.
  • The eager createNewFile cannot be steered out of the directory. name is whatever the server's content-disposition said, so I chased it: filenameFromDisposition → sanitiseFilename strips through the last / and \ and rejects "", ., .. before it ever reaches the claim. The old renameTo had the same exposure, but this version creates on that name, so it was worth confirming rather than assuming.
  • lost is tested before exists() in the predicate — it in lost || File(dir, it).exists(). That ordering is what keeps the retry loop's syscall count bounded by what is in the directory instead of by iteration count; reversed, a thousand refusals would be a stat storm on the IO dispatcher. Easy to lose in a later tidy-up.
  • AtomicMoveNotSupportedException is unreachable here, which settles the concern I raised: on Unix it is EXDEV-only, and temp lives in dir/.parts/ — same volume by construction. Handling it as a plain IOException is right anyway.
  • versionName 0.31.1 is the only occurrence in the tree; no changelog or README to keep in step. Import order in both files already matches what #6 will enforce, so the rebase note holds.
  • The dropped premise paragraph reads correctly now, and keeping createNewFile over a reserved-name set on the "costs no more" ground is the right call — the set would have been process-local and this is not.

Everything the last round blocked on is closed, and closed in a way a rerun would catch again. The four items above are follow-ups, not gates.

## The move is fixed, and I proved it rather than read it I did not take the rework on trust either. On the PR head (`4d18c66`) I flipped the one word back — `ATOMIC_MOVE` → `REPLACE_EXISTING`, nothing else — and ran the suite three times: **`concurrent claims of one name never lose a transfer's bytes` failed 3/3**, `expected:<8> but was:<7>`. Restored the word: **5/5 green**. So the test does catch the exact regression it was written for, on a Linux host, reliably — that was the part of the last round that mattered most, and it holds. Then I pushed harder than the shipped test does: **3000 rounds × 16 threads** (15× the committed load, ~48 000 threads) — clean, 1.5 s. And I instrumented both retry branches to check the coverage claim is real rather than nominal: one class run hits `createNewFile() == false` **752 times**, so `lost` and the retry loop are genuinely walked, not just reachable in principle. CI parity checked too, since `.forgejo/workflows/build.yml` only fires on push to main and this PR therefore gets no pre-merge run: `lintDebug` clean (the only output is the baseline's own meta-warnings plus a pre-existing `ModifierParameter`, nothing from this diff — `NewApi` in particular is silent, `java.nio.file` is API 26 and `minSdk` is 26), and `testReleaseUnitTest` — the variant CI actually runs, not the debug one you quoted — green across every class. Approving. Four follow-ups below, none blocking. ## 1. The refusal branch has no coverage on the host CI uses The instrumented run recorded **zero** refusals on Linux. That is expected — unlink of an open file just works here — but it means `refused`, `target.delete()`, and the `IllegalStateException("cannot claim a name …", refused)` backstop are exercised only incidentally, on your desk, by Defender. The branch you added in answer to the last review is the one part of the new code nothing pins. It is deterministic in three lines: hand the claim a temp that does not exist, and every fill is refused with `NoSuchFileException`. I wrote it and ran it — green, 13/13: ```kotlin @Test fun `a fill the filesystem refuses leaves no placeholder behind`() { val downloads = Files.createTempDirectory("downloads").toFile() try { val gone = File(downloads, ".a.txt.gone.part") val e = try { claimFinalName(downloads, "a.txt", gone); null } catch (t: IllegalStateException) { t } assertTrue(e!!.message!!.startsWith("cannot claim a name for a.txt")) assertTrue(e.cause is java.nio.file.NoSuchFileException) // the refusal path released every placeholder it claimed assertEquals(emptyList<String>(), downloads.list()!!.toList()) } finally { downloads.deleteRecursively() } } ``` That one test covers the strike, the delete, the exhaustion, the cause riding along, and the "no corpse" property — all four, on the host CI runs. ## 2. The accepted loss is wider than the KDoc scopes it to > One loss is accepted: a process death between the claim and the rename … Process death is not the only way. `target.delete()`'s result is ignored, and the refusal you actually observed — a scanner holding the fresh placeholder — is precisely the state in which a Windows delete also fails. When it does, the placeholder survives as a zero-byte file under the final name with **no crash involved**, poisoning that number exactly as the process-death case does. Two consequences: the KDoc's "one loss, and it needs a process death" is too narrow, and the third assertion (`assertEquals(landed.keys, downloads.list()!!.toSet())`) is then a real flake on the one host where the refusal has been seen. The shipping impact is nil — Android unlinks an open file — so the code needs nothing. The sentence does: the loss is a claim this transfer could not release, whatever stopped it. ## 3. Worth writing down: this only ships to API 26–28 `claimFinalName` is reached from `downloadToAppDownloads`, which is the `else` of `SDK_INT >= Q` (line 1051). Everything from Android 10 up takes `downloadViaMediaStore`, where the final name is settled by MediaStore's own insert — an atomic dedupe, no check-then-claim anywhere in that path, so there is genuinely nothing to fix there. I checked, because "the PR fixed the legacy path and left the modern one racing" would have been blocking. It does mean the 507/1000 loss I measured last round was only ever reachable on Android 8–9. Fine — but the KDoc argues from "two pages on the back stack", which reads as a statement about any device, and the next person to touch the Q+ path deserves the qualifier where the reasoning lives. ## 4. "on the desk, where these tests run" They also run on `cth-ubuntu-latest`, twice per push to main (`lintDebug`, `testReleaseUnitTest`), where `renameTo` would have been fine. The **reason** for `ATOMIC_MOVE` is sound and I would not change it — a test that must be green on both hosts has to exercise the move that ships — but the clause puts the tests in one place and there are two. ## Checked and good Re-verified at the new head, not carried over from the last round: - **The cancellation rethrow still cannot strand the spinner.** `runAction` (1417) and `submitForm` (2068) now throw past `result.fold`, and neither launch has a `finally` — so it is load-bearing that the release always wins. Both cancel sites hold: `start` (595-599) and `endRun` (1918-1921) each `cancel()` the jobs and then `claim.release()` / `releaseRun(error)` with no suspension in between, and `releaseRun` clears `uploading`, `runningAction`, `submittingForm`, `uploadPercent` in one `update`. A cancellation dispatched from the IO thread queues behind that. Nothing sticks. - **`downloadJob` self-nulling is right against `abandonDownload()`**, which nulls the field itself — the handler's `===` then finds a field that is null or a later job and no-ops. Assigning after `launch` is safe under `Main.immediate`: a body that finished first gets its handler invoked on registration. - **The eager `createNewFile` cannot be steered out of the directory.** `name` is whatever the server's `content-disposition` said, so I chased it: `filenameFromDisposition` → `sanitiseFilename` strips through the last `/` and `\` and rejects `""`, `.`, `..` before it ever reaches the claim. The old `renameTo` had the same exposure, but this version *creates* on that name, so it was worth confirming rather than assuming. - **`lost` is tested before `exists()` in the predicate** — `it in lost || File(dir, it).exists()`. That ordering is what keeps the retry loop's syscall count bounded by what is in the directory instead of by iteration count; reversed, a thousand refusals would be a stat storm on the IO dispatcher. Easy to lose in a later tidy-up. - **`AtomicMoveNotSupportedException` is unreachable here**, which settles the concern I raised: on Unix it is `EXDEV`-only, and `temp` lives in `dir/.parts/` — same volume by construction. Handling it as a plain `IOException` is right anyway. - **`versionName` 0.31.1** is the only occurrence in the tree; no changelog or README to keep in step. Import order in both files already matches what #6 will enforce, so the rebase note holds. - The dropped premise paragraph reads correctly now, and keeping `createNewFile` over a reserved-name set on the "costs no more" ground is the right call — the set would have been process-local and this is not. Everything the last round blocked on is closed, and closed in a way a rerun would catch again. The four items above are follow-ups, not gates.
julian merged commit cc702a6638 into main 2026-08-24 16:56:14 +02:00
julian deleted branch fix/download-name-claim 2026-08-24 16:56:14 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
thisilike
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
OpsDeck/mobile!7
No description provided.