Issue 847: Changeset erroneously MERGED, and DB error
Status:  New
Owner: ----
Reported by phil.hord, Feb 15, 2011
Affected Version:
2.1.6.1

What steps will reproduce the problem?
Detailed report follows.

What is the expected output? What do you see instead?

A patch was reportedly submitted to master, and the accused submitter (an admin with Force-push-branch permission and who is prone to typos) says he didn't do it.  The patch does not appear on origin/master.  But the changeset in Gerrit says it is "(Merged)".

Please provide any additional information below.

Could be a race condition.  Gerrit + Hudson + Users
Could be a database migration error.  Upgraded to 2.1.6.1 early this month.
Could be a configuration problem. Error-prone admin and new admins.

Database reports "OrmDuplicateKeyException: patch_set_approvals"

Clues in order of time:

15:20      I reviewed changeset 136 via Gerrit.  The first commit on this
           patch did not have a Change-Id: in the comment because the git hook was 
           not yet installed (relatively new repo getting bootstrapped when this change 
           was first created).  After the 3rd "new" changeset was created, I suggested 
           to the author that he might add the Change-Id to the next commit message.  I 
           do not know if he followed this advice.  I do not know if this advice was correct.
           This is probably a red herring.

15:23:07   Hudson reports a build is starting because of a different changeset (137) on 
           a different project.

15:23:??   The Hudson Gerrit plugin failed to build, possibly because this changeset is
           dependent on an abandoned changeset, or maybe because of a missing submodule ref 
           (gitlink) erroneously included on patchset 137.  Hudson is new on this repo
           and our gitlink policies are not hook-enforced yet.  Regardless, Hudson 
           submitted a Verified:-1 update; it reported a build duration of 01:47, 
           making the end time 15:24:54 or so.

15:23:41   A database error in the gerrit logs appears during the Hudson build.  
           Detail below.
 
15:24:??   I reviewed changeset 137 via Gerrit.  Hudson was testing this changeset
           at the same time, coincidentally.

15:45      Error-prone admin posts a comment to the review and follows up with a push to
15:47      the wrong "magic branch" at 15:47.   There is an anomalous message in the 
           now-Merged commit which reports the patch was submitted to fres/for/master.  
           "git ls-remote gerrit" does not show any branch with this name.  

15:47:16   Gerrit change notification email is sent out reporting that the error-prone admin
           has submitted his changeset, a charge he vehemently denies.

Notes:  The score on this merged changeset is "V:0 R:-1"


