Skip to content

fix: stop direct/mirror download race from clobbering destination file #487

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rainxchzed merged 1 commit into from
May 2, 2026
Merged

fix: stop direct/mirror download race from clobbering destination file #487

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 61 additions & 40 deletions core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AndroidDownloader.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ import java.net.Authenticator
import java.net.InetSocketAddress
import java.net.PasswordAuthentication
import java.net.Proxy
import java.nio.file.AtomicMoveNotSupportedException
import java.nio.file.Files
import java.nio.file.StandardCopyOption
import java.util.UUID
import java.util.concurrent.ConcurrentHashMap
import java.util.concurrent.TimeUnit
Expand All @@ -29,7 +32,7 @@ class AndroidDownloader(
private val files: FileLocationsProvider,
) : Downloader {
private val activeDownloads = ConcurrentHashMap<String, Call>()
private val activeFileNames = ConcurrentHashMap<String, String>()
private val idsByName = ConcurrentHashMap<String, MutableSet<String>>()

private fun buildClient(): OkHttpClient {
Authenticator.setDefault(null)
Expand Down Expand Up @@ -111,25 +114,24 @@ class AndroidDownloader(
"Invalid file name: $rawName"
}

check(!activeFileNames.containsKey(safeName)) {
"A download for '$safeName' is already in progress"
}

val downloadId = UUID.randomUUID().toString()

val destination = File(dir, safeName)
if (destination.exists()) {
Logger.d { "Deleting existing file before download: ${destination.absolutePath}" }
destination.delete()
}
// Each attempt writes to its own temp file so MultiSourceDownloader's
// direct/mirror race cannot have two jobs trampling the same path
// (see issue: "File not ready after download" with custom mirror).
// Temp lives in the same dir so the final rename stays on one FS
// and ATOMIC_MOVE works.
val tempFile = File(dir, "$safeName.part-$downloadId")
if (tempFile.exists()) tempFile.delete()

Logger.d { "Starting download: $url (id=$downloadId)" }

val request = Request.Builder().url(url).build()
val call = client.newCall(request)

activeDownloads[downloadId] = call
activeFileNames[safeName] = downloadId
idsByName.computeIfAbsent(safeName) { ConcurrentHashMap.newKeySet() }.add(downloadId)

try {
call.execute().use { response ->
Expand All @@ -142,7 +144,7 @@ class AndroidDownloader(
val total = if (contentLength > 0) contentLength else null

body.byteStream().use { input ->
destination.outputStream().use { output ->
tempFile.outputStream().use { output ->
val buffer = ByteArray(8192)
var downloaded: Long = 0
var bytesRead: Int
Expand All @@ -156,26 +158,49 @@ class AndroidDownloader(
}
}

if (destination.exists() && destination.length() > 0) {
Logger.d { "Download complete: ${destination.absolutePath}" }
val finalDownloaded = destination.length()
val finalPercent =
if (total != null) ((finalDownloaded * 100L) / total).toInt() else 100
emit(DownloadProgress(finalDownloaded, total, finalPercent))
} else {
throw IllegalStateException("File not ready after download: ${destination.absolutePath}")
if (!tempFile.exists() || tempFile.length() <= 0) {
throw IllegalStateException(
"Download produced empty file: ${tempFile.absolutePath} (contentLength=$contentLength)",
)
}

moveAtomic(tempFile, destination)

Logger.d { "Download complete: ${destination.absolutePath}" }
val finalDownloaded = destination.length()
val finalPercent =
if (total != null) ((finalDownloaded * 100L) / total).toInt() else 100
emit(DownloadProgress(finalDownloaded, total, finalPercent))
}
} catch (e: Exception) {
destination.delete()
tempFile.delete()
Logger.e(e) { "Download failed" }
throw e
} finally {
activeDownloads.remove(downloadId)
activeFileNames.remove(safeName)
idsByName.computeIfPresent(safeName) { _, set ->
set.remove(downloadId)
if (set.isEmpty()) null else set
}
}
}.flowOn(Dispatchers.IO)

private fun moveAtomic(source: File, target: File) {
try {
Files.move(
source.toPath(),
target.toPath(),
StandardCopyOption.REPLACE_EXISTING,
StandardCopyOption.ATOMIC_MOVE,
)
} catch (_: AtomicMoveNotSupportedException) {
// Fallback for filesystems without atomic-move support — still
// safer than writing directly to target because the partial bytes
// were never visible at `target` until this step.
Files.move(source.toPath(), target.toPath(), StandardCopyOption.REPLACE_EXISTING)
}
}

override suspend fun saveToFile(
url: String,
suggestedFileName: String?,
Expand Down Expand Up @@ -219,28 +244,24 @@ class AndroidDownloader(

override suspend fun cancelDownload(fileName: String): Boolean =
withContext(Dispatchers.IO) {
var cancelled = false

val downloadId = activeFileNames[fileName]
if (downloadId != null) {
activeDownloads[downloadId]?.let { call: Call ->
if (!call.isCanceled()) {
call.cancel()
cancelled = true
}
activeDownloads.remove(downloadId)
}
activeFileNames.remove(fileName)
// Cancel every in-flight download for this fileName — MultiSource
// races run direct + mirror in parallel under the same logical
// name, so a single-id lookup would leave one of them running.
val ids = idsByName.remove(fileName)?.toList().orEmpty()
if (ids.isEmpty()) return@withContext false

// Only delete the file if we cancelled an active download (incomplete file)
if (cancelled) {
val file = File(files.appDownloadsDir(), fileName)
if (file.exists()) {
file.delete()
}
var cancelled = false
for (id in ids) {
val call = activeDownloads.remove(id) ?: continue
if (!call.isCanceled()) {
call.cancel()
cancelled = true
}
}

// No destination delete: the flow's catch handles its own temp
// file. The final destination is only written via atomic-rename
// on success, so it's either a prior valid download (keep) or
// doesn't exist yet.
cancelled
}
}
89 changes: 51 additions & 38 deletions core/data/src/jvmMain/kotlin/zed/rainxch/core/data/services/DesktopDownloader.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,17 @@ import java.net.Authenticator
import java.net.InetSocketAddress
import java.net.PasswordAuthentication
import java.net.Proxy
import java.nio.file.AtomicMoveNotSupportedException
import java.nio.file.Files
import java.nio.file.StandardCopyOption
import java.util.UUID
import java.util.concurrent.ConcurrentHashMap

class DesktopDownloader(
private val files: FileLocationsProvider,
) : Downloader {
private val activeDownloads = ConcurrentHashMap<String, Call>()
private val nameToId = ConcurrentHashMap<String, String>()
private val idsByName = ConcurrentHashMap<String, MutableSet<String>>()

private fun buildClient(): OkHttpClient {
Authenticator.setDefault(null)
Expand Down Expand Up @@ -101,22 +104,19 @@ class DesktopDownloader(
}

val downloadId = UUID.randomUUID().toString()
val previous = nameToId.putIfAbsent(safeName, downloadId)
if (previous != null) {
throw IllegalStateException("A download for '$safeName' is already in progress")
}

val destination = File(dir, safeName)
if (destination.exists()) {
Logger.d { "Deleting existing file before download: ${destination.absolutePath}" }
destination.delete()
}
// Each attempt writes to its own temp file so MultiSourceDownloader's
// direct/mirror race cannot have two jobs trampling the same path.
val tempFile = File(dir, "$safeName.part-$downloadId")
if (tempFile.exists()) tempFile.delete()

Logger.d { "Starting download: $url" }

val request = Request.Builder().url(url).build()
val call = client.newCall(request)
activeDownloads[downloadId] = call
idsByName.computeIfAbsent(safeName) { ConcurrentHashMap.newKeySet() }.add(downloadId)

try {
call.execute().use { response ->
Expand All @@ -129,7 +129,7 @@ class DesktopDownloader(
val total = if (contentLength > 0) contentLength else null

body.byteStream().use { input ->
destination.outputStream().use { output ->
tempFile.outputStream().use { output ->
val buffer = ByteArray(DEFAULT_BUFFER_SIZE)
var downloaded: Long = 0
var bytesRead: Int
Expand All @@ -143,26 +143,46 @@ class DesktopDownloader(
}
}

if (destination.exists() && destination.length() > 0) {
Logger.d { "Download complete: ${destination.absolutePath}" }
val finalDownloaded = destination.length()
val finalPercent =
if (total != null) ((finalDownloaded * 100L) / total).toInt() else 100
emit(DownloadProgress(finalDownloaded, total, finalPercent))
} else {
throw IllegalStateException("File not ready after download: ${destination.absolutePath}")
if (!tempFile.exists() || tempFile.length() <= 0) {
throw IllegalStateException(
"Download produced empty file: ${tempFile.absolutePath} (contentLength=$contentLength)",
)
}

moveAtomic(tempFile, destination)

Logger.d { "Download complete: ${destination.absolutePath}" }
val finalDownloaded = destination.length()
val finalPercent =
if (total != null) ((finalDownloaded * 100L) / total).toInt() else 100
emit(DownloadProgress(finalDownloaded, total, finalPercent))
}
} catch (e: Exception) {
destination.delete()
tempFile.delete()
Logger.e(e) { "Download failed" }
throw e
} finally {
activeDownloads.remove(downloadId)
nameToId.remove(safeName)
idsByName.computeIfPresent(safeName) { _, set ->
set.remove(downloadId)
if (set.isEmpty()) null else set
}
}
}.flowOn(Dispatchers.IO)

private fun moveAtomic(source: File, target: File) {
try {
Files.move(
source.toPath(),
target.toPath(),
StandardCopyOption.REPLACE_EXISTING,
StandardCopyOption.ATOMIC_MOVE,
)
} catch (_: AtomicMoveNotSupportedException) {
Files.move(source.toPath(), target.toPath(), StandardCopyOption.REPLACE_EXISTING)
}
}

override suspend fun saveToFile(
url: String,
suggestedFileName: String?,
Expand Down Expand Up @@ -200,27 +220,20 @@ class DesktopDownloader(

override suspend fun cancelDownload(fileName: String): Boolean =
withContext(Dispatchers.IO) {
var cancelled = false
val downloadId = nameToId[fileName]
if (downloadId != null) {
activeDownloads[downloadId]?.let { call ->
if (!call.isCanceled()) {
call.cancel()
cancelled = true
}
}
activeDownloads.remove(downloadId)
nameToId.remove(fileName)
// Cancel every in-flight download for this fileName — MultiSource
// races run direct + mirror in parallel under the same logical
// name, so a single-id lookup would leave one of them running.
val ids = idsByName.remove(fileName)?.toList().orEmpty()
if (ids.isEmpty()) return@withContext false

// Only delete the file if we cancelled an active download (incomplete file)
if (cancelled) {
val file = File(files.userDownloadsDir(), fileName)
if (file.exists()) {
file.delete()
}
var cancelled = false
for (id in ids) {
val call = activeDownloads.remove(id) ?: continue
if (!call.isCanceled()) {
call.cancel()
cancelled = true
}
}

cancelled
}

Expand Down