From 620c8b20cdc95b24d5d12fa2403788a6bd5ffb89 Mon Sep 17 00:00:00 2001 From: John McLear Date: Sat, 16 May 2026 11:22:00 +0100 Subject: [PATCH] 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 219d3f2be..6fb4793a5 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 974130041..e8e6a7983 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".');