From 4f4c655de68d24827147300806bc778d037a75ec Mon Sep 17 00:00:00 2001 From: NimBold Date: Fri, 10 Jul 2026 12:07:25 +0330 Subject: [PATCH] fix: address post-audit regressions across queue, db, and ui - Preserved extension-captured cookies through the Add modal, with a clean fallback when captured cookies break metadata fetching. - Prevented batched extension captures from losing URLs or reusing stale cookie/header contexts. - Fixed pause/resume and enqueue generation races, including cancellation during queue reservation and replay after task removal. - Made startup database initialization safe under React StrictMode. - Serialized keyring operations and corrected Linux legacy migration/deletion behavior. - Restored `Downloading` state after yt-dlp retries. - Replaced hardcoded media heights with dynamically detected formats, including nonstandard qualities such as 576p and 2880p. --- src-tauri/src/db.rs | 82 +++++++++- src-tauri/src/extension_server.rs | 18 +-- src-tauri/src/lib.rs | 223 +++++++++++++++++++------- src-tauri/src/queue.rs | 143 ++++++++++------- src-tauri/tests/queue_manager.rs | 41 ++++- src/App.tsx | 21 ++- src/components/AddDownloadsModal.tsx | 47 ++++-- src/store/useDownloadStore.test.ts | 50 +++++- src/store/useDownloadStore.ts | 59 ++++--- src/utils/addDownloadMetadata.test.ts | 21 +++ src/utils/addDownloadMetadata.ts | 31 ++++ 11 files changed, 550 insertions(+), 186 deletions(-) diff --git a/src-tauri/src/db.rs b/src-tauri/src/db.rs index f386fd2..7d2442e 100644 --- a/src-tauri/src/db.rs +++ b/src-tauri/src/db.rs @@ -12,6 +12,7 @@ const CURRENT_SCHEMA_VERSION: i64 = 1; const TOKEN_CHANGED_NOTICE: &str = "pairing-token-changed"; pub const PAIRING_TOKEN_KEYCHAIN_ID: &str = "extension-pairing-token"; const KEYCHAIN_SERVICE: &str = "com.firelink.app"; +static KEYRING_OPERATION_LOCK: Mutex<()> = Mutex::new(()); pub struct DbState { conn: Mutex, @@ -888,16 +889,16 @@ fn ensure_keyring_store() -> Result<(), String> { #[cfg(target_os = "windows")] { - let store = windows_native_keyring_store::Store::new() - .map_err(|error| error.to_string())?; + let store = + windows_native_keyring_store::Store::new().map_err(|error| error.to_string())?; keyring_core::set_default_store(store); Ok(()) } #[cfg(target_os = "linux")] { - let store = zbus_secret_service_keyring_store::Store::new() - .map_err(|error| error.to_string())?; + let store = + zbus_secret_service_keyring_store::Store::new().map_err(|error| error.to_string())?; keyring_core::set_default_store(store); Ok(()) } @@ -934,29 +935,98 @@ fn keychain_entry(id: &str) -> Result { keychain_entry_with_target(id, None) } +fn lock_keyring_operations() -> Result, String> { + KEYRING_OPERATION_LOCK + .lock() + .map_err(|_| "keyring operation lock is unavailable".to_string()) +} + +#[cfg(target_os = "linux")] +fn legacy_linux_keychain_entries(id: &str) -> Result, String> { + ensure_keyring_store()?; + let entries = keyring_core::Entry::search(&std::collections::HashMap::from([ + ("service", KEYCHAIN_SERVICE), + ("username", id), + ])) + .map_err(|error| error.to_string())?; + let mut legacy = Vec::new(); + for entry in entries { + let attributes = entry.get_attributes().map_err(|error| error.to_string())?; + if !attributes.contains_key("target") { + legacy.push(entry); + } + } + Ok(legacy) +} + +#[cfg(target_os = "linux")] +fn unique_legacy_linux_keychain_entry(id: &str) -> Result, String> { + let mut entries = legacy_linux_keychain_entries(id)?; + match entries.len() { + 0 => Ok(None), + 1 => Ok(entries.pop()), + count => Err(format!( + "Entry is matched by {count} legacy Linux credentials" + )), + } +} + pub fn set_keychain_password(id: &str, password: &str) -> Result<(), String> { + let _guard = lock_keyring_operations()?; let entry = keychain_entry(id)?; + + #[cfg(target_os = "linux")] + if let Err(error) = entry.get_credential() { + match error { + keyring_core::Error::NoEntry => { + if let Some(legacy) = unique_legacy_linux_keychain_entry(id)? { + return legacy + .set_password(password) + .map_err(|error| error.to_string()); + } + } + error => return Err(error.to_string()), + } + } + entry .set_password(password) .map_err(|error| error.to_string()) } pub fn get_keychain_password(id: &str) -> Result { + let _guard = lock_keyring_operations()?; let entry = keychain_entry(id)?; match entry.get_password() { Ok(password) => Ok(password), #[cfg(target_os = "linux")] - Err(_) => keychain_entry_with_target(id, None)? + Err(keyring_core::Error::NoEntry) => unique_legacy_linux_keychain_entry(id)? + .ok_or_else(|| keyring_core::Error::NoEntry.to_string())? .get_password() .map_err(|error| error.to_string()), + #[cfg(target_os = "linux")] + Err(error) => Err(error.to_string()), #[cfg(not(target_os = "linux"))] Err(error) => Err(error.to_string()), } } pub fn delete_keychain_password(id: &str) -> Result<(), String> { + let _guard = lock_keyring_operations()?; let entry = keychain_entry(id)?; - let _ = entry.delete_credential(); + match entry.delete_credential() { + Ok(()) | Err(keyring_core::Error::NoEntry) => {} + Err(error) => return Err(error.to_string()), + } + + #[cfg(target_os = "linux")] + for legacy in legacy_linux_keychain_entries(id)? { + match legacy.delete_credential() { + Ok(()) | Err(keyring_core::Error::NoEntry) => {} + Err(error) => return Err(error.to_string()), + } + } + Ok(()) } diff --git a/src-tauri/src/extension_server.rs b/src-tauri/src/extension_server.rs index 841d415..018b953 100644 --- a/src-tauri/src/extension_server.rs +++ b/src-tauri/src/extension_server.rs @@ -315,13 +315,10 @@ fn normalize_download(payload: ExtensionRequest) -> Option { silent: payload.silent, filename, headers: payload.headers.filter(|value| !value.trim().is_empty()), - // A full browser Cookie header can exceed upstream request-header - // limits. Media uses yt-dlp's configured browser-cookie source - // instead; regular captured downloads retain their exact cookies. - cookies: (!payload.media) - .then_some(payload.cookies) - .flatten() - .filter(|value| !value.trim().is_empty()), + // Keep the exact tab/container cookie context. The Add modal may retry + // public media without this header when an upstream rejects its size, + // but authenticated and container-scoped media must not lose it here. + cookies: payload.cookies.filter(|value| !value.trim().is_empty()), media: payload.media, }) } @@ -499,7 +496,7 @@ mod tests { } #[test] - fn explicit_media_drops_the_extension_cookie_header() { + fn explicit_media_preserves_the_extension_cookie_header() { let download = normalize_download(ExtensionRequest { urls: vec!["https://www.youtube.com/watch?v=example".to_string()], referer: None, @@ -512,7 +509,10 @@ mod tests { .expect("valid media handoff"); assert!(download.media); - assert!(download.cookies.is_none()); + assert_eq!( + download.cookies.as_deref(), + Some("large=browser-cookie-header") + ); assert_eq!(download.headers.as_deref(), Some("User-Agent: Firefox")); } diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index 949d88f..f76b79c 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -3,7 +3,7 @@ // Learn more about Tauri commands at https://tauri.app/develop/calling-rust/ use regex::Regex; use serde::Serialize; -use std::collections::{HashMap, VecDeque}; +use std::collections::{BTreeSet, HashMap, VecDeque}; use std::hash::{Hash, Hasher}; use std::path::PathBuf; use std::sync::OnceLock; @@ -312,26 +312,43 @@ fn is_excluded_yt_dlp_format(value: &serde_json::Value) -> bool { } fn format_height(value: &serde_json::Value) -> Option { - if let Some(height) = json_u64(value, "height") { + if let Some(height) = json_u64(value, "height").filter(|height| *height > 0) { return Some(height); } if let Some(resolution) = json_str(value, "resolution") { - if let Some((_, height)) = resolution.split_once('x') { - if let Ok(parsed) = height.parse::() { - return Some(parsed); + if let Some((_, height)) = resolution + .split_once('x') + .or_else(|| resolution.split_once('X')) + { + if let Ok(parsed) = height.trim().parse::() { + if parsed > 0 { + return Some(parsed); + } } } } let note = json_lower(value, "format_note"); - for height in [4320_u64, 2160, 1440, 1080, 720, 480, 360, 240, 144] { - if note.contains(&format!("{height}p")) { - return Some(height); + let bytes = note.as_bytes(); + let mut heights = Vec::new(); + for (index, byte) in bytes.iter().enumerate() { + if *byte != b'p' || index == 0 { + continue; + } + let mut start = index; + while start > 0 && bytes[start - 1].is_ascii_digit() { + start -= 1; + } + if start < index { + if let Ok(height) = note[start..index].parse::() { + if height > 0 { + heights.push(height); + } + } } } - - None + heights.into_iter().max() } fn matches_media_height(value: &serde_json::Value, target: u64) -> bool { @@ -339,11 +356,6 @@ fn matches_media_height(value: &serde_json::Value, target: u64) -> bool { return false; } - let note = json_lower(value, "format_note"); - if note.contains(&format!("{target}p")) { - return true; - } - format_height(value) == Some(target) } @@ -536,13 +548,13 @@ fn build_media_format_options( let mut options = Vec::new(); if has_video { - let available_heights: Vec = [4320_u64, 2160, 1440, 1080, 720, 480, 360, 240, 144] + let available_heights: Vec = clean_formats + .iter() + .filter(|format| has_video_stream(format)) + .filter_map(|format| format_height(format)) + .collect::>() .into_iter() - .filter(|height| { - clean_formats - .iter() - .any(|format| matches_media_height(format, *height)) - }) + .rev() .collect(); for height in available_heights { @@ -2882,11 +2894,11 @@ pub(crate) async fn start_media_download_internal( let max_retries = max_tries.unwrap_or(0).max(0) as usize; let mut strike = 0_usize; - let mut processing_started = false; let mut effective_cookie_source = cookie_source.clone(); let mut browser_cookie_fallback_used = false; while strike <= max_retries { + let mut processing_started = false; let ytdlp_path = resolve_bundled_binary_path(&app_handle, "yt-dlp")?; let mut cmd = app_handle.shell().command(&ytdlp_path); for arg in media_progress_args() { @@ -2986,6 +2998,16 @@ pub(crate) async fn start_media_download_internal( let (mut rx, child) = cmd .spawn() .map_err(|e| format!("Failed to spawn yt-dlp: {}", e))?; + if strike > 0 { + // The backoff path emits `Retrying`. Restore the live transfer + // state when the replacement process actually starts so React + // accepts progress from this attempt instead of staying stuck. + progress_state.speed_sampler.reset(); + let _ = app_handle.emit( + "download-state", + DownloadStateEvent::new(id, crate::ipc::DownloadStatus::Downloading), + ); + } log::info!("yt-dlp spawned for id: {} (strike {})", id, strike); let mut stderr_tail = String::new(); @@ -3101,7 +3123,6 @@ pub(crate) async fn start_media_download_internal( ); } if !processing_started && is_media_processing_line(&line) { - processing_started = true; let _ = app_handle.emit( "download-state", DownloadStateEvent::new( @@ -3946,6 +3967,18 @@ async fn get_pending_order( Ok(state.queue_manager.pending_order(queue_id.as_deref()).await) } +fn enqueue_lifecycle_generation(item: &queue::EnqueueItem) -> Result { + item.lifecycle_generation + .as_deref() + .map(|generation| { + generation + .parse::() + .map_err(|_| "Invalid enqueue lifecycle generation".to_string()) + }) + .transpose() + .map(|generation| generation.unwrap_or_default()) +} + #[tauri::command] async fn enqueue_download( app_handle: tauri::AppHandle, @@ -3955,30 +3988,35 @@ async fn enqueue_download( let id = item.id.clone(); item.filename = crate::download_ownership::canonical_download_filename(&item.filename); let accepted_filename = item.filename.clone(); - crate::download_ownership::register_expected( + let lifecycle_generation = enqueue_lifecycle_generation(&item).map_err(AppError::Internal)?; + let previous_generation = state + .queue_manager + .reserve_enqueue_generation(&id, lifecycle_generation) + .await + .map_err(AppError::Internal)?; + if let Err(error) = crate::download_ownership::register_expected( &app_handle, &item.id, &item.destination, &item.filename, - )?; - let lifecycle_generation = item - .lifecycle_generation - .as_deref() - .map(|generation| { - generation - .parse::() - .map_err(|_| AppError::Internal("Invalid enqueue lifecycle generation".to_string())) - }) - .transpose()? - .unwrap_or_default(); - if let Err(e) = state + ) { + state + .queue_manager + .rollback_enqueue_reservation(&id, lifecycle_generation, previous_generation) + .await; + return Err(AppError::Internal(error)); + } + if let Err(error) = state .queue_manager - .push_with_generation(item.into_task(), lifecycle_generation) + .commit_reserved_enqueue(item.into_task(), lifecycle_generation) .await { let _ = crate::download_ownership::remove(&app_handle, &id); - state.queue_manager.release_registered_id(&id).await; - return Err(AppError::Internal(e)); + state + .queue_manager + .rollback_enqueue_reservation(&id, lifecycle_generation, previous_generation) + .await; + return Err(AppError::Internal(error)); } Ok(crate::ipc::EnqueueAccepted { id, @@ -4006,30 +4044,83 @@ async fn cancel_enqueue_generation( async fn enqueue_many( app_handle: tauri::AppHandle, state: tauri::State<'_, AppState>, - mut items: Vec, + items: Vec, ) -> Result, AppError> { - for item in &mut items { + let mut results = Vec::with_capacity(items.len()); + for mut item in items { item.filename = crate::download_ownership::canonical_download_filename(&item.filename); - } - for item in &items { - crate::download_ownership::register_expected( + let id = item.id.clone(); + let filename = item.filename.clone(); + let lifecycle_generation = match enqueue_lifecycle_generation(&item) { + Ok(generation) => generation, + Err(error) => { + results.push(crate::ipc::EnqueueResult { + id, + success: false, + filename: None, + error: Some(error), + }); + continue; + } + }; + let previous_generation = match state + .queue_manager + .reserve_enqueue_generation(&id, lifecycle_generation) + .await + { + Ok(previous) => previous, + Err(error) => { + results.push(crate::ipc::EnqueueResult { + id, + success: false, + filename: None, + error: Some(error), + }); + continue; + } + }; + if let Err(error) = crate::download_ownership::register_expected( &app_handle, &item.id, &item.destination, &item.filename, - )?; - } - let tasks = items - .into_iter() - .map(queue::EnqueueItem::into_task) - .collect(); - let results = state.queue_manager.enqueue_many(tasks).await; - - for result in &results { - if !result.success { - let _ = crate::download_ownership::remove(&app_handle, &result.id); - state.queue_manager.release_registered_id(&result.id).await; + ) { + state + .queue_manager + .rollback_enqueue_reservation(&id, lifecycle_generation, previous_generation) + .await; + results.push(crate::ipc::EnqueueResult { + id, + success: false, + filename: None, + error: Some(error), + }); + continue; } + if let Err(error) = state + .queue_manager + .commit_reserved_enqueue(item.into_task(), lifecycle_generation) + .await + { + let _ = crate::download_ownership::remove(&app_handle, &id); + state + .queue_manager + .rollback_enqueue_reservation(&id, lifecycle_generation, previous_generation) + .await; + results.push(crate::ipc::EnqueueResult { + id, + success: false, + filename: None, + error: Some(error), + }); + continue; + } + results.push(crate::ipc::EnqueueResult { + id, + success: true, + filename: Some(filename), + error: None, + }); } Ok(results) @@ -5048,7 +5139,7 @@ mod tests { } #[test] - fn keeps_low_and_ultra_high_available_video_qualities() { + fn keeps_every_available_video_height_including_nonstandard_qualities() { let formats = vec![ json!({ "format_id": "401", @@ -5063,12 +5154,28 @@ mod tests { "height": 144, "vcodec": "avc1.42E01E", "acodec": "mp4a.40.2" - }) + }), + json!({ + "format_id": "five-k", + "ext": "webm", + "resolution": "5120X2880", + "vcodec": "av01.0.17M.08", + "acodec": "none" + }), + json!({ + "format_id": "pal", + "ext": "mp4", + "format_note": "Premium 576p", + "vcodec": "avc1.4d401f", + "acodec": "none" + }), ]; let options = build_media_format_options(&formats, Some(60.0)); assert!(options.iter().any(|format| format.resolution == "4320p")); + assert!(options.iter().any(|format| format.resolution == "2880p")); + assert!(options.iter().any(|format| format.resolution == "576p")); assert!(options.iter().any(|format| format.resolution == "144p")); } diff --git a/src-tauri/src/queue.rs b/src-tauri/src/queue.rs index 7f149bf..cf35262 100644 --- a/src-tauri/src/queue.rs +++ b/src-tauri/src/queue.rs @@ -85,6 +85,7 @@ pub trait SidecarSpawner: Send + Sync + 'static { pub struct QueueManager { registered_ids: Mutex>, enqueue_cancellations: Mutex>, + enqueue_generations: Mutex>, pending: Mutex>, semaphore: Arc, active_permits: Mutex>, @@ -136,6 +137,7 @@ impl QueueManager { Self { registered_ids: Mutex::new(HashSet::new()), enqueue_cancellations: Mutex::new(HashMap::new()), + enqueue_generations: Mutex::new(HashMap::new()), pending: Mutex::new(VecDeque::new()), semaphore: Arc::new(Semaphore::new(capacity)), active_permits: Mutex::new(HashMap::new()), @@ -184,32 +186,99 @@ impl QueueManager { .or_insert(generation); } - /// Atomically checks the cancellation watermark before registering a task. - pub async fn push_with_generation( + /// Atomically reserve an ID after rejecting cancelled or replayed generations. + /// The returned watermark must be passed to `rollback_enqueue_reservation` + /// if ownership registration fails before the task is committed. + pub async fn reserve_enqueue_generation( + &self, + id: &str, + generation: u64, + ) -> Result, String> { + let cancellations = self.enqueue_cancellations.lock().await; + if cancellations + .get(id) + .is_some_and(|cancelled| *cancelled >= generation) + { + return Err("Download enqueue was superseded by a newer user action".to_string()); + } + let mut generations = self.enqueue_generations.lock().await; + let previous_generation = generations.get(id).copied(); + if previous_generation.is_some_and(|seen| seen >= generation) { + return Err("Download enqueue was superseded by a newer user action".to_string()); + } + + let mut registered = self.registered_ids.lock().await; + if registered.contains(id) { + return Err("Duplicate task".to_string()); + } + registered.insert(id.to_string()); + generations.insert(id.to_string(), generation); + Ok(previous_generation) + } + + pub async fn rollback_enqueue_reservation( + &self, + id: &str, + generation: u64, + previous_generation: Option, + ) { + let mut generations = self.enqueue_generations.lock().await; + let mut registered = self.registered_ids.lock().await; + if generations.get(id).copied() != Some(generation) { + return; + } + registered.remove(id); + match previous_generation { + Some(previous) => { + generations.insert(id.to_string(), previous); + } + None => { + generations.remove(id); + } + } + } + + pub async fn commit_reserved_enqueue( &self, task: QueuedTask, generation: u64, ) -> Result<(), String> { let id = task.id.clone(); let cancellations = self.enqueue_cancellations.lock().await; - if cancellations.get(&id).is_some_and(|cancelled| *cancelled >= generation) { + if cancellations + .get(&id) + .is_some_and(|cancelled| *cancelled >= generation) + { return Err("Download enqueue was superseded by a newer user action".to_string()); } - - let mut registered = self.registered_ids.lock().await; - if registered.contains(&id) { - return Err("Duplicate task".to_string()); - } - registered.insert(id.clone()); - drop(registered); - drop(cancellations); - self.pending.lock().await.push_back(task); self.emit_state(id, DownloadStatus::Queued); self.notify.notify_one(); Ok(()) } + /// Atomically checks the generation watermark before registering a task. + pub async fn push_with_generation( + &self, + task: QueuedTask, + generation: u64, + ) -> Result<(), String> { + let id = task.id.clone(); + let previous_generation = self.reserve_enqueue_generation(&id, generation).await?; + if let Err(error) = self.commit_reserved_enqueue(task, generation).await { + self.rollback_enqueue_reservation(&id, generation, previous_generation) + .await; + return Err(error); + } + Ok(()) + } + + /// Enqueue a task without a frontend lifecycle token. This is retained for + /// internal/test callers and still gets replay protection at generation 0. + pub async fn push(&self, task: QueuedTask) -> Result<(), String> { + self.push_with_generation(task, 0).await + } + pub async fn next_aria2_control_epoch(&self, id: &str) -> u64 { let mut epochs = self.aria2_control_epochs.lock().await; let epoch = epochs.get(id).copied().unwrap_or_default().wrapping_add(1); @@ -235,22 +304,6 @@ impl QueueManager { self.aria2_retry_strikes.lock().await.contains_key(id) } - /// Enqueue a task. Checks the centralized `registered_ids` for deduplication. - pub async fn push(&self, task: QueuedTask) -> Result<(), String> { - let id = task.id.clone(); - let mut registered = self.registered_ids.lock().await; - if registered.contains(&id) { - return Err("Duplicate task".to_string()); - } - registered.insert(id.clone()); - drop(registered); - - self.pending.lock().await.push_back(task); - self.emit_state(id, DownloadStatus::Queued); - self.notify.notify_one(); - Ok(()) - } - /// Pop the next task, or None if empty. pub async fn pop_front(&self) -> Option { self.pending.lock().await.pop_front() @@ -878,40 +931,6 @@ impl QueueManager { } removed } - - /// Bulk enqueue by appending tasks. Used by startup and start-all. - pub async fn enqueue_many(&self, tasks: Vec) -> Vec { - let mut results = Vec::new(); - let mut registered = self.registered_ids.lock().await; - let mut pending = self.pending.lock().await; - - for task in tasks { - let id = task.id.clone(); - let filename = task.payload.filename.clone(); - if registered.contains(&id) { - results.push(crate::ipc::EnqueueResult { - id: id.clone(), - success: false, - filename: None, - error: Some("Duplicate task".to_string()), - }); - continue; - } - registered.insert(id.clone()); - pending.push_back(task); - self.emit_state(id.clone(), DownloadStatus::Queued); - results.push(crate::ipc::EnqueueResult { - id, - success: true, - filename: Some(filename), - error: None, - }); - } - drop(pending); - drop(registered); - self.notify.notify_one(); - results - } } fn automatic_retry_limit(max_tries: Option) -> usize { diff --git a/src-tauri/tests/queue_manager.rs b/src-tauri/tests/queue_manager.rs index b574365..2902f9b 100644 --- a/src-tauri/tests/queue_manager.rs +++ b/src-tauri/tests/queue_manager.rs @@ -107,7 +107,10 @@ async fn cancelled_enqueue_generation_cannot_register_after_a_newer_user_action( mgr.cancel_enqueue_generation("a", 4).await; let stale = mgr.push_with_generation(sample_task("a"), 4).await; - assert!(stale.is_err(), "cancelled generation must not enter the queue"); + assert!( + stale.is_err(), + "cancelled generation must not enter the queue" + ); assert!(!mgr.is_registered("a").await); mgr.push_with_generation(sample_task("a"), 5) @@ -116,6 +119,42 @@ async fn cancelled_enqueue_generation_cannot_register_after_a_newer_user_action( assert_eq!(mgr.pending_order(None).await, vec!["a".to_string()]); } +#[tokio::test] +async fn cancellation_between_reservation_and_commit_cannot_start_the_task() { + let (mgr, _spawner) = make_manager(2); + let previous = mgr + .reserve_enqueue_generation("a", 7) + .await + .expect("reservation should succeed"); + mgr.cancel_enqueue_generation("a", 7).await; + + let committed = mgr.commit_reserved_enqueue(sample_task("a"), 7).await; + assert!(committed.is_err(), "cancelled reservation must not commit"); + mgr.rollback_enqueue_reservation("a", 7, previous).await; + + assert!(!mgr.is_registered("a").await); + assert!(mgr.pending_order(None).await.is_empty()); + assert!(mgr.push_with_generation(sample_task("a"), 7).await.is_err()); + mgr.push_with_generation(sample_task("a"), 8) + .await + .expect("a newer generation should remain startable"); +} + +#[tokio::test] +async fn accepted_generation_cannot_be_replayed_after_registry_release() { + let (mgr, _spawner) = make_manager(2); + mgr.push_with_generation(sample_task("a"), 3) + .await + .expect("first enqueue should succeed"); + assert!(mgr.remove_from_pending("a").await); + mgr.release_registered_id("a").await; + + assert!(mgr.push_with_generation(sample_task("a"), 3).await.is_err()); + mgr.push_with_generation(sample_task("a"), 4) + .await + .expect("only a newer lifecycle may reuse the id"); +} + #[tokio::test] async fn release_permit_is_idempotent() { let (mgr, _spawner) = make_manager(2); diff --git a/src/App.tsx b/src/App.tsx index e657236..562950d 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -37,6 +37,20 @@ const waitForSettingsHydration = (): Promise => { }); }; +let downloadStateInitialization: Promise | null = null; +const initializeDownloadState = (): Promise => { + if (!downloadStateInitialization) { + downloadStateInitialization = (async () => { + await waitForSettingsHydration(); + await useDownloadStore.getState().initDB(); + })().catch(error => { + downloadStateInitialization = null; + throw error; + }); + } + return downloadStateInitialization; +}; + const getScheduledQueueIds = () => { const downloadState = useDownloadStore.getState(); const availableQueueIds = new Set(downloadState.queues.map(queue => queue.id)); @@ -227,10 +241,11 @@ function App() { let active = true; const initialize = async () => { try { - await waitForSettingsHydration(); - await useDownloadStore.getState().initDB(); - if (active) setCoreReady(true); + await initializeDownloadState(); + if (!active) return; + setCoreReady(true); } catch (error) { + if (!active) return; console.error('Failed to initialize Firelink state:', error); addToast({ message: `Could not initialize saved downloads: ${String(error)}`, diff --git a/src/components/AddDownloadsModal.tsx b/src/components/AddDownloadsModal.tsx index 00296c6..89a1cfa 100644 --- a/src/components/AddDownloadsModal.tsx +++ b/src/components/AddDownloadsModal.tsx @@ -23,6 +23,7 @@ import { isTransferLocked } from '../utils/downloadActions'; import { useToast } from '../contexts/ToastContext'; import { canSubmitMetadataRows, + appendRequestUrlsAfterVersion, mediaFileNameForSelectedFormat, mediaFormatSelectorForRow, metadataSummaryMessage, @@ -65,7 +66,6 @@ export const AddDownloadsModal = () => { pendingAddMediaUrls, pendingAddRequestContexts, pendingAddRequestVersion, - pendingAddLatestUrls, toggleAddModal, addDownload, queues @@ -120,8 +120,9 @@ export const AddDownloadsModal = () => { if (context) return extensionHeaders(context).trim(); return hasExtensionRequestContext ? '' : headers.trim(); }; - const cookiesForRow = (sourceUrl: string) => { + const cookiesForRow = (sourceUrl: string, omitRequestCookies = false) => { if (cookiesManuallyEditedRef.current) return cookies.trim(); + if (omitRequestCookies) return ''; const context = requestContextForUrl(sourceUrl); if (context) return context.cookies.trim(); return hasExtensionRequestContext ? '' : cookies.trim(); @@ -189,14 +190,14 @@ export const AddDownloadsModal = () => { useEffect(() => { if (!isAddModalOpen || !modalSessionRef.current || observedRequestVersionRef.current === pendingAddRequestVersion) return; + const observedVersion = observedRequestVersionRef.current; observedRequestVersionRef.current = pendingAddRequestVersion; - const additions = pendingAddLatestUrls - .split('\n') - .map(url => url.trim()) - .filter(Boolean); - if (additions.length === 0) return; - setUrls(current => current.trim() ? `${current.trim()}\n${additions.join('\n')}` : additions.join('\n')); - }, [isAddModalOpen, pendingAddRequestVersion, pendingAddLatestUrls]); + setUrls(current => appendRequestUrlsAfterVersion( + current, + pendingAddRequestContexts, + observedVersion + )); + }, [isAddModalOpen, pendingAddRequestContexts, pendingAddRequestVersion]); useEffect(() => { if (!isQueueMenuOpen) return; @@ -302,7 +303,25 @@ export const AddDownloadsModal = () => { cookies: rowCookies || null, proxy }; - const mediaData = await fetchMediaMetadataDeduped(mediaMetadataArgs); + let requestCookiesOmitted = false; + let mediaData; + try { + mediaData = await fetchMediaMetadataDeduped(mediaMetadataArgs); + } catch (error) { + const capturedCookies = requestContextForUrl(row.sourceUrl)?.cookies.trim(); + if (!rowCookies || !capturedCookies || cookiesManuallyEditedRef.current) { + throw error; + } + console.warn( + 'Media metadata rejected the captured Cookie header; retrying without request cookies', + error + ); + mediaData = await fetchMediaMetadataDeduped({ + ...mediaMetadataArgs, + cookies: null + }); + requestCookiesOmitted = true; + } if (mediaData && mediaData.formats.length > 0) { const mappedFormats = mediaData.formats.map(f => { const quality = f.resolution || 'Video'; @@ -334,6 +353,7 @@ export const AddDownloadsModal = () => { size: mappedFormats[0].bytes ? mappedFormats[0].detail : undefined, sizeBytes: mappedFormats[0].bytes || undefined, status: 'ready', + requestCookiesOmitted, formats: mappedFormats, selectedFormat: 0 }) @@ -709,7 +729,7 @@ export const AddDownloadsModal = () => { checksum: checksumEnabled && checksumValue.trim() ? `${checksumAlgo}=${checksumValue.trim()}` : undefined, - cookies: cookiesForRow(item.sourceUrl) || undefined, + cookies: cookiesForRow(item.sourceUrl, item.requestCookiesOmitted) || undefined, mirrors: mirrors.trim() || undefined, destination: useSharedDestination ? finalLocation @@ -1128,6 +1148,11 @@ export const AddDownloadsModal = () => { className="add-download-control w-full px-3 py-1.5 text-xs font-mono" aria-label="Cookies" /> + {!cookiesManuallyEditedRef.current && parsedItems.some(item => item.requestCookiesOmitted) && ( +

+ Media metadata only worked without the captured cookies, so they will be omitted for affected rows. Edit this field to force a manual value. +

+ )}
diff --git a/src/store/useDownloadStore.test.ts b/src/store/useDownloadStore.test.ts index fd4965c..ae3c4fc 100644 --- a/src/store/useDownloadStore.test.ts +++ b/src/store/useDownloadStore.test.ts @@ -90,7 +90,6 @@ describe('useDownloadStore', () => { pendingAddMediaUrls: [], pendingAddRequestContexts: {}, pendingAddRequestVersion: 0, - pendingAddLatestUrls: '', }); }); @@ -315,6 +314,15 @@ describe('useDownloadStore', () => { ); }); const pause = useDownloadStore.getState().pauseDownload('paused'); + await vi.waitFor(() => { + expect(ipc.invokeCommand).toHaveBeenCalledWith( + 'cancel_enqueue_generation', + expect.objectContaining({ id: 'paused' }) + ); + }); + expect( + vi.mocked(ipc.invokeCommand).mock.calls.some(([command]) => command === 'pause_download') + ).toBe(false); resolveEnqueue({ id: 'paused', filename: 'paused.bin' }); await expect(pause).resolves.toBeUndefined(); @@ -784,21 +792,22 @@ describe('useDownloadStore', () => { 'https://first.example/file.zip\nhttps://second.example/file.zip' ); expect(state.pendingAddRequestVersion).toBe(2); - expect(state.pendingAddLatestUrls).toBe('https://second.example/file.zip'); expect(state.pendingAddRequestContexts).toEqual({ 'https://first.example/file.zip': { version: 1, referer: 'https://first.example/page', filename: 'first.zip', headers: 'User-Agent: First Browser', - cookies: 'first=session' + cookies: 'first=session', + media: false }, 'https://second.example/file.zip': { version: 2, referer: 'https://second.example/page', filename: 'second.zip', headers: 'User-Agent: Second Browser', - cookies: 'second=session' + cookies: 'second=session', + media: false } }); }); @@ -818,7 +827,38 @@ describe('useDownloadStore', () => { expect(state.isAddModalOpen).toBe(true); expect(state.pendingAddUrls).toBe('https://adult.example/watch/123'); expect(state.pendingAddMediaUrls).toEqual(['https://adult.example/watch/123']); - expect(state.pendingAddCookies).toBe(''); + expect(state.pendingAddCookies).toBe('session=secret'); + }); + + it('clears stale request context when the same URL is captured without it later', async () => { + const url = 'https://example.com/file.zip'; + await useDownloadStore.getState().handleExtensionDownload({ + urls: [url], + referer: 'https://example.com/private', + silent: true, + filename: 'private.zip', + headers: 'Authorization: secret', + cookies: 'session=secret', + media: false + }); + await useDownloadStore.getState().handleExtensionDownload({ + urls: [url], + referer: null, + silent: true, + filename: null, + headers: null, + cookies: null, + media: false + }); + + expect(useDownloadStore.getState().pendingAddRequestContexts[url]).toEqual({ + version: 2, + referer: '', + filename: '', + headers: '', + cookies: '', + media: false + }); }); it('deduplicates forced media URLs and drops stale media intent when opening fresh', async () => { diff --git a/src/store/useDownloadStore.ts b/src/store/useDownloadStore.ts index d56d8a6..cb06a83 100644 --- a/src/store/useDownloadStore.ts +++ b/src/store/useDownloadStore.ts @@ -311,6 +311,7 @@ export type PendingAddRequestContext = { filename: string; headers: string; cookies: string; + media: boolean; }; export type DeleteModalState = { @@ -338,7 +339,6 @@ interface DownloadState { pendingAddMediaUrls: string[]; pendingAddRequestContexts: Record; pendingAddRequestVersion: number; - pendingAddLatestUrls: string; selectedPropertiesDownloadId: string | null; toggleAddModal: (isOpen: boolean) => void; openAddModalWithUrls: ( @@ -475,7 +475,6 @@ export const useDownloadStore = create((set, get) => ({ pendingAddMediaUrls: [], pendingAddRequestContexts: {}, pendingAddRequestVersion: 0, - pendingAddLatestUrls: '', selectedPropertiesDownloadId: null, deleteModalState: { isOpen: false }, openDeleteModal: (downloadIds) => set({ @@ -493,8 +492,7 @@ export const useDownloadStore = create((set, get) => ({ pendingAddHeaders: '', pendingAddCookies: '', pendingAddMediaUrls: [], - pendingAddRequestContexts: {}, - pendingAddLatestUrls: '' + pendingAddRequestContexts: {} }), openAddModalWithUrls: (urls, referer, filename, headers, cookies, media = false) => set((state) => { const isAppending = state.isAddModalOpen && Boolean(state.pendingAddUrls); @@ -512,28 +510,30 @@ export const useDownloadStore = create((set, get) => ({ const cleanHeaders = headers?.trim() || ''; const cleanCookies = cookies?.trim() || ''; const requestVersion = state.pendingAddRequestVersion + 1; - const hasRequestContext = Boolean(cleanReferer || cleanFilename || cleanHeaders || cleanCookies); const pendingAddRequestContexts = isAppending ? { ...state.pendingAddRequestContexts } : {}; - if (hasRequestContext) { - for (const rawUrl of urls.split('\n')) { - const trimmedUrl = rawUrl.trim(); - if (!trimmedUrl) continue; - let key = trimmedUrl; - try { - key = new URL(trimmedUrl).href; - } catch { - // The Add modal will mark malformed input invalid; retain its original key here. - } - pendingAddRequestContexts[key] = { - version: requestVersion, - referer: cleanReferer, - filename: cleanFilename, - headers: cleanHeaders, - cookies: cleanCookies - }; + // Every handoff gets a versioned row context, including an intentionally + // empty one. Otherwise a later request for the same URL cannot clear stale + // cookies/headers from an earlier capture, and batched React renders can + // lose all but the most recent appended URL. + for (const rawUrl of urls.split('\n')) { + const trimmedUrl = rawUrl.trim(); + if (!trimmedUrl) continue; + let key = trimmedUrl; + try { + key = new URL(trimmedUrl).href; + } catch { + // The Add modal will mark malformed input invalid; retain its original key here. } + pendingAddRequestContexts[key] = { + version: requestVersion, + referer: cleanReferer, + filename: cleanFilename, + headers: cleanHeaders, + cookies: cleanCookies, + media + }; } return { isAddModalOpen: true, @@ -544,25 +544,19 @@ export const useDownloadStore = create((set, get) => ({ pendingAddCookies: cleanCookies, pendingAddMediaUrls, pendingAddRequestContexts, - pendingAddRequestVersion: requestVersion, - pendingAddLatestUrls: urls + pendingAddRequestVersion: requestVersion }; }), handleExtensionDownload: async (request) => { const urls = [...new Set(request.urls.map(url => url.trim()).filter(Boolean))]; if (urls.length === 0) return; - // Explicit media uses yt-dlp and its configured browser-cookie source. - // Passing Firefox's complete page Cookie header can exceed YouTube's - // request-header limit; ordinary captured file downloads keep it. - const cookies = request.media === true ? null : request.cookies; - get().openAddModalWithUrls( urls.join('\n'), request.referer, urls.length === 1 ? request.filename : null, request.headers, - cookies, + request.cookies, request.media === true ); }, @@ -687,7 +681,10 @@ export const useDownloadStore = create((set, get) => ({ syncSystemIntegrations(); }, pauseDownload: async (id) => { - const { generation } = await invalidateDispatch(id); + const { generation, pendingDispatch } = await invalidateDispatch(id); + if (pendingDispatch) { + await pendingDispatch; + } await invoke('pause_download', { id }); diff --git a/src/utils/addDownloadMetadata.test.ts b/src/utils/addDownloadMetadata.test.ts index fd1afbd..006d5e2 100644 --- a/src/utils/addDownloadMetadata.test.ts +++ b/src/utils/addDownloadMetadata.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from 'vitest'; import { + appendRequestUrlsAfterVersion, canSubmitMetadataRows, mediaFormatSelectorForRow, mediaFileNameForSelectedFormat, @@ -93,6 +94,7 @@ describe('add download metadata workflow', () => { status: 'ready', generation: 4, requestContextVersion: 1, + requestCookiesOmitted: true, formats: [{ name: '1080p MP4', selector: '137+140', @@ -119,11 +121,30 @@ describe('add download metadata workflow', () => { status: 'loading', generation: 5, requestContextVersion: 2, + requestCookiesOmitted: false, formats: undefined, selectedFormat: undefined }); }); + it('appends every unseen handoff after the observed version', () => { + const merged = appendRequestUrlsAfterVersion( + 'https://existing.example/file.zip', + { + 'https://first.example/file.zip': { version: 2 }, + 'https://second.example/file.zip': { version: 3 }, + 'https://existing.example/file.zip': { version: 4 } + }, + 1 + ); + + expect(merged).toBe( + 'https://existing.example/file.zip\n' + + 'https://first.example/file.zip\n' + + 'https://second.example/file.zip' + ); + }); + it('upgrades an existing normal row when the user explicitly fetches it as media', () => { const existing = row({ sourceUrl: 'https://adult.example/watch/123', diff --git a/src/utils/addDownloadMetadata.ts b/src/utils/addDownloadMetadata.ts index d63874e..9464f18 100644 --- a/src/utils/addDownloadMetadata.ts +++ b/src/utils/addDownloadMetadata.ts @@ -27,6 +27,7 @@ export interface AddDownloadDraftRow { status: MetadataStatus; generation: number; requestContextVersion?: number; + requestCookiesOmitted?: boolean; isMedia: boolean; resumable?: boolean; formats?: AddMediaFormat[]; @@ -93,6 +94,7 @@ export const reconcileDownloadRows = ( status: 'loading', generation: preserved.generation + 1, requestContextVersion, + requestCookiesOmitted: false, isMedia: preserved.isMedia || forcedMedia, formats: preserved.isMedia || forcedMedia ? undefined : preserved.formats, selectedFormat: preserved.isMedia || forcedMedia ? undefined : preserved.selectedFormat @@ -120,6 +122,35 @@ export const reconcileDownloadRows = ( }); }; +const comparableUrl = (rawUrl: string): string => { + try { + return new URL(rawUrl).href; + } catch { + return rawUrl.trim(); + } +}; + +export const appendRequestUrlsAfterVersion = ( + rawText: string, + requestContexts: Readonly>, + observedVersion: number +): string => { + const lines = rawText.split('\n').map(line => line.trim()).filter(Boolean); + const seen = new Set(lines.map(comparableUrl)); + const additions = Object.entries(requestContexts) + .filter(([, context]) => context.version > observedVersion) + .sort(([, left], [, right]) => left.version - right.version); + + for (const [url] of additions) { + const identity = comparableUrl(url); + if (seen.has(identity)) continue; + seen.add(identity); + lines.push(url); + } + + return lines.join('\n'); +}; + export const updateRowIfCurrent = ( rows: AddDownloadDraftRow[], id: string,