From 77c7ec210ecc37010764f98ea4748dc22122077f Mon Sep 17 00:00:00 2001 From: Linuxuser67 <41808005+Linuxuser67@users.noreply.github.com> Date: Mon, 26 Oct 2026 16:00:23 +0100 Subject: [PATCH 1/6] fix: keep legacy restore staging scans off GTK --- src/download.rs | 162 ++++++++++++++++++++++++++++++++---------- src/download_tests.rs | 39 ++++++++++ 2 files changed, 164 insertions(+), 37 deletions(-) diff --git a/src/download.rs b/src/download.rs index 70553cba8..5304f93b3 100644 --- a/src/download.rs +++ b/src/download.rs @@ -758,18 +758,6 @@ impl DownloadManager { id } - /// Fresh id that lands on no existing staging dir: the allocator must skip - /// leftover `grab--*` files in dest dirs and legacy tmp dirs, or a new row - /// would resume (or sweep) a stranger's scratch. - fn alloc_id_for(&self, dest_dir: &std::path::Path) -> u64 { - loop { - let id = self.alloc_id(); - if !crate::video::staging_occupied(dest_dir, id) { - return id; - } - } - } - /// In-memory allocator for callers that listed the destination off GTK. /// Callers must pass a complete listing; an unreadable directory must be /// reported rather than falling back to filesystem I/O on the main thread. @@ -782,20 +770,24 @@ impl DownloadManager { } } - /// Reuse a restored row's persisted id where possible, else allocate. The id is the staging key, so a fresh number would point at the wrong `grab--*` files and risk colliding with a later row. - fn claim_id(&self, stored: Option, dest_dir: &std::path::Path) -> u64 { - let Some(id) = stored else { - return self.alloc_id_for(dest_dir); - }; - // Already owned in this session: a hand-edited/duplicated queue must never share one staging directory. + /// Claim a stored id only if this session has not already assigned it. + fn claim_persisted_id(&self, stored: Option) -> Option { + let id = stored?; + // A hand-edited/duplicated queue must never share one staging identity. if self.find(id).is_some() || self.epoch.borrow().contains_key(&id) { - return self.alloc_id_for(dest_dir); + return None; } - // Keep the allocator ahead of every adopted id so a new row never reuses a restored row's number. + // Keep the allocator ahead of every adopted id so a new row never reuses its number. if id >= self.next_id.get() { self.next_id.set(id + 1); } - id + Some(id) + } + + /// Reuse a persisted id or allocate against a destination snapshot made off GTK. + fn claim_id_in(&self, stored: Option, existing: &[String]) -> u64 { + self.claim_persisted_id(stored) + .unwrap_or_else(|| self.alloc_id_for_in(existing)) } /// The underlying download list. @@ -1375,12 +1367,22 @@ impl DownloadManager { (added, pl.total, skipped_by_host_limit) } - /// Re-queue one persisted entry, preserving its intent (paused/failed stay). Names restore verbatim to keep resume identity. - /// The video-page source is staged *before* insert (see `enqueue_video`). + /// Synchronous restore helper for tests; production passes a prelisted destination. /// /// # Errors /// Returns a display-ready message when the stored entry is invalid. + #[cfg(test)] pub fn restore_existing(self: &Rc, stored: &StoredItem) -> Result { + let existing = + crate::video_staging::dir_file_names(std::path::Path::new(&stored.dest_dir)); + self.restore_existing_with_listing(stored, &existing) + } + + fn restore_existing_with_listing( + self: &Rc, + stored: &StoredItem, + existing: &[String], + ) -> Result { let StoredItem { id: stored_id, url: raw_url, @@ -1406,7 +1408,7 @@ impl DownloadManager { } let filename = shorten_filename(raw_filename); let item = DownloadItem::new( - self.claim_id(*stored_id, std::path::Path::new(dest_dir)), + self.claim_id_in(*stored_id, existing), &url, &filename, dest_dir, @@ -1463,12 +1465,23 @@ impl DownloadManager { Ok(self.insert(item)) } - /// Restore a gallery row from a validated stored entry + parsed job. - /// Mirrors `enqueue_gallery` but reuses the stored id and status. + /// Synchronous gallery restore helper for tests; production passes a prelisted destination. + #[cfg(test)] pub fn restore_existing_gallery( self: &Rc, stored: &StoredItem, job: Option, + ) -> Result { + let existing = + crate::video_staging::dir_file_names(std::path::Path::new(&stored.dest_dir)); + self.restore_existing_gallery_with_listing(stored, job, &existing) + } + + fn restore_existing_gallery_with_listing( + self: &Rc, + stored: &StoredItem, + job: Option, + existing: &[String], ) -> Result { let mut job = job.ok_or_else(|| "gallery entry missing job".to_string())?; let url = normalize_url(&stored.url)?; @@ -1485,7 +1498,7 @@ impl DownloadManager { // load; use it verbatim so the restored row matches the on-disk // subfolder instead of the payload's base title. let title = stored.filename.clone(); - let id = self.claim_id(stored.id, std::path::Path::new(&stored.dest_dir)); + let id = self.claim_id_in(stored.id, existing); job.item_id = id; let item = DownloadItem::new(id, &url, &title, &stored.dest_dir); item.set_public_only(public_only); @@ -1593,7 +1606,12 @@ impl DownloadManager { }); } - fn insert_history(self: &Rc, stored: StoredItem, output_dir: Option) { + fn insert_history( + self: &Rc, + stored: StoredItem, + output_dir: Option, + existing: &[String], + ) { let StoredItem { id: stored_id, url: raw_url, @@ -1616,7 +1634,7 @@ impl DownloadManager { return; } let item = DownloadItem::new( - self.claim_id(stored_id, std::path::Path::new(&dir)), + self.claim_id_in(stored_id, existing), &url, &name, &dir, @@ -4315,6 +4333,34 @@ impl DownloadManager { }) } + /// Find restore rows whose id allocation may need a staging-name scan. + /// Normal rows with unique persisted ids need no directory listing. + fn restore_dirs_requiring_id_listing(&self, pending: &[PendingRestore]) -> HashSet { + let mut used_ids: HashSet = self.items().map(|item| item.id()).collect(); + used_ids.extend(self.epoch.borrow().keys().copied()); + pending + .iter() + .filter_map(|entry| match entry.item.id { + Some(id) if used_ids.insert(id) => None, + _ => Some(entry.item.dest_dir.clone()), + }) + .collect() + } + + /// Read only destination directories that may need a new staging id. + /// This runs on a blocking worker; best-effort behavior matches the former + /// allocator for legacy queue entries whose ids were not persisted. + fn list_restore_destinations(directories: HashSet) -> HashMap> { + directories + .into_iter() + .map(|dir| { + let names = + crate::video_staging::dir_file_names(std::path::Path::new(&dir)); + (dir, names) + }) + .collect() + } + /// Read and validate the saved queue away from GTK's main context. /// Only ListStore/model mutation is applied on the main context; disk sweeps /// are sent to Tokio's blocking pool before restore becomes Ready. @@ -4328,7 +4374,9 @@ impl DownloadManager { // the same sweep implementation with production. let pending = Self::load_restore_queue(queue_file); if let Some(pending) = pending { - let plan = self.apply_restored_queue(pending); + let directories = self.restore_dirs_requiring_id_listing(&pending); + let existing_by_dir = Self::list_restore_destinations(directories); + let plan = self.apply_restored_queue(pending, &existing_by_dir); let keep = Self::run_restore_sweeps(plan); Self::schedule_session_orphan_sweep(keep); } @@ -4358,8 +4406,32 @@ impl DownloadManager { match loaded { Ok(Some(pending)) => { manager.restore_dirty.set(false); + let directories = manager.restore_dirs_requiring_id_listing(&pending); + let prepared = tokio_rt() + .spawn_blocking(move || { + let existing_by_dir = Self::list_restore_destinations(directories); + (pending, existing_by_dir) + }) + .await; + let (pending, existing_by_dir) = match prepared { + Ok(prepared) => prepared, + Err(e) => { + tracing::error!( + "download queue destination listing failed: {e}; preserving the saved queue" + ); + manager.finish_restore(RestoreOutcome::Failed); + completion.finished = true; + return; + } + }; + // The main context yielded while legacy/duplicate ids were checked. + if manager.restore_should_abort() { + manager.finish_restore(RestoreOutcome::Failed); + completion.finished = true; + return; + } let plan = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { - manager.apply_restored_queue(pending) + manager.apply_restored_queue(pending, &existing_by_dir) })); let plan = match plan { Ok(plan) => plan, @@ -4529,24 +4601,36 @@ impl DownloadManager { /// Apply validated rows to GTK's model and snapshot cleanup keep-sets. /// Filesystem traversal/deletion happens later on a blocking worker. - fn apply_restored_queue(self: &Rc, pending: Vec) -> RestoreSweepPlan { + fn apply_restored_queue( + self: &Rc, + pending: Vec, + existing_by_dir: &HashMap>, + ) -> RestoreSweepPlan { let mut selections = Vec::new(); { let _batch = self.batch_guard(); - // JSON parsing and per-row validation completed off-thread. + // JSON parsing, validation and any necessary listings completed off-thread. // GObject construction and model mutation stay on this context. for p in pending { + let existing = existing_by_dir + .get(&p.item.dest_dir) + .map(Vec::as_slice) + .unwrap_or(&[]); match p.item.status { DownloadStatus::Done => { if p.gallery_job.is_some() { let mut restored_item = p.item.clone(); restored_item.segments = p.segments; - match self.restore_existing_gallery(&restored_item, p.gallery_job) { + match self.restore_existing_gallery_with_listing( + &restored_item, + p.gallery_job, + existing, + ) { Ok(_) => {} Err(e) => tracing::warn!("skipping queue entry: {e}"), } } else { - self.insert_history(p.item, p.output_dir); + self.insert_history(p.item, p.output_dir, existing); } } status => { @@ -4561,9 +4645,13 @@ impl DownloadManager { restored_item.status = status; restored_item.segments = p.segments; let result = if let Some(job) = p.gallery_job { - self.restore_existing_gallery(&restored_item, Some(job)) + self.restore_existing_gallery_with_listing( + &restored_item, + Some(job), + existing, + ) } else { - self.restore_existing(&restored_item) + self.restore_existing_with_listing(&restored_item, existing) }; match result { Ok(restored) => { diff --git a/src/download_tests.rs b/src/download_tests.rs index 90e837cae..7f9b2a043 100644 --- a/src/download_tests.rs +++ b/src/download_tests.rs @@ -220,6 +220,45 @@ fn test_port(offset: u16) -> u16 { % 40000) as u16 } +#[test] +fn legacy_restore_id_allocation_skips_prelisted_staging_names() { + let settings = test_settings(); + let manager = DownloadManager::new(gio::ListStore::new::(), settings); + manager.next_id.set(7); + let existing = vec!["Title.7.mp4.part".to_string()]; + + assert_eq!(manager.claim_id_in(None, &existing), 8); +} + +#[test] +fn restore_prepares_destination_listings_only_for_fallback_ids() { + let settings = test_settings(); + let manager = DownloadManager::new(gio::ListStore::new::(), settings); + let legacy = PendingRestore { + item: stored_row( + "https://example.test/file.mp4", + "/tmp/grab-restore-legacy", + "file.mp4", + DownloadStatus::Paused, + ), + output_dir: None, + segments: None, + gallery_job: None, + }; + assert_eq!( + manager.restore_dirs_requiring_id_listing(std::slice::from_ref(&legacy)), + std::collections::HashSet::from(["/tmp/grab-restore-legacy".to_string()]) + ); + + let mut current = legacy; + current.item.id = Some(7); + assert!( + manager + .restore_dirs_requiring_id_listing(std::slice::from_ref(¤t)) + .is_empty() + ); +} + #[test] fn backoff_delay_stays_within_cap() { // Bound-only: deterministic, never flakes. Do NOT assert distinctness (jitter is random). From bdc1798fa48a67068b6520233239b4d5820ad5b3 Mon Sep 17 00:00:00 2001 From: Linuxuser67 <41808005+Linuxuser67@users.noreply.github.com> Date: Mon, 26 Oct 2026 16:30:53 +0100 Subject: [PATCH 2/6] style: format legacy restore listing changes --- src/download.rs | 8 +++++--- src/download_tests.rs | 5 ++--- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/download.rs b/src/download.rs index dc2c3984a..c2f720ae5 100644 --- a/src/download.rs +++ b/src/download.rs @@ -1471,8 +1471,7 @@ impl DownloadManager { stored: &StoredItem, job: Option, ) -> Result { - let existing = - crate::video_staging::dir_file_names(std::path::Path::new(&stored.dest_dir)); + let existing = crate::video_staging::dir_file_names(std::path::Path::new(&stored.dest_dir)); self.restore_existing_gallery_with_listing(stored, job, &existing) } @@ -4356,7 +4355,10 @@ impl DownloadManager { let names = crate::video_staging::try_dir_file_names(path).map_err(|error| { std::io::Error::new( error.kind(), - format!("cannot list restore destination {}: {error}", path.display()), + format!( + "cannot list restore destination {}: {error}", + path.display() + ), ) })?; Ok((dir, names)) diff --git a/src/download_tests.rs b/src/download_tests.rs index eadce77ab..4aecfc1a6 100644 --- a/src/download_tests.rs +++ b/src/download_tests.rs @@ -287,9 +287,8 @@ fn restore_destination_listing_reports_unreadable_directories() { )); std::fs::write(&path, b"not a directory").expect("test fixture should be created"); let directory = path.to_string_lossy().into_owned(); - let result = DownloadManager::list_restore_destinations( - std::collections::HashSet::from([directory]), - ); + let result = + DownloadManager::list_restore_destinations(std::collections::HashSet::from([directory])); let _ = std::fs::remove_file(&path); let error = result.expect_err("a file cannot be listed as a destination directory"); From 58a18401f0770832059c532eda2f835dd63ea850 Mon Sep 17 00:00:00 2001 From: Linuxuser67 <41808005+Linuxuser67@users.noreply.github.com> Date: Mon, 26 Oct 2026 16:37:07 +0100 Subject: [PATCH 3/6] test: exercise prelisted staging occupancy checks --- src/video_staging.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/video_staging.rs b/src/video_staging.rs index 2a6c83607..8a8efbd63 100644 --- a/src/video_staging.rs +++ b/src/video_staging.rs @@ -1327,11 +1327,11 @@ mod tests { // No manifest written. assert!( - staging_occupied(&dir, 7), + staging_occupied_in(&dir_file_names(&dir), 7), "id 7 must stay occupied via id-in-name even with no manifest" ); assert!( - !staging_occupied(&dir, 8), + !staging_occupied_in(&dir_file_names(&dir), 8), "unrelated id 8 must not be occupied" ); let _ = std::fs::remove_dir_all(&dir); @@ -1413,7 +1413,7 @@ mod tests { "corrupt manifest must read as None" ); assert!( - staging_occupied(&dir, 7), + staging_occupied_in(&dir_file_names(&dir), 7), "id 7 must stay occupied via id-in-name despite corrupt manifest" ); let _ = std::fs::remove_dir_all(&dir); @@ -1429,8 +1429,8 @@ mod tests { std::fs::write(dir.join("Title.4.mp4.part"), b"row4").unwrap(); std::fs::write(dir.join("Title.9.mp4.part"), b"row9").unwrap(); - assert!(staging_occupied(&dir, 4), "id 4 occupied"); - assert!(staging_occupied(&dir, 9), "id 9 occupied"); + assert!(staging_occupied_in(&dir_file_names(&dir), 4), "id 4 occupied"); + assert!(staging_occupied_in(&dir_file_names(&dir), 9), "id 9 occupied"); // Without a manifest, the sweep must NOT pattern-delete: Title.4.mp4.part // is indistinguishable from a user's own file. Deletion requires the From aaac476f5e90b49b57d5edadb64319ed943a1266 Mon Sep 17 00:00:00 2001 From: Linuxuser67 <41808005+Linuxuser67@users.noreply.github.com> Date: Mon, 26 Oct 2026 16:38:24 +0100 Subject: [PATCH 4/6] docs: clarify prelisted staging ID checks --- src/video_staging.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/video_staging.rs b/src/video_staging.rs index 8a8efbd63..415c76c57 100644 --- a/src/video_staging.rs +++ b/src/video_staging.rs @@ -464,8 +464,8 @@ pub(crate) fn is_grab_part(file_name: &str, stem: &str) -> bool { // Legacy ID-namespaced live: {id}.live. (e.g., "1.live.mp4.part"). // The bare {id}. pattern is NOT checked here: it false-positives on user // files like Movie.2024.mp4. Pattern-based deletion of id-in-name files - // is unsafe (no marker); only `staging_occupied` uses the pattern, and - // only conservatively. + // is unsafe (no marker); the ID allocator only checks these patterns + // conservatively. if let Some(dot_pos) = remainder.find('.') { let (id_part, rest) = remainder.split_at(dot_pos); if !id_part.is_empty() @@ -593,7 +593,7 @@ fn stem_has_subtitle_sidecar(names: &[String], stem: &str) -> bool { /// Only legacy marked patterns (`PART_KINDS`, `.live.`) are matched: the /// id-in-name scheme has no marker, so pattern-deleting it would risk user /// files like `Movie.2024.mp4.part`. ID reuse is prevented separately by -/// `staging_occupied`. +/// the allocator's `staging_occupied_in` check. pub fn clean_dest_parts(dest: &Path) { let (Some(dir), Some(stem)) = (dest.parent(), dest.file_stem().and_then(|s| s.to_str())) else { return; @@ -1369,7 +1369,7 @@ mod tests { // Without a manifest, id-in-name sidecars are NOT swept by pattern: // without a marker, `Title.7.mp4.part` is indistinguishable from a // user's own file. Deletion requires the manifest's exact name. - // ID reuse is still prevented by `staging_occupied`. + // ID reuse is still prevented by the allocator's `staging_occupied_in` check. let dir = unique_dir("manifest-loss-sweep"); std::fs::create_dir_all(&dir).unwrap(); std::fs::write(dir.join("Title.7.mp4.part"), b"partial").unwrap(); From 8e80e08c52718353915086ef16ea272c9c2e69f3 Mon Sep 17 00:00:00 2001 From: Linuxuser67 <41808005+Linuxuser67@users.noreply.github.com> Date: Mon, 26 Oct 2026 16:39:33 +0100 Subject: [PATCH 5/6] style: format staging occupancy tests --- src/video_staging.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/video_staging.rs b/src/video_staging.rs index 415c76c57..9b72ef25d 100644 --- a/src/video_staging.rs +++ b/src/video_staging.rs @@ -1429,8 +1429,14 @@ mod tests { std::fs::write(dir.join("Title.4.mp4.part"), b"row4").unwrap(); std::fs::write(dir.join("Title.9.mp4.part"), b"row9").unwrap(); - assert!(staging_occupied_in(&dir_file_names(&dir), 4), "id 4 occupied"); - assert!(staging_occupied_in(&dir_file_names(&dir), 9), "id 9 occupied"); + assert!( + staging_occupied_in(&dir_file_names(&dir), 4), + "id 4 occupied" + ); + assert!( + staging_occupied_in(&dir_file_names(&dir), 9), + "id 9 occupied" + ); // Without a manifest, the sweep must NOT pattern-delete: Title.4.mp4.part // is indistinguishable from a user's own file. Deletion requires the From cd71a9e2981d233d527e1e460154e2b8e8ec7f59 Mon Sep 17 00:00:00 2001 From: Linuxuser67 <41808005+Linuxuser67@users.noreply.github.com> Date: Mon, 26 Oct 2026 16:44:23 +0100 Subject: [PATCH 6/6] refactor: remove unused gallery restore wrapper --- src/download.rs | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/download.rs b/src/download.rs index c2f720ae5..946e9154e 100644 --- a/src/download.rs +++ b/src/download.rs @@ -1464,17 +1464,6 @@ impl DownloadManager { Ok(self.insert(item)) } - /// Synchronous gallery restore helper for tests; production passes a prelisted destination. - #[cfg(test)] - pub fn restore_existing_gallery( - self: &Rc, - stored: &StoredItem, - job: Option, - ) -> Result { - let existing = crate::video_staging::dir_file_names(std::path::Path::new(&stored.dest_dir)); - self.restore_existing_gallery_with_listing(stored, job, &existing) - } - fn restore_existing_gallery_with_listing( self: &Rc, stored: &StoredItem,