CandyShop / gerrit

Automatically exported from code.google.com/p/gerrit
Apache License 2.0
1 stars 0 forks source link

Changeset erroneously MERGED, and DB error #847

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
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

Original issue reported on code.google.com by phil.hord on 16 Feb 2011 at 1:30

GoogleCodeExporter commented 9 years ago
I think this might be related to this discussion:
http://groups.google.com/group/repo-discuss/browse_thread/thread/8aeef7d09964fd9
b/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.

Original comment by phil.hord on 5 Mar 2011 at 11:53

GoogleCodeExporter commented 9 years ago
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

Original comment by phil.hord on 14 Nov 2011 at 8:20

GoogleCodeExporter commented 9 years ago
This is a dup of #1142, but #1142 is more focused and being addressed.

Original comment by phil.hord on 17 Feb 2012 at 3:20

GoogleCodeExporter commented 9 years ago
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.

Original comment by nemow...@gmail.com on 6 Feb 2014 at 8:27