fix(conversations): atomic item mutations with transactional message cache sync

[eoinfennessy]

Summary

• Adds create_items_and_sync_messages and delete_item_and_sync_messages to the ConversationItemStore trait, combining position assignment, item mutation, and message cache rebuild into single database transactions. This eliminate races under concurrent appends and stale cache overwrites
• Removes refresh_message_cache and sync_conversation_messages, which constructed a ConversationRecord with bogus field values just to thread tenant_id and conversation_id through to the sync logic
• Adds a unique index on (tenant_id, conversation_id, position) as defense-in-depth against position collisions
• Uses BEGIN IMMEDIATE (SQLite) and SELECT ... FOR UPDATE (PostgreSQL) to serialize concurrent writes

Test plan

• 5 new SQLite tests covering position assignment, multi-batch appends, deletion with cache sync, empty batches, and nonexistent items
• 2 new PostgreSQL tests covering position assignment and deletion with cache sync
• Existing duplicate_positions test replaced with duplicate_position_rejected_by_unique_constraint
• Full test suite passes (2005 tests)

Closes #358

[praxis-bot-app]

PR too large: 815 lines added (limit: 750, excludes Cargo files, tests, docs, examples, and benchmarks). Please split into smaller PRs. Add skip/pr-conventions label to override.

[franciscojavierarceo]

Requesting changes for two merge-blocking correctness issues and one SQLite performance regression. The existing mixed-batch API thread at filter.rs:581 also remains valid and unresolved.\n\nI reproduced the legacy-duplicate initialization failure on SQLite. Targeted transactional tests otherwise passed locally, while the PR is currently also failing nightly rustfmt and the coverage threshold.

[praxis-bot]

Review: fix(conversations): atomic item mutations with transactional message cache sync

Severity	Count
Large	1
Medium	1

Solid architectural improvement. The transactional approach correctly eliminates the read-compute-write race on max_item_position that caused position collisions under concurrent appends. SELECT ... FOR UPDATE (PostgreSQL) and BEGIN IMMEDIATE (SQLite) are the right serialization strategies for each backend. The removal of the bogus ConversationRecord construction in refresh_message_cache / sync_conversation_messages is a clean simplification. Test coverage for the new methods is thorough, including the concurrent SQLite test that proves the locking works.

[praxis-bot]

Review: fix(conversations): atomic item mutations with transactional message cache sync

Severity	Count
Medium	1

The transactional approach is well-designed and the serialization strategies are correct for each backend (SELECT ... FOR UPDATE for PostgreSQL, BEGIN IMMEDIATE for SQLite). Clean removal of the ConversationRecord construction hack in refresh_message_cache / sync_conversation_messages. The concurrent SQLite test is a good addition. Handler refactoring correctly simplifies the existence-check and mutation calls.

[praxis-bot]

Review: fix(conversations): atomic item mutations with transactional message cache sync

Severity	Count
(none new)	0

Strong improvement. The transactional approach correctly eliminates the TOCTOU race on max_item_position that caused position collisions under concurrent appends. Serialization strategies are appropriate for each backend: SELECT ... FOR UPDATE for PostgreSQL and BEGIN IMMEDIATE for SQLite. The *_rebuild_messages helpers properly detect concurrent conversation deletion via rows_affected() == 0 and propagate the error, rolling back the entire transaction so no orphaned items persist.

Handler refactoring is clean. The move from two separate operations (create_conversation_items + sync_conversation_messages) to a single create_items_and_sync_messages call removes the bogus ConversationRecord construction that existed only to thread tenant_id/conversation_id through to the sync logic. The handle_create_items and handle_delete_item existence checks are simplified to discard the record since it is no longer needed for the sync call.

Test coverage is thorough: position assignment, multi-batch continuation, empty batch no-op, missing conversation rollback (verifying items are cleaned up), deletion with cache sync, nonexistent item returns false, and a concurrent write test verifying distinct positions under contention. The PostgreSQL tests mirror the SQLite ones for the core paths. The rollback verification tests (*_missing_conversation_errors) are well-designed.

