Skip to content

Add idle-session TTL and cap to bound session memory growth - #19

Merged
ToxicOrca merged 2 commits into
mainfrom
fix/session-registry-leak
Aug 6, 2026
Merged

Add idle-session TTL and cap to bound session memory growth#19
ToxicOrca merged 2 commits into
mainfrom
fix/session-registry-leak

Conversation

@ToxicOrca

@ToxicOrca ToxicOrca commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • The HTTP transport's session bookkeeping (transports/sessionAuth, now SessionRegistry) was only ever cleaned up via transport.onclose. A session that gets abandoned instead of closed cleanly (client vanishes, network drop, no clean shutdown) never fires that callback, so its entry — and the full McpServer instance + tool closures it pins — is never released. Over the lifetime of a long-running deployment this is unbounded memory growth with no cap.
  • Adds an idle-session TTL sweep for SessionRegistry, mirroring the existing periodic-sweep pattern already used in oauth/kv-store.ts's InMemoryKvStore: a setInterval (unref'd) evicts sessions idle past the TTL, with a lazy expiry check on access as a backstop between sweep ticks.
  • Both eviction paths (sweepExpired() and the lazy purgeIfExpired()) now close the transport before dropping it from bookkeeping, via a shared evict() helper — matching the close-then-delete ordering shutdown() already used. evict() closes fire-and-forget (not awaited, unlike shutdown()), since sweepExpired() runs inside a setInterval callback with no process-exit coordination to synchronize with — intentional, not a regression.
  • Adds a hard cap on total concurrent sessions (BOOKSTACK_MAX_SESSIONS) as an independent backstop to the TTL logic — new session creation is rejected once at capacity.
  • Adds observability: /health now reports the current session count and process.memoryUsage().rss, and the server logs the configured idle TTL and session cap on startup.
  • New env vars, documented in .env.example: BOOKSTACK_SESSION_IDLE_TTL_MS (default 30 minutes) and BOOKSTACK_MAX_SESSIONS (default 1000).

Test plan

  • New unit tests in src/session-registry.test.ts:
    • Reproduces the leak: an abandoned session (its close path never invoked) is confirmed swept once the idle TTL elapses.
    • Confirms a session that keeps receiving activity survives repeated sweep ticks across several reconnect cycles (verifies the TTL margin doesn't evict live sessions).
    • Confirms the hard session cap is enforced once reached.
    • Confirms the sweep timer is created on construction and torn down via dispose().
    • Confirms both eviction paths (periodic sweep and lazy on-access purge) call close() on the transport before removing it from bookkeeping, via a close() spy on the fake transport.
  • Existing test suite (src/util/semaphore.test.ts) passes unchanged.
  • npm run type-check passes.
  • Not yet field-verified against real production traffic — this has only been validated with unit tests against the extracted SessionRegistry class, not exercised under live load or a real abandoned-connection scenario.

transports/sessionAuth are only reaped via transport.onclose, which
never fires for a session that's abandoned rather than closed
cleanly - each one pins a full McpServer instance, so long-lived
deployments can accumulate unbounded memory over time.

Adds an idle-session TTL sweep (mirrors the existing kv-store.ts
pattern), a hard session cap as an independent backstop, and session
count/RSS on /health so this is observable going forward.
@ttpears

ttpears commented Aug 5, 2026

Copy link
Copy Markdown
Owner

SessionRegistry evicts without closing, so the TTL bounds the bookkeeping and not the leak.

sweepExpired and purgeIfExpired both drop the three Record entries and stop. In the SDK, close() is the only thing that walks _streamMapping calling each cleanup() and fires onclose — so the SSE response stays open and the transport, McpServer and tool closures stay reachable from the HTTP layer.

shutdown in this same PR already gets it right — await sessions.get(sid)?.close() then delete. Same ordering needed in both eviction paths.

private sweepExpired(): void {
const now = Date.now();
for (const sid of Object.keys(this.transports)) {
if (now - (this.lastSeen[sid] ?? 0) >= this.idleTtlMs) this.delete(sid);
}
}

Watch for: after a sweep, /health drops the session and atCapacity() frees a slot while the resource is still live, so the new counter can hide this leak instead of surfacing it. The client is never told either — stream stays open, later POSTs get 400 No valid session ID.

fakeTransport is {} with no close, so the tests pass because the sweep never calls it. Needs a close spy.

Two smaller ones, not blocking:

  • atCapacity()/size() skip the purgeIfExpired check that has()/get() apply — dead-but-unswept sessions hold cap slots for a sweep interval.
  • atCapacity() runs before await server.connect() while register() lands in onsessioninitialized — a concurrent burst overshoots maxSessions.

Close-before-delete in both eviction paths plus a close spy on the test, then good to merge.

sweepExpired() and purgeIfExpired() dropped the transports/lastSeen
bookkeeping without ever calling transport.close() first, unlike
shutdown() (which already does close-then-delete correctly). That
leaked the transport (and the McpServer + tool closures it holds) on
every TTL-based eviction, silently working around the memory-growth
fix just added in d4faded.

Add a shared evict() helper that closes best-effort before calling
delete(), and use it from both eviction paths. Also give the fake
transport in the tests a close() spy so a missing close() call is
actually detectable, and assert it fires from both the periodic sweep
and the lazy on-access purge.

Flagged in review. Not addressed here, tracked as follow-up: atCapacity()/
size() don't run the purge check first, and there's a race between
atCapacity() and register().
@ttpears

ttpears commented Aug 6, 2026

Copy link
Copy Markdown
Owner

Both blockers are fixed. evict() is the single chokepoint now, and sweepExpired() and purgeIfExpired() both route through it — no path left that drops a Record entry without calling close() first.

The test is a real spy rather than a stub:

✔ an abandoned session (onclose never fires) is swept after the idle TTL
✔ an expired session accessed between sweeps is closed by the lazy purge on access
✔ a session touched within the idle TTL survives repeated sweep ticks
ℹ tests 9  ℹ pass 9  ℹ fail 0

close.mock.callCount() is asserted as 1 on the sweep path, 1 on the lazy-purge path and 0 for the touched session, so it would actually fail if evict() regressed to delete-only. That was the gap.

Two new ones from this push, neither blocking:

  • shutdown()await sessions.get(sid)?.close(). get() runs purgeIfExpired() first, so a session already idle-expired at shutdown gets evicted inside get() (close fired un-awaited behind .catch(() => {})) and returns undefined, and ?. skips the awaited close. It still closes, just not awaited — which is the one thing shutdown() exists to do.

    bookstack-mcp/src/index.ts

    Lines 1190 to 1196 in b0b99e8

    const shutdown = async () => {
    console.error("Shutting down HTTP server...");
    for (const sid of sessions.sessionIds()) {
    try { await sessions.get(sid)?.close(); } catch {}
    sessions.delete(sid);
    }
    sessions.dispose();
  • transport is StreamableHTTPServerTransport | undefined where handleRequest is called, and tsconfig.json sets "strict": false so the compiler stays quiet. has() and get() each re-evaluate expiry against a fresh Date.now(), so a session sitting on the TTL boundary can pass has() and then purge inside get()transport lands undefined and handleRequest throws. Narrow, but the old transports[sessionId] read couldn't do it. A falsy check dropping into the existing 400 No valid session ID closes it.

Still open from last round, still not blocking: atCapacity()/size() skip the purge, and the capacity check runs before await server.connect() while register() lands in onsessioninitialized.

Watch for: eviction closes the transport now, so an idle client is disconnected server-side at 30m instead of just forgotten — its next POST gets 400 No valid session ID and it has to re-initialize. Intended, and the ~5m stream recycle keeps active sessions alive, but it is new user-visible behavior worth watching after it ships.

Good to merge.

@ToxicOrca
ToxicOrca merged commit a785f90 into main Aug 6, 2026
3 checks passed
@ToxicOrca
ToxicOrca deleted the fix/session-registry-leak branch August 6, 2026 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants