From c3800461a46c1eeea66291decf70b8d55694b84d Mon Sep 17 00:00:00 2001 From: ddidderr Date: Thu, 21 May 2026 00:07:12 +0200 Subject: [PATCH] fix(peer): delete partial files when a download is cancelled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cancelling an in-flight download via `PeerCommand::CancelDownload` previously torn down the network transfer and cleared `active_downloads`, but left the partial `.eti` archive(s) sitting in the game root forever. The next library scan still picked up the half-written files as a "downloaded" game, and the only escape was the `Remove files` action. This is the symmetric fix to `62ceb06 feat(peer): remove downloaded game files safely`: the cancel path must clean up after itself the same way an explicit remove does. The fix introduces a dedicated `download/storage.rs` module that owns both the existing pre-allocation step (`prepare_game_storage`, moved out of `planning.rs` because pure file I/O has no business sitting next to chunk planning) and a new `discard_cancelled_download` sweep. The orchestrator calls the sweep at every cancellation exit point, immediately after `rollback_version_ini_transaction` so the version sentinel transients are gone before the bulk deletion runs. The sweep deliberately preserves a known set of names so a cancelled update of an installed game does not destroy user-extracted files: - `local/` committed install directory - `.local.installing/`, `.local.backup/` in-flight install transaction state, needed by `install::recover_game_root` on next startup - `.lanspread.json` per-game install intent log - `.softlan_game_installed` external softlan installer marker - `.sync/` external sync tooling Everything else under the game root (the `.eti` archives, any nested payload directories, partial chunk files) is removed, and the game root itself is removed if it ends up empty. The set matches `should_ignore_game_child` in `services/local_monitor.rs` minus the version.ini transients (which the rollback step removes itself just before the discard runs). Tradeoff worth knowing: this does NOT restore the pre-update `version.ini` sentinel. `begin_version_ini_transaction` parks the existing sentinel as `.version.ini.discarded`, and `rollback_version_ini_transaction` deletes that file rather than renaming it back. The user-visible consequence is that cancelling a mid-flight update of an installed game leaves the local install playable but no longer flagged as "downloaded" — the documented "settles as local-only" behaviour now recorded in `crates/lanspread-peer/ARCHITECTURE.md` and `README.md`. Restoring the sentinel on cancel was considered, but it would mean a cancelled update keeps advertising the OLD version as Ready, which is worse than the current outcome. Two unrelated correctness issues that surfaced while threading cancellation through the orchestrator are bundled in here because they belong to the same user-visible "Cancel button works" story: 1. `download_from_peer` now races `connect_to_peer` against `cancel_token.cancelled()` (`download/transport.rs:314-322`). Previously a cancel arriving while QUIC was still in its connect handshake had to wait for the connect timeout to elapse before the cleanup could run. 2. The download task in `handlers.rs` now calls `refresh_local_game_for_ending_operation` on every terminal branch — success-without-install, install-handoff-failure, and the `Err(e)` / cancel branch — before `end_download_operation` clears `active_downloads`. Without this, the UI's settled snapshot on the cancel path could lag behind the actual file system state because the active-operation snapshot was cleared while the discard was still running, leaving a brief window where the card showed the pre-cancel state. What this does NOT fix: a crash (process kill, power loss) during a download still leaves orphan `.eti` files because `recover_download_transients` in `install/transaction.rs` only sweeps the version.ini transients. Closing that gap would mean calling the same discard from startup recovery for any game root whose install intent is None and whose `version.ini` is absent. Tracked in `FINDINGS.md` as a follow-up. Test Plan: - `just clippy && just test` — 102 unit tests pass, no new warnings. - Two new storage tests: - `discard_cancelled_download_removes_peer_owned_payload` exercises the fresh-download cancel (no `local/`, root sweeps clean). - `discard_cancelled_download_preserves_local_install_state` exercises the update cancel (`local/`, `.lanspread.json`, `.local.backup/` survive; `version.ini` and `.eti` go away). - Manual GUI smoke (operator): start a fresh download of a multi-archive game from a peer, click Cancel from the detail modal while the progress bar is between 5% and 95%. Expect the game root to be empty (or absent) afterwards and no orphan `.eti` files. Repeat against an installed game by clicking Update, then Cancel mid-download; expect `local/` contents intact and the card to drop back to Play (or Update if the newer-version peer is still around). - `lanspread-peer-cli` has no `cancel` command yet, so the headless `PEER_CLI_SCENARIOS.md` matrix does not cover this end-to-end. Adding a CLI cancel command + scenario is the natural follow-up. Refs: 62ceb06 (feat(peer): remove downloaded game files safely) Refs: b7df2de (fix(download): emit failure events on early-returns and update UI transition) --- crates/lanspread-peer/ARCHITECTURE.md | 3 + crates/lanspread-peer/README.md | 10 +- crates/lanspread-peer/src/download/mod.rs | 1 + .../src/download/orchestrator.rs | 35 ++- .../lanspread-peer/src/download/planning.rs | 75 +----- crates/lanspread-peer/src/download/storage.rs | 227 ++++++++++++++++++ .../lanspread-peer/src/download/transport.rs | 7 +- crates/lanspread-peer/src/handlers.rs | 49 ++-- 8 files changed, 300 insertions(+), 107 deletions(-) create mode 100644 crates/lanspread-peer/src/download/storage.rs diff --git a/crates/lanspread-peer/ARCHITECTURE.md b/crates/lanspread-peer/ARCHITECTURE.md index 37c04d1..396c6f3 100644 --- a/crates/lanspread-peer/ARCHITECTURE.md +++ b/crates/lanspread-peer/ARCHITECTURE.md @@ -152,6 +152,9 @@ Most scans become O(number of game dirs), with full recursion only when needed. active for that ID, and the root-level `version.ini` sentinel exists. - `local/` paths are never served, even if a stale or malicious manifest request asks for them. +- Cancelling a download discards the peer-owned root download payload and + scratch sentinel files. `local/` and install transaction metadata are + preserved, so a cancelled update of an installed game settles as local-only. ## Fault tolerance rules diff --git a/crates/lanspread-peer/README.md b/crates/lanspread-peer/README.md index 27f7c0c..32ff7ef 100644 --- a/crates/lanspread-peer/README.md +++ b/crates/lanspread-peer/README.md @@ -77,15 +77,17 @@ When the UI asks to download a game: different regions of the same file. 5. `version.ini` chunks are buffered in memory and committed last via `.version.ini.tmp` followed by an atomic rename. Failures are accumulated and - retried (up to `MAX_RETRY_COUNT`) via `retry_failed_chunks`; failed or - cancelled downloads sweep `.version.ini.tmp` and `.version.ini.discarded` - without restoring the previous sentinel. + retried (up to `MAX_RETRY_COUNT`) via `retry_failed_chunks`; failed downloads + sweep `.version.ini.tmp` and `.version.ini.discarded` without restoring the + previous sentinel. Cancelled downloads also discard the peer-owned download + payload while preserving `local/` and install transaction metadata. 6. After a successful sentinel commit, `PeerEvent::DownloadGameFilesFinished` is emitted and the peer auto-runs the install transaction. `PeerCommand::CancelDownload` cancels the tracked download token for an active transfer. The transfer task remains responsible for clearing `active_operations`, -so the UI continues to treat active-operation snapshots as the single source of +discarding partial payload files, and refreshing the settled local snapshot, so +the UI continues to treat active-operation snapshots as the single source of truth for whether a download is still running. ### Install Transactions diff --git a/crates/lanspread-peer/src/download/mod.rs b/crates/lanspread-peer/src/download/mod.rs index d468a56..a91bb03 100644 --- a/crates/lanspread-peer/src/download/mod.rs +++ b/crates/lanspread-peer/src/download/mod.rs @@ -4,6 +4,7 @@ mod orchestrator; mod planning; mod progress; mod retry; +mod storage; mod transport; mod version_ini; diff --git a/crates/lanspread-peer/src/download/orchestrator.rs b/crates/lanspread-peer/src/download/orchestrator.rs index dc38479..18f4064 100644 --- a/crates/lanspread-peer/src/download/orchestrator.rs +++ b/crates/lanspread-peer/src/download/orchestrator.rs @@ -10,15 +10,10 @@ use tokio::sync::mpsc::UnboundedSender; use tokio_util::sync::CancellationToken; use super::{ - planning::{ - ChunkDownloadResult, - DownloadChunk, - build_peer_plans, - extract_version_descriptor, - prepare_game_storage, - }, + planning::{ChunkDownloadResult, DownloadChunk, build_peer_plans, extract_version_descriptor}, progress::{DownloadProgressTracker, sample_download_progress}, retry::{RetryContext, retry_failed_chunks}, + storage::{discard_cancelled_download, prepare_game_storage}, transport::download_from_peer, version_ini::{ VersionIniBuffer, @@ -67,8 +62,17 @@ pub async fn download_game_files( })?; return Err(err); } + if cancel_token.is_cancelled() { + rollback_version_ini_transaction(&game_root).await; + discard_cancelled_download_best_effort(&games_folder, game_id).await; + eyre::bail!("download cancelled for game {game_id}"); + } if let Err(err) = prepare_game_storage(&games_folder, &transfer_descs).await { rollback_version_ini_transaction(&game_root).await; + if cancel_token.is_cancelled() { + discard_cancelled_download_best_effort(&games_folder, game_id).await; + eyre::bail!("download cancelled for game {game_id}"); + } tx_notify_ui.send(PeerEvent::DownloadGameFilesFailed { id: game_id.to_string(), })?; @@ -76,6 +80,7 @@ pub async fn download_game_files( } if cancel_token.is_cancelled() { rollback_version_ini_transaction(&game_root).await; + discard_cancelled_download_best_effort(&games_folder, game_id).await; eyre::bail!("download cancelled for game {game_id}"); } @@ -104,7 +109,9 @@ pub async fn download_game_files( if let Err(err) = transfer_result { rollback_version_ini_transaction(&game_root).await; - if !cancel_token.is_cancelled() { + if cancel_token.is_cancelled() { + discard_cancelled_download_best_effort(&games_folder, game_id).await; + } else { tx_notify_ui.send(PeerEvent::DownloadGameFilesFailed { id: game_id.to_string(), })?; @@ -112,6 +119,12 @@ pub async fn download_game_files( return Err(err); } + if cancel_token.is_cancelled() { + rollback_version_ini_transaction(&game_root).await; + discard_cancelled_download_best_effort(&games_folder, game_id).await; + eyre::bail!("download cancelled for game {game_id}"); + } + if let Err(err) = commit_version_ini_buffer(&game_root, &version_buffer).await { rollback_version_ini_transaction(&game_root).await; tx_notify_ui.send(PeerEvent::DownloadGameFilesFailed { @@ -126,6 +139,12 @@ pub async fn download_game_files( Ok(()) } +async fn discard_cancelled_download_best_effort(games_folder: &Path, game_id: &str) { + if let Err(err) = discard_cancelled_download(games_folder, game_id).await { + log::warn!("Failed to discard cancelled download payload for {game_id}: {err}"); + } +} + struct TransferContext<'a> { game_id: &'a str, games_folder: &'a Path, diff --git a/crates/lanspread-peer/src/download/planning.rs b/crates/lanspread-peer/src/download/planning.rs index 417f412..196b0e8 100644 --- a/crates/lanspread-peer/src/download/planning.rs +++ b/crates/lanspread-peer/src/download/planning.rs @@ -1,9 +1,9 @@ -use std::{collections::HashMap, net::SocketAddr, path::Path}; +use std::{collections::HashMap, net::SocketAddr}; use lanspread_db::db::GameFileDescription; -use tokio::{fs::OpenOptions, sync::mpsc::UnboundedSender}; +use tokio::sync::mpsc::UnboundedSender; -use crate::{PeerEvent, config::CHUNK_SIZE, path_validation::validate_game_file_path}; +use crate::{PeerEvent, config::CHUNK_SIZE}; /// Represents a chunk of a file to be downloaded. #[derive(Debug, Clone)] @@ -60,56 +60,6 @@ pub(super) fn extract_version_descriptor( Ok((version_desc, transfer_descs)) } -/// Prepares storage for game files by creating directories and pre-allocating files. -pub(super) async fn prepare_game_storage( - games_folder: &Path, - file_descs: &[GameFileDescription], -) -> eyre::Result<()> { - for desc in file_descs { - if desc.is_version_ini() { - continue; - } - - // Validate the path to prevent directory traversal - let validated_path = validate_game_file_path(games_folder, &desc.relative_path)?; - - if desc.is_dir { - tokio::fs::create_dir_all(&validated_path).await?; - } else { - if let Some(parent) = validated_path.parent() { - tokio::fs::create_dir_all(parent).await?; - } - - // Create and pre-allocate the file with the expected size - let file = OpenOptions::new() - .create(true) - .truncate(true) - .write(true) - .open(&validated_path) - .await?; - - // Pre-allocate the file with the expected size - let size = desc.size; - if let Err(e) = file.set_len(size).await { - log::warn!( - "Failed to pre-allocate file {} (size: {}): {}", - desc.relative_path, - size, - e - ); - // Continue without pre-allocation - the file will grow as chunks are written - } else { - log::debug!( - "Pre-allocated file {} with {} bytes", - desc.relative_path, - size - ); - } - } - } - Ok(()) -} - /// Resolves which peers have a specific file. pub(super) fn resolve_file_peers<'a>( relative_path: &str, @@ -206,8 +156,6 @@ mod tests { use lanspread_db::db::GameFileDescription; use super::*; - use crate::test_support::TempDir; - fn loopback_addr(port: u16) -> SocketAddr { SocketAddr::from(([127, 0, 0, 1], port)) } @@ -346,23 +294,6 @@ mod tests { } } - #[tokio::test] - async fn prepare_game_storage_skips_version_ini_sentinel() { - let temp = TempDir::new("lanspread-download"); - let descs = vec![GameFileDescription { - game_id: "game".to_string(), - relative_path: "game/version.ini".to_string(), - is_dir: false, - size: 8, - }]; - - prepare_game_storage(temp.path(), &descs) - .await - .expect("storage preparation should succeed"); - - assert!(!temp.path().join("game").join("version.ini").exists()); - } - #[test] fn version_descriptor_extraction_keeps_nested_decoy_in_transfer_list() { let (tx, _rx) = tokio::sync::mpsc::unbounded_channel(); diff --git a/crates/lanspread-peer/src/download/storage.rs b/crates/lanspread-peer/src/download/storage.rs new file mode 100644 index 0000000..20c69ec --- /dev/null +++ b/crates/lanspread-peer/src/download/storage.rs @@ -0,0 +1,227 @@ +use std::{io::ErrorKind, path::Path}; + +use lanspread_db::db::GameFileDescription; +use tokio::fs::OpenOptions; + +use crate::{local_games::is_local_dir_name, path_validation::validate_game_file_path}; + +const INTENT_LOG_FILE: &str = ".lanspread.json"; +const SOFTLAN_INSTALL_MARKER: &str = ".softlan_game_installed"; +const SYNC_DIR: &str = ".sync"; + +/// Prepares storage for game files by creating directories and pre-allocating files. +pub(super) async fn prepare_game_storage( + games_folder: &Path, + file_descs: &[GameFileDescription], +) -> eyre::Result<()> { + for desc in file_descs { + if desc.is_version_ini() { + continue; + } + + let validated_path = validate_game_file_path(games_folder, &desc.relative_path)?; + + if desc.is_dir { + tokio::fs::create_dir_all(&validated_path).await?; + } else { + if let Some(parent) = validated_path.parent() { + tokio::fs::create_dir_all(parent).await?; + } + + let file = OpenOptions::new() + .create(true) + .truncate(true) + .write(true) + .open(&validated_path) + .await?; + + let size = desc.size; + if let Err(e) = file.set_len(size).await { + log::warn!( + "Failed to pre-allocate file {} (size: {}): {}", + desc.relative_path, + size, + e + ); + } else { + log::debug!( + "Pre-allocated file {} with {} bytes", + desc.relative_path, + size + ); + } + } + } + Ok(()) +} + +/// Discards the peer-owned downloaded payload after a cancelled transfer. +/// +/// Downloads own the root archive/cache files, but not `local/` or install +/// transaction metadata. Preserving those paths lets a cancelled update settle +/// as a local-only install instead of deleting user-owned extracted files. +pub(super) async fn discard_cancelled_download( + games_folder: &Path, + game_id: &str, +) -> eyre::Result<()> { + let game_root = games_folder.join(game_id); + let Some(metadata) = symlink_metadata_if_exists(&game_root).await? else { + return Ok(()); + }; + + if metadata.file_type().is_symlink() { + eyre::bail!( + "refusing to discard cancelled download through symlink root {}", + game_root.display() + ); + } + if !metadata.is_dir() { + eyre::bail!( + "refusing to discard cancelled download from non-directory root {}", + game_root.display() + ); + } + + let mut entries = tokio::fs::read_dir(&game_root).await?; + while let Some(entry) = entries.next_entry().await? { + let name = entry.file_name(); + if name + .to_str() + .is_some_and(should_preserve_on_download_discard) + { + continue; + } + + remove_entry(&entry.path()).await?; + } + + remove_dir_if_empty(&game_root).await +} + +fn should_preserve_on_download_discard(name: &str) -> bool { + is_local_dir_name(name) + || name.starts_with(".local.") + || name == INTENT_LOG_FILE + || name == SOFTLAN_INSTALL_MARKER + || name == SYNC_DIR +} + +async fn remove_entry(path: &Path) -> eyre::Result<()> { + let Some(metadata) = symlink_metadata_if_exists(path).await? else { + return Ok(()); + }; + + if metadata.file_type().is_symlink() || metadata.is_file() { + remove_file_if_exists(path).await + } else if metadata.is_dir() { + tokio::fs::remove_dir_all(path).await?; + Ok(()) + } else { + remove_file_if_exists(path).await + } +} + +async fn remove_file_if_exists(path: &Path) -> eyre::Result<()> { + match tokio::fs::remove_file(path).await { + Ok(()) => Ok(()), + Err(err) if err.kind() == ErrorKind::NotFound => Ok(()), + Err(err) => Err(err.into()), + } +} + +async fn remove_dir_if_empty(path: &Path) -> eyre::Result<()> { + match tokio::fs::remove_dir(path).await { + Ok(()) => Ok(()), + Err(err) + if matches!( + err.kind(), + ErrorKind::NotFound | ErrorKind::DirectoryNotEmpty + ) => + { + Ok(()) + } + Err(err) => Err(err.into()), + } +} + +async fn symlink_metadata_if_exists(path: &Path) -> eyre::Result> { + match tokio::fs::symlink_metadata(path).await { + Ok(metadata) => Ok(Some(metadata)), + Err(err) if err.kind() == ErrorKind::NotFound => Ok(None), + Err(err) => Err(err.into()), + } +} + +#[cfg(test)] +mod tests { + use lanspread_db::db::GameFileDescription; + + use super::*; + use crate::test_support::TempDir; + + fn write_file(path: &Path, bytes: &[u8]) { + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent).expect("parent dir should be created"); + } + std::fs::write(path, bytes).expect("file should be written"); + } + + #[tokio::test] + async fn prepare_game_storage_skips_version_ini_sentinel() { + let temp = TempDir::new("lanspread-download"); + let descs = vec![GameFileDescription { + game_id: "game".to_string(), + relative_path: "game/version.ini".to_string(), + is_dir: false, + size: 8, + }]; + + prepare_game_storage(temp.path(), &descs) + .await + .expect("storage preparation should succeed"); + + assert!(!temp.path().join("game").join("version.ini").exists()); + } + + #[tokio::test] + async fn discard_cancelled_download_removes_peer_owned_payload() { + let temp = TempDir::new("lanspread-download-discard"); + let root = temp.game_root(); + write_file(&root.join("version.ini"), b"20250101"); + write_file(&root.join(".version.ini.tmp"), b"tmp"); + write_file(&root.join(".version.ini.discarded"), b"old"); + write_file(&root.join("archive.eti"), b"partial"); + write_file(&root.join("nested").join("payload.bin"), b"partial"); + + discard_cancelled_download(temp.path(), "game") + .await + .expect("cancelled payload should be discarded"); + + assert!(!root.exists()); + } + + #[tokio::test] + async fn discard_cancelled_download_preserves_local_install_state() { + let temp = TempDir::new("lanspread-download-discard-local"); + let root = temp.game_root(); + write_file(&root.join("version.ini"), b"20250101"); + write_file(&root.join("archive.eti"), b"partial"); + write_file(&root.join("local").join("save.dat"), b"user-data"); + write_file(&root.join(".lanspread.json"), b"{\"intent\":\"None\"}"); + write_file(&root.join(".local.backup").join(".lanspread_owned"), b""); + + discard_cancelled_download(temp.path(), "game") + .await + .expect("cancelled payload should be discarded"); + + assert!(!root.join("version.ini").exists()); + assert!(!root.join("archive.eti").exists()); + assert_eq!( + std::fs::read(root.join("local").join("save.dat")) + .expect("local install should remain"), + b"user-data" + ); + assert!(root.join(".lanspread.json").is_file()); + assert!(root.join(".local.backup").is_dir()); + } +} diff --git a/crates/lanspread-peer/src/download/transport.rs b/crates/lanspread-peer/src/download/transport.rs index 36a94c3..8329e1b 100644 --- a/crates/lanspread-peer/src/download/transport.rs +++ b/crates/lanspread-peer/src/download/transport.rs @@ -311,7 +311,12 @@ pub(super) async fn download_from_peer( ensure_download_not_cancelled(cancel_token, game_id)?; - let mut conn = match connect_to_peer(peer_addr).await { + let mut conn = match tokio::select! { + () = cancel_token.cancelled() => { + eyre::bail!("download cancelled for game {game_id}"); + } + result = connect_to_peer(peer_addr) => result, + } { Ok(conn) => conn, Err(err) => return Ok(failed_plan_results(plan, peer_addr, err)), }; diff --git a/crates/lanspread-peer/src/handlers.rs b/crates/lanspread-peer/src/handlers.rs index 15370a9..8a0ffcd 100644 --- a/crates/lanspread-peer/src/handlers.rs +++ b/crates/lanspread-peer/src/handlers.rs @@ -330,6 +330,15 @@ pub async fn handle_download_game_files_command( let Some(prepared) = prepare_install_operation(&ctx_clone, &tx_notify_ui_clone, &download_id).await else { + if let Err(err) = refresh_local_game_for_ending_operation( + &ctx_clone, + &tx_notify_ui_clone, + &download_id, + ) + .await + { + log::error!("Failed to refresh local library after download: {err}"); + } end_download_operation(&ctx_clone, &tx_notify_ui_clone, &download_id).await; download_state_guard.disarm(); return; @@ -356,16 +365,31 @@ pub async fn handle_download_game_files_command( clear_active_download(&ctx_clone, &download_id).await; } } else { - end_download_operation(&ctx_clone, &tx_notify_ui_clone, &download_id).await; - if let Err(err) = - refresh_local_game(&ctx_clone, &tx_notify_ui_clone, &download_id).await + if let Err(err) = refresh_local_game_for_ending_operation( + &ctx_clone, + &tx_notify_ui_clone, + &download_id, + ) + .await { log::error!("Failed to refresh local library after download: {err}"); } + end_download_operation(&ctx_clone, &tx_notify_ui_clone, &download_id).await; } download_state_guard.disarm(); } Err(e) => { + if let Err(refresh_err) = refresh_local_game_for_ending_operation( + &ctx_clone, + &tx_notify_ui_clone, + &download_id, + ) + .await + { + log::error!( + "Failed to refresh local library after download failure: {refresh_err}" + ); + } end_download_operation(&ctx_clone, &tx_notify_ui_clone, &download_id).await; download_state_guard.disarm(); log::error!("Download failed for {download_id}: {e}"); @@ -851,25 +875,6 @@ async fn scan_and_announce_local_library( Ok(()) } -async fn refresh_local_game( - ctx: &Ctx, - tx_notify_ui: &UnboundedSender, - id: &str, -) -> eyre::Result<()> { - let game_dir = { ctx.game_dir.read().await.clone() }; - let catalog = ctx.catalog.read().await.clone(); - let scan = rescan_local_game(&game_dir, &catalog, id).await?; - update_and_announce_games_with_policy( - ctx, - tx_notify_ui, - scan, - LocalLibraryEventPolicy::OnChange, - None, - ) - .await; - Ok(()) -} - /// Refreshes the game whose operation has completed before clearing its /// active-operation snapshot, while preserving freeze behavior for other games. async fn refresh_local_game_for_ending_operation(