From eb5a3b1ed48dce554680ac7e00788dfc6c651d2f Mon Sep 17 00:00:00 2001 From: rainxchzed Date: Sat, 2 May 2026 21:24:43 +0500 Subject: [PATCH] fix: stop direct/mirror download race from clobbering destination file --- .../core/data/services/AndroidDownloader.kt | 101 +++++++++++------- .../core/data/services/DesktopDownloader.kt | 89 ++++++++------- 2 files changed, 112 insertions(+), 78 deletions(-) diff --git a/core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AndroidDownloader.kt b/core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AndroidDownloader.kt index 718d2cb70..def0b1971 100644 --- a/core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AndroidDownloader.kt +++ b/core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AndroidDownloader.kt @@ -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 @@ -29,7 +32,7 @@ class AndroidDownloader( private val files: FileLocationsProvider, ) : Downloader { private val activeDownloads = ConcurrentHashMap() - private val activeFileNames = ConcurrentHashMap() + private val idsByName = ConcurrentHashMap>() private fun buildClient(): OkHttpClient { Authenticator.setDefault(null) @@ -111,17 +114,16 @@ 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)" } @@ -129,7 +131,7 @@ class AndroidDownloader( val call = client.newCall(request) activeDownloads[downloadId] = call - activeFileNames[safeName] = downloadId + idsByName.computeIfAbsent(safeName) { ConcurrentHashMap.newKeySet() }.add(downloadId) try { call.execute().use { response -> @@ -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 @@ -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?, @@ -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 } } diff --git a/core/data/src/jvmMain/kotlin/zed/rainxch/core/data/services/DesktopDownloader.kt b/core/data/src/jvmMain/kotlin/zed/rainxch/core/data/services/DesktopDownloader.kt index 0cd54b537..84a585f7c 100644 --- a/core/data/src/jvmMain/kotlin/zed/rainxch/core/data/services/DesktopDownloader.kt +++ b/core/data/src/jvmMain/kotlin/zed/rainxch/core/data/services/DesktopDownloader.kt @@ -20,6 +20,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 @@ -27,7 +30,7 @@ class DesktopDownloader( private val files: FileLocationsProvider, ) : Downloader { private val activeDownloads = ConcurrentHashMap() - private val nameToId = ConcurrentHashMap() + private val idsByName = ConcurrentHashMap>() private fun buildClient(): OkHttpClient { Authenticator.setDefault(null) @@ -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 -> @@ -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 @@ -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?, @@ -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 }