Fix occasional 500 when reading unread forum topics

[purfectliterature]

Closes #6691.

This has been a long-standing problem when reading unread forum topics. Occasionally, reading an unread forum topic will crash the browser because the server responds 500. Refreshing the tab will show the forum topic and correctly mark it as read.

The error comes from topics_controller.rb:20 when @topic.mark_as_read!. The raised error is an ActiveRecord::RecordNotUnique, and the full error is as follows.

ActiveRecord::RecordNotUnique (PG::UniqueViolation: ERROR:  duplicate key value violates unique constraint "read_marks_reader_readable_index"
DETAIL:  Key (reader_id, reader_type, readable_type, readable_id)=(21181, User, Course::Forum::Topic, 25977) already exists.

This error came about after ledermann/unread adds support for maintaining unique read marks in 2016 by ledermann/unread#78. A unique: true constraint is added to their migration, and that explains why the error above is originally raised by PostgreSQL.

It is an issue for us now because there are 3 calls made to topics_controller#show at roughly the same time. Even though GET calls are supposed to be idempotent, topics_controller#show is notably not idempotent because @topic.mark_as_read! can fail. It fails because each request hasn't seen the read mark for the @topic, and so attempts to create a new read mark for it. Since the database only allows synchronous operations, the first request's read mark creation succeeds, but the following requests' creations fail because the same read mark already exists, hence raising the PG::UniqueViolation error. Sometimes it doesn't raise this error because the following requests take place strictly after the first request actually responds. Hence, occasional.

You can replay this error by usurping ledermann/unread's mark_as_read! and removing if unread?(reader) and changing rm to only read_marks.build so that it always tries to create a new read mark. Running mark_as_read! on a read forum topic now will always raise ActiveRecord::RecordNotUnique.

Solution

Notably, we need to ensure that GET calls are idempotent no matter what. This also means that mark_as_read! (with or without the bang) must always be idempotent. There's no reason why mark_as_read! should fail if the read mark exists. It simply means that the readable is read.

This PR approaches the problem in a non-disruptive manner by creating a new SafeMarkAsReadConcern that includes a safely_mark_as_read! method. safely_mark_as_read! will just run mark_as_read!, but if it catches ActiveRecord::RecordNotUnique, it will double-check if the item is indeed read already in the database (in 1-2 SQL calls). If not, it will re-raise the error, because that is unexpected.

I'm not including SafeMarkAsReadConcern everywhere for now because this is rather a hack, and we haven't had complaints in other places so far. If we so believe that this concern is the right way to go, we can easily include it respectively in the future. That's why I wrote it this way, i.e., a model concern.

[ekowidianto]

Thanks!