1237261 - Autolander created two attachments in a bug for the same PR

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::GithubBot, defect)

Description

[Ed Morley [:emorley]]

In bug 1237237, autolander created two identical attachments for a newly opened PR within a few seconds of each other.

Comment 1

[Ed Morley [:emorley]]

The same thing happened in bug 1240523.

Comment 2

[Ed Morley [:emorley]]

This happened again in bug 1240385.

Looking at the deliveries reported by the GitHub hook, it's showing two: one for action "opened" (096c6c80-c13b-11e5-951d-75e92b173dcb) and one for "assigned" ( 096c6c80-c13b-11e5-9688-32cb72afea14):
https://github.com/mozilla/treeherder/settings/hooks/6742001

When I create PRs I tend to:
* Visit the open PR page (SourceTree automatically opens this for me)
* Compose the description
* Press "assign to me" whilst still in draft mode
* Press open PR

It then appears that in this case GitHub fires off two notification payloads within short succession - one for each of opened and assigned.

I'm presuming there's a race condition such that if these two events occur too closely together, autolander isn't de-duping them (unlike if I use the "assign" feature as a separate action to opening the PR).

Comment 3

[Kevin Grandon :kgrandon]

Interesting. So I believe autolander will check the bug to see if there is an attachment to the pull request already, but nothing to see if we are currently in the process of attaching one. I guess we should implement either some kind of "locking" on the bug when we are processing it.

I do believe we currently store bug state in azure table storage, a quick fix might be to check the azure table record before attaching the PR request, possibly storing additional information in azure if needed. Right now it looks like the azure write happens too late though: https://github.com/mozilla/autolander/blob/master/routes/pull_request.js#L43-L44

Comment 4

[Ed Morley [:emorley]]

The other option I guess would be to just filter the hook deliveries, so the action 'assigned' one is excluded.

Aside from the PR being opened, the only time the hook deliveries are relevant are when commits are added/updated, or the PR title changes.

Comment 5

[Ed Morley [:emorley]]

Yeah looks like "opened", "edited" or "synchronize" are the events of interest, with "assigned", "unassigned", "labeled", "unlabeled", "closed" and "reopened" being noise:
https://developer.github.com/v3/activity/events/types/#pullrequestevent

Comment 6

[Kevin Grandon :kgrandon]

Comment on attachment 8756273 [details] [review]
[autolander] edmorley:filter-event-actions > mozilla:master

Drive-by R+. Thanks :)

Comment 7

[Ed Morley [:emorley]]

https://github.com/mozilla/autolander/commit/5fbfcc0c9ae247c858df0405e14f0edc42636188

Comment 8

[Ed Morley [:emorley]]

Working :-)

May 25 09:12:52 autolander app/worker.1: Wed, 25 May 2016 16:12:51 GMT autolander:route:pull_request Skipping pull request event with action assigned

Comment 9

[Pete Moore [:pmoore][:pete]]

Comment on attachment 8756273 [details] [review]
[autolander] edmorley:filter-event-actions > mozilla:master

lgtm!

Comment 10

[GitHub Autolander Bot]

Attachment: [autolander] edmorley:filter-event-actions > mozilla:master

Comment 11

[Ed Morley [:emorley]]

Comment on attachment 8756273 [details] [review]
[autolander] edmorley:filter-event-actions > mozilla:master

There doesn't seem to be an owner for Autolander any more, so would you mind taking a look? (Saw you in the contributors list)