From 04804e67d54e5da45c11ff43bc7d1a75f3882cda Mon Sep 17 00:00:00 2001
From: Aaron Schulz <aschulz@wikimedia.org>
Date: Sun, 3 Jun 2012 01:49:29 -0700
Subject: [PATCH] [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
---
 includes/filerepo/file/LocalFile.php | 34 +++++++++++++++++-----------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/includes/filerepo/file/LocalFile.php b/includes/filerepo/file/LocalFile.php
index cc66649fe6..673bf8e086 100644
--- 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] );
 		}
 
-- 
2.20.1