Hi Rinrin.

The lockfile has nothing to do with Jenkins. It is merely a file, that the Jenkins Bash Build Step is waiting for, precisely NOT to need to use a fixed sleep time.

This is used to let specific builds of "Test_1" and "Test_2" finish in the desired order, by deleting the "lock files" in the appropriate order. The Bash build step looks like that:

#!bash

# Write out a lock file, that delays, until it is deleted
lck="LOCKFILE-${JOB_NAME}-${BUILD_NUMBER}"
echo "Creating file and waiting for deletion of it: $lck"
touch "$lck"

while [[ -e "$lck" ]]; do
    sleep 1
done

exit 1

It is easy to see, that this buildstep looks like a totally normal build, to both Jenkins and the Gerrit Trigger plugin. It's just a shell build step that is taking some user-defined amount of time.

Thus, the only change over a normal "sleep <time>" is that you, as the debugger, can select the exact order in which these build steps will return.

You can easily change this to a fixed timeout, but that way, you need to be either quick about re-triggering the builds, or make the timeout so long, that you're needlessly waiting for the tests to end.

In any case, the bug is fully and wholly independent of what the build step does. It only matters in which order they return and that the retriggering happens WHILE the previously triggered tests are still running.

Sorry, I missed attachment and fully understand.

BTW, it seems that this plugin has some useful logs around the related logic.
So I think you had better provide logs if you have.

I reproduced this on the latest plugin code but there is a bit difference with what you mentioned.

UPDATE:
The below results are in condition: jobs has one completed build as "#1".
So "#2" is triggered build by event, "#3" is triggered build by manual trigger.

This will let Test_2 finish successfully (as Test_2 always succeeds).
The bug that happens now, is that the trigger plugin uploads a "Verified +1" message, DESPITE "Test_1 #2" not yet being finished.

When LOCKFILE_Test_2-3 was removed, any message was not uploaded.

Next, LOCKFILE_Test_1-2 was removed. Then message "Verified -1" was uploaded from "Test_1 #2". Also message "Verified +1" was uploaded when LOCKFILE_Test_2-2 was removed.

Finally, LOCKFILE_Test_1-3 was removed. Then message "Verified -1" was uploaded with verdict value both "Test_1 #3" and "Test_2 #3". It means that uploading message for "Test_2 #3" was pended until "Test_1 #3" is completed.

It means that:

1. Running builds for event from Gerrit were deviated from verdict logic by manual triggered ones.
2. But these were still enabled as individual triggered build.

So I think that the issue you should address is #2. right?

Sorry, missed one condition. So I will update.

In case jobs has no builds(means started as #1), NPE was happened.

INFO: Test_1 #1 main build action completed: FAILURE
Aug 28, 2014 6:13:06 PM com.sonyericsson.hudson.plugins.gerrit.trigger.gerritnotifier.ToGerritRunListener onCompleted
INFO: Obtained failure message: null
Aug 28, 2014 6:13:06 PM com.sonyericsson.hudson.plugins.gerrit.trigger.gerritnotifier.model.BuildMemory setEntryFailureMessage
INFO: Recording unsuccessful message for PatchsetCreated: Change: 7 PatchSet: 28: null
Aug 28, 2014 6:13:06 PM com.sonyericsson.hudson.plugins.gerrit.trigger.gerritnotifier.ToGerritRunListener allBuildsCompleted
INFO: All Builds are completed for cause: GerritCause: PatchsetCreated: Change: 7 PatchSet: 28 silent: false
Aug 28, 2014 6:13:06 PM com.sonyericsson.hudson.plugins.gerrit.trigger.gerritnotifier.GerritNotifier buildCompleted
SEVERE: Could not complete BuildCompleted notification!
java.lang.NullPointerException
	at com.sonyericsson.hudson.plugins.gerrit.trigger.gerritnotifier.ParameterExpander.createBuildsStats(ParameterExpander.java:554)
	at com.sonyericsson.hudson.plugins.gerrit.trigger.gerritnotifier.ParameterExpander.getBuildCompletedCommand(ParameterExpander.java:485)
	at com.sonyericsson.hudson.plugins.gerrit.trigger.gerritnotifier.GerritNotifier.buildCompleted(GerritNotifier.java:113)
	at com.sonyericsson.hudson.plugins.gerrit.trigger.gerritnotifier.job.ssh.BuildCompletedCommandJob.run(BuildCompletedCommandJob.java:64)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:471)
	at java.util.concurrent.FutureTask.run(FutureTask.java:262)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
	at java.lang.Thread.run(Thread.java:745)

But I think the issue in previous comment is potential one for this NPE.

Hi Rin.

The main point of this issue is, that the Gerrit Trigger fails evaluating patch sets correctly. You can confuse it with the manual trigger so much, that it forgets that it needs to wait 2 tests to complete, before it can approve or deny a change.

Taking a look at the log in my original submission, the issue is as follows.

