Skip to content

Add chat swipe append fast path - #2509

Closed
Xelvanis wants to merge 2 commits into
refactorfrom
perf/issue-2493-add-swipe-fast-path
Closed

Add chat swipe append fast path#2509
Xelvanis wants to merge 2 commits into
refactorfrom
perf/issue-2493-add-swipe-fast-path

Conversation

@Xelvanis

@Xelvanis Xelvanis commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Linked issue

Part of #2493

Why this change

  • Regenerating assistant messages still paid the old full atomic rewrite cost for sidecar-backed swipes. Legacy execution only inserted the new swipe row and patched the parent message row, so this keeps the refactor storage path closer to that granular behavior while preserving fallback cleanup.

What changed

  • Added a storage primitive that stages append-only collection writes together with clean collection replacements, then commits or rolls back the whole set.
  • Updated chat_message_add_swipe to fast-path sidecar-backed messages by replacing the parent messages row, appending only new message-swipes rows, and applying chat memory invalidation in the same transaction when active content changes.
  • Kept the full replacement writer for embedded swipes, dirty cached collections, missing expected rows, and non-canonical legacy sidecars.
  • Added focused Rust coverage for canonical sidecar gating, trimmed legacy sidecar cleanup, active add-swipe memory pruning, cache fallback, rollback, and temp cleanup.

Refactor impact

Primary owner:

Rust storage and chat storage commands.

Impact areas reviewed:

  • FileStorage append/update transaction staging, rollback, and cache refresh.
  • Message swipe sidecar materialization and persistence shape.
  • chat_message_add_swipe active/inactive semantics and legacy silent compatibility.
  • Chat memory pruning when an active assistant message changes.
  • Legacy sidecar rows that match only after trimming and must fall back to full cleanup.

Boundary notes:

  • Changes stay in src-tauri Rust storage/command modules.
  • No React, TypeScript engine, shared API, command registration, or remote-runtime dispatch changes.
  • The new fast path returns None on unsafe storage shape so the existing full replacement path remains the correctness fallback.

Pressure points touched:

  • Rust storage command helpers only.
  • Did not touch ModeSurface, GameSurface, shared mode UI, src-tauri/src/lib.rs command registration, or import modules.

Validation

  • Matching validation command passes locally (for example pnpm typecheck, pnpm build, pnpm check:architecture, pnpm check:docs, or full pnpm check when warranted)
  • Full pnpm check passes before PR push/handoff
  • Human/manual validation completed by contributor or reviewer

Manual verification notes

Commands run locally:

  • cargo test --manifest-path src-tauri/Cargo.toml -p marinara-engine message_swipes_append_cleans_trimmed_legacy_sidecar_rows --lib
  • cargo test --manifest-path src-tauri/Cargo.toml -p marinara-engine message_swipes --lib
  • cargo test --manifest-path src-tauri/Cargo.toml -p marinara-storage append_many --lib
  • cargo check --manifest-path src-tauri/Cargo.toml --workspace
  • rustfmt --edition 2021 --check src-tauri/crates/storage/src/lib.rs src-tauri/src/commands/storage/chats.rs src-tauri/src/commands/storage/message_swipes.rs
  • git diff --check
  • pnpm check:architecture
  • pnpm check

Durable test rationale:

  • This changes a risky storage invariant: fast append must not preserve stale or malformed sidecar rows that the full replacement path would canonicalize.
  • Existing command output alone would not pin the fallback condition for legacy trimmed sidecar rows.
  • The added tests are narrow Rust owner tests around sidecar canonicality, append cleanup, and storage transaction behavior.

Native Tauri large-chat timing:

  • Rerun against the Phase 2 storage diff through the native Tauri/WebView2 QA path.
  • chat_message_add_swipe: 358.9ms -> 266.4ms on the 5k-message fixture.
  • Regenerate two-step total: 998.9ms -> 897.4ms.
  • The follow-up patchChatMessageExtra write still costs 631.0ms, so Phase 3 remains the clearest next latency win.
  • Broad refresh cost is effectively unchanged at about 190-201ms per scheduled refresh.

Remaining proof gap:

  • This timing pass did not run a live LM Studio generation stream.

Feature Discoverability

Check exactly one:

  • Updated 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.
  • N/A because this PR is only a bugfix, refactor, test, docs, internal wiring, visual polish, copy edit, or compatibility fix and does not add a new thing users need to find.

