From 620c8b20cdc95b24d5d12fa2403788a6bd5ffb89 Mon Sep 17 00:00:00 2001 From: John McLear Date: Sat, 16 May 2026 11:22:00 +0100 Subject: [PATCH 1/2] perf: don't log settings.loadTest warning per-message (#7756) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/node/db/SecurityManager.ts | 11 +++++++---- src/node/utils/Settings.ts | 6 ++++++ 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/node/db/SecurityManager.ts b/src/node/db/SecurityManager.ts index 219d3f2be9a..6fb4793a5f1 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 fires once at startup, not + // per request (see Settings.ts). 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 97413004100..e8e6a798306 100644 --- a/src/node/utils/Settings.ts +++ b/src/node/utils/Settings.ts @@ -1193,6 +1193,12 @@ export const reloadSettings = () => { logger.warn("logLayoutType: " + settings.logLayoutType); initLogging(settings.logconfig); + if (settings.loadTest) { + logger.warn( + 'settings.loadTest is true: socket.io authentication and authorization checks ' + + 'will be bypassed for every connection. 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".'); From afb3a4a928337c8a24a55eb1dbd1faba60ff9a8c Mon Sep 17 00:00:00 2001 From: John McLear Date: Sat, 16 May 2026 12:14:26 +0100 Subject: [PATCH 2/2] 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) --- src/node/db/SecurityManager.ts | 10 +++++----- src/node/utils/Settings.ts | 5 +++-- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/node/db/SecurityManager.ts b/src/node/db/SecurityManager.ts index 6fb4793a5f1..33996c15557 100644 --- a/src/node/db/SecurityManager.ts +++ b/src/node/db/SecurityManager.ts @@ -76,11 +76,11 @@ exports.checkAccess = async (padID:string, sessionCookie:string, token:string, u // Authentication and authorization checks. // settings.loadTest just short-circuits authn/authz; the user-facing - // warning about this configuration choice fires once at startup, not - // per request (see Settings.ts). 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. + // 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'); diff --git a/src/node/utils/Settings.ts b/src/node/utils/Settings.ts index e8e6a798306..c603c13206e 100644 --- a/src/node/utils/Settings.ts +++ b/src/node/utils/Settings.ts @@ -1195,8 +1195,9 @@ export const reloadSettings = () => { if (settings.loadTest) { logger.warn( - 'settings.loadTest is true: socket.io authentication and authorization checks ' + - 'will be bypassed for every connection. Do NOT enable this in production.'); + '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) {