ddidderr/lanspread
1
0
Fork 0
Code Issues 7 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(peer): delete partial files when a download is cancelled

Browse Source 
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)
This commit is contained in:
ddidderr
2026-05-21 00:07:12 +02:00
parent 47e2bbd454
commit c3800461a4
8 changed files with 300 additions and 107 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/lanspread-peer/src/download/mod.rs
+1
View File
@@ -4,6 +4,7 @@ mod orchestrator;
mod planning;
mod progress;
mod retry;
mod storage;
mod transport;
mod version_ini;
crates/lanspread-peer/src/download/orchestrator.rs
+27 -8
View File
@@ -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,
crates/lanspread-peer/src/download/planning.rs
+3 -72
View File
@@ -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();
crates/lanspread-peer/src/download/storage.rs
+227
View File
@@ -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<Option<std::fs::Metadata>> {
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());
}
}
crates/lanspread-peer/src/download/transport.rs
+6 -1
View File
@@ -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)),
};
crates/lanspread-peer/src/handlers.rs
+27 -22
View File
@@ -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<PeerEvent>,
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(