Fix pipeline pauser edge case

After many days of this being in the back of my mind I thought of a
solution I liked to fix the edge case where someone just set a pipeline
and none of its builds have run.

Previously a pipeline in that state would be paused. This is no longer
the case. I added an additional clause to the query to only get
pipelines that were also last_updated more than 24hrs ago. 24hrs should
be long enough for someone to have created at least one build for their
pipeline.

Also made the integration test simpler, no more goroutine to continously
pause the pipeline. I also realized this was a race condition for any
test if the pauser was configured. This should make the pauser
friendlier to use.

Signed-off-by: Taylor Silva <tasilva@vmware.com>

atc/db/pipeline_pauser.go

@@ -33,6 +33,9 @@ func (p *pipelinePauser) PausePipelines(ctx context.Context, daysSinceLastBuild
 		sq.Eq{
 			"p.paused": false,
 		},
+		// subquery returns a list of pipelines who jobs ran WITHIN the range.
+		// These are the pipelines that SHOULD NOT be paused which we use to
+		// build our list of pipelines that SHOULD be paused
 		sq.Expr(`p.id NOT IN (SELECT j.pipeline_id FROM jobs j
 							LEFT JOIN builds b ON j.latest_completed_build_id = b.id
 							WHERE j.pipeline_id = p.id AND j.active = true
@@ -40,6 +43,9 @@ func (p *pipelinePauser) PausePipelines(ctx context.Context, daysSinceLastBuild
 								b.end_time > CURRENT_DATE - ?::INTERVAL
 							))`,
 			strconv.Itoa(daysSinceLastBuild)+" day"),
+		// Covers edge case where pipelines that were just set could be paused automatically
+		// Only pauses the pipeline if it was last updated more than 24hrs ago
+		sq.Expr(`p.last_updated < CURRENT_DATE - '1 day'::INTERVAL`),
 	}).RunWith(p.conn).Query()
 
 	if err != nil {

atc/db/pipeline_pauser_test.go

@@ -40,6 +40,9 @@ var _ = Describe("PipelinePauser", func() {
 					twoJobPipeline, _, err = defaultTeam.SavePipeline(pipelineRef, twoJobPipelineConfig, db.ConfigVersion(0), false)
 					Expect(err).NotTo(HaveOccurred())
 					Expect(twoJobPipeline.Paused()).To(BeFalse(), "pipeline should start unpaused")
+					By("making it look like the pipeline was set 15 days ago as well")
+					_, err = dbConn.Exec(`UPDATE pipelines SET last_updated = NOW() - INTERVAL '15' DAY WHERE id = $1`, twoJobPipeline.ID())
+					Expect(err).NotTo(HaveOccurred())
 
 					By("creating a job that ran 15 days ago")
 					jobOne, found, err := twoJobPipeline.Job("job-one")
@@ -70,6 +73,9 @@ var _ = Describe("PipelinePauser", func() {
 					twoJobPipeline, _, err = defaultTeam.SavePipeline(pipelineRef, twoJobPipelineConfig, db.ConfigVersion(0), false)
 					Expect(err).NotTo(HaveOccurred())
 					Expect(twoJobPipeline.Paused()).To(BeFalse(), "pipeline should start unpaused")
+					By("making it look like the pipeline was set 15 days ago as well")
+					_, err = dbConn.Exec(`UPDATE pipelines SET last_updated = NOW() - INTERVAL '15' DAY WHERE id = $1`, twoJobPipeline.ID())
+					Expect(err).NotTo(HaveOccurred())
 
 					By("creating a job that ran 15 days ago")
 					jobOne, found, err := twoJobPipeline.Job("job-one")
@@ -105,6 +111,9 @@ var _ = Describe("PipelinePauser", func() {
 			It("should say the pipeline was paused by 'automatic-pipeline-pauser'", func() {
 				By("using the default pipeline with one job")
 				Expect(defaultPipeline.Paused()).To(BeFalse(), "pipeline should start unpaused")
+				By("making it look like the pipeline was set 20 days ago as well")
+				_, err = dbConn.Exec(`UPDATE pipelines SET last_updated = NOW() - INTERVAL '20' DAY WHERE id = $1`, defaultPipeline.ID())
+				Expect(err).NotTo(HaveOccurred())
 
 				By("creating a job that ran 20 days ago")
 				b1, err := defaultJob.CreateBuild(defaultBuildCreatedBy)
@@ -197,4 +206,20 @@ var _ = Describe("PipelinePauser", func() {
 			})
 		})
 	})
+	Describe("newly set pipeline whose jobs have no builds", func() {
+		It("should not be paused if all of its jobs have no builds", func() {
+			By("creating a new pipeline")
+			newPipeline, _, err := defaultTeam.SavePipeline(atc.PipelineRef{Name: "new-pipeline"}, defaultPipelineConfig, db.ConfigVersion(0), false)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(newPipeline.Paused()).To(BeFalse(), "pipeline should start unpaused")
+
+			By("running the pipeline pauser")
+			err = pauser.PausePipelines(context.TODO(), 10)
+			Expect(err).NotTo(HaveOccurred())
+
+			_, err = newPipeline.Reload()
+			Expect(err).To(BeNil())
+			Expect(newPipeline.Paused()).To(BeFalse(), "pipeline should NOT be paused")
+		})
+	})
 })

integration/pauser/pipeline_pauser_test.go

@@ -18,23 +18,8 @@ func TestPipelinePauser(t *testing.T) {
 	fly := flytest.Init(t, pastDc)
 	t.Run("set and run test pipeline", func(t *testing.T) {
 		fly.Run(t, "set-pipeline", "-p", "pauser-test", "-c", "pipelines/one-job.yml", "-n")
-		stopUnpausing := make(chan int)
-		go func() {
-			// sometimes the pipeline pauser pauses the pipeline even after we
-			// unpaused it. This workaround persistently unpauses the pipeline
-			// until the job is done running
-			for {
-				select {
-				case <-stopUnpausing:
-					return
-				default:
-					fly.Run(t, "unpause-pipeline", "-p", "pauser-test")
-					time.Sleep(10 * time.Second)
-				}
-			}
-		}()
+		fly.Run(t, "unpause-pipeline", "-p", "pauser-test")
 		fly.Run(t, "trigger-job", "-j", "pauser-test/one-job", "-w")
-		close(stopUnpausing)
 	})
 
 	presentDc := dctest.Init(t, "../docker-compose.yml", "overrides/pauser-config.yml", "overrides/short-pauser-interval.yml")