Skip to content

fix: share one server scope across tools and resources - #244

Draft
Valiunia wants to merge 6 commits into
mainfrom
fix/shared-server-scope
Draft

fix: share one server scope across tools and resources#244
Valiunia wants to merge 6 commits into
mainfrom
fix/shared-server-scope

Conversation

@Valiunia

@Valiunia Valiunia commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #242, which fixed last-install-wins server binding in BaseTool and BaseResource. Targets fix/per-invocation-server-binding; retarget to main once #242 merges.

What changed

BaseTool and BaseResource each got their own per-instance AsyncLocalStorage<McpServer> in #242. This replaces both with a single module-level store in src/utils/serverScope.ts:

export function runWithServer<T>(server: McpServer, fn: () => T): T;
export function getActiveServer(): McpServer | undefined;

protected get activeServer() on both base classes becomes getActiveServer() ?? this.server. protected server stays as the fallback for direct run() / read() calls made outside any registered callback. No signature change to run() or read().

Also in this PR: the ui:// resource registration moves out of src/index.ts into registerUiResources() so that path is testable, and the previously untested ResourceTemplate branch of BaseResource.installTo() gets coverage.

The problem it solves

Two code paths reach a tool or resource without going through the callback that installTo() registers, so neither could resolve the server whose request it was servicing — a per-instance store cannot fix either, because the component whose store would be consulted is not the one that was entered.

  1. resource_reader_tool (the fallback for clients without the native MCP resource API) calls getResourceByUri() and then resource.read(...) directly. That instance comes from the module-level singleton array ALL_RESOURCES, and the call bypasses the resource's own registered handler, so the read resolved the last-installed server. Latent today — no resource calls log() from read() — but live in the repo.

  2. MCP Apps UI resources (ui://…) are registered through registerAppResource, never installTo(). They resolved no server at all, making MapAppUIResource's inherited log() a silent no-op both before and after refactor: bind tool and resource invocations to the server that received them #242.

Because the store is shared and async context propagates into nested calls, a read() invoked from inside a tool callback now inherits the tool's server automatically. That fixes (1) with no plumbing through ResourceReaderTool and makes (2) a one-line wrap. It also removes the ~12 lines of binding logic #242's description flagged as knowingly duplicated.

How to verify

npm test          # 932 pass, 0 fail
npx tsc --noEmit
npm run build

Four tests were written against the unfixed code and observed failing with the wrong server id before any implementation existed. Verbatim output from that run:

PASS (12) FAIL (4)

1. registerUiResources binds the read to the server the resource was registered on
   AssertionError: expected { before: null, after: null } to deeply equal { before: 'a', after: 'a' }
2. BaseTool server binding binds a nested run() to the server handling the outer call
   AssertionError: expected { before: 'b', after: 'b' } to deeply equal { before: 'a', after: 'a' }
3. ResourceReaderTool server binding of the nested resource read binds the read to the server that received the tool call
   AssertionError: expected { before: 'b', after: 'b' } to deeply equal { before: 'a', after: 'a' }
4. ResourceReaderTool server binding of the nested resource read binds the read for a templated resource URI too
   AssertionError: expected { before: 'b', after: 'b' } to deeply equal { before: 'a', after: 'a' }

They pass after the change with no test modified. Each asserts the observed server both before and after an await, so a binding that only survives to the first suspension point still fails.

Coverage-only, not evidence of the above: the three new templated URI registration tests in test/resources/BaseResource.test.ts and test/utils/serverScope.test.ts pass against the pre-change code. The first group covers the ResourceTemplate branch, which had none, and acts as a regression net for this refactor rewriting both registration branches. The second is a unit spec for the new module.

What to watch for

  • A component installed nowhere now inherits the caller's server. With per-instance stores, a tool or resource never passed to installTo() resolved null. With a shared store it resolves the active request's server when invoked from inside another component's handler. Reads and logs that used to go nowhere will now reach a client. Benign, but visible.
  • serverScope is deliberately not barrel-exported from src/utils/index.ts (the public @mapbox/mcp-server/utils subpath). Exporting runWithServer would publish a way to redirect any Mapbox tool's elicitations and sampling at an arbitrary server — runWithServer(myServer, () => directionsTool.run(...)). Deep imports aren't in the exports map, so keeping it out makes it genuinely private. The file header says so; test/exports.test.ts is untouched.
  • Two copies of the module would mean two stores and a silent fall back to this.server. Avoided by never mocking serverScope.ts in tests — the behavioral tests assert through the base classes — and by keeping both importers in-package.
  • getActiveServer() means the server of the current request, not the server a component was installed into. Every current reader talks back to a client (log()sendLoggingMessage, elicitInput in DirectionsTool/SearchAndGeocodeTool, createMessage in GroundLocationTool), so the originating connection is always right. A future feature wanting the installation server — notifications/resources/updated to subscribers, say — must reach for this.server deliberately. Documented on the function.
  • Stale-context audit, no leaks found. httpPipeline's retry setTimeout is created and awaited inside the request context. temporaryResourceManager's setInterval is created by a module-load singleton, so it captures an empty context — a comment now notes the hazard if anyone later constructs one inside a request handler. process.on(...) in index.ts is module scope. No worker threads, no stored-for-later callbacks on these paths.

🤖 Generated with Claude Code

Valiunia and others added 5 commits July 31, 2026 10:41
BaseTool stored its target McpServer in a single mutable instance field
assigned by installTo(), and the callback registered with the SDK read
that field at call time instead of capturing the server it was
registered on. The tool instances exported from @mapbox/mcp-server/tools
are module-level singletons, so installing one instance into a second
McpServer overwrote the field and redirected the first server's
callbacks to the second — affecting logging, sampling in
ground_location_tool, and elicitations in search_and_geocode_tool and
directions_tool.

Each invocation now resolves its server through an AsyncLocalStorage
scope entered by the registered callback, so concurrent calls arriving
through different servers each observe their own. this.server remains as
a fallback for run() called outside a registered callback, which keeps
existing direct-invocation callers and their tests working unchanged.

Single-server applications, including the server this package ships, are
unaffected: both lookups resolve to the same object.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
BaseResource had the same shared-server-field pattern as BaseTool: the
handlers registered by installTo() read a mutable this.server at call
time rather than capturing the server they were registered on, and the
instances in ALL_RESOURCES are module-level singletons. Installing one
into a second McpServer redirected the first server's handlers.

Only log() read the field, so the effect was misdirected log messages
rather than misdirected client interaction, but the hazard is the same
and resources are part of the published @mapbox/mcp-server/resources
export.

Applies the same per-invocation AsyncLocalStorage binding, with
this.server retained as the fallback for direct read() calls. Both
registration paths (plain URI and ResourceTemplate) are covered.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two code paths reach a tool or resource without going through the callback
that installTo() registers, so neither could resolve the MCP server whose
request it was servicing:

- resource_reader_tool looks a resource up in the module-level registry and
  calls read() on it directly, so the read bypassed that resource's own
  registered handler and resolved the last-installed server instead.
- MCP Apps UI resources (ui://...) are registered through
  registerAppResource, never installTo(), so they resolved no server at all
  and their inherited log() was a silent no-op.

Replaces the two per-instance AsyncLocalStorage fields in BaseTool and
BaseResource with one module-level store in src/utils/serverScope.ts. Async
context propagates into nested calls, so a read invoked from inside a tool
callback inherits the tool's server with no plumbing and no signature
changes to run() or read().

serverScope is intentionally not re-exported from src/utils/index.ts:
publishing runWithServer would let any caller redirect a Mapbox tool's
elicitations and sampling at a server of their choosing.

Also extracts the ui:// registration out of index.ts into
registerUiResources() so that path is testable, and adds coverage for the
previously untested ResourceTemplate branch of BaseResource.installTo().

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@mattpodwysocki
mattpodwysocki changed the base branch from fix/per-invocation-server-binding to main August 3, 2026 16:32
# Conflicts:
#	CHANGELOG.md
#	src/resources/BaseResource.ts
#	src/tools/BaseTool.ts
#	test/resources/BaseResource.test.ts
#	test/tools/BaseTool.test.ts
@mattpodwysocki

Copy link
Copy Markdown
Contributor

This is exactly the right follow-up. A per-instance store literally can't fix either of the two cases you called out, resource_reader_tool's direct read() and the ui:// resources that never went through installTo() at all, so moving to one shared module-level store is the correct fix rather than a workaround.

Went through this locally: tsc clean, eslint clean, full suite 931/932 (the one failure is the pre-existing unrelated urlSafety.test.ts issue on main). Also checked the two specific claims in the description instead of taking them on faith:

  • The barrel export claim holds up. serverScope isn't in src/utils/index.ts, and package.json's exports map is closed to just ., ./tools, ./resources, ./prompts, ./utils, so there's no deep import path around it either.
  • The httpPipeline claim holds up too, its retry setTimeout is inside an awaited Promise in the same context, not a leak.

I also reverted the runWithServer wrap in registerUiResources.ts back to a bare call and reran registerUiResources.test.ts to make sure that test isn't tautological. It fails exactly as described, before/after both come back null instead of 'a'. Restored after.

This also happens to close out both things I flagged as non-blocking on #242 (the resource_reader_tool gap and the missing ResourceTemplate coverage), plus a third gap in ui:// resources that #242 didn't even know about. Good to merge whenever you're ready to take it out of draft.

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