Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: STORE should not compare by length, but by keys #569

Closed
wants to merge 1 commit into from
Closed

fix: STORE should not compare by length, but by keys #569

wants to merge 1 commit into from

Conversation

titanism
Copy link
Contributor

This fixes issues such as when message counts in folders were incorrect when messages were moved/appended to them.

This fixes issues such as when message counts in folders were incorrect when messages were moved/appended to them.
@andris9
Copy link
Member

andris9 commented Nov 28, 2023

Same comment as with FETCH. Before fixing protocol issues, please provide a sample IMAP transaction that fails and provide information on what you expect to happen (and why you expect it), and what actually happens. Without notifying about appended messages first, these messages do not exist for the client. From this PR it is unclear if the server already notified the client about the new messages or not.

@titanism
Copy link
Contributor Author

titanism commented Nov 28, 2023

A sample IMAP transaction for all of these cases:

  1. You create two messages, and they get appended to Sent Mail folder
  2. Mark them as unread, now your Sent Mail folder has 2 unread messages (count of 2)
  3. Move them to INBOX
  4. The unread count shows as 1
  5. The reason is because the mismatch in selected UID length. These PR's fix this issue. Without this, then you're relying on inaccurate UID list. We need strict comparison of UID's for the queries to be correct for FETCH and other ops. The FETCH is doing a queryAll when it should be doing a query with pageQuery.uid for example. It's returning incorrect data (and vice versa for the other case).

@andris9
Copy link
Member

andris9 commented Nov 28, 2023

I mean, could you provide actual transaction in the style of:

C: X APPEND ....
S: X OK

From this description, it is not possible to determine if anything is broken. The server can use the updated message list only after it has notified the client about the change in currently selected folder. Until then this message does not exist for the client.

@andris9
Copy link
Member

andris9 commented Nov 28, 2023

In particular, does the client use SELECT folder after the message has been appended, or does the server send EXISTS with updated counter to the client before the client uses FETCH?

@NickOvt
Copy link
Contributor

NickOvt commented Nov 30, 2023

A sample IMAP transaction for all of these cases:

1. You create two messages, and they get appended to Sent Mail folder

2. Mark them as unread, now your Sent Mail folder has 2 unread messages (count of 2)

3. Move them to INBOX

4. The unread count shows as 1

5. The reason is because the mismatch in selected UID length.  These PR's fix this issue.  Without this, then you're relying on inaccurate UID list.  We need strict comparison of UID's for the queries to be correct for FETCH and other ops.  The FETCH is doing a `queryAll` when it should be doing a query with `pageQuery.uid` for example.  It's returning incorrect data (and vice versa for the other case).

Hi! Doing it in the wildduck-webmail results in correct display of unread messages. Did exactly as you wrote and inbox is showing me 2 unread messages. Also tried replicating the error directly in IMAP (connected to a wildduck server, did all the manipulations using IMAP commands and checked the flags and response from the API using Postman), but based on flags everything seems correct. Can you please retest or write a more elaborate tutorial on how to reproduce the error?

@titanism
Copy link
Contributor Author

Can you test in an email client such as Thunderbird? @NickOvt

Basically - why is the comparison by length in the first place? Doesn't that seem wrong to compare by length instead of UID's for accuracy? Same for similar PR's/open issues.

@andris9
Copy link
Member

andris9 commented Nov 30, 2023

session.selected.uidList includes all UID values the server has synced to the client. If the UID is not listed in this array, even if the email actually exists, it does not exist for the client and can not be used. Only after the server notifies the client about the change and updates the list can this specific email be included for commands like STORE, FETCH, and SEARCH. So the real question is, why hasn't the server notified the client about the change? The server can not send notifications randomly, but maybe it should have done it already. So, there might be an issue, but the issue is not with this if condition.

Protocol issues can not be validated by using email client user interfaces. It does not matter how Thunderbirds UI behaves. The UI can only give a hint that something is not correct, but it is not a basis for confirmation. Instead, you should check out the IMAP commands Thunderbird sends and the responses the server sends (check IMAP logs from Thunderbird or use an MITM proxy that logs traffic) and check if there is a potential window to send a notification about an appended email to the client. And if there were, did the server send that notification, or did it not?

@titanism
Copy link
Contributor Author

Here is a recording showing what happens without this change (you can see incorrect count in mailbox after 21 second mark, as well as the messages not rendering/being fetched):

recording.mp4

@titanism
Copy link
Contributor Author

Note that the video above is showing FETCH, which is a separate PR. Perhaps this PR is only needed for FETCH and not STORE?

@andris9
Copy link
Member

andris9 commented Nov 30, 2023

I appreciate your effort, but visual proof does not carry any value on this issue. At most, it can show that there is an issue, but it tells nothing about the core of the issue. What is needed to know is the following:

  • Does the server send the client an EXISTS response about the new message in a folder
  • If it does send it, does it send it at the correct time and with the correct value
  • If it does not send it, then why is it skipped

@NickOvt
Copy link
Contributor

NickOvt commented Nov 30, 2023

Could not replicate the issue/problem in ThunderBird. Did it myself first then followed your video - to no avail. Shows correct number of unread messages. Based on the video might as well be issue on the ThunderBird side, perhaps a lag? Because sometimes in ThunderBird the update takes like half a second to a whole second. Connection speed is 500mbps up/down.

UPD: Managed to replicate something similar as on the video. But this indeed seems more like a client (That is Thundebird) issue. It showed 0 unread in sent mail, although the title said there are 1 messages in it. Restarting thunderbird resolved the issue.

But even in this case a proper test should look at the sent and received IMAP commands/responses.

@titanism
Copy link
Contributor Author

titanism commented Dec 1, 2023

@NickOvt I don't think this is a Thunderbird issue. I've used Thunderbird and tested with nearly all other services and self-hosted solutions out there and this problem doesn't exist with them.

@titanism
Copy link
Contributor Author

titanism commented Dec 2, 2023

Another issue (not sure if this is related) but in Thunderbird (and ONLY in WildDuck) we have random messages being shown as replied to (when they're not):

Screen Shot 2023-12-01 at 6 21 38 PM

@titanism titanism closed this Dec 4, 2023
@titanism titanism deleted the patch-6 branch December 4, 2023 04:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants