From 1de572fe0d3d3427c4c6ed1994e35f66c83ab383 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 21 Jul 2026 11:19:17 +0000 Subject: [PATCH] fix(tui): guard metadata background races Co-authored-by: Patrick Mill --- romm-tui/src/tui/app/background/tasks.rs | 12 ++- romm-tui/src/tui/app/background/types.rs | 1 + romm-tui/src/tui/app/handlers/game_detail.rs | 17 +++- .../src/tui/app/handlers/metadata_match.rs | 23 +++++ romm-tui/src/tui/app/mod.rs | 6 ++ romm-tui/src/tui/app/tests.rs | 85 ++++++++++++++++++- 6 files changed, 140 insertions(+), 4 deletions(-) diff --git a/romm-tui/src/tui/app/background/tasks.rs b/romm-tui/src/tui/app/background/tasks.rs index 3d04344..52289e8 100644 --- a/romm-tui/src/tui/app/background/tasks.rs +++ b/romm-tui/src/tui/app/background/tasks.rs @@ -486,6 +486,8 @@ impl super::super::App { rom_id: u64, search_term: String, ) { + self.metadata_search_gen = self.metadata_search_gen.wrapping_add(1); + let gen = self.metadata_search_gen; let client = self.client.clone(); let tx = self.metadata_search_tx.clone(); tokio::spawn(async move { @@ -493,7 +495,11 @@ impl super::super::App { search_metadata_matches(&client, rom_id, Some(search_term), Some("name".into())) .await .map_err(RommError::from); - let _ = tx.send(MetadataSearchDone { rom_id, result }); + let _ = tx.send(MetadataSearchDone { + gen, + rom_id, + result, + }); }); } @@ -504,6 +510,10 @@ impl super::super::App { fields: romm_api::endpoints::roms::RomUpdateFields, unmatch_metadata: bool, ) { + if !self.metadata_apply_inflight_roms.insert(rom_id) { + tracing::debug!("metadata apply already in flight for rom_id={rom_id}"); + return; + } let client = self.client.clone(); let tx = self.metadata_apply_tx.clone(); tokio::spawn(async move { diff --git a/romm-tui/src/tui/app/background/types.rs b/romm-tui/src/tui/app/background/types.rs index 253563c..49702c2 100644 --- a/romm-tui/src/tui/app/background/types.rs +++ b/romm-tui/src/tui/app/background/types.rs @@ -71,6 +71,7 @@ pub(crate) struct CoverLoadDone { #[derive(Debug)] pub(crate) struct MetadataSearchDone { + pub(crate) gen: u64, pub(crate) rom_id: u64, pub(crate) result: Result, RommError>, } diff --git a/romm-tui/src/tui/app/handlers/game_detail.rs b/romm-tui/src/tui/app/handlers/game_detail.rs index 3026b98..5772b42 100644 --- a/romm-tui/src/tui/app/handlers/game_detail.rs +++ b/romm-tui/src/tui/app/handlers/game_detail.rs @@ -68,8 +68,17 @@ impl App { use std::time::{Duration, Instant}; let supported = self.metadata_edit_supported(); let blocked = self.metadata_edit_blocked_message(); + let in_flight = match &self.screen { + AppScreen::GameDetail(detail) => { + self.metadata_apply_inflight_roms.contains(&detail.rom.id) + } + _ => false, + }; if let AppScreen::GameDetail(detail) = &mut self.screen { - if !supported { + if in_flight { + detail.message = Some(Self::metadata_apply_in_progress_message()); + detail.message_clear_at = Some(Instant::now() + Duration::from_secs(5)); + } else if !supported { detail.message = Some(blocked); detail.message_clear_at = Some(Instant::now() + Duration::from_secs(5)); } else { @@ -98,6 +107,12 @@ impl App { } let rom_id = detail.rom.id; let platform_id = detail.rom.platform_id; + if self.metadata_apply_inflight_roms.contains(&rom_id) { + detail.message = Some(Self::metadata_apply_in_progress_message()); + detail.message_clear_at = Some(Instant::now() + Duration::from_secs(5)); + detail.metadata_unmatch_confirm = false; + return Ok(false); + } detail.metadata_unmatch_confirm = false; detail.message = Some("Removing metadata match…".into()); detail.message_clear_at = None; diff --git a/romm-tui/src/tui/app/handlers/metadata_match.rs b/romm-tui/src/tui/app/handlers/metadata_match.rs index 9231bf6..3e3b2b0 100644 --- a/romm-tui/src/tui/app/handlers/metadata_match.rs +++ b/romm-tui/src/tui/app/handlers/metadata_match.rs @@ -13,6 +13,10 @@ use crate::tui::screens::game_detail::GameDetailPrevious; use crate::tui::screens::metadata_match::{MetadataMatchPhase, MetadataMatchScreen}; impl App { + pub(in crate::tui::app) fn metadata_apply_in_progress_message() -> String { + "Metadata update already in progress.".into() + } + pub(in crate::tui::app) fn metadata_edit_supported(&self) -> bool { self.metadata_edit_compat.supported } @@ -22,6 +26,17 @@ impl App { } pub(in crate::tui::app) fn open_metadata_match_screen(&mut self) { + let rom_id = match &self.screen { + AppScreen::GameDetail(detail) => Some(detail.rom.id), + _ => None, + }; + if rom_id.is_some_and(|id| self.metadata_apply_inflight_roms.contains(&id)) { + if let AppScreen::GameDetail(detail) = &mut self.screen { + detail.message = Some(Self::metadata_apply_in_progress_message()); + detail.message_clear_at = Some(Instant::now() + std::time::Duration::from_secs(5)); + } + return; + } if !self.metadata_edit_supported() { let msg = self.metadata_edit_blocked_message(); if let AppScreen::GameDetail(detail) = &mut self.screen { @@ -35,6 +50,7 @@ impl App { let AppScreen::GameDetail(detail) = prev else { return; }; + self.metadata_search_gen = self.metadata_search_gen.wrapping_add(1); self.screen = AppScreen::MetadataMatch(Box::new(MetadataMatchScreen::new_for_rom(detail))); } @@ -220,7 +236,13 @@ impl App { &mut self, done: super::super::background::types::MetadataSearchDone, ) { + if done.gen != self.metadata_search_gen { + return; + } if let AppScreen::MetadataMatch(picker) = &mut self.screen { + if !matches!(picker.phase, MetadataMatchPhase::Loading) { + return; + } if picker.previous.rom.id != done.rom_id { return; } @@ -235,6 +257,7 @@ impl App { &mut self, done: super::super::background::types::MetadataApplyDone, ) { + self.metadata_apply_inflight_roms.remove(&done.rom_id); match done.result { Ok(rom) => self.finish_metadata_apply_success(done.rom_id, *rom), Err(e) => { diff --git a/romm-tui/src/tui/app/mod.rs b/romm-tui/src/tui/app/mod.rs index 5ef792c..3d79013 100644 --- a/romm-tui/src/tui/app/mod.rs +++ b/romm-tui/src/tui/app/mod.rs @@ -146,8 +146,12 @@ pub struct App { save_upload_tx: tokio::sync::mpsc::UnboundedSender, save_download_rx: tokio::sync::mpsc::UnboundedReceiver, save_download_tx: tokio::sync::mpsc::UnboundedSender, + /// Incremented for each metadata picker/search session; stale search completions are ignored. + metadata_search_gen: u64, metadata_search_rx: tokio::sync::mpsc::UnboundedReceiver, metadata_search_tx: tokio::sync::mpsc::UnboundedSender, + /// ROM ids with a metadata match/unmatch PUT currently in flight. + metadata_apply_inflight_roms: HashSet, metadata_apply_rx: tokio::sync::mpsc::UnboundedReceiver, metadata_apply_tx: tokio::sync::mpsc::UnboundedSender, device_list_rx: tokio::sync::mpsc::UnboundedReceiver, @@ -317,8 +321,10 @@ impl App { save_upload_tx, save_download_rx, save_download_tx, + metadata_search_gen: 0, metadata_search_rx, metadata_search_tx, + metadata_apply_inflight_roms: HashSet::new(), metadata_apply_rx, metadata_apply_tx, device_list_rx, diff --git a/romm-tui/src/tui/app/tests.rs b/romm-tui/src/tui/app/tests.rs index 5398f4e..5a21258 100644 --- a/romm-tui/src/tui/app/tests.rs +++ b/romm-tui/src/tui/app/tests.rs @@ -1,7 +1,8 @@ use super::{ background::types::{ AchievementLoadDone, CollectionPrefetchDone, LibraryMetadataRefreshDone, MetadataApplyDone, - RomLoadDone, RomLoadEvent, SaveListDone, SearchLoadDone, SearchLoadEvent, + MetadataSearchDone, RomLoadDone, RomLoadEvent, SaveListDone, SearchLoadDone, + SearchLoadEvent, }, event::{map_key_to_actions, Action, AppEvent, BackgroundAction}, rom_load::{primary_rom_load_result_is_current, primary_rom_load_result_matches_selection}, @@ -28,7 +29,7 @@ use romm_api::feature_compat::{ supported_achievements_compatibility, supported_metadata_edit_compatibility, supported_save_sync_compatibility, }; -use romm_api::types::{Platform, RomList}; +use romm_api::types::{Platform, RomList, SearchRom}; use romm_api::update::UpdateStatus; use serde_json::json; use std::path::PathBuf; @@ -181,6 +182,28 @@ fn rom_fixture_json(id: u64) -> serde_json::Value { }) } +fn metadata_row(name: &str) -> SearchRom { + serde_json::from_value(json!({ + "name": name, + "platform_id": 1, + "igdb_id": 5 + })) + .expect("valid metadata row") +} + +fn app_with_game_detail() -> App { + let mut app = app_with_library(vec![platform(1, "NES", 1)]); + let detail = GameDetailScreen::new( + rom_fixture(), + Vec::new(), + GameDetailPrevious::Search(SearchScreen::new()), + app.downloads.shared(), + COVER_PANEL_WIDTH_DEFAULT, + ); + app.screen = AppScreen::GameDetail(Box::new(detail)); + app +} + fn empty_rom_list_with_total(total: u64) -> RomList { RomList { items: vec![], @@ -1723,6 +1746,64 @@ fn metadata_match_keeps_game_detail_background_results() { } } +#[tokio::test] +async fn stale_metadata_search_result_from_previous_picker_is_ignored() { + let mut app = app_with_game_detail(); + + app.open_metadata_match_screen(); + let stale_gen = app.metadata_search_gen; + + app.handle_key_event(&KeyEvent::new(KeyCode::Esc, KeyModifiers::empty())) + .await + .expect("close first metadata picker"); + assert!(matches!(app.screen, AppScreen::GameDetail(_))); + + app.open_metadata_match_screen(); + + app.apply_background(BackgroundAction::MetadataSearch(MetadataSearchDone { + gen: stale_gen, + rom_id: 10, + result: Ok(vec![metadata_row("Wrong Game")]), + })); + + match &app.screen { + AppScreen::MetadataMatch(picker) => { + assert!( + matches!( + picker.phase, + crate::tui::screens::metadata_match::MetadataMatchPhase::QueryInput + ), + "stale results must not move the reopened picker out of query input" + ); + assert!(picker.rows.is_empty(), "stale rows must not be shown"); + } + _ => panic!("expected metadata picker"), + } +} + +#[tokio::test] +async fn metadata_match_cannot_start_while_apply_is_in_flight_for_same_rom() { + let mut app = app_with_game_detail(); + app.metadata_apply_inflight_roms.insert(10); + + app.handle_key_event(&KeyEvent::new(KeyCode::Char('m'), KeyModifiers::empty())) + .await + .expect("handled metadata key"); + + match &app.screen { + AppScreen::GameDetail(detail) => { + assert!( + detail + .message + .as_deref() + .is_some_and(|msg| msg.contains("already in progress")), + "user should be told the existing metadata update is still running" + ); + } + _ => panic!("metadata picker must not open while an update is in flight"), + } +} + fn app_on_library() -> App { let config = Config { base_url: "http://127.0.0.1:9".into(),