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
This commit is contained in:
NimBold
2026-08-27 07:48:38 +03:30
parent 3a1703889a
commit cfe929680c
3 changed files with 413 additions and 72 deletions
+209 -70
View File
@@ -7117,44 +7117,83 @@ async fn remove_download_container_assets_permanently<R: tauri::Runtime>(
// unlinking, so a later substitution still fails closed. // unlinking, so a later substitution still fails closed.
validate_download_sidecars_permanently(primary, app_handle).await?; validate_download_sidecars_permanently(primary, app_handle).await?;
let mut entries = tokio::fs::read_dir(primary) // Inspect the complete tree without following links. If a real unowned
.await // entry is found, stop before deleting anything in the container. This
.map_err(|error| format!("could not inspect Torrent output directory: {error}"))?; // preserves the whole user-owned subtree rather than trying to clean
let mut metadata_entries = Vec::new(); // 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; let mut has_unrelated_entries = false;
while let Some(entry) = entries 'inspect: while let Some((directory, visited)) = pending.pop() {
.next_entry() if visited {
.await empty_directories.push(directory);
.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;
continue; continue;
} }
if !entry_metadata.is_file() { let mut entries = match tokio::fs::read_dir(&directory).await {
return Err(format!( Ok(entries) => entries,
"refusing to remove non-file OS metadata entry '{}'", Err(error) if error.kind() == std::io::ErrorKind::NotFound => {
path.display() 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?; 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 { if has_unrelated_entries {
log::debug!( log::debug!(
"keeping Torrent output directory '{}': unrelated entries remain", "keeping Torrent output directory '{}': unrelated entries remain",
@@ -7163,29 +7202,33 @@ async fn remove_download_container_assets_permanently<R: tauri::Runtime>(
return Ok(()); return Ok(());
} }
for attempt in 0..=5 { for path in metadata_files {
match tokio::fs::remove_dir(primary).await { remove_exact_file_permanently(&path, app_handle).await?;
Ok(()) => return Ok(()), }
Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(()),
Err(error) if error.kind() == std::io::ErrorKind::DirectoryNotEmpty => { // remove_dir is deliberately non-recursive. The visit markers put child
// A user or another process may have added an unrelated file // directories before their parents, so a file or directory added after
// after inspection. Preserve it and leave the directory in // inspection makes that directory (and then its parents) fail closed
// place; owned files were already removed exactly. // without deleting newly-created unowned content.
log::debug!( for directory in &empty_directories {
"keeping Torrent output directory '{}': it became non-empty during cleanup", let removed = remove_empty_directory_permanently(directory).await?;
primary.display() if crate::platform::paths_equal(directory, primary) && !removed {
); log::debug!(
return Ok(()); "keeping Torrent output directory '{}': it became non-empty during cleanup",
} primary.display()
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,
} }
} }
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(()) 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<bool, String> { async fn directory_has_non_metadata_entries(path: &std::path::Path) -> Result<bool, String> {
match tokio::fs::read_dir(path).await { let mut directories = vec![path.to_path_buf()];
Ok(mut entries) => loop { while let Some(directory) = directories.pop() {
match entries.next_entry().await { let mut entries = match tokio::fs::read_dir(&directory).await {
Ok(Some(entry)) if is_os_directory_metadata(&entry.file_name()) => continue, Ok(entries) => entries,
Ok(Some(_)) => break Ok(true), Err(error) if error.kind() == std::io::ErrorKind::NotFound => continue,
Ok(None) => break Ok(false), 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) => { Err(error) => {
break Err(format!( return Err(format!(
"could not inspect directory '{}': {}", "could not inspect directory entry '{}': {}",
path.display(), entry.path().display(),
error error
)); ));
} }
};
if metadata_is_link_or_reparse(&metadata) {
return Ok(true);
} }
}, if metadata.is_dir() {
Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(false), directories.push(entry.path());
Err(error) => Err(format!( } else if is_os_directory_metadata(&entry.file_name()) {
"could not inspect directory '{}': {}", // These names are ignorable only for regular OS metadata
path.display(), // files. A directory with a metadata-like name may contain
error // 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<bool, String> {
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<R: tauri::Runtime>( async fn remove_download_container_assets<R: tauri::Runtime>(
@@ -16224,10 +16312,26 @@ mod tests {
assert!(!directory_has_non_metadata_entries(directory.path()) assert!(!directory_has_non_metadata_entries(directory.path())
.await .await
.unwrap()); .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()) assert!(directory_has_non_metadata_entries(directory.path())
.await .await
.unwrap()); .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] #[test]
@@ -16664,7 +16768,8 @@ mod tests {
let container = directory.path().join("torrent-output"); let container = directory.path().join("torrent-output");
std::fs::create_dir(&container).unwrap(); std::fs::create_dir(&container).unwrap();
std::fs::write(container.join(".DS_Store"), b"metadata").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(); std::fs::write(&unrelated, b"preserve").unwrap();
remove_download_container_assets_permanently(&container, app.handle()) remove_download_container_assets_permanently(&container, app.handle())
@@ -16676,6 +16781,40 @@ mod tests {
drop(directory); 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")] #[cfg(target_os = "macos")]
#[test] #[test]
fn dock_badge_updates_reject_stale_sessions_and_generations() { fn dock_badge_updates_reject_stale_sessions_and_generations() {
+190
View File
@@ -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<void>(resolve => {
signalDownloadingCommitStarted = resolve;
});
const downloadingCommitGate = new Promise<void>(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 () => { it('waits for durable queued state before resuming an existing lifecycle', async () => {
useDownloadStore.setState({ useDownloadStore.setState({
downloads: [{ 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<void>(resolve => {
signalCommitStarted = resolve;
});
const commitGate = new Promise<void>(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<void>(resolve => {
signalEnqueueStarted = resolve;
});
const enqueueGate = new Promise<void>(resolve => {
releaseEnqueue = resolve;
});
const completedCommitStarted = new Promise<void>(resolve => {
signalCompletedCommitStarted = resolve;
});
const completedCommitGate = new Promise<void>(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 () => { it('starts staged queue items in their persisted queue order', async () => {
useDownloadStore.setState({ useDownloadStore.setState({
downloads: [ downloads: [
+14 -2
View File
@@ -2061,6 +2061,13 @@ export const useDownloadStore = create<DownloadState>((set, get) => {
if (pendingDispatch) { if (pendingDispatch) {
await 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); const item = get().downloads.find(d => d.id === id);
if (item) { if (item) {
@@ -3303,9 +3310,14 @@ export const flushDownloadPersistence = async (): Promise<void> => {
if (!downloadPersistenceReady) return; if (!downloadPersistenceReady) return;
while (true) { while (true) {
const snapshot = persistenceSnapshotForState(useDownloadStore.getState()); const snapshot = persistenceSnapshotForState(useDownloadStore.getState());
if (snapshot.key === lastCommittedPersistenceKey) return;
await queuePersistenceSnapshot(snapshot); 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; return;
} }
} }