diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -585,6 +585,8 @@ '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 @@ // 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 --- a/src/applications/harbormaster/engine/HarbormasterBuildEngine.php +++ b/src/applications/harbormaster/engine/HarbormasterBuildEngine.php @@ -406,43 +406,59 @@ $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) {