From b0aaa0062b1cb04c20e2835e508bc5f6abb1660c Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 2 Aug 2026 21:08:30 +0200 Subject: [PATCH 1/2] stop a failed save from destroying the document it was saving saveSync opened the user's own document and copied into it in place, so the window between "destination truncated" and "new content written" was one where any throw left them with a mangled file and a "save failed" toast. One of the one-star reviews in the play console is exactly that: "tried to save, and got an 'Internal Server Error' that destroyed the file. Good thing it was a copy." Four things, in rising order of how often they bite: The destination was opened with the default mode "w", which providers are not required to truncate - that is what "wt" is for. Saving a document shorter than the one already there left the tail of the old file sitting behind the new content, and for a zip container like odt or docx trailing bytes after the central directory are what make it stop opening. This is the likely everyday corruptor, and it needs no failure at all to happen. A provider that rejects the mode outright falls back to "w", so this cannot make saving fail where it worked. close() was called on the output stream but not from a finally, so any throw during the copy leaked the descriptor and left flushing undefined. It is a use block now. The retranslated temp file was only deleted on the success path, so a failed save leaked it. It is deleted in a finally, and only that one: the other branch hands back the cache file of the document that is still open, which must survive. Nothing was kept, so a half-written destination could not be undone. The current content is copied into the cache before the write and put back if the write throws. If the rollback itself fails, that copy is deliberately left behind rather than deleted - at that point it is the only copy of the user's document that exists. Also post onSaveError to the main handler, which onSaveSuccess already did. The listener touches fragment state and calls requireActivity(), neither of which belongs on the loader's background thread. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01E1ANEeai87KtnX5pBzgDEJ --- .../droid/background/LoaderService.kt | 100 ++++++++++++++++-- 1 file changed, 91 insertions(+), 9 deletions(-) diff --git a/app/src/main/java/app/opendocument/droid/background/LoaderService.kt b/app/src/main/java/app/opendocument/droid/background/LoaderService.kt index cb2088a971a7..a22c896dbe82 100644 --- a/app/src/main/java/app/opendocument/droid/background/LoaderService.kt +++ b/app/src/main/java/app/opendocument/droid/background/LoaderService.kt @@ -15,6 +15,7 @@ import app.opendocument.droid.nonfree.CrashManager import app.opendocument.droid.ui.activity.DocumentFragment import com.google.android.gms.common.ConnectionResult import com.google.android.gms.common.GoogleApiAvailability +import java.io.File /** * Owns the four loaders and the background thread they run on, and decides what to try next when @@ -203,11 +204,21 @@ class LoaderService : Service(), FileLoader.FileLoaderListener { private fun saveSync(lastResult: FileLoader.Result, outFile: Uri, htmlDiff: String?) { val options = lastResult.options + // only the retranslated file is ours to remove afterwards - the other branch hands back + // the cache file of the document that is still open + var retranslated: File? = null + var backup: File? = null + var backupIsTheLastCopy = false + try { val fileToSave = if (htmlDiff != null) { - coreLoader.retranslate(options, htmlDiff) - ?: throw RuntimeException("retranslate failed") + val edited = + coreLoader.retranslate(options, htmlDiff) + ?: throw RuntimeException("retranslate failed") + retranslated = edited + + edited } else { // "full save" from the main UI checkNotNull( @@ -217,13 +228,16 @@ class LoaderService : Service(), FileLoader.FileLoaderListener { } } - val outputStream = - checkNotNull(contentResolver.openOutputStream(outFile)) { "cannot write $outFile" } - StreamUtil.copy(fileToSave, outputStream) - outputStream.close() + // the write goes straight into the user's own document, so keep what is there + // until the new content has landed whole + backup = backUp(outFile) - if (htmlDiff != null) { - fileToSave.delete() + try { + writeTo(outFile, fileToSave) + } catch (e: Throwable) { + backupIsTheLastCopy = !restore(outFile, backup) + + throw e } mainHandler.post { withListener { it.onSaveSuccess(outFile) } } @@ -235,7 +249,75 @@ class LoaderService : Service(), FileLoader.FileLoaderListener { ) crashManager.log(e, options.originalUri) - withListener { it.onSaveError() } + mainHandler.post { withListener { it.onSaveError() } } + } finally { + retranslated?.delete() + + // if the rollback did not get the old content back in, this copy is all that is + // left of it - leave it in the cache rather than finishing the job + if (!backupIsTheLastCopy) { + backup?.delete() + } + } + } + + /** Copies [source] over [uri], truncating whatever was there. */ + private fun writeTo(uri: Uri, source: File) { + // "wt" and not the default "w": not every provider truncates for "w", which leaves the + // tail of a longer previous document sitting behind the new content. for a zip container + // like odt or docx that trailing garbage is what makes the file stop opening + val outputStream = + try { + contentResolver.openOutputStream(uri, "wt") + } catch (e: Exception) { + // a provider that rejects the mode outright - saving at all beats truncating + crashManager.log(e, uri) + + contentResolver.openOutputStream(uri) + } + + checkNotNull(outputStream) { "cannot write $uri" }.use { StreamUtil.copy(source, it) } + } + + /** What [uri] holds right now, so a half finished write can be rolled back. */ + private fun backUp(uri: Uri): File? { + val backup = AndroidFileCache.createCacheFile(this) + + return try { + val input = checkNotNull(contentResolver.openInputStream(uri)) { "cannot read $uri" } + StreamUtil.copy(input, backup) + + if (backup.length() > 0) { + backup + } else { + // a document that was just created has nothing worth keeping + backup.delete() + + null + } + } catch (e: Throwable) { + // unreadable target: no worse than before, the save just cannot be rolled back + crashManager.log(e, uri) + backup.delete() + + null + } + } + + /** Puts [backup] back into [uri]. False if the old content could not be restored. */ + private fun restore(uri: Uri, backup: File?): Boolean { + if (backup == null) { + return false + } + + return try { + writeTo(uri, backup) + + true + } catch (e: Throwable) { + crashManager.log(e, uri) + + false } } From ec318c8319672977a9d43d30317e0b2b3a47a7eb Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 2 Aug 2026 21:18:44 +0200 Subject: [PATCH 2/2] keep the backup when the rollback could not truncate writeTo falls back to mode "w" for a provider that rejects "wt", and "w" is the mode that may not truncate - the whole reason the fallback is a fallback. A restore that went in that way can leave the tail of the failed save sitting behind the old content and still look like it worked, and the finally block would then delete the only clean copy there was. writeTo reports whether it truncated, and restore passes that through, so a rollback that could not truncate counts as failed and the backup stays. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01E1ANEeai87KtnX5pBzgDEJ --- .../droid/background/LoaderService.kt | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/app/src/main/java/app/opendocument/droid/background/LoaderService.kt b/app/src/main/java/app/opendocument/droid/background/LoaderService.kt index a22c896dbe82..297910e0de70 100644 --- a/app/src/main/java/app/opendocument/droid/background/LoaderService.kt +++ b/app/src/main/java/app/opendocument/droid/background/LoaderService.kt @@ -261,11 +261,13 @@ class LoaderService : Service(), FileLoader.FileLoaderListener { } } - /** Copies [source] over [uri], truncating whatever was there. */ - private fun writeTo(uri: Uri, source: File) { + /** Copies [source] over [uri]. False if the provider would not truncate first. */ + private fun writeTo(uri: Uri, source: File): Boolean { // "wt" and not the default "w": not every provider truncates for "w", which leaves the // tail of a longer previous document sitting behind the new content. for a zip container // like odt or docx that trailing garbage is what makes the file stop opening + var truncated = true + val outputStream = try { contentResolver.openOutputStream(uri, "wt") @@ -273,10 +275,14 @@ class LoaderService : Service(), FileLoader.FileLoaderListener { // a provider that rejects the mode outright - saving at all beats truncating crashManager.log(e, uri) + truncated = false + contentResolver.openOutputStream(uri) } checkNotNull(outputStream) { "cannot write $uri" }.use { StreamUtil.copy(source, it) } + + return truncated } /** What [uri] holds right now, so a half finished write can be rolled back. */ @@ -304,7 +310,11 @@ class LoaderService : Service(), FileLoader.FileLoaderListener { } } - /** Puts [backup] back into [uri]. False if the old content could not be restored. */ + /** + * Puts [backup] back into [uri]. False if the old content could not be restored - including the + * case where it went in without truncating, which leaves the tail of the failed save behind it + * and is no more readable than what it replaced. + */ private fun restore(uri: Uri, backup: File?): Boolean { if (backup == null) { return false @@ -312,8 +322,6 @@ class LoaderService : Service(), FileLoader.FileLoaderListener { return try { writeTo(uri, backup) - - true } catch (e: Throwable) { crashManager.log(e, uri)