fix expiration when syncing only new messages

this was broken by commit de6dc699 - we now iterated only over far-side
messages, which we don't necessarily load, unlike the near-side ones
(which we need to do to know their current importance).

fix by iterating over sync entries instead of messages, which basically
restores the pre-19128f15 state in that regard. the minor catch here is
that we now need an auxiliary array to sort the sync entries by UIDs. on
the upside, we can also use it to avoid repeated calculation of the
flags.

src/run-tests.pl

@@ -1235,6 +1235,17 @@ my @X33 = (
 );
 test("max messages + expire - full", \@x33, \@X33, \@O31);
 
+my @O34 = ("", "", "Sync New\nMaxMessages 3\n");
+my @X34 = (
+  I, F, I,
+  B, "", "+~", "+T",
+  E, "", "+~", "+T",
+  F, "", "+~", "+T",
+  H, "", "*", "*",
+  I, "", "*", "*",
+);
+test("max messages + expire - new", \@x33, \@X34, \@O34);
+
 my @x38 = (
   F, C, 0,
   A, "*FS", "*FS", "*S",

src/sync.c

@@ -864,6 +864,21 @@ load_box( sync_vars_t *svars, int t, uint minwuid, uint_array_t mexcs )
 	svars->drv[t]->load_box( svars->ctx[t], minwuid, maxwuid, svars->finduid[t], pairuid, svars->maxuid[t], mexcs, box_loaded, AUX );
 }
 
