Duplicate Digest Email Incident Retro From January
Tags: incident, ruby, postmortem
Published: 2021-03-16 18:13:20

_Sorry all that this incident retro is a couple of months late. We did the retro internally immediately after the incident occurred and then I put it on my TODO list to make it public on DEV and that's where it stayed for far too long. 😬_

## Background Context

Over the past few months, we have been working hard to make the Forem setup more configurable for our creators. In an effort to do this we recently updated how we create a Forem's community name. This was done by merging the following PR that appended the `collective_noun`, if it was not disabled for the particular Forem, to the `SiteConfig.community_name` through a DataUpdateScript:

{% github https://github.com/forem/forem/pull/12055 %}

Data Update Script: 
```ruby
module DataUpdateScripts
  class AppendCollectiveNounToCommunityName
    def run
      return if SiteConfig.collective_noun_disabled || SiteConfig.collective_noun.blank?

      SiteConfig.community_name = "#{SiteConfig.community_name} #{SiteConfig.collective_noun}"
    end
  end
end
```


The PR nicely addresses and updates all places in the code where we were using `community_name` and therefore went out without any problems. 

However, 4 days later on Wednesday, December, 30th, we began to get reports of folks receiving duplicate digest emails. 

{% twitter 1344320876741861377 %}

I quickly jumped in and determined the problem to be an `Emails::EnqueueDigestWorker` that was continuously being restarted by deploys and sending the duplicate digest emails. 

## Bug Trigger*

* *NOTE: I did not label this section "Cause" because I believe the real, root cause of this problem came much earlier in the year. Scroll down for more details.*

The `Emails::EnqueueDigestWorker` is a worker that we have "turned off" for DEV because it takes too long to complete given the high number of users we have. We know it will keep restarting on deploys so we choose to use a rake task to send digest emails. To keep the worker from running we have a guard clause at the beginning of the worker that tells it to exit early if the environment is DEV. The guard clause at the time of the incident looked like this:

```ruby
return if SiteConfig.community_name == "DEV"
``` 

The problem was, when the above PR was deployed we changed `community_name` from "DEV" to "DEV Community" causing this guard clause to no longer work which allowed the worker to start running.

## Mitigation

The first thing I did to mitigate the number of those affected by this problem was to clear all of the existing digest worker jobs out of Sidekiq. Once that was done, I pushed out a quick fix to update the hardcoded string to the current community name by adding "Community" to it.
{% github https://github.com/forem/forem/pull/12082 %}

While this fixed the immediate issue, the hardcoded string is what got us into this jam in the first place. To prevent future problems I quickly followed up with a second, more resilient PR, that used a more robust method to check for the DEV environment. 

{% github https://github.com/forem/forem/pull/12083 %}

## Root Cause and Learnings

Even though the [the community name PR](https://github.com/forem/forem/pull/11846) was the trigger for this issue, it was far from the root cause. This issue was really caused by the poor decision by myself to hard code this string in the first place.

{% github https://github.com/forem/forem/pull/10070 %}

**Hardcoded strings, especially those for variables like this that can change, should be avoided at all costs when writing code.** At the very least you should be writing a method or using a CONSTANT to represent the string you want to check. This allows you to easily Find/Replace that method or CONSTANT in the future so it doesn't get lost in the shuffle as it did here. 

**The second problem was that I deployed that fix without a PR review.** I announced the problem in Slack and linked the PR, but based on the Slack message our database was in a dire state which is why I chose to expedite the fix. In the future, trying to get at least 1 PR review should always be the goal even if it means waiting an extra minute or two. Even if you don't immediately take the advice of the PR review, at least you can go back and update things later. 

The first fix that I pushed out in order to quickly update the digest worker worked but was un-ideal. This was pointed on in the PR review. I was still able to quickly push the hotfix but then immediately went back and wrote more thoughtful and robust code after. **That flow is how the initial bug should have been handled.** Our recently renewed commitment to sticking more to our processes now is what led to us fixing this problem correctly the second time around, while the first time we left the door open for future problems.

Finally, the more we can move away from this "DEV is a special snowflake" concept the better. If every environment is treated the same then you don't have to worry about edge cases popping up unexpectedly in production for specific environments. The final goal is to remove that guard clause altogether so every Forem sends its email digests the same way. Once again, this eliminates another special case solution that lowers the risk of future surprises. 

## Handling Hotfixes

**Always try to get a PR review even for hotfixes!** Most of us are not running life-saving software. If your site remains down for an extra min or two because you did the responsible thing and got a code review, it's not the end of the world. Having a good code review will decrease your chances of shipping unreliable code that could cause future issues. 

If a fix MUST go out immediately and you cannot get a review beforehand(maybe it's super late and no one is online), then **get a follow-up review.** Ask someone to review the code after the fact to make sure it is sound and is not so hacky as to lead to future problems.

## TL;DR

Don't hard code strings and get a PR review! And if all else fails, learn from your mistakes and strive not to make them again. Happy coding!