In Progress

Bug

Status Update

No update yet.

Description

lu...@gmail.com

created issue #1

Jan 5, 2024 12:25AM

*** !!!! THIS BUG TRACKER IS FOR GERRIT CODE REVIEW !!!! *** Do not submit bugs for chrome/android and issues with your company's *** Gerrit setup here. Those issues belong in different issue trackers.

Background

The project eclipse-jgit/jgit on eclipse.gerrithub.io was initially developed on git.eclipse.org and then moved to GerritHub.io on the 20th of November 2023.

Some of the changes started on git.eclipse.org and then were moved to eclipse.gerrithub.io. The identities of the authors on git.eclipse.org have been remapped to other accounts on gerrithub.io.

Example: David Ostrovsky has accountId=1000177 on eclipse.gerrithub.io and accountId=1684 on git.eclipse.org.

The serverId of eclipse.gerrithub.io is d5d70762-12d0-45a1-890d-524b12d3f735, the serverId of git.eclipse.org is 97ee7c02-f12f-4043-b43e-dea463d88b31, therefore David Ostrovsky is identified as 1000177@d5d70762-12d0-45a1-890d-524b12d3f735 or 1684@97ee7c02-f12f-4043-b43e-dea463d88b31 in change messages.

With regards to change comments, they are stored in NoteDb (the refs/changes/NN/NNN/meta ref) with account=1684 when serverId=97ee7c02-f12f-4043-b43e-dea463d88b31 or account=1000177 when serverId=d5d70762-12d0-45a1-890d-524b12d3f735.

What steps will reproduce the problem?

On Sat Dec 30 01:17:24 2023 I have published two new comments to the change eclipse-jgit/jgit:194140

What is the expected output? The updated change notes refs/changes/40/194140/meta should have contained two more comments.

What do you see instead? The updated change notes (Commit SHA1=f62b199bd2b00a3f73f11fa97392fc6a65270690) had David Ostrovsky's comments associated with accountId=1684 on serverId=97ee7c02-f12f-4043-b43e-dea463d88b31 rewritten as accountId=1000177.

Additional information below.

See below the diff of the changes introduced on refs/changes/40/194140/meta with the commit f62b199bd2b00a3f73f11fa97392fc6a65270690:

commit f62b199bd2b00a3f73f11fa97392fc6a65270690
Author: Gerrit User 1000000 <1000000@d5d70762-12d0-45a1-890d-524b12d3f735>
Date:   Sat Dec 30 01:17:24 2023 +0000

    Update patch set 25
    
    Patch Set 25:
    
    (2 comments)
    
    Patch-set: 25

diff --git a/4566262c4845cf3071587391a7276c55f28a9de5 b/4566262c4845cf3071587391a7276c55f28a9de5
index cd0f75d428..1f0a158677 100644
--- a/4566262c4845cf3071587391a7276c55f28a9de5
+++ b/4566262c4845cf3071587391a7276c55f28a9de5
@@ -9,7 +9,7 @@
       },
       "lineNbr": 12,
       "author": {
-        "id": 1684
+        "id": 1000177
       },
       "writtenOn": "2022-06-14T06:13:12Z",
       "side": 1,
@@ -26,7 +26,7 @@
       },
       "lineNbr": 12,
       "author": {
-        "id": 4
+        "id": 1000671
       },
       "writtenOn": "2022-06-27T11:48:43Z",
       "side": 1,
@@ -44,7 +44,7 @@
       },
       "lineNbr": 12,
       "author": {
-        "id": 290522
+        "id": 1005270
       },
       "writtenOn": "2023-11-16T13:38:09Z",
       "side": 1,
@@ -62,7 +62,7 @@
       },
       "lineNbr": 0,
       "author": {
-        "id": 1684
+        "id": 1000177
       },
       "writtenOn": "2022-06-14T06:13:12Z",
       "side": 1,
