improve cancel to force cancel dangling stages

CHANGELOG.md

@@ -5,6 +5,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## Unreleased
+
+## [1.2.3] - 2019-07-30
 ### Added
 
 - disable github status for cron jobs
@@ -12,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- improve cancel logic for dangling stages, by [@bradrydzewski](https://github.com/bradrydzewski).
 - improve error when kubernetes malforms the port configuration, by [@bradrydzewski](https://github.com/bradrydzewski). [#2742](https://github.com/drone/drone/issues/2742).
 - copy parameters from parent build when promoting, by [@bradrydzewski](https://github.com/bradrydzewski). [#2748](https://github.com/drone/drone/issues/2748).
 

handler/api/repos/builds/cancel.go

@@ -16,7 +16,6 @@ package builds
 
 import (
 	"context"
-	"errors"
 	"net/http"
 	"strconv"
 	"time"
@@ -75,64 +74,61 @@ func HandleCancel(
 			return
 		}
 
-		if build.Status != core.StatusPending &&
-			build.Status != core.StatusRunning {
-			logger.FromRequest(r).
-				WithField("build", build.Number).
-				WithField("namespace", namespace).
-				WithField("name", name).
-				Debugln("api: cannot cancel completed build")
-			render.InternalError(w, errors.New("Build must be pending or running to cancel"))
-			return
-		}
+		done := build.Status != core.StatusPending &&
+			build.Status != core.StatusRunning
 
-		build.Status = core.StatusKilled
-		build.Finished = time.Now().Unix()
-		if build.Started == 0 {
-			build.Started = time.Now().Unix()
-		}
+		// do not cancel the build if the build status is
+		// complete. only cancel the build if the status is
+		// running or pending.
+		if !done {
+			build.Status = core.StatusKilled
+			build.Finished = time.Now().Unix()
+			if build.Started == 0 {
+				build.Started = time.Now().Unix()
+			}
 
-		err = builds.Update(r.Context(), build)
-		if err != nil {
-			logger.FromRequest(r).
-				WithError(err).
-				WithField("build", build.Number).
-				WithField("namespace", namespace).
-				WithField("name", name).
-				Warnln("api: cannot update build status to cancelled")
-			render.ErrorCode(w, err, http.StatusConflict)
-			return
-		}
-
-		err = scheduler.Cancel(r.Context(), build.ID)
-		if err != nil {
-			logger.FromRequest(r).
-				WithError(err).
-				WithField("build", build.Number).
-				WithField("namespace", namespace).
-				WithField("name", name).
-				Warnln("api: cannot signal cancelled build is complete")
-		}
-
-		user, err := users.Find(r.Context(), repo.UserID)
-		if err != nil {
-			logger.FromRequest(r).
-				WithError(err).
-				WithField("namespace", namespace).
-				WithField("name", name).
-				Debugln("api: cannot repository owner")
-		} else {
-			err := status.Send(r.Context(), user, &core.StatusInput{
-				Repo:  repo,
-				Build: build,
-			})
+			err = builds.Update(r.Context(), build)
 			if err != nil {
 				logger.FromRequest(r).
 					WithError(err).
 					WithField("build", build.Number).
 					WithField("namespace", namespace).
 					WithField("name", name).
-					Debugln("api: cannot set status")
+					Warnln("api: cannot update build status to cancelled")
+				render.ErrorCode(w, err, http.StatusConflict)
+				return
+			}
+
+			err = scheduler.Cancel(r.Context(), build.ID)
+			if err != nil {
+				logger.FromRequest(r).
+					WithError(err).
+					WithField("build", build.Number).
+					WithField("namespace", namespace).
+					WithField("name", name).
+					Warnln("api: cannot signal cancelled build is complete")
+			}
+
+			user, err := users.Find(r.Context(), repo.UserID)
+			if err != nil {
+				logger.FromRequest(r).
+					WithError(err).
+					WithField("namespace", namespace).
+					WithField("name", name).
+					Debugln("api: cannot repository owner")
+			} else {
+				err := status.Send(r.Context(), user, &core.StatusInput{
+					Repo:  repo,
+					Build: build,
+				})
+				if err != nil {
+					logger.FromRequest(r).
+						WithError(err).
+						WithField("build", build.Number).
+						WithField("namespace", namespace).
+						WithField("name", name).
+						Debugln("api: cannot set status")
+				}
 			}
 		}
 
@@ -201,16 +197,22 @@ func HandleCancel(
 			Debugln("api: successfully cancelled build")
 
 		build.Stages = stagez
-		payload := &core.WebhookData{
-			Event:  core.WebhookEventBuild,
-			Action: core.WebhookActionUpdated,
-			Repo:   repo,
-			Build:  build,
-		}
-		err = webhooks.Send(context.Background(), payload)
-		if err != nil {
-			logger.FromRequest(r).WithError(err).
-				Warnln("manager: cannot send global webhook")
+
+		// do not trigger a webhook if the build was already
+		// complete. only trigger a webhook if the build was
+		// pending or running and then cancelled.
+		if !done {
+			payload := &core.WebhookData{
+				Event:  core.WebhookEventBuild,
+				Action: core.WebhookActionUpdated,
+				Repo:   repo,
+				Build:  build,
+			}
+			err = webhooks.Send(context.Background(), payload)
+			if err != nil {
+				logger.FromRequest(r).WithError(err).
+					Warnln("manager: cannot send global webhook")
+			}
 		}
 
 		render.JSON(w, build, 200)

version/version.go

@@ -27,7 +27,7 @@ var (
 	// VersionMinor is for functionality in a backwards-compatible manner.
 	VersionMinor int64 = 2
 	// VersionPatch is for backwards-compatible bug fixes.
-	VersionPatch int64 = 2
+	VersionPatch int64 = 3
 	// VersionPre indicates prerelease.
 	VersionPre = ""
 	// VersionDev indicates development branch. Releases will be empty string.

version/version_test.go

@@ -9,7 +9,7 @@ package version
 import "testing"
 
 func TestVersion(t *testing.T) {
-	if got, want := Version.String(), "1.2.2"; got != want {
+	if got, want := Version.String(), "1.2.3"; got != want {
 		t.Errorf("Want version %s, got %s", want, got)
 	}
 }