recaptime.dev / phorge
@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

Fix an issue where migrating files could prematurely destroy duplicates

Summary:
Fixes T5912. When migrating files, we try to clean up the old data. However, this code isn't aware of reference counting, and unconditionally destroys the old data.

For example, if you migrate files `F1` and `F2` and they have the same data, we'll delete the shared data when we migrate `F1`. Then you'll get an error when you migrate `F2`.

Since this only affects duplicate files, it primarily hits default profile pictures, which are the most numerous duplicate files on most installs.

Test Plan:
- Verified that the theory was correct by uploading two copies of a file and migrating the first one, before applying the patch. The second one's data was nuked and it couldn't be migrated.
- Applied patch.
- Uploaded two copies of a new file, migrated the first one (no data deletion), migrated the second one (data correctly deleted).
- Uploaded two copies of another new file, `bin/remove destory'd` the first one (no data deletion), then did it to the second one (data correctly deleted).

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5912

Differential Revision: https://secure.phabricator.com/D10312

epriestley 66fa59d0 e5acdd85

+32 -15
+32 -15
src/applications/files/storage/PhabricatorFile.php
··· 318 318 $params); 319 319 320 320 $old_engine = $this->instantiateStorageEngine(); 321 + $old_identifier = $this->getStorageEngine(); 321 322 $old_handle = $this->getStorageHandle(); 322 323 323 324 $this->setStorageEngine($new_identifier); 324 325 $this->setStorageHandle($new_handle); 325 326 $this->save(); 326 327 327 - $old_engine->deleteFile($old_handle); 328 + $this->deleteFileDataIfUnused( 329 + $old_engine, 330 + $old_identifier, 331 + $old_handle); 328 332 329 333 return $this; 330 334 } ··· 438 442 $ret = parent::delete(); 439 443 $this->saveTransaction(); 440 444 441 - // Check to see if other files are using storage 442 - $other_file = id(new PhabricatorFile())->loadAllWhere( 443 - 'storageEngine = %s AND storageHandle = %s AND 444 - storageFormat = %s AND id != %d LIMIT 1', 445 + $this->deleteFileDataIfUnused( 446 + $this->instantiateStorageEngine(), 445 447 $this->getStorageEngine(), 446 - $this->getStorageHandle(), 447 - $this->getStorageFormat(), 448 - $this->getID()); 448 + $this->getStorageHandle()); 449 + 450 + return $ret; 451 + } 452 + 453 + 454 + /** 455 + * Destroy stored file data if there are no remaining files which reference 456 + * it. 457 + */ 458 + private function deleteFileDataIfUnused( 459 + PhabricatorFileStorageEngine $engine, 460 + $engine_identifier, 461 + $handle) { 462 + 463 + // Check to see if any files are using storage. 464 + $usage = id(new PhabricatorFile())->loadAllWhere( 465 + 'storageEngine = %s AND storageHandle = %s LIMIT 1', 466 + $engine_identifier, 467 + $handle); 449 468 450 - // If this is the only file using the storage, delete storage 451 - if (!$other_file) { 452 - $engine = $this->instantiateStorageEngine(); 469 + // If there are no files using the storage, destroy the actual storage. 470 + if (!$usage) { 453 471 try { 454 - $engine->deleteFile($this->getStorageHandle()); 472 + $engine->deleteFile($handle); 455 473 } catch (Exception $ex) { 456 474 // In the worst case, we're leaving some data stranded in a storage 457 - // engine, which is fine. 475 + // engine, which is not a big deal. 458 476 phlog($ex); 459 477 } 460 478 } 479 + } 461 480 462 - return $ret; 463 - } 464 481 465 482 public static function hashFileContent($data) { 466 483 return sha1($data);