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.

Don't report search indexing errors to the daemon log except from "bin/search index"

Summary:
Depends on D20177. Fixes T12425. See <https://discourse.phabricator-community.org/t/importing-libphutil-repository-on-fresh-phabricator-triggers-an-error/2391/>.

Search indexing currently reports failures to load objects to the log. This log is noisy, not concerning, not actionable, and not locally debuggable (it depends on the reporting user's entire state).

I think one common, fully legitimate case of this is indexing temporary files: they may fully legitimately be deleted by the time the indexer runs.

Instead of sending these errors to the log, eat them. If users don't notice the indexes aren't working: no harm, no foul. If users do notice, we'll run or have them run `bin/search index` as a first diagnostic step anyway, which will now report an actionable/reproducible error.

Test Plan:
- Faked errors in both cases (initial load, re-load inside the locked section).
- Ran indexes in strict/non-strict mode.
- Got exception reports from both branches in strict mode.
- Got task success without errors in both cases in non-strict mode.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12425

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

epriestley 312ba307 aa470d21

+51 -13
+8 -1
src/applications/search/management/PhabricatorSearchManagementIndexWorkflow.php
··· 116 116 // them a hint that they might want to use "--force". 117 117 $track_skips = (!$is_background && !$is_force); 118 118 119 + // Activate "strict" error reporting if we're running in the foreground 120 + // so we'll report a wider range of conditions as errors. 121 + $is_strict = !$is_background; 122 + 119 123 $count_updated = 0; 120 124 $count_skipped = 0; 121 125 ··· 125 129 $old_versions = $this->loadIndexVersions($phid); 126 130 } 127 131 128 - PhabricatorSearchWorker::queueDocumentForIndexing($phid, $parameters); 132 + PhabricatorSearchWorker::queueDocumentForIndexing( 133 + $phid, 134 + $parameters, 135 + $is_strict); 129 136 130 137 if ($track_skips) { 131 138 $new_versions = $this->loadIndexVersions($phid);
+43 -12
src/applications/search/worker/PhabricatorSearchWorker.php
··· 2 2 3 3 final class PhabricatorSearchWorker extends PhabricatorWorker { 4 4 5 - public static function queueDocumentForIndexing($phid, $parameters = null) { 5 + public static function queueDocumentForIndexing( 6 + $phid, 7 + $parameters = null, 8 + $is_strict = false) { 9 + 6 10 if ($parameters === null) { 7 11 $parameters = array(); 8 12 } ··· 12 16 array( 13 17 'documentPHID' => $phid, 14 18 'parameters' => $parameters, 19 + 'strict' => $is_strict, 15 20 ), 16 21 array( 17 22 'priority' => parent::PRIORITY_INDEX, ··· 23 28 $data = $this->getTaskData(); 24 29 $object_phid = idx($data, 'documentPHID'); 25 30 26 - $object = $this->loadObjectForIndexing($object_phid); 31 + // See T12425. By the time we run an indexing task, the object it indexes 32 + // may have been deleted. This is unusual, but not concerning, and failing 33 + // to index these objects is correct. 34 + 35 + // To avoid showing these non-actionable errors to users, don't report 36 + // indexing exceptions unless we're in "strict" mode. This mode is set by 37 + // the "bin/search index" tool. 38 + 39 + $is_strict = idx($data, 'strict', false); 40 + 41 + try { 42 + $object = $this->loadObjectForIndexing($object_phid); 43 + } catch (PhabricatorWorkerPermanentFailureException $ex) { 44 + if ($is_strict) { 45 + throw $ex; 46 + } else { 47 + return; 48 + } 49 + } 27 50 28 51 $engine = id(new PhabricatorIndexEngine()) 29 52 ->setObject($object); ··· 35 58 return; 36 59 } 37 60 38 - $key = "index.{$object_phid}"; 39 - $lock = PhabricatorGlobalLock::newLock($key); 61 + $lock = PhabricatorGlobalLock::newLock( 62 + 'index', 63 + array( 64 + 'objectPHID' => $object_phid, 65 + )); 40 66 41 67 try { 42 68 $lock->lock(1); ··· 48 74 throw new PhabricatorWorkerYieldException(15); 49 75 } 50 76 77 + $caught = null; 51 78 try { 52 79 // Reload the object now that we have a lock, to make sure we have the 53 80 // most current version. 54 81 $object = $this->loadObjectForIndexing($object->getPHID()); 55 82 56 83 $engine->setObject($object); 57 - 58 84 $engine->indexObject(); 59 85 } catch (Exception $ex) { 60 - $lock->unlock(); 86 + $caught = $ex; 87 + } 88 + 89 + // Release the lock before we deal with the exception. 90 + $lock->unlock(); 61 91 62 - if (!($ex instanceof PhabricatorWorkerPermanentFailureException)) { 63 - $ex = new PhabricatorWorkerPermanentFailureException( 92 + if ($caught) { 93 + if (!($caught instanceof PhabricatorWorkerPermanentFailureException)) { 94 + $caught = new PhabricatorWorkerPermanentFailureException( 64 95 pht( 65 96 'Failed to update search index for document "%s": %s', 66 97 $object_phid, 67 - $ex->getMessage())); 98 + $caught->getMessage())); 68 99 } 69 100 70 - throw $ex; 101 + if ($is_strict) { 102 + throw $caught; 103 + } 71 104 } 72 - 73 - $lock->unlock(); 74 105 } 75 106 76 107 private function loadObjectForIndexing($phid) {