fix(http): detach per-session loggers from the shared file transport on stop() - #412
Merged
Conversation
…on stop() Every Streamable HTTP session's DebugMcpServer piped its logger into the process-lifetime shared file transport and nothing ever unpiped — one listener set + retained logger per session, forever (issue #404). createLogger now records the attached shared transport in a WeakMap; detachSharedFileTransport(logger) removes it without closing it — shadowing transport.close during the remove because winston-transport's close-on-unpipe fires when the FIRST attacher unpipes and would close the shared file stream for everyone. The DI container exposes the detach as Dependencies.disposeLogger and DebugMcpServer.stop() calls it last. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #404
In Streamable HTTP mode every MCP session builds a full
DebugMcpServer, whose DI container creates a winston logger piped into the process-lifetime shared file transport (fileTransportCache, one transport per path — issue #121's rotation-correctness invariant). Nothing ever unpiped:stop()didn't touch the logger, andlogger.close()is forbidden because it would close the shared transport for every logger. So the shared transport accumulated one pipe edge + listener set + retained logger per HTTP session, forever — exactly the churn theMCP_HTTP_STALE_SESSION_MSreaper (#337) exists for, and it couldn't reclaim this.Change
logger.ts: aWeakMap<logger, transport>records which shared transport eachcreateLoggercall attached; newdetachSharedFileTransport(logger)removes that transport from the logger (unpiping it) without closing it.once('unpipe', src => { if (src === this.parent) { this.parent = null; this.close(); } })whereparentis the first logger that piped the transport — a plainlogger.remove()from that logger closes the shared file stream for everyone still using it. The detach shadowstransport.closewith an ownundefinedproperty for the duration of the remove (the handler'sif (this.close)is then falsy) and restores it infinally.Dependenciesgains optionaldisposeLogger?: () => void;createProductionDependenciesreturns() => detachSharedFileTransport(logger)(the closure keeps the concrete winston type —ILoggerin shared never widens).DebugMcpServer.stop()callsthis.disposeLogger?.()last, after the final "Debug MCP Server stopped" line, so that line still reaches the file.logger.close()remains forbidden; the shared transport stays open and cached for the process lifetime, as documented.Tests (TDD, watched fail first)
logger-detach.test.tsruns against real winston (no module mock — the leak lives in winston's pipe mechanics):unpipe/error) don't grow, cache identity holds.closespy) and the surviving logger still logs.dependencies.test.ts:disposeLoggerwired todetachSharedFileTransportwith the container's logger (its logger.js mock factory gained the new export).server-lifecycle.test.ts:stop()invokes the disposer exactly once.npm testgreen,npm run lintclean.Not touched (noted in the issue): the deprecated SSE transport's session map has no stale reaper, and the module-level
defaultLoggeroverwrite retains only the newest logger — neither is a growth leak.🤖 Generated with Claude Code