I've put a header on the patch describing the bug. Basically,
the result code from mailbox_open_locked() wasn't being tested
sufficiently, and hence the new mailbox name would be created
in mailboxes.db even though the files were no longer available
to be copied - causing sync bailouts and IOERRORs and all sorts
of fun.
What causes this? Clients that send a RENAME or DELETE event
and then you do something else in the GUI and they open a
_separate_ connection which tries to do something else to the
source folder.
I think it is our only remaining source of bailouts! It
requires a reconstruct to fix when it happens.
Patch attached, and the perl script I used to confirm that it
existed and confirm that this patch fixed it.
Regards,
Bron ( P.S. isn't it about time for a 2.3.12? I'm getting sick
of posting skiplist patches to people running the
lastest and having issues! )
#!/usr/bin/perl -w
use IO::Socket::INET;
$| = 1;
our $ADDR = '127.0.0.1:143';
our $USER = 'renametest@xxxxxxxxxxxxxxxx';
our $PASS = 'FIXME';
my $source = prepare_source();
my %h;
foreach my $n (1..3) {
my $pid = fork();
unless ($pid) {
my $res = do_rename($source, $n);
print "RES: $n => $res\n";
exit();
}
$h{$pid} = 1;
}
foreach my $pid (keys %h) {
waitpid($pid, 0);
}
dump_folders();
sub prepare_source {
my $sourcename = "INBOX.source$$";
my $fh = IO::Socket::INET->new($ADDR);
$fh->getline();
$fh->print(". login $USER $PASS\r\n");
$fh->getline();
$fh->print(". delete $sourcename\r\n"); # just in case!
$fh->getline();
$fh->print(". create $sourcename\r\n");
$fh->getline();
print "$sourcename: ";
foreach my $n (1..10000) {
my $msg = gen_msg($n);
my $len = length($msg);
$fh->print(". append $sourcename {$len}\r\n");
$fh->print("$msg\r\n");
print "." unless $n % 50;
}
print " done\n";
$fh->print(" . bye\r\n");
$fh->getline();
return $sourcename;
}
sub do_rename {
my $sourcename = shift;
my $n = shift;
my $fh = IO::Socket::INET->new($ADDR);
$fh->getline();
$fh->print(". login $USER $PASS\r\n");
$fh->getline();
$fh->print(". delete $sourcename-dest$n\r\n"); # just in case!
$fh->getline();
$fh->print(". rename $sourcename $sourcename-dest$n\r\n");
my $res = $fh->getline();
$fh->print(" . bye\r\n");
$fh->getline();
return $res;
}
sub gen_msg {
my $n = shift;
return qq{Subject: Message $n
From: <test1\@fastmailtest.com>
To: <tupp\@fastmailtest.com>
This is a test message made long by shoving a whole pile of lines on it!
} . ("$n\n" x 100000);
}
sub dump_folders {
my $fh = IO::Socket::INET->new($ADDR);
$fh->getline();
$fh->print(". login $USER $PASS\r\n");
$fh->getline();
$fh->print("TAG1 list \"INBOX.*\" \"*\"\r\n");
$fh->getline();
while (my $res = $fh->getline()) {
last if $res =~ m{^TAG1 };
print $res;
}
$fh->print(". bye\r\n");
$fh->getline();
}
Handle concurrent mailbox renames safely
*CANDIDATE FOR UPSTREAM*
About the last cause of sync bailouts at FastMail has been
the case where a user starts renaming a folder, and then does
something else (including deletes with renames here, since we
have the deleterename option enabled)
This was caused by the mailbox_rename_copy function not
checking the return code of mailbox_open_locked properly
(probably due to it being a huge function with multiple
different styles of using the if (!r) error checking
paradigm in different places, but my can be bothered on
major refactors is low at the moment)
The side effect of this was that if the mailbox existed
going in (copy was still underway) then the old mailbox
would get deleted (noop, or corruption if you don't have
all the skiplist patches applied!) and the new mailbox name
created but with no files in place!
This patch checks the return code of mailbox_open_locked
and aborts with an IO error if the mailbox has already been
locked.
Index: cyrus-imapd-2.3.11/imap/mboxlist.c
===================================================================
--- cyrus-imapd-2.3.11.orig/imap/mboxlist.c 2008-04-04 00:59:36.000000000 +0000
+++ cyrus-imapd-2.3.11/imap/mboxlist.c 2008-04-04 02:47:46.000000000 +0000
@@ -1339,11 +1339,15 @@ int mboxlist_renamemailbox(char *oldname
if(!r) {
r = mailbox_open_locked(oldname, oldpath, oldmpath, oldacl, auth_state,
&oldmailbox, 0);
- oldopen = 1;
+ if (r) {
+ goto done;
+ } else {
+ oldopen = 1;
+ }
}
/* 6. Copy mailbox */
- if (!r && !(mbtype & MBTYPE_REMOTE)) {
+ if (!(mbtype & MBTYPE_REMOTE)) {
/* Rename the actual mailbox */
r = mailbox_rename_copy(&oldmailbox, newname, newpartition,
NULL, NULL, &newmailbox,
----
Cyrus Home Page: http://cyrusimap.web.cmu.edu/
Cyrus Wiki/FAQ: http://cyrusimap.web.cmu.edu/twiki
List Archives/Info: http://asg.web.cmu.edu/cyrus/mailing-list.html