Reason:

  • Internal storage performance/correctness path only; no new user-discoverable feature or workflow.

Docs and release impact

  • No docs changes needed
  • Updated README.md
  • Updated CONTRIBUTING.md
  • Updated docs/developer/
  • Updated repo skills or AGENTS.md
  • Confirmed this PR does not restore old staging/package-workspace/release claims

UI evidence

No UI screenshots or recordings. This PR changes Rust storage behavior only.

Summary by CodeRabbit

  • Improvements
    • Optimized message swipe handling for faster append operations when certain conditions are met, avoiding full message replacement in common cases
    • Refined chat memory updates to invalidate only when visible content actually changes, improving performance
    • Added validation checks to ensure swipe data integrity during storage operations

@coderabbitai

coderabbitai Bot commented Jun 6, 2026

Copy link
Copy Markdown
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 39.29% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Add chat swipe append fast path' directly and clearly describes the main change: implementing a fast path for appending chat message swipes.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The PR description is comprehensive and well-structured, covering linked issues, rationale, detailed changes, validation steps, and impact analysis.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch perf/issue-2493-add-swipe-fast-path

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown

✅ Bunny Review Completed

Tip

Review posted. The specimen has left the observation table.

  • Commit: d4571a0 - Harden swipe append transaction checks

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown

🐰 Bunny Review

Bunny Merge Signal: Action Needed

Warning

ACTION NEEDED
Actionable findings or blocking proof gaps remain for this head.

Findings Nitpicks Controls Reviewed Head Mode
1 1 2 warn, 2 pass d4571a0f incremental

Note

Mode: incremental
Head: d4571a0 - Harden swipe append transaction checks
Base: c2c4b92 - Add chat swipe append fast path

🧭 Specimen Summary

  • The specimen drags swipe sidecar canonicality into the hybrid storage append path, where the rows are at last examined beneath the same write lock that stages the append. It also repairs the projected-cache afterlife of appended collections, sparing filtered reads from consulting stale ghosts. The experiment is mostly sound, but its sequencing has one curious deformity: update work is allowed to run before append eligibility has been proven.

🔎 Isolated Defects

Severity Location Finding
⚠️ MEDIUM src-tauri/crates/storage/src/lib.rs:697 Append validation waits behind update work

✅ Resolved Since Last Review

  • src-tauri/src/commands/storage/message_swipes.rs:510 - Canonical sidecar check escapes the transaction
  • src-tauri/crates/storage/src/lib.rs:2072 - Append projections survive the hybrid transaction

🧹 Nitpicks

Location Nitpick
src-tauri/src/commands/storage/message_swipes.rs:509 Mutable iterator without mutation
🤖 Copy prompt for isolated Bunny findings
Task: Fix `src-tauri/crates/storage/src/lib.rs:697`.
Finding: Append validation waits behind update work
Severity: medium
Goal: If append collection validation fails, no update callback effects, update errors, parent mutations, or extra-collection mutations may be observable before the caller receives the fallback signal.
Cover: Noncanonical `message-swipes` rows causing append fallback after `update_collections` has already inspected or mutated extra collection rows.; Malformed or concurrently changed append collections returning `None` only after the update closure has produced an unrelated error.; Future append-check callers performing captured side effects inside the update closure before append eligibility is known.
Avoid: Validating only during staging still leaves the update closure running on a path that should have fallen back.; Documenting the callback as pure without enforcing or testing caller purity leaves the same observable failure available.; Checking dirty cache state before the callback is not equivalent to validating the exact append rows that govern the commit.
Acceptable fixes: Load and validate unique append collections before calling `update(&mut loaded)`.; Split the helper into an append-validation phase followed by the update closure, with a stamp or dirty recheck preserving the validated append snapshot until staging.; For the swipe caller, perform the locked sidecar canonical check before invoking parent or extra collection update logic, then fall back without running that logic when validation fails.
Proof required: A focused test where append validation fails and the update closure would otherwise error or mutate captured state, proving the closure is not invoked.; A swipe-specific regression where noncanonical existing sidecars cause fallback without touching parent message rows or extra collection update logic.; A concurrency or interleaving test showing append rows changed after initial eligibility are rejected or fall back before update callback effects occur.
Run the narrowest relevant check. If stale, leave code unchanged and record why.

✅ Control Checks

