Having N application servers will result in N comments for "Reply by email" messages

The main discussion has moved to https://github.com/tpitale/mail_room/issues/46.

If we have N application servers, we have N mail_room processes watching the single IMAP mailbox, 1 message received N times, N jobs pushed onto the Sidekiq Redis queue, N Sidekiq workers processing the message, and N comments being created.

I don't think it's possible to make it so that a specific message is only handled by one mail_room, since the IMAP inbox isn't a (threadsafe) queue, and doesn't have the locking/atomicity tools to allow it to be coerced into behaving like one.

Solving this on the GitLab side would be doable, if we save the email's Message-ID for a created comment, and add a unique index on the combination of Message-ID and the author, so we don't create two comments for the same message. This would result in a high load on the DB server though, if 48 workers all try to insert the same comment.

@jacobvosmaer and I spent a little while trying to think of ways to solve this using a Redis queue or set, but couldn't come up with a solution that wasn't still likely to have race conditions.

We could of course use Redis to go from 48 messages to be processed to a minimal amount of them (in which case some race conditions would be okay), to greatly diminish the load on Postgres which would still have the unicity constraint in place as a final check, but I wonder if this adds too much complexity for too little gain.

If we can get Redis to do a Compare-and-Swap, which appears to be possible using transactions and the WATCH command per this, we can use that to set a boolean key in Redis like mail_room/processed/[Message-ID] to see if this mail_room is the first mail_room that tries to process the message with this Message-ID. We can set the key to expire after a number of minutes or hours so Redis doesn't get too full.

(I am repeating some of what Douwe already said.)

I think we need to stop the problem as early as possible. Having 48 mail_rooms create 48 Sidekiq jobs, 47 of which fail because of a unique index on the Message-ID in the SQL database would work but it solves the problem 'too late' I think.

Ideally we should solve the problem in mail_room using Redis, by preventing the same Message-ID from going into the Sidekiq queue twice. On the other hand we need to be careful not to globally lock Redis every time mail_room processes a new email.

@jacobvosmaer What do you think about the Redis CAS idea?

On the other hand we need to be careful not to globally lock Redis every time mail_room processes a new email.

Right. We'll have to investigate how (if it all) this scheme affects other Redis keys, like those for Sidekiq, sessions, and caches.

@DouweM CAS example from antirez sounds good. Reading about Redis transactions it sounds like INCR is atomic. Maybe that is already enough.

key = Message-ID
if INCR(key) != 1
  return # another mail_room is handling this email
end
EXPIRE(key, 1 hour)
# send job to Sidekiq
# delete message from IMAP

@jacobvosmaer Nice, that does sound like it would work.

@jacobvosmaer Is INCR really atomic? This comment begs to differ:

The page should be more explicit that INCR is NOT ATOMIC. The phrase "Using other atomic increment/decrement commands like DECR or INCRBY it is possible..." is ambiguous. I had read previous pages that were mistaken on this point, which made me parse this sentence as though INCR, DECR, and INCRBY are all in the same cohort of atomic methods. I'm now in the process of fixing a (minor, fortunately) production issue that arose as a result of this. Why ISN'T INCR atomic?

Created https://github.com/tpitale/mail_room/issues/46 to discuss this with the author of mail_room.

@DouweM comments in https://code.google.com/p/redis/wiki/IncrCommand suggest INCR is atomic. I saw that one comment on http://redis.io/commands/incr too, but I cannot find anything to corroborate what they say.

@jacobvosmaer

Milestone changed to 8.1

I think what's needed here is a Redis distributed lock (see this and this).

The locking can be done in 2 places:

1. at pooling level: the server which acquires the lock pools for new emails. the other servers check for the lock periodically and when another server acquire the lock (probably when the lock owner crashes or something) it starts pooling
2. at message level: all server pools messages but only one server can acquire the lock to send the message to sidekiq

I think the optimal solution is 1. I'm thinking of some edge cases (for example when restarting on deploy) where messages might be lost or messages being processed multiple times so probably both locking solutions above should be implemented.

Another solution would be to declare only one server as gitlab_mail_fetcher and only that server starts the mail_root fetcher but that is pretty complicated to enforce as deploy solutions vary a lot.

@cristianbica what does a Redis distributed lock add that is not already in the INCR-based solution proposed above?

@jacobvosmaer from what I'm reading shouldn't be a difference (except the extremely rare case where the process will crash before calling expire). On the other hand the recommended Redis' way to do locking is SETNX.

Anyway the locking mechanism can be tested and the implementation changed anytime if an issue is discovered with any of the implementations. I'm interested on what you guys think about my proposal on when the apply the locking/unlocking logic.

@cristianbica the main discussion is in https://github.com/tpitale/mail_room/issues/46 now.

FWIW I think locking per message is better. It creates more noise in Redis but if one mail_room process hangs without releasing its locks, the others will keep on going without having to wait for a lock timeout. With a global lock all email processing would stop when one mail_room hangs.

Reassigned to @DouweM

mentioned in merge request !1566 (merged)

Status changed to closed by commit 82da19ce

mentioned in commit 82da19ce

mentioned in merge request !3143 (merged)

@cristianbica I am rereading your comments now and asking myself why I did not pick up on what you said about SETNX. Please consider this a late 'thanks' for pointing it out.

For future reference I must add that I now understand that the nicer solution (nicer than INCR/EXPIRE) is to us a single SET key value NX EX timeout, which is not strictly the same as SETNX. This information was there in the sources @cristianbica pointed to.