To elaborate, I have an Unstable trigger sent to email Recipients, Committers,
and Culprits. Person A is on the global list, and person B commits and breaks a
test. The build is now unstable and an email goes to A+B. Now person C commits,
and the test still fails because no one has fixed it yet. An email goes to A+C,
instead of A+B+C.

So it seems that include culprits isn't working, which is very useful to remind
the person who broke the test that it is affecting other committers and they
should fix it in a timely manner.

Can you update the latest email-ext (released today) and see if this is still a problem? Also turn on Hudson's built-in email notification with "Send separate e-mails to individuals who broke the build" checked, and see if it has the same problem (this also sends to build.getCulprits()).

Reminder to retest on current releases, thanks..

Still an issue with Hudson 1.341 and email-ext 2.4, indeed.

what SCM are you using? I wasn't able to reproduce this problem with a subversion project. In a project where email-ext isn't emailing the right people, access /job/JOBNAME/BUILDNUMBER/api/xml and look at the culprit info listed here.. is this list accurate? (email-ext should be using that list)

We are using SVN. Were you trying with sequential Unstable builds with different committers, not Failed builds? If I look at the api/xml it is indeed wrong there. The first unstable build it has the two committers as culprits (correct), but the next unstable build doesn't have those people but instead the two people that made commits in that revision set. It doesn't seem to be carrying over culprits.

What is your Hudson version and subversion plugin version?

Moving out of email-ext component, as core determines the culprit list and email-ext just uses that info from the build. If the remote API also has incorrect data, the problem is not in email-ext.

I did: failed build with personA, failed build with personB, unstable build
I'll try now with only unstables..

Hudson 1.341 with Subversion 1.8. I'll upgrade to 1.9 and test but I don't see anything relevant in the changes.

Yup, sequence of unstables shows the problem. Odd that unstable following a failed build does pull cuprits from that previous failed build, but doesn't pull from previous unstable.

Looks like "culprits" was coded to only be those causing failed builds (not unstable).. it has been that way since r4689 (Sep 2007). See the request in JENKINS-747.. no mention of unstable builds there.

So, should the culprits calculated by Hudson core be since last stable build (to catch committers for unstable builds) or since last "successful" build (which may be unstable.. so only tracking failed builds, or aborted I guess).

I guess it is obvious since I filed this bug but intuitively to me someone who broke tests is a "culprit".

I suppose it's a reasonable change to include those who broke the tests.

The only thing I wonder is if we can do something clever in situations where you have some tests regularly failing, then people who committed a change that didn't increase the failure count wouldn't be counted toward culprits. But I suppose it's not possible since it collapses multiple abstraction layers.

kohsuke commented:
>> The only thing I wonder is if we can do something clever in situations where you have some tests regularly failing, then people who committed a change that didn't increase the failure count wouldn't be counted toward culprits

JENKINS-5282 requests this very change.

Just as an update on this issue, I attempted to fix this locally and will commit if it works, though I'm not convinced as of yet.