@@ -79,7 +79,7 @@
       },
       "lineNbr": 2,
       "author": {
-        "id": 1684
+        "id": 1000177
       },
       "writtenOn": "2022-06-14T06:13:12Z",
       "side": 1,
@@ -102,7 +102,7 @@
       },
       "lineNbr": 2,
       "author": {
-        "id": 290522
+        "id": 1005270
       },
       "writtenOn": "2023-11-06T11:19:59Z",
       "side": 1,
@@ -126,7 +126,7 @@
       },
       "lineNbr": 58,
       "author": {
-        "id": 1684
+        "id": 1000177
       },
       "writtenOn": "2022-06-14T06:13:12Z",
       "side": 1,
@@ -149,7 +149,7 @@
       },
       "lineNbr": 58,
       "author": {
-        "id": 4
+        "id": 1000671
       },
       "writtenOn": "2022-06-27T11:48:43Z",
       "side": 1,
@@ -173,7 +173,7 @@
       },
       "lineNbr": 58,
       "author": {
-        "id": 290522
+        "id": 1005270
       },
       "writtenOn": "2023-11-07T13:17:26Z",
       "side": 1,
@@ -197,7 +197,7 @@
       },
       "lineNbr": 27,
       "author": {
-        "id": 1684
+        "id": 1000177
       },
       "writtenOn": "2022-06-14T06:13:12Z",
       "side": 1,
@@ -220,7 +220,7 @@
       },
       "lineNbr": 27,
       "author": {
-        "id": 290522
+        "id": 1005270
       },
       "writtenOn": "2023-11-06T11:19:59Z",
       "side": 1,
@@ -244,7 +244,7 @@
       },
       "lineNbr": 0,
       "author": {
-        "id": 1684
+        "id": 1000177
       },
       "writtenOn": "2022-06-14T06:13:12Z",
       "side": 1,
@@ -261,7 +261,7 @@
       },
       "lineNbr": 0,
       "author": {
-        "id": 4
+        "id": 1000671
       },
       "writtenOn": "2022-06-27T11:48:43Z",
       "side": 1,
@@ -279,7 +279,7 @@
       },
       "lineNbr": 0,
       "author": {
-        "id": 290522
+        "id": 1005270
       },
       "writtenOn": "2023-11-16T13:38:09Z",
       "side": 1,
@@ -297,7 +297,7 @@
       },
       "lineNbr": 232,
       "author": {
-        "id": 1684
+        "id": 1000177
       },
       "writtenOn": "2022-06-14T06:13:12Z",
       "side": 1,
@@ -320,7 +320,7 @@
       },
       "lineNbr": 232,
       "author": {
-        "id": 290522
+        "id": 1005270
       },
       "writtenOn": "2023-11-08T15:23:38Z",
       "side": 1,
@@ -344,7 +344,7 @@
       },
       "lineNbr": 233,
       "author": {
-        "id": 1684
+        "id": 1000177
       },
       "writtenOn": "2022-06-14T06:13:12Z",
       "side": 1,
@@ -367,7 +367,7 @@
       },
       "lineNbr": 233,
       "author": {
-        "id": 4
+        "id": 1000671
       },
       "writtenOn": "2022-06-27T11:48:43Z",
       "side": 1,
@@ -391,7 +391,7 @@
       },
       "lineNbr": 233,
       "author": {
-        "id": 290522
+        "id": 1005270
       },
       "writtenOn": "2023-10-13T14:17:04Z",
       "side": 1,
@@ -415,7 +415,7 @@
       },
       "lineNbr": 233,
       "author": {
-        "id": 97483
+        "id": 1000000
       },
       "writtenOn": "2023-10-13T16:37:12Z",
       "side": 1,
@@ -439,7 +439,7 @@
       },
       "lineNbr": 233,
       "author": {
-        "id": 4
+        "id": 1000671
       },
       "writtenOn": "2023-10-14T20:42:43Z",
       "side": 1,
