Skip to content

Commit 0f07023

Browse files
author
epriestley
committed
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
1 parent b91c863 commit 0f07023

File tree

7 files changed

+131
-66
lines changed

7 files changed

+131
-66
lines changed

‎scripts/files/manage_files.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,9 @@
1717
$workflows = array(
1818
new PhabricatorFilesManagementEnginesWorkflow(),
1919
new PhabricatorFilesManagementMigrateWorkflow(),
20-
new PhutilHelpArgumentWorkflow(),
2120
new PhabricatorFilesManagementRebuildWorkflow(),
21+
new PhabricatorFilesManagementPurgeWorkflow(),
22+
new PhutilHelpArgumentWorkflow(),
2223
);
2324

2425
$args->parseWorkflows($workflows);

‎src/__phutil_library_map__.php‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1014,6 +1014,7 @@
10141014
'PhabricatorFilesConfigOptions' => 'applications/files/config/PhabricatorFilesConfigOptions.php',
10151015
'PhabricatorFilesManagementEnginesWorkflow' => 'applications/files/management/PhabricatorFilesManagementEnginesWorkflow.php',
10161016
'PhabricatorFilesManagementMigrateWorkflow' => 'applications/files/management/PhabricatorFilesManagementMigrateWorkflow.php',
1017+
'PhabricatorFilesManagementPurgeWorkflow' => 'applications/files/management/PhabricatorFilesManagementPurgeWorkflow.php',
10171018
'PhabricatorFilesManagementRebuildWorkflow' => 'applications/files/management/PhabricatorFilesManagementRebuildWorkflow.php',
10181019
'PhabricatorFilesManagementWorkflow' => 'applications/files/management/PhabricatorFilesManagementWorkflow.php',
10191020
'PhabricatorFlag' => 'applications/flag/storage/PhabricatorFlag.php',
@@ -2814,6 +2815,7 @@
28142815
'PhabricatorFilesConfigOptions' => 'PhabricatorApplicationConfigOptions',
28152816
'PhabricatorFilesManagementEnginesWorkflow' => 'PhabricatorFilesManagementWorkflow',
28162817
'PhabricatorFilesManagementMigrateWorkflow' => 'PhabricatorFilesManagementWorkflow',
2818+
'PhabricatorFilesManagementPurgeWorkflow' => 'PhabricatorFilesManagementWorkflow',
28172819
'PhabricatorFilesManagementRebuildWorkflow' => 'PhabricatorFilesManagementWorkflow',
28182820
'PhabricatorFilesManagementWorkflow' => 'PhutilArgumentWorkflow',
28192821
'PhabricatorFlag' => 'PhabricatorFlagDAO',

‎src/applications/files/management/PhabricatorFilesManagementMigrateWorkflow.php‎