Index: core/src/main/java/hudson/model/AbstractBuild.java
===================================================================
--- core/src/main/java/hudson/model/AbstractBuild.java	(revision 31234)
+++ core/src/main/java/hudson/model/AbstractBuild.java	(working copy)
@@ -263,7 +263,7 @@
             R p = getPreviousCompletedBuild();
             if (p !=null && isBuilding()) {
                 Result pr = p.getResult();
-                if (pr!=null && pr.isWorseThan(Result.UNSTABLE)) {
+                if (pr!=null && pr.isWorseOrEqualTo(Result.UNSTABLE)) {
                     // we are still building, so this is just the current latest information,
                     // but we seems to be failing so far, so inherit culprits from the previous build.
                     // isBuilding() check is to avoid recursion when loading data from old Hudson, which doesn't record

looks ok to me.

Alas, it doesn't seem to be working but I'm not positive. This area was really confusing, especially how the block I changed is only executed if the previous build is building it seems. If anyone with some more insight wants to give that function (getCulprits) a peek to see, it would be appreciated.

yes, that code is odd.. looks like it relies on the fact that getCulprits is called further down in AbstractBuild.java from post().. isBuilding() will be true in this call, right?

In the getCulprits()-method the line
Map <AbstractProject,AbstractBuild.DependencyChange> depmap = getDependencyChanges(getPreviousNotFailedBuild());
may be wrong.
I thing the
getPreviousNotFailedBuild()
must be replaced with
getPreviousSuccessfulBuild()
The getPreviousNotFailedBuild() provides the last unstable build. This can be the previous build. And then getDependencyChanges(..) provides only the dependency to the previous build and not to the last successful build. And the culprits from all previous unstable builds until to the last successful build will not be found.

for me it works.
here is the patch.

diff --git a/core/src/main/java/hudson/model/AbstractBuild.java b/core/src/main/java/hudson/model/AbstractBuild.java
index 1b776f1..b1f4144 100644
--- a/core/src/main/java/hudson/model/AbstractBuild.java
+++ b/core/src/main/java/hudson/model/AbstractBuild.java
@@ -278,7 +278,7 @@ public abstract class AbstractBuild<P extends AbstractProject<P,R>,R extends Abs
             if (upstreamCulprits) {
                 // If we have dependencies since the last successful build, add their authors to our list
                 if (getPreviousNotFailedBuild() != null) {
-                    Map <AbstractProject,AbstractBuild.DependencyChange> depmap = getDependencyChanges(getPreviousNotFailedBuild());
+                    Map <AbstractProject,AbstractBuild.DependencyChange> depmap = getDependencyChanges(getPreviousSuccessfulBuild());
                     for (AbstractBuild.DependencyChange dep : depmap.values()) {
                         for (AbstractBuild<?,?> b : dep.getBuilds()) {
                             for (Entry entry : b.getChangeSet()) {

has anyone tried my patch?
can the patch commited to hudson-core?

I notice you didn't get any response to the question "has anyone tried my patch?", so I guess you should release it and folks like me can give some feedback. I'm not in a position to test patches.

m211, that change is only in the "if (upstreamCulprits)" block.. have you tested the simple case where there is no upstream job?

yes it works also for the simple case without any upstream job.

Code changed in jenkins
User: marco
Path:
core/src/main/java/hudson/model/AbstractBuild.java
http://jenkins-ci.org/commit/jenkins/502219cf1b8be65f91d4ebd81e590558eb8c5b26
Log:
JENKINS-4617
we are looking for dependencies since the last successful build
and not since the last previous not failed build

Code changed in jenkins
User: Kohsuke Kawaguchi
Path:
changelog.html
core/src/main/java/hudson/model/AbstractBuild.java
test/src/main/java/org/jvnet/hudson/test/FakeChangeLogSCM.java
test/src/test/groovy/hudson/model/AbstractBuildTest.groovy
http://jenkins-ci.org/commit/jenkins/e761d80f4d53a0f368fa8e9fbb06f01db247f8cc
Log:
[FIXED JENKINS-4617]
this and the previous commit constitutes the fix.
Also added a test case.

Compare: https://github.com/jenkinsci/jenkins/compare/ad646cd...e761d80

Integrated in jenkins_main_trunk #683
JENKINS-4617
[FIXED JENKINS-4617]

Kohsuke Kawaguchi : 502219cf1b8be65f91d4ebd81e590558eb8c5b26
Files :

• core/src/main/java/hudson/model/AbstractBuild.java

Kohsuke Kawaguchi : e761d80f4d53a0f368fa8e9fbb06f01db247f8cc
Files :

• test/src/main/java/org/jvnet/hudson/test/FakeChangeLogSCM.java
• test/src/test/groovy/hudson/model/AbstractBuildTest.groovy
• core/src/main/java/hudson/model/AbstractBuild.java
• changelog.html

Code changed in jenkins
User: marco
Path:
core/src/main/java/hudson/model/AbstractBuild.java
http://jenkins-ci.org/commit/jenkins/502219cf1b8be65f91d4ebd81e590558eb8c5b26
Log:
JENKINS-4617
we are looking for dependencies since the last successful build
and not since the last previous not failed build

Code changed in jenkins
User: Kohsuke Kawaguchi
Path:
changelog.html
core/src/main/java/hudson/model/AbstractBuild.java
test/src/main/java/org/jvnet/hudson/test/FakeChangeLogSCM.java
test/src/test/groovy/hudson/model/AbstractBuildTest.groovy
http://jenkins-ci.org/commit/jenkins/e761d80f4d53a0f368fa8e9fbb06f01db247f8cc
Log:
[FIXED JENKINS-4617]
this and the previous commit constitutes the fix.
Also added a test case.

Code changed in jenkins
User: marco
Path:
core/src/main/java/hudson/model/AbstractBuild.java
http://jenkins-ci.org/commit/jenkins/502219cf1b8be65f91d4ebd81e590558eb8c5b26
Log:
JENKINS-4617
we are looking for dependencies since the last successful build
and not since the last previous not failed build

Code changed in jenkins
User: Kohsuke Kawaguchi
Path:
changelog.html
core/src/main/java/hudson/model/AbstractBuild.java
test/src/main/java/org/jvnet/hudson/test/FakeChangeLogSCM.java
test/src/test/groovy/hudson/model/AbstractBuildTest.groovy
http://jenkins-ci.org/commit/jenkins/e761d80f4d53a0f368fa8e9fbb06f01db247f8cc
Log:
[FIXED JENKINS-4617]
this and the previous commit constitutes the fix.
Also added a test case.