Fix/update draft race with send - #774
Conversation
|
Warning Review limit reached
Next review available in: 26 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR adds message-row locking during draft update and send flows. It adds guarded recipient-status persistence for outbound delivery. It also adds frontend send guards and regression tests for delivery races and finalized-message updates. ChangesDraft and Delivery Race Fix
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant MessageForm
participant SendMessageView
participant Database
participant OutboundWorker
participant MTA
User->>MessageForm: submit message
MessageForm->>SendMessageView: send draft
SendMessageView->>Database: lock and finalize message
SendMessageView->>OutboundWorker: dispatch after commit
OutboundWorker->>MTA: send message
MTA-->>OutboundWorker: delivery statuses
OutboundWorker->>Database: save each recipient status
alt send fails
SendMessageView-->>MessageForm: error
MessageForm->>MessageForm: restore autosave
else recipient row changed
Database-->>OutboundWorker: savepoint error
OutboundWorker->>OutboundWorker: log warning and continue
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/frontend/src/features/forms/components/message-form/index.tsx`:
- Around line 629-632: Keep autosave disabled throughout the send flow by adding
a submission-state ref used by every autosave restart path, including
saveDraftInner’s finally block and the [draft] effect’s startAutoSave call. Set
the ref before the awaited send preparation begins, prevent startAutoSave while
submission is active, and clear the ref only after messageMutation.mutate()
settles or the submission aborts.
- Around line 651-653: Prevent draft saves from starting while a message
submission is active: update the visible saveDraft/descendant blur handling to
check the submission ref used by messageMutation and return without saving when
submission has begun. Ensure this guard covers blur events occurring during or
after the existing saveDraftPromiseRef wait, and keep editing disabled or
blocked until the mutation settles.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 6d1d2e6d-3322-4b19-b0d1-8d0ab6bb5def
📒 Files selected for processing (4)
src/backend/core/api/viewsets/draft.pysrc/backend/core/mda/outbound.pysrc/backend/core/tests/mda/test_outbound_recipient_race.pysrc/frontend/src/features/forms/components/message-form/index.tsx
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/frontend/src/features/forms/components/message-form/index.tsx (1)
666-668: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAbort the send when draft persistence fails.
saveDraftInnercatches draft mutation errors at Line 594 to Line 596 and returns the current draft ID. The waits at Line 651 to Line 652 and Line 666 to Line 668 therefore only wait for completion. They do not prove that the latest recipient update was stored.The send payload at Line 674 to Line 681 contains no recipients. A failed recipient update can therefore send the message using the old persisted recipient set. Propagate the send-preparation save error and abort before
messageMutation.mutate.🤖 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/frontend/src/features/forms/components/message-form/index.tsx` around lines 666 - 668, Update the send preparation flow in the message form, including saveDraftInner and the waits on saveDraftPromiseRef.current, to propagate recipient-persistence failures instead of swallowing them. Before messageMutation.mutate, abort the send when the latest recipient save fails; preserve the existing send path only after the draft save completes successfully.
🤖 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/backend/core/mda/outbound.py`:
- Around line 522-532: The exception handler in the atomic transaction block
catches all DatabaseError instances, including deadlocks and timeouts, and logs
them as "vanished during delivery" without recording the actual failure. This
causes the recipient row to retain delivery_status = NULL and get re-processed,
creating duplicate sends. After logging the warning for DatabaseError (and
removing the unreachable ValidationError branch), check whether the recipient
row still exists in the database by querying with recipient.pk. If the row
exists, re-raise the caught DatabaseError so the caller-side handler at lines
815-820 records the real failure. Only absorb the exception if the row is
confirmed to be gone, which is the safe vanished-row case.
In `@src/backend/core/tests/mda/test_outbound_recipient_race.py`:
- Around line 88-93: Add API-level tests alongside the existing
finalized-message coverage for the draft PUT and send endpoints: verify PUT
returns 404 for a finalized message and send rejects a message that cannot be
locked. Exercise the endpoint routes and assert the response status, ensuring
the tests depend on the endpoint’s row-locking and is_draft=True behavior rather
than only calling core.mda functions.
In `@src/frontend/src/features/forms/components/message-form/index.tsx`:
- Line 533: The ensureDraft method allows force-bypassed mutations that race
with messageMutation during send, as shown by the condition that permits
mutations when force=true despite isSendingRef.current. Create a private
prepare-on-send method without the force parameter for internal use during
message delivery, update the send operation to call this private path instead of
ensureDraft, and modify ensureDraft to reject all mutations (including forced
ones) when isSendingRef.current is true by removing the force bypass. Also guard
deleteDraft to return early when isSendingRef.current is true to prevent draft
deletion until the send operation completes.
---
Outside diff comments:
In `@src/frontend/src/features/forms/components/message-form/index.tsx`:
- Around line 666-668: Update the send preparation flow in the message form,
including saveDraftInner and the waits on saveDraftPromiseRef.current, to
propagate recipient-persistence failures instead of swallowing them. Before
messageMutation.mutate, abort the send when the latest recipient save fails;
preserve the existing send path only after the draft save completes
successfully.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: dee2d29a-0bd2-4d02-921c-0a5123e7e4be
📒 Files selected for processing (7)
src/backend/core/api/viewsets/draft.pysrc/backend/core/api/viewsets/send.pysrc/backend/core/mda/draft.pysrc/backend/core/mda/outbound.pysrc/backend/core/tests/mda/conftest.pysrc/backend/core/tests/mda/test_outbound_recipient_race.pysrc/frontend/src/features/forms/components/message-form/index.tsx
There was a problem hiding this comment.
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/backend/core/tests/api/test_draft_send_race_guards.py`:
- Around line 132-137: The locking-query assertions in the draft PUT and send
race-guard tests must verify the locked query itself checks the draft state. In
src/backend/core/tests/api/test_draft_send_race_guards.py lines 132-137 and
181-186, inspect each captured FOR UPDATE query’s WHERE clause and require the
messages_message.is_draft predicate, while keeping the assertions focused on the
respective locking query.
- Around line 140-153: The test_send_finalized_message_returns_404 test must
also verify that rejected sends do not initiate outbound delivery. Patch
prepare_outbound_message and send_message_task in the test, then assert both
mocks were not called after the finalized-message request returns 404.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: e9891195-191b-4224-9ca8-2ae5eef03402
📒 Files selected for processing (1)
src/backend/core/tests/api/test_draft_send_race_guards.py
dd08769 to
43c0563
Compare
A draft PUT racing a send could pass its is_draft=True fetch before the send finalized the message, then rewrite the MessageRecipient rows (delete + recreate, new UUIDs) while the outbound worker held the old rows. The worker's post-SMTP status save then crashed the delivery with "Save with update_fields did not affect any rows", and the recreated rows were left without delivery status, so the retry task re-sent an already-delivered email. The PUT now locks the message row and re-checks is_draft in the same transaction as the rewrite, serializing it against the send's finalize. The worker records statuses through a queryset UPDATE (warning instead of crash when the row is gone), and the SMTP-failure fallback no longer flips already-delivered recipients back to RETRY.
The 30s autosave tick could fire between the submit's awaits (draft save, editor export) and the send mutation, dispatching a draft PUT concurrently with POST /send/ — the client half of the recipient-rewrite race fixed backend-side. Stop the timer before any await, wait for a blur-triggered save to settle right before sending, and restore the timer when the submit aborts since the draft stays open.
d595b94 to
b6581a2
Compare
1 participant
Purpose
Fix rare race condition that can occured between message send and draft update.
Summary by CodeRabbit