From e1c2fcd1f1a8bb9e3bbdc0db503172635d1177ff Mon Sep 17 00:00:00 2001 From: John McLear Date: Mon, 27 Jul 2026 20:03:26 +0100 Subject: [PATCH] perf: don't log settings.loadTest warning per-message (#7756) (#7776) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * perf: don't log settings.loadTest warning per-message (#7756) CPU profile of develop (and of the open #7775 branch) at the 100-400 author dive sweep attributed ~4% of total process CPU to log4js inside SecurityManager.checkAccess. Tracing the actual log call: line 79-80 emits `console.warn('bypassing socket.io authentication...')` on every checkAccess invocation when settings.loadTest is true — once per inbound message. With log4js's replaceConsole + cluster-mode dispatch enabled, that warning allocated, formatted, and dispatched a LogEvent through sendToListeners -> sendLogEventToAppender for every CLIENT_READY, COMMIT_CHANGESET, USERINFO_UPDATE, etc. settings.loadTest is a configuration choice, not a per-request condition. The warning belongs at startup. Move it to Settings.ts init alongside the other "you set X, beware" warnings, and drop the per-message branch (the loadTest short-circuit still applies). Test plan: - tests/backend/specs/api/sessionsAndGroups.ts: 32 passing - tests/backend/specs/socketio.ts: 39 passing (handleMessage paths) Co-Authored-By: Claude Opus 4.7 (1M context) * fixup: address Qodo review on #7776 Three issues flagged: 1. Indentation: outdented the continuation lines inside the new `if (settings.loadTest)` block from 10 spaces to 8 (one level from `logger.warn(`), matching 2-space indent rule for added code. 2. Warning scope: the original wording said only socket.io authn/authz is bypassed, but settings.loadTest short-circuits SecurityManager.checkAccess() which is called from both HTTP (padaccess, importexport) and socket.io (PadMessageHandler) paths. Reword to "SecurityManager.checkAccess() will bypass authentication and authorization for both HTTP and socket.io requests". 3. Misleading "fires once at startup" comment in SecurityManager.ts: the warning is logged from Settings.ts reloadSettings(), which is also called on admin restart and plugin install. Rephrase to "logged from Settings.ts during settings load/reload, not on every request". All three issues are accurate. No behaviour change for the fix itself; only comment + warning text. Co-Authored-By: Claude Opus 4.7 (1M context) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- src/node/db/SecurityManager.ts | 11 +++++++---- src/node/utils/Settings.ts | 7 +++++++ 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/node/db/SecurityManager.ts b/src/node/db/SecurityManager.ts index 219d3f2be..33996c155 100644 --- a/src/node/db/SecurityManager.ts +++ b/src/node/db/SecurityManager.ts @@ -75,10 +75,13 @@ exports.checkAccess = async (padID:string, sessionCookie:string, token:string, u } // Authentication and authorization checks. - if (settings.loadTest) { - console.warn( - 'bypassing socket.io authentication and authorization checks due to settings.loadTest'); - } else if (settings.requireAuthentication) { + // settings.loadTest just short-circuits authn/authz; the user-facing + // warning about this configuration choice is logged from Settings.ts + // during settings load/reload, not on every request. Re-logging it + // here was costing ~4% of process CPU in the 100-400 author dive + // sweep (#7756): the routed-console-warn went through log4js's + // clustering dispatch on every message. + if (!settings.loadTest && settings.requireAuthentication) { if (userSettings == null) { authLogger.debug('access denied: authentication is required'); return DENY; diff --git a/src/node/utils/Settings.ts b/src/node/utils/Settings.ts index b0762b72f..f1ed4e4e0 100644 --- a/src/node/utils/Settings.ts +++ b/src/node/utils/Settings.ts @@ -1266,6 +1266,13 @@ export const reloadSettings = () => { logger.warn("logLayoutType: " + settings.logLayoutType); initLogging(settings.logconfig); + if (settings.loadTest) { + logger.warn( + 'settings.loadTest is true: SecurityManager.checkAccess() will bypass ' + + 'authentication and authorization for both HTTP and socket.io requests. ' + + 'Do NOT enable this in production.'); + } + if (!settings.skinName) { logger.warn('No "skinName" parameter found. Please check out settings.json.template and ' + 'update your settings.json. Falling back to the default "colibris".');