- Bottom line - we need to manage shutting down gracefully when doing long-running tasks
- To achieve that, we're going to use job queues
In this commit:
- added new @tryghost/job-manager dependency
- added a minimal job service, that handles in passing things like logging and (maybe later) config
- job service is wired up to server shutdown, so that the queue finishes before the server exits
- also added a new job endpoint to testmode so that it's easy to test job behaviour without needing to do real work
- Using consistent patterns for shutdown and stop
- Make it clear what each method does
- Use async/await to make the code more readable and simple
- This lays groundwork for having more cleanup tasks in stop than just server.stop()
- deleted files under `core/server/lib/promise` and related test files
- added `@tryghost/promise` as a dependency
- fixed all local requires to point to the new package
closes#12118
- server.start was mistakenly removed in 71f02d25e9
- it is used for loading themes (and other things) and is critical
- added tests to prevent this regressing again in future
no issue
- New Member API batched import is meant to be a substitution to current import
with improved performance while keeping same behaviore. Current
import processes 1 record at a time using internal API calls and times
out consistently when large number of members has to be imported (~10k
records without Stripe).
- New import's aim is to improve performance and process >50K
records without timing out both with and without Stripe connected
members
- Batched import can be conceptually devided into 3 stages which have
their own ways to improve performance:
1. labels - can be at current performance as number of
labels is usually small, but could also be improved through batching
2. member records + member<->labels relations - these could
be performed as batched inserts into the database
3. Stripe connections - most challanging bottleneck to solve because
API request are slow by it's nature and have to deal with rate limits of
Stripe's API itself
- It's a heavy WIP, with lots of known pitfalls which are marked with
TODOs. Will be solved iteratively through time untill the method can be
declared stable
- The new batched import method will be hidden behind 'enableDeveloperExperiments' flag to
allow early testing
- A simple way to test behaviours without having to do complex interactions to e.g. generate errors or slow requests
- Makes it easier to test the new shutdown behaviour, among other things
- there can now be quite a big delay between SIGINT/TERM being received and shutdown finishing
- add an extra log message to acknowledge the SIGINT/TERM to facilitate debugging and just be clear
- stopppable is a dependency that handles closing connections properly, which server.close does not
- active connections are allowed to complete what they are doing
- idle connections are closed
- no new connections are allowed
- we call stoppable in stop() instead of server.close so that idle connections don't hold the server open
- calling await stop() from shutdown then ensures that we have a consistent experience of stop
- all together this allows ghost to shutdown gracefully when there are long-running requests
- @TODO: handle graceful shutdown of long-running processes
- @TODO: consider do we need to send 503s whilst the server is shutting down?
- changed method to logStopMessages, as we use start and stop, not start and shutdown
- changed logStopMesasges to output the "proper" messages and use this method consistently - the closing connections message isn't really useful
- changed uptime message to always be output cos I can't see a case where there isn't interesting/useful
- Connection handling is legacy code added in 1438278ce4
- Although we were tracking all connections in memory, we weren't actually closing any because stop isn't called
- This (and restart) were both added as part of the now long-deprecated system for using Ghost directly as an npm module
- If we want to close connections cleanly, we should use a tool to do this
- the announce functions exist for the purpose of communicating with Ghost CLI
- the functions were called announceServerStart and announceServerStopped, which implies they tell Ghost CLI when the server starts and stops
- however, the true intention / purpose of these functions is to:
- either tell Ghost CLI when Ghost has successfully booted (e.g. is ready to serve requests)
- or tell Ghost CLI when the server failed to boot, and report the error so that Ghost CLI can communicate it to the user
- therefore, I've refactored the old functions into 1 function to make it clearer they do the same job, but with 2 different states
- also added some tests :D
refs 022a433e56
- noticed I missed adding debug info in one place (not that it's used, just for consistency)
- also made the SIGINT/TERM code slightly more readable
no-issue
- switch from `membersService.api.members.list` to using bookshelf `Member.findPage()` with the `{paid: true}` filter to avoid per-member queries (N+1) to decorate members with subscriptions and a heavy post-fetch filter via `contentGating`
- add concurrency to the Mailgun API requests in `bulk-email` service to reduce overall time submitting API requests
- add debug statements with timing output for easier measurements
no issue
For large numbers of members we're unable to perform queries for free/paid members in a reasonable time without indexes. Bulk delete is also very slow when looping through bookshelf models and having bookshelf manage deletion of child records, adding `ON DELETE CASCADE` to the foreign key indexes moves child deletion to the DB level and allows for performant bulk delete queries.
- migrations only run on mysql because sqlite does not support altering tables
- adds foreign key indexes and constraints with 'ON DELETE CASCADE' for members_labels, members_stripe_customers, and members_stripe_customers_subscriptions
refs #12100
For performance reasons we want to add foreign key and unique constraints
to the members_stripe_* tables so we can utilised cascading deletes and
joins across the tables when querying.
In order to do this we must first ensure that:
- There are no duplicate entries in the `subscription_id` or `customer_id` columns
- There are no orphaned rows in the subscription or customers tables
If the first is not true, the unique constraint will fail, and if the second is not true,
the foreign key constraint will fail.
As we are only adding the indexes to existing MySQL databases at this point, the
cleanup migrations will also only be done for existing MySQL databases too.
The migrations for removing orphaned rows splits the deletion into a `SELECT`
followed by a `WHERE IN` to avoid the database "optimising" the query into a
`JOIN` which ends up taking much longer due to the lack of indexes.
no-issue
We are in the process of creating migrations to add foreign key constraints
and cascading deletes to the members_stripe_* tables to make listing members
and deleting members faster. As well as the migrations we need to update the
database schema so that new installations have the correct indexes and constraints.
Co-authored-by: Kevin Ansfield <kevin@lookingsideways.co.uk>
refs 173e3292fa
- The bug was initially introduced in referenced commit. When request is done with `api_key` context, there should always be an `integration` object associated with it - 71c17539d8/core/server/services/permissions/parse-context.js (L36) . An `id` from `context.integration` not `context.api_key` has to be assigned to newly created webhook!
- The webhooks API is about to be declared stable in upcoming release, so no migration will be done
closes#11907
The image in the bookmark card was being shown out of the bounds of
the card because of a general style `height: auto !important`.
I added a new `max-height` property to the image to avoid exceeding
parent height.
no issue
- Webhooks API has been stabilized with latest changes and there are no breaking changes planned for v3. The change has strictly "informative" purpose
- Changed variable naming from "whitelisted" to "allowlisted" to follow updated naming convention (refs. https://mysqlhighavailability.com/mysql-terminology-updates/)
refs #12064
- `critical` is meant to be something unpredictable like internal error, something worthy attention, as described in Ignition -3439456d94/README.md (list-of-errors)
- This error level was introduced with - this PR https://github.com/TryGhost/Ghost/pull/9426, but there is no context provided why this specific value was used. Assuming it's an outdated value as 'not found' is nowhere to be treated in any special way
no issue
- `mailgun()` expects the `host` option not to include a port but `url.host` will include the port, we instead want to use `url.hostname` which skips the port
no issue
- bookshelf adds `DISTINCT` to any relation query that does not have an explicit `columns` statement
- when measuring the impact of `DISTINCT` on the eager-loading association query when listing members using `{withRelated: 'labels'}`, it can be 2x slower with no index on the sort_order column or 4x slower with an index on sort_order
no issue
- When processing entries with new labels in parallel Bookshelf relations is trying to create them which caused unique key constraints to fail. To avoid the failure, all labels should be pre-created before proceeding with creating members
- Member limit code was duplicated in 2 places unnecessarily
- Also used member api code that fetched members and subscriptions fully hyrated when we only need a count
- Using a raw query significantly improves performance here
no-issue
This updates the Admin API Member resource to *not* cancel subscriptions
by default, and adds a `cancel` option. This can be used over HTTP by
including a `cancel=true` query parameter.
- mailgun has a testmode flag we can use to get email to be accepted but not delivered
- this is useful for developers testing general bulk email code - not for users - so it is only available via config
no-issue
Up until now we have left orphaned rows in members_stripe_* tables when
a member is deleted, this updates the destroy method so that we cascade
and remove any MemberStripeCustomer and StripeCustomerSubscription
models related to the Member.
This also adds regression tests for the new functionality as well as to
confirm the existing functionality of cascading to the members_labels
join table
This adds the relations of Subscription->Customer & Customer->Member
refs https://github.com/TryGhost/members.js/issues/72
- Portal is using using publication logo from settings for signup/signin pages
- Instead, we are switching to using publication icon from settings, which also needs to be passed in site data API
no issue
Having all members created during an import labelled with a specific "import label" is useful for later operations such as bulk delete/edit or simply recording how and when a member was created.
- automatically create a label with the date/time the members CSV import occurred and assign it to all imported members
- return the import label data in the API response so that clients can react accordingly such as automatically filtering the members list by the label once an import finishes
refs #12074
Since we've split members settings into multiple keys the
reconfiguration of the members-api has been happening in quick
succession as the stripe_connect_* settings are all set at once.
This debounces the call to reconfigure the members-api so that we only
need to instantiate it once.
no issue
- By default, GhostMailer throws EmailError with statusCode as `500` for any failure in sending mail
- In case of failure due to `RecipientError`, status code as now correctly sent as `400` as its a bad request and not an error we can't handle.
closes#12049
Stripe plans used to default to 0, and our new validation of plan
amounts were causing issues when importing from an older version of
Ghost, this updates the validation to be skipped when importing.
- Added regression test for importing plans
refs https://github.com/TryGhost/Ghost/issues/11971
- Added statusCode from bulk email provider to API response
- Updated error messages for different bulk email(mailgun) failure states
- Added `context` to preview mail API error message with mail provider's error message
closes#12033
- Added webhooks schemas and definitions.
- Added validation checking if integration_id is present when using session auth. This is needed to prevent orphan webhooks.
- Integrated webhook schemas into frame's validation layer.
- Added isLowerCase ajv keyword support. This is needed to be able to do isLowerCase validation using JSON Schema for webhooks.
closes#12001
* Moved settings validation to the model
This moves the settings validation out of the validation file and into
the model, as it is _only_ used there.
It also sets us up in the future for custom validators on individual
settings.
* Improved validation of stripe_plans setting
- Checks `interval` is a valid string
- Checks `name` & `currency` are strings
* Moved stripe key validation into model
The stripe key settings are all nullable and the regex validation fails
when the input is `null`. Rather than reworking the entirety of how we
validate with default-settings validation objects, this moves the
validation into methods on the Settings model.
* Added tests for new setting validations
Adds tests for both valid and invalid settings, as well as helpers
making future tests easier and less repetitive
