From a26c6147f537e4703548252c629c5011b5d64a86 Mon Sep 17 00:00:00 2001
From: James Rhodes <jrhodes@redpointsoftware.com.au>
Date: Thu, 28 Aug 2014 08:21:36 +1000
Subject: [PATCH] Prevent artifact key collision when builds are restarted

Summary: Ref T1049.  Because we no longer destroy artifacts when builds are restarted, we need the build generation number to be part of the artifact key, otherwise we get collisions when restarting builds that contain build steps that emit artifacts.

Test Plan: Ran it with a build plan of "Lease Host" and "Run Command", no longer got an artifact key crash.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T1049

Differential Revision: https://secure.phabricator.com/D10336
---
 .../query/HarbormasterBuildArtifactQuery.php             | 9 +++++++--
 .../harbormaster/storage/build/HarbormasterBuild.php     | 6 +++++-
 .../storage/build/HarbormasterBuildArtifact.php          | 4 ++--
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/src/applications/harbormaster/query/HarbormasterBuildArtifactQuery.php b/src/applications/harbormaster/query/HarbormasterBuildArtifactQuery.php
index 79cee61a1b..077e77a4bd 100644
--- a/src/applications/harbormaster/query/HarbormasterBuildArtifactQuery.php
+++ b/src/applications/harbormaster/query/HarbormasterBuildArtifactQuery.php
@@ -8,6 +8,7 @@ final class HarbormasterBuildArtifactQuery
   private $artifactTypes;
   private $artifactKeys;
   private $keyBuildPHID;
+  private $keyBuildGeneration;
 
   public function withIDs(array $ids) {
     $this->ids = $ids;
@@ -24,8 +25,12 @@ final class HarbormasterBuildArtifactQuery
     return $this;
   }
 
-  public function withArtifactKeys($build_phid, array $artifact_keys) {
+  public function withArtifactKeys(
+    $build_phid,
+    $build_gen,
+    array $artifact_keys) {
     $this->keyBuildPHID = $build_phid;
+    $this->keyBuildGeneration = $build_gen;
     $this->artifactKeys = $artifact_keys;
     return $this;
   }
@@ -98,7 +103,7 @@ final class HarbormasterBuildArtifactQuery
       $indexes = array();
       foreach ($this->artifactKeys as $key) {
         $indexes[] = PhabricatorHash::digestForIndex(
-          $this->keyBuildPHID.$key);
+          $this->keyBuildPHID.$this->keyBuildGeneration.$key);
       }
 
       $where[] = qsprintf(
diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuild.php b/src/applications/harbormaster/storage/build/HarbormasterBuild.php
index e6849ac17d..2c4b4f9bab 100644
--- a/src/applications/harbormaster/storage/build/HarbormasterBuild.php
+++ b/src/applications/harbormaster/storage/build/HarbormasterBuild.php
@@ -213,7 +213,10 @@ final class HarbormasterBuild extends HarbormasterDAO
 
     $artifact =
       HarbormasterBuildArtifact::initializeNewBuildArtifact($build_target);
-    $artifact->setArtifactKey($this->getPHID(), $artifact_key);
+    $artifact->setArtifactKey(
+      $this->getPHID(),
+      $this->getBuildGeneration(),
+      $artifact_key);
     $artifact->setArtifactType($artifact_type);
     $artifact->save();
     return $artifact;
@@ -224,6 +227,7 @@ final class HarbormasterBuild extends HarbormasterDAO
       ->setViewer(PhabricatorUser::getOmnipotentUser())
       ->withArtifactKeys(
         $this->getPHID(),
+        $this->getBuildGeneration(),
         array($name))
       ->executeOne();
     if ($artifact === null) {
diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuildArtifact.php b/src/applications/harbormaster/storage/build/HarbormasterBuildArtifact.php
index 74b85532bc..08608a746b 100644
--- a/src/applications/harbormaster/storage/build/HarbormasterBuildArtifact.php
+++ b/src/applications/harbormaster/storage/build/HarbormasterBuildArtifact.php
@@ -38,9 +38,9 @@ final class HarbormasterBuildArtifact extends HarbormasterDAO
     return $this->assertAttached($this->buildTarget);
   }
 
-  public function setArtifactKey($build_phid, $key) {
+  public function setArtifactKey($build_phid, $build_gen, $key) {
     $this->artifactIndex =
-      PhabricatorHash::digestForIndex($build_phid.$key);
+      PhabricatorHash::digestForIndex($build_phid.$build_gen.$key);
     $this->artifactKey = $key;
     return $this;
   }