From cfe929680c861a8060f9567797afdee03b16f119 Mon Sep 17 00:00:00 2001 From: NimBold Date: Thu, 27 Aug 2026 07:48:38 +0330 Subject: [PATCH] fix(torrents): harden unfinished asset cleanup - recursively prune empty Torrent output directories without deleting unrelated content - handle metadata-named directories and fail closed on links or substitutions - flush current persisted status after in-flight dispatch before Delete File removal - add recursive cleanup and persistence race regressions --- src-tauri/src/lib.rs | 279 +++++++++++++++++++++-------- src/store/useDownloadStore.test.ts | 190 ++++++++++++++++++++ src/store/useDownloadStore.ts | 16 +- 3 files changed, 413 insertions(+), 72 deletions(-) diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index 081e8bb..7cc6385 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -7117,44 +7117,83 @@ async fn remove_download_container_assets_permanently( // unlinking, so a later substitution still fails closed. validate_download_sidecars_permanently(primary, app_handle).await?; - let mut entries = tokio::fs::read_dir(primary) - .await - .map_err(|error| format!("could not inspect Torrent output directory: {error}"))?; - let mut metadata_entries = Vec::new(); + // Inspect the complete tree without following links. If a real unowned + // entry is found, stop before deleting anything in the container. This + // preserves the whole user-owned subtree rather than trying to clean + // around it. + let mut pending = vec![(primary.to_path_buf(), false)]; + let mut empty_directories = Vec::new(); + let mut metadata_files = Vec::new(); let mut has_unrelated_entries = false; - while let Some(entry) = entries - .next_entry() - .await - .map_err(|error| format!("could not inspect Torrent output directory: {error}"))? - { - let path = entry.path(); - let entry_metadata = tokio::fs::symlink_metadata(&path) - .await - .map_err(|error| format!("could not inspect Torrent output entry: {error}"))?; - if metadata_is_link_or_reparse(&entry_metadata) { - return Err(format!( - "refusing to remove Torrent output directory containing symbolic link '{}'", - path.display() - )); - } - if !is_os_directory_metadata(&entry.file_name()) { - has_unrelated_entries = true; + 'inspect: while let Some((directory, visited)) = pending.pop() { + if visited { + empty_directories.push(directory); continue; } - if !entry_metadata.is_file() { - return Err(format!( - "refusing to remove non-file OS metadata entry '{}'", - path.display() - )); + let mut entries = match tokio::fs::read_dir(&directory).await { + Ok(entries) => entries, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => { + continue; + } + Err(error) => { + return Err(format!( + "could not inspect Torrent output directory '{}': {error}", + directory.display() + )); + } + }; + let mut child_directories = Vec::new(); + while let Some(entry) = entries + .next_entry() + .await + .map_err(|error| format!("could not inspect Torrent output directory: {error}"))? + { + let path = entry.path(); + let entry_metadata = match tokio::fs::symlink_metadata(&path).await { + Ok(metadata) => metadata, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => continue, + Err(error) => { + return Err(format!( + "could not inspect Torrent output entry '{}': {error}", + path.display() + )); + } + }; + if metadata_is_link_or_reparse(&entry_metadata) { + return Err(format!( + "refusing to remove Torrent output directory containing symbolic link '{}'", + path.display() + )); + } + if entry_metadata.is_dir() { + child_directories.push(path); + } else if is_os_directory_metadata(&entry.file_name()) { + if !entry_metadata.is_file() { + return Err(format!( + "refusing to remove non-file OS metadata entry '{}'", + path.display() + )); + } + validate_exact_file_for_permanent_removal(&path, app_handle).await?; + metadata_files.push(path); + } else { + has_unrelated_entries = true; + break 'inspect; + } } - validate_exact_file_for_permanent_removal(&path, app_handle).await?; - metadata_entries.push(path); + + pending.push((directory, true)); + pending.extend( + child_directories + .into_iter() + .rev() + .map(|child| (child, false)), + ); } + // Sidecars are still exact, independently owned assets, so remove them + // even when an unrelated entry means that the output container remains. remove_download_sidecars_permanently(primary, app_handle).await?; - for path in metadata_entries { - remove_exact_file_permanently(&path, app_handle).await?; - } if has_unrelated_entries { log::debug!( "keeping Torrent output directory '{}': unrelated entries remain", @@ -7163,29 +7202,33 @@ async fn remove_download_container_assets_permanently( return Ok(()); } - for attempt in 0..=5 { - match tokio::fs::remove_dir(primary).await { - Ok(()) => return Ok(()), - Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(()), - Err(error) if error.kind() == std::io::ErrorKind::DirectoryNotEmpty => { - // A user or another process may have added an unrelated file - // after inspection. Preserve it and leave the directory in - // place; owned files were already removed exactly. - log::debug!( - "keeping Torrent output directory '{}': it became non-empty during cleanup", - primary.display() - ); - return Ok(()); - } - Err(error) if attempt == 5 => { - return Err(format!( - "could not permanently remove Torrent output directory '{}' after retries: {error}", - primary.display() - )); - } - Err(_) => tokio::time::sleep(std::time::Duration::from_millis(200)).await, + for path in metadata_files { + remove_exact_file_permanently(&path, app_handle).await?; + } + + // remove_dir is deliberately non-recursive. The visit markers put child + // directories before their parents, so a file or directory added after + // inspection makes that directory (and then its parents) fail closed + // without deleting newly-created unowned content. + for directory in &empty_directories { + let removed = remove_empty_directory_permanently(directory).await?; + if crate::platform::paths_equal(directory, primary) && !removed { + log::debug!( + "keeping Torrent output directory '{}': it became non-empty during cleanup", + primary.display() + ); } } + + if !empty_directories + .iter() + .any(|directory| crate::platform::paths_equal(directory, primary)) + { + log::debug!( + "keeping Torrent output directory '{}': it disappeared during cleanup", + primary.display() + ); + } Ok(()) } @@ -7257,28 +7300,73 @@ fn is_os_directory_metadata(name: &std::ffi::OsStr) -> bool { } async fn directory_has_non_metadata_entries(path: &std::path::Path) -> Result { - match tokio::fs::read_dir(path).await { - Ok(mut entries) => loop { - match entries.next_entry().await { - Ok(Some(entry)) if is_os_directory_metadata(&entry.file_name()) => continue, - Ok(Some(_)) => break Ok(true), - Ok(None) => break Ok(false), + let mut directories = vec![path.to_path_buf()]; + while let Some(directory) = directories.pop() { + let mut entries = match tokio::fs::read_dir(&directory).await { + Ok(entries) => entries, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => continue, + Err(error) => { + return Err(format!( + "could not inspect directory '{}': {}", + directory.display(), + error + )); + } + }; + while let Some(entry) = entries.next_entry().await.map_err(|error| { + format!( + "could not inspect directory '{}': {}", + directory.display(), + error + ) + })? { + let metadata = match tokio::fs::symlink_metadata(entry.path()).await { + Ok(metadata) => metadata, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => continue, Err(error) => { - break Err(format!( - "could not inspect directory '{}': {}", - path.display(), + return Err(format!( + "could not inspect directory entry '{}': {}", + entry.path().display(), error )); } + }; + if metadata_is_link_or_reparse(&metadata) { + return Ok(true); } - }, - Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(false), - Err(error) => Err(format!( - "could not inspect directory '{}': {}", - path.display(), - error - )), + if metadata.is_dir() { + directories.push(entry.path()); + } else if is_os_directory_metadata(&entry.file_name()) { + // These names are ignorable only for regular OS metadata + // files. A directory with a metadata-like name may contain + // real user content and must be inspected normally. + continue; + } else { + return Ok(true); + } + } } + Ok(false) +} + +async fn remove_empty_directory_permanently(path: &std::path::Path) -> Result { + for attempt in 0..=5 { + match tokio::fs::remove_dir(path).await { + Ok(()) => return Ok(true), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(true), + Err(error) if error.kind() == std::io::ErrorKind::DirectoryNotEmpty => { + return Ok(false); + } + Err(error) if attempt == 5 => { + return Err(format!( + "could not permanently remove empty Torrent directory '{}' after retries: {error}", + path.display() + )); + } + Err(_) => tokio::time::sleep(std::time::Duration::from_millis(200)).await, + } + } + Ok(false) } async fn remove_download_container_assets( @@ -16224,10 +16312,26 @@ mod tests { assert!(!directory_has_non_metadata_entries(directory.path()) .await .unwrap()); - std::fs::write(directory.path().join("MD5"), b"unselected").unwrap(); + let empty_nested = directory.path().join("MD5").join("nested"); + std::fs::create_dir_all(&empty_nested).unwrap(); + assert!(!directory_has_non_metadata_entries(directory.path()) + .await + .unwrap()); + std::fs::write(empty_nested.join("unselected"), b"unselected").unwrap(); assert!(directory_has_non_metadata_entries(directory.path()) .await .unwrap()); + + let metadata_named_root = tempfile::tempdir().unwrap(); + let metadata_named_directory = metadata_named_root.path().join(".localized"); + std::fs::create_dir_all(&metadata_named_directory).unwrap(); + std::fs::write(metadata_named_directory.join("unselected"), b"unselected").unwrap(); + assert!( + directory_has_non_metadata_entries(metadata_named_root.path()) + .await + .unwrap() + ); + drop(metadata_named_root); } #[test] @@ -16664,7 +16768,8 @@ mod tests { let container = directory.path().join("torrent-output"); std::fs::create_dir(&container).unwrap(); std::fs::write(container.join(".DS_Store"), b"metadata").unwrap(); - let unrelated = container.join("unselected.bin"); + let unrelated = container.join("unselected").join("unselected.bin"); + std::fs::create_dir_all(unrelated.parent().unwrap()).unwrap(); std::fs::write(&unrelated, b"preserve").unwrap(); remove_download_container_assets_permanently(&container, app.handle()) @@ -16676,6 +16781,40 @@ mod tests { drop(directory); } + #[tokio::test] + async fn permanent_torrent_container_cleanup_removes_empty_nested_directories() { + use tauri::Manager; + + let app = tauri::test::mock_builder() + .build(tauri::test::mock_context(tauri::test::noop_assets())) + .expect("mock app"); + let storage_root = tempfile::tempdir_in(std::env::current_dir().unwrap()).unwrap(); + let storage_layout = crate::storage::StorageLayout::resolve( + app.handle(), + crate::storage::StorageMode::Portable { + root: storage_root.path().to_path_buf(), + }, + ) + .unwrap(); + app.manage(crate::db::init(&storage_layout).unwrap()); + let download_root = + configure_test_download_root(app.handle(), &storage_root.path().join("downloads")); + let directory = tempfile::tempdir_in(&download_root).unwrap(); + let container = directory.path().join("torrent-output"); + let empty_nested = container.join("MD5").join("nested"); + std::fs::create_dir_all(&empty_nested).unwrap(); + std::fs::create_dir(container.join(".localized")).unwrap(); + std::fs::write(container.join(".DS_Store"), b"metadata").unwrap(); + std::fs::write(container.join("MD5").join(".DS_Store"), b"metadata").unwrap(); + + remove_download_container_assets_permanently(&container, app.handle()) + .await + .expect("an empty Torrent output tree should be permanently removable"); + + assert!(!container.exists()); + drop(directory); + } + #[cfg(target_os = "macos")] #[test] fn dock_badge_updates_reject_stale_sessions_and_generations() { diff --git a/src/store/useDownloadStore.test.ts b/src/store/useDownloadStore.test.ts index a976f46..dcd5caa 100644 --- a/src/store/useDownloadStore.test.ts +++ b/src/store/useDownloadStore.test.ts @@ -2196,6 +2196,64 @@ describe('useDownloadStore', () => { } }); + it('waits for in-flight persistence before flushing the current state', async () => { + const id = 'flush-current-state'; + const completed = { + id, + url: 'https://example.com/file', + fileName: 'file', + status: 'completed' as const, + category: 'Other' as const, + dateAdded: '' + }; + useDownloadStore.setState({ downloads: [completed] as any[] }); + const disposePersistence = initializeDownloadPersistence('main'); + const events: string[] = []; + let releaseDownloadingCommit!: () => void; + let signalDownloadingCommitStarted!: () => void; + const downloadingCommitStarted = new Promise(resolve => { + signalDownloadingCommitStarted = resolve; + }); + const downloadingCommitGate = new Promise(resolve => { + releaseDownloadingCommit = resolve; + }); + vi.mocked(ipc.invokeCommand).mockImplementation(async (command: string, args?: any) => { + if (command === 'db_commit_download_state') { + const status = (JSON.parse(args.downloadsData) as Array<{ status: string }>)[0]?.status ?? 'empty'; + events.push(`commit:${status}`); + if (status === 'downloading') { + signalDownloadingCommitStarted(); + await downloadingCommitGate; + } + return undefined; + } + return undefined; + }); + + try { + await flushDownloadPersistence(); + events.length = 0; + useDownloadStore.getState().updateDownload(id, { status: 'downloading' }); + await downloadingCommitStarted; + useDownloadStore.getState().updateDownload(id, { status: 'completed' }); + + let flushResolved = false; + const flushing = flushDownloadPersistence().then(() => { + flushResolved = true; + }); + await Promise.resolve(); + await Promise.resolve(); + expect(flushResolved).toBe(false); + + releaseDownloadingCommit(); + await flushing; + expect(events).toEqual(['commit:downloading', 'commit:completed']); + } finally { + releaseDownloadingCommit(); + disposePersistence(); + } + }); + it('waits for durable queued state before resuming an existing lifecycle', async () => { useDownloadStore.setState({ downloads: [{ @@ -3873,6 +3931,138 @@ describe('useDownloadStore', () => { }); }); + it('flushes the current Delete File status before invoking native removal', async () => { + const disposePersistence = initializeDownloadPersistence('main'); + const events: string[] = []; + let releaseCommit!: () => void; + let signalCommitStarted!: () => void; + const commitStarted = new Promise(resolve => { + signalCommitStarted = resolve; + }); + const commitGate = new Promise(resolve => { + releaseCommit = resolve; + }); + vi.mocked(ipc.invokeCommand).mockImplementation(async (command: string, args?: any) => { + if (command === 'db_commit_download_state') { + const status = (JSON.parse(args.downloadsData) as Array<{ status: string }>)[0]?.status ?? 'empty'; + events.push(`commit:${status}`); + signalCommitStarted(); + await commitGate; + return undefined; + } + if (command === 'remove_download') { + events.push('remove'); + } + return undefined; + }); + useDownloadStore.setState({ + downloads: [ + { id: 'completed-delete', url: 'https://example.com/file', fileName: 'file', status: 'completed', category: 'Other', dateAdded: '' } + ] as any[] + }); + + try { + await commitStarted; + const removing = useDownloadStore.getState().removeDownload( + 'completed-delete', + true, + false, + 'permanentIfUnfinished' + ); + + await Promise.resolve(); + expect(events).toEqual(['commit:completed']); + + releaseCommit(); + await removing; + expect(events).toEqual(['commit:completed', 'remove', 'commit:empty']); + } finally { + releaseCommit(); + disposePersistence(); + } + }); + + it('waits for an in-flight dispatch before flushing Delete File status', async () => { + useDownloadStore.setState({ + downloads: [ + { + id: 'dispatch-delete-race', + url: 'https://example.com/file', + fileName: 'file', + destination: '/tmp', + status: 'ready', + category: 'Other', + dateAdded: '' + } + ] as any[] + }); + const disposePersistence = initializeDownloadPersistence('main'); + const events: string[] = []; + let releaseEnqueue!: () => void; + let signalEnqueueStarted!: () => void; + let releaseCompletedCommit!: () => void; + let signalCompletedCommitStarted!: () => void; + const enqueueStarted = new Promise(resolve => { + signalEnqueueStarted = resolve; + }); + const enqueueGate = new Promise(resolve => { + releaseEnqueue = resolve; + }); + const completedCommitStarted = new Promise(resolve => { + signalCompletedCommitStarted = resolve; + }); + const completedCommitGate = new Promise(resolve => { + releaseCompletedCommit = resolve; + }); + + vi.mocked(ipc.invokeCommand).mockImplementation(async (command: string, args?: any) => { + if (command === 'db_commit_download_state') { + const status = (JSON.parse(args.downloadsData) as Array<{ status: string }>)[0]?.status ?? 'empty'; + events.push(`commit:${status}`); + if (status === 'completed') { + signalCompletedCommitStarted(); + await completedCommitGate; + } + return undefined; + } + if (command === 'enqueue_download') { + signalEnqueueStarted(); + await enqueueGate; + useDownloadStore.getState().updateDownload('dispatch-delete-race', { status: 'completed' }); + return { id: 'dispatch-delete-race', filename: 'file' }; + } + if (command === 'remove_download') { + events.push(args.deleteAssets ? 'remove-user' : 'remove-stale'); + } + if (command === 'get_pending_order') return []; + return undefined; + }); + + try { + const dispatching = dispatchItem('dispatch-delete-race'); + await enqueueStarted; + const removing = useDownloadStore.getState().removeDownload( + 'dispatch-delete-race', + true, + false, + 'permanentIfUnfinished' + ); + + releaseEnqueue(); + await completedCommitStarted; + await expect(dispatching).resolves.toBe(false); + expect(events).not.toContain('remove-user'); + + releaseCompletedCommit(); + await removing; + expect(events.indexOf('commit:completed')).toBeLessThan(events.indexOf('remove-user')); + } finally { + releaseEnqueue(); + releaseCompletedCommit(); + disposePersistence(); + } + }); + it('starts staged queue items in their persisted queue order', async () => { useDownloadStore.setState({ downloads: [ diff --git a/src/store/useDownloadStore.ts b/src/store/useDownloadStore.ts index 8f2db2a..26f641d 100644 --- a/src/store/useDownloadStore.ts +++ b/src/store/useDownloadStore.ts @@ -2061,6 +2061,13 @@ export const useDownloadStore = create((set, get) => { if (pendingDispatch) { await pendingDispatch; } + // The native command classifies PermanentIfUnfinished from the durable + // row. Flush the current UI snapshot only after invalidating and joining + // any pending dispatch, so a status transition from that dispatch cannot + // leave SQLite behind the state used for native removal. + if (deleteFile && !preserveResumable && assetRemovalPolicy === 'permanentIfUnfinished') { + await flushDownloadPersistence(); + } const item = get().downloads.find(d => d.id === id); if (item) { @@ -3303,9 +3310,14 @@ export const flushDownloadPersistence = async (): Promise => { if (!downloadPersistenceReady) return; while (true) { const snapshot = persistenceSnapshotForState(useDownloadStore.getState()); - if (snapshot.key === lastCommittedPersistenceKey) return; await queuePersistenceSnapshot(snapshot); - if (persistenceSnapshotForState(useDownloadStore.getState()).key === lastCommittedPersistenceKey) { + const current = persistenceSnapshotForState(useDownloadStore.getState()); + if ( + current.key === snapshot.key && + current.key === lastCommittedPersistenceKey && + !persistenceSaveInFlight && + nextPersistenceSnapshot === null + ) { return; } }