Skip to content

Conversation

@okxaas
Copy link
Contributor

@okxaas okxaas commented Oct 25, 2025

Problem Description

The cached discovery functionality was not working correctly because the DiscoveryLoader was not calling $registry->setDiscoveryState($discoveryState) when using cached results. This caused a critical issue where:

  1. Cache Hit Scenario: When CachedDiscoverer returned cached DiscoveryState from the cache, the DiscoveryLoader would receive the cached state but never apply it to the registry via setDiscoveryState().

  2. Registry State Mismatch: The registry would remain empty or contain stale data, even though the discovery process had successfully retrieved cached results.

  3. Silent Failure: This bug was particularly insidious because it would fail silently - the cache would appear to work (no errors thrown), but the discovered MCP elements (tools, resources, prompts, resource templates) would not be available in the registry.

Root Cause Analysis

The issue occurred during the refactoring of the discovery process in PR #111. The original Discoverer::discover() method was responsible for both:

  • Performing the discovery process
  • Setting the discovery state on the registry via $this->registry->setDiscoveryState($discoveryState)

However, when the discovery logic was moved to DiscoveryLoader, the responsibility for setting the discovery state was not properly transferred. The DiscoveryLoader was calling the discoverer but not applying the returned DiscoveryState to the registry.

Technical Details

Before Fix:

// DiscoveryLoader.php - BROKEN
$cachedDiscoverer->discover($this->basePath, $this->scanDirs, $this->excludeDirs);
// Missing: $registry->setDiscoveryState($discoveryState);

After Fix:

// DiscoveryLoader.php - FIXED
$discoveryState = $cachedDiscoverer->discover($this->basePath, $this->scanDirs, $this->excludeDirs);
$registry->setDiscoveryState($discoveryState);

Impact

  • Performance: Cached discovery was not providing the expected performance benefits
  • Functionality: MCP servers using cached discovery would not expose their discovered capabilities
  • User Experience: Tools, resources, and prompts would not be available to MCP clients

Solution

This fix ensures that:

  1. The DiscoveryLoader properly captures the DiscoveryState returned by the discoverer (whether cached or fresh)
  2. The DiscoveryState is applied to the registry via setDiscoveryState()
  3. Both cached and non-cached discovery paths work identically
  4. The registry contains the correct discovered elements regardless of cache hit/miss

Testing

The fix maintains backward compatibility and ensures that:

  • Fresh discovery works as before
  • Cached discovery now properly populates the registry
  • All existing tests continue to pass
  • The stdio-cached-discovery example now functions correctly

Files Changed

  • src/Capability/Registry/Loader/DiscoveryLoader.php: Added missing setDiscoveryState() call
  • src/Capability/Discovery/Discoverer.php: Removed redundant setDiscoveryState() call (now handled by loader)

Summary

This fix resolves the core issue where cached discovery results were not being applied to the registry, ensuring that MCP servers can properly utilize cached discovery for improved performance while maintaining full functionality.


Type: Bug Fix
Breaking Change: No
Related Issue: Fixes cached discovery functionality introduced in PR #111

@okxaas
Copy link
Contributor Author

okxaas commented Oct 25, 2025

Due to the release of #111 , I updated the code related to this issue, the original PR #113

Copy link
Member

@chr-hertel chr-hertel left a comment

Choose a reason for hiding this comment

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

This still misses the code path in src/Capability/Discovery/Discoverer.php:99

please change that as well and we can merge it

@okxaas okxaas force-pushed the refactor-discovery-cache branch from 7a9a89c to a8007f6 Compare October 25, 2025 15:44
@okxaas
Copy link
Contributor Author

okxaas commented Oct 25, 2025

Hey @chr-hertel ,I have completed the code changes and submitted them to the specified branch

@chr-hertel chr-hertel force-pushed the refactor-discovery-cache branch 4 times, most recently from 26391ca to a54feac Compare October 26, 2025 22:06
Copy link
Member

@chr-hertel chr-hertel left a comment

Choose a reason for hiding this comment

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

Thank you @okxaas - made another rebase and adoptions after other PRs

@chr-hertel chr-hertel force-pushed the refactor-discovery-cache branch from a54feac to 3cbcb28 Compare October 26, 2025 22:22
…te(), it is found that the discoveryState cannot be used
@chr-hertel chr-hertel force-pushed the refactor-discovery-cache branch from 3cbcb28 to 3082134 Compare October 26, 2025 22:24
@chr-hertel chr-hertel merged commit 42b5261 into modelcontextprotocol:main Oct 26, 2025
10 checks passed
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