[PATCH 0/2] imap: fix some missing EXISTS responses

[PATCH 0/2] imap: fix some missing EXISTS responses

[Eric Wong]

I noticed there were still some cases mutt didn't pick up
messages that showed up and I think I've fixed the last of
them.

Been testing these throughout the day on public-inbox.org
and things seem good.

Eric Wong (2):
  imap: always send EXISTS on uo2m_extend
  imap: EXAMINE: avoid potential race conditions

 lib/PublicInbox/IMAP.pm | 90 ++++++++++++++++++++++-------------------
 lib/PublicInbox/Over.pm | 18 ++-------
 2 files changed, 52 insertions(+), 56 deletions(-)

[PATCH 1/2] imap: always send EXISTS on uo2m_extend

[Eric Wong]

Clients which are NOT in an IDLE state still need to be
notified of message existence.  Unlike the EXPUNGE response,
untagged EXISTS responses seem to be allowed at any time
according to RFC 3501.

We'll also perform uo2m_extend on the NOOP command, since
NOOP is the recommended command for message polling.
---
 lib/PublicInbox/IMAP.pm | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/lib/PublicInbox/IMAP.pm b/lib/PublicInbox/IMAP.pm
index 0a6993c64c4..888c9becfe0 100644
--- a/lib/PublicInbox/IMAP.pm
+++ b/lib/PublicInbox/IMAP.pm
@@ -190,8 +190,6 @@ sub cmd_capability ($$) {
 	'* '.capa($self)."\r\n$tag OK Capability done\r\n";
 }
 