Status Type Check Detail
✅ PASS CI Timing CI Status Expected CI controls completed without a reported failure.
⚠️ WARN Proof Gap Prior contract: canonical sidecar check escapes the transaction The sidecar predicate now reloads append rows under the storage write lock before staging, which repairs the original row-race anatomy; the remaining lesion is that update callback work still precedes the predicate.
✅ PASS Non-blocking Coverage Prior contract: append projections survive the hybrid transaction The hybrid path now invalidates projected caches for appended collections before extending ordinary cached rows, and the warmed projected-cache regression test pins the behavior.
⚠️ WARN Proof Gap Proof The added tests cover malformed append files, append validation, reentrant write rejection, and projected-cache invalidation; none proves failed append validation prevents update callback execution.

🧪 Observations

  • Inspected the incremental diff from c2c4b924f258149dd43a88cde10475b773a0daef to d4571a0f18c6fcde9b7c7bc28184916eb7835837 across src-tauri/crates/storage/src/lib.rs and src-tauri/src/commands/storage/message_swipes.rs.
  • Judged both prior Bunny repair contracts before issuing the remaining sequencing finding.
  • Checked the hybrid append transaction ordering: dirty-cache checks, update-row stamp checks, callback execution, append-row reload, validation, staging, rollback, and cache repair.

🧰 CI Status

  • pass: Frontend, Architecture, and Organization
  • pass: Rust Capability Layer
  • pass: Browser Smoke and Performance
  • note: Bunny gates only required job conclusions from ci-checks.json; advisory reports such as continue-on-error dependency checks are not included in this status.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bunny Review inline findings

Comment thread src-tauri/src/commands/storage/message_swipes.rs Outdated
Comment thread src-tauri/crates/storage/src/lib.rs
@coderabbitai coderabbitai Bot added the bugfix Bug fix label Jun 6, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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.

Inline comments:
In `@src-tauri/crates/storage/src/lib.rs`:
- Around line 2021-2033: The loop treating any Some(item) from
stage_appended_collection as safe must not trust a mere trailing ']' check;
update stage_appended_collection (the function called in the for loop over
appends) to fully validate that the existing target file is a parseable JSON
array before returning Some(staged_item), and if validation fails return
Ok(None) so the caller falls back to the safe (non-fast-path) commit flow;
specifically, inside stage_appended_collection verify the file parses as a JSON
array (e.g., attempt to deserialize or at least parse the full array structure)
and only prepare/rename the staged file when parsing succeeds, otherwise return
Ok(None) to trigger fallback.
- Around line 640-655: The write lock obtained via self.lock.write() is held
while the update callback is invoked, risking deadlock; change the flow in the
function that calls append_many_and_update_collections_uncached_locked so that
you drop the write guard before invoking the user-provided update closure (i.e.
acquire lock, perform pre-checks like ensure_writes_available() and
any_collection_dirty_cached(...), then release the guard), and then run the
closure outside the lock while using the atomic_update_active flag (like
atomic_update_active used in update_collections_atomically) to detect and fail
fast on re-entrant updates; ensure
append_many_and_update_collections_uncached_locked (and any call sites at lines
~2002-2004) are adjusted to expect the closure to run without the write guard
held.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 0ecaab3c-dcc5-4558-9bc5-5179e698d436

📥 Commits

Reviewing files that changed from the base of the PR and between 237babc and c2c4b92.

📒 Files selected for processing (3)
  • src-tauri/crates/storage/src/lib.rs
  • src-tauri/src/commands/storage/chats.rs
  • src-tauri/src/commands/storage/message_swipes.rs

Comment thread src-tauri/crates/storage/src/lib.rs
Comment thread src-tauri/crates/storage/src/lib.rs

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bunny Review inline findings

(loaded, original_stamps)
};

let Some(output) = update(&mut loaded)? else {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ MEDIUM: Append validation waits behind update work

Location: src-tauri/crates/storage/src/lib.rs:697

A fascinating inversion: the append-side predicate is presented as the commit-time gate, yet the update closure runs before the append collection is loaded and validated. In the swipe caller, noncanonical sidecars can still trigger parent or extra-collection update logic, closure errors, or captured side effects before the fast path finally returns None for fallback. The previous precheck refused this poisoned sidecar genealogy before touching the update callback; this version delays the diagnosis until after the callback has already been invited into the laboratory.

Tip

Suggested fix: Validate the append collections before invoking the update closure, or enforce and prove that the callback is sterile enough to run on a path that will be rejected.

@Xelvanis
Xelvanis marked this pull request as draft June 6, 2026 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants