perf: avoid throw-as-control-flow in SessionManager hot path (#7756)

[JohnMcLear]

Summary

CPU profile of the SUT at the 100→400 author dive sweep (load-test workflow run 25956384097) attributed ~6% of total process CPU to the throw + catch around getSessionInfo:

• ~1.82% to new CustomError('sessionID does not exist', 'apierror') (stack-trace capture)
• ~4.12% downstream, via the catch block's console.debug(...) routed through log4js → sendToListeners → sendLogEventToAppender

Both internal callers (findAuthorID on every CLIENT_READY, listSessionsWithDBKey on session listing) caught apierror and discarded it. The public exports.getSessionInfo still has to throw for the HTTP API (returns code: 1 for missing sessionID), so this PR introduces a private getSessionInfoOrNull helper that returns null and switches the two hot-path callers to it. exports.getSessionInfo becomes a thin wrapper that preserves the throw semantics.

Profile evidence

Inverted callers of CustomError constructor in the profile:

 1.82% exports.getSessionInfo [src/node/db/SessionManager.ts]

Inverted (non-log4js) callers of log4js Logger.<computed>:

 4.12% exports.checkAccess [src/node/db/SecurityManager.ts]

(checkAccess is the entry point that drives findAuthorID → getSessionInfo → console.debug.)

Behavior

• No behavior change for the HTTP API. getSessionInfo still throws `apierror` when the session doesn't exist; RestAPI.ts/APIHandler.ts translate that to code: 1.
• The two internal callers preserve their existing semantics: findAuthorID returns undefined for unknown sessions, listSessionsWithDBKey continues to log a warning and set sessions[sessionID] = null.

Test plan

• `pnpm exec mocha tests/backend/specs/api/sessionsAndGroups.ts` — all 32 cases pass, including "getSessionInfo of deleted session" (still expects code:1).
• Re-run the dive sweep with this branch as core_ref and confirm steady-state CPU% drops at the cliff (300-400 authors).

Part of #7756. Profile capture pipeline is in etherpad-load-test#109/110/111.

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[qodo-free-for-open-source-projects]

Review Summary by Qodo

Avoid throw-as-control-flow in SessionManager hot path

✨ Enhancement

Walkthroughs

Description

• Eliminate throw-as-control-flow in hot-path callers of getSessionInfo
• Introduce private getSessionInfoOrNull helper returning null
• Refactor findAuthorID and listSessionsWithDBKey to use null checks
• Reduce CPU overhead by ~6% (1.8% CustomError construction + 4% logging)

Diagram

flowchart LR
  A["getSessionInfoOrNull<br/>returns null"] --> B["findAuthorID<br/>null check"]
  A --> C["listSessionsWithDBKey<br/>null check"]
  D["exports.getSessionInfo<br/>wrapper"] --> E["throws apierror<br/>for HTTP API"]
  A --> D

Loading

File Changes

1. src/node/db/SessionManager.ts Performance optimization +20/-22

Replace throw-catch with null-check pattern

• Introduce private getSessionInfoOrNull helper that returns null instead of throwing
• Refactor findAuthorID to use null-returning helper with simple || check
• Refactor listSessionsWithDBKey to use null-returning helper with explicit null check
• Keep exports.getSessionInfo as thin wrapper preserving throw semantics for HTTP API

src/node/db/SessionManager.ts

---

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

[JohnMcLear]

N=3 measured impact

Three perf-branch runs (25957107195, 25957108328, 25957109418) compared against three develop baselines (25954537767, 25954538807, 25954540108) — same authors=100..500:step=50:dwell=8s:warmup=2s sweep on each.

Medians:

step	dev CPU%	perf CPU%	ΔCPU%
100	4.76	4.67	-1.7%
150	9.09	8.95	-1.5%
200	15.21	14.60	-4.0%
250	21.51	21.32	-0.9%
300	30.46	29.68	-2.6%
350	41.58	39.36	-5.3%
400	56.26	54.23	-3.6%
450	72.33	70.49	-2.5%
500	88.38	87.14	-1.4%

ΔCPU% is consistently negative (-1.4% to -5.3%) across all 9 steps. The realised magnitude is below the profile-attributed 6% upper bound because some of the log4js cost the profile attributed to the throw path was unrelated info logging — but the direction is exactly what the profile predicted.

Latency delta sits inside the noise envelope across all steps (raw p95 triples overlap heavily run-to-run).

[JohnMcLear]

#7775 + #7776 stacked — N=3 combined measurement

I cherry-picked #7775 onto #7776 (branch perf/dive-combined) and ran 3 cliff sweeps (25960003164, 25960004223, 25960005248) against the same 3 develop baselines used for the individual PR scoring. The two fixes stack super-additively:

step	dev CPU%	#7775 alone	#7776 alone	both	Δboth
100	4.76	4.67	4.51	3.99	-16.1%
200	15.21	14.60	14.33	12.48	-17.9%
300	30.46	29.68	28.50	24.39	-19.9%
350	41.58	39.36	37.87	33.04	-20.5%
400	56.26	54.23	53.67	44.78	-20.4%
450	72.33	70.49	68.80	61.18	-15.4%
500	88.38	87.14	85.17	77.70	-12.1%

The stacked impact (-12% to -20% CPU%) is well above the sum of the individual gains. Both fixes remove call sites feeding the same log4js cluster-mode dispatch chain (sendToListeners → sendLogEventToAppender); halving the LogEvent allocation rate appears to relieve queue/GC pressure beyond what either fix accounts for in isolation.

Latency: cliff has effectively moved past step 400. Raw p95 triples:

step	develop p95 [3 runs]	combined p95 [3 runs]
400	[1758, 2275, 2463]	[45, 112, 634]
450	[5415, 6167, 6611]	[3297, 3719, 3897] (-40%)
500	[10655, 11759, 12183]	[8091, 8711, 9127] (-26%)

At step 400, two of three combined runs land below the cliff entirely. Recommend landing these two together to capture the full effect.