bugfix: Skip pending status when no run is scheduled

github-pullrequest-plugin/src/main/java/com/github/kostyasha/github/integration/branch/trigger/JobRunnerForBranchCause.java

@@ -65,7 +65,12 @@ public boolean apply(GitHubBranchCause cause) {
 
             QueueTaskFuture<?> queueTaskFuture = startJob(cause);
             if (isNull(queueTaskFuture)) {
-                LOGGER.error("{} job didn't start", job.getFullName());
+                // See JobRunnerForCause: the schedule was refused (job not buildable, or vetoed
+                // by a Queue.QueueDecisionHandler), so no run will ever publish a final commit
+                // status and the PENDING pre-status below would stick on the commit forever.
+                LOGGER.error("{} job didn't start, skipping pending status for {}",
+                        job.getFullName(), cause.getCommitSha());
+                return false;
             }
 
             LOGGER.info(sb.toString());

github-pullrequest-plugin/src/main/java/org/jenkinsci/plugins/github/pullrequest/trigger/JobRunnerForCause.java

@@ -101,7 +101,17 @@ public boolean apply(final GitHubPRCause cause) {
 
             QueueTaskFuture<?> queueTaskFuture = startJob(cause);
             if (isNull(queueTaskFuture)) {
-                LOGGER.error("{} job didn't start", job.getFullName());
+                // ParameterizedJobMixIn.scheduleBuild2() returned null, which means the job is
+                // not buildable (disabled) or a Queue.QueueDecisionHandler vetoed the schedule.
+                // This is NOT the "identical item already queued" case: Queue.schedule2() returns
+                // the existing item there, so the future is non null and a run does happen.
+                // Here no run will ever exist to publish a final commit status, so the PENDING
+                // pre-status below must not be published. GitHub keeps only the newest status per
+                // context, so an orphan PENDING pins a required check to "pending" forever and
+                // permanently blocks the pull request.
+                LOGGER.error("{} job didn't start, skipping pending status for {}",
+                        job.getFullName(), cause.getHeadSha());
+                return false;
             }
 
             LOGGER.info(sb.toString());

github-pullrequest-plugin/src/test/java/com/github/kostyasha/github/integration/branch/trigger/PreStatusJobRunnerForBranchCauseTest.java

@@ -0,0 +1,110 @@
+package com.github.kostyasha.github.integration.branch.trigger;
+
+import com.coravy.hudson.plugins.github.GithubProjectProperty;
+import com.github.kostyasha.github.integration.branch.GitHubBranchCause;
+import com.github.kostyasha.github.integration.branch.GitHubBranchPollingLogAction;
+import com.github.kostyasha.github.integration.branch.GitHubBranchTrigger;
+import hudson.model.FreeStyleProject;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.kohsuke.github.GHCommitState;
+import org.kohsuke.github.GHRepository;
+
+import java.net.URL;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.commons.io.FileUtils.writeStringToFile;
+import static org.hamcrest.core.Is.is;
+import static org.junit.Assert.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoInteractions;
+import static org.mockito.Mockito.when;
+
+/**
+ * Branch counterpart of
+ * {@code org.jenkinsci.plugins.github.pullrequest.trigger.PreStatusJobRunnerForCauseTest}: the
+ * PENDING pre-status may only be published when a run was really scheduled, otherwise nothing ever
+ * overwrites it on the commit.
+ *
+ * @see JobRunnerForBranchCause#apply(GitHubBranchCause)
+ */
+public class PreStatusJobRunnerForBranchCauseTest {
+    private static final String COMMIT_SHA = "6dcb09b5b57875f334f61aebed695e2e4193db5e";
+    private static final String BRANCH = "master";
+
+    @Rule
+    public JenkinsRule j = new JenkinsRule();
+
+    private GHRepository remoteRepo;
+
+    @Before
+    public void setUp() {
+        remoteRepo = mock(GHRepository.class);
+    }
+
+    @Test
+    public void shouldNotPublishPendingStatusWhenJobDidNotStart() throws Exception {
+        FreeStyleProject job = j.createFreeStyleProject("disabled-project");
+        job.addProperty(new GithubProjectProperty("https://github.com/org/repo"));
+        // makes isBuildable() false, so ParameterizedJobMixIn.scheduleBuild2() returns null
+        job.disable();
+
+        GitHubBranchTrigger trigger = preStatusTrigger(job);
+
+        assertThat("cause shouldn't be reported as triggered",
+                new JobRunnerForBranchCause(job, trigger).apply(cause()), is(false));
+
+        verifyNoInteractions(remoteRepo);
+        assertThat("nothing should be queued", j.jenkins.getQueue().getItems().length, is(0));
+    }
+
+    /**
+     * Guards the assertion above: the very same setup does publish PENDING once a run is scheduled.
+     */
+    @Test
+    public void shouldPublishPendingStatusWhenJobStarted() throws Exception {
+        FreeStyleProject job = j.createFreeStyleProject("enabled-project");
+        job.addProperty(new GithubProjectProperty("https://github.com/org/repo"));
+        // keep the run in the queue so it can't finish and publish a final status under us
+        job.setQuietPeriod(1000);
+
+        GitHubBranchTrigger trigger = preStatusTrigger(job);
+
+        try {
+            assertThat("cause should be reported as triggered",
+                    new JobRunnerForBranchCause(job, trigger).apply(cause()), is(true));
+
+            verify(remoteRepo).createCommitStatus(eq(COMMIT_SHA), eq(GHCommitState.PENDING),
+                    any(), anyString(), eq(job.getFullName()));
+        } finally {
+            j.jenkins.getQueue().clear();
+        }
+    }
+
+    private GitHubBranchTrigger preStatusTrigger(FreeStyleProject job) throws Exception {
+        GitHubBranchPollingLogAction logAction = new GitHubBranchPollingLogAction(job);
+        // apply() reads the polling log into the cause, an absent file would make it return false
+        // for the wrong reason (IOException)
+        writeStringToFile(logAction.getPollingLogFile(), "", UTF_8);
+
+        GitHubBranchTrigger trigger = mock(GitHubBranchTrigger.class);
+        when(trigger.getPollingLogAction()).thenReturn(logAction);
+        when(trigger.isPreStatus()).thenReturn(true);
+        when(trigger.getRemoteRepository()).thenReturn(remoteRepo);
+        return trigger;
+    }
+
+    private static GitHubBranchCause cause() throws Exception {
+        GitHubBranchCause cause = new GitHubBranchCause(BRANCH, COMMIT_SHA);
+        // GitHubBranchBadgeAction dereferences it while scheduling
+        cause.withHtmlUrl(new URL("https://github.com/org/repo/tree/" + BRANCH));
+        cause.withReason("test");
+        return cause;
+    }
+}

github-pullrequest-plugin/src/test/java/org/jenkinsci/plugins/github/pullrequest/trigger/PreStatusJobRunnerForCauseTest.java

@@ -0,0 +1,106 @@
+package org.jenkinsci.plugins.github.pullrequest.trigger;
+
+import com.coravy.hudson.plugins.github.GithubProjectProperty;
+import hudson.model.FreeStyleProject;
+import org.jenkinsci.plugins.github.pullrequest.GitHubPRCause;
+import org.jenkinsci.plugins.github.pullrequest.GitHubPRPollingLogAction;
+import org.jenkinsci.plugins.github.pullrequest.GitHubPRTrigger;
+import org.junit.Before;
+import org.junit.Test;
+import org.kohsuke.github.GHCommitState;
+import org.kohsuke.github.GHRepository;
+
+import java.net.URL;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.commons.io.FileUtils.writeStringToFile;
+import static org.hamcrest.core.Is.is;
+import static org.jenkinsci.plugins.github.pullrequest.GitHubPRCause.newGitHubPRCause;
+import static org.junit.Assert.assertThat;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoInteractions;
+import static org.mockito.Mockito.when;
+
+/**
+ * The PENDING pre-status may only be published when a run was really scheduled. Without a run
+ * nothing ever overwrites it and, because GitHub keeps only the newest status per context, the
+ * commit stays pinned to "pending" forever.
+ *
+ * @see JobRunnerForCause#apply(GitHubPRCause)
+ */
+public class PreStatusJobRunnerForCauseTest extends JobRunnerForCauseTest {
+    private static final String HEAD_SHA = "6dcb09b5b57875f334f61aebed695e2e4193db5e";
+
+    private GHRepository remoteRepo;
+
+    @Before
+    public void setUp() {
+        remoteRepo = mock(GHRepository.class);
+    }
+
+    @Test
+    public void shouldNotPublishPendingStatusWhenJobDidNotStart() throws Exception {
+        FreeStyleProject job = j.createFreeStyleProject("disabled-project");
+        job.addProperty(new GithubProjectProperty("https://github.com/org/repo"));
+        // makes isBuildable() false, so ParameterizedJobMixIn.scheduleBuild2() returns null
+        job.disable();
+
+        GitHubPRTrigger trigger = preStatusTrigger(job);
+
+        assertThat("cause shouldn't be reported as triggered",
+                new JobRunnerForCause(job, trigger).apply(cause()), is(false));
+
+        verifyNoInteractions(remoteRepo);
+        assertThat("nothing should be queued", j.jenkins.getQueue().getItems().length, is(0));
+    }
+
+    /**
+     * Guards the assertion above: the very same setup does publish PENDING once a run is scheduled,
+     * so the test can't pass just because the status call was broken.
+     */
+    @Test
+    public void shouldPublishPendingStatusWhenJobStarted() throws Exception {
+        FreeStyleProject job = j.createFreeStyleProject("enabled-project");
+        job.addProperty(new GithubProjectProperty("https://github.com/org/repo"));
+        // keep the run in the queue so it can't finish and publish a final status under us
+        job.setQuietPeriod(1000);
+
+        GitHubPRTrigger trigger = preStatusTrigger(job);
+
+        try {
+            assertThat("cause should be reported as triggered",
+                    new JobRunnerForCause(job, trigger).apply(cause()), is(true));
+
+            verify(remoteRepo).createCommitStatus(eq(HEAD_SHA), eq(GHCommitState.PENDING),
+                    anyString(), anyString(), eq(job.getFullName()));
+        } finally {
+            j.jenkins.getQueue().clear();
+        }
+    }
+
+    private GitHubPRTrigger preStatusTrigger(FreeStyleProject job) throws Exception {
+        GitHubPRPollingLogAction logAction = new GitHubPRPollingLogAction(job);
+        // apply() reads the polling log into the cause, an absent file would make it return false
+        // for the wrong reason (IOException)
+        writeStringToFile(logAction.getPollingLogFile(), "", UTF_8);
+
+        GitHubPRTrigger trigger = mock(GitHubPRTrigger.class);
+        when(trigger.getPollingLogAction()).thenReturn(logAction);
+        when(trigger.isPreStatus()).thenReturn(true);
+        when(trigger.getRemoteRepository()).thenReturn(remoteRepo);
+        return trigger;
+    }
+
+    private static GitHubPRCause cause() throws Exception {
+        GitHubPRCause cause = newGitHubPRCause()
+                .withNumber(10)
+                .withHeadSha(HEAD_SHA);
+        // GitHubPREnv.URL dereferences it while filling build parameters
+        cause.withHtmlUrl(new URL("https://github.com/org/repo/pull/10"));
+        cause.withReason("test");
+        return cause;
+    }
+}