Speed up chat message storage appends - #2504
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Bunny Review CompletedTip Review posted. The specimen has left the observation table.
|
🐰 Bunny ReviewBunny Merge Signal: ReadyTip READY
Note Mode: 🧭 Specimen Summary
🔎 Isolated DefectsTip No actionable defects isolated. ✅ Resolved Since Last Review
🧹 Nitpicks
✅ Control Checks
🧪 Observations
🧰 CI Status
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src-tauri/src/commands/storage/message_swipes.rs (1)
430-466:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis fast path still fractures create-time uniqueness for explicit IDs.
Still unresolved, Doctor's note: the explicit-ID precheck happens earlier in
prepare_message_create_row, before the storage write lock. Two concurrent creates with the same caller-suppliedidcan both reach this branch and append duplicatemessagesplus duplicatemessage-swipesrows, while the fallback path still replaces bymessage_id. That makes persisted semantics depend on cache state instead of the create contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src-tauri/src/commands/storage/message_swipes.rs` around lines 430 - 466, The fast-path in append_created_message_and_swipes_if_uncached allows two concurrent creates with the same caller-supplied id to both succeed because the explicit-ID precheck in prepare_message_create_row runs before any storage write lock; fix by ensuring uniqueness is enforced inside the atomic append or by taking the same write lock/replace semantics as the fallback: either 1) have state.storage.append_many_uncached check for existing message id (and return false if present) so append_many_uncached enforces uniqueness for the ("messages", ...) tuple, or 2) move the explicit-ID existence check into persist_created_message_with_swipes under the same lock used by write_message_and_swipes so the code path that calls append_created_message_and_swipes_if_uncached cannot race on caller-supplied ids; update append_created_message_and_swipes_if_uncached to rely on the storage append result (state.storage.append_many_uncached) to decide fallback to write_message_and_swipes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src-tauri/src/commands/storage/message_swipes.rs`:
- Around line 430-466: The fast-path in
append_created_message_and_swipes_if_uncached allows two concurrent creates with
the same caller-supplied id to both succeed because the explicit-ID precheck in
prepare_message_create_row runs before any storage write lock; fix by ensuring
uniqueness is enforced inside the atomic append or by taking the same write
lock/replace semantics as the fallback: either 1) have
state.storage.append_many_uncached check for existing message id (and return
false if present) so append_many_uncached enforces uniqueness for the
("messages", ...) tuple, or 2) move the explicit-ID existence check into
persist_created_message_with_swipes under the same lock used by
write_message_and_swipes so the code path that calls
append_created_message_and_swipes_if_uncached cannot race on caller-supplied
ids; update append_created_message_and_swipes_if_uncached to rely on the storage
append result (state.storage.append_many_uncached) to decide fallback to
write_message_and_swipes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ffe2293-3a61-4dbe-80f8-d8d8b7931325
📒 Files selected for processing (2)
src-tauri/crates/storage/src/lib.rssrc-tauri/src/commands/storage/message_swipes.rs
Linked issue
Related to #2493
Why this change
messagesandmessage-swipescaches, which made the append fast path unreachable unless the collections were completely uncached.What changed
messagesandmessage-swipesrows together instead of rewriting both full collections.Refactor impact
Primary owner:
Rust storage
Impact areas reviewed:
src-tauri/crates/storage/src/lib.rscollection append, cache, temp-file rollback, and dirty-cache behavior.src-tauri/src/commands/storage/message_swipes.rscreate-message sidecar persistence.messages/message-swipescache behavior.Boundary notes:
Pressure points touched:
ModeSurface,GameSurface, command registration, or import modules touched.Validation
pnpm typecheck,pnpm build,pnpm check:architecture,pnpm check:docs, or fullpnpm checkwhen warranted)pnpm checkpasses before PR push/handoffManual verification notes
Commands run locally:
cargo fmt --manifest-path src-tauri/crates/storage/Cargo.toml -- --checkgit diff --checkcargo test --manifest-path src-tauri/Cargo.toml -p marinara-storage append_many_uncachedcargo test --manifest-path src-tauri/Cargo.toml -p marinara-engine message_swipes --libcargo check --manifest-path src-tauri/Cargo.toml --workspacepnpm checkManual/native Tauri timing proof was not run yet. Follow-up should compare send and regenerate timings on a large chat.
Feature Discoverability
Check exactly one:
src/features/shell/discovery/because this PR adds or materially changes a user-discoverable feature, workflow, setting, mode, panel, import path, agent, media capability, or advanced tool.Reason:
Docs and release impact
README.mdCONTRIBUTING.mddocs/developer/AGENTS.mdUI evidence
No visible UI changes. Runtime timing proof remains follow-up work for large-chat performance measurement.
Summary by CodeRabbit
Release Notes
New Features
Refactor
Tests