desktop: fix chat switch deleting the in-app updater's download (#7295)

* desktop: fix chat switch deleting the in-app updater's download createTmpFileAndDelete registered every temp file it created in ChatModel.filesToDelete, which ComposeView.deleteUnusedFiles() empties on every chat open/switch/close. The updater's ~350 MB download lives in such a file for minutes, so switching chats during it deleted the file. copyTo kept writing through the still-open fd, so the transfer completed and progress reached 100%; only the following Files.move failed, with NoSuchFileException on the source, and that went into a catch that logs to stdout only - no dialog, no error, no file. The registration was redundant for every caller: the helper's own finally { tmpFile.delete() } already removes the file, deleteOnExit() covers a clean JVM exit, and Main.kt wipes tmpDir at startup for crashes. It only ever duplicated the finally while arming a trap for long-running lambdas - the same window also existed for the settings and themes writers. * desktop: download the app update into a file the updater owns Per review of #7295: instead of removing the ChatModel.filesToDelete registration from createTmpFileAndDelete, stop using that helper for the downloaded file. downloadAsset now writes into "<asset name>.part" in tmpDir and moves it onto the asset name when the transfer completes, so the download is never registered in ChatModel.filesToDelete and ComposeView.deleteUnusedFiles() can no longer delete it mid-transfer. Cleanup is unchanged in substance: the finally block removes the partial file when the download fails or is cancelled (and is a no-op after a successful move, as the helper's own finally was), deleteOnExit covers a clean exit, and Main.kt wipes tmpDir at startup after a crash. createTmpFileAndDelete is restored to its previous behaviour, with a comment warning that its file does not survive a chat switch. * desktop: reduce comments in updater download fix
2026-07-28 12:09:41 +00:00 · 2026-07-24 21:07:56 +01:00
parent 5c196d0e2d
commit e7c94472c4
2 changed files with 154 additions and 6 deletions
@@ -316,21 +316,23 @@ private suspend fun downloadAsset(asset: GitHubAsset) {
    call.execute().use { response ->
      response.body?.use { body ->
        body.byteStream().use { stream ->
+          val newFile = File(tmpDir, asset.name)
          // On Windows the install path exits the app before it can delete the downloaded file, so a
          // previous update's installer can be left in the temp dir; remove it here at the next
          // download instead of letting it accumulate. Other platforms delete the file after install.
-          if (desktopPlatform.isWindows()) File(tmpDir, asset.name).delete()
-          createTmpFileAndDelete { file ->
+          if (desktopPlatform.isWindows()) newFile.delete()
+          val partFile = File(tmpDir, "${asset.name}.part")
+          partFile.parentFile.mkdirs()
+          partFile.deleteOnExit()
+          try {
            // It's important to close output stream (with use{}), otherwise, Windows cannot rename the file
-            file.outputStream().use { output ->
+            partFile.outputStream().use { output ->
              stream.copyTo(output)
            }
-            val newFile = File(file.parentFile, asset.name)
            // Moving instead of renameTo: a bare rename can silently fail (returns false, ignored),
-            // and the enclosing createTmpFileAndDelete then deletes the only copy in its finally block,
            // leaving the user with an empty download dir. Files.move performs the same in-place rename
            // when possible, falls back to copy when it can't, and throws (handled below) on real failure.
-            Files.move(file.toPath(), newFile.toPath(), StandardCopyOption.REPLACE_EXISTING)
+            Files.move(partFile.toPath(), newFile.toPath(), StandardCopyOption.REPLACE_EXISTING)

            AlertManager.shared.showAlertDialogButtonsColumn(
              generalGetString(MR.strings.app_check_for_updates_download_completed_title),
@@ -370,6 +372,8 @@ private suspend fun downloadAsset(asset: GitHubAsset) {
                }
              }
            )
+          } finally {
+            partFile.delete()
          }
        }
      }
@@ -0,0 +1,144 @@
+# Fix: chat switch deletes the in-app updater's download mid-transfer
+
+## Symptom
+
+Desktop in-app updater (Linux AppImage, v7.0.0-beta.5): the download progress reaches 100%, and then
+nothing happens — no "App update is downloaded" dialog, no error, and `/tmp/simplex` is empty.
+The update cannot proceed and there is no indication of why.
+
+Captured by starting the app from a terminal (the log goes nowhere otherwise — see *Why this was invisible*):
+
+```
+E: Failed to download the asset from release: java.nio.file.NoSuchFileException: /tmp/simplex/e859e2cb-b4c6-4ebf-812d-c327812db212
+    at java.base/sun.nio.fs.UnixCopyFile.move(Unknown Source)
+    at java.base/java.nio.file.Files.move(Unknown Source)
+    at chat.simplex.common.views.helpers.AppUpdaterKt.downloadAsset$lambda$...(AppUpdater.kt:333)
+    at chat.simplex.common.views.helpers.UtilsKt.createTmpFileAndDelete(Utils.kt:401)
+    at chat.simplex.common.views.helpers.AppUpdaterKt.downloadAsset(AppUpdater.kt:323)
+```
+
+The exception names the **source** path, not the destination: the file being moved no longer exists.
+
+## Root cause
+
+The updater downloads into `createTmpFileAndDelete`, which registers every file it creates in a
+chat-scoped cleanup set:
+
+```kotlin
+fun <T> createTmpFileAndDelete(dir: File = tmpDir, onCreated: (File) -> T): T {
+  val tmpFile = File(dir, UUID.randomUUID().toString())
+  tmpFile.parentFile.mkdirs()
+  tmpFile.deleteOnExit()
+  ChatModel.filesToDelete.add(tmpFile)          // <-- kills the download
+  try { return onCreated(tmpFile) } finally { tmpFile.delete() }
+}
+```
+
+`ComposeView.deleteUnusedFiles()` empties that set:
+
+```kotlin
+chatModel.filesToDelete.forEach { it.delete() }
+chatModel.filesToDelete.clear()
+```
+
+and it is called from `KeyChangeEffect(chatModel.chatId.value)` — on chat open, switch and close,
+whenever the composer is empty and there is no draft (`ComposeView.kt:1320`), plus after sending a
+live message (`:1301`) and a voice message (`:860`).
+
+`downloadAsset` writes the ~350 MB update into that temp file, which takes minutes, and the toast shown
+during it ("Downloading app update, don't close the app") invites the user to keep using the app
+meanwhile. Any chat switch in that window deletes the file out from under the running download.
+`copyTo` keeps writing through the still-open file descriptor, so the transfer completes normally and
+the progress indicator reaches 100%; only the subsequent `Files.move` fails.
+
+The failure is then swallowed by `downloadAsset`'s outer `catch`, which only calls `Log.e` — on desktop
+that is a bare `println` (`Log.desktop.kt`), invisible unless the app was launched from a terminal.
+Net effect: silence.
+
+This is not specific to the updater. The same window exists for the settings and themes writers
+(`Resources.desktop.kt`, `Files.kt`), which use the same helper and then `Files.move` the temp file into
+place; a chat switch landing between the write and the move surfaces there as an
+"Error saving settings" alert and a rethrow.
+
+### Why this was invisible until v7.0-beta.1
+
+Before #7104 the same line was `file.renameTo(newFile)`, whose `false` return was ignored, so the
+dialog appeared regardless — over an empty folder (that was the bug #7104 fixed). Replacing it with
+`Files.move` was correct, but it converted a silently-ignored failure into a thrown exception routed
+into a stdout-only log, which turned the visible-but-wrong symptom into no symptom at all. The
+underlying deletion predates both.
+
+## Fix
+
+`downloadAsset` stops using `createTmpFileAndDelete` and owns the file itself:
+
+```kotlin
+val newFile = File(tmpDir, asset.name)
+if (desktopPlatform.isWindows()) newFile.delete()
+val partFile = File(tmpDir, "${asset.name}.part")
+partFile.parentFile.mkdirs()
+partFile.deleteOnExit()
+try {
+  partFile.outputStream().use { output -> stream.copyTo(output) }
+  Files.move(partFile.toPath(), newFile.toPath(), StandardCopyOption.REPLACE_EXISTING)
+  AlertManager.shared.showAlertDialogButtonsColumn(...)   // unchanged
+} finally {
+  partFile.delete()
+}
+```
+
+The download is never registered in `ChatModel.filesToDelete`, so `deleteUnusedFiles()` cannot reach
+it and a chat switch during the transfer is harmless. Cleanup is unchanged in substance: the `finally`
+removes a partial file when the download fails or is cancelled (and is a no-op after a successful
+move, exactly as the helper's own `finally` was), `deleteOnExit()` covers a clean exit, and `Main.kt`
+wipes `tmpDir` at startup after a crash — a leftover `.part` is also truncated by the next download.
+
+This is the fix requested in review (#7295): the helper's contract is a scratch file that dies with
+the lambda, and the updater's file is neither — it is moved to `asset.name` and kept for the user to
+install, so the helper's `finally` never applied to it on the success path. `createTmpFileAndDelete`
+is left as it is on master, with a comment warning that its file does not survive a chat switch.
+
+## Other callers
+
+Unchanged. The same trap still exists for anyone whose lambda outlives a chat switch, but no current
+caller does: the settings/themes writers (`Resources.desktop.kt` ×2, `Files.kt`) write and `Files.move`
+within milliseconds, and `CIFileView.kt`, `Utils.kt` and `Share.android.kt` finish with the file inside
+the lambda. See "Follow-up" below.
+
+## Alternatives considered
+
+- **Removing `ChatModel.filesToDelete.add(tmpFile)` from the helper** (the first version of this PR) —
+  fixes the whole class rather than one caller, and the registration is redundant with the helper's
+  own `finally` at every call site. Rejected in review as too broad for this bug: it changes behaviour
+  at six unrelated call sites to fix the updater. Kept as a follow-up, see below.
+- **`filesToDelete.remove(file)` at the updater's call site** — leaves a window between the `add`
+  inside the helper and the `remove` in the lambda, and adds a third concurrent mutation of
+  `ChatModel.filesToDelete` (a plain `mutableSetOf`, already mutated from both `Dispatchers.Default`
+  and the main thread).
+
+## Follow-up (not in this PR)
+
+`createTmpFileAndDelete` still registers its file in `ChatModel.filesToDelete` from whatever thread
+calls it, including `Dispatchers.IO` (settings save, `saveFileFromUri`), while
+`ComposeView.deleteUnusedFiles()` iterates that plain `mutableSetOf` on the UI thread — a concurrent
+`add` during that `forEach` throws `ConcurrentModificationException`. The registration is also
+redundant with the helper's `finally` at all six call sites. Removing it, or making the set
+thread-safe, is a separate change.
+
+Note also that `preferencesTmpDir` on desktop is `File(configPath, "tmp")` — outside `tmpDir`, so it is
+not covered by the startup wipe in `Main.kt`; its temp files rely on the `finally` and `deleteOnExit`.
+
+## Testing
+
+Repro and verification were done against the first version of this fix: with `desktop.version_name`
+temporarily set to `7.0-beta.4` the updater offers v7.0.0-beta.5, and switching chats repeatedly during
+the download reproduces the disappearing file and the missing dialog. This version compiles
+(`:common:compileKotlinDesktop`); the manual download-and-switch run should be repeated before merge,
+including the install path on Windows (where the app exits before deleting the file).
+
+## Out of scope
+
+`downloadAsset` and `checkForUpdate` report failures only via `Log.e`, which on desktop reaches nothing
+but stdout — that is why this bug went unnoticed. Surfacing updater failures to the user is a separate
+concern: it would not have prevented the deletion, and this change does not make future failures
+visible. Left for a follow-up.