From c493a4fa7b61e13e82c1136ef0c22247e3e9b4b0 Mon Sep 17 00:00:00 2001 From: Tien Do Nam Date: Mon, 27 Jul 2026 05:15:51 +0200 Subject: [PATCH] feat: reopen file instead of deleting and creating --- .../localsend/localsend_app/MainActivity.kt | 34 +++++++--- packages/core/src/http/server/common/save.rs | 16 +++++ packages/core/tests/v2_server.rs | 64 +++++++++++++++++++ .../lib/src/isolate/child/server_isolate.dart | 41 +++++++----- .../lib/src/task/server/file_saver.dart | 40 ++++++------ .../lib/util/android_channel.dart | 16 +++-- 6 files changed, 156 insertions(+), 55 deletions(-) diff --git a/app/android/app/src/main/kotlin/org/localsend/localsend_app/MainActivity.kt b/app/android/app/src/main/kotlin/org/localsend/localsend_app/MainActivity.kt index 7bc27337..8ddc23b7 100644 --- a/app/android/app/src/main/kotlin/org/localsend/localsend_app/MainActivity.kt +++ b/app/android/app/src/main/kotlin/org/localsend/localsend_app/MainActivity.kt @@ -63,7 +63,7 @@ class MainActivity : FlutterActivity() { "createFile" -> handleCreateFile(call, result) - "deleteFile" -> handleDeleteFile(call, result) + "openFileForWriting" -> handleOpenFileForWriting(call, result) "openContentUri" -> { openUri(context, call.argument("uri")!!) @@ -157,7 +157,9 @@ class MainActivity : FlutterActivity() { return } - val parcelFileDescriptor = contentResolver.openFileDescriptor(documentUri, "w") + // "wt" is write + truncate: the document is new, unless the provider + // handed out an existing one instead of creating a second document. + val parcelFileDescriptor = contentResolver.openFileDescriptor(documentUri, "wt") if (parcelFileDescriptor == null) { result.error("OPEN_FAILED", "The content provider did not return a file descriptor", null) return @@ -178,11 +180,15 @@ class MainActivity : FlutterActivity() { } } - /// Deletes a document created by [handleCreateFile]. + /// Opens an existing document created by [handleCreateFile] for writing, + /// discarding its current content. /// - /// Returns whether the document was deleted. A document that no longer - /// exists is reported as not deleted instead of as an error. - private fun handleDeleteFile(call: MethodCall, result: MethodChannel.Result) { + /// Used to write a file again after a failed attempt, so that it keeps its + /// name instead of being created a second time under a numbered one. + /// + /// Returns an owned writable file descriptor. It stays open after this call + /// and must be closed by the native consumer it is passed to. + private fun handleOpenFileForWriting(call: MethodCall, result: MethodChannel.Result) { val uriString = call.argument("uri") if (uriString == null) { result.error("INVALID_ARGUMENT", "Missing content URI", null) @@ -196,13 +202,21 @@ class MainActivity : FlutterActivity() { } try { - result.success(DocumentsContract.deleteDocument(contentResolver, uri)) + // "wt" is write + truncate. A document provider may ignore the + // truncation, so the writer additionally shortens the file itself. + val parcelFileDescriptor = contentResolver.openFileDescriptor(uri, "wt") + if (parcelFileDescriptor == null) { + result.error("OPEN_FAILED", "The content provider did not return a file descriptor", null) + return + } + + parcelFileDescriptor.use { + result.success(it.detachFd()) + } } catch (e: SecurityException) { result.error("PERMISSION_DENIED", e.message ?: "Permission denied for content URI", null) - } catch (e: java.io.FileNotFoundException) { - result.success(false) } catch (e: Exception) { - result.error("DELETE_FAILED", e.message ?: "Failed to delete file", null) + result.error("OPEN_FAILED", e.message ?: "Failed to open content URI", null) } } diff --git a/packages/core/src/http/server/common/save.rs b/packages/core/src/http/server/common/save.rs index f93dd1d5..f6690969 100644 --- a/packages/core/src/http/server/common/save.rs +++ b/packages/core/src/http/server/common/save.rs @@ -242,6 +242,9 @@ fn spawn_file_writer( /// /// Fails if the total number of written bytes does not match `expected_size` /// (e.g. the sender disconnected mid-transfer). +/// +/// The file is truncated to the written size, so that a target that pointed at +/// a longer, pre-existing file cannot keep a tail of the old content. async fn write_file_from_receiver( open: impl Future>, expected_size: u64, @@ -276,5 +279,18 @@ async fn write_file_from_receiver( "Expected {expected_size} bytes, received {written}" )); } + + // Drops content beyond the file that was just written, in case the target + // pointed at a longer, pre-existing file: opening truncates for paths and + // for descriptors opened with the SAF "wt" mode, but a document provider is + // free to ignore that mode. Retries of the same upload are covered by the + // exact size check above either way. + // + // Best-effort: a provider may back the descriptor by something that cannot + // be truncated (e.g. a pipe), which must not fail the completed transfer. + if let Err(e) = file.set_len(written).await { + tracing::warn!("Could not truncate file to {written} bytes: {e}"); + } + Ok(()) } diff --git a/packages/core/tests/v2_server.rs b/packages/core/tests/v2_server.rs index d81f76fa..6bde4789 100644 --- a/packages/core/tests/v2_server.rs +++ b/packages/core/tests/v2_server.rs @@ -498,6 +498,70 @@ async fn test_upload_retry_after_mismatched_sha256() { assert!(matches!(session_ends[0].1, SessionEndReasonV2::Finished)); } +/// A retry writes to the same path as the failed attempt, so the receiver ends +/// up with exactly one file holding the correct content. +#[tokio::test] +async fn test_upload_retry_reuses_the_same_path() { + let save_dir = std::env::temp_dir().join(format!("localsend-test-{}", uuid::Uuid::new_v4())); + tokio::fs::create_dir_all(&save_dir).await.unwrap(); + + let server = start_test_server(None, true, Some(save_dir.clone())).await; + let client = LsHttpClientV2::try_new_without_cert().unwrap(); + + let bytes: Vec = (0..50_000u32).map(|i| i as u8).collect(); + let mut corrupted = bytes.clone(); + *corrupted.last_mut().unwrap() ^= 0xff; + let mut file = file_dto("file-a", "a.bin", bytes.len() as u64); + file.sha256 = Some(sha256_hex(&bytes)); + + let response = client + .prepare_upload( + ProtocolType::Http, + "127.0.0.1", + server.port, + None, + prepare_upload_request(&[file]), + None, + ) + .await + .unwrap() + .response + .unwrap(); + + let result = upload_bytes( + &client, + server.port, + &response.session_id, + "file-a", + &response.files["file-a"], + &corrupted, + ) + .await; + assert_status(result, 422); + + upload_bytes( + &client, + server.port, + &response.session_id, + "file-a", + &response.files["file-a"], + &bytes, + ) + .await + .unwrap(); + + // The corrupted attempt has been overwritten, not kept as a second file. + let mut entries = tokio::fs::read_dir(&save_dir).await.unwrap(); + let mut file_names = Vec::new(); + while let Some(entry) = entries.next_entry().await.unwrap() { + file_names.push(entry.file_name().to_string_lossy().to_string()); + } + assert_eq!(file_names, vec!["file-a".to_string()]); + assert_eq!(tokio::fs::read(save_dir.join("file-a")).await.unwrap(), bytes); + + tokio::fs::remove_dir_all(&save_dir).await.unwrap(); +} + #[tokio::test] async fn test_upload_mismatched_sha256_attempts_exhausted() { let server = start_test_server(None, true, None).await; diff --git a/packages/localsend_isolates/lib/src/isolate/child/server_isolate.dart b/packages/localsend_isolates/lib/src/isolate/child/server_isolate.dart index fc589838..ec497b71 100644 --- a/packages/localsend_isolates/lib/src/isolate/child/server_isolate.dart +++ b/packages/localsend_isolates/lib/src/isolate/child/server_isolate.dart @@ -340,9 +340,15 @@ class _ReceiveSession { /// One queue per file ID, so that uploads of the same file do not overlap. /// /// A sender may upload the same file again after it was rejected because of - /// a checksum mismatch. + /// a checksum mismatch. Both attempts write to the same [targets] entry. final Map uploads = {}; + /// The destination of each file of this session, by file ID. + /// + /// Remembered so that another attempt at the same file overwrites it instead + /// of being saved next to it under a numbered name. + final Map targets = {}; + _ReceiveSession(this.config); } @@ -618,15 +624,21 @@ Future _handleFileUpload({ final FileSaveTarget target; try { - target = await prepareFileSaveTarget( - destinationDirectory: config.destinationDirectory, - cacheDirectory: config.cacheDirectory, - fileName: desiredName, - saveToGallery: shouldSaveToGallery, - isImage: isImage, - createdDirectories: session.createdDirectories, - androidSdkInt: config.androidSdkInt, - ); + // A previous attempt at this file already picked a destination, which this + // attempt overwrites instead of creating a numbered version. + final previous = session.targets[fileId]; + target = previous != null + ? await reopenFileSaveTarget(previous) + : await prepareFileSaveTarget( + destinationDirectory: config.destinationDirectory, + cacheDirectory: config.cacheDirectory, + fileName: desiredName, + saveToGallery: shouldSaveToGallery, + isImage: isImage, + createdDirectories: session.createdDirectories, + androidSdkInt: config.androidSdkInt, + ); + session.targets[fileId] = target; } catch (e, st) { _logger.severe('Failed to prepare save target', e, st); @@ -663,18 +675,13 @@ Future _handleFileUpload({ ); } } catch (e, st) { + // The incomplete file is kept: a retry of this file overwrites it, and + // otherwise it stays behind as the partial file of a failed transfer. _logger.severe('Failed to save file', e, st); - - // Delete the partial (or checksum-mismatched) file so a retried upload - // gets the same file name again instead of a renamed one. - await deleteFileSaveTarget(target); - emitFailed(e); return; } - // The file is fully received and the sender was already told success, - // so failures from here on must not delete the file. try { await applyFileTimestamps( target: target, diff --git a/packages/localsend_isolates/lib/src/task/server/file_saver.dart b/packages/localsend_isolates/lib/src/task/server/file_saver.dart index f306054e..d307b837 100644 --- a/packages/localsend_isolates/lib/src/task/server/file_saver.dart +++ b/packages/localsend_isolates/lib/src/task/server/file_saver.dart @@ -93,30 +93,26 @@ Future prepareFileSaveTarget({ ); } -/// Deletes the file behind [target]. +/// Prepares [target] for another attempt at the same file, e.g. after the +/// previous attempt was rejected because of a checksum mismatch. /// -/// Used when a file could not be received completely, so that a retry of the -/// same file gets the original file name again instead of a numbered one. -/// -/// Failures are logged and swallowed: a leftover file is not worth failing -/// the transfer for. -Future deleteFileSaveTarget(FileSaveTarget target) async { - // SAF targets are written through a file descriptor and must be deleted - // through the Storage Access Framework, addressed by their document URI. - final uri = target.path ?? target.displayPath; - try { - if (uri.startsWith('content://')) { - await android_channel.deleteFileAndroid(uri: uri); - return; - } - - final file = File(uri); - if (await file.exists()) { - await file.delete(); - } - } catch (e) { - _logger.warning('Could not delete file at $uri', e); +/// The destination is kept, so the file is overwritten instead of being +/// created a second time under a numbered name. +Future reopenFileSaveTarget(FileSaveTarget target) async { + final path = target.path; + if (path != null) { + // The server opens (and truncates) the path itself. + return target; } + + // The descriptor of the previous attempt was consumed by it, so the SAF + // document has to be opened again. + _logger.info('Reopening ${target.displayPath}'); + return FileSaveTarget( + path: null, + fileDescriptor: await android_channel.openFileForWritingAndroid(uri: target.displayPath), + displayPath: target.displayPath, + ); } /// Applies the file timestamps after the file has been written to a plain path. diff --git a/packages/localsend_isolates/lib/util/android_channel.dart b/packages/localsend_isolates/lib/util/android_channel.dart index 33d856f5..f3bf701c 100644 --- a/packages/localsend_isolates/lib/util/android_channel.dart +++ b/packages/localsend_isolates/lib/util/android_channel.dart @@ -85,13 +85,17 @@ Future createFileAndroid({ ); } -/// Deletes the document at [uri], e.g. a file created by [createFileAndroid]. +/// Opens an existing document created by [createFileAndroid] for writing and +/// discards its current content. /// -/// Returns whether the document has been deleted. A document that does not -/// exist (anymore) returns `false` instead of throwing. -Future deleteFileAndroid({required String uri}) async { - final deleted = await _methodChannel.invokeMethod('deleteFile', { +/// The descriptor stays open after this call and must be closed by the native +/// consumer it is passed to. +Future openFileForWritingAndroid({required String uri}) async { + final fileDescriptor = await _methodChannel.invokeMethod('openFileForWriting', { 'uri': uri, }); - return deleted ?? false; + if (fileDescriptor == null) { + throw StateError('Android returned no file descriptor for $uri'); + } + return fileDescriptor; }