@@ -463,7 +463,7 @@
       },
       "lineNbr": 233,
       "author": {
-        "id": 290522
+        "id": 1005270
       },
       "writtenOn": "2023-11-08T15:23:38Z",
       "side": 1,
@@ -526,6 +526,30 @@
       "revId": "4566262c4845cf3071587391a7276c55f28a9de5",
       "serverId": "d5d70762-12d0-45a1-890d-524b12d3f735"
     },
+    {
+      "unresolved": true,
+      "key": {
+        "uuid": "178f1ab6_fec5fe4f",
+        "filename": "org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevWalk.java",
+        "patchSetId": 3
+      },
+      "lineNbr": 233,
+      "author": {
+        "id": 1000000
+      },
+      "writtenOn": "2023-12-30T01:17:24Z",
+      "side": 1,
+      "message": "Ping @matthias.sohn@sap.com that\u0027s your first decision for January 2024 😊\n\nJGit is downloaded millions of times and this queue is a central point for many use-cases. We\u0027ve been using it for years with Gerrit and it works fine, but it would be safer to have a _safety net_ just-in-case.\n\nI agree with the 3-steps approach proposed by @david.ostrovsky@gmail.com.",
+      "parentUuid": "ebef9d62_4fb99ced",
+      "range": {
+        "startLine": 233,
+        "startChar": 2,
+        "endLine": 233,
+        "endChar": 93
+      },
+      "revId": "4566262c4845cf3071587391a7276c55f28a9de5",
+      "serverId": "d5d70762-12d0-45a1-890d-524b12d3f735"
+    },
     {
       "unresolved": true,
       "key": {
@@ -535,7 +559,7 @@
       },
       "lineNbr": 361,
       "author": {
-        "id": 1684
+        "id": 1000177
       },
       "writtenOn": "2022-06-14T06:13:12Z",
       "side": 1,
@@ -558,7 +582,7 @@
       },
       "lineNbr": 361,
       "author": {
-        "id": 290522
+        "id": 1005270
       },
       "writtenOn": "2023-11-06T13:17:31Z",
       "side": 1,
@@ -582,7 +606,7 @@
       },
       "lineNbr": 362,
       "author": {
-        "id": 1684
+        "id": 1000177
       },
       "writtenOn": "2022-06-14T06:13:12Z",
       "side": 1,
@@ -605,7 +629,7 @@
       },
       "lineNbr": 362,
       "author": {
-        "id": 290522
+        "id": 1005270
       },
       "writtenOn": "2023-11-06T13:17:31Z",
       "side": 1,
@@ -629,7 +653,7 @@
       },
       "lineNbr": 98,
       "author": {
-        "id": 1684
+        "id": 1000177
       },
       "writtenOn": "2022-06-14T06:13:12Z",
       "side": 1,
@@ -652,7 +676,7 @@
       },
       "lineNbr": 98,
       "author": {
-        "id": 290522
+        "id": 1005270
       },
       "writtenOn": "2023-11-06T13:17:31Z",
       "side": 1,
@@ -676,7 +700,7 @@
       },
       "lineNbr": 99,
       "author": {
-        "id": 1684
+        "id": 1000177
       },
       "writtenOn": "2022-06-14T06:13:12Z",
       "side": 1,
@@ -699,7 +723,7 @@
       },
       "lineNbr": 99,
       "author": {
-        "id": 290522
+        "id": 1005270
       },
       "writtenOn": "2023-11-06T13:17:31Z",
       "side": 1,
diff --git a/d3877e6ae3a7d286ff614ee6ac1258d2bf7f25fa b/d3877e6ae3a7d286ff614ee6ac1258d2bf7f25fa
index 4620e07d95..8cb896c761 100644
--- a/d3877e6ae3a7d286ff614ee6ac1258d2bf7f25fa
+++ b/d3877e6ae3a7d286ff614ee6ac1258d2bf7f25fa
@@ -71,6 +71,30 @@
       "revId": "d3877e6ae3a7d286ff614ee6ac1258d2bf7f25fa",
       "serverId": "d5d70762-12d0-45a1-890d-524b12d3f735"
     },
