From 96a64b4982bf662ce6b6721c75cbb79783fd55b7 Mon Sep 17 00:00:00 2001 From: Naz Date: Mon, 27 Feb 2023 19:41:08 +0800 Subject: [PATCH] Added error rethrow to update check run as a job closes https://github.com/TryGhost/Toolbox/issues/525 - When the update check is run as an offloaded job (in a worker thread) the errors that are handled over "logger.error" do not get bubbled up to the job manager to be handled fully. With an optional "rethrowErrors" flag it's not possible to throw errors when the update check is run as a "job" and handle as usual when run in the main thread (through the admin route trigger) --- ghost/core/core/server/run-update-check.js | 4 ++- ghost/core/core/server/update-check.js | 8 +++-- .../lib/update-check-service.js | 10 ++++++- .../test/update-check-service.test.js | 29 ++++++++++++++++++- 4 files changed, 45 insertions(+), 6 deletions(-) diff --git a/ghost/core/core/server/run-update-check.js b/ghost/core/core/server/run-update-check.js index 23b3c9aa895..4045d7d6a3d 100644 --- a/ghost/core/core/server/run-update-check.js +++ b/ghost/core/core/server/run-update-check.js @@ -42,7 +42,9 @@ if (parentPort) { await settings.init(); // Finished INIT - await updateCheck(); + await updateCheck({ + rethrowErrors: true + }); postParentPortMessage(`Ran update check`); diff --git a/ghost/core/core/server/update-check.js b/ghost/core/core/server/update-check.js index 9c227590efc..347fdfd61ee 100644 --- a/ghost/core/core/server/update-check.js +++ b/ghost/core/core/server/update-check.js @@ -12,10 +12,11 @@ const UpdateCheckService = require('@tryghost/update-check-service'); /** * Initializes and triggers update check - * + * @param {Object} [options] + * @param {Boolean} [options.rethrowErrors] - if true, errors will be thrown instead of logged * @returns {Promise} */ -module.exports = async () => { +module.exports = async ({rethrowErrors = false} = {}) => { const allowedCheckEnvironments = ['development', 'production']; // CASE: The check will not happen if your NODE_ENV is not in the allowed defined environments. @@ -51,7 +52,8 @@ module.exports = async () => { notificationGroups: config.get('notificationGroups'), siteUrl: urlUtils.urlFor('home', true), forceUpdate: config.get('updateCheck:forceUpdate'), - ghostVersion: ghostVersion.original + ghostVersion: ghostVersion.original, + rethrowErrors }, request, sendEmail: ghostMailer.send.bind(ghostMailer) diff --git a/ghost/update-check-service/lib/update-check-service.js b/ghost/update-check-service/lib/update-check-service.js index 8ec3fe07d44..35ceef5634d 100644 --- a/ghost/update-check-service/lib/update-check-service.js +++ b/ghost/update-check-service/lib/update-check-service.js @@ -45,6 +45,7 @@ class UpdateCheckService { * @param {string} options.config.checkEndpoint - update check service URL * @param {boolean} [options.config.isPrivacyDisabled] * @param {string[]} [options.config.notificationGroups] - example values ["migration", "something"] + * @param {boolean} [options.config.rethrowErrors] - allows to force throwing errors (useful in worker threads) * @param {string} options.config.siteUrl - Ghost instance URL * @param {boolean} [options.config.forceUpdate] * @param {string} options.config.ghostVersion - Ghost instance version @@ -88,6 +89,10 @@ class UpdateCheckService { err.help = tpl(messages.checkingForUpdatesFailedHelp, {url: 'https://ghost.org/docs/'}); this.logging.error(err); + + if (this.config.rethrowErrors) { + throw err; + } } /** @@ -330,7 +335,10 @@ class UpdateCheckService { forceTextContent: true }); } catch (err) { - this.logging.err(err); + this.logging.error(err); + if (this.config.rethrowErrors) { + throw err; + } } } } diff --git a/ghost/update-check-service/test/update-check-service.test.js b/ghost/update-check-service/test/update-check-service.test.js index f85e3c620b4..2f8c5277cd0 100644 --- a/ghost/update-check-service/test/update-check-service.test.js +++ b/ghost/update-check-service/test/update-check-service.test.js @@ -3,6 +3,7 @@ require('./utils'); const sinon = require('sinon'); const moment = require('moment'); const uuid = require('uuid'); +const assert = require('assert'); const logging = require('@tryghost/logging'); const UpdateCheckService = require('../lib/update-check-service'); @@ -371,7 +372,8 @@ describe('Update Check', function () { settings: { edit: settingsStub } - } + }, + config: {} }); updateCheckService.updateCheckError({}); @@ -381,5 +383,30 @@ describe('Update Check', function () { logging.error.args[0][0].context.should.equal('Checking for updates failed, your site will continue to function.'); logging.error.args[0][0].help.should.equal('If you get this error repeatedly, please seek help from https://ghost.org/docs/'); }); + + it('logs and rethrows an error when error with rethrow configuration', function () { + const updateCheckService = new UpdateCheckService({ + api: { + settings: { + edit: settingsStub + } + }, + config: { + rethrowErrors: true + } + }); + + try { + updateCheckService.updateCheckError({}); + assert.fail('should have thrown'); + } catch (e) { + settingsStub.called.should.be.true(); + logging.error.called.should.be.true(); + logging.error.args[0][0].context.should.equal('Checking for updates failed, your site will continue to function.'); + logging.error.args[0][0].help.should.equal('If you get this error repeatedly, please seek help from https://ghost.org/docs/'); + + e.context.should.equal('Checking for updates failed, your site will continue to function.'); + } + }); }); });