Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x/build/cmd/gerritbot: GitHub message sync de-duplicatation mechanism fails when Gerrit account name changes #41997

Closed
dmitshur opened this issue Oct 15, 2020 · 3 comments
Closed

x/build/cmd/gerritbot: GitHub message sync de-duplicatation mechanism fails when Gerrit account name changes #41997

dmitshur opened this issue Oct 15, 2020 · 3 comments
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Soon This needs action soon. (recent regressions, service outages, unusual time-sensitive situations)
Milestone
Backlog

Comments

@dmitshur
Copy link
Contributor

GopherBot (along with some other services) was restarted around 12 hours ago and when it came back up, the congratulateNewContributors task ended up posting a repeat "congratulations" message on some CLs that already had it posted (for example, golang/lint#496 (comment), golang/tools#215 (comment), golang/tour#940 (comment)). The duplicate messages don't show up in the Gerrit UI, but they got mirrored to GitHub.

The addGerritComment method has a mechanism to suppress duplicate messages from being posted:

// One final staleness check before sending a message: get the list
// of comments from the API and check whether any of them match.

But it seems it was ineffective in this scenario.

We may need to update it to catch new-style patch-set level messages after a Gerrit UI change (similarly to golang/build@243a34b). But I don't think this was what caused the issue during this occurrence, it must've been something else.

Filing for tracking purposes. This needs more investigation and a fix, especially if it starts to happen more often (this was the first observed and reported occurrence). Thanks to @toothrot and @cagedmantis who've also investigated this.
@dmitshur dmitshur added Builders x/build issues (builders, bots, dashboards) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Oct 15, 2020
@dmitshur dmitshur added this to the Backlog milestone Oct 15, 2020
@dmitshur
Copy link
Contributor Author

I think I see the problem. There wasn't a duplicate message sent to Gerrit. Instead, the "gobot@golang.org" Gerrit account name changed from "Gobot Gobot" to "Go Bot", which caused the Gerrit → GitHub comment de-duplication mechanism to think it was a different comment.

So, I'll retitle this as an issue for GerritBot. It shouldn't post a duplicate comment when a Gerrit account name changes.

@dmitshur dmitshur changed the title x/build/cmd/gopherbot: "congratulations" message was posted a second time x/build/cmd/gerritbot: GitHub message sync de-duplicatation mechanism fails when Gerrit account name changes Oct 15, 2020
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Oct 15, 2020
@dmitshur
Copy link
Contributor Author

dmitshur commented Dec 3, 2021 •
edited
Loading

The account "gobot@golang.org" has again been renamed recently (from "Go Bot" to "Gopher Robot"), so fixing this bug before the next time gerritbot restarts should prevent this duplicate thundering herd problem from happening another time.

The problem can be reproduced locally using a slightly improved -dry-run mode:

2021/12/02 19:59:44 [dry-run] would post comment to GitHub: Message from Gopher Robot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!
[...]

@dmitshur dmitshur added the Soon This needs action soon. (recent regressions, service outages, unusual time-sensitive situations) label Dec 3, 2021
@heschi heschi self-assigned this Dec 13, 2021
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/371455 mentions this issue: cmd/gerritbot: prevent duplicate posts, improve testability

@rsc rsc unassigned heschi Jun 23, 2022
@heschi heschi moved this to Done in Go Release Sep 27, 2022
@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Soon This needs action soon. (recent regressions, service outages, unusual time-sensitive situations)
Projects
Go Release
Archived in project
Milestone
Backlog
Development

No branches or pull requests
3 participants
@dmitshur @gopherbot @heschi