diff --git a/apps/staged/src-tauri/src/note_commands.rs b/apps/staged/src-tauri/src/note_commands.rs index a278336b1..079529e59 100644 --- a/apps/staged/src-tauri/src/note_commands.rs +++ b/apps/staged/src-tauri/src/note_commands.rs @@ -138,22 +138,35 @@ pub(crate) fn update_note_impl( } /// Delete a note and optionally its linked session. +/// +/// Async, with the body on the blocking pool: deciding which of the note's +/// media nothing else uses reads every note body that embeds media and every +/// chat attachment list, and deleting the session cascades its messages, all +/// on the one store connection. Tauri runs a synchronous command on the main +/// thread, where this froze the window for as long as it took. #[tauri::command(rename_all = "camelCase")] -pub fn delete_note( +pub async fn delete_note( store: tauri::State<'_, Mutex>>>, app_handle: tauri::AppHandle, note_id: String, delete_session: Option, ) -> Result<(), String> { - delete_note_impl( - &crate::get_store(&store)?, - &app_handle, - ¬e_id, - delete_session.unwrap_or(false), - ) + let store = crate::get_store(&store)?; + + tauri::async_runtime::spawn_blocking(move || { + delete_note_impl( + &store, + &app_handle, + ¬e_id, + delete_session.unwrap_or(false), + ) + }) + .await + .map_err(|e| format!("Note delete task failed: {e}"))? } -/// The body of [`delete_note`], shared with the web dispatcher's arm. +/// The body of [`delete_note`], shared with the web dispatcher's arm. Callers +/// run it inside `spawn_blocking`. /// /// A note's session may be the branch's busy marker or a barrier in its /// queue, so the branch is kicked after the delete; the kick is what starts @@ -335,23 +348,47 @@ pub fn set_project_note_read( /// Delete a project note, its child notes, and every session they owned. /// -/// The store cascade removes the child-note rows, but their sessions may still -/// be running — the note-delete trigger skips those, and a child session that -/// outlived its note would keep running with nowhere to write its result. So -/// each orphaned session (the project note's own plus every child's) is -/// cancelled in the registry and then deleted. +/// Async, with the body on the blocking pool, for the reason given on +/// [`delete_note`]; here the media check covers the parent and every child, +/// and there is a session cascade per note that had one. #[tauri::command(rename_all = "camelCase")] -pub fn delete_project_note( +pub async fn delete_project_note( registry: tauri::State<'_, Arc>, store: tauri::State<'_, Mutex>>>, note_id: String, ) -> Result<(), String> { let store = crate::get_store(&store)?; - let media_ids = crate::note_media::project_note_media_ids(&store, ¬e_id)?; + let registry = Arc::clone(®istry); + + tauri::async_runtime::spawn_blocking(move || { + delete_project_note_impl(&store, ®istry, ¬e_id) + }) + .await + .map_err(|e| format!("Project note delete task failed: {e}"))? +} + +/// The body of [`delete_project_note`], shared with the web dispatcher's arm. +/// Callers run it inside `spawn_blocking`. +/// +/// The media ids are read before the delete, while the bodies still exist, +/// and checked after it in one pass for the whole family (see +/// [`crate::note_media::cleanup_unreferenced`]). +/// +/// The store cascade removes the child-note rows, but their sessions may still +/// be running — the note-delete trigger skips those, and a child session that +/// outlived its note would keep running with nowhere to write its result. So +/// each orphaned session (the project note's own plus every child's) is +/// cancelled in the registry and then deleted. +pub(crate) fn delete_project_note_impl( + store: &Store, + registry: &crate::session_runner::SessionRegistry, + note_id: &str, +) -> Result<(), String> { + let media_ids = crate::note_media::project_note_media_ids(store, note_id)?; let orphaned = store - .delete_project_note(¬e_id) + .delete_project_note(note_id) .map_err(|e| e.to_string())?; - crate::note_media::cleanup_unreferenced(&store, media_ids); + crate::note_media::cleanup_unreferenced(store, media_ids); for sid in orphaned.all_session_ids() { registry.cancel(&sid); @@ -373,6 +410,39 @@ mod tests { format!("![{}](staged-media://{}.png)", image.filename, image.id) } + /// `count` media files of the fixture's branch, as a copy or a written + /// note's paste lands them. + fn stored_media(f: &Fixture, count: usize) -> Vec { + (0..count) + .map(|n| { + store_media_file( + &f.store, + Some(&f.branch.id), + &f.project.id, + Some(NOTE_MEDIA_SESSION_ID), + &format!("shot-{n}.png"), + PNG, + true, + ) + .unwrap() + }) + .collect() + } + + fn embed_all(images: &[Image]) -> String { + images.iter().map(embed).collect::>().join("\n\n") + } + + fn assert_collected(f: &Fixture, image: &Image) { + assert!( + f.store.get_image(&image.id).unwrap().is_none(), + "{} kept its row", + image.filename + ); + let path = image_file_path(&image.project_id, &image.id, &image.filename).unwrap(); + assert!(!path.exists(), "{} kept its file", image.filename); + } + fn assert_kept(f: &Fixture, image: &Image) { assert!( f.store.get_image(&image.id).unwrap().is_some(), @@ -419,6 +489,174 @@ mod tests { assert_kept(&f, &image); } + /// Deleting a note checks all the media it embedded in one pass over the + /// note bodies and one over the chat attachments, not one per image. + #[test] + fn deleting_a_note_checks_its_media_in_one_pass() { + use crate::store::images::{take_reference_passes, ReferencePasses}; + + let f = Fixture::new(); + let images = stored_media(&f, 8); + let mut note = Note::new_standalone(&f.branch.id, "Shots", &embed_all(&images)) + .with_subtype(Note::SUBTYPE_WRITTEN); + f.store.create_note_with_unique_title(&mut note).unwrap(); + // Another note still embeds the first image. + let mut other = Note::new_standalone(&f.branch.id, "Other", &embed(&images[0])) + .with_subtype(Note::SUBTYPE_WRITTEN); + f.store.create_note_with_unique_title(&mut other).unwrap(); + let app = tauri::test::mock_builder() + .build(tauri::test::mock_context(tauri::test::noop_assets())) + .expect("failed to build mock app"); + + take_reference_passes(); + delete_note_impl(&f.store, app.handle(), ¬e.id, false).unwrap(); + assert_eq!( + take_reference_passes(), + ReferencePasses { notes: 1, chat: 1 } + ); + + assert_kept(&f, &images[0]); + for image in &images[1..] { + assert_collected(&f, image); + } + } + + /// The body both transports run for a project note. The parent's media and + /// every child's are checked together, in one pass over the note bodies + /// and one over the chat attachments; what a note outside the family or a + /// chat message still uses is kept; and every session the family owned is + /// stopped and removed, running or not. + #[test] + fn deleting_a_project_note_checks_the_family_media_in_one_pass_and_ends_its_sessions() { + use crate::session_runner::SessionRegistry; + use crate::store::images::{take_reference_passes, ReferencePasses}; + use crate::store::models::{ProjectNote, Session}; + use crate::store::{MessageRole, SessionStatus}; + + let f = Fixture::new(); + let images = stored_media(&f, 8); + let registry = Arc::new(SessionRegistry::new()); + let running_session = |prompt: &str| { + let session = Session::new_running(prompt, f.dir.path()); + f.store.create_session(&session).unwrap(); + session + }; + + // The parent embeds three images and its session is still running. + let parent_session = running_session("write project note"); + let parent = ProjectNote::new(&f.project.id, "Parent", &embed_all(&images[..3])) + .with_session(&parent_session.id); + f.store.create_project_note(&parent).unwrap(); + let parent_run = registry.register_external(&parent_session.id); + + // Three children: one still running, one finished, one with no + // session, embedding the other five images between them. + let child_session = running_session("write child note"); + let finished_session = running_session("write finished child note"); + f.store + .update_session_status(&finished_session.id, SessionStatus::Completed, None, None) + .unwrap(); + let child_run = registry.register_external(&child_session.id); + let children = [ + Note::new(&f.branch.id, "Running child", &embed_all(&images[3..5])) + .with_session(&child_session.id), + Note::new(&f.branch.id, "Finished child", &embed_all(&images[5..7])) + .with_session(&finished_session.id), + Note::new(&f.branch.id, "Plain child", &embed(&images[7])), + ] + .map(|child| child.with_parent_project_note(&parent.id)); + for child in &children { + f.store.create_note(child).unwrap(); + } + + // Outside the family: a note embedding one of the parent's images and + // one of a child's, and a chat message attaching another child's. + let mut outsider = Note::new_standalone( + &f.branch.id, + "Outsider", + &embed_all(&[images[0].clone(), images[3].clone()]), + ) + .with_subtype(Note::SUBTYPE_WRITTEN); + f.store + .create_note_with_unique_title(&mut outsider) + .unwrap(); + f.store + .add_session_message_with_images( + &f.session.id, + MessageRole::User, + "Screenshot", + std::slice::from_ref(&images[5].id), + ) + .unwrap(); + let kept = [0, 3, 5]; + + take_reference_passes(); + delete_project_note_impl(&f.store, ®istry, &parent.id).unwrap(); + assert_eq!( + take_reference_passes(), + ReferencePasses { notes: 1, chat: 1 } + ); + + assert!(f.store.get_project_note(&parent.id).unwrap().is_none()); + for child in &children { + assert!(f.store.get_note(&child.id).unwrap().is_none()); + } + assert!(f.store.get_note(&outsider.id).unwrap().is_some()); + for (n, image) in images.iter().enumerate() { + if kept.contains(&n) { + assert_kept(&f, image); + } else { + assert_collected(&f, image); + } + } + + // Both running sessions were told to stop, and no session row of the + // family is left. The session the chat message belongs to is not the + // family's and stays. + assert!(parent_run.token().is_cancelled()); + assert!(child_run.token().is_cancelled()); + for session in [&parent_session, &child_session, &finished_session] { + assert!( + f.store.get_session(&session.id).unwrap().is_none(), + "session `{}` outlived its note", + session.prompt + ); + } + assert!(f.store.get_session(&f.session.id).unwrap().is_some()); + + // A note that is already gone: nothing to delete and nothing to check. + delete_project_note_impl(&f.store, ®istry, &parent.id).unwrap(); + assert_eq!(take_reference_passes(), ReferencePasses::default()); + } + + /// Tauri runs a synchronous command on the main thread, where these two + /// held the window for the whole media check and session cascade. Each + /// must stay async and hand its shared body to the blocking pool. + #[test] + fn the_delete_commands_run_their_bodies_on_the_blocking_pool() { + let source = include_str!("note_commands.rs"); + for (command, body) in [ + ("delete_note", "delete_note_impl("), + ("delete_project_note", "delete_project_note_impl("), + ] { + let start = source + .find(&format!("pub async fn {command}(")) + .unwrap_or_else(|| panic!("`{command}` must be an async command")); + let end = start + + source[start..] + .find("\n}\n") + .expect("the command should have a body"); + let declaration = &source[start..end]; + let pool = declaration.find("spawn_blocking(").unwrap_or_else(|| { + panic!("`{command}` must run on the blocking pool: {declaration}") + }); + assert!( + declaration[pool..].contains(body), + "`{command}` must call `{body}` inside `spawn_blocking`: {declaration}" + ); + } + } + #[test] fn standalone_notes_start_read_only_when_the_user_wrote_them() { let store = Store::in_memory().unwrap(); diff --git a/apps/staged/src-tauri/src/note_media/mod.rs b/apps/staged/src-tauri/src/note_media/mod.rs index 868e7c04b..a8b144b60 100644 --- a/apps/staged/src-tauri/src/note_media/mod.rs +++ b/apps/staged/src-tauri/src/note_media/mod.rs @@ -193,6 +193,7 @@ pub(crate) fn ingest_note_media_with( previous_files_hashed: 0, }; let mut previous = PreviousAttachments::load(store, &scope); + let mut embedded = EmbeddedInNotes::new(markdown); // Process in document order for filename suffixes/deduplication; apply edits // backwards so offsets continue to refer to the original source. let mut edits = Vec::new(); @@ -214,17 +215,7 @@ pub(crate) fn ingest_note_media_with( return Ok(image.clone()); } let local_target = files::local_target(target)?; - let existing = stored_ref(target).or_else(|| { - // Local materialization uses the OS temp directory (on macOS - // it is not /tmp). Only our exact temp roots get this shortcut. - let path = Path::new(&local_target); - let parent = path.parent()?; - if parent != Path::new("/tmp") && parent != std::env::temp_dir() { - return None; - } - refs::parse_media_file(path.file_name()?.to_str()?.strip_prefix("staged-image-")?) - }); - if let Some((id, ext)) = existing { + if let Some((id, ext)) = stored_media(target, &local_target) { let image = store .get_image(id) .map_err(|e| e.to_string())? @@ -236,8 +227,7 @@ pub(crate) fn ingest_note_media_with( // notes. Copy cross-scope refs so subsequent moves are safe. if image.project_id == scope.project_id && image.branch_id.as_deref() == scope.branch_id - && (previous.contains(id) - || store.media_is_referenced(id).map_err(|e| e.to_string())?) + && (previous.contains(id) || embedded.contains(store, id)?) { return Ok(image); } @@ -323,6 +313,60 @@ pub(crate) fn ingest_note_media_with( outcome } +/// The stored media a reference names: a `staged-media://` target, or the +/// temp file an attachment was materialized to (`local_target` is `target` +/// as a local path). +fn stored_media<'a>(target: &'a str, local_target: &'a str) -> Option<(&'a str, &'a str)> { + stored_ref(target).or_else(|| { + // Local materialization uses the OS temp directory (on macOS + // it is not /tmp). Only our exact temp roots get this shortcut. + let path = Path::new(local_target); + let parent = path.parent()?; + if parent != Path::new("/tmp") && parent != std::env::temp_dir() { + return None; + } + refs::parse_media_file(path.file_name()?.to_str()?.strip_prefix("staged-image-")?) + }) +} + +/// Which of the stored media a note body names some note already embeds. +/// +/// Most bodies never ask: a new file is not stored yet, and a reference the +/// note already carried is kept without looking. The first one that does ask +/// reads the answer for every stored reference in the body, so an ingest +/// passes over the note bodies once at most, however many it embeds. +struct EmbeddedInNotes<'a> { + markdown: &'a str, + ids: Option>, +} + +impl<'a> EmbeddedInNotes<'a> { + fn new(markdown: &'a str) -> Self { + Self { + markdown, + ids: None, + } + } + + fn contains(&mut self, store: &Store, id: &str) -> Result { + if self.ids.is_none() { + let named: HashSet = extract_media_refs(self.markdown) + .iter() + .filter_map(|reference| { + let local_target = files::local_target(&reference.target).ok()?; + stored_media(&reference.target, &local_target).map(|(id, _)| id.to_owned()) + }) + .collect(); + self.ids = Some( + store + .media_embedded_in_notes(named.iter().map(String::as_str)) + .map_err(|e| e.to_string())?, + ); + } + Ok(self.ids.as_ref().is_some_and(|ids| ids.contains(id))) + } +} + fn save_candidate( store: &Store, scope: &NoteScope<'_>, @@ -354,20 +398,27 @@ fn save_candidate( /// The note has already been removed/updated when this runs. Protect shared /// refs and ordinary timeline/chat attachments from destructive cleanup. +/// +/// What still uses the ids is read once for the whole set, so the cost is one +/// pass over the note bodies and the chat attachments however many there are; +/// only an id nothing uses costs a row read and a delete after that. pub(crate) fn cleanup_unreferenced(store: &Store, ids: HashSet) { + let references = match store.media_references(ids.iter().map(String::as_str), None) { + Ok(references) => references, + Err(e) => { + log::warn!("Cannot clean up note media: {e}"); + return; + } + }; for id in ids { + if !references.unused(&id) { + continue; + } let result = (|| { - if store.media_is_referenced(&id).map_err(|e| e.to_string())? { - return Ok(()); - } let Some(image) = store.get_image(&id).map_err(|e| e.to_string())? else { return Ok(()); }; - if image.session_id.is_none() - || store - .image_has_chat_references(&id) - .map_err(|e| e.to_string())? - { + if image.session_id.is_none() { return Ok(()); } files::delete_media_file(store, &image) diff --git a/apps/staged/src-tauri/src/note_media/tests.rs b/apps/staged/src-tauri/src/note_media/tests.rs index c940d9bd0..ec71690ac 100644 --- a/apps/staged/src-tauri/src/note_media/tests.rs +++ b/apps/staged/src-tauri/src/note_media/tests.rs @@ -1,4 +1,5 @@ use super::*; +use crate::store::images::{take_reference_passes, ReferencePasses}; use crate::store::{Branch, Note, Project, Session}; pub(crate) const PNG: &[u8] = b"\x89PNG\r\n\x1a\nfixture"; @@ -1115,3 +1116,199 @@ fn indented_list_images_are_ingested_without_changing_indentation() { ); assert!(media_ids(&outcome.markdown).contains(id)); } + +/// `count` attachments stored on the fixture's branch under its session, each +/// with its own bytes so ingest never folds one into another. +fn stored_media(f: &Fixture, count: usize) -> Vec { + (0..count) + .map(|n| { + files::store_media_file( + &f.store, + Some(&f.branch.id), + &f.project.id, + Some(&f.session.id), + &format!("shot-{n}.png"), + &[PNG, &[n as u8][..]].concat(), + true, + ) + .unwrap() + }) + .collect() +} + +fn embedding(images: &[Image]) -> String { + images + .iter() + .map(|image| format!("![a](staged-media://{}.png)", image.id)) + .collect::>() + .join("\n\n") +} + +fn has_file(image: &Image) -> bool { + crate::store::images::image_file_path(&image.project_id, &image.id, &image.filename) + .unwrap() + .exists() +} + +/// A deleted note's media is checked in one pass over the note bodies and +/// one over the chat attachments, however many it embedded, and what goes is +/// exactly what the per-image checks would have let go. +#[test] +fn cleanup_checks_every_id_in_one_pass_and_collects_what_the_per_image_checks_would() { + let f = Fixture::new(); + let mut images = stored_media(&f, 8); + // A branch-timeline attachment: no session, so never note media to collect. + images.push( + files::store_media_file( + &f.store, + Some(&f.branch.id), + &f.project.id, + None, + "timeline.png", + PNG, + true, + ) + .unwrap(), + ); + let note = Note::new(&f.branch.id, "Shots", &embedding(&images)); + let other = Note::new(&f.branch.id, "Other", &embedding(&images[..1])); + f.store.create_note(¬e).unwrap(); + f.store.create_note(&other).unwrap(); + f.store + .add_session_message_with_images( + &f.session.id, + crate::store::MessageRole::User, + "look", + std::slice::from_ref(&images[1].id), + ) + .unwrap(); + f.store.delete_note(¬e.id).unwrap(); + + // The rule, one id at a time, as the cleanup applied it before. + let kept: HashSet = images + .iter() + .filter(|image| { + f.store.media_is_referenced(&image.id).unwrap() + || image.session_id.is_none() + || f.store.image_has_chat_references(&image.id).unwrap() + }) + .map(|image| image.id.clone()) + .collect(); + assert_eq!( + kept, + HashSet::from([ + images[0].id.clone(), + images[1].id.clone(), + images[8].id.clone() + ]) + ); + + take_reference_passes(); + cleanup_unreferenced(&f.store, media_ids(¬e.content)); + assert_eq!( + take_reference_passes(), + ReferencePasses { notes: 1, chat: 1 } + ); + + for image in &images { + let expected = kept.contains(&image.id); + assert_eq!( + f.store.get_image(&image.id).unwrap().is_some(), + expected, + "{} row", + image.filename + ); + assert_eq!(has_file(image), expected, "{} file", image.filename); + } + + // Nothing to check: no pass at all. + cleanup_unreferenced(&f.store, HashSet::new()); + assert_eq!(take_reference_passes(), ReferencePasses::default()); +} + +/// A project note's media and its children's are collected together. +#[test] +fn project_note_deletion_checks_the_whole_family_in_one_pass() { + let f = Fixture::new(); + let images = stored_media(&f, 4); + let parent = crate::store::ProjectNote::new(&f.project.id, "Parent", &embedding(&images[..2])); + f.store.create_project_note(&parent).unwrap(); + for (n, image) in images[2..].iter().enumerate() { + let mut child = Note::new( + &f.branch.id, + &format!("Child {n}"), + &embedding(std::slice::from_ref(image)), + ); + child.parent_project_note_id = Some(parent.id.clone()); + f.store.create_note(&child).unwrap(); + } + let ids = project_note_media_ids(&f.store, &parent.id).unwrap(); + assert_eq!(ids.len(), 4); + f.store.delete_project_note(&parent.id).unwrap(); + + take_reference_passes(); + cleanup_unreferenced(&f.store, ids); + assert_eq!( + take_reference_passes(), + ReferencePasses { notes: 1, chat: 1 } + ); + for image in &images { + assert!(f.store.get_image(&image.id).unwrap().is_none()); + assert!(!has_file(image)); + } +} + +/// A body repeating stored references it did not carry before asks whether a +/// note embeds each: once for all of them, with the per-image answers. +#[test] +fn ingest_checks_stored_references_in_one_pass_and_keeps_what_the_per_image_check_would() { + let f = Fixture::new(); + let images = stored_media(&f, 6); + let other = Note::new(&f.branch.id, "Other", &embedding(&images[..3])); + f.store.create_note(&other).unwrap(); + let referenced: Vec = images + .iter() + .map(|image| f.store.media_is_referenced(&image.id).unwrap()) + .collect(); + assert_eq!(referenced, [true, true, true, false, false, false]); + + take_reference_passes(); + let outcome = f.ingest("", &embedding(&images)); + assert_eq!( + take_reference_passes(), + ReferencePasses { notes: 1, chat: 0 } + ); + + // Media a note embeds is referenced as it is; the rest is copied. + for (image, referenced) in images.iter().zip(referenced) { + assert_eq!( + outcome.kept_ids.contains(&image.id), + referenced, + "{}", + image.filename + ); + assert_eq!( + outcome.markdown.contains(&image.id), + referenced, + "{}", + image.filename + ); + } + assert_eq!(outcome.kept_ids.len(), 6); +} + +/// New files are not stored yet and references the note already carried are +/// kept without looking, so the usual ingest makes no pass at all. +#[test] +fn ingest_of_new_files_and_repeated_references_makes_no_reference_pass() { + let f = Fixture::new(); + std::fs::write(f.dir.path().join("shot.png"), PNG).unwrap(); + take_reference_passes(); + + let first = f.ingest("", "![a](shot.png)"); + assert_eq!(first.kept_ids.len(), 1); + let again = f.ingest(&first.markdown, &first.markdown); + assert_eq!(again.kept_ids, first.kept_ids); + + assert_eq!(take_reference_passes(), ReferencePasses::default()); +} diff --git a/apps/staged/src-tauri/src/note_transfer.rs b/apps/staged/src-tauri/src/note_transfer.rs index 825babfa2..db74935b1 100644 --- a/apps/staged/src-tauri/src/note_transfer.rs +++ b/apps/staged/src-tauri/src/note_transfer.rs @@ -808,9 +808,14 @@ fn finish_all( media.commit(); } if mode == TransferMode::Move { - for content in previous_contents { - cleanup_unreferenced(store, refs::media_ids(content)); - } + // Every note's media at once: one pass for the whole move. + cleanup_unreferenced( + store, + previous_contents + .iter() + .flat_map(|content| refs::media_ids(content)) + .collect(), + ); } Ok(value) } @@ -902,14 +907,13 @@ struct MoveSource<'a> { } impl MoveSource<'_> { - /// Whether `image` moves with the note as it is; see the module docs. - fn takes(&self, store: &Store, image: &Image) -> Result { - Ok(image.project_id == self.project_id + /// Whether `image` is a row the note could take along as it is: one in + /// the scope the note is leaving, with a session. It goes when the note + /// is also the only thing using it; see the module docs. + fn holds(&self, image: &Image) -> bool { + image.project_id == self.project_id && image.branch_id.as_deref() == self.branch_id && image.session_id.is_some() - && store - .media_is_exclusive_to(&image.id, self.note_id) - .map_err(|e| e.to_string())?) } } @@ -952,6 +956,9 @@ fn stage_media( // in place once however often the note embeds it. let mut landed = HashMap::>::new(); let mut edits = Vec::new(); + // What else uses the note's media, read for all of it the first time a + // row could move as it is: one pass per note, not one per attachment. + let mut references = None; let result = (|| -> Result<(), String> { for reference in refs::extract_media_refs(content) { let Some((id, _)) = refs::stored_ref(&reference.target) else { @@ -974,8 +981,22 @@ fn stage_media( ) .map_err(|e| e.to_string())?; let rehomes = match &rehome { - Some(from) => from.takes(store, &source)?, - None => false, + Some(from) if from.holds(&source) => { + if references.is_none() { + references = Some( + store + .media_references( + refs::media_ids(content).iter().map(String::as_str), + Some(from.note_id), + ) + .map_err(|e| e.to_string())?, + ); + } + references + .as_ref() + .is_some_and(|references| references.unused(&source.id)) + } + _ => false, }; let replacement = if rehomes { if !rehome_file(&source_path, project_id, &source, &mut staged)? { @@ -1252,6 +1273,7 @@ mod tests { use super::*; use crate::note_media::tests::PNG; + use crate::store::images::{take_reference_passes, ReferencePasses}; use crate::store::models::{Project, ProjectRepo, Session}; struct Fixture { @@ -2002,6 +2024,208 @@ mod tests { ); } + /// Staging a move reads what else uses the note's media once, and takes + /// along exactly the rows the per-image check calls the note's alone. + #[test] + fn staging_a_move_checks_its_media_in_one_pass_and_rehomes_what_the_per_image_check_would() { + let f = fixture(); + let alone = [f.attachment("one.png"), f.attachment("two.png")]; + let embedded = f.attachment("embedded.png"); + f.note( + "Other", + &format!("![o](staged-media://{}.png)", embedded.id), + ); + let chatted = f.attachment("chatted.png"); + f.store + .add_session_message_with_images( + chatted.session_id.as_deref().unwrap(), + crate::store::models::MessageRole::User, + "look", + std::slice::from_ref(&chatted.id), + ) + .unwrap(); + let timeline = f.attachment_with_session("timeline.png", None); + let all = [&alone[0], &alone[1], &embedded, &chatted, &timeline]; + let note = f.note( + "Shots", + &all.map(|image| format!("![a](staged-media://{}.png)", image.id)) + .join(" "), + ); + f.attach_repo(RepoKey { + github_repo: "acme/widgets".into(), + subpath: Some("apps/web".into()), + }) + .unwrap(); + let target_branch = f.target_branch().unwrap(); + let source = || MoveSource { + note_id: ¬e.id, + project_id: &f.source.id, + branch_id: Some(&f.source_branch.id), + }; + // The rule, one image at a time, as staging applied it before. + let expected: HashSet = all + .iter() + .filter(|image| { + source().holds(image) && f.store.media_is_exclusive_to(&image.id, ¬e.id).unwrap() + }) + .map(|image| image.id.clone()) + .collect(); + assert_eq!( + expected, + HashSet::from([alone[0].id.clone(), alone[1].id.clone()]) + ); + + take_reference_passes(); + let media = stage_media( + &f.store, + ¬e.content, + Some(&target_branch.id), + &f.target.id, + None, + Some(source()), + &mut HashSet::new(), + ) + .unwrap(); + assert_eq!( + take_reference_passes(), + ReferencePasses { notes: 1, chat: 1 } + ); + + let rehomed: HashSet = media.rehomed.iter().map(|row| row.id.clone()).collect(); + assert_eq!(rehomed, expected); + assert_eq!(media.images.len(), 3); + media.discard(&f.store); + + // A copy takes nothing along, so it has nothing to ask. + let copy = stage_media( + &f.store, + ¬e.content, + Some(&target_branch.id), + &f.target.id, + None, + None, + &mut HashSet::new(), + ) + .unwrap(); + assert_eq!(take_reference_passes(), ReferencePasses::default()); + assert!(copy.rehomed.is_empty()); + assert_eq!(copy.images.len(), 5); + copy.discard(&f.store); + } + + /// A move passes over the note bodies and the chat attachments three + /// times (staging, the check under the transaction, the collection after + /// the commit), whether the note embeds one image or eight. + #[tokio::test] + async fn a_move_makes_three_reference_passes_however_many_images_the_note_embeds() { + for count in [1, 8] { + let f = fixture(); + let images: Vec = (0..count) + .map(|n| f.attachment(&format!("shot-{n}.png"))) + .collect(); + let note = f.note( + "Shots", + &images + .iter() + .map(|image| format!("![a](staged-media://{}.png)", image.id)) + .collect::>() + .join("\n\n"), + ); + take_reference_passes(); + + transfer_branch_note( + &f.store, + ¬e.id, + &f.target.id, + TransferMode::Move, + |key| std::future::ready(f.attach_repo(key)), + ) + .await + .unwrap(); + + assert_eq!( + take_reference_passes(), + ReferencePasses { notes: 3, chat: 3 }, + "{count} images" + ); + // Every image went along as it is and survived the collection. + for image in &images { + let row = f.store.get_image(&image.id).unwrap().unwrap(); + assert_eq!(row.project_id, f.target.id); + assert!(image_file_path(&f.target.id, &row.id, &row.filename) + .unwrap() + .exists()); + } + } + } + + /// A copy takes no media from the source, so it never asks what uses it. + #[tokio::test] + async fn a_copy_makes_no_reference_pass() { + let f = fixture(); + let (note, _) = f.media_note(); + take_reference_passes(); + + transfer_branch_note( + &f.store, + ¬e.id, + &f.target.id, + TransferMode::Copy, + |key| std::future::ready(f.attach_repo(key)), + ) + .await + .unwrap(); + + assert_eq!(take_reference_passes(), ReferencePasses::default()); + } + + /// A project-note move stages and re-checks each note against its own + /// body, so those two passes are per note that has media to take along; + /// the collection after the commit is one pass for the whole family. + #[tokio::test] + async fn a_project_note_move_passes_once_per_note_to_stage_and_recheck_and_once_to_collect() { + let f = fixture(); + let parent_image = f.project_attachment(); + let parent = ProjectNote::new( + &f.source.id, + "Plan", + &format!("![a](staged-media://{}.png)", parent_image.id), + ); + f.store.create_project_note(&parent).unwrap(); + let child_images = [f.attachment("one.png"), f.attachment("two.png")]; + let (child, _) = f.child_of(&parent, &f.source_branch.id, "Findings"); + f.store + .update_note_content( + &child.id, + &child_images + .each_ref() + .map(|image| format!("![a](staged-media://{}.png)", image.id)) + .join(" "), + ) + .unwrap(); + take_reference_passes(); + + transfer_project_note( + &f.store, + &parent.id, + &f.target.id, + TransferMode::Move, + |key| std::future::ready(f.attach_repo(key)), + ) + .await + .unwrap(); + + // Two notes staged, two re-checked, one collection. + assert_eq!( + take_reference_passes(), + ReferencePasses { notes: 5, chat: 5 } + ); + for image in [&parent_image, &child_images[0], &child_images[1]] { + let row = f.store.get_image(&image.id).unwrap().unwrap(); + assert_eq!(row.project_id, f.target.id); + } + } + /// The files are linked into the destination before the transaction that /// re-homes their rows, and the source path stays until it commits: the /// file is at whichever path the row names throughout. A transaction that diff --git a/apps/staged/src-tauri/src/session_commands.rs b/apps/staged/src-tauri/src/session_commands.rs index be3205502..20f4937cb 100644 --- a/apps/staged/src-tauri/src/session_commands.rs +++ b/apps/staged/src-tauri/src/session_commands.rs @@ -9290,6 +9290,121 @@ mod tests { ); } + /// The attachments a queued start hands the runner are read with one pass + /// over the note bodies however many were queued with the session, where + /// each attachment used to cost a scan of both note tables. The start + /// needs a concrete app runtime to drive, so this queues the session as + /// the commands do and makes the read the start makes; the source test + /// below pins that the start makes it once. + #[test] + fn queued_start_reads_its_attachments_in_one_note_pass() { + use crate::store::images::{take_reference_passes, ReferencePasses}; + + let queue = |attached: usize| { + let (store, branch) = setup_branch_store(); + let image_ids: Vec = (0..attached) + .map(|n| { + let image = store::Image::new( + Some(&branch.id), + &branch.project_id, + &format!("shot-{n}.png"), + "image/png", + 1, + true, + ); + store.create_image(&image).unwrap(); + image.id + }) + .collect(); + // Several notes for the pass to read. With more than one + // attachment, a project note embeds the last, which makes it + // note media and no longer something to send with the prompt. + let embedded = image_ids.last().filter(|_| attached > 1); + for n in 0..3 { + let content = format!("![other](staged-media://unrelated-{n}.png)"); + store + .create_note(&store::Note::new_standalone(&branch.id, "Other", &content)) + .unwrap(); + } + if let Some(id) = embedded { + let content = format!("![shot](staged-media://{id}.png)"); + store + .create_project_note(&store::ProjectNote::new( + &branch.project_id, + "Plan", + &content, + )) + .unwrap(); + } + let queued = insert_queued_branch_session( + &store, + None, + &branch.id, + "look at these", + &BranchSessionType::Note, + None, + &image_ids, + None, + None, + None, + ) + .unwrap(); + let sent: Vec = image_ids + .iter() + .filter(|id| Some(*id) != embedded) + .cloned() + .collect(); + (store, queued.session_id, sent) + }; + + for attached in [1, 2, 8, 32] { + let (store, session_id, sent) = queue(attached); + take_reference_passes(); + assert_eq!( + store.get_image_ids_for_session(&session_id).unwrap(), + sent, + "{attached} attached" + ); + assert_eq!( + take_reference_passes(), + ReferencePasses { notes: 1, chat: 0 }, + "{attached} attached" + ); + } + + // Nothing attached: nothing to ask the notes about. + let (store, session_id, _) = queue(0); + take_reference_passes(); + assert!(store + .get_image_ids_for_session(&session_id) + .unwrap() + .is_empty()); + assert_eq!(take_reference_passes(), ReferencePasses::default()); + } + + /// The queued start reads its attachments once, through the lookup the + /// test above holds to one note pass, and asks nothing else about media + /// references. Pinned by source text, like the claim invariants below, + /// because the start needs a concrete app runtime to drive. + #[test] + fn queued_start_reads_its_attachments_once() { + let source = include_str!("session_commands.rs"); + let start = "async fn start_queued_session_for_branch("; + let body = &source[source.find(start).expect(start)..]; + let body = &body[..body.find("\n}\n").expect("fn end")]; + + assert_eq!( + body.matches(".get_image_ids_for_session(&session_id)") + .count(), + 1, + "`{start}` reads the session's attachments once" + ); + assert_eq!(body.matches("get_image_ids_for_session").count(), 1); + for other in ["media_references(", "media_embedded_in_notes("] { + assert!(!body.contains(other), "`{start}` calls `{other}`"); + } + } + /// The actor a control caller hands the shared start bodies lands on the /// row for both insert shapes; the desktop's `None` leaves it unset. #[test] diff --git a/apps/staged/src-tauri/src/store/images.rs b/apps/staged/src-tauri/src/store/images.rs index 26eb868b6..a2726e9aa 100644 --- a/apps/staged/src-tauri/src/store/images.rs +++ b/apps/staged/src-tauri/src/store/images.rs @@ -22,9 +22,257 @@ const UNREADABLE_DIMENSIONS: ImageDimensions = ImageDimensions { density: 0, }; +/// The note bodies that embed any stored media, leaving out note `?1`. Note +/// ids are UUIDs, so one id names at most one row across both tables. +const NOTE_BODIES_SQL: [&str; 2] = [ + "SELECT content FROM notes + WHERE id IS NOT ?1 AND instr(content, 'staged-media://') > 0", + "SELECT content FROM project_notes + WHERE id IS NOT ?1 AND instr(content, 'staged-media://') > 0", +]; + +/// The attachment list of every sent and queued chat message that has one. +/// +/// `image_ids IS NOT NULL` is the predicate of `idx_session_messages_image_ids` +/// (migration 0049, a partial index over the messages that attach something), +/// so the first statement reads that index and never a row of +/// `session_messages`. `images_tests.rs` pins the plan. +pub(super) const CHAT_ATTACHMENTS_SQL: [&str; 2] = [ + "SELECT image_ids FROM session_messages WHERE image_ids IS NOT NULL", + "SELECT image_ids FROM queued_session_messages WHERE image_ids IS NOT NULL", +]; + +/// Whether any sent or queued chat message attaches the image whose quoted id +/// is `?1`: the per-image form of [`CHAT_ATTACHMENTS_SQL`], kept for the tests +/// that hold the batched pass to it. +/// +/// `image_ids IS NOT NULL` changes no result, since `instr` of NULL is NULL, +/// but it is what lets SQLite read `idx_session_messages_image_ids` instead of +/// every row of `session_messages`: the planner does not infer the index's +/// `WHERE` from `instr(...) > 0`. +#[cfg(test)] +pub(super) const CHAT_REFERENCE_SQL: &str = "SELECT EXISTS( + SELECT 1 FROM session_messages + WHERE image_ids IS NOT NULL AND instr(image_ids, ?1) > 0 + UNION ALL + SELECT 1 FROM queued_session_messages + WHERE image_ids IS NOT NULL AND instr(image_ids, ?1) > 0)"; + +/// The images and PDFs linked to session `?1`, before the ones a note embeds +/// are dropped ([`Store::get_image_ids_for_session`]). Videos and other note +/// media are linked to a session too and are never chat attachments. +/// +/// Nothing indexes `images.session_id`, so this reads the `images` table: one +/// row per stored file, small beside the note bodies it used to be joined to. +const SESSION_ATTACHMENTS_SQL: &str = "SELECT id FROM images + WHERE session_id = ?1 + AND (mime_type LIKE 'image/%' OR mime_type = 'application/pdf')"; + +/// [`Store::get_image_ids_for_session`] as one statement, with the note +/// check as two correlated scans of the note tables for each attachment: +/// the query it ran before the note pass, kept for the test that holds the +/// two to the same answer. +#[cfg(test)] +const SESSION_ATTACHMENTS_PER_IMAGE_SQL: &str = "SELECT id FROM images WHERE session_id = ?1 + AND (mime_type LIKE 'image/%' OR mime_type = 'application/pdf') + AND NOT EXISTS (SELECT 1 FROM notes WHERE instr(content, 'staged-media://' || images.id || '.') > 0) + AND NOT EXISTS (SELECT 1 FROM project_notes WHERE instr(content, 'staged-media://' || images.id || '.') > 0)"; + +/// What still uses each of a set of candidate media ids, read in one pass +/// over the note bodies and the chat attachment lists instead of one pass per +/// id ([`Store::media_references`]). +#[derive(Debug, Default, PartialEq, Eq)] +pub(crate) struct MediaReferences { + /// The ids the pass was asked about: the only ones it can clear. + asked: HashSet, + notes: HashSet, + chat: HashSet, +} + +impl MediaReferences { + /// Whether a note embeds `id`, not counting the note the pass left out. + pub(crate) fn in_notes(&self, id: &str) -> bool { + self.notes.contains(id) + } + + /// Whether a sent or queued chat message attaches `id`. + pub(crate) fn in_chat(&self, id: &str) -> bool { + self.chat.contains(id) + } + + /// Whether nothing the pass read uses `id`. With a note left out, that + /// note is the only thing using it, so the row can follow the note to + /// another scope without breaking anything else. + /// + /// This is the answer that lets a caller delete a file or take a row + /// along, so it is given only for an id the pass was asked about. The + /// pass looked for nothing else, and finding no use of an id it never + /// looked for says nothing: any other id is reported as in use. + pub(crate) fn unused(&self, id: &str) -> bool { + self.asked.contains(id) && !self.in_notes(id) && !self.in_chat(id) + } +} + +/// How many passes over the note bodies and over the chat attachment lists +/// this thread has made, so a test can hold an operation to one of each. +#[cfg(test)] +#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] +pub(crate) struct ReferencePasses { + pub notes: usize, + pub chat: usize, +} + +#[cfg(test)] +thread_local! { + static REFERENCE_PASSES: std::cell::Cell = + const { std::cell::Cell::new(ReferencePasses { notes: 0, chat: 0 }) }; +} + +/// The passes made on this thread since the last call, resetting the count. +#[cfg(test)] +pub(crate) fn take_reference_passes() -> ReferencePasses { + REFERENCE_PASSES.with(std::cell::Cell::take) +} + +#[cfg(test)] +fn count_reference_pass(count: impl FnOnce(&mut ReferencePasses)) { + REFERENCE_PASSES.with(|passes| { + let mut current = passes.get(); + count(&mut current); + passes.set(current); + }); +} + +/// The ids whose needle occurs in a text `statements` yield, each run with +/// `params` and read in turn until every needle is found. `needles` pairs a +/// needle with its id. +/// +/// The needles are matched as plain substrings, as `instr` matched them one +/// id at a time, on the bytes SQLite holds: a value that is not valid UTF-8 +/// is still searched rather than failing the pass. +fn ids_found( + conn: &rusqlite::Connection, + statements: &[&str], + params: &[&dyn rusqlite::ToSql], + mut needles: Vec<(String, &str)>, +) -> Result, StoreError> { + let mut found = HashSet::new(); + for sql in statements { + if needles.is_empty() { + break; + } + let mut stmt = conn.prepare(sql)?; + let mut rows = stmt.query(params)?; + while let Some(row) = rows.next()? { + let value = row.get_ref(0)?; + let text = + String::from_utf8_lossy(value.as_bytes().map_err(|e| StoreError(e.to_string()))?); + needles.retain(|(needle, id)| { + let hit = text.contains(needle.as_str()); + if hit { + found.insert((*id).to_owned()); + } + !hit + }); + if needles.is_empty() { + break; + } + } + } + Ok(found) +} + impl Store { + /// Which of `ids` are still in use, leaving `except_note_id` (a branch or + /// project note) out of the count: one pass over the note bodies and one + /// over the chat attachment lists, under one lock, however many ids are + /// asked about. + /// /// Conservative reference check: even a reference inside an example keeps /// its file. No note ownership columns or reconciliation state are needed. + pub(crate) fn media_references<'a>( + &self, + ids: impl IntoIterator, + except_note_id: Option<&str>, + ) -> Result { + let ids: Vec<&str> = ids.into_iter().collect(); + if ids.is_empty() { + return Ok(MediaReferences::default()); + } + let conn = self.conn.lock().unwrap(); + Self::media_references_on(&conn, ids, except_note_id) + } + + /// [`Self::media_references`] on an open connection, so a transaction can + /// confirm what it is about to re-home under its own snapshot. + pub(super) fn media_references_on<'a>( + conn: &rusqlite::Connection, + ids: impl IntoIterator, + except_note_id: Option<&str>, + ) -> Result { + let ids: Vec<&str> = ids.into_iter().collect(); + Ok(MediaReferences { + asked: ids.iter().map(|id| (*id).to_owned()).collect(), + notes: Self::embedded_in_notes(conn, &ids, except_note_id)?, + chat: Self::attached_in_chat(conn, &ids)?, + }) + } + + /// Which of `ids` some note embeds: the note half of + /// [`Self::media_references`], for a caller with no use for the chat half. + pub(crate) fn media_embedded_in_notes<'a>( + &self, + ids: impl IntoIterator, + ) -> Result, StoreError> { + let ids: Vec<&str> = ids.into_iter().collect(); + if ids.is_empty() { + return Ok(HashSet::new()); + } + let conn = self.conn.lock().unwrap(); + Self::embedded_in_notes(&conn, &ids, None) + } + + /// Which of `ids` a note other than `except_note_id` embeds. + fn embedded_in_notes( + conn: &rusqlite::Connection, + ids: &[&str], + except_note_id: Option<&str>, + ) -> Result, StoreError> { + if ids.is_empty() { + return Ok(HashSet::new()); + } + #[cfg(test)] + count_reference_pass(|passes| passes.notes += 1); + let needles = ids + .iter() + .map(|id| (format!("staged-media://{id}."), *id)) + .collect(); + ids_found(conn, &NOTE_BODIES_SQL, params![except_note_id], needles) + } + + /// Which of `ids` a sent or queued chat message attaches. + fn attached_in_chat( + conn: &rusqlite::Connection, + ids: &[&str], + ) -> Result, StoreError> { + if ids.is_empty() { + return Ok(HashSet::new()); + } + #[cfg(test)] + count_reference_pass(|passes| passes.chat += 1); + // Image IDs are UUIDs; quote the exact JSON string to avoid substrings. + let needles = ids.iter().map(|id| (format!("\"{id}\""), *id)).collect(); + ids_found(conn, &CHAT_ATTACHMENTS_SQL, params![], needles) + } +} + +/// The checks one id at a time, as every caller ran them before +/// [`Store::media_references`] answered for a whole set in one pass, and the +/// session attachment lookup as it ran before it used that pass. Nothing +/// outside the tests calls these now; they stay as the definition the batched +/// pass is held to (`images_tests.rs`). +#[cfg(test)] +impl Store { pub(crate) fn media_is_referenced(&self, id: &str) -> Result { let conn = self.conn.lock().unwrap(); Self::referenced_by_notes(&conn, id, None) @@ -37,31 +285,18 @@ impl Store { /// Whether `note_id` (a branch or project note) is the only thing using /// media `id`: no other note embeds it and no chat message attaches it. - /// Such a row can follow the note to another scope without breaking - /// anything else. pub(crate) fn media_is_exclusive_to( &self, id: &str, note_id: &str, ) -> Result { let conn = self.conn.lock().unwrap(); - Self::media_exclusive_to(&conn, id, note_id) + Ok(!Self::referenced_by_notes(&conn, id, Some(note_id))? + && !Self::referenced_by_chat(&conn, id)?) } - /// [`Self::media_is_exclusive_to`] on an open connection, so a - /// transaction can confirm it under its own snapshot. - pub(super) fn media_exclusive_to( - conn: &rusqlite::Connection, - id: &str, - note_id: &str, - ) -> Result { - Ok(!Self::referenced_by_notes(conn, id, Some(note_id))? - && !Self::referenced_by_chat(conn, id)?) - } - - /// Whether any note other than `except_note_id` embeds media `id`. Note - /// ids are UUIDs, so one id names at most one row across both tables. - fn referenced_by_notes( + /// Whether any note other than `except_note_id` embeds media `id`. + pub(super) fn referenced_by_notes( conn: &rusqlite::Connection, id: &str, except_note_id: Option<&str>, @@ -76,18 +311,31 @@ impl Store { .map_err(Into::into) } - fn referenced_by_chat(conn: &rusqlite::Connection, id: &str) -> Result { + pub(super) fn referenced_by_chat( + conn: &rusqlite::Connection, + id: &str, + ) -> Result { // Image IDs are UUIDs; quote the exact JSON string to avoid substrings. let needle = format!("\"{id}\""); - conn.query_row( - "SELECT EXISTS(SELECT 1 FROM session_messages WHERE instr(image_ids, ?1) > 0 - UNION ALL SELECT 1 FROM queued_session_messages WHERE instr(image_ids, ?1) > 0)", - params![needle], - |row| row.get(0), - ) - .map_err(Into::into) + conn.query_row(CHAT_REFERENCE_SQL, params![needle], |row| row.get(0)) + .map_err(Into::into) } + /// [`Self::get_image_ids_for_session`] as it ran before the note pass. + pub(crate) fn get_image_ids_for_session_per_image( + &self, + session_id: &str, + ) -> Result, StoreError> { + let conn = self.conn.lock().unwrap(); + let mut stmt = conn.prepare(SESSION_ATTACHMENTS_PER_IMAGE_SQL)?; + let ids = stmt + .query_map(params![session_id], |row| row.get(0))? + .collect::, _>>()?; + Ok(ids) + } +} + +impl Store { pub fn create_image(&self, image: &Image) -> Result<(), StoreError> { let conn = self.conn.lock().unwrap(); Self::insert_image(&conn, image)?; @@ -314,15 +562,23 @@ impl Store { /// Return chat attachments linked to a session, excluding note-owned media /// so restarted sessions don't send videos as ACP image input. PDFs are /// chat attachments too; the runner renders their pages for the prompt. + /// + /// The ids come back in the order the table yields them. Whatever a note + /// embeds is dropped by one pass over the note bodies for the whole set, + /// the note half of [`Self::media_references`], under the same lock, + /// rather than by a scan of both note tables for each attachment. A + /// session with nothing attached makes no pass. pub fn get_image_ids_for_session(&self, session_id: &str) -> Result, StoreError> { let conn = self.conn.lock().unwrap(); - let mut stmt = conn.prepare("SELECT id FROM images WHERE session_id = ?1 - AND (mime_type LIKE 'image/%' OR mime_type = 'application/pdf') - AND NOT EXISTS (SELECT 1 FROM notes WHERE instr(content, 'staged-media://' || images.id || '.') > 0) - AND NOT EXISTS (SELECT 1 FROM project_notes WHERE instr(content, 'staged-media://' || images.id || '.') > 0)")?; - let ids = stmt + let mut stmt = conn.prepare(SESSION_ATTACHMENTS_SQL)?; + let mut ids = stmt .query_map(params![session_id], |row| row.get(0))? .collect::, _>>()?; + let embedded = { + let candidates: Vec<&str> = ids.iter().map(String::as_str).collect(); + Self::embedded_in_notes(&conn, &candidates, None)? + }; + ids.retain(|id| !embedded.contains(id)); Ok(ids) } diff --git a/apps/staged/src-tauri/src/store/images_tests.rs b/apps/staged/src-tauri/src/store/images_tests.rs new file mode 100644 index 000000000..4e46139dc --- /dev/null +++ b/apps/staged/src-tauri/src/store/images_tests.rs @@ -0,0 +1,767 @@ +//! Tests for the image-reference checks in `images.rs`: what counts as a +//! reference, that the batched pass answers exactly what the per-image checks +//! answer, that the chat statements read the partial index from migration +//! 0049 instead of scanning `session_messages`, and that a session's +//! attachment lookup returns what its one-statement form returned. + +use std::collections::HashSet; +use std::path::Path; + +use rusqlite::{params, Connection}; + +use super::images::{ + take_reference_passes, ReferencePasses, CHAT_ATTACHMENTS_SQL, CHAT_REFERENCE_SQL, +}; +use super::models::*; +use super::Store; + +const IMAGE_IDS_INDEX: &str = "idx_session_messages_image_ids"; + +/// The `detail` column of each `EXPLAIN QUERY PLAN` row for `sql`, in plan +/// order. +fn query_plan(conn: &Connection, sql: &str, params: &[&dyn rusqlite::ToSql]) -> Vec { + let mut stmt = conn.prepare(&format!("EXPLAIN QUERY PLAN {sql}")).unwrap(); + let details = stmt + .query_map(params, |row| row.get::<_, String>(3)) + .unwrap() + .collect::, _>>() + .unwrap(); + details +} + +/// Assert the chat reference checks read `session_messages` only through the +/// partial index, and as a covering index, so they never touch a table row: +/// the statement the batched pass runs, and the per-image statement the tests +/// hold it to. +/// +/// Plan details read `SCAN ...` or `SEARCH
...`; the table is +/// matched as a whole word because `queued_session_messages` contains the +/// other table's name. +pub(super) fn assert_chat_reference_check_reads_the_index(conn: &Connection) { + let statements: [(&str, &[&dyn rusqlite::ToSql]); 2] = [ + (CHAT_ATTACHMENTS_SQL[0], params![]), + (CHAT_REFERENCE_SQL, params!["\"some-image\""]), + ]; + for (sql, params) in statements { + let plan = query_plan(conn, sql, params); + let message_steps: Vec<&str> = plan + .iter() + .map(String::as_str) + .filter(|detail| detail.split_whitespace().nth(1) == Some("session_messages")) + .collect(); + assert_eq!( + message_steps.len(), + 1, + "expected one step over session_messages in {plan:?} for {sql}" + ); + assert!( + message_steps[0].contains(&format!("USING COVERING INDEX {IMAGE_IDS_INDEX}")), + "the chat reference check should read {IMAGE_IDS_INDEX}, not scan \ + session_messages; plan: {plan:?} for {sql}" + ); + } +} + +fn running_session(store: &Store) -> Session { + let session = Session::new_running("work", Path::new("/tmp")); + store.create_session(&session).unwrap(); + session +} + +#[test] +fn test_chat_reference_check_reads_the_image_ids_index() { + let store = Store::in_memory().unwrap(); + let conn = store.conn.lock().unwrap(); + // The app never runs ANALYZE, so this is the plan a real store gets. + assert_chat_reference_check_reads_the_index(&conn); +} + +#[test] +fn test_chat_reference_check_keeps_the_index_once_the_table_has_statistics() { + let store = Store::in_memory().unwrap(); + let session = running_session(&store); + // Mostly messages without attachments, as in a real transcript. + for n in 0..600 { + let images: Vec = if n % 100 == 0 { + vec![format!("image-{n}")] + } else { + Vec::new() + }; + store + .add_session_message_with_images( + &session.id, + MessageRole::User, + "a message long enough to give the table wider rows than the index", + &images, + ) + .unwrap(); + } + + let conn = store.conn.lock().unwrap(); + conn.execute_batch("ANALYZE;").unwrap(); + assert_chat_reference_check_reads_the_index(&conn); + + // Only the attaching messages are in the index. + let indexed: i64 = conn + .query_row( + &format!( + "SELECT COUNT(*) FROM session_messages INDEXED BY {IMAGE_IDS_INDEX} + WHERE image_ids IS NOT NULL" + ), + [], + |row| row.get(0), + ) + .unwrap(); + assert_eq!(indexed, 6); +} + +#[test] +fn test_image_has_chat_references_matches_sent_and_queued_attachments_only() { + let store = Store::in_memory().unwrap(); + let session = running_session(&store); + + let sent = "11111111-aaaa-4aaa-8aaa-000000000001".to_string(); + let also_sent = "11111111-aaaa-4aaa-8aaa-000000000002".to_string(); + let queued = "22222222-bbbb-4bbb-8bbb-000000000001".to_string(); + let mentioned = "33333333-cccc-4ccc-8ccc-000000000001"; + + store + .add_session_message_with_images( + &session.id, + MessageRole::User, + "two attachments", + &[sent.clone(), also_sent.clone()], + ) + .unwrap(); + // No attachments: `image_ids` is NULL, and naming an id in the text is + // not a reference. + store + .add_session_message(&session.id, MessageRole::User, mentioned) + .unwrap(); + store + .add_queued_session_message(&session.id, "later", std::slice::from_ref(&queued), None) + .unwrap(); + + assert!(store.image_has_chat_references(&sent).unwrap()); + assert!(store.image_has_chat_references(&also_sent).unwrap()); + assert!(store.image_has_chat_references(&queued).unwrap()); + assert!(!store.image_has_chat_references(mentioned).unwrap()); + // The needle is the quoted id, so part of an attached id does not match. + assert!(!store + .image_has_chat_references(&sent[..sent.len() - 1]) + .unwrap()); + assert!(!store + .image_has_chat_references("44444444-dddd-4ddd-8ddd-000000000001") + .unwrap()); +} + +/// A store holding one of everything a reference check has to tell apart, +/// the candidate ids to ask about, and the notes a caller might leave out. +struct References { + store: Store, + candidates: Vec, + /// `None`, every branch and project note, and an id that names no note. + exclusions: Vec>, + only_one: String, + in_two: String, + in_project: String, + in_example: String, + in_blob: String, + sent: String, + queued: String, + note_and_chat: String, + unused: String, + bare: String, + mentioned: String, + owner: String, + blob_note: String, +} + +fn references() -> References { + let store = Store::in_memory().unwrap(); + let project = Project::new("test/references"); + store.create_project(&project).unwrap(); + let branch = Branch::new(&project.id, "feature", "main"); + store.create_branch(&branch).unwrap(); + let session = running_session(&store); + + let id = |n: u32| format!("aaaaaaaa-0000-4000-8000-{n:012}"); + let embed = |id: &str| format!("![shot](staged-media://{id}.png)"); + let only_one = id(1); + let in_two = id(2); + let in_project = id(3); + let in_example = id(4); + let in_blob = id(5); + let sent = id(6); + let queued = id(7); + let note_and_chat = id(8); + let unused = id(9); + let bare = id(10); + let mentioned = id(11); + + // Embeds three ids, names `mentioned` without the scheme, and ends on a + // reference with no extension: neither of those two is a reference. + let owner = Note::new_standalone( + &branch.id, + "Owner", + &format!( + "{} then {} and {}\n\nsee {mentioned}.png and staged-media://{bare}", + embed(&only_one), + embed(&in_two), + embed(¬e_and_chat), + ), + ); + let sharer = Note::new_standalone(&branch.id, "Sharer", &embed(&in_two)); + // The conservative rule: a reference inside an example still counts. + let example = Note::new_standalone( + &branch.id, + "Example", + &format!("```md\n{}\n```", embed(&in_example)), + ); + // No media at all, so the pass never reads it. + let plain = Note::new_standalone(&branch.id, "Plain", "nothing embedded"); + // A body that is not valid UTF-8, stored as a BLOB below. + let blob_note = Note::new_standalone(&branch.id, "Blob", "placeholder"); + for note in [&owner, &sharer, &example, &plain, &blob_note] { + store.create_note(note).unwrap(); + } + let project_note = ProjectNote::new(&project.id, "Plan", &embed(&in_project)); + store.create_project_note(&project_note).unwrap(); + + store + .add_session_message_with_images( + &session.id, + MessageRole::User, + "two attachments", + &[sent.clone(), note_and_chat.clone()], + ) + .unwrap(); + // Naming an id in a message's text attaches nothing. + store + .add_session_message(&session.id, MessageRole::User, &mentioned) + .unwrap(); + store + .add_queued_session_message(&session.id, "later", std::slice::from_ref(&queued), None) + .unwrap(); + { + let conn = store.conn.lock().unwrap(); + let mut body = vec![0xff, 0xfe]; + body.extend_from_slice(embed(&in_blob).as_bytes()); + conn.execute( + "UPDATE notes SET content = ?1 WHERE id = ?2", + params![body, blob_note.id], + ) + .unwrap(); + } + + let mut candidates = vec![ + only_one.clone(), + in_two.clone(), + in_project.clone(), + in_example.clone(), + in_blob.clone(), + sent.clone(), + queued.clone(), + note_and_chat.clone(), + unused.clone(), + bare.clone(), + mentioned.clone(), + ]; + // Part of an id that is embedded and attached, and an id one of them is + // part of: the needles end at the id's delimiter, so neither matches. + for whole in [&only_one, &sent, ¬e_and_chat] { + candidates.push(whole[..whole.len() - 1].to_string()); + candidates.push(whole[1..].to_string()); + candidates.push(format!("{whole}0")); + } + let exclusions = [None, Some("no-such-note".to_string())] + .into_iter() + .chain( + [ + &owner.id, + &sharer.id, + &example.id, + &plain.id, + &blob_note.id, + &project_note.id, + ] + .map(|id| Some(id.clone())), + ) + .collect(); + + References { + store, + candidates, + exclusions, + only_one, + in_two, + in_project, + in_example, + in_blob, + sent, + queued, + note_and_chat, + unused, + bare, + mentioned, + owner: owner.id, + blob_note: blob_note.id, + } +} + +/// The batched pass must answer, for every candidate and every note left +/// out, exactly what the per-image checks answer one id at a time. +#[test] +fn test_media_references_match_the_per_image_checks() { + let f = references(); + let ids = || f.candidates.iter().map(String::as_str); + let conn = f.store.conn.lock().unwrap(); + let (mut in_notes, mut in_chat) = (0, 0); + for except in &f.exclusions { + let except = except.as_deref(); + let batched = Store::media_references_on(&conn, ids(), except).unwrap(); + for id in ids() { + let by_notes = Store::referenced_by_notes(&conn, id, except).unwrap(); + let by_chat = Store::referenced_by_chat(&conn, id).unwrap(); + assert_eq!( + batched.in_notes(id), + by_notes, + "notes: {id} leaving out {except:?}" + ); + assert_eq!( + batched.in_chat(id), + by_chat, + "chat: {id} leaving out {except:?}" + ); + assert_eq!(batched.unused(id), !by_notes && !by_chat); + in_notes += usize::from(by_notes); + in_chat += usize::from(by_chat); + + // An id's answer does not depend on what else was asked about. + let alone = Store::media_references_on(&conn, [id], except).unwrap(); + assert_eq!( + (alone.in_notes(id), alone.in_chat(id)), + (by_notes, by_chat), + "alone: {id} leaving out {except:?}" + ); + } + } + // The comparison saw both answers from both halves. + let asked = f.exclusions.len() * f.candidates.len(); + assert!(0 < in_notes && in_notes < asked); + assert!(0 < in_chat && in_chat < asked); +} + +/// The same comparison through the locking wrappers the callers use. +#[test] +fn test_media_reference_wrappers_match_the_per_image_wrappers() { + let f = references(); + let ids = || f.candidates.iter().map(String::as_str); + + let all = f.store.media_references(ids(), None).unwrap(); + let embedded = f.store.media_embedded_in_notes(ids()).unwrap(); + for id in ids() { + let referenced = f.store.media_is_referenced(id).unwrap(); + assert_eq!(all.in_notes(id), referenced, "{id}"); + assert_eq!(embedded.contains(id), referenced, "{id}"); + assert_eq!( + all.in_chat(id), + f.store.image_has_chat_references(id).unwrap(), + "{id}" + ); + } + for note_id in f.exclusions.iter().flatten() { + let except = f.store.media_references(ids(), Some(note_id)).unwrap(); + for id in ids() { + assert_eq!( + except.unused(id), + f.store.media_is_exclusive_to(id, note_id).unwrap(), + "{id} leaving out {note_id}" + ); + } + } +} + +/// What the fixture is meant to exercise, spelled out, so the comparison +/// above cannot pass on a store where nothing matches. +#[test] +fn test_media_references_tell_users_apart() { + let f = references(); + let ids = || f.candidates.iter().map(String::as_str); + let set = |ids: &[&String]| -> HashSet { ids.iter().map(|id| (*id).clone()).collect() }; + let embedded = |references: &super::images::MediaReferences| -> HashSet { + ids() + .filter(|id| references.in_notes(id)) + .map(str::to_owned) + .collect() + }; + let attached = |references: &super::images::MediaReferences| -> HashSet { + ids() + .filter(|id| references.in_chat(id)) + .map(str::to_owned) + .collect() + }; + + let all = f.store.media_references(ids(), None).unwrap(); + assert_eq!( + embedded(&all), + set(&[ + &f.only_one, + &f.in_two, + &f.in_project, + &f.in_example, + &f.in_blob, + &f.note_and_chat, + ]) + ); + assert_eq!(attached(&all), set(&[&f.sent, &f.queued, &f.note_and_chat])); + for id in [&f.unused, &f.bare, &f.mentioned] { + assert!(all.unused(id), "{id}"); + } + + // Leaving the owner out frees what only it embeds; media another note + // shares, or a chat message attaches, is still in use. + let without_owner = f.store.media_references(ids(), Some(&f.owner)).unwrap(); + assert_eq!( + embedded(&without_owner), + set(&[&f.in_two, &f.in_project, &f.in_example, &f.in_blob]) + ); + assert!(without_owner.unused(&f.only_one)); + assert!(!without_owner.unused(&f.in_two)); + assert!(!without_owner.unused(&f.note_and_chat)); + assert_eq!(attached(&without_owner), attached(&all)); + + let without_blob = f.store.media_references(ids(), Some(&f.blob_note)).unwrap(); + assert!(without_blob.unused(&f.in_blob)); +} + +#[test] +fn test_media_references_take_one_pass_however_many_ids_are_asked_about() { + let f = references(); + let ids = || f.candidates.iter().map(String::as_str); + take_reference_passes(); + + f.store.media_references(ids(), Some(&f.owner)).unwrap(); + assert_eq!( + take_reference_passes(), + ReferencePasses { notes: 1, chat: 1 } + ); + + f.store.media_embedded_in_notes(ids()).unwrap(); + assert_eq!( + take_reference_passes(), + ReferencePasses { notes: 1, chat: 0 } + ); + + // Nothing to ask about: no pass, and so nothing it can clear. + let none = std::iter::empty::<&str>; + let unasked = f.store.media_references(none(), None).unwrap(); + assert!(!unasked.unused("any")); + assert!(!unasked.unused(&f.unused)); + assert!(f.store.media_embedded_in_notes(none()).unwrap().is_empty()); + assert_eq!(take_reference_passes(), ReferencePasses::default()); +} + +/// `unused` is what lets a caller delete a file or take a row along, so a +/// pass clears only the ids it was asked about. One that was asked about a +/// narrower set says nothing of the rest, whatever does or does not use them. +#[test] +fn test_media_references_clear_only_the_ids_they_were_asked_about() { + let f = references(); + let ids = || f.candidates.iter().map(String::as_str); + let asked = [f.only_one.as_str(), f.unused.as_str()]; + let passes = [ + ("no note left out", None), + ("the owner left out", Some(f.owner.as_str())), + ]; + + for (label, except) in passes { + let whole = f.store.media_references(ids(), except).unwrap(); + let narrow = f.store.media_references(asked, except).unwrap(); + let conn = f.store.conn.lock().unwrap(); + let on_connection = Store::media_references_on(&conn, asked, except).unwrap(); + drop(conn); + // Both ways of running the pass keep what they were asked about. + assert_eq!(narrow, on_connection, "{label}"); + + let mut cleared = 0; + for id in ids() { + if asked.contains(&id) { + // Asked about: the narrower pass answers as the whole one. + assert_eq!(narrow.unused(id), whole.unused(id), "{label}: {id}"); + } else { + // Not asked about: in use, even where the whole pass, which + // did look, found nothing using it. + assert!(!narrow.unused(id), "{label}: {id}"); + cleared += usize::from(whole.unused(id)); + } + // A pass finds uses only of what it was asked about. + if narrow.in_notes(id) || narrow.in_chat(id) { + assert!(asked.contains(&id), "{label}: {id}"); + } + } + // The refusal was not for want of ids the whole pass does clear. + assert!(cleared > 0, "{label}"); + // An id in no candidate set at all is in use to every pass. + for references in [&whole, &narrow] { + assert!(!references.unused("no-such-id"), "{label}"); + assert!(!references.unused(""), "{label}"); + } + } + + // The image nothing uses is cleared exactly when it is asked about. + let asking = f.store.media_references([f.unused.as_str()], None).unwrap(); + let not_asking = f.store.media_references([f.sent.as_str()], None).unwrap(); + assert!(asking.unused(&f.unused)); + assert!(!not_asking.unused(&f.unused)); + // And the image only the owner embeds, with the owner left out. + let owner = Some(f.owner.as_str()); + let asking = f + .store + .media_references([f.only_one.as_str()], owner) + .unwrap(); + let not_asking = f + .store + .media_references([f.in_two.as_str()], owner) + .unwrap(); + assert!(asking.unused(&f.only_one)); + assert!(!not_asking.unused(&f.only_one)); + + // A set of references that was never read clears nothing. + let unread = super::images::MediaReferences::default(); + assert!(ids().all(|id| !unread.unused(id))); +} + +/// A store holding one of every kind of row the session attachment lookup +/// has to tell apart: linked to the session or not, an attachment's mime type +/// or not, embedded by a branch note, a project note, both or neither. +struct Attachments { + store: Store, + project: String, + session: String, + other_session: String, + /// What the lookup should return for `session`, in the order the rows + /// were inserted. + expected: Vec, + /// What it should return for `other_session`. + other_expected: Vec, + /// What it should return for the pending-session marker. + pending_expected: Vec, +} + +fn attachments() -> Attachments { + let store = Store::in_memory().unwrap(); + let project = Project::new("test/attachments"); + store.create_project(&project).unwrap(); + let branch = Branch::new(&project.id, "feature", "main"); + store.create_branch(&branch).unwrap(); + let queued = Session::new_queued("work"); + store.create_session(&queued).unwrap(); + let other = running_session(&store); + + // Ids are handed out high to low, so the order the rows are inserted in + // is the reverse of the order of their ids. + let next = std::cell::Cell::new(900u32); + let add = |session: Option<&str>, mime: &str| -> String { + let n = next.replace(next.get() - 1); + let mut image = Image::new(Some(&branch.id), &project.id, "file.bin", mime, 1, false); + image.id = format!("bbbbbbbb-0000-4000-8000-{n:012}"); + image.session_id = session.map(str::to_owned); + store.create_image(&image).unwrap(); + image.id + }; + let mine = Some(queued.id.as_str()); + let theirs = Some(other.id.as_str()); + let embed = |id: &str| format!("![shot](staged-media://{id}.png)"); + + // Unreferenced attachments: returned. + let plain = add(mine, "image/png"); + let pdf = add(mine, "application/pdf"); + // `LIKE` ignores ASCII case; `=` does not. + let upper_image = add(mine, "IMAGE/JPEG"); + add(mine, "application/PDF"); + // Referenced by a branch note, a project note, or both: dropped. + let in_note = add(mine, "image/jpeg"); + let in_project = add(mine, "image/png"); + let in_both = add(mine, "image/webp"); + let pdf_in_note = add(mine, "application/pdf"); + // The conservative rule: a reference inside an example still counts, and + // so does one in a body that is not valid UTF-8. + let in_example = add(mine, "image/png"); + let in_blob = add(mine, "image/png"); + // Named in a note without the scheme, or with no extension: not embedded. + let mentioned = add(mine, "image/png"); + let bare = add(mine, "image/png"); + // An id that an embedded id is part of, or is part of an embedded id. + let longer = format!("{in_note}0"); + let shorter = in_note[1..].to_string(); + for id in [&longer, &shorter] { + let mut image = Image::new(None, &project.id, "file.bin", "image/png", 1, false); + image.id = id.clone(); + image.session_id = Some(queued.id.clone()); + store.create_image(&image).unwrap(); + } + // Not an attachment's mime type, embedded or not: never returned. + add(mine, "video/mp4"); + let video_in_note = add(mine, "video/quicktime"); + add(mine, "text/plain"); + add(mine, "image"); + // Linked to another session, to none, or still pending. + let theirs_plain = add(theirs, "image/png"); + let theirs_in_note = add(theirs, "image/png"); + add(theirs, "video/mp4"); + add(None, "image/png"); + let pending = add(Some(PENDING_SESSION_ID), "image/png"); + add(Some(NOTE_MEDIA_SESSION_ID), "video/mp4"); + + let notes = [ + Note::new_standalone( + &branch.id, + "Owner", + &format!( + "{} {} {} {} {}\n\nsee {mentioned}.png and staged-media://{bare}", + embed(&in_note), + embed(&in_both), + embed(&pdf_in_note), + embed(&video_in_note), + embed(&theirs_in_note), + ), + ), + Note::new_standalone( + &branch.id, + "Example", + &format!("```md\n{}\n```", embed(&in_example)), + ), + Note::new_standalone(&branch.id, "Plain", "nothing embedded"), + ]; + for note in ¬es { + store.create_note(note).unwrap(); + } + let blob_note = Note::new_standalone(&branch.id, "Blob", "placeholder"); + store.create_note(&blob_note).unwrap(); + { + let conn = store.conn.lock().unwrap(); + let mut body = vec![0xff, 0xfe]; + body.extend_from_slice(embed(&in_blob).as_bytes()); + conn.execute( + "UPDATE notes SET content = ?1 WHERE id = ?2", + params![body, blob_note.id], + ) + .unwrap(); + } + let project_note = ProjectNote::new( + &project.id, + "Plan", + &format!("{} {}", embed(&in_project), embed(&in_both)), + ); + store.create_project_note(&project_note).unwrap(); + + // A chat message attaching a row is what makes it an attachment, and is + // no reason to leave it out: the lookup asks the notes, never the chat. + store + .add_session_message_with_images( + &queued.id, + MessageRole::User, + "three attachments", + &[plain.clone(), pdf.clone(), in_note.clone()], + ) + .unwrap(); + + Attachments { + store, + project: project.id, + session: queued.id, + other_session: other.id, + expected: vec![plain, pdf, upper_image, mentioned, bare, longer, shorter], + other_expected: vec![theirs_plain], + pending_expected: vec![pending], + } +} + +/// The lookup must return, for every session, exactly what its one-statement +/// form returned, in the same order. +#[test] +fn test_session_attachments_match_the_per_image_query() { + let f = attachments(); + for session_id in [ + f.session.as_str(), + f.other_session.as_str(), + PENDING_SESSION_ID, + NOTE_MEDIA_SESSION_ID, + "no-such-session", + "", + ] { + assert_eq!( + f.store.get_image_ids_for_session(session_id).unwrap(), + f.store + .get_image_ids_for_session_per_image(session_id) + .unwrap(), + "session {session_id:?}" + ); + } + + // What the fixture is meant to exercise, spelled out, so the comparison + // cannot pass on a store where both forms return nothing. + let lookup = |session_id: &str| f.store.get_image_ids_for_session(session_id).unwrap(); + assert_eq!(lookup(&f.session), f.expected); + assert_eq!(lookup(&f.other_session), f.other_expected); + assert_eq!(lookup(PENDING_SESSION_ID), f.pending_expected); + assert!(lookup(NOTE_MEDIA_SESSION_ID).is_empty()); + assert!(lookup("no-such-session").is_empty()); +} + +/// The answer follows the notes: an attachment a note starts to embed drops +/// out, and comes back when the note stops, in both forms alike. +#[test] +fn test_session_attachments_follow_the_notes_like_the_per_image_query() { + let f = attachments(); + let both = |store: &Store| { + let ids = store.get_image_ids_for_session(&f.session).unwrap(); + assert_eq!( + ids, + store + .get_image_ids_for_session_per_image(&f.session) + .unwrap() + ); + ids + }; + let embedded = f.expected[0].clone(); + let note = ProjectNote::new( + &f.project, + "Later", + &format!("![later](staged-media://{embedded}.png)"), + ); + f.store.create_project_note(¬e).unwrap(); + assert_eq!(both(&f.store), f.expected[1..]); + + f.store.delete_project_note(¬e.id).unwrap(); + assert_eq!(both(&f.store), f.expected); +} + +/// One pass over the note bodies for the whole set, none over the chat +/// attachment lists, and no pass at all for a session with no attachment. +#[test] +fn test_session_attachments_take_one_note_pass_however_many_are_attached() { + let f = attachments(); + take_reference_passes(); + + assert_eq!( + f.store.get_image_ids_for_session(&f.session).unwrap().len(), + f.expected.len() + ); + assert_eq!( + take_reference_passes(), + ReferencePasses { notes: 1, chat: 0 } + ); + + // Only a video is linked to the note-media marker, and nothing to an + // unknown session: no attachment, so nothing to ask the notes about. + for session_id in [NOTE_MEDIA_SESSION_ID, "no-such-session"] { + assert!(f + .store + .get_image_ids_for_session(session_id) + .unwrap() + .is_empty()); + } + assert_eq!(take_reference_passes(), ReferencePasses::default()); +} diff --git a/apps/staged/src-tauri/src/store/migration_tests.rs b/apps/staged/src-tauri/src/store/migration_tests.rs index 0ddc8bc5b..21554c491 100644 --- a/apps/staged/src-tauri/src/store/migration_tests.rs +++ b/apps/staged/src-tauri/src/store/migration_tests.rs @@ -67,6 +67,19 @@ fn add_trigger_tables(conn: &Connection) { .unwrap(); } +/// Give a minimal old schema the `session_messages.image_ids` column that +/// migration 0049 indexes, whether or not the test built the table. +fn add_message_image_ids(conn: &Connection) { + conn.execute_batch( + "CREATE TABLE IF NOT EXISTS session_messages (id INTEGER PRIMARY KEY AUTOINCREMENT);", + ) + .unwrap(); + if !column_exists(conn, "session_messages", "image_ids") { + conn.execute_batch("ALTER TABLE session_messages ADD COLUMN image_ids TEXT DEFAULT NULL;") + .unwrap(); + } +} + #[test] fn test_migration_registry_is_valid() { migrations::validate().unwrap(); @@ -175,7 +188,7 @@ fn test_store_bootstraps_fresh_database_with_baseline_migration() { ) .unwrap(); - assert_eq!(version, 48); + assert_eq!(version, 49); assert_eq!(app_version, super::APP_VERSION); assert!(table_exists(&conn, "projects")); assert!(table_exists(&conn, "project_notes")); @@ -231,6 +244,9 @@ fn test_store_bootstraps_fresh_database_with_baseline_migration() { assert!(column_exists(&conn, "sessions", "provider_metadata")); assert!(column_exists(&conn, "sessions", "rate_limit_resets_at")); assert!(column_exists(&conn, "sessions", "rate_limit_kind")); + // 0049: the chat image-reference check reads this instead of the table. + assert!(index_exists(&conn, "idx_session_messages_image_ids")); + super::images_tests::assert_chat_reference_check_reads_the_index(&conn); let trigger_count: i64 = conn .query_row( @@ -333,7 +349,7 @@ fn test_store_repairs_github_comment_tracking_user_version() { let version: i64 = conn .query_row("PRAGMA user_version", [], |row| row.get(0)) .unwrap(); - assert_eq!(version, 48); + assert_eq!(version, 49); assert!(column_exists(&conn, "sessions", "pipeline")); assert!(column_exists(&conn, "sessions", "acp_config_selection")); assert!(column_exists(&conn, "sessions", "acp_title")); @@ -433,7 +449,7 @@ fn test_store_repairs_pipeline_user_version() { let version: i64 = conn .query_row("PRAGMA user_version", [], |row| row.get(0)) .unwrap(); - assert_eq!(version, 48); + assert_eq!(version, 49); assert!(column_exists(&conn, "sessions", "provider_metadata")); assert!(column_exists(&conn, "sessions", "rate_limit_resets_at")); assert!(column_exists(&conn, "sessions", "rate_limit_kind")); @@ -519,6 +535,7 @@ fn test_completion_effects_migration_backfills_finished_pipeline_sessions() { ) .unwrap(); add_trigger_tables(&conn); + add_message_image_ids(&conn); drop(conn); let store = Store::new(&path).unwrap(); @@ -528,7 +545,7 @@ fn test_completion_effects_migration_backfills_finished_pipeline_sessions() { let version: i64 = conn .query_row("PRAGMA user_version", [], |row| row.get(0)) .unwrap(); - assert_eq!(version, 48); + assert_eq!(version, 49); assert!(column_exists(&conn, "sessions", "completion_effects_at")); let marker = |id: &str| -> Option { @@ -602,6 +619,7 @@ fn test_auto_review_removal_migration_deletes_auto_reviews_and_drops_flag() { ) .unwrap(); add_trigger_tables(&conn); + add_message_image_ids(&conn); drop(conn); let store = Store::new(&path).unwrap(); @@ -611,7 +629,7 @@ fn test_auto_review_removal_migration_deletes_auto_reviews_and_drops_flag() { let version: i64 = conn .query_row("PRAGMA user_version", [], |row| row.get(0)) .unwrap(); - assert_eq!(version, 48); + assert_eq!(version, 49); assert!(!column_exists(&conn, "reviews", "is_auto")); // Reviews the removed auto-review feature created in the background are @@ -683,6 +701,7 @@ fn test_detecting_pid_migration_clears_orphaned_detection_flags() { ) .unwrap(); add_trigger_tables(&conn); + add_message_image_ids(&conn); drop(conn); let store = Store::new(&path).unwrap(); @@ -692,7 +711,7 @@ fn test_detecting_pid_migration_clears_orphaned_detection_flags() { let version: i64 = conn .query_row("PRAGMA user_version", [], |row| row.get(0)) .unwrap(); - assert_eq!(version, 48); + assert_eq!(version, 49); assert!(column_exists(&conn, "action_contexts", "detecting_pid")); // No shipped build ever cleared the flag from outside the process that set @@ -768,6 +787,7 @@ fn test_note_migrations_backfill_subtype_and_completion_for_session_less_notes() ) .unwrap(); add_trigger_tables(&conn); + add_message_image_ids(&conn); drop(conn); let store = Store::new(&path).unwrap(); @@ -777,7 +797,7 @@ fn test_note_migrations_backfill_subtype_and_completion_for_session_less_notes() let version: i64 = conn .query_row("PRAGMA user_version", [], |row| row.get(0)) .unwrap(); - assert_eq!(version, 48); + assert_eq!(version, 49); assert!(column_exists(&conn, "notes", "subtype")); let subtype = |id: &str| -> Option { @@ -874,6 +894,7 @@ fn test_pinned_actions_migration_pins_each_contexts_first_run_action() { ) .unwrap(); add_trigger_tables(&conn); + add_message_image_ids(&conn); drop(conn); let store = Store::new(&path).unwrap(); @@ -883,7 +904,7 @@ fn test_pinned_actions_migration_pins_each_contexts_first_run_action() { let version: i64 = conn .query_row("PRAGMA user_version", [], |row| row.get(0)) .unwrap(); - assert_eq!(version, 48); + assert_eq!(version, 49); assert!(column_exists(&conn, "repo_actions", "pinned")); assert!(column_exists(&conn, "repo_actions", "icon")); @@ -952,6 +973,7 @@ fn test_auto_commit_removal_migration_drops_flag_and_keeps_actions() { ) .unwrap(); add_trigger_tables(&conn); + add_message_image_ids(&conn); drop(conn); let store = Store::new(&path).unwrap(); @@ -961,7 +983,7 @@ fn test_auto_commit_removal_migration_drops_flag_and_keeps_actions() { let version: i64 = conn .query_row("PRAGMA user_version", [], |row| row.get(0)) .unwrap(); - assert_eq!(version, 48); + assert_eq!(version, 49); assert!(!column_exists(&conn, "repo_actions", "auto_commit")); // Only the flag goes: an action that had auto-commit enabled keeps running @@ -1016,6 +1038,7 @@ fn test_session_branch_id_backfill_links_artifact_backed_sessions() { ) .unwrap(); add_trigger_tables(&conn); + add_message_image_ids(&conn); drop(conn); let store = Store::new(&path).unwrap(); @@ -1025,7 +1048,7 @@ fn test_session_branch_id_backfill_links_artifact_backed_sessions() { let version: i64 = conn .query_row("PRAGMA user_version", [], |row| row.get(0)) .unwrap(); - assert_eq!(version, 48); + assert_eq!(version, 49); let branch_of = |id: &str| -> Option { conn.query_row( @@ -1072,6 +1095,7 @@ fn test_session_chat_read_at_migration_marks_existing_chats_read() { ) .unwrap(); add_trigger_tables(&conn); + add_message_image_ids(&conn); drop(conn); let migrated_from = super::now_timestamp() / 1000 * 1000; @@ -1083,7 +1107,7 @@ fn test_session_chat_read_at_migration_marks_existing_chats_read() { let version: i64 = conn .query_row("PRAGMA user_version", [], |row| row.get(0)) .unwrap(); - assert_eq!(version, 48); + assert_eq!(version, 49); assert!(column_exists(&conn, "sessions", "chat_read_at")); let read_at = |id: &str| -> Option { @@ -1133,6 +1157,7 @@ fn test_pr_title_migration_adds_an_empty_title_to_existing_branches() { ) .unwrap(); add_trigger_tables(&conn); + add_message_image_ids(&conn); drop(conn); let store = Store::new(&path).unwrap(); @@ -1142,7 +1167,7 @@ fn test_pr_title_migration_adds_an_empty_title_to_existing_branches() { let version: i64 = conn .query_row("PRAGMA user_version", [], |row| row.get(0)) .unwrap(); - assert_eq!(version, 48); + assert_eq!(version, 49); assert!(column_exists(&conn, "branches", "pr_title")); // No backfill: a linked PR's title arrives with the next status poll. @@ -1183,6 +1208,7 @@ fn test_note_read_at_migration_marks_landed_notes_read() { ) .unwrap(); add_trigger_tables(&conn); + add_message_image_ids(&conn); drop(conn); let store = Store::new(&path).unwrap(); @@ -1192,7 +1218,7 @@ fn test_note_read_at_migration_marks_landed_notes_read() { let version: i64 = conn .query_row("PRAGMA user_version", [], |row| row.get(0)) .unwrap(); - assert_eq!(version, 48); + assert_eq!(version, 49); for (table, landed_at) in [("notes", 1000), ("project_notes", 2000)] { assert!(column_exists(&conn, table, "read_at")); @@ -1248,6 +1274,7 @@ fn test_child_note_repair_detaches_only_cross_project_parents() { ) .unwrap(); add_trigger_tables(&conn); + add_message_image_ids(&conn); drop(conn); let store = Store::new(&path).unwrap(); @@ -1257,7 +1284,7 @@ fn test_child_note_repair_detaches_only_cross_project_parents() { let version: i64 = conn .query_row("PRAGMA user_version", [], |row| row.get(0)) .unwrap(); - assert_eq!(version, 48); + assert_eq!(version, 49); let parent_of = |id: &str| -> Option { conn.query_row( @@ -1303,6 +1330,7 @@ fn test_session_pre_head_and_blox_command_migration_leaves_existing_rows_null() ) .unwrap(); add_trigger_tables(&conn); + add_message_image_ids(&conn); drop(conn); let store = Store::new(&path).unwrap(); @@ -1312,7 +1340,7 @@ fn test_session_pre_head_and_blox_command_migration_leaves_existing_rows_null() let version: i64 = conn .query_row("PRAGMA user_version", [], |row| row.get(0)) .unwrap(); - assert_eq!(version, 48); + assert_eq!(version, 49); let (pre_head, command): (Option, Option) = conn .query_row( "SELECT pre_head_sha, blox_command_id FROM sessions WHERE id = 'old'", @@ -1345,6 +1373,7 @@ fn test_session_started_by_migration_leaves_existing_rows_null() { ", ) .unwrap(); + add_message_image_ids(&conn); drop(conn); let store = Store::new(&path).unwrap(); @@ -1354,7 +1383,7 @@ fn test_session_started_by_migration_leaves_existing_rows_null() { let version: i64 = conn .query_row("PRAGMA user_version", [], |row| row.get(0)) .unwrap(); - assert_eq!(version, 48); + assert_eq!(version, 49); let started_by: Option = conn .query_row( "SELECT started_by FROM sessions WHERE id = 'old'", @@ -1405,6 +1434,7 @@ fn test_artifact_delete_cleanup_migration_spares_disconnected_sessions() { ", ) .unwrap(); + add_message_image_ids(&conn); drop(conn); let store = Store::new(&path).unwrap(); @@ -1414,7 +1444,7 @@ fn test_artifact_delete_cleanup_migration_spares_disconnected_sessions() { let version: i64 = conn .query_row("PRAGMA user_version", [], |row| row.get(0)) .unwrap(); - assert_eq!(version, 48); + assert_eq!(version, 49); for (table, prefix) in [ ("commits", "c"), ("notes", "n"), @@ -1468,6 +1498,7 @@ fn test_control_grants_migration_creates_the_table_and_cascades_on_project() { ", ) .unwrap(); + add_message_image_ids(&conn); drop(conn); let store = Store::new(&path).unwrap(); @@ -1478,7 +1509,7 @@ fn test_control_grants_migration_creates_the_table_and_cascades_on_project() { let version: i64 = conn .query_row("PRAGMA user_version", [], |row| row.get(0)) .unwrap(); - assert_eq!(version, 48); + assert_eq!(version, 49); assert!(table_exists(&conn, "control_grants")); for column in [ "id", @@ -1541,6 +1572,7 @@ fn test_branch_deferred_push_migration_leaves_existing_branches_unarmed() { ", ) .unwrap(); + add_message_image_ids(&conn); drop(conn); let store = Store::new(&path).unwrap(); @@ -1550,7 +1582,7 @@ fn test_branch_deferred_push_migration_leaves_existing_branches_unarmed() { let version: i64 = conn .query_row("PRAGMA user_version", [], |row| row.get(0)) .unwrap(); - assert_eq!(version, 48); + assert_eq!(version, 49); assert!(column_exists(&conn, "branches", "deferred_push")); assert!(column_exists( &conn, @@ -1632,6 +1664,7 @@ fn test_project_tags_migration_then_import_moves_status_overrides_into_tags() { ", ) .unwrap(); + add_message_image_ids(&conn); drop(conn); let store = Store::new(&path).unwrap(); @@ -1667,7 +1700,7 @@ fn test_project_tags_migration_then_import_moves_status_overrides_into_tags() { let version: i64 = conn .query_row("PRAGMA user_version", [], |row| row.get(0)) .unwrap(); - assert_eq!(version, 48); + assert_eq!(version, 49); assert!(!table_exists(&conn, "legacy_project_status_import")); let group: (String, String, i64, i64) = conn @@ -1831,6 +1864,7 @@ fn test_tag_group_sidebar_migration_adds_the_column_off_for_existing_groups() { ", ) .unwrap(); + add_message_image_ids(&conn); drop(conn); let store = Store::new(&path).unwrap(); @@ -1847,7 +1881,7 @@ fn test_tag_group_sidebar_migration_adds_the_column_off_for_existing_groups() { let version: i64 = conn .query_row("PRAGMA user_version", [], |row| row.get(0)) .unwrap(); - assert_eq!(version, 48); + assert_eq!(version, 49); assert!(column_exists(&conn, "tag_groups", "show_in_sidebar")); let shown: i64 = conn .query_row( @@ -1864,3 +1898,109 @@ fn test_tag_group_sidebar_migration_adds_the_column_off_for_existing_groups() { cleanup_db(&path); } + +#[test] +fn test_message_image_ids_index_migration_indexes_existing_attachments() { + let path = temp_db_path("message-image-ids-index"); + let conn = Connection::open(&path).unwrap(); + conn.execute_batch( + " + PRAGMA user_version = 48; + CREATE TABLE app_metadata ( + id INTEGER PRIMARY KEY CHECK (id = 1), + app_version TEXT NOT NULL + ); + INSERT INTO app_metadata (id, app_version) VALUES (1, '0.2.9'); + -- Only the two tables the chat reference check reads, with the + -- baseline columns of `session_messages` and the one queued column it + -- searches. + CREATE TABLE session_messages ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + session_id TEXT NOT NULL, + role TEXT NOT NULL, + content TEXT NOT NULL, + created_at INTEGER NOT NULL, + image_ids TEXT DEFAULT NULL + ); + CREATE TABLE queued_session_messages (id TEXT PRIMARY KEY, image_ids TEXT); + -- The batched check reads the note bodies in the same pass. + CREATE TABLE notes (id TEXT PRIMARY KEY, content TEXT NOT NULL); + CREATE TABLE project_notes (id TEXT PRIMARY KEY, content TEXT NOT NULL); + INSERT INTO session_messages (session_id, role, content, created_at, image_ids) VALUES + ('s', 'user', 'no attachments', 1, NULL), + ('s', 'user', 'two attachments', 2, '[\"img-sent-1\",\"img-sent-2\"]'), + ('s', 'assistant', 'a reply', 3, NULL); + INSERT INTO queued_session_messages (id, image_ids) VALUES + ('q-1', NULL), + ('q-2', '[\"img-queued\"]'); + ", + ) + .unwrap(); + drop(conn); + + // The messages written before the upgrade answer the check afterwards. + let store = Store::new(&path).unwrap(); + assert!(store.image_has_chat_references("img-sent-1").unwrap()); + assert!(store.image_has_chat_references("img-sent-2").unwrap()); + assert!(store.image_has_chat_references("img-queued").unwrap()); + assert!(!store.image_has_chat_references("img-sent").unwrap()); + assert!(!store.image_has_chat_references("img-unused").unwrap()); + // And the batched pass, which is what the app runs, answers the same. + let ids = [ + "img-sent-1", + "img-sent-2", + "img-queued", + "img-sent", + "img-unused", + ]; + let references = store.media_references(ids, None).unwrap(); + for id in ids { + assert_eq!( + references.in_chat(id), + store.image_has_chat_references(id).unwrap(), + "{id}" + ); + } + drop(store); + + let conn = Connection::open(&path).unwrap(); + let version: i64 = conn + .query_row("PRAGMA user_version", [], |row| row.get(0)) + .unwrap(); + assert_eq!(version, 49); + assert!(index_exists(&conn, "idx_session_messages_image_ids")); + + // Partial: messages without attachments stay out of the index. + let index_sql: String = conn + .query_row( + "SELECT sql FROM sqlite_master + WHERE type = 'index' AND name = 'idx_session_messages_image_ids'", + [], + |row| row.get(0), + ) + .unwrap(); + assert!( + index_sql.contains("WHERE image_ids IS NOT NULL"), + "expected a partial index, got: {index_sql}" + ); + let indexed: i64 = conn + .query_row( + "SELECT COUNT(*) FROM session_messages INDEXED BY idx_session_messages_image_ids + WHERE image_ids IS NOT NULL", + [], + |row| row.get(0), + ) + .unwrap(); + assert_eq!(indexed, 1); + let messages: i64 = conn + .query_row("SELECT COUNT(*) FROM session_messages", [], |row| { + row.get(0) + }) + .unwrap(); + assert_eq!(messages, 3); + + super::images_tests::assert_chat_reference_check_reads_the_index(&conn); + drop(conn); + + cleanup_db(&path); +} diff --git a/apps/staged/src-tauri/src/store/migrations/0049-index-session-message-image-ids/up.sql b/apps/staged/src-tauri/src/store/migrations/0049-index-session-message-image-ids/up.sql new file mode 100644 index 000000000..5f7badc01 --- /dev/null +++ b/apps/staged/src-tauri/src/store/migrations/0049-index-session-message-image-ids/up.sql @@ -0,0 +1,37 @@ +-- Deleting or moving a note that embeds images asks whether any chat message +-- attaches one of them. The answer is in `session_messages.image_ids`, and +-- nothing indexed that column, so the question read all of `session_messages`, +-- the largest table in the store, while holding the one store connection, and +-- the window froze for seconds. +-- +-- Messages without attachments store NULL (`image_ids_json` in +-- store/messages.rs), so a partial index over the non-NULL values holds only +-- the messages that attach something. The app asks once per reference pass, +-- for all of that pass's images together, not once per image. A delete makes +-- one pass and a move can make several. Each pass is one call of +-- `Store::attached_in_chat` in store/images.rs, which runs +-- `CHAT_ATTACHMENTS_SQL`, +-- `SELECT image_ids FROM session_messages WHERE image_ids IS NOT NULL`, and +-- matches the ids itself. That WHERE is this index's, so the statement reads +-- the index as a covering index and no row of the table. +-- +-- A statement has to spell out `image_ids IS NOT NULL` to be given the index. +-- SQLite does not infer it from a term such as `instr(image_ids, ?) > 0`, +-- which is why the per-image form the tests keep (`CHAT_REFERENCE_SQL`, +-- `Store::referenced_by_chat`) carries the term as well. +-- +-- `queued_session_messages` is left alone: it holds one row per queued +-- follow-up, not one per message. +-- +-- Building the index reads every row of `session_messages` once, when the +-- store is opened during launch, the first time a build with this migration +-- runs. The build itself has not been timed. The code review of this change +-- measured, read-only, one pass over `session_messages` reading `image_ids` on +-- the 4.7 GB store the freeze was reported on: 2,201,194 rows, 9 of them with +-- attachments, 12.4 s the first time and 0.66 s when repeated, at a load +-- average of about 21. The build walks the same pages, so on a store that size +-- expect a one-off wait of up to about 12 s at launch, longer with nothing +-- cached, as after a reboot. The index is small: there it holds 9 entries. + +CREATE INDEX idx_session_messages_image_ids + ON session_messages(image_ids) WHERE image_ids IS NOT NULL; diff --git a/apps/staged/src-tauri/src/store/mod.rs b/apps/staged/src-tauri/src/store/mod.rs index 0f59cdb5e..87a920ca2 100644 --- a/apps/staged/src-tauri/src/store/mod.rs +++ b/apps/staged/src-tauri/src/store/mod.rs @@ -44,6 +44,8 @@ mod tool_bodies; mod tool_images; mod workdirs; +#[cfg(test)] +mod images_tests; #[cfg(test)] mod migration_tests; #[cfg(test)] diff --git a/apps/staged/src-tauri/src/store/note_transfer.rs b/apps/staged/src-tauri/src/store/note_transfer.rs index d6079f238..b3839d880 100644 --- a/apps/staged/src-tauri/src/store/note_transfer.rs +++ b/apps/staged/src-tauri/src/store/note_transfer.rs @@ -446,7 +446,12 @@ impl Store { /// /// A row some other note or a chat message has started using since the /// caller checked, or one deleted meanwhile, refuses the move rather than - /// being taken from under that user. + /// being taken from under that user. What uses the rows is read once, for + /// all of them, before the first is written: the transaction is the + /// snapshot, and nothing below adds a reference. Collecting a displaced + /// session can only remove one, with that session's messages; a row those + /// messages attached still refuses the move, as it did whenever the row + /// came before the session's in `rows`. /// /// The row takes the moved note's session, as a copy would. Ingest already /// stores a note's media under its session, so that rarely changes @@ -459,6 +464,11 @@ impl Store { title: &str, rows: &[Image], ) -> Result<(), StoreError> { + if rows.is_empty() { + return Ok(()); + } + let references = + Self::media_references_on(tx, rows.iter().map(|row| row.id.as_str()), Some(note_id))?; for row in rows { let displaced: Option> = tx .query_row( @@ -470,7 +480,7 @@ impl Store { let Some(displaced) = displaced else { return Err(media_changed_during_transfer(title)); }; - if !Self::media_exclusive_to(tx, &row.id, note_id)? { + if !references.unused(&row.id) { return Err(media_changed_during_transfer(title)); } tx.execute( @@ -1536,6 +1546,136 @@ mod tests { } } + /// The check under the transaction reads what uses the rows once, for + /// all of them, and a move with nothing to re-home reads nothing. + #[test] + fn move_confirms_every_rehomed_row_in_one_pass() { + use crate::store::images::{take_reference_passes, ReferencePasses}; + + let f = fixture(); + let session = completed_session(&f); + let images: Vec = (0..3).map(|_| source_media(&f, &session.id)).collect(); + let content = images + .iter() + .map(|image| format!("![a](staged-media://{}.png)", image.id)) + .collect::>() + .join("\n"); + let note = + Note::new_standalone(&f.source_branch.id, "Shots", &content).with_session(&session.id); + f.store.create_note(¬e).unwrap(); + let rehomed: Vec = images + .iter() + .enumerate() + .map(|(n, image)| rehomed_onto(&f, image, &format!("shot {n}.png"), &session.id)) + .collect(); + + take_reference_passes(); + f.store + .move_note_to_branch( + ¬e.id, + &f.source_branch.id, + &f.target_branch.id, + &content, + &[], + &rehomed, + ) + .unwrap(); + assert_eq!( + take_reference_passes(), + ReferencePasses { notes: 1, chat: 1 } + ); + for image in &images { + let row = f.store.get_image(&image.id).unwrap().unwrap(); + assert_eq!(row.project_id, f.target.id); + } + + f.store + .move_note_to_branch( + ¬e.id, + &f.target_branch.id, + &f.source_branch.id, + &content, + &[], + &[], + ) + .unwrap(); + assert_eq!(take_reference_passes(), ReferencePasses::default()); + } + + /// The one read covers every row: whichever of several gained another + /// user, the move is refused and none of them is written. + #[test] + fn move_is_refused_when_any_of_several_rehomed_rows_gained_another_user() { + for (taken, other_user) in [(0, "note"), (2, "note"), (0, "chat"), (2, "chat")] { + let f = fixture(); + let session = completed_session(&f); + let images: Vec = (0..3).map(|_| source_media(&f, &session.id)).collect(); + let embed = |image: &Image| format!("![a](staged-media://{}.png)", image.id); + let content = images.iter().map(embed).collect::>().join("\n"); + let note = Note::new_standalone(&f.source_branch.id, "Shots", &content) + .with_session(&session.id); + f.store.create_note(¬e).unwrap(); + match other_user { + "note" => f + .store + .create_note(&Note::new_standalone( + &f.source_branch.id, + "Other", + &embed(&images[taken]), + )) + .unwrap(), + _ => { + f.store + .add_session_message_with_images( + &session.id, + crate::store::models::MessageRole::User, + "look", + std::slice::from_ref(&images[taken].id), + ) + .unwrap(); + } + } + // What the per-image check says about each row decides the move. + for (n, image) in images.iter().enumerate() { + assert_eq!( + f.store.media_is_exclusive_to(&image.id, ¬e.id).unwrap(), + n != taken, + "{other_user} {taken}: row {n}" + ); + } + let rehomed: Vec = images + .iter() + .enumerate() + .map(|(n, image)| rehomed_onto(&f, image, &format!("shot {n}.png"), &session.id)) + .collect(); + + let err = f + .store + .move_note_to_branch( + ¬e.id, + &f.source_branch.id, + &f.target_branch.id, + &content, + &[], + &rehomed, + ) + .unwrap_err(); + + assert!( + err.to_string().contains("changed during the transfer"), + "{other_user} {taken}: {err}" + ); + for image in &images { + let row = f.store.get_image(&image.id).unwrap().unwrap(); + assert_eq!(row.project_id, f.source.id, "{other_user} {taken}"); + } + assert_eq!( + f.store.get_note(¬e.id).unwrap().unwrap().branch_id, + f.source_branch.id + ); + } + } + #[test] fn project_note_move_rehomes_its_media() { let f = fixture(); diff --git a/apps/staged/src-tauri/src/store/project_notes.rs b/apps/staged/src-tauri/src/store/project_notes.rs index b9162ab38..4e92e6e5b 100644 --- a/apps/staged/src-tauri/src/store/project_notes.rs +++ b/apps/staged/src-tauri/src/store/project_notes.rs @@ -221,7 +221,7 @@ impl Store { /// children fires `trg_cleanup_session_after_note_delete`, which cleans up /// their sessions — but that trigger deliberately skips sessions that are /// still `running`, so the child session ids are returned for the caller to - /// cancel and remove (see `note_commands::delete_project_note`). + /// cancel and remove (see `note_commands::delete_project_note_impl`). pub fn delete_project_note(&self, id: &str) -> Result { let mut conn = self.conn.lock().unwrap(); let tx = conn.transaction()?; diff --git a/apps/staged/src-tauri/src/web_server.rs b/apps/staged/src-tauri/src/web_server.rs index adc4a023e..6b9447c80 100644 --- a/apps/staged/src-tauri/src/web_server.rs +++ b/apps/staged/src-tauri/src/web_server.rs @@ -3004,12 +3004,17 @@ pub(crate) async fn dispatch( let store = get_store(store_mutex)?; let note_id: String = arg(&args, "noteId")?; let delete_session_flag: Option = opt_arg(&args, "deleteSession")?; - crate::note_commands::delete_note_impl( - &store, - app_handle, - ¬e_id, - delete_session_flag.unwrap_or(false), - )?; + let app = app_handle.clone(); + tauri::async_runtime::spawn_blocking(move || { + crate::note_commands::delete_note_impl( + &store, + &app, + ¬e_id, + delete_session_flag.unwrap_or(false), + ) + }) + .await + .map_err(|e| format!("Note delete task failed: {e}"))??; Ok(Value::Null) } "get_note" => { @@ -3102,17 +3107,12 @@ pub(crate) async fn dispatch( "delete_project_note" => { let store = get_store(store_mutex)?; let note_id: String = arg(&args, "noteId")?; - let media_ids = crate::note_media::project_note_media_ids(&store, ¬e_id)?; - let orphaned = store - .delete_project_note(¬e_id) - .map_err(|e| e.to_string())?; - crate::note_media::cleanup_unreferenced(&store, media_ids); - // Mirrors note_commands::delete_project_note: child sessions can - // still be running, so cancel before removing their rows. - for sid in orphaned.all_session_ids() { - session_registry.cancel(&sid); - let _ = store.delete_session(&sid); - } + let registry = Arc::clone(session_registry); + tauri::async_runtime::spawn_blocking(move || { + crate::note_commands::delete_project_note_impl(&store, ®istry, ¬e_id) + }) + .await + .map_err(|e| format!("Project note delete task failed: {e}"))??; Ok(Value::Null) } @@ -4879,6 +4879,10 @@ mod tests { let source = include_str!("web_server.rs"); for (command, body) in [ ("delete_note", "note_commands::delete_note_impl("), + ( + "delete_project_note", + "note_commands::delete_project_note_impl(", + ), ("delete_review", "timeline::delete_review_impl("), ( "delete_pending_commit", @@ -4900,6 +4904,37 @@ mod tests { } } + /// A note delete reads every note body that embeds media and every chat + /// attachment list, and cascades a session's messages, on the one store + /// connection. Inline, that held the tokio worker that also services + /// `/api/events`; the desktop commands moved to the blocking pool for the + /// same scans. The project-note arm also used to carry its own copy of the + /// delete, which is what the second half rules out. + #[test] + fn web_note_delete_arms_run_under_spawn_blocking() { + let source = include_str!("web_server.rs"); + for command in ["delete_note", "delete_project_note"] { + let arm = dispatch_arm_body(source, command); + assert!( + arm.contains("spawn_blocking("), + "the web `{command}` arm must run its body on the blocking pool: {arm}" + ); + } + + let arm = dispatch_arm_body(source, "delete_project_note"); + for inline in [ + ".delete_project_note(", + "cleanup_unreferenced(", + ".delete_session(", + ] { + assert!( + !arm.contains(inline), + "the web `delete_project_note` arm must not delete around the shared \ + body (`{inline}`): {arm}" + ); + } + } + /// The web editor saves through the same body as the desktop one, so the /// two can't drift apart; in particular, neither collects the media an /// edit removed (see `note_commands::update_note_impl`).