Avoid use of DB rollback() in LocalFileMoveBatch

* Verify the DB updates and bail before doing anything instead
  of relying on rollback() if something does not match up.
* Do the file copying before updating the DB so that there is
  nothing to rollback if they fail.
* Improved failCount in case the current version is missing.
  It should also reflect all the missing old versions.

Change-Id: Ie2d316548d8e5584cc69bb9f1425db108b05be5c

diff --git a/includes/filerepo/file/LocalFile.php b/includes/filerepo/file/LocalFile.php
index e35af75..f91026f 100644 (file)
--- a/includes/filerepo/file/LocalFile.php
+++ b/includes/filerepo/file/LocalFile.php
@@ -1973,16 +1973,6 @@ class LocalFile extends File {
                }
        }
 
-       /**
-        * Roll back the DB transaction and mark the image unlocked
-        */
-       function unlockAndRollback() {
-               $this->locked = false;
-               $dbw = $this->repo->getMasterDB();
-               $dbw->rollback( __METHOD__ );
-               $this->lockedOwnTrx = false;
-       }
-
        /**
         * @return Status
         */
@@ -2858,33 +2848,30 @@ class LocalFileMoveBatch {
        public function execute() {
                $repo = $this->file->repo;
                $status = $repo->newGood();
+               $destFile = wfLocalFile( $this->target );
+
+               $this->file->lock(); // begin
+               $destFile->lock(); // quickly fail if destination is not available
 
                $triplets = $this->getMoveTriplets();
                $checkStatus = $this->removeNonexistentFiles( $triplets );
                if ( !$checkStatus->isGood() ) {
-                       $status->merge( $checkStatus );
+                       $destFile->unlock();
+                       $this->file->unlock();
+                       $status->merge( $checkStatus ); // couldn't talk to file backend
                        return $status;
                }
                $triplets = $checkStatus->value;
-               $destFile = wfLocalFile( $this->target );
 
-               $this->file->lock(); // begin
-               $destFile->lock(); // quickly fail if destination is not available
-               // 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();
+               // Verify the file versions metadata in the DB.
+               $statusDb = $this->verifyDBUpdates();
                if ( !$statusDb->isGood() ) {
                        $destFile->unlock();
-                       $this->file->unlockAndRollback();
+                       $this->file->unlock();
                        $statusDb->ok = false;
 
                        return $statusDb;
                }
-               wfDebugLog( 'imagemove', "Renamed {$this->file->getName()} in database: " .
-                       "{$statusDb->successCount} successes, {$statusDb->failCount} failures" );
 
                if ( !$repo->hasSha1Storage() ) {
                        // Copy the files into their new location.
@@ -2897,7 +2884,7 @@ class LocalFileMoveBatch {
                                // Delete any files copied over (while the destination is still locked)
                                $this->cleanupTarget( $triplets );
                                $destFile->unlock();
-                               $this->file->unlockAndRollback(); // unlocks the destination
+                               $this->file->unlock();
                                wfDebugLog( 'imagemove', "Error in moving files: "
                                        . $statusMove->getWikiText( false, false, 'en' ) );
                                $statusMove->ok = false;
@@ -2907,6 +2894,12 @@ class LocalFileMoveBatch {
                        $status->merge( $statusMove );
                }
 
+               // Rename the file versions metadata in the DB.
+               $this->doDBUpdates();
+
+               wfDebugLog( 'imagemove', "Renamed {$this->file->getName()} in database: " .
+                       "{$statusDb->successCount} successes, {$statusDb->failCount} failures" );
+
                $destFile->unlock();
                $this->file->unlock(); // done
 
@@ -2919,33 +2912,62 @@ class LocalFileMoveBatch {
        }
 
        /**
-        * Do the database updates and return a new FileRepoStatus indicating how
-        * many rows where updated.
+        * Verify the database updates and return a new FileRepoStatus indicating how
+        * many rows would be updated.
         *
         * @return FileRepoStatus
         */
-       protected function doDBUpdates() {
+       protected function verifyDBUpdates() {
                $repo = $this->file->repo;
                $status = $repo->newGood();
                $dbw = $this->db;
 
-               // Update current image
-               $dbw->update(
+               $hasCurrent = $dbw->selectField(
                        'image',
-                       [ 'img_name' => $this->newName ],
+                       '1',
                        [ 'img_name' => $this->oldName ],
-                       __METHOD__
+                       __METHOD__,
+                       [ 'FOR UPDATE' ]
+               );
+               $oldRowCount = $dbw->selectField(
+                       'oldimage',
+                       'COUNT(*)',
+                       [ 'oi_name' => $this->oldName ],
+                       __METHOD__,
+                       [ 'FOR UPDATE' ]
                );
 
-               if ( $dbw->affectedRows() ) {
+               if ( $hasCurrent ) {
                        $status->successCount++;
                } else {
                        $status->failCount++;
-                       $status->fatal( 'imageinvalidfilename' );
-
-                       return $status;
                }
+               $status->successCount += $oldRowCount;
+               // Bug 34934: oldCount is based on files that actually exist.
+               // There may be more DB rows than such files, in which case $affected
+               // can be greater than $total. We use max() to avoid negatives here.
+               $status->failCount += max( 0, $this->oldCount - $oldRowCount );
+               if ( $status->failCount ) {
+                       $status->error( 'imageinvalidfilename' );
+               }
+
+               return $status;
+       }
+
+       /**
+        * Do the database updates and return a new FileRepoStatus indicating how
+        * many rows where updated.
+        */
+       protected function doDBUpdates() {
+               $dbw = $this->db;
 
+               // Update current image
+               $dbw->update(
+                       'image',
+                       [ 'img_name' => $this->newName ],
+                       [ 'img_name' => $this->oldName ],
+                       __METHOD__
+               );
                // Update old images
                $dbw->update(
                        'oldimage',
@@ -2957,19 +2979,6 @@ class LocalFileMoveBatch {
                        [ 'oi_name' => $this->oldName ],
                        __METHOD__
                );
-
-               $affected = $dbw->affectedRows();
-               $total = $this->oldCount;
-               $status->successCount += $affected;
-               // Bug 34934: $total is based on files that actually exist.
-               // There may be more DB rows than such files, in which case $affected
-               // can be greater than $total. We use max() to avoid negatives here.
-               $status->failCount += max( 0, $total - $affected );
-               if ( $status->failCount ) {
-                       $status->error( 'imageinvalidfilename' );
-               }
-
-               return $status;
        }
 
        /**