Bug 319450 - Mailbox synchronization can miss important mailbox state updates -- broken EXISTS and UIDNEXT heuristic
Summary: Mailbox synchronization can miss important mailbox state updates -- broken EX...
Status: RESOLVED NOT A BUG
Alias: None
Product: Akonadi
Classification: Frameworks and Libraries
Component: IMAP resource (show other bugs)
Version: GIT (master)
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Kevin Ottens
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-07 07:50 UTC by Jan Kundrát
Modified: 2013-05-16 19:37 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Kundrát 2013-05-07 07:50:47 UTC
This is how the current kdepim-runtime/resources/imap/retrieveitemstask.cpp looks like:


  } else if ( messageCount > realMessageCount && messageCount > 0 ) {
    // The amount on the server is bigger than that we have in the cache
    // that probably means that there is new mail. Fetch missing.

    kDebug( 5327 ) << "Fetch missing: " << messageCount << " But: " << realMessageCount;

    KIMAP::FetchJob *fetch = new KIMAP::FetchJob( m_session );
    fetch->setSequenceSet( KIMAP::ImapSet( realMessageCount+1, messageCount ) );


If I understand this correctly and it really sends ``FETCH oldExists+1:*`` over the wire, then it's broken -- it will fail to notice any deletions among the old messages, for example.

Goging further, the next branch apparently assumes that UIDs are assigned on a consecutive manner, i.e. after 123, there will always be 124. This is wrong; the UIDNEXT as provided by the IMAP server is a lower bound on what the next visible UID might be. It is perfectly safe to have messages with assigned UIDs 10, 15, 21 etc from the very beginning (not to mention a possible removal on the server side from a concurrent client while Akonadi is offline).

  } else if ( messageCount == realMessageCount && oldNextUid != nextUid
           && oldNextUid != 0 && !firstTime && messageCount > 0 ) {
    // amount is right but uidnext is different.... something happened
    // behind our back...
    kDebug( 5327 ) << "UIDNEXT check failed, refetching mailbox";

    qint64 startIndex = 1;
    // one scenario we can recover from is that an equal amount of mails has been deleted and added while we were not looking
    // the amount has to be less or equal to (nextUid - oldNextUid) due to strictly ascending UIDs
    // so, we just have to reload the last (nextUid - oldNextUid) mails if the uidnext values seem sane
    if ( oldNextUid < nextUid && oldNextUid != 0 && !firstTime )
      startIndex = qMax( 1ll, messageCount - ( nextUid - oldNextUid ) );


This is wrong -- if the UIDNEXT was 10 previosuly and is now 11, the code will do a FETCH 8:*. However, there is no guaranteee whatsoever that the removal was not for, let's say, message with seq. number 1.

This is based just on reading the synchronization code, I have not performed any real tests, but it looks like testretrieveitemstask.cpp does not have a test coverage for this situation.

There's quite a lot of testcases for this in Trojita; please feel free to get in touch if you cannot find them.

Reproducible: Always
Comment 1 Volker Krause 2013-05-16 19:37:13 UTC
Please only report verified problems, not misunderstandings of the code.

The code cited above is only the part handling downloading of new messages, detection of deletions, flag changes, etc happens elsewhere in that class as well as to a large part in Akonadi::ItemSync.