From db1bc7fd7f1f3c8dda98f9950e3463d0f2551f2c Mon Sep 17 00:00:00 2001 From: epriestley <git@epriestley.com> Date: Thu, 25 Jun 2015 10:05:37 -0700 Subject: [PATCH] Carefully avoid the harbormaster/differential race Summary: Ref T8650. This should stop the problem, but isn't a root cause fix. See discussion on the task. Test Plan: Made some local diffs, but this is a bit hard to reproduce reliably. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T8650 Differential Revision: https://secure.phabricator.com/D13441 --- .../editor/DifferentialTransactionEditor.php | 4 + .../engine/HarbormasterBuildEngine.php | 86 +++++++++++-------- 2 files changed, 55 insertions(+), 35 deletions(-) diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index fce8e423b4..c73bb53956 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -585,6 +585,8 @@ final class DifferentialTransactionEditor 'a race?')); } + // TODO: This can race with diff updates, particularly those from + // Harbormaster. See discussion in T8650. $diff->setRevisionID($object->getID()); $diff->save(); @@ -593,6 +595,8 @@ final class DifferentialTransactionEditor // the old (`null`) container. // TODO: This is a bit iffy, maybe we can find a cleaner approach? + // In particular, this could (rarely) be overwritten by Harbormaster + // workers. $table = new HarbormasterBuildable(); $conn_w = $table->establishConnection('w'); queryfx( diff --git a/src/applications/harbormaster/engine/HarbormasterBuildEngine.php b/src/applications/harbormaster/engine/HarbormasterBuildEngine.php index 71c14414e7..8d8c26818e 100644 --- a/src/applications/harbormaster/engine/HarbormasterBuildEngine.php +++ b/src/applications/harbormaster/engine/HarbormasterBuildEngine.php @@ -406,43 +406,59 @@ final class HarbormasterBuildEngine extends Phobject { $should_publish = $did_update && $new_status != HarbormasterBuildable::STATUS_BUILDING && !$buildable->getIsManualBuildable(); - if ($should_publish) { - $object = id(new PhabricatorObjectQuery()) - ->setViewer($viewer) - ->withPHIDs(array($buildable->getBuildablePHID())) - ->executeOne(); - if ($object instanceof PhabricatorApplicationTransactionInterface) { - $template = $object->getApplicationTransactionTemplate(); - if ($template) { - $template - ->setTransactionType(PhabricatorTransactions::TYPE_BUILDABLE) - ->setMetadataValue( - 'harbormaster:buildablePHID', - $buildable->getPHID()) - ->setOldValue($old_status) - ->setNewValue($new_status); - - $harbormaster_phid = id(new PhabricatorHarbormasterApplication()) - ->getPHID(); - - $daemon_source = PhabricatorContentSource::newForSource( - PhabricatorContentSource::SOURCE_DAEMON, - array()); - - $editor = $object->getApplicationTransactionEditor() - ->setActor($viewer) - ->setActingAsPHID($harbormaster_phid) - ->setContentSource($daemon_source) - ->setContinueOnNoEffect(true) - ->setContinueOnMissingFields(true); - - $editor->applyTransactions( - $object->getApplicationTransactionObject(), - array($template)); - } - } + if (!$should_publish) { + return; } + + $object = id(new PhabricatorObjectQuery()) + ->setViewer($viewer) + ->withPHIDs(array($buildable->getBuildablePHID())) + ->executeOne(); + if (!$object) { + return; + } + + if (!($object instanceof PhabricatorApplicationTransactionInterface)) { + return; + } + + // TODO: Publishing these transactions is causing a race. See T8650. + // We shouldn't be publishing to diffs anyway. + if ($object instanceof DifferentialDiff) { + return; + } + + $template = $object->getApplicationTransactionTemplate(); + if (!$template) { + return; + } + + $template + ->setTransactionType(PhabricatorTransactions::TYPE_BUILDABLE) + ->setMetadataValue( + 'harbormaster:buildablePHID', + $buildable->getPHID()) + ->setOldValue($old_status) + ->setNewValue($new_status); + + $harbormaster_phid = id(new PhabricatorHarbormasterApplication()) + ->getPHID(); + + $daemon_source = PhabricatorContentSource::newForSource( + PhabricatorContentSource::SOURCE_DAEMON, + array()); + + $editor = $object->getApplicationTransactionEditor() + ->setActor($viewer) + ->setActingAsPHID($harbormaster_phid) + ->setContentSource($daemon_source) + ->setContinueOnNoEffect(true) + ->setContinueOnMissingFields(true); + + $editor->applyTransactions( + $object->getApplicationTransactionObject(), + array($template)); } private function releaseAllArtifacts(HarbormasterBuild $build) {