From 09e1f622992d873ef0d294b348772b9d9ffeadea Mon Sep 17 00:00:00 2001 From: Myles Anderson Date: Thu, 22 Oct 2026 14:42:56 -0700 Subject: [PATCH 1/3] Decide project-delete refusals before purging native chat history A refused project delete (busy run artifacts, data-dir move, runs in flight, artifact cleanup failure, or contended native lock) no longer kills OpenCode history while the chat rows survive. The store now takes the native-deletion lock only after the run-artifacts guard and move check, then cleans artifacts, purges native sessions, and deletes rows, so the native lock is never held across the run-artifacts wait. Part of OR-569. Co-Authored-By: Claude Opus 5.5 --- src/commands/up.rs | 20 ++++----- src/local/chat/mod.rs | 12 +++-- src/store.rs | 100 +++++++++++++++++++++++++++++++++++++++++- 3 files changed, 115 insertions(+), 17 deletions(-) diff --git a/src/commands/up.rs b/src/commands/up.rs index 64689a70b..0e20ef233 100644 --- a/src/commands/up.rs +++ b/src/commands/up.rs @@ -2108,20 +2108,16 @@ async fn delete_project_inner(state: AppState, id: String, query: DeleteProjectQ .iter() .map(|session| session.id.clone()) .collect::>(); - // Other deletions fail fast on this lock, so it covers only the purge and the row delete. - let native_deletion = if session_ids.is_empty() { - None - } else { - let lock = store.lock_native_deletion()?; - local::chat::ChatHost::delete_native_sessions(&session_ids).await?; - Some(lock) - }; let project_id = id.clone(); // Row deletion can wait up to RUN_ARTIFACTS_WAIT for the run-artifacts lock, so keep it off the async workers. - tokio::task::spawn_blocking(move || store.delete_local_project(&project_id)) - .await - .map_err(|error| anyhow!("project row deletion task failed: {error}"))??; - drop(native_deletion); + tokio::task::spawn_blocking(move || { + let native_purge = (!session_ids.is_empty()).then_some(|| { + local::chat::ChatHost::delete_native_sessions_blocking(&store, &session_ids) + }); + store.delete_local_project_with(&project_id, native_purge) + }) + .await + .map_err(|error| anyhow!("project row deletion task failed: {error}"))??; for session in &sessions { state.chat.claude.forget_session(&session.id).await; local::chat::cleanup_session_transcript_artifacts(&session.id); diff --git a/src/local/chat/mod.rs b/src/local/chat/mod.rs index 9ab06da2b..34f63bd4a 100644 --- a/src/local/chat/mod.rs +++ b/src/local/chat/mod.rs @@ -6718,12 +6718,16 @@ impl ChatHost { } pub async fn delete_native_sessions(session_ids: &[String]) -> Result<()> { - let (ids, retained) = Store::open()?.opencode_sessions_for_deletion(session_ids)?; + let session_ids = session_ids.to_vec(); tokio::task::spawn_blocking(move || { - crate::local::native_store::delete_opencode_sessions(&ids, &retained) + Self::delete_native_sessions_blocking(&Store::open()?, &session_ids) }) - .await??; - Ok(()) + .await? + } + + pub fn delete_native_sessions_blocking(store: &Store, session_ids: &[String]) -> Result<()> { + let (ids, retained) = store.opencode_sessions_for_deletion(session_ids)?; + crate::local::native_store::delete_opencode_sessions(&ids, &retained) } } diff --git a/src/store.rs b/src/store.rs index 96b1ea447..c8d86b624 100644 --- a/src/store.rs +++ b/src/store.rs @@ -2130,9 +2130,19 @@ impl Store { )?) } + #[cfg(test)] + pub fn delete_local_project(&self, id: &str) -> Result<()> { + self.delete_local_project_with(id, None:: Result<()>>) + } + /// Delete a project, its rows and its run artifacts; its artifacts folder is the caller's. GitHub repo and cache clone are kept. /// Blocks up to [`RUN_ARTIFACTS_WAIT`] while a submission or orphan scan holds the artifacts lock. - pub fn delete_local_project(&self, id: &str) -> Result<()> { + /// `native_purge` runs under the native-deletion lock after run-artifact cleanup, just before the rows go. + pub fn delete_local_project_with( + &self, + id: &str, + native_purge: Option Result<()>>, + ) -> Result<()> { let mut lock = self.run_artifacts_lock()?; let deadline = std::time::Instant::now() + RUN_ARTIFACTS_WAIT; let _guard = loop { @@ -2156,8 +2166,15 @@ impl Store { "The data directory is being moved; retry project deletion afterwards." )); } + let _native_deletion = native_purge + .is_some() + .then(|| self.lock_native_deletion()) + .transpose()?; // Keep rows on filesystem errors so deletion can be retried with the same handles. self.cleanup_project_run_artifacts(id)?; + if let Some(purge) = native_purge { + purge()?; + } let tx = self.begin()?; tx.execute( "DELETE FROM overleaf_links WHERE project_id = ?1", @@ -4329,6 +4346,87 @@ mod tests { std::fs::remove_dir_all(dir).unwrap(); } + #[test] + fn refused_project_deletion_never_reaches_the_native_purge() { + let dir = std::env::temp_dir().join(format!("orx-delete-order-{}", uuid::Uuid::new_v4())); + let store = Store::open_at(dir.clone()).unwrap(); + project_with_run_artifacts(&store, "project", "run", &"e".repeat(64)); + let mut session = chat_session_fixture("chat"); + session.project_id = "project".into(); + session.harness = "opencode".into(); + session.native_session_id = Some("native".into()); + store.create_chat_session(&session).unwrap(); + let purged = std::cell::Cell::new(false); + let purge = || { + purged.set(true); + Ok(()) + }; + let refuse = |reason: &str| { + let error = store + .delete_local_project_with("project", Some(purge)) + .unwrap_err(); + assert!(error.to_string().contains(reason), "{error}"); + assert!(!purged.get()); + assert!(store.get_chat_session("chat").unwrap().is_some()); + }; + + store + .conn + .execute("UPDATE runs SET status = 'running'", []) + .unwrap(); + refuse("in flight"); + store + .conn + .execute("UPDATE runs SET status = 'done'", []) + .unwrap(); + + let staging_lock = store.run_artifacts_lock().unwrap(); + let staging = staging_lock.read().unwrap(); + refuse("busy"); + drop(staging); + + let moving = store.acquire_data_dir_move_lock().unwrap(); + assert!(store.claim_data_dir_move("move").unwrap()); + refuse("being moved"); + store.release_data_dir_move("move").unwrap(); + drop(moving); + + let native_deletion = store.lock_native_deletion().unwrap(); + refuse("native chat deletion"); + drop(native_deletion); + assert!(dir.join("local-runs/run/repo/output").is_file()); + + #[cfg(unix)] + { + std::fs::rename(dir.join("local-runs"), dir.join("outside")).unwrap(); + std::os::unix::fs::symlink(dir.join("outside"), dir.join("local-runs")).unwrap(); + refuse("Refusing artifact cleanup"); + std::fs::remove_file(dir.join("local-runs")).unwrap(); + std::fs::rename(dir.join("outside"), dir.join("local-runs")).unwrap(); + } + + assert!(store + .delete_local_project_with("project", Some(|| Err(anyhow!("purge failed")))) + .is_err()); + assert!(store.get_chat_session("chat").unwrap().is_some()); + + store + .delete_local_project_with( + "project", + Some(|| { + assert!(store.run_artifacts_lock()?.try_read().is_err()); + assert!(store.lock_native_deletion().is_err()); + purge() + }), + ) + .unwrap(); + assert!(purged.get()); + assert!(store.get_chat_session("chat").unwrap().is_none()); + assert!(store.get_local_project("project").unwrap().is_none()); + drop(store); + std::fs::remove_dir_all(dir).unwrap(); + } + #[test] fn project_deletion_rejects_unsafe_artifact_identifiers() { let dir = std::env::temp_dir().join(format!("orx-delete-unsafe-{}", uuid::Uuid::new_v4())); From 147181ff52951e3d5c1deab0a6eb20a1069825d9 Mon Sep 17 00:00:00 2001 From: Myles Anderson Date: Thu, 22 Oct 2026 15:06:46 -0700 Subject: [PATCH 2/3] Delete chat attachments with their chats and sweep orphans (OR-574) Chat and project deletes now remove the attachment files their messages showed once no remaining message shows them, skipping while a data-dir move is claimed. The periodic reclaim task also sweeps att-* files no message shows after a 24h settle, within the orphan-pass budget. Co-Authored-By: Claude Opus 5.5 --- src/commands/up.rs | 11 +++ src/local/chat/mod.rs | 13 ++- src/store.rs | 205 +++++++++++++++++++++++++++++++++++++++++- 3 files changed, 227 insertions(+), 2 deletions(-) diff --git a/src/commands/up.rs b/src/commands/up.rs index 0e20ef233..f49652704 100644 --- a/src/commands/up.rs +++ b/src/commands/up.rs @@ -4680,6 +4680,17 @@ async fn reclaim_orphaned_run_artifacts( eprintln!("orx up: orphaned run artifact cleanup failed: {error}") } } + match tokio::task::spawn_blocking(|| { + Store::open()?.cleanup_orphaned_chat_attachments() + }) + .await + { + Ok(Ok(left)) => more |= left, + Ok(Err(error)) => { + eprintln!("orx up: could not clean orphaned chat attachments: {error}") + } + Err(error) => eprintln!("orx up: chat attachment cleanup failed: {error}"), + } } } let pause = if more { diff --git a/src/local/chat/mod.rs b/src/local/chat/mod.rs index 34f63bd4a..0eaf7d30d 100644 --- a/src/local/chat/mod.rs +++ b/src/local/chat/mod.rs @@ -986,8 +986,10 @@ pub struct SavedAttachment { pub is_pdf: bool, } +pub const ATTACHMENTS_DIR: &str = "chat-attachments"; + pub fn attachments_dir() -> Result { - let dir = crate::store::data_dir().join("chat-attachments"); + let dir = crate::store::data_dir().join(ATTACHMENTS_DIR); std::fs::create_dir_all(&dir) .map_err(|e| anyhow!("Could not create {}: {}", dir.display(), e))?; Ok(dir) @@ -1038,6 +1040,15 @@ fn safe_attachment_name(original: Option<&str>, ext: &str) -> String { format!("{safe}.{ext}") } +/// Matches the `att-__.` names `save_images` mints. +pub(crate) fn is_attachment_name(name: &str) -> bool { + name.starts_with("att-") + && !name.contains("..") + && name + .chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '_' | '-' | '.')) +} + /// Decode pasted/uploaded attachments to the attachments dir. The original file /// name (when present) is preserved after a `__` marker so the transcript and the /// agent see a meaningful name; the uuid prefix keeps names collision-free. diff --git a/src/store.rs b/src/store.rs index c8d86b624..1de3c2c75 100644 --- a/src/store.rs +++ b/src/store.rs @@ -479,6 +479,8 @@ pub(crate) const RUN_ARTIFACTS_POLL: Duration = Duration::from_millis(250); const ORPHAN_PASS_BUDGET: Duration = Duration::from_secs(5); /// Finished runs older than each project's newest few local runs lose their rebuildable environments. const FULL_RUNS_PER_PROJECT: i64 = 3; +/// Uploads are written before the message naming them, so the sweep spares recent files. +const CHAT_ATTACHMENT_SETTLE: Duration = Duration::from_secs(24 * 60 * 60); const SLIMMED_RUN_PATHS: [&[&str]; 2] = [&["repo", ".venv"], &["repo", ".cache", "uv"]]; const SUBMISSION_HANDLE: (&str, &str) = ("submission-handles", ".json"); @@ -2135,7 +2137,7 @@ impl Store { self.delete_local_project_with(id, None:: Result<()>>) } - /// Delete a project, its rows and its run artifacts; its artifacts folder is the caller's. GitHub repo and cache clone are kept. + /// Delete a project, its rows, run artifacts and chat attachments; its artifacts folder is the caller's. GitHub repo and cache clone are kept. /// Blocks up to [`RUN_ARTIFACTS_WAIT`] while a submission or orphan scan holds the artifacts lock. /// `native_purge` runs under the native-deletion lock after run-artifact cleanup, just before the rows go. pub fn delete_local_project_with( @@ -2176,6 +2178,12 @@ impl Store { purge()?; } let tx = self.begin()?; + let attachments = self + .chat_attachment_names_where( + "m.session_id IN (SELECT id FROM chat_sessions WHERE project_id = ?1)", + [id], + ) + .unwrap_or_default(); tx.execute( "DELETE FROM overleaf_links WHERE project_id = ?1", params![id], @@ -2208,6 +2216,7 @@ impl Store { self.conn .execute("DELETE FROM local_projects WHERE id = ?1", params![id])?; tx.commit()?; + self.remove_unreferenced_chat_attachments(attachments); Ok(()) } @@ -2621,6 +2630,9 @@ impl Store { if has_children { return Err(anyhow!("delete side chats before deleting their parent")); } + let attachments = self + .chat_attachment_names_where("m.session_id = ?1", [id]) + .unwrap_or_default(); tx.execute( "DELETE FROM chat_queued_messages WHERE session_id = ?1", params![id], @@ -2632,9 +2644,92 @@ impl Store { )?; tx.execute("DELETE FROM chat_sessions WHERE id = ?1", params![id])?; tx.commit()?; + self.remove_unreferenced_chat_attachments(attachments); Ok(()) } + /// Attachment file names that image parts of matching messages show; only user messages carry them. + fn chat_attachment_names_where( + &self, + filter: &'static str, + params: impl rusqlite::Params, + ) -> Result> { + let mut query = self.conn.prepare(&format!( + "SELECT DISTINCT json_extract(p.value, '$.text') + FROM chat_messages m, json_each(m.parts_json) p + WHERE m.role = 'user' AND json_extract(p.value, '$.type') = 'image' + AND json_extract(p.value, '$.text') IS NOT NULL AND {filter}" + ))?; + let names = query + .query_map(params, |row| row.get::<_, String>(0))? + .collect::>()?; + Ok(names) + } + + /// Best effort once the rows are gone; the periodic sweep retries what is left. + fn remove_unreferenced_chat_attachments(&self, names: std::collections::HashSet) { + // Another dashboard may be copying this data directory. + if names.is_empty() || self.data_dir_move_claimed().unwrap_or(true) { + return; + } + let Ok(referenced) = self.chat_attachment_names_where("1", []) else { + return; + }; + let dir = self.data_dir.join(crate::local::chat::ATTACHMENTS_DIR); + for name in names.difference(&referenced) { + if crate::local::chat::is_attachment_name(name) { + let _ = std::fs::remove_file(dir.join(name)); + } + } + } + + /// Remove attachment files no message shows; returns whether the budget ran out first. + pub(crate) fn cleanup_orphaned_chat_attachments(&self) -> Result { + // Another dashboard may be copying this data directory. + if self.data_dir_move_claimed()? { + return Ok(false); + } + let entries = + match std::fs::read_dir(self.data_dir.join(crate::local::chat::ATTACHMENTS_DIR)) { + Ok(entries) => entries, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(false), + Err(error) => return Err(error.into()), + }; + let mut started = std::time::Instant::now(); + let mut referenced = None; + for entry in entries.flatten() { + if started.elapsed() >= ORPHAN_PASS_BUDGET { + return Ok(true); + } + let Some(name) = entry.file_name().to_str().map(str::to_string) else { + continue; + }; + let settled = entry.metadata().ok().filter(|metadata| { + metadata.is_file() + && metadata + .modified() + .is_ok_and(|at| at.elapsed().unwrap_or_default() >= CHAT_ATTACHMENT_SETTLE) + }); + if !crate::local::chat::is_attachment_name(&name) || settled.is_none() { + continue; + } + // Built only once a candidate exists, and kept out of the removal budget. + let referenced = match &mut referenced { + Some(referenced) => referenced, + None => { + let names = self.chat_attachment_names_where("1", [])?; + started = std::time::Instant::now(); + referenced.insert(names) + } + }; + if !referenced.contains(&name) { + // A file that cannot be removed is retried next pass without stalling the rest. + let _ = std::fs::remove_file(entry.path()); + } + } + Ok(false) + } + /// `None` clears it, which a fork of the session's very first turn needs: /// that turn ran with no harness session to resume from. pub fn set_chat_session_native_id(&self, id: &str, native_id: Option<&str>) -> Result<()> { @@ -4427,6 +4522,114 @@ mod tests { std::fs::remove_dir_all(dir).unwrap(); } + #[test] + fn deleting_chats_removes_only_attachments_no_other_message_shows() { + let dir = std::env::temp_dir().join(format!("orx-attachments-{}", uuid::Uuid::new_v4())); + let store = Store::open_at(dir.clone()).unwrap(); + project_with_run_artifacts(&store, "project", "run", &"a".repeat(64)); + let attachments = dir.join(crate::local::chat::ATTACHMENTS_DIR); + std::fs::create_dir_all(&attachments).unwrap(); + let chats: [(&str, &str, &[&str]); 3] = [ + ( + "first", + "project", + &["att-1__own.png", "att-2__shared.pdf", "keep.txt"], + ), + ("second", "project", &["att-3__own.png"]), + ("other", "elsewhere", &["att-2__shared.pdf"]), + ]; + for (chat, project, files) in chats { + let mut session = chat_session_fixture(chat); + session.project_id = project.into(); + store.create_chat_session(&session).unwrap(); + let parts = files + .iter() + .enumerate() + .map(|(i, file)| serde_json::json!({ "id": format!("img{i}"), "type": "image", "text": file })) + .collect::>(); + store + .upsert_chat_message(&StoredChatMessage { + id: format!("msg_{chat}"), + session_id: chat.into(), + role: "user".into(), + parts_json: serde_json::Value::from(parts).to_string(), + created_at: 1, + completed_at: None, + parent_id: None, + base_native_session_id: None, + result_native_session_id: None, + }) + .unwrap(); + } + for file in [ + "att-1__own.png", + "att-2__shared.pdf", + "att-3__own.png", + "keep.txt", + ] { + std::fs::write(attachments.join(file), "bytes").unwrap(); + } + + store.delete_chat_session("first").unwrap(); + assert!(!attachments.join("att-1__own.png").exists()); + assert!(attachments.join("att-2__shared.pdf").exists()); + store.delete_local_project("project").unwrap(); + assert!(!attachments.join("att-3__own.png").exists()); + assert!(attachments.join("att-2__shared.pdf").exists()); + store.delete_chat_session("other").unwrap(); + assert!(!attachments.join("att-2__shared.pdf").exists()); + assert!(attachments.join("keep.txt").exists()); + drop(store); + std::fs::remove_dir_all(dir).unwrap(); + } + + #[test] + fn attachment_sweep_spares_shown_recent_and_foreign_files() { + let dir = + std::env::temp_dir().join(format!("orx-attachment-sweep-{}", uuid::Uuid::new_v4())); + let store = Store::open_at(dir.clone()).unwrap(); + assert!(!store.cleanup_orphaned_chat_attachments().unwrap()); + store + .create_chat_session(&chat_session_fixture("chat")) + .unwrap(); + store + .upsert_chat_message(&StoredChatMessage { + id: "msg".into(), + session_id: "chat".into(), + role: "user".into(), + parts_json: r#"[{"id":"img0","type":"image","text":"att-1__shown.png"}]"#.into(), + created_at: 1, + completed_at: None, + parent_id: None, + base_native_session_id: None, + result_native_session_id: None, + }) + .unwrap(); + let attachments = dir.join(crate::local::chat::ATTACHMENTS_DIR); + std::fs::create_dir_all(&attachments).unwrap(); + let old = std::time::SystemTime::now() - 2 * CHAT_ATTACHMENT_SETTLE; + for file in ["att-1__shown.png", "att-2__orphan.png", "notes.txt"] { + let file = std::fs::File::create(attachments.join(file)).unwrap(); + file.set_modified(old).unwrap(); + } + std::fs::write(attachments.join("att-3__unsent.png"), "bytes").unwrap(); + + let moving = store.acquire_data_dir_move_lock().unwrap(); + assert!(store.claim_data_dir_move("move").unwrap()); + store.cleanup_orphaned_chat_attachments().unwrap(); + assert!(attachments.join("att-2__orphan.png").exists()); + store.release_data_dir_move("move").unwrap(); + drop(moving); + + assert!(!store.cleanup_orphaned_chat_attachments().unwrap()); + assert!(!attachments.join("att-2__orphan.png").exists()); + for kept in ["att-1__shown.png", "att-3__unsent.png", "notes.txt"] { + assert!(attachments.join(kept).exists(), "{kept}"); + } + drop(store); + std::fs::remove_dir_all(dir).unwrap(); + } + #[test] fn project_deletion_rejects_unsafe_artifact_identifiers() { let dir = std::env::temp_dir().join(format!("orx-delete-unsafe-{}", uuid::Uuid::new_v4())); From d1cc352607d1bbb22f83622c67651abb33691d6f Mon Sep 17 00:00:00 2001 From: Myles Anderson Date: Thu, 22 Oct 2026 18:05:14 -0700 Subject: [PATCH 3/3] Hold off data-dir moves during attachment cleanup; keep sweep progress Attachment removal now holds a shared lock on the data-dir move lock, which a move takes exclusively before claiming its lease, so another dashboard cannot start copying mid-removal. Only removals count against the sweep budget, so a pass that walks many retained files still reaches later orphans. Co-Authored-By: Claude Opus 5.5 --- src/commands/up.rs | 3 +- src/store.rs | 82 ++++++++++++++++++++++++++++++++++++---------- 2 files changed, 66 insertions(+), 19 deletions(-) diff --git a/src/commands/up.rs b/src/commands/up.rs index 16295ac8d..c753d5d4a 100644 --- a/src/commands/up.rs +++ b/src/commands/up.rs @@ -5150,7 +5150,8 @@ async fn move_data_dir(State(state): State, Json(req): Json Result { self.clear_stale_data_dir_move_lease()?; + self.data_dir_move_lease_exists() + } + + fn data_dir_move_lease_exists(&self) -> Result { Ok(self.conn.query_row( "SELECT EXISTS (SELECT 1 FROM data_dir_move_lease WHERE id = 1)", [], @@ -2667,10 +2671,12 @@ impl Store { /// Best effort once the rows are gone; the periodic sweep retries what is left. fn remove_unreferenced_chat_attachments(&self, names: std::collections::HashSet) { - // Another dashboard may be copying this data directory. - if names.is_empty() || self.data_dir_move_claimed().unwrap_or(true) { + if names.is_empty() { return; } + let Ok(Some(_move_hold)) = self.hold_off_data_dir_move() else { + return; + }; let Ok(referenced) = self.chat_attachment_names_where("1", []) else { return; }; @@ -2682,24 +2688,40 @@ impl Store { } } + /// Keeps any dashboard from starting a data-dir move until dropped; `None` while a move holds or claims it. + fn hold_off_data_dir_move(&self) -> Result> { + if self.data_dir_move_claimed()? { + return Ok(None); + } + // A move takes this lock exclusively before claiming its lease. + let hold = open_lifecycle_lock_at(&self.data_dir_move_lock_path)?.into_inner(); + match hold.try_lock_shared() { + Ok(()) => {} + Err(std::fs::TryLockError::WouldBlock) => return Ok(None), + Err(std::fs::TryLockError::Error(error)) => return Err(error.into()), + } + Ok((!self.data_dir_move_lease_exists()?).then_some(hold)) + } + /// Remove attachment files no message shows; returns whether the budget ran out first. pub(crate) fn cleanup_orphaned_chat_attachments(&self) -> Result { - // Another dashboard may be copying this data directory. - if self.data_dir_move_claimed()? { + self.cleanup_orphaned_chat_attachments_within(ORPHAN_PASS_BUDGET) + } + + /// The budget starts at the first removal, so every pass makes progress; removed files leave the listing. + fn cleanup_orphaned_chat_attachments_within(&self, budget: Duration) -> Result { + let Some(_move_hold) = self.hold_off_data_dir_move()? else { return Ok(false); - } + }; let entries = match std::fs::read_dir(self.data_dir.join(crate::local::chat::ATTACHMENTS_DIR)) { Ok(entries) => entries, Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(false), Err(error) => return Err(error.into()), }; - let mut started = std::time::Instant::now(); + let mut started = None; let mut referenced = None; for entry in entries.flatten() { - if started.elapsed() >= ORPHAN_PASS_BUDGET { - return Ok(true); - } let Some(name) = entry.file_name().to_str().map(str::to_string) else { continue; }; @@ -2712,18 +2734,18 @@ impl Store { if !crate::local::chat::is_attachment_name(&name) || settled.is_none() { continue; } - // Built only once a candidate exists, and kept out of the removal budget. let referenced = match &mut referenced { Some(referenced) => referenced, - None => { - let names = self.chat_attachment_names_where("1", [])?; - started = std::time::Instant::now(); - referenced.insert(names) - } + None => referenced.insert(self.chat_attachment_names_where("1", [])?), }; - if !referenced.contains(&name) { - // A file that cannot be removed is retried next pass without stalling the rest. - let _ = std::fs::remove_file(entry.path()); + if referenced.contains(&name) { + continue; + } + let started = *started.get_or_insert_with(std::time::Instant::now); + // A file that cannot be removed is retried next pass without stalling the rest. + let _ = std::fs::remove_file(entry.path()); + if started.elapsed() >= budget { + return Ok(true); } } Ok(false) @@ -4648,11 +4670,35 @@ mod tests { store.release_data_dir_move("move").unwrap(); drop(moving); + let hold = store.hold_off_data_dir_move().unwrap().unwrap(); + assert!(store.acquire_data_dir_move_lock().is_err()); + drop(hold); + assert!(!store.cleanup_orphaned_chat_attachments().unwrap()); assert!(!attachments.join("att-2__orphan.png").exists()); for kept in ["att-1__shown.png", "att-3__unsent.png", "notes.txt"] { assert!(attachments.join(kept).exists(), "{kept}"); } + + let orphans = ["att-4__a.png", "att-5__b.png", "att-6__c.png"]; + for file in orphans { + let file = std::fs::File::create(attachments.join(file)).unwrap(); + file.set_modified(old).unwrap(); + } + let passes = (0..=orphans.len()) + .take_while(|_| { + store + .cleanup_orphaned_chat_attachments_within(Duration::ZERO) + .unwrap() + }) + .count(); + assert_eq!( + passes, + orphans.len(), + "each exhausted pass still removes one" + ); + assert!(orphans.iter().all(|file| !attachments.join(file).exists())); + assert!(attachments.join("att-1__shown.png").exists()); drop(store); std::fs::remove_dir_all(dir).unwrap(); }