From 0f070236bd49fe00d57775a6c8411e2810834f82 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 29 May 2013 06:28:57 -0700 Subject: [PATCH] Add a `bin/files purge` workflow Summary: We can lose file data through various means; one reasonable way is if files get deleted from disk with 'local-disk' storage. If data goes missing, Ref T3265. Also, reduce some code duplication. Test Plan: Ran `bin/files purge`, `bin/files migrate`, `bin/files rebuild` with various args. Deleted a file with "local-disk" storage, ran `bin/files purge`, made sure it got picked up. Reviewers: dctrwatson, btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T3265 Differential Revision: https://secure.phabricator.com/D6068 --- scripts/files/manage_files.php | 3 +- src/__phutil_library_map__.php | 2 + ...bricatorFilesManagementMigrateWorkflow.php | 35 +--------- ...habricatorFilesManagementPurgeWorkflow.php | 69 +++++++++++++++++++ ...bricatorFilesManagementRebuildWorkflow.php | 37 ++-------- .../PhabricatorFilesManagementWorkflow.php | 43 ++++++++++++ .../files/storage/PhabricatorFile.php | 8 ++- 7 files changed, 131 insertions(+), 66 deletions(-) create mode 100644 src/applications/files/management/PhabricatorFilesManagementPurgeWorkflow.php diff --git a/scripts/files/manage_files.php b/scripts/files/manage_files.php index 36726de040..c1f276999c 100755 --- a/scripts/files/manage_files.php +++ b/scripts/files/manage_files.php @@ -17,8 +17,9 @@ $args->parseStandardArguments(); $workflows = array( new PhabricatorFilesManagementEnginesWorkflow(), new PhabricatorFilesManagementMigrateWorkflow(), - new PhutilHelpArgumentWorkflow(), new PhabricatorFilesManagementRebuildWorkflow(), + new PhabricatorFilesManagementPurgeWorkflow(), + new PhutilHelpArgumentWorkflow(), ); $args->parseWorkflows($workflows); diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index c7626bcc20..5c3e806b2a 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1014,6 +1014,7 @@ phutil_register_library_map(array( 'PhabricatorFilesConfigOptions' => 'applications/files/config/PhabricatorFilesConfigOptions.php', 'PhabricatorFilesManagementEnginesWorkflow' => 'applications/files/management/PhabricatorFilesManagementEnginesWorkflow.php', 'PhabricatorFilesManagementMigrateWorkflow' => 'applications/files/management/PhabricatorFilesManagementMigrateWorkflow.php', + 'PhabricatorFilesManagementPurgeWorkflow' => 'applications/files/management/PhabricatorFilesManagementPurgeWorkflow.php', 'PhabricatorFilesManagementRebuildWorkflow' => 'applications/files/management/PhabricatorFilesManagementRebuildWorkflow.php', 'PhabricatorFilesManagementWorkflow' => 'applications/files/management/PhabricatorFilesManagementWorkflow.php', 'PhabricatorFlag' => 'applications/flag/storage/PhabricatorFlag.php', @@ -2814,6 +2815,7 @@ phutil_register_library_map(array( 'PhabricatorFilesConfigOptions' => 'PhabricatorApplicationConfigOptions', 'PhabricatorFilesManagementEnginesWorkflow' => 'PhabricatorFilesManagementWorkflow', 'PhabricatorFilesManagementMigrateWorkflow' => 'PhabricatorFilesManagementWorkflow', + 'PhabricatorFilesManagementPurgeWorkflow' => 'PhabricatorFilesManagementWorkflow', 'PhabricatorFilesManagementRebuildWorkflow' => 'PhabricatorFilesManagementWorkflow', 'PhabricatorFilesManagementWorkflow' => 'PhutilArgumentWorkflow', 'PhabricatorFlag' => 'PhabricatorFlagDAO', diff --git a/src/applications/files/management/PhabricatorFilesManagementMigrateWorkflow.php b/src/applications/files/management/PhabricatorFilesManagementMigrateWorkflow.php index a5d8455957..62946a0079 100644 --- a/src/applications/files/management/PhabricatorFilesManagementMigrateWorkflow.php +++ b/src/applications/files/management/PhabricatorFilesManagementMigrateWorkflow.php @@ -41,39 +41,8 @@ final class PhabricatorFilesManagementMigrateWorkflow $engine = PhabricatorFile::buildEngine($engine_id); - if ($args->getArg('all')) { - if ($args->getArg('names')) { - throw new PhutilArgumentUsageException( - "Specify either a list of files or `--all`, but not both."); - } - $iterator = new LiskMigrationIterator(new PhabricatorFile()); - } else if ($args->getArg('names')) { - $iterator = array(); - - foreach ($args->getArg('names') as $name) { - $name = trim($name); - - $id = preg_replace('/^F/i', '', $name); - if (ctype_digit($id)) { - $file = id(new PhabricatorFile())->loadOneWhere( - 'id = %d', - $id); - if (!$file) { - throw new PhutilArgumentUsageException( - "No file exists with id '{$name}'."); - } - } else { - $file = id(new PhabricatorFile())->loadOneWhere( - 'phid = %d', - $name); - if (!$file) { - throw new PhutilArgumentUsageException( - "No file exists with PHID '{$name}'."); - } - } - $iterator[] = $file; - } - } else { + $iterator = $this->buildIterator($args); + if (!$iterator) { throw new PhutilArgumentUsageException( "Either specify a list of files to migrate, or use `--all` ". "to migrate all files."); diff --git a/src/applications/files/management/PhabricatorFilesManagementPurgeWorkflow.php b/src/applications/files/management/PhabricatorFilesManagementPurgeWorkflow.php new file mode 100644 index 0000000000..d807f906b5 --- /dev/null +++ b/src/applications/files/management/PhabricatorFilesManagementPurgeWorkflow.php @@ -0,0 +1,69 @@ +setName('purge') + ->setSynopsis('Delete files with missing data.') + ->setArguments( + array( + array( + 'name' => 'all', + 'help' => 'Update all files.', + ), + array( + 'name' => 'dry-run', + 'help' => 'Show what would be updated.', + ), + array( + 'name' => 'names', + 'wildcard' => true, + ), + )); + } + + public function execute(PhutilArgumentParser $args) { + $console = PhutilConsole::getConsole(); + + $iterator = $this->buildIterator($args); + if (!$iterator) { + throw new PhutilArgumentUsageException( + "Either specify a list of files to purge, or use `--all` ". + "to purge all files."); + } + + $is_dry_run = $args->getArg('dry-run'); + + foreach ($iterator as $file) { + $fid = 'F'.$file->getID(); + + try { + $file->loadFileData(); + $okay = true; + } catch (Exception $ex) { + $okay = false; + } + + if ($okay) { + $console->writeOut( + "%s: File data is OK, not purging.\n", + $fid); + } else { + if ($is_dry_run) { + $console->writeOut( + "%s: Would purge (dry run).\n", + $fid); + } else { + $console->writeOut( + "%s: Purging.\n", + $fid); + $file->delete(); + } + } + } + + return 0; + } +} diff --git a/src/applications/files/management/PhabricatorFilesManagementRebuildWorkflow.php b/src/applications/files/management/PhabricatorFilesManagementRebuildWorkflow.php index 430b4021e0..b937623db8 100644 --- a/src/applications/files/management/PhabricatorFilesManagementRebuildWorkflow.php +++ b/src/applications/files/management/PhabricatorFilesManagementRebuildWorkflow.php @@ -13,12 +13,6 @@ final class PhabricatorFilesManagementRebuildWorkflow 'name' => 'all', 'help' => 'Update all files.', ), - array( - 'name' => 'id', - 'wildcard' => true, - 'help' => 'Update the given file. You can specify this flag '. - 'multiple times.', - ), array( 'name' => 'dry-run', 'help' => 'Show what would be updated.', @@ -31,37 +25,18 @@ final class PhabricatorFilesManagementRebuildWorkflow 'name' => 'rebuild-dimensions', 'help' => 'Rebuild image dimension information.', ), + array( + 'name' => 'names', + 'wildcard' => true, + ), )); } public function execute(PhutilArgumentParser $args) { $console = PhutilConsole::getConsole(); - if ($args->getArg('all')) { - if ($args->getArg('id')) { - throw new PhutilArgumentUsageException( - "Specify either a list of files or `--all`, but not both."); - } - $iterator = new LiskMigrationIterator(new PhabricatorFile()); - } else if ($args->getArg('id')) { - $iterator = array(); - - foreach ($args->getArg('id') as $id) { - $id = trim($id); - - $id = preg_replace('/^F/i', '', $id); - if (ctype_digit($id)) { - $file = id(new PhabricatorFile())->loadOneWhere( - 'id = %d', - $id); - if (!$file) { - throw new PhutilArgumentUsageException( - "No file exists with ID '{$id}'."); - } - } - $iterator[] = $file; - } - } else { + $iterator = $this->buildIterator($args); + if (!$iterator) { throw new PhutilArgumentUsageException( "Either specify a list of files to update, or use `--all` ". "to update all files."); diff --git a/src/applications/files/management/PhabricatorFilesManagementWorkflow.php b/src/applications/files/management/PhabricatorFilesManagementWorkflow.php index 9e7eb3da1c..760e922b58 100644 --- a/src/applications/files/management/PhabricatorFilesManagementWorkflow.php +++ b/src/applications/files/management/PhabricatorFilesManagementWorkflow.php @@ -7,4 +7,47 @@ abstract class PhabricatorFilesManagementWorkflow return true; } + protected function buildIterator(PhutilArgumentParser $args) { + if ($args->getArg('all')) { + if ($args->getArg('names')) { + throw new PhutilArgumentUsageException( + "Specify either a list of files or `--all`, but not both."); + } + return new LiskMigrationIterator(new PhabricatorFile()); + } + + if ($args->getArg('names')) { + $iterator = array(); + + foreach ($args->getArg('names') as $name) { + $name = trim($name); + + $id = preg_replace('/^F/i', '', $name); + if (ctype_digit($id)) { + $file = id(new PhabricatorFile())->loadOneWhere( + 'id = %d', + $id); + if (!$file) { + throw new PhutilArgumentUsageException( + "No file exists with ID '{$name}'."); + } + } else { + $file = id(new PhabricatorFile())->loadOneWhere( + 'phid = %d', + $name); + if (!$file) { + throw new PhutilArgumentUsageException( + "No file exists with PHID '{$name}'."); + } + } + $iterator[] = $file; + } + + return $iterator; + } + + return null; + } + + } diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php index 1ef5ae5aed..281c2ff61b 100644 --- a/src/applications/files/storage/PhabricatorFile.php +++ b/src/applications/files/storage/PhabricatorFile.php @@ -429,7 +429,13 @@ final class PhabricatorFile extends PhabricatorFileDAO // If this is the only file using the storage, delete storage if (!$other_file) { $engine = $this->instantiateStorageEngine(); - $engine->deleteFile($this->getStorageHandle()); + try { + $engine->deleteFile($this->getStorageHandle()); + } catch (Exception $ex) { + // In the worst case, we're leaving some data stranded in a storage + // engine, which is fine. + phlog($ex); + } } return $ret;