CORRECT PATCH Re: sync_client bails when encountering a deleted message

[David Carter]

On Thu, 17 May 2007, Ken Murchison wrote:

> Here is an untested patch which SHOULD allow the client to abort the 
> UPLOAD, with the server successfully accepting the messages transmitted 
> up to the abort.
>
> Thoughts?

I believe that there is a problem with this patch. Sorry that its taken 
this long for me to look at it properly.

A single mailbox event can cover multiple messages, particularly if 
sync_client is catching up from a backlog of transactions. Imagine the 
second message in a sequence disappearing under sync_client's feet.

1) The first message will be replicated correctly.

2) The second message will be discarded, which is probably correctly.

3) The third (and any subsequent) messages will also be discarded.

The replica will remain out of sync until the next event on that mailbox. 
I think that it is important for sync_client to retry immediately.

Here is a patch against 2.3 CVS. This patch also causes the client and 
server to resynchronise from an incomplete UPLOAD command by throwing a 
syntax error at the server end. I'm uncomfortable about just dropping off 
part way through an UPLOAD and pretending that everything is fine.

-- 
David Carter                             Email: David.Carter at ucs.cam.ac.uk
University Computing Service,            Phone: (01223) 334502
New Museums Site, Pembroke Street,       Fax:   (01223) 334679
Cambridge UK. CB2 3QH.
-------------- next part --------------
Index: imap/sync_client.c
===================================================================
RCS file: /cvs/src/cyrus/imap/sync_client.c,v
retrieving revision 1.11
diff -u -r1.11 sync_client.c
--- imap/sync_client.c	18 May 2007 13:24:39 -0000	1.11
+++ imap/sync_client.c	2 Jul 2007 16:49:55 -0000
@@ -1132,7 +1132,6 @@
         prot_printf(toserver, " COPY");
         need_body = 0;
     } else {
-        sync_msgid_add(msgid_onserver, &record->uuid);
         prot_printf(toserver, " PARSED");
         need_body = 1;
     }
@@ -1177,9 +1176,9 @@
         if (r) {
             syslog(LOG_ERR, "IOERROR: opening message file %lu of %s: %m",
                    record->uid, mailbox->name);
-	    sync_msgid_remove(msgid_onserver, &record->uuid);
             return(IMAP_IOERROR);
         }
+        sync_msgid_add(msgid_onserver, &record->uuid);
 
         prot_printf(toserver, " %lu %lu %lu {%lu+}\r\n",
 		    record->header_size, record->content_lines,
@@ -1259,6 +1258,10 @@
     prot_printf(toserver, "\r\n"); 
     prot_flush(toserver);
 
+    if (r) {
+        sync_parse_code("UPLOAD", fromserver, SYNC_PARSE_EAT_OKLINE, NULL);
+        return(r);
+    }
     r = sync_parse_code("UPLOAD", fromserver, SYNC_PARSE_NOEAT_OKLINE, NULL);
     if (r) return(r);
 
@@ -1331,6 +1334,10 @@
     prot_printf(toserver, "\r\n"); 
     prot_flush(toserver);
 
+    if (r) {
+        sync_parse_code("UPLOAD", fromserver, SYNC_PARSE_EAT_OKLINE, NULL);
+        return(r);
+    }
     r = sync_parse_code("UPLOAD", fromserver, SYNC_PARSE_NOEAT_OKLINE, NULL);
     if (r) return(r);
 
Index: imap/sync_server.c
===================================================================
RCS file: /cvs/src/cyrus/imap/sync_server.c,v
retrieving revision 1.6
diff -u -r1.6 sync_server.c
--- imap/sync_server.c	30 May 2007 15:04:12 -0000	1.6
+++ imap/sync_server.c	2 Jul 2007 16:49:55 -0000
@@ -1870,7 +1870,7 @@
         switch (msg_type) {
         case MSG_SIMPLE:
             if (c != ' ') {
-                err = "Invalid flags";
+                err = "Invalid flags or missing message";
                 goto parse_err;
             }
 
@@ -1897,9 +1897,8 @@
             break;
         case MSG_PARSED:
             if (c != ' ') {
-		/* Missing message - UPLOAD short-circuited by client */
-		sync_upload_list_remove(upload_list, item);
-		goto done;
+                err = "Invalid flags or missing message";
+                goto parse_err;
             }
 
             message = sync_message_add(message_list, &item->uuid);