The unique index on (tenant_id, conversation_id, position) is good defense-in-depth. Previous review feedback about migration risk on existing data with duplicate positions remains the main outstanding consideration for the upgrade path.

No new findings at Medium or above.

[eoinfennessy]

Hi @aslakknutsen @franciscojavierarceo, I have addressed your requested changes. PTAL when you get a chance

[praxis-bot]

Review: fix(conversations): atomic item mutations with transactional message cache sync

Severity	Count
Medium	1

The transactional redesign is solid and the test coverage for the new methods is thorough (position assignment, multi-batch continuation, empty batch no-op, missing conversation rollback, deletion with cache sync, concurrent writes). The prior review findings remain the main outstanding items.

One new finding: the coverage exclusion added for store/postgres.rs is overly broad and suppresses coverage measurement for all existing and future PostgreSQL store code.

[praxis-bot]

Round 6 review -- 1 new finding (Medium).

The transactional approach is sound and the test coverage for the new methods is solid. One scope-consistency concern in the INSERT path.

[praxis-bot]

Review: fix(conversations): atomic item mutations with transactional message cache sync

Severity	Count
(none new)	0

Round 7 review -- no new findings at Medium or above.

The iterative work since the last review has been good: commit da27ad4 restored the existence guard in *_rebuild_messages via rows_affected() checks, and f747003 eliminated the empty-messages fallback in handle_create_conversation. The transactional design is sound, serialization strategies are correct per backend, and the test suite is thorough (position assignment, multi-batch continuation, concurrent writes, missing-conversation rollback for both create and delete paths).

The following prior findings remain the outstanding items for consideration before merge:

1. [Large] Unique index migration risk on existing data with duplicate positions (schemas.rs)
2. [Medium] PostgreSQL lock query result discarded without checking row existence -- pg_rebuild_messages catches it downstream, but early-out on locked.is_none() would avoid inserting items that get rolled back (postgres.rs)
3. [Medium] INSERT binds item.tenant_id / item.conversation_id instead of the function parameters in both PostgreSQL and SQLite -- scope fields can silently diverge (postgres.rs, sqlite.rs)
4. [Medium] Coverage exclusion suppresses the entire store/postgres.rs file, not just the new transactional methods (Makefile)

[leseb]

@aslakknutsen @franciscojavierarceo PTAL to unblock this, thanks

[eoinfennessy]

@aslakknutsen @franciscojavierarceo can you please review again to unblock this?

test-praxis-main now fails, but I don't think this is related to my changes.

[leseb]

@eoinfennessy please rebase

[praxis-bot-app]

Missing Signed-off-by: eeccf2f, ad90f9e. All commits require sign-off (via git commit --signoff).

[praxis-bot]

Review: fix(conversations): atomic item mutations with transactional message cache sync

Severity	Count
(none new)	0

Round 8 review -- no new findings at Medium or above.

The two actionable findings from Round 7 have been addressed:

• [Fixed] rows_affected() check restored in pg_rebuild_messages and sqlite_rebuild_messages (9c9e8ff). A conversation deleted concurrently between the handler's existence check and the transaction now produces "conversation disappeared during message sync" and rolls back item mutations. Both create and delete paths are covered, with SQLite tests for each and mirrored #[ignore]d PostgreSQL tests.
• [Fixed] INSERT in create_items_and_sync_messages now binds the function parameters tenant_id / conversation_id instead of the item record fields (345ad3d). The scope invariant is enforced at the query level across both backends.

Remaining prior findings (unchanged from Round 7):

1. [Large] Unique index migration risk on existing data with duplicate positions (schemas.rs)
2. [Medium] Coverage exclusion suppresses the entire store/postgres.rs file (Makefile)