mustang-im / mustang

Mustang - New full-featured desktop email, chat and video conference client
https://mustang.im
Other
9 stars 2 forks source link

IMAP: Deleted mails come back #120

Open benbucksch opened 3 months ago

benbucksch commented 3 months ago

This is a followup to #105

Actual result:

Expected result:

NeilRashbrook commented 3 months ago

I don't have easy access to an IMAP folder with 10000 deletable messages so I tried with 100 or so messages and although I was unable to reproduce the problem I did trip over this error whatever it means:

SqliteError: NOT NULL constraint failed: emailPersonRel.emailID at Database.run (/home/neil/github/mustang/e2/out/main/index.js:98176:38) at JPCWebSocket.funcListener (/home/neil/github/mustang/e2/out/main/index.js:46416:28) at process.processTicksAndRejections (node:internal/process/task_queues:95:5) at async WSCall._incomingMessage (/home/neil/github/mustang/e2/out/main/index.js:45981:20) at async WebSocket. (/home/neil/github/mustang/e2/out/main/index.js:46075:9) { code: 'SQLITE_CONSTRAINT_NOTNULL' } at message.js:153:16 at new Promise () at WSCall.makeCall (message.js:148:12) at JPCWebSocket.callRemote (protocol.js:142:31) at RemoteClass.run (obj.js:225:55) at SQLEMail.saveRecipient (SQLEMail.ts:128:33) at async SQLEMail.saveRecipients (SQLEMail.ts:94:5) at async SQLEMail.save (SQLEMail.ts:67:5) at async IMAPFolder.messageFlagsChanged (IMAPFolder.ts:222:5) at async IMAPAccount.ts:161:9

NeilRashbrook commented 3 months ago

I don't know whether it's relevant but I don't see any logic in IMAPEMail.ts that actually saves the desired read state in the database, but if you're worried about the email becoming unread while you're executing the command to mark it read on the server, you could always set its read state twice, once before and once after the server call, just in case. Or maybe you want a flag on the message that says you're changing flags and you don't want anyone changing them during the server call?

benbucksch commented 3 months ago

NOT NULL constraint failed: emailPersonRel.emailID

I see that very rarely, maybe 1-2% of deletes. It seems like it's a race as well. (If you find the cause, please fix it.) But that's not the bug here.

benbucksch commented 3 months ago

maybe you want a flag on the message that says you're changing flags and you don't want anyone changing them during the server call?

That's a nice idea, and clean to implement in the current function structure. I did that in c2c2ebb .

benbucksch commented 3 months ago

However, not surprisingly, while testing, that doesn't fix the deleted emails coming back. I didn't touch that, because the folder.deleted array in IMAPEMail.deleteMessageOnServer() should already fix that, no? Could you please look into that?

benbucksch commented 3 months ago

It can fairly easily reproduce that by deleting a bunch of messages, while reading (and automatically marking as read) a bunch of others. The typical actions when going through 20-30 messages and deleting half of them. After 20-30 seconds, a few of them (1 out of 5?) will come back, but not all.

NeilRashbrook commented 3 months ago

I wasn't able to reproduce deleted emails coming back. I did reproduce what appears to be a front-end issue corrupting the message list. It behaves as follows:

  1. If at any point you get an IMAP error, quit, clear out the mail database, and restart
  2. In the Parula account, delete any messages in the folder "Bigger"
  3. Select all of the messages in the folder "Big" and copy them to the folder "Bigger", trying to avoid marking them as read (easier said than done)
  4. Delete the first message, then move down and delete the now second message, eventually marking all of the messages read and also deleting half of them

Expected results: A folder with the same emails as the folder "Biggest".

Actual results: The message list becomes corrupt part way though, typically when trying to read the message "Test of ExQuilla on TB 115"; instead, you end up reading a clone (it has the same dbID but it doesn't compare equal) of "Test BCC". Eventually "Test of ExQuilla on TB 115" reappears but of course you haven't read it.

I added logging to try and find out what was mutating the message list but I didn't find anywhere in app/logic where it might have been happening (we aren't even calling listMessages for instance).

benbucksch commented 1 month ago

I've added a HACKish workaround that checks only recent messages for deleted messages.