From 339707f5a99b774003aeb2ad46ef43e2544b076f Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Fri, 13 Nov 2026 11:56:11 +1100 Subject: [PATCH 1/6] perf(staged): index the chat image-reference check Deleting or moving a note that embeds images froze the window for seconds. Each image is checked for chat references by searching `session_messages.image_ids` for its quoted id, and with nothing indexing that column every check read the whole table, the largest in the store, while holding the one store connection. This is step 1 of the fix in the "Image-reference scans freeze the window" note: make that chat check cheap. The per-operation batching, the off-main-thread deletes and the queued-attachment lookup follow separately. - Add migration 0049 with a partial index, `idx_session_messages_image_ids ON session_messages(image_ids) WHERE image_ids IS NOT NULL`. Messages without attachments store NULL, so it holds only the messages that attach something. - Add `image_ids IS NOT NULL` to both arms of the query in `referenced_by_chat`. It changes no result, but SQLite does not infer the index's WHERE from `instr(...) > 0`, so without it the index is ignored. The SQL moves to a `CHAT_REFERENCE_SQL` const so tests can explain the exact statement the store runs. Tests: - `images_tests.rs` asserts the plan reads `session_messages` only through the covering index, with and without ANALYZE statistics, and that sent and queued attachments still count as references while a partial id or an id named in message text does not. - A migration test upgrades a version 48 database with existing rows and checks the index is partial, holds only the attaching message, and that the plan and the check's answers hold on the upgraded file. The fresh-store test asserts the same index and plan. - The hand-built schemas of the older migration tests gain the `session_messages.image_ids` column the new migration indexes. The plan is `SEARCH session_messages USING COVERING INDEX idx_session_messages_image_ids (image_ids>?)`. With the `IS NOT NULL` term removed the four plan assertions fail on `SCAN session_messages`. Signed-off-by: Matt Toohey Co-Authored-By: Claude Fable 5.2 --- apps/staged/src-tauri/src/store/images.rs | 24 ++- .../src-tauri/src/store/images_tests.rs | 145 ++++++++++++++++ .../src-tauri/src/store/migration_tests.rs | 163 +++++++++++++++--- .../up.sql | 18 ++ apps/staged/src-tauri/src/store/mod.rs | 2 + 5 files changed, 324 insertions(+), 28 deletions(-) create mode 100644 apps/staged/src-tauri/src/store/images_tests.rs create mode 100644 apps/staged/src-tauri/src/store/migrations/0049-index-session-message-image-ids/up.sql diff --git a/apps/staged/src-tauri/src/store/images.rs b/apps/staged/src-tauri/src/store/images.rs index 26eb868b6..8d4c95316 100644 --- a/apps/staged/src-tauri/src/store/images.rs +++ b/apps/staged/src-tauri/src/store/images.rs @@ -22,6 +22,21 @@ const UNREADABLE_DIMENSIONS: ImageDimensions = ImageDimensions { density: 0, }; +/// Whether any sent or queued chat message attaches the image whose quoted id +/// is `?1`. +/// +/// `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` (migration +/// 0049, a partial index over the messages that attach something) instead of +/// every row of `session_messages`: the planner does not infer the index's +/// `WHERE` from `instr(...) > 0`. `images_tests.rs` pins the plan. +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)"; + impl Store { /// Conservative reference check: even a reference inside an example keeps /// its file. No note ownership columns or reconciliation state are needed. @@ -79,13 +94,8 @@ impl Store { 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) } pub fn create_image(&self, image: &Image) -> Result<(), StoreError> { 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..04499ef22 --- /dev/null +++ b/apps/staged/src-tauri/src/store/images_tests.rs @@ -0,0 +1,145 @@ +//! Tests for the image-reference checks in `images.rs`: what counts as a chat +//! reference, and that the check reads the partial index from migration 0049 +//! instead of scanning `session_messages`. + +use std::path::Path; + +use rusqlite::{params, Connection}; + +use super::images::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 the chat +/// reference check, in plan order. +fn chat_reference_plan(conn: &Connection) -> Vec { + let mut stmt = conn + .prepare(&format!("EXPLAIN QUERY PLAN {CHAT_REFERENCE_SQL}")) + .unwrap(); + let details = stmt + .query_map(params!["\"some-image\""], |row| row.get::<_, String>(3)) + .unwrap() + .collect::, _>>() + .unwrap(); + details +} + +/// Assert the chat reference check reads `session_messages` only through the +/// partial index, and as a covering index, so it never touches a table row. +/// +/// 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 plan = chat_reference_plan(conn); + 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:?}" + ); + 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:?}" + ); +} + +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()); +} diff --git a/apps/staged/src-tauri/src/store/migration_tests.rs b/apps/staged/src-tauri/src/store/migration_tests.rs index 0ddc8bc5b..b3f2e26e0 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,90 @@ 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); + 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()); + 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..4f36bb809 --- /dev/null +++ b/apps/staged/src-tauri/src/store/migrations/0049-index-session-message-image-ids/up.sql @@ -0,0 +1,18 @@ +-- The image-reference check (`Store::referenced_by_chat` in store/images.rs) +-- asks whether any chat message attaches a given image by searching +-- `image_ids` for the quoted id. Nothing indexed that column, so every check +-- read all of `session_messages`, the largest table in the store, once per +-- image. Deleting or moving a note with images froze the window 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, and the check reads that instead of the +-- table. The query has to spell out `image_ids IS NOT NULL` itself: SQLite +-- does not infer it from `instr(image_ids, ?) > 0`, and without it the +-- partial index is not considered. +-- +-- `queued_session_messages` is left alone: it holds one row per queued +-- follow-up, not one per message. + +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)] From eb4b63ca81121707dc26f033dab3e42aaa189b97 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Fri, 13 Nov 2026 12:27:40 +1100 Subject: [PATCH 2/6] perf(staged): check image references once per operation Deleting or moving a note asked what else used each of its images one image at a time. Every question was a scan of all note bodies and a read of the chat attachments under its own lock, so an 8-image move scanned the note bodies 24 times and an 8-image delete took the store connection up to 32 times. This is step 2 of the fix in the "Image-reference scans freeze the window" note, on top of the chat index from step 1: one pass per operation instead of one per image. The off-main-thread deletes and the queued-attachment lookup follow separately. No schema change. - Add `Store::media_references`, which takes all candidate ids and an optional excluded note and answers under one lock. It reads the note bodies that embed any media and the attachment list of every chat message that has one, and matches the same two needles in Rust with `str::contains`, so the conservative rule is unchanged. An empty set takes no lock. `media_embedded_in_notes` is the note half alone, for ingest, which never asked about chat. - The chat statement is `SELECT image_ids FROM session_messages WHERE image_ids IS NOT NULL`, the predicate of the step 1 partial index, so it reads that index as a covering index and no table row. - `cleanup_unreferenced` makes one pass for the whole set. A move now collects every note's old media in one call rather than one per note. - `stage_media` makes one pass per note, the first time a row could move as it is. A copy makes none. - `rehome_note_media` makes one pass on the transaction before its loop. The race guard is otherwise as it was and its test is untouched. - Ingest makes at most one note pass, the first time a stored reference the note did not already carry needs it. - The per-image helpers have no caller left outside the tests. They stay, compiled for tests only, as the definition the batched pass is held to. One behaviour differs, inside the race guard. The recheck used to run row by row, after any session an earlier row displaced had been collected along with its messages. It now reads once before the first row is written, so a row attached by a message of such a session refuses the move where row order used to decide it. That needs a chat message to attach the row between planning and the transaction, and a retry copies the row instead. Passes over the note bodies, and over the chat attachments: delete, any number of images 1 branch-note move 3 project-note move 2 per note with media, plus 1 copy 0 ingest of new files or kept refs 0 Tests: - `images_tests.rs` compares the batched pass with the per-image checks for every candidate and every excluded note, covering partial ids, an id named without the scheme, a reference with no extension, a reference inside a code example, a body that is not valid UTF-8, and sent and queued attachments. A second test states the expected sets outright. The plan assertion now covers the batched chat statement too, on a fresh store, with ANALYZE statistics, and on a database upgraded from version 48. - A test-only, per-thread counter records each pass. Tests hold `delete_note_impl`, `cleanup_unreferenced`, a project-note family delete, ingest, `stage_media`, the transaction recheck and whole moves of 1 and 8 images to the counts above, and compare what each keeps or takes along with the per-image helpers' answers. - A new test refuses a move when any of several re-homed rows gained another user, with nothing written. Dropping the trailing dot from the note needle, or re-reading on every image in `stage_media`, fails six of the new tests. Removing the WHERE from the batched chat statement fails three plan assertions on `SCAN session_messages`. Signed-off-by: Matt Toohey Co-Authored-By: Claude Fable 5.2 --- apps/staged/src-tauri/src/note_commands.rs | 51 +++ apps/staged/src-tauri/src/note_media/mod.rs | 93 ++++- apps/staged/src-tauri/src/note_media/tests.rs | 197 +++++++++ apps/staged/src-tauri/src/note_transfer.rs | 246 +++++++++++- apps/staged/src-tauri/src/store/images.rs | 240 ++++++++++- .../src-tauri/src/store/images_tests.rs | 377 ++++++++++++++++-- .../src-tauri/src/store/migration_tests.rs | 19 + .../src-tauri/src/store/note_transfer.rs | 144 ++++++- 8 files changed, 1282 insertions(+), 85 deletions(-) diff --git a/apps/staged/src-tauri/src/note_commands.rs b/apps/staged/src-tauri/src/note_commands.rs index a278336b1..54d125e09 100644 --- a/apps/staged/src-tauri/src/note_commands.rs +++ b/apps/staged/src-tauri/src/note_commands.rs @@ -419,6 +419,57 @@ 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: Vec = (0..8) + .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(); + let body = images.iter().map(embed).collect::>().join("\n\n"); + let mut note = + Note::new_standalone(&f.branch.id, "Shots", &body).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!(f.store.get_image(&image.id).unwrap().is_none()); + assert!( + !image_file_path(&image.project_id, &image.id, &image.filename) + .unwrap() + .exists() + ); + } + } + #[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/store/images.rs b/apps/staged/src-tauri/src/store/images.rs index 8d4c95316..b72380aed 100644 --- a/apps/staged/src-tauri/src/store/images.rs +++ b/apps/staged/src-tauri/src/store/images.rs @@ -22,14 +22,35 @@ 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`. +/// 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` (migration -/// 0049, a partial index over the messages that attach something) instead of +/// 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`. `images_tests.rs` pins the plan. +/// `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 @@ -37,9 +58,192 @@ pub(super) const CHAT_REFERENCE_SQL: &str = "SELECT EXISTS( SELECT 1 FROM queued_session_messages WHERE image_ids IS NOT NULL AND instr(image_ids, ?1) > 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 { + 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. + pub(crate) fn unused(&self, id: &str) -> bool { + !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 { + 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. 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) @@ -52,31 +256,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>, @@ -91,13 +282,18 @@ 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(CHAT_REFERENCE_SQL, params![needle], |row| row.get(0)) .map_err(Into::into) } +} +impl Store { pub fn create_image(&self, image: &Image) -> Result<(), StoreError> { let conn = self.conn.lock().unwrap(); Self::insert_image(&conn, image)?; diff --git a/apps/staged/src-tauri/src/store/images_tests.rs b/apps/staged/src-tauri/src/store/images_tests.rs index 04499ef22..2e56326e4 100644 --- a/apps/staged/src-tauri/src/store/images_tests.rs +++ b/apps/staged/src-tauri/src/store/images_tests.rs @@ -1,54 +1,64 @@ -//! Tests for the image-reference checks in `images.rs`: what counts as a chat -//! reference, and that the check reads the partial index from migration 0049 -//! instead of scanning `session_messages`. +//! 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, and that the chat statements read the partial index from migration +//! 0049 instead of scanning `session_messages`. +use std::collections::HashSet; use std::path::Path; use rusqlite::{params, Connection}; -use super::images::CHAT_REFERENCE_SQL; +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 the chat -/// reference check, in plan order. -fn chat_reference_plan(conn: &Connection) -> Vec { - let mut stmt = conn - .prepare(&format!("EXPLAIN QUERY PLAN {CHAT_REFERENCE_SQL}")) - .unwrap(); +/// 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!["\"some-image\""], |row| row.get::<_, String>(3)) + .query_map(params, |row| row.get::<_, String>(3)) .unwrap() .collect::, _>>() .unwrap(); details } -/// Assert the chat reference check reads `session_messages` only through the -/// partial index, and as a covering index, so it never touches a table row. +/// 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 plan = chat_reference_plan(conn); - 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:?}" - ); - 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:?}" - ); + 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 { @@ -143,3 +153,312 @@ fn test_image_has_chat_references_matches_sent_and_queued_attachments_only() { .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. + let none = std::iter::empty::<&str>; + assert!(f + .store + .media_references(none(), None) + .unwrap() + .unused("any")); + assert!(f.store.media_embedded_in_notes(none()).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 b3f2e26e0..21554c491 100644 --- a/apps/staged/src-tauri/src/store/migration_tests.rs +++ b/apps/staged/src-tauri/src/store/migration_tests.rs @@ -1923,6 +1923,9 @@ fn test_message_image_ids_index_migration_indexes_existing_attachments() { 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\"]'), @@ -1942,6 +1945,22 @@ fn test_message_image_ids_index_migration_indexes_existing_attachments() { 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(); 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(); From 40c8f4b38560546b922ebba3776e4748cb0dc161 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Fri, 13 Nov 2026 12:45:49 +1100 Subject: [PATCH 3/6] perf(staged): take the note deletes off the main thread `delete_note` and `delete_project_note` were synchronous Tauri commands, which run on the main thread. Each holds the one store connection while it checks which of the note's media nothing else uses and while it cascades the sessions it removes, so the window froze for the whole delete. This is step 3 of the fix in the "Image-reference scans freeze the window" note, on top of the chat index from step 1 and the one pass per operation from step 2. The queued-attachment lookup follows separately. No schema change. - Make both commands async and run their bodies on the blocking pool with `tauri::async_runtime::spawn_blocking`, the pattern the timeline and git commands already use. - Wrap the two web arms the same way, so a delete no longer holds the tokio worker that also serves `/api/events`. - Add `delete_project_note_impl` as the one body for a project-note delete. The web arm carried its own copy of the media collection, the store delete and the session clean-up, and now calls the shared body as the `delete_note` arm already did. This does not shorten the hold. A synchronous command that needs the store still waits behind a delete, as it does behind a move. The delete can now also run between two main-thread commands rather than queue with them, which the web arm and the moves always could. The frontend needs no change. `deleteNote` and `deleteProjectNote` in `commands.ts` are hand-written `invoke` wrappers, both callers await them, and the command names and arguments are unchanged. The note also asks for a `note-delete` span on the web arm. That span belongs to perf instrumentation that is not on this branch, so there is nothing to add here. Sharing the body means the arm inherits one when the instrumentation lands. Tests: - `note_commands` runs the shared project-note body on a parent with three children and eight images. It asserts one pass over the note bodies and one over the chat attachments for the whole family, that the images an outside note or a chat message still uses keep their rows and files while the rest lose both, that the running parent and child sessions are cancelled, that no session row of the family is left, and that a repeated delete takes no pass. - A source test asserts both commands are `async` and call their shared body inside `spawn_blocking`. - `web_server` asserts both arms run under `spawn_blocking`, that the project-note arm calls the shared body, and that it no longer deletes around it. Checking the family's media one image at a time fails the first test on 8 passes against 1. Calling the body directly in the `delete_note` command or its web arm fails the other two. Signed-off-by: Matt Toohey Co-Authored-By: Claude Fable 5.2 --- apps/staged/src-tauri/src/note_commands.rs | 263 +++++++++++++++--- .../src-tauri/src/store/project_notes.rs | 2 +- apps/staged/src-tauri/src/web_server.rs | 69 +++-- 3 files changed, 278 insertions(+), 56 deletions(-) diff --git a/apps/staged/src-tauri/src/note_commands.rs b/apps/staged/src-tauri/src/note_commands.rs index 54d125e09..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(), @@ -426,23 +496,9 @@ mod tests { use crate::store::images::{take_reference_passes, ReferencePasses}; let f = Fixture::new(); - let images: Vec = (0..8) - .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(); - let body = images.iter().map(embed).collect::>().join("\n\n"); - let mut note = - Note::new_standalone(&f.branch.id, "Shots", &body).with_subtype(Note::SUBTYPE_WRITTEN); + 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])) @@ -461,11 +517,142 @@ mod tests { assert_kept(&f, &images[0]); for image in &images[1..] { - assert!(f.store.get_image(&image.id).unwrap().is_none()); + 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!( - !image_file_path(&image.project_id, &image.id, &image.filename) - .unwrap() - .exists() + declaration[pool..].contains(body), + "`{command}` must call `{body}` inside `spawn_blocking`: {declaration}" ); } } 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`). From e474e29177d05fbd4a282fcb583218f04ad40a6c Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Fri, 13 Nov 2026 13:15:08 +1100 Subject: [PATCH 4/6] perf(staged): read a queued start's attachments in one note pass Starting a queued session reads the attachments queued with it, and leaves out any a note embeds. `get_image_ids_for_session` did that in one statement with two correlated `NOT EXISTS` scans, so every attached image read every note body and then every project-note body, on a tokio worker, holding the one store connection. This is step 4, the last, of the fix in the "Image-reference scans freeze the window" note, on top of the chat index from step 1, the one pass per operation from step 2 and the off-main-thread deletes from step 3. No schema change: the note's `images.session_id` index is left for the separately reviewed set of `session_id` indexes. - Reduce the statement to the session and mime-type filter, as `SESSION_ATTACHMENTS_SQL`. It still reads the `images` table, one row per stored file. - Drop the ids a note embeds with the note half of the step 2 pass, `embedded_in_notes`, on the same connection and under the same lock. The needle is the same `staged-media://.`, matched as a substring, so the conservative rule is unchanged. The chat half is not asked: a message attaching a row is what makes it an attachment. - The ids come back in the order the statement yields them, as before. - A session with no image or PDF linked to it makes no pass. - The old statement stays, compiled for tests only, as the definition the lookup is held to, beside the per-image helpers step 2 kept. `start_queued_session_for_branch` is the only caller outside the tests and is unchanged. It still makes the call inline on its tokio worker. Passes over the note bodies per queued start, and over the chat attachments: any number of attachments 1 and 0 no attachment 0 and 0 Tests: - `images_tests.rs` compares the lookup with the old statement, ids and order, for six session ids over one fixture: rows a branch note, a project note or both embed, unreferenced images and PDFs, a reference inside a code example, a body that is not valid UTF-8, an id named without the scheme or with no extension, ids that contain or are contained in an embedded id, mime types that differ only in case, videos and other mime types, and rows linked to another session, to none, to the pending marker and to the note-media marker. It then states the expected lists outright. A sent message attaches three of the rows, so a lookup that also asked the chat would fail. - A second test embeds an attachment in a new project note and deletes the note again, and holds both forms to the same answer each time. - A third holds the lookup to one note pass and no chat pass, and to no pass for a session with nothing to return. - `session_commands` queues a session through `insert_queued_branch_session` with 1, 2, 8 and 32 attachments, among notes with media of their own, and holds the read the start makes to one note pass and no chat pass each time, to the attachments no note embeds, in order, and to no pass with nothing attached. The start needs a concrete app runtime to drive, so a source test pins that it makes that read exactly once and no other reference call. Asking the notes once per attachment fails the two pass-count tests. Dropping the mime filter, also asking the chat, or reading only the branch notes each fails the comparison. Read-only against the 4.7 GB store this was reported on, the old and the reduced statement plus the same substring pass, run from Python, agree on ids and order for all 337 sessions with a linked row. Of 1,112 image rows, 1,103 are note media and 9 are returned. Signed-off-by: Matt Toohey Co-Authored-By: Claude Fable 5.2 --- apps/staged/src-tauri/src/session_commands.rs | 115 +++++++++ apps/staged/src-tauri/src/store/images.rs | 54 +++- .../src-tauri/src/store/images_tests.rs | 239 +++++++++++++++++- 3 files changed, 400 insertions(+), 8 deletions(-) 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 b72380aed..0d2cf084f 100644 --- a/apps/staged/src-tauri/src/store/images.rs +++ b/apps/staged/src-tauri/src/store/images.rs @@ -58,6 +58,26 @@ pub(super) const CHAT_REFERENCE_SQL: &str = "SELECT EXISTS( 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`]). @@ -239,7 +259,8 @@ impl Store { } /// The checks one id at a time, as every caller ran them before -/// [`Store::media_references`] answered for a whole set in one pass. Nothing +/// [`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)] @@ -291,6 +312,19 @@ impl Store { 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 { @@ -520,15 +554,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 index 2e56326e4..92d423f93 100644 --- a/apps/staged/src-tauri/src/store/images_tests.rs +++ b/apps/staged/src-tauri/src/store/images_tests.rs @@ -1,7 +1,8 @@ //! 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, and that the chat statements read the partial index from migration -//! 0049 instead of scanning `session_messages`. +//! 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; @@ -462,3 +463,237 @@ fn test_media_references_take_one_pass_however_many_ids_are_asked_about() { assert!(f.store.media_embedded_in_notes(none()).unwrap().is_empty()); assert_eq!(take_reference_passes(), ReferencePasses::default()); } + +/// 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()); +} From ebabbe81eb84349442d34204581f1e2ea3729392 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Fri, 13 Nov 2026 13:35:42 +1100 Subject: [PATCH 5/6] fix(staged): clear only the media ids a reference pass was asked about `MediaReferences::unused` answered true for an id the pass was never asked about. The pass looks only for the ids it is given, so it had found nothing using such an id because it had not looked. That answer is what lets `cleanup_unreferenced` delete a file and lets `stage_media` and `rehome_note_media` take a row along to another scope. This applies the two suggestions from the code review of e474e291, on top of the four steps of the "Image-reference scans freeze the window" fix. No caller is affected today: all three test exactly the ids they asked about. A later caller that narrowed the candidate set and still tested another id would have been told a shared image was free to delete or move. - `MediaReferences` keeps the ids the pass was asked about, and `unused` is true only for one of them that nothing uses. Any other id is reported as in use. A pass with nothing to ask about, which takes no lock and reads nothing, clears nothing. `in_notes` and `in_chat` are unchanged. - The header comment of migration 0049 named `Store::referenced_by_chat` and a check run once per image. Step 2 made that helper test-only. The comment now names what the app runs, `CHAT_ATTACHMENTS_SQL` through `Store::attached_in_chat`, once per operation, and keeps the reason a statement must spell out `image_ids IS NOT NULL`. The migration is unreleased and tracked by `user_version` alone, so a store that has already applied it is unaffected. The statement is unchanged. - The same comment records the one-off cost of building the index, as the review measured it: read-only against the 4.7 GB store the freeze was reported on, one pass over `session_messages` reading `image_ids`, 2,201,194 rows with 9 attaching something, took 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 during launch. The build itself was not timed. Tests: - The assertion that pinned the old answer, `unused("any")` on a pass asked about nothing, now asserts the opposite, also for an image nothing uses. - A new test asks about two of the fixture's ids, with no note left out and with the owner left out, through the locking wrapper and on an open connection. It holds the answers for those two to the answers of a pass over every candidate, and every other candidate to in use, including the ones the whole pass clears. It then states both directions outright for the image nothing uses and for the image only the owner embeds, and checks ids in no candidate set and a default value. Restoring the old body of `unused` fails both tests. Signed-off-by: Matt Toohey Co-Authored-By: Claude Fable 5.2 --- apps/staged/src-tauri/src/store/images.rs | 10 ++- .../src-tauri/src/store/images_tests.rs | 80 +++++++++++++++++-- .../up.sql | 35 +++++--- 3 files changed, 109 insertions(+), 16 deletions(-) diff --git a/apps/staged/src-tauri/src/store/images.rs b/apps/staged/src-tauri/src/store/images.rs index 0d2cf084f..a2726e9aa 100644 --- a/apps/staged/src-tauri/src/store/images.rs +++ b/apps/staged/src-tauri/src/store/images.rs @@ -83,6 +83,8 @@ const SESSION_ATTACHMENTS_PER_IMAGE_SQL: &str = "SELECT id FROM images WHERE ses /// 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, } @@ -101,8 +103,13 @@ impl MediaReferences { /// 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.in_notes(id) && !self.in_chat(id) + self.asked.contains(id) && !self.in_notes(id) && !self.in_chat(id) } } @@ -205,6 +212,7 @@ impl Store { ) -> 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)?, }) diff --git a/apps/staged/src-tauri/src/store/images_tests.rs b/apps/staged/src-tauri/src/store/images_tests.rs index 92d423f93..4e46139dc 100644 --- a/apps/staged/src-tauri/src/store/images_tests.rs +++ b/apps/staged/src-tauri/src/store/images_tests.rs @@ -453,17 +453,85 @@ fn test_media_references_take_one_pass_however_many_ids_are_asked_about() { ReferencePasses { notes: 1, chat: 0 } ); - // Nothing to ask about: no pass. + // Nothing to ask about: no pass, and so nothing it can clear. let none = std::iter::empty::<&str>; - assert!(f - .store - .media_references(none(), None) - .unwrap() - .unused("any")); + 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. 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 index 4f36bb809..070c5038e 100644 --- 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 @@ -1,18 +1,35 @@ --- The image-reference check (`Store::referenced_by_chat` in store/images.rs) --- asks whether any chat message attaches a given image by searching --- `image_ids` for the quoted id. Nothing indexed that column, so every check --- read all of `session_messages`, the largest table in the store, once per --- image. Deleting or moving a note with images froze the window for seconds. +-- 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, and the check reads that instead of the --- table. The query has to spell out `image_ids IS NOT NULL` itself: SQLite --- does not infer it from `instr(image_ids, ?) > 0`, and without it the --- partial index is not considered. +-- the messages that attach something. The app asks once per operation, for +-- all of the operation's images together: `Store::attached_in_chat` in +-- store/images.rs 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; From f82cc2b252be366090259665df1fd94973fa5b1a Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Fri, 13 Nov 2026 13:49:06 +1100 Subject: [PATCH 6/6] docs(staged): say the chat check runs once per pass in migration 0049 The header comment of migration 0049 said the app asks which chat messages attach an image "once per operation, for all of the operation's images together". That holds for a delete but not for a move, which the comment's first sentence also names. A branch-note move runs `CHAT_ATTACHMENTS_SQL` three times: staging, the recheck under the transaction and the collection after the commit. A project-note move runs it twice per note with media, plus once. This applies the one suggestion from the code review of ebabbe81, on top of the four steps of the "Image-reference scans freeze the window" fix. It is worth doing now because the migration is unreleased and the file is append-only once it ships. - The comment now says the app asks once per reference pass, for all of that pass's images together, not once per image. It adds that a delete makes one pass and a move can make several, and that each pass is one call of `Store::attached_in_chat`. - No count for a move is given in the file. The counts depend on the note, and the step 2 commit message and the pass-count tests hold them. Comment-only. The `CREATE INDEX` statement is byte-identical, and the migration is tracked by `user_version` alone, so a store that has already applied it is unaffected. Verification: `git diff` shows one file changed and every added or removed line is a `--` comment line. The file with its comment lines removed is identical to the same view of the previous commit. No test reads the comment text. No cargo build or test run was started. The machine's disk is full, and a comment in a SQL file changes no compiled behaviour. Signed-off-by: Matt Toohey Co-Authored-By: Claude Fable 5.2 --- .../0049-index-session-message-image-ids/up.sql | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) 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 index 070c5038e..5f7badc01 100644 --- 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 @@ -6,9 +6,11 @@ -- -- 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 operation, for --- all of the operation's images together: `Store::attached_in_chat` in --- store/images.rs runs `CHAT_ATTACHMENTS_SQL`, +-- 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.