Download follow-ups from #4: atomic final-name claim (S29) + small cleanups #5

New issue

Closed

opened 2026-08-13 23:10:34 +02:00 by julian · 0 comments

julian commented

2026-08-13 23:10:34 +02:00

Owner

Carried out of the #4 review trail — the approving review (b1c13f5) left one follow-up and four nits, none blocking. Original write-up: #4 (comment)

S29 — the final name has the same race the temp name lost

In SchemaViewModel.downloadToAppDownloads (pre-Q path):

val finalName = uniqueName(name) { File(dir, it).exists() }
val target = File(dir, finalName)
if (!temp.renameTo(target)) error(…)

Two transfers can genuinely run at once — two pages on the back stack, each with its own ViewModel and its own guard. If both resolve the same finalName, neither exists() at check time, both compute the same candidate, and the second renameTo silently replaces the first — exactly what uniqueName exists to prevent. Check and claim are two steps; the filesystem is the shared thing between them.

Pre-existing (a per-ViewModel guard never bound cross-page downloads), not introduced by #4.

Fix: minSdk is 26, so Files.move(temp.toPath(), target.toPath()) without REPLACE_EXISTING throws FileAlreadyExistsException atomically. Retry with the next uniqueName candidate on that exception — the loop becomes a claim instead of a poll.

Nits

runAction and submitForm wrap their POST in runCatching without rethrowing CancellationException; probe() and openAction both rethrow. Harmless (token guard catches it), but the rule has two exceptions left.
downloadJob is the only job field without the self-nulling invokeOnCompletion that submitJob/actionJob have. Functionally fine; odd one out.
sweepOrphanParts KDoc overclaims: Q+ "deletes its own pending row on failure" only for failures it sees — process death leaves a pending IS_PENDING row this code never deletes (platform expires it after ~7 days). Doc fix.
Partials written directly into the downloads directory by intermediate commits of the #4 branch sit outside the .parts/ sweep's reach forever. Only affects machines that ran those commits; decide whether a one-time root sweep of .{name}.{random}.part-shaped files is worth it or leave it.

Carried out of the #4 review trail — the approving review (`b1c13f5`) left one follow-up and four nits, none blocking. Original write-up: https://git.imhof.cloud/OpsDeck/mobile/pulls/4#issuecomment-666 ## S29 — the final name has the same race the temp name lost In `SchemaViewModel.downloadToAppDownloads` (pre-Q path): ```kotlin val finalName = uniqueName(name) { File(dir, it).exists() } val target = File(dir, finalName) if (!temp.renameTo(target)) error(…) ``` Two transfers can genuinely run at once — two pages on the back stack, each with its own ViewModel and its own guard. If both resolve the same `finalName`, neither `exists()` at check time, both compute the same candidate, and the second `renameTo` silently replaces the first — exactly what `uniqueName` exists to prevent. Check and claim are two steps; the filesystem is the shared thing between them. Pre-existing (a per-ViewModel guard never bound cross-page downloads), not introduced by #4. **Fix:** `minSdk` is 26, so `Files.move(temp.toPath(), target.toPath())` without `REPLACE_EXISTING` throws `FileAlreadyExistsException` atomically. Retry with the next `uniqueName` candidate on that exception — the loop becomes a claim instead of a poll. ## Nits - [ ] `runAction` and `submitForm` wrap their POST in `runCatching` without rethrowing `CancellationException`; `probe()` and `openAction` both rethrow. Harmless (token guard catches it), but the rule has two exceptions left. - [ ] `downloadJob` is the only job field without the self-nulling `invokeOnCompletion` that `submitJob`/`actionJob` have. Functionally fine; odd one out. - [ ] `sweepOrphanParts` KDoc overclaims: Q+ "deletes its own pending row on failure" only for failures it sees — process death leaves a pending `IS_PENDING` row this code never deletes (platform expires it after ~7 days). Doc fix. - [ ] Partials written directly into the downloads directory by intermediate commits of the #4 branch sit outside the `.parts/` sweep's reach forever. Only affects machines that ran those commits; decide whether a one-time root sweep of `.{name}.{random}.part`-shaped files is worth it or leave it.

julian referenced this issue from a commit

2026-08-13 23:37:00 +02:00

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

julian referenced this issue from a pull request that will close it,

2026-08-13 23:37:16 +02:00

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

julian closed this issue

2026-08-24 16:56:14 +02:00