Skip to content

fix(everything): scope the session resource registry to the owning server - #4809

Open
ycwlzx wants to merge 2 commits into
modelcontextprotocol:mainfrom
ycwlzx:fix/session-resource-scope
Open

ycwlzx wants to merge 2 commits into
modelcontextprotocol:mainfrom
ycwlzx:fix/session-resource-scope

Conversation

@ycwlzx

@ycwlzx ycwlzx commented Sep 15, 2026

Copy link
Copy Markdown

Description

registerSessionResource keeps its "already registered" bookkeeping in a module-level Map keyed by URI only. Every session gets its own McpServer from createServer(), so that map is shared across sessions: when session B registers a URI session A already registered, the existing entry's remove() is called and A's resource is unregistered from A's server. A's later resources/read then fails with -32602 and the resource also disappears from A's resources/list.

The registry is now scoped to the McpServer that owns the resource, via a WeakMap<McpServer, Map<string, RegisteredResource>>, so removal only ever touches resources registered on that same server instance. A WeakMap also lets a closed session's registry be collected.

Server Details

  • Server: everything
  • Changes to: resources

Motivation and Context

Session resources are documented as per-session, and the server is built per session:

  • src/everything/docs/features.md:44 — "Session Scoped: demo://resource/session/<name> (per-session resources registered dynamically; available only for the lifetime of the session)"
  • src/everything/docs/how-it-works.md:36 — "The content is served from memory for the life of the session only."
  • src/everything/server/index.ts:35createServer() returns a fresh server per session
  • src/everything/resources/subscriptions.ts — the sibling module already keys its state by sessionId

The module-level map came from 3e1be88 ("fix(everything): allow re-registration of session resources", 2026-01-26), whose purpose was re-registration within a session so that a retried gzip-file-as-resource call does not fail with "Resource already registered". Cross-session eviction is an unintended side effect of that fix, not a product decision — no doc scopes session resource names globally.

How Has This Been Tested?

Two real Clients against two real McpServers over InMemoryTransport, calling the real gzip-file-as-resource tool. No network, no API keys. Session C registers a different name as a control.

Before:

1. A registers shared.gz, A reads it      -> OK (1 content)
2. C registers other.gz, A reads again    -> still OK (control)
3. B registers shared.gz, A reads         -> FAILED: MCP error -32602: Resource demo://resource/session/shared.gz not found
4. B reads its own resource               -> OK (1 content)
5. A's resources/list still contains it?  -> false

After:

1. A registers shared.gz, A reads it      -> OK (1 content)
2. C registers other.gz, A reads again    -> still OK (control)
3. B registers shared.gz, A reads         -> still OK
4. B reads its own resource               -> OK (1 content)
5. A's resources/list still contains it?  -> true

Unit tests added to __tests__/resources.test.ts, run with the repo's vitest setup:

npx vitest run __tests__/resources.test.ts
unpatched: 1 failed | 25 passed   (the new cross-session test is the failure)
patched:   26 passed

One test covers the cross-session eviction; the other asserts that re-registration within one session still replaces that session's own previous registration, so 3e1be88's behaviour is pinned rather than regressed. That second test passes before and after.

Breaking Changes

No. The change only narrows which previously-registered resources a re-registration may remove; nothing observable changes for a single session.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Protocol Documentation
  • My changes follows MCP security best practices
  • I have updated the server's README accordingly
  • I have tested this with an LLM client
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have documented all environment variables and configuration options

Four boxes above are deliberately left unticked rather than ticked by default:

  • MCP Protocol Documentation / security best practices — this change does not alter protocol-visible behaviour or any auth path; it only narrows an internal registry, so I did not treat the protocol docs as a prerequisite. Happy to revisit if you consider it in scope.
  • README — the README does not describe the session resource registry, so there was nothing to update.
  • Tested with an LLM client — I have not, and I will not claim otherwise. Verification used two real MCP clients over InMemoryTransport driving the real tool, plus the unit tests above. If an LLM-client pass is required before review, tell me the client/preferred way and I will run it.
  • Error handling / env vars — the change adds neither.

Additional context

The registry is a WeakMap keyed by server instance, so no explicit cleanup is needed when a session ends and its server is collected.

Scope of this PR is the collision between sessions only. A separate, pre-existing observation, not addressed here: the sync/streamable-HTTP path is unchanged, and nothing scopes session resource names within one server if the same URI is registered through different tools.

…rver

registerSessionResource kept its re-registration bookkeeping in a module-level
Map keyed by URI only. Every session gets its own McpServer from createServer(),
so that map is shared: when one session registered a URI another session had
already registered, existingResource.remove() unregistered the first session's
resource from its own server, and that session's resources/read then failed with
-32602 while the resource also disappeared from its resources/list.

Session resources are documented as per-session (docs/features.md, "available
only for the lifetime of the session"; docs/how-it-works.md, "served from memory
for the life of the session"), and the sibling subscriptions.ts already keys its
state by sessionId. A module-level map is the odd one out.

Scope the registry to the McpServer that owns the resource with a
WeakMap<McpServer, Map<string, RegisteredResource>>, so remove()/set() only ever
touch resources registered on that same server instance. A WeakMap also lets a
closed session's registry be collected.

Re-registration within one session still replaces that session's own previous
registration, which is what 3e1be88 added the map for.
Copilot AI balanced review requested due to automatic review settings September 15, 2026 11:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

The previous commit pushed working-tree bytes, which carried CRLF because of a
local core.autocrlf setting. GitHub therefore rendered both files as fully
rewritten instead of the actual change. Content is identical apart from line
endings.
@ycwlzx

ycwlzx commented Sep 15, 2026

Copy link
Copy Markdown
Author

Note: the first commit pushed working-tree bytes that carried CRLF line endings (a local core.autocrlf setting), so GitHub rendered both files as fully rewritten. 5494131 normalises them to LF; the code and tests are unchanged from the previous commit. The real diff is +81/-8 across the two files.

Copilot AI review requested due to automatic review settings September 15, 2026 11:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

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