+    {
+      "unresolved": false,
+      "key": {
+        "uuid": "11e7b900_7bd5e1e8",
+        "filename": "org.eclipse.jgit/src/org/eclipse/jgit/revwalk/AbstractRevQueue.java",
+        "patchSetId": 23
+      },
+      "lineNbr": 34,
+      "author": {
+        "id": 1000000
+      },
+      "writtenOn": "2023-12-30T01:17:24Z",
+      "side": 1,
+      "message": "+1 to @david.ostrovsky@gmail.com, never implement features that aren\u0027t used by anyone.\n\nShould Matthias decide to go with a staged rollout of this change (old priority queue + new priority queue in parallel), we can still reintroduce as default method in the interface.\n\nDeleted.",
+      "parentUuid": "88c60c68_6222a64c",
+      "range": {
+        "startLine": 23,
+        "startChar": 1,
+        "endLine": 34,
+        "endChar": 2
+      },
+      "revId": "d3877e6ae3a7d286ff614ee6ac1258d2bf7f25fa",
+      "serverId": "d5d70762-12d0-45a1-890d-524b12d3f735"
+    },
     {
       "unresolved": true,
       "key": {

It looks like the ChangeUpdate.java class had rewritten the author of some of its comments instead of just reading the existing ones on NoteDb.

The only method that allows to rewrite the author in a comment is in Comment.setRealAuthor():

  public void setRealAuthor(Account.Id id) {
    realAuthor = id != null && id.get() != author.id ? new Comment.Identity(id) : null;
  }

It is unclear how publishing a draft comment for me could have triggered the call to setRealAuthor() for a series of other changes.

Comments

da...@gmail.com <da...@gmail.com> Jan 5, 2024 12:21PM

Accepted by da...@gmail.com.

da...@gmail.com <da...@gmail.com> #2Jan 8, 2024 05:28PM

Here's a more detailed timeline from the change metadata:

Dec 30 01:04:10 - patch set 24 was rebased by Marcin Czech, creating patch set 25
Dec 30 01:17:19 - patch set 26 was pushed by Luca Milanesio
Dec 30 01:17:24 - 2 comments by Luca Milanesio were published on patch set 25
Dec 30 01:17:24 - attention set on patch set 26 was updated

Step #3 has broken the account mapping. On a closer look, that single publication of two comments is more complex than it looks. From the diff, we can tell that it is adding comments on two patch sets:

patch set 23 (commit id d3877e6) - all good here, no changes to account ids
patch set 3 (commit id 4566262) - introduces breaking changes, all comment.author.id values were changed to current server account id from the migrated server account id.

Let's now have a closer look at how the external to current account ID mapping works. The external account ID mapping is handled when metadata are read from git. The NoteDB stores comments in JSON files, which are first parsed, and then the comment author's IDs are "negotiated", meaning that a mapping between the current instance account ID and external account ID is found. From that point, all (in memory) comments are pointing to account IDs from the current instance (whereas NoteDB data stays unchanged and uses external account id).

This looks to me as a potential serialization problem. We either haven't "reverse" mapped the account IDs or used a list of comments "from memory" instead of "from git" when creating a commit object.

The other question is why, this happens on that single occasion, as we haven't observed similar issues in other changes.

I'll continue investigating this issue.

da...@gmail.com <da...@gmail.com> #3Jan 9, 2024 09:12AM

One suspicious area is com.google.gerrit.server.notedb.ChangeUpdate#getRevisionNoteMap(). The ChangeUpdate class is not threaded safe, as stated in the class javadoc, additionally the getRevisionNoteMap(), can potentially reuse previously loaded ChangeRevisionNote, which makes me wonder if it would be possible that the reused ChangeRevisionNote could have mapped account IDs, this could potentially lead to the result we observed in this bug report, but I don't know how (or even if) it is technically possible to happen.

Message last modified on Jan 9, 2024 09:15AM

da...@gmail.com <da...@gmail.com> #4Jan 10, 2024 11:05PM

I was able to reproduce it locally and it's a caching-related problem.

For the reproduction an imported project with external account ID mapping and a change with comments created on the old server instance is required.

Create a draft commit on an imported patch set with comments created on the other instance
Connect the debugger and put a thread breakpoint in com.google.gerrit.server.notedb.ChangeNotesCache:405
Submit draft comment created in step #1
Each time the breakpoint is hit, flush all caches over SSH
Investigate the meta branch for change and observe changed comment.author.id values

Disabling change_notes should also result in a similar outcome.

What is happening here?

First of all, as explained in the second comment, we keep the original account ID in the meta branch, but when we read (or cache) the data, we automatically map that account id to the current one and store that in the cache.

Looking at a comment in ChangeNotesCache L375 gives a hint of what is actually going on. Another part of the puzzle is in ChangeUpdate L652, where we use cached note when notes.revisionNoteMap is not null (remember that the cached value has mapped account IDs). The final part is in the RevisionNoteBuilder L170, where we just serialize Comments objects.

Let's now put it all together. When we don't find an entry in the change_notes cache, we'll load it from the meta branch, migrate the account IDs and keep that modified value in memory. Now when a comment is added and change notes were just evicted from the cache, the ChangeUpdate will load it. Then in ChagneUpdate.getRevisionMap() we'll use the cached value. Finally when we serialize data back to JSON format to store it in the meta branch, it the RevisionNoteBuilder we don't use any GSON mapping from Comment.author field which will just store the current server account ID.

This issue will only occur when change note is evicted from cache "just" before ChangeUpdate is creating the revision map.

ap...@google.com <ap...@google.com> #5Jan 12, 2024 04:55PM

Project: gerrit
Branch: stable-3.7

commit e2e7d68c093fd790c60b976736a05d8962a6fe98
Author: Luca Milanesio <luca.milanesio@gmail.com>
Date: Thu Jan 11 23:43:23 2024

Complete comment identity with the associated serverId in comments

Change note-db /meta ref contains the list of change notes associated,
including all the comments were added to files and commit messages.

The remapping of account-ids performed with I4b0fe6689 has correctly
adapted the comment's author account-ids to the local associated
accounts, however, it failed to update the comment serverId, leaving
the rest of Gerrit with an inconsistent identity.

Even though the identity was not causing apparent damage, mostly
because the serverId was ignored mainly by Gerrit; it left the cached
entities with the incorrect data and, in some cases, persisted back
to NoteDb (see

Issue 318745940

).

Rectify the situation by updating the serverId when accounts are
remapped.

Release-Notes: Fix serverId in identity parsing for imported human comments
Forward-Compatible: checked
Bug:

Issue 318745940

Change-Id: I5904dc46120858beb54e20ac339f10a02d785d3a

M java/com/google/gerrit/server/notedb/ChangeNotesParser.java
M java/com/google/gerrit/server/notedb/NoteDbUtil.java
M javatests/com/google/gerrit/server/notedb/ImportedChangeNotesTest.java

https://gerrit-review.googlesource.com/402157

Issue 318745940

Description

Background

What steps will reproduce the problem?

Additional information below.

Issue summary

Comments

da...@gmail.com <da...@gmail.com> Jan 5, 2024 12:21PM

da...@gmail.com <da...@gmail.com> #2Jan 8, 2024 05:28PM

da...@gmail.com <da...@gmail.com> #3Jan 9, 2024 09:12AM

da...@gmail.com <da...@gmail.com> #4Jan 10, 2024 11:05PM

ap...@google.com <ap...@google.com> #5Jan 12, 2024 04:55PM

Add comment

Issue metadata