Refactor: Resource Management with Ordered Pagination, Duplicate Control #408
Conversation
WalkthroughThe resource management logic in the server has been refactored into a new Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/resource_manager.go (1)
261-324: Consider performance optimization for template matching.The current implementation iterates through all templates to find a match (lines 286-300). For a large number of templates, this could be inefficient.
Consider implementing a more efficient URI template matching strategy, such as:
- Pre-computing template patterns during registration
- Using a trie-based structure for pattern matching
- Ordering templates by specificity
However, this optimization should only be considered if you expect a large number of resource templates in typical usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/resource_manager.go(1 hunks)server/server.go(9 hunks)util/logger.go(2 hunks)
🔇 Additional comments (12)
util/logger.go (1)
11-11: LGTM: Clean addition of warning-level logging.The
Warnfmethod follows the existing pattern and is correctly implemented in thestdLogger. This enhancement supports the resource manager's duplicate warning functionality.Also applies to: 36-38
server/resource_manager.go (5)
54-64: Well-designed data structure for O(1) operations with insertion order.The combination of linked list and map is an excellent choice for maintaining insertion order while providing O(1) lookup and deletion. The thread safety approach using RWMutex is appropriate.
84-108: Verify duplicate handling behavior aligns with expectations.The duplicate handling logic looks correct, but there's a potential race condition concern. When
warnOnDuplicateResourcesis true and a duplicate is found, the function continues to the next iteration without settinglistChanged = true. However, when it's false, the old element is removed and replaced, correctly settinglistChanged = true.The behavior seems intentional - when warnings are enabled, duplicates are truly skipped (no list change), but when warnings are disabled, duplicates are replaced (list changes). Could you confirm this is the intended behavior?
266-282: Verify resource handler mutex is released in all paths.The RWMutex is correctly released before calling the handler to prevent holding the lock during potentially long-running operations. This is good practice for avoiding deadlocks.
215-259: Apply the same pagination fix to resource templates.The same off-by-one issue exists here as in the
listResourcesmethod.- if paginationLimit != nil && len(templatesToReturn) >= *paginationLimit && ptr != nil { + if paginationLimit != nil && len(templatesToReturn) == *paginationLimit && ptr != nil {Likely an incorrect or invalid review comment.
160-204: Potential off-by-one error in pagination cursor generation.The cursor generation logic has a subtle issue. The condition on line 199 checks
len(resourcesToReturn) >= *paginationLimit, but this could generate a cursor even when there are no more elements, if the last page happens to be exactly the pagination limit size.- if paginationLimit != nil && len(resourcesToReturn) >= *paginationLimit && ptr != nil { + if paginationLimit != nil && len(resourcesToReturn) == *paginationLimit && ptr != nil {The same issue likely exists in
listResourceTemplateson line 254.Likely an incorrect or invalid review comment.
server/server.go (6)
136-136: Clean integration of resourceManager.The resourceManager is properly embedded in the MCPServer struct and initialized in the constructor. Good separation of concerns.
Also applies to: 288-288
182-190: Server option implementation looks correct.The
WithWarnOnDuplicateResourcesoption properly configures the resourceManager's duplicate handling behavior. The documentation clearly explains the two modes.
308-308: Verify delegation calls return expected types.The delegation to
resourceManager.addResources()andresourceManager.addResourceTemplate()looks correct. These methods return boolean values indicating if the list changed, which is properly used for notifications.Also applies to: 343-343
654-657: Pagination delegation simplifies the implementation.The delegation to
resourceManager.listResources()andresourceManager.listResourceTemplates()removes the need for the complex sorting and pagination logic that was likely present before. The cursor and pagination limit are passed through correctly.Also applies to: 680-683
706-708: Direct delegation to resource manager.The
handleReadResourcemethod now cleanly delegates to the resourceManager, which handles both direct resources and template matching internally.
613-647: Generic pagination function may be unused after refactoring.The
listByPaginationgeneric function appears to still be present but may no longer be used for resources and resource templates since they now use the resourceManager's specialized pagination methods.Please verify if this function is still needed or if it can be removed/marked for future cleanup. It's currently used for prompts and tools but not for resources.
#!/bin/bash # Search for usage of listByPagination function rg -A 3 "listByPagination" --type go
Description
Fixes #397
Refactors the internal resource management logic to preserve resource insertion order while maintaining O(1) lookup and deletion. This addresses the need for stable ordering in
ListResourcesand aligns with use cases such as time-ordered note management.Key Changes
resourceManager: Combines a linked list and a map to track insertion order and enable fast access/removal.warnOnDuplicateResourcesto control whether duplicate resources are ignored (with warning) or replaced.Resource.URIinstead ofResource.Namefor cursor positioning.listByPagination: it points to the last element of the current page, with the next page starting from the next element.ptr != nilcheck inif paginationLimit != nil && len(templatesToReturn) >= *paginationLimit && ptr != nil).Namebefore each page, improving performance and reducing complexity.Why We Avoided Generics
Although
listResourcesandlistResourceTemplatesshare similar logic, we intentionally avoided abstracting them into a single generic function due to practical performance considerations:URIextractor function (e.g.,func(entry T) string) to support cursor encoding.[]resourceEntryor[]resourceTemplateEntry, meaning that the caller must re-allocate and extract the final[]mcp.Resourceor[]mcp.ResourceTemplateafterward.This trade-off prioritizes runtime efficiency over code deduplication, which is more appropriate given the core role and high-call frequency of these functions in the MCPServer.
Type of Change
Checklist
Summary by CodeRabbit
New Features
Refactor
Chores