-sub cmd_noop ($$) { "$_[1] OK Noop done\r\n" }
-
 # uo2m: UID Offset to MSN, this is an arrayref by default,
 # but uo2m_hibernate can compact and deduplicate it
 sub uo2m_ary_new ($) {
@@ -233,7 +231,7 @@ sub uo2m_pack ($) {
 
 # extend {uo2m} to account for new messages which arrived since
 # {uo2m} was created.
-sub uo2m_extend ($$) {
+sub uo2m_extend ($$;$) {
 	my ($self, $new_uid_max) = @_;
 	defined(my $uo2m = $self->{uo2m}) or
 		return($self->{uo2m} = uo2m_ary_new($self));
@@ -245,20 +243,30 @@ sub uo2m_extend ($$) {
 	++$beg;
 	my $uids = $self->{ibx}->over->uid_range($beg, $base + UID_SLICE);
 	my @tmp; # [$UID_OFFSET] => $MSN
+	my $write_method = $_[2] // 'msg_more';
 	if (ref($uo2m)) {
 		my $msn = $uo2m->[-1];
 		$tmp[$_ - $beg] = ++$msn for @$uids;
+		$self->$write_method("* $msn EXISTS\r\n");
 		push @$uo2m, @tmp;
 		$uo2m;
 	} else {
 		my $msn = unpack('S', substr($uo2m, -2, 2));
 		$tmp[$_ - $beg] = ++$msn for @$uids;
+		$self->$write_method("* $msn EXISTS\r\n");
 		$uo2m .= uo2m_pack(\@tmp);
 		my %dedupe = ($uo2m => undef);
 		$self->{uo2m} = (keys %dedupe)[0];
 	}
 }
 
+sub cmd_noop ($$) {
+	my ($self, $tag) = @_;
+	defined($self->{uid_base}) and
+		uo2m_extend($self, $self->{uid_base} + UID_SLICE);
+	\"$tag OK Noop done\r\n";
+}
+
 # the flexible version which works on scalars and array refs.
 # Must call uo2m_extend before this
 sub uid2msn ($$) {
@@ -295,14 +303,10 @@ sub msn_to_uid_range ($$) {
 # called by PublicInbox::InboxIdle
 sub on_inbox_unlock {
 	my ($self, $ibx) = @_;
-	my $old = uo2m_last_uid($self);
 	my $uid_end = $self->{uid_base} + UID_SLICE;
-	uo2m_extend($self, $uid_end);
+	uo2m_extend($self, $uid_end, 'write');
 	my $new = uo2m_last_uid($self);
-	if ($new > $old) {
-		my $msn = uid2msn($self, $new);
-		$self->write(\"* $msn EXISTS\r\n");
-	} elsif ($new == $uid_end) { # max exceeded $uid_end
+	if ($new == $uid_end) { # max exceeded $uid_end
 		# continue idling w/o inotify
 		my $sock = $self->{sock} or return;
 		$ibx->unsubscribe_unlock(fileno($sock));
@@ -329,14 +333,13 @@ sub cmd_idle ($$) {
 	my ($self, $tag) = @_;
 	# IDLE seems allowed by dovecot w/o a mailbox selected *shrug*
 	my $ibx = $self->{ibx} or return "$tag BAD no mailbox selected\r\n";
-	$self->{-idle_tag} = $tag;
-	my $max = $ibx->over->max;
 	my $uid_end = $self->{uid_base} + UID_SLICE;
+	uo2m_extend($self, $uid_end);
 	my $sock = $self->{sock} or return;
 	my $fd = fileno($sock);
+	$self->{-idle_tag} = $tag;
 	# only do inotify on most recent slice
-	if ($max < $uid_end) {
-		uo2m_extend($self, $uid_end);
+	if ($ibx->over->max < $uid_end) {
 		$ibx->subscribe_unlock($fd, $self);
 		$self->{imapd}->idler_start;
 	}

[PATCH 2/2] imap: EXAMINE: avoid potential race conditions

[Eric Wong]

We need to rely on num_highwater for UIDNEXT since the
highest `num' stored in over.sqlite3 may be rolled back
if the most recent messages were spam.

We also need to load the uo2m immediately on EXAMINE to ensure
EXISTS responses are always consistent with regard to future
updates.
---
 lib/PublicInbox/IMAP.pm | 61 ++++++++++++++++++++++-------------------
 lib/PublicInbox/Over.pm | 18 ++----------
 2 files changed, 36 insertions(+), 43 deletions(-)

diff --git a/lib/PublicInbox/IMAP.pm b/lib/PublicInbox/IMAP.pm
index 888c9becfe0..9ad74de837f 100644
--- a/lib/PublicInbox/IMAP.pm
+++ b/lib/PublicInbox/IMAP.pm
@@ -192,8 +192,8 @@ sub cmd_capability ($$) {
 
 # uo2m: UID Offset to MSN, this is an arrayref by default,
 # but uo2m_hibernate can compact and deduplicate it
-sub uo2m_ary_new ($) {
-	my ($self) = @_;
+sub uo2m_ary_new ($;$) {
+	my ($self, $exists) = @_;
 	my $base = $self->{uid_base};
 	my $uids = $self->{ibx}->over->uid_range($base + 1, $base + UID_SLICE);
 
@@ -202,6 +202,7 @@ sub uo2m_ary_new ($) {
 	my $msn = 0;
 	++$base;
 	$tmp[$_ - $base] = ++$msn for @$uids;
+	$$exists = $msn if $exists;
 	\@tmp;
 }
 
@@ -385,44 +386,48 @@ sub ensure_slices_exist ($$$) {
 	push @$l, map { qq[* LIST (\\HasNoChildren) "." $_\r\n] } @created;
 }
 
-sub inbox_lookup ($$) {
-	my ($self, $mailbox) = @_;
-	my ($ibx, $exists, $uidnext, $uid_base);
-	if ($mailbox =~ /\A(.+)\.([0-9]+)\z/) {
-		# old mail: inbox.comp.foo.$SLICE_IDX
-		my $mb_top = $1;
-		$uid_base = $2 * UID_SLICE;
-		$ibx = $self->{imapd}->{mailboxes}->{lc $mailbox} or return;
-		my $max;
-		($exists, $uidnext, $max) = $ibx->over->imap_status($uid_base,
-							$uid_base + UID_SLICE);
-		ensure_slices_exist($self->{imapd}, $ibx, $max);
-	} else { # check for dummy inboxes
-		$mailbox = lc $mailbox;
-		$ibx = $self->{imapd}->{mailboxes}->{$mailbox} or return;
-
+sub inbox_lookup ($$;$) {
+	my ($self, $mailbox, $examine) = @_;
+	my ($ibx, $exists, $uidmax, $uid_base) = (undef, 0, 0, 0);
+	$mailbox = lc $mailbox;
+	$ibx = $self->{imapd}->{mailboxes}->{$mailbox} or return;
+	my $over = $ibx->over;
+	if ($over != $ibx) { # not a dummy
+		$mailbox =~ /\.([0-9]+)\z/ or
+				die "BUG: unexpected dummy mailbox: $mailbox\n";
+		$uid_base = $1 * UID_SLICE;
+
+		# ->num_highwater caches for writers, so use ->meta_accessor
+		$uidmax = $ibx->mm->meta_accessor('num_highwater') // 0;
+		if ($examine) {
+			$self->{uid_base} = $uid_base;
+			$self->{ibx} = $ibx;
+			$self->{uo2m} = uo2m_ary_new($self, \$exists);
+		} else {
+			$exists = $over->imap_exists;
+		}
+		ensure_slices_exist($self->{imapd}, $ibx, $over->max);
+	} else {
+		if ($examine) {
+			$self->{uid_base} = $uid_base;
+			$self->{ibx} = $ibx;
+			delete $self->{uo2m};
+		}
 		# if "INBOX.foo.bar" is selected and "INBOX.foo.bar.0",
 		# check for new UID ranges (e.g. "INBOX.foo.bar.1")
 		if (my $z = $self->{imapd}->{mailboxes}->{"$mailbox.0"}) {
 			ensure_slices_exist($self->{imapd}, $z, $z->over->max);
 		}
-
-		$uid_base = $exists = 0;
-		$uidnext = 1;
 	}
-	($ibx, $exists, $uidnext, $uid_base);
+	($ibx, $exists, $uidmax + 1, $uid_base);
 }
 
 sub cmd_examine ($$$) {
 	my ($self, $tag, $mailbox) = @_;
-	my ($ibx, $exists, $uidnext, $base) = inbox_lookup($self, $mailbox);
-	return "$tag NO Mailbox doesn't exist: $mailbox\r\n" if !$ibx;
-	$self->{uid_base} = $base;
-	delete $self->{uo2m};
-
 	# XXX: do we need this? RFC 5162/7162
 	my $ret = $self->{ibx} ? "* OK [CLOSED] previous closed\r\n" : '';
-	$self->{ibx} = $ibx;
+	my ($ibx, $exists, $uidnext, $base) = inbox_lookup($self, $mailbox, 1);
+	return "$tag NO Mailbox doesn't exist: $mailbox\r\n" if !$ibx;
 	$ret .= <<EOF;
 * $exists EXISTS\r
 * $exists RECENT\r
diff --git a/lib/PublicInbox/Over.pm b/lib/PublicInbox/Over.pm
index 74c8fb86270..e5a980d59ea 100644
--- a/lib/PublicInbox/Over.pm
+++ b/lib/PublicInbox/Over.pm
@@ -248,25 +248,13 @@ SELECT MAX(num) FROM over WHERE num > 0
 	$sth->fetchrow_array // 0;
 }
 
-sub imap_status {
+sub imap_exists {
 	my ($self, $uid_base, $uid_end) = @_;
-	my $dbh = $self->connect;
-	my $sth = $dbh->prepare_cached(<<'', undef, 1);
+	my $sth = $self->connect->prepare_cached(<<'', undef, 1);
 SELECT COUNT(num) FROM over WHERE num > ? AND num <= ?
 
 	$sth->execute($uid_base, $uid_end);
-	my $exists = $sth->fetchrow_array;
-
-	$sth = $dbh->prepare_cached(<<'', undef, 1);
-SELECT MAX(num) + 1 FROM over WHERE num <= ?
-
-	$sth->execute($uid_end);
-	my $uidnext = $sth->fetchrow_array;
-
-	$sth = $dbh->prepare_cached(<<'', undef, 1);
-SELECT MAX(num) FROM over WHERE num > 0
-
-	($exists, $uidnext, $sth->fetchrow_array // 0);
+	$sth->fetchrow_array;
 }
 
 sub check_inodes {