Skip to content

fix(mcp-oauth): anchor 401 handler task to prevent GC mid-flight - #12228

Closed
haileymarshall wants to merge 1 commit into
NousResearch:mainfrom
haileymarshall:fix/mcp-oauth-track-401-handler-task
Closed

haileymarshall wants to merge 1 commit into
NousResearch:mainfrom
haileymarshall:fix/mcp-oauth-track-401-handler-task

Conversation

@haileymarshall

Copy link
Copy Markdown
Contributor

Summary

MCPOAuthManager.handle_401() spawned its dedup'd recovery coroutine via asyncio.create_task(_do_handle()) and dropped the returned task handle. Python's event loop only keeps weak references to tasks, so the coroutine can be garbage-collected between its await checkpoints. When that happens:

  • pending.set_result(...) never runs → every concurrent caller stuck on await pending hangs forever.
  • The finally: entry.pending_401.pop(key, None) cleanup never runs → subsequent 401s for the same access token latch onto the dead future, so the manager is poisoned for that key until process restart.

This is the exact pattern the adapter-side PRs are cleaning up (#11997 dingtalk, #11998 weixin, #12000 qqbot, #12001 rl_training, #12006 bluebubbles). mcp_oauth_manager.py was missed.

Fix

  • Add self._inflight_tasks: set[asyncio.Task] to MCPOAuthManager.__init__.
  • In handle_401, keep the task alive via self._inflight_tasks.add(task) and drop it on completion via task.add_done_callback(self._inflight_tasks.discard).

Test plan

  • New test_handle_401_tracks_inflight_task_to_prevent_gc — asserts the task is added to _inflight_tasks while running and removed via the done-callback once finished.
  • New test_handle_401_dedup_survives_even_if_task_reference_dropped — fans out 8 concurrent callers onto the same dedup'd handler, forces gc.collect() between scheduling and completion, and verifies all callers resolve within a 5s timeout. Without the fix this test can intermittently hang.
  • python3 -m py_compile tools/mcp_oauth_manager.py tests/tools/test_mcp_oauth_manager.py

Why invalidate_if_disk_changed wasn't affected

It runs on the caller's stack (directly await-ed from HermesMCPOAuthProvider.async_auth_flow), so no orphaned task can exist. Only the create_task path in handle_401 has this exposure.

`handle_401` spawned a dedup'd recovery coroutine via
`asyncio.create_task(_do_handle())` and discarded the returned task
reference. Python's event loop only keeps weak references to tasks, so
the coroutine could be garbage-collected before it called
`pending.set_result(...)`. Every concurrent caller awaiting that future
then hangs forever, and the `finally: entry.pending_401.pop(...)`
cleanup never runs — so subsequent 401s for the same key latch onto the
dead future too. Same pattern the adapter-side fixes address (NousResearch#11997,
NousResearch#11998, NousResearch#12000, NousResearch#12001, NousResearch#12006).

Hold the task in a process-wide set on the manager and discard it via
`add_done_callback` once it completes. Regression test covers both the
structural invariant (task tracked, then removed on completion) and a
concurrent dedup path with a forced `gc.collect()` between the handler's
await points.
@alt-glitch alt-glitch added type/bug Something isn't working P1 High — major feature broken, no workaround tool/mcp MCP client and OAuth area/auth Authentication, OAuth, credential pools labels Apr 23, 2026
@Bartok9

Bartok9 commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Salvaged onto current main as #51757 with full credit to you, @haileymarshall — the GC-eligible-task defect is still live (the call site moved to tools/mcp_oauth_manager.py:575 but is unchanged). I re-targeted your _inflight_tasks fix and carried your two regression tests (both fail without the fix, pass with it). Happy to defer if you would rather refresh this branch yourself. 🙏

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jun 29, 2026
@teknium1

Copy link
Copy Markdown
Collaborator

Merged via PR #55952. Your fix commit was cherry-picked onto current main with your authorship preserved in git log (commit 9f22f36). I added a small test follow-up on top: the two new inflight-task tests asserted the discard done-callback had already run by the time handle_401 returned, but that callback fires on a later event-loop tick via call_soon, so I added an await asyncio.sleep(0) before the cleanup assertions. Thanks for catching the GC-anchor gap in mcp_oauth_manager.py — same class as the adapter PRs. #55952

@teknium1 teknium1 closed this Jun 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools P1 High — major feature broken, no workaround sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages tool/mcp MCP client and OAuth type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants