-
Notifications
You must be signed in to change notification settings - Fork 531
fix: avoid infinite loop when cursor is empty string #770
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
cursor = toolResults.NextCursor; | ||
} | ||
while (cursor is not null); | ||
while (!string.IsNullOrEmpty(cursor)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to add an integration test that recreates the conditions of the infinite loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not very familiar with xUnit or the project's test structure, so I'm not able to write a new integration test at this time.
If you'd like to reproduce the issue, you can do so by performing a paginated operation for tools, and on the last page, set the NextCursor property to an empty string ("") instead of null. This should help simulate the scenario in question.
What happens (or what should happen) when a subsequent request is made with cusror set to an empty string? Shouldn't that fail? If it did, then there would be no infinite loop. |
When should this pull request be merged? |
If we do make this change, we should probably copy the pattern for prompts, resources and templated resources, and add tests.
Looking at the screenshot, Alibaba Bailian's MCP service continues to return the same tool listing with an empty cursor whether or not it should. I think that stopping iteration given an empty string as a nextCursor makes more sense than using it. We could also treat an empty nextCursor as an error, but I don't think that's helpful. Looking at the TypeScript and Python SDK, neither automatically enumerates cursors the way we do. I cannot even find a sample in the TypeScript SDK repo that iterates over paginated results. Here's the list tools sample code: The Python SDK repo has a sample of iterating over paginated resources that checks In the meantime, people can use the lower level |
I've also encountered this problem. When connecting to Alibaba's MCP service, the cursor returns an empty string instead of null. Please approve this PR as soon as possible. |
IMHO this is a bug in Alibaba's MCP Server. According to the spec:
Opaque means the client should not be inspecting its value for meaning. The client should only use the presence or absence of the token to determine when it can issue a subsequent request.
Another possible workaround is a custom deserializer for ListToolsResult that omits the cursor if it is empty. This might be a cleaner alternative to dropping down to the lower level API. |
Replace
while (cursor is not null)
withwhile (!string.IsNullOrEmpty(cursor))
to prevent potential infinite loops if the cursor is an empty string.Motivation and Context
When integrating with Alibaba Bailian's MCP service, I encountered an issue where the pagination API returns an empty string ("") instead of null for NextCursor when there is no more data. The previous loop condition failed to terminate in this case, resulting in infinite requests. This PR updates the loop condition to improve SDK compatibility with this scenario.
How Has This Been Tested?
Yes, I verified the change by testing the compiled code and confirmed that the modified logic works as expected and resolves the infinite request issue.
Breaking Changes
No.
Types of changes
Checklist
Additional context
This change ensures more robust pagination handling and prevents potential infinite loops caused by empty cursor values.