diff --git a/Cargo.lock b/Cargo.lock index 9e6ddd9..a843d1a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2214,6 +2214,7 @@ dependencies = [ "lanspread-peer", "log", "mimalloc", + "serde", "tauri", "tauri-build", "tauri-plugin-dialog", diff --git a/FOLLOW_UP_2.md b/FOLLOW_UP_2.md index 38810e5..8ed0068 100644 --- a/FOLLOW_UP_2.md +++ b/FOLLOW_UP_2.md @@ -1,6 +1,8 @@ # Follow-up Plan #2 -State of FOLLOW_UP_PLAN.md after two implementation rounds. Items here are what's still open. +State of FOLLOW_UP_PLAN.md after two implementation rounds. Items here are +what's still open, plus notes for follow-up items completed after this file was +created. ## Context for next time @@ -8,14 +10,16 @@ Branch `p2p-codex-muenchhausen` has uncommitted work that completes the bulk of The OperationGuard ordering fix in `handlers.rs` is the structurally most important change: install/update/uninstall now drop the guard _before_ the finish event and `refresh_local_game`, so peers see settled state in the next announcement instead of waiting for a scan tick. Tests `*_refreshes_settled_state_after_guard_release` pin this. +## Completed after this file was created + +- Tauri-side `active_operations` reconciliation: `PeerEvent::LocalGamesUpdated` + now carries an authoritative active-operation snapshot, the Tauri bridge + replaces its UI operation map from that snapshot, and the React game-list + merge uses the snapshot to clear stale download/install/update/uninstall + spinners even when a begin/finish/fail event was missed. + ## Still open -### Correctness / runtime - -#### 1. Tauri-side `active_operations` has no reconciliation - -If a `PeerEvent::InstallGameFinished` / `…Failed` is dropped, the UI is stuck "installing" until app restart. Include an in-progress snapshot in `PeerEvent::LocalGamesUpdated` so the UI recomputes from authoritative state instead of accumulating from event history. (Was `#10` in original plan, deferred both rounds.) - ### Tests still missing #### 2. Install-recovery matrix (~10 rows) diff --git a/crates/lanspread-peer/src/handlers.rs b/crates/lanspread-peer/src/handlers.rs index 174a0a4..8486141 100644 --- a/crates/lanspread-peer/src/handlers.rs +++ b/crates/lanspread-peer/src/handlers.rs @@ -11,6 +11,8 @@ use lanspread_db::db::{GameDB, GameFileDescription}; use tokio::sync::{RwLock, mpsc::UnboundedSender}; use crate::{ + ActiveOperation, + ActiveOperationKind, InstallOperation, PeerEvent, context::{Ctx, OperationGuard, OperationKind}, @@ -553,20 +555,14 @@ pub async fn update_and_announce_games( revision, } = scan; - let active_ids = ctx - .active_operations - .read() - .await - .keys() - .cloned() - .collect::>(); - if !active_ids.is_empty() { + let active_operations = active_operation_snapshot(ctx).await; + if !active_operations.is_empty() { let previous = ctx.local_library.read().await.games.clone(); - for id in active_ids { - if let Some(summary) = previous.get(&id) { - summaries.insert(id, summary.clone()); + for id in active_operations.iter().map(|operation| &operation.id) { + if let Some(summary) = previous.get(id) { + summaries.insert(id.clone(), summary.clone()); } else { - summaries.remove(&id); + summaries.remove(id); } } game_db = GameDB::from(summaries.values().map(game_from_summary).collect()); @@ -577,10 +573,6 @@ pub async fn update_and_announce_games( library_guard.update_from_scan(summaries, revision) }; - let Some(delta) = delta else { - return; - }; - { let mut db_guard = ctx.local_game_db.write().await; *db_guard = Some(game_db.clone()); @@ -588,10 +580,17 @@ pub async fn update_and_announce_games( let all_games = game_db.all_games().into_iter().cloned().collect::>(); - if let Err(e) = tx_notify_ui.send(PeerEvent::LocalGamesUpdated(all_games.clone())) { + if let Err(e) = tx_notify_ui.send(PeerEvent::LocalGamesUpdated { + games: all_games.clone(), + active_operations, + }) { log::error!("Failed to send LocalGamesUpdated event: {e}"); } + let Some(delta) = delta else { + return; + }; + let peer_targets = { let db = ctx.peer_game_db.read().await; db.peer_identities() @@ -625,6 +624,28 @@ pub async fn update_and_announce_games( } } +async fn active_operation_snapshot(ctx: &Ctx) -> Vec { + let active_operations = ctx.active_operations.read().await; + let mut snapshot = active_operations + .iter() + .map(|(id, operation)| ActiveOperation { + id: id.clone(), + operation: active_operation_kind(*operation), + }) + .collect::>(); + snapshot.sort_by(|left, right| left.id.cmp(&right.id)); + snapshot +} + +fn active_operation_kind(operation: OperationKind) -> ActiveOperationKind { + match operation { + OperationKind::Downloading => ActiveOperationKind::Downloading, + OperationKind::Installing => ActiveOperationKind::Installing, + OperationKind::Updating => ActiveOperationKind::Updating, + OperationKind::Uninstalling => ActiveOperationKind::Uninstalling, + } +} + #[cfg(test)] mod tests { use std::{ @@ -710,9 +731,17 @@ mod tests { } fn assert_local_update(event: PeerEvent, installed: bool, downloaded: bool) { - let PeerEvent::LocalGamesUpdated(games) = event else { + let PeerEvent::LocalGamesUpdated { + games, + active_operations, + } = event + else { panic!("expected LocalGamesUpdated"); }; + assert!( + active_operations.is_empty(), + "settled local update should not report active operations" + ); let game = games .iter() .find(|game| game.id == "game") @@ -721,6 +750,87 @@ mod tests { assert_eq!(game.downloaded, downloaded); } + #[tokio::test] + async fn local_games_update_reports_authoritative_active_operations() { + let temp = TempDir::new("lanspread-handler-active-snapshot"); + let root = temp.game_root(); + write_file(&root.join("version.ini"), b"20250101"); + write_file(&root.join("game.eti"), b"archive"); + + let ctx = test_ctx(temp.path().to_path_buf()); + ctx.active_operations + .write() + .await + .insert("game".to_string(), OperationKind::Installing); + let (tx, mut rx) = mpsc::unbounded_channel(); + let catalog = ctx.catalog.read().await.clone(); + let scan = scan_local_library(temp.path(), &catalog) + .await + .expect("scan should succeed"); + + update_and_announce_games(&ctx, &tx, scan).await; + + let PeerEvent::LocalGamesUpdated { + games, + active_operations, + } = recv_event(&mut rx).await + else { + panic!("expected LocalGamesUpdated"); + }; + assert!( + games.is_empty(), + "active game should keep its previous announced state" + ); + assert_eq!( + active_operations, + vec![ActiveOperation { + id: "game".to_string(), + operation: ActiveOperationKind::Installing, + }] + ); + } + + #[tokio::test] + async fn unchanged_scan_still_reports_active_operation_snapshot() { + let temp = TempDir::new("lanspread-handler-active-unchanged"); + let root = temp.game_root(); + write_file(&root.join("version.ini"), b"20250101"); + write_file(&root.join("game.eti"), b"archive"); + + let ctx = test_ctx(temp.path().to_path_buf()); + let (tx, mut rx) = mpsc::unbounded_channel(); + let catalog = ctx.catalog.read().await.clone(); + + let scan = scan_local_library(temp.path(), &catalog) + .await + .expect("first scan should succeed"); + update_and_announce_games(&ctx, &tx, scan).await; + assert_local_update(recv_event(&mut rx).await, false, true); + + ctx.active_operations + .write() + .await + .insert("game".to_string(), OperationKind::Updating); + let scan = scan_local_library(temp.path(), &catalog) + .await + .expect("second scan should succeed"); + update_and_announce_games(&ctx, &tx, scan).await; + + let PeerEvent::LocalGamesUpdated { + active_operations, .. + } = recv_event(&mut rx).await + else { + panic!("expected LocalGamesUpdated"); + }; + assert_eq!( + active_operations, + vec![ActiveOperation { + id: "game".to_string(), + operation: ActiveOperationKind::Updating, + }] + ); + } + #[tokio::test] async fn install_refreshes_settled_state_after_guard_release() { let temp = TempDir::new("lanspread-handler-install"); diff --git a/crates/lanspread-peer/src/lib.rs b/crates/lanspread-peer/src/lib.rs index b1f26e4..bb9be55 100644 --- a/crates/lanspread-peer/src/lib.rs +++ b/crates/lanspread-peer/src/lib.rs @@ -111,8 +111,11 @@ pub enum PeerEvent { PeerLost(SocketAddr), /// The total peer count has changed. PeerCountUpdated(usize), - /// Local games have been updated. - LocalGamesUpdated(Vec), + /// Local games have been scanned, with authoritative in-progress work. + LocalGamesUpdated { + games: Vec, + active_operations: Vec, + }, /// A required peer runtime component failed. RuntimeFailed { component: PeerRuntimeComponent, @@ -144,6 +147,26 @@ pub enum InstallOperation { Updating, } +/// In-progress operation snapshot attached to local library updates. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct ActiveOperation { + pub id: String, + pub operation: ActiveOperationKind, +} + +/// Operation kinds visible to UI reconciliation. +#[derive(Clone, Copy, Debug, PartialEq, Eq, strum::IntoStaticStr)] +pub enum ActiveOperationKind { + /// Downloading or replacing archive files. + Downloading, + /// Extracting into a previously uninstalled game root. + Installing, + /// Replacing an existing `local/` install. + Updating, + /// Removing an existing `local/` install. + Uninstalling, +} + /// Commands sent to the peer system from the UI. #[derive(Clone, Debug)] pub enum PeerCommand { diff --git a/crates/lanspread-tauri-deno-ts/src-tauri/Cargo.toml b/crates/lanspread-tauri-deno-ts/src-tauri/Cargo.toml index 6f7bc93..551ae01 100644 --- a/crates/lanspread-tauri-deno-ts/src-tauri/Cargo.toml +++ b/crates/lanspread-tauri-deno-ts/src-tauri/Cargo.toml @@ -13,7 +13,7 @@ edition = "2024" # This seems to be only an issue on Windows, see https://github.com/rust-lang/cargo/issues/8519 name = "lanspread_tauri_deno_ts_lib" crate-type = ["staticlib", "cdylib", "rlib"] -test = false +test = true doctest = false [lints.clippy] @@ -36,6 +36,7 @@ base64 = { workspace = true } eyre = { workspace = true } log = { workspace = true } mimalloc = { workspace = true } +serde = { workspace = true } tauri = { workspace = true } tauri-plugin-log = { workspace = true } tauri-plugin-shell = { workspace = true } diff --git a/crates/lanspread-tauri-deno-ts/src-tauri/src/lib.rs b/crates/lanspread-tauri-deno-ts/src-tauri/src/lib.rs index 59f3c00..e28cc42 100644 --- a/crates/lanspread-tauri-deno-ts/src-tauri/src/lib.rs +++ b/crates/lanspread-tauri-deno-ts/src-tauri/src/lib.rs @@ -17,6 +17,8 @@ use lanspread_db::db::{ GameFileDescription, }; use lanspread_peer::{ + ActiveOperation, + ActiveOperationKind, InstallOperation, PeerCommand, PeerEvent, @@ -49,7 +51,7 @@ struct LanSpreadState { struct PeerEventTx(UnboundedSender); -#[derive(Clone, Copy, Debug)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, serde::Serialize)] enum UiOperationKind { Downloading, Installing, @@ -57,6 +59,18 @@ enum UiOperationKind { Uninstalling, } +#[derive(Clone, Debug, PartialEq, Eq, serde::Serialize)] +struct UiActiveOperation { + id: String, + operation: UiOperationKind, +} + +#[derive(Clone, Debug, serde::Serialize)] +struct GamesListPayload { + games: Vec, + active_operations: Vec, +} + struct SidecarUnpacker { app_handle: AppHandle, } @@ -527,13 +541,59 @@ async fn refresh_games_list(app_handle: &AppHandle) { drop(game_db); - if let Err(e) = app_handle.emit("games-list-updated", Some(games_to_emit)) { + let active_operations = { + let active_operations = state.active_operations.read().await; + ui_active_operations_from_map(&active_operations) + }; + + let payload = GamesListPayload { + games: games_to_emit, + active_operations, + }; + + if let Err(e) = app_handle.emit("games-list-updated", Some(payload)) { log::error!("Failed to emit games-list-updated event: {e}"); } else { log::info!("Emitted games-list-updated event"); } } +fn ui_active_operations_from_map( + active_operations: &HashMap, +) -> Vec { + let mut snapshot = active_operations + .iter() + .map(|(id, operation)| UiActiveOperation { + id: id.clone(), + operation: *operation, + }) + .collect::>(); + snapshot.sort_by(|left, right| left.id.cmp(&right.id)); + snapshot +} + +fn reconcile_active_operations( + active_operations: &mut HashMap, + snapshot: &[ActiveOperation], +) { + active_operations.clear(); + active_operations.extend(snapshot.iter().map(|operation| { + ( + operation.id.clone(), + ui_operation_from_peer(operation.operation), + ) + })); +} + +fn ui_operation_from_peer(operation: ActiveOperationKind) -> UiOperationKind { + match operation { + ActiveOperationKind::Downloading => UiOperationKind::Downloading, + ActiveOperationKind::Installing => UiOperationKind::Installing, + ActiveOperationKind::Updating => UiOperationKind::Updating, + ActiveOperationKind::Uninstalling => UiOperationKind::Uninstalling, + } +} + #[tauri::command] async fn update_game_directory(app_handle: tauri::AppHandle, path: String) -> tauri::Result<()> { log::info!("update_game_directory: {path}"); @@ -816,8 +876,16 @@ async fn handle_peer_event(app_handle: &AppHandle, event: PeerEvent) { log::info!("PeerEvent::ListGames received"); update_game_db(games, app_handle.clone()).await; } - PeerEvent::LocalGamesUpdated(local_games) => { + PeerEvent::LocalGamesUpdated { + games: local_games, + active_operations, + } => { log::info!("PeerEvent::LocalGamesUpdated received"); + { + let state = app_handle.state::(); + let mut ui_active_operations = state.active_operations.write().await; + reconcile_active_operations(&mut ui_active_operations, &active_operations); + } update_local_games_in_db(local_games, app_handle.clone()).await; } PeerEvent::GotGameFiles { @@ -1061,6 +1129,69 @@ async fn handle_download_finished(app_handle: &AppHandle, id: String) { .remove(&id); } +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn active_operation_reconciliation_replaces_stale_ui_history() { + let mut active_operations = HashMap::from([ + ("stale".to_string(), UiOperationKind::Installing), + ("keep".to_string(), UiOperationKind::Downloading), + ]); + let snapshot = vec![ + ActiveOperation { + id: "keep".to_string(), + operation: ActiveOperationKind::Updating, + }, + ActiveOperation { + id: "new".to_string(), + operation: ActiveOperationKind::Uninstalling, + }, + ]; + + reconcile_active_operations(&mut active_operations, &snapshot); + + assert_eq!(active_operations.len(), 2); + assert_eq!( + active_operations.get("keep"), + Some(&UiOperationKind::Updating) + ); + assert_eq!( + active_operations.get("new"), + Some(&UiOperationKind::Uninstalling) + ); + assert!( + !active_operations.contains_key("stale"), + "snapshot reconciliation should drop stale UI operations" + ); + } + + #[test] + fn active_operation_payload_is_sorted_for_stable_ui_updates() { + let active_operations = HashMap::from([ + ("zeta".to_string(), UiOperationKind::Downloading), + ("alpha".to_string(), UiOperationKind::Installing), + ]); + + let snapshot = ui_active_operations_from_map(&active_operations); + + assert_eq!( + snapshot, + vec![ + UiActiveOperation { + id: "alpha".to_string(), + operation: UiOperationKind::Installing, + }, + UiActiveOperation { + id: "zeta".to_string(), + operation: UiOperationKind::Downloading, + }, + ] + ); + } +} + #[allow(clippy::missing_panics_doc)] #[cfg_attr(mobile, tauri::mobile_entry_point)] pub fn run() { diff --git a/crates/lanspread-tauri-deno-ts/src/App.tsx b/crates/lanspread-tauri-deno-ts/src/App.tsx index bbbd164..50af46c 100644 --- a/crates/lanspread-tauri-deno-ts/src/App.tsx +++ b/crates/lanspread-tauri-deno-ts/src/App.tsx @@ -55,6 +55,23 @@ enum GameAvailability { LocalOnly = 'LocalOnly', } +enum ActiveOperationKind { + Downloading = 'Downloading', + Installing = 'Installing', + Updating = 'Updating', + Uninstalling = 'Uninstalling', +} + +interface ActiveOperation { + id: string; + operation: ActiveOperationKind; +} + +interface GamesListPayload { + games: Game[]; + active_operations?: ActiveOperation[]; +} + interface GameThumbnailProps { gameId: string; alt: string; @@ -95,27 +112,78 @@ const IN_PROGRESS_INSTALL_STATUSES = new Set([ InstallStatus.Uninstalling, ]); +const RECONCILED_OPERATION_STATUSES = new Set([ + InstallStatus.Downloading, + InstallStatus.Installing, + InstallStatus.Uninstalling, +]); + const isInProgressInstallStatus = (status: InstallStatus): boolean => { return IN_PROGRESS_INSTALL_STATUSES.has(status); }; -const mergeGameUpdate = (game: Game, previous?: Game): Game => { +const isReconciledOperationStatus = (status: InstallStatus): boolean => { + return RECONCILED_OPERATION_STATUSES.has(status); +}; + +const installStatusFromActiveOperation = (operation: ActiveOperationKind): InstallStatus => { + switch (operation) { + case ActiveOperationKind.Downloading: + return InstallStatus.Downloading; + case ActiveOperationKind.Installing: + case ActiveOperationKind.Updating: + return InstallStatus.Installing; + case ActiveOperationKind.Uninstalling: + return InstallStatus.Uninstalling; + } +}; + +const activeStatusById = (activeOperations: ActiveOperation[] = []): Map => { + return new Map(activeOperations.map(operation => [ + operation.id, + installStatusFromActiveOperation(operation.operation), + ])); +}; + +const normalizeGamesListPayload = (payload: GamesListPayload | Game[]): GamesListPayload => { + if (Array.isArray(payload)) { + return { games: payload }; + } + return payload; +}; + +const mergeGameUpdate = ( + game: Game, + previous?: Game, + activeStatus?: InstallStatus, + hasAuthoritativeSnapshot = false, +): Game => { let installStatus = InstallStatus.NotInstalled; - if (game.installed) { + if (activeStatus !== undefined) { + installStatus = activeStatus; + } else if (game.installed) { installStatus = InstallStatus.Installed; - } else if (previous && isInProgressInstallStatus(previous.install_status)) { + } else if ( + previous + && isInProgressInstallStatus(previous.install_status) + && (!hasAuthoritativeSnapshot || previous.install_status === InstallStatus.CheckingPeers) + ) { installStatus = previous.install_status; } const localStateChanged = previous !== undefined && (previous.installed !== game.installed || previous.downloaded !== game.downloaded); + const activeStateReconciled = hasAuthoritativeSnapshot + && (activeStatus !== undefined + || (previous !== undefined && isReconciledOperationStatus(previous.install_status))); + const clearStatus = localStateChanged || activeStateReconciled; return { ...game, availability: game.availability ?? (game.downloaded ? GameAvailability.Ready : GameAvailability.LocalOnly), install_status: installStatus, - status_message: localStateChanged ? undefined : previous?.status_message, - status_level: localStateChanged ? undefined : previous?.status_level, + status_message: clearStatus ? undefined : previous?.status_message, + status_level: clearStatus ? undefined : previous?.status_level, peer_count: game.peer_count ?? 0, }; }; @@ -356,11 +424,19 @@ const App = () => { // Listen for games-list-updated events const unlisten_games = await listen('games-list-updated', (event) => { console.log('🗲 Received games-list-updated event'); - const games = event.payload as Game[]; + const payload = normalizeGamesListPayload(event.payload as GamesListPayload | Game[]); + const games = payload.games; + const activeStatuses = activeStatusById(payload.active_operations); + const hasAuthoritativeSnapshot = payload.active_operations !== undefined; console.log(`🎮 ${games.length} Games received`); setGameItems(prev => { const previousById = new Map(prev.map(item => [item.id, item])); - return games.map(game => mergeGameUpdate(game, previousById.get(game.id))); + return games.map(game => mergeGameUpdate( + game, + previousById.get(game.id), + activeStatuses.get(game.id), + hasAuthoritativeSnapshot, + )); }); void getInitialGameDir(); });