I uploaded the patch-set, and let the two tests continue running. The message uploaded by the plugin is correct so far:

-------------------------------------------------------
Jenkins
Patch Set 1: -Verified
Build Started Test_2/1/ (1/2)
-------------------------------------------------------
Jenkins
Patch Set 1:
Build Started Test_1/1/ (2/2)
-------------------------------------------------------

This is all normal behaviour. It notified the user of the start, and waits for the jobs to finish, before deciding on a verdict.

Now while those two jobs were still running, I started a "manual retrigger" on the same patch set, causing Gerrit Trigger to spawn two new builds:

-------------------------------------------------------
Jenkins
Patch Set 1:
Build Started Test_2/2/ (2/2)
-------------------------------------------------------
Jenkins
Patch Set 1:
Build Started Test_1/2/ (2/2)
-------------------------------------------------------

It did not need to do a "-Verified", as it knows that it is currently building the same patchset. If you have enabled the "Only build on newest patchset" feature, it even helpfully aborts the two previous jobs for you.

But there's something wrong here: It actually lost count of the number of tests it started. Instead of writing (1/2) & (2/2) – or maybe even (3/4) & (4/4) – it instead writes (2/2) twice. So it lost count.

This might not be so bad, but now, the bug strikes, when we deliberately let only "Test_2" finish, which always succeeds. Remember, Gerrit Trigger should wait for BOTH tests to finish.

-------------------------------------------------------
Jenkins
Patch Set 1: Verified+1
Build Successful 
Test_2/2/ : SUCCESS
-------------------------------------------------------

As you can see, it did not wait. It immediately uploaded a Verified+1. If you have any form of auto-commit in Gerrit, this is immediately deadly. It also allows users to (accidentally) subvert the verification process.

I will try to replicate this again later on my machine, and look if I also get an NPE.

This is not critical issue since only caused in rare condition. But it would be critical if once happens.

From code, It is certain that gerrit-trigger runs under the below assumptions:

1. A patchset-created event for a patchset in a change is generated only once
2. Only one build is scheduled from a job triggered by a event.

The event instance generated by Gerrit event stream is different with the one generated by manual trigger. But object comparison by equals() is true. This fact breaks assumption #1.

If manual trigger happens while triggered job has running build triggered by Gerrit stream event, a job has 2 builds generated by the same event. This breaks assumption #2.

So when manual trigger happens, plugin behavior is already unstable. Unfortunately, result would be undefined.

Perhaps this is design issue. So this could not be fixed easily with no plan.

Hi Rin.

I would consider it a critical issue, if

• a feature that is linked-to from the main page's side-panel,
• and activated by default for most users,
• is breaking the fundamental assumption, that the Gerrit Trigger will only update the review
  • once all tests submitted for a given patch-sets have passed.

I am also not sure, if your assumptions about how many builds should be scheduled for events are entirely valid.
After all, the Gerrit Trigger plugin has another way of retriggering all builds belonging to a given patch-set:

1. Navigate to any build started by the patch-set in question, once it has finished.
2. You will see, in the side-panel of that build, a "Retrigger All" button.

This button does – from the user perspective – the same thing as the "manual trigger" and it works absolutely correctly – because it can only be run, if all builds on that patch-set have finished.
That's the only reason it works. The manual trigger works, too, if you also wait until all builds for patch-set have finished.
Both features only break if you let them start concurrently with running tests.

But the problem is, that the manual trigger allows you to do that while other builds for the same patch-set are running. Since it does not check for this, it must work correctly even when builds are still running.
Otherwise, this feature is a direct way to break the entire purpose of the plugin.

And the nastiest thing: It breaks it without feedback. Nothing indicates that this behaviour breaks stuff. There isn't even an easy way to ensure that you can use that feature safely, as it not easy to find out which patch-sets are currently under test.

"Critical" is used to indicate issues resulting in crashes, loss of data, severe memory leaks.

Hi Daniel.

We originally set it to critical, because we consider corruption-of-data, by approving possibly unverified changes, to be pretty equivalent to loss-of-data.

I've bumped it down to "major" anyway, per your (and Rin's) recommendation.

We've simply disallowed the feature completely for all users, so it's not blocking us either.

Code changed in jenkins
User: rinrinne
Path:
src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ToGerritRunListener.java
src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/GerritTrigger.java
src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/GerritTriggerTest.java
src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/spec/SpecGerritTriggerHudsonTest.java
http://jenkins-ci.org/commit/gerrit-trigger-plugin/407fcae820ea9a3be7c8ed0f5f50545fb30bdff3
Log:
Don't trigger builds triggered by the same event

Now any builds are triggered even if existing builds triggerd by the same
event are running.

This patch prevents to trigger build according to the below policy:

• Project has triggered/running build triggered by the same event.
• The event trigger builds has still running build.

This would fix JENKINS-24445.

Fix for JENKINS-24445

Task-Url: https://issues.jenkins-ci.org/browse/JENKINS-24445