perf: don't log settings.loadTest warning per-message (#7756) (#7776)

* 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) <noreply@anthropic.com>

* 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) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
John McLear 2026-07-27 20:03:26 +01:00 committed by GitHub
parent 63f95d6029
commit e1c2fcd1f1
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 14 additions and 4 deletions

View file

@ -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;

View file

@ -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".');