Lines changed: 2 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -41,39 +41,8 @@ public function execute(PhutilArgumentParser $args) {
4141

4242
$engine = PhabricatorFile::buildEngine($engine_id);
4343

44-
if ($args->getArg('all')) {
45-
if ($args->getArg('names')) {
46-
throw new PhutilArgumentUsageException(
47-
"Specify either a list of files or `--all`, but not both.");
48-
}
49-
$iterator = new LiskMigrationIterator(new PhabricatorFile());
50-
} else if ($args->getArg('names')) {
51-
$iterator = array();
52-
53-
foreach ($args->getArg('names') as $name) {
54-
$name = trim($name);
55-
56-
$id = preg_replace('/^F/i', '', $name);
57-
if (ctype_digit($id)) {
58-
$file = id(new PhabricatorFile())->loadOneWhere(
59-
'id = %d',
60-
$id);
61-
if (!$file) {
62-
throw new PhutilArgumentUsageException(
63-
"No file exists with id '{$name}'.");
64-
}
65-
} else {
66-
$file = id(new PhabricatorFile())->loadOneWhere(
67-
'phid = %d',
68-
$name);
69-
if (!$file) {
70-
throw new PhutilArgumentUsageException(
71-
"No file exists with PHID '{$name}'.");
72-
}
73-
}
74-
$iterator[] = $file;
75-
}
76-
} else {
44+
$iterator = $this->buildIterator($args);
45+
if (!$iterator) {
7746
throw new PhutilArgumentUsageException(
7847
"Either specify a list of files to migrate, or use `--all` ".
7948
"to migrate all files.");

‎src/applications/files/management/PhabricatorFilesManagementPurgeWorkflow.php‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
<?php
2+
3+
final class PhabricatorFilesManagementPurgeWorkflow
4+
extends PhabricatorFilesManagementWorkflow {
5+
6+
public function didConstruct() {
7+
$this
8+
->setName('purge')
9+
->setSynopsis('Delete files with missing data.')
10+
->setArguments(
11+
array(
12+
array(
13+
'name' => 'all',
14+
'help' => 'Update all files.',
15+
),
16+
array(
17+
'name' => 'dry-run',
18+
'help' => 'Show what would be updated.',
19+
),
20+
array(
21+
'name' => 'names',
22+
'wildcard' => true,
23+
),
24+
));
25+
}
26+
27+
public function execute(PhutilArgumentParser $args) {
28+
$console = PhutilConsole::getConsole();
29+
30+
$iterator = $this->buildIterator($args);
31+
if (!$iterator) {
32+
throw new PhutilArgumentUsageException(
33+
"Either specify a list of files to purge, or use `--all` ".
34+
"to purge all files.");
35+
}
36+
37+
$is_dry_run = $args->getArg('dry-run');
38+
39+
foreach ($iterator as $file) {
40+
$fid = 'F'.$file->getID();
41+
42+
try {
43+
$file->loadFileData();
44+
$okay = true;
45+
} catch (Exception $ex) {
46+
$okay = false;
47+
}
48+
49+
if ($okay) {
50+
$console->writeOut(
51+
"%s: File data is OK, not purging.\n",
52+
$fid);
53+
} else {
54+
if ($is_dry_run) {
55+
$console->writeOut(
56+
"%s: Would purge (dry run).\n",
57+
$fid);
58+
} else {
59+
$console->writeOut(
60+
"%s: Purging.\n",
61+
$fid);
62+
$file->delete();
63+
}
64+
}
65+
}
66+
67+
return 0;
68+
}
69+
}

‎src/applications/files/management/PhabricatorFilesManagementRebuildWorkflow.php‎

Lines changed: 6 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -13,12 +13,6 @@ public function didConstruct() {
1313
'name' => 'all',
1414
'help' => 'Update all files.',
1515
),
16-
array(
17-
'name' => 'id',
18-
'wildcard' => true,
19-
'help' => 'Update the given file. You can specify this flag '.
20-
'multiple times.',
21-
),
2216
array(
2317
'name' => 'dry-run',
2418
'help' => 'Show what would be updated.',
@@ -31,37 +25,18 @@ public function didConstruct() {
3125
'name' => 'rebuild-dimensions',
3226
'help' => 'Rebuild image dimension information.',
3327
),
28+
array(
29+
'name' => 'names',
30+
'wildcard' => true,
31+
),
3432
));
3533
}
3634

3735
public function execute(PhutilArgumentParser $args) {
3836
$console = PhutilConsole::getConsole();
3937

40-
if ($args->getArg('all')) {
41-
if ($args->getArg('id')) {
42-
throw new PhutilArgumentUsageException(
43-
"Specify either a list of files or `--all`, but not both.");
44-
}
45-
$iterator = new LiskMigrationIterator(new PhabricatorFile());
46-
} else if ($args->getArg('id')) {
47-
$iterator = array();
48-
49-
foreach ($args->getArg('id') as $id) {
50-
$id = trim($id);
51-
52-
$id = preg_replace('/^F/i', '', $id);
53-
if (ctype_digit($id)) {
54-
$file = id(new PhabricatorFile())->loadOneWhere(
55-
'id = %d',
56-
$id);
57-
if (!$file) {
58-
throw new PhutilArgumentUsageException(
59-
"No file exists with ID '{$id}'.");
60-
}
61-
}
62-
$iterator[] = $file;
63-
}
64-
} else {
38+
$iterator = $this->buildIterator($args);
39+
if (!$iterator) {
6540
throw new PhutilArgumentUsageException(
6641
"Either specify a list of files to update, or use `--all` ".
6742
"to update all files.");

‎src/applications/files/management/PhabricatorFilesManagementWorkflow.php‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,4 +7,47 @@ public function isExecutable() {
77
return true;
88
}
99

10+
protected function buildIterator(PhutilArgumentParser $args) {
11+
if ($args->getArg('all')) {
12+
if ($args->getArg('names')) {
13+
throw new PhutilArgumentUsageException(
14+
"Specify either a list of files or `--all`, but not both.");
15+
}
16+
return new LiskMigrationIterator(new PhabricatorFile());
17+
}
18+
19+
if ($args->getArg('names')) {
20+
$iterator = array();
21+
22+
foreach ($args->getArg('names') as $name) {
23+
$name = trim($name);
24+
25+
$id = preg_replace('/^F/i', '', $name);
26+
if (ctype_digit($id)) {
27+
$file = id(new PhabricatorFile())->loadOneWhere(
28+
'id = %d',
29+
$id);
30+
if (!$file) {
31+
throw new PhutilArgumentUsageException(
32+
"No file exists with ID '{$name}'.");
33+
}
34+
} else {
35+
$file = id(new PhabricatorFile())->loadOneWhere(
36+
'phid = %d',
37+
$name);
38+
if (!$file) {
39+
throw new PhutilArgumentUsageException(
40+
"No file exists with PHID '{$name}'.");
41+
}
42+
}
43+
$iterator[] = $file;
44+
}
45+
46+
return $iterator;
47+
}
48+
49+
return null;
50+
}
51+
52+
1053
}

‎src/applications/files/storage/PhabricatorFile.php‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -429,7 +429,13 @@ public function delete() {
429429
// If this is the only file using the storage, delete storage
430430
if (!$other_file) {
431431
$engine = $this->instantiateStorageEngine();
432-
$engine->deleteFile($this->getStorageHandle());
432+
try {
433+
$engine->deleteFile($this->getStorageHandle());
434+
} catch (Exception $ex) {
435+
// In the worst case, we're leaving some data stranded in a storage
436+
// engine, which is fine.
437+
phlog($ex);
438+
}
433439
}
434440

435441
return $ret;

0 commit comments

Comments
 (0)