[FileRepo] Fixed file move data-loss race condition.

* Update the DB before copying the files over.
  This fixes a serious regession from 395c5907ff6191afa263e557ce44164e032e15b6.

Change-Id: Ibeec2cda839adecf28f4b944fa1cdb6e4eff7e3b

diff --git a/includes/filerepo/file/LocalFile.php b/includes/filerepo/file/LocalFile.php
index cc66649..673bf8e 100644 (file)
--- a/includes/filerepo/file/LocalFile.php
+++ b/includes/filerepo/file/LocalFile.php
@@ -2370,26 +2370,33 @@ class LocalFileMoveBatch {
                $triplets = $this->getMoveTriplets();
                $triplets = $this->removeNonexistentFiles( $triplets );
 
-               // Copy the files into their new location
-               $statusMove = $repo->storeBatch( $triplets );
-               wfDebugLog( 'imagemove', "Moved files for {$this->file->getName()}: {$statusMove->successCount} successes, {$statusMove->failCount} failures" );
-               if ( !$statusMove->isGood() ) {
-                       wfDebugLog( 'imagemove', "Error in moving files: " . $statusMove->getWikiText() );
-                       $this->cleanupTarget( $triplets );
-                       $statusMove->ok = false;
-                       return $statusMove;
-               }
-
                $this->file->lock(); // begin
+               // Rename the file versions metadata in the DB.
+               // This implicitly locks the destination file, which avoids race conditions.
+               // If we moved the files from A -> C before DB updates, another process could
+               // move files from B -> C at this point, causing storeBatch() to fail and thus
+               // cleanupTarget() to trigger. It would delete the C files and cause data loss.
                $statusDb = $this->doDBUpdates();
-               wfDebugLog( 'imagemove', "Renamed {$this->file->getName()} in database: {$statusDb->successCount} successes, {$statusDb->failCount} failures" );
                if ( !$statusDb->isGood() ) {
                        $this->file->unlockAndRollback();
-                       // Something went wrong with the DB updates, so remove the target files
-                       $this->cleanupTarget( $triplets );
                        $statusDb->ok = false;
                        return $statusDb;
                }
+               wfDebugLog( 'imagemove', "Renamed {$this->file->getName()} in database: {$statusDb->successCount} successes, {$statusDb->failCount} failures" );
+
+               // Copy the files into their new location.
+               // If a prior process fataled copying or cleaning up files we tolerate any
+               // of the existing files if they are identical to the ones being stored.
+               $statusMove = $repo->storeBatch( $triplets, FileRepo::OVERWRITE_SAME );
+               wfDebugLog( 'imagemove', "Moved files for {$this->file->getName()}: {$statusMove->successCount} successes, {$statusMove->failCount} failures" );
+               if ( !$statusMove->isGood() ) {
+                       // Delete any files copied over (while the destination is still locked)
+                       $this->cleanupTarget( $triplets );
+                       $this->file->unlockAndRollback(); // unlocks the destination
+                       wfDebugLog( 'imagemove', "Error in moving files: " . $statusMove->getWikiText() );
+                       $statusMove->ok = false;
+                       return $statusMove;
+               }
                $this->file->unlock(); // done
 
                // Everything went ok, remove the source files
@@ -2506,6 +2513,7 @@ class LocalFileMoveBatch {
                // Create dest pairs from the triplets
                $pairs = array();
                foreach ( $triplets as $triplet ) {
+                       // $triplet: (old source virtual URL, dst zone, dest rel)
                        $pairs[] = array( $triplet[1], $triplet[2] );
                }