From d1d294c22c51cc2158eaa27cf2e6bb4206520276 Mon Sep 17 00:00:00 2001 From: Bob Lee Date: Sun, 27 Sep 2026 21:29:37 +0800 Subject: [PATCH 1/3] fix(remote-transfer): keep file transfers running instead of timing out Every remote file command inherited one 30s frontend request deadline, so a slow-but-healthy transfer was reported as "Request timeout" while the host kept transferring. Transfers now stream instead of buffering whole files, stage the destination and publish it only after the bytes completed, and carry one cancellable transfer id from the progress card to the backend so a stop reaches the operation it displays. Co-authored-by: OpenBitFun <318544290+bitfun-ai@users.noreply.github.com> --- .../remote-workspace-transport.md | 21 + src/apps/desktop/src/api/ssh_api.rs | 670 ++++++++++++------ .../src/remote_ssh/manager.rs | 306 +++++++- .../manager/tests/workspace_sftp.rs | 69 ++ .../src/remote_ssh/remote_fs.rs | 113 ++- .../src/app/components/panels/FilesPanel.tsx | 34 +- .../src/features/ssh-remote/sshApi.test.ts | 42 ++ src/web-ui/src/features/ssh-remote/sshApi.ts | 18 +- .../api/service-api/ApiClient.test.ts | 32 + .../api/service-api/ApiClient.ts | 19 +- .../infrastructure/api/service-api/types.ts | 8 + .../src/locales/en-US/panels/files.json | 1 + .../src/locales/zh-CN/panels/files.json | 1 + .../src/locales/zh-TW/panels/files.json | 1 + .../file-system/hooks/useWorkspaceFileDrop.ts | 16 +- .../services/workspaceFileTransfer.test.ts | 126 ++++ .../services/workspaceFileTransfer.ts | 105 ++- 17 files changed, 1330 insertions(+), 252 deletions(-) create mode 100644 src/web-ui/src/features/ssh-remote/sshApi.test.ts diff --git a/docs/architecture/remote-workspace-transport.md b/docs/architecture/remote-workspace-transport.md index 18c368cca9..75e3c6006a 100644 --- a/docs/architecture/remote-workspace-transport.md +++ b/docs/architecture/remote-workspace-transport.md @@ -183,6 +183,27 @@ through the destination to preserve existing links and permissions. The final write is not an atomic transaction against other writers; interruption during that phase can have a partial or unknown outcome. +Both providers publish an upload only after the transfer completed: the SFTP +path streams into a same-directory `-upload-.tmp` sibling +and renames it over the destination, the container command does the same after +its size check, and a downloaded file is staged beside its local destination +the same way. A failed or cancelled transfer therefore leaves any previous +destination unchanged, and staging temporaries are removed when the owning side +can still reach the path. Because a published upload replaces its destination +rather than writing through it, the result takes the staging file's mode and +ownership, and a replaced symlink or hard link is not followed; that is what +distinguishes a transfer from a workspace tool write above. A hard kill between +staging and commit can leave one orphaned temporary, which the container +commands sweep by age and the SFTP path leaves to the user. + +The controller applies no wall-clock deadline to commands whose duration scales +with file size or entry count (file read and write, directory listing, tree, +upload, download). Those operations are bounded where the work happens — every +SFTP request has its own response timeout and the SSH transport drops a stalled +connection — and a transfer ends when it completes, fails, or is stopped. A +short controller deadline only reported a slow-but-healthy transfer as a failure +while the host kept transferring it. + Directory and stat records use NUL-separated fields. File names containing newlines or the delimiters used by older implementations remain round-trippable. The records are decoded only after the full byte stream is assembled; invalid diff --git a/src/apps/desktop/src/api/ssh_api.rs b/src/apps/desktop/src/api/ssh_api.rs index d89012c158..fb48363382 100644 --- a/src/apps/desktop/src/api/ssh_api.rs +++ b/src/apps/desktop/src/api/ssh_api.rs @@ -3,7 +3,10 @@ //! Tauri commands for SSH connection management and remote file operations. use serde::Serialize; +use std::collections::HashMap; +use std::path::{Path, PathBuf}; use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::{Arc, Mutex}; use std::time::Instant; use tauri::{Emitter, State}; @@ -387,6 +390,242 @@ pub async fn remote_rename( .map_err(|e| e.to_string()) } +/// Bytes read or written per transfer step. Matches the chunk the remote file +/// service uses, so one progress step is one protocol round trip. +const TRANSFER_CHUNK_BYTES: usize = 256 * 1024; + +/// Progress events are throttled to one per interval, plus the terminal event. +const TRANSFER_PROGRESS_INTERVAL_MS: u128 = 100; + +/// Cancellation and cooperative abort share one message so a user-initiated +/// stop is recognizable on every transfer path. +const TRANSFER_CANCELLED: &str = "Transfer cancelled"; + +/// Cancellation registry shared with the `cancel_transfer` command. +type TransferRegistry = Arc>>>; + +/// One in-flight transfer's cancellation slot. +/// +/// The entry is removed on every exit path — including `?` early returns and +/// panics — so a failed or rejected transfer cannot leave a stale flag behind +/// that would abort a later transfer reusing the same id. +struct ActiveTransfer { + registry: TransferRegistry, + transfer_id: String, + cancel_flag: Arc, +} + +impl ActiveTransfer { + fn register(registry: &TransferRegistry, transfer_id: &str) -> Result { + let cancel_flag = Arc::new(AtomicBool::new(false)); + registry + .lock() + .map_err(|error| error.to_string())? + .insert(transfer_id.to_string(), cancel_flag.clone()); + Ok(Self { + registry: registry.clone(), + transfer_id: transfer_id.to_string(), + cancel_flag, + }) + } + + fn is_cancelled(&self) -> bool { + self.cancel_flag.load(Ordering::Relaxed) + } +} + +impl Drop for ActiveTransfer { + fn drop(&mut self) { + let Ok(mut registry) = self.registry.lock() else { + return; + }; + // A reused id may already belong to a newer transfer; only this + // transfer's own flag may be removed. + if registry + .get(&self.transfer_id) + .is_some_and(|flag| Arc::ptr_eq(flag, &self.cancel_flag)) + { + registry.remove(&self.transfer_id); + } + } +} + +/// Emit a throttled progress event; `terminal` always emits. +fn emit_transfer_progress( + app_handle: &tauri::AppHandle, + event: &str, + payload: impl Serialize + Clone, + last_emit: &mut Instant, + terminal: bool, +) { + let now = Instant::now(); + if !terminal && now.duration_since(*last_emit).as_millis() < TRANSFER_PROGRESS_INTERVAL_MS { + return; + } + *last_emit = now; + let _ = app_handle.emit(event, payload); +} + +fn emit_download_progress( + app_handle: &tauri::AppHandle, + transfer_id: &str, + downloaded: u64, + total: u64, + last_emit: &mut Instant, + terminal: bool, +) { + emit_transfer_progress( + app_handle, + "download_progress", + DownloadProgressPayload { + transfer_id: transfer_id.to_string(), + downloaded, + total, + }, + last_emit, + terminal, + ); +} + +/// Local staging for a streamed remote download. +/// +/// Bytes land in a private temporary file beside the destination and replace it +/// only after the transfer completed, so a cancelled or failed download never +/// truncates an existing local file. This mirrors the controller-local peer +/// sink in [`crate::api::local_file_download`]. +struct LocalDownloadStaging { + file: tokio::fs::File, + temp_path: tempfile::TempPath, + destination: PathBuf, +} + +impl LocalDownloadStaging { + async fn begin(destination: &Path) -> Result { + let parent = destination + .parent() + .filter(|parent| !parent.as_os_str().is_empty()) + .unwrap_or_else(|| Path::new(".")); + tokio::fs::create_dir_all(parent).await.map_err(|error| { + format!( + "Failed to prepare local download directory '{}': {}", + parent.display(), + error + ) + })?; + let (file, temp_path) = tempfile::Builder::new() + .prefix(".openbitfun-download-") + .tempfile_in(parent) + .map_err(|error| { + format!( + "Failed to stage local download beside '{}': {}", + destination.display(), + error + ) + })? + .into_parts(); + Ok(Self { + file: tokio::fs::File::from_std(file), + temp_path, + destination: destination.to_path_buf(), + }) + } + + async fn write(&mut self, chunk: &[u8]) -> Result<(), String> { + use tokio::io::AsyncWriteExt; + self.file.write_all(chunk).await.map_err(|error| { + format!( + "Failed to write local download '{}': {}", + self.destination.display(), + error + ) + }) + } + + async fn publish(self) -> Result<(), String> { + let Self { + file, + temp_path, + destination, + } = self; + file.sync_all().await.map_err(|error| { + format!( + "Failed to flush local download '{}': {}", + destination.display(), + error + ) + })?; + drop(file); + let failure_path = destination.clone(); + tokio::task::spawn_blocking(move || temp_path.persist(&destination)) + .await + .map_err(|error| error.to_string())? + .map_err(|error| { + format!( + "Failed to publish local download '{}': {}", + failure_path.display(), + error + ) + }) + } +} + +/// Stream one remote file into a staged local file and publish it. +/// +/// `on_progress` receives the bytes read for this file after every chunk; the +/// caller owns throttling and the cumulative total it reports to the frontend. +/// The remote reader bounds every protocol request, so a cancelled transfer +/// stops at the next chunk boundary instead of waiting for the whole file. +async fn download_remote_file_to_local( + remote_fs: &openbitfun_core::service::remote_ssh::RemoteFileService, + connection_id: &str, + remote_path: &str, + destination: &Path, + transfer: &ActiveTransfer, + on_progress: &mut impl FnMut(u64), +) -> Result { + let mut reader = remote_fs + .open_read(connection_id, remote_path) + .await + .map_err(|error| format!("Failed to open remote file '{remote_path}': {error}"))?; + let mut staging = LocalDownloadStaging::begin(destination).await?; + let mut buffer = vec![0_u8; TRANSFER_CHUNK_BYTES]; + let mut downloaded = 0_u64; + loop { + if transfer.is_cancelled() { + return Err(TRANSFER_CANCELLED.to_string()); + } + use tokio::io::AsyncReadExt; + let read = reader + .read(&mut buffer) + .await + .map_err(|error| format!("Failed to read remote file '{remote_path}': {error}"))?; + if read == 0 { + break; + } + staging.write(&buffer[..read]).await?; + downloaded = downloaded.saturating_add(read as u64); + on_progress(downloaded); + } + staging.publish().await?; + Ok(downloaded) +} + +/// Remote file size used only to keep the reported percentage sane; the actual +/// byte count always comes from the stream. +async fn remote_declared_size( + remote_fs: &openbitfun_core::service::remote_ssh::RemoteFileService, + connection_id: &str, + remote_path: &str, +) -> u64 { + remote_fs + .workspace_metadata(connection_id, remote_path, true) + .await + .ok() + .flatten() + .and_then(|metadata| metadata.size) + .unwrap_or(0) +} + /// Payload emitted via `download_progress` events during a remote download. #[derive(Debug, Clone, Serialize)] #[serde(rename_all = "camelCase")] @@ -396,19 +635,20 @@ pub struct DownloadProgressPayload { pub total: u64, } -/// Read a remote file or directory via SFTP and write it to a local path. +/// Read a remote file or directory and stream it to a local path. /// -/// If `remote_path` is a file, its bytes are read via SFTP and written to -/// `local_path` (binary-safe). If it is a directory, the directory tree is -/// recreated locally: subdirectories are created with `create_dir_all`, and -/// each file is read via SFTP and written to disk. +/// If `remote_path` is a file, its bytes are streamed to `local_path` +/// (binary-safe) through a staged temporary file that replaces the destination +/// only after the transfer completed. If it is a directory, the directory tree +/// is recreated locally: subdirectories are created with `create_dir_all`, and +/// each file is streamed the same way. /// /// Emits `download_progress` events with `{ transferId, downloaded, total }` -/// (bytes) during the SFTP read so the frontend can render a determinate -/// progress bar with speed display. The `transfer_id` lets the frontend -/// distinguish concurrent downloads and cancel individual transfers. Events -/// are throttled to at most one per 100 ms (plus a guaranteed final event) to -/// avoid flooding the webview. +/// (bytes) during the read so the frontend can render a determinate progress +/// bar with speed display. The `transfer_id` lets the frontend distinguish +/// concurrent downloads and cancel individual transfers. Events are throttled +/// to at most one per 100 ms (plus a guaranteed final event) to avoid flooding +/// the webview. #[tauri::command] pub async fn remote_download_to_local_path( app_handle: tauri::AppHandle, @@ -419,13 +659,8 @@ pub async fn remote_download_to_local_path( transfer_id: String, ) -> Result<(), String> { let remote_fs = state.get_remote_file_service_async().await?; - - // Register a cancellation flag for this transfer. - let cancel_flag = std::sync::Arc::new(AtomicBool::new(false)); - { - let mut map = state.active_transfers.lock().map_err(|e| e.to_string())?; - map.insert(transfer_id.clone(), cancel_flag.clone()); - } + let transfer = ActiveTransfer::register(&state.active_transfers, &transfer_id)?; + let mut last_emit = Instant::now(); // Check if the remote path is a directory. let is_dir = remote_fs @@ -434,65 +669,48 @@ pub async fn remote_download_to_local_path( .map_err(|e| e.to_string())?; if is_dir { - let dir_result = download_directory_from_remote( + return download_directory_from_remote( &app_handle, &state, &connection_id, &remote_path, &local_path, &transfer_id, - &cancel_flag, + &transfer, + &mut last_emit, ) .await; - // Clean up the cancellation flag. - { - let mut map = state.active_transfers.lock().map_err(|e| e.to_string())?; - map.remove(&transfer_id); - } - return dir_result; - } - - // Regular file: read via SFTP with progress. - let mut last_emit = Instant::now(); - let result = remote_fs - .read_file_with_progress(&connection_id, &remote_path, &mut |downloaded, total| { - // Throttle: emit at most every 100 ms, plus the final 100% event. - let now = Instant::now(); - if downloaded >= total || now.duration_since(last_emit).as_millis() >= 100 { - let _ = app_handle.emit( - "download_progress", - DownloadProgressPayload { - transfer_id: transfer_id.clone(), - downloaded, - total, - }, - ); - last_emit = now; - } - // Return false to abort the read if cancelled. - !cancel_flag.load(Ordering::Relaxed) - }) - .await; - - // Clean up the cancellation flag. - { - let mut map = state.active_transfers.lock().map_err(|e| e.to_string())?; - map.remove(&transfer_id); } - let bytes = result.map_err(|e| e.to_string())?; - - tokio::task::spawn_blocking(move || { - let path = std::path::Path::new(&local_path); - if let Some(parent) = path.parent() { - if !parent.as_os_str().is_empty() { - std::fs::create_dir_all(parent).map_err(|e| e.to_string())?; - } - } - std::fs::write(path, &bytes).map_err(|e| e.to_string()) - }) - .await - .map_err(|e| e.to_string())? + let declared_total = remote_declared_size(&remote_fs, &connection_id, &remote_path).await; + let downloaded = download_remote_file_to_local( + &remote_fs, + &connection_id, + &remote_path, + Path::new(&local_path), + &transfer, + &mut |downloaded| { + // A file that grew while it was being read must not report over 100%. + emit_download_progress( + &app_handle, + &transfer_id, + downloaded, + declared_total.max(downloaded), + &mut last_emit, + false, + ); + }, + ) + .await?; + emit_download_progress( + &app_handle, + &transfer_id, + downloaded, + declared_total.max(downloaded), + &mut last_emit, + true, + ); + Ok(()) } fn validate_remote_name_for_local_download(name: &str) -> Result<(), String> { @@ -568,9 +786,10 @@ fn local_download_name_key(name: &str) -> String { /// Recursively download a remote directory to a local path. /// /// Pre-scans the remote tree to determine total file size, then walks the tree -/// and downloads each file with chunked progress reporting. Emits cumulative +/// and streams each file through a staged local file. Emits cumulative /// `download_progress` events so the frontend can show overall directory /// download progress. +#[allow(clippy::too_many_arguments)] async fn download_directory_from_remote( app_handle: &tauri::AppHandle, state: &State<'_, AppState>, @@ -578,12 +797,13 @@ async fn download_directory_from_remote( remote_dir: &str, local_dir: &str, transfer_id: &str, - cancel_flag: &std::sync::Arc, + transfer: &ActiveTransfer, + last_emit: &mut Instant, ) -> Result<(), String> { let remote_fs = state.get_remote_file_service_async().await?; // Create the top-level local directory. - let local_dir_path = std::path::PathBuf::from(local_dir); + let local_dir_path = PathBuf::from(local_dir); tokio::task::spawn_blocking(move || { std::fs::create_dir_all(&local_dir_path).map_err(|e| e.to_string()) }) @@ -615,15 +835,14 @@ async fn download_directory_from_remote( } let mut downloaded: u64 = 0; - let mut last_emit = Instant::now(); // Walk the remote directory tree. - let mut stack: Vec<(String, std::path::PathBuf)> = - vec![(remote_dir.to_string(), std::path::PathBuf::from(local_dir))]; + let mut stack: Vec<(String, PathBuf)> = + vec![(remote_dir.to_string(), PathBuf::from(local_dir))]; while let Some((remote_current, local_current)) = stack.pop() { - if cancel_flag.load(Ordering::Relaxed) { - return Err("Transfer cancelled".to_string()); + if transfer.is_cancelled() { + return Err(TRANSFER_CANCELLED.to_string()); } let entries = remote_fs @@ -659,56 +878,39 @@ async fn download_directory_from_remote( stack.push((remote_child, local_child)); } else { let base_downloaded = downloaded; - let bytes = remote_fs - .read_file_with_progress(connection_id, &remote_child, &mut |read_bytes, _| { - let cumulative = base_downloaded + read_bytes; - let now = Instant::now(); - if cumulative >= total_bytes - || now.duration_since(last_emit).as_millis() >= 100 - { - let _ = app_handle.emit( - "download_progress", - DownloadProgressPayload { - transfer_id: transfer_id.to_string(), - downloaded: cumulative, - total: total_bytes, - }, - ); - last_emit = now; - } - !cancel_flag.load(Ordering::Relaxed) - }) - .await - .map_err(|e| e.to_string())?; - - let file_size = bytes.len() as u64; - let local_child_write = local_child.clone(); - tokio::task::spawn_blocking(move || { - if let Some(parent) = local_child_write.parent() { - if !parent.as_os_str().is_empty() { - std::fs::create_dir_all(parent).map_err(|e| e.to_string())?; - } - } - std::fs::write(&local_child_write, &bytes).map_err(|e| e.to_string()) - }) - .await - .map_err(|e| e.to_string())??; - - downloaded += file_size; + let file_size = download_remote_file_to_local( + &remote_fs, + connection_id, + &remote_child, + &local_child, + transfer, + &mut |file_bytes| { + let cumulative = base_downloaded.saturating_add(file_bytes); + emit_download_progress( + app_handle, + transfer_id, + cumulative, + total_bytes.max(cumulative), + last_emit, + false, + ); + }, + ) + .await?; + + downloaded = base_downloaded.saturating_add(file_size); + emit_download_progress( + app_handle, + transfer_id, + downloaded, + total_bytes.max(downloaded), + last_emit, + true, + ); } } } - // Final progress event. - let _ = app_handle.emit( - "download_progress", - DownloadProgressPayload { - transfer_id: transfer_id.to_string(), - downloaded: total_bytes, - total: total_bytes, - }, - ); - Ok(()) } @@ -728,6 +930,27 @@ pub struct UploadProgressPayload { pub total: u64, } +fn emit_upload_progress( + app_handle: &tauri::AppHandle, + transfer_id: &str, + uploaded: u64, + total: u64, + last_emit: &mut Instant, + terminal: bool, +) { + emit_transfer_progress( + app_handle, + "upload_progress", + UploadProgressPayload { + transfer_id: transfer_id.to_string(), + uploaded, + total, + }, + last_emit, + terminal, + ); +} + /// Recursively scan a local directory and return the total size of all /// regular files in bytes. Used for pre-scanning before directory upload /// so that overall progress can be reported. @@ -768,18 +991,22 @@ fn scan_directory_total_size(dir: &std::path::Path) -> Result { Ok(total) } -/// Upload a local file or directory tree to a remote path via SFTP. +/// Upload a local file or directory tree to a remote path. +/// +/// If `local_path` is a file, its bytes are streamed to `remote_path` without +/// buffering the file in memory. If it is a directory, the directory tree is +/// recreated on the remote side: subdirectories are created with +/// `create_dir_all`, and each file is streamed the same way. /// -/// If `local_path` is a file, its bytes are written to `remote_path`. If it is -/// a directory, the directory tree is recreated on the remote side: -/// subdirectories are created with `create_dir_all`, and each file is read -/// locally and written via SFTP. +/// An upload stages its bytes on the remote side and publishes them only after +/// the transfer completed, so a cancelled or failed upload never truncates a +/// valid destination with partial content. /// /// Emits `upload_progress` events with `{ transferId, uploaded, total }` -/// (bytes) during the SFTP write so the frontend can render a determinate -/// progress bar with speed display. The `transfer_id` lets the frontend -/// distinguish concurrent uploads and cancel individual transfers. Events -/// are throttled to at most one per 100 ms (plus a guaranteed final event). +/// (bytes) during the write so the frontend can render a determinate progress +/// bar with speed display. The `transfer_id` lets the frontend distinguish +/// concurrent uploads and cancel individual transfers. Events are throttled to +/// at most one per 100 ms (plus a guaranteed final event). #[tauri::command] pub async fn remote_upload_from_local_path( app_handle: tauri::AppHandle, @@ -811,75 +1038,47 @@ pub async fn remote_upload_from_local_path( } // Register a cancellation flag for this transfer. - let cancel_flag = std::sync::Arc::new(AtomicBool::new(false)); - { - let mut map = state.active_transfers.lock().map_err(|e| e.to_string())?; - map.insert(transfer_id.clone(), cancel_flag.clone()); - } + let transfer = ActiveTransfer::register(&state.active_transfers, &transfer_id)?; // A directory needs to be walked locally and recreated on the remote side. if local_path.is_dir() { - let dir_result = upload_directory_to_remote( + upload_directory_to_remote( &app_handle, &state, &connection_id, local_path, &remote_path, &transfer_id, - &cancel_flag, + &transfer, ) - .await; - // Clean up the cancellation flag. - { - let mut map = state.active_transfers.lock().map_err(|e| e.to_string())?; - map.remove(&transfer_id); - } - dir_result?; + .await?; return Ok(RemoteUploadResult { was_directory: true, }); } - // Regular file: read locally, write via SFTP with progress. - let local_path_owned = local_path.to_path_buf(); - let bytes = tokio::task::spawn_blocking(move || { - std::fs::read(&local_path_owned).map_err(|e| e.to_string()) - }) - .await - .map_err(|e| e.to_string())??; - + // Regular file: stream it to the remote without buffering it in memory. let remote_fs = state.get_remote_file_service_async().await?; let mut last_emit = Instant::now(); - let write_result = remote_fs - .write_file_with_progress( + remote_fs + .write_file_from_local_path_with_progress( &connection_id, &remote_path, - &bytes, + local_path, &mut |written, total| { - let now = Instant::now(); - if written >= total || now.duration_since(last_emit).as_millis() >= 100 { - let _ = app_handle.emit( - "upload_progress", - UploadProgressPayload { - transfer_id: transfer_id.clone(), - uploaded: written, - total, - }, - ); - last_emit = now; - } - !cancel_flag.load(Ordering::Relaxed) + emit_upload_progress( + &app_handle, + &transfer_id, + written, + total, + &mut last_emit, + written >= total, + ); + !transfer.is_cancelled() }, ) - .await; - - // Clean up the cancellation flag. - { - let mut map = state.active_transfers.lock().map_err(|e| e.to_string())?; - map.remove(&transfer_id); - } - - write_result.map_err(|e| e.to_string())?; + .await + .map_err(|e| e.to_string())?; Ok(RemoteUploadResult { was_directory: false, @@ -899,7 +1098,7 @@ async fn upload_directory_to_remote( local_dir: &std::path::Path, remote_dir: &str, transfer_id: &str, - cancel_flag: &std::sync::Arc, + transfer: &ActiveTransfer, ) -> Result<(), String> { let remote_fs = state.get_remote_file_service_async().await?; @@ -936,8 +1135,8 @@ async fn upload_directory_to_remote( .map_err(|e| e.to_string())??; for entry in entries { - if cancel_flag.load(Ordering::Relaxed) { - return Err("Transfer cancelled".to_string()); + if transfer.is_cancelled() { + return Err(TRANSFER_CANCELLED.to_string()); } let entry_path = entry.path(); let file_name = entry.file_name().into_string().map_err(|name| { @@ -966,42 +1165,37 @@ async fn upload_directory_to_remote( .map_err(|e| e.to_string())?; stack.push((entry_path, remote_child)); } else if file_type.is_file() { - let local_file = entry_path.clone(); - let bytes = tokio::task::spawn_blocking(move || { - std::fs::read(&local_file).map_err(|e| e.to_string()) - }) - .await - .map_err(|e| e.to_string())??; - - let file_size = bytes.len() as u64; let base_uploaded = uploaded; - remote_fs - .write_file_with_progress( + let file_size = remote_fs + .write_file_from_local_path_with_progress( connection_id, &remote_child, - &bytes, + &entry_path, &mut |written, _| { - let cumulative = base_uploaded + written; - let now = Instant::now(); - if cumulative >= total_bytes - || now.duration_since(last_emit).as_millis() >= 100 - { - let _ = app_handle.emit( - "upload_progress", - UploadProgressPayload { - transfer_id: transfer_id.to_string(), - uploaded: cumulative, - total: total_bytes, - }, - ); - last_emit = now; - } - !cancel_flag.load(Ordering::Relaxed) + let cumulative = base_uploaded.saturating_add(written); + emit_upload_progress( + app_handle, + transfer_id, + cumulative, + total_bytes.max(cumulative), + &mut last_emit, + false, + ); + !transfer.is_cancelled() }, ) .await .map_err(|e| e.to_string())?; - uploaded += file_size; + + uploaded = base_uploaded.saturating_add(file_size); + emit_upload_progress( + app_handle, + transfer_id, + uploaded, + total_bytes.max(uploaded), + &mut last_emit, + true, + ); } else { return Err(format!( "Unsupported local entry type in directory upload: '{}'", @@ -1211,7 +1405,69 @@ pub async fn ssh_list_remote_listening_ports( mod tests { use super::{ hydrate_stored_password, local_download_name_key, validate_remote_name_for_local_download, + ActiveTransfer, LocalDownloadStaging, }; + use std::sync::atomic::Ordering; + use std::sync::{Arc, Mutex}; + + #[tokio::test] + async fn staged_local_download_publishes_only_after_it_completed() { + let directory = tempfile::tempdir().unwrap(); + let destination = directory.path().join("existing.txt"); + tokio::fs::write(&destination, b"original").await.unwrap(); + + // A staging that never publishes keeps the destination and leaves no + // temporary behind, so a cancelled download cannot corrupt local data. + { + let mut staging = LocalDownloadStaging::begin(&destination).await.unwrap(); + staging.write(b"partial").await.unwrap(); + } + assert_eq!( + tokio::fs::read(&destination).await.unwrap(), + b"original".to_vec() + ); + assert_eq!(std::fs::read_dir(directory.path()).unwrap().count(), 1); + + let mut staging = LocalDownloadStaging::begin(&destination).await.unwrap(); + staging.write(b"complete").await.unwrap(); + staging.publish().await.unwrap(); + assert_eq!( + tokio::fs::read(&destination).await.unwrap(), + b"complete".to_vec() + ); + assert_eq!(std::fs::read_dir(directory.path()).unwrap().count(), 1); + } + + #[test] + fn active_transfer_registration_is_removed_on_every_exit_path() { + let registry: super::TransferRegistry = Arc::new(Mutex::new(Default::default())); + + { + let transfer = ActiveTransfer::register(®istry, "transfer-1").unwrap(); + let map = registry.lock().unwrap(); + assert!(map.contains_key("transfer-1")); + drop(map); + drop(transfer); + } + // `?` early returns and panics drop the guard the same way. + assert!(registry.lock().unwrap().is_empty()); + + let transfer = ActiveTransfer::register(®istry, "transfer-2").unwrap(); + assert!(!transfer.is_cancelled()); + let flag = { + let map = registry.lock().unwrap(); + map.get("transfer-2").cloned().unwrap() + }; + flag.store(true, Ordering::Relaxed); + assert!(transfer.is_cancelled()); + + // A newer transfer reusing the id keeps its own flag when an older + // guard is dropped afterwards. + let newer = ActiveTransfer::register(®istry, "transfer-2").unwrap(); + drop(transfer); + assert!(!newer.is_cancelled()); + assert!(registry.lock().unwrap().contains_key("transfer-2")); + } #[test] fn download_names_cannot_escape_the_selected_local_directory() { diff --git a/src/crates/services/services-integrations/src/remote_ssh/manager.rs b/src/crates/services/services-integrations/src/remote_ssh/manager.rs index 8d720a7946..9d08666670 100644 --- a/src/crates/services/services-integrations/src/remote_ssh/manager.rs +++ b/src/crates/services/services-integrations/src/remote_ssh/manager.rs @@ -269,6 +269,27 @@ impl ContainerFileReader { } } +/// Byte source for one streamed container upload. +/// +/// Both arms feed the same staging command; only the local reader differs, so a +/// local file never has to be buffered in memory first. +enum ContainerUploadSource<'a> { + Memory(&'a [u8]), + LocalFile(std::path::PathBuf), +} + +/// Stop a container upload process and wait for its target-aware cleanup while +/// preserving the caller's error as the operation result. +async fn abort_container_upload( + control: &crate::remote_ssh::WorkspaceProcessControl, + completion: crate::remote_ssh::WorkspaceProcessCompletion, + error: anyhow::Error, +) -> anyhow::Error { + let _ = control.kill().await; + let _ = completion.wait().await; + error +} + impl AsyncRead for ContainerFileReader { fn poll_read( self: std::pin::Pin<&mut Self>, @@ -715,6 +736,22 @@ impl std::error::Error for StagedError { /// send, short enough that a chain of dead hops cannot stall shutdown. const SSH_DISCONNECT_TIMEOUT: Duration = Duration::from_secs(1); +/// Per-request SFTP response timeout. +/// +/// russh-sftp defaults to 10 s, which the 256 KiB chunk this client sends turns +/// into a ~25 KiB/s throughput floor: on a healthy but slow link every request +/// would time out instead of transferring slowly. A single stalled request is +/// still bounded here, and the SSH transport independently drops a connection +/// that stops answering. +const SFTP_REQUEST_TIMEOUT_SECS: u64 = 60; + +fn sftp_client_config() -> russh_sftp::client::Config { + russh_sftp::client::Config { + request_timeout_secs: SFTP_REQUEST_TIMEOUT_SECS, + ..Default::default() + } +} + fn tag_failed_stage(stage: &str, error: anyhow::Error) -> anyhow::Error { anyhow::Error::new(StagedError { stage: stage.to_string(), @@ -5342,6 +5379,64 @@ impl SSHConnectionManager { path: &str, content: &[u8], on_progress: &mut impl FnMut(u64, u64) -> bool, + ) -> anyhow::Result<()> { + self.container_stream_write( + connection_id, + path, + content.len() as u64, + ContainerUploadSource::Memory(content), + on_progress, + ) + .await + } + + /// Stream one local regular file into the container upload command without + /// buffering it in memory. + /// + /// The destination is replaced only after the staged bytes passed the + /// command's size check, so a cancelled or failed upload leaves the previous + /// destination untouched. + pub async fn container_write_file_from_path_with_progress( + &self, + connection_id: &str, + path: &str, + local_path: &std::path::Path, + on_progress: &mut impl FnMut(u64, u64) -> bool, + ) -> anyhow::Result { + let local_metadata = tokio::fs::symlink_metadata(local_path) + .await + .map_err(|error| { + anyhow!( + "Failed to inspect local upload file '{}': {}", + local_path.display(), + error + ) + })?; + if local_metadata.file_type().is_symlink() || !local_metadata.is_file() { + return Err(anyhow!( + "Local upload source is not a regular file: {}", + local_path.display() + )); + } + let expected_size = local_metadata.len(); + self.container_stream_write( + connection_id, + path, + expected_size, + ContainerUploadSource::LocalFile(local_path.to_path_buf()), + on_progress, + ) + .await?; + Ok(expected_size) + } + + async fn container_stream_write( + &self, + connection_id: &str, + path: &str, + expected_size: u64, + source: ContainerUploadSource<'_>, + on_progress: &mut impl FnMut(u64, u64) -> bool, ) -> anyhow::Result<()> { use tokio::io::{AsyncReadExt, AsyncWriteExt}; @@ -5359,7 +5454,6 @@ impl SSHConnectionManager { let quoted_temporary = crate::remote_ssh::shell::quote_arg(&temporary); let quoted_path = crate::remote_ssh::shell::quote_arg(&path); let quoted_parent = crate::remote_ssh::shell::quote_arg(parent); - let expected_size = content.len(); let sweep = stale_upload_sweep("ed_parent); let command = format!( "{sweep} \ @@ -5388,25 +5482,109 @@ impl SSHConnectionManager { let _ = stderr.read_to_end(&mut bytes).await; bytes }); - let total = content.len() as u64; + let total = expected_size; if !on_progress(0, total) { - let _ = control.kill().await; - let _ = completion.wait().await; - anyhow::bail!("Transfer cancelled"); + return Err(abort_container_upload( + &control, + completion, + anyhow!("Transfer cancelled"), + ) + .await); } let mut written = 0u64; - for chunk in content.chunks(256 * 1024) { - if let Err(error) = stdin.write_all(chunk).await { - let _ = control.kill().await; - let _ = completion.wait().await; - anyhow::bail!("Failed to stream container file '{}': {}", path, error); + match source { + ContainerUploadSource::Memory(content) => { + for chunk in content.chunks(256 * 1024) { + if let Err(error) = stdin.write_all(chunk).await { + return Err(abort_container_upload( + &control, + completion, + anyhow!("Failed to stream container file '{}': {}", path, error), + ) + .await); + } + written += chunk.len() as u64; + if !on_progress(written, total) { + return Err(abort_container_upload( + &control, + completion, + anyhow!("Transfer cancelled"), + ) + .await); + } + } } - written += chunk.len() as u64; - if !on_progress(written, total) { - let _ = control.kill().await; - let _ = completion.wait().await; - anyhow::bail!("Transfer cancelled"); + ContainerUploadSource::LocalFile(local_path) => { + let mut local = match tokio::fs::File::open(&local_path).await { + Ok(local) => local, + Err(error) => { + return Err(abort_container_upload( + &control, + completion, + anyhow!( + "Failed to open local upload file '{}': {}", + local_path.display(), + error + ), + ) + .await); + } + }; + let mut buffer = vec![0u8; 256 * 1024]; + loop { + let read = match local.read(&mut buffer).await { + Ok(read) => read, + Err(error) => { + return Err(abort_container_upload( + &control, + completion, + anyhow!( + "Failed to read local upload file '{}': {}", + local_path.display(), + error + ), + ) + .await); + } + }; + if read == 0 { + break; + } + if let Err(error) = stdin.write_all(&buffer[..read]).await { + return Err(abort_container_upload( + &control, + completion, + anyhow!("Failed to stream container file '{}': {}", path, error), + ) + .await); + } + written = written.saturating_add(read as u64); + if written > expected_size { + return Err(abort_container_upload( + &control, + completion, + anyhow!("Local upload file changed while it was being sent"), + ) + .await); + } + if !on_progress(written, total) { + return Err(abort_container_upload( + &control, + completion, + anyhow!("Transfer cancelled"), + ) + .await); + } + } + if written != expected_size { + return Err(abort_container_upload( + &control, + completion, + anyhow!("Local upload file changed while it was being sent"), + ) + .await); + } } } stdin @@ -5768,7 +5946,7 @@ impl SSHConnectionManager { .await .map_err(|e| anyhow!("Failed to request SFTP subsystem: {}", e))?; - let sftp = SftpSession::new(channel.into_stream()) + let sftp = SftpSession::new_with_config(channel.into_stream(), sftp_client_config()) .await .map_err(|e| anyhow!("Failed to create SFTP session: {}", e))?; @@ -5838,7 +6016,7 @@ impl SSHConnectionManager { .request_subsystem(true, "sftp") .await .map_err(|error| anyhow!("Failed to request SFTP subsystem: {}", error))?; - let session = RawSftpSession::new(channel.into_stream()); + let session = RawSftpSession::new_with_config(channel.into_stream(), sftp_client_config()); session .init() .await @@ -5973,6 +6151,34 @@ impl SSHConnectionManager { path: &str, local_path: &std::path::Path, max_bytes: u64, + ) -> anyhow::Result { + self.sftp_write_from_file_with_progress( + connection_id, + path, + local_path, + max_bytes, + &mut |_, _| true, + ) + .await + } + + /// Stream one local regular file to a remote SFTP path without buffering + /// the complete file in memory, reporting `(bytes_written, total_bytes)` + /// after every chunk. Returning `false` from `on_progress` stops the + /// transfer at the next chunk boundary. + /// + /// The destination is created before the first chunk, so a cancelled or + /// failed transfer can leave partial bytes behind. Callers that must keep + /// the previous destination intact stage the upload and publish it only + /// after this returns successfully; see + /// `RemoteFileService::write_file_from_local_path_with_progress`. + pub async fn sftp_write_from_file_with_progress( + &self, + connection_id: &str, + path: &str, + local_path: &std::path::Path, + max_bytes: u64, + on_progress: &mut impl FnMut(u64, u64) -> bool, ) -> anyhow::Result { let local_metadata = tokio::fs::symlink_metadata(local_path) .await @@ -6012,6 +6218,7 @@ impl SSHConnectionManager { use tokio::io::{AsyncReadExt, AsyncWriteExt}; let mut buffer = vec![0_u8; 256 * 1024]; let mut written = 0_u64; + let total = local_metadata.len(); loop { let read = local.read(&mut buffer).await.map_err(|error| { anyhow!( @@ -6031,6 +6238,9 @@ impl SSHConnectionManager { .write_all(&buffer[..read]) .await .map_err(|error| anyhow!("Failed to write remote file '{}': {}", path, error))?; + if !on_progress(written, total) { + return Err(anyhow!("Transfer cancelled")); + } } if written != local_metadata.len() { return Err(anyhow!("Local upload file changed while it was being sent")); @@ -6573,6 +6783,17 @@ mod tests { mod workspace_sftp; + #[test] + fn sftp_requests_tolerate_a_slow_but_healthy_link() { + let crate_default = russh_sftp::client::Config::default().request_timeout_secs; + let configured = sftp_client_config().request_timeout_secs; + assert_eq!(configured, SFTP_REQUEST_TIMEOUT_SECS); + assert!( + configured > crate_default, + "the {crate_default}s dependency default is a throughput floor for one 256 KiB chunk" + ); + } + struct UnpublishedSessionTestServer { opens: usize, delayed_open: Option, @@ -7694,6 +7915,57 @@ mod tests { assert!(!staged.exists()); } + #[cfg(unix)] + #[tokio::test] + async fn workspace_container_streams_a_local_file_and_keeps_the_destination_on_cancel() { + let temp = tempfile::tempdir().unwrap(); + let (manager, _provider) = shell_workspace_provider_fixture(temp.path()).await; + let source = temp.path().join("source file.bin"); + let target = temp.path().join("target.bin"); + let payload = vec![7_u8; 700_000]; + std::fs::write(&source, &payload).unwrap(); + std::fs::write(&target, b"preserved").unwrap(); + + let mut progress = Vec::new(); + let written = manager + .container_write_file_from_path_with_progress( + "workspace-shell-fixture", + target.to_str().unwrap(), + &source, + &mut |written, total| { + progress.push((written, total)); + true + }, + ) + .await + .unwrap(); + assert_eq!(written, payload.len() as u64); + assert_eq!( + std::fs::read(&target).unwrap(), + payload, + "a completed stream must replace the destination with the local file" + ); + assert_eq!(progress.first(), Some(&(0, payload.len() as u64))); + assert_eq!( + progress.last(), + Some(&(payload.len() as u64, payload.len() as u64)) + ); + assert!(progress.windows(2).all(|pair| pair[0].0 <= pair[1].0)); + + // The staging command publishes through a rename, so a cancelled + // stream must leave the previously committed destination untouched. + let cancelled = manager + .container_write_file_from_path_with_progress( + "workspace-shell-fixture", + target.to_str().unwrap(), + &source, + &mut |written, _| written < 262_144, + ) + .await; + assert!(cancelled.is_err()); + assert_eq!(std::fs::read(&target).unwrap(), payload); + } + #[cfg(unix)] #[tokio::test] async fn workspace_container_write_stops_after_signal_between_staging_and_commit() { diff --git a/src/crates/services/services-integrations/src/remote_ssh/manager/tests/workspace_sftp.rs b/src/crates/services/services-integrations/src/remote_ssh/manager/tests/workspace_sftp.rs index 9297092ccb..8021a04f49 100644 --- a/src/crates/services/services-integrations/src/remote_ssh/manager/tests/workspace_sftp.rs +++ b/src/crates/services/services-integrations/src/remote_ssh/manager/tests/workspace_sftp.rs @@ -400,6 +400,75 @@ async fn dependency_drop_reproduces_client_limit_with_zero_server_handles() { )); } +#[tokio::test] +async fn streamed_upload_reports_progress_and_stops_at_a_chunk_boundary() { + let f = Fixture::new().await; + let local = f._dir.path().join("upload"); + let bytes: Vec = (0..600_000).map(|index| (index % 251) as u8).collect(); + tokio::fs::write(&local, &bytes).await.unwrap(); + + let mut progress = Vec::new(); + let written = f + .manager + .sftp_write_from_file_with_progress( + "sftp-test", + "/file", + &local, + u64::MAX, + &mut |written, total| { + progress.push((written, total)); + true + }, + ) + .await + .unwrap(); + assert_eq!(written, bytes.len() as u64); + assert_eq!( + f.manager.sftp_read("sftp-test", "/file").await.unwrap(), + bytes + ); + assert_eq!( + progress.last(), + Some(&(bytes.len() as u64, bytes.len() as u64)) + ); + assert!(progress.windows(2).all(|pair| pair[0].0 <= pair[1].0)); + + let cancelled = f + .manager + .sftp_write_from_file_with_progress( + "sftp-test", + "/file", + &local, + u64::MAX, + &mut |written, _| written < 262_144, + ) + .await; + assert!(cancelled.is_err()); + assert!( + f.manager + .sftp_read("sftp-test", "/file") + .await + .unwrap() + .len() + < bytes.len(), + "a cancelled stream must stop before the whole file reaches the remote" + ); + assert_eq!(f.state.live.load(Ordering::SeqCst), 0); + + // The cancelled stream must not poison the next upload on the same channel. + assert_eq!( + f.manager + .sftp_write_from_file("sftp-test", "/file", &local, u64::MAX) + .await + .unwrap(), + bytes.len() as u64 + ); + assert_eq!( + f.manager.sftp_read("sftp-test", "/file").await.unwrap(), + bytes + ); +} + #[tokio::test] async fn repeated_manager_transfers_release_handles_and_preserve_bytes() { let f = Fixture::new().await; diff --git a/src/crates/services/services-integrations/src/remote_ssh/remote_fs.rs b/src/crates/services/services-integrations/src/remote_ssh/remote_fs.rs index 021c1b52f5..9a2be35cfd 100644 --- a/src/crates/services/services-integrations/src/remote_ssh/remote_fs.rs +++ b/src/crates/services/services-integrations/src/remote_ssh/remote_fs.rs @@ -4,6 +4,7 @@ use crate::remote_ssh::types::{RemoteDirEntry, RemoteFileEntry, RemoteTreeNode}; use anyhow::anyhow; +use openbitfun_services_core::product_identity::hidden_data_directory; use std::sync::Arc; /// Names skipped when listing workspace root for system-prompt preview (still lazy: no descent). @@ -41,6 +42,38 @@ fn remote_posix_basename(path: &str) -> String { .to_string() } +/// Sibling staging path used to publish an upload without truncating the +/// current destination. +/// +/// Remote paths are POSIX on every client platform, so the parent directory is +/// split without host `std::path` semantics. +fn staged_upload_path(destination: &str, token: &str) -> String { + let name = format!("{}-upload-{}.tmp", hidden_data_directory(), token); + match destination.rsplit_once('/') { + None => name, + Some(("", _)) => format!("/{name}"), + Some((parent, _)) => format!("{parent}/{name}"), + } +} + +/// Best-effort removal of a staged upload that never replaced its destination. +/// +/// The transfer already failed, so a failed cleanup only leaves one orphaned +/// temporary beside the destination and must not replace the real error. +async fn discard_staged_upload( + manager: &crate::remote_ssh::manager::SSHConnectionManager, + connection_id: &str, + staging: &str, +) { + if let Err(error) = manager.sftp_remove(connection_id, staging).await { + log::warn!( + "Failed to remove staged remote upload '{}': {}", + staging, + error + ); + } +} + /// Remote file service using SFTP protocol #[derive(Clone)] pub struct RemoteFileService { @@ -246,6 +279,60 @@ impl RemoteFileService { .await } + /// Stream a local file to a remote upload destination. + /// + /// The bytes land in a sibling temporary file and replace the destination + /// only after the transfer completed, so a cancelled or failed upload never + /// truncates a valid destination with partial content. The temporary name + /// follows the same shape as the container upload staging, so both + /// providers leave identically named temporaries beside the destination. + /// + /// Container workspaces run their own staging command, which verifies the + /// received size and publishes the destination itself, so they stream + /// straight into that command instead of through SFTP. + pub async fn write_file_from_local_path_with_progress( + &self, + connection_id: &str, + path: &str, + local_path: &std::path::Path, + on_progress: &mut impl FnMut(u64, u64) -> bool, + ) -> anyhow::Result { + let manager = self.get_manager(connection_id).await?; + if manager.is_shell_workspace(connection_id).await { + return manager + .container_write_file_from_path_with_progress( + connection_id, + path, + local_path, + on_progress, + ) + .await; + } + + let staging = staged_upload_path(path, &uuid::Uuid::new_v4().to_string()); + let written = match manager + .sftp_write_from_file_with_progress( + connection_id, + &staging, + local_path, + u64::MAX, + on_progress, + ) + .await + { + Ok(written) => written, + Err(error) => { + discard_staged_upload(&manager, connection_id, &staging).await; + return Err(error); + } + }; + if let Err(error) = self.atomic_replace(connection_id, &staging, path).await { + discard_staged_upload(&manager, connection_id, &staging).await; + return Err(error); + } + Ok(written) + } + /// Check if a remote path exists pub async fn exists(&self, connection_id: &str, path: &str) -> anyhow::Result { let manager = self.get_manager(connection_id).await?; @@ -692,11 +779,35 @@ fn format_permissions(mode: Option) -> String { #[cfg(test)] mod tests { use super::{ - remote_file_entry_from_metadata, remote_file_entry_from_stat_result, remote_posix_basename, + hidden_data_directory, remote_file_entry_from_metadata, remote_file_entry_from_stat_result, + remote_posix_basename, staged_upload_path, }; use russh_sftp::client::error::Error as SftpError; use russh_sftp::protocol::{Status, StatusCode}; + #[test] + fn staged_upload_stays_beside_its_destination_directory() { + let hidden = hidden_data_directory(); + assert_eq!( + staged_upload_path("/workspace/目录/name.txt", "token"), + format!("/workspace/目录/{hidden}-upload-token.tmp") + ); + assert_eq!( + staged_upload_path("relative/name.txt", "token"), + format!("relative/{hidden}-upload-token.tmp") + ); + // A workspace-root and a bare filename must not gain a parent the + // destination never had, or the publish would move across directories. + assert_eq!( + staged_upload_path("/name.txt", "token"), + format!("/{hidden}-upload-token.tmp") + ); + assert_eq!( + staged_upload_path("name.txt", "token"), + format!("{hidden}-upload-token.tmp") + ); + } + #[test] fn remote_basename_never_uses_host_path_separators() { assert_eq!( diff --git a/src/web-ui/src/app/components/panels/FilesPanel.tsx b/src/web-ui/src/app/components/panels/FilesPanel.tsx index c2dca4502d..1ebbb7192e 100644 --- a/src/web-ui/src/app/components/panels/FilesPanel.tsx +++ b/src/web-ui/src/app/components/panels/FilesPanel.tsx @@ -46,6 +46,7 @@ import type { WorkspaceSearchRepoPhase, } from '@/infrastructure/api/service-api/tauri-commands'; import { + cancelWorkspaceTransfer, downloadWorkspaceFileToDisk, joinWorkspaceTargetPath, normalizeWorkspaceTargetDirectory, @@ -227,6 +228,9 @@ const FilesPanel: React.FC = ({ /** Stop an in-progress transfer by its ID. */ const handleStopTransfer = useCallback((transferId: string) => { cancelledTransferIdsRef.current.add(transferId); + // A peer download runs in this process, so it also needs the stop signal + // that `cancel_transfer` can only deliver to the host running the transfer. + cancelWorkspaceTransfer(transferId); void sshApi.cancelTransfer(transferId); setTransfers((prev) => { const next = new Map(prev); @@ -236,11 +240,15 @@ const FilesPanel: React.FC = ({ }, []); /** - * Stable callback for drag-and-drop file uploads. Uses a ref to track the - * current drop's transfer ID so each drop session gets its own entry in the - * `transfers` Map. + * Stable callback for drag-and-drop file uploads. The drop hook owns the + * transfer id and reports it with every progress state, so the card and the + * cancellable backend transfer stay the same operation. */ const handleDropProgress = useCallback((state: TransferProgressState | null) => { + const reportedId = state?.transferId; + if (reportedId) { + dropTransferIdRef.current = reportedId; + } setTransfers((prev) => { const next = new Map(prev); if (state === null) { @@ -251,7 +259,7 @@ const FilesPanel: React.FC = ({ } } else { if (!dropTransferIdRef.current) { - dropTransferIdRef.current = crypto.randomUUID(); + dropTransferIdRef.current = reportedId ?? crypto.randomUUID(); } next.set(dropTransferIdRef.current, state); } @@ -513,6 +521,8 @@ const FilesPanel: React.FC = ({ id, data.isDirectory, ); + // A stop that raced with completion must not suppress a later failure. + cancelledTransferIdsRef.current.delete(id); } catch (error) { log.error('Failed to download file', error); onProgress(null); @@ -884,17 +894,29 @@ const FilesPanel: React.FC = ({ setFileDropHighlight(overPanel); }, []); - const handleFileDropComplete = useCallback((targetDirectory: string) => { + const handleFileDropComplete = useCallback(( + targetDirectory: string, + transferId: string, + ) => { setFileDropHighlight(false); + // A stop that raced with completion must not suppress a later failure. + cancelledTransferIdsRef.current.delete(transferId); void loadFileTree(workspacePath || '', true); if (workspacePath && !pathsEquivalentFs(targetDirectory, workspacePath)) { expandFolder(targetDirectory, true); } }, [workspacePath, loadFileTree, expandFolder]); - const handleFileDropError = useCallback((error: unknown) => { + const handleFileDropError = useCallback(( + error: unknown, + transferId: string, + ) => { handleDropProgress(null); setFileDropHighlight(false); + if (cancelledTransferIdsRef.current.has(transferId)) { + cancelledTransferIdsRef.current.delete(transferId); + return; + } notification.error(t('transfer.failed', { error: String(error) })); }, [notification, t, handleDropProgress]); diff --git a/src/web-ui/src/features/ssh-remote/sshApi.test.ts b/src/web-ui/src/features/ssh-remote/sshApi.test.ts new file mode 100644 index 0000000000..32ee4446f1 --- /dev/null +++ b/src/web-ui/src/features/ssh-remote/sshApi.test.ts @@ -0,0 +1,42 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const invoke = vi.hoisted(() => vi.fn()); + +vi.mock('@/infrastructure/api/service-api/ApiClient', () => ({ + api: { invoke }, +})); + +import { sshApi } from './sshApi'; + +describe('sshApi request deadlines', () => { + beforeEach(() => { + invoke.mockReset(); + invoke.mockResolvedValue(undefined); + }); + + it('lets a file transfer run past the default request deadline', async () => { + await sshApi.downloadToLocalPath('connection-1', '/remote/a.bin', '/local/a.bin'); + expect(invoke).toHaveBeenCalledWith( + 'remote_download_to_local_path', + expect.objectContaining({ transferId: expect.any(String) }), + { timeout: 0 }, + ); + + await sshApi.uploadFromLocalPath('connection-1', '/local/b.bin', '/remote/b.bin'); + expect(invoke).toHaveBeenLastCalledWith( + 'remote_upload_from_local_path', + expect.objectContaining({ transferId: expect.any(String) }), + { timeout: 0 }, + ); + }); + + it('keeps the default deadline for commands with no stop action', async () => { + await sshApi.readFile('connection-1', '/remote/a.txt'); + // No third argument: the shared client keeps its own deadline, which is + // what ends a command whose host never answers. + expect(invoke).toHaveBeenCalledWith('remote_read_file', { + connectionId: 'connection-1', + path: '/remote/a.txt', + }); + }); +}); diff --git a/src/web-ui/src/features/ssh-remote/sshApi.ts b/src/web-ui/src/features/ssh-remote/sshApi.ts index 4407c2df12..f87bedb851 100644 --- a/src/web-ui/src/features/ssh-remote/sshApi.ts +++ b/src/web-ui/src/features/ssh-remote/sshApi.ts @@ -23,6 +23,20 @@ import type { // API adapter for Tauri/Server Mode compatibility import { api } from '@/infrastructure/api/service-api/ApiClient'; +/** + * Deadline override for the remote file transfers that expose a stop action. + * + * These commands are already bounded where the work happens: every SFTP request + * has its own response timeout, the SSH transport drops a stalled connection, + * and the transfer itself can be stopped from its card. A wall-clock deadline + * here only guaranteed failure for slow-but-healthy links, which is how a + * completed transfer used to surface as "Request timeout". + * + * Commands without a progress or stop surface keep the default deadline: it is + * what keeps an unresponsive host from freezing the UI with no way out. + */ +const NO_REQUEST_DEADLINE = { timeout: 0 } as const; + export const sshApi = { // === Connection Management === @@ -234,7 +248,7 @@ export const sshApi = { remotePath, localPath, transferId: tid, - }); + }, NO_REQUEST_DEADLINE); } finally { unlisten?.(); } @@ -280,7 +294,7 @@ export const sshApi = { localPath, remotePath, transferId: tid, - }); + }, NO_REQUEST_DEADLINE); } finally { unlisten?.(); } diff --git a/src/web-ui/src/infrastructure/api/service-api/ApiClient.test.ts b/src/web-ui/src/infrastructure/api/service-api/ApiClient.test.ts index 2963f28d32..099278093c 100644 --- a/src/web-ui/src/infrastructure/api/service-api/ApiClient.test.ts +++ b/src/web-ui/src/infrastructure/api/service-api/ApiClient.test.ts @@ -245,6 +245,38 @@ describe('ApiClient startup trace classification', () => { } }); + it('lets a streamed transfer opt out of the request deadline', async () => { + vi.useFakeTimers(); + try { + let settle: (value: unknown) => void = () => {}; + adapterMocks.request.mockReturnValueOnce( + new Promise((resolve) => { + settle = resolve; + }), + ); + const client = new ApiClient({ enableLogging: false, retries: 0 }); + + const pending = client.invoke( + 'remote_download_to_local_path', + {}, + { timeout: 0 }, + ); + // Any wall-clock budget is longer than the default 30s deadline, so a + // transfer that is still running must not be aborted. + await vi.advanceTimersByTimeAsync(24 * 60 * 60 * 1000); + settle('transferred'); + + await expect(pending).resolves.toBe('transferred'); + expect(client.getStats()).toMatchObject({ + successfulRequests: 1, + failedRequests: 0, + activeRequests: 0, + }); + } finally { + vi.useRealTimers(); + } + }); + it('uses the message from plain structured Tauri errors', async () => { const transportError = { code: 'worktree_not_found', diff --git a/src/web-ui/src/infrastructure/api/service-api/ApiClient.ts b/src/web-ui/src/infrastructure/api/service-api/ApiClient.ts index 1b8b318b61..e5d512b557 100644 --- a/src/web-ui/src/infrastructure/api/service-api/ApiClient.ts +++ b/src/web-ui/src/infrastructure/api/service-api/ApiClient.ts @@ -355,9 +355,14 @@ export class ApiClient implements IApiClient { }); - const timeoutId = setTimeout(() => { - controller.abort(); - }, request.config.timeout || this.config.timeout); + // A non-positive timeout opts out of the deadline. `??` is deliberate: a + // caller that disables the deadline must not fall back to the default. + const timeoutMs = request.config.timeout ?? this.config.timeout; + const timeoutId = timeoutMs > 0 + ? setTimeout(() => { + controller.abort(); + }, timeoutMs) + : undefined; try { @@ -379,7 +384,9 @@ export class ApiClient implements IApiClient { }); this.assertRequestSurface(request, traceCommand); - clearTimeout(timeoutId); + if (timeoutId !== undefined) { + clearTimeout(timeoutId); + } maxConcurrentRequests = this.activeRequestPressure.get(request.id)?.maxConcurrentRequests ?? this.activeRequests.size; this.activeRequests.delete(request.id); this.activeRequestPressure.delete(request.id); @@ -439,7 +446,9 @@ export class ApiClient implements IApiClient { maxConcurrentRequests = maxConcurrentRequests || this.activeRequestPressure.get(request.id)?.maxConcurrentRequests || this.activeRequests.size; - clearTimeout(timeoutId); + if (timeoutId !== undefined) { + clearTimeout(timeoutId); + } this.activeRequests.delete(request.id); this.activeRequestPressure.delete(request.id); activeRequestsAtEnd = this.activeRequests.size; diff --git a/src/web-ui/src/infrastructure/api/service-api/types.ts b/src/web-ui/src/infrastructure/api/service-api/types.ts index 8aec338b23..cd0e689595 100644 --- a/src/web-ui/src/infrastructure/api/service-api/types.ts +++ b/src/web-ui/src/infrastructure/api/service-api/types.ts @@ -19,6 +19,14 @@ export interface ApiError { export interface ApiRequestConfig { + /** + * Deadline for this request in milliseconds. + * + * Non-positive values disable the deadline, which long-running streaming + * commands (file transfers) use because their own layer already bounds every + * protocol request and the user can stop them explicitly. Such a command must + * provide cancellation; without a deadline nothing else ends the wait. + */ timeout?: number; retries?: number; retryDelay?: number; diff --git a/src/web-ui/src/locales/en-US/panels/files.json b/src/web-ui/src/locales/en-US/panels/files.json index 685e41a5cb..84e3cb35af 100644 --- a/src/web-ui/src/locales/en-US/panels/files.json +++ b/src/web-ui/src/locales/en-US/panels/files.json @@ -128,6 +128,7 @@ "downloading": "Downloading", "uploading": "Uploading", "missingConnection": "Remote workspace has no SSH connection id.", + "cancelled": "Transfer cancelled", "failed": "File transfer failed: {{error}}", "dropHint": "Drop files here to upload", "stop": "Stop" diff --git a/src/web-ui/src/locales/zh-CN/panels/files.json b/src/web-ui/src/locales/zh-CN/panels/files.json index 75826d53ca..369e9a45a4 100644 --- a/src/web-ui/src/locales/zh-CN/panels/files.json +++ b/src/web-ui/src/locales/zh-CN/panels/files.json @@ -128,6 +128,7 @@ "downloading": "正在下载", "uploading": "正在上传", "missingConnection": "远程工作区缺少 SSH 连接信息。", + "cancelled": "传输已取消", "failed": "文件传输失败:{{error}}", "dropHint": "将文件拖放到此处以上传", "stop": "停止" diff --git a/src/web-ui/src/locales/zh-TW/panels/files.json b/src/web-ui/src/locales/zh-TW/panels/files.json index 4b38fa84fb..8395e60b0c 100644 --- a/src/web-ui/src/locales/zh-TW/panels/files.json +++ b/src/web-ui/src/locales/zh-TW/panels/files.json @@ -128,6 +128,7 @@ "downloading": "正在下載", "uploading": "正在上傳", "missingConnection": "遠程工作區缺少 SSH 連接資訊。", + "cancelled": "傳輸已取消", "failed": "檔案傳輸失敗:{{error}}", "dropHint": "將檔案拖放到此處以上傳", "stop": "停止" diff --git a/src/web-ui/src/tools/file-system/hooks/useWorkspaceFileDrop.ts b/src/web-ui/src/tools/file-system/hooks/useWorkspaceFileDrop.ts index 7df09912b4..a819f743b9 100644 --- a/src/web-ui/src/tools/file-system/hooks/useWorkspaceFileDrop.ts +++ b/src/web-ui/src/tools/file-system/hooks/useWorkspaceFileDrop.ts @@ -19,8 +19,8 @@ export interface UseWorkspaceFileDropOptions { enabled?: boolean; onProgress: (state: TransferProgressState | null) => void; onDragOver?: (overPanel: boolean) => void; - onComplete: (targetDirectory: string) => void; - onError: (error: unknown) => void; + onComplete: (targetDirectory: string, transferId: string) => void; + onError: (error: unknown, transferId: string) => void; } export function useWorkspaceFileDrop({ @@ -151,17 +151,23 @@ export function useWorkspaceFileDrop({ isDragOverPanelRef.current = false; dropProcessingRef.current = true; + // One id per drop session: it keys the progress card and reaches the + // backend, so the card's stop action cancels this upload instead of + // sending an id no transfer ever registered. + const transferId = crypto.randomUUID(); try { await uploadLocalPathsToWorkspaceDirectory( paths, targetDir, currentWorkspace, - onProgress + onProgress, + undefined, + transferId ); - onComplete(targetDir); + onComplete(targetDir, transferId); } catch (error) { log.error('Failed to upload dropped files', error); - onError(error); + onError(error, transferId); } finally { dropProcessingRef.current = false; } diff --git a/src/web-ui/src/tools/file-system/services/workspaceFileTransfer.test.ts b/src/web-ui/src/tools/file-system/services/workspaceFileTransfer.test.ts index fcb66d01af..748c726ebd 100644 --- a/src/web-ui/src/tools/file-system/services/workspaceFileTransfer.test.ts +++ b/src/web-ui/src/tools/file-system/services/workspaceFileTransfer.test.ts @@ -1,5 +1,6 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import { + cancelWorkspaceTransfer, decodeBase64FileChunk, writeAllToLocalFile, readPeerFileChunks, @@ -7,7 +8,10 @@ import { joinWorkspaceTargetPath, normalizeClipboardLocalPaths, resolvePasteTargetDirectory, + uploadLocalPathsToWorkspaceDirectory, } from "./workspaceFileTransfer"; +import { WorkspaceKind, WorkspaceType, type WorkspaceInfo } from "@/shared/types"; +import { i18nService } from "@/infrastructure/i18n"; describe("workspaceFileTransfer", () => { it("decodes peer file chunks without corrupting binary bytes", () => { @@ -157,4 +161,126 @@ describe("fixed peer download identity", () => { await stream.next(); await expect(stream.next()).rejects.toThrow("changed during download"); }); + + it("stops a peer download when the controller-side stop request arrives", async () => { + const requestPeerCommand = vi.fn() + .mockResolvedValueOnce({ resp: "file_info", size: 4 }) + .mockImplementationOnce(async () => { + // The user pressed stop while the first chunk was in flight. + cancelWorkspaceTransfer("peer-transfer"); + return { resp: "file_chunk", offset: 0, chunk_size: 1, total_size: 4, chunk_base64: "AQ==", revision: "r1" }; + }); + const adapter = { requestPeerCommand } as unknown as Parameters[0]; + const stream = readPeerFileChunks( + adapter, + "/workspace/file", + vi.fn(), + { workspace_path: "/workspace" }, + "peer-transfer", + ); + expect((await stream.next()).value).toEqual(new Uint8Array([1])); + await expect(stream.next()).rejects.toThrow( + i18nService.t("panels/files:transfer.cancelled"), + ); + expect(requestPeerCommand).toHaveBeenCalledTimes(2); + + // The id is only stopped once: a later chunk read for another transfer of + // the same file must proceed. + const retry = vi.fn() + .mockResolvedValueOnce({ resp: "file_info", size: 1 }) + .mockResolvedValueOnce({ resp: "file_chunk", offset: 0, chunk_size: 1, total_size: 1, chunk_base64: "AQ==", revision: "r1" }); + const retryAdapter = { requestPeerCommand: retry } as unknown as Parameters[0]; + const chunks: number[] = []; + for await (const chunk of readPeerFileChunks( + retryAdapter, + "/workspace/file", + vi.fn(), + { workspace_path: "/workspace" }, + "another-transfer", + )) { + chunks.push(...chunk); + } + expect(chunks).toEqual([1]); + }); +}); + +describe("remote workspace uploads report their cancellable transfer id", () => { + const uploadFromLocalPath = vi.fn(); + + beforeEach(() => { + vi.resetModules(); + vi.resetAllMocks(); + uploadFromLocalPath.mockResolvedValue({ wasDirectory: false }); + (globalThis as { window?: unknown }).window = globalThis; + (globalThis as unknown as { __TAURI__: unknown }).__TAURI__ = {}; + }); + + const remoteWorkspace: WorkspaceInfo = { + id: "workspace-1", + name: "remote", + rootPath: "/workspace", + workspaceType: WorkspaceType.SingleProject, + workspaceKind: WorkspaceKind.Remote, + connectionId: "saved-ssh", + languages: [], + openedAt: "2024-01-01T00:00:00.000Z", + lastAccessed: "2024-01-01T00:00:00.000Z", + tags: [], + }; + + async function loadTransferModule() { + vi.doMock("@/features/ssh-remote/sshApi", () => ({ + sshApi: { uploadFromLocalPath }, + })); + return await import("./workspaceFileTransfer"); + } + + it("passes one id to the backend command and to every progress state", async () => { + const { uploadLocalPathsToWorkspaceDirectory: upload } = + await loadTransferModule(); + const states: Array<{ transferId?: string }> = []; + const result = await upload( + ["/local/a.txt"], + "/workspace", + remoteWorkspace, + (state) => states.push(state ?? {}), + undefined, + "transfer-42", + ); + + expect(result.successCount).toBe(1); + expect(uploadFromLocalPath).toHaveBeenCalledTimes(1); + const [, , , , forwardedId] = uploadFromLocalPath.mock.calls[0]!; + expect(forwardedId).toBe("transfer-42"); + expect(states.filter((state) => state.transferId).length).toBeGreaterThan(0); + for (const state of states) { + if (state.transferId) { + expect(state.transferId).toBe("transfer-42"); + } + } + }); + + it("stops the remaining items of a multi-file upload after a stop request", async () => { + const { cancelWorkspaceTransfer, uploadLocalPathsToWorkspaceDirectory: upload } = + await loadTransferModule(); + // The first item is stopped while it is being sent, which the backend + // reports as a failure. The remaining items must not start new transfers. + uploadFromLocalPath.mockImplementation(async () => { + cancelWorkspaceTransfer("transfer-multi"); + throw new Error("Transfer cancelled"); + }); + const states: Array<{ transferId?: string }> = []; + + await expect( + upload( + ["/local/a.txt", "/local/b.txt", "/local/c.txt"], + "/workspace", + remoteWorkspace, + (state) => states.push(state ?? {}), + undefined, + "transfer-multi", + ), + ).rejects.toThrow(/a\.txt/); + expect(uploadFromLocalPath).toHaveBeenCalledTimes(1); + }); }); diff --git a/src/web-ui/src/tools/file-system/services/workspaceFileTransfer.ts b/src/web-ui/src/tools/file-system/services/workspaceFileTransfer.ts index a6551a65c4..5ca93f761e 100644 --- a/src/web-ui/src/tools/file-system/services/workspaceFileTransfer.ts +++ b/src/web-ui/src/tools/file-system/services/workspaceFileTransfer.ts @@ -35,6 +35,13 @@ export interface TransferProgressState { bytesTotal?: number; /** Transfer speed in bytes per second (smoothed) */ speed?: number; + /** + * Transfer id this progress belongs to. + * + * A path that can be stopped must report the same id it registered, so a + * progress card's stop action reaches the operation it displays. + */ + transferId?: string; } export interface WorkspaceTransferResult { @@ -99,6 +106,45 @@ function currentPeerAdapter(): PeerDeviceTransportAdapter | null { return adapter instanceof PeerDeviceTransportAdapter ? adapter : null; } +/** + * Controller-side stop requests for transfers the controller executes itself. + * + * A peer download reads chunks through this process, so its stop signal has to + * live here: `cancel_transfer` only reaches the host that runs the SSH + * connection, and in Peer Device Mode no such command is in flight. + */ +const cancelledWorkspaceTransfers = new Set(); + +/** + * Stop a transfer started by this module. Returns whether an id was stopped + * locally; the remote host still has to be told separately for hosted + * transfers. + */ +export function cancelWorkspaceTransfer(transferId: string): boolean { + if (!transferId) { + return false; + } + cancelledWorkspaceTransfers.add(transferId); + return true; +} + +/** Unregister a finished transfer so its stop mark cannot leak into a later id. */ +export function completeWorkspaceTransfer(transferId: string | undefined): void { + if (transferId) { + cancelledWorkspaceTransfers.delete(transferId); + } +} + +function isWorkspaceTransferCancelled(transferId: string | undefined): boolean { + return Boolean(transferId && cancelledWorkspaceTransfers.has(transferId)); +} + +function throwIfCancelled(transferId: string | undefined): void { + if (isWorkspaceTransferCancelled(transferId)) { + throw new Error(i18nService.t("panels/files:transfer.cancelled")); + } +} + export async function writeAllToLocalFile( destinationPath: string, chunks: AsyncIterable, @@ -134,7 +180,9 @@ export async function* readPeerFileChunks( sourcePath: string, onFileSize: (size: number) => void, identity: PeerFileWorkspaceIdentity, + transferId?: string, ): AsyncGenerator { + throwIfCancelled(transferId); const info = await adapter.requestPeerCommand({ cmd: "get_file_info", path: sourcePath, @@ -149,6 +197,8 @@ export async function* readPeerFileChunks( let offset = 0; let revision: string | undefined; while (offset < info.size) { + // A stop request must end the chunk loop instead of reading the whole file. + throwIfCancelled(transferId); const response = await adapter.requestPeerCommand({ cmd: "read_file_chunk", path: sourcePath, @@ -231,6 +281,7 @@ async function downloadPeerWorkspacePathToDisk( workspace: WorkspaceInfo | null, isDirectory: boolean, onProgress: (state: TransferProgressState | null) => void, + transferId?: string, ): Promise { if (!workspace?.id) throw new Error("A fixed peer workspace is required for download"); if (isRemoteWorkspace(workspace) && !workspace.connectionId) { @@ -265,7 +316,13 @@ async function downloadPeerWorkspacePathToDisk( bytesTotal += size - expectedEntrySize; expectedEntrySize = size; }; - const chunks = readPeerFileChunks(adapter, entry.sourcePath, updateExpectedEntrySize, identity); + const chunks = readPeerFileChunks( + adapter, + entry.sourcePath, + updateExpectedEntrySize, + identity, + transferId, + ); await writeAllToLocalFile(entry.destinationPath, chunks, (written) => { entryWritten += written; bytesTransferred = entryStart + entryWritten; @@ -292,6 +349,7 @@ async function downloadPeerWorkspacePathToDisk( bytesTransferred, bytesTotal: Math.max(bytesTotal, bytesTransferred), speed: smoothedSpeed, + ...(transferId ? { transferId } : {}), }); }); bytesTransferred = entryStart + entryWritten; @@ -306,6 +364,7 @@ async function downloadPeerWorkspacePathToDisk( bytesTransferred, bytesTotal: bytesTransferred, speed: smoothedSpeed, + ...(transferId ? { transferId } : {}), }); } @@ -593,6 +652,7 @@ export async function downloadWorkspaceFileToDisk( workspace, Boolean(isDirectory), onProgress, + transferId, ); } else if (isRemoteWorkspace(workspace)) { const cid = workspace?.connectionId; @@ -649,8 +709,12 @@ export async function downloadWorkspaceFileToDisk( total: 1, label: baseName, indeterminate: false, + ...(transferId ? { transferId } : {}), }); } finally { + // The id is only meaningful while this download runs; a later transfer + // must not inherit its stop mark. + completeWorkspaceTransfer(transferId); window.setTimeout(() => onProgress(null), 450); } } @@ -691,6 +755,11 @@ export async function uploadLocalPathsToWorkspaceDirectory( const total = normalizedLocalPaths.length; for (let i = 0; i < total; i++) { + // A stop ends the whole drop session: the remaining items must not start + // a fresh backend transfer that no card is watching any more. + if (isWorkspaceTransferCancelled(transferId)) { + break; + } const localPath = normalizedLocalPaths[i]!; const name = localPath.split(/[/\\]/).pop(); if (!name) { @@ -713,6 +782,7 @@ export async function uploadLocalPathsToWorkspaceDirectory( total, label: singleItem ? name : `${name} (${i + 1}/${total})`, indeterminate: singleItem, + ...(transferId ? { transferId } : {}), }); try { @@ -755,6 +825,7 @@ export async function uploadLocalPathsToWorkspaceDirectory( bytesTransferred: uploaded, bytesTotal: totalBytes, speed: smoothedSpeed, + ...(transferId ? { transferId } : {}), }); } }, @@ -765,7 +836,15 @@ export async function uploadLocalPathsToWorkspaceDirectory( directoryCount += 1; } } else { - const uploadResult = await sshApi.uploadFromLocalPath(cid, localPath, destPath); + // Multi-item uploads share one transfer id, so the visible stop + // action reaches whichever item is currently being sent. + const uploadResult = await sshApi.uploadFromLocalPath( + cid, + localPath, + destPath, + undefined, + transferId, + ); successCount += 1; if (uploadResult.wasDirectory) { directoryCount += 1; @@ -779,13 +858,18 @@ export async function uploadLocalPathsToWorkspaceDirectory( } } - onProgress({ - phase: "upload", - current: total, - total, - label: "", - indeterminate: false, - }); + const wasCancelled = isWorkspaceTransferCancelled(transferId); + if (!wasCancelled) { + onProgress({ + phase: "upload", + current: total, + total, + label: "", + indeterminate: false, + ...(transferId ? { transferId } : {}), + }); + } + completeWorkspaceTransfer(transferId); window.setTimeout(() => onProgress(null), 450); if (successCount === 0 && failedFiles.length > 0) { @@ -798,6 +882,9 @@ export async function uploadLocalPathsToWorkspaceDirectory( return { successCount, directoryCount, failedFiles }; } + // A local paste is not cancellable: the clipboard helper owns it, and no + // stop mark may linger for an id this module never registered. + completeWorkspaceTransfer(transferId); onProgress({ phase: "upload", current: 0, From 03005b1d8b879c7484aacdc003919b2e21b4745c Mon Sep 17 00:00:00 2001 From: GCWing Date: Mon, 28 Sep 2026 18:01:52 +0800 Subject: [PATCH 2/3] fix(remote-transfer): align installer CI and cancellation paths --- OpenBitFun-Installer/src-tauri/Cargo.toml | 14 ++++ .../remote-workspace-transport.md | 18 ++-- src/apps/desktop/src/api/ssh_api.rs | 17 +++- .../src/remote_ssh/manager.rs | 24 +++++- .../src/remote_ssh/remote_fs.rs | 7 ++ .../src/app/components/panels/FilesPanel.tsx | 6 ++ .../ssh-remote/RemoteFileBrowser.test.tsx | 33 ++++++++ .../features/ssh-remote/RemoteFileBrowser.tsx | 83 ++++++++++++++++--- .../src/features/ssh-remote/sshApi.test.ts | 25 +++++- src/web-ui/src/features/ssh-remote/sshApi.ts | 14 +++- .../services/workspaceFileTransfer.test.ts | 32 +++++++ .../services/workspaceFileTransfer.ts | 18 +++- 12 files changed, 259 insertions(+), 32 deletions(-) diff --git a/OpenBitFun-Installer/src-tauri/Cargo.toml b/OpenBitFun-Installer/src-tauri/Cargo.toml index aa5c67036f..d2782ca6c7 100644 --- a/OpenBitFun-Installer/src-tauri/Cargo.toml +++ b/OpenBitFun-Installer/src-tauri/Cargo.toml @@ -15,10 +15,16 @@ path = "src/main.rs" [build-dependencies] tauri-build = { version = "=2.6.3", features = [] } +# This standalone app does not commit a Cargo.lock. Keep the Tauri build-time +# crates aligned with the patched 2.11 runtime instead of resolving 2.7/2.10. +tauri-codegen = "=2.6.3" zip = "0.6" [dependencies] tauri = { version = "=2.11.5", features = [] } +tauri-macros = "=2.6.3" +tauri-plugin = "=2.6.3" +tauri-utils = "=2.9.3" tauri-plugin-dialog = "=2.7.3" serde = { version = "1", features = ["derive"] } serde_json = "1" @@ -35,6 +41,14 @@ openbitfun-services-core = { path = "../../src/crates/services/services-core", f winreg = "0.52" mslnk = "0.1" +# The installer is a separate Cargo workspace. Keep Tauri's runtime crates on +# the same revision as the desktop workspace; otherwise the exact Tauri 2.11.5 +# dependency can resolve newer, incompatible runtime and utils releases. +[patch.crates-io] +tauri-runtime = { git = "https://github.com/tauri-apps/tauri.git", rev = "7cc68e74ff6981f5c50a52a67d56c5eb2d227188" } +tauri-runtime-wry = { git = "https://github.com/tauri-apps/tauri.git", rev = "7cc68e74ff6981f5c50a52a67d56c5eb2d227188" } +tauri-utils = { git = "https://github.com/tauri-apps/tauri.git", rev = "7cc68e74ff6981f5c50a52a67d56c5eb2d227188" } + [dev-dependencies] tempfile = "3" diff --git a/docs/architecture/remote-workspace-transport.md b/docs/architecture/remote-workspace-transport.md index 75e3c6006a..1fa507c2a7 100644 --- a/docs/architecture/remote-workspace-transport.md +++ b/docs/architecture/remote-workspace-transport.md @@ -189,20 +189,22 @@ and renames it over the destination, the container command does the same after its size check, and a downloaded file is staged beside its local destination the same way. A failed or cancelled transfer therefore leaves any previous destination unchanged, and staging temporaries are removed when the owning side -can still reach the path. Because a published upload replaces its destination +can still reach the path. For directory transfers, files already published and +directories already created remain when a later entry fails or is stopped. +Because a published upload replaces its destination rather than writing through it, the result takes the staging file's mode and ownership, and a replaced symlink or hard link is not followed; that is what distinguishes a transfer from a workspace tool write above. A hard kill between staging and commit can leave one orphaned temporary, which the container commands sweep by age and the SFTP path leaves to the user. -The controller applies no wall-clock deadline to commands whose duration scales -with file size or entry count (file read and write, directory listing, tree, -upload, download). Those operations are bounded where the work happens — every -SFTP request has its own response timeout and the SSH transport drops a stalled -connection — and a transfer ends when it completes, fails, or is stopped. A -short controller deadline only reported a slow-but-healthy transfer as a failure -while the host kept transferring it. +The desktop upload and download commands opt out of the shared 30-second +controller deadline when their caller provides a progress and stop surface. +Every SFTP request retains its own response timeout, the SSH transport drops a +stalled connection, and the transfer ends when it completes, fails, or is +stopped. File reads, writes, listings, and transfers without a stop surface keep +the default controller deadline. A short deadline on a stoppable transfer only +reported a slow-but-healthy operation as failed while the host kept working. Directory and stat records use NUL-separated fields. File names containing newlines or the delimiters used by older implementations remain round-trippable. diff --git a/src/apps/desktop/src/api/ssh_api.rs b/src/apps/desktop/src/api/ssh_api.rs index fb48363382..555c6ac74b 100644 --- a/src/apps/desktop/src/api/ssh_api.rs +++ b/src/apps/desktop/src/api/ssh_api.rs @@ -606,6 +606,11 @@ async fn download_remote_file_to_local( downloaded = downloaded.saturating_add(read as u64); on_progress(downloaded); } + // Stop can arrive during the final read (including an empty file). Do not + // replace a destination after that stop just because the stream reached EOF. + if transfer.is_cancelled() { + return Err(TRANSFER_CANCELLED.to_string()); + } staging.publish().await?; Ok(downloaded) } @@ -658,8 +663,8 @@ pub async fn remote_download_to_local_path( local_path: String, transfer_id: String, ) -> Result<(), String> { - let remote_fs = state.get_remote_file_service_async().await?; let transfer = ActiveTransfer::register(&state.active_transfers, &transfer_id)?; + let remote_fs = state.get_remote_file_service_async().await?; let mut last_emit = Instant::now(); // Check if the remote path is a directory. @@ -814,11 +819,17 @@ async fn download_directory_from_remote( let mut total_bytes: u64 = 0; let mut scan_stack = vec![remote_dir.to_string()]; while let Some(current) = scan_stack.pop() { + if transfer.is_cancelled() { + return Err(TRANSFER_CANCELLED.to_string()); + } let entries = remote_fs .read_dir(connection_id, ¤t) .await .map_err(|e| e.to_string())?; for entry in entries { + if transfer.is_cancelled() { + return Err(TRANSFER_CANCELLED.to_string()); + } validate_remote_name_for_local_download(&entry.name)?; if entry.is_symlink { return Err(format!( @@ -1016,6 +1027,7 @@ pub async fn remote_upload_from_local_path( remote_path: String, transfer_id: String, ) -> Result { + let transfer = ActiveTransfer::register(&state.active_transfers, &transfer_id)?; let local_path = std::path::Path::new(&local_path); let local_metadata = std::fs::symlink_metadata(local_path).map_err(|error| { format!( @@ -1037,9 +1049,6 @@ pub async fn remote_upload_from_local_path( )); } - // Register a cancellation flag for this transfer. - let transfer = ActiveTransfer::register(&state.active_transfers, &transfer_id)?; - // A directory needs to be walked locally and recreated on the remote side. if local_path.is_dir() { upload_directory_to_remote( diff --git a/src/crates/services/services-integrations/src/remote_ssh/manager.rs b/src/crates/services/services-integrations/src/remote_ssh/manager.rs index 9d08666670..4191053529 100644 --- a/src/crates/services/services-integrations/src/remote_ssh/manager.rs +++ b/src/crates/services/services-integrations/src/remote_ssh/manager.rs @@ -5587,10 +5587,26 @@ impl SSHConnectionManager { } } } - stdin - .shutdown() - .await - .with_context(|| format!("Failed to finish container file upload '{}'", path))?; + if !on_progress(written, total) { + return Err(abort_container_upload( + &control, + completion, + anyhow!("Transfer cancelled"), + ) + .await); + } + if let Err(error) = stdin.shutdown().await { + return Err(abort_container_upload( + &control, + completion, + anyhow!( + "Failed to finish container file upload '{}': {}", + path, + error + ), + ) + .await); + } let exit = completion.wait().await; let _stdout = stdout_task.await.unwrap_or_default(); let stderr = stderr_task.await.unwrap_or_default(); diff --git a/src/crates/services/services-integrations/src/remote_ssh/remote_fs.rs b/src/crates/services/services-integrations/src/remote_ssh/remote_fs.rs index 9a2be35cfd..33d8a6f21d 100644 --- a/src/crates/services/services-integrations/src/remote_ssh/remote_fs.rs +++ b/src/crates/services/services-integrations/src/remote_ssh/remote_fs.rs @@ -326,6 +326,13 @@ impl RemoteFileService { return Err(error); } }; + // An empty file has no chunk callback, and a stop can also arrive + // while the final chunk is being flushed. Check once more before the + // staged file becomes the destination. + if !on_progress(written, written) { + discard_staged_upload(&manager, connection_id, &staging).await; + anyhow::bail!("Transfer cancelled"); + } if let Err(error) = self.atomic_replace(connection_id, &staging, path).await { discard_staged_upload(&manager, connection_id, &staging).await; return Err(error); diff --git a/src/web-ui/src/app/components/panels/FilesPanel.tsx b/src/web-ui/src/app/components/panels/FilesPanel.tsx index 1ebbb7192e..d9da386034 100644 --- a/src/web-ui/src/app/components/panels/FilesPanel.tsx +++ b/src/web-ui/src/app/components/panels/FilesPanel.tsx @@ -213,6 +213,9 @@ const FilesPanel: React.FC = ({ const id = crypto.randomUUID(); const onProgress = (state: TransferProgressState | null) => { setTransfers((prev) => { + if (state !== null && cancelledTransferIdsRef.current.has(id)) { + return prev; + } const next = new Map(prev); if (state === null) { next.delete(id); @@ -250,6 +253,9 @@ const FilesPanel: React.FC = ({ dropTransferIdRef.current = reportedId; } setTransfers((prev) => { + if (state !== null && cancelledTransferIdsRef.current.has(dropTransferIdRef.current ?? reportedId ?? '')) { + return prev; + } const next = new Map(prev); if (state === null) { const id = dropTransferIdRef.current; diff --git a/src/web-ui/src/features/ssh-remote/RemoteFileBrowser.test.tsx b/src/web-ui/src/features/ssh-remote/RemoteFileBrowser.test.tsx index ef2a14f230..71f95ed437 100644 --- a/src/web-ui/src/features/ssh-remote/RemoteFileBrowser.test.tsx +++ b/src/web-ui/src/features/ssh-remote/RemoteFileBrowser.test.tsx @@ -10,9 +10,12 @@ globalThis.IS_REACT_ACT_ENVIRONMENT = true; const api = vi.hoisted(() => ({ readDir: vi.fn(async () => [{ name: 'old.txt', path: '/srv/project/old.txt', isDir: false }]), rename: vi.fn(async () => undefined), + downloadToLocalPath: vi.fn(), + cancelTransfer: vi.fn(async () => undefined), })); vi.mock('./sshApi', () => ({ sshApi: api })); vi.mock('@/infrastructure/i18n', () => ({ useI18n: () => ({ t: (key: string) => key }) })); +vi.mock('@tauri-apps/plugin-dialog', () => ({ save: vi.fn(async () => '/downloads/old.txt') })); it('renames a remote file once and keeps IME and outside clicks from dismissing the dialog', async () => { const container = document.createElement('div'); @@ -50,3 +53,33 @@ it('renames a remote file once and keeps IME and outside clicks from dismissing container.remove(); } }); + +it('stops a browser download using the id of its in-flight command', async () => { + Object.defineProperty(window, '__TAURI__', { value: {}, configurable: true }); + let finishDownload!: () => void; + api.downloadToLocalPath.mockImplementationOnce(() => new Promise((resolve) => { + finishDownload = resolve; + })); + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + try { + await act(async () => root.render( undefined} onCancel={() => undefined} />)); + const row = [...document.querySelectorAll('.remote-file-browser__row')].find(node => node.textContent?.includes('old.txt'))!; + act(() => row.dispatchEvent(new MouseEvent('contextmenu', { bubbles: true }))); + const download = [...document.querySelectorAll('[role="menuitem"]')].find(node => node.textContent === 'ssh.remote.download')!; + await act(async () => download.click()); + + const transferId = api.downloadToLocalPath.mock.calls[0]?.[4]; + expect(transferId).toEqual(expect.any(String)); + const stop = [...document.querySelectorAll('button')].find(node => node.textContent === 'actions.cancel')!; + await act(async () => stop.click()); + expect(api.cancelTransfer).toHaveBeenCalledExactlyOnceWith(transferId); + expect(api.downloadToLocalPath.mock.calls[0]?.[5]()).toBe(true); + await act(async () => finishDownload()); + } finally { + act(() => root.unmount()); + container.remove(); + delete (window as Window & { __TAURI__?: unknown }).__TAURI__; + } +}); diff --git a/src/web-ui/src/features/ssh-remote/RemoteFileBrowser.tsx b/src/web-ui/src/features/ssh-remote/RemoteFileBrowser.tsx index 6a039a558a..2608866404 100644 --- a/src/web-ui/src/features/ssh-remote/RemoteFileBrowser.tsx +++ b/src/web-ui/src/features/ssh-remote/RemoteFileBrowser.tsx @@ -37,6 +37,11 @@ interface DeleteConfirmState { entry: RemoteFileEntry | null; } +interface ActiveBrowserTransfer { + id: string; + cancelled: boolean; +} + function joinRemotePath(dir: string, fileName: string): string { const name = fileName.replace(/^\/+/, ''); if (!dir || dir === '/') { @@ -104,8 +109,44 @@ export const RemoteFileBrowser: React.FC = ({ entry: null, }); const [transferBusy, setTransferBusy] = useState(false); + const [transferCancelling, setTransferCancelling] = useState(false); + const activeTransferRef = useRef(null); const contextMenuRef = useRef(null); + const beginTransfer = (): ActiveBrowserTransfer | null => { + if (activeTransferRef.current) return null; + const transfer = { id: crypto.randomUUID(), cancelled: false }; + activeTransferRef.current = transfer; + setTransferBusy(true); + setTransferCancelling(false); + setError(null); + return transfer; + }; + + const endTransfer = (transfer: ActiveBrowserTransfer): void => { + if (activeTransferRef.current !== transfer) return; + activeTransferRef.current = null; + setTransferBusy(false); + setTransferCancelling(false); + }; + + const stopTransfer = (): void => { + const transfer = activeTransferRef.current; + if (!transfer || transfer.cancelled) return; + transfer.cancelled = true; + setTransferCancelling(true); + void sshApi.cancelTransfer(transfer.id).catch((error: unknown) => { + setError(error instanceof Error ? error.message : t('ssh.remote.transferFailed')); + }); + }; + + useEffect(() => () => { + const transfer = activeTransferRef.current; + if (!transfer) return; + transfer.cancelled = true; + void sshApi.cancelTransfer(transfer.id).catch(() => undefined); + }, []); + // One-shot retry: when the SSH session was torn down by a transient network // blip, the backend transparently reconnects on the next call but the // already-in-flight request still fails. Retrying once gives the recovery @@ -230,14 +271,19 @@ export const RemoteFileBrowser: React.FC = ({ }); if (localPath === null) return; - setTransferBusy(true); - setError(null); + const transfer = beginTransfer(); + if (!transfer) return; try { - await sshApi.downloadToLocalPath(connectionId, entry.path, localPath); + await sshApi.downloadToLocalPath( + connectionId, entry.path, localPath, undefined, transfer.id, + () => transfer.cancelled, + ); } catch (e) { - setError(e instanceof Error ? e.message : t('ssh.remote.transferFailed')); + if (!transfer.cancelled) { + setError(e instanceof Error ? e.message : t('ssh.remote.transferFailed')); + } } finally { - setTransferBusy(false); + endTransfer(transfer); } }; @@ -322,21 +368,29 @@ export const RemoteFileBrowser: React.FC = ({ const paths = Array.isArray(selected) ? selected : [selected]; if (paths.length === 0) return; - setTransferBusy(true); - setError(null); + const transfer = beginTransfer(); + if (!transfer) return; try { for (const localPath of paths) { + if (transfer.cancelled) break; const segments = localPath.split(/[/\\]/); const base = segments.pop(); if (!base) continue; const remotePath = joinRemotePath(currentPath, base); - await sshApi.uploadFromLocalPath(connectionId, localPath, remotePath); + await sshApi.uploadFromLocalPath( + connectionId, localPath, remotePath, undefined, transfer.id, + () => transfer.cancelled, + ); } await loadDirectory(currentPath); } catch (e) { - setError(e instanceof Error ? e.message : t('ssh.remote.transferFailed')); + if (!transfer.cancelled) { + setError(e instanceof Error ? e.message : t('ssh.remote.transferFailed')); + } else { + void loadDirectory(currentPath); + } } finally { - setTransferBusy(false); + endTransfer(transfer); } }; @@ -506,6 +560,15 @@ export const RemoteFileBrowser: React.FC = ({
{t('ssh.remote.transferring')} +
)} diff --git a/src/web-ui/src/features/ssh-remote/sshApi.test.ts b/src/web-ui/src/features/ssh-remote/sshApi.test.ts index 32ee4446f1..38b7d26f09 100644 --- a/src/web-ui/src/features/ssh-remote/sshApi.test.ts +++ b/src/web-ui/src/features/ssh-remote/sshApi.test.ts @@ -15,14 +15,14 @@ describe('sshApi request deadlines', () => { }); it('lets a file transfer run past the default request deadline', async () => { - await sshApi.downloadToLocalPath('connection-1', '/remote/a.bin', '/local/a.bin'); + await sshApi.downloadToLocalPath('connection-1', '/remote/a.bin', '/local/a.bin', undefined, 'download-1'); expect(invoke).toHaveBeenCalledWith( 'remote_download_to_local_path', expect.objectContaining({ transferId: expect.any(String) }), { timeout: 0 }, ); - await sshApi.uploadFromLocalPath('connection-1', '/local/b.bin', '/remote/b.bin'); + await sshApi.uploadFromLocalPath('connection-1', '/local/b.bin', '/remote/b.bin', undefined, 'upload-1'); expect(invoke).toHaveBeenLastCalledWith( 'remote_upload_from_local_path', expect.objectContaining({ transferId: expect.any(String) }), @@ -38,5 +38,26 @@ describe('sshApi request deadlines', () => { connectionId: 'connection-1', path: '/remote/a.txt', }); + + await sshApi.downloadToLocalPath('connection-1', '/remote/a.bin', '/local/a.bin'); + expect(invoke).toHaveBeenLastCalledWith( + 'remote_download_to_local_path', + expect.objectContaining({ transferId: expect.any(String) }), + undefined, + ); + + await sshApi.uploadFromLocalPath('connection-1', '/local/b.bin', '/remote/b.bin'); + expect(invoke).toHaveBeenLastCalledWith( + 'remote_upload_from_local_path', + expect.objectContaining({ transferId: expect.any(String) }), + undefined, + ); + }); + + it('does not start a transfer stopped before the backend registration', async () => { + await expect(sshApi.uploadFromLocalPath( + 'connection-1', '/local/b.bin', '/remote/b.bin', undefined, 'stopped', () => true, + )).rejects.toThrow('Transfer cancelled'); + expect(invoke).not.toHaveBeenCalled(); }); }); diff --git a/src/web-ui/src/features/ssh-remote/sshApi.ts b/src/web-ui/src/features/ssh-remote/sshApi.ts index f87bedb851..b5e1c205f2 100644 --- a/src/web-ui/src/features/ssh-remote/sshApi.ts +++ b/src/web-ui/src/features/ssh-remote/sshApi.ts @@ -226,6 +226,7 @@ export const sshApi = { localPath: string, onProgress?: (downloaded: number, total: number) => void, transferId?: string, + isCancelled?: () => boolean, ): Promise { const tid = transferId ?? crypto.randomUUID(); let unlisten: (() => void) | null = null; @@ -243,12 +244,17 @@ export const sshApi = { } try { + // The stop action can run while the progress listener is being installed, + // before the backend has registered this transfer id. + if (isCancelled?.()) { + throw new Error('Transfer cancelled'); + } await api.invoke('remote_download_to_local_path', { connectionId, remotePath, localPath, transferId: tid, - }, NO_REQUEST_DEADLINE); + }, transferId ? NO_REQUEST_DEADLINE : undefined); } finally { unlisten?.(); } @@ -272,6 +278,7 @@ export const sshApi = { remotePath: string, onProgress?: (uploaded: number, total: number) => void, transferId?: string, + isCancelled?: () => boolean, ): Promise<{ wasDirectory: boolean }> { const tid = transferId ?? crypto.randomUUID(); let unlisten: (() => void) | null = null; @@ -289,12 +296,15 @@ export const sshApi = { } try { + if (isCancelled?.()) { + throw new Error('Transfer cancelled'); + } return await api.invoke('remote_upload_from_local_path', { connectionId, localPath, remotePath, transferId: tid, - }, NO_REQUEST_DEADLINE); + }, transferId ? NO_REQUEST_DEADLINE : undefined); } finally { unlisten?.(); } diff --git a/src/web-ui/src/tools/file-system/services/workspaceFileTransfer.test.ts b/src/web-ui/src/tools/file-system/services/workspaceFileTransfer.test.ts index 748c726ebd..0743f449c6 100644 --- a/src/web-ui/src/tools/file-system/services/workspaceFileTransfer.test.ts +++ b/src/web-ui/src/tools/file-system/services/workspaceFileTransfer.test.ts @@ -202,6 +202,21 @@ describe("fixed peer download identity", () => { } expect(chunks).toEqual([1]); }); + + it("does not publish a peer file stopped after its final chunk", async () => { + const requestPeerCommand = vi.fn() + .mockResolvedValueOnce({ resp: "file_info", size: 1 }) + .mockResolvedValueOnce({ resp: "file_chunk", offset: 0, chunk_size: 1, total_size: 1, chunk_base64: "AQ==", revision: "r1" }); + const adapter = { requestPeerCommand } as unknown as Parameters[0]; + const stream = readPeerFileChunks( + adapter, "/workspace/file", vi.fn(), { workspace_path: "/workspace" }, "last-chunk", + ); + expect((await stream.next()).value).toEqual(new Uint8Array([1])); + cancelWorkspaceTransfer("last-chunk"); + await expect(stream.next()).rejects.toThrow( + i18nService.t("panels/files:transfer.cancelled"), + ); + }); }); describe("remote workspace uploads report their cancellable transfer id", () => { @@ -283,4 +298,21 @@ describe("remote workspace uploads report their cancellable transfer id", () => ).rejects.toThrow(/a\.txt/); expect(uploadFromLocalPath).toHaveBeenCalledTimes(1); }); + + it("does not start an upload stopped from its first progress card", async () => { + const { cancelWorkspaceTransfer, uploadLocalPathsToWorkspaceDirectory: upload } = + await loadTransferModule(); + const result = await upload( + ["/local/a.txt"], + "/workspace", + remoteWorkspace, + (state) => { + if (state?.current === 0) cancelWorkspaceTransfer("before-start"); + }, + undefined, + "before-start", + ); + expect(result.successCount).toBe(0); + expect(uploadFromLocalPath).not.toHaveBeenCalled(); + }); }); diff --git a/src/web-ui/src/tools/file-system/services/workspaceFileTransfer.ts b/src/web-ui/src/tools/file-system/services/workspaceFileTransfer.ts index 5ca93f761e..483b75f7a1 100644 --- a/src/web-ui/src/tools/file-system/services/workspaceFileTransfer.ts +++ b/src/web-ui/src/tools/file-system/services/workspaceFileTransfer.ts @@ -233,6 +233,9 @@ export async function* readPeerFileChunks( offset += bytes.byteLength; yield bytes; } + // The sink resumes this generator before publishing the staged file. A stop + // during the last chunk (or an empty file's info request) must still abort. + throwIfCancelled(transferId); return info.size; } @@ -240,15 +243,18 @@ async function collectPeerDirectoryEntries( sourceDirectory: string, destinationDirectory: string, remoteConnectionId?: string, + transferId?: string, ): Promise { const { mkdir } = await import("@tauri-apps/plugin-fs"); const pending = [{ source: sourceDirectory, destination: destinationDirectory }]; const files: PeerDownloadEntry[] = []; while (pending.length > 0) { + throwIfCancelled(transferId); const current = pending.shift()!; await mkdir(current.destination, { recursive: true }); const children = await workspaceAPI.getDirectoryChildren(current.source, remoteConnectionId ?? ''); + throwIfCancelled(transferId); for (const child of children) { if (!isSafePeerTransferEntryName(child.name)) { throw new Error(`Unsafe peer file name: '${child.name}'`); @@ -295,7 +301,7 @@ async function downloadPeerWorkspacePathToDisk( remote_connection_id: isRemoteWorkspace(workspace) ? workspace.connectionId : undefined, }; const entries = isDirectory - ? await collectPeerDirectoryEntries(sourcePath, destinationPath, identity.remote_connection_id) + ? await collectPeerDirectoryEntries(sourcePath, destinationPath, identity.remote_connection_id, transferId) : [{ sourcePath, destinationPath, @@ -309,6 +315,7 @@ async function downloadPeerWorkspacePathToDisk( let smoothedSpeed = 0; for (const entry of entries) { + throwIfCancelled(transferId); const entryStart = bytesTransferred; let entryWritten = 0; let expectedEntrySize = entry.size; @@ -355,6 +362,7 @@ async function downloadPeerWorkspacePathToDisk( bytesTransferred = entryStart + entryWritten; } + throwIfCancelled(transferId); onProgress({ phase: "download", current: Math.max(bytesTransferred, 1), @@ -643,6 +651,7 @@ export async function downloadWorkspaceFileToDisk( indeterminate: true, }); try { + throwIfCancelled(transferId); const peerAdapter = currentPeerAdapter(); if (peerAdapter) { await downloadPeerWorkspacePathToDisk( @@ -699,7 +708,7 @@ export async function downloadWorkspaceFileToDisk( speed: smoothedSpeed, }); } - }, transferId); + }, transferId, () => isWorkspaceTransferCancelled(transferId)); } else { await workspaceAPI.exportLocalFileToPath(filePath, dest, workspace?.id); } @@ -784,6 +793,9 @@ export async function uploadLocalPathsToWorkspaceDirectory( indeterminate: singleItem, ...(transferId ? { transferId } : {}), }); + if (isWorkspaceTransferCancelled(transferId)) { + break; + } try { if (singleItem) { @@ -830,6 +842,7 @@ export async function uploadLocalPathsToWorkspaceDirectory( } }, transferId, + () => isWorkspaceTransferCancelled(transferId), ); successCount += 1; if (uploadResult.wasDirectory) { @@ -844,6 +857,7 @@ export async function uploadLocalPathsToWorkspaceDirectory( destPath, undefined, transferId, + () => isWorkspaceTransferCancelled(transferId), ); successCount += 1; if (uploadResult.wasDirectory) { From c8e0ee44e94663318b0730f72269ff9174d0c425 Mon Sep 17 00:00:00 2001 From: GCWing Date: Mon, 28 Sep 2026 18:28:16 +0800 Subject: [PATCH 3/3] fix(remote-transfer): finish cancelled upload sessions cleanly --- .../src/app/components/panels/FilesPanel.tsx | 65 +++++++++---------- .../file-system/hooks/useWorkspaceFileDrop.ts | 4 +- .../services/workspaceFileTransfer.test.ts | 46 +++++++++---- .../services/workspaceFileTransfer.ts | 7 +- 4 files changed, 71 insertions(+), 51 deletions(-) diff --git a/src/web-ui/src/app/components/panels/FilesPanel.tsx b/src/web-ui/src/app/components/panels/FilesPanel.tsx index d9da386034..a03e0fd98d 100644 --- a/src/web-ui/src/app/components/panels/FilesPanel.tsx +++ b/src/web-ui/src/app/components/panels/FilesPanel.tsx @@ -188,7 +188,6 @@ const FilesPanel: React.FC = ({ const [renamingPath, setRenamingPath] = useState(null); const [transfers, setTransfers] = useState>(new Map()); - const dropTransferIdRef = useRef(null); const [fileDropHighlight, setFileDropHighlight] = useState(false); const [inputDialog, setInputDialog] = useState<{ isOpen: boolean; @@ -212,8 +211,9 @@ const FilesPanel: React.FC = ({ const createTransferProgress = useCallback(() => { const id = crypto.randomUUID(); const onProgress = (state: TransferProgressState | null) => { + const wasStopped = state !== null && cancelledTransferIdsRef.current.has(id); setTransfers((prev) => { - if (state !== null && cancelledTransferIdsRef.current.has(id)) { + if (wasStopped) { return prev; } const next = new Map(prev); @@ -224,6 +224,9 @@ const FilesPanel: React.FC = ({ } return next; }); + if (state === null) { + cancelledTransferIdsRef.current.delete(id); + } }; return { id, onProgress }; }, []); @@ -247,30 +250,23 @@ const FilesPanel: React.FC = ({ * transfer id and reports it with every progress state, so the card and the * cancellable backend transfer stay the same operation. */ - const handleDropProgress = useCallback((state: TransferProgressState | null) => { - const reportedId = state?.transferId; - if (reportedId) { - dropTransferIdRef.current = reportedId; - } + const handleDropProgress = useCallback((state: TransferProgressState | null, transferId: string) => { + const wasStopped = state !== null && cancelledTransferIdsRef.current.has(transferId); setTransfers((prev) => { - if (state !== null && cancelledTransferIdsRef.current.has(dropTransferIdRef.current ?? reportedId ?? '')) { + if (wasStopped) { return prev; } const next = new Map(prev); if (state === null) { - const id = dropTransferIdRef.current; - if (id) { - next.delete(id); - dropTransferIdRef.current = null; - } + next.delete(transferId); } else { - if (!dropTransferIdRef.current) { - dropTransferIdRef.current = reportedId ?? crypto.randomUUID(); - } - next.set(dropTransferIdRef.current, state); + next.set(transferId, state); } return next; }); + if (state === null) { + cancelledTransferIdsRef.current.delete(transferId); + } }, []); const searchLimitNotice = @@ -527,14 +523,11 @@ const FilesPanel: React.FC = ({ id, data.isDirectory, ); - // A stop that raced with completion must not suppress a later failure. - cancelledTransferIdsRef.current.delete(id); } catch (error) { log.error('Failed to download file', error); + const wasStopped = cancelledTransferIdsRef.current.has(id); onProgress(null); - if (cancelledTransferIdsRef.current.has(id)) { - cancelledTransferIdsRef.current.delete(id); - } else { + if (!wasStopped) { notification.error(t('transfer.failed', { error: String(error) })); } } @@ -687,6 +680,13 @@ const FilesPanel: React.FC = ({ id ); + if (result.cancelled && result.failedFiles.length === 0) { + if (result.successCount > 0) { + await loadFileTree(undefined, true); + } + return; + } + if (result.successCount === 0 && result.failedFiles.length === 0) { notification.info(t('notifications.pasteNoFiles')); return; @@ -722,10 +722,9 @@ const FilesPanel: React.FC = ({ } } catch (error) { log.error('Failed to paste files', error); + const wasStopped = cancelledTransferIdsRef.current.has(id); onProgress(null); - if (cancelledTransferIdsRef.current.has(id)) { - cancelledTransferIdsRef.current.delete(id); - } else { + if (!wasStopped) { notification.error(t('notifications.pasteFailed', { count: 1 })); } } @@ -900,13 +899,8 @@ const FilesPanel: React.FC = ({ setFileDropHighlight(overPanel); }, []); - const handleFileDropComplete = useCallback(( - targetDirectory: string, - transferId: string, - ) => { + const handleFileDropComplete = useCallback((targetDirectory: string) => { setFileDropHighlight(false); - // A stop that raced with completion must not suppress a later failure. - cancelledTransferIdsRef.current.delete(transferId); void loadFileTree(workspacePath || '', true); if (workspacePath && !pathsEquivalentFs(targetDirectory, workspacePath)) { expandFolder(targetDirectory, true); @@ -917,13 +911,12 @@ const FilesPanel: React.FC = ({ error: unknown, transferId: string, ) => { - handleDropProgress(null); + const wasStopped = cancelledTransferIdsRef.current.has(transferId); + handleDropProgress(null, transferId); setFileDropHighlight(false); - if (cancelledTransferIdsRef.current.has(transferId)) { - cancelledTransferIdsRef.current.delete(transferId); - return; + if (!wasStopped) { + notification.error(t('transfer.failed', { error: String(error) })); } - notification.error(t('transfer.failed', { error: String(error) })); }, [notification, t, handleDropProgress]); useWorkspaceFileDrop({ diff --git a/src/web-ui/src/tools/file-system/hooks/useWorkspaceFileDrop.ts b/src/web-ui/src/tools/file-system/hooks/useWorkspaceFileDrop.ts index a819f743b9..f15989f6b3 100644 --- a/src/web-ui/src/tools/file-system/hooks/useWorkspaceFileDrop.ts +++ b/src/web-ui/src/tools/file-system/hooks/useWorkspaceFileDrop.ts @@ -17,7 +17,7 @@ export interface UseWorkspaceFileDropOptions { workspacePath?: string; panelRef: RefObject; enabled?: boolean; - onProgress: (state: TransferProgressState | null) => void; + onProgress: (state: TransferProgressState | null, transferId: string) => void; onDragOver?: (overPanel: boolean) => void; onComplete: (targetDirectory: string, transferId: string) => void; onError: (error: unknown, transferId: string) => void; @@ -160,7 +160,7 @@ export function useWorkspaceFileDrop({ paths, targetDir, currentWorkspace, - onProgress, + (state) => onProgress(state, transferId), undefined, transferId ); diff --git a/src/web-ui/src/tools/file-system/services/workspaceFileTransfer.test.ts b/src/web-ui/src/tools/file-system/services/workspaceFileTransfer.test.ts index 0743f449c6..e176f44d14 100644 --- a/src/web-ui/src/tools/file-system/services/workspaceFileTransfer.test.ts +++ b/src/web-ui/src/tools/file-system/services/workspaceFileTransfer.test.ts @@ -278,27 +278,48 @@ describe("remote workspace uploads report their cancellable transfer id", () => it("stops the remaining items of a multi-file upload after a stop request", async () => { const { cancelWorkspaceTransfer, uploadLocalPathsToWorkspaceDirectory: upload } = await loadTransferModule(); - // The first item is stopped while it is being sent, which the backend - // reports as a failure. The remaining items must not start new transfers. + // The first item is stopped while it is being sent. The remaining items + // must not start, and a user stop is not reported as a failed file. uploadFromLocalPath.mockImplementation(async () => { cancelWorkspaceTransfer("transfer-multi"); throw new Error("Transfer cancelled"); }); const states: Array<{ transferId?: string }> = []; - await expect( - upload( - ["/local/a.txt", "/local/b.txt", "/local/c.txt"], - "/workspace", - remoteWorkspace, - (state) => states.push(state ?? {}), - undefined, - "transfer-multi", - ), - ).rejects.toThrow(/a\.txt/); + const result = await upload( + ["/local/a.txt", "/local/b.txt", "/local/c.txt"], + "/workspace", + remoteWorkspace, + (state) => states.push(state ?? {}), + undefined, + "transfer-multi", + ); + expect(result).toMatchObject({ successCount: 0, failedFiles: [], cancelled: true }); expect(uploadFromLocalPath).toHaveBeenCalledTimes(1); }); + it("keeps completed items without treating a later stop as a failure", async () => { + const { cancelWorkspaceTransfer, uploadLocalPathsToWorkspaceDirectory: upload } = + await loadTransferModule(); + uploadFromLocalPath + .mockResolvedValueOnce({ wasDirectory: false }) + .mockImplementationOnce(async () => { + cancelWorkspaceTransfer("transfer-partial"); + throw new Error("Transfer cancelled"); + }); + + const result = await upload( + ["/local/a.txt", "/local/b.txt", "/local/c.txt"], + "/workspace", + remoteWorkspace, + () => undefined, + undefined, + "transfer-partial", + ); + expect(result).toMatchObject({ successCount: 1, failedFiles: [], cancelled: true }); + expect(uploadFromLocalPath).toHaveBeenCalledTimes(2); + }); + it("does not start an upload stopped from its first progress card", async () => { const { cancelWorkspaceTransfer, uploadLocalPathsToWorkspaceDirectory: upload } = await loadTransferModule(); @@ -313,6 +334,7 @@ describe("remote workspace uploads report their cancellable transfer id", () => "before-start", ); expect(result.successCount).toBe(0); + expect(result.cancelled).toBe(true); expect(uploadFromLocalPath).not.toHaveBeenCalled(); }); }); diff --git a/src/web-ui/src/tools/file-system/services/workspaceFileTransfer.ts b/src/web-ui/src/tools/file-system/services/workspaceFileTransfer.ts index 483b75f7a1..04298a1645 100644 --- a/src/web-ui/src/tools/file-system/services/workspaceFileTransfer.ts +++ b/src/web-ui/src/tools/file-system/services/workspaceFileTransfer.ts @@ -48,6 +48,8 @@ export interface WorkspaceTransferResult { successCount: number; directoryCount: number; failedFiles: Array<{ path: string; error: string }>; + /** A user stopped the remaining items; completed items still count. */ + cancelled?: boolean; } export interface UploadToWorkspaceOptions { @@ -865,6 +867,9 @@ export async function uploadLocalPathsToWorkspaceDirectory( } } } catch (error) { + if (isWorkspaceTransferCancelled(transferId)) { + break; + } failedFiles.push({ path: localPath, error: error instanceof Error ? error.message : String(error), @@ -893,7 +898,7 @@ export async function uploadLocalPathsToWorkspaceDirectory( throw new Error(details); } - return { successCount, directoryCount, failedFiles }; + return { successCount, directoryCount, failedFiles, cancelled: wasCancelled }; } // A local paste is not cancellable: the clipboard helper owns it, and no