+typedef struct {
+	sync_rec_t *srec;
+	uchar flags;
+} alive_srec_t;
+
+static int
+cmp_srec_far( const void *a, const void *b )
+{
+	uint au = (*(const alive_srec_t *)a).srec->uid[F];
+	uint bu = (*(const alive_srec_t *)b).srec->uid[F];
+	assert( au && bu );
+	assert( au != bu );
+	return au > bu ? 1 : -1;  // Can't subtract, the result might not fit into signed int.
+}
+
 typedef struct {
 	void *aux;
 	sync_rec_t *srec;
@@ -883,7 +898,7 @@ box_loaded( int sts, message_t *msgs, int total_msgs, int recent_msgs, void *aux
 	sync_rec_t *srec, **srecmap;
 	message_t *tmsg;
 	flag_vars_t *fv;
-	int no[2], del[2], alive, todel;
+	int no[2], del[2];
 	uchar sflags, nflags, aflags, dflags;
 	uint hashsz, idx;
 
@@ -1238,79 +1253,68 @@ box_loaded( int sts, message_t *msgs, int total_msgs, int recent_msgs, void *aux
 		/* Expire excess messages. Important (flagged, unread, or unpropagated) messages
 		 * older than the first not expired message are not counted towards the total. */
 		debug( "preparing message expiration\n" );
-		// Due to looping only over the far side, we completely ignore unpaired
+		alive_srec_t *arecs = nfmalloc( sizeof(*arecs) * svars->nsrecs );
-		// near-side messages. This is correct, as we cannot expire them without
+		int alive = 0;
-		// data loss anyway; consequently, we also don't count them.
+		for (srec = svars->srecs; srec; srec = srec->next) {
-		// Note that we also ignore near-side messages we're currently propagating,
+			if (srec->status & S_DEAD)
-		// which delays expiration of some messages by one cycle. Otherwise, we'd have
-		// to sequence flag propagation after message propagation to avoid a race
-		// with 3rd-party expunging, and that seems unreasonably expensive.
-		alive = 0;
-		for (tmsg = svars->msgs[F]; tmsg; tmsg = tmsg->next) {
-			if (tmsg->status & M_DEAD)
 				continue;
-			// We ignore unpaired far-side messages, as there is obviously nothing
+			// We completely ignore unpaired near-side messages, as we cannot expire
-			// to expire in the first place.
+			// them without data loss; consequently, we also don't count them.
-			if (!(srec = tmsg->srec))
+			// Note that we also ignore near-side messages we're currently propagating,
+			// which delays expiration of some messages by one cycle. Otherwise, we'd
+			// have to sequence flag updating after message propagation to avoid a race
+			// with external expunging, and that seems unreasonably expensive.
+			if (!srec->uid[F])
 				continue;
 			if (!(srec->status & S_PENDING)) {
+				// We ignore unpaired far-side messages, as there is obviously nothing
+				// to expire in the first place.
 				if (!srec->msg[N])
-					continue;  // Already expired or skipped.
+					continue;
 				nflags = (srec->msg[N]->flags | srec->aflags[N]) & ~srec->dflags[N];
 			} else {
-				nflags = tmsg->flags;
+				nflags = srec->msg[F]->flags;
 			}
 			if (!(nflags & F_DELETED) || (srec->status & (S_EXPIRE | S_EXPIRED))) {
 				// The message is not deleted, or it is, but only due to being expired.
-				alive++;
+				arecs[alive++] = (alive_srec_t){ srec, nflags };
 			}
 		}
-		todel = alive - svars->chan->max_messages;
+		// Sort such that the messages which have been in the
+		// complete store longest expire first.
+		qsort( arecs, alive, sizeof(*arecs), cmp_srec_far );
+		int todel = alive - svars->chan->max_messages;
 		debug( "%d alive messages, %d excess - expiring\n", alive, todel );
-		alive = 0;
+		int unseen = 0;
-		for (tmsg = svars->msgs[F]; tmsg; tmsg = tmsg->next) {
+		for (int sri = 0; sri < alive; sri++) {
-			if (tmsg->status & M_DEAD)
+			srec = arecs[sri].srec;
-				continue;
+			nflags = arecs[sri].flags;
-			if (!(srec = tmsg->srec))
-				continue;
-			if (!(srec->status & S_PENDING)) {
-				if (!srec->msg[N])
-					continue;
-				nflags = (srec->msg[N]->flags | srec->aflags[N]) & ~srec->dflags[N];
-			} else {
-				nflags = tmsg->flags;
-			}
-			if (!(nflags & F_DELETED) || (srec->status & (S_EXPIRE|S_EXPIRED))) {
 			if ((nflags & F_FLAGGED) ||
-				    !((nflags & F_SEEN) || ((void)(todel > 0 && alive++), svars->chan->expire_unread > 0))) {
+			    !((nflags & F_SEEN) || ((void)(todel > 0 && unseen++), svars->chan->expire_unread > 0))) {
 				// Important messages are always fetched/kept.
 				debug( "  pair(%u,%u) is important\n", srec->uid[F], srec->uid[N] );
 				todel--;
 			} else if (todel > 0 ||
-				           ((srec->status & (S_EXPIRE|S_EXPIRED)) == (S_EXPIRE|S_EXPIRED)) ||
+			           ((srec->status & (S_EXPIRE | S_EXPIRED)) == (S_EXPIRE | S_EXPIRED)) ||
-				           ((srec->status & (S_EXPIRE|S_EXPIRED)) && (srec->msg[N]->flags & F_DELETED))) {
+			           ((srec->status & (S_EXPIRE | S_EXPIRED)) && (srec->msg[N]->flags & F_DELETED))) {
 				/* The message is excess or was already (being) expired. */
 				srec->status |= S_NEXPIRE;
 				debug( "  expiring pair(%u,%u)\n", srec->uid[F], srec->uid[N] );
 				todel--;
 			}
 		}
-		}
 		debug( "%d excess messages remain\n", todel );
-		if (svars->chan->expire_unread < 0 && alive * 2 > svars->chan->max_messages) {
+		if (svars->chan->expire_unread < 0 && unseen * 2 > svars->chan->max_messages) {
 			error( "%s: %d unread messages in excess of MaxMessages (%d).\n"
 			       "Please set ExpireUnread to decide outcome. Skipping mailbox.\n",
-			       svars->orig_name[N], alive, svars->chan->max_messages );
+			       svars->orig_name[N], unseen, svars->chan->max_messages );
 			svars->ret |= SYNC_FAIL;
 			cancel_sync( svars );
 			return;
 		}
-		for (srec = svars->srecs; srec; srec = srec->next) {
+		for (int sri = 0; sri < alive; sri++) {
-			if (srec->status & S_DEAD)
+			srec = arecs[sri].srec;
-				continue;
 			if (!(srec->status & S_PENDING)) {
-				if (!srec->msg[N])
-					continue;
 				uchar nex = (srec->status / S_NEXPIRE) & 1;
 				if (nex != ((srec->status / S_EXPIRED) & 1)) {
 					/* The record needs a state change ... */
@@ -1340,6 +1344,7 @@ box_loaded( int sts, message_t *msgs, int total_msgs, int recent_msgs, void *aux
 				}
 			}
 		}
+		free( arecs );
 	}
 
 	sync_ref( svars );