ref
https://linear.app/tryghost/issue/ENG-1240/race-condition-when-updating-members-last-seen-at-timestamp
When members click a link in an email, Ghost updates the member's
`last_seen_at` timestamp, but it should only update the timestamp if the
member hasn't yet been seen in the current day (based on the
publication's timezone).
Currently there is a race condition present where multiple simultaneous
requests from the same member (if e.g. an email link checker is
following all links in an email) can cause the `last_seen_at` timestamp
to be updated multiple times in the same day for the same member. These
additional queries add a significant load on Ghost and its database,
which can contribute to the exhaustion of the connection pool and
eventually requests may time out.
The primary motivation for this change is to avoid that race condition
by adding a lock to the member row, checking if `last_seen_at` has
already been updated in the current day, and only updating it if it
hasn't.
Another beneficial side-effect of this change is that it avoids locking
the `labels` and `newsletters` tables, which are locked when we update
the `last_seen_at` timestamp in the `members` table currently. This
should improve Ghost's ability to handle a large influx of requests to
redirect endpoints (confirmed with load tests), which tend to happen
immediately after a publisher sends an email.
As discussed with the product team we want to enforce kebab-case file names for
all files, with the exception of files which export a single class, in which
case they should be PascalCase and reflect the class which they export.
This will help find classes faster, and should push better naming for them too.
Some files and packages have been excluded from this linting, specifically when
a library or framework depends on the naming of a file for the functionality
e.g. Ember, knex-migrator, adapter-manager
refs https://ghost.slack.com/archives/C02G9E68C/p1670960248186789
This reverts a change that was made here:
f4fdb4fa6c (r93071549),
but it still moved the original code to a new location in the
LastSeenAtUpdater
It includes a new E2E test to make sure timezones are supported
correctly.
- By not using Bookshelf, we no longer fire webhook calls
- By not using the member repository, we don't fetch and update the
member model and the labels relation in a forUpdate transaction, which
caused deadlock issues on the labels/members_labels tables which were
hard to resolve. Until now I was unable to find the other conflicting
transaction that caused this deadlock. Moving to raw knex (instead of
Bookshelf) and only updating the last_updated_at column should remove
the deadlock issue.
This removed the test for the email service wrapper, since it started
failing for an unknown reason and the test didn't make much sense (was
added earlier only to bump test threshold).
refs https://ghost.slack.com/archives/C02G9E68C/p1670215917451249
When a member is deleted, and we receive an opened event for an email to
that member. We threw an uncaught Bookshelf EmptyResponse error.
- This change makes fetching the member not a requirement when handling
that event in the last seen at updater.
- It also adds try catches for all event listeners in the last seen at
updater
fixes https://github.com/TryGhost/Team/issues/2310
This moves the processing of the events from the event-processor to a
new email-event-processor in the email-service package.
- The `EmailEventProcessor` only translates events from
providerId/emailId to their known emailId, memberId and recipientId, and
dispatches the corresponding events.
- Since `EmailEventProcessor` runs in a separate worker thread, we can't
listen for the dispatched events on the main thread. To accomplish this
communication, the events dispatched from the `EmailEventProcessor`
class are 'posted' via the postMessage method and redispatched on the
main thread.
- A new `EmailEventStorage` class reacts to the email events and stores
it in the database. This code mostly corresponds to the (now deleted)
subclass of the old `EmailEventProcessor`
- Updating a members last_seen_at timestamp has moved to the
lastSeenAtUpdater.
- Email events no longer store `ObjectID` because these are not
encodable across threads via postMessage
- Includes new E2E tests that test the storage of all supported Mailgun
events. Note that in these tests we run the processing on the main
thread instead of on a separate thread (couldn't do this because
stubbing is not possible across threads)
There are some missing pieces that will get added in later PRs (this PR
focuses on porting the existing functionality):
- Handling temporary failures/bounces
- Capturing the error messages of bounce events
- renames `refSource`, `refMedium` and `refUrl` to `referrerSource`, `referrerMedium` and `referrerUrl` respectively for consistent naming across files and usages
fixes https://github.com/TryGhost/Team/issues/1952
Adds a new MemberLinkClickEvent event that is fired when a member clicks a link. This code has been added to the `linkClickRepository` because that is the only place that has access to the member model (and the event requires the id and current last seen at value). The LastSeenAtUpdater listens for this event and updates the timestamp if required.
fixes https://github.com/TryGhost/Team/issues/1821
This change moves all the event storage logic to one new place: the event storage class in the MembersEventsService, which is initialised in a new members events service wrapper.
Apart from this, this includes some improvements:
- Removed DomainEvents from the constructor arguments to the subscribe method (to make it more clear where to subscribe to and decrease dependencies)
- LastSeenAtUpdater doesn't subscribe in the constructor any longer (removes unclear side effect)
- Moved LastSeenAtUpdater initialisation to new members events service wrapper
- Added missing tests to LastSeenAtUpdater to assure that the MembersEventsService package has 100% coverage.
refs https://github.com/TryGhost/Team/issues/1717
- Updates last_commented_at and last_seen_at (only once a day)
- Used the LastSeenAtUpdater, so we can combine updating last_commented_at and last_seen_at in one query + used same pattern
- Updated comments service to await emails in order to make E2E tests more stable (as we don't have any method to await emails and test emails otherwise). This removed the email sending logic from the `onCreated` hook of the model.
refs https://github.com/TryGhost/Team/issues/1577
The call to `edit` was not loading the newsletter relations which is needed
by the serializer used by the webhooks service.
Co-authored-by: Fabien "egg" O'Carroll <fabien@allou.is>
refs https://github.com/TryGhost/Ghost/pull/14197
- Uses the right method to update a model (`edit`)
- Also fixes the `updateLastSeenAt` comment that wasn't reflecting the code
refs https://github.com/TryGhost/Ghost/pull/14197
- Using the package directly was creating a second instance and was never triggering the subscriber
- Passing DomainEvents as a dependency solves this issue
refs https://github.com/TryGhost/Team/issues/1306
- This removes the limitation described in commit ff46449ad6
- The only edge case is that when a publication changes their timezone, it will have maximum 24 hours where the member last_seen_at could be incorrect
refs https://github.com/TryGhost/Ghost/pull/14197
- Moved from updating the last_seen_at value "at most every 24h" to "at most every UTC day".
- It will simplify explaining the following behavior: a publication is set in UTC-10, a user visits at 2pm on Monday and at 1pm on Tuesday, the last_seen_at value is still Monday.
- There is no way to go around the above issue due to the technical constraint of updating the `last_seen_at` value at most once a day.
- This might create database write spikes at midnight UTC
refs https://github.com/TryGhost/Team/issues/1306
- Contains all services that listen on member events
- Only contains the last-seen-at-updater service for now
- Listens for `MemberViewEvent` events to update the `member.last_seen_at` timestamp
- Updates after 24hours of the last timestamp to avoid too many writes
- Also updates when the value is NULL
- This is using the existing `last_seen_at` value to avoid an SQL query when no writes are required