======================================================================================================
Here is the error in the database log: 

    [2011-02-15 15:23:41,866] WARN  /gerrit : Error in publishComments
    com.google.gwtorm.client.OrmDuplicateKeyException: patch_set_approvals
        at com.google.gwtorm.schema.sql.DialectPostgreSQL.convertError(DialectPostgreSQL.java:53)
        at com.google.gwtorm.jdbc.JdbcAccess.convertError(JdbcAccess.java:331)
        at com.google.gwtorm.jdbc.JdbcAccess.doInsert(JdbcAccess.java:178)
        at com.google.gwtorm.jdbc.JdbcAccess.doInsert(JdbcAccess.java:35)
        at com.google.gwtorm.client.impl.AbstractAccess.insert(AbstractAccess.java:56)
        at com.google.gerrit.server.patch.PublishComments.publishApprovals(PublishComments.java:222)
        at com.google.gerrit.server.patch.PublishComments.call(PublishComments.java:121)
        at com.google.gerrit.server.patch.PublishComments.call(PublishComments.java:53)
        at com.google.gerrit.httpd.rpc.Handler$1.call(Handler.java:53)
        at com.google.gerrit.httpd.rpc.Handler.to(Handler.java:65)
        at com.google.gerrit.httpd.rpc.patch.PatchDetailServiceImpl.publishComments(PatchDetailServiceImpl.java:132)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:616)
        at com.google.gwtjsonrpc.server.MethodHandle.invoke(MethodHandle.java:91)
        at com.google.gwtjsonrpc.server.JsonServlet.doService(JsonServlet.java:382)
        at com.google.gwtjsonrpc.server.JsonServlet.service(JsonServlet.java:268)
        at javax.servlet.http.HttpServlet.service(HttpServlet.java:717)
        at com.google.inject.servlet.ServletDefinition.doService(ServletDefinition.java:216)
        at com.google.inject.servlet.ServletDefinition.service(ServletDefinition.java:141)
        at com.google.inject.servlet.ManagedServletPipeline.service(ManagedServletPipeline.java:93)
        at com.google.inject.servlet.FilterChainInvocation.doFilter(FilterChainInvocation.java:63)
        at com.google.inject.servlet.FilterDefinition.doFilter(FilterDefinition.java:134)
        at com.google.inject.servlet.FilterChainInvocation.doFilter(FilterChainInvocation.java:59)
        at com.google.inject.servlet.FilterDefinition.doFilter(FilterDefinition.java:134)
        at com.google.inject.servlet.FilterChainInvocation.doFilter(FilterChainInvocation.java:59)
        at com.google.inject.servlet.FilterDefinition.doFilter(FilterDefinition.java:134)
        at com.google.inject.servlet.FilterChainInvocation.doFilter(FilterChainInvocation.java:59)
        at com.google.gwtexpui.server.CacheControlFilter.doFilter(CacheControlFilter.java:76)
        at com.google.inject.servlet.FilterDefinition.doFilter(FilterDefinition.java:129)
        at com.google.inject.servlet.FilterChainInvocation.doFilter(FilterChainInvocation.java:59)
        at com.google.gerrit.httpd.RequestCleanupFilter.doFilter(RequestCleanupFilter.java:54)
        at com.google.inject.servlet.FilterDefinition.doFilter(FilterDefinition.java:129)
        at com.google.inject.servlet.FilterChainInvocation.doFilter(FilterChainInvocation.java:59)
        at com.google.inject.servlet.ManagedFilterPipeline.dispatch(ManagedFilterPipeline.java:122)
        at com.google.inject.servlet.GuiceFilter.doFilter(GuiceFilter.java:110)
        at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1322)
        at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:473)
        at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:921)
        at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:403)
        at org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:856)
        at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:117)
        at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:114)
        at org.eclipse.jetty.server.Server.handle(Server.java:352)
        at org.eclipse.jetty.server.HttpConnection.handleRequest(HttpConnection.java:596)
        at org.eclipse.jetty.server.HttpConnection$RequestHandler.content(HttpConnection.java:1069)
        at org.eclipse.jetty.http.HttpParser.parseNext(HttpParser.java:805)
        at org.eclipse.jetty.http.HttpParser.parseAvailable(HttpParser.java:218)
        at org.eclipse.jetty.server.HttpConnection.handle(HttpConnection.java:426)
        at org.eclipse.jetty.io.nio.SelectChannelEndPoint.handle(SelectChannelEndPoint.java:510)
        at org.eclipse.jetty.io.nio.SelectChannelEndPoint.access$000(SelectChannelEndPoint.java:34)
        at org.eclipse.jetty.io.nio.SelectChannelEndPoint$1.run(SelectChannelEndPoint.java:40)
        at org.eclipse.jetty.util.thread.QueuedThreadPool$2.run(QueuedThreadPool.java:450)
        at java.lang.Thread.run(Thread.java:636)
    Caused by: java.sql.BatchUpdateException: Batch entry 0 INSERT INTO patch_set_approvals(value,granted,change_open,change_sort_key,change_id,patch_set_id,account_id,category_id)VALUES('0','2011-02-15 15:23:41.816000 -05:00:00','Y','001311a700000088','136','1','1000002','VRIF') was aborted.  Call getNextException to see the cause.
        at org.postgresql.jdbc2.AbstractJdbc2Statement$BatchResultHandler.handleError(AbstractJdbc2Statement.java:2569)
        at org.postgresql.core.v3.QueryExecutorImpl.processResults(QueryExecutorImpl.java:1796)
        at org.postgresql.core.v3.QueryExecutorImpl.execute(QueryExecutorImpl.java:407)
        at org.postgresql.jdbc2.AbstractJdbc2Statement.executeBatch(AbstractJdbc2Statement.java:2708)
        at org.apache.commons.dbcp.DelegatingStatement.executeBatch(DelegatingStatement.java:297)
        at org.apache.commons.dbcp.DelegatingStatement.executeBatch(DelegatingStatement.java:297)
        at com.google.gwtorm.jdbc.JdbcAccess.execute(JdbcAccess.java:293)
        at com.google.gwtorm.jdbc.JdbcAccess.doInsert(JdbcAccess.java:171)
        ... 52 more
    Caused by: org.postgresql.util.PSQLException: ERROR: duplicate key value violates unique constraint "patch_set_approvals_pkey"
        at org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse(QueryExecutorImpl.java:2062)
        at org.postgresql.core.v3.QueryExecutorImpl.processResults(QueryExecutorImpl.java:1795)
        ... 58 more

