[PATCH] Race condition in svnsync can wedge a mirror repository

#! /bin/bash

# Create two repositories, $source_url and $target_url,
# with revprop changes enabled in $target_url.
#
# The "rm -rf" lines are commented out for safety; feel free to uncomment them
# if you want to run this script repeatedly
scratch_dir=`pwd`
#rm -rf $scratch_dir/source
#rm -rf $scratch_dir/target
svnadmin create $scratch_dir/source
svnadmin create $scratch_dir/target
echo '#! /bin/bash' >$scratch_dir/target/hooks/pre-revprop-change
chmod +x $scratch_dir/target/hooks/pre-revprop-change
source_url=file://$scratch_dir/source
target_url=file://$scratch_dir/target

# Set up svnsync between the two repositories
svnsync init $target_url $source_url

# Lock the target repo.  (We're pretending to be another instance of svnsync).
svn propset svn:sync-lock --revprop -r0 'test:3bd51d9e-7d3f-4e90-af25-78de900e46b5' $target_url

# Schedule an unlock for 8.5 seconds in the future.
(sleep 8.5 && svn propdel svn:sync-lock --revprop -r0 $target_url) &

# Run svnsync
svnsync sync $target_url

# Wait for the unlock process to complete.
for job in `jobs -p`; do
    wait $job
done

# Print results.
# NOTE: The lock should not exist.  svnsync should either take and release
# the lock, or never take it at all.
echo Checking if lock property exists...
svn propget svn:sync-lock --revprop -r0 $target_url

diff -urp orig/subversion-1.6.6/subversion/svnsync/main.c subversion-1.6.6/subversion/svnsync/main.c
--- orig/subversion-1.6.6/subversion/svnsync/main.c	2009-08-17 19:41:38.000000000 +0100
+++ subversion-1.6.6/subversion/svnsync/main.c	2009-11-25 17:02:50.000000000 +0000
@@ -235,7 +235,7 @@ get_lock(svn_ra_session_t *session, apr_
   svn_string_t *mylocktoken, *reposlocktoken;
   apr_status_t apr_err;
   apr_pool_t *subpool;
-  int i;
+  int i = 0;
 
   apr_err = apr_gethostname(hostname_str, sizeof(hostname_str), pool);
   if (apr_err)
@@ -246,7 +246,7 @@ get_lock(svn_ra_session_t *session, apr_
 
   subpool = svn_pool_create(pool);
 
-  for (i = 0; i < 10; ++i)
+  for (;;)
     {
       svn_pool_clear(subpool);
       SVN_ERR(check_cancel(NULL));
@@ -259,18 +259,39 @@ get_lock(svn_ra_session_t *session, apr_
           /* Did we get it?   If so, we're done, otherwise we sleep. */
           if (strcmp(reposlocktoken->data, mylocktoken->data) == 0)
             return SVN_NO_ERROR;
-          else
-            {
-              SVN_ERR(svn_cmdline_printf
-                      (pool, _("Failed to get lock on destination "
-                               "repos, currently held by '%s'\n"),
-                       reposlocktoken->data));
 
-              apr_sleep(apr_time_from_sec(1));
-            }
+          SVN_ERR(svn_cmdline_printf
+                  (pool, _("Failed to get lock on destination "
+                           "repos, currently held by '%s'\n"),
+                   reposlocktoken->data));
+
+          /* We check loop condition before we sleep, to
+           * avoid doing a pointless sleep at the end.
+           *
+           * XXX: If ++i==9 here then we cannot possibly take the lock.
+           * Even if it's released next time we look, we're going to
+           * bail out of the loop just before grabbing the lock.
+           * Is there any point in sleeping and going arond the loop
+           * again?  (Note that a fix for this should maybe wait for
+           * 1.7, it's probably not worth worrying about in 1.6.x).
+           */
+          ++i;
+          if (i >= 10)
+            break;
+
+          apr_sleep(apr_time_from_sec(1));
         }
       else
         {
+          /* We check loop condition before we try to take the lock,
+           * to avoid the bug where we take the lock then report
+           * a failure - that would permanently wedge the repository,
+           * requiring administrator intervention to fix.
+           */
+          ++i;
+          if (i >= 10)
+            break;
+
           SVN_ERR(svn_ra_change_rev_prop(session, 0, SVNSYNC_PROP_LOCK,
                                          mylocktoken, subpool));
         }