<DKIM> maxage causes loss of local email

[Nicolas Sebrecht]

On Mon, Mar 09, 2015 at 10:18:17PM -0400, Janna Martl wrote:

> >I think the implementation get it wrong. If you just strip off the
> >hrs/mins, you are only be reducing up to
> >
> >  (maxage - X hours) with X within [0; 24],
> >
> >not the (maxage - 24 hours) we want.
> 
> I'll address the rest of your comments later, but I'm not following
> here. This isn't where the "request +/- 1 day's worth of
> extra messages" is happening; this is just pre-existing code that got
> moved...

Right I got it wrong.

>          to recap, the strategy is:
> 
> (1) Get local messages of "acceptable age".
> (2) Get remote messages. Because of timezone issues, when we make
> SEARCH SINCE queries, we don't know exactly what we're getting, but we
> can at least make sure we're getting *at least* the messages of
> "acceptable age" (but possibly more).
> (3) Reduce that list to messages of "acceptable age".
> 
> I don't think you need to ask anyone for messages of age
> < maxage - 1.

Strictly speaking and regarding the SINCE timezone issue, you're plain
right.

But thinking about this a bit more. We are rounding the current strict
maxage up to a range to avoid issues with the server. Approximating the
inner range allows much better safety regarding the timezone stored from
previous syncs.

Since the whole computing code is there, I think it's good to prevent
from other real life issues that might be rare but hard to debug for
sure:
- wrong timezone written in the filename (for whatever reason: server
  timezone sightly diverged),
- IMAP to IMAP sync, with both sides at very edges timezones (near 24h
  gap).

Now, I was wrong with my comment because we want to consider
(maxage + 1) at first step from both sides. See below.

> Now, this definition does seem like it could be simplified (especially
> since I get the feeling it was written out of a mistaken intention to
> capture exactly what the SEARCH SINCE command is doing); defining
> "acceptable age" as time > now - maxage*24*60*60 makes more intuitive
> sense, but this cutoff might change over the course of offlineimap's
> run, if things take multiple seconds.

Are you talking about the run time to compute the whole thing? If so
don't worry, we are far from the delays we put in the course by simply
sending the SINCE IMAP command and waiting for the response.

> Another thing that I really want to clarify is the place in Base.py
> where the deletion occurs:
> 
> def __syncmessagesto_delete(self, dstfolder, statusfolder):
> 	...
>     deletelist = filter(lambda uid: uid>=0 \
>                             and not self.uidexists(uid),
>                         statusfolder.getmessageuidlist())
> 		...
>         self.ui.deletingmessages(deletelist, [dstfolder])
> 		...
>         for folder in [statusfolder, dstfolder]:
>             folder.deletemessages(deletelist)
> 
> So deletelist = messages in statusfolder that aren't in self's
> messagelist. Immediately afterwards, dstfolder.deletemessages() ends
> up failing to delete messages that aren't in dstfolder's messagelist
> (e.g. messages that are excluded because of maxage), but the "deleting
> blah" message is printed before those messages are excluded.  It would
> be clearer, both to the user and the reader of code, to make
> deletelist = messages in statusfolder that aren't in self.messagelist
> but *are* in dstfolder.messagelist. But then the statusfolder isn't
> updated, which is bad (?) because now it contains messages that were
> excluded because of maxage. So I was going to change it to
> 
> deletelist = filter(lambda uid: uid>=0 \
>                         and not self.uidexists(uid),
>                     statusfolder.getmessageuidlist())
> 	...
> 	# delete messages from statusfolder that are either deleted, or
> 	# not being tracked because of maxage
> 	statusfolder.deletemessages(deletelist)
> 	deletelist = filter(lambda uid: dstfolder.uidexists(uid))
> 		self.ui.deletingmessages(deletelist, [dstfolder])
> 		dstfolder.deletemessages(deletelist)
> 
> but I'm not really confident that this is actually an improvement.

Well, that looks like the thing to do. It's really bug to have the UI
lying about what's happening.

> Also, even though I think messages are no longer being deleted when
> they shouldn't be, I can still cause messages to get copied when they
> already exist in the destination... I'll try to look into that some
> more.

This is because it is not reducing the messagelist on both sides.  Here
is the offending case (letters are messages, time +N define the hours of
the message around maxage):

    - N+0 == time.time() + maxage

    - Local has                   [A+2, B+1, Y-1     ]
    - IMAP has                    [A+2, B+1, Y-1, Z-2]

    - Local (maxage)     returns  [          Y-1     ]
    - Local (maxage + 1) returns  [A+2, B+1, Y-1     ]
    - SINCE (maxage)     returns  [               Z-2]
    - SINCE (maxage + 1) returns  [A+2, B+1, Y-1, Z-2]

Before the timezone fix:
    - Local (maxage)     returns  [          Y-1     ]
    - SINCE (maxage)     returns  [               Z-2]
=> FAILED: Y-1 is deleted on IMAP, .

With the current fix:
  In order to not delete B+1, we request with SINCE (maxage + 1):
    - Local (maxage)     returns  [          Y-1     ]
    - SINCE (maxage + 1) returns  [A+2, B+1, Y-1, Z-2]
  If after exclusion (still based on maxage computations), we get:
    - Local (maxage)     returns  [          Y-1     ]
    - SINCE (maxage    ) returns  [     B+1, Y-1, Z-2]
=> FAILED: B+1 is duplicated on local.

Relying on delicate computations (you've seen how it works...), is sad
because we have the UIDs.

What we want to do for safety is THIS.
  Request both sides with (maxage + 1):
    - Local (maxage + 1) returns  [A+2, B+1, Y-1     ]
    - SINCE (maxage + 1) returns  [A+2, B+1, Y-1, Z-2]
  First, compare UIDs and exclude messages on both sides within the
  range [(maxage)..(maxage + 1)]:
    - Local (maxage + 1) returns  [                  ]
    - SINCE (maxage + 1) returns  [               Z-2]
  AND AFTER, exclude whatever above maxage (to not get confused with
  whatever could remain around (maxage +1): J+23, K+25, etc:
    - Local (maxage + 1) returns  [                  ]
    - SINCE (maxage + 1) returns  [               Z-2]
=> GOOD.


Also, the documentation in offlineimap.conf has to be updated.

-- 
Nicolas Sebrecht