Repository navigation
perf(staged): stop image-reference scans freezing the window on note delete and move - #1066
Merged
Merged
Conversation
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 <contact@matttoohey.com> Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
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 <contact@matttoohey.com> Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
`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 <contact@matttoohey.com> Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
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://<id>.`, 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 <contact@matttoohey.com> Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
`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 e474e29, 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 <contact@matttoohey.com> Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
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 ebabbe8, 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 <contact@matttoohey.com> Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Deleting or moving a note that embeds images froze the window for seconds. Each image was checked for other users one at a time, every check scanned all note bodies and read the whole
session_messagestable (the largest in the store) while holding the one store connection, and the note deletes ran on the main thread. This fixes the "Image-reference scans freeze the window" report in four steps plus two review follow-ups.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 attaching messages. Statements spell outimage_ids IS NOT NULLbecause SQLite does not infer it frominstr(...) > 0.Store::media_referencestakes all candidate ids and answers under one lock, reading the media-embedding note bodies and the chat attachment list (via the covering index) and matching in Rust with the same needles. A delete makes 1 pass instead of up to 4 per image; a branch-note move makes 3; a copy makes none. The per-image helpers remain test-only as the definition the batched pass is held to.delete_noteanddelete_project_noteare now async and run onspawn_blocking, as are their web arms. The project-note web arm now sharesdelete_project_note_implinstead of carrying its own copy.get_image_ids_for_sessiondrops its two correlatedNOT EXISTSscans per attached image in favour of a session/mime filter plus oneembedded_in_notespass.MediaReferences::unusedis true only for ids the pass was actually asked about (anything else reports as in use), and the migration header comment describes what the app runs, including the measured one-off index build cost (~12 s on a 4.7 GB store with 2.2 M messages).One behavioural difference, inside the move race guard: the recheck now runs once before the first row is written, so a row attached by a chat message of a displaced session refuses the move where row order used to decide it. A retry copies the row instead.
Tests
session_messagesis read only through the covering index, on a fresh store, with ANALYZE statistics, and on a database upgraded from version 48.spawn_blocking, and that the web arms call the shared body.🤖 Generated with Claude Code