Page MenuHomePhabricator
Create Task
Maniphest T10709

incoming email handler replies multiple times for a single unrecognized command
Closed, ResolvedPublic
Actions

Description

steps to reproduce:

  1. reply to an email from Audit with !accept (trying to accept a commit, like you would with Differential) (the email is triggered by Herald rules)

expect:

  • commit accepted, but I'll take "unrecognized command" once too

actual:

  • multiple failure replies over several hours

reproduced on phacility instance, discovered by coworker, confirmed by me

Screen Shot 2016-04-02 at 16.30.34.png (1×1 px, 143 KB)

Revisions and Commits

rP Phabricator
D15575rPfea2389066ed (stable) Improve error and header behaviors for Mailgun received mail webhook
D15575rPa8c9a5597d64 Improve error and header behaviors for Mailgun received mail webhook

Related Objects

Event Timeline

allixsenos created this task.Apr 2 2016, 2:31 PM
allixsenos updated the task description. (Show Details)
epriestley claimed this task.Apr 2 2016, 3:07 PM
epriestley triaged this task as Normal priority.
epriestley added a project: Mail.
epriestley added a comment.Apr 2 2016, 3:10 PM

I think this is happening:

  • The upstream mail provider is submitting mail to us via webhook.
  • We're processing it, but failing to handle/resolve the exception arising from the invalid command properly.
  • Because the exception escapes, we don't return HTTP 200.
  • The upstream provider interprets this as a failed delivery and re-transmits the mail after a delay.

I think this is fairly straightforward to resolve.

epriestley added a comment.Apr 2 2016, 5:25 PM

We have an additional safety check which is supposed to prevent this kind of issue: we refuse to process messages with the same "Message-ID" more than once.

However, I think MailGun either changed their API or we never implemented this correctly in the first place, as we don't seem to be reading headers properly. Since we never read the "Message-ID", the check for duplicates fails. I'll resolve this, too.

epriestley added a revision: D15575: Improve error and header behaviors for Mailgun received mail webhook.Apr 2 2016, 5:36 PM
epriestley added a commit: rPa8c9a5597d64: Improve error and header behaviors for Mailgun received mail webhook.Apr 2 2016, 5:51 PM
epriestley added a commit: rPfea2389066ed: (stable) Improve error and header behaviors for Mailgun received mail webhook.Apr 2 2016, 5:53 PM
epriestley closed this task as Resolved.Apr 2 2016, 6:01 PM

This should now be resolved in production.

Although this isn't a hugely critical issue and I probably wouldn't hotfix it normally, it's an issue that's hard to be sure about the fix for without verifying it in production, and it's still fairly early on Saturday morning, and it would have to wait a whole week to deploy normally. So:

  • I cherry-picked this to stable (as rPfea2389066ed).
  • I deployed stable to production.
  • I verified that sending an invalid command responds with exactly one reply.
  • I verified that inbound mail now gets proper "Message-ID" headers parsed.
  • (And I verified that normal inbound mail works properly.)

If you have existing threads that are still stuck retransmitting, you may receive one additional copy of the error mail (the queued retry on the upstream side), but it should stop after that. New mail should behave correctly.

Thanks for the report! Let us know if you run into anything else.

epriestley mentioned this in T11512: Add error handling to email commands.Aug 22 2016, 9:02 PM
Phabricator · Built by Phacility