From 135ba75a699b538629a09256a0d9f63feb6290b1 Mon Sep 17 00:00:00 2001 From: NimBold Date: Tue, 4 Aug 2026 19:37:25 +0330 Subject: [PATCH] fix(properties): harden resume lifecycle --- src-tauri/src/lib.rs | 139 +++++++++++++++++- src-tauri/src/queue.rs | 14 +- src-tauri/tests/queue_manager.rs | 57 +++++++ src/components/PropertiesWindowApp.tsx | 32 +++- src/components/PropertiesWindowBridgeHost.tsx | 43 ++++-- src/propertiesBridge.test.ts | 37 +++++ src/propertiesBridge.ts | 20 +++ 7 files changed, 318 insertions(+), 24 deletions(-) diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index c87dafc..f438649 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -4873,6 +4873,9 @@ async fn resume_download( ) .await; if !parked { + queue_manager + .release_aria2_permit_candidate(&id_clone, lifecycle_generation) + .await; log::warn!( "aria2 resume [{}]: permit ownership was not established before unpause; leaving gid {} paused", id_clone, @@ -4881,13 +4884,6 @@ async fn resume_download( return; } } - let _ = app_handle_clone.emit( - "download-state", - crate::ipc::DownloadStateEvent::new( - &id_clone, - crate::ipc::DownloadStatus::Downloading, - ), - ); let unpause_error = match rpc_call( aria2_port, &aria2_secret, @@ -4902,8 +4898,31 @@ async fn resume_download( Err(error) => Some(format!("failed to resume aria2 gid {gid_clone}: {error}")), }; if let Some(unpause_error) = unpause_error { - match aria2_download_status(aria2_port, &aria2_secret, &gid_clone).await { + match verify_aria2_resume_status(aria2_port, &aria2_secret, &gid_clone).await { Ok(status) if matches!(status.as_str(), "active" | "waiting") => { + let still_current = queue_manager + .is_aria2_control_epoch_current(&id_clone, control_epoch) + .await + && queue_manager.aria2_gid_for_download(&id_clone).as_deref() + == Some(gid_clone.as_str()); + if !still_current { + let _ = rpc_call( + aria2_port, + &aria2_secret, + "aria2.forcePause", + serde_json::json!([gid_clone]), + ) + .await; + return; + } + use tauri::Emitter; + let _ = app_handle_clone.emit( + "download-state", + crate::ipc::DownloadStateEvent::new( + &id_clone, + crate::ipc::DownloadStatus::Downloading, + ), + ); log::warn!( "aria2 resume [{}]: {} but daemon reports gid {} as {}; retaining permit", id_clone, @@ -4985,6 +5004,79 @@ async fn resume_download( } } } + // A successful unpause RPC is not itself the postcondition: + // aria2 may still report the GID as paused, complete, or + // otherwise unavailable. Verify the daemon state before + // publishing Downloading to the renderer. + let status_after_unpause = match verify_aria2_resume_status( + aria2_port, + &aria2_secret, + &gid_clone, + ) + .await + { + Ok(status) => status, + Err(error) => { + log::error!( + "aria2 resume [{}]: unpause succeeded but gid {} could not be verified: {}; retaining permit", + id_clone, + gid_clone, + error + ); + return; + } + }; + match status_after_unpause.as_str() { + "active" | "waiting" => {} + "complete" => { + queue_manager + .apply_completion_locked( + &id_clone, + crate::queue::PendingOutcome::Complete, + ) + .await; + return; + } + "error" | "removed" => { + let terminal_error = format!( + "aria2 resume left gid {gid_clone} in terminal state {status_after_unpause}" + ); + queue_manager + .apply_completion_locked( + &id_clone, + crate::queue::PendingOutcome::Error(terminal_error), + ) + .await; + return; + } + "paused" => { + queue_manager.next_aria2_control_epoch(&id_clone).await; + queue_manager.cancel_aria2_retries(&id_clone).await; + queue_manager.release_permit(&id_clone).await; + let error = "aria2 kept the download paused after resume".to_string(); + log::error!( + "aria2 resume [{}]: {}; gid {} remains paused", + id_clone, + error, + gid_clone + ); + let _ = app_handle_clone.emit( + "download-state", + crate::ipc::DownloadStateEvent::paused_with_error(&id_clone, error), + ); + return; + } + other => { + log::error!( + "aria2 resume [{}]: unpause left gid {} in unexpected state {}; retaining permit", + id_clone, + gid_clone, + other + ); + return; + } + } + let current_epoch = queue_manager .is_aria2_control_epoch_current(&id_clone, control_epoch) .await; @@ -5002,6 +5094,14 @@ async fn resume_download( .await; return; } + use tauri::Emitter; + let _ = app_handle_clone.emit( + "download-state", + crate::ipc::DownloadStateEvent::new( + &id_clone, + crate::ipc::DownloadStatus::Downloading, + ), + ); log::info!("aria2 resume [{}]: unpaused gid {}", id_clone, gid_clone); }); Ok(true) @@ -5067,6 +5167,9 @@ async fn resume_download( ) .await; if !parked && !queue_manager.has_active_permit(&id_clone).await { + queue_manager + .release_aria2_permit_candidate(&id_clone, lifecycle_generation) + .await; return; } } @@ -5521,6 +5624,26 @@ async fn aria2_download_status(port: u16, secret: &str, gid: &str) -> Result Result { + let mut last_observation = Err(format!("aria2 resume status for gid {gid} was not observed")); + for attempt in 0..4u32 { + last_observation = match aria2_download_status(port, secret, gid).await { + Ok(status) if status != "paused" => return Ok(status), + Ok(status) => Ok(status), + Err(error) => Err(error), + }; + if attempt < 3 { + tokio::time::sleep(Duration::from_millis(25 * (1_u64 << attempt))).await; + } + } + last_observation +} + async fn reconcile_aria2_downloads(app_handle: &tauri::AppHandle) { let state = app_handle.state::(); let port = state.aria2_port.load(std::sync::atomic::Ordering::Relaxed); diff --git a/src-tauri/src/queue.rs b/src-tauri/src/queue.rs index 0c0ea92..6e82603 100644 --- a/src-tauri/src/queue.rs +++ b/src-tauri/src/queue.rs @@ -3421,9 +3421,15 @@ impl QueueManager { if let Some(epoch) = aria2_lifecycle_epoch { self.begin_aria2_dispatch(&id, epoch).await; } - self.emit_state(&id, DownloadStatus::Downloading); drop(control_guard); + // Media runners do not receive an Aria2 GID. Their permit is already + // active at this point, so publish their live state before spawning + // the runner; Aria2 tasks publish only after remember_gid below. + if matches!(&task.kind, TaskKind::Media) { + self.emit_state(&id, DownloadStatus::Downloading); + } + match task.kind { TaskKind::Aria2 => { let lifecycle_epoch = aria2_lifecycle_epoch @@ -3477,7 +3483,13 @@ impl QueueManager { } return; } + // A queued task is not a live transfer until aria2 has + // accepted it and Firelink has installed the GID + // mapping. Emitting Downloading before this point + // lets the UI (and a concurrent Properties pause) + // act on a lifecycle that does not yet exist. let buffered_outcome = self.remember_gid(id.clone(), gid.clone()).await; + self.emit_state(&id, DownloadStatus::Downloading); let install_web_seeds = buffered_outcome.is_none() && task.payload.is_torrent && !task.payload.torrent_verify_only diff --git a/src-tauri/tests/queue_manager.rs b/src-tauri/tests/queue_manager.rs index c566759..f8cfef0 100644 --- a/src-tauri/tests/queue_manager.rs +++ b/src-tauri/tests/queue_manager.rs @@ -1782,6 +1782,7 @@ async fn media_terminal_error_emits_failed_without_completed() { tokio::time::sleep(Duration::from_millis(100)).await; let statuses = emitted_statuses(&event_rx); + assert!(statuses.iter().any(|status| status == "downloading")); assert!(statuses.iter().any(|status| status == "failed")); assert!(!statuses.iter().any(|status| status == "completed")); assert_eq!(manager.available_permits(), 1); @@ -1841,6 +1842,62 @@ async fn aria2_permit_survives_rpc_return() { handle.abort(); } +#[tokio::test] +async fn aria2_does_not_emit_downloading_before_gid_mapping() { + let app = mock_builder() + .build(mock_context(noop_assets())) + .expect("mock app"); + let (gid_started_tx, gid_started_rx) = tokio::sync::oneshot::channel(); + let spawner = Arc::new(DelayedAria2Spawner::new(gid_started_tx)); + let (event_tx, event_rx) = std::sync::mpsc::channel(); + app.handle().listen("download-state", move |event| { + let _ = event_tx.send(event.payload().to_string()); + }); + let manager = Arc::new(QueueManager::test_new(app.handle().clone(), 1, spawner)); + manager.push(aria2_task("delayed-start")).await.unwrap(); + + let dispatcher = { + let manager = Arc::clone(&manager); + tokio::spawn(async move { manager.run_dispatcher().await }) + }; + + gid_started_rx + .await + .expect("add_uri should begin before the delayed GID is returned"); + let early_statuses = emitted_statuses(&event_rx); + assert!( + !early_statuses.iter().any(|status| status == "downloading"), + "a queued task must not be reported as downloading before its GID is mapped" + ); + + timeout(Duration::from_secs(1), async { + loop { + if manager.aria2_gid_for_download("delayed-start").is_some() { + break; + } + tokio::task::yield_now().await; + } + }) + .await + .expect("the delayed GID should eventually be mapped"); + timeout(Duration::from_secs(1), async { + loop { + if emitted_statuses(&event_rx) + .iter() + .any(|status| status == "downloading") + { + break; + } + tokio::task::yield_now().await; + } + }) + .await + .expect("the transfer should become downloading only after its GID is owned"); + + manager.release_permit("delayed-start").await; + dispatcher.abort(); +} + #[tokio::test] async fn failed_refresh_that_leaves_gid_paused_releases_permit_but_keeps_resume_mapping() { let app = mock_builder() diff --git a/src/components/PropertiesWindowApp.tsx b/src/components/PropertiesWindowApp.tsx index 575804e..feb008c 100644 --- a/src/components/PropertiesWindowApp.tsx +++ b/src/components/PropertiesWindowApp.tsx @@ -14,6 +14,7 @@ import { PROPERTIES_WINDOW_ACTION_RESULT, PROPERTIES_WINDOW_REMOVED, PROPERTIES_WINDOW_SNAPSHOT, + attachAsyncPropertiesListener, getPropertiesLifecycleAction, sendPropertiesActionRequest, sendPropertiesReady, @@ -190,7 +191,7 @@ export const PropertiesWindowApp = () => { const id = await invoke('get_properties_window_download_id'); if (cancelled) return; setDownloadId(id); - unlistenSnapshot = await listen(PROPERTIES_WINDOW_SNAPSHOT, async event => { + const snapshotListener = await listen(PROPERTIES_WINDOW_SNAPSHOT, async event => { if (event.payload.windowLabel !== windowLabel || event.payload.downloadId !== id || event.payload.sessionId !== sessionId) return; @@ -220,7 +221,12 @@ export const PropertiesWindowApp = () => { } } }); - unlistenResult = await listen(PROPERTIES_WINDOW_ACTION_RESULT, event => { + if (cancelled) { + snapshotListener(); + return; + } + unlistenSnapshot = snapshotListener; + const resultListener = await listen(PROPERTIES_WINDOW_ACTION_RESULT, event => { if (event.payload.windowLabel !== windowLabel || event.payload.downloadId !== id || event.payload.sessionId !== sessionId) return; @@ -248,7 +254,12 @@ export const PropertiesWindowApp = () => { } } }); - unlistenRemoved = await listen<{ windowLabel: string; downloadId: string }>(PROPERTIES_WINDOW_REMOVED, event => { + if (cancelled) { + resultListener(); + return; + } + unlistenResult = resultListener; + const removedListener = await listen<{ windowLabel: string; downloadId: string }>(PROPERTIES_WINDOW_REMOVED, event => { if (event.payload.windowLabel === windowLabel && event.payload.downloadId === id) { if (readyRetryTimer !== undefined) { window.clearInterval(readyRetryTimer); @@ -258,6 +269,11 @@ export const PropertiesWindowApp = () => { setNotice(t($ => $.downloadTable.noDownloads)); } }); + if (cancelled) { + removedListener(); + return; + } + unlistenRemoved = removedListener; await sendPropertiesReady(sessionId); if (cancelled) return; // Tauri event listeners are registered asynchronously. If the main @@ -315,12 +331,16 @@ export const PropertiesWindowApp = () => { useEffect(() => { if (!isDirty) return; + let disposed = false; let unlisten: UnlistenFn | undefined; - void currentWindow.onCloseRequested(event => { + attachAsyncPropertiesListener(currentWindow.onCloseRequested(event => { event.preventDefault(); setClosePrompt(true); - }).then(value => { unlisten = value; }); - return () => unlisten?.(); + }), () => disposed, value => { unlisten = value; }); + return () => { + disposed = true; + unlisten?.(); + }; }, [currentWindow, isDirty]); const requestAction = useCallback(async ( diff --git a/src/components/PropertiesWindowBridgeHost.tsx b/src/components/PropertiesWindowBridgeHost.tsx index aeff1f4..2b6ad80 100644 --- a/src/components/PropertiesWindowBridgeHost.tsx +++ b/src/components/PropertiesWindowBridgeHost.tsx @@ -14,6 +14,7 @@ import { PROPERTIES_WINDOW_CLOSED, PROPERTIES_WINDOW_READY, applySecretPatch, + attachAsyncPropertiesListener, beginExclusivePropertiesAction, createFrameCoalescer, enqueuePropertiesAction, @@ -176,8 +177,10 @@ export const PropertiesWindowBridgeHost = () => { }; const handleReady = async (payload: PropertiesWindowReady) => { + if (disposed) return; try { await invoke('validate_properties_window_request', payload); + if (disposed) return; const item = useDownloadStore.getState().downloads.find(download => download.id === payload.downloadId); if (!item) { await sendPropertiesRemoved(payload.windowLabel, payload.downloadId); @@ -192,12 +195,14 @@ export const PropertiesWindowBridgeHost = () => { }; const processAction = async (request: PropertiesActionRequest) => { + if (disposed) return; let ok = false; let error: string | undefined; const actionKey = `${request.windowLabel}:${request.downloadId}`; let releaseAction: (() => void) | undefined; try { await invoke('validate_properties_window_request', request); + if (disposed) return; const registration = windows.get(request.windowLabel); if (!registration || registration.downloadId !== request.downloadId @@ -302,6 +307,7 @@ export const PropertiesWindowBridgeHost = () => { } finally { releaseAction?.(); } + if (disposed) return; if (ok) { try { await sendFor(request.windowLabel, request.downloadId); @@ -325,12 +331,14 @@ export const PropertiesWindowBridgeHost = () => { }; const handleAction = async (request: PropertiesActionRequest) => { + if (disposed) return; const actionKey = `${request.windowLabel}:${request.downloadId}`; try { // The native command validates the caller, download binding, and // renderer session. If a ready event is delayed or lost, this valid // action can also establish the main-window registration. await invoke('validate_properties_window_request', request); + if (disposed) return; synchronizeRegistration(request.windowLabel, request.downloadId, request.sessionId); } catch { // Stale renderer actions are deliberately ignored. The current child @@ -349,15 +357,32 @@ export const PropertiesWindowBridgeHost = () => { await enqueuePropertiesAction(actionChains, actionKey, () => processAction(request)); }; - void listen(PROPERTIES_WINDOW_READY, event => void handleReady(event.payload)).then(value => { unlistenReady = value; }); - void listen(PROPERTIES_WINDOW_ACTION_REQUEST, event => void handleAction(event.payload)).then(value => { unlistenAction = value; }); - void listen(PROPERTIES_WINDOW_CLOSED, event => { - const registration = windows.get(event.payload); - windows.delete(event.payload); - snapshotRevisions.delete(event.payload); - snapshotCoalescer.cancel(event.payload); - if (registration) actionsInFlight.delete(`${event.payload}:${registration.downloadId}`); - }).then(value => { unlistenClosed = value; }); + attachAsyncPropertiesListener( + listen(PROPERTIES_WINDOW_READY, event => { + if (!disposed) void handleReady(event.payload); + }), + () => disposed, + value => { unlistenReady = value; }, + ); + attachAsyncPropertiesListener( + listen(PROPERTIES_WINDOW_ACTION_REQUEST, event => { + if (!disposed) void handleAction(event.payload); + }), + () => disposed, + value => { unlistenAction = value; }, + ); + attachAsyncPropertiesListener( + listen(PROPERTIES_WINDOW_CLOSED, event => { + if (disposed) return; + const registration = windows.get(event.payload); + windows.delete(event.payload); + snapshotRevisions.delete(event.payload); + snapshotCoalescer.cancel(event.payload); + if (registration) actionsInFlight.delete(`${event.payload}:${registration.downloadId}`); + }), + () => disposed, + value => { unlistenClosed = value; }, + ); const unsubscribeStore = useDownloadStore.subscribe((state, previous) => { for (const [windowLabel, registration] of windows) { diff --git a/src/propertiesBridge.test.ts b/src/propertiesBridge.test.ts index 379526e..1968c70 100644 --- a/src/propertiesBridge.test.ts +++ b/src/propertiesBridge.test.ts @@ -12,6 +12,7 @@ vi.mock('@tauri-apps/api/event', () => ({ import { applySecretPatch, + attachAsyncPropertiesListener, beginExclusivePropertiesAction, createFrameCoalescer, enqueuePropertiesAction, @@ -130,6 +131,7 @@ describe('Properties window bridge', () => { it('derives truthful lifecycle commands from the current status', () => { expect(getPropertiesLifecycleAction('downloading')).toBe('pause'); + expect(getPropertiesLifecycleAction('queued')).toBe('pause'); expect(getPropertiesLifecycleAction('retrying')).toBe('pause'); expect(getPropertiesLifecycleAction('paused')).toBe('resume'); expect(getPropertiesLifecycleAction('ready')).toBe('start'); @@ -255,4 +257,39 @@ describe('Properties window bridge', () => { coalescer.cancelAll(); expect(frames.size).toBe(0); }); + + it('unlistens a Tauri listener that resolves after bridge cleanup', async () => { + let resolveListener!: (unlisten: () => void) => void; + const listener = new Promise<() => void>(resolve => { resolveListener = resolve; }); + let disposed = true; + let assigned = false; + let unlistened = false; + + attachAsyncPropertiesListener( + listener, + () => disposed, + () => { assigned = true; }, + ); + resolveListener(() => { unlistened = true; }); + await listener; + await Promise.resolve(); + + expect(assigned).toBe(false); + expect(unlistened).toBe(true); + }); + + it('assigns a live Tauri listener while the bridge is mounted', async () => { + let resolveListener!: (unlisten: () => void) => void; + const listener = new Promise<() => void>(resolve => { resolveListener = resolve; }); + const unlisten = vi.fn(); + let assigned: (() => void) | undefined; + + attachAsyncPropertiesListener(listener, () => false, value => { assigned = value; }); + resolveListener(unlisten); + await listener; + await Promise.resolve(); + + expect(assigned).toBe(unlisten); + expect(unlisten).not.toHaveBeenCalled(); + }); }); diff --git a/src/propertiesBridge.ts b/src/propertiesBridge.ts index b062399..166101b 100644 --- a/src/propertiesBridge.ts +++ b/src/propertiesBridge.ts @@ -1,4 +1,5 @@ import { emitTo } from '@tauri-apps/api/event'; +import type { UnlistenFn } from '@tauri-apps/api/event'; import type { DownloadProgressEvent } from './bindings/DownloadProgressEvent'; import type { DownloadStatus } from './bindings/DownloadStatus'; import type { DownloadItem } from './store/useDownloadStore'; @@ -303,6 +304,25 @@ export const createFrameCoalescer = ( }; }; +// Tauri listener registration is asynchronous. React StrictMode can unmount +// an effect before `listen()` resolves; in that case assigning the late +// unlisten callback after cleanup leaks a second bridge listener. A leaked +// Properties host can process one click twice, observe the queued state from +// the first action, and turn the intended resume into an immediate pause. +export const attachAsyncPropertiesListener = ( + listener: Promise, + isDisposed: () => boolean, + assign: (unlisten: T) => void, +): void => { + void listener.then(unlisten => { + if (isDisposed()) { + unlisten(); + return; + } + assign(unlisten); + }).catch(() => undefined); +}; + export const openPropertiesWindow = (downloadId: string): Promise => invoke('open_download_properties_window', { id: downloadId });