fix(mcp-oauth): graceful fallback when OAuth callback port is already bound - #55741
fix(mcp-oauth): graceful fallback when OAuth callback port is already bound#55741oppih wants to merge 1 commit into
Conversation
Same MCP-OAuth callback-port cluster as #34260 (module-level |
dfc2bd8 to
278d41f
Compare
… bound Fixes a crash in `hermes mcp login` where `_wait_for_callback()` tries to bind a second HTTPServer to the same port already occupied by the MCP SDK's callback server, causing OSError: Address already in use that aborts the entire OAuth flow. Instead of raising OAuthNonInteractiveError, the fix gracefully falls through to polling the SDK's existing server. Also bumps connect_timeout from 30s to 600s for interactive OAuth flows. Fixes: NousResearch#55741
278d41f to
0d645a2
Compare
Relationship to #58154 (merged)These two PRs are complementary, not duplicate — they address the same port-collision symptom at different layers:
#58154 handles the background/non-interactive case — when a cached-but-unusable token would cause the SDK to enter the auth-code flow during gateway startup. It fails fast before printing a URL or binding a listener. This PR handles the interactive case — when a user runs Both guards can coexist: #58154 prevents non-interactive hangs; this PR prevents interactive crashes when the SDK pre-binds. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the interactive callback-port path. The premise is still present on current main, but the proposed fallback needs a shared callback-state mechanism before it can work.
Problems
tools/mcp_oauth.py:664suppresses the bind failure and polls the new localresultfrom line 651._make_callback_handler()writes only the result dictionary closed over by its own handler (tools/mcp_oauth.py:497-516); a callback handled by another listener cannot update this dict. The fallback will therefore wait until the existing timeout path (tools/mcp_oauth.py:701-704).hermes_cli/mcp_config.py:515changesmcp add, notmcp login. Current main already gives the actual login flow a 315-second floor athermes_cli/mcp_config.py:794-801(f26ae4f).- No bound-port regression test accompanies the new behavior.
Suggested changes
- Establish a single callback owner or an explicit result hand-off that this coroutine can observe; do not poll an unconnected result dict.
- Remove the unrelated add-time 600-second override and add a collision regression test.
Automated hermes-sweeper review.
| server_thread.start() | ||
| # Port already in use — the server from the MCP SDK is running. | ||
| # Fall back to polling: the SDK's server will handle the callback. | ||
| pass |
There was a problem hiding this comment.
result was freshly created at line 651 and is only written by the handler returned from _make_callback_handler(). Since this fallback does not start that handler, a callback received by a different listener cannot update result; polling will run to the 300-second timeout. Please use a shared callback-result hand-off or retain a prompt bind failure.
|
|
||
| try: | ||
| tools = _probe_single_server(name, server_config) | ||
| tools = _probe_single_server(name, server_config, connect_timeout=600) |
There was a problem hiding this comment.
This is cmd_mcp_add, not the hermes mcp login path described by the PR. Current main already grants interactive login at least 315 seconds in _reauth_oauth_server (hermes_cli/mcp_config.py:794-801); extending every add-time probe to 600 seconds changes unrelated discovery failures.
|
Closing after the OAuth callback rework landed (#65622, #65664). The premise here doesn't hold against how the flow actually works: |
What does this PR do?
Fixes a crash in
hermes mcp loginwhere_wait_for_callback()tries to bind a secondHTTPServerto the same port already occupied by the MCP SDK'sbuild_oauth_auth()callback server, causingOSError: Address already in usethat aborts the entire OAuth flow.Instead of raising
OAuthNonInteractiveError, the fix gracefully falls through to polling the SDK's existing server.Related Issue
N/A (no existing issue found for this specific double-bind bug)
Type of Change
Changes Made
tools/mcp_oauth.py: OnOSError(port already in use by SDK server),passinstead of raisingOAuthNonInteractiveError, allowing the polling loop to detect the callback result from the SDK's server. Guardserver.server_close()withif server is not Noneto handle the unbound variable in the fallback path.hermes_cli/mcp_config.py: Bump_probe_single_server()connect_timeout from default 30s to 600s, giving users enough time to complete interactive browser-based OAuth authorization.How to Test
hermes mcp login <server-name>OSError: Address already in use→OAuthNonInteractiveErrorChecklist
Code
Documentation & Housekeeping