======================================================================================================
Here's the notification email:
    -------- Original Message --------
    Subject: Change in pr1...tech[master]: added compile option to not use NAND in API Libs
    Date: Tue, 15 Feb 2011 15:47:16 -0500
    From: Foo Barr (Code Review) <gerrit@myco.com>
    Reply-To: foo.barr@myco.com
    CC: Phil Hord <hordp@myco.com>

    Foo Barr has submitted this change and it was merged.

    Change subject: added compile option to not use NAND in API Libs
    ......................................................................
    added compile option to not use NAND in API Libs

    Change-Id: Icac26c836966710e87895e70e306a333223de202
    ---

    M apps/somepath/renamed-file.c
    <deletia>
    8 files changed, 41 insertions(+), 16 deletions(-)

    Objections:
      Phil Hord: I would prefer that you didn't submit this

    --

    Gerrit-MessageType: merged
    Gerrit-Change-Id: Icac26c836966710e87895e70e306a333223de202
    Gerrit-PatchSet: 1
    Gerrit-Project: pr1/module/tech
    Gerrit-Branch: master
    Gerrit-Owner: Foo Barr <foo.barr@myco.com>
    Gerrit-Reviewer: Foo Barr <foo.barr@myco.com>
    Gerrit-Reviewer: Phil Hord <hordp@myco.com>
    Gerrit-Reviewer: Steve Austin <sAustin@myco.com>

======================================================================================================
Here's the Gerrit review page for 133 (related to the above email):
    Change Icac26c83: added compile option to not use NAND in API Libs
    Change-Id: 	Icac26c836966710e87895e70e306a333223de202
    Owner	Foo Barr
    Project	pr1/module/tech
    Branch	master
    Topic	
    Uploaded	Feb 14, 2011 3:11 PM
    Updated	Feb 15, 2011 3:47 PM
    Status	Merged
     	
    added compile option to not use NAND in API Libs

    Set Compile flag to define NO_NAND to turn this on
    Changed pre Steve to #endif line see Gerrit

    Removed #ifdef in NandBdmShim.c as it was not needed.
    Moved define NO_NAND to sades.h

    Change-Id: Icac26c836966710e87895e70e306a333223de202
    Reviewer	 	Verified	Code Review	 
    Foo Barr	 	 	 	
    Phil Hord	 	 	-1	I would prefer that you didn't submit this
    Steve Austin	 	 	 	

    Author	    Foo Barr<foo.barr@myco.com>Feb 11, 2011 1:46 PM
    Committer	Foo Barr<foo.barr@myco.com>Feb 14, 2011 11:35 AM

     	 	File Path	Comments	Size	Diff	Reviewed
	     	Commit Message		 	Side-by-Side	Unified	
     	M	apps/mongo/makefile_common		+2, -0	Side-by-Side	Unified	
    <deletia>

    Comments
	
    Expand RecentExpand AllCollapse All
    Phil Hord	Patch Set 1: I would prefer that you didn't submit this (8 inline …	3:20 PM

    Patch Set 1: I would prefer that you didn't submit this

    (8 inline comments)

    Whitespace noise issues.

    I think if you add "Change-Id: Icac26c836966710e87895e70e306a333223de202" to your commit msg, you will get a patch instead of a new review...
    Foo Barr	Patch Set 1: (2 inline comments) I can fix these in my next commit or …	3:45 PM

    Patch Set 1: (2 inline comments)

    I can fix these in my next commit or squash them together. and resubmit.
    Foo Barr	Change has been successfully pushed into branch fres/for/master.	3:47 PM

    Change has been successfully pushed into branch fres/for/master.
    Press '?' to view keyboard shortcuts
    Powered by Gerrit Code Review (2.1.6.1) | Report Bug

