diff --git a/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php b/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php --- a/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php +++ b/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php @@ -514,10 +514,16 @@ $repository->getID(), $commit_identifier); if ($conn_w->getAffectedRows()) { + $commit = $commit->loadOneWhere( + 'repositoryID = %d AND commitIdentifier = %s', + $repository->getID(), + $commit_identifier); + + // After reviving a commit, schedule new daemons for it. + $this->didDiscoverCommit($repository, $commit, $epoch); return; } - $commit->setRepositoryID($repository->getID()); $commit->setCommitIdentifier($commit_identifier); $commit->setEpoch($epoch); @@ -575,21 +581,7 @@ } $commit->saveTransaction(); - $this->insertTask($repository, $commit); - - queryfx( - $conn_w, - 'INSERT INTO %T (repositoryID, size, lastCommitID, epoch) - VALUES (%d, 1, %d, %d) - ON DUPLICATE KEY UPDATE - size = size + 1, - lastCommitID = - IF(VALUES(epoch) > epoch, VALUES(lastCommitID), lastCommitID), - epoch = IF(VALUES(epoch) > epoch, VALUES(epoch), epoch)', - PhabricatorRepository::TABLE_SUMMARY, - $repository->getID(), - $commit->getID(), - $epoch); + $this->didDiscoverCommit($repository, $commit, $epoch); if ($this->repairMode) { // Normally, the query should throw a duplicate key exception. If we @@ -614,6 +606,29 @@ } } + private function didDiscoverCommit( + PhabricatorRepository $repository, + PhabricatorRepositoryCommit $commit, + $epoch) { + + $this->insertTask($repository, $commit); + + // Update the repository summary table. + queryfx( + $commit->establishConnection('w'), + 'INSERT INTO %T (repositoryID, size, lastCommitID, epoch) + VALUES (%d, 1, %d, %d) + ON DUPLICATE KEY UPDATE + size = size + 1, + lastCommitID = + IF(VALUES(epoch) > epoch, VALUES(lastCommitID), lastCommitID), + epoch = IF(VALUES(epoch) > epoch, VALUES(epoch), epoch)', + PhabricatorRepository::TABLE_SUMMARY, + $repository->getID(), + $commit->getID(), + $epoch); + } + private function didDiscoverRefs(array $refs) { foreach ($refs as $ref) { $this->workingSet[$ref->getIdentifier()] = true; diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php --- a/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php +++ b/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php @@ -3,6 +3,10 @@ final class PhabricatorRepositoryCommitHeraldWorker extends PhabricatorRepositoryCommitParserWorker { + protected function getImportStepFlag() { + return PhabricatorRepositoryCommit::IMPORTED_HERALD; + } + public function getRequiredLeaseTime() { // Herald rules may take a long time to process. return phutil_units('4 hours in seconds'); @@ -12,6 +16,12 @@ PhabricatorRepository $repository, PhabricatorRepositoryCommit $commit) { + if ($this->shouldSkipImportStep()) { + // This worker has no followup tasks, so we can just bail out + // right away without queueing anything. + return; + } + // Reload the commit to pull commit data and audit requests. $commit = id(new DiffusionCommitQuery()) ->setViewer(PhabricatorUser::getOmnipotentUser()) diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php --- a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php +++ b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php @@ -3,14 +3,18 @@ final class PhabricatorRepositoryCommitOwnersWorker extends PhabricatorRepositoryCommitParserWorker { + protected function getImportStepFlag() { + return PhabricatorRepositoryCommit::IMPORTED_OWNERS; + } + protected function parseCommit( PhabricatorRepository $repository, PhabricatorRepositoryCommit $commit) { - $this->triggerOwnerAudits($repository, $commit); - - $commit->writeImportStatusFlag( - PhabricatorRepositoryCommit::IMPORTED_OWNERS); + if (!$this->shouldSkipImportStep()) { + $this->triggerOwnerAudits($repository, $commit); + $commit->writeImportStatusFlag($this->getImportStepFlag()); + } if ($this->shouldQueueFollowupTasks()) { $this->queueTask( diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitParserWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitParserWorker.php --- a/src/applications/repository/worker/PhabricatorRepositoryCommitParserWorker.php +++ b/src/applications/repository/worker/PhabricatorRepositoryCommitParserWorker.php @@ -26,6 +26,14 @@ pht('Commit "%s" does not exist.', $commit_id)); } + if ($commit->isUnreachable()) { + throw new PhabricatorWorkerPermanentFailureException( + pht( + 'Commit "%s" has been deleted: it is no longer reachable from '. + 'any ref.', + $commit_id)); + } + $this->commit = $commit; return $commit; @@ -44,6 +52,42 @@ return !idx($this->getTaskData(), 'only'); } + protected function getImportStepFlag() { + return null; + } + + final protected function shouldSkipImportStep() { + // If this step has already been performed and this is a "natural" task + // which was queued by the normal daemons, decline to do the work again. + // This mitigates races if commits are rapidly deleted and revived. + $flag = $this->getImportStepFlag(); + if (!$flag) { + // This step doesn't have an associated flag. + return false; + } + + $commit = $this->commit; + if (!$commit->isPartiallyImported($flag)) { + // This commit doesn't have the flag set yet. + return false; + } + + + if (!$this->shouldQueueFollowupTasks()) { + // This task was queued by administrative tools, so do the work even + // if it duplicates existing work. + return false; + } + + $this->log( + "%s\n", + pht( + 'Skipping import step; this step was previously completed for '. + 'this commit.')); + + return true; + } + abstract protected function parseCommit( PhabricatorRepository $repository, PhabricatorRepositoryCommit $commit); diff --git a/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php b/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php --- a/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php +++ b/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php @@ -3,6 +3,10 @@ abstract class PhabricatorRepositoryCommitChangeParserWorker extends PhabricatorRepositoryCommitParserWorker { + protected function getImportStepFlag() { + return PhabricatorRepositoryCommit::IMPORTED_CHANGE; + } + public function getRequiredLeaseTime() { // It can take a very long time to parse commits; some commits in the // Facebook repository affect many millions of paths. Acquire 24h leases. @@ -23,9 +27,15 @@ return; } - $results = $this->parseCommitChanges($repository, $commit); - if ($results) { - $this->writeCommitChanges($repository, $commit, $results); + if (!$this->shouldSkipImportStep()) { + $results = $this->parseCommitChanges($repository, $commit); + if ($results) { + $this->writeCommitChanges($repository, $commit, $results); + } + + $commit->writeImportStatusFlag($this->getImportStepFlag()); + + PhabricatorSearchWorker::queueDocumentForIndexing($commit->getPHID()); } $this->finishParse(); @@ -85,12 +95,6 @@ protected function finishParse() { $commit = $this->commit; - - $commit->writeImportStatusFlag( - PhabricatorRepositoryCommit::IMPORTED_CHANGE); - - PhabricatorSearchWorker::queueDocumentForIndexing($commit->getPHID()); - if ($this->shouldQueueFollowupTasks()) { $this->queueTask( 'PhabricatorRepositoryCommitOwnersWorker', diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php --- a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php +++ b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php @@ -3,42 +3,52 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker extends PhabricatorRepositoryCommitParserWorker { - abstract protected function parseCommitWithRef( - PhabricatorRepository $repository, - PhabricatorRepositoryCommit $commit, - DiffusionCommitRef $ref); + protected function getImportStepFlag() { + return PhabricatorRepositoryCommit::IMPORTED_MESSAGE; + } + + abstract protected function getFollowupTaskClass(); final protected function parseCommit( PhabricatorRepository $repository, PhabricatorRepositoryCommit $commit) { - $viewer = PhabricatorUser::getOmnipotentUser(); + if (!$this->shouldSkipImportStep()) { + $viewer = PhabricatorUser::getOmnipotentUser(); - $refs_raw = DiffusionQuery::callConduitWithDiffusionRequest( - $viewer, - DiffusionRequest::newFromDictionary( + $refs_raw = DiffusionQuery::callConduitWithDiffusionRequest( + $viewer, + DiffusionRequest::newFromDictionary( + array( + 'repository' => $repository, + 'user' => $viewer, + )), + 'diffusion.querycommits', array( - 'repository' => $repository, - 'user' => $viewer, - )), - 'diffusion.querycommits', - array( - 'repositoryPHID' => $repository->getPHID(), - 'phids' => array($commit->getPHID()), - 'bypassCache' => true, - 'needMessages' => true, - )); - - if (empty($refs_raw['data'])) { - throw new Exception( - pht( - 'Unable to retrieve details for commit "%s"!', - $commit->getPHID())); - } + 'repositoryPHID' => $repository->getPHID(), + 'phids' => array($commit->getPHID()), + 'bypassCache' => true, + 'needMessages' => true, + )); + + if (empty($refs_raw['data'])) { + throw new Exception( + pht( + 'Unable to retrieve details for commit "%s"!', + $commit->getPHID())); + } - $ref = DiffusionCommitRef::newFromConduitResult(head($refs_raw['data'])); + $ref = DiffusionCommitRef::newFromConduitResult(head($refs_raw['data'])); + $this->updateCommitData($ref); + } - $this->parseCommitWithRef($repository, $commit, $ref); + if ($this->shouldQueueFollowupTasks()) { + $this->queueTask( + $this->getFollowupTaskClass(), + array( + 'commitID' => $commit->getID(), + )); + } } final protected function updateCommitData(DiffusionCommitRef $ref) { diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryGitCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryGitCommitMessageParserWorker.php --- a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryGitCommitMessageParserWorker.php +++ b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryGitCommitMessageParserWorker.php @@ -3,20 +3,8 @@ final class PhabricatorRepositoryGitCommitMessageParserWorker extends PhabricatorRepositoryCommitMessageParserWorker { - protected function parseCommitWithRef( - PhabricatorRepository $repository, - PhabricatorRepositoryCommit $commit, - DiffusionCommitRef $ref) { - - $this->updateCommitData($ref); - - if ($this->shouldQueueFollowupTasks()) { - $this->queueTask( - 'PhabricatorRepositoryGitCommitChangeParserWorker', - array( - 'commitID' => $commit->getID(), - )); - } + protected function getFollowupTaskClass() { + return 'PhabricatorRepositoryGitCommitChangeParserWorker'; } } diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryMercurialCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryMercurialCommitMessageParserWorker.php --- a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryMercurialCommitMessageParserWorker.php +++ b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryMercurialCommitMessageParserWorker.php @@ -3,20 +3,8 @@ final class PhabricatorRepositoryMercurialCommitMessageParserWorker extends PhabricatorRepositoryCommitMessageParserWorker { - protected function parseCommitWithRef( - PhabricatorRepository $repository, - PhabricatorRepositoryCommit $commit, - DiffusionCommitRef $ref) { - - $this->updateCommitData($ref); - - if ($this->shouldQueueFollowupTasks()) { - $this->queueTask( - 'PhabricatorRepositoryMercurialCommitChangeParserWorker', - array( - 'commitID' => $commit->getID(), - )); - } + protected function getFollowupTaskClass() { + return 'PhabricatorRepositoryMercurialCommitChangeParserWorker'; } } diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositorySvnCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositorySvnCommitMessageParserWorker.php --- a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositorySvnCommitMessageParserWorker.php +++ b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositorySvnCommitMessageParserWorker.php @@ -3,20 +3,8 @@ final class PhabricatorRepositorySvnCommitMessageParserWorker extends PhabricatorRepositoryCommitMessageParserWorker { - protected function parseCommitWithRef( - PhabricatorRepository $repository, - PhabricatorRepositoryCommit $commit, - DiffusionCommitRef $ref) { - - $this->updateCommitData($ref); - - if ($this->shouldQueueFollowupTasks()) { - $this->queueTask( - 'PhabricatorRepositorySvnCommitChangeParserWorker', - array( - 'commitID' => $commit->getID(), - )); - } + protected function getFollowupTaskClass() { + return 'PhabricatorRepositorySvnCommitChangeParserWorker'; } }