1216522 - Heroku GitHub "auto-deploy after CI passes" feature can cause duplicate deployments

Status: RESOLVED INCOMPLETE

(Tree Management :: Treeherder: Infrastructure, defect, P3)

Description

[Ed Morley [:emorley]]

Filing this to track this issue I've filed using the Heroku support system:

https://help.heroku.com/tickets/292648

"""
Our app is connected to the GitHub repo, with the automatic deploy from master option enabled, along with "Wait for CI to pass before deploy" selected:
https://dashboard.heroku.com/apps/treeherder-heroku/deploy/github

However for some of the pushes to master, there are two deploys performed. eg v502+v503:
https://dashboard.heroku.com/apps/treeherder-heroku/activity

Any ideas why this is? Might it be due to there being two Travis runs, one for the push, one for the branch - and a deploy is being triggered after both?
"""

Comment 1

[Ed Morley [:emorley]]

So after a bit of back and forth on the ticket, I tried digging into this more myself (since Heroku can't see our Hooks to debug etc).

It seems that this is a Travis/GitHub bug; I've filed:
https://github.com/travis-ci/travis-ci/issues/5008

That said, I've also added this to the Heroku ticket:

"""
That said, I wonder if the Heroku GitHub deploy feature should de-dupe based on the SHA? ie: even with that Travis/GitHub bug fixed - at the moment it seems like if I retrigger a Travis build, I'd get another deploy, even if that deploy has already been performed?

Or to put it another way:
As a user I would expect the ""Wait for CI to pass before deploy"" option to not cause more deployments than if the option wasn't enabled.

The text says "Wait for ..." rather than "Deploy every time the CI passes" etc.
"""

Comment 2

[Ed Morley [:emorley]]

Heroku's latest response:

"""
> I wonder if the Heroku GitHub deploy feature should de-dupe based on the SHA?

We've been kicking the same idea around internally. We won't rebuild if you git push the same commit so it would follow that we behave the same with GitHub sync.

> The text says "Wait for ..." rather than "Deploy every time the CI passes" etc.

Great point - I do think that the actual experience I want (as a user) is "Wait for CI" and that can be more accurately achieved by de-duping the shas.

Thanks for the great feedback!
"""

The ticket has been closed on their end, but I think that's just the way their public facing issue tracker works.

Comment 3

[Ed Morley [:emorley]]

Since they said they'll take a look, I'll come back to this in a while and check if it's been fixed. However at the moment since Heroku is auto-deploying from the autoclassify branch, not master, the root cause no longer exists, so I can't confirm either way.

Comment 4

[Ed Morley [:emorley]]

Since we won't be auto-deploying for prod, this needn't block the transition. It may also be fixed already on the Heroku side, however it's not easy to test whilst our Heroku instance is tracking the less-frequently updated autoclassify branch and not master.

Comment 5

[Ed Morley [:emorley]]

I've not seen double-deploys now that we've re-enabled deploying from master, so think we're good here.

Comment 6

[Ed Morley [:emorley]]

Occurred again, this time after a Travis retrigger - and 12 duplicate deploys were created.

Filed:
https://help.heroku.com/tickets/403467

Comment 7

[Ed Morley [:emorley]]

(In reply to Ed Morley [:emorley] from comment #6)
> Occurred again, this time after a Travis retrigger - and 12 duplicate deploys were created.

Appears that this SSL cert regression was the cause: https://status.heroku.com/incidents/949

Comment 8

[Ed Morley [:emorley]]

Still occurring:
http://logs.glob.uno/?c=mozilla%23treeherder&s=3+Apr+2017&e=3+Apr+2017#c133550
https://github.com/travis-ci/travis-ci/issues/5008#issuecomment-291185634

Comment 9

[Ed Morley [:emorley]]

Filed:
https://help.heroku.com/tickets/465797

(Continuation of https://help.heroku.com/tickets/292648)

Ask if access desired.

Comment 10

[Ed Morley [:emorley]]

(In reply to Ed Morley [:emorley] from comment #9)
> Filed:
> https://help.heroku.com/tickets/465797

Ticket contents (pleasantries trimmed):

"""
The "auto-deploy from GitHub when CI passes" feature triggers a Heroku deployment in response to receiving a GitHub webhook event of type X-GitHub-Event: status.

Unfortunately this feature currently assumes that there will never be duplicate events reported by GitHub, which is not the case.

As such, if multiple events are received from GitHub for the same revision SHA in a row, Heroku will perform the auto-deploy multiple times unnecessarily. This happened again today on three of our apps:
https://dashboard.heroku.com/apps/treeherder-prod/activity
https://dashboard.heroku.com/apps/treeherder-stage/activity
https://dashboard.heroku.com/apps/treeherder-prototype/activity
(Note the repeated "Deployed 9332b6a")

The circumstances in which this duplicated GitHub event can occur, are:
1) A CI job is retriggered (eg retrigger an already green travis job on the branch that is set to auto-deploy)
2) The same SHA is pushed to multiple branches at once (since each branch will get it's own Travis job, which then all notify the GitHub status API about the same revision SHA).

This was previously reported in #292648, however since then I've realised that (2) isn't Travis' fault, since the GitHub status API only allows specifying a revision SHA, so there isn't actually any way for Travis to help here. In addition, there would still need to be changes on Heroku's side to handle (1) anyway.

As such, it would be great if the Heroku auto-deploy feature could de-duplicate incoming events, and not re-deploy a particular SHA if it's the same SHA as the last deploy. This would mean the behaviour would then match the UI text on the deploy settings page, which says "Wait for CI to pass before deploy", whereas the current behaviour would more correctly be described as "Deploy every time the CI passes".
"""

Comment 11

[Ed Morley [:emorley]]

Their reply in April:

"""
Thanks for excellent report. It's rare to get a ticket with such a clear illustration of the problem and the feature request.

Just so I'm clear on your need, are you commonly seeing these re-deploys? I would not expect users to re-build green travis builds often, and we don't commonly see users pushing to multiple branches at once. Do you perhaps have some automation that might be causing this to happen more often than we'd anticipate?
"""

I didn't get around to replying straight away, so they closed the ticket for inactivity:

"""
I haven't heard anymore about your use case, so I'm going to flag this ticket pending. I'll still bring up this issue with the team, though. Thanks for pointing it out and have a good week!
"""

I suspect this is wontfixed on their side, and with the new "squash and merge" github feature, I'm not using the workflow that causes this issue to surface as much, so we can probably just ignore this for now.