======================================================================================================
Here's the Gerrit review page for 136 (which the DB error refers to):
    Change I6d46057d: Fritzed the whaznot, twice
    Change-Id: 	I6d46057d9e9e8e31808344ba35df8b8cc5b618a1
    Owner	Foo Barr
    Project	pr1/super-project
    Branch	master
    Topic	
    Uploaded	Feb 15, 2011 3:23 PM
    Updated	Feb 15, 2011 4:04 PM
    Status	Abandoned

    Fritzed the whaznot, twice

    Change-Id: I6d46057d9e9e8e31808344ba35df8b8cc5b618a1
    Reviewer	 	Verified	Code Review	 
    Hudson CI	 		 	Fails
    Phil Hord	 	 	-1	I would prefer that you didn't submit this

		    ID	Subject	Owner	Project	Branch	Updated
    Depends On
		    I71e43d86	ReAdded copy of commit-msg hook from scripts to submodules (ABANDONED)	Foo Barr	pr1/super-project	master	Feb 11
    Needed By
    (None)
	    Patch Set 1
	     4143821fcc2cd26dc7816eb6701d29f495bc2a8d (gitweb)
    Author	Foo Barr<foo.barr@myco.com>Feb 15, 2011 3:13 PM
    Committer	Foo Barr<foo.barr@myco.com>Feb 15, 2011 3:13 PM

    Comments
	
    Patch Set 1:

    Build Started http://mongo.myco.com/hudson/job/gerrit-super-project/47/
    Hudson CI	Patch Set 1: Fails Build Failed FAILURE: …	3:23 PM

    Patch Set 1: Fails

    Build Failed

    FAILURE: http://mongo.myco.com/hudson/job/gerrit-super-project/47/
    Phil Hord	Patch Set 1: 
    1. Depends on an abandoned change. 2. Do not commit gitlinks …	3:24 PM

    Patch Set 1:

    1. Depends on an abandoned change. 2. Do not commit gitlinks to the superproject.
    Phil Hord	Patch Set 1: I would prefer that you didn't submit this	3:41 PM

    Patch Set 1: I would prefer that you didn't submit this
    Foo Barr	Patch Set 1: Abandoned gitlinks are not wanted :-(	4:04 PM

    Patch Set 1: Abandoned

    gitlinks are not wanted :-(
    Press '?' to view keyboard shortcuts
    Powered by Gerrit Code Review (2.1.6.1) | Report Bug


 
	
======================================================================================================
Here's the Gerrit review page for 137 (which the Hudson was testing at the time):
    Change I6d46057d: s2diags and launchdir changes
    Change-Id: 	I6d46057d9e9e8e31808344ba35df8b8cc5b618a1
    Owner	Foo Barr
    Project	pr1/super-project
    Branch	master
    Topic	
    Uploaded	Feb 15, 2011 3:23 PM
    Updated	Feb 15, 2011 4:04 PM
    Status	Abandoned
     	
    
    Change-Id: I6d46057d9e9e8e31808344ba35df8b8cc5b618a1
    Reviewer	 	Verified	Code Review	 
    Hudson CI	 		 	Fails
    Phil Hord	 	 	-1	I would prefer that you didn't submit this

    Add Reviewer
	    Included in
     
	    Dependencies
		    ID	Subject	Owner	Project	Branch	Updated
    Depends On
		    I71e43d86	ReAdded copy of commit-msg hook from scripts to submodules (ABANDONED)	Foo Barr	pr1/super-project	master	Feb 11
    Needed By
    (None)
	    Patch Set 1
	     4143821fcc2cd26dc7816eb6701d29f495bc2a8d (gitweb)
    Author	Foo Barr<foo.barr@myco.com>Feb 15, 2011 3:13 PM
    Committer	Foo Barr<foo.barr@myco.com>Feb 15, 2011 3:13 PM

    ReviewRestore ChangeDiff All Side-by-SideDiff All Unified
     	 	File Path	Comments	Size	Diff	Reviewed
	     	Commit Message		 	Side-by-Side	Unified	
     	M	makery		+0, -0	Side-by-Side	Unified	
     	M	tech		+0, -0	Side-by-Side	Unified	
     		+0, -0
    Comments
	
    Expand RecentExpand AllCollapse All
    Hudson CI	Patch Set 1: Build Started …	3:23 PM

    Patch Set 1:

    Build Started http://mongo.myco.com/hudson/job/gerrit-super-project/47/
    Hudson CI	Patch Set 1: Fails Build Failed FAILURE: …	3:23 PM

    Patch Set 1: Fails

    Build Failed

    FAILURE: http://mongo.myco.com/hudson/job/gerrit-super-project/47/
    Phil Hord	Patch Set 1: 1. Depends on an abandoned change. 2. Do not commit gitlinks …	3:24 PM

    Patch Set 1:

    1. Depends on an abandoned change. 2. Do not commit gitlinks to the superproject.
    Phil Hord	Patch Set 1: I would prefer that you didn't submit this	3:41 PM

    Patch Set 1: I would prefer that you didn't submit this
    Foo Barr	Patch Set 1: Abandoned gitlinks are not wanted :-(	4:04 PM

    Patch Set 1: Abandoned

    gitlinks are not wanted :-(
    Press '?' to view keyboard shortcuts
    Powered by Gerrit Code Review (2.1.6.1) | Report Bug

Mar 5, 2011
#1 phil.hord
I think this might be related to this discussion:
http://groups.google.com/group/repo-discuss/browse_thread/thread/8aeef7d09964fd9b/5d7d519166f8ed5b

I think push to rfes/for/master would create a new branch and cause the changes to get merged, just as happened here.  But I can't explain why the wayward branch did not show up in the repo.
Nov 14, 2011
#2 phil.hord
I've confirmed this is a duplicate of #635.  I suspect the OrmDuplicateKey error is caused by two changesets having the same changeid and the DB not handling it.  But the main problem is repeatable and is the same as  bug #635 .

Having said that, I see there has been some improvement on this problem and #635 has been closed.  But the problem covered here still exists:

   # Developer wants a patch reviewed
   git push origin HEAD:refs/for/master
   
   # Developer (intentionally or not) pushes the same commit to a public branch
   git push origin HEAD:refs/heads/share/public

   # At this point the original patchset is marked as merged

Should this be closed as a dup?
I still want the original issue addressed (also covered in #635).  Should #635 be re-opened, a new bug created, or this one cleaned up?

Demo:
{
  Push commit for review.
  Check status of our commit; see that it is "Open"
  Push commit to share/public branch
  Check status of our commit; see that it is "Merged"
}

   echo foo >> foo && git add foo && git commit -mFoo 
   [detached HEAD ac40f2b] Foo
    1 files changed, 1 insertions(+), 0 deletions(-)
   CID=$(git log -1 | grep Change-Id: |cut -d: -f2)
   echo $CID
    I4ca9183e1225ed94e355b2f62b7e255952df2045

   git push origin HEAD:refs/for/master
   Counting objects: 5, done.
   Delta compression using up to 2 threads.
   Compressing objects: 100% (2/2), done.
   Writing objects: 100% (3/3), 291 bytes, done.
   Total 3 (delta 1), reused 0 (delta 0)
   remote: Resolving deltas:   0% (0/1)
   remote: 
   remote: New Changes:
   remote:   https://gerrit.myco.com/gerrit/759
   remote: 
   To gerrit:testing
    * [new branch]      HEAD -> refs/for/master
   git                                                   

   ssh gerrit gerrit query --current-patch-set ${CID} 
   change I4ca9183e1225ed94e355b2f62b7e255952df2045
     project: testing
     branch: master
     id: I4ca9183e1225ed94e355b2f62b7e255952df2045
     number: 759
     subject: Foo
     owner:
       name: Phil Hord
       email: hordp@cisco.com
     url: https://gerrit.myco.com/gerrit/759
     lastUpdated: 2011-11-14 14:54:40 EST
     sortKey: 00190b8a000002f7
     open: true
     status: NEW
     currentPatchSet:
       number: 1
       revision: ac40f2b66323aabd8d62de9d758aadb79e051853
       ref: refs/changes/59/759/1
       uploader:
         name: Phil Hord
         email: hordp@cisco.com
   
   type: stats
   rowCount: 1
   runTimeMilliseconds: 4
   
   git push origin HEAD:refs/heads/share/public       
   Total 0 (delta 0), reused 0 (delta 0)
   To gerrit:testing
      25ea87d..ac40f2b  HEAD -> share/public

   ssh gerrit gerrit query --current-patch-set ${CID} 
   change I4ca9183e1225ed94e355b2f62b7e255952df2045
     project: testing
     branch: master
     id: I4ca9183e1225ed94e355b2f62b7e255952df2045
     number: 759
     subject: Foo
     owner:
       name: Phil Hord
       email: hordp@cisco.com
     url: https://gerrit.myco.com/gerrit/759
     lastUpdated: 2011-11-14 14:55:44 EST
     sortKey: 00190b8b000002f7
     open: false
     status: MERGED
     currentPatchSet:
       number: 1
       revision: ac40f2b66323aabd8d62de9d758aadb79e051853
       ref: refs/changes/59/759/1
       uploader:
         name: Phil Hord
         email: hordp@cisco.com
   
   type: stats
   rowCount: 1
   runTimeMilliseconds: 2
 

Feb 17, 2012
#3 phil.hord
This is a dup of #1142, but #1142 is more focused and being addressed.
Feb 6, 2014
#4 nemow...@gmail.com
Can also happen the other way round, at gerrit.wikimedia.org we had one patch which was marked open by the interface but had one depending change merged, because it was in fact merged.