Skip to content

msglist: Drop outbox message when its message arrives in a fetch - #2413

Merged
chrisbobbe merged 3 commits into
zulip:mainfrom
chrisbobbe:pr-reconcileMessage-outbox-duplicates
Aug 6, 2026
Merged

msglist: Drop outbox message when its message arrives in a fetch#2413
chrisbobbe merged 3 commits into
zulip:mainfrom
chrisbobbe:pr-reconcileMessage-outbox-duplicates

Conversation

@chrisbobbe

@chrisbobbe chrisbobbe commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

This PR fixes the common case of #2397:

  • The common case is that the send-message request completes first, and the fetch arrives later, carrying the message. This is covered by the reconcileMessage proposal above.

I did the following for a manual test on my iPhone:

  • Open the app under WiFi connectivity
  • Prepare a message to send
  • Turn off WiFi, falling back to cellular
  • Send the message; see the outbox spinner on it
  • Open a different message list where the message should appear, e.g. by tapping the topic header. (Do this within 90 seconds; see api: Time out long-poll requests, as the server recommends #2396.)
    • Before: the message appeared, but the outbox placeholder was still present, looking like a duplicate.
    • After: the message appears without the outbox duplicate.

Optionally, to see item 3 in #2397 (comment):

  • If we act on the fetched Message by removing the outbox placeholder from all the narrows where it appears, including message lists below the top of the nav stack, then ideally we'd replace it synchronously with the delivered Message in all those same narrows. (E.g. a channel narrow and a topic narrow, where the fetch happened in the topic narrow but the outbox message was removed from both narrows.)

Tap "back" and see that the outbox placeholder is also gone from the first message list, but that the delivered message isn't present. We can fix that in a followup.

chrisbobbe and others added 3 commits August 4, 2026 16:41
@chrisbobbe @claude
message [nfc]: Pull out _removeOutboxMessage helper
fc57beb 
This remove-and-cancel-timers sequence is about to get a fourth
call site.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@chrisbobbe @claude
message: Record message ID from send response on outbox message
0c303cb 
When the send request succeeds, remember the new message ID on the
OutboxMessage.  Nothing reads it yet; this prepares for recognizing
the sent message when it arrives in a fetch (zulip#2397).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@chrisbobbe @claude
msglist: Drop outbox message when its message arrives in a fetch
bc011d8 
Fixes part of zulip#2397.

When the event queue is stuck but other requests are succeeding
(see e.g. zulip#1884 and zulip#514 for ways that can happen), a sent message
can arrive in a message fetch with no message event to remove the
corresponding outbox message.  The user would see the message stuck
in a "sending" state, next to the delivered copy of the same message
if that copy is in view.

Now reconcileMessages drops any outbox message whose message is in
the store, recognized by the message ID recorded from the send
response (zulip#2397).

Message lists respond by removing their copy of the outbox message,
through a new method named for this cause of removal.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@chrisbobbe chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Aug 5, 2026
rajveermalviya
rajveermalviya approved these changes Aug 6, 2026
View reviewed changes

@rajveermalviya rajveermalviya left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @chrisbobbe! All LGTM and tests great, please go ahead and merge.

@chrisbobbe
chrisbobbe merged commit bc011d8 into zulip:main Aug 6, 2026
3 checks passed
@chrisbobbe
chrisbobbe deleted the pr-reconcileMessage-outbox-duplicates branch August 6, 2026 16:12
@chrisbobbe

chrisbobbe commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the review! Done.

@chrisbobbe chrisbobbe mentioned this pull request Aug 14, 2026
Open
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Aug 14, 2026
@chrisbobbe @claude
msglist: Drop outbox message on send success if message already fetched
6e297e1 
Partly fixes zulip#2397.

In PR zulip#2413, we started dropping an outbox message when its event
arrives in a fetch. This helps in the case where the event stream is
stuck (see e.g. zulip#1884 and zulip#514 for ways this can happen).

It should be much rarer in practice, but it's technically possible
for a fetch to arrive with the message before we get the send
request's success. This commit handles that ordering, as anticipated
in my comment on zulip#2397:
  zulip#2397 (comment)

In particular, if the send request is still in flight when the
message arrives in a fetch, now we clear the outbox message when the
send request succeeds, which is when we're able to know the message
ID corresponding to the outbox message. (This means the
duplicate-message symptom described in zulip#2397 can actually still
happen -- between the fetch arriving and the send-message-success --
but that's unavoidable because we can't associate the outbox message
with a real message ID until we get the send-success response.)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Aug 17, 2026
@chrisbobbe @claude
msglist: Drop outbox message on send success if message already fetched
80cbe7c 
Partly fixes zulip#2397.

In PR zulip#2413, we started dropping an outbox message when its message
arrives in a fetch. This helps in the case where the event stream is
stuck (see e.g. zulip#1884 and zulip#514 for ways this can happen).

It should be much rarer in practice, but it's technically possible
for a fetch to arrive with the message before we get the send
request's success. This commit handles that ordering, as anticipated
in my comment on zulip#2397:
  zulip#2397 (comment)

In particular, if the send request is still in flight when the
message arrives in a fetch, now we clear the outbox message when the
send request succeeds, which is when we're able to know the message
ID corresponding to the outbox message. (This means the
duplicate-message symptom described in zulip#2397 can actually still
happen -- between the fetch arriving and the send-message-success --
but that's unavoidable because we can't associate the outbox message
with a real message ID until we get the send-success response.)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Aug 17, 2026
@chrisbobbe @claude
msglist test: Cover a view's own fetch finding a sent message
1d32923 
The tests for dropping an outbox message when its message arrives in
a fetch (from PRs zulip#2413 and zulip#2425) drive the store directly, which
models a fetch made by some other message list. Nothing covered the
view's own fetch returning the message.

That case relies on an ordering inside the fetch: reconcileMessages
deletes the outbox message from the store before
_syncOutboxMessagesFromStore copies outbox messages to the view. If
those were reordered, the view would show the message twice -- once
fetched and once as a stale outbox copy, the duplicate symptom of
zulip#2397 -- and no test would fail.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Aug 17, 2026
@chrisbobbe @claude
msglist: Advance newestFetchedMessageId on new-message events
2771797 
As part of making MessageListView more robust against a stuck event
stream (e.g. zulip#514, zulip#2415), we'd like to add support for invalidating
`haveNewest: true`, for the case where newer messages are learned
about through *other* MessageListViews' fetches.

Example:
- Long-polling is stuck, but message sends and fetches are
  succeeding.
- You send a message; it succeeds but the event doesn't arrive.
- You pop and re-push the message-list page where you sent the
  message, and the new message appears.
- Because the new message was seen in a fetch, the outbox
  placeholder is removed. (This is recent behavior, from zulip#2413
  and zulip#2425.)
  -> But the outbox placeholder was removed *in all message lists
     where it appeared*, not just the current one. E.g. if you
     opened a channel narrow at the newest messages, then opened a
     topic narrow and sent your message from there.
- While long-polling is stuck, the other message list (the channel
  narrow in this example) won't get the new message except if it
  happens to do an independent message fetch, e.g. if the user pops
  and re-pushes that message-list page too.

Planned solution (the third bullet point in my comment on zulip#2397):
since we know the new message belongs in the other narrow (the
channel narrow) and that it's newer than the latest messages there,
we'll invalidate the other narrow's `haveNewest: true`, to make sure
it eventually gets updated with the new message, and shows an
accurate "still-loading-newer" state until that update arrives.

To prepare for that solution, this commit ensures that
newestFetchedMessageId didn't get surpassed by new-message events
that may have landed in the list before the long-poll got stuck.
Since we anchor fetch-newer requests with newestFetchedMessageId,
it's important that newestFetchedMessageId not be lower than any of
the latest messages we're already showing in the list, else the
fetch-newer request would end up duplicating those messages.

This hasn't caused a live message-duplicating bug because we've
never triggered a fetch-newer request when the list contains
messages that landed there via new-message events. We only apply
new-message events when haveNewest is true, and we only make
fetch-newer requests when haveNewest is false, and the transition
from true to false has long been impossible (except on _reset() when
the whole state is cleared, including all tracked messages).

Message moves don't need the same treatment. A move into the view
resets and refetches (see _messagesMovedIntoMessageList), and other
moves don't add messages to the view.

When the view has no messages when a new-message event arrives, also
fill in oldestFetchedMessageId, keeping the anchors' meanings
symmetric.

Also assert the resulting invariant in checkInvariants: [messages]
doesn't extend outside the range bounded by oldestFetchedMessageId
and newestFetchedMessageId. Without the change here, the newest-side
bound fails in existing tests that append a message on an event.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rajveermalviya rajveermalviya rajveermalviya approved these changes

Assignees

@rajveermalviya rajveermalviya

Labels

maintainer review PR ready for review by Zulip maintainers

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@chrisbobbe @rajveermalviya